bring back setEmailAddress for the user management

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

lib/base.php

@@ -870,7 +870,7 @@ class OC {
 	}
 
 	private static function registerAccountHooks() {
-		$hookHandler = new \OC\Accounts\Hooks();
+		$hookHandler = new \OC\Accounts\Hooks(\OC::$server->getLogger());
 		\OCP\Util::connectHook('OC_User', 'changeUser', $hookHandler, 'changeUserHook');
 	}
 

lib/private/Accounts/AccountManager.php

@@ -23,7 +23,6 @@
 
 namespace OC\Accounts;
 
-use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\IUser;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
@@ -63,9 +62,6 @@ class AccountManager {
 	/** @var EventDispatcherInterface */
 	private $eventDispatcher;
 
-	/** @var IConfig */
-	private $config;
-
 	/**
 	 * AccountManager constructor.
 	 *

lib/private/Accounts/Hooks.php

@@ -22,6 +22,7 @@
 
 namespace OC\Accounts;
 
+use OCP\ILogger;
 use OCP\IUser;
 
 class Hooks {
@@ -29,6 +30,18 @@ class Hooks {
 	/** @var  AccountManager */
 	private $accountManager = null;
 
+	/** @var ILogger */
+	private $logger;
+
+	/**
+	 * Hooks constructor.
+	 *
+	 * @param ILogger $logger
+	 */
+	public function __construct(ILogger $logger) {
+		$this->logger = $logger;
+	}
+
 	/**
 	 * update accounts table if email address or display name was changed from outside
 	 *
@@ -36,25 +49,31 @@ class Hooks {
 	 */
 	public function changeUserHook($params) {
 
-		$this->instantiateAccountManager();
+		$accountManager = $this->getAccountManager();
 
 		/** @var IUser $user */
-		$user = $params['user'];
-		$feature = $params['feature'];
-		$newValue = $params['value'];
-		$accountData = $this->accountManager->getUser($user);
+		$user = isset($params['user']) ? $params['user'] : null;
+		$feature = isset($params['feature']) ? $params['feature'] : null;
+		$newValue = isset($params['value']) ? $params['value'] : null;
+
+		if (is_null($user) || is_null($feature) || is_null($newValue)) {
+			$this->logger->warning('Missing expected parameters in change user hook');
+			return;
+		}
+
+		$accountData = $accountManager->getUser($user);
 
 		switch ($feature) {
 			case 'eMailAddress':
 				if ($accountData[AccountManager::PROPERTY_EMAIL]['value'] !== $newValue) {
 					$accountData[AccountManager::PROPERTY_EMAIL]['value'] = $newValue;
-					$this->accountManager->updateUser($user, $accountData);
+					$accountManager->updateUser($user, $accountData);
 				}
 				break;
 			case 'displayName':
 				if ($accountData[AccountManager::PROPERTY_DISPLAYNAME]['value'] !== $newValue) {
 					$accountData[AccountManager::PROPERTY_DISPLAYNAME]['value'] = $newValue;
-					$this->accountManager->updateUser($user, $accountData);
+					$accountManager->updateUser($user, $accountData);
 				}
 				break;
 		}
@@ -66,13 +85,14 @@ class Hooks {
 	 *
 	 * @return AccountManager
 	 */
-	protected function instantiateAccountManager() {
+	protected function getAccountManager() {
 		if (is_null($this->accountManager)) {
 			$this->accountManager = new AccountManager(
 				\OC::$server->getDatabaseConnection(),
 				\OC::$server->getEventDispatcher()
 			);
 		}
+		return $this->accountManager;
 	}
 
 }

settings/Controller/UsersController.php

@@ -618,10 +618,12 @@ class UsersController extends Controller {
 		if (isset($data[AccountManager::PROPERTY_EMAIL]['value'])
 			&& $oldEmailAddress !== $data[AccountManager::PROPERTY_EMAIL]['value']
 		) {
-			$result = $user->setEMailAddress($data[AccountManager::PROPERTY_EMAIL]['value']);
-			if ($result === false) {
-				throw new ForbiddenException($this->l10n->t('Unable to change mail address'));
+			// this is the only permission a backend provides and is also used
+			// for the permission of setting a email address
+			if (!$user->canChangeDisplayName()) {
+				throw new ForbiddenException($this->l10n->t('Unable to change email address'));
 			}
+			$user->setEMailAddress($data[AccountManager::PROPERTY_EMAIL]['value']);
 		}
 
 		$this->accountManager->updateUser($user, $data);
@@ -722,4 +724,95 @@ class UsersController extends Controller {
 			]);
 		}
 	}
+
+	/**
+	 * Set the mail address of a user
+	 *
+	 * @NoAdminRequired
+	 * @NoSubadminRequired
+	 * @PasswordConfirmationRequired
+	 *
+	 * @param string $id
+	 * @param string $mailAddress
+	 * @return DataResponse
+	 */
+	public function setEMailAddress($id, $mailAddress) {
+		$user = $this->userManager->get($id);
+		if (!$this->isAdmin
+			&& !$this->groupManager->getSubAdmin()->isUserAccessible($this->userSession->getUser(), $user)
+		) {
+			return new DataResponse(
+				array(
+					'status' => 'error',
+					'data' => array(
+						'message' => (string)$this->l10n->t('Forbidden')
+					)
+				),
+				Http::STATUS_FORBIDDEN
+			);
+		}
+
+		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
+			);
+		}
+
+		$userData = $this->accountManager->getUser($user);
+		$userData[AccountManager::PROPERTY_EMAIL]['value'] = $mailAddress;
+
+		try {
+			$this->saveUserSettings($user, $userData);
+			return new DataResponse(
+				array(
+					'status' => 'success',
+					'data' => array(
+						'username' => $id,
+						'mailAddress' => $mailAddress,
+						'message' => (string)$this->l10n->t('Email saved')
+					)
+				),
+				Http::STATUS_OK
+			);
+		} catch (ForbiddenException $e) {
+			return new DataResponse([
+				'status' => 'error',
+				'data' => [
+					'message' => $e->getMessage()
+				],
+			]);
+		}
+	}
+
 }

settings/routes.php

@@ -51,6 +51,7 @@ $application->registerRoutes($this, [
 		['name' => 'AppSettings#listApps', 'url' => '/settings/apps/list', 'verb' => 'GET'],
 		['name' => 'SecuritySettings#trustedDomains', 'url' => '/settings/admin/security/trustedDomains', 'verb' => 'POST'],
 		['name' => 'Users#setDisplayName', 'url' => '/settings/users/{username}/displayName', 'verb' => 'POST'],
+		['name' => 'Users#setEMailAddress', 'url' => '/settings/users/{id}/mailAddress', 'verb' => 'PUT'],
 		['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'],

tests/Settings/Controller/UsersControllerTest.php

@@ -11,7 +11,6 @@
 namespace Tests\Settings\Controller;
 
 use OC\Accounts\AccountManager;
-use OC\ForbiddenException;
 use OC\Group\Manager;
 use OC\Settings\Controller\UsersController;
 use OCP\App\IAppManager;
@@ -2045,7 +2044,7 @@ class UsersControllerTest extends \Test\TestCase {
 	 */
 	public function testSetUserSettings($email, $validEmail, $expectedStatus) {
 		$controller = $this->getController(false, ['saveUserSettings']);
-		$user = $this->getMock(IUser::class);
+		$user = $this->createMock(IUser::class);
 
 		$this->userSession->method('getUser')->willReturn($user);
 
@@ -2102,10 +2101,11 @@ class UsersControllerTest extends \Test\TestCase {
 										 $oldDisplayName
 	) {
 		$controller = $this->getController();
-		$user = $this->getMock(IUser::class);
+		$user = $this->createMock(IUser::class);
 
 		$user->method('getDisplayName')->willReturn($oldDisplayName);
 		$user->method('getEMailAddress')->willReturn($oldEmailAddress);
+		$user->method('canChangeDisplayName')->willReturn(true);
 
 		if ($data[AccountManager::PROPERTY_EMAIL]['value'] !== $oldEmailAddress) {
 			$user->expects($this->once())->method('setEMailAddress')
@@ -2174,7 +2174,7 @@ class UsersControllerTest extends \Test\TestCase {
 	 * @param string $oldEmailAddress
 	 * @param string $oldDisplayName
 	 * @param bool $setDisplayNameResult
-	 * @param bool $setEmailResult
+	 * @param bool $canChangeEmail
 	 *
 	 * @expectedException \OC\ForbiddenException
 	 */
@@ -2182,18 +2182,17 @@ class UsersControllerTest extends \Test\TestCase {
 												  $oldEmailAddress,
 												  $oldDisplayName,
 												  $setDisplayNameResult,
-												  $setEmailResult
+												  $canChangeEmail
 	) {
 		$controller = $this->getController();
-		$user = $this->getMock(IUser::class);
+		$user = $this->createMock(IUser::class);
 
 		$user->method('getDisplayName')->willReturn($oldDisplayName);
 		$user->method('getEMailAddress')->willReturn($oldEmailAddress);
 
 		if ($data[AccountManager::PROPERTY_EMAIL]['value'] !== $oldEmailAddress) {
-			$user->method('setEMailAddress')
-				->with($data[AccountManager::PROPERTY_EMAIL]['value'])
-				->willReturn($setEmailResult);
+			$user->method('canChangeDisplayName')
+				->willReturn($canChangeEmail);
 		}
 
 		if ($data[AccountManager::PROPERTY_DISPLAYNAME]['value'] !== $oldDisplayName) {
@@ -2241,4 +2240,57 @@ class UsersControllerTest extends \Test\TestCase {
 
 		];
 	}
+
+	/**
+	 * @return array
+	 */
+	public function setEmailAddressData() {
+		return [
+			/* mailAddress,    isValid, expectsUpdate, canChangeDisplayName, responseCode */
+			[ '',              true,    true,          true,                 Http::STATUS_OK ],
+			[ 'foo@local',     true,    true,          true,                 Http::STATUS_OK],
+			[ 'foo@bar@local', false,   false,         true,                 Http::STATUS_UNPROCESSABLE_ENTITY],
+			[ 'foo@local',     true,    false,         false,                Http::STATUS_FORBIDDEN],
+		];
+	}
+	/**
+	 * @dataProvider setEmailAddressData
+	 *
+	 */
+	public function testSetEMailAddress($mailAddress, $isValid, $expectsUpdate, $canChangeDisplayName, $responseCode) {
+		$user = $this->getMockBuilder('\OC\User\User')
+			->disableOriginalConstructor()->getMock();
+		$user
+			->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue('foo'));
+		$user
+			->expects($this->any())
+			->method('canChangeDisplayName')
+			->will($this->returnValue($canChangeDisplayName));
+		$user
+			->expects($expectsUpdate ? $this->once() : $this->never())
+			->method('setEMailAddress')
+			->with(
+				$this->equalTo($mailAddress)
+			);
+		$this->mailer
+			->expects($this->any())
+			->method('validateMailAddress')
+			->with($mailAddress)
+			->willReturn($isValid);
+		if ($isValid) {
+			$user->expects($this->atLeastOnce())
+				->method('canChangeDisplayName')
+				->willReturn(true);
+			$this->userManager
+				->expects($this->atLeastOnce())
+				->method('get')
+				->with('foo')
+				->will($this->returnValue($user));
+		}
+		$controller = $this->getController(true);
+		$response = $controller->setEMailAddress($user->getUID(), $mailAddress);
+		$this->assertSame($responseCode, $response->getStatus());
+	}
 }

tests/lib/Accounts/HooksTest.php

@@ -0,0 +1,157 @@
+<?php
+/**
+ * @copyright Copyright (c) 2016 Bjoern Schiessle <bjoern@schiessle.org>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * 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
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+
+namespace Test\Accounts;
+
+
+use OC\Accounts\AccountManager;
+use OC\Accounts\Hooks;
+use OCP\ILogger;
+use OCP\IUser;
+use Test\TestCase;
+
+/**
+ * Class HooksTest
+ *
+ * @package Test\Accounts
+ * @group DB
+ */
+class HooksTest extends TestCase  {
+
+	/** @var  ILogger | \PHPUnit_Framework_MockObject_MockObject */
+	private $logger;
+
+	/** @var  AccountManager | \PHPUnit_Framework_MockObject_MockObject */
+	private $accountManager;
+
+	/** @var  Hooks | \PHPUnit_Framework_MockObject_MockObject */
+	private $hooks;
+
+	public function setUp() {
+		parent::setUp();
+
+		$this->logger = $this->createMock(ILogger::class);
+		$this->accountManager = $this->getMockBuilder(AccountManager::class)
+			->disableOriginalConstructor()->getMock();
+
+		$this->hooks = $this->getMockBuilder(Hooks::class)
+			->setConstructorArgs([$this->logger])
+			->setMethods(['getAccountManager'])
+			->getMock();
+
+		$this->hooks->method('getAccountManager')->willReturn($this->accountManager);
+	}
+
+	/**
+	 * @dataProvider dataTestChangeUserHook
+	 *
+	 * @param $params
+	 * @param $data
+	 * @param $setEmail
+	 * @param $setDisplayName
+	 * @param $error
+	 */
+	public function testChangeUserHook($params, $data, $setEmail, $setDisplayName, $error) {
+		if ($error) {
+			$this->accountManager->expects($this->never())->method('getUser');
+			$this->accountManager->expects($this->never())->method('updateUser');
+		} else {
+			$this->accountManager->expects($this->once())->method('getUser')->willReturn($data);
+			$newData = $data;
+			if ($setEmail) {
+				$newData[AccountManager::PROPERTY_EMAIL]['value'] = $params['value'];
+				$this->accountManager->expects($this->once())->method('updateUser')
+					->with($params['user'], $newData);
+			} elseif ($setDisplayName) {
+				$newData[AccountManager::PROPERTY_DISPLAYNAME]['value'] = $params['value'];
+				$this->accountManager->expects($this->once())->method('updateUser')
+					->with($params['user'], $newData);
+			} else {
+				$this->accountManager->expects($this->never())->method('updateUser');
+			}
+		}
+
+		$this->hooks->changeUserHook($params);
+
+	}
+
+	public function dataTestChangeUserHook() {
+		$user = $this->createMock(IUser::class);
+		return [
+			[
+				['feature' => '', 'value' => ''],
+				[
+					AccountManager::PROPERTY_EMAIL => ['value' => ''],
+					AccountManager::PROPERTY_DISPLAYNAME => ['value' => '']
+				],
+				false, false, true
+			],
+			[
+				['user' => $user, 'value' => ''],
+				[
+					AccountManager::PROPERTY_EMAIL => ['value' => ''],
+					AccountManager::PROPERTY_DISPLAYNAME => ['value' => '']
+				],
+				false, false, true
+			],
+			[
+				['user' => $user, 'feature' => ''],
+				[
+					AccountManager::PROPERTY_EMAIL => ['value' => ''],
+					AccountManager::PROPERTY_DISPLAYNAME => ['value' => '']
+				],
+				false, false, true
+			],
+			[
+				['user' => $user, 'feature' => 'foo', 'value' => 'bar'],
+				[
+					AccountManager::PROPERTY_EMAIL => ['value' => 'oldMail@example.com'],
+					AccountManager::PROPERTY_DISPLAYNAME => ['value' => 'oldDisplayName']
+				],
+				false, false, false
+			],
+			[
+				['user' => $user, 'feature' => 'eMailAddress', 'value' => 'newMail@example.com'],
+				[
+					AccountManager::PROPERTY_EMAIL => ['value' => 'oldMail@example.com'],
+					AccountManager::PROPERTY_DISPLAYNAME => ['value' => 'oldDisplayName']
+				],
+				true, false, false
+			],
+			[
+				['user' => $user, 'feature' => 'displayName', 'value' => 'newDisplayName'],
+				[
+					AccountManager::PROPERTY_EMAIL => ['value' => 'oldMail@example.com'],
+					AccountManager::PROPERTY_DISPLAYNAME => ['value' => 'oldDisplayName']
+				],
+				false, true, false
+			],
+		];
+	}
+
+	public function testGetAccountManager() {
+		$hooks = new Hooks($this->logger);
+		$result = $this->invokePrivate($hooks, 'getAccountManager');
+		$this->assertInstanceOf(AccountManager::class, $result);
+	}
+
+}