Skip to content

Commit 0cf35e4

Browse files
committed
fix(audit_log): gate entity labels on the owning module's permission
The audit table stopped showing bare uuids by asking each module to name its own rows, and the users module answers with `full_name or email`. Nothing checked whether the reader was allowed that answer, so `audit_log.view` on its own became a second, unguarded read of the user directory: page the log and collect the display name of every account that has ever been edited — and, for any account still holding an unaccepted invite, its email address, because those have no `full_name` yet and the resolver falls through to the email. The CSV export had the same hole in its `entity_label` column, and the users edit page's activity card resolved through the same helper. Naming a row is a read of that row, so it now takes that row's permission. `AuditLink` gains `label_permission`; `resolve_entity_labels` takes the requesting principal's grants and skips — before dispatch, so the resolver never even queries — any entity type whose owner declared a permission the reader lacks. `users` declares `users.manage`, the same permission that opens the user list at the front door. Everything else is left ungated by default: a setting key, a filename, a task name and a flag name are what changed, not who somebody is, and withholding them would blind a legitimate auditor for nothing. The entry itself is never withheld. A reader without `users.manage` still sees that a `User` was updated, when, by whom, which id and which fields — that is the audit trail, and an auditor who cannot see it is not an auditor. The actor column stays ungated, and that is the deliberate half of the answer the issue asks for: an audit trail that will not say who acted is not one. The two columns disclose the same field and answer different questions — the entity column names people who were merely *edited*. `docs/modules/audit_log.md` now says so outright under § Entity and actor names, so `audit_log.view` is granted knowing what it discloses. Supporting changes: - `simple_module_core.permissions.grants()` — one place for the wildcard rule, so gating part of a *response* reads a grant the way `RequiresPermission` reads it at the door. A hand-rolled `in` check forgets that `admin` holds `*` and no named permission at all; `RequiresPermission` now routes through it. - `simple_module_hosting.permissions.resolved_permissions_for(request)` — extracted from `RequiresPermission.__call__`, which already had this middleware-cache-or-resolve dance inline. - `Browse.tsx` was 286 lines against the 300-line cap, so the table and the pager move to `components/EntriesTable.tsx` and `components/Pager.tsx`. Pure extraction, no behaviour change — it is now 168 lines, and the next change to this screen is a change rather than a forced split. Known adjacent surface, deliberately not addressed here: the `changes` column records the values that were written, so a `User` create entry still contains the email that was set. That is the audit trail doing its job rather than a label leaking, it needs its own decision about redaction versus `__audit_exclude_fields__`, and folding it into this fix would have changed what the log records rather than who may read a name. Closes #300
1 parent 750bd9d commit 0cf35e4

20 files changed

Lines changed: 698 additions & 170 deletions

File tree

‎docs/framework/lifecycle.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,11 @@ def register_audit_links(self, registry: AuditLinkRegistry) -> None:
243243

244244
`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.
245245

246-
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.
246+
`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.
247+
248+
`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.
249+
250+
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.
247251

248252
## `on_startup()` / `on_shutdown()`
249253

‎docs/framework/permissions.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,5 +150,5 @@ async def test_create_requires_permission(client, db_session):
150150

151151
- **Namespace with the module name.** `orders.create`, not `create_order`.
152152
- **Use `<module>.<verb>` form.** Nouns like `orders.admin` are fine for broad grants.
153-
- **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.
153+
- **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.
154154
- **Don't use roles in business logic.** Check permissions, not `user.roles`. Roles are an admin concept; permissions are the enforcement boundary.

‎docs/modules/audit_log.md‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,14 +108,26 @@ from audit_log.contracts.schemas import AuditEntryRead, AuditEntryList
108108

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

111+
## Entity and actor names
112+
113+
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.
114+
115+
**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.
116+
117+
**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.
118+
119+
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).
120+
111121
## Permissions
112122

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

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

129+
`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.
130+
119131
## Menu
120132

121133
| Label | URL | Icon | Section | Group | Order |

‎docs/modules/users.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,8 @@ Everything else is DB-backed (initial values are pydantic defaults; edit under U
207207
| `users.manage` | admin: list / invite / disable / role-assign |
208208
| `users.self.profile` | edit own profile (granted to `user` role) |
209209

210+
`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.
211+
210212
## Menu
211213

212214
| Label | URL | Icon | Section | Group | Order | Roles |

‎framework/core/simple_module_core/audit_links.py‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,15 @@
1111
collects them into one registry at boot and stores it on
1212
``app.state.sm.audit_links``.
1313
14-
**The registry maps model class names to URL templates, nothing more.** It does
15-
not verify the row exists or that the reader may open it — following a link to a
16-
deleted record lands on that screen's own 404, and permissions are enforced by
17-
the target route as usual.
14+
**A link is a route, not an authorisation.** The registry does not verify the
15+
row exists or that the reader may open it — following a link to a deleted
16+
record lands on that screen's own 404, and permissions are enforced by the
17+
target route as usual.
18+
19+
The one exception is ``label_permission``: the *name* of a row travels in the
20+
audit payload itself, so no downstream route ever gets the chance to refuse it.
21+
A module whose row names disclose something it gates elsewhere says so there,
22+
and readers without that permission see the id instead.
1823
"""
1924

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

8195
entity_type: str
@@ -84,6 +98,7 @@ class name.
8498
label_key: str = ""
8599
table_name: str | None = None
86100
label_resolver: LabelResolver | None = field(default=None, compare=False)
101+
label_permission: str = ""
87102

88103
def __post_init__(self) -> None:
89104
if self.url_template and _ID_PLACEHOLDER not in self.url_template:

‎framework/core/simple_module_core/permissions.py‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from __future__ import annotations
44

5+
from collections.abc import Collection
56
from dataclasses import dataclass, field
67

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

2829

30+
def grants(held: Collection[str], required: str) -> bool:
31+
"""Whether a principal holding *held* satisfies *required*.
32+
33+
One place for the wildcard rule, so code that gates something *inside* a
34+
response — a column, a label, a card — reads the grant the same way
35+
``RequiresPermission`` reads it at the door. Two hand-rolled ``in`` checks
36+
are two chances to forget that ``admin`` holds ``*`` and nothing else.
37+
38+
An empty *required* is satisfied by anything: callers use it to mean "no
39+
permission gates this", which is the default for a declaration that never
40+
named one.
41+
"""
42+
if not required:
43+
return True
44+
return WILDCARD in held or required in held
45+
46+
2947
@dataclass
3048
class PermissionGroup:
3149
"""A named group of related permissions (typically one per module)."""

‎framework/core/tests/test_audit_links.py‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,25 @@ def test_registering_twice_with_distinct_closures_is_idempotent(self):
113113
)
114114
assert len(reg.all_links) == 1
115115

116+
def test_label_permission_defaults_to_ungated(self):
117+
"""Most row names disclose nothing a reader of the audit log should not
118+
already see — a setting key, a filename — so declaring nothing means
119+
"anyone who may read the trail may read the name"."""
120+
assert AuditLink(entity_type="Setting", url_template="/s/{id}").label_permission == ""
121+
122+
def test_label_permission_is_part_of_the_claim(self):
123+
"""Unlike the resolver, it is compared: two modules disagreeing about
124+
who may see a name is exactly the conflict the registry exists to
125+
catch, and silently keeping the looser one would be the wrong answer."""
126+
reg = AuditLinkRegistry()
127+
reg.register(AuditLink(entity_type="User", url_template="/u/{id}"))
128+
with pytest.raises(ValueError, match="Two modules claim"):
129+
reg.register(
130+
AuditLink(
131+
entity_type="User", url_template="/u/{id}", label_permission="users.manage"
132+
)
133+
)
134+
116135
def test_genuinely_conflicting_claims_still_raise(self):
117136
reg = AuditLinkRegistry()
118137
reg.register(AuditLink(entity_type="User", url_template="/a/{id}"))

‎framework/core/tests/test_permissions.py‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
from __future__ import annotations
44

5-
from simple_module_core.permissions import WILDCARD, PermissionRegistry
5+
from simple_module_core.permissions import WILDCARD, PermissionRegistry, grants
66

77

88
class TestPermissionRegistry:
@@ -113,3 +113,31 @@ async def test_role_map_returns_plain_dict_of_lists(self):
113113
for key, val in result.items():
114114
assert isinstance(key, str)
115115
assert isinstance(val, list)
116+
117+
118+
class TestGrants:
119+
"""The wildcard rule, in one place.
120+
121+
Anything gating part of a *response* rather than a route reads grants
122+
through here — a second hand-rolled ``in`` check is a second chance to
123+
forget that ``admin`` holds ``*`` and no named permission at all.
124+
"""
125+
126+
async def test_a_held_permission_is_granted(self):
127+
assert grants({"users.manage"}, "users.manage") is True
128+
129+
async def test_a_missing_permission_is_not(self):
130+
assert grants({"audit_log.view"}, "users.manage") is False
131+
132+
async def test_the_wildcard_satisfies_anything(self):
133+
assert grants({WILDCARD}, "users.manage") is True
134+
135+
async def test_an_empty_requirement_is_satisfied_by_nothing_held(self):
136+
"""Empty means "no permission gates this" — the default for a
137+
declaration that never named one, not a requirement nobody meets."""
138+
assert grants(set(), "") is True
139+
140+
async def test_it_reads_any_collection(self):
141+
"""Callers hand it whatever they have — the middleware caches a set,
142+
a test hands a list."""
143+
assert grants(["users.manage"], "users.manage") is True

‎framework/hosting/simple_module_hosting/permissions.py‎

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from __future__ import annotations
44

55
from fastapi import HTTPException, Request
6-
from simple_module_core.permissions import DEFAULT_ROLE_PERMISSIONS, WILDCARD
6+
from simple_module_core.permissions import DEFAULT_ROLE_PERMISSIONS, WILDCARD, grants
77

88
__all__ = [
99
"DEFAULT_ROLE_PERMISSIONS",
@@ -12,6 +12,7 @@
1212
"RequiresPermission",
1313
"expand_permissions",
1414
"resolve_permissions",
15+
"resolved_permissions_for",
1516
]
1617

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

4748

49+
def resolved_permissions_for(request: Request) -> set[str]:
50+
"""The permission set this request's principal holds, wildcard included.
51+
52+
Prefers what ``InertiaLayoutDataMiddleware`` already resolved and cached on
53+
``request.state``; falls back to resolving from the registry's role map when
54+
the middleware is not in the stack (a bare router under a test transport),
55+
caching the result so a route with several gates resolves once.
56+
57+
Anonymous requests hold nothing. Exposed because a handler sometimes has to
58+
gate part of its *response* rather than the route — the audit log's entity
59+
labels, for one — and that decision must read the same permission set the
60+
door did.
61+
62+
Role-derived only, matching ``RequiresPermission``: the ``permissions``
63+
module's direct per-user grants are its own dependency's business, and a
64+
caller wanting those consults ``permissions.deps.RequiresPermission``.
65+
"""
66+
cached: set[str] | None = getattr(request.state, "resolved_permissions", None)
67+
if cached is not None:
68+
return cached
69+
70+
user = getattr(request.state, "user", None)
71+
if user is None:
72+
return set()
73+
74+
sm = getattr(getattr(request.app, "state", None), "sm", None)
75+
perm_registry = getattr(sm, "permissions", None) if sm is not None else None
76+
role_map = perm_registry.role_map if perm_registry is not None else None
77+
permissions = resolve_permissions(user.roles, role_map=role_map)
78+
request.state.resolved_permissions = permissions
79+
return permissions
80+
81+
4882
class RequiresPermission:
4983
"""FastAPI dependency that enforces a specific permission.
5084
@@ -63,20 +97,7 @@ def __call__(self, request: Request) -> None:
6397
if user is None:
6498
raise HTTPException(status_code=401, detail="Authentication required")
6599

66-
# Use cached permissions from middleware if available
67-
permissions: set[str] | None = getattr(request.state, "resolved_permissions", None)
68-
if permissions is None:
69-
# Fallback: middleware did not run — consult registry role_map if available
70-
sm = getattr(getattr(request.app, "state", None), "sm", None)
71-
perm_registry = getattr(sm, "permissions", None) if sm is not None else None
72-
role_map = perm_registry.role_map if perm_registry is not None else None
73-
permissions = resolve_permissions(user.roles, role_map=role_map)
74-
request.state.resolved_permissions = permissions
75-
76-
if WILDCARD in permissions:
77-
return
78-
79-
if self.permission not in permissions:
100+
if not grants(resolved_permissions_for(request), self.permission):
80101
raise HTTPException(
81102
status_code=403,
82103
detail=f"{PERMISSION_DENIED_PREFIX}{self.permission}",

‎modules/audit_log/audit_log/endpoints/api.py‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
from fastapi import APIRouter, Depends, Query, Request
88
from fastapi.responses import StreamingResponse
99
from simple_module_db.deps import get_db
10-
from simple_module_hosting.permissions import RequiresPermission
10+
from simple_module_hosting.permissions import RequiresPermission, resolved_permissions_for
1111
from sqlalchemy.ext.asyncio import AsyncSession
1212

1313
from audit_log.constants import DEFAULT_PAGE_SIZE, PERM_VIEW
@@ -83,7 +83,16 @@ async def export_audit_entries(
8383
)
8484

8585
return StreamingResponse(
86-
stream_csv(service, db, request.app.state.sm.audit_links, filters),
86+
# Resolved here rather than inside the generator: the response body is
87+
# produced after the endpoint returns, and `request.state` is the
88+
# wrong thing to reach into from a background task.
89+
stream_csv(
90+
service,
91+
db,
92+
request.app.state.sm.audit_links,
93+
filters,
94+
resolved_permissions_for(request),
95+
),
8796
media_type=CSV_MEDIA_TYPE,
8897
headers={"Content-Disposition": f'attachment; filename="{CSV_FILENAME}"'},
8998
)

0 commit comments

Comments
 (0)