From 3d4741783afc45310346732c7961d7a2012d6111 Mon Sep 17 00:00:00 2001 From: Nattfarinn Date: Tue, 11 Feb 2020 13:17:53 +0000 Subject: [PATCH] EZP-31079: Added way to use login or email (or both) during authentication --- doc/bc/changes-8.0.md | 6 +- .../Resources/config/security.yml | 12 +- .../API/Repository/Tests/UserServiceTest.php | 140 ++----------- eZ/Publish/API/Repository/UserService.php | 27 ++- .../RepositoryAuthenticationProviderTest.php | 2 +- .../Security/Tests/User/EmailProviderTest.php | 186 ++++++++++++++++++ ...viderTest.php => UsernameProviderTest.php} | 18 +- .../User/{Provider.php => BaseProvider.php} | 61 +----- .../Symfony/Security/User/EmailProvider.php | 34 ++++ .../Security/User/UsernameProvider.php | 34 ++++ .../Cache/Tests/UserHandlerTest.php | 5 +- .../Core/Persistence/Cache/UserHandler.php | 34 +++- .../Legacy/Tests/User/UserHandlerTest.php | 97 +++++++-- .../Core/Persistence/Legacy/User/Handler.php | 27 ++- .../SiteAccessAware/Tests/UserServiceTest.php | 4 +- .../SiteAccessAware/UserService.php | 16 +- eZ/Publish/Core/Repository/UserService.php | 96 ++++----- eZ/Publish/SPI/Persistence/User/Handler.php | 13 +- .../Decorator/UserServiceDecorator.php | 19 +- .../Decorator/UserServiceDecoratorTest.php | 9 +- 20 files changed, 527 insertions(+), 313 deletions(-) create mode 100644 eZ/Publish/Core/MVC/Symfony/Security/Tests/User/EmailProviderTest.php rename eZ/Publish/Core/MVC/Symfony/Security/Tests/User/{ProviderTest.php => UsernameProviderTest.php} (93%) rename eZ/Publish/Core/MVC/Symfony/Security/User/{Provider.php => BaseProvider.php} (57%) create mode 100644 eZ/Publish/Core/MVC/Symfony/Security/User/EmailProvider.php create mode 100644 eZ/Publish/Core/MVC/Symfony/Security/User/UsernameProvider.php diff --git a/doc/bc/changes-8.0.md b/doc/bc/changes-8.0.md index 29b9467da13..8d41527afac 100644 --- a/doc/bc/changes-8.0.md +++ b/doc/bc/changes-8.0.md @@ -242,13 +242,17 @@ Changes affecting version compatibility with former or future versions. * Deprecated `viewLocation` and `embedLocation` actions of `ViewController` have been dropped, along with related route `_ezpublishLocation`. As stated in controller, use `viewAction` in place of `viewLocation` and - `embedAction` in place od `embedLocation`. + `embedAction` in place of `embedLocation`. * Obsolete DeferredLegacy Content Type Update handler (`eZ\Publish\Core\Persistence\Legacy\Content\Type\Update\Handler\DeferredLegacy`) and its optional Symfony Container Service (`ezpublish.persistence.legacy.content_type.update_handler.deferred`) have been removed. Subscribe to eZ Platform Symfony Events to handle deferring of updating of Content items after their Content Type update instead. + +* Deprecated `UserService::loadUserByCredentials` method has been dropped. From now on, we rely on + Security package of Symfony framework to provide authenticated user, as it may happen there are + different ways of providing users configured in the application (ref.: https://symfony.com/doc/current/security/user_provider.html). ## Deprecated features diff --git a/eZ/Bundle/EzPublishCoreBundle/Resources/config/security.yml b/eZ/Bundle/EzPublishCoreBundle/Resources/config/security.yml index ad8463610a1..d3fd73ce0f4 100644 --- a/eZ/Bundle/EzPublishCoreBundle/Resources/config/security.yml +++ b/eZ/Bundle/EzPublishCoreBundle/Resources/config/security.yml @@ -1,6 +1,12 @@ services: - ezpublish.security.user_provider: - class: eZ\Publish\Core\MVC\Symfony\Security\User\Provider + ezpublish.security.user_provider.username: + class: eZ\Publish\Core\MVC\Symfony\Security\User\UsernameProvider + arguments: + - '@eZ\Publish\API\Repository\UserService' + - '@eZ\Publish\API\Repository\PermissionResolver' + + ezpublish.security.user_provider.email: + class: eZ\Publish\Core\MVC\Symfony\Security\User\EmailProvider arguments: - '@eZ\Publish\API\Repository\UserService' - '@eZ\Publish\API\Repository\PermissionResolver' @@ -40,3 +46,5 @@ services: - "%fragment.path%" tags: - { name: kernel.event_subscriber } + + ezpublish.security.user_provider: '@ezpublish.security.user_provider.username' diff --git a/eZ/Publish/API/Repository/Tests/UserServiceTest.php b/eZ/Publish/API/Repository/Tests/UserServiceTest.php index f6824700cd3..76909c64ce9 100644 --- a/eZ/Publish/API/Repository/Tests/UserServiceTest.php +++ b/eZ/Publish/API/Repository/Tests/UserServiceTest.php @@ -1252,12 +1252,10 @@ public function testLoadUserThrowsNotFoundException() } /** - * Test for the loadUserByCredentials() method. - * - * @see \eZ\Publish\API\Repository\UserService::loadUserByCredentials() - * @depends eZ\Publish\API\Repository\Tests\UserServiceTest::testCreateUser + * @see \eZ\Publish\API\Repository\UserService::checkUserCredentials() + * @depends \eZ\Publish\API\Repository\Tests\UserServiceTest::testCreateUser */ - public function testLoadUserByCredentials() + public function testCheckUserCredentialsValid(): void { $repository = $this->getRepository(); @@ -1266,103 +1264,31 @@ public function testLoadUserByCredentials() /* BEGIN: Use Case */ $user = $this->createUserVersion1(); - // Load the newly created user - $userReloaded = $userService->loadUserByCredentials('user', 'secret', Language::ALL); + // Load the newly created user credentials + $credentialsValid = $userService->loadUserByCredentials($user, 'secret'); /* END: Use Case */ - $this->assertEquals($user, $userReloaded); - } - - /** - * Test for the loadUserByCredentials() method. - * - * @see \eZ\Publish\API\Repository\UserService::loadUserByCredentials() - * @depends eZ\Publish\API\Repository\Tests\UserServiceTest::testLoadUserByCredentials - */ - public function testLoadUserByCredentialsThrowsNotFoundExceptionForUnknownPassword() - { - $this->expectException(\eZ\Publish\API\Repository\Exceptions\NotFoundException::class); - - $repository = $this->getRepository(); - - $userService = $repository->getUserService(); - - /* BEGIN: Use Case */ - $this->createUserVersion1(); - - // This call will fail with a "NotFoundException", because the given - // login/password combination does not exist. - $userService->loadUserByCredentials('user', 'SeCrEt'); - /* END: Use Case */ + $this->assertTrue($credentialsValid); } /** - * Test for the loadUserByCredentials() method. - * - * @see \eZ\Publish\API\Repository\UserService::loadUserByCredentials() - * @depends eZ\Publish\API\Repository\Tests\UserServiceTest::testLoadUserByCredentials + * @see \eZ\Publish\API\Repository\UserService::checkUserCredentials() + * @depends \eZ\Publish\API\Repository\Tests\UserServiceTest::testCreateUser */ - public function testLoadUserByCredentialsThrowsNotFoundExceptionForUnknownPasswordEmtpy() + public function testCheckUserCredentialsInvalid(): void { - $this->expectException(\eZ\Publish\API\Repository\Exceptions\NotFoundException::class); - $repository = $this->getRepository(); $userService = $repository->getUserService(); /* BEGIN: Use Case */ - $this->createUserVersion1(); + $user = $this->createUserVersion1(); - // This call will fail with a "NotFoundException", because the given - // login/password combination does not exist. - $userService->loadUserByCredentials('user', ''); + // Load the newly created user credentials + $credentialsValid = $userService->loadUserByCredentials($user, '1234'); /* END: Use Case */ - } - - /** - * Test for the loadUserByCredentials() method. - * - * @see \eZ\Publish\API\Repository\UserService::loadUserByCredentials() - * @depends eZ\Publish\API\Repository\Tests\UserServiceTest::testLoadUserByCredentials - */ - public function testLoadUserByCredentialsThrowsNotFoundExceptionForUnknownLogin() - { - $this->expectException(\eZ\Publish\API\Repository\Exceptions\NotFoundException::class); - $repository = $this->getRepository(); - - $userService = $repository->getUserService(); - - /* BEGIN: Use Case */ - $this->createUserVersion1(); - - // This call will fail with a "NotFoundException", because the given - // login/password combination does not exist. - $userService->loadUserByCredentials('üser', 'secret'); - /* END: Use Case */ - } - - /** - * Test for the loadUserByCredentials() method. - * - * @see \eZ\Publish\API\Repository\UserService::loadUserByCredentials() - * @depends eZ\Publish\API\Repository\Tests\UserServiceTest::testLoadUserByCredentials - */ - public function testLoadUserByCredentialsThrowsInvalidArgumentValueForEmptyLogin() - { - $this->expectException(\eZ\Publish\Core\Base\Exceptions\InvalidArgumentValue::class); - - $repository = $this->getRepository(); - - $userService = $repository->getUserService(); - - /* BEGIN: Use Case */ - $this->createUserVersion1(); - - // This call will fail with a "InvalidArgumentValue", because the given - // login is empty. - $userService->loadUserByCredentials('', 'secret'); - /* END: Use Case */ + $this->assertFalse($credentialsValid); } /** @@ -2441,46 +2367,6 @@ public function testLoadUserByLoginWithPrioritizedLanguagesList( } } - /** - * Test that multi-language logic for the loadUserByCredentials method respects - * prioritized language list. - * - * @covers \eZ\Publish\API\Repository\UserService::loadUserByCredentials - * @dataProvider getPrioritizedLanguageList - * @param string[] $prioritizedLanguages - * @param string|null $expectedLanguageCode language code of expected translation - */ - public function testLoadUserByCredentialsWithPrioritizedLanguagesList( - array $prioritizedLanguages, - $expectedLanguageCode - ) { - $repository = $this->getRepository(); - $userService = $repository->getUserService(); - $user = $this->createMultiLanguageUser(); - - // load, with prioritized languages, the newly created user - $loadedUser = $userService->loadUserByCredentials( - $user->login, - 'secret', - $prioritizedLanguages - ); - if ($expectedLanguageCode === null) { - $expectedLanguageCode = $loadedUser->contentInfo->mainLanguageCode; - } - - self::assertEquals( - $loadedUser->getName($expectedLanguageCode), - $loadedUser->getName() - ); - - foreach (['first_name', 'last_name', 'signature'] as $fieldIdentifier) { - self::assertEquals( - $loadedUser->getFieldValue($fieldIdentifier, $expectedLanguageCode), - $loadedUser->getFieldValue($fieldIdentifier) - ); - } - } - /** * Test that multi-language logic for the loadUsersByEmail method respects * prioritized language list. diff --git a/eZ/Publish/API/Repository/UserService.php b/eZ/Publish/API/Repository/UserService.php index a26f292e6d9..cfbc42faebb 100644 --- a/eZ/Publish/API/Repository/UserService.php +++ b/eZ/Publish/API/Repository/UserService.php @@ -142,24 +142,22 @@ public function createUser(UserCreateStruct $userCreateStruct, array $parentGrou public function loadUser($userId, array $prioritizedLanguages = []); /** - * Loads a user for the given login and password. + * Loads a user for the given login. * - * Since 6.1 login is case-insensitive across all storage engines and database backends, however if login - * is part of the password hash this method will essentially be case sensitive. + * Since 6.1 login is case-insensitive across all storage engines and database backends, like was the case + * with mysql before in eZ Publish 3.x/4.x/5.x. * * @deprecated since eZ Platform 2.5, will be dropped in the next major version as authentication * may depend on various user providers. Use UserService::checkUserCredentials() instead. * * @param string $login - * @param string $password the plain password * @param string[] $prioritizedLanguages Used as prioritized language code on translated properties of returned object. * * @return \eZ\Publish\API\Repository\Values\User\User * - * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException if credentials are invalid * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException if a user with the given credentials was not found */ - public function loadUserByCredentials($login, $password, array $prioritizedLanguages = []); + public function loadUserByLogin($login, array $prioritizedLanguages = []); /** * Checks if credentials are valid for provided User. @@ -172,22 +170,19 @@ public function loadUserByCredentials($login, $password, array $prioritizedLangu public function checkUserCredentials(User $user, string $credentials): bool; /** - * Loads a user for the given login. - * - * Since 6.1 login is case-insensitive across all storage engines and database backends, like was the case - * with mysql before in eZ Publish 3.x/4.x/5.x. + * Loads a user for the given email. * - * @param string $login + * @param string $email * @param string[] $prioritizedLanguages Used as prioritized language code on translated properties of returned object. * * @return \eZ\Publish\API\Repository\Values\User\User * - * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException if a user with the given credentials was not found + * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException */ - public function loadUserByLogin($login, array $prioritizedLanguages = []); + public function loadUserByEmail(string $email, array $prioritizedLanguages = []): User; /** - * Loads a user for the given email. + * Loads a users for the given email. * * Note: This method loads user by $email where $email might be case-insensitive on certain storage engines! * @@ -198,8 +193,10 @@ public function loadUserByLogin($login, array $prioritizedLanguages = []); * @param string[] $prioritizedLanguages Used as prioritized language code on translated properties of returned object. * * @return \eZ\Publish\API\Repository\Values\User\User[] + * + * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException */ - public function loadUsersByEmail($email, array $prioritizedLanguages = []); + public function loadUsersByEmail(string $email, array $prioritizedLanguages = []): array; /** * Loads a user with user hash key. diff --git a/eZ/Publish/Core/MVC/Symfony/Security/Tests/Authentication/RepositoryAuthenticationProviderTest.php b/eZ/Publish/Core/MVC/Symfony/Security/Tests/Authentication/RepositoryAuthenticationProviderTest.php index 93f8470cd50..1129ad3d017 100644 --- a/eZ/Publish/Core/MVC/Symfony/Security/Tests/Authentication/RepositoryAuthenticationProviderTest.php +++ b/eZ/Publish/Core/MVC/Symfony/Security/Tests/Authentication/RepositoryAuthenticationProviderTest.php @@ -157,7 +157,7 @@ public function testCheckAuthentication() $password = 'foo'; $token = new UsernamePasswordToken($userName, $password, 'bar'); - $apiUser = $this->getMockForAbstractClass(APIUser::class); + $apiUser = $this->createMock(APIUser::class); $user = $this->createMock(User::class); $user->method('getAPIUser') ->willReturn($apiUser); diff --git a/eZ/Publish/Core/MVC/Symfony/Security/Tests/User/EmailProviderTest.php b/eZ/Publish/Core/MVC/Symfony/Security/Tests/User/EmailProviderTest.php new file mode 100644 index 00000000000..0111505437f --- /dev/null +++ b/eZ/Publish/Core/MVC/Symfony/Security/Tests/User/EmailProviderTest.php @@ -0,0 +1,186 @@ +userService = $this->createMock(UserService::class); + $this->permissionResolver = $this->createMock(PermissionResolver::class); + $this->userProvider = new EmailProvider($this->userService, $this->permissionResolver); + } + + public function testLoadUserByUsernameAlreadyUserObject() + { + $user = $this->createMock(UserInterface::class); + $this->assertSame($user, $this->userProvider->loadUserByUsername($user)); + } + + public function testLoadUserByUsernameUserNotFound() + { + $this->expectException(\Symfony\Component\Security\Core\Exception\UsernameNotFoundException::class); + + $username = 'foobar@example.org'; + $this->userService + ->expects($this->once()) + ->method('loadUserByEmail') + ->with($username) + ->will($this->throwException(new NotFoundException('user', $username))); + $this->userProvider->loadUserByUsername($username); + } + + public function testLoadUserByUsername() + { + $username = 'foobar@example.org'; + $apiUser = $this->createMock(APIUser::class); + + $this->userService + ->expects($this->once()) + ->method('loadUserByEmail') + ->with($username) + ->will($this->returnValue($apiUser)); + + $user = $this->userProvider->loadUserByUsername($username); + $this->assertInstanceOf(UserInterface::class, $user); + $this->assertSame($apiUser, $user->getAPIUser()); + $this->assertSame(['ROLE_USER'], $user->getRoles()); + } + + public function testRefreshUserNotSupported() + { + $this->expectException(\Symfony\Component\Security\Core\Exception\UnsupportedUserException::class); + + $user = $this->createMock(SymfonyUserInterface::class); + $this->userProvider->refreshUser($user); + } + + public function testRefreshUser() + { + $userId = 123; + $apiUser = new User( + [ + 'content' => new Content( + [ + 'versionInfo' => new VersionInfo( + ['contentInfo' => new ContentInfo(['id' => $userId])] + ), + ] + ), + ] + ); + $refreshedAPIUser = clone $apiUser; + $user = $this->createMock(UserInterface::class); + $user + ->expects($this->once()) + ->method('getAPIUser') + ->will($this->returnValue($apiUser)); + $user + ->expects($this->once()) + ->method('setAPIUser') + ->with($refreshedAPIUser); + + $this->userService + ->expects($this->once()) + ->method('loadUser') + ->with($userId) + ->will($this->returnValue($refreshedAPIUser)); + + $this->permissionResolver + ->expects($this->once()) + ->method('setCurrentUserReference') + ->with(new UserReference($apiUser->getUserId())); + + $this->assertSame($user, $this->userProvider->refreshUser($user)); + } + + public function testRefreshUserNotFound() + { + $this->expectException(\Symfony\Component\Security\Core\Exception\UsernameNotFoundException::class); + + $userId = 123; + $apiUser = new User( + [ + 'content' => new Content( + [ + 'versionInfo' => new VersionInfo( + ['contentInfo' => new ContentInfo(['id' => $userId])] + ), + ] + ), + ] + ); + $user = $this->createMock(UserInterface::class); + $user + ->expects($this->once()) + ->method('getAPIUser') + ->will($this->returnValue($apiUser)); + + $this->userService + ->expects($this->once()) + ->method('loadUser') + ->with($userId) + ->will($this->throwException(new NotFoundException('user', 'foo'))); + + $this->userProvider->refreshUser($user); + } + + /** + * @dataProvider supportsClassProvider + */ + public function testSupportsClass($class, $supports) + { + $this->assertSame($supports, $this->userProvider->supportsClass($class)); + } + + public function supportsClassProvider() + { + return [ + [SymfonyUserInterface::class, false], + [MVCUser::class, true], + [get_class($this->createMock(MVCUser::class)), true], + ]; + } + + public function testLoadUserByAPIUser() + { + $apiUser = $this->createMock(APIUser::class); + + $user = $this->userProvider->loadUserByAPIUser($apiUser); + + $this->assertInstanceOf(MVCUser::class, $user); + $this->assertSame($apiUser, $user->getAPIUser()); + $this->assertSame(['ROLE_USER'], $user->getRoles()); + } +} diff --git a/eZ/Publish/Core/MVC/Symfony/Security/Tests/User/ProviderTest.php b/eZ/Publish/Core/MVC/Symfony/Security/Tests/User/UsernameProviderTest.php similarity index 93% rename from eZ/Publish/Core/MVC/Symfony/Security/Tests/User/ProviderTest.php rename to eZ/Publish/Core/MVC/Symfony/Security/Tests/User/UsernameProviderTest.php index ad64e90a0f7..ad444f029a7 100644 --- a/eZ/Publish/Core/MVC/Symfony/Security/Tests/User/ProviderTest.php +++ b/eZ/Publish/Core/MVC/Symfony/Security/Tests/User/UsernameProviderTest.php @@ -1,11 +1,11 @@ userService = $this->createMock(UserService::class); $this->permissionResolver = $this->createMock(PermissionResolver::class); - $this->userProvider = new Provider($this->userService, $this->permissionResolver); + $this->userProvider = new UsernameProvider($this->userService, $this->permissionResolver); } public function testLoadUserByUsernameAlreadyUserObject() diff --git a/eZ/Publish/Core/MVC/Symfony/Security/User/Provider.php b/eZ/Publish/Core/MVC/Symfony/Security/User/BaseProvider.php similarity index 57% rename from eZ/Publish/Core/MVC/Symfony/Security/User/Provider.php rename to eZ/Publish/Core/MVC/Symfony/Security/User/BaseProvider.php index 0287897516a..b2ce2646844 100644 --- a/eZ/Publish/Core/MVC/Symfony/Security/User/Provider.php +++ b/eZ/Publish/Core/MVC/Symfony/Security/User/BaseProvider.php @@ -1,11 +1,11 @@ userService = $userService; } - /** - * Loads the user for the given user ID. - * $user can be either the user ID or an instance of \eZ\Publish\Core\MVC\Symfony\Security\User - * (anonymous user we try to check access via SecurityContext::isGranted()). - * - * @param string|\eZ\Publish\Core\MVC\Symfony\Security\User $user Either the user ID to load an instance of User object. A value of -1 represents an anonymous user. - * - * @return \eZ\Publish\Core\MVC\Symfony\Security\UserInterface - * - * @throws \Symfony\Component\Security\Core\Exception\UsernameNotFoundException if the user is not found - */ - public function loadUserByUsername($user) - { - try { - // SecurityContext always tries to authenticate anonymous users when checking granted access. - // In that case $user is an instance of \eZ\Publish\Core\MVC\Symfony\Security\User. - // We don't need to reload the user here. - if ($user instanceof UserInterface) { - return $user; - } - - return $this->createSecurityUser( - $this->userService->loadUserByLogin($user) - ); - } catch (NotFoundException $e) { - throw new UsernameNotFoundException($e->getMessage(), 0, $e); - } - } - - /** - * Refreshes the user for the account interface. - * - * It is up to the implementation to decide if the user data should be - * totally reloaded (e.g. from the database), or if the UserInterface - * object can just be merged into some internal array of users / identity - * map. - * - * @param \Symfony\Component\Security\Core\User\UserInterface $user - * - * @throws \Symfony\Component\Security\Core\Exception\UnsupportedUserException - * - * @return \Symfony\Component\Security\Core\User\UserInterface - */ public function refreshUser(CoreUserInterface $user) { if (!$user instanceof UserInterface) { @@ -87,9 +44,9 @@ public function refreshUser(CoreUserInterface $user) try { $refreshedAPIUser = $this->userService->loadUser( - $user instanceof ReferenceUserInterface ? - $user->getAPIUserReference()->getUserId() : - $user->getAPIUser()->id + $user instanceof ReferenceUserInterface + ? $user->getAPIUserReference()->getUserId() + : $user->getAPIUser()->id ); $user->setAPIUser($refreshedAPIUser); $this->permissionResolver->setCurrentUserReference( @@ -135,7 +92,7 @@ public function loadUserByAPIUser(APIUser $apiUser) * * @return \eZ\Publish\Core\MVC\Symfony\Security\User */ - private function createSecurityUser(APIUser $apiUser): User + protected function createSecurityUser(APIUser $apiUser): User { return new User($apiUser, ['ROLE_USER']); } diff --git a/eZ/Publish/Core/MVC/Symfony/Security/User/EmailProvider.php b/eZ/Publish/Core/MVC/Symfony/Security/User/EmailProvider.php new file mode 100644 index 00000000000..14f152dd93e --- /dev/null +++ b/eZ/Publish/Core/MVC/Symfony/Security/User/EmailProvider.php @@ -0,0 +1,34 @@ +createSecurityUser( + $this->userService->loadUserByEmail($user) + ); + } catch (NotFoundException $e) { + throw new UsernameNotFoundException($e->getMessage(), 0, $e); + } + } +} diff --git a/eZ/Publish/Core/MVC/Symfony/Security/User/UsernameProvider.php b/eZ/Publish/Core/MVC/Symfony/Security/User/UsernameProvider.php new file mode 100644 index 00000000000..a4c24d394af --- /dev/null +++ b/eZ/Publish/Core/MVC/Symfony/Security/User/UsernameProvider.php @@ -0,0 +1,34 @@ +createSecurityUser( + $this->userService->loadUserByLogin($user) + ); + } catch (NotFoundException $e) { + throw new UsernameNotFoundException($e->getMessage(), 0, $e); + } + } +} diff --git a/eZ/Publish/Core/Persistence/Cache/Tests/UserHandlerTest.php b/eZ/Publish/Core/Persistence/Cache/Tests/UserHandlerTest.php index e94ecb70ca0..24717ec8628 100644 --- a/eZ/Publish/Core/Persistence/Cache/Tests/UserHandlerTest.php +++ b/eZ/Publish/Core/Persistence/Cache/Tests/UserHandlerTest.php @@ -45,9 +45,11 @@ public function providerForUnCachedMethods(): array 'ez-user-14', 'ez-user-' . str_replace('@', '_A', $user->login) . '-by-login', 'ez-user-' . str_replace('@', '_A', $user->email) . '-by-email', + 'ez-users-' . str_replace('@', '_A', $user->email) . '-by-email', ], $user, false], ['update', [$user], ['content-14', 'user-14'], [ 'ez-user-' . str_replace('@', '_A', $user->email) . '-by-email', + 'ez-users-' . str_replace('@', '_A', $user->email) . '-by-email', ], $user, false], ['updateUserToken', [$userToken], ['user-14-account-key'], ['ez-user-4irj8t43r-by-account-key']], ['expireUserToken', ['4irj8t43r'], null, ['ez-user-4irj8t43r-by-account-key']], @@ -82,7 +84,8 @@ public function providerForCachedLoadMethods(): array return [ ['load', [14], 'ez-user-14', $user], ['loadByLogin', ['admin'], 'ez-user-admin-by-login', $user], - ['loadByEmail', ['nospam@ez.no'], 'ez-user-nospam_Aez.no-by-email', [$user]], + ['loadByEmail', ['nospam@ez.no'], 'ez-user-nospam_Aez.no-by-email', $user], + ['loadUsersByEmail', ['nospam@ez.no'], 'ez-users-nospam_Aez.no-by-email', [$user]], ['loadUserByToken', ['hash'], 'ez-user-hash-by-account-key', $user], ['loadRole', [9], 'ez-role-9', $role], ['loadRoleByIdentifier', ['member'], 'ez-role-member-by-identifier', $role], diff --git a/eZ/Publish/Core/Persistence/Cache/UserHandler.php b/eZ/Publish/Core/Persistence/Cache/UserHandler.php index bd4564d5b9f..f31e928cb26 100644 --- a/eZ/Publish/Core/Persistence/Cache/UserHandler.php +++ b/eZ/Publish/Core/Persistence/Cache/UserHandler.php @@ -52,6 +52,8 @@ public function init(): void return [ 'ez-user-' . $user->id, 'ez-user-' . $this->escapeForCacheKey($user->login) . '-by-login', + 'ez-user-' . $this->escapeForCacheKey($user->email) . '-by-email', + 'ez-users-' . $this->escapeForCacheKey($user->email) . '-by-email', //'ez-user-' . $hash . '-by-account-key', ]; }; @@ -91,6 +93,7 @@ public function create(User $user) 'ez-user-' . $user->id, 'ez-user-' . $this->escapeForCacheKey($user->login) . '-by-login', 'ez-user-' . $this->escapeForCacheKey($user->email) . '-by-email', + 'ez-users-' . $this->escapeForCacheKey($user->email) . '-by-email', ]); return $user; @@ -132,13 +135,33 @@ function ($escapedLogin) use ($login) { /** * {@inheritdoc} */ - public function loadByEmail($email) + public function loadByEmail(string $email): User + { + /** @var \eZ\Publish\SPI\Persistence\User $cachedValue */ + $cachedValue = $this->getCacheValue( + $this->escapeForCacheKey($email), + 'ez-user-', + function () use ($email) { + return $this->persistenceHandler->userHandler()->loadByEmail($email); + }, + $this->getUserTags, + $this->getUserKeys, + '-by-email' + ); + + return $cachedValue; + } + + /** + * {@inheritdoc} + */ + public function loadUsersByEmail(string $email): array { // As load by email can return several items we threat it like a list here. return $this->getListCacheValue( - 'ez-user-' . $this->escapeForCacheKey($email) . '-by-email', + 'ez-users-' . $this->escapeForCacheKey($email) . '-by-email', function () use ($email) { - return $this->persistenceHandler->userHandler()->loadByEmail($email); + return $this->persistenceHandler->userHandler()->loadUsersByEmail($email); }, $this->getUserTags, $this->getUserKeys @@ -186,7 +209,10 @@ public function update(User $user) // Clear corresponding content cache as update of the User changes it's external data $this->cache->invalidateTags(['content-' . $user->id, 'user-' . $user->id]); // Clear especially by email key as it might already be cached and this might represent change to email - $this->cache->deleteItems(['ez-user-' . $this->escapeForCacheKey($user->email) . '-by-email']); + $this->cache->deleteItems([ + 'ez-user-' . $this->escapeForCacheKey($user->email) . '-by-email', + 'ez-users-' . $this->escapeForCacheKey($user->email) . '-by-email', + ]); return $user; } diff --git a/eZ/Publish/Core/Persistence/Legacy/Tests/User/UserHandlerTest.php b/eZ/Publish/Core/Persistence/Legacy/Tests/User/UserHandlerTest.php index 9983a0de134..5732f874f63 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Tests/User/UserHandlerTest.php +++ b/eZ/Publish/Core/Persistence/Legacy/Tests/User/UserHandlerTest.php @@ -18,6 +18,7 @@ use eZ\Publish\SPI\Persistence; use eZ\Publish\SPI\Persistence\User\Handler; use eZ\Publish\SPI\Persistence\User\Role; +use LogicException; /** * Test case for UserHandlerTest. @@ -77,16 +78,28 @@ public function testCreateUser() protected function getGatewayReturnValue(): array { return [ - [ - 'contentobject_id' => self::TEST_USER_ID, - 'login' => 'kore', - 'email' => 'kore@example.org', - 'password_hash' => '1234567890', - 'password_hash_type' => 2, - 'is_enabled' => true, - 'max_login' => 23, - 'password_updated_at' => 1569229200, - ], + $this->getDummyUser( + self::TEST_USER_ID, + 'kore', + 'kore@example.org' + ), + ]; + } + + protected function getDummyUser( + int $id, + string $login, + string $email + ): array { + return [ + 'contentobject_id' => $id, + 'login' => $login, + 'email' => $email, + 'password_hash' => '1234567890', + 'password_hash_type' => 2, + 'is_enabled' => true, + 'max_login' => 23, + 'password_updated_at' => 1569229200, ]; } @@ -146,6 +159,48 @@ public function testLoadUserByLogin() ); } + public function testLoadMultipleUsersByLogin() + { + $this->expectException(LogicException::class); + + $gatewayMock = $this + ->createMock(User\Gateway::class); + + $gatewayMock + ->method('loadByLogin') + ->with('kore') + ->willReturn([ + $this->getDummyUser(self::TEST_USER_ID, 'kore', 'kore@example.org'), + $this->getDummyUser(self::TEST_USER_ID + 1, 'kore', 'kore@example.org'), + ]); + + $handler = $this->getUserHandler($gatewayMock); + $user = $this->getValidUser(); + + $handler->loadByLogin($user->login); + } + + public function testLoadMultipleUsersByEmail() + { + $this->expectException(LogicException::class); + + $gatewayMock = $this + ->createMock(User\Gateway::class); + + $gatewayMock + ->method('loadByEmail') + ->with('kore@example.org') + ->willReturn([ + $this->getDummyUser(self::TEST_USER_ID, 'kore_a', 'kore@example.org'), + $this->getDummyUser(self::TEST_USER_ID + 1, 'kore_b', 'kore@example.org'), + ]); + + $handler = $this->getUserHandler($gatewayMock); + $user = $this->getValidUser(); + + $handler->loadByEmail($user->email); + } + public function testLoadUserByEmailNotFound() { $this->expectException(NotFoundException::class); @@ -157,6 +212,26 @@ public function testLoadUserByEmailNotFound() } public function testLoadUserByEmail() + { + $gatewayMock = $this + ->createMock(User\Gateway::class); + + $gatewayMock + ->method('loadByEmail') + ->with('kore@example.org') + ->willReturn($this->getGatewayReturnValue()); + + $handler = $this->getUserHandler($gatewayMock); + $validUser = $this->getValidUser(); + + $user = $handler->loadByEmail($validUser->email); + $this->assertEquals( + $validUser, + $user + ); + } + + public function testLoadUsersByEmail() { $gatewayMock = $this ->createMock(User\Gateway::class); @@ -169,7 +244,7 @@ public function testLoadUserByEmail() $handler = $this->getUserHandler($gatewayMock); $user = $this->getValidUser(); - $users = $handler->loadByEmail($user->email); + $users = $handler->loadUsersByEmail($user->email); $this->assertEquals( $user, $users[0] diff --git a/eZ/Publish/Core/Persistence/Legacy/User/Handler.php b/eZ/Publish/Core/Persistence/Legacy/User/Handler.php index 6945de49bce..caef4d1c7f7 100644 --- a/eZ/Publish/Core/Persistence/Legacy/User/Handler.php +++ b/eZ/Publish/Core/Persistence/Legacy/User/Handler.php @@ -117,13 +117,36 @@ public function loadByLogin($login) if (empty($data)) { throw new NotFound('user', $login); - } elseif (isset($data[1])) { + } elseif (count($data) > 1) { throw new LogicException("Found more then one user with login '{$login}'"); } return $this->mapper->mapUser($data[0]); } + /** + * Loads user(s) with user email. + * + * As earlier eZ Publish versions supported several users having same email (ini config), + * this function may return several users. + * + * @param string $email + * + * @return \eZ\Publish\SPI\Persistence\User + */ + public function loadByEmail(string $email): User + { + $data = $this->userGateway->loadByEmail($email); + + if (empty($data)) { + throw new NotFound('user', $email); + } elseif (count($data) > 1) { + throw new LogicException("Found more then one user with login '{$email}'"); + } + + return $this->mapper->mapUser($data[0]); + } + /** * Loads user(s) with user email. * @@ -134,7 +157,7 @@ public function loadByLogin($login) * * @return \eZ\Publish\SPI\Persistence\User[] */ - public function loadByEmail($email) + public function loadUsersByEmail(string $email): array { $data = $this->userGateway->loadByEmail($email); diff --git a/eZ/Publish/Core/Repository/SiteAccessAware/Tests/UserServiceTest.php b/eZ/Publish/Core/Repository/SiteAccessAware/Tests/UserServiceTest.php index 3cd7eb6b638..4387a3b38f5 100644 --- a/eZ/Publish/Core/Repository/SiteAccessAware/Tests/UserServiceTest.php +++ b/eZ/Publish/Core/Repository/SiteAccessAware/Tests/UserServiceTest.php @@ -84,9 +84,9 @@ public function providerForLanguagesLookupMethods() ['loadUserGroup', [4, self::LANG_ARG], true, 1], ['loadSubUserGroups', [$userGroup, 50, 50, self::LANG_ARG], true, 3], ['loadUser', [14, self::LANG_ARG], true, 1], - ['loadUserByCredentials', ['admin', 'Best passPhrase EvA!!', self::LANG_ARG], true, 2], ['loadUserByLogin', ['admin', self::LANG_ARG], true, 1], - ['loadUsersByEmail', ['nospam@ez.no', self::LANG_ARG], true, 1], + ['loadUserByEmail', ['nospam@ez.no', self::LANG_ARG], $user, 1], + ['loadUsersByEmail', ['nospam@ez.no', self::LANG_ARG], [], 1], ['loadUserGroupsOfUser', [$user, 50, 50, self::LANG_ARG], true, 3], ['loadUsersOfUserGroup', [$userGroup, 50, 50, self::LANG_ARG], true, 3], ['loadUserByToken', ['43ir43jrt43', self::LANG_ARG], true, 1], diff --git a/eZ/Publish/Core/Repository/SiteAccessAware/UserService.php b/eZ/Publish/Core/Repository/SiteAccessAware/UserService.php index 11abbcdb80a..103686844f7 100644 --- a/eZ/Publish/Core/Repository/SiteAccessAware/UserService.php +++ b/eZ/Publish/Core/Repository/SiteAccessAware/UserService.php @@ -94,13 +94,6 @@ public function loadUser($userId, array $prioritizedLanguages = null) return $this->service->loadUser($userId, $prioritizedLanguages); } - public function loadUserByCredentials($login, $password, array $prioritizedLanguages = null) - { - $prioritizedLanguages = $this->languageResolver->getPrioritizedLanguages($prioritizedLanguages); - - return $this->service->loadUserByCredentials($login, $password, $prioritizedLanguages); - } - public function checkUserCredentials(User $user, string $credentials): bool { return $this->service->checkUserCredentials($user, $credentials); @@ -113,7 +106,14 @@ public function loadUserByLogin($login, array $prioritizedLanguages = null) return $this->service->loadUserByLogin($login, $prioritizedLanguages); } - public function loadUsersByEmail($email, array $prioritizedLanguages = null) + public function loadUserByEmail(string $email, array $prioritizedLanguages = null): User + { + $prioritizedLanguages = $this->languageResolver->getPrioritizedLanguages($prioritizedLanguages); + + return $this->service->loadUserByEmail($email, $prioritizedLanguages); + } + + public function loadUsersByEmail(string $email, array $prioritizedLanguages = null): array { $prioritizedLanguages = $this->languageResolver->getPrioritizedLanguages($prioritizedLanguages); diff --git a/eZ/Publish/Core/Repository/UserService.php b/eZ/Publish/Core/Repository/UserService.php index 289b3625814..5a71e5996d5 100644 --- a/eZ/Publish/Core/Repository/UserService.php +++ b/eZ/Publish/Core/Repository/UserService.php @@ -42,7 +42,6 @@ use eZ\Publish\Core\Base\Exceptions\ContentValidationException; use eZ\Publish\Core\Base\Exceptions\InvalidArgumentException; use eZ\Publish\Core\Base\Exceptions\InvalidArgumentValue; -use eZ\Publish\Core\Base\Exceptions\NotFoundException; use eZ\Publish\Core\Base\Exceptions\UnauthorizedException; use eZ\Publish\Core\FieldType\User\Value as UserValue; use eZ\Publish\Core\FieldType\User\Type as UserType; @@ -488,48 +487,6 @@ public function loadUser($userId, array $prioritizedLanguages = []) return $this->buildDomainUserObject($spiUser, $content); } - /** - * Loads a user for the given login and password. - * - * If the password hash type differs from that configured for the service, it will be updated to the configured one. - * - * {@inheritdoc} - * - * @param string $login - * @param string $password the plain password - * @param string[] $prioritizedLanguages Used as prioritized language code on translated properties of returned object. - * - * @return \eZ\Publish\API\Repository\Values\User\User - * - * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException if credentials are invalid - * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException if a user with the given credentials was not found - */ - public function loadUserByCredentials($login, $password, array $prioritizedLanguages = []) - { - @trigger_error( - sprintf('%s method will be removed in eZ Platform 3.0. Use UserService::checkUserCredentials instead.', __METHOD__), - E_USER_DEPRECATED - ); - - if (!is_string($login) || empty($login)) { - throw new InvalidArgumentValue('login', $login); - } - - if (!is_string($password)) { - throw new InvalidArgumentValue('password', $password); - } - - $spiUser = $this->userHandler->loadByLogin($login); - if (!$this->comparePasswordHashForSPIUser($login, $password, $spiUser)) { - throw new NotFoundException('user', $login); - } - - // Don't catch BadStateException, on purpose, to avoid broken hashes. - $this->updatePasswordHash($login, $password, $spiUser); - - return $this->buildDomainUserObject($spiUser, null, $prioritizedLanguages); - } - /** * Checks if credentials are valid for provided User. * @@ -540,7 +497,7 @@ public function loadUserByCredentials($login, $password, array $prioritizedLangu */ public function checkUserCredentials(APIUser $user, string $credentials): bool { - return $this->comparePasswordHashForAPIUser($user->login, $credentials, $user); + return $this->comparePasswordHashForAPIUser($user, $credentials); } /** @@ -605,6 +562,29 @@ public function loadUserByLogin($login, array $prioritizedLanguages = []) return $this->buildDomainUserObject($spiUser, null, $prioritizedLanguages); } + /** + * Loads a user for the given email. + * + * {@inheritdoc} + * + * @param string $email + * @param string[] $prioritizedLanguages Used as prioritized language code on translated properties of returned object. + * + * @return \eZ\Publish\API\Repository\Values\User\User + * + * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException + */ + public function loadUserByEmail(string $email, array $prioritizedLanguages = []): APIUser + { + if (empty($email)) { + throw new InvalidArgumentValue('email', $email); + } + + $spiUser = $this->userHandler->loadByEmail($email); + + return $this->buildDomainUserObject($spiUser, null, $prioritizedLanguages); + } + /** * Loads a user for the given email. * @@ -614,15 +594,17 @@ public function loadUserByLogin($login, array $prioritizedLanguages = []) * @param string[] $prioritizedLanguages Used as prioritized language code on translated properties of returned object. * * @return \eZ\Publish\API\Repository\Values\User\User[] + * + * @throws \eZ\Publish\API\Repository\Exceptions\InvalidArgumentException */ - public function loadUsersByEmail($email, array $prioritizedLanguages = []) + public function loadUsersByEmail(string $email, array $prioritizedLanguages = []): array { - if (!is_string($email) || empty($email)) { + if (empty($email)) { throw new InvalidArgumentValue('email', $email); } $users = []; - foreach ($this->userHandler->loadByEmail($email) as $spiUser) { + foreach ($this->userHandler->loadUsersByEmail($email) as $spiUser) { $users[] = $this->buildDomainUserObject($spiUser, null, $prioritizedLanguages); } @@ -1225,7 +1207,7 @@ public function validatePassword(string $password, PasswordValidationContext $co $isNewPasswordRequired = $configuration['PasswordValueValidator']['requireNewPassword'] ?? false; if (($isPasswordTTLEnabled || $isNewPasswordRequired) && - $this->comparePasswordHashForAPIUser($context->user->login, $password, $context->user) + $this->comparePasswordHashForAPIUser($context->user, $password) ) { $errors[] = new ValidationError('New password cannot be the same as old password', null, [], 'password'); } @@ -1335,35 +1317,26 @@ private function getUserFieldDefinition(ContentType $contentType): ?FieldDefinit /** * Verifies if the provided login and password are valid for eZ\Publish\SPI\Persistence\User. * - * @param string $login User login - * @param string $password User password - * @param \eZ\Publish\SPI\Persistence\User $spiUser Loaded user handler - * * @return bool return true if the login and password are sucessfully validated and false, if not. */ - protected function comparePasswordHashForSPIUser(string $login, string $password, SPIUser $spiUser): bool + protected function comparePasswordHashForSPIUser(SPIUser $user, string $password): bool { - return $this->comparePasswordHashes($login, $password, $spiUser->passwordHash, $spiUser->hashAlgorithm); + return $this->comparePasswordHashes($password, $user->passwordHash, $user->hashAlgorithm); } /** * Verifies if the provided login and password are valid for eZ\Publish\API\Repository\Values\User\User. * - * @param string $login User login - * @param string $password User password - * @param \eZ\Publish\API\Repository\Values\User\User $apiUser Loaded user - * * @return bool return true if the login and password are sucessfully validated and false, if not. */ - protected function comparePasswordHashForAPIUser(string $login, string $password, APIUser $apiUser): bool + protected function comparePasswordHashForAPIUser(APIUser $user, string $password): bool { - return $this->comparePasswordHashes($login, $password, $apiUser->passwordHash, $apiUser->hashAlgorithm); + return $this->comparePasswordHashes($password, $user->passwordHash, $user->hashAlgorithm); } /** * Verifies if the provided login and password are valid against given password hash and hash type. * - * @param string $login User login * @param string $plainPassword User password * @param string $passwordHash User password hash * @param int $hashAlgorithm Hash type @@ -1371,7 +1344,6 @@ protected function comparePasswordHashForAPIUser(string $login, string $password * @return bool return true if the login and password are sucessfully validated and false, if not. */ private function comparePasswordHashes( - string $login, string $plainPassword, string $passwordHash, int $hashAlgorithm diff --git a/eZ/Publish/SPI/Persistence/User/Handler.php b/eZ/Publish/SPI/Persistence/User/Handler.php index fa6c1d36ad8..9b4fd775d23 100644 --- a/eZ/Publish/SPI/Persistence/User/Handler.php +++ b/eZ/Publish/SPI/Persistence/User/Handler.php @@ -51,6 +51,17 @@ public function load($userId); */ public function loadByLogin($login); + /** + * Loads user with user email. + * + * Note: This method loads user by $email case in-sensitive on certain storage engines! + * + * @param string $email + * + * @return \eZ\Publish\SPI\Persistence\User + */ + public function loadByEmail(string $email): User; + /** * Loads user(s) with user email. * @@ -63,7 +74,7 @@ public function loadByLogin($login); * * @return \eZ\Publish\SPI\Persistence\User[] */ - public function loadByEmail($email); + public function loadUsersByEmail(string $email): array; /** * Loads user with user hash. diff --git a/eZ/Publish/SPI/Repository/Decorator/UserServiceDecorator.php b/eZ/Publish/SPI/Repository/Decorator/UserServiceDecorator.php index 3fa0c14b010..63699689fdc 100644 --- a/eZ/Publish/SPI/Repository/Decorator/UserServiceDecorator.php +++ b/eZ/Publish/SPI/Repository/Decorator/UserServiceDecorator.php @@ -86,14 +86,6 @@ public function loadUser( return $this->innerService->loadUser($userId, $prioritizedLanguages); } - public function loadUserByCredentials( - $login, - $password, - array $prioritizedLanguages = [] - ) { - return $this->innerService->loadUserByCredentials($login, $password, $prioritizedLanguages); - } - public function checkUserCredentials( User $user, string $credentials @@ -108,10 +100,17 @@ public function loadUserByLogin( return $this->innerService->loadUserByLogin($login, $prioritizedLanguages); } + public function loadUserByEmail( + string $email, + array $prioritizedLanguages = [] + ): User { + return $this->innerService->loadUserByEmail($email, $prioritizedLanguages); + } + public function loadUsersByEmail( - $email, + string $email, array $prioritizedLanguages = [] - ) { + ): array { return $this->innerService->loadUsersByEmail($email, $prioritizedLanguages); } diff --git a/eZ/Publish/SPI/Repository/Tests/Decorator/UserServiceDecoratorTest.php b/eZ/Publish/SPI/Repository/Tests/Decorator/UserServiceDecoratorTest.php index 5668ec1646f..18688c94d90 100644 --- a/eZ/Publish/SPI/Repository/Tests/Decorator/UserServiceDecoratorTest.php +++ b/eZ/Publish/SPI/Repository/Tests/Decorator/UserServiceDecoratorTest.php @@ -154,20 +154,19 @@ public function testLoadUserDecorator() $decoratedService->loadUser(...$parameters); } - public function testLoadUserByCredentialsDecorator() + public function testCheckUserCredentialsDecorator() { $serviceMock = $this->createServiceMock(); $decoratedService = $this->createDecorator($serviceMock); $parameters = [ + $this->createMock(User::class), 'random_value_5ced05ce1771c7.58152750', - 'random_value_5ced05ce1771d3.89279980', - ['random_value_5ced05ce1771e1.45786513'], ]; - $serviceMock->expects($this->once())->method('loadUserByCredentials')->with(...$parameters); + $serviceMock->expects($this->once())->method('checkUserCredentials')->with(...$parameters); - $decoratedService->loadUserByCredentials(...$parameters); + $decoratedService->checkUserCredentials(...$parameters); } public function testLoadUserByLoginDecorator()