Separate pipelines query from vuln findings query

* Refactor VulnerabilityFindingsFinder to take pipeline IDs and use
    them in the query for vulnerability findings
* Move query for pipelines to the controller actions and the caching
    classes
* Removes Vulnerable. We are no longer using the methods included in it

https://gitlab.com/gitlab-org/gitlab/issues/202183

ee/app/controllers/concerns/project_vulnerability_findings_actions.rb

@@ -4,20 +4,22 @@
 # on security dashboards.
 #
 # Note: Consumers of this module will need to define a `def vulnerable` method, which must return
-# an object with an interface that matches the one provided by the Vulnerable model concern.
+# an object that includes an `#all_pipelines` method
 
 module ProjectVulnerabilityFindingsActions
   extend ActiveSupport::Concern
 
   def index
-    findings = vulnerability_findings(:with_sha).ordered.page(params[:page])
+    pipeline_ids = vulnerable.all_pipelines.with_vulnerabilities.latest_successful_ids_per_project
+    findings = ::Security::VulnerabilityFindingsFinder.new(pipeline_ids, params: filter_params, include_sha: true).execute
+    ordered_findings = findings.ordered.page(params[:page])
 
     respond_to do |format|
       format.json do
         render json: Vulnerabilities::OccurrenceSerializer
           .new(current_user: current_user)
           .with_pagination(request, response)
-          .represent(findings, preload: true)
+          .represent(ordered_findings, preload: true)
       end
     end
   end
@@ -37,8 +39,4 @@ module ProjectVulnerabilityFindingsActions
   def filter_params
     params.permit(:scope, report_type: [], confidence: [], project_id: [], severity: [])
   end
-
-  def vulnerability_findings(collection = :latest)
-    ::Security::VulnerabilityFindingsFinder.new(vulnerable, params: filter_params).execute(collection)
-  end
 end

ee/app/finders/security/vulnerability_findings_finder.rb

@@ -5,7 +5,7 @@
 # Used to filter Vulnerabilities::Occurrences  by set of params for Security Dashboard
 #
 # Arguments:
-#   vulnerable - object to filter vulnerabilities
+#   pipeline_ids - IDs for the pipelines to fetch vulnerabilities for
 #   params:
 #     severity: Array<String>
 #     confidence: Array<String>
@@ -16,15 +16,16 @@
 module Security
   class VulnerabilityFindingsFinder
     attr_accessor :params
-    attr_reader :vulnerable
+    attr_reader :include_sha, :pipeline_ids
 
-    def initialize(vulnerable, params: {})
-      @vulnerable = vulnerable
+    def initialize(pipeline_ids, params: {}, include_sha: false)
+      @pipeline_ids = pipeline_ids
       @params = params
+      @include_sha = include_sha
     end
 
-    def execute(collection_scope = :latest)
-      collection = init_collection(collection_scope)
+    def execute
+      collection = init_collection
       collection = by_report_type(collection)
       collection = by_project(collection)
       collection = by_severity(collection)
@@ -73,16 +74,11 @@ module Security
       items.undismissed
     end
 
-    def init_collection(collection_scope)
-      case collection_scope
-      when :all
-        vulnerable.all_vulnerabilities
-      when :with_sha
-        vulnerable.latest_vulnerabilities_with_sha
-      when :latest
-        vulnerable.latest_vulnerabilities
+    def init_collection
+      if include_sha
+        ::Vulnerabilities::Occurrence.for_pipelines_with_sha(pipeline_ids)
       else
-        raise ArgumentError, "invalid value for 'collection_scope': #{collection_scope}"
+        ::Vulnerabilities::Occurrence.for_pipelines(pipeline_ids)
       end
     end
   end

ee/app/models/concerns/vulnerable.rb

-# frozen_string_literal: true
-
-module Vulnerable
-  def latest_vulnerabilities
-    Vulnerabilities::Occurrence
-      .for_pipelines(all_pipelines.with_vulnerabilities.latest_successful_ids_per_project)
-  end
-
-  def latest_vulnerabilities_with_sha
-    Vulnerabilities::Occurrence
-      .for_pipelines_with_sha(all_pipelines.with_vulnerabilities.latest_successful_ids_per_project)
-  end
-
-  def all_vulnerabilities
-    Vulnerabilities::Occurrence
-      .for_pipelines(all_pipelines.with_vulnerabilities.success)
-  end
-end

ee/app/models/ee/group.rb

@@ -10,7 +10,6 @@ module EE
     extend ::Gitlab::Utils::Override
 
     prepended do
-      include Vulnerable
       include TokenAuthenticatable
       include InsightsFeature
       include HasTimelogsReport

ee/app/models/ee/project.rb

@@ -20,7 +20,6 @@ module EE
       include EE::DeploymentPlatform # rubocop: disable Cop/InjectEnterpriseEditionModule
       include EachBatch
       include InsightsFeature
-      include Vulnerable
       include DeprecatedApprovalsBeforeMerge
       include UsageStatistics
 

ee/app/models/instance_security_dashboard.rb

@@ -2,7 +2,6 @@
 
 class InstanceSecurityDashboard
   extend ActiveModel::Naming
-  include ::Vulnerable
 
   def self.name
     'Instance'

ee/lib/gitlab/vulnerabilities/history.rb

@@ -25,7 +25,7 @@ module Gitlab
       private
 
       def vulnerability_findings
-        ::Security::VulnerabilityFindingsFinder.new(vulnerable, params: filters).execute(:all)
+        ::Security::VulnerabilityFindingsFinder.new(pipeline_ids, params: filters).execute
       end
 
       def cached_vulnerability_history
@@ -59,6 +59,10 @@ module Gitlab
 
         vulnerable.project_ids_with_security_reports
       end
+
+      def pipeline_ids
+        vulnerable.all_pipelines.with_vulnerabilities.success
+      end
     end
   end
 end

ee/lib/gitlab/vulnerabilities/history_cache.rb

@@ -13,8 +13,8 @@ module Gitlab
       def fetch(range, force: false)
         Rails.cache.fetch(cache_key, force: force, expires_in: 1.day) do
           findings = ::Security::VulnerabilityFindingsFinder
-            .new(vulnerable, params: { project_id: [project_id] })
-            .execute(:all)
+            .new(pipeline_ids, params: { project_id: [project_id] })
+            .execute
             .count_by_day_and_severity(range)
           ::Vulnerabilities::HistorySerializer.new.represent(findings)
         end
@@ -27,6 +27,10 @@ module Gitlab
         # https://gitlab.com/gitlab-org/gitlab/issues/32963
         ['projects', project_id, 'vulnerabilities']
       end
+
+      def pipeline_ids
+        vulnerable.all_pipelines.with_vulnerabilities.success
+      end
     end
   end
 end

ee/lib/gitlab/vulnerabilities/summary.rb

@@ -46,7 +46,7 @@ module Gitlab
       end
 
       def found_vulnerabilities
-        ::Security::VulnerabilityFindingsFinder.new(vulnerable, params: filters).execute
+        ::Security::VulnerabilityFindingsFinder.new(pipeline_ids, params: filters).execute
       end
 
       def use_vulnerability_cache?
@@ -68,6 +68,10 @@ module Gitlab
           vulnerable.project_ids_with_security_reports
         end
       end
+
+      def pipeline_ids
+        vulnerable.all_pipelines.with_vulnerabilities.latest_successful_ids_per_project
+      end
     end
   end
 end

ee/lib/gitlab/vulnerabilities/summary_cache.rb

@@ -13,8 +13,8 @@ module Gitlab
       def fetch(force: false)
         Rails.cache.fetch(cache_key, force: force, expires_in: 1.day) do
           findings = ::Security::VulnerabilityFindingsFinder
-            .new(vulnerable, params: { project_id: [project_id] })
-            .execute(:all)
+            .new(pipeline_ids, params: { project_id: [project_id] })
+            .execute
             .counted_by_severity
 
           VulnerabilitySummarySerializer.new.represent(findings)
@@ -26,6 +26,10 @@ module Gitlab
       def cache_key
         ['projects', project_id, 'findings-summary']
       end
+
+      def pipeline_ids
+        vulnerable.all_pipelines.with_vulnerabilities.success
+      end
     end
   end
 end

ee/spec/finders/security/vulnerability_findings_finder_spec.rb

@@ -4,18 +4,47 @@ require 'spec_helper'
 
 describe Security::VulnerabilityFindingsFinder do
   describe '#execute' do
-    let_it_be(:group) { create(:group) }
-    let_it_be(:project1) { create(:project, :private, :repository, group: group) }
-    let_it_be(:project2) { create(:project, :private, :repository, group: group) }
+    let_it_be(:project1) { create(:project, :private, :repository) }
+    let_it_be(:project2) { create(:project, :private, :repository) }
     let_it_be(:pipeline1) { create(:ci_pipeline, :success, project: project1) }
     let_it_be(:pipeline2) { create(:ci_pipeline, :success, project: project2) }
+    let(:pipelines) { [pipeline1, pipeline2] }
 
     let_it_be(:finding1) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :high, confidence: :high, pipelines: [pipeline1], project: project1) }
     let_it_be(:finding2) { create(:vulnerabilities_occurrence, report_type: :dependency_scanning, severity: :medium, confidence: :low, pipelines: [pipeline2], project: project2) }
     let_it_be(:finding3) { create(:vulnerabilities_occurrence, report_type: :sast, severity: :low, pipelines: [pipeline2], project: project2) }
     let_it_be(:finding4) { create(:vulnerabilities_occurrence, report_type: :dast, severity: :medium, pipelines: [pipeline1], project: project1) }
 
-    subject { described_class.new(group, params: params).execute }
+    let(:params) { {} }
+    let(:include_sha) { false }
+
+    subject { described_class.new(pipelines, params: params, include_sha: include_sha).execute }
+
+    context 'when given a pipeline' do
+      let(:pipelines) { [pipeline1] }
+
+      it "returns vulnerability findings found in the pipeline" do
+        is_expected.to contain_exactly(finding1, finding4)
+      end
+    end
+
+    context 'include_sha' do
+      context 'when false' do
+        it 'does not include a sha' do
+          expect(subject.map(&:sha)).to be_all(&:nil?)
+        end
+      end
+
+      context 'when true' do
+        let(:include_sha) { true }
+
+        it 'includes the sha for the latest pipeline associated with the finding' do
+          subject.each do |finding|
+            expect(finding.sha).to eq(finding.pipelines.last.sha)
+          end
+        end
+      end
+    end
 
     context 'by report type' do
       context 'when sast' do
@@ -160,31 +189,5 @@ describe Security::VulnerabilityFindingsFinder do
         end
       end
     end
-
-    describe 'scope specifiers' do
-      using RSpec::Parameterized::TableSyntax
-
-      where(:scope) do
-        [
-          [:all],
-          [:with_sha],
-          [:latest]
-        ]
-      end
-
-      with_them do
-        it 'accepts the scope specifier as valid' do
-          expect { described_class.new(group).execute(scope) }.not_to raise_error
-        end
-      end
-
-      context 'with an invalid collection scope specifier' do
-        it 'raises error' do
-          expect { described_class.new(group).execute(:invalid) }.to(
-            raise_error(ArgumentError, "invalid value for 'collection_scope': invalid")
-          )
-        end
-      end
-    end
   end
 end

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

@@ -7,9 +7,12 @@ describe Gitlab::Vulnerabilities::HistoryCache do
     shared_examples 'the history cache when given an expected Vulnerable' do
       let(:project) { create(:project, :public, namespace: group) }
       let(:project_cache_key) { described_class.new(vulnerable, project.id).send(:cache_key) }
+      let(:today) { '1980-10-31' }
 
       before do
-        create_vulnerabilities(1, project)
+        Timecop.freeze(today) do
+          create_vulnerabilities(1, project)
+        end
       end
 
       it 'reads from cache when records are cached' do
@@ -23,7 +26,7 @@ describe Gitlab::Vulnerabilities::HistoryCache do
       end
 
       it 'returns the proper format for uncached history' do
-        Timecop.freeze do
+        Timecop.freeze(today) do
           fetched_history = described_class.new(vulnerable, project.id).fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE)
 
           expect(fetched_history[:total]).to eq( Date.today => 1 )
@@ -32,7 +35,7 @@ describe Gitlab::Vulnerabilities::HistoryCache do
       end
 
       it 'returns the proper format for cached history' do
-        Timecop.freeze do
+        Timecop.freeze(today) do
           described_class.new(vulnerable, project.id).fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE)
           fetched_history = described_class.new(vulnerable, project.id).fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE)
 

ee/spec/models/group_spec.rb

@@ -5,10 +5,6 @@ require 'spec_helper'
 describe Group do
   let(:group) { create(:group) }
 
-  it_behaves_like Vulnerable do
-    let(:vulnerable) { group }
-  end
-
   it { is_expected.to include_module(EE::Group) }
 
   describe 'associations' do

ee/spec/models/instance_security_dashboard_spec.rb

@@ -17,10 +17,6 @@ describe InstanceSecurityDashboard do
 
   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')

ee/spec/models/project_spec.rb

@@ -9,10 +9,6 @@ describe Project do
 
   let(:project) { create(:project) }
 
-  it_behaves_like Vulnerable do
-    let(:vulnerable) { project }
-  end
-
   describe 'associations' do
     it { is_expected.to delegate_method(:shared_runners_minutes).to(:statistics) }
     it { is_expected.to delegate_method(:shared_runners_seconds).to(:statistics) }

ee/spec/serializers/vulnerabilities/history_entity_spec.rb

@@ -3,13 +3,12 @@
 require 'spec_helper'
 
 describe Vulnerabilities::HistoryEntity do
-  let(:group) { create(:group) }
-  let(:project) { create(:project, group: group) }
+  let(:project) { create(:project) }
   let(:time) { Time.zone.parse('2018-11-10') }
 
   let(:entity) do
     travel_to(Time.zone.parse('2018-11-15')) do
-      described_class.represent(group.all_vulnerabilities.count_by_day_and_severity(3.months))
+      described_class.represent(project.vulnerability_findings.count_by_day_and_severity(3.months))
     end
   end
 

ee/spec/support/shared_examples/controllers/concerns/project_vulnerability_findings_actions_shared_examples.rb

@@ -33,6 +33,13 @@ RSpec.shared_examples ProjectVulnerabilityFindingsActions do
       expect(response).to have_gitlab_http_status(:ok)
       expect(json_response.length).to eq 2
       expect(json_response.first['id']).to be(critical_vulnerability.id)
+
+      blob_path = project_blob_path(vulnerable_project, File.join(pipeline.sha, critical_vulnerability.location['file'])).tap do |path|
+        path << "#L#{critical_vulnerability.location['start_line']}"
+        path << "-#{critical_vulnerability.location['end_line']}"
+      end
+
+      expect(json_response.first['blob_path']).to eq(blob_path)
       expect(response).to match_response_schema('vulnerabilities/occurrence_list', dir: 'ee')
     end
 

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

-# frozen_string_literal: true
-
-RSpec.shared_examples Vulnerable do
-  include VulnerableHelpers
-
-  let(:external_project) { create(:project) }
-  let(:failed_pipeline) { create(:ci_pipeline, :failed, project: vulnerable_project) }
-
-  let!(:old_vuln) { create_vulnerability(vulnerable_project) }
-  let!(:new_vuln) { create_vulnerability(vulnerable_project) }
-  let!(:external_vuln) { create_vulnerability(external_project) }
-  let!(:failed_vuln) { create_vulnerability(vulnerable_project, failed_pipeline) }
-  let(:vulnerable_project) { as_vulnerable_project(vulnerable) }
-
-  before do
-    pipeline_ran_against_new_sha = create(:ci_pipeline, :success, project: vulnerable_project, sha: '123')
-    new_vuln.pipelines << pipeline_ran_against_new_sha
-  end
-
-  def create_vulnerability(project, pipeline = nil)
-    if project
-      pipeline ||= create(:ci_pipeline, :success, project: project)
-      create(:vulnerabilities_occurrence, pipelines: [pipeline], project: project)
-    end
-  end
-
-  describe '#latest_vulnerabilities' do
-    subject { vulnerable.latest_vulnerabilities }
-
-    it 'returns vulnerabilities for the latest successful pipelines of projects belonging to the vulnerable entity' do
-      is_expected.to contain_exactly(new_vuln)
-    end
-
-    context 'with vulnerabilities from other branches' do
-      let!(:branch_pipeline) { create(:ci_pipeline, :success, project: vulnerable_project, ref: 'feature-x') }
-      let!(:branch_vuln) { create(:vulnerabilities_occurrence, pipelines: [branch_pipeline], project: vulnerable_project) }
-
-      # TODO: This should actually fail and we must scope vulns
-      # per branch as soon as we store them for other branches
-      # Dependent on https://gitlab.com/gitlab-org/gitlab/issues/9524
-      it 'includes vulnerabilities from all branches' do
-        is_expected.to contain_exactly(branch_vuln)
-      end
-    end
-  end
-
-  describe '#latest_vulnerabilities_with_sha' do
-    subject { vulnerable.latest_vulnerabilities_with_sha }
-
-    it 'returns vulns only for the latest successful pipelines of projects belonging to the vulnerable' do
-      is_expected.to contain_exactly(new_vuln)
-    end
-
-    it { is_expected.to all(respond_to(:sha)) }
-
-    context 'with vulnerabilities from other branches' do
-      let!(:branch_pipeline) { create(:ci_pipeline, :success, project: vulnerable_project, ref: 'feature-x') }
-      let!(:branch_vuln) { create(:vulnerabilities_occurrence, pipelines: [branch_pipeline], project: vulnerable_project) }
-
-      # TODO: This should actually fail and we must scope vulns
-      # per branch as soon as we store them for other branches
-      # Dependent on https://gitlab.com/gitlab-org/gitlab/issues/9524
-      it 'includes vulnerabilities from all branches' do
-        is_expected.to contain_exactly(branch_vuln)
-      end
-    end
-  end
-
-  describe '#all_vulnerabilities' do
-    subject { vulnerable.all_vulnerabilities }
-
-    it 'returns vulns for all successful pipelines of projects belonging to the vulnerable' do
-      is_expected.to contain_exactly(old_vuln, new_vuln, new_vuln)
-    end
-
-    context 'with vulnerabilities from other branches' do
-      let!(:branch_pipeline) { create(:ci_pipeline, :success, project: vulnerable_project, ref: 'feature-x') }
-      let!(:branch_vuln) { create(:vulnerabilities_occurrence, pipelines: [branch_pipeline], project: vulnerable_project) }
-
-      # TODO: This should actually fail and we must scope vulns
-      # per branch as soon as we store them for other branches
-      # Dependent on https://gitlab.com/gitlab-org/gitlab/issues/9524
-      it 'includes vulnerabilities from all branches' do
-        is_expected.to contain_exactly(old_vuln, new_vuln, new_vuln, branch_vuln)
-      end
-    end
-  end
-end