From ceb14dceb25e62f7562b0beb377ff41495215c78 Mon Sep 17 00:00:00 2001 From: Marc Riemer Date: Tue, 21 May 2024 17:32:37 +0200 Subject: [PATCH] Fix for a possible race condition. #1306 --- src/Grant/AuthCodeGrant.php | 44 +++++++----- .../AuthCodeRepositoryInterface.php | 22 ++++++ tests/Grant/AuthCodeGrantTest.php | 67 +++++++++++++++++++ 3 files changed, 118 insertions(+), 15 deletions(-) diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 8a24a8e95..f560d3335 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -141,23 +141,33 @@ public function respondToAccessTokenRequest( $this->validateCodeChallenge($authCodePayload, $codeVerifier); } - // Issue and persist new access token - $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $authCodePayload->user_id, $scopes); - $this->getEmitter()->emit(new RequestAccessTokenEvent(RequestEvent::ACCESS_TOKEN_ISSUED, $request, $accessToken)); - $responseType->setAccessToken($accessToken); - - // Issue and persist new refresh token if given - $refreshToken = $this->issueRefreshToken($accessToken); + if ($this->authCodeRepository->lockAuthCode($authCodePayload->auth_code_id)) { + try { + // Issue and persist new access token + $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $authCodePayload->user_id, $scopes); + $this->getEmitter()->emit(new RequestAccessTokenEvent(RequestEvent::ACCESS_TOKEN_ISSUED, $request, $accessToken)); + $responseType->setAccessToken($accessToken); + + // Issue and persist new refresh token if given + $refreshToken = $this->issueRefreshToken($accessToken); + + if ($refreshToken !== null) { + $this->getEmitter()->emit(new RequestRefreshTokenEvent(RequestEvent::REFRESH_TOKEN_ISSUED, $request, $refreshToken)); + $responseType->setRefreshToken($refreshToken); + } - if ($refreshToken !== null) { - $this->getEmitter()->emit(new RequestRefreshTokenEvent(RequestEvent::REFRESH_TOKEN_ISSUED, $request, $refreshToken)); - $responseType->setRefreshToken($refreshToken); + return $responseType; + } catch (Exception $e) { + $this->authCodeRepository->unlockAuthCode($authCodePayload->auth_code_id); + throw OAuthServerException::serverError( + 'access_token', + $e + ); + } finally { + // Revoke used auth code + $this->authCodeRepository->revokeAuthCode($authCodePayload->auth_code_id); + } } - - // Revoke used auth code - $this->authCodeRepository->revokeAuthCode($authCodePayload->auth_code_id); - - return $responseType; } private function validateCodeChallenge(object $authCodePayload, ?string $codeVerifier): void @@ -213,6 +223,10 @@ private function validateAuthorizationCode( throw OAuthServerException::invalidGrant('Authorization code has been revoked'); } + if ($this->authCodeRepository->isAuthCodeLocked($authCodePayload->auth_code_id) === true) { + throw OAuthServerException::invalidGrant('Authorization code has been locked while an access code beeing issued'); + } + if ($authCodePayload->client_id !== $client->getIdentifier()) { throw OAuthServerException::invalidRequest('code', 'Authorization code was not issued to this client'); } diff --git a/src/Repositories/AuthCodeRepositoryInterface.php b/src/Repositories/AuthCodeRepositoryInterface.php index 89ff86b87..e2a0720fb 100644 --- a/src/Repositories/AuthCodeRepositoryInterface.php +++ b/src/Repositories/AuthCodeRepositoryInterface.php @@ -30,4 +30,26 @@ public function persistNewAuthCode(AuthCodeEntityInterface $authCodeEntity): voi public function revokeAuthCode(string $codeId): void; public function isAuthCodeRevoked(string $codeId): bool; + + /** + * Method locks an auth code in case an error occurs during the issuance of an access code. + * + * The storage engine should make this persistent immediately to prevent possible race conditions while issuing an access token. + * + * @param string $codeId + * + * @return void + */ + public function lockAuthCode(string $codeId): bool; + + /** + * This method is used to make the auth code available again after an error occurred in the access code issuance process. + * + * @param string $codeId + * + * @return void + */ + public function unlockAuthCode(string $codeId): void; + + public function isAuthCodeLocked(string $codeId): bool; } diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index 6a6842661..84a18fba6 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -1387,6 +1387,73 @@ public function testRespondToAccessTokenRequestRevokedCode(): void } } + public function testRespondToAccessTokenRequestLockedCode(): void + { + $client = new ClientEntity(); + + $client->setIdentifier('foo'); + $client->setRedirectUri(self::REDIRECT_URI); + $client->setConfidential(); + + $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); + + $clientRepositoryMock->method('getClientEntity')->willReturn($client); + $clientRepositoryMock->method('validateClient')->willReturn(true); + + $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); + $accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf(); + + $refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(); + $refreshTokenRepositoryMock->method('persistNewRefreshToken')->willReturnSelf(); + + $authCodeRepositoryMock = $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(); + $authCodeRepositoryMock->method('isAuthCodeLocked')->willReturn(true); + + $grant = new AuthCodeGrant( + $authCodeRepositoryMock, + $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), + new DateInterval('PT10M') + ); + $grant->setClientRepository($clientRepositoryMock); + $grant->setAccessTokenRepository($accessTokenRepositoryMock); + $grant->setRefreshTokenRepository($refreshTokenRepositoryMock); + $grant->setEncryptionKey($this->cryptStub->getKey()); + + $request = new ServerRequest( + [], + [], + null, + 'POST', + 'php://input', + [], + [], + [], + [ + 'grant_type' => 'authorization_code', + 'client_id' => 'foo', + 'redirect_uri' => self::REDIRECT_URI, + 'code' => $this->cryptStub->doEncrypt( + json_encode([ + 'auth_code_id' => uniqid(), + 'expire_time' => time() + 3600, + 'client_id' => 'foo', + 'user_id' => 123, + 'scopes' => ['foo'], + 'redirect_uri' => 'http://foo/bar', + ], JSON_THROW_ON_ERROR) + ), + ] + ); + + try { + /* @var StubResponseType $response */ + $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M')); + } catch (OAuthServerException $e) { + self::assertEquals($e->getHint(), 'Authorization code has been revoked'); + self::assertEquals($e->getErrorType(), 'invalid_grant'); + } + } + public function testRespondToAccessTokenRequestClientMismatch(): void { $client = new ClientEntity();