Tech-debt batch 22: permissions.manage can no longer grant what it does not hold (#687)

CHANGELOG.md

@@ -19,6 +19,12 @@ All notable changes to this project are documented in this file. The format is b
   and refresh tokens, sharing revocation with password changes and sign-out-everywhere (#686).
 - Defer single admin create/invite events and invitation-related emails until the
   request commits; failed commits deliver neither mail nor ordinary events (#692, #627).
+- **Behaviour change:** permission managers may only add keys they hold and cannot
+  edit stronger users; scoped admin tokens obey their permission ceiling. Both
+  API and Inertia writes return a refusal reason (#687, following #624 and #653).
+- Permission service user-grant writes now invalidate caches after commit for
+  every caller, including cross-worker delivery with an app; deprecate the
+  endpoint invalidation helper for removal in 0.1.0 (#687, #621).
 
 ## [0.0.41] — 2026-10-25
 

docs/modules/permissions.md

@@ -28,6 +28,16 @@ module still depends on `Users`.
 
 All require authentication. Read endpoints need `permissions.view`; mutate endpoints need `permissions.manage`.
 
+**Behaviour change (#687):** `permissions.manage` alone does not allow escalation.
+Role and user updates refuse added keys the actor does not hold, using the same
+resolved permissions as authorization (roles, direct grants, other grant sources,
+and token scopes). User updates also refuse targets with permissions beyond the
+actor's, admin targets, or targets whose provider cannot report their roles.
+Unscoped admins are unrestricted. Refusals return HTTP 403 with a reason in
+`detail`, on both API and Inertia write routes. Existing role keys may be retained
+or removed without holding them; unregistered submitted keys are still ignored and
+stored grants for unavailable modules are preserved.
+
 | Method + path | Body / response | Permission |
 |---|---|---|
 | `GET /api/permissions/` | → `list[PermissionGroupOut]` | `permissions.view` |
@@ -72,7 +82,14 @@ All three are resolved once per request by `InertiaLayoutDataMiddleware` and cac
 
 ### Caching and propagation
 
-The grant source runs on every authenticated request, so each process caches a user's direct grants for 30 seconds (`permissions.grants.GRANTS_TTL_SECONDS`). Saving a user's grants publishes `permissions.user_grants` on the `InvalidationBus` once the transaction commits. The worker that made the change sees it on the next request. Other workers see it immediately when `background_tasks` provides a Redis transport, and otherwise within the TTL.
+The grant source runs on every authenticated request, so each process caches a user's direct grants for 30 seconds (`permissions.grants.GRANTS_TTL_SECONDS`). Saving a user's grants publishes `permissions.user_grants` on the `InvalidationBus` once the transaction commits. `PermissionService.set_user_permissions` owns this callback, so service callers
+receive the same invalidation as endpoints (#687, #621); rollback publishes nothing.
+Pass `app` to the service for cross-worker delivery. Without an app, commit clears
+the local grant cache. The endpoint helper
+`permissions.deps.invalidate_grants_on_commit` is deprecated until 0.1.0; callers
+no longer need it. A service constructed without a `request` is a trusted
+in-process caller; pass the actor's request for delegated writes.
+The worker that made the change sees it on the next request. Other workers see it immediately when `background_tasks` provides a Redis transport, and otherwise within the TTL.
 
 ## Public contracts
 

host/tests/test_deprecation_expiry.py

@@ -22,6 +22,7 @@
 # Modules whose import registers a deprecation (shim modules and call-site shims).
 _SHIM_MODULES = (
     "host.routes_legacy",
+    "permissions.deps",
     "settings.registration",
     "settings.module_registry",
     "settings.hydrate",
@@ -31,6 +32,7 @@
 )
 _EXPECTED = {
     "host.routes_legacy (pre-/admin view URL redirects)",
+    "permissions.deps.invalidate_grants_on_commit",
     "settings.registration",
     "settings.module_registry",
     "settings.hydrate",

modules/permissions/permissions/deps.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 from fastapi import Depends, Request
+from simple_module_core.deprecation import deprecated
 from simple_module_core.permissions import PermissionRegistry
 from simple_module_core.user_directory import get_user_directory
 from simple_module_db.deps import get_db
@@ -30,7 +31,9 @@ async def get_permission_service(
     db: AsyncSession = Depends(get_db),
     registry: PermissionRegistry = Depends(get_permission_registry),
 ) -> PermissionService:
-    return PermissionService(db, registry, get_user_directory(request.app), app=request.app)
+    return PermissionService(
+        db, registry, get_user_directory(request.app), app=request.app, request=request
+    )
 
 
 def assigned_by(request: Request) -> str | None:
@@ -39,8 +42,9 @@ def assigned_by(request: Request) -> str | None:
     return str(user.id) if user is not None else None
 
 
+@deprecated(remove_in="0.1.0", use="PermissionService.set_user_permissions (invalidates on commit)")
 def invalidate_grants_on_commit(request: Request, service: PermissionService, user_id) -> None:
-    """Evict *user_id*'s cached direct grants in every worker once this commits."""
+    """Compatibility for callers predating service-owned invalidation."""
     service.db.on_commit(lambda: publish_grants_changed(request.app, user_id))
 
 

modules/permissions/permissions/endpoints/api.py

@@ -18,7 +18,6 @@
     RequiresPermission,
     assigned_by,
     get_permission_service,
-    invalidate_grants_on_commit,
 )
 from permissions.service import PermissionService
 
@@ -104,5 +103,4 @@ async def set_user_permissions(
     result = await service.set_user_permissions(user_id, data.permissions, assigned_by(request))
     if result is None:
         raise HTTPException(status_code=404, detail="User not found")
-    invalidate_grants_on_commit(request, service, user_id)
     return result

modules/permissions/permissions/endpoints/views.py

@@ -21,13 +21,15 @@
     RequiresPermission,
     assigned_by,
     get_permission_service,
-    invalidate_grants_on_commit,
 )
 from permissions.service import PermissionService
+from permissions.write_safety import PermissionWriteRefused
 
 router = APIRouter()
 
 _ADMIN_URL = "/admin/users/"
+# Form-error key the edit pages read to show why a save was refused (#687).
+_REFUSAL_FIELD = "permissions"
 
 
 @router.get("/", response_model=None)
@@ -78,7 +80,10 @@ async def update_role(
         data = RolePermissionsUpdate(**body)
     except ValidationError as exc:
         return redirect_back_with_errors(request, validation_errors_to_dict(exc))
-    await service.set_role_permissions(role_id, data.permissions, assigned_by(request))
+    try:
+        await service.set_role_permissions(role_id, data.permissions, assigned_by(request))
+    except PermissionWriteRefused as exc:
+        return redirect_back_with_errors(request, {_REFUSAL_FIELD: exc.detail})
     return RedirectResponse(_ADMIN_URL, status_code=303)
 
 
@@ -128,6 +133,8 @@ async def update_user(
         data = UserPermissionsUpdate(**body)
     except ValidationError as exc:
         return redirect_back_with_errors(request, validation_errors_to_dict(exc))
-    await service.set_user_permissions(user_id, data.permissions, assigned_by(request))
-    invalidate_grants_on_commit(request, service, user_id)
+    try:
+        await service.set_user_permissions(user_id, data.permissions, assigned_by(request))
+    except PermissionWriteRefused as exc:
+        return redirect_back_with_errors(request, {_REFUSAL_FIELD: exc.detail})
     return RedirectResponse(_ADMIN_URL, status_code=303)

modules/permissions/permissions/pages/RoleEdit.tsx

@@ -69,7 +69,8 @@ function RoleEdit({ role, assigned, groups }: Props) {
     put(`/admin/permissions/roles/${role.id}`, {
       preserveScroll: true,
       onSuccess: () => toast.success(t(keys.permissions.toasts.saved)),
-      onError: () => toast.error(t(keys.permissions.toasts.save_failed)),
+      onError: (errors) =>
+        toast.error(errors.permissions ?? t(keys.permissions.toasts.save_failed)),
       onFinish: () => {
         savingRef.current = false;
       },

modules/permissions/permissions/pages/UserEdit.tsx

@@ -65,7 +65,8 @@ function UserEdit({ user, roles, direct, inherited, inherited_by: inheritedBy, g
     put(`/admin/permissions/users/${user.id}`, {
       preserveScroll: true,
       onSuccess: () => toast.success(t(keys.permissions.toasts.saved)),
-      onError: () => toast.error(t(keys.permissions.toasts.save_failed)),
+      onError: (errors) =>
+        toast.error(errors.permissions ?? t(keys.permissions.toasts.save_failed)),
       onFinish: () => {
         savingRef.current = false;
       },

modules/permissions/permissions/service.py

@@ -6,7 +6,7 @@
 from collections.abc import Callable
 from typing import TYPE_CHECKING
 
-from fastapi import FastAPI
+from fastapi import FastAPI, Request
 from simple_module_core.user_directory import DirectoryUser, NullUserDirectory, UserDirectory
 from sqlalchemy import delete, func, select
 from sqlalchemy.ext.asyncio import AsyncSession
@@ -22,6 +22,7 @@
 )
 from permissions.group_labels import localized_groups
 from permissions.models import RolePermission, UserPermission
+from permissions.write_safety import check_permission_write
 
 if TYPE_CHECKING:
     from simple_module_core.permissions import PermissionRegistry
@@ -50,8 +51,11 @@ def __init__(
         registry: PermissionRegistry,
         directory: UserDirectory | None = None,
         app: FastAPI | None = None,
+        *,
+        request: Request | None = None,
     ) -> None:
         self.app = app
+        self.request = request
         self.db = db
         self.registry = registry
         self.directory: UserDirectory = directory or NullUserDirectory()
@@ -114,6 +118,12 @@ async def set_role_permissions(
         registered = self._registered_keys()
         wanted = {k for k in keys if k in registered}
         existing = set(await self._get_role_keys(role.name))
+        await check_permission_write(
+            self.request,
+            self.registry,
+            wanted - existing,
+            role=(role.name, self._resolve_role_permissions([role.name]) | existing),
+        )
         to_remove = (existing - wanted) & registered  # unregistered grants are never pruned
 
         if to_remove:
@@ -201,6 +211,7 @@ async def set_user_permissions(
         registered = self._registered_keys()
         wanted = {k for k in keys if k in registered}
         existing = set(await self.get_user_direct_keys(user_id))
+        await check_permission_write(self.request, self.registry, wanted - existing, user)
         to_remove = (existing - wanted) & registered
 
         if to_remove:
@@ -216,6 +227,14 @@ async def set_user_permissions(
         )
         await self.db.flush()
 
+        from permissions.grants import clear_grants_cache, publish_grants_changed
+
+        app = self.app
+        if app is None:
+            self.db.on_commit(clear_grants_cache)
+        else:
+            self.db.on_commit(lambda: publish_grants_changed(app, user_id))
+
         role_names = _role_names(user)
         inherited = self._resolve_role_permissions(role_names) - wanted
         return UserPermissionsOut(

modules/permissions/permissions/write_safety.py

@@ -0,0 +1,57 @@
+"""Delegated grant writes are bounded by the actor's effective access (#687)."""
+
+from fastapi import HTTPException, Request, status
+from simple_module_core.permissions import ADMIN_ROLE, PermissionRegistry, grants
+from simple_module_core.user_directory import DirectoryUser
+from simple_module_hosting.permissions import (
+    ensure_resolved_permissions,
+    resolve_principal_permissions,
+)
+
+
+class PermissionWriteRefused(HTTPException):
+    """A 403 with a reason but no ``required_permission``: the actor holds
+    ``permissions.manage``, so nothing may claim it is missing. View routes turn
+    it into a form error so the edit screen can show the reason."""
+
+    def __init__(self, reason: str) -> None:
+        super().__init__(status_code=status.HTTP_403_FORBIDDEN, detail=reason)
+
+
+def _refuse(reason: str) -> PermissionWriteRefused:
+    return PermissionWriteRefused(reason)
+
+
+async def check_permission_write(
+    request: Request | None,
+    registry: PermissionRegistry,
+    added: set[str],
+    target: DirectoryUser | None = None,
+    *,
+    role: tuple[str, set[str]] | None = None,
+) -> None:
+    """Trusted in-process calls and unscoped admins may assign any registered key.
+
+    A delegated actor may change a user or role only if it already holds
+    everything the target holds: otherwise it could strip access from (or
+    reshape) principals stronger than itself. ``role`` is ``(name, keys)``.
+    """
+    if request is None:
+        return
+    actor = getattr(request.state, "user", None)
+    if actor is not None and ADMIN_ROLE in actor.roles and not actor.is_scoped:
+        return
+    held = await ensure_resolved_permissions(request)
+    if target is not None:
+        # Unknown provider roles cannot prove that the target is within reach.
+        if target.roles is None or ADMIN_ROLE in target.roles:
+            raise _refuse("You cannot manage a more privileged account.")
+        target_permissions = await resolve_principal_permissions(request, target, registry)
+        if any(not grants(held, key) for key in target_permissions):
+            raise _refuse("You cannot manage a more privileged account.")
+    if role is not None:
+        name, role_keys = role
+        if name == ADMIN_ROLE or any(not grants(held, key) for key in role_keys):
+            raise _refuse("You cannot manage a more privileged role.")
+    if any(not grants(held, key) for key in added):
+        raise _refuse("You cannot assign permissions you do not hold.")

modules/permissions/tests/test_permissions_role_invalidation.py

@@ -66,7 +66,13 @@ async def test_role_request_publishes_only_after_commit(app, commit):
         role = Role(id=uuid.uuid4(), name="editor")
         db.add(role)
         await db.flush()
-        service = await get_permission_service(Request({"type": "http", "app": app}), db, registry)
+        from auth.contracts.schemas import UserContext
+
+        request = Request({"type": "http", "app": app})
+        request.state.user = UserContext(
+            id="admin", email="admin@test", name="Admin", roles=["admin"]
+        )
+        service = await get_permission_service(request, db, registry)
         await service.set_role_permissions(role.id, ["test.view"])
         assert "test.view" not in registry.role_map.get("editor", [])
         assert messages == []

modules/permissions/tests/test_permissions_service_grant_invalidation.py

@@ -0,0 +1,62 @@
+"""Service callers get durable direct-grant invalidation too (#687, #621)."""
+
+import uuid
+from types import SimpleNamespace
+
+import pytest
+from fastapi import FastAPI
+from permissions.grants import CHANNEL, clear_grants_cache, direct_grant_source, subscribe
+from permissions.models import UserPermission
+from permissions.service import PermissionService
+from simple_module_core.invalidation import InvalidationBus
+from simple_module_core.permissions import PermissionRegistry
+from simple_module_core.user_directory import DirectoryUser, NullUserDirectory
+from simple_module_db.transaction import finalize_session, rollback_session
+from sqlalchemy.ext.asyncio import async_sessionmaker
+from starlette.requests import Request
+
+
+@pytest.mark.parametrize("with_app", [False, True])
+@pytest.mark.parametrize("commit", [False, True])
+@pytest.mark.parametrize("clear", [False, True])
+async def test_service_invalidates_user_grants_only_on_commit(db_session, commit, clear, with_app):
+    user = DirectoryUser(id=str(uuid.uuid4()), email="target@test", roles=())
+
+    class Directory(NullUserDirectory):
+        async def get_many(self, db, ids, *, with_roles=False):
+            return {user.id: user} if user.id in ids else {}
+
+    registry = PermissionRegistry()
+    registry.add_group("Test", ["test.view"])
+    bus = InvalidationBus()
+    subscribe(bus)
+    messages = []
+    bus.subscribe(CHANNEL, messages.append)
+    app = FastAPI()
+    app.state.sm = SimpleNamespace(
+        invalidation=bus, db=SimpleNamespace(session_factory=async_sessionmaker(db_session.bind))
+    )
+    request = Request({"type": "http", "app": app})
+    user_id = uuid.UUID(user.id)
+    if clear:
+        db_session.add(UserPermission(user_id=user_id, permission_key="test.view"))
+        await db_session.commit()
+    clear_grants_cache()
+    before = frozenset({"test.view"} if clear else set())
+    after = frozenset() if clear else frozenset({"test.view"})
+    try:
+        assert await direct_grant_source(request, user) == before
+        service = PermissionService(
+            db_session, registry, Directory(), app=app if with_app else None
+        )
+        await service.set_user_permissions(user_id, [] if clear else ["test.view"])
+        assert await direct_grant_source(request, user) == before
+        assert messages == []
+        if commit:
+            await finalize_session(db_session)
+        else:
+            await rollback_session(db_session)
+        assert len(messages) == int(commit and with_app)
+        assert await direct_grant_source(request, user) == (after if commit else before)
+    finally:
+        clear_grants_cache()