fix(auth): make require_auth async so the user ContextVar reaches the endpoint#485
Open
truffle-dev wants to merge 1 commit into
Open
fix(auth): make require_auth async so the user ContextVar reaches the endpoint#485truffle-dev wants to merge 1 commit into
truffle-dev wants to merge 1 commit into
Conversation
… endpoint Closes HKUDS#481. When AUTH_ENABLED=true, every non-admin HTTP request hit 404 because the user ContextVar set inside require_auth was invisible by the time the endpoint resolved the per-user path service. require_auth was declared as a sync def. FastAPI dispatches sync deps via anyio.to_thread.run_sync, which captures the request context with copy_context() and runs the function in a worker thread under that copy. Mutations made inside the thread, including ContextVar.set, live on the copy and are discarded when the thread returns. The endpoint then ran in the original event-loop context with _current_user unset, so get_current_path_service() fell back to PathService.get_instance(), which points at data/user/ -- the admin workspace. Admin requests appeared to work because the fallback coincided with their intended workspace; user requests resolved to the admin DB and 404'd on every session lookup. The user-context middleware that PR HKUDS#474 introduced for exactly this case was removed in 8bae3ca on the assumption that require_auth was sufficient. With require_auth running in a threadpool, it was not. Declaring require_auth (and require_admin, for symmetry) as async def keeps the dependency on the event loop; the ContextVar mutation now propagates to the endpoint and per-user path resolution works. Regression coverage in tests/api/test_auth_contextvar.py pins both invariants: the dependencies are async, and the user ContextVar set inside require_auth is observable from the endpoint and from get_path_service().
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.
Description
When
AUTH_ENABLED=true, every non-admin HTTP request hits 404 because the userContextVarset insiderequire_authis invisible by the time the endpoint resolves the per-user path service.require_authis declared as a syncdef. FastAPI dispatches sync deps viaanyio.to_thread.run_sync, which captures the request context withcopy_context()and runs the function in a worker thread under that copy. Mutations made inside the thread, includingContextVar.set, live on the copy and are discarded when the thread returns. The endpoint then runs in the original event-loop context with_current_userunset, soget_current_path_service()falls back toPathService.get_instance(), which points atdata/user/— the admin workspace. Admin requests appear to work because the fallback coincides with their intended workspace; user requests resolve to the admin DB and 404 on every session lookup.The user-context middleware that #474 introduced for exactly this case was removed in 8bae3ca on the assumption that
require_authwas sufficient. Withrequire_authrunning in a threadpool, it was not.Declaring
require_auth(andrequire_admin, for symmetry) asasync defkeeps the dependency on the event loop; theContextVarmutation now propagates to the endpoint and per-user path resolution works.Related Issues
Module(s) Affected
apitestsChecklist
pre-commit run --all-filesand fixed any issues.Additional Notes
Regression coverage in
tests/api/test_auth_contextvar.pypins two invariants:require_authandrequire_adminare declaredasync.AUTH_ENABLED=trueand a valid token, the userContextVarset insiderequire_authis visible from inside the endpoint, soget_path_service()resolves to the per-usermulti-user/<uid>/workspace.Verified locally with
pytest tests/api/test_auth_contextvar.py— all four cases pass on the fix and fail on the prior syncdef.