diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index 96015d55..bdf13c47 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 + start_new_session_for @user 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 2df933ef..c9ffee57 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -20,7 +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? + 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)") } @@ -52,10 +52,10 @@ 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 + # 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. + def sign_out_all_sessions + sessions.delete_all end # After commit, so a connection that drops can't come back on a session that's still there. The password is diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index c5f95614..6d017260 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/test/controllers/users/profiles_controller_test.rb @@ -27,7 +27,7 @@ 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 + test "changing the password signs out the account's other devices but not this browser" 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 @@ -43,6 +43,18 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest assert_equal 1, users(:david).sessions.count end + 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" } } + assert_not_equal copied_cookie, cookies[:session_token] + + copy = open_session + copy.cookies[:session_token] = copied_cookie + copy.get user_profile_url + copy.assert_redirected_to new_session_url + 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 22b8109e..2262dcac 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -42,15 +42,15 @@ 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") + test "changing the password signs out every session, the current one included" do + Current.session = users(:david).sessions.start!(user_agent: "This 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 outside a request closes the account's live connections" do + 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) }) @@ -58,14 +58,6 @@ class UserTest < ActiveSupport::TestCase users(:david).update!(password: "a new secret123456") 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 ActionCable.server.expects(:remote_connections).never