Story #12995

[Workbench] Allow user to add a new Google account to their Arvados account

Added by Tom Morris over 1 year ago. Updated over 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
05/17/2018
Due date:
% Done:

100%

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

Description

Use case: A user has a new authentication (eg Google) account. User has previously logged in using some other authentication account (eg LDAP) and already has an Aravdos account. User wants to link their existing Arvados account to the new authentication account so that when they log in with the new authentication account, they are logged into their existing Arvados account.

Entry points

  1. User is logged in to Workbench using old authentication account, selects "link a new authentication method" from menu
  2. User attempts to log in using new authentication account, gets an inactive account page
  3. User attempts to log in using new authentication account, is logged into a new active empty account.

Flow for (1)

  1. On workbench, click on "link new auth method"
  2. Browser stashes the API token in session storage
  3. Browser is sent to api_server/logout?return_to=http://workbench/link_accounts
  4. Browser is logged out from API and SSO, and redirected to workbench link_accounts
  5. Workbench redirects browser to api_server/login?return_to=http://workbench/link_accounts
  6. User logs in and browser is sent back to workbench with &api_token=... of new Arvados account
  7. Workbench now has both API token of the old account (in session storage), and an api_token of the newly logged in account
  8. Browser determines which user account should be merged into the other (based on account creation time, whether it is "empty")
  9. Browser displays a confirmation page stating one account will be linked to the other
  10. Workbench sends request to API server to link one account to the other (#12626)
  11. Workbench uses the API token of the linked account, and presents the user with a "success" page

Flow for (2)

  1. User is at inactive user page. Text says "if you have logged in with a different account, click here to link your account"
  2. Do (1) starting from 2

Flow for (3)

  1. Same as (1) (workbench figures out which way the account linking goes)

Subtasks

Task #13001: Investigate turning off session cookies for SSO serverResolvedPeter Amstutz

Task #13457: Review 12995-wb-merge-acctResolvedPeter Amstutz

Task #13503: Document setting SSO session timeoutResolvedPeter Amstutz

Task #13504: Review 12995-session-timeout (SSO)ResolvedLucas Di Pentima


Related issues

Related to Arvados - Story #12703: [Workbench] Self serve account mergeNew

Blocked by Arvados - Feature #12626: [API] Merge user accounts (redirect=true case)Resolved05/03/2018

Associated revisions

Revision 1471ad4b
Added by Peter Amstutz over 1 year ago

Merge branch '12995-wb-merge-acct' refs #12995

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

Revision 3bccde8b
Added by Peter Amstutz over 1 year ago

Merge branch '12995-session-timeout' refs #12995

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

History

#1 Updated by Lucas Di Pentima over 1 year ago

  • Description updated (diff)

#2 Updated by Tom Morris over 1 year ago

  • Related to Feature #12626: [API] Merge user accounts (redirect=true case) added

#3 Updated by Peter Amstutz over 1 year ago

  • Description updated (diff)

#4 Updated by Peter Amstutz over 1 year ago

  • Description updated (diff)

#5 Updated by Peter Amstutz over 1 year ago

  • Description updated (diff)

#6 Updated by Tom Morris over 1 year ago

  • Related to deleted (Feature #12626: [API] Merge user accounts (redirect=true case))

#7 Updated by Tom Morris over 1 year ago

  • Blocked by Feature #12626: [API] Merge user accounts (redirect=true case) added

#8 Updated by Tom Morris over 1 year ago

  • Story points set to 3.0

#9 Updated by Peter Amstutz over 1 year ago

  • Description updated (diff)

#10 Updated by Tom Morris over 1 year ago

  • Target version changed from To Be Groomed to Arvados Future Sprints

#11 Updated by Tom Morris over 1 year ago

  • Target version changed from Arvados Future Sprints to 2018-05-09 Sprint

#12 Updated by Tom Morris over 1 year ago

  • Target version changed from 2018-05-09 Sprint to 2018-05-23 Sprint

#13 Updated by Peter Amstutz over 1 year ago

  • Assigned To set to Peter Amstutz

#14 Updated by Peter Amstutz over 1 year ago

  • Related to Story #12703: [Workbench] Self serve account merge added

#15 Updated by Peter Amstutz over 1 year ago

12995-wb-merge-acct @ 4aa2e9342254971e92b5836a56728015e9cfc714

Adds a /users/link_account page accessible from user menu and on the "inactive user" page.

  • User can choose whether to add a login to the current account, or link the current login to another account
  • When one of the accounts is inactive, can only merge the login of the inactive account to the active account

Manually tested:

  • From existing user, select "Add another login to this account", login as the "new" user, then link accounts.
  • From new, active user, select "Use this login to access another account", login as "old" user, then link accounts
  • From new, inactive user, select "Use this login to access another account", login as "old" user, then link accounts.

No automated tests. The problem is, this process relies on the SSO server, which the run-tests.sh / workbench test environment doesn't provide. Hard to work around. Could possibly rig something up based on arvbox.

Also:

12995-session-timeout @ commit:21f2ee32fe8fc6391c95b5dcdb59d787629dceff branch on the sso-provider repository.

  • Sets session timeout to 1 second so that users always have to log in (otherwise sessions mess up the "log in as a different user" part of the flow.)

#16 Updated by Lucas Di Pentima over 1 year ago

  • Maybe it would be convenient to add a date on the group name to avoid possible conflicts when the merge action fails after the group is created, or just reuse it.
  • Using either direction, I get the api error: "supplied API token is not from a trusted client" - API Request URL: https://172.17.0.2:8000/arvados/v1/users/merge
  • Tests done with arvbox

#17 Updated by Lucas Di Pentima over 1 year ago

  • After adding the trusted client setting, manual tests on both directions worked well.
  • The first bulletpoint from note-16 may be a valid one, just in case there's some transient api server problem.
  • Is there an explicit way for workbench to ask for the new sso-provider (package dependency)?
  • Issues that may not be for this review but instead for the api server side:
    • When linking an admin account into a non-admin account left me without admin access
    • After linking account A into account B, and later on linking account B into account C, when try to login with account A, I get an error like this: {"errors":["#<Exception: identity_url h793v-tpzed-f1svf5ts12yw4c3 redirects to nonexistent uuid 2bs4c-tpzed-2cx7fuz2l783jhi>"],"error_token":"1526663055+db8a355d"}
  • After clicking any of the linking buttons, the next thing the user sees is a login dialog saying "Your session expired, please sign in again to continue." and I think it can be confusing, if this is not worth correcting on the SSO's side, maybe we could have a message on wb's link account page warning what will happen after clicking.

#18 Updated by Tom Clegg over 1 year ago

A→B + B→C = error during login

Can the merge API detect this when B→C is happening, and flatten the tree by changing A→B to A→C?

admin→non-admin

Yes, Workbench should warn if you're about to do this, but I suppose we might as well go ahead and do it if you accept/ignore the warning.

#19 Updated by Peter Amstutz over 1 year ago

Lucas Di Pentima wrote:

  • The first bulletpoint from note-16 may be a valid one, just in case there's some transient api server problem.

Added ensure_unique_name: true

  • Is there an explicit way for workbench to ask for the new sso-provider (package dependency)?

No, there isn't.

  • Issues that may not be for this review but instead for the api server side:
    • When linking an admin account into a non-admin account left me without admin access

Fixed workbench so it detects that case and won't let you do that.

  • After linking account A into account B, and later on linking account B into account C, when try to login with account A, I get an error like this: {"errors":["#<Exception: identity_url h793v-tpzed-f1svf5ts12yw4c3 redirects to nonexistent uuid 2bs4c-tpzed-2cx7fuz2l783jhi>"],"error_token":"1526663055+db8a355d"}

This was an API server bug in following chained redirects. Fixed & added tests.

  • After clicking any of the linking buttons, the next thing the user sees is a login dialog saying "Your session expired, please sign in again to continue." and I think it can be confusing, if this is not worth correcting on the SSO's side, maybe we could have a message on wb's link account page warning what will happen after clicking.

I noticed that too. I don't really want to mess with the SSO server too much. It might be possible to change the flow to have it go through the logout procedure and then immediately to the login procedure, which would also eliminate the need to upgrade SSO server.

Now @ 6237a718e292de02dc06c2885e4a96260616ce03

Still todo: add a documentation page.

#20 Updated by Peter Amstutz over 1 year ago

12995-wb-merge-acct e2632a25d3aab230bdc44936fa42a3d27ff15d30

  • Added username to the link accounts page to further disambiguate the two accounts being linked
  • Added documentation pages
  • Updated to use a login/logout flow instead of relying on session timeout. However, the SSO server still needs to be updated (see below)

12995-session-timeout 4ed380efb64d50a6c74defe74ffef08166f4f0c7

  • Added a "choose" page when more than one provider is configured. Enables users to select between LDAP and Google.

#22 Updated by Lucas Di Pentima over 1 year ago

Manually tested updates on both branches, providing that Jenkins' run goes well, LGTM. Thanks!

#23 Updated by Peter Amstutz over 1 year ago

  • Status changed from New to Resolved

#24 Updated by Tom Morris over 1 year ago

  • Release set to 13

Also available in: Atom PDF