From 86a78a669f01d15ec8fa5da8a3dfa29f5ef28128 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Wed, 24 Aug 2022 16:20:30 -0400 Subject: [PATCH] 19269: Upgrade user->all_users group membership links. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- services/api/app/models/user.rb | 26 +++++++++---------- .../20220726034131_write_via_all_users.rb | 24 +++++++++++++++++ services/api/db/structure.sql | 5 ++-- services/api/test/fixtures/links.yml | 12 ++++----- .../arvados/v1/users_controller_test.rb | 14 +++++----- .../api/test/helpers/users_test_helper.rb | 15 +++++------ services/api/test/integration/users_test.rb | 12 ++++----- services/api/test/unit/user_test.rb | 12 ++++----- 8 files changed, 70 insertions(+), 50 deletions(-) create mode 100644 services/api/db/migrate/20220726034131_write_via_all_users.rb diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index 1662278cc3..8c8039f1b8 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_write').first || Link.create(tail_uuid: self.uuid, head_uuid: all_users_group_uuid, link_class: 'permission', - name: 'can_read')] + name: 'can_write')] 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_via_all_users.rb b/services/api/db/migrate/20220726034131_write_via_all_users.rb new file mode 100644 index 0000000000..30e6463beb --- /dev/null +++ b/services/api/db/migrate/20220726034131_write_via_all_users.rb @@ -0,0 +1,24 @@ +# Copyright (C) The Arvados Authors. All rights reserved. +# +# SPDX-License-Identifier: AGPL-3.0 + +class WriteViaAllUsers < ActiveRecord::Migration[5.2] + include CurrentApiClient + def up + changelinks(from: "can_read", to: "can_write") + end + def down + changelinks(from: "can_write", 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 c5f6d567bf..525300833e 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 b7f1aaa1fa..99b97510db 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_write 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_write 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_write 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_write 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_write 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_write 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 6a7b00a005..b7d683df29 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_write', '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_write', '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_write', '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_write', '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_write', '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_write', '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}, 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 6ca9977a5e..e106d994cd 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,17 +77,14 @@ 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], + group_write_perms = Link.where(tail_uuid: uuid, + head_uuid: all_users_group_uuid, link_class: 'permission', - name: 'can_read') + name: 'can_write') if expect_group_perms - assert group_read_perms.any?, "expected all users group read perms" + assert group_write_perms.any?, "expected all users group write perms" else - assert !group_read_perms.any?, "expected all users group perm deleted" + assert !group_write_perms.any?, "expected all users group write perms deleted" end signed_uuids = Link.where(link_class: 'signature', diff --git a/services/api/test/integration/users_test.rb b/services/api/test/integration/users_test.rb index 430f0d385d..f7fddb44d3 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_write', '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_write', '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_write', '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@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_write', '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@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_write', '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_write', '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 9a0e1dbf9c..12384cba92 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_write', 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_write', 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_write', 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_write', 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_write', 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_write") # Check for repository. if named_repo = (prior_repo or -- 2.30.2