Set vulnerability as dismissed when there is dismissal feedback

This change adds background migration to fix problem with
vulnerabilities that are not dismissed, when they have dismissal
feedback created for associated findings.

changelogs/unreleased/280589-set-vulnerabilities-as-dismissed-when-they-have-dismissed-feedback.yml

+---
+title: Set vulnerability as dismissed when there is dismissal feedback
+merge_request: 48795
+author:
+type: added

db/post_migrate/20201130103926_schedule_populate_dismissed_state_for_vulnerabilities.rb

+# frozen_string_literal: true
+
+class SchedulePopulateDismissedStateForVulnerabilities < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  TMP_INDEX_NAME = 'tmp_index_on_vulnerabilities_non_dismissed'
+
+  DOWNTIME = false
+  BATCH_SIZE = 1_000
+  VULNERABILITY_BATCH_SIZE = 5_000
+  DELAY_INTERVAL = 3.minutes.to_i
+  MIGRATION_CLASS = 'PopulateDismissedStateForVulnerabilities'
+
+  VULNERABILITY_JOIN_CONDITION = 'JOIN "vulnerability_occurrences" ON "vulnerability_occurrences"."vulnerability_id" = "vulnerabilities"."id"'
+  FEEDBACK_WHERE_CONDITION = <<~SQL
+    EXISTS (SELECT 1 FROM vulnerability_feedback
+      WHERE "vulnerability_occurrences"."project_id" = "vulnerability_feedback"."project_id"
+      AND "vulnerability_occurrences"."report_type" = "vulnerability_feedback"."category"
+      AND ENCODE("vulnerability_occurrences"."project_fingerprint", 'hex') = "vulnerability_feedback"."project_fingerprint"
+      AND "vulnerability_feedback"."feedback_type" = 0
+    )
+  SQL
+
+  class Vulnerability < ActiveRecord::Base # rubocop:disable Style/Documentation
+    include EachBatch
+
+    self.table_name = 'vulnerabilities'
+  end
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:vulnerabilities, :id, where: 'state <> 2', name: TMP_INDEX_NAME)
+
+    batch = []
+    index = 1
+
+    Vulnerability.where('state <> 2').each_batch(of: VULNERABILITY_BATCH_SIZE) do |relation|
+      ids = relation
+        .joins(VULNERABILITY_JOIN_CONDITION)
+        .where(FEEDBACK_WHERE_CONDITION)
+        .pluck('vulnerabilities.id')
+
+      ids.each do |id|
+        batch << id
+
+        if batch.size == BATCH_SIZE
+          migrate_in(index * DELAY_INTERVAL, MIGRATION_CLASS, batch)
+          index += 1
+
+          batch.clear
+        end
+      end
+    end
+
+    migrate_in(index * DELAY_INTERVAL, MIGRATION_CLASS, batch) unless batch.empty?
+  end
+
+  def down
+    remove_concurrent_index_by_name(:vulnerabilities, TMP_INDEX_NAME)
+  end
+end

db/schema_migrations/20201130103926

+27cd7e7cd01175c157e6aa666b2263bf29210277d5acd997a0619cee67870345
\ No newline at end of file

db/structure.sql

@@ -22873,6 +22873,8 @@ CREATE INDEX tmp_build_stage_position_index ON ci_builds USING btree (stage_id,
 
 CREATE INDEX tmp_index_for_email_unconfirmation_migration ON emails USING btree (id) WHERE (confirmed_at IS NOT NULL);
 
+CREATE INDEX tmp_index_on_vulnerabilities_non_dismissed ON vulnerabilities USING btree (id) WHERE (state <> 2);
+
 CREATE UNIQUE INDEX unique_merge_request_metrics_by_merge_request_id ON merge_request_metrics USING btree (merge_request_id);
 
 CREATE UNIQUE INDEX vulnerability_feedback_unique_idx ON vulnerability_feedback USING btree (project_id, category, feedback_type, project_fingerprint);

ee/spec/migrations/schedule_populate_dismissed_state_for_vulnerabilities_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20201130103926_schedule_populate_dismissed_state_for_vulnerabilities.rb')
+
+RSpec.describe SchedulePopulateDismissedStateForVulnerabilities do
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:vulnerabilities) { table(:vulnerabilities) }
+  let(:findings) { table(:vulnerability_occurrences) }
+  let(:feedback) { table(:vulnerability_feedback) }
+  let(:identifiers) { table(:vulnerability_identifiers) }
+  let(:scanners) { table(:vulnerability_scanners) }
+
+  let!(:namespace) { namespaces.create!(name: "foo", path: "bar") }
+  let!(:user) { users.create!(name: 'John Doe', email: 'test@example.com', projects_limit: 5) }
+  let!(:project) { projects.create!(namespace_id: namespace.id) }
+  let!(:scanner) { scanners.create!(project_id: project.id, external_id: 'foo', name: 'bar') }
+  let!(:identifier) { identifiers.create!(project_id: project.id, fingerprint: 'foo', external_type: 'bar', external_id: 'zoo', name: 'identifier') }
+  let!(:vulnerability_params) do
+    {
+      project_id: project.id,
+      author_id: user.id,
+      title: 'Vulnerability',
+      severity: 5,
+      confidence: 5,
+      report_type: 5
+    }
+  end
+
+  let!(:identifier_1) { identifiers.create!(project_id: project.id, fingerprint: 'foo1', external_type: 'bar', external_id: 'zoo', name: 'identifier') }
+  let!(:identifier_2) { identifiers.create!(project_id: project.id, fingerprint: 'foo2', external_type: 'bar', external_id: 'zoo', name: 'identifier') }
+  let!(:identifier_3) { identifiers.create!(project_id: project.id, fingerprint: 'foo3', external_type: 'bar', external_id: 'zoo', name: 'identifier') }
+
+  let!(:feedback_1) { feedback.create!(feedback_type: 0, category: 'sast', project_fingerprint: '418291a26024a1445b23fe64de9380cdcdfd1fa8', project_id: project.id, author_id: user.id) }
+  let!(:feedback_2) { feedback.create!(feedback_type: 0, category: 'dast', project_fingerprint: 'a98c8aed53514eddba2976b942162bf3418291a2', project_id: project.id, author_id: user.id) }
+  let!(:feedback_3) { feedback.create!(feedback_type: 0, category: 'dast', project_fingerprint: 'a98c8aed53514eddba2976b942162bf3418291a3', project_id: project.id, author_id: user.id) }
+
+  let!(:vulnerability_1) { vulnerabilities.create!(vulnerability_params.merge(state: 1)) }
+  let!(:vulnerability_2) { vulnerabilities.create!(vulnerability_params.merge(state: 3)) }
+  let!(:vulnerability_3) { vulnerabilities.create!(vulnerability_params.merge(state: 2)) }
+
+  let!(:finding_1) do
+    findings.create!(name: 'Finding',
+      report_type: 'sast',
+      project_fingerprint: Gitlab::Database::ShaAttribute.new.serialize('418291a26024a1445b23fe64de9380cdcdfd1fa8'),
+      location_fingerprint: 'bar',
+      severity: 1,
+      confidence: 1,
+      metadata_version: 1,
+      raw_metadata: '',
+      uuid: SecureRandom.uuid,
+      project_id: project.id,
+      vulnerability_id: vulnerability_1.id,
+      scanner_id: scanner.id,
+      primary_identifier_id: identifier_1.id)
+  end
+
+  let!(:finding_2) do
+    findings.create!(name: 'Finding',
+      report_type: 'dast',
+      project_fingerprint: Gitlab::Database::ShaAttribute.new.serialize('a98c8aed53514eddba2976b942162bf3418291a2'),
+      location_fingerprint: 'bar',
+      severity: 1,
+      confidence: 1,
+      metadata_version: 1,
+      raw_metadata: '',
+      uuid: SecureRandom.uuid,
+      project_id: project.id,
+      vulnerability_id: vulnerability_2.id,
+      scanner_id: scanner.id,
+      primary_identifier_id: identifier_2.id)
+  end
+
+  let!(:finding_3) do
+    findings.create!(name: 'Finding',
+      report_type: 'dast',
+      project_fingerprint: Gitlab::Database::ShaAttribute.new.serialize('a98c8aed53514eddba2976b942162bf3418291a3'),
+      location_fingerprint: 'bar',
+      severity: 1,
+      confidence: 1,
+      metadata_version: 1,
+      raw_metadata: '',
+      uuid: SecureRandom.uuid,
+      project_id: project.id,
+      vulnerability_id: vulnerability_3.id,
+      scanner_id: scanner.id,
+      primary_identifier_id: identifier_3.id)
+  end
+
+  it 'correctly schedules background migrations invalid vulnerabilities only', :aggregate_failures do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+
+    Sidekiq::Testing.fake! do
+      freeze_time do
+        migrate!
+
+        expect(described_class::MIGRATION_CLASS)
+          .to be_scheduled_delayed_migration(3.minutes, 1)
+
+        expect(described_class::MIGRATION_CLASS)
+          .to be_scheduled_delayed_migration(6.minutes, 2)
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+      end
+    end
+  end
+end

lib/gitlab/background_migration/populate_dismissed_state_for_vulnerabilities.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # This class updates vulnerabilities entities with state dismissed
+    class PopulateDismissedStateForVulnerabilities
+      class Vulnerability < ActiveRecord::Base # rubocop:disable Style/Documentation
+        self.table_name = 'vulnerabilities'
+      end
+
+      def perform(*vulnerability_ids)
+        Vulnerability.where(id: vulnerability_ids).update_all(state: 2)
+        PopulateMissingVulnerabilityDismissalInformation.new.perform(*vulnerability_ids)
+      end
+    end
+  end
+end

lib/gitlab/background_migration/populate_missing_vulnerability_dismissal_information.rb

@@ -26,13 +26,16 @@ module Gitlab
 
       class Finding < ActiveRecord::Base # rubocop:disable Style/Documentation
         include ShaAttribute
+        include ::Gitlab::Utils::StrongMemoize
 
         self.table_name = 'vulnerability_occurrences'
 
         sha_attribute :project_fingerprint
 
         def dismissal_feedback
-          Feedback.dismissal.where(category: report_type, project_fingerprint: project_fingerprint, project_id: project_id).first
+          strong_memoize(:dismissal_feedback) do
+            Feedback.dismissal.where(category: report_type, project_fingerprint: project_fingerprint, project_id: project_id).first
+          end
         end
       end
 

spec/lib/gitlab/background_migration/populate_dismissed_state_for_vulnerabilities_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Gitlab::BackgroundMigration::PopulateDismissedStateForVulnerabilities, schema: 2020_11_30_103926 do
+  let(:users) { table(:users) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:vulnerabilities) { table(:vulnerabilities) }
+
+  let!(:namespace) { namespaces.create!(name: "foo", path: "bar") }
+  let!(:user) { users.create!(name: 'John Doe', email: 'test@example.com', projects_limit: 5) }
+  let!(:project) { projects.create!(namespace_id: namespace.id) }
+  let!(:vulnerability_params) do
+    {
+      project_id: project.id,
+      author_id: user.id,
+      title: 'Vulnerability',
+      severity: 5,
+      confidence: 5,
+      report_type: 5
+    }
+  end
+
+  let!(:vulnerability_1) { vulnerabilities.create!(vulnerability_params.merge(state: 1)) }
+  let!(:vulnerability_2) { vulnerabilities.create!(vulnerability_params.merge(state: 3)) }
+
+  describe '#perform' do
+    it 'changes state of vulnerability to dismissed' do
+      subject.perform(vulnerability_1.id, vulnerability_2.id)
+
+      expect(vulnerability_1.reload.state).to eq(2)
+      expect(vulnerability_2.reload.state).to eq(2)
+    end
+
+    it 'populates missing dismissal information' do
+      expect_next_instance_of(::Gitlab::BackgroundMigration::PopulateMissingVulnerabilityDismissalInformation) do |migration|
+        expect(migration).to receive(:perform).with(vulnerability_1.id, vulnerability_2.id)
+      end
+
+      subject.perform(vulnerability_1.id, vulnerability_2.id)
+    end
+  end
+end