feat(audit): structured audit trail for user-initiated changes - #243
Merged
Conversation
First phase of the audit log (#17): the domain vocabulary only, no persistence or interception yet. `IAuditable` is an opt-in marker. The interceptor added in a later phase ignores every entity that does not implement it, so forgetting the marker means a change goes unrecorded rather than the audit table filling with machine-written noise. An inverted opt-out marker would have been fewer files to touch, but it would silently capture high-volume tables such as CheckDataPoint the moment a new entity appears. 34 entities are marked: monitoring configuration, incidents, postmortems, maintenances, alerting and escalation, on-call, identity and access, integrations, and taxonomy. Deliberately unmarked are the machine-written tables (CheckDataPoint, Alert, the delivery logs, the outbox, worker registrations) and the token tables, which hold secrets and are better left out entirely than trusted to a property-level exclusion. `[NotAudited]` keeps secrets out of the JSON snapshots. It is applied to ApiKey.HashedKey, OidcProviderConfig.ClientSecret and Integration.ConfigJson. Saml2ProviderConfig.IdpSigningCertificate is left audited because a public signing certificate is not a secret. The attribute cannot reach members inherited from ASP.NET Core Identity, so the interceptor will also carry a name-based deny list. AuditLog itself is append-only by construction: no UpdatedAt, and the API will expose reads only. Entries carry a CorrelationId — one UUIDv7 per SaveChanges — so a single user action that touches an entity and its join rows reads as one transaction instead of several unrelated rows, with IsPrimary marking the entry that names it. Refs #17
Second phase of the audit log (#17): EF mapping and migration. Nothing writes to the table yet — the interceptor comes next. No foreign key on UserId, following the precedent set by NotificationDeliveryLogConfiguration. The trail has to survive the account being deleted, which is precisely when it matters most, so UserEmail is denormalised onto the row rather than joined from AspNetUsers. Four indexes, each backing a specific access pattern. CorrelationId serves double duty: it fetches every entry in a transaction, and it orders the feed — the listing paginates over transactions rather than rows, and since the interceptor assigns UUIDv7 whose ordering is already chronological, the default sort is an index scan with no aggregate over CreatedAt. (EntityType, EntityId) answers "what happened to this Service?", UserId answers "what did this person do?", and CreatedAt covers date ranges. Action is stored as a string rather than an int, matching how the other enums in this schema are mapped. The value snapshots are unbounded text: a snapshot is as wide as the audited entity's scalars. The generated migration contains only the new table and its indexes, with no incidental changes elsewhere, confirming that marking 34 entities with an empty interface leaves the schema untouched. Refs #17
`dotnet list package --vulnerable --include-transitive` reported eight advisories across three packages. All are now clear. Scriban 7.1.0 -> 7.2.5, closing two high and two moderate advisories. The minimum patched version was 7.2.2 (GHSA-7jvp-hj45-2f2m, template writes reaching arbitrary CLR properties through TypedObjectAccessor). Microsoft.OpenApi 2.4.1 -> 2.11.0. The advisory publishes patches on two branches, 2.7.5 and 3.5.4, so staying on 2.x closes it without the breaking changes 3.x would bring to PatchSchemaTransformer and SecuritySchemeTransformer. Whether to take the major is a separate call. System.Security.Cryptography.Xml 10.0.7 -> 10.0.10 (CVE-2026-50648, five high advisories) needs a direct PackageReference: it arrives transitively through ITfoxtec.Identity.Saml2 4.20.1, which is the latest published version and still pins 10.0.7, so no upgrade resolves it. The comment on that line records why it exists and when it can go. This is the most pointed of the three, since SAML is the path that parses signed XML coming from an external identity provider. Scriban had no test coverage at all despite rendering every email and notification template, so a clean build verified nothing about the 7.1 -> 7.2 jump across the mass-assignment fix. Added tests over the rendering patterns actually in use — anonymous and typed models, conditionals and loops, unknown members degrading to empty — plus one asserting that a template can no longer write back into its model, which is the behaviour the advisory changed.
Third phase of the audit log (#17): the interceptor that actually writes entries, plus the seam that tells it who is acting. The interceptor runs in two passes because neither half of what an entry needs exists at a single moment. Before the save the change tracker still holds original values and entity states; after it, inserted rows finally have their database-generated keys. So SavingChanges captures states and snapshots, and SavedChanges resolves ids and writes. That write is itself a SaveChanges, so a reentrancy flag keeps the audit write from being audited. ICurrentUserAccessor is the seam that keeps HttpContext out of Infrastructure. The API implements it over IHttpContextAccessor; hosts with no request pipeline resolve nothing. The DbContext only attaches the interceptor when the container can resolve the accessor, so the Worker does not merely skip auditing, it never walks the change tracker at all. Nothing is recorded when no user resolves, which is the "only what a person did" rule from the design discussion. Reverting to a fuller forensic record later means deleting one early return, with no schema change. Two details worth naming. An update touching no audited property writes no row: rotating a [NotAudited] secret would otherwise produce an entry with two identical snapshots, which is worse than no entry because it implies a change you cannot see. And SaveChangesFailed clears the captured batch, so a rolled-back transaction cannot leak its entries into the next save on the same context. Join-entity detection — a composite key made entirely of foreign keys — is a heuristic, used only to decide which entry names the transaction. A false positive degrades a label; it cannot corrupt an entry. Seven integration tests against a real Postgres cover create, update and delete, that CheckDataPoint is ignored, that an unauthenticated save writes nothing, that a Service and its ServiceTag share one correlation id with IsPrimary on the Service, and that ApiKey.HashedKey stays out of the snapshot while the already-masked value does not. Refs #17
Fourth phase of the audit log (#17): the read endpoint, and the writer for events the interceptor cannot see. GET /api/v1/audit-logs is restricted to Owner and Admin — the trail records changes to identity and access configuration across the whole instance, so it is administrative rather than operational. There is no write endpoint of any kind: the table is append-only. The feed paginates over transactions rather than rows, in two queries: page the correlation ids, then fetch every entry belonging to those groups. A page therefore means a stable number of user actions and no transaction is ever split across a page boundary. A filter selects transactions but never prunes them — filtering by Service still returns the ServiceTag entries that the same action wrote, since hiding the siblings would misrepresent what changed. Authentication is not a change to an entity, so IAuditLogWriter states those events explicitly from AuthController, where the caller's IP lives and where — for a rejected attempt — there is no principal for ICurrentUserAccessor to resolve. A failed login is the one entry written with no verified actor: the email recorded is only what the request claimed. Two corrections to earlier phases, both found by tests rather than by reading: Ordering by CorrelationId alone was wrong. UUIDv7 is time-ordered only to millisecond precision; within one millisecond the remaining bits are random, so transactions written in the same millisecond came back shuffled — reproduced with five ids sharing a timestamp prefix that sorted 4,1,3,2,0. The feed now orders by the group's timestamp with the id as a deterministic tie-break, at the cost of the GROUP BY that ordering by the id was meant to avoid. UUIDv7 stays for index locality, which is what it is actually good for here. The comments asserting otherwise are fixed in all four places. SetAuditTimestamps restamped CreatedAt on every added entity, AuditLog included, overwriting the timestamp the interceptor had just set from TimeProvider. An audit entry has to be able to state when the change it records happened, so it is now excluded. Ten integration tests cover the grouping, the transaction count, ordering under identical timestamps, each filter, whole-transaction results under a partial filter, and the auth events. Refs #17
Final phase of the audit log (#17): the UI, under Logs alongside System Logs and Delivery Logs. One row per transaction, not per entity change, matching how the API paginates. Clicking a row opens a dialog with a GitHub-style unified diff per entity the action touched: removed values on red `-` lines, new values on green `+` lines, one property per row. A dialog rather than an expandable row. A single action can touch several entities and each diff is a wide block of monospaced text, which deforms the table's columns when nested inside a row. Property names are shown exactly as the backend recorded them — the C# model's names, untranslated. A mapping of friendlier labels would drift from the entities every time one changes, and a stale label on an audit trail is worse than a blunt one. Unchanged properties are hidden behind a toggle rather than dropped. A snapshot records the whole entity so most rows are usually noise, but "what else was true at the time" is a fair question to ask of an audit trail. The diff logic lives in a plain module with 15 tests covering updates, creates, deletes, added and removed properties, non-string values, nested objects, and malformed or absent snapshots — the last of which returns an empty diff rather than throwing, since a snapshot that cannot be parsed must not take the page down with it. No role gating in the frontend, following the existing convention: the endpoint is restricted to Owner and Admin server-side, which is where it actually matters. api-types.ts is regenerated from the OpenAPI document rather than hand-written, per the project's convention. Refs #17
cl8dep
added
enhancement
cl8dep
marked this pull request as ready for review
July 29, 2026 15:14
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a structured audit trail. Until now
SetAuditTimestamps()only stampedCreatedAt/UpdatedAton entities, so there was no record of who changed what. This adds an append-onlyAuditLogtable, aSaveChangesinterceptor that records changes to entities markedIAuditable, a read-only API endpoint restricted to Owner and Admin, and an admin page under Logs with a diff viewer.The trail records what people did, not what the system did. Nothing is written when no authenticated user resolves, so background jobs, Quartz, seeding and migrations leave no trace even when they touch a marked entity. Entries are grouped by a correlation id assigned per
SaveChanges, so one user action that touches an entity and its join rows reads as one transaction rather than several unrelated rows.Related
Closes #17
Changes
Domain
IAuditable, an opt-in marker. The interceptor ignores anything without it, so forgetting the marker means a change goes unrecorded rather than the table filling with machine-written noise. An opt-out marker would have been fewer files to touch but would silently capture high-volume tables likeCheckDataPointthe moment a new entity appears.CheckDataPoint,Alert, the delivery logs, the outbox, worker registrations) and the token tables, which hold secrets and are better left out entirely than trusted to a property-level exclusion.[NotAudited]keeps secrets out of the JSON snapshots:ApiKey.HashedKey,OidcProviderConfig.ClientSecret,Integration.ConfigJson.Saml2ProviderConfig.IdpSigningCertificatestays audited because a public signing certificate is not a secret.Persistence
AuditLogis append-only by construction: noUpdatedAt, no write endpoints, and no code path that revisits a written row.UserId, following the precedent set byNotificationDeliveryLogConfiguration. The trail has to survive the account being deleted, which is precisely when it matters most, soUserEmailis denormalised onto the row.CorrelationIdfor the grouping,(EntityType, EntityId)for one row's history,UserIdfor one person's activity,CreatedAtfor date ranges and the feed's ordering.Interceptor
ICurrentUserAccessoris the seam that keepsHttpContextout of Infrastructure. The API implements it overIHttpContextAccessor; the DbContext only attaches the interceptor when the container can resolve it, so the Worker never walks the change tracker at all.[NotAudited]secret would otherwise produce an entry with two identical snapshots, which is worse than no entry because it implies a change you cannot see.SaveChangesFailedclears the captured batch, so a rolled-back transaction cannot leak entries into the next save on the same context.API
GET /api/v1/audit-logs,[Authorize(Roles = "Owner,Admin")]. The trail covers identity and access configuration across the whole instance, so it is administrative rather than operational.Servicestill returns theServiceTagentries the same action wrote, since hiding the siblings would misrepresent what changed.IAuditLogWriterrecords authentication events, which are not changes to an entity and so are invisible to the interceptor. A failed login is the one entry written with no verified actor; the email recorded is only what the request claimed.Frontend
api-types.tsregenerated from the OpenAPI document rather than hand-written, per the project convention.Dependencies (unrelated to the audit log, folded in as its own commit)
dotnet list package --vulnerable --include-transitivereported eight advisories. All are now clear: Scriban 7.1.0 to 7.2.5, Microsoft.OpenApi 2.4.1 to 2.11.0, System.Security.Cryptography.Xml 10.0.7 to 10.0.10.PackageReference: it arrives transitively throughITfoxtec.Identity.Saml2 4.20.1, which is the latest published version and still pins the vulnerable one. The comment on that line records why it exists and when it can go. This is the most pointed of the three, since SAML is the path that parses signed XML from an external identity provider.Testing
dotnet testpasses (unit + integration)pnpm exec tsc -bpasses in affected frontend app(s) (apps/web,apps/admin)487 backend tests (389 unit, 98 integration) and 72 admin tests.
pnpm buildalso passes inapps/admin.New coverage:
CheckDataPointignored; an unauthenticated save writing nothing; aServiceand itsServiceTagsharing one correlation id with theServicemarked primary;ApiKey.HashedKeystaying out of the snapshot while the masked value does not.Two defects were caught by these tests rather than by reading, both fixed here:
Ordering the feed by
CorrelationIdalone was wrong. UUIDv7 is time-ordered only to millisecond precision; within one millisecond the remaining bits are random, so transactions written in the same millisecond came back shuffled. Reproduced with five ids sharing a timestamp prefix that sorted 4,1,3,2,0. The feed now orders by the group's timestamp with the id as a deterministic tie-break, at the cost of theGROUP BYthat ordering by the id was meant to avoid.SetAuditTimestampsrestampedCreatedAton every added entity,AuditLogincluded, overwriting the timestamp the interceptor had just set. An audit entry has to be able to state when the change it records happened, so it is now excluded.Not verified: the page was not exercised in a browser, so the rendered diff has no visual confirmation from me. The diff logic is covered by unit tests and both
tsc -band the Vite build pass.Worth flagging separately:
pnpm lintcurrently reports 24 errors inapps/admin, all pre-existing. Verified by stashing this branch's changes and re-running against the base, which produces the identical 29 problems. None are in the newaudit-logscode. That means lint is already failing onmainand probably deserves its own issue.Database
Down()is reversible20260728235941_AddAuditLogcreates one new table and its four indexes, with no changes to any existing table. That the generated migration is purely additive also confirms marking 34 entities with an empty interface leaves the schema untouched.Down()drops the new table.One thing to note for operations: retention is out of scope here, so the table grows unbounded. Same caveat as
NotificationDeliveryLog. Worth a follow-up issue for a purge job.Screenshots
Checklist
.env/appsettings.*.jsonvalues committedDocs are not updated. The wiki has no page for the audit log yet, and the feature is self-explanatory in the UI, but a short page covering what is and is not recorded would be worth adding.