15531: Adjust redirect_to_new_user to intended behavior

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

diff --git a/sdk/python/arvados/commands/federation_migrate.py b/sdk/python/arvados/commands/federation_migrate.py
index ec846d2c4aa2c7c298a2f092839de373aa1e3fb1..3b3a7ee6655940755ed8744746abd7a8acadfd06 100755 (executable)
--- a/sdk/python/arvados/commands/federation_migrate.py
+++ b/sdk/python/arvados/commands/federation_migrate.py
@@ -286,7 +286,7 @@ def main():
                         migratearv.users().merge(old_user_uuid=old_user_uuid,
                                                  new_user_uuid=new_user_uuid,
                                                  new_owner_uuid=grp["uuid"],
-                                                 redirect_to_new_user=old_user_uuid.startswith(migratecluster)).execute()
+                                                 redirect_to_new_user=True).execute()
                 except arvados.errors.ApiError as e:
                     print("(%s) Error migrating user: %s" % (email, e))
 

diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 0ef129481492c9aad9ba06114b2ad12304e2cffd..2889eacee644ba080439faa6a0e17ad629c8171c 100644 (file)
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -176,15 +176,10 @@ class Arvados::V1::UsersController < ApplicationController
       return send_error("cannot move objects into supplied new_owner_uuid: new user does not have write permission", status: 403)
     end
 
-    redirect = params[:redirect_to_new_user]
-    if @object.uuid[0..4] != Rails.configuration.ClusterID && redirect
-      return send_error("cannot merge remote user to other with redirect_to_new_user=true", status: 422)
-    end
-
     act_as_system_user do
       @object.merge(new_owner_uuid: params[:new_owner_uuid],
-                    redirect_to_user_uuid: new_user.uuid,
-                    redirect_auth: redirect)
+                    new_user_uuid: new_user.uuid,
+                    redirect_to_new_user: params[:redirect_to_new_user])
     end
     show
   end

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 59fb3fc09dd7ff7daa2b58c7c98c36225b8c9fbf..08476be57caf686834cbe3cfc1d3bced1f8625c0 100644 (file)
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -272,26 +272,28 @@ class User < ArvadosModel
     end
   end
 
-  # Move this user's (i.e., self's) owned items into new_owner_uuid.
-  # Also redirect future uses of this account to
-  # redirect_to_user_uuid, i.e., when a caller authenticates to this
-  # account in the future, the account redirect_to_user_uuid account
-  # will be used instead.
+  # Move this user's (i.e., self's) owned items to new_owner_uuid and
+  # new_user_uuid (for things normally owned directly by the user).
+  #
+  # If redirect_auth is true, also reassign auth tokens and ssh keys,
+  # and redirect this account to redirect_to_user_uuid, i.e., when a
+  # caller authenticates to this account in the future, the account
+  # redirect_to_user_uuid account will be used instead.
   #
   # current_user must have admin privileges, i.e., the caller is
   # responsible for checking permission to do this.
-  def merge(new_owner_uuid:, redirect_to_user_uuid:, redirect_auth:)
+  def merge(new_owner_uuid:, new_user_uuid:, redirect_to_new_user:)
     raise PermissionDeniedError if !current_user.andand.is_admin
-    raise "not implemented" if !redirect_to_user_uuid
+    raise "not implemented" if !new_user_uuid
     transaction(requires_new: true) do
       reload
       raise "cannot merge an already merged user" if self.redirect_to_user_uuid
 
-      new_user = User.where(uuid: redirect_to_user_uuid).first
+      new_user = User.where(uuid: new_user_uuid).first
       raise "user does not exist" if !new_user
       raise "cannot merge to an already merged user" if new_user.redirect_to_user_uuid
 
-      if redirect_auth
+      if redirect_to_new_user
         # Existing API tokens and ssh keys are updated to authenticate
         # to the new user.
         ApiClientAuthorization.
@@ -313,8 +315,7 @@ class User < ArvadosModel
         AuthorizedKey.where(authorized_user_uuid: uuid).destroy_all
         user_updates = [
           [Link, :owner_uuid],
-          [Link, :tail_uuid],
-          [Link, :head_uuid],
+          [Link, :tail_uuid]
         ]
       end
 
@@ -351,7 +352,9 @@ class User < ArvadosModel
         klass.where(owner_uuid: uuid).update_all(owner_uuid: new_owner_uuid)
       end
 
-      update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil)
+      if redirect_to_new_user
+        update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil)
+      end
       invalidate_permissions_cache
     end
   end

diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 4e19988de1bee16528ccc410a4225509e4a40368..f5c4ea0eff55b16bb51235a901c497d40605df55 100644 (file)
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -826,7 +826,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
            redirect_to_new_user: false,
          }
     assert_response(:success)
-    assert_equal(users(:active).uuid, User.unscoped.find_by_uuid(users(:project_viewer).uuid).redirect_to_user_uuid)
+    assert_nil(User.unscoped.find_by_uuid(users(:project_viewer).uuid).redirect_to_user_uuid)
 
     # because redirect_to_new_user=false, token owned by
     # project_viewer should be deleted