Skip to content

fix: let tenant admins manage models of their own tenant #4043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeffwu-1999 merged 1 commit into from
Sep 29, 2026
Merged

fix: let tenant admins manage models of their own tenant #4043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 32 additions & 16 deletions backend/apps/model_managment_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
Authorization: Mutating endpoints require RBAC permissions (model:create /
model:update / model:delete) via ``permissions.depends.require``; read endpoints
require ``model:read``. Cross-tenant ``/manage/*`` endpoints additionally
require the SU role. Identity is resolved from the bearer token into a
require the SU role, or the ADMIN role when the targeted tenant is the
caller's own. Identity is resolved from the bearer token into a
``CurrentUser`` and propagated as ``user_id`` / ``tenant_id`` to services.
"""

Expand Down Expand Up @@ -77,9 +78,11 @@
MODEL_READ_PERMISSION = "model:read"
MODEL_UPDATE_PERMISSION = "model:update"
MODEL_DELETE_PERMISSION = "model:delete"
# Cross-tenant manage endpoints are SU-only; ADMIN shares the same MODEL seeds
# so permission strings cannot separate them.
_MANAGE_ALLOWED_ROLES = ("SU",)
# Roles allowed on the cross-tenant /manage/* endpoints. ADMIN shares the same
# MODEL permission seeds as SU, so permission strings cannot separate the two
# and the role itself must be checked. ADMIN is scoped to its own tenant by
# ``_require_manage_scope``; SU may target any tenant.
_MANAGE_ALLOWED_ROLES = ("SU", "ADMIN")

# Model Catalog loader (with graceful fallback)
try:
Expand Down Expand Up @@ -147,12 +150,25 @@ def _log_safe(value: Any) -> str:
return _LOG_UNSAFE_CHARS.sub("", str(value))


def _require_manage_role(current_user: CurrentUser) -> None:
"""Restrict cross-tenant manage endpoints to super admins."""
if current_user.normalized_role not in _MANAGE_ALLOWED_ROLES:
def _require_manage_scope(current_user: CurrentUser, target_tenant_id: str) -> None:
"""Authorize a /manage/* call against the tenant it targets.

SU may manage any tenant. ADMIN may manage only the tenant its token
belongs to -- the tenant-resource page always passes the caller's own
tenant_id, so restricting ADMIN outright would break tenant admins
managing their own models while blocking no cross-tenant access. Any
other role, or an ADMIN naming a foreign tenant, is rejected.
"""
role = current_user.normalized_role
if role not in _MANAGE_ALLOWED_ROLES:
raise HTTPException(
status_code=HTTPStatus.FORBIDDEN,
detail="This operation requires SU or tenant ADMIN role",
)
if role != "SU" and target_tenant_id != current_user.tenant_id:
raise HTTPException(
status_code=HTTPStatus.FORBIDDEN,
detail="This operation requires SU role",
detail="Tenant admins may only manage models of their own tenant",
)


Expand Down Expand Up @@ -716,7 +732,7 @@ async def manage_check_model_health(
Returns:
Connectivity check result with updated status.
"""
_require_manage_role(current_user)
_require_manage_scope(current_user, request.tenant_id)
try:
logger.debug(
f"Start to check model connectivity for tenant, user_id: {current_user.user_id}, "
Expand Down Expand Up @@ -760,7 +776,7 @@ async def manage_create_model(
Returns:
Success message on successful creation.
"""
_require_manage_role(current_user)
_require_manage_scope(current_user, request.tenant_id)
try:
user_id = current_user.user_id
logger.debug(
Expand Down Expand Up @@ -811,7 +827,7 @@ async def manage_update_model(
Returns:
Success message on successful update.
"""
_require_manage_role(current_user)
_require_manage_scope(current_user, request.tenant_id)
try:
user_id = current_user.user_id
logger.debug(
Expand Down Expand Up @@ -863,7 +879,7 @@ async def manage_delete_model(
Returns:
Success message with deleted model name.
"""
_require_manage_role(current_user)
_require_manage_scope(current_user, request.tenant_id)
try:
user_id = current_user.user_id
logger.debug(
Expand Down Expand Up @@ -908,7 +924,7 @@ async def manage_batch_create_models(
Returns:
Success message on completion.
"""
_require_manage_role(current_user)
_require_manage_scope(current_user, request.tenant_id)
try:
user_id = current_user.user_id
logger.debug(
Expand Down Expand Up @@ -960,7 +976,7 @@ async def manage_list_models(
Returns:
Paginated model list for the specified tenant.
"""
_require_manage_role(current_user)
_require_manage_scope(current_user, request.tenant_id)
try:
logger.debug(
f"Start to list models for tenant, user_id: {current_user.user_id}, target_tenant_id: {request.tenant_id}, "
Expand Down Expand Up @@ -1001,7 +1017,7 @@ async def manage_list_provider_models(
Returns:
List of available provider models for the specified tenant.
"""
_require_manage_role(current_user)
_require_manage_scope(current_user, request.tenant_id)
try:
logger.debug(
f"Start to list provider models for tenant, user_id: {current_user.user_id}, target_tenant_id: {request.tenant_id}, "
Expand Down Expand Up @@ -1039,7 +1055,7 @@ async def manage_create_provider_models(
Returns:
List of available provider models for the specified tenant.
"""
_require_manage_role(current_user)
_require_manage_scope(current_user, request.tenant_id)
try:
logger.debug(
f"Start to create provider models for tenant, user_id: {current_user.user_id}, target_tenant_id: {request.tenant_id}, "
Expand Down
87 changes: 83 additions & 4 deletions test/backend/app/test_model_rbac.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

Verifies that mutating /model/* endpoints reject the DEV role (which only
holds model:read), read endpoints stay accessible to DEV, and the
cross-tenant /manage/* endpoints are restricted to SU.
/manage/* endpoints accept SU for any tenant plus ADMIN for its own tenant
only.
"""

import sys
Expand Down Expand Up @@ -185,9 +186,9 @@ async def test_admin_can_create_model(admin_client, mocker):


@pytest.mark.asyncio
async def test_admin_cannot_access_manage_endpoints(admin_client, mocker):
"""ADMIN shares the SU model seeds, so manage/* must fall back to the
SU role whitelist."""
async def test_admin_cannot_access_foreign_tenant_manage_endpoints(admin_client, mocker):
"""ADMIN shares the SU model seeds, so cross-tenant manage/* calls must be
rejected by the role+tenant scope check rather than by permission strings."""
mocker.patch(
'backend.apps.model_managment_app.list_models_for_admin',
return_value={"models": [], "total": 0},
Expand All @@ -200,8 +201,86 @@ async def test_admin_cannot_access_manage_endpoints(admin_client, mocker):
assert response.status_code == HTTPStatus.FORBIDDEN


@pytest.mark.asyncio
async def test_admin_cannot_mutate_foreign_tenant_models(admin_client, mocker):
"""The own-tenant allowance must not extend to mutating endpoints either."""
mock_update = mocker.patch(
'backend.apps.model_managment_app.update_single_model_for_tenant',
return_value=None,
)
response = admin_client.post(
"/model/manage/update",
json={
"tenant_id": "other_tenant",
"current_display_name": "m",
"model_name": "m2",
},
headers=auth_header,
)
assert response.status_code == HTTPStatus.FORBIDDEN
mock_update.assert_not_awaited()


@pytest.mark.asyncio
async def test_admin_can_list_own_tenant_models(admin_client, mocker):
"""Tenant admins manage their own tenant via /resource-manage, which always
passes the caller's own tenant_id. Regression: manage/list used to be
SU-only, so the Models tab rendered an empty table for ADMIN."""
mock_list = mocker.patch(
'backend.apps.model_managment_app.list_models_for_admin',
return_value={"models": [], "total": 0},
)
response = admin_client.post(
"/model/manage/list",
json={"tenant_id": "rbac_tenant", "page": 1, "page_size": 10},
headers=auth_header,
)
assert response.status_code == HTTPStatus.OK
mock_list.assert_awaited_once()
# The own-tenant id must be the one forwarded to the service.
assert mock_list.await_args.args[0] == "rbac_tenant"


@pytest.mark.asyncio
async def test_admin_can_mutate_own_tenant_models(admin_client, mocker):
"""Create/update/delete of the ADMIN's own tenant models stay available."""
mock_update = mocker.patch(
'backend.apps.model_managment_app.update_single_model_for_tenant',
return_value=None,
)
response = admin_client.post(
"/model/manage/update",
json={
"tenant_id": "rbac_tenant",
"current_display_name": "m",
"model_name": "m2",
},
headers=auth_header,
)
assert response.status_code == HTTPStatus.OK
mock_update.assert_awaited_once()


@pytest.mark.asyncio
async def test_dev_cannot_access_own_tenant_manage_endpoints(dev_client, mocker):
"""DEV holds model:read and passes require(), so the scope check is the only
thing keeping it off the manage surface -- even for its own tenant."""
mock_list = mocker.patch(
'backend.apps.model_managment_app.list_models_for_admin',
return_value={"models": [], "total": 0},
)
response = dev_client.post(
"/model/manage/list",
json={"tenant_id": "rbac_tenant", "page": 1, "page_size": 10},
headers=auth_header,
)
assert response.status_code == HTTPStatus.FORBIDDEN
mock_list.assert_not_awaited()


@pytest.mark.asyncio
async def test_su_can_access_manage_endpoints(su_client, mocker):
"""SU may target any tenant, including one that is not its own."""
mocker.patch(
'backend.apps.model_managment_app.list_models_for_admin',
return_value={"models": [], "total": 0},
Expand Down
Loading