Skip to content

Close the audit-trail gaps: enforced privilege separation, system-scope review, complete exports - #115

Merged
tgerke merged 5 commits into
mainfrom
fix/audit-trail-gaps
Jul 30, 2026
Merged

Close the audit-trail gaps: enforced privilege separation, system-scope review, complete exports#115
tgerke merged 5 commits into
mainfrom
fix/audit-trail-gaps

Conversation

@tgerke

@tgerke tgerke commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Closes the gaps found while assessing edc-core against FDA audit-trail requirements. Every regulatory claim below was checked against the source mirror (21 CFR Part 11 rev. 2024-04-01; FDA Electronic Systems Q&A, Oct 2024; ICH E6(R3) final). Worth stating up front: 11.10(e) requires the trail to exist, retain, and be copyable — it contains no review obligation, and FDA guidance says only that review "may be helpful." The review expectation the docs cite comes from ICH E6(R3) §4.2.3.

Privilege separation (migration 0024)

ADR-0002 claimed the application role "has no privilege to alter or drop these triggers," but nothing enforced it: the app connected as the table owner, which can ALTER TABLE ... DISABLE TRIGGER or TRUNCATE around the append-only guards. Now:

  • The API connects as edc_app, which owns nothing and holds no TRIGGER, TRUNCATE, or REFERENCES privilege anywhere; UPDATE/DELETE are additionally revoked on the seven append-only tables (signatures keep UPDATE for the trigger-constrained one-way invalidation).
  • Migrations run as the owning role via MIGRATE_DATABASE_URL (falls back to DATABASE_URL for single-role setups, so tests and scripts are unchanged).
  • Fresh compose volumes create the login via infra/initdb/; existing databases need one line, documented in the deployment guide: ALTER ROLE edc_app LOGIN PASSWORD '...'.
  • The migration also hands ownership of pre-split ducklake_% catalog schemas to edc_app so in-place catalog upgrades keep working. Serial-owned sequences are skipped — they cannot change owner independently and follow their table; this failed on a synthetic pre-split schema until fixed, so it is worth a close look.
  • New integration tests prove the runtime role cannot disable or drop the triggers, TRUNCATE, or UPDATE/DELETE the trail, and can still read and append. Also verified live in psql against the compose stack.

System-level audit review (/admin/audit)

Events written with no study — logins, lockouts, account lifecycle, cross-study role changes — were recorded but unreviewable: no endpoint, no UI, absent from the archive. GET /admin/audit (system-administration gated, mirroring /admin/access-log) plus a System audit page now cover them.

Decision to review: these events deliberately stay out of the per-study archive. They span studies, and bundling the global trail into one study's inspection copy would leak activity from the others. The /admin/audit CSV is their inspection copy.

Complete CSV exports

The route exports silently truncated at 10,000 rows — a truncated inspection copy with no warning. Exports now stream the full trail with keyset pagination. The cursor is the id alone: a JS Date cursor loses Postgres's microsecond precision and silently drops rows (caught by a test crossing the batch boundary), so the export orders by id rather than the UI's timestamp sort. Two smaller landmines fixed along the way: Readable.from() defaults to objectMode, which breaks Fastify replies, and raw Date params in drizzle sql fragments miss the driver's type mapping.

Timestamps, UI, and docs

  • The review UI rendered toLocaleString() with no timezone label, so two reviewers in different zones read different strings for the same event (E6(R3) 4.2.2(d) wants unambiguous times). It now renders labeled UTC, and exposes the from/to filters the API already had — compliance.md claimed time filtering that the UI didn't have.
  • Both audit pages share one AuditTrail component (studyId null selects the system scope; Biome's useHookAtTopLevel pushed the design away from hook-as-prop).
  • deployment.md gains Database roles and Clock synchronization sections plus checklist items — host NTP was an unaddressed dependency of the "time-stamped" claim.
  • Traceability matrix: P11-01/P11-02/E6-03 mechanisms updated to match what now exists; stale .qmd evidence paths from the Astro migration fixed.

Verification

  • Validation pack regenerated: 485/485, no problems.
  • pnpm check green (lint, typecheck, all package suites; API at 273).
  • Compose stack rebuilt on split credentials: boots, logs in, writes audit events as edc_app; all four bypass attempts rejected in psql.
  • Browser-verified both audit pages, date filters, and both CSV endpoints.

Dev-environment note: your local cluster already has the ALTER ROLE applied, and demo-admin was set is_system_admin = true in the dev DB to exercise the admin pages. Outside the repo, the two 21 CFR files are now hashed into the source mirror's MANIFEST.sha256 (full check passes).

tgerke added 5 commits July 30, 2026 07:52
ADR-0002 claimed the application role could not alter the append-only
triggers, but nothing enforced it: the app connected as the table owner.
Migration 0024 creates edc_app (no ownership, no TRIGGER/TRUNCATE
privilege), compose splits DATABASE_URL from MIGRATE_DATABASE_URL, and
integration tests prove the runtime role cannot disable, drop, or bypass
the triggers.
… UTC display

- /admin/audit (API + page) covers events written with no study — logins,
  account lifecycle, cross-study role changes — which were recorded but
  unreviewable. System-administration gated, mirroring /admin/access-log.
- CSV exports stream the complete trail with keyset pagination instead of
  silently truncating at 10,000 rows. The cursor is the id alone: a JS Date
  cursor loses Postgres's microsecond precision and drops rows.
- The review UI renders UTC (E6(R3) 4.2.2(d) unambiguous timestamps) and
  exposes the from/to time filters the API already had; the shared trail
  view now backs both the study and system pages.
- System events stay out of the per-study archive deliberately: they span
  studies, and bundling them into one study's inspection copy would leak
  activity from the others. The /admin/audit CSV is their inspection copy.
- deployment.md: new Database roles and Clock synchronization sections;
  checklist items for the split credential and NTP verification. Host time
  was an unaddressed dependency of the 11.10(e) time-stamp claim.
- Migration 0024 hands ownership of pre-split DuckLake catalog schemas to
  edc_app so in-place catalog upgrades keep working after the role split;
  existing deployments need only the ALTER ROLE ... LOGIN step.
- Traceability matrix: point E6-01 and DP-02 evidence at the Astro doc
  paths that replaced the retired .qmd files.
Biome correctly flagged the hook-as-prop pattern (useHookAtTopLevel);
passing studyId (null = system scope) into AuditTrail keeps the single
hook call at the top level and thins both pages.
A serial/identity sequence cannot change owner on its own (it follows its
table), so the upgrade loop in migration 0024 failed on any pre-split
catalog containing one. Verified against a synthetic pre-split schema:
tables carry their owned sequences and indexes; standalone relations
transfer explicitly.
@tgerke
tgerke merged commit cc5fd8f into main Jul 30, 2026
1 check passed
@tgerke
tgerke deleted the fix/audit-trail-gaps branch July 30, 2026 15:25
tgerke added a commit that referenced this pull request Jul 30, 2026
CHANGELOG entry for the PR #115 changes (missed the lands-with-the-PR
convention); review guide describes the UTC display, time filters, and
uncapped export and points at the system trail; user-admin guide gains a
System audit trail section; the data-lifecycle 4.2.2(b) passage now
states the enforced role boundary instead of implying the app credential
could modify the trail.
rake93 pushed a commit to rake93/edc-core that referenced this pull request Aug 13, 2026
Full scripted regeneration on a fresh stack: audit-trail.png now shows
the UTC column and time-range filters that landed in PR tgerke#115, and the
new system-audit.png illustrates /admin/audit in the user-admin guide.
The audit shots wait on a table cell, not getByText — the action names
also appear as hidden facet <option>s, which never become visible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant