From f8f15ef7f8fbc2b07bbdf75a209eae64e2696c8d Mon Sep 17 00:00:00 2001 From: Andrew Millington Date: Thu, 21 Mar 2024 12:02:38 +0000 Subject: [PATCH] Simplify device code response - remove payload --- composer.json | 3 +- src/Grant/DeviceCodeGrant.php | 81 +++++-------------- .../DeviceCodeRepositoryInterface.php | 10 +++ src/ResponseTypes/DeviceCodeResponse.php | 26 +++--- tests/Grant/DeviceCodeGrantTest.php | 30 ++++--- .../DeviceCodeResponseTypeTest.php | 7 +- 6 files changed, 65 insertions(+), 92 deletions(-) diff --git a/composer.json b/composer.json index 9376506a2..a3a53b00c 100644 --- a/composer.json +++ b/composer.json @@ -13,7 +13,8 @@ "defuse/php-encryption": "^2.4", "ext-json": "*", "lcobucci/clock": "^2.3 || ^3.0", - "psr/http-server-middleware": "^1.0" + "psr/http-server-middleware": "^1.0", + "ramsey/uuid": "^4.7" }, "require-dev": { "phpunit/phpunit": "^9.6.15", diff --git a/src/Grant/DeviceCodeGrant.php b/src/Grant/DeviceCodeGrant.php index d32afd421..b48aa946b 100644 --- a/src/Grant/DeviceCodeGrant.php +++ b/src/Grant/DeviceCodeGrant.php @@ -28,15 +28,11 @@ use League\OAuth2\Server\RequestEvent; use League\OAuth2\Server\ResponseTypes\DeviceCodeResponse; use League\OAuth2\Server\ResponseTypes\ResponseTypeInterface; -use LogicException; use Psr\Http\Message\ServerRequestInterface; -use stdClass; +use Ramsey\Uuid\Uuid; use TypeError; use function is_null; -use function json_decode; -use function json_encode; -use function property_exists; use function random_int; use function strlen; use function time; @@ -93,35 +89,20 @@ public function respondToDeviceAuthorizationRequest(ServerRequestInterface $requ $scopes = $this->validateScopes($this->getRequestParameter('scope', $request, $this->defaultScope)); - $deviceCode = $this->issueDeviceCode( + $deviceCodeEntity = $this->issueDeviceCode( $this->deviceCodeTTL, $client, $this->verificationUri, $scopes ); - // TODO: Check payload generation. Is this the best way to handle things? - $payload = [ - 'device_code_id' => $deviceCode->getIdentifier(), - 'user_code' => $deviceCode->getUserCode(), - 'verification_uri' => $deviceCode->getVerificationUri(), - 'expire_time' => $deviceCode->getExpiryDateTime()->getTimestamp(), - 'client_id' => $deviceCode->getClient()->getIdentifier(), - 'scopes' => $deviceCode->getScopes(), - 'interval' => $deviceCode->getInterval(), - ]; - $response = new DeviceCodeResponse(); if ($this->includeVerificationUriComplete === true) { $response->includeVerificationUriComplete(); - $payload['verification_uri_complete'] = $deviceCode->getVerificationUriComplete(); } - $jsonPayload = json_encode($payload, JSON_THROW_ON_ERROR); - - $response->setDeviceCode($deviceCode); - $response->setPayload($this->encrypt($jsonPayload)); + $response->setDeviceCodeEntity($deviceCodeEntity); return $response; } @@ -198,45 +179,39 @@ public function respondToAccessTokenRequest( */ protected function validateDeviceCode(ServerRequestInterface $request, ClientEntityInterface $client): DeviceCodeEntityInterface { - $encryptedDeviceCode = $this->getRequestParameter('device_code', $request); + $deviceCode = $this->getRequestParameter('device_code', $request); - if (is_null($encryptedDeviceCode)) { + if (is_null($deviceCode)) { throw OAuthServerException::invalidRequest('device_code'); } - $deviceCodePayload = $this->decodeDeviceCode($encryptedDeviceCode); + $deviceCodeEntity = $this->deviceCodeRepository->getDeviceCodeEntityByDeviceCode( + $deviceCode + ); - if (!property_exists($deviceCodePayload, 'device_code_id')) { - throw OAuthServerException::invalidRequest('device_code', 'Device code malformed'); + if ($deviceCodeEntity instanceof DeviceCodeEntityInterface === false) { + $this->getEmitter()->emit(new RequestEvent(RequestEvent::USER_AUTHENTICATION_FAILED, $request)); + + throw OAuthServerException::invalidGrant(); } - if (time() > $deviceCodePayload->expire_time) { + if (time() > $deviceCodeEntity->getExpiryDateTime()->getTimestamp()) { throw OAuthServerException::expiredToken('device_code'); } - if ($this->deviceCodeRepository->isDeviceCodeRevoked($deviceCodePayload->device_code_id) === true) { + if ($this->deviceCodeRepository->isDeviceCodeRevoked($deviceCode) === true) { throw OAuthServerException::invalidRequest('device_code', 'Device code has been revoked'); } - if ($deviceCodePayload->client_id !== $client->getIdentifier()) { + if ($deviceCodeEntity->getClient()->getIdentifier() !== $client->getIdentifier()) { throw OAuthServerException::invalidRequest('device_code', 'Device code was not issued to this client'); } - $deviceCode = $this->deviceCodeRepository->getDeviceCodeEntityByDeviceCode( - $deviceCodePayload->device_code_id - ); - - if ($deviceCode instanceof DeviceCodeEntityInterface === false) { - $this->getEmitter()->emit(new RequestEvent(RequestEvent::USER_AUTHENTICATION_FAILED, $request)); - - throw OAuthServerException::invalidGrant(); - } - - if ($this->deviceCodePolledTooSoon($deviceCode->getLastPolledAt()) === true) { + if ($this->deviceCodePolledTooSoon($deviceCodeEntity->getLastPolledAt()) === true) { throw OAuthServerException::slowDown(); } - return $deviceCode; + return $deviceCodeEntity; } private function deviceCodePolledTooSoon(?DateTimeImmutable $lastPoll): bool @@ -244,18 +219,6 @@ private function deviceCodePolledTooSoon(?DateTimeImmutable $lastPoll): bool return $lastPoll !== null && $lastPoll->getTimestamp() + $this->retryInterval > time(); } - /** - * @throws OAuthServerException - */ - protected function decodeDeviceCode(string $encryptedDeviceCode): stdClass - { - try { - return json_decode($this->decrypt($encryptedDeviceCode)); - } catch (LogicException $e) { - throw OAuthServerException::invalidRequest('device_code', 'Cannot decrypt the device code', $e); - } - } - /** * Set the verification uri */ @@ -309,7 +272,8 @@ protected function issueDeviceCode( while ($maxGenerationAttempts-- > 0) { $deviceCode->setIdentifier($this->generateUniqueIdentifier()); - $deviceCode->setUserCode($this->generateUniqueUserCode()); + $deviceCode->setUserCode($this->generateUserCode()); + try { $this->deviceCodeRepository->persistDeviceCode($deviceCode); @@ -321,19 +285,16 @@ protected function issueDeviceCode( } } - // This should never be hit. It is here to work around a PHPStan false error return $deviceCode; } /** - * Generate a new unique user code. - * - * + * Generate a new user code. * * @throws OAuthServerException */ - protected function generateUniqueUserCode(int $length = 8): string + protected function generateUserCode(int $length = 8): string { try { $userCode = ''; diff --git a/src/Repositories/DeviceCodeRepositoryInterface.php b/src/Repositories/DeviceCodeRepositoryInterface.php index 46e9492ff..634cf4992 100644 --- a/src/Repositories/DeviceCodeRepositoryInterface.php +++ b/src/Repositories/DeviceCodeRepositoryInterface.php @@ -51,4 +51,14 @@ public function revokeDeviceCode(string $codeId): void; * @return bool Return true if this code has been revoked */ public function isDeviceCodeRevoked(string $codeId): bool; + + /** + * Get the expiry time of the device code. + */ + public function getDeviceCodeExpiryTime(string $codeId): int; + + /** + * Get the client ID of the device code. + */ + public function getDeviceCodeClientId(string $codeId): string; } diff --git a/src/ResponseTypes/DeviceCodeResponse.php b/src/ResponseTypes/DeviceCodeResponse.php index 1b0268b8d..339c75bd3 100644 --- a/src/ResponseTypes/DeviceCodeResponse.php +++ b/src/ResponseTypes/DeviceCodeResponse.php @@ -23,8 +23,7 @@ class DeviceCodeResponse extends AbstractResponseType { - protected DeviceCodeEntityInterface $deviceCode; - protected string $payload; + protected DeviceCodeEntityInterface $deviceCodeEntity; private bool $includeVerificationUriComplete = false; private const DEFAULT_INTERVAL = 5; @@ -33,21 +32,21 @@ class DeviceCodeResponse extends AbstractResponseType */ public function generateHttpResponse(ResponseInterface $response): ResponseInterface { - $expireDateTime = $this->deviceCode->getExpiryDateTime()->getTimestamp(); + $expireDateTime = $this->deviceCodeEntity->getExpiryDateTime()->getTimestamp(); $responseParams = [ - 'device_code' => $this->payload, - 'user_code' => $this->deviceCode->getUserCode(), - 'verification_uri' => $this->deviceCode->getVerificationUri(), + 'device_code' => $this->deviceCodeEntity->getIdentifier(), + 'user_code' => $this->deviceCodeEntity->getUserCode(), + 'verification_uri' => $this->deviceCodeEntity->getVerificationUri(), 'expires_in' => $expireDateTime - time(), ]; if ($this->includeVerificationUriComplete === true) { - $responseParams['verification_uri_complete'] = $this->deviceCode->getVerificationUriComplete(); + $responseParams['verification_uri_complete'] = $this->deviceCodeEntity->getVerificationUriComplete(); } - if ($this->deviceCode->getInterval() !== self::DEFAULT_INTERVAL) { - $responseParams['interval'] = $this->deviceCode->getInterval(); + if ($this->deviceCodeEntity->getInterval() !== self::DEFAULT_INTERVAL) { + $responseParams['interval'] = $this->deviceCodeEntity->getInterval(); } $responseParams = json_encode($responseParams); @@ -67,17 +66,12 @@ public function generateHttpResponse(ResponseInterface $response): ResponseInter return $response; } - public function setPayload(string $payload): void - { - $this->payload = $payload; - } - /** * {@inheritdoc} */ - public function setDeviceCode(DeviceCodeEntityInterface $deviceCode): void + public function setDeviceCodeEntity(DeviceCodeEntityInterface $deviceCodeEntity): void { - $this->deviceCode = $deviceCode; + $this->deviceCodeEntity = $deviceCodeEntity; } public function includeVerificationUriComplete(): void diff --git a/tests/Grant/DeviceCodeGrantTest.php b/tests/Grant/DeviceCodeGrantTest.php index 66d7d3024..4383469a7 100644 --- a/tests/Grant/DeviceCodeGrantTest.php +++ b/tests/Grant/DeviceCodeGrantTest.php @@ -355,13 +355,15 @@ public function testRespondToAccessTokenRequest(): void $refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity()); $deviceCodeRepositoryMock = $this->getMockBuilder(DeviceCodeRepositoryInterface::class)->getMock(); - $deviceCode = new DeviceCodeEntity(); + $deviceCodeEntity = new DeviceCodeEntity(); - $deviceCode->setUserIdentifier('baz'); - $deviceCode->setIdentifier('deviceCodeEntityIdentifier'); - $deviceCode->setUserCode('123456'); + $deviceCodeEntity->setUserIdentifier('baz'); + $deviceCodeEntity->setIdentifier('deviceCodeEntityIdentifier'); + $deviceCodeEntity->setUserCode('123456'); + $deviceCodeEntity->setExpiryDateTime(new DateTimeImmutable('+1 hour')); + $deviceCodeEntity->setClient($client); - $deviceCodeRepositoryMock->method('getDeviceCodeEntityByDeviceCode')->willReturn($deviceCode); + $deviceCodeRepositoryMock->method('getDeviceCodeEntityByDeviceCode')->willReturn($deviceCodeEntity); $scope = new ScopeEntity(); $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); @@ -382,7 +384,7 @@ public function testRespondToAccessTokenRequest(): void $grant->setEncryptionKey($this->cryptStub->getKey()); $grant->setPrivateKey(new CryptKey('file://' . __DIR__ . '/../Stubs/private.key')); - $grant->completeDeviceAuthorizationRequest($deviceCode->getUserCode(), "1", true); + $grant->completeDeviceAuthorizationRequest($deviceCodeEntity->getUserCode(), "1", true); $serverRequest = (new ServerRequest())->withParsedBody([ 'grant_type' => 'urn:ietf:params:oauth:grant-type:device_code', @@ -511,6 +513,8 @@ public function testIssueSlowDownError(): void $deviceCodeRepositoryMock = $this->getMockBuilder(DeviceCodeRepositoryInterface::class)->getMock(); $deviceCodeEntity = new DeviceCodeEntity(); $deviceCodeEntity->setLastPolledAt(new DateTimeImmutable()); + $deviceCodeEntity->setExpiryDateTime(new DateTimeImmutable('+1 hour')); + $deviceCodeEntity->setClient($client); $deviceCodeRepositoryMock->method('getDeviceCodeEntityByDeviceCode')->willReturn($deviceCodeEntity); $scope = new ScopeEntity(); @@ -569,8 +573,10 @@ public function testIssueAuthorizationPendingError(): void $refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity()); $deviceCodeRepositoryMock = $this->getMockBuilder(DeviceCodeRepositoryInterface::class)->getMock(); - $deviceCode = new DeviceCodeEntity(); - $deviceCodeRepositoryMock->method('getDeviceCodeEntityByDeviceCode')->willReturn($deviceCode); + $deviceCodeEntity = new DeviceCodeEntity(); + $deviceCodeEntity->setExpiryDateTime(new DateTimeImmutable('+1 hour')); + $deviceCodeEntity->setClient($client); + $deviceCodeRepositoryMock->method('getDeviceCodeEntityByDeviceCode')->willReturn($deviceCodeEntity); $scope = new ScopeEntity(); $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); @@ -628,8 +634,10 @@ public function testIssueExpiredTokenError(): void $refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity()); $deviceCodeRepositoryMock = $this->getMockBuilder(DeviceCodeRepositoryInterface::class)->getMock(); - $deviceCode = new DeviceCodeEntity(); - $deviceCodeRepositoryMock->method('getDeviceCodeEntityByDeviceCode')->willReturn($deviceCode); + $deviceCodeEntity = new DeviceCodeEntity(); + $deviceCodeEntity->setExpiryDateTime(new DateTimeImmutable('-1 hour')); + $deviceCodeEntity->setClient($client); + $deviceCodeRepositoryMock->method('getDeviceCodeEntityByDeviceCode')->willReturn($deviceCodeEntity); $scope = new ScopeEntity(); $scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock(); @@ -736,6 +744,8 @@ public function testIssueAccessDeniedError(): void $deviceCode = new DeviceCodeEntity(); + $deviceCode->setExpiryDateTime(new DateTimeImmutable('+1 hour')); + $deviceCode->setClient($client); $deviceCode->setUserCode('12345678'); $deviceCodeRepositoryMock->method('getDeviceCodeEntityByDeviceCode')->willReturn($deviceCode); diff --git a/tests/ResponseTypes/DeviceCodeResponseTypeTest.php b/tests/ResponseTypes/DeviceCodeResponseTypeTest.php index 3d6f960fe..93bd9d6b3 100644 --- a/tests/ResponseTypes/DeviceCodeResponseTypeTest.php +++ b/tests/ResponseTypes/DeviceCodeResponseTypeTest.php @@ -13,7 +13,6 @@ use LeagueTests\Stubs\DeviceCodeEntity; use LeagueTests\Stubs\ScopeEntity; use PHPUnit\Framework\TestCase; -use Psr\Http\Message\ResponseInterface; use function base64_encode; use function json_decode; @@ -41,9 +40,7 @@ public function testGenerateHttpResponse(): void $deviceCode->addScope($scope); $deviceCode->setVerificationUri('https://example.com/device'); - - $responseType->setDeviceCode($deviceCode); - $responseType->setPayload('test'); + $responseType->setDeviceCodeEntity($deviceCode); $response = $responseType->generateHttpResponse(new Response()); @@ -56,7 +53,7 @@ public function testGenerateHttpResponse(): void $json = json_decode($response->getBody()->getContents()); $this::assertObjectHasProperty('expires_in', $json); $this::assertObjectHasProperty('device_code', $json); - $this::assertEquals('test', $json->device_code); + $this::assertEquals('abcdef', $json->device_code); $this::assertObjectHasProperty('verification_uri', $json); $this::assertObjectHasProperty('user_code', $json); }