From 9ad44a5687362c5e525e6348c10d6a42b1fecadd Mon Sep 17 00:00:00 2001 From: Jens Kraemer Date: Wed, 18 Nov 2020 10:41:45 +0800 Subject: [PATCH 1/2] require explicit confirmation before deleting a user - removes the JS confirm - adds a confirmation page that requires entering the login of the user to be deleted and gives the option to lock the user instead --- app/controllers/users_controller.rb | 14 ++++++--- app/views/users/destroy.html.erb | 21 +++++++++++++ app/views/users/index.html.erb | 2 +- config/locales/de.yml | 2 ++ config/locales/en.yml | 2 ++ test/functional/users_controller_test.rb | 38 ++++++++++++++++++++---- 6 files changed, 68 insertions(+), 11 deletions(-) create mode 100644 app/views/users/destroy.html.erb diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 9ff242709..c0bc4f783 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -184,10 +184,16 @@ class UsersController < ApplicationController end def destroy - @user.destroy - respond_to do |format| - format.html {redirect_back_or_default(users_path)} - format.api {render_api_ok} + if api_request? || params[:lock] || params[:confirm] == @user.login + if params[:lock] + @user.update_attribute :status, User::STATUS_LOCKED + else + @user.destroy + end + respond_to do |format| + format.html {redirect_back_or_default(users_path)} + format.api {render_api_ok} + end end end diff --git a/app/views/users/destroy.html.erb b/app/views/users/destroy.html.erb new file mode 100644 index 000000000..1a30f4ebb --- /dev/null +++ b/app/views/users/destroy.html.erb @@ -0,0 +1,21 @@ +<%= title l(:label_confirmation) %> + +<%= form_tag user_path(@user), method: :delete do %> +
+

<%= @user.name %> (<%= @user.login %>)

+ +

<%= l :text_user_destroy_confirmation, login: @user.login %>

+ +

+ + <%= text_field_tag 'confirm' %> +

+
+ +

+ <%= submit_tag l(:button_delete) %> + <%= submit_tag l(:button_lock), name: 'lock' unless @user.locked? %> + <%= link_to l(:button_cancel), users_path %> +

+<% end %> + diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index b92df96f1..7987d3b99 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -53,7 +53,7 @@ <%= format_time(user.last_login_on) unless user.last_login_on.nil? %> <%= change_status_link(user) %> - <%= delete_link user_path(user, :back_url => request.original_fullpath) unless User.current == user %> + <%= delete_link user_path(user, :back_url => request.original_fullpath), :data => {} unless User.current == user %> <% end -%> diff --git a/config/locales/de.yml b/config/locales/de.yml index 80484859d..3b22f3fe1 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -1369,3 +1369,5 @@ de: error_invalid_size_parameter: Invalid size parameter error_attachment_not_found: Attachment %{name} not found field_twofa_scheme: Two-factor authentication scheme + + text_user_destroy_confirmation: "Wollen Sie diesen Benutzer inklusive aller Referenzen darauf wirklich löschen? Dies kann nicht rückgängig gemacht werden. Oftmals ist es besser, einen Benutzer lediglich zu sperren. Geben Sie bitte zur Bestätigung den Login des Benutzers (%{login}) ein." diff --git a/config/locales/en.yml b/config/locales/en.yml index dce5bda76..b5899df75 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1344,3 +1344,5 @@ en: twofa_text_backup_codes_hint: Use these codes instead of a one-time password should you not have access to your second factor. Each code can only be used once. It is recommended to print and store them in a safe place. twofa_text_backup_codes_created_at: Backup codes generated %{datetime}. twofa_backup_codes_already_shown: Backup codes cannot be shown again, please generate new backup codes if required. + + text_user_destroy_confirmation: "Are you sure you want to delete this user and remove all references to them? This cannot be undone. Often, locking a user instead of deleting them is the better solution. To confirm, please enter their login (%{login}) below." diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 459762c50..5883f344b 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -770,7 +770,7 @@ class UsersControllerTest < Redmine::ControllerTest # if user is already locked, destroying should not send a second mail # (for active admins see furtherbelow) ActionMailer::Base.deliveries.clear - delete :destroy, :params => {:id => 1} + delete :destroy, :params => {:id => 1, :confirm => User.find(1).login} assert_nil ActionMailer::Base.deliveries.last end @@ -834,17 +834,41 @@ class UsersControllerTest < Redmine::ControllerTest def test_destroy assert_difference 'User.count', -1 do - delete :destroy, :params => {:id => 2} + delete :destroy, :params => {:id => 2, :confirm => User.find(2).login} end assert_redirected_to '/users' assert_nil User.find_by_id(2) end + def test_destroy_with_lock_param_should_lock_instead + assert_no_difference 'User.count' do + delete :destroy, :params => {:id => 2, :lock => 'lock'} + end + assert_redirected_to '/users' + assert User.find_by_id(2).locked? + end + + def test_destroy_should_require_confirmation + assert_no_difference 'User.count' do + delete :destroy, :params => {:id => 2} + end + assert_response :success + assert_select '.warning', :text => /Are you sure you want to delete this user/ + end + + def test_destroy_should_require_correct_confirmation + assert_no_difference 'User.count' do + delete :destroy, :params => {:id => 2, :confirm => 'wrong'} + end + assert_response :success + assert_select '.warning', :text => /Are you sure you want to delete this user/ + end + def test_destroy_should_be_denied_for_non_admin_users @request.session[:user_id] = 3 assert_no_difference 'User.count' do - get :destroy, :params => {:id => 2} + delete :destroy, :params => {:id => 2, :confirm => User.find(2).login} end assert_response 403 end @@ -852,14 +876,16 @@ class UsersControllerTest < Redmine::ControllerTest def test_destroy_should_be_denied_for_anonymous assert User.find(6).anonymous? assert_no_difference 'User.count' do - put :destroy, :params => {:id => 6} + delete :destroy, :params => {:id => 6, :confirm => User.find(6).login} end assert_response 404 end def test_destroy_should_redirect_to_back_url_param assert_difference 'User.count', -1 do - delete :destroy, :params => {:id => 2, :back_url => '/users?name=foo'} + delete :destroy, :params => {:id => 2, + :confirm => User.find(2).login, + :back_url => '/users?name=foo'} end assert_redirected_to '/users?name=foo' end @@ -869,7 +895,7 @@ class UsersControllerTest < Redmine::ControllerTest user.admin = true user.save! ActionMailer::Base.deliveries.clear - delete :destroy, :params => {:id => user.id} + delete :destroy, :params => {:id => user.id, :confirm => user.login} assert_not_nil (mail = ActionMailer::Base.deliveries.last) assert_mail_body_match( -- 2.20.1