diff options
9 files changed, 76 insertions, 11 deletions
diff --git a/app/assets/javascripts/issue_show/components/app.vue b/app/assets/javascripts/issue_show/components/app.vue index 9ca38d6bbfa..88975c2cc73 100644 --- a/app/assets/javascripts/issue_show/components/app.vue +++ b/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 diff --git a/app/assets/javascripts/issue_show/components/edit_actions.vue b/app/assets/javascripts/issue_show/components/edit_actions.vue index eb51a074f84..ce867f16acf 100644 --- a/app/assets/javascripts/issue_show/components/edit_actions.vue +++ b/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 }); } }, }, diff --git a/app/assets/javascripts/issue_show/services/index.js b/app/assets/javascripts/issue_show/services/index.js index 9546eb22c27..3c8334bee50 100644 --- a/app/assets/javascripts/issue_show/services/index.js +++ b/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) { diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index b86e4451a7e..8da33cddf5a 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/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,31 @@ module IssuableActions end end + def check_destroy_confirmation! + return true if params[:destroy_confirm] + + exception = RuntimeError.new("Destroy confirmation not provided for #{issuable.class.name}") + Gitlab::Sentry.track_acceptable_exception( + exception, + extra: { + project_path: issuable.project.full_path, + issuable_type: issuable.class.name, + issuable_id: issuable.id + } + ) + + error_message = 'Destroy confirmation not provided, aborting delete' + flash[:notice] = error_message + index_path = polymorphic_path([parent, issuable.class]) + + respond_to do |format| + format.html { redirect_to index_path } + 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] diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 214e87052da..04a70e406ca 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/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 diff --git a/changelogs/unreleased/sh-add-delete-confirmation.yml b/changelogs/unreleased/sh-add-delete-confirmation.yml new file mode 100644 index 00000000000..21c383183e7 --- /dev/null +++ b/changelogs/unreleased/sh-add-delete-confirmation.yml @@ -0,0 +1,5 @@ +--- +title: Make it harder to delete issuables accidentally +merge_request: 32376 +author: +type: fixed diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 187c7864ad7..32aeea2ea95 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -1084,16 +1084,32 @@ 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, aborting delete') + 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 diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 11b1eaf11b7..db8e0afef1e 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/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, aborting delete') + 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, aborting delete' }) + 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 diff --git a/spec/javascripts/issue_show/components/edit_actions_spec.js b/spec/javascripts/issue_show/components/edit_actions_spec.js index d92c54ea83f..2ab74ae4e10 100644 --- a/spec/javascripts/issue_show/components/edit_actions_spec.js +++ b/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 => { |