From 5db7d5cb833452736295d70e561ccb001405227a Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 20 Mar 2024 15:09:09 +0100 Subject: [PATCH 1/4] Add manage_merge_request_settings custom permission This adds the manage_merge_request_settings custom permission that allows to manage merge request settings on group and/or project level. --- .../json_schemas/member_role_permissions.json | 3 +++ doc/api/graphql/reference/index.md | 1 + doc/user/custom_roles/abilities.md | 6 ++++++ .../manage_merge_request_settings.yml | 17 +++++++++++++++++ ...ser_member_roles_in_groups_preloader_spec.rb | 3 ++- ...r_member_roles_in_projects_preloader_spec.rb | 3 ++- 6 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 ee/config/custom_abilities/manage_merge_request_settings.yml diff --git a/app/validators/json_schemas/member_role_permissions.json b/app/validators/json_schemas/member_role_permissions.json index a2d45d354fb10f..191cbdb8b39622 100644 --- a/app/validators/json_schemas/member_role_permissions.json +++ b/app/validators/json_schemas/member_role_permissions.json @@ -37,6 +37,9 @@ "manage_group_access_tokens": { "type": "boolean" }, + "manage_merge_request_settings": { + "type": "boolean" + }, "manage_project_access_tokens": { "type": "boolean" }, diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 0985270811fb1f..0b38cbc95d98f2 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -33863,6 +33863,7 @@ Member role permission. | `ARCHIVE_PROJECT` | Allows archiving of projects. | | `MANAGE_DEPLOY_TOKENS` | Manage deploy tokens at the group or project level. | | `MANAGE_GROUP_ACCESS_TOKENS` | Create, read, update, and delete group access tokens. When creating a token, users with this custom permission must select a role for that token that has the same or fewer permissions as the default role used as the base for the custom role. | +| `MANAGE_MERGE_REQUEST_SETTINGS` | Configure merge request settings at the group or project level. Group actions include managing merge checks and approval settings. Project actions include managing MR configurations, approval rules and settings, and branch targets. In order to enable Suggested reviewers, the "Manage project access tokens" custom permission needs to be enabled. | | `MANAGE_PROJECT_ACCESS_TOKENS` | Create, read, update, and delete project access tokens. When creating a token, users with this custom permission must select a role for that token that has the same or fewer permissions as the default role used as the base for the custom role. | | `MANAGE_SECURITY_POLICY_LINK` | Allows linking security policy projects. | | `READ_CODE` | Allows read-only access to the source code in the user interface. Does not allow users to edit or download files, clone or pull repositories, view source code in an IDE, or view merge requests for private projects. | diff --git a/doc/user/custom_roles/abilities.md b/doc/user/custom_roles/abilities.md index 7df0c69e0d2c1f..d13e1b07494cf3 100644 --- a/doc/user/custom_roles/abilities.md +++ b/doc/user/custom_roles/abilities.md @@ -23,6 +23,12 @@ Some permissions require having other permissions enabled first. For example, ad These requirements are documented in the `Required permission` column in the following table. +## Code review workflow + +| Name | Required permission | Description | Introduced in | Feature flag | Enabled in | +|:-----|:------------|:------------------|:---------|:--------------|:---------| +| [`manage_merge_request_settings`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/151586) | | Configure merge request settings at the group or project level. Group actions include managing merge checks and approval settings. Project actions include managing MR configurations, approval rules and settings, and branch targets. In order to enable Suggested reviewers, the "Manage project access tokens" custom permission needs to be enabled. | GitLab [17.0](https://gitlab.com/gitlab-org/gitlab/-/issues/443235) | | | + ## Compliance management | Name | Required permission | Description | Introduced in | Feature flag | Enabled in | diff --git a/ee/config/custom_abilities/manage_merge_request_settings.yml b/ee/config/custom_abilities/manage_merge_request_settings.yml new file mode 100644 index 00000000000000..54244405039465 --- /dev/null +++ b/ee/config/custom_abilities/manage_merge_request_settings.yml @@ -0,0 +1,17 @@ +--- +name: manage_merge_request_settings +description: + Configure merge request settings at the group or project level. Group + actions include managing merge checks and approval settings. Project actions + include managing MR configurations, approval rules and settings, + and branch targets. In order to enable Suggested reviewers, the "Manage project access + tokens" custom permission needs to be enabled. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/443235 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/151586 +feature_category: code_review_workflow +milestone: "17.0" +group_ability: true +project_ability: true +requirements: + - read_code +available_from_access_level: diff --git a/ee/spec/models/preloaders/user_member_roles_in_groups_preloader_spec.rb b/ee/spec/models/preloaders/user_member_roles_in_groups_preloader_spec.rb index 48c595855a882c..5b1c6118146d35 100644 --- a/ee/spec/models/preloaders/user_member_roles_in_groups_preloader_spec.rb +++ b/ee/spec/models/preloaders/user_member_roles_in_groups_preloader_spec.rb @@ -17,7 +17,8 @@ def ability_requirements(ability) ability_definition = MemberRole.all_customizable_permissions[ability] - ability_definition[:requirements]&.map(&:to_sym) || [] + requirements = ability_definition[:requirements]&.map(&:to_sym) || [] + requirements & MemberRole.all_customizable_group_permissions end def create_member_role(ability, member) diff --git a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb index 44ee51043d27fa..6d0610d7fc89df 100644 --- a/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb +++ b/ee/spec/models/preloaders/user_member_roles_in_projects_preloader_spec.rb @@ -15,7 +15,8 @@ def ability_requirements(ability) ability_definition = MemberRole.all_customizable_permissions[ability] - ability_definition[:requirements]&.map(&:to_sym) || [] + requirements = ability_definition[:requirements]&.map(&:to_sym) || [] + requirements & MemberRole.all_customizable_project_permissions end def create_member_role(ability, member) -- GitLab From b1f88bbd4a9f8a82045f6ec775917bee63d334a7 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 20 Mar 2024 15:10:17 +0100 Subject: [PATCH 2/4] Enable project level manage_merge_request_settings This enables the manage_merge_request_settings custom permission on project level. --- app/graphql/types/ci/ci_cd_setting_type.rb | 2 +- app/policies/project_policy.rb | 1 + .../approvals/project_settings/app.vue | 5 +- .../mount_project_settings.js | 1 + .../settings/merge_requests_controller.rb | 3 + ee/app/helpers/ee/projects_helper.rb | 5 + ee/app/policies/ee/project_policy.rb | 16 ++ .../external_status_checks/create_service.rb | 2 +- .../external_status_checks/destroy_service.rb | 2 +- .../external_status_checks/update_service.rb | 2 +- .../helpers/project_approval_rules_helpers.rb | 8 +- ee/lib/api/status_checks.rb | 2 +- .../sidebars/projects/menus/settings_menu.rb | 5 + .../approvals/project_settings/app_spec.js | 13 +- ee/spec/helpers/projects_helper_spec.rb | 1 + .../projects/menus/settings_menu_spec.rb | 16 ++ ee/spec/policies/project_policy_spec.rb | 29 +++ .../request_spec.rb | 246 ++++++++++++++++++ .../create_service_spec.rb | 2 +- .../policies/project_policy_shared_context.rb | 2 +- 20 files changed, 340 insertions(+), 23 deletions(-) create mode 100644 ee/spec/requests/custom_roles/manage_merge_request_settings/request_spec.rb diff --git a/app/graphql/types/ci/ci_cd_setting_type.rb b/app/graphql/types/ci/ci_cd_setting_type.rb index 0c2d1b788afd82..700d3c8136e6cf 100644 --- a/app/graphql/types/ci/ci_cd_setting_type.rb +++ b/app/graphql/types/ci/ci_cd_setting_type.rb @@ -5,7 +5,7 @@ module Ci class CiCdSettingType < BaseObject graphql_name 'ProjectCiCdSetting' - authorize :admin_project + authorize :manage_merge_request_settings field :inbound_job_token_scope_enabled, GraphQL::Types::Boolean, diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index badbd5761e39b5..ca897d5375ba42 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -606,6 +606,7 @@ class ProjectPolicy < BasePolicy enable :admin_cicd_variables enable :admin_push_rules enable :manage_deploy_tokens + enable :manage_merge_request_settings end rule { can?(:admin_build) }.enable :manage_trigger diff --git a/ee/app/assets/javascripts/approvals/project_settings/app.vue b/ee/app/assets/javascripts/approvals/project_settings/app.vue index cb72ff8153c4d1..84ae67436530d7 100644 --- a/ee/app/assets/javascripts/approvals/project_settings/app.vue +++ b/ee/app/assets/javascripts/approvals/project_settings/app.vue @@ -1,5 +1,4 @@ @@ -23,7 +22,7 @@ export default { - + diff --git a/ee/app/assets/javascripts/approvals/project_settings/mount_project_settings.js b/ee/app/assets/javascripts/approvals/project_settings/mount_project_settings.js index 817060e7dbe251..b1da1a06292d5e 100644 --- a/ee/app/assets/javascripts/approvals/project_settings/mount_project_settings.js +++ b/ee/app/assets/javascripts/approvals/project_settings/mount_project_settings.js @@ -42,6 +42,7 @@ export default function mountProjectSettingsApprovals(el) { coverageCheckHelpPagePath, fullPath, newPolicyPath, + canReadSecurityPolicies: parseBoolean(el.dataset.canReadSecurityPolicies), }, render(h) { return h(ProjectSettingsApp); diff --git a/ee/app/controllers/ee/projects/settings/merge_requests_controller.rb b/ee/app/controllers/ee/projects/settings/merge_requests_controller.rb index 1a0f79a0dc20ce..48ec82897a3533 100644 --- a/ee/app/controllers/ee/projects/settings/merge_requests_controller.rb +++ b/ee/app/controllers/ee/projects/settings/merge_requests_controller.rb @@ -8,6 +8,9 @@ module MergeRequestsController extend ::ActiveSupport::Concern prepended do + skip_before_action :authorize_admin_project! + before_action :authorize_manage_merge_request_settings! + before_action do if @project&.licensed_feature_available?(:security_orchestration_policies) push_licensed_feature(:security_orchestration_policies) diff --git a/ee/app/helpers/ee/projects_helper.rb b/ee/app/helpers/ee/projects_helper.rb index 0f3a77d11fb8be..32932ee45ec74d 100644 --- a/ee/app/helpers/ee/projects_helper.rb +++ b/ee/app/helpers/ee/projects_helper.rb @@ -62,6 +62,7 @@ def approvals_app_data(project = @project) can_edit: can_modify_approvers.to_s, can_modify_author_settings: can_modify_author_settings.to_s, can_modify_commiter_settings: can_modify_commiter_settings.to_s, + can_read_security_policies: can_read_security_policies.to_s, saml_provider_enabled: saml_provider_enabled_for_project?(project).to_s, project_path: expose_path(api_v4_projects_path(id: project.id)), approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)), @@ -104,6 +105,10 @@ def can_modify_commiter_settings(project = @project) can?(current_user, :modify_merge_request_committer_setting, project) end + def can_read_security_policies(project = @project) + can?(current_user, :read_security_orchestration_policies, project) + end + def permanent_delete_message(project) message = _('This action deletes %{codeOpen}%{project_path_with_namespace}%{codeClose} and everything this project contains. %{strongOpen}There is no going back.%{strongClose}') ERB::Util.html_escape(message) % remove_message_data(project) diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 995fb29198c5d8..32a214bbdf41dd 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -803,6 +803,22 @@ module ProjectPolicy enable :manage_resource_access_tokens end + rule { custom_role_enables_manage_merge_request_settings }.policy do + enable :manage_merge_request_settings + enable :edit_approval_rule + enable :modify_approvers_rules + enable :modify_merge_request_author_setting + enable :modify_merge_request_committer_setting + end + + rule { can?(:manage_merge_request_settings) & target_branch_rules_available }.policy do + enable :admin_target_branch_rule + end + + rule { can?(:manage_merge_request_settings) & group_merge_request_approval_settings_enabled }.policy do + enable :admin_merge_request_approval_settings + end + rule { security_policy_bot }.policy do enable :create_pipeline enable :create_bot_pipeline diff --git a/ee/app/services/external_status_checks/create_service.rb b/ee/app/services/external_status_checks/create_service.rb index ef55067347d38f..dbe2a443bb2f5b 100644 --- a/ee/app/services/external_status_checks/create_service.rb +++ b/ee/app/services/external_status_checks/create_service.rb @@ -19,7 +19,7 @@ def execute(skip_authorization: false) end def can_create_status_check? - current_user.can?(:admin_project, container) + current_user.can?(:manage_merge_request_settings, container) end def access_denied_error diff --git a/ee/app/services/external_status_checks/destroy_service.rb b/ee/app/services/external_status_checks/destroy_service.rb index fad2bc098d4a7e..aca24e84d215d5 100644 --- a/ee/app/services/external_status_checks/destroy_service.rb +++ b/ee/app/services/external_status_checks/destroy_service.rb @@ -19,7 +19,7 @@ def execute(external_status_check, skip_authorization: false) private def can_destroy_external_status_check? - current_user.can?(:admin_project, container) + current_user.can?(:manage_merge_request_settings, container) end def unauthorized_error_response diff --git a/ee/app/services/external_status_checks/update_service.rb b/ee/app/services/external_status_checks/update_service.rb index db0cde6ab86173..2d871c538871df 100644 --- a/ee/app/services/external_status_checks/update_service.rb +++ b/ee/app/services/external_status_checks/update_service.rb @@ -23,7 +23,7 @@ def execute(skip_authorization: false) private def can_update_external_status_check? - current_user.can?(:admin_project, container) + current_user.can?(:manage_merge_request_settings, container) end def resource_params diff --git a/ee/lib/api/helpers/project_approval_rules_helpers.rb b/ee/lib/api/helpers/project_approval_rules_helpers.rb index 3842e148967d7b..dfb1da7b41280d 100644 --- a/ee/lib/api/helpers/project_approval_rules_helpers.rb +++ b/ee/lib/api/helpers/project_approval_rules_helpers.rb @@ -68,13 +68,13 @@ def check_feature_availability end def authorize_read_project_approval_rule! - return if can?(current_user, :admin_project, user_project) + return if can?(current_user, :manage_merge_request_settings, user_project) authorize! :create_merge_request_in, user_project end def create_project_approval_rule(present_with:) - authorize! :admin_project, user_project + authorize! :manage_merge_request_settings, user_project authorize! :modify_approvers_rules, user_project result = ::ApprovalRules::CreateService.new(user_project, current_user, declared_params(include_missing: false)).execute @@ -87,7 +87,7 @@ def create_project_approval_rule(present_with:) end def update_project_approval_rule(present_with:) - authorize! :admin_project, user_project + authorize! :manage_merge_request_settings, user_project authorize! :modify_approvers_rules, user_project params = declared_params(include_missing: false) @@ -103,7 +103,7 @@ def update_project_approval_rule(present_with:) end def destroy_project_approval_rule - authorize! :admin_project, user_project + authorize! :manage_merge_request_settings, user_project authorize! :modify_approvers_rules, user_project approval_rule = user_project.approval_rules.not_from_scan_result_policy.find(params[:approval_rule_id]) diff --git a/ee/lib/api/status_checks.rb b/ee/lib/api/status_checks.rb index 58bf9be72f8ea9..d8137c2a137c83 100644 --- a/ee/lib/api/status_checks.rb +++ b/ee/lib/api/status_checks.rb @@ -54,7 +54,7 @@ def check_feature_enabled! use :pagination end get do - unauthorized! unless current_user.can?(:admin_project, user_project) + unauthorized! unless current_user.can?(:manage_merge_request_settings, user_project) present paginate(user_project.external_status_checks), with: ::API::Entities::ExternalStatusCheck end diff --git a/ee/lib/ee/sidebars/projects/menus/settings_menu.rb b/ee/lib/ee/sidebars/projects/menus/settings_menu.rb index 215bb866a6f08b..edc80b08b45656 100644 --- a/ee/lib/ee/sidebars/projects/menus/settings_menu.rb +++ b/ee/lib/ee/sidebars/projects/menus/settings_menu.rb @@ -45,6 +45,7 @@ def custom_roles_menu_items items << general_menu_item if custom_roles_general_menu_item? items << access_tokens_menu_item if custom_roles_access_token_menu_item? items << repository_menu_item if custom_roles_repository_menu_item? + items << merge_requests_menu_item if custom_roles_merge_requests_menu_item? items << ci_cd_menu_item if custom_roles_ci_cd_menu_item? items @@ -63,6 +64,10 @@ def custom_roles_repository_menu_item? can?(context.current_user, :manage_deploy_tokens, context.project) end + def custom_roles_merge_requests_menu_item? + can?(context.current_user, :manage_merge_request_settings, context.project) + end + def custom_roles_ci_cd_menu_item? can?(context.current_user, :admin_cicd_variables, context.project) end diff --git a/ee/spec/frontend/approvals/project_settings/app_spec.js b/ee/spec/frontend/approvals/project_settings/app_spec.js index 41ffee24e1e724..1d11bf7ca25596 100644 --- a/ee/spec/frontend/approvals/project_settings/app_spec.js +++ b/ee/spec/frontend/approvals/project_settings/app_spec.js @@ -11,14 +11,9 @@ describe('Approvals ProjectSettings App', () => { const findScanResultPolicies = () => wrapper.findComponent(ScanResultPolicies); const findProjectApprovalSettings = () => wrapper.findComponent(ProjectApprovalSettings); - const factory = (glFeatures = {}) => { + const factory = (provide = { canReadSecurityPolicies: true }) => { wrapper = shallowMount(App, { - provide: { - glFeatures: { - securityOrchestrationPolicies: true, - ...glFeatures, - }, - }, + provide, }); }; @@ -33,9 +28,9 @@ describe('Approvals ProjectSettings App', () => { expect(findProjectApprovalSettings().exists()).toBe(true); }); - describe('without licensed feature securityOrchestrationPolicies', () => { + describe('without ability to read security policies', () => { beforeEach(() => { - factory({ securityOrchestrationPolicies: false }); + factory({ canReadSecurityPolicies: false }); }); it('renders all but the scan result policies component', () => { diff --git a/ee/spec/helpers/projects_helper_spec.rb b/ee/spec/helpers/projects_helper_spec.rb index 6c7fe085a489a3..be6f6ee4501dcb 100644 --- a/ee/spec/helpers/projects_helper_spec.rb +++ b/ee/spec/helpers/projects_helper_spec.rb @@ -613,6 +613,7 @@ can_edit: 'true', can_modify_author_settings: 'true', can_modify_commiter_settings: 'true', + can_read_security_policies: 'true', saml_provider_enabled: 'true', approvals_path: expose_path(api_v4_projects_merge_request_approval_setting_path(id: project.id)), project_path: expose_path(api_v4_projects_path(id: project.id)), diff --git a/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb b/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb index 37f27647c8fe9a..631f4bf7fbae2d 100644 --- a/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb +++ b/ee/spec/lib/ee/sidebars/projects/menus/settings_menu_spec.rb @@ -100,6 +100,22 @@ end end + describe 'Merge requests' do + let(:item_id) { :merge_requests } + + describe 'when the user is not an admin but has `manage_merge_request_settings` custom ability' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_project, project).and_return(false) + allow(Ability).to receive(:allowed?).with(user, :manage_merge_request_settings, project).and_return(true) + end + + it 'includes Merge Requests menu item' do + expect(subject.title).to eql('Merge requests') + end + end + end + describe 'CI/CD' do let(:item_id) { :ci_cd } diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 64154fb9fa1e59..c2f1a43fd3a571 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -2996,6 +2996,35 @@ def create_member_role(member, abilities = member_role_abilities) it_behaves_like 'custom roles abilities' end + + context 'for a custom role with the `manage_merge_request_settings` ability' do + let(:member_role_abilities) { { manage_merge_request_settings: true } } + let(:allowed_abilities) do + [ + :manage_merge_request_settings, + :edit_approval_rule, + :modify_approvers_rules, + :modify_merge_request_author_setting, + :modify_merge_request_committer_setting + ] + end + + it_behaves_like 'custom roles abilities' + + context 'when `target_branch_rules` feature is available' do + let(:licensed_features) { { target_branch_rules: true } } + let(:allowed_abilities) { [:admin_target_branch_rule] } + + it_behaves_like 'custom roles abilities' + end + + context 'when `merge_request_approvers` feature is available' do + let(:licensed_features) { { merge_request_approvers: true } } + let(:allowed_abilities) { [:admin_merge_request_approval_settings] } + + it_behaves_like 'custom roles abilities' + end + end end describe 'permissions for suggested reviewers bot', :saas do diff --git a/ee/spec/requests/custom_roles/manage_merge_request_settings/request_spec.rb b/ee/spec/requests/custom_roles/manage_merge_request_settings/request_spec.rb new file mode 100644 index 00000000000000..afb3126a479e50 --- /dev/null +++ b/ee/spec/requests/custom_roles/manage_merge_request_settings/request_spec.rb @@ -0,0 +1,246 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'User with manage_merge_request_settings custom role', feature_category: :code_review_workflow do + include ApiHelpers + include GraphqlHelpers + + let_it_be(:user) { create(:user) } + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, :repository, namespace: group) } + let_it_be_with_reload(:role) { create(:member_role, :guest, namespace: group, manage_merge_request_settings: true) } + let_it_be(:membership) { create(:group_member, :guest, user: user, source: group, member_role: role) } + + before do + stub_licensed_features(custom_roles: true) + + sign_in(user) + end + + describe 'project merge request settings' do + describe Projects::Settings::MergeRequestsController do + describe '#show' do + it 'user has access via a custom role' do + get project_settings_merge_requests_path(project) + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to have_text(_('Merge requests')) + end + end + + describe '#update' do + it 'user has access via a custom role' do + patch project_settings_merge_requests_path(project, project: { allow_merge_on_skipped_pipeline: true }) + + expect(response).to have_gitlab_http_status(:redirect) + expect(response).to redirect_to(project_settings_merge_requests_path(project)) + + expect(project.reload.allow_merge_on_skipped_pipeline).to eq(true) + end + end + end + + describe Projects::TargetBranchRulesController do + before do + stub_licensed_features(custom_roles: true, target_branch_rules: true) + end + + describe '#create' do + it 'user has access via a custom role' do + params = { projects_target_branch_rule: attributes_for(:target_branch_rule) } + + expect do + post project_target_branch_rules_path(project), params: params + end.to change { Projects::TargetBranchRule.count }.by(1) + + expect(response).to have_gitlab_http_status(:redirect) + expect(response).to redirect_to(project_settings_merge_requests_path(project, anchor: 'target-branch-rules')) + expect(flash[:notice]).to eq('Branch target created.') + end + end + + describe '#destroy' do + let_it_be(:rule) { create(:target_branch_rule, project: project) } + + it 'user has access via a custom role' do + expect do + delete project_target_branch_rule_path(project, rule) + end.to change { Projects::TargetBranchRule.count }.by(-1) + + expect(response).to have_gitlab_http_status(:redirect) + expect(response).to redirect_to(project_settings_merge_requests_path(project, anchor: 'target-branch-rules')) + expect(flash[:notice]).to eq('Branch target deleted.') + end + end + end + + describe API::ProjectApprovalRules do + let_it_be(:url) { "/projects/#{project.id}/approval_rules" } + let_it_be(:approval_rule) { create(:approval_project_rule, project: project, approvals_required: 2) } + + describe 'GET' do + it 'user has access via a custom role' do + get api(url, user) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.first['approvals_required']).to eq(approval_rule.approvals_required) + end + end + + describe 'POST' do + it 'user has access via a custom role' do + expect do + post api(url, user), params: { name: 'new rule', approvals_required: 10 } + end.to change { ApprovalProjectRule.count }.by(1) + + expect(response).to have_gitlab_http_status(:created) + end + end + + describe 'PUT' do + it 'user has access via a custom role' do + put api("#{url}/#{approval_rule.id}", user), params: { approvals_required: 5 } + + expect(response).to have_gitlab_http_status(:ok) + expect(approval_rule.reload.approvals_required).to eq(5) + end + end + + describe 'DELETE' do + it 'user has access via a custom role' do + expect do + delete api("#{url}/#{approval_rule.id}", user) + end.to change { ApprovalProjectRule.count }.by(-1) + + expect(response).to have_gitlab_http_status(:no_content) + end + end + end + + describe API::StatusChecks do + let_it_be(:url) { "/projects/#{project.id}/external_status_checks" } + let_it_be(:external_status_check) { create(:external_status_check, project: project) } + + before do + stub_licensed_features(custom_roles: true, external_status_checks: true) + end + + describe 'GET' do + it 'user has access via a custom role' do + get api(url, user) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(1) + end + end + + describe 'POST' do + it 'user has access via a custom role' do + expect do + post api(url, user), params: attributes_for(:external_status_check) + end.to change { MergeRequests::ExternalStatusCheck.count }.by(1) + + expect(response).to have_gitlab_http_status(:created) + end + end + + describe 'PUT' do + it 'user has access via a custom role' do + put api("#{url}/#{external_status_check.id}", user), params: { name: 'new name' } + + expect(response).to have_gitlab_http_status(:ok) + expect(external_status_check.reload.name).to eq('new name') + end + end + + describe 'DELETE' do + it 'user has access via a custom role' do + expect do + delete api("#{url}/#{external_status_check.id}", user) + end.to change { MergeRequests::ExternalStatusCheck.count }.by(-1) + + expect(response).to have_gitlab_http_status(:no_content) + end + end + end + + describe API::MergeRequestApprovalSettings do + before do + stub_licensed_features(custom_roles: true, merge_request_approvers: true) + end + + let_it_be(:url) { "/projects/#{project.id}/merge_request_approval_setting" } + + describe 'GET' do + it 'user has access via a custom role' do + get api(url, user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee') + end + end + + describe 'PUT' do + it 'user has access via a custom role' do + put api(url, user), params: { allow_author_approval: true, allow_committer_approval: true } + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee') + + project.reload + + expect(project.merge_requests_author_approval).to eq(true) + expect(project.merge_requests_disable_committers_approval).to eq(false) + end + end + end + + describe API::Branches do + let_it_be(:url) { "/projects/#{project.id}/repository/branches" } + + describe 'GET' do + it 'user has access via a custom role' do + get api(url, user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/branches') + end + end + end + + describe API::ProtectedBranches do + let_it_be(:url) { "/projects/#{project.id}/protected_branches" } + + describe 'GET' do + it 'user has access via a custom role' do + get api(url, user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('protected_branches') + end + end + end + + describe 'Querying CI/CD Settings' do + let(:query) do + %( + query { + project(fullPath: "#{project.full_path}") { + ciCdSettings { + mergePipelinesEnabled + mergeTrainsEnabled + } + } + } + ) + end + + it 'returns the CI/CD settings' do + result = GitlabSchema.execute(query, context: { current_user: user }).as_json + settings = result.dig('data', 'project', 'ciCdSettings') + expect(settings).to eq('mergePipelinesEnabled' => false, 'mergeTrainsEnabled' => false) + end + end + end +end diff --git a/ee/spec/services/external_status_checks/create_service_spec.rb b/ee/spec/services/external_status_checks/create_service_spec.rb index 30a137f9f7773a..885a2e704b820c 100644 --- a/ee/spec/services/external_status_checks/create_service_spec.rb +++ b/ee/spec/services/external_status_checks/create_service_spec.rb @@ -19,7 +19,7 @@ before do allow(Ability) - .to receive(:allowed?).with(user, :admin_project, project) + .to receive(:allowed?).with(user, :manage_merge_request_settings, project) .and_return(action_allowed) stub_licensed_features(audit_events: true) diff --git a/spec/support/shared_contexts/policies/project_policy_shared_context.rb b/spec/support/shared_contexts/policies/project_policy_shared_context.rb index 8b4d94989fcd8d..2d60beb57df876 100644 --- a/spec/support/shared_contexts/policies/project_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/project_policy_shared_context.rb @@ -70,7 +70,7 @@ admin_project admin_project_member admin_snippet admin_terraform_state admin_wiki create_deploy_token destroy_deploy_token push_to_delete_protected_branch read_deploy_token update_snippet - destroy_upload admin_member_access_request rename_project + destroy_upload admin_member_access_request rename_project manage_merge_request_settings ] end -- GitLab From 756eba7d515676acfb0410ecdeec98af9fca0204 Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Wed, 27 Mar 2024 14:15:29 +0100 Subject: [PATCH 3/4] Enable group level manage_merge_request_settings This enables the manage_merge_request_settings custom permission on group level. --- app/policies/group_policy.rb | 1 + app/views/groups/edit.html.haml | 2 + .../settings/merge_requests_controller.rb | 2 +- ee/app/policies/ee/group_policy.rb | 10 +++- .../ee/sidebars/groups/menus/settings_menu.rb | 3 +- .../groups/menus/settings_menu_spec.rb | 22 +++++++ ee/spec/policies/group_policy_spec.rb | 18 ++++++ .../request_spec.rb | 59 +++++++++++++++++++ .../policies/group_policy_shared_context.rb | 1 + 9 files changed, 115 insertions(+), 3 deletions(-) diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index c56fd8ca8e3313..ad7c6ea542d66c 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -291,6 +291,7 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy enable :edit_billing enable :remove_group + enable :manage_merge_request_settings end rule { can?(:read_nested_project_resources) }.policy do diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml index bff7be82747d09..3894255afca4b1 100644 --- a/app/views/groups/edit.html.haml +++ b/app/views/groups/edit.html.haml @@ -29,9 +29,11 @@ .settings-content = render 'groups/settings/permissions' +- if can?(current_user, :manage_merge_request_settings, @group) = render_if_exists 'groups/settings/merge_requests/merge_requests', expanded: expanded, group: @group = render_if_exists 'groups/settings/merge_requests/merge_request_approval_settings', expanded: expanded, group: @group, user: current_user +- if can?(current_user, :admin_group, @group) %section.settings.no-animate#js-badge-settings{ class: ('expanded' if expanded) } .settings-header %h4.settings-title.js-settings-toggle.js-settings-toggle-trigger-only{ role: 'button' } diff --git a/ee/app/controllers/groups/settings/merge_requests_controller.rb b/ee/app/controllers/groups/settings/merge_requests_controller.rb index d186cf3f833ad7..4781bc8087806d 100644 --- a/ee/app/controllers/groups/settings/merge_requests_controller.rb +++ b/ee/app/controllers/groups/settings/merge_requests_controller.rb @@ -5,7 +5,7 @@ module Settings class MergeRequestsController < Groups::ApplicationController layout 'group_settings' - before_action :authorize_admin_group! + before_action :authorize_manage_merge_request_settings! feature_category :code_review_workflow diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb index 097a3ad2791fa9..fb2e6680e62b3c 100644 --- a/ee/app/policies/ee/group_policy.rb +++ b/ee/app/policies/ee/group_policy.rb @@ -550,7 +550,7 @@ module GroupPolicy enable :admin_push_rules end - rule { can?(:admin_group) | can?(:admin_compliance_framework) | can?(:manage_deploy_tokens) }.policy do + rule { can?(:admin_group) | can?(:admin_compliance_framework) | can?(:manage_deploy_tokens) | can?(:manage_merge_request_settings) }.policy do enable :view_edit_page end @@ -567,6 +567,14 @@ module GroupPolicy enable :read_security_resource end + rule { custom_role_enables_manage_merge_request_settings }.policy do + enable :manage_merge_request_settings + end + + rule { can?(:manage_merge_request_settings) & group_merge_request_approval_settings_enabled }.policy do + enable :admin_merge_request_approval_settings + end + rule { can?(:admin_vulnerability) }.policy do enable :read_vulnerability end diff --git a/ee/lib/ee/sidebars/groups/menus/settings_menu.rb b/ee/lib/ee/sidebars/groups/menus/settings_menu.rb index 04ae18bd753232..e2cfae3297e9f6 100644 --- a/ee/lib/ee/sidebars/groups/menus/settings_menu.rb +++ b/ee/lib/ee/sidebars/groups/menus/settings_menu.rb @@ -24,7 +24,8 @@ def configure_menu_items add_item(billing_menu_item) add_item(reporting_menu_item) else - add_menu_item_for_abilities(general_menu_item, [:remove_group, :admin_compliance_framework]) + add_menu_item_for_abilities(general_menu_item, [:remove_group, :admin_compliance_framework, + :manage_merge_request_settings]) add_menu_item_for_abilities(access_tokens_menu_item, :read_resource_access_tokens) add_menu_item_for_abilities(repository_menu_item, [:admin_push_rules, :manage_deploy_tokens]) add_menu_item_for_abilities(ci_cd_menu_item, :admin_cicd_variables) diff --git a/ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb b/ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb index 38ebd24dd60cc5..3a754b72c3710d 100644 --- a/ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb +++ b/ee/spec/lib/ee/sidebars/groups/menus/settings_menu_spec.rb @@ -425,5 +425,27 @@ end end end + + context 'when the user is not an owner but has `manage_merge_request_settings` custom ability', feature_category: :code_review_workflow do + let_it_be(:user) { create(:user) } + + subject { menu.renderable_items.find { |e| e.item_id == item_id } } + + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?).with(user, :admin_group, group).and_return(false) + allow(Ability).to receive(:allowed?).with(user, :manage_merge_request_settings, group).and_return(true) + end + + describe 'General menu item' do + let(:item_id) { :general } + + it { is_expected.to be_present } + + it 'does not show any other menu items' do + expect(menu.renderable_items.length).to equal(1) + end + end + end end end diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb index 0b36df043a6a02..5c73046f3f5186 100644 --- a/ee/spec/policies/group_policy_spec.rb +++ b/ee/spec/policies/group_policy_spec.rb @@ -3573,6 +3573,24 @@ def create_member_role(member, abilities = member_role_abilities) it_behaves_like 'custom roles abilities' end + + context 'for a custom role with the `manage_merge_request_settings` ability' do + let(:member_role_abilities) { { manage_merge_request_settings: true } } + let(:allowed_abilities) { [:manage_merge_request_settings, :view_edit_page] } + + it_behaves_like 'custom roles abilities' + + context 'when the group is a top level group and the `merge_request_approvers` feature is available' do + before do + create_member_role(parent_group_member_guest) + stub_licensed_features(custom_roles: true, merge_request_approvers: true) + end + + subject { described_class.new(current_user, parent_group) } + + it { is_expected.to be_allowed(:admin_merge_request_approval_settings) } + end + end end context 'for :read_limit_alert' do diff --git a/ee/spec/requests/custom_roles/manage_merge_request_settings/request_spec.rb b/ee/spec/requests/custom_roles/manage_merge_request_settings/request_spec.rb index afb3126a479e50..58f40a3a6bcdfb 100644 --- a/ee/spec/requests/custom_roles/manage_merge_request_settings/request_spec.rb +++ b/ee/spec/requests/custom_roles/manage_merge_request_settings/request_spec.rb @@ -18,6 +18,65 @@ sign_in(user) end + describe 'group merge request settings' do + before do + stub_licensed_features(custom_roles: true, group_level_merge_checks_setting: true) + end + + describe GroupsController do + describe '#edit' do + it 'user has access via a custom role' do + get edit_group_path(group) + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to have_text(_('Merge requests')) + end + end + end + + describe Groups::Settings::MergeRequestsController do + describe '#update' do + it 'user has access via a custom role' do + patch group_settings_merge_requests_path(group, namespace_setting: { allow_merge_on_skipped_pipeline: true }) + + expect(response).to have_gitlab_http_status(:redirect) + expect(response).to redirect_to(edit_group_path(group, anchor: 'js-merge-requests-settings')) + + expect(group.reload.namespace_settings.allow_merge_on_skipped_pipeline).to eq(true) + end + end + end + + describe API::MergeRequestApprovalSettings do + before do + stub_licensed_features(custom_roles: true, merge_request_approvers: true) + end + + let_it_be(:url) { "/groups/#{group.id}/merge_request_approval_setting" } + + describe 'GET' do + it 'user has access via a custom role' do + get api(url, user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee') + end + end + + describe 'PUT' do + it 'user has access via a custom role' do + put api(url, user), params: { allow_author_approval: true, allow_committer_approval: true } + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('public_api/v4/group_merge_request_approval_settings', dir: 'ee') + + expect(group.group_merge_request_approval_setting.allow_author_approval).to eq(true) + expect(group.group_merge_request_approval_setting.allow_committer_approval).to eq(true) + end + end + end + end + describe 'project merge request settings' do describe Projects::Settings::MergeRequestsController do describe '#show' do diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb index 4afee6c86991d8..e49ea2dc2a6859 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -89,6 +89,7 @@ update_git_access_protocol remove_group view_edit_page + manage_merge_request_settings ] end -- GitLab From 4492219106ccd7942ca2090b0c39d182bde2170c Mon Sep 17 00:00:00 2001 From: Alex Buijs Date: Fri, 10 May 2024 13:52:58 +0200 Subject: [PATCH 4/4] Implemented domain expert review feedback --- app/graphql/types/ci/ci_cd_setting_type.rb | 12 ++++++++---- ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb | 3 ++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/app/graphql/types/ci/ci_cd_setting_type.rb b/app/graphql/types/ci/ci_cd_setting_type.rb index 700d3c8136e6cf..f6f2fe0ef9d0d4 100644 --- a/app/graphql/types/ci/ci_cd_setting_type.rb +++ b/app/graphql/types/ci/ci_cd_setting_type.rb @@ -12,18 +12,21 @@ class CiCdSettingType < BaseObject null: true, description: 'Indicates CI/CD job tokens generated in other projects ' \ 'have restricted access to this project.', - method: :inbound_job_token_scope_enabled? + method: :inbound_job_token_scope_enabled?, + authorize: :admin_project field :job_token_scope_enabled, GraphQL::Types::Boolean, null: true, description: 'Indicates CI/CD job tokens generated in this project ' \ 'have restricted access to other projects.', - method: :job_token_scope_enabled? + method: :job_token_scope_enabled?, + authorize: :admin_project field :keep_latest_artifact, GraphQL::Types::Boolean, null: true, description: 'Whether to keep the latest builds artifacts.', - method: :keep_latest_artifacts_available? + method: :keep_latest_artifacts_available?, + authorize: :admin_project field :merge_pipelines_enabled, GraphQL::Types::Boolean, null: true, @@ -32,7 +35,8 @@ class CiCdSettingType < BaseObject field :project, Types::ProjectType, null: true, - description: 'Project the CI/CD settings belong to.' + description: 'Project the CI/CD settings belong to.', + authorize: :admin_project end end end diff --git a/ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb b/ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb index 84bb36adea29d8..251f4a2feee37b 100644 --- a/ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb +++ b/ee/app/graphql/ee/types/ci/ci_cd_setting_type.rb @@ -11,7 +11,8 @@ module CiCdSettingType GraphQL::Types::Boolean, null: false, description: 'Whether merge immediately is allowed for merge trains.', - method: :merge_trains_skip_train_allowed? + method: :merge_trains_skip_train_allowed?, + authorize: :admin_project field :merge_trains_enabled, GraphQL::Types::Boolean, null: true, -- GitLab