Feature #4570

[SSO] Implement Google OAuth2 for new SSO installations

Added by Peter Amstutz almost 7 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
11/05/2014
Due date:
% Done:

67%

Estimated time:
(Total: 0.00 h)
Story points:
2.0

Subtasks

Task #4640: Review 4570-multi-auth-methodResolvedPeter Amstutz

Task #4639: Review 4570-support-google-oauth2 in the sso-provider repositoryResolvedWard Vandewege

Task #4419: Support OpenID Connect for new log insResolvedPeter Amstutz


Related issues

Related to Arvados - Bug #3465: [SSO] Research requirements to upgrade SSO codebase to use oauth2 (or openid connect)Resolved11/05/2014

Related to Arvados - Bug #4296: [SSO] 500 from a fresh loginResolved10/23/2014

Related to Arvados - Feature #4601: [SSO] Migrate OpenId users to OAuth/Google+Resolved11/05/2014

Associated revisions

Revision 8cf505f8
Added by Peter Amstutz over 6 years ago

Merge branch '4570-multi-auth-method' refs #4570

Revision 0cfe939f
Added by Peter Amstutz over 6 years ago

Merge branch '4570-support-google-oauth2' closes #4570

History

#1 Updated by Peter Amstutz almost 7 years ago

  • Target version changed from Arvados Future Sprints to 2014-11-19 sprint

#2 Updated by Peter Amstutz almost 7 years ago

  • Target version changed from 2014-11-19 sprint to Arvados Future Sprints

#3 Updated by Ward Vandewege almost 7 years ago

  • Story points set to 2.0

#4 Updated by Peter Amstutz almost 7 years ago

  • Target version changed from Arvados Future Sprints to 2014-12-10 sprint

#5 Updated by Tom Clegg almost 7 years ago

  • Subject changed from [SSO] Implement Google+ sign in to [SSO] Implement Google+ sign in for new SSO installations

#6 Updated by Peter Amstutz almost 7 years ago

  • Assigned To set to Peter Amstutz

#7 Updated by Peter Amstutz almost 7 years ago

  • Subject changed from [SSO] Implement Google+ sign in for new SSO installations to [SSO] Implement Google OAauth2 in for new SSO installations

#8 Updated by Peter Amstutz almost 7 years ago

  • Subject changed from [SSO] Implement Google OAauth2 in for new SSO installations to [SSO] Implement Google OAuth2 in for new SSO installations

#9 Updated by Peter Amstutz almost 7 years ago

Ward: Would you rather (a) put the SSO server in the discovery document (b) configure workbench with the sso server or (c) have API server proxy the "list authentication methods" query or (d) just leave it hardcoded for now?

#10 Updated by Tom Clegg almost 7 years ago

If this is about asking the user which authentication method to use, I think (d) is closest, but it should be "no choice expressed" rather than a hard-coded choice. So Workbench doesn't need to change at all; it just lets the API server and SSO server decide what to do. SSO's config will determine which method is used.

(As long as SSO server has exactly one authentication method enabled, letting Workbench express a preference is kind of academic.)

#11 Updated by Ward Vandewege almost 7 years ago

17:12:59 @cure : this question is about having a way for the user to select the auth provider they want to log in with. On the (workbench) login page.
17:13:29 @cure : obviously we need to have some sort of whitelist/blacklist config knob somewhere
17:13:58 @cure : but it would be nice if the api server could discover the supported mechanisms from the sso server it is configured to talk to
17:14:31 @cure : so maybe e) expose the list of supported auth mechanisms in the discovery doc
17:14:54 @cure : (and the api server could do a call on startup to populate that - requiring an api server restart to repopulate that list is totally ok)
17:15:47 @cure : so yeah. I think e) + an optional whitelist and separate blacklist config knob for the api server
17:15:50 @cure : that would be my vote

#12 Updated by Tom Clegg almost 7 years ago

  • Subject changed from [SSO] Implement Google OAuth2 in for new SSO installations to [SSO] Implement Google OAuth2 for new SSO installations

#13 Updated by Peter Amstutz almost 7 years ago

The short term solution is none of the above; so some of the infrastructure is now in place to have more than one provider, actually having more than one enabled is out of scope of this story. For now, if ?auth_provider is not specified, the SSO server will default to the first configured omniauth provider.

#14 Updated by Peter Amstutz almost 7 years ago

  • Status changed from New to In Progress

#15 Updated by Ward Vandewege almost 7 years ago

I started reviewing this weekend. Will wrap up today, hopefully.

#16 Updated by Ward Vandewege almost 7 years ago

Reviewing 4570-support-google-oauth2:

- don't change environment.rb -- this stuff should go into environment/production.rb etc.

- choosing google vs google-oauth2 has consequences for the URL at the SSO server that needs to be redirected to. That should probably be documented in the sso server config file.

#17 Updated by Peter Amstutz almost 7 years ago

  • Target version changed from 2014-12-10 sprint to 2015-01-07 sprint

#18 Updated by Tom Clegg over 6 years ago

At sso-provider|26caf5a

Suggest renaming config.google_client_* to config.google_oauth2_client_* to match the strategy name.

Suggest using a config var (config.google_open_id?) to disable the :open_id strategy via config/production.rb etc., rather than commenting out the line in initializers/devise.rb.

(and +1 Ward's comment above about config/environments/*.rb)

#19 Updated by Peter Amstutz over 6 years ago

Ward Vandewege wrote:

Reviewing 4570-support-google-oauth2:

- don't change environment.rb -- this stuff should go into environment/production.rb etc.

Fixed.

- choosing google vs google-oauth2 has consequences for the URL at the SSO server that needs to be redirected to. That should probably be documented in the sso server config file.

I don't think this is true. From the API server's perspective, the user is still authenticating using the josh_id provider.

When the user does need to log in, the SSO SessionsController reads the auth_provider parameter and redirects to the appropriate omninauth endpoint. If not specified, this will redirect to the first omniauth provider listed in the Users model (this should retain the current behavior).

4570-multi-auth-method updates the API server to pass through the auth provider from Workbench through API server to the SSO server, allowing the user to choose a provider from the workbench login page. The missing piece is propagating the list of available providers from SSO to API to Workbench. See note #11 above.

#20 Updated by Peter Amstutz over 6 years ago

Tom Clegg wrote:

At sso-provider|26caf5a

Suggest renaming config.google_client_* to config.google_oauth2_client_* to match the strategy name.

Fixed

Suggest using a config var (config.google_open_id?) to disable the :open_id strategy via config/production.rb etc., rather than commenting out the line in initializers/devise.rb.

Fixed.

#21 Updated by Tom Clegg over 6 years ago

Notes on 4570-multi-auth-method at ec33dfc

Couple of tab characters in apps/workbench/app/views/users/welcome.html.erb
  • try commit hook at Coding Standards "git setup" -- you can use git commit --no-verify to override

Remove debug printf (html comment) in welcome.html.erb

Suggest revert to using a link instead of a form. Show how to make separate "Log in with X" and "Log in with Y" a.btn elements, instead of showing how to make a dropdown-then-button form. (One click is better than three, visible options are better than hidden options, provider logo icons are helpful, etc.)
  • Currently logins_test.rb fails because the login button changed from <a> to <button>.

docs should say "copy config/environments/production.rb.example to production.rb and edit" instead of "edit config/environment.rb" with sso changes ^^.

doc section "create a Client record" → rails console should be RAILS_ENV=production bundle exec rails console. Ditto rake db:create and rake db:migrate.

The "Production environment" section is even more "left as an exercise for the reader" than its counterpart on the API server install page. I like pointing out that there are many options, but is there any reason not to mention Passenger specifically as a "known working" default/example?

The "development environment" section seems misplaced: this is an "install" doc, not a "develop" doc. I suggest we use RAILS_ENV=production consistently, and rephrase this as "run a standalone server" (i.e., to verify quickly in your terminal that the app is configured correctly) rather than "run in development mode".

(Unless made moot by the above comment...) This sentence seems to say the same thing twice, with a comma between -- maybe drop one?

To run in development mode, you can now run the development server this way:

#22 Updated by Peter Amstutz over 6 years ago

Tom Clegg wrote:

Notes on 4570-multi-auth-method at ec33dfc

Couple of tab characters in apps/workbench/app/views/users/welcome.html.erb
  • try commit hook at Coding Standards "git setup" -- you can use git commit --no-verify to override

Fixed.

Remove debug printf (html comment) in welcome.html.erb

Fixed.

Suggest revert to using a link instead of a form. Show how to make separate "Log in with X" and "Log in with Y" a.btn elements, instead of showing how to make a dropdown-then-button form. (One click is better than three, visible options are better than hidden options, provider logo icons are helpful, etc.)

Reverted.

  • Currently logins_test.rb fails because the login button changed from <a> to <button>.

Running workbench tests now.

docs should say "copy config/environments/production.rb.example to production.rb and edit" instead of "edit config/environment.rb" with sso changes ^^.

Fixed.

doc section "create a Client record" → rails console should be RAILS_ENV=production bundle exec rails console. Ditto rake db:create and rake db:migrate.

Fixed.

The "Production environment" section is even more "left as an exercise for the reader" than its counterpart on the API server install page. I like pointing out that there are many options, but is there any reason not to mention Passenger specifically as a "known working" default/example?

Fixed.

The "development environment" section seems misplaced: this is an "install" doc, not a "develop" doc. I suggest we use RAILS_ENV=production consistently, and rephrase this as "run a standalone server" (i.e., to verify quickly in your terminal that the app is configured correctly) rather than "run in development mode".

Fixed.

(Unless made moot by the above comment...) This sentence seems to say the same thing twice, with a comma between -- maybe drop one?

To run in development mode, you can now run the development server this way:

Mooted.

#23 Updated by Tom Clegg over 6 years ago

Peter Amstutz wrote:

Couple of tab characters in apps/workbench/app/views/users/welcome.html.erb
  • try commit hook at Coding Standards "git setup" -- you can use git commit --no-verify to override

Fixed.

Still tabs in apps/workbench/app/views/users/welcome.html.erb at 7b5729d...

Suggest revert to using a link instead of a form. Show how to make separate "Log in with X" and "Log in with Y" a.btn elements, instead of showing how to make a dropdown-then-button form. (One click is better than three, visible options are better than hidden options, provider logo icons are helpful, etc.)

Reverted.

Better, thanks. The new <div class="row pull-right"> around the button isn't quite right though. It's redundant to have pull-right inside pull-right (should remove from either the div or the link), and more importantly children of a row must be col-* (here, row is making the button intrude into the well's padding).

The rest looks good, thanks.

#24 Updated by Peter Amstutz over 6 years ago

Tom Clegg wrote:

Peter Amstutz wrote:

Couple of tab characters in apps/workbench/app/views/users/welcome.html.erb
  • try commit hook at Coding Standards "git setup" -- you can use git commit --no-verify to override

Fixed.

Still tabs in apps/workbench/app/views/users/welcome.html.erb at 7b5729d...

Hmm, I added the git options but it didn't trigger any warnings. Maybe because those lines didn't change? Fixed the other tabs.

Suggest revert to using a link instead of a form. Show how to make separate "Log in with X" and "Log in with Y" a.btn elements, instead of showing how to make a dropdown-then-button form. (One click is better than three, visible options are better than hidden options, provider logo icons are helpful, etc.)

Reverted.

Better, thanks. The new <div class="row pull-right"> around the button isn't quite right though. It's redundant to have pull-right inside pull-right (should remove from either the div or the link), and more importantly children of a row must be col-* (here, row is making the button intrude into the well's padding).

Fixed.

#25 Updated by Tom Clegg over 6 years ago

Peter Amstutz wrote:

Still tabs in apps/workbench/app/views/users/welcome.html.erb at 7b5729d...

Hmm, I added the git options but it didn't trigger any warnings. Maybe because those lines didn't change? Fixed the other tabs.

Yes, git-commit only complains about changed/added lines. If you do git config --global color.ui then git diff master...branch will highlight whitespace errors.

LGTM @ d639251, thanks.

#26 Updated by Tom Clegg over 6 years ago

My only other comment on the 4570-support-google-oauth2 (sso) is that the use of session[:auth_provider] is mysterious to me. Why can't SessionsController just use params[:auth_provider] directly? The purpose of the session hash is to preserve information across requests / browser tabs, which we seem not to need, given that we overwrite it in a before_filter on each request. I feel like I'm missing something...

#27 Updated by Peter Amstutz over 6 years ago

Tom Clegg wrote:

My only other comment on the 4570-support-google-oauth2 (sso) is that the use of session[:auth_provider] is mysterious to me. Why can't SessionsController just use params[:auth_provider] directly? The purpose of the session hash is to preserve information across requests / browser tabs, which we seem not to need, given that we overwrite it in a before_filter on each request. I feel like I'm missing something...

The login process is mysterious in general.

See https://arvados.org/projects/arvados/wiki/Workbench_authentication_process

In AuthController:

before_filter :authenticate_user!

This innocent little line actually sets in motion a whole bunch of stuff which culminates in a redirect to /users/sign_in (which routes to SessionsController), but the redirect URI is constructed deep in the warden gem code and would have to be monkey patched to pass along the auth_provider, so adding it to the session was a much simpler option.

However, more recently I have some better ideas about how to handle multiple providers, so we ultimately might not need to pass along auth_provider:

https://arvados.org/issues/4601#note-4

#28 Updated by Tom Clegg over 6 years ago

Peter Amstutz wrote:

This innocent little line actually sets in motion a whole bunch of stuff which culminates in a redirect to /users/sign_in (which routes to SessionsController), but the redirect URI is constructed deep in the warden gem code and would have to be monkey patched to pass along the auth_provider, so adding it to the session was a much simpler option.

So, to sum up, authenticate_user! causes a redirect, and the redirect doesn't preserve params, so we use the session to hack around it. This line (from warden-1.2.3/lib/warden/strategies/base.rb) sure looks like it tries to preserve params. But, after trying to find my way through devise and warden to guess how to propagate params this far, I begin to understand why you gave up and used session...

headers["Location"] << "?" << Rack::Utils.build_query(params) unless params.empty?

A comment might be nice, and some tests would be really really nice, but either way I think this looks good to merge.

Caveat: I haven't had time to actually try running it yet, I'm just reading the code. If Ward (or someone else) has tried running it, or there were tests, I'd feel better about signing off. Thanks.

#29 Updated by Peter Amstutz over 6 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF