Skip to content

Commit 610a4e9

Browse files
fabpotfabpot
committed
feature #33308 [SecurityGuard] Deprecate returning non-boolean values from checkCredentials() (derrabus)
This PR was merged into the 4.4 branch. Discussion ---------- [SecurityGuard] Deprecate returning non-boolean values from checkCredentials() | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | prepares #33228 | License | MIT | Doc PR | TODO This PR suggests to deprecate implementations of `Symfony\Component\Security\Guard\AuthenticatorInterface::checkCredentials()` that return non-boolean values. This will allow us to add `bool` as return type declaration in the future. nicolas-grekas#26 (comment) /cc @nicolas-grekas @weaverryan Commits ------- a0ca3af Deprecate returning non-boolean values from checkCredentials().
2 parents 5b3028a + a0ca3af commit 610a4e9

File tree

6 files changed

+46
-4
lines changed

6 files changed

+46
-4
lines changed

‎UPGRADE-4.4.md

+1
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,7 @@ Security
194194

195195
* The `LdapUserProvider` class has been deprecated, use `Symfony\Component\Ldap\Security\LdapUserProvider` instead.
196196
* Implementations of `PasswordEncoderInterface` and `UserPasswordEncoderInterface` should add a new `needsRehash()` method
197+
* Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`. Please explicitly return `false` to indicate invalid credentials.
197198

198199
Stopwatch
199200
---------

‎UPGRADE-5.0.md

+1
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,7 @@ Security
467467
* The `BCryptPasswordEncoder` class has been removed, use `NativePasswordEncoder` instead.
468468
* Classes implementing the `TokenInterface` must implement the two new methods
469469
`__serialize` and `__unserialize`
470+
* Implementations of `Guard\AuthenticatorInterface::checkCredentials()` must return a boolean value now. Please explicitly return `false` to indicate invalid credentials.
470471

471472
SecurityBundle
472473
--------------

‎src/Symfony/Component/Security/CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ CHANGELOG
1111
* Added `Guard\PasswordAuthenticatedInterface`, an optional interface
1212
for "guard" authenticators that deal with user passwords
1313
* Marked all dispatched event classes as `@final`
14+
* Deprecated returning a non-boolean value when implementing `Guard\AuthenticatorInterface::checkCredentials()`.
1415

1516
4.3.0
1617
-----

‎src/Symfony/Component/Security/Guard/AuthenticatorInterface.php

+2-3
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,8 @@ public function getUser($credentials, UserProviderInterface $userProvider);
8383
/**
8484
* Returns true if the credentials are valid.
8585
*
86-
* If any value other than true is returned, authentication will
87-
* fail. You may also throw an AuthenticationException if you wish
88-
* to cause authentication to fail.
86+
* If false is returned, authentication will fail. You may also throw
87+
* an AuthenticationException if you wish to cause authentication to fail.
8988
*
9089
* The *credentials* are the return value from getCredentials()
9190
*

‎src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php

+5-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,11 @@ private function authenticateViaGuard(AuthenticatorInterface $guardAuthenticator
113113
}
114114

115115
$this->userChecker->checkPreAuth($user);
116-
if (true !== $guardAuthenticator->checkCredentials($token->getCredentials(), $user)) {
116+
if (true !== $checkCredentialsResult = $guardAuthenticator->checkCredentials($token->getCredentials(), $user)) {
117+
if (false !== $checkCredentialsResult) {
118+
@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);
119+
}
120+
117121
throw new BadCredentialsException(sprintf('Authentication failed because %s::checkCredentials() did not return true.', \get_class($guardAuthenticator)));
118122
}
119123
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)) {

‎src/Symfony/Component/Security/Guard/Tests/Provider/GuardAuthenticationProviderTest.php

+36
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
16+
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
1617
use Symfony\Component\Security\Core\User\UserInterface;
1718
use Symfony\Component\Security\Guard\AuthenticatorInterface;
1819
use Symfony\Component\Security\Guard\Provider\GuardAuthenticationProvider;
@@ -87,6 +88,41 @@ public function testAuthenticate()
8788
$this->assertSame($authedToken, $actualAuthedToken);
8889
}
8990

91+
public function testCheckCredentialsReturningFalseFailsAuthentication()
92+
{
93+
$this->expectException(BadCredentialsException::class);
94+
$providerKey = 'my_uncool_firewall';
95+
96+
$authenticator = $this->createMock(AuthenticatorInterface::class);
97+
98+
// make sure the authenticator is used
99+
$this->preAuthenticationToken->expects($this->any())
100+
->method('getGuardProviderKey')
101+
// the 0 index, to match the only authenticator
102+
->willReturn('my_uncool_firewall_0');
103+
104+
$this->preAuthenticationToken->expects($this->atLeastOnce())
105+
->method('getCredentials')
106+
->willReturn('non-null-value');
107+
108+
$mockedUser = $this->createMock(UserInterface::class);
109+
$authenticator->expects($this->once())
110+
->method('getUser')
111+
->willReturn($mockedUser);
112+
// checkCredentials is called
113+
$authenticator->expects($this->once())
114+
->method('checkCredentials')
115+
// authentication fails :(
116+
->willReturn(false);
117+
118+
$provider = new GuardAuthenticationProvider([$authenticator], $this->userProvider, $providerKey, $this->userChecker);
119+
$provider->authenticate($this->preAuthenticationToken);
120+
}
121+
122+
/**
123+
* @group legacy
124+
* @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.
125+
*/
90126
public function testCheckCredentialsReturningNonTrueFailsAuthentication()
91127
{
92128
$this->expectException('Symfony\Component\Security\Core\Exception\BadCredentialsException');

0 commit comments

Comments
 (0)