Merge branch '10364-update-dismissal-comments' into 'master'

Support editing and deleting of dismissal comments

See merge request gitlab-org/gitlab-ee!11963

config/routes/project.rb

@@ -483,7 +483,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       # EE-specific end
 
       ## EE-specific
-      resources :vulnerability_feedback, only: [:index, :create, :destroy], constraints: { id: /\d+/ }
+      resources :vulnerability_feedback, only: [:index, :create, :update, :destroy], constraints: { id: /\d+/ }
 
       get :issues, to: 'issues#calendar', constraints: lambda { |req| req.format == :ics }
 

ee/app/controllers/projects/vulnerability_feedback_controller.rb

 # frozen_string_literal: true
 
 class Projects::VulnerabilityFeedbackController < Projects::ApplicationController
-  before_action :vulnerability_feedback, only: [:destroy]
+  before_action :vulnerability_feedback, only: [:update, :destroy]
   before_action :authorize_read_vulnerability_feedback!, only: [:index]
   before_action :authorize_create_vulnerability_feedback!, only: [:create]
+  before_action :authorize_update_vulnerability_feedback!, only: [:update]
   before_action :authorize_destroy_vulnerability_feedback!, only: [:destroy]
+
   skip_before_action :authenticate_user!, only: [:index], raise: false
 
   respond_to :json
@@ -39,6 +41,17 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
     end
   end
 
+  def update
+    service = VulnerabilityFeedbackModule::UpdateService.new(project, current_user, vulnerability_feedback_params )
+    result = service.execute(vulnerability_feedback)
+
+    if result[:status] == :success
+      render json: serializer.represent(result[:vulnerability_feedback])
+    else
+      render json: result[:message], status: :unprocessable_entity
+    end
+  end
+
   def destroy
     service = VulnerabilityFeedback::DestroyService.new(project, current_user, vulnerability_feedback)
     service.execute
@@ -61,6 +74,10 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
     render_403 unless can?(current_user, :destroy_vulnerability_feedback, vulnerability_feedback)
   end
 
+  def authorize_update_vulnerability_feedback!
+    render_403 unless can?(current_user, :update_vulnerability_feedback, vulnerability_feedback)
+  end
+
   def serializer
     Vulnerabilities::FeedbackSerializer.new(current_user: current_user, project: project)
   end

ee/app/models/vulnerabilities/feedback.rb

@@ -53,5 +53,11 @@ module Vulnerabilities
         raise ArgumentError.new("'#{feedback_params[:category]}' is not a valid category")
       end
     end
+
+    # A hard delete of the comment_author will cause the comment_author to be nil, but the comment
+    # will still exist.
+    def has_comment?
+      comment.present? && comment_author.present?
+    end
   end
 end

ee/app/policies/ee/project_policy.rb

@@ -113,6 +113,7 @@ module EE
         enable :admin_board
         enable :create_vulnerability_feedback
         enable :destroy_vulnerability_feedback
+        enable :update_vulnerability_feedback
         enable :create_package
         enable :read_feature_flag
         enable :create_feature_flag

ee/app/policies/vulnerabilities/feedback_policy.rb

@@ -15,6 +15,6 @@ module Vulnerabilities
         (~can?(:create_merge_request_in) | ~can?(:create_merge_request_from))
     end.prevent :create_vulnerability_feedback
 
-    rule { ~dismissal }.prevent :destroy_vulnerability_feedback
+    rule { ~dismissal }.prevent :destroy_vulnerability_feedback, :update_vulnerability_feedback
   end
 end

ee/app/serializers/vulnerabilities/feedback_entity.rb

@@ -7,7 +7,7 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
   expose :created_at
   expose :project_id
   expose :author, using: UserEntity
-  expose :comment_details, if: -> (feedback, _) { feedback.comment.present? } do
+  expose :comment_details, if: -> (feedback, _) { feedback.has_comment? } do
     expose :comment
     expose :comment_timestamp
     expose :comment_author, using: UserEntity

ee/app/services/vulnerability_feedback_module/update_service.rb

+# frozen_string_literal: true
+
+module VulnerabilityFeedbackModule
+  class UpdateService < ::BaseService
+    def execute(vulnerability_feedback)
+      raise Gitlab::Access::AccessDeniedError unless can?(current_user, :update_vulnerability_feedback, vulnerability_feedback)
+
+      if vulnerability_feedback.update(vulnerability_feedback_attributes)
+        success(vulnerability_feedback)
+      else
+        error(vulnerability_feedback.errors)
+      end
+
+    rescue ArgumentError => e
+      # VulnerabilityFeedback relies on #enum attributes which raise this exception
+      error(e.message)
+    end
+
+    private
+
+    def vulnerability_feedback_attributes
+      if params[:comment].present?
+        { comment: params[:comment], comment_timestamp: Time.zone.now, comment_author: current_user }
+      else
+        { comment: nil, comment_timestamp: nil, comment_author: nil }
+      end
+    end
+
+    def success(vulnerability_feedback)
+      super().merge(vulnerability_feedback: vulnerability_feedback)
+    end
+  end
+end

ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb

@@ -195,6 +195,75 @@ describe Projects::VulnerabilityFeedbackController do
     end
   end
 
+  describe 'PATCH #update' do
+    let(:vuln_feedback) do
+      create(:vulnerability_feedback, :dismissal, :sast, :comment,
+             project: project,
+             author: user
+      )
+    end
+
+    context 'with valid params' do
+      before do
+        vuln_feedback.comment = 'new dismissal comment'
+        update_feedback user: user, params: vuln_feedback
+      end
+
+      it 'returns the updated feedback' do
+        expect(response).to match_response_schema('vulnerability_feedback', dir: 'ee')
+      end
+
+      it 'returns a successful 200 response' do
+        expect(response).to have_gitlab_http_status(200)
+      end
+
+      it 'updates the comment attributes' do
+        expect(vuln_feedback.reload.comment).to eq('new dismissal comment')
+      end
+    end
+
+    context 'with invalid params' do
+      it 'returns a not found 404 response for invalid vulnerability feedback id' do
+        vuln_feedback.id = 123
+        update_feedback user: user, params: vuln_feedback
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    context 'with unauthorized user for feedback update' do
+      it 'returns a forbidden 403 response' do
+        group.add_guest(guest)
+
+        update_feedback user: guest, params: vuln_feedback
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
+    context 'with unauthorized user for given project' do
+      let(:unauthorized_user) { create(:user) }
+      let(:project) { create(:project, :private, namespace: group) }
+
+      it 'returns a 404 response' do
+        update_feedback user: unauthorized_user, params: vuln_feedback
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+
+    def update_feedback(user:, params:)
+      sign_in(user)
+
+      patch :update, params: {
+                       namespace_id: project.namespace.to_param,
+                       project_id: project,
+                       id: params[:id],
+                       vulnerability_feedback: params
+                     }, as: :json
+    end
+  end
+
   describe 'DELETE #destroy' do
     let(:pipeline) { create(:ci_pipeline, project: project) }
     let!(:vuln_feedback) { create(:vulnerability_feedback, :dismissal, :sast, project: project, author: user, pipeline: pipeline) }

ee/spec/models/vulnerabilities/feedback_spec.rb

@@ -35,6 +35,31 @@ describe Vulnerabilities::Feedback do
     end
   end
 
+  describe '#has_comment?' do
+    let(:feedback) { build(:vulnerability_feedback, comment: comment, comment_author: comment_author) }
+    let(:comment) { 'a comment' }
+    let(:comment_author) { build(:user) }
+
+    subject { feedback.has_comment? }
+
+    context 'comment and comment_author are set' do
+      it { is_expected.to be_truthy }
+    end
+
+    context 'comment is set and comment_author is not' do
+      let(:comment_author) { nil }
+
+      it { is_expected.to be_falsy }
+    end
+
+    context 'comment and comment_author are not set' do
+      let(:comment) { nil }
+      let(:comment_author) { nil }
+
+      it { is_expected.to be_falsy }
+    end
+  end
+
   describe '#find_or_init_for' do
     let(:group) { create(:group) }
     let(:project) { create(:project, :public, :repository, namespace: group) }

ee/spec/policies/project_policy_spec.rb

@@ -324,6 +324,7 @@ describe ProjectPolicy do
 
     where(permission: %i[
       create_vulnerability_feedback
+      update_vulnerability_feedback
       destroy_vulnerability_feedback
     ])
 

ee/spec/policies/vulnerabilities/feedback_policy_spec.rb

@@ -75,6 +75,32 @@ describe Vulnerabilities::FeedbackPolicy do
     end
   end
 
+  describe 'update_vulnerability_feedback' do
+    context 'when feedback type is issue' do
+      let(:vulnerability_feedback) { Vulnerabilities::Feedback.new(project: project, feedback_type: :issue) }
+
+      it 'does not allow to update issue feedback' do
+        is_expected.to be_disallowed(:update_vulnerability_feedback)
+      end
+    end
+
+    context 'when feedback type is merge_request' do
+      let(:vulnerability_feedback) { Vulnerabilities::Feedback.new(project: project, feedback_type: :merge_request) }
+
+      it 'does not allow to update merge request feedback' do
+        is_expected.to be_disallowed(:update_vulnerability_feedback)
+      end
+    end
+
+    context 'when feedback type is dismissal' do
+      let(:vulnerability_feedback) { Vulnerabilities::Feedback.new(project: project, feedback_type: :dismissal) }
+
+      it 'allows to update dismissal feedback' do
+        is_expected.to be_allowed(:update_vulnerability_feedback)
+      end
+    end
+  end
+
   describe 'destroy_vulnerability_feedback' do
     context 'when feedback type is issue' do
       let(:vulnerability_feedback) { Vulnerabilities::Feedback.new(project: project, feedback_type: :issue) }

ee/spec/services/ee/vulnerability_feedback_module/update_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe VulnerabilityFeedbackModule::UpdateService, '#execute' do
+  let(:group)   { create(:group) }
+  let(:project) { create(:project, :public, :repository, namespace: group) }
+  let(:user)    { create(:user) }
+  let(:pipeline) { create(:ci_pipeline, project: project) }
+  let(:vuln_feedback) { create(:vulnerability_feedback, :comment, project: project) }
+  let(:service) { described_class.new(project, user, comment: comment) }
+
+  before do
+    group.add_developer(user)
+  end
+
+  subject(:result) { service.execute(vuln_feedback) }
+
+  context 'when params are valid' do
+    context 'when new comment is passed' do
+      let(:comment) {'new dismissal comment' }
+
+      let(:vuln_feedback) do
+        create(:vulnerability_feedback, :dismissal, :sast,
+               project: project,
+               author: user,
+               pipeline: pipeline)
+      end
+
+      it 'adds the comments' do
+        expect(result[:status]).to eq(:success)
+
+        feedback = result[:vulnerability_feedback]
+
+        expect(feedback.comment).to eq('new dismissal comment')
+        expect(feedback.comment_author).to eq(user)
+        expect(feedback.comment_timestamp).not_to be_nil
+      end
+    end
+
+    context 'second user updates the comment' do
+      let(:comment) { 'updated dismissal comment' }
+      let(:second_user) { create(:user) }
+      let(:service) { described_class.new(project, second_user, vuln_feedback) }
+
+      before do
+        group.add_developer(second_user)
+      end
+
+      it 'sets second user as the comment author' do
+        feedback = result[:vulnerability_feedback]
+
+        expect(feedback.comment_author).to eq(second_user)
+      end
+    end
+
+    context 'when updated comment is passed' do
+      let(:comment) { 'updated dismissal comment' }
+
+      it 'updates the comments' do
+        feedback = result[:vulnerability_feedback]
+
+        expect(feedback.comment).to eq('updated dismissal comment')
+      end
+    end
+
+    context 'when deleting a comment' do
+      let(:comment) { '' }
+
+      it 'removes the comment data' do
+        expect(result[:status]).to eq(:success)
+
+        feedback = result[:vulnerability_feedback]
+
+        expect(feedback.comment).to be_nil
+        expect(feedback.comment_author).to be_nil
+        expect(feedback.comment_timestamp).to be_nil
+      end
+    end
+  end
+end