[ARVADOS] updated: 2.1.0-1988-gc3d04aeb8
Git user
git at public.arvados.org
Tue Mar 15 15:04:25 UTC 2022
Summary of changes:
doc/api/methods/groups.html.textile.liquid | 2 +-
doc/install/install-postgresql.html.textile.liquid | 2 +-
services/api/app/models/arvados_model.rb | 51 ++++++++++++----------
services/api/app/models/user.rb | 16 +++++++
services/api/test/unit/group_test.rb | 5 ++-
services/api/test/unit/permission_test.rb | 13 ++++++
6 files changed, 61 insertions(+), 28 deletions(-)
via c3d04aeb81a04b5dc527af8f9297e9fefb5f4851 (commit)
via c83df787be356ebee4dfe1adc417484c4bc900c7 (commit)
via 7bc292b326db211a167fad7d74dc2fe548987f7d (commit)
via 174c072a594e5979ed2e32fd19a749893a7e88a7 (commit)
via ce641fa0ec1bc0729f8e5864ba8eec6fe99832c0 (commit)
via 08d24348e4dacec6ca0d7c1bb4927281e3991303 (commit)
from 67a86e26e3cc33af9ffe65d486137077b99a6944 (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 c3d04aeb81a04b5dc527af8f9297e9fefb5f4851
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 15 10:30:56 2022 -0400
18691: Expand on "frozen" behavior, mention "...even by admins."
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 cf4c346fa..4d57079b2 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.||
+|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.||
h3. Frozen projects
commit c83df787be356ebee4dfe1adc417484c4bc900c7
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 15 10:21:40 2022 -0400
18691: Rephrase expression.
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 6deab8952..c71996a37 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -249,9 +249,9 @@ class ArvadosModel < ApplicationRecord
# Return [] if this is a frozen project and the current user can't
# unfreeze
return [] if respond_to?(:frozen_by_uuid) && frozen_by_uuid &&
- !(Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
- current_user.andand.is_admin :
- current_user.can?(manage: uuid))
+ (Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
+ !current_user.andand.is_admin :
+ !current_user.can?(manage: uuid))
# Return [] if nobody can write because this object is inside a
# frozen project
return [] if FrozenGroup.where(uuid: owner_uuid).any?
commit 7bc292b326db211a167fad7d74dc2fe548987f7d
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 15 02:58:36 2022 -0400
18691: Comment excluded_trash sql, add more parentheses.
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 3ddbafcdb..6deab8952 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -302,6 +302,15 @@ class ArvadosModel < ApplicationRecord
# For details on how the trashed_groups table is constructed, see
# see db/migrate/20200501150153_permission_table.rb
+ # excluded_trash is a SQL expression that determines whether a row
+ # should be excluded from the results due to being trashed.
+ # Trashed items inside frozen projects are invisible to regular
+ # (non-admin) users even when using include_trash, so we have:
+ #
+ # (item_trashed || item_inside_trashed_project)
+ # &&
+ # (!caller_requests_include_trash ||
+ # (item_inside_frozen_project && caller_is_not_admin))
if (admin && include_trash) || sql_table == "api_client_authorizations"
excluded_trash = "false"
else
@@ -322,7 +331,7 @@ class ArvadosModel < ApplicationRecord
# on trashed items.
if !include_trash && sql_table != "api_client_authorizations"
# Only include records where the owner is not trashed
- sql_conds = "NOT #{excluded_trash}"
+ sql_conds = "NOT (#{excluded_trash})"
end
else
# The core of the permission check is a join against the
@@ -422,7 +431,7 @@ class ArvadosModel < ApplicationRecord
" WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 3))) "
end
- sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) AND NOT #{excluded_trash}"
+ sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) AND NOT (#{excluded_trash})"
end
commit 174c072a594e5979ed2e32fd19a749893a7e88a7
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 15 02:56:09 2022 -0400
18691: Refactor frozen_groups check.
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 ffae867c1..3ddbafcdb 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 febb8ea51..096f5a86a 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 2c084cc88..11c7da090 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.
commit ce641fa0ec1bc0729f8e5864ba8eec6fe99832c0
Author: Tom Clegg <tom at curii.com>
Date: Mon Mar 14 15:01:16 2022 -0400
18691: Recommend postgresql 10+ (9 is unsupported anyway).
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/doc/install/install-postgresql.html.textile.liquid b/doc/install/install-postgresql.html.textile.liquid
index 1413890cd..a9614b9be 100644
--- a/doc/install/install-postgresql.html.textile.liquid
+++ b/doc/install/install-postgresql.html.textile.liquid
@@ -9,7 +9,7 @@ Copyright (C) The Arvados Authors. All rights reserved.
SPDX-License-Identifier: CC-BY-SA-3.0
{% endcomment %}
-Arvados requires at least version *9.4* of PostgreSQL.
+Arvados requires at least version *9.4* of PostgreSQL. We recommend using version 10 or newer.
* "AWS":#aws
* "CentOS 7":#centos7
commit 08d24348e4dacec6ca0d7c1bb4927281e3991303
Author: Tom Clegg <tom at curii.com>
Date: Mon Mar 14 14:54:57 2022 -0400
18691: Show readable_by query plan in test suite.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 647139d9e..efc43dfde 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -602,4 +602,17 @@ class PermissionTest < ActiveSupport::TestCase
end
end
end
+
+ # Show query plan for readable_by query. The plan for a test db
+ # might not resemble the plan for a production db, but it doesn't
+ # hurt to show the test db plan in test logs, and the .
+ [false, true].each do |include_trash|
+ test "query plan, include_trash=#{include_trash}" do
+ sql = Collection.readable_by(users(:active), include_trash: include_trash).to_sql
+ sql = "explain analyze #{sql}"
+ STDERR.puts sql
+ q = ActiveRecord::Base.connection.exec_query(sql)
+ q.rows.each do |row| STDERR.puts(row) end
+ end
+ end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list