From fa18c41677648450f3c026be52f02869fb125869 Mon Sep 17 00:00:00 2001 From: Flea Flicker Date: Sun, 5 Apr 2026 19:37:23 +0000 Subject: [PATCH] fix(api): exempt OOBE setup from staff middleware and auto-create staff (GRO-485) Exempt POST /api/setup from resolveStaffMiddleware so OOBE users (with no pre-existing staff record) can complete the out-of-box experience without getting blocked by the "no staff record found" 403 error. Changes: - rbac.ts: add /api/setup to path exemption alongside /api/auth/ - setup.ts POST /: add find-or-create logic that: - Looks up existing staff by userId from JWT - Auto-links legacy staff records by email if userId is null - Creates a new staff record if none exists (OOBE case) - Returns 400 if JWT has no email and no staff record found - setup.test.ts: add regression tests for all scenarios Fixes GRO-485 (OOBE regression introduced by GRO-480). Co-Authored-By: Paperclip --- apps/api/src/__tests__/setup.test.ts | 314 +++++++++++++++++++++++++-- apps/api/src/middleware/rbac.ts | 3 +- apps/api/src/routes/setup.ts | 74 ++++++- 3 files changed, 360 insertions(+), 31 deletions(-) diff --git a/apps/api/src/__tests__/setup.test.ts b/apps/api/src/__tests__/setup.test.ts index 5940976..095d791 100644 --- a/apps/api/src/__tests__/setup.test.ts +++ b/apps/api/src/__tests__/setup.test.ts @@ -13,8 +13,10 @@ interface MockStaff { // ─── Mock DB state ──────────────────────────────────────────────────────────── let dbStaffRows: MockStaff[] = []; +let dbBusinessSettingsRows: { id: string; businessName: string }[] = []; let dbAuthConfigRows: { id: string; enabled: boolean }[] = []; let insertedAuthConfig: Record[] = []; +let insertedStaff: Record[] = []; let encryptCalls: string[] = []; // Track env vars set per test @@ -22,8 +24,10 @@ const originalEnv = { ...process.env }; function resetMock() { dbStaffRows = []; + dbBusinessSettingsRows = []; dbAuthConfigRows = []; insertedAuthConfig = []; + insertedStaff = []; encryptCalls = []; } @@ -58,39 +62,173 @@ vi.mock("@groombook/db", () => { } ); - return { - getDb: () => ({ + const businessSettings = new Proxy( + { _name: "business_settings" }, + { + get(_target, prop) { + if (prop === "_name") return "business_settings"; + if (prop === "$inferSelect") return {}; + return { table: "business_settings", column: prop }; + }, + } + ); + + // Build a shared tx mock that operates on current-state snapshots + function makeTxMock() { + function getRowsForTable(table: unknown) { + if (table === authProviderConfig) return dbAuthConfigRows; + if (table === staff) return dbStaffRows; + if (table === businessSettings) return dbBusinessSettingsRows; + return []; + } + + return { select: () => ({ - from: (table: unknown) => ({ - where: () => ({ - limit: () => { - if (table === authProviderConfig) return dbAuthConfigRows; - if (table === staff) return dbStaffRows; - return []; + from: (table: unknown) => { + const rows = getRowsForTable(table); + const base = { + where: (cond?: unknown) => { + const filtered = cond ? rows.filter((r) => evaluateCond(cond, r)) : rows; + return { + limit: () => filtered, + for: () => ({ + limit: () => filtered, + [Symbol.iterator]: function* () { + for (const item of filtered) yield item; + }, + 0: filtered[0], + length: filtered.length, + }), + [Symbol.iterator]: function* () { + for (const item of filtered) yield item; + }, + 0: filtered[0], + length: filtered.length, + }; }, [Symbol.iterator]: function* () { - const rows = table === authProviderConfig ? dbAuthConfigRows : dbStaffRows; for (const item of rows) yield item; }, - 0: (table === authProviderConfig ? dbAuthConfigRows : dbStaffRows)[0], - length: (table === authProviderConfig ? dbAuthConfigRows : dbStaffRows).length, - }), - }), + 0: rows[0], + length: rows.length, + }; + // Some calls use .limit() directly on from() result (no where()) + (base as any).limit = () => rows; + return base; + }, }), insert: () => ({ values: (vals: Record) => { - const row = { ...vals, id: "new-id-1", createdAt: new Date(), updatedAt: new Date() }; - insertedAuthConfig.push(vals); + const row = { ...vals, id: "new-id-" + Math.random(), createdAt: new Date(), updatedAt: new Date() }; if (vals.providerId) { + insertedAuthConfig.push(vals); dbAuthConfigRows.push({ id: row.id as string, enabled: vals.enabled as boolean }); + } else if (vals.email) { + // staff insert + insertedStaff.push(vals); + dbStaffRows.push(row as MockStaff); + } else if (vals.businessName) { + dbBusinessSettingsRows.push(row as { id: string; businessName: string }); } return { returning: () => [row] }; }, }), + update: () => ({ + set: (vals: Record) => ({ + where: () => ({ + returning: () => { + const updated = { ...dbStaffRows[0], ...vals, updatedAt: new Date() }; + return [updated]; + }, + }), + }), + }), + }; + } + + return { + getDb: () => ({ + select: () => ({ + from: (table: unknown) => ({ + where: (cond?: unknown) => { + const rows = + table === authProviderConfig + ? dbAuthConfigRows + : table === staff + ? dbStaffRows + : table === businessSettings + ? dbBusinessSettingsRows + : []; + const filtered = cond ? rows.filter((r) => evaluateCond(cond, r)) : rows; + return { + limit: () => filtered, + for: () => ({ + limit: () => filtered, + [Symbol.iterator]: function* () { + for (const item of filtered) yield item; + }, + 0: filtered[0], + length: filtered.length, + }), + [Symbol.iterator]: function* () { + for (const item of filtered) yield item; + }, + 0: filtered[0], + length: filtered.length, + }; + }, + [Symbol.iterator]: function* () { + const rows = + table === authProviderConfig + ? dbAuthConfigRows + : table === staff + ? dbStaffRows + : table === businessSettings + ? dbBusinessSettingsRows + : []; + for (const item of rows) yield item; + }, + 0: + table === authProviderConfig + ? dbAuthConfigRows[0] + : table === staff + ? dbStaffRows[0] + : table === businessSettings + ? dbBusinessSettingsRows[0] + : undefined, + length: + table === authProviderConfig + ? dbAuthConfigRows.length + : table === staff + ? dbStaffRows.length + : table === businessSettings + ? dbBusinessSettingsRows.length + : 0, + }), + }), + insert: () => ({ + values: (vals: Record) => { + const row = { ...vals, id: "new-id-" + Math.random(), createdAt: new Date(), updatedAt: new Date() }; + if (vals.providerId) { + insertedAuthConfig.push(vals); + dbAuthConfigRows.push({ id: row.id as string, enabled: vals.enabled as boolean }); + } else if (vals.email) { + insertedStaff.push(vals); + dbStaffRows.push(row as MockStaff); + } else if (vals.businessName) { + dbBusinessSettingsRows.push(row as { id: string; businessName: string }); + } + return { returning: () => [row] }; + }, + }), + transaction: (cb: (tx: unknown) => Promise) => cb(makeTxMock()), }), authProviderConfig, staff, - eq: (_col: unknown, _val: unknown) => ({ col: _col, val: _val }), + businessSettings, + eq: (col: unknown, val: unknown) => ({ __type: "eq", col, val }), + and: (...conds: unknown[]) => ({ __type: "and", conds }), + isNull: (col: unknown) => ({ __type: "isNull", col }), encryptSecret: (val: string) => { encryptCalls.push(val); return `encrypted:${val}`; @@ -98,13 +236,42 @@ vi.mock("@groombook/db", () => { }; }); +// Helper to evaluate mock conditions against a row +function evaluateCond(cond: unknown, row: Record): boolean { + if (!cond || typeof cond !== "object") return true; + const c = cond as Record; + if (c.__type === "eq") { + const colObj = c.col as Record; + const colName = colObj.column as string; + return row[colName] === c.val; + } + if (c.__type === "and") { + return (c.conds as unknown[]).every((sub) => evaluateCond(sub, row)); + } + if (c.__type === "isNull") { + const colObj = c.col as Record; + const colName = colObj.column as string; + return row[colName] === null || row[colName] === undefined; + } + return true; +} + // ─── Build test app ─────────────────────────────────────────────────────────── -function makeApp(staff?: MockStaff | null) { +interface JwtPayload { + sub: string; + email?: string; + name?: string; +} + +function makeApp(staff?: MockStaff | null, jwtPayload?: JwtPayload | null) { const app = new Hono(); - // Inject optional staff context for authenticated routes + // Inject optional staff and jwtPayload context for authenticated routes app.use("/setup/*", async (c, next) => { + if (jwtPayload) { + (c as any).set("jwtPayload", jwtPayload); + } if (staff) { (c as any).set("staff", staff); } @@ -156,6 +323,22 @@ async function postAuthProviderTest(app: Hono, body: unknown) { return { status: res.status, body: parsed }; } +async function postSetup(app: Hono, body: unknown) { + const res = await app.request("/setup", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); + const text = await res.text(); + let parsed: ResponseBody; + try { + parsed = JSON.parse(text) as ResponseBody; + } catch { + parsed = { error: text }; + } + return { status: res.status, body: parsed }; +} + // ─── Tests ──────────────────────────────────────────────────────────────────── describe("GET /setup/status — OOBE bootstrap logic", () => { @@ -388,4 +571,99 @@ describe("POST /setup/auth-provider/test — OOBE test connection", () => { vi.restoreAllMocks(); }); +}); + +describe("POST /setup — OOBE regression (GRO-485)", () => { + beforeEach(() => { + resetMock(); + process.env = { ...originalEnv }; + clearAuthEnv(); + }); + + afterEach(() => { + process.env = { ...originalEnv }; + }); + + it("creates staff record during OOBE when no staff record exists for authenticated user", async () => { + // No staff rows — this is a fresh OOBE user + dbStaffRows = []; + dbBusinessSettingsRows = []; + + const jwtPayload = { sub: "user-123", email: "alice@example.com", name: "Alice" }; + const app = makeApp(null, jwtPayload); + + const { status, body } = await postSetup(app, { businessName: "Alice's Pet Grooming" }); + + expect(status).toBe(201); + expect(body.ok).toBe(true); + expect(body.staff).toBeDefined(); + expect(body.staff.isSuperUser).toBe(true); + expect(body.staff.email).toBe("alice@example.com"); + expect(body.staff.role).toBe("manager"); + // New staff record was created + expect(insertedStaff.length).toBe(1); + expect(insertedStaff[0]!.email).toBe("alice@example.com"); + expect(insertedStaff[0]!.userId).toBe("user-123"); + }); + + it("still works for user who already has a staff record", async () => { + // Staff record exists for this user + dbStaffRows = [{ id: "staff-existing", role: "groomer", isSuperUser: false }]; + dbBusinessSettingsRows = []; + + const jwtPayload = { sub: "user-123", email: "alice@example.com", name: "Alice" }; + // Inject the existing staff record into context + const app = makeApp({ id: "staff-existing", role: "groomer", isSuperUser: false }, jwtPayload); + + const { status, body } = await postSetup(app, { businessName: "Alice's Pet Grooming" }); + + expect(status).toBe(201); + expect(body.ok).toBe(true); + expect(body.staff.isSuperUser).toBe(true); + // No new staff was created (insertedStaff should be empty since staff was pre-existing) + }); + + it("auto-links staff by email if record exists with matching email but no userId", async () => { + // Staff record exists with matching email but no userId (legacy record) + dbStaffRows = [{ id: "staff-legacy", role: "manager", isSuperUser: false, email: "alice@example.com", userId: null }]; + dbBusinessSettingsRows = []; + + const jwtPayload = { sub: "user-123", email: "alice@example.com", name: "Alice" }; + // No staff injected into context — the handler must find it by email + const app = makeApp(null, jwtPayload); + + const { status, body } = await postSetup(app, { businessName: "Alice's Pet Grooming" }); + + expect(status).toBe(201); + expect(body.ok).toBe(true); + expect(body.staff.isSuperUser).toBe(true); + }); + + it("returns 400 if JWT has no email claim and no staff record exists", async () => { + dbStaffRows = []; + dbBusinessSettingsRows = []; + + // JWT with no email + const jwtPayload = { sub: "user-123" }; + const app = makeApp(null, jwtPayload); + + const { status, body } = await postSetup(app, { businessName: "Alice's Pet Grooming" }); + + expect(status).toBe(400); + expect(body.error).toMatch(/no email claim/i); + }); + + it("returns 409 if a super user already exists", async () => { + // Super user already exists + dbStaffRows = [{ id: "staff-super", role: "manager", isSuperUser: true }]; + dbBusinessSettingsRows = []; + + const jwtPayload = { sub: "user-456", email: "bob@example.com", name: "Bob" }; + const app = makeApp(null, jwtPayload); + + const { status, body } = await postSetup(app, { businessName: "Bob's Grooming" }); + + expect(status).toBe(409); + expect(body.error).toMatch(/already been completed/i); + }); }); \ No newline at end of file diff --git a/apps/api/src/middleware/rbac.ts b/apps/api/src/middleware/rbac.ts index d5e764e..b8473e8 100644 --- a/apps/api/src/middleware/rbac.ts +++ b/apps/api/src/middleware/rbac.ts @@ -23,7 +23,8 @@ export const resolveStaffMiddleware: MiddlewareHandler = async ( next ) => { // Better-Auth's own routes handle their own auth — skip staff resolution - if (c.req.path.startsWith("/api/auth/")) { + // OOBE setup routes also handle their own auth — staff record is created during setup + if (c.req.path.startsWith("/api/auth/") || c.req.path.startsWith("/api/setup")) { await next(); return; } diff --git a/apps/api/src/routes/setup.ts b/apps/api/src/routes/setup.ts index 2b94d20..b10cad7 100644 --- a/apps/api/src/routes/setup.ts +++ b/apps/api/src/routes/setup.ts @@ -1,7 +1,7 @@ import { Hono } from "hono"; import { zValidator } from "@hono/zod-validator"; import { z } from "zod/v3"; -import { eq, getDb, staff, businessSettings, authProviderConfig, encryptSecret } from "@groombook/db"; +import { and, eq, getDb, isNull, staff, businessSettings, authProviderConfig, encryptSecret } from "@groombook/db"; import type { AppEnv } from "../middleware/rbac.js"; export const setupRouter = new Hono(); @@ -44,20 +44,16 @@ const setupSchema = z.object({ businessName: z.string().min(1).max(200), }); -// POST /api/setup — authenticated, marks current staff as super user and sets business name +// POST /api/setup — authenticated (Better-Auth JWT), creates staff record if needed and sets business name +// This endpoint is exempt from resolveStaffMiddleware so that OOBE users (with no staff record yet) can complete setup setupRouter.post("/", zValidator("json", setupSchema), async (c) => { const db = getDb(); const body = c.req.valid("json"); - const currentStaff = c.get("staff"); + const jwt = c.get("jwtPayload"); + const currentStaff = c.get("staff"); // may be undefined during OOBE // Use a transaction with row-level locking to prevent race conditions const result = await db.transaction(async (tx) => { - // Lock the business_settings row for update to prevent concurrent setup - const [existingSettings] = await tx - .select({ id: businessSettings.id }) - .from(businessSettings) - .limit(1); - // Lock super user rows to prevent concurrent claims // FOR UPDATE serializes concurrent claims: second transaction blocks until first commits const [existingSuperUser] = await tx @@ -71,6 +67,12 @@ setupRouter.post("/", zValidator("json", setupSchema), async (c) => { return { error: "Setup has already been completed. A super user already exists.", code: 409 }; } + // Lock the business_settings row for update to prevent concurrent setup + const [existingSettings] = await tx + .select({ id: businessSettings.id }) + .from(businessSettings) + .limit(1); + // Update or create business settings with the business name if (existingSettings) { await tx @@ -81,18 +83,66 @@ setupRouter.post("/", zValidator("json", setupSchema), async (c) => { await tx.insert(businessSettings).values({ businessName: body.businessName }); } - // Mark the current staff as super user + // Find or create staff record for the authenticated user + let resolvedStaff = currentStaff; + + if (!resolvedStaff) { + // Try to find by userId + const [byUserId] = await tx + .select() + .from(staff) + .where(eq(staff.userId, jwt.sub)); + if (byUserId) { + resolvedStaff = byUserId; + } + } + + if (!resolvedStaff && jwt.email) { + // Try auto-link by email: staff record exists with matching email but no userId + const [byEmail] = await tx + .select() + .from(staff) + .where(and(eq(staff.email, jwt.email), isNull(staff.userId))); + if (byEmail) { + await tx + .update(staff) + .set({ userId: jwt.sub }) + .where(eq(staff.id, byEmail.id)); + resolvedStaff = { ...byEmail, userId: jwt.sub }; + } + } + + if (!resolvedStaff) { + // Brand new user during OOBE — create staff record + if (!jwt.email) { + return { error: "Cannot complete setup: authenticated user has no email claim", code: 400 }; + } + const [newStaff] = await tx + .insert(staff) + .values({ + name: jwt.name || jwt.email, + email: jwt.email, + userId: jwt.sub, + role: "manager", + isSuperUser: false, // will be set below + }) + .returning(); + resolvedStaff = newStaff; + } + + // Mark as super user const [updatedStaff] = await tx .update(staff) .set({ isSuperUser: true, updatedAt: new Date() }) - .where(eq(staff.id, currentStaff.id)) + .where(eq(staff.id, resolvedStaff.id)) .returning(); return { staff: updatedStaff }; }); if ("error" in result) { - return c.json({ error: result.error }, 409); + const status = (result as { code?: number }).code || 409; + return c.json({ error: result.error }, status as any); } return c.json({ ok: true, staff: result.staff }, 201);