From 297fdfd552ef1a8608cc4df8e8c578818f143252 Mon Sep 17 00:00:00 2001 From: Nate Lenart Date: Wed, 6 Jun 2018 16:44:10 -0700 Subject: [PATCH 1/2] Delegate user credential checking to PasswordChecker --- DependencyInjection/Configuration.php | 1 + .../FOSOAuthServerExtension.php | 1 + Resources/config/oauth.xml | 4 + Storage/OAuthStorage.php | 15 ++- Storage/PasswordChecker.php | 43 +++++++++ Storage/PasswordCheckerInterface.php | 28 ++++++ Tests/Storage/OAuthStorageTest.php | 52 +++------- Tests/Storage/PasswordCheckerTest.php | 94 +++++++++++++++++++ 8 files changed, 190 insertions(+), 48 deletions(-) create mode 100644 Storage/PasswordChecker.php create mode 100644 Storage/PasswordCheckerInterface.php create mode 100644 Tests/Storage/PasswordCheckerTest.php diff --git a/DependencyInjection/Configuration.php b/DependencyInjection/Configuration.php index 571fcafd..8aff98d7 100644 --- a/DependencyInjection/Configuration.php +++ b/DependencyInjection/Configuration.php @@ -121,6 +121,7 @@ private function addServiceSection(ArrayNodeDefinition $node) ->addDefaultsIfNotSet() ->children() ->scalarNode('storage')->defaultValue('fos_oauth_server.storage.default')->cannotBeEmpty()->end() + ->scalarNode('password_checker')->defaultValue('fos_oauth_server.password_checker.default')->cannotBeEmpty()->end() ->scalarNode('user_provider')->defaultNull()->end() ->scalarNode('client_manager')->defaultValue('fos_oauth_server.client_manager.default')->end() ->scalarNode('access_token_manager')->defaultValue('fos_oauth_server.access_token_manager.default')->end() diff --git a/DependencyInjection/FOSOAuthServerExtension.php b/DependencyInjection/FOSOAuthServerExtension.php index 256bff31..821b7156 100644 --- a/DependencyInjection/FOSOAuthServerExtension.php +++ b/DependencyInjection/FOSOAuthServerExtension.php @@ -46,6 +46,7 @@ public function load(array $configs, ContainerBuilder $container) } $container->setAlias('fos_oauth_server.storage', $config['service']['storage']); + $container->setAlias('fos_oauth_server.password_checker', $config['service']['password_checker']); $container->setAlias('fos_oauth_server.client_manager', $config['service']['client_manager']); $container->setAlias('fos_oauth_server.access_token_manager', $config['service']['access_token_manager']); $container->setAlias('fos_oauth_server.refresh_token_manager', $config['service']['refresh_token_manager']); diff --git a/Resources/config/oauth.xml b/Resources/config/oauth.xml index 9ec29f6d..be6303c0 100644 --- a/Resources/config/oauth.xml +++ b/Resources/config/oauth.xml @@ -14,7 +14,11 @@ + + + + diff --git a/Storage/OAuthStorage.php b/Storage/OAuthStorage.php index a56dd25a..bfcc4c06 100644 --- a/Storage/OAuthStorage.php +++ b/Storage/OAuthStorage.php @@ -18,6 +18,7 @@ use FOS\OAuthServerBundle\Model\ClientInterface; use FOS\OAuthServerBundle\Model\ClientManagerInterface; use FOS\OAuthServerBundle\Model\RefreshTokenManagerInterface; +use FOS\OAuthServerBundle\Storage\PasswordCheckerInterface; use OAuth2\IOAuth2GrantClient; use OAuth2\IOAuth2GrantCode; use OAuth2\IOAuth2GrantExtension; @@ -28,7 +29,6 @@ use OAuth2\OAuth2; use OAuth2\OAuth2ServerException; use Symfony\Component\HttpFoundation\Response; -use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Core\User\UserProviderInterface; @@ -60,9 +60,9 @@ class OAuthStorage implements IOAuth2RefreshTokens, IOAuth2GrantUser, IOAuth2Gra protected $userProvider; /** - * @var EncoderFactoryInterface + * @var PasswordCheckerInterface */ - protected $encoderFactory; + protected $passwordChecker; /** * @var array [uri] => GrantExtensionInterface @@ -74,19 +74,19 @@ class OAuthStorage implements IOAuth2RefreshTokens, IOAuth2GrantUser, IOAuth2Gra * @param AccessTokenManagerInterface $accessTokenManager * @param RefreshTokenManagerInterface $refreshTokenManager * @param AuthCodeManagerInterface $authCodeManager + * @param PasswordCheckerInterface $passwordChecker * @param null|UserProviderInterface $userProvider - * @param null|EncoderFactoryInterface $encoderFactory */ public function __construct(ClientManagerInterface $clientManager, AccessTokenManagerInterface $accessTokenManager, RefreshTokenManagerInterface $refreshTokenManager, AuthCodeManagerInterface $authCodeManager, - UserProviderInterface $userProvider = null, EncoderFactoryInterface $encoderFactory = null) + PasswordCheckerInterface $passwordChecker, UserProviderInterface $userProvider = null) { $this->clientManager = $clientManager; $this->accessTokenManager = $accessTokenManager; $this->refreshTokenManager = $refreshTokenManager; $this->authCodeManager = $authCodeManager; + $this->passwordChecker = $passwordChecker; $this->userProvider = $userProvider; - $this->encoderFactory = $encoderFactory; $this->grantExtensions = []; } @@ -165,8 +165,7 @@ public function checkUserCredentials(IOAuth2Client $client, $username, $password return false; } - $encoder = $this->encoderFactory->getEncoder($user); - if ($encoder->isPasswordValid($user->getPassword(), $password, $user->getSalt())) { + if ($this->passwordChecker->validate($user, $password)) { return [ 'data' => $user, ]; diff --git a/Storage/PasswordChecker.php b/Storage/PasswordChecker.php new file mode 100644 index 00000000..2dd36ee8 --- /dev/null +++ b/Storage/PasswordChecker.php @@ -0,0 +1,43 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FOS\OAuthServerBundle\Storage; + +use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface; +use Symfony\Component\Security\Core\User\UserInterface; + +class PasswordChecker implements PasswordCheckerInterface +{ + /** + * @var EncoderFactoryInterface + */ + protected $encoderFactory; + + /** + * @param EncoderFactoryInterface $encoderFactory + */ + public function __construct(EncoderFactoryInterface $encoderFactory) + { + $this->encoderFactory = $encoderFactory; + } + + /** + * {@inheritDoc} + */ + public function validate(UserInterface $user, $password): bool + { + $encoder = $this->encoderFactory->getEncoder($user); + + return $encoder->isPasswordValid($user->getPassword(), $password, $user->getSalt()); + } +} diff --git a/Storage/PasswordCheckerInterface.php b/Storage/PasswordCheckerInterface.php new file mode 100644 index 00000000..113e14a8 --- /dev/null +++ b/Storage/PasswordCheckerInterface.php @@ -0,0 +1,28 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FOS\OAuthServerBundle\Storage; + +use Symfony\Component\Security\Core\User\UserInterface; + +interface PasswordCheckerInterface +{ + /** + * Validate the user's password matches + * + * @param UserInterface $user + * @param string $password + * @return bool + */ + public function validate(UserInterface $user, $password): bool; +} diff --git a/Tests/Storage/OAuthStorageTest.php b/Tests/Storage/OAuthStorageTest.php index f628327b..6201ee7d 100644 --- a/Tests/Storage/OAuthStorageTest.php +++ b/Tests/Storage/OAuthStorageTest.php @@ -22,7 +22,7 @@ use FOS\OAuthServerBundle\Model\RefreshToken; use FOS\OAuthServerBundle\Model\RefreshTokenManagerInterface; use FOS\OAuthServerBundle\Storage\OAuthStorage; -use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface; +use FOS\OAuthServerBundle\Storage\PasswordCheckerInterface; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Core\User\UserProviderInterface; @@ -39,7 +39,7 @@ class OAuthStorageTest extends \PHPUnit\Framework\TestCase protected $userProvider; - protected $encoderFactory; + protected $passwordChecker; protected $storage; @@ -65,12 +65,12 @@ public function setUp() ->disableOriginalConstructor() ->getMock() ; - $this->encoderFactory = $this->getMockBuilder(EncoderFactoryInterface::class) + $this->passwordChecker = $this->getMockBuilder(PasswordCheckerInterface::class) ->disableOriginalConstructor() ->getMock() ; - $this->storage = new OAuthStorage($this->clientManager, $this->accessTokenManager, $this->refreshTokenManager, $this->authCodeManager, $this->userProvider, $this->encoderFactory); + $this->storage = new OAuthStorage($this->clientManager, $this->accessTokenManager, $this->refreshTokenManager, $this->authCodeManager, $this->passwordChecker, $this->userProvider); } public function testGetClientReturnsClientWithGivenId() @@ -365,20 +365,6 @@ public function testCheckUserCredentialsReturnsTrueOnValidCredentials() ->disableOriginalConstructor() ->getMock() ; - $user->expects($this->once()) - ->method('getPassword')->with()->will($this->returnValue('foo')); - $user->expects($this->once()) - ->method('getSalt')->with()->will($this->returnValue('bar')); - - $encoder = $this->getMockBuilder('Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface') - ->disableOriginalConstructor() - ->getMock() - ; - $encoder->expects($this->once()) - ->method('isPasswordValid') - ->with('foo', 'baz', 'bar') - ->will($this->returnValue(true)) - ; $this->userProvider->expects($this->once()) ->method('loadUserByUsername') @@ -386,10 +372,10 @@ public function testCheckUserCredentialsReturnsTrueOnValidCredentials() ->will($this->returnValue($user)) ; - $this->encoderFactory->expects($this->once()) - ->method('getEncoder') - ->with($user) - ->will($this->returnValue($encoder)) + $this->passwordChecker->expects($this->once()) + ->method('validate') + ->with($user, 'baz') + ->will($this->returnValue(true)) ; $this->assertSame([ @@ -404,20 +390,6 @@ public function testCheckUserCredentialsReturnsFalseOnInvalidCredentials() ->disableOriginalConstructor() ->getMock() ; - $user->expects($this->once()) - ->method('getPassword')->with()->will($this->returnValue('foo')); - $user->expects($this->once()) - ->method('getSalt')->with()->will($this->returnValue('bar')); - - $encoder = $this->getMockBuilder('Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface') - ->disableOriginalConstructor() - ->getMock() - ; - $encoder->expects($this->once()) - ->method('isPasswordValid') - ->with('foo', 'baz', 'bar') - ->will($this->returnValue(false)) - ; $this->userProvider->expects($this->once()) ->method('loadUserByUsername') @@ -425,10 +397,10 @@ public function testCheckUserCredentialsReturnsFalseOnInvalidCredentials() ->will($this->returnValue($user)) ; - $this->encoderFactory->expects($this->once()) - ->method('getEncoder') - ->with($user) - ->will($this->returnValue($encoder)) + $this->passwordChecker->expects($this->once()) + ->method('validate') + ->with($user, 'baz') + ->will($this->returnValue(false)) ; $this->assertFalse($this->storage->checkUserCredentials($client, 'Joe', 'baz')); diff --git a/Tests/Storage/PasswordCheckerTest.php b/Tests/Storage/PasswordCheckerTest.php new file mode 100644 index 00000000..c18c85ae --- /dev/null +++ b/Tests/Storage/PasswordCheckerTest.php @@ -0,0 +1,94 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace FOS\OAuthServerBundle\Tests\Storage; + +use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface; +use FOS\OAuthServerBundle\Storage\PasswordChecker; + +class PasswordCheckerTest extends \PHPUnit\Framework\TestCase +{ + protected $encoderFactory; + + protected $passwordChecker; + + public function setUp() + { + $this->encoderFactory = $this->getMockBuilder(EncoderFactoryInterface::class) + ->disableOriginalConstructor() + ->getMock() + ; + + $this->passwordChecker = new PasswordChecker($this->encoderFactory); + } + + public function testValidateReturnsTrueOnValidCredentials() + { + $user = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserInterface') + ->disableOriginalConstructor() + ->getMock() + ; + $user->expects($this->once()) + ->method('getPassword')->with()->will($this->returnValue('foo')); + $user->expects($this->once()) + ->method('getSalt')->with()->will($this->returnValue('bar')); + + $encoder = $this->getMockBuilder('Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface') + ->disableOriginalConstructor() + ->getMock() + ; + $encoder->expects($this->once()) + ->method('isPasswordValid') + ->with('foo', 'baz', 'bar') + ->will($this->returnValue(true)) + ; + + $this->encoderFactory->expects($this->once()) + ->method('getEncoder') + ->with($user) + ->will($this->returnValue($encoder)) + ; + + $this->assertTrue($this->passwordChecker->validate($user, 'baz')); + } + + public function testValidateReturnsFalseOnInvalidCredentials() + { + $user = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserInterface') + ->disableOriginalConstructor() + ->getMock() + ; + $user->expects($this->once()) + ->method('getPassword')->with()->will($this->returnValue('foo')); + $user->expects($this->once()) + ->method('getSalt')->with()->will($this->returnValue('bar')); + + $encoder = $this->getMockBuilder('Symfony\Component\Security\Core\Encoder\PasswordEncoderInterface') + ->disableOriginalConstructor() + ->getMock() + ; + $encoder->expects($this->once()) + ->method('isPasswordValid') + ->with('foo', 'baz', 'bar') + ->will($this->returnValue(false)) + ; + + $this->encoderFactory->expects($this->once()) + ->method('getEncoder') + ->with($user) + ->will($this->returnValue($encoder)) + ; + + $this->assertFalse($this->passwordChecker->validate($user, 'baz')); + } +} From 6213a0be84882e92d718ab00b4ace7a953b645ec Mon Sep 17 00:00:00 2001 From: Nate Lenart Date: Thu, 14 Jun 2018 13:48:33 -0700 Subject: [PATCH 2/2] php-cs-fixer fixups --- Storage/OAuthStorage.php | 1 - Storage/PasswordCheckerInterface.php | 7 ++++--- Tests/Storage/PasswordCheckerTest.php | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Storage/OAuthStorage.php b/Storage/OAuthStorage.php index bfcc4c06..5263ed2b 100644 --- a/Storage/OAuthStorage.php +++ b/Storage/OAuthStorage.php @@ -18,7 +18,6 @@ use FOS\OAuthServerBundle\Model\ClientInterface; use FOS\OAuthServerBundle\Model\ClientManagerInterface; use FOS\OAuthServerBundle\Model\RefreshTokenManagerInterface; -use FOS\OAuthServerBundle\Storage\PasswordCheckerInterface; use OAuth2\IOAuth2GrantClient; use OAuth2\IOAuth2GrantCode; use OAuth2\IOAuth2GrantExtension; diff --git a/Storage/PasswordCheckerInterface.php b/Storage/PasswordCheckerInterface.php index 113e14a8..c937242b 100644 --- a/Storage/PasswordCheckerInterface.php +++ b/Storage/PasswordCheckerInterface.php @@ -18,10 +18,11 @@ interface PasswordCheckerInterface { /** - * Validate the user's password matches + * Validate the user's password matches. + * + * @param UserInterface $user + * @param string $password * - * @param UserInterface $user - * @param string $password * @return bool */ public function validate(UserInterface $user, $password): bool; diff --git a/Tests/Storage/PasswordCheckerTest.php b/Tests/Storage/PasswordCheckerTest.php index c18c85ae..b475870a 100644 --- a/Tests/Storage/PasswordCheckerTest.php +++ b/Tests/Storage/PasswordCheckerTest.php @@ -13,8 +13,8 @@ namespace FOS\OAuthServerBundle\Tests\Storage; -use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface; use FOS\OAuthServerBundle\Storage\PasswordChecker; +use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface; class PasswordCheckerTest extends \PHPUnit\Framework\TestCase {