Merge branch '239349-fix-instance-mr-approval-settings-inheritance' into 'master'

Fix permission to modify project MR rules on compliance-labeled projects

See merge request gitlab-org/gitlab!40229

doc/user/admin_area/merge_requests_approvals.md

@@ -31,9 +31,8 @@ Merge request approval rules that can be set at an instance level are:
 - **Prevent approval of merge requests by merge request committers**. Prevents project
   maintainers from allowing users to approve merge requests if they have submitted
   any commits to the source branch.
-- **Prevent users from modifying merge request approvers list**. Prevents project
-  maintainers from allowing users to modify the approvers list in project settings
-  or in individual merge requests.
+- **Can override approvers and approvals required per merge request**. Allows project
+  maintainers to modify the approvers list in individual merge requests.
 
 ## Scope rules to compliance-labeled projects
 
@@ -52,4 +51,4 @@ Maintainer role and above can modify these.
 
 | Instance-level | Project-level |
 | -------------- | ------------- |
-| ![Scope MR approval settings to compliance frameworks](img/scope_mr_approval_settings_v13_1.png) | ![MR approval settings on compliance projects](img/mr_approval_settings_compliance_project_v13_1.png) |
+| ![Scope MR approval settings to compliance frameworks](img/scope_mr_approval_settings_v13_5.png) | ![MR approval settings on compliance projects](img/mr_approval_settings_compliance_project_v13_5.png) |

ee/app/controllers/ee/projects_controller.rb

@@ -120,7 +120,7 @@ module EE
         attrs << :merge_requests_disable_committers_approval
       end
 
-      if can?(current_user, :modify_approvers_rules, project)
+      if can?(current_user, :modify_overriding_approvers_per_merge_request_setting, project)
         attrs << :disable_overriding_approvers_per_merge_request
       end
 

ee/app/policies/ee/project_policy.rb

@@ -227,6 +227,7 @@ module EE
         enable :admin_path_locks
         enable :update_approvers
         enable :modify_approvers_rules
+        enable :modify_overriding_approvers_per_merge_request_setting
         enable :modify_auto_fix_setting
         enable :modify_merge_request_author_setting
         enable :modify_merge_request_committer_setting
@@ -306,7 +307,7 @@ module EE
       end
 
       rule { regulated_merge_request_approval_settings }.policy do
-        prevent :modify_approvers_rules
+        prevent :modify_overriding_approvers_per_merge_request_setting
         prevent :modify_merge_request_author_setting
         prevent :modify_merge_request_committer_setting
       end

ee/app/views/admin/push_rules/_merge_request_approvals_fields.html.haml

@@ -9,6 +9,6 @@
       = f.label :prevent_merge_requests_committers_approval, class: 'form-check-label' do
         = _('Prevent approval of merge requests by merge request committers')
     .form-check
-      = f.check_box :disable_overriding_approvers_per_merge_request , class: 'form-check-input'
+      = f.check_box :disable_overriding_approvers_per_merge_request , { class: 'form-check-input' }, false, true
       = f.label :disable_overriding_approvers_per_merge_request , class: 'form-check-label' do
-        = _('Prevent users from modifying merge request approvers list')
+        = _('Can override approvers and approvals required per merge request')

ee/app/views/projects/_merge_request_approvals_settings_form.html.haml

-- can_override_approvers = project.can_override_approvers?
+- can_modify_overriding_approvers_per_merge_request_settings = can?(current_user, :modify_overriding_approvers_per_merge_request_setting, @project)
 - can_modify_merge_request_author_settings = can?(current_user, :modify_merge_request_author_setting, @project)
 - can_modify_merge_request_committer_settings = can?(current_user, :modify_merge_request_committer_setting, @project)
 
@@ -22,7 +22,7 @@
 
 .form-group
   .form-check
-    = form.check_box(:disable_overriding_approvers_per_merge_request, { checked: can_override_approvers, class: 'form-check-input', disabled: !can_modify_approvers }, false, true)
+    = form.check_box :disable_overriding_approvers_per_merge_request, { class: 'form-check-input', disabled: !can_modify_overriding_approvers_per_merge_request_settings }, false, true
     = form.label :disable_overriding_approvers_per_merge_request, class: 'form-check-label' do
       %span= _('Can override approvers and approvals required per merge request')
       = link_to sprite_icon('question-o'), help_page_path('user/project/merge_requests/merge_request_approvals', anchor: 'prevent-overriding-default-approvals'), target: '_blank'
@@ -35,7 +35,7 @@
 
 .form-group.self-approval
   .form-check
-    = form.check_box :merge_requests_author_approval, { class: 'form-check-input', checked: !project.merge_requests_author_approval?, disabled: !can_modify_merge_request_author_settings }, false, true
+    = form.check_box :merge_requests_author_approval, { class: 'form-check-input', disabled: !can_modify_merge_request_author_settings }, false, true
     = form.label :merge_requests_author_approval, class: 'form-check-label' do
       %span= _('Prevent approval of merge requests by merge request author')
       = link_to sprite_icon('question-o'), help_page_path('user/project/merge_requests/merge_request_approvals',

ee/changelogs/unreleased/239349-fix-instance-mr-approval-settings-inheritance.yml

+---
+title: Fix permission to modify project MR rules on compliance-labeled projects
+merge_request: 40229
+author:
+type: fixed

ee/lib/api/project_approvals.rb

@@ -13,7 +13,7 @@ module API
       def filter_params(params)
         params
           .then { |params| filter_forbidden_param(params, :modify_merge_request_committer_setting, :merge_requests_disable_committers_approval) }
-          .then { |params| filter_forbidden_param(params, :modify_approvers_rules, :disable_overriding_approvers_per_merge_request) }
+          .then { |params| filter_forbidden_param(params, :modify_overriding_approvers_per_merge_request_setting, :disable_overriding_approvers_per_merge_request) }
           .then { |params| filter_forbidden_param(params, :modify_merge_request_author_setting, :merge_requests_author_approval) }
       end
     end

ee/spec/policies/project_policy_spec.rb

@@ -1195,7 +1195,7 @@ RSpec.describe ProjectPolicy do
     end
   end
 
-  shared_examples 'merge request rules' do
+  shared_examples 'regulated merge request approval settings' do
     let(:project) { private_project }
 
     using RSpec::Parameterized::TableSyntax
@@ -1258,19 +1258,45 @@ RSpec.describe ProjectPolicy do
   end
 
   describe ':modify_approvers_rules' do
-    it_behaves_like 'merge request rules' do
-      let(:policy) { :modify_approvers_rules }
+    let(:policy) { :modify_approvers_rules }
+
+    using RSpec::Parameterized::TableSyntax
+
+    where(:role, :admin_mode, :allowed) do
+      :guest      | nil    | false
+      :reporter   | nil    | false
+      :developer  | nil    | false
+      :maintainer | nil    | true
+      :owner      | nil    | true
+      :admin      | false  | false
+      :admin      | true   | true
+    end
+
+    with_them do
+      let(:current_user) { public_send(role) }
+
+      before do
+        enable_admin_mode!(current_user) if admin_mode
+      end
+
+      it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
+    end
+  end
+
+  describe ':modify_overriding_approvers_per_merge_request_setting' do
+    it_behaves_like 'regulated merge request approval settings' do
+      let(:policy) { :modify_overriding_approvers_per_merge_request_setting }
     end
   end
 
   describe ':modify_merge_request_author_setting' do
-    it_behaves_like 'merge request rules' do
+    it_behaves_like 'regulated merge request approval settings' do
       let(:policy) { :modify_merge_request_author_setting }
     end
   end
 
   describe ':modify_merge_request_committer_setting' do
-    it_behaves_like 'merge request rules' do
+    it_behaves_like 'regulated merge request approval settings' do
       let(:policy) { :modify_merge_request_committer_setting }
     end
   end

ee/spec/requests/api/project_approvals_spec.rb

@@ -144,7 +144,7 @@ RSpec.describe API::ProjectApprovals do
       context 'updates merge requests settings' do
         it_behaves_like 'updates merge requests settings when possible' do
           let(:current_user) { admin }
-          let(:permission) { :modify_approvers_rules }
+          let(:permission) { :modify_overriding_approvers_per_merge_request_setting }
           let(:setting) { :disable_overriding_approvers_per_merge_request }
         end
 

locale/gitlab.pot

@@ -19843,9 +19843,6 @@ msgstr ""
 msgid "Prevent users from changing their profile name"
 msgstr ""
 
-msgid "Prevent users from modifying merge request approvers list"
-msgstr ""
-
 msgid "Prevent users from performing write operations on GitLab while performing maintenance."
 msgstr ""