Arvados

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?

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.)

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

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.

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

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.

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)

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.

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.

Notes on 4570-multi-auth-method at ec33dfc

• 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

• 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:

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.

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.

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.

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.

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...

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

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.