Ask for the current password to change it

Changing the password now ends every other session, so whoever holds
one could shut the owner out of their own account with a new password.
The profile asks for the current password whenever the password
changes; without it, or with a wrong one, nothing is saved and the page
says why. Other profile edits don't need it, and the admin
reset-password script still works without it.

Attempts are rate limited like signing in, 10 in 3 minutes per address,
so the current password can't be guessed here instead.

From #371. The cached-response test it touches isn't in 1.5.2.

(cherry picked from commit 44a92ff6c6)

app/controllers/users/profiles_controller.rb

@@ -1,16 +1,23 @@
 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?,
+    with: -> { redirect_to user_profile_url, alert: "Too many attempts. Try again in a few minutes." }
+
   def show
     @direct_memberships, @shared_memberships =
       Current.user.memberships.with_ordered_room.partition { |m| m.room.direct? }
   end
 
   def update
-    @user.update user_params
-    start_new_session_for @user if @user.saved_change_to_password_digest?
+    if @user.update user_params
+      start_new_session_for @user if @user.saved_change_to_password_digest?
 
-    redirect_to user_profile_url, notice: update_notice
+      redirect_to user_profile_url, notice: update_notice
+    else
+      redirect_to user_profile_url, alert: update_alert
+    end
   end
 
   private
@@ -19,10 +26,31 @@ class Users::ProfilesController < ApplicationController
     end
 
     def user_params
-      params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact
+      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
+    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?
     end
 
     def update_notice
       params[:user][:avatar] ? "It may take up to 30 minutes to change everywhere." : "✓"
     end
+
+    def update_alert
+      if !@user.errors.include?(:password_challenge)
+        @user.errors.full_messages.to_sentence
+      elsif params[:user][:password_challenge].blank?
+        "Enter your current password to change it."
+      else
+        "That isn't your current password."
+      end
+    end
 end

app/helpers/translations_helper.rb

@@ -2,6 +2,7 @@ module TranslationsHelper
   TRANSLATIONS = {
     email_address:  { "🇺🇸": "Enter your email address", "🇪🇸": "Introduce tu correo electrónico", "🇫🇷": "Entrez votre adresse courriel", "🇮🇳": "अपना ईमेल पता दर्ज करें", "🇩🇪": "Geben Sie Ihre E-Mail-Adresse ein", "🇧🇷": "Insira seu endereço de email", "🇯🇵": "メールアドレスを入力してください" },
     password: { "🇺🇸": "Enter your password", "🇪🇸": "Introduce tu contraseña", "🇫🇷": "Saisissez votre mot de passe", "🇮🇳": "अपना पासवर्ड दर्ज करें", "🇩🇪": "Geben Sie Ihr Passwort ein", "🇧🇷": "Insira sua senha", "🇯🇵": "パスワードを入力してください" },
+    current_password: { "🇺🇸": "Enter your current password", "🇪🇸": "Introduce tu contraseña actual", "🇫🇷": "Saisissez votre mot de passe actuel", "🇮🇳": "अपना वर्तमान पासवर्ड दर्ज करें", "🇩🇪": "Geben Sie Ihr aktuelles Passwort ein", "🇧🇷": "Insira sua senha atual", "🇯🇵": "現在のパスワードを入力してください" },
     update_password: { "🇺🇸": "Change password", "🇪🇸": "Cambiar contraseña", "🇫🇷": "Changer le mot de passe", "🇮🇳": "पासवर्ड बदलें", "🇩🇪": "Passwort ändern", "🇧🇷": "Alterar senha", "🇯🇵": "パスワードを変更" },
     user_name: { "🇺🇸": "Enter your name", "🇪🇸": "Introduce tu nombre", "🇫🇷": "Entrez votre nom", "🇮🇳": "अपना नाम दर्ज करें", "🇩🇪": "Geben Sie Ihren Namen ein", "🇧🇷": "Insira seu nome", "🇯🇵": "お名前を入力してください" },
     account_name: { "🇺🇸": "Name this account", "🇪🇸": "Nombre de esta cuenta", "🇫🇷": "Nommez ce compte", "🇮🇳": "इस खाते का नाम दें", "🇩🇪": "Benennen Sie dieses Konto", "🇧🇷": "Dê um nome a essa conta", "🇯🇵": "アカウントに名前を付ける" },

app/models/user.rb

@@ -18,6 +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? }
 
   after_create_commit :grant_membership_to_open_rooms
   after_update :sign_out_all_sessions, if: :saved_change_to_password_digest?
@@ -51,6 +52,13 @@ class User < ApplicationRecord
   end
 
   private
+    # The profile sets password_challenge whenever it changes the password; scripts that reset one don't.
+    def password_challenge_matches
+      unless password_digest_was.present? && BCrypt::Password.new(password_digest_was).is_password?(password_challenge)
+        errors.add :password_challenge, :invalid
+      end
+    end
+
     # A new password is how someone shuts out whoever else got into their account, so it ends every session, copies
     # of the changing browser's cookie included. Connections close once that commits, registered on the transaction
     # because a later save in it would replace the saved change an after_commit condition reads.

app/views/users/profiles/show.html.erb

@@ -69,6 +69,16 @@
         </label>
       </div>
 
+      <div class="flex align-center gap">
+        <%= translation_button(:current_password) %>
+
+        <label class="flex align-center gap flex-item-grow input input--actor">
+          <%= form.password_field :password_challenge, class: "input txt-large", autocomplete: "current-password",
+                placeholder: "Current password", required: false, maxlength: 72 %>
+          <%= image_tag "password.svg", aria: { hidden: "true" }, size: 24, class: "colorize--black" %>
+        </label>
+      </div>
+
       <div class="flex align-center gap">
         <%= translation_button(:update_password) %>
 

test/controllers/users/profiles_controller_test.rb

@@ -33,7 +33,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     other_device.get user_profile_url
     other_device.assert_response :success
 
-    put user_profile_url, params: { user: { password: "a new secret123456" } }
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
     assert_redirected_to user_profile_url
 
     other_device.get user_profile_url
@@ -46,7 +46,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
   test "changing the password gives this browser a new session, so a copy of its cookie stops working" do
     copied_cookie = cookies[:session_token]
 
-    put user_profile_url, params: { user: { password: "a new secret123456" } }
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
     assert_not_equal copied_cookie, cookies[:session_token]
 
     copy = open_session
@@ -60,7 +60,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     remote_connections.expects(:disconnect).with(reconnect: true)
     ActionCable.server.stubs(:remote_connections).returns(mock.tap { |m| m.expects(:where).with(current_user: users(:david)).returns(remote_connections) })
 
-    put user_profile_url, params: { user: { password: "a new secret123456", avatar: fixture_file_upload("moon.jpg", "image/jpeg") } }
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456", avatar: fixture_file_upload("moon.jpg", "image/jpeg") } }
     assert_redirected_to user_profile_url
     assert users(:david).reload.avatar.attached?
 
@@ -73,7 +73,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     remote_connections.expects(:disconnect).with(reconnect: true)
     ActionCable.server.stubs(:remote_connections).returns(mock.tap { |m| m.expects(:where).with(current_user: users(:david)).returns(remote_connections) })
 
-    put user_profile_url, params: { user: { password: "a new secret123456" } }
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
 
     assert_redirected_to user_profile_url
   end
@@ -82,7 +82,7 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     other_session = users(:david).sessions.first
     ActionCable.server.stubs(:remote_connections).raises(RuntimeError.new("cable down"))
 
-    put user_profile_url, params: { user: { password: "a new secret123456" } }
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
 
     assert_redirected_to user_profile_url
     assert_not Session.exists?(other_session.id)
@@ -98,9 +98,65 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     assert_redirected_to user_profile_url
   end
 
+  test "changing the password with the current one" do
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+
+    assert_redirected_to user_profile_url
+    assert_equal "✓", flash[:notice]
+    assert users(:david).reload.authenticate("a new secret123456")
+  end
+
+  test "changing the password without the current one changes nothing" do
+    assert_password_change_refused "Enter your current password to change it.",
+      user: { name: "Someone else", password: "a new secret123456" }
+  end
+
+  test "changing the password with a wrong current one changes nothing" do
+    assert_password_change_refused "That isn't your current password.",
+      user: { name: "Someone else", password: "a new secret123456", password_challenge: "not my password" }
+  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: "" } }
+
+    assert_redirected_to user_profile_url
+    assert_equal "Dave", users(:david).reload.name
+    assert users(:david).authenticate("secret123456")
+  end
+
+  test "attempts to change the password are rate limited like signing in" do
+    Users::ProfilesController.cache_store.stubs(:increment).returns(11)
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+    assert_redirected_to user_profile_url
+    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: { name: "Dave" } }
+    assert_equal "Dave", users(:david).reload.name
+  end
+
   test "updates are limited to the current user" do
     put user_profile_url(users(:jason)), params: { user: { name: "John Doe" } }
 
     assert_equal "Jason", users(:jason).reload.name
   end
+
+  private
+    def assert_password_change_refused(alert, params)
+      other_device = open_session
+      other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" }
+      ActionCable.server.expects(:remote_connections).never
+
+      assert_no_difference -> { Session.count } do
+        put user_profile_url, params: params
+      end
+
+      assert_redirected_to user_profile_url
+      assert_equal alert, flash[:alert]
+      assert users(:david).reload.authenticate("secret123456")
+      assert_equal "David", users(:david).name
+      other_device.get user_profile_url
+      other_device.assert_response :success
+    end
 end

test/script/admin/reset_password_test.rb

@@ -0,0 +1,21 @@
+require "test_helper"
+
+class ResetPasswordScriptTest < ActiveSupport::TestCase
+  test "resets a password without the current one and signs out every session" do
+    assert_output "Password has been reset\n" do
+      run_script "david@37signals.com", "a new secret123456"
+    end
+
+    assert users(:david).reload.authenticate("a new secret123456")
+    assert_empty users(:david).sessions
+  end
+
+  private
+    def run_script(*arguments)
+      original_arguments = ARGV.dup
+      ARGV.replace(arguments)
+      load Rails.root.join("script/admin/reset-password").to_s
+    ensure
+      ARGV.replace(original_arguments)
+    end
+end