Merge branch '337941-2-enforce-settings' into 'master'

Enforce instance, group and project merge request approval settings

See merge request gitlab-org/gitlab!69546

app/models/project.rb

@@ -2655,10 +2655,6 @@ class Project < ApplicationRecord
     ProjectStatistics.increment_statistic(self, statistic, delta)
   end
 
-  def merge_requests_author_approval
-    !!read_attribute(:merge_requests_author_approval)
-  end
-
   def ci_forward_deployment_enabled?
     return false unless ci_cd_settings
 

doc/user/project/merge_requests/approvals/settings.md

@@ -5,7 +5,7 @@ info: "To determine the technical writer assigned to the Stage/Group associated
 type: reference, concepts
 ---
 
-# Merge request approval settings **(FREE)**
+# Merge request approval settings **(PREMIUM)**
 
 You can configure the settings for [merge request approvals](index.md) to
 ensure the approval rules meet your use case. You can also configure
@@ -30,7 +30,7 @@ In this section of general settings, you can configure the following settings:
 | [Require user password to approve](#require-user-password-to-approve) | Force potential approvers to first authenticate with a password. |
 | [Remove all approvals when commits are added to the source branch](#remove-all-approvals-when-commits-are-added-to-the-source-branch) | When enabled, remove all existing approvals on a merge request when more changes are added to it. |
 
-## Prevent approval by author **(PREMIUM)**
+## Prevent approval by author
 
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/3349) in GitLab 11.3.
 > - Moved to GitLab Premium in 13.9.
@@ -52,7 +52,7 @@ this setting, unless you configure one of these options:
   at the instance level, you can't edit this setting at the project or individual
   merge request levels.
 
-## Prevent approvals by users who add commits **(PREMIUM)**
+## Prevent approvals by users who add commits
 
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/10441) in GitLab 11.10.
 > - Moved to GitLab Premium in 13.9.
@@ -126,13 +126,25 @@ merge request could introduce a vulnerability.
 
 To learn more, see [Security approvals in merge requests](../../../application_security/index.md#security-approvals-in-merge-requests).
 
-## Code coverage check approvals **(PREMIUM)**
+## Code coverage check approvals
 
 You can require specific approvals if a merge request would result in a decline in code test
 coverage.
 
 To learn more, see [Coverage check approval rule](../../../../ci/pipelines/settings.md#coverage-check-approval-rule).
 
+## Merge request approval settings cascading
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.4. [Deployed behind the `group_merge_request_approval_settings_feature_flag` flag](../../../../administration/feature_flags.md), disabled by default.
+
+FLAG:
+On self-managed GitLab, by default this feature is not available. To make it available per group, ask an administrator to [enable the `group_merge_request_approval_settings_feature_flag` flag](../../../../administration/feature_flags.md). On GitLab.com, this feature is not available.
+You should not use this feature for production environments
+
+You can now enforce merge request approval settings at an instance level which will apply to all groups on an instance and, by extension, all projects. It is also possible to enforce merge request approval settings on an individual root group which will apply to all subgroups and projects.
+
+If the settings are inherited by a group or project, they cannot be overridden by the group or project that inherited them.
+
 ## Related links
 
 - [Instance-level merge request approval settings](../../../admin_area/merge_requests_approvals.md)

ee/app/models/ee/project.rb

@@ -543,7 +543,13 @@ module EE
     end
 
     def reset_approvals_on_push
-      super && feature_available?(:merge_request_approvers)
+      if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self.group&.root_ancestor)
+        !ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: self)
+                                                                    .retain_approvals_on_push
+                                                                    .value && feature_available?(:merge_request_approvers)
+      else
+        super && feature_available?(:merge_request_approvers)
+      end
     end
     alias_method :reset_approvals_on_push?, :reset_approvals_on_push
 
@@ -571,7 +577,13 @@ module EE
     end
 
     def require_password_to_approve
-      super && password_authentication_enabled_for_web?
+      if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self&.group&.root_ancestor)
+        ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group, project: self)
+                                                                    .require_password_to_approve
+                                                                    .value && password_authentication_enabled_for_web?
+      else
+        super && password_authentication_enabled_for_web?
+      end
     end
 
     def require_password_to_approve?
@@ -728,9 +740,17 @@ module EE
 
     def disable_overriding_approvers_per_merge_request
       strong_memoize(:disable_overriding_approvers_per_merge_request) do
-        next super unless License.feature_available?(:admin_merge_request_approvers_rules)
+        if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self)
+          super unless feature_available?(:admin_merge_request_approvers_rules)
 
-        ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super
+          !ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
+                                                                      .allow_overrides_to_approver_list_per_merge_request
+                                                                      .value
+        else
+          next super unless feature_available?(:admin_merge_request_approvers_rules)
+
+          ::Gitlab::CurrentSettings.disable_overriding_approvers_per_merge_request? || super
+        end
       end
     end
 
@@ -740,10 +760,18 @@ module EE
 
     def merge_requests_author_approval
       strong_memoize(:merge_requests_author_approval) do
-        next super unless License.feature_available?(:admin_merge_request_approvers_rules)
-        next false if ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
+        if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self)
+          super unless feature_available?(:admin_merge_request_approvers_rules)
 
-        super
+          ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
+                                                                       .allow_author_approval
+                                                                       .value
+        else
+          next super unless License.feature_available?(:admin_merge_request_approvers_rules)
+          next false if ::Gitlab::CurrentSettings.prevent_merge_requests_author_approval?
+
+          !!read_attribute(:merge_requests_author_approval)
+        end
       end
     end
 
@@ -753,9 +781,17 @@ module EE
 
     def merge_requests_disable_committers_approval
       strong_memoize(:merge_requests_disable_committers_approval) do
-        next super unless License.feature_available?(:admin_merge_request_approvers_rules)
+        if ::Feature.enabled?(:group_merge_request_approval_settings_feature_flag, self)
+          super unless feature_available?(:admin_merge_request_approvers_rules)
 
-        ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super
+          !ComplianceManagement::MergeRequestApprovalSettings::Resolver.new(group&.root_ancestor, project: self)
+                                                                       .allow_committer_approval
+                                                                       .value
+        else
+          next super unless License.feature_available?(:admin_merge_request_approvers_rules)
+
+          ::Gitlab::CurrentSettings.prevent_merge_requests_committers_approval? || super
+        end
       end
     end
 

ee/app/models/license.rb

@@ -99,7 +99,6 @@ class License < ApplicationRecord
     group_coverage_reports
     group_forking_protection
     group_merge_request_analytics
-    group_merge_request_approval_settings
     group_milestone_project_releases
     group_project_templates
     group_repository_analytics

ee/app/policies/ee/group_policy.rb

@@ -122,7 +122,7 @@ module EE
       end
 
       condition(:group_merge_request_approval_settings_enabled) do
-        @subject.feature_available?(:group_merge_request_approval_settings) && @subject.root?
+        @subject.feature_available?(:merge_request_approvers) && @subject.root?
       end
 
       condition(:over_storage_limit, scope: :subject) { @subject.over_storage_limit? }

ee/app/policies/ee/project_policy.rb

@@ -33,7 +33,7 @@ module EE
       end
 
       condition(:group_merge_request_approval_settings_enabled) do
-        @subject.feature_available?(:group_merge_request_approval_settings)
+        @subject.feature_available?(:merge_request_approvers)
       end
 
       with_scope :global

ee/lib/compliance_management/merge_request_approval_settings/resolver.rb

@@ -20,7 +20,7 @@ module ComplianceManagement
       def allow_author_approval
         instance_value = !instance_settings.prevent_merge_requests_author_approval
         group_value = group_settings&.allow_author_approval
-        project_value = @project&.merge_requests_author_approval
+        project_value = @project && @project.read_attribute(:merge_requests_author_approval)
 
         setting(instance_value, group_value, project_value)
       end
@@ -28,7 +28,7 @@ module ComplianceManagement
       def allow_committer_approval
         instance_value = !instance_settings.prevent_merge_requests_committers_approval
         group_value = group_settings&.allow_committer_approval
-        project_value = @project ? !@project.merge_requests_disable_committers_approval : nil
+        project_value = @project && !@project.read_attribute(:merge_requests_disable_committers_approval)
 
         setting(instance_value, group_value, project_value)
       end
@@ -36,21 +36,21 @@ module ComplianceManagement
       def allow_overrides_to_approver_list_per_merge_request
         instance_value = !instance_settings.disable_overriding_approvers_per_merge_request
         group_value = group_settings&.allow_overrides_to_approver_list_per_merge_request
-        project_value = @project ? !@project.disable_overriding_approvers_per_merge_request : nil
+        project_value = @project && !@project.read_attribute(:disable_overriding_approvers_per_merge_request)
 
         setting(instance_value, group_value, project_value)
       end
 
       def retain_approvals_on_push
         group_value = group_settings&.retain_approvals_on_push
-        project_value = @project ? !@project.reset_approvals_on_push : nil
+        project_value = @project && !@project.read_attribute(:reset_approvals_on_push)
 
         setting(nil, group_value, project_value)
       end
 
       def require_password_to_approve
         group_value = group_settings&.require_password_to_approve
-        project_value = @project&.require_password_to_approve
+        project_value = @project && @project.read_attribute(:require_password_to_approve)
 
         ComplianceManagement::MergeRequestApprovalSettings::Setting.new(
           value: require_password_value(group_value, project_value),

ee/spec/features/groups/group_settings_spec.rb

@@ -322,7 +322,7 @@ RSpec.describe 'Edit group settings' do
     context 'when feature flag is enabled and group is licensed' do
       before do
         stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
-        stub_licensed_features(group_merge_request_approval_settings: true)
+        stub_licensed_features(merge_request_approvers: true)
       end
 
       it 'allows to save settings' do
@@ -355,7 +355,7 @@ RSpec.describe 'Edit group settings' do
     context 'when feature flag is disabled and group is not licensed' do
       before do
         stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
-        stub_licensed_features(group_merge_request_approval_settings: false)
+        stub_licensed_features(merge_request_approvers: false)
       end
 
       it 'is not visible' do

ee/spec/features/projects/audit_events_spec.rb

@@ -140,7 +140,7 @@ RSpec.describe 'Projects > Audit Events', :js do
       describe 'changing merge request approval permission for authors and reviewers' do
         before do
           stub_feature_flags(group_merge_request_approval_settings_feature_flag: feature_enabled)
-          stub_licensed_features(group_merge_request_approval_settings: feature_enabled)
+          stub_licensed_features(merge_request_approvers: true)
           project.add_developer(pete)
         end
 
@@ -157,6 +157,9 @@ RSpec.describe 'Projects > Audit Events', :js do
 
           page.within('.sidebar-top-level-items') do
             click_link 'Security & Compliance'
+
+            wait_for_all_requests
+
             click_link 'Audit Events'
           end
 

ee/spec/models/approval_state_spec.rb

@@ -785,23 +785,121 @@ RSpec.describe ApprovalState do
     end
 
     describe '#authors_can_approve?' do
-      context 'when project allows author approval' do
+      context 'group_merge_request_approval_settings_feature_flag is enabled' do
         before do
-          project.update!(merge_requests_author_approval: true)
+          stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
         end
+        context 'allow_author_approval is resolved to not be permitted' do
+          before do
+            allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
+              allow(instance).to receive(:allow_author_approval).and_return(
+                ComplianceManagement::MergeRequestApprovalSettings::Setting.new(value: false, locked: false, inherited_from: nil)
+              )
+            end
+          end
 
-        it 'returns true' do
-          expect(subject.authors_can_approve?).to eq(true)
+          it 'returns false' do
+            expect(subject.authors_can_approve?).to be false
+          end
+        end
+
+        context 'allow_author_approval is resolved to be permitted' do
+          before do
+            allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
+              allow(instance).to receive(:allow_author_approval).and_return(
+                ComplianceManagement::MergeRequestApprovalSettings::Setting.new(value: true, locked: false, inherited_from: nil)
+              )
+            end
+          end
+
+          it 'returns true' do
+            expect(subject.authors_can_approve?).to be true
+          end
         end
       end
 
-      context 'when project disallows author approval' do
+      context 'group_merge_request_approval_settings_feature_flag is disabled' do
         before do
-          project.update!(merge_requests_author_approval: false)
+          stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
         end
+        context 'when project allows author approval' do
+          before do
+            project.update!(merge_requests_author_approval: true)
+          end
 
-        it 'returns true' do
-          expect(subject.authors_can_approve?).to eq(false)
+          it 'returns true' do
+            expect(subject.authors_can_approve?).to eq(true)
+          end
+        end
+
+        context 'when project disallows author approval' do
+          before do
+            project.update!(merge_requests_author_approval: false)
+          end
+
+          it 'returns true' do
+            expect(subject.authors_can_approve?).to eq(false)
+          end
+        end
+      end
+    end
+
+    describe '#committers_can_approve?' do
+      context 'group_merge_request_approval_settings_feature_flag is enabled' do
+        before do
+          stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
+        end
+        context 'allow_committer_approval is resolved to not be permitted' do
+          before do
+            allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
+              allow(instance).to receive(:allow_committer_approval).and_return(
+                ComplianceManagement::MergeRequestApprovalSettings::Setting.new(value: false, locked: false, inherited_from: nil)
+              )
+            end
+          end
+
+          it 'returns false' do
+            expect(subject.committers_can_approve?).to be false
+          end
+        end
+
+        context 'allow_committer_approval is resolved to be permitted' do
+          before do
+            allow_next_instance_of ComplianceManagement::MergeRequestApprovalSettings::Resolver do |instance|
+              allow(instance).to receive(:allow_committer_approval).and_return(
+                ComplianceManagement::MergeRequestApprovalSettings::Setting.new(value: true, locked: false, inherited_from: nil)
+              )
+            end
+          end
+
+          it 'returns false' do
+            expect(subject.committers_can_approve?).to be true
+          end
+        end
+      end
+
+      context 'group_merge_request_approval_settings_feature_flag is disabled' do
+        before do
+          stub_feature_flags(group_merge_request_approval_settings_feature_flag: false)
+        end
+        context 'when project allows committer approval' do
+          before do
+            project.update!(merge_requests_disable_committers_approval: false)
+          end
+
+          it 'returns true' do
+            expect(subject.committers_can_approve?).to eq(true)
+          end
+        end
+
+        context 'when project disallows committer approval' do
+          before do
+            project.update!(merge_requests_disable_committers_approval: true)
+          end
+
+          it 'returns true' do
+            expect(subject.committers_can_approve?).to eq(false)
+          end
         end
       end
     end

ee/spec/policies/group_policy_spec.rb

@@ -1589,7 +1589,7 @@ RSpec.describe GroupPolicy do
         let(:current_user) { public_send(role) }
 
         before do
-          stub_licensed_features(group_merge_request_approval_settings: licensed)
+          stub_licensed_features(merge_request_approvers: licensed)
           enable_admin_mode!(current_user) if admin_mode
           group.parent = build(:group) unless root_group
         end

ee/spec/policies/project_policy_spec.rb

@@ -1425,7 +1425,7 @@ RSpec.describe ProjectPolicy do
       let(:current_user) { public_send(role) }
 
       before do
-        stub_licensed_features(group_merge_request_approval_settings: licensed)
+        stub_licensed_features(merge_request_approvers: licensed)
         enable_admin_mode!(current_user) if role == :admin
       end
 

ee/spec/requests/api/merge_request_approval_settings_spec.rb

@@ -232,7 +232,7 @@ RSpec.describe API::MergeRequestApprovalSettings do
         group.add_owner(user)
         allow(Ability).to receive(:allowed?).and_call_original
         stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
-        stub_licensed_features(group_merge_request_approval_settings: true)
+        stub_licensed_features(merge_request_approvers: true)
         allow(Ability).to receive(:allowed?)
                             .with(user, :admin_merge_request_approval_settings, project)
                             .and_return(true)
@@ -291,7 +291,7 @@ RSpec.describe API::MergeRequestApprovalSettings do
         group.add_owner(user)
         allow(Ability).to receive(:allowed?).and_call_original
         stub_feature_flags(group_merge_request_approval_settings_feature_flag: true)
-        stub_licensed_features(group_merge_request_approval_settings: true)
+        stub_licensed_features(merge_request_approvers: true)
         allow(Ability).to receive(:allowed?)
                             .with(user, :admin_merge_request_approval_settings, project)
                             .and_return(true)

spec/models/project_spec.rb

@@ -495,23 +495,6 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
-  describe '#merge_requests_author_approval' do
-    where(:attribute_value, :return_value) do
-      true  | true
-      false | false
-      nil   | false
-    end
-
-    with_them do
-      let(:project) { create(:project, merge_requests_author_approval: attribute_value) }
-
-      it 'returns expected value' do
-        expect(project.merge_requests_author_approval).to eq(return_value)
-        expect(project.merge_requests_author_approval?).to eq(return_value)
-      end
-    end
-  end
-
   describe '#all_pipelines' do
     let_it_be(:project) { create(:project) }