Unlink instance settings with project policy

Permission to modify project-level MR approval settings is also limited
to only to administrator when licensed.

doc/user/admin_area/merge_requests_approvals.md

@@ -19,6 +19,22 @@ To enable merge request approval rules for an instance:
 
 GitLab administrators can later override these settings in a project’s settings.
 
+## Merge request controls **(PREMIUM)**
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/207250) in GitLab 13.0.
+
+Merge request approval settings, by default, are inherited by all projects in an instance.
+
+However, organizations with regulated projects may also have unregulated projects
+that should not inherit these same controls.
+
+Project-level merge request approval rules can now be edited by administrators.
+Project owners and maintainers can still view project-level merge request approval rules.
+
+In upcoming releases, we plan to provide a more holistic experience to scope instance-level merge request settings.
+For more information, review our plans to provide custom [approval settings for compliance-
+labeled projects](https://gitlab.com/gitlab-org/gitlab/-/issues/213601).
+
 ## Available rules
 
 Merge request approval rules that can be set at an instance level are:

ee/app/controllers/ee/admin/application_settings_controller.rb

@@ -5,6 +5,8 @@ module EE
     module ApplicationSettingsController
       extend ::Gitlab::Utils::Override
 
+      include ::Admin::PushRulesHelper
+
       EE_VALID_SETTING_PANELS = %w(templates).freeze
 
       EE_VALID_SETTING_PANELS.each do |action|
@@ -50,7 +52,7 @@ module EE
           attrs << :updating_name_disabled_for_users
         end
 
-        if License.feature_available?(:admin_merge_request_approvers_rules)
+        if show_merge_request_approvals_settings?
           attrs += EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes
         end
 

ee/app/helpers/admin/push_rules_helper.rb

+# frozen_string_literal: true
+
+module Admin
+  module PushRulesHelper
+    def show_merge_request_approvals_settings?
+      Feature.enabled?(:admin_merge_request_approvals_settings) &&
+        License.feature_available?(:admin_merge_request_approvers_rules)
+    end
+  end
+end

ee/app/models/ee/project.rb

@@ -687,31 +687,6 @@ module EE
       packages.where(package_type: package_type).exists?
     end
 
-    def disable_overriding_approvers_per_merge_request
-      return super unless License.feature_available?(:admin_merge_request_approvers_rules)
-
-      ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? ||
-        super
-    end
-    alias_method :disable_overriding_approvers_per_merge_request?, :disable_overriding_approvers_per_merge_request
-
-    def merge_requests_author_approval
-      return super unless License.feature_available?(:admin_merge_request_approvers_rules)
-
-      return false if ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
-
-      super
-    end
-    alias_method :merge_requests_author_approval?, :merge_requests_author_approval
-
-    def merge_requests_disable_committers_approval
-      return super unless License.feature_available?(:admin_merge_request_approvers_rules)
-
-      ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? ||
-        super
-    end
-    alias_method :merge_requests_disable_committers_approval?, :merge_requests_disable_committers_approval
-
     def license_compliance
       strong_memoize(:license_compliance) { SCA::LicenseCompliance.new(self) }
     end

ee/app/policies/ee/project_policy.rb

@@ -55,24 +55,8 @@ module EE
       end
 
       with_scope :global
-      condition(:owner_cannot_modify_approvers_rules) do
-        License.feature_available?(:admin_merge_request_approvers_rules) &&
-          ::Gitlab::CurrentSettings.current_application_settings
-            .disable_overriding_approvers_per_merge_request
-      end
-
-      with_scope :global
-      condition(:owner_cannot_modify_merge_request_author_setting) do
-        License.feature_available?(:admin_merge_request_approvers_rules) &&
-          ::Gitlab::CurrentSettings.current_application_settings
-            .prevent_merge_requests_author_approval
-      end
-
-      with_scope :global
-      condition(:owner_cannot_modify_merge_request_committer_setting) do
-        License.feature_available?(:admin_merge_request_approvers_rules) &&
-          ::Gitlab::CurrentSettings.current_application_settings
-            .prevent_merge_requests_committers_approval
+      condition(:admin_merge_request_approvers_rules_available) do
+        License.feature_available?(:admin_merge_request_approvers_rules)
       end
 
       with_scope :global
@@ -362,24 +346,15 @@ module EE
         prevent :read_project
       end
 
-      rule { owner_cannot_modify_approvers_rules & ~admin }.policy do
+      rule { admin_merge_request_approvers_rules_available & ~admin }.policy do
         prevent :modify_approvers_rules
-      end
-
-      rule { owner_cannot_modify_merge_request_author_setting & ~admin }.policy do
+        prevent :modify_approvers_list
         prevent :modify_merge_request_author_setting
-      end
-
-      rule { owner_cannot_modify_merge_request_committer_setting & ~admin }.policy do
         prevent :modify_merge_request_committer_setting
       end
 
       rule { can?(:read_cluster) & cluster_health_available }.enable :read_cluster_health
 
-      rule { owner_cannot_modify_approvers_rules & ~admin }.policy do
-        prevent :modify_approvers_list
-      end
-
       rule { web_ide_terminal_available & can?(:create_pipeline) & can?(:maintainer_access) }.enable :create_web_ide_terminal
 
       rule { build_service_proxy_enabled }.enable :build_service_proxy_enabled

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

-- return unless License.feature_available?(:admin_merge_request_approvers_rules)
+- return unless show_merge_request_approvals_settings?
 %section.settings.merge-request-approval-settings.no-animate{ class: ('expanded' if expanded_by_default?) }
   .settings-header
     %h4

ee/changelogs/unreleased/207250-only-admins-modify-project-mr-approvals.yml

+---
+title: Allow only admins to modify MR approval settings at the project-level
+merge_request: 30358
+author:
+type: changed

ee/lib/ee/api/entities/application_setting.rb

@@ -6,12 +6,14 @@ module EE
       module ApplicationSetting
         extend ActiveSupport::Concern
 
+        include ::Admin::PushRulesHelper
+
         prepended do
           expose(*EE::ApplicationSettingsHelper.repository_mirror_attributes, if: ->(_instance, _options) do
             ::License.feature_available?(:repository_mirrors)
           end)
           expose(*EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes, if: ->(_instance, _options) do
-            ::License.feature_available?(:admin_merge_request_approvers_rules)
+            show_merge_request_approvals_settings?
           end)
           expose :email_additional_text, if: ->(_instance, _opts) { ::License.feature_available?(:email_additional_text) }
           expose :file_template_project_id, if: ->(_instance, _opts) { ::License.feature_available?(:custom_file_templates) }

ee/lib/ee/api/settings.rb

@@ -9,6 +9,8 @@ module EE
         helpers do
           extend ::Gitlab::Utils::Override
 
+          include ::Admin::PushRulesHelper
+
           override :filter_attributes_using_license
           def filter_attributes_using_license(attrs)
             unless ::License.feature_available?(:repository_mirrors)
@@ -35,7 +37,7 @@ module EE
               attrs = attrs.except(:updating_name_disabled_for_users)
             end
 
-            unless License.feature_available?(:admin_merge_request_approvers_rules)
+            unless show_merge_request_approvals_settings?
               attrs = attrs.except(*EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes)
             end
 

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -157,9 +157,29 @@ describe Admin::ApplicationSettingsController do
       end
       let(:feature) { :admin_merge_request_approvers_rules }
 
+      context 'when feature flag is enabled' do
+        before do
+          stub_feature_flags(admin_merge_request_approvals_settings: true)
+        end
+
         it_behaves_like 'settings for licensed features'
       end
 
+      context 'when feature flag is disabled' do
+        before do
+          stub_licensed_features(feature => true)
+          stub_feature_flags(admin_merge_request_approvals_settings: false)
+        end
+
+        it 'does not update settings' do
+          attribute_names = settings.keys.map(&:to_s)
+
+          expect { put :update, params: { application_setting: settings } }
+            .not_to change { ApplicationSetting.current.reload.attributes.slice(*attribute_names) }
+        end
+      end
+    end
+
     context 'required instance ci template' do
       let(:settings) { { required_instance_ci_template: 'Auto-DevOps' } }
       let(:feature) { :required_ci_templates }

ee/spec/controllers/projects_controller_spec.rb

@@ -324,21 +324,17 @@ describe ProjectsController do
       shared_examples 'merge request approvers rules' do
         using RSpec::Parameterized::TableSyntax
 
-        where(:license_value, :setting_value, :param_value, :final_value) do
-          false | false | false | false
-          false | true  | false | false
-          false | false | true  | true
-          false | true  | true  | true
-          true  | false | false | false
-          true  | true  | false | nil
-          true  | false | true  | true
-          true  | true  | true  | nil
+        where(:can_modify, :param_value, :final_value) do
+          true  | true  | true
+          true  | false | false
+          false | true  | nil
+          false | false | nil
         end
 
         with_them do
           before do
-            stub_licensed_features(admin_merge_request_approvers_rules: license_value)
-            stub_application_setting(app_setting => setting_value)
+            allow(controller).to receive(:can?).and_call_original
+            allow(controller).to receive(:can?).with(user, rule_name, project).and_return(can_modify)
           end
 
           it 'updates project if needed' do
@@ -357,21 +353,21 @@ describe ProjectsController do
 
       describe ':disable_overriding_approvers_per_merge_request' do
         it_behaves_like 'merge request approvers rules' do
-          let(:app_setting) { :disable_overriding_approvers_per_merge_request }
+          let(:rule_name) { :modify_approvers_rules }
           let(:setting) { :disable_overriding_approvers_per_merge_request }
         end
       end
 
       describe ':merge_requests_author_approval' do
         it_behaves_like 'merge request approvers rules' do
-          let(:app_setting) { :prevent_merge_requests_author_approval }
+          let(:rule_name) { :modify_merge_request_author_setting }
           let(:setting) { :merge_requests_author_approval }
         end
       end
 
       describe ':merge_requests_disable_committers_approval' do
         it_behaves_like 'merge request approvers rules' do
-          let(:app_setting) { :prevent_merge_requests_committers_approval }
+          let(:rule_name) { :modify_merge_request_committer_setting }
           let(:setting) { :merge_requests_disable_committers_approval }
         end
       end

ee/spec/helpers/admin/push_rules_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Admin::PushRulesHelper do
+  describe '#show_merge_request_approvals_settings?' do
+    using RSpec::Parameterized::TableSyntax
+
+    subject { helper.show_merge_request_approvals_settings? }
+
+    where(:feature_flag, :licensed, :result) do
+      true  | true  | true
+      true  | false | false
+      false | true  | false
+      false | false | false
+    end
+
+    with_them do
+      before do
+        stub_feature_flags(admin_merge_request_approvals_settings: feature_flag)
+        stub_licensed_features(admin_merge_request_approvers_rules: licensed)
+      end
+
+      it { is_expected.to eq(result) }
+    end
+  end
+end

ee/spec/models/approval_state_spec.rb

@@ -63,28 +63,6 @@ describe ApprovalState do
       it 'excludes authors' do
         expect(results).not_to include(merge_request.author)
       end
-
-      context 'when self approval is enabled on instance level' do
-        before do
-          stub_application_setting(prevent_merge_requests_author_approval: false)
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-        end
-
-        it 'excludes author' do
-          expect(results).not_to include(merge_request.author)
-        end
-      end
-
-      context 'when self approval is disabled on instance level' do
-        before do
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-          stub_application_setting(prevent_merge_requests_author_approval: true)
-        end
-
-        it 'excludes authors' do
-          expect(results).not_to include(merge_request.author)
-        end
-      end
     end
 
     context 'when self approval is enabled on project level' do
@@ -93,28 +71,6 @@ describe ApprovalState do
       it 'includes author' do
         expect(results).to include(merge_request.author)
       end
-
-      context 'when self approval is enabled on instance level' do
-        before do
-          stub_application_setting(prevent_merge_requests_author_approval: false)
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-        end
-
-        it 'includes author' do
-          expect(results).to include(merge_request.author)
-        end
-      end
-
-      context 'when self approval is disabled on instance level' do
-        before do
-          stub_application_setting(prevent_merge_requests_author_approval: true)
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-        end
-
-        it 'excludes authors' do
-          expect(results).not_to include(merge_request.author)
-        end
-      end
     end
 
     context 'when committers approval is enabled on project level' do
@@ -124,28 +80,6 @@ describe ApprovalState do
       it 'includes committers' do
         expect(results).to include(*committers)
       end
-
-      context 'when committers approval is enabled on instance level' do
-        before do
-          stub_application_setting(prevent_merge_requests_committers_approval: false)
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-        end
-
-        it 'includes committers' do
-          expect(results).to include(*committers)
-        end
-      end
-
-      context 'when committers approval is disabled on instance level' do
-        before do
-          stub_application_setting(prevent_merge_requests_committers_approval: true)
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-        end
-
-        it 'excludes committers' do
-          expect(results).not_to include(*committers)
-        end
-      end
     end
 
     context 'when committers approval is disabled on project level' do
@@ -155,28 +89,6 @@ describe ApprovalState do
       it 'excludes committers' do
         expect(results).not_to include(*committers)
       end
-
-      context 'when committers approval is enabled on instance level' do
-        before do
-          stub_application_setting(prevent_merge_requests_committers_approval: false)
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-        end
-
-        it 'excludes committers' do
-          expect(results).not_to include(*committers)
-        end
-      end
-
-      context 'when committers approval is disabled on instance level' do
-        before do
-          stub_application_setting(prevent_merge_requests_committers_approval: true)
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-        end
-
-        it 'excludes committers' do
-          expect(results).not_to include(*committers)
-        end
-      end
     end
   end
 

ee/spec/models/project_spec.rb

@@ -514,87 +514,6 @@ describe Project do
     end
   end
 
-  context 'merge requests related settings' do
-    shared_examples 'setting modified by application setting' do
-      using RSpec::Parameterized::TableSyntax
-
-      where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
-        true     | true      | true   | true
-        false    | true      | true   | true
-        true     | false     | true   | true
-        false    | false     | true   | false
-        true     | true      | false  | true
-        false    | true      | false  | true
-        true     | false     | false  | false
-        false    | false     | false  | false
-      end
-
-      with_them do
-        let(:project) { create(:project) }
-
-        before do
-          stub_licensed_features(feature => feature_enabled)
-          stub_application_setting(application_setting => app_setting)
-          project.update(setting => project_setting)
-        end
-
-        it 'shows proper setting' do
-          expect(project.send(setting)).to eq(final_setting)
-          expect(project.send("#{setting}?")).to eq(final_setting)
-        end
-      end
-    end
-
-    describe '#disable_overriding_approvers_per_merge_request' do
-      it_behaves_like 'setting modified by application setting' do
-        let(:feature) { :admin_merge_request_approvers_rules }
-        let(:setting) { :disable_overriding_approvers_per_merge_request }
-        let(:application_setting) { :disable_overriding_approvers_per_merge_request }
-      end
-    end
-
-    describe '#merge_requests_disable_committers_approval' do
-      it_behaves_like 'setting modified by application setting' do
-        let(:feature) { :admin_merge_request_approvers_rules }
-        let(:setting) { :merge_requests_disable_committers_approval }
-        let(:application_setting) { :prevent_merge_requests_committers_approval }
-      end
-    end
-
-    describe '#merge_requests_author_approval' do
-      using RSpec::Parameterized::TableSyntax
-
-      where(:app_setting, :project_setting, :feature_enabled, :final_setting) do
-        true     | true      | true   | false
-        false    | true      | true   | true
-        true     | false     | true   | false
-        false    | false     | true   | false
-        true     | true      | false  | true
-        false    | true      | false  | true
-        true     | false     | false  | false
-        false    | false     | false  | false
-      end
-
-      with_them do
-        let(:project) { create(:project) }
-        let(:feature) { :admin_merge_request_approvers_rules }
-        let(:setting) { :merge_requests_author_approval }
-        let(:application_setting) { :prevent_merge_requests_author_approval }
-
-        before do
-          stub_licensed_features(feature => feature_enabled)
-          stub_application_setting(application_setting => app_setting)
-          project.update(setting => project_setting)
-        end
-
-        it 'shows proper setting' do
-          expect(project.send(setting)).to eq(final_setting)
-          expect(project.send("#{setting}?")).to eq(final_setting)
-        end
-      end
-    end
-  end
-
   describe '#has_active_hooks?' do
     context "with group hooks" do
       let(:group) { create(:group) }

ee/spec/policies/project_policy_spec.rb

@@ -1387,18 +1387,14 @@ describe ProjectPolicy do
 
     using RSpec::Parameterized::TableSyntax
     context 'with merge request approvers rules available in license' do
-      where(:role, :setting, :admin_mode, :allowed) do
-        :guest      | true  | nil    | false
-        :reporter   | true  | nil    | false
-        :developer  | true  | nil    | false
-        :maintainer | false | nil    | true
-        :maintainer | true  | nil    | false
-        :owner      | false | nil    | true
-        :owner      | true  | nil    | false
-        :admin      | false | false  | false
-        :admin      | false | true   | true
-        :admin      | true  | false  | false
-        :admin      | true  | true   | true
+      where(:role, :admin_mode, :allowed) do
+        :guest      | nil   | false
+        :reporter   | nil   | false
+        :developer  | nil   | false
+        :maintainer | nil   | false
+        :owner      | nil   | false
+        :admin      | false | false
+        :admin      | true  | true
       end
 
       with_them do
@@ -1406,7 +1402,6 @@ describe ProjectPolicy do
 
         before do
           stub_licensed_features(admin_merge_request_approvers_rules: true)
-          stub_application_setting(setting_name => setting)
           enable_admin_mode!(current_user) if admin_mode
         end
 
@@ -1415,18 +1410,14 @@ describe ProjectPolicy do
     end
 
     context 'with merge request approvers not available in license' do
-      where(:role, :setting, :admin_mode, :allowed) do
-        :guest      | true  | nil    | false
-        :reporter   | true  | nil    | false
-        :developer  | true  | nil    | false
-        :maintainer | false | nil    | true
-        :maintainer | true  | nil    | true
-        :owner      | false | nil    | true
-        :owner      | true  | nil    | true
-        :admin      | false | false  | false
-        :admin      | false | true   | true
-        :admin      | true  | false  | false
-        :admin      | true  | true   | true
+      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
@@ -1434,7 +1425,6 @@ describe ProjectPolicy do
 
         before do
           stub_licensed_features(admin_merge_request_approvers_rules: false)
-          stub_application_setting(setting_name => setting)
           enable_admin_mode!(current_user) if admin_mode
         end
 
@@ -1465,66 +1455,9 @@ describe ProjectPolicy do
   end
 
   describe ':modify_approvers_list' do
+    it_behaves_like 'merge request rules' do
       let(:setting_name) { :disable_overriding_approvers_per_merge_request }
       let(:policy) { :modify_approvers_list }
-    let(:project) { create(:project, namespace: owner.namespace) }
-
-    using RSpec::Parameterized::TableSyntax
-
-    context 'with merge request approvers rules available in license' do
-      where(:role, :setting, :admin_mode, :allowed) do
-        :guest      | true  | nil   | false
-        :reporter   | true  | nil   | false
-        :developer  | true  | nil   | false
-        :maintainer | false | nil   | true
-        :maintainer | true  | nil   | false
-        :owner      | false | nil   | true
-        :owner      | true  | nil   | false
-        :admin      | false | false | false
-        :admin      | false | true  | true
-        :admin      | true  | false | false
-        :admin      | true  | true  | true
-      end
-
-      with_them do
-        let(:current_user) { public_send(role) }
-
-        before do
-          stub_licensed_features(admin_merge_request_approvers_rules: true)
-          stub_application_setting(setting_name => setting)
-          enable_admin_mode!(current_user) if admin_mode
-        end
-
-        it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
-      end
-    end
-
-    context 'with merge request approvers not available in license' do
-      where(:role, :setting, :admin_mode, :allowed) do
-        :guest      | true  | nil   | false
-        :reporter   | true  | nil   | false
-        :developer  | true  | nil   | false
-        :maintainer | false | nil   | true
-        :maintainer | true  | nil   | true
-        :owner      | false | nil   | true
-        :owner      | true  | nil   | true
-        :admin      | false | false | false
-        :admin      | false | true  | true
-        :admin      | true  | false | false
-        :admin      | true  | true  | true
-      end
-
-      with_them do
-        let(:current_user) { public_send(role) }
-
-        before do
-          stub_licensed_features(admin_merge_request_approvers_rules: false)
-          stub_application_setting(setting_name => setting)
-          enable_admin_mode!(current_user) if admin_mode
-        end
-
-        it { is_expected.to(allowed ? be_allowed(policy) : be_disallowed(policy)) }
-      end
     end
   end
 

ee/spec/requests/api/settings_spec.rb

@@ -181,9 +181,41 @@ describe API::Settings, 'EE Settings' do
     end
     let(:feature) { :admin_merge_request_approvers_rules }
 
+    context 'when feature flag is enabled' do
+      before do
+        stub_feature_flags(admin_merge_request_approvals_settings: true)
+      end
+
       it_behaves_like 'settings for licensed features'
     end
 
+    context 'when feature flag is disabled' do
+      let(:attribute_names) { settings.keys.map(&:to_s) }
+
+      before do
+        stub_licensed_features(feature => true)
+        stub_feature_flags(admin_merge_request_approvals_settings: false)
+      end
+
+      it 'hides the attributes in the API' do
+        get api("/application/settings", admin)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        attribute_names.each do |attribute|
+          expect(json_response.keys).not_to include(attribute)
+        end
+      end
+
+      it 'does not update application settings' do
+        # Make sure the settings exist before the specs
+        get api("/application/settings", admin)
+
+        expect { put api("/application/settings", admin), params: settings }
+          .not_to change { ApplicationSetting.current.reload.attributes.slice(*attribute_names) }
+      end
+    end
+  end
+
   context 'updating npm packages request forwarding' do
     let(:settings) { { npm_package_requests_forwarding: true } }
     let(:feature) { :packages }