Merge branch '277328_use_only_uuid_to_pick_correct_finding_from_the_reports' into 'master'

Use only uuid to pick correct finding from the reports

See merge request gitlab-org/gitlab!55643

ee/app/finders/security/findings_finder.rb

@@ -63,7 +63,7 @@ module Security
     end
 
     def report_finding_for(security_finding)
-      security_reports[security_finding.build.id].findings[security_finding.position]
+      report_findings.dig(security_finding.build.id, security_finding.uuid)
     end
 
     def vulnerability_for(security_finding)
@@ -89,10 +89,13 @@ module Security
       security_findings.map(&:project_fingerprint)
     end
 
-    def security_reports
-      @security_reports ||= begin
+    def report_findings
+      @report_findings ||= begin
         builds.each_with_object({}) do |build, memo|
-          memo[build.id] = build.job_artifacts.map(&:security_report).compact.first
+          report = build.job_artifacts.map(&:security_report).compact.first
+          next unless report
+
+          memo[build.id] = report.findings.group_by(&:uuid).transform_values(&:first)
         end
       end
     end

ee/app/models/security/finding.rb

@@ -20,9 +20,9 @@ module Security
     enum severity: ::Enums::Vulnerability.severity_levels, _prefix: :severity
 
     validates :project_fingerprint, presence: true, length: { maximum: 40 }
-    validates :position, presence: true
+    validates :uuid, presence: true
 
-    scope :by_position, -> (positions) { where(position: positions) }
+    scope :by_uuid, -> (uuids) { where(uuid: uuids) }
     scope :by_build_ids, -> (build_ids) { joins(scan: :build).where(ci_builds: { id: build_ids }) }
     scope :by_project_fingerprints, -> (fingerprints) { where(project_fingerprint: fingerprints) }
     scope :by_severity_levels, -> (severity_levels) { where(severity: severity_levels) }

ee/app/services/security/store_findings_metadata_service.rb

@@ -30,25 +30,24 @@ module Security
     end
 
     def store_findings
-      report_findings.each_with_index { |report_finding, position| store_finding!(report_finding, position) }
+      report_findings.each { |report_finding| store_finding!(report_finding) }
     end
 
     def report_findings
       report.findings.select(&:valid?)
     end
 
-    def store_finding!(report_finding, position)
-      security_scan.findings.create!(finding_data(report_finding, position))
+    def store_finding!(report_finding)
+      security_scan.findings.create!(finding_data(report_finding))
     end
 
-    def finding_data(report_finding, position)
+    def finding_data(report_finding)
       {
         severity: report_finding.severity,
         confidence: report_finding.confidence,
         uuid: report_finding.uuid,
         project_fingerprint: report_finding.project_fingerprint,
-        scanner: persisted_scanner_for(report_finding.scanner),
-        position: position
+        scanner: persisted_scanner_for(report_finding.scanner)
       }
     end
 

ee/app/services/security/store_scan_service.rb

@@ -43,19 +43,19 @@ module Security
         security_scan.findings.update_all(deduplicated: false)
 
         security_scan.findings
-                     .by_position(register_finding_keys)
+                     .by_uuid(register_finding_keys)
                      .update_all(deduplicated: true)
       end
     end
 
     # This method registers all finding keys and
-    # returns the positions of unique findings
+    # returns the UUIDs of the unique findings
     def register_finding_keys
-      @register_finding_keys ||= security_report.findings.map.with_index { |finding, index| register_keys(finding.keys) && index }.compact
+      @register_finding_keys ||= security_report.findings.map { |finding| register_keys(finding.keys) && finding.uuid }.compact
     end
 
     def register_keys(keys)
-      return false if known_keys.intersect?(keys.to_set)
+      return if known_keys.intersect?(keys.to_set)
 
       known_keys.merge(keys)
     end

ee/changelogs/unreleased/277328_use_only_uuid_to_pick_correct_finding_from_the_reports.yml

+---
+title: Select security findings by their UUIDs instead of their positions in the report
+  artifacts
+merge_request: 55643
+author:
+type: changed

ee/spec/factories/security/findings.rb

@@ -7,7 +7,7 @@ FactoryBot.define do
 
     severity { :critical }
     confidence { :high }
+    uuid { SecureRandom.uuid }
     project_fingerprint { generate(:project_fingerprint) }
-    sequence :position
   end
 end

ee/spec/finders/security/findings_finder_spec.rb

@@ -8,8 +8,8 @@ RSpec.describe Security::FindingsFinder do
   let_it_be(:build_sast) { create(:ci_build, :success, name: 'sast', pipeline: pipeline) }
   let_it_be(:artifact_ds) { create(:ee_ci_job_artifact, :dependency_scanning, job: build_ds) }
   let_it_be(:artifact_sast) { create(:ee_ci_job_artifact, :sast, job: build_sast) }
-  let_it_be(:report_ds) { create(:ci_reports_security_report, type: :dependency_scanning) }
-  let_it_be(:report_sast) { create(:ci_reports_security_report, type: :sast) }
+  let_it_be(:report_ds) { create(:ci_reports_security_report, pipeline: pipeline, type: :dependency_scanning) }
+  let_it_be(:report_sast) { create(:ci_reports_security_report, pipeline: pipeline, type: :sast) }
 
   let(:severity_levels) { nil }
   let(:confidence_levels) { nil }
@@ -55,6 +55,7 @@ RSpec.describe Security::FindingsFinder do
                    severity: finding.severity,
                    confidence: finding.confidence,
                    project_fingerprint: finding.project_fingerprint,
+                   uuid: finding.uuid,
                    deduplicated: true,
                    position: index,
                    scan: scan)

ee/spec/models/security/finding_spec.rb

@@ -11,20 +11,20 @@ RSpec.describe Security::Finding do
 
   describe 'validations' do
     it { is_expected.to validate_presence_of(:project_fingerprint) }
-    it { is_expected.to validate_presence_of(:position) }
     it { is_expected.to validate_length_of(:project_fingerprint).is_at_most(40) }
+    it { is_expected.to validate_presence_of(:uuid) }
   end
 
   describe 'delegations' do
     it { is_expected.to delegate_method(:scan_type).to(:scan).allow_nil }
   end
 
-  describe '.by_position' do
-    let!(:finding_1) { create(:security_finding, position: 0) }
-    let!(:finding_2) { create(:security_finding, position: 1) }
+  describe '.by_uuid' do
+    let!(:finding_1) { create(:security_finding) }
+    let!(:finding_2) { create(:security_finding) }
     let(:expected_findings) { [finding_1] }
 
-    subject { described_class.by_position(finding_1.position) }
+    subject { described_class.by_uuid(finding_1.uuid) }
 
     it { is_expected.to match_array(expected_findings) }
   end

ee/spec/services/security/store_findings_metadata_service_spec.rb

@@ -43,8 +43,8 @@ RSpec.describe Security::StoreFindingsMetadataService do
                                  .and change { security_scan.findings.first&.confidence }.to(security_finding_1.confidence.to_s)
                                  .and change { security_scan.findings.first&.uuid }.to(security_finding_1.uuid)
                                  .and change { security_scan.findings.first&.project_fingerprint }.to(security_finding_1.project_fingerprint)
-                                 .and change { security_scan.findings.first&.position }.to(0)
-                                 .and change { security_scan.findings.last&.position }.to(1)
+                                 .and change { security_scan.findings.first&.uuid }.to(security_finding_1.uuid)
+                                 .and change { security_scan.findings.last&.uuid }.to(security_finding_2.uuid)
       end
 
       context 'when the scanners already exist in the database' do

ee/spec/services/security/store_scan_service_spec.rb

@@ -25,6 +25,9 @@ RSpec.describe Security::StoreScanService do
   end
 
   describe '#execute' do
+    let_it_be(:unique_finding_uuid) { artifact.security_report.findings[0].uuid }
+    let_it_be(:duplicate_finding_uuid) { artifact.security_report.findings[5].uuid }
+
     let(:deduplicate) { false }
     let(:service_object) { described_class.new(artifact, known_keys, deduplicate) }
     let(:finding_key) do
@@ -52,13 +55,13 @@ RSpec.describe Security::StoreScanService do
       let_it_be(:unique_security_finding) do
         create(:security_finding,
                scan: security_scan,
-               position: 0)
+               uuid: unique_finding_uuid)
       end
 
       let_it_be(:duplicated_security_finding) do
         create(:security_finding,
                scan: security_scan,
-               position: 5)
+               uuid: duplicate_finding_uuid)
       end
 
       it 'does not create a new security scan' do
@@ -90,11 +93,11 @@ RSpec.describe Security::StoreScanService do
 
     context 'when the security scan does not exist for the artifact' do
       let(:unique_finding_attribute) do
-        -> { Security::Finding.by_position(0).first&.deduplicated }
+        -> { Security::Finding.by_uuid(unique_finding_uuid).first&.deduplicated }
       end
 
       let(:duplicated_finding_attribute) do
-        -> { Security::Finding.by_position(5).first&.deduplicated }
+        -> { Security::Finding.by_uuid(duplicate_finding_uuid).first&.deduplicated }
       end
 
       before do