Merge branch '239177_create_remediation_entries' into 'master'

Create remediation entries while storing reports in database

See merge request gitlab-org/gitlab!48618

ee/app/models/vulnerabilities/remediation.rb

@@ -17,5 +17,11 @@ module Vulnerabilities
     validates :summary, presence: true, length: { maximum: 200 }
     validates :file, presence: true
     validates :checksum, presence: true
+
+    scope :by_checksum, -> (checksum) { where(checksum: checksum) }
+
+    def retrieve_upload(_identifier, paths)
+      Upload.find_by(model: self, path: paths)
+    end
   end
 end

ee/app/services/security/store_report_service.rb

@@ -54,6 +54,7 @@ module Security
       update_vulnerability_scanner(finding)
 
       update_vulnerability_finding(vulnerability_finding, vulnerability_params)
+      reset_remediations_for(vulnerability_finding, finding)
 
       # The maximum number of identifiers is not used in validation
       # we just want to ignore the rest if a finding has more than that.
@@ -133,6 +134,34 @@ module Security
     rescue ActiveRecord::RecordNotUnique
     end
 
+    def reset_remediations_for(vulnerability_finding, finding)
+      existing_remediations = find_existing_remediations_for(finding)
+      new_remediations = build_new_remediations_for(finding, existing_remediations)
+
+      vulnerability_finding.remediations = existing_remediations + new_remediations
+    end
+
+    def find_existing_remediations_for(finding)
+      checksums = finding.remediations.map(&:checksum)
+
+      Vulnerabilities::Remediation.by_checksum(checksums)
+    end
+
+    def build_new_remediations_for(finding, existing_remediations)
+      find_missing_remediations_for(finding, existing_remediations)
+        .map { |remediation| build_vulnerability_remediation(remediation) }
+    end
+
+    def find_missing_remediations_for(finding, existing_remediations)
+      existing_remediation_checksums = existing_remediations.map(&:checksum)
+
+      finding.remediations.select { |remediation| !remediation.checksum.in?(existing_remediation_checksums) }
+    end
+
+    def build_vulnerability_remediation(remediation)
+      Vulnerabilities::Remediation.new(summary: remediation.summary, file: remediation.diff_file, checksum: remediation.checksum)
+    end
+
     def create_vulnerability_pipeline_object(vulnerability_finding, pipeline)
       vulnerability_finding.finding_pipelines.find_or_create_by!(pipeline: pipeline)
     rescue ActiveRecord::RecordNotUnique

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

@@ -57,6 +57,7 @@ module Gitlab
             identifiers = create_identifiers(report, data['identifiers'])
             links = create_links(report, data['links'])
             location = create_location(data['location'] || {})
+            remediations = create_remediations(data['remediations'])
 
             report.add_finding(
               ::Gitlab::Ci::Reports::Security::Finding.new(
@@ -71,6 +72,7 @@ module Gitlab
                 scan: report&.scan,
                 identifiers: identifiers,
                 links: links,
+                remediations: remediations,
                 raw_metadata: data.to_json,
                 metadata_version: version))
           end
@@ -126,6 +128,12 @@ module Gitlab
               url: link['url'])
           end
 
+          def create_remediations(remediations_data)
+            remediations_data.to_a.compact.map do |remediation_data|
+              ::Gitlab::Ci::Reports::Security::Remediation.new(remediation_data['summary'], remediation_data['diff'])
+            end
+          end
+
           def parse_severity_level(input)
             return input if ::Vulnerabilities::Finding::SEVERITY_LEVELS.key?(input)
 

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

@@ -22,10 +22,11 @@ module Gitlab
           attr_reader :scan
           attr_reader :severity
           attr_reader :uuid
+          attr_reader :remediations
 
           delegate :file_path, :start_line, :end_line, to: :location
 
-          def initialize(compare_key:, identifiers:, links: [], location:, metadata_version:, name:, raw_metadata:, report_type:, scanner:, scan:, uuid:, confidence: nil, severity: nil) # rubocop:disable Metrics/ParameterLists
+          def initialize(compare_key:, identifiers:, links: [], remediations: [], location:, metadata_version:, name:, raw_metadata:, report_type:, scanner:, scan:, uuid:, confidence: nil, severity: nil) # rubocop:disable Metrics/ParameterLists
             @compare_key = compare_key
             @confidence = confidence
             @identifiers = identifiers
@@ -39,6 +40,7 @@ module Gitlab
             @scan = scan
             @severity = severity
             @uuid = uuid
+            @remediations = remediations
 
             @project_fingerprint = generate_project_fingerprint
           end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Reports
+      module Security
+        class Remediation
+          attr_reader :summary, :diff
+
+          def initialize(summary, diff)
+            @summary = summary
+            @diff = diff
+          end
+
+          def diff_file
+            @diff_file ||= DiffFile.new(diff)
+          end
+
+          delegate :checksum, to: :diff_file
+
+          class DiffFile < StringIO
+            # This method is used by the `carrierwave` gem
+            def original_filename
+              "#{checksum}.diff"
+            end
+
+            def checksum
+              @checksum ||= Digest::SHA256.hexdigest(string)
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/factories/ci/reports/security/remediations.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :ci_reports_security_remediation, class: '::Gitlab::Ci::Reports::Security::Remediation' do
+    summary { 'Remediation summary' }
+    diff { 'foo' }
+
+    skip_create
+
+    initialize_with do
+      ::Gitlab::Ci::Reports::Security::Remediation.new(summary, diff)
+    end
+  end
+end

ee/spec/factories/vulnerabilities/remediations.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :vulnerabilities_remediation, class: 'Vulnerabilities::Remediation' do
+    summary { 'Remediation Summary' }
+    file { Tempfile.new }
+
+    sequence :checksum do |i|
+      Digest::SHA256.hexdigest(i.to_s)
+    end
+  end
+end

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

@@ -66,16 +66,22 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
     end
 
     context 'parsing remediations' do
+      let(:expected_remediation) { create(:ci_reports_security_remediation, diff: '') }
+
       it 'finds remediation with same cve' do
         vulnerability = report.findings.find { |x| x.compare_key == "CVE-1020" }
         remediation = { 'fixes' => [{ 'cve' => 'CVE-1020' }], 'summary' => '', 'diff' => '' }
+
         expect(Gitlab::Json.parse(vulnerability.raw_metadata).dig('remediations').first).to include remediation
+        expect(vulnerability.remediations.first.checksum).to eq(expected_remediation.checksum)
       end
 
       it 'finds remediation with same id' do
         vulnerability = report.findings.find { |x| x.compare_key == "CVE-1030" }
         remediation = { 'fixes' => [{ 'cve' => 'CVE', 'id' => 'bb2fbeb1b71ea360ce3f86f001d4e84823c3ffe1a1f7d41ba7466b14cfa953d3' }], 'summary' => '', 'diff' => '' }
+
         expect(Gitlab::Json.parse(vulnerability.raw_metadata).dig('remediations').first).to include remediation
+        expect(vulnerability.remediations.first.checksum).to eq(expected_remediation.checksum)
       end
 
       it 'does not find remediation with different id' do

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

@@ -8,11 +8,12 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
   describe '#initialize' do
     subject { described_class.new(**params) }
 
-    let(:primary_identifier) { create(:ci_reports_security_identifier) }
-    let(:other_identifier) { create(:ci_reports_security_identifier) }
-    let(:link) { create(:ci_reports_security_link) }
-    let(:scanner) { create(:ci_reports_security_scanner) }
-    let(:location) { create(:ci_reports_security_locations_sast) }
+    let_it_be(:primary_identifier) { build(:ci_reports_security_identifier) }
+    let_it_be(:other_identifier) { build(:ci_reports_security_identifier) }
+    let_it_be(:link) { build(:ci_reports_security_link) }
+    let_it_be(:scanner) { build(:ci_reports_security_scanner) }
+    let_it_be(:location) { build(:ci_reports_security_locations_sast) }
+    let_it_be(:remediation) { build(:ci_reports_security_remediation) }
 
     let(:params) do
       {
@@ -20,6 +21,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
         confidence: :medium,
         identifiers: [primary_identifier, other_identifier],
         links: [link],
+        remediations: [remediation],
         location: location,
         metadata_version: 'sast:1.0',
         name: 'Cipher with no integrity',
@@ -42,6 +44,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
           project_fingerprint: '9a73f32d58d87d94e3dc61c4c1a94803f6014258',
           identifiers: [primary_identifier, other_identifier],
           links: [link],
+          remediations: [remediation],
           location: location,
           metadata_version: 'sast:1.0',
           name: 'Cipher with no integrity',

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Reports::Security::Remediation do
+  let(:diff) { 'foo' }
+  let(:remediation) { build(:ci_reports_security_remediation, diff: diff) }
+
+  describe '#diff_file' do
+    subject { remediation.diff_file.read }
+
+    it { is_expected.to eq(diff) }
+  end
+
+  describe '#checksum' do
+    let(:expected_checksum) { '2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae' }
+
+    subject { remediation.checksum }
+
+    it { is_expected.to eq(expected_checksum) }
+  end
+end

ee/spec/models/vulnerabilities/remediation_spec.rb

@@ -10,4 +10,13 @@ RSpec.describe Vulnerabilities::Remediation do
   it { is_expected.to validate_presence_of(:file) }
   it { is_expected.to validate_presence_of(:checksum) }
   it { is_expected.to validate_length_of(:summary).is_at_most(200) }
+
+  describe '.by_checksum' do
+    let_it_be(:remediation_1) { create(:vulnerabilities_remediation) }
+    let_it_be(:remediation_2) { create(:vulnerabilities_remediation) }
+
+    subject { described_class.by_checksum(remediation_2.checksum) }
+
+    it { is_expected.to match_array([remediation_2]) }
+  end
 end

ee/spec/services/security/store_report_service_spec.rb

@@ -37,11 +37,11 @@ RSpec.describe Security::StoreReportService, '#execute' do
 
     using RSpec::Parameterized::TableSyntax
 
-    where(:case_name, :trait, :scanners, :identifiers, :findings, :finding_identifiers, :finding_pipelines, :finding_links) do
-      'with SAST report'                | :sast                       | 3 | 17 | 33 | 39 | 33 | 0
-      'with exceeding identifiers'      | :with_exceeding_identifiers | 1 | 20 | 1  | 20 | 1  | 0
-      'with Dependency Scanning report' | :dependency_scanning        | 2 | 7  | 4  | 7  | 4  | 6
-      'with Container Scanning report'  | :container_scanning         | 1 | 8  | 8  | 8  | 8  | 8
+    where(:case_name, :trait, :scanners, :identifiers, :findings, :finding_identifiers, :finding_pipelines, :remediations) do
+      'with SAST report'                | :sast                            | 3 | 17 | 33 | 39 | 33 | 0
+      'with exceeding identifiers'      | :with_exceeding_identifiers      | 1 | 20 | 1  | 20 | 1  | 0
+      'with Dependency Scanning report' | :dependency_scanning_remediation | 1 | 3  | 2  | 3  | 2  | 1
+      'with Container Scanning report'  | :container_scanning              | 1 | 8  | 8  | 8  | 8  | 0
     end
 
     with_them do
@@ -65,6 +65,10 @@ RSpec.describe Security::StoreReportService, '#execute' do
         expect { subject }.to change { Vulnerabilities::FindingPipeline.count }.by(finding_pipelines)
       end
 
+      it 'inserts all remediations' do
+        expect { subject }.to change { Vulnerabilities::Remediation.count }.by(remediations)
+      end
+
       it 'inserts all vulnerabilties' do
         expect { subject }.to change { Vulnerability.count }.by(findings)
       end