Merge pull request #1410 from hafezdivandari/master-fix-some-bugs

Fix including interval in response of device authorization request
thephpleague · Dec 20, 2024 · 34a14d3 · 34a14d3
2 parents 902d5f2 + 03dcdd7
commit 34a14d3
Show file tree

Hide file tree

Showing 4 changed files with 71 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - 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)
+- Fixed bug on setting interval visibility of device authorization grant (PR #1410)
+- Fix a bug where the new poll date were not persisted when `slow_down` error happens, because the exception is thrown before calling `persistDeviceCode`. (PR #1410)
+- Fix a bug where `slow_down` error response may have been returned even after the user has completed the auth flow (already approved / denied the request). (PR #1410)
 
 ## [9.0.1] - released 2024-10-14
 ### Fixed

diff --git a/src/Grant/DeviceCodeGrant.php b/src/Grant/DeviceCodeGrant.php
@@ -51,7 +51,7 @@ public function __construct(
         RefreshTokenRepositoryInterface $refreshTokenRepository,
         private DateInterval $deviceCodeTTL,
         string $verificationUri,
-        private int $retryInterval = 5
+        private readonly int $retryInterval = 5
     ) {
         $this->setDeviceCodeRepository($deviceCodeRepository);
         $this->setRefreshTokenRepository($refreshTokenRepository);
@@ -101,6 +101,10 @@ public function respondToDeviceAuthorizationRequest(ServerRequestInterface $requ
             $response->includeVerificationUriComplete();
         }
 
+        if ($this->intervalVisibility === true) {
+            $response->includeInterval();
+        }
+
         $response->setDeviceCodeEntity($deviceCodeEntity);
 
         return $response;
@@ -139,11 +143,17 @@ public function respondToAccessTokenRequest(
         $client = $this->validateClient($request);
         $deviceCodeEntity = $this->validateDeviceCode($request, $client);
 
-        $deviceCodeEntity->setLastPolledAt(new DateTimeImmutable());
-        $this->deviceCodeRepository->persistDeviceCode($deviceCodeEntity);
-
-        // If device code has no user associated, respond with pending
+        // If device code has no user associated, respond with pending or slow down
         if (is_null($deviceCodeEntity->getUserIdentifier())) {
+            $shouldSlowDown = $this->deviceCodePolledTooSoon($deviceCodeEntity->getLastPolledAt());
+
+            $deviceCodeEntity->setLastPolledAt(new DateTimeImmutable());
+            $this->deviceCodeRepository->persistDeviceCode($deviceCodeEntity);
+
+            if ($shouldSlowDown) {
+                throw OAuthServerException::slowDown();
+            }
+
             throw OAuthServerException::authorizationPending();
         }
 
@@ -205,10 +215,6 @@ protected function validateDeviceCode(ServerRequestInterface $request, ClientEnt
             throw OAuthServerException::invalidRequest('device_code', 'Device code was not issued to this client');
         }
 
-        if ($this->deviceCodePolledTooSoon($deviceCodeEntity->getLastPolledAt()) === true) {
-            throw OAuthServerException::slowDown();
-        }
-
         return $deviceCodeEntity;
     }
 
@@ -258,10 +264,7 @@ protected function issueDeviceCode(
         $deviceCode->setExpiryDateTime((new DateTimeImmutable())->add($deviceCodeTTL));
         $deviceCode->setClient($client);
         $deviceCode->setVerificationUri($verificationUri);
-
-        if ($this->getIntervalVisibility() === true) {
-            $deviceCode->setInterval($this->retryInterval);
-        }
+        $deviceCode->setInterval($this->retryInterval);
 
         foreach ($scopes as $scope) {
             $deviceCode->addScope($scope);

diff --git a/src/ResponseTypes/DeviceCodeResponse.php b/src/ResponseTypes/DeviceCodeResponse.php
@@ -25,7 +25,7 @@ class DeviceCodeResponse extends AbstractResponseType
 {
     protected DeviceCodeEntityInterface $deviceCodeEntity;
     private bool $includeVerificationUriComplete = false;
-    private const DEFAULT_INTERVAL = 5;
+    private bool $includeInterval = false;
 
     /**
      * {@inheritdoc}
@@ -45,7 +45,7 @@ public function generateHttpResponse(ResponseInterface $response): ResponseInter
             $responseParams['verification_uri_complete'] = $this->deviceCodeEntity->getVerificationUriComplete();
         }
 
-        if ($this->deviceCodeEntity->getInterval() !== self::DEFAULT_INTERVAL) {
+        if ($this->includeInterval === true) {
             $responseParams['interval'] = $this->deviceCodeEntity->getInterval();
         }
 
@@ -79,6 +79,11 @@ public function includeVerificationUriComplete(): void
         $this->includeVerificationUriComplete = true;
     }
 
+    public function includeInterval(): void
+    {
+        $this->includeInterval = true;
+    }
+
     /**
      * Add custom fields to your Bearer Token response here, then override
      * AuthorizationServer::getResponseType() to pull in your version of

diff --git a/tests/Grant/DeviceCodeGrantTest.php b/tests/Grant/DeviceCodeGrantTest.php
@@ -656,6 +656,51 @@ public function testSettingDeviceCodeIntervalRate(): void
         $this::assertEquals(self::INTERVAL_RATE, $deviceCode->interval);
     }
 
+    public function testSettingInternalVisibility(): void
+    {
+        $client = new ClientEntity();
+        $client->setIdentifier('foo');
+
+        $clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
+        $clientRepositoryMock->method('getClientEntity')->willReturn($client);
+
+        $deviceCode = new DeviceCodeEntity();
+
+        $deviceCodeRepository = $this->getMockBuilder(DeviceCodeRepositoryInterface::class)->getMock();
+        $deviceCodeRepository->method('getNewDeviceCode')->willReturn($deviceCode);
+
+        $scope = new ScopeEntity();
+        $scope->setIdentifier('basic');
+        $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
+        $scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scope);
+
+        $grant = new DeviceCodeGrant(
+            $deviceCodeRepository,
+            $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock(),
+            new DateInterval('PT10M'),
+            'http://foo/bar',
+        );
+
+        $grant->setClientRepository($clientRepositoryMock);
+        $grant->setDefaultScope(self::DEFAULT_SCOPE);
+        $grant->setScopeRepository($scopeRepositoryMock);
+        $grant->setIntervalVisibility(true);
+
+        $request = (new ServerRequest())->withParsedBody([
+            'client_id' => 'foo',
+            'scope' => 'basic',
+        ]);
+
+        $deviceCodeResponse = $grant
+            ->respondToDeviceAuthorizationRequest($request)
+            ->generateHttpResponse(new Response());
+
+        $deviceCode = json_decode((string) $deviceCodeResponse->getBody());
+
+        $this::assertObjectHasProperty('interval', $deviceCode);
+        $this::assertEquals(5, $deviceCode->interval);
+    }
+
     public function testIssueAccessDeniedError(): void
     {
         $client = new ClientEntity();