From 27f3423ab974547d8ff666e7d9f9af7aec933765 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Tue, 2 Jun 2020 16:45:21 -0400 Subject: [PATCH] 16007: Add migration to fix invalid groups & permission links Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- .../20200602141328_fix_roles_projects.rb | 13 ++++ services/api/lib/fix_roles_projects.rb | 63 +++++++++++++++++++ services/api/test/fixtures/links.yml | 14 ----- services/api/test/unit/group_test.rb | 50 +++++++++++++++ 4 files changed, 126 insertions(+), 14 deletions(-) create mode 100644 services/api/db/migrate/20200602141328_fix_roles_projects.rb create mode 100644 services/api/lib/fix_roles_projects.rb diff --git a/services/api/db/migrate/20200602141328_fix_roles_projects.rb b/services/api/db/migrate/20200602141328_fix_roles_projects.rb new file mode 100644 index 0000000000..f9b2bd6faf --- /dev/null +++ b/services/api/db/migrate/20200602141328_fix_roles_projects.rb @@ -0,0 +1,13 @@ +# Copyright (C) The Arvados Authors. All rights reserved. +# +# SPDX-License-Identifier: AGPL-3.0 + +require 'fix_roles_projects' + +class FixRolesProjects < ActiveRecord::Migration[5.0] + def change + # This migration is not reversible. However, the results are + # backwards compatible. + fix_roles_projects + end +end diff --git a/services/api/lib/fix_roles_projects.rb b/services/api/lib/fix_roles_projects.rb new file mode 100644 index 0000000000..dafef61aa9 --- /dev/null +++ b/services/api/lib/fix_roles_projects.rb @@ -0,0 +1,63 @@ +# Copyright (C) The Arvados Authors. All rights reserved. +# +# SPDX-License-Identifier: AGPL-3.0 + +def fix_roles_projects + # This migration is not reversible. However, the behavior it + # enforces is backwards-compatible, and most of the time there + # shouldn't be anything to do at all. + act_as_system_user do + ActiveRecord::Base.transaction do + q = ActiveRecord::Base.connection.exec_query %{ +select uuid from groups limit 1 +} + + # 1) any group not group_class != project becomes a 'role' (both empty and invalid groups) + ActiveRecord::Base.connection.exec_query %{ +UPDATE groups set group_class='role' where group_class != 'project' or group_class is null + } + + Group.where(group_class: "role").each do |g| + if g.owner_uuid != system_user_uuid + # 2) Ownership of a role becomes a can_manage link + Link.create!(link_class: 'permission', + name: 'can_manage', + tail_uuid: g.owner_uuid, + head_uuid: g.uuid) + g.owner_uuid = system_user_uuid + g.save_with_unique_name! + end + + # 3) If a role owns anything, give it to system user and it + # becomes a can_manage link + ActiveRecord::Base.descendants.reject(&:abstract_class?).each do |klass| + next if [ApiClientAuthorization, + AuthorizedKey, + Log].include?(klass) + next if !klass.columns.collect(&:name).include?('owner_uuid') + + klass.where(owner_uuid: g.uuid).each do |owned| + Link.create!(link_class: 'permission', + name: 'can_manage', + tail_uuid: g.uuid, + head_uuid: owned.uuid) + owned.owner_uuid = system_user_uuid + owned.save_with_unique_name! + end + end + end + + # 4) Projects can't have outgoing permission links. Just delete them. + q = ActiveRecord::Base.connection.exec_query %{ +select links.uuid from links, groups where groups.uuid = links.tail_uuid and + links.link_class = 'permission' and groups.group_class = 'project' +} + q.each do |lu| + ln = Link.find_by_uuid(lu['uuid']) + puts "Projects cannot have outgoing permission links, '#{ln.name}' link from #{ln.tail_uuid} to #{ln.head_uuid} will be removed" + Rails.logger.warn "Destroying invalid permission link from project #{ln.tail_uuid} to #{ln.head_uuid}" + ln.destroy! + end + end + end +end diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml index 4293b0466e..b30d1edc25 100644 --- a/services/api/test/fixtures/links.yml +++ b/services/api/test/fixtures/links.yml @@ -198,20 +198,6 @@ foo_file_readable_by_active_redundant_permission_via_private_group: head_uuid: zzzzz-4zz18-znfnqtbbv4spc3w properties: {} -foo_file_readable_by_aproject: - uuid: zzzzz-o0j2j-fp1d8395ldqw22p - owner_uuid: zzzzz-tpzed-000000000000000 - created_at: 2014-01-24 20:42:26 -0800 - modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr - modified_by_user_uuid: zzzzz-tpzed-000000000000000 - modified_at: 2014-01-24 20:42:26 -0800 - updated_at: 2014-01-24 20:42:26 -0800 - tail_uuid: zzzzz-j7d0g-v955i6s2oi1cbso - link_class: permission - name: can_read - head_uuid: zzzzz-4zz18-znfnqtbbv4spc3w - properties: {} - bar_file_readable_by_active: uuid: zzzzz-o0j2j-8hppiuduf8eqdng owner_uuid: zzzzz-tpzed-000000000000000 diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb index 631e0137c1..3d1fda927f 100644 --- a/services/api/test/unit/group_test.rb +++ b/services/api/test/unit/group_test.rb @@ -3,6 +3,7 @@ # SPDX-License-Identifier: AGPL-3.0 require 'test_helper' +require 'fix_roles_projects' class GroupTest < ActiveSupport::TestCase @@ -293,4 +294,53 @@ class GroupTest < ActiveSupport::TestCase end end end + + def insert_group uuid, owner_uuid, name, group_class + q = ActiveRecord::Base.connection.exec_query %{ +insert into groups (uuid, owner_uuid, name, group_class, created_at, updated_at) + values ('#{uuid}', '#{owner_uuid}', + '#{name}', #{if group_class then "'"+group_class+"'" else 'NULL' end}, + statement_timestamp(), statement_timestamp()) +} + uuid + end + + test "migration to fix roles and projects" do + g1 = insert_group Group.generate_uuid, system_user_uuid, 'group with no class', nil + g2 = insert_group Group.generate_uuid, users(:active).uuid, 'role owned by a user', 'role' + + g3 = insert_group Group.generate_uuid, system_user_uuid, 'role that owns a project', 'role' + g4 = insert_group Group.generate_uuid, g3, 'the project', 'project' + + g5 = insert_group Group.generate_uuid, users(:active).uuid, 'a project with an outgoing permission link', 'project' + + g6 = insert_group Group.generate_uuid, system_user_uuid, 'name collision', 'role' + g7 = insert_group Group.generate_uuid, users(:active).uuid, 'name collision', 'role' + + refresh_permissions + + act_as_system_user do + l1 = Link.create!(link_class: 'permission', name: 'can_manage', tail_uuid: g3, head_uuid: g4) + q = ActiveRecord::Base.connection.exec_query %{ +update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}' +} + refresh_permissions + end + + assert_equal nil, Group.find_by_uuid(g1).group_class + assert_equal users(:active).uuid, Group.find_by_uuid(g2).owner_uuid + assert_equal g3, Group.find_by_uuid(g4).owner_uuid + assert !Link.where(tail_uuid: users(:active).uuid, head_uuid: g2, link_class: "permission", name: "can_manage").any? + assert !Link.where(tail_uuid: g3, head_uuid: g4, link_class: "permission", name: "can_manage").any? + assert Link.where(link_class: 'permission', name: 'can_manage', tail_uuid: g5, head_uuid: g4).any? + + fix_roles_projects + + assert_equal 'role', Group.find_by_uuid(g1).group_class + assert_equal system_user_uuid, Group.find_by_uuid(g2).owner_uuid + assert_equal system_user_uuid, Group.find_by_uuid(g4).owner_uuid + assert Link.where(tail_uuid: users(:active).uuid, head_uuid: g2, link_class: "permission", name: "can_manage").any? + assert Link.where(tail_uuid: g3, head_uuid: g4, link_class: "permission", name: "can_manage").any? + assert !Link.where(link_class: 'permission', name: 'can_manage', tail_uuid: g5, head_uuid: g4).any? + end end -- 2.39.5