Story #15558

[SSO] [API] Identify users by (alternate) email addresses

Added by Peter Amstutz 11 months ago. Updated 5 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
08/22/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

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:

  1. try primary address first
  2. 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.)


Subtasks

Task #15568: Review 15558-alternate-email-addresses in SSO repoResolvedEric Biagiotti

Task #15587: Review 15558-alternate-email-addresses in arvadosResolvedEric Biagiotti


Related issues

Related to Arvados - Story #15529: [API] [Controller] Share user account database with a group of trusted clustersResolved08/22/2019

Has duplicate Arvados - Story #15477: Use email address for Arvados account linkingDuplicate

Has duplicate Arvados - Story #15493: Allow admin to configure Unix account idDuplicate

Associated revisions

Revision 47c0c13e
Added by Peter Amstutz 10 months ago

Merge branch '15558-alternate-email-addresses' refs #15558

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 1c3c8f7f
Added by Peter Amstutz 10 months ago

Merge branch '15558-alternate-email-addresses' refs #15558

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 65705937 (diff)
Added by Peter Amstutz 10 months ago

Add CurrentApiClient to User fix login failure refs #15558

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

Revision 1cb4ddc6 (diff)
Added by Peter Amstutz 9 months ago

Commit missing migration refs #15558

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <>

History

#1 Updated by Peter Amstutz 11 months ago

  • Status changed from New to In Progress

#2 Updated by Peter Amstutz 11 months ago

  • Description updated (diff)

#3 Updated by Peter Amstutz 11 months ago

  • Assigned To set to Peter Amstutz

#4 Updated by Peter Amstutz 11 months ago

  • Has duplicate Story #15477: Use email address for Arvados account linking added

#5 Updated by Peter Amstutz 11 months ago

  • Has duplicate Story #15493: Allow admin to configure Unix account id added

#6 Updated by Peter Amstutz 11 months ago

  • Related to Story #15529: [API] [Controller] Share user account database with a group of trusted clusters added

#8 Updated by Tom Morris 10 months ago

  • Target version changed from 2019-08-28 Sprint to 2019-09-11 Sprint

#9 Updated by Eric Biagiotti 10 months 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.email right? 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.

#10 Updated by Peter Amstutz 10 months 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.email right? 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/

#11 Updated by Eric Biagiotti 10 months 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.

#12 Updated by Peter Amstutz 10 months 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/

#13 Updated by Eric Biagiotti 10 months ago

Thanks, given the tests pass, this LGTM!

#14 Updated by Peter Amstutz 10 months ago

Eric Biagiotti wrote:

Thanks, given the tests pass, this LGTM!

https://ci.curoverse.com/view/Developer/job/developer-run-tests/1500/

#15 Updated by Peter Amstutz 10 months ago

  • Target version deleted (2019-09-11 Sprint)

15558-alternate-email-addresses @ 2fd0a7680e075431baa61288f34bf400ccaae849

https://ci.curoverse.com/view/Developer/job/developer-run-tests/1503/

#17 Updated by Peter Amstutz 10 months ago

  • Target version set to 2019-09-11 Sprint

#18 Updated by Peter Amstutz 10 months ago

  • Status changed from In Progress to Resolved

#19 Updated by Peter Amstutz 5 months ago

  • Release set to 22

Also available in: Atom PDF