Merge branch '9928-add-report_approver-to-approval_rules' into 'master'

Security gates within MRs - Sync security report_approvers to pipeline security reports

See merge request gitlab-org/gitlab-ee!13109

ee/app/controllers/ee/projects_controller.rb

@@ -5,6 +5,10 @@ module EE
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
 
+    prepended do
+      before_action :set_report_approver_rules_feature_flag, only: [:edit]
+    end
+
     override :project_params_attributes
     def project_params_attributes
       super + project_params_ee
@@ -77,5 +81,9 @@ module EE
     def allow_merge_pipelines_params?
       project&.feature_available?(:merge_pipelines)
     end
+
+    def set_report_approver_rules_feature_flag
+      push_frontend_feature_flag(:report_approver_rules, default_enabled: false)
+    end
   end
 end

ee/app/models/approval_merge_request_rule.rb

@@ -7,6 +7,22 @@ class ApprovalMergeRequestRule < ApplicationRecord
   scope :not_matching_pattern, -> (pattern) { code_owner.where.not(name: pattern) }
   scope :matching_pattern, -> (pattern) { code_owner.where(name: pattern) }
 
+  scope :from_project_rule, -> (project_rule) do
+    joins(:approval_merge_request_rule_source)
+    .where(
+      approval_merge_request_rule_sources: { approval_project_rule_id: project_rule.id }
+    )
+  end
+  scope :for_unmerged_merge_requests, -> (merge_requests = nil) do
+    query = joins(:merge_request).where.not(merge_requests: { state: 'merged' })
+
+    if merge_requests
+      query.where(merge_request_id: merge_requests)
+    else
+      query
+    end
+  end
+
   validates :name, uniqueness: { scope: [:merge_request, :code_owner] }
   validates :report_type, presence: true, if: :report_approver?
   # Temporary validations until `code_owner` can be dropped in favor of `rule_type`
@@ -22,7 +38,7 @@ class ApprovalMergeRequestRule < ApplicationRecord
   has_one :approval_project_rule, through: :approval_merge_request_rule_source
   alias_method :source_rule, :approval_project_rule
 
-  validate :validate_approvals_required
+  validate :validate_approvals_required, unless: :report_approver?
 
   enum rule_type: {
     regular: 1,
@@ -37,6 +53,7 @@ class ApprovalMergeRequestRule < ApplicationRecord
   # Deprecated scope until code_owner column has been migrated to rule_type
   # To be removed with https://gitlab.com/gitlab-org/gitlab-ee/issues/11834
   scope :code_owner, -> { where(code_owner: true).or(where(rule_type: :code_owner)) }
+  scope :security_report, -> { report_approver.where(report_type: :security) }
 
   def self.find_or_create_code_owner_rule(merge_request, pattern)
     merge_request.approval_rules.code_owner.where(name: pattern).first_or_create do |rule|
@@ -55,7 +72,7 @@ class ApprovalMergeRequestRule < ApplicationRecord
   # Temporary override to handle legacy records that have not yet been migrated
   # To be removed with https://gitlab.com/gitlab-org/gitlab-ee/issues/11834
   def regular?
-    read_attribute(:rule_type) == 'regular' || code_owner == false
+    read_attribute(:rule_type) == 'regular' || (!report_approver? && !code_owner)
   end
   alias_method :regular, :regular?
 

ee/app/models/approval_project_rule.rb

@@ -5,30 +5,15 @@ class ApprovalProjectRule < ApplicationRecord
 
   belongs_to :project
 
-  # To allow easier duck typing
-  scope :regular, -> { all }
-  scope :code_owner, -> { none }
+  enum rule_type: {
+    regular: 0,
+    code_owner: 1, # currently unused
+    report_approver: 2
+  }
 
-  def regular
-    true
-  end
-  alias_method :regular?, :regular
-
-  def code_owner
-    false
-  end
-  alias_method :code_owner?, :code_owner
-
-  def report_approver
-    false
-  end
-  alias_method :report_approver?, :report_approver
+  alias_method :code_owner, :code_owner?
 
   def source_rule
     nil
   end
-
-  def rule_type
-    'regular'
-  end
 end

ee/app/models/approval_state.rb

@@ -46,12 +46,13 @@ class ApprovalState
 
       result = use_fallback? ? [fallback_rule] : regular_rules
       result += code_owner_rules
+      result += report_approver_rules
       result
     end
   end
 
   def has_non_fallback_rules?
-    has_regular_rule_with_approvers? || code_owner_rules.present?
+    has_regular_rule_with_approvers? || code_owner_rules.present? || report_approver_rules.present?
   end
 
   # Use the fallback rule if regular rules are empty
@@ -110,12 +111,14 @@ class ApprovalState
 
   # @param regular [Boolean]
   # @param code_owner [Boolean]
+  # @param report_approver [Boolean]
   # @param target [:approvers, :users]
   # @param unactioned [Boolean]
-  def filtered_approvers(regular: true, code_owner: true, target: :approvers, unactioned: false)
+  def filtered_approvers(regular: true, code_owner: true, report_approver: true, target: :approvers, unactioned: false)
     rules = []
     rules.concat(regular_rules) if regular
     rules.concat(code_owner_rules) if code_owner
+    rules.concat(report_approver_rules) if report_approver
 
     users = rules.flat_map(&target)
     users.uniq!
@@ -202,6 +205,12 @@ class ApprovalState
     end
   end
 
+  def report_approver_rules
+    strong_memoize(:report_approver_rules) do
+      wrap_rules(merge_request.approval_rules.select(&:report_approver?))
+    end
+  end
+
   def wrap_rules(rules)
     rules.map { |rule| ApprovalWrappedRule.new(merge_request, rule) }
   end

ee/app/models/concerns/approval_rule_like.rb

@@ -4,6 +4,7 @@ module ApprovalRuleLike
   extend ActiveSupport::Concern
 
   DEFAULT_NAME = 'Default'
+  DEFAULT_NAME_FOR_SECURITY_REPORT = 'Vulnerability-Check'
   APPROVALS_REQUIRED_MAX = 100
 
   included do

ee/app/models/ee/ci/pipeline.rb

@@ -89,10 +89,11 @@ module EE
 
         state_machine :status do
           after_transition any => ::Ci::Pipeline.completed_statuses do |pipeline|
-            next unless pipeline.has_reports?(::Ci::JobArtifact.security_reports) && pipeline.default_branch?
+            next unless pipeline.has_reports?(::Ci::JobArtifact.security_reports)
 
             pipeline.run_after_commit do
-              StoreSecurityReportsWorker.perform_async(pipeline.id)
+              StoreSecurityReportsWorker.perform_async(pipeline.id) if pipeline.default_branch?
+              SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline.id)
             end
           end
 

ee/app/models/ee/merge_request.rb

@@ -90,7 +90,7 @@ module EE
       local_project_rule_ids.compact!
 
       invalid = if new_record?
-                  local_project_rule_ids.to_set != project.approval_rule_ids.to_set
+                  local_project_rule_ids.to_set != project.visible_regular_approval_rules.pluck(:id).to_set
                 else
                   (local_project_rule_ids - project.approval_rule_ids).present?
                 end

ee/app/models/ee/project.rb

@@ -322,6 +322,12 @@ module EE
       super
     end
 
+    def visible_approval_rules
+      strong_memoize(:visible_approval_rules) do
+        visible_regular_approval_rules + approval_rules.report_approver
+      end
+    end
+
     def visible_regular_approval_rules
       strong_memoize(:visible_regular_approval_rules) do
         regular_rules = approval_rules.regular.order(:id)

ee/app/models/license.rb

@@ -110,6 +110,7 @@ class License < ApplicationRecord
     insights
     web_ide_terminal
     incident_management
+    report_approver_rules
     group_ip_restriction
   ]
   EEU_FEATURES.freeze

ee/app/services/approval_rules/create_service.rb

@@ -5,6 +5,13 @@ module ApprovalRules
     # @param target [Project, MergeRequest]
     def initialize(target, user, params)
       @rule = target.approval_rules.build
+
+      # report_approver rule_type is currently auto-set according to rulename
+      # Techdebt to be addressed with: https://gitlab.com/gitlab-org/gitlab-ee/issues/12759
+      if target.is_a?(Project) && params[:name] == ApprovalProjectRule::DEFAULT_NAME_FOR_SECURITY_REPORT
+        params.reverse_merge!(rule_type: :report_approver)
+      end
+
       super(@rule.project, user, params)
     end
   end

ee/app/services/approval_rules/project_rule_destroy_service.rb

+# frozen_string_literal: true
+
+module ApprovalRules
+  class ProjectRuleDestroyService < ::BaseService
+    attr_reader :rule
+
+    def initialize(approval_rule)
+      @rule = approval_rule
+    end
+
+    def execute
+      ActiveRecord::Base.transaction do
+        # Removes only MR rules associated with project rule
+        remove_associated_approval_rules_from_unmerged_merge_requests
+
+        rule.destroy
+      end
+
+      if rule.destroyed?
+        success
+      else
+        error(rule.errors.messages)
+      end
+    end
+
+    private
+
+    def remove_associated_approval_rules_from_unmerged_merge_requests
+      ApprovalMergeRequestRule
+        .from_project_rule(rule)
+        .for_unmerged_merge_requests
+        .delete_all
+    end
+  end
+end

ee/app/services/ee/merge_requests/create_service.rb

@@ -10,6 +10,13 @@ module EE
         super
 
         ::MergeRequests::SyncCodeOwnerApprovalRules.new(issuable).execute
+        ::MergeRequests::SyncReportApproverApprovalRules.new(issuable).execute
+
+        # Attempt to sync reports if pipeline has finished before MR has been created
+        pipeline = issuable.find_actual_head_pipeline
+        if pipeline
+          ::SyncSecurityReportsToReportApprovalRulesWorker.perform_async(pipeline.id)
+        end
       end
     end
   end

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -38,12 +38,11 @@ module EE
       end
 
       def update_approvers
-        return yield unless project.feature_available?(:code_owners)
-
         results = yield
 
         merge_requests_for_source_branch.each do |merge_request|
-          ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute
+          ::MergeRequests::SyncCodeOwnerApprovalRules.new(merge_request).execute if project.feature_available?(:code_owners)
+          ::MergeRequests::SyncReportApproverApprovalRules.new(merge_request).execute if project.beta_feature_available?(:report_approver_rules)
         end
 
         results

ee/app/services/merge_requests/sync_report_approver_approval_rules.rb

+# frozen_string_literal: true
+
+module MergeRequests
+  class SyncReportApproverApprovalRules
+    def initialize(merge_request, params = {})
+      @merge_request = merge_request
+    end
+
+    def execute
+      if merge_request.target_project.beta_feature_available?(:report_approver_rules)
+        sync_rules
+      end
+    end
+
+    private
+
+    attr_reader :merge_request
+
+    def sync_rules
+      return if merge_request.merged?
+
+      sync_project_approval_rules_to_merge_request_rules
+    end
+
+    def sync_project_approval_rules_to_merge_request_rules
+      merge_request.target_project.approval_rules.report_approver.each do |project_rule|
+        merge_request.approval_rules.report_approver.first_or_initialize.tap do |rule|
+          rule.update(attributes_from(project_rule))
+        end
+      end
+    end
+
+    def attributes_from(project_rule)
+      project_rule.attributes
+       .slice('approvals_required', 'name')
+       .merge(
+         users: project_rule.users,
+         groups: project_rule.groups,
+         approval_project_rule: project_rule,
+         rule_type: :report_approver,
+         report_type: :security
+       )
+    end
+  end
+end

ee/app/services/security/sync_reports_to_approval_rules_service.rb

+# frozen_string_literal: true
+
+module Security
+  # Service for syncing security reports results to report_approver approval rules
+  #
+  class SyncReportsToApprovalRulesService < ::BaseService
+    def initialize(pipeline)
+      @pipeline = pipeline
+    end
+
+    def execute
+      reports = @pipeline.security_reports.reports
+
+      safe = reports.any? && reports.none? do |_report_type, report|
+        report.unsafe_severity?
+      end
+
+      return success unless safe
+
+      if remove_required_report_approvals(@pipeline.merge_requests_as_head_pipeline)
+        success
+      else
+        error("Failed to update approval rules")
+      end
+    end
+
+    private
+
+    def remove_required_report_approvals(merge_requests)
+      ApprovalMergeRequestRule
+        .security_report
+        .for_unmerged_merge_requests(merge_requests)
+        .update_all(approvals_required: 0)
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -43,6 +43,7 @@
 - geo:geo_scheduler_secondary_scheduler
 
 - pipeline_default:store_security_reports
+- pipeline_default:sync_security_reports_to_report_approval_rules
 - pipeline_default:ci_create_cross_project_pipeline
 - pipeline_default:ci_pipeline_bridge_status
 

ee/app/workers/sync_security_reports_to_report_approval_rules_worker.rb

+# frozen_string_literal: true
+
+# Worker for syncing report_type approval_rules approvals_required
+#
+class SyncSecurityReportsToReportApprovalRulesWorker
+  include ApplicationWorker
+  include PipelineQueue
+
+  def perform(pipeline_id)
+    pipeline = Ci::Pipeline.find_by_id(pipeline_id)
+    return unless pipeline
+
+    ::Security::SyncReportsToApprovalRulesService.new(pipeline).execute
+  end
+end

ee/changelogs/unreleased/9928-add-report_approver-to-approval_rules-services.yml

+---
+title: Enable security gates for merge requests
+merge_request: 13109
+author:
+type: added

ee/lib/api/project_approval_rules.rb

@@ -29,6 +29,7 @@ module API
           params do
             requires :name, type: String, desc: 'The name of the approval rule'
             requires :approvals_required, type: Integer, desc: 'The number of required approvals for this rule'
+            optional :rule_type, type: String, desc: 'The type of approval rule', default: 'regular'
             optional :users, as: :user_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The user ids for this rule'
             optional :groups, as: :group_ids, type: Array, coerce_with: ARRAY_COERCION_LAMBDA, desc: 'The group ids for this rule'
           end
@@ -81,9 +82,10 @@ module API
               authorize! :admin_project, user_project
 
               approval_rule = user_project.approval_rules.find(params[:approval_rule_id])
-              destroy_conditionally!(approval_rule)
 
-              no_content!
+              destroy_conditionally!(approval_rule) do |rule|
+                ::ApprovalRules::ProjectRuleDestroyService.new(rule).execute
+              end
             end
           end
         end

ee/lib/ee/api/entities.rb

@@ -346,7 +346,7 @@ module EE
 
       # Decorates Project
       class ProjectApprovalRules < Grape::Entity
-        expose :visible_regular_approval_rules, as: :rules, using: ApprovalRule
+        expose :visible_approval_rules, as: :rules, using: ApprovalRule
         expose :min_fallback_approvals, as: :fallback_approvals_required
       end
 

ee/lib/gitlab/ci/reports/security/report.rb

@@ -5,6 +5,8 @@ module Gitlab
     module Reports
       module Security
         class Report
+          UNSAFE_SEVERITIES = %w[unknown high critical].freeze
+
           attr_reader :type
           attr_reader :commit_sha
           attr_reader :occurrences
@@ -50,6 +52,10 @@ module Gitlab
           def merge!(other)
             replace_with!(::Security::MergeReportsService.new(self, other).execute)
           end
+
+          def unsafe_severity?
+            occurrences.any? { |occurrence| UNSAFE_SEVERITIES.include?(occurrence.severity) }
+          end
         end
       end
     end

ee/spec/factories/approval_rules.rb

@@ -23,5 +23,11 @@ FactoryBot.define do
   factory :approval_project_rule do
     project
     name ApprovalRuleLike::DEFAULT_NAME
+    rule_type :regular
+
+    trait :security_report do
+      rule_type :report_approver
+      name ApprovalRuleLike::DEFAULT_NAME_FOR_SECURITY_REPORT
+    end
   end
 end

ee/spec/models/approval_merge_request_rule_spec.rb

@@ -76,6 +76,7 @@ describe ApprovalMergeRequestRule do
     set(:js_rule) { create(:code_owner_rule, name: '*.js') }
     set(:css_rule) { create(:code_owner_rule, name: '*.css') }
     set(:approval_rule) { create(:approval_merge_request_rule) }
+    set(:report_approver_rule) { create(:report_approver_rule) }
 
     describe '.not_matching_pattern' do
       it 'returns the correct rules' do
@@ -97,6 +98,13 @@ describe ApprovalMergeRequestRule do
           .to contain_exactly(rb_rule, js_rule, css_rule)
       end
     end
+
+    describe '.security_report' do
+      it 'returns the correct rules' do
+        expect(described_class.security_report)
+          .to contain_exactly(report_approver_rule)
+      end
+    end
   end
 
   describe '.find_or_create_code_owner_rule' do
@@ -241,7 +249,7 @@ describe ApprovalMergeRequestRule do
     describe 'approvals_required' do
       subject { build(:approval_merge_request_rule, merge_request: merge_request) }
 
-      it 'is a natual number' do
+      it 'is a natural number' do
         subject.assign_attributes(approvals_required: 2)
         expect(subject).to be_valid
 
@@ -261,6 +269,22 @@ describe ApprovalMergeRequestRule do
 
           expect(subject.errors[:approvals_required]).to include("must be greater than or equal to 3")
         end
+
+        context 'when report_approver rule' do
+          subject do
+            build(:report_approver_rule, merge_request: merge_request, approvals_required: 1).tap do |rule|
+              rule.approval_project_rule = project_rule
+            end
+          end
+
+          it 'skips validation' do
+            expect(subject).to be_valid
+
+            subject.approvals_required = 0
+
+            expect(subject).to be_valid
+          end
+        end
       end
     end
   end

ee/spec/models/approval_project_rule_spec.rb

@@ -6,8 +6,9 @@ describe ApprovalProjectRule do
   subject { create(:approval_project_rule) }
 
   describe '.regular' do
-    it 'returns all records' do
+    it 'returns non-report_approver records' do
       rules = create_list(:approval_project_rule, 2)
+      create(:approval_project_rule, :security_report)
 
       expect(described_class.regular).to contain_exactly(*rules)
     end
@@ -21,23 +22,43 @@ describe ApprovalProjectRule do
     end
   end
 
-  describe '#regular' do
-    it 'returns true' do
-      expect(subject.regular).to eq(true)
+  describe '#regular?' do
+    let(:security_approver_rule) { build(:approval_project_rule, :security_report) }
+
+    it 'returns true for regular rules' do
       expect(subject.regular?).to eq(true)
     end
+
+    it 'returns false for report_approver rules' do
+      expect(security_approver_rule.regular?). to eq(false)
+    end
   end
 
-  describe '#code_owner' do
+  describe '#code_owner?' do
     it 'returns false' do
-      expect(subject.code_owner).to eq(false)
       expect(subject.code_owner?).to eq(false)
     end
   end
 
+  describe '#report_approver?' do
+    let(:security_approver_rule) { build(:approval_project_rule, :security_report) }
+
+    it 'returns false for regular rules' do
+      expect(subject.report_approver?).to eq(false)
+    end
+
+    it 'returns true for report_approver rules' do
+      expect(security_approver_rule.report_approver?). to eq(true)
+    end
+  end
+
   describe '#rule_type' do
-    it 'returns the correct type' do
+    it 'returns the regular type for regular rules' do
       expect(build(:approval_project_rule).rule_type).to eq('regular')
     end
+
+    it 'returns the report_approver type for security report approvers rules' do
+      expect(build(:approval_project_rule, :security_report).rule_type).to eq('report_approver')
+    end
   end
 end

ee/spec/models/approval_state_spec.rb

@@ -6,7 +6,12 @@ describe ApprovalState do
   def create_rule(additional_params = {})
     default_approver = create(:user)
     params = additional_params.reverse_merge(merge_request: merge_request, users: [default_approver])
-    factory = params.delete(:code_owner) ? :code_owner_rule : :approval_merge_request_rule
+    factory =
+      case params.delete(:rule_type)
+      when :code_owner then :code_owner_rule
+      when :report_approver then :report_approver_rule
+      else :approval_merge_request_rule
+      end
 
     create(factory, params)
   end
@@ -265,7 +270,16 @@ describe ApprovalState do
 
       context 'when only code owner rules present' do
         before do
-          2.times { create_rule(users: [create(:user)], code_owner: true) }
+          2.times { create_rule(users: [create(:user)], rule_type: :code_owner) }
+        end
+
+        it_behaves_like 'when rules are present'
+        it_behaves_like 'checking fallback_approvals_required'
+      end
+
+      context 'when only report approver rules present' do
+        before do
+          2.times { create_rule(users: [create(:user)], rule_type: :report_approver) }
         end
 
         it_behaves_like 'when rules are present'
@@ -380,13 +394,16 @@ describe ApprovalState do
     end
 
     describe '#filtered_approvers' do
-      describe 'only direct users, without code owners' do
+      describe 'only direct users, without code owners or report_approvers' do
         it 'includes only rule user members' do
           create_rule(users: [approver1])
           create_rule(users: [approver1], groups: [group1])
-          create_rule(users: [approver2], code_owner: true)
+          create_rule(users: [approver2], rule_type: :code_owner)
+          create_rule(users: [approver3], rule_type: :report_approver)
 
-          expect(subject.filtered_approvers(code_owner: false, target: :users)).to contain_exactly(approver1)
+          expect(
+            subject.filtered_approvers(code_owner: false, report_approver: false, target: :users)
+          ).to contain_exactly(approver1)
         end
       end
 
@@ -394,7 +411,17 @@ describe ApprovalState do
         it 'includes only code owners' do
           create_rule(users: [approver1])
           create_rule(users: [approver1], groups: [group1])
-          create_rule(users: [approver2], code_owner: true)
+          create_rule(users: [approver2], rule_type: :code_owner)
+
+          expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2)
+        end
+      end
+
+      describe 'only report approvers' do
+        it 'includes only report approvers' do
+          create_rule(users: [approver1])
+          create_rule(users: [approver1], groups: [group1])
+          create_rule(users: [approver2], rule_type: :report_approver)
 
           expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2)
         end
@@ -404,11 +431,12 @@ describe ApprovalState do
         it 'excludes approved approvers' do
           create_rule(users: [approver1])
           create_rule(users: [approver1], groups: [group1])
-          create_rule(users: [approver2], code_owner: true)
+          create_rule(users: [approver2], rule_type: :code_owner)
+          create_rule(users: [approver3], rule_type: :report_approver)
 
           create(:approval, merge_request: merge_request, user: approver1)
 
-          expect(subject.filtered_approvers(unactioned: true)).to contain_exactly(approver2, group_approver1)
+          expect(subject.filtered_approvers(unactioned: true)).to contain_exactly(approver2, approver3, group_approver1)
         end
       end
 
@@ -889,11 +917,13 @@ describe ApprovalState do
       rule1
       rule2
       code_owner_rule
+      report_approver_rule
     end
 
     let(:rule1) { create_unapproved_rule }
     let(:rule2) { create_unapproved_rule }
-    let(:code_owner_rule) { create_unapproved_rule(code_owner: true, approvals_required: 0) }
+    let(:code_owner_rule) { create_unapproved_rule(rule_type: :code_owner, approvals_required: 0) }
+    let(:report_approver_rule) { create_unapproved_rule(rule_type: :report_approver, approvals_required: 0) }
 
     before do
       stub_licensed_features multiple_approval_rules: false
@@ -907,7 +937,7 @@ describe ApprovalState do
           expect(rule.is_a?(ApprovalWrappedRule)).to eq(true)
         end
 
-        expect(subject.wrapped_approval_rules.size).to eq(2)
+        expect(subject.wrapped_approval_rules.size).to eq(3)
       end
     end
 
@@ -1021,7 +1051,16 @@ describe ApprovalState do
       context 'when only code owner rules present' do
         before do
           # setting approvals required to 0 since we don't want to block on them now
-          2.times { create_rule(users: [create(:user)], code_owner: true, approvals_required: 0) }
+          2.times { create_rule(users: [create(:user)], rule_type: :code_owner, approvals_required: 0) }
+        end
+
+        it_behaves_like 'when rules are present'
+        it_behaves_like 'checking fallback_approvals_required'
+      end
+
+      context 'when only report approver rules present' do
+        before do
+          2.times { create_rule(users: [create(:user)], rule_type: :report_approver) }
         end
 
         it_behaves_like 'when rules are present'
@@ -1114,11 +1153,12 @@ describe ApprovalState do
     end
 
     describe '#approvers' do
-      let(:code_owner_rule) { create_rule(code_owner: true, groups: [group1]) }
+      let(:code_owner_rule) { create_rule(rule_type: :code_owner, groups: [group1]) }
+      let(:report_approver_rule) { create_rule(rule_type: :report_approver, users: [approver2]) }
 
-      it 'includes approvers from first rule and code owner rule' do
+      it 'includes approvers from first rule, code owner rule, and report approver rule' do
         create_rules
-        approvers = rule1.users + code_owner_rule.users + [group_approver1]
+        approvers = rule1.users + code_owner_rule.users + [group_approver1, approver2]
 
         expect(subject.approvers).to contain_exactly(*approvers)
       end
@@ -1129,13 +1169,16 @@ describe ApprovalState do
     end
 
     describe '#filtered_approvers' do
-      describe 'only direct users, without code owners' do
+      describe 'only direct users, without code owners or report approvers' do
         it 'includes only rule user members' do
           create_rule(users: [approver1])
           create_rule(users: [approver1], groups: [group1])
-          create_rule(users: [approver2], code_owner: true)
+          create_rule(users: [approver2], rule_type: :code_owner)
+          create_rule(users: [approver3], rule_type: :report_approver)
 
-          expect(subject.filtered_approvers(code_owner: false, target: :users)).to contain_exactly(approver1)
+          expect(
+            subject.filtered_approvers(code_owner: false, report_approver: false, target: :users)
+          ).to contain_exactly(approver1)
         end
       end
 
@@ -1143,7 +1186,15 @@ describe ApprovalState do
         it 'includes only code owners' do
           create_rule(users: [approver1])
           create_rule(users: [approver1], groups: [group1])
-          create_rule(users: [approver2], code_owner: true)
+          create_rule(users: [approver2], rule_type: :code_owner)
+
+          expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2)
+        end
+
+        it 'includes only report approvers' do
+          create_rule(users: [approver1])
+          create_rule(users: [approver1], groups: [group1])
+          create_rule(users: [approver2], rule_type: :report_approver)
 
           expect(subject.filtered_approvers(regular: false)).to contain_exactly(approver2)
         end
@@ -1153,11 +1204,12 @@ describe ApprovalState do
         it 'excludes approved approvers' do
           create_rule(users: [approver1])
           create_rule(users: [approver1], groups: [group1])
-          create_rule(users: [approver2], code_owner: true)
+          create_rule(users: [approver2], rule_type: :code_owner)
+          create_rule(users: [approver3], rule_type: :report_approver)
 
           create(:approval, merge_request: merge_request, user: approver1)
 
-          expect(subject.filtered_approvers(unactioned: true)).to contain_exactly(approver2)
+          expect(subject.filtered_approvers(unactioned: true)).to contain_exactly(approver2, approver3)
         end
       end
 

ee/spec/models/merge_request_spec.rb

@@ -51,6 +51,10 @@ describe MergeRequest do
   end
 
   describe 'approval_rules' do
+    before do
+      stub_licensed_features(multiple_approval_rules: true)
+    end
+
     context 'when project contains approval_rules' do
       let!(:project_rule1) {  project.approval_rules.create(name: 'p1') }
       let!(:project_rule2) {  project.approval_rules.create(name: 'p2') }

ee/spec/requests/api/project_approval_rules_spec.rb

@@ -71,6 +71,21 @@ describe API::ProjectApprovalRules do
           expect(rule['groups'].size).to eq(1)
         end
       end
+
+      context 'report_approver rules' do
+        let!(:report_approver_rule) do
+          create(:approval_project_rule, :security_report, project: project)
+        end
+
+        it 'includes report_approver rules' do
+          get api(url, developer)
+
+          json = json_response
+
+          expect(json['rules'].size).to eq(2)
+          expect(json['rules'].map { |rule| rule['name'] }).to contain_exactly(rule.name, report_approver_rule.name)
+        end
+      end
     end
   end
 

ee/spec/services/approval_rules/create_service_spec.rb

@@ -82,6 +82,22 @@ describe ApprovalRules::CreateService do
     let(:target) { project }
 
     it_behaves_like "creatable"
+
+    context 'when name matches default for security reports' do
+      it 'sets rule_type as report_approver' do
+        result = described_class.new(target, user, {
+          name: ApprovalProjectRule::DEFAULT_NAME_FOR_SECURITY_REPORT,
+          approvals_required: 1
+        }).execute
+
+        expect(result[:status]).to eq(:success)
+
+        rule = result[:rule]
+
+        expect(rule.approvals_required).to eq(1)
+        expect(rule.rule_type).to eq('report_approver')
+      end
+    end
   end
 
   context 'when target is merge request' do

ee/spec/services/approval_rules/project_rule_destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ApprovalRules::ProjectRuleDestroyService do
+  let(:project) { create(:project, :repository) }
+  let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+
+  describe '#execute' do
+    let!(:project_rule) { create(:approval_project_rule, project: project) }
+
+    subject { described_class.new(project_rule) }
+
+    context 'when there is no merge request rules' do
+      it 'destroys project rule' do
+        expect { subject.execute }.to change { ApprovalProjectRule.count }.by(-1)
+      end
+    end
+
+    context 'when there is a merge request rule' do
+      let!(:merge_request_rule) do
+        create(:approval_merge_request_rule, merge_request: merge_request).tap do |rule|
+          rule.approval_project_rule = project_rule
+        end
+      end
+
+      context 'when open' do
+        it 'destroys merge request rules' do
+          expect { subject.execute }.to change { ApprovalMergeRequestRule.count }.by(-1)
+        end
+      end
+
+      context 'when merged' do
+        before do
+          merge_request.mark_as_merged!
+        end
+
+        it 'does nothing' do
+          expect { subject.execute }.not_to change { ApprovalMergeRequestRule.count }
+        end
+      end
+    end
+  end
+end

ee/spec/services/ee/merge_requests/create_service_spec.rb

@@ -34,6 +34,37 @@ describe MergeRequests::CreateService do
       service.execute
     end
 
+    context 'report approvers' do
+      let(:sha) { project.repository.commits(opts[:source_branch], limit: 1).first.id }
+      let(:pipeline) { instance_double(Ci::Pipeline, id: 42, project_id: project.id, triggered_by_merge_request?: true) }
+
+      it 'refreshes report approvers for the merge request' do
+        expect_next_instance_of(::MergeRequests::SyncReportApproverApprovalRules) do |service|
+          expect(service).to receive(:execute)
+        end
+
+        service.execute
+      end
+
+      it 'enqueues approval rule report syncing when pipeline exists' do
+        expect_next_instance_of(MergeRequest) do |merge_request|
+          allow(merge_request).to receive(:find_actual_head_pipeline).and_return(pipeline)
+          allow(merge_request).to receive(:update_head_pipeline).and_return(true)
+        end
+        expect(::SyncSecurityReportsToReportApprovalRulesWorker)
+          .to receive(:perform_async)
+
+        service.execute
+      end
+
+      it 'wont enqueue approval rule report syncing without pipeline' do
+        expect(::SyncSecurityReportsToReportApprovalRulesWorker)
+          .not_to receive(:perform_async)
+
+        service.execute
+      end
+    end
+
     it_behaves_like 'new issuable with scoped labels' do
       let(:parent) { project }
     end

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -44,11 +44,13 @@ describe MergeRequests::RefreshService do
       let(:current_user) { merge_request.author }
       let(:service) { described_class.new(project, current_user) }
       let(:enable_code_owner) { true }
+      let(:enable_report_approver_rules) { true }
       let(:todo_service) { double(:todo_service) }
       let(:notification_service) { double(:notification_service) }
 
       before do
         stub_licensed_features(code_owners: enable_code_owner)
+        stub_licensed_features(report_approver_rules: enable_report_approver_rules)
 
         allow(service).to receive(:mark_pending_todos_done)
         allow(service).to receive(:notify_about_push)
@@ -88,6 +90,20 @@ describe MergeRequests::RefreshService do
           subject
         end
       end
+
+      context 'when report_approver_rules enabled, with approval_rule enabled' do
+        let(:relevant_merge_requests) { [merge_request, another_merge_request] }
+
+        it 'refreshes the report_approver rules for all relevant merge requests' do
+          relevant_merge_requests.each do |merge_request|
+            expect_next_instance_of(::MergeRequests::SyncReportApproverApprovalRules, merge_request) do |service|
+              expect(service).to receive(:execute)
+            end
+          end
+
+          subject
+        end
+      end
     end
 
     describe 'Pipelines for merge requests' do

ee/spec/services/merge_requests/sync_report_approver_approval_rules_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::SyncReportApproverApprovalRules do
+  let(:merge_request) { create(:merge_request) }
+  let!(:security_approval_project_rule) { create(:approval_project_rule, :security_report, project: merge_request.target_project, approvals_required: 2) }
+
+  subject(:service) { described_class.new(merge_request) }
+
+  describe '#execute' do
+    context 'when report_approver_rules are enabled' do
+      let!(:regular_approval_project_rule) { create(:approval_project_rule, project: merge_request.target_project) }
+
+      before do
+        stub_feature_flags(report_approver_rules: true)
+      end
+
+      it 'creates rule for report approvers' do
+        expect { service.execute }
+          .to change { merge_request.approval_rules.security_report.count }.from(0).to(1)
+
+        rule = merge_request.approval_rules.security_report.first
+
+        expect(rule).to be_report_approver
+        expect(rule.report_type).to eq 'security'
+        expect(rule.name).to eq(security_approval_project_rule.name)
+        expect(rule.approval_project_rule).to eq(security_approval_project_rule)
+      end
+
+      it 'updates previous rules if defined' do
+        mr_rule = create(:report_approver_rule, merge_request: merge_request, approvals_required: 0)
+
+        expect { service.execute }
+          .not_to change { merge_request.approval_rules.security_report.count }
+
+        expect(mr_rule.reload).to be_report_approver
+        expect(mr_rule.report_type).to eq 'security'
+        expect(mr_rule.name).to eq(security_approval_project_rule.name)
+        expect(mr_rule.approvals_required).to eq security_approval_project_rule.approvals_required
+        expect(mr_rule.approval_project_rule).to eq(security_approval_project_rule)
+      end
+    end
+
+    context 'when report_approver_rules are disabled' do
+      before do
+        stub_feature_flags(report_approver_rules: false)
+      end
+
+      it 'copies nothing' do
+        expect { service.execute }
+          .not_to change { merge_request.approval_rules.count }
+      end
+    end
+  end
+end

ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Security::SyncReportsToApprovalRulesService, '#execute' do
+  let(:merge_request) { create(:merge_request) }
+  let(:project) { merge_request.project }
+  let(:pipeline) { create(:ee_ci_pipeline, :success, project: project, merge_requests_as_head_pipeline: [merge_request]) }
+  let(:report_approver_rule) { create(:report_approver_rule, merge_request: merge_request, approvals_required: 2) }
+
+  subject { described_class.new(pipeline).execute }
+
+  before do
+    allow(Ci::Pipeline).to receive(:find).with(pipeline.id) { pipeline }
+
+    stub_licensed_features(dependency_scanning: true, dast: true)
+  end
+
+  context 'when there are reports' do
+    context 'when pipeline passes' do
+      context 'when high-severity vulnerabilities are present' do
+        before do
+          create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: pipeline, project: project)
+        end
+
+        it "won't change approvals_required count" do
+          expect(
+            pipeline.security_reports.reports.values.all?(&:unsafe_severity?)
+          ).to be true
+
+          expect { subject }
+            .not_to change { report_approver_rule.reload.approvals_required }
+        end
+      end
+
+      context 'when only low-severity vulnerabilities are present' do
+        before do
+          create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project)
+        end
+
+        it 'lowers approvals_required count to zero' do
+          expect(
+            pipeline.security_reports.reports.values.none?(&:unsafe_severity?)
+          ).to be true
+
+          expect { subject }
+            .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
+        end
+      end
+
+      context 'when merge_requests are merged' do
+        let!(:merge_request) { create(:merge_request, :merged) }
+
+        before do
+          create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project)
+        end
+
+        it "won't change approvals_required count" do
+          expect(
+            pipeline.security_reports.reports.values.all?(&:unsafe_severity?)
+          ).to be false
+
+          expect { subject }
+            .not_to change { report_approver_rule.reload.approvals_required }
+        end
+      end
+    end
+
+    context 'when pipeline fails' do
+      before do
+        pipeline.update!(status: :failed)
+      end
+
+      context 'when high-severity vulnerabilities are present' do
+        before do
+          create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: pipeline, project: project)
+        end
+
+        it "won't change approvals_required count" do
+          expect(
+            pipeline.security_reports.reports.values.all?(&:unsafe_severity?)
+          ).to be true
+
+          expect { subject }
+            .not_to change { report_approver_rule.reload.approvals_required }
+        end
+      end
+
+      context 'when only low-severity vulnerabilities are present' do
+        before do
+          create(:ee_ci_build, :success, :dast, name: 'dast_job', pipeline: pipeline, project: project)
+        end
+
+        it 'lowers approvals_required count to zero' do
+          expect(
+            pipeline.security_reports.reports.values.none?(&:unsafe_severity?)
+          ).to be true
+
+          expect { subject }
+            .to change { report_approver_rule.reload.approvals_required }
+        end
+      end
+    end
+  end
+
+  context 'without reports' do
+    it "won't change approvals_required count" do
+      expect { subject }
+        .not_to change { report_approver_rule.reload.approvals_required }
+    end
+  end
+end

ee/spec/workers/sync_security_reports_to_report_approval_rules_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe SyncSecurityReportsToReportApprovalRulesWorker do
+  describe '#perform' do
+    let(:pipeline) { double(:pipeline, id: 42) }
+    let(:sync_service) { double(:service, execute: true) }
+
+    context 'when pipeline exists' do
+      before do
+        allow(Ci::Pipeline).to receive(:find_by_id).with(pipeline.id) { pipeline }
+      end
+
+      it "executes SyncReportsToApprovalRulesService for given pipeline" do
+        expect(Security::SyncReportsToApprovalRulesService).to receive(:new)
+          .with(pipeline).once.and_return(sync_service)
+
+        described_class.new.perform(pipeline.id)
+      end
+    end
+
+    context 'when pipeline is missing' do
+      it 'does not execute SyncReportsToApprovalRulesService' do
+        expect(Security::SyncReportsToApprovalRulesService).not_to receive(:new)
+
+        described_class.new.perform(pipeline.id)
+      end
+    end
+  end
+end