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

Refactor: move response_type check in pre_authorization to a method to be easily to override #1326

Conversation

linhdangduy
Copy link
Contributor

Summary

This pull request refactors error_response method of pre_authorization in authorization request.

In Oauth2, response_on_fragment=true when response_type=token (which is implicit flow).
But OIDC introduces other response_type which also define that response_on_fragment=true (response_type = id_token or id_token token...)

I move the response_type check to response_on_fragment? method, so it could be easily override this behavior like:

def response_on_fragment?
  super || response_type == "id_token"
end

References:

https://openid.net/specs/openid-connect-core-1_0.html#ImplicitAuthRequest

@linhdangduy linhdangduy changed the title Refactor: move response_type check to a method to be easily to override Refactor: move response_type check in pre_authorization to a method to be easily to override Nov 13, 2019
@linhdangduy linhdangduy force-pushed the check_response_type_in_another_method_to_be_override branch from 99341e3 to 9837699 Compare November 13, 2019 03:08
@doorkeeper-bot
Copy link

1 Warning
⚠️ There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 Danger

@linhdangduy
Copy link
Contributor Author

linhdangduy commented Nov 14, 2019

Haven't found out where to fix to avoid the coverage decreased error. 👀
I think this pull is just a refactoring work.

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
Copy link
Member

nbulaj commented Nov 14, 2019

Agree @linhdangduy

@nbulaj nbulaj merged commit ad68a38 into doorkeeper-gem:master Nov 14, 2019
@linhdangduy linhdangduy deleted the check_response_type_in_another_method_to_be_override branch March 20, 2021 02:40
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.

4 participants