Automatic merge of gitlab-org/gitlab master

ee/app/services/security/store_report_service.rb

@@ -139,11 +139,11 @@ module Security
         # This might happen if we're processing another report in parallel and it finds the same Finding
         # faster. In that case we need to perform the lookup again
 
-        by_uuid = project.vulnerability_findings.reset.find_by(uuid: finding.uuid)
-        return by_uuid if by_uuid
+        vulnerability_finding = project.vulnerability_findings.reset.find_by(uuid: finding.uuid)
+        return vulnerability_finding if vulnerability_finding
 
-        by_find_params = project.vulnerability_findings.reset.find_by(find_params)
-        return by_find_params if by_find_params
+        vulnerability_finding = project.vulnerability_findings.reset.find_by(find_params)
+        return vulnerability_finding if vulnerability_finding
 
         Gitlab::ErrorTracking.track_and_raise_exception(e, find_params: find_params, uuid: finding.uuid)
       rescue ActiveRecord::ActiveRecordError => e
@@ -175,32 +175,46 @@ module Security
       begin
         vulnerability_finding = matched_findings.first
         if vulnerability_finding.nil?
-          find_params[:uuid] = finding.uuid
           vulnerability_finding = project
             .vulnerability_findings
-            .create_with(create_params)
-            .find_or_initialize_by(find_params)
+            .create_with(create_params.merge(find_params))
+            .new
         end
 
-        vulnerability_finding.uuid = finding.uuid
-
-        vulnerability_finding.location_fingerprint = if finding.signatures.empty?
-                                                       finding.location.fingerprint
-                                                     else
-                                                       finding.signatures.max_by(&:priority).signature_hex
-                                                     end
-
-        vulnerability_finding.location = create_params.dig(:location)
+        sync_vulnerability_finding(vulnerability_finding, finding, create_params.dig(:location))
         vulnerability_finding.save!
 
         vulnerability_finding
-      rescue ActiveRecord::RecordNotUnique
-        get_matched_findings(finding, normalized_signatures, find_params).first
+      rescue ActiveRecord::RecordNotUnique => e
+        # the uuid is the only unique constraint on the vulnerability_occurrences
+        # table - no need to use get_matched_findings(...).first here. Fetching
+        # the finding with the same uuid will be enough
+        vulnerability_finding = project.vulnerability_findings.reset.find_by(uuid: finding.uuid)
+        if vulnerability_finding
+          sync_vulnerability_finding(vulnerability_finding, finding, create_params.dig(:location))
+          vulnerability_finding.save!
+          return vulnerability_finding
+        end
+
+        Gitlab::ErrorTracking.track_and_raise_exception(e, find_params: find_params, uuid: finding.uuid)
       rescue ActiveRecord::RecordInvalid => e
         Gitlab::ErrorTracking.track_and_raise_exception(e, create_params: create_params&.dig(:raw_metadata))
       end
     end
 
+    def sync_vulnerability_finding(vulnerability_finding, finding, location_data)
+      vulnerability_finding.uuid = finding.uuid
+      vulnerability_finding.location_fingerprint = fingerprint_for(finding)
+
+      vulnerability_finding.location = location_data
+    end
+
+    def fingerprint_for(finding)
+      return finding.location.fingerprint if finding.signatures.empty?
+
+      finding.signatures.max_by(&:priority).signature_hex
+    end
+
     def update_vulnerability_scanner(finding)
       scanner = scanners_objects[finding.scanner.key]
       scanner.update!(finding.scanner.to_hash)

ee/spec/services/security/store_report_service_spec.rb

@@ -376,6 +376,45 @@ RSpec.describe Security::StoreReportService, '#execute' do
         expect(finding.reload).to have_attributes(severity: 'medium', name: 'Cipher with no integrity')
       end
 
+      context 'when RecordNotUnique error has been raised' do
+        let(:report_finding) { report.findings.find { |f| f.location.fingerprint == finding.location_fingerprint} }
+
+        subject(:store_report_service) { described_class.new(new_pipeline, new_report) }
+
+        before do
+          allow(store_report_service).to receive(:get_matched_findings).and_wrap_original do |orig_method, other_finding, *args|
+            if other_finding.uuid != report_finding.uuid
+              orig_method.call(other_finding, *args)
+            else
+              finding.update!(name: 'SHOULD BE UPDATED', uuid: report_finding.uuid)
+              []
+            end
+          end
+
+          allow(Gitlab::ErrorTracking).to receive(:track_and_raise_exception).and_call_original
+        end
+
+        it 'handles the error correctly' do
+          next unless vulnerability_finding_signatures_enabled
+
+          report_finding = report.findings.find { |f| f.location.fingerprint == finding.location_fingerprint}
+
+          store_report_service.execute
+
+          expect(finding.reload.name).to eq(report_finding.name)
+        end
+
+        it 'raises the error if there exists no vulnerability finding' do
+          next unless vulnerability_finding_signatures_enabled
+
+          allow(store_report_service).to receive(:sync_vulnerability_finding).and_raise(ActiveRecord::RecordNotUnique)
+
+          expect { store_report_service.execute }.to raise_error { |error|
+            expect(Gitlab::ErrorTracking).to have_received(:track_and_raise_exception).with(error, any_args)
+          }
+        end
+      end
+
       it 'updates signatures to match new values' do
         next unless vulnerability_finding_signatures_enabled
 

lib/gitlab/import_export/after_export_strategies/web_upload_strategy.rb

@@ -32,6 +32,10 @@ module Gitlab
           end
         end
 
+        def delete_export?
+          false
+        end
+
         private
 
         def send_file

spec/lib/gitlab/import_export/after_export_strategies/web_upload_strategy_spec.rb

@@ -27,20 +27,30 @@ RSpec.describe Gitlab::ImportExport::AfterExportStrategies::WebUploadStrategy do
       expect(subject.new(url: example_url, http_method: 'whatever')).not_to be_valid
     end
 
-    it 'onyl allow urls as upload urls' do
+    it 'only allow urls as upload urls' do
       expect(subject.new(url: example_url)).to be_valid
       expect(subject.new(url: 'whatever')).not_to be_valid
     end
   end
 
   describe '#execute' do
-    it 'removes the exported project file after the upload' do
-      allow(strategy).to receive(:send_file)
-      allow(strategy).to receive(:handle_response_error)
+    context 'when upload succeeds' do
+      before do
+        allow(strategy).to receive(:send_file)
+        allow(strategy).to receive(:handle_response_error)
+      end
+
+      it 'does not remove the exported project file after the upload' do
+        expect(project).not_to receive(:remove_exports)
 
-      expect(project).to receive(:remove_exports)
+        strategy.execute(user, project)
+      end
 
-      strategy.execute(user, project)
+      it 'has finished export status' do
+        strategy.execute(user, project)
+
+        expect(project.export_status).to eq(:finished)
+      end
     end
 
     context 'when upload fails' do