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

apps/api/drizzle/0024_privilege_separation.sql

@@ -0,0 +1,74 @@
+-- Privilege separation (ADR-0002; traceability P11-01).
+--
+-- The append-only triggers in 0001 reject UPDATE/DELETE through any SQL
+-- path, but a role that owns the tables can ALTER TABLE ... DISABLE TRIGGER
+-- or TRUNCATE around them. The runtime role (edc_app) therefore never owns
+-- clinical tables: migrations run as the owning role, the API connects as
+-- edc_app, and edc_app holds no TRIGGER, TRUNCATE, or REFERENCES privilege
+-- anywhere, so it cannot disable, drop, or bypass a trigger.
+--
+-- edc_app is created NOLOGIN if missing so this migration succeeds on any
+-- database. Deployments make it connectable themselves (ALTER ROLE edc_app
+-- LOGIN PASSWORD '...'); infra/initdb does this for the local compose stack.
+-- CREATE on the database is needed because the API creates one DuckLake
+-- catalog schema per study.
+DO $$
+BEGIN
+  IF NOT EXISTS (SELECT FROM pg_roles WHERE rolname = 'edc_app') THEN
+    CREATE ROLE edc_app NOLOGIN;
+  END IF;
+  EXECUTE format('GRANT CREATE ON DATABASE %I TO edc_app', current_database());
+END $$;
+--> statement-breakpoint
+GRANT USAGE ON SCHEMA public TO edc_app;
+--> statement-breakpoint
+GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA public TO edc_app;
+--> statement-breakpoint
+GRANT USAGE, SELECT ON ALL SEQUENCES IN SCHEMA public TO edc_app;
+--> statement-breakpoint
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO edc_app;
+--> statement-breakpoint
+ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT USAGE, SELECT ON SEQUENCES TO edc_app;
+--> statement-breakpoint
+-- Append-only tables: the trigger is the enforcement; withholding UPDATE and
+-- DELETE outright is defense in depth. A migration adding a new append-only
+-- table must revoke these the same way (default privileges grant them).
+REVOKE UPDATE, DELETE, TRUNCATE ON audit_events, item_value_versions, study_metadata_versions, codings, rtsm_events, subject_unblindings, protocol_versions FROM edc_app;
+--> statement-breakpoint
+-- Signatures keep UPDATE for the one-way invalidation transition; the
+-- signatures_guard trigger constrains it to exactly that.
+REVOKE DELETE, TRUNCATE ON signatures FROM edc_app;
+--> statement-breakpoint
+-- Existing deployments: DuckLake catalog schemas created before the role
+-- split are owned by the old single role, and the API must own them to run
+-- in-place catalog upgrades at startup. Transfer the ones this role owns;
+-- schemas created after the split are made by edc_app directly. ALTER TABLE
+-- ... OWNER TO also covers sequences and views.
+DO $$
+DECLARE
+  s record;
+  c record;
+BEGIN
+  FOR s IN
+    SELECT n.oid, n.nspname
+    FROM pg_namespace n
+    JOIN pg_roles r ON r.oid = n.nspowner
+    WHERE n.nspname LIKE 'ducklake\_%' AND r.rolname = current_user
+  LOOP
+    EXECUTE format('ALTER SCHEMA %I OWNER TO edc_app', s.nspname);
+    FOR c IN
+      SELECT cl.relname FROM pg_class cl
+      WHERE cl.relnamespace = s.oid AND cl.relkind IN ('r', 'p', 'S', 'v', 'm')
+        -- Sequences owned by a table column (serial/identity) cannot change
+        -- owner on their own; they follow their table's ALTER automatically.
+        AND NOT EXISTS (
+          SELECT 1 FROM pg_depend d
+          WHERE d.objid = cl.oid
+            AND d.classid = 'pg_class'::regclass
+            AND d.deptype IN ('a', 'i')
+        )
+    LOOP
+      EXECUTE format('ALTER TABLE %I.%I OWNER TO edc_app', s.nspname, c.relname);
+    END LOOP;
+  END LOOP;
+END $$;

apps/api/drizzle/meta/_journal.json

@@ -169,6 +169,13 @@
       "when": 1784691027650,
       "tag": "0023_query_source",
       "breakpoints": true
+    },
+    {
+      "idx": 24,
+      "version": "7",
+      "when": 1785436800000,
+      "tag": "0024_privilege_separation",
+      "breakpoints": true
     }
   ]
 }

apps/api/src/db/audit.test.ts

@@ -207,3 +207,69 @@ describe.skipIf(!dbAvailable)("audit core (integration)", () => {
     );
   });
 });
+
+describe.skipIf(!dbAvailable)("privilege separation (integration)", () => {
+  // The runtime role (migration 0024) must not be able to reach around the
+  // append-only triggers. Each attempt runs in its own transaction under
+  // SET LOCAL ROLE, which the test connection can assume because the dev/CI
+  // migration role is superuser; rollback restores it.
+  const own = createDb();
+
+  beforeAll(async () => {
+    await runMigrations();
+  });
+
+  afterAll(async () => {
+    await own.client.end();
+  });
+
+  function asAppRole(statement: string) {
+    return own.client.begin(async (tx) => {
+      await tx`SET LOCAL ROLE edc_app`;
+      await tx.unsafe(statement);
+    });
+  }
+
+  it("cannot disable the append-only trigger", async () => {
+    await expectRejection(
+      asAppRole("ALTER TABLE audit_events DISABLE TRIGGER audit_events_append_only"),
+      /must be owner/,
+    );
+  });
+
+  it("cannot drop the append-only trigger", async () => {
+    await expectRejection(
+      asAppRole("DROP TRIGGER audit_events_append_only ON audit_events"),
+      /must be owner/,
+    );
+  });
+
+  it("cannot TRUNCATE the audit trail or version history", async () => {
+    await expectRejection(asAppRole("TRUNCATE audit_events"), /permission denied/);
+    await expectRejection(asAppRole("TRUNCATE item_value_versions"), /permission denied/);
+  });
+
+  it("lacks UPDATE and DELETE privileges on append-only tables entirely", async () => {
+    await expectRejection(asAppRole("UPDATE audit_events SET reason = 'x'"), /permission denied/);
+    await expectRejection(asAppRole("DELETE FROM audit_events"), /permission denied/);
+    await expectRejection(asAppRole("DELETE FROM signatures"), /permission denied/);
+  });
+
+  it("can still read and append", async () => {
+    const suffix = randomUUID().slice(0, 8);
+    await own.client.begin(async (tx) => {
+      await tx`SET LOCAL ROLE edc_app`;
+      const [user] = await tx`
+        INSERT INTO users (username, email, full_name, password_hash)
+        VALUES (${`priv-${suffix}`}, ${`priv-${suffix}@example.com`}, 'Priv Test', 'not-a-real-hash')
+        RETURNING id`;
+      if (!user) throw new Error("insert as edc_app failed");
+      await tx`
+        INSERT INTO audit_events (actor_id, action, entity_type, entity_id)
+        VALUES (${user.id}, 'auth.login', 'user', ${user.id})`;
+      const [row] = await tx`
+        SELECT count(*)::int AS n FROM audit_events WHERE actor_id = ${user.id}`;
+      if (row?.n !== 1) throw new Error("select as edc_app failed");
+    });
+  });
+});

apps/api/src/db/client.ts

@@ -8,6 +8,13 @@ export function databaseUrl(): string {
   return process.env.DATABASE_URL ?? DEFAULT_DATABASE_URL;
 }
 
+// Migrations run as the role that owns the tables; the runtime role
+// (edc_app, migration 0024) cannot alter the append-only triggers. Falls
+// back to DATABASE_URL for single-role setups (tests, local scripts).
+export function migrateDatabaseUrl(): string {
+  return process.env.MIGRATE_DATABASE_URL ?? databaseUrl();
+}
+
 export function createDb(url = databaseUrl()) {
   const client = postgres(url, { onnotice: () => {} });
   return { db: drizzle(client, { schema }), client };

apps/api/src/db/migrate.ts

@@ -2,9 +2,9 @@ import path from "node:path";
 import { fileURLToPath } from "node:url";
 import { sql } from "drizzle-orm";
 import { migrate } from "drizzle-orm/postgres-js/migrator";
-import { createDb, databaseUrl } from "./client.js";
+import { createDb, migrateDatabaseUrl } from "./client.js";
 
-export async function runMigrations(url = databaseUrl()): Promise<void> {
+export async function runMigrations(url = migrateDatabaseUrl()): Promise<void> {
   const { db, client } = createDb(url);
   const migrationsFolder = path.join(fileURLToPath(import.meta.url), "../../../drizzle");
   try {

apps/api/src/routes/audit.test.ts

@@ -6,7 +6,7 @@ import { hashPassword } from "../auth/password.js";
 import { grantRole } from "../auth/rbac.js";
 import { createDb, databaseUrl } from "../db/client.js";
 import { runMigrations } from "../db/migrate.js";
-import { roles, sites, studies, users } from "../db/schema/index.js";
+import { auditEvents, roles, sites, studies, users } from "../db/schema/index.js";
 import { buildServer } from "../server.js";
 
 const { db, client } = createDb();
@@ -140,4 +140,66 @@ describe.skipIf(!dbAvailable)("audit trail review", () => {
     );
     expect(lines.some((line) => line.includes("subject.enrolled"))).toBe(true);
   });
+
+  describe("system scope (/admin/audit)", () => {
+    let adminToken = "";
+    let adminId = "";
+
+    beforeAll(async () => {
+      const [admin] = await db
+        .insert(users)
+        .values({
+          username: `sysadmin-${suffix}`,
+          email: `sysadmin-${suffix}@example.com`,
+          fullName: "System Admin",
+          passwordHash: await hashPassword(PASSWORD),
+          isSystemAdmin: true,
+        })
+        .returning();
+      if (!admin) throw new Error("fixture failed");
+      adminId = admin.id;
+      adminToken = (
+        await server.inject({
+          method: "POST",
+          url: "/auth/login",
+          payload: { username: `sysadmin-${suffix}`, password: PASSWORD },
+        })
+      ).json().token;
+    });
+
+    it("requires system administration, not just audit.review", async () => {
+      const denied = await get("/admin/audit", fx.dmToken);
+      expect(denied.statusCode).toBe(403);
+    });
+
+    it("lists only events without a study (logins, account lifecycle)", async () => {
+      const res = await get("/admin/audit", adminToken);
+      expect(res.statusCode).toBe(200);
+      const body = res.json();
+      const actions = body.events.map((e: { action: string }) => e.action);
+      expect(actions).toContain("auth.login");
+      expect(actions).not.toContain("subject.enrolled");
+      expect(body.facets.actions).toContain("auth.login");
+    });
+
+    it("streams a complete CSV across keyset batches (no row cap)", async () => {
+      // 1,500 rows crosses the 1,000-row batch boundary; a unique entity
+      // type isolates the assertion from other fixtures' events.
+      const entityType = `bulk-${suffix}`;
+      const bulk = Array.from({ length: 1500 }, (_, i) => ({
+        actorId: adminId,
+        action: "auth.login",
+        entityType,
+        entityId: `row-${i}`,
+      }));
+      await db.insert(auditEvents).values(bulk);
+
+      const res = await get(`/admin/audit?format=csv&entityType=${entityType}`, adminToken);
+      expect(res.statusCode).toBe(200);
+      const lines = res.body.split("\n").filter((line: string) => line.length > 0);
+      expect(lines).toHaveLength(1501); // header + every row
+      const ids = new Set(lines.slice(1).map((line: string) => line.split(",")[5]));
+      expect(ids.size).toBe(1500); // keyset pagination neither drops nor repeats
+    });
+  });
 });