From c78c3fd7efada9d64d2bb6b3b3b31d1b845266d0 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 10:28:06 -0700 Subject: [PATCH] Sign out the account's other sessions when its password changes Changing the password is how someone shuts out whoever else is signed in to their account, but every session opened with the old password stayed valid. Now the others end and their live connections close, as on sign-out. The session the password was changed from stays signed in. Changing the password outside a request, as from a console, ends every session. --- app/controllers/users/profiles_controller.rb | 2 + app/models/user.rb | 7 +++ test/controllers/cached_responses_test.rb | 23 ++++++++++ .../users/profiles_controller_test.rb | 46 +++++++++++++++++++ test/models/user_test.rb | 22 +++++++++ 5 files changed, 100 insertions(+) diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index 96015d55..9f3726a7 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/app/controllers/users/profiles_controller.rb @@ -8,6 +8,8 @@ class Users::ProfilesController < ApplicationController def update @user.update user_params + disconnect_remote_connections if @user.saved_change_to_password_digest? + redirect_to user_profile_url, notice: update_notice end diff --git a/app/models/user.rb b/app/models/user.rb index df2fee19..449ab02b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -20,6 +20,7 @@ class User < ApplicationRecord has_secure_password validations: false after_create_commit :grant_membership_to_open_rooms + after_update :sign_out_other_sessions, if: :saved_change_to_password_digest? scope :ordered, -> { order("LOWER(name)") } scope :filtered_by, ->(query) { where("name like ?", "%#{query}%") } @@ -50,6 +51,12 @@ class User < ApplicationRecord end private + # A new password is how someone shuts out whoever else got into their account, so it ends the sessions opened + # with the old one. The session it was changed from, if any, stays. + def sign_out_other_sessions + sessions.excluding(Current.session).delete_all + end + # One statement, which SQLite runs under the write lock, so no room is closed between reading the open ones and # inserting. A room that Room#destroy_later closes is either closed before, and skipped, or after, and takes this # membership away with the others. A room that granted itself to every user when it was created is skipped too, diff --git a/test/controllers/cached_responses_test.rb b/test/controllers/cached_responses_test.rb index 888994d0..df4102c4 100644 --- a/test/controllers/cached_responses_test.rb +++ b/test/controllers/cached_responses_test.rb @@ -136,6 +136,29 @@ class CachedResponsesTest < ActionDispatch::IntegrationTest assert_redirected_to new_session_url end + test "a session signed out by a password change isn't served from a cached snapshot or page" do + other_device = open_session { |session| session.host! "once.campfire.test" } + other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" } + other_device.get user_sidebar_url(:me) + other_device.assert_response :success + + Session.expects(:find_by).never + ResponseCache.instance.expects(:write).never + other_device.get user_sidebar_url(:me) + other_device.assert_response :success + Session.unstub(:find_by) + ResponseCache.instance.unstub(:write) + + put user_profile_url, params: { user: { password: "a new secret123456" } }, + headers: { "Sec-Fetch-Site" => "same-origin", "Origin" => "http://once.campfire.test" } + assert_redirected_to user_profile_url + + other_device.get user_sidebar_url(:me) + other_device.assert_redirected_to new_session_url + get user_sidebar_url(:me) + assert_response :success + end + test "conditional requests keep native validators" do get room_messages_url(@room) etag = response.headers["ETag"] diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 0331aafb..c5f95614 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -27,6 +27,52 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest assert_equal "david@37signals.com", users(:david).email_address end + test "changing the password signs out the account's other sessions but not this one" do + other_device = open_session + other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" } + other_device.get user_profile_url + other_device.assert_response :success + + put user_profile_url, params: { user: { password: "a new secret123456" } } + assert_redirected_to user_profile_url + + other_device.get user_profile_url + other_device.assert_redirected_to new_session_url + get user_profile_url + assert_response :success + assert_equal 1, users(:david).sessions.count + end + + test "changing the password closes the account's live connections" do + remote_connections = mock + 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" } } + + assert_redirected_to user_profile_url + end + + test "changing the password still signs out other sessions when the realtime service is unreachable" do + 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" } } + + assert_redirected_to user_profile_url + assert_not Session.exists?(other_session.id) + end + + test "updating the profile without a new password leaves other sessions signed in" do + ActionCable.server.expects(:remote_connections).never + + assert_no_difference -> { users(:david).sessions.count } do + put user_profile_url, params: { user: { name: "David", password: "" } } + end + + assert_redirected_to user_profile_url + end + test "updates are limited to the current user" do put user_profile_url(users(:jason)), params: { user: { name: "John Doe" } } diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 61bb1c39..0926adaa 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -42,6 +42,28 @@ class UserTest < ActiveSupport::TestCase end end + test "changing the password outside a request signs out every session" do + users(:david).sessions.start!(user_agent: "Other device", ip_address: "127.0.0.1") + + assert_changes -> { users(:david).sessions.count }, to: 0 do + users(:david).update!(password: "a new secret123456") + end + end + + test "changing the password keeps the session it was changed from" do + Current.session = users(:david).sessions.start!(user_agent: "This device", ip_address: "127.0.0.1") + + users(:david).update!(password: "a new secret123456") + + assert_equal [ Current.session ], users(:david).sessions.reload + end + + test "changing other details keeps every session" do + assert_no_changes -> { users(:david).sessions.count } do + users(:david).update!(name: "Dave", email_address: "dave@37signals.com") + end + end + private def create_new_user User.create!(name: "User", email_address: "user@example.com", password: "secret123456")