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.

Sign up for GitHub

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

Jump to bottom

Allows ui_locales, claims_locales and login_hint as request params #6

Merged
merged 2 commits into from
Feb 2, 2019
Merged

Allows ui_locales, claims_locales and login_hint as request params #6

merged 2 commits into from
Feb 2, 2019

Conversation

yannvery
Copy link
Contributor

Hello,

These params are allowed to be used during Authentication request.
Ref: http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

I removed the login_hint option, the param has to be dynamic. I don't understand why it should be declared as an option.

Please let me know if I misunderstood something.

ghost
ghost reviewed Aug 24, 2018
View reviewed changes
test/lib/omniauth/strategies/openid_connect_test.rb
@@ -18,6 +18,16 @@ def test_request_phase
strategy.request_phase
end

def test_request_phase_with_params
expected_redirect = /^https:\/\/example\.com\/authorize\?claims_locales=es&client_id=1234&login_hint=john.doe%40example.com&nonce=\w{32}&response_type=code&scope=openid&state=\w{32}&ui_locales=en$/
Copy link

Choose a reason for hiding this comment

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

Line is too long. [205/80]

test/lib/omniauth/strategies/openid_connect_test.rb
expected_redirect = /^https:\/\/example\.com\/authorize\?claims_locales=es&client_id=1234&login_hint=john.doe%40example.com&nonce=\w{32}&response_type=code&scope=openid&state=\w{32}&ui_locales=en$/
strategy.options.issuer = 'example.com'
strategy.options.client_options.host = 'example.com'
request.stubs(:params).returns('login_hint' => 'john.doe@example.com', 'ui_locales' => 'en', 'claims_locales' => 'es')
Copy link

Choose a reason for hiding this comment

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

Line is too long. [126/80]

m0n9oose
m0n9oose requested changes Jan 4, 2019
View reviewed changes
Copy link
Collaborator

@m0n9oose m0n9oose left a comment

Choose a reason for hiding this comment

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

Hey @yannvery

Thanks a lot for your contribution and sorry for delayed response, not sure why i missed the notifications.

Please consider fixing of few minor comments and make sure all test are passed.

Btw i have noticed one more similar PR #8. Could we merge them?

lib/omniauth/strategies/openid_connect.rb Outdated
@@ -235,6 +236,10 @@ def redirect_uri
"#{ client_options.redirect_uri }?redirect_uri=#{ CGI.escape(request.params['redirect_uri']) }"
end

def params
request.params
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using delegation instead of separate method definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to use Forwardable module, and I also replace all request.params calls.
Tell me If you prefer another solution.

@yannvery yannvery force-pushed the authentication-params branch from d0608fe to 0e42206 Compare January 17, 2019 14:14
ghost
ghost reviewed Jan 17, 2019
View reviewed changes
test/lib/omniauth/strategies/openid_connect_test.rb
@@ -18,6 +18,16 @@ def test_request_phase
strategy.request_phase
end

def test_request_phase_with_params
expected_redirect = /^https:\/\/example\.com\/authorize\?claims_locales=es&client_id=1234&login_hint=john.doe%40example.com&nonce=\w{32}&response_type=code&scope=openid&state=\w{32}&ui_locales=en$/
Copy link

Choose a reason for hiding this comment

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

Use %r around regular expression.

test/lib/omniauth/strategies/openid_connect_test.rb
@@ -18,6 +18,16 @@ def test_request_phase
strategy.request_phase
end

def test_request_phase_with_params
expected_redirect = /^https:\/\/example\.com\/authorize\?claims_locales=es&client_id=1234&login_hint=john.doe%40example.com&nonce=\w{32}&response_type=code&scope=openid&state=\w{32}&ui_locales=en$/
Copy link

Choose a reason for hiding this comment

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

Line is too long. [205/80]

test/lib/omniauth/strategies/openid_connect_test.rb
expected_redirect = /^https:\/\/example\.com\/authorize\?claims_locales=es&client_id=1234&login_hint=john.doe%40example.com&nonce=\w{32}&response_type=code&scope=openid&state=\w{32}&ui_locales=en$/
strategy.options.issuer = 'example.com'
strategy.options.client_options.host = 'example.com'
request.stubs(:params).returns('login_hint' => 'john.doe@example.com', 'ui_locales' => 'en', 'claims_locales' => 'es')
Copy link

Choose a reason for hiding this comment

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

Line is too long. [126/80]

lib/omniauth/strategies/openid_connect.rb
if error
raise CallbackError.new(request.params['error'], request.params['error_description'] || request.params['error_reason'], request.params['error_uri'])
elsif request.params['state'].to_s.empty? || request.params['state'] != stored_state
raise CallbackError.new(params['error'], params['error_description'] || params['error_reason'], params['error_uri'])
Copy link

Choose a reason for hiding this comment

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

Line is too long. [126/80]

lib/omniauth/strategies/openid_connect.rb
elsif !request.params['code']
return fail!(:missing_code, OmniAuth::OpenIDConnect::MissingCodeError.new(request.params['error']))
elsif !params['code']
return fail!(:missing_code, OmniAuth::OpenIDConnect::MissingCodeError.new(params['error']))
Copy link

Choose a reason for hiding this comment

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

Line is too long. [101/80]

lib/omniauth/strategies/openid_connect.rb
@@ -231,10 +236,11 @@ def decode(str)
end

def redirect_uri
return client_options.redirect_uri unless request.params['redirect_uri']
"#{ client_options.redirect_uri }?redirect_uri=#{ CGI.escape(request.params['redirect_uri']) }"
return client_options.redirect_uri unless params['redirect_uri']
Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

lib/omniauth/strategies/openid_connect.rb
return client_options.redirect_uri unless request.params['redirect_uri']
"#{ client_options.redirect_uri }?redirect_uri=#{ CGI.escape(request.params['redirect_uri']) }"
return client_options.redirect_uri unless params['redirect_uri']
"#{ client_options.redirect_uri }?redirect_uri=#{ CGI.escape(params['redirect_uri']) }"
Copy link

Choose a reason for hiding this comment

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

Space inside string interpolation detected.

lib/omniauth/strategies/openid_connect.rb
return client_options.redirect_uri unless request.params['redirect_uri']
"#{ client_options.redirect_uri }?redirect_uri=#{ CGI.escape(request.params['redirect_uri']) }"
return client_options.redirect_uri unless params['redirect_uri']
"#{ client_options.redirect_uri }?redirect_uri=#{ CGI.escape(params['redirect_uri']) }"
Copy link

Choose a reason for hiding this comment

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

Line is too long. [95/80]

lib/omniauth/strategies/openid_connect.rb Outdated Show resolved Hide resolved
@yannvery yannvery force-pushed the authentication-params branch from 0e42206 to f3e706a Compare January 17, 2019 14:22
@yannvery
Copy link
Contributor Author

Thanks for your feedback @m0n9oose.
Do you want that I try to fix all the issues pointed by the bot?

@m0n9oose
Copy link
Collaborator

Do you want that I try to fix all the issues pointed by the bot?

totally up to you.

What about test? Is everything okay?

@m0n9oose m0n9oose mentioned this pull request Jan 17, 2019
Closed
@yannvery
Copy link
Contributor Author

yannvery commented Feb 1, 2019

It seems ok for me.
I won't fix all bot issues at one time, but in further PR.

@m0n9oose m0n9oose merged commit 4d645b2 into omniauth:master Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m0n9oose m0n9oose m0n9oose approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yannvery @m0n9oose