Merge branch '230558-allow-reverting-vulnerabilities-to-detected' into 'master'

Add services to allow revert vulnerabilities to detected state

See merge request gitlab-org/gitlab!41781

ee/app/helpers/ee/system_note_helper.rb

@@ -19,6 +19,7 @@ module EE
       'weight' => 'weight',
       'relate_epic' => 'epic',
       'unrelate_epic' => 'epic',
+      'vulnerability_detected' => 'search-dot',
       'vulnerability_confirmed' => 'shield',
       'vulnerability_dismissed' => 'cancel',
       'vulnerability_resolved' => 'status_closed',

ee/app/models/ee/system_note_metadata.rb

@@ -8,7 +8,7 @@ module EE
       weight published
       epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
       epic_issue_moved issue_changed_epic epic_date_changed relate_epic unrelate_epic
-      vulnerability_confirmed vulnerability_dismissed vulnerability_resolved
+      vulnerability_confirmed vulnerability_dismissed vulnerability_resolved vulnerability_detected
       iteration
     ].freeze
 

ee/app/models/ee/vulnerability.rb

@@ -40,6 +40,7 @@ module EE
       has_one :group, through: :project
 
       has_many :findings, class_name: '::Vulnerabilities::Finding', inverse_of: :vulnerability
+      has_many :dismissed_findings, -> { dismissed }, class_name: 'Vulnerabilities::Finding', inverse_of: :vulnerability
       has_many :issue_links, class_name: '::Vulnerabilities::IssueLink', inverse_of: :vulnerability
       has_many :created_issue_links, -> { created }, class_name: '::Vulnerabilities::IssueLink', inverse_of: :vulnerability
       has_many :related_issues, through: :issue_links, source: :issue do

ee/app/models/vulnerabilities/finding.rb

@@ -147,15 +147,21 @@ module Vulnerabilities
       end
     end
 
+    def self.related_dismissal_feedback
+      Feedback
+      .where(arel_table[:report_type].eq(Feedback.arel_table[:category]))
+      .where(arel_table[:project_id].eq(Feedback.arel_table[:project_id]))
+      .where(Arel::Nodes::NamedFunction.new('ENCODE', [arel_table[:project_fingerprint], Arel::Nodes::SqlLiteral.new("'HEX'")]).eq(Feedback.arel_table[:project_fingerprint]))
+      .for_dismissal
+    end
+    private_class_method :related_dismissal_feedback
+
+    def self.dismissed
+      where('EXISTS (?)', related_dismissal_feedback.select(1))
+    end
+
     def self.undismissed
-      where(
-        "NOT EXISTS (?)",
-        Feedback.select(1)
-        .where("#{table_name}.report_type = vulnerability_feedback.category")
-        .where("#{table_name}.project_id = vulnerability_feedback.project_id")
-        .where("ENCODE(#{table_name}.project_fingerprint, 'HEX') = vulnerability_feedback.project_fingerprint") # rubocop:disable GitlabSecurity/SqlInjection
-        .for_dismissal
-      )
+      where('NOT EXISTS (?)', related_dismissal_feedback.select(1))
     end
 
     def self.batch_count_by_project_and_severity(project_id, severity)

ee/app/services/ee/system_notes/vulnerabilities_service.rb

@@ -5,7 +5,8 @@ module EE
     class VulnerabilitiesService < ::SystemNotes::BaseService
       # Called when state is changed for 'vulnerability'
       def change_vulnerability_state
-        body = "changed vulnerability status to #{noteable.state}"
+        type = noteable.detected? ? 'reverted' : 'changed'
+        body = "#{type} vulnerability status to #{noteable.state}"
 
         create_note(NoteSummary.new(noteable, project, author, body, action: "vulnerability_#{noteable.state}"))
       end

ee/app/services/vulnerabilities/revert_to_detected_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class RevertToDetectedService < BaseService
+    include Gitlab::Allowable
+
+    REVERT_PARAMS = { resolved_by: nil, resolved_at: nil, dismissed_by: nil, dismissed_at: nil, confirmed_by: nil, confirmed_at: nil }.freeze
+
+    def execute
+      raise Gitlab::Access::AccessDeniedError unless authorized?
+
+      @vulnerability.transaction do
+        revert_result = revert_findings_to_detected_state
+        raise ActiveRecord::Rollback unless revert_result
+
+        update_with_note(@vulnerability, state: Vulnerability.states[:detected], **REVERT_PARAMS)
+      end
+
+      @vulnerability
+    end
+
+    private
+
+    def destroy_feedback_for(finding)
+      VulnerabilityFeedback::DestroyService
+        .new(@project, @user, finding.dismissal_feedback)
+        .execute
+    end
+
+    def revert_findings_to_detected_state
+      @vulnerability
+        .dismissed_findings
+        .each do |finding|
+          result = destroy_feedback_for(finding)
+
+          unless result
+            handle_finding_revert_error(finding)
+            return false
+          end
+        end
+
+      true
+    end
+
+    def handle_finding_revert_error(finding)
+      @vulnerability.errors.add(
+        :base,
+        :finding_revert_to_detected_error,
+        message: _("failed to revert associated finding(id=%{finding_id}) to detected") %
+          {
+            finding_id: finding.id
+          })
+    end
+  end
+end

ee/spec/models/ee/system_note_metadata_spec.rb

@@ -4,7 +4,7 @@ require 'spec_helper'
 
 RSpec.describe EE::SystemNoteMetadata do
   %i[
-    vulnerability_confirmed vulnerability_dismissed vulnerability_resolved
+    vulnerability_confirmed vulnerability_dismissed vulnerability_resolved vulnerability_detected
   ].each do |action|
     context 'when action type is valid' do
       subject do

ee/spec/models/ee/vulnerability_spec.rb

@@ -33,6 +33,7 @@ RSpec.describe Vulnerability do
     it { is_expected.to belong_to(:milestone) }
     it { is_expected.to belong_to(:epic) }
     it { is_expected.to have_many(:findings).class_name('Vulnerabilities::Finding').inverse_of(:vulnerability) }
+    it { is_expected.to have_many(:dismissed_findings).class_name('Vulnerabilities::Finding').inverse_of(:vulnerability) }
     it { is_expected.to have_many(:issue_links).class_name('Vulnerabilities::IssueLink').inverse_of(:vulnerability) }
     it { is_expected.to have_many(:created_issue_links).class_name('Vulnerabilities::IssueLink').inverse_of(:vulnerability).conditions(link_type: Vulnerabilities::IssueLink.link_types['created']) }
     it { is_expected.to have_many(:related_issues).through(:issue_links).source(:issue) }

ee/spec/models/vulnerabilities/finding_spec.rb

@@ -252,6 +252,44 @@ RSpec.describe Vulnerabilities::Finding do
     end
   end
 
+  describe '.dismissed' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:project2) { create(:project) }
+    let!(:finding1) { create(:vulnerabilities_finding, project: project) }
+    let!(:finding2) { create(:vulnerabilities_finding, project: project, report_type: :dast) }
+    let!(:finding3) { create(:vulnerabilities_finding, project: project2) }
+
+    before do
+      create(
+        :vulnerability_feedback,
+        :dismissal,
+        project: finding1.project,
+        project_fingerprint: finding1.project_fingerprint
+      )
+      create(
+        :vulnerability_feedback,
+        :dismissal,
+        project_fingerprint: finding2.project_fingerprint,
+        project: project2
+      )
+      create(
+        :vulnerability_feedback,
+        :dismissal,
+        category: :sast,
+        project_fingerprint: finding2.project_fingerprint,
+        project: finding2.project
+      )
+    end
+
+    it 'returns all dismissed findings' do
+      expect(described_class.dismissed).to contain_exactly(finding1)
+    end
+
+    it 'returns dismissed findings for project' do
+      expect(project.vulnerability_findings.dismissed).to contain_exactly(finding1)
+    end
+  end
+
   describe '.batch_count_by_project_and_severity' do
     let(:pipeline) { create(:ci_pipeline, :success, project: project) }
     let(:project) { create(:project) }

ee/spec/services/vulnerabilities/revert_to_detected_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::RevertToDetectedService do
+  include AccessMatchersGeneric
+
+  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(:revert_vulnerability_to_detected) { service.execute }
+
+  shared_examples 'reverts vulnerability' do
+    it 'reverts a vulnerability and its associated findings to detected state' do
+      Timecop.freeze do
+        revert_vulnerability_to_detected
+
+        expect(vulnerability.reload).to(
+          have_attributes(state: 'detected', dismissed_by: nil, dismissed_at: nil, resolved_by: nil, resolved_at: nil, confirmed_by: nil, confirmed_at: nil))
+        expect(vulnerability.findings).to all not_have_vulnerability_dismissal_feedback
+      end
+    end
+
+    it 'creates note' do
+      expect(SystemNoteService).to receive(:change_vulnerability_state).with(vulnerability, user)
+
+      revert_vulnerability_to_detected
+    end
+
+    it_behaves_like 'calls vulnerability statistics utility services in order'
+  end
+
+  context 'with an authorized user with proper permissions' do
+    before do
+      project.add_developer(user)
+    end
+
+    context 'when vulnerability is dismissed' do
+      let(:vulnerability) { create(:vulnerability, :dismissed, :with_findings, project: project) }
+
+      include_examples 'reverts vulnerability'
+    end
+
+    context 'when vulnerability is confirmed' do
+      let(:vulnerability) { create(:vulnerability, :confirmed, :with_findings, project: project) }
+
+      include_examples 'reverts vulnerability'
+    end
+
+    context 'when vulnerability is resolved' do
+      let(:vulnerability) { create(:vulnerability, :resolved, :with_findings, project: project) }
+
+      include_examples 'reverts vulnerability'
+    end
+
+    context 'when there is an error' do
+      let(:broken_finding) { vulnerability.findings.first }
+
+      let!(:dismissal_feedback) do
+        create(:vulnerability_feedback, :dismissal, project: broken_finding.project, project_fingerprint: broken_finding.project_fingerprint)
+      end
+
+      before do
+        allow(service).to receive(:destroy_feedback_for).and_return(false)
+      end
+
+      it 'responds with error' do
+        expect(revert_vulnerability_to_detected.errors.messages).to eq(
+          base: ["failed to revert associated finding(id=#{broken_finding.id}) to detected"])
+      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 { revert_vulnerability_to_detected }.to raise_error(Gitlab::Access::AccessDeniedError)
+      end
+    end
+  end
+
+  describe 'permissions' do
+    it { expect { revert_vulnerability_to_detected }.to be_allowed_for(:admin) }
+    it { expect { revert_vulnerability_to_detected }.to be_allowed_for(:owner).of(project) }
+    it { expect { revert_vulnerability_to_detected }.to be_allowed_for(:maintainer).of(project) }
+    it { expect { revert_vulnerability_to_detected }.to be_allowed_for(:developer).of(project) }
+
+    it { expect { revert_vulnerability_to_detected }.to be_denied_for(:auditor) }
+    it { expect { revert_vulnerability_to_detected }.to be_denied_for(:reporter).of(project) }
+    it { expect { revert_vulnerability_to_detected }.to be_denied_for(:guest).of(project) }
+    it { expect { revert_vulnerability_to_detected }.to be_denied_for(:anonymous) }
+  end
+end

ee/spec/support/helpers/vulnerability_helpers.rb

@@ -7,3 +7,5 @@ RSpec::Matchers.define :have_vulnerability_dismissal_feedback do
                                                           project_fingerprint: finding.project_fingerprint)
   end
 end
+
+RSpec::Matchers.define_negated_matcher :not_have_vulnerability_dismissal_feedback, :have_vulnerability_dismissal_feedback

locale/gitlab.pot

@@ -30182,6 +30182,9 @@ msgstr ""
 msgid "failed to dismiss associated finding(id=%{finding_id}): %{message}"
 msgstr ""
 
+msgid "failed to revert associated finding(id=%{finding_id}) to detected"
+msgstr ""
+
 msgid "file"
 msgid_plural "files"
 msgstr[0] ""