From 09f1f98367b4cda692d017aa6c4d7ea003f14985 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Sat, 12 Oct 2019 15:03:54 +0300 Subject: [PATCH 01/11] Rename shared examples for accurate description --- ee/spec/requests/api/vulnerabilities_spec.rb | 2 +- ee/spec/requests/api/vulnerability_findings_spec.rb | 2 +- .../requests/api/vulnerabilities_shared_examples.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index c93767ba603fcb..18f11e3049827c 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -43,6 +43,6 @@ 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 end diff --git a/ee/spec/requests/api/vulnerability_findings_spec.rb b/ee/spec/requests/api/vulnerability_findings_spec.rb index 0c0141bdc29ec8..c4d138519e3612 100644 --- a/ee/spec/requests/api/vulnerability_findings_spec.rb +++ b/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 diff --git a/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb index f71cc233c9c368..c85fb1364d28d6 100644 --- a/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb +++ b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb @@ -1,6 +1,6 @@ # 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) -- GitLab From bdb0db8fcd5e7ca997c8f8c2a7166c2997981d7b Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Sat, 12 Oct 2019 15:36:17 +0300 Subject: [PATCH 02/11] Update Occurrence and Vulnerability models --- ee/app/models/vulnerabilities/occurrence.rb | 1 + ee/app/models/vulnerability.rb | 5 +++++ ee/spec/models/vulnerabilities/occurrence_spec.rb | 1 + ee/spec/models/vulnerability_spec.rb | 2 +- 4 files changed, 8 insertions(+), 1 deletion(-) diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index 3e3336681480d4..0f6191de507b6b 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/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' diff --git a/ee/app/models/vulnerability.rb b/ee/app/models/vulnerability.rb index 088ecc04ebdc1b..12957adc731d5e 100644 --- a/ee/app/models/vulnerability.rb +++ b/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 diff --git a/ee/spec/models/vulnerabilities/occurrence_spec.rb b/ee/spec/models/vulnerabilities/occurrence_spec.rb index f4e0b6867c7185..479dc5a109aa14 100644 --- a/ee/spec/models/vulnerabilities/occurrence_spec.rb +++ b/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') } diff --git a/ee/spec/models/vulnerability_spec.rb b/ee/spec/models/vulnerability_spec.rb index 3e6d7f3af85543..7b9ebd708930bc 100644 --- a/ee/spec/models/vulnerability_spec.rb +++ b/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') } -- GitLab From 8a19e4e24d996f7acf257ad54abb0dfa737bf2c7 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Sat, 12 Oct 2019 15:38:15 +0300 Subject: [PATCH 03/11] Add dismiss_vulnerability privilege for Project --- ee/app/policies/ee/project_policy.rb | 3 +++ ee/spec/policies/project_policy_spec.rb | 27 +++++++++++++++++++------ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 25bf2699010fb0..adabc6e6438f49 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/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 diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 548af049e003a2..9e2aebb0392096 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/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 -- GitLab From 6542773fc96a6b7163428aad73b26cf50b32bcc1 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Sat, 12 Oct 2019 19:04:08 +0300 Subject: [PATCH 04/11] Add Vulnerabilities::DismissService class w/ tests Vulnerabilities::DismissSerivce is responsible for dismissal of Vulnerabilities and their associated Findings (creation of dismissal feedback records for all Findings as a cascade. --- .../vulnerabilities/dismiss_service.rb | 67 ++++++++++++++++++ ee/spec/factories/vulnerabilities.rb | 6 ++ .../vulnerabilities/dismiss_service_spec.rb | 68 +++++++++++++++++++ .../support/helpers/vulnerability_helpers.rb | 7 ++ 4 files changed, 148 insertions(+) create mode 100644 ee/app/services/vulnerabilities/dismiss_service.rb create mode 100644 ee/spec/services/vulnerabilities/dismiss_service_spec.rb create mode 100644 ee/spec/support/helpers/vulnerability_helpers.rb diff --git a/ee/app/services/vulnerabilities/dismiss_service.rb b/ee/app/services/vulnerabilities/dismiss_service.rb new file mode 100644 index 00000000000000..95606258311bc0 --- /dev/null +++ b/ee/app/services/vulnerabilities/dismiss_service.rb @@ -0,0 +1,67 @@ +# 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 diff --git a/ee/spec/factories/vulnerabilities.rb b/ee/spec/factories/vulnerabilities.rb index 7c1bb96825ac61..2277a77cd797b3 100644 --- a/ee/spec/factories/vulnerabilities.rb +++ b/ee/spec/factories/vulnerabilities.rb @@ -17,5 +17,11 @@ state { :closed } closed_at { Time.now } end + + trait :with_findings do + after(:build) do |vulnerability| + vulnerability.findings = build_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability) + end + end end end diff --git a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb new file mode 100644 index 00000000000000..29b52a60ac9222 --- /dev/null +++ b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Vulnerabilities::DismissService do + include VulnerabilityHelpers + + 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 + vulnerability.findings.each do |finding| + expect(dismissal_feedback_for(finding)).to have_attributes(category: finding.report_type, + project_fingerprint: finding.project_fingerprint) + end + 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 diff --git a/ee/spec/support/helpers/vulnerability_helpers.rb b/ee/spec/support/helpers/vulnerability_helpers.rb new file mode 100644 index 00000000000000..64944f10c8d208 --- /dev/null +++ b/ee/spec/support/helpers/vulnerability_helpers.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +module VulnerabilityHelpers + def dismissal_feedback_for(finding) + Vulnerabilities::Feedback.find_by!(feedback_type: 'dismissal', project_fingerprint: finding.project_fingerprint) + end +end -- GitLab From d2c52f874444c010cd73afbba6b84c19febeaeb4 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Sat, 12 Oct 2019 19:05:26 +0300 Subject: [PATCH 05/11] Add Dismiss a Vulnerability API call --- ee/lib/api/vulnerabilities.rb | 38 ++++++++- ee/spec/requests/api/vulnerabilities_spec.rb | 89 +++++++++++++++++++- 2 files changed, 125 insertions(+), 2 deletions(-) diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb index 63e58a366ba826..dd49b245f3e469 100644 --- a/ee/lib/api/vulnerabilities.rb +++ b/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 |v| + authorize! action, v.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,27 @@ 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) + render_vulnerability( + ::Vulnerabilities::DismissService.new(current_user, vulnerability).execute + ) + 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 diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index 18f11e3049827c..a596ee49aa22fb 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -3,11 +3,13 @@ require 'spec_helper' describe API::Vulnerabilities do + include VulnerabilityHelpers + before do 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 @@ -45,4 +47,89 @@ 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) + 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 + vulnerability.findings.each do |finding| + expect(dismissal_feedback_for(finding)).to have_attributes(category: finding.report_type, + project_fingerprint: finding.project_fingerprint) + end + end + + context 'when there is a dismissal error' do + before do + allow(Vulnerabilities::DismissService).to receive(:new).and_wrap_original do |method, *args| + method.call(*args).tap do |service| + allow(service).to receive(:execute).and_return( + double('Vulnerability', valid?: false, errors: + double('Errors', any?: true, messages: { base: ['failed to dismiss findings'] })) + ) + end + end + end + + it 'responds with error' do + subject + + expect(response).to have_gitlab_http_status(400) + expect(json_response['message']).to eq('base' => ['failed to dismiss findings']) + 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 -- GitLab From 678ec3234992fbba08b2b34e144523d512f8abd9 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Sat, 12 Oct 2019 21:27:49 +0300 Subject: [PATCH 06/11] Fix Rubocop offense --- ee/app/services/vulnerabilities/dismiss_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/services/vulnerabilities/dismiss_service.rb b/ee/app/services/vulnerabilities/dismiss_service.rb index 95606258311bc0..afe1e62f1039fa 100644 --- a/ee/app/services/vulnerabilities/dismiss_service.rb +++ b/ee/app/services/vulnerabilities/dismiss_service.rb @@ -57,7 +57,7 @@ def handle_finding_dismissal_error(finding, message) @vulnerability.errors.add( :base, :finding_dismissal_error, - message: ("failed to dismiss associated finding(id=%{finding_id}): %{message}") % + message: "failed to dismiss associated finding(id=%{finding_id}): %{message}" % { finding_id: finding.id, message: message -- GitLab From 0f5c9e492200a9ea7deb4cdc48881c3db83b8c27 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Sat, 12 Oct 2019 21:36:24 +0300 Subject: [PATCH 07/11] Localize the findings dismissal error message --- ee/app/services/vulnerabilities/dismiss_service.rb | 2 +- locale/gitlab.pot | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/ee/app/services/vulnerabilities/dismiss_service.rb b/ee/app/services/vulnerabilities/dismiss_service.rb index afe1e62f1039fa..ba1d41307be071 100644 --- a/ee/app/services/vulnerabilities/dismiss_service.rb +++ b/ee/app/services/vulnerabilities/dismiss_service.rb @@ -57,7 +57,7 @@ def handle_finding_dismissal_error(finding, message) @vulnerability.errors.add( :base, :finding_dismissal_error, - message: "failed to dismiss associated finding(id=%{finding_id}): %{message}" % + message: _("failed to dismiss associated finding(id=%{finding_id}): %{message}") % { finding_id: finding.id, message: message diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 237b2b7cb0d82b..15f635ee40e331 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -19610,6 +19610,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 "" -- GitLab From 50d2114384a18dfdcc31f725602c6be5f8257be7 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 15 Oct 2019 14:17:42 +0000 Subject: [PATCH 08/11] Apply suggestion to ee/lib/api/vulnerabilities.rb --- ee/lib/api/vulnerabilities.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb index dd49b245f3e469..48abf978452503 100644 --- a/ee/lib/api/vulnerabilities.rb +++ b/ee/lib/api/vulnerabilities.rb @@ -16,7 +16,7 @@ def find_vulnerability! end def find_and_authorize_vulnerability!(action) - find_vulnerability!.tap do |v| + find_vulnerability!.tap do |vulnerability| authorize! action, v.project end end -- GitLab From 35f25935470158253cd9f2b56af24c1a702f36fb Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 15 Oct 2019 18:30:06 +0300 Subject: [PATCH 09/11] Make var name more readable --- ee/lib/api/vulnerabilities.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb index 48abf978452503..1b6a1f91666b1a 100644 --- a/ee/lib/api/vulnerabilities.rb +++ b/ee/lib/api/vulnerabilities.rb @@ -17,7 +17,7 @@ def find_vulnerability! def find_and_authorize_vulnerability!(action) find_vulnerability!.tap do |vulnerability| - authorize! action, v.project + authorize! action, vulnerability.project end end @@ -44,9 +44,8 @@ def render_vulnerability(vulnerability) post ':id/dismiss' do if Feature.enabled?(:first_class_vulnerabilities) vulnerability = find_and_authorize_vulnerability!(:dismiss_vulnerability) - render_vulnerability( - ::Vulnerabilities::DismissService.new(current_user, vulnerability).execute - ) + vulnerability = ::Vulnerabilities::DismissService.new(current_user, vulnerability).execute + render_vulnerability(vulnerability) else not_found! end -- GitLab From 97af32e78e7bbd24eb156afd2052f4f34c6be9b7 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 15 Oct 2019 18:30:15 +0300 Subject: [PATCH 10/11] Fix factories and change helper for custom matcher Fix the vulnerability_occurence factory records to be created with the same project that parent vulnerability belongs to. Add a custom matcher to validate the expectation of all Vulnerability's findings to have a correct dismissal Feedback associated with each of them. --- ee/spec/factories/vulnerabilities.rb | 6 +++++- ee/spec/requests/api/vulnerabilities_spec.rb | 9 ++------- ee/spec/services/vulnerabilities/dismiss_service_spec.rb | 7 +------ ee/spec/support/helpers/vulnerability_helpers.rb | 8 +++++--- 4 files changed, 13 insertions(+), 17 deletions(-) diff --git a/ee/spec/factories/vulnerabilities.rb b/ee/spec/factories/vulnerabilities.rb index 2277a77cd797b3..407d3b0efb9c0f 100644 --- a/ee/spec/factories/vulnerabilities.rb +++ b/ee/spec/factories/vulnerabilities.rb @@ -20,7 +20,11 @@ trait :with_findings do after(:build) do |vulnerability| - vulnerability.findings = build_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability) + vulnerability.findings = build_list( + :vulnerabilities_occurrence, + 2, + vulnerability: vulnerability, + project: vulnerability.project) end end end diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index a596ee49aa22fb..e4303ab025c1e5 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' describe API::Vulnerabilities do - include VulnerabilityHelpers - before do stub_licensed_features(security_dashboard: true) end @@ -50,7 +48,7 @@ describe "POST /vulnerabilities:id/dismiss" do before do - create_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability) + create_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability, project: vulnerability.project) end let(:vulnerability) { project.vulnerabilities.first } @@ -69,10 +67,7 @@ expect(response).to match_response_schema('vulnerability', dir: 'ee') expect(vulnerability.reload).to be_closed - vulnerability.findings.each do |finding| - expect(dismissal_feedback_for(finding)).to have_attributes(category: finding.report_type, - project_fingerprint: finding.project_fingerprint) - end + expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback end context 'when there is a dismissal error' do diff --git a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb index 29b52a60ac9222..e70fc493050d90 100644 --- a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb +++ b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' describe Vulnerabilities::DismissService do - include VulnerabilityHelpers - before do stub_licensed_features(security_dashboard: true) end @@ -25,10 +23,7 @@ subject expect(vulnerability.reload).to be_closed - vulnerability.findings.each do |finding| - expect(dismissal_feedback_for(finding)).to have_attributes(category: finding.report_type, - project_fingerprint: finding.project_fingerprint) - end + expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback end context 'when there is a finding dismissal error' do diff --git a/ee/spec/support/helpers/vulnerability_helpers.rb b/ee/spec/support/helpers/vulnerability_helpers.rb index 64944f10c8d208..b7fb2502bba107 100644 --- a/ee/spec/support/helpers/vulnerability_helpers.rb +++ b/ee/spec/support/helpers/vulnerability_helpers.rb @@ -1,7 +1,9 @@ # frozen_string_literal: true -module VulnerabilityHelpers - def dismissal_feedback_for(finding) - Vulnerabilities::Feedback.find_by!(feedback_type: 'dismissal', project_fingerprint: finding.project_fingerprint) +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 -- GitLab From 7922576a3f3c94b7bb3071e581f1f783fae0913c Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 16 Oct 2019 13:50:38 +0300 Subject: [PATCH 11/11] Hide internal details from dismissal error test --- ee/spec/requests/api/vulnerabilities_spec.rb | 24 ++++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index e4303ab025c1e5..708497fde2de9d 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -72,21 +72,31 @@ context 'when there is a dismissal error' do before do - allow(Vulnerabilities::DismissService).to receive(:new).and_wrap_original do |method, *args| - method.call(*args).tap do |service| - allow(service).to receive(:execute).and_return( - double('Vulnerability', valid?: false, errors: - double('Errors', any?: true, messages: { base: ['failed to dismiss findings'] })) - ) + 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' => ['failed to dismiss findings']) + expect(json_response['message']).to eq('base' => ['something went wrong']) end end -- GitLab