laravel / passport

Laravel Passport provides OAuth2 server support to Laravel.
https://laravel.com/docs/passport
MIT License
3.28k stars 777 forks source link

How to work with OAuth errors #289

Closed lotarbo closed 5 years ago

lotarbo commented 7 years ago

Hi, what is the right way to custom or work with errors? I mean when we have invalidClient error, this auth popup is bad for me, mb better redirect to client site with some message? And how change error? I read http://www.kingpabel.com/oauth2-exception-custom-error-message/ but it works only for clean League\OAuth2, passport has own HandlesOAuthErrors, can somebody help?

sebastiaanluca commented 7 years ago

Running into the same issue. Thought my exception handler was botched, but turns out Passport intercepts all errors and returns a plain text string. That's not really useful, to just output raw exceptions to the user without a chance to format these as a developer?

@themsaid @taylorotwell What's the course here to disable/extend this custom Passport exception handler? Really hitting a brick wall here.

Similar issue: https://github.com/laravel/passport/issues/97

deividaspetraitis commented 7 years ago

Not sure what's wrong to catch and handle these in Hander.php:

     * Determine if the given exception is an OAuth2 exception.
     *
     * @param Exception $e
     *
     * @return bool
     */
    public function isOAuthException(Exception $e)
    {
        return $e instanceof \League\OAuth2\Server\Exception\OAuthServerException
                ||
                $e instanceof AuthenticationException;
    }

In case you want to determine specific exception you can use $e->getErrorType() for more details see OAuthServerException constructor.

EdwardNelson commented 7 years ago

The exceptions do not bubble up to the exception handler. Passport catches them

deividaspetraitis commented 7 years ago

Yea, seems you guys are right.

If I'm correct error handling happens using predefined controllers that calls $this->withErrorHandling() function that refers to HandlesOAuthErrors trait.

Such handling is not useful not only in case when you do need to catch exception but there is some headache when you do need to return these errors in some other format for example JSON, XML, etc.

What I have done so far in order to solve this I just created my own controllers and removed such handling.

That means that you must to:

For example: Please see original AccessTokenController: https://github.com/laravel/passport/blob/2.0/src/Http/Controllers/AccessTokenController.php and my re-defined: https://gist.github.com/deividaspetraitis/7c3958381d33a06b511d17e2e22b8bb5

Following extending ( re-defining ) solves both problems I've mentioned and I think is quite reasonable and simple. Hope it helps.

sebastiaanluca commented 7 years ago

@themsaid @taylorotwell Is there a way to handle this in the app exception handler? Few months later and we still have issues with this. API just returns 500 for any wrong user input (e.g. wrong access token) and the apps that rely on it have no proper way of dealing with it since the returned data is not our proprietary JSON format.


Edit

As per @deividaspetraitis' example, a simplified workaround (no need to redefine routes). Do this for every OAuth controller.

In a service provider's register method (e.g. AppServiceProvider), replace the /oauth/token route controller:

// Bypass Laravel Passport handling exceptions itself by overriding
// each controller that uses the HandlesOAuthErrors trait. Note the
// double backslash prefixed to the class namespace and name in order
// to get an exact match. Won't work otherwise.
// See https://github.com/laravel/passport/issues/289
$this->app->bind('\\' . AccessTokenController::class, \OAuth\Http\Controllers\AccessTokenController::class);

Extend the access token controller:

<?php

namespace OAuth\Http\Controllers;

use GuzzleHttp\Exception\ClientException;
use Laravel\Passport\Http\Controllers\AccessTokenController as PassportAccessTokenController;
use League\OAuth2\Server\Exception\OAuthServerException;
use Psr\Http\Message\ServerRequestInterface;
use Zend\Diactoros\Response as Psr7Response;

class AccessTokenController extends PassportAccessTokenController
{
    /**
     * Authorize a client to access the user's account.
     *
     * @param  ServerRequestInterface $request
     *
     * @return \Psr\Http\Message\ResponseInterface
     * @throws \League\OAuth2\Server\Exception\OAuthServerException
     */
    public function issueToken(ServerRequestInterface $request)
    {
        try {
            return $this->server->respondToAccessTokenRequest($request, new Psr7Response);
        } catch (ClientException $exception) {
            $error = json_decode($exception->getResponse()->getBody());

            throw OAuthServerException::invalidRequest('access_token', object_get($error, 'error.message'));
        }
    }
}

Handle the exception in \App\Exceptions\Handler (simplified code):

protected $dontReport = [
    OAuthServerException::class,
];

/**
 * Render an exception into an HTTP response.
 *
 * @param Request $request
 * @param \Exception $exception
 *
 * @return \Illuminate\Http\Response|\Symfony\Component\HttpFoundation\Response
 */
public function render($request, Exception $exception)
{
    switch (get_class($exception)) {
        case OAuthServerException::class:
            return response()->json([
                'error' => $exception->getErrorType(),
                'message' => $exception->getMessage(),
                'hint' => $exception->getHint(),
            ], $exception->getHttpStatusCode());
    }
}

Important note

Important to remember btw is that you'll need to handle a lot of other exceptions too. Like passing a client ID that's a string or one that's 1 character too long will return a database exception and 500 internal server error. Highly undesirable, so do some validation on the input fields before passing them on to Passport.

psolom commented 7 years ago

+1 It's extreamly problematic to implement custom exception handler for Passport so long as errors are caught in the HandlesOAuthErrors trait

adiachenko commented 7 years ago

I don't think this will work for everyone, but if you're developing an API for first-party clients, you can kinda sweep all that complexity under the rug by utilizing internal requests. Meaning, your client talks to a proxy instead of directly communicating with oauth/token. That way, you can simply throw your own Exception whenever the latter returns anything but a 200, include other resources onto response, inject client credentials automatically etc. Here is the rough outline. Not the most elegant solution, but it's what I've been doing with Passport ever since it was released.

fjzboy commented 6 years ago

maybe, the oauth/token route should not be accessed directly by client, you can define a custom route (ie.'/api/login'), in that route function, you can call oauth/token api, and process result before return a response to your client

webdevog commented 6 years ago

Hello I've also used some workaround solution using solution above. It is not the best way of resolving the issue but maybe for somebody it could be useful:

class AccessTokenController extends PassportAccessTokenController
{
    /**
     * Authorize a client to access the user's account.
     *
     * @param  ServerRequestInterface $request
     *
     * @return \Psr\Http\Message\ResponseInterface
     * @throws \League\OAuth2\Server\Exception\OAuthServerException
     */
    public function issueToken(ServerRequestInterface $request)
    {
        try {
            return $this->convertResponse(
                $this->server->respondToAccessTokenRequest($request, new Psr7Response)
            );
        } catch (OAuthServerException $exception) {
           // your custom logic

            return $this->withErrorHandling(function () use($exception) {
                throw $exception;
            });
        }
    }
}
yushengbuilder commented 6 years ago

@webdevog 我觉得这是一个很好的方式,因为token接口并不是专为单一前端设计的。可能多个前端之间的报错信息需求不是很相同。如果都后端重新定义会很繁琐。而且错误形式很多。

driesvints commented 5 years ago

@rockoo you're free to send in a PR. Eventually I'll get to taking a look at this but I can't do everything at the same time. Would appreciate any help.

driesvints commented 5 years ago

Looking into this atm and from what I understand you can overwrite the convertResponse method atm to customize the error response?

driesvints commented 5 years ago

Maybe a middleware approach would be better for this. Also indeed not sure why regular exceptions and throwables are caught like this.

EdwardNelson commented 5 years ago

Are you planning on a PR?

driesvints commented 5 years ago

I've just sent in a PR which would make handling errors a lot easier. Please take a look at it and respond there if you like the new solution. I've added an example on how you can roll with your own implementation: https://github.com/laravel/passport/pull/937

After working on a solution I think the main reasoning why this is implemented so strictly is because you don't want to "leak" any sensitive information to your consumers of your library if stuff breaks.

Anyway, let me know what you think of the new solution on the PR and we'll merge it in a few days.

Cintrust commented 5 years ago

HAVING THE SAME ISSUE, ANY OFFICIAL FIX YET ??

driesvints commented 5 years ago

@Cintrust please see the above message

driesvints commented 5 years ago

The PR was merged and the refactor will be released with the next major Passport release.

eithed commented 5 years ago

Given that currently there's no way to re-format the exceptions (hopefully this will change soon, as I'd like to rely on #937 instead of below workaround), I've created a middleware that will look at the response and reformat the output. It would be great if exception property was populated on Response so I could rely on instanceof OAuthServerException match, but nope, it's null.

namespace App\Http\Middleware;

use Closure;

class TranslateOAuthException
{
    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Closure  $next
     * @return mixed
     */
    public function handle($request, Closure $next)
    {
        $response = $next($request);

        // we don't have access to exception property on response, so
        // we have to parse the content
        if ($response->status() == 400) {
            $content = $response->getContent();

            $json = json_decode($content);
            if (!empty($json->error) && in_array($json->error, ['invalid_request', 'invalid_grant']) && !empty($json->message) && !empty($json->hint)) {
                $payload = [
                    'error' => $json->error,
                    'error_description' => $json->message." ".$json->hint
                ];

                $response->setContent(json_encode($payload));
            }
        }

        return $response;
    }
}
shorif2000 commented 3 years ago

https://gist.github.com/deividaspetraitis/7c3958381d33a06b511d17e2e22b8bb5

can you provide example of the oauth classes you created

FranciscoCaldeira commented 2 years ago

so much code.. is this implemented? How can I use it?

biodunbamigboye commented 2 months ago

I faced this same issue today, and came out with a very simple though not syntactically elegant solution, but it works like magic Note that my project is running on laravel 8 and might not be able to benefit from most recent fixes

Inside Handler.php add League\OAuth2\Server\Exception\OAuthServerException to $dontReport array then in render method, based on the message Log a custom error ` use League\OAuth2\Server\Exception\OAuthServerException;

protected $dontReport = [
    OAuthServerException::class
];

public function render($request, Throwable $exception)
 {
      if ($exception->getMessage() === 'Unauthenticated.') {
        Log::error('error message, it could be json encoded array, string, or any thing you desire'));
        }
 }

`