diff --git a/changelogs/unreleased/most-affected-projects.yml b/changelogs/unreleased/most-affected-projects.yml new file mode 100644 index 0000000000000000000000000000000000000000..1835f62e5338849cd040f51deb4e4e0f297d188d --- /dev/null +++ b/changelogs/unreleased/most-affected-projects.yml @@ -0,0 +1,5 @@ +--- +title: Add endpoint for a group's vulnerable projects +merge_request: 15317 +author: +type: added diff --git a/ee/app/controllers/groups/security/vulnerable_projects_controller.rb b/ee/app/controllers/groups/security/vulnerable_projects_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..97fb2b058b27cf3b39accf08e3a3ff807cb0deba --- /dev/null +++ b/ee/app/controllers/groups/security/vulnerable_projects_controller.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class Groups::Security::VulnerableProjectsController < Groups::ApplicationController + include SecurityDashboardsPermissions + + alias_method :vulnerable, :group + + def index + projects = group.vulnerable_projects.non_archived.without_deleted.with_route + + vulnerable_projects = projects.map do |project| + ::Security::VulnerableProjectPresenter.new(project) + end + + render json: VulnerableProjectSerializer.new.represent(vulnerable_projects) + end +end diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 4b69ca6fef8d39c1c632df847d3349cc71e5acf0..91e4fe875323730811337e0c41c7946501caff18 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -118,6 +118,10 @@ def ip_restriction_ranges ip_restrictions.map(&:range).join(",") end + def vulnerable_projects + projects.where("EXISTS(?)", ::Vulnerabilities::Occurrence.select(1).undismissed.where('vulnerability_occurrences.project_id = projects.id')) + end + def human_ldap_access ::Gitlab::Access.options_with_owner.key(ldap_access) end diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index fdb5c1ad3b8ff2241a39cde41c7e1464cdc2f847..97b29d45c23389976215fa9aa07a941824f4f580 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/ee/app/models/vulnerabilities/occurrence.rb @@ -139,6 +139,33 @@ def state end end + def self.undismissed + where( + "NOT EXISTS (?)", + Feedback.select(1) + .where("ENCODE(#{table_name}.project_fingerprint, 'HEX') = vulnerability_feedback.project_fingerprint") # rubocop:disable GitlabSecurity/SqlInjection + .for_dismissal + ) + end + + def self.batch_count_by_project_and_severity(project_id, severity) + BatchLoader.for(project_id: project_id, severity: severity).batch(default_value: 0) do |items, loader| + project_ids = items.map { |i| i[:project_id] }.uniq + severities = items.map { |i| i[:severity] }.uniq + + counts = undismissed + .by_severities(severities) + .by_projects(project_ids) + .group(:project_id, :severity) + .count + + counts.each do |(found_project_id, found_severity), count| + loader_key = { project_id: found_project_id, severity: found_severity } + loader.call(loader_key, count) + end + end + end + def feedback(feedback_type:) params = { project_id: project_id, diff --git a/ee/app/presenters/security/vulnerable_project_presenter.rb b/ee/app/presenters/security/vulnerable_project_presenter.rb new file mode 100644 index 0000000000000000000000000000000000000000..c5d25cf7201dbbee2282cee5e07930f59dcf39f7 --- /dev/null +++ b/ee/app/presenters/security/vulnerable_project_presenter.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Security + class VulnerableProjectPresenter < ::Gitlab::View::Presenter::Delegated + SEVERITY_LEVELS = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys + + presents :project + + def initialize(project) + super(project, counts_for_project(project)) + end + + private + + def counts_for_project(project) + SEVERITY_LEVELS.each_with_object({}) do |severity, counts| + counts["#{severity}_vulnerability_count".to_sym] = ::Vulnerabilities::Occurrence.batch_count_by_project_and_severity(project.id, severity) + end + end + end +end diff --git a/ee/app/serializers/vulnerable_project_entity.rb b/ee/app/serializers/vulnerable_project_entity.rb new file mode 100644 index 0000000000000000000000000000000000000000..d691bf5cd500de4231d51ecf2fe711d95a7f3a61 --- /dev/null +++ b/ee/app/serializers/vulnerable_project_entity.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class VulnerableProjectEntity < ProjectEntity + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.each_key do |severity_level| + expose "#{severity_level}_vulnerability_count" + end +end diff --git a/ee/app/serializers/vulnerable_project_serializer.rb b/ee/app/serializers/vulnerable_project_serializer.rb new file mode 100644 index 0000000000000000000000000000000000000000..477b0344ba106591ec22ab9d57a37ea69739346f --- /dev/null +++ b/ee/app/serializers/vulnerable_project_serializer.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class VulnerableProjectSerializer < BaseSerializer + entity VulnerableProjectEntity +end diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index 87a4729556f2db10007d22b66f40404c385a9b71..f2f8b54c4da393a89371cd457d1409fe6650af22 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -110,6 +110,7 @@ namespace :security do resource :dashboard, only: [:show], controller: :dashboard + resources :vulnerable_projects, only: [:index] # We have to define both legacy and new routes for Vulnerability Findings # because they are loaded upon application initialization and preloaded by # web server. diff --git a/ee/spec/controllers/groups/security/vulnerable_projects_controller_spec.rb b/ee/spec/controllers/groups/security/vulnerable_projects_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..40513e730a5c8ab1e68ea4848939a22fc768b610 --- /dev/null +++ b/ee/spec/controllers/groups/security/vulnerable_projects_controller_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Groups::Security::VulnerableProjectsController do + let(:group) { create(:group) } + let(:user) { create(:user) } + + it_behaves_like SecurityDashboardsPermissions do + let(:vulnerable) { group } + let(:security_dashboard_action) { get :index, params: { group_id: group }, format: :json } + end + + describe '#index' do + before do + stub_licensed_features(security_dashboard: true) + + group.add_developer(user) + sign_in(user) + end + + subject { get :index, params: { group_id: group }, format: :json } + + it "responds with a list of the group's most vulnerable projects" do + _ungrouped_project = create(:project) + _safe_project = create(:project, namespace: group) + vulnerable_project = create(:project, namespace: group) + create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project, severity: :critical) + + subject + + expect(response).to have_gitlab_http_status(200) + expect(json_response.count).to be(1) + expect(json_response.first['id']).to eq(vulnerable_project.id) + expect(json_response.first['full_path']).to eq(project_path(vulnerable_project)) + expect(json_response.first['critical_vulnerability_count']).to eq(2) + end + + it 'does not include archived or deleted projects' do + archived_project = create(:project, :archived, namespace: group) + deleted_project = create(:project, namespace: group, pending_delete: true) + create(:vulnerabilities_occurrence, project: archived_project) + create(:vulnerabilities_occurrence, project: deleted_project) + + subject + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to be_empty + end + end +end diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index dcd9b9947753c06c68014b6c84de71763aa837b7..1e9413bc4ecdf5d2e33b22bbf76effb259d165d7 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -142,6 +142,47 @@ end end + describe '#vulnerable_projects' do + it "fetches the group's projects that have vulnerabilities" do + vulnerable_project = create(:project, namespace: group) + _safe_project = create(:project, namespace: group) + create(:vulnerabilities_occurrence, project: vulnerable_project) + + vulnerable_projects = group.vulnerable_projects + + expect(vulnerable_projects.count).to be(1) + expect(vulnerable_projects.first).to eq(vulnerable_project) + end + + it 'does not include projects that only have dismissed vulnerabilities' do + project = create(:project, namespace: group) + vulnerability = create(:vulnerabilities_occurrence, project: project) + create( + :vulnerability_feedback, + project_fingerprint: vulnerability.project_fingerprint, + feedback_type: :dismissal + ) + + vulnerable_projects = group.vulnerable_projects + + expect(vulnerable_projects).to be_empty + end + + it 'only uses 1 query' do + project_one = create(:project, namespace: group) + project_two = create(:project, namespace: group) + create(:vulnerabilities_occurrence, project: project_one) + dismissed_vulnerability = create(:vulnerabilities_occurrence, project: project_two) + create( + :vulnerability_feedback, + project_fingerprint: dismissed_vulnerability.project_fingerprint, + feedback_type: :dismissal + ) + + expect { group.vulnerable_projects }.not_to exceed_query_limit(1) + end + end + describe '#mark_ldap_sync_as_failed' do it 'sets the state to failed' do group.start_ldap_sync diff --git a/ee/spec/models/vulnerabilities/occurrence_spec.rb b/ee/spec/models/vulnerabilities/occurrence_spec.rb index 05bffacfd3899cc42492e293fcb6c3eefccb899a..3526e3a3e6eb407f7038794f92b6a2c281fb43cb 100644 --- a/ee/spec/models/vulnerabilities/occurrence_spec.rb +++ b/ee/spec/models/vulnerabilities/occurrence_spec.rb @@ -280,6 +280,71 @@ end end + describe '.undismissed' do + it 'returns occurrences that do not have a corresponding dismissal feedback' do + undismissed_occurrence = create(:vulnerabilities_occurrence) + dismissed_occurrence = create(:vulnerabilities_occurrence) + create(:vulnerability_feedback, project_fingerprint: dismissed_occurrence.project_fingerprint) + + expect(described_class.undismissed).to contain_exactly(undismissed_occurrence) + end + end + + describe '.batch_count_by_project_and_severity' do + let(:project) { create(:project) } + + it 'fetches a vulnerability count for the given project and severity' do + create(:vulnerabilities_occurrence, project: project, severity: :high) + + count = described_class.batch_count_by_project_and_severity(project.id, 'high') + + expect(count).to be(1) + end + + it 'returns 0 when there are no vulnerabilities for that severity level' do + count = described_class.batch_count_by_project_and_severity(project.id, 'high') + + expect(count).to be(0) + end + + it 'batch loads the counts' do + projects = create_list(:project, 2) + + projects.each do |project| + create(:vulnerabilities_occurrence, project: project, severity: :high) + create(:vulnerabilities_occurrence, project: project, severity: :low) + end + + projects_and_severities = [ + [projects.first, 'high'], + [projects.first, 'low'], + [projects.second, 'high'], + [projects.second, 'low'] + ] + + counts = projects_and_severities.map do |(project, severity)| + described_class.batch_count_by_project_and_severity(project.id, severity) + end + + expect { expect(counts).to all(be 1) }.not_to exceed_query_limit(1) + end + + it 'does not include dismissed vulnerabilities in the counts' do + create(:vulnerabilities_occurrence, project: project, severity: :high) + dismissed_vulnerability = create(:vulnerabilities_occurrence, project: project, severity: :high) + create( + :vulnerability_feedback, + project: project, + project_fingerprint: dismissed_vulnerability.project_fingerprint, + feedback_type: :dismissal + ) + + count = described_class.batch_count_by_project_and_severity(project.id, 'high') + + expect(count).to be(1) + end + end + describe 'feedback' do set(:project) { create(:project) } let(:occurrence) do diff --git a/ee/spec/presenters/security/vulnerable_project_presenter_spec.rb b/ee/spec/presenters/security/vulnerable_project_presenter_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..8ba9080ad0d57ce98946d9174321e219abc27316 --- /dev/null +++ b/ee/spec/presenters/security/vulnerable_project_presenter_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Security::VulnerableProjectPresenter do + SEVERITY_LEVELS = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys + + let(:project) { create(:project) } + + before do + allow(::Vulnerabilities::Occurrence).to receive(:batch_count_by_project_and_severity).and_return(1) + end + + subject { described_class.new(project) } + + it 'presents the given project' do + expect(subject.id).to be(project.id) + end + + SEVERITY_LEVELS.each do |severity_level| + it "exposes a vulnerability count attribute for #{severity_level} vulnerabilities" do + expect(subject.public_send("#{severity_level}_vulnerability_count")).to be(1) + end + end +end diff --git a/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb b/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..b34bd1ed70f354459f07842f2d9dee0bbf6cc77a --- /dev/null +++ b/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'GET /groups/*group_id/-/security/projects' do + let(:group) { create(:group) } + let(:user) { create(:user) } + + before do + stub_licensed_features(security_dashboard: true) + login_as(user) + + group.add_developer(user) + end + + it 'does not use N+1 queries' do + control_project = create(:project, namespace: group) + create(:vulnerabilities_occurrence, project: control_project) + + control_count = ActiveRecord::QueryRecorder.new do + get group_security_vulnerable_projects_path(group, format: :json) + end + + projects = create_list(:project, 2, namespace: group) + + projects.each do |project| + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity| + create(:vulnerabilities_occurrence, severity: severity, project: project) + end + end + + expect do + get group_security_vulnerable_projects_path(group, format: :json) + end.not_to exceed_query_limit(control_count) + + expect(response).to have_gitlab_http_status(200) + expect(json_response.size).to be(3) + end +end diff --git a/ee/spec/serializers/vulnerable_project_entity_spec.rb b/ee/spec/serializers/vulnerable_project_entity_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..bc55a9fd50c9a0a3b72b518b71905859de12ed8a --- /dev/null +++ b/ee/spec/serializers/vulnerable_project_entity_spec.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe VulnerableProjectEntity do + SEVERITY_LEVELS = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys + + let(:project) { create(:project) } + let(:vulnerable_project) { ::Security::VulnerableProjectPresenter.new(project) } + + before do + allow(::Vulnerabilities::Occurrence).to receive(:batch_count_by_project_and_severity).and_return(2) + end + + subject { described_class.new(vulnerable_project) } + + SEVERITY_LEVELS.each do |severity_level| + it "exposes a vulnerability count attribute for #{severity_level} vulnerabilities" do + expect(subject.as_json["#{severity_level}_vulnerability_count".to_sym]).to be(2) + end + end +end diff --git a/ee/spec/serializers/vulnerable_project_serializer_spec.rb b/ee/spec/serializers/vulnerable_project_serializer_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..e9065cd97d69547eed70d9af1354576108636062 --- /dev/null +++ b/ee/spec/serializers/vulnerable_project_serializer_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe VulnerableProjectSerializer do + let(:project) { create(:project) } + let(:serializer) { described_class.new(project: project, current_user: user) } + let(:user) { create(:user) } + let(:vulnerable_project) { ::Security::VulnerableProjectPresenter.new(project) } + + before do + project.add_developer(user) + + allow(::Vulnerabilities::Occurrence).to receive(:batch_count_by_project_and_severity) + end + + describe '#represent' do + subject { serializer.represent(vulnerable_project) } + + it 'includes counts for each severity of vulnerability' do + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| + expect(subject).to include("#{severity_level}_vulnerability_count".to_sym) + end + end + end +end