From 5e4b8ac7997c68ffa45471b9879789c96068885d Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Fri, 29 Oct 2021 10:08:48 -0400 Subject: [PATCH] 16817: Add Users.ActivatedUsersAreVisibleToOthers config. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- doc/admin/upgrading.html.textile.liquid | 4 ++ lib/config/config.default.yml | 10 ++++ lib/config/export.go | 1 + lib/config/generated_config.go | 10 ++++ sdk/go/arvados/config.go | 1 + services/api/app/models/user.rb | 27 +++++++---- .../arvados/v1/users_controller_test.rb | 1 + services/api/test/unit/permission_test.rb | 2 + services/api/test/unit/user_test.rb | 47 ++++++++++++------- 9 files changed, 78 insertions(+), 25 deletions(-) diff --git a/doc/admin/upgrading.html.textile.liquid b/doc/admin/upgrading.html.textile.liquid index 0aea90bd0d..a059021253 100644 --- a/doc/admin/upgrading.html.textile.liquid +++ b/doc/admin/upgrading.html.textile.liquid @@ -39,6 +39,10 @@ h2(#main). development main (as of 2021-10-27) "previous: Upgrading from 2.3.0":#v2_3_0 +h3. Users are visible to other users by default + +When a new user is set up (either via @AutoSetupNewUsers@ config or via Workbench admin interface) the user immediately becomes visible to other users. To revert to the previous behavior, where the administrator must add two users to the same group using the Workbench admin interface in order for the users to see each other, change the new @Users.ActivatedUsersAreVisibleToOthers@ config to @false@. + h3. Dedicated keepstore process for each container When Arvados runs a container via @arvados-dispatch-cloud@, the @crunch-run@ supervisor process now brings up its own keepstore server to handle I/O for mounted collections, outputs, and logs. With the default configuration, the keepstore process allocates one 64 MiB block buffer per VCPU requested by the container. For most workloads this will increase throughput, reduce total network traffic, and make it possible to run more containers at once without provisioning additional keepstore nodes to handle the I/O load. diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index 97ded6bf68..12415a83a0 100644 --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@ -259,6 +259,16 @@ Clusters: # user agreements. Should only be enabled for development. NewUsersAreActive: false + # Newly activated users (whether set up by an admin or via + # AutoSetupNewUsers) immediately become visible to other active + # users. + # + # On a multi-tenant cluster, where the intent is for users to be + # invisible to one another unless they have been added to the + # same group(s) via Workbench admin interface, change this to + # false. + ActivatedUsersAreVisibleToOthers: true + # The e-mail address of the user you would like to become marked as an admin # user on their first login. AutoAdminUserWithEmail: "" diff --git a/lib/config/export.go b/lib/config/export.go index e36d6e76ca..73ef2d0cfc 100644 --- a/lib/config/export.go +++ b/lib/config/export.go @@ -215,6 +215,7 @@ var whitelist = map[string]bool{ "SystemRootToken": false, "TLS": false, "Users": true, + "Users.ActivatedUsersAreVisibleToOthers": false, "Users.AdminNotifierEmailFrom": false, "Users.AnonymousUserToken": true, "Users.AutoAdminFirstUser": false, diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go index f7849d6142..3d7d90f40c 100644 --- a/lib/config/generated_config.go +++ b/lib/config/generated_config.go @@ -265,6 +265,16 @@ Clusters: # user agreements. Should only be enabled for development. NewUsersAreActive: false + # Newly activated users (whether set up by an admin or via + # AutoSetupNewUsers) immediately become visible to other active + # users. + # + # On a multi-tenant cluster, where the intent is for users to be + # invisible to one another unless they have been added to the + # same group(s) via Workbench admin interface, change this to + # false. + ActivatedUsersAreVisibleToOthers: true + # The e-mail address of the user you would like to become marked as an admin # user on their first login. AutoAdminUserWithEmail: "" diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index e736f79fd7..e7e60aed78 100644 --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@ -220,6 +220,7 @@ type Cluster struct { Insecure bool } Users struct { + ActivatedUsersAreVisibleToOthers bool AnonymousUserToken string AdminNotifierEmailFrom string AutoAdminFirstUser bool diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index 366c03e309..febb8ea516 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -234,8 +234,9 @@ SELECT target_uuid, perm_level name: 'can_read').empty? # Add can_read link from this user to "all users" which makes this - # user "invited" - group_perm = create_user_group_link + # user "invited", and (depending on config) a link in the opposite + # direction which makes this user visible to other users. + group_perms = add_to_all_users_group # Add git repo repo_perm = if (!repo_name.nil? || Rails.configuration.Users.AutoSetupNewUsersWithRepository) and !username.nil? @@ -267,7 +268,7 @@ SELECT target_uuid, perm_level forget_cached_group_perms - return [repo_perm, vm_login_perm, group_perm, self].compact + return [repo_perm, vm_login_perm, *group_perms, self].compact end # delete user signatures, login, repo, and vm perms, and mark as inactive @@ -728,16 +729,26 @@ SELECT target_uuid, perm_level login_perm end - # add the user to the 'All users' group - def create_user_group_link - return (Link.where(tail_uuid: self.uuid, + def add_to_all_users_group + resp = [Link.where(tail_uuid: self.uuid, head_uuid: all_users_group_uuid, link_class: 'permission', - name: 'can_read').first or + name: 'can_read').first || Link.create(tail_uuid: self.uuid, head_uuid: all_users_group_uuid, link_class: 'permission', - name: 'can_read')) + name: 'can_read')] + if Rails.configuration.Users.ActivatedUsersAreVisibleToOthers + resp += [Link.where(tail_uuid: all_users_group_uuid, + head_uuid: self.uuid, + link_class: 'permission', + name: 'can_read').first || + Link.create(tail_uuid: all_users_group_uuid, + head_uuid: self.uuid, + link_class: 'permission', + name: 'can_read')] + end + return resp end # Give the special "System group" permission to manage this user and 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 c807a7d6cb..ae7b21dec8 100644 --- a/services/api/test/functional/arvados/v1/users_controller_test.rb +++ b/services/api/test/functional/arvados/v1/users_controller_test.rb @@ -13,6 +13,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase @initial_link_count = Link.count @vm_uuid = virtual_machines(:testvm).uuid ActionMailer::Base.deliveries = [] + Rails.configuration.Users.ActivatedUsersAreVisibleToOthers = false end test "activate a user after signing UA" do diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index 123031b35f..128d0ebaa6 100644 --- a/services/api/test/unit/permission_test.rb +++ b/services/api/test/unit/permission_test.rb @@ -218,6 +218,7 @@ class PermissionTest < ActiveSupport::TestCase end test "manager user gets permission to minions' articles via can_manage link" do + Rails.configuration.Users.ActivatedUsersAreVisibleToOthers = false manager = create :active_user, first_name: "Manage", last_name: "Er" minion = create :active_user, first_name: "Min", last_name: "Ion" minions_specimen = act_as_user minion do @@ -314,6 +315,7 @@ class PermissionTest < ActiveSupport::TestCase end test "users with bidirectional read permission in group can see each other, but cannot see each other's private articles" do + Rails.configuration.Users.ActivatedUsersAreVisibleToOthers = false a = create :active_user, first_name: "A" b = create :active_user, first_name: "B" other = create :active_user, first_name: "OTHER" diff --git a/services/api/test/unit/user_test.rb b/services/api/test/unit/user_test.rb index c00164c0a3..7368d89374 100644 --- a/services/api/test/unit/user_test.rb +++ b/services/api/test/unit/user_test.rb @@ -447,30 +447,40 @@ class UserTest < ActiveSupport::TestCase assert_not_allowed { User.new.save } end - test "setup new user" do - set_user_from_auth :admin + [true, false].each do |visible| + test "setup new user with ActivatedUsersAreVisibleToOthers=#{visible}" do + Rails.configuration.Users.ActivatedUsersAreVisibleToOthers = visible + set_user_from_auth :admin - email = 'foo@example.com' + email = 'foo@example.com' - user = User.create ({uuid: 'zzzzz-tpzed-abcdefghijklmno', email: email}) + user = User.create ({uuid: 'zzzzz-tpzed-abcdefghijklmno', email: email}) - vm = VirtualMachine.create + vm = VirtualMachine.create - response = user.setup(repo_name: 'foo/testrepo', - vm_uuid: vm.uuid) + response = user.setup(repo_name: 'foo/testrepo', + vm_uuid: vm.uuid) - resp_user = find_obj_in_resp response, 'User' - verify_user resp_user, email + resp_user = find_obj_in_resp response, 'User' + 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 + group_perm = find_obj_in_resp response, 'Link', 'arvados#group' + verify_link group_perm, 'permission', 'can_read', resp_user[:uuid], nil - repo_perm = find_obj_in_resp response, 'Link', 'arvados#repository' - verify_link repo_perm, 'permission', 'can_manage', resp_user[:uuid], nil + group_perm2 = find_obj_in_resp response, 'Link', 'arvados#user' + if visible + verify_link group_perm2, 'permission', 'can_read', groups(:all_users).uuid, nil + else + assert_nil group_perm2 + end - vm_perm = find_obj_in_resp response, 'Link', 'arvados#virtualMachine' - verify_link vm_perm, 'permission', 'can_login', resp_user[:uuid], vm.uuid - assert_equal("foo", vm_perm.properties["username"]) + repo_perm = find_obj_in_resp response, 'Link', 'arvados#repository' + verify_link repo_perm, 'permission', 'can_manage', resp_user[:uuid], nil + + vm_perm = find_obj_in_resp response, 'Link', 'arvados#virtualMachine' + verify_link vm_perm, 'permission', 'can_login', resp_user[:uuid], vm.uuid + assert_equal("foo", vm_perm.properties["username"]) + end end test "setup new user with junk in database" do @@ -514,6 +524,9 @@ class UserTest < ActiveSupport::TestCase group_perm = find_obj_in_resp response, 'Link', 'arvados#group' verify_link group_perm, 'permission', 'can_read', resp_user[:uuid], nil + group_perm2 = find_obj_in_resp response, 'Link', 'arvados#user' + verify_link group_perm2, 'permission', 'can_read', groups(:all_users).uuid, nil + # invoke setup again with repo_name response = user.setup(repo_name: 'foo/testrepo') resp_user = find_obj_in_resp response, 'User', nil @@ -560,7 +573,7 @@ class UserTest < ActiveSupport::TestCase break end else # looking for a link - if ArvadosModel::resource_class_for_uuid(x['head_uuid']).kind == head_kind + if ArvadosModel::resource_class_for_uuid(x['head_uuid']).andand.kind == head_kind return_obj = x break end -- 2.30.2