From e158388f3d006a7b01163f5fee95f62a252fd517 Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Mon, 19 Oct 2026 10:57:14 -0700
Subject: [PATCH] Close live connections on the transaction that changed the
 password

An after_commit condition reads the record's last save, so another save
of the user in the same transaction skipped the disconnect even though
the sessions were gone. The callback that deletes them now registers
the disconnect on the transaction itself, which also leaves a rolled
back change alone. A password changed along with the avatar is covered
too: the attachment's touch doesn't hide the change.
---
 app/models/user.rb                            | 10 ++++-----
 .../users/profiles_controller_test.rb         | 13 +++++++++++
 test/models/user_test.rb                      | 22 +++++++++++++++++++
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index c9ffee57..de25de1b 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -21,7 +21,6 @@ class User < ApplicationRecord
 
   after_create_commit :grant_membership_to_open_rooms
   after_update :sign_out_all_sessions, if: :saved_change_to_password_digest?
-  after_update_commit :disconnect_signed_out_sessions, if: :saved_change_to_password_digest?
 
   scope :ordered, -> { order("LOWER(name)") }
   scope :filtered_by, ->(query) { where("name like ?", "%#{query}%") }
@@ -52,14 +51,15 @@ class User < ApplicationRecord
   end
 
   private
-    # A new password is how someone shuts out whoever else got into their account, so it ends every session opened
-    # with the old one, copies of the changing browser's cookie included. The profile page signs that browser back in.
+    # 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.
     def sign_out_all_sessions
       sessions.delete_all
+      self.class.current_transaction.after_commit { disconnect_signed_out_sessions }
     end
 
-    # After commit, so a connection that drops can't come back on a session that's still there. The password is
-    # saved by then, so a realtime service that can't be reached mustn't fail the change.
+    # The password is saved by now, so a realtime service that can't be reached mustn't fail the change.
     def disconnect_signed_out_sessions
       reset_remote_connections
     rescue => error
diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb
index 6d017260..835166e6 100644
--- a/test/controllers/users/profiles_controller_test.rb
+++ b/test/controllers/users/profiles_controller_test.rb
@@ -55,6 +55,19 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     copy.assert_redirected_to new_session_url
   end
 
+  test "changing the password along with the avatar still signs this browser back in and closes 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", avatar: fixture_file_upload("moon.jpg", "image/jpeg") } }
+    assert_redirected_to user_profile_url
+    assert users(:david).reload.avatar.attached?
+
+    get user_profile_url
+    assert_response :success
+  end
+
   test "changing the password closes the account's live connections" do
     remote_connections = mock
     remote_connections.expects(:disconnect).with(reconnect: true)
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 2262dcac..8dbe0ea4 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -58,6 +58,28 @@ class UserTest < ActiveSupport::TestCase
     users(:david).update!(password: "a new secret123456")
   end
 
+  test "changing the password closes live connections even when the user is saved again before commit" 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) })
+
+    User.transaction do
+      users(:david).update!(password: "a new secret123456")
+      users(:david).update!(name: "Dave")
+    end
+  end
+
+  test "a password change that rolls back keeps every session and connection" do
+    ActionCable.server.expects(:remote_connections).never
+
+    assert_no_changes -> { users(:david).sessions.count } do
+      User.transaction do
+        users(:david).update!(password: "a new secret123456")
+        raise ActiveRecord::Rollback
+      end
+    end
+  end
+
   test "changing other details keeps every session" do
     ActionCable.server.expects(:remote_connections).never