From 1c548b8d90a5111acd46fbc7e212bab4f79f10f3 Mon Sep 17 00:00:00 2001 From: David Condrey Date: Tue, 4 Aug 2026 23:52:48 -0700 Subject: [PATCH] feat(server): record every authorized access to the CRM contact store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing recorded reads, exports or deletes on the one table holding directly-identifying PII. requireTeamRole already resolved the operator and their role on every request and discarded both. crm_audit_log lives in CRM_DB, beside the data it describes: every entry names a target_id that points into the contact store, and the analytics database is deliberately free of anything that resolves to a named person. An unbound deployment has no audit table for the same reason it has no contacts. The entry is written BEFORE the handler runs. Logging afterwards leaves any failure between the disclosure and the record as an access that happened and was never written down, and for a DELETE there is then nothing left to notice the gap against. Writing first inverts that: the insert throws, the handler never runs, nothing was read or changed. The cost is that an entry states an operator was authorized to do this to this id, not that it worked, and a request that then 404s is recorded like any other — which for id probing is the more useful reading. crmGate(role, action) composes the role guard, the per-operator rate limit and the audit entry into one middleware, replacing the requireTeamRole/crmRateLimit pair repeated at fourteen call sites. A route cannot be given a role without also being given an audited action. Order is load-bearing: a request refused on role or shed by the limiter never reached the data, so it is not an access to record, and letting it write one would hand a single session the log's own denial of service. Entries hold ids, a role, an action name and a timestamp — never a contact field. That is what makes them safe to outlive the contact: once the row is deleted the pointer resolves to nothing, so erasure does not have to reach in here, and a log an operator can clear by deleting the contact is not evidence of anything. Nothing updates or deletes an entry except the retention cron. GET /api/crm/audit is admin-only, and not for the bulk-disclosure reason that gates the export — no entry carries PII. It is that the entries are about the deployment's own operators, which is oversight in an administrator's hands and surveillance in a peer's. Reading it is itself recorded. The log is the one CRM table on a schedule: CRM_AUDIT_RETENTION_DAYS, default 365, purged by its own cron job so an unreachable CRM_DB cannot stop raw events being purged from the analytics database. Longer than RAW_RETENTION_DAYS deliberately — raw events are visitors' data and the short window is the privacy measure, while these entries record what operators did, and an access log that expires before the misuse it evidences is noticed has protected nobody. Contacts stay on no schedule. Binding CRM_DB now also claims dpv:ActivityMonitoring. The claims are signed, and a deployment that records every access to its contact store is applying a measure the previous claim set did not name. Also, autonomously: dropped the duplicated middleware pair from all fourteen CRM routes; documented the audit surface in docs/api.md. --- .../0002_marvelous_agent_zero.sql | 13 + .../migrations-crm/meta/0002_snapshot.json | 379 ++++++++++++++ apps/server/migrations-crm/meta/_journal.json | 7 + apps/server/src/db/crm-schema.ts | 50 ++ apps/server/src/db/crm.ts | 90 +++- apps/server/src/env.ts | 4 + apps/server/src/lib/constants.ts | 14 + apps/server/src/lib/dpv.ts | 12 +- apps/server/src/lib/retention.ts | 38 +- apps/server/src/lib/scheduled.ts | 9 +- apps/server/src/routes/crm.ts | 148 +++++- apps/server/test/crm-audit.test.ts | 488 ++++++++++++++++++ apps/server/test/dpv.test.ts | 8 + apps/server/wrangler.jsonc | 16 + docs/api.md | 46 +- packages/shared/src/crm.ts | 44 ++ 16 files changed, 1332 insertions(+), 34 deletions(-) create mode 100644 apps/server/migrations-crm/0002_marvelous_agent_zero.sql create mode 100644 apps/server/migrations-crm/meta/0002_snapshot.json create mode 100644 apps/server/test/crm-audit.test.ts diff --git a/apps/server/migrations-crm/0002_marvelous_agent_zero.sql b/apps/server/migrations-crm/0002_marvelous_agent_zero.sql new file mode 100644 index 0000000..520d7ff --- /dev/null +++ b/apps/server/migrations-crm/0002_marvelous_agent_zero.sql @@ -0,0 +1,13 @@ +CREATE TABLE `crm_audit_log` ( + `id` text PRIMARY KEY NOT NULL, + `site_id` text NOT NULL, + `actor_user_id` text NOT NULL, + `actor_role` text NOT NULL, + `action` text NOT NULL, + `target_id` text, + `occurred_at` integer NOT NULL +); +--> statement-breakpoint +CREATE INDEX `idx_crm_audit_site_time` ON `crm_audit_log` (`site_id`,`occurred_at`);--> statement-breakpoint +CREATE INDEX `idx_crm_audit_site_target` ON `crm_audit_log` (`site_id`,`target_id`);--> statement-breakpoint +CREATE INDEX `idx_crm_audit_occurred` ON `crm_audit_log` (`occurred_at`); \ No newline at end of file diff --git a/apps/server/migrations-crm/meta/0002_snapshot.json b/apps/server/migrations-crm/meta/0002_snapshot.json new file mode 100644 index 0000000..f88f62d --- /dev/null +++ b/apps/server/migrations-crm/meta/0002_snapshot.json @@ -0,0 +1,379 @@ +{ + "version": "6", + "dialect": "sqlite", + "id": "d5ac3527-c40c-4c64-9a7c-33ee2819ab19", + "prevId": "6dbe8789-6aab-43b0-adb2-f05317a871c8", + "tables": { + "companies": { + "name": "companies", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "site_id": { + "name": "site_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "name": { + "name": "name", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "domain": { + "name": "domain", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "status": { + "name": "status", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'lead'" + }, + "notes": { + "name": "notes", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "owner_user_id": { + "name": "owner_user_id", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "created_at": { + "name": "created_at", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "updated_at": { + "name": "updated_at", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false + } + }, + "indexes": { + "idx_companies_site_created": { + "name": "idx_companies_site_created", + "columns": [ + "site_id", + "created_at" + ], + "isUnique": false + }, + "idx_companies_site_status": { + "name": "idx_companies_site_status", + "columns": [ + "site_id", + "status" + ], + "isUnique": false + }, + "idx_companies_site_name": { + "name": "idx_companies_site_name", + "columns": [ + "site_id", + "name" + ], + "isUnique": true + }, + "idx_companies_site_domain": { + "name": "idx_companies_site_domain", + "columns": [ + "site_id", + "domain" + ], + "isUnique": true + } + }, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + }, + "contacts": { + "name": "contacts", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "site_id": { + "name": "site_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "external_user_id": { + "name": "external_user_id", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "email": { + "name": "email", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "name": { + "name": "name", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "phone": { + "name": "phone", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "company": { + "name": "company", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "company_id": { + "name": "company_id", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "title": { + "name": "title", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "status": { + "name": "status", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false, + "default": "'lead'" + }, + "source": { + "name": "source", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "notes": { + "name": "notes", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "owner_user_id": { + "name": "owner_user_id", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "created_at": { + "name": "created_at", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "updated_at": { + "name": "updated_at", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false + } + }, + "indexes": { + "idx_contacts_site_created": { + "name": "idx_contacts_site_created", + "columns": [ + "site_id", + "created_at" + ], + "isUnique": false + }, + "idx_contacts_site_status": { + "name": "idx_contacts_site_status", + "columns": [ + "site_id", + "status" + ], + "isUnique": false + }, + "idx_contacts_site_company": { + "name": "idx_contacts_site_company", + "columns": [ + "site_id", + "company_id" + ], + "isUnique": false + }, + "idx_contacts_site_email": { + "name": "idx_contacts_site_email", + "columns": [ + "site_id", + "email" + ], + "isUnique": true + }, + "idx_contacts_site_extuser": { + "name": "idx_contacts_site_extuser", + "columns": [ + "site_id", + "external_user_id" + ], + "isUnique": true + } + }, + "foreignKeys": { + "contacts_company_id_companies_id_fk": { + "name": "contacts_company_id_companies_id_fk", + "tableFrom": "contacts", + "tableTo": "companies", + "columnsFrom": [ + "company_id" + ], + "columnsTo": [ + "id" + ], + "onDelete": "no action", + "onUpdate": "no action" + } + }, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + }, + "crm_audit_log": { + "name": "crm_audit_log", + "columns": { + "id": { + "name": "id", + "type": "text", + "primaryKey": true, + "notNull": true, + "autoincrement": false + }, + "site_id": { + "name": "site_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "actor_user_id": { + "name": "actor_user_id", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "actor_role": { + "name": "actor_role", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "action": { + "name": "action", + "type": "text", + "primaryKey": false, + "notNull": true, + "autoincrement": false + }, + "target_id": { + "name": "target_id", + "type": "text", + "primaryKey": false, + "notNull": false, + "autoincrement": false + }, + "occurred_at": { + "name": "occurred_at", + "type": "integer", + "primaryKey": false, + "notNull": true, + "autoincrement": false + } + }, + "indexes": { + "idx_crm_audit_site_time": { + "name": "idx_crm_audit_site_time", + "columns": [ + "site_id", + "occurred_at" + ], + "isUnique": false + }, + "idx_crm_audit_site_target": { + "name": "idx_crm_audit_site_target", + "columns": [ + "site_id", + "target_id" + ], + "isUnique": false + }, + "idx_crm_audit_occurred": { + "name": "idx_crm_audit_occurred", + "columns": [ + "occurred_at" + ], + "isUnique": false + } + }, + "foreignKeys": {}, + "compositePrimaryKeys": {}, + "uniqueConstraints": {}, + "checkConstraints": {} + } + }, + "views": {}, + "enums": {}, + "_meta": { + "schemas": {}, + "tables": {}, + "columns": {} + }, + "internal": { + "indexes": {} + } +} \ No newline at end of file diff --git a/apps/server/migrations-crm/meta/_journal.json b/apps/server/migrations-crm/meta/_journal.json index b51f69d..6d79167 100644 --- a/apps/server/migrations-crm/meta/_journal.json +++ b/apps/server/migrations-crm/meta/_journal.json @@ -15,6 +15,13 @@ "when": 1785902667995, "tag": "0001_perfect_tombstone", "breakpoints": true + }, + { + "idx": 2, + "version": "6", + "when": 1785912191326, + "tag": "0002_marvelous_agent_zero", + "breakpoints": true } ] } \ No newline at end of file diff --git a/apps/server/src/db/crm-schema.ts b/apps/server/src/db/crm-schema.ts index b520eeb..8759825 100644 --- a/apps/server/src/db/crm-schema.ts +++ b/apps/server/src/db/crm-schema.ts @@ -12,6 +12,8 @@ // designed in rather than deferred: // • Contacts are NOT on the raw-event retention schedule (`lib/retention.ts`). A contact is a // business record with its own lifecycle; deleting it is an explicit act, not a cron side effect. +// `crm_audit_log` is the one table here that IS on a schedule, on its own longer window, because +// it is the one that grows without anybody deciding to add a row. // • No column here caches a derived `visitor_hash`. The ONLY bridge to analytics is // `external_user_id`, resolved at read time through an active `identified` consent record. When // retention purges that record (or its identity salt), the link severs on its own — nothing here @@ -109,3 +111,51 @@ export const contacts = sqliteTable( uniqueIndex('idx_contacts_site_extuser').on(t.site_id, t.external_user_id), ], ); + +/** + * Who touched the contact store, what they touched, and when. One row per authorized `/api/crm` + * request, written BEFORE the handler runs. + * + * WHY IT IS HERE rather than in the analytics database. Every row names a `target_id`, which is a + * pointer into this database's PII, and the analytics database is deliberately free of anything that + * resolves to a named person. Keeping the log beside the data it describes also means an unbound + * deployment has no audit table for the same reason it has no contacts — the extension does not + * exist — and that the log and the row it records are one database apart rather than two, which is + * the only reason the two writes can be reasoned about at all. + * + * WHAT IT DELIBERATELY DOES NOT HOLD is any contact FIELD. It records that contact `x` was read, not + * what reading it returned, so the log is a record about the OPERATOR and only a pointer to the + * subject. That is what makes it safe to outlive the contact: once the row is deleted the pointer + * resolves to nothing, so an erasure request does not have to reach in here — and must not, since a + * log an operator can clear by deleting the contact is not evidence of anything. Nothing in the API + * updates or deletes these rows; only the retention cron does. + * + * `actor_role` is stored rather than resolved at read time because it is the role the request was + * AUTHORIZED under. Roles change; "an admin exported this" has to stay true after they are demoted. + */ +export const crmAuditLog = sqliteTable( + 'crm_audit_log', + { + id: text('id').primaryKey(), + site_id: text('site_id').notNull(), + /** `users.id` in the ANALYTICS database, so no foreign key is possible — the same cross-database + * limitation as `contacts.owner_user_id`, and the same reason it is validated in the Worker. */ + actor_user_id: text('actor_user_id').notNull(), + actor_role: text('actor_role').notNull(), + /** One of `CRM_AUDIT_ACTIONS`. A closed set, so the log is filterable by equality. */ + action: text('action').notNull(), + /** The contact or company the request named, or NULL for a collection-level action. */ + target_id: text('target_id'), + occurred_at: integer('occurred_at').notNull(), + }, + (t) => [ + // The default view: one site's log, newest first. Also what the actor and action filters scan. + index('idx_crm_audit_site_time').on(t.site_id, t.occurred_at), + // "Everything anyone did to this contact" — the question an erasure or subject-access request + // asks, and the one a site-and-time scan answers worst. + index('idx_crm_audit_site_target').on(t.site_id, t.target_id), + // Retention purges across every site at once, so it needs the timestamp leading. Same shape and + // same reason as `idx_identity_salts_window_end` in the analytics schema. + index('idx_crm_audit_occurred').on(t.occurred_at), + ], +); diff --git a/apps/server/src/db/crm.ts b/apps/server/src/db/crm.ts index 73a4bb8..cc1a969 100644 --- a/apps/server/src/db/crm.ts +++ b/apps/server/src/db/crm.ts @@ -6,10 +6,11 @@ // never from a body, so a session with a role on one site cannot reach another site's contacts even // by guessing a contact id. That covers the contact→company link too: the foreign key proves the // company row exists, not that it belongs to the caller's site, so the site predicate is the check -// and the constraint is only the backstop. +// and the constraint is only the backstop. `purgeCrmAudit` is the one exception and says why: it runs +// from cron, on behalf of no request, and a per-site purge would be the same delete run N times. -import { normalizeCompanyDomain } from '@facet/shared'; -import { type SQL, and, desc, eq, isNotNull, or, sql } from 'drizzle-orm'; +import { type CrmAuditAction, normalizeCompanyDomain } from '@facet/shared'; +import { type SQL, and, desc, eq, isNotNull, lt, or, sql } from 'drizzle-orm'; import { drizzle } from 'drizzle-orm/d1'; import type { SQLiteColumn } from 'drizzle-orm/sqlite-core'; import type { MiddlewareHandler } from 'hono'; @@ -659,3 +660,86 @@ export async function deleteCompany( if (deleted.length === 0) return undefined; return { company, contacts_unlinked: counted[0]?.n ?? 0 }; } + +/** One entry as the audit log stores and returns it. There is no separate wire shape: every column + * is already an id, a role, an action name or a timestamp, so there is nothing to redact on the way + * out that was safe to record on the way in. */ +export type CrmAuditEntry = typeof crmSchema.crmAuditLog.$inferSelect; + +/** What an audit entry is written from. The actor and role come from the session guard that + * authorized the request, never from anything the caller sent. */ +export interface CrmAuditInput { + actorUserId: string; + actorRole: string; + action: CrmAuditAction; + targetId: string | null; + occurredAt: number; +} + +/** Append one entry. Insert-only by design — nothing in this module updates or deletes an entry + * except `purgeCrmAudit`, so a recorded access cannot be rewritten by the operator it names. */ +export async function recordCrmAccess( + binding: D1Database, + siteId: string, + entry: CrmAuditInput, +): Promise { + await crmDb(binding).insert(crmSchema.crmAuditLog).values({ + id: crypto.randomUUID(), + site_id: siteId, + actor_user_id: entry.actorUserId, + actor_role: entry.actorRole, + action: entry.action, + target_id: entry.targetId, + occurred_at: entry.occurredAt, + }); +} + +export interface ListCrmAuditOptions { + /** Typed to the closed set rather than to `string`: an action outside it matches nothing, so + * accepting one would answer "no such access" to a question that was never asked. */ + action?: CrmAuditAction; + actorUserId?: string; + targetId?: string; + limit: number; + offset: number; +} + +/** One site's audit entries, newest first. Every filter is an equality — see the wire schema for why + * there is no substring search over a log of ids. */ +export async function listCrmAudit( + binding: D1Database, + siteId: string, + opts: ListCrmAuditOptions, +): Promise<{ entries: CrmAuditEntry[]; total: number }> { + const filters = [eq(crmSchema.crmAuditLog.site_id, siteId)]; + if (opts.action) filters.push(eq(crmSchema.crmAuditLog.action, opts.action)); + if (opts.actorUserId) { + filters.push(eq(crmSchema.crmAuditLog.actor_user_id, opts.actorUserId)); + } + if (opts.targetId) filters.push(eq(crmSchema.crmAuditLog.target_id, opts.targetId)); + const where = and(...filters); + const client = crmDb(binding); + const [entries, totalRow] = await Promise.all([ + client + .select() + .from(crmSchema.crmAuditLog) + .where(where) + // Two requests inside the same millisecond would otherwise come back in an arbitrary and + // unstable order, which for a paged log means an entry can be shown twice or skipped. `id` + // is the tiebreak because it is the only unique column. + .orderBy(desc(crmSchema.crmAuditLog.occurred_at), desc(crmSchema.crmAuditLog.id)) + .limit(opts.limit) + .offset(opts.offset), + client.select({ n: sql`count(*)` }).from(crmSchema.crmAuditLog).where(where).get(), + ]); + return { entries, total: totalRow?.n ?? 0 }; +} + +/** Delete audit entries older than `cutoff`, across every site — the retention cron acts for the + * deployment, not for a request, and there is no site to scope it to. Returns rows deleted. */ +export async function purgeCrmAudit(binding: D1Database, cutoff: number): Promise { + const res = await crmDb(binding) + .delete(crmSchema.crmAuditLog) + .where(lt(crmSchema.crmAuditLog.occurred_at, cutoff)); + return res.meta.changes ?? 0; +} diff --git a/apps/server/src/env.ts b/apps/server/src/env.ts index ad814ca..f14a3d7 100644 --- a/apps/server/src/env.ts +++ b/apps/server/src/env.ts @@ -18,6 +18,10 @@ export interface Env { AE?: AnalyticsEngineDataset; /** Rolling retention window for raw events, in days (string var). */ RAW_RETENTION_DAYS: string; + /** Rolling retention window for the CRM audit log, in days (string var). Optional: unset means the + * default in `constants.ts`, which is deliberately longer than the raw-event window because the log + * records what OPERATORS did rather than what visitors did. Only read when `CRM_DB` is bound. */ + CRM_AUDIT_RETENTION_DAYS?: string; /** Cloudflare account id (var), used for Analytics Engine SQL-over-HTTP reads. */ CF_ACCOUNT_ID: string; /** Cloudflare API token (Worker secret) for Analytics Engine SQL-over-HTTP reads. */ diff --git a/apps/server/src/lib/constants.ts b/apps/server/src/lib/constants.ts index cb27827..62ee44f 100644 --- a/apps/server/src/lib/constants.ts +++ b/apps/server/src/lib/constants.ts @@ -9,6 +9,20 @@ export const SALT_BYTES = 32 as const; /** Default rolling retention window for raw events, in days. */ export const DEFAULT_RAW_RETENTION_DAYS = 90 as const; +/** + * Default retention for the CRM audit log, in days. + * + * Deliberately longer than the raw-event window, because it answers a different question about + * different people. Raw events are visitors' data and the short window IS the privacy measure; audit + * entries are a record of what the deployment's own operators did with contact data, and an access + * log that expires before the misuse it evidences is noticed has protected nobody. A year covers an + * annual review and the interval in which a complaint or a breach is normally traced. + * + * Bounded rather than kept forever, because "we never delete it" is not a retention policy and an + * append-only table with no ceiling is a growth defect however small each row is. + */ +export const DEFAULT_CRM_AUDIT_RETENTION_DAYS = 365 as const; + /** CORS max-age for preflight responses, in seconds. */ export const CORS_MAX_AGE = 86400 as const; diff --git a/apps/server/src/lib/dpv.ts b/apps/server/src/lib/dpv.ts index 19e65c0..a8a1f79 100644 --- a/apps/server/src/lib/dpv.ts +++ b/apps/server/src/lib/dpv.ts @@ -34,8 +34,9 @@ export const DPV_PD_CONTEXT = { * active signed consent record, never by legitimate interest. * • Pseudonymisation stops being the sole measure. It still describes the analytics half, but * directly-supplied contact details are not pseudonymised; what protects them is access control - * (an authenticated operator session with a team role, never an API key), so that is named - * alongside it rather than letting one term imply coverage it does not have. + * (an authenticated operator session with a team role, never an API key) and a record of every + * access made under it, so both are named alongside it rather than letting one term imply + * coverage it does not have. */ export function privacyDpvClaims(env: Env): Record { if (!env.CRM_DB) { @@ -62,6 +63,13 @@ export function privacyDpvClaims(env: Env): Record { 'dpv:hasTechnicalOrganisationalMeasure': [ 'dpv:Pseudonymisation', 'dpv:AccessControlMethod', + // The CRM audit log: every authorized request against the contact store is recorded before + // it runs. `dpv:ActivityMonitoring` rather than `dpv:RecordsOfActivities`, which in DPV sits + // with ROPA and the other compliance documents — this is a live access log, not a register + // of processing. Claimed only on the CRM branch because it is the only data this deployment + // holds that a person can read one record at a time, and therefore the only data where who + // looked is a fact worth keeping. + 'dpv:ActivityMonitoring', ], 'dpv:hasDataSubject': 'dpv:Customer', // The categories the contact schema has columns for. Free-text fields an operator may fill diff --git a/apps/server/src/lib/retention.ts b/apps/server/src/lib/retention.ts index 7961fcd..28c0cb6 100644 --- a/apps/server/src/lib/retention.ts +++ b/apps/server/src/lib/retention.ts @@ -1,11 +1,20 @@ // Retention cleanup: delete raw events, sessions, salts, and identity mappings older than the rolling // window. `event_rollups` are durable history and are never deleted. Invoked from the cron handler. +// +// The optional CRM has its own window and its own function. Contacts are NOT on any schedule — a +// contact is a business record that is deleted by an explicit act, never by a cron — but the audit +// log recording who read them is, because it is the one CRM table that grows on its own. import { lt } from 'drizzle-orm'; +import { purgeCrmAudit } from '../db/crm.js'; import { db } from '../db/queries.js'; import * as schema from '../db/schema.js'; import type { Env } from '../env.js'; -import { DAY_MS, DEFAULT_RAW_RETENTION_DAYS } from './constants.js'; +import { + DAY_MS, + DEFAULT_CRM_AUDIT_RETENTION_DAYS, + DEFAULT_RAW_RETENTION_DAYS, +} from './constants.js'; /** * The deployment's raw-data window in days — the ONE reading of `RAW_RETENTION_DAYS`. Every caller @@ -35,3 +44,30 @@ export async function enforceRetention(env: Env, now: number): Promise { // the at-rest raw uid. (Elevation already stops the instant a record expires or is revoked.) await db(env).delete(schema.consentRecords).where(lt(schema.consentRecords.granted_at, cutoff)); } + +/** + * The audit log's window in days — the ONE reading of `CRM_AUDIT_RETENTION_DAYS`, validated exactly + * as `retentionDays` validates its own var and for the same reason: a zero or negative value puts the + * cutoff at or after `now` and every run would erase the log it was meant to age. + */ +export function crmAuditRetentionDays(env: Env): number { + const days = Number.parseInt(env.CRM_AUDIT_RETENTION_DAYS ?? '', 10); + return Number.isInteger(days) && days >= 1 ? days : DEFAULT_CRM_AUDIT_RETENTION_DAYS; +} + +/** + * Purge audit entries older than `CRM_AUDIT_RETENTION_DAYS`. A no-op on a deployment with no CRM + * binding, which has no such table — the extension being off means it does not exist, not that it is + * empty. + * + * Separate from `enforceRetention` rather than folded into it because they are different windows over + * different databases, and because the cron isolates failures per job: an unreachable `CRM_DB` must + * not be able to stop raw events being purged from the analytics one. + * + * Returns entries purged, which is zero on an unbound deployment for the same reason it is zero on a + * quiet one. + */ +export async function enforceCrmAuditRetention(env: Env, now: number): Promise { + if (!env.CRM_DB) return 0; + return purgeCrmAudit(env.CRM_DB, now - crmAuditRetentionDays(env) * DAY_MS); +} diff --git a/apps/server/src/lib/scheduled.ts b/apps/server/src/lib/scheduled.ts index f11ad17..8b45c3d 100644 --- a/apps/server/src/lib/scheduled.ts +++ b/apps/server/src/lib/scheduled.ts @@ -4,7 +4,7 @@ import type { Env } from '../env.js'; import { HOUR_MS } from './constants.js'; import { createLogger } from './log.js'; -import { enforceRetention } from './retention.js'; +import { enforceCrmAuditRetention, enforceRetention } from './retention.js'; import { runRollups } from './rollups.js'; import { dayKey } from './salt.js'; import { buildSessions } from './sessions.js'; @@ -36,6 +36,13 @@ registerJob({ name: 'retention', run: (env, now) => enforceRetention(env, now), }); +// Optional: age out the CRM audit log on its own, longer window. No-op unless CRM_DB is bound. +registerJob({ + name: 'crm-audit-retention', + run: async (env, now) => { + await enforceCrmAuditRetention(env, now); + }, +}); // Optional: deliver anomaly-alert webhooks. No-op unless WEBHOOK_URL is configured. registerJob({ name: 'anomaly-alerts', diff --git a/apps/server/src/routes/crm.ts b/apps/server/src/routes/crm.ts index 3010464..c1af7ab 100644 --- a/apps/server/src/routes/crm.ts +++ b/apps/server/src/routes/crm.ts @@ -1,4 +1,4 @@ -// CRM endpoints — the optional contacts-and-companies extension. Two gates apply to every route +// CRM endpoints — the optional contacts-and-companies extension. Three gates apply to every route // here, in order. // // 1. THE BINDING. No `CRM_DB` means this deployment has no CRM database: 501 `crm_unavailable`, @@ -18,6 +18,10 @@ // `viewer` — who can see aggregate analytics — has no CRM access at all, because PII is a // different kind of access, not more of the same one. // +// 3. THE RECORD. Every request that clears both gates is written to the CRM audit log BEFORE its +// handler runs. `crmGate` below is what binds the three together, so a route cannot be given a +// role without also being given an audited action. +// // The contact→analytics link (`GET /contacts/:id/analytics`) never queries events by anything a // contact row controls. It resolves `external_user_id` through `findLinkedVisitorHashes`, which // returns hashes only from consent statements that verify against the deployment key — so a contact @@ -37,11 +41,14 @@ import { ContactCreateSchema, ContactListQuerySchema, ContactUpdateSchema, + type CrmAuditAction, + CrmAuditListQuerySchema, } from '@facet/shared'; import { vValidator } from '@hono/valibot-validator'; import { eq } from 'drizzle-orm'; -import { Hono } from 'hono'; +import { Hono, type MiddlewareHandler } from 'hono'; import { bodyLimit } from 'hono/body-limit'; +import { every } from 'hono/combine'; import { COMPANY_ROLLUP_MAX_CONTACTS, CONTACT_EXPORT_MAX_EVENTS, @@ -63,6 +70,8 @@ import { listCompanies, listCompanyContacts, listContacts, + listCrmAudit, + recordCrmAccess, requireCrm, requireCrmDb, uniqueConstraintText, @@ -72,6 +81,7 @@ import { import { db } from '../db/queries.js'; import * as schema from '../db/schema.js'; import type { AppEnv, Env } from '../env.js'; +import type { Role } from '../lib/accounts.js'; import { requireTeamRole } from '../lib/auth.js'; import { eraseConsentByExternalUserId, @@ -108,11 +118,72 @@ crmRoutes.use( * legitimate traffic and would punish their colleagues for it; keying per operator caps the session * that is actually doing it. * - * Applied AFTER the role guard at every call site, matching /api/event: an unauthenticated request is - * rejected before it can consume anyone's bucket, and `userId` is only set once a session resolves. + * Applied AFTER the role guard — see `crmGate`, which is now the only thing that applies it — matching + * /api/event: an unauthenticated request is rejected before it can consume anyone's bucket, and + * `userId` is only set once a session resolves. */ const crmRateLimit = rateLimit((c) => `crm:${c.get('userId') ?? 'unauthenticated'}`); +/** + * Record this request in the CRM audit log, then run it. + * + * BEFORE, NOT AFTER, and that ordering is the whole design. Logging afterwards means the log write + * is the last thing to happen, so any failure between the disclosure and the record — a D1 error, an + * eviction, a crash — leaves an access that happened and was never written down, and for a DELETE + * there is then nothing left to notice the gap against. Logging first inverts every one of those: + * the insert throws, the handler never runs, and nothing was read or changed. The order that can + * fail open is not available to an audit log. + * + * What it costs is precision about outcome. A request that goes on to 404 or to fail validation is + * recorded exactly like one that succeeded, so an entry states that an operator was authorized to do + * this to this id, not that it worked. That is the honest reading, and for the id-probing case it is + * the more useful one — a run of `contact.read` entries against ids that do not exist is a signal + * that a log of successes only would have thrown away. + * + * A create therefore names no target: the record it makes does not exist yet when its entry is + * written, and recording the new id would mean writing the entry afterwards, which is the ordering + * this rejects. The row's own `created_at` sits beside the entry's `occurred_at`, so the two are + * still correlatable — and a create is the one action that discloses nothing. + * + * The actor is re-checked rather than assumed. `crmGate` always runs `requireTeamRole` first, so + * both are set; if this middleware is ever mounted without it, an entry naming nobody is worse than + * a 401. + */ +function auditCrm(action: CrmAuditAction): MiddlewareHandler { + return async (c, next) => { + const actorUserId = c.get('userId'); + const actorRole = c.get('role'); + if (!actorUserId || !actorRole) { + throw new ApiError('unauthorized', 401); + } + await recordCrmAccess(requireCrmDb(c.env), c.get('siteId'), { + actorUserId, + actorRole, + action, + // Undefined on the collection routes, which name no single record. + targetId: c.req.param('id') ?? null, + occurredAt: Date.now(), + }); + return next(); + }; +} + +/** + * The one gate every CRM route passes: a team role, the per-operator rate limit, and the audit entry, + * composed in that order and applied as a single middleware. + * + * Composed rather than listed at each route because these three are one decision, not three. The + * previous form repeated `requireTeamRole(x), crmRateLimit` at fourteen call sites, which made + * "authorized but unrecorded" a thing a new route could quietly be — you cannot forget the audit + * action here without also forgetting the role, and a route with no role guard is not a route anyone + * ships. The order matters too: a request rejected for its role, or shed by the rate limiter, never + * reached the data and so is not an access to record — and letting it write one would hand a single + * session the ability to fill the audit table with entries for requests it was never allowed to make. + */ +function crmGate(need: Role, action: CrmAuditAction): MiddlewareHandler { + return every(requireTeamRole(need), crmRateLimit, auditCrm(action)); +} + /** Resolve a contact or raise the canonical 404. Scoped by the authorized site, so a contact id from * another site is indistinguishable from one that does not exist. */ async function loadContact(env: Env, siteId: string, id: string): Promise { @@ -195,8 +266,7 @@ function linkedHashes( */ crmRoutes.get( '/contacts', - requireTeamRole('analyst'), - crmRateLimit, + crmGate('analyst', 'contact.list'), vValidator('query', ContactListQuerySchema, validationErrorHook), async (c) => { const query = c.req.valid('query'); @@ -212,8 +282,7 @@ crmRoutes.get( crmRoutes.post( '/contacts', - requireTeamRole('analyst'), - crmRateLimit, + crmGate('analyst', 'contact.create'), vValidator('json', ContactCreateSchema, validationErrorHook), async (c) => { const body = c.req.valid('json'); @@ -229,15 +298,14 @@ crmRoutes.post( }, ); -crmRoutes.get('/contacts/:id', requireTeamRole('analyst'), crmRateLimit, async (c) => { +crmRoutes.get('/contacts/:id', crmGate('analyst', 'contact.read'), async (c) => { const contact = await loadContact(c.env, c.get('siteId'), c.req.param('id')); return c.json({ contact }); }); crmRoutes.patch( '/contacts/:id', - requireTeamRole('analyst'), - crmRateLimit, + crmGate('analyst', 'contact.update'), vValidator('json', ContactUpdateSchema, validationErrorHook), async (c) => { const body = c.req.valid('json'); @@ -267,7 +335,7 @@ crmRoutes.patch( * keyed by a salted hash, and with the consent record gone nothing can ever re-associate them with a * person; destroying the link is what erasure of the identifiable data means here. */ -crmRoutes.delete('/contacts/:id', requireTeamRole('admin'), crmRateLimit, async (c) => { +crmRoutes.delete('/contacts/:id', crmGate('admin', 'contact.delete'), async (c) => { const siteId = c.get('siteId'); const contact = await loadContact(c.env, siteId, c.req.param('id') ?? ''); // The two writes land in DIFFERENT databases and D1 has no transaction spanning them, so one of @@ -293,7 +361,7 @@ crmRoutes.delete('/contacts/:id', requireTeamRole('admin'), crmRateLimit, async /** A contact's analytics, if and only if an active signed consent record authorizes the link. When * it does not, the response says so explicitly rather than returning zeroes that read like "this * person did nothing" — `linked: false` and a reason are the honest answer. */ -crmRoutes.get('/contacts/:id/analytics', requireTeamRole('analyst'), crmRateLimit, async (c) => { +crmRoutes.get('/contacts/:id/analytics', crmGate('analyst', 'contact.analytics'), async (c) => { const siteId = c.get('siteId'); const contact = await loadContact(c.env, siteId, c.req.param('id')); if (!contact.external_user_id) { @@ -320,7 +388,7 @@ crmRoutes.get('/contacts/:id/analytics', requireTeamRole('analyst'), crmRateLimi * (their claims are a derived hash, a tier and a window), so including them adds cryptographic * evidence of what was consented to without widening what the export reveals. */ -crmRoutes.get('/contacts/:id/export', requireTeamRole('admin'), crmRateLimit, async (c) => { +crmRoutes.get('/contacts/:id/export', crmGate('admin', 'contact.export'), async (c) => { const siteId = c.get('siteId'); const contact = await loadContact(c.env, siteId, c.req.param('id')); const externalUserId = contact.external_user_id; @@ -377,8 +445,7 @@ function companyConflict(err: unknown): never { crmRoutes.get( '/companies', - requireTeamRole('analyst'), - crmRateLimit, + crmGate('analyst', 'company.list'), vValidator('query', CompanyListQuerySchema, validationErrorHook), async (c) => { const query = c.req.valid('query'); @@ -394,8 +461,7 @@ crmRoutes.get( crmRoutes.post( '/companies', - requireTeamRole('analyst'), - crmRateLimit, + crmGate('analyst', 'company.create'), vValidator('json', CompanyCreateSchema, validationErrorHook), async (c) => { const body = c.req.valid('json'); @@ -411,15 +477,14 @@ crmRoutes.post( }, ); -crmRoutes.get('/companies/:id', requireTeamRole('analyst'), crmRateLimit, async (c) => { +crmRoutes.get('/companies/:id', crmGate('analyst', 'company.read'), async (c) => { const company = await loadCompany(c.env, c.get('siteId'), c.req.param('id')); return c.json({ company }); }); crmRoutes.patch( '/companies/:id', - requireTeamRole('analyst'), - crmRateLimit, + crmGate('analyst', 'company.update'), vValidator('json', CompanyUpdateSchema, validationErrorHook), async (c) => { const body = c.req.valid('json'); @@ -447,7 +512,7 @@ crmRoutes.patch( * `admin` rather than `analyst` because it is irreversible and it rewrites rows the caller did not * name — the same reason deleting a contact is. */ -crmRoutes.delete('/companies/:id', requireTeamRole('admin'), crmRateLimit, async (c) => { +crmRoutes.delete('/companies/:id', crmGate('admin', 'company.delete'), async (c) => { const result = await deleteCompany(requireCrmDb(c.env), c.get('siteId'), c.req.param('id')); if (!result) { throw new ApiError('not_found', 404); @@ -457,8 +522,7 @@ crmRoutes.delete('/companies/:id', requireTeamRole('admin'), crmRateLimit, async crmRoutes.get( '/companies/:id/contacts', - requireTeamRole('analyst'), - crmRateLimit, + crmGate('analyst', 'company.contacts'), vValidator('query', CompanyContactsQuerySchema, validationErrorHook), async (c) => { const siteId = c.get('siteId'); @@ -495,7 +559,7 @@ crmRoutes.get( * start reasoning about the eleven who never consented. So `contacts_total` and `contacts_linked` are * reported side by side and one-of-twelve is visible as one-of-twelve. */ -crmRoutes.get('/companies/:id/analytics', requireTeamRole('analyst'), crmRateLimit, async (c) => { +crmRoutes.get('/companies/:id/analytics', crmGate('analyst', 'company.analytics'), async (c) => { const siteId = c.get('siteId'); const company = await loadCompany(c.env, siteId, c.req.param('id')); const linkage = await companyContactLinkage( @@ -543,3 +607,37 @@ crmRoutes.get('/companies/:id/analytics', requireTeamRole('analyst'), crmRateLim activity: await contactActivity(c.env, siteId, hashes), }); }); + +/** + * Read this site's audit log: who touched the contact store, what they touched, and when. + * + * `admin` rather than `analyst`, and not for the usual reason. Nothing in an entry is contact PII — + * every field is an id, a role, an action name or a timestamp — so this is not the bulk-disclosure + * argument that gates the export. It is that the entries are about the deployment's own OPERATORS: a + * log of what each colleague read is oversight in an administrator's hands and surveillance in a + * peer's, and the person answerable for how contact data is used is the one who should hold it. + * + * The filters are the three questions worth asking of an access log — what happened to this record, + * what did this operator do, who does this kind of thing — and each is an exact match, because a log + * of ids has no fragments to search for. + * + * There is no way to write here. No route updates or deletes an entry, deleting a contact leaves its + * entries standing, and only the retention cron removes anything: a log the people it names can edit + * records nothing. Reading it is itself recorded, by the same gate as every other route. + */ +crmRoutes.get( + '/audit', + crmGate('admin', 'audit.read'), + vValidator('query', CrmAuditListQuerySchema, validationErrorHook), + async (c) => { + const query = c.req.valid('query'); + const { entries, total } = await listCrmAudit(requireCrmDb(c.env), c.get('siteId'), { + action: query.action, + actorUserId: query.actor_user_id, + targetId: query.target_id, + limit: query.limit ?? CRM_DEFAULT_PAGE, + offset: query.offset ?? 0, + }); + return c.json({ entries, total, role: c.get('role') }); + }, +); diff --git a/apps/server/test/crm-audit.test.ts b/apps/server/test/crm-audit.test.ts new file mode 100644 index 0000000..95f21bf --- /dev/null +++ b/apps/server/test/crm-audit.test.ts @@ -0,0 +1,488 @@ +// The CRM audit log. What matters here is not that entries can be written and listed, but the four +// properties that make the log worth having: that EVERY route writes one, that it is written before +// the handler so nothing can happen unrecorded, that a request which was never authorized writes +// nothing, and that deleting the contact does not delete the evidence. +// +// The route-coverage test is deliberately table-driven over `CRM_AUDIT_ACTIONS`: a new CRM route that +// is given a role but no audited action cannot compile, and an action added to the set with no route +// behind it fails here. + +import { env } from 'cloudflare:test'; +import { CRM_AUDIT_ACTIONS } from '@facet/shared'; +import { beforeEach, describe, expect, it } from 'vitest'; +import { createApp } from '../src/app.js'; +import { + SESSION_COOKIE, + signSession, + upsertUserByEmail, + userMemberships, +} from '../src/lib/accounts.js'; +import { issueKey } from '../src/lib/apikeys.js'; +import { enforceCrmAuditRetention } from '../src/lib/retention.js'; + +const SITE = '77777777-7777-4777-8777-777777777777'; +const OTHER_SITE = '88888888-8888-4888-8888-888888888888'; +const DAY = 86_400_000; +const app = createApp(); + +type TestEnv = typeof env; + +interface AuditRow { + id: string; + site_id: string; + actor_user_id: string; + actor_role: string; + action: string; + target_id: string | null; + occurred_at: number; +} + +/** An env with the CRM binding removed — what a deployment that never created the database is. */ +function unbound(e: TestEnv): TestEnv { + const { CRM_DB: Omitted, ...rest } = e; + return rest as unknown as TestEnv; +} + +async function seedSite(e: TestEnv): Promise { + await e.DB.prepare( + 'INSERT OR IGNORE INTO sites (id, name, domain, created_at) VALUES (?, ?, ?, ?)', + ) + .bind(SITE, 'Test', 'shop.example.com', Date.now()) + .run(); +} + +/** Create an operator with `role` on the team owning SITE; returns their id and session cookie. */ +async function operator( + e: TestEnv, + email: string, + role: string, +): Promise<{ id: string; cookie: string }> { + const now = Date.now(); + const user = await upsertUserByEmail(e, email, now); + const teamId = (await userMemberships(e, user.id))[0]?.teamId as string; + await e.DB.prepare('UPDATE memberships SET role = ? WHERE team_id = ? AND user_id = ?') + .bind(role, teamId, user.id) + .run(); + await e.DB.prepare('UPDATE sites SET team_id = ? WHERE id = ?').bind(teamId, SITE).run(); + const secret = e.SESSION_SECRET as string; + return { id: user.id, cookie: `${SESSION_COOKIE}=${await signSession(user.id, secret, now)}` }; +} + +function crm(e: TestEnv, path: string, init: RequestInit = {}, cookie?: string) { + const sep = path.includes('?') ? '&' : '?'; + return app.request( + `/api/crm${path}${sep}site_id=${SITE}`, + { + ...init, + headers: { + 'content-type': 'application/json', + ...(cookie ? { cookie } : {}), + ...(init.headers ?? {}), + }, + }, + e, + ); +} + +/** The raw log, oldest first. Read straight from D1 rather than through the API, so the assertions + * do not depend on the very route they are checking. */ +async function log(e: TestEnv): Promise { + const { results } = await (e.CRM_DB as D1Database) + .prepare('SELECT * FROM crm_audit_log ORDER BY occurred_at, rowid') + .all(); + return results ?? []; +} + +beforeEach(async () => { + await seedSite(env); +}); + +describe('every route records the access before it performs it', () => { + it('writes one entry per authorized request, naming the action and the record', async () => { + const { id: userId, cookie } = await operator(env, 'admin@example.com', 'admin'); + + const created = await crm( + env, + '/contacts', + { method: 'POST', body: JSON.stringify({ name: 'Ada', external_user_id: 'ada-uid' }) }, + cookie, + ); + expect(created.status).toBe(201); + const contactId = ((await created.json()) as { contact: { id: string } }).contact.id; + + const madeCompany = await crm( + env, + '/companies', + { method: 'POST', body: JSON.stringify({ name: 'Acme' }) }, + cookie, + ); + expect(madeCompany.status).toBe(201); + const companyId = ((await madeCompany.json()) as { company: { id: string } }).company.id; + + // The rest of the surface, in one pass. `contact.create` and `company.create` are already done + // above because the ids they mint are what the other routes address. + const rest: [string, RequestInit, string, string | null][] = [ + ['/contacts', {}, 'contact.list', null], + [`/contacts/${contactId}`, {}, 'contact.read', contactId], + [ + `/contacts/${contactId}`, + { method: 'PATCH', body: JSON.stringify({ title: 'CTO' }) }, + 'contact.update', + contactId, + ], + [`/contacts/${contactId}/analytics`, {}, 'contact.analytics', contactId], + [`/contacts/${contactId}/export`, {}, 'contact.export', contactId], + ['/companies', {}, 'company.list', null], + [`/companies/${companyId}`, {}, 'company.read', companyId], + [ + `/companies/${companyId}`, + { method: 'PATCH', body: JSON.stringify({ status: 'active' }) }, + 'company.update', + companyId, + ], + [`/companies/${companyId}/contacts`, {}, 'company.contacts', companyId], + [`/companies/${companyId}/analytics`, {}, 'company.analytics', companyId], + ['/audit', {}, 'audit.read', null], + [`/contacts/${contactId}`, { method: 'DELETE' }, 'contact.delete', contactId], + [`/companies/${companyId}`, { method: 'DELETE' }, 'company.delete', companyId], + ]; + for (const [path, init, ,] of rest) { + expect((await crm(env, path, init, cookie)).status).toBe(200); + } + + const entries = await log(env); + expect(entries.map((e) => e.action)).toEqual([ + 'contact.create', + 'company.create', + ...rest.map(([, , action]) => action), + ]); + expect(entries.map((e) => e.target_id)).toEqual([ + null, + null, + ...rest.map(([, , , target]) => target), + ]); + // Every entry attributes the request to the session that made it, under the role it was + // authorized with — not the role that user holds when the log is read. + expect(entries.every((e) => e.actor_user_id === userId && e.actor_role === 'admin')).toBe( + true, + ); + expect(entries.every((e) => e.site_id === SITE)).toBe(true); + }); + + it('records every action the vocabulary declares, and declares every action it records', async () => { + // The set is closed so the log can be filtered by equality; a route with no action, or an + // action with no route, makes it a set of names that means nothing. + const { cookie } = await operator(env, 'admin@example.com', 'admin'); + const contact = await crm( + env, + '/contacts', + { method: 'POST', body: JSON.stringify({ name: 'Ada' }) }, + cookie, + ); + const contactId = ((await contact.json()) as { contact: { id: string } }).contact.id; + const company = await crm( + env, + '/companies', + { method: 'POST', body: JSON.stringify({ name: 'Acme' }) }, + cookie, + ); + const companyId = ((await company.json()) as { company: { id: string } }).company.id; + for (const path of [ + '/contacts', + `/contacts/${contactId}`, + `/contacts/${contactId}/analytics`, + `/contacts/${contactId}/export`, + '/companies', + `/companies/${companyId}`, + `/companies/${companyId}/contacts`, + `/companies/${companyId}/analytics`, + '/audit', + ]) { + await crm(env, path, {}, cookie); + } + await crm(env, `/contacts/${contactId}`, { method: 'PATCH', body: '{}' }, cookie); + await crm(env, `/companies/${companyId}`, { method: 'PATCH', body: '{}' }, cookie); + await crm(env, `/contacts/${contactId}`, { method: 'DELETE' }, cookie); + await crm(env, `/companies/${companyId}`, { method: 'DELETE' }, cookie); + + const recorded = new Set((await log(env)).map((e) => e.action)); + expect([...recorded].sort()).toEqual([...CRM_AUDIT_ACTIONS].sort()); + }); + + it('records a read that then finds nothing, because probing ids is what a log should show', async () => { + const { cookie } = await operator(env, 'admin@example.com', 'admin'); + const missing = '00000000-0000-4000-8000-000000000000'; + expect((await crm(env, `/contacts/${missing}`, {}, cookie)).status).toBe(404); + const entries = await log(env); + expect(entries).toHaveLength(1); + expect(entries[0]?.action).toBe('contact.read'); + expect(entries[0]?.target_id).toBe(missing); + }); +}); + +describe('an unauthorized request is not an access', () => { + it('records nothing for a caller with no session, the wrong role, or an API key', async () => { + const { cookie } = await operator(env, 'viewer@example.com', 'viewer'); + expect((await crm(env, '/contacts')).status).toBe(401); + expect((await crm(env, '/contacts', {}, cookie)).status).toBe(403); + + // The load-bearing one: a clk_ key is handed out on purpose, and it must not be able to write + // into the log any more than it can read a contact. + const { key } = await issueKey(env, SITE, null, Date.now()); + for (const path of ['/contacts', '/audit']) { + const withKey = await crm(env, path, { headers: { Authorization: `Bearer ${key}` } }); + expect(withKey.status, path).toBe(401); + } + + expect(await log(env)).toHaveLength(0); + }); + + it('records nothing for a request the rate limiter shed', async () => { + // Otherwise one stolen session could fill the audit table with entries for requests it was + // never allowed to make — the log's own denial of service. + const denying = { + ...env, + RATE_LIMITER: { limit: async () => ({ success: false }) }, + } as TestEnv; + const { cookie } = await operator(denying, 'admin@example.com', 'admin'); + expect((await crm(denying, '/contacts', {}, cookie)).status).toBe(429); + expect(await log(denying)).toHaveLength(0); + }); + + it('answers 501 without a CRM database, having nowhere to record anything', async () => { + const e = unbound(env); + const { cookie } = await operator(env, 'admin@example.com', 'admin'); + expect((await crm(e, '/audit', {}, cookie)).status).toBe(501); + }); +}); + +describe('the log cannot be skipped', () => { + /** Break the audit table specifically, leaving the contact store intact. This is what a D1 failure + * on the log write looks like from the route's side. */ + async function breakTheLog(e: TestEnv): Promise { + await (e.CRM_DB as D1Database).exec('DROP TABLE crm_audit_log'); + } + + async function contactCount(e: TestEnv): Promise { + const row = await (e.CRM_DB as D1Database) + .prepare('SELECT count(*) AS n FROM contacts') + .first<{ n: number }>(); + return row?.n ?? 0; + } + + it('fails a read closed rather than disclosing what it could not record', async () => { + const { cookie } = await operator(env, 'admin@example.com', 'admin'); + const created = await crm( + env, + '/contacts', + { method: 'POST', body: JSON.stringify({ name: 'Ada', email: 'ada@example.com' }) }, + cookie, + ); + const contactId = ((await created.json()) as { contact: { id: string } }).contact.id; + await breakTheLog(env); + + const res = await crm(env, `/contacts/${contactId}`, {}, cookie); + expect(res.status).toBe(500); + // The name and email never left the Worker. + expect(await res.text()).not.toContain('ada@example.com'); + }); + + it('fails a delete closed, leaving the contact for a retry that can be recorded', async () => { + const { cookie } = await operator(env, 'admin@example.com', 'admin'); + const created = await crm( + env, + '/contacts', + { method: 'POST', body: JSON.stringify({ name: 'Ada' }) }, + cookie, + ); + const contactId = ((await created.json()) as { contact: { id: string } }).contact.id; + await breakTheLog(env); + + expect( + (await crm(env, `/contacts/${contactId}`, { method: 'DELETE' }, cookie)).status, + ).toBe(500); + // An unrecorded deletion is the one outcome an audit log exists to prevent; the row is still + // there, so the operator can retry once the log is writable again. + expect(await contactCount(env)).toBe(1); + }); +}); + +describe('the log outlives what it describes', () => { + it("keeps a contact's entries after the contact is erased", async () => { + const { cookie } = await operator(env, 'admin@example.com', 'admin'); + const created = await crm( + env, + '/contacts', + { method: 'POST', body: JSON.stringify({ name: 'Ada', external_user_id: 'ada-uid' }) }, + cookie, + ); + const contactId = ((await created.json()) as { contact: { id: string } }).contact.id; + await crm(env, `/contacts/${contactId}/export`, {}, cookie); + expect( + (await crm(env, `/contacts/${contactId}`, { method: 'DELETE' }, cookie)).status, + ).toBe(200); + + // The entries name the contact by id and hold none of its fields, so what survives is a record + // of what operators did — not personal data the erasure should have reached. A log an operator + // can clear by deleting the row is not evidence of anything. + const entries = await log(env); + expect(entries.map((e) => e.action)).toEqual([ + 'contact.create', + 'contact.export', + 'contact.delete', + ]); + expect(entries.every((e) => e.target_id === null || e.target_id === contactId)).toBe(true); + const serialized = JSON.stringify(entries); + expect(serialized).not.toContain('Ada'); + expect(serialized).not.toContain('ada-uid'); + }); +}); + +describe('retention', () => { + /** Backdate every entry by `days`, standing in for a log that has been running that long. */ + async function ageLog(e: TestEnv, days: number): Promise { + await (e.CRM_DB as D1Database) + .prepare('UPDATE crm_audit_log SET occurred_at = occurred_at - ?') + .bind(days * DAY) + .run(); + } + + async function seedEntry(e: TestEnv): Promise { + const { cookie } = await operator(e, 'admin@example.com', 'admin'); + const created = await crm( + e, + '/contacts', + { method: 'POST', body: JSON.stringify({ name: 'Ada' }) }, + cookie, + ); + return ((await created.json()) as { contact: { id: string } }).contact.id; + } + + it('purges entries past the window and leaves the contacts they name alone', async () => { + await seedEntry(env); + await ageLog(env, 400); + expect(await enforceCrmAuditRetention(env, Date.now())).toBe(1); + expect(await log(env)).toHaveLength(0); + // Contacts are business records with their own lifecycle and are on NO schedule; only the log + // ages out. + const row = await (env.CRM_DB as D1Database) + .prepare('SELECT count(*) AS n FROM contacts') + .first<{ n: number }>(); + expect(row?.n).toBe(1); + }); + + it('keeps an entry inside the default window', async () => { + await seedEntry(env); + await ageLog(env, 300); + await enforceCrmAuditRetention(env, Date.now()); + expect(await log(env)).toHaveLength(1); + }); + + it('honours a configured window', async () => { + const e = { ...env, CRM_AUDIT_RETENTION_DAYS: '2' } as TestEnv; + await seedEntry(e); + await ageLog(e, 3); + await enforceCrmAuditRetention(e, Date.now()); + expect(await log(e)).toHaveLength(0); + }); + + it('falls back to the default rather than erasing the log on a bad value', async () => { + // A window of 0 or less puts the cutoff at or after now, so every run would wipe the entire + // log — including the entries written seconds earlier. + for (const bad of ['0', '-5', 'not-a-number', '']) { + const e = { ...env, CRM_AUDIT_RETENTION_DAYS: bad } as TestEnv; + await (e.CRM_DB as D1Database).exec('DELETE FROM crm_audit_log'); + await seedEntry(e); + await enforceCrmAuditRetention(e, Date.now()); + expect(await log(e), `window=${bad}`).toHaveLength(1); + } + }); + + it('does nothing on a deployment with no CRM database', async () => { + expect(await enforceCrmAuditRetention(unbound(env), Date.now())).toBe(0); + }); +}); + +describe('GET /api/crm/audit', () => { + it('is admin-only, because it reports on colleagues rather than on contacts', async () => { + const { cookie } = await operator(env, 'analyst@example.com', 'analyst'); + expect((await crm(env, '/audit', {}, cookie)).status).toBe(403); + // And the refusal wrote nothing: the gate records only what it authorized. + expect(await log(env)).toHaveLength(0); + }); + + it("returns one site's entries, newest first, filtered by action, actor and target", async () => { + const { id: userId, cookie } = await operator(env, 'admin@example.com', 'admin'); + const created = await crm( + env, + '/contacts', + { method: 'POST', body: JSON.stringify({ name: 'Ada' }) }, + cookie, + ); + const contactId = ((await created.json()) as { contact: { id: string } }).contact.id; + await crm(env, `/contacts/${contactId}`, {}, cookie); + // Another site's entry, written straight into the table: the list is scoped by the authorized + // site exactly as every other CRM read is. + await (env.CRM_DB as D1Database) + .prepare( + 'INSERT INTO crm_audit_log (id, site_id, actor_user_id, actor_role, action, target_id, occurred_at) VALUES (?, ?, ?, ?, ?, ?, ?)', + ) + .bind('other', OTHER_SITE, userId, 'admin', 'contact.read', contactId, Date.now()) + .run(); + + const res = await crm(env, '/audit', {}, cookie); + expect(res.status).toBe(200); + const body = (await res.json()) as { entries: AuditRow[]; total: number; role: string }; + expect(body.role).toBe('admin'); + // Newest first: this very request, then the read, then the create. + expect(body.entries.map((e) => e.action)).toEqual([ + 'audit.read', + 'contact.read', + 'contact.create', + ]); + expect(body.total).toBe(3); + expect(body.entries.every((e) => e.site_id === SITE)).toBe(true); + + const byAction = (await ( + await crm(env, '/audit?action=contact.read', {}, cookie) + ).json()) as { entries: AuditRow[]; total: number }; + expect(byAction.total).toBe(1); + expect(byAction.entries[0]?.target_id).toBe(contactId); + + const byTarget = (await ( + await crm(env, `/audit?target_id=${contactId}`, {}, cookie) + ).json()) as { total: number }; + expect(byTarget.total).toBe(1); + + const byActor = (await ( + await crm(env, `/audit?actor_user_id=${userId}`, {}, cookie) + ).json()) as { total: number }; + // Every entry on this site is this operator's; the other site's is still excluded. + expect(byActor.total).toBe(6); + + const noneSuch = (await ( + await crm(env, '/audit?actor_user_id=nobody', {}, cookie) + ).json()) as { total: number; entries: AuditRow[] }; + expect(noneSuch.total).toBe(0); + expect(noneSuch.entries).toEqual([]); + }); + + it('rejects an action outside the closed set', async () => { + const { cookie } = await operator(env, 'admin@example.com', 'admin'); + expect((await crm(env, '/audit?action=contact.everything', {}, cookie)).status).toBe(400); + }); + + it('pages within the same bounds as every other CRM list', async () => { + const { cookie } = await operator(env, 'admin@example.com', 'admin'); + for (let i = 0; i < 3; i++) { + await crm(env, '/contacts', {}, cookie); + } + const page = (await (await crm(env, '/audit?limit=2', {}, cookie)).json()) as { + entries: AuditRow[]; + total: number; + }; + expect(page.entries).toHaveLength(2); + expect(page.total).toBeGreaterThan(2); + expect((await crm(env, '/audit?limit=101', {}, cookie)).status).toBe(400); + }); +}); diff --git a/apps/server/test/dpv.test.ts b/apps/server/test/dpv.test.ts index 2ab4974..423b8d5 100644 --- a/apps/server/test/dpv.test.ts +++ b/apps/server/test/dpv.test.ts @@ -28,6 +28,11 @@ describe('analytics-only deployment', () => { expect(claims['dpv:hasPurpose']).toBe('dpv:ServiceOptimisation'); expect(claims['dpv:hasLegalBasis']).toBe('dpv:LegitimateInterest'); expect(claims['dpv:hasTechnicalOrganisationalMeasure']).toEqual(['dpv:Pseudonymisation']); + // Nothing here is read one record at a time by a named operator, so there is no access log and + // no claim of one. + expect(claims['dpv:hasTechnicalOrganisationalMeasure']).not.toContain( + 'dpv:ActivityMonitoring', + ); }); it('names no stored personal data and no data subject, because it holds neither', () => { @@ -61,6 +66,9 @@ describe('CRM-enabled deployment', () => { expect(measures).toContain('dpv:Pseudonymisation'); expect(measures).toContain('dpv:AccessControlMethod'); expect(measures).not.toEqual(['dpv:Pseudonymisation']); + // The CRM audit log. Access control says who MAY read a contact; this says the deployment + // records who did, and it is claimed only where such a record exists to be claimed. + expect(measures).toContain('dpv:ActivityMonitoring'); }); it('names the personal data it holds and the subject it holds it about', () => { diff --git a/apps/server/wrangler.jsonc b/apps/server/wrangler.jsonc index 1263129..2fe84ae 100644 --- a/apps/server/wrangler.jsonc +++ b/apps/server/wrangler.jsonc @@ -49,6 +49,13 @@ // /.well-known/facet-privacy.json. Do not bind this and leave that // unchanged; the deployment would be signing a false statement. // + // It also starts an ACCESS LOG. Every authorized /api/crm request is + // written to crm_audit_log in this same database before its handler + // runs — reads included, since a delete leaves a hole you can see and a + // read leaves nothing. That is a third claim the attestation gains + // (dpv:ActivityMonitoring) and the one CRM table on a retention + // schedule: CRM_AUDIT_RETENTION_DAYS below, default 365. + // // To turn it on: // wrangler d1 create facet-crm // pnpm --filter @facet/server migrate:crm:remote @@ -63,6 +70,15 @@ "vars": { // Rolling retention window for raw events, in days. "RAW_RETENTION_DAYS": "90" + // Rolling retention window for the CRM access log, in days. Only read + // when CRM_DB is bound; unset means 365. Deliberately longer than the + // raw-event window — raw events are visitors' data and the short window + // IS the privacy measure, while these entries record what OPERATORS did + // with contact data, and an access log that expires before the misuse + // it evidences is noticed has protected nobody. A value below 1 falls + // back to the default rather than putting the cutoff at or after now + // and wiping the log on every run. + // ,"CRM_AUDIT_RETENTION_DAYS": "365" }, // Workers AI: translates natural-language analytics questions into a constrained query intent. "ai": { diff --git a/docs/api.md b/docs/api.md index da172d7..047b2e7 100644 --- a/docs/api.md +++ b/docs/api.md @@ -1324,7 +1324,9 @@ to turn it on, and note that doing so changes the DPV claims this deployment sig record carries a structured employer that a free-text box did not). Every route is rate limited per *operator* (not per site, so one compromised session cannot hide -inside its team's traffic) and write bodies are capped at 16 KB. +inside its team's traffic), write bodies are capped at 16 KB, and every authorized request — reads +included — is written to the [audit log](#get-apicrmauditsite_idactionactor_user_idtarget_idlimitoffset-session-admin) +before its handler runs. **Auth is a session cookie, never an API key.** This is the one authenticated surface that refuses `Authorization: Bearer `. A `clk_` key authorizes aggregate analytics and is meant to be @@ -1337,7 +1339,7 @@ that. Every route takes `?site_id=`, and the caller must hold a role on th | --- | --- | | `viewer` | no access at all | | `analyst` | list, read, create, update, view the analytics link | -| `admin` / `owner` | the above, plus delete and export | +| `admin` / `owner` | the above, plus delete, export, and read the audit log | ### `GET /api/crm/contacts?site_id&status&q&limit&offset` (session, analyst) @@ -1482,6 +1484,46 @@ There is **no company export**. A data-subject export is per person by definitio would be a bulk PII dump with no data-protection meaning. Use `/companies/:id/contacts` and then the per-contact export, which accounts for each person separately. +### `GET /api/crm/audit?site_id&action&actor_user_id&target_id&limit&offset` (session, admin) + +The access log. **Every authorized request to any route above writes one entry, before its handler +runs** — reads included, which is the point: a delete leaves a hole you can see, a read leaves +nothing. An entry is `{ id, site_id, actor_user_id, actor_role, action, target_id, occurred_at }`, +and the response is `{ entries: [...], total, role }`, newest first. + +Written **first**, not last. Logging afterwards means any failure between the disclosure and the +record — a D1 error, a crash — leaves an access that happened and was never written down, and for a +delete there is then nothing left to notice the gap against. Writing first inverts that: if the log +write fails the request fails `500` and nothing was read or changed. The cost is that an entry states +an operator was *authorized* to do this to this id, not that it succeeded; a request that goes on to +`404` is recorded like any other. For the id-probing case that is the more useful reading anyway. + +`action` is one of `contact.list`, `contact.create`, `contact.read`, `contact.update`, +`contact.delete`, `contact.analytics`, `contact.export`, `company.list`, `company.create`, +`company.read`, `company.update`, `company.delete`, `company.contacts`, `company.analytics`, +`audit.read` — a closed set, so the log is filterable by equality. `target_id` is the contact or +company the request named, or `null` for a collection route and for a **create**, whose record does +not exist yet when the entry is written. `actor_role` is the role the request was +**authorized under**, stored rather than resolved later, so "an admin exported this" stays true after +they are demoted. All three filters are exact matches; a log of ids has no fragments to search for. + +`admin` rather than `analyst`, and not for the usual reason — no entry carries contact PII. It is +that entries are about the deployment's own *operators*: a log of what each colleague read is +oversight in an administrator's hands and surveillance in a peer's. Reading it is itself recorded. + +**Nothing can write here.** There is no update or delete route, and deleting a contact leaves its +entries standing — they name it by id and hold none of its fields, so once the row is gone the +pointer resolves to nothing and there is no personal data left for an erasure request to reach. A log +an operator can clear by deleting the contact is not evidence of anything. + +The log is the one CRM table on a retention schedule. `CRM_AUDIT_RETENTION_DAYS` (default **365**) +is enforced by the hourly cron; a value below `1` falls back to the default rather than putting the +cutoff at or after now and wiping the log on every run. It is deliberately longer than +`RAW_RETENTION_DAYS`: raw events are visitors' data and the short window *is* the privacy measure, +while these entries record what operators did with contact data, and an access log that expires +before the misuse it evidences is noticed has protected nobody. Contacts themselves remain on no +schedule at all. + --- ## `GET /api/health` diff --git a/packages/shared/src/crm.ts b/packages/shared/src/crm.ts index 6d53bb3..a07b5de 100644 --- a/packages/shared/src/crm.ts +++ b/packages/shared/src/crm.ts @@ -205,6 +205,48 @@ export const CompanyListQuerySchema = v.object({ /** Query for `GET /api/crm/companies/:id/contacts` — the same page bounds, no independent filter. */ export const CompanyContactsQuerySchema = v.object(pageBounds); +/** + * Every action the CRM audit log records — one per route, and the closed set is the point. + * + * A free-form action string would make the log filterable only by whatever spelling each handler + * happened to use, and "was this contact exported?" has to be answerable by equality rather than by + * guessing at synonyms. The names are `.` so a prefix filter reads as "everything + * anyone did to contacts". + * + * `audit.read` is in the set because reading the log is itself an access worth recording: it names + * which operator looked at which contact, so it is the one route whose readers a deployment most + * wants to know. + */ +export const CRM_AUDIT_ACTIONS = [ + 'contact.list', + 'contact.create', + 'contact.read', + 'contact.update', + 'contact.delete', + 'contact.analytics', + 'contact.export', + 'company.list', + 'company.create', + 'company.read', + 'company.update', + 'company.delete', + 'company.contacts', + 'company.analytics', + 'audit.read', +] as const; + +export const CrmAuditActionSchema = v.picklist(CRM_AUDIT_ACTIONS); + +/** Query for `GET /api/crm/audit`. Every filter is an exact match on a recorded column — there is no + * substring search, because the log holds ids and action names rather than anything a person would + * search for by fragment. */ +export const CrmAuditListQuerySchema = v.object({ + action: v.optional(CrmAuditActionSchema), + actor_user_id: optionalText(64), + target_id: optionalText(64), + ...pageBounds, +}); + export type ContactStatus = v.InferOutput; export type ContactCreateInput = v.InferOutput; export type ContactUpdateInput = v.InferOutput; @@ -213,3 +255,5 @@ export type CompanyStatus = v.InferOutput; export type CompanyCreateInput = v.InferOutput; export type CompanyUpdateInput = v.InferOutput; export type CompanyListQueryInput = v.InferOutput; +export type CrmAuditAction = v.InferOutput; +export type CrmAuditListQueryInput = v.InferOutput;