[ARVADOS] updated: 2.1.0-1982-g67a86e26e
Git user
git at public.arvados.org
Tue Mar 8 20:23:35 UTC 2022
Summary of changes:
services/api/app/models/arvados_model.rb | 32 +++++++-----
services/api/app/models/group.rb | 59 ++++++++++++----------
.../arvados/v1/groups_controller_test.rb | 20 ++++++++
services/api/test/unit/group_test.rb | 32 +++++++++++-
4 files changed, 102 insertions(+), 41 deletions(-)
via 67a86e26e3cc33af9ffe65d486137077b99a6944 (commit)
via 58b0ff2409f362e6ed5ac3666627ef390ae34004 (commit)
via 6849c9e2d8462e1cde96941a2eb980dfccda10db (commit)
via 8dd26bc10a63aa7499e36f4913f9862fe9c53250 (commit)
from 3d5ac704cca086a5ce66e6724f7087ff487abe3c (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 67a86e26e3cc33af9ffe65d486137077b99a6944
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 8 15:23:24 2022 -0500
18691: Prevent freezing trashed project / trashing frozen project.
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 216397a1b..ee8690acb 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 1a37eb058..2c084cc88 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,
commit 58b0ff2409f362e6ed5ac3666627ef390ae34004
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 8 14:16:06 2022 -0500
18691: include_trash does not return trash in frozen projects.
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 a979338e4..ffae867c1 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -297,26 +297,32 @@ class ArvadosModel < ApplicationRecord
user_uuids = users_list.map { |u| u.uuid }
all_user_uuids = []
+ admin = users_list.select { |u| u.is_admin }.any?
+
# For details on how the trashed_groups table is constructed, see
# see db/migrate/20200501150153_permission_table.rb
- exclude_trashed_records = ""
- if !include_trash and (sql_table == "groups" or sql_table == "collections") then
- # Only include records that are not trashed
- exclude_trashed_records = "AND (#{sql_table}.trash_at is NULL or #{sql_table}.trash_at > statement_timestamp())"
- end
+ if (admin && include_trash) || sql_table == "api_client_authorizations"
+ excluded_trash = "false"
+ else
+ excluded_trash = "(#{sql_table}.owner_uuid IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " +
+ "WHERE trash_at <= statement_timestamp()))"
+ if sql_table == "groups" || sql_table == "collections"
+ excluded_trash = "(#{excluded_trash} OR #{sql_table}.trash_at <= statement_timestamp() IS TRUE)"
+ end
- trashed_check = ""
- if !include_trash && sql_table != "api_client_authorizations"
- trashed_check = "#{sql_table}.owner_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " +
- "where trash_at <= statement_timestamp()) #{exclude_trashed_records}"
+ if include_trash
+ # Exclude trash inside frozen projects
+ excluded_trash = "(#{excluded_trash} AND #{sql_table}.owner_uuid IN (SELECT uuid FROM #{FROZEN_GROUPS}))"
+ end
end
- if users_list.select { |u| u.is_admin }.any?
- # Admin skips most permission checks, but still want to filter on trashed items.
+ if admin
+ # Admin skips most permission checks, but still want to filter
+ # on trashed items.
if !include_trash && sql_table != "api_client_authorizations"
# Only include records where the owner is not trashed
- sql_conds = trashed_check
+ sql_conds = "NOT #{excluded_trash}"
end
else
# The core of the permission check is a join against the
@@ -416,7 +422,7 @@ class ArvadosModel < ApplicationRecord
" WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 3))) "
end
- sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) #{trashed_check.empty? ? "" : "AND"} #{trashed_check}"
+ sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) AND NOT #{excluded_trash}"
end
commit 6849c9e2d8462e1cde96941a2eb980dfccda10db
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 8 14:16:04 2022 -0500
18691: Test hiding of trashed items inside a frozen project.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 0819c2306..fcdce0e60 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -920,4 +920,24 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_response 422
end
+
+ test "include_trash does not return trash inside frozen project" do
+ authorize_with :active
+ trashtime = Time.now - 1.second
+ outerproj = Group.create!(group_class: 'project')
+ innerproj = Group.create!(group_class: 'project', owner_uuid: outerproj.uuid)
+ innercoll = Collection.create!(name: 'inner-not-trashed', owner_uuid: innerproj.uuid)
+ innertrash = Collection.create!(name: 'inner-trashed', owner_uuid: innerproj.uuid, trash_at: trashtime)
+ innertrashproj = Group.create!(group_class: 'project', name: 'inner-trashed-proj', owner_uuid: innerproj.uuid, trash_at: trashtime)
+ outertrash = Collection.create!(name: 'outer-trashed', owner_uuid: outerproj.uuid, trash_at: trashtime)
+ innerproj.update_attributes!(frozen_by_uuid: users(:active).uuid)
+ get :contents, params: {id: outerproj.uuid, include_trash: true, recursive: true}
+ assert_response :success
+ uuids = json_response['items'].collect { |item| item['uuid'] }
+ assert_includes uuids, outertrash.uuid
+ assert_includes uuids, innerproj.uuid
+ assert_includes uuids, innercoll.uuid
+ refute_includes uuids, innertrash.uuid
+ refute_includes uuids, innertrashproj.uuid
+ end
end
commit 8dd26bc10a63aa7499e36f4913f9862fe9c53250
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 8 10:51:52 2022 -0500
18691: Test preventing trashing parent of frozen project.
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 bbb0f1957..1a37eb058 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -317,7 +317,8 @@ 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
- proj = Group.create!(group_class: 'project', name: 'freeze-test', owner_uuid: users(:active).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)
@@ -431,6 +432,10 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
frozen.update_attributes!(trash_at: db_current_time)
end
frozen.reload
+ assert_raises(StandardError, "should reject setting delete_at of #{frozen.uuid}") do
+ frozen.update_attributes!(delete_at: db_current_time)
+ end
+ frozen.reload
assert_raises(StandardError, "should reject delete of #{frozen.uuid}") do
frozen.destroy
end
@@ -457,6 +462,16 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
assert_match /can only be changed by an admin user, once set/, err.inspect
proj.reload
+ # Cannot trash or delete a frozen project's ancestor
+ assert_raises(StandardError, "should not be able to set trash_at on parent of frozen project") do
+ parent.update_attributes!(trash_at: db_current_time)
+ end
+ parent.reload
+ assert_raises(StandardError, "should not be able to set delete_at on parent of frozen project") do
+ parent.update_attributes!(delete_at: db_current_time)
+ end
+ parent.reload
+
act_as_user users(:admin) do
# Even admin cannot change frozen_by_uuid to someone else's UUID.
err = assert_raises do
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list