Merge branch '10242-1-create-vulnerability-from-finding' into 'master'

API call for Create Vulnerability from Finding

See merge request gitlab-org/gitlab!18283

db/migrate/20191112115247_add_cached_markdown_version_to_vulnerabilities.rb

+# frozen_string_literal: true
+
+class AddCachedMarkdownVersionToVulnerabilities < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :vulnerabilities, :cached_markdown_version, :integer
+  end
+end

db/post_migrate/20191112115317_change_vulnerabilities_title_html_to_nullable.rb

+# frozen_string_literal: true
+
+class ChangeVulnerabilitiesTitleHtmlToNullable < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    change_column_null :vulnerabilities, :title_html, true
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_11_11_115431) do
+ActiveRecord::Schema.define(version: 2019_11_12_115317) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -3928,7 +3928,7 @@ ActiveRecord::Schema.define(version: 2019_11_11_115431) do
     t.datetime_with_timezone "created_at", null: false
     t.datetime_with_timezone "updated_at", null: false
     t.string "title", limit: 255, null: false
-    t.text "title_html", null: false
+    t.text "title_html"
     t.text "description"
     t.text "description_html"
     t.bigint "start_date_sourcing_milestone_id"
@@ -3941,6 +3941,7 @@ ActiveRecord::Schema.define(version: 2019_11_11_115431) do
     t.integer "confidence", limit: 2, null: false
     t.boolean "confidence_overridden", default: false
     t.integer "report_type", limit: 2, null: false
+    t.integer "cached_markdown_version"
     t.index ["author_id"], name: "index_vulnerabilities_on_author_id"
     t.index ["closed_by_id"], name: "index_vulnerabilities_on_closed_by_id"
     t.index ["due_date_sourcing_milestone_id"], name: "index_vulnerabilities_on_due_date_sourcing_milestone_id"

ee/app/models/ee/project.rb

@@ -67,7 +67,11 @@ module EE
       # https://gitlab.com/gitlab-org/gitlab/issues/10252#terminology
       has_many :vulnerabilities
       has_many :vulnerability_feedback, class_name: 'Vulnerabilities::Feedback'
-      has_many :vulnerability_findings, class_name: 'Vulnerabilities::Occurrence'
+      has_many :vulnerability_findings, class_name: 'Vulnerabilities::Occurrence' do
+        def lock_for_confirmation!(id)
+          where(vulnerability_id: nil).lock.find(id)
+        end
+      end
       has_many :vulnerability_identifiers, class_name: 'Vulnerabilities::Identifier'
       has_many :vulnerability_scanners, class_name: 'Vulnerabilities::Scanner'
 

ee/app/models/vulnerability.rb

 # frozen_string_literal: true
 
 class Vulnerability < ApplicationRecord
-  belongs_to :project
+  include CacheMarkdownField
+  include Redactable
+  include StripAttribute
+
+  cache_markdown_field :title, pipeline: :single_line
+  cache_markdown_field :description, issuable_state_filter_enabled: true
+
+  strip_attributes :title
+
+  redact_field :description
+
+  belongs_to :project # keep this association named 'project' for correct work of markdown cache
   belongs_to :milestone
   belongs_to :epic
 
-  belongs_to :author, class_name: 'User'
+  belongs_to :author, class_name: 'User' # keep this association named 'author' for correct work of markdown cache
   belongs_to :updated_by, class_name: 'User'
   belongs_to :last_edited_by, class_name: 'User'
   belongs_to :closed_by, class_name: 'User'
@@ -17,7 +28,7 @@ class Vulnerability < ApplicationRecord
   enum confidence: Vulnerabilities::Occurrence::CONFIDENCE_LEVELS, _prefix: :confidence
   enum report_type: Vulnerabilities::Occurrence::REPORT_TYPES
 
-  validates :project, :author, :title, :title_html, :severity, :confidence, :report_type, presence: true
+  validates :project, :author, :title, :severity, :confidence, :report_type, presence: true
 
   # at this stage Vulnerability is not an Issuable, has some important attributes (and their constraints) in common
   validates :title, length: { maximum: Issuable::TITLE_LENGTH_MAX }

ee/app/policies/ee/project_policy.rb

@@ -151,12 +151,14 @@ module EE
 
       rule { can?(:developer_access) }.policy do
         enable :read_project_security_dashboard
+        enable :create_vulnerability
         enable :resolve_vulnerability
         enable :dismiss_vulnerability
       end
 
       rule { security_dashboard_feature_disabled }.policy do
         prevent :read_project_security_dashboard
+        prevent :create_vulnerability
         prevent :resolve_vulnerability
         prevent :dismiss_vulnerability
       end
@@ -203,6 +205,7 @@ module EE
         enable :read_deployment
         enable :read_pages
         enable :read_project_security_dashboard
+        enable :create_vulnerability
         enable :resolve_vulnerability
         enable :dismiss_vulnerability
       end

ee/app/services/vulnerabilities/create_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class CreateService
+    include Gitlab::Allowable
+
+    def initialize(project, author, finding_id:)
+      @project = project
+      @author = author
+      @finding_id = finding_id
+    end
+
+    def execute
+      raise Gitlab::Access::AccessDeniedError unless can?(@author, :create_vulnerability, @project)
+
+      vulnerability = Vulnerability.new
+
+      Vulnerabilities::Occurrence.transaction(requires_new: true) do
+        # we're using `lock` instead of `with_lock` to avoid extra call to `find` under the hood
+        finding = @project.vulnerability_findings.lock_for_confirmation!(@finding_id)
+
+        save_vulnerability(vulnerability, finding)
+      rescue ActiveRecord::RecordNotFound
+        vulnerability.errors.add(:base, _('finding is not found or is already attached to a vulnerability'))
+        raise ActiveRecord::Rollback
+      end
+
+      vulnerability
+    end
+
+    private
+
+    def save_vulnerability(vulnerability, finding)
+      vulnerability.assign_attributes(
+        author: @author,
+        project: @project,
+        title: finding.name,
+        state: :opened,
+        severity: finding.severity,
+        severity_overridden: false,
+        confidence: finding.confidence,
+        confidence_overridden: false,
+        report_type: finding.report_type
+      )
+      vulnerability.save && vulnerability.findings << finding
+    end
+  end
+end

ee/changelogs/unreleased/10242-1-create-vulnerability-from-finding.yml

+---
+title: Added autogenerated Markdown support for Vulnerability title and description
+merge_request: 18283
+author:
+type: other

ee/lib/api/vulnerabilities.rb

@@ -19,6 +19,14 @@ module API
         end
       end
 
+      def authorize_can_read!
+        authorize! :read_project_security_dashboard, user_project
+      end
+
+      def authorize_can_create!
+        authorize! :create_vulnerability, user_project
+      end
+
       def render_vulnerability(vulnerability)
         if vulnerability.valid?
           present vulnerability, with: EE::API::Entities::Vulnerability
@@ -68,8 +76,11 @@ module API
       desc 'Get a list of project vulnerabilities' do
         success EE::API::Entities::Vulnerability
       end
+      params do
+        use :pagination
+      end
       get ':id/vulnerabilities' do
-        authorize! :read_project_security_dashboard, user_project
+        authorize_can_read!
 
         vulnerabilities = paginate(
           vulnerabilities_by(user_project)
@@ -77,6 +88,26 @@ module API
 
         present vulnerabilities, with: EE::API::Entities::Vulnerability
       end
+
+      desc 'Create a new Vulnerability (from a confirmed Finding)' do
+        success EE::API::Entities::Vulnerability
+      end
+      params do
+        requires :finding_id, type: Integer, desc: 'The id of confirmed vulnerability finding'
+      end
+      post ':id/vulnerabilities' do
+        authorize_can_create!
+
+        vulnerability = ::Vulnerabilities::CreateService.new(
+          user_project, current_user, finding_id: params[:finding_id]
+        ).execute
+
+        if vulnerability.persisted?
+          present vulnerability, with: EE::API::Entities::Vulnerability
+        else
+          render_validation_error!(vulnerability)
+        end
+      end
     end
   end
 end

ee/spec/models/vulnerability_spec.rb

@@ -48,6 +48,8 @@ describe Vulnerability do
     it { is_expected.to belong_to(:updated_by).class_name('User') }
     it { is_expected.to belong_to(:last_edited_by).class_name('User') }
     it { is_expected.to belong_to(:closed_by).class_name('User') }
+
+    it { is_expected.to have_many(:findings).class_name('Vulnerabilities::Occurrence').dependent(false) }
   end
 
   describe 'validations' do
@@ -56,7 +58,6 @@ describe Vulnerability do
     it { is_expected.to validate_presence_of(:project) }
     it { is_expected.to validate_presence_of(:author) }
     it { is_expected.to validate_presence_of(:title) }
-    it { is_expected.to validate_presence_of(:title_html) }
     it { is_expected.to validate_presence_of(:severity) }
     it { is_expected.to validate_presence_of(:confidence) }
     it { is_expected.to validate_presence_of(:report_type) }
@@ -66,4 +67,29 @@ describe Vulnerability do
     it { is_expected.to validate_length_of(:description).is_at_most(::Issuable::DESCRIPTION_LENGTH_MAX).allow_nil }
     it { is_expected.to validate_length_of(:description_html).is_at_most(::Issuable::DESCRIPTION_HTML_LENGTH_MAX).allow_nil }
   end
+
+  describe 'text fields' do
+    subject { create(:vulnerability, title: '_My title_ ', description: '**Hello `world`**') }
+
+    it 'has proper markdown for title field' do
+      expect(subject.title_html).to eq('_My title_') # no markdown rendering because it's a single line field
+    end
+
+    it 'has proper markdown for title field' do
+      expect(subject.description_html).to(
+        eq('<p data-sourcepos="1:1-1:17" dir="auto"><strong>Hello <code>world</code></strong></p>')
+      )
+    end
+
+    context 'redactable fields' do
+      before do
+        stub_commonmark_sourcepos_disabled
+      end
+
+      it_behaves_like 'model with redactable field' do
+        let(:model) { create(:vulnerability) }
+        let(:field) { :description }
+      end
+    end
+  end
 end

ee/spec/policies/project_policy_spec.rb

@@ -33,7 +33,7 @@ describe ProjectPolicy do
     let(:additional_developer_permissions) do
       %i[
         admin_vulnerability_feedback read_project_security_dashboard read_feature_flag
-        resolve_vulnerability dismiss_vulnerability
+        create_vulnerability resolve_vulnerability dismiss_vulnerability
       ]
     end
     let(:additional_maintainer_permissions) { %i[push_code_to_protected_branches admin_feature_flags_client] }
@@ -46,7 +46,8 @@ describe ProjectPolicy do
         read_pipeline read_build read_commit_status read_container_image
         read_environment read_deployment read_merge_request read_pages
         create_merge_request_in award_emoji
-        read_project_security_dashboard resolve_vulnerability dismiss_vulnerability
+        read_project_security_dashboard
+        create_vulnerability resolve_vulnerability dismiss_vulnerability
         read_vulnerability_feedback read_software_license_policy
       ]
     end
@@ -494,6 +495,7 @@ describe ProjectPolicy do
 
         include_context 'when security dashboard feature is not available'
 
+        it { is_expected.to be_disallowed(:create_vulnerability) }
         it { is_expected.to be_disallowed(:resolve_vulnerability) }
         it { is_expected.to be_disallowed(:dismiss_vulnerability) }
       end

ee/spec/requests/api/vulnerabilities_spec.rb

@@ -7,8 +7,8 @@ describe API::Vulnerabilities do
     stub_licensed_features(security_dashboard: true)
   end
 
-  let_it_be(:project) { create(:project, :with_vulnerabilities) }
   let_it_be(:user) { create(:user) }
+  let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerabilities" }
 
   shared_examples 'forbids actions on vulnerability in case of disabled features' do
     context 'when "first-class vulnerabilities" feature is disabled' do
@@ -37,7 +37,7 @@ describe API::Vulnerabilities do
   end
 
   describe 'GET /projects/:id/vulnerabilities' do
-    let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerabilities" }
+    let_it_be(:project) { create(:project, :with_vulnerabilities) }
 
     subject { get api(project_vulnerabilities_path, user) }
 
@@ -70,7 +70,79 @@ describe API::Vulnerabilities do
     end
 
     it_behaves_like 'responds with "not found" when there is no access to the project'
-    it_behaves_like 'prevents working with vulnerabilities in case of insufficient privileges'
+    it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level'
+  end
+
+  describe 'POST /projects/:id/vulnerabilities' do
+    let_it_be(:project) { create(:project) }
+    let(:finding) { create(:vulnerabilities_occurrence, project: project) }
+    let(:finding_id) { finding.id }
+    let(:expected_error_messages) { { 'base' => ['finding is not found or is already attached to a vulnerability'] } }
+
+    subject do
+      post api(project_vulnerabilities_path, user), params: { finding_id: finding_id }
+    end
+
+    context 'with an authorized user with proper permissions' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'creates a vulnerability from finding and attaches it to the vulnerability' do
+        expect { subject }.to change { project.vulnerabilities.count }.by(1)
+        expect(project.vulnerabilities.last).to(
+          have_attributes(
+            author: user,
+            title: finding.name,
+            state: 'opened',
+            severity: finding.severity,
+            severity_overridden: false,
+            confidence: finding.confidence,
+            confidence_overridden: false,
+            report_type: finding.report_type
+          ))
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(response).to match_response_schema('public_api/v4/vulnerability', dir: 'ee')
+      end
+
+      context 'when finding id is unknown' do
+        let(:finding_id) { 0 }
+
+        it 'responds with expected error' do
+          subject
+
+          expect(response).to have_gitlab_http_status(400)
+          expect(json_response['message']).to eq(expected_error_messages)
+        end
+      end
+
+      context 'when a vulnerability already exists for a specific finding' do
+        before do
+          create(:vulnerability, findings: [finding], project: finding.project)
+        end
+
+        it 'rejects creation of a new vulnerability from this finding' do
+          subject
+
+          expect(response).to have_gitlab_http_status(400)
+          expect(json_response['message']).to eq(expected_error_messages)
+        end
+      end
+
+      it_behaves_like 'forbids actions on vulnerability in case of disabled features'
+    end
+
+    it_behaves_like 'responds with "not found" when there is no access to the project'
+    it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level'
+  end
+
+  shared_examples 'prevents working with vulnerabilities for anonymous users' do
+    it do
+      subject
+
+      expect(response).to have_gitlab_http_status(403)
+    end
   end
 
   describe "POST /vulnerabilities:id/dismiss" do
@@ -78,6 +150,7 @@ describe API::Vulnerabilities do
       create_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability, project: vulnerability.project)
     end
 
+    let_it_be(:project) { create(:project, :with_vulnerabilities) }
     let(:vulnerability) { project.vulnerabilities.first }
 
     subject { post api("/vulnerabilities/#{vulnerability.id}/dismiss", user) }
@@ -143,7 +216,8 @@ describe API::Vulnerabilities do
       it_behaves_like 'forbids actions on vulnerability in case of disabled features'
     end
 
-    it_behaves_like 'prevents working with vulnerabilities in case of insufficient privileges'
+    it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level'
+    it_behaves_like 'prevents working with vulnerabilities for anonymous users'
   end
 
   describe "POST /vulnerabilities:id/resolve" do
@@ -151,6 +225,7 @@ describe API::Vulnerabilities do
       create_list(:vulnerabilities_finding, 2, vulnerability: vulnerability)
     end
 
+    let_it_be(:project) { create(:project, :with_vulnerabilities) }
     let(:vulnerability) { project.vulnerabilities.first }
 
     subject { post api("/vulnerabilities/#{vulnerability.id}/resolve", user) }
@@ -186,6 +261,7 @@ describe API::Vulnerabilities do
       it_behaves_like 'forbids actions on vulnerability in case of disabled features'
     end
 
-    it_behaves_like 'prevents working with vulnerabilities in case of insufficient privileges'
+    it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level'
+    it_behaves_like 'prevents working with vulnerabilities for anonymous users'
   end
 end

ee/spec/requests/api/vulnerability_findings_spec.rb

@@ -181,7 +181,7 @@ describe API::VulnerabilityFindings do
       subject { get api(project_vulnerability_findings_path, user) }
     end
 
-    it_behaves_like 'prevents working with vulnerabilities in case of insufficient privileges' do
+    it_behaves_like 'prevents working with vulnerabilities in case of insufficient access level' do
       subject { get api(project_vulnerability_findings_path, user) }
     end
   end

ee/spec/services/vulnerabilities/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Vulnerabilities::CreateService 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(:finding) { create(:vulnerabilities_occurrence, project: project) }
+  let(:finding_id) { finding.id }
+  let(:expected_error_messages) { { base: ['finding is not found or is already attached to a vulnerability'] } }
+
+  subject { described_class.new(project, user, finding_id: finding_id).execute }
+
+  context 'with an authorized user with proper permissions' do
+    before do
+      project.add_developer(user)
+    end
+
+    it 'creates a vulnerability from finding and attaches it to the vulnerability' do
+      expect { subject }.to change { project.vulnerabilities.count }.by(1)
+      expect(project.vulnerabilities.last).to(
+        have_attributes(
+          author: user,
+          title: finding.name,
+          state: 'opened',
+          severity: finding.severity,
+          severity_overridden: false,
+          confidence: finding.confidence,
+          confidence_overridden: false,
+          report_type: finding.report_type
+        ))
+    end
+
+    it 'starts a new transaction for the create sequence' do
+      allow(Vulnerabilities::Occurrence).to receive(:transaction).and_call_original
+
+      subject
+      expect(Vulnerabilities::Occurrence).to have_received(:transaction).with(requires_new: true).once
+    end
+
+    context 'when finding id is unknown' do
+      let(:finding_id) { 0 }
+
+      it 'adds expected error to the response' do
+        expect(subject.errors.messages).to eq(expected_error_messages)
+      end
+    end
+
+    context 'when finding does not belong to the vulnerability project' do
+      let(:finding) { create(:vulnerabilities_occurrence) }
+
+      it 'adds expected error to the response' do
+        expect(subject.errors.messages).to eq(expected_error_messages)
+      end
+    end
+
+    context 'when a vulnerability already exists for a specific finding' do
+      before do
+        create(:vulnerability, findings: [finding], project: finding.project)
+      end
+
+      it 'rejects creation of a new vulnerability from this finding' do
+        expect(subject.errors.messages).to eq(expected_error_messages)
+      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 dismiss 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

ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb

 # frozen_string_literal: true
 
-shared_examples 'prevents working with vulnerabilities in case of insufficient privileges' do
-  context 'with lesser access level than required' do
-    it 'responds with 403 Forbidden' do
-      project.add_reporter(user)
+shared_examples 'prevents working with vulnerabilities in case of insufficient access level' do
+  it 'responds 403 Forbidden when accessed by reporter' do
+    project.add_reporter(user)
 
-      subject
+    subject
 
-      expect(response).to have_gitlab_http_status(403)
-    end
+    expect(response).to have_gitlab_http_status(403)
+  end
+
+  it 'responds 403 Forbidden when accessed by guest' do
+    project.add_guest(user)
+
+    subject
+
+    expect(response).to have_gitlab_http_status(403)
   end
 end
 

locale/gitlab.pot

@@ -20349,6 +20349,9 @@ msgstr ""
 msgid "failed to dismiss associated finding(id=%{finding_id}): %{message}"
 msgstr ""
 
+msgid "finding is not found or is already attached to a vulnerability"
+msgstr ""
+
 msgid "for %{link_to_merge_request} with %{link_to_merge_request_source_branch}"
 msgstr ""
 

spec/models/concerns/redactable_spec.rb

@@ -7,44 +7,6 @@ describe Redactable do
     stub_commonmark_sourcepos_disabled
   end
 
-  shared_examples 'model with redactable field' do
-    it 'redacts unsubscribe token' do
-      model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
-
-      model.save!
-
-      expect(model[field]).to eq 'some text /sent_notifications/REDACTED/unsubscribe more text'
-    end
-
-    it 'ignores not hexadecimal tokens' do
-      text = 'some text /sent_notifications/token/unsubscribe more text'
-      model[field] = text
-
-      model.save!
-
-      expect(model[field]).to eq text
-    end
-
-    it 'ignores not matching texts' do
-      text = 'some text /sent_notifications/.*/unsubscribe more text'
-      model[field] = text
-
-      model.save!
-
-      expect(model[field]).to eq text
-    end
-
-    it 'redacts the field when saving the model before creating markdown cache' do
-      model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
-
-      model.save!
-
-      expected = 'some text /sent_notifications/REDACTED/unsubscribe more text'
-      expect(model[field]).to eq expected
-      expect(model["#{field}_html"]).to eq "<p dir=\"auto\">#{expected}</p>"
-    end
-  end
-
   context 'when model is an issue' do
     it_behaves_like 'model with redactable field' do
       let(:model) { create(:issue) }

spec/support/shared_examples/models/concerns/redactable_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'model with redactable field' do
+  it 'redacts unsubscribe token' do
+    model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
+
+    model.save!
+
+    expect(model[field]).to eq 'some text /sent_notifications/REDACTED/unsubscribe more text'
+  end
+
+  it 'ignores not hexadecimal tokens' do
+    text = 'some text /sent_notifications/token/unsubscribe more text'
+    model[field] = text
+
+    model.save!
+
+    expect(model[field]).to eq text
+  end
+
+  it 'ignores not matching texts' do
+    text = 'some text /sent_notifications/.*/unsubscribe more text'
+    model[field] = text
+
+    model.save!
+
+    expect(model[field]).to eq text
+  end
+
+  it 'redacts the field when saving the model before creating markdown cache' do
+    model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
+
+    model.save!
+
+    expected = 'some text /sent_notifications/REDACTED/unsubscribe more text'
+    expect(model[field]).to eq expected
+    expect(model["#{field}_html"]).to eq "<p dir=\"auto\">#{expected}</p>"
+  end
+end