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

API call for Resolve Vulnerability

See merge request gitlab-org/gitlab!18439

ee/app/finders/security/pipeline_vulnerabilities_finder.rb

@@ -33,7 +33,9 @@ module Security
 
         raise ParseError, 'JSON parsing failed' if report.error.is_a?(Gitlab::Ci::Parsers::Security::Common::SecurityReportParserError)
 
-        normalized_occurrences = normalize_report_occurrences(report.occurrences)
+        normalized_occurrences = normalize_report_occurrences(
+          report.occurrences,
+          vulnerabilities_by_finding_fingerprint(type, report))
         filtered_occurrences = filter(normalized_occurrences)
 
         occurrences.concat(filtered_occurrences)
@@ -48,12 +50,25 @@ module Security
       pipeline&.security_reports&.reports
     end
 
-    def normalize_report_occurrences(report_occurrences)
+    def vulnerabilities_by_finding_fingerprint(report_type, report)
+      Vulnerabilities::Occurrence
+        .with_vulnerabilities_for_state(
+          project: pipeline.project,
+          report_type: report_type,
+          project_fingerprints: report.occurrences.map(&:project_fingerprint))
+       .each_with_object({}) do |occurrence, hash|
+        hash[occurrence.project_fingerprint] = occurrence.vulnerability
+      end
+    end
+
+    def normalize_report_occurrences(report_occurrences, vulnerabilities)
       report_occurrences.map do |report_occurrence|
         occurrence_hash = report_occurrence.to_hash
           .except(:compare_key, :identifiers, :location, :scanner)
 
         occurrence = Vulnerabilities::Occurrence.new(occurrence_hash)
+        # assigning Vulnerabilities to Findings to enable the computed state
+        occurrence.vulnerability = vulnerabilities[occurrence.project_fingerprint]
 
         occurrence.project = pipeline.project
         occurrence.build_scanner(report_occurrence.scanner.to_hash)

ee/app/models/vulnerabilities/occurrence.rb

@@ -115,6 +115,30 @@ module Vulnerabilities
       end
     end
 
+    def self.with_vulnerabilities_for_state(project:, report_type:, project_fingerprints:)
+      Vulnerabilities::Occurrence
+        .joins(:vulnerability)
+        .where(
+          project: project,
+          report_type: report_type,
+          project_fingerprint: project_fingerprints
+        )
+        .select('report_type, vulnerability_id, project_fingerprint, raw_metadata, '\
+                'vulnerabilities.id, vulnerabilities.state') # fetching only required attributes
+    end
+
+    def state
+      return 'dismissed' if dismissal_feedback.present?
+
+      if vulnerability.nil?
+        'new'
+      elsif vulnerability.closed?
+        'resolved'
+      else
+        'confirmed'
+      end
+    end
+
     def feedback(feedback_type:)
       params = {
         project_id: project_id,

ee/app/models/vulnerability.rb

@@ -10,7 +10,6 @@ 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 }

ee/app/policies/ee/project_policy.rb

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

ee/app/serializers/vulnerabilities/occurrence_entity.rb

@@ -31,6 +31,8 @@ class Vulnerabilities::OccurrenceEntity < Grape::Entity
     expose :solution
   end
 
+  expose :state
+
   expose :blob_path do |occurrence|
     occurrence.present.blob_path
   end

ee/app/services/vulnerabilities/dismiss_service.rb

@@ -6,14 +6,14 @@ module Vulnerabilities
 
     FindingsDismissResult = Struct.new(:ok?, :finding, :message)
 
-    def initialize(current_user, vulnerability)
-      @current_user = current_user
+    def initialize(user, vulnerability)
+      @user = user
       @vulnerability = vulnerability
       @project = vulnerability.project
     end
 
     def execute
-      raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :dismiss_vulnerability, @project)
+      raise Gitlab::Access::AccessDeniedError unless can?(@user, :dismiss_vulnerability, @project)
 
       @vulnerability.transaction do
         result = dismiss_findings
@@ -23,7 +23,7 @@ module Vulnerabilities
           raise ActiveRecord::Rollback
         end
 
-        @vulnerability.update(state: 'closed')
+        @vulnerability.update(state: :closed, closed_by: @user, closed_at: Time.zone.now)
       end
 
       @vulnerability
@@ -32,7 +32,7 @@ module Vulnerabilities
     private
 
     def feedback_service_for(finding)
-      VulnerabilityFeedback::CreateService.new(@project, @current_user, feedback_params_for(finding))
+      VulnerabilityFeedback::CreateService.new(@project, @user, feedback_params_for(finding))
     end
 
     def feedback_params_for(finding)

ee/app/services/vulnerabilities/resolve_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class ResolveService
+    include Gitlab::Allowable
+
+    def initialize(user, vulnerability)
+      @user = user
+      @vulnerability = vulnerability
+    end
+
+    def execute
+      raise Gitlab::Access::AccessDeniedError unless can?(@user, :resolve_vulnerability, @vulnerability.project)
+
+      @vulnerability.tap do |vulnerability|
+        vulnerability.update(state: :closed, closed_by: @user, closed_at: Time.zone.now)
+      end
+    end
+  end
+end

ee/lib/api/vulnerabilities.rb

@@ -38,6 +38,21 @@ module API
       requires :id, type: String, desc: 'The ID of a vulnerability'
     end
     resource :vulnerabilities do
+      desc 'Resolve a vulnerability' do
+        success VulnerabilityEntity
+      end
+      post ':id/resolve' do
+        if Feature.enabled?(:first_class_vulnerabilities)
+          vulnerability = find_and_authorize_vulnerability!(:resolve_vulnerability)
+          break not_modified! if vulnerability.closed?
+
+          vulnerability = ::Vulnerabilities::ResolveService.new(current_user, vulnerability).execute
+          render_vulnerability(vulnerability)
+        else
+          not_found!
+        end
+      end
+
       desc 'Dismiss a vulnerability' do
         success VulnerabilityEntity
       end
@@ -61,7 +76,7 @@ module API
       params do
         # These params have no effect for Vulnerabilities API but are required to support falling back to
         # responding with Vulnerability Findings when :first_class_vulnerabilities feature is disabled.
-        # TODO: remove usage of :vulnerability_findings_params when feature flag is removed
+        # TODO: replace :vulnerability_findings_params with just :pagination when feature flag is removed
         # https://gitlab.com/gitlab-org/gitlab/issues/33488
         use :vulnerability_findings_params
       end

ee/spec/factories/vulnerabilities/occurrences.rb

@@ -5,7 +5,7 @@ FactoryBot.define do
     Digest::SHA1.hexdigest("uuid-#{n}")[0..35]
   end
 
-  factory :vulnerabilities_occurrence, class: Vulnerabilities::Occurrence do
+  factory :vulnerabilities_occurrence, class: Vulnerabilities::Occurrence, aliases: [:vulnerabilities_finding] do
     name { 'Cipher with no integrity' }
     project
     sequence(:uuid) { generate(:vulnerability_occurrence_uuid) }
@@ -36,5 +36,27 @@ FactoryBot.define do
         ]
       }.to_json
     end
+
+    trait :confirmed do
+      after(:create) do |finding|
+        create(:vulnerability, :opened, project: finding.project, findings: [finding])
+      end
+    end
+
+    trait :resolved do
+      after(:create) do |finding|
+        create(:vulnerability, :closed, project: finding.project, findings: [finding])
+      end
+    end
+
+    trait :dismissed do
+      after(:create) do |finding|
+        create(:vulnerability, :closed, project: finding.project, findings: [finding])
+        create(:vulnerability_feedback,
+               :dismissal,
+               project: finding.project,
+               project_fingerprint: finding.project_fingerprint)
+      end
+    end
   end
 end

ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb

@@ -218,6 +218,54 @@ describe Security::PipelineVulnerabilitiesFinder do
       end
     end
 
+    context 'when matching vulnerability records exist' do
+      before do
+        create(:vulnerabilities_finding,
+               :confirmed,
+               project: project,
+               report_type: 'sast',
+               project_fingerprint: confirmed_fingerprint)
+        create(:vulnerabilities_finding,
+               :resolved,
+               project: project,
+               report_type: 'sast',
+               project_fingerprint: resolved_fingerprint)
+        create(:vulnerabilities_finding,
+               :dismissed,
+               project: project,
+               report_type: 'sast',
+               project_fingerprint: dismissed_fingerprint)
+      end
+
+      let(:confirmed_fingerprint) do
+        Digest::SHA1.hexdigest(
+          'python/hardcoded/hardcoded-tmp.py:52865813c884a507be1f152d654245af34aba8a391626d01f1ab6d3f52ec8779:B108')
+      end
+
+      let(:resolved_fingerprint) do
+        Digest::SHA1.hexdigest(
+          'groovy/src/main/java/com/gitlab/security_products/tests/App.groovy:47:PREDICTABLE_RANDOM')
+      end
+
+      let(:dismissed_fingerprint) do
+        Digest::SHA1.hexdigest(
+          'groovy/src/main/java/com/gitlab/security_products/tests/App.groovy:41:PREDICTABLE_RANDOM')
+      end
+
+      subject { described_class.new(pipeline: pipeline, params: { report_type: %w[sast], scope: 'all' }).execute }
+
+      it 'assigns vulnerability records to findings providing them with computed state' do
+        confirmed = subject.find { |f| f.project_fingerprint == confirmed_fingerprint }
+        resolved = subject.find { |f| f.project_fingerprint == resolved_fingerprint }
+        dismissed = subject.find { |f| f.project_fingerprint == dismissed_fingerprint }
+
+        expect(confirmed.state).to eq 'confirmed'
+        expect(resolved.state).to eq 'resolved'
+        expect(dismissed.state).to eq 'dismissed'
+        expect(subject - [confirmed, resolved, dismissed]).to all have_attributes(state: 'new')
+      end
+    end
+
     def read_fixture(fixture)
       JSON.parse(File.read(fixture.file.path))
     end

ee/spec/models/vulnerabilities/occurrence_spec.rb

@@ -356,4 +356,47 @@ describe Vulnerabilities::Occurrence do
       end
     end
   end
+
+  describe '#state' do
+    let(:new_finding) { create(:vulnerabilities_finding) }
+    let(:confirmed_finding) { create(:vulnerabilities_finding, :confirmed) }
+    let(:resolved_finding) { create(:vulnerabilities_finding, :resolved) }
+    let(:dismissed_finding) { create(:vulnerabilities_finding, :dismissed) }
+
+    it 'returns the expected state for a new finding' do
+      expect(new_finding.state).to eq 'new'
+    end
+
+    it 'returns the expected state for a confirmed finding' do
+      expect(confirmed_finding.state).to eq 'confirmed'
+    end
+
+    it 'returns the expected state for a resolved finding' do
+      expect(resolved_finding.state).to eq 'resolved'
+    end
+
+    it 'returns the expected state for a dismissed finding' do
+      expect(dismissed_finding.state).to eq 'dismissed'
+    end
+
+    context 'when a vulnerability present for a dismissed finding' do
+      before do
+        create(:vulnerability, project: dismissed_finding.project, findings: [dismissed_finding])
+      end
+
+      it 'still reports a dismissed state' do
+        expect(dismissed_finding.state).to eq 'dismissed'
+      end
+    end
+
+    context 'when a non-dismissal feedback present for a finding belonging to a closed vulnerability' do
+      before do
+        create(:vulnerability_feedback, :issue, project: resolved_finding.project)
+      end
+
+      it 'reports as resolved' do
+        expect(resolved_finding.state).to eq 'resolved'
+      end
+    end
+  end
 end

ee/spec/policies/project_policy_spec.rb

@@ -30,7 +30,12 @@ describe ProjectPolicy do
       %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 dismiss_vulnerability] }
+    let(:additional_developer_permissions) do
+      %i[
+        admin_vulnerability_feedback read_project_security_dashboard read_feature_flag
+        resolve_vulnerability dismiss_vulnerability
+      ]
+    end
     let(:additional_maintainer_permissions) { %i[push_code_to_protected_branches admin_feature_flags_client] }
     let(:auditor_permissions) do
       %i[
@@ -41,9 +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
+        read_project_security_dashboard resolve_vulnerability dismiss_vulnerability
         read_vulnerability_feedback read_software_license_policy
-        dismiss_vulnerability
       ]
     end
 
@@ -490,6 +494,7 @@ describe ProjectPolicy do
 
         include_context 'when security dashboard feature is not available'
 
+        it { is_expected.to be_disallowed(:resolve_vulnerability) }
         it { is_expected.to be_disallowed(:dismiss_vulnerability) }
       end
     end

ee/spec/requests/api/vulnerabilities_spec.rb

@@ -61,14 +61,17 @@ describe API::Vulnerabilities do
       end
 
       it 'dismisses a vulnerability and its associated findings' do
+        Timecop.freeze 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.reload).to(
+            have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
           expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
         end
+      end
 
       context 'when there is a dismissal error' do
         before do
@@ -147,4 +150,79 @@ describe API::Vulnerabilities do
       end
     end
   end
+
+  describe "POST /vulnerabilities:id/resolve" do
+    before do
+      create_list(:vulnerabilities_finding, 2, vulnerability: vulnerability)
+    end
+
+    let(:vulnerability) { project.vulnerabilities.first }
+
+    subject { post api("/vulnerabilities/#{vulnerability.id}/resolve", user) }
+
+    context 'with an authorized user with proper permissions' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'resolves a vulnerability and its associated findings' do
+        Timecop.freeze do
+          subject
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(response).to match_response_schema('vulnerability', dir: 'ee')
+
+          expect(vulnerability.reload).to(
+            have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
+          expect(vulnerability.findings).to all have_attributes(state: 'resolved')
+        end
+      end
+
+      context 'when the vulnerability is already resolved' do
+        let(:vulnerability) { create(:vulnerability, :closed, project: project) }
+
+        it 'responds with 304 Not Modified response' do
+          subject
+
+          expect(response).to have_gitlab_http_status(304)
+        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 resolve a vulnerability' 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/services/vulnerabilities/dismiss_service_spec.rb

@@ -20,11 +20,14 @@ describe Vulnerabilities::DismissService do
     end
 
     it 'dismisses a vulnerability and its associated findings' do
+      Timecop.freeze do
         subject
 
-      expect(vulnerability.reload).to be_closed
+        expect(vulnerability.reload).to(
+          have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
         expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
       end
+    end
 
     context 'when there is a finding dismissal error' do
       before do

ee/spec/services/vulnerabilities/resolve_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Vulnerabilities::ResolveService 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, 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 'resolves a vulnerability' do
+      Timecop.freeze do
+        subject
+
+        expect(vulnerability.reload).to(
+          have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
+      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

@@ -108,7 +108,9 @@ shared_examples 'getting list of vulnerability findings' do
         get api(project_vulnerabilities_path, user), params: { report_type: 'dependency_scanning' }
       end.count
 
-      expect { get api(project_vulnerabilities_path, user) }.not_to exceed_query_limit(control_count)
+      # Threshold is required for the extra query performed in Security::PipelineVulnerabilitiesFinder to load
+      # the Vulnerabilities providing computed states for the associated Vulnerability::Occurrences
+      expect { get api(project_vulnerabilities_path, user) }.not_to exceed_query_limit(control_count).with_threshold(1)
     end
 
     describe 'filtering' do