Merge branch 'fix_vuln_list_on_group_dashboard' into 'master'

Fix vulnerabilities list of group dashboard

See merge request gitlab-org/gitlab-ee!8107

app/models/ci/pipeline.rb

@@ -251,6 +251,10 @@ module Ci
       end
     end
 
+    def self.latest_successful_ids_per_project
+      success.group(:project_id).select('max(id) as id')
+    end
+
     def self.truncate_sha(sha)
       sha[0...8]
     end

app/models/namespace.rb

@@ -233,6 +233,12 @@ class Namespace < ActiveRecord::Base
     Project.inside_path(full_path)
   end
 
+  # Includes pipelines from this namespace and pipelines from all subgroups
+  # that belongs to this namespace
+  def all_pipelines
+    Ci::Pipeline.where(project: all_projects)
+  end
+
   def has_parent?
     parent.present?
   end

ee/app/controllers/groups/security/vulnerabilities_controller.rb

 # frozen_string_literal: true
 class Groups::Security::VulnerabilitiesController < Groups::Security::ApplicationController
   def index
-    @vulnerabilities = group.all_vulnerabilities.ordered
+    @vulnerabilities = group.latest_vulnerabilities.ordered
       .page(params[:page])
 
     respond_to do |format|

ee/app/models/ee/group.rb

@@ -82,8 +82,9 @@ module EE
       end
     end
 
-    def all_vulnerabilities
-      Vulnerabilities::Occurrence.where(project: all_projects)
+    def latest_vulnerabilities
+      Vulnerabilities::Occurrence
+        .for_pipelines(all_pipelines.latest_successful_ids_per_project)
     end
 
     def human_ldap_access

ee/app/models/vulnerabilities/occurrence.rb

@@ -68,6 +68,11 @@ module Vulnerabilities
       preload(:scanner, :identifiers, :project)
     end
 
+    def self.for_pipelines(pipelines)
+      joins(:occurrence_pipelines)
+        .where(vulnerability_occurrence_pipelines: { pipeline_id: pipelines })
+    end
+
     def feedback(feedback_type:)
       params = {
         project_id: project_id,

ee/app/serializers/vulnerability_summary_entity.rb

@@ -15,6 +15,6 @@ class VulnerabilitySummaryEntity < Grape::Entity
   private
 
   def grouped_vulnerabilities
-    @grouped_by_report_and_severity ||= object.all_vulnerabilities.counted_by_report_and_severity
+    @grouped_by_report_and_severity ||= object.latest_vulnerabilities.counted_by_report_and_severity
   end
 end

ee/spec/controllers/groups/security/vulnerabilities_controller_spec.rb

@@ -55,7 +55,7 @@ describe Groups::Security::VulnerabilitiesController do
         context 'when no page request' do
           before do
             projects.each do |project|
-              create(:vulnerabilities_occurrence, project: project)
+              create_vulnerabilities(1, project)
             end
           end
 
@@ -72,7 +72,7 @@ describe Groups::Security::VulnerabilitiesController do
         context 'when page requested' do
           before do
             projects.each do |project|
-              create_list(:vulnerabilities_occurrence, 11, project: project)
+              create_vulnerabilities(11, project)
             end
           end
 
@@ -87,11 +87,11 @@ describe Groups::Security::VulnerabilitiesController do
 
         context 'with vulnerability feedback' do
           it "avoids N+1 queries" do
-            create_vulnerabilities(2, project_dev)
+            create_vulnerabilities(2, project_dev, with_feedback: true)
 
             control_count = ActiveRecord::QueryRecorder.new { get_summary }
 
-            create_vulnerabilities(2, project_guest)
+            create_vulnerabilities(2, project_guest, with_feedback: true)
 
             expect { get_summary }.not_to exceed_all_query_limit(control_count)
           end
@@ -101,22 +101,24 @@ describe Groups::Security::VulnerabilitiesController do
           def get_summary
             get :index, group_id: group, format: :json
           end
+        end
 
-          def create_vulnerabilities(count, project)
-            pipeline = create(:ci_empty_pipeline, project: project)
-            vulnerabilities = create_list(:vulnerabilities_occurrence, count, project: project)
-            vulnerabilities.each do |occurrence|
-              create(:vulnerability_feedback, :sast, :dismissal,
-                     pipeline: pipeline,
-                     project: project_dev,
-                     project_fingerprint: occurrence.project_fingerprint)
-
-              create(:vulnerability_feedback, :sast, :issue,
-                     pipeline: pipeline,
-                     issue: create(:issue, project: project_dev),
-                     project: project_dev,
-                     project_fingerprint: occurrence.project_fingerprint)
-            end
+        def create_vulnerabilities(count, project, options = {})
+          pipeline = create(:ci_pipeline, :success, project: project)
+          vulnerabilities = create_list(:vulnerabilities_occurrence, count, pipelines: [pipeline], project: project)
+          return vulnerabilities unless options[:with_feedback]
+
+          vulnerabilities.each do |occurrence|
+            create(:vulnerability_feedback, :sast, :dismissal,
+                   pipeline: pipeline,
+                   project: project_dev,
+                   project_fingerprint: occurrence.project_fingerprint)
+
+            create(:vulnerability_feedback, :sast, :issue,
+                   pipeline: pipeline,
+                   issue: create(:issue, project: project),
+                   project: project_dev,
+                   project_fingerprint: occurrence.project_fingerprint)
           end
         end
       end
@@ -142,20 +144,22 @@ describe Groups::Security::VulnerabilitiesController do
       before do
         stub_licensed_features(security_dashboard: true)
 
+        pipeline = create(:ci_pipeline, :success, project: project_dev)
+
         create_list(:vulnerabilities_occurrence, 3,
-                    project: project_dev, report_type: :sast, severity: :high)
+                    pipelines: [pipeline], project: project_dev, report_type: :sast, severity: :high)
 
         create_list(:vulnerabilities_occurrence, 1,
-                    project: project_dev, report_type: :dependency_scanning, severity: :low)
+                    pipelines: [pipeline], project: project_dev, report_type: :dependency_scanning, severity: :low)
 
         create_list(:vulnerabilities_occurrence, 2,
-                    project: project_guest, report_type: :dependency_scanning, severity: :low)
+                    pipelines: [pipeline], project: project_guest, report_type: :dependency_scanning, severity: :low)
 
         create_list(:vulnerabilities_occurrence, 1,
-                    project: project_guest, report_type: :dast, severity: :medium)
+                    pipelines: [pipeline], project: project_guest, report_type: :dast, severity: :medium)
 
         create_list(:vulnerabilities_occurrence, 1,
-                    project: project_other, report_type: :dast, severity: :low)
+                    pipelines: [pipeline], project: project_other, report_type: :dast, severity: :low)
       end
 
       context 'when user has guest access' do

ee/spec/models/group_spec.rb

@@ -259,4 +259,37 @@ describe Group do
       end
     end
   end
+
+  describe '#latest_vulnerabilities' do
+    let(:project) { create(:project, namespace: group) }
+    let(:external_project) { create(:project) }
+    let(:failed_pipeline) { create(:ci_pipeline, :failed, project: project) }
+
+    let!(:old_vuln) { create_vulnerability(project) }
+    let!(:new_vuln) { create_vulnerability(project) }
+    let!(:external_vuln) { create_vulnerability(external_project) }
+    let!(:failed_vuln) { create_vulnerability(project, failed_pipeline) }
+
+    subject { group.latest_vulnerabilities }
+
+    def create_vulnerability(project, pipeline = nil)
+      pipeline ||= create(:ci_pipeline, :success, project: project)
+      create(:vulnerabilities_occurrence, pipelines: [pipeline], project: project)
+    end
+
+    it 'returns vulns only for the latest successful pipelines of projects belonging to the group' do
+      is_expected.to contain_exactly(new_vuln)
+    end
+
+    context 'with vulnerabilities from other branches' do
+      let!(:branch_pipeline) { create(:ci_pipeline, :success, project: project, ref: 'feature-x') }
+      let!(:branch_vuln) { create(:vulnerabilities_occurrence, pipelines: [branch_pipeline], project: project) }
+
+      # TODO: This should actually fail and we must scope vulns
+      # per branch as soon as we store them for other branches
+      it 'includes vulnerabilities from all branches' do
+        is_expected.to contain_exactly(branch_vuln)
+      end
+    end
+  end
 end

spec/models/ci/pipeline_spec.rb

@@ -1157,6 +1157,19 @@ describe Ci::Pipeline, :mailer do
     end
   end
 
+  describe '.latest_successful_ids_per_project' do
+    let(:projects) { create_list(:project, 2) }
+    let!(:pipeline1) { create(:ci_pipeline, :success, project: projects[0]) }
+    let!(:pipeline2) { create(:ci_pipeline, :success, project: projects[0]) }
+    let!(:pipeline3) { create(:ci_pipeline, :failed, project: projects[0]) }
+    let!(:pipeline4) { create(:ci_pipeline, :success, project: projects[1]) }
+
+    it 'returns expected pipeline ids' do
+      expect(described_class.latest_successful_ids_per_project)
+        .to contain_exactly(pipeline2, pipeline4)
+    end
+  end
+
   describe '.internal_sources' do
     subject { described_class.internal_sources }
 

spec/models/namespace_spec.rb

@@ -574,6 +574,17 @@ describe Namespace do
     it { expect(group.all_projects.to_a).to match_array([project2, project1]) }
   end
 
+  describe '#all_pipelines' do
+    let(:group) { create(:group) }
+    let(:child) { create(:group, parent: group) }
+    let!(:project1) { create(:project_empty_repo, namespace: group) }
+    let!(:project2) { create(:project_empty_repo, namespace: child) }
+    let!(:pipeline1) { create(:ci_empty_pipeline, project: project1) }
+    let!(:pipeline2) { create(:ci_empty_pipeline, project: project2) }
+
+    it { expect(group.all_pipelines.to_a).to match_array([pipeline1, pipeline2]) }
+  end
+
   describe '#share_with_group_lock with subgroups', :nested_groups do
     context 'when creating a subgroup' do
       let(:subgroup) { create(:group, parent: root_group )}