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

Add endpoints to fetch notes/discussions for vulnerability

See merge request gitlab-org/gitlab!27585

ee/app/controllers/projects/security/vulnerabilities/notes_controller.rb

+# frozen_string_literal: true
+
+module Projects
+  module Security
+    module Vulnerabilities
+      class NotesController < Projects::ApplicationController
+        extend ::Gitlab::Utils::Override
+
+        include SecurityDashboardsPermissions
+        include NotesActions
+        include NotesHelper
+        include ToggleAwardEmoji
+
+        before_action :not_found, unless: -> { project.first_class_vulnerabilities_enabled? }
+        before_action :vulnerability
+
+        private
+
+        alias_method :vulnerable, :project
+
+        def note
+          @note ||= noteable.notes.find(params[:id])
+        end
+        alias_method :awardable, :note
+
+        def vulnerability
+          @vulnerability ||= @project.vulnerabilities.find(params[:vulnerability_id])
+
+          return render_404 unless can?(current_user, :read_vulnerability, @vulnerability)
+
+          @vulnerability
+        end
+        alias_method :noteable, :vulnerability
+
+        def finder_params
+          params.merge(last_fetched_at: last_fetched_at, target_id: vulnerability.id, target_type: 'vulnerability', project: @project)
+        end
+
+        override :note_serializer
+        def note_serializer
+          VulnerabilityNoteSerializer.new(project: project, noteable: noteable, current_user: current_user)
+        end
+
+        override :discussion_serializer
+        def discussion_serializer
+          DiscussionSerializer.new(project: project, noteable: noteable, current_user: current_user, note_entity: VulnerabilityNoteEntity)
+        end
+      end
+    end
+  end
+end

ee/app/controllers/projects/security/vulnerabilities_controller.rb

@@ -4,22 +4,31 @@ module Projects
   module Security
     class VulnerabilitiesController < Projects::ApplicationController
       include SecurityDashboardsPermissions
+      include IssuableActions
+      include RendersNotes
+
+      before_action :not_found, unless: -> { project.first_class_vulnerabilities_enabled? }
+      before_action :vulnerability, except: :index
 
       alias_method :vulnerable, :project
 
       def index
-        return render_404 unless Feature.enabled?(:first_class_vulnerabilities, project)
-
         @vulnerabilities = project.vulnerabilities.page(params[:page])
       end
 
       def show
-        return render_404 unless Feature.enabled?(:first_class_vulnerabilities, project)
-
-        @vulnerability = project.vulnerabilities.find(params[:id])
-        pipeline = @vulnerability.finding.pipelines.first
+        pipeline = vulnerability.finding.pipelines.first
         @pipeline = pipeline if Ability.allowed?(current_user, :read_pipeline, pipeline)
       end
+
+      private
+
+      def vulnerability
+        @issuable = @noteable = @vulnerability ||= vulnerable.vulnerabilities.find(params[:id])
+      end
+
+      alias_method :issuable, :vulnerability
+      alias_method :noteable, :vulnerability
     end
   end
 end

ee/app/finders/ee/notes_finder.rb

@@ -9,6 +9,8 @@ module EE
     def noteables_for_type(noteable_type)
       if noteable_type == 'epic'
         return EpicsFinder.new(@current_user, group_id: @params[:group_id]) # rubocop:disable Gitlab/ModuleWithInstanceVariables
+      elsif noteable_type == 'vulnerability'
+        return Security::VulnerabilitiesFinder.new(@project) # rubocop:disable Gitlab/ModuleWithInstanceVariables
       end
 
       super

ee/app/finders/security/vulnerabilities_finder.rb

@@ -15,6 +15,8 @@
 
 module Security
   class VulnerabilitiesFinder
+    include FinderMethods
+
     def initialize(vulnerable, filters = {})
       @filters = filters
       @vulnerabilities = vulnerable.vulnerabilities

ee/app/helpers/ee/notes_helper.rb

@@ -7,6 +7,7 @@ module EE
     override :notes_url
     def notes_url(params = {})
       return group_epic_notes_path(@epic.group, @epic) if @epic.is_a?(Epic)
+      return project_security_vulnerability_notes_path(@vulnerability.project, @vulnerability) if @vulnerability.is_a?(Vulnerability)
 
       super
     end
@@ -14,6 +15,7 @@ module EE
     override :discussions_path
     def discussions_path(issuable)
       return discussions_group_epic_path(issuable.group, issuable, format: :json) if issuable.is_a?(Epic)
+      return discussions_project_security_vulnerability_path(issuable.project, issuable, format: :json) if issuable.is_a?(Vulnerability)
 
       super
     end

ee/app/helpers/ee/system_note_helper.rb

@@ -22,7 +22,10 @@ module EE
       'designs_added' => 'doc-image',
       'designs_modified' => 'doc-image',
       'designs_removed' => 'doc-image',
-      'designs_discussion_added' => 'doc-image'
+      'designs_discussion_added' => 'doc-image',
+      'vulnerability_confirmed' => 'shield',
+      'vulnerability_dismissed' => 'cancel',
+      'vulnerability_resolved' => 'status_closed'
     }.freeze
 
     override :system_note_icon_name

ee/app/models/concerns/ee/noteable.rb

@@ -9,7 +9,7 @@ module EE
       # We can't specify `override` here:
       # https://gitlab.com/gitlab-org/gitlab-foss/issues/50911
       def replyable_types
-        super + %w(Epic)
+        super + %w[Epic Vulnerability]
       end
 
       def resolvable_types
@@ -22,6 +22,8 @@ module EE
       case self
       when Epic
         ::Gitlab::Routing.url_helpers.group_epic_notes_path(group, self)
+      when Vulnerability
+        ::Gitlab::Routing.url_helpers.project_security_vulnerability_notes_path(project, self)
       when DesignManagement::Design
         ::Gitlab::Routing.url_helpers.designs_project_issue_path(project, issue, { vueroute: filename })
       else

ee/app/models/ee/discussion_note.rb

@@ -9,7 +9,7 @@ module EE
 
       override :noteable_types
       def noteable_types
-        super + %w(Epic)
+        super + %w[Epic Vulnerability]
       end
     end
   end

ee/app/models/ee/note.rb

@@ -29,6 +29,10 @@ module EE
       noteable.is_a?(Epic)
     end
 
+    def for_vulnerability?
+      noteable.is_a?(Vulnerability)
+    end
+
     override :for_project_noteable?
     def for_project_noteable?
       !for_epic? && super

ee/app/models/ee/project.rb

@@ -292,6 +292,10 @@ module EE
       ::Feature.enabled?(:repository_push_audit_event, self)
     end
 
+    def first_class_vulnerabilities_enabled?
+      ::Feature.enabled?(:first_class_vulnerabilities, self)
+    end
+
     def feature_available?(feature, user = nil)
       if ::ProjectFeature::FEATURES.include?(feature)
         super

ee/app/models/ee/system_note_metadata.rb

@@ -9,6 +9,7 @@ module EE
       epic_issue_added issue_added_to_epic epic_issue_removed issue_removed_from_epic
       epic_issue_moved issue_changed_epic epic_date_changed relate_epic unrelate_epic
       designs_added designs_modified designs_removed designs_discussion_added
+      vulnerability_confirmed vulnerability_dismissed vulnerability_resolved
     ].freeze
 
     EE_TYPES_WITH_CROSS_REFERENCES = %w[

ee/app/models/vulnerability.rb

@@ -70,8 +70,12 @@ class Vulnerability < ApplicationRecord
 
   delegate :scanner_name, :metadata, to: :finding, prefix: true, allow_nil: true
 
-  def self.parent_class
-    ::Project
+  def resource_parent
+    project
+  end
+
+  def discussions_rendered_on_frontend?
+    true
   end
 
   def resolved_on_default_branch
@@ -81,4 +85,12 @@ class Vulnerability < ApplicationRecord
     latest_pipeline_with_vulnerability = finding.pipelines.order(created_at: :desc).first
     latest_pipeline_with_vulnerability != latest_successful_pipeline_for_default_branch
   end
+
+  def self.parent_class
+    ::Project
+  end
+
+  def self.to_ability_name
+    model_name.singular
+  end
 end

ee/app/serializers/vulnerability_note_entity.rb

+# frozen_string_literal: true
+
+class VulnerabilityNoteEntity < NoteEntity
+  expose :toggle_award_path, if: -> (note, _) { note.emoji_awardable? } do |note|
+    toggle_award_emoji_project_security_vulnerability_note_path(note.noteable.project, note.noteable, note)
+  end
+end

ee/app/serializers/vulnerability_note_serializer.rb

+# frozen_string_literal: true
+
+class VulnerabilityNoteSerializer < BaseSerializer
+  entity VulnerabilityNoteEntity
+end

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

@@ -6,9 +6,8 @@ module EE
       # 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))
+        create_note(NoteSummary.new(noteable, project, author, body, action: "vulnerability_#{noteable.state}"))
       end
     end
   end

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

+---
+title: Add endpoints to fetch notes/discussions for vulnerability
+merge_request: 27585
+author:
+type: added

ee/config/routes/project.rb

@@ -95,7 +95,15 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
             end
           end
 
-          resources :vulnerabilities, only: [:show, :index]
+          resources :vulnerabilities, only: [:show, :index] do
+            member do
+              get :discussions, format: :json
+            end
+
+            scope module: :vulnerabilities do
+              resources :notes, only: [:index], concerns: :awardable, constraints: { id: /\d+/ }
+            end
+          end
         end
 
         namespace :analytics do

ee/lib/ee/api/helpers/notes_helpers.rb

@@ -11,7 +11,7 @@ module EE
 
           override :noteable_types
           def noteable_types
-            [::Epic, *super]
+            [::Epic, ::Vulnerability, *super]
           end
         end
 

ee/lib/ee/gitlab/url_builder.rb

@@ -12,6 +12,8 @@ module EE
           design_url
         when Epic
           group_epic_url(object.group, object)
+        when Vulnerability
+          project_security_vulnerability_url(object.project, object)
         else
           super
         end
@@ -19,10 +21,15 @@ module EE
 
       override :note_url
       def note_url
-        return super unless object.for_epic?
+        noteable = object.noteable
 
-        epic = object.noteable
-        group_epic_url(epic.group, epic, anchor: dom_id(object))
+        if object.for_epic?
+          group_epic_url(noteable.group, noteable, anchor: dom_id(object))
+        elsif object.for_vulnerability?
+          project_security_vulnerability_url(noteable.project, noteable, anchor: dom_id(object))
+        else
+          super
+        end
       end
 
       private

ee/spec/controllers/projects/security/vulnerabilities/notes_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::Security::Vulnerabilities::NotesController do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:vulnerability) { create(:vulnerability, project: project) }
+  let_it_be(:note) { create(:note, noteable: vulnerability, project: project) }
+
+  it_behaves_like SecurityDashboardsPermissions do
+    let(:vulnerable) { project }
+
+    let(:security_dashboard_action) do
+      get :index, params: { namespace_id: project.namespace, project_id: project, vulnerability_id: vulnerability }
+    end
+  end
+
+  before do
+    stub_licensed_features(security_dashboard: true)
+  end
+
+  describe 'GET index' do
+    subject(:view_all_notes) do
+      get :index, params: { namespace_id: project.namespace, project_id: project, vulnerability_id: vulnerability }
+    end
+
+    before do
+      project.add_developer(user)
+      sign_in(user)
+    end
+
+    it 'responds with array of notes' do
+      view_all_notes
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(response).to match_response_schema('vulnerability_notes', dir: 'ee')
+
+      expect(json_response['notes']).to be_an Array
+      expect(json_response['notes'].pluck('id')).to eq([note.id.to_s])
+    end
+
+    context 'when the feature flag is disabled' do
+      before do
+        stub_feature_flags(first_class_vulnerabilities: false)
+      end
+
+      it 'renders the 404 page' do
+        view_all_notes
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
+end

ee/spec/controllers/projects/security/vulnerabilities_controller_spec.rb

@@ -108,4 +108,37 @@ describe Projects::Security::VulnerabilitiesController do
       end
     end
   end
+
+  describe 'GET #discussions' do
+    let_it_be(:vulnerability) { create(:vulnerability, project: project, author: user) }
+    let_it_be(:discussion_note) { create(:discussion_note_on_vulnerability, noteable: vulnerability, project: vulnerability.project) }
+
+    render_views
+
+    def show_vulnerability_discussion_list
+      sign_in(user)
+      get :discussions, params: { namespace_id: project.namespace, project_id: project, id: vulnerability }
+    end
+
+    it 'renders discussions' do
+      show_vulnerability_discussion_list
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(response).to match_response_schema('entities/discussions')
+
+      expect(json_response.pluck('id')).to eq([discussion_note.discussion_id])
+    end
+
+    context 'when the feature flag is disabled' do
+      before do
+        stub_feature_flags(first_class_vulnerabilities: false)
+      end
+
+      it 'renders the 404 page' do
+        show_vulnerability_discussion_list
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+  end
 end

ee/spec/factories/notes.rb

@@ -7,6 +7,10 @@ FactoryBot.modify do
       project { nil }
     end
 
+    trait :on_vulnerability do
+      noteable { create(:vulnerability) }
+    end
+
     trait :on_design do
       transient do
         issue { create(:issue, project: project) }
@@ -29,6 +33,9 @@ end
 
 FactoryBot.define do
   factory :note_on_epic, parent: :note, traits: [:on_epic]
+  factory :note_on_vulnerability, parent: :note, traits: [:on_vulnerability]
+
+  factory :discussion_note_on_vulnerability, parent: :note, traits: [:on_vulnerability], class: 'DiscussionNote'
 
   factory :diff_note_on_design, parent: :note, traits: [:on_design], class: 'DiffNote' do
     position { build(:image_diff_position, file: noteable.full_path, diff_refs: noteable.diff_refs) }

ee/spec/fixtures/api/schemas/vulnerability_notes.json

+{
+  "type": "object",
+  "required": [
+    "notes",
+    "last_fetched_at"
+  ],
+  "properties": {
+    "notes": {
+      "type": "array",
+      "items": {
+        "type": "object",
+        "required": [
+          "id",
+          "type",
+          "attachment",
+          "author",
+          "created_at",
+          "updated_at",
+          "system",
+          "noteable_id",
+          "noteable_type",
+          "resolvable",
+          "noteable_iid",
+          "note",
+          "note_html",
+          "current_user",
+          "suggestions",
+          "resolved",
+          "resolved_by",
+          "discussion_id",
+          "emoji_awardable",
+          "award_emoji",
+          "report_abuse_path",
+          "noteable_note_url",
+          "cached_markdown_version",
+          "toggle_award_path"
+        ],
+        "properties": {
+          "id": { "type": "string" },
+          "author": {
+            "$ref": "../../../../../spec/fixtures/api/schemas/entities/note_user_entity.json",
+            "required": [
+              "id",
+              "state",
+              "avatar_url",
+              "path",
+              "name",
+              "username"
+            ]
+          },
+          "created_at": { "type": "string" },
+          "updated_at": { "type": "string" },
+          "system": { "type": "boolean" },
+          "noteable_id": { "type": "integer" },
+          "noteable_type": { "type": "string" },
+          "resolvable": { "type": "boolean" },
+          "note": { "type": "string" },
+          "note_html": { "type": "string" },
+          "current_user": {
+            "type": "object",
+            "required": [
+              "can_edit",
+              "can_award_emoji",
+              "can_resolve"
+            ],
+            "properties": {
+              "can_edit": { "type": "boolean" },
+              "can_award_emoji": { "type": "boolean" },
+              "can_resolve": { "type": "boolean" }
+            }
+          },
+          "suggestions": { "type": "array" },
+          "resolved": { "type": "boolean" },
+          "discussion_id": { "type": "string" },
+          "emoji_awardable": { "type": "boolean" },
+          "award_emoji": { "type": "array" },
+          "report_abuse_path": { "type": "string" },
+          "noteable_note_url": { "type": "string" },
+          "cached_markdown_version": { "type": "integer" },
+          "toggle_award_path": { "type": "string" }
+        }
+      }
+    },
+    "last_fetched_at": { "type": "integer" }
+  },
+  "additionalProperties": false
+}

ee/spec/helpers/notes_helper_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe NotesHelper do
+  let_it_be(:vulnerability) { create(:vulnerability) }
+
+  describe '#notes_url' do
+    context 'for vulnerability' do
+      it 'return vulnerability notes path for vulnerability' do
+        @vulnerability = vulnerability
+
+        expect(notes_url).to eq("/#{@vulnerability.project.full_path}/-/security/vulnerabilities/#{@vulnerability.id}/notes")
+      end
+    end
+  end
+
+  describe '#discussions_path' do
+    subject { discussions_path(issueable) }
+
+    context 'for vulnerability' do
+      let(:issueable) { vulnerability }
+
+      it { is_expected.to eq("/#{vulnerability.project.full_path}/-/security/vulnerabilities/#{vulnerability.id}/discussions.json") }
+    end
+  end
+end

ee/spec/lib/ee/gitlab/url_builder_spec.rb

@@ -42,5 +42,25 @@ describe Gitlab::UrlBuilder do
         expect(url).to eq "#{Settings.gitlab['url']}/groups/#{epic.group.full_path}/-/epics/#{epic.iid}#note_#{note.id}"
       end
     end
+
+    context 'when passing a vulnerability' do
+      it 'returns a proper URL' do
+        vulnerability = build_stubbed(:vulnerability, id: 42)
+
+        url = described_class.build(vulnerability)
+
+        expect(url).to eq "#{Settings.gitlab['url']}/#{vulnerability.project.full_path}/-/security/vulnerabilities/#{vulnerability.id}"
+      end
+    end
+
+    context 'when passing a vulnerability note' do
+      it 'returns a proper URL' do
+        vulnerability = create(:vulnerability)
+        note = build_stubbed(:note_on_vulnerability, noteable: vulnerability)
+        url = described_class.build(note)
+
+        expect(url).to eq "#{Settings.gitlab['url']}/#{vulnerability.project.full_path}/-/security/vulnerabilities/#{vulnerability.id}#note_#{note.id}"
+      end
+    end
   end
 end

ee/spec/models/concerns/ee/noteable_spec.rb

@@ -9,6 +9,10 @@ describe EE::Noteable do
     it 'adds Epic to replyable_types after being included' do
       expect(klazz.replyable_types).to include("Epic")
     end
+
+    it 'adds Vulnerability to replyable_types after being included' do
+      expect(klazz.replyable_types).to include("Vulnerability")
+    end
   end
 
   describe '.resolvable_types' do

ee/spec/models/ee/system_note_metadata_spec.rb

@@ -3,7 +3,10 @@
 require 'spec_helper'
 
 describe EE::SystemNoteMetadata do
-  %i[designs_added designs_modified designs_removed].each do |action|
+  %i[
+    designs_added designs_modified designs_removed
+    vulnerability_confirmed vulnerability_dismissed vulnerability_resolved
+  ].each do |action|
     context 'when action type is valid' do
       subject do
         build(:system_note_metadata, note: build(:note), action: action )

ee/spec/models/vulnerability_spec.rb

@@ -222,4 +222,42 @@ describe Vulnerability do
       it { is_expected.to eq(true) }
     end
   end
+
+  describe '#resource_parent' do
+    let(:vulnerability) { build(:vulnerability) }
+
+    subject(:resource_parent) { vulnerability.resource_parent }
+
+    it { is_expected.to eq(vulnerability.project) }
+  end
+
+  describe '#discussions_rendered_on_frontend?' do
+    let(:vulnerability) { build(:vulnerability) }
+
+    subject(:discussions_rendered_on_frontend) { vulnerability.discussions_rendered_on_frontend? }
+
+    it { is_expected.to be true }
+  end
+
+  describe '.parent_class' do
+    subject(:parent_class) { Vulnerability.parent_class }
+
+    it { is_expected.to eq(::Project) }
+  end
+
+  describe '.to_ability_name' do
+    subject(:ability_name) { Vulnerability.to_ability_name }
+
+    it { is_expected.to eq('vulnerability') }
+  end
+
+  describe '#note_etag_key' do
+    let(:vulnerability) { create(:vulnerability) }
+
+    it 'returns a correct etag key' do
+      expect(vulnerability.note_etag_key).to eq(
+        ::Gitlab::Routing.url_helpers.project_security_vulnerability_notes_path(vulnerability.project, vulnerability)
+      )
+    end
+  end
 end

ee/spec/serializers/vulnerability_note_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe VulnerabilityNoteEntity do
+  include Gitlab::Routing
+
+  let(:request) { double('request', current_user: user, noteable: note.noteable) }
+
+  let(:entity) { described_class.new(note, request: request) }
+  let(:project) { create(:project) }
+  let(:vulnerability) { create(:vulnerability, project: project, author: user) }
+  let(:note) { create(:note, project: project, noteable: vulnerability, author: user) }
+  let(:user) { create(:user) }
+
+  subject { entity.as_json }
+
+  it_behaves_like 'note entity'
+
+  it 'exposes vulnerability-specific elements' do
+    expect(subject).to include(:toggle_award_path)
+  end
+end

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

@@ -6,45 +6,22 @@ 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' }
+    %w(dismissed resolved confirmed).each do |state|
+      context "state changed to #{state}" do
+        let(:noteable) { create(:vulnerability, project: project, state: state) }
 
         it_behaves_like 'a system note', exclude_project: true do
-        let(:action) { 'closed' }
+          let(:action) { "vulnerability_#{state}" }
         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' }
+          expect(subject.note).to eq("changed vulnerability status to #{state}")
         end
-
-      it 'creates the note text correctly' do
-        expect(subject.note).to eq("changed vulnerability status to confirmed")
       end
     end
   end

spec/fixtures/api/schemas/entities/discussion.json

@@ -36,7 +36,7 @@
           "updated_at": { "type": "date" },
           "system": { "type": "boolean" },
           "noteable_id": { "type": "integer" },
-          "noteable_iid": { "type": "integer" },
+          "noteable_iid": { "type": ["integer", "null"] },
           "noteable_type": { "type": "string" },
           "resolved": { "type": "boolean" },
           "resolvable": { "type": "boolean" },