Merge branch 'master' into always-validate-client

thephpleague · Nov 19, 2024 · 44bb1bd · 44bb1bd
2 parents cd2f0bd + f843573
commit 44bb1bd
Show file tree

Hide file tree

Showing 16 changed files with 221 additions and 156 deletions.
diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml
@@ -12,7 +12,7 @@ jobs:
 
     strategy:
       matrix:
-        php-version: [8.1, 8.2, 8.3]
+        php-version: [8.1, 8.2, 8.3, 8.4]
         composer-stability: [prefer-lowest, prefer-stable]
         operating-system:
           - ubuntu-latest

diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -11,7 +11,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        php: [8.1, 8.2, 8.3]
+        php: [8.1, 8.2, 8.3, 8.4]
         os: [ubuntu-22.04]
         stability: [prefer-lowest, prefer-stable]
 

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,13 @@ 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]
+### Added
+- Support for PHP 8.4 (PR #1454)
+
+### 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)
+- 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

diff --git a/composer.json b/composer.json
@@ -4,7 +4,7 @@
     "homepage": "https://oauth2.thephpleague.com/",
     "license": "MIT",
     "require": {
-        "php": "~8.1.0 || ~8.2.0 || ~8.3.0",
+        "php": "~8.1.0 || ~8.2.0 || ~8.3.0 || ~8.4.0",
         "ext-openssl": "*",
         "league/event": "^3.0",
         "league/uri": "^7.0",
@@ -16,9 +16,9 @@
         "psr/http-server-middleware": "^1.0"
     },
     "require-dev": {
-        "phpunit/phpunit": "^9.6.15",
-        "laminas/laminas-diactoros": "^3.3.0",
-        "phpstan/phpstan": "^1.10.55",
+        "phpunit/phpunit": "^9.6.21",
+        "laminas/laminas-diactoros": "^3.5",
+        "phpstan/phpstan": "^1.12",
         "phpstan/phpstan-phpunit": "^1.3.15",
         "roave/security-advisories": "dev-master",
         "phpstan/extension-installer": "^1.3.1",

diff --git a/examples/src/Repositories/DeviceCodeRepository.php b/examples/src/Repositories/DeviceCodeRepository.php
@@ -17,6 +17,7 @@
 use League\OAuth2\Server\Repositories\DeviceCodeRepositoryInterface;
 use OAuth2ServerExamples\Entities\ClientEntity;
 use OAuth2ServerExamples\Entities\DeviceCodeEntity;
+use OAuth2ServerExamples\Entities\ScopeEntity;
 
 class DeviceCodeRepository implements DeviceCodeRepositoryInterface
 {
@@ -49,6 +50,14 @@ public function getDeviceCodeEntityByDeviceCode($deviceCode): ?DeviceCodeEntityI
         $deviceCodeEntity->setIdentifier($deviceCode);
         $deviceCodeEntity->setExpiryDateTime(new DateTimeImmutable('now +1 hour'));
         $deviceCodeEntity->setClient($clientEntity);
+        $deviceCodeEntity->setLastPolledAt(new DateTimeImmutable());
+
+        $scopes = [];
+        foreach ($scopes as $scope) {
+            $scopeEntity = new ScopeEntity();
+            $scopeEntity->setIdentifier($scope);
+            $deviceCodeEntity->addScope($scopeEntity);
+        }
 
         // The user identifier should be set when the user authenticates on the
         // OAuth server, along with whether they approved the request

diff --git a/phpcs.xml.dist b/phpcs.xml.dist
@@ -33,4 +33,5 @@
     <rule ref="SlevomatCodingStandard.TypeHints.PropertyTypeHint" />
     <rule ref="SlevomatCodingStandard.Commenting.EmptyComment" />
     <rule ref="SlevomatCodingStandard.Classes.RequireConstructorPropertyPromotion" />
+    <rule ref="SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue" />
 </ruleset>
diff --git a/src/CryptTrait.php b/src/CryptTrait.php
@@ -15,8 +15,11 @@
 namespace League\OAuth2\Server;
 
 use Defuse\Crypto\Crypto;
+use Defuse\Crypto\Exception\EnvironmentIsBrokenException;
+use Defuse\Crypto\Exception\WrongKeyOrModifiedCiphertextException;
 use Defuse\Crypto\Key;
 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/Exception/OAuthServerException.php b/src/Exception/OAuthServerException.php
@@ -33,7 +33,7 @@ class OAuthServerException extends Exception
     /**
      * Throw a new exception.
      */
-    final public function __construct(string $message, int $code, private string $errorType, private int $httpStatusCode = 400, private ?string $hint = null, private ?string $redirectUri = null, Throwable $previous = null)
+    final public function __construct(string $message, int $code, private string $errorType, private int $httpStatusCode = 400, private ?string $hint = null, private ?string $redirectUri = null, ?Throwable $previous = null)
     {
         parent::__construct($message, $code, $previous);
         $this->payload = [
@@ -88,7 +88,7 @@ public static function unsupportedGrantType(): static
     /**
      * Invalid request error.
      */
-    public static function invalidRequest(string $parameter, ?string $hint = null, Throwable $previous = null): static
+    public static function invalidRequest(string $parameter, ?string $hint = null, ?Throwable $previous = null): static
     {
         $errorMessage = 'The request is missing a required parameter, includes an invalid parameter value, ' .
             'includes a parameter more than once, or is otherwise malformed.';
@@ -141,7 +141,7 @@ public static function invalidCredentials(): static
      *
      * @codeCoverageIgnore
      */
-    public static function serverError(string $hint, Throwable $previous = null): static
+    public static function serverError(string $hint, ?Throwable $previous = null): static
     {
         return new static(
             'The authorization server encountered an unexpected condition which prevented it from fulfilling'
@@ -158,15 +158,15 @@ public static function serverError(string $hint, Throwable $previous = null): st
     /**
      * Invalid refresh token.
      */
-    public static function invalidRefreshToken(?string $hint = null, Throwable $previous = null): static
+    public static function invalidRefreshToken(?string $hint = null, ?Throwable $previous = null): static
     {
         return new static('The refresh token is invalid.', 8, 'invalid_grant', 400, $hint, null, $previous);
     }
 
     /**
      * Access denied.
      */
-    public static function accessDenied(?string $hint = null, ?string $redirectUri = null, Throwable $previous = null): static
+    public static function accessDenied(?string $hint = null, ?string $redirectUri = null, ?Throwable $previous = null): static
     {
         return new static(
             'The resource owner or authorization server denied the request.',
@@ -207,15 +207,15 @@ public function getErrorType(): string
      *
      * @return static
      */
-    public static function expiredToken(?string $hint = null, Throwable $previous = null): static
+    public static function expiredToken(?string $hint = null, ?Throwable $previous = null): static
     {
         $errorMessage = 'The `device_code` has expired and the device ' .
                         'authorization session has concluded.';
 
         return new static($errorMessage, 11, 'expired_token', 400, $hint, null, $previous);
     }
 
-    public static function authorizationPending(string $hint = '', Throwable $previous = null): static
+    public static function authorizationPending(string $hint = '', ?Throwable $previous = null): static
     {
         return new static(
             'The authorization request is still pending as the end user ' .
@@ -236,7 +236,7 @@ public static function authorizationPending(string $hint = '', Throwable $previo
      *
      * @return static
      */
-    public static function slowDown(string $hint = '', Throwable $previous = null): static
+    public static function slowDown(string $hint = '', ?Throwable $previous = null): static
     {
         return new static(
             'The authorization request is still pending and polling should ' .

diff --git a/src/Grant/AbstractGrant.php b/src/Grant/AbstractGrant.php
@@ -247,7 +247,7 @@ protected function validateRedirectUri(
      *
      * @return ScopeEntityInterface[]
      */
-    public function validateScopes(string|array|null $scopes, string $redirectUri = null): array
+    public function validateScopes(string|array|null $scopes, ?string $redirectUri = null): array
     {
         if ($scopes === null) {
             $scopes = [];

diff --git 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;
@@ -116,8 +117,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);
@@ -199,7 +202,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) {

diff --git a/src/Grant/DeviceCodeGrant.php b/src/Grant/DeviceCodeGrant.php
@@ -137,7 +137,6 @@ public function respondToAccessTokenRequest(
     ): ResponseTypeInterface {
         // Validate request
         $client = $this->validateClient($request);
-        $scopes = $this->validateScopes($this->getRequestParameter('scope', $request, $this->defaultScope));
         $deviceCodeEntity = $this->validateDeviceCode($request, $client);
 
         $deviceCodeEntity->setLastPolledAt(new DateTimeImmutable());
@@ -153,7 +152,7 @@ public function respondToAccessTokenRequest(
         }
 
         // Finalize the requested scopes
-        $finalizedScopes = $this->scopeRepository->finalizeScopes($scopes, $this->getIdentifier(), $client, $deviceCodeEntity->getUserIdentifier());
+        $finalizedScopes = $this->scopeRepository->finalizeScopes($deviceCodeEntity->getScopes(), $this->getIdentifier(), $client, $deviceCodeEntity->getUserIdentifier());
 
         // Issue and persist new access token
         $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $deviceCodeEntity->getUserIdentifier(), $finalizedScopes);

diff --git a/src/Grant/RefreshTokenGrant.php b/src/Grant/RefreshTokenGrant.php
@@ -78,7 +78,11 @@ public function respondToAccessTokenRequest(
         }
 
         // Issue and persist new access token
-        $accessToken = $this->issueAccessToken($accessTokenTTL, $client, $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);
 

diff --git a/src/ResourceServer.php b/src/ResourceServer.php
@@ -22,19 +22,15 @@ class ResourceServer
 {
     private CryptKeyInterface $publicKey;
 
-    private ?AuthorizationValidatorInterface $authorizationValidator = null;
-
     public function __construct(
         private AccessTokenRepositoryInterface $accessTokenRepository,
         CryptKeyInterface|string $publicKey,
-        AuthorizationValidatorInterface $authorizationValidator = null
+        private ?AuthorizationValidatorInterface $authorizationValidator = null
     ) {
         if ($publicKey instanceof CryptKeyInterface === false) {
             $publicKey = new CryptKey($publicKey);
         }
         $this->publicKey = $publicKey;
-
-        $this->authorizationValidator = $authorizationValidator;
     }
 
     protected function getAuthorizationValidator(): AuthorizationValidatorInterface

diff --git a/tests/Grant/AuthCodeGrantTest.php b/tests/Grant/AuthCodeGrantTest.php
@@ -1648,7 +1648,7 @@ public function testRespondToAccessTokenRequestClientMismatch(): void
         }
     }
 
-    public function testRespondToAccessTokenRequestBadCodeEncryption(): void
+    public function testRespondToAccessTokenRequestBadCode(): void
     {
         $client = new ClientEntity();
 
@@ -1691,15 +1691,71 @@ public function testRespondToAccessTokenRequestBadCodeEncryption(): void
                 'client_id'    => 'foo',
                 'client_secret' => 'bar',
                 'redirect_uri' => self::REDIRECT_URI,
-                'code'         => 'sdfsfsd',
+                'code'         => 'badCode',
             ]
         );
 
         try {
             /* @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');
         }
     }