[ARVADOS] updated: 1.3.0-2617-g618ce6d7a
Git user
git at public.arvados.org
Fri May 29 21:25:27 UTC 2020
Summary of changes:
services/api/app/models/arvados_model.rb | 3 ++
services/api/app/models/database_seeds.rb | 1 -
services/api/app/models/group.rb | 3 ++
services/api/lib/current_api_client.rb | 18 ++------
services/api/test/fixtures/groups.yml | 13 +-----
.../arvados/v1/groups_controller_test.rb | 24 ++++++-----
services/api/test/integration/permissions_test.rb | 3 +-
services/api/test/unit/group_test.rb | 49 +++++++++++++++++++---
8 files changed, 68 insertions(+), 46 deletions(-)
via 618ce6d7a5bd435fc9a885a3e555ccffd0dca785 (commit)
from e5f25fe8e906411b7c30dc5e6aec659c758d894a (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 618ce6d7a5bd435fc9a885a3e555ccffd0dca785
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Fri May 29 17:25:05 2020 -0400
16007: Make it so that only projects can own things (WIP)
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index cc6f7816a..23d30f24b 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -569,6 +569,9 @@ class ArvadosModel < ApplicationRecord
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"
raise PermissionDeniedError
+ elsif rsc_class == Group && Group.find_by_uuid(owner_uuid).group_class != "project"
+ errors.add :owner_uuid, "must be a project"
+ raise PermissionDeniedError
end
end
else
diff --git a/services/api/app/models/database_seeds.rb b/services/api/app/models/database_seeds.rb
index 39f491503..a86a2c854 100644
--- a/services/api/app/models/database_seeds.rb
+++ b/services/api/app/models/database_seeds.rb
@@ -13,7 +13,6 @@ class DatabaseSeeds
anonymous_group
anonymous_group_read_permission
anonymous_user
- empty_collection
refresh_permissions
refresh_trashed
end
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 4dc9a434d..769b0ab63 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -49,6 +49,9 @@ class Group < ArvadosModel
if group_class != 'project' && group_class != 'role'
errors.add :group_class, "value must be one of 'project' or 'role', was '#{group_class}'"
end
+ if group_class_changed? && !group_class_was.nil?
+ errors.add :group_class, "cannot be modified after record is created"
+ end
end
def update_trash
diff --git a/services/api/lib/current_api_client.rb b/services/api/lib/current_api_client.rb
index c7b48c0cd..f3ec0a03e 100644
--- a/services/api/lib/current_api_client.rb
+++ b/services/api/lib/current_api_client.rb
@@ -8,7 +8,6 @@ $all_users_group = nil
$anonymous_user = nil
$anonymous_group = nil
$anonymous_group_read_permission = nil
-$empty_collection = nil
module CurrentApiClient
def current_user
@@ -90,7 +89,8 @@ module CurrentApiClient
ActiveRecord::Base.transaction do
Group.where(uuid: system_group_uuid).
first_or_create!(name: "System group",
- description: "System group") do |g|
+ description: "System group",
+ group_class: "role") do |g|
g.save!
User.all.collect(&:uuid).each do |user_uuid|
Link.create!(link_class: 'permission',
@@ -188,22 +188,10 @@ module CurrentApiClient
end
end
- def empty_collection_uuid
+ def empty_collection_pdh
'd41d8cd98f00b204e9800998ecf8427e+0'
end
- def empty_collection
- $empty_collection = check_cache $empty_collection do
- act_as_system_user do
- ActiveRecord::Base.transaction do
- Collection.
- where(portable_data_hash: empty_collection_uuid).
- first_or_create!(manifest_text: '', owner_uuid: anonymous_group.uuid)
- end
- end
- end
- end
-
private
# If the given value is nil, or the cache has been cleared since it
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 68377eb5f..ac4a5251f 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -14,7 +14,7 @@ private:
owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
name: Private
description: Private Group
- group_class: role
+ group_class: project
private_and_can_read_foofile:
uuid: zzzzz-j7d0g-22xp1wpjul508rk
@@ -248,17 +248,6 @@ fuse_owned_project:
description: Test project belonging to FUSE test user
group_class: project
-group_with_no_class:
- uuid: zzzzz-j7d0g-groupwithnoclas
- owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
- created_at: 2014-04-21 15:37:48 -0400
- modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
- modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
- modified_at: 2014-04-21 15:37:48 -0400
- updated_at: 2014-04-21 15:37:48 -0400
- name: group_with_no_class
- description: This group has no class at all. So rude!
-
# This wouldn't pass model validation, but it enables a workbench
# infinite-loop test. See #4389
project_owns_itself:
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 2b5e8d5a9..15223b0c0 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -44,8 +44,6 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
end
assert_not_includes group_uuids, groups(:aproject).uuid
assert_not_includes group_uuids, groups(:asubproject).uuid
- assert_includes group_uuids, groups(:private).uuid
- assert_includes group_uuids, groups(:group_with_no_class).uuid
end
test "get list of groups with bogus group_class" do
@@ -746,20 +744,23 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_equal 0, json_response['included'].length
end
- test 'get shared, owned by non-project' do
+ test 'get shared, add permission link' do
authorize_with :user_bar_in_sharing_group
act_as_system_user do
- Group.find_by_uuid(groups(:project_owned_by_foo).uuid).update!(owner_uuid: groups(:group_for_sharing_tests).uuid)
+ Link.create!(tail_uuid: groups(:group_for_sharing_tests).uuid,
+ head_uuid: groups(:project_owned_by_foo).uuid,
+ link_class: 'permission',
+ name: 'can_manage')
end
get :shared, params: {:filters => [["group_class", "=", "project"]], :include => "owner_uuid"}
assert_equal 1, json_response['items'].length
- assert_equal json_response['items'][0]["uuid"], groups(:project_owned_by_foo).uuid
+ assert_equal groups(:project_owned_by_foo).uuid, json_response['items'][0]["uuid"]
assert_equal 1, json_response['included'].length
- assert_equal json_response['included'][0]["uuid"], groups(:group_for_sharing_tests).uuid
+ assert_equal users(:user_foo_in_sharing_group).uuid, json_response['included'][0]["uuid"]
end
### contents with exclude_home_project
@@ -810,20 +811,23 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_equal 0, json_response['included'].length
end
- test 'contents, exclude home, owned by non-project' do
+ test 'contents, exclude home, add permission link' do
authorize_with :user_bar_in_sharing_group
act_as_system_user do
- Group.find_by_uuid(groups(:project_owned_by_foo).uuid).update!(owner_uuid: groups(:group_for_sharing_tests).uuid)
+ Link.create!(tail_uuid: groups(:group_for_sharing_tests).uuid,
+ head_uuid: groups(:project_owned_by_foo).uuid,
+ link_class: 'permission',
+ name: 'can_manage')
end
get :contents, params: {:include => "owner_uuid", :exclude_home_project => true}
assert_equal 1, json_response['items'].length
- assert_equal json_response['items'][0]["uuid"], groups(:project_owned_by_foo).uuid
+ assert_equal groups(:project_owned_by_foo).uuid, json_response['items'][0]["uuid"]
assert_equal 1, json_response['included'].length
- assert_equal json_response['included'][0]["uuid"], groups(:group_for_sharing_tests).uuid
+ assert_equal users(:user_foo_in_sharing_group).uuid, json_response['included'][0]["uuid"]
end
test 'contents, exclude home, with parent specified' do
diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index eec41aa08..ff33fe65b 100644
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -6,7 +6,6 @@ require 'test_helper'
class PermissionsTest < ActionDispatch::IntegrationTest
include DbCurrentTime
- include CurrentApiClient # for empty_collection
fixtures :users, :groups, :api_client_authorizations, :collections
test "adding and removing direct can_read links" do
@@ -441,7 +440,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
test "active user can read the empty collection" do
# The active user should be able to read the empty collection.
- get("/arvados/v1/collections/#{empty_collection_uuid}",
+ get("/arvados/v1/collections/#{empty_collection_pdh}",
params: {:format => :json},
headers: auth(:active))
assert_response :success
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index f7a531fc0..e34c1a44f 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -60,6 +60,43 @@ class GroupTest < ActiveSupport::TestCase
assert g_foo.errors.messages[:owner_uuid].join(" ").match(/ownership cycle/)
end
+ test "cannot create a group that is not a 'role' or 'project'" do
+ set_user_from_auth :active_trustedclient
+
+ assert_raises(ActiveRecord::RecordInvalid) do
+ Group.create!(name: "foo")
+ end
+
+ assert_raises(ActiveRecord::RecordInvalid) do
+ Group.create!(name: "foo", group_class: "")
+ end
+
+ assert_raises(ActiveRecord::RecordInvalid) do
+ Group.create!(name: "foo", group_class: "bogus")
+ end
+ end
+
+ test "cannot change group_class on an already created group" do
+ set_user_from_auth :active_trustedclient
+ g = Group.create!(name: "foo", group_class: "role")
+ assert_raises(ActiveRecord::RecordInvalid) do
+ g.update_attributes!(group_class: "project")
+ end
+ end
+
+ test "role cannot own things" do
+ set_user_from_auth :active_trustedclient
+ role = Group.create!(name: "foo", group_class: "role")
+ assert_raises(ArvadosModel::PermissionDeniedError) do
+ Collection.create!(name: "bzzz123", owner_uuid: role.uuid)
+ end
+
+ c = Collection.create!(name: "bzzz124")
+ assert_raises(ArvadosModel::PermissionDeniedError) do
+ c.update_attributes!(owner_uuid: role.uuid)
+ end
+ end
+
test "trash group hides contents" do
set_user_from_auth :active_trustedclient
@@ -237,7 +274,8 @@ class GroupTest < ActiveSupport::TestCase
set_user_from_auth :active
["", "{SOLIDUS}"].each do |subst|
Rails.configuration.Collections.ForwardSlashNameSubstitution = subst
- g = Group.create group_class: "project"
+ proj = Group.create group_class: "project"
+ role = Group.create group_class: "role"
[[nil, true],
["", true],
[".", false],
@@ -248,11 +286,10 @@ class GroupTest < ActiveSupport::TestCase
["../..", subst != ""],
["/", subst != ""],
].each do |name, valid|
- g.name = name
- g.group_class = "role"
- assert_equal true, g.valid?
- g.group_class = "project"
- assert_equal valid, g.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
+ role.name = name
+ assert_equal true, role.valid?
+ proj.name = name
+ assert_equal valid, proj.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
end
end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list