From fbf8bc0e7827b1212f4c468c1eddddf8aeda44b4 Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Sun, 21 Jun 2026 19:24:59 -0500 Subject: [PATCH 1/4] feat(chats): account_id override for GET /api/chats/{id}/messages Parse an optional account_id (or camelCase accountId) query param in validateGetChatMessagesQuery, validate it as a UUID, and thread it into validateChatAccess via a new optional options arg. validateChatAccess forwards it to validateAuthContext(request, { accountId }) and resolves room access against the overridden account, so a caller with access to multiple accounts (org members / Recoup admins) can read another account's chat messages. A non-admin passing an inaccessible account is still rejected by canAccessAccount (403). The override is opt-in per call site: only validateGetChatMessagesQuery passes it, so the other validateChatAccess callers are unchanged. chat#1811 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../__tests__/validateChatAccess.test.ts | 28 ++++++++++ .../validateGetChatMessagesQuery.test.ts | 56 +++++++++++++++++++ lib/chats/validateChatAccess.ts | 13 ++++- lib/chats/validateGetChatMessagesQuery.ts | 23 +++++++- 4 files changed, 118 insertions(+), 2 deletions(-) diff --git a/lib/chats/__tests__/validateChatAccess.test.ts b/lib/chats/__tests__/validateChatAccess.test.ts index 4c9255446..a96ff2b82 100644 --- a/lib/chats/__tests__/validateChatAccess.test.ts +++ b/lib/chats/__tests__/validateChatAccess.test.ts @@ -159,4 +159,32 @@ describe("validateChatAccess", () => { const result = await validateChatAccess(request, roomId); expect(result).toEqual({ roomId, room, accountId }); }); + + it("passes the accountId override to validateAuthContext and resolves against it", async () => { + const overrideAccountId = "123e4567-e89b-12d3-a456-426614174999"; + const room = { + id: roomId, + account_id: overrideAccountId, + artist_id: null, + topic: "Topic", + updated_at: "2026-03-30T00:00:00Z", + }; + + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId: overrideAccountId, + orgId: null, + authToken: "test-key", + }); + vi.mocked(selectRoom).mockResolvedValue(room); + vi.mocked(buildGetChatsParams).mockResolvedValue({ + params: { account_ids: [overrideAccountId] }, + error: null, + }); + + const result = await validateChatAccess(request, roomId, { accountId: overrideAccountId }); + + expect(validateAuthContext).toHaveBeenCalledWith(request, { accountId: overrideAccountId }); + expect(buildGetChatsParams).toHaveBeenCalledWith({ account_id: overrideAccountId }); + expect(result).toEqual({ roomId, room, accountId: overrideAccountId }); + }); }); diff --git a/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts b/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts index ffdfbdd38..e22d226c6 100644 --- a/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts +++ b/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts @@ -49,4 +49,60 @@ describe("validateGetChatMessagesQuery", () => { expect(result).not.toBeInstanceOf(NextResponse); expect((result as { roomId: string }).roomId).toBe(roomId); }); + + describe("account_id query override", () => { + const roomId = "123e4567-e89b-42d3-a456-426614174000"; + const overrideAccountId = "223e4567-e89b-42d3-a456-426614174999"; + + const successMock = () => + vi.mocked(validateChatAccess).mockResolvedValue({ + roomId, + room: { + id: roomId, + account_id: overrideAccountId, + artist_id: null, + topic: null, + updated_at: null, + }, + accountId: overrideAccountId, + }); + + it("passes the account_id query override to validateChatAccess", async () => { + successMock(); + const request = new NextRequest( + `http://localhost/api/chats/${roomId}/messages?account_id=${overrideAccountId}`, + ); + + await validateGetChatMessagesQuery(request, roomId); + + expect(validateChatAccess).toHaveBeenCalledWith(request, roomId, { + accountId: overrideAccountId, + }); + }); + + it("accepts the camelCase accountId query alias", async () => { + successMock(); + const request = new NextRequest( + `http://localhost/api/chats/${roomId}/messages?accountId=${overrideAccountId}`, + ); + + await validateGetChatMessagesQuery(request, roomId); + + expect(validateChatAccess).toHaveBeenCalledWith(request, roomId, { + accountId: overrideAccountId, + }); + }); + + it("returns 400 when the account_id query is not a valid UUID", async () => { + const request = new NextRequest( + `http://localhost/api/chats/${roomId}/messages?account_id=not-a-uuid`, + ); + + const result = await validateGetChatMessagesQuery(request, roomId); + + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(400); + expect(validateChatAccess).not.toHaveBeenCalled(); + }); + }); }); diff --git a/lib/chats/validateChatAccess.ts b/lib/chats/validateChatAccess.ts index 30f89e5db..529b9e091 100644 --- a/lib/chats/validateChatAccess.ts +++ b/lib/chats/validateChatAccess.ts @@ -13,18 +13,29 @@ export interface ValidatedChatAccess { accountId: string; } +export interface ValidateChatAccessOptions { + /** Optional account_id override; authorized via validateAuthContext. */ + accountId?: string; +} + const chatIdSchema = z.string().uuid("id must be a valid UUID"); /** * Validates that the authenticated caller can access a chat room. * + * When `options.accountId` is provided, access is resolved against that account + * instead of the caller's own. The override is authorized by `validateAuthContext` + * (org members / Recoup admins), so a caller without access still gets a 403. + * * @param request - The incoming request (used for auth context) * @param roomId - The room/chat UUID to validate access for + * @param options - Optional account_id override * @returns NextResponse on auth/access failure, or validated access data */ export async function validateChatAccess( request: NextRequest, roomId: string, + options: ValidateChatAccessOptions = {}, ): Promise { const roomIdResult = chatIdSchema.safeParse(roomId); if (!roomIdResult.success) { @@ -34,7 +45,7 @@ export async function validateChatAccess( ); } - const authResult = await validateAuthContext(request); + const authResult = await validateAuthContext(request, { accountId: options.accountId }); if (authResult instanceof NextResponse) { return authResult; } diff --git a/lib/chats/validateGetChatMessagesQuery.ts b/lib/chats/validateGetChatMessagesQuery.ts index d28b42b3d..14f561275 100644 --- a/lib/chats/validateGetChatMessagesQuery.ts +++ b/lib/chats/validateGetChatMessagesQuery.ts @@ -5,10 +5,14 @@ import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { validateChatAccess, type ValidatedChatAccess } from "@/lib/chats/validateChatAccess"; const chatIdSchema = z.string().uuid("id must be a valid UUID"); +const accountIdSchema = z.string().uuid("account_id must be a valid UUID"); /** * Validates auth and params for GET /api/chats/[id]/messages. * + * Accepts an optional `account_id` (or camelCase `accountId`) query override so a + * caller with access to multiple accounts can read another account's messages. + * * @param request - Incoming request used to validate chat access. * @param id - Chat identifier from route params. * @returns NextResponse on failure, or validated chat access data. @@ -28,5 +32,22 @@ export async function validateGetChatMessagesQuery( ); } - return validateChatAccess(request, parsedId.data); + const { searchParams } = new URL(request.url); + const accountIdParam = + searchParams.get("account_id") ?? searchParams.get("accountId") ?? undefined; + + if (accountIdParam !== undefined) { + const parsedAccountId = accountIdSchema.safeParse(accountIdParam); + if (!parsedAccountId.success) { + return NextResponse.json( + { + status: "error", + error: parsedAccountId.error.issues[0]?.message || "Invalid account_id", + }, + { status: 400, headers: getCorsHeaders() }, + ); + } + } + + return validateChatAccess(request, parsedId.data, { accountId: accountIdParam }); } From 710c9bd34230c1b0b542d77b9e97f2fda909066e Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Tue, 23 Jun 2026 08:12:25 -0500 Subject: [PATCH 2/4] refactor(chats): admin bypass (not account_id param) for GET messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aligns GET /api/chats/{id}/messages with the shipped docs contract — docs#247 rolled back the account_id query param. The chat is identified by the path id and the owner is resolved server-side, so no param is needed. Instead, validateChatAccess gains an opt-in `allowAdmin` flag that grants RECOUP_ORG admins access to any room (mirrors checkAccountArtistAccess). Only the messages read path opts in; chat mutations (update/delete/copy) stay ownership-gated, so admin write access is not silently broadened. - drop account_id/accountId query parsing from validateGetChatMessagesQuery - validateChatAccess: remove accountId override; add allowAdmin + checkIsAdmin bypass - tests: admin bypass grants access; non-admin still 403 even with allowAdmin; mutation paths never consult admin status - mock checkIsAdmin in getChatArtistHandler.test.ts (now a transitive dep) Refs recoupable/chat#1811 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../__tests__/getChatArtistHandler.test.ts | 4 ++ .../__tests__/validateChatAccess.test.ts | 72 ++++++++++++++++--- .../validateGetChatMessagesQuery.test.ts | 58 +++------------ lib/chats/validateChatAccess.ts | 26 +++++-- lib/chats/validateGetChatMessagesQuery.ts | 25 ++----- 5 files changed, 98 insertions(+), 87 deletions(-) diff --git a/lib/chats/__tests__/getChatArtistHandler.test.ts b/lib/chats/__tests__/getChatArtistHandler.test.ts index c88ce650d..7bbf0d388 100644 --- a/lib/chats/__tests__/getChatArtistHandler.test.ts +++ b/lib/chats/__tests__/getChatArtistHandler.test.ts @@ -17,6 +17,10 @@ vi.mock("@/lib/chats/buildGetChatsParams", () => ({ buildGetChatsParams: vi.fn(), })); +vi.mock("@/lib/admins/checkIsAdmin", () => ({ + checkIsAdmin: vi.fn(() => Promise.resolve(false)), +})); + const createRequest = () => new NextRequest("http://localhost/api/chats/chat-id/artist"); describe("getChatArtistHandler", () => { diff --git a/lib/chats/__tests__/validateChatAccess.test.ts b/lib/chats/__tests__/validateChatAccess.test.ts index a96ff2b82..f93064986 100644 --- a/lib/chats/__tests__/validateChatAccess.test.ts +++ b/lib/chats/__tests__/validateChatAccess.test.ts @@ -4,6 +4,7 @@ import { validateChatAccess } from "../validateChatAccess"; import { validateAuthContext } from "@/lib/auth/validateAuthContext"; import selectRoom from "@/lib/supabase/rooms/selectRoom"; import { buildGetChatsParams } from "@/lib/chats/buildGetChatsParams"; +import { checkIsAdmin } from "@/lib/admins/checkIsAdmin"; vi.mock("@/lib/networking/getCorsHeaders", () => ({ getCorsHeaders: vi.fn(() => ({ "Access-Control-Allow-Origin": "*" })), @@ -21,6 +22,10 @@ vi.mock("@/lib/chats/buildGetChatsParams", () => ({ buildGetChatsParams: vi.fn(), })); +vi.mock("@/lib/admins/checkIsAdmin", () => ({ + checkIsAdmin: vi.fn(), +})); + describe("validateChatAccess", () => { const roomId = "123e4567-e89b-12d3-a456-426614174000"; const accountId = "123e4567-e89b-12d3-a456-426614174001"; @@ -30,6 +35,7 @@ describe("validateChatAccess", () => { beforeEach(() => { vi.clearAllMocks(); + vi.mocked(checkIsAdmin).mockResolvedValue(false); }); it("returns 400 when roomId is invalid uuid", async () => { @@ -160,31 +166,79 @@ describe("validateChatAccess", () => { expect(result).toEqual({ roomId, room, accountId }); }); - it("passes the accountId override to validateAuthContext and resolves against it", async () => { - const overrideAccountId = "123e4567-e89b-12d3-a456-426614174999"; + it("grants a Recoup admin access to any room when allowAdmin is set (bypasses ownership)", async () => { + const room = { + id: roomId, + account_id: "another-account", + artist_id: null, + topic: "Topic", + updated_at: "2026-03-30T00:00:00Z", + }; + + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId, + orgId: null, + authToken: "test-key", + }); + vi.mocked(selectRoom).mockResolvedValue(room); + vi.mocked(checkIsAdmin).mockResolvedValue(true); + + const result = await validateChatAccess(request, roomId, { allowAdmin: true }); + + expect(validateAuthContext).toHaveBeenCalledWith(request); + expect(checkIsAdmin).toHaveBeenCalledWith(accountId); + expect(buildGetChatsParams).not.toHaveBeenCalled(); + expect(result).toEqual({ roomId, room, accountId: "another-account" }); + }); + + it("still 403s a non-admin caller even when allowAdmin is set", async () => { + vi.mocked(validateAuthContext).mockResolvedValue({ + accountId, + orgId: null, + authToken: "test-key", + }); + vi.mocked(selectRoom).mockResolvedValue({ + id: roomId, + account_id: "another-account", + artist_id: null, + topic: "Topic", + updated_at: "2026-03-30T00:00:00Z", + }); + vi.mocked(checkIsAdmin).mockResolvedValue(false); + vi.mocked(buildGetChatsParams).mockResolvedValue({ + params: { account_ids: [accountId] }, + error: null, + }); + + const result = await validateChatAccess(request, roomId, { allowAdmin: true }); + + expect(result).toBeInstanceOf(NextResponse); + expect((result as NextResponse).status).toBe(403); + }); + + it("does not consult admin status when allowAdmin is not set (mutation paths stay ownership-gated)", async () => { const room = { id: roomId, - account_id: overrideAccountId, + account_id: accountId, artist_id: null, topic: "Topic", updated_at: "2026-03-30T00:00:00Z", }; vi.mocked(validateAuthContext).mockResolvedValue({ - accountId: overrideAccountId, + accountId, orgId: null, authToken: "test-key", }); vi.mocked(selectRoom).mockResolvedValue(room); vi.mocked(buildGetChatsParams).mockResolvedValue({ - params: { account_ids: [overrideAccountId] }, + params: { account_ids: [accountId] }, error: null, }); - const result = await validateChatAccess(request, roomId, { accountId: overrideAccountId }); + const result = await validateChatAccess(request, roomId); - expect(validateAuthContext).toHaveBeenCalledWith(request, { accountId: overrideAccountId }); - expect(buildGetChatsParams).toHaveBeenCalledWith({ account_id: overrideAccountId }); - expect(result).toEqual({ roomId, room, accountId: overrideAccountId }); + expect(checkIsAdmin).not.toHaveBeenCalled(); + expect(result).toEqual({ roomId, room, accountId }); }); }); diff --git a/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts b/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts index e22d226c6..7e1a85dd3 100644 --- a/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts +++ b/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts @@ -50,59 +50,17 @@ describe("validateGetChatMessagesQuery", () => { expect((result as { roomId: string }).roomId).toBe(roomId); }); - describe("account_id query override", () => { + it("opts into the admin bypass (allowAdmin: true) when delegating to validateChatAccess", async () => { const roomId = "123e4567-e89b-42d3-a456-426614174000"; - const overrideAccountId = "223e4567-e89b-42d3-a456-426614174999"; - - const successMock = () => - vi.mocked(validateChatAccess).mockResolvedValue({ - roomId, - room: { - id: roomId, - account_id: overrideAccountId, - artist_id: null, - topic: null, - updated_at: null, - }, - accountId: overrideAccountId, - }); - - it("passes the account_id query override to validateChatAccess", async () => { - successMock(); - const request = new NextRequest( - `http://localhost/api/chats/${roomId}/messages?account_id=${overrideAccountId}`, - ); - - await validateGetChatMessagesQuery(request, roomId); - - expect(validateChatAccess).toHaveBeenCalledWith(request, roomId, { - accountId: overrideAccountId, - }); - }); - - it("accepts the camelCase accountId query alias", async () => { - successMock(); - const request = new NextRequest( - `http://localhost/api/chats/${roomId}/messages?accountId=${overrideAccountId}`, - ); - - await validateGetChatMessagesQuery(request, roomId); - - expect(validateChatAccess).toHaveBeenCalledWith(request, roomId, { - accountId: overrideAccountId, - }); + vi.mocked(validateChatAccess).mockResolvedValue({ + roomId, + room: { id: roomId, account_id: null, artist_id: null, topic: null, updated_at: null }, + accountId: "acc-123", }); + const request = new NextRequest(`http://localhost/api/chats/${roomId}/messages`); - it("returns 400 when the account_id query is not a valid UUID", async () => { - const request = new NextRequest( - `http://localhost/api/chats/${roomId}/messages?account_id=not-a-uuid`, - ); - - const result = await validateGetChatMessagesQuery(request, roomId); + await validateGetChatMessagesQuery(request, roomId); - expect(result).toBeInstanceOf(NextResponse); - expect((result as NextResponse).status).toBe(400); - expect(validateChatAccess).not.toHaveBeenCalled(); - }); + expect(validateChatAccess).toHaveBeenCalledWith(request, roomId, { allowAdmin: true }); }); }); diff --git a/lib/chats/validateChatAccess.ts b/lib/chats/validateChatAccess.ts index 529b9e091..af0e68e5d 100644 --- a/lib/chats/validateChatAccess.ts +++ b/lib/chats/validateChatAccess.ts @@ -3,6 +3,7 @@ import { NextResponse } from "next/server"; import { z } from "zod"; import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { validateAuthContext } from "@/lib/auth/validateAuthContext"; +import { checkIsAdmin } from "@/lib/admins/checkIsAdmin"; import selectRoom from "@/lib/supabase/rooms/selectRoom"; import { buildGetChatsParams } from "@/lib/chats/buildGetChatsParams"; import type { Tables } from "@/types/database.types"; @@ -14,8 +15,13 @@ export interface ValidatedChatAccess { } export interface ValidateChatAccessOptions { - /** Optional account_id override; authorized via validateAuthContext. */ - accountId?: string; + /** + * Opt-in admin bypass. When true, a Recoup admin (member of `RECOUP_ORG_ID`) + * is granted access to any room without the ownership check — so an admin can + * read a customer's chat by id. Only read endpoints set this; chat mutations + * leave it off so admin write access stays gated by ownership. + */ + allowAdmin?: boolean; } const chatIdSchema = z.string().uuid("id must be a valid UUID"); @@ -23,13 +29,13 @@ const chatIdSchema = z.string().uuid("id must be a valid UUID"); /** * Validates that the authenticated caller can access a chat room. * - * When `options.accountId` is provided, access is resolved against that account - * instead of the caller's own. The override is authorized by `validateAuthContext` - * (org members / Recoup admins), so a caller without access still gets a 403. + * The room is fully identified by `roomId`, so no account override is accepted. + * When `options.allowAdmin` is set, a Recoup admin bypasses the ownership check + * (the room's owner is resolved server-side); everyone else must own the room. * * @param request - The incoming request (used for auth context) * @param roomId - The room/chat UUID to validate access for - * @param options - Optional account_id override + * @param options - Optional admin-bypass opt-in (read endpoints only) * @returns NextResponse on auth/access failure, or validated access data */ export async function validateChatAccess( @@ -45,7 +51,7 @@ export async function validateChatAccess( ); } - const authResult = await validateAuthContext(request, { accountId: options.accountId }); + const authResult = await validateAuthContext(request); if (authResult instanceof NextResponse) { return authResult; } @@ -60,6 +66,12 @@ export async function validateChatAccess( ); } + // Admin bypass (read endpoints only): a Recoup admin may access any room. + // The owner is resolved server-side, so no account_id input is needed. + if (options.allowAdmin && (await checkIsAdmin(accountId))) { + return { roomId: room.id, room, accountId: room.account_id ?? accountId }; + } + const { params, error } = await buildGetChatsParams({ account_id: accountId, }); diff --git a/lib/chats/validateGetChatMessagesQuery.ts b/lib/chats/validateGetChatMessagesQuery.ts index 14f561275..3b99ea8dd 100644 --- a/lib/chats/validateGetChatMessagesQuery.ts +++ b/lib/chats/validateGetChatMessagesQuery.ts @@ -5,13 +5,13 @@ import { getCorsHeaders } from "@/lib/networking/getCorsHeaders"; import { validateChatAccess, type ValidatedChatAccess } from "@/lib/chats/validateChatAccess"; const chatIdSchema = z.string().uuid("id must be a valid UUID"); -const accountIdSchema = z.string().uuid("account_id must be a valid UUID"); /** * Validates auth and params for GET /api/chats/[id]/messages. * - * Accepts an optional `account_id` (or camelCase `accountId`) query override so a - * caller with access to multiple accounts can read another account's messages. + * Reading is a resource-scoped operation: the chat is identified by the path id + * and the owner is resolved server-side, so no `account_id` input is accepted. + * Recoup admins are granted read access to any chat via the admin bypass. * * @param request - Incoming request used to validate chat access. * @param id - Chat identifier from route params. @@ -32,22 +32,5 @@ export async function validateGetChatMessagesQuery( ); } - const { searchParams } = new URL(request.url); - const accountIdParam = - searchParams.get("account_id") ?? searchParams.get("accountId") ?? undefined; - - if (accountIdParam !== undefined) { - const parsedAccountId = accountIdSchema.safeParse(accountIdParam); - if (!parsedAccountId.success) { - return NextResponse.json( - { - status: "error", - error: parsedAccountId.error.issues[0]?.message || "Invalid account_id", - }, - { status: 400, headers: getCorsHeaders() }, - ); - } - } - - return validateChatAccess(request, parsedId.data, { accountId: accountIdParam }); + return validateChatAccess(request, parsedId.data, { allowAdmin: true }); } From 1c48115b733028f447014d3adf41dd6bfdd8f674 Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Tue, 23 Jun 2026 08:38:38 -0500 Subject: [PATCH 3/4] =?UTF-8?q?refactor(chats):=20drop=20allowAdmin=20flag?= =?UTF-8?q?=20=E2=80=94=20admins=20access=20any=20chat=20(read=20+=20write?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit YAGNI/KISS per internal review: RECOUP_ORG admins already have broad cross-account power (delete any artist, read any account), and chat ops are resource-scoped by chatId, so an unconditional admin bypass is the coherent model. Removes the opt-in flag entirely. The admin check now runs ONLY after the ownership check fails, so the common owner path never pays the extra checkIsAdmin lookup (better than both the flag and a top-of-function bypass). Applies across all validateChatAccess call sites (messages + getChatArtist reads; update/delete-trailing/copy mutations), so admins can read and write any account's chats; non-admins are unchanged (403). Refs recoupable/chat#1811 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../__tests__/validateChatAccess.test.ts | 36 +++---------- .../validateGetChatMessagesQuery.test.ts | 4 +- lib/chats/validateChatAccess.ts | 50 ++++++------------- lib/chats/validateGetChatMessagesQuery.ts | 4 +- 4 files changed, 27 insertions(+), 67 deletions(-) diff --git a/lib/chats/__tests__/validateChatAccess.test.ts b/lib/chats/__tests__/validateChatAccess.test.ts index f93064986..cac17ccfa 100644 --- a/lib/chats/__tests__/validateChatAccess.test.ts +++ b/lib/chats/__tests__/validateChatAccess.test.ts @@ -166,7 +166,7 @@ describe("validateChatAccess", () => { expect(result).toEqual({ roomId, room, accountId }); }); - it("grants a Recoup admin access to any room when allowAdmin is set (bypasses ownership)", async () => { + it("grants a Recoup admin access to a room they don't own (admin checked after ownership fails)", async () => { const room = { id: roomId, account_id: "another-account", @@ -181,42 +181,20 @@ describe("validateChatAccess", () => { authToken: "test-key", }); vi.mocked(selectRoom).mockResolvedValue(room); - vi.mocked(checkIsAdmin).mockResolvedValue(true); - - const result = await validateChatAccess(request, roomId, { allowAdmin: true }); - - expect(validateAuthContext).toHaveBeenCalledWith(request); - expect(checkIsAdmin).toHaveBeenCalledWith(accountId); - expect(buildGetChatsParams).not.toHaveBeenCalled(); - expect(result).toEqual({ roomId, room, accountId: "another-account" }); - }); - - it("still 403s a non-admin caller even when allowAdmin is set", async () => { - vi.mocked(validateAuthContext).mockResolvedValue({ - accountId, - orgId: null, - authToken: "test-key", - }); - vi.mocked(selectRoom).mockResolvedValue({ - id: roomId, - account_id: "another-account", - artist_id: null, - topic: "Topic", - updated_at: "2026-03-30T00:00:00Z", - }); - vi.mocked(checkIsAdmin).mockResolvedValue(false); vi.mocked(buildGetChatsParams).mockResolvedValue({ params: { account_ids: [accountId] }, error: null, }); + vi.mocked(checkIsAdmin).mockResolvedValue(true); - const result = await validateChatAccess(request, roomId, { allowAdmin: true }); + const result = await validateChatAccess(request, roomId); - expect(result).toBeInstanceOf(NextResponse); - expect((result as NextResponse).status).toBe(403); + expect(buildGetChatsParams).toHaveBeenCalled(); + expect(checkIsAdmin).toHaveBeenCalledWith(accountId); + expect(result).toEqual({ roomId, room, accountId: "another-account" }); }); - it("does not consult admin status when allowAdmin is not set (mutation paths stay ownership-gated)", async () => { + it("does not consult admin status when the caller owns the room", async () => { const room = { id: roomId, account_id: accountId, diff --git a/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts b/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts index 7e1a85dd3..ebf4d5c16 100644 --- a/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts +++ b/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts @@ -50,7 +50,7 @@ describe("validateGetChatMessagesQuery", () => { expect((result as { roomId: string }).roomId).toBe(roomId); }); - it("opts into the admin bypass (allowAdmin: true) when delegating to validateChatAccess", async () => { + it("delegates to validateChatAccess with the parsed chat id", async () => { const roomId = "123e4567-e89b-42d3-a456-426614174000"; vi.mocked(validateChatAccess).mockResolvedValue({ roomId, @@ -61,6 +61,6 @@ describe("validateGetChatMessagesQuery", () => { await validateGetChatMessagesQuery(request, roomId); - expect(validateChatAccess).toHaveBeenCalledWith(request, roomId, { allowAdmin: true }); + expect(validateChatAccess).toHaveBeenCalledWith(request, roomId); }); }); diff --git a/lib/chats/validateChatAccess.ts b/lib/chats/validateChatAccess.ts index af0e68e5d..de663feaf 100644 --- a/lib/chats/validateChatAccess.ts +++ b/lib/chats/validateChatAccess.ts @@ -14,34 +14,23 @@ export interface ValidatedChatAccess { accountId: string; } -export interface ValidateChatAccessOptions { - /** - * Opt-in admin bypass. When true, a Recoup admin (member of `RECOUP_ORG_ID`) - * is granted access to any room without the ownership check — so an admin can - * read a customer's chat by id. Only read endpoints set this; chat mutations - * leave it off so admin write access stays gated by ownership. - */ - allowAdmin?: boolean; -} - const chatIdSchema = z.string().uuid("id must be a valid UUID"); /** * Validates that the authenticated caller can access a chat room. * * The room is fully identified by `roomId`, so no account override is accepted. - * When `options.allowAdmin` is set, a Recoup admin bypasses the ownership check - * (the room's owner is resolved server-side); everyone else must own the room. + * Access is granted to the room's owner; a Recoup admin (`RECOUP_ORG_ID` member) + * may access any room. The admin check runs only when the ownership check fails, + * so the common owner path never pays the extra lookup. * * @param request - The incoming request (used for auth context) * @param roomId - The room/chat UUID to validate access for - * @param options - Optional admin-bypass opt-in (read endpoints only) * @returns NextResponse on auth/access failure, or validated access data */ export async function validateChatAccess( request: NextRequest, roomId: string, - options: ValidateChatAccessOptions = {}, ): Promise { const roomIdResult = chatIdSchema.safeParse(roomId); if (!roomIdResult.success) { @@ -66,29 +55,22 @@ export async function validateChatAccess( ); } - // Admin bypass (read endpoints only): a Recoup admin may access any room. - // The owner is resolved server-side, so no account_id input is needed. - if (options.allowAdmin && (await checkIsAdmin(accountId))) { - return { roomId: room.id, room, accountId: room.account_id ?? accountId }; - } - - const { params, error } = await buildGetChatsParams({ - account_id: accountId, - }); + const { params } = await buildGetChatsParams({ account_id: accountId }); - if (!params) { - return NextResponse.json( - { status: "error", error: error ?? "Access denied" }, - { status: 403, headers: getCorsHeaders() }, - ); + const isOwner = !!params && !!room.account_id && params.account_ids.includes(room.account_id); + if (isOwner) { + return { roomId: room.id, room, accountId }; } - if (!room.account_id || !params.account_ids.includes(room.account_id)) { - return NextResponse.json( - { status: "error", error: "Access denied to this chat" }, - { status: 403, headers: getCorsHeaders() }, - ); + // Non-owner: a Recoup admin may access any chat (read or write). The owner is + // resolved server-side, so no account_id input is needed. Checked only here so + // the owner path above never pays the lookup. + if (await checkIsAdmin(accountId)) { + return { roomId: room.id, room, accountId: room.account_id ?? accountId }; } - return { roomId: room.id, room, accountId }; + return NextResponse.json( + { status: "error", error: "Access denied to this chat" }, + { status: 403, headers: getCorsHeaders() }, + ); } diff --git a/lib/chats/validateGetChatMessagesQuery.ts b/lib/chats/validateGetChatMessagesQuery.ts index 3b99ea8dd..2c52417ad 100644 --- a/lib/chats/validateGetChatMessagesQuery.ts +++ b/lib/chats/validateGetChatMessagesQuery.ts @@ -11,7 +11,7 @@ const chatIdSchema = z.string().uuid("id must be a valid UUID"); * * Reading is a resource-scoped operation: the chat is identified by the path id * and the owner is resolved server-side, so no `account_id` input is accepted. - * Recoup admins are granted read access to any chat via the admin bypass. + * Recoup admins are granted access to any chat via `validateChatAccess`. * * @param request - Incoming request used to validate chat access. * @param id - Chat identifier from route params. @@ -32,5 +32,5 @@ export async function validateGetChatMessagesQuery( ); } - return validateChatAccess(request, parsedId.data, { allowAdmin: true }); + return validateChatAccess(request, parsedId.data); } From 429fdb70511cf3b7bce7ca639d8e4f623c78a1ad Mon Sep 17 00:00:00 2001 From: Sweets Sweetman Date: Tue, 23 Jun 2026 08:41:23 -0500 Subject: [PATCH 4/4] refactor(chats): revert validateGetChatMessagesQuery (no change needed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin bypass lives entirely in validateChatAccess, which the messages endpoint already delegates to — so validateGetChatMessagesQuery needs no change. Reverts the doc-only edit and the redundant delegation test to keep the PR scoped to validateChatAccess. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../__tests__/validateGetChatMessagesQuery.test.ts | 14 -------------- lib/chats/validateGetChatMessagesQuery.ts | 4 ---- 2 files changed, 18 deletions(-) diff --git a/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts b/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts index ebf4d5c16..ffdfbdd38 100644 --- a/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts +++ b/lib/chats/__tests__/validateGetChatMessagesQuery.test.ts @@ -49,18 +49,4 @@ describe("validateGetChatMessagesQuery", () => { expect(result).not.toBeInstanceOf(NextResponse); expect((result as { roomId: string }).roomId).toBe(roomId); }); - - it("delegates to validateChatAccess with the parsed chat id", async () => { - const roomId = "123e4567-e89b-42d3-a456-426614174000"; - vi.mocked(validateChatAccess).mockResolvedValue({ - roomId, - room: { id: roomId, account_id: null, artist_id: null, topic: null, updated_at: null }, - accountId: "acc-123", - }); - const request = new NextRequest(`http://localhost/api/chats/${roomId}/messages`); - - await validateGetChatMessagesQuery(request, roomId); - - expect(validateChatAccess).toHaveBeenCalledWith(request, roomId); - }); }); diff --git a/lib/chats/validateGetChatMessagesQuery.ts b/lib/chats/validateGetChatMessagesQuery.ts index 2c52417ad..d28b42b3d 100644 --- a/lib/chats/validateGetChatMessagesQuery.ts +++ b/lib/chats/validateGetChatMessagesQuery.ts @@ -9,10 +9,6 @@ const chatIdSchema = z.string().uuid("id must be a valid UUID"); /** * Validates auth and params for GET /api/chats/[id]/messages. * - * Reading is a resource-scoped operation: the chat is identified by the path id - * and the owner is resolved server-side, so no `account_id` input is accepted. - * Recoup admins are granted access to any chat via `validateChatAccess`. - * * @param request - Incoming request used to validate chat access. * @param id - Chat identifier from route params. * @returns NextResponse on failure, or validated chat access data.