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.
This commit is contained in:
Ian Littman 2019-05-11 14:05:39 -05:00
parent d7defafd83
commit 27d5c5ed8d
No known key found for this signature in database
GPG Key ID: 55488EB78A0AFBE3
3 changed files with 30 additions and 15 deletions

View File

@ -193,6 +193,33 @@ abstract class AbstractGrant implements GrantTypeInterface
return $client; 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 * Gets the client credentials from the request from the request body or
* the Http Basic Authorization header * the Http Basic Authorization header

View File

@ -14,7 +14,6 @@ use DateTimeImmutable;
use League\OAuth2\Server\CodeChallengeVerifiers\CodeChallengeVerifierInterface; use League\OAuth2\Server\CodeChallengeVerifiers\CodeChallengeVerifierInterface;
use League\OAuth2\Server\CodeChallengeVerifiers\PlainVerifier; use League\OAuth2\Server\CodeChallengeVerifiers\PlainVerifier;
use League\OAuth2\Server\CodeChallengeVerifiers\S256Verifier; use League\OAuth2\Server\CodeChallengeVerifiers\S256Verifier;
use League\OAuth2\Server\Entities\ClientEntityInterface;
use League\OAuth2\Server\Entities\ScopeEntityInterface; use League\OAuth2\Server\Entities\ScopeEntityInterface;
use League\OAuth2\Server\Entities\UserEntityInterface; use League\OAuth2\Server\Entities\UserEntityInterface;
use League\OAuth2\Server\Exception\OAuthServerException; use League\OAuth2\Server\Exception\OAuthServerException;
@ -94,7 +93,7 @@ class AuthCodeGrant extends AbstractAuthorizeGrant
) { ) {
list($clientId) = $this->getClientCredentials($request); list($clientId) = $this->getClientCredentials($request);
$client = $this->clientRepository->getClientEntity($clientId); $client = $this->getClientEntityOrFail($clientId, $request);
// Only validate the client if it is confidential // Only validate the client if it is confidential
if ($client->isConfidential()) { if ($client->isConfidential()) {
@ -247,12 +246,7 @@ class AuthCodeGrant extends AbstractAuthorizeGrant
throw OAuthServerException::invalidRequest('client_id'); throw OAuthServerException::invalidRequest('client_id');
} }
$client = $this->clientRepository->getClientEntity($clientId); $client = $this->getClientEntityOrFail($clientId, $request);
if ($client instanceof ClientEntityInterface === false) {
$this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request));
throw OAuthServerException::invalidClient($request);
}
$redirectUri = $this->getQueryStringParameter('redirect_uri', $request); $redirectUri = $this->getQueryStringParameter('redirect_uri', $request);

View File

@ -10,7 +10,6 @@
namespace League\OAuth2\Server\Grant; namespace League\OAuth2\Server\Grant;
use DateInterval; use DateInterval;
use League\OAuth2\Server\Entities\ClientEntityInterface;
use League\OAuth2\Server\Entities\UserEntityInterface; use League\OAuth2\Server\Entities\UserEntityInterface;
use League\OAuth2\Server\Exception\OAuthServerException; use League\OAuth2\Server\Exception\OAuthServerException;
use League\OAuth2\Server\Repositories\RefreshTokenRepositoryInterface; use League\OAuth2\Server\Repositories\RefreshTokenRepositoryInterface;
@ -124,12 +123,7 @@ class ImplicitGrant extends AbstractAuthorizeGrant
throw OAuthServerException::invalidRequest('client_id'); throw OAuthServerException::invalidRequest('client_id');
} }
$client = $this->clientRepository->getClientEntity($clientId); $client = $this->getClientEntityOrFail($clientId, $request);
if ($client instanceof ClientEntityInterface === false) {
$this->getEmitter()->emit(new RequestEvent(RequestEvent::CLIENT_AUTHENTICATION_FAILED, $request));
throw OAuthServerException::invalidClient($request);
}
$redirectUri = $this->getQueryStringParameter('redirect_uri', $request); $redirectUri = $this->getQueryStringParameter('redirect_uri', $request);