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")