From 5faa32372932d74804713b929b2eea22cda9484f Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 24 Mar 2020 15:29:28 -0500 Subject: [PATCH] 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 | 14 ++++++---- app/logical/user_deletion.rb | 6 +--- app/models/user.rb | 25 ++++++----------- app/policies/password_policy.rb | 4 --- test/functional/passwords_controller_test.rb | 29 ++++++++++++++++++-- test/functional/users_controller_test.rb | 2 ++ 6 files changed, 47 insertions(+), 33 deletions(-) diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index 9391d330e..d1b16b1f0 100644 --- a/app/controllers/passwords_controller.rb +++ b/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 diff --git a/app/logical/user_deletion.rb b/app/logical/user_deletion.rb index e06b2260e..4fa4b06a5 100644 --- a/app/logical/user_deletion.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 69db7613d..106392c7b 100644 --- a/app/models/user.rb +++ b/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)) diff --git a/app/policies/password_policy.rb b/app/policies/password_policy.rb index de693f981..a315c507e 100644 --- a/app/policies/password_policy.rb +++ b/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 diff --git a/test/functional/passwords_controller_test.rb b/test/functional/passwords_controller_test.rb index e6bab032f..83057663d 100644 --- a/test/functional/passwords_controller_test.rb +++ b/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 diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index 689471895..c002f7b6c 100644 --- a/test/functional/users_controller_test.rb +++ b/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