From b70c8634fb9c503caebc32001ff7023f75c9cec0 Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Mon, 27 Jul 2026 10:54:00 -0600 Subject: [PATCH] feat(rbac): expose admin scopes over the API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-3 of docs/specs/granular-admin-permissions.md. Serves the scope registry and the caller's own scopes, so PR-4 has something to build the SPA against. No behavior change: nothing consumes these yet. - GET /admin/roles/admin-scopes returns the closed registry that feeds the role form's scope picker. Non-delegable scopes are included with `delegable: false` rather than filtered out, so the picker can show them as unavailable instead of leaving an admin wondering why two areas are missing. Lives on the roles router, which is non-delegable — granting scopes is a system_admin-only act. - /users/me/permissions gains `adminScopes`, and `skills`, which UserEffectivePermissions has carried for months without it ever reaching the response — the field-added-to-model, forgotten-in-the-response-shape bug the spec flagged. Both default to [] so an older client is unaffected. The registry handler must be declared above `/{role_id}`: FastAPI matches in declaration order and a single-segment literal loses to a single-segment path parameter declared first. The symptom is a 404, not an import-time error, so a declaration-order test guards it — verified to fail when the handler is moved. Frontend `UserPermissions` gains both fields to keep the cross-package contract honest. The guard and nav still gate on system_admin; wiring them up is PR-4. Also corrects spec §6.2, which described the dependency as `_require` across 15 files — it shipped as `require__admin` across 13 — and records what PR-2 and PR-3 added beyond the plan. Co-Authored-By: Claude Opus 5 --- .../src/apis/app_api/admin/roles/routes.py | 40 ++++ backend/src/apis/app_api/users/models.py | 10 + backend/src/apis/app_api/users/routes.py | 2 + backend/src/apis/shared/rbac/models.py | 26 +++ backend/tests/rbac/test_admin_scopes_api.py | 187 ++++++++++++++++++ docs/specs/granular-admin-permissions.md | 13 +- frontend/ai.client/src/app/auth/user.model.ts | 10 + 7 files changed, 283 insertions(+), 5 deletions(-) create mode 100644 backend/tests/rbac/test_admin_scopes_api.py 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; }