From 0c90b88beddd9d5d6dcf777af43afe1817d37d61 Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Fri, 3 Sep 2021 15:39:05 -0300 Subject: [PATCH] 18094: Removes update_uuid code & tests from railsAPI. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- .../arvados/v1/users_controller.rb | 17 +---- services/api/app/models/user.rb | 31 --------- services/api/config/routes.rb | 1 - .../arvados/v1/users_controller_test.rb | 21 ------ services/api/test/unit/owner_test.rb | 25 +------ services/api/test/unit/user_test.rb | 66 ------------------- 6 files changed, 3 insertions(+), 158 deletions(-) diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb index f4d42edf6c..52f4b1e5eb 100644 --- a/services/api/app/controllers/arvados/v1/users_controller.rb +++ b/services/api/app/controllers/arvados/v1/users_controller.rb @@ -10,7 +10,7 @@ class Arvados::V1::UsersController < ApplicationController [:activate, :current, :system, :setup, :merge, :batch_update] skip_before_action :render_404_if_no_object, only: [:activate, :current, :system, :setup, :merge, :batch_update] - before_action :admin_required, only: [:setup, :unsetup, :update_uuid, :batch_update] + before_action :admin_required, only: [:setup, :unsetup, :batch_update] # Internal API used by controller to update local cache of user # records from LoginCluster. @@ -145,13 +145,6 @@ class Arvados::V1::UsersController < ApplicationController show end - # Change UUID to a new (unused) uuid and transfer all owned/linked - # objects accordingly. - def update_uuid - @object.update_uuid(new_uuid: params[:new_uuid]) - show - end - def merge if (params[:old_user_uuid] || params[:new_user_uuid]) if !current_user.andand.is_admin @@ -261,14 +254,6 @@ class Arvados::V1::UsersController < ApplicationController }) end - def self._update_uuid_requires_parameters - { - new_uuid: { - type: 'string', required: true, - }, - } - end - def apply_filters(model_class=nil) return super if @read_users.any?(&:is_admin) if params[:uuid] != current_user.andand.uuid diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index da7e7b310d..2e862d3ae6 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -362,37 +362,6 @@ SELECT target_uuid, perm_level end end - def update_uuid(new_uuid:) - if !current_user.andand.is_admin - raise PermissionDeniedError - end - if uuid == system_user_uuid || uuid == anonymous_user_uuid - raise "update_uuid cannot update system accounts" - end - if self.class != self.class.resource_class_for_uuid(new_uuid) - raise "invalid new_uuid #{new_uuid.inspect}" - end - transaction(requires_new: true) do - reload - old_uuid = self.uuid - self.uuid = new_uuid - save!(validate: false) - change_all_uuid_refs(old_uuid: old_uuid, new_uuid: new_uuid) - ActiveRecord::Base.connection.exec_update %{ -update #{PERMISSION_VIEW} set user_uuid=$1 where user_uuid = $2 -}, - 'User.update_uuid.update_permissions_user_uuid', - [[nil, new_uuid], - [nil, old_uuid]] - ActiveRecord::Base.connection.exec_update %{ -update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2 -}, - 'User.update_uuid.update_permissions_target_uuid', - [[nil, new_uuid], - [nil, old_uuid]] - end - end - # 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). # diff --git a/services/api/config/routes.rb b/services/api/config/routes.rb index 6975858035..738426b1d8 100644 --- a/services/api/config/routes.rb +++ b/services/api/config/routes.rb @@ -81,7 +81,6 @@ Rails.application.routes.draw do post 'activate', on: :member post 'setup', on: :collection post 'unsetup', on: :member - post 'update_uuid', on: :member post 'merge', on: :collection patch 'batch_update', on: :collection 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 e0f7b8970d..c807a7d6cb 100644 --- a/services/api/test/functional/arvados/v1/users_controller_test.rb +++ b/services/api/test/functional/arvados/v1/users_controller_test.rb @@ -800,27 +800,6 @@ The Arvados team. "user's writable_by should include its owner_uuid") end - [ - [:admin, true], - [:active, false], - ].each do |auth_user, expect_success| - test "update_uuid as #{auth_user}" do - authorize_with auth_user - orig_uuid = users(:active).uuid - post :update_uuid, params: { - id: orig_uuid, - new_uuid: 'zbbbb-tpzed-abcde12345abcde', - } - if expect_success - assert_response :success - assert_empty User.where(uuid: orig_uuid) - else - assert_response 403 - assert_not_empty User.where(uuid: orig_uuid) - end - end - end - test "merge with redirect_to_user_uuid=false" do authorize_with :project_viewer_trustedclient tok = api_client_authorizations(:project_viewer).api_token diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb index e356f4d9fa..aa0ac5f361 100644 --- a/services/api/test/unit/owner_test.rb +++ b/services/api/test/unit/owner_test.rb @@ -92,12 +92,8 @@ class OwnerTest < ActiveSupport::TestCase "new #{o_class} should really be in DB") old_uuid = o.uuid new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9]) - if o.respond_to? :update_uuid - o.update_uuid(new_uuid: new_uuid) - else - assert(o.update_attributes(uuid: new_uuid), - "should change #{o_class} uuid from #{old_uuid} to #{new_uuid}") - end + assert(o.update_attributes(uuid: new_uuid), + "should change #{o_class} uuid from #{old_uuid} to #{new_uuid}") assert_equal(false, o_class.where(uuid: old_uuid).any?, "#{old_uuid} should disappear when renamed to #{new_uuid}") end @@ -142,21 +138,4 @@ class OwnerTest < ActiveSupport::TestCase check_permissions_against_full_refresh end - test "change uuid of User that owns self" do - o = User.create! - assert User.where(uuid: o.uuid).any?, "new User should really be in DB" - assert_equal(true, o.update_attributes(owner_uuid: o.uuid), - "setting owner to self should work") - old_uuid = o.uuid - new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9]) - o.update_uuid(new_uuid: new_uuid) - o = User.find_by_uuid(new_uuid) - assert_equal(false, User.where(uuid: old_uuid).any?, - "#{old_uuid} should not be in DB after deleting") - assert_equal(true, User.where(uuid: new_uuid).any?, - "#{new_uuid} should be in DB after renaming") - assert_equal(new_uuid, User.where(uuid: new_uuid).first.owner_uuid, - "#{new_uuid} should be its own owner in DB after renaming") - end - end diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb index f973c6ba1f..c00164c0a3 100644 --- a/services/api/test/unit/user_test.rb +++ b/services/api/test/unit/user_test.rb @@ -686,72 +686,6 @@ class UserTest < ActiveSupport::TestCase end end - [ - [:active, 'zzzzz-borkd-abcde12345abcde'], - [:active, 'zzzzz-j7d0g-abcde12345abcde'], - [:active, 'zzzzz-tpzed-borkd'], - [:system_user, 'zzzzz-tpzed-abcde12345abcde'], - [:anonymous, 'zzzzz-tpzed-abcde12345abcde'], - ].each do |fixture, new_uuid| - test "disallow update_uuid #{fixture} -> #{new_uuid}" do - u = users(fixture) - orig_uuid = u.uuid - act_as_system_user do - assert_raises do - u.update_uuid(new_uuid: new_uuid) - end - end - # "Successfully aborted orig->new" outcome looks the same as - # "successfully updated new->orig". - assert_update_success(old_uuid: new_uuid, - new_uuid: orig_uuid, - expect_owned_objects: fixture == :active) - end - end - - [:active, :spectator, :admin].each do |target| - test "update_uuid on #{target} as non-admin user" do - act_as_user users(:active) do - assert_raises(ArvadosModel::PermissionDeniedError) do - users(target).update_uuid(new_uuid: 'zzzzz-tpzed-abcde12345abcde') - end - end - end - end - - test "update_uuid to existing uuid" do - u = users(:active) - orig_uuid = u.uuid - new_uuid = users(:admin).uuid - act_as_system_user do - assert_raises do - u.update_uuid(new_uuid: new_uuid) - end - end - u.reload - assert_equal u.uuid, orig_uuid - assert_not_empty Collection.where(owner_uuid: orig_uuid) - assert_not_empty Group.where(owner_uuid: orig_uuid) - end - - [ - [:active, 'zbbbb-tpzed-abcde12345abcde'], - [:active, 'zzzzz-tpzed-abcde12345abcde'], - [:admin, 'zbbbb-tpzed-abcde12345abcde'], - [:admin, 'zzzzz-tpzed-abcde12345abcde'], - ].each do |fixture, new_uuid| - test "update_uuid #{fixture} to unused uuid #{new_uuid}" do - u = users(fixture) - orig_uuid = u.uuid - act_as_system_user do - u.update_uuid(new_uuid: new_uuid) - end - assert_update_success(old_uuid: orig_uuid, - new_uuid: new_uuid, - expect_owned_objects: fixture == :active) - end - end - def assert_update_success(old_uuid:, new_uuid:, expect_owned_objects: true) [[User, :uuid], [Link, :head_uuid], -- 2.30.2