From: Tom Clegg Date: Fri, 10 Dec 2021 14:46:00 +0000 (-0500) Subject: 18277: Add RoleGroupsVisibleToAll config, default true. X-Git-Tag: 2.4.0~138^2~1 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/b7fb5c4593dcc679f5343f0f55b3774a7bcfe499 18277: Add RoleGroupsVisibleToAll config, default true. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/doc/admin/upgrading.html.textile.liquid b/doc/admin/upgrading.html.textile.liquid index 517c2dd0cd..411e2ea0ff 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-11-10) "previous: Upgrading from 2.3.0":#v2_3_0 +h3. Role groups are visible to all users by default + +The permission model has changed such that all role groups are visible to all active users. This enables users to share objects with groups they don't belong to. To preserve the previous behavior, where role groups are only visible to members and admins, add @RoleGroupsVisibleToAll: false@ to the @Users@ section of your configuration file. + h3. Default LSF arguments have changed If you use LSF and your configuration specifies @Containers.LSF.BsubArgumentsList@, you should update it to include the new arguments (@"-R", "select[mem>=%MMB]", ...@, see "configuration reference":{{site.baseurl}}/admin/config.html). Otherwise, containers that are too big to run on any LSF host will remain in the LSF queue instead of being cancelled. diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index a84dc5d316..7f85982253 100644 --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@ -319,6 +319,14 @@ Clusters: Thanks, Your Arvados administrator. + # If RoleGroupsVisibleToAll is true, all role groups are visible + # to all active users. + # + # If false, users must be granted permission to role groups in + # order to see them. This is more appropriate for a multi-tenant + # cluster. + RoleGroupsVisibleToAll: true + AuditLogs: # Time to keep audit logs, in seconds. (An audit log is a row added # to the "logs" table in the PostgreSQL database each time an diff --git a/lib/config/export.go b/lib/config/export.go index 4c4e341f5a..cfc4cb6272 100644 --- a/lib/config/export.go +++ b/lib/config/export.go @@ -234,6 +234,7 @@ var whitelist = map[string]bool{ "Users.UserNotifierEmailBcc": false, "Users.UserProfileNotificationAddress": false, "Users.UserSetupMailText": false, + "Users.RoleGroupsVisibleToAll": false, "Volumes": true, "Volumes.*": true, "Volumes.*.*": false, diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go index 567ac30a9b..485273e1b0 100644 --- a/lib/config/generated_config.go +++ b/lib/config/generated_config.go @@ -325,6 +325,14 @@ Clusters: Thanks, Your Arvados administrator. + # If RoleGroupsVisibleToAll is true, all role groups are visible + # to all active users. + # + # If false, users must be granted permission to role groups in + # order to see them. This is more appropriate for a multi-tenant + # cluster. + RoleGroupsVisibleToAll: true + AuditLogs: # Time to keep audit logs, in seconds. (An audit log is a row added # to the "logs" table in the PostgreSQL database each time an diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index 474ce33b0e..6d5156e765 100644 --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@ -241,6 +241,7 @@ type Cluster struct { UserProfileNotificationAddress string PreferDomainForUsername string UserSetupMailText string + RoleGroupsVisibleToAll bool } StorageClasses map[string]StorageClassConfig Volumes map[string]Volume diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index af226cde82..54b1394ffd 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -384,6 +384,14 @@ class ArvadosModel < ApplicationRecord direct_check = " OR " + direct_check end + if Rails.configuration.Users.RoleGroupsVisibleToAll && + sql_table == "groups" && + users_list.select { |u| u.is_active }.any? + # All role groups are readable (but we still need the other + # direct_check clauses to handle non-role groups). + direct_check += " OR #{sql_table}.group_class = 'role'" + end + links_cond = "" if sql_table == "links" # 1) Match permission links incoming or outgoing on the diff --git a/services/api/config/arvados_config.rb b/services/api/config/arvados_config.rb index 49865039c4..c5db6e6b9e 100644 --- a/services/api/config/arvados_config.rb +++ b/services/api/config/arvados_config.rb @@ -100,6 +100,7 @@ arvcfg.declare_config "Users.UserNotifierEmailFrom", String, :user_notifier_emai arvcfg.declare_config "Users.UserNotifierEmailBcc", Hash arvcfg.declare_config "Users.NewUserNotificationRecipients", Hash, :new_user_notification_recipients, ->(cfg, k, v) { arrayToHash cfg, "Users.NewUserNotificationRecipients", v } arvcfg.declare_config "Users.NewInactiveUserNotificationRecipients", Hash, :new_inactive_user_notification_recipients, method(:arrayToHash) +arvcfg.declare_config "Users.RoleGroupsVisibleToAll", Boolean arvcfg.declare_config "Login.LoginCluster", String arvcfg.declare_config "Login.TrustedClients", Hash arvcfg.declare_config "Login.RemoteTokenRefresh", ActiveSupport::Duration 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 4dbccc5eb2..0819c23067 100644 --- a/services/api/test/functional/arvados/v1/groups_controller_test.rb +++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb @@ -6,12 +6,19 @@ require 'test_helper' class Arvados::V1::GroupsControllerTest < ActionController::TestCase - test "attempt to delete group without read or write access" do + test "attempt to delete group that cannot be seen" do + Rails.configuration.Users.RoleGroupsVisibleToAll = false authorize_with :active post :destroy, params: {id: groups(:empty_lonely_group).uuid} assert_response 404 end + test "attempt to delete group without read or write access" do + authorize_with :active + post :destroy, params: {id: groups(:empty_lonely_group).uuid} + assert_response 403 + end + test "attempt to delete group without write access" do authorize_with :active post :destroy, params: {id: groups(:all_users).uuid} @@ -553,6 +560,30 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase end end + [ + [false, :inactive, :private_role, false], + [false, :spectator, :private_role, false], + [false, :admin, :private_role, true], + [true, :inactive, :private_role, false], + [true, :spectator, :private_role, true], + [true, :admin, :private_role, true], + # project (non-role) groups are invisible even when RoleGroupsVisibleToAll is true + [true, :inactive, :private, false], + [true, :spectator, :private, false], + [true, :admin, :private, true], + ].each do |visibleToAll, userFixture, groupFixture, visible| + test "with RoleGroupsVisibleToAll=#{visibleToAll}, #{groupFixture} group is #{visible ? '' : 'in'}visible to #{userFixture} user" do + Rails.configuration.Users.RoleGroupsVisibleToAll = visibleToAll + authorize_with userFixture + get :show, params: {id: groups(groupFixture).uuid, format: :json} + if visible + assert_response :success + else + assert_response 404 + end + end + end + ### trashed project tests ### # diff --git a/services/api/test/integration/remote_user_test.rb b/services/api/test/integration/remote_user_test.rb index 568d5088d9..179d30f3cb 100644 --- a/services/api/test/integration/remote_user_test.rb +++ b/services/api/test/integration/remote_user_test.rb @@ -304,6 +304,7 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest end test "list readable groups with salted token" do + Rails.configuration.Users.RoleGroupsVisibleToAll = false salted_token = salt_token(fixture: :active, remote: 'zbbbb') get '/arvados/v1/groups', params: { diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index 128d0ebaa6..647139d9ec 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.RoleGroupsVisibleToAll = false 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"