[ARVADOS] updated: 2.1.0-1994-g7b070fc84

Git user git at public.arvados.org
Thu Mar 17 20:37:59 UTC 2022


Summary of changes:
 doc/api/methods/groups.html.textile.liquid |  2 +-
 services/api/app/models/arvados_model.rb   |  2 +-
 services/api/app/models/group.rb           | 16 ++++++++--
 services/api/app/models/user.rb            | 12 ++++---
 services/api/test/unit/group_test.rb       | 51 +++++++++++++++++++++++-------
 5 files changed, 62 insertions(+), 21 deletions(-)

       via  7b070fc8458f4108d44d6bfb939e36d3cc76af84 (commit)
       via  f4e31e5805e2880dd2083318cea46be17789fff9 (commit)
       via  74ba49615bbce04863068b095bd103f870c90b4f (commit)
       via  cf736bbc9cfa288cca1dbced48dd144770a37ce4 (commit)
       via  9ede4c0a5cdadd5f1b5664950146626b794a6921 (commit)
       via  0b547182f8d7225e4ebefb90eaf33b5136051e23 (commit)
      from  c3d04aeb81a04b5dc527af8f9297e9fefb5f4851 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit 7b070fc8458f4108d44d6bfb939e36d3cc76af84
Author: Tom Clegg <tom at curii.com>
Date:   Thu Mar 17 15:55:11 2022 -0400

    18691: Disallow freezing projects with active container requests.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index f56c3281d..b1b2e942c 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -176,6 +176,18 @@ class Group < ArvadosModel
       "Group.update_frozen.select",
       [[nil, self.uuid],
        [nil, !self.frozen_by_uuid.nil?]])
+    if frozen_by_uuid
+      rows = ActiveRecord::Base.connection.exec_query(
+        "select cr.uuid, cr.state from container_requests cr, #{temptable} frozen " +
+        "where cr.owner_uuid = frozen.uuid and frozen.is_frozen " +
+        "and cr.state not in ($1, $2) limit 1",
+        "Group.update_frozen.check_container_requests",
+        [[nil, ContainerRequest::Uncommitted],
+         [nil, ContainerRequest::Final]])
+      if rows.any?
+        raise ArgumentError.new("cannot freeze project containing container request #{rows.first['uuid']} with state = #{rows.first['state']}")
+      end
+    end
     ActiveRecord::Base.connection.exec_delete(
       "delete from frozen_groups where uuid in (select uuid from #{temptable} where not is_frozen)",
       "Group.update_frozen.delete")
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 1f1d37908..7a1696240 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -317,6 +317,18 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
   test "freeze project" do
     act_as_user users(:active) do
       Rails.configuration.API.UnfreezeProjectRequiresAdmin = false
+
+      test_cr_attrs = {
+        command: ["echo", "foo"],
+        container_image: links(:docker_image_collection_tag).name,
+        cwd: "/tmp",
+        environment: {},
+        mounts: {"/out" => {"kind" => "tmp", "capacity" => 1000000}},
+        output_path: "/out",
+        runtime_constraints: {"vcpus" => 1, "ram" => 2},
+        name: "foo",
+        description: "bar",
+      }
       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)
@@ -398,18 +410,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
         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",
-          environment: {},
-          mounts: {"/out" => {"kind" => "tmp", "capacity" => 1000000}},
-          output_path: "/out",
-          runtime_constraints: {"vcpus" => 1, "ram" => 2},
-          name: "foo",
-          description: "bar",
-          owner_uuid: frozen.uuid,
-        )
+        cr = ContainerRequest.new(test_cr_attrs.merge(owner_uuid: frozen.uuid))
         assert_raises ArvadosModel::PermissionDeniedError do
           cr.save
         end
@@ -509,6 +510,22 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
         # Admin can unfreeze.
         assert proj.update_attributes(frozen_by_uuid: nil), proj.errors.messages
       end
+
+      # Cannot freeze a project if it contains container requests in
+      # Committed state (this would cause operations on the relevant
+      # Containers to fail when syncing container request state)
+      creq_uncommitted = ContainerRequest.create!(test_cr_attrs.merge(owner_uuid: proj_inner.uuid))
+      creq_committed = ContainerRequest.create!(test_cr_attrs.merge(owner_uuid: proj_inner.uuid, state: 'Committed'))
+      err = assert_raises do
+        proj.update_attributes!(frozen_by_uuid: users(:active).uuid)
+      end
+      assert_match /container request zzzzz-xvhdp-.* with state = Committed/, err.inspect
+      proj.reload
+
+      # Can freeze once all container requests are in Uncommitted or
+      # Final state
+      creq_committed.update_attributes!(state: ContainerRequest::Final)
+      assert proj.update_attributes(frozen_by_uuid: users(:active).uuid)
     end
   end
 end

commit f4e31e5805e2880dd2083318cea46be17789fff9
Author: Tom Clegg <tom at curii.com>
Date:   Thu Mar 17 15:19:06 2022 -0400

    18691: Clarify that frozen projects cannot be moved.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index 4d57079b2..2a762d924 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -34,7 +34,7 @@ table(table table-bordered table-condensed).
 |trash_at|datetime|If @trash_at@ is non-null and in the past, this group and all objects directly or indirectly owned by the group will be hidden from API calls.  May be untrashed.||
 |delete_at|datetime|If @delete_at@ is non-null and in the past, the group and all objects directly or indirectly owned by the group may be permanently deleted.||
 |is_trashed|datetime|True if @trash_at@ is in the past, false if not.||
-|frozen_by_uuid|string|For a frozen project, indicates the user who froze the project; null in all other cases. When a project is frozen, no further changes can be made to the project or its contents, even by admins. Attempting to add new items or modify, rename, trash, or delete the project or its contents, including any subprojects, will return an error.||
+|frozen_by_uuid|string|For a frozen project, indicates the user who froze the project; null in all other cases. When a project is frozen, no further changes can be made to the project or its contents, even by admins. Attempting to add new items or modify, rename, move, trash, or delete the project or its contents, including any subprojects, will return an error.||
 
 h3. Frozen projects
 

commit 74ba49615bbce04863068b095bd103f870c90b4f
Author: Tom Clegg <tom at curii.com>
Date:   Thu Mar 17 15:12:46 2022 -0400

    18691: Add parens to clarify operator precedence.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index d2c332b9b..f56c3281d 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -116,7 +116,7 @@ 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?
+      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?

commit cf736bbc9cfa288cca1dbced48dd144770a37ce4
Author: Tom Clegg <tom at curii.com>
Date:   Thu Mar 17 14:56:04 2022 -0400

    18691: Go back to old attribute_was method for consistency.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index c71996a37..327bf63b5 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -656,7 +656,7 @@ class ArvadosModel < ApplicationRecord
       # 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) ?
+      unless ((respond_to?(:frozen_by_uuid) && frozen_by_uuid_was && !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"
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index ee8690acb..d2c332b9b 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -248,7 +248,7 @@ class Group < ArvadosModel
   def permission_to_update
     if !super
       return false
-    elsif frozen_by_uuid && frozen_by_uuid_in_database
+    elsif frozen_by_uuid && frozen_by_uuid_was
       errors.add :uuid, "#{uuid} is frozen and cannot be modified"
       return false
     else

commit 9ede4c0a5cdadd5f1b5664950146626b794a6921
Author: Tom Clegg <tom at curii.com>
Date:   Thu Mar 17 14:26:58 2022 -0400

    18691: Make it more obvious that unfreeze requires :manage.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 096f5a86a..44e6ca757 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -86,7 +86,7 @@ class User < ArvadosModel
   VAL_FOR_PERM =
     {:read => 1,
      :write => 2,
-     :unfreeze => 2,
+     :unfreeze => 3,
      :manage => 3}
 
 
@@ -148,10 +148,12 @@ SELECT 1 FROM #{PERMISSION_VIEW}
           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.
+        # "unfreeze" permission means "can write, but only if
+        # explicitly un-freezing at the same time" (see
+        # ArvadosModel#ensure_owner_uuid_is_permitted). If the
+        # permission query above passed the permission level of
+        # :unfreeze (which is the same as :manage), and the parent
+        # isn't also frozen, then un-freeze is allowed.
         if FrozenGroup.where(uuid: target_owner_uuid).any?
           return false
         end

commit 0b547182f8d7225e4ebefb90eaf33b5136051e23
Author: Tom Clegg <tom at curii.com>
Date:   Thu Mar 17 14:20:07 2022 -0400

    18691: Test that write permission is insufficient to unfreeze.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 11c7da090..1f1d37908 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -460,6 +460,16 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
         end
       end
 
+      # User with write permission (but not manage) cannot unfreeze
+      act_as_user users(:spectator) do
+        # First confirm we have write permission on the parent project
+        assert Collection.create(name: 'bar', owner_uuid: parent.uuid)
+        assert_raises(ArvadosModel::PermissionDeniedError) do
+          proj.update_attributes!(frozen_by_uuid: nil)
+        end
+      end
+      proj.reload
+
       # User with manage permission can unfreeze, then create items
       # inside it and its children
       assert proj.update_attributes(frozen_by_uuid: nil)

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list