diff --git a/UAT_PLAYBOOK.md b/UAT_PLAYBOOK.md index ec555be..b8949de 100644 --- a/UAT_PLAYBOOK.md +++ b/UAT_PLAYBOOK.md @@ -146,6 +146,7 @@ Expected: one row, `role = 'groomer'`. If zero rows return, the request hit the | TC-API-3.19a | Get pet profile summary — customer owner-bypass (GRO-2013) | Sign in as `uat-customer@groombook.dev`; `POST /api/portal/session-from-auth`; then `GET /api/pets/{ownPetId}/profile-summary` with header `X-Impersonation-Session-Id: {sessionId}` for either of the customer's seeded pets (`c0000001-0000-0000-0000-000000000002` UAT Pup Alpha, `c0000001-0000-0000-0000-000000000003` UAT Pup Beta) | 200 OK, aggregated profile returned (owner-bypass: customer with valid portal session for pet's clientId is allowed even though rbac.ts auto-provisions them as a `groomer` staff row with no appointment linkage) | | TC-API-3.19b | Get pet profile summary — customer cross-tenant blocked (GRO-2013) | Sign in as `uat-customer@groombook.dev`; reuse the customer's sessionId from TC-API-3.19a; `GET /api/pets/{otherClientPetId}/profile-summary` for a pet owned by a different client (`c0000002-...` or any non-customer pet) | 403 Forbidden (owner-bypass requires session.clientId === pet.clientId) | | TC-API-3.19c | Get pet profile summary — customer without portal session header | Same as TC-API-3.19a but omit the `X-Impersonation-Session-Id` header | 403 Forbidden (no owner-bypass without valid portal session) | +| TC-API-3.19d | Get pet profile summary — owner-bypass writes audit row (GRO-2063) | Same setup as TC-API-3.19a (sign in as `uat-customer@groombook.dev`, establish a portal session for the customer's own clientId, call `GET /api/pets/{ownPetId}/profile-summary` with `X-Impersonation-Session-Id: {sessionId}` and a 200 OK response). Then call `GET /api/impersonation/sessions/{sessionId}/audit-log` and confirm there is exactly one entry with `action === "read_profile_summary"`, `pageVisited` matching the profile-summary path, and `metadata` containing `petId` and `actorStaffId` for the customer. Repeat TC-API-3.19b (cross-tenant attempt) and confirm NO new `read_profile_summary` row was written for the cross-tenant attempt. | 200 OK on the profile-summary call AND an audit log entry is present with the correct shape (defense-in-depth audit row; bypass attempts against other clients must NOT log) | | TC-API-3.29 | Get pet profile summary — unknown UUID returns 404 (GRO-2014) | GET /api/pets/00000000-0000-0000-0000-000000000001/profile-summary while authenticated (any role) | 404 Not Found with body `{"error":"Not found"}` (was empty-body 500 in GRO-2014) | | TC-API-3.30 | Get pet profile summary — malformed UUID returns 404 (GRO-2014) | GET /api/pets/not-a-uuid/profile-summary while authenticated | 404 Not Found with body `{"error":"Not found"}` (was empty-body 500 in GRO-2014 — Postgres uuid cast failure) | | TC-API-3.31 | Get pet profile summary — never empty-body 500 (GRO-2014) | GET /api/pets/{anyId}/profile-summary across the test sweep | No response has status 500 with an empty body. Any 500 must include a JSON body `{"error":"Internal Server Error"}` | @@ -191,6 +192,33 @@ Expected: one row, `role = 'groomer'`. If zero rows return, the request hit the | TC-API-5.4 | Update service | PATCH /api/services/{id} with updated fields | 200 OK, service updated | | TC-API-5.5 | Delete service | DELETE /api/services/{id} | 200 OK, service deleted | +#### 4.5.1 Seed/Reset idempotency (GRO-2064) + +Services seeding is now keyed on the deterministic `services.id` (not `name`) and +the reset path now `TRUNCATE`s `services` alongside the other dynamic tables. +This means: + +- Running the seed Job twice in a row (no reset in between) converges to the + same catalogue — no `services_pkey` collision. +- A `pnpm reset` followed by `pnpm seed` (or a CronJob reset fire) leaves the + catalogue exactly matching `servicesDef` (10 rows, ids `b0000001-…-001` … + `…-00a`), regardless of any stale rows that were present beforehand. +- Mixed `seedKnownUsers` + full `seed()` invocations are safe — the + `demoSvcs` subset (Bath & Brush, Full Groom Small/Medium, Nail Trim) is + keyed on ids `…-001`, `…-002`, `…-003`, `…-005` and the upsert target + is `services.id`, so the same-id / different-name collision that broke + GRO-2033 (id `…-004` = "Nail Trim" vs servicesDef `…-004` = + "Full Groom — Large") cannot recur. + +**UAT regression** (verify after a new image is rolled out): + +| # | Scenario | Steps | Expected | +|---|----------|-------|----------| +| TC-SEED-1 | Reset → seed converges | `kubectl -n groombook exec deploy/api -- pnpm reset && pnpm seed` | Seed completes 1/1, `services` count = 10, all ids match `servicesDef` | +| TC-SEED-2 | Idempotent re-seed | Re-run `pnpm seed` without reset | Seed completes 1/1, no `services_pkey` errors, `services` count still 10 | +| TC-SEED-3 | Catalogue matches servicesDef | `psql -c "SELECT id, name FROM services ORDER BY id"` | Rows `…-001`…`…-00a` with names "Bath & Brush"…"Sanitary Trim" exactly as in `servicesDef` | +| TC-SEED-4 | Demo subset coexists | Run `seedKnownUsers` then full `seed` | No collision, demo subset (4 services) ends up with the same rows the full seed would write | + ### 4.6 Staff Management | # | Scenario | Steps | Expected | diff --git a/apps/api/src/db/seed.ts b/apps/api/src/db/seed.ts index 5b48dd6..bf57144 100644 --- a/apps/api/src/db/seed.ts +++ b/apps/api/src/db/seed.ts @@ -636,21 +636,28 @@ async function seedKnownUsers() { } } - // ── Services: idempotent upsert using name as unique key ───────────────────── - // UNIQUE constraint on services.name (migration 0020) must exist first. - // Uses b0000001-... IDs to match main seed servicesDef for same-named services. + // ── Services: idempotent upsert keyed on `id` ───────────────────────────── + // GRO-2064: previously keyed on `services.name` while writing a + // deterministic `id`. If a stale row existed with the same `id` but a + // different `name`, PostgreSQL raised `services_pkey` (id collision) + // before the name-targeted ON CONFLICT could fire. Switch the conflict + // target to `services.id` so deterministic ids always win; pair with + // `TRUNCATE services … CASCADE` above so each reset rebuilds the + // catalogue from `servicesDef` cleanly. GRO-2033 close-out. + // Id↔name map MUST stay in sync with `servicesDef` (the canonical source + // of truth in the main `seed()` function). const demoSvcs = [ { id: "b0000001-0000-0000-0000-000000000001", name: "Bath & Brush", description: "Full bath, blow-dry, brush out, and ear cleaning", basePriceCents: 4500, durationMinutes: 45 }, { id: "b0000001-0000-0000-0000-000000000002", name: "Full Groom — Small", description: "Complete grooming for dogs under 25 lbs", basePriceCents: 6500, durationMinutes: 60 }, { id: "b0000001-0000-0000-0000-000000000003", name: "Full Groom — Medium", description: "Complete grooming for dogs 25-50 lbs", basePriceCents: 8000, durationMinutes: 75 }, - { id: "b0000001-0000-0000-0000-000000000004", name: "Nail Trim", description: "Nail clipping and filing", basePriceCents: 1500, durationMinutes: 15 }, + { id: "b0000001-0000-0000-0000-000000000005", name: "Nail Trim", description: "Nail clipping and filing", basePriceCents: 1500, durationMinutes: 15 }, ]; for (const svc of demoSvcs) { await db.insert(schema.services) .values({ ...svc, active: true }) .onConflictDoUpdate({ - target: schema.services.name, - set: { description: svc.description, basePriceCents: svc.basePriceCents, durationMinutes: svc.durationMinutes, active: true }, + target: schema.services.id, + set: { name: svc.name, description: svc.description, basePriceCents: svc.basePriceCents, durationMinutes: svc.durationMinutes, active: true }, }); } console.log(`✓ Seeded ${demoSvcs.length} services`); @@ -757,7 +764,13 @@ async function seed() { ({ id: uuid(), name: `Bather ${i + 1}`, email: `bather${i + 1}@groombook.dev`, role: "groomer" as const, isSuperUser: false }) ); - await db.execute(sql`TRUNCATE impersonation_sessions, impersonation_audit_logs, appointments, invoices, invoice_line_items, invoice_tip_splits, grooming_visit_logs CASCADE`); + // GRO-2064: also TRUNCATE `services` so each reset rebuilds the catalogue + // from `servicesDef` (deterministic IDs + UNIQUE(name)). Stale service rows + // (e.g. a prior `seedKnownUsers` run that wrote a different `name` for the + // same `id`) would otherwise cause the deterministic upsert to PK-collide + // on `services.id` — see CTO review on infra PR #605 (rev #4230). TRUNCATE + // CASCADE handles appointments/invoices FKs to services.id. + await db.execute(sql`TRUNCATE services, impersonation_sessions, impersonation_audit_logs, appointments, invoices, invoice_line_items, invoice_tip_splits, grooming_visit_logs CASCADE`); const allStaff = [...managerStaff, ...receptionistStaff, ...groomers, ...bathers]; for (const s of allStaff) { @@ -828,9 +841,11 @@ async function seed() { } // ── Services ── - // Upsert services using name as unique key. With deterministic IDs in - // servicesDef and TRUNCATE clearing downstream tables first, this is - // idempotent: first run inserts, subsequent runs update existing rows. + // GRO-2064: key the upsert on `services.id` (not `name`) so deterministic + // ids always win, and rely on the TRUNCATE above to clear stale rows before + // the catalogue is rebuilt. The previous name-targeted upsert failed with + // `services_pkey` when a prior run had left a row with the same id but a + // different name (CTO review on infra PR #605, rev #4230). const serviceIds: string[] = []; for (const s of servicesDef) { serviceIds.push(s.id); @@ -844,8 +859,8 @@ async function seed() { active: true, }) .onConflictDoUpdate({ - target: schema.services.name, - set: { description: s.desc, basePriceCents: s.price, durationMinutes: s.dur, active: true }, + target: schema.services.id, + set: { name: s.name, description: s.desc, basePriceCents: s.price, durationMinutes: s.dur, active: true }, }); } console.log(`✓ Created ${servicesDef.length} services`); diff --git a/packages/db/src/seed.ts b/packages/db/src/seed.ts index cf65909..c647098 100644 --- a/packages/db/src/seed.ts +++ b/packages/db/src/seed.ts @@ -747,21 +747,28 @@ async function seedKnownUsers() { // and the full seed() UAT branch. await seedUatStaffAccounts(db); - // ── Services: idempotent upsert using name as unique key ───────────────────── - // UNIQUE constraint on services.name (migration 0020) must exist first. - // Uses b0000001-... IDs to match main seed servicesDef for same-named services. + // ── Services: idempotent upsert keyed on `id` ───────────────────────────── + // GRO-2064: previously keyed on `services.name` while writing a + // deterministic `id`. If a stale row existed with the same `id` but a + // different `name`, PostgreSQL raised `services_pkey` (id collision) + // before the name-targeted ON CONFLICT could fire. Switch the conflict + // target to `services.id` so deterministic ids always win; pair with + // `TRUNCATE services … CASCADE` above so each reset rebuilds the + // catalogue from `servicesDef` cleanly. GRO-2033 close-out. + // Id↔name map MUST stay in sync with `servicesDef` (the canonical source + // of truth in the main `seed()` function). const demoSvcs = [ { id: "b0000001-0000-0000-0000-000000000001", name: "Bath & Brush", description: "Full bath, blow-dry, brush out, and ear cleaning", basePriceCents: 4500, durationMinutes: 45 }, { id: "b0000001-0000-0000-0000-000000000002", name: "Full Groom — Small", description: "Complete grooming for dogs under 25 lbs", basePriceCents: 6500, durationMinutes: 60 }, { id: "b0000001-0000-0000-0000-000000000003", name: "Full Groom — Medium", description: "Complete grooming for dogs 25-50 lbs", basePriceCents: 8000, durationMinutes: 75 }, - { id: "b0000001-0000-0000-0000-000000000004", name: "Nail Trim", description: "Nail clipping and filing", basePriceCents: 1500, durationMinutes: 15 }, + { id: "b0000001-0000-0000-0000-000000000005", name: "Nail Trim", description: "Nail clipping and filing", basePriceCents: 1500, durationMinutes: 15 }, ]; for (const svc of demoSvcs) { await db.insert(schema.services) .values({ ...svc, active: true }) .onConflictDoUpdate({ - target: schema.services.name, - set: { description: svc.description, basePriceCents: svc.basePriceCents, durationMinutes: svc.durationMinutes, active: true }, + target: schema.services.id, + set: { name: svc.name, description: svc.description, basePriceCents: svc.basePriceCents, durationMinutes: svc.durationMinutes, active: true }, }); } console.log(`✓ Seeded ${demoSvcs.length} services`); @@ -868,7 +875,13 @@ async function seed() { ({ id: uuid(), name: `Bather ${i + 1}`, email: `bather${i + 1}@groombook.dev`, role: "groomer" as const, isSuperUser: false }) ); - await db.execute(sql`TRUNCATE impersonation_sessions, impersonation_audit_logs, appointments, invoices, invoice_line_items, invoice_tip_splits, grooming_visit_logs CASCADE`); + // GRO-2064: also TRUNCATE `services` so each reset rebuilds the catalogue + // from `servicesDef` (deterministic IDs + UNIQUE(name)). Stale service rows + // (e.g. a prior `seedKnownUsers` run that wrote a different `name` for the + // same `id`) would otherwise cause the deterministic upsert to PK-collide + // on `services.id` — see CTO review on infra PR #605 (rev #4230). TRUNCATE + // CASCADE handles appointments/invoices FKs to services.id. + await db.execute(sql`TRUNCATE services, impersonation_sessions, impersonation_audit_logs, appointments, invoices, invoice_line_items, invoice_tip_splits, grooming_visit_logs CASCADE`); const allStaff = [...managerStaff, ...receptionistStaff, ...groomers, ...bathers]; for (const s of allStaff) { @@ -919,9 +932,11 @@ async function seed() { await seedUatStaffAccounts(db); // ── Services ── - // Upsert services using name as unique key. With deterministic IDs in - // servicesDef and TRUNCATE clearing downstream tables first, this is - // idempotent: first run inserts, subsequent runs update existing rows. + // GRO-2064: key the upsert on `services.id` (not `name`) so deterministic + // ids always win, and rely on the TRUNCATE above to clear stale rows before + // the catalogue is rebuilt. The previous name-targeted upsert failed with + // `services_pkey` when a prior run had left a row with the same id but a + // different name (CTO review on infra PR #605, rev #4230). const serviceIds: string[] = []; for (const s of servicesDef) { serviceIds.push(s.id); @@ -935,8 +950,8 @@ async function seed() { active: true, }) .onConflictDoUpdate({ - target: schema.services.name, - set: { description: s.desc, basePriceCents: s.price, durationMinutes: s.dur, active: true }, + target: schema.services.id, + set: { name: s.name, description: s.desc, basePriceCents: s.price, durationMinutes: s.dur, active: true }, }); } console.log(`✓ Created ${servicesDef.length} services`); diff --git a/src/__tests__/petProfileSummary.test.ts b/src/__tests__/petProfileSummary.test.ts index 069d1cb..d18d2da 100644 --- a/src/__tests__/petProfileSummary.test.ts +++ b/src/__tests__/petProfileSummary.test.ts @@ -182,6 +182,11 @@ let selectQueue: Array<{ throw?: string; }> = []; +// Captured `db.insert(table).values(vals)` calls. Mirrors the pattern from +// src/__tests__/impersonation.test.ts so the GRO-2063 audit row assertions +// can inspect what the route tried to write without needing a real DB. +let insertCapture: Array<{ table: string; vals: Record }> = []; + function enqueue(table: string, rows: unknown[] = []) { selectQueue.push({ table, rows }); } @@ -196,6 +201,7 @@ function resetMock() { servicesTable = [makeService()]; sessionsTable = [makeSession()]; selectQueue = []; + insertCapture = []; } // ─── Module mocks ─────────────────────────────────────────────────────────── @@ -269,7 +275,12 @@ vi.mock("@groombook/db", () => { select: (_cols?: Record) => ({ from: (table: { _name?: string }) => wrapRows(takeQueuedRows(table._name ?? "")), }), - insert: () => ({ values: () => ({ returning: () => [{}] }) }), + insert: (table: { _name?: string }) => ({ + values: (vals: Record) => { + insertCapture.push({ table: table._name ?? "unknown", vals }); + return { returning: () => [{}] }; + }, + }), update: () => ({ set: () => ({ where: () => ({ returning: () => [{}] }) }) }), delete: () => ({ where: () => ({ returning: () => [{}] }) }), }), @@ -278,6 +289,7 @@ vi.mock("@groombook/db", () => { staff: makeTable("staff"), services: makeTable("services"), impersonationSessions: makeTable("impersonationSessions"), + impersonationAuditLogs: makeTable("impersonation_audit_logs"), and: vi.fn((..._args: unknown[]) => ({})), desc: vi.fn((c: unknown) => c), eq: vi.fn((_a: unknown, _b: unknown) => ({})), @@ -567,3 +579,69 @@ describe("GET /:id/profile-summary — owner-bypass (GRO-2013)", () => { expect(res.status).toBe(403); }); }); + +// ─── GRO-2063 owner-bypass audit write ────────────────────────────────────── + +describe("GET /:id/profile-summary — owner-bypass audit row (GRO-2063)", () => { + it("writes exactly one audit row on the owner-bypass success path", async () => { + enqueue("pets", petsTable); + enqueue("impersonationSessions", sessionsTable); // valid active session for CLIENT_ID + enqueue("appointments", appointmentsTable); + enqueue("appointments", [{ count: 1 }]); + enqueue("appointments", []); + + const app = buildApp(CUSTOMER_STAFF); + const res = await app.request(`/pets/${PET_ID}/profile-summary`, { + headers: { "X-Impersonation-Session-Id": "sess-owner" }, + }); + expect(res.status).toBe(200); + + const auditInserts = insertCapture.filter((c) => c.table === "impersonation_audit_logs"); + expect(auditInserts).toHaveLength(1); + const vals = auditInserts[0]!.vals; + expect(vals.action).toBe("read_profile_summary"); + expect(vals.sessionId).toBe("sess-owner"); + expect(vals.pageVisited).toBe(`/pets/${PET_ID}/profile-summary`); + expect(vals.metadata).toEqual({ + petId: PET_ID, + actorStaffId: CUSTOMER_STAFF.id, + }); + }); + + it("does NOT write an audit row on the normal groomer-linkage success path", async () => { + // GROOMER is a "real" groomer with appointment linkage, NOT the + // auto-provisioned customer-as-groomer. No impersonation header is + // present, so the owner-bypass branch never executes. + enqueue("pets", petsTable); + enqueue("appointments", [{ id: "appt-1" }]); // linkage found + enqueue("appointments", appointmentsTable); + enqueue("appointments", [{ count: 1 }]); + enqueue("appointments", []); + + const app = buildApp(GROOMER); + const res = await app.request(`/pets/${PET_ID}/profile-summary`); + expect(res.status).toBe(200); + + const auditInserts = insertCapture.filter((c) => c.table === "impersonation_audit_logs"); + expect(auditInserts).toHaveLength(0); + }); + + it("does NOT write an audit row when the owner-bypass attempt is denied (cross-tenant)", async () => { + // Customer has a valid session but it points at a different client. + // isOwner=false, falls through to groomer linkage check, returns 403. + enqueue("pets", [ + makePet({ id: OTHER_CLIENT_PET_ID, clientId: "c0000002-0000-0000-0000-000000000002" }), + ]); + enqueue("impersonationSessions", sessionsTable); // session is for CLIENT_ID + enqueue("appointments", []); // no linkage → 403 + + const app = buildApp(CUSTOMER_STAFF); + const res = await app.request(`/pets/${OTHER_CLIENT_PET_ID}/profile-summary`, { + headers: { "X-Impersonation-Session-Id": "sess-owner" }, + }); + expect(res.status).toBe(403); + + const auditInserts = insertCapture.filter((c) => c.table === "impersonation_audit_logs"); + expect(auditInserts).toHaveLength(0); + }); +}); diff --git a/src/routes/pets.ts b/src/routes/pets.ts index 45dcde2..5c4aaec 100644 --- a/src/routes/pets.ts +++ b/src/routes/pets.ts @@ -7,6 +7,7 @@ import { eq, exists, getDb, + impersonationAuditLogs, impersonationSessions, or, pets, @@ -156,6 +157,40 @@ async function resolveImpersonationClientId( return session.clientId; } +/** + * Defense-in-depth audit write for the staff-side owner-bypass path in + * GET /pets/:id/profile-summary. Mirrors the failure-isolation pattern in + * src/middleware/portalAudit.ts: errors are logged but never thrown, so a + * misbehaving audit insert cannot turn a working read into a 500. + * + * Called only when the owner-bypass actually fires (i.e. the requester is a + * groomer-role staff row with no appointment linkage, but supplies a valid + * X-Impersonation-Session-Id whose clientId matches the pet's owner). The + * `petId` and `actorStaffId` are written inside `metadata` because the + * impersonation_audit_logs schema has no first-class columns for them and + * adding a migration is out of scope. + */ +async function writeOwnerBypassAudit( + db: ReturnType, + args: { + sessionId: string; + petId: string; + actorStaffId: string; + pageVisited: string; + } +): Promise { + try { + await db.insert(impersonationAuditLogs).values({ + sessionId: args.sessionId, + action: "read_profile_summary", + pageVisited: args.pageVisited, + metadata: { petId: args.petId, actorStaffId: args.actorStaffId }, + }); + } catch (err) { + console.error("[pets] failed to write owner-bypass audit log:", err); + } +} + petsRouter.get("/:id/profile-summary", async (c) => { const db = getDb(); const petId = c.req.param("id"); @@ -188,8 +223,22 @@ petsRouter.get("/:id/profile-summary", async (c) => { // `groomer` staff row with no appointment linkage. let isOwner = false; if (isGroomer) { + const headerSessionId = c.req.header("X-Impersonation-Session-Id"); const ownerClientId = await resolveImpersonationClientId(db, c); isOwner = !!ownerClientId && ownerClientId === pet.clientId; + if (isOwner && headerSessionId) { + // GRO-2063: defense-in-depth audit row. Only fires when the bypass + // is actually granted; never on the normal groomer-linkage path, + // 403/404/401, or when the header is absent. Failure is swallowed + // (try/catch inside writeOwnerBypassAudit) so this can never turn a + // working read into a 500. + await writeOwnerBypassAudit(db, { + sessionId: headerSessionId, + petId: pet.id, + actorStaffId: staffRow.id, + pageVisited: c.req.path, + }); + } } // Groomer RBAC: check appointment linkage to this pet's client