Merge branch '223142-be-create-issues-createfromvulnerabilityservice-service-object' into 'master'

Create Issues::CreateFromVulnerabilityService

See merge request gitlab-org/gitlab!40958

ee/app/assets/javascripts/vulnerabilities/components/related_issues.vue

@@ -6,7 +6,7 @@ import RelatedIssuesBlock from '~/related_issues/components/related_issues_block
 import { issuableTypesMap, PathIdSeparator } from '~/related_issues/constants';
 import { sprintf, __, s__ } from '~/locale';
 import { joinPaths, redirectTo } from '~/lib/utils/url_utility';
-import { RELATED_ISSUES_ERRORS, FEEDBACK_TYPES } from '../constants';
+import { RELATED_ISSUES_ERRORS } from '../constants';
 import { deprecatedCreateFlash as createFlash } from '~/flash';
 import { getFormattedIssue, getAddRelatedIssueRequestParams } from '../helpers';
 
@@ -89,20 +89,9 @@ export default {
       this.errorCreatingIssue = false;
 
       return axios
-        .post(this.createIssueUrl, {
-          vulnerability_feedback: {
-            feedback_type: FEEDBACK_TYPES.ISSUE,
-            category: this.reportType,
-            project_fingerprint: this.projectFingerprint,
-            vulnerability_data: {
-              ...this.vulnerability,
-              category: this.reportType,
-              vulnerability_id: this.vulnerabilityId,
-            },
-          },
-        })
-        .then(({ data: { issue_url } }) => {
-          redirectTo(issue_url);
+        .post(this.createIssueUrl)
+        .then(({ data: { web_url } }) => {
+          redirectTo(web_url);
         })
         .catch(() => {
           this.isProcessingAction = false;

ee/app/controllers/projects/security/vulnerabilities_controller.rb

@@ -17,6 +17,24 @@ module Projects
         @gfm_form = true
       end
 
+      def create_issue
+        result = ::Issues::CreateFromVulnerabilityService
+          .new(
+            container: vulnerability.project,
+            current_user: current_user,
+            params: {
+              vulnerability: vulnerability,
+              link_type: ::Vulnerabilities::IssueLink.link_types[:created]
+            })
+          .execute
+
+        if result[:status] == :success
+          render json: issue_serializer.represent(result[:issue], only: [:web_url])
+        else
+          render json: result[:message], status: :unprocessable_entity
+        end
+      end
+
       private
 
       def vulnerability
@@ -25,6 +43,10 @@ module Projects
 
       alias_method :issuable, :vulnerability
       alias_method :noteable, :vulnerability
+
+      def issue_serializer
+        IssueSerializer.new(current_user: current_user)
+      end
     end
   end
 end

ee/app/helpers/vulnerabilities_helper.rb

@@ -10,7 +10,7 @@ module VulnerabilitiesHelper
 
     result = {
       timestamp: Time.now.to_i,
-      create_issue_url: create_vulnerability_feedback_issue_path(vulnerability.finding.project),
+      create_issue_url: create_issue_project_security_vulnerability_path(vulnerability.project, vulnerability),
       has_mr: !!vulnerability.finding.merge_request_feedback.try(:merge_request_iid),
       create_mr_url: create_vulnerability_feedback_merge_request_path(vulnerability.finding.project),
       discussions_url: discussions_project_security_vulnerability_path(vulnerability.project, vulnerability),

ee/app/models/ee/vulnerability.rb

@@ -11,6 +11,7 @@ module EE
       include ::Noteable
       include ::Awardable
       include ::Referable
+      include ::Presentable
 
       TooManyDaysError = Class.new(StandardError)
 
@@ -93,6 +94,8 @@ module EE
       delegate :default_branch, :name, to: :project, prefix: true, allow_nil: true
       delegate :name, to: :group, prefix: true, allow_nil: true
 
+      delegate :solution, :identifiers, :links, :remediations, :file, to: :finding, allow_nil: true
+
       def to_reference(from = nil, full: false)
         reference = "#{self.class.reference_prefix}#{id}"
 

ee/app/models/vulnerabilities/finding.rb

@@ -242,6 +242,10 @@ module Vulnerabilities
       metadata.fetch('location', {})
     end
 
+    def file
+      location.dig('file')
+    end
+
     def links
       metadata.fetch('links', [])
     end

ee/app/presenters/vulnerability_presenter.rb

+# frozen_string_literal: true
+
+class VulnerabilityPresenter < Gitlab::View::Presenter::Delegated
+  presents :vulnerability
+
+  def links
+    vulnerability.links.map(&:with_indifferent_access)
+  end
+
+  def location_text
+    return file unless line
+
+    "#{file}:#{line}"
+  end
+
+  def location_link
+    return location_text unless blob_path
+
+    "#{root_url}#{blob_path}"
+  end
+
+  def blob_path
+    return unless file
+
+    branch = finding.pipelines&.last&.sha || project.default_branch
+    path = project_blob_path(vulnerability.project, File.join(branch, file))
+
+    return unless path
+
+    path = path.gsub(/^\//, '')
+
+    add_line_numbers(path, finding.location['start_line'], finding.location['end_line'])
+  end
+
+  private
+
+  def root_url
+    Gitlab::Routing.url_helpers.root_url
+  end
+
+  def line
+    finding.location.dig("start_line")
+  end
+
+  def file
+    finding.location.dig("file")
+  end
+
+  def add_line_numbers(path, start_line, end_line)
+    return path unless start_line
+
+    path.tap do |complete_path|
+      complete_path << "#L#{start_line}"
+      complete_path << "-#{end_line}" if end_line != start_line
+    end
+  end
+end

ee/app/services/issues/create_from_vulnerability_service.rb

+# frozen_string_literal: true
+module Issues
+  class CreateFromVulnerabilityService < ::BaseContainerService
+    def execute
+      return error("Can't create issue") unless can?(@current_user, :create_issue, @container)
+
+      vulnerability = params[:vulnerability].present
+      link_type = params[:link_type]
+
+      return error("Vulnerability not found") unless vulnerability
+      return error("Invalid link type for Vulnerability") unless link_type
+
+      begin
+        issue_params = {
+          title: "Investigate vulnerability: #{vulnerability.title}",
+          description: render_description(vulnerability),
+          confidential: true
+        }
+
+        issue = Issues::CreateService.new(@container, @current_user, issue_params).execute
+
+        if issue.valid?
+          issue_link_creation_result = VulnerabilityIssueLinks::CreateService.new(
+            @current_user,
+            vulnerability.subject,
+            issue,
+            link_type: link_type
+          ).execute
+
+          error(issue_link_creation_result.errors) if issue_link_creation_result.error?
+
+          success(issue)
+        else
+          error(issue.errors)
+        end
+      end
+    end
+
+    private
+
+    def success(issue)
+      super(issue: issue)
+    end
+
+    def render_description(vulnerability)
+      ApplicationController.render(
+        template: 'vulnerabilities/issue_description.md.erb',
+        locals: { vulnerability: vulnerability }
+      )
+    end
+  end
+end

ee/app/views/vulnerabilities/issue_description.md.erb

@@ -8,7 +8,7 @@
 <% if vulnerability.confidence.present? %>
 * <%= _("Confidence") %>: <%= vulnerability.confidence %>
 <% end %>
-<% if defined?(vulnerability.file) && vulnerability.file.present? %>
+<% if vulnerability.try(:file) %>
 * <%= _("Location") %>: [<%= vulnerability.location_text %>](<%= vulnerability.location_link %>)
 <% end %>
 

ee/config/routes/project.rb

@@ -68,6 +68,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           resources :vulnerabilities, only: [:show] do
             member do
               get :discussions, format: :json
+              post :create_issue, format: :json
             end
 
             scope module: :vulnerabilities do

ee/spec/factories/vulnerabilities.rb

@@ -9,6 +9,7 @@ FactoryBot.define do
     severity { :high }
     confidence { :medium }
     report_type { :sast }
+    description { "Description of #{title}" }
 
     trait :detected do
       state { Vulnerability.states[:detected] }
@@ -71,17 +72,33 @@ FactoryBot.define do
       end
     end
 
+    trait :with_finding do
+      after(:build) do |vulnerability|
+        finding = build(
+          :vulnerabilities_finding,
+          :identifier,
+          vulnerability: vulnerability,
+          report_type: vulnerability.report_type,
+          project: vulnerability.project
+        )
+
+        vulnerability.findings = [finding]
+      end
+    end
+
     trait :with_findings do
       after(:build) do |vulnerability|
         findings_with_solution = build_list(
           :vulnerabilities_finding,
           2,
+          :identifier,
           vulnerability: vulnerability,
           report_type: vulnerability.report_type,
           project: vulnerability.project)
         findings_with_remediation = build_list(
           :vulnerabilities_finding,
           2,
+          :identifier,
           :with_remediation,
           vulnerability: vulnerability,
           report_type: vulnerability.report_type,

ee/spec/factories/vulnerabilities/findings.rb

@@ -121,6 +121,18 @@ FactoryBot.define do
       end
     end
 
+    trait :identifier do
+      after(:build) do |finding|
+        identifier = build(
+          :vulnerabilities_identifier,
+          fingerprint: SecureRandom.hex(20),
+          project: finding.project
+        )
+
+        finding.identifiers = [identifier]
+      end
+    end
+
     ::Vulnerabilities::Finding::REPORT_TYPES.keys.each do |security_report_type|
       trait security_report_type do
         report_type { security_report_type }

ee/spec/frontend/vulnerabilities/related_issues_spec.js

@@ -2,7 +2,6 @@ import { GlAlert } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 import MockAdapter from 'axios-mock-adapter';
 import RelatedIssues from 'ee/vulnerabilities/components/related_issues.vue';
-import { FEEDBACK_TYPES } from 'ee/vulnerabilities/constants';
 import waitForPromises from 'helpers/wait_for_promises';
 import RelatedIssuesBlock from '~/related_issues/components/related_issues_block.vue';
 import { issuableTypesMap, PathIdSeparator } from '~/related_issues/constants';
@@ -299,10 +298,10 @@ describe('Vulnerability related issues component', () => {
     });
 
     it('calls create issue endpoint on click and redirects to new issue', async () => {
-      const issueUrl = '/group/project/issues/123';
+      const issueUrl = `/group/project/-/security/vulnerabilities/${vulnerabilityId}/create_issue`;
       const spy = jest.spyOn(urlUtility, 'redirectTo');
       mockAxios.onPost(propsData.createIssueUrl).reply(200, {
-        issue_url: issueUrl,
+        web_url: issueUrl,
       });
 
       findCreateIssueButton().vm.$emit('click');
@@ -313,17 +312,6 @@ describe('Vulnerability related issues component', () => {
       expect(mockAxios.history.post).toHaveLength(1);
       expect(postRequest.url).toBe(createIssueUrl);
       expect(spy).toHaveBeenCalledWith(issueUrl);
-      expect(JSON.parse(postRequest.data)).toMatchObject({
-        vulnerability_feedback: {
-          feedback_type: FEEDBACK_TYPES.ISSUE,
-          category: reportType,
-          project_fingerprint: projectFingerprint,
-          vulnerability_data: {
-            category: reportType,
-            vulnerability_id: vulnerabilityId,
-          },
-        },
-      });
     });
 
     it('shows an error message when issue creation fails', async () => {

ee/spec/helpers/vulnerabilities_helper_spec.rb

@@ -58,7 +58,7 @@ RSpec.describe VulnerabilitiesHelper do
     it 'has expected vulnerability properties' do
       expect(subject).to include(
         timestamp: Time.now.to_i,
-        create_issue_url: "/#{project.full_path}/-/vulnerability_feedback",
+        create_issue_url: "/#{project.full_path}/-/security/vulnerabilities/#{vulnerability.id}/create_issue",
         has_mr: anything,
         create_mr_url: "/#{project.full_path}/-/vulnerability_feedback",
         discussions_url: "/#{project.full_path}/-/security/vulnerabilities/#{vulnerability.id}/discussions",

ee/spec/services/ee/issues/create_from_vulnerability_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issues::CreateFromVulnerabilityService, '#execute' do
+  let_it_be(:group)   { create(:group) }
+  let_it_be(:project) { create(:project, :public, :repository, namespace: group) }
+  let_it_be(:user)    { create(:user) }
+  let_it_be(:vulnerability) { create(:vulnerability, :with_finding, project: project) }
+  let(:params) { { vulnerability: vulnerability, link_type: Vulnerabilities::IssueLink.link_types[:created] } }
+
+  before do
+    stub_licensed_features(security_dashboard: true)
+    group.add_developer(user)
+  end
+
+  shared_examples 'a created issue' do
+    let(:result) { described_class.new(container: project, current_user: user, params: params).execute }
+
+    it 'creates the issue with the given params' do
+      expect(result[:status]).to eq(:success)
+      issue = result[:issue]
+      expect(issue).to be_persisted
+      expect(issue.project).to eq(project)
+      expect(issue.author).to eq(user)
+      expect(issue.title).to eq(expected_title)
+      expect(issue.description).to eq(expected_description)
+      expect(issue).to be_confidential
+    end
+  end
+
+  context 'when a vulnerability exists' do
+    let(:result) { described_class.new(container: project, current_user: user, params: params).execute }
+
+    context 'when user does not have permission to create issue' do
+      before do
+        allow_next_instance_of(described_class) do |instance|
+          allow(instance).to receive(:can?).with(user, :create_issue, project).and_return(false)
+        end
+      end
+
+      it 'returns expected error' do
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Can't create issue")
+      end
+    end
+
+    context 'when issues are disabled on project' do
+      let(:project) { create(:project, :public, namespace: group, issues_access_level: ProjectFeature::DISABLED) }
+
+      it 'returns expected error' do
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Can't create issue")
+      end
+    end
+
+    context 'when report type is SAST' do
+      let(:expected_title) { "Investigate vulnerability: #{vulnerability.title}" }
+      let(:expected_description) do
+        <<~DESC.chomp
+          ### Description:
+
+          Description of #{vulnerability.title}
+
+          * Severity: #{vulnerability.severity}
+          * Confidence: #{vulnerability.confidence}
+          * Location: [maven/src/main/java/com/gitlab/security_products/tests/App.java:29](http://localhost/#{project.full_path}/-/blob/master/maven/src/main/java/com/gitlab/security_products/tests/App.java#L29)
+
+          ### Solution:
+
+          #{vulnerability.solution}
+
+          ### Identifiers:
+
+          * [CVE-2018-1234](http://cve.mitre.org/cgi-bin/cvename.cgi?name=2018-1234)
+
+          ### Links:
+
+          * [Cipher does not check for integrity first?](https://crypto.stackexchange.com/questions/31428/pbewithmd5anddes-cipher-does-not-check-for-integrity-first)
+        DESC
+      end
+
+      it_behaves_like 'a created issue'
+    end
+  end
+end