Merge branch 'workaround_for_non_unique_project_fingerprints' into 'master'

Deduplicate findings within the artifact itself and use position instead of project_fingerprint

See merge request gitlab-org/gitlab!44815

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