diff --git a/db/migrate/20230802205051_add_admin_merge_request_to_member_roles.rb b/db/migrate/20230802205051_add_admin_merge_request_to_member_roles.rb new file mode 100644 index 0000000000000000000000000000000000000000..52560621c9a85dad03a7443458229855d68a4b3f --- /dev/null +++ b/db/migrate/20230802205051_add_admin_merge_request_to_member_roles.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddAdminMergeRequestToMemberRoles < Gitlab::Database::Migration[2.1] + enable_lock_retries! + + def up + return if column_exists?(:member_roles, :admin_merge_request) + + add_column :member_roles, :admin_merge_request, :boolean, default: false, null: false + end + + def down + return unless column_exists?(:member_roles, :admin_merge_request) + + remove_column :member_roles, :admin_merge_request + end +end diff --git a/db/schema_migrations/20230802205051 b/db/schema_migrations/20230802205051 new file mode 100644 index 0000000000000000000000000000000000000000..3f09a569f68f4d6118eee5082dd5e9184897cb3b --- /dev/null +++ b/db/schema_migrations/20230802205051 @@ -0,0 +1 @@ +f682f6c1dd19fa508282bfcd30e61197652bf14b4f0717ef2313a920f0eb3516 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 1c2e81950c4489686ba7f4da6073537be363c5fb..740d4fdde4e6d56095bbd4b258ef054f8157954d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18143,6 +18143,7 @@ CREATE TABLE member_roles ( read_dependency boolean DEFAULT false NOT NULL, name text DEFAULT 'Custom'::text NOT NULL, description text, + admin_merge_request 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 9d3e51efabd32cbcff4fb12b1856d9ddf2b248d1..311f43bcebf265415419ad350c9f97e5ee5e2d1c 100644 --- a/doc/api/member_roles.md +++ b/doc/api/member_roles.md @@ -12,6 +12,11 @@ info: To determine the technical writer assigned to the Stage/Group associated w > - [Admin vulnerability added](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121534) in GitLab 16.1. > - [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. + +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 GitLab.com, this feature is not available. ## List all member roles of a group @@ -34,6 +39,7 @@ If successful, returns [`200`](rest/index.md#status-codes) and the following res | `[].description` | string | The description of the member role. | | `[].group_id` | integer | The ID of the group that the member role belongs to. | | `[].base_access_level` | integer | Base access level for member role. Valid values are 10 (Guest), 20 (Reporter), 30 (Developer), 40 (Maintainer), or 50 (Owner).| +| `[].admin_merge_request` | boolean | Permission to admin project merge requests and enables the ability to `download_code`. | | `[].admin_vulnerability` | boolean | Permission to admin project vulnerabilities. | | `[].read_code` | boolean | Permission to read project code. | | `[].read_dependency` | boolean | Permission to read project dependencies. | @@ -55,6 +61,7 @@ Example response: "description: "Custom guest that can read code", "group_id": 84, "base_access_level": 10, + "admin_merge_request": false, "admin_vulnerability": false, "read_code": true, "read_dependency": false, @@ -66,6 +73,7 @@ Example response: "description: "Custom guest that read and admin security entities", "group_id": 84, "base_access_level": 10, + "admin_merge_request": false, "admin_vulnerability": true, "read_code": false, "read_dependency": true, @@ -92,6 +100,7 @@ To add a member role to a group, the group must be at root-level (have no parent | `name` | string | yes | The name of the member role. | | `description` | string | no | The description of the member role. | | `base_access_level` | integer | yes | Base access level for configured role. Valid values are 10 (Guest), 20 (Reporter), 30 (Developer), 40 (Maintainer), or 50 (Owner).| +| `admin_merge_request` | boolean | no | Permission to admin project merge requests. | | `admin_vulnerability` | boolean | no | Permission to admin project vulnerabilities. | | `read_code` | boolean | no | Permission to read project code. | | `read_dependency` | boolean | no | Permission to read project dependencies. | @@ -106,6 +115,7 @@ If successful, returns [`201`](rest/index.md#status-codes) and the following att | `description` | string | The description of the member role. | | `group_id` | integer | The ID of the group that the member role belongs to. | | `base_access_level` | integer | Base access level for member role. | +| `admin_merge_request` | boolean | Permission to admin project merge requests. | | `admin_vulnerability` | boolean | Permission to admin project vulnerabilities. | | `read_code` | boolean | Permission to read project code. | | `read_dependency` | boolean | Permission to read project dependencies. | @@ -126,6 +136,7 @@ Example response: "description": null, "group_id": 84, "base_access_level": 10, + "admin_merge_requests": false, "admin_vulnerability": false, "read_code": true, "read_dependency": false, diff --git a/doc/user/permissions.md b/doc/user/permissions.md index d19f98b98ed6d0c88b45079122215a6e76df37a1..ba186bf883c8ccbb157c24d4db45b5ffaf4f6e2a 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -488,6 +488,8 @@ The following custom roles are available: - The Guest+1 role, which allows users with the Guest role to view code. - 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. You can discuss individual custom role and permission requests in [issue 391760](https://gitlab.com/gitlab-org/gitlab/-/issues/391760). @@ -518,7 +520,9 @@ You can see the required minimal access levels and abilities requirements in the | Ability | Minimal access level | Required ability | | -- | -- | -- | | `read_code` | Guest | - | +| `read_dependency` | Guest | - | | `read_vulnerability` | Guest | - | +| `admin_merge_request` | Guest | - | | `admin_vulnerability` | Guest | `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 e615766cae4917a6ceb1f8f6fd8c24a599aec7e0..7047bcb5a14b05bdbae38f1ae775d7ab6938aff8 100644 --- a/ee/app/models/members/member_role.rb +++ b/ee/app/models/members/member_role.rb @@ -3,6 +3,10 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass MAX_COUNT_PER_GROUP_HIERARCHY = 10 ALL_CUSTOMIZABLE_PERMISSIONS = { + admin_merge_request: { + descripition: 'Permission to admin merge requests', + minimal_level: Gitlab::Access::GUEST + }, admin_vulnerability: { description: 'Permission to admin vulnerability', minimal_level: Gitlab::Access::GUEST, @@ -25,6 +29,7 @@ class MemberRole < ApplicationRecord # rubocop:disable Gitlab/NamespacedClass :read_code, :read_dependency, :read_vulnerability, + :admin_merge_request, :admin_vulnerability ].freeze ALL_CUSTOMIZABLE_GROUP_PERMISSIONS = [ diff --git a/ee/app/policies/ee/merge_request_policy.rb b/ee/app/policies/ee/merge_request_policy.rb index d96ce648cc02b35edb6ac834cde319ffb219fad0..f28068d61c658c3f915fb18c7e9eddee316b8e03 100644 --- a/ee/app/policies/ee/merge_request_policy.rb +++ b/ee/app/policies/ee/merge_request_policy.rb @@ -58,6 +58,18 @@ module MergeRequestPolicy ::Gitlab::Llm::StageCheck.available?(subject.project.root_ancestor, :summarize_submitted_review) end + condition(:role_enables_admin_merge_request) do + next unless @user.is_a?(User) + + ::Feature.enabled?(:admin_merge_request, subject&.project) && + @user.custom_permission_for?(subject&.project, :admin_merge_request) + end + + with_scope :subject + condition(:custom_roles_allowed) do + subject&.project&.custom_roles_enabled? + end + def read_only? @subject.target_project&.namespace&.read_only? end @@ -99,6 +111,10 @@ def group_access?(protected_branch) rule do ai_features_enabled & summarize_submitted_review_enabled & can?(:read_merge_request) end.enable :summarize_submitted_review + + rule { custom_roles_allowed & role_enables_admin_merge_request }.policy do + enable :approve_merge_request + end end private diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 11d2cfc65458e5630721d963ad9da88b31f6f531..461cf1e40bcaf0933e0b0163a20f28554ad195a8 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -226,6 +226,14 @@ module ProjectPolicy @user.custom_permission_for?(@subject, :read_vulnerability) end + desc "Custom role on project that enables admin merge request" + condition(:role_enables_admin_merge_request) do + next unless @user.is_a?(User) + + ::Feature.enabled?(:admin_merge_request, subject) && + @user.custom_permission_for?(project, :admin_merge_request) + end + desc "Custom role on project that enables admin vulnerability" condition(:role_enables_admin_vulnerability) do next unless @user.is_a?(User) @@ -626,6 +634,11 @@ module ProjectPolicy enable :create_vulnerability_export end + rule { custom_roles_allowed & role_enables_admin_merge_request }.policy do + enable :admin_merge_request + enable :download_code # required to negate https://gitlab.com/gitlab-org/gitlab/-/blob/3061d30d9b3d6d4c4dd5abe68bc1e4a8a93c7966/app/policies/project_policy.rb#L603-607 + end + rule { custom_roles_allowed & role_enables_admin_vulnerability }.policy do enable :admin_vulnerability end diff --git a/ee/config/feature_flags/development/admin_merge_request.yml b/ee/config/feature_flags/development/admin_merge_request.yml new file mode 100644 index 0000000000000000000000000000000000000000..15b7d0ac3ae98062767b9b5ca44e3e4df9490764 --- /dev/null +++ b/ee/config/feature_flags/development/admin_merge_request.yml @@ -0,0 +1,8 @@ +--- +name: admin_merge_request +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/128302 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/421478 +milestone: '16.4' +type: development +group: group::threat insights +default_enabled: false diff --git a/ee/spec/features/merge_request/user_sees_approve_via_custom_role_spec.rb b/ee/spec/features/merge_request/user_sees_approve_via_custom_role_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..18db6f9e090d785378f8ed31346841ea87001b8d --- /dev/null +++ b/ee/spec/features/merge_request/user_sees_approve_via_custom_role_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Merge request > User approves via custom role', :js, feature_category: :code_review_workflow do + let_it_be(:current_user) { create(:user) } + let_it_be(:project) { create(:project, :public, :repository, :in_group) } + let_it_be(:merge_request) { create(:merge_request, source_project: project) } + + before do + stub_licensed_features(custom_roles: true) + sign_in(current_user) + end + + context 'when the user has `admin_merge_request` enabled at the project level' do + let_it_be(:admin_merge_request_role) do + create(:member_role, :guest, namespace: project.group, admin_merge_request: true) + end + + let_it_be(:project_member) do + create(:project_member, :guest, member_role: admin_merge_request_role, user: current_user, project: project) + end + + it 'allows approving and revoking approval' do + visit project_merge_request_path(project, merge_request) + expect(page).to have_button('Approve', exact: true) + + click_approval_button('Approve') + expect(page).to have_content('Approved by you') + + click_approval_button('Revoke approval') + expect(page).to have_content('Approval is optional') + end + end + + context 'when the user does not have the `admin_merge_request` permission enabled' do + let_it_be(:non_admin_merge_request_role) do + create(:member_role, :guest, namespace: project.group, admin_merge_request: false) + end + + let_it_be(:project_member) do + create(:project_member, :guest, member_role: non_admin_merge_request_role, user: current_user, project: project) + end + + it 'prevents approving' do + visit project_merge_request_path(project, merge_request) + + expect(page).not_to have_button('Approve', exact: true) + end + end + + def click_approval_button(action) + page.within('.mr-state-widget') do + click_button(action) + end + + wait_for_requests + end +end diff --git a/ee/spec/policies/merge_request_policy_spec.rb b/ee/spec/policies/merge_request_policy_spec.rb index 8f4a7051ceb02e83126512f083f9d4a0acddcd9d..e2a799776af528c3528cb6be380e4f8f5b750be8 100644 --- a/ee/spec/policies/merge_request_policy_spec.rb +++ b/ee/spec/policies/merge_request_policy_spec.rb @@ -548,4 +548,55 @@ def policy_for(user) it { is_expected.to be_disallowed(:summarize_submitted_review) } end end + + describe "Custom roles `admin_merge_request` ability" do + let_it_be(:project) { create(:project, :public, :in_group) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + + subject { described_class.new(guest, merge_request) } + + context 'when the `custom_roles` feature is enabled' do + before do + stub_licensed_features(custom_roles: true) + end + + context 'when the user is a member of a custom role with `admin_merge_request` enabled' do + let_it_be(:custom_role) { create(:member_role, :guest, namespace: project.group, admin_merge_request: true) } + let_it_be(:project_member) { create(:project_member, :guest, member_role: custom_role, project: project, user: guest) } + + it 'enables the `approve_merge_request` ability' do + expect(subject).to be_allowed(:approve_merge_request) + end + + context 'when the `admin_merge_request` feature flag is disabled' do + before do + stub_feature_flags(admin_merge_request: false) + end + + it 'disables the `approve_merge_request` ability' do + expect(subject).to be_disallowed(:approve_merge_request) + end + end + end + + context 'when the user is a member of a custom role with `admin_merge_request` disabled' do + let_it_be(:custom_role) { create(:member_role, :guest, namespace: project.group, admin_merge_request: false) } + let_it_be(:project_member) { create(:project_member, :guest, member_role: custom_role, project: project, user: guest) } + + it 'disables the `approve_merge_request` ability' do + expect(subject).to be_disallowed(:approve_merge_request) + end + end + end + + context 'when the `custom_roles` feature is disabled' do + before do + stub_licensed_features(custom_roles: false) + end + + it 'disables the `approve_merge_request` ability' do + expect(subject).to be_disallowed(:approve_merge_request) + end + end + end end diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index a100266a86cc6eec05d97dd2281b2bed43cd3d08..bbbd80a8d3088a36347b36d2033c61ac33e972ef 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2574,6 +2574,24 @@ def create_member_role(member, abilities = member_role_abilities) it_behaves_like 'custom roles abilities' end + + context 'for a member role with admin_merge_request true' do + let(:member_role_abilities) { { admin_merge_request: true } } + let(:allowed_abilities) { [:admin_merge_request] } + + it_behaves_like 'custom roles abilities' + + context 'with `admin_merge_request` feature disabled' do + before do + stub_feature_flags(admin_merge_request: false) + stub_licensed_features(custom_roles: true) + create_member_role(group_member_guest, admin_merge_request: true) + end + + it { is_expected.to be_disallowed(:admin_merge_request) } + it { is_expected.to be_disallowed(:download_code) } + end + end end describe 'permissions for suggested reviewers bot', :saas do diff --git a/ee/spec/requests/api/member_roles_spec.rb b/ee/spec/requests/api/member_roles_spec.rb index 6d968e4c1530601cade1a1c93f82d17ae999b5c0..e7290307659d96ae34e9cf88bcc4f8f2f0a851dd 100644 --- a/ee/spec/requests/api/member_roles_spec.rb +++ b/ee/spec/requests/api/member_roles_spec.rb @@ -33,6 +33,7 @@ :member_role, namespace: group_with_member_roles, base_access_level: ::Gitlab::Access::REPORTER, + admin_merge_request: true, read_code: true, read_dependency: true, read_vulnerability: false @@ -101,6 +102,7 @@ "read_code" => false, "read_dependency" => false, "read_vulnerability" => true, + "admin_merge_request" => false, "admin_vulnerability" => false, "group_id" => group_id }, @@ -112,6 +114,7 @@ "read_code" => true, "read_dependency" => true, "read_vulnerability" => false, + "admin_merge_request" => true, "admin_vulnerability" => false, "group_id" => group_id } diff --git a/ee/spec/requests/custom_roles/admin_merge_request/request_spec.rb b/ee/spec/requests/custom_roles/admin_merge_request/request_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..51ce2566ce0f93e10c3f79c971ddb97f0f5c7419 --- /dev/null +++ b/ee/spec/requests/custom_roles/admin_merge_request/request_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe "User with admin_merge_request custom role", feature_category: :code_review_workflow do + let_it_be(:current_user) { create(:user) } + let_it_be(:project) { create(:project, :public, :repository, :in_group) } + let_it_be(:merge_request) { create(:merge_request, target_project: project, source_project: project) } + + before do + stub_licensed_features(custom_roles: true) + + sign_in(current_user) + end + + describe Projects::MergeRequestsController do + let_it_be(:role) { create(:member_role, :guest, namespace: project.group, admin_merge_request: true) } + let_it_be(:member) { create(:project_member, :guest, member_role: role, user: current_user, project: project) } + + describe "#show" do + it "user has access via a custom role" do + get namespace_project_merge_request_path( + namespace_id: project.namespace.to_param, + project_id: project, + id: merge_request.iid + ) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to render_template(:show) + end + end + end +end