Merge branch '213623_calculate_vulnerability_statistics' into 'master'

Calculate and store vulnerability statistics on database

See merge request gitlab-org/gitlab!35052

ee/app/models/vulnerabilities/stat_diff.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class StatDiff
+    def initialize(vulnerability)
+      self.vulnerability = vulnerability
+    end
+
+    def changed_attributes
+      update_required? ? changes.keys : []
+    end
+
+    def changed_values
+      update_required? ? changes.values : []
+    end
+
+    def changes
+      @changes ||= total_change.merge(severity_changes)
+    end
+
+    def update_required?
+      severity_changes.present?
+    end
+
+    delegate :project_id, to: :vulnerability
+
+    private
+
+    delegate :active_states, :passive_states, to: Vulnerability, private: true
+    delegate :destroyed?, to: :vulnerability, prefix: true, private: true
+    delegate :state, :severity, :state_previous_change, :severity_previous_change, :severity_previously_changed?, :state_previously_changed?,
+             to: :vulnerability, private: true
+
+    attr_accessor :vulnerability
+
+    def total_change
+      { 'total' => severity_changes.values.sum }
+    end
+
+    def severity_changes
+      @severity_changes ||= previous_severity_value.merge(current_severity_value)
+    end
+
+    def previous_severity_value
+      decrease_previous_severity? ? { previous_severity => -1 } : {}
+    end
+
+    def current_severity_value
+      if decrease_current_severity?
+        { severity => -1 }
+      elsif increment_current_severity?
+        { severity => 1 }
+      else
+        {}
+      end
+    end
+
+    def decrease_previous_severity?
+      previous_severity && (state_moved_to_passive? || state_remained_active?)
+    end
+
+    def decrease_current_severity?
+      vulnerability_destroyed? || (!severity_previously_changed? && state_moved_to_passive?)
+    end
+
+    def increment_current_severity?
+      (severity_previously_changed? && state.in?(active_states)) || state_moved_to_active?
+    end
+
+    def state_moved_to_passive?
+      previous_state.in?(active_states) && state.in?(passive_states)
+    end
+
+    def state_moved_to_active?
+      previous_state.in?(passive_states) && state.in?(active_states)
+    end
+
+    def state_remained_active?
+      state.in?(active_states) && (!state_previously_changed? || previous_state.in?(active_states))
+    end
+
+    def previous_state
+      state_previous_change&.first
+    end
+
+    def previous_severity
+      severity_previous_change&.first
+    end
+  end
+end

ee/app/models/vulnerabilities/statistic.rb

@@ -15,5 +15,32 @@ module Vulnerabilities
     validates :low, numericality: { greater_than_or_equal_to: 0 }
     validates :unknown, numericality: { greater_than_or_equal_to: 0 }
     validates :info, numericality: { greater_than_or_equal_to: 0 }
+
+    before_save :assign_letter_grade
+
+    class << self
+      # Takes an object which responds to `#[]` method call
+      # like an instance of ActiveRecord::Base or a Hash and
+      # returns the letter grade value for given object.
+      def letter_grade_for(object)
+        if object['critical'].to_i > 0
+          letter_grades[:f]
+        elsif object['high'].to_i > 0 || object['unknown'].to_i > 0
+          letter_grades[:d]
+        elsif object['medium'].to_i > 0
+          letter_grades[:c]
+        elsif object['low'].to_i > 0
+          letter_grades[:b]
+        else
+          letter_grades[:a]
+        end
+      end
+    end
+
+    private
+
+    def assign_letter_grade
+      self.letter_grade = self.class.letter_grade_for(self)
+    end
   end
 end

ee/app/models/vulnerability.rb

@@ -10,6 +10,8 @@ class Vulnerability < ApplicationRecord
   TooManyDaysError = Class.new(StandardError)
 
   MAX_DAYS_OF_HISTORY = 10
+  ACTIVE_STATES = %w(detected confirmed).freeze
+  PASSIVE_STATES = %w(dismissed resolved).freeze
 
   cache_markdown_field :title, pipeline: :single_line
   cache_markdown_field :description, issuable_state_filter_enabled: true
@@ -100,6 +102,14 @@ class Vulnerability < ApplicationRecord
         :day, :severity
       )
     end
+
+    def active_states
+      ACTIVE_STATES
+    end
+
+    def passive_states
+      PASSIVE_STATES
+    end
   end
 
   # There will only be one finding associated with a vulnerability for the foreseeable future
@@ -143,6 +153,10 @@ class Vulnerability < ApplicationRecord
   alias_method :after_note_created,   :after_note_changed
   alias_method :after_note_destroyed, :after_note_changed
 
+  def stat_diff
+    Vulnerabilities::StatDiff.new(self)
+  end
+
   private
 
   def user_notes_count_service

ee/app/services/vulnerabilities/base_service.rb

@@ -16,6 +16,7 @@ module Vulnerabilities
       return false unless vulnerability.update(params)
 
       SystemNoteService.change_vulnerability_state(vulnerability, @user) if vulnerability.state_previously_changed?
+      Vulnerabilities::Statistics::UpdateService.update_for(vulnerability)
       true
     end
 

ee/app/services/vulnerabilities/create_service.rb

@@ -20,6 +20,7 @@ module Vulnerabilities
         finding = @project.vulnerability_findings.lock_for_confirmation!(@finding_id)
 
         save_vulnerability(vulnerability, finding)
+        Statistics::UpdateService.update_for(vulnerability)
       rescue ActiveRecord::RecordNotFound
         vulnerability.errors.add(:base, _('finding is not found or is already attached to a vulnerability'))
         raise ActiveRecord::Rollback

ee/app/services/vulnerabilities/statistics/update_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  module Statistics
+    class UpdateService
+      LETTER_GRADE_SQL = <<~SQL
+        CASE
+        WHEN TARGET.critical + EXCLUDED.critical > 0 THEN
+          #{Statistic.letter_grades[:f]}
+        WHEN TARGET.high + TARGET.unknown + EXCLUDED.high + EXCLUDED.unknown > 0 THEN
+          #{Statistic.letter_grades[:d]}
+        WHEN TARGET.medium + EXCLUDED.medium > 0 THEN
+          #{Statistic.letter_grades[:c]}
+        WHEN TARGET.low + EXCLUDED.low > 0 THEN
+          #{Statistic.letter_grades[:b]}
+        ELSE
+          #{Statistic.letter_grades[:a]}
+        END
+      SQL
+
+      UPSERT_SQL = <<~SQL
+        INSERT INTO #{Statistic.table_name} AS target (project_id, %{insert_attributes}, letter_grade, created_at, updated_at)
+          VALUES (%{project_id}, %{insert_values}, %{letter_grade}, now(), now())
+        ON CONFLICT (project_id)
+          DO UPDATE SET
+            %{update_values}, letter_grade = (#{LETTER_GRADE_SQL}), updated_at = now()
+      SQL
+
+      def self.update_for(vulnerability)
+        new(vulnerability).execute
+      end
+
+      def initialize(vulnerability)
+        self.vulnerability = vulnerability
+      end
+
+      def execute
+        return unless stat_diff.update_required?
+
+        connection.execute(upsert_sql)
+      end
+
+      private
+
+      attr_accessor :vulnerability
+
+      delegate :connection, to: Statistic, private: true
+      delegate :quote, :quote_column_name, to: :connection, private: true
+
+      def stat_diff
+        @stat_diff ||= vulnerability.stat_diff
+      end
+
+      def upsert_sql
+        format(
+          UPSERT_SQL,
+          project_id: stat_diff.project_id,
+          insert_attributes: insert_attributes,
+          insert_values: insert_values,
+          letter_grade: letter_grade,
+          update_values: update_values
+        )
+      end
+
+      def insert_attributes
+        stat_diff.changed_attributes.map { |attribute| quote_column_name(attribute) }.join(', ')
+      end
+
+      def insert_values
+        stat_diff.changed_values.map { |value| quote(value) }.join(', ')
+      end
+
+      def letter_grade
+        quote(Statistic.letter_grade_for(stat_diff.changes))
+      end
+
+      def update_values
+        stat_diff.changes.map do |attribute, value|
+          column_name = quote_column_name(attribute)
+          quoted_value = quote(value)
+
+          "#{column_name} = GREATEST(TARGET.#{column_name} + #{quoted_value}, 0)"
+        end.join(', ')
+      end
+    end
+  end
+end

ee/app/services/vulnerabilities/update_service.rb

@@ -18,6 +18,7 @@ module Vulnerabilities
       raise Gitlab::Access::AccessDeniedError unless can?(author, :create_vulnerability, project)
 
       vulnerability.update!(vulnerability_params)
+      Statistics::UpdateService.update_for(vulnerability)
 
       vulnerability
     end

ee/spec/models/vulnerabilities/stat_diff_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::StatDiff do
+  using RSpec::Parameterized::TableSyntax
+
+  let!(:vulnerability) { create(:vulnerability, :detected, severity: :high, title: 'Title') }
+  let(:stat_diff) { described_class.new(vulnerability) }
+  let(:update_vulnerability) {}
+
+  describe '#update_required?' do
+    subject(:update_required?) { update_vulnerability.then { stat_diff.update_required? } }
+
+    context 'when the vulnerability is destroyed' do
+      let(:update_vulnerability) { vulnerability.destroy! }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when the vulnerability is not destroyed' do
+      context 'when the severity is changed' do
+        let(:update_vulnerability) { vulnerability.update_attribute(:severity, :critical) }
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when the severity is not changed' do
+        context 'when the state is changed' do
+          where(:from, :to, :is_update_required) do
+            'confirmed' | 'detected'  | false
+            'confirmed' | 'resolved'  | true
+            'confirmed' | 'dismissed' | true
+
+            'detected'  | 'confirmed' | false
+            'detected'  | 'resolved'  | true
+            'detected'  | 'dismissed' | true
+
+            'resolved'  | 'dismissed' | false
+            'resolved'  | 'confirmed' | true
+            'resolved'  | 'detected'  | true
+
+            'dismissed' | 'resolved'  | false
+            'dismissed' | 'confirmed' | true
+            'dismissed' | 'detected'  | true
+          end
+
+          with_them do
+            let(:update_vulnerability) { vulnerability.update_attribute(:state, to) }
+
+            before do
+              vulnerability.update_attribute(:state, from)
+            end
+
+            it { is_expected.to eq(is_update_required) }
+          end
+        end
+
+        context 'when the state is not changed' do
+          let(:update_vulnerability) { vulnerability.update_attribute(:title, 'New Title') }
+
+          it { is_expected.to be_falsey }
+        end
+      end
+    end
+  end
+
+  describe '#changes' do
+    subject(:changes) { update_vulnerability.then { stat_diff.changes } }
+
+    context 'when the vulnerability is destroyed' do
+      let(:update_vulnerability) { vulnerability.destroy! }
+      let(:expected_changes) { { 'total' => -1, 'high' => -1 } }
+
+      it { is_expected.to eq(expected_changes) }
+    end
+
+    context 'when the vulnerability is not destroyed' do
+      context 'when the severity is changed' do
+        context 'when the state is not changed' do
+          let(:update_vulnerability) { vulnerability.update_attribute(:severity, :critical) }
+          let(:expected_changes) { { 'total' => 0, 'high' => -1, 'critical' => 1 } }
+
+          it { is_expected.to eq(expected_changes) }
+        end
+
+        context 'when the state is changed' do
+          where(:from, :to, :expected_changes) do
+            'confirmed' | 'detected'  | { 'total' => 0, 'high' => -1, 'critical' => 1 }
+            'confirmed' | 'resolved'  | { 'total' => -1, 'high' => -1 }
+            'confirmed' | 'dismissed' | { 'total' => -1, 'high' => -1 }
+
+            'detected'  | 'confirmed' | { 'total' => 0, 'high' => -1, 'critical' => 1 }
+            'detected'  | 'resolved'  | { 'total' => -1, 'high' => -1 }
+            'detected'  | 'dismissed' | { 'total' => -1, 'high' => -1 }
+
+            'resolved'  | 'dismissed' | { 'total' => 0 }
+            'resolved'  | 'confirmed' | { 'total' => 1, 'critical' => 1 }
+            'resolved'  | 'detected'  | { 'total' => 1, 'critical' => 1 }
+
+            'dismissed' | 'resolved'  | { 'total' => 0 }
+            'dismissed' | 'confirmed' | { 'total' => 1, 'critical' => 1 }
+            'dismissed' | 'detected'  | { 'total' => 1, 'critical' => 1 }
+          end
+
+          with_them do
+            let(:update_vulnerability) { vulnerability.update!(state: to, severity: :critical) }
+
+            before do
+              vulnerability.update_attribute(:state, from)
+            end
+
+            it { is_expected.to eq(expected_changes) }
+          end
+        end
+      end
+
+      context 'when the severity is not changed' do
+        context 'when the state is changed' do
+          where(:from, :to, :expected_changes) do
+            'confirmed' | 'detected'  | { 'total' => 0 }
+            'confirmed' | 'resolved'  | { 'total' => -1, 'high' => -1 }
+            'confirmed' | 'dismissed' | { 'total' => -1, 'high' => -1 }
+
+            'detected'  | 'confirmed' | { 'total' => 0 }
+            'detected'  | 'resolved'  | { 'total' => -1, 'high' => -1 }
+            'detected'  | 'dismissed' | { 'total' => -1, 'high' => -1 }
+
+            'resolved'  | 'dismissed' | { 'total' => 0 }
+            'resolved'  | 'confirmed' | { 'total' => 1, 'high' => 1 }
+            'resolved'  | 'detected'  | { 'total' => 1, 'high' => 1 }
+
+            'dismissed' | 'resolved'  | { 'total' => 0 }
+            'dismissed' | 'confirmed' | { 'total' => 1, 'high' => 1 }
+            'dismissed' | 'detected'  | { 'total' => 1, 'high' => 1 }
+          end
+
+          with_them do
+            let(:update_vulnerability) { vulnerability.update_attribute(:state, to) }
+
+            before do
+              vulnerability.update_attribute(:state, from)
+            end
+
+            it { is_expected.to eq(expected_changes) }
+          end
+        end
+
+        context 'when the state is not changed' do
+          let(:update_vulnerability) { vulnerability.update_attribute(:title, 'New Title') }
+          let(:expected_changes) { { 'total' => 0 } }
+
+          it { is_expected.to eq(expected_changes) }
+        end
+      end
+    end
+  end
+
+  describe '#changed_attributes' do
+    subject { stat_diff.changed_attributes }
+
+    context 'when there are changes' do
+      let(:expected_attribute_names) { %w(total high) }
+
+      before do
+        vulnerability.destroy!
+      end
+
+      it { is_expected.to eq(expected_attribute_names) }
+    end
+
+    context 'when there is no change' do
+      let(:expected_attribute_names) { [] }
+
+      before do
+        vulnerability.reload
+      end
+
+      it { is_expected.to eq(expected_attribute_names) }
+    end
+  end
+
+  describe '#changed_values' do
+    subject { stat_diff.changed_values }
+
+    context 'when there are changes' do
+      let(:expected_values) { [-1, -1] }
+
+      before do
+        vulnerability.destroy!
+      end
+
+      it { is_expected.to eq(expected_values) }
+    end
+
+    context 'when there is no change' do
+      let(:expected_values) { [] }
+
+      before do
+        vulnerability.reload
+      end
+
+      it { is_expected.to eq(expected_values) }
+    end
+  end
+end

ee/spec/models/vulnerabilities/statistic_spec.rb

@@ -17,4 +17,32 @@ RSpec.describe Vulnerabilities::Statistic do
     it { is_expected.to validate_numericality_of(:info).is_greater_than_or_equal_to(0) }
     it { is_expected.to define_enum_for(:letter_grade).with_values(%i(a b c d f)) }
   end
+
+  describe '.before_save' do
+    describe '#assign_letter_grade' do
+      let(:statistic) { build(:vulnerability_statistic, letter_grade: nil, critical: 5) }
+
+      subject(:save_statistic) { statistic.save! }
+
+      it 'assigns the letter_grade' do
+        expect { save_statistic }.to change { statistic.letter_grade }.from(nil).to('f')
+      end
+    end
+  end
+
+  describe '.letter_grade_for' do
+    subject { described_class.letter_grade_for(object) }
+
+    context 'when the given object is an instance of Vulnerabilities::Statistic' do
+      let(:object) { build(:vulnerability_statistic, critical: 1) }
+
+      it { is_expected.to eq(4) }
+    end
+
+    context 'when the given object is a Hash' do
+      let(:object) { { 'high' => 1 } }
+
+      it { is_expected.to eq(3) }
+    end
+  end
 end

ee/spec/models/vulnerability_spec.rb

@@ -424,4 +424,12 @@ RSpec.describe Vulnerability do
 
     it { is_expected.to eq(after_note_changed_method) }
   end
+
+  describe '#stat_diff' do
+    let(:vulnerability) { build(:vulnerability) }
+
+    subject { vulnerability.stat_diff }
+
+    it { is_expected.to be_an_instance_of(Vulnerabilities::StatDiff) }
+  end
 end

ee/spec/services/vulnerabilities/confirm_service_spec.rb

@@ -21,6 +21,8 @@ RSpec.describe Vulnerabilities::ConfirmService do
       project.add_developer(user)
     end
 
+    it_behaves_like 'calls Vulnerabilities::Statistics::UpdateService'
+
     it 'confirms a vulnerability' do
       Timecop.freeze do
         confirm_vulnerability

ee/spec/services/vulnerabilities/create_service_spec.rb

@@ -20,6 +20,8 @@ RSpec.describe Vulnerabilities::CreateService do
       project.add_developer(user)
     end
 
+    it_behaves_like 'calls Vulnerabilities::Statistics::UpdateService'
+
     it 'creates a vulnerability from finding and attaches it to the vulnerability' do
       expect { subject }.to change { project.vulnerabilities.count }.by(1)
       expect(project.vulnerabilities.last).to(

ee/spec/services/vulnerabilities/dismiss_service_spec.rb

@@ -21,6 +21,8 @@ RSpec.describe Vulnerabilities::DismissService do
       project.add_developer(user)
     end
 
+    it_behaves_like 'calls Vulnerabilities::Statistics::UpdateService'
+
     it 'dismisses a vulnerability and its associated findings' do
       Timecop.freeze do
         dismiss_vulnerability

ee/spec/services/vulnerabilities/resolve_service_spec.rb

@@ -21,6 +21,8 @@ RSpec.describe Vulnerabilities::ResolveService do
       project.add_developer(user)
     end
 
+    it_behaves_like 'calls Vulnerabilities::Statistics::UpdateService'
+
     it 'resolves a vulnerability' do
       Timecop.freeze do
         resolve_vulnerability

ee/spec/services/vulnerabilities/statistics/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Vulnerabilities::Statistics::UpdateService do
+  describe '.update_for' do
+    let(:mock_service_object) { instance_double(described_class, execute: true) }
+    let(:vulnerability) { instance_double(Vulnerability) }
+
+    subject(:update_stats) { described_class.update_for(vulnerability) }
+
+    before do
+      allow(described_class).to receive(:new).with(vulnerability).and_return(mock_service_object)
+    end
+
+    it 'instantiates an instance of service class and calls execute on it' do
+      update_stats
+
+      expect(mock_service_object).to have_received(:execute)
+    end
+  end
+
+  describe '#execute' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:statistic) { create(:vulnerability_statistic, project: project) }
+
+    let(:vulnerability) { create(:vulnerability, severity: :high, project: project) }
+    let(:stat_diff) { Vulnerabilities::StatDiff.new(vulnerability) }
+
+    subject(:update_stats) { described_class.new(vulnerability).execute }
+
+    context 'when the diff is empty' do
+      it 'does not change existing statistic entity' do
+        expect { update_stats }.not_to change { statistic.reload }
+      end
+    end
+
+    context 'when the diff is not empty' do
+      before do
+        vulnerability.update_attribute(:severity, :critical)
+      end
+
+      context 'when there is already a record in the database' do
+        it 'changes the existing statistic entity' do
+          expect { update_stats }.to change { statistic.reload.critical }.by(1)
+                                 .and not_change { statistic.reload.high }
+        end
+      end
+
+      context 'when there is no existing record in the database' do
+        before do
+          statistic.destroy!
+        end
+
+        it 'creates a new record in the database' do
+          expect { update_stats }.to change { Vulnerabilities::Statistic.count }.by(1)
+        end
+      end
+    end
+  end
+end

ee/spec/services/vulnerabilities/update_service_spec.rb

@@ -22,6 +22,8 @@ RSpec.describe Vulnerabilities::UpdateService do
       project.add_developer(user)
     end
 
+    it_behaves_like 'calls Vulnerabilities::Statistics::UpdateService'
+
     context 'when neither severity nor confidence are overridden' do
       it 'updates the vulnerability from updated finding (title, severity and confidence only)', :aggregate_failures do
         expect { subject }.not_to change { project.vulnerabilities.count }

ee/spec/support/shared_examples/services/vulnerabilities/calls_vulnerabilities_statistics_update_service_shared_example.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'calls Vulnerabilities::Statistics::UpdateService' do
+  before do
+    allow(Vulnerabilities::Statistics::UpdateService).to receive(:update_for)
+  end
+
+  it 'calls the service class' do
+    subject
+
+    expect(Vulnerabilities::Statistics::UpdateService).to have_received(:update_for)
+  end
+end