Arvados

related: https://docs.google.com/document/d/1LdKscxuPbBnK4qX2ZHUQpSRhuDg8IEHJh_W2y8Y0-WY

12626-merge-accounts @ 26538afdf1c8fdad14208d08a19bafb41e42044c

• 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 ?

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

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.

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"?

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

• permission graph update
• update email on auth user, not target user, during auth with a merged/redirected account

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.

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.