summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorStan Hu <stanhu@gmail.com>2019-08-28 21:57:54 -0700
committerStan Hu <stanhu@gmail.com>2019-08-29 12:54:19 -0700
commitf6c7e38040492db018943e537e30a7dd10e46120 (patch)
treeaf6f64104403475d080c5a867e5dee715e4520d1
parentf7e3693435307b56e4da8d8584c6af01459e4813 (diff)
downloadgitlab-ce-f6c7e38040492db018943e537e30a7dd10e46120.tar.gz
Make it harder to delete issuables accidentally
Previously submitting a DELETE request to an issuable URL would be enough to destroy it, but this should require human confirmation. We now require that the `destroy_confirm` parameter is set to a truthy value before this can complete. In addition, we log a Sentry error if a deletion arrived without confirmation. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/62387
-rw-r--r--app/assets/javascripts/issue_show/components/app.vue4
-rw-r--r--app/assets/javascripts/issue_show/components/edit_actions.vue2
-rw-r--r--app/assets/javascripts/issue_show/services/index.js4
-rw-r--r--app/controllers/concerns/issuable_actions.rb28
-rw-r--r--app/views/shared/issuable/_form.html.haml2
-rw-r--r--changelogs/unreleased/sh-add-delete-confirmation.yml5
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb29
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb22
-rw-r--r--spec/javascripts/issue_show/components/edit_actions_spec.js2
9 files changed, 87 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..05d865916af 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,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]
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..608131dcbc8 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/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
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 11b1eaf11b7..80f54dd258d 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 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
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 => {