Fix race condition in test helpers where background requests steal login_as callbacks#220
Open
MatheusRich wants to merge 1 commit into
Open
Conversation
…gin_as callbacks `login_as` and `logout` used `Warden.on_next_request`, a global FIFO queue consumed by the next non-asset request through the Warden middleware. This meant any background request (Turbo Frame fetches, ActionCable `/cable` reconnects) could race against the intended `visit` and consume the authentication callback, leaving the user silently unauthenticated. The `asset_paths` mechanism (introduced in wardencommunity#45 for wardencommunity#44) partially addressed this by skipping asset requests, but it cannot cover application-level background requests like `/cable` or Turbo Frame endpoints — these are normal HTTP requests that go through the full middleware stack. Users had to work around this by extending `asset_paths` to match `/cable` and other paths (wardencommunity#182), which is fragile and requires knowledge of Warden internals. This commit replaces the one-shot queue mechanism in `login_as` with a persistent `_test_users` hash keyed by scope. On each request, the `on_request` hook checks whether the scope already has a user stored in the Rack session before applying the test user. This approach fixes the race condition because: - Background requests from an established session have `stored?=true`, so they are skipped — there is no callback to steal. - The intended browser's first request has `stored?=false` and gets the test user applied via `set_user`, which writes to the Rack session. - Subsequent requests from the same browser load the user from session normally — the test user is not "consumed" like `on_next_request` was. - `on_next_request` is preserved for backward compatibility and is still used by `logout` to clear the Rack session on the next request. Fixes wardencommunity#163, fixes wardencommunity#182.
Author
|
cc @jsmestad |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
login_asandlogoutusedWarden.on_next_request, a global FIFO queue consumed by the next non-asset request through the Warden middleware. This meant any background request (Turbo Frame fetches, ActionCable/cablereconnects) could race against the intendedvisitand consume the authentication callback, leaving the user silently unauthenticated.The
asset_pathsmechanism (introduced in #45 for #44) partially addressed this by skipping asset requests, but it cannot cover application-level background requests like/cableor Turbo Frame endpoints — these are normal HTTP requests that go through the full middleware stack. Users had to work around this by extendingasset_pathsto match/cableand other paths (#182), which is fragile and requires knowledge of Warden internals.This commit replaces the one-shot queue mechanism in
login_aswith a persistent_test_usershash keyed by scope. On each request, theon_requesthook checks whether the scope already has a user stored in the Rack session before applying the test user.This approach fixes the race condition because:
stored?=true, so they are skipped — there is no callback to steal.stored?=falseand gets the test user applied viaset_user, which writes to the Rack session.on_next_requestwas.on_next_requestis preserved for backward compatibility and is still used bylogoutto clear the Rack session on the next request.Fixes #163, fixes #182.