refactor: consolidate route auth for UI and API tokens#25473
refactor: consolidate route auth for UI and API tokens#25473ryan-crabbe-berri wants to merge 1 commit intomainfrom
Conversation
Unify UI and API token authorization through the shared RBAC path and backfill missing routes in role-based route lists.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR consolidates UI-token and API-token authorization by routing all tokens through the shared RBAC path, replacing the old role-agnostic
Confidence Score: 4/5Safe to merge if the team confirms no deployments use One P1 finding: deleting
|
| Filename | Overview |
|---|---|
| litellm/proxy/_types.py | Removes ui_routes enum member and backfills routes into RBAC lists; ui_routes deletion silently breaks JWT auth configs that reference it by name. |
| litellm/proxy/auth/auth_checks.py | Consolidates _is_allowed_route/_is_ui_route into a single _is_api_route_allowed that always applies RBAC; logic is correct and a security improvement. |
| tests/proxy_unit_tests/test_jwt.py | Mostly Black formatting; parametrize change from ["ui_routes"] to ["info_routes"] removes test coverage of the now-deleted config value. |
| tests/proxy_unit_tests/test_user_api_key_auth.py | Replaces _is_ui_route test with a more comprehensive _is_api_route_allowed parametrize covering RBAC roles; coverage is improved for all roles. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request] --> B{Token type?}
B -- "Before PR: team_id == litellm-dashboard" --> C[_is_ui_route check\nagainst ui_routes list\nrole-agnostic]
B -- "Before PR: API token" --> D[_is_api_route_allowed\nRBAC-based]
C -- "route in ui_routes" --> PASS1[✅ Allowed]
C -- "route not in ui_routes" --> D
B -- "After PR: ALL tokens" --> E[_is_api_route_allowed\nUnified RBAC path]
E --> F{_is_user_proxy_admin?}
F -- "Yes" --> PASS2[✅ Allowed]
F -- "No" --> G[non_proxy_admin_allowed_routes_check]
G --> H{Role?}
H -- "INTERNAL_USER" --> I[internal_user_routes\n+ spend_tracking_routes\n+ global_spend_tracking_routes]
H -- "INTERNAL_USER_VIEW_ONLY" --> J[internal_user_view_only_routes]
H -- "PROXY_ADMIN_VIEW_ONLY" --> K[admin_viewer_routes]
H -- "Org Admin" --> L[org_admin_allowed_routes]
I --> PASS3[✅ Allowed if route matches]
J --> PASS3
K --> PASS3
L --> PASS3
Reviews (1): Last reviewed commit: "refactor: consolidate route auth for UI ..." | Re-trigger Greptile
| internal_user_routes = ( | ||
| [ | ||
| "/global/spend/tags", | ||
| "/global/spend/keys", | ||
| "/global/spend/models", | ||
| "/global/spend/provider", | ||
| "/global/spend/end_users", | ||
| "/global/activity", | ||
| "/global/activity/model", | ||
| "/global/activity/cache_hits", | ||
| "/v1/models/{model_id}", | ||
| "/models/{model_id}", | ||
| "/guardrails/list", | ||
| "/v2/guardrails/list", | ||
| ] | ||
| + spend_tracking_routes | ||
| + global_spend_tracking_routes | ||
| + key_management_routes | ||
| ) |
There was a problem hiding this comment.
ui_routes removal breaks existing JWT auth configurations
Deleting LiteLLMRoutes.ui_routes from the enum is backward-incompatible for any JWT auth configuration that lists "ui_routes" in admin_allowed_routes (or team_allowed_routes). That was a supported config — the test in test_jwt.py previously exercised ["ui_routes"] as a valid admin_allowed_routes value.
After this deletion, _allowed_routes_check finds that "ui_routes" is not in LiteLLMRoutes.__members__, so it falls through to the elif allowed_route == user_route branch and treats "ui_routes" as a literal route path. That never matches a real request path, so all routes those admins previously accessed via this config are silently blocked.
If you need to deprecate ui_routes, consider keeping the enum member as a thin alias over info_routes (or whatever the closest equivalent is) rather than deleting it outright, so existing configs continue to work.
Rule Used: What: avoid backwards-incompatible changes without... (source)
|
|
||
|
|
||
| @pytest.mark.parametrize("admin_allowed_routes", [None, ["ui_routes"]]) | ||
| @pytest.mark.parametrize("admin_allowed_routes", [None, ["info_routes"]]) |
There was a problem hiding this comment.
Test change removes coverage of the breaking config case
Swapping ["ui_routes"] for ["info_routes"] in this parametrize is necessary since the enum member is gone, but it also means no test exercises what happens when an existing deployment has admin_allowed_routes: ["ui_routes"] in its JWT auth config — which, per the analysis above, now silently grants access to nothing.
If the intent is to treat the removal as an intentional breaking change, this is fine; but if the goal is a transparent migration, a test (or at minimum a changelog entry) documenting the old config value is now inert would be valuable.
Rule Used: What: Flag any modifications to existing tests and... (source)
Summary
Test plan
test_user_api_key_auth.py,test_jwt.py)