From b79ca63958f1ce2286b5e801535e8930d8308699 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Sun, 30 Jun 2024 06:51:55 +0330 Subject: [PATCH 1/6] check client handles grant --- src/Bridge/ClientRepository.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Bridge/ClientRepository.php b/src/Bridge/ClientRepository.php index 0420808e..8071e976 100644 --- a/src/Bridge/ClientRepository.php +++ b/src/Bridge/ClientRepository.php @@ -32,11 +32,11 @@ public function __construct(ClientModelRepository $clients, Hasher $hasher) /** * {@inheritdoc} */ - public function getClientEntity(string $clientIdentifier): ?ClientEntityInterface + public function getClientEntity(string $clientIdentifier, ?string $grantType): ?ClientEntityInterface { $record = $this->clients->findActive($clientIdentifier); - if (! $record) { + if (! $record || ! $this->handlesGrant($record, $grantType)) { return null; } From eb3d427b0fdd090e75dbbdc858c12ee3b4919549 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Tue, 1 Oct 2024 17:39:06 +0330 Subject: [PATCH 2/6] override Bridge\Client::hasGrantType --- src/Bridge/Client.php | 11 ++++++++++- src/Bridge/ClientRepository.php | 7 ++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/Bridge/Client.php b/src/Bridge/Client.php index df9522d1..7cb5d83f 100644 --- a/src/Bridge/Client.php +++ b/src/Bridge/Client.php @@ -21,7 +21,8 @@ public function __construct( string $name, array $redirectUri, bool $isConfidential = false, - public ?string $provider = null + public ?string $provider = null, + public ?array $grantTypes = null, ) { $this->setIdentifier($identifier); @@ -29,4 +30,12 @@ public function __construct( $this->isConfidential = $isConfidential; $this->redirectUri = $redirectUri; } + + /** + * {@inheritdoc} + */ + public function hasGrantType(string $grantType): bool + { + return is_null($this->grantTypes) || in_array($grantType, $this->grantTypes); + } } diff --git a/src/Bridge/ClientRepository.php b/src/Bridge/ClientRepository.php index 77d25fdd..07644e43 100644 --- a/src/Bridge/ClientRepository.php +++ b/src/Bridge/ClientRepository.php @@ -22,11 +22,11 @@ public function __construct( /** * {@inheritdoc} */ - public function getClientEntity(string $clientIdentifier, ?string $grantType): ?ClientEntityInterface + public function getClientEntity(string $clientIdentifier): ?ClientEntityInterface { $record = $this->clients->findActive($clientIdentifier); - return $record && $this->handlesGrant($record, $grantType) ? $this->fromClientModel($record) : null; + return $record ? $this->fromClientModel($record) : null; } /** @@ -82,7 +82,8 @@ protected function fromClientModel(ClientModel $model): ClientEntityInterface $model->name, $model->redirect_uris, $model->confidential(), - $model->provider + $model->provider, + isset($model['grant_types']) ? $model->grant_types : null ); } } From 1fe0de4da589c918471efa2449f11b7b8cafeb6c Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Sun, 20 Oct 2024 03:04:47 +0330 Subject: [PATCH 3/6] simplify validate client --- src/Bridge/Client.php | 4 +- src/Bridge/ClientRepository.php | 19 +-- src/Client.php | 36 +++-- tests/Feature/ClientTest.php | 4 +- tests/Unit/BridgeClientRepositoryTest.php | 153 +--------------------- 5 files changed, 38 insertions(+), 178 deletions(-) diff --git a/src/Bridge/Client.php b/src/Bridge/Client.php index 7cb5d83f..c5a58d8e 100644 --- a/src/Bridge/Client.php +++ b/src/Bridge/Client.php @@ -22,7 +22,7 @@ public function __construct( array $redirectUri, bool $isConfidential = false, public ?string $provider = null, - public ?array $grantTypes = null, + public array $grantTypes = [], ) { $this->setIdentifier($identifier); @@ -36,6 +36,6 @@ public function __construct( */ public function hasGrantType(string $grantType): bool { - return is_null($this->grantTypes) || in_array($grantType, $this->grantTypes); + return in_array($grantType, $this->grantTypes); } } diff --git a/src/Bridge/ClientRepository.php b/src/Bridge/ClientRepository.php index 07644e43..a5d4ee26 100644 --- a/src/Bridge/ClientRepository.php +++ b/src/Bridge/ClientRepository.php @@ -34,24 +34,9 @@ public function getClientEntity(string $clientIdentifier): ?ClientEntityInterfac */ public function validateClient(string $clientIdentifier, ?string $clientSecret, ?string $grantType): bool { - // First, we will verify that the client exists and is authorized to create personal - // access tokens. Generally personal access tokens are only generated by the user - // from the main interface. We'll only let certain clients generate the tokens. $record = $this->clients->findActive($clientIdentifier); - if (! $record || ! $this->handlesGrant($record, $grantType)) { - return false; - } - - return ! $record->confidential() || $this->verifySecret($clientSecret, $record->secret); - } - - /** - * Determine if the given client can handle the given grant type. - */ - protected function handlesGrant(ClientModel $record, string $grantType): bool - { - return $record->hasGrantType($grantType); + return $record && ! empty($clientSecret) && $this->verifySecret($clientSecret, $record->secret); } /** @@ -83,7 +68,7 @@ protected function fromClientModel(ClientModel $model): ClientEntityInterface $model->redirect_uris, $model->confidential(), $model->provider, - isset($model['grant_types']) ? $model->grant_types : null + $model->grant_types ); } } diff --git a/src/Client.php b/src/Client.php index 7bda223d..3cf1245d 100644 --- a/src/Client.php +++ b/src/Client.php @@ -147,6 +147,30 @@ protected function redirectUris(): Attribute ); } + /** + * Get the client's grant types. + */ + protected function grantTypes(): Attribute + { + return Attribute::make( + get: function (?string $value, array $attributes) { + if (isset($value)) { + return $this->fromJson($value); + } + + return array_keys(array_filter([ + 'authorization_code' => ! empty($this->redirect_uris), + 'client_credentials' => $this->confidential(), + 'implicit' => ! empty($this->redirect_uris), + 'password' => $this->password_client, + 'personal_access' => $this->personal_access_client, + 'refresh_token' => true, + 'urn:ietf:params:oauth:grant-type:device_code' => true, + ])); + }, + ); + } + /** * Determine if the client is a "first party" client. */ @@ -170,17 +194,7 @@ public function skipsAuthorization(Authenticatable $user, array $scopes): bool */ public function hasGrantType(string $grantType): bool { - if (isset($this->attributes['grant_types']) && is_array($this->grant_types)) { - return in_array($grantType, $this->grant_types); - } - - return match ($grantType) { - 'authorization_code' => ! $this->personal_access_client && ! $this->password_client, - 'personal_access' => $this->personal_access_client && $this->confidential(), - 'password' => $this->password_client, - 'client_credentials' => $this->confidential(), - default => true, - }; + return in_array($grantType, $this->grant_types); } /** diff --git a/tests/Feature/ClientTest.php b/tests/Feature/ClientTest.php index fbaac916..c4b46b1b 100644 --- a/tests/Feature/ClientTest.php +++ b/tests/Feature/ClientTest.php @@ -75,7 +75,7 @@ public function testGrantTypesWhenColumnDoesNotExist(): void $client = new Client(); $client->exists = true; - $this->assertTrue($client->hasGrantType('foo')); + $this->assertFalse($client->hasGrantType('foo')); $client->personal_access_client = false; $client->password_client = false; @@ -100,7 +100,7 @@ public function testGrantTypesWhenColumnIsNull(): void $client = new Client(['grant_types' => null]); $client->exists = true; - $this->assertTrue($client->hasGrantType('foo')); + $this->assertFalse($client->hasGrantType('foo')); $client->personal_access_client = false; $client->password_client = false; diff --git a/tests/Unit/BridgeClientRepositoryTest.php b/tests/Unit/BridgeClientRepositoryTest.php index a5330d77..7072cf63 100644 --- a/tests/Unit/BridgeClientRepositoryTest.php +++ b/tests/Unit/BridgeClientRepositoryTest.php @@ -57,156 +57,17 @@ public function test_can_get_client() $this->assertTrue($client->isConfidential()); } - public function test_can_validate_client_for_auth_code_grant() + public function test_can_validate_client() { $this->assertTrue($this->repository->validateClient(1, 'secret', 'authorization_code')); $this->assertFalse($this->repository->validateClient(1, 'wrong-secret', 'authorization_code')); $this->assertFalse($this->repository->validateClient(1, 'wrong-secret', 'client_credentials')); - } - - public function test_can_validate_client_for_client_credentials_grant() - { - $client = $this->clientModelRepository->findActive(1); - $client->grant_types = ['client_credentials']; - - $this->assertTrue($this->repository->validateClient(1, 'secret', 'client_credentials')); - $this->assertFalse($this->repository->validateClient(1, 'wrong-secret', 'client_credentials')); - $this->assertFalse($this->repository->validateClient(1, 'secret', 'authorization_code')); - } - - public function test_password_grant_is_permitted() - { - $client = $this->clientModelRepository->findActive(1); - $client->grant_types = ['password']; - - $this->assertTrue($this->repository->validateClient(1, 'secret', 'password')); - } - - public function test_public_client_password_grant_is_permitted() - { - $client = $this->clientModelRepository->findActive(1); - $client->grant_types = ['password']; - $client->secret = null; - - $this->assertTrue($this->repository->validateClient(1, null, 'password')); - } - - public function test_password_grant_is_prevented() - { - $this->assertFalse($this->repository->validateClient(1, 'secret', 'password')); - } - - public function test_authorization_code_grant_is_permitted() - { - $this->assertTrue($this->repository->validateClient(1, 'secret', 'authorization_code')); - } - - public function test_public_client_authorization_code_grant_is_permitted() - { - $client = $this->clientModelRepository->findActive(1); - $client->secret = null; - - $this->assertTrue($this->repository->validateClient(1, null, 'authorization_code')); - } - - public function test_authorization_code_grant_is_prevented() - { - $client = $this->clientModelRepository->findActive(1); - $client->grant_types = ['password']; - - $this->assertFalse($this->repository->validateClient(1, 'secret', 'authorization_code')); - } - - public function test_personal_access_grant_is_permitted() - { - $client = $this->clientModelRepository->findActive(1); - $client->grant_types = ['personal_access']; - - $this->assertTrue($this->repository->validateClient(1, 'secret', 'personal_access')); - } - - public function test_personal_access_grant_is_prevented() - { - $this->assertFalse($this->repository->validateClient(1, 'secret', 'personal_access')); - } - - public function test_client_credentials_grant_is_prevented() - { - $this->assertFalse($this->repository->validateClient(1, 'secret', 'client_credentials')); - } - - public function test_grant_types_allows_request() - { - $client = $this->clientModelRepository->findActive(1); - $client->grant_types = ['client_credentials']; - - $this->assertTrue($this->repository->validateClient(1, 'secret', 'client_credentials')); - } - - public function test_grant_types_disallows_request() - { - $client = $this->clientModelRepository->findActive(1); - $client->grant_types = ['client_credentials']; - - $this->assertFalse($this->repository->validateClient(1, 'secret', 'authorization_code')); - } - - public function test_refresh_grant_is_permitted() - { - $this->assertTrue($this->repository->validateClient(1, 'secret', 'refresh_token')); - } - - public function test_public_refresh_grant_is_permitted() - { - $client = $this->clientModelRepository->findActive(1); - $client->secret = null; - - $this->assertTrue($this->repository->validateClient(1, null, 'refresh_token')); - } - - public function test_refresh_grant_is_prevented() - { - $this->assertFalse($this->repository->validateClient(1, 'wrong-secret', 'refresh_token')); - } - - public function test_without_grant_types() - { - $client = $this->clientModelRepository->findActive(1); - $client->grant_types = null; - - $this->assertTrue($this->repository->validateClient(1, 'secret', 'client_credentials')); - $this->assertFalse($this->repository->validateClient(1, 'wrong-secret', 'client_credentials')); - - $client->personal_access_client = true; - $client->password_client = false; - - $this->assertTrue($this->repository->validateClient(1, 'secret', 'client_credentials')); - $this->assertFalse($this->repository->validateClient(1, 'secret', 'authorization_code')); - $this->assertTrue($this->repository->validateClient(1, 'secret', 'personal_access')); - $this->assertFalse($this->repository->validateClient(1, 'wrong-secret', 'personal_access')); - $this->assertFalse($this->repository->validateClient(1, 'secret', 'password')); - - $client->personal_access_client = false; - $client->password_client = true; - - $this->assertTrue($this->repository->validateClient(1, 'secret', 'client_credentials')); - $this->assertFalse($this->repository->validateClient(1, 'secret', 'authorization_code')); - $this->assertTrue($this->repository->validateClient(1, 'secret', 'password')); - $this->assertFalse($this->repository->validateClient(1, 'wrong-secret', 'password')); - $this->assertFalse($this->repository->validateClient(1, 'secret', 'personal_access')); - - $client->personal_access_client = false; - $client->password_client = true; - $client->secret = null; - - $this->assertFalse($this->repository->validateClient(1, null, 'client_credentials')); - $this->assertTrue($this->repository->validateClient(1, null, 'password')); - - $client->personal_access_client = true; - $client->password_client = false; - $client->secret = null; - - $this->assertFalse($this->repository->validateClient(1, null, 'personal_access')); + $this->assertFalse($this->repository->validateClient(1, null, 'authorization_code')); + $this->assertFalse($this->repository->validateClient(1, '', 'authorization_code')); + $this->assertTrue($this->repository->validateClient(1, 'secret', null)); + $this->assertFalse($this->repository->validateClient(1, 'wrong-secret', null)); + $this->assertFalse($this->repository->validateClient(1, null, null)); + $this->assertFalse($this->repository->validateClient(1, '', null)); } } From b26fa2efbbd1e8db3c2b4ad8b703ccede2069bbd Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Tue, 22 Oct 2024 17:25:36 +0330 Subject: [PATCH 4/6] formatting --- src/Bridge/ClientRepository.php | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/Bridge/ClientRepository.php b/src/Bridge/ClientRepository.php index a5d4ee26..b7fbd770 100644 --- a/src/Bridge/ClientRepository.php +++ b/src/Bridge/ClientRepository.php @@ -36,15 +36,7 @@ public function validateClient(string $clientIdentifier, ?string $clientSecret, { $record = $this->clients->findActive($clientIdentifier); - return $record && ! empty($clientSecret) && $this->verifySecret($clientSecret, $record->secret); - } - - /** - * Verify the client secret is valid. - */ - protected function verifySecret(string $clientSecret, string $storedHash): bool - { - return $this->hasher->check($clientSecret, $storedHash); + return $record && ! empty($clientSecret) && $this->hasher->check($clientSecret, $record->secret); } /** From bce09dd6d7c74b2f3dae63fb0ccd06b396a15b0a Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Tue, 22 Oct 2024 19:34:07 +0330 Subject: [PATCH 5/6] add tests --- tests/Feature/AuthorizationCodeGrantTest.php | 63 ++++++++++++++++++++ tests/Feature/ClientCredentialsGrantTest.php | 17 ++++++ tests/Feature/ClientTest.php | 22 +++++-- tests/Feature/ImplicitGrantTest.php | 25 ++++++++ 4 files changed, 123 insertions(+), 4 deletions(-) diff --git a/tests/Feature/AuthorizationCodeGrantTest.php b/tests/Feature/AuthorizationCodeGrantTest.php index 60121372..d59d9f01 100644 --- a/tests/Feature/AuthorizationCodeGrantTest.php +++ b/tests/Feature/AuthorizationCodeGrantTest.php @@ -338,4 +338,67 @@ public function testPromptLogin() $response->assertSessionHas('promptedForLogin', true); $response->assertRedirectToRoute('login'); } + + public function testUnauthorizedClient() + { + $client = ClientFactory::new()->create([ + 'grant_types' => [], + ]); + + $query = http_build_query([ + 'client_id' => $client->getKey(), + 'redirect_uri' => $client->redirect_uris[0], + 'response_type' => 'code', + ]); + + $user = UserFactory::new()->create(); + $this->actingAs($user, 'web'); + + $json = $this->get('/oauth/authorize?'.$query) + ->assertBadRequest() + ->assertSessionMissing(['authRequest', 'authToken']) + ->json(); + + $this->assertSame('unauthorized_client', $json['error']); + $this->assertSame( + 'The authenticated client is not authorized to use this authorization grant type.', + $json['error_description'] + ); + } + + public function testIssueAccessTokenWithoutRefreshToken() + { + $client = ClientFactory::new()->create([ + 'grant_types' => ['authorization_code'], + ]); + + $query = http_build_query([ + 'client_id' => $client->getKey(), + 'redirect_uri' => $redirect = $client->redirect_uris[0], + 'response_type' => 'code', + ]); + + $user = UserFactory::new()->create(); + $this->actingAs($user, 'web'); + + $authToken = $this->get('/oauth/authorize?'.$query) + ->assertOk() + ->json('authToken'); + + $response = $this->post('/oauth/authorize', ['auth_token' => $authToken])->assertRedirect(); + parse_str(parse_url($response->headers->get('Location'), PHP_URL_QUERY), $params); + + $json = $this->post('/oauth/token', [ + 'grant_type' => 'authorization_code', + 'client_id' => $client->getKey(), + 'client_secret' => $client->plainSecret, + 'redirect_uri' => $redirect, + 'code' => $params['code'], + ])->assertOk()->json(); + + $this->assertArrayHasKey('access_token', $json); + $this->assertArrayNotHasKey('refresh_token', $json); + $this->assertSame('Bearer', $json['token_type']); + $this->assertArrayHasKey('expires_in', $json); + } } diff --git a/tests/Feature/ClientCredentialsGrantTest.php b/tests/Feature/ClientCredentialsGrantTest.php index b0711e09..fcb08b85 100644 --- a/tests/Feature/ClientCredentialsGrantTest.php +++ b/tests/Feature/ClientCredentialsGrantTest.php @@ -54,4 +54,21 @@ public function testIssueAccessToken() $response = $this->withToken($json['access_token'], $json['token_type'])->get('/bar'); $response->assertForbidden(); } + + public function testUnauthorizedClient() + { + $client = ClientFactory::new()->create(); + + $json = $this->post('/oauth/token', [ + 'grant_type' => 'client_credentials', + 'client_id' => $client->getKey(), + 'client_secret' => $client->plainSecret, + ])->assertBadRequest()->json(); + + $this->assertSame('unauthorized_client', $json['error']); + $this->assertSame( + 'The authenticated client is not authorized to use this authorization grant type.', + $json['error_description'] + ); + } } diff --git a/tests/Feature/ClientTest.php b/tests/Feature/ClientTest.php index c4b46b1b..b58a714b 100644 --- a/tests/Feature/ClientTest.php +++ b/tests/Feature/ClientTest.php @@ -75,12 +75,19 @@ public function testGrantTypesWhenColumnDoesNotExist(): void $client = new Client(); $client->exists = true; - $this->assertFalse($client->hasGrantType('foo')); - $client->personal_access_client = false; $client->password_client = false; + $this->assertFalse($client->hasGrantType('foo')); + $this->assertFalse($client->hasGrantType('authorization_code')); + $this->assertFalse($client->hasGrantType('password')); + $this->assertFalse($client->hasGrantType('personal_access')); + $this->assertFalse($client->hasGrantType('client_credentials')); + + $client->redirect = 'http://localhost'; $this->assertTrue($client->hasGrantType('authorization_code')); + $this->assertTrue($client->hasGrantType('implicit')); + unset($client->redirect); $client->personal_access_client = false; $client->password_client = true; @@ -100,11 +107,18 @@ public function testGrantTypesWhenColumnIsNull(): void $client = new Client(['grant_types' => null]); $client->exists = true; - $this->assertFalse($client->hasGrantType('foo')); - $client->personal_access_client = false; $client->password_client = false; + $this->assertFalse($client->hasGrantType('foo')); + $this->assertFalse($client->hasGrantType('authorization_code')); + $this->assertFalse($client->hasGrantType('password')); + $this->assertFalse($client->hasGrantType('personal_access')); + $this->assertFalse($client->hasGrantType('client_credentials')); + + $client->redirect = 'http://localhost'; $this->assertTrue($client->hasGrantType('authorization_code')); + $this->assertTrue($client->hasGrantType('implicit')); + unset($client->redirect); $client->personal_access_client = false; $client->password_client = true; diff --git a/tests/Feature/ImplicitGrantTest.php b/tests/Feature/ImplicitGrantTest.php index aa23a407..658f7c27 100644 --- a/tests/Feature/ImplicitGrantTest.php +++ b/tests/Feature/ImplicitGrantTest.php @@ -319,4 +319,29 @@ public function testPromptLogin() $response->assertSessionHas('promptedForLogin', true); $response->assertRedirectToRoute('login'); } + + public function testUnauthorizedClient() + { + $client = ClientFactory::new()->create(); + + $query = http_build_query([ + 'client_id' => $client->getKey(), + 'redirect_uri' => $client->redirect_uris[0], + 'response_type' => 'token', + ]); + + $user = UserFactory::new()->create(); + $this->actingAs($user, 'web'); + + $json = $this->get('/oauth/authorize?'.$query) + ->assertBadRequest() + ->assertSessionMissing(['authRequest', 'authToken']) + ->json(); + + $this->assertSame('unauthorized_client', $json['error']); + $this->assertSame( + 'The authenticated client is not authorized to use this authorization grant type.', + $json['error_description'] + ); + } } From 2c0564368286a5800d36fec9dfd42cb8c4c74457 Mon Sep 17 00:00:00 2001 From: Hafez Divandari Date: Mon, 28 Oct 2024 00:41:54 +0330 Subject: [PATCH 6/6] formatting --- src/Client.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Client.php b/src/Client.php index 3cf1245d..1c588590 100644 --- a/src/Client.php +++ b/src/Client.php @@ -160,7 +160,7 @@ protected function grantTypes(): Attribute return array_keys(array_filter([ 'authorization_code' => ! empty($this->redirect_uris), - 'client_credentials' => $this->confidential(), + 'client_credentials' => $this->confidential() && $this->firstParty(), 'implicit' => ! empty($this->redirect_uris), 'password' => $this->password_client, 'personal_access' => $this->personal_access_client,