Give the changing browser a new session rather than keeping its own

Keeping the session the password was changed from also kept any copy of
its cookie working, and a copied cookie is one of the ways someone else
gets into an account. Every session ends now, and the profile page
signs the browser that changed the password back in with a new one.

From #371.

(cherry picked from commit b01bb7a0a7)

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
 

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

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)

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