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

See merge request gitlab-org/gitlab!50283

ee/app/services/security/store_report_service.rb

@@ -80,15 +80,25 @@ module Security
       }
 
       begin
-        vulnerability_finding = project
-          .vulnerability_findings
-          .create_with(create_params)
-          .find_or_initialize_by(find_params)
+        # 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
+        Gitlab::ErrorTracking.track_and_raise_exception(e, find_params: find_params, uuid: finding.uuid)
+
+        vulnerability_finding
       rescue ActiveRecord::RecordInvalid => e
         Gitlab::ErrorTracking.track_and_raise_exception(e, create_params: create_params&.dig(:raw_metadata))
       end

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

@@ -31,7 +31,9 @@ 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|
+      Gitlab::UUID.v5("#{report_type}-#{identifiers.first&.fingerprint}-#{location.fingerprint}-#{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,13 @@ 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 }
+    sequence(:uuid) do
+      Gitlab::UUID.v5("#{report_type}-#{primary_identifier.fingerprint}-#{location_fingerprint}-#{project_id}")
+    end
     severity { :high }
     confidence { :medium }
     scanner factory: :vulnerabilities_scanner

ee/spec/services/security/store_report_service_spec.rb

@@ -124,9 +124,16 @@ RSpec.describe Security::StoreReportService, '#execute' do
         primary_identifier: identifier,
         scanner: scanner,
         project: project,
+        uuid: "80571acf-8660-4bc8-811a-1d8dec9ab6f4",
         location_fingerprint: 'd869ba3f0b3347eb2749135a437dc07c8ae0f420')
     end
 
+    let(:uuid_v5_components) do
+      "#{finding.report_type}-#{finding.primary_identifier.fingerprint}-#{finding.location_fingerprint}-#{finding.project_id}"
+    end
+
+    let(:desired_uuid) { Gitlab::UUID.v5(uuid_v5_components) }
+
     let!(:vulnerability) { create(:vulnerability, findings: [finding], project: project) }
 
     before do
@@ -136,6 +143,12 @@ RSpec.describe Security::StoreReportService, '#execute' do
 
     subject { described_class.new(new_pipeline, new_report).execute }
 
+    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 +171,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