Merge branch 'move_uuid_calculation_out_of_store_report_service' into 'master'

Move uuid calculation out of store report service

See merge request gitlab-org/gitlab!49784

ee/app/finders/security/findings_finder.rb

@@ -111,7 +111,7 @@ module Security
 
     def all_security_findings
       pipeline.security_findings
-              .with_build_and_artifacts
+              .with_pipeline_entities
               .with_scan
               .with_scanner
               .deduplicated

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

@@ -118,7 +118,7 @@ module EE
       strong_memoize(:security_report) do
         next unless file_type.in?(SECURITY_REPORT_FILE_TYPES)
 
-        report = ::Gitlab::Ci::Reports::Security::Report.new(file_type, nil, nil).tap do |report|
+        report = ::Gitlab::Ci::Reports::Security::Report.new(file_type, job.pipeline, nil).tap do |report|
           each_blob do |blob|
             ::Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, report)
           end

ee/app/models/security/finding.rb

@@ -38,7 +38,7 @@ module Security
                 .where('vulnerability_feedback.project_fingerprint = security_findings.project_fingerprint'))
     end
     scope :ordered, -> { order(severity: :desc, confidence: :desc, id: :asc) }
-    scope :with_build_and_artifacts, -> { includes(build: :job_artifacts) }
+    scope :with_pipeline_entities, -> { includes(build: [:job_artifacts, pipeline: :project]) }
     scope :with_scan, -> { includes(:scan) }
     scope :with_scanner, -> { includes(:scanner) }
     scope :deduplicated, -> { where(deduplicated: true) }

ee/app/services/security/store_report_service.rb

@@ -50,7 +50,6 @@ module Security
       end
 
       vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan, :links)
-      vulnerability_params[:uuid] = calculate_uuid_v5(finding)
       vulnerability_finding = create_or_find_vulnerability_finding(finding, vulnerability_params)
 
       update_vulnerability_scanner(finding)
@@ -94,23 +93,6 @@ module Security
       end
     end
 
-    def calculate_uuid_v5(vulnerability_finding)
-      uuid_v5_name_components = {
-        report_type: vulnerability_finding.report_type,
-        primary_identifier_fingerprint: vulnerability_finding.primary_fingerprint,
-        location_fingerprint: vulnerability_finding.location.fingerprint,
-        project_id: project.id
-      }
-
-      if uuid_v5_name_components.values.any?(&:nil?)
-        Gitlab::AppLogger.warn(message: "One or more UUID name components are nil", components: uuid_v5_name_components)
-      end
-
-      name = uuid_v5_name_components.values.join('-')
-
-      Gitlab::Vulnerabilities::CalculateFindingUUID.call(name)
-    end
-
     def update_vulnerability_scanner(finding)
       scanner = scanners_objects[finding.scanner.key]
       scanner.update!(finding.scanner.to_hash)

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

@@ -61,7 +61,7 @@ module Gitlab
 
             report.add_finding(
               ::Gitlab::Ci::Reports::Security::Finding.new(
-                uuid: SecureRandom.uuid,
+                uuid: calculate_uuid_v5(report, location, identifiers.first),
                 report_type: report.type,
                 name: finding_name(data, identifiers, location),
                 compare_key: data['cve'] || '',
@@ -160,6 +160,24 @@ module Gitlab
             identifier = identifiers.find(&:cve?) || identifiers.find(&:cwe?) || identifiers.first
             "#{identifier.name} in #{location&.fingerprint_path}"
           end
+
+          def calculate_uuid_v5(report, location, primary_identifier)
+            uuid_v5_name_components = {
+              report_type: report.type,
+              primary_identifier_fingerprint: primary_identifier&.fingerprint,
+              location_fingerprint: location&.fingerprint,
+              project_id: report.project_id
+            }
+
+            if uuid_v5_name_components.values.any?(&:nil?)
+              Gitlab::AppLogger.warn(message: "One or more UUID name components are nil", components: uuid_v5_name_components)
+              return
+            end
+
+            name = uuid_v5_name_components.values.join('-')
+
+            Gitlab::UUID.v5(name)
+          end
         end
       end
     end

ee/lib/gitlab/ci/reports/security/report.rb

@@ -5,16 +5,10 @@ module Gitlab
     module Reports
       module Security
         class Report
-          attr_reader :created_at
-          attr_reader :type
-          attr_reader :pipeline
-          attr_reader :findings
-          attr_reader :scanners
-          attr_reader :identifiers
+          attr_reader :created_at, :type, :pipeline, :findings, :scanners, :identifiers
+          attr_accessor :scan, :scanned_resources, :error
 
-          attr_accessor :scan
-          attr_accessor :scanned_resources
-          attr_accessor :error
+          delegate :project_id, to: :pipeline
 
           def initialize(type, pipeline, created_at)
             @type = type

ee/lib/gitlab/vulnerabilities/calculate_finding_uuid.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Vulnerabilities
-    class CalculateFindingUUID
-      FINDING_NAMESPACES_IDS = {
-        development: "a143e9e2-41b3-47bc-9a19-081d089229f4",
-        test: "a143e9e2-41b3-47bc-9a19-081d089229f4",
-        staging: "a6930898-a1b2-4365-ab18-12aa474d9b26",
-        production:  "58dc0f06-936c-43b3-93bb-71693f1b6570"
-      }.freeze
-
-      NAMESPACE_REGEX = /(\h{8})-(\h{4})-(\h{4})-(\h{4})-(\h{4})(\h{8})/.freeze
-      PACK_PATTERN = "NnnnnN".freeze
-
-      def self.call(value)
-        Digest::UUID.uuid_v5(namespace_id, value)
-      end
-
-      def self.namespace_id
-        namespace_uuid = FINDING_NAMESPACES_IDS.fetch(Rails.env.to_sym)
-        # Digest::UUID is broken when using an UUID in namespace_id
-        # https://github.com/rails/rails/issues/37681#issue-520718028
-        namespace_uuid.scan(NAMESPACE_REGEX).flatten.map { |s| s.to_i(16) }.pack(PACK_PATTERN)
-      end
-    end
-  end
-end

ee/spec/finders/security/findings_finder_spec.rb

@@ -91,7 +91,7 @@ RSpec.describe Security::FindingsFinder do
         end
 
         it 'does not cause N+1 queries' do
-          expect { finder_result }.not_to exceed_query_limit(7)
+          expect { finder_result }.not_to exceed_query_limit(9)
         end
 
         describe '#current_page' do

ee/spec/fixtures/security_reports/master/gl-common-scanning-report.json

@@ -13,7 +13,13 @@
                 "name": "Gemnasium"
             },
             "location": {},
-            "identifiers": [],
+            "identifiers": [
+                {
+                    "type": "GitLab",
+                    "name": "Foo vulnerability",
+                    "value": "foo"
+                }
+            ],
             "links": [
                 {
                     "url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1020"
@@ -52,7 +58,13 @@
                 "name": "Gemnasium"
             },
             "location": {},
-            "identifiers": [],
+            "identifiers": [
+                {
+                    "type": "GitLab",
+                    "name": "Bar vulnerability",
+                    "value": "bar"
+                }
+            ],
             "links": [
                 {
                   "name": "CVE-1030",

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

@@ -13,12 +13,11 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
 
     before do
       allow(parser).to receive(:create_location).and_return(location)
-      artifact.each_blob do |blob|
-        parser.parse!(blob, report)
-      end
+
+      artifact.each_blob { |blob| parser.parse!(blob, report) }
     end
 
-    context 'parsing finding.name' do
+    describe 'parsing finding.name' do
       let(:artifact) { build(:ee_ci_job_artifact, :common_security_report_with_blank_names) }
 
       context 'when message is provided' do
@@ -65,9 +64,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
       end
     end
 
-    context 'parsing finding.details' do
-      let(:artifact) { build(:ee_ci_job_artifact, :common_security_report) }
-
+    describe 'parsing finding.details' do
       context 'when details are provided' do
         it 'sets details from the report' do
           vulnerability = report.findings.find { |x| x.compare_key == 'CVE-1020' }
@@ -85,7 +82,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
       end
     end
 
-    context 'parsing remediations' do
+    describe 'parsing remediations' do
       let(:expected_remediation) { create(:ci_reports_security_remediation, diff: '') }
 
       it 'finds remediation with same cve' do
@@ -122,7 +119,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
       end
     end
 
-    context 'parsing scanners' do
+    describe 'parsing scanners' do
       subject(:scanner) { report.findings.first.scanner }
 
       context 'when vendor is not missing in scanner' do
@@ -132,7 +129,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
       end
     end
 
-    context 'parsing scan' do
+    describe 'parsing scan' do
       it 'returns scan object for each finding' do
         scans = report.findings.map(&:scan)
 
@@ -153,7 +150,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
       end
     end
 
-    context 'parsing links' do
+    describe 'parsing links' do
       it 'returns links object for each finding', :aggregate_failures do
         links = report.findings.flat_map(&:links)
 
@@ -163,5 +160,18 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
         expect(links.first).to be_a(::Gitlab::Ci::Reports::Security::Link)
       end
     end
+
+    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(:expected_uuids) { [uuid_1, uuid_2, nil] }
+
+      it 'sets the UUIDv5 for findings', :aggregate_failures do
+        expect(finding_uuids).to match_array(expected_uuids)
+      end
+    end
   end
 end

ee/spec/lib/gitlab/ci/reports/security/report_spec.rb

@@ -5,10 +5,12 @@ require 'spec_helper'
 RSpec.describe Gitlab::Ci::Reports::Security::Report do
   let_it_be(:pipeline) { create(:ci_pipeline) }
 
-  let(:report) { described_class.new('sast', pipeline, created_at) }
   let(:created_at) { 2.weeks.ago }
 
+  subject(:report) { described_class.new('sast', pipeline, created_at) }
+
   it { expect(report.type).to eq('sast') }
+  it { is_expected.to delegate_method(:project_id).to(:pipeline) }
 
   describe '#add_scanner' do
     let(:scanner) { create(:ci_reports_security_scanner, external_id: 'find_sec_bugs') }

ee/spec/lib/gitlab/vulnerabilities/calculate_finding_uuid_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Vulnerabilities::CalculateFindingUUID do
-  let_it_be(:value) { "GitLab" }
-
-  subject { described_class.call(value) }
-
-  context 'in development' do
-    let_it_be(:development_proper_uuid) { "5b593e54-90f5-504b-8805-5394a4d14b94" }
-
-    before do
-      allow(Rails).to receive(:env).and_return(:development)
-    end
-
-    it { is_expected.to eq(development_proper_uuid) }
-  end
-
-  context 'in test' do
-    let_it_be(:test_proper_uuid) { "5b593e54-90f5-504b-8805-5394a4d14b94" }
-
-    it { is_expected.to eq(test_proper_uuid) }
-  end
-
-  context 'in staging' do
-    let_it_be(:staging_proper_uuid) { "dd190b37-7754-5c7c-80a0-85621a5823ad" }
-
-    before do
-      allow(Rails).to receive(:env).and_return(:staging)
-    end
-
-    it { is_expected.to eq(staging_proper_uuid) }
-  end
-
-  context 'in production' do
-    let_it_be(:production_proper_uuid) { "4961388b-9d8e-5da0-a499-3ef5da58daf0" }
-
-    before do
-      allow(Rails).to receive(:env).and_return(:production)
-    end
-
-    it { is_expected.to eq(production_proper_uuid) }
-  end
-end

ee/spec/services/security/store_report_service_spec.rb

@@ -2,19 +2,6 @@
 
 require 'spec_helper'
 
-UUID_REGEXP = Regexp.new("^([0-9a-f]{8})-([0-9a-f]{4})-([0-9a-f]{4})-" \
-                         "([0-9a-f]{2})([0-9a-f]{2})-([0-9a-f]{12})$").freeze
-
-RSpec::Matchers.define :be_uuid_v5 do
-  match do |string|
-    expect(string).to be_a(String)
-
-    uuid_components = string.downcase.scan(UUID_REGEXP).first
-    time_hi_and_version = uuid_components[2].to_i(16)
-    (time_hi_and_version >> 12) == 5
-  end
-end
-
 RSpec.describe Security::StoreReportService, '#execute' do
   let_it_be(:user) { create(:user) }
   let(:artifact) { create(:ee_ci_job_artifact, trait) }
@@ -78,12 +65,6 @@ RSpec.describe Security::StoreReportService, '#execute' do
       it 'inserts all vulnerabilties' do
         expect { subject }.to change { Vulnerability.count }.by(findings)
       end
-
-      it 'calculates UUIDv5 for all findings' do
-        subject
-        uuids = Vulnerabilities::Finding.pluck(:uuid)
-        expect(uuids).to all(be_uuid_v5)
-      end
     end
 
     context 'invalid data' do
@@ -149,10 +130,6 @@ RSpec.describe Security::StoreReportService, '#execute' do
       expect { subject }.to change { Vulnerabilities::Finding.count }.by(32)
     end
 
-    it 'calculates UUIDv5 for all findings' do
-      expect(Vulnerabilities::Finding.pluck(:uuid)).to all(be_a(String))
-    end
-
     it 'inserts all finding pipelines (join model) for this new pipeline' do
       expect { subject }.to change { Vulnerabilities::FindingPipeline.where(pipeline: new_pipeline).count }.by(33)
     end

lib/gitlab/uuid.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class UUID
+    NAMESPACE_IDS = {
+      development: "a143e9e2-41b3-47bc-9a19-081d089229f4",
+      test: "a143e9e2-41b3-47bc-9a19-081d089229f4",
+      staging: "a6930898-a1b2-4365-ab18-12aa474d9b26",
+      production: "58dc0f06-936c-43b3-93bb-71693f1b6570"
+    }.freeze
+
+    NAMESPACE_REGEX = /(\h{8})-(\h{4})-(\h{4})-(\h{4})-(\h{4})(\h{8})/.freeze
+    PACK_PATTERN = "NnnnnN".freeze
+
+    class << self
+      def v5(name, namespace_id: default_namespace_id)
+        Digest::UUID.uuid_v5(namespace_id, name)
+      end
+
+      private
+
+      def default_namespace_id
+        @default_namespace_id ||= begin
+          namespace_uuid = NAMESPACE_IDS.fetch(Rails.env.to_sym)
+          # Digest::UUID is broken when using a UUID as a namespace_id
+          # https://github.com/rails/rails/issues/37681#issue-520718028
+          namespace_uuid.scan(NAMESPACE_REGEX).flatten.map { |s| s.to_i(16) }.pack(PACK_PATTERN)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/uuid_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::UUID do
+  let_it_be(:name) { "GitLab" }
+
+  describe '.v5' do
+    subject { described_class.v5(name) }
+
+    before do
+      # This is necessary to clear memoization for testing different environments
+      described_class.instance_variable_set(:@default_namespace_id, nil)
+    end
+
+    context 'in development' do
+      let_it_be(:development_proper_uuid) { "5b593e54-90f5-504b-8805-5394a4d14b94" }
+
+      before do
+        allow(Rails).to receive(:env).and_return(:development)
+      end
+
+      it { is_expected.to eq(development_proper_uuid) }
+    end
+
+    context 'in test' do
+      let_it_be(:test_proper_uuid) { "5b593e54-90f5-504b-8805-5394a4d14b94" }
+
+      it { is_expected.to eq(test_proper_uuid) }
+    end
+
+    context 'in staging' do
+      let_it_be(:staging_proper_uuid) { "dd190b37-7754-5c7c-80a0-85621a5823ad" }
+
+      before do
+        allow(Rails).to receive(:env).and_return(:staging)
+      end
+
+      it { is_expected.to eq(staging_proper_uuid) }
+    end
+
+    context 'in production' do
+      let_it_be(:production_proper_uuid) { "4961388b-9d8e-5da0-a499-3ef5da58daf0" }
+
+      before do
+        allow(Rails).to receive(:env).and_return(:production)
+      end
+
+      it { is_expected.to eq(production_proper_uuid) }
+    end
+  end
+end