From 67a86e26e3cc33af9ffe65d486137077b99a6944 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curii.com>
Date: Tue, 8 Mar 2022 15:23:24 -0500
Subject: [PATCH] 18691: Prevent freezing trashed project / trashing frozen
 project.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---
 services/api/app/models/group.rb     | 59 +++++++++++++++-------------
 services/api/test/unit/group_test.rb | 15 +++++++
 2 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 216397a1bb..ee8690acb5 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -116,6 +116,9 @@ class Group < ArvadosModel
       if !new_record? && !current_user.can?(manage: uuid)
         raise PermissionDeniedError
       end
+      if trash_at || delete_at || !new_record? && TrashedGroup.where(group_uuid: uuid).any?
+        errors.add(:frozen_by_uuid, "cannot be set on a trashed project")
+      end
       if frozen_by_uuid_was.nil?
         if Rails.configuration.API.FreezeProjectRequiresDescription && !attribute_present?(:description)
           errors.add(:frozen_by_uuid, "can only be set if description is non-empty")
@@ -131,36 +134,38 @@ class Group < ArvadosModel
   end
 
   def update_trash
-    if saved_change_to_trash_at? or saved_change_to_owner_uuid?
-      # The group was added or removed from the trash.
-      #
-      # Strategy:
-      #   Compute project subtree, propagating trash_at to subprojects
-      #   Remove groups that don't belong from trash
-      #   Add/update groups that do belong in the trash
-
-      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_trash_at($1, LEAST($2, $3)::timestamp)
-},
-                                               'Group.update_trash.select',
-                                               [[nil, self.uuid],
-                                                [nil, TrashedGroup.find_by_group_uuid(self.owner_uuid).andand.trash_at],
-                                                [nil, self.trash_at]]
+    return unless saved_change_to_trash_at? || saved_change_to_owner_uuid?
 
-      ActiveRecord::Base.connection.exec_delete %{
-delete from trashed_groups where group_uuid in (select target_uuid from #{temptable} where trash_at is NULL);
-},
-                                            "Group.update_trash.delete"
+    # The group was added or removed from the trash.
+    #
+    # Strategy:
+    #   Compute project subtree, propagating trash_at to subprojects
+    #   Ensure none of the newly trashed descendants were frozen (if so, bail out)
+    #   Remove groups that don't belong from trash
+    #   Add/update groups that do belong in the trash
 
-      ActiveRecord::Base.connection.exec_query %{
-insert into trashed_groups (group_uuid, trash_at)
-  select target_uuid as group_uuid, trash_at from #{temptable} where trash_at is not NULL
-on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
-},
-                                            "Group.update_trash.insert"
+    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_trash_at($1, LEAST($2, $3)::timestamp)",
+      "Group.update_trash.select",
+      [[nil, self.uuid],
+       [nil, TrashedGroup.find_by_group_uuid(self.owner_uuid).andand.trash_at],
+       [nil, self.trash_at]])
+    frozen_descendants = ActiveRecord::Base.connection.exec_query(
+      "select uuid from frozen_groups, #{temptable} where uuid = target_uuid",
+      "Group.update_trash.check_frozen")
+    if frozen_descendants.any?
+      raise ArgumentError.new("cannot trash project containing frozen project #{frozen_descendants[0]["uuid"]}")
     end
+    ActiveRecord::Base.connection.exec_delete(
+      "delete from trashed_groups where group_uuid in (select target_uuid from #{temptable} where trash_at is NULL)",
+      "Group.update_trash.delete")
+    ActiveRecord::Base.connection.exec_query(
+      "insert into trashed_groups (group_uuid, trash_at) "+
+      "select target_uuid as group_uuid, trash_at from #{temptable} where trash_at is not NULL " +
+      "on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at",
+      "Group.update_trash.insert")
   end
 
   def update_frozen
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 1a37eb058b..2c084cc887 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -364,6 +364,21 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
       assert_match /can only be set if properties\[frobity\] value is non-empty/, err.inspect
       proj.reload
 
+      # Cannot set frozen_by_uuid while project or its parent is
+      # trashed
+      [parent, proj].each do |trashed|
+        trashed.update_attributes!(trash_at: db_current_time)
+        err = assert_raises do
+          proj.update_attributes!(
+            frozen_by_uuid: users(:active).uuid,
+            description: 'ready to freeze',
+            properties: {'frobity' => 'bar baz'})
+        end
+        assert_match /cannot be set on a trashed project/, err.inspect
+        proj.reload
+        trashed.update_attributes!(trash_at: nil)
+      end
+
       # Can set frozen_by_uuid if all conditions are met
       ok = proj.update_attributes(
         frozen_by_uuid: users(:active).uuid,
-- 
2.30.2