[arvados] created: 2.1.0-2786-g68a98cf55
git repository hosting
git at public.arvados.org
Mon Aug 1 15:26:55 UTC 2022
at 68a98cf559d6743bf4d0e829932951cf26c87fff (commit)
commit 68a98cf559d6743bf4d0e829932951cf26c87fff
Author: Tom Clegg <tom at curii.com>
Date: Mon Aug 1 11:26:46 2022 -0400
19269: Upgrade user->all_users group membership links.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 1662278cc..37ed195e3 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -311,28 +311,27 @@ SELECT target_uuid, perm_level
# note: these permission links are obsolete, they have no effect
# on anything and they are not created for new users.
Link.where(tail_uuid: self.email,
- link_class: 'permission',
- name: 'can_login').destroy_all
+ link_class: 'permission',
+ name: 'can_login').destroy_all
# delete repo_perms for this user
Link.where(tail_uuid: self.uuid,
- link_class: 'permission',
- name: 'can_manage').destroy_all
+ link_class: 'permission',
+ name: 'can_manage').destroy_all
# delete vm_login_perms for this user
Link.where(tail_uuid: self.uuid,
- link_class: 'permission',
- name: 'can_login').destroy_all
+ link_class: 'permission',
+ name: 'can_login').destroy_all
# delete "All users" group read permissions for this user
Link.where(tail_uuid: self.uuid,
- head_uuid: all_users_group_uuid,
- link_class: 'permission',
- name: 'can_read').destroy_all
+ head_uuid: all_users_group_uuid,
+ link_class: 'permission').destroy_all
# delete any signatures by this user
Link.where(link_class: 'signature',
- tail_uuid: self.uuid).destroy_all
+ tail_uuid: self.uuid).destroy_all
# delete tokens for this user
ApiClientAuthorization.where(user_id: self.id).destroy_all
@@ -381,8 +380,7 @@ SELECT target_uuid, perm_level
#
if Link.where(tail_uuid: self.uuid,
head_uuid: all_users_group_uuid,
- link_class: 'permission',
- name: 'can_read').any?
+ link_class: 'permission').any?
errors.add :is_active, "cannot be set to false directly, use the 'Deactivate' button on Workbench, or the 'unsetup' API call"
end
end
@@ -785,11 +783,11 @@ SELECT target_uuid, perm_level
resp = [Link.where(tail_uuid: self.uuid,
head_uuid: all_users_group_uuid,
link_class: 'permission',
- name: 'can_read').first ||
+ name: 'can_manage').first ||
Link.create(tail_uuid: self.uuid,
head_uuid: all_users_group_uuid,
link_class: 'permission',
- name: 'can_read')]
+ name: 'can_manage')]
if Rails.configuration.Users.ActivatedUsersAreVisibleToOthers
resp += [Link.where(tail_uuid: all_users_group_uuid,
head_uuid: self.uuid,
diff --git a/services/api/db/migrate/20220726034131_write_manage_via_all_users.rb b/services/api/db/migrate/20220726034131_write_manage_via_all_users.rb
new file mode 100644
index 000000000..1fae22672
--- /dev/null
+++ b/services/api/db/migrate/20220726034131_write_manage_via_all_users.rb
@@ -0,0 +1,24 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class WriteManageViaAllUsers < ActiveRecord::Migration[5.2]
+ include CurrentApiClient
+ def up
+ changelinks(from: "can_read", to: "can_manage")
+ end
+ def down
+ changelinks(from: "can_manage", to: "can_read")
+ end
+ def changelinks(from:, to:)
+ ActiveRecord::Base.connection.exec_query(
+ "update links set name=$1 where link_class=$2 and name=$3 and tail_uuid like $4 and head_uuid = $5",
+ "migrate", [
+ [nil, to],
+ [nil, "permission"],
+ [nil, from],
+ [nil, "_____-tpzed-_______________"],
+ [nil, all_users_group_uuid],
+ ])
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index c5f6d567b..525300833 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -254,8 +254,6 @@ $$;
SET default_tablespace = '';
-SET default_with_oids = false;
-
--
-- Name: api_client_authorizations; Type: TABLE; Schema: public; Owner: -
--
@@ -3182,6 +3180,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20220301155729'),
('20220303204419'),
('20220401153101'),
-('20220505112900');
+('20220505112900'),
+('20220726034131');
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index b7f1aaa1f..24f02aa9d 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -54,7 +54,7 @@ active_user_member_of_all_users_group:
updated_at: 2014-01-24 20:42:26 -0800
tail_uuid: zzzzz-tpzed-xurymjxw79nv3jz
link_class: permission
- name: can_read
+ name: can_manage
head_uuid: zzzzz-j7d0g-fffffffffffffff
properties: {}
@@ -110,7 +110,7 @@ spectator_user_member_of_all_users_group:
updated_at: 2014-01-24 20:42:26 -0800
tail_uuid: zzzzz-tpzed-l1s2piq4t4mps8r
link_class: permission
- name: can_read
+ name: can_manage
head_uuid: zzzzz-j7d0g-fffffffffffffff
properties: {}
@@ -124,7 +124,7 @@ inactive_user_member_of_all_users_group:
updated_at: 2013-12-26T20:52:21Z
tail_uuid: zzzzz-tpzed-x9kqpd79egh49c7
link_class: permission
- name: can_read
+ name: can_manage
head_uuid: zzzzz-j7d0g-fffffffffffffff
properties: {}
@@ -138,7 +138,7 @@ inactive_signed_ua_user_member_of_all_users_group:
updated_at: 2013-12-26T20:52:21Z
tail_uuid: zzzzz-tpzed-7sg468ezxwnodxs
link_class: permission
- name: can_read
+ name: can_manage
head_uuid: zzzzz-j7d0g-fffffffffffffff
properties: {}
@@ -433,7 +433,7 @@ project_viewer_member_of_all_users_group:
updated_at: 2015-07-28T21:34:41.361747000Z
tail_uuid: zzzzz-tpzed-projectviewer1a
link_class: permission
- name: can_read
+ name: can_manage
head_uuid: zzzzz-j7d0g-fffffffffffffff
properties: {}
@@ -1044,7 +1044,7 @@ user1-with-load_member_of_all_users_group:
updated_at: 2014-01-24 20:42:26 -0800
tail_uuid: zzzzz-tpzed-user1withloadab
link_class: permission
- name: can_read
+ name: can_manage
head_uuid: zzzzz-j7d0g-fffffffffffffff
properties: {}
diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 6a7b00a00..d7117fc8e 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -151,7 +151,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
"foo/#{repo_name}", created['uuid'], 'arvados#repository', true, 'Repository'
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#virtualMachine', false, 'permission', 'can_login',
@@ -335,7 +335,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
# two extra links; system_group, and group
verify_links_added 2
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', response_object['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#repository', false, 'permission', 'can_manage',
@@ -420,7 +420,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
'foo/usertestrepo', created['uuid'], 'arvados#repository', true, 'Repository'
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#virtualMachine', false, 'permission', 'can_login',
@@ -458,7 +458,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
'foo/usertestrepo', created['uuid'], 'arvados#repository', true, 'Repository'
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#virtualMachine', true, 'permission', 'can_login',
@@ -511,7 +511,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
assert_equal active_user[:email], created['email'], 'expected input email'
# verify links
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
@@ -545,7 +545,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
assert_equal active_user['email'], created['email'], 'expected original email'
# verify links
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
assert_equal(repos_count, repos_query.count)
@@ -666,7 +666,7 @@ The Arvados team.
assert_equal active_user['uuid'], json_response['uuid']
updated = User.where(uuid: active_user['uuid']).first
assert_equal(true, updated.is_active)
- assert_equal({read: true}, updated.group_permissions[all_users_group_uuid])
+ assert_equal({read: true, write: true, manage: true}, updated.group_permissions[all_users_group_uuid])
end
test "non-admin user can get basic information about readable users" do
diff --git a/services/api/test/helpers/users_test_helper.rb b/services/api/test/helpers/users_test_helper.rb
index 6ca9977a5..d87bec9fd 100644
--- a/services/api/test/helpers/users_test_helper.rb
+++ b/services/api/test/helpers/users_test_helper.rb
@@ -3,6 +3,8 @@
# SPDX-License-Identifier: AGPL-3.0
module UsersTestHelper
+ include CurrentApiClient
+
def verify_link(response_items, link_object_name, expect_link, link_class,
link_name, head_uuid, tail_uuid, head_kind, fetch_object, class_name)
link = find_obj_in_resp response_items, 'arvados#link', link_object_name
@@ -75,13 +77,10 @@ module UsersTestHelper
assert !vm_login_perms.any?, "expected all vm_login_perms deleted"
end
- group = Group.where(name: 'All users').select do |g|
- g[:uuid].match(/-f+$/)
- end.first
group_read_perms = Link.where(tail_uuid: uuid,
- head_uuid: group[:uuid],
+ head_uuid: all_users_group_uuid,
link_class: 'permission',
- name: 'can_read')
+ name: 'can_manage')
if expect_group_perms
assert group_read_perms.any?, "expected all users group read perms"
else
diff --git a/services/api/test/integration/users_test.rb b/services/api/test/integration/users_test.rb
index 430f0d385..7d9638113 100644
--- a/services/api/test/integration/users_test.rb
+++ b/services/api/test/integration/users_test.rb
@@ -40,7 +40,7 @@ class UsersTest < ActionDispatch::IntegrationTest
verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
'foo/usertestrepo', created['uuid'], 'arvados#repository', true, 'Repository'
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#virtualMachine', false, 'permission', 'can_login',
@@ -85,7 +85,7 @@ class UsersTest < ActionDispatch::IntegrationTest
verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
'foo/usertestrepo', created['uuid'], 'arvados#repository', true, 'Repository'
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#virtualMachine', true, 'permission', 'can_login',
@@ -113,7 +113,7 @@ class UsersTest < ActionDispatch::IntegrationTest
# two new links: system_group, and 'All users' group.
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#virtualMachine', false, 'permission', 'can_login',
@@ -135,7 +135,7 @@ class UsersTest < ActionDispatch::IntegrationTest
assert_equal 'foo at example.com', created['email'], 'expected input email'
# verify links
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
@@ -163,7 +163,7 @@ class UsersTest < ActionDispatch::IntegrationTest
assert_equal created['email'], 'foo at example.com', 'expected original email'
# verify links
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#virtualMachine', true, 'permission', 'can_login',
@@ -187,7 +187,7 @@ class UsersTest < ActionDispatch::IntegrationTest
# four extra links: system_group, login, group, repo and vm
- verify_link response_items, 'arvados#group', true, 'permission', 'can_read',
+ verify_link response_items, 'arvados#group', true, 'permission', 'can_manage',
'All users', created['uuid'], 'arvados#group', true, 'Group'
verify_link response_items, 'arvados#repository', true, 'permission', 'can_manage',
diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb
index 9a0e1dbf9..42b444497 100644
--- a/services/api/test/unit/user_test.rb
+++ b/services/api/test/unit/user_test.rb
@@ -465,7 +465,7 @@ class UserTest < ActiveSupport::TestCase
verify_user resp_user, email
group_perm = find_obj_in_resp response, 'Link', 'arvados#group'
- verify_link group_perm, 'permission', 'can_read', resp_user[:uuid], nil
+ verify_link group_perm, 'permission', 'can_manage', resp_user[:uuid], groups(:all_users).uuid
group_perm2 = find_obj_in_resp response, 'Link', 'arvados#user'
if visible
@@ -499,7 +499,7 @@ class UserTest < ActiveSupport::TestCase
verify_user resp_user, email
group_perm = find_obj_in_resp response, 'Link', 'arvados#group'
- verify_link group_perm, 'permission', 'can_read', resp_user[:uuid], nil
+ verify_link group_perm, 'permission', 'can_manage', resp_user[:uuid], groups(:all_users).uuid
repo_perm = find_obj_in_resp response, 'Link', 'arvados#repository'
verify_link repo_perm, 'permission', 'can_manage', resp_user[:uuid], nil
@@ -522,7 +522,7 @@ class UserTest < ActiveSupport::TestCase
verify_user resp_user, email
group_perm = find_obj_in_resp response, 'Link', 'arvados#group'
- verify_link group_perm, 'permission', 'can_read', resp_user[:uuid], nil
+ verify_link group_perm, 'permission', 'can_manage', resp_user[:uuid], groups(:all_users).uuid
group_perm2 = find_obj_in_resp response, 'Link', 'arvados#user'
verify_link group_perm2, 'permission', 'can_read', groups(:all_users).uuid, nil
@@ -534,7 +534,7 @@ class UserTest < ActiveSupport::TestCase
assert_equal user.uuid, resp_user[:uuid], 'expected uuid not found'
group_perm = find_obj_in_resp response, 'Link', 'arvados#group'
- verify_link group_perm, 'permission', 'can_read', resp_user[:uuid], nil
+ verify_link group_perm, 'permission', 'can_manage', resp_user[:uuid], groups(:all_users).uuid
repo_perm = find_obj_in_resp response, 'Link', 'arvados#repository'
verify_link repo_perm, 'permission', 'can_manage', resp_user[:uuid], nil
@@ -550,7 +550,7 @@ class UserTest < ActiveSupport::TestCase
assert_equal user.uuid, resp_user[:uuid], 'expected uuid not found'
group_perm = find_obj_in_resp response, 'Link', 'arvados#group'
- verify_link group_perm, 'permission', 'can_read', resp_user[:uuid], nil
+ verify_link group_perm, 'permission', 'can_manage', resp_user[:uuid], groups(:all_users).uuid
repo_perm = find_obj_in_resp response, 'Link', 'arvados#repository'
verify_link repo_perm, 'permission', 'can_manage', resp_user[:uuid], nil
@@ -625,7 +625,7 @@ class UserTest < ActiveSupport::TestCase
# check user setup
verify_link_exists(Rails.configuration.Users.AutoSetupNewUsers || active,
groups(:all_users).uuid, user.uuid,
- "permission", "can_read")
+ "permission", "can_manage")
# Check for repository.
if named_repo = (prior_repo or
commit 2e11d91caf17d1a41417f8bee9b24adae4ad4ec3
Author: Tom Clegg <tom at curii.com>
Date: Mon Jul 25 23:36:34 2022 -0400
19269: Prohibit changes to roles by non-admin users.
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 0c36a048d..78b3f08c1 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -250,7 +250,6 @@ class Group < ArvadosModel
if self.owner_uuid != system_user_uuid
raise "Owner uuid for role must be system user"
end
- raise PermissionDeniedError unless current_user.can?(manage: uuid)
true
else
super
@@ -274,6 +273,9 @@ class Group < ArvadosModel
elsif frozen_by_uuid && frozen_by_uuid_was
errors.add :uuid, "#{uuid} is frozen and cannot be modified"
return false
+ elsif group_class_was == 'role' && !current_user.andand.is_admin
+ errors.add :uuid, "#{uuid} is a role group and can only be modified by admin"
+ return false
else
return true
end
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 cfcb33d40..17b5c3b3c 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -795,7 +795,7 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
{group_class: "filter", properties: {"filters":[]}},
].each do |params|
test "destroy group #{params} returns object" do
- authorize_with :active
+ authorize_with :admin
group = Group.create!(params)
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index efc43dfde..c83998f5f 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -149,29 +149,59 @@ class PermissionTest < ActiveSupport::TestCase
":spectator missing from writers list")
end
- test "user owns group, group can_manage object's group, user can add permissions" do
- set_user_from_auth :admin
-
- owner_grp = Group.create!(owner_uuid: users(:active).uuid, group_class: "role")
-
- sp_grp = Group.create!(group_class: "project")
-
- Link.create!(link_class: 'permission',
- name: 'can_manage',
- tail_uuid: owner_grp.uuid,
- head_uuid: sp_grp.uuid)
-
- sp = Collection.create!(owner_uuid: sp_grp.uuid)
+ ['read', 'write', 'manage'].each do |access|
+ test "grant #{access} access through a role" do
+ set_user_from_auth :active_trustedclient
+ role = Group.create!(group_class: "role", name: "test role")
+ project = Group.create!(group_class: "project", name: "test project")
+ Link.create!(link_class: 'permission',
+ name: "can_#{access}",
+ tail_uuid: role.uuid,
+ head_uuid: project.uuid)
+ collection = Collection.create!(owner_uuid: project.uuid, name: "test collection")
+
+ set_user_from_auth :admin
+ [:spectator, :project_viewer].each do |u|
+ Link.create!(link_class: 'permission',
+ name: 'can_manage',
+ tail_uuid: users(u).uuid,
+ head_uuid: role.uuid)
+ Link.create!(link_class: 'permission',
+ name: 'can_read',
+ tail_uuid: role.uuid,
+ head_uuid: users(u).uuid)
+ end
- # active user owns owner_grp, which has can_manage permission on sp_grp
- # user should be able to add permissions on sp.
- set_user_from_auth :active_trustedclient
- test_perm = Link.create(tail_uuid: users(:active).uuid,
- head_uuid: sp.uuid,
- link_class: 'permission',
- name: 'can_write')
- assert test_perm.save, "could not save new permission on target object"
- assert test_perm.destroy, "could not delete new permission on target object"
+ set_user_from_auth :spectator
+ if access == 'read'
+ assert_raises do
+ Collection.where(uuid: collection.uuid).first.update_attributes!(name: 'renamed test collection')
+ end
+ else
+ assert Collection.where(uuid: collection.uuid).first.update_attributes(name: 'renamed test collection')
+ end
+ if access == 'manage'
+ # "spectator" user belongs to role, which has can_manage
+ # permission on project, so should be able to add/remove
+ # permissions on collection inside project.
+ test_perm = Link.create(tail_uuid: users(:spectator).uuid,
+ head_uuid: collection.uuid,
+ link_class: 'permission',
+ name: 'can_write')
+ assert test_perm.save, lambda { test_perm.errors.inspect }
+ assert test_perm.destroy
+ else
+ assert_raises do
+ Link.create!(tail_uuid: users(:spectator).uuid,
+ head_uuid: collection.uuid,
+ link_class: 'permission',
+ name: 'can_write')
+ end
+ end
+ assert_raises(ArvadosModel::PermissionDeniedError) do
+ Group.where(uuid: role.uuid).first.update_attributes(name: 'user tried to rename a role')
+ end
+ end
end
# bug #3091
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list