diff --git a/src/Exceptions/OAuthServerException.php b/src/Exceptions/OAuthServerException.php new file mode 100644 index 000000000..3c9ec70e8 --- /dev/null +++ b/src/Exceptions/OAuthServerException.php @@ -0,0 +1,42 @@ +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 index 53cfed8c0..255646b9b 100644 --- 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 index fb305dab4..a51a0fc02 100644 --- 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 new file mode 100644 index 000000000..e7881d7ec --- /dev/null +++ b/src/Http/Controllers/HandlesOAuthErrors.php @@ -0,0 +1,32 @@ +convertResponse($e->generateHttpResponse(new Psr7Response)) + ); + } + } +} diff --git a/src/Http/Middleware/HandleOAuthErrors.php b/src/Http/Middleware/HandleOAuthErrors.php deleted file mode 100644 index 92ccbc3dc..000000000 --- a/src/Http/Middleware/HandleOAuthErrors.php +++ /dev/null @@ -1,74 +0,0 @@ -config = $config; - $this->exceptionHandler = $exceptionHandler; - } - - /** - * Handle an incoming request. - * - * @param \Illuminate\Http\Request $request - * @param \Closure $next - * @return mixed - */ - public function handle($request, Closure $next) - { - try { - return $next($request); - } catch (OAuthServerException $e) { - $this->exceptionHandler->report($e); - - return $this->convertResponse( - $e->generateHttpResponse(new Psr7Response) - ); - } catch (Exception $e) { - $this->exceptionHandler->report($e); - - return new Response($this->config->get('app.debug') ? $e->getMessage() : 'Error.', 500); - } catch (Throwable $e) { - $this->exceptionHandler->report(new FatalThrowableError($e)); - - return new Response($this->config->get('app.debug') ? $e->getMessage() : 'Error.', 500); - } - } -} diff --git a/src/RouteRegistrar.php b/src/RouteRegistrar.php index a9e5832df..12ab3d74e 100644 --- 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 index 5dcb21f0c..cd96a582a 100644 --- 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 index 0e593919e..9dfd5abf5 100644 --- 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([ diff --git a/tests/HandleOAuthErrorsTest.php b/tests/HandleOAuthErrorsTest.php deleted file mode 100644 index e82de9bc7..000000000 --- a/tests/HandleOAuthErrorsTest.php +++ /dev/null @@ -1,90 +0,0 @@ -middleware(); - $response = new Response; - - $result = $middleware->handle(new Request, function () use ($response) { - return $response; - }); - - $this->assertSame($response, $result); - } - - public function test_it_handles_oauth_server_exceptions() - { - $exception = new OAuthServerException('Error', 1, 'fatal'); - $middleware = $this->middleware($exception); - - $result = $middleware->handle(new Request, function () use ($exception) { - throw $exception; - }); - - $this->assertInstanceOf(Response::class, $result); - $this->assertJsonStringEqualsJsonString('{"error":"fatal","error_description":"Error","message":"Error"}', $result->content()); - } - - public function test_it_handles_other_exceptions() - { - $exception = new RuntimeException('Exception occurred', 1); - $middleware = $this->middleware($exception, $mockDebugReturn = true); - - $result = $middleware->handle(new Request, function () use ($exception) { - throw $exception; - }); - - $this->assertInstanceOf(Response::class, $result); - $this->assertSame('Exception occurred', $result->content()); - } - - public function test_it_handles_throwables() - { - $exception = new Error('Fatal Error', 1); - $middleware = $this->middleware(m::type(FatalThrowableError::class), $mockDebugReturn = true); - - $result = $middleware->handle(new Request, function () use ($exception) { - throw $exception; - }); - - $this->assertInstanceOf(Response::class, $result); - $this->assertSame('Fatal Error', $result->content()); - } - - private function middleware($expectedException = null, bool $mockDebugReturn = false): HandleOAuthErrors - { - $config = m::mock(Repository::class); - $exceptionHandler = m::mock(ExceptionHandler::class); - - if ($mockDebugReturn) { - $config->shouldReceive('get')->with('app.debug')->once()->andReturn(true); - } - - if ($expectedException) { - $exceptionHandler->shouldReceive('report')->once()->with($expectedException); - } - - return new HandleOAuthErrors($config, $exceptionHandler); - } -} diff --git a/tests/HandlesOAuthErrorsTest.php b/tests/HandlesOAuthErrorsTest.php new file mode 100644 index 000000000..fbcebbfaf --- /dev/null +++ b/tests/HandlesOAuthErrorsTest.php @@ -0,0 +1,83 @@ +test(function () use ($response) { + return $response; + }); + + $this->assertSame($response, $result); + } + + public function testShouldHandleOAuthServerException() + { + $controller = new HandlesOAuthErrorsStubController; + + $exception = new LeagueException('Error', 1, 'fatal'); + + $e = null; + + try { + $controller->test(function () use ($exception) { + throw $exception; + }); + } catch (OAuthServerException $e) { + $e = $e; + } + + $this->assertInstanceOf(OAuthServerException::class, $e); + $this->assertEquals('Error', $e->getMessage()); + $this->assertInstanceOf(LeagueException::class, $e->getPrevious()); + + $response = $e->render(new Request); + + $this->assertJsonStringEqualsJsonString( + '{"error":"fatal","error_description":"Error","message":"Error"}', + $response->getContent() + ); + } + + public function testShouldIgnoreOtherExceptions() + { + $controller = new HandlesOAuthErrorsStubController; + + $exception = new RuntimeException('Exception occurred', 1); + + $this->expectException(RuntimeException::class); + + $controller->test(function () use ($exception) { + throw $exception; + }); + } +} + +class HandlesOAuthErrorsStubController +{ + use HandlesOAuthErrors; + + public function test($callback) + { + return $this->withErrorHandling($callback); + } +}