diff --git a/CHANGELOG.md b/CHANGELOG.md index 6aa241cbf..fc6045ff6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,12 +8,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - Auto-generated event emitter is now persisted. Previously, a new emitter was generated every time (PR #1428) - Fixed bug where you could not omit a redirect uri even if one had not been specified during the auth request (PR #1428) -- Fixed bug where disabling refresh token revocation via `revokeRefreshTokens(false)` unintentionally disables issuing new refresh token (PR #1449) - +- Fixed bug where "state" parameter wasn't present on `invalid_scope` error response and wasn't on fragment part of `access_denied` redirect URI on Implicit grant (PR #1298) +- Fixed bug where disabling refresh token revocation via `revokeRefreshTokens(false)` unintentionally disables issuing new refresh token (PR #1449) +- ## [9.0.0] - released 2024-05-13 ### Added - Device Authorization Grant added (PR #1074) -- GrantTypeInterface has a new function, `revokeRefreshTokens()` for enabling or disabling refresh token revocation (PR #1375) +- GrantTypeInterface has a new function, `revokeRefreshTokens()` for enabling or disabling refresh tokens after use (PR #1375) - A CryptKeyInterface to allow developers to change the CryptKey implementation with greater ease (PR #1044) - The authorization server can now finalize scopes when a client uses a refresh token (PR #1094) - An AuthorizationRequestInterface to make it easier to extend the AuthorizationRequest (PR #1110) diff --git a/src/Grant/AbstractAuthorizeGrant.php b/src/Grant/AbstractAuthorizeGrant.php index 22b58c4e2..7b44016e4 100644 --- a/src/Grant/AbstractAuthorizeGrant.php +++ b/src/Grant/AbstractAuthorizeGrant.php @@ -14,6 +14,7 @@ namespace League\OAuth2\Server\Grant; +use League\OAuth2\Server\Entities\ClientEntityInterface; use League\OAuth2\Server\RequestTypes\AuthorizationRequest; use League\OAuth2\Server\RequestTypes\AuthorizationRequestInterface; @@ -35,4 +36,14 @@ protected function createAuthorizationRequest(): AuthorizationRequestInterface { return new AuthorizationRequest(); } + + /** + * Get the client redirect URI. + */ + protected function getClientRedirectUri(ClientEntityInterface $client): string + { + return is_array($client->getRedirectUri()) + ? $client->getRedirectUri()[0] + : $client->getRedirectUri(); + } } diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 3b4467a5f..22b00d02a 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -280,17 +280,16 @@ public function validateAuthorizationRequest(ServerRequestInterface $request): A throw OAuthServerException::invalidClient($request); } - $defaultClientRedirectUri = is_array($client->getRedirectUri()) - ? $client->getRedirectUri()[0] - : $client->getRedirectUri(); + $stateParameter = $this->getQueryStringParameter('state', $request); $scopes = $this->validateScopes( $this->getQueryStringParameter('scope', $request, $this->defaultScope), - $redirectUri ?? $defaultClientRedirectUri + $this->makeRedirectUri( + $redirectUri ?? $this->getClientRedirectUri($client), + $stateParameter !== null ? ['state' => $stateParameter] : [] + ) ); - $stateParameter = $this->getQueryStringParameter('state', $request); - $authorizationRequest = $this->createAuthorizationRequest(); $authorizationRequest->setGrantTypeId($this->getIdentifier()); $authorizationRequest->setClient($client); @@ -354,7 +353,7 @@ public function completeAuthorizationRequest(AuthorizationRequestInterface $auth } $finalRedirectUri = $authorizationRequest->getRedirectUri() - ?? $this->getClientRedirectUri($authorizationRequest); + ?? $this->getClientRedirectUri($authorizationRequest->getClient()); // The user approved the client, redirect them back with an auth code if ($authorizationRequest->isAuthorizationApproved() === true) { @@ -408,14 +407,4 @@ public function completeAuthorizationRequest(AuthorizationRequestInterface $auth ) ); } - - /** - * Get the client redirect URI if not set in the request. - */ - private function getClientRedirectUri(AuthorizationRequestInterface $authorizationRequest): string - { - return is_array($authorizationRequest->getClient()->getRedirectUri()) - ? $authorizationRequest->getClient()->getRedirectUri()[0] - : $authorizationRequest->getClient()->getRedirectUri(); - } } diff --git a/src/Grant/ImplicitGrant.php b/src/Grant/ImplicitGrant.php index 7723ab8b1..81252dea1 100644 --- a/src/Grant/ImplicitGrant.php +++ b/src/Grant/ImplicitGrant.php @@ -111,24 +111,24 @@ public function validateAuthorizationRequest(ServerRequestInterface $request): A if ($redirectUri !== null) { $this->validateRedirectUri($redirectUri, $client, $request); } elseif ( - is_array($client->getRedirectUri()) && count($client->getRedirectUri()) !== 1 - || $client->getRedirectUri() === '' + $client->getRedirectUri() === '' || + (is_array($client->getRedirectUri()) && count($client->getRedirectUri()) !== 1) ) { $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); throw OAuthServerException::invalidClient($request); - } else { - $redirectUri = is_array($client->getRedirectUri()) - ? $client->getRedirectUri()[0] - : $client->getRedirectUri(); } + $stateParameter = $this->getQueryStringParameter('state', $request); + $scopes = $this->validateScopes( $this->getQueryStringParameter('scope', $request, $this->defaultScope), - $redirectUri + $this->makeRedirectUri( + $redirectUri ?? $this->getClientRedirectUri($client), + $stateParameter !== null ? ['state' => $stateParameter] : [], + $this->queryDelimiter + ) ); - $stateParameter = $this->getQueryStringParameter('state', $request); - $authorizationRequest = $this->createAuthorizationRequest(); $authorizationRequest->setGrantTypeId($this->getIdentifier()); $authorizationRequest->setClient($client); @@ -152,11 +152,8 @@ public function completeAuthorizationRequest(AuthorizationRequestInterface $auth throw new LogicException('An instance of UserEntityInterface should be set on the AuthorizationRequest'); } - $clientRegisteredRedirectUri = is_array($authorizationRequest->getClient()->getRedirectUri()) - ? $authorizationRequest->getClient()->getRedirectUri()[0] - : $authorizationRequest->getClient()->getRedirectUri(); - - $finalRedirectUri = $authorizationRequest->getRedirectUri() ?? $clientRegisteredRedirectUri; + $finalRedirectUri = $authorizationRequest->getRedirectUri() + ?? $this->getClientRedirectUri($authorizationRequest->getClient()); // The user approved the client, redirect them back with an access token if ($authorizationRequest->isAuthorizationApproved() === true) { @@ -199,7 +196,8 @@ public function completeAuthorizationRequest(AuthorizationRequestInterface $auth $finalRedirectUri, [ 'state' => $authorizationRequest->getState(), - ] + ], + $this->queryDelimiter ) ); } diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php index fc6ac07c3..646056618 100644 --- a/tests/Grant/AuthCodeGrantTest.php +++ b/tests/Grant/AuthCodeGrantTest.php @@ -469,6 +469,49 @@ public function testValidateAuthorizationRequestInvalidCodeChallengeMethod(): vo $grant->validateAuthorizationRequest($request); } + public function testValidateAuthorizationRequestInvalidScopes(): void + { + $client = new ClientEntity(); + $client->setRedirectUri(self::REDIRECT_URI); + $client->setConfidential(); + + $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); + $clientRepositoryMock->method('getClientEntity')->willReturn($client); + + $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); + $scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn(null); + + $grant = new AuthCodeGrant( + $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(), + $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(), + new DateInterval('PT10M') + ); + + $grant->setClientRepository($clientRepositoryMock); + $grant->setScopeRepository($scopeRepositoryMock); + $grant->setDefaultScope(self::DEFAULT_SCOPE); + + $request = (new ServerRequest())->withQueryParams([ + 'response_type' => 'code', + 'client_id' => 'foo', + 'redirect_uri' => self::REDIRECT_URI, + 'scope' => 'foo', + 'state' => 'foo', + ]); + + try { + $grant->validateAuthorizationRequest($request); + } catch (OAuthServerException $e) { + self::assertSame(5, $e->getCode()); + self::assertSame('invalid_scope', $e->getErrorType()); + self::assertSame('https://foo/bar?state=foo', $e->getRedirectUri()); + + return; + } + + self::fail('The expected exception was not thrown'); + } + public function testCompleteAuthorizationRequest(): void { $client = new ClientEntity(); @@ -529,6 +572,7 @@ public function testCompleteAuthorizationRequestDenied(): void $authRequest->setClient($client); $authRequest->setGrantTypeId('authorization_code'); $authRequest->setUser(new UserEntity()); + $authRequest->setState('foo'); $authCodeRepository = $this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(); $authCodeRepository->method('getNewAuthCode')->willReturn(new AuthCodeEntity()); @@ -540,10 +584,17 @@ public function testCompleteAuthorizationRequestDenied(): void ); $grant->setEncryptionKey($this->cryptStub->getKey()); - $this->expectException(OAuthServerException::class); - $this->expectExceptionCode(9); + try { + $grant->completeAuthorizationRequest($authRequest); + } catch (OAuthServerException $e) { + self::assertSame(9, $e->getCode()); + self::assertSame('access_denied', $e->getErrorType()); + self::assertSame('http://foo/bar?state=foo', $e->getRedirectUri()); + + return; + } - $grant->completeAuthorizationRequest($authRequest); + self::fail('The expected exception was not thrown'); } public function testRespondToAccessTokenRequest(): void diff --git a/tests/Grant/ImplicitGrantTest.php b/tests/Grant/ImplicitGrantTest.php index c2b943197..617aaa842 100644 --- a/tests/Grant/ImplicitGrantTest.php +++ b/tests/Grant/ImplicitGrantTest.php @@ -95,7 +95,7 @@ public function testValidateAuthorizationRequest(): void $grant->setDefaultScope(self::DEFAULT_SCOPE); $request = (new ServerRequest())->withQueryParams([ - 'response_type' => 'code', + 'response_type' => 'token', 'client_id' => 'foo', 'redirect_uri' => self::REDIRECT_URI, ]); @@ -120,7 +120,7 @@ public function testValidateAuthorizationRequestRedirectUriArray(): void $grant->setDefaultScope(self::DEFAULT_SCOPE); $request = (new ServerRequest())->withQueryParams([ - 'response_type' => 'code', + 'response_type' => 'token', 'client_id' => 'foo', 'redirect_uri' => self::REDIRECT_URI, ]); @@ -135,7 +135,7 @@ public function testValidateAuthorizationRequestMissingClientId(): void $grant = new ImplicitGrant(new DateInterval('PT10M')); $grant->setClientRepository($clientRepositoryMock); - $request = (new ServerRequest())->withQueryParams(['response_type' => 'code']); + $request = (new ServerRequest())->withQueryParams(['response_type' => 'token']); $this->expectException(OAuthServerException::class); $this->expectExceptionCode(3); @@ -152,7 +152,7 @@ public function testValidateAuthorizationRequestInvalidClientId(): void $grant->setClientRepository($clientRepositoryMock); $request = (new ServerRequest())->withQueryParams([ - 'response_type' => 'code', + 'response_type' => 'token', 'client_id' => 'foo', ]); @@ -173,7 +173,7 @@ public function testValidateAuthorizationRequestBadRedirectUriString(): void $grant->setClientRepository($clientRepositoryMock); $request = (new ServerRequest())->withQueryParams([ - 'response_type' => 'code', + 'response_type' => 'token', 'client_id' => 'foo', 'redirect_uri' => 'http://bar', ]); @@ -195,7 +195,7 @@ public function testValidateAuthorizationRequestBadRedirectUriArray(): void $grant->setClientRepository($clientRepositoryMock); $request = (new ServerRequest())->withQueryParams([ - 'response_type' => 'code', + 'response_type' => 'token', 'client_id' => 'foo', 'redirect_uri' => 'http://bar', ]); @@ -206,6 +206,44 @@ public function testValidateAuthorizationRequestBadRedirectUriArray(): void $grant->validateAuthorizationRequest($request); } + public function testValidateAuthorizationRequestInvalidScopes(): void + { + $client = new ClientEntity(); + $client->setRedirectUri(self::REDIRECT_URI); + + $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock(); + $clientRepositoryMock->method('getClientEntity')->willReturn($client); + + $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); + $scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn(null); + + $grant = new ImplicitGrant(new DateInterval('PT10M')); + + $grant->setClientRepository($clientRepositoryMock); + $grant->setScopeRepository($scopeRepositoryMock); + $grant->setDefaultScope(self::DEFAULT_SCOPE); + + $request = (new ServerRequest())->withQueryParams([ + 'response_type' => 'token', + 'client_id' => 'foo', + 'redirect_uri' => self::REDIRECT_URI, + 'scope' => 'foo', + 'state' => 'foo', + ]); + + try { + $grant->validateAuthorizationRequest($request); + } catch (OAuthServerException $e) { + self::assertSame(5, $e->getCode()); + self::assertSame('invalid_scope', $e->getErrorType()); + self::assertSame('https://foo/bar#state=foo', $e->getRedirectUri()); + + return; + } + + self::fail('Did not throw expected exception'); + } + public function testCompleteAuthorizationRequest(): void { $client = new ClientEntity(); @@ -248,6 +286,7 @@ public function testCompleteAuthorizationRequestDenied(): void $authRequest->setClient($client); $authRequest->setGrantTypeId('authorization_code'); $authRequest->setUser(new UserEntity()); + $authRequest->setState('foo'); $accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock(); $accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity()); @@ -261,10 +300,17 @@ public function testCompleteAuthorizationRequestDenied(): void $grant->setAccessTokenRepository($accessTokenRepositoryMock); $grant->setScopeRepository($scopeRepositoryMock); - $this->expectException(OAuthServerException::class); - $this->expectExceptionCode(9); + try { + $grant->completeAuthorizationRequest($authRequest); + } catch (OAuthServerException $e) { + self::assertSame(9, $e->getCode()); + self::assertSame('access_denied', $e->getErrorType()); + self::assertSame('https://foo/bar#state=foo', $e->getRedirectUri()); - $grant->completeAuthorizationRequest($authRequest); + return; + } + + self::fail('Did not throw expected exception'); } public function testAccessTokenRepositoryUniqueConstraintCheck(): void