fix: enforce scenario RBAC by reading role from request.state.user - #5
Open
MoonWindow wants to merge 1 commit into
Open
fix: enforce scenario RBAC by reading role from request.state.user#5MoonWindow wants to merge 1 commit into
MoonWindow wants to merge 1 commit into
Conversation
The /api/scenarios handler read the role via getattr(request.state, "role", "admin"), but AuthMiddleware stores the authenticated user as request.state.user (a dict), never as request.state.role. The attribute is therefore always missing and the lookup falls back to "admin", so the per-scene access.roles filter never excludes anyone: viewer/analyst users see scenes restricted to roles they do not hold.
Use get_current_user(request).get("role", "viewer"), consistent with every other handler in this file.
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.
What is wrong
api/main.py, in the/api/scenarioshandler (get_scenarios, around line 1508), reads the caller's role like this:But
AuthMiddleware.dispatch(inauth.py) stores the authenticated user as a dict onrequest.state.user:It never sets
request.state.role. Sogetattr(request.state, "role", ...)always misses and falls back to the default"admin".Concrete failure
Every caller is treated as
adminfor scenario listing, which defeats the per-scene RBAC filter a few lines below:Because
user_roleis always"admin", this condition never excludes anything. Avieweroranalystuser is served scenes whoseaccess.rolesdo not include their role (for example an admin-only scene). The RBAC restriction on this endpoint is silently a no-op for non-admin users.The fix
Use the same helper every other handler in this file already uses to read the current user:
get_current_user(already imported at the top ofmain.py) readsrequest.state.user, so the real role is returned and theaccess.rolesfilter is actually enforced. The default is"viewer"(least privilege) rather than"admin".How it was verified
Stateobject (which is exactly whatrequest.stateis) plus the verbatimget_current_userbody fromauth.py: a middleware-populatedvieweryieldsuser_role == "admin"with the old code and"viewer"with the fix; the admin-only scene filter admits the viewer before the change and rejects them after.python -m py_compile api/main.py auth.pypasses.Not verified
I did not boot the full FastAPI app end-to-end against live DynamoDB (
strandsand AWS resources are required to import/run the app), so this was verified at the unit level of the role-resolution logic rather than through a running server request.