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

@@ -2,7 +2,7 @@ class Users::ProfilesController < ApplicationController
   before_action :set_user
 
   # Limited like signing in, so the current password can't be guessed here instead.
-  rate_limit to: 10, within: 3.minutes, only: :update, if: :changing_password?,
+  rate_limit to: 10, within: 3.minutes, only: :update, if: :answering_password_challenge?,
     with: -> { redirect_to user_profile_url, alert: "Too many attempts. Try again in a few minutes." }
 
   def show
@@ -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
@@ -25,19 +28,22 @@ class Users::ProfilesController < ApplicationController
       @user = Current.user
     end
 
+    # Always with the challenge, so the model checks it whenever the save would change the password, however the
+    # request spelled the new one.
     def user_params
-      permitted = params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact
+      params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact
-
+        .merge(password_challenge: password_challenge)
-      if changing_password?
-        permitted.merge(password_challenge: params[:user][:password_challenge].to_s)
-      else
-        permitted
-      end
     end
 
-    # As has_secure_password reads it: an empty password leaves the current one alone.
+    # Only a string is a challenge, so the rate limit counts exactly what the model checks.
-    def changing_password?
+    def password_challenge
-      !params.dig(:user, :password).to_s.empty?
+      challenge = params.dig(:user, :password_challenge)
+      challenge.is_a?(String) ? challenge : ""
+    end
+
+    # Guessing the current password means sending one, so this is what the rate limit counts.
+    def answering_password_challenge?
+      !password_challenge.empty?
     end
 
     def update_notice
@@ -47,7 +53,7 @@ class Users::ProfilesController < ApplicationController
     def update_alert
       if !@user.errors.include?(:password_challenge)
         @user.errors.full_messages.to_sentence
-      elsif params[:user][:password_challenge].blank?
+      elsif password_challenge.empty?
         "Enter your current password to change it."
       else
         "That isn't your current password."

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

app/models/user.rb

@@ -18,7 +18,7 @@ class User < ApplicationRecord
   enum :status, %i[ active deactivated banned ], default: :active
 
   has_secure_password validations: false
-  validate :password_challenge_matches, unless: -> { password_challenge.nil? }
+  validate :password_challenge_matches, if: :will_save_change_to_password_digest?, unless: -> { password_challenge.nil? }
 
   after_create_commit :grant_membership_to_open_rooms
   after_update :sign_out_all_sessions, if: :saved_change_to_password_digest?
@@ -52,7 +52,8 @@ class User < ApplicationRecord
   end
 
   private
-    # The profile sets password_challenge whenever it changes the password; scripts that reset one don't.
+    # The profile always sets password_challenge, and it's checked whenever the save would change the password; scripts
+    # that reset one don't set it.
     def password_challenge_matches
       unless password_digest_was.present? && BCrypt::Password.new(password_digest_was).is_password?(password_challenge)
         errors.add :password_challenge, :invalid

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]
 
@@ -116,6 +126,18 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
       user: { name: "Someone else", password: "a new secret123456", password_challenge: "not my password" }
   end
 
+  test "a new password needs the current one however the form spells it" do
+    assert_password_change_refused "Enter your current password to change it.",
+      user: { name: "Someone else", "password(1)" => "a new secret123456" }
+  end
+
+  test "editing the profile with an empty current password doesn't need it" do
+    put user_profile_url, params: { user: { name: "Dave", password: "", password_challenge: "" } }
+
+    assert_redirected_to user_profile_url
+    assert_equal "Dave", users(:david).reload.name
+  end
+
   test "editing the profile without changing the password doesn't need the current one" do
     put user_profile_url, params: { user: { name: "Dave", bio: "Acrobat", password: "" } }
 
@@ -132,10 +154,30 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     assert_equal "Too many attempts. Try again in a few minutes.", flash[:alert]
     assert users(:david).reload.authenticate("secret123456")
 
+    put user_profile_url, params: { user: { password: "a new secret123456" } }
+    assert_equal "Enter your current password to change it.", flash[:alert]
+
     put user_profile_url, params: { user: { name: "Dave" } }
     assert_equal "Dave", users(:david).reload.name
   end
 
+  test "any current password sent counts toward the rate limit, however blank it looks" do
+    Users::ProfilesController.cache_store.stubs(:increment).returns(11)
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: " " } }
+    assert_equal "Too many attempts. Try again in a few minutes.", flash[:alert]
+  end
+
+  test "a current password that isn't text is no current password" do
+    [ false, [], {}, 0 ].each do |challenge|
+      put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: challenge } }, as: :json
+
+      assert_redirected_to user_profile_url
+      assert_equal "Enter your current password to change it.", flash[:alert]
+      assert users(:david).reload.authenticate("secret123456")
+    end
+  end
+
   test "changing the password removes the other devices' push subscriptions, and this browser can register its own again" do
     stub_web_push_dns_resolution
     endpoint = push_subscriptions(:david_chrome).attributes.slice("endpoint", "p256dh_key", "auth_key")

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

@@ -6,6 +6,14 @@ class UserTest < ActiveSupport::TestCase
     assert users(:david).valid?
   end
 
+  test "a password challenge is checked whenever the password would change" do
+    assert_not User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "")
+    assert_not User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "not my password")
+    assert User.find(users(:david).id).update(name: "Dave", password_challenge: "")
+    assert User.find(users(:david).id).update(password: "a new secret123456", password_challenge: "secret123456")
+    assert User.find(users(:david).id).update(password: "another secret123456")
+  end
+
   test "creating users grants membership to the open rooms" do
     assert_difference -> { Membership.count }, +Rooms::Open.count do
       create_new_user
@@ -43,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")