Commit 7f86aa5a authored by Avielle Wolfe's avatar Avielle Wolfe Committed by Bob Van Landuyt

Fix heavy findings query on instance sec dash

The query we were using to fetch the list of vulnerability findings for
the instance security dashboard was fetching vulnerabilities for *all*
pipelines and _then_ filtering them by project.

Now we filter the pipelines we're fetching by project,
greatly reducing the time to execute the query.

https://gitlab.com/gitlab-org/gitlab/issues/197238
parent 580c2829
......@@ -16,7 +16,10 @@ module Security
end
def vulnerable
@vulnerable ||= ApplicationInstance.new
@vulnerable ||= InstanceSecurityDashboard.new(
current_user,
project_ids: params.fetch(:project_id, [])
)
end
end
end
......@@ -3,37 +3,5 @@
module Security
class VulnerabilityFindingsController < ::Security::ApplicationController
include ProjectCollectionVulnerabilityFindingsActions
before_action :remove_invalid_project_ids
private
def remove_invalid_project_ids
render_empty_response if valid_project_ids.empty?
params[:project_id] = valid_project_ids
end
def render_empty_response
respond_to do |format|
format.json do
render json: {}
end
end
end
def valid_project_ids
return security_dashboard_project_ids if request_project_ids.empty?
security_dashboard_project_ids & request_project_ids
end
def request_project_ids
params.fetch(:project_id, []).map(&:to_i)
end
def security_dashboard_project_ids
current_user.security_dashboard_project_ids
end
end
end
......@@ -62,11 +62,11 @@ module EE
end
def security_dashboard_available?
app_instance = ApplicationInstance.new
security_dashboard = InstanceSecurityDashboard.new(current_user)
::Feature.enabled?(:security_dashboard, default_enabled: true) &&
app_instance.feature_available?(:security_dashboard) &&
can?(current_user, :read_application_instance_security_dashboard, app_instance)
security_dashboard.feature_available?(:security_dashboard) &&
can?(current_user, :read_instance_security_dashboard, security_dashboard)
end
end
end
# frozen_string_literal: true
class ApplicationInstance
extend ActiveModel::Naming
include ::Vulnerable
def all_pipelines
::Ci::Pipeline.all
end
def feature_available?(feature)
License.feature_available?(feature)
end
end
......@@ -346,14 +346,6 @@ module EE
read_attribute(:support_bot)
end
def security_dashboard_project_ids
if self.can?(:read_all_resources)
security_dashboard_projects.ids
else
security_dashboard_projects.visible_to_user(self).ids
end
end
protected
override :password_required?
......
# frozen_string_literal: true
class InstanceSecurityDashboard
extend ActiveModel::Naming
include ::Vulnerable
def self.name
'Instance'
end
def initialize(user, project_ids: [])
@project_ids = project_ids
@user = user
end
def all_pipelines
::Ci::Pipeline.where(project_id: users_projects_with_security_reports)
end
def project_ids_with_security_reports
users_projects_with_security_reports.pluck(:project_id)
end
def feature_available?(feature)
License.feature_available?(feature)
end
private
attr_reader :project_ids, :user
def users_projects_with_security_reports
return visible_users_security_dashboard_projects if project_ids.empty?
visible_users_security_dashboard_projects.where(project_id: project_ids)
end
def visible_users_security_dashboard_projects
return users_security_dashboard_projects if user.can?(:read_all_resources)
users_security_dashboard_projects.where('EXISTS(?)', project_authorizations)
end
def users_security_dashboard_projects
UsersSecurityDashboardProject.select(:project_id).where(user: user)
end
def project_authorizations
ProjectAuthorization
.select(1)
.where(users_security_dashboard_projects: { user_id: user.id })
.where(project_authorizations: { user_id: user.id })
.where('users_security_dashboard_projects.project_id = project_authorizations.project_id')
end
end
# frozen_string_literal: true
class ApplicationInstancePolicy < BasePolicy
rule { ~anonymous }.enable :read_application_instance_security_dashboard
end
# frozen_string_literal: true
class InstancePolicy < BasePolicy
rule { ~anonymous }.enable :read_instance_security_dashboard
end
---
title: Fix vulnerability finding list endpoint query timeout on instance security dashboard
merge_request: 23232
author:
type: fixed
......@@ -58,10 +58,6 @@ module Gitlab
return filters[:project_id] if filters.key?('project_id')
vulnerable.project_ids_with_security_reports
rescue NoMethodError
vulnerable_name = vulnerable.model_name.human.downcase
raise NoProjectIDsError, "A project_id filter must be given with this #{vulnerable_name}"
end
end
end
......
......@@ -60,11 +60,13 @@ module Gitlab
end
def project_ids_to_fetch
project_ids = vulnerable.is_a?(Project) ? [vulnerable.id] : []
return [vulnerable.id] if vulnerable.is_a?(Project)
return filters[:project_id] + project_ids if filters.key?('project_id')
vulnerable.is_a?(Group) ? vulnerable.project_ids_with_security_reports : project_ids
if filters.key?('project_id')
vulnerable.project_ids_with_security_reports & filters[:project_id].map(&:to_i)
else
vulnerable.project_ids_with_security_reports
end
end
end
end
......
......@@ -58,9 +58,9 @@ describe DashboardHelper, type: :helper do
end
where(:ability, :feature_flag, :nav_link) do
:read_operations_dashboard | nil | :operations
:read_operations_dashboard | :environments_dashboard | :environments
:read_application_instance_security_dashboard | :security_dashboard | :security
:read_operations_dashboard | nil | :operations
:read_operations_dashboard | :environments_dashboard | :environments
:read_instance_security_dashboard | :security_dashboard | :security
end
with_them do
......@@ -120,10 +120,9 @@ describe DashboardHelper, type: :helper do
def stub_resource_visibility(feature_flag, read_other_resources:, read_security_dashboard:, security_dashboard_available:)
if feature_flag == :security_dashboard
app_instance = double(ApplicationInstance, feature_available?: security_dashboard_available)
allow(ApplicationInstance).to receive(:new).and_return(app_instance)
stub_licensed_features(feature_flag => security_dashboard_available)
allow(helper).to receive(:can?).with(user, ability, app_instance).and_return(read_security_dashboard)
allow(helper).to receive(:can?).and_return(read_security_dashboard)
else
allow(helper).to receive(:can?).with(user, ability).and_return(read_other_resources)
end
......
......@@ -55,10 +55,16 @@ describe Gitlab::Vulnerabilities::HistoryCache do
end
end
context 'when given an ApplicationInstance' do
context 'when given an InstanceSecurityDashboard' do
it_behaves_like 'the history cache when given an expected Vulnerable' do
let(:group) { create(:group) }
let(:vulnerable) { ApplicationInstance.new }
let(:user) { create(:user) }
let(:vulnerable) { InstanceSecurityDashboard.new(user) }
before do
project.add_developer(user)
user.security_dashboard_projects << project
end
end
end
end
......
......@@ -5,7 +5,7 @@ require 'spec_helper'
describe Gitlab::Vulnerabilities::History do
describe '#findings_counter', :use_clean_rails_memory_store_caching do
shared_examples 'the history cache when given an expected Vulnerable' do
let(:filters) { project_ids }
let(:filters) { ActionController::Parameters.new }
let(:today) { Date.parse('20191031') }
before do
......@@ -18,7 +18,7 @@ describe Gitlab::Vulnerabilities::History do
subject(:counter) { described_class.new(vulnerable, params: filters).findings_counter }
context 'when filters are passed' do
let(:filters) { project_ids.merge(report_type: :sast) }
let(:filters) { ActionController::Parameters.new({ 'report_type' => ['sast'] }) }
it 'does not call Gitlab::Vulnerabilities::HistoryCache' do
expect(Gitlab::Vulnerabilities::HistoryCache).not_to receive(:new)
......@@ -57,6 +57,20 @@ describe Gitlab::Vulnerabilities::History do
end
end
context 'when a project_id filter is passed' do
let(:filters) { ActionController::Parameters.new({ 'project_id' => [project1] }) }
it 'only fetches history for the filtered by projects' do
expect(Gitlab::Vulnerabilities::HistoryCache).to receive(:new).once.and_call_original
Timecop.freeze(today) do
expect(counter[:total]).to eq({ today => 1 })
expect(counter[:high]).to eq({})
expect(counter[:medium]).to eq({ today => 1 })
end
end
end
def create_vulnerabilities(count, project, options = {})
report_type = options[:report_type] || :sast
severity = options[:severity] || :high
......@@ -71,29 +85,23 @@ describe Gitlab::Vulnerabilities::History do
let(:group) { create(:group) }
let(:project1) { create(:project, :public, namespace: group) }
let(:project2) { create(:project, :public, namespace: group) }
let(:project_ids) { {} }
let(:vulnerable) { group }
end
end
context 'when given an ApplicationInstance' do
let(:vulnerable) { ApplicationInstance.new }
context 'when given an InstanceSecurityDashboard' do
it_behaves_like 'the history cache when given an expected Vulnerable' do
let(:group) { create(:group) }
let(:project1) { create(:project, :public, namespace: group) }
let(:project2) { create(:project, :public, namespace: group) }
let(:user) { create(:user) }
let(:vulnerable) { InstanceSecurityDashboard.new(user) }
context 'and a project_id filter' do
it_behaves_like 'the history cache when given an expected Vulnerable' do
let(:group) { create(:group) }
let(:project1) { create(:project, :public, namespace: group) }
let(:project2) { create(:project, :public, namespace: group) }
let(:project_ids) { ActionController::Parameters.new({ 'project_id' => [project1, project2] }) }
end
end
before do
project1.add_developer(user)
project2.add_developer(user)
context 'and no project_id filter' do
it 'throws an error saying that the filter must be given' do
expect { described_class.new(vulnerable, params: {}).findings_counter }.to raise_error(
Gitlab::Vulnerabilities::History::NoProjectIDsError,
"A project_id filter must be given with this #{vulnerable.model_name.human.downcase}"
)
user.security_dashboard_projects << [project1, project2]
end
end
end
......
......@@ -60,6 +60,15 @@ describe Gitlab::Vulnerabilities::Summary do
expect(counter[:high]).to eq(2)
end
end
context 'when a project_id param is passed' do
let(:filters) { ActionController::Parameters.new({ project_id: [project1.id.to_s] }) }
it 'only fetches findings for the given projects' do
expect(counter[:high]).to eq(0)
expect(counter[:medium]).to eq(1)
end
end
end
def create_vulnerabilities(count, project, options = {})
......
# frozen_string_literal: true
require 'spec_helper'
describe ApplicationInstance do
it_behaves_like Vulnerable do
let(:vulnerable) { described_class.new }
end
describe '#all_pipelines' do
it 'returns all CI pipelines for the instance' do
allow(::Ci::Pipeline).to receive(:all)
described_class.new.all_pipelines
expect(::Ci::Pipeline).to have_received(:all)
end
end
describe '#feature_available?' do
subject { described_class.new.feature_available?(:security_dashboard) }
context "when the feature is available for the instance's license" do
before do
stub_licensed_features(security_dashboard: true)
end
it 'returns true' do
is_expected.to be_truthy
end
end
context "when the feature is not available for the instance's license" do
before do
stub_licensed_features(security_dashboard: false)
end
it 'returns false' do
is_expected.to be_falsy
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe InstanceSecurityDashboard do
let(:pipeline1) { create(:ci_pipeline, project: project1) }
let(:pipeline2) { create(:ci_pipeline, project: project2) }
let(:project1) { create(:project) }
let(:project2) { create(:project) }
let(:project_ids) { [project1.id] }
let(:user) { create(:user) }
before do
project1.add_developer(user)
user.security_dashboard_projects << [project1, project2]
end
subject { described_class.new(user, project_ids: project_ids) }
it_behaves_like Vulnerable do
let(:vulnerable) { described_class.new(user, project_ids: project_ids) }
end
describe '.name' do
it 'is programmatically named Instance' do
expect(described_class.name).to eq('Instance')
end
end
describe '#all_pipelines' do
it 'returns pipelines for the projects with security reports' do
expect(subject.all_pipelines).to contain_exactly(pipeline1)
end
end
describe '#project_ids_with_security_reports' do
context 'when given project IDs' do
it "returns the project IDs that are also on the user's security dashboard" do
expect(subject.project_ids_with_security_reports).to contain_exactly(project1.id)
end
end
context 'when not given project IDs' do
let(:project_ids) { [] }
it "returns the security dashboard projects' IDs" do
expect(subject.project_ids_with_security_reports).to contain_exactly(project1.id)
end
end
context 'when the user cannot read all resources' do
let(:project_ids) { [project1.id, project2.id] }
it 'only includes projects they can read' do
expect(subject.project_ids_with_security_reports).to contain_exactly(project1.id)
end
end
context 'when the user can read all resources' do
let(:project_ids) { [project1.id, project2.id] }
let(:user) { create(:auditor) }
it 'includes all dashboard projects' do
expect(subject.project_ids_with_security_reports).to contain_exactly(project1.id, project2.id)
end
end
end
describe '#feature_available?' do
subject { described_class.new(user).feature_available?(:security_dashboard) }
context "when the feature is available for the instance's license" do
before do
stub_licensed_features(security_dashboard: true)
end
it 'returns true' do
is_expected.to be_truthy
end
end
context "when the feature is not available for the instance's license" do
before do
stub_licensed_features(security_dashboard: false)
end
it 'returns false' do
is_expected.to be_falsy
end
end
end
end
......@@ -824,32 +824,4 @@ describe User do
end
end
end
describe '#security_dashboard_project_ids' do
let(:project) { create(:project) }
context 'when the user can read all resources' do
it "returns the ids for all of the user's security dashboard projects" do
admin = create(:admin)
auditor = create(:auditor)
admin.security_dashboard_projects << project
auditor.security_dashboard_projects << project
expect(admin.security_dashboard_project_ids).to eq([project.id])
expect(auditor.security_dashboard_project_ids).to eq([project.id])
end
end
context 'when the user cannot read all resources' do
it 'returns the ids for security dashboard projects visible to the user' do
user = create(:user)
member_project = create(:project)
member_project.add_developer(user)
user.security_dashboard_projects << [project, member_project]
expect(user.security_dashboard_project_ids).to eq([member_project.id])
end
end
end
end
......@@ -2,7 +2,7 @@
require 'spec_helper'
describe ApplicationInstancePolicy do
describe InstancePolicy do
let(:current_user) { create(:user) }
let(:user) { create(:user) }
......@@ -12,15 +12,15 @@ describe ApplicationInstancePolicy do
subject { described_class.new(current_user, [user]) }
describe 'read_application_instance_security_dashboard' do
describe 'read_instance_security_dashboard' do
context 'when the user is not logged in' do
let(:current_user) { nil }
it { is_expected.not_to be_allowed(:read_application_instance_security_dashboard) }
it { is_expected.not_to be_allowed(:read_instance_security_dashboard) }
end
context 'when the user is logged in' do
it { is_expected.to be_allowed(:read_application_instance_security_dashboard) }
it { is_expected.to be_allowed(:read_instance_security_dashboard) }
end
end
end
......@@ -3,7 +3,7 @@
module VulnerableHelpers
class BadVulnerableError < StandardError
def message
'The given vulnerable must be either `Project`, `Namespace`, or `ApplicationInstance`'
'The given vulnerable must be either `Project`, `Namespace`, or `InstanceSecurityDashboard`'
end
end
......@@ -13,21 +13,8 @@ module VulnerableHelpers
vulnerable
when Namespace
create(:project, namespace: vulnerable)
when ApplicationInstance
create(:project)
else
raise BadVulnerableError
end
end
def as_external_vulnerable_project(vulnerable)
case vulnerable
when Project
create(:project)
when Namespace
create(:project)
when ApplicationInstance
nil
when InstanceSecurityDashboard
Project.find(vulnerable.project_ids_with_security_reports.first)
else
raise BadVulnerableError
end
......
......@@ -3,7 +3,7 @@
RSpec.shared_examples Vulnerable do
include VulnerableHelpers
let(:external_project) { as_external_vulnerable_project(vulnerable) }
let(:external_project) { create(:project) }
let(:failed_pipeline) { create(:ci_pipeline, :failed, project: vulnerable_project) }
let!(:old_vuln) { create_vulnerability(vulnerable_project) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment