Merge branch '10242-dismiss-vulnerability' into 'master'

Add Dismiss Vulnerability API call to Vulnerabilities API

See merge request !18495

ee/app/models/vulnerabilities/occurrence.rb

@@ -18,6 +18,7 @@ class Occurrence < ApplicationRecord
     belongs_to :project
     belongs_to :scanner, class_name: 'Vulnerabilities::Scanner'
     belongs_to :primary_identifier, class_name: 'Vulnerabilities::Identifier', inverse_of: :primary_occurrences
+    belongs_to :vulnerability, inverse_of: :findings
 
     has_many :occurrence_identifiers, class_name: 'Vulnerabilities::OccurrenceIdentifier'
     has_many :identifiers, through: :occurrence_identifiers, class_name: 'Vulnerabilities::Identifier'

ee/app/models/vulnerability.rb

@@ -10,6 +10,9 @@ class Vulnerability < ApplicationRecord
   belongs_to :last_edited_by, class_name: 'User'
   belongs_to :closed_by, class_name: 'User'
 
+  # TODO: temporary, remove when https://gitlab.com/gitlab-org/gitlab/merge_requests/18283 is merged and rebased onto
+  has_many :findings, class_name: 'Vulnerabilities::Occurrence', inverse_of: :vulnerability
+
   enum state: { opened: 1, closed: 2 }
   enum severity: Vulnerabilities::Occurrence::SEVERITY_LEVELS, _prefix: :severity
   enum confidence: Vulnerabilities::Occurrence::CONFIDENCE_LEVELS, _prefix: :confidence
@@ -21,4 +24,6 @@ class Vulnerability < ApplicationRecord
   validates :title_html, length: { maximum: Issuable::TITLE_HTML_LENGTH_MAX }, allow_blank: true
   validates :description, length: { maximum: Issuable::DESCRIPTION_LENGTH_MAX }, allow_blank: true
   validates :description_html, length: { maximum: Issuable::DESCRIPTION_HTML_LENGTH_MAX }, allow_blank: true
+
+  scope :with_findings, -> { includes(:findings) }
 end

ee/app/policies/ee/project_policy.rb

@@ -146,10 +146,12 @@ module ProjectPolicy
 
       rule { can?(:developer_access) }.policy do
         enable :read_project_security_dashboard
+        enable :dismiss_vulnerability
       end
 
       rule { security_dashboard_feature_disabled }.policy do
         prevent :read_project_security_dashboard
+        prevent :dismiss_vulnerability
       end
 
       rule { can?(:read_project) & (can?(:read_merge_request) | can?(:read_build)) }.enable :read_vulnerability_feedback
@@ -194,6 +196,7 @@ module ProjectPolicy
         enable :read_deployment
         enable :read_pages
         enable :read_project_security_dashboard
+        enable :dismiss_vulnerability
       end
 
       rule { auditor & ~guest }.policy do

ee/app/services/vulnerabilities/dismiss_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class DismissService
+    include Gitlab::Allowable
+
+    FindingsDismissResult = Struct.new(:ok?, :finding, :message)
+
+    def initialize(current_user, vulnerability)
+      @current_user = current_user
+      @vulnerability = vulnerability
+      @project = vulnerability.project
+    end
+
+    def execute
+      raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :dismiss_vulnerability, @project)
+
+      @vulnerability.transaction do
+        result = dismiss_findings
+
+        unless result.ok?
+          handle_finding_dismissal_error(result.finding, result.message)
+          raise ActiveRecord::Rollback
+        end
+
+        @vulnerability.update(state: 'closed')
+      end
+
+      @vulnerability
+    end
+
+    private
+
+    def feedback_service_for(finding)
+      VulnerabilityFeedback::CreateService.new(@project, @current_user, feedback_params_for(finding))
+    end
+
+    def feedback_params_for(finding)
+      {
+        category: finding.report_type,
+        feedback_type: 'dismissal',
+        project_fingerprint: finding.project_fingerprint
+      }
+    end
+
+    def dismiss_findings
+      @vulnerability.findings.each do |finding|
+        result = feedback_service_for(finding).execute
+
+        return FindingsDismissResult.new(false, finding, result[:message]) if result[:status] == :error
+      end
+
+      FindingsDismissResult.new(true)
+    end
+
+    def handle_finding_dismissal_error(finding, message)
+      @vulnerability.errors.add(
+        :base,
+        :finding_dismissal_error,
+        message: _("failed to dismiss associated finding(id=%{finding_id}): %{message}") %
+          {
+            finding_id: finding.id,
+            message: message
+          })
+    end
+  end
+end

ee/lib/api/vulnerabilities.rb

@@ -10,6 +10,24 @@ class Vulnerabilities < Grape::API
       def vulnerabilities_by(project)
         Security::VulnerabilitiesFinder.new(project).execute
       end
+
+      def find_vulnerability!
+        Vulnerability.with_findings.find(params[:id])
+      end
+
+      def find_and_authorize_vulnerability!(action)
+        find_vulnerability!.tap do |vulnerability|
+          authorize! action, vulnerability.project
+        end
+      end
+
+      def render_vulnerability(vulnerability)
+        if vulnerability.valid?
+          present vulnerability, with: VulnerabilityEntity
+        else
+          render_validation_error!(vulnerability)
+        end
+      end
     end
 
     before do
@@ -17,9 +35,26 @@ def vulnerabilities_by(project)
     end
 
     params do
-      requires :id, type: String, desc: 'The ID of a project'
+      requires :id, type: String, desc: 'The ID of a vulnerability'
+    end
+    resource :vulnerabilities do
+      desc 'Dismiss a vulnerability' do
+        success VulnerabilityEntity
+      end
+      post ':id/dismiss' do
+        if Feature.enabled?(:first_class_vulnerabilities)
+          vulnerability = find_and_authorize_vulnerability!(:dismiss_vulnerability)
+          vulnerability = ::Vulnerabilities::DismissService.new(current_user, vulnerability).execute
+          render_vulnerability(vulnerability)
+        else
+          not_found!
+        end
+      end
     end
 
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+    end
     resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
       params do
         # These params have no effect for Vulnerabilities API but are required to support falling back to

ee/spec/factories/vulnerabilities.rb

@@ -17,5 +17,15 @@
       state { :closed }
       closed_at { Time.now }
     end
+
+    trait :with_findings do
+      after(:build) do |vulnerability|
+        vulnerability.findings = build_list(
+          :vulnerabilities_occurrence,
+          2,
+          vulnerability: vulnerability,
+          project: vulnerability.project)
+      end
+    end
   end
 end

ee/spec/models/vulnerabilities/occurrence_spec.rb

@@ -11,6 +11,7 @@
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:primary_identifier).class_name('Vulnerabilities::Identifier') }
     it { is_expected.to belong_to(:scanner).class_name('Vulnerabilities::Scanner') }
+    it { is_expected.to belong_to(:vulnerability).inverse_of(:findings) }
     it { is_expected.to have_many(:pipelines).class_name('Ci::Pipeline') }
     it { is_expected.to have_many(:occurrence_pipelines).class_name('Vulnerabilities::OccurrencePipeline') }
     it { is_expected.to have_many(:identifiers).class_name('Vulnerabilities::Identifier') }

ee/spec/models/vulnerability_spec.rb

@@ -33,7 +33,7 @@
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:milestone) }
     it { is_expected.to belong_to(:epic) }
-
+    it { is_expected.to have_many(:findings).class_name('Vulnerabilities::Occurrence').inverse_of(:vulnerability) }
     it { is_expected.to belong_to(:author).class_name('User') }
     it { is_expected.to belong_to(:updated_by).class_name('User') }
     it { is_expected.to belong_to(:last_edited_by).class_name('User') }

ee/spec/policies/project_policy_spec.rb

@@ -30,7 +30,7 @@
       %i[read_issue_link read_software_license_policy]
     end
     let(:additional_reporter_permissions) { [:admin_issue_link] }
-    let(:additional_developer_permissions) { %i[admin_vulnerability_feedback read_project_security_dashboard read_feature_flag] }
+    let(:additional_developer_permissions) { %i[admin_vulnerability_feedback read_project_security_dashboard read_feature_flag dismiss_vulnerability] }
     let(:additional_maintainer_permissions) { %i[push_code_to_protected_branches admin_feature_flags_client] }
     let(:auditor_permissions) do
       %i[
@@ -43,6 +43,7 @@
         create_merge_request_in award_emoji
         read_project_security_dashboard
         read_vulnerability_feedback read_software_license_policy
+        dismiss_vulnerability
       ]
     end
 
@@ -466,16 +467,30 @@
     end
   end
 
+  shared_context 'when security dashboard feature is not available' do
+    before do
+      stub_licensed_features(security_dashboard: false)
+    end
+  end
+
   describe 'read_project_security_dashboard' do
     context 'with developer' do
       let(:current_user) { developer }
 
-      context 'when security dashboard features is not available' do
-        before do
-          stub_licensed_features(security_dashboard: false)
-        end
+      include_context 'when security dashboard feature is not available'
+
+      it { is_expected.to be_disallowed(:read_project_security_dashboard) }
+    end
+  end
+
+  describe 'vulnerability permissions' do
+    describe 'dismiss_vulnerability' do
+      context 'with developer' do
+        let(:current_user) { developer }
+
+        include_context 'when security dashboard feature is not available'
 
-        it { is_expected.to be_disallowed(:read_project_security_dashboard) }
+        it { is_expected.to be_disallowed(:dismiss_vulnerability) }
       end
     end
   end

ee/spec/requests/api/vulnerabilities_spec.rb

@@ -7,7 +7,7 @@
     stub_licensed_features(security_dashboard: true)
   end
 
-  let_it_be(:project) { create(:project, :public, :with_vulnerabilities) }
+  let_it_be(:project) { create(:project, :with_vulnerabilities) }
   let_it_be(:user) { create(:user) }
 
   describe "GET /projects/:id/vulnerabilities" do
@@ -43,6 +43,98 @@
       end
     end
 
-    it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases'
+    it_behaves_like 'forbids access to project vulnerabilities endpoint in expected cases'
+  end
+
+  describe "POST /vulnerabilities:id/dismiss" do
+    before do
+      create_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability, project: vulnerability.project)
+    end
+
+    let(:vulnerability) { project.vulnerabilities.first }
+
+    subject { post api("/vulnerabilities/#{vulnerability.id}/dismiss", user) }
+
+    context 'with an authorized user with proper permissions' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'dismisses a vulnerability and its associated findings' do
+        subject
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(response).to match_response_schema('vulnerability', dir: 'ee')
+
+        expect(vulnerability.reload).to be_closed
+        expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
+      end
+
+      context 'when there is a dismissal error' do
+        before do
+          Grape::Endpoint.before_each do |endpoint|
+            allow(endpoint).to receive(:find_vulnerability!).and_wrap_original do |method, *args|
+              vulnerability = method.call(*args)
+
+              errors = ActiveModel::Errors.new(vulnerability)
+              errors.add(:base, 'something went wrong')
+
+              allow(vulnerability).to receive(:valid?).and_return(false)
+              allow(vulnerability).to receive(:errors).and_return(errors)
+
+              vulnerability
+            end
+          end
+        end
+
+        after do
+          # resetting according to the https://github.com/ruby-grape/grape#stubbing-helpers
+          Grape::Endpoint.before_each nil
+        end
+
+        it 'responds with error' do
+          subject
+
+          expect(response).to have_gitlab_http_status(400)
+          expect(json_response['message']).to eq('base' => ['something went wrong'])
+        end
+      end
+
+      context 'and when security dashboard feature is not available' do
+        before do
+          stub_licensed_features(security_dashboard: false)
+        end
+
+        it 'responds with 403 Forbidden' do
+          subject
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+    end
+
+    context 'when user does not have permissions to create a dismissal feedback' do
+      before do
+        project.add_reporter(user)
+      end
+
+      it 'responds with 403 Forbidden' do
+        subject
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
+    context 'when first-class vulnerabilities feature is disabled' do
+      before do
+        stub_feature_flags(first_class_vulnerabilities: false)
+      end
+
+      it 'responds with 404 Not Found' do
+        subject
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
   end
 end

ee/spec/requests/api/vulnerability_findings_spec.rb

@@ -10,7 +10,7 @@
     let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerability_findings" }
 
     it_behaves_like 'getting list of vulnerability findings'
-    it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases'
+    it_behaves_like 'forbids access to project vulnerabilities endpoint in expected cases'
 
     context 'with an authorized user with proper permissions' do
       before do

ee/spec/services/vulnerabilities/dismiss_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Vulnerabilities::DismissService 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(:vulnerability) { create(:vulnerability, :with_findings, project: project) }
+  let(:service) { described_class.new(user, vulnerability) }
+
+  subject { service.execute }
+
+  context 'with an authorized user with proper permissions' do
+    before do
+      project.add_developer(user)
+    end
+
+    it 'dismisses a vulnerability and its associated findings' do
+      subject
+
+      expect(vulnerability.reload).to be_closed
+      expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
+    end
+
+    context 'when there is a finding dismissal error' do
+      before do
+        allow(service).to receive(:dismiss_findings).and_return(
+          described_class::FindingsDismissResult.new(false, broken_finding, 'something went wrong'))
+      end
+
+      let(:broken_finding) { vulnerability.findings.first }
+
+      it 'responds with error' do
+        expect(subject.errors.messages).to eq(
+          base: ["failed to dismiss associated finding(id=#{broken_finding.id}): something went wrong"])
+      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/helpers/vulnerability_helpers.rb

+# frozen_string_literal: true
+
+RSpec::Matchers.define :have_vulnerability_dismissal_feedback do
+  match do |finding|
+    expect(finding.dismissal_feedback).to have_attributes(project: finding.vulnerability.project,
+                                                          category: finding.report_type,
+                                                          project_fingerprint: finding.project_fingerprint)
+  end
+end

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

 # frozen_string_literal: true
 
-shared_examples 'forbids access to vulnerability-like endpoint in expected cases' do
+shared_examples 'forbids access to project vulnerabilities endpoint in expected cases' do
   context 'with authorized user without read permissions' do
     before do
       project.add_reporter(user)

locale/gitlab.pot

@@ -19619,6 +19619,9 @@ msgstr ""
 msgid "failed"
 msgstr ""
 
+msgid "failed to dismiss associated finding(id=%{finding_id}): %{message}"
+msgstr ""
+
 msgid "for %{link_to_merge_request} with %{link_to_merge_request_source_branch}"
 msgstr ""