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

Jump to bottom

Always validate the client and determine if it handles the grant type #1420

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+82 −30
Open

Always validate the client and determine if it handles the grant type #1420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 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
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: 4 additions & 1 deletion examples/src/Repositories/ClientRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ public function validateClient($clientIdentifier, $clientSecret, $grantType): bo
return false;
}

if (password_verify($clientSecret, $clients[$clientIdentifier]['secret']) === false) {
if (
$clients[$clientIdentifier]['is_confidential'] === true
&& password_verify($clientSecret, $clients[$clientIdentifier]['secret']) === false
) {
return false;
}

Expand Down
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.
*/
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
6 changes: 5 additions & 1 deletion src/Grant/AbstractGrant.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 @@ -486,7 +490,7 @@ protected function issueRefreshToken(AccessTokenEntityInterface $accessToken): ?
{
$refreshToken = $this->refreshTokenRepository->getNewRefreshToken();

if ($refreshToken === null) {
if ($refreshToken === null || !$accessToken->getClient()->hasGrantType('refresh_token')) {
return 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
1 change: 1 addition & 0 deletions tests/Grant/AbstractGrantTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ public function testIssueRefreshToken(): void
$issueRefreshTokenMethod->setAccessible(true);

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

/** @var RefreshTokenEntityInterface $refreshToken */
$refreshToken = $issueRefreshTokenMethod->invoke($grantMock, $accessToken);
Expand Down
40 changes: 32 additions & 8 deletions tests/Grant/AuthCodeGrantTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,9 @@ public function testRespondToAccessTokenRequest(): void
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -623,13 +625,16 @@ public function testRespondToAccessTokenRequestUsingHttpBasicAuth(): void
$client->setIdentifier('foo');
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
$clientRepositoryMock->method('validateClient')->willReturn(true);

$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn(new ScopeEntity());
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
$refreshTokenRepositoryMock->method('getNewRefreshToken')->willReturn(new RefreshTokenEntity());
Expand Down Expand Up @@ -686,14 +691,17 @@ public function testRespondToAccessTokenRequestForPublicClient(): void
$client->setRedirectUri(self::REDIRECT_URI);
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
$clientRepositoryMock->method('validateClient')->willReturn(true);

$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
$scopeEntity = new ScopeEntity();
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity);
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -751,6 +759,7 @@ public function testRespondToAccessTokenRequestNullRefreshToken(): void
$client->setRedirectUri(self::REDIRECT_URI);
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
$clientRepositoryMock->method('validateClient')->willReturn(true);

$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
$scopeEntity = new ScopeEntity();
Expand Down Expand Up @@ -828,7 +837,9 @@ public function testRespondToAccessTokenRequestCodeChallengePlain(): void
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -902,7 +913,9 @@ public function testRespondToAccessTokenRequestCodeChallengeS256(): void
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -1187,6 +1200,7 @@ public function testRespondToAccessTokenRequestWithRefreshTokenInsteadOfAuthCode

$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
$clientRepositoryMock->method('validateClient')->willReturn(true);

$grant = new AuthCodeGrant(
$this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(),
Expand Down Expand Up @@ -1276,6 +1290,7 @@ public function testRespondToAccessTokenRequestExpiredCode(): void

$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
$clientRepositoryMock->method('validateClient')->willReturn(true);

$grant = new AuthCodeGrant(
$this->getMockBuilder(AuthCodeRepositoryInterface::class)->getMock(),
Expand Down Expand Up @@ -1980,14 +1995,17 @@ public function testRefreshTokenRepositoryUniqueConstraintCheck(): void
$client->setRedirectUri(self::REDIRECT_URI);
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
$clientRepositoryMock->method('validateClient')->willReturn(true);

$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
$scopeEntity = new ScopeEntity();
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity);
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -2055,14 +2073,17 @@ public function testRefreshTokenRepositoryFailToPersist(): void
$client->setRedirectUri(self::REDIRECT_URI);
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
$clientRepositoryMock->method('validateClient')->willReturn(true);

$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
$scopeEntity = new ScopeEntity();
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity);
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -2123,14 +2144,17 @@ public function testRefreshTokenRepositoryFailToPersistUniqueNoInfiniteLoop(): v
$client->setRedirectUri(self::REDIRECT_URI);
$clientRepositoryMock = $this->getMockBuilder(ClientRepositoryInterface::class)->getMock();
$clientRepositoryMock->method('getClientEntity')->willReturn($client);
$clientRepositoryMock->method('validateClient')->willReturn(true);

$scopeRepositoryMock = $this->getMockBuilder(ScopeRepositoryInterface::class)->getMock();
$scopeEntity = new ScopeEntity();
$scopeRepositoryMock->method('getScopeEntityByIdentifier')->willReturn($scopeEntity);
$scopeRepositoryMock->method('finalizeScopes')->willReturnArgument(0);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down
4 changes: 3 additions & 1 deletion tests/Grant/DeviceCodeGrantTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,9 @@ public function testRespondToAccessTokenRequest(): void
$clientRepositoryMock->method('validateClient')->willReturn(true);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down
4 changes: 3 additions & 1 deletion tests/Grant/PasswordGrantTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ public function testRespondToRequest(): void
$clientRepositoryMock->method('validateClient')->willReturn(true);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$userRepositoryMock = $this->getMockBuilder(UserRepositoryInterface::class)->getMock();
Expand Down
12 changes: 9 additions & 3 deletions tests/Grant/RefreshTokenGrantTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ public function testRespondToRequest(): void
$scopeRepositoryMock->method('finalizeScopes')->willReturn([$scopeEntity]);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->expects(self::once())->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -181,7 +183,9 @@ public function testRespondToReducedScopes(): void
$clientRepositoryMock->method('validateClient')->willReturn(true);

$accessTokenRepositoryMock = $this->getMockBuilder(AccessTokenRepositoryInterface::class)->getMock();
$accessTokenRepositoryMock->method('getNewToken')->willReturn(new AccessTokenEntity());
$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock->method('getNewToken')->willReturn($accessToken);
$accessTokenRepositoryMock->method('persistNewAccessToken')->willReturnSelf();

$refreshTokenRepositoryMock = $this->getMockBuilder(RefreshTokenRepositoryInterface::class)->getMock();
Expand Down Expand Up @@ -567,10 +571,12 @@ public function testRespondToRequestFinalizeScopes(): void
->with($scopes, $grant->getIdentifier(), $client)
->willReturn($finalizedScopes);

$accessToken = new AccessTokenEntity();
$accessToken->setClient($client);
$accessTokenRepositoryMock
->method('getNewToken')
->with($client, $finalizedScopes)
->willReturn(new AccessTokenEntity());
->willReturn($accessToken);

$oldRefreshToken = json_encode(
[
Expand Down