Merge pull request #372 from basecamp/password-check-on-digest-change

Check the current password whenever the saved password changes

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

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

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

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