From 3a3e04be0d988de3d0742eae81cb34de25be3b9c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micha=C5=82=20Zaj=C4=85c?= <mzajac@gitlab.com>
Date: Tue, 11 Jan 2022 00:41:03 +0000
Subject: [PATCH] Add vulnerabilityFindingDismiss GraphQL mutation
* Add Vulnerabilities::FindingDismissService wrapping
VulnerabilitiesFeedback::CreateService
* Add FindingPolicy
Changelog: added
EE: true
---
doc/api/graphql/reference/index.md | 27 +++++
ee/app/graphql/ee/types/mutation_type.rb | 1 +
.../vulnerabilities/finding_dismiss.rb | 56 ++++++++++
.../vulnerabilities/finding_policy.rb | 8 ++
.../finding_dismiss_service.rb | 56 ++++++++++
.../vulnerabilities/finding_dismiss_spec.rb | 51 +++++++++
.../vulnerabilities/finding_dismiss_spec.rb | 102 ++++++++++++++++++
.../finding_dismiss_service_spec.rb | 96 +++++++++++++++++
locale/gitlab.pot | 3 +
9 files changed, 400 insertions(+)
create mode 100644 ee/app/graphql/mutations/vulnerabilities/finding_dismiss.rb
create mode 100644 ee/app/policies/vulnerabilities/finding_policy.rb
create mode 100644 ee/app/services/vulnerabilities/finding_dismiss_service.rb
create mode 100644 ee/spec/graphql/mutations/vulnerabilities/finding_dismiss_spec.rb
create mode 100644 ee/spec/requests/api/graphql/mutations/vulnerabilities/finding_dismiss_spec.rb
create mode 100644 ee/spec/services/vulnerabilities/finding_dismiss_service_spec.rb
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index 0f5bdc9f224ad..7315d3f2701dd 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -4931,6 +4931,27 @@ Input type: `VulnerabilityExternalIssueLinkDestroyInput`
| <a id="mutationvulnerabilityexternalissuelinkdestroyclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
| <a id="mutationvulnerabilityexternalissuelinkdestroyerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
+### `Mutation.vulnerabilityFindingDismiss`
+
+Input type: `VulnerabilityFindingDismissInput`
+
+#### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationvulnerabilityfindingdismissclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationvulnerabilityfindingdismisscomment"></a>`comment` | [`String`](#string) | Comment why finding should be dismissed. |
+| <a id="mutationvulnerabilityfindingdismissdismissalreason"></a>`dismissalReason` | [`VulnerabilityDismissalReason`](#vulnerabilitydismissalreason) | Reason why finding should be dismissed. |
+| <a id="mutationvulnerabilityfindingdismissid"></a>`id` | [`VulnerabilitiesFindingID!`](#vulnerabilitiesfindingid) | ID of the finding to be dismissed. |
+
+#### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationvulnerabilityfindingdismissclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationvulnerabilityfindingdismisserrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
+| <a id="mutationvulnerabilityfindingdismissfinding"></a>`finding` | [`PipelineSecurityReportFinding`](#pipelinesecurityreportfinding) | Finding after dismissal. |
+
### `Mutation.vulnerabilityResolve`
Input type: `VulnerabilityResolveInput`
@@ -18181,6 +18202,12 @@ A `VulnerabilitiesExternalIssueLinkID` is a global ID. It is encoded as a string
An example `VulnerabilitiesExternalIssueLinkID` is: `"gid://gitlab/Vulnerabilities::ExternalIssueLink/1"`.
+### `VulnerabilitiesFindingID`
+
+A `VulnerabilitiesFindingID` is a global ID. It is encoded as a string.
+
+An example `VulnerabilitiesFindingID` is: `"gid://gitlab/Vulnerabilities::Finding/1"`.
+
### `VulnerabilitiesScannerID`
A `VulnerabilitiesScannerID` is a global ID. It is encoded as a string.
diff --git a/ee/app/graphql/ee/types/mutation_type.rb b/ee/app/graphql/ee/types/mutation_type.rb
index de99f6289cde9..0984a1bfcee4f 100644
--- a/ee/app/graphql/ee/types/mutation_type.rb
+++ b/ee/app/graphql/ee/types/mutation_type.rb
@@ -31,6 +31,7 @@ module MutationType
mount_mutation ::Mutations::RequirementsManagement::UpdateRequirement
mount_mutation ::Mutations::Vulnerabilities::Create
mount_mutation ::Mutations::Vulnerabilities::Dismiss
+ mount_mutation ::Mutations::Vulnerabilities::FindingDismiss
mount_mutation ::Mutations::Vulnerabilities::Resolve
mount_mutation ::Mutations::Vulnerabilities::Confirm
mount_mutation ::Mutations::Vulnerabilities::RevertToDetected
diff --git a/ee/app/graphql/mutations/vulnerabilities/finding_dismiss.rb b/ee/app/graphql/mutations/vulnerabilities/finding_dismiss.rb
new file mode 100644
index 0000000000000..f644617a44124
--- /dev/null
+++ b/ee/app/graphql/mutations/vulnerabilities/finding_dismiss.rb
@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+module Mutations
+ module Vulnerabilities
+ class FindingDismiss < BaseMutation
+ graphql_name 'VulnerabilityFindingDismiss'
+
+ authorize :admin_vulnerability
+
+ field :finding, Types::PipelineSecurityReportFindingType,
+ null: true,
+ description: 'Finding after dismissal.'
+
+ argument :id,
+ ::Types::GlobalIDType[::Vulnerabilities::Finding],
+ required: true,
+ description: 'ID of the finding to be dismissed.'
+
+ argument :comment,
+ GraphQL::Types::String,
+ required: false,
+ description: 'Comment why finding should be dismissed.'
+
+ argument :dismissal_reason,
+ Types::Vulnerabilities::DismissalReasonEnum,
+ required: false,
+ description: 'Reason why finding should be dismissed.'
+
+ def resolve(id:, comment: nil, dismissal_reason: nil)
+ finding = authorized_find!(id: id)
+ result = dismiss_finding(finding, comment, dismissal_reason)
+
+ {
+ finding: result.success? ? result.payload[:finding] : nil,
+ errors: result.message || []
+ }
+ end
+
+ private
+
+ def dismiss_finding(finding, comment, dismissal_reason)
+ ::Vulnerabilities::FindingDismissService.new(
+ current_user,
+ finding,
+ comment,
+ dismissal_reason
+ ).execute
+ end
+
+ def find_object(id:)
+ id = ::Types::GlobalIDType[::Vulnerabilities::Finding].coerce_isolated_input(id)
+ GitlabSchema.find_by_gid(id)
+ end
+ end
+ end
+end
diff --git a/ee/app/policies/vulnerabilities/finding_policy.rb b/ee/app/policies/vulnerabilities/finding_policy.rb
new file mode 100644
index 0000000000000..08d857397d109
--- /dev/null
+++ b/ee/app/policies/vulnerabilities/finding_policy.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+module Vulnerabilities
+ class FindingPolicy < BasePolicy
+ delegate { @subject.project }
+
+ rule { ~can?(:read_security_resource) }.prevent :create_note
+ end
+end
diff --git a/ee/app/services/vulnerabilities/finding_dismiss_service.rb b/ee/app/services/vulnerabilities/finding_dismiss_service.rb
new file mode 100644
index 0000000000000..d30679f9d66a4
--- /dev/null
+++ b/ee/app/services/vulnerabilities/finding_dismiss_service.rb
@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+module Vulnerabilities
+ class FindingDismissService < BaseProjectService
+ include Gitlab::Allowable
+
+ def initialize(user, finding, comment = nil, dismissal_reason = nil)
+ super(project: finding.project, current_user: user)
+ @finding = finding
+ @comment = comment
+ @dismissal_reason = dismissal_reason
+ end
+
+ def execute
+ return ServiceResponse.error(message: "Access denied", http_status: :forbidden) unless authorized?
+
+ dismiss_finding
+ end
+
+ private
+
+ def authorized?
+ can?(@current_user, :admin_vulnerability, @project)
+ end
+
+ def dismiss_finding
+ result = ::VulnerabilityFeedback::CreateService.new(
+ @project,
+ @current_user,
+ feedback_params_for(@finding, @comment, @dismissal_reason)
+ ).execute
+
+ case result[:status]
+ when :success
+ ServiceResponse.success(payload: { finding: @finding })
+ when :error
+ all_errors = result[:message].full_messages.join(",")
+ error_string = _("failed to dismiss finding: %{message}") % { message: all_errors }
+ ServiceResponse.error(message: error_string, http_status: :unprocessable_entity)
+ end
+ end
+
+ def feedback_params_for(finding, comment, dismissal_reason)
+ {
+ category: @finding.report_type,
+ feedback_type: 'dismissal',
+ project_fingerprint: @finding.project_fingerprint,
+ comment: @comment,
+ dismissal_reason: @dismissal_reason,
+ pipeline: @project.latest_pipeline_with_security_reports(only_successful: true),
+ finding_uuid: @finding.uuid_v5,
+ dismiss_vulnerability: false
+ }
+ end
+ end
+end
diff --git a/ee/spec/graphql/mutations/vulnerabilities/finding_dismiss_spec.rb b/ee/spec/graphql/mutations/vulnerabilities/finding_dismiss_spec.rb
new file mode 100644
index 0000000000000..1a5f260f2660c
--- /dev/null
+++ b/ee/spec/graphql/mutations/vulnerabilities/finding_dismiss_spec.rb
@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+RSpec.describe Mutations::Vulnerabilities::FindingDismiss do
+ include GraphqlHelpers
+
+ let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+
+ describe '#resolve' do
+ let_it_be(:finding) { create(:vulnerabilities_finding) }
+ let_it_be(:user) { create(:user) }
+ let_it_be(:finding_id) { GitlabSchema.id_from_object(finding).to_s }
+
+ let(:comment) { 'Dismissal Feedback' }
+ let(:mutated_finding) { subject[:finding] }
+
+ subject { mutation.resolve(id: finding_id, comment: comment, dismissal_reason: 'used_in_tests') }
+
+ context 'when the user can dismiss the finding' do
+ before do
+ stub_licensed_features(security_dashboard: true)
+ end
+
+ context 'when user does not have access to the project' do
+ it 'raises an error' do
+ expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+ end
+ end
+
+ context 'with invalid params' do
+ let(:finding_id) { global_id_of(user) }
+
+ it 'raises an error' do
+ expect { subject }.to raise_error(::GraphQL::CoercionError)
+ end
+ end
+
+ context 'when user has access to the project' do
+ before do
+ finding.project.add_developer(user)
+ end
+
+ it 'returns the dismissed finding' do
+ expect(mutated_finding).to eq(finding)
+ expect(mutated_finding.state).to eq('dismissed')
+ expect(subject[:errors]).to be_empty
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/requests/api/graphql/mutations/vulnerabilities/finding_dismiss_spec.rb b/ee/spec/requests/api/graphql/mutations/vulnerabilities/finding_dismiss_spec.rb
new file mode 100644
index 0000000000000..b2246f7f93e3d
--- /dev/null
+++ b/ee/spec/requests/api/graphql/mutations/vulnerabilities/finding_dismiss_spec.rb
@@ -0,0 +1,102 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+RSpec.describe 'Dismissing a Vulnerabilities::Finding object' do
+ include GraphqlHelpers
+
+ let_it_be(:current_user) { create(:user) }
+ let_it_be(:project) { create(:project) }
+ let_it_be(:pipeline) { create(:ci_pipeline, :success, project: project) }
+ let_it_be(:finding) { create(:vulnerabilities_finding, :with_pipeline, project: project, severity: :high) }
+
+ let(:params) do
+ {
+ id: finding.to_global_id.to_s
+ }
+ end
+
+ let(:mutation) do
+ graphql_mutation(
+ :vulnerability_finding_dismiss,
+ params
+ )
+ end
+
+ let(:mutation_response) do
+ graphql_mutation_response(:vulnerability_finding_dismiss)
+ end
+
+ subject { post_graphql_mutation(mutation, current_user: current_user) }
+
+ context 'when the user does not have permission' do
+ before do
+ stub_licensed_features(security_dashboard: true)
+ end
+
+ it_behaves_like 'a mutation that returns a top-level access error'
+
+ it 'does not dismiss the Finding' do
+ expect { subject }.not_to change(finding, :state)
+ end
+ end
+
+ context 'when the user has permission' do
+ before do
+ finding.project.add_developer(current_user)
+ end
+
+ context 'when security_dashboard is disabled' do
+ before do
+ stub_licensed_features(security_dashboard: false)
+ end
+
+ it_behaves_like 'a mutation that returns top-level errors',
+ errors: ['The resource that you are attempting to access does not '\
+ 'exist or you don\'t have permission to perform this action']
+ end
+
+ context 'when security_dashboard is enabled' do
+ before do
+ stub_licensed_features(security_dashboard: true)
+ end
+
+ it 'dismisses the Finding' do
+ expect { subject }.to change(finding, :state).from('detected').to('dismissed')
+ end
+
+ context 'when comment is given' do
+ let(:comment) { "Used in tests" }
+ let(:params) do
+ {
+ id: finding.to_global_id.to_s,
+ comment: comment
+ }
+ end
+
+ let(:feedback) { finding.dismissal_feedback }
+
+ it 'saves the comment' do
+ expect { subject }.to change(finding, :state).from('detected').to('dismissed')
+ expect(feedback.comment).to eq(comment)
+ end
+ end
+
+ context 'when dismissal reason is given' do
+ let(:dismissal_reason) { "USED_IN_TESTS" }
+ let(:params) do
+ {
+ id: finding.to_global_id.to_s,
+ dismissal_reason: dismissal_reason
+ }
+ end
+
+ let(:feedback) { finding.dismissal_feedback }
+
+ it 'saves the dismissal reason' do
+ expect { subject }.to change(finding, :state).from('detected').to('dismissed')
+ expect(feedback.dismissal_reason).to eq('used_in_tests')
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/services/vulnerabilities/finding_dismiss_service_spec.rb b/ee/spec/services/vulnerabilities/finding_dismiss_service_spec.rb
new file mode 100644
index 0000000000000..98ef7d9507e5f
--- /dev/null
+++ b/ee/spec/services/vulnerabilities/finding_dismiss_service_spec.rb
@@ -0,0 +1,96 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::FindingDismissService 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!(:pipeline) { create(:ci_pipeline, :success, project: project) }
+ let!(:build) { create(:ee_ci_build, :sast, pipeline: pipeline) }
+ let!(:finding) { create(:vulnerabilities_finding, project: project) }
+ let(:service) { described_class.new(user, finding) }
+
+ subject(:dismiss_finding) { service.execute }
+
+ context 'with an authorized user with proper permissions' do
+ before do
+ project.add_developer(user)
+ end
+
+ context 'when comment is added' do
+ let(:comment) { 'Dismissal Comment' }
+ let(:service) { described_class.new(user, finding, comment) }
+
+ it 'dismisses a finding with comment', :aggregate_failures do
+ freeze_time do
+ dismiss_finding
+
+ aggregate_failures do
+ expect(finding.reload).to(have_attributes(state: 'dismissed'))
+ expect(finding.dismissal_feedback).to have_attributes(comment: comment, comment_author: user, comment_timestamp: be_like_time(Time.current), pipeline_id: pipeline.id)
+ end
+ end
+ end
+ end
+
+ context 'when the dismissal_reason is added' do
+ let(:dismissal_reason) { 'used_in_tests' }
+ let(:service) { described_class.new(user, finding, nil, dismissal_reason) }
+
+ it 'dismisses a finding', :aggregate_failures do
+ dismiss_finding
+
+ expect(finding.reload).to have_attributes(state: 'dismissed')
+ expect(finding.dismissal_feedback).to have_attributes(dismissal_reason: dismissal_reason)
+ end
+ end
+
+ context 'when Vulnerabilities::Feedback creation fails' do
+ let(:create_service_double) { instance_double("VulnerabilityFeedback::CreateService", execute: service_failure_payload) }
+ let(:service_failure_payload) do
+ {
+ status: :error,
+ message: errors_double
+ }
+ end
+
+ let(:errors_double) { instance_double("ActiveModel::Errors", full_messages: error_messages_array) }
+ let(:error_messages_array) { instance_double("Array", join: "something went wrong") }
+
+ before do
+ allow(VulnerabilityFeedback::CreateService).to receive(:new).and_return(create_service_double)
+ end
+
+ it 'returns the error' do
+ expect(create_service_double).to receive(:execute).once
+
+ result = dismiss_finding
+
+ expect(result).not_to be_success
+ expect(result.http_status).to be(:unprocessable_entity)
+ expect(result.message).to eq("failed to dismiss finding: 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
+ result = dismiss_finding
+
+ expect(result).not_to be_success
+ expect(result.http_status).to be(:forbidden)
+ expect(result.message).to eq("Access denied")
+ end
+ end
+ end
+end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 98806ac7b0f3e..109653d4b7a28 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -42040,6 +42040,9 @@ msgstr ""
msgid "failed to dismiss associated finding(id=%{finding_id}): %{message}"
msgstr ""
+msgid "failed to dismiss finding: %{message}"
+msgstr ""
+
msgid "failed to revert associated finding(id=%{finding_id}) to detected"
msgstr ""
--
GitLab