-
Notifications
You must be signed in to change notification settings - Fork 786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[8.0] Use a renderable exception for OAuth errors #1066
Conversation
Also rebase once to resolve the conflicts. |
Hey @driesvints, thanks for the review. I made the requested changes and rebased. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, just some DocBlocks and we're good.
@driesvints thanks, I added the docblocks and squashed. |
@matt-allan I'm going to re-open this if that's okay because we'll need this for the next release. Planning to work on Passport 8 in September so I'll take a look at this then. |
Sure, no problem. |
@driesvints I am using passport 11.10.5. Can I still catch and render Passport's Laravel\Passport\Exceptions\OAuthServerException from Handler.php? |
I faced this same issue today, and came out with a very simple though not syntactically elegant solution, but it works like magic Inside Handler.php add League\OAuth2\Server\Exception\OAuthServerException to $dontReport array then in render method, based on the message Log a custom error
` |
This PR updates the Passport controllers to use a renderable exception instead of middleware for rendering OAuth errors.
Originally Passport handled exception reporting and rendering itself, but developers wanted to be able to control the rendering so the behavior was moved to middleware in #937.
The problem with using middleware is the pipeline catches exceptions before they reach middleware (#1062).
The only reason Passport needed to handle exceptions in the first place was to convert the League exception's PSR response to something Laravel can render, so this PR wraps the League exception in a renderable Passport exception.
If the user wants to change how the exception is rendered they can do that in their application's exception handler.
I don't see any reason that Passport needs to override 500 error rendering so these fall through to the application's normal rendering.
Upgrading:
If you are explicitly handling
League\OAuth2\Server\Exception\OAuthServerException
in your exception handler'sreport
method you will need to check for an instance ofLaravel\Passport\Exceptions\OAuthServerException
instead.If for some reason your OAuth client's are relying on 500 errors returning the string
Error.
in the response you will need to override the rendering for/oauth/authorize
and/oauth/token
, i.e.