ADR-0018: Org-scoped audit logging (FIT-20)

Status: Accepted Date: 2026-08-07 Context owner: Saar Issue: FIT-20  Related: ADR-0004 (org scoping) · ADR-0007 (compliance bucket) · ADR-0013 (append-only ledger precedent)

TL;DR

An append-only audit_events table recording who changed what inside one gym, written in the application layer by a single service, read by the owner behind a flag, and aged to R2 after 24 months.

Five decisions worth recording, because each had a plausible alternative:

1. App-level capture, not database triggers — PGBouncer transaction pooling destroys per-transaction actor attribution, and the repo has zero trigger precedent.
2. @Global() module + @Optional() injection — the only loop-free wiring, bought at the cost of silent-miswiring risk, which is now covered by a composition spec.
3. The feature flag gates reading only — capture is unconditional, so turning the viewer on later shows history rather than starting it.
4. Archive-before-delete at 24 months to the compliance bucket, per (org, month).
5. The trail audits itself — viewing, exporting and denied access all write rows.

A sixth was added in the follow-up that extended coverage to staff destruction and to every charge path — see the appendix.

Context

Three pressures land on the same table: member-versus-owner disputes (“I cancelled last month”), staff accountability in a business whose operating model is the owner’s trust in a handful of admins and coaches, and compliance. The compliance driver is specific: the Israeli Privacy Protection Regulations (Data Security) 5777-2017 impose an automatic access-recording duty (reg. 10) with a 24-month retention floor on databases at the medium and high security levels, and Amendment 13 (in force 2025-08-14) turned the enforcement behind it from theoretical into administrative fines. A gym SaaS holding member contact details, payment records and — for many orgs — health information is a plausible high-level database.

audit_logs already existed but is the platform-admin ledger: Taikan staff acting across orgs, keyed on actor_clerk_id, no organization_id. It answers a different question for a different reader and was deliberately not merged.

Decision 1 — capture in the application, not in database triggers

The issue title floated “table + triggers”. Triggers were rejected.

Actor attribution is the whole point of an audit trail, and triggers cannot get it. A trigger sees current_user — the single Postgres role the API connects as — not the gym owner who clicked. The usual workaround is a per-transaction GUC (SET LOCAL app.actor_id = …) read back by the trigger. Prod runs behind PGBouncer in transaction pooling mode, where a session is not stably bound to a backend across statements; SET LOCAL survives only within one explicit transaction, and much of the domain layer writes with autocommit statements outside one. The attribution would be correct in dev and quietly wrong in prod — the worst failure mode available for a compliance record.

Secondary reasons: the repo has no trigger precedent (every invariant lives in TS, per ADR-0003’s spirit), triggers are invisible to the Drizzle types and to anyone reading the service code, and a trigger cannot express “record plan.updated only when a commercially meaningful field moved” without duplicating domain logic in PL/pgSQL.

Consequence, stated honestly: app-level capture is not a completeness guarantee. Anything that writes to the DB without going through an instrumented service — a manual psql, a migration, a future service someone forgets to instrument — leaves no trace. A trigger would have caught those. We accept the gap because an attributable record of the paths that matter beats an unattributable record of all of them. See “Known gaps”.

Decision 2 — @Global() module, @Optional() injection

AuditService is injected at ~13 domain services. The obvious wiring — list AuditModule in each consumer’s imports and make the dependency required — creates a cycle: AuditModule imports MembershipsModule for the controller’s requireMembership, so MembershipsModule importing back needs forwardRef on both sides, and the same knot repeats for payments and subscriptions.

@Global() + @Optional() is the loop-free arrangement. It also keeps every existing direct-construction unit driver (new PlansService(...)) and partial-graph integration spec compiling untouched, which the “never modify tests for code you didn’t intend to touch” rule requires. The events?: EventTrackingService in PlansService set the precedent.

The cost is real and it bit during this build. @Optional() means a miswiring degrades to silence rather than a boot failure: AuditAccessService was written, imported into the controller and left out of providers, and every unit spec stayed green while the API refused to boot. The mitigation is apps/api/src/audit/audit.module.unit.spec.ts, which asserts statically that every class dependency of everything the module declares is resolvable, and then makes Nest actually build the graph with useMocker standing in for the globals. It was verified to fail against the exact regression it was written for. A second consequence of @Optional() — an audit-only DB lookup running in audit-less compositions and breaking three unrelated payment specs — is why PaymentMethodService gates its org lookup on this.audit being present.

Decision 3 — the flag gates reading, never writing

audit-logging is a per-org PostHog flag (per the repo’s flag policy: PostHog, never an env var; organization group; default OFF). It gates the read surface only, and fail-closed: only a definite true opens it, so unset / off / PostHog-unreachable all yield 404 rather than 403 — an un-rolled-out org cannot tell the endpoint exists.

AuditService.record() has no flag check at all. A flag is a rollout control, and a compliance record whose history begins at the flip is worth much less than one that was running all along. This is the one place the repo’s “default OFF / fail toward current behavior” rule is deliberately not applied to a write path, because here “current behavior” is the absence of evidence.

Decision 4 — 24-month retention, archive before delete

Monthly (0 3 1 * *). Rows older than 24 months are grouped by (org, YYYY-MM) in JS (pure-Drizzle query, unit-testable month logic — the Database Policy’s call), written to the compliance bucket (ADR-0007) as NDJSON at audit-archive/<orgId>/<YYYY-MM>.ndjson, and only then deleted by exact row id, org scoped, in batches of 500.

Upload before delete, and never delete what didn’t upload. A failed upload aborts that bucket’s delete and leaves the rows for next month; a duplicate archive is recoverable, a deleted-but-unarchived audit row is not. 24 months is the regulatory floor, so the boundary is a strict lt(created_at, cutoff) — a row exactly on the boundary survives — and computeRetentionCutoff is exported specifically so that boundary is directly unit-tested rather than asserted in a comment.

A pg_try_advisory_lock guards the sweep. Non-blocking, so an overlapping run no-ops. This matters more here than for other crons: two interleaved runs can produce an archive written by the earlier run overwriting the later one’s, followed by the later run deleting rows the surviving object never contained. Note the PGBouncer caveat from Decision 1 applies to the lock too (session-scoped locks are unreliable under transaction pooling) — it is the recurring-charge cron’s existing pattern, and it is listed as a gap rather than silently trusted.

Decision 5 — the trail audits itself

Reading a log of who touched personal data is itself a touch of personal data. ISO/IEC 27001:2022 A.8.15 requires logs be protected and their access controlled, NIST SP 800-53 AU-9 makes the audit store’s own access a control, and the 2017 Regulations’ recording duty asks for the outcome of every access attempt without carving out the log’s own table. So:

• audit.exported — every CSV export, always, with the filter scope and row count. An export moves the whole filtered trail onto someone’s laptop; it is rare, deliberate, and always worth a row.
• audit.viewed — list access, coalesced to one row per actor per 15 minutes. A page load, a filter change, a pagination click and each debounced search keystroke are four requests describing one act of looking; recording all four would make the trail mostly a record of itself and crowd out the membership and billing rows it exists for. The row carries coalesceWindowMinutes so a reader can see the rule that produced it, and the window is one constant away from “record every access”.
• audit.access_denied — a refusal, with reason separating a rollout gate (flag_disabled) from an authorization failure (insufficient_role).

All three are settings, not auth: an owner filtering auth is asking “who signed in”, and burying their own page loads in that answer degrades the more important signal.

Denied attempts are recorded only for verified members of the org. orgId comes straight off the URL, so recording non-members would let any authenticated user write unbounded rows into any gym’s trail by looping over org ids — turning the access-denied control into a log-injection and storage-exhaustion vector against the table it protects. Catching cross-org probing belongs in a global guard, not here.

Appendix — Decision 6, added in the coverage follow-up

The initial build instrumented business services one call site at a time (Decision 1). Two additions exposed the limit of that rule and refined it.

Where a domain has many callers and one shared writer, instrument the writer. Nine code paths charge a card: desk charge, renewal cron, debt collection, plan-change proration, hosted plan checkout, course checkout, staff-triggered renewal, the legacy createRecurring, and provider-webhook settlement. Instrumenting each was nine chances to forget and a tenth the next time a path is added. All nine resolve through PaymentTransactionService, so payment.charge_succeeded / payment.charge_failed are emitted there — the only place where “exactly once, from every path” is a checkable property rather than a convention.

This is a narrowing of Decision 1’s “capture where the decision is made”, not a reversal. It applies where a genuine choke point exists and the audit row needs nothing the choke point lacks. Refunds have the same table but not the same shape: the reason, the capability and the human actor live at the business site and never reach the DAL, so refunds stayed per-site and the gaps were closed by instrumenting the three paths that had none (manual-refund settlement, provider-portal refunds, platform-admin refunds).

Two consequences of putting a capture at the DAL:

• It cannot know why it was called. metadata.source is therefore copied off the transaction row, where the callers already label themselves, rather than inferred.
• It must be idempotent by construction. A provider re-delivering a webhook would otherwise write a second row, so updateStatus reads the prior status first — gated on the audit service being present, so an un-audited graph pays nothing for it.

A sixth category, operations, and a bulk shape. Staff destroying operating data (sessions, bookings, workouts, programs, templates, class types, forms, leads, announcements) had no honest home in the original five, and filing it under settings would have made that filter meaningless. It was the only category added after the initial build, and the enum stays coarse: the fine grain is still in action.

Bulk deletes in this category write one row per batch with the count, the ids and the filter — not one per entity. A 300-session sweep would otherwise bury the trail under itself, and for a bulk operation the filter is the scope of access reg. 10 asks to record. This deliberately differs from the per-entity behavior of bulk membership changes, which was left alone rather than changed under an unrelated PR.

The category is curated, not exhaustive. About thirty other staff-facing DELETE endpoints exist; they were left out because each is low-consequence, low-volume, or trivially re-creatable. Member self-service — a member cancelling their own seat — is out on principle: it is the highest-volume write in the product, and “who did this” is only a question when the answer isn’t the member.

payment.charge_recorded was retired rather than kept alongside the new pair. It was emitted only by the desk charge, which the choke point now covers, and keeping both would have double-logged exactly the charges an owner is most likely to scrutinise. Nothing has ever written it to a database — the feature is unreleased — so no legacy label was owed.

Appendix — Decision 7, the auth fan-out is staff-only

The first build fanned every Clerk session.* event out to every active membership the user held, member memberships included. That was wrong on both axes it could be wrong on, and the two arguments point the same way.

Purpose. The auth category answers “when did someone with authority over this gym’s data get into the system”. Reg. 10’s access record is about authorized personnel; a member opening the app to book a class holds none on every org-level module in the permission matrix, so their session carries no authority to account for. Their sign-in is not an access to the gym’s database in the sense the regulation means.

Cost, of two kinds. Members outnumber staff by orders of magnitude and sign in far more often, so member logins would have been the highest-volume write in the trail — burying the staff signal the log exists to surface, and filling 24 months of retention (then an unbounded R2 archive) with it. And each row would persist a member’s IP address and user-agent into a table the gym owner can read and export as CSV. Collecting a member’s location trail as a side effect of a staff-accountability control is a privacy liability the log gains nothing from; the cheapest way not to mishandle that data is not to hold it.

So the fan-out filters on STAFF_ROLES in SQL, and does so for the whole category rather than per action — auth.user_updated is excluded for members on the same reasoning. A user who staffs one gym and is a member of another gets a row for the first and nothing for the second, from one Clerk event.

The cost accepted: if a member’s account is ever taken over, the trail cannot help reconstruct it. That is recorded as a known gap. Widening it later is one predicate, and would be the right move only if member sessions ever gain authority.

Because the change is narrowing — strictly fewer rows, and the pre-change behavior is the bug — it ships unflagged. A per-org flag here would mean “some orgs keep collecting member IPs”, which is the outcome the change exists to prevent, and it would put a PostHog read on a webhook path where a flag-service failure is a new failure mode.

The rule is enforced in SQL, so it is guarded by an integration spec against a real database (audit-auth.service.int.spec.ts), not a mocked query chain: a mock could only assert on the shape of a condition object and would keep passing if the predicate were dropped.

Consequences

Positive

• Disputes and staff questions become answerable from one filterable page.
• Attribution is honest: no request context yields actor_type = 'system' rather than crediting whoever happened to trigger a cron.
• An audit failure can never fail a business request — record() swallows into Sentry. A gap in the log is recoverable; a role change that 500s because of a logging bug is not.
• The 24-month floor and the field set line up with the 2017 Regulations (mapping in docs/features/audit-logging/README.md § Compliance).

Negative

• Capture completeness depends on remembering to instrument (Decision 1), except where a choke point exists and was used instead (appendix, Decision 6).
• @Optional() degrades miswiring to silence (Decision 2) — mitigated, not eliminated.
• The app’s DB role holds DELETE on audit_events because the retention cron runs in-process, so “append-only” is a code property, not a database-enforced one.
• The R2 archive has no expiry, so archived rows are retained indefinitely — the opposite failure from under-retention, and a storage-limitation question for counsel.
• Read access to member data generally is not logged; only mutations plus audit-log access are.
• Members’ own sign-ins are not recorded at all (appendix, Decision 7), so the trail cannot help reconstruct a member account takeover.

Alternatives considered

Related

• Feature docs — behavior, data model, QA plan, compliance mapping
• ADR-0004 — every read is org-scoped in code
• ADR-0007 — why the archive goes to the compliance bucket
• ADR-0013 — the append-only ledger precedent