Mark findings as deduplicated based on their position

It is possible to have more than one finding with the same project
fingerprint for a report artifact which breaks the logic of marking
findings as deduplicated based on project fingerprint values.

For this reason, we need to mark findings as deduplicated based on
their position within their reports.

changelogs/unreleased/workaround_for_non_unique_project_fingerprints.yml

+---
+title: Add `position` column into security_findings table
+merge_request: 44815
+author:
+type: fixed

db/migrate/20201008224441_add_position_into_security_findings.rb

+# frozen_string_literal: true
+
+class AddPositionIntoSecurityFindings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :security_findings, :position, :integer
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :security_findings, :position
+    end
+  end
+end

db/migrate/20201019091307_add_unique_index_on_scan_id_and_position_of_security_findings.rb

+# frozen_string_literal: true
+
+class AddUniqueIndexOnScanIdAndPositionOfSecurityFindings < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_security_findings_on_scan_id_and_position'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :security_findings, [:scan_id, :position], unique: true, name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name :security_findings, INDEX_NAME
+  end
+end

db/schema_migrations/20201008224441

+d0ca8f0dbe0cf0fbbdd715867f3ae20862683433d919ee5cd942086d21f3b44d
\ No newline at end of file

db/schema_migrations/20201019091307

+f19ab0de07415e728849ef4e56804909a3a4a57ad8f55fe71a27bc43c535ac66
\ No newline at end of file

db/structure.sql

@@ -15857,6 +15857,7 @@ CREATE TABLE security_findings (
     confidence smallint NOT NULL,
     project_fingerprint text NOT NULL,
     deduplicated boolean DEFAULT false NOT NULL,
+    "position" integer,
     CONSTRAINT check_b9508c6df8 CHECK ((char_length(project_fingerprint) <= 40))
 );
 
@@ -21528,6 +21529,8 @@ CREATE INDEX index_security_findings_on_project_fingerprint ON security_findings
 
 CREATE INDEX index_security_findings_on_scan_id_and_deduplicated ON security_findings USING btree (scan_id, deduplicated);
 
+CREATE UNIQUE INDEX index_security_findings_on_scan_id_and_position ON security_findings USING btree (scan_id, "position");
+
 CREATE INDEX index_security_findings_on_scanner_id ON security_findings USING btree (scanner_id);
 
 CREATE INDEX index_security_findings_on_severity ON security_findings USING btree (severity);

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

@@ -118,11 +118,14 @@ module EE
       strong_memoize(:security_report) do
         next unless file_type.in?(SECURITY_REPORT_FILE_TYPES)
 
-        ::Gitlab::Ci::Reports::Security::Report.new(file_type, nil, nil).tap do |report|
+        report = ::Gitlab::Ci::Reports::Security::Report.new(file_type, nil, nil).tap do |report|
           each_blob do |blob|
             ::Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, report)
           end
         end
+
+        # This will remove the duplicated findings within the artifact itself
+        ::Security::MergeReportsService.new(report).execute
       end
     end
 

ee/app/models/security/finding.rb

@@ -20,8 +20,9 @@ module Security
     enum severity: Vulnerabilities::Finding::SEVERITY_LEVELS, _prefix: :severity
 
     validates :project_fingerprint, presence: true, length: { maximum: 40 }
+    validates :position, presence: true
 
-    scope :by_project_fingerprint, -> (fingerprints) { where(project_fingerprint: fingerprints) }
+    scope :by_position, -> (positions) { where(position: positions) }
     scope :by_build_ids, -> (build_ids) { joins(scan: :build).where(ci_builds: { id: build_ids }) }
   end
 end

ee/app/services/security/store_findings_metadata_service.rb

@@ -31,21 +31,22 @@ module Security
     end
 
     def store_findings
-      report_findings.each { |report_finding| store_finding!(report_finding) }
+      report_findings.each_with_index { |report_finding, position| store_finding!(report_finding, position) }
     end
 
-    def store_finding!(report_finding)
+    def store_finding!(report_finding, position)
       return if report_finding.scanner.blank?
 
-      security_scan.findings.create!(finding_data(report_finding))
+      security_scan.findings.create!(finding_data(report_finding, position))
     end
 
-    def finding_data(report_finding)
+    def finding_data(report_finding, position)
       {
         severity: report_finding.severity,
         confidence: report_finding.confidence,
         project_fingerprint: report_finding.project_fingerprint,
-        scanner: persisted_scanner_for(report_finding.scanner)
+        scanner: persisted_scanner_for(report_finding.scanner),
+        position: position
       }
     end
 

ee/app/services/security/store_scan_service.rb

@@ -43,21 +43,19 @@ module Security
         security_scan.findings.update_all(deduplicated: false)
 
         security_scan.findings
-                     .by_project_fingerprint(deduplicated_project_fingerprints)
+                     .by_position(register_finding_keys)
                      .update_all(deduplicated: true)
       end
     end
 
-    def deduplicated_project_fingerprints
-      register_finding_keys.map(&:project_fingerprint)
-    end
-
+    # This method registers all finding keys and
+    # returns the positions of unique findings
     def register_finding_keys
-      @register_finding_keys ||= security_report.findings.select { |finding| register_keys(finding.keys) }
+      @register_finding_keys ||= security_report.findings.map.with_index { |finding, index| register_keys(finding.keys) && index }.compact
     end
 
     def register_keys(keys)
-      keys.map { |key| known_keys.add?(key) }.all?
+      keys.all? { |key| known_keys.add?(key) }
     end
   end
 end

ee/spec/factories/security/findings.rb

@@ -8,5 +8,6 @@ FactoryBot.define do
     severity { :critical }
     confidence { :high }
     project_fingerprint { generate(:project_fingerprint) }
+    sequence :position
   end
 end

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

@@ -256,7 +256,9 @@ RSpec.describe Ci::JobArtifact do
       clear_security_report
       job_artifact.security_report
 
-      expect(::Gitlab::Ci::Reports::Security::Report).to have_received(:new).once
+      # This entity class receives the call twice
+      # because of the way MergeReportsService is implemented.
+      expect(::Gitlab::Ci::Reports::Security::Report).to have_received(:new).twice
     end
   end
 end

ee/spec/models/security/finding_spec.rb

@@ -10,15 +10,16 @@ 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) }
   end
 
-  describe '.by_project_fingerprint' do
-    let!(:finding_1) { create(:security_finding) }
-    let!(:finding_2) { create(:security_finding) }
+  describe '.by_position' do
+    let!(:finding_1) { create(:security_finding, position: 0) }
+    let!(:finding_2) { create(:security_finding, position: 1) }
     let(:expected_findings) { [finding_1] }
 
-    subject { described_class.by_project_fingerprint(finding_1.project_fingerprint) }
+    subject { described_class.by_position(finding_1.position) }
 
     it { is_expected.to match_array(expected_findings) }
   end

ee/spec/services/security/store_findings_metadata_service_spec.rb

@@ -5,12 +5,13 @@ require 'spec_helper'
 RSpec.describe Security::StoreFindingsMetadataService do
   let_it_be(:security_scan) { create(:security_scan) }
   let_it_be(:project) { security_scan.project }
-  let_it_be(:security_finding) { build(:ci_reports_security_finding) }
+  let_it_be(:security_finding_1) { build(:ci_reports_security_finding) }
+  let_it_be(:security_finding_2) { build(:ci_reports_security_finding) }
   let_it_be(:security_scanner) { build(:ci_reports_security_scanner) }
   let_it_be(:report) do
     build(
       :ci_reports_security_report,
-      findings: [security_finding],
+      findings: [security_finding_1, security_finding_2],
       scanners: [security_scanner]
     )
   end
@@ -36,10 +37,12 @@ RSpec.describe Security::StoreFindingsMetadataService do
       end
 
       it 'creates the security finding entries in database' do
-        expect { store_findings }.to change { security_scan.findings.count }.by(1)
-                                 .and change { security_scan.findings.last&.severity }.to(security_finding.severity.to_s)
-                                 .and change { security_scan.findings.last&.confidence }.to(security_finding.confidence.to_s)
-                                 .and change { security_scan.findings.last&.project_fingerprint }.to(security_finding.project_fingerprint)
+        expect { store_findings }.to change { security_scan.findings.count }.by(2)
+                                 .and change { security_scan.findings.first&.severity }.to(security_finding_1.severity.to_s)
+                                 .and change { security_scan.findings.first&.confidence }.to(security_finding_1.confidence.to_s)
+                                 .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)
       end
 
       context 'when the scanners already exist in the database' do

ee/spec/services/security/store_scan_service_spec.rb

@@ -49,16 +49,16 @@ RSpec.describe Security::StoreScanService do
 
     context 'when the security scan already exists for the artifact' do
       let_it_be(:security_scan) { create(:security_scan, build: artifact.job, scan_type: :sast) }
-      let_it_be(:duplicated_security_finding) do
+      let_it_be(:unique_security_finding) do
         create(:security_finding,
                scan: security_scan,
-               project_fingerprint: 'd533c3a12403b6c6033a50b53f9c73f894a40fc6')
+               position: 0)
       end
 
-      let_it_be(:unique_security_finding) do
+      let_it_be(:duplicated_security_finding) do
         create(:security_finding,
                scan: security_scan,
-               project_fingerprint: 'b9c0d1cdc7cb9c180ebb6981abbddc2df0172509')
+               position: 5)
       end
 
       it 'does not create a new security scan' do
@@ -89,12 +89,12 @@ RSpec.describe Security::StoreScanService do
     end
 
     context 'when the security scan does not exist for the artifact' do
-      let(:duplicated_finding_attribute) do
-        -> { Security::Finding.by_project_fingerprint('d533c3a12403b6c6033a50b53f9c73f894a40fc6').first&.deduplicated }
+      let(:unique_finding_attribute) do
+        -> { Security::Finding.by_position(0).first&.deduplicated }
       end
 
-      let(:unique_finding_attribute) do
-        -> { Security::Finding.by_project_fingerprint('b9c0d1cdc7cb9c180ebb6981abbddc2df0172509').first&.deduplicated }
+      let(:duplicated_finding_attribute) do
+        -> { Security::Finding.by_position(5).first&.deduplicated }
       end
 
       before do