Merge pull request #375 from basecamp/refuse-session-for-changed-password

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

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/controllers/users/profiles_controller.rb

@@ -18,6 +18,9 @@ class Users::ProfilesController < ApplicationController
     else
       redirect_to user_profile_url, alert: update_alert
     end
+  rescue ActiveRecord::RecordInvalid
+    # The password changed again before this browser's new session could start, and that change signed it out.
+    redirect_to new_session_url
   end
 
   private

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:)
-    create! user_agent: user_agent, ip_address: ip_address
-  end
+  validate :user_password_unchanged, on: :create
 
   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. Read past the query cache,
+    # and compared here rather than in the query, which logs its values.
+    def user_password_unchanged
+      unless User.uncached { 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/controllers/users/profiles_controller_test.rb

@@ -43,6 +43,16 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     assert_equal 1, users(:david).sessions.count
   end
 
+  test "changing the password signs this browser out when it changes again before its new session starts" do
+    User.stubs(:uncached).returns(BCrypt::Password.create("someone else's secret", cost: BCrypt::Engine::MIN_COST))
+
+    assert_changes -> { users(:david).sessions.count }, to: 0 do
+      put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+    end
+
+    assert_redirected_to new_session_url
+  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]
 

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