users: clean up password update logic.

Pull the password reauthentication logic out of the user model and put
it in the password update controller where it belongs.

This fixes an issue where when a new user was created the user model had
an incorrect password error set on it by `encrypt_password_on_update`.
It was trying to verify the old password even though we don't have one
when creating a new user. This error caused the user create action to
redirect back to the signup page because `respond_with` thought that
creating the user failed.

app/controllers/passwords_controller.rb

@@ -8,13 +8,17 @@ class PasswordsController < ApplicationController
 
   def update
     @user = authorize User.find(params[:user_id]), policy_class: PasswordPolicy
-    @user.update(user_params)
+
+    if User.authenticate(@user.name, params[:user][:old_password])
+      @user.update(password: params[:user][:password], password_confirmation: params[:user][:password_confirmation])
+    elsif @user.authenticate_login_key(params[:user][:signed_user_id])
+      @user.update(password: params[:user][:password], password_confirmation: params[:user][:password_confirmation])
+    else
+      @user.errors[:base] << "Incorrect password"
+    end
+
     flash[:notice] = @user.errors.none? ? "Password updated" : @user.errors.full_messages.join("; ")
 
     respond_with(@user, location: @user)
   end
-
-  def user_params
-    params.fetch(:user, {}).permit(policy(:password).permitted_attributes)
-  end
 end

app/logical/user_deletion.rb

@@ -41,11 +41,7 @@ class UserDeletion
   end
 
   def reset_password
-    random = SecureRandom.hex(16)
-    user.password = random
-    user.password_confirmation = random
-    user.old_password = password
-    user.save!
+    user.update!(password: SecureRandom.hex(16))
   end
 
   def remove_favorites

app/models/user.rb

@@ -67,7 +67,7 @@ class User < ApplicationRecord
 
   has_bit_flags BOOLEAN_ATTRIBUTES, :field => "bit_prefs"
 
-  attr_accessor :password, :old_password, :signed_user_id
+  attr_reader :password
 
   after_initialize :initialize_attributes, if: :new_record?
   validates :name, user_name: true, on: :create
@@ -78,8 +78,6 @@ class User < ApplicationRecord
   validates_presence_of :comment_threshold
   validate :validate_sock_puppets, :on => :create, :if => -> { Danbooru.config.enable_sock_puppet_validation? }
   before_validation :normalize_blacklisted_tags
-  before_create :encrypt_password_on_create
-  before_update :encrypt_password_on_update
   before_create :promote_to_admin_if_first_user
   before_create :customize_new_user
   has_many :artists, foreign_key: :creator_id
@@ -170,26 +168,19 @@ class User < ApplicationRecord
       bcrypt_password_hash.slice(20, 100)
     end
 
-    def encrypt_password_on_create
-      self.bcrypt_password_hash = User.bcrypt(password)
-    end
-
-    def encrypt_password_on_update
-      return if password.blank?
-
-      if signed_user_id.present? && id == Danbooru::MessageVerifier.new(:login).verify(signed_user_id)
-        self.bcrypt_password_hash = User.bcrypt(password)
-      elsif old_password.present? && bcrypt_password == User.sha1(old_password)
-        self.bcrypt_password_hash = User.bcrypt(password)
-      else
-        errors[:old_password] << "is incorrect"
-      end
+    def password=(new_password)
+      @password = new_password
+      self.bcrypt_password_hash = User.bcrypt(new_password)
     end
   end
 
   module AuthenticationMethods
     extend ActiveSupport::Concern
 
+    def authenticate_login_key(signed_user_id)
+      signed_user_id.present? && id == Danbooru::MessageVerifier.new(:login).verify(signed_user_id)
+    end
+
     module ClassMethods
       def authenticate(name, pass)
         authenticate_hash(name, sha1(pass))

app/policies/password_policy.rb

@@ -2,8 +2,4 @@ class PasswordPolicy < ApplicationPolicy
   def update?
     record.id == user.id || user.is_admin?
   end
-
-  def permitted_attributes
-    [:signed_user_id, :old_password, :password, :password_confirmation]
-  end
 end

test/functional/passwords_controller_test.rb

@@ -14,13 +14,38 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest
     end
 
     context "update action" do
-      should "work" do
+      should "update the password when given a valid old password" do
         put_auth user_password_path(@user), @user, params: { user: { old_password: "12345", password: "abcde", password_confirmation: "abcde" } }
 
-        assert_redirected_to user_path(@user)
+        assert_redirected_to @user
         assert_equal(nil, User.authenticate(@user.name, "12345"))
         assert_equal(@user, User.authenticate(@user.name, "abcde"))
       end
+
+      should "update the password when given a valid login key" do
+        signed_user_id = Danbooru::MessageVerifier.new(:login).generate(@user.id)
+        put_auth user_password_path(@user), @user, params: { user: { password: "abcde", password_confirmation: "abcde", signed_user_id: signed_user_id } }
+
+        assert_redirected_to @user
+        assert_equal(nil, User.authenticate(@user.name, "12345"))
+        assert_equal(@user, User.authenticate(@user.name, "abcde"))
+      end
+
+      should "not update the password when given an invalid old password" do
+        put_auth user_password_path(@user), @user, params: { user: { old_password: "3qoirjqe", password: "abcde", password_confirmation: "abcde" } }
+
+        assert_response :success
+        assert_equal(@user, User.authenticate(@user.name, "12345"))
+        assert_equal(nil, User.authenticate(@user.name, "abcde"))
+      end
+
+      should "not update the password when password confirmation fails for the new password" do
+        put_auth user_password_path(@user), @user, params: { user: { old_password: "12345", password: "abcde", password_confirmation: "qerogijqe" } }
+
+        assert_response :success
+        assert_equal(@user, User.authenticate(@user.name, "12345"))
+        assert_equal(nil, User.authenticate(@user.name, "abcde"))
+      end
     end
   end
 end

test/functional/users_controller_test.rb

@@ -118,6 +118,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
 
         assert_redirected_to User.last
         assert_equal("xxx", User.last.name)
+        assert_equal(User.last, User.authenticate("xxx", "xxxxx1"))
         assert_equal(nil, User.last.email_address)
         assert_no_enqueued_emails
       end
@@ -127,6 +128,7 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
 
         assert_redirected_to User.last
         assert_equal("xxx", User.last.name)
+        assert_equal(User.last, User.authenticate("xxx", "xxxxx1"))
         assert_equal("webmaster@danbooru.donmai.us", User.last.email_address.address)
         assert_enqueued_email_with UserMailer, :welcome_user, args: [User.last]
       end