From e9e866473703b77abc1eed1fdbaf247a1e1f1048 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 07:30:22 +0100 Subject: [PATCH 1/3] fix(permissions): bound delegated grant writes by the actor's access (#687) A permissions.manage holder could grant any registered key, to anyone including itself. Role and user grant writes now refuse keys the actor does not hold and refuse edits to more privileged accounts (unscoped admins and in-process callers are unrestricted), with a refusal reason. User-grant cache eviction moves into PermissionService so every caller gets cross-worker invalidation on commit; the endpoint helper is deprecated for removal in 0.1.0. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 6 + docs/modules/permissions.md | 19 ++- host/tests/test_deprecation_expiry.py | 2 + modules/permissions/permissions/deps.py | 8 +- .../permissions/permissions/endpoints/api.py | 2 - .../permissions/endpoints/views.py | 2 - modules/permissions/permissions/service.py | 16 +- .../permissions/permissions/write_safety.py | 36 ++++ .../test_permissions_role_invalidation.py | 8 +- ..._permissions_service_grant_invalidation.py | 62 +++++++ .../tests/test_permissions_write_safety.py | 161 ++++++++++++++++++ 11 files changed, 313 insertions(+), 9 deletions(-) create mode 100644 modules/permissions/permissions/write_safety.py create mode 100644 modules/permissions/tests/test_permissions_service_grant_invalidation.py create mode 100644 modules/permissions/tests/test_permissions_write_safety.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 0079fd8e8..859c774c3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/modules/permissions.md b/docs/modules/permissions.md index 667e2c04a..710bddd63 100644 --- a/docs/modules/permissions.md +++ b/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 diff --git a/host/tests/test_deprecation_expiry.py b/host/tests/test_deprecation_expiry.py index 202e80718..ade2bba85 100644 --- a/host/tests/test_deprecation_expiry.py +++ b/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", diff --git a/modules/permissions/permissions/deps.py b/modules/permissions/permissions/deps.py index c2e9eb976..0abaf1f75 100644 --- a/modules/permissions/permissions/deps.py +++ b/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)) diff --git a/modules/permissions/permissions/endpoints/api.py b/modules/permissions/permissions/endpoints/api.py index 984c84771..9e4edf9f2 100644 --- a/modules/permissions/permissions/endpoints/api.py +++ b/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 diff --git a/modules/permissions/permissions/endpoints/views.py b/modules/permissions/permissions/endpoints/views.py index dbf41d0d5..63cd676d6 100644 --- a/modules/permissions/permissions/endpoints/views.py +++ b/modules/permissions/permissions/endpoints/views.py @@ -21,7 +21,6 @@ RequiresPermission, assigned_by, get_permission_service, - invalidate_grants_on_commit, ) from permissions.service import PermissionService @@ -129,5 +128,4 @@ async def update_user( 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) return RedirectResponse(_ADMIN_URL, status_code=303) diff --git a/modules/permissions/permissions/service.py b/modules/permissions/permissions/service.py index 83c9a70b5..caab82f72 100644 --- a/modules/permissions/permissions/service.py +++ b/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,7 @@ 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) to_remove = (existing - wanted) & registered # unregistered grants are never pruned if to_remove: @@ -201,6 +206,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 +222,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( diff --git a/modules/permissions/permissions/write_safety.py b/modules/permissions/permissions/write_safety.py new file mode 100644 index 000000000..5732e9d8c --- /dev/null +++ b/modules/permissions/permissions/write_safety.py @@ -0,0 +1,36 @@ +"""Delegated grant writes are bounded by the actor's effective access (#687).""" + +from fastapi import Request +from simple_module_core.permission_denied import PermissionDenied +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, +) + +from permissions.constants import PERM_MANAGE + + +async def check_permission_write( + request: Request | None, + registry: PermissionRegistry, + added: set[str], + target: DirectoryUser | None = None, +) -> None: + """Trusted in-process calls and unscoped admins may assign any registered key.""" + 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 PermissionDenied(PERM_MANAGE, "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 PermissionDenied(PERM_MANAGE, "You cannot manage a more privileged account.") + if any(not grants(held, key) for key in added): + raise PermissionDenied(PERM_MANAGE, "You cannot assign permissions you do not hold.") diff --git a/modules/permissions/tests/test_permissions_role_invalidation.py b/modules/permissions/tests/test_permissions_role_invalidation.py index ac318238e..69dab2eb6 100644 --- a/modules/permissions/tests/test_permissions_role_invalidation.py +++ b/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 == [] diff --git a/modules/permissions/tests/test_permissions_service_grant_invalidation.py b/modules/permissions/tests/test_permissions_service_grant_invalidation.py new file mode 100644 index 000000000..53c5c027b --- /dev/null +++ b/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() diff --git a/modules/permissions/tests/test_permissions_write_safety.py b/modules/permissions/tests/test_permissions_write_safety.py new file mode 100644 index 000000000..ecf1017dc --- /dev/null +++ b/modules/permissions/tests/test_permissions_write_safety.py @@ -0,0 +1,161 @@ +"""Delegated permission writes cannot escalate access (#687).""" + +import uuid +from types import SimpleNamespace + +import httpx +import pytest +from auth.contracts.schemas import UserContext +from fastapi import FastAPI, Request +from permissions.deps import get_permission_service +from permissions.endpoints.api import router as api_router +from permissions.endpoints.views import router as view_router +from permissions.models import RolePermission, UserPermission +from simple_module_core.permissions import PermissionRegistry +from simple_module_core.user_directory import DirectoryUser, NullUserDirectory +from sqlalchemy import select +from users.models import Role + + +class _Directory(NullUserDirectory): + def __init__(self, users): + self.users = {user.id: user for user in users} + + async def get_many(self, db, ids, *, with_roles=False): + return {key: self.users[key] for key in ids if key in self.users} + + +@pytest.fixture +async def writes(db_session): + actor = UserContext(id=str(uuid.uuid4()), email="manager@test", name="Manager") + target = DirectoryUser(id=str(uuid.uuid4()), email="target@test", roles=()) + directory = _Directory([DirectoryUser(id=actor.id, email=actor.email, roles=()), target]) + registry = PermissionRegistry() + registry.add_group("Test", ["permissions.manage", "test.held", "test.unheld"]) + registry.map_role("admin", ["*"]) + actor.permissions = ["permissions.manage", "test.held"] + role = Role(id=uuid.uuid4(), name="user") + db_session.add(role) + await db_session.flush() + + async def direct_grants(request, user): + result = await db_session.execute( + select(UserPermission.permission_key).where( + UserPermission.user_id == uuid.UUID(user.id) + ) + ) + return set(result.scalars()) + + registry.add_grant_source(direct_grants) + app = FastAPI() + app.state.sm = SimpleNamespace(permissions=registry) + app.state.user_directory = directory + + @app.middleware("http") + async def authenticate(request, call_next): + request.state.user = actor + return await call_next(request) + + # Keep the real dependency's request binding, directory, and safety context. + async def service(request: Request): + return await get_permission_service(request, db_session, registry) + + app.dependency_overrides[get_permission_service] = service + app.include_router(api_router, prefix="/api/permissions") + app.include_router(view_router, prefix="/admin/permissions") + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://testserver" + ) as client: + yield SimpleNamespace( + client=client, + actor=actor, + target=target, + directory=directory, + role=role, + registry=registry, + db=db_session, + ) + + +@pytest.mark.parametrize("prefix", ["/api/permissions", "/admin/permissions"]) +@pytest.mark.parametrize("kind", ["roles", "users"]) +@pytest.mark.parametrize("key, allowed", [("test.unheld", False), ("test.held", True)]) +async def test_added_grants_must_be_held(writes, prefix, kind, key, allowed): + target_id = writes.role.id if kind == "roles" else writes.actor.id + response = await writes.client.put(f"{prefix}/{kind}/{target_id}", json={"permissions": [key]}) + assert response.status_code == ((200 if prefix.startswith("/api") else 303) if allowed else 403) + if not allowed: + assert response.json()["detail"] + model = RolePermission if kind == "roles" else UserPermission + rows = (await writes.db.execute(select(model.permission_key))).scalars().all() + assert rows == ([key] if allowed else []) + + +@pytest.mark.parametrize("privilege", ["admin", "role", "direct", "source", "unknown"]) +async def test_cannot_edit_more_privileged_user(writes, privilege): + roles = () + if privilege in {"admin", "role"}: + roles = ("admin" if privilege == "admin" else "powerful",) + writes.registry.map_role("powerful", ["test.unheld"]) + elif privilege == "direct": + writes.db.add( + UserPermission(user_id=uuid.UUID(writes.target.id), permission_key="test.unheld") + ) + await writes.db.flush() + elif privilege == "unknown": + roles = None + else: + + async def extra_source(request, user): + return {"test.unheld"} if user.id == writes.target.id else set() + + writes.registry.add_grant_source(extra_source) + writes.directory.users[writes.target.id] = DirectoryUser( + id=writes.target.id, email=writes.target.email, roles=roles + ) + response = await writes.client.put( + f"/api/permissions/users/{writes.target.id}", json={"permissions": []} + ) + assert response.status_code == 403 + assert response.json()["detail"] + + +@pytest.mark.parametrize("kind", ["roles", "users"]) +async def test_scoped_admin_cannot_grant_outside_scope(writes, kind): + writes.actor.roles = ["admin"] + writes.actor.scopes = ["permissions.manage"] + target_id = writes.role.id if kind == "roles" else writes.target.id + response = await writes.client.put( + f"/api/permissions/{kind}/{target_id}", json={"permissions": ["test.unheld"]} + ) + assert response.status_code == 403 + + +@pytest.mark.parametrize("kind", ["roles", "users"]) +async def test_unscoped_admin_can_grant_any_registered_key(writes, kind): + writes.actor.roles = ["admin"] + target_id = writes.role.id if kind == "roles" else writes.target.id + response = await writes.client.put( + f"/api/permissions/{kind}/{target_id}", json={"permissions": ["test.unheld"]} + ) + assert response.status_code == 200 + + +async def test_actor_direct_grant_can_be_delegated(writes): + writes.actor.permissions = ["permissions.manage"] + writes.db.add(UserPermission(user_id=uuid.UUID(writes.actor.id), permission_key="test.held")) + await writes.db.flush() + response = await writes.client.put( + f"/api/permissions/users/{writes.target.id}", json={"permissions": ["test.held"]} + ) + assert response.status_code == 200 + + +async def test_existing_role_grant_can_be_retained_without_holding_it(writes): + writes.db.add(RolePermission(role_name="user", permission_key="test.unheld")) + await writes.db.flush() + response = await writes.client.put( + f"/api/permissions/roles/{writes.role.id}", + json={"permissions": ["test.unheld", "test.held"]}, + ) + assert response.status_code == 200 From a8a00fcceb6441419f292b87c5d6fa700c0a5c0c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 07:33:34 +0100 Subject: [PATCH 2/3] fix(permissions): refuse delegated edits to roles stronger than the actor (#687) Role writes checked only the added keys, so a delegated manager could strip keys from a more privileged role. Refuse the admin role and any role whose effective permissions the actor does not fully hold. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/permissions/permissions/service.py | 7 +++++- .../permissions/permissions/write_safety.py | 13 ++++++++++- .../tests/test_permissions_write_safety.py | 23 +++++++++++++++---- 3 files changed, 37 insertions(+), 6 deletions(-) diff --git a/modules/permissions/permissions/service.py b/modules/permissions/permissions/service.py index caab82f72..75be88e98 100644 --- a/modules/permissions/permissions/service.py +++ b/modules/permissions/permissions/service.py @@ -118,7 +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) + 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: diff --git a/modules/permissions/permissions/write_safety.py b/modules/permissions/permissions/write_safety.py index 5732e9d8c..6c31c3734 100644 --- a/modules/permissions/permissions/write_safety.py +++ b/modules/permissions/permissions/write_safety.py @@ -17,8 +17,15 @@ async def check_permission_write( 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.""" + """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) @@ -32,5 +39,9 @@ async def check_permission_write( target_permissions = await resolve_principal_permissions(request, target, registry) if any(not grants(held, key) for key in target_permissions): raise PermissionDenied(PERM_MANAGE, "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 PermissionDenied(PERM_MANAGE, "You cannot manage a more privileged role.") if any(not grants(held, key) for key in added): raise PermissionDenied(PERM_MANAGE, "You cannot assign permissions you do not hold.") diff --git a/modules/permissions/tests/test_permissions_write_safety.py b/modules/permissions/tests/test_permissions_write_safety.py index ecf1017dc..652e5aa8b 100644 --- a/modules/permissions/tests/test_permissions_write_safety.py +++ b/modules/permissions/tests/test_permissions_write_safety.py @@ -151,11 +151,26 @@ async def test_actor_direct_grant_can_be_delegated(writes): assert response.status_code == 200 -async def test_existing_role_grant_can_be_retained_without_holding_it(writes): +@pytest.mark.parametrize("kept", [["test.unheld", "test.held"], ["test.held"]]) +async def test_cannot_edit_role_holding_unheld_grant(writes, kept): + """Retaining or stripping a key the actor lacks are both refused.""" writes.db.add(RolePermission(role_name="user", permission_key="test.unheld")) await writes.db.flush() response = await writes.client.put( - f"/api/permissions/roles/{writes.role.id}", - json={"permissions": ["test.unheld", "test.held"]}, + f"/api/permissions/roles/{writes.role.id}", json={"permissions": kept} ) - assert response.status_code == 200 + assert response.status_code == 403 + keys = await writes.db.execute( + select(RolePermission.permission_key).where(RolePermission.role_name == "user") + ) + assert set(keys.scalars()) == {"test.unheld"} + + +async def test_cannot_edit_admin_role(writes): + admin = Role(id=uuid.uuid4(), name="admin") + writes.db.add(admin) + await writes.db.flush() + response = await writes.client.put( + f"/api/permissions/roles/{admin.id}", json={"permissions": ["test.held"]} + ) + assert response.status_code == 403 From 0d423d8bc8f6b2982a3b5b09c0d32af1b69f518b Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 08:04:17 +0100 Subject: [PATCH 3/3] fix(permissions): show grant-write refusals on the edit screens (#687) A refusal was a PermissionDenied naming permissions.manage, which the actor holds. It is now a plain 403 with the reason on the API, and a form error on the Inertia view routes that UserEdit/RoleEdit toast instead of the generic save-failed message. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../permissions/endpoints/views.py | 13 ++++++++-- .../permissions/pages/RoleEdit.tsx | 3 ++- .../permissions/pages/UserEdit.tsx | 3 ++- .../permissions/permissions/write_safety.py | 24 +++++++++++++------ .../tests/test_permissions_write_safety.py | 12 ++++++++-- 5 files changed, 42 insertions(+), 13 deletions(-) diff --git a/modules/permissions/permissions/endpoints/views.py b/modules/permissions/permissions/endpoints/views.py index 63cd676d6..a3d9d4d04 100644 --- a/modules/permissions/permissions/endpoints/views.py +++ b/modules/permissions/permissions/endpoints/views.py @@ -23,10 +23,13 @@ get_permission_service, ) 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) @@ -77,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) @@ -127,5 +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)) + 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) diff --git a/modules/permissions/permissions/pages/RoleEdit.tsx b/modules/permissions/permissions/pages/RoleEdit.tsx index acd435ff5..0adbfbe15 100644 --- a/modules/permissions/permissions/pages/RoleEdit.tsx +++ b/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; }, diff --git a/modules/permissions/permissions/pages/UserEdit.tsx b/modules/permissions/permissions/pages/UserEdit.tsx index cb87dc1cd..c97aae2a5 100644 --- a/modules/permissions/permissions/pages/UserEdit.tsx +++ b/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; }, diff --git a/modules/permissions/permissions/write_safety.py b/modules/permissions/permissions/write_safety.py index 6c31c3734..6b6db1144 100644 --- a/modules/permissions/permissions/write_safety.py +++ b/modules/permissions/permissions/write_safety.py @@ -1,7 +1,6 @@ """Delegated grant writes are bounded by the actor's effective access (#687).""" -from fastapi import Request -from simple_module_core.permission_denied import PermissionDenied +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 ( @@ -9,7 +8,18 @@ resolve_principal_permissions, ) -from permissions.constants import PERM_MANAGE + +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( @@ -35,13 +45,13 @@ async def check_permission_write( 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 PermissionDenied(PERM_MANAGE, "You cannot manage a more privileged account.") + 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 PermissionDenied(PERM_MANAGE, "You cannot manage a more privileged account.") + 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 PermissionDenied(PERM_MANAGE, "You cannot manage a more privileged role.") + raise _refuse("You cannot manage a more privileged role.") if any(not grants(held, key) for key in added): - raise PermissionDenied(PERM_MANAGE, "You cannot assign permissions you do not hold.") + raise _refuse("You cannot assign permissions you do not hold.") diff --git a/modules/permissions/tests/test_permissions_write_safety.py b/modules/permissions/tests/test_permissions_write_safety.py index 652e5aa8b..69f0e7da8 100644 --- a/modules/permissions/tests/test_permissions_write_safety.py +++ b/modules/permissions/tests/test_permissions_write_safety.py @@ -14,6 +14,7 @@ from simple_module_core.permissions import PermissionRegistry from simple_module_core.user_directory import DirectoryUser, NullUserDirectory from sqlalchemy import select +from starlette.middleware.sessions import SessionMiddleware from users.models import Role @@ -48,6 +49,7 @@ async def direct_grants(request, user): registry.add_grant_source(direct_grants) app = FastAPI() + app.add_middleware(SessionMiddleware, secret_key="test-only") app.state.sm = SimpleNamespace(permissions=registry) app.state.user_directory = directory @@ -83,9 +85,15 @@ async def service(request: Request): async def test_added_grants_must_be_held(writes, prefix, kind, key, allowed): target_id = writes.role.id if kind == "roles" else writes.actor.id response = await writes.client.put(f"{prefix}/{kind}/{target_id}", json={"permissions": [key]}) - assert response.status_code == ((200 if prefix.startswith("/api") else 303) if allowed else 403) - if not allowed: + api = prefix.startswith("/api") + if allowed: + assert response.status_code == (200 if api else 303) + elif api: + assert response.status_code == 403 assert response.json()["detail"] + else: + # The edit page gets the reason back as a form error, not an error page. + assert response.status_code in (302, 303) model = RolePermission if kind == "roles" else UserPermission rows = (await writes.db.execute(select(model.permission_key))).scalars().all() assert rows == ([key] if allowed else [])