18691: Prevent changes in frozen projects.
authorTom Clegg <tom@curii.com>
Wed, 2 Mar 2022 19:32:42 +0000 (14:32 -0500)
committerTom Clegg <tom@curii.com>
Thu, 3 Mar 2022 16:34:34 +0000 (11:34 -0500)
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

services/api/app/models/arvados_model.rb
services/api/app/models/frozen_group.rb [new file with mode: 0644]
services/api/app/models/group.rb
services/api/db/migrate/20220301155729_frozen_groups.rb [new file with mode: 0644]
services/api/db/structure.sql
services/api/lib/20200501150153_permission_table_constants.rb
services/api/test/unit/group_test.rb

index 1c842c48d6bf1d283ae08608dc242967b0cc2b56..029034d8bdfdf5d8f222170f2bb8e35d0b32e99c 100644 (file)
@@ -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 (file)
index 0000000..bf4ee5d
--- /dev/null
@@ -0,0 +1,6 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class FrozenGroup < ApplicationRecord
+end
index d6f698432c86cd97ce5170ff563b687927f5d7a4..93bafc45a272f47ce0d1664d624fb9c126af2483 100644 (file)
@@ -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 (file)
index 0000000..730d051
--- /dev/null
@@ -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
index 573a4375cd98562a7933615a3173953f0de805f4..cf92bcad907b443f11953e9809f2410adf974934 100644 (file)
@@ -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');
 
 
index 74c15bc2e9381a13ae57021386d9393b35d86543..7ee5039368a282e3fabf8ec2d62f6b7f0d28c943 100644 (file)
@@ -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
index f3367118c580728cb4cb48a64a278a2fe3f1097d..d71a0b5f983ca5dccaa7a5142a6b5bea770e65c5 100644 (file)
@@ -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