Merge branch 'add_scan_result_policies_into_sync_logic' into 'master'

Add project rules based on scan result policies

See merge request gitlab-org/gitlab!70633

ee/app/models/approval_merge_request_rule.rb

@@ -128,6 +128,10 @@ class ApprovalMergeRequestRule < ApplicationRecord
     refresh_license_scanning_approvals(project_approval_rule) if license_scanning?
   end
 
+  def self.remove_required_approved(approval_rules)
+    where(id: approval_rules).update_all(approvals_required: 0)
+  end
+
   private
 
   def compare_with_project_rule

ee/app/models/approval_project_rule.rb

@@ -24,6 +24,7 @@ class ApprovalProjectRule < ApplicationRecord
   }
 
   scope :report_approver_without_scan_finding, -> { report_approver.where.not(report_type: :scan_finding) }
+  scope :distinct_scanners, -> { scan_finding.select(:scanners).distinct }
 
   alias_method :code_owner, :code_owner?
   validate :validate_default_license_report_name, on: :update, if: :report_approver?

ee/app/models/concerns/security/scan_result_policy.rb

@@ -22,6 +22,13 @@ module Security
       def scan_result_policies
         policy_by_type(:scan_result_policy)
       end
+
+      def uniq_scanners
+        distinct_scanners = approval_rules.distinct_scanners
+        return [] if distinct_scanners.none?
+
+        distinct_scanners.pluck(:scanners).flatten.uniq
+      end
     end
   end
 end

ee/app/services/ci/sync_reports_to_approval_rules_service.rb

@@ -4,6 +4,18 @@ module Ci
   class SyncReportsToApprovalRulesService < ::BaseService
     include Gitlab::Utils::StrongMemoize
 
+    MEMOIZATIONS = %i(
+      policy_configuration
+      policy_rule_reports
+      policy_rule_scanners
+      project_rule_scanners
+      project_rule_severity_levels
+      project_rule_vulnerabilities_allowed
+      project_rule_vulnerability_states
+      project_vulnerability_report
+      reports
+    ).freeze
+
     def initialize(pipeline)
       @pipeline = pipeline
     end
@@ -12,6 +24,7 @@ module Ci
       sync_license_scanning_rules
       sync_vulnerability_rules
       sync_coverage_rules
+      sync_scan_finding
       success
     rescue StandardError => error
       payload = {
@@ -23,7 +36,7 @@ module Ci
       log_error(payload)
       error("Failed to update approval rules")
     ensure
-      [:project_rule_vulnerabilities_allowed, :project_rule_scanners, :project_rule_severity_levels, :project_vulnerability_report, :reports, :project_rule_vulnerability_states].each do |memoization|
+      MEMOIZATIONS.each do |memoization|
         clear_memoization(memoization)
       end
     end
@@ -57,18 +70,37 @@ module Ci
       remove_required_approvals_for(ApprovalMergeRequestRule.code_coverage, merge_requests_approved_coverage)
     end
 
+    def sync_scan_finding
+      return if ::Feature.disabled?(:scan_result_policy, pipeline.project)
+      return if policy_rule_reports.empty? && !pipeline.complete?
+
+      remove_required_approvals_for_scan_finding(pipeline.merge_requests_as_head_pipeline.opened)
+    end
+
     def reports
       strong_memoize(:reports) do
         project_rule_scanners ? pipeline.security_reports(report_types: project_rule_scanners) : []
       end
     end
 
+    def policy_rule_reports
+      strong_memoize(:policy_rule_reports) do
+        policy_rule_scanners ? pipeline.security_reports(report_types: policy_rule_scanners) : []
+      end
+    end
+
     def project_rule_scanners
       strong_memoize(:project_rule_scanners) do
         project_vulnerability_report&.scanners
       end
     end
 
+    def policy_rule_scanners
+      strong_memoize(:policy_rule_scanners) do
+        policy_configuration&.uniq_scanners
+      end
+    end
+
     def project_rule_vulnerabilities_allowed
       strong_memoize(:project_rule_vulnerabilities_allowed) do
         project_vulnerability_report&.vulnerabilities_allowed
@@ -96,6 +128,17 @@ module Ci
         .update_all(approvals_required: 0)
     end
 
+    def remove_required_approvals_for_scan_finding(merge_requests)
+      merge_requests.each do |merge_request|
+        base_reports = merge_request.base_pipeline&.security_reports
+        scan_finding_rules = merge_request.approval_rules.scan_finding
+        selected_rules = scan_finding_rules.reject do |rule|
+          violates_default_policy?(rule.source_rule, base_reports)
+        end
+        scan_finding_rules.remove_required_approved(selected_rules)
+      end
+    end
+
     def project_rule_severity_levels
       strong_memoize(:project_rule_severity_levels) do
         project_vulnerability_report&.severity_levels
@@ -113,5 +156,15 @@ module Ci
         pipeline.project.vulnerability_report_rule
       end
     end
+
+    def policy_configuration
+      strong_memoize(:policy_configuration) do
+        pipeline.project.security_orchestration_policy_configuration
+      end
+    end
+
+    def violates_default_policy?(source_rule, base_reports)
+      policy_rule_reports.violates_default_policy_against?(base_reports, source_rule.vulnerabilities_allowed, source_rule.severity_levels, source_rule.vulnerability_states_for_branch, source_rule.scanners)
+    end
   end
 end

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

@@ -11,11 +11,11 @@ module EE
             private
 
             override :unsafe_findings_count
-            def unsafe_findings_count(target_reports, severity_levels, vulnerability_states)
-              pipeline_uuids = unsafe_findings_uuids(severity_levels)
+            def unsafe_findings_count(target_reports, severity_levels, vulnerability_states, report_types)
+              pipeline_uuids = unsafe_findings_uuids(severity_levels, report_types)
               pipeline_count = count_by_uuid(pipeline_uuids, vulnerability_states)
 
-              new_uuids = pipeline_uuids - target_reports&.unsafe_findings_uuids(severity_levels).to_a
+              new_uuids = pipeline_uuids - target_reports&.unsafe_findings_uuids(severity_levels, report_types).to_a
 
               if vulnerability_states.include?(ApprovalProjectRule::NEWLY_DETECTED)
                 pipeline_count += new_uuids.count

ee/spec/factories/approval_rules.rb

@@ -42,6 +42,11 @@ FactoryBot.define do
       name { ApprovalRuleLike::DEFAULT_NAME_FOR_COVERAGE }
       report_type { :code_coverage }
     end
+
+    trait :scan_finding do
+      sequence(:name) { |n| "Scan finding #{n}" }
+      report_type { :scan_finding }
+    end
   end
 
   factory :any_approver_rule, parent: :approval_merge_request_rule do

ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb

@@ -185,19 +185,20 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
   end
 
   describe '#unsafe?' do
-    where(:severity, :levels, :unsafe?) do
-      'critical' | %w(critical high) | true
-      'high'     | %w(critical high) | true
-      'medium'   | %w(critical high) | false
-      'low'      | %w(critical high) | false
-      'info'     | %w(critical high) | false
-      'unknown'  | []                | false
+    where(:severity, :levels, :report_types, :unsafe?) do
+      'critical' | %w(critical high) | %w(dast)               | true
+      'high'     | %w(critical high) | %w(dast sast)          | true
+      'high'     | %w(critical high) | %w(container_scanning) | false
+      'medium'   | %w(critical high) | %w(dast)               | false
+      'low'      | %w(critical high) | %w(dast)               | false
+      'info'     | %w(critical high) | %w(dast)               | false
+      'unknown'  | []                | %w(dast)               | false
     end
 
     with_them do
-      let(:finding) { create(:ci_reports_security_finding, severity: severity) }
+      let(:finding) { create(:ci_reports_security_finding, severity: severity, report_type: 'dast') }
 
-      subject { finding.unsafe?(levels) }
+      subject { finding.unsafe?(levels, report_types) }
 
       it { is_expected.to be(unsafe?) }
     end

ee/spec/models/approval_project_rule_spec.rb

@@ -303,4 +303,44 @@ RSpec.describe ApprovalProjectRule do
       end
     end
   end
+
+  describe '.distinct_scanners scope' do
+    subject { described_class.distinct_scanners }
+
+    before do
+      create(:approval_project_rule, type, scanners: ['dast'])
+    end
+
+    context 'with scan_finding approval rules' do
+      let(:type) { :scan_finding }
+
+      it { is_expected.to be_present }
+
+      context 'with duplicated scanners' do
+        before do
+          create(:approval_project_rule, :scan_finding, scanners: ['dast'])
+        end
+
+        it 'returns only one record' do
+          expect(subject.count).to be 1
+        end
+      end
+
+      context 'without duplicated scanners' do
+        before do
+          create(:approval_project_rule, :scan_finding, scanners: ['sast'])
+        end
+
+        it 'returns both records' do
+          expect(subject.count).to be 2
+        end
+      end
+    end
+
+    context 'without scan_finding approval rules' do
+      let(:type) { :vulnerability }
+
+      it { is_expected.to be_empty }
+    end
+  end
 end

ee/spec/models/security/orchestration_policy_configuration_spec.rb

@@ -380,4 +380,23 @@ RSpec.describe Security::OrchestrationPolicyConfiguration do
       expect(scan_result_policies.pluck(:enabled)).to contain_exactly(true, true, false, true, true, true, true, true)
     end
   end
+
+  describe '#uniq_scanners' do
+    let(:project) { security_orchestration_policy_configuration.project }
+
+    subject { security_orchestration_policy_configuration.uniq_scanners }
+
+    context 'with approval rules' do
+      before do
+        create(:approval_project_rule, :scan_finding, scanners: %w(dast sast), project: project)
+        create(:approval_project_rule, :scan_finding, scanners: %w(dast container_scanning), project: project)
+      end
+
+      it { is_expected.to contain_exactly('dast', 'sast', 'container_scanning') }
+    end
+
+    context 'without approval rules' do
+      it { is_expected.to be_empty }
+    end
+  end
 end

ee/spec/services/ci/sync_reports_to_approval_rules_service_spec.rb

@@ -8,6 +8,10 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
   let(:merge_request) { create(:merge_request, source_project: project) }
   let(:pipeline) { create(:ee_ci_pipeline, :success, project: project, merge_requests_as_head_pipeline: [merge_request]) }
   let(:base_pipeline) { create(:ee_ci_pipeline, :success, project: project, ref: merge_request.target_branch, sha: merge_request.diff_base_sha) }
+  let(:scanners) { %w[dependency_scanning] }
+  let(:vulnerabilities_allowed) { 0 }
+  let(:severity_levels) { %w[high unknown] }
+  let(:vulnerability_states) { %w(newly_detected) }
 
   subject(:sync_rules) { described_class.new(pipeline).execute }
 
@@ -17,27 +21,17 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
     stub_licensed_features(dependency_scanning: true, dast: true, license_scanning: true)
   end
 
-  context 'with security rules' do
-    let(:report_approver_rule) { create(:report_approver_rule, merge_request: merge_request, approvals_required: 2) }
-    let(:scanners) { %w[dependency_scanning] }
-    let(:vulnerabilities_allowed) { 0 }
-    let(:severity_levels) { %w[high unknown] }
-    let(:vulnerability_states) { %w(newly_detected) }
-
-    before do
-      create(:approval_project_rule, :vulnerability, project: project, approvals_required: 2, scanners: scanners, vulnerabilities_allowed: vulnerabilities_allowed, severity_levels: severity_levels, vulnerability_states: vulnerability_states)
-    end
-
+  shared_context 'security reports with vulnerabilities' do
     context 'when there are security 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)
+            create(:ee_ci_build, :success, :dependency_scanning, :coverage, name: 'ds_job', pipeline: pipeline, project: project)
           end
 
           context 'when high-severity vulnerabilities already present in target branch pipeline' do
             before do
-              create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: base_pipeline, project: project)
+              create(:ee_ci_build, :success, :dependency_scanning, :coverage, name: 'ds_job', pipeline: base_pipeline, project: project)
             end
 
             it 'lowers approvals_required count to zero' do
@@ -161,12 +155,12 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' 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)
+            create(:ee_ci_build, :success, :dependency_scanning, :coverage, name: 'ds_job', pipeline: pipeline, project: project)
           end
 
           context 'when high-severity vulnerabilities already present in target branch pipeline' do
             before do
-              create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: base_pipeline, project: project)
+              create(:ee_ci_build, :success, :dependency_scanning, :coverage, name: 'ds_job', pipeline: base_pipeline, project: project)
             end
 
             it 'lowers approvals_required count to zero' do
@@ -215,6 +209,16 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
     end
   end
 
+  context 'with security rules' do
+    let(:report_approver_rule) { create(:report_approver_rule, merge_request: merge_request, approvals_required: 2) }
+
+    before do
+      create(:approval_project_rule, :vulnerability, project: project, approvals_required: 2, scanners: scanners, vulnerabilities_allowed: vulnerabilities_allowed, severity_levels: severity_levels, vulnerability_states: vulnerability_states)
+    end
+
+    include_context 'security reports with vulnerabilities'
+  end
+
   context 'with code coverage rules' do
     let!(:head_pipeline_builds) do
       [
@@ -315,4 +319,43 @@ RSpec.describe Ci::SyncReportsToApprovalRulesService, '#execute' do
       end
     end
   end
+
+  context 'with security orchestration rules' do
+    let(:report_approver_rule) { create(:report_approver_rule, :scan_finding, merge_request: merge_request, approvals_required: 2) }
+    let(:approval_project_rule) { create(:approval_project_rule, :scan_finding, project: project, approvals_required: 2, scanners: scanners, vulnerabilities_allowed: vulnerabilities_allowed, severity_levels: severity_levels, vulnerability_states: vulnerability_states) }
+    let!(:security_orchestration_policy_configuration) { create(:security_orchestration_policy_configuration, project: project) }
+
+    before do
+      create(:approval_merge_request_rule_source, approval_merge_request_rule: report_approver_rule, approval_project_rule: approval_project_rule)
+    end
+
+    context 'when there are security reports' do
+      context 'when pipeline passes' do
+        context 'when new vulnerabilities are present' do
+          before do
+            create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: pipeline, project: project)
+          end
+
+          context 'when only existing vulnerabilities are present' do
+            before do
+              create(:ee_ci_build, :success, :dependency_scanning, name: 'ds_job', pipeline: base_pipeline, project: project)
+            end
+
+            context 'with feature flag disabled' do
+              before do
+                stub_feature_flags(scan_result_policy: false)
+              end
+
+              it "won't change approval_required count" do
+                expect { subject }
+                .not_to change { report_approver_rule.reload.approvals_required }
+              end
+            end
+          end
+        end
+      end
+    end
+
+    include_context 'security reports with vulnerabilities'
+  end
 end

lib/gitlab/ci/reports/security/finding.rb

@@ -88,8 +88,8 @@ module Gitlab
             @location = new_location
           end
 
-          def unsafe?(severity_levels)
-            severity.in?(severity_levels)
+          def unsafe?(severity_levels, report_types)
+            severity.to_s.in?(severity_levels) && (report_types.blank? || report_type.to_s.in?(report_types) )
           end
 
           def eql?(other)

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

@@ -22,18 +22,18 @@ module Gitlab
             reports.values.flat_map(&:findings)
           end
 
-          def violates_default_policy_against?(target_reports, vulnerabilities_allowed, severity_levels, vulnerability_states)
-            unsafe_findings_count(target_reports, severity_levels, vulnerability_states) > vulnerabilities_allowed
+          def violates_default_policy_against?(target_reports, vulnerabilities_allowed, severity_levels, vulnerability_states, report_types = [])
+            unsafe_findings_count(target_reports, severity_levels, vulnerability_states, report_types) > vulnerabilities_allowed
           end
 
-          def unsafe_findings_uuids(severity_levels)
-            findings.select { |finding| finding.unsafe?(severity_levels) }.map(&:uuid)
+          def unsafe_findings_uuids(severity_levels, report_types)
+            findings.select { |finding| finding.unsafe?(severity_levels, report_types) }.map(&:uuid)
           end
 
           private
 
-          def unsafe_findings_count(target_reports, severity_levels, vulnerability_states)
-            new_uuids = unsafe_findings_uuids(severity_levels) - target_reports&.unsafe_findings_uuids(severity_levels).to_a
+          def unsafe_findings_count(target_reports, severity_levels, vulnerability_states, report_types)
+            new_uuids = unsafe_findings_uuids(severity_levels, report_types) - target_reports&.unsafe_findings_uuids(severity_levels, report_types).to_a
             new_uuids.count
           end
         end

spec/lib/gitlab/ci/reports/security/reports_spec.rb

@@ -54,7 +54,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Reports do
   end
 
   describe "#violates_default_policy_against?" do
-    let(:high_severity_dast) { build(:ci_reports_security_finding, severity: 'high', report_type: :dast) }
+    let(:high_severity_dast) { build(:ci_reports_security_finding, severity: 'high', report_type: 'dast') }
     let(:vulnerabilities_allowed) { 0 }
     let(:severity_levels) { %w(critical high) }
     let(:vulnerability_states) { %w(newly_detected)}
@@ -109,6 +109,22 @@ RSpec.describe Gitlab::Ci::Reports::Security::Reports do
 
         it { is_expected.to be(false) }
       end
+
+      context 'with related report_types' do
+        let(:report_types) { %w(dast sast) }
+
+        subject { security_reports.violates_default_policy_against?(target_reports, vulnerabilities_allowed, severity_levels, vulnerability_states, report_types) }
+
+        it { is_expected.to be(true) }
+      end
+
+      context 'with unrelated report_types' do
+        let(:report_types) { %w(dependency_scanning sast) }
+
+        subject { security_reports.violates_default_policy_against?(target_reports, vulnerabilities_allowed, severity_levels, vulnerability_states, report_types) }
+
+        it { is_expected.to be(false) }
+      end
     end
   end
 end