Merge branch '239174-add-vulnerability-link-model' into 'master'

Add Vulnerabilities::FindingLink model

See merge request gitlab-org/gitlab!46555

db/migrate/20201029144444_create_vulnerability_finding_links.rb

+# frozen_string_literal: true
+
+class CreateVulnerabilityFindingLinks < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    create_table :vulnerability_finding_links, if_not_exists: true do |t|
+      t.timestamps_with_timezone null: false
+      t.references :vulnerability_occurrence, index: { name: 'finding_links_on_vulnerability_occurrence_id' }, null: false, foreign_key: { on_delete: :cascade }
+      t.text :name, limit: 255
+      t.text :url, limit: 2048, null: false
+    end
+
+    add_text_limit :vulnerability_finding_links, :name, 255
+    add_text_limit :vulnerability_finding_links, :url, 2048
+  end
+
+  def down
+    drop_table :vulnerability_finding_links
+  end
+end

db/schema_migrations/20201029144444

+50e4e42c804d3abdcfe9ab2bbb890262d4b2ddd93bff1b2af1da1e55a0300cf5
\ No newline at end of file

db/structure.sql

@@ -17104,6 +17104,26 @@ CREATE SEQUENCE vulnerability_feedback_id_seq
 
 ALTER SEQUENCE vulnerability_feedback_id_seq OWNED BY vulnerability_feedback.id;
 
+CREATE TABLE vulnerability_finding_links (
+    id bigint NOT NULL,
+    created_at timestamp with time zone NOT NULL,
+    updated_at timestamp with time zone NOT NULL,
+    vulnerability_occurrence_id bigint NOT NULL,
+    name text,
+    url text NOT NULL,
+    CONSTRAINT check_55f0a95439 CHECK ((char_length(name) <= 255)),
+    CONSTRAINT check_b7fe886df6 CHECK ((char_length(url) <= 2048))
+);
+
+CREATE SEQUENCE vulnerability_finding_links_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE vulnerability_finding_links_id_seq OWNED BY vulnerability_finding_links.id;
+
 CREATE TABLE vulnerability_historical_statistics (
     id bigint NOT NULL,
     created_at timestamp with time zone NOT NULL,
@@ -18203,6 +18223,8 @@ ALTER TABLE ONLY vulnerability_exports ALTER COLUMN id SET DEFAULT nextval('vuln
 
 ALTER TABLE ONLY vulnerability_feedback ALTER COLUMN id SET DEFAULT nextval('vulnerability_feedback_id_seq'::regclass);
 
+ALTER TABLE ONLY vulnerability_finding_links ALTER COLUMN id SET DEFAULT nextval('vulnerability_finding_links_id_seq'::regclass);
+
 ALTER TABLE ONLY vulnerability_historical_statistics ALTER COLUMN id SET DEFAULT nextval('vulnerability_historical_statistics_id_seq'::regclass);
 
 ALTER TABLE ONLY vulnerability_identifiers ALTER COLUMN id SET DEFAULT nextval('vulnerability_identifiers_id_seq'::regclass);
@@ -19646,6 +19668,9 @@ ALTER TABLE ONLY vulnerability_exports
 ALTER TABLE ONLY vulnerability_feedback
     ADD CONSTRAINT vulnerability_feedback_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY vulnerability_finding_links
+    ADD CONSTRAINT vulnerability_finding_links_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY vulnerability_historical_statistics
     ADD CONSTRAINT vulnerability_historical_statistics_pkey PRIMARY KEY (id);
 
@@ -19870,6 +19895,8 @@ CREATE UNIQUE INDEX epic_user_mentions_on_epic_id_and_note_id_index ON epic_user
 
 CREATE UNIQUE INDEX epic_user_mentions_on_epic_id_index ON epic_user_mentions USING btree (epic_id) WHERE (note_id IS NULL);
 
+CREATE INDEX finding_links_on_vulnerability_occurrence_id ON vulnerability_finding_links USING btree (vulnerability_occurrence_id);
+
 CREATE INDEX idx_audit_events_on_entity_id_desc_author_id_created_at ON audit_events USING btree (entity_id, entity_type, id DESC, author_id, created_at);
 
 CREATE INDEX idx_ci_pipelines_artifacts_locked ON ci_pipelines USING btree (ci_ref_id, id) WHERE (locked = 1);
@@ -24195,6 +24222,9 @@ ALTER TABLE ONLY gpg_signatures
 ALTER TABLE ONLY board_group_recent_visits
     ADD CONSTRAINT fk_rails_ca04c38720 FOREIGN KEY (board_id) REFERENCES boards(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY vulnerability_finding_links
+    ADD CONSTRAINT fk_rails_cbdfde27ce FOREIGN KEY (vulnerability_occurrence_id) REFERENCES vulnerability_occurrences(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY issues_self_managed_prometheus_alert_events
     ADD CONSTRAINT fk_rails_cc5d88bbb0 FOREIGN KEY (issue_id) REFERENCES issues(id) ON DELETE CASCADE;
 

ee/app/finders/security/pipeline_vulnerabilities_finder.rb

@@ -75,7 +75,7 @@ module Security
     def normalize_report_findings(report_findings, vulnerabilities)
       report_findings.map do |report_finding|
         finding_hash = report_finding.to_hash
-          .except(:compare_key, :identifiers, :location, :scanner)
+          .except(:compare_key, :identifiers, :location, :scanner, :links)
 
         finding = Vulnerabilities::Finding.new(finding_hash)
         # assigning Vulnerabilities to Findings to enable the computed state
@@ -84,6 +84,9 @@ module Security
         finding.project = pipeline.project
         finding.sha = pipeline.sha
         finding.build_scanner(report_finding.scanner&.to_hash)
+        finding.finding_links = report_finding.links.map do |link|
+          Vulnerabilities::FindingLink.new(link.to_hash)
+        end
         finding.identifiers = report_finding.identifiers.map do |identifier|
           Vulnerabilities::Identifier.new(identifier.to_hash)
         end

ee/app/models/vulnerabilities/finding.rb

@@ -26,6 +26,8 @@ module Vulnerabilities
     has_many :finding_identifiers, class_name: 'Vulnerabilities::FindingIdentifier', inverse_of: :finding, foreign_key: 'occurrence_id'
     has_many :identifiers, through: :finding_identifiers, class_name: 'Vulnerabilities::Identifier'
 
+    has_many :finding_links, class_name: 'Vulnerabilities::FindingLink', inverse_of: :finding, foreign_key: 'vulnerability_occurrence_id'
+
     has_many :finding_pipelines, class_name: 'Vulnerabilities::FindingPipeline', inverse_of: :finding, foreign_key: 'occurrence_id'
     has_many :pipelines, through: :finding_pipelines, class_name: 'Ci::Pipeline'
 
@@ -256,7 +258,9 @@ module Vulnerabilities
     end
 
     def links
-      metadata.fetch('links', [])
+      return metadata.fetch('links', []) if finding_links.load.empty?
+
+      finding_links.as_json(only: [:name, :url])
     end
 
     def remediations

ee/app/models/vulnerabilities/finding_link.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class FindingLink < ApplicationRecord
+    self.table_name = 'vulnerability_finding_links'
+
+    belongs_to :finding, class_name: 'Vulnerabilities::Finding', inverse_of: :finding_identifiers, foreign_key: 'vulnerability_occurrence_id'
+
+    validates :finding, presence: true
+    validates :url, presence: true, length: { maximum: 255 }
+    validates :name, length: { maximum: 2048 }
+  end
+end

ee/app/services/security/store_report_service.rb

@@ -47,7 +47,7 @@ module Security
         return
       end
 
-      vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan)
+      vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan, :links)
       vulnerability_finding = create_or_find_vulnerability_finding(finding, vulnerability_params)
 
       update_vulnerability_scanner(finding)
@@ -60,6 +60,8 @@ module Security
         create_or_update_vulnerability_identifier_object(vulnerability_finding, identifier)
       end
 
+      create_or_update_vulnerability_links(finding, vulnerability_finding)
+
       create_vulnerability_pipeline_object(vulnerability_finding, pipeline)
 
       create_vulnerability(vulnerability_finding, pipeline)
@@ -125,6 +127,15 @@ module Security
     rescue ActiveRecord::RecordNotUnique
     end
 
+    def create_or_update_vulnerability_links(finding, vulnerability_finding)
+      return if finding.links.blank?
+
+      finding.links.each do |link|
+        vulnerability_finding.finding_links.safe_find_or_create_by!(link.to_hash)
+      end
+    rescue ActiveRecord::RecordNotUnique
+    end
+
     def create_vulnerability_pipeline_object(vulnerability_finding, pipeline)
       vulnerability_finding.finding_pipelines.find_or_create_by!(pipeline: pipeline)
     rescue ActiveRecord::RecordNotUnique

ee/changelogs/unreleased/239174-add-vulnerability-link-model.yml

+---
+title: Add Vulnerabilities::FindingLink model
+merge_request: 46555
+author:
+type: added

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

@@ -55,6 +55,7 @@ module Gitlab
 
           def create_vulnerability(report, data, version)
             identifiers = create_identifiers(report, data['identifiers'])
+            links = create_links(report, data['links'])
             report.add_finding(
               ::Gitlab::Ci::Reports::Security::Finding.new(
                 uuid: SecureRandom.uuid,
@@ -67,6 +68,7 @@ module Gitlab
                 scanner: create_scanner(report, data['scanner']),
                 scan: report&.scan,
                 identifiers: identifiers,
+                links: links,
                 raw_metadata: data.to_json,
                 metadata_version: version))
           end
@@ -106,6 +108,22 @@ module Gitlab
                 url: identifier['url']))
           end
 
+          def create_links(report, links)
+            return [] unless links.is_a?(Array)
+
+            links
+              .map { |link| create_link(report, link) }
+              .compact
+          end
+
+          def create_link(report, link)
+            return unless link.is_a?(Hash)
+
+            ::Gitlab::Ci::Reports::Security::Link.new(
+              name: link['name'],
+              url: link['url'])
+          end
+
           def parse_severity_level(input)
             return input if ::Vulnerabilities::Finding::SEVERITY_LEVELS.key?(input)
 

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

@@ -10,6 +10,7 @@ module Gitlab
           attr_reader :compare_key
           attr_reader :confidence
           attr_reader :identifiers
+          attr_reader :links
           attr_reader :location
           attr_reader :metadata_version
           attr_reader :name
@@ -24,10 +25,11 @@ module Gitlab
 
           delegate :file_path, :start_line, :end_line, to: :location
 
-          def initialize(compare_key:, identifiers:, 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: [], 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
+            @links = links
             @location = location
             @metadata_version = metadata_version
             @name = name
@@ -46,6 +48,7 @@ module Gitlab
               compare_key
               confidence
               identifiers
+              links
               location
               metadata_version
               name

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Reports
+      module Security
+        class Link
+          attr_accessor :name, :url
+
+          def initialize(name: nil, url: nil)
+            @name = name
+            @url = url
+          end
+
+          def to_hash
+            {
+              name: name,
+              url: url
+            }.compact
+          end
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :ci_reports_security_link, class: '::Gitlab::Ci::Reports::Security::Link' do
+    name { 'CVE-2020-0202' }
+    url { 'https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0202' }
+
+    skip_create
+
+    initialize_with do
+      ::Gitlab::Ci::Reports::Security::Link.new(**attributes)
+    end
+  end
+end

ee/spec/factories/vulnerabilities/finding_links.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :finding_link, class: 'Vulnerabilities::FindingLink' do
+    finding factory: :vulnerabilities_finding
+    name { 'CVE-2018-1234' }
+    url { 'http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-1234' }
+  end
+end

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

@@ -16,7 +16,7 @@
             "identifiers": [],
             "links": [
                 {
-                    "url": ""
+                    "url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1020"
                 }
             ]
         },
@@ -37,7 +37,8 @@
             "identifiers": [],
             "links": [
                 {
-                    "url": ""
+                  "name": "CVE-1030",
+                  "url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1030"
                 }
             ]
         },
@@ -56,12 +57,6 @@
             "location": {},
             "identifiers": [],
             "links": [
-                {
-                    "url": ""
-                },
-                {
-                    "url": ""
-                }
             ]
         }
     ],
@@ -122,4 +117,4 @@
         "end_time": "placeholder-value",
         "status": "success"
     }
-}
\ No newline at end of file
+}

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

@@ -78,5 +78,16 @@ RSpec.describe Gitlab::Ci::Parsers::Security::Common do
         expect(empty_report.scan).to be(nil)
       end
     end
+
+    context 'parsing links' do
+      it 'returns links object for each finding', :aggregate_failures do
+        links = report.findings.flat_map(&:links)
+
+        expect(links.map(&:url)).to match_array(['https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1020', 'https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1030'])
+        expect(links.map(&:name)).to match_array([nil, 'CVE-1030'])
+        expect(links.size).to eq(2)
+        expect(links.first).to be_a(::Gitlab::Ci::Reports::Security::Link)
+      end
+    end
   end
 end

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

@@ -10,6 +10,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
 
     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) }
 
@@ -18,6 +19,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
         compare_key: 'this_is_supposed_to_be_a_unique_value',
         confidence: :medium,
         identifiers: [primary_identifier, other_identifier],
+        links: [link],
         location: location,
         metadata_version: 'sast:1.0',
         name: 'Cipher with no integrity',
@@ -39,6 +41,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
           confidence: :medium,
           project_fingerprint: '9a73f32d58d87d94e3dc61c4c1a94803f6014258',
           identifiers: [primary_identifier, other_identifier],
+          links: [link],
           location: location,
           metadata_version: 'sast:1.0',
           name: 'Cipher with no integrity',
@@ -84,6 +87,7 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
         compare_key: occurrence.compare_key,
         confidence: occurrence.confidence,
         identifiers: occurrence.identifiers,
+        links: occurrence.links,
         location: occurrence.location,
         metadata_version: occurrence.metadata_version,
         name: occurrence.name,

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Reports::Security::Link do
+  subject(:security_link) { described_class.new(name: 'CVE-2020-0202', url: 'https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0202') }
+
+  describe '#initialize' do
+    context 'when all params are given' do
+      it 'initializes an instance' do
+        expect { subject }.not_to raise_error
+
+        expect(subject).to have_attributes(
+          name: 'CVE-2020-0202',
+          url: 'https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0202'
+        )
+      end
+    end
+
+    describe '#to_hash' do
+      it 'returns expected hash' do
+        expect(security_link.to_hash).to eq(
+          {
+            name: 'CVE-2020-0202',
+            url: 'https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-0202'
+          }
+        )
+      end
+    end
+  end
+end

ee/spec/models/vulnerabilities/finding_link_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::FindingLink do
+  describe 'associations' do
+    it { is_expected.to belong_to(:finding).class_name('Vulnerabilities::Finding') }
+  end
+
+  describe 'validations' do
+    let_it_be(:link) { create(:finding_link) }
+
+    it { is_expected.to validate_presence_of(:url) }
+    it { is_expected.to validate_length_of(:url).is_at_most(255) }
+    it { is_expected.to validate_length_of(:name).is_at_most(2048) }
+    it { is_expected.to validate_presence_of(:finding) }
+  end
+end

ee/spec/models/vulnerabilities/finding_spec.rb

@@ -16,6 +16,7 @@ RSpec.describe Vulnerabilities::Finding do
     it { is_expected.to have_many(:finding_pipelines).class_name('Vulnerabilities::FindingPipeline').with_foreign_key('occurrence_id') }
     it { is_expected.to have_many(:identifiers).class_name('Vulnerabilities::Identifier') }
     it { is_expected.to have_many(:finding_identifiers).class_name('Vulnerabilities::FindingIdentifier').with_foreign_key('occurrence_id') }
+    it { is_expected.to have_many(:finding_links).class_name('Vulnerabilities::FindingLink').with_foreign_key('vulnerability_occurrence_id') }
   end
 
   describe 'validations' do
@@ -405,6 +406,33 @@ RSpec.describe Vulnerabilities::Finding do
     end
   end
 
+  describe '#links' do
+    let_it_be(:finding, reload: true) do
+      create(
+        :vulnerabilities_finding,
+        raw_metadata: {
+          links: [{ url: 'https://raw.gitlab.com', name: 'raw_metadata_link' }]
+        }.to_json
+      )
+    end
+
+    subject(:links) { finding.links }
+
+    context 'when there are no finding links' do
+      it 'returns links from raw_metadata' do
+        expect(links).to eq([{ 'url' => 'https://raw.gitlab.com', 'name' => 'raw_metadata_link' }])
+      end
+    end
+
+    context 'when there are finding links assigned to given finding' do
+      let_it_be(:finding_link) { create(:finding_link, name: 'finding_link', url: 'https://link.gitlab.com', finding: finding) }
+
+      it 'returns links from finding link' do
+        expect(links).to eq([{ 'url' => 'https://link.gitlab.com', 'name' => 'finding_link' }])
+      end
+    end
+  end
+
   describe 'feedback' do
     let_it_be(:project) { create(:project) }
     let(:finding) do

ee/spec/services/security/store_report_service_spec.rb

@@ -24,11 +24,11 @@ RSpec.describe Security::StoreReportService, '#execute' do
 
     using RSpec::Parameterized::TableSyntax
 
-    where(:case_name, :trait, :scanners, :identifiers, :findings, :finding_identifiers, :finding_pipelines) do
-      'with SAST report'                | :sast                       | 3 | 17 | 33 | 39 | 33
-      'with exceeding identifiers'      | :with_exceeding_identifiers | 1 | 20 | 1  | 20 | 1
-      'with Dependency Scanning report' | :dependency_scanning        | 2 | 7  | 4  | 7  | 4
-      'with Container Scanning report'  | :container_scanning         | 1 | 8  | 8  | 8  | 8
+    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
     end
 
     with_them do