Merge pull request #39288 from /issues/35101-1

Jan · web-flow · commit 9d263e43682b · 2021-09-29T12:49:36.000+02:00
Decouple change email from 'allow_user_to_change_display_name' setting
diff --git a/changelog/unreleased/39288 b/changelog/unreleased/39288
@@ -0,0 +1,14 @@
+Bugfix: Decouple change email from 'allow_user_to_change_display_name' setting
+
+Before this change, with setting 'allow_user_to_change_display_name' false,
+the user was not able to change the mail address in Settings->Personal->General
+via the webUI.
+
+With this change, the setting 'allow_user_to_change_mail_address' has been
+introduced and change mail address has been decoupled from  setting
+`allow_user_to_change_display_name`.
+
+'allow_user_to_change_mail_address' must be set explicitly to false, to take effect.
+
+https://github.com/owncloud/core/pull/39288
+https://github.com/owncloud/core/issues/35103
diff --git a/config/config.sample.php b/config/config.sample.php
@@ -205,6 +205,13 @@
  */
 'allow_user_to_change_display_name' => true,
 
+/**
+ * Allow or disallow users to change their email addresses
+ * `true` allows users to change their email address (on their Personal pages),
+ * `false` prevents them from changing their email address.
+ */
+'allow_user_to_change_mail_address' => true,
+
 /**
  * Define the lifetime of the remember-login cookie
  * The remember-login cookie is set when the user clicks the `remember` checkbox
diff --git a/core/Migrations/Version20210928123126.php b/core/Migrations/Version20210928123126.php
@@ -0,0 +1,22 @@
+<?php
+namespace OC\Migrations;
+
+use OCP\Migration\ISimpleMigration;
+use OCP\Migration\IOutput;
+
+/**
+ * Set allow_user_to_change_mail_address equal to allow_user_to_change_display_name
+ * so the behaviour does not change.
+ */
+class Version20210928123126 implements ISimpleMigration {
+
+	/**
+	 * @param IOutput $out
+	 */
+	public function run(IOutput $out) {
+		$value = \OC::$server->getSystemConfig()->getValue('allow_user_to_change_display_name');
+		if ($value !== null) {
+			\OC::$server->getSystemConfig()->setValue('allow_user_to_change_mail_address', $value);
+		}
+	}
+}
diff --git a/lib/private/User/DeletedUser.php b/lib/private/User/DeletedUser.php
@@ -178,6 +178,10 @@ public function canChangeDisplayName() {
 		return false;
 	}
 
+	public function canChangeMailAddress() {
+		return false;
+	}
+
 	public function isEnabled() {
 		return false;
 	}
diff --git a/lib/private/User/RemoteUser.php b/lib/private/User/RemoteUser.php
@@ -141,6 +141,13 @@ public function canChangeDisplayName() {
 		return false;
 	}
 
+	/**
+	 * @inheritdoc
+	 */
+	public function canChangeMailAddress() {
+		return false;
+	}
+
 	/**
 	 * @inheritdoc
 	 */
diff --git a/lib/private/User/User.php b/lib/private/User/User.php
@@ -410,6 +410,26 @@ public function canChangeDisplayName() {
 		return $backend->implementsActions(Backend::SET_DISPLAYNAME);
 	}
 
+	/**
+	 * check if the backend supports changing email addresses
+	 *
+	 * @return bool
+	 */
+	public function canChangeMailAddress() {
+		if ($this->userSession instanceof IUserSession) {
+			$user = $this->userSession->getUser();
+			if (
+				($this->config->getSystemValue('allow_user_to_change_mail_address') === false) &&
+				(($user !== null) && (!$this->groupManager->isAdmin($user->getUID()))) &&
+				(($user !== null) && (!$this->groupManager->getSubAdmin()->isSubAdmin($user)))
+			) {
+				return false;
+			}
+		}
+
+		return true;
+	}
+
 	/**
 	 * check if the user is enabled
 	 *
diff --git a/lib/public/IUser.php b/lib/public/IUser.php
@@ -161,6 +161,14 @@ public function canChangePassword();
 	 */
 	public function canChangeDisplayName();
 
+	/**
+	 * check if the backend supports changing email addresses
+	 *
+	 * @return bool
+	 * @since 10.9.0
+	 */
+	public function canChangeMailAddress();
+
 	/**
 	 * check if the user is enabled
 	 *
diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php
@@ -929,7 +929,7 @@ public function setMailAddress($id, $mailAddress) {
 
 		// this is the only permission a backend provides and is also used
 		// for the permission of setting a email address
-		if (!$user->canChangeDisplayName()) {
+		if (!$user->canChangeMailAddress()) {
 			return new DataResponse(
 				[
 					'status' => 'error',
@@ -1062,7 +1062,7 @@ public function setDisplayName($username, $displayName) {
 			return new DataResponse([
 				'status' => 'error',
 				'data' => [
-					'message' => $this->l10n->t('Authentication error'),
+					'message' => $this->l10n->t('Unable to change display name'),
 				],
 			]);
 		}
diff --git a/settings/Panels/Personal/Profile.php b/settings/Panels/Personal/Profile.php
@@ -101,6 +101,7 @@ public function getPanel() {
 		$tmpl->assign('enableAvatars', $this->config->getSystemValue('enable_avatars', true) === true);
 		$tmpl->assign('avatarChangeSupported', $this->userSession->getUser()->canChangeAvatar());
 		$tmpl->assign('displayNameChangeSupported', $this->userSession->getUser()->canChangeDisplayName());
+		$tmpl->assign('mailAddressChangeSupported', $this->userSession->getUser()->canChangeMailAddress());
 		$tmpl->assign('passwordChangeSupported', $this->userSession->getUser()->canChangePassword());
 		$groups = $this->groupManager->getUserGroupIds($this->userSession->getUser());
 		\sort($groups);
diff --git a/settings/css/settings.css b/settings/css/settings.css
@@ -50,7 +50,7 @@ input#webdav {
 #displaynameform,
 #groups,
 #language,
-#lostpassword,
+#emailform,
 #passwordform {
 	display: inline-block;
 	margin-bottom: 0;
diff --git a/settings/js/panels/profile.js b/settings/js/panels/profile.js
@@ -7,8 +7,8 @@ function changeEmailAddress () {
 		return;
 	}
 	emailInfo.defaultValue = emailInfo.val();
-	OC.msg.startSaving('#lostpassword .msg');
-	var post = $("#lostpassword").serializeArray();
+	OC.msg.startSaving('#emailform .msg');
+	var post = $("#emailform").serializeArray();
 	$.ajax({
 		type: 'PUT',
 		url: OC.generateUrl('/settings/users/{id}/mailAddress', {id: OC.currentUser}),
@@ -19,9 +19,9 @@ function changeEmailAddress () {
 		// I know the following 4 lines look weird, but that is how it works
 		// in jQuery -  for success the first parameter is the result
 		//              for failure the first parameter is the result object
-		OC.msg.finishedSaving('#lostpassword .msg', result);
+		OC.msg.finishedSaving('#emailform .msg', result);
 	}).fail(function(result){
-		OC.msg.finishedSaving('#lostpassword .msg', result.responseJSON);
+		OC.msg.finishedSaving('#emailform .msg', result.responseJSON);
 	});
 }
 
diff --git a/settings/templates/panels/personal/profile.php b/settings/templates/panels/personal/profile.php
@@ -59,9 +59,9 @@
 ?>
 
 <?php
-if ($_['displayNameChangeSupported']) {
+if ($_['mailAddressChangeSupported']) {
 	?>
-<form id="lostpassword" class="section" autocapitalize="none">
+<form id="emailform" class="section" autocapitalize="none">
 	<h2>
 		<label for="email"><?php p($l->t('Email')); ?></label>
 	</h2>
@@ -79,7 +79,7 @@
 <?php
 } else {
 		?>
-<div id="lostpassword" class="section">
+<div id="emailform" class="section">
 	<h2><?php echo $l->t('Email'); ?></h2>
 	<span><?php if (isset($_['email'][0])) {
 			p($_['email']);
diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php
@@ -1960,7 +1960,7 @@ public function testSetEmailAddressSendEmail($id, $mailaddress) {
 
 		$iUser = $this->createMock(IUser::class);
 		$iUser->expects($this->once())
-			->method('canChangeDisplayName')
+			->method('canChangeMailAddress')
 			->willReturn(true);
 		$userManager->method('get')->willReturn($iUser);
 		$iUser->method('getUID')->willReturn($id);
@@ -2011,10 +2011,10 @@ public function setEmailAddressData() {
 	 * @param string $mailAddress
 	 * @param bool $isValid
 	 * @param bool $expectsUpdate
-	 * @param bool $canChangeDisplayName
+	 * @param bool $chanChangeMailAddress
 	 * @param bool $responseCode
 	 */
-	public function testSetEmailAddress($mailAddress, $isValid, $expectsUpdate, $canChangeDisplayName, $responseCode) {
+	public function testSetEmailAddress($mailAddress, $isValid, $expectsUpdate, $chanChangeMailAddress, $responseCode) {
 		$this->container['IsAdmin'] = true;
 
 		$user = $this->getMockBuilder('\OC\User\User')
@@ -2029,8 +2029,8 @@ public function testSetEmailAddress($mailAddress, $isValid, $expectsUpdate, $can
 			->will($this->returnValue('foo@local'));
 		$user
 			->expects($this->any())
-			->method('canChangeDisplayName')
-			->will($this->returnValue($canChangeDisplayName));
+			->method('canChangeMailAddress')
+			->will($this->returnValue($chanChangeMailAddress));
 		$user
 			->expects($this->any())
 			->method('setEMailAddress')
@@ -2050,7 +2050,7 @@ public function testSetEmailAddress($mailAddress, $isValid, $expectsUpdate, $can
 
 		if ($isValid) {
 			$user->expects($this->atLeastOnce())
-				->method('canChangeDisplayName')
+				->method('canChangeMailAddress')
 				->willReturn(true);
 		}
 
@@ -2195,7 +2195,7 @@ public function testSetDisplayNameNull() {
 			[
 				'status' => 'error',
 				'data' => [
-					'message' => 'Authentication error',
+					'message' => 'Unable to change display name',
 				],
 			]
 		);
@@ -2292,7 +2292,7 @@ public function testSetDisplayName($currentUser, $editUser, $isAdmin, $isSubAdmi
 				[
 					'status' => 'error',
 					'data' => [
-						'message' => 'Authentication error',
+						'message' => 'Unable to change display name',
 					],
 				]
 			);
diff --git a/tests/Settings/Panels/Personal/ProfileTest.php b/tests/Settings/Panels/Personal/ProfileTest.php
@@ -61,7 +61,7 @@ public function testGetPriority() {
 	public function testGetPanel() {
 		$user = $this->createMock(IUser::class);
 		$user->expects($this->once())->method('getEMailAddress')->will($this->returnValue('test@example.com'));
-		$this->userSession->expects($this->exactly(7))->method('getUser')->will($this->returnValue($user));
+		$this->userSession->expects($this->exactly(8))->method('getUser')->will($this->returnValue($user));
 		$this->groupManager->expects($this->once())->method('getUserGroupIds')->will($this->returnValue(['group1', 'group2']));
 		$this->lfactory->expects($this->once())->method('findLanguage')->will($this->returnValue('en'));
 		$this->config->expects($this->once())->method('getUserValue')->will($this->returnValue(''));
diff --git a/tests/acceptance/features/bootstrap/WebUIPersonalGeneralSettingsContext.php b/tests/acceptance/features/bootstrap/WebUIPersonalGeneralSettingsContext.php
@@ -219,6 +219,23 @@ public function theUserChangesTheEmailAddressToUsingTheWebUI($emailAddress) {
 		);
 	}
 
+	/**
+	 * @Then the user should not be able to change the email address using the webUI
+	 *
+	 * @return void
+	 * @throws Exception
+	 */
+	public function theUserShouldNotBeAbleToChangeTheEmailAddressUsingTheWebui() {
+		try {
+			$this->personalGeneralSettingsPage->changeEmailAddress(
+				"somebody@example.com",
+				$this->getSession()
+			);
+			Assert::fail("changing the email address was possible, but should not be");
+		} catch (SensioLabs\Behat\PageObjectExtension\PageObject\Exception\ElementNotFoundException $e) {
+		}
+	}
+
 	/**
 	 * @Then the owncloud version should be displayed on the personal general settings page on the webUI
 	 *
diff --git a/tests/acceptance/features/webUIPersonalSettings/accessToChangeEmailAddressThroughConfig.feature b/tests/acceptance/features/webUIPersonalSettings/accessToChangeEmailAddressThroughConfig.feature
@@ -0,0 +1,22 @@
+@webUI @insulated @disablePreviews
+Feature: Control access to edit email address of user through config file
+  As an admin
+  I want to control the access to users to edit their email address in the settings page
+  So that users can edit their email address after getting permission from the administrator
+
+  Background:
+    Given user "Alice" has been created with default attributes and without skeleton files
+    And user "Alice" has logged in using the webUI
+
+  Scenario: Admin gives access to users to change their email address
+    When the administrator updates system config key "allow_user_to_change_mail_address" with value "true" and type "boolean" using the occ command
+    And the user browses to the personal general settings page
+    And the user changes the email address to "new-address@owncloud.com" using the webUI
+    And the user follows the email change confirmation link received by "new-address@owncloud.com" using the webUI
+    Then the attributes of user "Alice" returned by the API should include
+      | email | new-address@owncloud.com |
+
+  Scenario: Admin does not give access to users to change their email address
+    When the administrator updates system config key "allow_user_to_change_mail_address" with value "false" and type "boolean" using the occ command
+    And the user browses to the personal general settings page
+    Then the user should not be able to change the email address using the webUI
diff --git a/tests/lib/User/DeletedUserTest.php b/tests/lib/User/DeletedUserTest.php
@@ -137,6 +137,10 @@ public function testCanChangeDisplayName() {
 		$this->assertFalse($this->deletedUser->canChangeDisplayName());
 	}
 
+	public function testCanChangeMailAddress() {
+		$this->assertFalse($this->deletedUser->canChangeMailAddress());
+	}
+
 	public function testIsEnabled() {
 		$this->assertFalse($this->deletedUser->isEnabled());
 	}
diff --git a/tests/lib/User/RemoteUserTest.php b/tests/lib/User/RemoteUserTest.php
@@ -0,0 +1,49 @@
+<?php
+
+/**
+ * @author Jannik Stehle <jstehle@owncloud.com>
+ * @author Jan Ackermann <jackermann@owncloud.com>
+ *
+ * @copyright Copyright (c) 2021, ownCloud GmbH
+ * @license AGPL-3.0
+ *
+ * This code is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace Test\User;
+
+use OC\User\RemoteUser;
+use OCP\IUser;
+use Test\TestCase;
+
+/**
+ *
+ * @package Test\User
+ */
+class RemoteUserTest extends TestCase {
+
+	/** @var RemoteUser */
+	private $remoteUser;
+
+	protected function setUp(): void {
+		$user = $this->createMock(IUser::class);
+		$user->method('getUID')->willReturn('testUser');
+
+		$this->remoteUser = new RemoteUser($user->getUID());
+	}
+
+	public function testCanChangeMailAddress() {
+		$this->assertFalse($this->remoteUser->canChangeMailAddress());
+	}
+}
diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php

Original file line number	Diff line number	Diff line change
`@@ -178,6 +178,10 @@ public function canChangeDisplayName() {`
`178`	`178`	`return false;`
`179`	`179`	`}`
`180`	`180`
	`181`	`+ public function canChangeMailAddress() {`
	`182`	`+ return false;`
	`183`	`+ }`
	`184`	`+`
`181`	`185`	`public function isEnabled() {`
`182`	`186`	`return false;`
`183`	`187`	`}`