From 119a0f807848b308f23b6a773263bfb2fa03a0b2 Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Sat, 14 Dec 2024 18:55:31 +0100 Subject: [PATCH] Validate `user_code` expiry during the Device Code grant. Add mock responses related to the Device Code grant. --- api/eventListeners/MockDataResponse.php | 62 +++++++++++++++++++ api/modules/oauth/models/OauthProcess.php | 4 +- .../functional/oauth/CompleteFlowCest.php | 19 ++++-- api/tests/functional/oauth/DeviceCodeCest.php | 13 ++++ api/tests/functional/oauth/ValidateCest.php | 26 ++++++++ .../OAuth2/Grants/DeviceCodeGrant.php | 8 ++- .../fixtures/data/oauth-device-codes.php | 22 ++++++- phpstan-baseline.neon | 5 -- 8 files changed, 143 insertions(+), 16 deletions(-) diff --git a/api/eventListeners/MockDataResponse.php b/api/eventListeners/MockDataResponse.php index edcaa57..d1218e1 100644 --- a/api/eventListeners/MockDataResponse.php +++ b/api/eventListeners/MockDataResponse.php @@ -9,6 +9,7 @@ use api\modules\accounts\actions\ChangeEmailAction; use api\modules\accounts\actions\EmailVerificationAction; use api\modules\accounts\actions\NewEmailVerificationAction; use api\modules\accounts\controllers\DefaultController; +use api\modules\oauth\controllers\AuthorizationController as OauthAuthorizationController; use Closure; use yii\base\ActionEvent; use yii\base\BootstrapInterface; @@ -37,11 +38,15 @@ final class MockDataResponse implements BootstrapInterface { $event->isValid = false; } + /** + * @return array|null + */ private function getResponse(ActionEvent $event): ?array { $action = $event->action; /** @var \yii\web\Controller $controller */ $controller = $action->controller; $request = $controller->request; + $response = $controller->response; if ($controller instanceof SignupController && $action->id === 'index') { $email = $request->post('email'); if ($email === 'let-me-register@ely.by') { @@ -91,6 +96,63 @@ final class MockDataResponse implements BootstrapInterface { } } + if ($controller instanceof OauthAuthorizationController) { + if ($action->id === 'validate') { + $userCode = $request->get('user_code'); + if ($userCode === 'E2E-APPROVED' || $userCode === 'E2E-UNAPPROVED') { + return [ + 'success' => true, + 'client' => [ + 'id' => 'test', + 'name' => 'Ely.by Test', + 'description' => "Some client's description", + ], + 'session' => [ + 'scopes' => 'account_info minecraft_server_session', + ], + ]; + } + + if ($userCode === 'E2E-EXPIRED') { + $response->setStatusCode(400); + return [ + 'success' => false, + 'error' => 'expired_token', + 'parameter' => 'user_code', + 'statusCode' => 400, + ]; + } + + if ($userCode === 'E2E-COMPLETED') { + $response->setStatusCode(400); + return [ + 'success' => false, + 'error' => 'used_user_code', + 'parameter' => 'user_code', + 'statusCode' => 400, + ]; + } + } + + if ($action->id === 'complete') { + $userCode = $request->get('user_code'); + $accept = $request->post('accept'); + if ($userCode === 'E2E-APPROVED' || ($userCode === 'E2E-UNAPPROVED' && $accept !== null)) { + return ['success' => true]; + } + + if ($userCode === 'E2E-UNAPPROVED' && $accept === null) { + $response->setStatusCode(401); + return [ + 'success' => false, + 'error' => 'accept_required', + 'parameter' => null, + 'statusCode' => 401, + ]; + } + } + } + if ($controller instanceof DefaultController && $action->id === 'get') { $httpAuth = $request->getHeaders()->get('authorization'); if ($httpAuth === 'Bearer dummy_token') { diff --git a/api/modules/oauth/models/OauthProcess.php b/api/modules/oauth/models/OauthProcess.php index 7d0f24e..a11daae 100644 --- a/api/modules/oauth/models/OauthProcess.php +++ b/api/modules/oauth/models/OauthProcess.php @@ -314,8 +314,8 @@ final readonly class OauthProcess { $parameter = $matches[1]; } - if ($parameter === null && str_starts_with($e->getErrorType(), 'invalid_')) { - $parameter = substr($e->getErrorType(), 8); // 8 is the length of the "invalid_" + if ($parameter === null && $hint === 'user_code') { + $parameter = $hint; } $response = [ diff --git a/api/tests/functional/oauth/CompleteFlowCest.php b/api/tests/functional/oauth/CompleteFlowCest.php index f9d3208..451d7c6 100644 --- a/api/tests/functional/oauth/CompleteFlowCest.php +++ b/api/tests/functional/oauth/CompleteFlowCest.php @@ -156,15 +156,22 @@ final class CompleteFlowCest { ]); } + public function tryToCompleteExpiredDeviceCodeFlow(FunctionalTester $I): void { + $I->amAuthenticated(); + $I->sendPOST('/api/oauth2/v1/complete?' . http_build_query([ + 'user_code' => 'EXPIRED', + ]), ['accept' => true]); + $I->canSeeResponseCodeIs(400); + $I->canSeeResponseContainsJson([ + 'success' => false, + 'error' => 'expired_token', + ]); + } + public function tryToCompleteAlreadyCompletedDeviceCodeFlow(FunctionalTester $I): void { $I->amAuthenticated(); $I->sendPOST('/api/oauth2/v1/complete?' . http_build_query([ - 'user_code' => 'AAAABBBB', - ]), ['accept' => true]); - $I->canSeeResponseCodeIs(200); - - $I->sendPOST('/api/oauth2/v1/complete?' . http_build_query([ - 'user_code' => 'AAAABBBB', + 'user_code' => 'COMPLETED', ]), ['accept' => true]); $I->canSeeResponseCodeIs(400); $I->canSeeResponseContainsJson([ diff --git a/api/tests/functional/oauth/DeviceCodeCest.php b/api/tests/functional/oauth/DeviceCodeCest.php index 504736f..35410cd 100644 --- a/api/tests/functional/oauth/DeviceCodeCest.php +++ b/api/tests/functional/oauth/DeviceCodeCest.php @@ -36,6 +36,19 @@ final class DeviceCodeCest { ]); } + public function pollExpiredDeviceCode(FunctionalTester $I): void { + $I->sendPOST('/api/oauth2/v1/token', [ + 'grant_type' => 'urn:ietf:params:oauth:grant-type:device_code', + 'client_id' => 'ely', + 'device_code' => 'ZFPbWycSxdRpHiYP2wnv3S0KHBgYky8fRDqfhhCqzke7nKuYFfwckZywqU8iUKv3ek4VtiMiMCkiC0YT', + ]); + $I->canSeeResponseCodeIs(400); + $I->canSeeResponseContainsJson([ + 'error' => 'expired_token', + 'message' => 'The `device_code` has expired and the device authorization session has concluded.', + ]); + } + /** * @param Example $case */ diff --git a/api/tests/functional/oauth/ValidateCest.php b/api/tests/functional/oauth/ValidateCest.php index 85a68cc..2fd82f4 100644 --- a/api/tests/functional/oauth/ValidateCest.php +++ b/api/tests/functional/oauth/ValidateCest.php @@ -108,6 +108,32 @@ final class ValidateCest { ]); } + public function expiredCodeForDeviceCode(FunctionalTester $I): void { + $I->sendGET('/api/oauth2/v1/validate', [ + 'user_code' => 'EXPIRED', + ]); + $I->canSeeResponseCodeIs(400); + $I->canSeeResponseContainsJson([ + 'success' => false, + 'error' => 'expired_token', + 'parameter' => 'user_code', + 'statusCode' => 400, + ]); + } + + public function completedCodeForDeviceCode(FunctionalTester $I): void { + $I->sendGET('/api/oauth2/v1/validate', [ + 'user_code' => 'COMPLETED', + ]); + $I->canSeeResponseCodeIs(400); + $I->canSeeResponseContainsJson([ + 'success' => false, + 'error' => 'used_user_code', + 'parameter' => 'user_code', + 'statusCode' => 400, + ]); + } + public function invalidScopesAuthFlow(FunctionalTester $I): void { $I->wantTo('check behavior on some invalid scopes'); $I->sendGET('/api/oauth2/v1/validate', [ diff --git a/common/components/OAuth2/Grants/DeviceCodeGrant.php b/common/components/OAuth2/Grants/DeviceCodeGrant.php index 4fff6a0..2c62a7c 100644 --- a/common/components/OAuth2/Grants/DeviceCodeGrant.php +++ b/common/components/OAuth2/Grants/DeviceCodeGrant.php @@ -50,11 +50,15 @@ final class DeviceCodeGrant extends BaseDeviceCodeGrant { $deviceCode = $this->deviceCodeRepository->getDeviceCodeEntityByUserCode($userCode); if ($deviceCode === null) { - throw new OAuthServerException('Unknown user code', 4, 'invalid_user_code', 401); + throw new OAuthServerException('Unknown user code', 4, 'invalid_user_code', 401, 'user_code'); + } + + if ($deviceCode->getExpiryDateTime()->getTimestamp() < time()) { + throw OAuthServerException::expiredToken('user_code'); } if ($deviceCode->getUserIdentifier() !== null) { - throw new OAuthServerException('The user code has already been used', 6, 'used_user_code', 400); + throw new OAuthServerException('The user code has already been used', 6, 'used_user_code', 400, 'user_code'); } $authorizationRequest = new AuthorizationRequest(); diff --git a/common/tests/fixtures/data/oauth-device-codes.php b/common/tests/fixtures/data/oauth-device-codes.php index 5e210a0..53afe1d 100644 --- a/common/tests/fixtures/data/oauth-device-codes.php +++ b/common/tests/fixtures/data/oauth-device-codes.php @@ -2,7 +2,7 @@ declare(strict_types=1); return [ - [ + 'pending-code' => [ 'device_code' => 'nKuYFfwckZywqU8iUKv3ek4VtiMiMCkiC0YTZFPbWycSxdRpHiYP2wnv3S0KHBgYky8fRDqfhhCqzke7', 'user_code' => 'AAAABBBB', 'client_id' => 'ely', @@ -12,4 +12,24 @@ return [ 'last_polled_at' => null, 'expires_at' => time() + 1800, ], + 'expired-code' => [ + 'device_code' => 'ZFPbWycSxdRpHiYP2wnv3S0KHBgYky8fRDqfhhCqzke7nKuYFfwckZywqU8iUKv3ek4VtiMiMCkiC0YT', + 'user_code' => 'EXPIRED', + 'client_id' => 'ely', + 'scopes' => ['minecraft_server_session', 'account_info'], + 'account_id' => null, + 'is_approved' => null, + 'last_polled_at' => time() - 1200, + 'expires_at' => time() - 1800, + ], + 'completed-code' => [ + 'device_code' => 'xdRpHiYP2wnv3S0KHBgYky8fRDqfhhCqzke7nKuYFfwckZywqU8iUKv3ek4VtiMiMCkiC0YTZFPbWycS', + 'user_code' => 'COMPLETED', + 'client_id' => 'ely', + 'scopes' => ['minecraft_server_session', 'account_info'], + 'account_id' => 1, + 'is_approved' => true, + 'last_polled_at' => time(), + 'expires_at' => time() + 1800, + ], ]; diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 8d865fc..2842a92 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -130,11 +130,6 @@ parameters: count: 1 path: api/controllers/SignupController.php - - - message: "#^Method api\\\\eventListeners\\\\MockDataResponse\\:\\:getResponse\\(\\) return type has no value type specified in iterable type array\\.$#" - count: 1 - path: api/eventListeners/MockDataResponse.php - - message: "#^Cannot access offset string on array\\|\\(callable\\(\\)\\: mixed\\)\\.$#" count: 1