diff --git a/lib/base.php b/lib/base.php index cad9121da1..69ec566f00 100644 --- a/lib/base.php +++ b/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'); } diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 8c7cd010de..3701421a20 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/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. * diff --git a/lib/private/Accounts/Hooks.php b/lib/private/Accounts/Hooks.php index 187024e547..38e7e20485 100644 --- a/lib/private/Accounts/Hooks.php +++ b/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; } } diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php index 42d464c961..20440e6d39 100644 --- a/settings/Controller/UsersController.php +++ b/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() + ], + ]); + } + } + } diff --git a/settings/routes.php b/settings/routes.php index 62cfc398fd..3f034d363e 100644 --- a/settings/routes.php +++ b/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'], diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php index 9f381e3195..69082a7929 100644 --- a/tests/Settings/Controller/UsersControllerTest.php +++ b/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()); + } } diff --git a/tests/lib/Accounts/HooksTest.php b/tests/lib/Accounts/HooksTest.php new file mode 100644 index 0000000000..071e78146e --- /dev/null +++ b/tests/lib/Accounts/HooksTest.php @@ -0,0 +1,157 @@ + + * + * @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 . + * + */ + + +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); + } + +}