Merge branch 'send_warning_to_sentry_if_finding_is_not_valid' into 'master'

Write warning log if finding is not valid

See merge request gitlab-org/gitlab!41321

ee/app/services/security/store_report_service.rb

@@ -42,7 +42,10 @@ module Security
     end
 
     def create_vulnerability_finding(finding)
-      return if finding.scanner.blank? || finding.primary_identifier.blank?
+      unless finding.valid?
+        put_warning_for(finding)
+        return
+      end
 
       vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner)
       vulnerability_finding = create_or_find_vulnerability_finding(finding, vulnerability_params)
@@ -172,5 +175,9 @@ module Security
         end.to_h
       end
     end
+
+    def put_warning_for(finding)
+      Gitlab::AppLogger.warn(message: "Invalid vulnerability finding record found", finding: finding.to_hash)
+    end
   end
 end

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

@@ -81,6 +81,10 @@ module Gitlab
             report_type.hash ^ location.fingerprint.hash ^ primary_identifier.fingerprint.hash
           end
 
+          def valid?
+            scanner.present? && primary_identifier.present? && location.present?
+          end
+
           private
 
           def generate_project_fingerprint

ee/spec/factories/ci/job_artifacts.rb

@@ -179,22 +179,6 @@ FactoryBot.define do
       end
     end
 
-    trait :sast_with_missing_identifiers do
-      file_type { :sast }
-      file_format { :raw }
-
-      after(:build) do |artifact, _|
-        file = fixture_file_upload(
-          Rails.root.join('ee/spec/fixtures/security_reports/master/gl-sast-report.json'), 'application/json')
-        data = Gitlab::Json.parse(file.tempfile.read)['vulnerabilities'].each { |v| v.delete('identifiers') }.to_json
-        output = Tempfile.new("gl-sast-missing-identifiers")
-        output.write(data)
-        artifact.file = fixture_file_upload(output.path, 'application/json')
-        output.close
-        output.unlink
-      end
-    end
-
     trait :license_management do
       to_create { |instance| instance.save!(validate: false) }
 

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

@@ -228,4 +228,42 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
       end
     end
   end
+
+  describe '#valid?' do
+    let(:scanner) { build(:ci_reports_security_scanner) }
+    let(:identifiers) { [build(:ci_reports_security_identifier)] }
+    let(:location) { build(:ci_reports_security_locations_sast) }
+
+    let(:finding) do
+      build(:ci_reports_security_finding,
+            scanner: scanner,
+            identifiers: identifiers,
+            location: location,
+            compare_key: '')
+    end
+
+    subject { finding.valid? }
+
+    context 'when the scanner is missing' do
+      let(:scanner) { nil }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when there is no identifier' do
+      let(:identifiers) { [] }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when the location is missing' do
+      let(:location) { nil }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when all required attributes present' do
+      it { is_expected.to be_truthy }
+    end
+  end
 end

ee/spec/services/security/store_report_service_spec.rb

@@ -181,20 +181,14 @@ RSpec.describe Security::StoreReportService, '#execute' do
       end
     end
 
-    context 'when the finding does not include a scanner' do
-      let(:bad_pipeline) { create(:ci_pipeline, project: project) }
-      let(:bad_build) { create(:ci_build, pipeline: bad_pipeline) }
-      let!(:bad_artifact) { create(:ee_ci_job_artifact, :sast_with_missing_scanner, job: bad_build) }
-      let(:bad_report) { bad_pipeline.security_reports.get_report(report_type.to_s, bad_artifact) }
-      let(:report_type) { :sast }
-
+    context 'when the finding is not valid' do
       before do
-        project.add_developer(user)
-        allow(bad_pipeline).to receive(:user).and_return(user)
+        allow(Gitlab::AppLogger).to receive(:warn)
+        allow_next_instance_of(::Gitlab::Ci::Reports::Security::Finding) do |finding|
+          allow(finding).to receive(:valid?).and_return(false)
+        end
       end
 
-      subject { described_class.new(bad_pipeline, bad_report).execute }
-
       it 'does not create a new finding' do
         expect { subject }.not_to change { Vulnerabilities::Finding.count }
       end
@@ -202,28 +196,11 @@ RSpec.describe Security::StoreReportService, '#execute' do
       it 'does not raise an error' do
         expect { subject }.not_to raise_error
       end
-    end
-
-    context 'when the finding does not include a primary identifier' do
-      let(:bad_project) { bad_artifact.project }
-      let(:bad_pipeline) { bad_artifact.job.pipeline }
-      let!(:bad_artifact) { create(:ee_ci_job_artifact, :sast_with_missing_identifiers) }
-      let(:bad_report) { bad_pipeline.security_reports.get_report(report_type.to_s, bad_artifact) }
-      let(:report_type) { :sast }
-
-      before do
-        bad_project.add_developer(user)
-        allow(bad_pipeline).to receive(:user).and_return(user)
-      end
 
-      subject { described_class.new(bad_pipeline, bad_report).execute }
+      it 'puts a warning log' do
+        subject
 
-      it 'does not create a new finding' do
-        expect { subject }.not_to change { Vulnerabilities::Finding.count }
-      end
-
-      it 'does not raise an error' do
-        expect { subject }.not_to raise_error
+        expect(Gitlab::AppLogger).to have_received(:warn).exactly(new_report.findings.length).times
       end
     end
   end