Merge branch...

Merge branch '339507-determine-if-manual-vulnerability-creation-mutation-should-respect-security-scanners-schema' into 'master'

Adjust Mutations::Vulnerabilities::Create to respect security scanner schema

See merge request gitlab-org/gitlab!69961

doc/api/graphql/reference/index.md

@@ -4715,13 +4715,13 @@ Input type: `VulnerabilityCreateInput`
 | <a id="mutationvulnerabilitycreatedismissedat"></a>`dismissedAt` | [`Time`](#time) | Timestamp of when the vulnerability state changed to dismissed (defaults to creation time if status is `dismissed`). |
 | <a id="mutationvulnerabilitycreateidentifiers"></a>`identifiers` | [`[VulnerabilityIdentifierInput!]!`](#vulnerabilityidentifierinput) | Array of CVE or CWE identifiers for the vulnerability. |
 | <a id="mutationvulnerabilitycreatemessage"></a>`message` | [`String`](#string) | Additional information about the vulnerability. |
+| <a id="mutationvulnerabilitycreatename"></a>`name` | [`String!`](#string) | Name of the vulnerability. |
 | <a id="mutationvulnerabilitycreateproject"></a>`project` | [`ProjectID!`](#projectid) | ID of the project to attach the vulnerability to. |
 | <a id="mutationvulnerabilitycreateresolvedat"></a>`resolvedAt` | [`Time`](#time) | Timestamp of when the vulnerability state changed to resolved (defaults to creation time if status is `resolved`). |
-| <a id="mutationvulnerabilitycreatescannername"></a>`scannerName` | [`String!`](#string) | Name of the security scanner used to discover the vulnerability. |
+| <a id="mutationvulnerabilitycreatescanner"></a>`scanner` | [`VulnerabilityScannerInput!`](#vulnerabilityscannerinput) | Information about the scanner used to discover the vulnerability. |
 | <a id="mutationvulnerabilitycreateseverity"></a>`severity` | [`VulnerabilitySeverity`](#vulnerabilityseverity) | Severity of the vulnerability (defaults to `unknown`). |
 | <a id="mutationvulnerabilitycreatesolution"></a>`solution` | [`String`](#string) | How to fix this vulnerability. |
 | <a id="mutationvulnerabilitycreatestate"></a>`state` | [`VulnerabilityState`](#vulnerabilitystate) | State of the vulnerability (defaults to `detected`). |
-| <a id="mutationvulnerabilitycreatetitle"></a>`title` | [`String!`](#string) | Title of the vulnerability. |
 
 #### Fields
 
@@ -18313,3 +18313,23 @@ A time-frame defined as a closed inclusive range of two dates.
 | <a id="vulnerabilityidentifierinputexternaltype"></a>`externalType` | [`String`](#string) | External type of the vulnerability identifier. |
 | <a id="vulnerabilityidentifierinputname"></a>`name` | [`String!`](#string) | Name of the vulnerability identifier. |
 | <a id="vulnerabilityidentifierinputurl"></a>`url` | [`String!`](#string) | URL of the vulnerability identifier. |
+
+### `VulnerabilityScannerInput`
+
+#### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="vulnerabilityscannerinputid"></a>`id` | [`String!`](#string) | Unique ID that identifies the scanner. |
+| <a id="vulnerabilityscannerinputname"></a>`name` | [`String!`](#string) | Human readable value that identifies the analyzer, not required to be unique. |
+| <a id="vulnerabilityscannerinputurl"></a>`url` | [`String!`](#string) | Link to more information about the analyzer. |
+| <a id="vulnerabilityscannerinputvendor"></a>`vendor` | [`VulnerabilityScannerVendorInput`](#vulnerabilityscannervendorinput) | Information about vendor/maintainer of the scanner. |
+| <a id="vulnerabilityscannerinputversion"></a>`version` | [`String!`](#string) | Version of the scanner. |
+
+### `VulnerabilityScannerVendorInput`
+
+#### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="vulnerabilityscannervendorinputname"></a>`name` | [`String!`](#string) | Name of the vendor/maintainer. |

ee/app/graphql/mutations/vulnerabilities/create.rb

@@ -11,17 +11,17 @@ module Mutations
         required: true,
         description: 'ID of the project to attach the vulnerability to.'
 
-      argument :title, GraphQL::Types::String,
+      argument :name, GraphQL::Types::String,
         required: true,
-        description: 'Title of the vulnerability.'
+        description: 'Name of the vulnerability.'
 
       argument :description, GraphQL::Types::String,
         required: true,
         description: 'Description of the vulnerability.'
 
-      argument :scanner_name, GraphQL::Types::String,
+      argument :scanner, Types::VulnerabilityScannerInputType,
         required: true,
-        description: 'Name of the security scanner used to discover the vulnerability.'
+        description: 'Information about the scanner used to discover the vulnerability.'
 
       argument :identifiers, [Types::VulnerabilityIdentifierInputType],
         required: true,
@@ -100,7 +100,7 @@ module Mutations
 
       def build_vulnerability_params(params)
         vulnerability_params = params.slice(*%i[
-          title
+          name
           state
           severity
           confidence
@@ -111,15 +111,11 @@ module Mutations
           resolved_at
           dismissed_at
           identifiers
+          scanner
         ])
 
-        scanner_params = {
-          name: params.fetch(:scanner_name)
-        }
-
         {
           vulnerability: vulnerability_params
-            .merge(scanner: scanner_params)
         }
       end
     end

ee/app/graphql/types/vulnerability_scanner_input_type.rb

+# frozen_string_literal: true
+module Types
+  class VulnerabilityScannerInputType < BaseInputObject
+    argument :id, GraphQL::Types::String,
+          description: 'Unique ID that identifies the scanner.',
+          required: true
+
+    argument :name, GraphQL::Types::String,
+          description: 'Human readable value that identifies the analyzer, not required to be unique.',
+          required: true
+
+    argument :url, GraphQL::Types::String,
+          description: 'Link to more information about the analyzer.',
+          required: true
+
+    argument :vendor, Types::VulnerabilityScannerVendorInputType,
+          description: 'Information about vendor/maintainer of the scanner.',
+          required: false
+
+    argument :version, GraphQL::Types::String,
+          description: 'Version of the scanner.',
+          required: true
+  end
+end

ee/app/graphql/types/vulnerability_scanner_vendor_input_type.rb

+# frozen_string_literal: true
+module Types
+  class VulnerabilityScannerVendorInputType < BaseInputObject
+    argument :name, GraphQL::Types::String,
+          description: 'Name of the vendor/maintainer.',
+          required: true
+  end
+end

ee/app/services/vulnerabilities/create_service_base.rb

@@ -44,7 +44,12 @@ module Vulnerabilities
         .merge(
           project: @project,
           author: @author,
-          title: vulnerability_hash[:title]&.truncate(::Issuable::TITLE_LENGTH_MAX),
+          # Our security report schema has name
+          # https://gitlab.com/gitlab-org/security-products/security-report-schemas/-/blob/master/src/security-report-format.json#L369
+          # Our database has title
+          # https://gitlab.com/gitlab-org/gitlab/blob/master/db/structure.sql#L20164
+          # We want the GraphQL mutation arguments to reflect the security scanner schema
+          title: vulnerability_hash[:name]&.truncate(::Issuable::TITLE_LENGTH_MAX),
           report_type: report_type
         )
 
@@ -61,8 +66,8 @@ module Vulnerabilities
     def initialize_identifiers(identifier_hashes)
       identifier_hashes.map do |identifier|
         name = identifier[:name]
-        external_type = map_external_type_from_name(name)
-        external_id = name
+        external_type = identifier[:external_type] || map_external_type_from_name(name)
+        external_id = identifier[:external_id] || name
         fingerprint = Digest::SHA1.hexdigest("#{external_type}:#{external_id}")
         url = identifier[:url]
 

ee/app/services/vulnerabilities/manually_create_service.rb

@@ -66,11 +66,14 @@ module Vulnerabilities
 
     # rubocop: disable CodeReuse/ActiveRecord
     def initialize_scanner(scanner_hash)
-      name = scanner_hash[:name]
-
-      Vulnerabilities::Scanner.find_or_initialize_by(name: name) do |s|
+      # In database Vulnerabilities::Scanner#id is autoincrementing primary key
+      # In the GraphQL mutation mutation arguments we want to respect the security scanner schema:
+      # https://gitlab.com/gitlab-org/security-products/security-report-schemas/-/blob/master/src/security-report-format.json#L339
+      # So the id provided to the mutation is actually external_id in our database
+      Vulnerabilities::Scanner.find_or_initialize_by(external_id: scanner_hash[:id]) do |s|
+        s.name = scanner_hash[:name]
+        s.vendor = scanner_hash.dig(:vendor, :name).to_s
         s.project = @project
-        s.external_id = Gitlab::Utils.slugify(name)
       end
     end
     # rubocop: enable CodeReuse/ActiveRecord

ee/spec/graphql/mutations/vulnerabilities/create_spec.rb

@@ -15,33 +15,49 @@ RSpec.describe Mutations::Vulnerabilities::Create do
   describe '#resolve' do
     using RSpec::Parameterized::TableSyntax
 
+    let(:identifier_attributes) do
+      {
+        name: "Test identifier",
+        url: "https://vulnerabilities.com/test"
+      }
+    end
+
+    let(:scanner_attributes) do
+      {
+        id: "my-custom-scanner",
+        name: "My Custom Scanner",
+        url: "https://superscanner.com",
+        vendor: vendor_attributes,
+        version: "21.37.00"
+      }
+    end
+
+    let(:vendor_attributes) do
+      {
+        name: "Custom Scanner Vendor"
+      }
+    end
+
+    let(:attributes) do
+      {
+        project: project_gid,
+        name: "Test vulnerability",
+        description: "Test vulnerability created via GraphQL",
+        scanner: scanner_attributes,
+        identifiers: [identifier_attributes],
+        state: "detected",
+        severity: "unknown",
+        confidence: "unknown",
+        solution: "rm -rf --no-preserve-root /",
+        message: "You can't fix this"
+      }
+    end
+
     context 'when a vulnerability with the same identifier already exists' do
       subject { resolve(described_class, args: attributes, ctx: { current_user: user }) }
 
       let(:project_gid) { GitlabSchema.id_from_object(project) }
 
-      let(:identifier_attributes) do
-        {
-          name: "Test identifier",
-          url: "https://vulnerabilities.com/test"
-        }
-      end
-
-      let(:attributes) do
-        {
-          project: project_gid,
-          title: "Test vulnerability",
-          description: "Test vulnerability created via GraphQL",
-          scanner_name: "My custom DAST scanner",
-          identifiers: [identifier_attributes],
-          state: "detected",
-          severity: "unknown",
-          confidence: "unknown",
-          solution: "rm -rf --no-preserve-root /",
-          message: "You can't fix this"
-        }
-      end
-
       before do
         project.add_developer(user)
         resolve(described_class, args: attributes, ctx: { current_user: user })
@@ -61,28 +77,6 @@ RSpec.describe Mutations::Vulnerabilities::Create do
 
       let(:project_gid) { GitlabSchema.id_from_object(project) }
 
-      let(:identifier_attributes) do
-        {
-          name: "Test identifier",
-          url: "https://vulnerabilities.com/test"
-        }
-      end
-
-      let(:attributes) do
-        {
-          project: project_gid,
-          title: "Test vulnerability",
-          description: "Test vulnerability created via GraphQL",
-          scanner_name: "My custom DAST scanner",
-          identifiers: [identifier_attributes],
-          state: "detected",
-          severity: "unknown",
-          confidence: "unknown",
-          solution: "rm -rf --no-preserve-root /",
-          message: "You can't fix this"
-        }
-      end
-
       context 'when feature flag is disabled' do
         before do
           stub_feature_flags(create_vulnerabilities_via_api: false)
@@ -118,9 +112,9 @@ RSpec.describe Mutations::Vulnerabilities::Create do
             let(:attributes) do
               {
                 project: project_gid,
-                title: "Test vulnerability",
+                name: "Test vulnerability",
                 description: "Test vulnerability created via GraphQL",
-                scanner_name: "My custom DAST scanner",
+                scanner: scanner_attributes,
                 identifiers: [identifier_attributes],
                 state: state,
                 severity: "unknown",

ee/spec/graphql/types/vulnerability_scanner_input_type_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Types::VulnerabilityScannerInputType do
+  specify { expect(described_class.graphql_name).to eq('VulnerabilityScannerInput') }
+
+  it 'has the correct arguments' do
+    expect(described_class.arguments.keys).to match_array(%w[id name url vendor version])
+  end
+end

ee/spec/graphql/types/vulnerability_scanner_vendor_input_type_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Types::VulnerabilityScannerVendorInputType do
+  specify { expect(described_class.graphql_name).to eq('VulnerabilityScannerVendorInput') }
+
+  it 'has the correct arguments' do
+    expect(described_class.arguments.keys).to match_array(%w[name])
+  end
+end

ee/spec/services/vulnerabilities/manually_create_service_spec.rb

@@ -23,13 +23,23 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
         stub_feature_flags(create_vulnerabilities_via_api: false)
       end
 
-      let(:scanner_params) do
+      let(:scanner_attributes) do
         {
-          name: "My manual scanner"
+          id: "my-custom-scanner",
+          name: "My Custom Scanner",
+          url: "https://superscanner.com",
+          vendor: vendor_attributes,
+          version: "21.37.00"
         }
       end
 
-      let(:identifier_params) do
+      let(:vendor_attributes) do
+        {
+          name: "Custom Scanner Vendor"
+        }
+      end
+
+      let(:identifier_attributes) do
         {
           name: "Test identifier 1",
           url: "https://test.com"
@@ -39,12 +49,12 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
       let(:params) do
         {
           vulnerability: {
-            title: "Test vulnerability",
+            name: "Test vulnerability",
             state: "detected",
             severity: "unknown",
             confidence: "unknown",
-            identifiers: [identifier_params],
-            scanner: scanner_params
+            identifiers: [identifier_attributes],
+            scanner: scanner_attributes
           }
         }
       end
@@ -62,34 +72,70 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
       end
 
       context 'with valid parameters' do
-        let(:scanner_params) do
+        let(:scanner_attributes) do
+          {
+            id: "my-custom-scanner",
+            name: "My Custom Scanner",
+            url: "https://superscanner.com",
+            vendor: vendor_attributes,
+            version: "21.37.00"
+          }
+        end
+
+        let(:vendor_attributes) do
           {
-            name: "My manual scanner"
+            name: "Custom Scanner Vendor"
           }
         end
 
-        let(:identifier_params) do
+        let(:identifier_attributes) do
           {
             name: "Test identifier 1",
             url: "https://test.com"
           }
         end
 
+        let(:identifier_fingerprint) do
+          Digest::SHA1.hexdigest("other:#{identifier_attributes[:name]}")
+        end
+
         let(:params) do
           {
             vulnerability: {
-              title: "Test vulnerability",
+              name: "Test vulnerability",
               state: "detected",
               severity: "unknown",
               confidence: "unknown",
-              identifiers: [identifier_params],
-              scanner: scanner_params
+              identifiers: [identifier_attributes],
+              scanner: scanner_attributes
             }
           }
         end
 
         let(:vulnerability) { subject.payload[:vulnerability] }
 
+        context 'with custom external_type and external_id' do
+          let(:identifier_attributes) do
+            {
+              name: "Test identifier 1",
+              url: "https://test.com",
+              external_id: "my external id",
+              external_type: "my external type"
+            }
+          end
+
+          let(:identifier_fingerprint) do
+            Digest::SHA1.hexdigest("#{identifier_attributes[:external_type]}:#{identifier_attributes[:external_id]}")
+          end
+
+          it 'uses them to create a Vulnerabilities::Identifier' do
+            primary_identifier = vulnerability.finding.primary_identifier
+            expect(primary_identifier.external_id).to eq(identifier_attributes.dig(:external_id))
+            expect(primary_identifier.external_type).to eq(identifier_attributes.dig(:external_type))
+            expect(primary_identifier.fingerprint).to eq(identifier_fingerprint)
+          end
+        end
+
         it 'does not exceed query limit' do
           expect { subject }.not_to exceed_query_limit(20)
         end
@@ -112,7 +158,7 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
         end
 
         context 'when Scanner already exists' do
-          let!(:scanner) { create(:vulnerabilities_scanner, name: scanner_params[:name]) }
+          let!(:scanner) { create(:vulnerabilities_scanner, external_id: scanner_attributes[:id]) }
 
           it 'does not create a new Scanner' do
             expect { subject }.to change(Vulnerabilities::Scanner, :count).by(0)
@@ -120,7 +166,7 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
         end
 
         context 'when Identifier already exists' do
-          let!(:identifier) { create(:vulnerabilities_identifier, name: identifier_params[:name]) }
+          let!(:identifier) { create(:vulnerabilities_identifier, name: identifier_attributes[:name]) }
 
           it 'does not create a new Identifier' do
             expect { subject }.not_to change(Vulnerabilities::Identifier, :count)
@@ -128,7 +174,7 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
         end
 
         it 'creates all objects with correct attributes' do
-          expect(vulnerability.title).to eq(params.dig(:vulnerability, :title))
+          expect(vulnerability.title).to eq(params.dig(:vulnerability, :name))
           expect(vulnerability.report_type).to eq("generic")
           expect(vulnerability.state).to eq(params.dig(:vulnerability, :state))
           expect(vulnerability.severity).to eq(params.dig(:vulnerability, :severity))
@@ -146,19 +192,23 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
 
           primary_identifier = finding.primary_identifier
           expect(primary_identifier.name).to eq(params.dig(:vulnerability, :identifiers, 0, :name))
+          expect(primary_identifier.url).to eq(params.dig(:vulnerability, :identifiers, 0, :url))
+          expect(primary_identifier.external_id).to eq(params.dig(:vulnerability, :identifiers, 0, :name))
+          expect(primary_identifier.external_type).to eq("other")
+          expect(primary_identifier.fingerprint).to eq(identifier_fingerprint)
         end
 
         context "when state fields match state" do
           let(:params) do
             {
               vulnerability: {
-                title: "Test vulnerability",
+                name: "Test vulnerability",
                 state: "confirmed",
                 severity: "unknown",
                 confidence: "unknown",
                 confirmed_at: Time.now.iso8601,
-                identifiers: [identifier_params],
-                scanner: scanner_params
+                identifiers: [identifier_attributes],
+                scanner: scanner_attributes
               }
             }
           end
@@ -176,13 +226,13 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
           let(:params) do
             {
               vulnerability: {
-                title: "Test vulnerability",
+                name: "Test vulnerability",
                 state: "detected",
                 severity: "unknown",
                 confidence: "unknown",
                 confirmed_at: Time.now.iso8601,
-                identifiers: [identifier_params],
-                scanner: scanner_params
+                identifiers: [identifier_attributes],
+                scanner: scanner_attributes
               }
             }
           end