Feature #12626

[API] Merge user accounts (redirect=true case)

Added by Peter Amstutz over 2 years ago. Updated almost 2 years ago.

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

100%

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

Description

New API endpoint: POST /arvados/v1/users/merge
  • Authorization header has valid API token for the "old" account
  • new_user_token (post form param in request body) has valid API token for the "new" account
  • new_owner_uuid (post form param in request body) has either new user's UUID, or a group UUID writable by the new user
  • redirect_to_new_user=true (optional)
Security checks
  • Current token ("old account") has scopes=["all"]
  • new_user_token ("new account") has scopes=["all"]
  • API logs show the UUID of the corresponding api_client_auth record instead of merge_into_token
Actions
  • Move all records (groups, links, collections, jobs, pipelines, container requests, etc) owned by the old user into new_owner_uuid (this is typically a new empty project or a new user who doesn't own anything, so name conflicts would be a surprise/error)
  • Update links set tail_uuid=new_user_uuid where tail_uuid=old_user_uuid
Additional actions if redirect_to_new_user=true
  • Set old user's redirect_to_user_uuid field to the new user's UUID
  • Move old user's SSH keys to the new user
  • Ensure API tokens associated with old user will give access to the new account.
  • Update links with head_uuid = old user to point to new user
...if redirect_to_new_user=false
  • Leave old user's redirect_to_user_uuid field alone
  • Delete old user's SSH keys
  • Leave old user's API tokens alone
  • Leave links with head_uuid = old user alone.

This is all done in a transaction: if anything fails, the entire operation is cancelled.

Implementation
  • New column (users.redirect_to_user_uuid) is needed.
  • #12995 and #12703 are blocked only by the redirect_to_new_user=true case.

Subtasks

Task #13397: Review 12626-merge-accountsResolvedPeter Amstutz


Related issues

Related to Arvados - Feature #4637: [SSO] Use "authentications" table and support account linkingRejected11/06/2017

Related to Arvados - Story #12702: Migrate user accountsResolved01/05/2018

Related to Arvados - Bug #13368: [API] Add "authorizations" tableClosed

Blocks Arvados - Story #12703: [Workbench] Self serve account mergeResolved

Blocks Arvados - Story #12995: [Workbench] Allow user to add a new Google account to their Arvados accountResolved05/17/2018

Associated revisions

Revision 945621a7
Added by Tom Clegg about 2 years ago

Merge branch '12626-merge-accounts'

refs #12626

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Peter Amstutz over 2 years ago

  • Description updated (diff)

#2 Updated by Tom Morris over 2 years ago

  • Target version set to To Be Groomed

#3 Updated by Tom Morris over 2 years ago

  • Related to Feature #4637: [SSO] Use "authentications" table and support account linking added

#5 Updated by Peter Amstutz over 2 years ago

#6 Updated by Peter Amstutz over 2 years ago

  • Description updated (diff)

#7 Updated by Peter Amstutz over 2 years ago

  • Description updated (diff)

#8 Updated by Peter Amstutz over 2 years ago

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

#11 Updated by Tom Morris over 2 years ago

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

#12 Updated by Tom Morris over 2 years ago

  • Related to Story #12995: [Workbench] Allow user to add a new Google account to their Arvados account added

#13 Updated by Tom Clegg over 2 years ago

  • Description updated (diff)

#14 Updated by Tom Morris over 2 years ago

  • Related to deleted (Story #12995: [Workbench] Allow user to add a new Google account to their Arvados account)

#15 Updated by Tom Morris over 2 years ago

  • Blocks Story #12995: [Workbench] Allow user to add a new Google account to their Arvados account added

#16 Updated by Tom Clegg about 2 years ago

  • Subject changed from Merge user accounts to [API] Merge user accounts
  • Description updated (diff)

#17 Updated by Tom Clegg about 2 years ago

  • Related to Bug #13368: [API] Add "authorizations" table added

#18 Updated by Tom Clegg about 2 years ago

  • Description updated (diff)

#19 Updated by Tom Morris about 2 years ago

  • Story points set to 3.0

#20 Updated by Tom Morris about 2 years ago

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

#21 Updated by Tom Morris about 2 years ago

  • Assigned To set to Tom Clegg

#22 Updated by Tom Clegg about 2 years ago

  • Status changed from New to In Progress

#23 Updated by Tom Clegg about 2 years ago

  • Description updated (diff)

#25 Updated by Peter Amstutz about 2 years ago

  • Distinguishing between "new user" and "old user" is confusing enough without UsersController#merge using the term "dst_user". User#merge also has a comment "# Merge this user's owned items into dst_user." but the term dst_user isn't used anywhere in that method. Please use consistent terminology.
  • Please add comments to UsersController#merge explaining the respective roles of "current user" and "new user".
  • I'm confused what this is checking for. I think it is checking that the new user has write access to the new owner (which I guess could be either the user itself, or a group). Also seems to be missing a default (or a check) when 'new_owner_uuid' is not supplied? Needs a comment.
    if !dst_user.can?(write: params[:new_owner_uuid])
      return send_error("new_owner_uuid is not writable", status: 403)
    end
  • This seems to assume new_owner_uuid is a user, but it could also be a group?
      if User.where('uuid in (?) and redirect_to_user_uuid is not null',
                    [new_owner_uuid, redirect_to_user_uuid]).any?
        raise "cannot merge to/from an already merged user" 
      end
  • Purely from a formatting standpoint, this is confusing, it looks like the line starting with "[" is part of the previous statement (could use a blank line between statements).
      ApiClientAuthorization.
        where(user_id: id).
        update_all(user_id: new_user.id)
      [
        [AuthorizedKey, :owner_uuid],
  • Why is explicit update of "AuthorizedKey.owner_uuid" &c necessary, won't they be updated by "change_all_uuid_refs"? → On further inspection I see this is because the first part updates to the user, the second one updates to the new owner. But the second one actually updates anything ending in _uuid, where we probably only want to update owner_uuid ?

#26 Updated by Tom Clegg about 2 years ago

Peter Amstutz wrote:

  • Distinguishing between "new user" and "old user" is confusing enough without UsersController#merge using the term "dst_user". User#merge also has a comment "# Merge this user's owned items into dst_user." but the term dst_user isn't used anywhere in that method. Please use consistent terminology.

Updated comment and controller method.

  • Please add comments to UsersController#merge explaining the respective roles of "current user" and "new user".

(included in updated comment)

  • I'm confused what this is checking for. I think it is checking that the new user has write access to the new owner (which I guess could be either the user itself, or a group). Also seems to be missing a default (or a check) when 'new_owner_uuid' is not supplied? Needs a comment.

Updated the error message instead of adding a comment, since I figure the purpose of the check should be obvious from the error message if the error message is any good.

    if !new_user.can?(write: params[:new_owner_uuid])
      return send_error("cannot move objects into supplied new_owner_uuid: new user does not have write permission", status: 403)
    end

Added tests for empty/missing new_owner_uuid.

Added a _merge_requires_parameters method for the discovery doc.

  • This seems to assume new_owner_uuid is a user, but it could also be a group?

[...]

Oops, that should have been self.uuid, not new_owner_uuid. Fixed, and split old/new user checks to make the error messages more specific.

  • Purely from a formatting standpoint, this is confusing, it looks like the line starting with "[" is part of the previous statement (could use a blank line between statements).

[...]

Added blank line.

  • Why is explicit update of "AuthorizedKey.owner_uuid" &c necessary, won't they be updated by "change_all_uuid_refs"? → On further inspection I see this is because the first part updates to the user, the second one updates to the new owner. But the second one actually updates anything ending in _uuid, where we probably only want to update owner_uuid ?

Yes... changing modified_by_user_uuid etc. shouldn't be necessary. Fixed so it just touches owner_uuid, and added comments.

12626-merge-accounts @ 4cbac38547d8047e5e23cb4945b25aaa31e3eb06

#27 Updated by Peter Amstutz about 2 years ago

So I tried using the merge API using arv and the behavior is still rather confusing:

  • The current "merge" API currently means roughly "give away all my stuff".

For the use case we have, we have an old login, and we want to associate a new login with the old login. The way I do this is log in to the new account and give away my stuff to the old account, using new_owner=old_account.

Once I've given away my stuff, the new token I used to give it a way indicates I'm now a new (old) user. The old (new) user becomes hidden.

This is a little bit confusing. Maybe "source" and "target" ?

  • It needs to trigger a permission graph update after reassigning ownership.
  • When you log in with the old account, the user record gets the name and email address of the old account. However, if you log in with the new (source) account, the user record gets the name and email address from the new account. This causes flapping in the user record if the user logs in both ways.

#28 Updated by Tom Clegg about 2 years ago

The current API makes more sense if you think "merge" means "merge me [into account X]". The reverse API would make more sense if you think "merge" means "merge [account X] into me". I think with this sort of thing we just have to pick one way and expect client devs to read the label if they care which way is up.

"Old" and "new" refer to the fact that one of the accounts is going away (the "old" one) and you'll be using the other one (the "new" one) instead in the future.

Is this what you have in mind for "target"?

New API endpoint: POST /arvados/v1/users/merge
  • Authorization header has valid API token for the current ("old") account
  • target_user_token (post form param in request body) has valid API token for the "target" account
  • target_owner_uuid (post form param in request body) has either target user's UUID, or a group UUID writable by the target user
  • redirect_to_target_user=true (optional)

(The term "old" isn't even part of the API so I suggest we not worry too much about it.)

#29 Updated by Tom Clegg about 2 years ago

12626-merge-accounts @ 931f77f9bff46dbba8adb0517720eb3c60b83bb3
  • permission graph update
  • update email on auth user, not target user, during auth with a merged/redirected account

#30 Updated by Peter Amstutz about 2 years ago

Tom Clegg wrote:

The current API makes more sense if you think "merge" means "merge me [into account X]". The reverse API would make more sense if you think "merge" means "merge [account X] into me". I think with this sort of thing we just have to pick one way and expect client devs to read the label if they care which way is up.

"Old" and "new" refer to the fact that one of the accounts is going away (the "old" one) and you'll be using the other one (the "new" one) instead in the future.

Is this what you have in mind for "target"?

New API endpoint: POST /arvados/v1/users/merge
  • Authorization header has valid API token for the current ("old") account
  • target_user_token (post form param in request body) has valid API token for the "target" account
  • target_owner_uuid (post form param in request body) has either target user's UUID, or a group UUID writable by the target user
  • redirect_to_target_user=true (optional)

(The term "old" isn't even part of the API so I suggest we not worry too much about it.)

Yea, forget I said anything. It's confusing but I don't want to derail on this.

#31 Updated by Peter Amstutz about 2 years ago

Tom Clegg wrote:

12626-merge-accounts @ 931f77f9bff46dbba8adb0517720eb3c60b83bb3
  • permission graph update
  • update email on auth user, not target user, during auth with a merged/redirected account

I did another manual test and it is working the way I expected now.

LGTM, please merge.

#32 Updated by Tom Clegg about 2 years ago

  • Subject changed from [API] Merge user accounts to [API] Merge user accounts (redirect=true case)
  • Status changed from In Progress to Resolved

#33 Updated by Tom Morris almost 2 years ago

  • Release set to 13

Also available in: Atom PDF