From 8134e92457be1fb7d39ee8278b215d25803dbeda Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Fri, 3 Apr 2020 23:36:27 -0500
Subject: [PATCH] user deletions: fix error when given incorrect password.

Use validations instead of raising an exception when the password is
incorrect so that the controller can display errors sensibly.

Also fix users being logged out even when the deletion attempt failed
due to an incorrect password.
---
 .../maintenance/user/deletions_controller.rb    | 13 +++++++++++--
 app/logical/user_deletion.rb                    | 12 +++++++-----
 .../user/deletions_controller_test.rb           | 17 ++++++++++++++---
 test/unit/user_deletion_test.rb                 | 12 ++++--------
 4 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/app/controllers/maintenance/user/deletions_controller.rb b/app/controllers/maintenance/user/deletions_controller.rb
index a74d16946..627a65a73 100644
--- a/app/controllers/maintenance/user/deletions_controller.rb
+++ b/app/controllers/maintenance/user/deletions_controller.rb
@@ -1,14 +1,23 @@
 module Maintenance
   module User
     class DeletionsController < ApplicationController
+      respond_to :html, :json, :xml
+
       def show
       end
 
       def destroy
         deletion = UserDeletion.new(CurrentUser.user, params.dig(:user, :password))
         deletion.delete!
-        session.delete(:user_id)
-        redirect_to(posts_path, :notice => "You are now logged out")
+
+        if deletion.errors.none?
+          session.delete(:user_id)
+          flash[:notice] = "Your account has been deactivated"
+          respond_with(deletion, location: posts_path)
+        else
+          flash[:notice] = deletion.errors.full_messages.join("; ")
+          redirect_to maintenance_user_deletion_path
+        end
       end
     end
   end
diff --git a/app/logical/user_deletion.rb b/app/logical/user_deletion.rb
index 67bd6ee50..b87e6297f 100644
--- a/app/logical/user_deletion.rb
+++ b/app/logical/user_deletion.rb
@@ -1,7 +1,8 @@
 class UserDeletion
-  class ValidationError < StandardError; end
+  include ActiveModel::Validations
 
   attr_reader :user, :password
+  validate :validate_deletion
 
   def initialize(user, password)
     @user = user
@@ -9,13 +10,14 @@ class UserDeletion
   end
 
   def delete!
-    validate
+    return false if invalid?
     clear_user_settings
     remove_favorites
     clear_saved_searches
     rename
     reset_password
     create_mod_action
+    user
   end
 
   private
@@ -56,13 +58,13 @@ class UserDeletion
     request.save!(validate: false) # XXX don't validate so that the 1 name change per week rule doesn't interfere
   end
 
-  def validate
+  def validate_deletion
     if !user.authenticate_password(password)
-      raise ValidationError.new("Password is incorrect")
+      errors[:base] << "Password is incorrect"
     end
 
     if user.level >= User::Levels::ADMIN
-      raise ValidationError.new("Admins cannot delete their account")
+      errors[:base] << "Admins cannot delete their account"
     end
   end
 end
diff --git a/test/functional/maintenance/user/deletions_controller_test.rb b/test/functional/maintenance/user/deletions_controller_test.rb
index 6f2adbbf1..bf4ce204e 100644
--- a/test/functional/maintenance/user/deletions_controller_test.rb
+++ b/test/functional/maintenance/user/deletions_controller_test.rb
@@ -16,9 +16,20 @@ module Maintenance
         end
 
         context "#destroy" do
-          should "render" do
-            delete_auth maintenance_user_deletion_path, @user, params: {:password => "password"}
-            assert_redirected_to(posts_path)
+          should "delete the user when given the correct password" do
+            delete_auth maintenance_user_deletion_path, @user, params: { user: { password: "password" }}
+            assert_redirected_to posts_path
+            assert_equal(true, @user.reload.is_deleted?)
+            assert_equal("Your account has been deactivated", flash[:notice])
+            assert_nil(session[:user_id])
+          end
+
+          should "not delete the user when given an incorrect password" do
+            delete_auth maintenance_user_deletion_path, @user, params: { user: { password: "hunter2" }}
+            assert_redirected_to maintenance_user_deletion_path
+            assert_equal(false, @user.reload.is_deleted?)
+            assert_equal("Password is incorrect", flash[:notice])
+            assert_equal(@user.id, session[:user_id])
           end
         end
       end
diff --git a/test/unit/user_deletion_test.rb b/test/unit/user_deletion_test.rb
index 267eb2904..a1d8b15df 100644
--- a/test/unit/user_deletion_test.rb
+++ b/test/unit/user_deletion_test.rb
@@ -6,10 +6,8 @@ class UserDeletionTest < ActiveSupport::TestCase
       should "fail" do
         @user = create(:user)
         @deletion = UserDeletion.new(@user, "wrongpassword")
-
-        assert_raise(UserDeletion::ValidationError) do
-          @deletion.delete!
-        end
+        @deletion.delete!
+        assert_includes(@deletion.errors[:base], "Password is incorrect")
       end
     end
 
@@ -17,10 +15,8 @@ class UserDeletionTest < ActiveSupport::TestCase
       should "fail" do
         @user = create(:admin_user)
         @deletion = UserDeletion.new(@user, "password")
-
-        assert_raise(UserDeletion::ValidationError) do
-          @deletion.delete!
-        end
+        @deletion.delete!
+        assert_includes(@deletion.errors[:base], "Admins cannot delete their account")
       end
     end
   end