fix(authz): cover create/run/memory/agent routes with permission checks; scope USER.md per user - #4989
fix(authz): cover create/run/memory/agent routes with permission checks; scope USER.md per user#4989simpleqt wants to merge 4 commits into
Conversation
…ks; scope USER.md per user
Route permissions: the AuthorizationProvider model only applied to routes
carrying @require_permission, so POST /api/threads, /api/threads/search,
POST /api/runs/stream and /api/runs/wait (runs:create), every /api/memory
route and the custom-agent routes ran with authentication only — a
provider configured to deny threads:write/runs:create/... could not
enforce those decisions. Add the missing decorators (threads:write,
threads:read, runs:create, memory:read/write, agents:read/write) and
register the new permission names in authz.Permissions. No owner checks
are added: per-user data scoping stays in the repository layer.
User profile: GET/PUT /api/user-profile read and wrote a single global
{base_dir}/USER.md, so in a multi-user deployment any authenticated user
could overwrite the prompt context injected for everyone else (and read
it). Scope the file to the caller's bucket
({base_dir}/users/{user_id}/USER.md) like user-scoped skills; no other
consumer of the old global path exists in the tree (verified by grep and
the updated tests). test_put_user_profile asserted the wrong effective
user under the autouse conftest user fixture; fixed to test-user-autouse.
github-actions
Bot
added
area:backend
|
|
||
|
|
||
| @router.post("", response_model=ThreadResponse) | ||
| @require_permission("threads", "write") |
There was a problem hiding this comment.
[P1] Preserve positional request binding
Adding this decorator wraps create_thread with require_permission, whose wrapper only looks for request in keyword arguments. Two existing callers in test_threads_router.py invoke create_thread(body, request) positionally, so the wrapper injects a stub request and then calls the handler with both a positional and keyword request, raising TypeError: create_thread() got multiple values for argument request. Running pytest tests/test_threads_router.py -q now fails those two tests. Please either bind positional arguments in the decorator or update both direct callers so the backend unit suite remains green.
| The profile is per-user (like custom skills/agents) so one user's | ||
| prompt context can never be written or injected for another user. | ||
| """ | ||
| return self.user_dir(user_id) / "USER.md" |
There was a problem hiding this comment.
[P2] Migrate the legacy global USER.md
This makes every existing {base_dir}/USER.md invisible after upgrade, while scripts/migrate_user_isolation.py currently migrates legacy memory, threads, agents, and skills but not this profile. Consequently, an auth-disabled or single-user installation that already saved a profile gets content: null after upgrading, and the next PUT creates a second file while the old profile remains stranded. A global runtime fallback would be unsafe for multi-user deployments, but the migration script should claim this file for --user-id (defaulting to default) just like the other unowned legacy artifacts.
…y USER.md Review follow-up on bytedance#4989 (willem-bd): - require_permission's wrapper only looked for request in keyword arguments, so direct positional calls like create_thread(body, request) made the injected keyword stub collide with the positional request (TypeError: multiple values). The wrapper now binds the wrapped signature via inspect.signature().bind() and honors a positionally-passed request (and thread_id) instead of assuming kwargs; the test-stub injection only fires when request is absent everywhere. The two positional callers in tests/test_threads_router.py pass again (8/8 channel tests). - migrate_user_isolation.py now claims the legacy global USER.md for --user-id (default 'default') like the other unowned legacy artifacts; without it, upgrading installs with an existing profile would read content: null and later strand the old file beside the new per-user one. Conflict handling mirrors migrate_memory (rename to USER.legacy.md).
|
Both findings fixed in a71251b: P1 — positional request binding: P2 — legacy USER.md migration: |
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at c8a51c49.
[P1] Update the internal-request test stubs for the new create_thread permission wrapper.
Adding @require_permission("threads", "write") makes the existing direct calls in test_insert_race_recovery_claims_unscoped_row_for_trusted_owner and test_internal_owner_header_assigns_thread_to_owner authenticate their SimpleNamespace requests. Those stubs have a state.user, but no state.auth_source=AUTH_SOURCE_INTERNAL and no cookies, so _authenticate() falls through to request.cookies.get(...) and raises AttributeError.
At this head, the focused suite reports 305 passed and these 2 failures. I checked the exact same two tests on base commit 1aa813dd; both pass there, so this is introduced by the new decorator rather than being an environment-only failure. Please give the stubs realistic internal-auth fields (auth_source and cookies={}), or deliberately call create_thread.__wrapped__ if those tests are intended to bypass the permission boundary.
Ruff lint and formatting pass on the current head.
|
The lint-backend failure was an E501 in the |
The create_thread permission wrapper added in this PR authenticates
the direct calls in the internal-owner tests; their SimpleNamespace
requests had state.user but no auth_source and no cookies, so
get_current_user_from_request fell through to request.cookies.get and
raised AttributeError (verified introduced by this branch: both tests
pass on the base commit).
The stubs now carry cookies={} and
state.auth_source=AUTH_SOURCE_INTERNAL, which is exactly what
AuthMiddleware stamps on real internal requests, so state.user is
honored without the JWT path. All 80 tests in the file pass.
|
You're right, and thanks for checking the base commit — my earlier "environmental" claim was wrong. My stash comparison only covered the uncommitted working-tree diff, so both sides of it still had the decorator from the earlier commit; the two failures were indeed introduced by this branch. Fixed in 03df120 with your first option: the SimpleNamespace request stubs in the three internal-owner call sites now carry |
|
That failing run (32819974159) is on a71251b — it predates the stub fix. Its two failures are exactly the The 03df120 workflow runs are all |
Route permissions: the AuthorizationProvider model only applied to routes
carrying @require_permission, so POST /api/threads, /api/threads/search,
POST /api/runs/stream and /api/runs/wait (runs:create), every /api/memory
route and the custom-agent routes ran with authentication only — a
provider configured to deny threads:write/runs:create/... could not
enforce those decisions. Add the missing decorators (threads:write,
threads:read, runs:create, memory:read/write, agents:read/write) and
register the new permission names in authz.Permissions. No owner checks
are added: per-user data scoping stays in the repository layer.
User profile: GET/PUT /api/user-profile read and wrote a single global
{base_dir}/USER.md, so in a multi-user deployment any authenticated user
could overwrite the prompt context injected for everyone else (and read
it). Scope the file to the caller's bucket
({base_dir}/users/{user_id}/USER.md) like user-scoped skills; no other
consumer of the old global path exists in the tree (verified by grep and
the updated tests). test_put_user_profile asserted the wrong effective
user under the autouse conftest user fixture; fixed to test-user-autouse.