From: Tom Clegg Date: Tue, 15 Mar 2022 06:56:09 +0000 (-0400) Subject: 18691: Refactor frozen_groups check. X-Git-Tag: 2.4.0~36^2~9 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/174c072a594e5979ed2e32fd19a749893a7e88a7 18691: Refactor frozen_groups check. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index ffae867c15..3ddbafcdb2 100644 --- 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 febb8ea516..096f5a86a4 100644 --- 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 2c084cc887..11c7da0905 100644 --- 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.