feat: permission/scope alignment (core — no live-read) - #5133
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aligns permissions and scopes, introduces a legacy scope alias bridge for backward compatibility, and adds an admin-target-takeover guard to prevent non-admins from modifying admin accounts. It also implements a live-read /v1/users/me endpoint and corresponding frontend PermissionsContext and PermissionGate to enforce real-time permission updates. Feedback highlights an N+1 query issue when resolving API key owners, a potential decoding failure in JWT parsing when using atob directly on base64url strings, and a recommendation to use optional chaining on the permissions array to prevent runtime errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Three issues from gemini-code-assist review + one real CI regression:
1. **C4 test regression** (4 failing Ubuntu CI jobs): conftest.py user3
had `permissions=["models:list", "models:read", "models:start"]`.
After C4 merged `models:start` + `models:stop` into a single
`models:write` scope (with alias bridge), user3's `models:start`
expands to `models:write` which covers BOTH launch AND terminate.
The tests `test_client_auth` / `test_async_client_auth` expected
user3's `terminate_model` to raise RuntimeError; after C4 it
succeeds → "DID NOT RAISE" failure.
Fix: update conftest user3 to use the new `models:write` scope name
directly, and remove the `with pytest.raises(RuntimeError):
terminate` assertions for user3 (both JWT and API key paths). The
"can launch but can't terminate" distinction is intentionally gone
per C4's merge design (see PR description breaking change). Admin
(user1) still terminates in the downstream flow, preserving the
`list_models() == 1` → `== 0` assertions.
2. **N+1 query in list_api_keys** (routes.py): admin path fetched
users one-by-one via `get_user_by_id`. Switched to a single
`list_users()` call when `is_admin` (admin sees all keys, so the
user set is large); non-admin path keeps N+1 (user set is
typically 1 — the caller themselves).
3. **JWT base64url decoding** (utils/jwt.js): `atob()` throws
DOMException on base64url strings (which contain `-` and `_` and
lack padding). Standard JWT payloads are base64url-encoded, so
direct `atob()` would silently fail token parsing in some
browsers. Fixed by replacing `-` → `+` and `_` → `/` before
`atob()`.
4. **Null-safety in apikey_management** (apikey_management/index.js):
`permissions.includes('admin')` throws TypeError if
`permissions` is null/undefined (e.g. during PermissionsContext
initial load before `/v1/users/me` resolves). Changed to
`permissions?.includes('admin')`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks @gemini-code-assist for the review — all 3 suggestions applied in db66e14, plus a real CI regression fix: 1. C4 test regression (4 failing Ubuntu CI jobs — real breakage, not flaky):
Fix: conftest user3 now uses `models:write` directly, and the `with pytest.raises(RuntimeError): terminate` assertions for user3 are removed (both JWT and API key paths). The "can launch but can't terminate" distinction is intentionally gone per C4's merge design (see PR description breaking change). Admin (user1) still terminates downstream, preserving the `list_models() == 1` → `== 0` assertions. 2. N+1 query in list_api_keys (routes.py): admin path now uses a single `list_users()` call instead of per-user `get_user_by_id`. Non-admin path keeps N+1 (user set is typically 1). 3. JWT base64url decoding (utils/jwt.js): `atob()` throws DOMException on base64url. Now normalizes `-`→`+` and `_`→`/` before decoding. 4. Null-safety (apikey_management/index.js): `permissions.includes('admin')` → `permissions?.includes('admin')` to handle the initial-load case where PermissionsContext hasn't resolved yet. Re: macOS / Windows CI — those jobs were still pending when the fix pushed; the Ubuntu jobs should now pass. The pre-existing flaky tests (test_log_rotation on macOS, test_whisper_mlx on metal, test_worker socket bind on Windows) are unrelated to this PR. |
|
Thanks for the detailed update and for resolving the CI regression. The approach of using |
|
Follow-up fix in 181d493: the N+1 fix from db66e14 introduced a mypy error that broke the lint CI job. Root cause: The local variable Fix: Renamed the else-branch variable to Lint CI should now pass. |
Adds `utils/jwt.js` with three helpers that the upcoming permission/scope
alignment series will build on:
- `parseTokenFromSession()` — reads JWT from `sessionStorage`, returns
`{username, scopes}`
- `hasPermission(scopes, scope)` — `admin` wildcard bypass + scope
membership check, mirroring the backend `if "admin" in token_scopes`
shortcut
- `isAdmin(scopes)` — convenience wrapper
No behavior change yet — pure additions. Subsequent PRs will wire these
into `MenuSide`, `router`, and `monitoring`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds three new permissions to the user management UI so the upcoming scope alignment series can wire backend routes to them: - `models:register` (in `models` group) - `logs:list` (new `logs` group) - `monitor:view` (new `monitor` group) Adds matching `permissions.*` i18n keys in en/zh/ja/ko locales. No behavior change yet — admins can now assign the new scopes, but no frontend or backend code consumes them until subsequent PRs land. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Splits menu items into two classes: - A-class (launch / running / register model): always visible, no route guard, backend 403 as fallback - B-class (cluster / monitoring / logs / audit / user mgmt / API key / security): visible only when the user has the required scope, with a `PermissionGate` redirect for direct URL access Also gates the two monitoring-page config buttons (settings gear + notConfigured placeholder button) behind `admin`, matching the existing `update_monitor_config` route scope. Menu visibility matrix: | Menu | Scope condition | |-------------------|--------------------------------------------------| | Launch model | (always) | | Running model | (always) | | Register model | (always) | | Cluster info | admin | | Monitoring | monitor:view | | Logs | esEnabled && logs:list | | Audit | admin | | User management | authAdvanced && users:manage | | API Key management| authAdvanced && (keys:create || keys:manage) | | Security | authAdvanced && admin | Routes for B-class pages are wrapped in `<PermissionGate requiredScope=...>` which redirects to `/` when the session lacks the scope. `admin` is a wildcard that passes any gate. A-class routes are intentionally NOT wrapped. Note: this PR uses JWT-snapshot scopes via `parseTokenFromSession()`. A follow-up PR will replace this with a live-read `PermissionsContext` backed by `/v1/users/me`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Aligns backend route scopes with the frontend permission list so the
two match 1:1. Adds a `SCOPE_ALIASES` bridge so existing tokens with
legacy scope names continue to work during the transition period.
Scope renames:
| Old scope | New scope | Affected routes |
|---------------------|-------------------|------------------------------------------|
| models:start | models:write | POST /v1/models/{uid}/stop, /cancel |
| models:stop | models:write | POST /v1/models/{uid}/terminate, etc. |
| models:add | models:register | POST /v1/model_registrations |
| models:unregister | models:register | DELETE /v1/model_registrations |
| (admin-only) | logs:list | GET /v1/cluster/logs, /context, /nodes |
`monitor:view` is a frontend-only scope (the monitoring page's data
dependencies `GET /v1/cluster/ui_config` and `GET /v1/cluster/logs` are
already covered by `monitor:view` menu gate and `logs:list` route scope
respectively).
The alias bridge lives in a new `xinference/api/oauth2/scope_aliases.py`
module imported by both `auth_service.py` modules. `_normalize_scopes`
expands legacy scopes to their new equivalents before the scope-by-scope
check, so a token carrying `models:start` passes a `models:write`
check.
The alias bridge is scheduled for removal in a future release; a
deprecation warning will be added in a follow-up PR once the warning
collection mechanism is in place.
BREAKING CHANGE: tokens carrying only the new scope names
(`models:write`, `models:register`, `logs:list`) issued by newer
deployments will NOT work on older deployments that still expect the
legacy names. The alias bridge is one-directional (legacy → new).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…handlers
The existing `_reject_permission_escalation` function only validates
Layer B (a caller cannot grant scopes they don't hold). It does NOT
validate Layer A: a non-admin caller with `users:manage` could delete
an admin account, change an admin's password and log in as them, or
disable the only admin and lock the deployment out.
This commit adds `_reject_admin_target_takeover(user_id)` which raises
403 when a non-admin caller tries to perform a sensitive write on an
admin user. Admin callers bypass the check entirely.
Wired into 4 handlers:
- `delete_user` (new guard)
- `change_password` (new guard)
- `update_user` enable/disable path (new guard) + permissions path
(new guard, in addition to existing Layer B)
- `update_user_permissions` (new guard, in addition to existing Layer B)
`create_user` does not need Layer A (the target user doesn't exist yet).
`create_api_key` does not need Layer A (API keys have no admin concept;
the model_permissions path is already covered by Layer B via
`_reject_permission_escalation` semantics — to be wired in a follow-up
for the `model_permissions` field specifically).
Test coverage: `test_oauth2_admin_target_takeover.py` parameterizes
across all 4 action types × {non-admin caller targets admin user → 403,
non-admin targets non-admin → ok, admin targets admin → ok, missing
user → ok (no existence leak), empty/None permissions → ok}.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…se64url Three fixes bundled (all from gemini review + C4 test regression on the original mega-PR xorbitsai#5133): 1. **C4 test regression**: conftest.py user3 had `permissions=["models:list", "models:read", "models:start"]`. After C4 merged `models:start` + `models:stop` into a single `models:write` scope (with alias bridge), user3's `models:start` expands to `models:write` which covers BOTH launch AND terminate. The tests `test_client_auth` / `test_async_client_auth` expected user3's `terminate_model` to raise `RuntimeError`; after C4 it succeeds. Fix: conftest user3 now uses `models:write` directly; the `with pytest.raises(RuntimeError): terminate` assertions for user3 are removed (both JWT and API key paths). Admin (user1) still terminates downstream. 2. **JWT base64url decoding** (utils/jwt.js): `atob()` throws DOMException on base64url strings. Now normalizes `-`→`+` and `_`→`/` before decoding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two independent fixes for the API key management flow (extracted from the original mega-PR's C7 commit, without the live-read changes that were dropped after maintainer feedback): 1. **owner_username field (G1)**: `list_api_keys` now batch-resolves `user_id → username` and adds `owner_username` to each key in the response. Non-admin callers (who can't GET /v1/admin/users) can now render the owner column without falling back to `#<id>`. `/v1/admin/users` stays admin-only because its response contains `permissions` / `enabled` / `must_change_password` fields. N+1 avoidance: admin path uses a single `list_users()` query; non-admin path keeps per-uid `get_user_by_id` (user set is typically 1 — the caller themselves). Frontend `apikey_management` owner column renderCell prefers `row.owner_username` over local `users[]` lookup. 2. **reveal_api_key scope: admin → keys:manage (J1)**: the frontend `canManageKeys` button guard already shows the reveal button to `keys:manage` users, but the backend route scope was still `admin`, causing 403 when `keys:manage` (non-admin) users clicked it. Reveal is a read operation; `keys:manage` users can already delete keys (more destructive), so blocking reveal was over-protection. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses maintainer @qinxuye's feedback on PR xorbitsai#5133: the frontend PermissionGate shows the API Key Management page to users holding either keys:create OR keys:manage, and the handler is_admin check treats both as admin-capable. But the GET routes were protected by Security(scopes=["keys:create"]) — AND-semantics — so a user with only keys:manage could reach the page but the initial GET /v1/admin/keys was rejected before the handler ran. Fix: add a require_keys_read dependency that accepts keys:create OR keys:manage (or admin wildcard). FastAPI's Security(scopes=[...]) is AND-only, so a custom dependency is needed for OR semantics. Apply to GET /v1/admin/keys (list) and GET /v1/admin/keys/{id} (get one). Write routes (POST create, PUT update, DELETE, GET reveal) are unchanged — they keep their existing single-scope Security because each write operation maps to exactly one scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
181d493 to
a40fbe9
Compare
m199369309
changed the title
|
@qinxuye Thanks for both reviews — valid points. This PR has been re-scoped to address them: Feedback 1 (live-read doesn't work at route layer): You're right — the `_get_caller_live_perms` helper and `/v1/users/me` + `PermissionsContext` only worked at the UI menu layer, not for actual API calls (`Security(scopes=[...])` still checks JWT snapshot). This created a worse UX than pre-PR (user sees a menu item that 403s on click). Action taken: Dropped C5 (`/v1/users/me` + `PermissionsContext`) and C7's live-read portion (`_get_caller_live_perms` + 3 handler switches) from this PR. The remaining C7 pieces (G1 `owner_username` + J1 `reveal` scope) are kept as independent improvements — they don't depend on live-read. Live-read design discussion moved to #5134 with four options (A: Security queries DB per request / B: drop live-read / C: shorten JWT + re-read on refresh / D: WebSocket push). Recommendation is C. Your input on direction would be appreciated. Feedback 2 (frontend/backend scope contract mismatch on read routes): Fixed in commit `a40fbe9e`. New `require_keys_read` dependency accepts `keys:create` OR `keys:manage` (FastAPI's `Security(scopes=[...])` is AND-only, so a custom dependency is needed for OR). Applied to GET `/v1/admin/keys` and GET `/v1/admin/keys/{id}`. Write routes unchanged (each maps to exactly one scope). PR now contains 8 commits (force-pushed to replace the original 7-commit mega-PR + 3 fix commits):
Note: force-push invalidated the previous inline comments, but the design feedback they raised is fully addressed above. CI is re-running on the new head. |
…WT decode Addresses maintainer @qinxuye's feedback on PR xorbitsai#5133: the custom require_keys_read dependency only called _get_current_user_from_token (which decodes the JWT payload) — it did not verify that the user still exists or is still enabled in the DB. A user with a keys:manage token could be disabled after login and still pass this dependency for GET /v1/admin/keys and GET /v1/admin/keys/{id} until the JWT expires. Fix: reuse Security(auth_service) as a sub-dependency so the full AdvancedAuthService.__call__ validation runs (JWT verify + user DB lookup + enabled check + audit log). Security(auth_service) with no scopes enforces authentication + user validity but not any specific scope. The OR check (keys:create || keys:manage || admin) is then performed manually on the JWT scopes — consistent with how Security(scopes=[...]) checks JWT scopes (not DB permissions). require_keys_read is now a closure inside register_advanced_auth_routes so it can reference the auth_service instance (which is only available after app initialization, not at module definition time). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@qinxuye Good catch — valid security regression. Pushed fix in d9f44df. Root cause: Fix: Implementation note: Pre-commit clean (black/flake8/isort/mypy/codespell). CI re-running on the new head. |
3 participants
Summary
Aligns the frontend permission list, backend route scopes, and menu/route gating so the three match 1:1. Adds an admin-target-takeover guard to prevent non-admin users from modifying admin accounts. Adds
owner_usernameto the API key list response so non-admin callers can render the owner column. Fixes thereveal_api_keyscope to match the frontend button guard.What changed vs the original mega-PR
Based on maintainer @qinxuye's feedback, this PR is a scoped-down version of the original 7-commit series. The live-read changes (C5
/v1/users/me+PermissionsContext, C7_get_caller_live_perms) are dropped because the live-read design only worked at the UI menu layer, not at the API route layer (Security(scopes=[...])still checks JWT snapshot). Live-read will be revisited in a separate follow-up — see the open issue linked in the comments.Commits
Background
Xinference's permission system had three structural issues:
This PR addresses all three. The alias bridge (commit 4) ensures existing tokens carrying legacy scope names keep working during the transition period; the bridge is scheduled for removal in a future release.
Tokens carrying only the new scope names (`models:write`, `models:register`, `logs:list`) issued by newer deployments will NOT work on older deployments that still expect the legacy names. The alias bridge is one-directional (legacy → new).
Admin default permissions now include `models:register`, `logs:list`, `monitor:view`.
Test plan
Out of scope (follow-up)
🤖 Generated with Claude Code