From 7d2a40c80ff552d68459c16d2ec0f453881a909f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Wed, 13 Sep 2023 20:39:05 +0200 Subject: [PATCH 1/3] Add manage group members as custom permissions - add migration adding a new column - change policy classes - add tests Changelog: added EE: true --- .../admin_group_member_custom_roles.yml | 8 +++ ...143103_add_admin_members_to_member_role.rb | 17 ++++++ db/schema_migrations/20230910143103 | 1 + db/structure.sql | 1 + doc/api/member_roles.md | 4 +- doc/user/custom_roles.md | 17 +++--- ee/app/models/members/member_role.rb | 4 ++ ee/app/policies/ee/group_policy.rb | 17 ++++++ ee/spec/models/ee/user_spec.rb | 3 +- ee/spec/policies/group_policy_spec.rb | 29 ++++++++++- ee/spec/requests/api/member_roles_spec.rb | 2 + .../admin_group_member/request_spec.rb | 52 +++++++++++++++++++ lib/api/helpers/members_helpers.rb | 8 +++ lib/api/invitations.rb | 27 ++++++++-- lib/api/members.rb | 14 +++-- 15 files changed, 187 insertions(+), 17 deletions(-) create mode 100644 config/feature_flags/development/admin_group_member_custom_roles.yml create mode 100644 db/migrate/20230910143103_add_admin_members_to_member_role.rb create mode 100644 db/schema_migrations/20230910143103 create mode 100644 ee/spec/requests/custom_roles/admin_group_member/request_spec.rb diff --git a/config/feature_flags/development/admin_group_member_custom_roles.yml b/config/feature_flags/development/admin_group_member_custom_roles.yml new file mode 100644 index 00000000000000..fa83a94963cd2f --- /dev/null +++ b/config/feature_flags/development/admin_group_member_custom_roles.yml @@ -0,0 +1,8 @@ +--- +name: admin_group_member_custom_roles +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131914 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/426580 +milestone: '16.4' +type: development +group: group::authentication and authorization +default_enabled: false diff --git a/db/migrate/20230910143103_add_admin_members_to_member_role.rb b/db/migrate/20230910143103_add_admin_members_to_member_role.rb new file mode 100644 index 00000000000000..950a427b0e2bdc --- /dev/null +++ b/db/migrate/20230910143103_add_admin_members_to_member_role.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddAdminMembersToMemberRole < Gitlab::Database::Migration[2.1] + enable_lock_retries! + + def up + return if column_exists?(:member_roles, :admin_group_member) + + add_column :member_roles, :admin_group_member, :boolean, default: false, null: false + end + + def down + return unless column_exists?(:member_roles, :admin_group_member) + + remove_column :member_roles, :admin_group_member + end +end diff --git a/db/schema_migrations/20230910143103 b/db/schema_migrations/20230910143103 new file mode 100644 index 00000000000000..0cdb16e4b854a4 --- /dev/null +++ b/db/schema_migrations/20230910143103 @@ -0,0 +1 @@ +42b5ec67a607af2774d224f6b90b05a419bedcb33ef17b66f893a446e03c0839 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 324888e9173b19..ee72e5cfba96dd 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18124,6 +18124,7 @@ CREATE TABLE member_roles ( name text DEFAULT 'Custom'::text NOT NULL, description text, admin_merge_request boolean DEFAULT false NOT NULL, + admin_group_member boolean DEFAULT false NOT NULL, CONSTRAINT check_4364846f58 CHECK ((char_length(description) <= 255)), CONSTRAINT check_9907916995 CHECK ((char_length(name) <= 255)) ); diff --git a/doc/api/member_roles.md b/doc/api/member_roles.md index c5d68371afd3de..0b94cdf8c0dcff 100644 --- a/doc/api/member_roles.md +++ b/doc/api/member_roles.md @@ -13,9 +13,10 @@ info: To determine the technical writer assigned to the Stage/Group associated w > - [Read dependency added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126247) in GitLab 16.3. > - [Name and description fields added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126423) in GitLab 16.3. > - [Admin merge request introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/128302) in GitLab 16.4 [with a flag](../administration/feature_flags.md) named `admin_merge_request`. Disabled by default. +> - [Admin group members introduced]([TODO](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131914)) in GitLab 16.4 [with a flag](../administration/feature_flags.md) named `admin_group_member_custom_roles`. Disabled by default. FLAG: -On self-managed GitLab, by default this feature is not available. To make it available, an administrator can [enable the feature flag](../administration/feature_flags.md) named `admin_merge_request`. +On self-managed GitLab, by default these two features are not available. To make them available, an administrator can [enable the feature flag](../administration/feature_flags.md) named `admin_merge_request` and `admin_member_custom_role`. On GitLab.com, this feature is not available. ## List all member roles of a group @@ -44,6 +45,7 @@ If successful, returns [`200`](rest/index.md#status-codes) and the following res | `[].read_code` | boolean | Permission to read project code. | | `[].read_dependency` | boolean | Permission to read project dependencies. | | `[].read_vulnerability` | boolean | Permission to read project vulnerabilities. | +| `[].admin_member` | boolean | Permission to admin members of a group. | Example request: diff --git a/doc/user/custom_roles.md b/doc/user/custom_roles.md index 9ae65c681fc552..ac81ec07f266ad 100644 --- a/doc/user/custom_roles.md +++ b/doc/user/custom_roles.md @@ -13,6 +13,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w > - Ability to view a vulnerability report [enabled by default](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123835) in GitLab 16.1. > - [Feature flag `custom_roles_vulnerability` removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124049) in GitLab 16.2. > - Ability to create and remove a custom role with the UI [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/393235) in GitLab 16.4. +> - Ability to manage group members [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/17364) in GitLab 16.5 under `admin_group_member_custom_roles` Feature flag. Custom roles allow group members who are assigned the Owner role to create roles specific to the needs of their organization. @@ -26,6 +27,7 @@ The following granular permissions are available. You can add these permissions - In GitLab 16.1 and later, you can create a custom role that can view vulnerability reports and change the status of the vulnerabilities. - In GitLab 16.3 and later, you can create a custom role that can view the dependency list. - In GitLab 16.4 and later, you can create a custom role that can approve merge requests. +- In GitLab 16.5 and later, you can create a custom role that can manage group members. You can discuss individual custom role and permission requests in [issue 391760](https://gitlab.com/gitlab-org/gitlab/-/issues/391760). @@ -92,13 +94,14 @@ Some roles and abilities require having other abilities enabled. For example, a You can see the required minimal access levels and abilities requirements in the following table. -| Ability | Minimal access level | Required ability | -| -- | -- | -- | -| `read_code` | Guest | - | -| `read_dependency` | Guest | - | -| `read_vulnerability` | Guest | - | -| `admin_merge_request` | Guest | - | -| `admin_vulnerability` | Guest | `read_vulnerability` | +| Ability | Required ability | +| -- | -- | +| `read_code` | - | +| `read_dependency` | - | +| `read_vulnerability` | - | +| `admin_merge_request` | - | +| `admin_vulnerability` | `read_vulnerability` | +| `admin_group_member` | `read_vulnerability` | ## Associate a custom role with an existing group member diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb index 1be5057d724f2d..f923264208e258 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -3,6 +3,9 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass MAX_COUNT_PER_GROUP_HIERARCHY = 10 ALL_CUSTOMIZABLE_PERMISSIONS = { + admin_group_member: { + descripition: 'Permission to admin group member' + }, admin_merge_request: { description: 'Allows admin access to the merge requests.' }, @@ -30,6 +33,7 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass ALL_CUSTOMIZABLE_GROUP_PERMISSIONS = [ :read_dependency, :read_vulnerability, + :admin_group_member, :admin_vulnerability ].freeze diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index bfaaba90a36aea..d5ddd0c4f8918e 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -195,6 +195,10 @@ module GroupPolicy @subject.custom_roles_enabled? end + condition(:admin_group_member_custom_roles_allowed) do + ::Feature.enabled?(:admin_group_member_custom_roles, @subject.root_ancestor) + end + desc "Custom role on group that enables read dependency" condition(:role_enables_read_dependency) do ::Auth::MemberRoleAbilityLoader.new( @@ -222,6 +226,13 @@ module GroupPolicy ).has_ability? end + desc "Custom role on group that enables admin members" + condition(:role_enables_admin_member) do + next unless @user.is_a?(User) + + @user.custom_permission_for?(@subject, :admin_group_member) + end + rule { owner & unique_project_download_limit_enabled }.policy do enable :ban_group_member end @@ -477,6 +488,12 @@ module GroupPolicy enable :admin_vulnerability end + rule { custom_roles_allowed & admin_group_member_custom_roles_allowed & role_enables_admin_member }.policy do + enable :admin_group_member + enable :update_group_member + enable :destroy_group_member + end + rule { can?(:read_group_security_dashboard) }.policy do enable :create_vulnerability_export enable :read_security_resource diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 9a9d2967ec8a21..700b4fa20484c8 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -1177,7 +1177,8 @@ WHERE "members"."user_id" = "users"."id" AND \(members.access_level > 10 OR "members"."access_level" = 10 - AND \(admin_merge_request = true + AND \(admin_group_member = true + OR admin_merge_request = true OR admin_vulnerability = true OR read_dependency = true OR read_vulnerability = true\)\)\)\)'.squish # allow_cross_joins_across_databases diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 5cfecca35d6c7c..ce81a3054715c1 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -2878,7 +2878,7 @@ def create_member_role(member, abilities = member_role_abilities) shared_examples 'custom roles abilities' do subject { described_class.new(current_user, group) } - context 'without custom_roles license enabled' do + context 'without custom_roles license disabled' do before do create_member_role(group_member_guest) @@ -2947,5 +2947,32 @@ def create_member_role(member, abilities = member_role_abilities) it_behaves_like 'custom roles abilities' end + + context 'for a member role with admin_group_member true' do + let(:member_role_abilities) { { admin_group_member: true } } + + context 'with admin_group_member_custom_roles FF enabled' do + before do + stub_feature_flags(admin_group_member_custom_roles: [parent_group]) + end + + let(:allowed_abilities) { [:admin_group_member] } + + it_behaves_like 'custom roles abilities' + end + + context 'with admin_group_member_custom_roles FF disabled' do + before do + stub_feature_flags(admin_group_member_custom_roles: false) + stub_licensed_features(custom_roles: true) + + create_member_role(group_member_guest) + end + + let(:disallowed_abilities) { [:admin_group_member] } + + it { is_expected.to be_disallowed(*disallowed_abilities) } + end + end end end diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index e7290307659d96..b426249efb5d26 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -102,6 +102,7 @@ "read_code" => false, "read_dependency" => false, "read_vulnerability" => true, + "admin_group_member" => false, "admin_merge_request" => false, "admin_vulnerability" => false, "group_id" => group_id @@ -114,6 +115,7 @@ "read_code" => true, "read_dependency" => true, "read_vulnerability" => false, + "admin_group_member" => false, "admin_merge_request" => true, "admin_vulnerability" => false, "group_id" => group_id diff --git a/ee/spec/requests/custom_roles/admin_group_member/request_spec.rb b/ee/spec/requests/custom_roles/admin_group_member/request_spec.rb new file mode 100644 index 00000000000000..ef2702e2aa5d42 --- /dev/null +++ b/ee/spec/requests/custom_roles/admin_group_member/request_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'User with admin_group_member custom role', feature_category: :groups_and_projects do + let_it_be(:current_user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:role) { create(:member_role, :guest, namespace: group, admin_group_member: true) } + let_it_be(:membership) { create(:group_member, :guest, member_role: role, user: current_user, group: group) } + + let_it_be(:group_member) { create(:group_member, :developer, group: group) } + + before do + stub_licensed_features(custom_roles: true) + + sign_in(current_user) + end + + describe Groups::GroupMembersController do + describe '#update' do + it 'user can update a member via a custom role' do + put group_group_member_path(group_id: group, id: group_member), params: { + group_member: { + access_level: 50 + } + } + + expect(response).to have_gitlab_http_status(:ok) + end + end + + describe '#delete' do + it 'user can delete a member via a custom role' do + delete group_group_member_path(group_id: group, id: group_member) + + expect(response).to have_gitlab_http_status(:see_other) + expect(flash[:notice]).to eq('User was successfully removed from group.') + end + end + + describe '#approve_access_request' do + let_it_be(:access_request) { create(:group_member, :access_request, group: group) } + + it 'user can delete a member via a custom role' do + post approve_access_request_group_group_member_path(group_id: group, id: access_request) + + expect(response).to redirect_to(group_group_members_path(group)) + expect(group.members).to include(access_request) + end + end + end +end diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb index a82aed507fd2ec..1a23dcd0d3c09f 100644 --- a/lib/api/helpers/members_helpers.rb +++ b/lib/api/helpers/members_helpers.rb @@ -22,6 +22,14 @@ def authorize_read_source_member!(source_type, source) authorize! :"read_#{source_type}_member", source end + def authorize_admin_source_member!(source_type, source) + authorize! :"admin_#{source_type}_member", source + end + + def authorize_update_source_member!(source_type, member) + authorize! :"update_#{source_type}_member", member + end + def authorize_admin_source!(source_type, source) authorize! :"admin_#{source_type}", source end diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb index dff7b5243aaecc..5af85ec0845f12 100644 --- a/lib/api/invitations.rb +++ b/lib/api/invitations.rb @@ -33,7 +33,12 @@ class Invitations < ::API::Base bad_request!('Must provide either email or user_id as a parameter') if params[:email].blank? && params[:user_id].blank? source = find_source(source_type, params[:id]) - authorize_admin_source!(source_type, source) + + if ::Feature.enabled?(:admin_group_member_custom_roles, source) + authorize_admin_source_member!(source_type, source) + else + authorize_admin_source!(source_type, source) + end create_service_params = params.merge(source: source) @@ -56,7 +61,11 @@ class Invitations < ::API::Base source = find_source(source_type, params[:id]) query = params[:query] - authorize_admin_source!(source_type, source) + if ::Feature.enabled?(:admin_group_member_custom_roles, source) + authorize_admin_source_member!(source_type, source) + else + authorize_admin_source!(source_type, source) + end invitations = paginate(retrieve_member_invitations(source, query)) @@ -75,7 +84,12 @@ class Invitations < ::API::Base put ":id/invitations/:email", requirements: { email: %r{[^/]+} } do source = find_source(source_type, params.delete(:id)) invite_email = params[:email] - authorize_admin_source!(source_type, source) + + if ::Feature.enabled?(:admin_group_member_custom_roles, source) + authorize_admin_source_member!(source_type, source) + else + authorize_admin_source!(source_type, source) + end invite = retrieve_member_invitations(source, invite_email).first not_found! unless invite @@ -112,7 +126,12 @@ class Invitations < ::API::Base delete ":id/invitations/:email", requirements: { email: %r{[^/]+} } do source = find_source(source_type, params[:id]) invite_email = params[:email] - authorize_admin_source!(source_type, source) + + if ::Feature.enabled?(:admin_group_member_custom_roles, source) + authorize_admin_source_member!(source_type, source) + else + authorize_admin_source!(source_type, source) + end invite = retrieve_member_invitations(source, invite_email).first not_found! unless invite diff --git a/lib/api/members.rb b/lib/api/members.rb index 6dc4e45ffc116d..f7c242142526a7 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -118,7 +118,11 @@ class Members < ::API::Base post ":id/members", feature_category: feature_category do source = find_source(source_type, params[:id]) - authorize_admin_source!(source_type, source) + if ::Feature.enabled?(:admin_group_member_custom_roles, source) + authorize_admin_source_member!(source_type, source) + else + authorize_admin_source!(source_type, source) + end create_service_params = params.merge(source: source) @@ -142,10 +146,14 @@ class Members < ::API::Base # rubocop: disable CodeReuse/ActiveRecord put ":id/members/:user_id", feature_category: feature_category do source = find_source(source_type, params.delete(:id)) - authorize_admin_source!(source_type, source) - member = source_members(source).find_by!(user_id: params[:user_id]) + if ::Feature.enabled?(:admin_group_member_custom_roles, source) + authorize_update_source_member!(source_type, member) + else + authorize_admin_source!(source_type, source) + end + result = ::Members::UpdateService .new(current_user, declared_params(include_missing: false)) .execute(member) -- GitLab From 437f7ae782255a68a42a96a4538b9e94b4600b23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Thu, 5 Oct 2023 10:31:46 +0200 Subject: [PATCH 2/3] Fix milestone --- .../development/admin_group_member_custom_roles.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/feature_flags/development/admin_group_member_custom_roles.yml b/config/feature_flags/development/admin_group_member_custom_roles.yml index fa83a94963cd2f..1b97eba10f33e3 100644 --- a/config/feature_flags/development/admin_group_member_custom_roles.yml +++ b/config/feature_flags/development/admin_group_member_custom_roles.yml @@ -2,7 +2,7 @@ name: admin_group_member_custom_roles introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131914 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/426580 -milestone: '16.4' +milestone: '16.5' type: development group: group::authentication and authorization default_enabled: false -- GitLab From 3ad1dc60b10c2266592f30b1701cc4aca33e345e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Thu, 12 Oct 2023 15:11:27 +0200 Subject: [PATCH 3/3] Apply code review suggestions --- ...ustom_roles.yml => admin_group_member.yml} | 2 +- ...143103_add_admin_members_to_member_role.rb | 4 - doc/api/member_roles.md | 6 +- doc/user/custom_roles.md | 6 +- ee/app/models/members/member_role.rb | 6 +- ee/app/policies/ee/group_policy.rb | 16 +- ee/spec/policies/group_policy_spec.rb | 8 +- lib/api/invitations.rb | 8 +- lib/api/members.rb | 4 +- spec/requests/api/invitations_spec.rb | 140 ++++++++++++++++-- spec/requests/api/members_spec.rb | 50 +++++-- 11 files changed, 192 insertions(+), 58 deletions(-) rename config/feature_flags/development/{admin_group_member_custom_roles.yml => admin_group_member.yml} (87%) diff --git a/config/feature_flags/development/admin_group_member_custom_roles.yml b/config/feature_flags/development/admin_group_member.yml similarity index 87% rename from config/feature_flags/development/admin_group_member_custom_roles.yml rename to config/feature_flags/development/admin_group_member.yml index 1b97eba10f33e3..c6267dd3fe3426 100644 --- a/config/feature_flags/development/admin_group_member_custom_roles.yml +++ b/config/feature_flags/development/admin_group_member.yml @@ -1,5 +1,5 @@ --- -name: admin_group_member_custom_roles +name: admin_group_member introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131914 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/426580 milestone: '16.5' diff --git a/db/migrate/20230910143103_add_admin_members_to_member_role.rb b/db/migrate/20230910143103_add_admin_members_to_member_role.rb index 950a427b0e2bdc..a4d191ad7db26a 100644 --- a/db/migrate/20230910143103_add_admin_members_to_member_role.rb +++ b/db/migrate/20230910143103_add_admin_members_to_member_role.rb @@ -4,14 +4,10 @@ class AddAdminMembersToMemberRole < Gitlab::Database::Migration[2.1] enable_lock_retries! def up - return if column_exists?(:member_roles, :admin_group_member) - add_column :member_roles, :admin_group_member, :boolean, default: false, null: false end def down - return unless column_exists?(:member_roles, :admin_group_member) - remove_column :member_roles, :admin_group_member end end diff --git a/doc/api/member_roles.md b/doc/api/member_roles.md index 0b94cdf8c0dcff..36f9b4694b95ae 100644 --- a/doc/api/member_roles.md +++ b/doc/api/member_roles.md @@ -13,10 +13,10 @@ info: To determine the technical writer assigned to the Stage/Group associated w > - [Read dependency added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126247) in GitLab 16.3. > - [Name and description fields added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/126423) in GitLab 16.3. > - [Admin merge request introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/128302) in GitLab 16.4 [with a flag](../administration/feature_flags.md) named `admin_merge_request`. Disabled by default. -> - [Admin group members introduced]([TODO](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131914)) in GitLab 16.4 [with a flag](../administration/feature_flags.md) named `admin_group_member_custom_roles`. Disabled by default. +> - [Admin group members introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131914) in GitLab 16.5 [with a flag](../administration/feature_flags.md) named `admin_group_member`. Disabled by default. FLAG: -On self-managed GitLab, by default these two features are not available. To make them available, an administrator can [enable the feature flag](../administration/feature_flags.md) named `admin_merge_request` and `admin_member_custom_role`. +On self-managed GitLab, by default these two features are not available. To make them available, an administrator can [enable the feature flags](../administration/feature_flags.md) named `admin_merge_request` and `admin_member_custom_role`. On GitLab.com, this feature is not available. ## List all member roles of a group @@ -45,7 +45,7 @@ If successful, returns [`200`](rest/index.md#status-codes) and the following res | `[].read_code` | boolean | Permission to read project code. | | `[].read_dependency` | boolean | Permission to read project dependencies. | | `[].read_vulnerability` | boolean | Permission to read project vulnerabilities. | -| `[].admin_member` | boolean | Permission to admin members of a group. | +| `[].admin_group_member` | boolean | Permission to admin members of a group. | Example request: diff --git a/doc/user/custom_roles.md b/doc/user/custom_roles.md index ac81ec07f266ad..53cb835eed1e7e 100644 --- a/doc/user/custom_roles.md +++ b/doc/user/custom_roles.md @@ -13,7 +13,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w > - Ability to view a vulnerability report [enabled by default](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123835) in GitLab 16.1. > - [Feature flag `custom_roles_vulnerability` removed](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/124049) in GitLab 16.2. > - Ability to create and remove a custom role with the UI [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/393235) in GitLab 16.4. -> - Ability to manage group members [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/17364) in GitLab 16.5 under `admin_group_member_custom_roles` Feature flag. +> - Ability to manage group members [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/17364) in GitLab 16.5 under `admin_group_member` Feature flag. Custom roles allow group members who are assigned the Owner role to create roles specific to the needs of their organization. @@ -92,7 +92,7 @@ For every ability, a minimal access level is defined. To be able to create a cus Some roles and abilities require having other abilities enabled. For example, a custom role can only have administration of vulnerabilities (`admin_vulnerability`) enabled if reading vulnerabilities (`read_vulnerability`) is also enabled. -You can see the required minimal access levels and abilities requirements in the following table. +You can see the abilities requirements in the following table. | Ability | Required ability | | -- | -- | @@ -101,7 +101,7 @@ You can see the required minimal access levels and abilities requirements in the | `read_vulnerability` | - | | `admin_merge_request` | - | | `admin_vulnerability` | `read_vulnerability` | -| `admin_group_member` | `read_vulnerability` | +| `admin_group_member` | - | ## Associate a custom role with an existing group member diff --git a/ee/app/models/members/member_role.rb b/ee/app/models/members/member_role.rb index f923264208e258..456d5d5b01056c 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -4,7 +4,7 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass MAX_COUNT_PER_GROUP_HIERARCHY = 10 ALL_CUSTOMIZABLE_PERMISSIONS = { admin_group_member: { - descripition: 'Permission to admin group member' + description: 'Allows admin access to group members.' }, admin_merge_request: { description: 'Allows admin access to the merge requests.' @@ -33,8 +33,8 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass ALL_CUSTOMIZABLE_GROUP_PERMISSIONS = [ :read_dependency, :read_vulnerability, - :admin_group_member, - :admin_vulnerability + :admin_vulnerability, + :admin_group_member ].freeze CUSTOMIZABLE_PERMISSIONS_EXEMPT_FROM_CONSUMING_SEAT = [:read_code].freeze diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index d5ddd0c4f8918e..e730b70692c97b 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -196,7 +196,7 @@ module GroupPolicy end condition(:admin_group_member_custom_roles_allowed) do - ::Feature.enabled?(:admin_group_member_custom_roles, @subject.root_ancestor) + ::Feature.enabled?(:admin_group_member, @subject.root_ancestor) end desc "Custom role on group that enables read dependency" @@ -226,11 +226,13 @@ module GroupPolicy ).has_ability? end - desc "Custom role on group that enables admin members" - condition(:role_enables_admin_member) do - next unless @user.is_a?(User) - - @user.custom_permission_for?(@subject, :admin_group_member) + desc "Custom role on group that enables admin group members" + condition(:role_enables_admin_group_member) do + ::Auth::MemberRoleAbilityLoader.new( + user: @user, + resource: @subject, + ability: :admin_group_member + ).has_ability? end rule { owner & unique_project_download_limit_enabled }.policy do @@ -488,7 +490,7 @@ module GroupPolicy enable :admin_vulnerability end - rule { custom_roles_allowed & admin_group_member_custom_roles_allowed & role_enables_admin_member }.policy do + rule { custom_roles_allowed & admin_group_member_custom_roles_allowed & role_enables_admin_group_member }.policy do enable :admin_group_member enable :update_group_member enable :destroy_group_member diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index ce81a3054715c1..4b97098846759d 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -2951,9 +2951,9 @@ def create_member_role(member, abilities = member_role_abilities) context 'for a member role with admin_group_member true' do let(:member_role_abilities) { { admin_group_member: true } } - context 'with admin_group_member_custom_roles FF enabled' do + context 'with admin_group_member FF enabled' do before do - stub_feature_flags(admin_group_member_custom_roles: [parent_group]) + stub_feature_flags(admin_group_member: [parent_group]) end let(:allowed_abilities) { [:admin_group_member] } @@ -2961,9 +2961,9 @@ def create_member_role(member, abilities = member_role_abilities) it_behaves_like 'custom roles abilities' end - context 'with admin_group_member_custom_roles FF disabled' do + context 'with admin_group_member FF disabled' do before do - stub_feature_flags(admin_group_member_custom_roles: false) + stub_feature_flags(admin_group_member: false) stub_licensed_features(custom_roles: true) create_member_role(group_member_guest) diff --git a/lib/api/invitations.rb b/lib/api/invitations.rb index 5af85ec0845f12..34f9538b047c6d 100644 --- a/lib/api/invitations.rb +++ b/lib/api/invitations.rb @@ -34,7 +34,7 @@ class Invitations < ::API::Base source = find_source(source_type, params[:id]) - if ::Feature.enabled?(:admin_group_member_custom_roles, source) + if ::Feature.enabled?(:admin_group_member, source) authorize_admin_source_member!(source_type, source) else authorize_admin_source!(source_type, source) @@ -61,7 +61,7 @@ class Invitations < ::API::Base source = find_source(source_type, params[:id]) query = params[:query] - if ::Feature.enabled?(:admin_group_member_custom_roles, source) + if ::Feature.enabled?(:admin_group_member, source) authorize_admin_source_member!(source_type, source) else authorize_admin_source!(source_type, source) @@ -85,7 +85,7 @@ class Invitations < ::API::Base source = find_source(source_type, params.delete(:id)) invite_email = params[:email] - if ::Feature.enabled?(:admin_group_member_custom_roles, source) + if ::Feature.enabled?(:admin_group_member, source) authorize_admin_source_member!(source_type, source) else authorize_admin_source!(source_type, source) @@ -127,7 +127,7 @@ class Invitations < ::API::Base source = find_source(source_type, params[:id]) invite_email = params[:email] - if ::Feature.enabled?(:admin_group_member_custom_roles, source) + if ::Feature.enabled?(:admin_group_member, source) authorize_admin_source_member!(source_type, source) else authorize_admin_source!(source_type, source) diff --git a/lib/api/members.rb b/lib/api/members.rb index f7c242142526a7..bdbdea70da0779 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -118,7 +118,7 @@ class Members < ::API::Base post ":id/members", feature_category: feature_category do source = find_source(source_type, params[:id]) - if ::Feature.enabled?(:admin_group_member_custom_roles, source) + if ::Feature.enabled?(:admin_group_member, source) authorize_admin_source_member!(source_type, source) else authorize_admin_source!(source_type, source) @@ -148,7 +148,7 @@ class Members < ::API::Base source = find_source(source_type, params.delete(:id)) member = source_members(source).find_by!(user_id: params[:user_id]) - if ::Feature.enabled?(:admin_group_member_custom_roles, source) + if ::Feature.enabled?(:admin_group_member, source) authorize_update_source_member!(source_type, member) else authorize_admin_source!(source_type, source) diff --git a/spec/requests/api/invitations_spec.rb b/spec/requests/api/invitations_spec.rb index 4eecf421add820..dc02e830027882 100644 --- a/spec/requests/api/invitations_spec.rb +++ b/spec/requests/api/invitations_spec.rb @@ -379,8 +379,24 @@ def invite_member_by_email(source, source_type, email, created_by, access_level: end describe 'POST /projects/:id/invitations' do - it_behaves_like 'POST /:source_type/:id/invitations', 'project' do - let(:source) { project } + context 'with admin_group_member FF disabled' do + before do + stub_feature_flags(admin_group_member: false) + end + + it_behaves_like 'POST /:source_type/:id/invitations', 'project' do + let(:source) { project } + end + end + + context 'with admin_group_member FF enabled' do + before do + stub_feature_flags(admin_group_member: true) + end + + it_behaves_like 'POST /:source_type/:id/invitations', 'project' do + let(:source) { project } + end end it 'does not exceed expected queries count for emails', :request_store, :use_sql_query_cache do @@ -442,8 +458,24 @@ def invite_member_by_email(source, source_type, email, created_by, access_level: end describe 'POST /groups/:id/invitations' do - it_behaves_like 'POST /:source_type/:id/invitations', 'group' do - let(:source) { group } + context 'with admin_group_member FF disabled' do + before do + stub_feature_flags(admin_group_member: false) + end + + it_behaves_like 'POST /:source_type/:id/invitations', 'group' do + let(:source) { group } + end + end + + context 'with admin_group_member FF enabled' do + before do + stub_feature_flags(admin_group_member: true) + end + + it_behaves_like 'POST /:source_type/:id/invitations', 'group' do + let(:source) { group } + end end it 'does not exceed expected queries count for emails', :request_store, :use_sql_query_cache do @@ -555,14 +587,46 @@ def invite_member_by_email(source, source_type, email, created_by, access_level: end describe 'GET /projects/:id/invitations' do - it_behaves_like 'GET /:source_type/:id/invitations', 'project' do - let(:source) { project } + context 'with admin_group_member FF disabled' do + before do + stub_feature_flags(admin_group_member: false) + end + + it_behaves_like 'GET /:source_type/:id/invitations', 'project' do + let(:source) { project } + end + end + + context 'with admin_group_member FF enabled' do + before do + stub_feature_flags(admin_group_member: true) + end + + it_behaves_like 'GET /:source_type/:id/invitations', 'project' do + let(:source) { project } + end end end describe 'GET /groups/:id/invitations' do - it_behaves_like 'GET /:source_type/:id/invitations', 'group' do - let(:source) { group } + context 'with admin_group_member FF disabled' do + before do + stub_feature_flags(admin_group_member: false) + end + + it_behaves_like 'GET /:source_type/:id/invitations', 'group' do + let(:source) { group } + end + end + + context 'with admin_group_member FF enabled' do + before do + stub_feature_flags(admin_group_member: true) + end + + it_behaves_like 'GET /:source_type/:id/invitations', 'group' do + let(:source) { group } + end end end @@ -648,14 +712,46 @@ def invite_api(source, user, email) end describe 'DELETE /projects/:id/inviations/:email' do - it_behaves_like 'DELETE /:source_type/:id/invitations/:email', 'project' do - let(:source) { project } + context 'with admin_group_member FF disabled' do + before do + stub_feature_flags(admin_group_member: false) + end + + it_behaves_like 'DELETE /:source_type/:id/invitations/:email', 'project' do + let(:source) { project } + end + end + + context 'with admin_group_member FF enabled' do + before do + stub_feature_flags(admin_group_member: true) + end + + it_behaves_like 'DELETE /:source_type/:id/invitations/:email', 'project' do + let(:source) { project } + end end end describe 'DELETE /groups/:id/inviations/:email' do - it_behaves_like 'DELETE /:source_type/:id/invitations/:email', 'group' do - let(:source) { group } + context 'with admin_group_member FF disabled' do + before do + stub_feature_flags(admin_group_member: false) + end + + it_behaves_like 'DELETE /:source_type/:id/invitations/:email', 'group' do + let(:source) { group } + end + end + + context 'with admin_group_member FF enabled' do + before do + stub_feature_flags(admin_group_member: true) + end + + it_behaves_like 'DELETE /:source_type/:id/invitations/:email', 'group' do + let(:source) { group } + end end end @@ -764,14 +860,26 @@ def update_api(source, user, email) end describe 'PUT /projects/:id/invitations' do - it_behaves_like 'PUT /:source_type/:id/invitations/:email', 'project' do - let(:source) { project } + context 'with admin_group_member FF disabled' do + before do + stub_feature_flags(admin_group_member: false) + end + + it_behaves_like 'PUT /:source_type/:id/invitations/:email', 'project' do + let(:source) { project } + end end end describe 'PUT /groups/:id/invitations' do - it_behaves_like 'PUT /:source_type/:id/invitations/:email', 'group' do - let(:source) { group } + context 'with admin_group_member FF enabled' do + before do + stub_feature_flags(admin_group_member: true) + end + + it_behaves_like 'PUT /:source_type/:id/invitations/:email', 'group' do + let(:source) { group } + end end end end diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 07e3e642b85db6..8dab9d555cfad1 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -803,10 +803,6 @@ def request end describe 'POST /projects/:id/members' do - it_behaves_like 'POST /:source_type/:id/members', 'project' do - let(:source) { project } - end - context 'adding owner to project' do it_behaves_like 'a 403 response when user does not have rights to manage members of a specific access level' do let(:route) do @@ -830,16 +826,48 @@ def request end end - it_behaves_like 'POST /:source_type/:id/members', 'group' do - let(:source) { group } - end + context 'with admin_group_member FF disabled' do + before do + stub_feature_flags(admin_group_member: false) + end - it_behaves_like 'PUT /:source_type/:id/members/:user_id', 'project' do - let(:source) { project } + it_behaves_like 'POST /:source_type/:id/members', 'project' do + let(:source) { project } + end + + it_behaves_like 'POST /:source_type/:id/members', 'group' do + let(:source) { group } + end + + it_behaves_like 'PUT /:source_type/:id/members/:user_id', 'project' do + let(:source) { project } + end + + it_behaves_like 'PUT /:source_type/:id/members/:user_id', 'group' do + let(:source) { group } + end end - it_behaves_like 'PUT /:source_type/:id/members/:user_id', 'group' do - let(:source) { group } + context 'with admin_group_member FF enabled' do + before do + stub_feature_flags(admin_group_member: true) + end + + it_behaves_like 'POST /:source_type/:id/members', 'project' do + let(:source) { project } + end + + it_behaves_like 'POST /:source_type/:id/members', 'group' do + let(:source) { group } + end + + it_behaves_like 'PUT /:source_type/:id/members/:user_id', 'project' do + let(:source) { project } + end + + it_behaves_like 'PUT /:source_type/:id/members/:user_id', 'group' do + let(:source) { group } + end end it_behaves_like 'DELETE /:source_type/:id/members/:user_id', 'project' do -- GitLab