diff --git a/docs/modules/auth.md b/docs/modules/auth.md index 092fd45e..0a91d22e 100644 --- a/docs/modules/auth.md +++ b/docs/modules/auth.md @@ -68,7 +68,7 @@ async def delete_order(order_id: int, user: CurrentUser) -> None: ... ``` -For permission checks that need to honour **direct user grants** (not just role-derived perms), use [`RequiresPermission` from the permissions module](/modules/permissions#using-requirespermission) instead. +`require_permission` passes when the user holds *any* of the listed keys. It reads the same per-request permission set as `simple_module_hosting.permissions.RequiresPermission`, which covers roles from the registry's role map plus every grant source (direct user grants, when the [permissions module](/modules/permissions#using-requirespermission) is installed). ## Pluggable auth diff --git a/docs/modules/permissions.md b/docs/modules/permissions.md index a58463ad..e0705491 100644 --- a/docs/modules/permissions.md +++ b/docs/modules/permissions.md @@ -4,9 +4,9 @@ The permissions module decouples role-based and per-user permission grants from - Two assignment tables (`permissions_role_permission`, `permissions_user_permission`). - An admin UI to edit them. -- A `RequiresPermission` dependency that consults *both* roles and direct user grants. +- A grant source that feeds direct user grants into the framework's permission resolution. -The framework ships its own simpler `RequiresPermission` in `simple_module_hosting.permissions` that only checks roles; if you install the `permissions` module, prefer the one re-exported from `permissions.deps` because it also honours direct grants. +There is one `RequiresPermission`, in `simple_module_hosting.permissions`. With this module installed it honours direct user grants as well as roles, and so does everything else that reads the resolved set: `resolved_permissions_for`, the menu filter, and the frontend's `auth.permissions`. `permissions.deps.RequiresPermission` is the same class, kept so existing imports still work. Before GH #337 they were separate classes, and a direct grant took effect only on routes that imported the `permissions.deps` one. ## ModuleMeta @@ -45,7 +45,7 @@ All require authentication. Read endpoints need `permissions.view`; mutate endpo ```python from fastapi import APIRouter, Depends -from permissions.deps import RequiresPermission +from simple_module_hosting.permissions import RequiresPermission router = APIRouter() @@ -59,11 +59,15 @@ async def delete_order(order_id: int) -> None: ... `RequiresPermission(permission)` takes a **single** permission key and 403s unless the request's user holds it, considering: -1. The keys assigned to any of the user's roles (read from the request-cached `resolved_permissions`). -2. The keys assigned directly to the user (`permissions_user_permission`). -3. The implicit `WILDCARD` grant — the `admin` role is synced to hold every permission key at startup, so admins pass any check. +1. The keys assigned to any of the user's roles. +2. The keys assigned directly to the user (`permissions_user_permission`), contributed by `permissions.grants.direct_grant_source` via `PermissionRegistry.add_grant_source`. +3. The implicit `WILDCARD` grant. The `admin` role is synced to hold every permission key at startup, so admins pass any check. -For something tied to *only* role membership (no direct grants), use `auth.deps.require_permission` instead — it's a hair cheaper. +All three are resolved once per request by `InertiaLayoutDataMiddleware` and cached on `request.state.resolved_permissions`. `auth.deps.require_permission(*keys)` reads the same set, with any-of semantics. + +### 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. ## Public contracts diff --git a/framework/core/simple_module_core/permissions.py b/framework/core/simple_module_core/permissions.py index 3dbbc545..9e6b5681 100644 --- a/framework/core/simple_module_core/permissions.py +++ b/framework/core/simple_module_core/permissions.py @@ -3,8 +3,9 @@ from __future__ import annotations import logging -from collections.abc import Callable, Collection, Iterable +from collections.abc import Awaitable, Callable, Collection, Iterable from dataclasses import dataclass, field +from typing import Any logger = logging.getLogger(__name__) @@ -50,6 +51,18 @@ def grants(held: Collection[str], required: str) -> bool: return WILDCARD in held or required in held +GrantSource = Callable[[Any, Any], Awaitable[Collection[str]]] +"""``async (request, user) -> keys`` — permissions a principal holds beyond its roles. + +How a module that stores grants of its own (``permissions``' per-user grants) +gets them into the one set every check reads, without the framework importing +it (SM009). Called once per authenticated request by +``simple_module_hosting.permissions.resolve_principal_permissions``, so a +source that reads the database must cache: it sits on the hot path of every +page load. +""" + + @dataclass class PermissionGroup: """A named group of related permissions (typically one per module).""" @@ -75,6 +88,7 @@ def __init__(self) -> None: self._role_overlay: dict[str, set[str]] = {} self._all_permissions_cache: list[str] | None = None self._role_map_cache: dict[str, list[str]] | None = None + self._grant_sources: list[GrantSource] = [] self._sources: dict[str, PermissionSourceProvider] = {} self._source_cache: dict[str, tuple[list[str], dict[str, str]]] = {} @@ -181,6 +195,22 @@ def map_role(self, role: str, permissions: list[str]) -> None: self._role_map[role].update(permissions) self._invalidate() + def add_grant_source(self, source: GrantSource) -> None: + """Contribute permissions a principal holds beyond its roles (GH #337). + + Whatever *source* returns is merged into the request's resolved set, so + ``RequiresPermission``, ``resolved_permissions_for``, the menu filter and + the frontend's ``auth.permissions`` all honour it. Before this seam the + ``permissions`` module's direct grants were seen by its own dependency + and nothing else, so a grant made in the admin UI did nothing on any + route guarded by the framework's ``RequiresPermission``. + """ + self._grant_sources.append(source) + + @property + def grant_sources(self) -> tuple[GrantSource, ...]: + return tuple(self._grant_sources) + def set_role_overlay(self, role: str, permissions: Collection[str]) -> None: """Replace *role*'s persisted (admin-editor) grants. diff --git a/framework/hosting/simple_module_hosting/middleware.py b/framework/hosting/simple_module_hosting/middleware.py index f2d076b2..04c35d9a 100644 --- a/framework/hosting/simple_module_hosting/middleware.py +++ b/framework/hosting/simple_module_hosting/middleware.py @@ -27,7 +27,7 @@ RequestLoggingMiddleware, ) from simple_module_hosting._tenant import TENANT_HEADER, TenantMiddleware, TenantResolver -from simple_module_hosting.permissions import expand_permissions, resolve_permissions +from simple_module_hosting.permissions import expand_permissions, resolve_principal_permissions if TYPE_CHECKING: from simple_module_core.menu import MenuRegistry @@ -200,9 +200,10 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: is_authenticated = user is not None roles = getattr(user, "roles", []) if user else [] - # Resolve permissions once and cache on request.state for RequiresPermission + # Resolve permissions once — roles plus module grant sources (GH #337) — + # and cache on request.state for RequiresPermission and the menu filter. resolved = ( - resolve_permissions(roles, role_map=self.permission_registry.role_map) + await resolve_principal_permissions(request, user, self.permission_registry) if is_authenticated else set() ) diff --git a/framework/hosting/simple_module_hosting/permissions.py b/framework/hosting/simple_module_hosting/permissions.py index 0872555b..e9f1baae 100644 --- a/framework/hosting/simple_module_hosting/permissions.py +++ b/framework/hosting/simple_module_hosting/permissions.py @@ -2,16 +2,26 @@ from __future__ import annotations +import logging +from typing import TYPE_CHECKING, Any + from fastapi import HTTPException, Request from simple_module_core.permissions import DEFAULT_ROLE_PERMISSIONS, WILDCARD, grants +if TYPE_CHECKING: + from simple_module_core.permissions import PermissionRegistry + +logger = logging.getLogger(__name__) + __all__ = [ "DEFAULT_ROLE_PERMISSIONS", "PERMISSION_DENIED_PREFIX", "WILDCARD", "RequiresPermission", + "ensure_resolved_permissions", "expand_permissions", "resolve_permissions", + "resolve_principal_permissions", "resolved_permissions_for", ] @@ -36,6 +46,59 @@ def resolve_permissions( return permissions +async def resolve_principal_permissions( + request: Request, + user: Any, + registry: PermissionRegistry | None, +) -> set[str]: + """Everything *user* holds: its roles' permissions plus every grant source. + + The one resolution both ``InertiaLayoutDataMiddleware`` and + ``RequiresPermission`` use, so the door, the menu and the frontend cannot + disagree about what a principal may do (GH #337). + + A source that raises contributes nothing — failing closed, a denied page + rather than an unauthorised one — and is logged rather than turned into a + 500 on every request. A principal already holding the wildcard skips the + sources: nothing they return could widen it, and a source may cost a read. + """ + role_map = registry.role_map if registry is not None else None + permissions = resolve_permissions(getattr(user, "roles", []), role_map=role_map) + if registry is None or WILDCARD in permissions: + return permissions + for source in registry.grant_sources: + try: + # Grants are additive keys only: the wildcard (admin) comes from + # roles, never from a source, so a stray ``*`` row can't escalate. + permissions.update(k for k in await source(request, user) if k != WILDCARD) + except Exception: + logger.exception("Grant source %r raised; contributing nothing", source) + return permissions + + +def _registry_for(request: Request) -> PermissionRegistry | None: + sm = getattr(getattr(request.app, "state", None), "sm", None) + return getattr(sm, "permissions", None) if sm is not None else None + + +async def ensure_resolved_permissions(request: Request) -> set[str]: + """:func:`resolved_permissions_for`, grant sources included on a cache miss. + + The middleware has normally resolved and cached the set already; this is + the fallback for a bare router without it, which — unlike the synchronous + reader — can await the grant sources. + """ + cached: set[str] | None = getattr(request.state, "resolved_permissions", None) + if cached is not None: + return cached + user = getattr(request.state, "user", None) + if user is None: + return set() + permissions = await resolve_principal_permissions(request, user, _registry_for(request)) + request.state.resolved_permissions = permissions + return permissions + + def expand_permissions( resolved: set[str], all_permissions: list[str], @@ -59,9 +122,11 @@ def resolved_permissions_for(request: Request) -> set[str]: labels, for one — and that decision must read the same permission set the door did. - Role-derived only, matching ``RequiresPermission``: the ``permissions`` - module's direct per-user grants are its own dependency's business, and a - caller wanting those consults ``permissions.deps.RequiresPermission``. + Includes every registered grant source (the ``permissions`` module's + direct per-user grants) whenever the middleware or ``RequiresPermission`` + resolved first — which is always in a real app. Only the bare-router + fallback below is role-derived, because it cannot await a source; a caller + that may run there first should use :func:`ensure_resolved_permissions`. """ cached: set[str] | None = getattr(request.state, "resolved_permissions", None) if cached is not None: @@ -71,8 +136,7 @@ def resolved_permissions_for(request: Request) -> set[str]: if user is None: return set() - sm = getattr(getattr(request.app, "state", None), "sm", None) - perm_registry = getattr(sm, "permissions", None) if sm is not None else None + perm_registry = _registry_for(request) role_map = perm_registry.role_map if perm_registry is not None else None permissions = resolve_permissions(user.roles, role_map=role_map) request.state.resolved_permissions = permissions @@ -82,6 +146,9 @@ def resolved_permissions_for(request: Request) -> set[str]: class RequiresPermission: """FastAPI dependency that enforces a specific permission. + Honours roles *and* every registered grant source, so a direct per-user + grant from the ``permissions`` module takes effect here too. + Usage:: @router.post("/", dependencies=[Depends(RequiresPermission("products.create"))]) @@ -92,12 +159,12 @@ async def create_product(...): def __init__(self, permission: str) -> None: self.permission = permission - def __call__(self, request: Request) -> None: + async def __call__(self, request: Request) -> None: user = getattr(request.state, "user", None) if user is None: raise HTTPException(status_code=401, detail="Authentication required") - if not grants(resolved_permissions_for(request), self.permission): + if not grants(await ensure_resolved_permissions(request), self.permission): raise HTTPException( status_code=403, detail=f"{PERMISSION_DENIED_PREFIX}{self.permission}", diff --git a/framework/hosting/tests/test_grant_sources.py b/framework/hosting/tests/test_grant_sources.py new file mode 100644 index 00000000..73941b71 --- /dev/null +++ b/framework/hosting/tests/test_grant_sources.py @@ -0,0 +1,122 @@ +"""Grant sources reach the door, the menu and the frontend alike (GH #337).""" + +from __future__ import annotations + +from types import SimpleNamespace + +from fastapi import Depends, FastAPI +from httpx import ASGITransport, AsyncClient +from simple_module_core.menu import MenuRegistry +from simple_module_core.permissions import PermissionRegistry +from simple_module_hosting.middleware import InertiaLayoutDataMiddleware +from simple_module_hosting.permissions import RequiresPermission, resolve_principal_permissions + + +def _registry(*sources) -> PermissionRegistry: + reg = PermissionRegistry() + reg.add_group("orders", ["orders.view", "orders.edit"]) + for source in sources: + reg.add_grant_source(source) + return reg + + +def _user(roles: list[str] | None = None) -> SimpleNamespace: + return SimpleNamespace(id="u1", roles=roles or []) + + +async def _grants_view(_request, _user): + return {"orders.view"} + + +async def _raises(_request, _user): + raise RuntimeError("database down") + + +class TestResolvePrincipalPermissions: + async def test_merges_source_with_roles(self): + reg = _registry(_grants_view) + reg.map_role("clerk", ["orders.edit"]) + held = await resolve_principal_permissions(None, _user(["clerk"]), reg) + assert held == {"orders.view", "orders.edit"} + + async def test_source_cannot_grant_the_wildcard(self): + async def _wildcard(_request, _user): + return {"*", "orders.view"} + + held = await resolve_principal_permissions(None, _user(), _registry(_wildcard)) + assert held == {"orders.view"} + + async def test_raising_source_fails_closed(self): + held = await resolve_principal_permissions(None, _user(), _registry(_raises, _grants_view)) + assert held == {"orders.view"} + + async def test_wildcard_skips_sources(self): + calls: list[str] = [] + + async def counting(_request, _user): + calls.append("called") + return set() + + held = await resolve_principal_permissions(None, _user(["admin"]), _registry(counting)) + assert "*" in held + assert calls == [] + + +class TestMiddlewareFoldsSources: + async def test_frontend_permissions_include_source(self): + captured: dict = {} + + async def inner_app(scope, receive, send): + from starlette.requests import Request + + captured["shared"] = Request(scope).state.inertia_shared + + mw = InertiaLayoutDataMiddleware( + inner_app, menu_registry=MenuRegistry(), permission_registry=_registry(_grants_view) + ) + scope = { + "type": "http", + "method": "GET", + "path": "/", + "headers": [], + "state": {"user": _user()}, + "app": SimpleNamespace(state=SimpleNamespace()), + } + + async def receive(): + return {"type": "http.request", "body": b"", "more_body": False} + + async def send(_message): + return None + + await mw(scope, receive, send) + assert captured["shared"]["auth"]["permissions"] == ["orders.view"] + + +class TestRequiresPermissionWithoutMiddleware: + """A bare router has no middleware to resolve first; the door must still await sources.""" + + def _app(self, reg: PermissionRegistry) -> FastAPI: + app = FastAPI() + app.state.sm = SimpleNamespace(permissions=reg) + + @app.middleware("http") + async def set_user(request, call_next): + request.state.user = _user() + return await call_next(request) + + @app.get("/view", dependencies=[Depends(RequiresPermission("orders.view"))]) + async def view(): + return {"ok": True} + + @app.get("/edit", dependencies=[Depends(RequiresPermission("orders.edit"))]) + async def edit(): + return {"ok": True} + + return app + + async def test_source_grant_admits_and_absence_denies(self): + transport = ASGITransport(app=self._app(_registry(_grants_view))) + async with AsyncClient(transport=transport, base_url="http://t") as client: + assert (await client.get("/view")).status_code == 200 + assert (await client.get("/edit")).status_code == 403 diff --git a/modules/auth/auth/deps.py b/modules/auth/auth/deps.py index 7bac3e0f..50ae670e 100644 --- a/modules/auth/auth/deps.py +++ b/modules/auth/auth/deps.py @@ -5,7 +5,9 @@ from typing import Annotated from fastapi import Depends, HTTPException, Request +from simple_module_core.permissions import grants from simple_module_hosting.i18n_deps import TranslatorDep +from simple_module_hosting.permissions import ensure_resolved_permissions from auth.contracts.schemas import UserContext @@ -44,11 +46,11 @@ async def check( if _ADMIN_ROLE in user.roles: return - # Get permission registry from app state - perm_registry = request.app.state.sm.permissions - user_perms = perm_registry.get_permissions_for_roles(user.roles) - - if not any(p in user_perms for p in permissions): + # The same resolution RequiresPermission reads: the registry's role map + # plus module grant sources. This used to call get_permissions_for_roles + # without a map, which grants every non-admin role nothing (GH #337). + held = await ensure_resolved_permissions(request) + if not any(grants(held, p) for p in permissions): raise HTTPException( status_code=403, detail=t.t( diff --git a/modules/auth/tests/test_deps.py b/modules/auth/tests/test_deps.py index 0f6e2297..07bbd3ca 100644 --- a/modules/auth/tests/test_deps.py +++ b/modules/auth/tests/test_deps.py @@ -27,6 +27,14 @@ def _translator() -> Translator: return Translator(registry, locale="en", default_locale="en") +def _request(app, user: UserContext) -> MagicMock: + """A request whose state is real, so the permission cache reads as absent.""" + request = MagicMock() + request.app.state.sm = SimpleNamespace(permissions=app.state.sm.permissions) + request.state = SimpleNamespace(user=user) + return request + + class TestGetCurrentUser: async def test_raises_401_when_no_user(self): """get_current_user raises 401 when request.state has no user.""" @@ -53,10 +61,8 @@ async def test_raises_403_when_missing_permission(self, app): dep = require_permission("products.delete") check_fn = dep.dependency - request = MagicMock() - request.app.state.sm = SimpleNamespace(permissions=app.state.sm.permissions) - user = UserContext(id="u1", email="u@test.com", name="User", roles=["viewer"]) + request = _request(app, user) with pytest.raises(HTTPException) as exc_info: await check_fn(request, _translator(), user) @@ -67,10 +73,8 @@ async def test_admin_bypasses_permission_check(self, app): dep = require_permission("products.delete") check_fn = dep.dependency - request = MagicMock() - request.app.state.sm = SimpleNamespace(permissions=app.state.sm.permissions) - admin_user = UserContext(id="a1", email="admin@test.com", name="Admin", roles=["admin"]) + request = _request(app, admin_user) await check_fn(request, _translator(), admin_user) @@ -80,21 +84,28 @@ async def test_multiple_permissions_any_match(self, app): dep = require_permission("products.view", "products.edit") check_fn = dep.dependency - request = MagicMock() - request.app.state.sm = SimpleNamespace(permissions=app.state.sm.permissions) - admin = UserContext(id="a1", email="a@t.com", name="Admin", roles=["admin"]) + request = _request(app, admin) await check_fn(request, _translator(), admin) async def test_non_admin_without_permission_fails(self, app): dep = require_permission("products.delete") check_fn = dep.dependency - request = MagicMock() - request.app.state.sm = SimpleNamespace(permissions=app.state.sm.permissions) - user = UserContext(id="u1", email="u@t.com", name="User", roles=["user"]) + request = _request(app, user) with pytest.raises(HTTPException) as exc_info: await check_fn(request, _translator(), user) assert exc_info.value.status_code == 403 assert "products.delete" in str(exc_info.value.detail) + + async def test_non_admin_role_mapped_in_registry_passes(self, app): + """A role granted the key via the registry's role map is admitted. + + Regression for GH #337: the check resolved roles without the role map, + so every non-admin role held nothing and this was a 403. + """ + app.state.sm.permissions.map_role("clerk", ["products.delete"]) + dep = require_permission("products.delete") + user = UserContext(id="u1", email="u@t.com", name="User", roles=["clerk"]) + await dep.dependency(_request(app, user), _translator(), user) diff --git a/modules/permissions/README.md b/modules/permissions/README.md index f1fc7b2a..9e59e0f7 100644 --- a/modules/permissions/README.md +++ b/modules/permissions/README.md @@ -13,7 +13,7 @@ pip install simple_module_permissions ## What it provides - `Role` and `Permission` SQLModel tables, seeded from module-registered defaults. -- `RequiresPermission("...")` FastAPI dependency that honours both role-based and direct user-level grants (the framework also exposes `require_permission(...)` from `auth.deps`). +- Direct user-level grants feed the framework's permission resolution through a grant source, so `simple_module_hosting.permissions.RequiresPermission` (re-exported as `permissions.deps.RequiresPermission`) and `auth.deps.require_permission(...)` both honour them. - Admin UI for assigning roles/permissions to users, reached through the users admin area at `/users/admin`; role and user editors live at `/permissions/roles/{id}/edit` and `/permissions/users/{id}/edit`. - `register_permissions(self, registry)` hook — every module declares its permission strings at boot via `registry.add_group(...)`; the registry dedupes and persists them. diff --git a/modules/permissions/permissions/deps.py b/modules/permissions/permissions/deps.py index 627e04e7..fc9f7d30 100644 --- a/modules/permissions/permissions/deps.py +++ b/modules/permissions/permissions/deps.py @@ -1,25 +1,24 @@ -"""FastAPI dependencies for the Permissions module. - -In addition to the standard service wiring this module exports a -:class:`RequiresPermission` dependency that honours *both* role-based -and direct user grants — the framework's own -:class:`simple_module_hosting.permissions.RequiresPermission` checks -only roles, because the framework has no concept of user-direct grants. -Endpoints that want users to be able to hold individual permissions on -top of their roles should depend on this version instead. -""" +"""FastAPI dependencies for the Permissions module.""" from __future__ import annotations -import uuid - -from fastapi import Depends, HTTPException, Request -from simple_module_core.permissions import WILDCARD, PermissionRegistry +from fastapi import Depends, Request +from simple_module_core.permissions import PermissionRegistry from simple_module_db.deps import get_db +from simple_module_hosting.permissions import RequiresPermission as _HostingRequiresPermission from sqlalchemy.ext.asyncio import AsyncSession +from permissions.grants import publish_grants_changed from permissions.service import PermissionService +__all__ = [ + "RequiresPermission", + "assigned_by", + "get_permission_registry", + "get_permission_service", + "invalidate_grants_on_commit", +] + def get_permission_registry(request: Request) -> PermissionRegistry: return request.app.state.sm.permissions @@ -38,35 +37,17 @@ def assigned_by(request: Request) -> str | None: return str(user.id) if user is not None else None -class RequiresPermission: - """FastAPI dependency enforcing a permission across roles *and* user grants. +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.""" + service.db.on_commit(lambda: publish_grants_changed(request.app, user_id)) - Behaves like the framework's ``simple_module_hosting.RequiresPermission`` - but additionally consults the ``permissions_user_permission`` table, so - a direct grant on a single user takes effect without inventing a role. - """ - def __init__(self, permission: str) -> None: - self.permission = permission +RequiresPermission = _HostingRequiresPermission +"""Kept for existing imports: the framework's class now honours direct grants. - async def __call__( - self, - request: Request, - service: PermissionService = Depends(get_permission_service), - ) -> None: - user = getattr(request.state, "user", None) - if user is None: - raise HTTPException(status_code=401, detail="Authentication required") - - role_perms: set[str] = getattr(request.state, "resolved_permissions", set()) or set() - if WILDCARD in role_perms or self.permission in role_perms: - return - - direct = await service.get_user_direct_keys(uuid.UUID(str(user.id))) - if self.permission in direct: - return - - raise HTTPException( - status_code=403, - detail=f"Permission required: {self.permission}", - ) +This used to be a separate class that read the ``permissions_user_permission`` +table itself, while the framework's read roles only — so a direct grant worked +on this module's routes and nowhere else (GH #337). The grants now reach every +check through :func:`permissions.grants.direct_grant_source`, and one class is +the only way the two cannot drift apart again. +""" diff --git a/modules/permissions/permissions/endpoints/api.py b/modules/permissions/permissions/endpoints/api.py index 81049e30..984c8477 100644 --- a/modules/permissions/permissions/endpoints/api.py +++ b/modules/permissions/permissions/endpoints/api.py @@ -14,7 +14,12 @@ UserPermissionsOut, UserPermissionsUpdate, ) -from permissions.deps import RequiresPermission, assigned_by, get_permission_service +from permissions.deps import ( + RequiresPermission, + assigned_by, + get_permission_service, + invalidate_grants_on_commit, +) from permissions.service import PermissionService router = APIRouter() @@ -99,4 +104,5 @@ 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 808316b3..cfeae1ba 100644 --- a/modules/permissions/permissions/endpoints/views.py +++ b/modules/permissions/permissions/endpoints/views.py @@ -16,7 +16,12 @@ from permissions.constants import PERM_MANAGE from permissions.contracts.schemas import RolePermissionsUpdate, UserPermissionsUpdate -from permissions.deps import RequiresPermission, assigned_by, get_permission_service +from permissions.deps import ( + RequiresPermission, + assigned_by, + get_permission_service, + invalidate_grants_on_commit, +) from permissions.service import PermissionService router = APIRouter() @@ -121,4 +126,5 @@ 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/grants.py b/modules/permissions/permissions/grants.py new file mode 100644 index 00000000..82094bb9 --- /dev/null +++ b/modules/permissions/permissions/grants.py @@ -0,0 +1,122 @@ +"""Direct per-user grants as a framework grant source, cached per process. + +Registered on the :class:`~simple_module_core.permissions.PermissionRegistry` +so the framework's one permission resolution — the door (``RequiresPermission``), +the menu filter and the frontend's ``auth.permissions`` — honours a grant made +on the permissions screen. Before this, only ``permissions.deps`` read the +table, so the same grant was a 200 on this module's routes and a 403 on every +other module's (GH #337). + +The source runs on every authenticated request, so it caches the same way +``users.session_version_cache`` does: a bounded TTL as the floor, and an +``InvalidationBus`` message so a change made in one worker drops the entry in +every worker that has a transport. The worker that made the change always +drops its own entry, because it is a local subscriber like any other. +""" + +from __future__ import annotations + +import logging +import uuid +from typing import TYPE_CHECKING, Any + +from cachetools import TTLCache +from sqlalchemy import select + +from permissions.models import UserPermission + +if TYPE_CHECKING: + from fastapi import FastAPI, Request + from simple_module_core.invalidation import Invalidation, InvalidationBus + +logger = logging.getLogger(__name__) + +__all__ = [ + "CHANNEL", + "GRANTS_TTL_SECONDS", + "apply_invalidation", + "clear_grants_cache", + "direct_grant_source", + "publish_grants_changed", + "subscribe", +] + +CHANNEL = "permissions.user_grants" +"""Carries one user id, or ``None`` for "forget every user".""" + +GRANTS_TTL_SECONDS = 30 +"""How long one user's direct grants are reused without re-reading. + +The window only matters for a revocation made in *another* worker with no +invalidation transport installed; see the module docstring. +""" + +_GRANTS: TTLCache = TTLCache(maxsize=10_000, ttl=GRANTS_TTL_SECONDS) +_MISS = object() + + +def clear_grants_cache() -> None: + """Empty the cache — for tests that need a cold read.""" + _GRANTS.clear() + + +def _user_uuid(raw: Any) -> uuid.UUID | None: + """The user id as the table stores it, or ``None`` for a non-UUID principal. + + A principal resolver may authenticate something that is not a ``users`` + row (an API key, a service account); it holds no direct grants rather + than turning every one of its requests into a logged exception. + """ + try: + return uuid.UUID(str(raw)) + except (ValueError, TypeError, AttributeError): + return None + + +async def direct_grant_source(request: Request, user: Any) -> frozenset[str]: + """The keys granted directly to *user*. A ``GrantSource``.""" + user_id = _user_uuid(getattr(user, "id", None)) + if user_id is None: + return frozenset() + # One ``get``, not ``in`` then ``[]``: a TTLCache entry can expire between. + cached = _GRANTS.get(user_id, _MISS) + if cached is not _MISS: + return cached + session_factory = request.app.state.sm.db.session_factory + async with session_factory() as db: + result = await db.execute( + select(UserPermission.permission_key).where(UserPermission.user_id == user_id) + ) + keys = frozenset(result.scalars().all()) + _GRANTS[user_id] = keys + return keys + + +def apply_invalidation(invalidation: Invalidation) -> None: + """Drop the cached grants named by *invalidation*. The bus handler.""" + if invalidation.key is None: + _GRANTS.clear() + return + user_id = _user_uuid(invalidation.key) + if user_id is not None: + _GRANTS.pop(user_id, None) + + +def subscribe(bus: InvalidationBus) -> None: + bus.subscribe(CHANNEL, apply_invalidation) + + +async def publish_grants_changed(app: FastAPI, user_id: uuid.UUID) -> None: + """Announce that *user_id*'s direct grants changed. + + Call it from a ``db.on_commit`` callback: evicting before the rows are + durable lets the next read re-cache the old grants, and a rolled-back + change must not be broadcast. Falls back to a local eviction when the app + has no bus (a single-module test harness), so a change never looks inert. + """ + bus = getattr(getattr(app.state, "sm", None), "invalidation", None) + if bus is None: + logger.debug("No invalidation bus on this app; dropping %s locally", user_id) + _GRANTS.pop(user_id, None) + return + await bus.publish(CHANNEL, key=str(user_id)) diff --git a/modules/permissions/permissions/module.py b/modules/permissions/permissions/module.py index 4d113254..2968bb02 100644 --- a/modules/permissions/permissions/module.py +++ b/modules/permissions/permissions/module.py @@ -16,6 +16,7 @@ if TYPE_CHECKING: from fastapi import FastAPI + from simple_module_core.invalidation import InvalidationBus from sqlalchemy.ext.asyncio import AsyncSession @@ -78,11 +79,21 @@ def register_audit_links(self, registry: AuditLinkRegistry) -> None: def register_permissions(self, registry: PermissionRegistry) -> None: from permissions.constants import PERM_MANAGE, PERM_VIEW, PERMISSION_GROUP + from permissions.grants import direct_grant_source registry.add_group( PERMISSION_GROUP, [PERM_VIEW, PERM_MANAGE], ) + # Direct per-user grants join the framework's one resolution, so every + # module's RequiresPermission, the menu and the frontend honour them. + registry.add_grant_source(direct_grant_source) + + def register_invalidations(self, bus: InvalidationBus, app: FastAPI) -> None: + """Let another worker's grant change drop this worker's cached grants.""" + from permissions.grants import subscribe + + subscribe(bus) def locale_dirs(self) -> dict[str, Path]: base = Path(str(importlib.resources.files(__package__) / "locales")) diff --git a/modules/permissions/pyproject.toml b/modules/permissions/pyproject.toml index 5d2e3ca5..8924fb1c 100644 --- a/modules/permissions/pyproject.toml +++ b/modules/permissions/pyproject.toml @@ -21,6 +21,7 @@ classifiers = [ "Typing :: Typed", ] dependencies = [ + "cachetools>=5.3", "simple_module_core==0.0.35", "simple_module_db==0.0.35", "simple_module_hosting==0.0.35", diff --git a/modules/permissions/tests/test_direct_grants_everywhere.py b/modules/permissions/tests/test_direct_grants_everywhere.py new file mode 100644 index 00000000..28a3897d --- /dev/null +++ b/modules/permissions/tests/test_direct_grants_everywhere.py @@ -0,0 +1,95 @@ +"""A direct grant opens routes guarded by the framework's RequiresPermission (GH #337). + +The reproduction from the issue: a user with no roles is granted a module's +``view`` permission on the permissions screen. Before the fix the grant was +stored and returned 200, and the user still got 403 on every route of a module +that imported ``simple_module_hosting.permissions.RequiresPermission``. +""" + +from __future__ import annotations + +import uuid + +import httpx +import pytest +from fastapi import FastAPI +from feature_flags.constants import PERM_FEATURE_FLAGS_VIEW, VIEW_PREFIX +from permissions.deps import RequiresPermission as ModuleRequiresPermission +from permissions.grants import clear_grants_cache +from simple_module_hosting.permissions import RequiresPermission +from simple_module_test import forge_session_cookie + +_GUARDED = "/api/feature_flags/" + + +@pytest.fixture(autouse=True) +def _cold_cache(): + clear_grants_cache() + yield + clear_grants_cache() + + +async def _seed_roleless_user(app: FastAPI) -> uuid.UUID: + from users.models import User + + user_id = uuid.uuid4() + async with app.state.sm.db.session_factory() as db: + db.add( + User( + id=user_id, + email=f"{user_id.hex[:8]}@test", + hashed_password="x", + is_active=True, + is_superuser=False, + is_verified=True, + ) + ) + await db.commit() + return user_id + + +def _client_for(app: FastAPI, user_id: uuid.UUID) -> httpx.AsyncClient: + cookie = forge_session_cookie(app.state.sm.settings.secret_key, {"user_id": str(user_id)}) + return httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://testserver", + cookies={"session": cookie}, + ) + + +def test_module_class_is_the_framework_class(): + assert ModuleRequiresPermission is RequiresPermission + + +async def test_grant_and_revoke_take_effect_on_framework_guarded_route( + app: FastAPI, authenticated_client: httpx.AsyncClient +): + user_id = await _seed_roleless_user(app) + async with _client_for(app, user_id) as user: + # Also warms the cache with "no grants" — the grant below must evict it. + assert (await user.get(_GUARDED)).status_code == 403 + + granted = await authenticated_client.put( + f"/api/permissions/users/{user_id}", json={"permissions": [PERM_FEATURE_FLAGS_VIEW]} + ) + assert granted.status_code == 200 + assert (await user.get(_GUARDED)).status_code == 200 + + revoked = await authenticated_client.put( + f"/api/permissions/users/{user_id}", json={"permissions": []} + ) + assert revoked.status_code == 200 + assert (await user.get(_GUARDED)).status_code == 403 + + +async def test_grant_reaches_inertia_auth_permissions( + app: FastAPI, authenticated_client: httpx.AsyncClient +): + user_id = await _seed_roleless_user(app) + await authenticated_client.put( + f"/api/permissions/users/{user_id}", json={"permissions": [PERM_FEATURE_FLAGS_VIEW]} + ) + async with _client_for(app, user_id) as user: + resp = await user.get(f"{VIEW_PREFIX}/", headers={"X-Inertia": "true"}) + assert resp.status_code == 200 + assert PERM_FEATURE_FLAGS_VIEW in resp.json()["props"]["auth"]["permissions"]