18691: Refactor frozen_groups check.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index ffae867c158a5772f231962d5b6887ca7262a60c..3ddbafcdb234fc9afddb8392c0860135711b6ff3 100644 (file)
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -629,8 +629,12 @@ class ArvadosModel < ApplicationRecord
         if check_uuid.nil?
           # old_owner_uuid is nil? New record, no need to check.
         elsif !current_user.can?(write: check_uuid)
-          logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
-          errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
+          if FrozenGroup.where(uuid: check_uuid).any?
+            errors.add :owner_uuid, "cannot be set or changed because #{which} owner is frozen"
+          else
+            logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
+            errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
+          end
           raise PermissionDeniedError
         elsif rsc_class == Group && Group.find_by_uuid(owner_uuid).group_class != "project"
           errors.add :owner_uuid, "must be a project"
@@ -640,8 +644,12 @@ class ArvadosModel < ApplicationRecord
     else
       # If the object already existed and we're not changing
       # owner_uuid, we only need write permission on the object
-      # itself.
-      if !current_user.can?(write: self.uuid)
+      # itself. (If we're in the act of unfreezing, we only need
+      # :unfreeze permission, which means "what write permission would
+      # be if target weren't frozen")
+      unless ((respond_to?(:frozen_by_uuid) && frozen_by_uuid_in_database && !frozen_by_uuid) ?
+                current_user.can?(unfreeze: uuid) :
+                current_user.can?(write: uuid))
         logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} without write permission"
         errors.add :uuid, " #{uuid} is not writable by #{current_user.uuid}"
         raise PermissionDeniedError
@@ -658,14 +666,7 @@ class ArvadosModel < ApplicationRecord
   end
 
   def permission_to_create
-    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
+    return current_user.andand.is_active
   end
 
   def permission_to_update
@@ -682,13 +683,6 @@ 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/user.rb b/services/api/app/models/user.rb
index febb8ea51611eb5a21549b8133770fe059a2ca5c..096f5a86a4816b3ef889533ceea7e07cfe9301ac 100644 (file)
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -86,6 +86,7 @@ class User < ArvadosModel
   VAL_FOR_PERM =
     {:read => 1,
      :write => 2,
+     :unfreeze => 2,
      :manage => 3}
 
 
@@ -140,6 +141,21 @@ SELECT 1 FROM #{PERMISSION_VIEW}
                   ).any?
         return false
       end
+
+      if action == :write
+        if FrozenGroup.where(uuid: [target_uuid, target_owner_uuid]).any?
+          # self or parent is frozen
+          return false
+        end
+      elsif action == :unfreeze
+        # "unfreeze" permission means "could write if target weren't
+        # frozen", which is relevant when a user is un-freezing a
+        # project. If the permission query above allows :write, and
+        # the parent isn't also frozen, then un-freeze is allowed.
+        if FrozenGroup.where(uuid: target_owner_uuid).any?
+          return false
+        end
+      end
     end
     true
   end

diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 2c084cc887f91a4c1af1ec9cd611b2d054069868..11c7da0905ab2009771006fcc7564af290cdbf28 100644 (file)
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -320,7 +320,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
       parent = Group.create!(group_class: 'project', name: 'freeze-test-parent', owner_uuid: users(:active).uuid)
       proj = Group.create!(group_class: 'project', name: 'freeze-test', owner_uuid: parent.uuid)
       proj_inner = Group.create!(group_class: 'project', name: 'freeze-test-inner', owner_uuid: proj.uuid)
-      coll = Collection.create!(name: 'foo', manifest_text: '', owner_uuid: proj_inner.uuid)
+      coll = Collection.create!(name: 'freeze-test-collection', manifest_text: '', owner_uuid: proj_inner.uuid)
 
       # Cannot set frozen_by_uuid to a different user
       assert_raises do
@@ -423,7 +423,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
       # Once project is frozen, cannot change name/contents, move,
       # 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
+        assert_raises(StandardError, "should reject rename of #{frozen.uuid} (#{frozen.name}) with parent #{frozen.owner_uuid}") do
           frozen.update_attributes!(name: 'foo2')
         end
         frozen.reload
@@ -486,6 +486,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
         parent.update_attributes!(delete_at: db_current_time)
       end
       parent.reload
+      assert_nil parent.frozen_by_uuid
 
       act_as_user users(:admin) do
         # Even admin cannot change frozen_by_uuid to someone else's UUID.