Skip to content

[Security][Guard] GuardAuthenticationProvider::authenticate cannot return null #27016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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\AuthenticationException;
use Symfony\Component\Security\Core\Exception\BadCredentialsException;
use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
use Symfony\Component\Security\Guard\GuardAuthenticatorInterface;
Expand Down Expand Up @@ -63,7 +64,7 @@ public function __construct(array $guardAuthenticators, UserProviderInterface $u
*/
public function authenticate(TokenInterface $token)
{
if (!$this->supports($token)) {
if (!$token instanceof GuardTokenInterface) {
throw new \InvalidArgumentException('GuardAuthenticationProvider only supports GuardTokenInterface.');
}

Expand All @@ -87,19 +88,13 @@ public function authenticate(TokenInterface $token)
throw new AuthenticationExpiredException();
}

// find the *one* GuardAuthenticator that this token originated from
foreach ($this->guardAuthenticators as $key => $guardAuthenticator) {
// get a key that's unique to *this* guard authenticator
// this MUST be the same as GuardAuthenticationListener
$uniqueGuardKey = $this->providerKey.'_'.$key;
$guardAuthenticator = $this->findOriginatingAuthenticator($token);

if ($uniqueGuardKey == $token->getGuardProviderKey()) {
return $this->authenticateViaGuard($guardAuthenticator, $token);
}
if (null === $guardAuthenticator) {
throw new AuthenticationException(sprintf('Token with provider key "%s" did not originate from any of the guard authenticators of provider "%s".', $token->getGuardProviderKey(), $this->providerKey));
}

// no matching authenticator found - but there will be multiple GuardAuthenticationProvider
// instances that will be checked if you have multiple firewalls.
return $this->authenticateViaGuard($guardAuthenticator, $token);
}

private function authenticateViaGuard(GuardAuthenticatorInterface $guardAuthenticator, PreAuthenticationGuardToken $token)
Expand All @@ -108,18 +103,11 @@ private function authenticateViaGuard(GuardAuthenticatorInterface $guardAuthenti
$user = $guardAuthenticator->getUser($token->getCredentials(), $this->userProvider);

if (null === $user) {
throw new UsernameNotFoundException(sprintf(
'Null returned from %s::getUser()',
get_class($guardAuthenticator)
));
throw new UsernameNotFoundException(sprintf('Null returned from %s::getUser()', get_class($guardAuthenticator)));
}

if (!$user instanceof UserInterface) {
throw new \UnexpectedValueException(sprintf(
'The %s::getUser() method must return a UserInterface. You returned %s.',
get_class($guardAuthenticator),
is_object($user) ? get_class($user) : gettype($user)
));
throw new \UnexpectedValueException(sprintf('The %s::getUser() method must return a UserInterface. You returned %s.', get_class($guardAuthenticator), is_object($user) ? get_class($user) : gettype($user)));
}

$this->userChecker->checkPreAuth($user);
Expand All @@ -131,18 +119,37 @@ private function authenticateViaGuard(GuardAuthenticatorInterface $guardAuthenti
// turn the UserInterface into a TokenInterface
$authenticatedToken = $guardAuthenticator->createAuthenticatedToken($user, $this->providerKey);
if (!$authenticatedToken instanceof TokenInterface) {
throw new \UnexpectedValueException(sprintf(
'The %s::createAuthenticatedToken() method must return a TokenInterface. You returned %s.',
get_class($guardAuthenticator),
is_object($authenticatedToken) ? get_class($authenticatedToken) : gettype($authenticatedToken)
));
throw new \UnexpectedValueException(sprintf('The %s::createAuthenticatedToken() method must return a TokenInterface. You returned %s.', get_class($guardAuthenticator), is_object($authenticatedToken) ? get_class($authenticatedToken) : gettype($authenticatedToken)));
}

return $authenticatedToken;
}

private function findOriginatingAuthenticator(PreAuthenticationGuardToken $token)
{
// find the *one* GuardAuthenticator that this token originated from
foreach ($this->guardAuthenticators as $key => $guardAuthenticator) {
// get a key that's unique to *this* guard authenticator
// this MUST be the same as GuardAuthenticationListener
$uniqueGuardKey = $this->providerKey.'_'.$key;

if ($uniqueGuardKey === $token->getGuardProviderKey()) {
return $guardAuthenticator;
}
}

// no matching authenticator found - but there will be multiple GuardAuthenticationProvider
// instances that will be checked if you have multiple firewalls.

return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does Symfony use explicit null returns at the end of function bodies? (I'm not sure, so don't immediately change this)

Copy link
Member

@chalasr chalasr Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it does :) this method could have a ?AuthenticatorInterface return type starting from 4.0, explicit null would be mandatory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those two are mutually exclusive, a nullable return type cannot have an implicit return value of null. Should I fix something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, read your comment wrong, thought it read 'implicit null' for some reason. Shall I add the return type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@biomedia-thomas nope. If this patch gets merged, it must be on 2.8 (the lowest maintained branch where it applies) where return types cannot be added due to support of old PHP versions. We'll take care of adding it when merging it up to master

}

public function supports(TokenInterface $token)
{
if ($token instanceof PreAuthenticationGuardToken) {
return null !== $this->findOriginatingAuthenticator($token);
}

return $token instanceof GuardTokenInterface;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\Security\Guard\Provider\GuardAuthenticationProvider;
use Symfony\Component\Security\Guard\Token\PostAuthenticationGuardToken;
use Symfony\Component\Security\Guard\Token\PreAuthenticationGuardToken;

/**
* @author Ryan Weaver <weaverryan@gmail.com>
Expand Down Expand Up @@ -133,6 +134,40 @@ public function testGuardWithNoLongerAuthenticatedTriggersLogout()
$actualToken = $provider->authenticate($token);
}

public function testSupportsChecksGuardAuthenticatorsTokenOrigin()
{
$authenticatorA = $this->getMockBuilder('Symfony\Component\Security\Guard\GuardAuthenticatorInterface')->getMock();
$authenticatorB = $this->getMockBuilder('Symfony\Component\Security\Guard\GuardAuthenticatorInterface')->getMock();
$authenticators = array($authenticatorA, $authenticatorB);

$mockedUser = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserInterface')->getMock();
$provider = new GuardAuthenticationProvider($authenticators, $this->userProvider, 'first_firewall', $this->userChecker);

$token = new PreAuthenticationGuardToken($mockedUser, 'first_firewall_1');
$supports = $provider->supports($token);
$this->assertTrue($supports);

$token = new PreAuthenticationGuardToken($mockedUser, 'second_firewall_0');
$supports = $provider->supports($token);
$this->assertFalse($supports);
}

/**
* @expectedException \Symfony\Component\Security\Core\Exception\AuthenticationException
* @expectedExceptionMessageRegExp /second_firewall_0/
*/
public function testAuthenticateFailsOnNonOriginatingToken()
{
$authenticatorA = $this->getMockBuilder('Symfony\Component\Security\Guard\GuardAuthenticatorInterface')->getMock();
$authenticators = array($authenticatorA);

$mockedUser = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserInterface')->getMock();
$provider = new GuardAuthenticationProvider($authenticators, $this->userProvider, 'first_firewall', $this->userChecker);

$token = new PreAuthenticationGuardToken($mockedUser, 'second_firewall_0');
$provider->authenticate($token);
}

protected function setUp()
{
$this->userProvider = $this->getMockBuilder('Symfony\Component\Security\Core\User\UserProviderInterface')->getMock();
Expand Down