diff --git a/Security/Authentication/Authenticator/OAuthAuthenticator.php b/Security/Authentication/Authenticator/OAuthAuthenticator.php index f9089f9d..1ea627db 100644 --- a/Security/Authentication/Authenticator/OAuthAuthenticator.php +++ b/Security/Authentication/Authenticator/OAuthAuthenticator.php @@ -25,6 +25,7 @@ use Symfony\Component\Security\Core\Exception\AccountStatusException; use Symfony\Component\Security\Core\Exception\AuthenticationException; use Symfony\Component\Security\Core\User\UserCheckerInterface; +use Symfony\Component\Security\Core\User\UserInterface; use Symfony\Component\Security\Core\User\UserProviderInterface; use Symfony\Component\Security\Http\Authenticator\AuthenticatorInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; @@ -76,42 +77,42 @@ public function __construct( */ public function authenticate(Request $request): UserPassportInterface { - try { - $token = $this->tokenStorage->getToken(); - $tokenString = $token->getToken(); + // remove the authorization header from the request on this check + $tokenString = $this->serverService->getBearerToken($request, true); + $accessToken = $scope = $user = $username = null; + try { $accessToken = $this->serverService->verifyAccessToken($tokenString); - - /** @var \Symfony\Component\Security\Core\User\UserInterface **/ + $scope = $accessToken->getScope(); $user = $accessToken->getUser(); + // allow for dependency on deprecated getUsername method + $username = $user instanceof UserInterface + ? (method_exists($user, 'getUserIdentifier') ? $user->getUserIdentifier() : $user->getUsername()) + : null + ; + } catch (OAuth2AuthenticateException $e) { + // do nothing - credentials will remain unresolved below + } - if (null === $user) { - throw new AuthenticationException('OAuth2 authentication failed'); - } + // configure the passport badges, ensuring requisite string types + $userBadge = new UserBadge($username ?? ''); + $credentials = new OAuthCredentials($tokenString ?? '', $scope ?? ''); - // check the user + // check the user if not null + if ($user instanceof UserInterface) { try { $this->userChecker->checkPreAuth($user); + + // mark the credentials as resolved + $credentials->markResolved(); } catch (AccountStatusException $e) { - throw new OAuth2AuthenticateException( - Response::HTTP_UNAUTHORIZED, - OAuth2::TOKEN_TYPE_BEARER, - $this->serverService->getVariable(OAuth2::CONFIG_WWW_REALM), - 'access_denied', - $e->getMessage() - ); + // do nothing - credentials remain unresolved } - - return new Passport( - new UserBadge($user->getUsername()), - new OAuthCredentials($tokenString, $accessToken->getScope()) - ); - } catch (OAuth2ServerException $e) { - throw new AuthenticationException('OAuth2 authentication failed', 0, $e); } - // this should never be reached - throw new AuthenticationException('OAuth2 authentication failed'); + // passport will only be valid if all badges are resolved (user badge + // is always resolved, credentials badge if passing the above check) + return new Passport($userBadge, $credentials); } /** @@ -160,7 +161,7 @@ public function createAuthenticatedToken(PassportInterface $passport, string $fi $token->setToken($credentials->getTokenString()); $token->setUser($user); - $credentials->markResolved(); + $this->tokenStorage->setToken($token); return $token; } @@ -194,6 +195,9 @@ public function onAuthenticationFailure(Request $request, AuthenticationExceptio */ public function supports(Request $request): ?bool { - return $this->tokenStorage->getToken() instanceof OAuthToken; + // do not remove the authorization header from the request on this check + $tokenString = $this->serverService->getBearerToken($request); + + return is_string($tokenString) && !empty($tokenString); } } diff --git a/Security/Authentication/Passport/OAuthCredentials.php b/Security/Authentication/Passport/OAuthCredentials.php index 214f3323..430250cb 100644 --- a/Security/Authentication/Passport/OAuthCredentials.php +++ b/Security/Authentication/Passport/OAuthCredentials.php @@ -69,8 +69,6 @@ public function getTokenString(): ?string public function markResolved(): void { $this->resolved = true; - $this->scope = null; - $this->tokenString = null; } public function isResolved(): bool diff --git a/Tests/Security/Authentication/Authenticator/OAuthAuthenticatorTest.php b/Tests/Security/Authentication/Authenticator/OAuthAuthenticatorTest.php index 91d49f2c..ae83ba69 100644 --- a/Tests/Security/Authentication/Authenticator/OAuthAuthenticatorTest.php +++ b/Tests/Security/Authentication/Authenticator/OAuthAuthenticatorTest.php @@ -26,7 +26,7 @@ use Symfony\Component\Security\Core\Exception\CredentialsExpiredException; use Symfony\Component\Security\Core\Exception\DisabledException; use Symfony\Component\Security\Core\User\UserCheckerInterface; -use Symfony\Component\Security\Core\User\UserInterface; +use Symfony\Component\Security\Core\User\User; use Symfony\Component\Security\Core\User\UserProviderInterface; use Symfony\Component\Security\Http\Authenticator\Passport\Badge\UserBadge; use Symfony\Component\Security\Http\Authenticator\Passport\Credentials\PasswordCredentials; @@ -50,7 +50,7 @@ class OAuthAuthenticatorTest extends \PHPUnit\Framework\TestCase protected $tokenStorage; /** - * @var \PHPUnit\Framework\MockObject\MockObject|UserInterface + * @var \PHPUnit\Framework\MockObject\MockObject|User */ protected $user; @@ -69,16 +69,20 @@ public function setUp(): void $this->serverService = $this->getMockBuilder(OAuth2::class) ->disableOriginalConstructor() ->setMethods([ + 'getBearerToken', 'getVariable', - 'verifyAccessToken' + 'verifyAccessToken', ]) ->getMock() ; $this->tokenStorage = $this->getMockBuilder(TokenStorageInterface::class)->disableOriginalConstructor()->getMock(); - $this->user = $this->getMockBuilder(UserInterface::class)->disableOriginalConstructor()->getMock(); $this->userChecker = $this->getMockBuilder(UserCheckerInterface::class)->disableOriginalConstructor()->getMock(); $this->userProvider = $this->getMockBuilder(UserProviderInterface::class)->disableOriginalConstructor()->getMock(); + // mock the core user object rather than the user interface that the new + // getUserIdentifier method is used rather than the deprecated getUsername + $this->user = $this->getMockBuilder(User::class)->disableOriginalConstructor()->getMock(); + $this->authenticator = new OAuthAuthenticator( $this->serverService, $this->tokenStorage, @@ -89,12 +93,15 @@ public function setUp(): void public function testAuthenticateReturnsPassportIfValid(): void { - // expect a token from the token storage - $token = new OAuthToken(); - $token->setToken('mock_token_string'); - $this->tokenStorage->expects($this->once()) - ->method('getToken') - ->will($this->returnValue($token)) + // expect the OAuth2 service to get the token from the request header, + // flagging the authorization header to be removed at the same time + $this->serverService->expects($this->once()) + ->method('getBearerToken') + ->with( + $this->isInstanceOf(Request::class), + $this->equalTo(true) + ) + ->will($this->returnValue('mock_token_string')) ; // expect the OAuth2 service to verify the token, returning an access token @@ -107,18 +114,18 @@ public function testAuthenticateReturnsPassportIfValid(): void ->will($this->returnValue($accessToken)) ; + // expect the username from the user + $this->user->expects($this->once()) + ->method('getUserIdentifier') + ->will($this->returnValue('test_user')) + ; + // expect the user checker to pass $this->userChecker->expects($this->once()) ->method('checkPreAuth') ->with($this->user) ; - // expect the username from the user - $this->user->expects($this->once()) - ->method('getUsername') - ->will($this->returnValue('test_user')) - ; - $passport = $this->authenticator->authenticate(new Request()); $this->assertInstanceOf(Passport::class, $passport); @@ -128,16 +135,20 @@ public function testAuthenticateReturnsPassportIfValid(): void $this->assertSame('test_user', $passport->getBadge(UserBadge::class)->getUserIdentifier()); $this->assertSame('mock_token_string', $passport->getBadge(OAuthCredentials::class)->getTokenString()); $this->assertSame(['ROLE_SCOPE_1', 'ROLE_SCOPE_2'], $passport->getBadge(OAuthCredentials::class)->getRoles($this->user)); + $this->assertTrue($passport->getBadge(OAuthCredentials::class)->isResolved()); } - public function testAuthenticateReturnsTokenInvalidWhenNullData(): void + public function testAuthenticateReturnsUnresolvedPassportWhenNullUser(): void { - // expect a token from the token storage - $token = new OAuthToken(); - $token->setToken('mock_token_string'); - $this->tokenStorage->expects($this->once()) - ->method('getToken') - ->will($this->returnValue($token)) + // expect the OAuth2 service to get the token from the request header, + // flagging the authorization header to be removed at the same time + $this->serverService->expects($this->once()) + ->method('getBearerToken') + ->with( + $this->isInstanceOf(Request::class), + $this->equalTo(true) + ) + ->will($this->returnValue('mock_token_string')) ; // expect the OAuth2 service to verify the token, returning an access @@ -149,26 +160,29 @@ public function testAuthenticateReturnsTokenInvalidWhenNullData(): void ->will($this->returnValue($accessToken)) ; - // expect an authentication exception - $this->expectException(AuthenticationException::class); - $this->expectExceptionMessage('OAuth2 authentication failed'); + // expect the null user value to not be processed + $this->userChecker->expects($this->never())->method('checkPreAuth'); - $this->authenticator->authenticate(new Request()); + $passport = $this->authenticator->authenticate(new Request()); + + // confirm that the returned passport won't pass validation + $this->assertFalse($passport->getBadge(OAuthCredentials::class)->isResolved()); } - public function testAuthenticateTransformsOAuthServerException(): void + public function testAuthenticateReturnsUnresolvedPassportWhenInvalidToken(): void { - // expect a token from the token storage - $token = new OAuthToken(); - $token->setToken('mock_token_string'); - $this->tokenStorage->expects($this->once()) - ->method('getToken') - ->will($this->returnValue($token)) + // expect the OAuth2 service to get the token from the request header, + // flagging the authorization header to be removed at the same time + $this->serverService->expects($this->once()) + ->method('getBearerToken') + ->with( + $this->isInstanceOf(Request::class), + $this->equalTo(true) + ) + ->will($this->returnValue('mock_token_string')) ; - // expect the OAuth2 service to verify the token, returning an access - // token, but without a related user - $accessToken = new AccessToken(); + // expect the OAuth2 service to not verify the token, throwing an exception $this->serverService->expects($this->once()) ->method('verifyAccessToken') ->with('mock_token_string') @@ -182,21 +196,26 @@ public function testAuthenticateTransformsOAuthServerException(): void )) ; - // expect the thrown exception to be transformed into an authentication exception - $this->expectException(AuthenticationException::class); - $this->expectExceptionMessage('OAuth2 authentication failed'); + // expect the null user value to not be processed + $this->userChecker->expects($this->never())->method('checkPreAuth'); + + $passport = $this->authenticator->authenticate(new Request()); - $this->authenticator->authenticate(new Request()); + // confirm that the returned passport won't pass validation + $this->assertFalse($passport->getBadge(OAuthCredentials::class)->isResolved()); } public function testAuthenticateTransformsAccountStatusException(): void { - // expect a token from the token storage - $token = new OAuthToken(); - $token->setToken('mock_token_string'); - $this->tokenStorage->expects($this->once()) - ->method('getToken') - ->will($this->returnValue($token)) + // expect the OAuth2 service to get the token from the request header, + // flagging the authorization header to be removed at the same time + $this->serverService->expects($this->once()) + ->method('getBearerToken') + ->with( + $this->isInstanceOf(Request::class), + $this->equalTo(true) + ) + ->will($this->returnValue('mock_token_string')) ; // expect the OAuth2 service to verify the token, returning an access token @@ -216,11 +235,10 @@ public function testAuthenticateTransformsAccountStatusException(): void ->willThrowException(new DisabledException('User account is disabled.')) ; - // expect the thrown exception to be transformed into an authentication exception - $this->expectException(AuthenticationException::class); - $this->expectExceptionMessage('OAuth2 authentication failed'); + $passport = $this->authenticator->authenticate(new Request()); - $this->authenticator->authenticate(new Request()); + // confirm that the returned passport won't pass validation + $this->assertFalse($passport->getBadge(OAuthCredentials::class)->isResolved()); } public function testCreateAuthenticatedTokenWithValidPassport(): void @@ -245,6 +263,12 @@ public function testCreateAuthenticatedTokenWithValidPassport(): void ->will($this->returnValue(['ROLE_USER'])) ; + // expect a new authenticated token to be stored + $this->tokenStorage->expects($this->once()) + ->method('setToken') + ->with($this->isInstanceOf(OAuthToken::class)) + ; + // configure the passport $passport = new Passport( new UserBadge('test_user'), diff --git a/Tests/Security/Authentication/Passport/OAuthCredentialsTest.php b/Tests/Security/Authentication/Passport/OAuthCredentialsTest.php index 8302b9be..87f27135 100644 --- a/Tests/Security/Authentication/Passport/OAuthCredentialsTest.php +++ b/Tests/Security/Authentication/Passport/OAuthCredentialsTest.php @@ -62,8 +62,12 @@ public function testMarkResolved(): void $credentials->markResolved(); + // marking credentials as resolved should not change any other state, + // as the transported data is still needed for creating the + // authenticated token when the AuthenticatorManager progresses in + // executing the OAuthAuthenticator $this->assertTrue($credentials->isResolved()); - $this->assertNull($credentials->getTokenString()); - $this->assertSame([], $credentials->getRoles($this->user)); + $this->assertSame('mock_token', $credentials->getTokenString()); + $this->assertSame(['ROLE_SCOPE_1', 'ROLE_SCOPE_2'], $credentials->getRoles($this->user)); } }