From 689a03452deaa571a36f35a1ac703110be93be83 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 7 Aug 2019 16:11:38 -0400 Subject: [PATCH 01/41] Add endpoint for a group's vulnerable projects * Adds `/group/:group_id/-/security/vulnerable_projects` which returns a list of the group's projects with vulnerabilities * Adds Vulnerabilities::Occurrence.batch_count_by_project_and_severity which batch loads vulnerability counts grouped by severity and by project * Adds Group#vulnerable_projects which returns a list of the group's vulnerable projects * Adds VulnerableProjectSerializer and VulnerableProjectEntity, which exposes a vulnerability count method for each severity * Adds VulnerableProjectPresenter which creates the vulnerability count method exposed in VulnerableProjectEntity https://gitlab.com/gitlab-org/gitlab-ee/issues/11190 --- .../groups/security/projects_controller.rb | 15 +++++++++++++++ ee/config/routes/group.rb | 3 ++- .../groups/security/projects_controller_spec.rb | 13 +++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) create mode 100644 ee/app/controllers/groups/security/projects_controller.rb create mode 100644 ee/spec/controllers/groups/security/projects_controller_spec.rb diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb new file mode 100644 index 00000000000000..ddb8d937667117 --- /dev/null +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class Groups::Security::ProjectsController < Groups::ApplicationController + include SecurityDashboardsPermissions + + alias_method :vulnerable, :group + + def index + respond_to do |format| + format.json do + render json: {} + end + end + end +end diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index 87a4729556f2db..64733f4b33f876 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -109,7 +109,8 @@ end namespace :security do - resource :dashboard, only: [:show], controller: :dashboard + resource :dashboard, only: [:show] + resources :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/projects_controller_spec.rb b/ee/spec/controllers/groups/security/projects_controller_spec.rb new file mode 100644 index 00000000000000..c7380648cb5550 --- /dev/null +++ b/ee/spec/controllers/groups/security/projects_controller_spec.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Groups::Security::ProjectsController 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 +end -- GitLab From 12cc5c54554a2ad1a2e62a005ef8c96d9c5ccca2 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 7 Aug 2019 19:11:27 -0400 Subject: [PATCH 02/41] Make it work Yay, a query! I'll be putting that in `Group` soon enough. --- .../groups/security/projects_controller.rb | 6 ++++- .../security/projects_controller_spec.rb | 24 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index ddb8d937667117..d1783bf550a5d2 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -6,9 +6,13 @@ class Groups::Security::ProjectsController < Groups::ApplicationController alias_method :vulnerable, :group def index + projects = Project.select( + 'projects.id, projects.name, projects.path, COUNT(vulnerability_occurrences) AS vulnerabilities_count' + ).left_outer_joins(:vulnerabilities).where(group: group).group('projects.id').order(vulnerabilities_count: :desc).limit(10) + respond_to do |format| format.json do - render json: {} + render json: projects end end end diff --git a/ee/spec/controllers/groups/security/projects_controller_spec.rb b/ee/spec/controllers/groups/security/projects_controller_spec.rb index c7380648cb5550..dd047f3cae9b79 100644 --- a/ee/spec/controllers/groups/security/projects_controller_spec.rb +++ b/ee/spec/controllers/groups/security/projects_controller_spec.rb @@ -10,4 +10,28 @@ let(:vulnerable) { group } let(:security_dashboard_action) { get :index, params: { group_id: group }, format: :json } end + + describe '#index' do + it "responds with an ordered list of the group's most vulnerable projects" do + sign_in(user) + stub_licensed_features(security_dashboard: true) + group.add_developer(user) + + _ungrouped_project = create(:project) + safe_project = create(:project, namespace: group) + vulnerable_project = create(:project, namespace: group) + create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project) + + get :index, params: { group_id: group }, format: :json + + expect(response).to have_gitlab_http_status(200) + expect(json_response.count).to be(2) + expect(json_response.first['id']).to eq(vulnerable_project.id) + expect(json_response.last['id']).to eq(safe_project.id) + end + + it "includes a link to the project's security dashboard" + it 'only counts high, critical, and unknown vulnerabilities' + it 'paginates the list by 10' + end end -- GitLab From 15cd370b177a8db8fb40552296fafa98f6649ab7 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 7 Aug 2019 19:44:16 -0400 Subject: [PATCH 03/41] Extract project query to Group Where it rightfully belongs --- .../groups/security/projects_controller.rb | 6 +----- ee/app/models/ee/group.rb | 8 ++++++++ ee/spec/models/group_spec.rb | 16 ++++++++++++++++ 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index d1783bf550a5d2..41f9d22a555d50 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -6,13 +6,9 @@ class Groups::Security::ProjectsController < Groups::ApplicationController alias_method :vulnerable, :group def index - projects = Project.select( - 'projects.id, projects.name, projects.path, COUNT(vulnerability_occurrences) AS vulnerabilities_count' - ).left_outer_joins(:vulnerabilities).where(group: group).group('projects.id').order(vulnerabilities_count: :desc).limit(10) - respond_to do |format| format.json do - render json: projects + render json: group.most_vulnerable_projects end end end diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 4b69ca6fef8d39..79436009b19ff1 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -118,6 +118,14 @@ def ip_restriction_ranges ip_restrictions.map(&:range).join(",") end + def most_vulnerable_projects + projects.select('projects.id, projects.name, projects.path, COUNT(vulnerability_occurrences) AS vulnerabilities_count') + .left_outer_joins(:vulnerabilities) + .group('projects.id') + .order('COUNT(vulnerability_occurrences) DESC') + .limit(10) + end + def human_ldap_access ::Gitlab::Access.options_with_owner.key(ldap_access) end diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index dcd9b9947753c0..4b772515dd17f4 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -142,6 +142,22 @@ end end + describe '#most_vulnerable_projects' do + it "returns the group's 10 projects with most vulnerabilities" do + create_list(:project, 9, namespace: group) + vulnerable_project = create(:project, namespace: group) + less_vulnerable_project = create(:project, namespace: group) + create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project) + create(:vulnerabilities_occurrence, project: less_vulnerable_project) + + vulnerable_projects = group.most_vulnerable_projects + + expect(vulnerable_projects.length).to be(10) + expect(vulnerable_projects.first).to eq(vulnerable_project) + expect(vulnerable_projects.second).to eq(less_vulnerable_project) + end + end + describe '#mark_ldap_sync_as_failed' do it 'sets the state to failed' do group.start_ldap_sync -- GitLab From ddab783dd974ce3315813882915b2615b054b1cb Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 8 Aug 2019 20:54:15 -0400 Subject: [PATCH 04/41] Fix broken specs I removed the controller specification for the group security dashboard route, and it turns out we need that. --- ee/config/routes/group.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index 64733f4b33f876..ebce15309092da 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -109,7 +109,7 @@ end namespace :security do - resource :dashboard, only: [:show] + resource :dashboard, only: [:show], controller: :dashboard resources :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 -- GitLab From 56e45fca63057526d3a4d7753d93fce2571844be Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 8 Aug 2019 21:13:31 -0400 Subject: [PATCH 05/41] Add some things Please don't be mad, Danger --- ee/app/models/ee/group.rb | 1 + .../groups/security/projects_controller_spec.rb | 1 - ee/spec/models/group_spec.rb | 16 ++++++++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 79436009b19ff1..7a9d5ad3b8ce8b 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -121,6 +121,7 @@ def ip_restriction_ranges def most_vulnerable_projects projects.select('projects.id, projects.name, projects.path, COUNT(vulnerability_occurrences) AS vulnerabilities_count') .left_outer_joins(:vulnerabilities) + .where('vulnerability_occurrences.severity IN (2, 6, 7)') .group('projects.id') .order('COUNT(vulnerability_occurrences) DESC') .limit(10) diff --git a/ee/spec/controllers/groups/security/projects_controller_spec.rb b/ee/spec/controllers/groups/security/projects_controller_spec.rb index dd047f3cae9b79..94bf8abdbe45d0 100644 --- a/ee/spec/controllers/groups/security/projects_controller_spec.rb +++ b/ee/spec/controllers/groups/security/projects_controller_spec.rb @@ -30,7 +30,6 @@ expect(json_response.last['id']).to eq(safe_project.id) end - it "includes a link to the project's security dashboard" it 'only counts high, critical, and unknown vulnerabilities' it 'paginates the list by 10' end diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index 4b772515dd17f4..4546e3827c1908 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -156,6 +156,22 @@ expect(vulnerable_projects.first).to eq(vulnerable_project) expect(vulnerable_projects.second).to eq(less_vulnerable_project) end + + it 'only counts high, critical, or unknown vulnerabilities' do + vulnerable_project = create(:project, namespace: group) + less_vulnerable_project = create(:project, namespace: group) + least_vulnerable_project = create(:project, namespace: group) + create(:vulnerabilities_occurrence, project: least_vulnerable_project, severity: :low) + create_list(:vulnerabilities_occurrence, 2, project: less_vulnerable_project, severity: :low) + create(:vulnerabilities_occurrence, project: less_vulnerable_project, severity: :high) + create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project, severity: :high) + + vulnerable_projects = group.most_vulnerable_projects + + expect(vulnerable_projects.first).to eq(vulnerable_project) + expect(vulnerable_projects.second).to eq(less_vulnerable_project) + expect(vulnerable_projects.third).to eq(least_vulnerable_project) + end end describe '#mark_ldap_sync_as_failed' do -- GitLab From 00bf59bdbf2b7e049963896b4da0f8f19477666f Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 13 Aug 2019 20:26:29 -0400 Subject: [PATCH 06/41] Use ProjectSerializer for response Now we have all the happy things we need to send via the API. --- ee/app/controllers/groups/security/projects_controller.rb | 2 +- ee/app/models/ee/group.rb | 4 ++-- .../controllers/groups/security/projects_controller_spec.rb | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 41f9d22a555d50..3e243dfff7409f 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -8,7 +8,7 @@ class Groups::Security::ProjectsController < Groups::ApplicationController def index respond_to do |format| format.json do - render json: group.most_vulnerable_projects + render json: ProjectSerializer.new.represent(group.most_vulnerable_projects) end end end diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 7a9d5ad3b8ce8b..ac8c17b8b94535 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -119,8 +119,8 @@ def ip_restriction_ranges end def most_vulnerable_projects - projects.select('projects.id, projects.name, projects.path, COUNT(vulnerability_occurrences) AS vulnerabilities_count') - .left_outer_joins(:vulnerabilities) + projects.select('projects.id, projects.name, projects.path, projects.namespace_id, COUNT(vulnerability_occurrences) AS vulnerabilities_count') + .joins(:vulnerabilities) .where('vulnerability_occurrences.severity IN (2, 6, 7)') .group('projects.id') .order('COUNT(vulnerability_occurrences) DESC') diff --git a/ee/spec/controllers/groups/security/projects_controller_spec.rb b/ee/spec/controllers/groups/security/projects_controller_spec.rb index 94bf8abdbe45d0..48fde4ccfa9093 100644 --- a/ee/spec/controllers/groups/security/projects_controller_spec.rb +++ b/ee/spec/controllers/groups/security/projects_controller_spec.rb @@ -18,16 +18,16 @@ group.add_developer(user) _ungrouped_project = create(:project) - safe_project = create(:project, namespace: group) + _safe_project = create(:project, namespace: group) vulnerable_project = create(:project, namespace: group) create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project) get :index, params: { group_id: group }, format: :json expect(response).to have_gitlab_http_status(200) - expect(json_response.count).to be(2) + expect(json_response.count).to be(1) expect(json_response.first['id']).to eq(vulnerable_project.id) - expect(json_response.last['id']).to eq(safe_project.id) + expect(json_response.first['full_path']).to eq(project_path(vulnerable_project)) end it 'only counts high, critical, and unknown vulnerabilities' -- GitLab From 0d45b4ce79e803568b39dd32d5bdaa58ad81e88c Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 16 Aug 2019 22:55:50 -0400 Subject: [PATCH 07/41] Add #vulnerable_projects to Group This method returns any projects that have undismissed vulnerabilities. --- ee/app/models/ee/group.rb | 13 ++++++------ ee/spec/models/group_spec.rb | 38 ++++++++++++++++-------------------- 2 files changed, 23 insertions(+), 28 deletions(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index ac8c17b8b94535..7cb3c1048652cc 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -118,13 +118,12 @@ def ip_restriction_ranges ip_restrictions.map(&:range).join(",") end - def most_vulnerable_projects - projects.select('projects.id, projects.name, projects.path, projects.namespace_id, COUNT(vulnerability_occurrences) AS vulnerabilities_count') - .joins(:vulnerabilities) - .where('vulnerability_occurrences.severity IN (2, 6, 7)') - .group('projects.id') - .order('COUNT(vulnerability_occurrences) DESC') - .limit(10) + def vulnerable_projects + projects.distinct.joins(:vulnerabilities).select do |project| + project.vulnerabilities.any? do |vulnerability| + !vulnerability.dismissal_feedback.present? + end + end end def human_ldap_access diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index 4546e3827c1908..3fd37388d9aa77 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -142,35 +142,31 @@ end end - describe '#most_vulnerable_projects' do - it "returns the group's 10 projects with most vulnerabilities" do - create_list(:project, 9, namespace: group) + describe '#vulnerable_projects' do + it "fetches the group's projects that have vulnerabilities" do vulnerable_project = create(:project, namespace: group) - less_vulnerable_project = create(:project, namespace: group) - create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project) - create(:vulnerabilities_occurrence, project: less_vulnerable_project) + _safe_project = create(:project, namespace: group) + create(:vulnerabilities_occurrence, project: vulnerable_project, severity: :high) + create(:vulnerabilities_occurrence, project: vulnerable_project, severity: :low) - vulnerable_projects = group.most_vulnerable_projects + vulnerable_projects = group.vulnerable_projects - expect(vulnerable_projects.length).to be(10) + expect(vulnerable_projects.count).to be(1) expect(vulnerable_projects.first).to eq(vulnerable_project) - expect(vulnerable_projects.second).to eq(less_vulnerable_project) end - it 'only counts high, critical, or unknown vulnerabilities' do - vulnerable_project = create(:project, namespace: group) - less_vulnerable_project = create(:project, namespace: group) - least_vulnerable_project = create(:project, namespace: group) - create(:vulnerabilities_occurrence, project: least_vulnerable_project, severity: :low) - create_list(:vulnerabilities_occurrence, 2, project: less_vulnerable_project, severity: :low) - create(:vulnerabilities_occurrence, project: less_vulnerable_project, severity: :high) - create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project, severity: :high) + 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: project, + project_fingerprint: vulnerability.project_fingerprint + ) - vulnerable_projects = group.most_vulnerable_projects + vulnerable_projects = group.vulnerable_projects - expect(vulnerable_projects.first).to eq(vulnerable_project) - expect(vulnerable_projects.second).to eq(less_vulnerable_project) - expect(vulnerable_projects.third).to eq(least_vulnerable_project) + expect(vulnerable_projects).to be_empty end end -- GitLab From e3302d49f7ce659b048e9720d36ae0e7051e5d8f Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Sat, 17 Aug 2019 00:00:33 -0400 Subject: [PATCH 08/41] Include vulnerability counts in response * Adds a method to `Project` for each vulnerability severity level that returns the vulnerability count * Exposes those methods in `::EE::ProjectEntity` --- app/serializers/project_entity.rb | 2 ++ .../groups/security/projects_controller.rb | 2 +- ee/app/models/ee/project.rb | 10 +++++++ ee/app/serializers/ee/project_entity.rb | 13 +++++++++ .../security/projects_controller_spec.rb | 8 +++--- ee/spec/models/project_spec.rb | 27 +++++++++++++++++++ ee/spec/serializers/ee/project_entity_spec.rb | 14 ++++++++++ 7 files changed, 70 insertions(+), 6 deletions(-) create mode 100644 ee/app/serializers/ee/project_entity.rb create mode 100644 ee/spec/serializers/ee/project_entity_spec.rb diff --git a/app/serializers/project_entity.rb b/app/serializers/project_entity.rb index 60c4ba135d6b4b..58fa91cea95854 100644 --- a/app/serializers/project_entity.rb +++ b/app/serializers/project_entity.rb @@ -3,6 +3,8 @@ class ProjectEntity < Grape::Entity include RequestAwareEntity + prepend ::EE::ProjectEntity # rubocop: disable Cop/InjectEnterpriseEditionModule + expose :id expose :name diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 3e243dfff7409f..72361bf1eefd5e 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -8,7 +8,7 @@ class Groups::Security::ProjectsController < Groups::ApplicationController def index respond_to do |format| format.json do - render json: ProjectSerializer.new.represent(group.most_vulnerable_projects) + render json: ProjectSerializer.new.represent(group.vulnerable_projects) end end end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index feeb57779b8db5..a45f90eda9ee55 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -651,6 +651,16 @@ def package_already_taken?(package_name) .exists? end + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| + method_name = "#{severity_level}_vulnerability_count" + + define_method(method_name) do + vulnerabilities.where(severity: severity_level).select do |vulnerability| + !vulnerability.dismissal_feedback.present? + end.length + end + end + private def set_override_pull_mirror_available diff --git a/ee/app/serializers/ee/project_entity.rb b/ee/app/serializers/ee/project_entity.rb new file mode 100644 index 00000000000000..d7810de4d4e693 --- /dev/null +++ b/ee/app/serializers/ee/project_entity.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module EE + module ProjectEntity + extend ActiveSupport::Concern + + prepended do + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| + expose "#{severity_level}_vulnerability_count" + end + end + end +end diff --git a/ee/spec/controllers/groups/security/projects_controller_spec.rb b/ee/spec/controllers/groups/security/projects_controller_spec.rb index 48fde4ccfa9093..f7fce7c7a26b16 100644 --- a/ee/spec/controllers/groups/security/projects_controller_spec.rb +++ b/ee/spec/controllers/groups/security/projects_controller_spec.rb @@ -12,7 +12,7 @@ end describe '#index' do - it "responds with an ordered list of the group's most vulnerable projects" do + it "responds with a list of the group's most vulnerable projects" do sign_in(user) stub_licensed_features(security_dashboard: true) group.add_developer(user) @@ -20,7 +20,7 @@ _ungrouped_project = create(:project) _safe_project = create(:project, namespace: group) vulnerable_project = create(:project, namespace: group) - create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project) + create_list(:vulnerabilities_occurrence, 2, project: vulnerable_project, severity: :critical) get :index, params: { group_id: group }, format: :json @@ -28,9 +28,7 @@ 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 'only counts high, critical, and unknown vulnerabilities' - it 'paginates the list by 10' end end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index afa423f1cf0a50..079e8715e9f027 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -2177,4 +2177,31 @@ def stub_default_url_options(host) end end end + + describe 'vulnerability counts by severity' do + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| + method_name = "#{severity_level}_vulnerability_count" + + describe "##{method_name}" do + it 'returns the number of undismissed vulnerabilities of that level' do + create_list(:vulnerabilities_occurrence, 2, project: project, severity: severity_level) + dismissed_vulnerability = create( + :vulnerabilities_occurrence, + project: project, + severity: severity_level + ) + create( + :vulnerability_feedback, + project: project, + project_fingerprint: dismissed_vulnerability.project_fingerprint, + feedback_type: :dismissal + ) + + count = project.public_send(method_name) + + expect(count).to be(2) + end + end + end + end end diff --git a/ee/spec/serializers/ee/project_entity_spec.rb b/ee/spec/serializers/ee/project_entity_spec.rb new file mode 100644 index 00000000000000..9e6d99aec531cf --- /dev/null +++ b/ee/spec/serializers/ee/project_entity_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ProjectEntity do + it 'exposes vulnerability count attributes' do + project = create(:project) + entity = described_class.new(project) + + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| + expect(entity.as_json).to include("#{severity_level}_vulnerability_count".to_sym) + end + end +end -- GitLab From 54f0a1dc5a257ba9d098ebf9c0c2ef58082216e0 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 30 Sep 2019 16:45:58 -0400 Subject: [PATCH 09/41] Add a changelog Add endpoint for a group's vulnerable projects --- changelogs/unreleased/most-affected-projects.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/most-affected-projects.yml diff --git a/changelogs/unreleased/most-affected-projects.yml b/changelogs/unreleased/most-affected-projects.yml new file mode 100644 index 00000000000000..1835f62e533884 --- /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 -- GitLab From ebae7c32ee73f5e2aec721812ce308828cd15005 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 30 Sep 2019 17:07:25 -0400 Subject: [PATCH 10/41] Create VulnerableProjectSerializer and Entity The vulnerability count methods that we need for this feature will increase the response time. In order to avoid affecting all serialized projects, it's better for the vulnerable projects to have their own serializer and entity. --- app/serializers/project_entity.rb | 2 -- .../groups/security/projects_controller.rb | 2 +- ee/app/serializers/ee/project_entity.rb | 13 ------------- ee/app/serializers/vulnerable_project_entity.rb | 9 +++++++++ ee/app/serializers/vulnerable_project_serializer.rb | 5 +++++ 5 files changed, 15 insertions(+), 16 deletions(-) delete mode 100644 ee/app/serializers/ee/project_entity.rb create mode 100644 ee/app/serializers/vulnerable_project_entity.rb create mode 100644 ee/app/serializers/vulnerable_project_serializer.rb diff --git a/app/serializers/project_entity.rb b/app/serializers/project_entity.rb index 58fa91cea95854..60c4ba135d6b4b 100644 --- a/app/serializers/project_entity.rb +++ b/app/serializers/project_entity.rb @@ -3,8 +3,6 @@ class ProjectEntity < Grape::Entity include RequestAwareEntity - prepend ::EE::ProjectEntity # rubocop: disable Cop/InjectEnterpriseEditionModule - expose :id expose :name diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 72361bf1eefd5e..e75794d558ee8f 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -8,7 +8,7 @@ class Groups::Security::ProjectsController < Groups::ApplicationController def index respond_to do |format| format.json do - render json: ProjectSerializer.new.represent(group.vulnerable_projects) + render json: VulnerableProjectSerializer.new.represent(group.vulnerable_projects) end end end diff --git a/ee/app/serializers/ee/project_entity.rb b/ee/app/serializers/ee/project_entity.rb deleted file mode 100644 index d7810de4d4e693..00000000000000 --- a/ee/app/serializers/ee/project_entity.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -module EE - module ProjectEntity - extend ActiveSupport::Concern - - prepended do - ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| - expose "#{severity_level}_vulnerability_count" - 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 00000000000000..d10ea970de661b --- /dev/null +++ b/ee/app/serializers/vulnerable_project_entity.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class VulnerableProjectEntity < ProjectEntity + extend ActiveSupport::Concern + + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each 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 00000000000000..477b0344ba1065 --- /dev/null +++ b/ee/app/serializers/vulnerable_project_serializer.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class VulnerableProjectSerializer < BaseSerializer + entity VulnerableProjectEntity +end -- GitLab From c126bd2d56133670821cd794e5c2df07db8e5afe Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 30 Sep 2019 17:15:51 -0400 Subject: [PATCH 11/41] Add specs for serializer and entity Vulnerable project serializer and entity, that is --- ...c.rb => vulnerable_project_entity_spec.rb} | 2 +- .../vulnerable_project_serializer_spec.rb | 23 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) rename ee/spec/serializers/{ee/project_entity_spec.rb => vulnerable_project_entity_spec.rb} (91%) create mode 100644 ee/spec/serializers/vulnerable_project_serializer_spec.rb diff --git a/ee/spec/serializers/ee/project_entity_spec.rb b/ee/spec/serializers/vulnerable_project_entity_spec.rb similarity index 91% rename from ee/spec/serializers/ee/project_entity_spec.rb rename to ee/spec/serializers/vulnerable_project_entity_spec.rb index 9e6d99aec531cf..9a85e3630ce0bd 100644 --- a/ee/spec/serializers/ee/project_entity_spec.rb +++ b/ee/spec/serializers/vulnerable_project_entity_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe ProjectEntity do +describe VulnerableProjectEntity do it 'exposes vulnerability count attributes' do project = create(:project) entity = described_class.new(project) 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 00000000000000..f83a1b3fc537dc --- /dev/null +++ b/ee/spec/serializers/vulnerable_project_serializer_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe VulnerableProjectSerializer do + let(:serializer) { described_class.new(project: project, current_user: user) } + let(:user) { create(:user) } + let(:project) { create(:project) } + + before do + project.add_developer(user) + end + + describe '#represent' do + subject { serializer.represent(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 -- GitLab From 697a5991830cb9776587b31ffb57ce6feb3e462a Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 4 Oct 2019 18:13:57 -0400 Subject: [PATCH 12/41] Simplify JSON response Much nicer! --- ee/app/controllers/groups/security/projects_controller.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index e75794d558ee8f..8a3b7a67290345 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -6,10 +6,6 @@ class Groups::Security::ProjectsController < Groups::ApplicationController alias_method :vulnerable, :group def index - respond_to do |format| - format.json do - render json: VulnerableProjectSerializer.new.represent(group.vulnerable_projects) - end - end + render json: VulnerableProjectSerializer.new.represent(group.vulnerable_projects) end end -- GitLab From 6537a9530c1fbb8ef34924e5da9e5fe850b3df0b Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 4 Oct 2019 18:32:02 -0400 Subject: [PATCH 13/41] Avoid N+1 queries Now the amount of variables is making me think about extracting a class...hmmmm... --- ee/app/models/ee/group.rb | 8 ++++++-- ee/spec/models/group_spec.rb | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 7cb3c1048652cc..d25c2719a3f446 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -119,9 +119,13 @@ def ip_restriction_ranges end def vulnerable_projects - projects.distinct.joins(:vulnerabilities).select do |project| + possibly_vulnerable_projects = projects.distinct.joins(:vulnerabilities).preload(:vulnerabilities) + vulnerabilities_fingerprints = possibly_vulnerable_projects.map { |project| project.vulnerabilities.map(&:project_fingerprint) }.flatten + dismissal_feedback_fingerprints = Vulnerabilities::Feedback.where(feedback_type: :dismissal, project_fingerprint: vulnerabilities_fingerprints) + + possibly_vulnerable_projects.select do |project| project.vulnerabilities.any? do |vulnerability| - !vulnerability.dismissal_feedback.present? + !dismissal_feedback_fingerprints.include?(vulnerability.project_fingerprint) end end end diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index 3fd37388d9aa77..7a7ffd5a4b8d0b 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -168,6 +168,25 @@ expect(vulnerable_projects).to be_empty end + + it 'does not create N+1 queries' do + project_one = create(:project, namespace: group) + project_two = create(:project, namespace: group) + create(:vulnerabilities_occurrence, project: project_one) + create(:vulnerabilities_occurrence, project: project_two) + dismissed_vulnerability = create(:vulnerabilities_occurrence, project: project_one) + create( + :vulnerability_feedback, + project: project_one, + project_fingerprint: dismissed_vulnerability.project_fingerprint + ) + + # 3 queries are expected: + # 1 to fetch projects with vulnerabilities + # 2 to preload the vulnerabilities + # 3 to fetch dismissal feedback + expect { group.vulnerable_projects }.not_to exceed_query_limit(3) + end end describe '#mark_ldap_sync_as_failed' do -- GitLab From bc365211ba48d40c5ba0cd2fa47ec0ca7f23364b Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 4 Oct 2019 22:35:26 +0000 Subject: [PATCH 14/41] Apply suggestion to ee/app/serializers/vulnerable_project_entity.rb --- ee/app/serializers/vulnerable_project_entity.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/serializers/vulnerable_project_entity.rb b/ee/app/serializers/vulnerable_project_entity.rb index d10ea970de661b..f2ddb18c18d28c 100644 --- a/ee/app/serializers/vulnerable_project_entity.rb +++ b/ee/app/serializers/vulnerable_project_entity.rb @@ -3,7 +3,7 @@ class VulnerableProjectEntity < ProjectEntity extend ActiveSupport::Concern - ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| + ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.each_key do |severity_level| expose "#{severity_level}_vulnerability_count" end end -- GitLab From ba71369f1b8f8ebae80c3072d6525eb3b946870c Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 7 Oct 2019 17:37:37 -0400 Subject: [PATCH 15/41] Update vulnerable projects to use new model With the recent introduction of the Vulnerability model, we can use its `state` attribute to determine whether a project is vulnerable much more easily. See https://gitlab.com/gitlab-org/gitlab/issues/10252#terminology for details. --- ee/app/models/ee/group.rb | 10 +--------- ee/spec/models/group_spec.rb | 31 +++---------------------------- 2 files changed, 4 insertions(+), 37 deletions(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index d25c2719a3f446..6b9d23bc7058e3 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -119,15 +119,7 @@ def ip_restriction_ranges end def vulnerable_projects - possibly_vulnerable_projects = projects.distinct.joins(:vulnerabilities).preload(:vulnerabilities) - vulnerabilities_fingerprints = possibly_vulnerable_projects.map { |project| project.vulnerabilities.map(&:project_fingerprint) }.flatten - dismissal_feedback_fingerprints = Vulnerabilities::Feedback.where(feedback_type: :dismissal, project_fingerprint: vulnerabilities_fingerprints) - - possibly_vulnerable_projects.select do |project| - project.vulnerabilities.any? do |vulnerability| - !dismissal_feedback_fingerprints.include?(vulnerability.project_fingerprint) - end - end + projects.distinct.joins(:vulnerabilities).where(vulnerabilities: { state: :opened }) end def human_ldap_access diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index 7a7ffd5a4b8d0b..af8e1f8b351d96 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -146,8 +146,7 @@ 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, severity: :high) - create(:vulnerabilities_occurrence, project: vulnerable_project, severity: :low) + create(:vulnerability, project: vulnerable_project, state: :opened) vulnerable_projects = group.vulnerable_projects @@ -155,38 +154,14 @@ expect(vulnerable_projects.first).to eq(vulnerable_project) end - it 'does not include projects that only have dismissed vulnerabilities' do + it 'does not include projects that only have closed vulnerabilities' do project = create(:project, namespace: group) - vulnerability = create(:vulnerabilities_occurrence, project: project) - create( - :vulnerability_feedback, - project: project, - project_fingerprint: vulnerability.project_fingerprint - ) + create(:vulnerability, project: project, state: :closed) vulnerable_projects = group.vulnerable_projects expect(vulnerable_projects).to be_empty end - - it 'does not create N+1 queries' do - project_one = create(:project, namespace: group) - project_two = create(:project, namespace: group) - create(:vulnerabilities_occurrence, project: project_one) - create(:vulnerabilities_occurrence, project: project_two) - dismissed_vulnerability = create(:vulnerabilities_occurrence, project: project_one) - create( - :vulnerability_feedback, - project: project_one, - project_fingerprint: dismissed_vulnerability.project_fingerprint - ) - - # 3 queries are expected: - # 1 to fetch projects with vulnerabilities - # 2 to preload the vulnerabilities - # 3 to fetch dismissal feedback - expect { group.vulnerable_projects }.not_to exceed_query_limit(3) - end end describe '#mark_ldap_sync_as_failed' do -- GitLab From c7f08c2bcbfd83d50dc5bb3222dc20d340124d85 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 9 Oct 2019 18:04:24 -0400 Subject: [PATCH 16/41] Do it in 1 query! Aw yeah Arel --- ee/app/models/ee/group.rb | 13 ++++++++++++- ee/spec/models/group_spec.rb | 25 ++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 6b9d23bc7058e3..97fa4c7864796e 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -119,7 +119,18 @@ def ip_restriction_ranges end def vulnerable_projects - projects.distinct.joins(:vulnerabilities).where(vulnerabilities: { state: :opened }) + occurrences_table = Vulnerabilities::Occurrence.arel_table + feedback_table = Vulnerabilities::Feedback.arel_table + escape_literal = Arel::Nodes.build_quoted('hex') + decoded_fingerprint = Arel::Nodes::NamedFunction.new('DECODE', [feedback_table[:project_fingerprint], escape_literal]) + + select = Arel::SelectManager.new(occurrences_table) + projects.distinct.joins(:vulnerability_findings).where( + select.project(Arel.star) + .join(feedback_table).on(occurrences_table[:project_fingerprint].eq(decoded_fingerprint)).where( + feedback_table[:feedback_type].eq(0) + ).exists.not + ) end def human_ldap_access diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index af8e1f8b351d96..cd7c22c39e9b12 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -146,7 +146,7 @@ it "fetches the group's projects that have vulnerabilities" do vulnerable_project = create(:project, namespace: group) _safe_project = create(:project, namespace: group) - create(:vulnerability, project: vulnerable_project, state: :opened) + create(:vulnerabilities_occurrence, project: vulnerable_project) vulnerable_projects = group.vulnerable_projects @@ -154,14 +154,33 @@ expect(vulnerable_projects.first).to eq(vulnerable_project) end - it 'does not include projects that only have closed vulnerabilities' do + it 'does not include projects that only have dismissed vulnerabilities' do project = create(:project, namespace: group) - create(:vulnerability, project: project, state: :closed) + 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 -- GitLab From bbb7c98be9282794890900880ba6ce9214dcd0ee Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Oct 2019 18:44:55 -0400 Subject: [PATCH 17/41] Extract VulnerableProjectsQuery class * Adds ee/app/queries directory --- ee/app/models/ee/group.rb | 15 ++---- ee/app/queries/vulnerable_projects_query.rb | 50 ++++++++++++++++++ .../queries/vulnerable_projects_query_spec.rb | 51 +++++++++++++++++++ lib/quality/test_level.rb | 1 + spec/lib/quality/test_level_spec.rb | 4 +- 5 files changed, 107 insertions(+), 14 deletions(-) create mode 100644 ee/app/queries/vulnerable_projects_query.rb create mode 100644 ee/spec/queries/vulnerable_projects_query_spec.rb diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 97fa4c7864796e..feef7e41d5dc1b 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "#{Rails.root}/ee/app/queries/vulnerable_projects_query" + module EE # Group EE mixin # @@ -119,18 +121,7 @@ def ip_restriction_ranges end def vulnerable_projects - occurrences_table = Vulnerabilities::Occurrence.arel_table - feedback_table = Vulnerabilities::Feedback.arel_table - escape_literal = Arel::Nodes.build_quoted('hex') - decoded_fingerprint = Arel::Nodes::NamedFunction.new('DECODE', [feedback_table[:project_fingerprint], escape_literal]) - - select = Arel::SelectManager.new(occurrences_table) - projects.distinct.joins(:vulnerability_findings).where( - select.project(Arel.star) - .join(feedback_table).on(occurrences_table[:project_fingerprint].eq(decoded_fingerprint)).where( - feedback_table[:feedback_type].eq(0) - ).exists.not - ) + ::VulnerableProjectsQuery.new(self).execute end def human_ldap_access diff --git a/ee/app/queries/vulnerable_projects_query.rb b/ee/app/queries/vulnerable_projects_query.rb new file mode 100644 index 00000000000000..74929ff4f5da4e --- /dev/null +++ b/ee/app/queries/vulnerable_projects_query.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +class VulnerableProjectsQuery + DECODE_FUNCTION = 'DECODE' + HEX = 'hex' + + def initialize(group) + @group = group + end + + def execute + group.projects.distinct.joins(:vulnerability_findings).where(vulnerabilities_not_dismissed) + end + + private + + attr_reader :group + + def vulnerabilities_not_dismissed + select_vulnerabilities.join(feedback_table).on(fingerprints_eq).where(dismissal_feedback).exists.not + end + + def fingerprints_eq + vulnerabilities_table[:project_fingerprint].eq(decoded_feedback_fingerprint) + end + + def decoded_feedback_fingerprint + Arel::Nodes::NamedFunction.new(DECODE_FUNCTION, [feedback_table[:project_fingerprint], decode_strategy]) + end + + def dismissal_feedback + feedback_table[:feedback_type].eq(::Vulnerabilities::Feedback.feedback_types[:dismissal]) + end + + def select_vulnerabilities + Arel::SelectManager.new(vulnerabilities_table) + end + + def vulnerabilities_table + ::Vulnerabilities::Occurrence.arel_table + end + + def feedback_table + ::Vulnerabilities::Feedback.arel_table + end + + def decode_strategy + Arel::Nodes.build_quoted(HEX) + end +end diff --git a/ee/spec/queries/vulnerable_projects_query_spec.rb b/ee/spec/queries/vulnerable_projects_query_spec.rb new file mode 100644 index 00000000000000..a96233c43527d1 --- /dev/null +++ b/ee/spec/queries/vulnerable_projects_query_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' +require "#{Rails.root}/ee/app/queries/vulnerable_projects_query" + +describe VulnerableProjectsQuery do + let(:group) { create(:group) } + + subject { described_class.new(group) } + + describe '#execute' do + it "fetches the given 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 = subject.execute + + 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 = subject.execute + + 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{ subject.execute }.not_to exceed_query_limit(1) + end + end +end diff --git a/lib/quality/test_level.rb b/lib/quality/test_level.rb index b7822adf6edb2e..1928df8a1f16d3 100644 --- a/lib/quality/test_level.rb +++ b/lib/quality/test_level.rb @@ -35,6 +35,7 @@ class TestLevel views workers elastic_integration + queries ], integration: %w[ controllers diff --git a/spec/lib/quality/test_level_spec.rb b/spec/lib/quality/test_level_spec.rb index 4db188bd8f2075..e6ee4aee075966 100644 --- a/spec/lib/quality/test_level_spec.rb +++ b/spec/lib/quality/test_level_spec.rb @@ -21,7 +21,7 @@ context 'when level is unit' do it 'returns a pattern' do expect(subject.pattern(:unit)) - .to eq("spec/{bin,config,db,dependencies,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,migrations,models,policies,presenters,rack_servers,routing,rubocop,serializers,services,sidekiq,tasks,uploaders,validators,views,workers,elastic_integration}{,/**/}*_spec.rb") + .to eq("spec/{bin,config,db,dependencies,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,migrations,models,policies,presenters,rack_servers,routing,rubocop,serializers,services,sidekiq,tasks,uploaders,validators,views,workers,elastic_integration,queries}{,/**/}*_spec.rb") end end @@ -75,7 +75,7 @@ context 'when level is unit' do it 'returns a regexp' do expect(subject.regexp(:unit)) - .to eq(%r{spec/(bin|config|db|dependencies|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|migrations|models|policies|presenters|rack_servers|routing|rubocop|serializers|services|sidekiq|tasks|uploaders|validators|views|workers|elastic_integration)}) + .to eq(%r{spec/(bin|config|db|dependencies|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|migrations|models|policies|presenters|rack_servers|routing|rubocop|serializers|services|sidekiq|tasks|uploaders|validators|views|workers|elastic_integration|queries)}) end end -- GitLab From a8d9586872fd4fe55e8cb5edd8fa4672086b02a7 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Oct 2019 18:48:10 -0400 Subject: [PATCH 18/41] Preload vulnerability findings Avoids making a query for each vulnerability findings count in the response. --- ee/app/controllers/groups/security/projects_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 8a3b7a67290345..dd24d5aece088c 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -6,6 +6,8 @@ class Groups::Security::ProjectsController < Groups::ApplicationController alias_method :vulnerable, :group def index - render json: VulnerableProjectSerializer.new.represent(group.vulnerable_projects) + render json: VulnerableProjectSerializer.new.represent( + group.vulnerable_projects.preload(:vulnerability_findings) + ) end end -- GitLab From 16bc7fc153663409e8fd74ced8bb3b88692f5dff Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 10 Oct 2019 18:54:03 -0400 Subject: [PATCH 19/41] Fix failing specs After the latest rebase from master, it is necessary to use the vulnerability_findings association, not vulnerabilities. This is the result of changes as part of the work for first class vulnerabilities. --- ee/app/models/ee/project.rb | 2 +- ee/spec/models/group_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index a45f90eda9ee55..29635259a0b178 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -655,7 +655,7 @@ def package_already_taken?(package_name) method_name = "#{severity_level}_vulnerability_count" define_method(method_name) do - vulnerabilities.where(severity: severity_level).select do |vulnerability| + vulnerability_findings.where(severity: severity_level).select do |vulnerability| !vulnerability.dismissal_feedback.present? end.length end diff --git a/ee/spec/models/group_spec.rb b/ee/spec/models/group_spec.rb index cd7c22c39e9b12..1e9413bc4ecdf5 100644 --- a/ee/spec/models/group_spec.rb +++ b/ee/spec/models/group_spec.rb @@ -179,7 +179,7 @@ feedback_type: :dismissal ) - expect{ group.vulnerable_projects }.not_to exceed_query_limit(1) + expect { group.vulnerable_projects }.not_to exceed_query_limit(1) end end -- GitLab From c0163f482540fa5c69d2b60896acab9a101566c3 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 15 Oct 2019 17:14:34 -0400 Subject: [PATCH 20/41] Revert "Extract VulnerableProjectsQuery class" This reverts commit 5601a8fa3f99f7eb6be123ade792692a196d1946. --- ee/app/models/ee/group.rb | 15 ++++-- ee/app/queries/vulnerable_projects_query.rb | 50 ------------------ .../queries/vulnerable_projects_query_spec.rb | 51 ------------------- lib/quality/test_level.rb | 1 - spec/lib/quality/test_level_spec.rb | 4 +- 5 files changed, 14 insertions(+), 107 deletions(-) delete mode 100644 ee/app/queries/vulnerable_projects_query.rb delete mode 100644 ee/spec/queries/vulnerable_projects_query_spec.rb diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index feef7e41d5dc1b..97fa4c7864796e 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require "#{Rails.root}/ee/app/queries/vulnerable_projects_query" - module EE # Group EE mixin # @@ -121,7 +119,18 @@ def ip_restriction_ranges end def vulnerable_projects - ::VulnerableProjectsQuery.new(self).execute + occurrences_table = Vulnerabilities::Occurrence.arel_table + feedback_table = Vulnerabilities::Feedback.arel_table + escape_literal = Arel::Nodes.build_quoted('hex') + decoded_fingerprint = Arel::Nodes::NamedFunction.new('DECODE', [feedback_table[:project_fingerprint], escape_literal]) + + select = Arel::SelectManager.new(occurrences_table) + projects.distinct.joins(:vulnerability_findings).where( + select.project(Arel.star) + .join(feedback_table).on(occurrences_table[:project_fingerprint].eq(decoded_fingerprint)).where( + feedback_table[:feedback_type].eq(0) + ).exists.not + ) end def human_ldap_access diff --git a/ee/app/queries/vulnerable_projects_query.rb b/ee/app/queries/vulnerable_projects_query.rb deleted file mode 100644 index 74929ff4f5da4e..00000000000000 --- a/ee/app/queries/vulnerable_projects_query.rb +++ /dev/null @@ -1,50 +0,0 @@ -# frozen_string_literal: true - -class VulnerableProjectsQuery - DECODE_FUNCTION = 'DECODE' - HEX = 'hex' - - def initialize(group) - @group = group - end - - def execute - group.projects.distinct.joins(:vulnerability_findings).where(vulnerabilities_not_dismissed) - end - - private - - attr_reader :group - - def vulnerabilities_not_dismissed - select_vulnerabilities.join(feedback_table).on(fingerprints_eq).where(dismissal_feedback).exists.not - end - - def fingerprints_eq - vulnerabilities_table[:project_fingerprint].eq(decoded_feedback_fingerprint) - end - - def decoded_feedback_fingerprint - Arel::Nodes::NamedFunction.new(DECODE_FUNCTION, [feedback_table[:project_fingerprint], decode_strategy]) - end - - def dismissal_feedback - feedback_table[:feedback_type].eq(::Vulnerabilities::Feedback.feedback_types[:dismissal]) - end - - def select_vulnerabilities - Arel::SelectManager.new(vulnerabilities_table) - end - - def vulnerabilities_table - ::Vulnerabilities::Occurrence.arel_table - end - - def feedback_table - ::Vulnerabilities::Feedback.arel_table - end - - def decode_strategy - Arel::Nodes.build_quoted(HEX) - end -end diff --git a/ee/spec/queries/vulnerable_projects_query_spec.rb b/ee/spec/queries/vulnerable_projects_query_spec.rb deleted file mode 100644 index a96233c43527d1..00000000000000 --- a/ee/spec/queries/vulnerable_projects_query_spec.rb +++ /dev/null @@ -1,51 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' -require "#{Rails.root}/ee/app/queries/vulnerable_projects_query" - -describe VulnerableProjectsQuery do - let(:group) { create(:group) } - - subject { described_class.new(group) } - - describe '#execute' do - it "fetches the given 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 = subject.execute - - 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 = subject.execute - - 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{ subject.execute }.not_to exceed_query_limit(1) - end - end -end diff --git a/lib/quality/test_level.rb b/lib/quality/test_level.rb index 1928df8a1f16d3..b7822adf6edb2e 100644 --- a/lib/quality/test_level.rb +++ b/lib/quality/test_level.rb @@ -35,7 +35,6 @@ class TestLevel views workers elastic_integration - queries ], integration: %w[ controllers diff --git a/spec/lib/quality/test_level_spec.rb b/spec/lib/quality/test_level_spec.rb index e6ee4aee075966..4db188bd8f2075 100644 --- a/spec/lib/quality/test_level_spec.rb +++ b/spec/lib/quality/test_level_spec.rb @@ -21,7 +21,7 @@ context 'when level is unit' do it 'returns a pattern' do expect(subject.pattern(:unit)) - .to eq("spec/{bin,config,db,dependencies,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,migrations,models,policies,presenters,rack_servers,routing,rubocop,serializers,services,sidekiq,tasks,uploaders,validators,views,workers,elastic_integration,queries}{,/**/}*_spec.rb") + .to eq("spec/{bin,config,db,dependencies,factories,finders,frontend,graphql,haml_lint,helpers,initializers,javascripts,lib,migrations,models,policies,presenters,rack_servers,routing,rubocop,serializers,services,sidekiq,tasks,uploaders,validators,views,workers,elastic_integration}{,/**/}*_spec.rb") end end @@ -75,7 +75,7 @@ context 'when level is unit' do it 'returns a regexp' do expect(subject.regexp(:unit)) - .to eq(%r{spec/(bin|config|db|dependencies|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|migrations|models|policies|presenters|rack_servers|routing|rubocop|serializers|services|sidekiq|tasks|uploaders|validators|views|workers|elastic_integration|queries)}) + .to eq(%r{spec/(bin|config|db|dependencies|factories|finders|frontend|graphql|haml_lint|helpers|initializers|javascripts|lib|migrations|models|policies|presenters|rack_servers|routing|rubocop|serializers|services|sidekiq|tasks|uploaders|validators|views|workers|elastic_integration)}) end end -- GitLab From d85846980648c2368067646162788a6a2b9b9fd5 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 15 Oct 2019 17:49:13 -0400 Subject: [PATCH 21/41] Extract .undimissed to Occurrence This will be a super handy little method. --- ee/app/models/ee/group.rb | 13 +------------ ee/app/models/vulnerabilities/occurrence.rb | 9 +++++++++ ee/spec/models/vulnerabilities/occurrence_spec.rb | 10 ++++++++++ 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 97fa4c7864796e..91e4fe87532373 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -119,18 +119,7 @@ def ip_restriction_ranges end def vulnerable_projects - occurrences_table = Vulnerabilities::Occurrence.arel_table - feedback_table = Vulnerabilities::Feedback.arel_table - escape_literal = Arel::Nodes.build_quoted('hex') - decoded_fingerprint = Arel::Nodes::NamedFunction.new('DECODE', [feedback_table[:project_fingerprint], escape_literal]) - - select = Arel::SelectManager.new(occurrences_table) - projects.distinct.joins(:vulnerability_findings).where( - select.project(Arel.star) - .join(feedback_table).on(occurrences_table[:project_fingerprint].eq(decoded_fingerprint)).where( - feedback_table[:feedback_type].eq(0) - ).exists.not - ) + projects.where("EXISTS(?)", ::Vulnerabilities::Occurrence.select(1).undismissed.where('vulnerability_occurrences.project_id = projects.id')) end def human_ldap_access diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index fdb5c1ad3b8ff2..8eaa4276a2cab6 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/ee/app/models/vulnerabilities/occurrence.rb @@ -139,6 +139,15 @@ def state end end + def self.undismissed + where( + "NOT EXISTS (?)", + Feedback.select(1) + .where("#{table_name}.project_fingerprint = decode(vulnerability_feedback.project_fingerprint, 'HEX')") + .where(feedback_type: :dismissal) + ) + end + def feedback(feedback_type:) params = { project_id: project_id, diff --git a/ee/spec/models/vulnerabilities/occurrence_spec.rb b/ee/spec/models/vulnerabilities/occurrence_spec.rb index 05bffacfd3899c..3f2a768340c3da 100644 --- a/ee/spec/models/vulnerabilities/occurrence_spec.rb +++ b/ee/spec/models/vulnerabilities/occurrence_spec.rb @@ -280,6 +280,16 @@ 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 'feedback' do set(:project) { create(:project) } let(:occurrence) do -- GitLab From 28b7c79c0dd30a4f9ef647644d4059f4bfd70ab1 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 15 Oct 2019 18:52:12 -0400 Subject: [PATCH 22/41] Extract vulnerability counts to entity The Project model is big, and we're only using the counts in this one endpoint. Let's avoid bloating it more if possible. --- ee/app/models/ee/project.rb | 10 ------- .../serializers/vulnerable_project_entity.rb | 6 ++++- ee/spec/models/project_spec.rb | 27 ------------------- .../vulnerable_project_entity_spec.rb | 26 ++++++++++++++---- 4 files changed, 26 insertions(+), 43 deletions(-) diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index 29635259a0b178..feeb57779b8db5 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -651,16 +651,6 @@ def package_already_taken?(package_name) .exists? end - ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| - method_name = "#{severity_level}_vulnerability_count" - - define_method(method_name) do - vulnerability_findings.where(severity: severity_level).select do |vulnerability| - !vulnerability.dismissal_feedback.present? - end.length - end - end - private def set_override_pull_mirror_available diff --git a/ee/app/serializers/vulnerable_project_entity.rb b/ee/app/serializers/vulnerable_project_entity.rb index f2ddb18c18d28c..b7775a6c25bb70 100644 --- a/ee/app/serializers/vulnerable_project_entity.rb +++ b/ee/app/serializers/vulnerable_project_entity.rb @@ -4,6 +4,10 @@ class VulnerableProjectEntity < ProjectEntity extend ActiveSupport::Concern ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.each_key do |severity_level| - expose "#{severity_level}_vulnerability_count" + expose "#{severity_level}_vulnerability_count" do |project| + project.vulnerability_findings.where(severity: severity_level).select do |vulnerability| + !vulnerability.dismissal_feedback.present? + end.length + end end end diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 079e8715e9f027..afa423f1cf0a50 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -2177,31 +2177,4 @@ def stub_default_url_options(host) end end end - - describe 'vulnerability counts by severity' do - ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| - method_name = "#{severity_level}_vulnerability_count" - - describe "##{method_name}" do - it 'returns the number of undismissed vulnerabilities of that level' do - create_list(:vulnerabilities_occurrence, 2, project: project, severity: severity_level) - dismissed_vulnerability = create( - :vulnerabilities_occurrence, - project: project, - severity: severity_level - ) - create( - :vulnerability_feedback, - project: project, - project_fingerprint: dismissed_vulnerability.project_fingerprint, - feedback_type: :dismissal - ) - - count = project.public_send(method_name) - - expect(count).to be(2) - end - end - end - end end diff --git a/ee/spec/serializers/vulnerable_project_entity_spec.rb b/ee/spec/serializers/vulnerable_project_entity_spec.rb index 9a85e3630ce0bd..99c2985222423c 100644 --- a/ee/spec/serializers/vulnerable_project_entity_spec.rb +++ b/ee/spec/serializers/vulnerable_project_entity_spec.rb @@ -3,12 +3,28 @@ require 'spec_helper' describe VulnerableProjectEntity do - it 'exposes vulnerability count attributes' do - project = create(:project) - entity = described_class.new(project) + SEVERITY_LEVELS = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys - ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| - expect(entity.as_json).to include("#{severity_level}_vulnerability_count".to_sym) + let(:project) { create(:project) } + + subject { described_class.new(project) } + + SEVERITY_LEVELS.each do |severity_level| + it "exposes a vulnerability count attribute for #{severity_level} vulnerabilities" do + create_list(:vulnerabilities_occurrence, 2, project: project, severity: severity_level) + dismissed_vulnerability = create( + :vulnerabilities_occurrence, + project: project, + severity: severity_level + ) + create( + :vulnerability_feedback, + project: project, + project_fingerprint: dismissed_vulnerability.project_fingerprint, + feedback_type: :dismissal + ) + + expect(subject.as_json["#{severity_level}_vulnerability_count".to_sym]).to be(2) end end end -- GitLab From 88689c1779aa32d8ab221545b2bdc732cf86e91e Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Wed, 16 Oct 2019 16:45:25 -0400 Subject: [PATCH 23/41] Fix rubocop errors We don't need the preload in the current form, and we can use existing scopes instead of relying on ActiveRecord methods directly. --- ee/app/controllers/groups/security/projects_controller.rb | 2 +- ee/app/serializers/vulnerable_project_entity.rb | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index dd24d5aece088c..f0991d1f107533 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -7,7 +7,7 @@ class Groups::Security::ProjectsController < Groups::ApplicationController def index render json: VulnerableProjectSerializer.new.represent( - group.vulnerable_projects.preload(:vulnerability_findings) + group.vulnerable_projects ) end end diff --git a/ee/app/serializers/vulnerable_project_entity.rb b/ee/app/serializers/vulnerable_project_entity.rb index b7775a6c25bb70..4bbd4873ae6bd3 100644 --- a/ee/app/serializers/vulnerable_project_entity.rb +++ b/ee/app/serializers/vulnerable_project_entity.rb @@ -5,9 +5,7 @@ class VulnerableProjectEntity < ProjectEntity ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.each_key do |severity_level| expose "#{severity_level}_vulnerability_count" do |project| - project.vulnerability_findings.where(severity: severity_level).select do |vulnerability| - !vulnerability.dismissal_feedback.present? - end.length + project.vulnerability_findings.undismissed.by_severities([severity_level]).count end end end -- GitLab From 01b29aed07c23f5b31f15b60b2456887b39cf3d2 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 17 Oct 2019 18:03:36 +0000 Subject: [PATCH 24/41] Apply suggestion to ee/app/models/vulnerabilities/occurrence.rb --- ee/app/models/vulnerabilities/occurrence.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index 8eaa4276a2cab6..d56c1a87879ada 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/ee/app/models/vulnerabilities/occurrence.rb @@ -144,7 +144,7 @@ def self.undismissed "NOT EXISTS (?)", Feedback.select(1) .where("#{table_name}.project_fingerprint = decode(vulnerability_feedback.project_fingerprint, 'HEX')") - .where(feedback_type: :dismissal) + .for_dismissal ) end -- GitLab From 52f6e918867133c0c1aa0db5ebec0b7cb268d3b5 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 17 Oct 2019 14:49:30 -0400 Subject: [PATCH 25/41] Encode instead of decode On gitlab.com, there are some junky feedback fingerprints (probably from testing) that cause DECODE to fail. Since occurrences fingerprints are binary instead of a string, there is less likely to be such junky testing data, so I've switched to ENCODE-ing the occurrence's fingerprint. --- ee/app/models/vulnerabilities/occurrence.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index d56c1a87879ada..99639027153858 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/ee/app/models/vulnerabilities/occurrence.rb @@ -143,7 +143,7 @@ def self.undismissed where( "NOT EXISTS (?)", Feedback.select(1) - .where("#{table_name}.project_fingerprint = decode(vulnerability_feedback.project_fingerprint, 'HEX')") + .where("ENCODE(#{table_name}.project_fingerprint, 'HEX') = vulnerability_feedback.project_fingerprint") .for_dismissal ) end -- GitLab From 545f4e44a609a2452bb1df33285328d6865c7add Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 17 Oct 2019 18:08:48 -0400 Subject: [PATCH 26/41] Add N+1 spec I'll probably need to iterate on this spec, but for now it will allow me to check whether I succeed in removing the N+1 query created by fetching the vulnerability counts. --- .../groups/security/projects_index_spec.rb | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 ee/spec/requests/groups/security/projects_index_spec.rb diff --git a/ee/spec/requests/groups/security/projects_index_spec.rb b/ee/spec/requests/groups/security/projects_index_spec.rb new file mode 100644 index 00000000000000..cbcc857ae649b5 --- /dev/null +++ b/ee/spec/requests/groups/security/projects_index_spec.rb @@ -0,0 +1,36 @@ +# 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_count = ActiveRecord::QueryRecorder.new do + get group_security_projects_path(group, format: :json) + end.count + + 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_projects_path(group, format: :json) + + expect(response).to have_gitlab_http_status(200) + expect(json_response.size).to be(2) + end.not_to exceed_query_limit(control_count) + end +end -- GitLab From 7495bab480e7becef1ddd69c95470666cf72cb9f Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Thu, 17 Oct 2019 21:26:39 -0400 Subject: [PATCH 27/41] Implement naive batch loading There's too much logic in the controller, I should maybe use a finder, and VulnerableProject is misnamed and in the wrong place and feels hacky, but it's working and there's no N+1! --- .../groups/security/projects_controller.rb | 16 +++++++++-- ee/app/models/vulnerabilities/occurrence.rb | 18 ++++++++++++ ee/app/models/vulnerable_project.rb | 28 +++++++++++++++++++ .../serializers/vulnerable_project_entity.rb | 4 +-- .../groups/security/projects_index_spec.rb | 3 ++ 5 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 ee/app/models/vulnerable_project.rb diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index f0991d1f107533..16b11ae0fd2400 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -6,8 +6,18 @@ class Groups::Security::ProjectsController < Groups::ApplicationController alias_method :vulnerable, :group def index - render json: VulnerableProjectSerializer.new.represent( - group.vulnerable_projects - ) + projects = group.vulnerable_projects + + vulnerable_projects = projects.map do |project| + vulnerability_counts = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.inject({}) do |counts, severity| + counts["#{severity}_vulnerability_count"] = ::Vulnerabilities::Occurrence.count_by_severity(project.id, severity) + + counts + end + + VulnerableProject.new(project, vulnerability_counts) + end + + render json: VulnerableProjectSerializer.new.represent(vulnerable_projects) end end diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index 99639027153858..da7f688957a06b 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/ee/app/models/vulnerabilities/occurrence.rb @@ -148,6 +148,24 @@ def self.undismissed ) end + def self.count_by_severity(project_id, severity_level) + BatchLoader.for(project_id: project_id, severity_level: severity_level).batch do |items, loader| + project_ids = items.map { |i| i[:project_id] } + severity_levels = items.map { |i| i[:severity_level] } + + counts = undismissed + .by_severities(severity_levels) + .by_projects(project_ids) + .group(:project_id, :severity) + .count + + counts.each do |(found_project_id, found_severity_level), count| + loader_key = { project_id: found_project_id, severity_level: found_severity_level } + loader.call(loader_key, count || 0) + end + end + end + def feedback(feedback_type:) params = { project_id: project_id, diff --git a/ee/app/models/vulnerable_project.rb b/ee/app/models/vulnerable_project.rb new file mode 100644 index 00000000000000..ea5eb328f86236 --- /dev/null +++ b/ee/app/models/vulnerable_project.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class VulnerableProject < SimpleDelegator + def initialize(object, attrs) + @attrs = attrs.transform_keys do |key| + if key.is_a?(String) + key + else + key.to_s + end + end + p @attrs + + super(object) + end + + def method_missing(method_name, *arguments, &block) + if @attrs.has_key?(method_name.to_s) + @attrs[method_name] + else + super + end + end + + def respond_to_missing?(method_name, include_private = false) + @attrs.has_key?(method_name.to_s) || super + end +end diff --git a/ee/app/serializers/vulnerable_project_entity.rb b/ee/app/serializers/vulnerable_project_entity.rb index 4bbd4873ae6bd3..f2ddb18c18d28c 100644 --- a/ee/app/serializers/vulnerable_project_entity.rb +++ b/ee/app/serializers/vulnerable_project_entity.rb @@ -4,8 +4,6 @@ class VulnerableProjectEntity < ProjectEntity extend ActiveSupport::Concern ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.each_key do |severity_level| - expose "#{severity_level}_vulnerability_count" do |project| - project.vulnerability_findings.undismissed.by_severities([severity_level]).count - end + expose "#{severity_level}_vulnerability_count" end end diff --git a/ee/spec/requests/groups/security/projects_index_spec.rb b/ee/spec/requests/groups/security/projects_index_spec.rb index cbcc857ae649b5..b3f4c644b8ad56 100644 --- a/ee/spec/requests/groups/security/projects_index_spec.rb +++ b/ee/spec/requests/groups/security/projects_index_spec.rb @@ -14,6 +14,9 @@ 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_projects_path(group, format: :json) end.count -- GitLab From b63d290f2a93f414f66f679d11e8fa22c83acb56 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 18 Oct 2019 16:54:08 -0400 Subject: [PATCH 28/41] Extract and test SimpleDelegatorStruct Hopefully other people think this is as neat as I do :P --- .../groups/security/projects_controller.rb | 2 +- ee/app/models/vulnerable_project.rb | 28 ---------------- ee/lib/gitlab/simple_delegator_struct.rb | 33 +++++++++++++++++++ .../gitlab/simple_delegator_struct_spec.rb | 28 ++++++++++++++++ 4 files changed, 62 insertions(+), 29 deletions(-) delete mode 100644 ee/app/models/vulnerable_project.rb create mode 100644 ee/lib/gitlab/simple_delegator_struct.rb create mode 100644 ee/spec/lib/gitlab/simple_delegator_struct_spec.rb diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 16b11ae0fd2400..5044aa45b16926 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -15,7 +15,7 @@ def index counts end - VulnerableProject.new(project, vulnerability_counts) + SimpleDelegatorStruct.new(project, vulnerability_counts) end render json: VulnerableProjectSerializer.new.represent(vulnerable_projects) diff --git a/ee/app/models/vulnerable_project.rb b/ee/app/models/vulnerable_project.rb deleted file mode 100644 index ea5eb328f86236..00000000000000 --- a/ee/app/models/vulnerable_project.rb +++ /dev/null @@ -1,28 +0,0 @@ -# frozen_string_literal: true - -class VulnerableProject < SimpleDelegator - def initialize(object, attrs) - @attrs = attrs.transform_keys do |key| - if key.is_a?(String) - key - else - key.to_s - end - end - p @attrs - - super(object) - end - - def method_missing(method_name, *arguments, &block) - if @attrs.has_key?(method_name.to_s) - @attrs[method_name] - else - super - end - end - - def respond_to_missing?(method_name, include_private = false) - @attrs.has_key?(method_name.to_s) || super - end -end diff --git a/ee/lib/gitlab/simple_delegator_struct.rb b/ee/lib/gitlab/simple_delegator_struct.rb new file mode 100644 index 00000000000000..375f36fec4981c --- /dev/null +++ b/ee/lib/gitlab/simple_delegator_struct.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class SimpleDelegatorStruct < SimpleDelegator + def initialize(object, struct_attrs = {}) + @struct_attrs = struct_attrs.transform_keys do |key| + if key.is_a?(String) + key + else + key.to_s + end + end + + super(object) + end + + def method_missing(method_name, *arguments, &block) + method_name = method_name.to_s + + if struct_attrs.has_key?(method_name) + struct_attrs[method_name] + else + super + end + end + + def respond_to_missing?(method_name, include_private = false) + struct_attrs.has_key?(method_name.to_s) || super + end + + private + + attr_reader :struct_attrs +end diff --git a/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb b/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb new file mode 100644 index 00000000000000..aeab46198ecf84 --- /dev/null +++ b/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe SimpleDelegatorStruct do + class TestWitch + def hex + 'I bind and confound you!' + end + end + + let(:struct_attrs) { { heal: 'Bones mend and wounds stitch', 'scry' => 'I see you!' } } + let(:witch) { TestWitch.new } + + subject { described_class.new(witch, struct_attrs) } + + it 'acts like SimpleDelegator on the first argument to its constructor' do + expect(subject.hex).to eq('I bind and confound you!') + end + + it 'acts like OpenStruct on the second argument to its constructor' do + expect(subject.heal).to eq('Bones mend and wounds stitch') + end + + it 'acts like OpenStruct even on string keys' do + expect(subject.scry).to eq('I see you!') + end +end -- GitLab From 9e020cbec6856370ae92ff2e9d7633fdc382e460 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 18 Oct 2019 17:07:17 -0400 Subject: [PATCH 29/41] Update VulnerableProjectEntity spec I hope it's not too confusing to folks where the count methods come from. --- .../vulnerable_project_entity_spec.rb | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/ee/spec/serializers/vulnerable_project_entity_spec.rb b/ee/spec/serializers/vulnerable_project_entity_spec.rb index 99c2985222423c..1d452a9eae362e 100644 --- a/ee/spec/serializers/vulnerable_project_entity_spec.rb +++ b/ee/spec/serializers/vulnerable_project_entity_spec.rb @@ -6,24 +6,20 @@ SEVERITY_LEVELS = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys let(:project) { create(:project) } + let(:vulnerable_project) do + vulnerability_counts = SEVERITY_LEVELS.inject({}) do |counts, severity| + counts["#{severity}_vulnerability_count"] = 2 - subject { described_class.new(project) } + counts + end + + SimpleDelegatorStruct.new(project, vulnerability_counts) + end + + subject { described_class.new(vulnerable_project) } SEVERITY_LEVELS.each do |severity_level| it "exposes a vulnerability count attribute for #{severity_level} vulnerabilities" do - create_list(:vulnerabilities_occurrence, 2, project: project, severity: severity_level) - dismissed_vulnerability = create( - :vulnerabilities_occurrence, - project: project, - severity: severity_level - ) - create( - :vulnerability_feedback, - project: project, - project_fingerprint: dismissed_vulnerability.project_fingerprint, - feedback_type: :dismissal - ) - expect(subject.as_json["#{severity_level}_vulnerability_count".to_sym]).to be(2) end end -- GitLab From fac20f3ff422dba5a6af431db3a17bc6ecd93b64 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 18 Oct 2019 18:18:51 -0400 Subject: [PATCH 30/41] Extract VulnerabilityCountsFinder class This class batch loads vulnerabilities grouped by severity and project. It returns the counts as "[severity]_vulnerability_count" methods on an object that delegates all other methods to the given project. --- .../groups/security/projects_controller.rb | 11 +--- .../security/vulnerability_counts_finder.rb | 65 +++++++++++++++++++ ee/app/models/vulnerabilities/occurrence.rb | 18 ----- .../vulnerability_counts_finder_spec.rb | 44 +++++++++++++ 4 files changed, 110 insertions(+), 28 deletions(-) create mode 100644 ee/app/finders/security/vulnerability_counts_finder.rb create mode 100644 ee/spec/finders/security/vulnerability_counts_finder_spec.rb diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 5044aa45b16926..5405b33de091ae 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -7,16 +7,7 @@ class Groups::Security::ProjectsController < Groups::ApplicationController def index projects = group.vulnerable_projects - - vulnerable_projects = projects.map do |project| - vulnerability_counts = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.inject({}) do |counts, severity| - counts["#{severity}_vulnerability_count"] = ::Vulnerabilities::Occurrence.count_by_severity(project.id, severity) - - counts - end - - SimpleDelegatorStruct.new(project, vulnerability_counts) - end + vulnerable_projects = ::Security::VulnerabilityCountsFinder.new(projects).execute render json: VulnerableProjectSerializer.new.represent(vulnerable_projects) end diff --git a/ee/app/finders/security/vulnerability_counts_finder.rb b/ee/app/finders/security/vulnerability_counts_finder.rb new file mode 100644 index 00000000000000..a00c82719225a6 --- /dev/null +++ b/ee/app/finders/security/vulnerability_counts_finder.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +# Security::VulnerabilityCountsFinder +# +# Used to fetch counts for each severity of Vulnerabilities::Occurrences for a given list of projects +# +# Arguments: +# projects - Array (or ActiveRecord::Association) +# +# Return: +# Array where the delegated to object is Project and the struct attributes +# are of the form `[severity_level]_vulnerability_count. The complete list of severity level names +# can be found in ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys +# +# Example: +# ``` +# project = Project.find(1) # this project has 1 high severity vulnerability +# vulnerable_projects = VulnerabilityCountsFinder.new([project]).execute +# +# vulnerable_projects.first.high_vulnerability_count # 1 +# ``` + +module Security + class VulnerabilityCountsFinder + SEVERITIES = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys + + def initialize(projects) + @projects = projects + end + + def execute + projects.map do |project| + vulnerability_counts = SEVERITIES.inject({}) do |counts, severity| + counts["#{severity}_vulnerability_count"] = count_by_severity(project.id, severity) + + counts + end + + SimpleDelegatorStruct.new(project, vulnerability_counts) + end + end + + private + + attr_reader :projects + + def count_by_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] } + severities = items.map { |i| i[:severity] } + + counts = ::Vulnerabilities::Occurrence.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 + end +end diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index da7f688957a06b..99639027153858 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/ee/app/models/vulnerabilities/occurrence.rb @@ -148,24 +148,6 @@ def self.undismissed ) end - def self.count_by_severity(project_id, severity_level) - BatchLoader.for(project_id: project_id, severity_level: severity_level).batch do |items, loader| - project_ids = items.map { |i| i[:project_id] } - severity_levels = items.map { |i| i[:severity_level] } - - counts = undismissed - .by_severities(severity_levels) - .by_projects(project_ids) - .group(:project_id, :severity) - .count - - counts.each do |(found_project_id, found_severity_level), count| - loader_key = { project_id: found_project_id, severity_level: found_severity_level } - loader.call(loader_key, count || 0) - end - end - end - def feedback(feedback_type:) params = { project_id: project_id, diff --git a/ee/spec/finders/security/vulnerability_counts_finder_spec.rb b/ee/spec/finders/security/vulnerability_counts_finder_spec.rb new file mode 100644 index 00000000000000..42c62a050d3d54 --- /dev/null +++ b/ee/spec/finders/security/vulnerability_counts_finder_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ::Security::VulnerabilityCountsFinder do + let(:project) { create(:project) } + + describe '#execute' do + it 'fetches vulnerability counts for each given project and adds them to the project' do + create(:vulnerabilities_occurrence, project: project, severity: :high) + + vulnerable_projects = described_class.new([project]).execute + + expect(vulnerable_projects.first.high_vulnerability_count).to be(1) + expect(vulnerable_projects.first.critical_vulnerability_count).to be(0) + end + + it 'only uses 1 query' 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 + + expect { described_class.new(projects).execute }.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 + ) + + vulnerable_projects = described_class.new([project]).execute + + expect(vulnerable_projects.first.high_vulnerability_count).to be(1) + end + end +end -- GitLab From 26bfe57fbb10ef10c1f28ae36791025e877b6498 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 18 Oct 2019 18:37:30 -0400 Subject: [PATCH 31/41] Namespace and document SimpleDelegatorStruct Oops yeah it should be in the Gitlab module. --- .../security/vulnerability_counts_finder.rb | 2 +- ee/lib/gitlab/simple_delegator_struct.rb | 66 ++++++++++++------- .../gitlab/simple_delegator_struct_spec.rb | 2 +- 3 files changed, 46 insertions(+), 24 deletions(-) diff --git a/ee/app/finders/security/vulnerability_counts_finder.rb b/ee/app/finders/security/vulnerability_counts_finder.rb index a00c82719225a6..c70b954eaa65f4 100644 --- a/ee/app/finders/security/vulnerability_counts_finder.rb +++ b/ee/app/finders/security/vulnerability_counts_finder.rb @@ -36,7 +36,7 @@ def execute counts end - SimpleDelegatorStruct.new(project, vulnerability_counts) + ::Gitlab::SimpleDelegatorStruct.new(project, vulnerability_counts) end end diff --git a/ee/lib/gitlab/simple_delegator_struct.rb b/ee/lib/gitlab/simple_delegator_struct.rb index 375f36fec4981c..7f1ef118f455ff 100644 --- a/ee/lib/gitlab/simple_delegator_struct.rb +++ b/ee/lib/gitlab/simple_delegator_struct.rb @@ -1,33 +1,55 @@ # frozen_string_literal: true -class SimpleDelegatorStruct < SimpleDelegator - def initialize(object, struct_attrs = {}) - @struct_attrs = struct_attrs.transform_keys do |key| - if key.is_a?(String) - key - else - key.to_s +# Gitlab::SimpleDelegatorStruct +# +# Used to temporarily add simple data methods to an existing class. +# Functions as a combination of the SimpleDelegator and OpenStruct concepts. +# +# Arguments: +# object - Object +# struct_attrs - Hash (works with both symbol and string keys) +# +# Example: +# ``` +# user = User.find(1) +# struct_attrs = { nickname: 'Satie' } +# +# delegator_struct = SimpleDelegatorStruct.new(user, struct_attrs) +# +# delegator_struct.id # 1 +# delegator_struct.nickname # Satie +# ``` + +module Gitlab + class SimpleDelegatorStruct < SimpleDelegator + def initialize(object, struct_attrs = {}) + @struct_attrs = struct_attrs.transform_keys do |key| + if key.is_a?(String) + key + else + key.to_s + end end - end - super(object) - end + super(object) + end - def method_missing(method_name, *arguments, &block) - method_name = method_name.to_s + def method_missing(method_name, *arguments, &block) + method_name = method_name.to_s - if struct_attrs.has_key?(method_name) - struct_attrs[method_name] - else - super + if struct_attrs.has_key?(method_name) + struct_attrs[method_name] + else + super + end end - end - def respond_to_missing?(method_name, include_private = false) - struct_attrs.has_key?(method_name.to_s) || super - end + def respond_to_missing?(method_name, include_private = false) + struct_attrs.has_key?(method_name.to_s) || super + end - private + private - attr_reader :struct_attrs + attr_reader :struct_attrs + end end diff --git a/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb b/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb index aeab46198ecf84..f9772d5bbcff2a 100644 --- a/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb +++ b/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe SimpleDelegatorStruct do +describe ::Gitlab::SimpleDelegatorStruct do class TestWitch def hex 'I bind and confound you!' -- GitLab From d52d78b5649e0da25bc5a8f2c827d9e4dfc699aa Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 18 Oct 2019 19:12:00 -0400 Subject: [PATCH 32/41] Fix entity specs I forgot that one --- ee/spec/serializers/vulnerable_project_entity_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/serializers/vulnerable_project_entity_spec.rb b/ee/spec/serializers/vulnerable_project_entity_spec.rb index 1d452a9eae362e..323d6c4b3c4660 100644 --- a/ee/spec/serializers/vulnerable_project_entity_spec.rb +++ b/ee/spec/serializers/vulnerable_project_entity_spec.rb @@ -13,7 +13,7 @@ counts end - SimpleDelegatorStruct.new(project, vulnerability_counts) + ::Gitlab::SimpleDelegatorStruct.new(project, vulnerability_counts) end subject { described_class.new(vulnerable_project) } -- GitLab From a498bd6066d308496a5252ebd07a476130be0fc3 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 21 Oct 2019 18:09:27 +0000 Subject: [PATCH 33/41] Apply suggestion to ee/app/finders/security/vulnerability_counts_finder.rb --- ee/app/finders/security/vulnerability_counts_finder.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/finders/security/vulnerability_counts_finder.rb b/ee/app/finders/security/vulnerability_counts_finder.rb index c70b954eaa65f4..32a6d609588601 100644 --- a/ee/app/finders/security/vulnerability_counts_finder.rb +++ b/ee/app/finders/security/vulnerability_counts_finder.rb @@ -46,8 +46,8 @@ def execute def count_by_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] } - severities = items.map { |i| i[:severity] } + project_ids = items.map { |i| i[:project_id] }.uniq + severities = items.map { |i| i[:severity] }.uniq counts = ::Vulnerabilities::Occurrence.undismissed .by_severities(severities) -- GitLab From 80481db05019604a74d836685ba8db53267bba13 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 25 Oct 2019 13:44:46 -0400 Subject: [PATCH 34/41] Move query to model and use presenter * Removes VulnerabilityCountsFinder * Removes SimpleDelegatorStruct * Adds VulnerableProjectPresenter * Adds Occurrence.batch_count_by_project_and_severity Keeping the query logic in the model and using a presenter brings this code in line with Gitlab's current patterns and standards. --- .../groups/security/projects_controller.rb | 5 +- .../security/vulnerability_counts_finder.rb | 65 ------------------- ee/app/models/vulnerabilities/occurrence.rb | 20 +++++- .../security/vulnerable_project_presenter.rb | 21 ++++++ ee/lib/gitlab/simple_delegator_struct.rb | 55 ---------------- .../vulnerability_counts_finder_spec.rb | 44 ------------- .../gitlab/simple_delegator_struct_spec.rb | 28 -------- .../models/vulnerabilities/occurrence_spec.rb | 55 ++++++++++++++++ .../vulnerable_project_presenter_spec.rb | 25 +++++++ .../vulnerable_project_entity_spec.rb | 10 +-- .../vulnerable_project_serializer_spec.rb | 7 +- 11 files changed, 132 insertions(+), 203 deletions(-) delete mode 100644 ee/app/finders/security/vulnerability_counts_finder.rb create mode 100644 ee/app/presenters/security/vulnerable_project_presenter.rb delete mode 100644 ee/lib/gitlab/simple_delegator_struct.rb delete mode 100644 ee/spec/finders/security/vulnerability_counts_finder_spec.rb delete mode 100644 ee/spec/lib/gitlab/simple_delegator_struct_spec.rb create mode 100644 ee/spec/presenters/security/vulnerable_project_presenter_spec.rb diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 5405b33de091ae..a24948ec1e1816 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -7,7 +7,10 @@ class Groups::Security::ProjectsController < Groups::ApplicationController def index projects = group.vulnerable_projects - vulnerable_projects = ::Security::VulnerabilityCountsFinder.new(projects).execute + + vulnerable_projects = projects.map do |project| + ::Security::VulnerableProjectPresenter.new(project) + end render json: VulnerableProjectSerializer.new.represent(vulnerable_projects) end diff --git a/ee/app/finders/security/vulnerability_counts_finder.rb b/ee/app/finders/security/vulnerability_counts_finder.rb deleted file mode 100644 index 32a6d609588601..00000000000000 --- a/ee/app/finders/security/vulnerability_counts_finder.rb +++ /dev/null @@ -1,65 +0,0 @@ -# frozen_string_literal: true - -# Security::VulnerabilityCountsFinder -# -# Used to fetch counts for each severity of Vulnerabilities::Occurrences for a given list of projects -# -# Arguments: -# projects - Array (or ActiveRecord::Association) -# -# Return: -# Array where the delegated to object is Project and the struct attributes -# are of the form `[severity_level]_vulnerability_count. The complete list of severity level names -# can be found in ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys -# -# Example: -# ``` -# project = Project.find(1) # this project has 1 high severity vulnerability -# vulnerable_projects = VulnerabilityCountsFinder.new([project]).execute -# -# vulnerable_projects.first.high_vulnerability_count # 1 -# ``` - -module Security - class VulnerabilityCountsFinder - SEVERITIES = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys - - def initialize(projects) - @projects = projects - end - - def execute - projects.map do |project| - vulnerability_counts = SEVERITIES.inject({}) do |counts, severity| - counts["#{severity}_vulnerability_count"] = count_by_severity(project.id, severity) - - counts - end - - ::Gitlab::SimpleDelegatorStruct.new(project, vulnerability_counts) - end - end - - private - - attr_reader :projects - - def count_by_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 = ::Vulnerabilities::Occurrence.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 - end -end diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb index 99639027153858..97b29d45c23389 100644 --- a/ee/app/models/vulnerabilities/occurrence.rb +++ b/ee/app/models/vulnerabilities/occurrence.rb @@ -143,11 +143,29 @@ def self.undismissed where( "NOT EXISTS (?)", Feedback.select(1) - .where("ENCODE(#{table_name}.project_fingerprint, 'HEX') = vulnerability_feedback.project_fingerprint") + .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 00000000000000..c5d25cf7201dbb --- /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/lib/gitlab/simple_delegator_struct.rb b/ee/lib/gitlab/simple_delegator_struct.rb deleted file mode 100644 index 7f1ef118f455ff..00000000000000 --- a/ee/lib/gitlab/simple_delegator_struct.rb +++ /dev/null @@ -1,55 +0,0 @@ -# frozen_string_literal: true - -# Gitlab::SimpleDelegatorStruct -# -# Used to temporarily add simple data methods to an existing class. -# Functions as a combination of the SimpleDelegator and OpenStruct concepts. -# -# Arguments: -# object - Object -# struct_attrs - Hash (works with both symbol and string keys) -# -# Example: -# ``` -# user = User.find(1) -# struct_attrs = { nickname: 'Satie' } -# -# delegator_struct = SimpleDelegatorStruct.new(user, struct_attrs) -# -# delegator_struct.id # 1 -# delegator_struct.nickname # Satie -# ``` - -module Gitlab - class SimpleDelegatorStruct < SimpleDelegator - def initialize(object, struct_attrs = {}) - @struct_attrs = struct_attrs.transform_keys do |key| - if key.is_a?(String) - key - else - key.to_s - end - end - - super(object) - end - - def method_missing(method_name, *arguments, &block) - method_name = method_name.to_s - - if struct_attrs.has_key?(method_name) - struct_attrs[method_name] - else - super - end - end - - def respond_to_missing?(method_name, include_private = false) - struct_attrs.has_key?(method_name.to_s) || super - end - - private - - attr_reader :struct_attrs - end -end diff --git a/ee/spec/finders/security/vulnerability_counts_finder_spec.rb b/ee/spec/finders/security/vulnerability_counts_finder_spec.rb deleted file mode 100644 index 42c62a050d3d54..00000000000000 --- a/ee/spec/finders/security/vulnerability_counts_finder_spec.rb +++ /dev/null @@ -1,44 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe ::Security::VulnerabilityCountsFinder do - let(:project) { create(:project) } - - describe '#execute' do - it 'fetches vulnerability counts for each given project and adds them to the project' do - create(:vulnerabilities_occurrence, project: project, severity: :high) - - vulnerable_projects = described_class.new([project]).execute - - expect(vulnerable_projects.first.high_vulnerability_count).to be(1) - expect(vulnerable_projects.first.critical_vulnerability_count).to be(0) - end - - it 'only uses 1 query' 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 - - expect { described_class.new(projects).execute }.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 - ) - - vulnerable_projects = described_class.new([project]).execute - - expect(vulnerable_projects.first.high_vulnerability_count).to be(1) - end - end -end diff --git a/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb b/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb deleted file mode 100644 index f9772d5bbcff2a..00000000000000 --- a/ee/spec/lib/gitlab/simple_delegator_struct_spec.rb +++ /dev/null @@ -1,28 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe ::Gitlab::SimpleDelegatorStruct do - class TestWitch - def hex - 'I bind and confound you!' - end - end - - let(:struct_attrs) { { heal: 'Bones mend and wounds stitch', 'scry' => 'I see you!' } } - let(:witch) { TestWitch.new } - - subject { described_class.new(witch, struct_attrs) } - - it 'acts like SimpleDelegator on the first argument to its constructor' do - expect(subject.hex).to eq('I bind and confound you!') - end - - it 'acts like OpenStruct on the second argument to its constructor' do - expect(subject.heal).to eq('Bones mend and wounds stitch') - end - - it 'acts like OpenStruct even on string keys' do - expect(subject.scry).to eq('I see you!') - end -end diff --git a/ee/spec/models/vulnerabilities/occurrence_spec.rb b/ee/spec/models/vulnerabilities/occurrence_spec.rb index 3f2a768340c3da..3526e3a3e6eb40 100644 --- a/ee/spec/models/vulnerabilities/occurrence_spec.rb +++ b/ee/spec/models/vulnerabilities/occurrence_spec.rb @@ -290,6 +290,61 @@ 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 00000000000000..8ba9080ad0d57c --- /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/serializers/vulnerable_project_entity_spec.rb b/ee/spec/serializers/vulnerable_project_entity_spec.rb index 323d6c4b3c4660..bc55a9fd50c9a0 100644 --- a/ee/spec/serializers/vulnerable_project_entity_spec.rb +++ b/ee/spec/serializers/vulnerable_project_entity_spec.rb @@ -6,14 +6,10 @@ SEVERITY_LEVELS = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys let(:project) { create(:project) } - let(:vulnerable_project) do - vulnerability_counts = SEVERITY_LEVELS.inject({}) do |counts, severity| - counts["#{severity}_vulnerability_count"] = 2 + let(:vulnerable_project) { ::Security::VulnerableProjectPresenter.new(project) } - counts - end - - ::Gitlab::SimpleDelegatorStruct.new(project, vulnerability_counts) + before do + allow(::Vulnerabilities::Occurrence).to receive(:batch_count_by_project_and_severity).and_return(2) end subject { described_class.new(vulnerable_project) } diff --git a/ee/spec/serializers/vulnerable_project_serializer_spec.rb b/ee/spec/serializers/vulnerable_project_serializer_spec.rb index f83a1b3fc537dc..e9065cd97d6954 100644 --- a/ee/spec/serializers/vulnerable_project_serializer_spec.rb +++ b/ee/spec/serializers/vulnerable_project_serializer_spec.rb @@ -3,16 +3,19 @@ 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(:project) { create(:project) } + 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(project) } + subject { serializer.represent(vulnerable_project) } it 'includes counts for each severity of vulnerability' do ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.each do |severity_level| -- GitLab From eaeffee86aefa1290a5374ecf9fdc1a0c6f23898 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 25 Oct 2019 14:29:41 -0400 Subject: [PATCH 35/41] Remove unnecessary `extend` Goodbye! --- ee/app/serializers/vulnerable_project_entity.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/ee/app/serializers/vulnerable_project_entity.rb b/ee/app/serializers/vulnerable_project_entity.rb index f2ddb18c18d28c..d691bf5cd500de 100644 --- a/ee/app/serializers/vulnerable_project_entity.rb +++ b/ee/app/serializers/vulnerable_project_entity.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class VulnerableProjectEntity < ProjectEntity - extend ActiveSupport::Concern - ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.each_key do |severity_level| expose "#{severity_level}_vulnerability_count" end -- GitLab From 40130c60897e9ca42c02871f3c7857730eefaa08 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Fri, 25 Oct 2019 14:50:06 -0400 Subject: [PATCH 36/41] Don't include archived projects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seems unnecessary… --- .../groups/security/projects_controller.rb | 2 +- .../security/projects_controller_spec.rb | 21 ++++++++++++++++--- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index a24948ec1e1816..7f1fa271f958ad 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -6,7 +6,7 @@ class Groups::Security::ProjectsController < Groups::ApplicationController alias_method :vulnerable, :group def index - projects = group.vulnerable_projects + projects = group.vulnerable_projects.non_archived vulnerable_projects = projects.map do |project| ::Security::VulnerableProjectPresenter.new(project) diff --git a/ee/spec/controllers/groups/security/projects_controller_spec.rb b/ee/spec/controllers/groups/security/projects_controller_spec.rb index f7fce7c7a26b16..c50335940f1b28 100644 --- a/ee/spec/controllers/groups/security/projects_controller_spec.rb +++ b/ee/spec/controllers/groups/security/projects_controller_spec.rb @@ -12,17 +12,22 @@ end describe '#index' do - it "responds with a list of the group's most vulnerable projects" do - sign_in(user) + 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) - get :index, params: { group_id: group }, format: :json + subject expect(response).to have_gitlab_http_status(200) expect(json_response.count).to be(1) @@ -30,5 +35,15 @@ 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 projects' do + archived_project = create(:project, :archived, namespace: group) + create(:vulnerabilities_occurrence, project: archived_project) + + subject + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to be_empty + end end end -- GitLab From d77ae563c60675e5ab41b2248f1bb193ef970589 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 28 Oct 2019 16:56:29 -0400 Subject: [PATCH 37/41] Preload project routes to avoid N+1 This allows us to use our request query count spec without having to do out of scope work to avoid this N+1. --- .../groups/security/projects_controller.rb | 2 +- .../requests/groups/security/projects_index_spec.rb | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 7f1fa271f958ad..8f493971cd3e3b 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -6,7 +6,7 @@ class Groups::Security::ProjectsController < Groups::ApplicationController alias_method :vulnerable, :group def index - projects = group.vulnerable_projects.non_archived + projects = group.vulnerable_projects.non_archived.with_route vulnerable_projects = projects.map do |project| ::Security::VulnerableProjectPresenter.new(project) diff --git a/ee/spec/requests/groups/security/projects_index_spec.rb b/ee/spec/requests/groups/security/projects_index_spec.rb index b3f4c644b8ad56..0374a23ae80c60 100644 --- a/ee/spec/requests/groups/security/projects_index_spec.rb +++ b/ee/spec/requests/groups/security/projects_index_spec.rb @@ -29,11 +29,16 @@ end end + # Make the request again before checking the query limit to ensure the query limit doesn't + # give a false positive because the request itself has errors. + # We can't do this within the query limit block because RSpec swallows errors from that block. + get group_security_projects_path(group, format: :json) + + expect(response).to have_gitlab_http_status(200) + expect(json_response.size).to be(3) + expect do get group_security_projects_path(group, format: :json) - - expect(response).to have_gitlab_http_status(200) - expect(json_response.size).to be(2) end.not_to exceed_query_limit(control_count) end end -- GitLab From 6015257b552bed3a80edd3f09296288115c67b79 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 28 Oct 2019 17:04:49 -0400 Subject: [PATCH 38/41] Don't include deleted projects Unnecessary clutter! --- ee/app/controllers/groups/security/projects_controller.rb | 2 +- .../controllers/groups/security/projects_controller_spec.rb | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/projects_controller.rb index 8f493971cd3e3b..12e8bfc5e28036 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/projects_controller.rb @@ -6,7 +6,7 @@ class Groups::Security::ProjectsController < Groups::ApplicationController alias_method :vulnerable, :group def index - projects = group.vulnerable_projects.non_archived.with_route + projects = group.vulnerable_projects.non_archived.without_deleted.with_route vulnerable_projects = projects.map do |project| ::Security::VulnerableProjectPresenter.new(project) diff --git a/ee/spec/controllers/groups/security/projects_controller_spec.rb b/ee/spec/controllers/groups/security/projects_controller_spec.rb index c50335940f1b28..52446261426449 100644 --- a/ee/spec/controllers/groups/security/projects_controller_spec.rb +++ b/ee/spec/controllers/groups/security/projects_controller_spec.rb @@ -36,9 +36,11 @@ expect(json_response.first['critical_vulnerability_count']).to eq(2) end - it 'does not include archived projects' do + 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 -- GitLab From 346c432c56ca1d12f8bb8ce8aea9b3897768f76f Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Mon, 28 Oct 2019 18:59:35 -0400 Subject: [PATCH 39/41] Rename Projects* to VulnerableProjects* Updating the route and controller to use Vulnerable brings them in line with the other new classes in this change, and makes the naming of upcoming controllers for this dashboard more clear. --- ...ects_controller.rb => vulnerable_projects_controller.rb} | 2 +- ee/config/routes/group.rb | 2 +- ...oller_spec.rb => vulnerable_projects_controller_spec.rb} | 2 +- ...ects_index_spec.rb => vulnerable_projects_index_spec.rb} | 6 +++--- 4 files changed, 6 insertions(+), 6 deletions(-) rename ee/app/controllers/groups/security/{projects_controller.rb => vulnerable_projects_controller.rb} (82%) rename ee/spec/controllers/groups/security/{projects_controller_spec.rb => vulnerable_projects_controller_spec.rb} (96%) rename ee/spec/requests/groups/security/{projects_index_spec.rb => vulnerable_projects_index_spec.rb} (85%) diff --git a/ee/app/controllers/groups/security/projects_controller.rb b/ee/app/controllers/groups/security/vulnerable_projects_controller.rb similarity index 82% rename from ee/app/controllers/groups/security/projects_controller.rb rename to ee/app/controllers/groups/security/vulnerable_projects_controller.rb index 12e8bfc5e28036..97fb2b058b27cf 100644 --- a/ee/app/controllers/groups/security/projects_controller.rb +++ b/ee/app/controllers/groups/security/vulnerable_projects_controller.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class Groups::Security::ProjectsController < Groups::ApplicationController +class Groups::Security::VulnerableProjectsController < Groups::ApplicationController include SecurityDashboardsPermissions alias_method :vulnerable, :group diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index ebce15309092da..f2f8b54c4da393 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -110,7 +110,7 @@ namespace :security do resource :dashboard, only: [:show], controller: :dashboard - resources :projects, only: [:index] + 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/projects_controller_spec.rb b/ee/spec/controllers/groups/security/vulnerable_projects_controller_spec.rb similarity index 96% rename from ee/spec/controllers/groups/security/projects_controller_spec.rb rename to ee/spec/controllers/groups/security/vulnerable_projects_controller_spec.rb index 52446261426449..40513e730a5c8a 100644 --- a/ee/spec/controllers/groups/security/projects_controller_spec.rb +++ b/ee/spec/controllers/groups/security/vulnerable_projects_controller_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -describe Groups::Security::ProjectsController do +describe Groups::Security::VulnerableProjectsController do let(:group) { create(:group) } let(:user) { create(:user) } diff --git a/ee/spec/requests/groups/security/projects_index_spec.rb b/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb similarity index 85% rename from ee/spec/requests/groups/security/projects_index_spec.rb rename to ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb index 0374a23ae80c60..22ac46de46341a 100644 --- a/ee/spec/requests/groups/security/projects_index_spec.rb +++ b/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb @@ -18,7 +18,7 @@ create(:vulnerabilities_occurrence, project: control_project) control_count = ActiveRecord::QueryRecorder.new do - get group_security_projects_path(group, format: :json) + get group_security_vulnerable_projects_path(group, format: :json) end.count projects = create_list(:project, 2, namespace: group) @@ -32,13 +32,13 @@ # Make the request again before checking the query limit to ensure the query limit doesn't # give a false positive because the request itself has errors. # We can't do this within the query limit block because RSpec swallows errors from that block. - get group_security_projects_path(group, format: :json) + get group_security_vulnerable_projects_path(group, format: :json) expect(response).to have_gitlab_http_status(200) expect(json_response.size).to be(3) expect do - get group_security_projects_path(group, format: :json) + get group_security_vulnerable_projects_path(group, format: :json) end.not_to exceed_query_limit(control_count) end end -- GitLab From 3db1b6cc406b8d9af647598d3e0500cc4e972ec9 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 29 Oct 2019 16:26:46 +0000 Subject: [PATCH 40/41] Apply suggestion to ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb --- .../requests/groups/security/vulnerable_projects_index_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb b/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb index 22ac46de46341a..93642d48b0799d 100644 --- a/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb +++ b/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb @@ -19,7 +19,7 @@ control_count = ActiveRecord::QueryRecorder.new do get group_security_vulnerable_projects_path(group, format: :json) - end.count + end projects = create_list(:project, 2, namespace: group) -- GitLab From 39448026ee0a25b03a9c97e2873270267c52d7b2 Mon Sep 17 00:00:00 2001 From: Avielle Wolfe Date: Tue, 29 Oct 2019 22:30:56 +0000 Subject: [PATCH 41/41] Apply suggestion to ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb --- .../groups/security/vulnerable_projects_index_spec.rb | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb b/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb index 93642d48b0799d..b34bd1ed70f354 100644 --- a/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb +++ b/ee/spec/requests/groups/security/vulnerable_projects_index_spec.rb @@ -29,16 +29,11 @@ end end - # Make the request again before checking the query limit to ensure the query limit doesn't - # give a false positive because the request itself has errors. - # We can't do this within the query limit block because RSpec swallows errors from that block. - get group_security_vulnerable_projects_path(group, format: :json) - - expect(response).to have_gitlab_http_status(200) - expect(json_response.size).to be(3) - 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 -- GitLab