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},