Merge branch...

Merge branch '331724_add_filtering_by_state_to_pipeline_security_report_findings_resolver' into 'master'

Add filtering by state to PipelineSecurityReportFindingsResolver

See merge request gitlab-org/gitlab!64762

doc/api/graphql/reference/index.md

@@ -11180,6 +11180,7 @@ four standard [pagination arguments](#connection-pagination-arguments):
 | <a id="pipelinesecurityreportfindingsreporttype"></a>`reportType` | [`[String!]`](#string) | Filter vulnerability findings by report type. |
 | <a id="pipelinesecurityreportfindingsscanner"></a>`scanner` | [`[String!]`](#string) | Filter vulnerability findings by Scanner.externalId. |
 | <a id="pipelinesecurityreportfindingsseverity"></a>`severity` | [`[String!]`](#string) | Filter vulnerability findings by severity. |
+| <a id="pipelinesecurityreportfindingsstate"></a>`state` | [`[VulnerabilityState!]`](#vulnerabilitystate) | Filter vulnerability findings by state. |
 
 ##### `Pipeline.testSuite`
 

ee/app/finders/security/pipeline_vulnerabilities_finder.rb

@@ -10,6 +10,10 @@
 #   params:
 #     report_type: Array<String>
 
+# DEPRECATED: This finder class is deprecated and will be removed
+# by https://gitlab.com/gitlab-org/gitlab/-/issues/334488.
+# Please inform us by adding a comment to aforementioned issue,
+# if you need to add an extra feature to this class.
 module Security
   class PipelineVulnerabilitiesFinder
     include Gitlab::Utils::StrongMemoize
@@ -24,18 +28,18 @@ module Security
     end
 
     def execute
-      findings = requested_reports.each_with_object([]) do |(type, report), findings|
+      findings = requested_reports.each_with_object([]) do |report, findings|
         raise ParseError, 'JSON parsing failed' if report.errored?
 
         normalized_findings = normalize_report_findings(
           report.findings,
-          vulnerabilities_by_finding_fingerprint(type, report))
+          vulnerabilities_by_finding_fingerprint(report))
         filtered_findings = filter(normalized_findings)
 
         findings.concat(filtered_findings)
       end
 
-      Gitlab::Ci::Reports::Security::AggregatedReport.new(requested_reports.values, sort_findings(findings))
+      Gitlab::Ci::Reports::Security::AggregatedReport.new(requested_reports, sort_findings(findings))
     end
 
     private
@@ -53,20 +57,23 @@ module Security
     end
 
     def requested_reports
-      @requested_reports ||= pipeline&.security_reports(report_types: report_types)&.reports || {}
+      @requested_reports ||= pipeline&.security_reports(report_types: report_types)&.reports&.values || []
     end
 
-    def vulnerabilities_by_finding_fingerprint(report_type, report)
-      Vulnerabilities::Finding
-        .by_project_fingerprints(report.findings.map(&:project_fingerprint))
-        .by_projects(pipeline.project)
-        .by_report_types(report_type)
-        .select(:vulnerability_id, :project_fingerprint)
-       .each_with_object({}) do |finding, hash|
-        hash[finding.project_fingerprint] = finding.vulnerability_id
+    def vulnerabilities_by_finding_fingerprint(report)
+      existing_findings_for(report).each_with_object({}) do |finding, memo|
+        memo[finding.project_fingerprint] = finding.vulnerability
       end
     end
 
+    def existing_findings_for(report)
+      Vulnerabilities::Finding.by_project_fingerprints(report.findings.map(&:project_fingerprint))
+                              .by_projects(pipeline.project)
+                              .by_report_types(report.type)
+                              .includes(:vulnerability) # rubocop:disable CodeReuse/ActiveRecord (We will remove this class)
+                              .select(:vulnerability_id, :project_fingerprint)
+    end
+
     # This finder is used for fetching vulnerabilities for any pipeline, if we used it to fetch
     # vulnerabilities for a non-default-branch, the findings will be unpersisted, so we
     # coerce the POROs into unpersisted AR records to give them a common object.
@@ -80,7 +87,7 @@ module Security
         finding = Vulnerabilities::Finding.new(finding_hash)
         # assigning Vulnerabilities to Findings to enable the computed state
         finding.location_fingerprint = report_finding.location.fingerprint
-        finding.vulnerability_id = vulnerabilities[finding.project_fingerprint]
+        finding.vulnerability = vulnerabilities[finding.project_fingerprint]
         finding.project = pipeline.project
         finding.sha = pipeline.sha
         finding.build_scanner(report_finding.scanner&.to_hash)
@@ -100,6 +107,7 @@ module Security
 
     def filter(findings)
       findings.select do |finding|
+        next unless in_selected_state?(finding)
         next if !include_dismissed? && dismissal_feedback?(finding)
         next unless confidence_levels.include?(finding.confidence)
         next unless severity_levels.include?(finding.severity)
@@ -109,8 +117,26 @@ module Security
       end
     end
 
+    def in_selected_state?(finding)
+      params[:state].blank? || states.include?(computed_finding_state(finding))
+    end
+
+    # Here we are checking the state of the `vulnerability` and preloaded `feedback` records
+    # instead of checking the `finding.state` as the `state` method of the `finding` fires
+    # an additional database query to load the `feedback` record for each `finding`.
+    def computed_finding_state(finding)
+      finding.vulnerability&.state ||
+        (dismissal_feedback?(finding) ? 'dismissed' : 'detected')
+    end
+
     def include_dismissed?
-      params[:scope] == 'all'
+      skip_scope_parameter? || params[:scope] == 'all'
+    end
+
+    # If the client explicitly asks for the dismissed findings, we shouldn't
+    # filter by the `scope` parameter as it's `skip_dismissed` by default.
+    def skip_scope_parameter?
+      params[:state].present? && states.include?('dismissed')
     end
 
     def dismissal_feedback?(finding)
@@ -145,19 +171,23 @@ module Security
     end
 
     def confidence_levels
-      Array(params.fetch(:confidence, Vulnerabilities::Finding.confidences.keys))
+      @confidence_levels ||= Array(params.fetch(:confidence, Vulnerabilities::Finding.confidences.keys))
     end
 
     def report_types
-      Array(params.fetch(:report_type, Vulnerabilities::Finding.report_types.keys))
+      @report_types ||= Array(params.fetch(:report_type, Vulnerabilities::Finding.report_types.keys))
     end
 
     def severity_levels
-      Array(params.fetch(:severity, Vulnerabilities::Finding.severities.keys))
+      @severity_levels ||= Array(params.fetch(:severity, Vulnerabilities::Finding.severities.keys))
     end
 
     def scanners
-      Array(params.fetch(:scanner, []))
+      @scanners ||= Array(params.fetch(:scanner, []))
+    end
+
+    def states
+      @state ||= Array(params.fetch(:state, Vulnerability.states.keys))
     end
   end
 end

ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb

@@ -18,6 +18,10 @@ module Resolvers
              required: false,
              description: 'Filter vulnerability findings by Scanner.externalId.'
 
+    argument :state, [Types::VulnerabilityStateEnum],
+             required: false,
+             description: 'Filter vulnerability findings by state.'
+
     def resolve(**args)
       Security::PipelineVulnerabilitiesFinder.new(pipeline: pipeline, params: args).execute.findings
     end

ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb

@@ -333,6 +333,74 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
       end
     end
 
+    context 'by state' do
+      let(:params) { {} }
+      let(:aggregated_report) { described_class.new(pipeline: pipeline, params: params).execute }
+
+      subject(:finding_uuids) { aggregated_report.findings.map(&:uuid) }
+
+      let(:finding_with_feedback) { pipeline.security_reports.reports['sast'].findings.first }
+
+      before do
+        create(:vulnerability_feedback, :dismissal,
+               :sast,
+               project: project,
+               pipeline: pipeline,
+               category: finding_with_feedback.report_type,
+               project_fingerprint: finding_with_feedback.project_fingerprint,
+               vulnerability_data: finding_with_feedback.raw_metadata,
+               finding_uuid: finding_with_feedback.uuid)
+      end
+
+      context 'when the state parameter is not given' do
+        it 'returns all findings' do
+          expect(finding_uuids.length).to be(40)
+        end
+      end
+
+      context 'when the state parameter is given' do
+        let(:params) { { state: state } }
+        let(:finding_with_associated_vulnerability) { pipeline.security_reports.reports['dependency_scanning'].findings.first }
+
+        before do
+          vulnerability = create(:vulnerability, state, project: project)
+
+          create(:vulnerabilities_finding, :identifier,
+                 vulnerability: vulnerability,
+                 report_type: finding_with_associated_vulnerability.report_type,
+                 project: project,
+                 project_fingerprint: finding_with_associated_vulnerability.project_fingerprint,
+                 uuid: finding_with_associated_vulnerability.uuid)
+        end
+
+        context 'when the given state is `dismissed`' do
+          let(:state) { 'dismissed' }
+
+          it { is_expected.to match_array([finding_with_associated_vulnerability.uuid, finding_with_feedback.uuid]) }
+        end
+
+        context 'when the given state is `detected`' do
+          let(:state) { 'detected' }
+
+          it 'returns all detected findings' do
+            expect(finding_uuids.length).to be(40)
+          end
+        end
+
+        context 'when the given state is `confirmed`' do
+          let(:state) { 'confirmed' }
+
+          it { is_expected.to match_array([finding_with_associated_vulnerability.uuid]) }
+        end
+
+        context 'when the given state is `resolved`' do
+          let(:state) { 'resolved' }
+
+          it { is_expected.to match_array([finding_with_associated_vulnerability.uuid]) }
+        end
+      end
+    end
+
     context 'by all filters' do
       context 'with found entity' do
         let(:params) { { report_type: %w[sast dast container_scanning dependency_scanning], scanner: %w[bundler_audit find_sec_bugs gemnasium trivy zaproxy], scope: 'all' } }

ee/spec/graphql/resolvers/pipeline_security_report_findings_resolver_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe Resolvers::PipelineSecurityReportFindingsResolver do
   let_it_be(:pipeline, reload: true) { create(:ci_pipeline, :success, project: project) }
 
   describe '#resolve' do
-    subject { resolve(described_class, obj: pipeline, args: params) }
+    subject(:resolve_query) { resolve(described_class, obj: pipeline, args: params) }
 
     let_it_be(:low_vulnerability_finding) { build(:vulnerabilities_finding, severity: :low, report_type: :dast, project: project) }
     let_it_be(:critical_vulnerability_finding) { build(:vulnerabilities_finding, severity: :critical, report_type: :sast, project: project) }
@@ -49,5 +49,19 @@ RSpec.describe Resolvers::PipelineSecurityReportFindingsResolver do
         is_expected.to contain_exactly(critical_vulnerability_finding, low_vulnerability_finding)
       end
     end
+
+    context 'when given states' do
+      let(:params) { { state: %w(detected confirmed) } }
+
+      before do
+        allow(Security::PipelineVulnerabilitiesFinder).to receive(:new).and_call_original
+      end
+
+      it 'calls the finder class with given parameters' do
+        resolve_query
+
+        expect(Security::PipelineVulnerabilitiesFinder).to have_received(:new).with(pipeline: pipeline, params: params)
+      end
+    end
   end
 end