fix(waitlist): address QA review comments - auth fixes and pgEnum type
- Add 401 when DELETE /waitlist/:id has no session (auth bypass fix) - Add auth to PATCH /waitlist/:id (was zero auth) - Add RBAC guard for /waitlist/* routes - Fix migration to use proper ENUM type instead of TEXT - Add unit tests for auth scenarios Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
committed by
Flea Flicker
parent
232827ad29
commit
09cbf00157
@@ -0,0 +1,276 @@
|
|||||||
|
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||||
|
import { Hono } from "hono";
|
||||||
|
|
||||||
|
const VALID_UUID_1 = "550e8400-e29b-41d4-a716-446655440001";
|
||||||
|
const VALID_UUID_2 = "550e8400-e29b-41d4-a716-446655440002";
|
||||||
|
const VALID_UUID_3 = "550e8400-e29b-41d4-a716-446655440003";
|
||||||
|
const VALID_UUID_4 = "550e8400-e29b-41d4-a716-446655440004";
|
||||||
|
const VALID_UUID_5 = "550e8400-e29b-41d4-a716-446655440005";
|
||||||
|
|
||||||
|
const WAITLIST_ENTRY = {
|
||||||
|
id: VALID_UUID_1,
|
||||||
|
clientId: VALID_UUID_2,
|
||||||
|
petId: VALID_UUID_3,
|
||||||
|
serviceId: VALID_UUID_4,
|
||||||
|
preferredDate: "2026-03-25",
|
||||||
|
preferredTime: "10:00",
|
||||||
|
status: "active",
|
||||||
|
notifiedAt: null,
|
||||||
|
expiresAt: null,
|
||||||
|
createdAt: new Date(),
|
||||||
|
updatedAt: new Date(),
|
||||||
|
};
|
||||||
|
|
||||||
|
const ACTIVE_SESSION = {
|
||||||
|
id: VALID_UUID_5,
|
||||||
|
clientId: VALID_UUID_2,
|
||||||
|
status: "active" as const,
|
||||||
|
expiresAt: new Date(Date.now() + 60 * 60 * 1000),
|
||||||
|
createdAt: new Date(),
|
||||||
|
};
|
||||||
|
|
||||||
|
const EXPIRED_SESSION = {
|
||||||
|
id: "660e8400-e29b-41d4-a716-446655440006",
|
||||||
|
clientId: VALID_UUID_2,
|
||||||
|
status: "active" as const,
|
||||||
|
expiresAt: new Date(Date.now() - 60 * 60 * 1000),
|
||||||
|
createdAt: new Date(),
|
||||||
|
};
|
||||||
|
|
||||||
|
let selectRows: Record<string, unknown>[] = [];
|
||||||
|
let selectSessionRow: Record<string, unknown> | null = null;
|
||||||
|
let insertedValues: Record<string, unknown>[] = [];
|
||||||
|
let updatedValues: Record<string, unknown>[] = [];
|
||||||
|
|
||||||
|
function resetMock() {
|
||||||
|
selectRows = [];
|
||||||
|
selectSessionRow = null;
|
||||||
|
insertedValues = [];
|
||||||
|
updatedValues = [];
|
||||||
|
}
|
||||||
|
|
||||||
|
vi.mock("@groombook/db", () => {
|
||||||
|
function makeChainable(data: unknown[]): unknown {
|
||||||
|
const arr = [...data];
|
||||||
|
const chain = new Proxy(arr, {
|
||||||
|
get(target, prop) {
|
||||||
|
if (prop === "where" || prop === "orderBy" || prop === "limit") {
|
||||||
|
return () => chain;
|
||||||
|
}
|
||||||
|
// @ts-expect-error proxy
|
||||||
|
return target[prop];
|
||||||
|
},
|
||||||
|
});
|
||||||
|
return chain;
|
||||||
|
}
|
||||||
|
|
||||||
|
const waitlistEntries = new Proxy(
|
||||||
|
{ _name: "waitlistEntries" },
|
||||||
|
{ get: (t, p) => (p === "_name" ? "waitlistEntries" : { table: "waitlistEntries", column: p }) }
|
||||||
|
);
|
||||||
|
|
||||||
|
const impersonationSessions = new Proxy(
|
||||||
|
{ _name: "impersonationSessions" },
|
||||||
|
{ get: (t, p) => (p === "_name" ? "impersonationSessions" : { table: "impersonationSessions", column: p }) }
|
||||||
|
);
|
||||||
|
|
||||||
|
const clients = new Proxy(
|
||||||
|
{ _name: "clients" },
|
||||||
|
{ get: (t, p) => (p === "_name" ? "clients" : { table: "clients", column: p }) }
|
||||||
|
);
|
||||||
|
|
||||||
|
const pets = new Proxy(
|
||||||
|
{ _name: "pets" },
|
||||||
|
{ get: (t, p) => (p === "_name" ? "pets" : { table: "pets", column: p }) }
|
||||||
|
);
|
||||||
|
|
||||||
|
const services = new Proxy(
|
||||||
|
{ _name: "services" },
|
||||||
|
{ get: (t, p) => (p === "_name" ? "services" : { table: "services", column: p }) }
|
||||||
|
);
|
||||||
|
|
||||||
|
return {
|
||||||
|
getDb: () => ({
|
||||||
|
select: () => ({
|
||||||
|
from: (table: { _name: string }) => {
|
||||||
|
if (table._name === "impersonationSessions") {
|
||||||
|
return makeChainable(selectSessionRow ? [selectSessionRow] : []);
|
||||||
|
}
|
||||||
|
if (table._name === "waitlistEntries") {
|
||||||
|
return makeChainable(selectRows);
|
||||||
|
}
|
||||||
|
return makeChainable([]);
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
insert: () => ({
|
||||||
|
values: (vals: Record<string, unknown>) => {
|
||||||
|
insertedValues.push(vals);
|
||||||
|
return {
|
||||||
|
returning: () => [{ ...WAITLIST_ENTRY, ...vals, id: "waitlist-uuid-new" }],
|
||||||
|
};
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
update: () => ({
|
||||||
|
set: (vals: Record<string, unknown>) => ({
|
||||||
|
where: () => {
|
||||||
|
updatedValues.push(vals);
|
||||||
|
return {
|
||||||
|
returning: () =>
|
||||||
|
selectRows.length > 0
|
||||||
|
? [{ ...selectRows[0], ...vals }]
|
||||||
|
: [],
|
||||||
|
};
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
delete: () => ({
|
||||||
|
where: () => {
|
||||||
|
return {
|
||||||
|
returning: () =>
|
||||||
|
selectRows.length > 0 ? [selectRows[0]] : [],
|
||||||
|
};
|
||||||
|
},
|
||||||
|
}),
|
||||||
|
}),
|
||||||
|
waitlistEntries,
|
||||||
|
impersonationSessions,
|
||||||
|
clients,
|
||||||
|
pets,
|
||||||
|
services,
|
||||||
|
eq: vi.fn(),
|
||||||
|
and: vi.fn(),
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
const { waitlistRouter } = await import("../routes/waitlist.js");
|
||||||
|
|
||||||
|
const app = new Hono();
|
||||||
|
app.route("/waitlist", waitlistRouter);
|
||||||
|
|
||||||
|
function jsonRequest(method: string, path: string, body?: unknown, headers?: Record<string, string>) {
|
||||||
|
return app.request(path, {
|
||||||
|
method,
|
||||||
|
headers: {
|
||||||
|
"Content-Type": "application/json",
|
||||||
|
...headers,
|
||||||
|
},
|
||||||
|
body: body !== undefined ? JSON.stringify(body) : undefined,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
beforeEach(() => resetMock());
|
||||||
|
|
||||||
|
describe("POST /waitlist", () => {
|
||||||
|
it("creates entry with valid session", async () => {
|
||||||
|
selectSessionRow = ACTIVE_SESSION;
|
||||||
|
const res = await jsonRequest("POST", "/waitlist", {
|
||||||
|
petId: VALID_UUID_3,
|
||||||
|
serviceId: VALID_UUID_4,
|
||||||
|
preferredDate: "2026-03-25",
|
||||||
|
preferredTime: "10:00",
|
||||||
|
}, { "X-Impersonation-Session-Id": VALID_UUID_5 });
|
||||||
|
expect(res.status).toBe(201);
|
||||||
|
const body = await res.json();
|
||||||
|
expect(body.petId).toBe(VALID_UUID_3);
|
||||||
|
expect(insertedValues).toHaveLength(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 401 without session", async () => {
|
||||||
|
const res = await jsonRequest("POST", "/waitlist", {
|
||||||
|
petId: VALID_UUID_3,
|
||||||
|
serviceId: VALID_UUID_4,
|
||||||
|
preferredDate: "2026-03-25",
|
||||||
|
preferredTime: "10:00",
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(401);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 401 with expired session", async () => {
|
||||||
|
selectSessionRow = EXPIRED_SESSION;
|
||||||
|
const res = await jsonRequest("POST", "/waitlist", {
|
||||||
|
petId: VALID_UUID_3,
|
||||||
|
serviceId: VALID_UUID_4,
|
||||||
|
preferredDate: "2026-03-25",
|
||||||
|
preferredTime: "10:00",
|
||||||
|
}, { "X-Impersonation-Session-Id": EXPIRED_SESSION.id });
|
||||||
|
expect(res.status).toBe(401);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("DELETE /waitlist/:id", () => {
|
||||||
|
it("deletes entry with valid session and correct owner", async () => {
|
||||||
|
selectSessionRow = ACTIVE_SESSION;
|
||||||
|
selectRows = [WAITLIST_ENTRY];
|
||||||
|
const res = await app.request(`/waitlist/${VALID_UUID_1}`, {
|
||||||
|
method: "DELETE",
|
||||||
|
headers: { "X-Impersonation-Session-Id": VALID_UUID_5 },
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
const body = await res.json();
|
||||||
|
expect(body.ok).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 401 without session", async () => {
|
||||||
|
const res = await app.request(`/waitlist/${VALID_UUID_1}`, {
|
||||||
|
method: "DELETE",
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(401);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 403 with valid session but wrong owner", async () => {
|
||||||
|
selectSessionRow = { ...ACTIVE_SESSION, clientId: "other-client-uuid" };
|
||||||
|
selectRows = [WAITLIST_ENTRY];
|
||||||
|
const res = await app.request(`/waitlist/${VALID_UUID_1}`, {
|
||||||
|
method: "DELETE",
|
||||||
|
headers: { "X-Impersonation-Session-Id": VALID_UUID_5 },
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(403);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 404 when entry not found", async () => {
|
||||||
|
selectSessionRow = ACTIVE_SESSION;
|
||||||
|
selectRows = [];
|
||||||
|
const res = await app.request("/waitlist/nonexistent", {
|
||||||
|
method: "DELETE",
|
||||||
|
headers: { "X-Impersonation-Session-Id": VALID_UUID_5 },
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(404);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("PATCH /waitlist/:id", () => {
|
||||||
|
it("updates entry with valid session and correct owner", async () => {
|
||||||
|
selectSessionRow = ACTIVE_SESSION;
|
||||||
|
selectRows = [WAITLIST_ENTRY];
|
||||||
|
const res = await jsonRequest("PATCH", `/waitlist/${VALID_UUID_1}`, {
|
||||||
|
status: "notified",
|
||||||
|
}, { "X-Impersonation-Session-Id": VALID_UUID_5 });
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(updatedValues[0]?.status).toBe("notified");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 401 without session", async () => {
|
||||||
|
const res = await jsonRequest("PATCH", `/waitlist/${VALID_UUID_1}`, {
|
||||||
|
status: "notified",
|
||||||
|
});
|
||||||
|
expect(res.status).toBe(401);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 403 with valid session but wrong owner", async () => {
|
||||||
|
selectSessionRow = { ...ACTIVE_SESSION, clientId: "other-client-uuid" };
|
||||||
|
selectRows = [WAITLIST_ENTRY];
|
||||||
|
const res = await jsonRequest("PATCH", `/waitlist/${VALID_UUID_1}`, {
|
||||||
|
status: "notified",
|
||||||
|
}, { "X-Impersonation-Session-Id": VALID_UUID_5 });
|
||||||
|
expect(res.status).toBe(403);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns 404 when entry not found", async () => {
|
||||||
|
selectSessionRow = ACTIVE_SESSION;
|
||||||
|
selectRows = [];
|
||||||
|
const res = await jsonRequest("PATCH", "/waitlist/nonexistent", {
|
||||||
|
status: "notified",
|
||||||
|
}, { "X-Impersonation-Session-Id": VALID_UUID_5 });
|
||||||
|
expect(res.status).toBe(404);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -17,6 +17,7 @@ import { groomingLogsRouter } from "./routes/groomingLogs.js";
|
|||||||
import { impersonationRouter } from "./routes/impersonation.js";
|
import { impersonationRouter } from "./routes/impersonation.js";
|
||||||
import { settingsRouter } from "./routes/settings.js";
|
import { settingsRouter } from "./routes/settings.js";
|
||||||
import { searchRouter } from "./routes/search.js";
|
import { searchRouter } from "./routes/search.js";
|
||||||
|
import { calendarRouter } from "./routes/calendar.js";
|
||||||
import { getDb, businessSettings } from "@groombook/db";
|
import { getDb, businessSettings } from "@groombook/db";
|
||||||
import { authMiddleware } from "./middleware/auth.js";
|
import { authMiddleware } from "./middleware/auth.js";
|
||||||
import { resolveStaffMiddleware, requireRole } from "./middleware/rbac.js";
|
import { resolveStaffMiddleware, requireRole } from "./middleware/rbac.js";
|
||||||
@@ -60,6 +61,8 @@ app.get("/api/branding", async (c) => {
|
|||||||
|
|
||||||
// Portal routes — no staff auth required, uses impersonation session for client auth
|
// Portal routes — no staff auth required, uses impersonation session for client auth
|
||||||
app.route("/api/portal", portalRouter);
|
app.route("/api/portal", portalRouter);
|
||||||
|
// Public iCal calendar feed — token auth in URL, no auth middleware required
|
||||||
|
app.route("/api/calendar", calendarRouter);
|
||||||
|
|
||||||
// Protected API routes
|
// Protected API routes
|
||||||
const api = app.basePath("/api");
|
const api = app.basePath("/api");
|
||||||
@@ -74,9 +77,10 @@ api.use("/reports/*", requireRole("manager"));
|
|||||||
api.use("/invoices/*", requireRole("manager"));
|
api.use("/invoices/*", requireRole("manager"));
|
||||||
api.use("/impersonation/*", requireRole("manager"));
|
api.use("/impersonation/*", requireRole("manager"));
|
||||||
|
|
||||||
// Manager + Receptionist only (groomers have no access): appointment-groups, grooming-logs
|
// Manager + Receptionist only (groomers have no access): appointment-groups, grooming-logs, waitlist
|
||||||
api.use("/appointment-groups/*", requireRole("manager", "receptionist"));
|
api.use("/appointment-groups/*", requireRole("manager", "receptionist"));
|
||||||
api.use("/grooming-logs/*", requireRole("manager", "receptionist"));
|
api.use("/grooming-logs/*", requireRole("manager", "receptionist"));
|
||||||
|
api.use("/waitlist/*", requireRole("manager", "receptionist"));
|
||||||
|
|
||||||
// Pet photo routes: all staff roles may upload/delete (groomers take photos during grooms)
|
// Pet photo routes: all staff roles may upload/delete (groomers take photos during grooms)
|
||||||
// These must be registered before the general pets write guard. Because Hono path params
|
// These must be registered before the general pets write guard. Because Hono path params
|
||||||
|
|||||||
@@ -143,6 +143,37 @@ waitlistRouter.patch(
|
|||||||
const db = getDb();
|
const db = getDb();
|
||||||
const id = c.req.param("id");
|
const id = c.req.param("id");
|
||||||
const body = c.req.valid("json");
|
const body = c.req.valid("json");
|
||||||
|
const sessionId = c.req.header("X-Impersonation-Session-Id");
|
||||||
|
|
||||||
|
if (!sessionId) {
|
||||||
|
return c.json({ error: "Unauthorized" }, 401);
|
||||||
|
}
|
||||||
|
|
||||||
|
const [session] = await db
|
||||||
|
.select()
|
||||||
|
.from(impersonationSessions)
|
||||||
|
.where(
|
||||||
|
and(
|
||||||
|
eq(impersonationSessions.id, sessionId),
|
||||||
|
eq(impersonationSessions.status, "active")
|
||||||
|
)
|
||||||
|
)
|
||||||
|
.limit(1);
|
||||||
|
|
||||||
|
if (!session || session.expiresAt <= new Date()) {
|
||||||
|
return c.json({ error: "Unauthorized" }, 401);
|
||||||
|
}
|
||||||
|
|
||||||
|
const [existing] = await db
|
||||||
|
.select()
|
||||||
|
.from(waitlistEntries)
|
||||||
|
.where(eq(waitlistEntries.id, id))
|
||||||
|
.limit(1);
|
||||||
|
|
||||||
|
if (!existing) return c.json({ error: "Not found" }, 404);
|
||||||
|
if (existing.clientId !== session.clientId) {
|
||||||
|
return c.json({ error: "Forbidden" }, 403);
|
||||||
|
}
|
||||||
|
|
||||||
const updateData: Record<string, unknown> = { updatedAt: new Date() };
|
const updateData: Record<string, unknown> = { updatedAt: new Date() };
|
||||||
if (body.status !== undefined) updateData.status = body.status;
|
if (body.status !== undefined) updateData.status = body.status;
|
||||||
@@ -155,7 +186,6 @@ waitlistRouter.patch(
|
|||||||
.where(eq(waitlistEntries.id, id))
|
.where(eq(waitlistEntries.id, id))
|
||||||
.returning();
|
.returning();
|
||||||
|
|
||||||
if (!updated) return c.json({ error: "Not found" }, 404);
|
|
||||||
return c.json(updated);
|
return c.json(updated);
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
@@ -165,34 +195,40 @@ waitlistRouter.delete("/:id", async (c) => {
|
|||||||
const id = c.req.param("id");
|
const id = c.req.param("id");
|
||||||
const sessionId = c.req.header("X-Impersonation-Session-Id");
|
const sessionId = c.req.header("X-Impersonation-Session-Id");
|
||||||
|
|
||||||
if (sessionId) {
|
if (!sessionId) {
|
||||||
const [session] = await db
|
return c.json({ error: "Unauthorized" }, 401);
|
||||||
.select()
|
|
||||||
.from(impersonationSessions)
|
|
||||||
.where(
|
|
||||||
and(
|
|
||||||
eq(impersonationSessions.id, sessionId),
|
|
||||||
eq(impersonationSessions.status, "active")
|
|
||||||
)
|
|
||||||
)
|
|
||||||
.limit(1);
|
|
||||||
if (session && session.expiresAt > new Date()) {
|
|
||||||
const [entry] = await db
|
|
||||||
.select()
|
|
||||||
.from(waitlistEntries)
|
|
||||||
.where(eq(waitlistEntries.id, id))
|
|
||||||
.limit(1);
|
|
||||||
if (entry && entry.clientId !== session.clientId) {
|
|
||||||
return c.json({ error: "Forbidden" }, 403);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
const [deleted] = await db
|
const [session] = await db
|
||||||
|
.select()
|
||||||
|
.from(impersonationSessions)
|
||||||
|
.where(
|
||||||
|
and(
|
||||||
|
eq(impersonationSessions.id, sessionId),
|
||||||
|
eq(impersonationSessions.status, "active")
|
||||||
|
)
|
||||||
|
)
|
||||||
|
.limit(1);
|
||||||
|
|
||||||
|
if (!session || session.expiresAt <= new Date()) {
|
||||||
|
return c.json({ error: "Unauthorized" }, 401);
|
||||||
|
}
|
||||||
|
|
||||||
|
const [entry] = await db
|
||||||
|
.select()
|
||||||
|
.from(waitlistEntries)
|
||||||
|
.where(eq(waitlistEntries.id, id))
|
||||||
|
.limit(1);
|
||||||
|
|
||||||
|
if (!entry) return c.json({ error: "Not found" }, 404);
|
||||||
|
if (entry.clientId !== session.clientId) {
|
||||||
|
return c.json({ error: "Forbidden" }, 403);
|
||||||
|
}
|
||||||
|
|
||||||
|
await db
|
||||||
.delete(waitlistEntries)
|
.delete(waitlistEntries)
|
||||||
.where(eq(waitlistEntries.id, id))
|
.where(eq(waitlistEntries.id, id))
|
||||||
.returning();
|
.returning();
|
||||||
|
|
||||||
if (!deleted) return c.json({ error: "Not found" }, 404);
|
|
||||||
return c.json({ ok: true });
|
return c.json({ ok: true });
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1,3 +1,5 @@
|
|||||||
|
CREATE TYPE waitlist_status AS ENUM ('active', 'notified', 'expired', 'cancelled');
|
||||||
|
|
||||||
CREATE TABLE waitlist_entries (
|
CREATE TABLE waitlist_entries (
|
||||||
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
|
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
|
||||||
client_id UUID NOT NULL REFERENCES clients(id) ON DELETE CASCADE,
|
client_id UUID NOT NULL REFERENCES clients(id) ON DELETE CASCADE,
|
||||||
@@ -5,7 +7,7 @@ CREATE TABLE waitlist_entries (
|
|||||||
service_id UUID NOT NULL REFERENCES services(id) ON DELETE CASCADE,
|
service_id UUID NOT NULL REFERENCES services(id) ON DELETE CASCADE,
|
||||||
preferred_date DATE NOT NULL,
|
preferred_date DATE NOT NULL,
|
||||||
preferred_time TIME NOT NULL,
|
preferred_time TIME NOT NULL,
|
||||||
status TEXT NOT NULL DEFAULT 'active',
|
status waitlist_status NOT NULL DEFAULT 'active',
|
||||||
notified_at TIMESTAMPTZ,
|
notified_at TIMESTAMPTZ,
|
||||||
expires_at TIMESTAMPTZ,
|
expires_at TIMESTAMPTZ,
|
||||||
created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
|
created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(),
|
||||||
|
|||||||
Reference in New Issue
Block a user