Refuse a session for a password that changed after it was checked

A password change ends every session, but a sign-in or sign-in link checked
against the old password just before the change could still save its session
just after, and that session outlived the change.

The session now checks, as it's saved, that the user's password is still the
one the request loaded. The save's transaction holds SQLite's write lock from
the start, so a change commits either before the check, which fails it, or
after the insert, and ends this session with the others. Sessions start
through the user's association so the check sees the user that was
authenticated, and a refused sign-in is answered like a wrong password.

(cherry picked from commit 19319a1cff)

app/controllers/concerns/authentication.rb

@@ -57,7 +57,7 @@ module Authentication
     end
 
     def start_new_session_for(user)
-      user.sessions.start!(user_agent: request.user_agent, ip_address: request.remote_ip).tap do |session|
+      user.sessions.create!(user_agent: request.user_agent, ip_address: request.remote_ip).tap do |session|
         authenticated_as session
       end
     end

app/controllers/sessions/transfers_controller.rb

@@ -11,5 +11,7 @@ class Sessions::TransfersController < ApplicationController
     else
       head :bad_request
     end
+  rescue ActiveRecord::RecordInvalid
+    head :bad_request
   end
 end

app/controllers/sessions_controller.rb

@@ -14,6 +14,8 @@ class SessionsController < ApplicationController
     else
       render_rejection :unauthorized
     end
+  rescue ActiveRecord::RecordInvalid
+    render_rejection :unauthorized
   end
 
   def destroy

app/models/session.rb

@@ -7,13 +7,22 @@ class Session < ApplicationRecord
 
   before_create { self.last_active_at ||= Time.now }
 
-  def self.start!(user_agent:, ip_address:)
+  validate :user_password_unchanged, on: :create
-    create! user_agent: user_agent, ip_address: ip_address
-  end
 
   def resume(user_agent:, ip_address:)
     if last_active_at.before?(ACTIVITY_REFRESH_RATE.ago)
       update! user_agent: user_agent, ip_address: ip_address, last_active_at: Time.now
     end
   end
+
+  private
+    # A password change ends every session, so one can't start on a password checked before the change. Validated in
+    # the save's transaction, which holds the database's write lock from the start, so a change commits either before
+    # this check, and fails it, or after the insert, and ends this session with the others. Compared here rather than in
+    # the query, which logs its values.
+    def user_password_unchanged
+      unless User.where(id: user_id).pick(:password_digest) == user.password_digest
+        errors.add :user, :invalid
+      end
+    end
 end

test/controllers/sessions/transfers_controller_test.rb

@@ -30,4 +30,19 @@ class Sessions::TransfersControllerTest < ActionDispatch::IntegrationTest
     assert_response :bad_request
     assert_nil parsed_cookies.signed[:session_token]
   end
+
+  test "update is refused when the password changes after the link was checked" do
+    user = users(:david)
+    transfer_id = user.transfer_id
+    stale = User.find(user.id)
+    User.find(user.id).update!(password: "a new secret123456")
+    User.stubs(:find_by_transfer_id).with(transfer_id).returns(stale)
+
+    assert_no_difference -> { Session.count } do
+      put session_transfer_url(transfer_id)
+    end
+
+    assert_response :bad_request
+    assert_nil parsed_cookies.signed[:session_token]
+  end
 end

test/controllers/sessions_controller_test.rb

@@ -36,6 +36,19 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest
     assert parsed_cookies.signed[:session_token]
   end
 
+  test "create is refused when the password changes after it was checked" do
+    user = User.find(users(:david).id)
+    User.find(user.id).update!(password: "a new secret123456")
+    User.stubs(:authenticate_by).returns(user)
+
+    assert_no_difference -> { Session.count } do
+      post session_url, params: { email_address: "david@37signals.com", password: "secret123456" }
+    end
+
+    assert_response :unauthorized
+    assert_nil parsed_cookies.signed[:session_token]
+  end
+
   test "create with invalid credentials" do
     post session_url, params: { email_address: "david@37signals.com", password: "wrong" }
 

test/models/session_test.rb

@@ -0,0 +1,20 @@
+require "test_helper"
+
+class SessionTest < ActiveSupport::TestCase
+  test "start a session" do
+    assert_difference -> { users(:david).sessions.count }, +1 do
+      users(:david).sessions.create!(user_agent: "This device", ip_address: "127.0.0.1")
+    end
+  end
+
+  test "a session doesn't start on a password checked before it changed" do
+    user = User.find(users(:david).id)
+    User.find(user.id).update!(password: "a new secret123456")
+
+    assert_no_difference -> { Session.count } do
+      assert_raises(ActiveRecord::RecordInvalid) do
+        user.sessions.create!(user_agent: "This device", ip_address: "127.0.0.1")
+      end
+    end
+  end
+end

test/models/user_test.rb

@@ -51,7 +51,7 @@ class UserTest < ActiveSupport::TestCase
   end
 
   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")
+    Current.session = users(:david).sessions.create!(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")