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

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
 

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 %>

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 -%>

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."

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."

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(