From 53eea058a6485497d540adfffd11b9da3d05ac4a Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Fri, 30 Oct 2015 15:12:11 -0400 Subject: [PATCH 1/2] Changing the behavior of GuardAuthenticatorInterface::checkCredentials(): you now *must* return true in order for authentication to pass. This was a suggestion at symfony_live to make things more secure: you must opt *into* authentication passing - you can't do nothing in this method and make authentication pass. --- .../Guard/GuardAuthenticatorInterface.php | 6 ++- .../Provider/GuardAuthenticationProvider.php | 5 ++- .../GuardAuthenticationProviderTest.php | 37 ++++++++++++++++++- 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php b/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php index 2db313cc2bd6e..8d397545d9e52 100644 --- a/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php +++ b/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php @@ -73,7 +73,11 @@ public function getCredentials(Request $request); public function getUser($credentials, UserProviderInterface $userProvider); /** - * Throw an AuthenticationException if the credentials are invalid. + * Return true if the credentials are valid. + * + * If any value other than true is returned, authentication will + * fail. You may also throw an AuthenticationException if you wish + * to cause authentication to fail. * * The *credentials* are the return value from getCredentials() * diff --git a/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php b/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php index 2a58085baeec2..4f58c59cbf287 100644 --- a/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php +++ b/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php @@ -13,6 +13,7 @@ use Symfony\Component\Security\Core\Authentication\Provider\AuthenticationProviderInterface; use Symfony\Component\Security\Core\Authentication\Token\AnonymousToken; +use Symfony\Component\Security\Core\Exception\BadCredentialsException; use Symfony\Component\Security\Core\Exception\UsernameNotFoundException; use Symfony\Component\Security\Guard\GuardAuthenticatorInterface; use Symfony\Component\Security\Guard\Token\GuardTokenInterface; @@ -122,7 +123,9 @@ private function authenticateViaGuard(GuardAuthenticatorInterface $guardAuthenti } $this->userChecker->checkPreAuth($user); - $guardAuthenticator->checkCredentials($token->getCredentials(), $user); + if (true !== $guardAuthenticator->checkCredentials($token->getCredentials(), $user)) { + throw new BadCredentialsException(sprintf('Authentication failed because %s::checkCredentials() did not return true', get_class($guardAuthenticator))); + }; $this->userChecker->checkPostAuth($user); // turn the UserInterface into a TokenInterface diff --git a/src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php b/src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php index 33c00e5073125..bfcf24b7709e7 100644 --- a/src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php +++ b/src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php @@ -60,7 +60,9 @@ public function testAuthenticate() // checkCredentials is called $authenticatorB->expects($this->once()) ->method('checkCredentials') - ->with($enteredCredentials, $mockedUser); + ->with($enteredCredentials, $mockedUser) + // authentication works! + ->will($this->returnValue(true)); $authedToken = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\TokenInterface'); $authenticatorB->expects($this->once()) ->method('createAuthenticatedToken') @@ -80,6 +82,39 @@ public function testAuthenticate() $this->assertSame($authedToken, $actualAuthedToken); } + /** + * @expectedException \Symfony\Component\Security\Core\Exception\BadCredentialsException + */ + public function testCheckCredentialsReturningNonTrueFailsAuthentication() + { + $providerKey = 'my_uncool_firewall'; + + $authenticator = $this->getMock('Symfony\Component\Security\Guard\GuardAuthenticatorInterface'); + + // make sure the authenticator is used + $this->preAuthenticationToken->expects($this->any()) + ->method('getGuardProviderKey') + // the 0 index, to match the only authenticator + ->will($this->returnValue('my_uncool_firewall_0')); + + $this->preAuthenticationToken->expects($this->atLeastOnce()) + ->method('getCredentials') + ->will($this->returnValue('non-null-value')); + + $mockedUser = $this->getMock('Symfony\Component\Security\Core\User\UserInterface'); + $authenticator->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($mockedUser)); + // checkCredentials is called + $authenticator->expects($this->once()) + ->method('checkCredentials') + // authentication fails :( + ->will($this->returnValue(null)); + + $provider = new GuardAuthenticationProvider(array($authenticator), $this->userProvider, $providerKey, $this->userChecker); + $provider->authenticate($this->preAuthenticationToken); + } + /** * @expectedException \Symfony\Component\Security\Core\Exception\AuthenticationExpiredException */ From c23ac4632c50278e7d87fdeb714dbf7996d70ef4 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Fri, 30 Oct 2015 17:42:21 -0400 Subject: [PATCH 2/2] Fixing small typos thanks to comments --- .../Component/Security/Guard/GuardAuthenticatorInterface.php | 2 +- .../Security/Guard/Provider/GuardAuthenticationProvider.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php b/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php index 8d397545d9e52..6e62ae659242e 100644 --- a/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php +++ b/src/Symfony/Component/Security/Guard/GuardAuthenticatorInterface.php @@ -73,7 +73,7 @@ public function getCredentials(Request $request); public function getUser($credentials, UserProviderInterface $userProvider); /** - * Return true if the credentials are valid. + * Returns true if the credentials are valid. * * If any value other than true is returned, authentication will * fail. You may also throw an AuthenticationException if you wish diff --git a/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php b/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php index 4f58c59cbf287..4347e020021fe 100644 --- a/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php +++ b/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php @@ -124,8 +124,8 @@ private function authenticateViaGuard(GuardAuthenticatorInterface $guardAuthenti $this->userChecker->checkPreAuth($user); if (true !== $guardAuthenticator->checkCredentials($token->getCredentials(), $user)) { - throw new BadCredentialsException(sprintf('Authentication failed because %s::checkCredentials() did not return true', get_class($guardAuthenticator))); - }; + throw new BadCredentialsException(sprintf('Authentication failed because %s::checkCredentials() did not return true.', get_class($guardAuthenticator))); + } $this->userChecker->checkPostAuth($user); // turn the UserInterface into a TokenInterface