Automatic merge of gitlab-org/gitlab master

app/controllers/jira_connect/events_controller.rb

@@ -19,7 +19,7 @@ class JiraConnect::EventsController < JiraConnect::ApplicationController
   end
 
   def uninstalled
-    if current_jira_installation.destroy
+    if JiraConnectInstallations::DestroyService.execute(current_jira_installation, jira_connect_base_path, jira_connect_events_uninstalled_path)
       head :ok
     else
       head :unprocessable_entity

app/services/jira_connect_installations/destroy_service.rb

+# frozen_string_literal: true
+
+module JiraConnectInstallations
+  class DestroyService
+    def self.execute(installation, jira_connect_base_path, jira_connect_uninstalled_event_path)
+      new(installation, jira_connect_base_path, jira_connect_uninstalled_event_path).execute
+    end
+
+    def initialize(installation, jira_connect_base_path, jira_connect_uninstalled_event_path)
+      @installation = installation
+      @jira_connect_base_path = jira_connect_base_path
+      @jira_connect_uninstalled_event_path = jira_connect_uninstalled_event_path
+    end
+
+    def execute
+      if @installation.instance_url.present?
+        JiraConnect::ForwardEventWorker.perform_async(@installation.id, @jira_connect_base_path, @jira_connect_uninstalled_event_path)
+        return true
+      end
+
+      @installation.destroy
+    end
+  end
+end

app/workers/all_queues.yml

@@ -1078,6 +1078,15 @@
   :weight: 2
   :idempotent: true
   :tags: []
+- :name: jira_connect:jira_connect_forward_event
+  :worker_name: JiraConnect::ForwardEventWorker
+  :feature_category: :integrations
+  :has_external_dependencies: true
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: jira_connect:jira_connect_sync_branch
   :worker_name: JiraConnect::SyncBranchWorker
   :feature_category: :integrations

app/workers/jira_connect/forward_event_worker.rb

+# frozen_string_literal: true
+
+module JiraConnect
+  class ForwardEventWorker # rubocop:disable Scalability/IdempotentWorker
+    include ApplicationWorker
+
+    queue_namespace :jira_connect
+    feature_category :integrations
+    worker_has_external_dependencies!
+
+    def perform(installation_id, base_path, event_path)
+      installation = JiraConnectInstallation.find_by_id(installation_id)
+
+      return if installation&.instance_url.nil?
+
+      proxy_url = installation.instance_url + event_path
+      qsh = Atlassian::Jwt.create_query_string_hash(proxy_url, 'POST', installation.instance_url + base_path)
+      jwt = Atlassian::Jwt.encode({ iss: installation.client_key, qsh: qsh }, installation.shared_secret)
+
+      Gitlab::HTTP.post(proxy_url, headers: { 'Authorization' => "JWT #{jwt}" })
+    ensure
+      installation.destroy if installation
+    end
+  end
+end

doc/api/graphql/reference/index.md

@@ -11205,6 +11205,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`
 

doc/ci/runners/build_cloud/linux_build_cloud.md

@@ -27,7 +27,6 @@ Below are the shared runners settings.
 
 | Setting                               | GitLab.com                                        | Default    |
 | -----------                           | -----------------                                 | ---------- |
-| [GitLab Runner](https://gitlab.com/gitlab-org/gitlab-runner) | [Runner versions dashboard](https://dashboards.gitlab.net/d/ci-runners-deployment/ci-runners-deployment-overview?orgId=1&refresh=1m) | - |
 | Executor                              | `docker+machine`                                  | -          |
 | Default Docker image                  | `ruby:2.5`                                        | -          |
 | `privileged` (run [Docker in Docker](https://hub.docker.com/_/docker/)) | `true`          | `false`    |

doc/development/documentation/testing.md

@@ -221,7 +221,7 @@ guidelines:
 | *correctly-capitalized* name of a product or service | Add the word to the [vale spelling exceptions list](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/.vale/gitlab/spelling-exceptions.txt). |
 | name of a person                                     | Remove the name if it's not needed, or [add the vale exception code in-line](#disable-vale-tests). |
 | a command, variable, code, or similar                | Put it in backticks or a code block. For example: ``The git clone command can be used with the CI_COMMIT_BRANCH variable.`` -> ``The `git clone` command can be used with the `CI_COMMIT_BRANCH` variable.`` |
-| UI text from GitLab                                  | Verify it correctly matches the UI, then: <ul><li>If it does not match the UI, update it.</li><li>If it matches the UI, but the UI seems incorrect, create an issue to see if the UI needs to be fixed.</li><li>If it matches the UI and seems correct, add it to the [vale spelling exceptions list](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/.vale/gitlab/spelling-exceptions.txt).</li></ul> |
+| UI text from GitLab                                  | Verify it correctly matches the UI, then: If it does not match the UI, update it. If it matches the UI, but the UI seems incorrect, create an issue to see if the UI needs to be fixed. If it matches the UI and seems correct, add it to the [vale spelling exceptions list](https://gitlab.com/gitlab-org/gitlab/-/blob/master/doc/.vale/gitlab/spelling-exceptions.txt). |
 | UI text from a third-party product                   | Rewrite the sentence to avoid it, or [add the vale exception code in-line](#disable-vale-tests). |
 
 ### Install linters

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,18 +57,21 @@ 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))
+    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)
+                              .by_report_types(report.type)
+                              .includes(:vulnerability) # rubocop:disable CodeReuse/ActiveRecord (We will remove this class)
                               .select(:vulnerability_id, :project_fingerprint)
-       .each_with_object({}) do |finding, hash|
-        hash[finding.project_fingerprint] = finding.vulnerability_id
-      end
     end
 
     # This finder is used for fetching vulnerabilities for any pipeline, if we used it to fetch
@@ -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

spec/controllers/jira_connect/events_controller_spec.rb

@@ -66,19 +66,19 @@ RSpec.describe JiraConnect::EventsController do
         request.headers['Authorization'] = "JWT #{auth_token}"
       end
 
-      subject { post :uninstalled }
+      subject(:post_uninstalled) { post :uninstalled }
 
       context 'when JWT is invalid' do
         let(:auth_token) { 'invalid_token' }
 
         it 'returns 403' do
-          subject
+          post_uninstalled
 
           expect(response).to have_gitlab_http_status(:forbidden)
         end
 
         it 'does not delete the installation' do
-          expect { subject }.not_to change { JiraConnectInstallation.count }
+          expect { post_uninstalled }.not_to change { JiraConnectInstallation.count }
         end
       end
 
@@ -87,8 +87,27 @@ RSpec.describe JiraConnect::EventsController do
           Atlassian::Jwt.encode({ iss: installation.client_key, qsh: qsh }, installation.shared_secret)
         end
 
-        it 'deletes the installation' do
-          expect { subject }.to change { JiraConnectInstallation.count }.by(-1)
+        let(:jira_base_path) { '/-/jira_connect' }
+        let(:jira_event_path) { '/-/jira_connect/events/uninstalled' }
+
+        it 'calls the DestroyService and returns ok in case of success' do
+          expect_next_instance_of(JiraConnectInstallations::DestroyService, installation, jira_base_path, jira_event_path) do |destroy_service|
+            expect(destroy_service).to receive(:execute).and_return(true)
+          end
+
+          post_uninstalled
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+
+        it 'calls the DestroyService and returns unprocessable_entity in case of failure' do
+          expect_next_instance_of(JiraConnectInstallations::DestroyService, installation, jira_base_path, jira_event_path) do |destroy_service|
+            expect(destroy_service).to receive(:execute).and_return(false)
+          end
+
+          post_uninstalled
+
+          expect(response).to have_gitlab_http_status(:unprocessable_entity)
         end
       end
     end

spec/services/jira_connect_installations/destroy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe JiraConnectInstallations::DestroyService do
+  describe '.execute' do
+    it 'creates an instance and calls execute' do
+      expect_next_instance_of(described_class, 'param1', 'param2', 'param3') do |destroy_service|
+        expect(destroy_service).to receive(:execute)
+      end
+
+      described_class.execute('param1', 'param2', 'param3')
+    end
+  end
+
+  describe '#execute' do
+    let!(:installation) { create(:jira_connect_installation) }
+    let(:jira_base_path) { '/-/jira_connect' }
+    let(:jira_event_path) { '/-/jira_connect/events/uninstalled' }
+
+    subject { described_class.new(installation, jira_base_path, jira_event_path).execute }
+
+    it { is_expected.to be_truthy }
+
+    it 'deletes the installation' do
+      expect { subject }.to change(JiraConnectInstallation, :count).by(-1)
+    end
+
+    context 'and the installation has an instance_url set' do
+      let!(:installation) { create(:jira_connect_installation, instance_url: 'http://example.com') }
+
+      it { is_expected.to be_truthy }
+
+      it 'schedules a ForwardEventWorker background job and keeps the installation' do
+        expect(JiraConnect::ForwardEventWorker).to receive(:perform_async).with(installation.id, jira_base_path, jira_event_path)
+
+        expect { subject }.not_to change(JiraConnectInstallation, :count)
+      end
+    end
+  end
+end

spec/workers/jira_connect/forward_event_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe JiraConnect::ForwardEventWorker do
+  describe '#perform' do
+    let!(:jira_connect_installation) { create(:jira_connect_installation, instance_url: self_managed_url, client_key: client_key, shared_secret: shared_secret) }
+    let(:base_path) { '/-/jira_connect' }
+    let(:event_path) { '/-/jira_connect/events/uninstalled' }
+
+    let(:self_managed_url) { 'http://example.com' }
+    let(:base_url) { self_managed_url + base_path }
+    let(:event_url) { self_managed_url + event_path }
+
+    let(:client_key) { '123' }
+    let(:shared_secret) { '123' }
+
+    subject { described_class.new.perform(jira_connect_installation.id, base_path, event_path) }
+
+    it 'forwards the event including the auth header and deletes the installation' do
+      stub_request(:post, event_url)
+
+      expect(Atlassian::Jwt).to receive(:create_query_string_hash).with(event_url, 'POST', base_url).and_return('some_qsh')
+      expect(Atlassian::Jwt).to receive(:encode).with({ iss: client_key, qsh: 'some_qsh' }, shared_secret).and_return('auth_token')
+      expect { subject }.to change(JiraConnectInstallation, :count).by(-1)
+
+      expect(WebMock).to have_requested(:post, event_url).with(headers: { 'Authorization' => 'JWT auth_token' })
+    end
+
+    context 'when installation does not exist' do
+      let(:jira_connect_installation) { instance_double(JiraConnectInstallation, id: -1) }
+
+      it 'does nothing' do
+        expect { subject }.not_to change(JiraConnectInstallation, :count)
+      end
+    end
+
+    context 'when installation does not have an instance_url' do
+      let!(:jira_connect_installation) { create(:jira_connect_installation) }
+
+      it 'forwards the event including the auth header' do
+        expect { subject }.to change(JiraConnectInstallation, :count).by(-1)
+
+        expect(WebMock).not_to have_requested(:post, '*')
+      end
+    end
+
+    context 'when it fails to forward the event' do
+      it 'still deletes the installation' do
+        allow(Gitlab::HTTP).to receive(:post).and_raise(StandardError)
+
+        expect { subject }.to raise_error(StandardError).and change(JiraConnectInstallation, :count).by(-1)
+      end
+    end
+  end
+end