diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index bdf13c4..a6acc99 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/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 diff --git a/app/helpers/translations_helper.rb b/app/helpers/translations_helper.rb index 6142ab4..6c269ab 100644 --- a/app/helpers/translations_helper.rb +++ b/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", "🇯🇵": "アカウントに名前を付ける" }, diff --git a/app/models/user.rb b/app/models/user.rb index de25de1..716088e 100644 --- a/app/models/user.rb +++ b/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. diff --git a/app/views/users/profiles/show.html.erb b/app/views/users/profiles/show.html.erb index 1fe919d..368853d 100644 --- a/app/views/users/profiles/show.html.erb +++ b/app/views/users/profiles/show.html.erb @@ -69,6 +69,16 @@ +
+ <%= translation_button(:current_password) %> + + +
+
<%= translation_button(:update_password) %> diff --git a/test/controllers/cached_responses_test.rb b/test/controllers/cached_responses_test.rb index df4102c..9168f3b 100644 --- a/test/controllers/cached_responses_test.rb +++ b/test/controllers/cached_responses_test.rb @@ -149,7 +149,7 @@ class CachedResponsesTest < ActionDispatch::IntegrationTest Session.unstub(:find_by) ResponseCache.instance.unstub(:write) - put user_profile_url, params: { user: { password: "a new secret123456" } }, + put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }, headers: { "Sec-Fetch-Site" => "same-origin", "Origin" => "http://once.campfire.test" } assert_redirected_to user_profile_url diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 835166e..4e93520 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/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 diff --git a/test/script/admin/reset_password_test.rb b/test/script/admin/reset_password_test.rb new file mode 100644 index 0000000..637bb95 --- /dev/null +++ b/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