From 17745128f72eeaef62ea2d367ec316502107f272 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Fri, 29 May 2020 17:25:05 -0400 Subject: [PATCH] 16007: Make it so that only projects can own things Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- 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 | 19 ++----- .../functional/arvados/v1/filters_test.rb | 10 ++-- .../arvados/v1/groups_controller_test.rb | 27 +++++----- .../api/test/integration/permissions_test.rb | 3 +- services/api/test/unit/group_test.rb | 49 ++++++++++++++++--- services/api/test/unit/permission_test.rb | 2 +- 10 files changed, 79 insertions(+), 56 deletions(-) diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 8afebfb79e..7270f7cdf2 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -574,6 +574,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 39f491503e..a86a2c8544 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 d6fc818cd7..e16433c815 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 c7b48c0cdd..f3ec0a03e2 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 68377eb5f9..89235d65b0 100644 --- a/services/api/test/fixtures/groups.yml +++ b/services/api/test/fixtures/groups.yml @@ -6,15 +6,15 @@ public: uuid: zzzzz-j7d0g-it30l961gq3t0oi owner_uuid: zzzzz-tpzed-d9tiejq69daie8f name: Public - description: Public Group - group_class: role + description: Public Project + group_class: project private: uuid: zzzzz-j7d0g-rew6elm53kancon owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz name: Private - description: Private Group - group_class: role + description: Private Project + 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/filters_test.rb b/services/api/test/functional/arvados/v1/filters_test.rb index b30afd7453..26270b1c3c 100644 --- a/services/api/test/functional/arvados/v1/filters_test.rb +++ b/services/api/test/functional/arvados/v1/filters_test.rb @@ -6,16 +6,16 @@ require 'test_helper' class Arvados::V1::FiltersTest < ActionController::TestCase test '"not in" filter passes null values' do - @controller = Arvados::V1::GroupsController.new + @controller = Arvados::V1::ContainerRequestsController.new authorize_with :admin get :index, params: { - filters: [ ['group_class', 'not in', ['project']] ], - controller: 'groups', + filters: [ ['container_uuid', 'not in', ['zzzzz-dz642-queuedcontainer', 'zzzzz-dz642-runningcontainr']] ], + controller: 'container_requests', } assert_response :success found = assigns(:objects) - assert_includes(found.collect(&:group_class), nil, - "'group_class not in ['project']' filter should pass null") + assert_includes(found.collect(&:container_uuid), nil, + "'container_uuid not in [zzzzz-dz642-queuedcontainer, zzzzz-dz642-runningcontainr]' filter should pass null") end test 'error message for non-array element in filters array' do 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 2b5e8d5a9d..ff89cd2129 100644 --- a/services/api/test/functional/arvados/v1/groups_controller_test.rb +++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb @@ -29,8 +29,9 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase end assert_includes group_uuids, groups(:aproject).uuid assert_includes group_uuids, groups(:asubproject).uuid + assert_includes group_uuids, groups(:private).uuid assert_not_includes group_uuids, groups(:system_group).uuid - assert_not_includes group_uuids, groups(:private).uuid + assert_not_includes group_uuids, groups(:private_and_can_read_foofile).uuid end test "get list of groups that are not projects" do @@ -44,8 +45,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 +745,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 +812,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 eec41aa085..ff33fe65b1 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 f7a531fc05..e34c1a44f6 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 diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index 8e2569ccc4..4849b385ba 100644 --- a/services/api/test/unit/permission_test.rb +++ b/services/api/test/unit/permission_test.rb @@ -423,7 +423,7 @@ class PermissionTest < ActiveSupport::TestCase test "add user to group, then remove them" do set_user_from_auth :admin - grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role") + grp = Group.create!(owner_uuid: system_user_uuid, group_class: "project") col = Collection.create!(owner_uuid: grp.uuid) assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid) assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid) -- 2.39.5