Merge branch '214987-update-vulnerability-on-pipeline-updates' into 'master'

Update Vulnerability when Finding is updated

See merge request gitlab-org/gitlab!35374

ee/app/services/security/store_report_service.rb

@@ -36,10 +36,15 @@ module Security
     end
 
     def create_vulnerability_finding(occurrence)
-      vulnerability_finding = create_or_find_vulnerability_finding(occurrence)
+      vulnerability_params = occurrence.to_hash.except(:compare_key, :identifiers, :location, :scanner)
+      vulnerability_finding = create_or_find_vulnerability_finding(occurrence, vulnerability_params)
+
+      update_vulnerability_scanner(occurrence)
+
+      update_vulnerability_finding(vulnerability_finding, vulnerability_params)
 
       occurrence.identifiers.map do |identifier|
-        create_vulnerability_identifier_object(vulnerability_finding, identifier)
+        create_or_update_vulnerability_identifier_object(vulnerability_finding, identifier)
       end
 
       create_vulnerability_pipeline_object(vulnerability_finding, pipeline)
@@ -48,16 +53,13 @@ module Security
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def create_or_find_vulnerability_finding(occurrence)
+    def create_or_find_vulnerability_finding(occurrence, create_params)
       find_params = {
         scanner: scanners_objects[occurrence.scanner.key],
         primary_identifier: identifiers_objects[occurrence.primary_identifier.key],
         location_fingerprint: occurrence.location.fingerprint
       }
 
-      create_params = occurrence.to_hash
-        .except(:compare_key, :identifiers, :location, :scanner)
-
       begin
         project
           .vulnerability_findings
@@ -69,23 +71,35 @@ module Security
         Gitlab::ErrorTracking.track_and_raise_exception(e, create_params: create_params&.dig(:raw_metadata))
       end
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    def create_vulnerability_identifier_object(vulnerability_finding, identifier)
-      vulnerability_finding.occurrence_identifiers.find_or_create_by!( # rubocop: disable CodeReuse/ActiveRecord
-        identifier: identifiers_objects[identifier.key])
+    def update_vulnerability_scanner(occurrence)
+      scanner = scanners_objects[occurrence.scanner.key]
+      scanner.update!(occurrence.scanner.to_hash)
+    end
+
+    def update_vulnerability_finding(vulnerability_finding, update_params)
+      vulnerability_finding.update!(update_params)
+    end
+
+    def create_or_update_vulnerability_identifier_object(vulnerability_finding, identifier)
+      identifier_object = identifiers_objects[identifier.key]
+      vulnerability_finding.occurrence_identifiers.find_or_create_by!(identifier: identifier_object)
+      identifier_object.update!(identifier.to_hash)
     rescue ActiveRecord::RecordNotUnique
     end
 
     def create_vulnerability_pipeline_object(vulnerability_finding, pipeline)
-      vulnerability_finding.occurrence_pipelines.find_or_create_by!(pipeline: pipeline) # rubocop: disable CodeReuse/ActiveRecord
+      vulnerability_finding.occurrence_pipelines.find_or_create_by!(pipeline: pipeline)
     rescue ActiveRecord::RecordNotUnique
     end
+    # rubocop: enable CodeReuse/ActiveRecord
 
     def create_vulnerability(vulnerability_finding, pipeline)
-      return if vulnerability_finding.vulnerability_id
-
-      Vulnerabilities::CreateService.new(vulnerability_finding.project, pipeline.user, finding_id: vulnerability_finding.id).execute
+      if vulnerability_finding.vulnerability_id
+        Vulnerabilities::UpdateService.new(vulnerability_finding.project, pipeline.user, finding: vulnerability_finding).execute
+      else
+        Vulnerabilities::CreateService.new(vulnerability_finding.project, pipeline.user, finding_id: vulnerability_finding.id).execute
+      end
     end
 
     def scanners_objects

ee/app/services/vulnerabilities/update_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class UpdateService
+    include Gitlab::Allowable
+
+    attr_reader :project, :author, :finding
+
+    delegate :vulnerability, to: :finding
+
+    def initialize(project, author, finding:)
+      @project = project
+      @author = author
+      @finding = finding
+    end
+
+    def execute
+      raise Gitlab::Access::AccessDeniedError unless can?(author, :create_vulnerability, project)
+
+      vulnerability.update!(vulnerability_params)
+
+      vulnerability
+    end
+
+    private
+
+    def vulnerability_params
+      {
+        title: finding.name,
+        severity: vulnerability.severity_overridden? ? vulnerability.severity : finding.severity,
+        confidence: vulnerability.confidence_overridden? ? vulnerability.confidence : finding.confidence
+      }
+    end
+  end
+end

ee/changelogs/unreleased/214987-update-vulnerability-on-pipeline-updates.yml

+---
+title: Update vulnerabilities and findings when report occurrences are updated
+merge_request: 35374
+author:
+type: added

ee/spec/services/security/store_report_service_spec.rb

@@ -97,6 +97,8 @@ RSpec.describe Security::StoreReportService, '#execute' do
         location_fingerprint: 'd869ba3f0b3347eb2749135a437dc07c8ae0f420')
     end
 
+    let!(:vulnerability) { create(:vulnerability, findings: [occurrence], project: project) }
+
     before do
       project.add_developer(user)
       allow(new_pipeline).to receive(:user).and_return(user)
@@ -119,6 +121,20 @@ RSpec.describe Security::StoreReportService, '#execute' do
     it 'inserts all occurrence pipelines (join model) for this new pipeline' do
       expect { subject }.to change { Vulnerabilities::OccurrencePipeline.where(pipeline: new_pipeline).count }.by(33)
     end
+
+    it 'inserts new vulnerabilities with data from findings from this new pipeline' do
+      expect { subject }.to change { Vulnerability.count }.by(32)
+    end
+
+    it 'updates existing occurrences with new data' do
+      subject
+      expect(occurrence.reload).to have_attributes(severity: 'medium', name: 'Probable insecure usage of temp file/directory.')
+    end
+
+    it 'updates existing vulnerability with new data' do
+      subject
+      expect(vulnerability.reload).to have_attributes(severity: 'medium', title: 'Probable insecure usage of temp file/directory.', title_html: 'Probable insecure usage of temp file/directory.')
+    end
   end
 
   context 'with existing data from same pipeline' do

ee/spec/services/vulnerabilities/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::UpdateService do
+  before do
+    stub_licensed_features(security_dashboard: true)
+  end
+
+  let_it_be(:user) { create(:user) }
+  let!(:project) { create(:project) } # cannot use let_it_be here: caching causes problems with permission-related tests
+  let!(:updated_finding) { create(:vulnerabilities_occurrence, project: project, name: 'New title', severity: :critical, confidence: :confirmed, vulnerability: vulnerability) }
+
+  let!(:vulnerability) { create(:vulnerability, project: project, severity: :low, severity_overridden: severity_overridden, confidence: :ignore, confidence_overridden: confidence_overridden) }
+  let(:severity_overridden) { false }
+  let(:confidence_overridden) { false }
+
+  subject { described_class.new(project, user, finding: updated_finding).execute }
+
+  context 'with an authorized user with proper permissions' do
+    before do
+      project.add_developer(user)
+    end
+
+    context 'when neither severity nor confidence are overridden' do
+      it 'updates the vulnerability from updated finding (title, severity and confidence only)', :aggregate_failures do
+        expect { subject }.not_to change { project.vulnerabilities.count }
+        expect(vulnerability.previous_changes.keys).to contain_exactly(*%w[updated_at title title_html severity confidence])
+        expect(vulnerability).to(
+          have_attributes(
+            title: 'New title',
+            severity: 'critical',
+            confidence: 'confirmed'
+          ))
+      end
+    end
+
+    context 'when severity is overridden' do
+      let(:severity_overridden) { true }
+
+      it 'updates the vulnerability from updated finding (title and confidence only)' do
+        expect { subject }.not_to change { project.vulnerabilities.count }
+        expect(vulnerability.previous_changes.keys).to contain_exactly(*%w[updated_at title title_html confidence])
+        expect(vulnerability).to(
+          have_attributes(
+            title: 'New title',
+            confidence: 'confirmed'
+          ))
+      end
+    end
+
+    context 'when confidence is overridden' do
+      let(:confidence_overridden) { true }
+
+      it 'updates the vulnerability from updated finding (title and severity only)' do
+        expect { subject }.not_to change { project.vulnerabilities.count }
+        expect(vulnerability.previous_changes.keys).to contain_exactly(*%w[updated_at title title_html severity])
+        expect(vulnerability).to(
+          have_attributes(
+            title: 'New title',
+            severity: 'critical'
+          ))
+      end
+    end
+
+    context 'when security dashboard feature is disabled' do
+      before do
+        stub_licensed_features(security_dashboard: false)
+      end
+
+      it 'raises an "access denied" error' do
+        expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
+      end
+    end
+  end
+
+  context 'when user does not have rights to update a vulnerability' do
+    before do
+      project.add_reporter(user)
+    end
+
+    it 'raises an "access denied" error' do
+      expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
+    end
+  end
+end