Merge branch '209990-add-notes-to-vulnerability' into 'master'

Save notes for Vulnerabilities when state is changed

See merge request gitlab-org/gitlab!27515

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

@@ -6612,6 +6612,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,
@@ -7359,6 +7377,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);
@@ -8287,6 +8307,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);
 
@@ -10129,6 +10152,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);
@@ -10844,6 +10873,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;
 
@@ -11381,6 +11413,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;
 
@@ -12743,6 +12778,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)