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