From 839439b805352694bd687c1141dcb2e5a7c6bfa8 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Mon, 7 Oct 2019 21:01:16 +0300 Subject: [PATCH 01/27] Add VulnerabilityEntity w/ JSON schema tests --- ee/app/serializers/vulnerability_entity.rb | 26 +++++++++ .../fixtures/api/schemas/vulnerability.json | 53 +++++++++++++++++++ .../serializers/vulnerability_entity_spec.rb | 17 ++++++ 3 files changed, 96 insertions(+) create mode 100644 ee/app/serializers/vulnerability_entity.rb create mode 100644 ee/spec/fixtures/api/schemas/vulnerability.json create mode 100644 ee/spec/serializers/vulnerability_entity_spec.rb diff --git a/ee/app/serializers/vulnerability_entity.rb b/ee/app/serializers/vulnerability_entity.rb new file mode 100644 index 00000000000000..abfeaafc46c51d --- /dev/null +++ b/ee/app/serializers/vulnerability_entity.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class VulnerabilityEntity < Grape::Entity + expose :id + expose :title + expose :description + + expose :state + expose :severity + expose :confidence + + expose :project, using: ::ProjectEntity + + expose :author_id + expose :updated_by_id + expose :last_edited_by_id + expose :closed_by_id + + expose :start_date + expose :due_date + + expose :created_at + expose :updated_at + expose :last_edited_at + expose :closed_at +end diff --git a/ee/spec/fixtures/api/schemas/vulnerability.json b/ee/spec/fixtures/api/schemas/vulnerability.json new file mode 100644 index 00000000000000..703c3a0097edb2 --- /dev/null +++ b/ee/spec/fixtures/api/schemas/vulnerability.json @@ -0,0 +1,53 @@ +{ + "type": "object", + "required": ["title", "state", "confidence", "severity", "project", "author_id"], + "properties": { + "title": { + "type": "string" + }, + "description": { "type": ["string", "null"] }, + "state": { "type": "string", "enum": ["opened", "closed"] }, + "severity": { + "type": "string", + "enum": ["undefined", "info", "unknown", "low", "medium", "high", "critical"] + }, + "confidence": { + "type": "string", + "enum": [ + "undefined", + "ignore", + "unknown", + "experimental", + "low", + "medium", + "high", + "confirmed" + ] + }, + "project": { + "required": ["id", "name", "full_path", "full_name"], + "id": { + "type": "integer" + }, + "name": { + "type": "string" + }, + "full_path": { + "type": "string" + }, + "full_name": { + "type": "string" + } + }, + "author_id": { "type": "integer" }, + "updated_by_id": { "type": ["integer", "null"] }, + "last_edited_by_id": { "type": ["integer", "null"] }, + "closed_by_id": { "type": ["integer", "null"] }, + "start_date": { "type": ["date", "null"] }, + "due_date": { "type": ["date", "null"] }, + "created_at": { "type": "date" }, + "updated_at": { "type": "date" }, + "last_edited_at": { "type": "date" }, + "closed_at": { "type": "date" } + } +} diff --git a/ee/spec/serializers/vulnerability_entity_spec.rb b/ee/spec/serializers/vulnerability_entity_spec.rb new file mode 100644 index 00000000000000..4d422955b12fba --- /dev/null +++ b/ee/spec/serializers/vulnerability_entity_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe VulnerabilityEntity do + let(:vulnerability) do + create(:vulnerability) + end + + let(:entity) do + described_class.represent(vulnerability) + end + + subject { entity.to_json } + + it { is_expected.to match_schema('vulnerability', dir: 'ee') } +end -- GitLab From b53a7cea9a9fab9b1e7bf3e74c7546fb940d0ada Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 8 Oct 2019 15:51:40 +0300 Subject: [PATCH 02/27] Add VulnerabilitiesFinder for new Vulnerability Rename old VulnerabilitiesFinder to VulnerabilityFindingsFinder because it finds Occurrences (Findings in new terminology) and update its usages. Add new VulnerabilitiesFinder that finds Vulnerabilities of a Project and respects the pagination of results. --- .../vulnerability_findings_actions.rb | 2 +- .../security/vulnerabilities_finder.rb | 72 ++-------- .../security/vulnerability_findings_finder.rb | 78 ++++++++++ ee/lib/gitlab/vulnerabilities/history.rb | 2 +- .../gitlab/vulnerabilities/history_cache.rb | 2 +- ee/spec/factories/projects.rb | 6 + .../security/vulnerabilities_finder_spec.rb | 130 +---------------- .../vulnerability_findings_finder_spec.rb | 134 ++++++++++++++++++ 8 files changed, 238 insertions(+), 188 deletions(-) create mode 100644 ee/app/finders/security/vulnerability_findings_finder.rb create mode 100644 ee/spec/finders/security/vulnerability_findings_finder_spec.rb diff --git a/ee/app/controllers/concerns/vulnerability_findings_actions.rb b/ee/app/controllers/concerns/vulnerability_findings_actions.rb index 613d719f9a7020..1e3cb97dbb25c9 100644 --- a/ee/app/controllers/concerns/vulnerability_findings_actions.rb +++ b/ee/app/controllers/concerns/vulnerability_findings_actions.rb @@ -40,6 +40,6 @@ def filter_params end def vulnerability_findings(collection = :latest) - ::Security::VulnerabilitiesFinder.new(vulnerable, params: filter_params).execute(collection) + ::Security::VulnerabilityFindingsFinder.new(vulnerable, params: filter_params).execute(collection) end end diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb index ef10d00b408a4c..efc2cd53a9760d 100644 --- a/ee/app/finders/security/vulnerabilities_finder.rb +++ b/ee/app/finders/security/vulnerabilities_finder.rb @@ -2,75 +2,27 @@ # Security::VulnerabilitiesFinder # -# Used to filter Vulnerabilities::Occurrences by set of params for Security Dashboard +# Used to filter Vulnerability record by set of params for Vulnerabilities API # # Arguments: -# vulnerable - object to filter vulnerabilities +# project: a Project to query for Vulnerabilities # params: -# severity: Array -# confidence: Array -# project: Array -# report_type: Array +# page: Integer +# per_page: Integer module Security class VulnerabilitiesFinder - attr_accessor :params - attr_reader :vulnerable + attr_reader :project + attr_reader :page, :per_page - def initialize(vulnerable, params: {}) - @vulnerable = vulnerable - @params = params + def initialize(project, params = {}) + @project = project + @page = params[:page] || 1 + @per_page = params[:per_page] || 20 end - def execute(scope = :latest) - collection = init_collection(scope) - collection = by_report_type(collection) - collection = by_project(collection) - collection = by_severity(collection) - collection = by_confidence(collection) - collection - end - - private - - def by_report_type(items) - return items unless params[:report_type].present? - - items.by_report_types( - Vulnerabilities::Occurrence::REPORT_TYPES.values_at( - *params[:report_type]).compact) - end - - def by_project(items) - return items unless params[:project_id].present? - - items.by_projects(params[:project_id]) - end - - def by_severity(items) - return items unless params[:severity].present? - - items.by_severities( - Vulnerabilities::Occurrence::SEVERITY_LEVELS.values_at( - *params[:severity]).compact) - end - - def by_confidence(items) - return items unless params[:confidence].present? - - items.by_confidences( - Vulnerabilities::Occurrence::CONFIDENCE_LEVELS.values_at( - *params[:confidence]).compact) - end - - def init_collection(scope) - if scope == :all - vulnerable.all_vulnerabilities - elsif scope == :with_sha - vulnerable.latest_vulnerabilities_with_sha - else - vulnerable.latest_vulnerabilities - end + def execute + project.vulnerabilities.page(page).per(per_page) end end end diff --git a/ee/app/finders/security/vulnerability_findings_finder.rb b/ee/app/finders/security/vulnerability_findings_finder.rb new file mode 100644 index 00000000000000..5dc019c565444c --- /dev/null +++ b/ee/app/finders/security/vulnerability_findings_finder.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +# Security::VulnerabilityFindingsFinder +# +# Used to filter Vulnerabilities::Occurrences by set of params for Security Dashboard +# +# Arguments: +# vulnerable - object to filter vulnerabilities +# params: +# severity: Array +# confidence: Array +# project: Array +# report_type: Array + +module Security + # TODO: check usages and rename `vulnerable` associations to mention the term "finding"; + # probably, add new changes to https://gitlab.com/gitlab-org/gitlab/merge_requests/17351 + class VulnerabilityFindingsFinder + attr_accessor :params + attr_reader :vulnerable + + def initialize(vulnerable, params: {}) + @vulnerable = vulnerable + @params = params + end + + def execute(scope = :latest) + collection = init_collection(scope) + collection = by_report_type(collection) + collection = by_project(collection) + collection = by_severity(collection) + collection = by_confidence(collection) + collection + end + + private + + def by_report_type(items) + return items unless params[:report_type].present? + + items.by_report_types( + Vulnerabilities::Occurrence::REPORT_TYPES.values_at( + *params[:report_type]).compact) + end + + def by_project(items) + return items unless params[:project_id].present? + + items.by_projects(params[:project_id]) + end + + def by_severity(items) + return items unless params[:severity].present? + + items.by_severities( + Vulnerabilities::Occurrence::SEVERITY_LEVELS.values_at( + *params[:severity]).compact) + end + + def by_confidence(items) + return items unless params[:confidence].present? + + items.by_confidences( + Vulnerabilities::Occurrence::CONFIDENCE_LEVELS.values_at( + *params[:confidence]).compact) + end + + def init_collection(scope) + if scope == :all + vulnerable.all_vulnerabilities + elsif scope == :with_sha + vulnerable.latest_vulnerabilities_with_sha + else + vulnerable.latest_vulnerabilities + end + end + end +end diff --git a/ee/lib/gitlab/vulnerabilities/history.rb b/ee/lib/gitlab/vulnerabilities/history.rb index f0eb7882ed8494..0951617d9e9fbd 100644 --- a/ee/lib/gitlab/vulnerabilities/history.rb +++ b/ee/lib/gitlab/vulnerabilities/history.rb @@ -24,7 +24,7 @@ def vulnerabilities_counter private def found_vulnerabilities - ::Security::VulnerabilitiesFinder.new(group, params: filters).execute(:all) + ::Security::VulnerabilityFindingsFinder.new(group, params: filters).execute(:all) end def cached_vulnerability_history diff --git a/ee/lib/gitlab/vulnerabilities/history_cache.rb b/ee/lib/gitlab/vulnerabilities/history_cache.rb index 236420da8bc33a..1a5a55696e91e9 100644 --- a/ee/lib/gitlab/vulnerabilities/history_cache.rb +++ b/ee/lib/gitlab/vulnerabilities/history_cache.rb @@ -12,7 +12,7 @@ def initialize(group, project_id) def fetch(range, force: false) Rails.cache.fetch(cache_key, force: force, expires_in: 1.day) do - vulnerabilities = ::Security::VulnerabilitiesFinder + vulnerabilities = ::Security::VulnerabilityFindingsFinder .new(group, params: { project_id: [project_id] }) .execute(:all) .count_by_day_and_severity(range) diff --git a/ee/spec/factories/projects.rb b/ee/spec/factories/projects.rb index 2db74a89a6d05c..f3a8bc309f962e 100644 --- a/ee/spec/factories/projects.rb +++ b/ee/spec/factories/projects.rb @@ -89,5 +89,11 @@ trait :github_imported do import_type { 'github' } end + + trait :with_vulnerabilities do + after(:create) do |project| + create_list(:vulnerability, 3, :opened, project: project) + end + end end end diff --git a/ee/spec/finders/security/vulnerabilities_finder_spec.rb b/ee/spec/finders/security/vulnerabilities_finder_spec.rb index 17a61c311ba7ea..62710f9cacf5ef 100644 --- a/ee/spec/finders/security/vulnerabilities_finder_spec.rb +++ b/ee/spec/finders/security/vulnerabilities_finder_spec.rb @@ -3,132 +3,12 @@ require 'spec_helper' describe Security::VulnerabilitiesFinder do - describe '#execute' do - set(:group) { create(:group) } - set(:project1) { create(:project, :private, :repository, group: group) } - set(:project2) { create(:project, :private, :repository, group: group) } - set(:pipeline1) { create(:ci_pipeline, :success, project: project1) } - set(:pipeline2) { create(:ci_pipeline, :success, project: project2) } + let(:project) { create(:project, :with_vulnerabilities) } + let(:params) { { page: 2, per_page: 1 } } - set(:vulnerability1) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :high, confidence: :high, pipelines: [pipeline1], project: project1) } - set(:vulnerability2) { create(:vulnerabilities_occurrence, report_type: :dependency_scanning, severity: :medium, confidence: :low, pipelines: [pipeline2], project: project2) } - set(:vulnerability3) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :low, pipelines: [pipeline2], project: project2) } - set(:vulnerability4) { create(:vulnerabilities_occurrence, report_type: :dast, severity: :medium, pipelines: [pipeline1], project: project1) } + subject { described_class.new(project, params).execute } - subject { described_class.new(group, params: params).execute } - - context 'by report type' do - context 'when sast' do - let(:params) { { report_type: %w[sast] } } - - it 'includes only sast' do - is_expected.to contain_exactly(vulnerability1, vulnerability3) - end - end - - context 'when dependency_scanning' do - let(:params) { { report_type: %w[dependency_scanning] } } - - it 'includes only depscan' do - is_expected.to contain_exactly(vulnerability2) - end - end - end - - context 'by severity' do - context 'when high' do - let(:params) { { severity: %w[high] } } - - it 'includes only high' do - is_expected.to contain_exactly(vulnerability1) - end - end - - context 'when medium' do - let(:params) { { severity: %w[medium] } } - - it 'includes only medium' do - is_expected.to contain_exactly(vulnerability2, vulnerability4) - end - end - end - - context 'by confidence' do - context 'when high' do - let(:params) { { confidence: %w[high] } } - - it 'includes only high confidence vulnerabilities' do - is_expected.to contain_exactly(vulnerability1) - end - end - - context 'when low' do - let(:params) { { confidence: %w[low] } } - - it 'includes only low confidence vulnerabilities' do - is_expected.to contain_exactly(vulnerability2) - end - end - end - - context 'by project' do - let(:params) { { project_id: [project2.id] } } - - it 'includes only vulnerabilities for one project' do - is_expected.to contain_exactly(vulnerability2, vulnerability3) - end - end - - # FIXME: unskip when this filter is implemented - context 'by dismissals' do - let!(:dismissal) do - create(:vulnerability_feedback, :sast, :dismissal, - pipeline: pipeline1, - project: project1, - project_fingerprint: vulnerability1.project_fingerprint) - end - - let(:params) { { hide_dismissed: true } } - - skip 'exclude dismissal' do - is_expected.to contain_exactly(vulnerability2, vulnerability3, vulnerability4) - end - end - - context 'by all filters' do - context 'with found entity' do - let(:params) { { severity: %w[high medium low], project_id: [project1.id, project2.id], report_type: %w[sast dast] } } - - it 'filters by all params' do - is_expected.to contain_exactly(vulnerability1, vulnerability3, vulnerability4) - end - end - - context 'without found entity' do - let(:params) { { severity: %w[low], project_id: [project1.id], report_type: %w[sast] } } - - it 'did not find anything' do - is_expected.to be_empty - end - end - end - - context 'by some filters' do - context 'with found entity' do - let(:params) { { project_id: [project2.id], severity: %w[medium low] } } - - it 'filters by all params' do - is_expected.to contain_exactly(vulnerability2, vulnerability3) - end - end - - context 'without found entity' do - let(:params) { { project_id: project1.id, severity: %w[low] } } - - it 'did not find anything' do - is_expected.to be_empty - end - end - end + it 'returns vulnerabilities of a project and respects pagination params' do + expect(subject).to contain_exactly(project.vulnerabilities.drop(1).take(1).first) end end diff --git a/ee/spec/finders/security/vulnerability_findings_finder_spec.rb b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb new file mode 100644 index 00000000000000..3edee7d6f31db1 --- /dev/null +++ b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Security::VulnerabilityFindingsFinder do + describe '#execute' do + set(:group) { create(:group) } + set(:project1) { create(:project, :private, :repository, group: group) } + set(:project2) { create(:project, :private, :repository, group: group) } + set(:pipeline1) { create(:ci_pipeline, :success, project: project1) } + set(:pipeline2) { create(:ci_pipeline, :success, project: project2) } + + set(:vulnerability1) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :high, confidence: :high, pipelines: [pipeline1], project: project1) } + set(:vulnerability2) { create(:vulnerabilities_occurrence, report_type: :dependency_scanning, severity: :medium, confidence: :low, pipelines: [pipeline2], project: project2) } + set(:vulnerability3) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :low, pipelines: [pipeline2], project: project2) } + set(:vulnerability4) { create(:vulnerabilities_occurrence, report_type: :dast, severity: :medium, pipelines: [pipeline1], project: project1) } + + subject { described_class.new(group, params: params).execute } + + context 'by report type' do + context 'when sast' do + let(:params) { { report_type: %w[sast] } } + + it 'includes only sast' do + is_expected.to contain_exactly(vulnerability1, vulnerability3) + end + end + + context 'when dependency_scanning' do + let(:params) { { report_type: %w[dependency_scanning] } } + + it 'includes only depscan' do + is_expected.to contain_exactly(vulnerability2) + end + end + end + + context 'by severity' do + context 'when high' do + let(:params) { { severity: %w[high] } } + + it 'includes only high' do + is_expected.to contain_exactly(vulnerability1) + end + end + + context 'when medium' do + let(:params) { { severity: %w[medium] } } + + it 'includes only medium' do + is_expected.to contain_exactly(vulnerability2, vulnerability4) + end + end + end + + context 'by confidence' do + context 'when high' do + let(:params) { { confidence: %w[high] } } + + it 'includes only high confidence vulnerabilities' do + is_expected.to contain_exactly(vulnerability1) + end + end + + context 'when low' do + let(:params) { { confidence: %w[low] } } + + it 'includes only low confidence vulnerabilities' do + is_expected.to contain_exactly(vulnerability2) + end + end + end + + context 'by project' do + let(:params) { { project_id: [project2.id] } } + + it 'includes only vulnerabilities for one project' do + is_expected.to contain_exactly(vulnerability2, vulnerability3) + end + end + + # FIXME: unskip when this filter is implemented + context 'by dismissals' do + let!(:dismissal) do + create(:vulnerability_feedback, :sast, :dismissal, + pipeline: pipeline1, + project: project1, + project_fingerprint: vulnerability1.project_fingerprint) + end + + let(:params) { { hide_dismissed: true } } + + skip 'exclude dismissal' do + is_expected.to contain_exactly(vulnerability2, vulnerability3, vulnerability4) + end + end + + context 'by all filters' do + context 'with found entity' do + let(:params) { { severity: %w[high medium low], project_id: [project1.id, project2.id], report_type: %w[sast dast] } } + + it 'filters by all params' do + is_expected.to contain_exactly(vulnerability1, vulnerability3, vulnerability4) + end + end + + context 'without found entity' do + let(:params) { { severity: %w[low], project_id: [project1.id], report_type: %w[sast] } } + + it 'did not find anything' do + is_expected.to be_empty + end + end + end + + context 'by some filters' do + context 'with found entity' do + let(:params) { { project_id: [project2.id], severity: %w[medium low] } } + + it 'filters by all params' do + is_expected.to contain_exactly(vulnerability2, vulnerability3) + end + end + + context 'without found entity' do + let(:params) { { project_id: project1.id, severity: %w[low] } } + + it 'did not find anything' do + is_expected.to be_empty + end + end + end + end +end -- GitLab From a9ab7f5f2c3f58a44a7cf5a3158b28d25dc25f45 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 8 Oct 2019 15:54:16 +0300 Subject: [PATCH 03/27] Introduce GET Project's Vulnerabilities API call Extract Vulnerabilities API into a separate module and implement GET projects/:id/vulnerabilities API operation. Respect :first_class_vulnerabilities feature flag. --- .../helpers/vulnerability_findings_helpers.rb | 62 +++++ ee/lib/api/vulnerabilities.rb | 49 ++++ ee/lib/api/vulnerability_findings.rb | 74 +----- ee/lib/ee/api/api.rb | 1 + .../api/schemas/vulnerability_list.json | 4 + ee/spec/requests/api/vulnerabilities_spec.rb | 41 ++++ .../api/vulnerability_findings_spec.rb | 229 +----------------- .../api/vulnerabilities_shared_examples.rb | 216 +++++++++++++++++ 8 files changed, 382 insertions(+), 294 deletions(-) create mode 100644 ee/lib/api/helpers/vulnerability_findings_helpers.rb create mode 100644 ee/lib/api/vulnerabilities.rb create mode 100644 ee/spec/fixtures/api/schemas/vulnerability_list.json create mode 100644 ee/spec/requests/api/vulnerabilities_spec.rb create mode 100644 ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb diff --git a/ee/lib/api/helpers/vulnerability_findings_helpers.rb b/ee/lib/api/helpers/vulnerability_findings_helpers.rb new file mode 100644 index 00000000000000..0f43e21a9896af --- /dev/null +++ b/ee/lib/api/helpers/vulnerability_findings_helpers.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module API + module Helpers + module VulnerabilityFindingsHelpers + extend Grape::API::Helpers + + params :vulnerability_findings_params do + optional :report_type, type: Array[String], desc: 'The type of report vulnerability belongs to', + values: ::Vulnerabilities::Occurrence.report_types.keys, + default: ::Vulnerabilities::Occurrence.report_types.keys + optional :scope, type: String, desc: 'Return vulnerabilities for the given scope: `dismissed` or `all`', + default: 'dismissed', values: %w[all dismissed] + optional :severity, + type: Array[String], + desc: 'Returns issues belonging to specified severity level: '\ + '`undefined`, `info`, `unknown`, `low`, `medium`, `high`, or `critical`. Defaults to all', + values: ::Vulnerabilities::Occurrence.severities.keys, + default: ::Vulnerabilities::Occurrence.severities.keys + optional :confidence, + type: Array[String], + desc: 'Returns vulnerabilities belonging to specified confidence level: '\ + '`undefined`, `ignore`, `unknown`, `experimental`, `low`, `medium`, `high`, or `confirmed`. '\ + 'Defaults to all', + values: ::Vulnerabilities::Occurrence.confidences.keys, + default: ::Vulnerabilities::Occurrence.confidences.keys + optional :pipeline_id, type: String, desc: 'The ID of the pipeline' + + use :pagination + end + + # TODO: rename to vulnerability_findings_by https://gitlab.com/gitlab-org/gitlab/issues/32963 + def vulnerability_occurrences_by(params) + pipeline = if params[:pipeline_id] + params[:project].all_pipelines.find_by(id: params[:pipeline_id]) # rubocop:disable CodeReuse/ActiveRecord + else + params[:project].latest_pipeline_with_security_reports + end + + return [] unless pipeline + + Security::PipelineVulnerabilitiesFinder.new(pipeline: pipeline, params: params).execute + end + + def respond_with_vulnerability_findings + authorize! :read_project_security_dashboard, user_project + + vulnerability_occurrences = paginate( + Kaminari.paginate_array( + vulnerability_occurrences_by(declared_params.merge(project: user_project)) + ) + ) + + Gitlab::Vulnerabilities::OccurrencesPreloader.preload_feedback!(vulnerability_occurrences) + + present vulnerability_occurrences, + with: ::Vulnerabilities::OccurrenceEntity, + request: GrapeRequestProxy.new(request, current_user) + end + end + end +end diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb new file mode 100644 index 00000000000000..83838ee4902afe --- /dev/null +++ b/ee/lib/api/vulnerabilities.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module API + class Vulnerabilities < Grape::API + include PaginationParams + + helpers ::API::Helpers::VulnerabilityFindingsHelpers + + helpers do + def vulnerabilities_by(project, params) + Security::VulnerabilitiesFinder.new(project, params).execute + end + end + + before do + authenticate! + 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 + # responding with Vulnerability Findings when :first_class_vulnerabilities feature is disabled. + # TODO: remove usage of :vulnerability_findings_params when feature flag is removed + # https://gitlab.com/gitlab-org/gitlab/issues/33488 + use :vulnerability_findings_params + end + desc 'Get a list of project vulnerabilities' do + success VulnerabilityEntity + end + get ':id/vulnerabilities' do + if Feature.enabled?(:first_class_vulnerabilities) + authorize! :read_project_security_dashboard, user_project + + vulnerabilities = paginate( + vulnerabilities_by(user_project, declared_params) + ) + + present vulnerabilities, with: VulnerabilityEntity + else + respond_with_vulnerability_findings + end + end + end + end +end diff --git a/ee/lib/api/vulnerability_findings.rb b/ee/lib/api/vulnerability_findings.rb index 5322d1cafce016..2dbcd0eb3091d8 100644 --- a/ee/lib/api/vulnerability_findings.rb +++ b/ee/lib/api/vulnerability_findings.rb @@ -4,65 +4,7 @@ module API class VulnerabilityFindings < Grape::API include PaginationParams - helpers do - params :vulnerability_findings_params do - optional :report_type, type: Array[String], desc: 'The type of report vulnerability belongs to', - values: ::Vulnerabilities::Occurrence.report_types.keys, - default: ::Vulnerabilities::Occurrence.report_types.keys - optional :scope, type: String, desc: 'Return vulnerabilities for the given scope: `dismissed` or `all`', - default: 'dismissed', values: %w[all dismissed] - optional :severity, - type: Array[String], - desc: 'Returns issues belonging to specified severity level: '\ - '`undefined`, `info`, `unknown`, `low`, `medium`, `high`, or `critical`. Defaults to all', - values: ::Vulnerabilities::Occurrence.severities.keys, - default: ::Vulnerabilities::Occurrence.severities.keys - optional :confidence, - type: Array[String], - desc: 'Returns vulnerabilities belonging to specified confidence level: '\ - '`undefined`, `ignore`, `unknown`, `experimental`, `low`, `medium`, `high`, or `confirmed`. '\ - 'Defaults to all', - values: ::Vulnerabilities::Occurrence.confidences.keys, - default: ::Vulnerabilities::Occurrence.confidences.keys - optional :pipeline_id, type: String, desc: 'The ID of the pipeline' - - use :pagination - end - - def vulnerability_occurrences_by(params) - pipeline = if params[:pipeline_id] - params[:project].all_pipelines.find_by(id: params[:pipeline_id]) # rubocop:disable CodeReuse/ActiveRecord - else - params[:project].latest_pipeline_with_security_reports - end - - return [] unless pipeline - - Security::PipelineVulnerabilitiesFinder.new(pipeline: pipeline, params: params).execute - end - - def respond_with_vulnerabilities - # TODO: implement the "Get a list of project's Vulnerabilities" step - # of https://gitlab.com/gitlab-org/gitlab-ee/issues/10242#status - not_found! - end - - def respond_with_vulnerability_findings - authorize! :read_project_security_dashboard, user_project - - vulnerability_occurrences = paginate( - Kaminari.paginate_array( - vulnerability_occurrences_by(declared_params.merge(project: user_project)) - ) - ) - - Gitlab::Vulnerabilities::OccurrencesPreloader.preload_feedback!(vulnerability_occurrences) - - present vulnerability_occurrences, - with: ::Vulnerabilities::OccurrenceEntity, - request: GrapeRequestProxy.new(request, current_user) - end - end + helpers ::API::Helpers::VulnerabilityFindingsHelpers before do authenticate! @@ -73,20 +15,6 @@ def respond_with_vulnerability_findings end resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do - params do - use :vulnerability_findings_params - end - desc 'Get a list of project vulnerabilities' do - success ::Vulnerabilities::OccurrenceEntity - end - get ':id/vulnerabilities' do - if Feature.enabled?(:first_class_vulnerabilities) - respond_with_vulnerabilities - else - respond_with_vulnerability_findings - end - end - params do use :vulnerability_findings_params end diff --git a/ee/lib/ee/api/api.rb b/ee/lib/ee/api/api.rb index bc7de4f1e07a31..375dd0a67f13bf 100644 --- a/ee/lib/ee/api/api.rb +++ b/ee/lib/ee/api/api.rb @@ -35,6 +35,7 @@ module API mount ::API::Scim mount ::API::ManagedLicenses mount ::API::ProjectApprovals + mount ::API::Vulnerabilities mount ::API::VulnerabilityFindings mount ::API::MergeRequestApprovals mount ::API::MergeRequestApprovalRules diff --git a/ee/spec/fixtures/api/schemas/vulnerability_list.json b/ee/spec/fixtures/api/schemas/vulnerability_list.json new file mode 100644 index 00000000000000..430ac7a386bbfb --- /dev/null +++ b/ee/spec/fixtures/api/schemas/vulnerability_list.json @@ -0,0 +1,4 @@ +{ + "type": "array", + "items": { "$ref": "vulnerability.json" } +} diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb new file mode 100644 index 00000000000000..2c2e12144a6a2b --- /dev/null +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Vulnerabilities do + before do + stub_licensed_features(security_dashboard: true) + end + + let_it_be(:project) { create(:project, :public, :with_vulnerabilities) } + let_it_be(:user) { create(:user) } + + describe "GET /projects/:id/vulnerabilities" do + let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerabilities" } + + context 'with an authorized user with proper permissions' do + before do + project.add_developer(user) + end + + it 'returns all vulnerabilities of a project' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(response).to match_response_schema('vulnerability_list', dir: 'ee') + expect(response.headers['X-Total']).to eq project.vulnerabilities.count.to_s + end + end + + it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' + + context 'when "first-class vulnerabilities" feature is disabled' do + before do + stub_feature_flags(first_class_vulnerabilities: false) + end + + it_behaves_like 'getting list of vulnerability findings' + end + end +end diff --git a/ee/spec/requests/api/vulnerability_findings_spec.rb b/ee/spec/requests/api/vulnerability_findings_spec.rb index d10c3a712342ff..112e8f0d278333 100644 --- a/ee/spec/requests/api/vulnerability_findings_spec.rb +++ b/ee/spec/requests/api/vulnerability_findings_spec.rb @@ -3,237 +3,24 @@ require 'spec_helper' describe API::VulnerabilityFindings do - set(:project) { create(:project, :public) } - set(:user) { create(:user) } + let_it_be(:project) { create(:project, :public) } + let_it_be(:user) { create(:user) } - let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) } - let(:pipeline_without_vulnerabilities) { create(:ci_pipeline_without_jobs, status: :created, project: project) } - - let(:build_ds) { create(:ci_build, :success, name: 'ds_job', pipeline: pipeline, project: project) } - let(:build_sast) { create(:ci_build, :success, name: 'sast_job', pipeline: pipeline, project: project) } - - let(:ds_report) { pipeline.security_reports.reports["dependency_scanning"] } - let(:sast_report) { pipeline.security_reports.reports["sast"] } - - let(:dismissal) do - create(:vulnerability_feedback, :dismissal, :sast, - project: project, - pipeline: pipeline, - project_fingerprint: sast_report.occurrences.first.project_fingerprint, - vulnerability_data: sast_report.occurrences.first.raw_metadata - ) - end - - before do - stub_licensed_features(security_dashboard: true, sast: true, dependency_scanning: true, container_scanning: true) - - create(:ee_ci_job_artifact, :dependency_scanning, job: build_ds, project: project) - create(:ee_ci_job_artifact, :sast, job: build_sast, project: project) - dismissal - end - - shared_examples 'getting list of vulnerability findings' do - let(:project_vulnerabilities_path) { "/projects/#{project.id}/#{api_resource_name}" } - - context 'with an authorized user with proper permissions' do - before do - project.add_developer(user) - end - - it 'returns all non-dismissed vulnerabilities' do - occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s - - get api(project_vulnerabilities_path, user), params: { per_page: 40 } - - expect(response).to have_gitlab_http_status(200) - expect(response).to include_pagination_headers - expect(response).to match_response_schema('vulnerabilities/occurrence_list', dir: 'ee') - - expect(response.headers['X-Total']).to eq occurrence_count - - expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning sast] - end - - it 'does not have N+1 queries' do - control_count = ActiveRecord::QueryRecorder.new do - 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) - end - - describe 'filtering' do - it 'returns vulnerabilities with sast report_type' do - occurrence_count = (sast_report.occurrences.count - 1).to_s - - get api(project_vulnerabilities_path, user), params: { report_type: 'sast' } - - expect(response).to have_gitlab_http_status(200) - - expect(response.headers['X-Total']).to eq occurrence_count - - expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[sast] - - # occurrences are implicitly sorted by Security::MergeReportsService, - # occurrences order differs from what is present in fixture file - expect(json_response.first['name']).to eq 'ECB mode is insecure' - end - - it 'returns vulnerabilities with dependency_scanning report_type' do - occurrence_count = ds_report.occurrences.count.to_s - - get api(project_vulnerabilities_path, user), params: { report_type: 'dependency_scanning' } - - expect(response).to have_gitlab_http_status(200) - - expect(response.headers['X-Total']).to eq occurrence_count - - expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning] - - # occurrences are implicitly sorted by Security::MergeReportsService, - # occurrences order differs from what is present in fixture file - expect(json_response.first['name']).to eq 'ruby-ffi DDL loading issue on Windows OS' - end - - it 'returns a "bad request" response for an unknown report type' do - get api(project_vulnerabilities_path, user), params: { report_type: 'blah' } - - expect(response).to have_gitlab_http_status(400) - end - - it 'returns dismissed vulnerabilities with `all` scope' do - occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count).to_s - - get api(project_vulnerabilities_path, user), params: { per_page: 40, scope: 'all' } - - expect(response).to have_gitlab_http_status(200) - - expect(response.headers['X-Total']).to eq occurrence_count - end - - it 'returns vulnerabilities with low severity' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, severity: 'low' } - - expect(response).to have_gitlab_http_status(200) - - expect(json_response.map { |v| v['severity'] }.uniq).to eq %w[low] - end - - it 'returns a "bad request" response for an unknown severity value' do - get api(project_vulnerabilities_path, user), params: { severity: 'foo' } - - expect(response).to have_gitlab_http_status(400) - end - - it 'returns vulnerabilities with high confidence' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, confidence: 'high' } - - expect(response).to have_gitlab_http_status(200) - - expect(json_response.map { |v| v['confidence'] }.uniq).to eq %w[high] - end - - it 'returns a "bad request" response for an unknown confidence value' do - get api(project_vulnerabilities_path, user), params: { confidence: 'qux' } - - expect(response).to have_gitlab_http_status(400) - end - - context 'when pipeline_id is supplied' do - it 'returns vulnerabilities from supplied pipeline' do - occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s - - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline.id } - - expect(response).to have_gitlab_http_status(200) - - expect(response.headers['X-Total']).to eq occurrence_count - end - - context 'pipeline has no reports' do - it 'returns empty results' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline_without_vulnerabilities.id } - - expect(json_response).to eq [] - end - end - - context 'with unknown pipeline' do - it 'returns empty results' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: 0 } + describe "GET /projects/:id/vulnerability_findings" do + let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerability_findings" } - expect(json_response).to eq [] - end - end - end - end - end + it_behaves_like 'getting list of vulnerability findings' - context 'with authorized user without read permissions' do + context 'when "first-class vulnerabilities" feature is disabled' do before do - project.add_reporter(user) - stub_licensed_features(security_dashboard: false, sast: true, dependency_scanning: true, container_scanning: true) + stub_feature_flags(first_class_vulnerabilities: false) end - it 'responds with 403 Forbidden' do + it 'responds with "not found"' do get api(project_vulnerabilities_path, user) - expect(response).to have_gitlab_http_status(403) - end - end - - context 'with no project access' do - it 'responds with 404 Not Found' do - private_project = create(:project) - - get api("/projects/#{private_project.id}/#{api_resource_name}", user) - expect(response).to have_gitlab_http_status(404) end end - - context 'with unknown project' do - it 'responds with 404 Not Found' do - get api("/projects/0/#{api_resource_name}", user) - - expect(response).to have_gitlab_http_status(404) - end - end - end - - shared_examples 'not found vulnerabilities endpoint' do - it do - get api("/projects/#{project.id}/#{api_resource_name}?", user), params: { per_page: 40 } - - expect(response).to have_gitlab_http_status(404) - end - end - - describe "GET /projects/:id/vulnerabilities" do - let(:api_resource_name) { 'vulnerabilities' } - - it_behaves_like 'not found vulnerabilities endpoint' - - context 'when vulnerability findings API is disabled' do - before do - stub_feature_flags(first_class_vulnerabilities: false) - end - - it_behaves_like 'getting list of vulnerability findings' - end - end - - describe "GET /projects/:id/vulnerability_findings" do - let(:api_resource_name) { 'vulnerability_findings' } - - it_behaves_like 'getting list of vulnerability findings' - - context 'when vulnerability findings API is disabled' do - before do - stub_feature_flags(first_class_vulnerabilities: false) - end - - it_behaves_like 'not found vulnerabilities endpoint' - 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 new file mode 100644 index 00000000000000..f213bd58eb9956 --- /dev/null +++ b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb @@ -0,0 +1,216 @@ +# frozen_string_literal: true + +shared_examples 'forbids access to vulnerability-like endpoint in expected cases' do + context 'with authorized user without read permissions' do + before do + project.add_reporter(user) + end + + it 'responds with 403 Forbidden' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(403) + end + end + + context 'with authorized user but when security dashboard is not available' do + before do + project.add_developer(user) + stub_licensed_features(security_dashboard: false) + end + + it 'responds with 403 Forbidden' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(403) + end + end + + context 'with no project access' do + let(:project) { create(:project) } + + it 'responds with 404 Not Found' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'with unknown project' do + before do + project.id = 0 + end + + let(:project) { build(:project) } + + it 'responds with 404 Not Found' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(404) + end + end +end + +shared_examples 'getting list of vulnerability findings' do + before do + stub_licensed_features(security_dashboard: true, sast: true, dependency_scanning: true, container_scanning: true) + + create(:ee_ci_job_artifact, :dependency_scanning, job: build_ds, project: project) + create(:ee_ci_job_artifact, :sast, job: build_sast, project: project) + dismissal + end + + let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) } + let(:pipeline_without_vulnerabilities) { create(:ci_pipeline_without_jobs, status: :created, project: project) } + + let(:build_ds) { create(:ci_build, :success, name: 'ds_job', pipeline: pipeline, project: project) } + let(:build_sast) { create(:ci_build, :success, name: 'sast_job', pipeline: pipeline, project: project) } + + let(:ds_report) { pipeline.security_reports.reports["dependency_scanning"] } + let(:sast_report) { pipeline.security_reports.reports["sast"] } + + let(:dismissal) do + create(:vulnerability_feedback, :dismissal, :sast, + project: project, + pipeline: pipeline, + project_fingerprint: sast_report.occurrences.first.project_fingerprint, + vulnerability_data: sast_report.occurrences.first.raw_metadata + ) + end + + context 'with an authorized user with proper permissions' do + before do + project.add_developer(user) + end + + it 'returns all non-dismissed vulnerabilities' do + occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s + + get api(project_vulnerabilities_path, user), params: { per_page: 40 } + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(response).to match_response_schema('vulnerabilities/occurrence_list', dir: 'ee') + + expect(response.headers['X-Total']).to eq occurrence_count + + expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning sast] + end + + it 'does not have N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new do + 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) + end + + describe 'filtering' do + it 'returns vulnerabilities with sast report_type' do + occurrence_count = (sast_report.occurrences.count - 1).to_s + + get api(project_vulnerabilities_path, user), params: { report_type: 'sast' } + + expect(response).to have_gitlab_http_status(200) + + expect(response.headers['X-Total']).to eq occurrence_count + + expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[sast] + + # occurrences are implicitly sorted by Security::MergeReportsService, + # occurrences order differs from what is present in fixture file + expect(json_response.first['name']).to eq 'ECB mode is insecure' + end + + it 'returns vulnerabilities with dependency_scanning report_type' do + occurrence_count = ds_report.occurrences.count.to_s + + get api(project_vulnerabilities_path, user), params: { report_type: 'dependency_scanning' } + + expect(response).to have_gitlab_http_status(200) + + expect(response.headers['X-Total']).to eq occurrence_count + + expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning] + + # occurrences are implicitly sorted by Security::MergeReportsService, + # occurrences order differs from what is present in fixture file + expect(json_response.first['name']).to eq 'ruby-ffi DDL loading issue on Windows OS' + end + + it 'returns a "bad request" response for an unknown report type' do + get api(project_vulnerabilities_path, user), params: { report_type: 'blah' } + + expect(response).to have_gitlab_http_status(400) + end + + it 'returns dismissed vulnerabilities with `all` scope' do + occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count).to_s + + get api(project_vulnerabilities_path, user), params: { per_page: 40, scope: 'all' } + + expect(response).to have_gitlab_http_status(200) + + expect(response.headers['X-Total']).to eq occurrence_count + end + + it 'returns vulnerabilities with low severity' do + get api(project_vulnerabilities_path, user), params: { per_page: 40, severity: 'low' } + + expect(response).to have_gitlab_http_status(200) + + expect(json_response.map { |v| v['severity'] }.uniq).to eq %w[low] + end + + it 'returns a "bad request" response for an unknown severity value' do + get api(project_vulnerabilities_path, user), params: { severity: 'foo' } + + expect(response).to have_gitlab_http_status(400) + end + + it 'returns vulnerabilities with high confidence' do + get api(project_vulnerabilities_path, user), params: { per_page: 40, confidence: 'high' } + + expect(response).to have_gitlab_http_status(200) + + expect(json_response.map { |v| v['confidence'] }.uniq).to eq %w[high] + end + + it 'returns a "bad request" response for an unknown confidence value' do + get api(project_vulnerabilities_path, user), params: { confidence: 'qux' } + + expect(response).to have_gitlab_http_status(400) + end + + context 'when pipeline_id is supplied' do + it 'returns vulnerabilities from supplied pipeline' do + occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s + + get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline.id } + + expect(response).to have_gitlab_http_status(200) + + expect(response.headers['X-Total']).to eq occurrence_count + end + + context 'pipeline has no reports' do + it 'returns empty results' do + get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline_without_vulnerabilities.id } + + expect(json_response).to eq [] + end + end + + context 'with unknown pipeline' do + it 'returns empty results' do + get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: 0 } + + expect(json_response).to eq [] + end + end + end + end + end + + it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' +end -- GitLab From a05b106b6a6dfd1d4d915f1326b0bd672d201415 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 8 Oct 2019 18:24:11 +0300 Subject: [PATCH 04/27] Move pagination to API level where it should be --- ee/app/finders/security/vulnerabilities_finder.rb | 10 ++-------- ee/lib/api/vulnerabilities.rb | 6 +++--- .../finders/security/vulnerabilities_finder_spec.rb | 5 ++--- ee/spec/requests/api/vulnerabilities_spec.rb | 7 +++++++ 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb index efc2cd53a9760d..888468d5c2344e 100644 --- a/ee/app/finders/security/vulnerabilities_finder.rb +++ b/ee/app/finders/security/vulnerabilities_finder.rb @@ -6,23 +6,17 @@ # # Arguments: # project: a Project to query for Vulnerabilities -# params: -# page: Integer -# per_page: Integer module Security class VulnerabilitiesFinder attr_reader :project - attr_reader :page, :per_page - def initialize(project, params = {}) + def initialize(project) @project = project - @page = params[:page] || 1 - @per_page = params[:per_page] || 20 end def execute - project.vulnerabilities.page(page).per(per_page) + project.vulnerabilities end end end diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb index 83838ee4902afe..63e58a366ba826 100644 --- a/ee/lib/api/vulnerabilities.rb +++ b/ee/lib/api/vulnerabilities.rb @@ -7,8 +7,8 @@ class Vulnerabilities < Grape::API helpers ::API::Helpers::VulnerabilityFindingsHelpers helpers do - def vulnerabilities_by(project, params) - Security::VulnerabilitiesFinder.new(project, params).execute + def vulnerabilities_by(project) + Security::VulnerabilitiesFinder.new(project).execute end end @@ -36,7 +36,7 @@ def vulnerabilities_by(project, params) authorize! :read_project_security_dashboard, user_project vulnerabilities = paginate( - vulnerabilities_by(user_project, declared_params) + vulnerabilities_by(user_project) ) present vulnerabilities, with: VulnerabilityEntity diff --git a/ee/spec/finders/security/vulnerabilities_finder_spec.rb b/ee/spec/finders/security/vulnerabilities_finder_spec.rb index 62710f9cacf5ef..70b12eaa103043 100644 --- a/ee/spec/finders/security/vulnerabilities_finder_spec.rb +++ b/ee/spec/finders/security/vulnerabilities_finder_spec.rb @@ -4,11 +4,10 @@ describe Security::VulnerabilitiesFinder do let(:project) { create(:project, :with_vulnerabilities) } - let(:params) { { page: 2, per_page: 1 } } - subject { described_class.new(project, params).execute } + subject { described_class.new(project).execute } it 'returns vulnerabilities of a project and respects pagination params' do - expect(subject).to contain_exactly(project.vulnerabilities.drop(1).take(1).first) + expect(subject).to match_array(project.vulnerabilities) end end diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index 2c2e12144a6a2b..93bdd635455915 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -26,6 +26,13 @@ expect(response).to match_response_schema('vulnerability_list', dir: 'ee') expect(response.headers['X-Total']).to eq project.vulnerabilities.count.to_s end + + it 'paginates the vulnerabilities according to the pagination params' do + get api("#{project_vulnerabilities_path}?page=2&per_page=1", user) + + expect(response).to have_gitlab_http_status(200) + expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.drop(1).take(1).first.id) + end end it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' -- GitLab From d3529fd2ddc659a6f20a7a86b081442b43721480 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 20:34:09 +0300 Subject: [PATCH 05/27] Fix typo in comment --- ee/app/finders/security/vulnerabilities_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb index 888468d5c2344e..932b9b31c1f1a4 100644 --- a/ee/app/finders/security/vulnerabilities_finder.rb +++ b/ee/app/finders/security/vulnerabilities_finder.rb @@ -2,7 +2,7 @@ # Security::VulnerabilitiesFinder # -# Used to filter Vulnerability record by set of params for Vulnerabilities API +# Used to filter Vulnerability records by set of params for Vulnerabilities API # # Arguments: # project: a Project to query for Vulnerabilities -- GitLab From 12baf134ee960a882c7aa37067851527057a027a Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 22:33:00 +0300 Subject: [PATCH 06/27] Rename VulnerabilityFindingsFinder dependencies --- .../groups/security/vulnerabilities_controller.rb | 2 +- ee/lib/gitlab/vulnerabilities/history.rb | 8 ++++---- ee/lib/gitlab/vulnerabilities/history_cache.rb | 6 ++++-- ee/spec/lib/gitlab/vulnerabilities/history_spec.rb | 4 ++-- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/ee/app/controllers/groups/security/vulnerabilities_controller.rb b/ee/app/controllers/groups/security/vulnerabilities_controller.rb index 5bfaacf9b1981a..19c84d06c26ced 100644 --- a/ee/app/controllers/groups/security/vulnerabilities_controller.rb +++ b/ee/app/controllers/groups/security/vulnerabilities_controller.rb @@ -7,7 +7,7 @@ class Groups::Security::VulnerabilitiesController < Groups::ApplicationControlle alias_method :vulnerable, :group def history - history_count = Gitlab::Vulnerabilities::History.new(group, filter_params).vulnerabilities_counter + history_count = Gitlab::Vulnerabilities::History.new(group, filter_params).findings_counter respond_to do |format| format.json do diff --git a/ee/lib/gitlab/vulnerabilities/history.rb b/ee/lib/gitlab/vulnerabilities/history.rb index 0951617d9e9fbd..749b50fe92f8c3 100644 --- a/ee/lib/gitlab/vulnerabilities/history.rb +++ b/ee/lib/gitlab/vulnerabilities/history.rb @@ -14,16 +14,16 @@ def initialize(group, filters) @filters = filters end - def vulnerabilities_counter + def findings_counter return cached_vulnerability_history if use_vulnerability_cache? - vulnerabilities = found_vulnerabilities.count_by_day_and_severity(HISTORY_RANGE) - ::Vulnerabilities::HistorySerializer.new.represent(vulnerabilities) + findings = vulnerability_findings.count_by_day_and_severity(HISTORY_RANGE) + ::Vulnerabilities::HistorySerializer.new.represent(findings) end private - def found_vulnerabilities + def vulnerability_findings ::Security::VulnerabilityFindingsFinder.new(group, params: filters).execute(:all) end diff --git a/ee/lib/gitlab/vulnerabilities/history_cache.rb b/ee/lib/gitlab/vulnerabilities/history_cache.rb index 1a5a55696e91e9..d6f48547f59492 100644 --- a/ee/lib/gitlab/vulnerabilities/history_cache.rb +++ b/ee/lib/gitlab/vulnerabilities/history_cache.rb @@ -12,17 +12,19 @@ def initialize(group, project_id) def fetch(range, force: false) Rails.cache.fetch(cache_key, force: force, expires_in: 1.day) do - vulnerabilities = ::Security::VulnerabilityFindingsFinder + findings = ::Security::VulnerabilityFindingsFinder .new(group, params: { project_id: [project_id] }) .execute(:all) .count_by_day_and_severity(range) - ::Vulnerabilities::HistorySerializer.new.represent(vulnerabilities) + ::Vulnerabilities::HistorySerializer.new.represent(findings) end end private def cache_key + # TODO: rename 'vulnerabilities' to 'findings' in the cache key, but carefully + # https://gitlab.com/gitlab-org/gitlab/issues/32963 ['projects', project_id, 'vulnerabilities'] end end diff --git a/ee/spec/lib/gitlab/vulnerabilities/history_spec.rb b/ee/spec/lib/gitlab/vulnerabilities/history_spec.rb index 6fee0f88610cc2..160816591cb66a 100644 --- a/ee/spec/lib/gitlab/vulnerabilities/history_spec.rb +++ b/ee/spec/lib/gitlab/vulnerabilities/history_spec.rb @@ -13,8 +13,8 @@ create_vulnerabilities(2, project2, { severity: :high, report_type: :sast }) end - describe '#vulnerabilities_counter', :use_clean_rails_memory_store_caching do - subject(:counter) { described_class.new(group, filters).vulnerabilities_counter } + describe '#findings_counter', :use_clean_rails_memory_store_caching do + subject(:counter) { described_class.new(group, filters).findings_counter } context 'feature disabled' do before do -- GitLab From ae80e0bed456df5df26548bd910a8e54c0b86156 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 22:33:46 +0300 Subject: [PATCH 07/27] Fix wording in test description --- ee/spec/finders/security/vulnerabilities_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/finders/security/vulnerabilities_finder_spec.rb b/ee/spec/finders/security/vulnerabilities_finder_spec.rb index 70b12eaa103043..372bfa16e3b5c7 100644 --- a/ee/spec/finders/security/vulnerabilities_finder_spec.rb +++ b/ee/spec/finders/security/vulnerabilities_finder_spec.rb @@ -7,7 +7,7 @@ subject { described_class.new(project).execute } - it 'returns vulnerabilities of a project and respects pagination params' do + it 'returns vulnerabilities of a project' do expect(subject).to match_array(project.vulnerabilities) end end -- GitLab From 83f135baea287deb6b56a732695b64819972623f Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 22:34:08 +0300 Subject: [PATCH 08/27] Reduce vulnerabilities count in trait to 2 --- ee/spec/factories/projects.rb | 2 +- ee/spec/requests/api/vulnerabilities_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/factories/projects.rb b/ee/spec/factories/projects.rb index f3a8bc309f962e..2c97a617acae53 100644 --- a/ee/spec/factories/projects.rb +++ b/ee/spec/factories/projects.rb @@ -92,7 +92,7 @@ trait :with_vulnerabilities do after(:create) do |project| - create_list(:vulnerability, 3, :opened, project: project) + create_list(:vulnerability, 2, :opened, project: project) end end end diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index 93bdd635455915..f5c05b4befec48 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -31,7 +31,7 @@ get api("#{project_vulnerabilities_path}?page=2&per_page=1", user) expect(response).to have_gitlab_http_status(200) - expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.drop(1).take(1).first.id) + expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.second.id) end end -- GitLab From 40292392a08c1b4b88f72af22a525716551d3bb1 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 22:35:01 +0300 Subject: [PATCH 09/27] Improve code and comments in shared examples --- .../api/vulnerabilities_shared_examples.rb | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) 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 f213bd58eb9956..6e1e042dd31870 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 @@ -83,10 +83,16 @@ project.add_developer(user) end + # Because fixture reports that power :ee_ci_job_artifact factory contain long report lists, + # we need to make sure that all occurrences for both SAST and Dependency Scanning are included in the response. + # That's why the page size is 40. + let(:pagination) { { per_page: 40 } } + it 'returns all non-dismissed vulnerabilities' do + # all occurrences except one that was dismissed occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s - get api(project_vulnerabilities_path, user), params: { per_page: 40 } + get api(project_vulnerabilities_path, user), params: pagination expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -107,7 +113,7 @@ describe 'filtering' do it 'returns vulnerabilities with sast report_type' do - occurrence_count = (sast_report.occurrences.count - 1).to_s + occurrence_count = (sast_report.occurrences.count - 1).to_s # all SAST occurrences except one that was dismissed get api(project_vulnerabilities_path, user), params: { report_type: 'sast' } @@ -147,7 +153,7 @@ it 'returns dismissed vulnerabilities with `all` scope' do occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count).to_s - get api(project_vulnerabilities_path, user), params: { per_page: 40, scope: 'all' } + get api(project_vulnerabilities_path, user), params: { scope: 'all' }.merge(pagination) expect(response).to have_gitlab_http_status(200) @@ -155,7 +161,7 @@ end it 'returns vulnerabilities with low severity' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, severity: 'low' } + get api(project_vulnerabilities_path, user), params: { severity: 'low' }.merge(pagination) expect(response).to have_gitlab_http_status(200) @@ -169,7 +175,7 @@ end it 'returns vulnerabilities with high confidence' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, confidence: 'high' } + get api(project_vulnerabilities_path, user), params: { confidence: 'high' }.merge(pagination) expect(response).to have_gitlab_http_status(200) @@ -186,7 +192,7 @@ it 'returns vulnerabilities from supplied pipeline' do occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline.id } + get api(project_vulnerabilities_path, user), params: { pipeline_id: pipeline.id }.merge(pagination) expect(response).to have_gitlab_http_status(200) @@ -195,7 +201,7 @@ context 'pipeline has no reports' do it 'returns empty results' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline_without_vulnerabilities.id } + get api(project_vulnerabilities_path, user), params: { pipeline_id: pipeline_without_vulnerabilities.id }.merge(pagination) expect(json_response).to eq [] end @@ -203,7 +209,7 @@ context 'with unknown pipeline' do it 'returns empty results' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: 0 } + get api(project_vulnerabilities_path, user), params: { pipeline_id: 0 }.merge(pagination) expect(json_response).to eq [] end -- GitLab From 52eb84bb360eb722fafdc6d5a554ef601d356323 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 16:07:24 +0300 Subject: [PATCH 10/27] Simplify API tests --- ee/spec/requests/api/vulnerabilities_spec.rb | 14 +++++++------- .../requests/api/vulnerability_findings_spec.rb | 17 ++++++++++++----- .../api/vulnerabilities_shared_examples.rb | 2 -- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index f5c05b4befec48..c93767ba603fcb 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -33,16 +33,16 @@ expect(response).to have_gitlab_http_status(200) expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.second.id) end - end - it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' + context 'when "first-class vulnerabilities" feature is disabled' do + before do + stub_feature_flags(first_class_vulnerabilities: false) + end - context 'when "first-class vulnerabilities" feature is disabled' do - before do - stub_feature_flags(first_class_vulnerabilities: false) + it_behaves_like 'getting list of vulnerability findings' end - - it_behaves_like 'getting list of vulnerability findings' end + + it_behaves_like 'forbids access to vulnerability-like 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 112e8f0d278333..0c0141bdc29ec8 100644 --- a/ee/spec/requests/api/vulnerability_findings_spec.rb +++ b/ee/spec/requests/api/vulnerability_findings_spec.rb @@ -10,16 +10,23 @@ 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' - context 'when "first-class vulnerabilities" feature is disabled' do + context 'with an authorized user with proper permissions' do before do - stub_feature_flags(first_class_vulnerabilities: false) + project.add_developer(user) end - it 'responds with "not found"' do - get api(project_vulnerabilities_path, user) + context 'when "first-class vulnerabilities" feature is disabled' do + before do + stub_feature_flags(first_class_vulnerabilities: false) + end - expect(response).to have_gitlab_http_status(404) + it 'responds with "not found"' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(404) + end 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 6e1e042dd31870..f71cc233c9c368 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 @@ -217,6 +217,4 @@ end end end - - it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' end -- GitLab From accb18469431fd530fafd63a2fc8c9a8510a2b32 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 16:41:09 +0000 Subject: [PATCH 11/27] Apply suggestion to ee/lib/api/helpers/vulnerability_findings_helpers.rb --- ee/lib/api/helpers/vulnerability_findings_helpers.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/lib/api/helpers/vulnerability_findings_helpers.rb b/ee/lib/api/helpers/vulnerability_findings_helpers.rb index 0f43e21a9896af..9398655bbe9870 100644 --- a/ee/lib/api/helpers/vulnerability_findings_helpers.rb +++ b/ee/lib/api/helpers/vulnerability_findings_helpers.rb @@ -13,7 +13,7 @@ module VulnerabilityFindingsHelpers default: 'dismissed', values: %w[all dismissed] optional :severity, type: Array[String], - desc: 'Returns issues belonging to specified severity level: '\ + desc: 'Returns vulnerabilities belonging to specified severity level: '\ '`undefined`, `info`, `unknown`, `low`, `medium`, `high`, or `critical`. Defaults to all', values: ::Vulnerabilities::Occurrence.severities.keys, default: ::Vulnerabilities::Occurrence.severities.keys -- GitLab From a8482a92f7f7cdd1a4b329597775c960a4cf481c Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Mon, 7 Oct 2019 21:01:16 +0300 Subject: [PATCH 12/27] Add VulnerabilityEntity w/ JSON schema tests --- ee/app/serializers/vulnerability_entity.rb | 26 +++++++++ .../fixtures/api/schemas/vulnerability.json | 53 +++++++++++++++++++ .../serializers/vulnerability_entity_spec.rb | 17 ++++++ 3 files changed, 96 insertions(+) create mode 100644 ee/app/serializers/vulnerability_entity.rb create mode 100644 ee/spec/fixtures/api/schemas/vulnerability.json create mode 100644 ee/spec/serializers/vulnerability_entity_spec.rb diff --git a/ee/app/serializers/vulnerability_entity.rb b/ee/app/serializers/vulnerability_entity.rb new file mode 100644 index 00000000000000..abfeaafc46c51d --- /dev/null +++ b/ee/app/serializers/vulnerability_entity.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class VulnerabilityEntity < Grape::Entity + expose :id + expose :title + expose :description + + expose :state + expose :severity + expose :confidence + + expose :project, using: ::ProjectEntity + + expose :author_id + expose :updated_by_id + expose :last_edited_by_id + expose :closed_by_id + + expose :start_date + expose :due_date + + expose :created_at + expose :updated_at + expose :last_edited_at + expose :closed_at +end diff --git a/ee/spec/fixtures/api/schemas/vulnerability.json b/ee/spec/fixtures/api/schemas/vulnerability.json new file mode 100644 index 00000000000000..703c3a0097edb2 --- /dev/null +++ b/ee/spec/fixtures/api/schemas/vulnerability.json @@ -0,0 +1,53 @@ +{ + "type": "object", + "required": ["title", "state", "confidence", "severity", "project", "author_id"], + "properties": { + "title": { + "type": "string" + }, + "description": { "type": ["string", "null"] }, + "state": { "type": "string", "enum": ["opened", "closed"] }, + "severity": { + "type": "string", + "enum": ["undefined", "info", "unknown", "low", "medium", "high", "critical"] + }, + "confidence": { + "type": "string", + "enum": [ + "undefined", + "ignore", + "unknown", + "experimental", + "low", + "medium", + "high", + "confirmed" + ] + }, + "project": { + "required": ["id", "name", "full_path", "full_name"], + "id": { + "type": "integer" + }, + "name": { + "type": "string" + }, + "full_path": { + "type": "string" + }, + "full_name": { + "type": "string" + } + }, + "author_id": { "type": "integer" }, + "updated_by_id": { "type": ["integer", "null"] }, + "last_edited_by_id": { "type": ["integer", "null"] }, + "closed_by_id": { "type": ["integer", "null"] }, + "start_date": { "type": ["date", "null"] }, + "due_date": { "type": ["date", "null"] }, + "created_at": { "type": "date" }, + "updated_at": { "type": "date" }, + "last_edited_at": { "type": "date" }, + "closed_at": { "type": "date" } + } +} diff --git a/ee/spec/serializers/vulnerability_entity_spec.rb b/ee/spec/serializers/vulnerability_entity_spec.rb new file mode 100644 index 00000000000000..4d422955b12fba --- /dev/null +++ b/ee/spec/serializers/vulnerability_entity_spec.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe VulnerabilityEntity do + let(:vulnerability) do + create(:vulnerability) + end + + let(:entity) do + described_class.represent(vulnerability) + end + + subject { entity.to_json } + + it { is_expected.to match_schema('vulnerability', dir: 'ee') } +end -- GitLab From 8e2844e4972db8af9a59301a52f13f08937f3adf Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 8 Oct 2019 15:51:40 +0300 Subject: [PATCH 13/27] Add VulnerabilitiesFinder for new Vulnerability Rename old VulnerabilitiesFinder to VulnerabilityFindingsFinder because it finds Occurrences (Findings in new terminology) and update its usages. Add new VulnerabilitiesFinder that finds Vulnerabilities of a Project and respects the pagination of results. --- .../vulnerability_findings_actions.rb | 2 +- .../security/vulnerabilities_finder.rb | 72 ++-------- .../security/vulnerability_findings_finder.rb | 78 ++++++++++ ee/lib/gitlab/vulnerabilities/history.rb | 2 +- .../gitlab/vulnerabilities/history_cache.rb | 2 +- ee/spec/factories/projects.rb | 6 + .../security/vulnerabilities_finder_spec.rb | 130 +---------------- .../vulnerability_findings_finder_spec.rb | 134 ++++++++++++++++++ 8 files changed, 238 insertions(+), 188 deletions(-) create mode 100644 ee/app/finders/security/vulnerability_findings_finder.rb create mode 100644 ee/spec/finders/security/vulnerability_findings_finder_spec.rb diff --git a/ee/app/controllers/concerns/vulnerability_findings_actions.rb b/ee/app/controllers/concerns/vulnerability_findings_actions.rb index 613d719f9a7020..1e3cb97dbb25c9 100644 --- a/ee/app/controllers/concerns/vulnerability_findings_actions.rb +++ b/ee/app/controllers/concerns/vulnerability_findings_actions.rb @@ -40,6 +40,6 @@ def filter_params end def vulnerability_findings(collection = :latest) - ::Security::VulnerabilitiesFinder.new(vulnerable, params: filter_params).execute(collection) + ::Security::VulnerabilityFindingsFinder.new(vulnerable, params: filter_params).execute(collection) end end diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb index ef10d00b408a4c..efc2cd53a9760d 100644 --- a/ee/app/finders/security/vulnerabilities_finder.rb +++ b/ee/app/finders/security/vulnerabilities_finder.rb @@ -2,75 +2,27 @@ # Security::VulnerabilitiesFinder # -# Used to filter Vulnerabilities::Occurrences by set of params for Security Dashboard +# Used to filter Vulnerability record by set of params for Vulnerabilities API # # Arguments: -# vulnerable - object to filter vulnerabilities +# project: a Project to query for Vulnerabilities # params: -# severity: Array -# confidence: Array -# project: Array -# report_type: Array +# page: Integer +# per_page: Integer module Security class VulnerabilitiesFinder - attr_accessor :params - attr_reader :vulnerable + attr_reader :project + attr_reader :page, :per_page - def initialize(vulnerable, params: {}) - @vulnerable = vulnerable - @params = params + def initialize(project, params = {}) + @project = project + @page = params[:page] || 1 + @per_page = params[:per_page] || 20 end - def execute(scope = :latest) - collection = init_collection(scope) - collection = by_report_type(collection) - collection = by_project(collection) - collection = by_severity(collection) - collection = by_confidence(collection) - collection - end - - private - - def by_report_type(items) - return items unless params[:report_type].present? - - items.by_report_types( - Vulnerabilities::Occurrence::REPORT_TYPES.values_at( - *params[:report_type]).compact) - end - - def by_project(items) - return items unless params[:project_id].present? - - items.by_projects(params[:project_id]) - end - - def by_severity(items) - return items unless params[:severity].present? - - items.by_severities( - Vulnerabilities::Occurrence::SEVERITY_LEVELS.values_at( - *params[:severity]).compact) - end - - def by_confidence(items) - return items unless params[:confidence].present? - - items.by_confidences( - Vulnerabilities::Occurrence::CONFIDENCE_LEVELS.values_at( - *params[:confidence]).compact) - end - - def init_collection(scope) - if scope == :all - vulnerable.all_vulnerabilities - elsif scope == :with_sha - vulnerable.latest_vulnerabilities_with_sha - else - vulnerable.latest_vulnerabilities - end + def execute + project.vulnerabilities.page(page).per(per_page) end end end diff --git a/ee/app/finders/security/vulnerability_findings_finder.rb b/ee/app/finders/security/vulnerability_findings_finder.rb new file mode 100644 index 00000000000000..5dc019c565444c --- /dev/null +++ b/ee/app/finders/security/vulnerability_findings_finder.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +# Security::VulnerabilityFindingsFinder +# +# Used to filter Vulnerabilities::Occurrences by set of params for Security Dashboard +# +# Arguments: +# vulnerable - object to filter vulnerabilities +# params: +# severity: Array +# confidence: Array +# project: Array +# report_type: Array + +module Security + # TODO: check usages and rename `vulnerable` associations to mention the term "finding"; + # probably, add new changes to https://gitlab.com/gitlab-org/gitlab/merge_requests/17351 + class VulnerabilityFindingsFinder + attr_accessor :params + attr_reader :vulnerable + + def initialize(vulnerable, params: {}) + @vulnerable = vulnerable + @params = params + end + + def execute(scope = :latest) + collection = init_collection(scope) + collection = by_report_type(collection) + collection = by_project(collection) + collection = by_severity(collection) + collection = by_confidence(collection) + collection + end + + private + + def by_report_type(items) + return items unless params[:report_type].present? + + items.by_report_types( + Vulnerabilities::Occurrence::REPORT_TYPES.values_at( + *params[:report_type]).compact) + end + + def by_project(items) + return items unless params[:project_id].present? + + items.by_projects(params[:project_id]) + end + + def by_severity(items) + return items unless params[:severity].present? + + items.by_severities( + Vulnerabilities::Occurrence::SEVERITY_LEVELS.values_at( + *params[:severity]).compact) + end + + def by_confidence(items) + return items unless params[:confidence].present? + + items.by_confidences( + Vulnerabilities::Occurrence::CONFIDENCE_LEVELS.values_at( + *params[:confidence]).compact) + end + + def init_collection(scope) + if scope == :all + vulnerable.all_vulnerabilities + elsif scope == :with_sha + vulnerable.latest_vulnerabilities_with_sha + else + vulnerable.latest_vulnerabilities + end + end + end +end diff --git a/ee/lib/gitlab/vulnerabilities/history.rb b/ee/lib/gitlab/vulnerabilities/history.rb index f0eb7882ed8494..0951617d9e9fbd 100644 --- a/ee/lib/gitlab/vulnerabilities/history.rb +++ b/ee/lib/gitlab/vulnerabilities/history.rb @@ -24,7 +24,7 @@ def vulnerabilities_counter private def found_vulnerabilities - ::Security::VulnerabilitiesFinder.new(group, params: filters).execute(:all) + ::Security::VulnerabilityFindingsFinder.new(group, params: filters).execute(:all) end def cached_vulnerability_history diff --git a/ee/lib/gitlab/vulnerabilities/history_cache.rb b/ee/lib/gitlab/vulnerabilities/history_cache.rb index 236420da8bc33a..1a5a55696e91e9 100644 --- a/ee/lib/gitlab/vulnerabilities/history_cache.rb +++ b/ee/lib/gitlab/vulnerabilities/history_cache.rb @@ -12,7 +12,7 @@ def initialize(group, project_id) def fetch(range, force: false) Rails.cache.fetch(cache_key, force: force, expires_in: 1.day) do - vulnerabilities = ::Security::VulnerabilitiesFinder + vulnerabilities = ::Security::VulnerabilityFindingsFinder .new(group, params: { project_id: [project_id] }) .execute(:all) .count_by_day_and_severity(range) diff --git a/ee/spec/factories/projects.rb b/ee/spec/factories/projects.rb index 2db74a89a6d05c..f3a8bc309f962e 100644 --- a/ee/spec/factories/projects.rb +++ b/ee/spec/factories/projects.rb @@ -89,5 +89,11 @@ trait :github_imported do import_type { 'github' } end + + trait :with_vulnerabilities do + after(:create) do |project| + create_list(:vulnerability, 3, :opened, project: project) + end + end end end diff --git a/ee/spec/finders/security/vulnerabilities_finder_spec.rb b/ee/spec/finders/security/vulnerabilities_finder_spec.rb index 17a61c311ba7ea..62710f9cacf5ef 100644 --- a/ee/spec/finders/security/vulnerabilities_finder_spec.rb +++ b/ee/spec/finders/security/vulnerabilities_finder_spec.rb @@ -3,132 +3,12 @@ require 'spec_helper' describe Security::VulnerabilitiesFinder do - describe '#execute' do - set(:group) { create(:group) } - set(:project1) { create(:project, :private, :repository, group: group) } - set(:project2) { create(:project, :private, :repository, group: group) } - set(:pipeline1) { create(:ci_pipeline, :success, project: project1) } - set(:pipeline2) { create(:ci_pipeline, :success, project: project2) } + let(:project) { create(:project, :with_vulnerabilities) } + let(:params) { { page: 2, per_page: 1 } } - set(:vulnerability1) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :high, confidence: :high, pipelines: [pipeline1], project: project1) } - set(:vulnerability2) { create(:vulnerabilities_occurrence, report_type: :dependency_scanning, severity: :medium, confidence: :low, pipelines: [pipeline2], project: project2) } - set(:vulnerability3) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :low, pipelines: [pipeline2], project: project2) } - set(:vulnerability4) { create(:vulnerabilities_occurrence, report_type: :dast, severity: :medium, pipelines: [pipeline1], project: project1) } + subject { described_class.new(project, params).execute } - subject { described_class.new(group, params: params).execute } - - context 'by report type' do - context 'when sast' do - let(:params) { { report_type: %w[sast] } } - - it 'includes only sast' do - is_expected.to contain_exactly(vulnerability1, vulnerability3) - end - end - - context 'when dependency_scanning' do - let(:params) { { report_type: %w[dependency_scanning] } } - - it 'includes only depscan' do - is_expected.to contain_exactly(vulnerability2) - end - end - end - - context 'by severity' do - context 'when high' do - let(:params) { { severity: %w[high] } } - - it 'includes only high' do - is_expected.to contain_exactly(vulnerability1) - end - end - - context 'when medium' do - let(:params) { { severity: %w[medium] } } - - it 'includes only medium' do - is_expected.to contain_exactly(vulnerability2, vulnerability4) - end - end - end - - context 'by confidence' do - context 'when high' do - let(:params) { { confidence: %w[high] } } - - it 'includes only high confidence vulnerabilities' do - is_expected.to contain_exactly(vulnerability1) - end - end - - context 'when low' do - let(:params) { { confidence: %w[low] } } - - it 'includes only low confidence vulnerabilities' do - is_expected.to contain_exactly(vulnerability2) - end - end - end - - context 'by project' do - let(:params) { { project_id: [project2.id] } } - - it 'includes only vulnerabilities for one project' do - is_expected.to contain_exactly(vulnerability2, vulnerability3) - end - end - - # FIXME: unskip when this filter is implemented - context 'by dismissals' do - let!(:dismissal) do - create(:vulnerability_feedback, :sast, :dismissal, - pipeline: pipeline1, - project: project1, - project_fingerprint: vulnerability1.project_fingerprint) - end - - let(:params) { { hide_dismissed: true } } - - skip 'exclude dismissal' do - is_expected.to contain_exactly(vulnerability2, vulnerability3, vulnerability4) - end - end - - context 'by all filters' do - context 'with found entity' do - let(:params) { { severity: %w[high medium low], project_id: [project1.id, project2.id], report_type: %w[sast dast] } } - - it 'filters by all params' do - is_expected.to contain_exactly(vulnerability1, vulnerability3, vulnerability4) - end - end - - context 'without found entity' do - let(:params) { { severity: %w[low], project_id: [project1.id], report_type: %w[sast] } } - - it 'did not find anything' do - is_expected.to be_empty - end - end - end - - context 'by some filters' do - context 'with found entity' do - let(:params) { { project_id: [project2.id], severity: %w[medium low] } } - - it 'filters by all params' do - is_expected.to contain_exactly(vulnerability2, vulnerability3) - end - end - - context 'without found entity' do - let(:params) { { project_id: project1.id, severity: %w[low] } } - - it 'did not find anything' do - is_expected.to be_empty - end - end - end + it 'returns vulnerabilities of a project and respects pagination params' do + expect(subject).to contain_exactly(project.vulnerabilities.drop(1).take(1).first) end end diff --git a/ee/spec/finders/security/vulnerability_findings_finder_spec.rb b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb new file mode 100644 index 00000000000000..3edee7d6f31db1 --- /dev/null +++ b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Security::VulnerabilityFindingsFinder do + describe '#execute' do + set(:group) { create(:group) } + set(:project1) { create(:project, :private, :repository, group: group) } + set(:project2) { create(:project, :private, :repository, group: group) } + set(:pipeline1) { create(:ci_pipeline, :success, project: project1) } + set(:pipeline2) { create(:ci_pipeline, :success, project: project2) } + + set(:vulnerability1) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :high, confidence: :high, pipelines: [pipeline1], project: project1) } + set(:vulnerability2) { create(:vulnerabilities_occurrence, report_type: :dependency_scanning, severity: :medium, confidence: :low, pipelines: [pipeline2], project: project2) } + set(:vulnerability3) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :low, pipelines: [pipeline2], project: project2) } + set(:vulnerability4) { create(:vulnerabilities_occurrence, report_type: :dast, severity: :medium, pipelines: [pipeline1], project: project1) } + + subject { described_class.new(group, params: params).execute } + + context 'by report type' do + context 'when sast' do + let(:params) { { report_type: %w[sast] } } + + it 'includes only sast' do + is_expected.to contain_exactly(vulnerability1, vulnerability3) + end + end + + context 'when dependency_scanning' do + let(:params) { { report_type: %w[dependency_scanning] } } + + it 'includes only depscan' do + is_expected.to contain_exactly(vulnerability2) + end + end + end + + context 'by severity' do + context 'when high' do + let(:params) { { severity: %w[high] } } + + it 'includes only high' do + is_expected.to contain_exactly(vulnerability1) + end + end + + context 'when medium' do + let(:params) { { severity: %w[medium] } } + + it 'includes only medium' do + is_expected.to contain_exactly(vulnerability2, vulnerability4) + end + end + end + + context 'by confidence' do + context 'when high' do + let(:params) { { confidence: %w[high] } } + + it 'includes only high confidence vulnerabilities' do + is_expected.to contain_exactly(vulnerability1) + end + end + + context 'when low' do + let(:params) { { confidence: %w[low] } } + + it 'includes only low confidence vulnerabilities' do + is_expected.to contain_exactly(vulnerability2) + end + end + end + + context 'by project' do + let(:params) { { project_id: [project2.id] } } + + it 'includes only vulnerabilities for one project' do + is_expected.to contain_exactly(vulnerability2, vulnerability3) + end + end + + # FIXME: unskip when this filter is implemented + context 'by dismissals' do + let!(:dismissal) do + create(:vulnerability_feedback, :sast, :dismissal, + pipeline: pipeline1, + project: project1, + project_fingerprint: vulnerability1.project_fingerprint) + end + + let(:params) { { hide_dismissed: true } } + + skip 'exclude dismissal' do + is_expected.to contain_exactly(vulnerability2, vulnerability3, vulnerability4) + end + end + + context 'by all filters' do + context 'with found entity' do + let(:params) { { severity: %w[high medium low], project_id: [project1.id, project2.id], report_type: %w[sast dast] } } + + it 'filters by all params' do + is_expected.to contain_exactly(vulnerability1, vulnerability3, vulnerability4) + end + end + + context 'without found entity' do + let(:params) { { severity: %w[low], project_id: [project1.id], report_type: %w[sast] } } + + it 'did not find anything' do + is_expected.to be_empty + end + end + end + + context 'by some filters' do + context 'with found entity' do + let(:params) { { project_id: [project2.id], severity: %w[medium low] } } + + it 'filters by all params' do + is_expected.to contain_exactly(vulnerability2, vulnerability3) + end + end + + context 'without found entity' do + let(:params) { { project_id: project1.id, severity: %w[low] } } + + it 'did not find anything' do + is_expected.to be_empty + end + end + end + end +end -- GitLab From d75b57237d185a316d2d26d69f7ba1a3db0faa56 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 8 Oct 2019 15:54:16 +0300 Subject: [PATCH 14/27] Introduce GET Project's Vulnerabilities API call Extract Vulnerabilities API into a separate module and implement GET projects/:id/vulnerabilities API operation. Respect :first_class_vulnerabilities feature flag. --- .../helpers/vulnerability_findings_helpers.rb | 62 +++++ ee/lib/api/vulnerabilities.rb | 49 ++++ ee/lib/api/vulnerability_findings.rb | 74 +----- ee/lib/ee/api/api.rb | 1 + .../api/schemas/vulnerability_list.json | 4 + ee/spec/requests/api/vulnerabilities_spec.rb | 41 ++++ .../api/vulnerability_findings_spec.rb | 229 +----------------- .../api/vulnerabilities_shared_examples.rb | 216 +++++++++++++++++ 8 files changed, 382 insertions(+), 294 deletions(-) create mode 100644 ee/lib/api/helpers/vulnerability_findings_helpers.rb create mode 100644 ee/lib/api/vulnerabilities.rb create mode 100644 ee/spec/fixtures/api/schemas/vulnerability_list.json create mode 100644 ee/spec/requests/api/vulnerabilities_spec.rb create mode 100644 ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb diff --git a/ee/lib/api/helpers/vulnerability_findings_helpers.rb b/ee/lib/api/helpers/vulnerability_findings_helpers.rb new file mode 100644 index 00000000000000..0f43e21a9896af --- /dev/null +++ b/ee/lib/api/helpers/vulnerability_findings_helpers.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module API + module Helpers + module VulnerabilityFindingsHelpers + extend Grape::API::Helpers + + params :vulnerability_findings_params do + optional :report_type, type: Array[String], desc: 'The type of report vulnerability belongs to', + values: ::Vulnerabilities::Occurrence.report_types.keys, + default: ::Vulnerabilities::Occurrence.report_types.keys + optional :scope, type: String, desc: 'Return vulnerabilities for the given scope: `dismissed` or `all`', + default: 'dismissed', values: %w[all dismissed] + optional :severity, + type: Array[String], + desc: 'Returns issues belonging to specified severity level: '\ + '`undefined`, `info`, `unknown`, `low`, `medium`, `high`, or `critical`. Defaults to all', + values: ::Vulnerabilities::Occurrence.severities.keys, + default: ::Vulnerabilities::Occurrence.severities.keys + optional :confidence, + type: Array[String], + desc: 'Returns vulnerabilities belonging to specified confidence level: '\ + '`undefined`, `ignore`, `unknown`, `experimental`, `low`, `medium`, `high`, or `confirmed`. '\ + 'Defaults to all', + values: ::Vulnerabilities::Occurrence.confidences.keys, + default: ::Vulnerabilities::Occurrence.confidences.keys + optional :pipeline_id, type: String, desc: 'The ID of the pipeline' + + use :pagination + end + + # TODO: rename to vulnerability_findings_by https://gitlab.com/gitlab-org/gitlab/issues/32963 + def vulnerability_occurrences_by(params) + pipeline = if params[:pipeline_id] + params[:project].all_pipelines.find_by(id: params[:pipeline_id]) # rubocop:disable CodeReuse/ActiveRecord + else + params[:project].latest_pipeline_with_security_reports + end + + return [] unless pipeline + + Security::PipelineVulnerabilitiesFinder.new(pipeline: pipeline, params: params).execute + end + + def respond_with_vulnerability_findings + authorize! :read_project_security_dashboard, user_project + + vulnerability_occurrences = paginate( + Kaminari.paginate_array( + vulnerability_occurrences_by(declared_params.merge(project: user_project)) + ) + ) + + Gitlab::Vulnerabilities::OccurrencesPreloader.preload_feedback!(vulnerability_occurrences) + + present vulnerability_occurrences, + with: ::Vulnerabilities::OccurrenceEntity, + request: GrapeRequestProxy.new(request, current_user) + end + end + end +end diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb new file mode 100644 index 00000000000000..83838ee4902afe --- /dev/null +++ b/ee/lib/api/vulnerabilities.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module API + class Vulnerabilities < Grape::API + include PaginationParams + + helpers ::API::Helpers::VulnerabilityFindingsHelpers + + helpers do + def vulnerabilities_by(project, params) + Security::VulnerabilitiesFinder.new(project, params).execute + end + end + + before do + authenticate! + 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 + # responding with Vulnerability Findings when :first_class_vulnerabilities feature is disabled. + # TODO: remove usage of :vulnerability_findings_params when feature flag is removed + # https://gitlab.com/gitlab-org/gitlab/issues/33488 + use :vulnerability_findings_params + end + desc 'Get a list of project vulnerabilities' do + success VulnerabilityEntity + end + get ':id/vulnerabilities' do + if Feature.enabled?(:first_class_vulnerabilities) + authorize! :read_project_security_dashboard, user_project + + vulnerabilities = paginate( + vulnerabilities_by(user_project, declared_params) + ) + + present vulnerabilities, with: VulnerabilityEntity + else + respond_with_vulnerability_findings + end + end + end + end +end diff --git a/ee/lib/api/vulnerability_findings.rb b/ee/lib/api/vulnerability_findings.rb index 5322d1cafce016..2dbcd0eb3091d8 100644 --- a/ee/lib/api/vulnerability_findings.rb +++ b/ee/lib/api/vulnerability_findings.rb @@ -4,65 +4,7 @@ module API class VulnerabilityFindings < Grape::API include PaginationParams - helpers do - params :vulnerability_findings_params do - optional :report_type, type: Array[String], desc: 'The type of report vulnerability belongs to', - values: ::Vulnerabilities::Occurrence.report_types.keys, - default: ::Vulnerabilities::Occurrence.report_types.keys - optional :scope, type: String, desc: 'Return vulnerabilities for the given scope: `dismissed` or `all`', - default: 'dismissed', values: %w[all dismissed] - optional :severity, - type: Array[String], - desc: 'Returns issues belonging to specified severity level: '\ - '`undefined`, `info`, `unknown`, `low`, `medium`, `high`, or `critical`. Defaults to all', - values: ::Vulnerabilities::Occurrence.severities.keys, - default: ::Vulnerabilities::Occurrence.severities.keys - optional :confidence, - type: Array[String], - desc: 'Returns vulnerabilities belonging to specified confidence level: '\ - '`undefined`, `ignore`, `unknown`, `experimental`, `low`, `medium`, `high`, or `confirmed`. '\ - 'Defaults to all', - values: ::Vulnerabilities::Occurrence.confidences.keys, - default: ::Vulnerabilities::Occurrence.confidences.keys - optional :pipeline_id, type: String, desc: 'The ID of the pipeline' - - use :pagination - end - - def vulnerability_occurrences_by(params) - pipeline = if params[:pipeline_id] - params[:project].all_pipelines.find_by(id: params[:pipeline_id]) # rubocop:disable CodeReuse/ActiveRecord - else - params[:project].latest_pipeline_with_security_reports - end - - return [] unless pipeline - - Security::PipelineVulnerabilitiesFinder.new(pipeline: pipeline, params: params).execute - end - - def respond_with_vulnerabilities - # TODO: implement the "Get a list of project's Vulnerabilities" step - # of https://gitlab.com/gitlab-org/gitlab-ee/issues/10242#status - not_found! - end - - def respond_with_vulnerability_findings - authorize! :read_project_security_dashboard, user_project - - vulnerability_occurrences = paginate( - Kaminari.paginate_array( - vulnerability_occurrences_by(declared_params.merge(project: user_project)) - ) - ) - - Gitlab::Vulnerabilities::OccurrencesPreloader.preload_feedback!(vulnerability_occurrences) - - present vulnerability_occurrences, - with: ::Vulnerabilities::OccurrenceEntity, - request: GrapeRequestProxy.new(request, current_user) - end - end + helpers ::API::Helpers::VulnerabilityFindingsHelpers before do authenticate! @@ -73,20 +15,6 @@ def respond_with_vulnerability_findings end resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do - params do - use :vulnerability_findings_params - end - desc 'Get a list of project vulnerabilities' do - success ::Vulnerabilities::OccurrenceEntity - end - get ':id/vulnerabilities' do - if Feature.enabled?(:first_class_vulnerabilities) - respond_with_vulnerabilities - else - respond_with_vulnerability_findings - end - end - params do use :vulnerability_findings_params end diff --git a/ee/lib/ee/api/api.rb b/ee/lib/ee/api/api.rb index bc7de4f1e07a31..375dd0a67f13bf 100644 --- a/ee/lib/ee/api/api.rb +++ b/ee/lib/ee/api/api.rb @@ -35,6 +35,7 @@ module API mount ::API::Scim mount ::API::ManagedLicenses mount ::API::ProjectApprovals + mount ::API::Vulnerabilities mount ::API::VulnerabilityFindings mount ::API::MergeRequestApprovals mount ::API::MergeRequestApprovalRules diff --git a/ee/spec/fixtures/api/schemas/vulnerability_list.json b/ee/spec/fixtures/api/schemas/vulnerability_list.json new file mode 100644 index 00000000000000..430ac7a386bbfb --- /dev/null +++ b/ee/spec/fixtures/api/schemas/vulnerability_list.json @@ -0,0 +1,4 @@ +{ + "type": "array", + "items": { "$ref": "vulnerability.json" } +} diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb new file mode 100644 index 00000000000000..2c2e12144a6a2b --- /dev/null +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe API::Vulnerabilities do + before do + stub_licensed_features(security_dashboard: true) + end + + let_it_be(:project) { create(:project, :public, :with_vulnerabilities) } + let_it_be(:user) { create(:user) } + + describe "GET /projects/:id/vulnerabilities" do + let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerabilities" } + + context 'with an authorized user with proper permissions' do + before do + project.add_developer(user) + end + + it 'returns all vulnerabilities of a project' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(response).to match_response_schema('vulnerability_list', dir: 'ee') + expect(response.headers['X-Total']).to eq project.vulnerabilities.count.to_s + end + end + + it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' + + context 'when "first-class vulnerabilities" feature is disabled' do + before do + stub_feature_flags(first_class_vulnerabilities: false) + end + + it_behaves_like 'getting list of vulnerability findings' + end + end +end diff --git a/ee/spec/requests/api/vulnerability_findings_spec.rb b/ee/spec/requests/api/vulnerability_findings_spec.rb index d10c3a712342ff..112e8f0d278333 100644 --- a/ee/spec/requests/api/vulnerability_findings_spec.rb +++ b/ee/spec/requests/api/vulnerability_findings_spec.rb @@ -3,237 +3,24 @@ require 'spec_helper' describe API::VulnerabilityFindings do - set(:project) { create(:project, :public) } - set(:user) { create(:user) } + let_it_be(:project) { create(:project, :public) } + let_it_be(:user) { create(:user) } - let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) } - let(:pipeline_without_vulnerabilities) { create(:ci_pipeline_without_jobs, status: :created, project: project) } - - let(:build_ds) { create(:ci_build, :success, name: 'ds_job', pipeline: pipeline, project: project) } - let(:build_sast) { create(:ci_build, :success, name: 'sast_job', pipeline: pipeline, project: project) } - - let(:ds_report) { pipeline.security_reports.reports["dependency_scanning"] } - let(:sast_report) { pipeline.security_reports.reports["sast"] } - - let(:dismissal) do - create(:vulnerability_feedback, :dismissal, :sast, - project: project, - pipeline: pipeline, - project_fingerprint: sast_report.occurrences.first.project_fingerprint, - vulnerability_data: sast_report.occurrences.first.raw_metadata - ) - end - - before do - stub_licensed_features(security_dashboard: true, sast: true, dependency_scanning: true, container_scanning: true) - - create(:ee_ci_job_artifact, :dependency_scanning, job: build_ds, project: project) - create(:ee_ci_job_artifact, :sast, job: build_sast, project: project) - dismissal - end - - shared_examples 'getting list of vulnerability findings' do - let(:project_vulnerabilities_path) { "/projects/#{project.id}/#{api_resource_name}" } - - context 'with an authorized user with proper permissions' do - before do - project.add_developer(user) - end - - it 'returns all non-dismissed vulnerabilities' do - occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s - - get api(project_vulnerabilities_path, user), params: { per_page: 40 } - - expect(response).to have_gitlab_http_status(200) - expect(response).to include_pagination_headers - expect(response).to match_response_schema('vulnerabilities/occurrence_list', dir: 'ee') - - expect(response.headers['X-Total']).to eq occurrence_count - - expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning sast] - end - - it 'does not have N+1 queries' do - control_count = ActiveRecord::QueryRecorder.new do - 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) - end - - describe 'filtering' do - it 'returns vulnerabilities with sast report_type' do - occurrence_count = (sast_report.occurrences.count - 1).to_s - - get api(project_vulnerabilities_path, user), params: { report_type: 'sast' } - - expect(response).to have_gitlab_http_status(200) - - expect(response.headers['X-Total']).to eq occurrence_count - - expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[sast] - - # occurrences are implicitly sorted by Security::MergeReportsService, - # occurrences order differs from what is present in fixture file - expect(json_response.first['name']).to eq 'ECB mode is insecure' - end - - it 'returns vulnerabilities with dependency_scanning report_type' do - occurrence_count = ds_report.occurrences.count.to_s - - get api(project_vulnerabilities_path, user), params: { report_type: 'dependency_scanning' } - - expect(response).to have_gitlab_http_status(200) - - expect(response.headers['X-Total']).to eq occurrence_count - - expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning] - - # occurrences are implicitly sorted by Security::MergeReportsService, - # occurrences order differs from what is present in fixture file - expect(json_response.first['name']).to eq 'ruby-ffi DDL loading issue on Windows OS' - end - - it 'returns a "bad request" response for an unknown report type' do - get api(project_vulnerabilities_path, user), params: { report_type: 'blah' } - - expect(response).to have_gitlab_http_status(400) - end - - it 'returns dismissed vulnerabilities with `all` scope' do - occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count).to_s - - get api(project_vulnerabilities_path, user), params: { per_page: 40, scope: 'all' } - - expect(response).to have_gitlab_http_status(200) - - expect(response.headers['X-Total']).to eq occurrence_count - end - - it 'returns vulnerabilities with low severity' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, severity: 'low' } - - expect(response).to have_gitlab_http_status(200) - - expect(json_response.map { |v| v['severity'] }.uniq).to eq %w[low] - end - - it 'returns a "bad request" response for an unknown severity value' do - get api(project_vulnerabilities_path, user), params: { severity: 'foo' } - - expect(response).to have_gitlab_http_status(400) - end - - it 'returns vulnerabilities with high confidence' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, confidence: 'high' } - - expect(response).to have_gitlab_http_status(200) - - expect(json_response.map { |v| v['confidence'] }.uniq).to eq %w[high] - end - - it 'returns a "bad request" response for an unknown confidence value' do - get api(project_vulnerabilities_path, user), params: { confidence: 'qux' } - - expect(response).to have_gitlab_http_status(400) - end - - context 'when pipeline_id is supplied' do - it 'returns vulnerabilities from supplied pipeline' do - occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s - - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline.id } - - expect(response).to have_gitlab_http_status(200) - - expect(response.headers['X-Total']).to eq occurrence_count - end - - context 'pipeline has no reports' do - it 'returns empty results' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline_without_vulnerabilities.id } - - expect(json_response).to eq [] - end - end - - context 'with unknown pipeline' do - it 'returns empty results' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: 0 } + describe "GET /projects/:id/vulnerability_findings" do + let(:project_vulnerabilities_path) { "/projects/#{project.id}/vulnerability_findings" } - expect(json_response).to eq [] - end - end - end - end - end + it_behaves_like 'getting list of vulnerability findings' - context 'with authorized user without read permissions' do + context 'when "first-class vulnerabilities" feature is disabled' do before do - project.add_reporter(user) - stub_licensed_features(security_dashboard: false, sast: true, dependency_scanning: true, container_scanning: true) + stub_feature_flags(first_class_vulnerabilities: false) end - it 'responds with 403 Forbidden' do + it 'responds with "not found"' do get api(project_vulnerabilities_path, user) - expect(response).to have_gitlab_http_status(403) - end - end - - context 'with no project access' do - it 'responds with 404 Not Found' do - private_project = create(:project) - - get api("/projects/#{private_project.id}/#{api_resource_name}", user) - expect(response).to have_gitlab_http_status(404) end end - - context 'with unknown project' do - it 'responds with 404 Not Found' do - get api("/projects/0/#{api_resource_name}", user) - - expect(response).to have_gitlab_http_status(404) - end - end - end - - shared_examples 'not found vulnerabilities endpoint' do - it do - get api("/projects/#{project.id}/#{api_resource_name}?", user), params: { per_page: 40 } - - expect(response).to have_gitlab_http_status(404) - end - end - - describe "GET /projects/:id/vulnerabilities" do - let(:api_resource_name) { 'vulnerabilities' } - - it_behaves_like 'not found vulnerabilities endpoint' - - context 'when vulnerability findings API is disabled' do - before do - stub_feature_flags(first_class_vulnerabilities: false) - end - - it_behaves_like 'getting list of vulnerability findings' - end - end - - describe "GET /projects/:id/vulnerability_findings" do - let(:api_resource_name) { 'vulnerability_findings' } - - it_behaves_like 'getting list of vulnerability findings' - - context 'when vulnerability findings API is disabled' do - before do - stub_feature_flags(first_class_vulnerabilities: false) - end - - it_behaves_like 'not found vulnerabilities endpoint' - 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 new file mode 100644 index 00000000000000..f213bd58eb9956 --- /dev/null +++ b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb @@ -0,0 +1,216 @@ +# frozen_string_literal: true + +shared_examples 'forbids access to vulnerability-like endpoint in expected cases' do + context 'with authorized user without read permissions' do + before do + project.add_reporter(user) + end + + it 'responds with 403 Forbidden' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(403) + end + end + + context 'with authorized user but when security dashboard is not available' do + before do + project.add_developer(user) + stub_licensed_features(security_dashboard: false) + end + + it 'responds with 403 Forbidden' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(403) + end + end + + context 'with no project access' do + let(:project) { create(:project) } + + it 'responds with 404 Not Found' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'with unknown project' do + before do + project.id = 0 + end + + let(:project) { build(:project) } + + it 'responds with 404 Not Found' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(404) + end + end +end + +shared_examples 'getting list of vulnerability findings' do + before do + stub_licensed_features(security_dashboard: true, sast: true, dependency_scanning: true, container_scanning: true) + + create(:ee_ci_job_artifact, :dependency_scanning, job: build_ds, project: project) + create(:ee_ci_job_artifact, :sast, job: build_sast, project: project) + dismissal + end + + let(:pipeline) { create(:ci_empty_pipeline, status: :created, project: project) } + let(:pipeline_without_vulnerabilities) { create(:ci_pipeline_without_jobs, status: :created, project: project) } + + let(:build_ds) { create(:ci_build, :success, name: 'ds_job', pipeline: pipeline, project: project) } + let(:build_sast) { create(:ci_build, :success, name: 'sast_job', pipeline: pipeline, project: project) } + + let(:ds_report) { pipeline.security_reports.reports["dependency_scanning"] } + let(:sast_report) { pipeline.security_reports.reports["sast"] } + + let(:dismissal) do + create(:vulnerability_feedback, :dismissal, :sast, + project: project, + pipeline: pipeline, + project_fingerprint: sast_report.occurrences.first.project_fingerprint, + vulnerability_data: sast_report.occurrences.first.raw_metadata + ) + end + + context 'with an authorized user with proper permissions' do + before do + project.add_developer(user) + end + + it 'returns all non-dismissed vulnerabilities' do + occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s + + get api(project_vulnerabilities_path, user), params: { per_page: 40 } + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(response).to match_response_schema('vulnerabilities/occurrence_list', dir: 'ee') + + expect(response.headers['X-Total']).to eq occurrence_count + + expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning sast] + end + + it 'does not have N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new do + 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) + end + + describe 'filtering' do + it 'returns vulnerabilities with sast report_type' do + occurrence_count = (sast_report.occurrences.count - 1).to_s + + get api(project_vulnerabilities_path, user), params: { report_type: 'sast' } + + expect(response).to have_gitlab_http_status(200) + + expect(response.headers['X-Total']).to eq occurrence_count + + expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[sast] + + # occurrences are implicitly sorted by Security::MergeReportsService, + # occurrences order differs from what is present in fixture file + expect(json_response.first['name']).to eq 'ECB mode is insecure' + end + + it 'returns vulnerabilities with dependency_scanning report_type' do + occurrence_count = ds_report.occurrences.count.to_s + + get api(project_vulnerabilities_path, user), params: { report_type: 'dependency_scanning' } + + expect(response).to have_gitlab_http_status(200) + + expect(response.headers['X-Total']).to eq occurrence_count + + expect(json_response.map { |v| v['report_type'] }.uniq).to match_array %w[dependency_scanning] + + # occurrences are implicitly sorted by Security::MergeReportsService, + # occurrences order differs from what is present in fixture file + expect(json_response.first['name']).to eq 'ruby-ffi DDL loading issue on Windows OS' + end + + it 'returns a "bad request" response for an unknown report type' do + get api(project_vulnerabilities_path, user), params: { report_type: 'blah' } + + expect(response).to have_gitlab_http_status(400) + end + + it 'returns dismissed vulnerabilities with `all` scope' do + occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count).to_s + + get api(project_vulnerabilities_path, user), params: { per_page: 40, scope: 'all' } + + expect(response).to have_gitlab_http_status(200) + + expect(response.headers['X-Total']).to eq occurrence_count + end + + it 'returns vulnerabilities with low severity' do + get api(project_vulnerabilities_path, user), params: { per_page: 40, severity: 'low' } + + expect(response).to have_gitlab_http_status(200) + + expect(json_response.map { |v| v['severity'] }.uniq).to eq %w[low] + end + + it 'returns a "bad request" response for an unknown severity value' do + get api(project_vulnerabilities_path, user), params: { severity: 'foo' } + + expect(response).to have_gitlab_http_status(400) + end + + it 'returns vulnerabilities with high confidence' do + get api(project_vulnerabilities_path, user), params: { per_page: 40, confidence: 'high' } + + expect(response).to have_gitlab_http_status(200) + + expect(json_response.map { |v| v['confidence'] }.uniq).to eq %w[high] + end + + it 'returns a "bad request" response for an unknown confidence value' do + get api(project_vulnerabilities_path, user), params: { confidence: 'qux' } + + expect(response).to have_gitlab_http_status(400) + end + + context 'when pipeline_id is supplied' do + it 'returns vulnerabilities from supplied pipeline' do + occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s + + get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline.id } + + expect(response).to have_gitlab_http_status(200) + + expect(response.headers['X-Total']).to eq occurrence_count + end + + context 'pipeline has no reports' do + it 'returns empty results' do + get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline_without_vulnerabilities.id } + + expect(json_response).to eq [] + end + end + + context 'with unknown pipeline' do + it 'returns empty results' do + get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: 0 } + + expect(json_response).to eq [] + end + end + end + end + end + + it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' +end -- GitLab From ad25c574d11a09bb3d2554eb8e11765926fd062c Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Tue, 8 Oct 2019 18:24:11 +0300 Subject: [PATCH 15/27] Move pagination to API level where it should be --- ee/app/finders/security/vulnerabilities_finder.rb | 10 ++-------- ee/lib/api/vulnerabilities.rb | 6 +++--- .../finders/security/vulnerabilities_finder_spec.rb | 5 ++--- ee/spec/requests/api/vulnerabilities_spec.rb | 7 +++++++ 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb index efc2cd53a9760d..888468d5c2344e 100644 --- a/ee/app/finders/security/vulnerabilities_finder.rb +++ b/ee/app/finders/security/vulnerabilities_finder.rb @@ -6,23 +6,17 @@ # # Arguments: # project: a Project to query for Vulnerabilities -# params: -# page: Integer -# per_page: Integer module Security class VulnerabilitiesFinder attr_reader :project - attr_reader :page, :per_page - def initialize(project, params = {}) + def initialize(project) @project = project - @page = params[:page] || 1 - @per_page = params[:per_page] || 20 end def execute - project.vulnerabilities.page(page).per(per_page) + project.vulnerabilities end end end diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb index 83838ee4902afe..63e58a366ba826 100644 --- a/ee/lib/api/vulnerabilities.rb +++ b/ee/lib/api/vulnerabilities.rb @@ -7,8 +7,8 @@ class Vulnerabilities < Grape::API helpers ::API::Helpers::VulnerabilityFindingsHelpers helpers do - def vulnerabilities_by(project, params) - Security::VulnerabilitiesFinder.new(project, params).execute + def vulnerabilities_by(project) + Security::VulnerabilitiesFinder.new(project).execute end end @@ -36,7 +36,7 @@ def vulnerabilities_by(project, params) authorize! :read_project_security_dashboard, user_project vulnerabilities = paginate( - vulnerabilities_by(user_project, declared_params) + vulnerabilities_by(user_project) ) present vulnerabilities, with: VulnerabilityEntity diff --git a/ee/spec/finders/security/vulnerabilities_finder_spec.rb b/ee/spec/finders/security/vulnerabilities_finder_spec.rb index 62710f9cacf5ef..70b12eaa103043 100644 --- a/ee/spec/finders/security/vulnerabilities_finder_spec.rb +++ b/ee/spec/finders/security/vulnerabilities_finder_spec.rb @@ -4,11 +4,10 @@ describe Security::VulnerabilitiesFinder do let(:project) { create(:project, :with_vulnerabilities) } - let(:params) { { page: 2, per_page: 1 } } - subject { described_class.new(project, params).execute } + subject { described_class.new(project).execute } it 'returns vulnerabilities of a project and respects pagination params' do - expect(subject).to contain_exactly(project.vulnerabilities.drop(1).take(1).first) + expect(subject).to match_array(project.vulnerabilities) end end diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index 2c2e12144a6a2b..93bdd635455915 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -26,6 +26,13 @@ expect(response).to match_response_schema('vulnerability_list', dir: 'ee') expect(response.headers['X-Total']).to eq project.vulnerabilities.count.to_s end + + it 'paginates the vulnerabilities according to the pagination params' do + get api("#{project_vulnerabilities_path}?page=2&per_page=1", user) + + expect(response).to have_gitlab_http_status(200) + expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.drop(1).take(1).first.id) + end end it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' -- GitLab From b93016ef1396c79bbea50a264d3588d1e57c175c Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 20:34:09 +0300 Subject: [PATCH 16/27] Fix typo in comment --- ee/app/finders/security/vulnerabilities_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb index 888468d5c2344e..932b9b31c1f1a4 100644 --- a/ee/app/finders/security/vulnerabilities_finder.rb +++ b/ee/app/finders/security/vulnerabilities_finder.rb @@ -2,7 +2,7 @@ # Security::VulnerabilitiesFinder # -# Used to filter Vulnerability record by set of params for Vulnerabilities API +# Used to filter Vulnerability records by set of params for Vulnerabilities API # # Arguments: # project: a Project to query for Vulnerabilities -- GitLab From 37cfbf80045782094b0dad4a711c5a6d01a0bba3 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 22:33:00 +0300 Subject: [PATCH 17/27] Rename VulnerabilityFindingsFinder dependencies --- .../groups/security/vulnerabilities_controller.rb | 2 +- ee/lib/gitlab/vulnerabilities/history.rb | 8 ++++---- ee/lib/gitlab/vulnerabilities/history_cache.rb | 6 ++++-- ee/spec/lib/gitlab/vulnerabilities/history_spec.rb | 4 ++-- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/ee/app/controllers/groups/security/vulnerabilities_controller.rb b/ee/app/controllers/groups/security/vulnerabilities_controller.rb index 5bfaacf9b1981a..19c84d06c26ced 100644 --- a/ee/app/controllers/groups/security/vulnerabilities_controller.rb +++ b/ee/app/controllers/groups/security/vulnerabilities_controller.rb @@ -7,7 +7,7 @@ class Groups::Security::VulnerabilitiesController < Groups::ApplicationControlle alias_method :vulnerable, :group def history - history_count = Gitlab::Vulnerabilities::History.new(group, filter_params).vulnerabilities_counter + history_count = Gitlab::Vulnerabilities::History.new(group, filter_params).findings_counter respond_to do |format| format.json do diff --git a/ee/lib/gitlab/vulnerabilities/history.rb b/ee/lib/gitlab/vulnerabilities/history.rb index 0951617d9e9fbd..749b50fe92f8c3 100644 --- a/ee/lib/gitlab/vulnerabilities/history.rb +++ b/ee/lib/gitlab/vulnerabilities/history.rb @@ -14,16 +14,16 @@ def initialize(group, filters) @filters = filters end - def vulnerabilities_counter + def findings_counter return cached_vulnerability_history if use_vulnerability_cache? - vulnerabilities = found_vulnerabilities.count_by_day_and_severity(HISTORY_RANGE) - ::Vulnerabilities::HistorySerializer.new.represent(vulnerabilities) + findings = vulnerability_findings.count_by_day_and_severity(HISTORY_RANGE) + ::Vulnerabilities::HistorySerializer.new.represent(findings) end private - def found_vulnerabilities + def vulnerability_findings ::Security::VulnerabilityFindingsFinder.new(group, params: filters).execute(:all) end diff --git a/ee/lib/gitlab/vulnerabilities/history_cache.rb b/ee/lib/gitlab/vulnerabilities/history_cache.rb index 1a5a55696e91e9..d6f48547f59492 100644 --- a/ee/lib/gitlab/vulnerabilities/history_cache.rb +++ b/ee/lib/gitlab/vulnerabilities/history_cache.rb @@ -12,17 +12,19 @@ def initialize(group, project_id) def fetch(range, force: false) Rails.cache.fetch(cache_key, force: force, expires_in: 1.day) do - vulnerabilities = ::Security::VulnerabilityFindingsFinder + findings = ::Security::VulnerabilityFindingsFinder .new(group, params: { project_id: [project_id] }) .execute(:all) .count_by_day_and_severity(range) - ::Vulnerabilities::HistorySerializer.new.represent(vulnerabilities) + ::Vulnerabilities::HistorySerializer.new.represent(findings) end end private def cache_key + # TODO: rename 'vulnerabilities' to 'findings' in the cache key, but carefully + # https://gitlab.com/gitlab-org/gitlab/issues/32963 ['projects', project_id, 'vulnerabilities'] end end diff --git a/ee/spec/lib/gitlab/vulnerabilities/history_spec.rb b/ee/spec/lib/gitlab/vulnerabilities/history_spec.rb index 6fee0f88610cc2..160816591cb66a 100644 --- a/ee/spec/lib/gitlab/vulnerabilities/history_spec.rb +++ b/ee/spec/lib/gitlab/vulnerabilities/history_spec.rb @@ -13,8 +13,8 @@ create_vulnerabilities(2, project2, { severity: :high, report_type: :sast }) end - describe '#vulnerabilities_counter', :use_clean_rails_memory_store_caching do - subject(:counter) { described_class.new(group, filters).vulnerabilities_counter } + describe '#findings_counter', :use_clean_rails_memory_store_caching do + subject(:counter) { described_class.new(group, filters).findings_counter } context 'feature disabled' do before do -- GitLab From d8cf6e5b839fc170246de174bad6c307c1468661 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 22:33:46 +0300 Subject: [PATCH 18/27] Fix wording in test description --- ee/spec/finders/security/vulnerabilities_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/finders/security/vulnerabilities_finder_spec.rb b/ee/spec/finders/security/vulnerabilities_finder_spec.rb index 70b12eaa103043..372bfa16e3b5c7 100644 --- a/ee/spec/finders/security/vulnerabilities_finder_spec.rb +++ b/ee/spec/finders/security/vulnerabilities_finder_spec.rb @@ -7,7 +7,7 @@ subject { described_class.new(project).execute } - it 'returns vulnerabilities of a project and respects pagination params' do + it 'returns vulnerabilities of a project' do expect(subject).to match_array(project.vulnerabilities) end end -- GitLab From 1eb03ae1ab09b51c372fca224b4cad684f0e4fa8 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 22:34:08 +0300 Subject: [PATCH 19/27] Reduce vulnerabilities count in trait to 2 --- ee/spec/factories/projects.rb | 2 +- ee/spec/requests/api/vulnerabilities_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/factories/projects.rb b/ee/spec/factories/projects.rb index f3a8bc309f962e..2c97a617acae53 100644 --- a/ee/spec/factories/projects.rb +++ b/ee/spec/factories/projects.rb @@ -92,7 +92,7 @@ trait :with_vulnerabilities do after(:create) do |project| - create_list(:vulnerability, 3, :opened, project: project) + create_list(:vulnerability, 2, :opened, project: project) end end end diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index 93bdd635455915..f5c05b4befec48 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -31,7 +31,7 @@ get api("#{project_vulnerabilities_path}?page=2&per_page=1", user) expect(response).to have_gitlab_http_status(200) - expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.drop(1).take(1).first.id) + expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.second.id) end end -- GitLab From e77bd057b5dbca1b17390aff01cf1bd2d4941172 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Wed, 9 Oct 2019 22:35:01 +0300 Subject: [PATCH 20/27] Improve code and comments in shared examples --- .../api/vulnerabilities_shared_examples.rb | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) 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 f213bd58eb9956..6e1e042dd31870 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 @@ -83,10 +83,16 @@ project.add_developer(user) end + # Because fixture reports that power :ee_ci_job_artifact factory contain long report lists, + # we need to make sure that all occurrences for both SAST and Dependency Scanning are included in the response. + # That's why the page size is 40. + let(:pagination) { { per_page: 40 } } + it 'returns all non-dismissed vulnerabilities' do + # all occurrences except one that was dismissed occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s - get api(project_vulnerabilities_path, user), params: { per_page: 40 } + get api(project_vulnerabilities_path, user), params: pagination expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -107,7 +113,7 @@ describe 'filtering' do it 'returns vulnerabilities with sast report_type' do - occurrence_count = (sast_report.occurrences.count - 1).to_s + occurrence_count = (sast_report.occurrences.count - 1).to_s # all SAST occurrences except one that was dismissed get api(project_vulnerabilities_path, user), params: { report_type: 'sast' } @@ -147,7 +153,7 @@ it 'returns dismissed vulnerabilities with `all` scope' do occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count).to_s - get api(project_vulnerabilities_path, user), params: { per_page: 40, scope: 'all' } + get api(project_vulnerabilities_path, user), params: { scope: 'all' }.merge(pagination) expect(response).to have_gitlab_http_status(200) @@ -155,7 +161,7 @@ end it 'returns vulnerabilities with low severity' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, severity: 'low' } + get api(project_vulnerabilities_path, user), params: { severity: 'low' }.merge(pagination) expect(response).to have_gitlab_http_status(200) @@ -169,7 +175,7 @@ end it 'returns vulnerabilities with high confidence' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, confidence: 'high' } + get api(project_vulnerabilities_path, user), params: { confidence: 'high' }.merge(pagination) expect(response).to have_gitlab_http_status(200) @@ -186,7 +192,7 @@ it 'returns vulnerabilities from supplied pipeline' do occurrence_count = (sast_report.occurrences.count + ds_report.occurrences.count - 1).to_s - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline.id } + get api(project_vulnerabilities_path, user), params: { pipeline_id: pipeline.id }.merge(pagination) expect(response).to have_gitlab_http_status(200) @@ -195,7 +201,7 @@ context 'pipeline has no reports' do it 'returns empty results' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: pipeline_without_vulnerabilities.id } + get api(project_vulnerabilities_path, user), params: { pipeline_id: pipeline_without_vulnerabilities.id }.merge(pagination) expect(json_response).to eq [] end @@ -203,7 +209,7 @@ context 'with unknown pipeline' do it 'returns empty results' do - get api(project_vulnerabilities_path, user), params: { per_page: 40, pipeline_id: 0 } + get api(project_vulnerabilities_path, user), params: { pipeline_id: 0 }.merge(pagination) expect(json_response).to eq [] end -- GitLab From 636c81c002185ac784ee676aa48fc3c26744df88 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 16:07:24 +0300 Subject: [PATCH 21/27] Simplify API tests --- ee/spec/requests/api/vulnerabilities_spec.rb | 14 +++++++------- .../requests/api/vulnerability_findings_spec.rb | 17 ++++++++++++----- .../api/vulnerabilities_shared_examples.rb | 2 -- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb index f5c05b4befec48..c93767ba603fcb 100644 --- a/ee/spec/requests/api/vulnerabilities_spec.rb +++ b/ee/spec/requests/api/vulnerabilities_spec.rb @@ -33,16 +33,16 @@ expect(response).to have_gitlab_http_status(200) expect(json_response.map { |v| v['id'] }).to contain_exactly(project.vulnerabilities.second.id) end - end - it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' + context 'when "first-class vulnerabilities" feature is disabled' do + before do + stub_feature_flags(first_class_vulnerabilities: false) + end - context 'when "first-class vulnerabilities" feature is disabled' do - before do - stub_feature_flags(first_class_vulnerabilities: false) + it_behaves_like 'getting list of vulnerability findings' end - - it_behaves_like 'getting list of vulnerability findings' end + + it_behaves_like 'forbids access to vulnerability-like 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 112e8f0d278333..0c0141bdc29ec8 100644 --- a/ee/spec/requests/api/vulnerability_findings_spec.rb +++ b/ee/spec/requests/api/vulnerability_findings_spec.rb @@ -10,16 +10,23 @@ 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' - context 'when "first-class vulnerabilities" feature is disabled' do + context 'with an authorized user with proper permissions' do before do - stub_feature_flags(first_class_vulnerabilities: false) + project.add_developer(user) end - it 'responds with "not found"' do - get api(project_vulnerabilities_path, user) + context 'when "first-class vulnerabilities" feature is disabled' do + before do + stub_feature_flags(first_class_vulnerabilities: false) + end - expect(response).to have_gitlab_http_status(404) + it 'responds with "not found"' do + get api(project_vulnerabilities_path, user) + + expect(response).to have_gitlab_http_status(404) + end 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 6e1e042dd31870..f71cc233c9c368 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 @@ -217,6 +217,4 @@ end end end - - it_behaves_like 'forbids access to vulnerability-like endpoint in expected cases' end -- GitLab From 6a04e661206ec57ee5e226088a16f3b88aa9d42c Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 19:16:58 +0300 Subject: [PATCH 22/27] Remove unnecessary wording from the comment --- ee/app/finders/security/vulnerabilities_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/finders/security/vulnerabilities_finder.rb b/ee/app/finders/security/vulnerabilities_finder.rb index 932b9b31c1f1a4..3954ca2c1629bc 100644 --- a/ee/app/finders/security/vulnerabilities_finder.rb +++ b/ee/app/finders/security/vulnerabilities_finder.rb @@ -2,7 +2,7 @@ # Security::VulnerabilitiesFinder # -# Used to filter Vulnerability records by set of params for Vulnerabilities API +# Used to filter Vulnerability records for Vulnerabilities API # # Arguments: # project: a Project to query for Vulnerabilities -- GitLab From 9c65354b65d933f45d34046565476a6e9eb143d8 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 19:33:31 +0300 Subject: [PATCH 23/27] Remove unnecessary comment from finder class --- ee/app/finders/security/vulnerability_findings_finder.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/ee/app/finders/security/vulnerability_findings_finder.rb b/ee/app/finders/security/vulnerability_findings_finder.rb index 5dc019c565444c..195301c7a92f6f 100644 --- a/ee/app/finders/security/vulnerability_findings_finder.rb +++ b/ee/app/finders/security/vulnerability_findings_finder.rb @@ -13,8 +13,6 @@ # report_type: Array module Security - # TODO: check usages and rename `vulnerable` associations to mention the term "finding"; - # probably, add new changes to https://gitlab.com/gitlab-org/gitlab/merge_requests/17351 class VulnerabilityFindingsFinder attr_accessor :params attr_reader :vulnerable -- GitLab From 3b1dbd53c9bd32cfc75ad54be6727d5b6f68017f Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 19:44:22 +0300 Subject: [PATCH 24/27] Improve scope handling in finder class --- .../security/vulnerability_findings_finder.rb | 9 ++++--- .../vulnerability_findings_finder_spec.rb | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/ee/app/finders/security/vulnerability_findings_finder.rb b/ee/app/finders/security/vulnerability_findings_finder.rb index 195301c7a92f6f..9f868d770ce403 100644 --- a/ee/app/finders/security/vulnerability_findings_finder.rb +++ b/ee/app/finders/security/vulnerability_findings_finder.rb @@ -64,12 +64,15 @@ def by_confidence(items) end def init_collection(scope) - if scope == :all + case scope + when :all vulnerable.all_vulnerabilities - elsif scope == :with_sha + when :with_sha vulnerable.latest_vulnerabilities_with_sha - else + when :latest vulnerable.latest_vulnerabilities + else + raise ArgumentError, "invalid value for 'scope': #{scope}" end end end diff --git a/ee/spec/finders/security/vulnerability_findings_finder_spec.rb b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb index 3edee7d6f31db1..4d47280ee99236 100644 --- a/ee/spec/finders/security/vulnerability_findings_finder_spec.rb +++ b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb @@ -130,5 +130,31 @@ end end end + + describe 'scope specifiers' do + using RSpec::Parameterized::TableSyntax + + where(:scope) do + [ + [:all], + [:with_sha], + [:latest] + ] + end + + with_them do + it 'accepts the scope specifier as valid' do + expect { described_class.new(group).execute(scope) }.not_to raise_error + end + end + + context 'with an invalid scope specifier' do + it 'raises error' do + expect { described_class.new(group).execute(:invalid) }.to( + raise_error(ArgumentError, "invalid value for 'scope': invalid") + ) + end + end + end end end -- GitLab From 40e7e5a26851ff1dc1fd34abeab709a1e3ae7165 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 19:57:27 +0300 Subject: [PATCH 25/27] Refactor VulnerabilityFindingsFinder specs --- .../vulnerability_findings_finder_spec.rb | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/ee/spec/finders/security/vulnerability_findings_finder_spec.rb b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb index 4d47280ee99236..c8f87529e9904e 100644 --- a/ee/spec/finders/security/vulnerability_findings_finder_spec.rb +++ b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb @@ -4,16 +4,16 @@ describe Security::VulnerabilityFindingsFinder do describe '#execute' do - set(:group) { create(:group) } - set(:project1) { create(:project, :private, :repository, group: group) } - set(:project2) { create(:project, :private, :repository, group: group) } - set(:pipeline1) { create(:ci_pipeline, :success, project: project1) } - set(:pipeline2) { create(:ci_pipeline, :success, project: project2) } + let_it_be(:group) { create(:group) } + let_it_be(:project1) { create(:project, :private, :repository, group: group) } + let_it_be(:project2) { create(:project, :private, :repository, group: group) } + let_it_be(:pipeline1) { create(:ci_pipeline, :success, project: project1) } + let_it_be(:pipeline2) { create(:ci_pipeline, :success, project: project2) } - set(:vulnerability1) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :high, confidence: :high, pipelines: [pipeline1], project: project1) } - set(:vulnerability2) { create(:vulnerabilities_occurrence, report_type: :dependency_scanning, severity: :medium, confidence: :low, pipelines: [pipeline2], project: project2) } - set(:vulnerability3) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :low, pipelines: [pipeline2], project: project2) } - set(:vulnerability4) { create(:vulnerabilities_occurrence, report_type: :dast, severity: :medium, pipelines: [pipeline1], project: project1) } + let_it_be(:finding1) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :high, confidence: :high, pipelines: [pipeline1], project: project1) } + let_it_be(:finding2) { create(:vulnerabilities_occurrence, report_type: :dependency_scanning, severity: :medium, confidence: :low, pipelines: [pipeline2], project: project2) } + let_it_be(:finding3) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :low, pipelines: [pipeline2], project: project2) } + let_it_be(:finding4) { create(:vulnerabilities_occurrence, report_type: :dast, severity: :medium, pipelines: [pipeline1], project: project1) } subject { described_class.new(group, params: params).execute } @@ -22,7 +22,7 @@ let(:params) { { report_type: %w[sast] } } it 'includes only sast' do - is_expected.to contain_exactly(vulnerability1, vulnerability3) + is_expected.to contain_exactly(finding1, finding3) end end @@ -30,7 +30,7 @@ let(:params) { { report_type: %w[dependency_scanning] } } it 'includes only depscan' do - is_expected.to contain_exactly(vulnerability2) + is_expected.to contain_exactly(finding2) end end end @@ -40,7 +40,7 @@ let(:params) { { severity: %w[high] } } it 'includes only high' do - is_expected.to contain_exactly(vulnerability1) + is_expected.to contain_exactly(finding1) end end @@ -48,7 +48,7 @@ let(:params) { { severity: %w[medium] } } it 'includes only medium' do - is_expected.to contain_exactly(vulnerability2, vulnerability4) + is_expected.to contain_exactly(finding2, finding4) end end end @@ -58,7 +58,7 @@ let(:params) { { confidence: %w[high] } } it 'includes only high confidence vulnerabilities' do - is_expected.to contain_exactly(vulnerability1) + is_expected.to contain_exactly(finding1) end end @@ -66,7 +66,7 @@ let(:params) { { confidence: %w[low] } } it 'includes only low confidence vulnerabilities' do - is_expected.to contain_exactly(vulnerability2) + is_expected.to contain_exactly(finding2) end end end @@ -75,7 +75,7 @@ let(:params) { { project_id: [project2.id] } } it 'includes only vulnerabilities for one project' do - is_expected.to contain_exactly(vulnerability2, vulnerability3) + is_expected.to contain_exactly(finding2, finding3) end end @@ -85,13 +85,13 @@ create(:vulnerability_feedback, :sast, :dismissal, pipeline: pipeline1, project: project1, - project_fingerprint: vulnerability1.project_fingerprint) + project_fingerprint: finding1.project_fingerprint) end let(:params) { { hide_dismissed: true } } skip 'exclude dismissal' do - is_expected.to contain_exactly(vulnerability2, vulnerability3, vulnerability4) + is_expected.to contain_exactly(finding2, finding3, finding4) end end @@ -100,7 +100,7 @@ let(:params) { { severity: %w[high medium low], project_id: [project1.id, project2.id], report_type: %w[sast dast] } } it 'filters by all params' do - is_expected.to contain_exactly(vulnerability1, vulnerability3, vulnerability4) + is_expected.to contain_exactly(finding1, finding3, finding4) end end @@ -118,7 +118,7 @@ let(:params) { { project_id: [project2.id], severity: %w[medium low] } } it 'filters by all params' do - is_expected.to contain_exactly(vulnerability2, vulnerability3) + is_expected.to contain_exactly(finding2, finding3) end end -- GitLab From 22e9202c1e24ce46ee76cae05e5686f133085b57 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 20:03:34 +0300 Subject: [PATCH 26/27] Remove extra :project param from finder helper --- ee/lib/api/helpers/vulnerability_findings_helpers.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/lib/api/helpers/vulnerability_findings_helpers.rb b/ee/lib/api/helpers/vulnerability_findings_helpers.rb index 0f43e21a9896af..875cfaa0560e7a 100644 --- a/ee/lib/api/helpers/vulnerability_findings_helpers.rb +++ b/ee/lib/api/helpers/vulnerability_findings_helpers.rb @@ -32,9 +32,9 @@ module VulnerabilityFindingsHelpers # TODO: rename to vulnerability_findings_by https://gitlab.com/gitlab-org/gitlab/issues/32963 def vulnerability_occurrences_by(params) pipeline = if params[:pipeline_id] - params[:project].all_pipelines.find_by(id: params[:pipeline_id]) # rubocop:disable CodeReuse/ActiveRecord + user_project.all_pipelines.find_by(id: params[:pipeline_id]) # rubocop:disable CodeReuse/ActiveRecord else - params[:project].latest_pipeline_with_security_reports + user_project.latest_pipeline_with_security_reports end return [] unless pipeline @@ -47,7 +47,7 @@ def respond_with_vulnerability_findings vulnerability_occurrences = paginate( Kaminari.paginate_array( - vulnerability_occurrences_by(declared_params.merge(project: user_project)) + vulnerability_occurrences_by(declared_params) ) ) -- GitLab From 0c8ca166f70ccc61398a0476adeda4a021bf3b40 Mon Sep 17 00:00:00 2001 From: Victor Zagorodny Date: Thu, 10 Oct 2019 22:01:58 +0300 Subject: [PATCH 27/27] Remove unnecessary pending example --- .../vulnerability_findings_finder_spec.rb | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/ee/spec/finders/security/vulnerability_findings_finder_spec.rb b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb index c8f87529e9904e..acbbca915e0fb9 100644 --- a/ee/spec/finders/security/vulnerability_findings_finder_spec.rb +++ b/ee/spec/finders/security/vulnerability_findings_finder_spec.rb @@ -79,22 +79,6 @@ end end - # FIXME: unskip when this filter is implemented - context 'by dismissals' do - let!(:dismissal) do - create(:vulnerability_feedback, :sast, :dismissal, - pipeline: pipeline1, - project: project1, - project_fingerprint: finding1.project_fingerprint) - end - - let(:params) { { hide_dismissed: true } } - - skip 'exclude dismissal' do - is_expected.to contain_exactly(finding2, finding3, finding4) - end - end - context 'by all filters' do context 'with found entity' do let(:params) { { severity: %w[high medium low], project_id: [project1.id, project2.id], report_type: %w[sast dast] } } -- GitLab