From 7c48177830585b150d7044cb87cea8e5fc31d527 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 13 Aug 2021 15:53:17 +0200 Subject: [PATCH 1/8] move verification token logic out of lost password controller - to make it reusable - needed for local email verification Signed-off-by: Arthur Schiwon --- core/Controller/LostController.php | 112 ++----- lib/composer/composer/autoload_classmap.php | 3 + lib/composer/composer/autoload_static.php | 3 + .../VerificationToken/VerificationToken.php | 111 +++++++ .../VerificationToken/IVerificationToken.php | 55 ++++ .../InvalidTokenException.php | 74 +++++ tests/Core/Controller/LostControllerTest.php | 301 +++--------------- .../VerificationTokenTest.php | 272 ++++++++++++++++ 8 files changed, 591 insertions(+), 340 deletions(-) create mode 100644 lib/private/Security/VerificationToken/VerificationToken.php create mode 100644 lib/public/Security/VerificationToken/IVerificationToken.php create mode 100644 lib/public/Security/VerificationToken/InvalidTokenException.php create mode 100644 tests/lib/Security/VerificationToken/VerificationTokenTest.php diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index 99427132f5e44..7a36e5970a6af 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -43,7 +43,6 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\TemplateResponse; -use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\Encryption\IEncryptionModule; use OCP\Encryption\IManager; @@ -56,8 +55,8 @@ use OCP\IUser; use OCP\IUserManager; use OCP\Mail\IMailer; -use OCP\Security\ICrypto; -use OCP\Security\ISecureRandom; +use OCP\Security\VerificationToken\InvalidTokenException; +use OCP\Security\VerificationToken\IVerificationToken; use function reset; /** @@ -82,67 +81,46 @@ class LostController extends Controller { protected $encryptionManager; /** @var IConfig */ protected $config; - /** @var ISecureRandom */ - protected $secureRandom; /** @var IMailer */ protected $mailer; - /** @var ITimeFactory */ - protected $timeFactory; - /** @var ICrypto */ - protected $crypto; /** @var ILogger */ private $logger; /** @var Manager */ private $twoFactorManager; /** @var IInitialStateService */ private $initialStateService; - - /** - * @param string $appName - * @param IRequest $request - * @param IURLGenerator $urlGenerator - * @param IUserManager $userManager - * @param Defaults $defaults - * @param IL10N $l10n - * @param IConfig $config - * @param ISecureRandom $secureRandom - * @param string $defaultMailAddress - * @param IManager $encryptionManager - * @param IMailer $mailer - * @param ITimeFactory $timeFactory - * @param ICrypto $crypto - */ - public function __construct($appName, - IRequest $request, - IURLGenerator $urlGenerator, - IUserManager $userManager, - Defaults $defaults, - IL10N $l10n, - IConfig $config, - ISecureRandom $secureRandom, - $defaultMailAddress, - IManager $encryptionManager, - IMailer $mailer, - ITimeFactory $timeFactory, - ICrypto $crypto, - ILogger $logger, - Manager $twoFactorManager, - IInitialStateService $initialStateService) { + /** @var IVerificationToken */ + private $verificationToken; + + public function __construct( + $appName, + IRequest $request, + IURLGenerator $urlGenerator, + IUserManager $userManager, + Defaults $defaults, + IL10N $l10n, + IConfig $config, + $defaultMailAddress, + IManager $encryptionManager, + IMailer $mailer, + ILogger $logger, + Manager $twoFactorManager, + IInitialStateService $initialStateService, + IVerificationToken $verificationToken + ) { parent::__construct($appName, $request); $this->urlGenerator = $urlGenerator; $this->userManager = $userManager; $this->defaults = $defaults; $this->l10n = $l10n; - $this->secureRandom = $secureRandom; $this->from = $defaultMailAddress; $this->encryptionManager = $encryptionManager; $this->config = $config; $this->mailer = $mailer; - $this->timeFactory = $timeFactory; - $this->crypto = $crypto; $this->logger = $logger; $this->twoFactorManager = $twoFactorManager; $this->initialStateService = $initialStateService; + $this->verificationToken = $verificationToken; } /** @@ -192,36 +170,14 @@ public function resetform($token, $userId) { * @param string $userId * @throws \Exception */ - protected function checkPasswordResetToken($token, $userId) { - $user = $this->userManager->get($userId); - if ($user === null || !$user->isEnabled()) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); - } - - $encryptedToken = $this->config->getUserValue($userId, 'core', 'lostpassword', null); - if ($encryptedToken === null) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); - } - + protected function checkPasswordResetToken(string $token, string $userId): void { try { - $mailAddress = !is_null($user->getEMailAddress()) ? $user->getEMailAddress() : ''; - $decryptedToken = $this->crypto->decrypt($encryptedToken, $mailAddress.$this->config->getSystemValue('secret')); - } catch (\Exception $e) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); - } - - $splittedToken = explode(':', $decryptedToken); - if (count($splittedToken) !== 2) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); - } - - if ($splittedToken[0] < ($this->timeFactory->getTime() - 60 * 60 * 24 * 7) || - $user->getLastLogin() > $splittedToken[0]) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is expired')); - } - - if (!hash_equals($splittedToken[1], $token)) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); + $this->verificationToken->check($token, $this->userManager->get($userId), 'lostpassword'); + } catch (InvalidTokenException $e) { + $error = $e->getCode() === InvalidTokenException::TOKEN_EXPIRED + ? $this->l10n->t('Could not reset password because the token is expired') + : $this->l10n->t('Could not reset password because the token is invalid'); + throw new \Exception($error, (int)$e->getCode(), $e); } } @@ -343,15 +299,7 @@ protected function sendEmail($input) { // secret being the users' email address appended with the system secret. // This makes the token automatically invalidate once the user changes // their email address. - $token = $this->secureRandom->generate( - 21, - ISecureRandom::CHAR_DIGITS. - ISecureRandom::CHAR_LOWER. - ISecureRandom::CHAR_UPPER - ); - $tokenValue = $this->timeFactory->getTime() .':'. $token; - $encryptedValue = $this->crypto->encrypt($tokenValue, $email . $this->config->getSystemValue('secret')); - $this->config->setUserValue($user->getUID(), 'core', 'lostpassword', $encryptedValue); + $token = $this->verificationToken->create($user, 'lostpassword', $email); $link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', ['userId' => $user->getUID(), 'token' => $token]); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 55d02c2feeb3a..b48b67104cf43 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -487,6 +487,8 @@ 'OCP\\Security\\ICrypto' => $baseDir . '/lib/public/Security/ICrypto.php', 'OCP\\Security\\IHasher' => $baseDir . '/lib/public/Security/IHasher.php', 'OCP\\Security\\ISecureRandom' => $baseDir . '/lib/public/Security/ISecureRandom.php', + 'OCP\\Security\\VerificationToken\\IVerificationToken' => $baseDir . '/lib/public/Security/VerificationToken/IVerificationToken.php', + 'OCP\\Security\\VerificationToken\\InvalidTokenException' => $baseDir . '/lib/public/Security/VerificationToken/InvalidTokenException.php', 'OCP\\Session\\Exceptions\\SessionNotAvailableException' => $baseDir . '/lib/public/Session/Exceptions/SessionNotAvailableException.php', 'OCP\\Settings\\IIconSection' => $baseDir . '/lib/public/Settings/IIconSection.php', 'OCP\\Settings\\IManager' => $baseDir . '/lib/public/Settings/IManager.php', @@ -1371,6 +1373,7 @@ 'OC\\Security\\RateLimiting\\Limiter' => $baseDir . '/lib/private/Security/RateLimiting/Limiter.php', 'OC\\Security\\SecureRandom' => $baseDir . '/lib/private/Security/SecureRandom.php', 'OC\\Security\\TrustedDomainHelper' => $baseDir . '/lib/private/Security/TrustedDomainHelper.php', + 'OC\\Security\\VerificationToken\\VerificationToken' => $baseDir . '/lib/private/Security/VerificationToken/VerificationToken.php', 'OC\\Server' => $baseDir . '/lib/private/Server.php', 'OC\\ServerContainer' => $baseDir . '/lib/private/ServerContainer.php', 'OC\\ServerNotAvailableException' => $baseDir . '/lib/private/ServerNotAvailableException.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 6f2bb064fc02b..e89d03d5e1303 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -516,6 +516,8 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OCP\\Security\\ICrypto' => __DIR__ . '/../../..' . '/lib/public/Security/ICrypto.php', 'OCP\\Security\\IHasher' => __DIR__ . '/../../..' . '/lib/public/Security/IHasher.php', 'OCP\\Security\\ISecureRandom' => __DIR__ . '/../../..' . '/lib/public/Security/ISecureRandom.php', + 'OCP\\Security\\VerificationToken\\IVerificationToken' => __DIR__ . '/../../..' . '/lib/public/Security/VerificationToken/IVerificationToken.php', + 'OCP\\Security\\VerificationToken\\InvalidTokenException' => __DIR__ . '/../../..' . '/lib/public/Security/VerificationToken/InvalidTokenException.php', 'OCP\\Session\\Exceptions\\SessionNotAvailableException' => __DIR__ . '/../../..' . '/lib/public/Session/Exceptions/SessionNotAvailableException.php', 'OCP\\Settings\\IIconSection' => __DIR__ . '/../../..' . '/lib/public/Settings/IIconSection.php', 'OCP\\Settings\\IManager' => __DIR__ . '/../../..' . '/lib/public/Settings/IManager.php', @@ -1400,6 +1402,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Security\\RateLimiting\\Limiter' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Limiter.php', 'OC\\Security\\SecureRandom' => __DIR__ . '/../../..' . '/lib/private/Security/SecureRandom.php', 'OC\\Security\\TrustedDomainHelper' => __DIR__ . '/../../..' . '/lib/private/Security/TrustedDomainHelper.php', + 'OC\\Security\\VerificationToken\\VerificationToken' => __DIR__ . '/../../..' . '/lib/private/Security/VerificationToken/VerificationToken.php', 'OC\\Server' => __DIR__ . '/../../..' . '/lib/private/Server.php', 'OC\\ServerContainer' => __DIR__ . '/../../..' . '/lib/private/ServerContainer.php', 'OC\\ServerNotAvailableException' => __DIR__ . '/../../..' . '/lib/private/ServerNotAvailableException.php', diff --git a/lib/private/Security/VerificationToken/VerificationToken.php b/lib/private/Security/VerificationToken/VerificationToken.php new file mode 100644 index 0000000000000..4ac5605eecfd9 --- /dev/null +++ b/lib/private/Security/VerificationToken/VerificationToken.php @@ -0,0 +1,111 @@ + + * + * @author Arthur Schiwon + * + * @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 OC\Security\VerificationToken; + +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IConfig; +use OCP\IUser; +use OCP\Security\ICrypto; +use OCP\Security\ISecureRandom; +use OCP\Security\VerificationToken\InvalidTokenException; +use OCP\Security\VerificationToken\IVerificationToken; + +class VerificationToken implements IVerificationToken { + + /** @var IConfig */ + private $config; + /** @var ICrypto */ + private $crypto; + /** @var ITimeFactory */ + private $timeFactory; + /** @var ISecureRandom */ + private $secureRandom; + + public function __construct( + IConfig $config, + ICrypto $crypto, + ITimeFactory $timeFactory, + ISecureRandom $secureRandom + ) { + $this->config = $config; + $this->crypto = $crypto; + $this->timeFactory = $timeFactory; + $this->secureRandom = $secureRandom; + } + + /** + * @throws InvalidTokenException + */ + protected function throwInvalidTokenException(int $code): void { + throw new InvalidTokenException($code); + } + + public function check(string $token, ?IUser $user, string $subject, string $passwordPrefix = ''): void { + if ($user === null || !$user->isEnabled()) { + $this->throwInvalidTokenException(InvalidTokenException::USER_UNKNOWN); + } + + $encryptedToken = $this->config->getUserValue($user->getUID(), 'core', $subject, null); + if ($encryptedToken === null) { + $this->throwInvalidTokenException(InvalidTokenException::TOKEN_NOT_FOUND); + } + + try { + $decryptedToken = $this->crypto->decrypt($encryptedToken, $passwordPrefix.$this->config->getSystemValue('secret')); + } catch (\Exception $e) { + $this->throwInvalidTokenException(InvalidTokenException::TOKEN_DECRYPTION_ERROR); + } + + $splitToken = explode(':', $decryptedToken ?? ''); + if (count($splitToken) !== 2) { + $this->throwInvalidTokenException(InvalidTokenException::TOKEN_INVALID_FORMAT); + } + + if ($splitToken[0] < ($this->timeFactory->getTime() - 60 * 60 * 24 * 7) || + $user->getLastLogin() > $splitToken[0]) { + $this->throwInvalidTokenException(InvalidTokenException::TOKEN_EXPIRED); + } + + if (!hash_equals($splitToken[1], $token)) { + $this->throwInvalidTokenException(InvalidTokenException::TOKEN_MISMATCH); + } + } + + public function create(IUser $user, string $subject, string $passwordPrefix = ''): string { + $token = $this->secureRandom->generate( + 21, + ISecureRandom::CHAR_DIGITS. + ISecureRandom::CHAR_LOWER. + ISecureRandom::CHAR_UPPER + ); + $tokenValue = $this->timeFactory->getTime() .':'. $token; + $encryptedValue = $this->crypto->encrypt($tokenValue, $passwordPrefix . $this->config->getSystemValue('secret')); + $this->config->setUserValue($user->getUID(), 'core', $subject, $encryptedValue); + + return $token; + } +} diff --git a/lib/public/Security/VerificationToken/IVerificationToken.php b/lib/public/Security/VerificationToken/IVerificationToken.php new file mode 100644 index 0000000000000..0cd10377a970c --- /dev/null +++ b/lib/public/Security/VerificationToken/IVerificationToken.php @@ -0,0 +1,55 @@ + + * + * @author Arthur Schiwon + * + * @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 OCP\Security\VerificationToken; + +use OCP\IUser; + +/** + * @since 22.2.0 + */ +interface IVerificationToken { + + /** + * Checks whether the a provided tokent matches a stored token and its + * constraints. An InvalidTokenException is thrown on issues, otherwise + * the check is successful. + * + * null can be passed as $user, but mind that this is for conveniently + * passing the return of IUserManager::getUser() to this method. When + * $user is null, InvalidTokenException is thrown for all the issued + * tokens are user related. + * + * @throws InvalidTokenException + * @since 22.2.0 + */ + public function check(string $token, ?IUser $user, string $subject, string $passwordPrefix = ''): void; + + /** + * @since 22.2.0 + */ + public function create(IUser $user, string $subject, string $passwordPrefix = ''): string; +} diff --git a/lib/public/Security/VerificationToken/InvalidTokenException.php b/lib/public/Security/VerificationToken/InvalidTokenException.php new file mode 100644 index 0000000000000..cd0520c1e0784 --- /dev/null +++ b/lib/public/Security/VerificationToken/InvalidTokenException.php @@ -0,0 +1,74 @@ + + * + * @author Arthur Schiwon + * + * @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 OCP\Security\VerificationToken; + +/** @since 22.2.0 */ +class InvalidTokenException extends \Exception { + + /** + * @since 22.2.0 + */ + public function __construct(int $code) { + parent::__construct('', $code); + } + + /** + * @var int + * @since 22.2.0 + */ + public const USER_UNKNOWN = 1; + + /** + * @var int + * @since 22.2.0 + */ + public const TOKEN_NOT_FOUND = 2; + + /** + * @var int + * @since 22.2.0 + */ + public const TOKEN_DECRYPTION_ERROR = 3; + + /** + * @var int + * @since 22.2.0 + */ + public const TOKEN_INVALID_FORMAT = 4; + + /** + * @var int + * @since 22.2.0 + */ + public const TOKEN_EXPIRED = 5; + + /** + * @var int + * @since 22.2.0 + */ + public const TOKEN_MISMATCH = 6; +} diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php index fd4e27d47f1b4..a9dd4c1797b2b 100644 --- a/tests/Core/Controller/LostControllerTest.php +++ b/tests/Core/Controller/LostControllerTest.php @@ -26,7 +26,6 @@ use OC\Mail\Message; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\TemplateResponse; -use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\Encryption\IEncryptionModule; use OCP\Encryption\IManager; @@ -40,8 +39,8 @@ use OCP\IUserManager; use OCP\Mail\IEMailTemplate; use OCP\Mail\IMailer; -use OCP\Security\ICrypto; -use OCP\Security\ISecureRandom; +use OCP\Security\VerificationToken\InvalidTokenException; +use OCP\Security\VerificationToken\IVerificationToken; /** * Class LostControllerTest @@ -66,22 +65,18 @@ class LostControllerTest extends \Test\TestCase { private $config; /** @var IMailer | \PHPUnit\Framework\MockObject\MockObject */ private $mailer; - /** @var ISecureRandom | \PHPUnit\Framework\MockObject\MockObject */ - private $secureRandom; /** @var IManager|\PHPUnit\Framework\MockObject\MockObject */ private $encryptionManager; - /** @var ITimeFactory | \PHPUnit\Framework\MockObject\MockObject */ - private $timeFactory; /** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */ private $request; - /** @var ICrypto|\PHPUnit\Framework\MockObject\MockObject */ - private $crypto; /** @var ILogger|\PHPUnit\Framework\MockObject\MockObject */ private $logger; /** @var Manager|\PHPUnit\Framework\MockObject\MockObject */ private $twofactorManager; /** @var IInitialStateService|\PHPUnit\Framework\MockObject\MockObject */ private $initialStateService; + /** @var IVerificationToken|\PHPUnit\Framework\MockObject\MockObject */ + private $verificationToken; protected function setUp(): void { parent::setUp(); @@ -123,10 +118,6 @@ protected function setUp(): void { ->disableOriginalConstructor()->getMock(); $this->mailer = $this->getMockBuilder('\OCP\Mail\IMailer') ->disableOriginalConstructor()->getMock(); - $this->secureRandom = $this->getMockBuilder('\OCP\Security\ISecureRandom') - ->disableOriginalConstructor()->getMock(); - $this->timeFactory = $this->getMockBuilder('\OCP\AppFramework\Utility\ITimeFactory') - ->disableOriginalConstructor()->getMock(); $this->request = $this->getMockBuilder(IRequest::class) ->disableOriginalConstructor()->getMock(); $this->encryptionManager = $this->getMockBuilder(IManager::class) @@ -134,10 +125,10 @@ protected function setUp(): void { $this->encryptionManager->expects($this->any()) ->method('isEnabled') ->willReturn(true); - $this->crypto = $this->createMock(ICrypto::class); $this->logger = $this->createMock(ILogger::class); $this->twofactorManager = $this->createMock(Manager::class); $this->initialStateService = $this->createMock(IInitialStateService::class); + $this->verificationToken = $this->createMock(IVerificationToken::class); $this->lostController = new LostController( 'Core', $this->request, @@ -146,89 +137,31 @@ protected function setUp(): void { $this->defaults, $this->l10n, $this->config, - $this->secureRandom, 'lostpassword-noreply@localhost', $this->encryptionManager, $this->mailer, - $this->timeFactory, - $this->crypto, $this->logger, $this->twofactorManager, - $this->initialStateService + $this->initialStateService, + $this->verificationToken ); } - public function testResetFormWithNotExistingUser() { - $this->userManager->method('get') - ->with('NotExistingUser') - ->willReturn(null); - - $expectedResponse = new TemplateResponse( - 'core', - 'error', - [ - 'errors' => [ - ['error' => 'Couldn\'t reset password because the token is invalid'], - ] - ], - 'guest' - ); - $this->assertEquals($expectedResponse, $this->lostController->resetform('MySecretToken', 'NotExistingUser')); - } - - public function testResetFormInvalidTokenMatch() { - $this->config->method('getUserValue') - ->with('ValidTokenUser', 'core', 'lostpassword', null) - ->willReturn('encryptedToken'); - $this->existingUser->method('getLastLogin') - ->willReturn(12344); + public function testResetFormTokenError() { $this->userManager->method('get') ->with('ValidTokenUser') ->willReturn($this->existingUser); - $this->crypto->method('decrypt') - ->with( - $this->equalTo('encryptedToken'), - $this->equalTo('test@example.comSECRET') - )->willReturn('12345:TheOnlyAndOnlyOneTokenToResetThePassword'); + $this->verificationToken->expects($this->once()) + ->method('check') + ->with('12345:MySecretToken', $this->existingUser, 'lostpassword') + ->willThrowException(new InvalidTokenException(InvalidTokenException::TOKEN_DECRYPTION_ERROR)); $response = $this->lostController->resetform('12345:MySecretToken', 'ValidTokenUser'); $expectedResponse = new TemplateResponse('core', 'error', [ 'errors' => [ - ['error' => 'Couldn\'t reset password because the token is invalid'], - ] - ], - 'guest'); - $this->assertEquals($expectedResponse, $response); - } - - - public function testResetFormExpiredToken() { - $this->userManager->method('get') - ->with('ValidTokenUser') - ->willReturn($this->existingUser); - $this->config - ->expects($this->once()) - ->method('getUserValue') - ->with('ValidTokenUser', 'core', 'lostpassword', null) - ->willReturn('encryptedToken'); - $this->crypto->method('decrypt') - ->with( - $this->equalTo('encryptedToken'), - $this->equalTo('test@example.comSECRET') - )->willReturn('12345:TheOnlyAndOnlyOneTokenToResetThePassword'); - $this->timeFactory - ->expects($this->once()) - ->method('getTime') - ->willReturn(999999); - - $response = $this->lostController->resetform('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser'); - $expectedResponse = new TemplateResponse('core', - 'error', - [ - 'errors' => [ - ['error' => 'Couldn\'t reset password because the token is expired'], + ['error' => 'Could not reset password because the token is invalid'], ] ], 'guest'); @@ -236,39 +169,14 @@ public function testResetFormExpiredToken() { } public function testResetFormValidToken() { - $this->existingUser->method('getLastLogin') - ->willReturn(12344); $this->userManager->method('get') ->with('ValidTokenUser') ->willReturn($this->existingUser); - $this->timeFactory - ->expects($this->once()) - ->method('getTime') - ->willReturn(12348); + $this->verificationToken->expects($this->once()) + ->method('check') + ->with('MySecretToken', $this->existingUser, 'lostpassword'); - $this->config->method('getUserValue') - ->with('ValidTokenUser', 'core', 'lostpassword', null) - ->willReturn('encryptedToken'); - - $this->crypto->method('decrypt') - ->with( - $this->equalTo('encryptedToken'), - $this->equalTo('test@example.comSECRET') - )->willReturn('12345:TheOnlyAndOnlyOneTokenToResetThePassword'); - $this->urlGenerator - ->expects($this->once()) - ->method('linkToRouteAbsolute') - ->with('core.lost.setPassword', ['userId' => 'ValidTokenUser', 'token' => 'TheOnlyAndOnlyOneTokenToResetThePassword']) - ->willReturn('https://example.tld/index.php/lostpassword/'); - - $this->initialStateService->expects($this->at(0)) - ->method('provideInitialState') - ->with('core', 'resetPasswordUser', 'ValidTokenUser'); - $this->initialStateService->expects($this->at(1)) - ->method('provideInitialState') - ->with('core', 'resetPasswordTarget', 'https://example.tld/index.php/lostpassword/'); - - $response = $this->lostController->resetform('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser'); + $response = $this->lostController->resetform('MySecretToken', 'ValidTokenUser'); $expectedResponse = new TemplateResponse('core', 'login', [], @@ -319,24 +227,14 @@ public function testEmailUnsuccessful() { } public function testEmailSuccessful() { - $this->secureRandom - ->expects($this->once()) - ->method('generate') - ->with('21') - ->willReturn('ThisIsMaybeANotSoSecretToken!'); $this->userManager ->expects($this->any()) ->method('get') ->with('ExistingUser') ->willReturn($this->existingUser); - $this->timeFactory - ->expects($this->once()) - ->method('getTime') - ->willReturn(12348); - $this->config - ->expects($this->once()) - ->method('setUserValue') - ->with('ExistingUser', 'core', 'lostpassword', 'encryptedToken'); + $this->verificationToken->expects($this->once()) + ->method('create') + ->willReturn('ThisIsMaybeANotSoSecretToken!'); $this->urlGenerator ->expects($this->once()) ->method('linkToRouteAbsolute') @@ -379,12 +277,6 @@ public function testEmailSuccessful() { ->method('send') ->with($message); - $this->crypto->method('encrypt') - ->with( - $this->equalTo('12348:ThisIsMaybeANotSoSecretToken!'), - $this->equalTo('test@example.comSECRET') - )->willReturn('encryptedToken'); - $response = $this->lostController->email('ExistingUser'); $expectedResponse = new JSONResponse(['status' => 'success']); $expectedResponse->throttle(); @@ -392,11 +284,6 @@ public function testEmailSuccessful() { } public function testEmailWithMailSuccessful() { - $this->secureRandom - ->expects($this->once()) - ->method('generate') - ->with('21') - ->willReturn('ThisIsMaybeANotSoSecretToken!'); $this->userManager ->expects($this->any()) ->method('get') @@ -407,14 +294,9 @@ public function testEmailWithMailSuccessful() { ->method('getByEmail') ->with('test@example.com') ->willReturn([$this->existingUser]); - $this->timeFactory - ->expects($this->once()) - ->method('getTime') - ->willReturn(12348); - $this->config - ->expects($this->once()) - ->method('setUserValue') - ->with('ExistingUser', 'core', 'lostpassword', 'encryptedToken'); + $this->verificationToken->expects($this->once()) + ->method('create') + ->willReturn('ThisIsMaybeANotSoSecretToken!'); $this->urlGenerator ->expects($this->once()) ->method('linkToRouteAbsolute') @@ -457,12 +339,6 @@ public function testEmailWithMailSuccessful() { ->method('send') ->with($message); - $this->crypto->method('encrypt') - ->with( - $this->equalTo('12348:ThisIsMaybeANotSoSecretToken!'), - $this->equalTo('test@example.comSECRET') - )->willReturn('encryptedToken'); - $response = $this->lostController->email('test@example.com'); $expectedResponse = new JSONResponse(['status' => 'success']); $expectedResponse->throttle(); @@ -470,24 +346,14 @@ public function testEmailWithMailSuccessful() { } public function testEmailCantSendException() { - $this->secureRandom - ->expects($this->once()) - ->method('generate') - ->with('21') - ->willReturn('ThisIsMaybeANotSoSecretToken!'); $this->userManager ->expects($this->any()) ->method('get') ->with('ExistingUser') ->willReturn($this->existingUser); - $this->config - ->expects($this->once()) - ->method('setUserValue') - ->with('ExistingUser', 'core', 'lostpassword', 'encryptedToken'); - $this->timeFactory - ->expects($this->once()) - ->method('getTime') - ->willReturn(12348); + $this->verificationToken->expects($this->once()) + ->method('create') + ->willReturn('ThisIsMaybeANotSoSecretToken!'); $this->urlGenerator ->expects($this->once()) ->method('linkToRouteAbsolute') @@ -530,12 +396,6 @@ public function testEmailCantSendException() { ->with($message) ->will($this->throwException(new \Exception())); - $this->crypto->method('encrypt') - ->with( - $this->equalTo('12348:ThisIsMaybeANotSoSecretToken!'), - $this->equalTo('test@example.comSECRET') - )->willReturn('encryptedToken'); - $this->logger->expects($this->exactly(1)) ->method('logException'); @@ -560,14 +420,6 @@ public function testSetPasswordUnsuccessful() { ->willReturn($this->existingUser); $this->config->expects($this->never()) ->method('deleteUserValue'); - $this->timeFactory->method('getTime') - ->willReturn(12348); - - $this->crypto->method('decrypt') - ->with( - $this->equalTo('encryptedData'), - $this->equalTo('test@example.comSECRET') - )->willReturn('12345:TheOnlyAndOnlyOneTokenToResetThePassword'); $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); $expectedResponse = ['status' => 'error', 'msg' => '']; @@ -590,14 +442,6 @@ public function testSetPasswordSuccessful() { $this->config->expects($this->once()) ->method('deleteUserValue') ->with('ValidTokenUser', 'core', 'lostpassword'); - $this->timeFactory->method('getTime') - ->willReturn(12348); - - $this->crypto->method('decrypt') - ->with( - $this->equalTo('encryptedData'), - $this->equalTo('test@example.comSECRET') - )->willReturn('12345:TheOnlyAndOnlyOneTokenToResetThePassword'); $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); $expectedResponse = ['user' => 'ValidTokenUser', 'status' => 'success']; @@ -611,19 +455,14 @@ public function testSetPasswordExpiredToken() { $this->userManager->method('get') ->with('ValidTokenUser') ->willReturn($this->existingUser); - $this->timeFactory->method('getTime') - ->willReturn(617146); - - $this->crypto->method('decrypt') - ->with( - $this->equalTo('encryptedData'), - $this->equalTo('test@example.comSECRET') - )->willReturn('12345:TheOnlyAndOnlyOneTokenToResetThePassword'); + $this->verificationToken->expects($this->atLeastOnce()) + ->method('check') + ->willThrowException(new InvalidTokenException(InvalidTokenException::TOKEN_EXPIRED)); $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); $expectedResponse = [ 'status' => 'error', - 'msg' => 'Couldn\'t reset password because the token is expired', + 'msg' => 'Could not reset password because the token is expired', ]; $this->assertSame($expectedResponse, $response); } @@ -636,45 +475,14 @@ public function testSetPasswordInvalidDataInDb() { ->method('get') ->with('ValidTokenUser') ->willReturn($this->existingUser); - - $this->crypto->method('decrypt') - ->with( - $this->equalTo('invalidEncryptedData'), - $this->equalTo('test@example.comSECRET') - )->willReturn('TheOnlyAndOnlyOneTokenToResetThePassword'); + $this->verificationToken->expects($this->atLeastOnce()) + ->method('check') + ->willThrowException(new InvalidTokenException(InvalidTokenException::TOKEN_INVALID_FORMAT)); $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); $expectedResponse = [ 'status' => 'error', - 'msg' => 'Couldn\'t reset password because the token is invalid', - ]; - $this->assertSame($expectedResponse, $response); - } - - public function testSetPasswordExpiredTokenDueToLogin() { - $this->config->method('getUserValue') - ->with('ValidTokenUser', 'core', 'lostpassword', null) - ->willReturn('encryptedData'); - $this->existingUser->method('getLastLogin') - ->willReturn(12346); - $this->userManager - ->method('get') - ->with('ValidTokenUser') - ->willReturn($this->existingUser); - $this->timeFactory - ->method('getTime') - ->willReturn(12345); - - $this->crypto->method('decrypt') - ->with( - $this->equalTo('encryptedData'), - $this->equalTo('test@example.comSECRET') - )->willReturn('12345:TheOnlyAndOnlyOneTokenToResetThePassword'); - - $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); - $expectedResponse = [ - 'status' => 'error', - 'msg' => 'Couldn\'t reset password because the token is expired', + 'msg' => 'Could not reset password because the token is invalid', ]; $this->assertSame($expectedResponse, $response); } @@ -686,33 +494,14 @@ public function testIsSetPasswordWithoutTokenFailing() { $this->userManager->method('get') ->with('ValidTokenUser') ->willReturn($this->existingUser); - - $this->crypto->method('decrypt') - ->with( - $this->equalTo('aValidtoken'), - $this->equalTo('test@example.comSECRET') - )->willThrowException(new \Exception()); - - $response = $this->lostController->setPassword('', 'ValidTokenUser', 'NewPassword', true); - $expectedResponse = [ - 'status' => 'error', - 'msg' => 'Couldn\'t reset password because the token is invalid' - ]; - $this->assertSame($expectedResponse, $response); - } - - public function testIsSetPasswordTokenNullFailing() { - $this->config->method('getUserValue') - ->with('ValidTokenUser', 'core', 'lostpassword', null) - ->willReturn(null); - $this->userManager->method('get') - ->with('ValidTokenUser') - ->willReturn($this->existingUser); + $this->verificationToken->expects($this->atLeastOnce()) + ->method('check') + ->willThrowException(new InvalidTokenException(InvalidTokenException::TOKEN_MISMATCH)); $response = $this->lostController->setPassword('', 'ValidTokenUser', 'NewPassword', true); $expectedResponse = [ 'status' => 'error', - 'msg' => 'Couldn\'t reset password because the token is invalid' + 'msg' => 'Could not reset password because the token is invalid' ]; $this->assertSame($expectedResponse, $response); } @@ -732,10 +521,14 @@ public function testSetPasswordForDisabledUser() { ->with('DisabledUser') ->willReturn($user); + $this->verificationToken->expects($this->atLeastOnce()) + ->method('check') + ->willThrowException(new InvalidTokenException(InvalidTokenException::USER_UNKNOWN)); + $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'DisabledUser', 'NewPassword', true); $expectedResponse = [ 'status' => 'error', - 'msg' => 'Couldn\'t reset password because the token is invalid' + 'msg' => 'Could not reset password because the token is invalid' ]; $this->assertSame($expectedResponse, $response); } @@ -798,14 +591,6 @@ public function testSetPasswordDontProceedMasterKey() { $this->config->expects($this->once()) ->method('deleteUserValue') ->with('ValidTokenUser', 'core', 'lostpassword'); - $this->timeFactory->method('getTime') - ->willReturn(12348); - - $this->crypto->method('decrypt') - ->with( - $this->equalTo('encryptedData'), - $this->equalTo('test@example.comSECRET') - )->willReturn('12345:TheOnlyAndOnlyOneTokenToResetThePassword'); $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', false); $expectedResponse = ['user' => 'ValidTokenUser', 'status' => 'success']; diff --git a/tests/lib/Security/VerificationToken/VerificationTokenTest.php b/tests/lib/Security/VerificationToken/VerificationTokenTest.php new file mode 100644 index 0000000000000..d1faf18dd8f2a --- /dev/null +++ b/tests/lib/Security/VerificationToken/VerificationTokenTest.php @@ -0,0 +1,272 @@ + + * + * @author Arthur Schiwon + * + * @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\Security\VerificationToken; + +use OC\Security\VerificationToken\VerificationToken; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IConfig; +use OCP\IUser; +use OCP\Security\ICrypto; +use OCP\Security\ISecureRandom; +use OCP\Security\VerificationToken\InvalidTokenException; +use Test\TestCase; + +class VerificationTokenTest extends TestCase { + /** @var VerificationToken */ + protected $token; + /** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */ + protected $config; + /** @var ISecureRandom|\PHPUnit\Framework\MockObject\MockObject */ + protected $secureRandom; + /** @var ICrypto|\PHPUnit\Framework\MockObject\MockObject */ + protected $crypto; + /** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */ + protected $timeFactory; + + protected function setUp(): void { + parent::setUp(); + + $this->config = $this->createMock(IConfig::class); + $this->crypto = $this->createMock(ICrypto::class); + $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->secureRandom = $this->createMock(ISecureRandom::class); + + $this->token = new VerificationToken( + $this->config, + $this->crypto, + $this->timeFactory, + $this->secureRandom + ); + } + + public function testTokenUserUnknown() { + $this->expectException(InvalidTokenException::class); + $this->expectExceptionCode(InvalidTokenException::USER_UNKNOWN); + $this->token->check('encryptedToken', null, 'fingerprintToken', 'foobar'); + } + + public function testTokenUserUnknown2() { + $user = $this->createMock(IUser::class); + $user->expects($this->atLeastOnce()) + ->method('isEnabled') + ->willReturn(false); + + $this->expectException(InvalidTokenException::class); + $this->expectExceptionCode(InvalidTokenException::USER_UNKNOWN); + $this->token->check('encryptedToken', $user, 'fingerprintToken', 'foobar'); + } + + public function testTokenNotFound() { + $user = $this->createMock(IUser::class); + $user->expects($this->atLeastOnce()) + ->method('isEnabled') + ->willReturn(true); + $user->expects($this->atLeastOnce()) + ->method('getUID') + ->willReturn('alice'); + + // implicit: IConfig::getUserValue returns null by default + + $this->expectException(InvalidTokenException::class); + $this->expectExceptionCode(InvalidTokenException::TOKEN_NOT_FOUND); + $this->token->check('encryptedToken', $user, 'fingerprintToken', 'foobar'); + } + + public function testTokenDecryptionError() { + $user = $this->createMock(IUser::class); + $user->expects($this->atLeastOnce()) + ->method('isEnabled') + ->willReturn(true); + $user->expects($this->atLeastOnce()) + ->method('getUID') + ->willReturn('alice'); + + $this->config->expects($this->atLeastOnce()) + ->method('getUserValue') + ->with('alice', 'core', 'fingerprintToken', null) + ->willReturn('encryptedToken'); + $this->config->expects($this->any()) + ->method('getSystemValue') + ->with('secret') + ->willReturn('357111317'); + + $this->crypto->method('decrypt') + ->with('encryptedToken', 'foobar' . '357111317') + ->willThrowException(new \Exception('decryption failed')); + + $this->expectException(InvalidTokenException::class); + $this->expectExceptionCode(InvalidTokenException::TOKEN_DECRYPTION_ERROR); + $this->token->check('encryptedToken', $user, 'fingerprintToken', 'foobar'); + } + + public function testTokenInvalidFormat() { + $user = $this->createMock(IUser::class); + $user->expects($this->atLeastOnce()) + ->method('isEnabled') + ->willReturn(true); + $user->expects($this->atLeastOnce()) + ->method('getUID') + ->willReturn('alice'); + + $this->config->expects($this->atLeastOnce()) + ->method('getUserValue') + ->with('alice', 'core', 'fingerprintToken', null) + ->willReturn('encryptedToken'); + $this->config->expects($this->any()) + ->method('getSystemValue') + ->with('secret') + ->willReturn('357111317'); + + $this->crypto->method('decrypt') + ->with('encryptedToken', 'foobar' . '357111317') + ->willReturn('decrypted^nonsense'); + + $this->expectException(InvalidTokenException::class); + $this->expectExceptionCode(InvalidTokenException::TOKEN_INVALID_FORMAT); + $this->token->check('encryptedToken', $user, 'fingerprintToken', 'foobar'); + } + + public function testTokenExpired() { + $user = $this->createMock(IUser::class); + $user->expects($this->atLeastOnce()) + ->method('isEnabled') + ->willReturn(true); + $user->expects($this->atLeastOnce()) + ->method('getUID') + ->willReturn('alice'); + $user->expects($this->any()) + ->method('getLastLogin') + ->willReturn(604803); + + $this->config->expects($this->atLeastOnce()) + ->method('getUserValue') + ->with('alice', 'core', 'fingerprintToken', null) + ->willReturn('encryptedToken'); + $this->config->expects($this->any()) + ->method('getSystemValue') + ->with('secret') + ->willReturn('357111317'); + + $this->crypto->method('decrypt') + ->with('encryptedToken', 'foobar' . '357111317') + ->willReturn('604800:mY70K3n'); + + $this->timeFactory->expects($this->any()) + ->method('getTime') + ->willReturn(604801); + + $this->expectException(InvalidTokenException::class); + $this->expectExceptionCode(InvalidTokenException::TOKEN_EXPIRED); + $this->token->check('encryptedToken', $user, 'fingerprintToken', 'foobar'); + } + + public function testTokenMismatch() { + $user = $this->createMock(IUser::class); + $user->expects($this->atLeastOnce()) + ->method('isEnabled') + ->willReturn(true); + $user->expects($this->atLeastOnce()) + ->method('getUID') + ->willReturn('alice'); + $user->expects($this->any()) + ->method('getLastLogin') + ->willReturn(604703); + + $this->config->expects($this->atLeastOnce()) + ->method('getUserValue') + ->with('alice', 'core', 'fingerprintToken', null) + ->willReturn('encryptedToken'); + $this->config->expects($this->any()) + ->method('getSystemValue') + ->with('secret') + ->willReturn('357111317'); + + $this->crypto->method('decrypt') + ->with('encryptedToken', 'foobar' . '357111317') + ->willReturn('604802:mY70K3n'); + + $this->timeFactory->expects($this->any()) + ->method('getTime') + ->willReturn(604801); + + $this->expectException(InvalidTokenException::class); + $this->expectExceptionCode(InvalidTokenException::TOKEN_MISMATCH); + $this->token->check('encryptedToken', $user, 'fingerprintToken', 'foobar'); + } + + public function testTokenSuccess() { + $user = $this->createMock(IUser::class); + $user->expects($this->atLeastOnce()) + ->method('isEnabled') + ->willReturn(true); + $user->expects($this->atLeastOnce()) + ->method('getUID') + ->willReturn('alice'); + $user->expects($this->any()) + ->method('getLastLogin') + ->willReturn(604703); + + $this->config->expects($this->atLeastOnce()) + ->method('getUserValue') + ->with('alice', 'core', 'fingerprintToken', null) + ->willReturn('encryptedToken'); + $this->config->expects($this->any()) + ->method('getSystemValue') + ->with('secret') + ->willReturn('357111317'); + + $this->crypto->method('decrypt') + ->with('encryptedToken', 'foobar' . '357111317') + ->willReturn('604802:barfoo'); + + $this->timeFactory->expects($this->any()) + ->method('getTime') + ->willReturn(604801); + + $this->token->check('barfoo', $user, 'fingerprintToken', 'foobar'); + } + + public function testCreate() { + $user = $this->createMock(IUser::class); + $user->expects($this->any()) + ->method('getUID') + ->willReturn('alice'); + + $this->secureRandom->expects($this->atLeastOnce()) + ->method('generate') + ->willReturn('barfoo'); + $this->crypto->expects($this->atLeastOnce()) + ->method('encrypt') + ->willReturn('encryptedToken'); + $this->config->expects($this->atLeastOnce()) + ->method('setUserValue') + ->with('alice', 'core', 'fingerprintToken', 'encryptedToken'); + + $vToken = $this->token->create($user, 'fingerprintToken', 'foobar'); + $this->assertSame('barfoo', $vToken); + } +} From c9c671a466148bf52dc61c6a536cce1fead015be Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 20 Aug 2021 19:59:08 +0200 Subject: [PATCH 2/8] implement verification for additional mails - mails added by (sub)admins are automatically verified - provisioning_api controller as verification endpoint - IAccountProperty gets a locallyVerified property - IPropertyCollection gets a method to fetch an IAccountProperty by value - an remove equivalent was already present - AccountManager always initiates mail verification on update if necessary - add core success template for arbitrary title and message Signed-off-by: Arthur Schiwon --- apps/provisioning_api/appinfo/routes.php | 4 + .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../composer/composer/installed.php | 4 +- .../lib/Controller/UsersController.php | 14 +- .../lib/Controller/VerificationController.php | 121 ++++++++++++++++ core/templates/success.php | 13 ++ lib/private/Accounts/AccountManager.php | 131 ++++++++++++++++-- lib/private/Accounts/AccountProperty.php | 21 ++- .../Accounts/AccountPropertyCollection.php | 9 ++ lib/private/Server.php | 4 + lib/public/Accounts/IAccountProperty.php | 20 +++ .../Accounts/IAccountPropertyCollection.php | 9 ++ tests/lib/Accounts/AccountManagerTest.php | 36 +++++ 14 files changed, 371 insertions(+), 17 deletions(-) create mode 100644 apps/provisioning_api/lib/Controller/VerificationController.php create mode 100644 core/templates/success.php diff --git a/apps/provisioning_api/appinfo/routes.php b/apps/provisioning_api/appinfo/routes.php index 2f981e0c9241b..81a5bb94e02f2 100644 --- a/apps/provisioning_api/appinfo/routes.php +++ b/apps/provisioning_api/appinfo/routes.php @@ -74,4 +74,8 @@ ['name' => 'AppConfig#setValue', 'url' => '/api/v1/config/apps/{app}/{key}', 'verb' => 'POST'], ['name' => 'AppConfig#deleteKey', 'url' => '/api/v1/config/apps/{app}/{key}', 'verb' => 'DELETE'], ], + 'routes' => [ + // Verification + ['name' => 'Verification#verifyMail', 'url' => '/mailVerification/{key}/{token}/{userId}', 'verb' => 'GET'], + ] ]; diff --git a/apps/provisioning_api/composer/composer/autoload_classmap.php b/apps/provisioning_api/composer/composer/autoload_classmap.php index 22927806e656d..447f92afc8d6c 100644 --- a/apps/provisioning_api/composer/composer/autoload_classmap.php +++ b/apps/provisioning_api/composer/composer/autoload_classmap.php @@ -14,6 +14,7 @@ 'OCA\\Provisioning_API\\Controller\\AppsController' => $baseDir . '/../lib/Controller/AppsController.php', 'OCA\\Provisioning_API\\Controller\\GroupsController' => $baseDir . '/../lib/Controller/GroupsController.php', 'OCA\\Provisioning_API\\Controller\\UsersController' => $baseDir . '/../lib/Controller/UsersController.php', + 'OCA\\Provisioning_API\\Controller\\VerificationController' => $baseDir . '/../lib/Controller/VerificationController.php', 'OCA\\Provisioning_API\\FederatedShareProviderFactory' => $baseDir . '/../lib/FederatedShareProviderFactory.php', 'OCA\\Provisioning_API\\Listener\\UserDeletedListener' => $baseDir . '/../lib/Listener/UserDeletedListener.php', 'OCA\\Provisioning_API\\Middleware\\Exceptions\\NotSubAdminException' => $baseDir . '/../lib/Middleware/Exceptions/NotSubAdminException.php', diff --git a/apps/provisioning_api/composer/composer/autoload_static.php b/apps/provisioning_api/composer/composer/autoload_static.php index f5a4b73f4f875..6dbf6b45c7995 100644 --- a/apps/provisioning_api/composer/composer/autoload_static.php +++ b/apps/provisioning_api/composer/composer/autoload_static.php @@ -29,6 +29,7 @@ class ComposerStaticInitProvisioning_API 'OCA\\Provisioning_API\\Controller\\AppsController' => __DIR__ . '/..' . '/../lib/Controller/AppsController.php', 'OCA\\Provisioning_API\\Controller\\GroupsController' => __DIR__ . '/..' . '/../lib/Controller/GroupsController.php', 'OCA\\Provisioning_API\\Controller\\UsersController' => __DIR__ . '/..' . '/../lib/Controller/UsersController.php', + 'OCA\\Provisioning_API\\Controller\\VerificationController' => __DIR__ . '/..' . '/../lib/Controller/VerificationController.php', 'OCA\\Provisioning_API\\FederatedShareProviderFactory' => __DIR__ . '/..' . '/../lib/FederatedShareProviderFactory.php', 'OCA\\Provisioning_API\\Listener\\UserDeletedListener' => __DIR__ . '/..' . '/../lib/Listener/UserDeletedListener.php', 'OCA\\Provisioning_API\\Middleware\\Exceptions\\NotSubAdminException' => __DIR__ . '/..' . '/../lib/Middleware/Exceptions/NotSubAdminException.php', diff --git a/apps/provisioning_api/composer/composer/installed.php b/apps/provisioning_api/composer/composer/installed.php index 244245bc0cf42..9f53826650b21 100644 --- a/apps/provisioning_api/composer/composer/installed.php +++ b/apps/provisioning_api/composer/composer/installed.php @@ -5,7 +5,7 @@ 'type' => 'library', 'install_path' => __DIR__ . '/../', 'aliases' => array(), - 'reference' => 'fb5ee6087bfd1f4cc2f37cda7a7cab7072aaae86', + 'reference' => '13a9cd28a5a5d92e285df040d084d5d608e2f768', 'name' => '__root__', 'dev' => false, ), @@ -16,7 +16,7 @@ 'type' => 'library', 'install_path' => __DIR__ . '/../', 'aliases' => array(), - 'reference' => 'fb5ee6087bfd1f4cc2f37cda7a7cab7072aaae86', + 'reference' => '13a9cd28a5a5d92e285df040d084d5d608e2f768', 'dev_requirement' => false, ), ), diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 20472bebaceb4..114dfcb074ab6 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -621,6 +621,10 @@ public function editUserMultiValue( throw new OCSException('', OCSController::RESPOND_NOT_FOUND); } + $subAdminManager = $this->groupManager->getSubAdmin(); + $isAdminOrSubadmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()) + || $subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser); + $permittedFields = []; if ($targetUser->getUID() === $currentLoggedInUser->getUID()) { // Editing self (display, email) @@ -628,11 +632,8 @@ public function editUserMultiValue( $permittedFields[] = IAccountManager::COLLECTION_EMAIL . self::SCOPE_SUFFIX; } else { // Check if admin / subadmin - $subAdminManager = $this->groupManager->getSubAdmin(); - if ($this->groupManager->isAdmin($currentLoggedInUser->getUID()) - || $subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) { + if ($isAdminOrSubadmin) { // They have permissions over the user - $permittedFields[] = IAccountManager::COLLECTION_EMAIL; } else { // No rights @@ -652,6 +653,11 @@ public function editUserMultiValue( $mailCollection->removePropertyByValue($key); if ($value !== '') { $mailCollection->addPropertyWithDefaults($value); + $property = $mailCollection->getPropertyByValue($key); + if ($isAdminOrSubadmin && $property) { + // admin set mails are auto-verified + $property->setLocallyVerified(IAccountManager::VERIFIED); + } } $this->accountManager->updateAccount($userAccount); break; diff --git a/apps/provisioning_api/lib/Controller/VerificationController.php b/apps/provisioning_api/lib/Controller/VerificationController.php new file mode 100644 index 0000000000000..b248d3e828575 --- /dev/null +++ b/apps/provisioning_api/lib/Controller/VerificationController.php @@ -0,0 +1,121 @@ + + * + * @author Arthur Schiwon + * + * @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 OCA\Provisioning_API\Controller; + +use InvalidArgumentException; +use OC\Security\Crypto; +use OCP\Accounts\IAccountManager; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\TemplateResponse; +use OCP\IL10N; +use OCP\IRequest; +use OCP\IUserManager; +use OCP\IUserSession; +use OCP\Security\VerificationToken\InvalidTokenException; +use OCP\Security\VerificationToken\IVerificationToken; + +class VerificationController extends Controller { + + /** @var IVerificationToken */ + private $verificationToken; + /** @var IUserManager */ + private $userManager; + /** @var IL10N */ + private $l10n; + /** @var IUserSession */ + private $userSession; + /** @var IAccountManager */ + private $accountManager; + /** @var Crypto */ + private $crypto; + + public function __construct( + string $appName, + IRequest $request, + IVerificationToken $verificationToken, + IUserManager $userManager, + IL10N $l10n, + IUserSession $userSession, + IAccountManager $accountManager, + Crypto $crypto + ) { + parent::__construct($appName, $request); + $this->verificationToken = $verificationToken; + $this->userManager = $userManager; + $this->l10n = $l10n; + $this->userSession = $userSession; + $this->accountManager = $accountManager; + $this->crypto = $crypto; + } + + /** + * @NoCSRFRequired + */ + public function verifyMail(string $token, string $userId, string $key) { + try { + if ($this->userSession->getUser()->getUID() !== $userId) { + throw new InvalidArgumentException('Logged in user is not mail address owner'); + } + $email = $this->crypto->decrypt($key); + $ref = \substr(hash('sha256', $email), 0, 8); + + $user = $this->userManager->get($userId); + $this->verificationToken->check($token, $user, 'verifyMail' . $ref, $email); + + $userAccount = $this->accountManager->getAccount($user); + $emailProperty = $userAccount->getPropertyCollection(IAccountManager::COLLECTION_EMAIL) + ->getPropertyByValue($email); + + if ($emailProperty === null) { + throw new InvalidArgumentException($this->l10n->t('Email was already removed from account and cannot be confirmed anymore.')); + } + $emailProperty->setLocallyVerified(IAccountManager::VERIFIED); + $this->accountManager->updateAccount($userAccount); + } catch (InvalidTokenException $e) { + $error = $e->getCode() === InvalidTokenException::TOKEN_EXPIRED + ? $this->l10n->t('Could not verify mail because the token is expired.') + : $this->l10n->t('Could not verify mail because the token is invalid.'); + } catch (InvalidArgumentException $e) { + $error = $e->getMessage(); + } catch (\Exception $e) { + $error = $this->l10n->t('An unexpected error occurred. Please consult your sysadmin.'); + } + + if (isset($error)) { + return new TemplateResponse( + 'core', 'error', [ + 'errors' => [['error' => $error]] + ], 'guest'); + } + + return new TemplateResponse( + 'core', 'success', [ + 'title' => $this->l10n->t('Email confirmation successful'), + 'message' => $this->l10n->t('Email confirmation successful'), + ], 'guest'); + } +} diff --git a/core/templates/success.php b/core/templates/success.php new file mode 100644 index 0000000000000..5ce8ff4f045bc --- /dev/null +++ b/core/templates/success.php @@ -0,0 +1,13 @@ + + +
+

+

+

+ t('Go to %s', [$theme->getName()])); ?> +

+
diff --git a/lib/private/Accounts/AccountManager.php b/lib/private/Accounts/AccountManager.php index 9fc5accfa08a4..a3f971df6a118 100644 --- a/lib/private/Accounts/AccountManager.php +++ b/lib/private/Accounts/AccountManager.php @@ -32,6 +32,7 @@ */ namespace OC\Accounts; +use Exception; use InvalidArgumentException; use libphonenumber\NumberParseException; use libphonenumber\PhoneNumber; @@ -45,9 +46,17 @@ use OCP\Accounts\PropertyDoesNotExistException; use OCP\BackgroundJob\IJobList; use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\Defaults; use OCP\IConfig; use OCP\IDBConnection; +use OCP\IL10N; +use OCP\IURLGenerator; use OCP\IUser; +use OCP\L10N\IFactory; +use OCP\Mail\IMailer; +use OCP\Security\ICrypto; +use OCP\Security\VerificationToken\IVerificationToken; +use OCP\Util; use Psr\Log\LoggerInterface; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\EventDispatcher\GenericEvent; @@ -88,17 +97,46 @@ class AccountManager implements IAccountManager { /** @var LoggerInterface */ private $logger; - - public function __construct(IDBConnection $connection, - IConfig $config, - EventDispatcherInterface $eventDispatcher, - IJobList $jobList, - LoggerInterface $logger) { + /** @var IVerificationToken */ + private $verificationToken; + /** @var IMailer */ + private $mailer; + /** @var Defaults */ + private $defaults; + /** @var IL10N */ + private $l10n; + /** @var IURLGenerator */ + private $urlGenerator; + /** @var ICrypto */ + private $crypto; + /** @var IFactory */ + private $l10nfactory; + + public function __construct( + IDBConnection $connection, + IConfig $config, + EventDispatcherInterface $eventDispatcher, + IJobList $jobList, + LoggerInterface $logger, + IVerificationToken $verificationToken, + IMailer $mailer, + Defaults $defaults, + IFactory $factory, + IURLGenerator $urlGenerator, + ICrypto $crypto + ) { $this->connection = $connection; $this->config = $config; $this->eventDispatcher = $eventDispatcher; $this->jobList = $jobList; $this->logger = $logger; + $this->verificationToken = $verificationToken; + $this->mailer = $mailer; + $this->defaults = $defaults; + $this->urlGenerator = $urlGenerator; + $this->crypto = $crypto; + // DIing IL10N results in a dependency loop + $this->l10nfactory = $factory; } /** @@ -337,7 +375,6 @@ protected function searchUsersForRelatedCollection(string $property, array $valu /** * check if we need to ask the server for email verification, if yes we create a cronjob - * */ protected function checkEmailVerification(IAccount $updatedAccount, array $oldData): void { try { @@ -358,11 +395,73 @@ protected function checkEmailVerification(IAccount $updatedAccount, array $oldDa ] ); + $property->setVerified(self::VERIFICATION_IN_PROGRESS); + } + } + + protected function checkLocalEmailVerification(IAccount $updatedAccount, array $oldData): void { + $mailCollection = $updatedAccount->getPropertyCollection(self::COLLECTION_EMAIL); + foreach ($mailCollection->getProperties() as $property) { + if ($property->getLocallyVerified() !== self::NOT_VERIFIED) { + continue; + } + if ($this->sendEmailVerificationEmail($updatedAccount->getUser(), $property->getValue())) { + $property->setLocallyVerified(self::VERIFICATION_IN_PROGRESS); + } + } + } + + protected function sendEmailVerificationEmail(IUser $user, string $email): bool { + $ref = \substr(hash('sha256', $email), 0, 8); + $key = $this->crypto->encrypt($email); + $token = $this->verificationToken->create($user, 'verifyMail' . $ref, $email); + $link = $this->urlGenerator->linkToRouteAbsolute('provisioning_api.Verification.verifyMail', + [ + 'userId' => $user->getUID(), + 'token' => $token, + 'key' => $key + ]); + $emailTemplate = $this->mailer->createEMailTemplate('core.EmailVerification', [ + 'link' => $link, + ]); - $property->setVerified(self::VERIFICATION_IN_PROGRESS); + if (!$this->l10n) { + $this->l10n = $this->l10nfactory->get('core'); } + + $emailTemplate->setSubject($this->l10n->t('%s email verification', [$this->defaults->getName()])); + $emailTemplate->addHeader(); + $emailTemplate->addHeading($this->l10n->t('Email verification')); + + $emailTemplate->addBodyText( + htmlspecialchars($this->l10n->t('Click the following button to confirm your email.')), + $this->l10n->t('Click the following link to confirm your email.') + ); + + $emailTemplate->addBodyButton( + htmlspecialchars($this->l10n->t('Confirm your email')), + $link, + false + ); + $emailTemplate->addFooter(); + + try { + $message = $this->mailer->createMessage(); + $message->setTo([$email => $user->getDisplayName()]); + $message->setFrom([Util::getDefaultEmailAddress('verification-noreply') => $this->defaults->getName()]); + $message->useTemplate($emailTemplate); + $this->mailer->send($message); + } catch (Exception $e) { + // Log the exception and continue + $this->logger->info('Failed to send verification mail', [ + 'app' => 'core', + 'exception' => $e + ]); + return false; + } + return true; } /** @@ -406,7 +505,6 @@ protected function updateVerificationStatus(IAccount $updatedAccount, array $old } } - /** * add new user to accounts table * @@ -435,6 +533,12 @@ protected function prepareJson(array $data): string { foreach ($data as $dataRow) { $propertyName = $dataRow['name']; unset($dataRow['name']); + + if (isset($dataRow['locallyVerified']) && $dataRow['locallyVerified'] === self::NOT_VERIFIED) { + // do not write default value, save DB space + unset($dataRow['locallyVerified']); + } + if (!$this->isCollection($propertyName)) { $preparedData[$propertyName] = $dataRow; continue; @@ -511,7 +615,6 @@ protected function writeUserDataProperties(IQueryBuilder $query, array $data): v continue; } - $query->setParameter('name', $property['name']) ->setParameter('value', $property['value'] ?? ''); $query->executeStatement(); @@ -587,6 +690,7 @@ private function arrayDataToCollection(IAccount $account, array $data): IAccount $data['verified'] ?? self::NOT_VERIFIED, '' ); + $p->setLocallyVerified($data['locallyVerified'] ?? self::NOT_VERIFIED); $collection->addProperty($p); return $collection; @@ -599,6 +703,10 @@ private function parseAccountData(IUser $user, $data): Account { $account->setPropertyCollection($this->arrayDataToCollection($account, $accountData)); } else { $account->setProperty($accountData['name'], $accountData['value'] ?? '', $accountData['scope'] ?? self::SCOPE_LOCAL, $accountData['verified'] ?? self::NOT_VERIFIED); + if (isset($accountData['locallyVerified'])) { + $property = $account->getProperty($accountData['name']); + $property->setLocallyVerified($accountData['locallyVerified']); + } } } return $account; @@ -640,14 +748,17 @@ public function updateAccount(IAccount $account): void { $oldData = $this->getUser($account->getUser(), false); $this->updateVerificationStatus($account, $oldData); $this->checkEmailVerification($account, $oldData); + $this->checkLocalEmailVerification($account, $oldData); $data = []; foreach ($account->getAllProperties() as $property) { + /** @var IAccountProperty $property */ $data[] = [ 'name' => $property->getName(), 'value' => $property->getValue(), 'scope' => $property->getScope(), 'verified' => $property->getVerified(), + 'locallyVerified' => $property->getLocallyVerified(), ]; } diff --git a/lib/private/Accounts/AccountProperty.php b/lib/private/Accounts/AccountProperty.php index 1a21baf96981a..0e6356e9e92e6 100644 --- a/lib/private/Accounts/AccountProperty.php +++ b/lib/private/Accounts/AccountProperty.php @@ -27,6 +27,7 @@ */ namespace OC\Accounts; +use InvalidArgumentException; use OCP\Accounts\IAccountManager; use OCP\Accounts\IAccountProperty; @@ -42,6 +43,8 @@ class AccountProperty implements IAccountProperty { private $verified; /** @var string */ private $verificationData; + /** @var string */ + private $locallyVerified = IAccountManager::NOT_VERIFIED; public function __construct(string $name, string $value, string $scope, string $verified, string $verificationData) { $this->name = $name; @@ -90,7 +93,7 @@ public function setScope(string $scope): IAccountProperty { IAccountManager::SCOPE_PRIVATE, IAccountManager::SCOPE_PUBLISHED ])) { - throw new \InvalidArgumentException('Invalid scope'); + throw new InvalidArgumentException('Invalid scope'); } $this->scope = $newScope; return $this; @@ -178,4 +181,20 @@ public function setVerificationData(string $verificationData): IAccountProperty public function getVerificationData(): string { return $this->verificationData; } + + public function setLocallyVerified(string $verified): IAccountProperty { + if (!in_array($verified, [ + IAccountManager::NOT_VERIFIED, + IAccountManager::VERIFICATION_IN_PROGRESS, + IAccountManager::VERIFIED, + ])) { + throw new InvalidArgumentException('Provided verification value is invalid'); + } + $this->locallyVerified = $verified; + return $this; + } + + public function getLocallyVerified(): string { + return $this->locallyVerified; + } } diff --git a/lib/private/Accounts/AccountPropertyCollection.php b/lib/private/Accounts/AccountPropertyCollection.php index eb92536a6a0a5..3aed76d8746a9 100644 --- a/lib/private/Accounts/AccountPropertyCollection.php +++ b/lib/private/Accounts/AccountPropertyCollection.php @@ -84,6 +84,15 @@ public function removeProperty(IAccountProperty $property): IAccountPropertyColl return $this; } + public function getPropertyByValue(string $value): ?IAccountProperty { + foreach ($this->properties as $i => $property) { + if ($property->getValue() === $value) { + return $property; + } + } + return null; + } + public function removePropertyByValue(string $value): IAccountPropertyCollection { foreach ($this->properties as $i => $property) { if ($property->getValue() === $value) { diff --git a/lib/private/Server.php b/lib/private/Server.php index 03d6a4146ed0f..883bf3f5c2406 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -135,6 +135,7 @@ use OC\Security\Hasher; use OC\Security\SecureRandom; use OC\Security\TrustedDomainHelper; +use OC\Security\VerificationToken\VerificationToken; use OC\Session\CryptoWrapper; use OC\Share20\ProviderFactory; use OC\Share20\ShareHelper; @@ -224,6 +225,7 @@ use OCP\Security\ICrypto; use OCP\Security\IHasher; use OCP\Security\ISecureRandom; +use OCP\Security\VerificationToken\IVerificationToken; use OCP\Share\IShareHelper; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; @@ -795,6 +797,8 @@ public function __construct($webRoot, \OC\Config $config) { /** @deprecated 19.0.0 */ $this->registerDeprecatedAlias('SecureRandom', \OCP\Security\ISecureRandom::class); + $this->registerAlias(IVerificationToken::class, VerificationToken::class); + $this->registerAlias(ICrypto::class, Crypto::class); /** @deprecated 19.0.0 */ $this->registerDeprecatedAlias('Crypto', ICrypto::class); diff --git a/lib/public/Accounts/IAccountProperty.php b/lib/public/Accounts/IAccountProperty.php index 20505f299ddaa..94712e50f7d64 100644 --- a/lib/public/Accounts/IAccountProperty.php +++ b/lib/public/Accounts/IAccountProperty.php @@ -115,4 +115,24 @@ public function setVerificationData(string $verificationData): IAccountProperty; * @since 22.0.0 */ public function getVerificationData(): string; + + /** + * Set the instance-based verification status of a property + * + * @since 22.2.0 + * + * @param string $verified must be one of the verification constants of IAccountManager + * @return IAccountProperty + * @throws InvalidArgumentException + */ + public function setLocallyVerified(string $verified): IAccountProperty; + + /** + * Get the instance-based verification status of a property + * + * @since 22.2.0 + * + * @return string + */ + public function getLocallyVerified(): string; } diff --git a/lib/public/Accounts/IAccountPropertyCollection.php b/lib/public/Accounts/IAccountPropertyCollection.php index 779fb1299b4cd..6ee3fed0acb53 100644 --- a/lib/public/Accounts/IAccountPropertyCollection.php +++ b/lib/public/Accounts/IAccountPropertyCollection.php @@ -89,4 +89,13 @@ public function removeProperty(IAccountProperty $property): IAccountPropertyColl * @since 22.0.0 */ public function removePropertyByValue(string $value): IAccountPropertyCollection; + + /** + * retrieves a property identified by its value. null, if none was found. + * + * Returns only the first property if there are more with the same value. + * + * @since 22.2.0 + */ + public function getPropertyByValue(string $value): ?IAccountProperty; } diff --git a/tests/lib/Accounts/AccountManagerTest.php b/tests/lib/Accounts/AccountManagerTest.php index 8ed0e29d7ce53..bf79d233131eb 100644 --- a/tests/lib/Accounts/AccountManagerTest.php +++ b/tests/lib/Accounts/AccountManagerTest.php @@ -25,9 +25,15 @@ use OC\Accounts\AccountManager; use OCP\Accounts\IAccountManager; use OCP\BackgroundJob\IJobList; +use OCP\Defaults; use OCP\IConfig; use OCP\IDBConnection; +use OCP\IURLGenerator; use OCP\IUser; +use OCP\L10N\IFactory; +use OCP\Mail\IMailer; +use OCP\Security\ICrypto; +use OCP\Security\VerificationToken\IVerificationToken; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Symfony\Component\EventDispatcher\EventDispatcherInterface; @@ -41,6 +47,18 @@ * @package Test\Accounts */ class AccountManagerTest extends TestCase { + /** @var IVerificationToken|MockObject */ + protected $verificationToken; + /** @var IMailer|MockObject */ + protected $mailer; + /** @var ICrypto|MockObject */ + protected $crypto; + /** @var IURLGenerator|MockObject */ + protected $urlGenerator; + /** @var Defaults|MockObject */ + protected $defaults; + /** @var IFactory|MockObject */ + protected $l10nFactory; /** @var \OCP\IDBConnection */ private $connection; @@ -70,6 +88,12 @@ protected function setUp(): void { $this->config = $this->createMock(IConfig::class); $this->jobList = $this->createMock(IJobList::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->verificationToken = $this->createMock(IVerificationToken::class); + $this->mailer = $this->createMock(IMailer::class); + $this->defaults = $this->createMock(Defaults::class); + $this->l10nFactory = $this->createMock(IFactory::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->crypto = $this->createMock(ICrypto::class); $this->accountManager = new AccountManager( $this->connection, @@ -77,6 +101,12 @@ protected function setUp(): void { $this->eventDispatcher, $this->jobList, $this->logger, + $this->verificationToken, + $this->mailer, + $this->defaults, + $this->l10nFactory, + $this->urlGenerator, + $this->crypto ); } @@ -310,6 +340,12 @@ public function getInstance($mockedMethods = null) { $this->eventDispatcher, $this->jobList, $this->logger, + $this->verificationToken, + $this->mailer, + $this->defaults, + $this->l10nFactory, + $this->urlGenerator, + $this->crypto ]) ->setMethods($mockedMethods) ->getMock(); From 5c4c162d0ddf71c5b39660fe79d2eeba9c1e9cb5 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 24 Aug 2021 17:43:21 +0200 Subject: [PATCH 3/8] fix parameter type hint in phpdoc Signed-off-by: Arthur Schiwon --- lib/private/Mail/EMailTemplate.php | 2 +- lib/public/Mail/IEMailTemplate.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/Mail/EMailTemplate.php b/lib/private/Mail/EMailTemplate.php index efe1a6eef1d63..a83f7787829c5 100644 --- a/lib/private/Mail/EMailTemplate.php +++ b/lib/private/Mail/EMailTemplate.php @@ -568,7 +568,7 @@ public function addBodyButtonGroup(string $textLeft, * * @param string $text Text of button; Note: When $plainText falls back to this, HTML is automatically escaped in the HTML email * @param string $url URL of button - * @param string $plainText Text of button in plain text version + * @param string|false $plainText Text of button in plain text version * if empty the $text is used, if false none will be used * * @since 12.0.0 diff --git a/lib/public/Mail/IEMailTemplate.php b/lib/public/Mail/IEMailTemplate.php index 39d6661b62421..2d7765498148f 100644 --- a/lib/public/Mail/IEMailTemplate.php +++ b/lib/public/Mail/IEMailTemplate.php @@ -130,7 +130,7 @@ public function addBodyButtonGroup(string $textLeft, string $urlLeft, string $te * * @param string $text Text of button; Note: When $plainText falls back to this, HTML is automatically escaped in the HTML email * @param string $url URL of button - * @param string $plainText Text of button in plain text version + * @param string|false $plainText Text of button in plain text version * if empty the $text is used, if false none will be used * * @since 12.0.0 From b699e8f48799e8e48c2110872d37123603e3e432 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 25 Aug 2021 12:44:38 +0200 Subject: [PATCH 4/8] add a job to clean up expired verification tokens Signed-off-by: Arthur Schiwon --- core/Controller/LostController.php | 2 +- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../Security/VerificationToken/CleanUpJob.php | 90 +++++++++++++++++++ .../VerificationToken/VerificationToken.php | 22 ++++- .../VerificationToken/IVerificationToken.php | 2 +- .../VerificationTokenTest.php | 41 ++++++++- 7 files changed, 151 insertions(+), 8 deletions(-) create mode 100644 lib/private/Security/VerificationToken/CleanUpJob.php diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index 7a36e5970a6af..5471f56847331 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -172,7 +172,7 @@ public function resetform($token, $userId) { */ protected function checkPasswordResetToken(string $token, string $userId): void { try { - $this->verificationToken->check($token, $this->userManager->get($userId), 'lostpassword'); + $this->verificationToken->check($token, $this->userManager->get($userId), 'lostpassword', '', true); } catch (InvalidTokenException $e) { $error = $e->getCode() === InvalidTokenException::TOKEN_EXPIRED ? $this->l10n->t('Could not reset password because the token is expired') diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index b48b67104cf43..113ccf9306cba 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1373,6 +1373,7 @@ 'OC\\Security\\RateLimiting\\Limiter' => $baseDir . '/lib/private/Security/RateLimiting/Limiter.php', 'OC\\Security\\SecureRandom' => $baseDir . '/lib/private/Security/SecureRandom.php', 'OC\\Security\\TrustedDomainHelper' => $baseDir . '/lib/private/Security/TrustedDomainHelper.php', + 'OC\\Security\\VerificationToken\\CleanUpJob' => $baseDir . '/lib/private/Security/VerificationToken/CleanUpJob.php', 'OC\\Security\\VerificationToken\\VerificationToken' => $baseDir . '/lib/private/Security/VerificationToken/VerificationToken.php', 'OC\\Server' => $baseDir . '/lib/private/Server.php', 'OC\\ServerContainer' => $baseDir . '/lib/private/ServerContainer.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index e89d03d5e1303..4eb5ead207057 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1402,6 +1402,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\Security\\RateLimiting\\Limiter' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Limiter.php', 'OC\\Security\\SecureRandom' => __DIR__ . '/../../..' . '/lib/private/Security/SecureRandom.php', 'OC\\Security\\TrustedDomainHelper' => __DIR__ . '/../../..' . '/lib/private/Security/TrustedDomainHelper.php', + 'OC\\Security\\VerificationToken\\CleanUpJob' => __DIR__ . '/../../..' . '/lib/private/Security/VerificationToken/CleanUpJob.php', 'OC\\Security\\VerificationToken\\VerificationToken' => __DIR__ . '/../../..' . '/lib/private/Security/VerificationToken/VerificationToken.php', 'OC\\Server' => __DIR__ . '/../../..' . '/lib/private/Server.php', 'OC\\ServerContainer' => __DIR__ . '/../../..' . '/lib/private/ServerContainer.php', diff --git a/lib/private/Security/VerificationToken/CleanUpJob.php b/lib/private/Security/VerificationToken/CleanUpJob.php new file mode 100644 index 0000000000000..331172898ec1e --- /dev/null +++ b/lib/private/Security/VerificationToken/CleanUpJob.php @@ -0,0 +1,90 @@ + + * + * @author Arthur Schiwon + * + * @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 OC\Security\VerificationToken; + +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IConfig; +use OCP\ILogger; +use OCP\IUserManager; +use OCP\Security\VerificationToken\InvalidTokenException; +use OCP\Security\VerificationToken\IVerificationToken; + +class CleanUpJob extends \OCP\BackgroundJob\Job { + + /** @var int */ + protected $runNotBefore; + /** @var string */ + protected $userId; + /** @var string */ + protected $subject; + /** @var string */ + protected $pwdPrefix; + /** @var IConfig */ + private $config; + /** @var IVerificationToken */ + private $verificationToken; + /** @var IUserManager */ + private $userManager; + + public function __construct(ITimeFactory $time, IConfig $config, IVerificationToken $verificationToken, IUserManager $userManager) { + parent::__construct($time); + $this->config = $config; + $this->verificationToken = $verificationToken; + $this->userManager = $userManager; + } + + public function setArgument($argument) { + parent::setArgument($argument); + $args = \json_decode($argument); + $this->userId = (string)$args['userId']; + $this->subject = (string)$args['subject']; + $this->pwdPrefix = (string)$args['pp']; + $this->runNotBefore = (int)$args['notBefore']; + } + + protected function run($argument) { + try { + $user = $this->userManager->get($this->userId); + if ($user === null) { + return; + } + $this->verificationToken->check('irrelevant', $user, $this->subject, $this->pwdPrefix); + } catch (InvalidTokenException $e) { + if ($e->getCode() === InvalidTokenException::TOKEN_EXPIRED) { + // make sure to only remove expired tokens + $this->config->deleteUserValue($this->userId, 'core', $this->subject); + } + } + } + + public function execute($jobList, ILogger $logger = null) { + if ($this->time->getTime() >= $this->runNotBefore) { + $jobList->remove($this, $this->argument); + parent::execute($jobList, $logger); + } + } +} diff --git a/lib/private/Security/VerificationToken/VerificationToken.php b/lib/private/Security/VerificationToken/VerificationToken.php index 4ac5605eecfd9..ff3cb90727a09 100644 --- a/lib/private/Security/VerificationToken/VerificationToken.php +++ b/lib/private/Security/VerificationToken/VerificationToken.php @@ -27,14 +27,17 @@ namespace OC\Security\VerificationToken; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\BackgroundJob\IJobList; use OCP\IConfig; use OCP\IUser; use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; use OCP\Security\VerificationToken\InvalidTokenException; use OCP\Security\VerificationToken\IVerificationToken; +use function json_encode; class VerificationToken implements IVerificationToken { + protected const TOKEN_LIFETIME = 60 * 60 * 24 * 7; /** @var IConfig */ private $config; @@ -44,17 +47,21 @@ class VerificationToken implements IVerificationToken { private $timeFactory; /** @var ISecureRandom */ private $secureRandom; + /** @var IJobList */ + private $jobList; public function __construct( IConfig $config, ICrypto $crypto, ITimeFactory $timeFactory, - ISecureRandom $secureRandom + ISecureRandom $secureRandom, + IJobList $jobList ) { $this->config = $config; $this->crypto = $crypto; $this->timeFactory = $timeFactory; $this->secureRandom = $secureRandom; + $this->jobList = $jobList; } /** @@ -64,7 +71,7 @@ protected function throwInvalidTokenException(int $code): void { throw new InvalidTokenException($code); } - public function check(string $token, ?IUser $user, string $subject, string $passwordPrefix = ''): void { + public function check(string $token, ?IUser $user, string $subject, string $passwordPrefix = '', bool $expiresWithLogin = false): void { if ($user === null || !$user->isEnabled()) { $this->throwInvalidTokenException(InvalidTokenException::USER_UNKNOWN); } @@ -85,8 +92,8 @@ public function check(string $token, ?IUser $user, string $subject, string $pass $this->throwInvalidTokenException(InvalidTokenException::TOKEN_INVALID_FORMAT); } - if ($splitToken[0] < ($this->timeFactory->getTime() - 60 * 60 * 24 * 7) || - $user->getLastLogin() > $splitToken[0]) { + if ($splitToken[0] < ($this->timeFactory->getTime() - self::TOKEN_LIFETIME) + || ($expiresWithLogin && $user->getLastLogin() > $splitToken[0])) { $this->throwInvalidTokenException(InvalidTokenException::TOKEN_EXPIRED); } @@ -105,6 +112,13 @@ public function create(IUser $user, string $subject, string $passwordPrefix = '' $tokenValue = $this->timeFactory->getTime() .':'. $token; $encryptedValue = $this->crypto->encrypt($tokenValue, $passwordPrefix . $this->config->getSystemValue('secret')); $this->config->setUserValue($user->getUID(), 'core', $subject, $encryptedValue); + $jobArgs = json_encode([ + 'userId' => $user->getUID(), + 'subject' => $subject, + 'pp' => $passwordPrefix, + 'notBefore' => $this->timeFactory->getTime() + self::TOKEN_LIFETIME * 2, // multiply to provide a grace period + ]); + $this->jobList->add(CleanUpJob::class, $jobArgs); return $token; } diff --git a/lib/public/Security/VerificationToken/IVerificationToken.php b/lib/public/Security/VerificationToken/IVerificationToken.php index 0cd10377a970c..cbcae69f833d8 100644 --- a/lib/public/Security/VerificationToken/IVerificationToken.php +++ b/lib/public/Security/VerificationToken/IVerificationToken.php @@ -46,7 +46,7 @@ interface IVerificationToken { * @throws InvalidTokenException * @since 22.2.0 */ - public function check(string $token, ?IUser $user, string $subject, string $passwordPrefix = ''): void; + public function check(string $token, ?IUser $user, string $subject, string $passwordPrefix = '', bool $expiresWithLogin = false): void; /** * @since 22.2.0 diff --git a/tests/lib/Security/VerificationToken/VerificationTokenTest.php b/tests/lib/Security/VerificationToken/VerificationTokenTest.php index d1faf18dd8f2a..4d90e304ab717 100644 --- a/tests/lib/Security/VerificationToken/VerificationTokenTest.php +++ b/tests/lib/Security/VerificationToken/VerificationTokenTest.php @@ -28,6 +28,7 @@ use OC\Security\VerificationToken\VerificationToken; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\BackgroundJob\IJobList; use OCP\IConfig; use OCP\IUser; use OCP\Security\ICrypto; @@ -54,12 +55,14 @@ protected function setUp(): void { $this->crypto = $this->createMock(ICrypto::class); $this->timeFactory = $this->createMock(ITimeFactory::class); $this->secureRandom = $this->createMock(ISecureRandom::class); + $this->jobList = $this->createMock(IJobList::class); $this->token = new VerificationToken( $this->config, $this->crypto, $this->timeFactory, - $this->secureRandom + $this->secureRandom, + $this->jobList ); } @@ -177,13 +180,47 @@ public function testTokenExpired() { $this->timeFactory->expects($this->any()) ->method('getTime') - ->willReturn(604801); + ->willReturn(604800 * 3); $this->expectException(InvalidTokenException::class); $this->expectExceptionCode(InvalidTokenException::TOKEN_EXPIRED); $this->token->check('encryptedToken', $user, 'fingerprintToken', 'foobar'); } + public function testTokenExpiredByLogin() { + $user = $this->createMock(IUser::class); + $user->expects($this->atLeastOnce()) + ->method('isEnabled') + ->willReturn(true); + $user->expects($this->atLeastOnce()) + ->method('getUID') + ->willReturn('alice'); + $user->expects($this->any()) + ->method('getLastLogin') + ->willReturn(604803); + + $this->config->expects($this->atLeastOnce()) + ->method('getUserValue') + ->with('alice', 'core', 'fingerprintToken', null) + ->willReturn('encryptedToken'); + $this->config->expects($this->any()) + ->method('getSystemValue') + ->with('secret') + ->willReturn('357111317'); + + $this->crypto->method('decrypt') + ->with('encryptedToken', 'foobar' . '357111317') + ->willReturn('604800:mY70K3n'); + + $this->timeFactory->expects($this->any()) + ->method('getTime') + ->willReturn(604801); + + $this->expectException(InvalidTokenException::class); + $this->expectExceptionCode(InvalidTokenException::TOKEN_EXPIRED); + $this->token->check('encryptedToken', $user, 'fingerprintToken', 'foobar', true); + } + public function testTokenMismatch() { $user = $this->createMock(IUser::class); $user->expects($this->atLeastOnce()) From 414676468679ba93befde00322fcf2b8af9f3d2d Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Sat, 28 Aug 2021 00:07:23 +0200 Subject: [PATCH 5/8] Confirm mails only per POST - this is to avoid automatic confirmation by certain softwares that open links Signed-off-by: Arthur Schiwon --- apps/provisioning_api/appinfo/routes.php | 3 ++- .../lib/Controller/VerificationController.php | 26 +++++++++++++++++-- core/templates/confirmation.php | 20 ++++++++++++++ .../VerificationToken/VerificationToken.php | 4 +++ .../VerificationToken/IVerificationToken.php | 7 +++++ 5 files changed, 57 insertions(+), 3 deletions(-) create mode 100644 core/templates/confirmation.php diff --git a/apps/provisioning_api/appinfo/routes.php b/apps/provisioning_api/appinfo/routes.php index 81a5bb94e02f2..54d550260b8f7 100644 --- a/apps/provisioning_api/appinfo/routes.php +++ b/apps/provisioning_api/appinfo/routes.php @@ -76,6 +76,7 @@ ], 'routes' => [ // Verification - ['name' => 'Verification#verifyMail', 'url' => '/mailVerification/{key}/{token}/{userId}', 'verb' => 'GET'], + ['name' => 'Verification#showVerifyMail', 'url' => '/mailVerification/{key}/{token}/{userId}', 'verb' => 'GET'], + ['name' => 'Verification#verifyMail', 'url' => '/mailVerification/{key}/{token}/{userId}', 'verb' => 'POST'], ] ]; diff --git a/apps/provisioning_api/lib/Controller/VerificationController.php b/apps/provisioning_api/lib/Controller/VerificationController.php index b248d3e828575..c4ddd1e644d6a 100644 --- a/apps/provisioning_api/lib/Controller/VerificationController.php +++ b/apps/provisioning_api/lib/Controller/VerificationController.php @@ -74,6 +74,27 @@ public function __construct( /** * @NoCSRFRequired + * @NoAdminRequired + * @NoSubAdminRequired + */ + public function showVerifyMail(string $token, string $userId, string $key) { + if ($this->userSession->getUser()->getUID() !== $userId) { + // not a public page, hence getUser() must return an IUser + throw new InvalidArgumentException('Logged in user is not mail address owner'); + } + $email = $this->crypto->decrypt($key); + + return new TemplateResponse( + 'core', 'confirmation', [ + 'title' => $this->l10n->t('Email confirmation'), + 'message' => $this->l10n->t('To enable the email address %s please click the button below.', [$email]), + 'action' => $this->l10n->t('Confirm'), + ], TemplateResponse::RENDER_AS_GUEST); + } + + /** + * @NoAdminRequired + * @NoSubAdminRequired */ public function verifyMail(string $token, string $userId, string $key) { try { @@ -95,6 +116,7 @@ public function verifyMail(string $token, string $userId, string $key) { } $emailProperty->setLocallyVerified(IAccountManager::VERIFIED); $this->accountManager->updateAccount($userAccount); + $this->verificationToken->delete($token, $user, 'verifyMail' . $ref); } catch (InvalidTokenException $e) { $error = $e->getCode() === InvalidTokenException::TOKEN_EXPIRED ? $this->l10n->t('Could not verify mail because the token is expired.') @@ -109,13 +131,13 @@ public function verifyMail(string $token, string $userId, string $key) { return new TemplateResponse( 'core', 'error', [ 'errors' => [['error' => $error]] - ], 'guest'); + ], TemplateResponse::RENDER_AS_GUEST); } return new TemplateResponse( 'core', 'success', [ 'title' => $this->l10n->t('Email confirmation successful'), 'message' => $this->l10n->t('Email confirmation successful'), - ], 'guest'); + ], TemplateResponse::RENDER_AS_GUEST); } } diff --git a/core/templates/confirmation.php b/core/templates/confirmation.php new file mode 100644 index 0000000000000..26014cd1e79a5 --- /dev/null +++ b/core/templates/confirmation.php @@ -0,0 +1,20 @@ + + +
+
+

+

+
+ +
+ $value) {?> + + + +
+
diff --git a/lib/private/Security/VerificationToken/VerificationToken.php b/lib/private/Security/VerificationToken/VerificationToken.php index ff3cb90727a09..c85e0e7b5a137 100644 --- a/lib/private/Security/VerificationToken/VerificationToken.php +++ b/lib/private/Security/VerificationToken/VerificationToken.php @@ -122,4 +122,8 @@ public function create(IUser $user, string $subject, string $passwordPrefix = '' return $token; } + + public function delete(string $token, IUser $user, string $subject): void { + $this->config->deleteUserValue($user->getUID(), 'core', $subject); + } } diff --git a/lib/public/Security/VerificationToken/IVerificationToken.php b/lib/public/Security/VerificationToken/IVerificationToken.php index cbcae69f833d8..584928426de2d 100644 --- a/lib/public/Security/VerificationToken/IVerificationToken.php +++ b/lib/public/Security/VerificationToken/IVerificationToken.php @@ -52,4 +52,11 @@ public function check(string $token, ?IUser $user, string $subject, string $pass * @since 22.2.0 */ public function create(IUser $user, string $subject, string $passwordPrefix = ''): string; + + /** + * Deletes the token identified by the provided parameters + * + * @since 22.2.0 + */ + public function delete(string $token, IUser $user, string $subject): void; } From 88fc177e2672d3fba5451e351967301f42cf3028 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 1 Sep 2021 14:04:57 +0200 Subject: [PATCH 6/8] enable the user to set a primary (notification) email address (backend) - specific getters and setters on IUser and implementation - new notify_email field in provisioning API Signed-off-by: Arthur Schiwon --- .../lib/Controller/AUserData.php | 18 +++-- .../lib/Controller/UsersController.php | 66 ++++++++++------ lib/private/Setup.php | 2 +- lib/private/User/Manager.php | 1 + lib/private/User/User.php | 77 ++++++++++++++++--- lib/public/IUser.php | 63 ++++++++++++++- lib/public/IUserManager.php | 2 + tests/lib/User/UserTest.php | 5 +- 8 files changed, 193 insertions(+), 41 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index e358d28206186..83ad887be7737 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -54,6 +54,13 @@ abstract class AUserData extends OCSController { public const SCOPE_SUFFIX = 'Scope'; + public const USER_FIELD_DISPLAYNAME = 'display'; + public const USER_FIELD_LANGUAGE = 'language'; + public const USER_FIELD_LOCALE = 'locale'; + public const USER_FIELD_PASSWORD = 'password'; + public const USER_FIELD_QUOTA = 'quota'; + public const USER_FIELD_NOTIFICATION_EMAIL = 'notify_email'; + /** @var IUserManager */ protected $userManager; /** @var IConfig */ @@ -139,7 +146,7 @@ protected function getUserData(string $userId, bool $includeScopes = false): arr $data['lastLogin'] = $targetUserObject->getLastLogin() * 1000; $data['backend'] = $targetUserObject->getBackendClassName(); $data['subadmin'] = $this->getUserSubAdminGroupsData($targetUserObject->getUID()); - $data['quota'] = $this->fillStorageInfo($targetUserObject->getUID()); + $data[self::USER_FIELD_QUOTA] = $this->fillStorageInfo($targetUserObject->getUID()); try { if ($includeScopes) { @@ -187,8 +194,9 @@ protected function getUserData(string $userId, bool $includeScopes = false): arr } $data['groups'] = $gids; - $data['language'] = $this->l10nFactory->getUserLanguage($targetUserObject); - $data['locale'] = $this->config->getUserValue($targetUserObject->getUID(), 'core', 'locale'); + $data[self::USER_FIELD_LANGUAGE] = $this->l10nFactory->getUserLanguage($targetUserObject); + $data[self::USER_FIELD_LOCALE] = $this->config->getUserValue($targetUserObject->getUID(), 'core', 'locale'); + $data[self::USER_FIELD_NOTIFICATION_EMAIL] = $targetUserObject->getPrimaryEMailAddress(); $backend = $targetUserObject->getBackend(); $data['backendCapabilities'] = [ @@ -238,7 +246,7 @@ protected function fillStorageInfo(string $userId): array { 'used' => $storage['used'], 'total' => $storage['total'], 'relative' => $storage['relative'], - 'quota' => $storage['quota'], + self::USER_FIELD_QUOTA => $storage['quota'], ]; } catch (NotFoundException $ex) { // User fs is not setup yet @@ -251,7 +259,7 @@ protected function fillStorageInfo(string $userId): array { $quota = OC_Helper::computerFileSize($quota); } $data = [ - 'quota' => $quota !== false ? $quota : 'none', + self::USER_FIELD_QUOTA => $quota !== false ? $quota : 'none', 'used' => 0 ]; } diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 114dfcb074ab6..8479100433b14 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -42,6 +42,7 @@ */ namespace OCA\Provisioning_API\Controller; +use InvalidArgumentException; use libphonenumber\NumberParseException; use libphonenumber\PhoneNumber; use libphonenumber\PhoneNumberFormat; @@ -418,15 +419,15 @@ public function addUser(string $userid, } if ($displayName !== '') { - $this->editUser($userid, 'display', $displayName); + $this->editUser($userid, self::USER_FIELD_DISPLAYNAME, $displayName); } if ($quota !== '') { - $this->editUser($userid, 'quota', $quota); + $this->editUser($userid, self::USER_FIELD_QUOTA, $quota); } if ($language !== '') { - $this->editUser($userid, 'language', $language); + $this->editUser($userid, self::USER_FIELD_LANGUAGE, $language); } // Send new user mail only if a mail is set @@ -466,7 +467,7 @@ public function addUser(string $userid, ] ); throw $e; - } catch (\InvalidArgumentException $e) { + } catch (InvalidArgumentException $e) { $this->logger->error('Failed addUser attempt with invalid argument exeption.', [ 'app' => 'ocs_api', @@ -676,7 +677,7 @@ public function editUserMultiValue( try { $targetProperty->setScope($value); $this->accountManager->updateAccount($userAccount); - } catch (\InvalidArgumentException $e) { + } catch (InvalidArgumentException $e) { throw new OCSException('', 102); } } else { @@ -717,7 +718,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) { if ($targetUser->getBackend() instanceof ISetDisplayNameBackend || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME)) { - $permittedFields[] = 'display'; + $permittedFields[] = self::USER_FIELD_DISPLAYNAME; $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; } $permittedFields[] = IAccountManager::PROPERTY_EMAIL; @@ -728,15 +729,16 @@ public function editUser(string $userId, string $key, string $value): DataRespon $permittedFields[] = IAccountManager::COLLECTION_EMAIL; - $permittedFields[] = 'password'; + $permittedFields[] = self::USER_FIELD_PASSWORD; + $permittedFields[] = self::USER_FIELD_NOTIFICATION_EMAIL; if ($this->config->getSystemValue('force_language', false) === false || $this->groupManager->isAdmin($currentLoggedInUser->getUID())) { - $permittedFields[] = 'language'; + $permittedFields[] = self::USER_FIELD_LANGUAGE; } if ($this->config->getSystemValue('force_locale', false) === false || $this->groupManager->isAdmin($currentLoggedInUser->getUID())) { - $permittedFields[] = 'locale'; + $permittedFields[] = self::USER_FIELD_LOCALE; } $permittedFields[] = IAccountManager::PROPERTY_PHONE; @@ -752,7 +754,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon // If admin they can edit their own quota if ($this->groupManager->isAdmin($currentLoggedInUser->getUID())) { - $permittedFields[] = 'quota'; + $permittedFields[] = self::USER_FIELD_QUOTA; } } else { // Check if admin / subadmin @@ -762,19 +764,19 @@ public function editUser(string $userId, string $key, string $value): DataRespon // They have permissions over the user if ($targetUser->getBackend() instanceof ISetDisplayNameBackend || $targetUser->getBackend()->implementsActions(Backend::SET_DISPLAYNAME)) { - $permittedFields[] = 'display'; + $permittedFields[] = self::USER_FIELD_DISPLAYNAME; $permittedFields[] = IAccountManager::PROPERTY_DISPLAYNAME; } $permittedFields[] = IAccountManager::PROPERTY_EMAIL; $permittedFields[] = IAccountManager::COLLECTION_EMAIL; - $permittedFields[] = 'password'; - $permittedFields[] = 'language'; - $permittedFields[] = 'locale'; + $permittedFields[] = self::USER_FIELD_PASSWORD; + $permittedFields[] = self::USER_FIELD_LANGUAGE; + $permittedFields[] = self::USER_FIELD_LOCALE; $permittedFields[] = IAccountManager::PROPERTY_PHONE; $permittedFields[] = IAccountManager::PROPERTY_ADDRESS; $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; $permittedFields[] = IAccountManager::PROPERTY_TWITTER; - $permittedFields[] = 'quota'; + $permittedFields[] = self::USER_FIELD_QUOTA; } else { // No rights throw new OCSException('', OCSController::RESPOND_NOT_FOUND); @@ -786,11 +788,11 @@ public function editUser(string $userId, string $key, string $value): DataRespon } // Process the edit switch ($key) { - case 'display': + case self::USER_FIELD_DISPLAYNAME: case IAccountManager::PROPERTY_DISPLAYNAME: $targetUser->setDisplayName($value); break; - case 'quota': + case self::USER_FIELD_QUOTA: $quota = $value; if ($quota !== 'none' && $quota !== 'default') { if (is_numeric($quota)) { @@ -820,7 +822,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon } $targetUser->setQuota($quota); break; - case 'password': + case self::USER_FIELD_PASSWORD: try { if (!$targetUser->canChangePassword()) { throw new OCSException('Setting the password is not supported by the users backend', 103); @@ -830,19 +832,39 @@ public function editUser(string $userId, string $key, string $value): DataRespon throw new OCSException($e->getMessage(), 103); } break; - case 'language': + case self::USER_FIELD_LANGUAGE: $languagesCodes = $this->l10nFactory->findAvailableLanguages(); if (!in_array($value, $languagesCodes, true) && $value !== 'en') { throw new OCSException('Invalid language', 102); } $this->config->setUserValue($targetUser->getUID(), 'core', 'lang', $value); break; - case 'locale': + case self::USER_FIELD_LOCALE: if (!$this->l10nFactory->localeExists($value)) { throw new OCSException('Invalid locale', 102); } $this->config->setUserValue($targetUser->getUID(), 'core', 'locale', $value); break; + case self::USER_FIELD_NOTIFICATION_EMAIL: + $success = false; + if ($value === '' || filter_var($value, FILTER_VALIDATE_EMAIL)) { + try { + $targetUser->setPrimaryEMailAddress($value); + $success = true; + } catch (InvalidArgumentException $e) { + $this->logger->info( + 'Cannot set primary email, because provided address is not verified', + [ + 'app' => 'provisioning_api', + 'exception' => $e, + ] + ); + } + } + if (!$success) { + throw new OCSException('', 102); + } + break; case IAccountManager::PROPERTY_EMAIL: if (filter_var($value, FILTER_VALIDATE_EMAIL) || $value === '') { $targetUser->setEMailAddress($value); @@ -878,7 +900,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon if ($userProperty->getName() === IAccountManager::PROPERTY_PHONE) { $this->knownUserService->deleteByContactUserId($targetUser->getUID()); } - } catch (\InvalidArgumentException $e) { + } catch (InvalidArgumentException $e) { throw new OCSException('Invalid ' . $e->getMessage(), 102); } } @@ -901,7 +923,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon try { $userProperty->setScope($value); $this->accountManager->updateAccount($userAccount); - } catch (\InvalidArgumentException $e) { + } catch (InvalidArgumentException $e) { throw new OCSException('Invalid ' . $e->getMessage(), 102); } } diff --git a/lib/private/Setup.php b/lib/private/Setup.php index 8e7e6ae6aea19..cb0d812c8855e 100644 --- a/lib/private/Setup.php +++ b/lib/private/Setup.php @@ -436,7 +436,7 @@ public function install($options) { // Set email for admin if (!empty($options['adminemail'])) { - $config->setUserValue($user->getUID(), 'settings', 'email', $options['adminemail']); + $user->setSystemEMailAddress($options['adminemail']); } } diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php index 0d340ad356e67..bebb9c8c72c24 100644 --- a/lib/private/User/Manager.php +++ b/lib/private/User/Manager.php @@ -700,6 +700,7 @@ private function getSeenUserIds($limit = null, $offset = null) { * @since 9.1.0 */ public function getByEmail($email) { + // looking for 'email' only (and not primary_mail) is intentional $userIds = $this->config->getUsersForUserValueCaseInsensitive('settings', 'email', $email); $users = array_map(function ($uid) { diff --git a/lib/private/User/User.php b/lib/private/User/User.php index f17824f51b9aa..5fa1272f95cc8 100644 --- a/lib/private/User/User.php +++ b/lib/private/User/User.php @@ -34,10 +34,12 @@ */ namespace OC\User; +use InvalidArgumentException; use OC\Accounts\AccountManager; use OC\Avatar\AvatarManager; use OC\Hooks\Emitter; use OC_Helper; +use OCP\Accounts\IAccountManager; use OCP\EventDispatcher\IEventDispatcher; use OCP\Group\Events\BeforeUserRemovedEvent; use OCP\Group\Events\UserRemovedEvent; @@ -55,6 +57,8 @@ use Symfony\Component\EventDispatcher\GenericEvent; class User implements IUser { + /** @var IAccountManager */ + protected $accountManager; /** @var string */ private $uid; @@ -165,24 +169,61 @@ public function setDisplayName($displayName) { } /** - * set the email address of the user - * - * @param string|null $mailAddress - * @return void - * @since 9.0.0 + * @inheritDoc */ public function setEMailAddress($mailAddress) { - $oldMailAddress = $this->getEMailAddress(); + $this->setSystemEMailAddress($mailAddress); + } + + /** + * @inheritDoc + */ + public function setSystemEMailAddress(string $mailAddress): void { + $oldMailAddress = $this->getSystemEMailAddress(); + + if ($mailAddress === '') { + $this->config->deleteUserValue($this->uid, 'settings', 'email'); + } else { + $this->config->setUserValue($this->uid, 'settings', 'email', $mailAddress); + } + + $primaryAddress = $this->getPrimaryEMailAddress(); + if ($primaryAddress === $mailAddress) { + // on match no dedicated primary settings is necessary + $this->setPrimaryEMailAddress(''); + } + if ($oldMailAddress !== $mailAddress) { - if ($mailAddress === '') { - $this->config->deleteUserValue($this->uid, 'settings', 'email'); - } else { - $this->config->setUserValue($this->uid, 'settings', 'email', $mailAddress); - } $this->triggerChange('eMailAddress', $mailAddress, $oldMailAddress); } } + /** + * @inheritDoc + */ + public function setPrimaryEMailAddress(string $mailAddress): void { + if ($mailAddress === '') { + $this->config->deleteUserValue($this->uid, 'settings', 'primary_email'); + return; + } + + $this->ensureAccountManager(); + $account = $this->accountManager->getAccount($this); + $property = $account->getPropertyCollection(IAccountManager::COLLECTION_EMAIL) + ->getPropertyByValue($mailAddress); + + if ($property === null || $property->getLocallyVerified() !== IAccountManager::VERIFIED) { + throw new InvalidArgumentException('Only verified emails can be set as primary'); + } + $this->config->setUserValue($this->uid, 'settings', 'primary_email', $mailAddress); + } + + private function ensureAccountManager() { + if (!$this->accountManager instanceof IAccountManager) { + $this->accountManager = \OC::$server->get(IAccountManager::class); + } + } + /** * returns the timestamp of the user's last login or 0 if the user did never * login @@ -390,9 +431,23 @@ public function setEnabled(bool $enabled = true) { * @since 9.0.0 */ public function getEMailAddress() { + return $this->getPrimaryEMailAddress() ?? $this->getSystemEMailAddress(); + } + + /** + * @inheritDoc + */ + public function getSystemEMailAddress(): ?string { return $this->config->getUserValue($this->uid, 'settings', 'email', null); } + /** + * @inheritDoc + */ + public function getPrimaryEMailAddress(): ?string { + return $this->config->getUserValue($this->uid, 'settings', 'primary_email', null); + } + /** * get the users' quota * diff --git a/lib/public/IUser.php b/lib/public/IUser.php index 7e75704ed5bc0..614748f6e28e0 100644 --- a/lib/public/IUser.php +++ b/lib/public/IUser.php @@ -27,6 +27,8 @@ */ namespace OCP; +use InvalidArgumentException; + /** * Interface IUser * @@ -157,13 +159,42 @@ public function isEnabled(); public function setEnabled(bool $enabled = true); /** - * get the users email address + * get the user's email address * * @return string|null * @since 9.0.0 */ public function getEMailAddress(); + /** + * get the user's system email address + * + * The system mail address may be read only and may be set from different + * sources like LDAP, SAML or simply the admin. + * + * Use this getter only when the system address is needed. For picking the + * proper address to e.g. send a mail to, use getEMailAddress(). + * + * @return string|null + * @since 22.2.0 + */ + public function getSystemEMailAddress(): ?string; + + /** + * get the user's preferred email address + * + * The primary mail address may be set be the user to specify a different + * email address where mails by Nextcloud are sent to. It is not necessarily + * set. + * + * Use this getter only when the primary address is needed. For picking the + * proper address to e.g. send a mail to, use getEMailAddress(). + * + * @return string|null + * @since 22.2.0 + */ + public function getPrimaryEMailAddress(): ?string; + /** * get the avatar image if it exists * @@ -184,12 +215,42 @@ public function getCloudId(); /** * set the email address of the user * + * It is an alias to setSystemEMailAddress() + * * @param string|null $mailAddress * @return void * @since 9.0.0 + * @deprecated 22.2.0 use setSystemEMailAddress() or setPrimaryEMailAddress() */ public function setEMailAddress($mailAddress); + /** + * Set the system email address of the user + * + * This is supposed to be used when the email is set from different sources + * (i.e. other user backends, admin). + * + * @since 22.2.0 + */ + public function setSystemEMailAddress(string $mailAddress): void; + + /** + * Set the primary email address of the user. + * + * This method should be typically called when the user is changing their + * own primary address and is not allowed to change their system email. + * + * The mail address provided here must be already registered as an + * additional mail in the user account and also be verified locally. Also + * an empty string is allowed to delete this preference. + * + * @throws InvalidArgumentException when the provided email address does not + * satisfy constraints. + * + * @since 22.2.0 + */ + public function setPrimaryEMailAddress(string $mailAddress): void; + /** * get the users' quota in human readable form. If a specific quota is not * set for the user, the default value is returned. If a default setting diff --git a/lib/public/IUserManager.php b/lib/public/IUserManager.php index c6cad6f0549db..e5c220af40c60 100644 --- a/lib/public/IUserManager.php +++ b/lib/public/IUserManager.php @@ -196,6 +196,8 @@ public function countSeenUsers(); public function callForSeenUsers(\Closure $callback); /** + * returns all users having the provided email set as system email address + * * @param string $email * @return IUser[] * @since 9.1.0 diff --git a/tests/lib/User/UserTest.php b/tests/lib/User/UserTest.php index 2366bf4532103..ad8b01555eadc 100644 --- a/tests/lib/User/UserTest.php +++ b/tests/lib/User/UserTest.php @@ -676,11 +676,14 @@ public function testSetEMailAddressNoChange() { $emitter->expects($this->never()) ->method('emit'); + $this->dispatcher->expects($this->never()) + ->method('dispatch'); + $config = $this->createMock(IConfig::class); $config->expects($this->any()) ->method('getUserValue') ->willReturn('foo@bar.com'); - $config->expects($this->never()) + $config->expects($this->any()) ->method('setUserValue'); $user = new User('foo', $backend, $this->dispatcher, $emitter, $config); From 0571d41df5aee3ccbfeb1ce573711e7f5add4b61 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 1 Sep 2021 15:41:02 +0200 Subject: [PATCH 7/8] use specific email getter where necessary Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/Principal.php | 7 ++-- apps/dav/lib/DAV/GroupPrincipalBackend.php | 2 +- .../unit/Connector/Sabre/PrincipalTest.php | 34 +++++++++---------- .../lib/Controller/ShareAPIController.php | 2 +- .../Controller/ShareAPIControllerTest.php | 8 ++--- .../lib/Controller/AUserData.php | 2 +- .../lib/Controller/UsersController.php | 2 +- .../tests/Controller/UsersControllerTest.php | 13 ++++--- .../lib/Controller/UsersController.php | 4 +-- .../tests/Controller/UsersControllerTest.php | 9 +++-- apps/user_ldap/lib/User/User.php | 2 +- core/Command/User/Info.php | 2 +- core/Command/User/ListCommand.php | 2 +- .../Collaborators/UserPlugin.php | 4 +-- tests/lib/AllConfigTest.php | 22 +++++++++++- 15 files changed, 67 insertions(+), 48 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/Principal.php b/apps/dav/lib/Connector/Sabre/Principal.php index 5e81e155d74f6..833943e86c531 100644 --- a/apps/dav/lib/Connector/Sabre/Principal.php +++ b/apps/dav/lib/Connector/Sabre/Principal.php @@ -295,16 +295,13 @@ protected function searchUserPrincipals(array $searchProperties, $test = 'allof' if (!$allowEnumeration) { if ($allowEnumerationFullMatch) { $users = $this->userManager->getByEmail($value); - $users = \array_filter($users, static function (IUser $user) use ($value) { - return $user->getEMailAddress() === $value; - }); } else { $users = []; } } else { $users = $this->userManager->getByEmail($value); $users = \array_filter($users, function (IUser $user) use ($currentUser, $value, $limitEnumerationPhone, $limitEnumerationGroup, $allowEnumerationFullMatch, $currentUserGroups) { - if ($allowEnumerationFullMatch && $user->getEMailAddress() === $value) { + if ($allowEnumerationFullMatch && $user->getSystemEMailAddress() === $value) { return true; } @@ -510,7 +507,7 @@ protected function userToPrincipal($user) { '{urn:ietf:params:xml:ns:caldav}calendar-user-type' => 'INDIVIDUAL', ]; - $email = $user->getEMailAddress(); + $email = $user->getSystemEMailAddress(); if (!empty($email)) { $principal['{http://sabredav.org/ns}email-address'] = $email; } diff --git a/apps/dav/lib/DAV/GroupPrincipalBackend.php b/apps/dav/lib/DAV/GroupPrincipalBackend.php index 34ac26d7d9725..6317fc59cc2e7 100644 --- a/apps/dav/lib/DAV/GroupPrincipalBackend.php +++ b/apps/dav/lib/DAV/GroupPrincipalBackend.php @@ -331,7 +331,7 @@ protected function userToPrincipal($user) { '{urn:ietf:params:xml:ns:caldav}calendar-user-type' => 'INDIVIDUAL', ]; - $email = $user->getEMailAddress(); + $email = $user->getSystemEMailAddress(); if (!empty($email)) { $principal['{http://sabredav.org/ns}email-address'] = $email; } diff --git a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php index acaa82dca8b5a..39bb41688dc05 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PrincipalTest.php @@ -113,7 +113,7 @@ public function testGetPrincipalsByPrefixWithUsers() { ->willReturn('Dr. Foo-Bar'); $fooUser ->expects($this->exactly(1)) - ->method('getEMailAddress') + ->method('getSystemEMailAddress') ->willReturn(''); $barUser = $this->createMock(User::class); $barUser @@ -122,7 +122,7 @@ public function testGetPrincipalsByPrefixWithUsers() { ->willReturn('bar'); $barUser ->expects($this->exactly(1)) - ->method('getEMailAddress') + ->method('getSystemEMailAddress') ->willReturn('bar@nextcloud.com'); $this->userManager ->expects($this->once()) @@ -183,7 +183,7 @@ public function testGetPrincipalsByPathWithMail() { $fooUser = $this->createMock(User::class); $fooUser ->expects($this->exactly(1)) - ->method('getEMailAddress') + ->method('getSystemEMailAddress') ->willReturn('foo@nextcloud.com'); $fooUser ->expects($this->exactly(1)) @@ -576,15 +576,15 @@ public function testSearchPrincipalWithEnumerationDisabledDisplayname() { $user2 = $this->createMock(IUser::class); $user2->method('getUID')->willReturn('user2'); $user2->method('getDisplayName')->willReturn('User 2'); - $user2->method('getEMailAddress')->willReturn('user2@foo.bar'); + $user2->method('getSystemEMailAddress')->willReturn('user2@foo.bar'); $user3 = $this->createMock(IUser::class); $user3->method('getUID')->willReturn('user3'); $user2->method('getDisplayName')->willReturn('User 22'); - $user2->method('getEMailAddress')->willReturn('user2@foo.bar123'); + $user2->method('getSystemEMailAddress')->willReturn('user2@foo.bar123'); $user4 = $this->createMock(IUser::class); $user4->method('getUID')->willReturn('user4'); $user2->method('getDisplayName')->willReturn('User 222'); - $user2->method('getEMailAddress')->willReturn('user2@foo.bar456'); + $user2->method('getSystemEMailAddress')->willReturn('user2@foo.bar456'); $this->userManager->expects($this->at(0)) ->method('searchDisplayName') @@ -636,20 +636,20 @@ public function testSearchPrincipalWithEnumerationDisabledEmail() { $user2 = $this->createMock(IUser::class); $user2->method('getUID')->willReturn('user2'); $user2->method('getDisplayName')->willReturn('User 2'); - $user2->method('getEMailAddress')->willReturn('user2@foo.bar'); + $user2->method('getSystemEMailAddress')->willReturn('user2@foo.bar'); $user3 = $this->createMock(IUser::class); $user3->method('getUID')->willReturn('user3'); $user2->method('getDisplayName')->willReturn('User 22'); - $user2->method('getEMailAddress')->willReturn('user2@foo.bar123'); + $user2->method('getSystemEMailAddress')->willReturn('user2@foo.bar123'); $user4 = $this->createMock(IUser::class); $user4->method('getUID')->willReturn('user4'); $user2->method('getDisplayName')->willReturn('User 222'); - $user2->method('getEMailAddress')->willReturn('user2@foo.bar456'); + $user2->method('getSystemEMailAddress')->willReturn('user2@foo.bar456'); - $this->userManager->expects($this->at(0)) + $this->userManager->expects($this->once()) ->method('getByEmail') ->with('user2@foo.bar') - ->willReturn([$user2, $user3, $user4]); + ->willReturn([$user2]); $this->assertEquals(['principals/users/user2'], $this->connector->searchPrincipals('principals/users', ['{http://sabredav.org/ns}email-address' => 'user2@foo.bar'])); @@ -697,15 +697,15 @@ public function testSearchPrincipalWithEnumerationLimitedDisplayname() { $user2 = $this->createMock(IUser::class); $user2->method('getUID')->willReturn('user2'); $user2->method('getDisplayName')->willReturn('User 2'); - $user2->method('getEMailAddress')->willReturn('user2@foo.bar'); + $user2->method('getSystemEMailAddress')->willReturn('user2@foo.bar'); $user3 = $this->createMock(IUser::class); $user3->method('getUID')->willReturn('user3'); $user3->method('getDisplayName')->willReturn('User 22'); - $user3->method('getEMailAddress')->willReturn('user2@foo.bar123'); + $user3->method('getSystemEMailAddress')->willReturn('user2@foo.bar123'); $user4 = $this->createMock(IUser::class); $user4->method('getUID')->willReturn('user4'); $user4->method('getDisplayName')->willReturn('User 222'); - $user4->method('getEMailAddress')->willReturn('user2@foo.bar456'); + $user4->method('getSystemEMailAddress')->willReturn('user2@foo.bar456'); $this->userSession->expects($this->at(0)) @@ -758,15 +758,15 @@ public function testSearchPrincipalWithEnumerationLimitedMail() { $user2 = $this->createMock(IUser::class); $user2->method('getUID')->willReturn('user2'); $user2->method('getDisplayName')->willReturn('User 2'); - $user2->method('getEMailAddress')->willReturn('user2@foo.bar'); + $user2->method('getSystemEMailAddress')->willReturn('user2@foo.bar'); $user3 = $this->createMock(IUser::class); $user3->method('getUID')->willReturn('user3'); $user3->method('getDisplayName')->willReturn('User 22'); - $user3->method('getEMailAddress')->willReturn('user2@foo.bar123'); + $user3->method('getSystemEMailAddress')->willReturn('user2@foo.bar123'); $user4 = $this->createMock(IUser::class); $user4->method('getUID')->willReturn('user4'); $user4->method('getDisplayName')->willReturn('User 222'); - $user4->method('getEMailAddress')->willReturn('user2@foo.bar456'); + $user4->method('getSystemEMailAddress')->willReturn('user2@foo.bar456'); $this->userSession->expects($this->at(0)) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index b323e5514a5c6..d82584ed4162c 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -238,7 +238,7 @@ protected function formatShare(IShare $share, Node $recipientNode = null): array $result['share_with'] = $share->getSharedWith(); $result['share_with_displayname'] = $sharedWith !== null ? $sharedWith->getDisplayName() : $share->getSharedWith(); $result['share_with_displayname_unique'] = $sharedWith !== null ? ( - $sharedWith->getEMailAddress() !== '' ? $sharedWith->getEMailAddress() : $sharedWith->getUID() + !empty($sharedWith->getSystemEMailAddress()) ? $sharedWith->getSystemEMailAddress() : $sharedWith->getUID() ) : $share->getSharedWith(); $result['status'] = []; diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index eff2d529a05f2..400291c0c1b6d 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -47,6 +47,7 @@ use OCP\Files\NotFoundException; use OCP\Files\Storage; use OCP\IConfig; +use OCP\IGroup; use OCP\IGroupManager; use OCP\IL10N; use OCP\IPreview; @@ -785,7 +786,7 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) { $user = $this->getMockBuilder(IUser::class)->getMock(); $user->method('getUID')->willReturn('userId'); $user->method('getDisplayName')->willReturn('userDisplay'); - $user->method('getEMailAddress')->willReturn('userId@example.com'); + $user->method('getSystemEMailAddress')->willReturn('userId@example.com'); $group = $this->getMockBuilder('OCP\IGroup')->getMock(); $group->method('getGID')->willReturn('groupId'); @@ -3586,7 +3587,7 @@ public function dataFormatShare() { $initiator->method('getDisplayName')->willReturn('initiatorDN'); $recipient = $this->getMockBuilder(IUser::class)->getMock(); $recipient->method('getDisplayName')->willReturn('recipientDN'); - $recipient->method('getEmailAddress')->willReturn('recipient'); + $recipient->method('getSystemEMailAddress')->willReturn('recipient'); $result = []; @@ -4387,7 +4388,7 @@ public function dataFormatShare() { public function testFormatShare(array $expects, \OCP\Share\IShare $share, array $users, $exception) { $this->userManager->method('get')->willReturnMap($users); - $recipientGroup = $this->createMock('\OCP\IGroup'); + $recipientGroup = $this->createMock(IGroup::class); $recipientGroup->method('getDisplayName')->willReturn('recipientGroupDisplayName'); $this->groupManager->method('get')->willReturnMap([ ['recipientGroup', $recipientGroup], @@ -4397,7 +4398,6 @@ public function testFormatShare(array $expects, \OCP\Share\IShare $share, array ->with('files_sharing.sharecontroller.showShare', ['token' => 'myToken']) ->willReturn('myLink'); - $this->rootFolder->method('getUserFolder') ->with($this->currentUser) ->willReturnSelf(); diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index 83ad887be7737..5bb62f2b7dc50 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -153,7 +153,7 @@ protected function getUserData(string $userId, bool $includeScopes = false): arr $data[IAccountManager::PROPERTY_AVATAR . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_AVATAR)->getScope(); } - $data[IAccountManager::PROPERTY_EMAIL] = $targetUserObject->getEMailAddress(); + $data[IAccountManager::PROPERTY_EMAIL] = $targetUserObject->getSystemEMailAddress(); if ($includeScopes) { $data[IAccountManager::PROPERTY_EMAIL . self::SCOPE_SUFFIX] = $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getScope(); } diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 8479100433b14..1e0be2f71c155 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -873,7 +873,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon } break; case IAccountManager::COLLECTION_EMAIL: - if (filter_var($value, FILTER_VALIDATE_EMAIL) && $value !== $targetUser->getEMailAddress()) { + if (filter_var($value, FILTER_VALIDATE_EMAIL) && $value !== $targetUser->getSystemEMailAddress()) { $userAccount = $this->accountManager->getAccount($targetUser); $mailCollection = $userAccount->getPropertyCollection(IAccountManager::COLLECTION_EMAIL); foreach ($mailCollection->getProperties() as $property) { diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index cc638c89a6393..7ae5d0c245f61 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -952,7 +952,7 @@ public function testGetUserDataAsAdmin() { ->disableOriginalConstructor() ->getMock(); $targetUser->expects($this->once()) - ->method('getEMailAddress') + ->method('getSystemEMailAddress') ->willReturn('demo@nextcloud.com'); $this->userSession ->expects($this->once()) @@ -1067,6 +1067,7 @@ public function testGetUserDataAsAdmin() { 'setPassword' => true, ], 'additional_mail' => [], + 'notify_email' => null, ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } @@ -1083,9 +1084,9 @@ public function testGetUserDataAsSubAdminAndUserIsAccessible() { ->disableOriginalConstructor() ->getMock(); $targetUser - ->expects($this->once()) - ->method('getEMailAddress') - ->willReturn('demo@nextcloud.com'); + ->expects($this->once()) + ->method('getSystemEMailAddress') + ->willReturn('demo@nextcloud.com'); $this->userSession ->expects($this->once()) ->method('getUser') @@ -1195,6 +1196,7 @@ public function testGetUserDataAsSubAdminAndUserIsAccessible() { 'setPassword' => true, ], 'additional_mail' => [], + 'notify_email' => null, ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } @@ -1306,7 +1308,7 @@ public function testGetUserDataAsSubAdminSelfLookup() { ->willReturn('Subadmin User'); $targetUser ->expects($this->once()) - ->method('getEMailAddress') + ->method('getSystemEMailAddress') ->willReturn('subadmin@nextcloud.com'); $targetUser ->method('getUID') @@ -1361,6 +1363,7 @@ public function testGetUserDataAsSubAdminSelfLookup() { 'setPassword' => false, ], 'additional_mail' => [], + 'notify_email' => null, ]; $this->assertEquals($expected, $this->invokePrivate($this->api, 'getUserData', ['UID'])); } diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php index f78fa7dd9b8c7..6be93d6a3a03d 100644 --- a/apps/settings/lib/Controller/UsersController.php +++ b/apps/settings/lib/Controller/UsersController.php @@ -482,7 +482,7 @@ protected function saveUserSettings(IAccount $userAccount): void { } } - $oldEmailAddress = $userAccount->getUser()->getEMailAddress(); + $oldEmailAddress = $userAccount->getUser()->getSystemEMailAddress(); $oldEmailAddress = strtolower((string)$oldEmailAddress); if ($oldEmailAddress !== $userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getValue()) { // this is the only permission a backend provides and is also used @@ -490,7 +490,7 @@ protected function saveUserSettings(IAccount $userAccount): void { if (!$userAccount->getUser()->canChangeDisplayName()) { throw new ForbiddenException($this->l10n->t('Unable to change email address')); } - $userAccount->getUser()->setEMailAddress($userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getValue()); + $userAccount->getUser()->setSystemEMailAddress($userAccount->getProperty(IAccountManager::PROPERTY_EMAIL)->getValue()); } try { diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php index 7faca378cdf72..797fa1621fa82 100644 --- a/apps/settings/tests/Controller/UsersControllerTest.php +++ b/apps/settings/tests/Controller/UsersControllerTest.php @@ -621,16 +621,15 @@ public function testSaveUserSettings($data, $user = $this->createMock(IUser::class); $user->method('getDisplayName')->willReturn($oldDisplayName); - $user->method('getEMailAddress')->willReturn($oldEmailAddress); + $user->method('getSystemEMailAddress')->willReturn($oldEmailAddress); $user->method('canChangeDisplayName')->willReturn(true); if ($data[IAccountManager::PROPERTY_EMAIL]['value'] === $oldEmailAddress || ($oldEmailAddress === null && $data[IAccountManager::PROPERTY_EMAIL]['value'] === '')) { - $user->expects($this->never())->method('setEMailAddress'); + $user->expects($this->never())->method('setSystemEMailAddress'); } else { - $user->expects($this->once())->method('setEMailAddress') - ->with($data[IAccountManager::PROPERTY_EMAIL]['value']) - ->willReturn(true); + $user->expects($this->once())->method('setSystemEMailAddress') + ->with($data[IAccountManager::PROPERTY_EMAIL]['value']); } if ($data[IAccountManager::PROPERTY_DISPLAYNAME]['value'] === $oldDisplayName || diff --git a/apps/user_ldap/lib/User/User.php b/apps/user_ldap/lib/User/User.php index 7d57fcbb275bc..b09fbd18327bf 100644 --- a/apps/user_ldap/lib/User/User.php +++ b/apps/user_ldap/lib/User/User.php @@ -448,7 +448,7 @@ public function updateEmail($valueFromLDAP = null) { if ($email !== '') { $user = $this->userManager->get($this->uid); if (!is_null($user)) { - $currentEmail = (string)$user->getEMailAddress(); + $currentEmail = (string)$user->getSystemEMailAddress(); if ($currentEmail !== $email) { $user->setEMailAddress($email); } diff --git a/core/Command/User/Info.php b/core/Command/User/Info.php index e6ba691a40dbe..347a4664bb0b4 100644 --- a/core/Command/User/Info.php +++ b/core/Command/User/Info.php @@ -75,7 +75,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $data = [ 'user_id' => $user->getUID(), 'display_name' => $user->getDisplayName(), - 'email' => $user->getEMailAddress() ? $user->getEMailAddress() : '', + 'email' => (string)$user->getSystemEMailAddress(), 'cloud_id' => $user->getCloudId(), 'enabled' => $user->isEnabled(), 'groups' => $groups, diff --git a/core/Command/User/ListCommand.php b/core/Command/User/ListCommand.php index 3a2708d4da233..20422afbf9de5 100644 --- a/core/Command/User/ListCommand.php +++ b/core/Command/User/ListCommand.php @@ -104,7 +104,7 @@ private function formatUsers(array $users, bool $detailed = false) { return [ 'user_id' => $user->getUID(), 'display_name' => $user->getDisplayName(), - 'email' => $user->getEMailAddress() ? $user->getEMailAddress() : '', + 'email' => (string)$user->getSystemEMailAddress(), 'cloud_id' => $user->getCloudId(), 'enabled' => $user->isEnabled(), 'groups' => $groups, diff --git a/lib/private/Collaboration/Collaborators/UserPlugin.php b/lib/private/Collaboration/Collaborators/UserPlugin.php index e3e4b37f3831e..9ed94082f0d0e 100644 --- a/lib/private/Collaboration/Collaborators/UserPlugin.php +++ b/lib/private/Collaboration/Collaborators/UserPlugin.php @@ -157,7 +157,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) { $userStatuses = $this->userStatusManager->getUserStatuses(array_keys($users)); foreach ($users as $uid => $user) { $userDisplayName = $user->getDisplayName(); - $userEmail = $user->getEMailAddress(); + $userEmail = $user->getSystemEMailAddress(); $uid = (string) $uid; $status = []; @@ -244,7 +244,7 @@ public function search($search, $limit, $offset, ISearchResult $searchResult) { if ($addUser) { $status = []; $uid = $user->getUID(); - $userEmail = $user->getEMailAddress(); + $userEmail = $user->getSystemEMailAddress(); if (array_key_exists($user->getUID(), $userStatuses)) { $userStatus = $userStatuses[$user->getUID()]; $status = [ diff --git a/tests/lib/AllConfigTest.php b/tests/lib/AllConfigTest.php index 9daa7c9be9c8a..b0b0b7eff8b50 100644 --- a/tests/lib/AllConfigTest.php +++ b/tests/lib/AllConfigTest.php @@ -15,6 +15,8 @@ * * @package Test */ + +use OC\SystemConfig; use OCP\IDBConnection; class AllConfigTest extends \Test\TestCase { @@ -145,7 +147,7 @@ public function testSetUserValueUnexpectedValue($value) { $config->setUserValue('userSetBool', 'appSetBool', 'keySetBool', $value); } - + public function testSetUserValueWithPreConditionFailure() { $this->expectException(\OCP\PreConditionNotMetException::class); @@ -437,4 +439,22 @@ public function testGetUsersForUserValue() { // cleanup $this->connection->executeUpdate('DELETE FROM `*PREFIX*preferences`'); } + + public function testGetUsersForUserValueCaseInsensitive() { + // mock the check for the database to run the correct SQL statements for each database type + $systemConfig = $this->createMock(SystemConfig::class); + $systemConfig->expects($this->once()) + ->method('getValue') + ->with($this->equalTo('dbtype'), $this->equalTo('sqlite')) + ->willReturn(\OC::$server->getConfig()->getSystemValue('dbtype', 'sqlite')); + $config = $this->getConfig($systemConfig); + + $config->setUserValue('user1', 'myApp', 'myKey', 'test123'); + $config->setUserValue('user2', 'myApp', 'myKey', 'TEST123'); + $config->setUserValue('user3', 'myApp', 'myKey', 'test12345'); + + $users = $config->getUsersForUserValueCaseInsensitive('myApp', 'myKey', 'test123'); + $this->assertSame(2, count($users)); + $this->assertSame(['user1', 'user2'], $users); + } } From cc9758e12a3d508dc9ca089b263a848564128e3f Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 1 Sep 2021 15:58:16 +0200 Subject: [PATCH 8/8] also allow admins to set the primary email - there will be times when it is necessary to reset this value for sure Signed-off-by: Arthur Schiwon --- apps/provisioning_api/lib/Controller/UsersController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 1e0be2f71c155..24768322f9d0e 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -777,6 +777,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon $permittedFields[] = IAccountManager::PROPERTY_WEBSITE; $permittedFields[] = IAccountManager::PROPERTY_TWITTER; $permittedFields[] = self::USER_FIELD_QUOTA; + $permittedFields[] = self::USER_FIELD_NOTIFICATION_EMAIL; } else { // No rights throw new OCSException('', OCSController::RESPOND_NOT_FOUND);