Use new unsafe findings to update MR approval rule

Instead of checking the unsafe findings in security reports, we are now
checking the new introduced unsafe findings to update the MR approval
rules.

ee/app/models/vulnerability.rb

@@ -114,7 +114,7 @@ class Vulnerability < ApplicationRecord
     end
 
     def active_state_values
-      states.values_at(*ACTIVE_STATES)
+      states.values_at(*active_states)
     end
   end
 

ee/app/services/security/sync_reports_to_approval_rules_service.rb

@@ -33,22 +33,31 @@ module Security
       return if report.empty? && !pipeline.complete?
       return if report.violates?(project.software_license_policies)
 
-      remove_required_approvals_for(ApprovalMergeRequestRule.report_approver.license_scanning)
+      remove_required_approvals_for(ApprovalMergeRequestRule.report_approver.license_scanning,
+                                    pipeline.merge_requests_as_head_pipeline)
     end
 
     def sync_vulnerability_rules
-      reports = pipeline.security_reports
       # If we have some reports, then we want to sync them early;
       # If we don't have reports, then we should wait until pipeline stops.
       return if reports.empty? && !pipeline.complete?
-      return if reports.violates_default_policy?
 
-      remove_required_approvals_for(ApprovalMergeRequestRule.security_report)
+      remove_required_approvals_for(ApprovalMergeRequestRule.security_report, sync_required_merge_requests)
     end
 
-    def remove_required_approvals_for(rules)
+    def reports
+      @reports ||= pipeline.security_reports
+    end
+
+    def sync_required_merge_requests
+      pipeline.merge_requests_as_head_pipeline.reject do |merge_request|
+        reports.violates_default_policy_against?(merge_request.base_pipeline&.security_reports)
+      end
+    end
+
+    def remove_required_approvals_for(rules, merge_requests)
       rules
-        .for_unmerged_merge_requests(pipeline.merge_requests_as_head_pipeline)
+        .for_unmerged_merge_requests(merge_requests)
         .update_all(approvals_required: 0)
     end
   end

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

@@ -5,6 +5,8 @@ module Gitlab
     module Reports
       module Security
         class Finding
+          UNSAFE_SEVERITIES = %w[unknown high critical].freeze
+
           attr_reader :compare_key
           attr_reader :confidence
           attr_reader :identifiers
@@ -65,6 +67,18 @@ module Gitlab
             @location = new_location
           end
 
+          def unsafe?
+            severity.in?(UNSAFE_SEVERITIES)
+          end
+
+          def eql?(other)
+            hash == other.hash
+          end
+
+          def hash
+            report_type.hash ^ location.fingerprint.hash ^ primary_identifier.fingerprint.hash
+          end
+
           private
 
           def generate_project_fingerprint

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

@@ -5,8 +5,6 @@ module Gitlab
     module Reports
       module Security
         class Report
-          UNSAFE_SEVERITIES = %w[unknown high critical].freeze
-
           attr_reader :created_at
           attr_reader :type
           attr_reader :pipeline
@@ -60,15 +58,6 @@ module Gitlab
           def merge!(other)
             replace_with!(::Security::MergeReportsService.new(self, other).execute)
           end
-
-          def unsafe_severity?
-            !safe?
-          end
-
-          def safe?
-            severities = findings.map(&:severity).compact.map(&:downcase)
-            (severities & UNSAFE_SEVERITIES).empty?
-          end
         end
       end
     end

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

@@ -18,8 +18,18 @@ module Gitlab
             reports[report_type] ||= Report.new(report_type, pipeline, report_artifact.created_at)
           end
 
-          def violates_default_policy?
-            reports.values.any? { |report| report.unsafe_severity? }
+          def findings
+            reports.values.flat_map(&:findings)
+          end
+
+          def violates_default_policy_against?(target_reports)
+            findings_diff(target_reports).any?(&:unsafe?)
+          end
+
+          private
+
+          def findings_diff(target_reports)
+            findings - target_reports&.findings.to_a
           end
         end
       end

ee/spec/factories/vulnerabilities/findings.rb

@@ -71,12 +71,18 @@ FactoryBot.define do
       }.to_json
     end
 
-    trait :confirmed do
+    trait :detected do
       after(:create) do |finding|
         create(:vulnerability, :detected, project: finding.project, findings: [finding])
       end
     end
 
+    trait :confirmed do
+      after(:create) do |finding|
+        create(:vulnerability, :confirmed, project: finding.project, findings: [finding])
+      end
+    end
+
     trait :resolved do
       after(:create) do |finding|
         create(:vulnerability, :resolved, project: finding.project, findings: [finding])
@@ -85,6 +91,7 @@ FactoryBot.define do
 
     trait :dismissed do
       after(:create) do |finding|
+        create(:vulnerability, :dismissed, project: finding.project, findings: [finding])
         create(:vulnerability_feedback,
                :dismissal,
                project: finding.project,

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

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Reports::Security::Finding do
+  using RSpec::Parameterized::TableSyntax
+
   describe '#initialize' do
     subject { described_class.new(**params) }
 
@@ -128,4 +130,102 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
       expect(occurrence.old_location).to eq(old_location)
     end
   end
+
+  describe '#unsafe?' do
+    where(:severity, :unsafe?) do
+      'critical' | true
+      'high'     | true
+      'medium'   | false
+      'low'      | false
+      'info'     | false
+      'unknown'  | true
+    end
+
+    with_them do
+      let(:finding) { create(:ci_reports_security_finding, severity: severity) }
+
+      subject { finding.unsafe? }
+
+      it { is_expected.to be(unsafe?) }
+    end
+  end
+
+  describe '#eql?' do
+    let(:identifier) { build(:ci_reports_security_identifier) }
+    let(:location) { build(:ci_reports_security_locations_sast) }
+    let(:finding) { build(:ci_reports_security_finding, severity: 'low', report_type: :sast, identifiers: [identifier], location: location) }
+
+    let(:report_type) { :secret_detection }
+    let(:identifier_external_id) { 'foo' }
+    let(:location_start_line) { 0 }
+    let(:other_identifier) { build(:ci_reports_security_identifier, external_id: identifier_external_id) }
+    let(:other_location) { build(:ci_reports_security_locations_sast, start_line: location_start_line) }
+    let(:other_finding) do
+      build(:ci_reports_security_finding,
+            severity: 'low',
+            report_type: report_type,
+            identifiers: [other_identifier],
+            location: other_location)
+    end
+
+    subject { finding.eql?(other_finding) }
+
+    context 'when the other finding has same `report_type`' do
+      let(:report_type) { :sast }
+
+      context 'when the other finding has same primary identifier fingerprint' do
+        let(:identifier_external_id) { identifier.external_id }
+
+        context 'when the other finding has same location fingerprint' do
+          let(:location_start_line) { location.start_line }
+
+          it { is_expected.to be(true) }
+        end
+
+        context 'when the other finding does not have same location fingerprint' do
+          it { is_expected.to be(false) }
+        end
+      end
+
+      context 'when the other finding does not have same primary identifier fingerprint' do
+        context 'when the other finding has same location fingerprint' do
+          let(:location_start_line) { location.start_line }
+
+          it { is_expected.to be(false) }
+        end
+
+        context 'when the other finding does not have same location fingerprint' do
+          it { is_expected.to be(false) }
+        end
+      end
+    end
+
+    context 'when the other finding does not have same `report_type`' do
+      context 'when the other finding has same primary identifier fingerprint' do
+        let(:identifier_external_id) { identifier.external_id }
+
+        context 'when the other finding has same location fingerprint' do
+          let(:location_start_line) { location.start_line }
+
+          it { is_expected.to be(false) }
+        end
+
+        context 'when the other finding does not have same location fingerprint' do
+          it { is_expected.to be(false) }
+        end
+      end
+
+      context 'when the other finding does not have same primary identifier fingerprint' do
+        context 'when the other finding has same location fingerprint' do
+          let(:location_start_line) { location.start_line }
+
+          it { is_expected.to be(false) }
+        end
+
+        context 'when the other finding does not have same location fingerprint' do
+          it { is_expected.to be(false) }
+        end
+      end
+    end
+  end
 end

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

@@ -3,8 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Reports::Security::Report do
-  let(:report) { described_class.new('sast', commit_sha, created_at) }
-  let(:commit_sha) { "d8978e74745e18ce44d88814004d4255ac6a65bb" }
+  let_it_be(:pipeline) { create(:ci_pipeline) }
+
+  let(:report) { described_class.new('sast', pipeline, created_at) }
   let(:created_at) { 2.weeks.ago }
 
   it { expect(report.type).to eq('sast') }
@@ -61,11 +62,11 @@ RSpec.describe Gitlab::Ci::Reports::Security::Report do
       )
     end
 
-    it 'creates a blank report with copied type and commit SHA' do
+    it 'creates a blank report with copied type and pipeline' do
       clone = report.clone_as_blank
 
       expect(clone.type).to eq(report.type)
-      expect(clone.commit_sha).to eq(report.commit_sha)
+      expect(clone.pipeline).to eq(report.pipeline)
       expect(clone.created_at).to eq(report.created_at)
       expect(clone.findings).to eq([])
       expect(clone.scanners).to eq({})
@@ -114,7 +115,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Report do
       allow(report).to receive(:replace_with!)
     end
 
-    subject { report.merge!(described_class.new('sast', commit_sha, created_at)) }
+    subject { report.merge!(described_class.new('sast', pipeline, created_at)) }
 
     it 'invokes the merge with other report and then replaces this report contents by merge result' do
       subject
@@ -122,63 +123,4 @@ RSpec.describe Gitlab::Ci::Reports::Security::Report do
       expect(report).to have_received(:replace_with!).with(merged_report)
     end
   end
-
-  describe "#safe?" do
-    subject { described_class.new('sast', commit_sha, created_at) }
-
-    context "when the sast report has an unsafe vulnerability" do
-      where(severity: %w[unknown Unknown high High critical Critical])
-      with_them do
-        let(:finding) { build(:ci_reports_security_finding, severity: severity) }
-
-        before do
-          subject.add_finding(finding)
-        end
-
-        it { expect(subject.unsafe_severity?).to be(true) }
-        it { expect(subject).not_to be_safe }
-      end
-    end
-
-    context "when the sast report has a medium to low severity vulnerability" do
-      where(severity: %w[medium Medium low Low])
-      with_them do
-        let(:finding) { build(:ci_reports_security_finding, severity: severity) }
-
-        before do
-          subject.add_finding(finding)
-        end
-
-        it { expect(subject.unsafe_severity?).to be(false) }
-        it { expect(subject).to be_safe }
-      end
-    end
-
-    context "when the sast report has a vulnerability with a `nil` severity" do
-      let(:finding) { build(:ci_reports_security_finding, severity: nil) }
-
-      before do
-        subject.add_finding(finding)
-      end
-
-      it { expect(subject.unsafe_severity?).to be(false) }
-      it { expect(subject).to be_safe }
-    end
-
-    context "when the sast report has a vulnerability with a blank severity" do
-      let(:finding) { build(:ci_reports_security_finding, severity: '') }
-
-      before do
-        subject.add_finding(finding)
-      end
-
-      it { expect(subject.unsafe_severity?).to be(false) }
-      it { expect(subject).to be_safe }
-    end
-
-    context "when the sast report has zero vulnerabilities" do
-      it { expect(subject.unsafe_severity?).to be(false) }
-      it { expect(subject).to be_safe }
-    end
-  end
 end

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

@@ -38,29 +38,69 @@ RSpec.describe Gitlab::Ci::Reports::Security::Reports do
     end
   end
 
-  describe "#violates_default_policy?" do
-    subject { described_class.new(pipeline) }
+  describe '#findings' do
+    let(:finding_1) { build(:ci_reports_security_finding, severity: 'low') }
+    let(:finding_2) { build(:ci_reports_security_finding, severity: 'high') }
+    let!(:expected_findings) { [finding_1, finding_2] }
 
-    let(:low_severity) { build(:ci_reports_security_finding, severity: 'low') }
-    let(:high_severity) { build(:ci_reports_security_finding, severity: 'high') }
+    subject { security_reports.findings }
 
-    context "when a report has a high severity vulnerability" do
-      before do
-        subject.get_report('sast', artifact).add_finding(high_severity)
-        subject.get_report('dependency_scanning', artifact).add_finding(low_severity)
+    before do
+      security_reports.get_report('sast', artifact).add_finding(finding_1)
+      security_reports.get_report('dependency_scanning', artifact).add_finding(finding_2)
+    end
+
+    it { is_expected.to match_array(expected_findings) }
+  end
+
+  describe "#violates_default_policy_against?" do
+    let(:low_severity_sast) { build(:ci_reports_security_finding, severity: 'low', report_type: :sast) }
+    let(:high_severity_dast) { build(:ci_reports_security_finding, severity: 'high', report_type: :dast) }
+
+    subject { security_reports.violates_default_policy_against?(target_reports) }
+
+    context 'when the target_reports is `nil`' do
+      let(:target_reports) { nil }
+
+      context "when a report has unsafe vulnerability" do
+        before do
+          security_reports.get_report('sast', artifact).add_finding(high_severity_dast)
+        end
+
+        it { is_expected.to be(true) }
       end
 
-      it { expect(subject.violates_default_policy?).to be(true) }
+      context "when none of the reports have an unsafe vulnerability" do
+        before do
+          security_reports.get_report('sast', artifact).add_finding(low_severity_sast)
+        end
+
+        it { is_expected.to be(false) }
+      end
     end
 
-    context "when none of the reports have a high severity vulnerability" do
-      before do
-        subject.get_report('sast', artifact).add_finding(low_severity)
-        subject.get_report('sast', artifact).add_finding(low_severity)
-        subject.get_report('dependency_scanning', artifact).add_finding(low_severity)
+    context 'when the target_reports is not `nil`' do
+      let(:target_reports) { described_class.new(pipeline) }
+
+      context "when a report has a new unsafe vulnerability" do
+        before do
+          security_reports.get_report('sast', artifact).add_finding(high_severity_dast)
+          security_reports.get_report('dependency_scanning', artifact).add_finding(low_severity_sast)
+          target_reports.get_report('dependency_scanning', artifact).add_finding(low_severity_sast)
+        end
+
+        it { is_expected.to be(true) }
       end
 
-      it { expect(subject.violates_default_policy?).to be(false) }
+      context "when none of the reports have a new unsafe vulnerability" do
+        before do
+          security_reports.get_report('sast', artifact).add_finding(high_severity_dast)
+          security_reports.get_report('sast', artifact).add_finding(low_severity_sast)
+          target_reports.get_report('sast', artifact).add_finding(high_severity_dast)
+        end
+
+        it { is_expected.to be(false) }
+      end
     end
   end
 end

ee/spec/services/security/sync_reports_to_approval_rules_service_spec.rb

@@ -7,6 +7,7 @@ RSpec.describe Security::SyncReportsToApprovalRulesService, '#execute' do
   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) }
+  let(:base_pipeline) { create(:ee_ci_pipeline, :success, project: project, ref: merge_request.target_branch, sha: merge_request.diff_base_sha) }
 
   subject { described_class.new(pipeline).execute }
 
@@ -23,13 +24,22 @@ RSpec.describe Security::SyncReportsToApprovalRulesService, '#execute' 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
+        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)
+          end
 
-          expect { subject }
-            .not_to change { report_approver_rule.reload.approvals_required }
+          it 'lowers approvals_required count to zero' do
+            expect { subject }
+              .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
+          end
+        end
+
+        context 'when high-severity vulnerabilities do not present in target branch pipeline' do
+          it "won't change approvals_required count" do
+            expect { subject }
+              .not_to change { report_approver_rule.reload.approvals_required }
+          end
         end
       end
 
@@ -39,10 +49,6 @@ RSpec.describe Security::SyncReportsToApprovalRulesService, '#execute' do
         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
@@ -131,13 +137,22 @@ RSpec.describe Security::SyncReportsToApprovalRulesService, '#execute' 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
+        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)
+          end
 
-          expect { subject }
-            .not_to change { report_approver_rule.reload.approvals_required }
+          it 'lowers approvals_required count to zero' do
+            expect { subject }
+              .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
+          end
+        end
+
+        context 'when high-severity vulnerabilities do not present in target branch pipeline' do
+          it "won't change approvals_required count" do
+            expect { subject }
+              .not_to change { report_approver_rule.reload.approvals_required }
+          end
         end
       end
 
@@ -147,12 +162,8 @@ RSpec.describe Security::SyncReportsToApprovalRulesService, '#execute' do
         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 }
+            .to change { report_approver_rule.reload.approvals_required }.from(2).to(0)
         end
       end
     end