diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index 3e3336681480d4ae1ab686526d166772cac00556..0f6191de507b6b327cd087f21c53036015b257bf 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 088ecc04ebdc1bc5c482650a8331caaa4cc652ca..12957adc731d5ebe868a784633844edd3de5c6ad 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/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 25bf2699010fb0d88638b4601501677ca4e6f794..adabc6e6438f4999ec1a5634fd71a4ba1b6d7d61 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/app/services/vulnerabilities/dismiss_service.rb b/ee/app/services/vulnerabilities/dismiss_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..ba1d41307be0717f8182b3debbd200751da2d83f --- /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/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb index 63e58a366ba8266ce0193f8b55e279c275f7e4d4..1b6a1f91666b1a498cb48a283de02230e9b3636d 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 |vulnerability| + authorize! action, vulnerability.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,26 @@ 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) + vulnerability = ::Vulnerabilities::DismissService.new(current_user, vulnerability).execute + render_vulnerability(vulnerability) + 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/factories/vulnerabilities.rb b/ee/spec/factories/vulnerabilities.rb index 7c1bb96825ac61863fdbd96d94315fcb9bf8872e..407d3b0efb9c0f390cb835bbc94deed4fa17d2e1 100644 --- a/ee/spec/factories/vulnerabilities.rb +++ b/ee/spec/factories/vulnerabilities.rb @@ -17,5 +17,15 @@ state { :closed } closed_at { Time.now } end + + trait :with_findings do + after(:build) do |vulnerability| + vulnerability.findings = build_list( + :vulnerabilities_occurrence, + 2, + vulnerability: vulnerability, + project: vulnerability.project) + end + end end end diff --git a/ee/spec/models/vulnerabilities/occurrence_spec.rb b/ee/spec/models/vulnerabilities/occurrence_spec.rb index f4e0b6867c71859053adfb3faeb37c76248d36ea..479dc5a109aa147a2367d7c4f2127924df11ae9a 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 3e6d7f3af855438f703c63306c268267c74a4407..7b9ebd708930bc49d916b70a2df4c17834006d5f 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') } diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index 548af049e003a2c695d7f969851f859047e60c56..9e2aebb03920962eba4bff17772ef32786d7825e 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 diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index c93767ba603fcbf8111e95a7cd65f1d354ba2937..708497fde2de9d42fee0650399cfd911f38e6217 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -7,7 +7,7 @@ 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 @@ -43,6 +43,98 @@ 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 + + describe "POST /vulnerabilities:id/dismiss" do + before do + create_list(:vulnerabilities_occurrence, 2, vulnerability: vulnerability, project: vulnerability.project) + 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 + expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback + end + + context 'when there is a dismissal error' do + before do + 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' => ['something went wrong']) + 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 diff --git a/ee/spec/requests/api/vulnerability_findings_spec.rb b/ee/spec/requests/api/vulnerability_findings_spec.rb index 0c0141bdc29ec8f84aabac54e790235ebdf99cd7..c4d138519e361213849d094a60ff910698b3fb10 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/services/vulnerabilities/dismiss_service_spec.rb b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..e70fc493050d902c42eaec0d5fa5e7333de7162a --- /dev/null +++ b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Vulnerabilities::DismissService 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, :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 + expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback + 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 0000000000000000000000000000000000000000..b7fb2502bba1071da5f1d2d923c9aaeb02609310 --- /dev/null +++ b/ee/spec/support/helpers/vulnerability_helpers.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +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 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 f71cc233c9c3684495a4208ec1efaa2509927860..c85fb1364d28d670a060e0eb64dccc6b01d02208 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) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 237b2b7cb0d82bbe91429103941a767b2fa1e30b..15f635ee40e331f4f2182f5dacf5c8cb6d5635b9 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 ""