Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate confidential clients and determine if the client handles the grant type #1420

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Validate confidential clients and determine if the client handles the grant type #1420

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3bb50c1
always validate the client
hafezdivandari Jun 30, 2024
07871d8
pass grant type to getClientEntity
hafezdivandari Jun 30, 2024
de05074
fix tests
hafezdivandari Aug 29, 2024
f7b9acb
add ClientEntityInterface::hasGrantType()
hafezdivandari Oct 1, 2024
3f36fe5
use unauthorized_client error
hafezdivandari Oct 4, 2024
0c5fd78
Merge branch 'master' into always-validate-client
hafezdivandari Oct 18, 2024
9d88ce9
validate confidential clients
hafezdivandari Oct 18, 2024
34d83aa
require client_secret for confidential clients
hafezdivandari Oct 22, 2024
c4c7362
redirect uri is required on auth code
hafezdivandari Nov 9, 2024
cd2f0bd
fix tests
hafezdivandari Nov 9, 2024
44bb1bd
Merge branch 'master' into always-validate-client
hafezdivandari Nov 19, 2024
4998c4a
fix tests
hafezdivandari Nov 19, 2024
cc3a244
bypass bc breaking change and add tests
hafezdivandari Dec 4, 2024
db44e64
formatting
hafezdivandari Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Entities/ClientEntityInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,9 @@ public function getRedirectUri(): string|array;
* Returns true if the client is confidential.
*/
public function isConfidential(): bool;

/**
* Returns true if the client handles the given grant type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a grant type settings. Otherwise the client must know about all grant types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the client may know the grant types it can handle, for example by having grant_types according to RFC7591

Copy link
Contributor

@eugene-borovov eugene-borovov Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC7591 describes the proccess of filling the client repository. It responses with a client information record. ClientRepositoryInterface has validateClient($clientIdentifier, $clientSecret, $grantType) method. The client's support for the grant type should be done in this method. Do you agree with it?

Copy link
Contributor Author

@hafezdivandari hafezdivandari Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC7591 describes the proccess of filling the client repository. It responses with a client information record

It was just an example of how client may define its supported grant types.

ClientRepositoryInterface has validateClient($clientIdentifier, $clientSecret, $grantType) method. The client's support for the grant type should be done in this method. Do you agree with it?

That's exactly what I don't agree with in the current implementation, and this PR tries to fix.

First of all, this method returns bool, and causes invalid_client error when it returns false, but we need unauthorized_client error, when the client doesn't support the given grant type.

Second this method (validateClient) main intention is to validate the client secret, you may check its description.

Third, this method is being called too late, in the last step of the flow, on respondToAccessTokenRequest method of all grants. But we want to check if the client actually handles the grant type in the very beginning of the flow.

Lets assume we have a client that only supports client_credentials grant, currently you can use this client to get an authorization code with no problem, and then in the last step when you want to use that code to get an access_token the server calls validateClient method, and you'll get invalid_client error. After this PR, you will get unauthorized_client error, in the first step, when you want to use that client to get an authorization code, because this client doesn't actually handle "authorization code" grant but only "client credentials" grant.

*/
public function hasGrantType(string $grantType): bool;
}
8 changes: 8 additions & 0 deletions src/Entities/Traits/ClientTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,12 @@ public function isConfidential(): bool
{
return $this->isConfidential;
}

/**
* Returns true if the client handles the given grant type.
*/
public function hasGrantType(string $grantType): bool
{
return true;
}
}
11 changes: 10 additions & 1 deletion src/Exception/OAuthServerException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,17 @@ public static function slowDown(string $hint = '', Throwable $previous = null):
}

/**
* Unauthorized client error.
*/
public static function unauthorizedClient(?string $hint = null): static
{
return $this->errorType;
return new static(
'The authenticated client is not authorized to use this authorization grant type.',
14,
'unauthorized_client',
400,
$hint
);
}

/**
Expand Down
26 changes: 17 additions & 9 deletions src/Grant/AbstractGrant.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,18 +151,18 @@ protected function validateClient(ServerRequestInterface $request): ClientEntity
{
[$clientId, $clientSecret] = $this->getClientCredentials($request);

if ($this->clientRepository->validateClient($clientId, $clientSecret, $this->getIdentifier()) === false) {
$this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request));

throw OAuthServerException::invalidClient($request);
}
$client = $this->getClientEntityOrFail($clientId, $request);

// If a redirect URI is provided ensure it matches what is pre-registered
$redirectUri = $this->getRequestParameter('redirect_uri', $request);
if ($client->isConfidential()) {
if ($clientSecret === '') {
throw OAuthServerException::invalidRequest('client_secret');
}

if ($redirectUri !== null) {
$this->validateRedirectUri($redirectUri, $client, $request);
if ($this->clientRepository->validateClient($clientId, $clientSecret, $this->getIdentifier()) === false) {
$this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request));

throw OAuthServerException::invalidClient($request);
}
}

return $client;
Expand All @@ -189,6 +189,10 @@ protected function getClientEntityOrFail(string $clientId, ServerRequestInterfac
throw OAuthServerException::invalidClient($request);
}

if (!$client->hasGrantType($this->getIdentifier())) {
throw OAuthServerException::unauthorizedClient();
}

return $client;
}

Expand Down Expand Up @@ -484,6 +488,10 @@ protected function issueAuthCode(
*/
protected function issueRefreshToken(AccessTokenEntityInterface $accessToken): ?RefreshTokenEntityInterface
{
if (!$accessToken->getClient()->hasGrantType('refresh_token')) {
return null;
}

$refreshToken = $this->refreshTokenRepository->getNewRefreshToken();

if ($refreshToken === null) {
Expand Down
9 changes: 1 addition & 8 deletions src/Grant/AuthCodeGrant.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,14 +96,7 @@ public function respondToAccessTokenRequest(
ResponseTypeInterface $responseType,
DateInterval $accessTokenTTL
): ResponseTypeInterface {
list($clientId) = $this->getClientCredentials($request);

$client = $this->getClientEntityOrFail($clientId, $request);

// Only validate the client if it is confidential
if ($client->isConfidential()) {
$this->validateClient($request);
}
$client = $this->validateClient($request);

$encryptedAuthCode = $this->getRequestParameter('code', $request);

Expand Down
7 changes: 1 addition & 6 deletions src/Grant/ClientCredentialsGrant.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,14 @@ public function respondToAccessTokenRequest(
ResponseTypeInterface $responseType,
DateInterval $accessTokenTTL
): ResponseTypeInterface {
list($clientId) = $this->getClientCredentials($request);

$client = $this->getClientEntityOrFail($clientId, $request);
$client = $this->validateClient($request);

if (!$client->isConfidential()) {
$this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request));

throw OAuthServerException::invalidClient($request);
}

// Validate request
$this->validateClient($request);

$scopes = $this->validateScopes($this->getRequestParameter('scope', $request, $this->defaultScope));

// Finalize the requested scopes
Expand Down
80 changes: 2 additions & 78 deletions tests/Grant/AbstractGrantTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,84 +265,6 @@ public function testValidateClientInvalidClientSecret(): void
$validateClientMethod->invoke($grantMock, $serverRequest, true, true);
}

public function testValidateClientInvalidRedirectUri(): void
{
$client = new ClientEntity();
$client->setRedirectUri('http://foo/bar');
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);

/** @var AbstractGrant $grantMock */
$grantMock = $this->getMockForAbstractClass(AbstractGrant::class);
$grantMock->setClientRepository($clientRepositoryMock);

$abstractGrantReflection = new ReflectionClass($grantMock);

$serverRequest = (new ServerRequest())->withParsedBody([
'client_id' => 'foo',
'redirect_uri' => 'http://bar/foo',
]);

$validateClientMethod = $abstractGrantReflection->getMethod('validateClient');
$validateClientMethod->setAccessible(true);

$this->expectException(OAuthServerException::class);

$validateClientMethod->invoke($grantMock, $serverRequest, true, true);
}

public function testValidateClientInvalidRedirectUriArray(): void
{
$client = new ClientEntity();
$client->setRedirectUri(['http://foo/bar']);
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);

/** @var AbstractGrant $grantMock */
$grantMock = $this->getMockForAbstractClass(AbstractGrant::class);
$grantMock->setClientRepository($clientRepositoryMock);

$abstractGrantReflection = new ReflectionClass($grantMock);

$serverRequest = (new ServerRequest())->withParsedBody([
'client_id' => 'foo',
'redirect_uri' => 'http://bar/foo',
]);

$validateClientMethod = $abstractGrantReflection->getMethod('validateClient');
$validateClientMethod->setAccessible(true);

$this->expectException(OAuthServerException::class);

$validateClientMethod->invoke($grantMock, $serverRequest, true, true);
}

public function testValidateClientMalformedRedirectUri(): void
{
$client = new ClientEntity();
$client->setRedirectUri('http://foo/bar');
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);

/** @var AbstractGrant $grantMock */
$grantMock = $this->getMockForAbstractClass(AbstractGrant::class);
$grantMock->setClientRepository($clientRepositoryMock);

$abstractGrantReflection = new ReflectionClass($grantMock);

$serverRequest = (new ServerRequest())->withParsedBody([
'client_id' => 'foo',
'redirect_uri' => ['not', 'a', 'string'],
]);

$validateClientMethod = $abstractGrantReflection->getMethod('validateClient');
$validateClientMethod->setAccessible(true);

$this->expectException(OAuthServerException::class);

$validateClientMethod->invoke($grantMock, $serverRequest, true, true);
}

public function testValidateClientBadClient(): void
{
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -398,6 +320,7 @@ public function testIssueRefreshToken(): void
$issueRefreshTokenMethod->setAccessible(true);

$accessToken = new AccessTokenEntity();
$accessToken->setClient(new ClientEntity());

/** @var RefreshTokenEntityInterface $refreshToken */
$refreshToken = $issueRefreshTokenMethod->invoke($grantMock, $accessToken);
Expand All @@ -423,6 +346,7 @@ public function testIssueNullRefreshToken(): void
$issueRefreshTokenMethod->setAccessible(true);

$accessToken = new AccessTokenEntity();
$accessToken->setClient(new ClientEntity());
self::assertNull($issueRefreshTokenMethod->invoke($grantMock, $accessToken));
}

Expand Down
Loading