Merge branch 'sh-add-delete-confirmation-ee' into 'master'

Make it harder to delete issuables accidentally

Closes gitlab-ce#62387

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

app/assets/javascripts/issue_show/components/app.vue

@@ -300,9 +300,9 @@ export default {
       this.closeRecaptcha();
     },
 
-    deleteIssuable() {
+    deleteIssuable(payload) {
       this.service
-        .deleteIssuable()
+        .deleteIssuable(payload)
         .then(res => res.data)
         .then(data => {
           // Stop the poll so we don't get 404's with the issuable not existing

app/assets/javascripts/issue_show/components/edit_actions.vue

@@ -55,7 +55,7 @@ export default {
       if (window.confirm(confirmMessage)) {
         this.deleteLoading = true;
 
-        eventHub.$emit('delete.issuable');
+        eventHub.$emit('delete.issuable', { destroy_confirm: true });
       }
     },
   },

app/assets/javascripts/issue_show/services/index.js

@@ -10,8 +10,8 @@ export default class Service {
     return axios.get(this.realtimeEndpoint);
   }
 
-  deleteIssuable() {
-    return axios.delete(this.endpoint);
+  deleteIssuable(payload) {
+    return axios.delete(this.endpoint, { params: payload });
   }
 
   updateIssuable(data) {

app/controllers/concerns/issuable_actions.rb

@@ -6,6 +6,7 @@ module IssuableActions
 
   included do
     before_action :authorize_destroy_issuable!, only: :destroy
+    before_action :check_destroy_confirmation!, only: :destroy
     before_action :authorize_admin_issuable!, only: :bulk_update
     before_action only: :show do
       push_frontend_feature_flag(:scoped_labels, default_enabled: true)
@@ -91,6 +92,33 @@ module IssuableActions
     end
   end
 
+  def check_destroy_confirmation!
+    return true if params[:destroy_confirm]
+
+    error_message = "Destroy confirmation not provided for #{issuable.human_class_name}"
+    exception = RuntimeError.new(error_message)
+    Gitlab::Sentry.track_acceptable_exception(
+      exception,
+      extra: {
+        project_path: issuable.project.full_path,
+        issuable_type: issuable.class.name,
+        issuable_id: issuable.id
+      }
+    )
+
+    index_path = polymorphic_path([parent, issuable.class])
+
+    respond_to do |format|
+      format.html do
+        flash[:notice] = error_message
+        redirect_to index_path
+      end
+      format.json do
+        render json: { errors: error_message }, status: :unprocessable_entity
+      end
+    end
+  end
+
   def bulk_update
     result = Issuable::BulkUpdateService.new(current_user, bulk_update_params).execute(resource_name)
     quantity = result[:count]

app/views/shared/issuable/_form.html.haml

@@ -66,7 +66,7 @@
       = link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable.class]), class: 'btn btn-cancel'
     - else
       - if can?(current_user, :"destroy_#{issuable.to_ability_name}", @project)
-        = link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), data: { confirm: "#{issuable.human_class_name} will be removed! Are you sure?" }, method: :delete, class: 'btn btn-danger btn-grouped'
+        = link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable], params: { destroy_confirm: true }), data: { confirm: "#{issuable.human_class_name} will be removed! Are you sure?" }, method: :delete, class: 'btn btn-danger btn-grouped'
       = link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel'
 
   %span.append-right-10

changelogs/unreleased/sh-add-delete-confirmation.yml

+---
+title: Make it harder to delete issuables accidentally
+merge_request: 32376
+author:
+type: fixed

ee/spec/controllers/groups/epics_controller_spec.rb

@@ -461,14 +461,14 @@ describe Groups::EpicsController do
 
       it "rejects a developer to destroy an epic" do
         group.add_developer(user)
-        delete :destroy, params: { group_id: group, id: epic.to_param }
+        delete :destroy, params: { group_id: group, id: epic.to_param, destroy_confirm: true }
 
         expect(response).to have_gitlab_http_status(404)
       end
 
       it "deletes the epic" do
         group.add_owner(user)
-        delete :destroy, params: { group_id: group, id: epic.to_param }
+        delete :destroy, params: { group_id: group, id: epic.to_param, destroy_confirm: true }
 
         expect(response).to have_gitlab_http_status(302)
         expect(controller).to set_flash[:notice].to(/The epic was successfully deleted\./)

spec/controllers/projects/issues_controller_spec.rb

@@ -1084,16 +1084,41 @@ describe Projects::IssuesController do
       end
 
       it "deletes the issue" do
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
 
         expect(response).to have_gitlab_http_status(302)
         expect(controller).to set_flash[:notice].to(/The issue was successfully deleted\./)
       end
 
+      it "deletes the issue" do
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
+
+        expect(response).to have_gitlab_http_status(302)
+        expect(controller).to set_flash[:notice].to(/The issue was successfully deleted\./)
+      end
+
+      it "prevents deletion if destroy_confirm is not set" do
+        expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
+
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
+
+        expect(response).to have_gitlab_http_status(302)
+        expect(controller).to set_flash[:notice].to('Destroy confirmation not provided for issue')
+      end
+
+      it "prevents deletion in JSON format if destroy_confirm is not set" do
+        expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
+
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, format: 'json' }
+
+        expect(response).to have_gitlab_http_status(422)
+        expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for issue' })
+      end
+
       it 'delegates the update of the todos count cache to TodoService' do
         expect_any_instance_of(TodoService).to receive(:destroy_target).with(issue).once
 
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: issue.iid, destroy_confirm: true }
       end
     end
   end

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -573,16 +573,34 @@ describe Projects::MergeRequestsController do
       end
 
       it "deletes the merge request" do
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid }
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, destroy_confirm: true }
 
         expect(response).to have_gitlab_http_status(302)
         expect(controller).to set_flash[:notice].to(/The merge request was successfully deleted\./)
       end
 
+      it "prevents deletion if destroy_confirm is not set" do
+        expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
+
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid }
+
+        expect(response).to have_gitlab_http_status(302)
+        expect(controller).to set_flash[:notice].to('Destroy confirmation not provided for merge request')
+      end
+
+      it "prevents deletion in JSON format if destroy_confirm is not set" do
+        expect(Gitlab::Sentry).to receive(:track_acceptable_exception).and_call_original
+
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, format: 'json' }
+
+        expect(response).to have_gitlab_http_status(422)
+        expect(json_response).to eq({ 'errors' => 'Destroy confirmation not provided for merge request' })
+      end
+
       it 'delegates the update of the todos count cache to TodoService' do
         expect_any_instance_of(TodoService).to receive(:destroy_target).with(merge_request).once
 
-        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid }
+        delete :destroy, params: { namespace_id: project.namespace, project_id: project, id: merge_request.iid, destroy_confirm: true }
       end
     end
   end

spec/javascripts/issue_show/components/edit_actions_spec.js

@@ -104,7 +104,7 @@ describe('Edit Actions components', () => {
       spyOn(window, 'confirm').and.returnValue(true);
       vm.$el.querySelector('.btn-danger').click();
 
-      expect(eventHub.$emit).toHaveBeenCalledWith('delete.issuable');
+      expect(eventHub.$emit).toHaveBeenCalledWith('delete.issuable', { destroy_confirm: true });
     });
 
     it('shows loading icon after clicking delete button', done => {