From 27d5c5ed8da27586de511cf40b8a44170a4b3ab7 Mon Sep 17 00:00:00 2001
From: Ian Littman <iansltx@gmail.com>
Date: Sat, 11 May 2019 14:05:39 -0500
Subject: [PATCH] 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);