From a74c5dc699722769537455504d2f59455100cd29 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Wed, 2 Mar 2022 14:32:42 -0500 Subject: [PATCH] 18691: Prevent changes in frozen projects. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- services/api/app/models/arvados_model.rb | 16 ++++- services/api/app/models/frozen_group.rb | 6 ++ services/api/app/models/group.rb | 42 ++++++++++-- .../migrate/20220301155729_frozen_groups.rb | 39 +++++++++++ services/api/db/structure.sql | 37 +++++++++- ...200501150153_permission_table_constants.rb | 20 +++++- services/api/test/unit/group_test.rb | 67 +++++++++++-------- 7 files changed, 190 insertions(+), 37 deletions(-) create mode 100644 services/api/app/models/frozen_group.rb create mode 100644 services/api/db/migrate/20220301155729_frozen_groups.rb diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 1c842c48d6..029034d8bd 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -643,7 +643,14 @@ class ArvadosModel < ApplicationRecord end def permission_to_create - current_user.andand.is_active + if !current_user.andand.is_active + return false + end + if self.respond_to?(:owner_uuid) && FrozenGroup.where(uuid: owner_uuid).any? + errors.add :owner_uuid, "#{owner_uuid} is frozen" + return false + end + return true end def permission_to_update @@ -660,6 +667,13 @@ class ArvadosModel < ApplicationRecord logger.warn "User #{current_user.uuid} tried to change uuid of #{self.class.to_s} #{self.uuid_was} to #{self.uuid}" return false end + if self.respond_to?(:owner_uuid) + frozen = FrozenGroup.where(uuid: [owner_uuid, owner_uuid_in_database]).to_a + if frozen.any? + errors.add :uuid, "#{uuid} cannot be modified in frozen project #{frozen[0]}" + return false + end + end return true end diff --git a/services/api/app/models/frozen_group.rb b/services/api/app/models/frozen_group.rb new file mode 100644 index 0000000000..bf4ee5d0bd --- /dev/null +++ b/services/api/app/models/frozen_group.rb @@ -0,0 +1,6 @@ +# Copyright (C) The Arvados Authors. All rights reserved. +# +# SPDX-License-Identifier: AGPL-3.0 + +class FrozenGroup < ApplicationRecord +end diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb index d6f698432c..93bafc45a2 100644 --- a/services/api/app/models/group.rb +++ b/services/api/app/models/group.rb @@ -23,6 +23,7 @@ class Group < ArvadosModel before_create :assign_name after_create :after_ownership_change after_create :update_trash + after_create :update_frozen before_update :before_ownership_change after_update :after_ownership_change @@ -30,7 +31,8 @@ class Group < ArvadosModel after_create :add_role_manage_link after_update :update_trash - before_destroy :clear_permissions_and_trash + after_update :update_frozen + before_destroy :clear_permissions_trash_frozen api_accessible :user, extend: :common do |t| t.add :name @@ -161,6 +163,22 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at; end end + def update_frozen + return unless saved_change_to_frozen_by_uuid? || saved_change_to_owner_uuid? + temptable = "group_subtree_#{rand(2**64).to_s(10)}" + ActiveRecord::Base.connection.exec_query( + "create temporary table #{temptable} on commit drop as select * from project_subtree_with_is_frozen($1,$2)", + "Group.update_frozen.select", + [[nil, self.uuid], + [nil, !self.frozen_by_uuid.nil?]]) + ActiveRecord::Base.connection.exec_delete( + "delete from frozen_groups where uuid in (select uuid from #{temptable} where not is_frozen)", + "Group.update_frozen.delete") + ActiveRecord::Base.connection.exec_query( + "insert into frozen_groups (uuid) select uuid from #{temptable} where is_frozen on conflict do nothing", + "Group.update_frozen.insert") + end + def before_ownership_change if owner_uuid_changed? and !self.owner_uuid_was.nil? MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all @@ -174,12 +192,13 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at; end end - def clear_permissions_and_trash + def clear_permissions_trash_frozen MaterializedPermission.where(target_uuid: uuid).delete_all - ActiveRecord::Base.connection.exec_delete %{ -delete from trashed_groups where group_uuid=$1 -}, "Group.clear_permissions_and_trash", [[nil, self.uuid]] - + ['trashed_groups', 'frozen_groups'].each do |table| + ActiveRecord::Base.connection.exec_delete %{ + delete from #{table} where group_uuid=$1 + }, "Group.clear_permissions_trash_frozen", [[nil, self.uuid]] + end end def assign_name @@ -217,4 +236,15 @@ delete from trashed_groups where group_uuid=$1 end end end + + def permission_to_update + if !super + return false + elsif frozen_by_uuid && frozen_by_uuid_in_database + errors.add :uuid, "#{uuid} is frozen and cannot be modified" + return false + else + return true + end + end end diff --git a/services/api/db/migrate/20220301155729_frozen_groups.rb b/services/api/db/migrate/20220301155729_frozen_groups.rb new file mode 100644 index 0000000000..730d051ce2 --- /dev/null +++ b/services/api/db/migrate/20220301155729_frozen_groups.rb @@ -0,0 +1,39 @@ +# Copyright (C) The Arvados Authors. All rights reserved. +# +# SPDX-License-Identifier: AGPL-3.0 + +require '20200501150153_permission_table_constants' + +class FrozenGroups < ActiveRecord::Migration[5.0] + def up + create_table :frozen_groups, :id => false do |t| + t.string :uuid + end + add_index :frozen_groups, :uuid, :unique => true + + ActiveRecord::Base.connection.execute %{ +create or replace function project_subtree_with_is_frozen (starting_uuid varchar(27), starting_is_frozen boolean) +returns table (uuid varchar(27), is_frozen boolean) +STABLE +language SQL +as $$ +WITH RECURSIVE + project_subtree(uuid, is_frozen) as ( + values (starting_uuid, starting_is_frozen) + union + select groups.uuid, project_subtree.is_frozen or groups.frozen_by_uuid is not null + from groups join project_subtree on project_subtree.uuid = groups.owner_uuid + ) + select uuid, is_frozen from project_subtree; +$$; +} + + # Initialize the table. After this, it is updated incrementally. + # See app/models/group.rb#update_frozen_groups + refresh_frozen + end + + def down + drop_table :frozen_groups + end +end diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql index 573a4375cd..cf92bcad90 100644 --- a/services/api/db/structure.sql +++ b/services/api/db/structure.sql @@ -190,6 +190,24 @@ case (edges.edge_id = perm_edge_id) $$; +-- +-- Name: project_subtree_with_is_frozen(character varying, boolean); Type: FUNCTION; Schema: public; Owner: - +-- + +CREATE FUNCTION public.project_subtree_with_is_frozen(starting_uuid character varying, starting_is_frozen boolean) RETURNS TABLE(uuid character varying, is_frozen boolean) + LANGUAGE sql STABLE + AS $$ +WITH RECURSIVE + project_subtree(uuid, is_frozen) as ( + values (starting_uuid, starting_is_frozen) + union + select groups.uuid, project_subtree.is_frozen or groups.frozen_by_uuid is not null + from groups join project_subtree on (groups.owner_uuid = project_subtree.uuid) + ) + select uuid, is_frozen from project_subtree; +$$; + + -- -- Name: project_subtree_with_trash_at(character varying, timestamp without time zone); Type: FUNCTION; Schema: public; Owner: - -- @@ -548,6 +566,15 @@ CREATE SEQUENCE public.containers_id_seq ALTER SEQUENCE public.containers_id_seq OWNED BY public.containers.id; +-- +-- Name: frozen_groups; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.frozen_groups ( + uuid character varying +); + + -- -- Name: groups; Type: TABLE; Schema: public; Owner: - -- @@ -2059,6 +2086,13 @@ CREATE INDEX index_containers_on_secret_mounts_md5 ON public.containers USING bt CREATE UNIQUE INDEX index_containers_on_uuid ON public.containers USING btree (uuid); +-- +-- Name: index_frozen_groups_on_uuid; Type: INDEX; Schema: public; Owner: - +-- + +CREATE UNIQUE INDEX index_frozen_groups_on_uuid ON public.frozen_groups USING btree (uuid); + + -- -- Name: index_groups_on_created_at; Type: INDEX; Schema: public; Owner: - -- @@ -3149,6 +3183,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20210621204455'), ('20210816191509'), ('20211027154300'), -('20220224203102'); +('20220224203102'), +('20220301155729'); diff --git a/services/api/lib/20200501150153_permission_table_constants.rb b/services/api/lib/20200501150153_permission_table_constants.rb index 74c15bc2e9..7ee5039368 100644 --- a/services/api/lib/20200501150153_permission_table_constants.rb +++ b/services/api/lib/20200501150153_permission_table_constants.rb @@ -15,8 +15,8 @@ # update_permissions reference that file instead. PERMISSION_VIEW = "materialized_permissions" - TRASHED_GROUPS = "trashed_groups" +FROZEN_GROUPS = "frozen_groups" # We need to use this parameterized query in a few different places, # including as a subquery in a larger query. @@ -83,3 +83,21 @@ INSERT INTO materialized_permissions }, "refresh_permission_view.do" end end + +def refresh_frozen + ActiveRecord::Base.transaction do + ActiveRecord::Base.connection.execute("LOCK TABLE #{FROZEN_GROUPS}") + ActiveRecord::Base.connection.execute("DELETE FROM #{FROZEN_GROUPS}") + + # Compute entire frozen_groups table, starting with top-level + # projects (i.e., project groups owned by a user). + ActiveRecord::Base.connection.execute(%{ +INSERT INTO #{FROZEN_GROUPS} +select ps.uuid from groups, + lateral project_subtree_with_is_frozen(groups.uuid, groups.frozen_by_uuid is not null) ps + where groups.owner_uuid like '_____-tpzed-_______________' + and group_class = 'project' + and ps.is_frozen +}) + end +end diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb index f3367118c5..d71a0b5f98 100644 --- a/services/api/test/unit/group_test.rb +++ b/services/api/test/unit/group_test.rb @@ -319,7 +319,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}' Rails.configuration.API.UnfreezeProjectRequiresAdmin = false proj = Group.create!(group_class: 'project', name: 'freeze-test', owner_uuid: users(:active).uuid) proj_inner = Group.create!(group_class: 'project', name: 'freeze-test-inner', owner_uuid: proj.uuid) - coll = Collection.create!(name: 'foo', manifest_text: '') + coll = Collection.create!(name: 'foo', manifest_text: '', owner_uuid: proj_inner.uuid) # Cannot set frozen_by_uuid to a different user assert_raises do @@ -357,35 +357,19 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}' properties: {'frobity' => 'bar baz'}) assert ok, proj.errors.messages.inspect - # Once project is frozen, cannot change name/contents, trash, or - # delete the project or anything beneath it - [proj, proj_inner, coll].each do |frozen| - assert_raises do - frozen.update_attributes!(name: 'foo2') - end - if frozen.is_a?(Collection) - assert_raises do - frozen.update_attributes!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo\n") - end - end - assert_raises do - frozen.update_attributes!(trash_at: db_current_time) - end - assert_raises do - frozen.destroy - end - end - # Once project is frozen, cannot create new items inside it or # its descendants - [proj, proj_inner].each do |parent| + [proj, proj_inner].each do |frozen| assert_raises do - Collection.create!(owner_uuid: parent.uuid, name: 'inside-frozen-project') + collections(:collection_owned_by_active).update_attributes!(owner_uuid: frozen.uuid) end assert_raises do - Group.create!(owner_uuid: parent.uuid, group_class: 'project', name: 'inside-frozen-project') + Collection.create!(owner_uuid: frozen.uuid, name: 'inside-frozen-project') end - cr = ContainerRequest.create( + assert_raises do + Group.create!(owner_uuid: frozen.uuid, group_class: 'project', name: 'inside-frozen-project') + end + cr = ContainerRequest.new( command: ["echo", "foo"], container_image: links(:docker_image_collection_tag).name, cwd: "/tmp", @@ -395,12 +379,39 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}' runtime_constraints: {"vcpus" => 1, "ram" => 2}, name: "foo", description: "bar", - owner_uuid: parent.uuid, + owner_uuid: frozen.uuid, ) - assert_raises do - cr.save! + assert_raises ArvadosModel::PermissionDeniedError do + cr.save + end + assert_match /frozen/, cr.errors.inspect + # Check the frozen-parent condition is the only reason save failed. + cr.owner_uuid = users(:active).uuid + assert cr.save + cr.destroy + end + + # Once project is frozen, cannot change name/contents, trash, or + # delete the project or anything beneath it + [proj, proj_inner, coll].each do |frozen| + assert_raises(StandardError, "should reject rename of #{frozen.uuid} with parent #{frozen.owner_uuid}") do + frozen.update_attributes!(name: 'foo2') + end + frozen.reload + if frozen.is_a?(Collection) + assert_raises(StandardError, "should reject manifest change of #{frozen.uuid}") do + frozen.update_attributes!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo\n") + end + frozen.reload + end + assert_raises(StandardError, "should reject setting trash_at of #{frozen.uuid}") do + frozen.update_attributes!(trash_at: db_current_time) + end + frozen.reload + assert_raises(StandardError, "should reject delete of #{frozen.uuid}") do + frozen.destroy end - assert_match cr.errors.inspect, /frozen/ + frozen.reload end # User with manage permission can unfreeze, then create items -- 2.30.2