Add ability to save notes for vulnerabilities when state is changed

This adds to First Class Vulnerabilities ability to store notes
and automatically creating notes when Vulnerability's state is
changed.

db/migrate/20200318140400_create_vulnerability_user_mentions.rb

+# frozen_string_literal: true
+
+class CreateVulnerabilityUserMentions < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    create_table :vulnerability_user_mentions do |t|
+      t.references :vulnerability, type: :bigint, index: false, null: false, foreign_key: { on_delete: :cascade }
+      t.references :note, type: :integer,
+                   index: { where: 'note_id IS NOT NULL', unique: true }, null: true, foreign_key: { on_delete: :cascade }
+      t.integer    :mentioned_users_ids, array: true
+      t.integer    :mentioned_projects_ids, array: true
+      t.integer    :mentioned_groups_ids, array: true
+    end
+
+    add_index :vulnerability_user_mentions, [:vulnerability_id], where: 'note_id is null', unique: true, name: 'index_vulns_user_mentions_on_vulnerability_id'
+    add_index :vulnerability_user_mentions, [:vulnerability_id, :note_id], unique: true, name: 'index_vulns_user_mentions_on_vulnerability_id_and_note_id'
+  end
+end

db/structure.sql

@@ -6611,6 +6611,24 @@ CREATE SEQUENCE public.vulnerability_scanners_id_seq
 
 ALTER SEQUENCE public.vulnerability_scanners_id_seq OWNED BY public.vulnerability_scanners.id;
 
+CREATE TABLE public.vulnerability_user_mentions (
+    id bigint NOT NULL,
+    vulnerability_id bigint NOT NULL,
+    note_id integer,
+    mentioned_users_ids integer[],
+    mentioned_projects_ids integer[],
+    mentioned_groups_ids integer[]
+);
+
+CREATE SEQUENCE public.vulnerability_user_mentions_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE public.vulnerability_user_mentions_id_seq OWNED BY public.vulnerability_user_mentions.id;
+
 CREATE TABLE public.web_hook_logs (
     id integer NOT NULL,
     web_hook_id integer NOT NULL,
@@ -7358,6 +7376,8 @@ ALTER TABLE ONLY public.vulnerability_occurrences ALTER COLUMN id SET DEFAULT ne
 
 ALTER TABLE ONLY public.vulnerability_scanners ALTER COLUMN id SET DEFAULT nextval('public.vulnerability_scanners_id_seq'::regclass);
 
+ALTER TABLE ONLY public.vulnerability_user_mentions ALTER COLUMN id SET DEFAULT nextval('public.vulnerability_user_mentions_id_seq'::regclass);
+
 ALTER TABLE ONLY public.web_hook_logs ALTER COLUMN id SET DEFAULT nextval('public.web_hook_logs_id_seq'::regclass);
 
 ALTER TABLE ONLY public.web_hooks ALTER COLUMN id SET DEFAULT nextval('public.web_hooks_id_seq'::regclass);
@@ -8286,6 +8306,9 @@ ALTER TABLE ONLY public.vulnerability_occurrences
 ALTER TABLE ONLY public.vulnerability_scanners
     ADD CONSTRAINT vulnerability_scanners_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY public.vulnerability_user_mentions
+    ADD CONSTRAINT vulnerability_user_mentions_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY public.web_hook_logs
     ADD CONSTRAINT web_hook_logs_pkey PRIMARY KEY (id);
 
@@ -10128,6 +10151,12 @@ CREATE INDEX index_vulnerability_occurrences_on_vulnerability_id ON public.vulne
 
 CREATE UNIQUE INDEX index_vulnerability_scanners_on_project_id_and_external_id ON public.vulnerability_scanners USING btree (project_id, external_id);
 
+CREATE UNIQUE INDEX index_vulnerability_user_mentions_on_note_id ON public.vulnerability_user_mentions USING btree (note_id) WHERE (note_id IS NOT NULL);
+
+CREATE UNIQUE INDEX index_vulns_user_mentions_on_vulnerability_id ON public.vulnerability_user_mentions USING btree (vulnerability_id) WHERE (note_id IS NULL);
+
+CREATE UNIQUE INDEX index_vulns_user_mentions_on_vulnerability_id_and_note_id ON public.vulnerability_user_mentions USING btree (vulnerability_id, note_id);
+
 CREATE INDEX index_web_hook_logs_on_created_at_and_web_hook_id ON public.web_hook_logs USING btree (created_at, web_hook_id);
 
 CREATE INDEX index_web_hook_logs_on_web_hook_id ON public.web_hook_logs USING btree (web_hook_id);
@@ -10843,6 +10872,9 @@ ALTER TABLE ONLY public.open_project_tracker_data
 ALTER TABLE ONLY public.gpg_signatures
     ADD CONSTRAINT fk_rails_19d4f1c6f9 FOREIGN KEY (gpg_key_subkey_id) REFERENCES public.gpg_key_subkeys(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY public.vulnerability_user_mentions
+    ADD CONSTRAINT fk_rails_1a41c485cd FOREIGN KEY (vulnerability_id) REFERENCES public.vulnerabilities(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.board_assignees
     ADD CONSTRAINT fk_rails_1c0ff59e82 FOREIGN KEY (assignee_id) REFERENCES public.users(id) ON DELETE CASCADE;
 
@@ -11380,6 +11412,9 @@ ALTER TABLE ONLY public.namespace_root_storage_statistics
 ALTER TABLE ONLY public.project_aliases
     ADD CONSTRAINT fk_rails_a1804f74a7 FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.vulnerability_user_mentions
+    ADD CONSTRAINT fk_rails_a18600f210 FOREIGN KEY (note_id) REFERENCES public.notes(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.todos
     ADD CONSTRAINT fk_rails_a27c483435 FOREIGN KEY (group_id) REFERENCES public.namespaces(id) ON DELETE CASCADE;
 
@@ -12742,6 +12777,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200316162648'),
 ('20200316173312'),
 ('20200317142110'),
+('20200318140400'),
 ('20200318152134'),
 ('20200318162148'),
 ('20200318163148'),

ee/app/models/vulnerability.rb

@@ -4,6 +4,8 @@ class Vulnerability < ApplicationRecord
   include CacheMarkdownField
   include Redactable
   include StripAttribute
+  include Noteable
+  include Awardable
 
   cache_markdown_field :title, pipeline: :single_line
   cache_markdown_field :description, issuable_state_filter_enabled: true
@@ -32,6 +34,9 @@ class Vulnerability < ApplicationRecord
     end
   end
 
+  has_many :notes, as: :noteable, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+  has_many :user_mentions, class_name: 'VulnerabilityUserMention'
+
   enum state: { detected: 1, dismissed: 2, resolved: 3, confirmed: 4 }
   enum severity: Vulnerabilities::Occurrence::SEVERITY_LEVELS, _prefix: :severity
   enum confidence: Vulnerabilities::Occurrence::CONFIDENCE_LEVELS, _prefix: :confidence
@@ -61,4 +66,8 @@ class Vulnerability < ApplicationRecord
   end
 
   delegate :scanner_name, :metadata, to: :finding, prefix: true, allow_nil: true
+
+  def self.parent_class
+    ::Project
+  end
 end

ee/app/models/vulnerability_user_mention.rb

+# frozen_string_literal: true
+
+class VulnerabilityUserMention < UserMention
+  belongs_to :vulnerability
+  belongs_to :note
+end

ee/app/services/ee/system_note_service.rb

@@ -159,5 +159,10 @@ module EE
     def abort_add_to_merge_train_when_pipeline_succeeds(noteable, project, author, reason)
       EE::SystemNotes::MergeTrainService.new(noteable: noteable, project: project, author: author).abort_add_when_pipeline_succeeds(reason)
     end
+
+    # Called when state is changed for 'vulnerability'
+    def change_vulnerability_state(noteable, author)
+      EE::SystemNotes::VulnerabilitiesService.new(noteable: noteable, project: noteable.project, author: author).change_vulnerability_state
+    end
   end
 end

ee/app/services/ee/system_notes/vulnerabilities_service.rb

+# frozen_string_literal: true
+
+module EE
+  module SystemNotes
+    class VulnerabilitiesService < ::SystemNotes::BaseService
+      # Called when state is changed for 'vulnerability'
+      def change_vulnerability_state
+        body = "changed vulnerability status to #{noteable.state}"
+        action = noteable.confirmed? ? 'opened' : 'closed'
+
+        create_note(NoteSummary.new(noteable, project, author, body, action: action))
+      end
+    end
+  end
+end

ee/app/services/vulnerabilities/base_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class BaseService
+    include Gitlab::Allowable
+
+    def initialize(user, vulnerability)
+      @user = user
+      @vulnerability = vulnerability
+      @project = vulnerability.project
+    end
+
+    private
+
+    def update_with_note(vulnerability, params)
+      return false unless vulnerability.update(params)
+
+      SystemNoteService.change_vulnerability_state(vulnerability, @user) if vulnerability.state_previously_changed?
+      true
+    end
+
+    def authorized?
+      can?(@user, :admin_vulnerability, @project)
+    end
+  end
+end

ee/app/services/vulnerabilities/confirm_service.rb

 # frozen_string_literal: true
 
 module Vulnerabilities
-  class ConfirmService
+  class ConfirmService < BaseService
     include Gitlab::Allowable
 
-    def initialize(user, vulnerability)
-      @user = user
-      @vulnerability = vulnerability
-    end
-
     def execute
-      raise Gitlab::Access::AccessDeniedError unless can?(@user, :admin_vulnerability, @vulnerability.project)
+      raise Gitlab::Access::AccessDeniedError unless authorized?
 
       @vulnerability.tap do |vulnerability|
-        vulnerability.update(state: Vulnerability.states[:confirmed], confirmed_by: @user, confirmed_at: Time.current)
+        update_with_note(vulnerability, state: Vulnerability.states[:confirmed], confirmed_by: @user, confirmed_at: Time.current)
       end
     end
   end

ee/app/services/vulnerabilities/dismiss_service.rb

 # frozen_string_literal: true
 
 module Vulnerabilities
-  class DismissService
+  class DismissService < BaseService
     include Gitlab::Allowable
 
     FindingsDismissResult = Struct.new(:ok?, :finding, :message)
 
-    def initialize(user, vulnerability)
-      @user = user
-      @vulnerability = vulnerability
-      @project = vulnerability.project
-    end
-
     def execute
-      raise Gitlab::Access::AccessDeniedError unless can?(@user, :admin_vulnerability, @project)
+      raise Gitlab::Access::AccessDeniedError unless authorized?
 
       @vulnerability.transaction do
         result = dismiss_findings
@@ -23,7 +17,7 @@ module Vulnerabilities
           raise ActiveRecord::Rollback
         end
 
-        @vulnerability.update(state: Vulnerability.states[:dismissed], dismissed_by: @user, dismissed_at: Time.current)
+        update_with_note(@vulnerability, state: Vulnerability.states[:dismissed], dismissed_by: @user, dismissed_at: Time.current)
       end
 
       @vulnerability

ee/app/services/vulnerabilities/resolve_service.rb

 # frozen_string_literal: true
 
 module Vulnerabilities
-  class ResolveService
-    include Gitlab::Allowable
-
-    def initialize(user, vulnerability)
-      @user = user
-      @vulnerability = vulnerability
-    end
-
+  class ResolveService < BaseService
     def execute
-      raise Gitlab::Access::AccessDeniedError unless can?(@user, :admin_vulnerability, @vulnerability.project)
+      raise Gitlab::Access::AccessDeniedError unless authorized?
 
       @vulnerability.tap do |vulnerability|
-        vulnerability.update(state: Vulnerability.states[:resolved], resolved_by: @user, resolved_at: Time.current)
+        update_with_note(vulnerability, state: Vulnerability.states[:resolved], resolved_by: @user, resolved_at: Time.current)
       end
     end
   end

ee/changelogs/unreleased/209990-add-notes-to-vulnerability.yml

+---
+title: Adds support for storing notes for vulnerabilities
+merge_request: 27515
+author:
+type: added

ee/spec/models/vulnerability_spec.rb

@@ -40,6 +40,8 @@ describe Vulnerability do
     it { is_expected.to belong_to(:confirmed_by).class_name('User') }
 
     it { is_expected.to have_many(:findings).class_name('Vulnerabilities::Occurrence').dependent(false) }
+    it { is_expected.to have_many(:notes).dependent(:delete_all) }
+    it { is_expected.to have_many(:user_mentions).class_name('VulnerabilityUserMention') }
   end
 
   describe 'validations' do

ee/spec/models/vulnerability_user_mention_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe VulnerabilityUserMention do
+  describe 'associations' do
+    it { is_expected.to belong_to(:vulnerability) }
+    it { is_expected.to belong_to(:note) }
+  end
+
+  it_behaves_like 'has user mentions'
+end

ee/spec/services/ee/system_notes/vulnerabilities_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe EE::SystemNotes::VulnerabilitiesService do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:author) { create(:user) }
+
+  let(:noteable) { create(:vulnerability, project: project, state: state) }
+  let(:service) { described_class.new(noteable: noteable, project: project, author: author) }
+
+  describe '#change_vulnerability_state' do
+    subject { service.change_vulnerability_state }
+
+    context 'state changed to dismissed' do
+      let(:state) { 'dismissed' }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'closed' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("changed vulnerability status to dismissed")
+      end
+    end
+
+    context 'state changed to resolved' do
+      let(:state) { 'resolved' }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'closed' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("changed vulnerability status to resolved")
+      end
+    end
+
+    context 'state changed to confirmed' do
+      let(:state) { 'confirmed' }
+
+      it_behaves_like 'a system note', exclude_project: true do
+        let(:action) { 'opened' }
+      end
+
+      it 'creates the note text correctly' do
+        expect(subject.note).to eq("changed vulnerability status to confirmed")
+      end
+    end
+  end
+end

ee/spec/services/system_note_service_spec.rb

@@ -220,4 +220,14 @@ describe SystemNoteService do
       described_class.abort_add_to_merge_train_when_pipeline_succeeds(noteable, project, author, message)
     end
   end
+
+  describe '.change_vulnerability_state' do
+    it 'calls VulnerabilitiesService' do
+      expect_next_instance_of(EE::SystemNotes::VulnerabilitiesService) do |service|
+        expect(service).to receive(:change_vulnerability_state)
+      end
+
+      described_class.change_vulnerability_state(noteable, author)
+    end
+  end
 end

ee/spec/services/vulnerabilities/confirm_service_spec.rb

@@ -30,6 +30,12 @@ describe Vulnerabilities::ConfirmService do
       end
     end
 
+    it 'creates note' do
+      expect(SystemNoteService).to receive(:change_vulnerability_state).with(vulnerability, user)
+
+      confirm_vulnerability
+    end
+
     context 'when security dashboard feature is disabled' do
       before do
         stub_licensed_features(security_dashboard: false)

ee/spec/services/vulnerabilities/dismiss_service_spec.rb

@@ -31,6 +31,12 @@ describe Vulnerabilities::DismissService do
       end
     end
 
+    it 'creates note' do
+      expect(SystemNoteService).to receive(:change_vulnerability_state).with(vulnerability, user)
+
+      dismiss_vulnerability
+    end
+
     context 'when there is a finding dismissal error' do
       before do
         allow(service).to receive(:dismiss_findings).and_return(

ee/spec/services/vulnerabilities/resolve_service_spec.rb

@@ -30,6 +30,12 @@ describe Vulnerabilities::ResolveService do
       end
     end
 
+    it 'creates note' do
+      expect(SystemNoteService).to receive(:change_vulnerability_state).with(vulnerability, user)
+
+      resolve_vulnerability
+    end
+
     context 'when security dashboard feature is disabled' do
       before do
         stub_licensed_features(security_dashboard: false)