From b586b7eece173fe0cf0ce5e53aa8969f1239ef16 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Mon, 28 Feb 2022 11:32:39 -0500 Subject: [PATCH] 18691: Add groups.frozen_by_uuid attribute. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- lib/config/config.default.yml | 12 ++ sdk/go/arvados/config.go | 33 ++--- services/api/app/models/group.rb | 36 ++++++ ...0224203102_add_frozen_by_uuid_to_groups.rb | 9 ++ services/api/db/structure.sql | 6 +- services/api/test/unit/group_test.rb | 120 ++++++++++++++++++ 6 files changed, 199 insertions(+), 17 deletions(-) create mode 100644 services/api/db/migrate/20220224203102_add_frozen_by_uuid_to_groups.rb diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index 9800be7047..656385cc1c 100644 --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@ -240,6 +240,18 @@ Clusters: # https://doc.arvados.org/admin/metadata-vocabulary.html VocabularyPath: "" + # If true, a project must have a non-empty description field in + # order to be frozen. + FreezeProjectRequiresDescription: false + + # Project properties that must have non-empty values in order to + # freeze a project. Example: {"property_name": true} + FreezeProjectRequiresProperties: {} + + # If true, only an admin user can un-freeze a project. If false, + # any user with "manage" permission can un-freeze. + UnfreezeProjectRequiresAdmin: false + Users: # Config parameters to automatically setup new users. If enabled, # this users will be able to self-activate. Enable this if you want diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index b8c8269f12..e0750bd8c5 100644 --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@ -94,21 +94,24 @@ type Cluster struct { PostgreSQL PostgreSQL API struct { - AsyncPermissionsUpdateInterval Duration - DisabledAPIs StringSet - MaxIndexDatabaseRead int - MaxItemsPerResponse int - MaxConcurrentRequests int - MaxKeepBlobBuffers int - MaxRequestAmplification int - MaxRequestSize int - MaxTokenLifetime Duration - RequestTimeout Duration - SendTimeout Duration - WebsocketClientEventQueue int - WebsocketServerEventQueue int - KeepServiceRequestTimeout Duration - VocabularyPath string + AsyncPermissionsUpdateInterval Duration + DisabledAPIs StringSet + MaxIndexDatabaseRead int + MaxItemsPerResponse int + MaxConcurrentRequests int + MaxKeepBlobBuffers int + MaxRequestAmplification int + MaxRequestSize int + MaxTokenLifetime Duration + RequestTimeout Duration + SendTimeout Duration + WebsocketClientEventQueue int + WebsocketServerEventQueue int + KeepServiceRequestTimeout Duration + VocabularyPath string + FreezeProjectRequiresDescription bool + FreezeProjectRequiresProperties StringSet + UnfreezeProjectRequiresAdmin bool } AuditLogs struct { MaxAge Duration diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb index 8565b2a417..d6f698432c 100644 --- a/services/api/app/models/group.rb +++ b/services/api/app/models/group.rb @@ -19,6 +19,7 @@ class Group < ArvadosModel validate :ensure_filesystem_compatible_name validate :check_group_class validate :check_filter_group_filters + validate :check_frozen_state_change_allowed before_create :assign_name after_create :after_ownership_change after_create :update_trash @@ -40,6 +41,7 @@ class Group < ArvadosModel t.add :trash_at t.add :is_trashed t.add :properties + t.add :frozen_by_uuid end def ensure_filesystem_compatible_name @@ -92,6 +94,40 @@ class Group < ArvadosModel end end + def check_frozen_state_change_allowed + if frozen_by_uuid == "" + self.frozen_by_uuid = nil + end + if frozen_by_uuid_changed? || (new_record? && frozen_by_uuid) + if group_class != "project" + errors.add(:frozen_by_uuid, "cannot be modified on a non-project group") + return + end + if frozen_by_uuid_was && Rails.configuration.API.UnfreezeProjectRequiresAdmin && !current_user.is_admin + errors.add(:frozen_by_uuid, "can only be changed by an admin user, once set") + return + end + if frozen_by_uuid && frozen_by_uuid != current_user.uuid + errors.add(:frozen_by_uuid, "can only be set to the current user's UUID") + return + end + if !new_record? && !current_user.can?(manage: uuid) + raise PermissionDeniedError + 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") + end + Rails.configuration.API.FreezeProjectRequiresProperties.andand.each do |key, _| + key = key.to_s + if !properties[key] || properties[key] == "" + errors.add(:frozen_by_uuid, "can only be set if properties[#{key}] value is non-empty") + end + end + end + end + 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. diff --git a/services/api/db/migrate/20220224203102_add_frozen_by_uuid_to_groups.rb b/services/api/db/migrate/20220224203102_add_frozen_by_uuid_to_groups.rb new file mode 100644 index 0000000000..d730b25350 --- /dev/null +++ b/services/api/db/migrate/20220224203102_add_frozen_by_uuid_to_groups.rb @@ -0,0 +1,9 @@ +# Copyright (C) The Arvados Authors. All rights reserved. +# +# SPDX-License-Identifier: AGPL-3.0 + +class AddFrozenByUuidToGroups < ActiveRecord::Migration[5.2] + def change + add_column :groups, :frozen_by_uuid, :string + end +end diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql index da9959593c..573a4375cd 100644 --- a/services/api/db/structure.sql +++ b/services/api/db/structure.sql @@ -567,7 +567,8 @@ CREATE TABLE public.groups ( trash_at timestamp without time zone, is_trashed boolean DEFAULT false NOT NULL, delete_at timestamp without time zone, - properties jsonb DEFAULT '{}'::jsonb + properties jsonb DEFAULT '{}'::jsonb, + frozen_by_uuid character varying ); @@ -3147,6 +3148,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20210126183521'), ('20210621204455'), ('20210816191509'), -('20211027154300'); +('20211027154300'), +('20220224203102'); diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb index 10932e116d..f3367118c5 100644 --- a/services/api/test/unit/group_test.rb +++ b/services/api/test/unit/group_test.rb @@ -313,4 +313,124 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}' assert Link.where(tail_uuid: g3, head_uuid: g4, link_class: "permission", name: "can_manage").any? assert !Link.where(link_class: 'permission', name: 'can_manage', tail_uuid: g5, head_uuid: g4).any? end + + test "freeze project" do + act_as_user users(:active) do + 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: '') + + # Cannot set frozen_by_uuid to a different user + assert_raises do + proj.update_attributes!(frozen_by_uuid: users(:spectator).uuid) + end + proj.reload + + # Cannot set frozen_by_uuid without description (if so configured) + Rails.configuration.API.FreezeProjectRequiresDescription = true + err = assert_raises do + proj.update_attributes!(frozen_by_uuid: users(:active).uuid) + end + assert_match /can only be set if description is non-empty/, err.inspect + proj.reload + err = assert_raises do + proj.update_attributes!(frozen_by_uuid: users(:active).uuid, description: '') + end + assert_match /can only be set if description is non-empty/, err.inspect + proj.reload + + # Cannot set frozen_by_uuid without properties (if so configured) + Rails.configuration.API.FreezeProjectRequiresProperties['frobity'] = true + err = assert_raises do + proj.update_attributes!( + frozen_by_uuid: users(:active).uuid, + description: 'ready to freeze') + end + assert_match /can only be set if properties\[frobity\] value is non-empty/, err.inspect + proj.reload + + # Can set frozen_by_uuid if all conditions are met + ok = proj.update_attributes( + frozen_by_uuid: users(:active).uuid, + description: 'ready to freeze', + 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| + assert_raises do + Collection.create!(owner_uuid: parent.uuid, name: 'inside-frozen-project') + end + assert_raises do + Group.create!(owner_uuid: parent.uuid, group_class: 'project', name: 'inside-frozen-project') + end + cr = ContainerRequest.create( + 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: parent.uuid, + ) + assert_raises do + cr.save! + end + assert_match cr.errors.inspect, /frozen/ + end + + # User with manage permission can unfreeze, then create items + # inside it and its children + assert proj.update_attributes(frozen_by_uuid: nil) + assert Collection.create!(owner_uuid: proj.uuid, name: 'inside-unfrozen-project') + assert Collection.create!(owner_uuid: proj_inner.uuid, name: 'inside-inner-unfrozen-project') + + # Re-freeze, and reconfigure so only admins can unfreeze. + assert proj.update_attributes(frozen_by_uuid: users(:active).uuid) + Rails.configuration.API.UnfreezeProjectRequiresAdmin = true + + # Owner cannot unfreeze, because not admin. + err = assert_raises do + proj.update_attributes!(frozen_by_uuid: nil) + end + assert_match /can only be changed by an admin user, once set/, err.inspect + proj.reload + + act_as_user users(:admin) do + # Even admin cannot change frozen_by_uuid to someone else's UUID. + err = assert_raises do + proj.update_attributes!(frozen_by_uuid: users(:project_viewer).uuid) + end + assert_match /can only be set to the current user's UUID/, err.inspect + proj.reload + + # Admin can unfreeze. + assert proj.update_attributes(frozen_by_uuid: nil), proj.errors.messages + end + end + end end -- 2.30.2