From 0cf35e451b22d337d06fdcccdf304a4c159ca4cb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 4 Sep 2026 23:23:28 +0200 Subject: [PATCH] fix(audit_log): gate entity labels on the owning module's permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/framework/lifecycle.md | 6 +- docs/framework/permissions.md | 2 +- docs/modules/audit_log.md | 14 +- docs/modules/users.md | 2 + .../core/simple_module_core/audit_links.py | 23 +- .../core/simple_module_core/permissions.py | 18 ++ framework/core/tests/test_audit_links.py | 19 ++ framework/core/tests/test_permissions.py | 30 +- .../simple_module_hosting/permissions.py | 51 +++- modules/audit_log/audit_log/endpoints/api.py | 13 +- .../audit_log/audit_log/endpoints/views.py | 10 +- modules/audit_log/audit_log/export.py | 14 +- modules/audit_log/audit_log/pages/Browse.tsx | 146 +-------- .../pages/components/EntriesTable.tsx | 121 ++++++++ .../audit_log/pages/components/Pager.tsx | 48 +++ modules/audit_log/audit_log/resolve.py | 29 +- .../tests/test_entity_label_batching.py | 20 +- .../tests/test_entity_label_permission.py | 279 ++++++++++++++++++ modules/users/users/admin/recent_activity.py | 14 +- modules/users/users/audit.py | 9 + 20 files changed, 698 insertions(+), 170 deletions(-) create mode 100644 modules/audit_log/audit_log/pages/components/EntriesTable.tsx create mode 100644 modules/audit_log/audit_log/pages/components/Pager.tsx create mode 100644 modules/audit_log/tests/test_entity_label_permission.py diff --git a/docs/framework/lifecycle.md b/docs/framework/lifecycle.md index 6ec916aa..8b20e753 100644 --- a/docs/framework/lifecycle.md +++ b/docs/framework/lifecycle.md @@ -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()` diff --git a/docs/framework/permissions.md b/docs/framework/permissions.md index e0440ae8..6ac63e64 100644 --- a/docs/framework/permissions.md +++ b/docs/framework/permissions.md @@ -150,5 +150,5 @@ async def test_create_requires_permission(client, db_session): - **Namespace with the module name.** `orders.create`, not `create_order`. - **Use `.` 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. diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index 28a837ee..1b2d048e 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -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 | diff --git a/docs/modules/users.md b/docs/modules/users.md index 4c083f25..2c5539d3 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -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 | diff --git a/framework/core/simple_module_core/audit_links.py b/framework/core/simple_module_core/audit_links.py index 35f211b5..47a56172 100644 --- a/framework/core/simple_module_core/audit_links.py +++ b/framework/core/simple_module_core/audit_links.py @@ -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 @@ -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 @@ -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: diff --git a/framework/core/simple_module_core/permissions.py b/framework/core/simple_module_core/permissions.py index 1fe15ae7..d79fdab8 100644 --- a/framework/core/simple_module_core/permissions.py +++ b/framework/core/simple_module_core/permissions.py @@ -2,6 +2,7 @@ from __future__ import annotations +from collections.abc import Collection from dataclasses import dataclass, field WILDCARD = "*" @@ -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).""" diff --git a/framework/core/tests/test_audit_links.py b/framework/core/tests/test_audit_links.py index 1bf743c2..e6b15031 100644 --- a/framework/core/tests/test_audit_links.py +++ b/framework/core/tests/test_audit_links.py @@ -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}")) diff --git a/framework/core/tests/test_permissions.py b/framework/core/tests/test_permissions.py index ddc8232b..6a65e2a1 100644 --- a/framework/core/tests/test_permissions.py +++ b/framework/core/tests/test_permissions.py @@ -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: @@ -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 diff --git a/framework/hosting/simple_module_hosting/permissions.py b/framework/hosting/simple_module_hosting/permissions.py index 57e23f91..0872555b 100644 --- a/framework/hosting/simple_module_hosting/permissions.py +++ b/framework/hosting/simple_module_hosting/permissions.py @@ -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", @@ -12,6 +12,7 @@ "RequiresPermission", "expand_permissions", "resolve_permissions", + "resolved_permissions_for", ] # How a denial spells the missing permission in ``HTTPException.detail``. @@ -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. @@ -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}", diff --git a/modules/audit_log/audit_log/endpoints/api.py b/modules/audit_log/audit_log/endpoints/api.py index 3a6f1425..a2f45a30 100644 --- a/modules/audit_log/audit_log/endpoints/api.py +++ b/modules/audit_log/audit_log/endpoints/api.py @@ -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 @@ -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}"'}, ) diff --git a/modules/audit_log/audit_log/endpoints/views.py b/modules/audit_log/audit_log/endpoints/views.py index c3b10a68..f98ec598 100644 --- a/modules/audit_log/audit_log/endpoints/views.py +++ b/modules/audit_log/audit_log/endpoints/views.py @@ -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 ( @@ -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 = [] diff --git a/modules/audit_log/audit_log/export.py b/modules/audit_log/audit_log/export.py index 6b6ad7be..e6ac67ff 100644 --- a/modules/audit_log/audit_log/export.py +++ b/modules/audit_log/audit_log/export.py @@ -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 @@ -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) @@ -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( diff --git a/modules/audit_log/audit_log/pages/Browse.tsx b/modules/audit_log/audit_log/pages/Browse.tsx index af6d82fe..27b35ac5 100644 --- a/modules/audit_log/audit_log/pages/Browse.tsx +++ b/modules/audit_log/audit_log/pages/Browse.tsx @@ -3,22 +3,13 @@ import { keys, useT } from '@simple-module-py/i18n'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { Button } from '@simple-module-py/ui/components/ui/button'; import { Card } from '@simple-module-py/ui/components/ui/card'; -import { - Table, - TableBody, - TableCell, - TableHead, - TableHeader, - TableRow, -} from '@simple-module-py/ui/components/ui/table'; import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; import { Download } from 'lucide-react'; import type React from 'react'; import { useState } from 'react'; import { BrowseEmpty } from './components/BrowseEmpty'; -import { type Change, ChangesList } from './components/ChangesList'; -import { CorrelationBanner, CorrelationLink } from './components/Correlation'; -import { ActorCell, EntityCell, type EntityRef } from './components/EntryCells'; +import { CorrelationBanner } from './components/Correlation'; +import { type AuditEntryRead, EntriesTable } from './components/EntriesTable'; import { ALL, type AppliedFilters, @@ -26,23 +17,7 @@ import { FilterBar, type FilterState, } from './components/FilterBar'; -import { formatEntryTime } from './components/format'; - -interface AuditEntryRead { - id: string; - entity_type: string; - entity_id: string; - action: 'created' | 'updated' | 'deleted' | 'soft_deleted'; - changes: Change[]; - user_id: string | null; - /** Display name resolved from user_id, or null for deleted/system actors. */ - actor: string | null; - /** Where the acting user's record lives, from the audit-link registry. */ - actor_url: string | null; - entity: EntityRef; - correlation_id: string | null; - created_at: string; -} +import { Pager } from './components/Pager'; interface Props { items: AuditEntryRead[]; @@ -57,18 +32,6 @@ interface Props { filters: AppliedFilters; } -// Borderless tints, lowercase values: the pill is a value in a dense table, -// not a badge competing with the row's links for attention. -const ACTION_PILL: Record = { - created: 'bg-primary-600/10 text-primary-700', - updated: 'bg-blue-50 text-blue-700', - deleted: 'bg-red-50 text-red-700', - soft_deleted: 'bg-amber-50 text-amber-700', -}; -const PILL = 'inline-flex rounded-full px-2.5 py-0.5 text-[11px] font-medium'; -const TH = 'sm:px-6 text-[11px] font-semibold uppercase tracking-[0.08em] text-muted-foreground'; -const TD = 'sm:px-6 align-top'; - const CLEARED: FilterState = { entityType: ALL, action: ALL, @@ -136,9 +99,6 @@ function Browse() { navigate(CLEARED, 1, id); } - const totalPages = Math.max(1, Math.ceil(total / page_size)); - const from = total === 0 ? 0 : (page - 1) * page_size + 1; - const to = Math.min(page * page_size, total); // The applied filters, not the unsubmitted form state: the button must // export what the table is showing. const exportHref = `${export_url}?${queryFor( @@ -185,97 +145,19 @@ function Browse() { {items.length === 0 ? ( ) : ( - - - - - {t(keys.audit_log.table.timestamp)} - - - {t(keys.audit_log.table.action)} - - {t(keys.audit_log.table.entity)} - - - - - - {items.map((entry) => ( - - -
- {formatEntryTime(entry.created_at)} - {/* The deck has no correlation control. It stays here - because it is the only way back from one row to the - request that wrote it, and under the timestamp is - where "this same moment" belongs. */} - {entry.correlation_id && !filters.correlation_id && ( - - )} -
- - - - {t(keys.audit_log.actions[entry.action])} - - - - - - - {/* `TableCell` is `whitespace-nowrap` by default, which - made one long value push the table wider than the card - and cut every updated row mid-value. */} - - - ))} - -
+ )} - {/* Always visible, one page or forty: the range is how a reader - checks the filter matched what they expected, and "Showing 0–0 - of 0" is the honest answer when it matched nothing. */} -
- - {t(keys.audit_log.browse.showing, { from, to, total: total.toLocaleString() })} - -
- - -
-
+ navigate(state, next)} + /> diff --git a/modules/audit_log/audit_log/pages/components/EntriesTable.tsx b/modules/audit_log/audit_log/pages/components/EntriesTable.tsx new file mode 100644 index 00000000..6bd89908 --- /dev/null +++ b/modules/audit_log/audit_log/pages/components/EntriesTable.tsx @@ -0,0 +1,121 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { + Table, + TableBody, + TableCell, + TableHead, + TableHeader, + TableRow, +} from '@simple-module-py/ui/components/ui/table'; +import { type Change, ChangesList } from './ChangesList'; +import { CorrelationLink } from './Correlation'; +import { ActorCell, EntityCell, type EntityRef } from './EntryCells'; +import { formatEntryTime } from './format'; + +export interface AuditEntryRead { + id: string; + entity_type: string; + entity_id: string; + action: 'created' | 'updated' | 'deleted' | 'soft_deleted'; + changes: Change[]; + user_id: string | null; + /** Display name resolved from user_id, or null for deleted/system actors. */ + actor: string | null; + /** Where the acting user's record lives, from the audit-link registry. */ + actor_url: string | null; + /** The subject row. `display` falls back to the stored id when the owning + * module named no resolver — or gated it behind a permission this reader + * does not hold, which is why a name can be absent for an admin and present + * for another (see `AuditLink.label_permission`). */ + entity: EntityRef; + correlation_id: string | null; + created_at: string; +} + +// Borderless tints, lowercase values: the pill is a value in a dense table, +// not a badge competing with the row's links for attention. +const ACTION_PILL: Record = { + created: 'bg-primary-600/10 text-primary-700', + updated: 'bg-blue-50 text-blue-700', + deleted: 'bg-red-50 text-red-700', + soft_deleted: 'bg-amber-50 text-amber-700', +}; +const PILL = 'inline-flex rounded-full px-2.5 py-0.5 text-[11px] font-medium'; +const TH = 'sm:px-6 text-[11px] font-semibold uppercase tracking-[0.08em] text-muted-foreground'; +const TD = 'sm:px-6 align-top'; + +interface Props { + items: AuditEntryRead[]; + /** The correlation currently being filtered on, if any — a row already + * inside that pivot has nowhere further to pivot to, so its link is hidden. */ + correlationId: string | null; + onCorrelationSelect: (id: string) => void; +} + +/** The audit table itself: five columns, one row per entry. + * + * Split out of `Browse` so the page keeps its filter/pagination/navigation + * logic in one screenful and the row rendering in another — the two change for + * unrelated reasons, and together they sat against the 300-line cap. + */ +export function EntriesTable({ items, correlationId, onCorrelationSelect }: Props) { + const { t } = useT(); + + return ( + + + + {t(keys.audit_log.table.timestamp)} + {t(keys.audit_log.table.action)} + {t(keys.audit_log.table.entity)} + + + + + + {items.map((entry) => ( + + +
+ {formatEntryTime(entry.created_at)} + {/* The deck has no correlation control. It stays here because + it is the only way back from one row to the request that + wrote it, and under the timestamp is where "this same + moment" belongs. */} + {entry.correlation_id && !correlationId && ( + + )} +
+ + + + {t(keys.audit_log.actions[entry.action])} + + + + + + + {/* `TableCell` is `whitespace-nowrap` by default, which made one + long value push the table wider than the card and cut every + updated row mid-value. */} + + + ))} + +
+ ); +} diff --git a/modules/audit_log/audit_log/pages/components/Pager.tsx b/modules/audit_log/audit_log/pages/components/Pager.tsx new file mode 100644 index 00000000..86df607d --- /dev/null +++ b/modules/audit_log/audit_log/pages/components/Pager.tsx @@ -0,0 +1,48 @@ +import { keys, useT } from '@simple-module-py/i18n'; +import { Button } from '@simple-module-py/ui/components/ui/button'; + +interface Props { + page: number; + pageSize: number; + total: number; + onPage: (page: number) => void; +} + +/** The table footer: which slice is on screen, and the way to the next one. + * + * Always rendered, one page or forty: the range is how a reader checks the + * filter matched what they expected, and "Showing 0–0 of 0" is the honest + * answer when it matched nothing. + */ +export function Pager({ page, pageSize, total, onPage }: Props) { + const { t } = useT(); + const totalPages = Math.max(1, Math.ceil(total / pageSize)); + const from = total === 0 ? 0 : (page - 1) * pageSize + 1; + const to = Math.min(page * pageSize, total); + + return ( +
+ {t(keys.audit_log.browse.showing, { from, to, total: total.toLocaleString() })} +
+ + +
+
+ ); +} diff --git a/modules/audit_log/audit_log/resolve.py b/modules/audit_log/audit_log/resolve.py index 08021945..b17cc3ab 100644 --- a/modules/audit_log/audit_log/resolve.py +++ b/modules/audit_log/audit_log/resolve.py @@ -20,10 +20,11 @@ import logging import re import uuid -from collections.abc import Callable, Iterable +from collections.abc import Callable, Collection, Iterable from typing import Any from simple_module_core.audit_links import AuditLinkRegistry +from simple_module_core.permissions import grants from simple_module_db import LIKE_ESCAPE_CHAR, like_contains_pattern from sqlalchemy import String, cast, or_, select from sqlalchemy.ext.asyncio import AsyncSession @@ -47,6 +48,14 @@ async def resolve_actors(db: AsyncSession, user_ids: list[str | None]) -> dict[s screen. Ids that no longer resolve (deleted accounts) or never named an account at all are simply absent from the result — the caller falls back to showing the raw id, which is still the truthful record of who acted. + + Deliberately **not** gated on ``users.manage`` the way the entity column is + (``resolve_entity_labels``). The two columns disclose the same field and + answer different questions: the entity column names people who were merely + *edited*, which an auditor does not need in order to review the trail, + while the actor column names the person who *acted*, which is the trail. + ``audit_log.view`` therefore does imply "may see who acted"; that is stated + in docs/modules/audit_log.md so it is granted knowingly. See GH #300. """ wanted = sorted({uid for uid in user_ids if uid}) if not wanted: @@ -107,6 +116,7 @@ async def resolve_entity_labels( db: AsyncSession, registry: AuditLinkRegistry, refs: Iterable[tuple[str, str]], + permissions: Collection[str], ) -> dict[tuple[str, str], str]: """Map ``(entity_type, entity_id)`` -> display name for one page of rows. @@ -115,6 +125,21 @@ async def resolve_entity_labels( registered no resolver — or none at all — are absent, and the caller shows the id. + *permissions* is what the **requesting principal** holds, and a type whose + owner set ``AuditLink.label_permission`` is skipped for a reader who lacks + it. Naming a row is a second reading of that row: ``users`` names accounts + by ``full_name or email``, so resolving unconditionally made + ``audit_log.view`` a way to read the user directory that ``users.manage`` + is supposed to gate — including the email of every account still holding an + unaccepted invite, where ``full_name`` is null. The entry itself is not + withheld: the reader still gets the action, the type and the id, which is + what an audit trail is for. See GH #300. + + Required rather than defaulted, because the safe value is the one the + caller has to look up: a default of "everything" would silently reopen the + hole at every new call site, and a default of "nothing" would silently + blind an admin at the same ones. + A resolver that raises is logged and skipped rather than allowed to fail the render: the audit log's job is to show what happened, and it can still do that with an id where a name would have been. @@ -129,6 +154,8 @@ async def resolve_entity_labels( link = registry.get(entity_type) if link is None or link.label_resolver is None: continue + if not grants(permissions, link.label_permission): + continue try: resolved = await link.label_resolver(db, sorted(ids)) except Exception: diff --git a/modules/audit_log/tests/test_entity_label_batching.py b/modules/audit_log/tests/test_entity_label_batching.py index ea73bbfd..205dcb7e 100644 --- a/modules/audit_log/tests/test_entity_label_batching.py +++ b/modules/audit_log/tests/test_entity_label_batching.py @@ -14,6 +14,14 @@ from sqlalchemy.exc import DBAPIError from sqlalchemy.ext.asyncio import AsyncSession +UNGATED: set[str] = set() +"""The reader's grants, for links that declare no ``label_permission``. + +Empty rather than "everything" on purpose: these tests are about batching, and +an ungated link must name its rows for a reader holding nothing at all. The +gate itself is exercised in ``test_entity_label_permission.py``. +""" + class TestBatching: """One call per entity type per page, never one per row: a 50-row page of @@ -37,7 +45,7 @@ async def resolve(_db, ids: list[str]) -> dict[str, str]: ) labels = await resolve_entity_labels( - db_session, registry, [("User", "a"), ("User", "b"), ("User", "a")] + db_session, registry, [("User", "a"), ("User", "b"), ("User", "a")], UNGATED ) assert calls == [["a", "b"]] @@ -47,7 +55,9 @@ async def test_types_without_a_resolver_are_skipped(self, db_session: AsyncSessi registry = AuditLinkRegistry() registry.register(AuditLink(entity_type="StoredFile", url_template="/f/{id}")) - assert await resolve_entity_labels(db_session, registry, [("StoredFile", "z")]) == {} + assert ( + await resolve_entity_labels(db_session, registry, [("StoredFile", "z")], UNGATED) == {} + ) async def test_a_failing_resolver_does_not_take_down_the_page( self, db_session: AsyncSession @@ -62,7 +72,7 @@ async def boom(_db, _ids): AuditLink(entity_type="User", url_template="/u/{id}", label_resolver=boom) ) - assert await resolve_entity_labels(db_session, registry, [("User", "a")]) == {} + assert await resolve_entity_labels(db_session, registry, [("User", "a")], UNGATED) == {} async def test_a_database_error_leaves_the_session_usable( self, db_session: AsyncSession @@ -87,7 +97,7 @@ async def db_boom(_db, _ids): ) db_session.rollback = spy # type: ignore[method-assign] try: - labels = await resolve_entity_labels(db_session, registry, [("Setting", "1")]) + labels = await resolve_entity_labels(db_session, registry, [("Setting", "1")], UNGATED) finally: db_session.rollback = original # type: ignore[method-assign] @@ -114,7 +124,7 @@ async def fine(_db, ids): ) labels = await resolve_entity_labels( - db_session, registry, [("Setting", "1"), ("User", "a")] + db_session, registry, [("Setting", "1"), ("User", "a")], UNGATED ) assert labels == {("User", "a"): "name-a"} diff --git a/modules/audit_log/tests/test_entity_label_permission.py b/modules/audit_log/tests/test_entity_label_permission.py new file mode 100644 index 00000000..79cbf7d1 --- /dev/null +++ b/modules/audit_log/tests/test_entity_label_permission.py @@ -0,0 +1,279 @@ +"""Naming an audited row is a read of that row, and takes that row's permission. + +``audit_log.view`` says "may read the audit trail". It was also, accidentally, +saying "may read the display name and email of every account that appears in +it": the entity column asks each owning module to name its own rows, and the +users module names an account by ``full_name or email`` — falling through to the +raw email for any account still holding an unaccepted invite. A role granted +audit access and nothing else could page through the log and harvest the +directory that ``users.manage`` guards at the front door. + +So an ``AuditLink`` may declare a ``label_permission``, and a reader without it +gets the id the row actually stored. The entry itself is never withheld — the +action, the type, the timestamp and the id are the audit trail, and an auditor +who cannot see them is not an auditor. GH #300. +""" + +from __future__ import annotations + +import csv +import io +import uuid +from collections.abc import AsyncIterator + +import httpx +import pytest +from _entity_label_support import browse as _browse +from _entity_label_support import entity_of as _entity +from _entity_label_support import seed_entry as _seed_entry +from audit_log.constants import PERM_VIEW +from audit_log.resolve import resolve_entity_labels +from settings.models import Setting +from simple_module_core.audit_links import AuditLink, AuditLinkRegistry +from simple_module_test import forge_session_cookie +from sqlalchemy.ext.asyncio import AsyncSession +from users.constants import PERM_USERS_MANAGE +from users.models import User + +EXPORT_URL = "/api/audit_log/export.csv" +_AUDITOR_ROLE = "auditor" + +_NAME = "Sam Okafor" +_EMAIL = "sam@example.com" + + +async def _seed_user(app, *, email: str = _EMAIL, full_name: str | None = _NAME) -> str: + """A user account for the audit row to point at, returning its id.""" + async with app.state.sm.db.session_factory() as session: + user = User(email=email, hashed_password="x", full_name=full_name, is_active=True) + session.add(user) + await session.commit() + return str(user.id) + + +async def _reader(app, *, permissions: list[str], email: str) -> AsyncIterator[httpx.AsyncClient]: + """A signed-in client whose single role holds exactly *permissions*. + + Built the long way (a real account, a real role, a forged session cookie) + rather than by stubbing the permission set, because the thing under test is + whether the view consults the *requesting principal's* grants at all — a + stub would pass even if it consulted nothing. + """ + from sqlalchemy import select + from users.models import UserRole + from users.models.role import Role + + async with app.state.sm.db.session_factory() as session: + user = User(email=email, hashed_password="x", is_active=True, is_verified=True) + session.add(user) + await session.flush() + # Get-or-create: the `app` fixture already seeded an admin, so the + # 'admin' Role exists and Role.name is unique. + role = ( + await session.execute(select(Role).where(Role.name == _AUDITOR_ROLE)) + ).scalar_one_or_none() + if role is None: + role = Role(name=_AUDITOR_ROLE) + session.add(role) + await session.flush() + session.add(UserRole(user_id=user.id, role_id=role.id)) + user_id = str(user.id) + await session.commit() + + app.state.sm.permissions.map_role(_AUDITOR_ROLE, permissions) + + cookie = forge_session_cookie(str(app.state.sm.settings.secret_key), {"user_id": user_id}) + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), + base_url="http://testserver", + cookies={"session": cookie}, + ) as client: + yield client + + +@pytest.fixture +async def auditor(app) -> AsyncIterator[httpx.AsyncClient]: + """Holds ``audit_log.view`` and nothing else — the role in the report.""" + async for client in _reader(app, permissions=[PERM_VIEW], email="auditor@example.com"): + yield client + + +@pytest.fixture +async def auditor_who_manages_users(app) -> AsyncIterator[httpx.AsyncClient]: + """Holds both grants, so the names are theirs to see.""" + async for client in _reader( + app, + permissions=[PERM_VIEW, PERM_USERS_MANAGE], + email="user-admin@example.com", + ): + yield client + + +class TestBrowseWithheldFromAuditOnlyReaders: + async def test_the_name_is_replaced_by_the_id(self, app, auditor) -> None: + user_id = await _seed_user(app) + await _seed_entry(app, entity_type="User", entity_id=user_id) + + entity = _entity(await _browse(auditor), user_id) + + assert entity["display"] == user_id + assert _NAME not in str(entity) + + async def test_an_outstanding_invite_does_not_leak_its_email(self, app, auditor) -> None: + """The worst case: no ``full_name`` yet, so the label *is* the email. + + Scoped to the entity column on purpose. Creating the account also wrote + its own ``created`` entry, whose ``changes`` blob records the email as + the value that was set — that is the audit trail doing its job, and a + separate surface from the one this file is about. + """ + user_id = await _seed_user(app, email="rob@example.com", full_name=None) + await _seed_entry(app, entity_type="User", entity_id=user_id) + + props = await _browse(auditor) + + assert _entity(props, user_id)["display"] == user_id + assert "rob@example.com" not in [item["entity"]["display"] for item in props["items"]] + + async def test_the_audit_row_itself_is_still_readable(self, app, auditor) -> None: + """Withholding the name must not withhold the record. An auditor who + cannot see what happened is not an auditor.""" + user_id = await _seed_user(app) + await _seed_entry(app, entity_type="User", entity_id=user_id) + + row = next(i for i in (await _browse(auditor))["items"] if i["entity_id"] == user_id) + + assert row["entity_type"] == "User" + assert row["action"] == "updated" + assert row["entity"]["table_name"] == "users_user" + assert row["entity"]["url"] == f"/admin/users/{user_id}" + + async def test_an_ungated_type_is_still_named(self, app, auditor) -> None: + """Only the types whose owner asked for a gate are affected — a setting + key is what changed, not who somebody is.""" + async with app.state.sm.db.session_factory() as session: + row = Setting(key="users.smtp_host", value="mail.example.com") + session.add(row) + await session.commit() + row_id = str(row.id) + await _seed_entry(app, entity_type="Setting", entity_id=row_id) + + assert _entity(await _browse(auditor), row_id)["display"] == "users.smtp_host" + + +class TestBrowseShownToPermittedReaders: + async def test_users_manage_sees_the_name(self, app, auditor_who_manages_users) -> None: + user_id = await _seed_user(app) + await _seed_entry(app, entity_type="User", entity_id=user_id) + + entity = _entity(await _browse(auditor_who_manages_users), user_id) + + assert entity["display"] == _NAME + + async def test_the_admin_wildcard_still_sees_the_name( + self, app, authenticated_client: httpx.AsyncClient + ) -> None: + """``admin`` holds ``*`` rather than a list, so the gate has to honour + the wildcard or it locks out the one role that always could.""" + user_id = await _seed_user(app, email="wild@example.com", full_name="Wilder Name") + await _seed_entry(app, entity_type="User", entity_id=user_id) + + entity = _entity(await _browse(authenticated_client), user_id) + + assert entity["display"] == "Wilder Name" + + +class TestExportFollowsTheScreen: + """The CSV is a second door onto the same rows; a guard on one is no guard.""" + + async def _labels(self, client: httpx.AsyncClient, entity_id: str) -> set[str]: + """Every distinct ``entity_label`` the export gives that row. + + A set because seeding the account wrote its own ``created`` entry + alongside the one the test adds, and both name the same row — what + matters is what they are *called*, not how many there are. + """ + resp = await client.get(EXPORT_URL) + assert resp.status_code == 200, resp.text + return { + row["entity_label"] + for row in csv.DictReader(io.StringIO(resp.text)) + if row["entity_id"] == entity_id + } + + async def test_the_name_is_not_downloadable_either(self, app, auditor) -> None: + user_id = await _seed_user(app) + await _seed_entry(app, entity_type="User", entity_id=user_id) + + assert await self._labels(auditor, user_id) == {user_id} + + async def test_a_permitted_reader_still_exports_the_name( + self, app, auditor_who_manages_users + ) -> None: + user_id = await _seed_user(app) + await _seed_entry(app, entity_type="User", entity_id=user_id) + + assert await self._labels(auditor_who_manages_users, user_id) == {_NAME} + + +class TestResolverGate: + """The unit underneath, so a regression names the line rather than a page.""" + + def _registry(self) -> AuditLinkRegistry: + registry = AuditLinkRegistry() + registry.register( + AuditLink( + entity_type="Secret", + url_template="/s/{id}", + label_resolver=self._never_called, + label_permission="secrets.view", + ) + ) + return registry + + async def _never_called(self, _db, ids: list[str]) -> dict[str, str]: + return {i: f"name-{i}" for i in ids} + + async def test_a_reader_without_the_permission_gets_nothing( + self, db_session: AsyncSession + ) -> None: + labels = await resolve_entity_labels( + db_session, self._registry(), [("Secret", "a")], {"audit_log.view"} + ) + + assert labels == {} + + async def test_a_reader_with_it_gets_the_name(self, db_session: AsyncSession) -> None: + labels = await resolve_entity_labels( + db_session, self._registry(), [("Secret", "a")], {"secrets.view"} + ) + + assert labels == {("Secret", "a"): "name-a"} + + async def test_the_wildcard_satisfies_it(self, db_session: AsyncSession) -> None: + labels = await resolve_entity_labels(db_session, self._registry(), [("Secret", "a")], {"*"}) + + assert labels == {("Secret", "a"): "name-a"} + + async def test_the_resolver_is_not_even_run(self, db_session: AsyncSession) -> None: + """Skipped before dispatch, not filtered after: a resolver that is not + allowed to answer must not be allowed to query either.""" + calls: list[list[str]] = [] + + async def resolve(_db, ids: list[str]) -> dict[str, str]: + calls.append(ids) + return {} + + registry = AuditLinkRegistry() + registry.register( + AuditLink( + entity_type="Secret", + url_template="/s/{id}", + label_resolver=resolve, + label_permission="secrets.view", + ) + ) + + await resolve_entity_labels(db_session, registry, [("Secret", str(uuid.uuid4()))], set()) + + assert calls == [] diff --git a/modules/users/users/admin/recent_activity.py b/modules/users/users/admin/recent_activity.py index 7aea55c1..9770d35e 100644 --- a/modules/users/users/admin/recent_activity.py +++ b/modules/users/users/admin/recent_activity.py @@ -13,9 +13,11 @@ import logging import uuid +from collections.abc import Collection from typing import Any from fastapi import Request +from simple_module_hosting.permissions import resolved_permissions_for from sqlalchemy.ext.asyncio import AsyncSession logger = logging.getLogger(__name__) @@ -93,7 +95,7 @@ def _kind_of(registry: Any, entity_type: str) -> str: async def _labels_for( - db: AsyncSession, registry: Any, entries: list[Any] + db: AsyncSession, registry: Any, entries: list[Any], permissions: Collection[str] ) -> dict[tuple[str, str], str]: """Display labels for every row an entry page refers to, by entity type. @@ -101,13 +103,19 @@ async def _labels_for( a ``Setting`` by its key, a ``StoredFile`` by its filename — batched to one query per entity type rather than one per row. Types whose owner registered no resolver are simply absent, and the caller shows a short id. + + *permissions* carries the reader's grants through to the resolvers, which + skip entity types their owner gated (GH #300). This card lives behind + ``users.manage`` so in practice nothing is withheld here — passing the real + set rather than assuming that keeps it true if the page's own guard ever + loosens. """ from audit_log.resolve import resolve_entity_labels refs = [(entry.entity_type, entry.entity_id) for entry in entries] if not refs: return {} - return await resolve_entity_labels(db, registry, refs) + return await resolve_entity_labels(db, registry, refs, permissions) async def recent_activity_for( @@ -139,7 +147,7 @@ async def recent_activity_for( registry = request.app.state.sm.audit_links try: page = await AuditLogService(db).list_entries(user_id=str(user_id), page_size=RECENT_LIMIT) - labels = await _labels_for(db, registry, page.items) + labels = await _labels_for(db, registry, page.items, resolved_permissions_for(request)) except Exception: # A card that cannot load is not a reason to 500 the whole edit page. logger.exception("Could not read recent activity for %s", user_id) diff --git a/modules/users/users/audit.py b/modules/users/users/audit.py index a242e839..54804245 100644 --- a/modules/users/users/audit.py +++ b/modules/users/users/audit.py @@ -19,6 +19,7 @@ from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession +from users.constants import PERM_USERS_MANAGE from users.models import User _LABEL = "User" @@ -65,4 +66,12 @@ def build_user_audit_link(admin_url_prefix: str) -> AuditLink: label_key=_LABEL_KEY, table_name=User.__tablename__, label_resolver=resolve_user_labels, + # A name here is the same disclosure the user list makes, so it takes + # the same permission. Without it, a role holding `audit_log.view` and + # nothing else could read the display name of every account that has + # ever been edited — and the email of every account still holding an + # unaccepted invite, since those have no `full_name` yet. The audit row + # itself is not withheld: a reader without `users.manage` still sees + # that a `User` was updated and which id, just not who. GH #300. + label_permission=PERM_USERS_MANAGE, )