diff --git a/ee/app/finders/security/pipeline_vulnerabilities_finder.rb b/ee/app/finders/security/pipeline_vulnerabilities_finder.rb index 5f111f6681320a34679666b8f6b4fad4e4a91d8a..4818d21737e39e5e65e6968fd037068057e85b1e 100644 --- a/ee/app/finders/security/pipeline_vulnerabilities_finder.rb +++ b/ee/app/finders/security/pipeline_vulnerabilities_finder.rb @@ -33,7 +33,9 @@ def execute raise ParseError, 'JSON parsing failed' if report.error.is_a?(Gitlab::Ci::Parsers::Security::Common::SecurityReportParserError) - normalized_occurrences = normalize_report_occurrences(report.occurrences) + normalized_occurrences = normalize_report_occurrences( + report.occurrences, + vulnerabilities_by_finding_fingerprint(type, report)) filtered_occurrences = filter(normalized_occurrences) occurrences.concat(filtered_occurrences) @@ -48,12 +50,25 @@ def pipeline_reports pipeline&.security_reports&.reports end - def normalize_report_occurrences(report_occurrences) + def vulnerabilities_by_finding_fingerprint(report_type, report) + Vulnerabilities::Occurrence + .with_vulnerabilities_for_state( + project: pipeline.project, + report_type: report_type, + project_fingerprints: report.occurrences.map(&:project_fingerprint)) + .each_with_object({}) do |occurrence, hash| + hash[occurrence.project_fingerprint] = occurrence.vulnerability + end + end + + def normalize_report_occurrences(report_occurrences, vulnerabilities) report_occurrences.map do |report_occurrence| occurrence_hash = report_occurrence.to_hash .except(:compare_key, :identifiers, :location, :scanner) occurrence = Vulnerabilities::Occurrence.new(occurrence_hash) + # assigning Vulnerabilities to Findings to enable the computed state + occurrence.vulnerability = vulnerabilities[occurrence.project_fingerprint] occurrence.project = pipeline.project occurrence.build_scanner(report_occurrence.scanner.to_hash) diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index aef4a8a1b3844ae43d03179221a07431bde54c31..0b015562ba9baddd480c32c9081c2d4e2729fc8f 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/ee/app/models/vulnerabilities/occurrence.rb @@ -115,6 +115,30 @@ def self.counted_by_severity end end + def self.with_vulnerabilities_for_state(project:, report_type:, project_fingerprints:) + Vulnerabilities::Occurrence + .joins(:vulnerability) + .where( + project: project, + report_type: report_type, + project_fingerprint: project_fingerprints + ) + .select('report_type, vulnerability_id, project_fingerprint, raw_metadata, '\ + 'vulnerabilities.id, vulnerabilities.state') # fetching only required attributes + end + + def state + return 'dismissed' if dismissal_feedback.present? + + if vulnerability.nil? + 'new' + elsif vulnerability.closed? + 'resolved' + else + 'confirmed' + end + end + def feedback(feedback_type:) params = { project_id: project_id, diff --git a/ee/app/models/vulnerability.rb b/ee/app/models/vulnerability.rb index 12957adc731d5ebe868a784633844edd3de5c6ad..95f5d40f4bc847e5f6b6a320323fbbaeae28137a 100644 --- a/ee/app/models/vulnerability.rb +++ b/ee/app/models/vulnerability.rb @@ -10,7 +10,6 @@ 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 } diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb index 968d3abeaa45cdacdc452c55bd8f0f6c39d50cfe..f5568d334d317ff5dba8faadab90e7900935c15f 100644 --- a/ee/app/policies/ee/project_policy.rb +++ b/ee/app/policies/ee/project_policy.rb @@ -151,11 +151,13 @@ module ProjectPolicy rule { can?(:developer_access) }.policy do enable :read_project_security_dashboard + enable :resolve_vulnerability enable :dismiss_vulnerability end rule { security_dashboard_feature_disabled }.policy do prevent :read_project_security_dashboard + prevent :resolve_vulnerability prevent :dismiss_vulnerability end @@ -201,6 +203,7 @@ module ProjectPolicy enable :read_deployment enable :read_pages enable :read_project_security_dashboard + enable :resolve_vulnerability enable :dismiss_vulnerability end diff --git a/ee/app/serializers/vulnerabilities/occurrence_entity.rb b/ee/app/serializers/vulnerabilities/occurrence_entity.rb index bab04f95b51e980dea65e063e3c28dbd03223110..239dd24cfd1cb3fd3bcad66acd9cabab6d36b537 100644 --- a/ee/app/serializers/vulnerabilities/occurrence_entity.rb +++ b/ee/app/serializers/vulnerabilities/occurrence_entity.rb @@ -31,6 +31,8 @@ class Vulnerabilities::OccurrenceEntity < Grape::Entity expose :solution end + expose :state + expose :blob_path do |occurrence| occurrence.present.blob_path end diff --git a/ee/app/services/vulnerabilities/dismiss_service.rb b/ee/app/services/vulnerabilities/dismiss_service.rb index ba1d41307be0717f8182b3debbd200751da2d83f..5bf7524acc68968a402b9dd9123b48abb29e9612 100644 --- a/ee/app/services/vulnerabilities/dismiss_service.rb +++ b/ee/app/services/vulnerabilities/dismiss_service.rb @@ -6,14 +6,14 @@ class DismissService FindingsDismissResult = Struct.new(:ok?, :finding, :message) - def initialize(current_user, vulnerability) - @current_user = current_user + def initialize(user, vulnerability) + @user = user @vulnerability = vulnerability @project = vulnerability.project end def execute - raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :dismiss_vulnerability, @project) + raise Gitlab::Access::AccessDeniedError unless can?(@user, :dismiss_vulnerability, @project) @vulnerability.transaction do result = dismiss_findings @@ -23,7 +23,7 @@ def execute raise ActiveRecord::Rollback end - @vulnerability.update(state: 'closed') + @vulnerability.update(state: :closed, closed_by: @user, closed_at: Time.zone.now) end @vulnerability @@ -32,7 +32,7 @@ def execute private def feedback_service_for(finding) - VulnerabilityFeedback::CreateService.new(@project, @current_user, feedback_params_for(finding)) + VulnerabilityFeedback::CreateService.new(@project, @user, feedback_params_for(finding)) end def feedback_params_for(finding) diff --git a/ee/app/services/vulnerabilities/resolve_service.rb b/ee/app/services/vulnerabilities/resolve_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..16adea5f091938139185db757d0470dbd8272b24 --- /dev/null +++ b/ee/app/services/vulnerabilities/resolve_service.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Vulnerabilities + class ResolveService + include Gitlab::Allowable + + def initialize(user, vulnerability) + @user = user + @vulnerability = vulnerability + end + + def execute + raise Gitlab::Access::AccessDeniedError unless can?(@user, :resolve_vulnerability, @vulnerability.project) + + @vulnerability.tap do |vulnerability| + vulnerability.update(state: :closed, closed_by: @user, closed_at: Time.zone.now) + end + end + end +end diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb index 93a19ad1bfe4507cf819e578baeba240afd13675..ec381915c21f5bb4635d675b3bb882857032dcd3 100644 --- a/ee/lib/api/vulnerabilities.rb +++ b/ee/lib/api/vulnerabilities.rb @@ -38,6 +38,21 @@ def render_vulnerability(vulnerability) requires :id, type: String, desc: 'The ID of a vulnerability' end resource :vulnerabilities do + desc 'Resolve a vulnerability' do + success VulnerabilityEntity + end + post ':id/resolve' do + if Feature.enabled?(:first_class_vulnerabilities) + vulnerability = find_and_authorize_vulnerability!(:resolve_vulnerability) + break not_modified! if vulnerability.closed? + + vulnerability = ::Vulnerabilities::ResolveService.new(current_user, vulnerability).execute + render_vulnerability(vulnerability) + else + not_found! + end + end + desc 'Dismiss a vulnerability' do success VulnerabilityEntity end @@ -61,7 +76,7 @@ def render_vulnerability(vulnerability) params do # These params have no effect for Vulnerabilities API but are required to support falling back to # responding with Vulnerability Findings when :first_class_vulnerabilities feature is disabled. - # TODO: remove usage of :vulnerability_findings_params when feature flag is removed + # TODO: replace :vulnerability_findings_params with just :pagination when feature flag is removed # https://gitlab.com/gitlab-org/gitlab/issues/33488 use :vulnerability_findings_params end diff --git a/ee/spec/factories/vulnerabilities/occurrences.rb b/ee/spec/factories/vulnerabilities/occurrences.rb index 05e0f1cbcebf94c3e59cc3b00cc00f3a56e2d8db..c2143fa69fc697e4aec277cb66f87327739c20ae 100644 --- a/ee/spec/factories/vulnerabilities/occurrences.rb +++ b/ee/spec/factories/vulnerabilities/occurrences.rb @@ -5,7 +5,7 @@ Digest::SHA1.hexdigest("uuid-#{n}")[0..35] end - factory :vulnerabilities_occurrence, class: Vulnerabilities::Occurrence do + factory :vulnerabilities_occurrence, class: Vulnerabilities::Occurrence, aliases: [:vulnerabilities_finding] do name { 'Cipher with no integrity' } project sequence(:uuid) { generate(:vulnerability_occurrence_uuid) } @@ -36,5 +36,27 @@ ] }.to_json end + + trait :confirmed do + after(:create) do |finding| + create(:vulnerability, :opened, project: finding.project, findings: [finding]) + end + end + + trait :resolved do + after(:create) do |finding| + create(:vulnerability, :closed, project: finding.project, findings: [finding]) + end + end + + trait :dismissed do + after(:create) do |finding| + create(:vulnerability, :closed, project: finding.project, findings: [finding]) + create(:vulnerability_feedback, + :dismissal, + project: finding.project, + project_fingerprint: finding.project_fingerprint) + end + end end end diff --git a/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb b/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb index 52a146c2125593cc08554d502ae8ca3242d1f77f..cd19809e270c7894680dba1ffd934d10aa20ae82 100644 --- a/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb +++ b/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb @@ -218,6 +218,54 @@ def disable_deduplication end end + context 'when matching vulnerability records exist' do + before do + create(:vulnerabilities_finding, + :confirmed, + project: project, + report_type: 'sast', + project_fingerprint: confirmed_fingerprint) + create(:vulnerabilities_finding, + :resolved, + project: project, + report_type: 'sast', + project_fingerprint: resolved_fingerprint) + create(:vulnerabilities_finding, + :dismissed, + project: project, + report_type: 'sast', + project_fingerprint: dismissed_fingerprint) + end + + let(:confirmed_fingerprint) do + Digest::SHA1.hexdigest( + 'python/hardcoded/hardcoded-tmp.py:52865813c884a507be1f152d654245af34aba8a391626d01f1ab6d3f52ec8779:B108') + end + + let(:resolved_fingerprint) do + Digest::SHA1.hexdigest( + 'groovy/src/main/java/com/gitlab/security_products/tests/App.groovy:47:PREDICTABLE_RANDOM') + end + + let(:dismissed_fingerprint) do + Digest::SHA1.hexdigest( + 'groovy/src/main/java/com/gitlab/security_products/tests/App.groovy:41:PREDICTABLE_RANDOM') + end + + subject { described_class.new(pipeline: pipeline, params: { report_type: %w[sast], scope: 'all' }).execute } + + it 'assigns vulnerability records to findings providing them with computed state' do + confirmed = subject.find { |f| f.project_fingerprint == confirmed_fingerprint } + resolved = subject.find { |f| f.project_fingerprint == resolved_fingerprint } + dismissed = subject.find { |f| f.project_fingerprint == dismissed_fingerprint } + + expect(confirmed.state).to eq 'confirmed' + expect(resolved.state).to eq 'resolved' + expect(dismissed.state).to eq 'dismissed' + expect(subject - [confirmed, resolved, dismissed]).to all have_attributes(state: 'new') + end + end + def read_fixture(fixture) JSON.parse(File.read(fixture.file.path)) end diff --git a/ee/spec/models/vulnerabilities/occurrence_spec.rb b/ee/spec/models/vulnerabilities/occurrence_spec.rb index 513546e12fabeecbd574d78da8622b10404d4333..05bffacfd3899cc42492e293fcb6c3eefccb899a 100644 --- a/ee/spec/models/vulnerabilities/occurrence_spec.rb +++ b/ee/spec/models/vulnerabilities/occurrence_spec.rb @@ -356,4 +356,47 @@ end end end + + describe '#state' do + let(:new_finding) { create(:vulnerabilities_finding) } + let(:confirmed_finding) { create(:vulnerabilities_finding, :confirmed) } + let(:resolved_finding) { create(:vulnerabilities_finding, :resolved) } + let(:dismissed_finding) { create(:vulnerabilities_finding, :dismissed) } + + it 'returns the expected state for a new finding' do + expect(new_finding.state).to eq 'new' + end + + it 'returns the expected state for a confirmed finding' do + expect(confirmed_finding.state).to eq 'confirmed' + end + + it 'returns the expected state for a resolved finding' do + expect(resolved_finding.state).to eq 'resolved' + end + + it 'returns the expected state for a dismissed finding' do + expect(dismissed_finding.state).to eq 'dismissed' + end + + context 'when a vulnerability present for a dismissed finding' do + before do + create(:vulnerability, project: dismissed_finding.project, findings: [dismissed_finding]) + end + + it 'still reports a dismissed state' do + expect(dismissed_finding.state).to eq 'dismissed' + end + end + + context 'when a non-dismissal feedback present for a finding belonging to a closed vulnerability' do + before do + create(:vulnerability_feedback, :issue, project: resolved_finding.project) + end + + it 'reports as resolved' do + expect(resolved_finding.state).to eq 'resolved' + end + end + end end diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb index f6ca81eb41594ac1784de3f6809513fd8e1023c2..86fd61fb648905e642ca176e5fb4cc93be22827a 100644 --- a/ee/spec/policies/project_policy_spec.rb +++ b/ee/spec/policies/project_policy_spec.rb @@ -30,7 +30,12 @@ %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 dismiss_vulnerability] } + let(:additional_developer_permissions) do + %i[ + admin_vulnerability_feedback read_project_security_dashboard read_feature_flag + resolve_vulnerability dismiss_vulnerability + ] + end let(:additional_maintainer_permissions) { %i[push_code_to_protected_branches admin_feature_flags_client] } let(:auditor_permissions) do %i[ @@ -41,9 +46,8 @@ read_pipeline read_build read_commit_status read_container_image read_environment read_deployment read_merge_request read_pages create_merge_request_in award_emoji - read_project_security_dashboard + read_project_security_dashboard resolve_vulnerability dismiss_vulnerability read_vulnerability_feedback read_software_license_policy - dismiss_vulnerability ] end @@ -490,6 +494,7 @@ include_context 'when security dashboard feature is not available' + it { is_expected.to be_disallowed(:resolve_vulnerability) } it { is_expected.to be_disallowed(:dismiss_vulnerability) } end end diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index a036746eab1d07f808b224e0d7d1951a59762a64..0a4e82d77851ee56d2825696a8cf38b4e7f4757d 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -61,13 +61,16 @@ end it 'dismisses a vulnerability and its associated findings' do - subject + Timecop.freeze do + subject - expect(response).to have_gitlab_http_status(201) - expect(response).to match_response_schema('vulnerability', dir: 'ee') + 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 + expect(vulnerability.reload).to( + have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now))) + expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback + end end context 'when there is a dismissal error' do @@ -147,4 +150,79 @@ end end end + + describe "POST /vulnerabilities:id/resolve" do + before do + create_list(:vulnerabilities_finding, 2, vulnerability: vulnerability) + end + + let(:vulnerability) { project.vulnerabilities.first } + + subject { post api("/vulnerabilities/#{vulnerability.id}/resolve", user) } + + context 'with an authorized user with proper permissions' do + before do + project.add_developer(user) + end + + it 'resolves a vulnerability and its associated findings' do + Timecop.freeze do + subject + + expect(response).to have_gitlab_http_status(201) + expect(response).to match_response_schema('vulnerability', dir: 'ee') + + expect(vulnerability.reload).to( + have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now))) + expect(vulnerability.findings).to all have_attributes(state: 'resolved') + end + end + + context 'when the vulnerability is already resolved' do + let(:vulnerability) { create(:vulnerability, :closed, project: project) } + + it 'responds with 304 Not Modified response' do + subject + + expect(response).to have_gitlab_http_status(304) + 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 resolve a vulnerability' 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/services/vulnerabilities/dismiss_service_spec.rb b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb index e70fc493050d902c42eaec0d5fa5e7333de7162a..9452116f8dd0c9036bc313440dc7e81f88718955 100644 --- a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb +++ b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb @@ -20,10 +20,13 @@ end it 'dismisses a vulnerability and its associated findings' do - subject + Timecop.freeze do + subject - expect(vulnerability.reload).to be_closed - expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback + expect(vulnerability.reload).to( + have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now))) + expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback + end end context 'when there is a finding dismissal error' do diff --git a/ee/spec/services/vulnerabilities/resolve_service_spec.rb b/ee/spec/services/vulnerabilities/resolve_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..db628cf6b6b2b0c22bc21482d247910170476a87 --- /dev/null +++ b/ee/spec/services/vulnerabilities/resolve_service_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Vulnerabilities::ResolveService 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, 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 'resolves a vulnerability' do + Timecop.freeze do + subject + + expect(vulnerability.reload).to( + have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now))) + 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/shared_examples/requests/api/vulnerabilities_shared_examples.rb b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb index 02e3b404d36287b61366466681ddeb5d78fd556f..1f9282e365ff227d50e60dead071ee13d7e77c4e 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 @@ -108,7 +108,9 @@ get api(project_vulnerabilities_path, user), params: { report_type: 'dependency_scanning' } end.count - expect { get api(project_vulnerabilities_path, user) }.not_to exceed_query_limit(control_count) + # Threshold is required for the extra query performed in Security::PipelineVulnerabilitiesFinder to load + # the Vulnerabilities providing computed states for the associated Vulnerability::Occurrences + expect { get api(project_vulnerabilities_path, user) }.not_to exceed_query_limit(control_count).with_threshold(1) end describe 'filtering' do