Merge branch '222643_use_only_new_vulnerabilities_on_approval_rules' into 'master'

Fix `Vulnerability-Check` approval rule logic

See merge request gitlab-org/gitlab!38589

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

@@ -102,7 +102,7 @@ module EE
       end
 
       def security_reports
-        ::Gitlab::Ci::Reports::Security::Reports.new(sha).tap do |security_reports|
+        ::Gitlab::Ci::Reports::Security::Reports.new(self).tap do |security_reports|
           builds.latest.with_reports(::Ci::JobArtifact.security_reports).each do |build|
             build.collect_security_reports!(security_reports)
           end

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/merge_reports_service.rb

@@ -31,7 +31,7 @@ module Security
       sort_by_ds_analyzers!
       @target_report = ::Gitlab::Ci::Reports::Security::Report.new(
         @source_reports.first.type,
-        @source_reports.first.commit_sha,
+        @source_reports.first.pipeline,
         @source_reports.first.created_at
       )
       @findings = []

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/changelogs/unreleased/222643_use_only_new_vulnerabilities_on_approval_rules.yml

+---
+title: Fix MR approval rule update logic for Vulnerability-Check
+merge_request: 38589
+author:
+type: fixed

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,20 @@ module Gitlab
             @location = new_location
           end
 
+          def unsafe?
+            severity.in?(UNSAFE_SEVERITIES)
+          end
+
+          def eql?(other)
+            report_type == other.report_type &&
+              location.fingerprint == other.location.fingerprint &&
+              primary_identifier.fingerprint == other.primary_identifier.fingerprint
+          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,11 +5,9 @@ module Gitlab
     module Reports
       module Security
         class Report
-          UNSAFE_SEVERITIES = %w[unknown high critical].freeze
-
           attr_reader :created_at
           attr_reader :type
-          attr_reader :commit_sha
+          attr_reader :pipeline
           attr_reader :findings
           attr_reader :scanners
           attr_reader :identifiers
@@ -17,9 +15,9 @@ module Gitlab
           attr_accessor :scanned_resources
           attr_accessor :error
 
-          def initialize(type, commit_sha, created_at)
+          def initialize(type, pipeline, created_at)
             @type = type
-            @commit_sha = commit_sha
+            @pipeline = pipeline
             @created_at = created_at
             @findings = []
             @scanners = {}
@@ -27,6 +25,10 @@ module Gitlab
             @scanned_resources = []
           end
 
+          def commit_sha
+            pipeline.sha
+          end
+
           def errored?
             error.present?
           end
@@ -44,7 +46,7 @@ module Gitlab
           end
 
           def clone_as_blank
-            Report.new(type, commit_sha, created_at)
+            Report.new(type, pipeline, created_at)
           end
 
           def replace_with!(other)
@@ -56,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

@@ -5,21 +5,31 @@ module Gitlab
     module Reports
       module Security
         class Reports
-          attr_reader :reports, :commit_sha
+          attr_reader :reports, :pipeline
 
           delegate :empty?, to: :reports
 
-          def initialize(commit_sha)
+          def initialize(pipeline)
             @reports = {}
-            @commit_sha = commit_sha
+            @pipeline = pipeline
           end
 
           def get_report(report_type, report_artifact)
-            reports[report_type] ||= Report.new(report_type, commit_sha, report_artifact.created_at)
+            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/ci/reports/security/reports.rb

@@ -3,7 +3,7 @@
 FactoryBot.define do
   factory :ci_reports_security_report, class: '::Gitlab::Ci::Reports::Security::Report' do
     type { :sast }
-    commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
+    pipeline { build(:ci_pipeline) }
     created_at { 2.weeks.ago }
     scanned_resources { [] }
 
@@ -22,7 +22,7 @@ FactoryBot.define do
     skip_create
 
     initialize_with do
-      ::Gitlab::Ci::Reports::Security::Report.new(type, commit_sha, created_at)
+      ::Gitlab::Ci::Reports::Security::Report.new(type, pipeline, created_at)
     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/parsers/security/common_spec.rb

@@ -4,8 +4,10 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Parsers::Security::Common do
   describe '#parse!' do
+    let_it_be(:pipeline) { create(:ci_pipeline) }
+
     let(:artifact) { build(:ee_ci_job_artifact, :dependency_scanning) }
-    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, 'sha', 2.weeks.ago) }
+    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
     let(:parser) { described_class.new }
 
     before do

ee/spec/lib/gitlab/ci/parsers/security/container_scanning_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::ContainerScanning do
   let(:parser) { described_class.new }
   let(:project) { artifact.project }
   let(:pipeline) { artifact.job.pipeline }
-  let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha, 2.weeks.ago) }
+  let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
 
   before do
     artifact.each_blob do |blob|

ee/spec/lib/gitlab/ci/parsers/security/coverage_fuzzing_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe Gitlab::Ci::Parsers::Security::CoverageFuzzing do
   let(:project) { artifact.project }
   let(:pipeline) { artifact.job.pipeline }
-  let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha, 2.weeks.ago) }
+  let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
   let(:parser) { described_class.new }
   let(:artifact) { create(:ee_ci_job_artifact, :coverage_fuzzing) }
 

ee/spec/lib/gitlab/ci/parsers/security/dast_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Dast do
     let(:project) { artifact.project }
     let(:pipeline) { artifact.job.pipeline }
     let(:artifact) { create(:ee_ci_job_artifact, :dast) }
-    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha, 2.weeks.ago) }
+    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
     let(:parser) { described_class.new }
 
     where(:report_format,

ee/spec/lib/gitlab/ci/parsers/security/dependency_scanning_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::DependencyScanning do
     let(:project) { artifact.project }
     let(:pipeline) { artifact.job.pipeline }
     let(:artifact) { create(:ee_ci_job_artifact, :dependency_scanning) }
-    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline.sha, 2.weeks.ago) }
+    let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
     let(:parser) { described_class.new }
 
     where(:report_format, :occurrence_count, :identifier_count, :scanner_count, :file_path, :package_name, :package_version, :version) do

ee/spec/lib/gitlab/ci/parsers/security/sast_spec.rb

@@ -4,16 +4,17 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Parsers::Security::Sast do
   describe '#parse!' do
-    subject(:parser) { described_class.new }
+    let_it_be(:pipeline) { create(:ci_pipeline) }
 
-    let(:commit_sha) { "d8978e74745e18ce44d88814004d4255ac6a65bb" }
     let(:created_at) { 2.weeks.ago }
 
+    subject(:parser) { described_class.new }
+
     context "when parsing valid reports" do
       where(report_format: %i(sast sast_deprecated))
 
       with_them do
-        let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, commit_sha, created_at) }
+        let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, created_at) }
         let(:artifact) { create(:ee_ci_job_artifact, report_format) }
 
         before do
@@ -48,7 +49,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Sast do
     end
 
     context "when parsing an empty report" do
-      let(:report) { Gitlab::Ci::Reports::Security::Report.new('sast', commit_sha, created_at) }
+      let(:report) { Gitlab::Ci::Reports::Security::Report.new('sast', pipeline, created_at) }
       let(:blob) { Gitlab::Json.generate({}) }
 
       it { expect(parser.parse!(blob, report)).to be_empty }

ee/spec/lib/gitlab/ci/parsers/security/secret_detection_spec.rb

@@ -4,16 +4,17 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Parsers::Security::SecretDetection do
   describe '#parse!' do
-    subject(:parser) { described_class.new }
+    let_it_be(:pipeline) { create(:ci_pipeline) }
 
-    let(:commit_sha) { "d8978e74745e18ce44d88814004d4255ac6a65bb" }
     let(:created_at) { 2.weeks.ago }
 
+    subject(:parser) { described_class.new }
+
     context "when parsing valid reports" do
       where(report_format: %i(secret_detection))
 
       with_them do
-        let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, commit_sha, created_at) }
+        let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, created_at) }
         let(:artifact) { create(:ee_ci_job_artifact, report_format) }
 
         before do
@@ -48,7 +49,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::SecretDetection do
     end
 
     context "when parsing an empty report" do
-      let(:report) { Gitlab::Ci::Reports::Security::Report.new('secret_detection', commit_sha, created_at) }
+      let(:report) { Gitlab::Ci::Reports::Security::Report.new('secret_detection', pipeline, created_at) }
       let(:blob) { Gitlab::Json.generate({}) }
 
       it { expect(parser.parse!(blob, report)).to be_empty }

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

@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Reports::Security::Reports do
-  let(:commit_sha) { '20410773a37f49d599e5f0d45219b39304763538' }
-  let(:security_reports) { described_class.new(commit_sha) }
-  let(:artifact) { create(:ee_ci_job_artifact, :sast) }
+  let_it_be(:pipeline) { create(:ci_pipeline) }
+  let_it_be(:artifact) { create(:ee_ci_job_artifact, :sast) }
+
+  let(:security_reports) { described_class.new(pipeline) }
 
   describe '#get_report' do
     subject { security_reports.get_report(report_type, artifact) }
@@ -14,12 +15,11 @@ RSpec.describe Gitlab::Ci::Reports::Security::Reports do
       let(:report_type) { 'sast' }
 
       it { expect(subject.type).to eq('sast') }
-      it { expect(subject.commit_sha).to eq(commit_sha) }
       it { expect(subject.created_at).to eq(artifact.created_at) }
 
       it 'initializes a new report and returns it' do
         expect(Gitlab::Ci::Reports::Security::Report).to receive(:new)
-          .with('sast', commit_sha, artifact.created_at).and_call_original
+          .with('sast', pipeline, artifact.created_at).and_call_original
 
         is_expected.to be_a(Gitlab::Ci::Reports::Security::Report)
       end
@@ -38,29 +38,69 @@ RSpec.describe Gitlab::Ci::Reports::Security::Reports do
     end
   end
 
-  describe "#violates_default_policy?" do
-    subject { described_class.new(commit_sha) }
+  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] }
+
+    subject { security_reports.findings }
+
+    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 }
 
-    let(:low_severity) { build(:ci_reports_security_finding, severity: 'low') }
-    let(:high_severity) { build(:ci_reports_security_finding, severity: 'high') }
+      context "when a report has unsafe vulnerability" do
+        before do
+          security_reports.get_report('sast', artifact).add_finding(high_severity_dast)
+        end
 
-    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)
+        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/models/ci/build_spec.rb

@@ -157,7 +157,7 @@ RSpec.describe Ci::Build do
   end
 
   describe '#collect_security_reports!' do
-    let(:security_reports) { ::Gitlab::Ci::Reports::Security::Reports.new(pipeline.sha) }
+    let(:security_reports) { ::Gitlab::Ci::Reports::Security::Reports.new(pipeline) }
 
     subject { job.collect_security_reports!(security_reports) }
 

ee/spec/models/ci/pipeline_spec.rb

@@ -146,12 +146,9 @@ RSpec.describe Ci::Pipeline do
       let!(:cs1_artifact) { create(:ee_ci_job_artifact, :container_scanning, job: build_cs_1, project: project) }
       let!(:cs2_artifact) { create(:ee_ci_job_artifact, :container_scanning, job: build_cs_2, project: project) }
 
-      before do
-      end
-
-      it 'assigns pipeline commit_sha to the reports' do
-        expect(subject.commit_sha).to eq(pipeline.sha)
-        expect(subject.reports.values.map(&:commit_sha).uniq).to contain_exactly(pipeline.sha)
+      it 'assigns pipeline to the reports' do
+        expect(subject.pipeline).to eq(pipeline)
+        expect(subject.reports.values.map(&:pipeline).uniq).to contain_exactly(pipeline)
       end
 
       it 'returns security reports with collected data grouped as expected' do

ee/spec/services/security/store_reports_service_spec.rb

@@ -34,7 +34,7 @@ RSpec.describe Security::StoreReportsService do
       end
 
       context 'when StoreReportService returns an error for a report' do
-        let(:reports) { Gitlab::Ci::Reports::Security::Reports.new(pipeline.sha) }
+        let(:reports) { Gitlab::Ci::Reports::Security::Reports.new(pipeline) }
         let(:sast_report) { reports.get_report('sast', sast_artifact) }
         let(:dast_report) { reports.get_report('dast', dast_artifact) }
         let(:success) { { status: :success } }

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