Merge branch 'vul_flag_info_in_api' into 'master'

Add vulnerability flag information in Vulnerability Findings API

See merge request gitlab-org/gitlab!66775

ee/app/finders/security/findings_finder.rb

@@ -61,6 +61,11 @@ module Security
         finding.project = project
         finding.sha = pipeline.sha
         finding.scanner = security_finding.scanner
+
+        if calculate_false_positive?
+          finding.vulnerability_flags = existing_vulnerability_flags.fetch(security_finding.uuid, [])
+        end
+
         finding.identifiers = identifiers
         finding.signatures = signatures
       end
@@ -74,6 +79,14 @@ module Security
       existing_vulnerabilities.dig(security_finding.scan.scan_type, security_finding.project_fingerprint)&.first
     end
 
+    def existing_vulnerability_flags
+      @existing_vulnerability_flags ||= project.vulnerability_flags_for(security_findings.map(&:uuid))
+    end
+
+    def calculate_false_positive?
+      ::Feature.enabled?(:vulnerability_flags, project) && project.licensed_feature_available?(:sast_fp_reduction)
+    end
+
     def existing_vulnerabilities
       @existing_vulnerabilities ||= begin
         project.vulnerabilities

ee/app/finders/security/pipeline_vulnerabilities_finder.rb

@@ -33,7 +33,9 @@ module Security
 
         normalized_findings = normalize_report_findings(
           report.findings,
-          vulnerabilities_by_finding_fingerprint(report))
+          vulnerabilities_by_finding_fingerprint(report),
+          existing_vulnerability_flags_for(report))
+
         filtered_findings = filter(normalized_findings)
 
         findings.concat(filtered_findings)
@@ -74,12 +76,16 @@ module Security
                               .select(:vulnerability_id, :project_fingerprint)
     end
 
+    def existing_vulnerability_flags_for(report)
+      pipeline.project.vulnerability_flags_for(report.findings.map(&:uuid))
+    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.
     # See https://gitlab.com/gitlab-org/gitlab/issues/33588#note_291849433 for more context
     # on why this happens.
-    def normalize_report_findings(report_findings, vulnerabilities)
+    def normalize_report_findings(report_findings, vulnerabilities, vulnerability_flags)
       report_findings.map do |report_finding|
         finding_hash = report_finding.to_hash
           .except(:compare_key, :identifiers, :location, :scanner, :links, :signatures)
@@ -101,10 +107,19 @@ module Security
           Vulnerabilities::FindingSignature.new(signature.to_hash)
         end
 
+        if calculate_false_positive?
+          finding.vulnerability_flags = vulnerability_flags.fetch(finding.uuid, [])
+        end
+
         finding
       end
     end
 
+    def calculate_false_positive?
+      project = pipeline.project
+      ::Feature.enabled?(:vulnerability_flags, project) && project.licensed_feature_available?(:sast_fp_reduction)
+    end
+
     def filter(findings)
       findings.select do |finding|
         next unless in_selected_state?(finding)

ee/app/models/concerns/ee/vulnerability_flag_helpers.rb

+# frozen_string_literal: true
+
+module EE
+  module VulnerabilityFlagHelpers
+    def vulnerability_flags_for(uuids)
+      vulnerability_findings
+        .by_uuid(uuids)
+        .eager_load_vulnerability_flags
+        .to_h { |finding| [finding.uuid, finding.vulnerability_flags] }
+    end
+  end
+end

ee/app/models/ee/project.rb

@@ -21,6 +21,7 @@ module EE
       include DeprecatedApprovalsBeforeMerge
       include UsageStatistics
       include ProjectSecurityScannersInformation
+      include VulnerabilityFlagHelpers
 
       before_save :set_override_pull_mirror_available, unless: -> { ::Gitlab::CurrentSettings.mirror_available }
       before_save :set_next_execution_timestamp_to_now, if: ->(project) { project.mirror? && project.mirror_changed? && project.import_state }

ee/app/models/vulnerabilities/finding.rb

@@ -87,6 +87,7 @@ module Vulnerabilities
     scope :by_severities, -> (values) { where(severity: values) }
     scope :by_confidences, -> (values) { where(confidence: values) }
     scope :by_project_fingerprints, -> (values) { where(project_fingerprint: values) }
+    scope :by_uuid, -> (uuids) { where(uuid: uuids) }
 
     scope :all_preloaded, -> do
       preload(:scanner, :identifiers, project: [:namespace, :project_feature])

ee/app/serializers/vulnerabilities/finding_entity.rb

@@ -12,6 +12,9 @@ class Vulnerabilities::FindingEntity < Grape::Entity
   expose :create_jira_issue_url do |occurrence|
     create_jira_issue_url_for(occurrence)
   end
+  expose :false_positive, if: -> (_, _) { expose_false_positive? } do |occurrence|
+    occurrence.vulnerability_flags.any?(&:false_positive?)
+  end
   expose :create_vulnerability_feedback_issue_path do |occurrence|
     create_vulnerability_feedback_issue_path(occurrence.project)
   end
@@ -54,6 +57,13 @@ class Vulnerabilities::FindingEntity < Grape::Entity
   def current_user
     return request.current_user if request.respond_to?(:current_user)
   end
+
+  private
+
+  def expose_false_positive?
+    project = occurrence.project
+    ::Feature.enabled?(:vulnerability_flags, project) && project.licensed_feature_available?(:sast_fp_reduction)
+  end
 end
 
 Vulnerabilities::FindingEntity.include_mod_with('ProjectsHelper')

ee/config/feature_flags/development/vulnerability_flags.yml

+---
+name: vulnerability_flags
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66775
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/336630
+milestone: '14.2'
+type: development
+group: group::static analysis
+default_enabled: false

ee/spec/finders/security/findings_finder_spec.rb

@@ -59,6 +59,8 @@ RSpec.describe Security::FindingsFinder do
                    deduplicated: true,
                    position: index,
                    scan: scan)
+
+            create(:vulnerabilities_finding, uuid: finding.uuid, project: pipeline.project)
           end
         end
 
@@ -76,9 +78,21 @@ RSpec.describe Security::FindingsFinder do
         stub_licensed_features(sast: true, dependency_scanning: true)
       end
 
+      context 'N+1 queries' do
+        it 'does not cause N+1 queries' do
+          expect { finder_result }.not_to exceed_query_limit(10)
+        end
+
+        context 'with vulnerability_flags disabled' do
+          before do
+            stub_feature_flags(vulnerability_flags: false)
+          end
+
           it 'does not cause N+1 queries' do
             expect { finder_result }.not_to exceed_query_limit(8)
           end
+        end
+      end
 
       describe '#current_page' do
         subject { finder_result.current_page }
@@ -169,6 +183,35 @@ RSpec.describe Security::FindingsFinder do
         end
       end
 
+      describe '#vulnerability_flags' do
+        before do
+          stub_licensed_features(sast_fp_reduction: true)
+        end
+
+        context 'with no vulnerability flags present' do
+          it 'does not have any vulnerability flag' do
+            expect(finder_result.findings).to all(have_attributes(vulnerability_flags: be_empty))
+          end
+        end
+
+        context 'with some vulnerability flags present' do
+          before do
+            create(:vulnerabilities_flag, finding: pipeline.project.vulnerability_findings.first)
+            create(:vulnerabilities_flag, finding: pipeline.project.vulnerability_findings.last)
+          end
+
+          it 'has some vulnerability_findings with vulnerability flag' do
+            expect(finder_result.findings).to include(have_attributes(vulnerability_flags: be_present))
+          end
+
+          it 'does not have any vulnerability_flag if license is not available' do
+            stub_licensed_features(sast_fp_reduction: false)
+
+            expect(finder_result.findings).to all(have_attributes(vulnerability_flags: be_empty))
+          end
+        end
+      end
+
       describe '#findings' do
         subject { finder_result.findings.map(&:project_fingerprint) }
 

ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb

@@ -38,7 +38,7 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
     end
 
     before do
-      stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true, dast: true)
+      stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true, dast: true, sast_fp_reduction: true)
       # Stub out deduplication, if not done the expectations will vary based on the fixtures (which may/may not have duplicates)
       disable_deduplication
     end
@@ -137,6 +137,33 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
           expect(subject.findings.map(&:uuid)).to match_array(sast_report_uuids)
           expect(subject.findings.count).to eq(sast_count)
         end
+
+        context "false-positive" do
+          before do
+            vulnerability_finding = create(:vulnerabilities_finding, uuid: sast_report_uuids.first, project: pipeline.project)
+            create(:vulnerabilities_flag, finding: vulnerability_finding)
+          end
+
+          it 'includes findings with false-positive' do
+            expect(subject.findings.flat_map(&:vulnerability_flags)).to be_present
+          end
+
+          it 'does not include findings with false-positive if license is not available' do
+            stub_licensed_features(sast_fp_reduction: false)
+
+            expect(subject.findings).to all(have_attributes(vulnerability_flags: be_empty))
+          end
+
+          context 'with vulnerability_flags FF disabled' do
+            before do
+              stub_feature_flags(vulnerability_flags: false)
+            end
+
+            it 'does not include findings with false-positive' do
+              expect(subject.findings).to all(have_attributes(vulnerability_flags: be_empty))
+            end
+          end
+        end
       end
 
       context 'when dependency_scanning' do

ee/spec/requests/api/vulnerability_findings_spec.rb

@@ -69,7 +69,8 @@ RSpec.describe API::VulnerabilityFindings do
 
         # Threshold is required for the extra query performed in Security::PipelineVulnerabilitiesFinder to load
         # the Vulnerabilities providing computed states for the associated Vulnerability::Findings
-        expect { get api(project_vulnerability_findings_path, user) }.not_to exceed_query_limit(control_count).with_threshold(1)
+        # and all associated vulnerability_flags
+        expect { get api(project_vulnerability_findings_path, user) }.not_to exceed_query_limit(control_count).with_threshold(3)
       end
 
       describe 'using different finders' do

ee/spec/serializers/vulnerabilities/finding_entity_spec.rb

@@ -23,10 +23,17 @@ RSpec.describe Vulnerabilities::FindingEntity do
       scanner: scanner,
       scan: scan,
       project: project,
-      identifiers: identifiers
+      identifiers: identifiers,
+      vulnerability_flags: flags
     )
   end
 
+  let(:flags) do
+    [
+      build(:vulnerabilities_flag)
+    ]
+  end
+
   let(:dismiss_feedback) do
     build(:vulnerability_feedback, :sast, :dismissal,
            project: project, project_fingerprint: occurrence.project_fingerprint)
@@ -47,6 +54,7 @@ RSpec.describe Vulnerabilities::FindingEntity do
     subject { entity.as_json }
 
     before do
+      stub_licensed_features(sast_fp_reduction: true)
       allow(request).to receive(:current_user).and_return(user)
     end
 
@@ -58,11 +66,30 @@ RSpec.describe Vulnerabilities::FindingEntity do
       expect(subject).to include(:description, :links, :location, :remediations, :solution, :evidence)
       expect(subject).to include(:blob_path, :request, :response)
       expect(subject).to include(:scan)
+      expect(subject).to include(:false_positive)
       expect(subject).to include(:assets, :evidence_source, :supporting_messages)
       expect(subject).to include(:uuid)
       expect(subject).to include(:details)
     end
 
+    context 'false-positive' do
+      it 'finds the vulnerability_finding as false_positive' do
+        expect(subject[:false_positive]).to be(true)
+      end
+
+      it 'does not contain false_positive field if feature_flag is disabled' do
+        stub_feature_flags(vulnerability_flags: false)
+
+        expect(subject).not_to include(:false_positive)
+      end
+
+      it 'does not contain false_positive field if license is not available' do
+        stub_licensed_features(sast_fp_reduction: false)
+
+        expect(subject).not_to include(:false_positive)
+      end
+    end
+
     context 'when not allowed to admin vulnerability feedback' do
       before do
         project.add_guest(user)