Merge pull request #377 from basecamp/backport/session-checks-1.5.5

End sessions reliably when a password changes (1.5.5)

app/channels/application_cable/connection.rb

@@ -2,19 +2,42 @@ module ApplicationCable
   class Connection < ActionCable::Connection::Base
     include Authentication::SessionLookup
 
+    SESSION_CHECK_INTERVAL = 1.minute
+
     identified_by :current_user
 
     def connect
       self.current_user = find_verified_user
+      @session_checked_at = Time.now
+    end
+
+    # Signing out or changing the password ends sessions and then disconnects their connections, but a connection can
+    # miss that: one that found its session just before isn't listening yet. So on the server's heartbeat, which
+    # reaches every open connection, each one looks its session up again now and then, and closes once it's gone. A
+    # rejected connection has no session, and stays registered until its socket closes.
+    def beat
+      super
+      check_session if @session_checked_at&.before?(SESSION_CHECK_INTERVAL.ago)
     end
 
     private
       def find_verified_user
-        if verified_session = find_session_by_cookie
+        if @verified_session = find_session_by_cookie
-          verified_session.user
+          @verified_session.user
         else
           reject_unauthorized_connection
         end
       end
+
+      def check_session
+        @session_checked_at = Time.now
+        perform_work -> { close_unless_session_remains }, :call
+      end
+
+      def close_unless_session_remains
+        unless Session.exists?(@verified_session.id)
+          close reason: ActionCable::INTERNAL[:disconnect_reasons][:unauthorized]
+        end
+      end
   end
 end

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:)
+  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. 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/channels/application_cable/connection_test.rb

@@ -18,4 +18,53 @@ class ApplicationCable::ConnectionTest < ActionCable::Connection::TestCase
 
     assert_reject_connection { connect }
   end
+
+  test "closes once its session has ended without it hearing" do
+    cookies.signed[:session_token] = sessions(:david_safari).token
+    connect
+    sessions(:david_safari).destroy
+
+    travel 2.minutes
+    connection.beat
+
+    assert socket.closed
+    assert_equal "disconnect", socket.transmissions.last[:type]
+    assert socket.transmissions.last[:reconnect]
+  end
+
+  test "keeps checking its session while open" do
+    cookies.signed[:session_token] = sessions(:david_safari).token
+    connect
+
+    travel 2.minutes
+    connection.beat
+    assert_not socket.closed
+
+    sessions(:david_safari).destroy
+    travel 2.minutes
+    connection.beat
+    assert socket.closed
+  end
+
+  test "checks its session at most once a minute" do
+    cookies.signed[:session_token] = sessions(:david_safari).token
+    connect
+    sessions(:david_safari).destroy
+
+    travel 30.seconds
+    connection.beat
+
+    assert_not socket.closed
+  end
+
+  test "a rejected connection's heartbeat doesn't fail" do
+    request = ActionCable::Connection::TestSocket.build_request(ActionCable.server.config.mount_path || "/cable", cookies: cookies)
+    rejected = ApplicationCable::Connection.new \
+      ActionCable::Connection::TestServer.new(ActionCable.server), ActionCable::Connection::TestSocket.new(request)
+    rejected.handle_open
+
+    travel 2.minutes
+
+    assert_nothing_raised { rejected.beat }
+  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")