Merge branch 'implement_override_uuids' into 'master'

Implement override_uuids! for Security Reports

See merge request gitlab-org/gitlab!66978

ee/app/models/ee/ci/job_artifact.rb

@@ -93,9 +93,11 @@ module EE
       strong_memoize(:security_report) do
         next unless file_type.in?(SECURITY_REPORT_FILE_TYPES)
 
+        signatures_enabled = ::Feature.enabled?(:vulnerability_finding_tracking_signatures, project) && project.licensed_feature_available?(:vulnerability_finding_signatures)
+
         report = ::Gitlab::Ci::Reports::Security::Report.new(file_type, job.pipeline, nil).tap do |report|
           each_blob do |blob|
-            ::Gitlab::Ci::Parsers.fabricate!(file_type, blob, report, validate: (validate && validate_schema?)).parse!
+            ::Gitlab::Ci::Parsers.fabricate!(file_type, blob, report, signatures_enabled, validate: (validate && validate_schema?)).parse!
           end
         rescue StandardError
           report.add_error('ParsingError')

ee/app/models/vulnerabilities/finding_signature.rb

@@ -11,6 +11,10 @@ module Vulnerabilities
     enum algorithm_type: VulnerabilityFindingSignatureHelpers::ALGORITHM_TYPES, _prefix: :algorithm
     validates :finding, presence: true
 
+    scope :by_project, -> (project) { joins(:finding).where(vulnerability_occurrences: { project_id: project.id }) }
+    scope :by_signature_sha, -> (shas) { where(signature_sha: shas) }
+    scope :eager_load_finding, -> { includes(:finding) }
+
     def signature_hex
       signature_sha.unpack1("H*")
     end

ee/app/services/security/override_uuids_service.rb

+# frozen_string_literal: true
+
+module Security
+  class OverrideUuidsService
+    def self.execute(security_report)
+      new(security_report).execute
+    end
+
+    def initialize(security_report)
+      @security_report = security_report
+    end
+
+    def execute
+      return unless type.to_s == 'sast' && finding_signature_shas.present?
+
+      findings.each { |finding| override_uuid_for(finding) }
+    end
+
+    private
+
+    attr_reader :security_report
+
+    delegate :pipeline, :findings, :type, to: :security_report
+
+    def override_uuid_for(finding)
+      existing_finding = existing_finding_by_signature(finding)
+
+      if existing_finding
+        finding.overridden_uuid = finding.uuid
+        finding.uuid = existing_finding.uuid
+      end
+    end
+
+    def existing_finding_by_signature(finding)
+      shas = finding.signatures.sort_by(&:priority).map(&:signature_sha)
+
+      existing_signatures.values_at(*shas).first&.finding
+    end
+
+    def existing_signatures
+      @existing_signatures ||= ::Vulnerabilities::FindingSignature.by_signature_sha(finding_signature_shas)
+        .by_project(pipeline.project)
+        .eager_load_finding
+        .index_by(&:signature_sha)
+    end
+
+    def finding_signature_shas
+      @finding_signature_shas ||= findings.flat_map(&:signatures).map(&:signature_sha)
+    end
+  end
+end

ee/app/services/security/store_findings_metadata_service.rb

@@ -46,6 +46,7 @@ module Security
         severity: report_finding.severity,
         confidence: report_finding.confidence,
         uuid: report_finding.uuid,
+        overridden_uuid: report_finding.overridden_uuid,
         project_fingerprint: report_finding.project_fingerprint,
         scanner: persisted_scanner_for(report_finding.scanner)
       }

ee/app/services/security/store_scan_service.rb

@@ -30,7 +30,17 @@ module Security
     private
 
     attr_reader :artifact, :known_keys, :deduplicate
-    delegate :security_report, :project, to: :artifact, private: true
+    delegate :project, to: :artifact, private: true
+
+    def security_report
+      @security_report ||= artifact.security_report.tap do |report|
+        OverrideUuidsService.execute(report) if override_uuids?
+      end
+    end
+
+    def override_uuids?
+      ::Feature.enabled?(:vulnerability_finding_tracking_signatures, project) && project.licensed_feature_available?(:vulnerability_finding_signatures)
+    end
 
     def security_scan
       @security_scan ||= Security::Scan.safe_find_or_create_by!(build: artifact.job, scan_type: artifact.file_type) do |scan|

ee/spec/finders/security/findings_finder_spec.rb

@@ -80,17 +80,7 @@ RSpec.describe Security::FindingsFinder do
 
       context 'N+1 queries' do
         it 'does not cause N+1 queries' do
-          expect { finder_result }.not_to exceed_query_limit(10)
-        end
-
-        context 'with vulnerability_flags disabled' do
-          before do
-            stub_feature_flags(vulnerability_flags: false)
-          end
-
-          it 'does not cause N+1 queries' do
-            expect { finder_result }.not_to exceed_query_limit(8)
-          end
+          expect { finder_result }.not_to exceed_query_limit(11)
         end
       end
 

ee/spec/models/ee/ci/job_artifact_spec.rb

@@ -276,7 +276,7 @@ RSpec.describe Ci::JobArtifact do
 
       with_them do
         let(:mock_parser) { double(:parser, parse!: true) }
-        let(:expected_parser_args) { ['sast', instance_of(String), instance_of(::Gitlab::Ci::Reports::Security::Report), validate: expected_validate_flag] }
+        let(:expected_parser_args) { ['sast', instance_of(String), instance_of(::Gitlab::Ci::Reports::Security::Report), false, validate: expected_validate_flag] }
 
         before do
           allow(job_artifact.job).to receive(:validate_schema?).and_return(build_is_subject_to_validation?)

ee/spec/models/vulnerabilities/finding_signature_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::FindingSignature do
+  let_it_be(:signature) { create(:vulnerabilities_finding_signature) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:finding).class_name('Vulnerabilities::Finding').with_foreign_key('finding_id') }
+  end
+
+  describe 'validations' do
+    it { is_expected.to validate_presence_of(:finding) }
+  end
+
+  describe '.by_project' do
+    let(:project) { create(:project) }
+    let(:finding) { create(:vulnerabilities_finding, project: project) }
+    let!(:expected_signature) { create(:vulnerabilities_finding_signature, finding: finding) }
+
+    subject { described_class.by_project(project) }
+
+    it { is_expected.to eq([expected_signature]) }
+  end
+
+  describe '.by_signature_sha' do
+    let(:signature_sha) { ::Digest::SHA1.digest(SecureRandom.hex(50)) }
+    let!(:expected_signature) { create(:vulnerabilities_finding_signature, signature_sha: signature_sha) }
+
+    subject { described_class.by_signature_sha(signature_sha) }
+
+    it { is_expected.to eq([expected_signature]) }
+  end
+end

ee/spec/services/security/override_uuids_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::OverrideUuidsService do
+  describe '#execute' do
+    let(:vulnerability_finding_uuid) { SecureRandom.uuid }
+    let(:report_finding_uuid) { SecureRandom.uuid }
+    let(:pipeline) { create(:ci_pipeline) }
+    let(:vulnerability_finding) { create(:vulnerabilities_finding, project: pipeline.project, uuid: vulnerability_finding_uuid) }
+    let(:signature) { ::Gitlab::Ci::Reports::Security::FindingSignature.new(algorithm_type: 'location', signature_value: 'value') }
+    let(:report_finding) { create(:ci_reports_security_finding, uuid: report_finding_uuid, vulnerability_finding_signatures_enabled: true, signatures: [signature]) }
+    let(:report) { create(:ci_reports_security_report, findings: [report_finding], pipeline: pipeline) }
+    let(:service_object) { described_class.new(report) }
+
+    before do
+      create(:vulnerabilities_finding_signature, finding: vulnerability_finding, algorithm_type: 'location', signature_sha: Digest::SHA1.digest('value'))
+    end
+
+    subject(:override_uuids) { service_object.execute }
+
+    it 'overrides finding uuids' do
+      expect { override_uuids }.to change { report_finding.uuid }.from(report_finding_uuid).to(vulnerability_finding_uuid)
+                               .and change { report_finding.overridden_uuid }.from(nil).to(report_finding_uuid)
+    end
+  end
+end

ee/spec/services/security/store_scan_service_spec.rb

@@ -57,6 +57,64 @@ RSpec.describe Security::StoreScanService do
       known_keys.add(finding_key)
     end
 
+    context 'when the `vulnerability_finding_signatures` licensed feature is available' do
+      before do
+        stub_feature_flags(vulnerability_finding_tracking_signatures: feature_enabled?)
+        stub_licensed_features(vulnerability_finding_signatures: true)
+
+        allow(Security::OverrideUuidsService).to receive(:execute)
+      end
+
+      context 'when the `vulnerability_finding_tracking_signatures` feature is enabled' do
+        let(:feature_enabled?) { true }
+
+        it 'calls `Security::OverrideUuidsService` with security report to re-calculate the finding UUIDs' do
+          store_scan
+
+          expect(Security::OverrideUuidsService).to have_received(:execute).with(artifact.security_report)
+        end
+      end
+
+      context 'when the `vulnerability_finding_tracking_signatures` feature is disabled' do
+        let(:feature_enabled?) { false }
+
+        it 'does not call `Security::OverrideUuidsService`' do
+          store_scan
+
+          expect(Security::OverrideUuidsService).not_to have_received(:execute)
+        end
+      end
+    end
+
+    context 'when the `vulnerability_finding_signatures` licensed feature is not available' do
+      before do
+        stub_feature_flags(vulnerability_finding_tracking_signatures: feature_enabled?)
+        stub_licensed_features(vulnerability_finding_signatures: false)
+
+        allow(Security::OverrideUuidsService).to receive(:execute)
+      end
+
+      context 'when the `vulnerability_finding_tracking_signatures` feature is enabled' do
+        let(:feature_enabled?) { true }
+
+        it 'does not call `Security::OverrideUuidsService`' do
+          store_scan
+
+          expect(Security::OverrideUuidsService).not_to have_received(:execute)
+        end
+      end
+
+      context 'when the `vulnerability_finding_tracking_signatures` feature is disabled' do
+        let(:feature_enabled?) { false }
+
+        it 'does not call `Security::OverrideUuidsService`' do
+          store_scan
+
+          expect(Security::OverrideUuidsService).not_to have_received(:execute)
+        end
+      end
+    end
+
     context 'when the report has some errors' do
       before do
         artifact.security_report.errors << { 'type' => 'foo', 'message' => 'bar' }

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

@@ -21,7 +21,8 @@ module Gitlab
           attr_reader :scanner
           attr_reader :scan
           attr_reader :severity
-          attr_reader :uuid
+          attr_accessor :uuid
+          attr_accessor :overridden_uuid
           attr_reader :remediations
           attr_reader :details
           attr_reader :signatures