diff --git a/app/channels/application_cable/connection.rb b/app/channels/application_cable/connection.rb index 749265c2..0094130e 100644 --- a/app/channels/application_cable/connection.rb +++ b/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 - verified_session.user + if @verified_session = find_session_by_cookie + @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 diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index e609d33a..be5cf2de 100644 --- a/app/controllers/concerns/authentication.rb +++ b/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 diff --git a/app/controllers/sessions/transfers_controller.rb b/app/controllers/sessions/transfers_controller.rb index 12dff8f4..aff85abd 100644 --- a/app/controllers/sessions/transfers_controller.rb +++ b/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 diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 1ac2bfd6..3a039462 100644 --- a/app/controllers/sessions_controller.rb +++ b/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 diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index 6b30b744..4f5ab314 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/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 diff --git a/app/models/session.rb b/app/models/session.rb index c70b6fa1..9fa3664d 100644 --- a/app/models/session.rb +++ b/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 diff --git a/test/channels/application_cable/connection_test.rb b/test/channels/application_cable/connection_test.rb index 8533ef1d..342e9a58 100644 --- a/test/channels/application_cable/connection_test.rb +++ b/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 diff --git a/test/controllers/sessions/transfers_controller_test.rb b/test/controllers/sessions/transfers_controller_test.rb index 18322a88..262fe7fc 100644 --- a/test/controllers/sessions/transfers_controller_test.rb +++ b/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 diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 4b20dd91..ed1f05be 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/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" } diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 49fcb3da..035318db 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/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] diff --git a/test/models/session_test.rb b/test/models/session_test.rb new file mode 100644 index 00000000..53cf5ac7 --- /dev/null +++ b/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 diff --git a/test/models/user_test.rb b/test/models/user_test.rb index db290288..4bd1bef2 100644 --- a/test/models/user_test.rb +++ b/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")