Merge branch '197238-fix-isd-timeout' into 'master'

Fix query timeout on instance security dashboard

See merge request gitlab-org/gitlab!23232

ee/app/controllers/security/application_controller.rb

@@ -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

ee/app/controllers/security/vulnerability_findings_controller.rb

@@ -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

ee/app/helpers/ee/dashboard_helper.rb

@@ -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

ee/app/models/application_instance.rb

-# 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

ee/app/models/ee/user.rb

@@ -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?

ee/app/models/instance_security_dashboard.rb

+# 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

ee/app/policies/application_instance_policy.rb

-# frozen_string_literal: true
-
-class ApplicationInstancePolicy < BasePolicy
-  rule { ~anonymous }.enable :read_application_instance_security_dashboard
-end

ee/app/policies/instance_policy.rb

+# frozen_string_literal: true
+
+class InstancePolicy < BasePolicy
+  rule { ~anonymous }.enable :read_instance_security_dashboard
+end

ee/changelogs/unreleased/197238-fix-isd-timeout.yml

+---
+title: Fix vulnerability finding list endpoint query timeout on instance security dashboard
+merge_request: 23232
+author:
+type: fixed

ee/lib/gitlab/vulnerabilities/history.rb

@@ -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

ee/lib/gitlab/vulnerabilities/summary.rb

@@ -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

ee/spec/helpers/ee/dashboard_helper_spec.rb

@@ -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

ee/spec/lib/gitlab/vulnerabilities/history_cache_spec.rb

@@ -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

ee/spec/lib/gitlab/vulnerabilities/history_spec.rb

@@ -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

ee/spec/lib/gitlab/vulnerabilities/summary_spec.rb

@@ -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 = {})

ee/spec/models/application_instance_spec.rb

-# 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

ee/spec/models/instance_security_dashboard_spec.rb

+# 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

ee/spec/models/user_spec.rb

@@ -836,32 +836,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

ee/spec/policies/application_instance_policy_spec.rb → ee/spec/policies/instance_policy_spec.rb

@@ -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

ee/spec/support/helpers/vulnerable_helpers.rb

@@ -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

ee/spec/support/shared_examples/models/concerns/vulnerable_shared_examples.rb

@@ -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) }