BE: Support Project Approval Settings in Security Policies

Why are we doing this work

We are extending policies to allow users to override project settings for MR approvals, namely:

• Prevent approval by merge request's author
• Prevent approval by anyone who added a commit
• Remove all approvals when a commit is added
• Require the user's password to approve

Relevant links

• See epic

Non-functional requirements

• Documentation: Document the new approval_settings option in YAML
• Feature flag: Put the changes behind a feature flag
• Performance: Lookup of the approval settings for MR should perform well
• Testing:

Implementation plan

• MR 1: Extend ee/app/validators/json_schemas/security_orchestration_policy.json with new attributes Example YAML:

type: scan_result_policy
name: MR approvals
description: This policy overrides project approval settings
enabled: true
rules:
  - type: any_merge_request
    branch_type: protected # or branches: ['main']
    commits: unsigned # or any
actions:
  - type: require_approval
    approvals_required: 1
    user_approvers:
      - the.one
approval_settings:
  prevent_approval_by_author: true
  prevent_approval_by_commit_author: true
  remove_approvals_with_new_commit: true
  require_password_to_approve: false

diff --git a/ee/app/validators/json_schemas/security_orchestration_policy.json b/ee/app/validators/json_schemas/security_orchestration_policy.json
index ad0ccacd30d7d95d23b0a35cfa23104a081efe76..ac5bdffbb114590bde6b9bea3802c7a7f785a22a 100644
--- a/ee/app/validators/json_schemas/security_orchestration_policy.json
+++ b/ee/app/validators/json_schemas/security_orchestration_policy.json
@@ -340,14 +340,15 @@
                 "type": {
                   "enum": [
                     "scan_finding",
-                    "license_finding"
+                    "license_finding",
+                    "any_merge_request"
                   ],
                   "type": "string",
-                  "description": "Specified a type of the policy rule. `scan_finding`/`license_finding` rule enforces the defined actions based on the provided information."
+                  "description": "Specified a type of the policy rule. `scan_finding`/`license_finding`/`any_merge_request` rule enforces the defined actions based on the provided information."
                 },
                 "branches": {
                   "type": "array",
-                  "description": "Specifies a list of protected branches that should be concidered to enforce this policy.",
+                  "description": "Specifies a list of protected branches that should be considered to enforce this policy.",
                   "additionalItems": false,
                   "items": {
                     "minLength": 1,
@@ -512,6 +513,14 @@
                       "detected"
                     ]
                   }
+                },
+                "commits": {
+                  "type": "string",
+                  "description": "Specifies the commits to match.",
+                  "enum": [
+                    "any",
+                    "unsigned"
+                  ]
                 }
               },
               "oneOf": [
@@ -561,6 +570,21 @@
                       "license_states"
                     ]
                   }
+                },
+                {
+                  "if": {
+                    "properties": {
+                      "type": {
+                        "const": "any_merge_request"
+                      }
+                    }
+                  },
+                  "then": {
+                    "required": [
+                      "type",
+                      "commits"
+                    ]
+                  }
                 }
               ],
               "additionalProperties": false
@@ -682,6 +706,24 @@
                 }
               }
             }
+          },
+          "approval_settings": {
+            "type": "object",
+            "additionalProperties": false,
+            "properties": {
+              "prevent_approval_by_author": {
+                "type": "boolean"
+              },
+              "prevent_approval_by_commit_author": {
+                "type": "boolean"
+              },
+              "remove_approvals_with_new_commit": {
+                "type": "boolean"
+              },
+              "require_password_to_approve": {
+                "type": "boolean"
+              }
+            }
           }
         },
         "additionalProperties": false

• MR 2: Add new columns in scan_result_policies table and persist the values from YAML:
• New columns

add_column :scan_result_policies, :approval_settings, :jsonb, default: {}, null: false
add_column :scan_result_policies, :commits, :smallint

• Persist the values:

diff --git a/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb b/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb
index 88bc0a2c302a64ca35dcdd398a4535e0123fd732..e4cad04318e2e192465e13f43b1c50cda0e7bd0c 100644
--- a/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb
+++ b/ee/app/services/security/security_orchestration_policies/process_scan_result_policy_service.rb
@@ -26,7 +26,15 @@ def create_new_approval_rules
         policy[:rules].first(Security::ScanResultPolicy::LIMIT).each_with_index do |rule, rule_index|
           next unless rule_type_allowed?(rule[:type])
 
-          scan_result_policy_read = create_scan_result_policy(rule, action_info, project, rule_index)
+          scan_result_policy_read = create_scan_result_policy(
+            rule,
+            action_info,
+            policy[:approval_settings],
+            project,
+            rule_index
+          )
 
           create_software_license_policies(rule, rule_index, scan_result_policy_read) if license_finding?(rule)
 
@@ -40,6 +48,10 @@ def license_finding?(rule)
         rule[:type] == Security::ScanResultPolicy::LICENSE_FINDING
       end
 
       def create_software_license_policies(rule, _rule_index, scan_result_policy_read)
         rule[:license_types].each do |license_type|
           create_params = {
@@ -54,7 +66,7 @@ def create_software_license_policies(rule, _rule_index, scan_result_policy_read)
         end
       end
 
-      def create_scan_result_policy(rule, action_info, project, rule_index)
+      def create_scan_result_policy(rule, action_info, approval_settings, project, rule_index)
         policy_configuration.scan_result_policy_reads.create!(
           orchestration_policy_idx: policy_index,
           rule_idx: rule_index,
@@ -65,7 +77,9 @@ def create_scan_result_policy(rule, action_info, project, rule_index)
           project_id: project.id,
           age_operator: rule.dig(:vulnerability_age, :operator),
           age_interval: rule.dig(:vulnerability_age, :interval),
-          age_value: rule.dig(:vulnerability_age, :value)
+          age_value: rule.dig(:vulnerability_age, :value),
+          commits: rule[:commits],
+          approval_settings: approval_settings || {}
         )
       end
 
@@ -116,7 +130,8 @@ def applies_to_all_protected_branches?(rule)
       def rule_type_allowed?(rule_type)
         [
           Security::ScanResultPolicy::SCAN_FINDING,
-          Security::ScanResultPolicy::LICENSE_FINDING
+          Security::ScanResultPolicy::LICENSE_FINDING,
+          Security::ScanResultPolicy::ANY_MERGE_REQUEST
         ].include?(rule_type)
       end

       def report_type(rule_type)
-        rule_type == Security::ScanResultPolicy::LICENSE_FINDING ? :license_scanning : :scan_finding
+        case rule_type
+        when Security::ScanResultPolicy::LICENSE_FINDING
+          :license_scanning
+        when Security::ScanResultPolicy::ANY_MERGE_REQUEST
+          :any_merge_request
+        else
+          :scan_finding
+        end
       end
 
       def rule_name(policy_name, rule_index)
diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb
index 49dab697e785..a9287bf72942 100644
--- a/ee/app/models/concerns/approval_rule_like.rb
+++ b/ee/app/models/concerns/approval_rule_like.rb
@@ -35,8 +35,7 @@ module ApprovalRuleLike
       vulnerability: 1, # To be removed after all MRs (related to https://gitlab.com/gitlab-org/gitlab/-/issues/356996) get merged
       license_scanning: 2,
       code_coverage: 3,
-      scan_finding: 4
+      scan_finding: 4,
+      any_merge_request: 5
     }

     validates :name, presence: true
diff --git a/ee/app/models/concerns/security/scan_result_policy.rb b/ee/app/models/concerns/security/scan_result_policy.rb
index c9056231496397b94a5ad3361b2722cde4a61190..1c43a2ecb5d90e71ca81207481ef679daba59c1b 100644
--- a/ee/app/models/concerns/security/scan_result_policy.rb
+++ b/ee/app/models/concerns/security/scan_result_policy.rb
@@ -11,6 +11,7 @@ module ScanResultPolicy
 
     SCAN_FINDING = 'scan_finding'
     LICENSE_FINDING = 'license_finding'
+    ANY_MERGE_REQUEST = 'any_merge_request'
 
     REQUIRE_APPROVAL = 'require_approval'

• MR 3: Enforce the policy and override the project settings
• Extend update_approvals_service to remove required approvals

diff --git a/ee/app/services/security/scan_result_policies/update_approvals_service.rb b/ee/app/services/security/scan_result_policies/update_approvals_service.rb
index 3df6bb9771553d1ce7222100726ac3b61016a5d6..6f1d29e21685e7af8f82b814a09ae8372e8e4cac 100644
--- a/ee/app/services/security/scan_result_policies/update_approvals_service.rb
+++ b/ee/app/services/security/scan_result_policies/update_approvals_service.rb
@@ -13,6 +13,13 @@ def initialize(merge_request:, pipeline:)
       end
 
       def execute
+        update_scan_finding_rules
+        update_any_merge_request_rules
+      end
+
+      private
+
+      def update_scan_finding_rules
         approval_rules = merge_request.approval_rules.scan_finding
         return if approval_rules.empty?
 
@@ -26,10 +33,24 @@ def execute
         violated_rules, unviolated_rules = partition_rules(approval_rules)
 
         ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any?
         generate_policy_bot_comment(violated_rules.any?)
       end
 
-      private
+      def update_any_merge_request_rules
+        approval_rules = merge_request.approval_rules.any_merge_request
+        return if approval_rules.empty?
+
+        merge_request_has_unsigned_commits = !merge_request.commits(load_from_gitaly: true).all?(&:has_signature?)
+        violated_rules, unviolated_rules = approval_rules.including_scan_result_policy_read
+                                                         .partition do |approval_rule|
+          scan_result_policy_read = approval_rule.scan_result_policy_read
+          scan_result_policy_read.commits_any? ||
+            (scan_result_policy_read.commits_unsigned? && merge_request_has_unsigned_commits)
+        end
+
+        ApprovalMergeRequestRule.remove_required_approved(unviolated_rules) if unviolated_rules.any?
+      end
 
       delegate :project, to: :pipeline

• Add method to lookup approval settings for a merge_request

diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb
index 49dab697e785..c0cfac9eb7d1 100644
--- a/ee/app/models/concerns/approval_rule_like.rb
+++ b/ee/app/models/concerns/approval_rule_like.rb
@@ -28,7 +28,7 @@ module ApprovalRuleLike
     belongs_to :scan_result_policy_read,
       class_name: 'Security::ScanResultPolicyRead',
       foreign_key: 'scan_result_policy_id',
-      inverse_of: :security_orchestration_policy_configuration,
+      inverse_of: :approval_merge_request_rules,
       optional: true

     enum report_type: {
@@ -49,7 +49,6 @@ module ApprovalRuleLike

     scope :with_users, -> { preload(:users, :group_users) }
     scope :regular_or_any_approver, -> { where(rule_type: [:regular, :any_approver]) }
+    scope :scan_finding_or_license_scanning, -> { where(report_type: %i[scan_finding license_scanning]) }
     scope :for_groups, -> (groups) { joins(:groups).where(approval_project_rules_groups: { group_id: groups }) }
     scope :including_scan_result_policy_read, -> { includes(:scan_result_policy_read) }
     scope :with_scan_result_policy_read, -> { where.not(scan_result_policy_id: nil) }
diff --git a/ee/app/models/security/scan_result_policy_read.rb b/ee/app/models/security/scan_result_policy_read.rb
index 7be63a51646e84f57c637504ea5790572801de69..d08aea59b9432c1e9c3eb87ab05836d51abffcab 100644
--- a/ee/app/models/security/scan_result_policy_read.rb
+++ b/ee/app/models/security/scan_result_policy_read.rb
@@ -8,10 +8,12 @@ class ScanResultPolicyRead < ApplicationRecord
 
     enum age_operator: { greater_than: 0, less_than: 1 }
     enum age_interval: { day: 0, week: 1, month: 2, year: 3 }
+    enum commits: { any: 0, unsigned: 1 }, _prefix: true
 
     belongs_to :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration'
     belongs_to :project, optional: true
     has_many :software_license_policies
+    has_many :approval_merge_request_rules, foreign_key: 'scan_result_policy_id', inverse_of: :scan_result_policy_read
 
     validates :match_on_inclusion, inclusion: { in: [true, false], message: 'must be a boolean value' }
     validates :role_approvers, inclusion: { in: Gitlab::Access.all_values }
@@ -31,5 +33,26 @@ def vulnerability_age
 
       { operator: age_operator.to_sym, interval: age_interval.to_sym, value: age_value }
     end
+
+    def self.approval_settings_for_merge_request(merge_request)
+      commits_lookup = %i[any]
+      commits_lookup << :unsigned unless merge_request.commits(load_from_gitaly: true).all?(&:has_signature?)
+      # scan_finding and license_scanning should enforce approval_settings only when there is a violation,
+      # which we can only find out by approvals_required > 0.
+      # If policy doesn't require approvals, we may still have violations and should enforce approval_settings,
+      # but we wouldn't find them at this point.
+      # Should we store `violations: true/false` for approval_merge_request_rules?
+      enforced_merge_request_rules = merge_request.approval_rules.scan_finding_or_license_scanning
+                                               .where(ApprovalMergeRequestRule.arel_table[:approvals_required].gt(0))
+      # any_merge_request should enforce approval_settings based on commits type
+      all_approval_settings = joins(:approval_merge_request_rules)
+                                .where(project: merge_request.target_project, commits: commits_lookup)
+                                .or(enforced_merge_request_rules)
+                                .pluck(:approval_settings)
+
+      all_approval_settings.each_with_object({}) do |setting, hash|
+        setting.each { |key, value| hash[key] = value if value }
+      end.symbolize_keys
+    end
   end
 end

• prevent_approval_by_author and prevent_approval_by_commit_author

diff --git a/ee/app/models/approval_state.rb b/ee/app/models/approval_state.rb
index f5dde9bd606d69c3305ca427a216ecc37093e193..24bd77f0d47cd4969db154801d24983be4d46b29 100644
--- a/ee/app/models/approval_state.rb
+++ b/ee/app/models/approval_state.rb
@@ -21,7 +21,7 @@ def initialize(merge_request, target_branch: nil)
 
   # Excludes the author if 'author-approval' is explicitly disabled on project settings.
   def self.filter_author(users, merge_request)
-    return users if merge_request.target_project.merge_requests_author_approval?
+    return users if merge_request.merge_requests_author_approval?
 
     if users.is_a?(ActiveRecord::Relation) && !users.loaded?
       users.where.not(id: merge_request.author_id).allow_cross_joins_across_databases(url: "https://gitlab.com/gitlab-org/gitlab/-/issues/417459")
@@ -32,7 +32,7 @@ def self.filter_author(users, merge_request)
 
   # Excludes the author if 'committers-approval' is explicitly disabled on project settings.
   def self.filter_committers(users, merge_request)
-    return users unless merge_request.target_project.merge_requests_disable_committers_approval?
+    return users unless merge_request.merge_requests_disable_committers_approval?
 
     with_merge_commits = Feature.enabled?(:keep_merge_commits_for_approvals, merge_request.target_project)
 
@@ -181,11 +181,11 @@ def eligible_for_approval_by?(user)
   end
 
   def authors_can_approve?
-    project.merge_requests_author_approval?
+    merge_request.merge_requests_author_approval?
   end
 
   def committers_can_approve?
-    !project.merge_requests_disable_committers_approval?
+    !merge_request.merge_requests_disable_committers_approval?
   end
 
   # TODO: remove after #1979 is closed

• remove_approvals_with_new_commit

diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb
index e14892443e2bd734344f59cf78bb7f2fa3f9aa25..7924c67f9e0e13f3f880b179d9c1f07ff9e3ebcd 100644
--- a/ee/app/services/ee/merge_requests/base_service.rb
+++ b/ee/app/services/ee/merge_requests/base_service.rb
@@ -39,14 +39,22 @@ def filter_suggested_reviewers
       end
 
       def reset_approvals?(merge_request, _newrev)
-        (merge_request.target_project.reset_approvals_on_push ||
-          merge_request.target_project.project_setting.selective_code_owner_removals)
+        delete_approvals?(merge_request) || merge_request.target_project.project_setting.selective_code_owner_removals
+      end
+
+      def delete_approvals?(merge_request)
+        merge_request.target_project.reset_approvals_on_push ||
+          policy_approval_settings(merge_request)[:remove_approvals_with_new_commit]
+      end
+
+      def policy_approval_settings(merge_request)
+        Security::ScanResultPolicyRead.approval_settings_for_merge_request(merge_request)
       end
 
       def reset_approvals(merge_request, newrev = nil)
         return unless reset_approvals?(merge_request, newrev)
 
-        if merge_request.target_project.reset_approvals_on_push
+        if delete_approvals?(merge_request)
           delete_approvals(merge_request)
         elsif merge_request.target_project.project_setting.selective_code_owner_removals
           delete_code_owner_approvals(merge_request)

• require_password_to_approve

diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
index fce8f72bb1ea63db0d0921619b343f422e1ab024..290ca7a7306ab47c5fae4fad70448447e9779511 100644
--- a/ee/app/models/ee/merge_request.rb
+++ b/ee/app/models/ee/merge_request.rb
@@ -100,11 +100,22 @@ def sync_merge_request_compliance_violation
       end
 
       def merge_requests_author_approval?
-        !!target_project&.merge_requests_author_approval?
+        !!target_project&.merge_requests_author_approval? && !policy_approval_settings[:prevent_approval_by_author]
       end
 
       def merge_requests_disable_committers_approval?
-        !!target_project&.merge_requests_disable_committers_approval?
+        !!target_project&.merge_requests_disable_committers_approval? ||
+          policy_approval_settings[:prevent_approval_by_commit_author]
+      end
+
+      def require_password_to_approve?
+        target_project&.require_password_to_approve? || policy_approval_settings[:require_password_to_approve]
+      end
+
+      def policy_approval_settings
+        strong_memoize(:policy_approval_settings) do
+          Security::ScanResultPolicyRead.approval_settings_for_merge_request(self)
+        end
       end
     end
 
diff --git a/ee/app/serializers/ee/merge_request_widget_entity.rb b/ee/app/serializers/ee/merge_request_widget_entity.rb
index 6d18ac96b7a33dd782d4545d07e016d3ebc9e35c..b29c9b0bc589c67c88f2671541fd8346409afe8f 100644
--- a/ee/app/serializers/ee/merge_request_widget_entity.rb
+++ b/ee/app/serializers/ee/merge_request_widget_entity.rb
@@ -7,7 +7,7 @@ module MergeRequestWidgetEntity
 
     prepended do
       expose :require_password_to_approve do |merge_request|
-        merge_request.target_project.require_password_to_approve?
+        merge_request.require_password_to_approve?
       end
 
       expose :merge_request_approvers_available do |merge_request|
diff --git a/ee/app/services/ee/merge_requests/approval_service.rb b/ee/app/services/ee/merge_requests/approval_service.rb
index 019a013168587c5b4fcab8d9739f8334b3a0120a..b28fe283820fdb658bdc962e143e1a9236d34474 100644
--- a/ee/app/services/ee/merge_requests/approval_service.rb
+++ b/ee/app/services/ee/merge_requests/approval_service.rb
@@ -17,7 +17,7 @@ def execute(merge_request)
       private
 
       def incorrect_approval_password?(merge_request)
-        merge_request.project.require_password_to_approve? &&
+        merge_request.require_password_to_approve? &&
           !::Gitlab::Auth.find_with_user_password(current_user.username, params[:approval_password])
       end

Verification steps

Requiring approvals based on commits signature

1. Go to Secure -> Policies and create a scan result policies with the example YAML:

type: scan_result_policy
name: MR approvals
description: This policy requires approvals for unsigned commits and overrides project approval settings
enabled: true
rules:
  - type: any_merge_request
    branch_type: protected
    commits: unsigned
actions:
  - type: require_approval
    approvals_required: 1
    user_approvers:
      - the.one
approval_settings:
  prevent_approval_by_author: true
  prevent_approval_by_commit_author: true
  remove_approvals_with_new_commit: true
  require_password_to_approve: true

1. Go to Settings -> Merge requests and set all the approval settings to false
2. Create MR using unsigned commits
3. Check that each setting is enforced
4. Check that a rule corresponding to the policy is present and an approval is required
5. Create MR using signed commits and check that setting is not enforced
6. Change the policy to use commits: any and check that the settings are enforced also for MR with signed commits
7. Create MR for unprotected branch and check that the policy is not enforced
8. Change the target branch to a protected one and check that policy is enforced

Only overriding project settings

1. Go to Secure -> Policies and create a scan result policies with the example YAML:

type: scan_result_policy
name: Approval settings override
description: This policy overrides project approval settings
enabled: true
rules:
  - type: any_merge_request
    branch_type: protected
    commits: any
approval_settings:
  prevent_approval_by_author: true
  prevent_approval_by_commit_author: true
  remove_approvals_with_new_commit: true
  require_password_to_approve: true

1. Go to Settings -> Merge requests and set all the approval settings to false
2. Create MR
3. Check that no rule has been created
4. Check that each setting is enforced
5. Create MR for unprotected branch and check that the policy is not enforced
6. Change the target branch to a protected one and check that policy is enforced