Skip to content
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

Improve AuthorizationsController error response handling #1676

Merged

Conversation

camero2734
Copy link
Contributor

@camero2734 camero2734 commented Nov 7, 2023

Summary

Hey, first time contributing, hope this is welcome

This PR has three changes to AuthorizationsController's error response, explained in more detail below:

  • Returns the proper status code instead of 200
  • Redirects back to the client with error (if redirectable?)
  • Honors the raise_on_errors? setting

I noticed that Doorkeeper returns a 200 response code when an error occurs, and also does not redirect back to the client on errors as my understanding of the spec indicates it should:

https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2.1

Non-redirecting

If the request fails due to a missing, invalid, or mismatching
redirection URI, or if the client identifier is missing or invalid,
the authorization server SHOULD inform the resource owner of the
error and MUST NOT automatically redirect the user-agent to the
invalid redirection URI.

Redirecting

If the resource owner denies the access request or if the request
fails for reasons other than a missing or invalid redirection URI,
the authorization server informs the client by adding the following
parameters to the query component of the redirection URI using the
"application/x-www-form-urlencoded" format

There is already the concept of redirectable? in Doorkeeper which adheres to this, but it seems like this isn't actually used for errors in authorizations_controller.

The status code change is applied normally, but I feel like the redirection bit is probably a breaking change since it adds redirects where there wasn't any before, so I added it as a config option via handle_auth_errors :redirect

Fixes #1643

@camero2734 camero2734 marked this pull request as ready for review November 7, 2023 21:17
Copy link
Member

@nbulaj nbulaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I like it 👍

@nbulaj
Copy link
Member

nbulaj commented Nov 8, 2023

Also can you please add a changelog entry?

@camero2734 camero2734 changed the title Improve AuthorizationController error response handling Improve AuthorizationsController error response handling Nov 8, 2023
#
# This will have no effect if handle_auth_errors is set to :raise.
#
# redirect_on_error
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe this should just be configured as handle_auth_errors :redirect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I went ahead and did this since I think it makes more sense and it cleaner this way 👍

Copy link
Member

@nbulaj nbulaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@nbulaj nbulaj merged commit 9fc81d5 into doorkeeper-gem:main Nov 9, 2023
20 checks passed
@camero2734 camero2734 deleted the authorizations-controller-response branch November 9, 2023 09:36
@camero2734
Copy link
Contributor Author

Hey @nbulaj, any plans for when the next release will be? We'd like to be able to replace our local monkeypatch with this change. Happy to help out in any way to get it released!

stanhu added a commit to stanhu/doorkeeper-openid_connect that referenced this pull request Nov 22, 2024
doorkeeper-gem/doorkeeper#1676 updated
the `render_error` to return a 400 Bad Request upon an error
instead of a 200. Adjust the spec accordingly.
stanhu added a commit to stanhu/doorkeeper-openid_connect that referenced this pull request Nov 22, 2024
doorkeeper-gem/doorkeeper#1676 updated
the `render_error` to return a 400 Bad Request upon an error
instead of a 200. Adjust the spec accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AuthorizationsController errors don't adhere to the "handle_auth_errors :raise" config
2 participants