Merge branch 'master' into fix-revoke-refresh-token

thephpleague · Oct 12, 2024 · c25b439 · c25b439
2 parents 844d86d + 2c698e2
commit c25b439
Show file tree

Hide file tree

Showing 6 changed files with 143 additions and 47 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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