allow to change display names in the user settings again

keep display name and email address in sync with the accounts table

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

3 files changed, 87 insertions, 110 deletions

diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php
index 6791475ce52..6bbf8786520 100644
--- a/settings/Controller/UsersController.php
+++ b/settings/Controller/UsersController.php
@@ -32,6 +32,7 @@ namespace OC\Settings\Controller;
 
 use OC\Accounts\AccountManager;
 use OC\AppFramework\Http;
+use OC\ForbiddenException;
 use OC\User\User;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Controller;
@@ -48,6 +49,7 @@ use OCP\IUserManager;
 use OCP\IUserSession;
 use OCP\Mail\IMailer;
 use OCP\IAvatarManager;
+use Punic\Exception;
 
 /**
  * @package OC\Settings\Controller
@@ -61,7 +63,7 @@ class UsersController extends Controller {
 	private $isAdmin;
 	/** @var IUserManager */
 	private $userManager;
-	/** @var IGroupManager */
+	/** @var \OC\Group\Manager */
 	private $groupManager;
 	/** @var IConfig */
 	private $config;
@@ -506,7 +508,6 @@ class UsersController extends Controller {
 	 * @NoSubadminRequired
 	 * @PasswordConfirmationRequired
 	 *
-	 * @param string $userId
 	 * @param string $avatarScope
 	 * @param string $displayname
 	 * @param string $displaynameScope
@@ -520,17 +521,29 @@ class UsersController extends Controller {
 	 * @param string $addressScope
 	 * @return DataResponse
 	 */
-	public function saveUserSettings($userId, $avatarScope,
-					$displayname, $displaynameScope,
-					$phone, $phoneScope,
-					$email, $emailScope,
-					$website, $websiteScope,
-					$address, $addressScope) {
-
-		if($userId === null) {
-			$userId = $this->userSession->getUser()->getUID();
+	public function setUserSettings($avatarScope,
+									 $displayname, $displaynameScope,
+									 $phone, $phoneScope,
+									 $email, $emailScope,
+									 $website, $websiteScope,
+									 $address, $addressScope
+	) {
+
+
+		if(!$this->mailer->validateMailAddress($email)) {
+			return new DataResponse(
+				array(
+					'status' => 'error',
+					'data' => array(
+						'message' => (string)$this->l10n->t('Invalid mail address')
+					)
+				),
+				Http::STATUS_UNPROCESSABLE_ENTITY
+			);
 		}
 
+		$userId = $this->userSession->getUser()->getUID();
+
 		$data = [
 			'avatar' =>  ['scope' => $avatarScope],
 			'displayName' => ['value' => $displayname, 'scope' => $displaynameScope],
@@ -542,106 +555,66 @@ class UsersController extends Controller {
 
 		$this->accountManager->updateUser($userId, $data);
 
-		return new DataResponse(
-			array(
-				'status' => 'success',
-				'data' => array(
-					'userId' => $userId,
-					'avatarScope' => $avatarScope,
-					'displayname' => $displayname,
-					'displaynameScope' => 'public', // force value for test purposes
-					'email' => $email,
-					'emailScope' => $emailScope,
-					'website' => $website,
-					'websiteScope' => $websiteScope,
-					'address' => $address,
-					'addressScope' => $addressScope,
-					'message' => (string)$this->l10n->t('Settings saved')
-				)
-			),
-			Http::STATUS_OK
-		);
-	}
-
-	/**
-	 * Set the mail address of a user
-	 *
-	 * @todo Merge into saveUserSettings
-	 *
-	 * @param string $id
-	 * @param string $mailAddress
-	 * @return DataResponse
-	 */
-	private function setMailAddress($id, $mailAddress) {
-		$userId = $this->userSession->getUser()->getUID();
-		$user = $this->userManager->get($id);
-
-		if($userId !== $id
-			&& !$this->isAdmin
-			&& !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)) {
+		try {
+			$this->saveUserSettings($userId, $data);
 			return new DataResponse(
 				array(
-					'status' => 'error',
+					'status' => 'success',
 					'data' => array(
-						'message' => (string)$this->l10n->t('Forbidden')
+						'userId' => $userId,
+						'avatarScope' => $avatarScope,
+						'displayname' => $displayname,
+						'displaynameScope' => $displaynameScope,
+						'email' => $email,
+						'emailScope' => $emailScope,
+						'website' => $website,
+						'websiteScope' => $websiteScope,
+						'address' => $address,
+						'addressScope' => $addressScope,
+						'message' => (string)$this->l10n->t('Settings saved')
 					)
 				),
-				Http::STATUS_FORBIDDEN
+				Http::STATUS_OK
 			);
+		} catch (ForbiddenException $e) {
+			return new DataResponse([
+				'status' => 'error',
+				'data' => [
+					'message' => $e->getMessage()
+				],
+			]);
 		}
 
-		if($mailAddress !== '' && !$this->mailer->validateMailAddress($mailAddress)) {
-			return new DataResponse(
-				array(
-					'status' => 'error',
-					'data' => array(
-						'message' => (string)$this->l10n->t('Invalid mail address')
-					)
-				),
-				Http::STATUS_UNPROCESSABLE_ENTITY
-			);
-		}
+	}
 
-		if(!$user){
-			return new DataResponse(
-				array(
-					'status' => 'error',
-					'data' => array(
-						'message' => (string)$this->l10n->t('Invalid user')
-					)
-				),
-				Http::STATUS_UNPROCESSABLE_ENTITY
-			);
-		}
 
-		// this is the only permission a backend provides and is also used
-		// for the permission of setting a email address
-		if(!$user->canChangeDisplayName()){
-			return new DataResponse(
-				array(
-					'status' => 'error',
-					'data' => array(
-						'message' => (string)$this->l10n->t('Unable to change mail address')
-					)
-				),
-				Http::STATUS_FORBIDDEN
-			);
+	/**
+	 * update account manager with new user data
+	 *
+	 * @param string $userId
+	 * @param array $data
+	 * @throws ForbiddenException
+	 */
+	private function saveUserSettings($userId, $data) {
+		$user = $this->userManager->get($userId);
+
+		// keep the user back-end up-to-date with the latest display name and email
+		// address
+		if (isset($data['displayName']['value'])  && $user->getDisplayName() !== $data['displayName']['value']) {
+			$result = $user->setDisplayName($data['displayName']['value']);
+			if ($result === false) {
+				throw new ForbiddenException($this->l10n->t('Unable to change full name'));
+			}
 		}
 
-		// delete user value if email address is empty
-		$user->setEMailAddress($mailAddress);
+		if (isset($data['email'][0]['value']) && $user->getEMailAddress() !== $data['email'][0]['value']) {
+			$result = $user->setEMailAddress($data['email'][0]['value']);
+			if ($result === false) {
+				throw new ForbiddenException($this->l10n->t('Unable to change mail address'));
+			}
+		}
 
-		return new DataResponse(
-			array(
-				'status' => 'success',
-				'data' => array(
-					'username' => $id,
-					'mailAddress' => $mailAddress,
-					'message' => (string)$this->l10n->t('Email saved')
-				)
-			),
-			Http::STATUS_OK
-		);
+		$this->accountManager->updateUser($userId, $data);
 	}
 
 	/**
@@ -694,13 +667,8 @@ class UsersController extends Controller {
 	 * @param string $displayName
 	 * @return DataResponse
 	 */
-	private function setDisplayName($username, $displayName) {
+	public function setDisplayName($username, $displayName) {
 		$currentUser = $this->userSession->getUser();
-
-		if ($username === null) {
-			$username = $currentUser->getUID();
-		}
-
 		$user = $this->userManager->get($username);
 
 		if ($user === null ||
@@ -708,8 +676,10 @@ class UsersController extends Controller {
 			(
 				!$this->groupManager->isAdmin($currentUser->getUID()) &&
 				!$this->groupManager->getSubAdmin()->isUserAccessible($currentUser, $user) &&
-				$currentUser !== $user)
-			) {
+				$currentUser->getUID() !== $username
+
+			)
+		) {
 			return new DataResponse([
 				'status' => 'error',
 				'data' => [
@@ -718,7 +688,12 @@ class UsersController extends Controller {
 			]);
 		}
 
-		if ($user->setDisplayName($displayName)) {
+		$userData = $this->accountManager->getUser($user->getUID());
+		$userData['displayName']['value'] = $displayName;
+
+
+		try {
+			$this->saveUserSettings($username, $userData);
 			return new DataResponse([
 				'status' => 'success',
 				'data' => [
@@ -727,11 +702,11 @@ class UsersController extends Controller {
 					'displayName' => $displayName,
 				],
 			]);
-		} else {
+		} catch (ForbiddenException $e) {
 			return new DataResponse([
 				'status' => 'error',
 				'data' => [
-					'message' => $this->l10n->t('Unable to change full name'),
+					'message' => $e->getMessage(),
 					'displayName' => $user->getDisplayName(),
 				],
 			]);
diff --git a/settings/routes.php b/settings/routes.php
index d4286cd56ff..62cfc398fdc 100644
--- a/settings/routes.php
+++ b/settings/routes.php
@@ -50,7 +50,8 @@ $application->registerRoutes($this, [
 		['name' => 'AppSettings#viewApps', 'url' => '/settings/apps', 'verb' => 'GET'],
 		['name' => 'AppSettings#listApps', 'url' => '/settings/apps/list', 'verb' => 'GET'],
 		['name' => 'SecuritySettings#trustedDomains', 'url' => '/settings/admin/security/trustedDomains', 'verb' => 'POST'],
-		['name' => 'Users#saveUserSettings', 'url' => '/settings/users/{username}/settings', 'verb' => 'PUT'],
+		['name' => 'Users#setDisplayName', 'url' => '/settings/users/{username}/displayName', 'verb' => 'POST'],
+		['name' => 'Users#setUserSettings', 'url' => '/settings/users/{username}/settings', 'verb' => 'PUT'],
 		['name' => 'Users#stats', 'url' => '/settings/users/stats', 'verb' => 'GET'],
 		['name' => 'LogSettings#setLogLevel', 'url' => '/settings/admin/log/level', 'verb' => 'POST'],
 		['name' => 'LogSettings#getEntries', 'url' => '/settings/admin/log/entries', 'verb' => 'GET'],
diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php
index 03c3a2e2ab4..e2b13b24b8c 100644
--- a/tests/Settings/Controller/UsersControllerTest.php
+++ b/tests/Settings/Controller/UsersControllerTest.php
@@ -2040,6 +2040,7 @@ class UsersControllerTest extends \Test\TestCase {
 			->expects($this->once())
 			->method('getUser')
 			->willReturn($user);
+
 		$this->userManager
 			->expects($this->once())
 			->method('get')