From 54f329b1f1e3fd79ff41f0c8d2671ea582b45556 Mon Sep 17 00:00:00 2001 From: jeffwu Date: Tue, 29 Sep 2026 21:20:19 +0800 Subject: [PATCH] fix: let tenant admins manage models of their own tenant #4008 closed a horizontal-privilege hole on /model/manage/* by restricting the endpoints to the SU role. The whitelist did not distinguish a cross-tenant call from one naming the caller's own tenant, so ADMIN users lost the whole Models tab on /resource-manage: manage/list returned 403 and the page rendered an empty table with no error, because the create, update, delete, healthcheck and provider endpoints share the same guard. The page always sends the caller's own tenant_id (UserManageComp falls back to user.tenantId for non-SU), so rejecting ADMIN blocked no cross-tenant access -- it only broke tenant admins managing their own models. Replace the role whitelist with a role + tenant scope check: SU may target any tenant, ADMIN only the tenant its token belongs to, and every other role is rejected as before. ADMIN and SU share the same model:* permission seeds, so the role itself still has to be checked -- permissions alone cannot separate them. The existing ADMIN cross-tenant test kept passing because it used a foreign tenant_id, which is why the regression went uncaught. Add own-tenant coverage for list and update, plus a DEV case proving require(model:read) is not sufficient on its own to reach the manage surface. Co-Authored-By: Claude Opus 5 (1M context) --- backend/apps/model_managment_app.py | 48 ++++++++++------ test/backend/app/test_model_rbac.py | 87 +++++++++++++++++++++++++++-- 2 files changed, 115 insertions(+), 20 deletions(-) diff --git a/backend/apps/model_managment_app.py b/backend/apps/model_managment_app.py index 4c478781d..71855d72c 100644 --- a/backend/apps/model_managment_app.py +++ b/backend/apps/model_managment_app.py @@ -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. """ @@ -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: @@ -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", ) @@ -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}, " @@ -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( @@ -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( @@ -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( @@ -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( @@ -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}, " @@ -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}, " @@ -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}, " diff --git a/test/backend/app/test_model_rbac.py b/test/backend/app/test_model_rbac.py index adc3ff6c7..2cf806466 100644 --- a/test/backend/app/test_model_rbac.py +++ b/test/backend/app/test_model_rbac.py @@ -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 @@ -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}, @@ -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},