From af5016004d8b7e2a0dafee9cb9c1c260ffa7c9e5 Mon Sep 17 00:00:00 2001
From: Hinam Mehra <hmehra@gitlab.com>
Date: Thu, 1 Feb 2024 14:28:11 +0000
Subject: [PATCH] Allow creation of group-level custom-roles on self-managed
instances
- Introduce beta feature-flag, restrict_member_roles,
which is disabled by default.
- When it's off, admins can create group-level and instance-level
custom roles when on a self-managed instance.
- When it's on, admins can only create instance-level custom
roles on a self-managed instance.
- When on SaaS mode, only group-level roles can be created.
Changelog: added
EE: true
---
.../graphql/mutations/member_roles/create.rb | 6 +-
.../beta/restrict_member_roles.yml | 9 ++
ee/spec/features/admin/member_roles_spec.rb | 48 ++++++---
ee/spec/features/groups/member_roles_spec.rb | 67 +++++++-----
.../finders/member_roles/roles_finder_spec.rb | 2 +-
.../member_role/create_member_role_spec.rb | 101 ++++++++++--------
6 files changed, 146 insertions(+), 87 deletions(-)
create mode 100644 ee/config/feature_flags/beta/restrict_member_roles.yml
diff --git a/ee/app/graphql/mutations/member_roles/create.rb b/ee/app/graphql/mutations/member_roles/create.rb
index ab825731acb11..49924de1819d1 100644
--- a/ee/app/graphql/mutations/member_roles/create.rb
+++ b/ee/app/graphql/mutations/member_roles/create.rb
@@ -49,7 +49,7 @@ def authorize_admin_roles!(group)
end
def authorize_group_member_roles!(group)
- raise_resource_not_available_error! unless saas?
+ raise_resource_not_available_error! if restrict_member_roles? && !saas?
raise_resource_not_available_error! unless Ability.allowed?(current_user, :admin_member_role, group)
raise_resource_not_available_error! unless group.custom_roles_enabled?
end
@@ -75,6 +75,10 @@ def canonicalize(args)
new_args[permission.downcase] = true
end
end
+
+ def restrict_member_roles?
+ Feature.enabled?(:restrict_member_roles, type: :beta)
+ end
end
end
end
diff --git a/ee/config/feature_flags/beta/restrict_member_roles.yml b/ee/config/feature_flags/beta/restrict_member_roles.yml
new file mode 100644
index 0000000000000..5f94cc05989f4
--- /dev/null
+++ b/ee/config/feature_flags/beta/restrict_member_roles.yml
@@ -0,0 +1,9 @@
+---
+name: restrict_member_roles
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/439167
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/142637
+rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/17458
+milestone: '16.9'
+group: group::authorization
+type: beta
+default_enabled: false
\ No newline at end of file
diff --git a/ee/spec/features/admin/member_roles_spec.rb b/ee/spec/features/admin/member_roles_spec.rb
index 023cb91a42bfa..22527be7765b3 100644
--- a/ee/spec/features/admin/member_roles_spec.rb
+++ b/ee/spec/features/admin/member_roles_spec.rb
@@ -11,10 +11,6 @@
let(:permission_name) { permission.to_s.humanize }
let(:access_level) { 'Developer' }
- before_all do
- sign_in(admin)
- end
-
before do
stub_licensed_features(custom_roles: true)
end
@@ -37,21 +33,45 @@ def created_role(name, id, access_level, permissions)
before do
allow(Gitlab::CustomRoles::Definition).to receive(:all).and_return(permissions)
- visit admin_application_settings_roles_and_permissions_path
+ gitlab_sign_in(admin)
+ end
+
+ shared_examples 'creates a new custom role' do
+ it 'and displays it' do
+ create_role(access_level, name, [permission_name])
+
+ created_member_role = MemberRole.find_by(
+ name: name,
+ base_access_level: Gitlab::Access.options[access_level],
+ permission => true)
+
+ expect(created_member_role).not_to be_nil
+
+ role = created_role(name, created_member_role.id, access_level, [permission_name])
+ expect(page).to have_content(role)
+ end
end
- it 'creates a new custom role' do
- create_role(access_level, name, [permission_name])
+ context 'when on self-managed' do
+ before do
+ stub_saas_features(group_custom_roles: false)
+
+ visit admin_application_settings_roles_and_permissions_path
+ end
+
+ it_behaves_like 'creates a new custom role'
+ end
- created_member_role = MemberRole.find_by(
- name: name,
- base_access_level: Gitlab::Access.options[access_level],
- permission => true)
+ context 'when on SaaS', :saas do
+ before do
+ visit admin_application_settings_roles_and_permissions_path
+ end
- expect(created_member_role).not_to be_nil
+ it 'shows an error message' do
+ create_role(access_level, name, [permission_name])
- role = created_role(name, created_member_role.id, access_level, [permission_name])
- expect(page).to have_content(role)
+ expect(page).to have_content('Failed to create role')
+ end
end
end
end
diff --git a/ee/spec/features/groups/member_roles_spec.rb b/ee/spec/features/groups/member_roles_spec.rb
index d8b72feb66286..a499ddbb16d98 100644
--- a/ee/spec/features/groups/member_roles_spec.rb
+++ b/ee/spec/features/groups/member_roles_spec.rb
@@ -39,46 +39,59 @@ def created_role(name, id, access_level, permissions)
allow(Gitlab::CustomRoles::Definition).to receive(:all).and_return(permissions)
sign_in(user)
- visit group_settings_roles_and_permissions_path(group)
end
- it 'creates a new custom role' do
- create_role(access_level, name, [permission_name])
+ shared_examples 'creates a new custom role' do
+ it 'and displays it' do
+ create_role(access_level, name, [permission_name])
- created_member_role = MemberRole.find_by(
- name: name,
- base_access_level: Gitlab::Access.options[access_level],
- permission => true)
+ created_member_role = MemberRole.find_by(
+ name: name,
+ base_access_level: Gitlab::Access.options[access_level],
+ permission => true)
- expect(created_member_role).not_to be_nil
+ expect(created_member_role).not_to be_nil
- role = created_role(name, created_member_role.id, access_level, [permission_name])
- expect(page).to have_content(role)
+ role = created_role(name, created_member_role.id, access_level, [permission_name])
+ expect(page).to have_content(role)
+ end
end
- context 'when the permission has a requirement' do
- let(:permissions) do
- { admin_vulnerability: { name: 'admin_vulnerability', requirements: ['read_vulnerability'] },
- read_vulnerability: { name: 'read_vulnerability' } }
+ context 'when on SaaS' do
+ before do
+ visit group_settings_roles_and_permissions_path(group)
end
- let(:permission) { :admin_vulnerability }
- let(:requirement) { permissions[permission][:requirements].first }
- let(:requirement_name) { requirement.to_s.humanize }
+ it_behaves_like 'creates a new custom role'
+ end
- it 'creates the custom role' do
- create_role(access_level, name, [permission_name])
+ context 'when on self-managed' do
+ before do
+ stub_saas_features(group_custom_roles: false)
+ end
- created_member_role = MemberRole.find_by(
- name: name,
- base_access_level: Gitlab::Access.options[access_level],
- permission => true,
- requirement => true)
+ context 'when restrict_member_roles feature-flag is disabled' do
+ before do
+ stub_feature_flags(restrict_member_roles: false)
- expect(created_member_role).not_to be_nil
+ visit group_settings_roles_and_permissions_path(group)
+ end
- role = created_role(name, created_member_role.id, access_level, [permission_name, requirement_name])
- expect(page).to have_content(role)
+ it_behaves_like 'creates a new custom role'
+ end
+
+ context 'when restrict_member_roles feature-flag is enabled' do
+ before do
+ stub_feature_flags(restrict_member_roles: true)
+
+ visit group_settings_roles_and_permissions_path(group)
+ end
+
+ it 'shows an error message' do
+ create_role(access_level, name, [permission_name])
+
+ expect(page).to have_content('Failed to create role')
+ end
end
end
end
diff --git a/ee/spec/finders/member_roles/roles_finder_spec.rb b/ee/spec/finders/member_roles/roles_finder_spec.rb
index 6ca297eaa0877..b42c39fd49f89 100644
--- a/ee/spec/finders/member_roles/roles_finder_spec.rb
+++ b/ee/spec/finders/member_roles/roles_finder_spec.rb
@@ -174,7 +174,7 @@
let(:current_user) { admin }
context 'when on self-managed' do
- it 'returns instance member roles for instance admin' do
+ it 'returns instance member roles' do
expect(find_member_roles).to eq([member_role_instance])
end
end
diff --git a/ee/spec/requests/api/graphql/mutations/member_role/create_member_role_spec.rb b/ee/spec/requests/api/graphql/mutations/member_role/create_member_role_spec.rb
index 0e3373ea6a31b..443f2fdc95813 100644
--- a/ee/spec/requests/api/graphql/mutations/member_role/create_member_role_spec.rb
+++ b/ee/spec/requests/api/graphql/mutations/member_role/create_member_role_spec.rb
@@ -38,6 +38,28 @@
subject(:create_member_role) { graphql_mutation_response(:member_role_create) }
+ shared_examples 'a mutation that creates a member role' do
+ it 'returns success', :aggregate_failures do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(graphql_errors).to be_nil
+
+ expect(create_member_role['memberRole']['enabledPermissions']['nodes'].flat_map(&:values))
+ .to match_array(permissions)
+ end
+
+ it 'creates the member role', :aggregate_failures do
+ expect { post_graphql_mutation(mutation, current_user: current_user) }
+ .to change { MemberRole.count }.by(1)
+
+ member_role = MemberRole.last
+
+ expect(member_role.read_vulnerability).to eq(true)
+
+ expect(member_role.namespace).to eq(group)
+ end
+ end
+
context 'without the custom roles feature' do
before do
stub_licensed_features(custom_roles: false)
@@ -74,49 +96,32 @@
end
context 'when on self-managed' do
- it_behaves_like 'a mutation that returns a top-level access error'
- end
+ context 'when restrict_member_roles feature-flag is disabled' do
+ before do
+ stub_feature_flags(restrict_member_roles: false)
+ end
- context 'when on Saas', :saas do
- context 'with valid arguments' do
- it 'returns success' do
- post_graphql_mutation(mutation, current_user: current_user)
+ it_behaves_like 'a mutation that creates a member role'
+ end
- expect(graphql_errors).to be_nil
- expect(create_member_role['memberRole']['enabledPermissions']['nodes'].flat_map(&:values))
- .to match_array(MemberRole.all_customizable_permissions.keys.map(&:to_s).map(&:upcase))
+ context 'when restrict_member_roles feature-flag is enabled' do
+ before do
+ stub_feature_flags(restrict_member_roles: true)
end
- it 'creates the member role' do
- expect { post_graphql_mutation(mutation, current_user: current_user) }
- .to change { MemberRole.count }.by(1)
-
- member_role = MemberRole.last
+ it_behaves_like 'a mutation that returns a top-level access error'
+ end
+ end
- expect(member_role.read_vulnerability).to eq(true)
- expect(member_role.namespace).to eq(group)
- end
+ context 'when on SaaS', :saas do
+ context 'with valid arguments' do
+ it_behaves_like 'a mutation that creates a member role'
end
context 'with an array of permissions' do
let(:permissions) { ['READ_VULNERABILITY'] }
- it 'returns success' do
- post_graphql_mutation(mutation, current_user: current_user)
-
- expect(graphql_errors).to be_nil
- mutation_response = create_member_role['memberRole']
- expect(mutation_response['enabledPermissions']['nodes'].flat_map(&:values)).to eq(['READ_VULNERABILITY'])
- end
-
- it 'creates a member role with the specified permissions' do
- expect do
- post_graphql_mutation(mutation, current_user: current_user)
- end.to change { MemberRole.count }.by(1)
-
- member_role = MemberRole.last
- expect(member_role.read_vulnerability).to eq(true)
- end
+ it_behaves_like 'a mutation that creates a member role'
end
context 'with an unknown permission' do
@@ -139,8 +144,11 @@
end
context 'when creating an instance level member role' do
- before do
- input.delete(:group_path)
+ let(:input) do
+ {
+ base_access_level: 'GUEST',
+ permissions: permissions
+ }
end
context 'with unauthorized user' do
@@ -152,30 +160,35 @@
current_user.update!(admin: true)
end
- context 'when on SaaS', :saas do
- it_behaves_like 'a mutation that returns top-level errors', errors: ['group_path argument is required.']
- end
-
- context 'when running on self-managed' do
- it 'returns success' do
+ context 'when on self-managed' do
+ it 'returns success', :aggregate_failures do
post_graphql_mutation(mutation, current_user: current_user)
expect(graphql_errors).to be_nil
+
expect(create_member_role['memberRole']['enabledPermissions']['nodes'].flat_map(&:values))
- .to include('READ_VULNERABILITY')
- expect(create_member_role['memberRole']['namespace']).to be_nil
+ .to match_array(permissions)
end
- it 'creates the member role' do
+ it 'creates the member role', :aggregate_failures do
expect { post_graphql_mutation(mutation, current_user: current_user) }
.to change { MemberRole.count }.by(1)
member_role = MemberRole.last
expect(member_role.read_vulnerability).to eq(true)
+
expect(member_role.namespace).to be_nil
end
end
+
+ context 'when on SaaS', :saas do
+ before do
+ stub_feature_flags(restrict_member_roles: false)
+ end
+
+ it_behaves_like 'a mutation that returns top-level errors', errors: ['group_path argument is required.']
+ end
end
end
end
--
GitLab