Merge branch 'revert-ff0a219a' into 'master'

Revert '207250-allow-admins-to-modify-mr-approval-settings..' to 'master'

See merge request gitlab-org/gitlab!32610

doc/user/admin_area/merge_requests_approvals.md

@@ -19,22 +19,6 @@ 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,8 +5,6 @@ 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|
@@ -52,7 +50,7 @@ module EE
           attrs << :updating_name_disabled_for_users
         end
 
-        if show_merge_request_approvals_settings?
+        if License.feature_available?(:admin_merge_request_approvers_rules)
           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,6 +687,31 @@ 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,8 +55,24 @@ module EE
       end
 
       with_scope :global
-      condition(:admin_merge_request_approvers_rules_available) do
-        License.feature_available?(:admin_merge_request_approvers_rules)
+      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
       end
 
       with_scope :global
@@ -346,15 +362,24 @@ module EE
         prevent :read_project
       end
 
-      rule { admin_merge_request_approvers_rules_available & ~admin }.policy do
+      rule { owner_cannot_modify_approvers_rules & ~admin }.policy do
         prevent :modify_approvers_rules
-        prevent :modify_approvers_list
+      end
+
+      rule { owner_cannot_modify_merge_request_author_setting & ~admin }.policy do
         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 show_merge_request_approvals_settings?
+- return unless License.feature_available?(:admin_merge_request_approvers_rules)
 %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,14 +6,12 @@ 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
-            show_merge_request_approvals_settings?
+            ::License.feature_available?(:admin_merge_request_approvers_rules)
           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,8 +9,6 @@ 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)
@@ -37,7 +35,7 @@ module EE
               attrs = attrs.except(:updating_name_disabled_for_users)
             end
 
-            unless show_merge_request_approvals_settings?
+            unless License.feature_available?(:admin_merge_request_approvers_rules)
               attrs = attrs.except(*EE::ApplicationSettingsHelper.merge_request_appovers_rules_attributes)
             end
 

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -157,29 +157,9 @@ 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,17 +324,21 @@ describe ProjectsController do
       shared_examples 'merge request approvers rules' do
         using RSpec::Parameterized::TableSyntax
 
-        where(:can_modify, :param_value, :final_value) do
-          true  | true  | true
-          true  | false | false
-          false | true  | nil
-          false | false | nil
+        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
         end
 
         with_them do
           before do
-            allow(controller).to receive(:can?).and_call_original
-            allow(controller).to receive(:can?).with(user, rule_name, project).and_return(can_modify)
+            stub_licensed_features(admin_merge_request_approvers_rules: license_value)
+            stub_application_setting(app_setting => setting_value)
           end
 
           it 'updates project if needed' do
@@ -353,21 +357,21 @@ describe ProjectsController do
 
       describe ':disable_overriding_approvers_per_merge_request' do
         it_behaves_like 'merge request approvers rules' do
-          let(:rule_name) { :modify_approvers_rules }
+          let(:app_setting) { :disable_overriding_approvers_per_merge_request }
           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(:rule_name) { :modify_merge_request_author_setting }
+          let(:app_setting) { :prevent_merge_requests_author_approval }
           let(:setting) { :merge_requests_author_approval }
         end
       end
 
       describe ':merge_requests_disable_committers_approval' do
         it_behaves_like 'merge request approvers rules' do
-          let(:rule_name) { :modify_merge_request_committer_setting }
+          let(:app_setting) { :prevent_merge_requests_committers_approval }
           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,6 +63,28 @@ 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
@@ -71,6 +93,28 @@ 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
@@ -80,6 +124,28 @@ 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
@@ -89,6 +155,28 @@ 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,6 +514,87 @@ 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,14 +1387,18 @@ describe ProjectPolicy do
 
     using RSpec::Parameterized::TableSyntax
     context 'with merge request approvers rules available in license' do
-      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
+      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
@@ -1402,6 +1406,7 @@ 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
 
@@ -1410,14 +1415,18 @@ describe ProjectPolicy do
     end
 
     context 'with merge request approvers not available in license' do
-      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
+      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
@@ -1425,6 +1434,7 @@ 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
 
@@ -1455,9 +1465,66 @@ 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,41 +181,9 @@ 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 }