[13.x] Enhance error responses (#1791)

* enhance error responses * add feature tests * formatting
laravel · Oct 3, 2024 · d80b6dd · d80b6dd
1 parent 0c33c4c
commit d80b6dd
Show file tree

Hide file tree

Showing 13 changed files with 713 additions and 115 deletions.
diff --git a/database/factories/ClientFactory.php b/database/factories/ClientFactory.php
@@ -38,6 +38,16 @@ public function definition(): array
         ];
     }
 
+    /**
+     * Use as a public client.
+     */
+    public function asPublic(): static
+    {
+        return $this->state([
+            'secret' => null,
+        ]);
+    }
+
     /**
      * Use as a Password client.
      */
@@ -67,6 +77,7 @@ public function asImplicitClient(): static
     {
         return $this->state([
             'grant_types' => ['implicit'],
+            'secret' => null,
         ]);
     }
 

diff --git a/src/Exceptions/OAuthServerException.php b/src/Exceptions/OAuthServerException.php
@@ -3,16 +3,65 @@
 namespace Laravel\Passport\Exceptions;
 
 use Illuminate\Http\Exceptions\HttpResponseException;
-use Illuminate\Http\Response;
+use Illuminate\Support\Arr;
+use Laravel\Passport\Http\Controllers\ConvertsPsrResponses;
 use League\OAuth2\Server\Exception\OAuthServerException as LeagueException;
+use League\OAuth2\Server\RequestTypes\AuthorizationRequestInterface;
+use Nyholm\Psr7\Response as Psr7Response;
 
 class OAuthServerException extends HttpResponseException
 {
+    use ConvertsPsrResponses;
+
     /**
      * Create a new OAuthServerException.
      */
-    public function __construct(LeagueException $e, Response $response)
+    public function __construct(LeagueException $e, bool $useFragment = false)
+    {
+        parent::__construct($this->convertResponse($e->generateHttpResponse(new Psr7Response, $useFragment)), $e);
+    }
+
+    /**
+     * Create a new OAuthServerException for when login is required.
+     */
+    public static function loginRequired(AuthorizationRequestInterface $authRequest): static
     {
-        parent::__construct($response, $e);
+        $exception = new LeagueException(
+            'The authorization server requires end-user authentication.',
+            9,
+            'login_required',
+            401,
+            'The user is not authenticated',
+            $authRequest->getRedirectUri() ?? Arr::wrap($authRequest->getClient()->getRedirectUri())[0]
+        );
+
+        $exception->setPayload([
+            'state' => $authRequest->getState(),
+            ...$exception->getPayload(),
+        ]);
+
+        return new static($exception, $authRequest->getGrantTypeId() === 'implicit');
+    }
+
+    /**
+     * Create a new OAuthServerException for when consent is required.
+     */
+    public static function consentRequired(AuthorizationRequestInterface $authRequest): static
+    {
+        $exception = new LeagueException(
+            'The authorization server requires end-user consent.',
+            9,
+            'consent_required',
+            401,
+            null,
+            $authRequest->getRedirectUri() ?? Arr::wrap($authRequest->getClient()->getRedirectUri())[0]
+        );
+
+        $exception->setPayload([
+            'state' => $authRequest->getState(),
+            ...$exception->getPayload(),
+        ]);
+
+        return new static($exception, $authRequest->getGrantTypeId() === 'implicit');
     }
 }
diff --git a/src/Http/Controllers/ApproveAuthorizationController.php b/src/Http/Controllers/ApproveAuthorizationController.php
@@ -30,6 +30,6 @@ public function approve(Request $request): Response
 
         return $this->withErrorHandling(fn () => $this->convertResponse(
             $this->server->completeAuthorizationRequest($authRequest, new Psr7Response)
-        ));
+        ), $authRequest->getGrantTypeId() === 'implicit');
     }
 }
diff --git a/src/Http/Controllers/AuthorizationController.php b/src/Http/Controllers/AuthorizationController.php
@@ -13,10 +13,10 @@
 use Laravel\Passport\ClientRepository;
 use Laravel\Passport\Contracts\AuthorizationViewResponse;
 use Laravel\Passport\Exceptions\AuthenticationException;
+use Laravel\Passport\Exceptions\OAuthServerException;
 use Laravel\Passport\Passport;
 use League\OAuth2\Server\AuthorizationServer;
 use League\OAuth2\Server\Entities\ScopeEntityInterface;
-use League\OAuth2\Server\Exception\OAuthServerException;
 use League\OAuth2\Server\RequestTypes\AuthorizationRequestInterface;
 use Nyholm\Psr7\Response as Psr7Response;
 use Psr\Http\Message\ServerRequestInterface;
@@ -41,14 +41,15 @@ public function __construct(
      */
     public function authorize(ServerRequestInterface $psrRequest, Request $request): Response|AuthorizationViewResponse
     {
-        $authRequest = $this->withErrorHandling(fn () => $this->server->validateAuthorizationRequest($psrRequest));
+        $authRequest = $this->withErrorHandling(
+            fn () => $this->server->validateAuthorizationRequest($psrRequest),
+            ($psrRequest->getQueryParams()['response_type'] ?? null) === 'token'
+        );
 
         if ($this->guard->guest()) {
-            if ($request->get('prompt') === 'none') {
-                return $this->denyRequest($authRequest);
-            }
-
-            $this->promptForLogin($request);
+            $request->get('prompt') === 'none'
+                ? throw OAuthServerException::loginRequired($authRequest)
+                : $this->promptForLogin($request);
         }
 
         if ($request->get('prompt') === 'login' &&
@@ -62,17 +63,19 @@ public function authorize(ServerRequestInterface $psrRequest, Request $request):
 
         $request->session()->forget('promptedForLogin');
 
-        $scopes = $this->parseScopes($authRequest);
         $user = $this->guard->user();
+        $authRequest->setUser(new User($user->getAuthIdentifier()));
+
+        $scopes = $this->parseScopes($authRequest);
         $client = $this->clients->find($authRequest->getClient()->getIdentifier());
 
         if ($request->get('prompt') !== 'consent' &&
             ($client->skipsAuthorization($user, $scopes) || $this->hasGrantedScopes($user, $client, $scopes))) {
-            return $this->approveRequest($authRequest, $user);
+            return $this->approveRequest($authRequest);
         }
 
         if ($request->get('prompt') === 'none') {
-            return $this->denyRequest($authRequest, $user);
+            throw OAuthServerException::consentRequired($authRequest);
         }
 
         $request->session()->put('authToken', $authToken = Str::random());
@@ -121,44 +124,13 @@ protected function hasGrantedScopes(Authenticatable $user, Client $client, array
     /**
      * Approve the authorization request.
      */
-    protected function approveRequest(AuthorizationRequestInterface $authRequest, Authenticatable $user): Response
+    protected function approveRequest(AuthorizationRequestInterface $authRequest): Response
     {
-        $authRequest->setUser(new User($user->getAuthIdentifier()));
-
         $authRequest->setAuthorizationApproved(true);
 
         return $this->withErrorHandling(fn () => $this->convertResponse(
             $this->server->completeAuthorizationRequest($authRequest, new Psr7Response)
-        ));
-    }
-
-    /**
-     * Deny the authorization request.
-     */
-    protected function denyRequest(AuthorizationRequestInterface $authRequest, ?Authenticatable $user = null): Response
-    {
-        if (is_null($user)) {
-            $uri = $authRequest->getRedirectUri()
-                ?? (is_array($authRequest->getClient()->getRedirectUri())
-                    ? $authRequest->getClient()->getRedirectUri()[0]
-                    : $authRequest->getClient()->getRedirectUri());
-
-            $separator = $authRequest->getGrantTypeId() === 'implicit' ? '#' : '?';
-
-            $uri = $uri.(str_contains($uri, $separator) ? '&' : $separator).'state='.$authRequest->getState();
-
-            return $this->withErrorHandling(function () use ($uri) {
-                throw OAuthServerException::accessDenied('Unauthenticated', $uri);
-            });
-        }
-
-        $authRequest->setUser(new User($user->getAuthIdentifier()));
-
-        $authRequest->setAuthorizationApproved(false);
-
-        return $this->withErrorHandling(fn () => $this->convertResponse(
-            $this->server->completeAuthorizationRequest($authRequest, new Psr7Response)
-        ));
+        ), $authRequest->getGrantTypeId() === 'implicit');
     }
 
     /**

diff --git a/src/Http/Controllers/DenyAuthorizationController.php b/src/Http/Controllers/DenyAuthorizationController.php
@@ -30,6 +30,6 @@ public function deny(Request $request): Response
 
         return $this->withErrorHandling(fn () => $this->convertResponse(
             $this->server->completeAuthorizationRequest($authRequest, new Psr7Response)
-        ));
+        ), $authRequest->getGrantTypeId() === 'implicit');
     }
 }
diff --git a/src/Http/Controllers/HandlesOAuthErrors.php b/src/Http/Controllers/HandlesOAuthErrors.php
@@ -5,12 +5,9 @@
 use Closure;
 use Laravel\Passport\Exceptions\OAuthServerException;
 use League\OAuth2\Server\Exception\OAuthServerException as LeagueException;
-use Nyholm\Psr7\Response as Psr7Response;
 
 trait HandlesOAuthErrors
 {
-    use ConvertsPsrResponses;
-
     /**
      * Perform the given callback with exception handling.
      *
@@ -21,15 +18,12 @@ trait HandlesOAuthErrors
      *
      * @throws \Laravel\Passport\Exceptions\OAuthServerException
      */
-    protected function withErrorHandling(Closure $callback)
+    protected function withErrorHandling(Closure $callback, bool $useFragment = false)
     {
         try {
             return $callback();
         } catch (LeagueException $e) {
-            throw new OAuthServerException(
-                $e,
-                $this->convertResponse($e->generateHttpResponse(new Psr7Response))
-            );
+            throw new OAuthServerException($e, $useFragment);
         }
     }
 }
diff --git a/src/Http/Controllers/RetrievesAuthRequestFromSession.php b/src/Http/Controllers/RetrievesAuthRequestFromSession.php
@@ -4,7 +4,6 @@
 
 use Exception;
 use Illuminate\Http\Request;
-use Laravel\Passport\Bridge\User;
 use Laravel\Passport\Exceptions\InvalidAuthTokenException;
 use League\OAuth2\Server\RequestTypes\AuthorizationRequest;
 
@@ -18,18 +17,14 @@ trait RetrievesAuthRequestFromSession
      */
     protected function getAuthRequestFromSession(Request $request): AuthorizationRequest
     {
-        if ($request->isNotFilled('auth_token') || $request->session()->pull('authToken') !== $request->get('auth_token')) {
+        if ($request->isNotFilled('auth_token') ||
+            $request->session()->pull('authToken') !== $request->get('auth_token')) {
             $request->session()->forget(['authToken', 'authRequest']);
 
             throw InvalidAuthTokenException::different();
         }
 
-        return tap($request->session()->pull('authRequest'), function ($authRequest) use ($request) {
-            if (! $authRequest) {
-                throw new Exception('Authorization request was not present in the session.');
-            }
-
-            $authRequest->setUser(new User($request->user()->getAuthIdentifier()));
-        });
+        return $request->session()->pull('authRequest')
+            ?? throw new Exception('Authorization request was not present in the session.');
     }
 }