Idea #15558: [SSO] [API] Identify users by (alternate) email addresses - Arvados

Actions

Copy link

Idea #15558

closed

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

Added by Peter Amstutz over 5 years ago. Updated about 5 years ago.

Status:

Resolved

Priority:

Normal

Assigned To:

Peter Amstutz

Category:

Target version:

2019-09-11 Sprint

Start date:

08/22/2019

Due date:

Story points:

Release:

Arvados 2.0 - Early 2020

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:

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

Subtasks 2 (0 open — 2 closed)

Related issues 3 (0 open — 3 closed)

Actions

Copy link

Updated by Peter Amstutz over 5 years ago

Status changed from New to In Progress

Actions

Copy link

Updated by Peter Amstutz over 5 years ago

Description updated (diff)

Actions

Copy link

Updated by Peter Amstutz over 5 years ago

Assigned To set to Peter Amstutz

Actions

Copy link

Updated by Peter Amstutz over 5 years ago

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

Actions

Copy link

Updated by Peter Amstutz over 5 years ago

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

Actions

Copy link

Updated by Peter Amstutz over 5 years ago

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

Actions

Copy link

Updated by Peter Amstutz over 5 years ago

15558-alternate-email-addresses @ 5713754c574254f9e3650ac80bf8fdca235898f6

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

Actions

Copy link

Updated by Tom Morris over 5 years ago

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

Actions

Copy link

Updated by Eric Biagiotti over 5 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.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.

Actions

Copy link

#10

Updated by Peter Amstutz over 5 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.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/

Actions

Copy link

#11

Updated by Eric Biagiotti over 5 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.

Actions

Copy link

#12