From a0ca3afeca0d49a4ec5a3dc692a69a98a42cdf0b Mon Sep 17 00:00:00 2001 From: "Alexander M. Turek" Date: Fri, 23 Aug 2019 17:57:29 +0200 Subject: [PATCH] Deprecate returning non-boolean values from checkCredentials(). --- UPGRADE-4.4.md | 1 + UPGRADE-5.0.md | 1 + src/Symfony/Component/Security/CHANGELOG.md | 1 + .../Security/Guard/AuthenticatorInterface.php | 5 ++- .../Provider/GuardAuthenticationProvider.php | 6 +++- .../GuardAuthenticationProviderTest.php | 36 +++++++++++++++++++ 6 files changed, 46 insertions(+), 4 deletions(-) diff --git a/UPGRADE-4.4.md b/UPGRADE-4.4.md index 7d19467d80..6a2eaea10d 100644 --- a/UPGRADE-4.4.md +++ b/UPGRADE-4.4.md @@ -194,6 +194,7 @@ Security * The `LdapUserProvider` class has been deprecated, use `Symfony\Component\Ldap\Security\LdapUserProvider` instead. * Implementations of `PasswordEncoderInterface` and `UserPasswordEncoderInterface` should add a new `needsRehash()` method + * Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`. Please explicitly return `false` to indicate invalid credentials. Stopwatch --------- diff --git a/UPGRADE-5.0.md b/UPGRADE-5.0.md index c6da522603..25749c546b 100644 --- a/UPGRADE-5.0.md +++ b/UPGRADE-5.0.md @@ -467,6 +467,7 @@ Security * The `BCryptPasswordEncoder` class has been removed, use `NativePasswordEncoder` instead. * Classes implementing the `TokenInterface` must implement the two new methods `__serialize` and `__unserialize` + * Implementations of `Guard\AuthenticatorInterface::checkCredentials()` must return a boolean value now. Please explicitly return `false` to indicate invalid credentials. SecurityBundle -------------- diff --git a/src/Symfony/Component/Security/CHANGELOG.md b/src/Symfony/Component/Security/CHANGELOG.md index 59743a3f13..d4db73958e 100644 --- a/src/Symfony/Component/Security/CHANGELOG.md +++ b/src/Symfony/Component/Security/CHANGELOG.md @@ -11,6 +11,7 @@ CHANGELOG * Added `Guard\PasswordAuthenticatedInterface`, an optional interface for "guard" authenticators that deal with user passwords * Marked all dispatched event classes as `@final` + * Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`. 4.3.0 ----- diff --git a/src/Symfony/Component/Security/Guard/AuthenticatorInterface.php b/src/Symfony/Component/Security/Guard/AuthenticatorInterface.php index 3c695a45ab..ff94aa6292 100644 --- a/src/Symfony/Component/Security/Guard/AuthenticatorInterface.php +++ b/src/Symfony/Component/Security/Guard/AuthenticatorInterface.php @@ -83,9 +83,8 @@ interface AuthenticatorInterface extends AuthenticationEntryPointInterface /** * 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 - * to cause authentication to fail. + * If false 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 ac295a6d08..3212973d96 100644 --- a/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php +++ b/src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php @@ -113,7 +113,11 @@ class GuardAuthenticationProvider implements AuthenticationProviderInterface } $this->userChecker->checkPreAuth($user); - if (true !== $guardAuthenticator->checkCredentials($token->getCredentials(), $user)) { + if (true !== $checkCredentialsResult = $guardAuthenticator->checkCredentials($token->getCredentials(), $user)) { + if (false !== $checkCredentialsResult) { + @trigger_error(sprintf('%s::checkCredentials() must return a boolean value. You returned %s. This behavior is deprecated in Symfony 4.4 and will trigger a TypeError in Symfony 5.', \get_class($guardAuthenticator), \is_object($checkCredentialsResult) ? \get_class($checkCredentialsResult) : \gettype($checkCredentialsResult)), E_USER_DEPRECATED); + } + 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)) { diff --git a/src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php b/src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php index 6de6ec029a..7b94f8077a 100644 --- a/src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php +++ b/src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php @@ -13,6 +13,7 @@ namespace Symfony\Component\Security\Guard\Tests\Provider; use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Exception\BadCredentialsException; use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Guard\AuthenticatorInterface; use Symfony\Component\Security\Guard\Provider\GuardAuthenticationProvider; @@ -87,6 +88,41 @@ class GuardAuthenticationProviderTest extends TestCase $this->assertSame($authedToken, $actualAuthedToken); } + public function testCheckCredentialsReturningFalseFailsAuthentication() + { + $this->expectException(BadCredentialsException::class); + $providerKey = 'my_uncool_firewall'; + + $authenticator = $this->createMock(AuthenticatorInterface::class); + + // make sure the authenticator is used + $this->preAuthenticationToken->expects($this->any()) + ->method('getGuardProviderKey') + // the 0 index, to match the only authenticator + ->willReturn('my_uncool_firewall_0'); + + $this->preAuthenticationToken->expects($this->atLeastOnce()) + ->method('getCredentials') + ->willReturn('non-null-value'); + + $mockedUser = $this->createMock(UserInterface::class); + $authenticator->expects($this->once()) + ->method('getUser') + ->willReturn($mockedUser); + // checkCredentials is called + $authenticator->expects($this->once()) + ->method('checkCredentials') + // authentication fails :( + ->willReturn(false); + + $provider = new GuardAuthenticationProvider([$authenticator], $this->userProvider, $providerKey, $this->userChecker); + $provider->authenticate($this->preAuthenticationToken); + } + + /** + * @group legacy + * @expectedDeprecation %s::checkCredentials() must return a boolean value. You returned NULL. This behavior is deprecated in Symfony 4.4 and will trigger a TypeError in Symfony 5. + */ public function testCheckCredentialsReturningNonTrueFailsAuthentication() { $this->expectException('Symfony\Component\Security\Core\Exception\BadCredentialsException');