Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion docs/framework/lifecycle.md
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,11 @@ def register_audit_links(self, registry: AuditLinkRegistry) -> None:

`label_key` translates the label the same way `MenuItem.label_key` does, falling back to `label` when the key resolves to nothing. Rows are rendered server-side, so the audit view translates these before they reach the page.

The registry maps class names to URL templates and nothing more: it does not check that the row still exists or that the reader may open it. A link to a deleted record lands on the target screen's own 404, and permissions are enforced by the target route as usual.
`label_resolver` names individual rows — a batch callable taking the request session and every id of that type on the page, returning `{id: display name}`. Only the owning module knows that a `Setting` is named by its key and a `User` by `full_name or email`, and without one the reader gets a bare primary key.

`label_permission` gates that name. **Set it whenever naming the row discloses something the module gates elsewhere.** The name travels inside the audit payload, so no downstream route ever gets the chance to refuse it — a reader holding `audit_log.view` and nothing else would otherwise read the display name (and, for accounts with an outstanding invite, the email) of every account that has ever been edited. `users` therefore declares `users.manage`. Readers without it still see the entry, the action and the id; they just do not get the name. Leaving it empty — the default — says the name is safe for anyone who may read the trail at all, which is true of setting keys, filenames and task names.

A link is a route, not an authorisation: apart from `label_permission`, the registry does not check that the row still exists or that the reader may open it. A link to a deleted record lands on the target screen's own 404, and permissions are enforced by the target route as usual.

## `on_startup()` / `on_shutdown()`

Expand Down
2 changes: 1 addition & 1 deletion docs/framework/permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,5 +150,5 @@ async def test_create_requires_permission(client, db_session):

- **Namespace with the module name.** `orders.create`, not `create_order`.
- **Use `<module>.<verb>` form.** Nouns like `orders.admin` are fine for broad grants.
- **Don't inline string checks.** `if "orders.create" in principal.permissions: ...` scattered through code is hard to audit. Use `RequiresPermission` or refactor into a dependency.
- **Don't inline string checks.** `if "orders.create" in principal.permissions: ...` scattered through code is hard to audit. Use `RequiresPermission` or refactor into a dependency. When the thing to gate is part of a *response* rather than a route — a column, a label, a card — read the caller's grants with `resolved_permissions_for(request)` and test them with `simple_module_core.permissions.grants`, which is where the wildcard rule lives. A hand-rolled `in` check forgets that `admin` holds `*` and no named permission at all.
- **Don't use roles in business logic.** Check permissions, not `user.roles`. Roles are an admin concept; permissions are the enforcement boundary.
14 changes: 13 additions & 1 deletion docs/modules/audit_log.md
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,26 @@ from audit_log.contracts.schemas import AuditEntryRead, AuditEntryList

The table itself carries `__audit_exclude__ = True` so audit writes never re-enter the capture loop.

## Entity and actor names

An entry stores a model class name and a primary key, which proves what happened and names nobody. The browse screen, the CSV export and the users edit page's activity card all resolve those ids at render time through the audit-link registry: each module supplies a batch `label_resolver` naming its own rows, one query per entity type per page. Nothing is stored — the row keeps the ids it recorded.

**The entity column's names are permission-gated.** An `AuditLink` may declare a `label_permission`, and a reader who does not hold it gets the id instead of the name. `users` declares `users.manage`, because a user row is named by `full_name or email` — so without the gate, `audit_log.view` alone was a way to read the display name of every account that appears in the log, and the email address of every account still holding an unaccepted invite (those have no `full_name` yet). The entry itself is never withheld: the action, the type, the timestamp and the id are the audit trail. Links whose owner declares nothing — setting keys, filenames, task names, flag names — are shown to anyone who may read the trail.

**The actor column's names are not.** `audit_log.view` does imply "may see who acted", and that is deliberate: an audit trail that will not say who performed a change is not one. The two columns disclose the same field and answer different questions — the entity column names people who were merely *edited*, which reviewing the trail does not require. Grant `audit_log.view` knowing it discloses the display name (or email) of everyone who has ever made a change.

The `changes` column is a third surface, and a wider one: it records the values that were written, so a `User` create entry contains the email that was set. Use `__audit_exclude_fields__` on a model to keep a column out of the diff (see § Opting out).

## Permissions

| Code | Purpose |
|---|---|
| `audit_log.view` | read the audit trail (API + browse page) |
| `audit_log.view` | read the audit trail (API + browse page + CSV export), including the name of every account that has *acted* |

There is no write permission — the trail is append-only and written by the framework, never edited through the UI.

`audit_log.view` does **not** confer the name of an account that merely appears as the *subject* of an entry — that takes `users.manage`, the same permission that opens the user list. See § Entity and actor names.

## Menu

| Label | URL | Icon | Section | Group | Order |
Expand Down
2 changes: 2 additions & 0 deletions docs/modules/users.md
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@ Everything else is DB-backed (initial values are pydantic defaults; edit under U
| `users.manage` | admin: list / invite / disable / role-assign |
| `users.self.profile` | edit own profile (granted to `user` role) |

`users.manage` also unlocks account *names* in the audit log's entity column: the module registers its audit link with `label_permission="users.manage"`, so a reader holding only `audit_log.view` sees the id of an edited account rather than its display name or email. See [audit_log](/modules/audit_log) § Entity and actor names.

## Menu

| Label | URL | Icon | Section | Group | Order | Roles |
Expand Down
23 changes: 19 additions & 4 deletions framework/core/simple_module_core/audit_links.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,15 @@
collects them into one registry at boot and stores it on
``app.state.sm.audit_links``.

**The registry maps model class names to URL templates, nothing more.** It does
not verify the row exists or that the reader may open it — following a link to a
deleted record lands on that screen's own 404, and permissions are enforced by
the target route as usual.
**A link is a route, not an authorisation.** The registry does not verify the
row exists or that the reader may open it — following a link to a deleted
record lands on that screen's own 404, and permissions are enforced by the
target route as usual.

The one exception is ``label_permission``: the *name* of a row travels in the
audit payload itself, so no downstream route ever gets the chance to refuse it.
A module whose row names disclose something it gates elsewhere says so there,
and readers without that permission see the id instead.
"""

from __future__ import annotations
Expand Down Expand Up @@ -76,6 +81,15 @@ class name.
a closure, and two boots would build two unequal objects out of one
module — a registry conflict is about two modules claiming one
entity type, which the identity fields already express.
label_permission: Permission a reader must hold before
``label_resolver`` is *run* for them. Empty — the default — means
the name is safe for anyone allowed to read the audit trail at all.
Set it when naming the row discloses something the module gates
elsewhere: ``users`` names an account by ``full_name or email``, so
without this an ``audit_log.view`` grant is a back door onto the
user directory that ``users.manage`` guards at the front. Denied
readers still see the row, the action and the id — the audit trail
stays complete, it just stops volunteering the name.
"""

entity_type: str
Expand All @@ -84,6 +98,7 @@ class name.
label_key: str = ""
table_name: str | None = None
label_resolver: LabelResolver | None = field(default=None, compare=False)
label_permission: str = ""

def __post_init__(self) -> None:
if self.url_template and _ID_PLACEHOLDER not in self.url_template:
Expand Down
18 changes: 18 additions & 0 deletions framework/core/simple_module_core/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from __future__ import annotations

from collections.abc import Collection
from dataclasses import dataclass, field

WILDCARD = "*"
Expand All @@ -26,6 +27,23 @@ def is_admin(roles: list[str] | None) -> bool:
return bool(roles) and ADMIN_ROLE in roles


def grants(held: Collection[str], required: str) -> bool:
"""Whether a principal holding *held* satisfies *required*.

One place for the wildcard rule, so code that gates something *inside* a
response — a column, a label, a card — reads the grant the same way
``RequiresPermission`` reads it at the door. Two hand-rolled ``in`` checks
are two chances to forget that ``admin`` holds ``*`` and nothing else.

An empty *required* is satisfied by anything: callers use it to mean "no
permission gates this", which is the default for a declaration that never
named one.
"""
if not required:
return True
return WILDCARD in held or required in held


@dataclass
class PermissionGroup:
"""A named group of related permissions (typically one per module)."""
Expand Down
19 changes: 19 additions & 0 deletions framework/core/tests/test_audit_links.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,25 @@ def test_registering_twice_with_distinct_closures_is_idempotent(self):
)
assert len(reg.all_links) == 1

def test_label_permission_defaults_to_ungated(self):
"""Most row names disclose nothing a reader of the audit log should not
already see — a setting key, a filename — so declaring nothing means
"anyone who may read the trail may read the name"."""
assert AuditLink(entity_type="Setting", url_template="/s/{id}").label_permission == ""

def test_label_permission_is_part_of_the_claim(self):
"""Unlike the resolver, it is compared: two modules disagreeing about
who may see a name is exactly the conflict the registry exists to
catch, and silently keeping the looser one would be the wrong answer."""
reg = AuditLinkRegistry()
reg.register(AuditLink(entity_type="User", url_template="/u/{id}"))
with pytest.raises(ValueError, match="Two modules claim"):
reg.register(
AuditLink(
entity_type="User", url_template="/u/{id}", label_permission="users.manage"
)
)

def test_genuinely_conflicting_claims_still_raise(self):
reg = AuditLinkRegistry()
reg.register(AuditLink(entity_type="User", url_template="/a/{id}"))
Expand Down
30 changes: 29 additions & 1 deletion framework/core/tests/test_permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from __future__ import annotations

from simple_module_core.permissions import WILDCARD, PermissionRegistry
from simple_module_core.permissions import WILDCARD, PermissionRegistry, grants


class TestPermissionRegistry:
Expand Down Expand Up @@ -113,3 +113,31 @@ async def test_role_map_returns_plain_dict_of_lists(self):
for key, val in result.items():
assert isinstance(key, str)
assert isinstance(val, list)


class TestGrants:
"""The wildcard rule, in one place.

Anything gating part of a *response* rather than a route reads grants
through here — a second hand-rolled ``in`` check is a second chance to
forget that ``admin`` holds ``*`` and no named permission at all.
"""

async def test_a_held_permission_is_granted(self):
assert grants({"users.manage"}, "users.manage") is True

async def test_a_missing_permission_is_not(self):
assert grants({"audit_log.view"}, "users.manage") is False

async def test_the_wildcard_satisfies_anything(self):
assert grants({WILDCARD}, "users.manage") is True

async def test_an_empty_requirement_is_satisfied_by_nothing_held(self):
"""Empty means "no permission gates this" — the default for a
declaration that never named one, not a requirement nobody meets."""
assert grants(set(), "") is True

async def test_it_reads_any_collection(self):
"""Callers hand it whatever they have — the middleware caches a set,
a test hands a list."""
assert grants(["users.manage"], "users.manage") is True
51 changes: 36 additions & 15 deletions framework/hosting/simple_module_hosting/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from __future__ import annotations

from fastapi import HTTPException, Request
from simple_module_core.permissions import DEFAULT_ROLE_PERMISSIONS, WILDCARD
from simple_module_core.permissions import DEFAULT_ROLE_PERMISSIONS, WILDCARD, grants

__all__ = [
"DEFAULT_ROLE_PERMISSIONS",
Expand All @@ -12,6 +12,7 @@
"RequiresPermission",
"expand_permissions",
"resolve_permissions",
"resolved_permissions_for",
]

# How a denial spells the missing permission in ``HTTPException.detail``.
Expand Down Expand Up @@ -45,6 +46,39 @@ def expand_permissions(
return sorted(resolved)


def resolved_permissions_for(request: Request) -> set[str]:
"""The permission set this request's principal holds, wildcard included.

Prefers what ``InertiaLayoutDataMiddleware`` already resolved and cached on
``request.state``; falls back to resolving from the registry's role map when
the middleware is not in the stack (a bare router under a test transport),
caching the result so a route with several gates resolves once.

Anonymous requests hold nothing. Exposed because a handler sometimes has to
gate part of its *response* rather than the route — the audit log's entity
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``.
"""
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()

sm = getattr(getattr(request.app, "state", None), "sm", None)
perm_registry = getattr(sm, "permissions", None) if sm is not None else None
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
return permissions


class RequiresPermission:
"""FastAPI dependency that enforces a specific permission.

Expand All @@ -63,20 +97,7 @@ def __call__(self, request: Request) -> None:
if user is None:
raise HTTPException(status_code=401, detail="Authentication required")

# Use cached permissions from middleware if available
permissions: set[str] | None = getattr(request.state, "resolved_permissions", None)
if permissions is None:
# Fallback: middleware did not run — consult registry role_map if available
sm = getattr(getattr(request.app, "state", None), "sm", None)
perm_registry = getattr(sm, "permissions", None) if sm is not None else None
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

if WILDCARD in permissions:
return

if self.permission not in permissions:
if not grants(resolved_permissions_for(request), self.permission):
raise HTTPException(
status_code=403,
detail=f"{PERMISSION_DENIED_PREFIX}{self.permission}",
Expand Down
13 changes: 11 additions & 2 deletions modules/audit_log/audit_log/endpoints/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from fastapi import APIRouter, Depends, Query, Request
from fastapi.responses import StreamingResponse
from simple_module_db.deps import get_db
from simple_module_hosting.permissions import RequiresPermission
from simple_module_hosting.permissions import RequiresPermission, resolved_permissions_for
from sqlalchemy.ext.asyncio import AsyncSession

from audit_log.constants import DEFAULT_PAGE_SIZE, PERM_VIEW
Expand Down Expand Up @@ -83,7 +83,16 @@ async def export_audit_entries(
)

return StreamingResponse(
stream_csv(service, db, request.app.state.sm.audit_links, filters),
# Resolved here rather than inside the generator: the response body is
# produced after the endpoint returns, and `request.state` is the
# wrong thing to reach into from a background task.
stream_csv(
service,
db,
request.app.state.sm.audit_links,
filters,
resolved_permissions_for(request),
),
media_type=CSV_MEDIA_TYPE,
headers={"Content-Disposition": f'attachment; filename="{CSV_FILENAME}"'},
)
10 changes: 8 additions & 2 deletions modules/audit_log/audit_log/endpoints/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
from simple_module_db.deps import get_db
from simple_module_hosting.i18n_deps import TranslatorDep
from simple_module_hosting.inertia_deps import InertiaDep
from simple_module_hosting.permissions import RequiresPermission
from simple_module_hosting.permissions import RequiresPermission, resolved_permissions_for
from sqlalchemy.ext.asyncio import AsyncSession

from audit_log.constants import (
Expand Down Expand Up @@ -115,8 +115,14 @@ async def browse(
links = request.app.state.sm.audit_links
entity_types = _type_options(await service.distinct_entity_types(), links)
actors = await resolve_actors(db, [item.user_id for item in result.items])
# Entity names are resolved against what *this* reader may see: naming the
# row is a read of the row, and `audit_log.view` alone must not be a way
# round the permission its owning module puts on that (GH #300).
labels = await resolve_entity_labels(
db, links, [(item.entity_type, item.entity_id) for item in result.items]
db,
links,
[(item.entity_type, item.entity_id) for item in result.items],
resolved_permissions_for(request),
)

items = []
Expand Down
14 changes: 12 additions & 2 deletions modules/audit_log/audit_log/export.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import csv
import io
import json
from collections.abc import AsyncIterator
from collections.abc import AsyncIterator, Collection
from typing import Any

from simple_module_core.audit_links import AuditLinkRegistry
Expand Down Expand Up @@ -97,12 +97,19 @@ async def stream_csv(
db: AsyncSession,
registry: AuditLinkRegistry,
filters: EntryFilters,
permissions: Collection[str],
) -> AsyncIterator[str]:
"""Yield the CSV a batch at a time, header first.

Names are resolved per batch, with the same batched lookups the screen
uses — a 5,000-row export costs one actor query and one query per entity
type per batch, not one per row.

*permissions* is the requesting principal's, and it reaches the entity
resolvers exactly as it does on the screen. The export is a second door
onto the same rows, so a name the browse page withholds must not be
downloadable from here (GH #300); the ``entity_label`` column falls back to
the id, which is what the row stored anyway.
"""
buffer = io.StringIO()
writer = csv.writer(buffer)
Expand All @@ -119,7 +126,10 @@ def flush() -> str:
async for batch in service.iter_entries(filters):
actors = await resolve_actors(db, [entry.user_id for entry in batch])
labels = await resolve_entity_labels(
db, registry, [(entry.entity_type, entry.entity_id) for entry in batch]
db,
registry,
[(entry.entity_type, entry.entity_id) for entry in batch],
permissions,
)
for entry in batch:
writer.writerow(
Expand Down
Loading
Loading