fix(proxy): allow proxy_admin_viewer to list all API keys - #27598
Conversation
validate_key_list_check() only allowed PROXY_ADMIN to bypass the user_id requirement. PROXY_ADMIN_VIEW_ONLY users hit the user_id check, which throws 403 because viewer accounts typically have no associated user_id. This blocked the /ui/?page=api-keys page entirely for Admin Viewer users. Add PROXY_ADMIN_VIEW_ONLY to the role check so viewers can list keys (read-only parity with PROXY_ADMIN). Fixes BerriAI#26689
|
@greptileai review |
Greptile SummaryThis PR fixes a permission gap where
Confidence Score: 5/5Safe to merge — minimal, targeted read-path fix with no write-access implications. The change adds a read-only viewer role to a listing gate that already treats both admin roles identically elsewhere in the same endpoint, and is covered by a new mock test. No existing tests are touched, no write paths are affected. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/key_management_endpoints.py | Adds PROXY_ADMIN_VIEW_ONLY to the role bypass in validate_key_list_check, consistent with existing handling of that role at line 4712–4715 in the same function's caller |
| tests/test_litellm/proxy/management_endpoints/test_key_management_endpoints.py | Adds a new mock-only test for the viewer role fix; no existing tests are modified or weakened |
Reviews (2): Last reviewed commit: "fix(proxy): allow proxy_admin_viewer to ..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| if user_api_key_dict.user_role == LitellmUserRoles.PROXY_ADMIN.value: | ||
| if user_api_key_dict.user_role in [ | ||
| LitellmUserRoles.PROXY_ADMIN.value, | ||
| LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value, |
There was a problem hiding this comment.
High: Bearer-equivalent key hashes exposed to view-only admins
/key/list returns token values from _list_key_helper by default, and _hash_token_if_needed() leaves non-sk- values unchanged during auth lookup. A proxy_admin_viewer can now list every stored key hash and use one directly as Authorization: Bearer <hash> to make requests as that key, bypassing the role's LLM-route restrictions.
Keep the all-key listing for this role only if the response no longer includes bearer-equivalent token hashes, or change authentication so stored hashes are not accepted as presented bearer credentials.
There was a problem hiding this comment.
This is not a new concern introduced by this PR. The /key/list endpoint returns SHA-256 hashed tokens to PROXY_ADMIN users today -- adding PROXY_ADMIN_VIEW_ONLY grants the same visibility that full admins already have. The viewer role is designed to have read-only parity with admin. If token hash exposure in /key/list responses is a concern, it applies to the existing PROXY_ADMIN path equally and should be addressed separately.
High: Admin viewers can obtain bearer-equivalent key hashesThis PR lets Status: 1 new · 1 open |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Summary
PROXY_ADMIN_VIEW_ONLYto the role check invalidate_key_list_check()so Admin Viewer users can access/key/listPROXY_ADMINto bypass theuser_idrequirementFixes #26689
Test plan
test_validate_key_list_check_proxy_admin_viewer-- verifiesPROXY_ADMIN_VIEW_ONLYwithuser_id=NonereturnsNone(allowed)test_validate_key_list_check_proxy_adminstill passes