-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add support for screen_hint=signup param #103
Conversation
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.
@bbean86 something to notice though is that this introduced param will only work with the "New Universal Login Experience". "Classic Experience" didn't support this, last time I checked.
Can you add a note somewhere in the readme for these additional params? (all of possible, they are not many) and for this one in particular, warn that it's only available with the new experience. Thanks
@@ -86,7 +86,7 @@ def client | |||
def authorize_params | |||
params = super | |||
parsed_query = Rack::Utils.parse_query(request.query_string) | |||
%w[connection connection_scope prompt].each do |key| | |||
%w[connection connection_scope prompt screen_hint].each do |key| |
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.
what is this line doing, allowing these keys to be taken from the URL query and setting them on the /authorize call?
@@ -123,6 +123,20 @@ | |||
expect(redirect_url).not_to have_query('connection') | |||
end | |||
|
|||
it 'redirects to hosted login page with screen_hint=signup' do | |||
get 'auth/auth0?screen_hint=signup' |
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.
There's a existing test for checking that on a "normal authorize call" (without extra args) these keys are not part of the query. Let's add this one there, and also the ones that are missing from previous changes.
The test is on these lines:
omniauth-auth0/spec/omniauth/strategies/auth0_spec.rb
Lines 84 to 86 in 894d4e2
expect(redirect_url).not_to have_query('auth0Client') | |
expect(redirect_url).not_to have_query('connection') | |
expect(redirect_url).not_to have_query('prompt') |
Let's add a line for testing that the following keys are NOT in the query:
- connection_scope
- screen_hint
👋 @bbean86 friendly check-in, are you still interested in these changes? |
@lbalmaceda apologies, I will circle back to these updates this weekend. This update is still relevant to my needs. |
@bbean86 thanks for the update! My team is tracking the pending release internally under |
Changes
This adds support for the Hosted Login
screen_hint=signup
parameter. I discovered this issue while implementing Auth0 in a new Rails 6 app using Hosted Login. I noticed others had the same experience, so I thought I'd share my solution.References
#87
Testing
This can be tested by adding the
screen_hint=signup
query parameter to the/auth/auth0
request. You should be able to observe thescreen_hint
parameter being sent to the Auth0/authorize
endpoint.Checklist