Idea #15558
closed
[SSO] [API] Identify users by (alternate) email addresses
Description
Goal: identify users by email address as a fallback when identity_url is different (due to a different upstream login) or user changes their primary email (assumes the old email is listed as an alternate).
SSO¶
When providing callback response to API, include all upstream-provided email addresses as alternates in addition to the primary.
Login¶
When logging in, if the identity_url doesn't match a user, look up user by email address, filtering out remote users:
- try primary address first
- then try alternate addresses
If more than one address matches: if there is exactly one match without a redirect, use that. If all matches have a redirect: if all redirect to the same account (or there is just one match), use that. If it is still ambiguous which account to use, login fails.
Once the primary user account has been selected, update the identity_url, email address, and name based on the SSO callback.
Additionally, because it is being used for identity, the 'email' column should no longer be user editable.
Database changes: add uniqueness constraint to identity_url (it is already de facto unique, but it ought to be enforced.)
Related issues
Updated by Peter Amstutz over 4 years ago
- Status changed from New to In Progress
Updated by Peter Amstutz over 4 years ago
- Has duplicate Idea #15477: Use email address for Arvados account linking added
Updated by Peter Amstutz over 4 years ago
- Has duplicate Idea #15493: Allow admin to configure Unix account id added
Updated by Peter Amstutz over 4 years ago
- Related to Idea #15529: [API] [Controller] Share user account database with a group of trusted clusters added
Updated by Peter Amstutz over 4 years ago
15558-alternate-email-addresses @ 5713754c574254f9e3650ac80bf8fdca235898f6
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1488/
Updated by Tom Morris over 4 years ago
- Target version changed from 2019-08-28 Sprint to 2019-09-11 Sprint
Updated by Eric Biagiotti over 4 years ago
Peter Amstutz wrote:
15558-alternate-email-addresses @ 5713754c574254f9e3650ac80bf8fdca235898f6
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1488/
SSO:
This LGTM
API:
- You have a broken test. Looks like the stub content needs to be updated.
RemoteUsersTest#test_remote_user_inactive_without_pre-activation [/usr/src/arvados/services/api/test/integration/remote_user_test.rb:318]: Expected false to be nil.
- services/api/app/models/user.rb line 405. The two entries in the first clause should both be
primary_user.emailright? Maybe tweak "fail lookup without identifiers" test to cover this?
if (!primary_user.email or primary_user.identity_url.empty?) and (!primary_user.identity_url or primary_user.identity_url.empty?)
- What about the read-only and uniqueness changes from the description and the paragraph about handling if more than one address matched? If these are no longer needed, please update the description.
Updated by Peter Amstutz over 4 years ago
Eric Biagiotti wrote:
SSO:
This LGTM
Thanks. I added a comment about the configuration option and merged it.
API:
- You have a broken test. Looks like the stub content needs to be updated.
Fixed. Unfortunately the "is_admin" field defaults to NULL (or nil) if not set explicitly.
- services/api/app/models/user.rb line 405. The two entries in the first clause should both be
primary_user.emailright? Maybe tweak "fail lookup without identifiers" test to cover this?
Fixed. As it happens, if email is "" and identity_url is nil this would still raise an exception, but then it would be "method empty? not found on nil" instead of the intended one.
- What about the read-only and uniqueness changes from the description and the paragraph about handling if more than one address matched? If these are no longer needed, please update the description.
I overlooked that, thanks. Implemented, with tests.
15558-alternate-email-addresses @ d6c4fc82452b6c8e7fe492a0e2a163a19477f95a
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1495/
Updated by Eric Biagiotti over 4 years ago
Thanks for the updates. Just a few more things:
- RemoteUserTest.rb, line 297 has some debug output.
- The description mentions updating the database to enforce identity_url uniqueness. Probably worth doing while we are working on it.
Updated by Peter Amstutz over 4 years ago
Eric Biagiotti wrote:
Thanks for the updates. Just a few more things:
- RemoteUserTest.rb, line 297 has some debug output.
Fixed.
- The description mentions updating the database to enforce identity_url uniqueness. Probably worth doing while we are working on it.
Thanks, I added a migration to do that.
15558-alternate-email-addresses @ 9902ba33a4006d8652e675f76b6d7e43a2446d14
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1498/
Updated by Eric Biagiotti over 4 years ago
Thanks, given the tests pass, this LGTM!
Updated by Peter Amstutz over 4 years ago
Eric Biagiotti wrote:
Thanks, given the tests pass, this LGTM!
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1500/
Updated by Peter Amstutz over 4 years ago
- Target version deleted (
2019-09-11 Sprint)
15558-alternate-email-addresses @ 2fd0a7680e075431baa61288f34bf400ccaae849
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1503/
Updated by Peter Amstutz over 4 years ago
15558-alternate-email-addresses @ 168c5a9a50b93f736b15b7a6c56af900b90aab39
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1506/
Updated by Peter Amstutz over 4 years ago
- Target version set to 2019-09-11 Sprint
Updated by Peter Amstutz over 4 years ago
- Status changed from In Progress to Resolved