diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index a6acc99..6b30b74 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/app/controllers/users/profiles_controller.rb @@ -2,7 +2,7 @@ class Users::ProfilesController < ApplicationController before_action :set_user # Limited like signing in, so the current password can't be guessed here instead. - rate_limit to: 10, within: 3.minutes, only: :update, if: :changing_password?, + rate_limit to: 10, within: 3.minutes, only: :update, if: :answering_password_challenge?, with: -> { redirect_to user_profile_url, alert: "Too many attempts. Try again in a few minutes." } def show @@ -25,19 +25,22 @@ class Users::ProfilesController < ApplicationController @user = Current.user end + # Always with the challenge, so the model checks it whenever the save would change the password, however the + # request spelled the new one. def user_params - permitted = params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact - - if changing_password? - permitted.merge(password_challenge: params[:user][:password_challenge].to_s) - else - permitted - end + params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact + .merge(password_challenge: password_challenge) end - # As has_secure_password reads it: an empty password leaves the current one alone. - def changing_password? - !params.dig(:user, :password).to_s.empty? + # Only a string is a challenge, so the rate limit counts exactly what the model checks. + def password_challenge + challenge = params.dig(:user, :password_challenge) + challenge.is_a?(String) ? challenge : "" + end + + # Guessing the current password means sending one, so this is what the rate limit counts. + def answering_password_challenge? + !password_challenge.empty? end def update_notice @@ -47,7 +50,7 @@ class Users::ProfilesController < ApplicationController def update_alert if !@user.errors.include?(:password_challenge) @user.errors.full_messages.to_sentence - elsif params[:user][:password_challenge].blank? + elsif password_challenge.empty? "Enter your current password to change it." else "That isn't your current password." diff --git a/app/models/user.rb b/app/models/user.rb index 07f4bd0..6f01f6b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -18,7 +18,7 @@ class User < ApplicationRecord enum :status, %i[ active deactivated banned ], default: :active has_secure_password validations: false - validate :password_challenge_matches, unless: -> { password_challenge.nil? } + validate :password_challenge_matches, if: :will_save_change_to_password_digest?, unless: -> { password_challenge.nil? } after_create_commit :grant_membership_to_open_rooms after_update :sign_out_all_sessions, if: :saved_change_to_password_digest? @@ -52,7 +52,8 @@ class User < ApplicationRecord end private - # The profile sets password_challenge whenever it changes the password; scripts that reset one don't. + # The profile always sets password_challenge, and it's checked whenever the save would change the password; scripts + # that reset one don't set it. def password_challenge_matches unless password_digest_was.present? && BCrypt::Password.new(password_digest_was).is_password?(password_challenge) errors.add :password_challenge, :invalid diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 3d82162..49fcb3d 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -116,6 +116,18 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest user: { name: "Someone else", password: "a new secret123456", password_challenge: "not my password" } end + test "a new password needs the current one however the form spells it" do + assert_password_change_refused "Enter your current password to change it.", + user: { name: "Someone else", "password(1)" => "a new secret123456" } + end + + test "editing the profile with an empty current password doesn't need it" do + put user_profile_url, params: { user: { name: "Dave", password: "", password_challenge: "" } } + + assert_redirected_to user_profile_url + assert_equal "Dave", users(:david).reload.name + end + test "editing the profile without changing the password doesn't need the current one" do put user_profile_url, params: { user: { name: "Dave", bio: "Acrobat", password: "" } } @@ -132,10 +144,30 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest assert_equal "Too many attempts. Try again in a few minutes.", flash[:alert] assert users(:david).reload.authenticate("secret123456") + put user_profile_url, params: { user: { password: "a new secret123456" } } + assert_equal "Enter your current password to change it.", flash[:alert] + put user_profile_url, params: { user: { name: "Dave" } } assert_equal "Dave", users(:david).reload.name end + test "any current password sent counts toward the rate limit, however blank it looks" do + Users::ProfilesController.cache_store.stubs(:increment).returns(11) + + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: " " } } + assert_equal "Too many attempts. Try again in a few minutes.", flash[:alert] + end + + test "a current password that isn't text is no current password" do + [ false, [], {}, 0 ].each do |challenge| + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: challenge } }, as: :json + + assert_redirected_to user_profile_url + assert_equal "Enter your current password to change it.", flash[:alert] + assert users(:david).reload.authenticate("secret123456") + end + end + test "changing the password removes the other devices' push subscriptions, and this browser can register its own again" do stub_web_push_dns_resolution endpoint = push_subscriptions(:david_chrome).attributes.slice("endpoint", "p256dh_key", "auth_key") diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 5cd445a..db29028 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -6,6 +6,14 @@ class UserTest < ActiveSupport::TestCase assert users(:david).valid? end + test "a password challenge is checked whenever the password would change" do + assert_not User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "") + assert_not User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "not my password") + assert User.find(users(:david).id).update(name: "Dave", password_challenge: "") + assert User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "secret123456") + assert User.find(users(:david).id).update(password: "another secret123456") + end + test "creating users grants membership to the open rooms" do assert_difference -> { Membership.count }, +Rooms::Open.count do create_new_user