Merge branch...

Merge branch '292236-adjust-security-storereportservice-to-look-up-findings-using-uuidv5' into 'master'

Adjust `Security::StoreReportService` to look up findings using UUIDv5, attempt 2

See merge request gitlab-org/gitlab!52231

app/services/security/vulnerability_uuid.rb

+# frozen_string_literal: true
+
+module Security
+  class VulnerabilityUUID
+    def self.generate(report_type:, primary_identifier_fingerprint:, location_fingerprint:, project_id:)
+      Gitlab::UUID.v5("#{report_type}-#{primary_identifier_fingerprint}-#{location_fingerprint}-#{project_id}")
+    end
+  end
+end

ee/app/models/vulnerabilities/finding.rb

@@ -369,7 +369,16 @@ module Vulnerabilities
     # We will eventually have only UUIDv5 values for the `uuid`
     # attribute of the finding records.
     def uuid_v5
-      Gitlab::UUID.v5?(uuid) ? uuid : Gitlab::UUID.v5(uuid_v5_name)
+      if Gitlab::UUID.v5?(uuid)
+        uuid
+      else
+        ::Security::VulnerabilityUUID.generate(
+          report_type: report_type,
+          primary_identifier_fingerprint: primary_identifier.fingerprint,
+          location_fingerprint: location_fingerprint,
+          project_id: project_id
+        )
+      end
     end
 
     def pipeline_branch
@@ -391,14 +400,5 @@ module Vulnerabilities
         project_fingerprint: project_fingerprint
       }
     end
-
-    def uuid_v5_name
-      [
-        report_type,
-        primary_identifier.fingerprint,
-        location_fingerprint,
-        project_id
-      ].join('-')
-    end
   end
 end

ee/app/services/security/store_report_service.rb

@@ -80,15 +80,32 @@ module Security
       }
 
       begin
-        vulnerability_finding = project
-          .vulnerability_findings
+        # Look for existing Findings using UUID
+        vulnerability_finding = project.vulnerability_findings.find_by(uuid: finding.uuid)
+
+        # If there's no Finding then we're dealing with one of two cases:
+        # 1. The Finding is a new one
+        # 2. The Finding is already saved but has UUIDv4
+        unless vulnerability_finding
+          vulnerability_finding = project.vulnerability_findings
             .create_with(create_params)
             .find_or_initialize_by(find_params)
+          vulnerability_finding.uuid = finding.uuid
+        end
 
         vulnerability_finding.save!
         vulnerability_finding
-      rescue ActiveRecord::RecordNotUnique
-        project.vulnerability_findings.find_by!(find_params)
+      rescue ActiveRecord::RecordNotUnique => e
+        # This might happen if we're processing another report in parallel and it finds the same Finding
+        # faster. In that case we need to perform the lookup again
+
+        by_uuid = project.vulnerability_findings.reset.find_by(uuid: finding.uuid)
+        return by_uuid if by_uuid
+
+        by_find_params = project.vulnerability_findings.reset.find_by(find_params)
+        return by_find_params if by_find_params
+
+        Gitlab::ErrorTracking.track_and_raise_exception(e, find_params: find_params, uuid: finding.uuid)
       rescue ActiveRecord::RecordInvalid => e
         Gitlab::ErrorTracking.track_and_raise_exception(e, create_params: create_params&.dig(:raw_metadata))
       end

ee/lib/gitlab/ci/parsers/security/common.rb

@@ -180,9 +180,12 @@ module Gitlab
               return
             end
 
-            name = uuid_v5_name_components.values.join('-')
-
-            Gitlab::UUID.v5(name)
+            ::Security::VulnerabilityUUID.generate(
+              report_type: uuid_v5_name_components[:report_type],
+              primary_identifier_fingerprint: uuid_v5_name_components[:primary_identifier_fingerprint],
+              location_fingerprint: uuid_v5_name_components[:location_fingerprint],
+              project_id: uuid_v5_name_components[:project_id]
+            )
           end
         end
       end

ee/spec/factories/ci/reports/security/findings.rb

@@ -31,7 +31,14 @@ FactoryBot.define do
     scanner factory: :ci_reports_security_scanner
     severity { :high }
     scan factory: :ci_reports_security_scan
-    sequence(:uuid) { generate(:vulnerability_finding_uuid) }
+    sequence(:uuid) do |n|
+      ::Security::VulnerabilityUUID.generate(
+        report_type: report_type,
+        primary_identifier_fingerprint: identifiers.first&.fingerprint,
+        location_fingerprint: location.fingerprint,
+        project_id: n
+      )
+    end
 
     skip_create
 

ee/spec/factories/vulnerabilities/findings.rb

 # frozen_string_literal: true
 
 FactoryBot.define do
-  sequence :vulnerability_finding_uuid do |n|
-    SecureRandom.uuid
-  end
-
   factory :vulnerabilities_finding_with_remediation, parent: :vulnerabilities_finding do
     transient do
       summary { nil }
@@ -47,11 +43,18 @@ FactoryBot.define do
   factory :vulnerabilities_finding, class: 'Vulnerabilities::Finding' do
     name { 'Cipher with no integrity' }
     project
-    sequence(:uuid) { generate(:vulnerability_finding_uuid) }
     project_fingerprint { generate(:project_fingerprint) }
     primary_identifier factory: :vulnerabilities_identifier
-    location_fingerprint { '4e5b6966dd100170b4b1ad599c7058cce91b57b4' }
+    location_fingerprint { SecureRandom.hex(20) }
     report_type { :sast }
+    uuid do
+      Security::VulnerabilityUUID.generate(
+        report_type: report_type,
+        primary_identifier_fingerprint: primary_identifier.fingerprint,
+        location_fingerprint: location_fingerprint,
+        project_id: project_id
+      )
+    end
     severity { :high }
     confidence { :medium }
     scanner factory: :vulnerabilities_scanner

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

@@ -163,10 +163,24 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
 
     describe 'setting the uuid' do
       let(:finding_uuids) { report.findings.map(&:uuid) }
-      let(:uuid_1_components) { "dependency_scanning-4ff8184cd18485b6e85d5b101e341b12eacd1b3b-33dc9f32c77dde16d39c69d3f78f27ca3114a7c5-#{pipeline.project_id}" }
-      let(:uuid_2_components) { "dependency_scanning-d55f9e66e79882ae63af9fd55cc822ab75307e31-33dc9f32c77dde16d39c69d3f78f27ca3114a7c5-#{pipeline.project_id}" }
-      let(:uuid_1) { Gitlab::UUID.v5(uuid_1_components) }
-      let(:uuid_2) { Gitlab::UUID.v5(uuid_2_components) }
+      let(:uuid_1) do
+        Security::VulnerabilityUUID.generate(
+          report_type: "dependency_scanning",
+          primary_identifier_fingerprint: "4ff8184cd18485b6e85d5b101e341b12eacd1b3b",
+          location_fingerprint: "33dc9f32c77dde16d39c69d3f78f27ca3114a7c5",
+          project_id: pipeline.project_id
+        )
+      end
+
+      let(:uuid_2) do
+        Security::VulnerabilityUUID.generate(
+          report_type: "dependency_scanning",
+          primary_identifier_fingerprint: "d55f9e66e79882ae63af9fd55cc822ab75307e31",
+          location_fingerprint: "33dc9f32c77dde16d39c69d3f78f27ca3114a7c5",
+          project_id: pipeline.project_id
+        )
+      end
+
       let(:expected_uuids) { [uuid_1, uuid_2, nil] }
 
       it 'sets the UUIDv5 for findings', :aggregate_failures do

ee/spec/migrations/remove_duplicated_cs_findings_spec.rb

@@ -5,6 +5,8 @@ require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20200910131218_remove_duplicated_cs_findings.rb')
 
 RSpec.describe RemoveDuplicatedCsFindings, :migration do
+  include MigrationHelpers::VulnerabilitiesFindingsHelper
+
   let(:migration) { 'RemoveDuplicateCsFindings'}
   let(:namespaces) { table(:namespaces) }
   let(:notes) { table(:notes) }
@@ -90,21 +92,16 @@ RSpec.describe RemoveDuplicatedCsFindings, :migration do
   end
 
   def finding_params(primary_identifier_id, project_id)
-    attrs = attributes_for(:vulnerabilities_finding) # rubocop: disable RSpec/FactoriesInMigrationSpecs
-    {
+    attrs = attributes_for_vulnerabilities_finding
+    custom_attrs = {
       severity: 0,
       confidence: 5,
       report_type: 2,
       project_id: project_id,
       scanner_id: 6,
-      primary_identifier_id: primary_identifier_id,
-      project_fingerprint: attrs[:project_fingerprint],
-      location_fingerprint: Digest::SHA1.hexdigest(SecureRandom.hex(10)),
-      uuid: attrs[:uuid],
-      name: attrs[:name],
-      metadata_version: '1.3',
-      raw_metadata: attrs[:raw_metadata]
+      primary_identifier_id: primary_identifier_id
     }
+    attrs.merge(custom_attrs)
   end
 
   def create_identifier(number_of)

ee/spec/services/security/store_report_service_spec.rb

@@ -111,6 +111,7 @@ RSpec.describe Security::StoreReportService, '#execute' do
   context 'with existing data from previous pipeline' do
     let(:scanner) { build(:vulnerabilities_scanner, project: project, external_id: 'bandit', name: 'Bandit') }
     let(:identifier) { build(:vulnerabilities_identifier, project: project, fingerprint: 'e6dd15eda2137be0034977a85b300a94a4f243a3') }
+    let(:different_identifier) { build(:vulnerabilities_identifier, project: project, fingerprint: 'fa47ee81f079e5c38ea6edb700b44eaeb62f67ee') }
     let!(:new_artifact) { create(:ee_ci_job_artifact, :sast, job: new_build) }
     let(:new_build) { create(:ci_build, pipeline: new_pipeline) }
     let(:new_pipeline) { create(:ci_pipeline, project: project) }
@@ -124,11 +125,33 @@ RSpec.describe Security::StoreReportService, '#execute' do
         primary_identifier: identifier,
         scanner: scanner,
         project: project,
+        uuid: "80571acf-8660-4bc8-811a-1d8dec9ab6f4",
         location_fingerprint: 'd869ba3f0b3347eb2749135a437dc07c8ae0f420')
     end
 
     let!(:vulnerability) { create(:vulnerability, findings: [finding], project: project) }
 
+    let(:desired_uuid) do
+      Security::VulnerabilityUUID.generate(
+        report_type: finding.report_type,
+        primary_identifier_fingerprint: finding.primary_identifier.fingerprint,
+        location_fingerprint: finding.location_fingerprint,
+        project_id: finding.project_id
+      )
+    end
+
+    let!(:finding_with_uuidv5) do
+      create(:vulnerabilities_finding,
+        pipelines: [pipeline],
+        identifiers: [different_identifier],
+        primary_identifier: different_identifier,
+        scanner: scanner,
+        project: project,
+        location_fingerprint: '34661e23abcf78ff80dfcc89d0700437612e3f88')
+    end
+
+    let!(:vulnerability_with_uuid5) { create(:vulnerability, findings: [finding_with_uuidv5], project: project) }
+
     before do
       project.add_developer(user)
       allow(new_pipeline).to receive(:user).and_return(user)
@@ -136,6 +159,16 @@ RSpec.describe Security::StoreReportService, '#execute' do
 
     subject { described_class.new(new_pipeline, new_report).execute }
 
+    it 'does not change existing UUIDv5' do
+      expect { subject }.not_to change(finding_with_uuidv5, :uuid)
+    end
+
+    it 'updates UUIDv4 to UUIDv5' do
+      subject
+
+      expect(finding.reload.uuid).to eq(desired_uuid)
+    end
+
     it 'inserts only new scanners and reuse existing ones' do
       expect { subject }.to change { Vulnerabilities::Scanner.count }.by(2)
     end
@@ -158,11 +191,13 @@ RSpec.describe Security::StoreReportService, '#execute' do
 
     it 'updates existing findings with new data' do
       subject
+
       expect(finding.reload).to have_attributes(severity: 'medium', name: 'Probable insecure usage of temp file/directory.')
     end
 
     it 'updates existing vulnerability with new data' do
       subject
+
       expect(vulnerability.reload).to have_attributes(severity: 'medium', title: 'Probable insecure usage of temp file/directory.', title_html: 'Probable insecure usage of temp file/directory.')
     end
 

lib/gitlab/background_migration/populate_finding_uuid_for_vulnerability_feedback.rb

@@ -61,16 +61,12 @@ module Gitlab
         private
 
         def calculated_uuid
-          Gitlab::UUID.v5(uuid_components)
-        end
-
-        def uuid_components
-          [
-            category,
-            vulnerability_finding.primary_identifier.fingerprint,
-            vulnerability_finding.location_fingerprint,
-            project_id
-          ].join('-')
+          ::Security::VulnerabilityUUID.generate(
+            report_type: category,
+            primary_identifier_fingerprint: vulnerability_finding.primary_identifier.fingerprint,
+            location_fingerprint: vulnerability_finding.location_fingerprint,
+            project_id: project_id
+          )
         end
 
         def finding_key

spec/lib/gitlab/background_migration/populate_finding_uuid_for_vulnerability_feedback_spec.rb

@@ -27,12 +27,33 @@ RSpec.describe Gitlab::BackgroundMigration::PopulateFindingUuidForVulnerabilityF
   let(:finding_1) { finding_creator.call(sast_report, location_fingerprint_1) }
   let(:finding_2) { finding_creator.call(dast_report, location_fingerprint_2) }
   let(:finding_3) { finding_creator.call(secret_detection_report, location_fingerprint_3) }
-  let(:uuid_1_components) { ['sast', identifier.fingerprint, location_fingerprint_1, project.id].join('-') }
-  let(:uuid_2_components) { ['dast', identifier.fingerprint, location_fingerprint_2, project.id].join('-') }
-  let(:uuid_3_components) { ['secret_detection', identifier.fingerprint, location_fingerprint_3, project.id].join('-') }
-  let(:expected_uuid_1) { Gitlab::UUID.v5(uuid_1_components) }
-  let(:expected_uuid_2) { Gitlab::UUID.v5(uuid_2_components) }
-  let(:expected_uuid_3) { Gitlab::UUID.v5(uuid_3_components) }
+  let(:expected_uuid_1) do
+    Security::VulnerabilityUUID.generate(
+      report_type: 'sast',
+      primary_identifier_fingerprint: identifier.fingerprint,
+      location_fingerprint: location_fingerprint_1,
+      project_id: project.id
+    )
+  end
+
+  let(:expected_uuid_2) do
+    Security::VulnerabilityUUID.generate(
+      report_type: 'dast',
+      primary_identifier_fingerprint: identifier.fingerprint,
+      location_fingerprint: location_fingerprint_2,
+      project_id: project.id
+    )
+  end
+
+  let(:expected_uuid_3) do
+    Security::VulnerabilityUUID.generate(
+      report_type: 'secret_detection',
+      primary_identifier_fingerprint: identifier.fingerprint,
+      location_fingerprint: location_fingerprint_3,
+      project_id: project.id
+    )
+  end
+
   let(:finding_creator) do
     -> (report_type, location_fingerprint) do
       findings.create!(