Adjust services to dismiss and undismiss findings

8f65712e · Subashis Chakraborty · Mehmet Emin INAC · 317ba63c · 8f65712e · 8f65712e
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -5308,6 +5308,7 @@ Input type: `SecurityFindingDismissInput`
 | ---- | ---- | ----------- |
 | <a id="mutationsecurityfindingdismissclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
 | <a id="mutationsecurityfindingdismisserrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
+| <a id="mutationsecurityfindingdismisssecurityfinding"></a>`securityFinding` | [`PipelineSecurityReportFinding`](#pipelinesecurityreportfinding) | Dismissed finding. |
 | <a id="mutationsecurityfindingdismissuuid"></a>`uuid` | [`String`](#string) | UUID of dismissed finding. |
  
 ### `Mutation.securityFindingRevertToDetected`
@@ -10736,6 +10737,29 @@ The edge type for [`VulnerabilityScanner`](#vulnerabilityscanner).
 | <a id="vulnerabilityscanneredgecursor"></a>`cursor` | [`String!`](#string) | A cursor for use in pagination. |
 | <a id="vulnerabilityscanneredgenode"></a>`node` | [`VulnerabilityScanner`](#vulnerabilityscanner) | The item at the end of the edge. |
  
+#### `VulnerabilityStateTransitionTypeConnection`
+
+The connection type for [`VulnerabilityStateTransitionType`](#vulnerabilitystatetransitiontype).
+
+##### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="vulnerabilitystatetransitiontypeconnectionedges"></a>`edges` | [`[VulnerabilityStateTransitionTypeEdge]`](#vulnerabilitystatetransitiontypeedge) | A list of edges. |
+| <a id="vulnerabilitystatetransitiontypeconnectionnodes"></a>`nodes` | [`[VulnerabilityStateTransitionType]`](#vulnerabilitystatetransitiontype) | A list of nodes. |
+| <a id="vulnerabilitystatetransitiontypeconnectionpageinfo"></a>`pageInfo` | [`PageInfo!`](#pageinfo) | Information to aid in pagination. |
+
+#### `VulnerabilityStateTransitionTypeEdge`
+
+The edge type for [`VulnerabilityStateTransitionType`](#vulnerabilitystatetransitiontype).
+
+##### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="vulnerabilitystatetransitiontypeedgecursor"></a>`cursor` | [`String!`](#string) | A cursor for use in pagination. |
+| <a id="vulnerabilitystatetransitiontypeedgenode"></a>`node` | [`VulnerabilityStateTransitionType`](#vulnerabilitystatetransitiontype) | The item at the end of the edge. |
+
 #### `WorkItemConnection`
  
 The connection type for [`WorkItem`](#workitem).
@@ -21450,6 +21474,7 @@ Represents a vulnerability.
 | <a id="vulnerabilityseverity"></a>`severity` | [`VulnerabilitySeverity`](#vulnerabilityseverity) | Severity of the vulnerability (INFO, UNKNOWN, LOW, MEDIUM, HIGH, CRITICAL). |
 | <a id="vulnerabilitystate"></a>`state` | [`VulnerabilityState`](#vulnerabilitystate) | State of the vulnerability (DETECTED, CONFIRMED, RESOLVED, DISMISSED). |
 | <a id="vulnerabilitystatecomment"></a>`stateComment` | [`String`](#string) | Comment given for the vulnerability state change. |
+| <a id="vulnerabilitystatetransitions"></a>`stateTransitions` | [`VulnerabilityStateTransitionTypeConnection`](#vulnerabilitystatetransitiontypeconnection) | List of state transitions related to the vulnerability. (see [Connections](#connections)) |
 | <a id="vulnerabilitytitle"></a>`title` | [`String`](#string) | Title of the vulnerability. |
 | <a id="vulnerabilityupdatedat"></a>`updatedAt` | [`Time`](#time) | Timestamp of when the vulnerability was last updated. |
 | <a id="vulnerabilityusernotescount"></a>`userNotesCount` | [`Int!`](#int) | Number of user notes attached to the vulnerability. |
@@ -21950,6 +21975,21 @@ Represents vulnerability counts by severity.
 | <a id="vulnerabilityseveritiescountmedium"></a>`medium` | [`Int`](#int) | Number of vulnerabilities of MEDIUM severity of the project. |
 | <a id="vulnerabilityseveritiescountunknown"></a>`unknown` | [`Int`](#int) | Number of vulnerabilities of UNKNOWN severity of the project. |
  
+### `VulnerabilityStateTransitionType`
+
+Represents a state transition of a vulnerability.
+
+#### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="vulnerabilitystatetransitiontypeauthor"></a>`author` | [`UserCore!`](#usercore) | User who changed the state of the vulnerability. |
+| <a id="vulnerabilitystatetransitiontypecomment"></a>`comment` | [`String`](#string) | Comment for the state change. |
+| <a id="vulnerabilitystatetransitiontypecreatedat"></a>`createdAt` | [`Time!`](#time) | Time of the state change of the vulnerability. |
+| <a id="vulnerabilitystatetransitiontypedismissalreason"></a>`dismissalReason` | [`VulnerabilityDismissalReason`](#vulnerabilitydismissalreason) | Reason for the dismissal. |
+| <a id="vulnerabilitystatetransitiontypefromstate"></a>`fromState` | [`VulnerabilityState!`](#vulnerabilitystate) | State of the vulnerability before transition. |
+| <a id="vulnerabilitystatetransitiontypetostate"></a>`toState` | [`VulnerabilityState!`](#vulnerabilitystate) | State of the vulnerability after transition. |
+
 ### `VulnerableDependency`
  
 Represents a vulnerable dependency. Used in vulnerability location data.
--- a/ee/app/controllers/projects/vulnerability_feedback_controller.rb
+++ b/ee/app/controllers/projects/vulnerability_feedback_controller.rb
@@ -97,7 +97,7 @@ def authorize_update_vulnerability_feedback!
  def serializer
    # This is needed as we do not want to depend on Vulnerabilities::Feedback in the frontend anymore.
    if Feature.enabled?(:deprecate_vulnerabilities_feedback, project)
-      VulnerabilitySerializer.new
+      VulnerabilitySerializer.new(current_user: current_user)
    else
      Vulnerabilities::FeedbackSerializer.new(current_user: current_user, project: project)
    end

--- a/ee/app/graphql/mutations/security/finding/dismiss.rb
+++ b/ee/app/graphql/mutations/security/finding/dismiss.rb
@@ -12,6 +12,11 @@ class Dismiss < BaseMutation
              null: true,
              description: 'UUID of dismissed finding.'

+        field :security_finding,
+              ::Types::PipelineSecurityReportFindingType,
+              null: true,
+              description: 'Dismissed finding.'
+
        argument :uuid,
                GraphQL::Types::String,
                required: true,
@@ -33,6 +38,7 @@ def resolve(uuid:, comment: nil, dismissal_reason: nil)

          {
            uuid: result.success? ? result.payload[:security_finding][:uuid] : nil,
+            security_finding: result.success? ? result.payload[:security_finding] : nil,
            errors: Array(result.message)
          }
        end

--- a/ee/app/graphql/resolvers/vulnerabilities_resolver.rb
+++ b/ee/app/graphql/resolvers/vulnerabilities_resolver.rb
@@ -81,6 +81,7 @@ def preloads
        has_solutions: { vulnerability: { findings: :remediations } },
        merge_request: { vulnerability: :merge_requests },
        state_comment: { vulnerability: :state_transitions },
+        state_transitions: { vulnerability: :state_transitions },
        false_positive: { vulnerability: { findings: :vulnerability_flags } }
      }
    end

--- a/ee/app/graphql/types/pipeline_security_report_finding_type.rb
+++ b/ee/app/graphql/types/pipeline_security_report_finding_type.rb
@@ -162,7 +162,8 @@ class PipelineSecurityReportFindingType < BaseObject

    def vulnerability
      BatchLoader::GraphQL.for(object.uuid).batch do |uuids, loader|
-        ::Vulnerability.with_findings_by_uuid(uuids).each do |vulnerability|
+        ::Vulnerability.with_findings_by_uuid(uuids)
+          .with_state_transitions.each do |vulnerability|
          loader.call(vulnerability.finding.uuid, vulnerability)
        end
      end

--- a/ee/app/graphql/types/vulnerability/state_transition_type.rb
+++ b/ee/app/graphql/types/vulnerability/state_transition_type.rb
+# frozen_string_literal: true
+
+module Types
+  module Vulnerability
+    class StateTransitionType < BaseObject
+      graphql_name 'VulnerabilityStateTransitionType'
+
+      description 'Represents a state transition of a vulnerability'
+
+      authorize :read_security_resource
+
+      field :from_state, VulnerabilityStateEnum,
+        null: false, description: 'State of the vulnerability before transition.'
+
+      field :to_state, VulnerabilityStateEnum,
+        null: false, description: 'State of the vulnerability after transition.'
+
+      field :comment, GraphQL::Types::String,
+        null: true, description: 'Comment for the state change.'
+
+      field :dismissal_reason, Types::Vulnerabilities::DismissalReasonEnum,
+        null: true, description: 'Reason for the dismissal.'
+
+      field :author, ::Types::UserType,
+        null: false, description: 'User who changed the state of the vulnerability.'
+
+      field :created_at, ::Types::TimeType,
+        null: false, description: 'Time of the state change of the vulnerability.'
+    end
+  end
+end
--- a/ee/app/graphql/types/vulnerability_type.rb
+++ b/ee/app/graphql/types/vulnerability_type.rb
@@ -116,6 +116,10 @@ class VulnerabilityType < BaseObject
    field :web_url, GraphQL::Types::String,
      null: true, description: "URL to the vulnerability's details page."

+    field :state_transitions, ::Types::Vulnerability::StateTransitionType.connection_type,
+      null: true,
+      description: "List of state transitions related to the vulnerability."
+
    def confirmed_by
      ::Gitlab::Graphql::Loaders::BatchModelLoader.new(::User, object.confirmed_by_id).find
    end

--- a/ee/app/models/ee/vulnerability.rb
+++ b/ee/app/models/ee/vulnerability.rb
@@ -78,6 +78,7 @@ def with_vulnerability_links

      scope :with_author_and_project, -> { includes(:author, :project) }
      scope :with_findings, -> { includes(:findings) }
+      scope :with_state_transitions, -> { includes(:state_transitions) }
      scope :with_findings_by_uuid, -> (uuid) { with_findings.where(findings: { uuid: uuid }) }
      scope :with_findings_by_uuid_and_state, -> (uuid, state) { with_findings.where(findings: { uuid: uuid }, state: state) }
      scope :with_findings_excluding_uuid, -> (uuid) { joins(:findings).merge(Vulnerabilities::Finding.excluding_uuids(uuid)) }

--- a/ee/app/policies/vulnerabilities/state_transition_policy.rb
+++ b/ee/app/policies/vulnerabilities/state_transition_policy.rb
+# frozen_string_literal: true
+
+module Vulnerabilities
+  class StateTransitionPolicy < BasePolicy
+    delegate { @subject.vulnerability.project }
+
+    rule { ~can?(:read_security_resource) }
+  end
+end
--- a/ee/app/services/vulnerabilities/find_or_create_from_security_finding_service.rb
+++ b/ee/app/services/vulnerabilities/find_or_create_from_security_finding_service.rb
@@ -37,7 +37,13 @@ def find_or_create_vulnerability(vulnerability_finding)
        ).execute
      else
        vulnerability = Vulnerability.find(vulnerability_finding.vulnerability_id)
-        update_state_for(vulnerability) if vulnerability.state != @state.to_s
+
+        if vulnerability.state != @state.to_s
+          update_state_for(vulnerability)
+        elsif vulnerability.dismissed? # We only update when vulnerability is in dismissed state
+          update_existing_state_transition(vulnerability)
+        end
+
        vulnerability
      end
    end
@@ -60,6 +66,11 @@ def update_state_for(vulnerability)
      end
    end

+    def update_existing_state_transition(vulnerability)
+      state_transition = vulnerability.state_transitions.by_to_states(:dismissed).last
+      state_transition.update!(comment: params[:comment].presence) if state_transition
+    end
+
    def with_vulnerability_finding
      response = ::Vulnerabilities::Findings::FindOrCreateFromSecurityFindingService.new(
        project: project,

--- a/ee/spec/graphql/mutations/security/finding/dismiss_spec.rb
+++ b/ee/spec/graphql/mutations/security/finding/dismiss_spec.rb
@@ -13,6 +13,7 @@

    let(:comment) { 'Dismissal Feedback' }
    let(:mutated_finding_uuid) { subject[:uuid] }
+    let(:mutated_finding) { subject[:security_finding] }

    subject { mutation.resolve(uuid: finding_uuid, comment: comment, dismissal_reason: 'used_in_tests') }

@@ -46,6 +47,11 @@
            expect(mutated_finding_uuid).to eq(finding_uuid)
            expect(subject[:errors]).to be_empty
          end
+
+          it 'returns the dismissed security finding' do
+            expect(mutated_finding).to eq(security_finding)
+            expect(subject[:errors]).to be_empty
+          end
        end

        context 'when the dismissal fails' do
@@ -63,6 +69,11 @@
            expect(mutated_finding_uuid).to be_nil
            expect(subject[:errors]).to match_array(['error'])
          end
+
+          it 'raises an error and no security finding is returned' do
+            expect(mutated_finding).to be_nil
+            expect(subject[:errors]).to match_array(['error'])
+          end
        end
      end
    end

--- a/ee/spec/graphql/types/vulnerability/state_transition_type_spec.rb
+++ b/ee/spec/graphql/types/vulnerability/state_transition_type_spec.rb
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GitlabSchema.types['VulnerabilityStateTransitionType'], feature_category: :vulnerability_management do
+  let(:expected_fields) { %i[from_state to_state comment dismissal_reason author created_at] }
+
+  subject { described_class }
+
+  it { is_expected.to have_graphql_fields(expected_fields) }
+  it { is_expected.to require_graphql_authorizations(:read_security_resource) }
+end
--- a/ee/spec/graphql/types/vulnerability_type_spec.rb
+++ b/ee/spec/graphql/types/vulnerability_type_spec.rb
@@ -58,7 +58,8 @@
       resolved_by
       dismissed_by
       details
-       commenters]
+       commenters
+       state_transitions]
  end

  RSpec.shared_examples "N+1 queries" do |single_query_count, multiple_queries_count|

--- a/ee/spec/policies/vulnerabilities/state_transition_policy_spec.rb
+++ b/ee/spec/policies/vulnerabilities/state_transition_policy_spec.rb
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::StateTransitionPolicy, feature_category: :vulnerability_management do
+  describe 'read_security_resource' do
+    let(:user) { create(:user) }
+    let(:state_transition) { create(:vulnerability_state_transitions) }
+
+    subject { described_class.new(user, state_transition) }
+
+    context 'when the security_dashboard feature is enabled' do
+      before do
+        stub_licensed_features(security_dashboard: true)
+      end
+
+      context "when the current user is not a project member" do
+        it { is_expected.to be_disallowed(:read_security_resource) }
+      end
+
+      context "when the current user has developer access to the vulnerability's project" do
+        before do
+          state_transition.vulnerability.project.add_developer(user)
+        end
+
+        it { is_expected.to be_allowed(:read_security_resource) }
+      end
+    end
+
+    context 'when the security_dashboard feature is disabled' do
+      before do
+        stub_licensed_features(security_dashboard: false)
+
+        state_transition.vulnerability.project.add_developer(user)
+      end
+
+      it { is_expected.to be_disallowed(:read_security_resource) }
+    end
+  end
+end
--- a/ee/spec/services/vulnerabilities/find_or_create_from_security_finding_service_spec.rb
+++ b/ee/spec/services/vulnerabilities/find_or_create_from_security_finding_service_spec.rb
@@ -49,7 +49,7 @@
  it 'matches an expected checksum' do
    code_file_path = Rails.root.join("ee/app/services/vulnerabilities/find_or_create_from_security_finding_service.rb")
    code_definition = File.read(code_file_path)
-    expected_checksum = "607625f9434aca06e505da03296dd93cb34543908f77e51c37af5618c58d8222"
+    expected_checksum = "6cbf54411beb5c5ca363242939b5b366e66fa001f5d6bcce825186688c388eb4"
    expect(Digest::SHA256.hexdigest(code_definition)).to eq(expected_checksum)
  end

@@ -118,6 +118,27 @@
      it 'does not create a state transition entry' do
        expect { subject }.not_to change(Vulnerabilities::StateTransition, :count)
      end
+
+      context 'when vulnerability state is dismissed' do
+        let!(:state_transition) do
+          create(:vulnerability_state_transitions,
+            :from_detected,
+            :to_dismissed,
+            vulnerability: vulnerability,
+            comment: nil)
+        end
+
+        let(:comment) { "Dismissal comment" }
+
+        before do
+          params.merge!({ comment: comment })
+        end
+
+        it 'updates the existing state transition with comment' do
+          state_transition = Vulnerabilities::StateTransition.last
+          expect { subject }.to change { state_transition.reload.comment }.from(nil).to(comment)
+        end
+      end
    end
  end