From 8277afbccf9ffc44764692852b071bc58d995a09 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sat, 3 Aug 2024 16:36:12 +0100 Subject: [PATCH 1/9] Issue invalid grant if auth code is not correct. Invalid request if decryption issue --- src/CryptTrait.php | 14 ++++++++++++++ src/Grant/AuthCodeGrant.php | 7 +++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/CryptTrait.php b/src/CryptTrait.php index 8b1946fc4..6a440829b 100644 --- a/src/CryptTrait.php +++ b/src/CryptTrait.php @@ -16,7 +16,10 @@ use Defuse\Crypto\Crypto; use Defuse\Crypto\Key; +use Defuse\Crypto\Exception\EnvironmentIsBrokenException; +use Defuse\Crypto\Exception\WrongKeyOrModifiedCiphertextException; use Exception; +use InvalidArgumentException; use LogicException; use function is_string; @@ -64,6 +67,17 @@ protected function decrypt(string $encryptedData): string } throw new LogicException('Encryption key not set when attempting to decrypt'); + } catch (WrongKeyOrModifiedCiphertextException $e) { + $exceptionMessage = 'The authcode or decryption key/password used ' + . 'is not correct'; + + throw new InvalidArgumentException($exceptionMessage, 0, $e); + } catch (EnvironmentIsBrokenException $e) { + $exceptionMessage = 'Auth code decryption failed. This is likely ' + . 'due to an environment issue or runtime bug in the ' + . 'decryption library'; + + throw new LogicException($exceptionMessage, 0, $e); } catch (Exception $e) { throw new LogicException($e->getMessage(), 0, $e); } diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 8a24a8e95..6a9821234 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -15,6 +15,7 @@ use DateInterval; use DateTimeImmutable; use Exception; +use InvalidArgumentException; use League\OAuth2\Server\CodeChallengeVerifiers\CodeChallengeVerifierInterface; use League\OAuth2\Server\CodeChallengeVerifiers\PlainVerifier; use League\OAuth2\Server\CodeChallengeVerifiers\S256Verifier; @@ -123,8 +124,10 @@ public function respondToAccessTokenRequest( $authCodePayload->user_id, $authCodePayload->auth_code_id ); + } catch (InvalidArgumentException $e) { + throw OAuthServerException::invalidGrant('Cannot validate the provided authorization code'); } catch (LogicException $e) { - throw OAuthServerException::invalidRequest('code', 'Cannot decrypt the authorization code', $e); + throw OAuthServerException::invalidRequest('code', 'Issue decrypting the authorization code', $e); } $codeVerifier = $this->getRequestParameter('code_verifier', $request); @@ -206,7 +209,7 @@ private function validateAuthorizationCode( } if (time() > $authCodePayload->expire_time) { - throw OAuthServerException::invalidRequest('code', 'Authorization code has expired'); + throw OAuthServerException::invalidGrant('Authorization code has expired'); } if ($this->authCodeRepository->isAuthCodeRevoked($authCodePayload->auth_code_id) === true) { From 16d654de6c02e6aa5d6984273d99f5696325f045 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sat, 3 Aug 2024 17:11:55 +0100 Subject: [PATCH 2/9] Update tests --- src/CryptTrait.php | 2 +- tests/Grant/AuthCodeGrantTest.php | 62 +++++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/CryptTrait.php b/src/CryptTrait.php index 6a440829b..ee481b55c 100644 --- a/src/CryptTrait.php +++ b/src/CryptTrait.php @@ -15,9 +15,9 @@ namespace League\OAuth2\Server; use Defuse\Crypto\Crypto; -use Defuse\Crypto\Key; use Defuse\Crypto\Exception\EnvironmentIsBrokenException; use Defuse\Crypto\Exception\WrongKeyOrModifiedCiphertextException; +use Defuse\Crypto\Key; use Exception; use InvalidArgumentException; use LogicException; diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index 6a6842661..2bcd756e3 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -1450,7 +1450,7 @@ public function testRespondToAccessTokenRequestClientMismatch(): void } } - public function testRespondToAccessTokenRequestBadCodeEncryption(): void + public function testRespondToAccessTokenRequestBadCode(): void { $client = new ClientEntity(); @@ -1492,7 +1492,7 @@ public function testRespondToAccessTokenRequestBadCodeEncryption(): void 'grant_type' => 'authorization_code', 'client_id' => 'foo', 'redirect_uri' => self::REDIRECT_URI, - 'code' => 'sdfsfsd', + 'code' => 'badCode', ] ); @@ -1500,10 +1500,66 @@ public function testRespondToAccessTokenRequestBadCodeEncryption(): void /* @var StubResponseType $response */ $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M')); } catch (OAuthServerException $e) { - self::assertEquals($e->getHint(), 'Cannot decrypt the authorization code'); + self::assertEquals($e->getErrorType(), 'invalid_grant'); + self::assertEquals($e->getHint(), 'Cannot validate the provided authorization code'); } } +public function testRespondToAccessTokenRequestNoEncryptionKey(): 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(); + + $grant = new AuthCodeGrant( + $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(), + $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), + new DateInterval('PT10M') + ); + $grant->setClientRepository($clientRepositoryMock); + $grant->setAccessTokenRepository($accessTokenRepositoryMock); + $grant->setRefreshTokenRepository($refreshTokenRepositoryMock); + // We deliberately don't set an encryption key here + + $request = new ServerRequest( + [], + [], + null, + 'POST', + 'php://input', + [], + [], + [], + [ + 'grant_type' => 'authorization_code', + 'client_id' => 'foo', + 'redirect_uri' => self::REDIRECT_URI, + 'code' => 'badCode', + ] + ); + + try { + /* @var StubResponseType $response */ + $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M')); + } catch (OAuthServerException $e) { + self::assertEquals($e->getErrorType(), 'invalid_request'); + self::assertEquals($e->getHint(), 'Issue decrypting the authorization code'); + } +} + public function testRespondToAccessTokenRequestBadCodeVerifierPlain(): void { $client = new ClientEntity(); From 46a504fe4ae711c40ef9637938b58f2650f82d11 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sat, 3 Aug 2024 17:16:54 +0100 Subject: [PATCH 3/9] Fix coding standard issues --- tests/Grant/AuthCodeGrantTest.php | 96 +++++++++++++++---------------- 1 file changed, 48 insertions(+), 48 deletions(-) diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index 2bcd756e3..e041b45a6 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -1505,60 +1505,60 @@ public function testRespondToAccessTokenRequestBadCode(): void } } -public function testRespondToAccessTokenRequestNoEncryptionKey(): 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(); - - $grant = new AuthCodeGrant( - $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(), - $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), - new DateInterval('PT10M') - ); - $grant->setClientRepository($clientRepositoryMock); - $grant->setAccessTokenRepository($accessTokenRepositoryMock); - $grant->setRefreshTokenRepository($refreshTokenRepositoryMock); - // We deliberately don't set an encryption key here - - $request = new ServerRequest( - [], - [], - null, - 'POST', - 'php://input', - [], - [], - [], - [ + public function testRespondToAccessTokenRequestNoEncryptionKey(): 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(); + + $grant = new AuthCodeGrant( + $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(), + $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), + new DateInterval('PT10M') + ); + $grant->setClientRepository($clientRepositoryMock); + $grant->setAccessTokenRepository($accessTokenRepositoryMock); + $grant->setRefreshTokenRepository($refreshTokenRepositoryMock); + // We deliberately don't set an encryption key here + + $request = new ServerRequest( + [], + [], + null, + 'POST', + 'php://input', + [], + [], + [], + [ 'grant_type' => 'authorization_code', 'client_id' => 'foo', 'redirect_uri' => self::REDIRECT_URI, 'code' => 'badCode', - ] - ); + ] + ); - try { - /* @var StubResponseType $response */ - $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M')); - } catch (OAuthServerException $e) { - self::assertEquals($e->getErrorType(), 'invalid_request'); - self::assertEquals($e->getHint(), 'Issue decrypting the authorization code'); + try { + /* @var StubResponseType $response */ + $grant->respondToAccessTokenRequest($request, new StubResponseType(), new DateInterval('PT10M')); + } catch (OAuthServerException $e) { + self::assertEquals($e->getErrorType(), 'invalid_request'); + self::assertEquals($e->getHint(), 'Issue decrypting the authorization code'); + } } -} public function testRespondToAccessTokenRequestBadCodeVerifierPlain(): void { From 5e69f11ee9d531d62ebee32fc9e6686aa22f0bf1 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Sat, 3 Aug 2024 17:20:10 +0100 Subject: [PATCH 4/9] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f0b78058..a8e99966b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [Unreleased] +### Fixed +- In the Auth Code grant, when requesting an access token with an invalid auth code, we now respond with an invalid_grant error instead of invalid_request (PR #1433) + ## [9.0.0] - released 2024-05-13 ### Added - Device Authorization Grant added (PR #1074) From d5419d58cbbc41c8e6691ed6e1b51713151c174c Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Thu, 8 Aug 2024 09:26:37 -0400 Subject: [PATCH 5/9] Fix refresh token int user ID --- src/Grant/RefreshTokenGrant.php | 2 +- tests/Grant/RefreshTokenGrantTest.php | 64 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/src/Grant/RefreshTokenGrant.php b/src/Grant/RefreshTokenGrant.php index a632990c7..42cef93d4 100644 --- a/src/Grant/RefreshTokenGrant.php +++ b/src/Grant/RefreshTokenGrant.php @@ -78,7 +78,7 @@ public function respondToAccessTokenRequest( } // Issue and persist new access token - $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $oldRefreshToken['user_id'], $scopes); + $accessToken = $this->issueAccessToken($accessTokenTTL, $client, (string)$oldRefreshToken['user_id'], $scopes); $this->getEmitter()->emit(new RequestAccessTokenEvent(RequestEvent::ACCESS_TOKEN_ISSUED, $request, $accessToken)); $responseType->setAccessToken($accessToken); diff --git a/tests/Grant/RefreshTokenGrantTest.php b/tests/Grant/RefreshTokenGrantTest.php index b37001a80..0b1f090fa 100644 --- a/tests/Grant/RefreshTokenGrantTest.php +++ b/tests/Grant/RefreshTokenGrantTest.php @@ -734,4 +734,68 @@ public function testUnrevokedRefreshToken(): void self::assertFalse($refreshTokenRepositoryMock->isRefreshTokenRevoked($refreshTokenId)); } + + public function testRespondToRequestWithIntUserId(): void + { + $client = new ClientEntity(); + $client->setIdentifier('foo'); + $client->setRedirectUri('http://foo/bar'); + + $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); + $clientRepositoryMock->method('getClientEntity')->willReturn($client); + $clientRepositoryMock->method('validateClient')->willReturn(true); + + $scopeEntity = new ScopeEntity(); + $scopeEntity->setIdentifier('foo'); + $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); + $scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity); + $scopeRepositoryMock->method('finalizeScopes')->willReturn([$scopeEntity]); + + $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); + $accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity()); + $accessTokenRepositoryMock->expects(self::once())->method('persistNewAccessToken')->willReturnSelf(); + + $refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(); + $refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity()); + $refreshTokenRepositoryMock->expects(self::once())->method('persistNewRefreshToken')->willReturnSelf(); + + $grant = new RefreshTokenGrant($refreshTokenRepositoryMock); + $grant->setClientRepository($clientRepositoryMock); + $grant->setScopeRepository($scopeRepositoryMock); + $grant->setAccessTokenRepository($accessTokenRepositoryMock); + $grant->setEncryptionKey($this->cryptStub->getKey()); + $grant->setPrivateKey(new CryptKey('file://' . __DIR__ . '/../Stubs/private.key')); + $grant->revokeRefreshTokens(true); + + $oldRefreshToken = json_encode( + [ + 'client_id' => 'foo', + 'refresh_token_id' => 'zyxwvu', + 'access_token_id' => 'abcdef', + 'scopes' => ['foo'], + 'user_id' => 123, + 'expire_time' => time() + 3600, + ] + ); + + if ($oldRefreshToken === false) { + self::fail('json_encode failed'); + } + + $encryptedOldRefreshToken = $this->cryptStub->doEncrypt( + $oldRefreshToken + ); + + $serverRequest = (new ServerRequest())->withParsedBody([ + 'client_id' => 'foo', + 'client_secret' => 'bar', + 'refresh_token' => $encryptedOldRefreshToken, + 'scopes' => ['foo'], + ]); + + $responseType = new StubResponseType(); + $grant->respondToAccessTokenRequest($serverRequest, $responseType, new DateInterval('PT5M')); + + self::assertInstanceOf(RefreshTokenEntityInterface::class, $responseType->getRefreshToken()); + } } From 113bb14daca8f03b74bb6113c38b49e4f7a9b3b9 Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Thu, 8 Aug 2024 09:30:05 -0400 Subject: [PATCH 6/9] Prevent casing null user ID to string --- src/Grant/RefreshTokenGrant.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Grant/RefreshTokenGrant.php b/src/Grant/RefreshTokenGrant.php index 42cef93d4..3a9516097 100644 --- a/src/Grant/RefreshTokenGrant.php +++ b/src/Grant/RefreshTokenGrant.php @@ -78,7 +78,10 @@ public function respondToAccessTokenRequest( } // Issue and persist new access token - $accessToken = $this->issueAccessToken($accessTokenTTL, $client, (string)$oldRefreshToken['user_id'], $scopes); + $userId = $oldRefreshToken['user_id']; + if (is_int($userId)) + $userId = (string)$userId; + $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $userId, $scopes); $this->getEmitter()->emit(new RequestAccessTokenEvent(RequestEvent::ACCESS_TOKEN_ISSUED, $request, $accessToken)); $responseType->setAccessToken($accessToken); From e0d630c99bb0d3b9b6d99186690e1b2ff0e608b6 Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Thu, 8 Aug 2024 09:30:53 -0400 Subject: [PATCH 7/9] Fix coding style --- src/Grant/RefreshTokenGrant.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Grant/RefreshTokenGrant.php b/src/Grant/RefreshTokenGrant.php index 3a9516097..f0488d3d8 100644 --- a/src/Grant/RefreshTokenGrant.php +++ b/src/Grant/RefreshTokenGrant.php @@ -79,8 +79,9 @@ public function respondToAccessTokenRequest( // Issue and persist new access token $userId = $oldRefreshToken['user_id']; - if (is_int($userId)) + if (is_int($userId)) { $userId = (string)$userId; + } $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $userId, $scopes); $this->getEmitter()->emit(new RequestAccessTokenEvent(RequestEvent::ACCESS_TOKEN_ISSUED, $request, $accessToken)); $responseType->setAccessToken($accessToken); From c406da5818388f3c6b7b5539a0ccf67ed1702ce2 Mon Sep 17 00:00:00 2001 From: Stephen Sigwart Date: Thu, 8 Aug 2024 09:31:26 -0400 Subject: [PATCH 8/9] More code style fixes --- src/Grant/RefreshTokenGrant.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Grant/RefreshTokenGrant.php b/src/Grant/RefreshTokenGrant.php index f0488d3d8..35de791fb 100644 --- a/src/Grant/RefreshTokenGrant.php +++ b/src/Grant/RefreshTokenGrant.php @@ -80,7 +80,7 @@ public function respondToAccessTokenRequest( // Issue and persist new access token $userId = $oldRefreshToken['user_id']; if (is_int($userId)) { - $userId = (string)$userId; + $userId = (string) $userId; } $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $userId, $scopes); $this->getEmitter()->emit(new RequestAccessTokenEvent(RequestEvent::ACCESS_TOKEN_ISSUED, $request, $accessToken)); From dd22d86fd11ecb9eba48d7258cbf1ab2a5ec0dc6 Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Thu, 14 Nov 2024 23:02:21 +0000 Subject: [PATCH 9/9] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb6ca90ea..975bbe398 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - Fixed spec compliance issue where device access token request was mistakenly expecting to receive scopes in the request (PR #1412) +- Refresh tokens pre version 9 might have had user IDs set as ints which meant they were incorrectly rejected. We now cast these values to strings to allow old refresh tokens (PR #1436) ## [9.0.1] - released 2024-10-14 ### Fixed