diff --git a/backend/src/apis/app_api/admin/roles/routes.py b/backend/src/apis/app_api/admin/roles/routes.py index 907740b4e..e2ae786f0 100644 --- a/backend/src/apis/app_api/admin/roles/routes.py +++ b/backend/src/apis/app_api/admin/roles/routes.py @@ -13,7 +13,10 @@ from fastapi import APIRouter, Depends, HTTPException, status, Query from apis.shared.auth import User, require_admin +from apis.shared.rbac.admin_scopes import ADMIN_SCOPES from apis.shared.rbac.models import ( + AdminScopeListResponse, + AdminScopeResponse, AppRoleCreate, AppRoleUpdate, AppRoleResponse, @@ -58,6 +61,43 @@ async def list_roles( ) +# ⚠️ Must stay ABOVE `/{role_id}`. FastAPI matches in declaration order, and +# `/{role_id}` is a single-segment catch-all — declared first, it would swallow +# `/admin-scopes` and return a 404 for a role literally named "admin-scopes". +# (`/cache/stats` below is safe only because it has two segments.) +@router.get("/admin-scopes", response_model=AdminScopeListResponse) +async def list_admin_scopes( + admin: User = Depends(require_admin), +): + """ + List the delegated admin scope registry. + + Feeds the role form's scope picker. The registry is closed and defined in + code (`apis/shared/rbac/admin_scopes.py`) rather than derived from a + catalog — the roles UI has no free-text entry, so a grantable id that + isn't served here cannot be granted at all. + + Non-delegable scopes are included with `delegable: false` so the picker can + show them as unavailable rather than silently omitting them. + + Requires full admin: granting admin scopes is a `system_admin`-only act, + and this router is non-delegable for exactly that reason. + """ + return AdminScopeListResponse( + scopes=[ + AdminScopeResponse( + id=scope.id, + label=scope.label, + group=scope.group, + description=scope.description, + delegable=scope.delegable, + ) + for scope in ADMIN_SCOPES + ], + total=len(ADMIN_SCOPES), + ) + + @router.get("/{role_id}", response_model=AppRoleResponse) async def get_role( role_id: str, diff --git a/backend/src/apis/app_api/users/models.py b/backend/src/apis/app_api/users/models.py index 7051a0f19..6e4188ef2 100644 --- a/backend/src/apis/app_api/users/models.py +++ b/backend/src/apis/app_api/users/models.py @@ -30,6 +30,16 @@ class UserPermissionsResponse(BaseModel): app_roles: List[str] = Field(..., alias="appRoles", description="Resolved application roles") tools: List[str] = Field(..., description="Accessible tool IDs") models: List[str] = Field(..., description="Accessible model IDs") + # `skills` predates admin scopes and was carried by UserEffectivePermissions + # for months without ever reaching this response — the field-added-to-model, + # forgotten-in-the-response-shape bug. Both default to [] so a client on an + # older build is unaffected. + skills: List[str] = Field(default_factory=list, description="Accessible skill IDs") + admin_scopes: List[str] = Field( + default_factory=list, + alias="adminScopes", + description="Delegated admin feature areas granted to this user", + ) quota_tier: Optional[str] = Field(None, alias="quotaTier", description="Assigned quota tier") resolved_at: str = Field(..., alias="resolvedAt", description="ISO timestamp of resolution") diff --git a/backend/src/apis/app_api/users/routes.py b/backend/src/apis/app_api/users/routes.py index 35d87fafd..6d2698b73 100644 --- a/backend/src/apis/app_api/users/routes.py +++ b/backend/src/apis/app_api/users/routes.py @@ -39,6 +39,8 @@ async def get_my_permissions( app_roles=permissions.app_roles, tools=permissions.tools, models=permissions.models, + skills=permissions.skills, + admin_scopes=permissions.admin_scopes, quota_tier=permissions.quota_tier, resolved_at=permissions.resolved_at, ) diff --git a/backend/src/apis/shared/rbac/models.py b/backend/src/apis/shared/rbac/models.py index 7abd291e2..e7ff6f972 100644 --- a/backend/src/apis/shared/rbac/models.py +++ b/backend/src/apis/shared/rbac/models.py @@ -288,6 +288,32 @@ class AppRoleListResponse(BaseModel): total: int +class AdminScopeResponse(BaseModel): + """One entry from the delegated admin scope registry.""" + + id: str + label: str + group: str + description: str + delegable: bool + + model_config = {"populate_by_name": True} + + +class AdminScopeListResponse(BaseModel): + """The admin scope registry, for building the role form's scope picker. + + Non-delegable scopes are included rather than filtered out: the picker + should *show* that `admin.roles` and `admin.auth_providers` exist and + explain that they cannot be granted, instead of leaving an admin wondering + why those two areas are missing. The client renders them disabled; the + server rejects them regardless (`validate_admin_scopes`). + """ + + scopes: List[AdminScopeResponse] + total: int + + class CacheStatsResponse(BaseModel): """Cache statistics response.""" diff --git a/backend/tests/rbac/test_admin_scopes_api.py b/backend/tests/rbac/test_admin_scopes_api.py new file mode 100644 index 000000000..0d8a3ac6b --- /dev/null +++ b/backend/tests/rbac/test_admin_scopes_api.py @@ -0,0 +1,187 @@ +"""API surface for delegated admin scopes. + +PR-3 of ``docs/specs/granular-admin-permissions.md``: the registry endpoint that +feeds the role form's scope picker, and ``adminScopes`` on the permissions +endpoint the SPA already calls. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, patch + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from apis.app_api.admin.roles.routes import router as roles_router +from apis.app_api.users.routes import router as users_router +from apis.shared.auth import require_admin +from apis.shared.auth.dependencies import get_current_user_from_session +from apis.shared.auth.models import User +from apis.shared.rbac.admin_scopes import ( + ADMIN_SCOPES, + NON_DELEGABLE_SCOPES, +) +from apis.shared.rbac.models import UserEffectivePermissions +from tests.conftest import override_admin_auth + + +def _user() -> User: + return User( + email="admin@example.com", + user_id="admin-1", + name="Admin", + roles=["system_admin"], + ) + + +# --------------------------------------------------------------------------- +# GET /admin/roles/admin-scopes +# --------------------------------------------------------------------------- + + +@pytest.fixture +def roles_client() -> TestClient: + app = FastAPI() + app.include_router(roles_router, prefix="/admin") + override_admin_auth(app, _user) + return TestClient(app) + + +def test_registry_lists_every_scope(roles_client) -> None: + resp = roles_client.get("/admin/roles/admin-scopes") + + assert resp.status_code == 200 + body = resp.json() + assert body["total"] == len(ADMIN_SCOPES) + assert {s["id"] for s in body["scopes"]} == {s.id for s in ADMIN_SCOPES} + + +def test_registry_marks_non_delegable_scopes(roles_client) -> None: + """The picker needs to *show* these as unavailable, not omit them.""" + body = roles_client.get("/admin/roles/admin-scopes").json() + + not_delegable = {s["id"] for s in body["scopes"] if not s["delegable"]} + assert not_delegable == set(NON_DELEGABLE_SCOPES) + + +def test_registry_entries_carry_display_metadata(roles_client) -> None: + body = roles_client.get("/admin/roles/admin-scopes").json() + + for scope in body["scopes"]: + assert scope["label"].strip(), scope["id"] + assert scope["group"].strip(), scope["id"] + assert scope["description"].strip(), scope["id"] + + +def test_registry_route_is_not_shadowed_by_the_role_id_route() -> None: + """`/admin-scopes` must be declared before `/{role_id}`. + + Single-segment literal paths lose to a single-segment path parameter + whenever the parameter is declared first, and the symptom is a confusing + 404 (or a lookup for a role named "admin-scopes") rather than an error at + import time. Asserted on the router's declaration order so a future edit + that moves the handler fails here instead of in production. + """ + paths = [getattr(r, "path", "") for r in roles_router.routes] + + assert "/roles/admin-scopes" in paths + assert paths.index("/roles/admin-scopes") < paths.index("/roles/{role_id}") + + +def test_registry_requires_full_admin() -> None: + """Granting scopes is system_admin-only, so reading the registry is too.""" + app = FastAPI() + app.include_router(roles_router, prefix="/admin") + + def _deny(): + from fastapi import HTTPException + + raise HTTPException(status_code=403, detail="Access denied.") + + app.dependency_overrides[require_admin] = _deny + + resp = TestClient(app).get("/admin/roles/admin-scopes") + assert resp.status_code == 403 + + +# --------------------------------------------------------------------------- +# GET /users/me/permissions +# --------------------------------------------------------------------------- + + +def _permissions(**kwargs) -> UserEffectivePermissions: + defaults = dict( + user_id="u-1", + app_roles=["content_admin"], + tools=["tool_a"], + models=["model_a"], + quota_tier="standard", + resolved_at="2026-07-27T00:00:00Z", + skills=["skill_a"], + admin_scopes=["admin.skills"], + ) + defaults.update(kwargs) + return UserEffectivePermissions(**defaults) + + +def _users_client(permissions: UserEffectivePermissions) -> TestClient: + app = FastAPI() + app.include_router(users_router) + app.dependency_overrides[get_current_user_from_session] = _user + + service = AsyncMock() + service.resolve_user_permissions = AsyncMock(return_value=permissions) + patcher = patch( + "apis.app_api.users.routes.get_app_role_service", return_value=service + ) + patcher.start() + client = TestClient(app) + client._patcher = patcher # type: ignore[attr-defined] + return client + + +def test_permissions_includes_admin_scopes() -> None: + client = _users_client(_permissions()) + try: + body = client.get("/users/me/permissions").json() + finally: + client._patcher.stop() # type: ignore[attr-defined] + + assert body["adminScopes"] == ["admin.skills"] + + +def test_permissions_includes_skills() -> None: + """Regression: `skills` was on the model for months but never in the response.""" + client = _users_client(_permissions()) + try: + body = client.get("/users/me/permissions").json() + finally: + client._patcher.stop() # type: ignore[attr-defined] + + assert body["skills"] == ["skill_a"] + + +def test_permissions_response_is_camel_cased() -> None: + """The SPA reads camelCase; a snake_case key would silently read undefined.""" + client = _users_client(_permissions()) + try: + body = client.get("/users/me/permissions").json() + finally: + client._patcher.stop() # type: ignore[attr-defined] + + assert "adminScopes" in body + assert "admin_scopes" not in body + assert "appRoles" in body + assert "quotaTier" in body + + +def test_permissions_defaults_to_empty_for_a_user_with_no_scopes() -> None: + client = _users_client(_permissions(admin_scopes=[], skills=[])) + try: + body = client.get("/users/me/permissions").json() + finally: + client._patcher.stop() # type: ignore[attr-defined] + + assert body["adminScopes"] == [] + assert body["skills"] == [] diff --git a/docs/specs/granular-admin-permissions.md b/docs/specs/granular-admin-permissions.md index 1ec42a35c..d3289cc13 100644 --- a/docs/specs/granular-admin-permissions.md +++ b/docs/specs/granular-admin-permissions.md @@ -295,13 +295,16 @@ module level and the handlers reference the alias: ```python # apis/app_api/admin/tools/routes.py -_require = require_admin_scope("admin.tools") +require_tools_admin = require_admin_scope("admin.tools") @router.get("") -async def list_tools(admin: User = Depends(_require)): +async def list_tools(admin: User = Depends(require_tools_admin)): ``` -One mechanical substitution per file, 15 files. `admin/roles/routes.py` and +**As built (PR-2):** one mechanical substitution per file, **13 packages**. +The dependency is named after its area rather than a private `_require` — +mirroring the pre-existing `require_marketplace_admin`, and giving tests a +stable public handle to override. `admin/roles/routes.py` and `admin/auth_providers/routes.py` keep bare `require_admin` (I1). The migration is fully contained: `require_admin` is *invoked* nowhere outside @@ -402,8 +405,8 @@ on the scope. | PR | Content | |---|---| | **PR-1** | Data + registry: `granted_admin_scopes` on `AppRole`, `admin_scopes` through `EffectivePermissions`/`UserEffectivePermissions`/`_merge_permissions`, `admin_scopes.py` registry, non-delegable + unknown-scope validation in `role_constraints.py` wired into `admin_service`, `grantedAdminScopes` on the `AppRoleCreate`/`AppRoleUpdate` bodies so the axis round-trips end to end. Unit tests. **Inert** — the value is stored and resolved, but no authorization path reads it. | -| **PR-2** | Enforcement: `require_admin_scope`, the 15-file alias migration, the I3 write-through guard on `set_roles_for_*`, the §6.3 architecture test. Behavior for `system_admin` unchanged. Also fix the two stale auth docs while the context is loaded — `apis/app_api/admin/README.md` and `apis/shared/auth/RBAC_QUICK_REFERENCE.md` both document helpers that do not exist (`require_roles`, `require_all_roles`, `has_any_role`, `require_faculty`, …) and both describe `require_admin` as "Admin or SuperAdmin". Anyone implementing delegated admin will read them first. | -| **PR-3** | API surface: `adminScopes` on `/users/me/permissions`, `GET /admin/roles/admin-scopes` registry endpoint. Watch the precedent bug here: `UserPermissionsResponse` (`app_api/users/routes.py:37-43`) still omits `skills` even though `UserEffectivePermissions` has carried it for months — a field added to the model and forgotten in the response shape. Add `skills` while in there. | +| **PR-2** ✅ #774 | Enforcement: `require_admin_scope`, the 13-package alias migration, the I3 write-through guard on `set_roles_for_*`, the §6.3 architecture test. Behavior for `system_admin` unchanged. Also fix the two stale auth docs while the context is loaded — `apis/app_api/admin/README.md` and `apis/shared/auth/RBAC_QUICK_REFERENCE.md` both document helpers that do not exist (`require_roles`, `require_all_roles`, `has_any_role`, `require_faculty`, …) and both describe `require_admin` as "Admin or SuperAdmin". Anyone implementing delegated admin will read them first. **As built**, two things were added beyond the plan: the write-through guard raises `RoleMutationForbidden` → 403 through a new app-level handler (the resource routes catch `ValueError` → 400, which would misreport a denied escalation as a bad request), and a `override_admin_auth` test helper reads dependencies off the app rather than importing them — `test_skills_feature_flag.py` reloads the admin routes module, which rebuilds every dependency object and silently breaks an import-based override list. | +| **PR-3** ✅ | API surface: `adminScopes` on `/users/me/permissions`, `GET /admin/roles/admin-scopes` registry endpoint. Watch the precedent bug here: `UserPermissionsResponse` (`app_api/users/routes.py:37-43`) still omits `skills` even though `UserEffectivePermissions` has carried it for months — a field added to the model and forgotten in the response shape. Add `skills` while in there. **As built**, the registry endpoint must be declared *above* `/{role_id}` — a single-segment literal loses to a single-segment path parameter declared first, and the symptom is a 404 rather than an import-time error. Guarded by a declaration-order test. | | **PR-4** | SPA: `adminScopes` signal + `hasAdminScope`/`canAccessAdmin`, per-route `adminScopeGuard`, nav filtering, landing resolver, scoped badge refresh, admin-scopes control on the role form. | | **PR-5** | Audit log for admin-surface mutations (decided in scope, §8). Promotes the existing structured-log emission points into durable, queryable records with before/after values. | diff --git a/frontend/ai.client/src/app/auth/user.model.ts b/frontend/ai.client/src/app/auth/user.model.ts index f08b87471..0fdafed46 100644 --- a/frontend/ai.client/src/app/auth/user.model.ts +++ b/frontend/ai.client/src/app/auth/user.model.ts @@ -21,6 +21,16 @@ export interface UserPermissions { appRoles: string[]; tools: string[]; models: string[]; + skills: string[]; + /** + * Delegated admin feature areas (e.g. `admin.tools`), from the closed + * registry served by GET /admin/roles/admin-scopes. Empty for a user who is + * not a delegated admin; `system_admin` holds every scope implicitly and so + * also carries an empty list here — check `appRoles` for the superuser. + * + * Not yet consumed: the admin guard and nav still gate on `system_admin`. + */ + adminScopes: string[]; quotaTier: string | null; resolvedAt: string; }