Arvados

15107-google-login @ deaf1d8f2f694b09562eddac055ccebba5a98517 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1622/

(also tested on 4xphq using real Google credentials)

Getting this working locally (I've done it successfully with SSO so I know its possible).

First thing I noticed, ProviderAppID can't be empty but it also conflicts with GoogleClientID:

2019-11-01_18:33:40.56022 Login.ProviderAppID cannot be empty
2019-11-01_18:33:40.56045 /usr/src/arvados/services/api/lib/config_loader.rb:118:in `block in coercion_and_check'
2019-11-01_18:33:40.56046 /usr/src/arvados/services/api/lib/config_loader.rb:80:in `each'
2019-11-01_18:33:40.56046 /usr/src/arvados/services/api/lib/config_loader.rb:80:in `coercion_and_check'
2019-11-01_18:33:40.56047 /usr/src/arvados/services/api/config/arvados_config.rb:232:in `<top (required)>'

(To work around it I made ProviderAppID non-essential).

Now in the callback phase I'm getting this error:

{"errors":["request failed: http://localhost:8004/auth/controller/callback: 401 Unauthorized: Invalid authorization header (req-1bz95m2kax6xz19va72o)"]}

After some poking I modified the error code to get this:

{"errors":["request failed: http://localhost:8004/auth/controller/callback: 401 Unauthorized: Invalid authorization header got 'Bearer' expected 'Bearer ' (req-ov89tfhbfya42uwb317e)"]}

It turns out arvbox isn't setting SystemRootToken. Whoops. That should be validated. We can check this in config/arvados_config.rb, but probably this belongs in the validation on the Go side.

arvcfg.declare_config "SystemRootToken", NonemptyString

In order to have feature parity with SSO Google login we need to support alternate emails and the customer-requested "username domain" feature. The Ruby code starts at https://dev.arvados.org/projects/arvados/repository/sso-provider/revisions/master/entry/app/controllers/users/omniauth_callbacks_controller.rb#L20

1. Needs the scope "user.emails.read" (might actually be called "https://www.googleapis.com/auth/user.emails.read")
2. Need to get the "me" record from the google people service (https://godoc.org/google.golang.org/api/people/v1) using the google token we received from the login callback
3. Go through the email addresses in the response to determine the primary & alternate email addresses for the account
4. "first_name" and "last_name" are also missing from auth_info, my new user is called "null null"
5. Support an optional "username domain", the email address that has that domain will be used for the preferred username

Still need to manually test that LoginCluster redirection works.

• RailsAPI doesn't error out if ProviderAppID/Secret are empty
• First/last name propagated to RailsAPI
• Test for propagation of user info to RailsAPI

Can add "alternate email addrs" and "preferred domain for choosing username" in a subsequent branch.

Making SystemRootToken mandatory sounds like an improvement. To avoid worsening the unforgiving sequence of install steps (and save a few db queries) we'll probably also want the RailsAPI auth middleware to recognize it by looking at config, instead of requiring the installer to create an api_client_authorizations row and then copy the token to config.

Can add "alternate email addrs" and "preferred domain for choosing username" in a subsequent branch.

Same ticket or new ticket?

Making SystemRootToken mandatory sounds like an improvement. To avoid worsening the unforgiving sequence of install steps (and save a few db queries) we'll probably also want the RailsAPI auth middleware to recognize it by looking at config, instead of requiring the installer to create an api_client_authorizations row and then copy the token to config.

Also agree. Same ticket or new ticket?

Peter Amstutz wrote:

Can add "alternate email addrs" and "preferred domain for choosing username" in a subsequent branch.

Same ticket or new ticket?

Same, this seems like part of implementing Google login.

Making SystemRootToken mandatory

Also agree. Same ticket or new ticket?

Added #15795

arvcfg.declare_config "SystemRootToken", String, :SystemRootToken

• The 3rd argument is the corresponding key to import from legacy application.yml, but I don't think that ever existed?
• Instead of 'String' it can be 'NonemptyString' to prevent the API server from starting if it is empty, the description in #15795 doesn't say anything about requiring SystemRootToken have a valid value for services to start

Are you sure the "claims" struct we get from Google doesn't already have first_name and last_name separated?

... ran out of time will look at it some more tomorrow

Peter Amstutz wrote:

• The 3rd argument is the corresponding key to import from legacy application.yml, but I don't think that ever existed?

Removed.

• Instead of 'String' it can be 'NonemptyString' to prevent the API server from starting if it is empty, the description in #15795 doesn't say anything about requiring SystemRootToken have a valid value for services to start

OK, noted on #15795. But if we made it mandatory now, wouldn't installation require you to add a bogus token, then run the "generate valid root token" rake task now that the config is valid, then replace the bogus token with the real one? (This is what I meant by "worsening the unforgiving sequence of install steps" above.)

Are you sure the "claims" struct we get from Google doesn't already have first_name and last_name separated?

I guess not, but I don't see them at https://developers.google.com/identity/protocols/OpenIDConnect

Also needs to be added to documentation as a "beta" feature

Tom Clegg wrote:

Are you sure the "claims" struct we get from Google doesn't already have first_name and last_name separated?

I guess not, but I don't see them at https://developers.google.com/identity/protocols/OpenIDConnect

Ok, I looked at what Omniauth does, it makes a callback to the "https://www.googleapis.com/oauth2/v3/userinfo" endpoint to get a record that includes the separated given / family name. The "people/me" endpoint should also provide this information, so we can just switch to using that when we do that branch (next).

ae562784e8d8d8bd501c0bd373739d0a2da8fc9f LGTM

Ah, I never did a manual test that LoginCluster works. Note to self: do more manual testing when reviewing the next branch.

15107-alt-email @ e0b0048703b78b6a6846e4b5e0f71ec4b1553aa0 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1640/

When EnableBetaController14287 is turned on, but I'm not using the Google login, when visiting the '/login' route, the redirect to "/auth/joshid" seems to be rewritten by controller to the Rails server internal URL.

With EnableBetaController14287 turned off, the redirect to "/auth/joshid" (and from there to the SSO server) works as expected.

If the bad redirect target was https://internal_rails_host:port (as opposed to http://internal_rails_host:port) then this should fix it. It turns out the "X-Forwarded-Proto: https" header -- in addition to reassuring Rails that it doesn't need to redirect plain requests to https -- also caused it to rewrite relative redirect targets as the bogus "https://host:port/target" (instead of "http://host:port/target"), and controller just passed it through as is instead of rewriting it, because it wasn't same-origin.

So it seems force_ssl really does force ssl (by either redirecting all reqs to bogus URLs or mangling relative redirect_to targets)... surprisingly enough.

15107-rails-bad-redirect @ 4f938f3a77ef2629d934dec56e25a314c682b6aa https://ci.curoverse.com/view/Developer/job/developer-run-tests/1644/

15107-rails-bad-redirect @ c00d9e1595d07e6941bb2fbfb8b4e57c3c4ba856 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1645/

• stop sending the X-Forwarded-Proto header (don't mangle redirect_to targets)
• turn off force_ssl (allow http reqs without an X-Forwarded-Proto header)
• leave force_ssl on (default) but disable the redirect-all-requests-to-https feature

• Retrieve additional email addresses and passes the verified ones through to RailsAPI
• ...but allow the admin to disable this via config (in case the People API can't be enabled immediately and they would rather sacrifice the feature for now than get stuck on it)
• Mention any ignored (non-verified) email addresses in logs, to help troubleshooting
• If the People API returns a "primary" name, use it (with the provided first/last split) instead of splitting the OIDC full name on whitespace

Tom Clegg wrote:

If the bad redirect target was https://internal_rails_host:port (as opposed to http://internal_rails_host:port) then this should fix it. It turns out the "X-Forwarded-Proto: https" header -- in addition to reassuring Rails that it doesn't need to redirect plain requests to https -- also caused it to rewrite relative redirect targets as the bogus "https://host:port/target" (instead of "http://host:port/target"), and controller just passed it through as is instead of rewriting it, because it wasn't same-origin.

So it seems force_ssl really does force ssl (by either redirecting all reqs to bogus URLs or mangling relative redirect_to targets)... surprisingly enough.

15107-rails-bad-redirect @ 4f938f3a77ef2629d934dec56e25a314c682b6aa https://ci.curoverse.com/view/Developer/job/developer-run-tests/1644/

15107-rails-bad-redirect @ c00d9e1595d07e6941bb2fbfb8b4e57c3c4ba856 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1645/

• stop sending the X-Forwarded-Proto header (don't mangle redirect_to targets)
• turn off force_ssl (allow http reqs without an X-Forwarded-Proto header)
• leave force_ssl on (default) but disable the redirect-all-requests-to-https feature

Tested this and it works for me, LGTM.

After banging my head against this for a while, I finally figured out that "go get ..." fetches the latest of everything, the correct command to use with modules is "go mod download".

I've pushed commit 3b9af4b0f to 15107-alt-email that fixes arvbox to use "go mod download".

Finally able to review the branch.

This is missing the feature of designating a specific email domain who's username will be used as the preferred arvados username. This is a customer-requested feature.

• adds Users.PreferDomainForUsername config entry

Tom Clegg wrote:

15107-prefer-domain-for-username @ 943827578884b09a155443a9d2bb685a327070f9 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1649/

• adds Users.PreferDomainForUsername config entry

The test case didn't properly verify that was getting the username from preferdomainforusername.example.com, and also didn't check the "+" section would be handled properly. I went ahead and just tweaked the tests and pushed that commit and the rest LGTM:

542a72e8ea402a65d75a5251ba219341834fb2c9