Require explicit confirmation before deleting a user (#34417).

Patch by Jens Krämer.

git-svn-id: http://svn.redmine.org/redmine/trunk@20600 e93f8b46-1217-0410-a6f0-8f06a7374b81

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 4368a9f1afede0702938ed68769950d04c8545de..5073812337433527b8371903dfe8a3aebccfdf77 100644 (file)
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -192,10 +192,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 (file)
index 0000000..6478519
--- /dev/null
+++ b/app/views/users/destroy.html.erb
@@ -0,0 +1,20 @@
+<%= title l(:label_confirmation) %>
+
+<%= form_tag user_path(@user), method: :delete do %>
+<div class="warning">
+  <p><strong><%= @user.name %> (<%= @user.login %>)</strong></p>
+
+  <p><%= l :text_user_destroy_confirmation, login: @user.login %></p>
+
+  <p>
+    <label for="confirm"><%= l :field_login %></label>
+    <%= text_field_tag 'confirm' %>
+  </p>
+</div>
+
+<p>
+  <%= submit_tag l(:button_delete) %>
+  <%= submit_tag l(:button_lock), name: 'lock' unless @user.locked? %>
+  <%= link_to l(:button_cancel), users_path %>
+</p>
+<% end %>

diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb
index b92df96f1f3150d6c162dd694b01b32d265d229d..7987d3b99a497d7c52a920e032a96e45ba94fe74 100644 (file)
--- a/app/views/users/index.html.erb
+++ b/app/views/users/index.html.erb
@@ -53,7 +53,7 @@
   <td class="last_login_on"><%= format_time(user.last_login_on) unless user.last_login_on.nil? %></td>
     <td class="buttons">
       <%= 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 %>
     </td>
   </tr>
 <% end -%>

diff --git a/config/locales/de.yml b/config/locales/de.yml
index 80484859d41158ccca590f3812d76cbca8f4ecfc..3b22f3fe1225191b55dab44577ee49f88eaed6bc 100644 (file)
--- 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 dce5bda76bbff3393be5bf0da75d6257c0463911..b5899df75b71b313d195f9d8995547542ab2e435 100644 (file)
--- 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 <a data-method="post" href="%{bc_path}">generate new backup codes</a> 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 459762c5002d477388300ea7da818e9e9cd54f1a..5883f344b195b34d2aa9db196d8484a32780af57 100644 (file)
--- 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(