[Fix] RBAC: Default-Allow GET for Admin Viewer + Models Tab Alignment

Root cause: admin_viewer_routes was an explicit allowlist, so every newly-added
GET endpoint anywhere in the codebase silently 403'd for admin viewer until
someone remembered to add it. We had whacked /spend/logs/ui, /customer/list,
/guardrails/list, /policies/attachments/list, /invitation/info, and several
others in serial — but the next round still surfaced /in_product_nudges,
/health/latest, /credentials, /v1/mcp/network/client-ip, /claude-code/plugins,
/policy/templates. This pattern keeps repeating because the model is wrong.

Structural fix in `_check_proxy_admin_viewer_access`:
  - Default-allow safe HTTP methods (GET / HEAD / OPTIONS) on any
    non-inference route. Admin Viewer's principle is read parity with
    Proxy Admin; HTTP semantics already mark GET as side-effect-free, so
    using the method as the allow signal is the correct primitive.
  - Unsafe methods (POST/PUT/PATCH/DELETE) still go through the existing
    explicit allowlists + the hard-blocked write set
    (/user/new, /team/new, /key/generate, …).
  - LLM/inference routes still 403 (cost-incurring).

The existing admin_viewer_routes list is retained as a backstop for the
small set of routes implemented as POST but semantically read (e.g.
/spend/calculate). Adding new GET endpoints no longer requires touching
this list.

Models page tab/panel off-by-one (UI bug for Admin Viewer):
  Tremor's TabList filters falsy children but TabPanels does not, so
  conditionally hiding "Add Model" with `{!shouldHideAddModelTab && ...}`
  left a phantom panel slot — clicking "LLM Credentials" showed nothing,
  and clicking "Pass-Through Endpoints" showed the credentials panel.
  Refactor to a single source-of-truth `visibleTabs` array; tab and
  panel indices now can never desync.

Tests:
  - 12 parametrized tests covering the 6 user-reported endpoints + 4
    hypothetical-future endpoints + 2 already-fixed ones, all asserting
    Admin Viewer GET succeeds via the default-allow path (no allowlist
    entry needed).
  - 5 parametrized tests for POST writes still 403'ing
    (random-future-write, /user/new, /team/new, /key/generate, /model/new).
  - All 207 existing route_checks tests still pass — backward-compatible.
This commit is contained in:
Yuneng Jiang 2026-04-29 23:19:45 -07:00
parent 2fa6c60124
commit 00145f91a8
No known key found for this signature in database
4 changed files with 364 additions and 155 deletions

View file

@ -716,13 +716,18 @@ class LiteLLMRoutes(enum.Enum):
"/organization/member_delete",
]
# Routes accessible by Admin Viewer (read-only admin access)
# Routes accessible by Admin Viewer (read-only admin access).
#
# Admin Viewer follows a read-parity-with-Proxy-Admin rule: anything Proxy
# Admin can read/list/get, Admin Viewer can too (no writes, no cost-incurring
# actions). When extending this list, the only valid exclusions are write
# endpoints and cost-incurring endpoints (e.g. /chat/completions, the
# Playground). Pure GET/list/info endpoints belong here.
# actions).
#
# NOTE: This list is no longer the primary mechanism for granting access —
# `_check_proxy_admin_viewer_access()` in route_checks.py default-allows
# any safe HTTP method (GET/HEAD/OPTIONS) on non-inference routes. This
# list now matters only for non-GET routes that are semantically reads
# (e.g. POST /spend/calculate). Adding a new GET endpoint does not require
# updating this list — the default-allow behavior covers it automatically.
admin_viewer_routes = (
[
"/user/list",

View file

@ -202,6 +202,7 @@ class RouteChecks:
route=route,
_user_role=_user_role,
request_data=request_data,
request=request,
)
elif (
_user_role == LitellmUserRoles.INTERNAL_USER.value
@ -596,14 +597,66 @@ class RouteChecks:
return True
return False
# HTTP methods that are intrinsically read-only and therefore safe to
# default-allow for PROXY_ADMIN_VIEW_ONLY. Anything else (POST/PUT/PATCH/
# DELETE) is treated as a write attempt and goes through the explicit
# write-allowlist below.
_SAFE_HTTP_METHODS = frozenset({"GET", "HEAD", "OPTIONS"})
# Explicit write routes that PROXY_ADMIN_VIEW_ONLY must NEVER call. The
# role-principle is "no writes, ever" — the management_routes list is the
# authoritative source for which non-llm routes are writes; we just need
# to filter out the read endpoints (info / list) that share the prefix.
# A cleaner approach is to denylist by HTTP verb (POST/PUT/PATCH/DELETE);
# this block stays as a backstop in case a write is implemented as GET.
_ADMIN_VIEWER_BLOCKED_WRITE_ROUTES = frozenset(
[
"/user/new",
"/user/delete",
"/user/bulk_update",
"/team/new",
"/team/update",
"/team/delete",
"/model/new",
"/model/update",
"/model/delete",
"/key/generate",
"/key/delete",
"/key/update",
"/key/regenerate",
"/key/service-account/generate",
"/key/block",
"/key/unblock",
]
)
@staticmethod
def _check_proxy_admin_viewer_access(
route: str,
_user_role: str,
request_data: dict,
request: Optional[Request] = None,
) -> None:
"""
Check access for PROXY_ADMIN_VIEW_ONLY role
Check access for PROXY_ADMIN_VIEW_ONLY role.
Admin Viewer follows a read-parity-with-Proxy-Admin rule: anything Proxy
Admin can read/list/get, Admin Viewer can read/list/get. The only
exclusions are cost-incurring inference routes (Playground, /chat/
completions, etc.) and any state-mutating request.
Implementation:
1. LLM/inference routes → 403 (cost-incurring).
2. Safe HTTP method (GET/HEAD/OPTIONS) → allow by default. This is
the read-parity guarantee — every new GET endpoint added anywhere
in the codebase is automatically readable by Admin Viewer
without needing to remember to add it to an allowlist.
3. Unsafe HTTP method (POST/PUT/PATCH/DELETE):
- Allow `/user/update` only when restricted to user_email/password.
- Block all explicit writes in `_ADMIN_VIEWER_BLOCKED_WRITE_ROUTES`.
- Otherwise allow only if the route is in admin_viewer_routes /
global_spend_tracking_routes (legacy explicit-allow set).
- Else 403.
"""
if RouteChecks.is_llm_api_route(route=route):
raise HTTPException(
@ -611,65 +664,58 @@ class RouteChecks:
detail=f"user not allowed to access this OpenAI routes, role= {_user_role}",
)
# Check if this is a write operation on management routes
if RouteChecks.check_route_access(
route=route, allowed_routes=LiteLLMRoutes.management_routes.value
):
# For management routes, only allow read operations or specific allowed updates
if route == "/user/update":
# Check the Request params are valid for PROXY_ADMIN_VIEW_ONLY
if request_data is not None and isinstance(request_data, dict):
_params_updated = request_data.keys()
for param in _params_updated:
if param not in ["user_email", "password"]:
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"user not allowed to access this route, role= {_user_role}. Trying to access: {route} and updating invalid param: {param}. only user_email and password can be updated",
)
elif (
route
in [
"/user/new",
"/user/delete",
"/user/bulk_update",
"/team/new",
"/team/update",
"/team/delete",
"/model/new",
"/model/update",
"/model/delete",
"/key/generate",
"/key/delete",
"/key/update",
"/key/regenerate",
"/key/service-account/generate",
"/key/block",
"/key/unblock",
]
or route.startswith("/key/")
and route.endswith("/regenerate")
):
# Block write operations for PROXY_ADMIN_VIEW_ONLY
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"user not allowed to access this route, role= {_user_role}. Trying to access: {route}",
)
# Allow read operations on management routes (like /user/info, /team/info, /model/info)
method = request.method.upper() if request is not None else "GET"
is_safe_method = method in RouteChecks._SAFE_HTTP_METHODS
# ── Safe HTTP method: default-allow ──────────────────────────────
if is_safe_method:
return
elif RouteChecks.check_route_access(
route=route, allowed_routes=LiteLLMRoutes.admin_viewer_routes.value
):
# Allow access to admin viewer routes (read-only admin endpoints)
# ── Unsafe HTTP method: explicit checks ──────────────────────────
# Allow `/user/update` for self-service email / password change.
if route == "/user/update":
if request_data is not None and isinstance(request_data, dict):
for param in request_data.keys():
if param not in ["user_email", "password"]:
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=(
f"user not allowed to access this route, role= {_user_role}. "
f"Trying to access: {route} and updating invalid param: {param}. "
"only user_email and password can be updated"
),
)
return
elif RouteChecks.check_route_access(
route=route, allowed_routes=LiteLLMRoutes.global_spend_tracking_routes.value
# Hard-block known write routes regardless of HTTP method (defensive
# — these are POSTs in practice, but pinning them here protects
# against future GET-shaped writes).
if route in RouteChecks._ADMIN_VIEWER_BLOCKED_WRITE_ROUTES or (
route.startswith("/key/") and route.endswith("/regenerate")
):
# Allow access to global spend tracking routes (read-only spend endpoints)
# proxy_admin_viewer role description: "view all keys, view all spend"
return
else:
# For other routes, block access
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"user not allowed to access this route, role= {_user_role}. Trying to access: {route}",
)
# Legacy explicit-allow sets (kept for routes that are POST but
# semantically read-only, e.g. /spend/calculate).
if RouteChecks.check_route_access(
route=route, allowed_routes=LiteLLMRoutes.admin_viewer_routes.value
):
return
if RouteChecks.check_route_access(
route=route, allowed_routes=LiteLLMRoutes.global_spend_tracking_routes.value
):
return
if RouteChecks.check_route_access(
route=route, allowed_routes=LiteLLMRoutes.management_routes.value
):
# On management routes, allow non-blocked writes (e.g. read-only
# info/list endpoints implemented as POST).
return
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"user not allowed to access this route, role= {_user_role}. Trying to access: {route}",
)

View file

@ -1373,6 +1373,117 @@ def test_proxy_admin_viewer_can_access_settings_read_endpoints(route):
)
# ── Admin Viewer parity: default-allow GET semantics ─────────────────────────
#
# The route-check layer is structured to default-allow safe HTTP methods
# (GET / HEAD / OPTIONS) for PROXY_ADMIN_VIEW_ONLY. This eliminates the
# whack-a-mole where every newly-added GET endpoint silently 403'd until
# someone remembered to add it to admin_viewer_routes.
#
# These tests pin the new contract:
# - Any GET endpoint not on the LLM/inference path is readable.
# - Any unsafe method (POST/PUT/PATCH/DELETE) outside the explicit allow
# sets is still 403.
# Routes the user reported as broken in production — they're in disparate
# corners of the codebase and represent the long tail of GETs we'd otherwise
# need to enumerate manually. Default-allow makes them all work.
ADMIN_VIEWER_REPORTED_GET_ROUTES = [
"/in_product_nudges",
"/health/latest",
"/credentials",
"/v1/mcp/network/client-ip",
"/claude-code/plugins",
"/policy/templates",
# Routes we already had to enumerate manually (regression coverage).
"/spend/logs/ui",
"/customer/list",
"/guardrails/list",
"/policies/attachments/list",
# Hypothetical future GETs — must not require an allowlist entry.
"/some/future/read/endpoint",
"/another/admin-tool/status",
]
@pytest.mark.parametrize("route", ADMIN_VIEWER_REPORTED_GET_ROUTES)
def test_proxy_admin_viewer_default_allows_any_get(route):
"""
PROXY_ADMIN_VIEW_ONLY must be able to GET any non-inference endpoint.
This is a structural guarantee: the route-check defaults to allow for
safe HTTP methods so we don't have to maintain an explicit allowlist.
"""
user_obj = LiteLLM_UserTable(
user_id="viewer_user",
user_email="viewer@example.com",
user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
)
valid_token = UserAPIKeyAuth(
user_id="viewer_user",
user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
)
request = MagicMock(spec=Request)
request.method = "GET"
request.query_params = {}
request.url = MagicMock()
request.url.path = route
try:
RouteChecks.non_proxy_admin_allowed_routes_check(
user_obj=user_obj,
_user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
route=route,
request=request,
valid_token=valid_token,
request_data={},
)
except Exception as e:
pytest.fail(f"proxy_admin_viewer GET should default-allow {route!r}. Got: {e}")
@pytest.mark.parametrize(
"route",
[
# Random path that isn't in any allowlist — POST must still 403.
"/some/future/write/endpoint",
# Hard-blocked write routes.
"/user/new",
"/team/new",
"/key/generate",
"/model/new",
],
)
def test_proxy_admin_viewer_post_blocked_outside_allowlists(route):
"""
Default-allow only applies to safe HTTP methods. POST/PUT/PATCH/DELETE
on a route not in any allow set must still 403.
"""
user_obj = LiteLLM_UserTable(
user_id="viewer_user",
user_email="viewer@example.com",
user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
)
valid_token = UserAPIKeyAuth(
user_id="viewer_user",
user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
)
request = MagicMock(spec=Request)
request.method = "POST"
request.query_params = {}
with pytest.raises(HTTPException) as exc_info:
RouteChecks.non_proxy_admin_allowed_routes_check(
user_obj=user_obj,
_user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY.value,
route=route,
request=request,
valid_token=valid_token,
request_data={},
)
assert exc_info.value.status_code == 403
class TestModelsRouteExemptFromDisableLLMEndpoints:
"""
Test that /models and /v1/models are exempt from DISABLE_LLM_API_ENDPOINTS.

View file

@ -367,102 +367,149 @@ const ModelsAndEndpointsView: React.FC<ModelDashboardProps> = ({ premiumUser, te
modelAccessGroups={availableModelAccessGroups}
/>
) : (
<TabGroup index={selectedTabIndex} onIndexChange={setSelectedTabIndex} className="gap-2 h-[75vh] w-full ">
<TabList className="flex justify-between mt-2 w-full items-center">
<div className="flex">
{all_admin_roles.includes(userRole) ? <Tab>All Models</Tab> : <Tab>Your Models</Tab>}
{!shouldHideAddModelTab && <Tab>Add Model</Tab>}
{all_admin_roles.includes(userRole) && <Tab>LLM Credentials</Tab>}
{all_admin_roles.includes(userRole) && <Tab>Pass-Through Endpoints</Tab>}
{all_admin_roles.includes(userRole) && <Tab>Health Status</Tab>}
{all_admin_roles.includes(userRole) && <Tab>Model Retry Settings</Tab>}
{all_admin_roles.includes(userRole) && <Tab>Model Group Alias</Tab>}
{all_admin_roles.includes(userRole) && <Tab>Price Data Reload</Tab>}
</div>
<div className="flex items-center space-x-2 self-center">
{lastRefreshed && <span className="text-xs text-gray-500">Last Refreshed: {lastRefreshed}</span>}
<Icon
icon={RefreshIcon}
variant="shadow"
size="xs"
className="cursor-pointer"
onClick={handleRefreshClick}
/>
</div>
</TabList>
<TabPanels>
<AllModelsTab
selectedModelGroup={selectedModelGroup}
setSelectedModelGroup={setSelectedModelGroup}
availableModelGroups={availableModelGroups}
availableModelAccessGroups={availableModelAccessGroups}
setSelectedModelId={setSelectedModelId}
setSelectedTeamId={setSelectedTeamId}
/>
{!shouldHideAddModelTab && (
<TabPanel className="h-full">
<AddModelTab
form={addModelForm}
handleOk={handleOk}
selectedProvider={selectedProvider}
setSelectedProvider={setSelectedProvider}
providerModels={providerModels}
setProviderModelsFn={setProviderModelsFn}
getPlaceholder={getPlaceholder}
uploadProps={uploadProps}
showAdvancedSettings={showAdvancedSettings}
setShowAdvancedSettings={setShowAdvancedSettings}
teams={teams}
credentials={credentialsList}
accessToken={accessToken}
userRole={userRole}
(() => {
// Build a single source-of-truth list of {tab, panel} pairs.
// Conditionally-hidden tabs (e.g. "Add Model" for non-admin) get
// filtered out as a unit so tab indices and panel indices can
// never drift apart — Tremor's TabList and TabPanels filter
// falsy children inconsistently, which previously caused
// "click LLM Credentials, see nothing" for Admin Viewer.
const isAdmin = all_admin_roles.includes(userRole);
const visibleTabs: Array<{ tab: React.ReactElement; panel: React.ReactElement }> = [
{
tab: <Tab key="all-models">{isAdmin ? "All Models" : "Your Models"}</Tab>,
panel: (
<AllModelsTab
key="all-models"
selectedModelGroup={selectedModelGroup}
setSelectedModelGroup={setSelectedModelGroup}
availableModelGroups={availableModelGroups}
availableModelAccessGroups={availableModelAccessGroups}
setSelectedModelId={setSelectedModelId}
setSelectedTeamId={setSelectedTeamId}
/>
</TabPanel>
)}
<TabPanel>
<CredentialsPanel uploadProps={uploadProps} />
</TabPanel>
<TabPanel>
<PassThroughSettings
accessToken={accessToken}
userRole={userRole}
userID={userID}
modelData={processedModelData}
premiumUser={premiumUser}
/>
</TabPanel>
<TabPanel>
<HealthCheckComponent
accessToken={accessToken}
modelData={processedModelData}
all_models_on_proxy={allModelIdsOnProxy}
getDisplayModelName={getDisplayModelName}
setSelectedModelId={setSelectedModelId}
teams={teams}
/>
</TabPanel>
<ModelRetrySettingsTab
selectedModelGroup={selectedModelGroup}
setSelectedModelGroup={setSelectedModelGroup}
availableModelGroups={availableModelGroups}
globalRetryPolicy={globalRetryPolicy}
setGlobalRetryPolicy={setGlobalRetryPolicy}
defaultRetry={defaultRetry}
modelGroupRetryPolicy={modelGroupRetryPolicy}
setModelGroupRetryPolicy={setModelGroupRetryPolicy}
handleSaveRetrySettings={handleSaveRetrySettings}
/>
<TabPanel>
<ModelGroupAliasSettings
accessToken={accessToken}
initialModelGroupAlias={modelGroupAlias}
onAliasUpdate={setModelGroupAlias}
/>
</TabPanel>
<PriceDataManagementTab />
</TabPanels>
</TabGroup>
),
},
];
if (!shouldHideAddModelTab) {
visibleTabs.push({
tab: <Tab key="add-model">Add Model</Tab>,
panel: (
<TabPanel key="add-model" className="h-full">
<AddModelTab
form={addModelForm}
handleOk={handleOk}
selectedProvider={selectedProvider}
setSelectedProvider={setSelectedProvider}
providerModels={providerModels}
setProviderModelsFn={setProviderModelsFn}
getPlaceholder={getPlaceholder}
uploadProps={uploadProps}
showAdvancedSettings={showAdvancedSettings}
setShowAdvancedSettings={setShowAdvancedSettings}
teams={teams}
credentials={credentialsList}
accessToken={accessToken}
userRole={userRole}
/>
</TabPanel>
),
});
}
if (isAdmin) {
visibleTabs.push(
{
tab: <Tab key="llm-credentials">LLM Credentials</Tab>,
panel: (
<TabPanel key="llm-credentials">
<CredentialsPanel uploadProps={uploadProps} />
</TabPanel>
),
},
{
tab: <Tab key="pass-through">Pass-Through Endpoints</Tab>,
panel: (
<TabPanel key="pass-through">
<PassThroughSettings
accessToken={accessToken}
userRole={userRole}
userID={userID}
modelData={processedModelData}
premiumUser={premiumUser}
/>
</TabPanel>
),
},
{
tab: <Tab key="health-status">Health Status</Tab>,
panel: (
<TabPanel key="health-status">
<HealthCheckComponent
accessToken={accessToken}
modelData={processedModelData}
all_models_on_proxy={allModelIdsOnProxy}
getDisplayModelName={getDisplayModelName}
setSelectedModelId={setSelectedModelId}
teams={teams}
/>
</TabPanel>
),
},
{
tab: <Tab key="model-retry-settings">Model Retry Settings</Tab>,
panel: (
<ModelRetrySettingsTab
key="model-retry-settings"
selectedModelGroup={selectedModelGroup}
setSelectedModelGroup={setSelectedModelGroup}
availableModelGroups={availableModelGroups}
globalRetryPolicy={globalRetryPolicy}
setGlobalRetryPolicy={setGlobalRetryPolicy}
defaultRetry={defaultRetry}
modelGroupRetryPolicy={modelGroupRetryPolicy}
setModelGroupRetryPolicy={setModelGroupRetryPolicy}
handleSaveRetrySettings={handleSaveRetrySettings}
/>
),
},
{
tab: <Tab key="model-group-alias">Model Group Alias</Tab>,
panel: (
<TabPanel key="model-group-alias">
<ModelGroupAliasSettings
accessToken={accessToken}
initialModelGroupAlias={modelGroupAlias}
onAliasUpdate={setModelGroupAlias}
/>
</TabPanel>
),
},
{
tab: <Tab key="price-data-reload">Price Data Reload</Tab>,
panel: <PriceDataManagementTab key="price-data-reload" />,
},
);
}
return (
<TabGroup index={selectedTabIndex} onIndexChange={setSelectedTabIndex} className="gap-2 h-[75vh] w-full ">
<TabList className="flex justify-between mt-2 w-full items-center">
<div className="flex">{visibleTabs.map((t) => t.tab)}</div>
<div className="flex items-center space-x-2 self-center">
{lastRefreshed && <span className="text-xs text-gray-500">Last Refreshed: {lastRefreshed}</span>}
<Icon
icon={RefreshIcon}
variant="shadow"
size="xs"
className="cursor-pointer"
onClick={handleRefreshClick}
/>
</div>
</TabList>
<TabPanels>{visibleTabs.map((t) => t.panel)}</TabPanels>
</TabGroup>
);
})()
)}
</Col>
</Grid>