From 27d5c5ed8da27586de511cf40b8a44170a4b3ab7 Mon Sep 17 00:00:00 2001 From: Ian Littman Date: Sat, 11 May 2019 14:05:39 -0500 Subject: [PATCH 1/4] Ensure unvalidated ClientEntity gets throw/emit if they return null In many cases, we validate client info before pulling from client itself from the repository, in which case it's safe to assume that you can grab the client once validation passes. However on implicit/auth code grants we don't have this guarantee due to non-confidential clients that just reference the client ID. In those cases the client may supply a client ID that doesn't exist, and we don't do a validation step before pulling it from the repo. The issue with that is that ClientRepository doesn't actually enforce returning a ClientInterface via typehint, nor does it even suggest an exception to throw if the client doesn't exist. So in most places we do an instanceof check after the repository returns and throw/emit an error event if the client doesn't exist. This approach ends up being a bit error-prone; we missed one case where we should've been doing this check: in the access token request on an auth code grant. We don't do enough validation beforehand to assume that the incoming request has an accurate client ID, so L96 could absolutely be a method call on a non-object. This commit centralizes the return-check-emit-throw logic so it's a one-liner for wherever we need it, including the access token request processor for auth code grants. --- src/Grant/AbstractGrant.php | 27 +++++++++++++++++++++++++++ src/Grant/AuthCodeGrant.php | 10 ++-------- src/Grant/ImplicitGrant.php | 8 +------- 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/Grant/AbstractGrant.php b/src/Grant/AbstractGrant.php index d64c9697..4836e8dd 100644 --- a/src/Grant/AbstractGrant.php +++ b/src/Grant/AbstractGrant.php @@ -193,6 +193,33 @@ abstract class AbstractGrant implements GrantTypeInterface return $client; } + /** + * Wrapper around ClientRepository::getClientEntity() that ensures we emit + * an event and throw an exception if the repo doesn't return a client + * entity. + * + * This is a bit of defensive coding because the interface contract + * doesn't actually enforce non-null returns/exception-on-no-client so + * getClientEntity might return null. By contrast, this method will + * always either return a ClientEntityInterface or throw. + * + * @param string $clientId + * @param ServerRequestInterface $request + * + * @return ClientEntityInterface + */ + protected function getClientEntityOrFail($clientId, ServerRequestInterface $request) + { + $client = $this->clientRepository->getClientEntity($clientId); + + if ($client instanceof ClientEntityInterface === false) { + $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); + throw OAuthServerException::invalidClient($request); + } + + return $client; + } + /** * Gets the client credentials from the request from the request body or * the Http Basic Authorization header diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index a26ea6ec..ea9cf37e 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -14,7 +14,6 @@ use DateTimeImmutable; use League\OAuth2\Server\CodeChallengeVerifiers\CodeChallengeVerifierInterface; use League\OAuth2\Server\CodeChallengeVerifiers\PlainVerifier; use League\OAuth2\Server\CodeChallengeVerifiers\S256Verifier; -use League\OAuth2\Server\Entities\ClientEntityInterface; use League\OAuth2\Server\Entities\ScopeEntityInterface; use League\OAuth2\Server\Entities\UserEntityInterface; use League\OAuth2\Server\Exception\OAuthServerException; @@ -94,7 +93,7 @@ class AuthCodeGrant extends AbstractAuthorizeGrant ) { list($clientId) = $this->getClientCredentials($request); - $client = $this->clientRepository->getClientEntity($clientId); + $client = $this->getClientEntityOrFail($clientId, $request); // Only validate the client if it is confidential if ($client->isConfidential()) { @@ -247,12 +246,7 @@ class AuthCodeGrant extends AbstractAuthorizeGrant throw OAuthServerException::invalidRequest('client_id'); } - $client = $this->clientRepository->getClientEntity($clientId); - - if ($client instanceof ClientEntityInterface === false) { - $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient($request); - } + $client = $this->getClientEntityOrFail($clientId, $request); $redirectUri = $this->getQueryStringParameter('redirect_uri', $request); diff --git a/src/Grant/ImplicitGrant.php b/src/Grant/ImplicitGrant.php index 5fc08cf1..8de22c36 100644 --- a/src/Grant/ImplicitGrant.php +++ b/src/Grant/ImplicitGrant.php @@ -10,7 +10,6 @@ namespace League\OAuth2\Server\Grant; use DateInterval; -use League\OAuth2\Server\Entities\ClientEntityInterface; use League\OAuth2\Server\Entities\UserEntityInterface; use League\OAuth2\Server\Exception\OAuthServerException; use League\OAuth2\Server\Repositories\RefreshTokenRepositoryInterface; @@ -124,12 +123,7 @@ class ImplicitGrant extends AbstractAuthorizeGrant throw OAuthServerException::invalidRequest('client_id'); } - $client = $this->clientRepository->getClientEntity($clientId); - - if ($client instanceof ClientEntityInterface === false) { - $this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request)); - throw OAuthServerException::invalidClient($request); - } + $client = $this->getClientEntityOrFail($clientId, $request); $redirectUri = $this->getQueryStringParameter('redirect_uri', $request); From c7d047f7f56b9e594401146e69ac061648a44551 Mon Sep 17 00:00:00 2001 From: sephster Date: Sun, 23 Jun 2019 17:35:24 +0100 Subject: [PATCH 2/4] Remove extra line spaces --- src/Grant/AuthCodeGrant.php | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index 51c9e90e..e1b2b981 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -112,11 +112,8 @@ class AuthCodeGrant extends AbstractAuthorizeGrant try { $authCodePayload = json_decode($this->decrypt($encryptedAuthCode)); - $this->validateAuthorizationCode($authCodePayload, $client, $request); - - $scopes = $this->scopeRepository->finalizeScopes( $this->validateScopes($authCodePayload->scopes), $this->getIdentifier(), From 0db54cf1e58a45bc78d23b973fd37cca547c6e27 Mon Sep 17 00:00:00 2001 From: sephster Date: Sun, 23 Jun 2019 17:40:39 +0100 Subject: [PATCH 3/4] Reinstate use for ClientEntityInterface --- src/Grant/AuthCodeGrant.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Grant/AuthCodeGrant.php b/src/Grant/AuthCodeGrant.php index e1b2b981..a11828d7 100644 --- a/src/Grant/AuthCodeGrant.php +++ b/src/Grant/AuthCodeGrant.php @@ -15,6 +15,7 @@ use Exception; use League\OAuth2\Server\CodeChallengeVerifiers\CodeChallengeVerifierInterface; use League\OAuth2\Server\CodeChallengeVerifiers\PlainVerifier; use League\OAuth2\Server\CodeChallengeVerifiers\S256Verifier; +use League\OAuth2\Server\Entities\ClientEntityInterface; use League\OAuth2\Server\Entities\UserEntityInterface; use League\OAuth2\Server\Exception\OAuthServerException; use League\OAuth2\Server\Repositories\AuthCodeRepositoryInterface; From 012808f094d2f7f2852b103078b16e021c792f9b Mon Sep 17 00:00:00 2001 From: sephster Date: Sun, 23 Jun 2019 17:56:32 +0100 Subject: [PATCH 4/4] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8afc6606..751379cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Public clients can now use the Auth Code Grant (PR #938) - `isConfidential` getter added to `ClientEntity` to identify type of client (PR #938) - Function `validateClient()` added to validate clients which was previously performed by the `getClientEntity()` function (PR #938) +- Add a new function to the AbstractGrant class called `getClientEntityOrFail()`. This is a wrapper around the `getClientEntity()` function that ensures we emit and throw an exception if the repo doesn't return a client entity. (PR #1010) ### Changed - Replace `convertToJWT()` interface with a more generic `__toString()` to improve extensibility; AccessTokenEntityInterface now requires `setPrivateKey(CryptKey $privateKey)` so `__toString()` has everything it needs to work (PR #874)