From 2cfc5c7dd6ea3e3f7bd596f084c8aed68e238864 Mon Sep 17 00:00:00 2001 From: Nicolas Grekas Date: Wed, 17 Apr 2019 21:13:54 +0200 Subject: [PATCH] [Security] add support for opportunistic password migrations --- .../Security/User/EntityUserProvider.php | 19 +++++++- .../Security/User/EntityUserProviderTest.php | 18 ++++++++ src/Symfony/Bridge/Doctrine/composer.json | 5 ++- .../SecurityBundle/Resources/config/guard.xml | 1 + .../Ldap/Security/LdapUserProvider.php | 28 +++++++++++- src/Symfony/Component/Security/CHANGELOG.md | 3 ++ .../Provider/DaoAuthenticationProvider.php | 9 +++- .../DaoAuthenticationProviderTest.php | 44 ++++++++++++++++++- .../Encoder/MigratingPasswordEncoderTest.php | 6 --- .../Encoder/TestPasswordEncoderInterface.php | 19 ++++++++ .../Core/Tests/User/ChainUserProviderTest.php | 24 ++++++++++ .../Security/Core/User/ChainUserProvider.php | 18 +++++++- .../Core/User/PasswordUpgraderInterface.php | 27 ++++++++++++ .../Guard/PasswordAuthenticatedInterface.php | 25 +++++++++++ .../Provider/GuardAuthenticationProvider.php | 12 +++-- 15 files changed, 242 insertions(+), 16 deletions(-) create mode 100644 src/Symfony/Component/Security/Core/Tests/Encoder/TestPasswordEncoderInterface.php create mode 100644 src/Symfony/Component/Security/Core/User/PasswordUpgraderInterface.php create mode 100644 src/Symfony/Component/Security/Guard/PasswordAuthenticatedInterface.php diff --git a/src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php b/src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php index 20f6839957..9a3ff10b82 100644 --- a/src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php +++ b/src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php @@ -14,6 +14,7 @@ namespace Symfony\Bridge\Doctrine\Security\User; use Doctrine\Common\Persistence\ManagerRegistry; use Symfony\Component\Security\Core\Exception\UnsupportedUserException; use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; +use Symfony\Component\Security\Core\User\PasswordUpgraderInterface; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Core\User\UserProviderInterface; @@ -25,7 +26,7 @@ use Symfony\Component\Security\Core\User\UserProviderInterface; * @author Fabien Potencier * @author Johannes M. Schmitt */ -class EntityUserProvider implements UserProviderInterface +class EntityUserProvider implements UserProviderInterface, PasswordUpgraderInterface { private $registry; private $managerName; @@ -107,6 +108,22 @@ class EntityUserProvider implements UserProviderInterface return $class === $this->getClass() || is_subclass_of($class, $this->getClass()); } + /** + * {@inheritdoc} + */ + public function upgradePassword(UserInterface $user, string $newEncodedPassword): void + { + $class = $this->getClass(); + if (!$user instanceof $class) { + throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', \get_class($user))); + } + + $repository = $this->getRepository(); + if ($repository instanceof PasswordUpgraderInterface) { + $repository->upgradePassword($user, $newEncodedPassword); + } + } + private function getObjectManager() { return $this->registry->getManager($this->managerName); diff --git a/src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php b/src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php index 50edcab8d0..c0b4a56e50 100644 --- a/src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php +++ b/src/Symfony/Bridge/Doctrine/Tests/Security/User/EntityUserProviderTest.php @@ -16,6 +16,7 @@ use PHPUnit\Framework\TestCase; use Symfony\Bridge\Doctrine\Security\User\EntityUserProvider; use Symfony\Bridge\Doctrine\Test\DoctrineTestHelper; use Symfony\Bridge\Doctrine\Tests\Fixtures\User; +use Symfony\Component\Security\Core\User\PasswordUpgraderInterface; class EntityUserProviderTest extends TestCase { @@ -175,6 +176,23 @@ class EntityUserProviderTest extends TestCase $provider->loadUserByUsername('name'); } + public function testPasswordUpgrades() + { + $user = new User(1, 1, 'user1'); + + $repository = $this->getMockBuilder(PasswordUpgraderInterface::class)->getMock(); + $repository->expects($this->once()) + ->method('upgradePassword') + ->with($user, 'foobar'); + + $provider = new EntityUserProvider( + $this->getManager($this->getObjectManager($repository)), + 'Symfony\Bridge\Doctrine\Tests\Fixtures\User' + ); + + $provider->upgradePassword($user, 'foobar'); + } + private function getManager($em, $name = null) { $manager = $this->getMockBuilder('Doctrine\Common\Persistence\ManagerRegistry')->getMock(); diff --git a/src/Symfony/Bridge/Doctrine/composer.json b/src/Symfony/Bridge/Doctrine/composer.json index 3256c521fd..8017af5acf 100644 --- a/src/Symfony/Bridge/Doctrine/composer.json +++ b/src/Symfony/Bridge/Doctrine/composer.json @@ -33,7 +33,7 @@ "symfony/property-access": "^3.4|^4.0|^5.0", "symfony/property-info": "^3.4|^4.0|^5.0", "symfony/proxy-manager-bridge": "^3.4|^4.0|^5.0", - "symfony/security-core": "^3.4|^4.0|^5.0", + "symfony/security-core": "^4.4|^5.0", "symfony/expression-language": "^3.4|^4.0|^5.0", "symfony/validator": "^3.4|^4.0|^5.0", "symfony/translation": "^3.4|^4.0|^5.0", @@ -49,7 +49,8 @@ "phpunit/phpunit": "<4.8.35|<5.4.3,>=5.0", "symfony/dependency-injection": "<3.4", "symfony/form": "<4.4", - "symfony/messenger": "<4.3" + "symfony/messenger": "<4.3", + "symfony/security-core": "<4.4" }, "suggest": { "symfony/form": "", diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.xml b/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.xml index 43321494e0..7b17aff868 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.xml +++ b/src/Symfony/Bundle/SecurityBundle/Resources/config/guard.xml @@ -29,6 +29,7 @@ + * @author Robin Chalas */ -class LdapUserProvider implements UserProviderInterface +class LdapUserProvider implements UserProviderInterface, PasswordUpgraderInterface { private $ldap; private $baseDn; @@ -109,6 +111,30 @@ class LdapUserProvider implements UserProviderInterface return new LdapUser($user->getEntry(), $user->getUsername(), $user->getPassword(), $user->getRoles()); } + /** + * {@inheritdoc} + */ + public function upgradePassword(UserInterface $user, string $newEncodedPassword): void + { + if (!$user instanceof LdapUser) { + throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', \get_class($user))); + } + + if (null === $this->passwordAttribute) { + return; + } + + try { + if ($user->isEqualTo($this->loadUserByUsername($user->getUsername()))) { + $user->getEntry()->setAttribute($this->passwordAttribute, [$newEncodedPassword]); + $this->ldap->getEntryManager()->update($user->getEntry()); + $user->setPassword($newEncodedPassword); + } + } catch (ExceptionInterface $e) { + // ignore failed password upgrades + } + } + /** * {@inheritdoc} */ diff --git a/src/Symfony/Component/Security/CHANGELOG.md b/src/Symfony/Component/Security/CHANGELOG.md index 3ac23ef992..6c14764368 100644 --- a/src/Symfony/Component/Security/CHANGELOG.md +++ b/src/Symfony/Component/Security/CHANGELOG.md @@ -7,6 +7,9 @@ CHANGELOG * Deprecated class `LdapUserProvider`, use `Symfony\Component\Ldap\Security\LdapUserProvider` instead * Added method `needsRehash()` to `PasswordEncoderInterface` and `UserPasswordEncoderInterface` * Added `MigratingPasswordEncoder` + * Added and implemented `PasswordUpgraderInterface`, for opportunistic password migrations + * Added `Guard\PasswordAuthenticatedInterface`, an optional interface + for "guard" authenticators that deal with user passwords 4.3.0 ----- diff --git a/src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php b/src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php index f8a1019723..ac635357d6 100644 --- a/src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php +++ b/src/Symfony/Component/Security/Core/Authentication/Provider/DaoAuthenticationProvider.php @@ -16,6 +16,7 @@ use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface; use Symfony\Component\Security\Core\Exception\AuthenticationServiceException; use Symfony\Component\Security\Core\Exception\BadCredentialsException; use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; +use Symfony\Component\Security\Core\User\PasswordUpgraderInterface; use Symfony\Component\Security\Core\User\UserCheckerInterface; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Core\User\UserProviderInterface; @@ -54,9 +55,15 @@ class DaoAuthenticationProvider extends UserAuthenticationProvider throw new BadCredentialsException('The presented password cannot be empty.'); } - if (!$this->encoderFactory->getEncoder($user)->isPasswordValid($user->getPassword(), $presentedPassword, $user->getSalt())) { + $encoder = $this->encoderFactory->getEncoder($user); + + if (!$encoder->isPasswordValid($user->getPassword(), $presentedPassword, $user->getSalt())) { throw new BadCredentialsException('The presented password is invalid.'); } + + if ($this->userProvider instanceof PasswordUpgraderInterface && method_exists($encoder, 'needsRehash') && $encoder->needsRehash($user->getPassword())) { + $this->userProvider->upgradePassword($user, $encoder->encodePassword($presentedPassword, $user->getSalt())); + } } } diff --git a/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/DaoAuthenticationProviderTest.php b/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/DaoAuthenticationProviderTest.php index d9f8a54a75..0981b13c8f 100644 --- a/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/DaoAuthenticationProviderTest.php +++ b/src/Symfony/Component/Security/Core/Tests/Authentication/Provider/DaoAuthenticationProviderTest.php @@ -15,6 +15,10 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Authentication\Provider\DaoAuthenticationProvider; use Symfony\Component\Security\Core\Encoder\PlaintextPasswordEncoder; use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; +use Symfony\Component\Security\Core\Tests\Encoder\TestPasswordEncoderInterface; +use Symfony\Component\Security\Core\User\PasswordUpgraderInterface; +use Symfony\Component\Security\Core\User\User; +use Symfony\Component\Security\Core\User\UserProviderInterface; class DaoAuthenticationProviderTest extends TestCase { @@ -247,6 +251,44 @@ class DaoAuthenticationProviderTest extends TestCase $method->invoke($provider, $this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserInterface')->getMock(), $token); } + public function testPasswordUpgrades() + { + $user = new User('user', 'pwd'); + + $encoder = $this->getMockBuilder(TestPasswordEncoderInterface::class)->getMock(); + $encoder->expects($this->once()) + ->method('isPasswordValid') + ->willReturn(true) + ; + $encoder->expects($this->once()) + ->method('encodePassword') + ->willReturn('foobar') + ; + $encoder->expects($this->once()) + ->method('needsRehash') + ->willReturn(true) + ; + + $provider = $this->getProvider(null, null, $encoder); + + $userProvider = ((array) $provider)[sprintf("\0%s\0userProvider", DaoAuthenticationProvider::class)]; + $userProvider->expects($this->once()) + ->method('upgradePassword') + ->with($user, 'foobar') + ; + + $method = new \ReflectionMethod($provider, 'checkAuthentication'); + $method->setAccessible(true); + + $token = $this->getSupportedToken(); + $token->expects($this->once()) + ->method('getCredentials') + ->willReturn('foo') + ; + + $method->invoke($provider, $user, $token); + } + protected function getSupportedToken() { $mock = $this->getMockBuilder('Symfony\\Component\\Security\\Core\\Authentication\\Token\\UsernamePasswordToken')->setMethods(['getCredentials', 'getUser', 'getProviderKey'])->disableOriginalConstructor()->getMock(); @@ -261,7 +303,7 @@ class DaoAuthenticationProviderTest extends TestCase protected function getProvider($user = null, $userChecker = null, $passwordEncoder = null) { - $userProvider = $this->getMockBuilder('Symfony\\Component\\Security\\Core\\User\\UserProviderInterface')->getMock(); + $userProvider = $this->getMockBuilder([UserProviderInterface::class, PasswordUpgraderInterface::class])->getMock(); if (null !== $user) { $userProvider->expects($this->once()) ->method('loadUserByUsername') diff --git a/src/Symfony/Component/Security/Core/Tests/Encoder/MigratingPasswordEncoderTest.php b/src/Symfony/Component/Security/Core/Tests/Encoder/MigratingPasswordEncoderTest.php index 245d6c182d..468c326f35 100644 --- a/src/Symfony/Component/Security/Core/Tests/Encoder/MigratingPasswordEncoderTest.php +++ b/src/Symfony/Component/Security/Core/Tests/Encoder/MigratingPasswordEncoderTest.php @@ -14,7 +14,6 @@ namespace Symfony\Component\Security\Core\Tests\Encoder; use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Encoder\MigratingPasswordEncoder; use Symfony\Component\Security\Core\Encoder\NativePasswordEncoder; -use Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface; class MigratingPasswordEncoderTest extends TestCase { @@ -66,8 +65,3 @@ class MigratingPasswordEncoderTest extends TestCase $this->assertTrue($encoder->isPasswordValid('abc', 'foo', 'salt')); } } - -interface TestPasswordEncoderInterface extends PasswordEncoderInterface -{ - public function needsRehash(string $encoded): bool; -} diff --git a/src/Symfony/Component/Security/Core/Tests/Encoder/TestPasswordEncoderInterface.php b/src/Symfony/Component/Security/Core/Tests/Encoder/TestPasswordEncoderInterface.php new file mode 100644 index 0000000000..13e2d0d3b3 --- /dev/null +++ b/src/Symfony/Component/Security/Core/Tests/Encoder/TestPasswordEncoderInterface.php @@ -0,0 +1,19 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Tests\Encoder; + +use Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface; + +interface TestPasswordEncoderInterface extends PasswordEncoderInterface +{ + public function needsRehash(string $encoded): bool; +} diff --git a/src/Symfony/Component/Security/Core/Tests/User/ChainUserProviderTest.php b/src/Symfony/Component/Security/Core/Tests/User/ChainUserProviderTest.php index 1592bcd2fe..aa9ade7020 100644 --- a/src/Symfony/Component/Security/Core/Tests/User/ChainUserProviderTest.php +++ b/src/Symfony/Component/Security/Core/Tests/User/ChainUserProviderTest.php @@ -15,6 +15,8 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Exception\UnsupportedUserException; use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Core\User\ChainUserProvider; +use Symfony\Component\Security\Core\User\PasswordUpgraderInterface; +use Symfony\Component\Security\Core\User\User; class ChainUserProviderTest extends TestCase { @@ -188,6 +190,28 @@ class ChainUserProviderTest extends TestCase $this->assertSame($account, $provider->refreshUser($this->getAccount())); } + public function testPasswordUpgrades() + { + $user = new User('user', 'pwd'); + + $provider1 = $this->getMockBuilder(PasswordUpgraderInterface::class)->getMock(); + $provider1 + ->expects($this->once()) + ->method('upgradePassword') + ->willThrowException(new UnsupportedUserException('unsupported')) + ; + + $provider2 = $this->getMockBuilder(PasswordUpgraderInterface::class)->getMock(); + $provider2 + ->expects($this->once()) + ->method('upgradePassword') + ->with($user, 'foobar') + ; + + $provider = new ChainUserProvider([$provider1, $provider2]); + $provider->upgradePassword($user, 'foobar'); + } + protected function getAccount() { return $this->getMockBuilder('Symfony\Component\Security\Core\User\UserInterface')->getMock(); diff --git a/src/Symfony/Component/Security/Core/User/ChainUserProvider.php b/src/Symfony/Component/Security/Core/User/ChainUserProvider.php index 4106ad190a..b5dff59870 100644 --- a/src/Symfony/Component/Security/Core/User/ChainUserProvider.php +++ b/src/Symfony/Component/Security/Core/User/ChainUserProvider.php @@ -22,7 +22,7 @@ use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; * * @author Johannes M. Schmitt */ -class ChainUserProvider implements UserProviderInterface +class ChainUserProvider implements UserProviderInterface, PasswordUpgraderInterface { private $providers; @@ -104,4 +104,20 @@ class ChainUserProvider implements UserProviderInterface return false; } + + /** + * {@inheritdoc} + */ + public function upgradePassword(UserInterface $user, string $newEncodedPassword): void + { + foreach ($this->providers as $provider) { + if ($provider instanceof PasswordUpgraderInterface) { + try { + $provider->upgradePassword($user, $newEncodedPassword); + } catch (UnsupportedUserException $e) { + // ignore: password upgrades are opportunistic + } + } + } + } } diff --git a/src/Symfony/Component/Security/Core/User/PasswordUpgraderInterface.php b/src/Symfony/Component/Security/Core/User/PasswordUpgraderInterface.php new file mode 100644 index 0000000000..9c65298b07 --- /dev/null +++ b/src/Symfony/Component/Security/Core/User/PasswordUpgraderInterface.php @@ -0,0 +1,27 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\User; + +/** + * @author Nicolas Grekas + */ +interface PasswordUpgraderInterface +{ + /** + * Upgrades the encoded password of a user, typically for using a better hash algorithm. + * + * This method should persist the new password in the user storage and update the $user object accordingly. + * Because you don't want your users not being able to log in, this method should be opportunistic: + * it's fine if it does nothing or if it fails without throwing any exception. + */ + public function upgradePassword(UserInterface $user, string $newEncodedPassword): void; +} diff --git a/src/Symfony/Component/Security/Guard/PasswordAuthenticatedInterface.php b/src/Symfony/Component/Security/Guard/PasswordAuthenticatedInterface.php new file mode 100644 index 0000000000..4dd7a7b446 --- /dev/null +++ b/src/Symfony/Component/Security/Guard/PasswordAuthenticatedInterface.php @@ -0,0 +1,25 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Guard; + +/** + * An optional interface for "guard" authenticators that deal with user passwords. + */ +interface PasswordAuthenticatedInterface +{ + /** + * Returns the clear-text password contained in credentials if any. + * + * @param mixed The user credentials + */ + public function getPassword($credentials): ?string; +} diff --git a/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php b/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php index ece66a8df0..ac295a6d08 100644 --- a/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php +++ b/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php @@ -13,14 +13,17 @@ namespace Symfony\Component\Security\Guard\Provider; use Symfony\Component\Security\Core\Authentication\Provider\AuthenticationProviderInterface; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Encoder\UserPasswordEncoderInterface; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Core\Exception\AuthenticationExpiredException; use Symfony\Component\Security\Core\Exception\BadCredentialsException; use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; +use Symfony\Component\Security\Core\User\PasswordUpgraderInterface; use Symfony\Component\Security\Core\User\UserCheckerInterface; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Core\User\UserProviderInterface; use Symfony\Component\Security\Guard\AuthenticatorInterface; +use Symfony\Component\Security\Guard\PasswordAuthenticatedInterface; use Symfony\Component\Security\Guard\Token\GuardTokenInterface; use Symfony\Component\Security\Guard\Token\PreAuthenticationGuardToken; @@ -39,19 +42,19 @@ class GuardAuthenticationProvider implements AuthenticationProviderInterface private $userProvider; private $providerKey; private $userChecker; + private $passwordEncoder; /** * @param iterable|AuthenticatorInterface[] $guardAuthenticators The authenticators, with keys that match what's passed to GuardAuthenticationListener - * @param UserProviderInterface $userProvider The user provider * @param string $providerKey The provider (i.e. firewall) key - * @param UserCheckerInterface $userChecker */ - public function __construct($guardAuthenticators, UserProviderInterface $userProvider, string $providerKey, UserCheckerInterface $userChecker) + public function __construct($guardAuthenticators, UserProviderInterface $userProvider, string $providerKey, UserCheckerInterface $userChecker, UserPasswordEncoderInterface $passwordEncoder = null) { $this->guardAuthenticators = $guardAuthenticators; $this->userProvider = $userProvider; $this->providerKey = $providerKey; $this->userChecker = $userChecker; + $this->passwordEncoder = $passwordEncoder; } /** @@ -113,6 +116,9 @@ class GuardAuthenticationProvider implements AuthenticationProviderInterface if (true !== $guardAuthenticator->checkCredentials($token->getCredentials(), $user)) { throw new BadCredentialsException(sprintf('Authentication failed because %s::checkCredentials() did not return true.', \get_class($guardAuthenticator))); } + if ($this->userProvider instanceof PasswordUpgraderInterface && $guardAuthenticator instanceof PasswordAuthenticatedInterface && null !== $this->passwordEncoder && (null !== $password = $guardAuthenticator->getPassword($token->getCredentials())) && method_exists($this->passwordEncoder, 'needsRehash') && $this->passwordEncoder->needsRehash($user)) { + $this->userProvider->upgradePassword($user, $this->passwordEncoder->encodePassword($user, $password)); + } $this->userChecker->checkPostAuth($user); // turn the UserInterface into a TokenInterface