Use a renderable exception for OAuth errors

laravel · Aug 13, 2019 · b3d9f05 · b3d9f05
1 parent b83ca3c
commit b3d9f05
Show file tree

Hide file tree

Showing 10 changed files with 216 additions and 181 deletions.
diff --git a/src/Exceptions/OAuthServerException.php b/src/Exceptions/OAuthServerException.php
@@ -0,0 +1,42 @@
+<?php
+
+namespace Laravel\Passport\Exceptions;
+
+use Exception;
+use Illuminate\Http\Response;
+use League\OAuth2\Server\Exception\OAuthServerException as LeagueException;
+
+class OAuthServerException extends Exception
+{
+    /**
+     * The response to render.
+     *
+     * @var \Illuminate\Http\Response
+     */
+    protected $response;
+
+    /**
+     * Create a new OAuthServerException.
+     *
+     * @param LeagueException $e
+     * @param Response        $response
+     * @return void
+     */
+    public function __construct(LeagueException $e, Response $response)
+    {
+        parent::__construct($e->getMessage(), $e->getCode(), $e);
+
+        $this->response = $response;
+    }
+
+    /**
+     * Render the exception into an HTTP response.
+     *
+     * @param  \Illuminate\Http\Request  $request
+     * @return \Illuminate\Http\Response
+     */
+    public function render($request)
+    {
+        return $this->response;
+    }
+}
diff --git a/src/Http/Controllers/AccessTokenController.php b/src/Http/Controllers/AccessTokenController.php
@@ -10,7 +10,7 @@
 
 class AccessTokenController
 {
-    use ConvertsPsrResponses;
+    use HandlesOAuthErrors;
 
     /**
      * The authorization server.
@@ -58,8 +58,10 @@ public function __construct(AuthorizationServer $server,
      */
     public function issueToken(ServerRequestInterface $request)
     {
-        return $this->convertResponse(
-            $this->server->respondToAccessTokenRequest($request, new Psr7Response)
-        );
+        return $this->withErrorHandling(function () use ($request) {
+            return $this->convertResponse(
+                $this->server->respondToAccessTokenRequest($request, new Psr7Response)
+            );
+        });
     }
 }
diff --git a/src/Http/Controllers/AuthorizationController.php b/src/Http/Controllers/AuthorizationController.php
@@ -14,7 +14,7 @@
 
 class AuthorizationController
 {
-    use ConvertsPsrResponses;
+    use HandlesOAuthErrors;
 
     /**
      * The authorization server.
@@ -57,7 +57,9 @@ public function authorize(ServerRequestInterface $psrRequest,
                               ClientRepository $clients,
                               TokenRepository $tokens)
     {
-        $authRequest = $this->server->validateAuthorizationRequest($psrRequest);
+        $authRequest = $this->withErrorHandling(function () use ($psrRequest) {
+            return $this->server->validateAuthorizationRequest($psrRequest);
+        });
 
         $scopes = $this->parseScopes($authRequest);
 
@@ -109,8 +111,10 @@ protected function approveRequest($authRequest, $user)
 
         $authRequest->setAuthorizationApproved(true);
 
-        return $this->convertResponse(
-            $this->server->completeAuthorizationRequest($authRequest, new Psr7Response)
-        );
+        return $this->withErrorHandling(function () use ($authRequest) {
+            return $this->convertResponse(
+                $this->server->completeAuthorizationRequest($authRequest, new Psr7Response)
+            );
+        });
     }
 }
diff --git a/src/Http/Controllers/HandlesOAuthErrors.php b/src/Http/Controllers/HandlesOAuthErrors.php
@@ -0,0 +1,32 @@
+<?php
+
+namespace Laravel\Passport\Http\Controllers;
+
+use Zend\Diactoros\Response as Psr7Response;
+use Laravel\Passport\Exceptions\OAuthServerException;
+use League\OAuth2\Server\Exception\OAuthServerException as LeagueException;
+
+trait HandlesOAuthErrors
+{
+    use ConvertsPsrResponses;
+
+    /**
+     * Perform the given callback with exception handling.
+     *
+     * @param  \Closure  $callback
+     * @return mixed
+     *
+     * @throws \Laravel\Passport\Exceptions\OAuthServerException
+     */
+    protected function withErrorHandling($callback)
+    {
+        try {
+            return $callback();
+        } catch (LeagueException $e) {
+            throw new OAuthServerException(
+                $e,
+                $this->convertResponse($e->generateHttpResponse(new Psr7Response))
+            );
+        }
+    }
+}
diff --git a/src/Http/Middleware/HandleOAuthErrors.php b/src/Http/Middleware/HandleOAuthErrors.php
diff --git a/src/RouteRegistrar.php b/src/RouteRegistrar.php
@@ -3,7 +3,6 @@
 namespace Laravel\Passport;
 
 use Illuminate\Contracts\Routing\Registrar as Router;
-use Laravel\Passport\Http\Middleware\HandleOAuthErrors;
 
 class RouteRegistrar
 {
@@ -50,13 +49,11 @@ public function forAuthorization()
             $router->get('/authorize', [
                 'uses' => 'AuthorizationController@authorize',
                 'as' => 'passport.authorizations.authorize',
-                'middleware' => [HandleOAuthErrors::class],
             ]);
 
             $router->post('/authorize', [
                 'uses' => 'ApproveAuthorizationController@approve',
                 'as' => 'passport.authorizations.approve',
-                'middleware' => [HandleOAuthErrors::class],
             ]);
 
             $router->delete('/authorize', [
@@ -76,7 +73,7 @@ public function forAccessTokens()
         $this->router->post('/token', [
             'uses' => 'AccessTokenController@issueToken',
             'as' => 'passport.token',
-            'middleware' => ['throttle', HandleOAuthErrors::class],
+            'middleware' => 'throttle',
         ]);
 
         $this->router->group(['middleware' => ['web', 'auth']], function ($router) {

diff --git a/tests/AccessTokenControllerTest.php b/tests/AccessTokenControllerTest.php
@@ -6,19 +6,19 @@
 use Lcobucci\JWT\Parser;
 use Zend\Diactoros\Response;
 use PHPUnit\Framework\TestCase;
-use Illuminate\Container\Container;
 use Laravel\Passport\TokenRepository;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use League\OAuth2\Server\AuthorizationServer;
+use Laravel\Passport\Exceptions\OAuthServerException;
 use Laravel\Passport\Http\Controllers\AccessTokenController;
+use League\OAuth2\Server\Exception\OAuthServerException as LeagueException;
 
 class AccessTokenControllerTest extends TestCase
 {
     public function tearDown()
     {
         m::close();
-        Container::getInstance()->flush();
     }
 
     public function test_a_token_can_be_issued()
@@ -40,6 +40,23 @@ public function test_a_token_can_be_issued()
 
         $this->assertEquals('{"access_token":"access-token"}', $controller->issueToken($request)->getContent());
     }
+
+    public function test_exceptions_are_handled()
+    {
+        $tokens = m::mock(TokenRepository::class);
+        $jwt = m::mock(Parser::class);
+
+        $server = m::mock(AuthorizationServer::class);
+        $server->shouldReceive('respondToAccessTokenRequest')->with(
+            m::type(ServerRequestInterface::class), m::type(ResponseInterface::class)
+        )->andThrow(LeagueException::invalidCredentials());
+
+        $controller = new AccessTokenController($server, $tokens, $jwt);
+
+        $this->expectException(OAuthServerException::class);
+
+        $controller->issueToken(m::mock(ServerRequestInterface::class));
+    }
 }
 
 class AccessTokenControllerTestStubToken

diff --git a/tests/AuthorizationControllerTest.php b/tests/AuthorizationControllerTest.php
@@ -10,22 +10,22 @@
 use Laravel\Passport\Passport;
 use PHPUnit\Framework\TestCase;
 use Laravel\Passport\Bridge\Scope;
-use Illuminate\Container\Container;
 use Laravel\Passport\TokenRepository;
 use Laravel\Passport\ClientRepository;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use League\OAuth2\Server\AuthorizationServer;
 use Illuminate\Contracts\Routing\ResponseFactory;
+use Laravel\Passport\Exceptions\OAuthServerException;
 use League\OAuth2\Server\RequestTypes\AuthorizationRequest;
 use Laravel\Passport\Http\Controllers\AuthorizationController;
+use League\OAuth2\Server\Exception\OAuthServerException as LeagueException;
 
 class AuthorizationControllerTest extends TestCase
 {
     public function tearDown()
     {
         m::close();
-        Container::getInstance()->flush();
     }
 
     public function test_authorization_view_is_presented()
@@ -71,6 +71,28 @@ public function test_authorization_view_is_presented()
         ));
     }
 
+    public function test_authorization_exceptions_are_handled()
+    {
+        $server = m::mock(AuthorizationServer::class);
+        $response = m::mock(ResponseFactory::class);
+
+        $controller = new AuthorizationController($server, $response);
+
+        $server->shouldReceive('validateAuthorizationRequest')->andThrow(LeagueException::invalidCredentials());
+
+        $request = m::mock(Request::class);
+        $request->shouldReceive('session')->andReturn($session = m::mock());
+
+        $clients = m::mock(ClientRepository::class);
+        $tokens = m::mock(TokenRepository::class);
+
+        $this->expectException(OAuthServerException::class);
+
+        $controller->authorize(
+            m::mock(ServerRequestInterface::class), $request, $clients, $tokens
+        );
+    }
+
     public function test_request_is_approved_if_valid_token_exists()
     {
         Passport::tokensCan([