diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b0326a4d4..b94fac06f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -1,3 +1,22 @@ +# Bot review rounds: two, then stop. +# +# CodeRabbit runs on the ASSERTIVE profile and re-reviews on every push, so each +# round of fixes produces a fresh round of comments on the diff those fixes +# created. Round 1 clears the substantive findings; round 2 clears whatever +# round 1 provoked. After that, answer remaining comments in the thread and +# merge — do not push again just to silence the bot. +# +# The drawback, stated so nobody has to rediscover it: stopping at two means a +# genuine finding raised in round 3 gets a reply rather than a fix in this PR. +# That is the accepted cost. The alternative — looping until the bot is silent — +# does not converge on an ASSERTIVE profile: it reliably finds something on any +# new diff, and past round 2 that something is overwhelmingly refactor-for-its- +# own-sake (cognitive-complexity thresholds, helper extraction). Each extra push +# also resets approvals and burns a full CI cycle, so the churn is not free. +# +# Anything real that surfaces late belongs in a follow-up issue, where it keeps +# its own context, instead of growing this PR's diff past what a human will read. + name: CI on: diff --git a/.gitignore b/.gitignore index 041260c9f..48a1b3ef4 100644 --- a/.gitignore +++ b/.gitignore @@ -106,3 +106,8 @@ node-payment-main/ .claude/worktrees/ screens/ prompts/ + +# Pre-images written by scripts/stream/*.ts before they mutate the live Stream +# app. These are snapshots of production configuration (role grants, channel +# rosters) and rollback material for an operator's machine — not source. +.stream-backups/ diff --git a/__tests__/chat/channel-search-race.test.tsx b/__tests__/chat/channel-search-race.test.tsx new file mode 100644 index 000000000..eff3cc4f8 --- /dev/null +++ b/__tests__/chat/channel-search-race.test.tsx @@ -0,0 +1,256 @@ +/** + * Search told two lies, and both were about time rather than data. + * + * 1. "No results found for michael" rendered while Michael Chen sat in the list + * underneath. The empty state was gated on `!loading`, but `setLoading(true)` + * runs INSIDE the search, which fires 300ms after the keystroke — so for that + * whole window `loading` was false and no request existed yet. It announced + * failure before it had looked, on every keystroke. + * + * 2. An empty search box with a stale result still listed. There was no + * `AbortController` and no latest-wins guard, so `setSearchResults` committed + * whichever response landed last regardless of which query it answered. + * + * Both are invisible to a test that resolves fetches in order, which is why + * these deliberately resolve them OUT of order and assert on the settled DOM. + */ + +// Silences React's "not wrapped in act" warning; every render here is. +( + globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean } +).IS_REACT_ACT_ENVIRONMENT = true; + +const setActiveChannel = jest.fn(); +const openConversation = jest.fn(); + +jest.mock("stream-chat-react", () => ({ + useChatContext: () => ({ + client: { userID: "me", channel: jest.fn() }, + setActiveChannel, + }), +})); +jest.mock("../../components/chat/ChatPaneContext", () => ({ + useChatPane: () => ({ openConversation }), +})); +jest.mock("next/image", () => ({ + __esModule: true, + default: () => null, +})); + +import React, { act } from "react"; +import { createRoot, type Root } from "react-dom/client"; + +import { ChannelSearch } from "../../components/chat/ChannelSearch"; + +let host: HTMLDivElement; +let root: Root; + +/** A consultation row as the search route returns it. */ +function row(name: string, channelId: string) { + return { + id: `${channelId}-id`, + type: "consultation" as const, + name: `${name}'s plan`, + counterpartyName: name, + counterpartyUserId: `${channelId}-user`, + organizationId: null, + channelId, + }; +} + +/** A fetch whose responses resolve only when the test says so. */ +function deferredFetch() { + const pending: Array<{ + query: string; + resolve: (rows: ReturnType[]) => void; + }> = []; + + const fetchMock = jest.fn((url: string, init?: RequestInit) => { + const query = new URL(url, "http://localhost").searchParams.get("q") ?? ""; + return new Promise((resolve, reject) => { + // `new DOMException(msg, name)` already sets `.name`. Do NOT then + // `Object.assign` it — `name` is a getter-only accessor on the prototype, + // so writing to it throws a TypeError in strict mode, the listener dies, + // the promise never rejects, and the test "fails" against perfectly + // correct component code. + init?.signal?.addEventListener("abort", () => + reject(new DOMException("aborted", "AbortError")), + ); + pending.push({ + query, + resolve: (rows) => + resolve({ ok: true, json: async () => rows } as Response), + }); + }); + }); + + return { fetchMock, pending }; +} + +async function type(value: string) { + const input = host.querySelector("input") as HTMLInputElement; + await act(async () => { + const setter = Object.getOwnPropertyDescriptor( + window.HTMLInputElement.prototype, + "value", + )!.set!; + setter.call(input, value); + input.dispatchEvent(new Event("input", { bubbles: true })); + }); +} + +/** Advance past the 300ms debounce and let promises flush. */ +async function settleDebounce() { + await act(async () => { + jest.advanceTimersByTime(350); + await Promise.resolve(); + }); +} + +beforeEach(() => { + jest.useFakeTimers(); + host = document.createElement("div"); + document.body.appendChild(host); + root = createRoot(host); +}); + +afterEach(() => { + act(() => root.unmount()); + host.remove(); + jest.useRealTimers(); + jest.restoreAllMocks(); +}); + +describe("the empty state does not fire before a search has run", () => { + it("stays silent during the debounce window", async () => { + const { fetchMock } = deferredFetch(); + global.fetch = fetchMock as unknown as typeof fetch; + + await act(async () => root.render()); + await type("michael"); + + // Mid-debounce: the request has not been issued yet. + expect(fetchMock).not.toHaveBeenCalled(); + // This is the reported bug — the box claimed there was nothing to find + // before it had looked. + expect(host.textContent).not.toContain("No results found"); + }); + + it("stays silent while the request is in flight", async () => { + const { fetchMock } = deferredFetch(); + global.fetch = fetchMock as unknown as typeof fetch; + + await act(async () => root.render()); + await type("michael"); + await settleDebounce(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(host.textContent).not.toContain("No results found"); + }); + + it("shows it only once a search has genuinely returned nothing", async () => { + const { fetchMock, pending } = deferredFetch(); + global.fetch = fetchMock as unknown as typeof fetch; + + await act(async () => root.render()); + await type("michael"); + await settleDebounce(); + + await act(async () => { + pending[0].resolve([]); + await Promise.resolve(); + }); + + expect(host.textContent).toContain("No results found"); + }); +}); + +describe("out-of-order responses", () => { + it("ignores a stale response that lands after a newer one", async () => { + const { fetchMock, pending } = deferredFetch(); + global.fetch = fetchMock as unknown as typeof fetch; + + await act(async () => root.render()); + + await type("chen"); + await settleDebounce(); + await type("michael"); + await settleDebounce(); + + expect(pending).toHaveLength(2); + expect(pending[0].query).toBe("chen"); + expect(pending[1].query).toBe("michael"); + + // Newer first, then the stale one — the order that used to repaint the + // dropdown with results for a query the user had already moved off. + await act(async () => { + pending[1].resolve([row("Michael Chen", "dm-a-michael")]); + await Promise.resolve(); + }); + await act(async () => { + pending[0].resolve([row("Samantha Chen", "dm-a-samantha")]); + await Promise.resolve(); + }); + + expect(host.textContent).toContain("Michael Chen"); + expect(host.textContent).not.toContain("Samantha Chen"); + }); + + it("aborts the previous request when a new one starts", async () => { + const { fetchMock } = deferredFetch(); + global.fetch = fetchMock as unknown as typeof fetch; + + await act(async () => root.render()); + + await type("chen"); + await settleDebounce(); + await type("michael"); + await settleDebounce(); + + const firstSignal = (fetchMock.mock.calls[0][1] as RequestInit).signal; + expect(firstSignal?.aborted).toBe(true); + }); + + it("does not repopulate the dropdown after the query is cleared", async () => { + const { fetchMock, pending } = deferredFetch(); + global.fetch = fetchMock as unknown as typeof fetch; + + await act(async () => root.render()); + + await type("samantha"); + await settleDebounce(); + + // Clear the box, then let the earlier response arrive. This is the reported + // screenshot: an empty search field with a result still listed under it. + await type(""); + await settleDebounce(); + + await act(async () => { + pending[0].resolve([row("Samantha Chen", "dm-a-samantha")]); + await Promise.resolve(); + }); + + expect(host.textContent).not.toContain("Samantha Chen"); + }); +}); + +describe("what matched is visible", () => { + it("shows the plan title, not just the counterparty", async () => { + const { fetchMock, pending } = deferredFetch(); + global.fetch = fetchMock as unknown as typeof fetch; + + await act(async () => root.render()); + await type("michael"); + await settleDebounce(); + + // The route matches plan titles too, so a row can match on something the + // UI never displayed — which reads as a wrong result. + await act(async () => { + pending[0].resolve([row("Robert Brown", "dm-a-robert")]); + await Promise.resolve(); + }); + + expect(host.textContent).toContain("Robert Brown"); + expect(host.textContent).toContain("Robert Brown's plan"); + }); +}); diff --git a/__tests__/chat/phantom-dm-filtering.test.ts b/__tests__/chat/phantom-dm-filtering.test.ts new file mode 100644 index 000000000..8d236b092 --- /dev/null +++ b/__tests__/chat/phantom-dm-filtering.test.ts @@ -0,0 +1,103 @@ +/** + * A DM with nobody on the other end must not be reachable. + * + * The first attempt at this only changed the LABEL: `channelUtils` stopped + * printing the raw channel id and printed "Unavailable conversation" instead. + * That is cosmetic, and it showed — the row was still in the list, still + * clickable, still accepted a message. Renaming a broken thing does not fix it. + * + * `isUsableDmChannel` is the predicate the sidebar filters on, so a phantom + * never reaches the rendered list at all. These tests pin the two properties + * that matter: it catches every under-populated `messaging` channel, and it + * leaves `team` channels alone — a webinar channel legitimately holds only its + * host between creation and the first registration, and filtering those would + * hide working event chats. + */ +import type { Channel } from "stream-chat"; + +import { + getChannelDisplayInfo, + isUsableDmChannel, +} from "@/components/chat/utils/channelUtils"; + +/** Minimal stand-in — only the fields the predicate and the labeller read. */ +function fakeChannel( + type: "messaging" | "team", + memberIds: string[], + data: Record = {}, +): Channel { + return { + type, + id: `${type}-fixture`, + cid: `${type}:fixture`, + data, + state: { + members: Object.fromEntries( + memberIds.map((id) => [id, { user: { id, name: `User ${id}` } }]), + ), + }, + } as unknown as Channel; +} + +describe("isUsableDmChannel", () => { + it("rejects a messaging channel with no members", () => { + // The `watch()`-created phantom: `created_by` is set, membership is empty. + expect(isUsableDmChannel(fakeChannel("messaging", []))).toBe(false); + }); + + it("rejects a messaging channel with only the viewer", () => { + expect(isUsableDmChannel(fakeChannel("messaging", ["me"]))).toBe(false); + }); + + it("accepts a real two-person DM", () => { + expect(isUsableDmChannel(fakeChannel("messaging", ["me", "them"]))).toBe( + true, + ); + }); + + it("accepts a group DM", () => { + expect(isUsableDmChannel(fakeChannel("messaging", ["me", "a", "b"]))).toBe( + true, + ); + }); + + it("leaves a one-member team channel alone", () => { + // A webinar channel is created with its host before anyone registers. + // Applying the DM rule here would hide a working event chat. + expect(isUsableDmChannel(fakeChannel("team", ["host"]))).toBe(true); + }); + + it("leaves an empty team channel alone", () => { + expect(isUsableDmChannel(fakeChannel("team", []))).toBe(true); + }); + + it("survives a channel whose state has not loaded", () => { + const bare = { + type: "messaging", + id: "x", + cid: "messaging:x", + } as unknown as Channel; + expect(isUsableDmChannel(bare)).toBe(false); + }); +}); + +describe("the label is the last resort, not the fix", () => { + it("never renders a raw channel id for a broken DM", () => { + const info = getChannelDisplayInfo(fakeChannel("messaging", ["me"]), "me"); + + // The reported symptom was a header reading `dm-cmqb1680e0014txyocn1f0dbz-…`. + // A channel id is an internal key: it is not a name, and it leaks both + // participants' user ids into the UI. + expect(info.displayName).not.toMatch(/^messaging-/); + expect(info.displayName).toBe("Unavailable conversation"); + expect(info.statusText).toBe("No other participants"); + }); + + it("still names the counterparty on a healthy DM", () => { + const info = getChannelDisplayInfo( + fakeChannel("messaging", ["me", "them"]), + "me", + ); + expect(info.displayName).toBe("User them"); + }); +}); diff --git a/__tests__/security/block-partial-failure.test.ts b/__tests__/security/block-partial-failure.test.ts new file mode 100644 index 000000000..d65a93b6b --- /dev/null +++ b/__tests__/security/block-partial-failure.test.ts @@ -0,0 +1,184 @@ +/** + * @jest-environment node + */ + +/** + * A partial block must not be reported as a block. + * + * The ban loop was sequential, so the first rejection abandoned every remaining + * channel AND skipped the moderation report. Switching to `Promise.allSettled` + * fixed the abandonment and introduced a subtler problem: every channel was now + * attempted, but the route still answered `success: true` regardless of how many + * had actually been banned. The UI branches on `response.ok` alone and renders + * "This user can no longer message you" — over a conversation they can still + * post in. + * + * A pair can share more than one thread (personal plus one per org context), so + * partial failure is a real state, not a theoretical one. These pin the three + * outcomes: all-succeed, some-succeed, none-succeed. + */ +import { NextRequest } from "next/server"; + +const mockPrisma = { + user: { findUnique: jest.fn() }, + moderationReport: { create: jest.fn() }, +}; + +const mockQueryChannels = jest.fn(); + +jest.mock("../../lib/prisma", () => ({ + __esModule: true, + default: mockPrisma, +})); + +jest.mock("../../lib/stream-client", () => ({ + getStreamChatClient: () => ({ queryChannels: mockQueryChannels }), +})); + +jest.mock("../../lib/stream-logger", () => ({ + streamLogger: { + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + debug: jest.fn(), + }, +})); + +jest.mock("@sentry/nextjs", () => ({ captureException: jest.fn() })); + +const mockGetSession = jest.fn(); +jest.mock("../../lib/auth-server", () => ({ + getSession: () => mockGetSession(), +})); + +/** A channel whose `banUser` resolves or rejects on command. */ +function channel(id: string, outcome: "ok" | "fail") { + return { + id, + cid: `messaging:${id}`, + banUser: + outcome === "ok" + ? jest.fn().mockResolvedValue({}) + : jest.fn().mockRejectedValue(new Error("Stream refused")), + }; +} + +function request() { + return new NextRequest("http://localhost/api/stream/users/block", { + method: "POST", + body: JSON.stringify({ targetUserId: "target-user" }), + headers: { "Content-Type": "application/json" }, + }); +} + +async function post() { + const { POST } = await import("../../app/api/stream/users/block/route"); + return POST(request()); +} + +describe("POST /api/stream/users/block", () => { + beforeEach(() => { + jest.clearAllMocks(); + mockGetSession.mockResolvedValue({ user: { id: "blocker" } }); + mockPrisma.user.findUnique.mockResolvedValue({ id: "target-user" }); + mockPrisma.moderationReport.create.mockResolvedValue({ id: "report-1" }); + }); + + it("reports success only when every shared thread is banned", async () => { + mockQueryChannels.mockResolvedValue([ + channel("dm-a-b", "ok"), + channel("dmo-org-pair", "ok"), + ]); + + const response = await post(); + const body = await response.json(); + + expect(response.status).toBe(200); + expect(body.success).toBe(true); + expect(mockPrisma.moderationReport.create).toHaveBeenCalled(); + }); + + it("does NOT claim success when one thread is left writable", async () => { + mockQueryChannels.mockResolvedValue([ + channel("dm-a-b", "ok"), + channel("dmo-org-pair", "fail"), + ]); + + const response = await post(); + const body = await response.json(); + + // Not 2xx — the UI's only signal is `response.ok`, so a 200 here renders + // "This user can no longer message you" over a live channel. + expect(response.ok).toBe(false); + expect(body.success).toBe(false); + expect(body.partial).toBe(true); + expect(body.blocked).toBe(1); + expect(body.total).toBe(2); + // The message has to name the shortfall, because the client surfaces it + // verbatim. + expect(body.error).toMatch(/1 of 2/); + }); + + it("still records the moderation report on a partial block", async () => { + mockQueryChannels.mockResolvedValue([ + channel("dm-a-b", "ok"), + channel("dmo-org-pair", "fail"), + ]); + + await post(); + + // The attempt happened and half of it took effect. The audit trail is not + // conditional on the outcome being clean. + expect(mockPrisma.moderationReport.create).toHaveBeenCalled(); + }); + + it("attempts every channel rather than stopping at the first failure", async () => { + const first = channel("dm-a-b", "fail"); + const second = channel("dmo-org-pair", "ok"); + mockQueryChannels.mockResolvedValue([first, second]); + + await post(); + + // The sequential loop aborted here, leaving the second thread unbanned. + expect(first.banUser).toHaveBeenCalled(); + expect(second.banUser).toHaveBeenCalled(); + }); + + it("answers 503 and writes no report when every ban fails", async () => { + mockQueryChannels.mockResolvedValue([ + channel("dm-a-b", "fail"), + channel("dmo-org-pair", "fail"), + ]); + + const response = await post(); + const body = await response.json(); + + // The header names three outcomes; this is the third, and it was the one + // left untested. It differs from the partial case in a way that matters: + // nothing was blocked, so there is no action to audit. + expect(response.status).toBe(503); + expect(body.success).toBeUndefined(); + expect(mockPrisma.moderationReport.create).not.toHaveBeenCalled(); + }); + + it("answers 503 when the Stream lookup itself fails", async () => { + mockQueryChannels.mockRejectedValue(new Error("Stream down")); + + const response = await post(); + const body = await response.json(); + + // NOT the 403 "you can only block users you have a conversation with" — + // an outage is not a policy decision, and saying so to two people + // mid-conversation is the worst possible moment to be wrong. + expect(response.status).toBe(503); + expect(body.error).not.toMatch(/conversation with/); + }); + + it("answers 403 when there is genuinely no shared conversation", async () => { + mockQueryChannels.mockResolvedValue([]); + + const response = await post(); + + expect(response.status).toBe(403); + }); +}); diff --git a/__tests__/security/dm-channel-prefix-coverage.test.ts b/__tests__/security/dm-channel-prefix-coverage.test.ts new file mode 100644 index 000000000..aa3ad4137 --- /dev/null +++ b/__tests__/security/dm-channel-prefix-coverage.test.ts @@ -0,0 +1,116 @@ +/** + * `getDmChannelId` mints three shapes; `stream-channel-ids.ts` used to know one. + * + * `"dmo-".startsWith("dm-")` is false, and so is `"dmh-".startsWith("dm-")`. + * Because `DM_PREFIX` was the only DM prefix declared: + * + * - `getChannelTypeFromId("dmo-…")` fell through to its `team` default and + * returned `"team"` for a channel that was created as `messaging`. Four + * call sites then addressed the wrong type: the member-add action, both + * maintenance drain passes, and the event-channel expiry job. + * - `isDMChannel` answered false for two of the three DM forms. + * - `MANAGED_CHANNEL_PREFIXES` omitted them, so the reconciler never saw + * org-context DMs at all. + * + * None of that throws. A wrong channel type is a 404 or a duplicate channel, + * both silent — which is why this is pinned rather than left to review. + */ +import { + DM_HASHED_PREFIX, + DM_ORG_PREFIX, + DM_PREFIX, + MANAGED_CHANNEL_PREFIXES, + getChannelTypeFromId, + isDMChannel, + isEventChannel, +} from "@/lib/stream-channel-ids"; +import { getDmChannelId } from "@/lib/stream-utils"; + +// Two cuids, as Better Auth / Prisma produce them. +const CUID_A = "cmqb1680e0014txyocn1f0dbz"; +const CUID_B = "cmqb1680e0015txyocn1f0abc"; +// 36-char uuids — the 17 legacy accounts that overflow the 64-char ceiling. +const UUID_A = "3f2504e0-4f89-11d3-9a0c-0305e82c3301"; +const UUID_B = "9c858901-8a57-4791-81fe-4c455b099bc9"; +const ORG_ID = "org_acme_0001"; + +describe("every DM id form is a recognised messaging channel", () => { + const cases: Array<[string, string]> = [ + ["personal", getDmChannelId(CUID_A, CUID_B)], + ["org-scoped", getDmChannelId(CUID_A, CUID_B, ORG_ID)], + ["hashed overflow", getDmChannelId(UUID_A, UUID_B)], + ]; + + it.each(cases)("%s ids start with a declared prefix", (_label, id) => { + expect( + [DM_PREFIX, DM_ORG_PREFIX, DM_HASHED_PREFIX].some((p) => + id.startsWith(p), + ), + ).toBe(true); + }); + + it.each(cases)("%s resolves to the messaging type", (_label, id) => { + expect(getChannelTypeFromId(id)).toBe("messaging"); + }); + + it.each(cases)("%s is recognised as a DM", (_label, id) => { + expect(isDMChannel(id)).toBe(true); + }); + + it.each(cases)("%s is not mistaken for an event channel", (_label, id) => { + expect(isEventChannel(id)).toBe(false); + }); + + it.each(cases)("%s is reconciled, not orphaned", (_label, id) => { + expect( + MANAGED_CHANNEL_PREFIXES.some((prefix) => id.startsWith(prefix)), + ).toBe(true); + }); +}); + +describe("the three forms stay distinct", () => { + it("org and personal ids for the same pair differ", () => { + expect(getDmChannelId(CUID_A, CUID_B)).not.toBe( + getDmChannelId(CUID_A, CUID_B, ORG_ID), + ); + }); + + it("two orgs do not collide for the same pair", () => { + expect(getDmChannelId(CUID_A, CUID_B, "org-one")).not.toBe( + getDmChannelId(CUID_A, CUID_B, "org-two"), + ); + }); + + it("stays inside Stream's 64-character ceiling", () => { + for (const id of [ + getDmChannelId(CUID_A, CUID_B), + getDmChannelId(CUID_A, CUID_B, ORG_ID), + getDmChannelId(UUID_A, UUID_B), + getDmChannelId(UUID_A, UUID_B, ORG_ID), + ]) { + expect(id.length).toBeLessThanOrEqual(64); + } + }); +}); + +describe("event channels are untouched by the DM prefixes", () => { + it("still resolves webinar and class ids to team", () => { + expect(getChannelTypeFromId("webinar-abc123")).toBe("team"); + expect(getChannelTypeFromId("class-abc123")).toBe("team"); + }); + + it("does not treat a webinar as a DM", () => { + expect(isDMChannel("webinar-abc123")).toBe(false); + }); +}); + +describe("self-pairs are refused rather than collapsed", () => { + it("throws instead of deriving dm--", () => { + // `createChannel` de-duplicates its member array through a Set, so a + // self-pair produced a one-member `messaging` channel: no counterparty to + // render, so the header fell through to the raw id, and no second member + // to reply. Refusing at derivation is what stops that reaching Stream. + expect(() => getDmChannelId(CUID_A, CUID_A)).toThrow(/self-DM/i); + expect(() => getDmChannelId(CUID_A, CUID_A, ORG_ID)).toThrow(/self-DM/i); + }); +}); diff --git a/__tests__/security/dm-eligibility.test.ts b/__tests__/security/dm-eligibility.test.ts new file mode 100644 index 000000000..b1c561963 --- /dev/null +++ b/__tests__/security/dm-eligibility.test.ts @@ -0,0 +1,344 @@ +/** + * The gate that decides whether two people may hold a direct message. + * + * This is the check that did not exist. `createDirectMessageChannel` validated + * two non-empty strings and nothing else — no session, no relationship query, + * not even `a !== b` — and was safe only because every caller happened to be a + * booking-approval or payment-success path. An implementation of the rule DID + * exist (`checkUserRelationship`) with a full unit-test suite and zero + * production call sites. + * + * What is pinned here is the behaviour that made the difference between the old + * advisory flag and a real gate: + * - the widened, unified status set (the three copies disagreed); + * - both directions, because one person can hold both profiles; + * - soft-deleted slots do not count; + * - a self-pair is refused without a query; + * - a failed lookup throws rather than answering "not related". + */ +import { DM_ELIGIBLE_STATUSES } from "@/lib/stream/dm-eligibility-statuses"; + +// Declared inline rather than pulled from `__tests__/stream/__mocks__`: the +// shared helper covers the whole Stream surface, and `jest/no-mocks-import` +// bars importing it from outside that directory. Four models is little enough +// to state here, and it keeps what this suite depends on visible. +const mockPrisma = { + user: { findUnique: jest.fn() }, + consultation: { findFirst: jest.fn(), findMany: jest.fn() }, + subscription: { findFirst: jest.fn(), findMany: jest.fn() }, + slotOfAppointment: { findFirst: jest.fn() }, +}; + +jest.mock("../../lib/prisma", () => ({ + __esModule: true, + default: mockPrisma, +})); + +const CONSULTANT = { + consultantProfileId: "cp-1", + consulteeProfileId: null, +}; +const CONSULTEE = { + consultantProfileId: null, + consulteeProfileId: "ce-1", +}; +/** One person holding both profiles — an ordinary shape, not an edge case. */ +const DUAL = { + consultantProfileId: "cp-dual", + consulteeProfileId: "ce-dual", +}; + +/** Nothing found anywhere unless a test says otherwise. */ +function noRelationships() { + mockPrisma.consultation.findFirst.mockResolvedValue(null); + mockPrisma.subscription.findFirst.mockResolvedValue(null); + mockPrisma.slotOfAppointment.findFirst.mockResolvedValue(null); +} + +function bothUsersExist(a: unknown, b: unknown) { + mockPrisma.user.findUnique.mockResolvedValueOnce(a).mockResolvedValueOnce(b); +} + +async function load() { + return import("../../lib/stream/dm-eligibility"); +} + +describe("canDirectMessage", () => { + beforeEach(() => { + jest.clearAllMocks(); + noRelationships(); + }); + + it("permits a pair with a consultation", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + mockPrisma.consultation.findFirst.mockResolvedValue({ id: "c-1" }); + + const { canDirectMessage } = await load(); + await expect(canDirectMessage("consultant-u", "consultee-u")).resolves.toBe( + true, + ); + }); + + it("permits a pair with a subscription", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + mockPrisma.subscription.findFirst.mockResolvedValue({ id: "s-1" }); + + const { canDirectMessage } = await load(); + await expect(canDirectMessage("consultant-u", "consultee-u")).resolves.toBe( + true, + ); + }); + + it("refuses a pair sharing ONLY an event slot (no consultee↔consultee DMs)", async () => { + // The group arm was removed from the gate: with `POST /api/stream/channels/open` + // live, co-membership of one event slot WOULD be a user-reachable path to a + // peer DM, which the moderation ADR forbids. Host↔attendee DMs are not + // offered by any surface either — event rows open the team channel. + bothUsersExist(CONSULTEE, CONSULTEE); + mockPrisma.slotOfAppointment.findFirst.mockResolvedValue({ id: "slot-1" }); + + const { canDirectMessage } = await load(); + await expect(canDirectMessage("attendee-a", "attendee-b")).resolves.toBe( + false, + ); + // The slot table is no longer consulted at all. + expect(mockPrisma.slotOfAppointment.findFirst).not.toHaveBeenCalled(); + }); + + it("refuses two strangers", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + + const { canDirectMessage } = await load(); + await expect(canDirectMessage("stranger-a", "stranger-b")).resolves.toBe( + false, + ); + }); + + it("refuses when either user does not exist", async () => { + mockPrisma.user.findUnique.mockResolvedValue(null); + + const { canDirectMessage } = await load(); + await expect(canDirectMessage("ghost-a", "ghost-b")).resolves.toBe(false); + }); + + it("refuses a self-pair without querying at all", async () => { + const { canDirectMessage } = await load(); + + await expect(canDirectMessage("same", "same")).resolves.toBe(false); + // The cheapest check runs first. It also has to run BEFORE the profile + // lookup, because a dual-profile user genuinely does satisfy both arms of + // every relationship query against themselves. + expect(mockPrisma.user.findUnique).not.toHaveBeenCalled(); + }); + + it("propagates a lookup failure instead of answering false", async () => { + mockPrisma.user.findUnique.mockRejectedValue(new Error("DB down")); + + const { canDirectMessage } = await load(); + // The old implementation caught this and returned `false`. As an unused + // advisory flag that was harmless; as the gate on channel creation it means + // a transient blip denies a real conversation, and on the reconcile path it + // makes a live channel look unexpected and therefore sweepable. + await expect(canDirectMessage("a", "b")).rejects.toThrow("DB down"); + }); +}); + +describe("the status set the gate queries", () => { + beforeEach(() => { + jest.clearAllMocks(); + noRelationships(); + }); + + it("is the shared ever-transacted set, not a narrower literal", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + + const { canDirectMessage } = await load(); + await canDirectMessage("consultant-u", "consultee-u"); + + expect(mockPrisma.consultation.findFirst).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ + status: { in: [...DM_ELIGIBLE_STATUSES] }, + }), + }), + ); + }); + + it("includes COMPLETED and APPROVED_PENDING_PAYMENT", () => { + // The two the reconciler and the old gate omitted while search included + // them. That gap is what produced search rows whose channel had never been + // created — which `channel.watch()` then created, memberless. + expect(DM_ELIGIBLE_STATUSES).toContain("COMPLETED"); + expect(DM_ELIGIBLE_STATUSES).toContain("APPROVED_PENDING_PAYMENT"); + }); + + it("excludes PENDING", () => { + // Otherwise anyone opens a channel with anyone by requesting a booking they + // never intend to pay for. + expect(DM_ELIGIBLE_STATUSES).not.toContain("PENDING"); + }); + + it("does not bound subscriptions by their scheduling window", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + + const { canDirectMessage } = await load(); + await canDirectMessage("consultant-u", "consultee-u"); + + const where = mockPrisma.subscription.findFirst.mock.calls[0][0].where; + // A lapsed subscription is still a relationship that happened. The window + // filter used to make the thread unreachable at midnight on the renewal + // date, mid-conversation and with no notice to either party. + expect(where).not.toHaveProperty("schedulingPeriodEndsAt"); + }); +}); + +describe("both directions are checked", () => { + beforeEach(() => { + jest.clearAllMocks(); + noRelationships(); + }); + + it("finds the link when the caller is the consultee", async () => { + bothUsersExist(CONSULTEE, CONSULTANT); + mockPrisma.consultation.findFirst.mockResolvedValue({ id: "c-1" }); + + const { canDirectMessage } = await load(); + await expect(canDirectMessage("consultee-u", "consultant-u")).resolves.toBe( + true, + ); + }); + + it("builds both direction clauses for a dual-profile pair", async () => { + bothUsersExist(DUAL, DUAL); + + const { canDirectMessage } = await load(); + await canDirectMessage("dual-a", "dual-b"); + + const where = mockPrisma.consultation.findFirst.mock.calls[0][0].where; + // Reading only one direction silently denied every pair where the + // "consultant" side happened to be listed second. + expect(where.OR).toHaveLength(2); + }); +}); + +describe("assertCanDirectMessage", () => { + beforeEach(() => { + jest.clearAllMocks(); + noRelationships(); + }); + + it("resolves quietly when permitted", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + mockPrisma.consultation.findFirst.mockResolvedValue({ id: "c-1" }); + + const { assertCanDirectMessage } = await load(); + await expect( + assertCanDirectMessage("consultant-u", "consultee-u"), + ).resolves.toBeUndefined(); + }); + + it("throws a typed error carrying both ids when refused", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + + const { assertCanDirectMessage, DmNotPermittedError } = await load(); + await expect( + assertCanDirectMessage("stranger-a", "stranger-b"), + ).rejects.toBeInstanceOf(DmNotPermittedError); + }); +}); + +describe("buildDirections is shared, not duplicated", () => { + beforeEach(() => { + jest.clearAllMocks(); + noRelationships(); + }); + + // The two link checks used to carry byte-identical copies of the + // direction-building. Asserting both produce the same OR shape is what stops + // a fix landing in one and not the other. + it("consultation and subscription build the same directions", async () => { + bothUsersExist(DUAL, DUAL); + + const { canDirectMessage } = await load(); + await canDirectMessage("dual-a", "dual-b"); + + const consultationOr = + mockPrisma.consultation.findFirst.mock.calls[0][0].where.OR; + const subscriptionOr = + mockPrisma.subscription.findFirst.mock.calls[0][0].where.OR; + + expect(consultationOr).toHaveLength(subscriptionOr.length); + expect( + consultationOr.map((d: { requestedById: string }) => d.requestedById), + ).toEqual( + subscriptionOr.map((d: { requestedById: string }) => d.requestedById), + ); + }); +}); + + +describe("pairBookingContexts (org-forgery guard)", () => { + beforeEach(() => { + jest.clearAllMocks(); + noRelationships(); + }); + + it("returns only org contexts the pair actually holds", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + mockPrisma.consultation.findMany.mockResolvedValue([ + { consultationPlan: { organizationId: "org-real" }, appointment: null }, + ]); + mockPrisma.subscription.findMany.mockResolvedValue([]); + + const { pairBookingContexts } = await load(); + const ctx = await pairBookingContexts("a", "b"); + + // The whole point: an arbitrary org id ("org-fake") can never be named, + // because the allowed set is derived from the bookings themselves. + expect(ctx).toEqual({ personalAllowed: false, organizations: ["org-real"] }); + }); + + it("marks personal context when a booking has no org", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + mockPrisma.consultation.findMany.mockResolvedValue([ + { + consultationPlan: { organizationId: null }, + appointment: { organizationId: null }, + }, + ]); + mockPrisma.subscription.findMany.mockResolvedValue([]); + + const { pairBookingContexts } = await load(); + const ctx = await pairBookingContexts("a", "b"); + + expect(ctx.personalAllowed).toBe(true); + expect(ctx.organizations).toEqual([]); + }); + + it("answers empty for strangers without querying bookings", async () => { + mockPrisma.user.findUnique.mockResolvedValue(null); + + const { pairBookingContexts } = await load(); + const ctx = await pairBookingContexts("ghost-a", "ghost-b"); + + expect(ctx).toEqual({ personalAllowed: false, organizations: [] }); + expect(mockPrisma.consultation.findMany).not.toHaveBeenCalled(); + }); + + it("uses bookingOrgId precedence — plan org wins over appointment org", async () => { + bothUsersExist(CONSULTANT, CONSULTEE); + mockPrisma.consultation.findMany.mockResolvedValue([ + { + consultationPlan: { organizationId: "org-plan" }, + appointment: { organizationId: "org-appointment" }, + }, + ]); + mockPrisma.subscription.findMany.mockResolvedValue([]); + + const { pairBookingContexts } = await load(); + const ctx = await pairBookingContexts("a", "b"); + + expect(ctx.organizations).toEqual(["org-plan"]); + expect(ctx.personalAllowed).toBe(false); + }); +}); diff --git a/__tests__/stream/channel-actions.test.ts b/__tests__/stream/channel-actions.test.ts index 2a506d6ab..f22247be9 100644 --- a/__tests__/stream/channel-actions.test.ts +++ b/__tests__/stream/channel-actions.test.ts @@ -52,9 +52,20 @@ jest.mock("../../lib/auth-helpers", () => ({ isPrivileged: (role?: string | null) => role === "ADMIN" || role === "STAFF", })); +// The DM eligibility gate is exercised against real query shapes in +// __tests__/security/dm-eligibility.test.ts. Here it is stubbed so these tests +// keep asserting what they are about — id derivation and Stream call shape — +// rather than turning into relationship fixtures. Default: permitted. +const mockAssertCanDirectMessage = jest.fn(); +jest.mock("../../lib/stream/dm-eligibility", () => ({ + assertCanDirectMessage: (...args: unknown[]) => + mockAssertCanDirectMessage(...args), +})); + describe("Channel Actions", () => { beforeEach(() => { jest.clearAllMocks(); + mockAssertCanDirectMessage.mockResolvedValue(undefined); mockStreamClient.channel.mockReturnValue(mockChannel); mockStreamClient.queryChannels.mockResolvedValue([]); mockGetSession.mockResolvedValue({ @@ -292,12 +303,52 @@ describe("Channel Actions", () => { await expect(createDirectMessageChannel("", "user2")).rejects.toThrow(); await expect(createDirectMessageChannel("user1", "")).rejects.toThrow(); }); + + it("consults the eligibility gate before creating anything", async () => { + const { createDirectMessageChannel } = + await import("../../actions/stream/chat/channel.action"); + + mockChannel.query.mockResolvedValue({ members: {} }); + await createDirectMessageChannel("bob", "alice"); + + expect(mockAssertCanDirectMessage).toHaveBeenCalledWith("bob", "alice"); + }); + + it("creates no channel when the pair has no booking link", async () => { + const { createDirectMessageChannel } = + await import("../../actions/stream/chat/channel.action"); + + mockAssertCanDirectMessage.mockRejectedValue( + new Error("Direct messages are only available between people who share a booking."), + ); + + await expect( + createDirectMessageChannel("stranger-a", "stranger-b"), + ).rejects.toThrow("share a booking"); + + // The gate has to run BEFORE the Stream call, not alongside it — a + // refusal that still creates the channel is not a refusal. + expect(mockStreamClient.channel).not.toHaveBeenCalled(); + }); + + it("refuses a self-pair at id derivation", async () => { + const { createDirectMessageChannel } = + await import("../../actions/stream/chat/channel.action"); + + // The gate is stubbed permissive here, so this asserts the SECOND line of + // defence: getDmChannelId itself rejects `a === a` rather than producing + // `dm-a-a`, which createChannel would then de-duplicate into a + // one-member channel that renders as its own raw id. + await expect( + createDirectMessageChannel("same-user", "same-user"), + ).rejects.toThrow(/self-DM/i); + }); }); describe("addMemberToChannel", () => { it("should add member to existing channel", async () => { const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); const result = await addMemberToChannel( "consultation-123", @@ -310,7 +361,7 @@ describe("Channel Actions", () => { it("should infer messaging type for consultation channels", async () => { const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); await addMemberToChannel("consultation-abc", "user123"); @@ -322,7 +373,7 @@ describe("Channel Actions", () => { it("should infer messaging type for subscription channels", async () => { const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); await addMemberToChannel("subscription-xyz", "user456"); @@ -334,7 +385,7 @@ describe("Channel Actions", () => { it("should infer team type for other channels", async () => { const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); await addMemberToChannel("webinar-123", "user789"); @@ -346,7 +397,7 @@ describe("Channel Actions", () => { it("should reject invalid inputs", async () => { const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); await expect(addMemberToChannel("", "user")).rejects.toThrow(); await expect(addMemberToChannel("channel", "")).rejects.toThrow(); @@ -358,7 +409,7 @@ describe("Channel Actions", () => { mockGetSession.mockResolvedValueOnce(null); const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); await expect( addMemberToChannel("consultation-123", "new-user-id"), @@ -378,7 +429,7 @@ describe("Channel Actions", () => { }); const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); await expect( addMemberToChannel("consultation-123", "new-user-id"), @@ -397,7 +448,7 @@ describe("Channel Actions", () => { }); const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); const result = await addMemberToChannel( "consultation-123", @@ -554,9 +605,10 @@ describe("Entity Channel Creation", () => { await import("../../actions/stream/chat/channel.action"); const result = await createConsultationChannel("consultation-789"); + expect(result).not.toBeNull(); // IDs sorted: "consultant-3" < "consultee-1" alphabetically - expect(result.channelId).toBe("dm-consultant-3-consultee-1"); + expect(result!.channelId).toBe("dm-consultant-3-consultee-1"); expect(mockStreamClient.channel).toHaveBeenCalledWith( "messaging", "dm-consultant-3-consultee-1", @@ -613,9 +665,10 @@ describe("Entity Channel Creation", () => { await import("../../actions/stream/chat/channel.action"); const result = await createSubscriptionChannel("subscription-101"); + expect(result).not.toBeNull(); // IDs sorted: "consultant-4" < "subscriber-1" alphabetically - expect(result.channelId).toBe("dm-consultant-4-subscriber-1"); + expect(result!.channelId).toBe("dm-consultant-4-subscriber-1"); expect(mockStreamClient.channel).toHaveBeenCalledWith( "messaging", "dm-consultant-4-subscriber-1", @@ -670,7 +723,7 @@ describe("addMemberToChannel error handling", () => { mockChannel.addMembers.mockRejectedValueOnce(new Error("API error")); const { addMemberToChannel } = - await import("../../actions/stream/chat/channel.action"); + await import("../../actions/stream/chat/member.action"); await expect( addMemberToChannel("test-channel", "user-123"), diff --git a/__tests__/stream/event-channel-actions.test.ts b/__tests__/stream/event-channel-actions.test.ts index 9b9a0fff4..ec6737582 100644 --- a/__tests__/stream/event-channel-actions.test.ts +++ b/__tests__/stream/event-channel-actions.test.ts @@ -10,6 +10,7 @@ import { createMockLogger, createMockChannelCache, } from "./__mocks__/stream-mocks"; +import { DM_ELIGIBLE_STATUSES } from "@/lib/stream/dm-eligibility-statuses"; // Create mock instances const mockPrisma = createMockPrisma(); @@ -747,7 +748,13 @@ describe("Event Channel Actions", () => { ); }); - it("should handle partial failures gracefully", async () => { + // Was "should handle partial failures gracefully", asserting the add + // pass's success/fail tally. That pass is gone: the sync no longer creates + // or joins channels, it only removes memberships the user is no longer + // entitled to. `POST /api/stream/channels/open` provisions on demand + // instead, so the sync's unbounded per-pair Stream calls — which grew with + // every COMPLETED booking, forever — are not paid on dashboard load. + it("creates no channels — the add pass is retired", async () => { mockPrisma.user.findUnique.mockResolvedValue({ consultantProfileId: null, consulteeProfileId: "consultee-123", @@ -771,14 +778,7 @@ describe("Event Channel Actions", () => { }, ]); mockPrisma.subscription.findMany.mockResolvedValue([]); - mockCache.getMembershipCached.mockReturnValue(false); - // Pair 1 addMembers succeeds; pair 2 addMembers rejects (falls through to create) - mockChannel.addMembers - .mockResolvedValueOnce({}) - .mockRejectedValueOnce(new Error("addMembers failed")); - // Pair 2's fallthrough create also fails → pair 2 counted as failed - mockChannel.create.mockRejectedValueOnce(new Error("Create failed")); const { syncUserEventChannels } = await import("../../actions/stream/chat/event-channel.action"); @@ -786,8 +786,12 @@ describe("Event Channel Actions", () => { const result = await syncUserEventChannels("user-with-failures"); expect(result.success).toBe(true); - expect(result.channelsSynced).toBe(1); - expect(result.failed).toBe(1); + // Both pairs are still EXPECTED — that set drives the stale-removal pass + // and must stay complete, or the reconciler evicts live conversations. + expect(result.channelsSynced).toBe(2); + // The point of the change: no Stream writes for those two pairs. + expect(mockChannel.create).not.toHaveBeenCalled(); + expect(mockChannel.addMembers).not.toHaveBeenCalled(); }); it("should throw on invalid user ID", async () => { @@ -907,7 +911,14 @@ describe("Event Channel Actions", () => { expect(mockPrisma.consultation.findMany).toHaveBeenCalledWith( expect.objectContaining({ where: expect.objectContaining({ - status: { in: ["APPROVED", "SCHEDULED"] }, + // Asserted against the shared constant, not a literal. These two + // assertions are exactly what would have caught the divergence + // that caused the bug — the reconciler pinned to a narrower set + // than the search routes used — except that they pinned the + // narrow side, so widening search sailed past them. Referencing + // DM_ELIGIBLE_STATUSES means the two can no longer drift apart + // without this failing. + status: { in: [...DM_ELIGIBLE_STATUSES] }, }), }), ); @@ -958,7 +969,14 @@ describe("Event Channel Actions", () => { expect(mockPrisma.subscription.findMany).toHaveBeenCalledWith( expect.objectContaining({ where: expect.objectContaining({ - status: { in: ["APPROVED", "SCHEDULED"] }, + // Asserted against the shared constant, not a literal. These two + // assertions are exactly what would have caught the divergence + // that caused the bug — the reconciler pinned to a narrower set + // than the search routes used — except that they pinned the + // narrow side, so widening search sailed past them. Referencing + // DM_ELIGIBLE_STATUSES means the two can no longer drift apart + // without this failing. + status: { in: [...DM_ELIGIBLE_STATUSES] }, }), }), ); diff --git a/__tests__/stream/user-actions.test.ts b/__tests__/stream/user-actions.test.ts index 3b1bff61e..d12d49214 100644 --- a/__tests__/stream/user-actions.test.ts +++ b/__tests__/stream/user-actions.test.ts @@ -38,6 +38,14 @@ jest.mock("../../lib/stream-logger", () => ({ jest.mock("../../lib/stream-cache", () => mockUserCache); +// searchUsersWithRelationships is session-scoped; identity comes from the +// mocked session ("current-user"), not from a client-supplied parameter. +const mockGetSession = jest.fn(); +jest.mock("../../lib/auth-server", () => ({ + getSession: (disableCookieCache?: boolean) => + mockGetSession(disableCookieCache), +})); + jest.mock("../../lib/user", () => mockRoleMapper); describe("User Actions", () => { @@ -46,6 +54,9 @@ describe("User Actions", () => { mockStreamClient.upsertUser.mockReset(); mockStreamClient.upsertUsers.mockReset(); mockUserCache.isUserSynced.mockReturnValue(false); + mockGetSession.mockResolvedValue({ + user: { id: "current-user", role: "CONSULTANT" }, + }); }); describe("upsertUserToStream", () => { @@ -219,272 +230,6 @@ describe("User Actions", () => { }); }); - describe("searchUsers", () => { - it("should search users by name or email", async () => { - const mockResults = [ - { - id: "user-1", - name: "John Doe", - email: "john@test.com", - image: null, - role: "CONSULTANT", - }, - { - id: "user-2", - name: "Jane Doe", - email: "jane@test.com", - image: null, - role: "CONSULTEE", - }, - ]; - - mockPrisma.user.findMany.mockResolvedValue(mockResults); - - const { searchUsers } = - await import("../../actions/stream/chat/user.action"); - - const results = await searchUsers("Doe"); - - expect(mockPrisma.user.findMany).toHaveBeenCalledWith({ - where: { - AND: [ - { - NOT: [ - { id: { startsWith: "recording-egress-" } }, - { id: { startsWith: "system-" } }, - ], - }, - { - OR: [ - { name: { contains: "Doe", mode: "insensitive" } }, - { email: { contains: "Doe", mode: "insensitive" } }, - ], - }, - ], - }, - select: { - id: true, - name: true, - email: true, - image: true, - role: true, - }, - take: 10, - orderBy: [{ name: "asc" }], - }); - - expect(results).toHaveLength(2); - }); - - it("should exclude system users from search", async () => { - mockPrisma.user.findMany.mockResolvedValue([]); - - const { searchUsers } = - await import("../../actions/stream/chat/user.action"); - - await searchUsers("test"); - - expect(mockPrisma.user.findMany).toHaveBeenCalledWith( - expect.objectContaining({ - where: expect.objectContaining({ - AND: expect.arrayContaining([ - { - NOT: [ - { id: { startsWith: "recording-egress-" } }, - { id: { startsWith: "system-" } }, - ], - }, - ]), - }), - }), - ); - }); - - it("should reject empty search term", async () => { - const { searchUsers } = - await import("../../actions/stream/chat/user.action"); - - await expect(searchUsers("")).rejects.toThrow(); - }); - - it("should reject too long search term", async () => { - const { searchUsers } = - await import("../../actions/stream/chat/user.action"); - - const longTerm = "a".repeat(101); - await expect(searchUsers(longTerm)).rejects.toThrow(); - }); - }); - - describe("checkUserRelationship", () => { - it("should return true if users share a consultation", async () => { - mockPrisma.user.findUnique - .mockResolvedValueOnce({ - consultantProfileId: "consultant-profile-1", - consulteeProfileId: null, - }) - .mockResolvedValueOnce({ - consultantProfileId: null, - consulteeProfileId: "consultee-profile-1", - }); - - mockPrisma.consultation.findFirst.mockResolvedValue({ - id: "consultation-1", - }); - - mockPrisma.subscription.findFirst.mockResolvedValue(null); - mockPrisma.slotOfAppointment.findFirst.mockResolvedValue(null); - - const { checkUserRelationship } = - await import("../../actions/stream/chat/user.action"); - - const hasRelationship = await checkUserRelationship( - "consultant-user", - "consultee-user", - ); - - expect(hasRelationship).toBe(true); - }); - - it("should return false if no relationship exists", async () => { - mockPrisma.user.findUnique - .mockResolvedValueOnce({ - consultantProfileId: "consultant-profile-1", - consulteeProfileId: null, - }) - .mockResolvedValueOnce({ - consultantProfileId: "consultant-profile-2", - consulteeProfileId: null, - }); - - mockPrisma.consultation.findFirst.mockResolvedValue(null); - mockPrisma.subscription.findFirst.mockResolvedValue(null); - mockPrisma.slotOfAppointment.findFirst.mockResolvedValue(null); - - const { checkUserRelationship } = - await import("../../actions/stream/chat/user.action"); - - const hasRelationship = await checkUserRelationship( - "consultant-1", - "consultant-2", - ); - - expect(hasRelationship).toBe(false); - }); - - it("should return false if user not found", async () => { - mockPrisma.user.findUnique.mockResolvedValue(null); - - const { checkUserRelationship } = - await import("../../actions/stream/chat/user.action"); - - const hasRelationship = await checkUserRelationship( - "nonexistent-1", - "nonexistent-2", - ); - - expect(hasRelationship).toBe(false); - }); - - it("should return true if users share appointment slot", async () => { - mockPrisma.user.findUnique - .mockResolvedValueOnce({ - consultantProfileId: null, - consulteeProfileId: "consultee-1", - }) - .mockResolvedValueOnce({ - consultantProfileId: null, - consulteeProfileId: "consultee-2", - }); - - mockPrisma.consultation.findFirst.mockResolvedValue(null); - mockPrisma.subscription.findFirst.mockResolvedValue(null); - mockPrisma.slotOfAppointment.findFirst.mockResolvedValue({ - id: "shared-slot-1", - }); - - const { checkUserRelationship } = - await import("../../actions/stream/chat/user.action"); - - const hasRelationship = await checkUserRelationship("user-a", "user-b"); - - expect(hasRelationship).toBe(true); - }); - - it("should return false on database error", async () => { - mockPrisma.user.findUnique.mockRejectedValue(new Error("DB error")); - - const { checkUserRelationship } = - await import("../../actions/stream/chat/user.action"); - - const hasRelationship = await checkUserRelationship("user-1", "user-2"); - - expect(hasRelationship).toBe(false); - expect(mockLogger.error).toHaveBeenCalledWith( - "Relationship check failed", - expect.any(Error), - expect.objectContaining({ userId1: "user-1", userId2: "user-2" }), - ); - }); - - it("should find reverse consultation relationship", async () => { - mockPrisma.user.findUnique - .mockResolvedValueOnce({ - consultantProfileId: null, - consulteeProfileId: "consultee-profile-1", - }) - .mockResolvedValueOnce({ - consultantProfileId: "consultant-profile-2", - consulteeProfileId: null, - }); - - // First check returns null, second check (reverse) returns a consultation - mockPrisma.consultation.findFirst.mockResolvedValue({ - id: "reverse-consultation-1", - }); - mockPrisma.subscription.findFirst.mockResolvedValue(null); - mockPrisma.slotOfAppointment.findFirst.mockResolvedValue(null); - - const { checkUserRelationship } = - await import("../../actions/stream/chat/user.action"); - - const hasRelationship = await checkUserRelationship( - "consultee-user", - "consultant-user", - ); - - expect(hasRelationship).toBe(true); - }); - - it("should find reverse subscription relationship", async () => { - mockPrisma.user.findUnique - .mockResolvedValueOnce({ - consultantProfileId: null, - consulteeProfileId: "consultee-profile-1", - }) - .mockResolvedValueOnce({ - consultantProfileId: "consultant-profile-2", - consulteeProfileId: null, - }); - - mockPrisma.consultation.findFirst.mockResolvedValue(null); - mockPrisma.subscription.findFirst.mockResolvedValue({ - id: "reverse-subscription-1", - }); - mockPrisma.slotOfAppointment.findFirst.mockResolvedValue(null); - - const { checkUserRelationship } = - await import("../../actions/stream/chat/user.action"); - - const hasRelationship = await checkUserRelationship( - "consultee-user", - "consultant-user", - ); - - expect(hasRelationship).toBe(true); - }); - }); - describe("upsertUsersToStream error paths", () => { it("should return empty result when no users found for unsynced IDs", async () => { mockUserCache.isUserSynced.mockReturnValue(false); @@ -573,13 +318,13 @@ describe("User Actions", () => { const { searchUsersWithRelationships } = await import("../../actions/stream/chat/user.action"); - const results = await searchUsersWithRelationships( - "test", - "current-user", - ); + const results = await searchUsersWithRelationships("test"); - expect(results).toHaveLength(2); - expect(results[0]).toHaveProperty("hasRelationship"); + // Filtered, not ranked. Both matches are unrelated to the caller, so + // neither is returned — previously both came back with + // `hasRelationship: false`, which made this endpoint a directory of the + // whole user base behind a two-character query. + expect(results).toHaveLength(0); }); it("should exclude current user from results", async () => { @@ -588,12 +333,12 @@ describe("User Actions", () => { const { searchUsersWithRelationships } = await import("../../actions/stream/chat/user.action"); - await searchUsersWithRelationships("test", "current-user-id"); + await searchUsersWithRelationships("test"); expect(mockPrisma.user.findMany).toHaveBeenCalledWith( expect.objectContaining({ where: expect.objectContaining({ - AND: expect.arrayContaining([{ id: { not: "current-user-id" } }]), + AND: expect.arrayContaining([{ id: { not: "current-user" } }]), }), }), ); @@ -640,12 +385,11 @@ describe("User Actions", () => { const { searchUsersWithRelationships } = await import("../../actions/stream/chat/user.action"); - const results = await searchUsersWithRelationships( - "test", - "current-user", - ); + const results = await searchUsersWithRelationships("test"); - // Connected user (Adam with relationship) should come first + // Only the connected user survives the filter. + expect(results).toHaveLength(1); + expect(results[0].id).toBe("user-with-rel"); expect(results[0].hasRelationship).toBe(true); }); @@ -656,7 +400,7 @@ describe("User Actions", () => { await import("../../actions/stream/chat/user.action"); await expect( - searchUsersWithRelationships("test", "current-user"), + searchUsersWithRelationships("test"), ).rejects.toThrow("DB failure"); expect(mockLogger.error).toHaveBeenCalledWith( @@ -670,28 +414,7 @@ describe("User Actions", () => { const { searchUsersWithRelationships } = await import("../../actions/stream/chat/user.action"); - await expect( - searchUsersWithRelationships("", "current-user"), - ).rejects.toThrow(); - }); - }); - - describe("searchUsers error handling", () => { - it("should throw and log error on database failure", async () => { - mockPrisma.user.findMany.mockRejectedValue( - new Error("DB connection failed"), - ); - - const { searchUsers } = - await import("../../actions/stream/chat/user.action"); - - await expect(searchUsers("test")).rejects.toThrow("DB connection failed"); - - expect(mockLogger.error).toHaveBeenCalledWith( - "Legacy user search failed", - expect.any(Error), - expect.objectContaining({ searchTerm: "test" }), - ); + await expect(searchUsersWithRelationships("")).rejects.toThrow(); }); }); -}); +}); \ No newline at end of file diff --git a/actions/stream/chat/channel.action.ts b/actions/stream/chat/channel.action.ts index 76b6cf905..d75d431f3 100644 --- a/actions/stream/chat/channel.action.ts +++ b/actions/stream/chat/channel.action.ts @@ -29,10 +29,7 @@ import { getDmChannelId, isChannelAlreadyExistsError, } from "@/lib/stream-utils"; -import { getChannelTypeFromId } from "@/lib/stream-channel-ids"; -import { getSession } from "@/lib/auth-server"; -import { isPrivileged } from "@/lib/auth-helpers"; -import * as Sentry from "@sentry/nextjs"; +import { assertCanDirectMessage } from "@/lib/stream/dm-eligibility"; // Input validation schemas const channelTypeSchema = z.enum(["messaging", "team"]); @@ -198,7 +195,21 @@ export async function createChannel(input: { } /** - * Create a direct message channel between two users + * Create a direct message channel between two users. + * + * The eligibility check is the point of this function now. It used to validate + * two non-empty strings and nothing else — no session, no relationship query, + * not even `a !== b` — and was safe only by accident, because every caller + * happened to be a booking-approval or payment-success path where the link was + * already established. That is an invariant held by convention across five call + * sites in three files, which is not an invariant. It is enforced here so that + * adding a sixth caller cannot quietly reopen the hole. + * + * Note this deliberately gates on the RELATIONSHIP, not on the caller's + * session. Every legitimate caller is server-side and acts on behalf of the + * system (a Razorpay webhook has no session at all), so a session check here + * would break the create path while adding nothing — the user-initiated + * surface is `POST /api/stream/channels/dm`, which does both. */ export async function createDirectMessageChannel( currentUserId: string, @@ -215,6 +226,8 @@ export async function createDirectMessageChannel( memberIdSchema.parse(currentUserId); memberIdSchema.parse(targetUserId); + await assertCanDirectMessage(currentUserId, targetUserId); + const channelId = getDmChannelId(currentUserId, targetUserId, organizationId); return createChannel({ @@ -432,6 +445,16 @@ export async function createConsultationChannel( const consultantId = consultation.consultationPlan.consultantProfile.user.id; const consulteeId = consultation.requestedBy.user.id; + // Legacy self-booked row (checkout blocks these now): getDmChannelId throws + // on a self-pair, so skip rather than take the whole approval path down. + // Same guard the search routes apply per row. + if (consultantId === consulteeId) { + streamLogger.warn("Skipping consultation channel — consultant and consultee are the same user", { + consultationId, + }); + return null; + } + if (!consultantId || !consulteeId) { throw new Error( `Participants not found for consultation: ${consultationId}`, @@ -521,6 +544,14 @@ export async function createSubscriptionChannel( const consultantId = subscription.subscriptionPlan.consultantProfile.user.id; const consulteeId = subscription.requestedBy.user.id; + // Same self-pair guard as the consultation path above. + if (consultantId === consulteeId) { + streamLogger.warn("Skipping subscription channel — consultant and consultee are the same user", { + subscriptionId, + }); + return null; + } + if (!consultantId || !consulteeId) { throw new Error( `Participants not found for subscription: ${subscriptionId}`, @@ -701,66 +732,3 @@ export async function createCollaboratorChannel( channelData, }; } - -/** - * Adds a user to a specific channel. - * - * Stream's server-side API bypasses its permission system entirely, so the - * authz gate lives here (#899): ADMIN/STAFF may add to any channel; anyone - * else only to a channel they created — mirroring the create-route checks. - * Non-privileged callers never lazily create channels they don't own. - */ -export async function addMemberToChannel( - channelId: string, - userId: string, - channelType?: "messaging" | "team", -) { - channelIdSchema.parse(channelId); - memberIdSchema.parse(userId); - - const session = await getSession(); - if (!session?.user?.id) { - throw new Error("Unauthorized: sign in to manage channel members"); - } - - const client = getStreamChatClient(); - - const resolvedChannelType = channelType ?? getChannelTypeFromId(channelId); - - streamLogger.debug("Adding member to channel", { - channelId, - userId, - channelType: resolvedChannelType, - }); - - try { - const channel = client.channel(resolvedChannelType, channelId); - const privileged = isPrivileged(session.user.role); - if (privileged) { - await channel.create(); // Creates if doesn't exist, no-op if exists - } else { - const state = await channel.query({}); - const createdById = state.channel?.created_by?.id; - if (createdById !== session.user.id) { - throw new Error( - "Forbidden: only the channel creator or staff may add members", - ); - } - } - - const response = await channel.addMembers([userId]); - - streamLogger.debug("Member added successfully", { channelId, userId }); - return { success: true, response }; - } catch (error) { - streamLogger.error("Failed to add member to channel", error, { - channelId, - userId, - }); - Sentry.captureException( - error instanceof Error ? error : new Error(String(error)), - { tags: { subsystem: "stream" } }, - ); - throw error; - } -} diff --git a/actions/stream/chat/event-channel.action.ts b/actions/stream/chat/event-channel.action.ts index 8bc6ebfe5..585af0c1d 100644 --- a/actions/stream/chat/event-channel.action.ts +++ b/actions/stream/chat/event-channel.action.ts @@ -24,6 +24,7 @@ import { getDmChannelId, isChannelAlreadyExistsError, } from "@/lib/stream-utils"; +import { dmEligibleStatusFilter } from "@/lib/stream/dm-eligibility-statuses"; import { ConsentRequiredError } from "@/lib/compliance/dpdp"; import { DEFAULT_RETENTION_DAYS, @@ -620,48 +621,35 @@ export async function syncUserEventChannels( ), ]); - // --- Add pass: join any channels the user is missing --- + // --- There is no longer an add pass. --- + // + // This used to walk `eventIds` and `dmPairs` five at a time, calling + // `addUserToEventChannel` / `addUserToDmChannel` for every one, creating + // any channel that did not exist yet. It ran on every cold dashboard load. + // + // Two things made that untenable. It is unbounded: neither + // `getDmPairsForUser` nor the event helpers carry a `take`, and since + // `DM_ELIGIBLE_STATUSES` includes `COMPLETED` — an absorbing state — the + // pair list is every consultation the consultant has EVER finished, so it + // only grows. A consultant with 500 completed bookings paid 100 serial + // waves of Stream calls in the background of every load. And it is now + // redundant: `POST /api/stream/channels/open` creates the channel on + // demand, with both members, at the moment someone actually opens the + // conversation. Provisioning 500 channels on the chance one gets opened is + // work done for nothing. + // + // `expectedChannelIds` above is still computed — the reconcile pass below + // needs it to decide what is stale, and that half is not replaceable by an + // on-demand path: nothing else notices that a membership OUGHT to be + // revoked. + // + // The trade, stated plainly: a user who has lost membership to a channel + // that still exists is no longer silently re-added here. They recover by + // opening the conversation from search, which routes through + // `/api/stream/channels/open` and re-adds them. Booking approval and + // payment success still provision channels eagerly, so this only affects + // repair, not creation. const BATCH_SIZE = 5; - let successCount = 0; - let failCount = 0; - - if (eventIds.length > 0) { - for (let i = 0; i < eventIds.length; i += BATCH_SIZE) { - const batch = eventIds.slice(i, i + BATCH_SIZE); - - const results = await Promise.allSettled( - batch.map((event) => - addUserToEventChannel(event.type, event.id, userId), - ), - ); - - results.forEach((result) => { - if (result.status === "fulfilled") successCount++; - else failCount++; - }); - } - } - - // --- DM add-pass: one channel per pair PER FUNDING CONTEXT --- - // A pair working both B2C and through an org now has two threads, and this - // pass joins the user to each. `dmPairs` is already keyed that way. - for (let i = 0; i < dmPairs.length; i += BATCH_SIZE) { - const batch = dmPairs.slice(i, i + BATCH_SIZE); - const results = await Promise.allSettled( - batch.map((pair) => - addUserToDmChannel( - pair.consultantUserId, - pair.consulteeUserId, - userId, - pair.organizationId, - ), - ), - ); - results.forEach((r) => { - if (r.status === "fulfilled") successCount++; - else failCount++; - }); - } // --- Reconciliation pass: remove user from stale channels --- // Query Stream for every channel this user currently belongs to. @@ -732,9 +720,9 @@ export async function syncUserEventChannels( const duration = Date.now() - startTime; streamLogger.info("Channel sync completed", { userId, - successCount, - failCount, + expectedChannels: expectedChannelIds.size, staleChannelsRemoved: staleRemovedCount, + staleFailed: staleFailCount, durationMs: duration, }); @@ -743,8 +731,11 @@ export async function syncUserEventChannels( return { success: true, - channelsSynced: successCount, - failed: failCount, + // Kept for the existing callers' shape. Nothing is "synced" in the + // create sense any more; this is how many channels the user is expected + // to be in, which is the useful number for the same debugging. + channelsSynced: expectedChannelIds.size, + failed: staleFailCount, staleChannelsRemoved: staleRemovedCount, durationMs: duration, }; @@ -785,7 +776,7 @@ async function getDmPairsForUser( prisma.consultation.findMany({ where: { consultationPlan: { consultantProfileId: user.consultantProfileId }, - status: { in: ["APPROVED", "SCHEDULED"] }, + status: dmEligibleStatusFilter(), }, include: { requestedBy: { include: { user: { select: { id: true } } } }, @@ -800,7 +791,7 @@ async function getDmPairsForUser( prisma.subscription.findMany({ where: { subscriptionPlan: { consultantProfileId: user.consultantProfileId }, - status: { in: ["APPROVED", "SCHEDULED"] }, + status: dmEligibleStatusFilter(), }, include: { requestedBy: { include: { user: { select: { id: true } } } }, @@ -820,7 +811,12 @@ async function getDmPairsForUser( ]); for (const c of [...consultations, ...subscriptions]) { const consulteeUserId = c.requestedBy?.user?.id; - if (!consulteeUserId) continue; + // Skip, do not throw. `getDmChannelId` rejects a self-pair, and this loop + // runs un-isolated inside syncUserEventChannels — one dual-profile user + // who self-booked would otherwise abort the entire reconcile for + // themselves and leave every other channel unsynced. Checkout blocks + // self-booking, so this only fires on legacy or seeded rows. + if (!consulteeUserId || consulteeUserId === userId) continue; const organizationId = bookingOrgId(c); const channelId = getDmChannelId(userId, consulteeUserId, organizationId); pairMap.set(channelId, { @@ -836,7 +832,7 @@ async function getDmPairsForUser( prisma.consultation.findMany({ where: { requestedById: user.consulteeProfileId, - status: { in: ["APPROVED", "SCHEDULED"] }, + status: dmEligibleStatusFilter(), }, include: { consultationPlan: { @@ -852,7 +848,7 @@ async function getDmPairsForUser( prisma.subscription.findMany({ where: { requestedById: user.consulteeProfileId, - status: { in: ["APPROVED", "SCHEDULED"] }, + status: dmEligibleStatusFilter(), }, include: { subscriptionPlan: { @@ -877,7 +873,7 @@ async function getDmPairsForUser( ]); for (const c of consultations) { const consultantUserId = c.consultationPlan?.consultantProfile?.user?.id; - if (!consultantUserId) continue; + if (!consultantUserId || consultantUserId === userId) continue; const organizationId = bookingOrgId(c); const channelId = getDmChannelId(consultantUserId, userId, organizationId); pairMap.set(channelId, { @@ -889,7 +885,7 @@ async function getDmPairsForUser( for (const sub of subscriptions) { const consultantUserId = sub.subscriptionPlan?.consultantProfile?.user?.id; - if (!consultantUserId) continue; + if (!consultantUserId || consultantUserId === userId) continue; const organizationId = bookingOrgId(sub); const channelId = getDmChannelId(consultantUserId, userId, organizationId); pairMap.set(channelId, { @@ -904,112 +900,15 @@ async function getDmPairsForUser( } /** - * Create or join a DM channel for a consultant-consultee pair. + * `addUserToDmChannel` used to live here — a private create-or-join for a + * consultant/consultee pair, called only by the sync's DM add pass. + * + * Removed with that pass. It duplicated `createDirectMessageChannel` in + * `channel.action.ts`, which is what `POST /api/stream/channels/open` and the + * booking paths use, and which unlike this one runs the eligibility gate. + * Leaving an ungated, unused channel-provisioning helper in the module is an + * invitation to wire it back in without the check. */ -async function addUserToDmChannel( - consultantUserId: string, - consulteeUserId: string, - currentUserId: string, - /** Funding context — the channel key differs per org (see getDmChannelId). */ - organizationId: string | null, -): Promise<{ success: boolean; channelId: string; created?: boolean }> { - const channelId = getDmChannelId( - consultantUserId, - consulteeUserId, - organizationId, - ); - const channelType = "messaging"; - - if (getMembershipCached(channelId, currentUserId) === true) { - return { success: true, channelId }; - } - - const client = getStreamChatClient(); - const channel = client.channel(channelType, channelId); - - // Try adding to existing channel first - try { - // #473 — surface breaker-open as StreamUnavailableError so an outage - // doesn't get mistaken for "channel missing" and trigger a doomed create. - await withStreamCircuitBreaker( - () => channel.addMembers([currentUserId]), - () => { - throw new StreamUnavailableError(); - }, - ); - markMembership(channelId, currentUserId, true); - return { success: true, channelId }; - } catch (addError) { - if (addError instanceof StreamUnavailableError) throw addError; - // Channel may not exist — fall through to creation - } - - // Create the DM channel - await upsertUsersToStream([consultantUserId, consulteeUserId]); - const channelWithData = client.channel(channelType, channelId, { - members: [consultantUserId, consulteeUserId], - created_by_id: consultantUserId, - dm_consultant_user_id: consultantUserId, - dm_consultee_user_id: consulteeUserId, - } as Record); - // F-HIGH-3: same adopt-on-duplicate-create contract as the event path — a - // concurrent creator of this DM wins the race, we adopt their channel. - let adoptRetryFailed = false; - try { - await withStreamCircuitBreaker( - () => channelWithData.create(), - () => { - throw new StreamUnavailableError(); - }, - ); - } catch (createError) { - if (!isChannelAlreadyExistsError(createError)) throw createError; - - streamLogger.info("Lost channel-create race; adopting existing channel", { - channelId, - currentUserId, - }); - - // The winner's roster snapshot may predate us — retry our own membership - // once. Best-effort: logged on failure, never fatal to the join. - try { - await channel.addMembers([currentUserId]); - } catch (adoptError) { - adoptRetryFailed = true; - streamLogger.warn("Post-adoption addMembers retry failed (non-fatal)", { - channelId, - currentUserId, - error: adoptError, - }); - } - } - - // Lazy-create bypasses createChannel, so the #899 channel-scoped host - // grant is repeated here. Non-fatal: chat still works without it. - try { - await channelWithData.assignRoles([ - { user_id: consultantUserId, channel_role: "channel_moderator" }, - ]); - } catch (grantError) { - streamLogger.warn("Failed to grant channel_moderator to DM consultant", { - channelId, - consultantUserId, - error: grantError, - }); - } - - markChannelExists(channelType, channelId); - // Same uncached-on-failed-retry rule as the event path above. - if (!adoptRetryFailed) { - markMembership(channelId, currentUserId, true); - } - streamLogger.info("Created DM channel", { - channelId, - consultantUserId, - consulteeUserId, - }); - return { success: true, channelId, created: true }; -} /** * F-HIGH-2 — Postgres rows outlive Stream channels. The retention cron diff --git a/actions/stream/chat/member.action.ts b/actions/stream/chat/member.action.ts new file mode 100644 index 000000000..d4e4893cc --- /dev/null +++ b/actions/stream/chat/member.action.ts @@ -0,0 +1,93 @@ +"use server"; + +/** + * Client-callable channel membership mutation. + * + * Lives in its own `"use server"` file, deliberately separate from + * `channel.action.ts`: that module lost its directive (architecture review + * F-HIGH-1) so its exports can never be invoked from a browser, but THIS + * operation is genuinely client-facing — `ChannelInfoAndManageDialog` calls it + * to add members through the server-side authorization gate (#899) instead of + * `channel.addMembers()` straight from the browser. A gated action in a + * dedicated file is the sanctioned shape for that (see the header comment in + * `channel.action.ts`). + * + * Stream's server-side API bypasses its permission system entirely, so the + * authz gate lives here: ADMIN/STAFF may add to any channel; anyone else only + * to a channel they created — mirroring the create-route checks. + */ +import { z } from "zod"; +import { getStreamChatClient } from "@/lib/stream-client"; +import { streamLogger } from "@/lib/stream-logger"; +import { getSession } from "@/lib/auth-server"; +import { isPrivileged } from "@/lib/auth-helpers"; +import { getChannelTypeFromId, isDMChannel } from "@/lib/stream-channel-ids"; +import * as Sentry from "@sentry/nextjs"; + +const channelIdSchema = z.string().min(1, "Channel ID is required"); +const memberIdSchema = z.string().min(1, "Member ID is required"); + +export async function addMemberToChannel( + channelId: string, + userId: string, + channelType?: "messaging" | "team", +) { + channelIdSchema.parse(channelId); + memberIdSchema.parse(userId); + + const session = await getSession(true); + if (!session?.user?.id) { + throw new Error("Unauthorized: sign in to manage channel members"); + } + + // DM membership is pair-derived (`getDmChannelId` + `canDirectMessage`). + // Allowing a DM creator to name a third member here would bypass that + // eligibility gate with server credentials — so direct-message channels are + // out of scope for this action entirely. + if (isDMChannel(channelId)) { + throw new Error( + "Forbidden: members cannot be added to direct messages", + ); + } + + const client = getStreamChatClient(); + + const resolvedChannelType = channelType ?? getChannelTypeFromId(channelId); + + streamLogger.debug("Adding member to channel", { + channelId, + userId, + channelType: resolvedChannelType, + }); + + try { + const channel = client.channel(resolvedChannelType, channelId); + const privileged = isPrivileged(session.user.role); + if (privileged) { + await channel.create(); // Creates if doesn't exist, no-op if exists + } else { + const state = await channel.query({}); + const createdById = state.channel?.created_by?.id; + if (createdById !== session.user.id) { + throw new Error( + "Forbidden: only the channel creator or staff may add members", + ); + } + } + + const response = await channel.addMembers([userId]); + + streamLogger.debug("Member added successfully", { channelId, userId }); + return { success: true, response }; + } catch (error) { + streamLogger.error("Failed to add member to channel", error, { + channelId, + userId, + }); + Sentry.captureException( + error instanceof Error ? error : new Error(String(error)), + { tags: { subsystem: "stream" } }, + ); + throw error; + } +} diff --git a/actions/stream/chat/user.action.ts b/actions/stream/chat/user.action.ts index 2a9ab3aea..beab63d28 100644 --- a/actions/stream/chat/user.action.ts +++ b/actions/stream/chat/user.action.ts @@ -11,9 +11,11 @@ import { import { forEachChunk } from "@/lib/stream/batch"; import { streamLogger } from "@/lib/stream-logger"; import { markUserSynced, isUserSynced } from "@/lib/stream-cache"; +import { dmEligibleStatusFilter } from "@/lib/stream/dm-eligibility-statuses"; import { checkConsent, ConsentRequiredError } from "@/lib/compliance/dpdp"; import { PURPOSE_CODES } from "@/lib/compliance/purpose-codes"; import * as Sentry from "@sentry/nextjs"; +import { getSession } from "@/lib/auth-server"; // Input validation schemas const userIdSchema = z.string().min(1, "User ID is required"); @@ -22,6 +24,27 @@ const userIdsSchema = z .min(1, "At least one user ID required"); const searchTermSchema = z.string().min(1, "Search term is required").max(100); +/** How many results the caller actually sees. */ +const SEARCH_RESULT_LIMIT = 20; + +/** + * How many name/email matches to pull before the relationship filter runs. + * + * These have to be different numbers. The relationship filter is applied in JS + * after the query, so capping the query at the display limit meant the database + * picked 20 rows alphabetically and the filter then discarded whichever of them + * were strangers — a search for a common surname could return twenty unrelated + * people, filter to zero, and never see the actual client sitting at position + * twenty-one. The filter was silently competing with the limit for the same + * budget. + * + * 200 is a deliberate ceiling rather than an unbounded fetch: `contains` on + * `name`/`email` is a sequential scan, and this runs on every keystroke past two + * characters. Wide enough that the filter is choosing from real candidates, + * narrow enough that a one-letter-over-the-minimum query cannot walk the table. + */ +const SEARCH_CANDIDATE_LIMIT = 200; + /** * Upserts a user to Stream Chat * Uses caching to avoid redundant upserts @@ -254,13 +277,20 @@ export const upsertUsersToStream = async (userIds: string[]) => { * @param currentUserId The current user's ID to exclude from results * @returns Users with relationship status information */ -export const searchUsersWithRelationships = async ( - searchTerm: string, - currentUserId: string, -) => { +export const searchUsersWithRelationships = async (searchTerm: string) => { // Validate inputs const validatedTerm = searchTermSchema.parse(searchTerm.trim()); - const validatedUserId = userIdSchema.parse(currentUserId); + + // Identity comes from the SESSION, not from a parameter. This module is + // `"use server"`, so every export is remotely invocable; the previous + // signature took `currentUserId` from the caller, which let anyone enumerate + // another user's related parties (names, emails, avatars) by passing their + // id. Same cookie-cache-bypass reasoning as assertCanMintToken. + const session = await getSession(true); + if (!session?.user?.id) { + throw new Error("Unauthorized: sign in to search users"); + } + const validatedUserId = userIdSchema.parse(session.user.id); try { // Fetch current user's profile IDs once @@ -296,24 +326,21 @@ export const searchUsersWithRelationships = async ( consultantProfileId: true, consulteeProfileId: true, }, - take: 20, + take: SEARCH_CANDIDATE_LIMIT, orderBy: [{ name: "asc" }], }), ]); + // No caller profile means no relationship is derivable, so nothing is + // returnable. This branch used to `return users.map(… hasRelationship: + // false)` — handing back the full unfiltered match set precisely when the + // relationship check could not run, which is the one case where it mattered + // most. Fail closed. if (!currentUser || users.length === 0) { - return users.map((u) => ({ - id: u.id, - name: u.name, - email: u.email, - image: u.image, - role: u.role, - hasRelationship: false, - })); + return []; } // Batch: find all related user IDs in a few queries instead of N+1 - const resultUserIds = users.map((u) => u.id); const resultConsultantProfileIds = users .map((u) => u.consultantProfileId) .filter((id): id is string => !!id); @@ -334,23 +361,24 @@ export const searchUsersWithRelationships = async ( where: { consultationPlan: { consultantProfileId: currentUser.consultantProfileId }, requestedById: { in: resultConsulteeProfileIds }, - status: { in: ["APPROVED", "SCHEDULED"] }, + status: dmEligibleStatusFilter(), }, select: { requestedBy: { select: { user: { select: { id: true } } } } }, }) .then((rows) => rows.forEach((r) => { if (r.requestedBy?.user?.id) relatedUserIds.add(r.requestedBy.user.id); })), - // Subscriptions are time-bounded (have a scheduling period), so we must - // filter by schedulingPeriodEndsAt to exclude expired ones. Consultations - // are per-event with no time window, so status alone is sufficient. + // No `schedulingPeriodEndsAt` bound: under the ever-transacted rule a + // lapsed subscription is still a relationship that happened, and the + // window used to make this disagree with the search routes about who + // counts as connected. Same set everywhere now — see + // lib/stream/dm-eligibility.ts. prisma.subscription .findMany({ where: { subscriptionPlan: { consultantProfileId: currentUser.consultantProfileId }, requestedById: { in: resultConsulteeProfileIds }, - status: { in: ["APPROVED", "SCHEDULED"] }, - schedulingPeriodEndsAt: { gte: new Date() }, + status: dmEligibleStatusFilter(), }, select: { requestedBy: { select: { user: { select: { id: true } } } } }, }) @@ -368,7 +396,7 @@ export const searchUsersWithRelationships = async ( where: { consultationPlan: { consultantProfileId: { in: resultConsultantProfileIds } }, requestedById: currentUser.consulteeProfileId, - status: { in: ["APPROVED", "SCHEDULED"] }, + status: dmEligibleStatusFilter(), }, select: { consultationPlan: { @@ -385,8 +413,7 @@ export const searchUsersWithRelationships = async ( where: { subscriptionPlan: { consultantProfileId: { in: resultConsultantProfileIds } }, requestedById: currentUser.consulteeProfileId, - status: { in: ["APPROVED", "SCHEDULED"] }, - schedulingPeriodEndsAt: { gte: new Date() }, + status: dmEligibleStatusFilter(), }, select: { subscriptionPlan: { @@ -401,47 +428,59 @@ export const searchUsersWithRelationships = async ( ); } - // Shared appointments (webinars, classes) - single batched query - if (resultUserIds.length > 0) { - relationshipQueries.push( - prisma.slotOfAppointment - .findMany({ - where: { - user: { some: { id: validatedUserId } }, - AND: { user: { some: { id: { in: resultUserIds } } } }, - }, - select: { user: { where: { id: { in: resultUserIds } }, select: { id: true } } }, - }) - .then((slots) => slots.forEach((s) => s.user.forEach((u) => relatedUserIds.add(u.id)))), - ); - } + // NOTE: no shared-slot arm here, matching `canDirectMessage` (PR #1188 + // review). Co-membership of an event slot used to mark fellow attendees as + // "related", which put consultee↔consultee rows into search that the open + // route's eligibility gate then refused — a row you can see but cannot + // click, and one step toward re-opening peer DMs. Event chat goes through + // the team channel via the open route's event arm. await Promise.all(relationshipQueries); - // Map results with batch-resolved relationship status - const usersWithRelationships = users.map((user) => ({ - id: user.id, - name: user.name, - email: user.email, - image: user.image, - role: user.role, - hasRelationship: relatedUserIds.has(user.id), - })); - - // Sort by relationship status (connected users first), then by name + // Drop unrelated users rather than ranking them below related ones. + // + // `hasRelationship` was a SORT KEY, not a filter: a search for "char" + // returned every Charlotte on the platform — name, email, avatar and role — + // with the connected ones merely listed first. That is a directory of the + // entire user base behind a two-character query, and the field name made it + // read like a gate. Unrelated users are now not returned at all. + // + // `hasRelationship` stays on the payload, and is now always `true`. It is + // kept so existing consumers do not break on a missing key; new code should + // treat presence in this list as the answer. + const usersWithRelationships = users + .filter((user) => relatedUserIds.has(user.id)) + .map((user) => ({ + id: user.id, + name: user.name, + email: user.email, + image: user.image, + role: user.role, + hasRelationship: true as const, + })); + + // Code-unit ordering, not localeCompare — same rule as the channel ids + // (#1134 P0-3). Nothing keys off this ordering, but the codebase has been + // bitten once by ICU-dependent sorts and a consistent habit is cheaper than + // remembering which sorts are load-bearing. usersWithRelationships.sort((a, b) => { - if (a.hasRelationship && !b.hasRelationship) return -1; - if (!a.hasRelationship && b.hasRelationship) return 1; - return (a.name || "").localeCompare(b.name || ""); + const an = a.name || ""; + const bn = b.name || ""; + return an < bn ? -1 : an > bn ? 1 : 0; }); + // Truncated here, AFTER filtering and sorting — never in the query. + const page = usersWithRelationships.slice(0, SEARCH_RESULT_LIMIT); + streamLogger.debug("User search completed", { term: validatedTerm, - resultCount: usersWithRelationships.length, - relatedCount: relatedUserIds.size, + candidateCount: users.length, + relatedCount: usersWithRelationships.length, + resultCount: page.length, + candidateLimitHit: users.length === SEARCH_CANDIDATE_LIMIT, }); - return usersWithRelationships; + return page; } catch (error) { Sentry.captureException(error instanceof Error ? error : new Error(String(error)), { tags: { subsystem: "stream" } }); streamLogger.error("User search failed", error, { @@ -452,235 +491,11 @@ export const searchUsersWithRelationships = async ( }; /** - * Check if two users have any relationship through appointments - * @param userId1 First user ID - * @param userId2 Second user ID - * @returns Boolean indicating if they have any relationship - */ -export const checkUserRelationship = async ( - userId1: string, - userId2: string, -): Promise => { - try { - // Get profile IDs for both users in parallel - const [user1, user2] = await Promise.all([ - prisma.user.findUnique({ - where: { id: userId1 }, - select: { consultantProfileId: true, consulteeProfileId: true }, - }), - prisma.user.findUnique({ - where: { id: userId2 }, - select: { consultantProfileId: true, consulteeProfileId: true }, - }), - ]); - - if (!user1 || !user2) return false; - - // Check for relationships in parallel - const relationshipChecks = await Promise.all([ - checkConsultationRelationship(user1, user2), - checkSubscriptionRelationship(user1, user2), - checkSharedAppointments(userId1, userId2), - ]); - - return relationshipChecks.some(Boolean); - } catch (error) { - Sentry.captureException(error instanceof Error ? error : new Error(String(error)), { tags: { subsystem: "stream" } }); - streamLogger.error("Relationship check failed", error, { - userId1, - userId2, - }); - return false; // Default to no relationship on error - } -}; - -/** - * Check consultation relationships between two users + * The ungated `checkUserRelationship` and deprecated `searchUsers` exports were + * REMOVED from this `"use server"` module (PR #1188 review): every export here + * is remotely invocable, so they answered "do these two arbitrary users share a + * booking?" and global PII search to any browser. The rule's one implementation + * is `canDirectMessage` in `lib/stream/dm-eligibility.ts`; user-facing search + * goes through the session-scoped `searchUsersWithRelationships` or the + * `/api/stream/search-consultees` route. */ -async function checkConsultationRelationship( - user1: { - consultantProfileId: string | null; - consulteeProfileId: string | null; - }, - user2: { - consultantProfileId: string | null; - consulteeProfileId: string | null; - }, -): Promise { - if (!user1.consultantProfileId && !user1.consulteeProfileId) return false; - if (!user2.consultantProfileId && !user2.consulteeProfileId) return false; - - const checks: Promise[] = []; - - // Check if user1 (consultant) has consultations with user2 (consultee) - if (user1.consultantProfileId && user2.consulteeProfileId) { - checks.push( - prisma.consultation - .findFirst({ - where: { - consultationPlan: { - consultantProfileId: user1.consultantProfileId, - }, - requestedById: user2.consulteeProfileId, - status: { in: ["APPROVED", "SCHEDULED"] }, - }, - select: { id: true }, - }) - .then((r) => !!r), - ); - } - - // Check reverse relationship - if (user2.consultantProfileId && user1.consulteeProfileId) { - checks.push( - prisma.consultation - .findFirst({ - where: { - consultationPlan: { - consultantProfileId: user2.consultantProfileId, - }, - requestedById: user1.consulteeProfileId, - status: { in: ["APPROVED", "SCHEDULED"] }, - }, - select: { id: true }, - }) - .then((r) => !!r), - ); - } - - if (checks.length === 0) return false; - - const results = await Promise.all(checks); - return results.some(Boolean); -} - -/** - * Check subscription relationships between two users - */ -async function checkSubscriptionRelationship( - user1: { - consultantProfileId: string | null; - consulteeProfileId: string | null; - }, - user2: { - consultantProfileId: string | null; - consulteeProfileId: string | null; - }, -): Promise { - if (!user1.consultantProfileId && !user1.consulteeProfileId) return false; - if (!user2.consultantProfileId && !user2.consulteeProfileId) return false; - - const checks: Promise[] = []; - - if (user1.consultantProfileId && user2.consulteeProfileId) { - checks.push( - prisma.subscription - .findFirst({ - where: { - subscriptionPlan: { - consultantProfileId: user1.consultantProfileId, - }, - requestedById: user2.consulteeProfileId, - status: { in: ["APPROVED", "SCHEDULED"] }, - schedulingPeriodEndsAt: { gte: new Date() }, - }, - select: { id: true }, - }) - .then((r) => !!r), - ); - } - - if (user2.consultantProfileId && user1.consulteeProfileId) { - checks.push( - prisma.subscription - .findFirst({ - where: { - subscriptionPlan: { - consultantProfileId: user2.consultantProfileId, - }, - requestedById: user1.consulteeProfileId, - status: { in: ["APPROVED", "SCHEDULED"] }, - schedulingPeriodEndsAt: { gte: new Date() }, - }, - select: { id: true }, - }) - .then((r) => !!r), - ); - } - - if (checks.length === 0) return false; - - const results = await Promise.all(checks); - return results.some(Boolean); -} - -/** - * Check if users share any appointments (webinars, classes) - */ -async function checkSharedAppointments( - userId1: string, - userId2: string, -): Promise { - const sharedSlot = await prisma.slotOfAppointment.findFirst({ - where: { - user: { some: { id: userId1 } }, - AND: { user: { some: { id: userId2 } } }, - }, - select: { id: true }, - }); - - return !!sharedSlot; -} - -/** - * @deprecated Use searchUsersWithRelationships instead — this performs a global - * unscoped search that exposes PII of arbitrary users. - * @param searchTerm The term to search for (name or email) - * @returns The users that match the search term - */ -export const searchUsers = async (searchTerm: string) => { - const validatedTerm = searchTermSchema.parse(searchTerm.trim()); - - try { - const users = await prisma.user.findMany({ - where: { - AND: [ - { - NOT: [ - { id: { startsWith: "recording-egress-" } }, - { id: { startsWith: "system-" } }, - ], - }, - { - OR: [ - { name: { contains: validatedTerm, mode: "insensitive" } }, - { email: { contains: validatedTerm, mode: "insensitive" } }, - ], - }, - ], - }, - select: { - id: true, - name: true, - email: true, - image: true, - role: true, - }, - take: 10, - orderBy: [{ name: "asc" }], - }); - - streamLogger.debug("Legacy user search completed", { - term: validatedTerm, - resultCount: users.length, - }); - - return users; - } catch (error) { - Sentry.captureException(error instanceof Error ? error : new Error(String(error)), { tags: { subsystem: "stream" } }); - streamLogger.error("Legacy user search failed", error, { - searchTerm: validatedTerm, - }); - throw error; - } -}; diff --git a/app/api/stream/channels/open/route.ts b/app/api/stream/channels/open/route.ts new file mode 100644 index 000000000..d90850ce9 --- /dev/null +++ b/app/api/stream/channels/open/route.ts @@ -0,0 +1,331 @@ +/** + * Resolve-or-create the channel behind a search result, server-side. + * + * ## Why this route exists + * + * `ChannelSearch` used to open a result by calling + * `client.channel(type, id).watch()` on an id the browser had computed. In + * `stream-chat`, `watch()` posts to the channel **query** endpoint — the same + * endpoint `create()` posts to; `channel.create()` is literally + * `query({ created_by_id })`. So `watch()` on an id that does not exist yet + * CREATES it. Created that way, with no `members` array, the caller becomes + * `created_by` and is *not* a member. + * + * That one behaviour produced every symptom of the reported bug at once: the + * header showed the raw `dm-…` id (channelUtils has no branch for a DM with + * zero counterparties), it said "No members", the message sent fine, and the + * thread vanished on reload because the sidebar queries + * `{ members: { $in: [me] } }`. It was reported as "I can talk to myself" but + * reproduces identically against a stranger — the phantom channel is not a + * property of the pair, it is a property of the id not existing. + * + * The id did not exist because search matches `APPROVED_PENDING_PAYMENT` and + * `COMPLETED` bookings, while channel creation only ever fired at approval and + * payment-success. Widening `DM_ELIGIBLE_STATUSES` fixes the *set*; this route + * fixes the *mechanism*, so a future gap cannot be papered over by the client + * inventing a channel. + * + * ## Contract + * + * The client sends WHO or WHAT it wants to talk to, never a channel id. The id + * is re-derived here from the caller's session plus the target. A client- + * supplied channel id would be an authorization bypass by construction: the id + * is a pure function of the two user ids, so anyone able to name a pair could + * name their channel. + * + * Both arms are idempotent — an existing channel is returned untouched. + */ +import { NextRequest, NextResponse } from "next/server"; +import { z } from "zod"; +import * as Sentry from "@sentry/nextjs"; + +import prisma from "@/lib/prisma"; +import { requireApiAuth } from "@/lib/auth-helpers"; +import { CLASS_PREFIX, WEBINAR_PREFIX } from "@/lib/stream-channel-ids"; +import { + canDirectMessage, + DmNotPermittedError, + pairBookingContexts, +} from "@/lib/stream/dm-eligibility"; +import { + DM_ELIGIBLE_STATUSES, + OPENABLE_EVENT_STATUSES, +} from "@/lib/stream/dm-eligibility-statuses"; +import { DEFAULT_RETENTION_DAYS, isPastRetention } from "@/lib/stream/channel-lifecycle"; +import { applyRateLimit, streamApiLimiter } from "@/lib/rate-limit"; +import { createDirectMessageChannel } from "@/actions/stream/chat/channel.action"; +import { addUserToEventChannel } from "@/actions/stream/chat/event-channel.action"; +import { streamLogger } from "@/lib/stream-logger"; + +const bodySchema = z.discriminatedUnion("kind", [ + z.object({ + kind: z.literal("dm"), + counterpartyUserId: z.string().min(1), + /** Funding context. Absent or null = personal. */ + organizationId: z.string().min(1).nullable().optional(), + }), + z.object({ + kind: z.literal("event"), + eventType: z.enum(["webinar", "class"]), + eventId: z.string().min(1), + }), +]); + +/** + * Is the caller a participant in this event — an attendee on one of its slots, + * or the host consultant? + * + * Deliberately NOT `authorizeEventAccess` from lib/auth-helpers: for webinars + * and classes that helper authorizes the plan owner and ACCEPTED collaborators + * only, and returns 403 for attendees. Attendees are exactly who needs the + * event chat. This mirrors the predicate the search route already applies, so + * the two cannot disagree about which rows are clickable. + * + * The retention guard (second query) is F-HIGH-2's other half: dev's fix keeps + * past-retention events out of the sync expected-set, but create-on-miss here + * would resurrect the hard-deleted channel anyway — writable until the expire + * cron's next pass re-freezes it. An event whose last slot ended more than + * `retentionDays` ago is not openable, full stop. + */ +async function isEventParticipant( + eventType: "webinar" | "class", + eventId: string, + userId: string, +): Promise { + if (eventType === "webinar") { + const hit = await prisma.webinar.findFirst({ + where: { + id: eventId, + status: { in: [...OPENABLE_EVENT_STATUSES] }, + OR: [ + { + appointment: { + deletedAt: null, + slotsOfAppointment: { + some: { deletedAt: null, user: { some: { id: userId } } }, + }, + }, + }, + { webinarPlan: { consultantProfile: { userId } } }, + ], + }, + select: { + id: true, + appointment: { + select: { + organization: { select: { streamRecordingRetentionDays: true } }, + slotsOfAppointment: { + orderBy: { endsAt: "desc" }, + take: 1, + select: { endsAt: true }, + }, + }, + }, + }, + }); + if (!hit) return false; + return !isPastRetention( + hit.appointment?.slotsOfAppointment[0]?.endsAt ?? null, + hit.appointment?.organization?.streamRecordingRetentionDays ?? + DEFAULT_RETENTION_DAYS, + ); + } + + const hit = await prisma.class.findFirst({ + where: { + id: eventId, + status: { in: [...OPENABLE_EVENT_STATUSES] }, + OR: [ + { + appointments: { + some: { + deletedAt: null, + slotsOfAppointment: { + some: { deletedAt: null, user: { some: { id: userId } } }, + }, + }, + }, + }, + { classPlan: { consultantProfile: { userId } } }, + ], + }, + select: { + id: true, + appointments: { + // A class spans one appointment per cohort but ONE channel; age is the + // latest end across cohorts, carrying that cohort's org dial — same + // collapse rule as the expire cron. Each appointment contributes only + // its own latest slot (orderBy+take below), so this stays one row per + // cohort. + select: { + organization: { select: { streamRecordingRetentionDays: true } }, + slotsOfAppointment: { + orderBy: { endsAt: "desc" }, + take: 1, + select: { endsAt: true }, + }, + }, + }, + }, + }); + if (!hit) return false; + const latestCohort = hit.appointments.reduce< + | { + endsAt: Date; + retentionDays: number; + } + | null + >((latest, apt) => { + const aptLatest = apt.slotsOfAppointment[0]?.endsAt; + if (!aptLatest) return latest; + const retentionDays = + apt.organization?.streamRecordingRetentionDays ?? DEFAULT_RETENTION_DAYS; + if (!latest || aptLatest > latest.endsAt) { + return { endsAt: aptLatest, retentionDays }; + } + return latest; + }, null); + return !isPastRetention( + latestCohort?.endsAt ?? null, + latestCohort?.retentionDays ?? DEFAULT_RETENTION_DAYS, + ); +} + +export async function POST(request: NextRequest) { + const auth = await requireApiAuth(); + if (auth.error) return auth.error; + const userId = auth.session.user.id; + + // Keyed on the user, after auth, before any Prisma or Stream work. This route + // is cheap to call and expensive to serve — an eligibility check plus a Stream + // create — and `streamApiLimiter` already existed for exactly this and had no + // callers. Route-slugged, per the helper's own guidance on sharing a limiter. + const limited = await applyRateLimit(streamApiLimiter, `open:${userId}`); + if (limited) return limited; + + let body: z.infer; + try { + body = bodySchema.parse(await request.json()); + } catch { + return NextResponse.json( + { error: "Invalid request body" }, + { status: 400 }, + ); + } + + try { + if (body.kind === "dm") { + const { counterpartyUserId } = body; + const requestedOrgId = body.organizationId ?? null; + + // Covers the self case too — `canDirectMessage` returns false for + // `a === b` before it touches the database. + if (!(await canDirectMessage(userId, counterpartyUserId))) { + streamLogger.warn("Refused DM open — no booking link", { + userId, + counterpartyUserId, + }); + return NextResponse.json( + { + error: + "Direct messages are only available between people who share a booking.", + eligibleStatuses: DM_ELIGIBLE_STATUSES, + }, + { status: 403 }, + ); + } + + // Funding-context forgery guard. The channel id is re-derived + // server-side, but it is a function of the pair AND the funding context — + // so an org id accepted unchecked would let anyone mint + // `dmo--…` channels tagged to an organization they + // have no relation to, and omitting it for an org-funded booking would + // mint a personal-id channel the reconciler immediately classifies stale. + // The allowed contexts come from the same rows (and the same + // `bookingOrgId` precedence) the reconciler's expected-set is built from. + const contexts = await pairBookingContexts(userId, counterpartyUserId); + let organizationId: string | null; + if (requestedOrgId !== null) { + if (!contexts.organizations.includes(requestedOrgId)) { + return NextResponse.json( + { + error: + "No booking ties this conversation to that organization.", + }, + { status: 403 }, + ); + } + organizationId = requestedOrgId; + } else if (contexts.personalAllowed) { + organizationId = null; + } else if (contexts.organizations.length === 1) { + // Personal context requested, but every eligible booking is org-funded: + // deriving the single real context here instead of minting a channel + // the reconciler would evict on the next sync. + organizationId = contexts.organizations[0]; + } else { + return NextResponse.json( + { + error: + "This conversation exists in multiple organizations — specify which one.", + }, + { status: 400 }, + ); + } + + // Idempotent: Stream's create is an upsert for an existing id, and the + // member list is passed atomically so the pair is always both members — + // which is the whole difference from what `watch()` was doing. + // + // The returned `channelId` is used rather than re-deriving it with + // `getDmChannelId`. Same inputs, same helper, so the two agreed — but + // deriving an id twice is two chances to derive it differently, and this + // codebase has already lost conversation history once to exactly that + // (#1134 P0-3, the `localeCompare` re-keying). One derivation, one source. + const { channelId } = await createDirectMessageChannel( + userId, + counterpartyUserId, + organizationId, + ); + + return NextResponse.json({ channelType: "messaging", channelId }); + } + + const { eventType, eventId } = body; + if (!(await isEventParticipant(eventType, eventId, userId))) { + return NextResponse.json( + { error: "You are not a participant in this event." }, + { status: 403 }, + ); + } + + // Creates the channel with the full roster if absent, adds the caller if + // present. Also idempotent. + await addUserToEventChannel(eventType, eventId, userId); + + const channelId = + eventType === "webinar" + ? `${WEBINAR_PREFIX}${eventId}` + : `${CLASS_PREFIX}${eventId}`; + return NextResponse.json({ channelType: "team", channelId }); + } catch (error) { + // A refusal is an answer, not an incident. This is currently unreachable — + // the DM branch checks `canDirectMessage` before calling — but + // `createDirectMessageChannel` asserts eligibility itself, so a future + // caller, or a booking cancelled between the check and the create, would + // otherwise page someone at 3am for a gate doing its job. + if (error instanceof DmNotPermittedError) { + return NextResponse.json({ error: error.message }, { status: 403 }); + } + + Sentry.captureException( + error instanceof Error ? error : new Error(String(error)), + { tags: { subsystem: "stream" } }, + ); + streamLogger.error("Failed to open channel", error, { userId }); + return NextResponse.json( + { error: "Failed to open conversation" }, + { status: 500 }, + ); + } +} diff --git a/app/api/stream/channels/search-appointments/route.ts b/app/api/stream/channels/search-appointments/route.ts index 34754f85a..c83a55ce1 100644 --- a/app/api/stream/channels/search-appointments/route.ts +++ b/app/api/stream/channels/search-appointments/route.ts @@ -3,11 +3,42 @@ import * as Sentry from "@sentry/nextjs"; import prisma from "lib/prisma"; import { getSession } from "@/lib/auth-server"; import { bookingOrgId, getDmChannelId } from "@/lib/stream-utils"; +import { + dmEligibleStatusFilter, + OPENABLE_EVENT_STATUSES, +} from "@/lib/stream/dm-eligibility-statuses"; import { AppointmentSearchResultSchema, type AppointmentSearchResult, } from "@/schemas/stream-search"; +/** + * The OTHER party, relative to whoever is signed in. + * + * Every row used to be labelled with the consultant, unconditionally. That is + * right on a consultee's dashboard and wrong on a consultant's, where it names + * the viewer — so a consultant searching two letters of their own name got back + * what looked like a conversation with themselves, subtitled with the booking + * kinds they hold. The channel id underneath was always correct and always + * pointed at the real consultee; only the label lied. + */ +function resolveCounterparty( + viewerUserId: string, + consultant: { id: string; name: string | null; image: string | null }, + consultee: { id: string; name: string | null; image: string | null }, +): { + counterpartyUserId: string; + counterpartyName: string; + counterpartyImage?: string; +} { + const other = consultant.id === viewerUserId ? consultee : consultant; + return { + counterpartyUserId: other.id, + counterpartyName: other.name || "Unknown", + counterpartyImage: other.image || undefined, + }; +} + export async function GET(request: NextRequest) { try { const session = await getSession(); @@ -29,7 +60,18 @@ export async function GET(request: NextRequest) { const results: AppointmentSearchResult[] = []; // Search Consultations (by plan title OR consultant name) - const consultations = await prisma.consultation.findMany({ + // The four searches are independent — none reads another's result — so they + // are issued together and awaited once. Sequential awaits made every + // keystroke pay the SUM of four round-trips to a remote database; this pays + // the slowest one. + // + // Each query also gains a deterministic `orderBy`. With `take: 10` and no + // ordering, Postgres returns an arbitrary ten of the matching rows and the + // `slice(0, 20)` below then cuts a set that can differ between two identical + // requests — the same search, run twice, returning different people. + // Newest-first is both stable and the more useful order. + + const consultationsPromise = prisma.consultation.findMany({ where: { AND: [ { @@ -56,16 +98,27 @@ export async function GET(request: NextRequest) { }, }, }, + // Search by consultee name. Absent until now, so a consultant + // could not find a conversation by their own client's name — + // only by their own name or the plan title. On a consultant's + // dashboard the consultant arm above matches THEMSELVES, which is + // why searching a few letters of their own name returned a row + // that looked like a self-conversation. + { + requestedBy: { + user: { + name: { + contains: query, + mode: "insensitive", + }, + }, + }, + }, ], }, { status: { - in: [ - "APPROVED", - "APPROVED_PENDING_PAYMENT", - "SCHEDULED", - "COMPLETED", - ], + ...dmEligibleStatusFilter(), }, }, { @@ -107,41 +160,18 @@ export async function GET(request: NextRequest) { requestedBy: { include: { user: { - select: { id: true }, + select: { id: true, name: true, image: true }, }, }, }, // Needed to resolve which DM thread this hit belongs to. appointment: { select: { organizationId: true } }, }, + orderBy: [{ createdAt: "desc" }, { id: "asc" }], take: 10, }); - for (const consultation of consultations) { - results.push({ - id: consultation.id, - type: "consultation", - name: consultation.consultationPlan.title, - consultantName: - consultation.consultationPlan.consultantProfile.user.name || - "Unknown", - consultantImage: - consultation.consultationPlan.consultantProfile.user.image || - undefined, - // Funding context is part of the DM key, so a hit must resolve to the - // SAME channel the creator made. Shared resolver, because reading only - // the appointment sent org-hosted-plan bookings to a personal channel - // that was never created — clicking the result opened an empty thread. - channelId: getDmChannelId( - consultation.consultationPlan.consultantProfile.user.id, - consultation.requestedBy.user.id, - bookingOrgId(consultation), - ), - }); - } - - // Search Subscriptions (by plan title OR consultant name) - const subscriptions = await prisma.subscription.findMany({ + const subscriptionsPromise = prisma.subscription.findMany({ where: { AND: [ { @@ -168,16 +198,22 @@ export async function GET(request: NextRequest) { }, }, }, + // Search by consultee name — see the consultation block. + { + requestedBy: { + user: { + name: { + contains: query, + mode: "insensitive", + }, + }, + }, + }, ], }, { status: { - in: [ - "APPROVED", - "APPROVED_PENDING_PAYMENT", - "SCHEDULED", - "COMPLETED", - ], + ...dmEligibleStatusFilter(), }, }, { @@ -219,7 +255,7 @@ export async function GET(request: NextRequest) { requestedBy: { include: { user: { - select: { id: true }, + select: { id: true, name: true, image: true }, }, }, }, @@ -238,31 +274,11 @@ export async function GET(request: NextRequest) { take: 1, }, }, + orderBy: [{ createdAt: "desc" }, { id: "asc" }], take: 10, }); - for (const subscription of subscriptions) { - results.push({ - id: subscription.id, - type: "subscription", - name: subscription.subscriptionPlan.title, - consultantName: - subscription.subscriptionPlan.consultantProfile.user.name || - "Unknown", - consultantImage: - subscription.subscriptionPlan.consultantProfile.user.image || - undefined, - // Same resolver as createSubscriptionChannel. - channelId: getDmChannelId( - subscription.subscriptionPlan.consultantProfile.user.id, - subscription.requestedBy.user.id, - bookingOrgId(subscription), - ), - }); - } - - // Search Webinars - const webinars = await prisma.webinar.findMany({ + const webinarsPromise = prisma.webinar.findMany({ where: { AND: [ { @@ -275,7 +291,7 @@ export async function GET(request: NextRequest) { }, { status: { - in: ["SCHEDULED", "IN_PROGRESS", "COMPLETED"], + in: [...OPENABLE_EVENT_STATUSES], }, }, { @@ -316,26 +332,11 @@ export async function GET(request: NextRequest) { }, }, }, + orderBy: [{ createdAt: "desc" }, { id: "asc" }], take: 10, }); - for (const webinar of webinars) { - if (webinar.webinarPlan.consultantProfile) { - results.push({ - id: webinar.id, - type: "webinar", - name: webinar.webinarPlan.title, - consultantName: - webinar.webinarPlan.consultantProfile.user.name || "Unknown", - consultantImage: - webinar.webinarPlan.consultantProfile.user.image || undefined, - channelId: `webinar-${webinar.id}`, - }); - } - } - - // Search Classes - const classes = await prisma.class.findMany({ + const classesPromise = prisma.class.findMany({ where: { AND: [ { @@ -348,7 +349,7 @@ export async function GET(request: NextRequest) { }, { status: { - in: ["SCHEDULED", "IN_PROGRESS", "COMPLETED"], + in: [...OPENABLE_EVENT_STATUSES], }, }, { @@ -391,18 +392,111 @@ export async function GET(request: NextRequest) { }, }, }, + orderBy: [{ createdAt: "desc" }, { id: "asc" }], take: 10, }); + const [consultations, subscriptions, webinars, classes] = await Promise.all( + [ + consultationsPromise, + subscriptionsPromise, + webinarsPromise, + classesPromise, + ], + ); + + for (const consultation of consultations) { + // One bad row must not take the whole search down. + // + // `getDmChannelId` throws on a self-pair — deliberately, because a + // one-member DM is unusable and silently collapsing it was the original + // bug. But it is called here inside a loop over query results, so a + // single legacy self-booked row (checkout blocks these now; seeded and + // pre-guard data may still hold them) throws past this handler and the + // outer catch answers 500 for the ENTIRE search. The user sees "no + // results" for every query that happens to match that row. + if ( + consultation.consultationPlan.consultantProfile.user.id === + consultation.requestedBy.user.id + ) { + continue; + } + const organizationId = bookingOrgId(consultation); + results.push({ + id: consultation.id, + type: "consultation", + name: consultation.consultationPlan.title, + ...resolveCounterparty( + userId, + consultation.consultationPlan.consultantProfile.user, + consultation.requestedBy.user, + ), + organizationId, + // Funding context is part of the DM key, so a hit must resolve to the + // SAME channel the creator made. Shared resolver, because reading only + // the appointment sent org-hosted-plan bookings to a personal channel + // that was never created — clicking the result opened an empty thread. + channelId: getDmChannelId( + consultation.consultationPlan.consultantProfile.user.id, + consultation.requestedBy.user.id, + organizationId, + ), + }); + } + + for (const subscription of subscriptions) { + // Same self-pair guard as the consultation loop above. + if ( + subscription.subscriptionPlan.consultantProfile.user.id === + subscription.requestedBy.user.id + ) { + continue; + } + const organizationId = bookingOrgId(subscription); + results.push({ + id: subscription.id, + type: "subscription", + name: subscription.subscriptionPlan.title, + ...resolveCounterparty( + userId, + subscription.subscriptionPlan.consultantProfile.user, + subscription.requestedBy.user, + ), + organizationId, + // Same resolver as createSubscriptionChannel. + channelId: getDmChannelId( + subscription.subscriptionPlan.consultantProfile.user.id, + subscription.requestedBy.user.id, + organizationId, + ), + }); + } + + for (const webinar of webinars) { + if (webinar.webinarPlan.consultantProfile) { + results.push({ + id: webinar.id, + type: "webinar", + name: webinar.webinarPlan.title, + // A group event has no single counterparty, so the host stands in. + counterpartyName: + webinar.webinarPlan.consultantProfile.user.name || "Unknown", + counterpartyImage: + webinar.webinarPlan.consultantProfile.user.image || undefined, + channelId: `webinar-${webinar.id}`, + }); + } + } + for (const classItem of classes) { if (classItem.classPlan.consultantProfile) { results.push({ id: classItem.id, type: "class", name: classItem.classPlan.title, - consultantName: + counterpartyName: classItem.classPlan.consultantProfile.user.name || "Unknown", - consultantImage: + counterpartyImage: classItem.classPlan.consultantProfile.user.image || undefined, channelId: `class-${classItem.id}`, }); diff --git a/app/api/stream/search-consultees/route.ts b/app/api/stream/search-consultees/route.ts index fd6532400..551b4fb3c 100644 --- a/app/api/stream/search-consultees/route.ts +++ b/app/api/stream/search-consultees/route.ts @@ -3,6 +3,7 @@ import { NextRequest, NextResponse } from "next/server"; import prisma from "lib/prisma"; import { getSession } from "@/lib/auth-server"; +import { dmEligibleStatusFilter } from "@/lib/stream/dm-eligibility-statuses"; // See schemas/stream-search.ts for why the shape does not live here. import { ConsulteeSearchResultSchema, @@ -43,7 +44,14 @@ export async function GET(req: NextRequest) { // Get all consultees from different relationship types const results: ConsulteeSearchResult[] = []; - const seenUserIds = new Set(excludeIds); + // Seeded with the caller as well as the caller-supplied exclusions. A user + // holding BOTH profiles — a consultant who also books sessions — appears in + // their own relationship queries the moment they are `requestedBy` on one of + // their own plans or share a slot on their own event, and the only previous + // filter was `excludeIds` from the query string plus the dialog's existing + // members. So they could add themselves to a channel and end up as their own + // counterparty. Self-exclusion belongs here, not in the caller. + const seenUserIds = new Set([...excludeIds, session.user.id]); // 1. Get consultees from active consultations const consultations = await prisma.consultation.findMany({ @@ -52,12 +60,7 @@ export async function GET(req: NextRequest) { consultantProfileId: consultantProfileId, }, status: { - in: [ - "APPROVED", - "APPROVED_PENDING_PAYMENT", - "SCHEDULED", - "COMPLETED", - ], + ...dmEligibleStatusFilter(), }, }, include: { @@ -103,12 +106,7 @@ export async function GET(req: NextRequest) { consultantProfileId: consultantProfileId, }, status: { - in: [ - "APPROVED", - "APPROVED_PENDING_PAYMENT", - "SCHEDULED", - "COMPLETED", - ], + ...dmEligibleStatusFilter(), }, }, include: { diff --git a/app/api/stream/search/route.ts b/app/api/stream/search/route.ts index ab96bdcce..9748579a8 100644 --- a/app/api/stream/search/route.ts +++ b/app/api/stream/search/route.ts @@ -30,11 +30,10 @@ export async function GET(req: NextRequest) { streamLogger.debug("Searching users", { searchTerm }); - // Always use relationship-scoped search to prevent global user enumeration - const users = await searchUsersWithRelationships( - searchTerm, - session.user.id, - ); + // Always use relationship-scoped search to prevent global user enumeration. + // The action now derives identity from the session itself — the second + // argument it used to take was a client-controlled impersonation handle. + const users = await searchUsersWithRelationships(searchTerm); streamLogger.debug("Search results", { count: users.length }); diff --git a/app/api/stream/users/block/route.ts b/app/api/stream/users/block/route.ts index 63a24fe6c..111d9e49f 100644 --- a/app/api/stream/users/block/route.ts +++ b/app/api/stream/users/block/route.ts @@ -4,7 +4,6 @@ import { getSession } from "@/lib/auth-server"; import { getStreamChatClient } from "@/lib/stream-client"; import { streamLogger } from "@/lib/stream-logger"; import prisma from "@/lib/prisma"; -import { getDmChannelId } from "@/lib/stream-utils"; export async function POST(req: NextRequest) { try { @@ -43,29 +42,98 @@ export async function POST(req: NextRequest) { // Verify the blocker has an existing DM channel with the target // (prevents arbitrary users from banning others they've never interacted with) + // + // Found by QUERY, not by deriving one id. This used to call + // `getDmChannelId(me, target)` with no org argument, which only ever names + // the PERSONAL `dm-` channel — so a pair whose conversation lives in an + // org context (`dmo-…`) had no such channel, the existence probe threw, and + // blocking answered 403 "you can only block users you have a conversation + // with" to two people mid-conversation. + // + // `members: { $eq: [a, b] }` is Stream's documented way to find a 1:1: it + // matches channels whose membership is exactly that pair, order-independent, + // which catches all three id forms (`dm-`, `dmo-`, `dmh-`) without this + // route having to know they exist. const chatClient = getStreamChatClient(); - const dmChannelId = getDmChannelId(session.user.id, targetUserId); - const dmChannel = chatClient.channel("messaging", dmChannelId); + + // `limit: 30` is Stream's real per-call cap for queryChannels regardless of + // what you pass, and it is not a constraint here: the filter matches + // channels whose membership is EXACTLY this pair, which is one personal + // thread plus at most one per organization, and `lib/auth.ts` caps a user at + // `organizationLimit: 5`. Six is the ceiling. No pagination needed. + let dmChannels: Awaited>; try { - const state = await dmChannel.query({ members: { limit: 0 } }); - if (!state.channel) { - return NextResponse.json( - { error: "You can only block users you have a conversation with" }, - { status: 403 }, - ); - } - } catch { + dmChannels = await chatClient.queryChannels( + { + type: "messaging", + members: { $eq: [session.user.id, targetUserId] }, + }, + { last_message_at: -1 }, + { limit: 30 }, + ); + } catch (queryError) { + // 5xx, NOT the 403 below. An earlier version assigned `[]` here and fell + // through, so a Stream outage told two people mid-conversation that they + // "can only block users you have a conversation with" — an infrastructure + // failure reported as a policy decision, and the one moment when someone + // reaching for the block button most needs it to work. + Sentry.captureException( + queryError instanceof Error + ? queryError + : new Error(String(queryError)), + { tags: { subsystem: "stream" } }, + ); + streamLogger.error("Block: DM channel lookup failed", queryError, { + userId: session.user.id, + targetUserId, + }); + return NextResponse.json( + { error: "Could not block this user right now. Please try again." }, + { status: 503 }, + ); + } + + if (dmChannels.length === 0) { return NextResponse.json( { error: "You can only block users you have a conversation with" }, { status: 403 }, ); } - // Ban the user in this specific channel (scoped, not global) - await dmChannel.banUser(targetUserId, { - banned_by_id: session.user.id, - reason: "user_block", - }); + // Ban in every shared DM, not just the one they happen to be looking at. + // A block is about the person; leaving their org thread writable while the + // personal one is banned would be a block that does not block. + // + // `allSettled`, not a sequential loop: the loop aborted on the first + // rejection, so a transient failure on the personal thread left the org + // thread unbanned AND skipped the moderation report — the worst of the three + // possible outcomes. Now every channel is attempted, and the block stands as + // long as one succeeded. + const banResults = await Promise.allSettled( + dmChannels.map((dmChannel) => + dmChannel.banUser(targetUserId, { + banned_by_id: session.user.id, + reason: "user_block", + }), + ), + ); + + const failures = banResults.filter((r) => r.status === "rejected"); + if (failures.length === dmChannels.length) { + Sentry.captureException(new Error("Block: every channel ban failed"), { + tags: { subsystem: "stream" }, + }); + streamLogger.error("Block: every channel ban failed", failures[0], { + userId: session.user.id, + targetUserId, + channelCount: dmChannels.length, + }); + return NextResponse.json( + { error: "Could not block this user right now. Please try again." }, + { status: 503 }, + ); + } + const blockedCount = dmChannels.length - failures.length; // Create a moderation report in our DB await prisma.moderationReport.create({ @@ -78,9 +146,58 @@ export async function POST(req: NextRequest) { }, }); + // A partial block is not a block. Reported AFTER the moderation report is + // written, because the attempt genuinely happened and the audit trail should + // record it — but the caller must not be told the person can no longer + // message them while one shared thread is still writable. The UI branches on + // `response.ok` alone, so a 2xx here would render "This user can no longer + // message you" over a channel they can still post in. + if (failures.length > 0) { + // The actual rejection, not just a synthetic marker. The all-fail branch + // above passes `failures[0]`; this one discarded it and reported counts + // only, so the alert said a partial block happened without saying why — + // which is the one thing you need to know to act on it. + const firstReason = + failures[0]?.status === "rejected" ? failures[0].reason : undefined; + Sentry.captureException( + firstReason instanceof Error + ? firstReason + : new Error("Block: some channel bans failed"), + { + tags: { subsystem: "stream" }, + extra: { + blocked: blockedCount, + failed: failures.length, + total: dmChannels.length, + }, + }, + ); + streamLogger.error("Block: some channel bans failed", firstReason, { + userId: session.user.id, + targetUserId, + blocked: blockedCount, + failed: failures.length, + total: dmChannels.length, + }); + + return NextResponse.json( + { + success: false, + partial: true, + blocked: blockedCount, + total: dmChannels.length, + error: + `Blocked in ${blockedCount} of ${dmChannels.length} conversations. ` + + "Please try again to block the rest.", + }, + { status: 502 }, + ); + } + streamLogger.info("User blocked", { blockedBy: session.user.id, targetUserId, + channelCount: dmChannels.length, }); return NextResponse.json({ @@ -88,7 +205,10 @@ export async function POST(req: NextRequest) { message: "User blocked successfully", }); } catch (error) { - Sentry.captureException(error instanceof Error ? error : new Error(String(error)), { tags: { subsystem: "stream" } }); + Sentry.captureException( + error instanceof Error ? error : new Error(String(error)), + { tags: { subsystem: "stream" } }, + ); streamLogger.error("Failed to block user", error); return NextResponse.json( { error: "Failed to block user" }, diff --git a/components/chat/AddMembersDialog.tsx b/components/chat/AddMembersDialog.tsx index b24cdc7e4..0b4b8735c 100644 --- a/components/chat/AddMembersDialog.tsx +++ b/components/chat/AddMembersDialog.tsx @@ -1,6 +1,7 @@ "use client"; -import { useState, useEffect, useCallback } from "react"; +import { useState, useEffect, useCallback, useRef } from "react"; +import { useDebouncedCallback } from "use-debounce"; import Image from "next/image"; import { ResponsiveModal, @@ -48,44 +49,96 @@ export const AddMembersDialog = ({ } }, [open]); - const searchConsultees = useCallback(async () => { - setIsSearching(true); - setHasSearched(true); + /** + * Same latest-wins guard and cancellation as `ChannelSearch`, for the same + * reason: this had a hand-rolled `setTimeout`, no `AbortController` and an + * unconditional `setConsultees(...)`, so whichever response landed last won + * regardless of which term it answered. Modelled on + * `hooks/scheduling/useCalendarData.ts`. + * + * Unlike ChannelSearch there is deliberately NO minimum-length guard. An + * empty term is a real query here — it lists everyone the consultant may add + * — and gating it would leave the dialog blank until you typed. + */ + const requestIdRef = useRef(0); + const abortRef = useRef(null); - try { - const excludeParam = existingMemberIds.join(","); - const response = await fetch( - `/api/stream/search-consultees?term=${encodeURIComponent(searchTerm)}&exclude=${excludeParam}`, - ); + // A primitive, so the debounced callback's identity does not change on every + // render the way it did with the `existingMemberIds` array in the deps — + // which re-ran the debounce effect continuously and re-armed the timer. + const excludeParam = existingMemberIds.join(","); + + const searchConsultees = useCallback( + async (term: string, exclude: string) => { + abortRef.current?.abort(); + const controller = new AbortController(); + abortRef.current = controller; + const requestId = ++requestIdRef.current; + + setIsSearching(true); + setHasSearched(true); + + try { + const response = await fetch( + // Both halves encoded. `term` was, `exclude` was not — so a member id + // containing `&`, `+`, `#` or a space truncated or corrupted the + // exclusion set, and people already in the channel reappeared as + // addable. + `/api/stream/search-consultees?term=${encodeURIComponent(term)}&exclude=${encodeURIComponent(exclude)}`, + { signal: controller.signal }, + ); + + if (!response.ok) { + throw new Error("Failed to search consultees"); + } - if (!response.ok) { - throw new Error("Failed to search consultees"); + const data = await response.json(); + if (requestId !== requestIdRef.current) return; + setConsultees(data.consultees || []); + } catch (error) { + // Our own cancellation, not a failure — and surfacing it as a toast + // would fire one per keystroke. + if (error instanceof DOMException && error.name === "AbortError") + return; + if (requestId !== requestIdRef.current) return; + console.error("Error searching consultees:", error); + toast({ + title: "Error", + description: "Failed to search consultees", + variant: "destructive", + }); + } finally { + if (requestId === requestIdRef.current) setIsSearching(false); } + }, + [toast], + ); - const data = await response.json(); - setConsultees(data.consultees || []); - } catch (error) { - console.error("Error searching consultees:", error); - toast({ - title: "Error", - description: "Failed to search consultees", - variant: "destructive", - }); - } finally { - setIsSearching(false); - } - }, [searchTerm, existingMemberIds, toast]); + const debouncedSearch = useDebouncedCallback(searchConsultees, 300); - // Debounced search useEffect(() => { if (!open) return; + debouncedSearch(searchTerm, excludeParam); + }, [searchTerm, excludeParam, open, debouncedSearch]); - const timeoutId = setTimeout(() => { - searchConsultees(); - }, 300); + // Abort on close as well as unmount: the dialog resets its state when it + // closes, and a late response would repopulate the list behind it. + useEffect(() => { + if (!open) { + debouncedSearch.cancel(); + abortRef.current?.abort(); + } + }, [open, debouncedSearch]); - return () => clearTimeout(timeoutId); - }, [searchTerm, open, searchConsultees]); + useEffect( + () => () => { + // See ChannelSearch — use-debounce v10 leaves trailing callbacks armed + // across unmount. + debouncedSearch.cancel(); + abortRef.current?.abort(); + }, + [debouncedSearch], + ); const toggleSelection = (userId: string) => { setSelectedIds((prev) => { diff --git a/components/chat/ChannelInfoAndManageDialog.tsx b/components/chat/ChannelInfoAndManageDialog.tsx index 0bfc7687f..f6e0f051f 100644 --- a/components/chat/ChannelInfoAndManageDialog.tsx +++ b/components/chat/ChannelInfoAndManageDialog.tsx @@ -45,6 +45,7 @@ import { useChatContext } from "stream-chat-react"; import { getChannelDisplayInfo } from "./utils/channelUtils"; import { AddMembersDialog } from "./AddMembersDialog"; import { isEventChannel } from "@/lib/stream-channel-ids"; +import { addMemberToChannel } from "@/actions/stream/chat/member.action"; interface ChannelMember { id: string; @@ -89,11 +90,26 @@ export const ChannelInfoAndManageDialog = ({ const displayName = displayInfo.displayName; - // Check if current user is the event owner consultant - const isEventOwner = - isEvent && - channel.data?.created_by_id === client?.userID && - client?.user?.role === "CONSULTANT"; + // Check if current user is the event owner consultant. + // + // The `client.user.role === "CONSULTANT"` conjunct that used to be here could + // never be true: `client.user.role` is the STREAM role, and `mapRoleToStream` + // collapses every non-staff account — consultants included — to `"user"`. + // So the whole predicate was constantly false and the host's remove-member + // control never rendered. ChatSidebar carries a comment warning about exactly + // this trap. + // + // Dropping the conjunct rather than swapping in the app role: for an event + // channel, `created_by_id` IS the host consultant (channel.action.ts sets it + // from the plan's consultantProfile), so the ownership test already implies + // the role. Adding a second source of truth would only create a way for the + // two to disagree. + // Queried channels expose the creator as `created_by` (object); + // `created_by_id` survives only on channels created in THIS session. Read + // both, prefer the reliable one. + const creatorId = + channel.data?.created_by?.id ?? channel.data?.created_by_id; + const isEventOwner = isEvent && creatorId === client?.userID; // Get the other user's ID for 1-on-1 DMs (for block/report) const otherUserId = @@ -141,11 +157,45 @@ export const ChannelInfoAndManageDialog = ({ if (!channel.id) return; try { - // Add members to channel - await channel.addMembers(userIds); + // Through the server action, not `channel.addMembers()` directly. + // + // `addMemberToChannel` is the only server-side authorization on channel + // membership — session required, and only staff, admins, or the channel's + // creator may add anyone. It had zero callers: this component called + // Stream from the browser and the action sat unused, so the gate existed + // in the codebase without ever being in the path. Membership is what + // Stream's own permissions key off, which makes an unchecked add the + // widest hole in the chat surface. + // allSettled, not a sequential loop: one rejection used to abandon every + // remaining id AND skip the toast, so adding five people and failing on + // the second silently added one. Same reasoning as the block route. + const results = await Promise.allSettled( + userIds.map((userId) => + addMemberToChannel( + channel.id as string, + userId, + // `Channel["type"]` is a bare `string` in stream-chat; the action + // takes the narrowed union. Every channel this dialog can open is + // one of the two. + channel.type as "messaging" | "team", + ), + ), + ); + + const added = results.filter((r) => r.status === "fulfilled").length; + const failed = results.length - added; + + if (added === 0) { + throw new Error("Could not add anyone to this channel"); + } + toast({ - title: "Success", - description: `${userIds.length} member${userIds.length !== 1 ? "s" : ""} added successfully`, + title: failed > 0 ? "Partially added" : "Success", + description: + failed > 0 + ? `Added ${added} of ${results.length}. Please retry the rest.` + : `${added} member${added !== 1 ? "s" : ""} added successfully`, + variant: failed > 0 ? "destructive" : undefined, }); // Refresh the member list loadMembers(); @@ -239,7 +289,7 @@ export const ChannelInfoAndManageDialog = ({ // In 1-on-1 DMs, both users can clear their view if (isDirectMessage && !displayInfo.isGroupDM) return true; // In group DMs and channels, only creator or privileged roles - const isCreator = channel.data?.created_by_id === client?.userID; + const isCreator = creatorId === client?.userID; const userRole = client?.user?.role; const isPrivileged = userRole === "CONSULTANT" || @@ -304,18 +354,23 @@ export const ChannelInfoAndManageDialog = ({ }); if (!response.ok) { - const data = await response.json(); - throw new Error(data.error || "Block failed"); + const data = await response.json().catch(() => null); + throw new Error(data?.error || "Block failed"); } toast({ title: "User blocked", description: "This user can no longer message you", }); - } catch { + } catch (error) { + // The server's message, not a generic one. A partial block answers 502 + // with "Blocked in 1 of 2 conversations" — telling someone only that it + // "failed" would hide that half of it succeeded, and telling them it + // worked would be worse. toast({ title: "Error", - description: "Failed to block user", + description: + error instanceof Error ? error.message : "Failed to block user", variant: "destructive", }); } finally { diff --git a/components/chat/ChannelSearch.tsx b/components/chat/ChannelSearch.tsx index 1e8179ece..d7a1263da 100644 --- a/components/chat/ChannelSearch.tsx +++ b/components/chat/ChannelSearch.tsx @@ -1,6 +1,7 @@ "use client"; -import { useState, useEffect, useCallback, useMemo } from "react"; +import { useState, useEffect, useCallback, useMemo, useRef } from "react"; +import { useDebouncedCallback } from "use-debounce"; import Image from "next/image"; import { useChatPane } from "./ChatPaneContext"; import { useChatContext } from "stream-chat-react"; @@ -20,15 +21,53 @@ const EVENT_TYPE_CONFIG = { }, } as const; -// Grouped consultant result for conversations (consultations + subscriptions) -type GroupedConsultant = { - consultantName: string; - consultantImage?: string; +/** A search hit for a group event. Narrower than AppointmentSearchResult. */ +type EventSearchResult = AppointmentSearchResult & { + type: "webinar" | "class"; +}; + +const isEventResult = (r: AppointmentSearchResult): r is EventSearchResult => + r.type === "webinar" || r.type === "class"; + +/** + * What `/api/stream/channels/open` accepts. + * + * A discriminated union rather than `Record`: the route parses + * this with a zod discriminated union, and typing the client side as an open + * bag meant a wrong `eventType` — `"consultation"` reaching the event arm, say — + * failed at runtime with a 400 instead of at compile time. The route is the + * authority on the shape; this mirrors it. + */ +type OpenChannelRequest = + | { kind: "dm"; counterpartyUserId: string; organizationId: string | null } + | { kind: "event"; eventType: "webinar" | "class"; eventId: string }; + +/** + * One row per conversation: a consultation and a subscription with the same + * person collapse into a single entry, because they are a single channel. + * A pair has one DM thread per funding context (#1134 P0-7), so the badges + * "Consultation & Subscription" describe two reasons to be in one thread, not + * two threads. + */ +type GroupedConversation = { + counterpartyName: string; + counterpartyImage?: string; + counterpartyUserId?: string; + organizationId?: string | null; hasConsultation: boolean; hasSubscription: boolean; - // Use the first available channel for navigation channelId: string; - channelType: "consultation" | "subscription"; + /** + * The plan titles behind this row, deduplicated. + * + * Rendered because the route matches on plan title as well as on names, so a + * row could match a query the UI never showed: searching "michael" surfacing + * a conversation with Robert Brown is correct if Robert booked a plan called + * "Michael's…", but with only the counterparty's name on screen it reads as a + * broken search. One row can hold more than one title — a consultation and a + * subscription with the same person share a channel. + */ + planTitles: string[]; }; export const ChannelSearch = () => { @@ -42,57 +81,138 @@ export const ChannelSearch = () => { const [searchResults, setSearchResults] = useState( [], ); + /** Why the last open attempt failed, shown inside the dropdown. */ + const [openError, setOpenError] = useState(null); + /** + * The query `searchResults` actually answers. + * + * Without this there is no way to distinguish "searched and found nothing" + * from "have not searched yet", and the empty state rendered during the + * 300ms debounce gap — announcing "No results found for michael" before a + * request had been issued, on every keystroke, while the matching person sat + * in the list underneath. + */ + const [settledQuery, setSettledQuery] = useState(""); + /** + * A search that FAILED, as distinct from one that found nothing. + * + * The catch used to set `settledQuery` and leave the results empty, which is + * indistinguishable from a successful empty search — so a 500 or a dropped + * connection rendered as "No results found for michael". That is the most + * misleading thing it could say: it tells you the person does not exist. + */ + const [searchError, setSearchError] = useState(null); + const containerRef = useRef(null); + /** + * Latest-wins guard, mirroring `hooks/scheduling/useCalendarData.ts`. + * + * The AbortController below stops the NETWORK work, but a response that has + * already been parsed is past the point of cancellation and still queued for + * React's state pipeline. Only the response matching the most recently issued + * request may commit — checked before every setState, including the one that + * clears `loading`, because a stale reply's `finally` otherwise turns the + * spinner off while a newer request is still in flight. + */ + const requestIdRef = useRef(0); + const abortRef = useRef(null); + + // Extracted from the memo below to keep its cognitive complexity under the + // threshold SonarCloud enforces. The merge rule is the interesting part + // anyway: a consultation and a subscription with the same person are two + // reasons to be in ONE thread, not two threads. + const mergeConversationRow = ( + byChannel: Map, + result: AppointmentSearchResult, + ) => { + const existing = byChannel.get(result.channelId); + if (existing) { + existing.hasConsultation ||= result.type === "consultation"; + existing.hasSubscription ||= result.type === "subscription"; + if (!existing.planTitles.includes(result.name)) { + existing.planTitles.push(result.name); + } + return; + } + byChannel.set(result.channelId, { + counterpartyName: result.counterpartyName, + counterpartyImage: result.counterpartyImage, + counterpartyUserId: result.counterpartyUserId, + organizationId: result.organizationId, + hasConsultation: result.type === "consultation", + hasSubscription: result.type === "subscription", + channelId: result.channelId, + planTitles: [result.name], + }); + }; - // Group consultations/subscriptions by consultant, keep events separate - const { groupedConsultants, events } = useMemo(() => { - const consultantMap = new Map(); - const eventResults: AppointmentSearchResult[] = []; + // Group 1:1 rows by CHANNEL, keep events separate. + // + // Keyed on `channelId`, not on the display name. Grouping by name merged two + // different people who happen to share one — and, more often here, split a + // single conversation in two whenever the name resolved differently between + // rows. The channel id is the identity of a conversation; the name is a label + // on it. + const { groupedConversations, events } = useMemo(() => { + const byChannel = new Map(); + const eventResults: EventSearchResult[] = []; // Defensive check in case searchResults is undefined if (!searchResults || !Array.isArray(searchResults)) { - return { groupedConsultants: [], events: [] }; + return { groupedConversations: [], events: [] }; } for (const result of searchResults) { - if (result.type === "consultation" || result.type === "subscription") { - // Group by consultant name - const existing = consultantMap.get(result.consultantName); - if (existing) { - if (result.type === "consultation") existing.hasConsultation = true; - if (result.type === "subscription") existing.hasSubscription = true; - } else { - consultantMap.set(result.consultantName, { - consultantName: result.consultantName, - consultantImage: result.consultantImage, - hasConsultation: result.type === "consultation", - hasSubscription: result.type === "subscription", - channelId: result.channelId, - channelType: result.type, - }); - } - } else { - // Webinars and classes shown individually + if (isEventResult(result)) { eventResults.push(result); + } else { + mergeConversationRow(byChannel, result); } } return { - groupedConsultants: Array.from(consultantMap.values()), + groupedConversations: Array.from(byChannel.values()), events: eventResults, }; }, [searchResults]); - const handleSearch = useCallback(async () => { - if (!query.trim() || query.trim().length < 2) { + const runSearch = useCallback(async (raw: string) => { + const term = raw.trim(); + + // Abort whatever is still in flight. Nobody is waiting on it, and on a + // route that fires every 300ms of typing the abandoned work is real server + // cost, not just a wasted response. + abortRef.current?.abort(); + + if (term.length < 2) { + abortRef.current = null; setSearchResults([]); + setSettledQuery(""); + setSearchError(null); + setLoading(false); + // Cleared here too. `handleSearch` used to early-return BEFORE the + // setOpenError below, so a 403 refusal outlived the input that produced + // it: emptying the box left an error panel floating over a blank search, + // and `isOpen` kept the dropdown up with no way to dismiss it. + setOpenError(null); return; } + const controller = new AbortController(); + abortRef.current = controller; + // Bumped synchronously, before the first await, so two searches started in + // the same tick still get distinct ids. + const requestId = ++requestIdRef.current; + try { setLoading(true); + // A refusal from the previous attempt must not outlive the query that + // caused it. + setOpenError(null); + setSearchError(null); const response = await fetch( - `/api/stream/channels/search-appointments?q=${encodeURIComponent(query.trim())}`, + `/api/stream/channels/search-appointments?q=${encodeURIComponent(term)}`, + { signal: controller.signal }, ); if (!response.ok) { @@ -100,76 +220,196 @@ export const ChannelSearch = () => { } const results: AppointmentSearchResult[] = await response.json(); + + if (requestId !== requestIdRef.current) return; setSearchResults(results); + setSettledQuery(term); } catch (error) { + // An abort is this component cancelling its own request, not a failure. + // Treating it as one would clear the results the newer request is about + // to populate. + if (error instanceof DOMException && error.name === "AbortError") return; + if (requestId !== requestIdRef.current) return; console.error("Error searching appointments:", error); setSearchResults([]); + setSettledQuery(term); + setSearchError("Search is unavailable right now. Please try again."); } finally { - setLoading(false); + if (requestId === requestIdRef.current) setLoading(false); } - }, [query]); + }, []); - // Debounced search effect - useEffect(() => { - if (!query.trim() || query.trim().length < 2) { - setSearchResults([]); - return; - } + // 300ms, matching the five other search inputs in the app, and now via the + // same `use-debounce` helper they use rather than a hand-rolled setTimeout. + // The delay was never what felt slow — the empty-state flash inside it was. + const debouncedSearch = useDebouncedCallback(runSearch, 300); - const timeoutId = setTimeout(() => { - handleSearch(); - }, 300); // 300ms delay - - return () => clearTimeout(timeoutId); - }, [query, handleSearch]); + useEffect(() => { + debouncedSearch(query); + }, [query, debouncedSearch]); + + // Abort anything outstanding when the component goes away, so a resolved + // fetch cannot setState on an unmounted tree. + useEffect( + () => () => { + // `use-debounce` v10 does NOT auto-cancel on unmount, so a trailing + // callback can fire after the component is gone and start a search + // nobody is waiting for. Abort covers a request already issued; cancel + // covers one still sitting in the timer. + debouncedSearch.cancel(); + abortRef.current?.abort(); + }, + [debouncedSearch], + ); const handleSearchSubmit = (e: React.FormEvent) => { e.preventDefault(); - handleSearch(); + // Submit means "now", so skip the pending debounce rather than adding a + // second in-flight request alongside it. + debouncedSearch.flush(); }; - // Handle click on grouped consultant (for conversations) - const handleConsultantClick = async (consultant: GroupedConsultant) => { - if (!client) return; - - try { - // Conversations use messaging channel type - const channel = client.channel("messaging", consultant.channelId); - await channel.watch(); - setActiveChannel(channel); - openConversation(); - } catch (error) { - console.error("Error opening channel:", error); - } - - // Clear search + /** + * Dismiss the dropdown on an outside click or Escape. + * + * Neither existed. The only way to close this was to pick a result or empty + * the input, so a search you had changed your mind about stayed on screen + * over the channel list indefinitely — and once a failed open stopped + * clearing the query, there was no way to close it at all. + * + * `pointerdown`, not `click`: a `click` listener fires after the button's own + * handler and after focus moves, which on a touch device closed the panel + * before the tap that opened a row could land. + */ + const dismiss = useCallback(() => { + // Clear the TERM too, not just the rows: `isOpen`/`showNoResults` derive + // from the settled query, so leaving it set turned dismiss into "replace + // the results with No results found". setQuery(""); + setSettledQuery(""); setSearchResults([]); - }; + setOpenError(null); + setSearchError(null); + }, []); - // Handle click on event (webinar/class) - const handleEventClick = async (result: AppointmentSearchResult) => { + useEffect(() => { + const onPointerDown = (event: PointerEvent) => { + if (!containerRef.current) return; + if (!containerRef.current.contains(event.target as Node)) dismiss(); + }; + const onKeyDown = (event: KeyboardEvent) => { + if (event.key === "Escape") dismiss(); + }; + document.addEventListener("pointerdown", onPointerDown); + document.addEventListener("keydown", onKeyDown); + return () => { + document.removeEventListener("pointerdown", onPointerDown); + document.removeEventListener("keydown", onKeyDown); + }; + }, [dismiss]); + + /** + * Ask the SERVER for the channel, then watch what it hands back. + * + * The previous implementation called `client.channel(type, id).watch()` on an + * id this component had received from search. `watch()` posts to Stream's + * channel *query* endpoint, which is the same endpoint `create()` posts to — + * so when the id did not exist yet, watching it CREATED it, with this user as + * `created_by` and with no members at all. The result was a channel titled + * with its own raw id, reporting "No members", that accepted a message and + * then disappeared on refresh (the sidebar lists `members: { $in: [me] }`). + * + * So the client no longer names a channel. It names a person or an event, and + * `/api/stream/channels/open` re-derives the id, checks the booking link, and + * creates the channel with both members if it is genuinely missing. By the + * time `watch()` runs here, the channel is known to exist. + */ + const openResolvedChannel = async ( + body: OpenChannelRequest, + ): Promise => { if (!client) return; + setOpenError(null); try { - // Events use team channel type - const channel = client.channel("team", result.channelId); + const response = await fetch("/api/stream/channels/open", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); + + if (!response.ok) { + const detail = (await response.json().catch(() => null)) as { + error?: string; + } | null; + // A 403 here is the eligibility gate refusing, which is a legitimate + // answer rather than a fault — but it still has to be SAID. This branch + // used to `console.error` and return, and because the return skipped the + // reset at the bottom of the function, the dropdown stayed open with the + // same rows and no explanation. It read as a frozen menu. + setOpenError( + detail?.error ?? + "Could not open this conversation. Please try again.", + ); + return; + } + + const { channelType, channelId } = (await response.json()) as { + channelType: "messaging" | "team"; + channelId: string; + }; + + const channel = client.channel(channelType, channelId); await channel.watch(); setActiveChannel(channel); openConversation(); + // Only cleared on the success path. On a refusal the query is kept so the + // message below has something to sit under and the person can pick a + // different row. + setQuery(""); + setSearchResults([]); } catch (error) { console.error("Error opening channel:", error); + setOpenError("Could not open this conversation. Please try again."); } + }; - // Clear search - setQuery(""); - setSearchResults([]); + const handleConversationClick = (conversation: GroupedConversation) => { + // Group-event rows carry no counterparty, and a malformed payload would be + // rejected by the route's zod schema as a 400 — a worse message than this + // one. Guard rather than round-trip. + if (!conversation.counterpartyUserId) { + setOpenError("This conversation is missing its participant."); + return; + } + void openResolvedChannel({ + kind: "dm", + counterpartyUserId: conversation.counterpartyUserId, + organizationId: conversation.organizationId ?? null, + }); }; - const hasResults = groupedConsultants.length > 0 || events.length > 0; + const handleEventClick = (result: EventSearchResult) => + void openResolvedChannel({ + kind: "event", + eventType: result.type, + eventId: result.id, + }); + + const hasResults = groupedConversations.length > 0 || events.length > 0; + const term = query.trim(); + /** + * True from the keystroke until a response for THIS query has committed — + * covering the debounce wait, which `loading` does not, because `loading` + * only goes true once the fetch actually starts 300ms later. + */ + const isPending = term.length >= 2 && (loading || settledQuery !== term); + const showNoResults = + term.length >= 2 && !isPending && !hasResults && searchError === null; + const isOpen = + hasResults || openError !== null || searchError !== null || showNoResults; return ( -
+
{ /> - {hasResults && ( + {isOpen && (
+ {openError && ( +
+ {openError} +
+ )} + {searchError && ( +
+ {searchError} +
+ )} + {/* + The empty state lives INSIDE the dropdown now. It used to be a + second absolutely-positioned box carrying identical + `absolute z-50 mt-1 w-full` classes, so whenever a refusal coexisted + with an empty result set the two rendered on top of each other at + the same coordinates. + */} + {showNoResults && ( +
+ No results found for “{query}” +
+ )} {/* Conversations Section (Consultants with consultations/subscriptions) */} - {groupedConsultants.length > 0 && ( + {groupedConversations.length > 0 && ( <>
Conversations
- {groupedConsultants.map((consultant) => ( + {groupedConversations.map((conversation) => ( @@ -256,15 +532,16 @@ export const ChannelSearch = () => { return (
@@ -302,13 +579,6 @@ export const ChannelSearch = () => { )}
)} - - {/* No results message */} - {query.trim().length >= 2 && !loading && !hasResults && ( -
- No results found for “{query}” -
- )}
); }; diff --git a/components/chat/ChatSidebar.tsx b/components/chat/ChatSidebar.tsx index a490f5869..b2fb6b411 100644 --- a/components/chat/ChatSidebar.tsx +++ b/components/chat/ChatSidebar.tsx @@ -18,7 +18,10 @@ import { TooltipProvider, TooltipTrigger, } from "../ui/tooltip"; -import { getChannelDisplayInfo } from "./utils/channelUtils"; +import { + getChannelDisplayInfo, + isUsableDmChannel, +} from "./utils/channelUtils"; import { useChatPane } from "./ChatPaneContext"; import { useOrgScope } from "@/hooks/useOrgScope"; import { useSession } from "@/lib/auth-client"; @@ -215,6 +218,19 @@ export const ChatSidebar = () => { // succeeds (not when a fetch is skipped) so a skipped/failed fetch never // marks a scope as done. Refetch still happens on a genuine client/scope change. const fetchedKeyRef = useRef(null); + /** + * How many rows Stream has actually returned per list, before filtering. + * + * The pagination offset must count what the SERVER has handed over, not what + * survived `isUsableDmChannel`. Using `directMessages.length` meant every + * phantom dropped from a page shifted the next offset backwards by one, so + * page two re-fetched rows already on screen and the tail of the list became + * unreachable — the filter silently ate the pagination. + */ + const fetchedCountRef = useRef<{ team: number; messaging: number }>({ + team: 0, + messaging: 0, + }); // Pagination state const [hasMoreTeamChannels, setHasMoreTeamChannels] = useState(true); @@ -295,10 +311,25 @@ export const ChatSidebar = () => { return; } + const usableDms = dmResponse.filter(isUsableDmChannel); + setTeamChannels(teamResponse); - setDirectMessages(dmResponse); + // Phantoms filtered out here rather than hidden at render: a row that is + // merely styled as unavailable is still selectable, still opens, and still + // accepts a message. See isUsableDmChannel. + setDirectMessages(usableDms); + + // Raw counts, for the next page's offset. + fetchedCountRef.current = { + team: teamResponse.length, + messaging: dmResponse.length, + }; - // Update pagination state + // Update pagination state — measured against the RAW response length, not + // the filtered one. `response.length === limit` is Stream's + // "there may be more" signal, and comparing a filtered count to the limit + // would report no-more-pages the moment a single phantom is dropped from + // an otherwise full page. setHasMoreTeamChannels(teamResponse.length === options.limit); setHasMoreDMChannels(dmResponse.length === options.limit); @@ -310,7 +341,10 @@ export const ChatSidebar = () => { if (!initialSelectionDoneRef.current) { initialSelectionDoneRef.current = true; const mostRecentTeam = teamResponse[0]; - const mostRecentDM = dmResponse[0]; + // The FILTERED list — auto-selecting `dmResponse[0]` could open a + // phantom on load, which is the exact thing being filtered out + // everywhere else. + const mostRecentDM = usableDms[0]; let channelToSelect = null; if (mostRecentTeam && mostRecentDM) { const teamTime = new Date( @@ -353,7 +387,6 @@ export const ChatSidebar = () => { async (type: "team" | "messaging") => { if (!client?.userID || isLoadingMore) return; - const currentChannels = type === "team" ? teamChannels : directMessages; const hasMore = type === "team" ? hasMoreTeamChannels : hasMoreDMChannels; if (!hasMore) return; @@ -376,7 +409,9 @@ export const ChatSidebar = () => { }; const sort: { last_message_at: -1 } = { last_message_at: -1 }; - const offset = currentChannels.length; + // Raw fetched count, never `currentChannels.length` — see + // fetchedCountRef. + const offset = fetchedCountRef.current[type]; const options = { watch: true, @@ -389,11 +424,17 @@ export const ChatSidebar = () => { const response = await client.queryChannels(filter, sort, options); + fetchedCountRef.current[type] += response.length; + if (type === "team") { setTeamChannels((prev) => [...prev, ...response]); setHasMoreTeamChannels(response.length === options.limit); } else { - setDirectMessages((prev) => [...prev, ...response]); + setDirectMessages((prev) => [ + ...prev, + ...response.filter(isUsableDmChannel), + ]); + // Raw length again — see fetchChannels. setHasMoreDMChannels(response.length === options.limit); } } catch (error) { @@ -404,8 +445,9 @@ export const ChatSidebar = () => { }, [ client, - teamChannels, - directMessages, + // `teamChannels` / `directMessages` are deliberately absent: the offset + // now comes from `fetchedCountRef`, so this callback no longer reads + // either list. Keeping them would rebuild it on every incoming message. hasMoreTeamChannels, hasMoreDMChannels, isLoadingMore, @@ -485,7 +527,7 @@ export const ChatSidebar = () => { setDirectMessages((prevChannels) => { const existingIds = new Set(prevChannels.map((ch) => ch.cid)); const newChannels = recentDMChannels.filter( - (ch) => !existingIds.has(ch.cid), + (ch) => !existingIds.has(ch.cid) && isUsableDmChannel(ch), ); if (newChannels.length > 0) { return [...newChannels, ...prevChannels]; // New channels at top @@ -691,7 +733,15 @@ export const ChatSidebar = () => {

Channels

{canCreateChannels && ( - + )}
{isLoading ? ( @@ -754,7 +804,18 @@ export const ChatSidebar = () => {

Conversations could not be loaded.

- ) : directMessages.length > 0 ? ( + ) : ( + // NOT `directMessages.length > 0 ? list : emptyState`. + // + // The list is filtered (phantom DMs are dropped, see + // isUsableDmChannel) but `hasMoreDMChannels` is measured against the + // RAW page length. So a page whose 20 rows are all phantoms leaves the + // list empty with more pages still to come — and with the load-more + // button living inside the non-empty branch, the empty state rendered + // "No conversations yet" over a stranded list the user could not + // reach. The button is now tied to `hasMoreDMChannels` alone, and the + // empty state only claims there is nothing when there is genuinely + // nothing left to fetch.
{directMessages.map((channel) => ( { onClick={() => handleChannelSelect(channel)} /> ))} + + {directMessages.length === 0 && !hasMoreDMChannels && ( +
+ {isConsultant + ? "No conversations yet. Conversations will appear here once clients book sessions." + : "No conversations yet. Book a consultation to start chatting."} +
+ )} + {hasMoreDMChannels && (
)}
- ) : ( -
- {isConsultant - ? "No conversations yet. Conversations will appear here once clients book sessions." - : "No conversations yet. Book a consultation to start chatting."} -
)} diff --git a/components/chat/CreateChannelDialog.tsx b/components/chat/CreateChannelDialog.tsx index 668596e2a..0c28c963a 100644 --- a/components/chat/CreateChannelDialog.tsx +++ b/components/chat/CreateChannelDialog.tsx @@ -26,10 +26,21 @@ import { useChatContext } from "stream-chat-react"; interface CreateChannelDialogProps { onChannelCreated?: () => void; + /** + * Whether the viewer may create a channel not bound to an event. + * + * `app/api/stream/channels/create/route.ts` restricts these to ADMIN/STAFF + * so an unprivileged caller cannot assemble an arbitrary member list. The + * option is hidden rather than left to fail, because a consultant selecting + * it previously succeeded — the dialog created the channel client-side and + * never consulted the route at all. + */ + canCreateCustomChannel?: boolean; } export const CreateChannelDialog = ({ onChannelCreated, + canCreateCustomChannel = false, }: CreateChannelDialogProps) => { const [open, setOpen] = useState(false); const [channelName, setChannelName] = useState(""); @@ -110,7 +121,20 @@ export const CreateChannelDialog = ({ setIsLoading(true); try { - if (selectedEvent && selectedEvent !== "custom") { + // Three states, not two. `selectedEvent` initialises to `null`, so an + // `if (event) … else custom` split sent "nothing chosen" down the custom + // branch — which for a consultant is a request the route answers 403. + // Each state is now named. + if (!selectedEvent) { + toast({ + title: "Error", + description: "Please choose what this channel is for", + variant: "destructive", + }); + return; + } + + if (selectedEvent !== "custom") { // Event-linked channel creation - use server-side API with full participant lists const [eventType, eventId] = selectedEvent.split("-"); @@ -152,17 +176,48 @@ export const CreateChannelDialog = ({ description: result.message || `Channel "${channelName}" created successfully`, }); + } else if (!canCreateCustomChannel) { + // Belt and braces: the option is hidden for callers who cannot use it, + // but a stale `selectedEvent` from before a role change would otherwise + // reach the route and come back 403 with no explanation. + toast({ + title: "Not allowed", + description: "Only staff can create channels that aren't tied to an event", + variant: "destructive", + }); + return; } else { - // Custom channel creation - use client-side creation (no predefined participants) - const channelId = crypto.randomUUID(); - - const channel = client.channel("team", channelId, { - name: channelName, - members: [currentUserId], // Only creator for custom channels - created_by_id: currentUserId, + // Custom channel creation — server-side, same as the event branch. + // + // This used to mint the channel straight from the browser with + // `client.channel("team", crypto.randomUUID(), …).create()`. That + // bypassed the admin/staff-only gate in + // `app/api/stream/channels/create/route.ts`, so anyone the sidebar + // considers able to create channels — every consultant — could create + // custom `team` channels that no server-side rule had approved. The + // route enforces the gate; the client asks it to. + const response = await fetch("/api/stream/channels/create", { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ + channelType: "team", + channelName, + createdById: currentUserId, + }), }); - await channel.create(); + const result = await response.json(); + + if (!response.ok || !result.success) { + throw new Error(result.error || "Failed to create channel"); + } + + // `createChannel` returns `{ channelId, members, channelData }` and the + // route wraps it in `data`. + const channel = client.channel("team", result.data.channelId); + await channel.watch(); // Set the new channel as active setActiveChannel(channel); @@ -238,7 +293,11 @@ export const CreateChannelDialog = ({ - Create a Custom Channel + {canCreateCustomChannel && ( + + Create a Custom Channel + + )} {events.length > 0 ? ( events.map((event) => ( diff --git a/components/chat/utils/channelUtils.ts b/components/chat/utils/channelUtils.ts index a841dd2a2..101aaa0a1 100644 --- a/components/chat/utils/channelUtils.ts +++ b/components/chat/utils/channelUtils.ts @@ -9,6 +9,30 @@ export interface ChannelDisplayInfo { fullGroupName?: string; // For tooltips } +/** + * Is this a direct message that actually has someone on the other end? + * + * A `messaging` channel with fewer than two members is a phantom. They exist + * because `channel.watch()` posts to the same endpoint `channel.create()` does, + * so watching a channel id that does not exist yet CREATES it — with the caller + * as `created_by` and no members at all. Every client-side path that could do + * that is now gone, and `ensure-chat-type-grants.ts` revokes `create-channel` + * from the `user` role so Stream itself refuses, but neither of those helps with + * the ones already sitting on the app: dev, preview and production share one + * Stream app, and a phantom is a real channel that a real message can be posted + * into. `scripts/stream/purge-memberless-dms.ts` deletes them; this keeps them + * off the screen in the meantime, and keeps any future one unreachable rather + * than merely mislabelled. + * + * Deliberately scoped to `messaging`. A `team` channel legitimately sits at one + * member — a webinar channel is created with its host before anyone registers — + * so applying this to events would hide real, working channels. + */ +export const isUsableDmChannel = (channel: Channel): boolean => { + if (channel.type !== "messaging") return true; + return Object.keys(channel.state?.members ?? {}).length >= 2; +}; + /** * Get consistent display information for any channel across all chat components */ @@ -85,13 +109,27 @@ export const getChannelDisplayInfo = ( } } - // Fallback + // Fallback: a messaging channel with no counterparty, or no `currentUserId` + // yet because the client is still connecting. + // + // This branch used to end in `channel.id`, which is why a broken DM rendered + // its raw `dm--` key as the conversation title. A channel id is an + // internal key — it is never a name, it leaks both participants' user ids into + // the UI, and showing it made a real defect (a channel created with no + // members, see lib/stream/dm-eligibility.ts) look like a formatting quirk. + // + // A one-member DM should not exist. If one is on screen, say so plainly + // instead of dressing it up, and keep the id out of the title. + const isOrphanedDm = isDirectMessage; + return { - displayName: channel.data?.name || channel.id || "Unknown", + displayName: + (channel.data?.name as string | undefined) || + (isOrphanedDm ? "Unavailable conversation" : channel.id || "Unknown"), displayImage: undefined, isGroupDM: false, memberCount: 0, - statusText: "No members", + statusText: isOrphanedDm ? "No other participants" : "No members", }; }; diff --git a/docs/decisions/2026-08-15-chat-eligibility-and-client-channel-creation.md b/docs/decisions/2026-08-15-chat-eligibility-and-client-channel-creation.md new file mode 100644 index 000000000..3cfe5c615 --- /dev/null +++ b/docs/decisions/2026-08-15-chat-eligibility-and-client-channel-creation.md @@ -0,0 +1,203 @@ +# ADR: DM eligibility, and taking channel creation away from the client + +- **Status**: Accepted +- **Date**: 2026-08-15 +- **Part of**: #1134 (follow-on), builds on #899, #981 + +## Context + +A consultant reported being able to hold a conversation with themselves from +their own dashboard: searching two letters of their own name returned a row +labelled with their name, clicking it opened a thread, the thread accepted a +message, its header showed a raw `dm--` id and "No members", and on +refresh the thread was gone. + +Four symptoms, two defects, and neither of them is what the report sounds like. + +**The label.** `app/api/stream/channels/search-appointments/route.ts` is +correctly scoped — the caller must be the consultee _or_ the consultant on every +row it returns — but it labelled every row `consultantName`. On a consultee's +dashboard that names the other party. On a consultant's it names the viewer. The +`channelId` underneath was always right and always pointed at the real +consultee; only the label lied. `ChannelSearch` then grouped rows by that name, +which is how one person's consultation and subscription collapsed into a single +row subtitled "Consultation & Subscription". + +**The phantom channel.** `ChannelSearch` opened a result with +`client.channel(type, id).watch()` on an id the browser had computed. In +`stream-chat`, `watch()` posts to the channel _query_ endpoint — the same +endpoint `create()` posts to; `channel.create()` is literally +`query({ created_by_id })`. So watching an id that does not exist creates it, +and created that way with no `members` array the caller becomes `created_by` and +is **not a member**. That single fact explains the raw-id header (channelUtils +filters the viewer out of the member list, finds nobody, and falls through to +`channel.id`), the "No members", the message sending fine, and the disappearance +on reload (the sidebar lists `{ members: { $in: [me] } }`). + +This reproduces identically against a stranger. The phantom is not a property of +the pair — it is a property of the id not existing. + +The id did not exist because the three answers to "are these two people +connected?" disagreed: + +| | statuses | +| ------------------------------------ | -------------------------------------------------------- | +| `checkUserRelationship` | APPROVED, SCHEDULED (+ subscription window open) | +| `getDmPairsForUser` (the reconciler) | APPROVED, SCHEDULED | +| the two search routes | APPROVED, APPROVED_PENDING_PAYMENT, SCHEDULED, COMPLETED | + +Search was widest, so it offered rows for bookings the create path had never +fired for. And `checkUserRelationship` — the only implementation of the rule, +with a full unit-test suite — had **zero production call sites**. Nothing +checked eligibility before a DM was created. `createDirectMessageChannel` +validated two non-empty strings: no session, no relationship query, not even +`a !== b`. It was safe by accident, because every caller happened to be a +booking-approval or payment-success path. + +Underneath all of it, nothing in this repo has ever configured Stream's chat +permissions. The only `updateAppSettings` call is the webhook subscription +script, and it writes `event_hooks` alone. So `messaging` and `team` run on +Stream's defaults, which grant `create-channel` to the plain `user` role — which +is what made the browser's `watch()` succeed at all. + +## Decision + +### 1. Eligibility is "ever transacted", and permanent + +A consultation or subscription that reached `APPROVED`, +`APPROVED_PENDING_PAYMENT`, `SCHEDULED` or `COMPLETED`, in either direction, +opens the thread and never closes it. (An earlier draft also counted a shared +non-deleted `SlotOfAppointment`; that group arm was removed before merge — see +§4 — because `/api/stream/channels/open` would have made it a user-reachable +path to consultee↔consultee DMs.) + +`PENDING` is excluded: a request the consultant has not accepted is not a +relationship, or anyone opens a channel with anyone by requesting a booking they +never intend to pay for. + +The scheduling-window filter on subscriptions is gone. A lapsed subscription is +still a relationship that happened, and gating on the window made a thread +unreachable at midnight on the renewal date — mid-conversation, with no notice, +and with the reconciler then classifying the channel stale and evicting both +parties from their own history. + +Rejected: active-only (tightest, but threads go read-only mid-exchange) and +active-plus-grace-window (best privacy balance, but needs a dated freeze job for +DMs on top of the one that already exists for event channels — worth revisiting +when there is a retention requirement to satisfy). + +`DM_ELIGIBLE_STATUSES` lives in `lib/stream/dm-eligibility-statuses.ts`, a pure +module with no Prisma import, and is the only definition. **It must move as one +unit**: `syncUserEventChannels` removes users from any managed DM channel absent +from the expected set it builds from that constant. + +### 2. The client never names a channel + +`POST /api/stream/channels/open` takes _who_ or _what_ to talk to — a +counterparty user id, or an event type and id — and re-derives the channel id +server-side from the caller's session. A client-supplied channel id would be an +authorization bypass by construction, since the id is a pure function of the two +user ids: anyone who can name a pair could name their channel. + +Both arms are idempotent, and both create with the full member list atomically, +which is the whole difference from what `watch()` was doing. + +`CreateChannelDialog` likewise stopped creating custom channels client-side — +that path bypassed the admin/staff-only gate in the create route entirely — and +`ChannelInfoAndManageDialog` now adds members through `addMemberToChannel`, the +server action that had been written for exactly this and never called. + +### 3. Stream's own permissions back it up + +`scripts/stream/ensure-chat-type-grants.ts`, modelled on the existing +`ensure-call-type-grants.ts`: dry-run by default, `--apply` to write, +`--restore-user-create` to roll back, and refusing to apply without +`--open-route-is-deployed`. It revokes `create-channel` and +`update-channel-members` from `user` and `guest` on both channel types, and sets +`user_search_disallowed_roles`. + +`guest` matters as much as `user`: the app has +`guest_user_creation_disabled: false`, so guest sessions are creatable +client-side with nothing but the public API key shipped as +`NEXT_PUBLIC_STREAM_API_KEY`. + +Unlike call types, channel types are hardened **in place** with no +grandfathering problem — grants are evaluated per request against the type, not +baked in at creation, so existing channels pick up the change. + +### 4. Org scoping stays app-side + +A pair keeps one thread per funding context: `dm-
-` personal, +`dmo--` per organization. ADR 19 requires it — dashboards split by +org-ness, so one merged thread could not live in the right place — and it keeps +an org's sponsored conversations separable for export and retention. + +Stream's native multi-tenant `teams` field was considered and is **not +available**: `docs/stream/14-pricing-and-cost-model.md` lists Multi-Tenancy / +Teams as an Elevate-tier feature, and this app is on the free Maker account. The +existing scheme — org in the channel key, `custom.organization_id` on the +channel, and the sidebar's `organization_id` filter — is the right approach +regardless, and costs nothing. + +While registering this: `dmo-` and `dmh-` were never declared in +`lib/stream-channel-ids.ts`, and `"dmo-".startsWith("dm-")` is false. So +`getChannelTypeFromId` returned `"team"` for org DMs created as `messaging` (four +call sites addressed the wrong type), `isDMChannel` answered false for two of the +three forms, and the reconciler never saw them at all. + +## Amendment, same day: the eager sync is retired + +Widening `DM_ELIGIBLE_STATUSES` to include `COMPLETED` had a consequence this +ADR did not think through. `COMPLETED` is an absorbing state, so +`getDmPairsForUser` no longer returns _currently active_ work — it returns every +consultation or subscription the consultant has ever finished. Neither query +carries a `take`, and `syncUserEventChannels` made a Stream API call per pair, +five at a time, on every cold dashboard load. A consultant with 500 completed +bookings would pay 100 serial waves in the background of every visit, growing +forever. + +The add pass is therefore gone. `syncUserEventChannels` now computes the +expected set and runs the **reconcile-and-remove** pass only. Creation moved to +where it is actually needed: `POST /api/stream/channels/open` provisions the one +channel someone opens, at the moment they open it, and booking approval and +payment success still provision eagerly at transaction time. + +The removal half cannot move to an on-demand path — nothing else notices that a +membership _ought_ to be revoked — which is why it stays on the sync. + +The trade: a user who has lost membership to a channel that still exists is no +longer silently re-added on load. They recover by opening the conversation from +search, which routes through `/open`. `addUserToDmChannel` was deleted with the +pass; it duplicated `createDirectMessageChannel` and, unlike it, ran no +eligibility check. + +This does **not** change the eligibility rule. Ever-transacted still governs who +may hold a DM; it simply no longer governs how many channels get provisioned +speculatively. + +## Consequences + +`checkUserRelationship` no longer swallows errors into `false`. That read as +fail-closed and behaved as the opposite: harmless for an unused advisory flag, +wrong for a gate, and actively dangerous on the reconcile path where a transient +blip would make a live channel look unexpected. + +`searchUsersWithRelationships` now filters instead of ranking. `hasRelationship` +was a sort key, so a two-character query returned every matching user on the +platform — name, email, avatar, role — with connected ones merely listed first. + +The gate's original draft had a GROUP ARM: any two attendees of the same +event slot were mutually eligible, on the theory that no code path offered a +consultee a way to open a DM. Review proved that self-refuting — this PR's own +`POST /api/stream/channels/open` is exactly such a path, and attendee ids are +readable from team-channel member state. The arm was removed from +`canDirectMessage` and from `searchUsersWithRelationships`' related-user +builder before merge; peer DMs remain blocked per the 2026-07-11 ADR. If a +"message the host" affordance is ever wanted, reintroduce it behind an explicit +host-check, never blanket co-membership. + +Named follow-ups, none of which this change makes worse: there is still no path +that pushes a name or avatar change to Stream (only the 5-minute TTL cache +expiring, incidentally); `restoreStreamAccess` is written but has zero callers, +so there is no unban path; and dev, preview and production still share one +Stream app (#1134 P0-6), which is why the grants script defaults to a dry run. diff --git a/docs/stream/04-chat-implementation.md b/docs/stream/04-chat-implementation.md index aaca4642f..d319e1e91 100644 --- a/docs/stream/04-chat-implementation.md +++ b/docs/stream/04-chat-implementation.md @@ -93,57 +93,97 @@ const channelId = getDmChannelId(currentUserId, targetUserId, organizationId); - User A: `user_abc123` - User B: `user_xyz789` -- Channel ID: `user_abc123-user_xyz789` - -**Why Alphabetical Sorting?** - -- Prevents duplicate channels for same conversation -- Ensures same channel ID regardless of who initiates -- Enables consistent channel lookup - -### Consultations - -**Format**: `consultation-{consultationId}` - -**Example**: `consultation-clr4h8x0j0000ab1cdcdef123` - -**Data**: +- Channel ID: `dm-user_abc123-user_xyz789` — the `dm-` prefix is part of the id. + It was missing from this example, which matters because `isDMChannel`, + `getChannelTypeFromId` and `MANAGED_CHANNEL_PREFIXES` all key off it. + +**Why order the ids at all?** + +- The same pair yields the same id regardless of who initiates, so the + conversation is found rather than recreated. +- Neither participant needs to store or look up the id; both derive it. + +Ordering is **code-unit**, per the warning above — not "alphabetical", which is +what this section used to say and is exactly the loose reading that led someone +to reach for `localeCompare`. + +**A self-pair is refused, not ordered.** `getDmChannelId` throws when the two +ids are equal. `createChannel` de-duplicates its member array through a `Set`, +so `dm--` would otherwise become a one-member channel: no counterparty for +`channelUtils` to name, so the header renders the raw id, and nobody to reply. + +**Never open a DM by asking Stream for a computed id.** `channel.watch()` posts +to the same query endpoint `channel.create()` does, so watching an id that does +not exist *creates* it — as `created_by`, with no members, invisible to the +sidebar's `{ members: { $in: [me] } }` filter on the next reload. Go through +`POST /api/stream/channels/open`, which checks eligibility and creates the +channel with both members. + +### Consultations and subscriptions — no channel of their own + +**There is no `consultation-` or `subscription-` channel.** Both reuse +the pair's DM above. + +This section used to document two separate formats with their own member lists. +They never worked. `createConsultationChannel` minted a DM and always had; the +`consultation-` id existed only in this document and in a reconciler blocklist. +Worse, `syncUserEventChannels` built its expected set from webinars, classes and +DMs while treating both prefixes as MANAGED — so any channel that *did* carry +one was classified stale and the buyer was removed from it on their very next +dashboard load. #1134 P0-7 deleted the concept rather than repairing it: the +pair already has a thread, and removing the second one removed a contradiction +rather than a feature. + +`CONSULTATION_PREFIX` and `SUBSCRIPTION_PREFIX` remain exported from +`lib/stream-channel-ids.ts` so `getChannelTypeFromId` can still resolve rows +created before the change. They are deliberately absent from +`MANAGED_CHANNEL_PREFIXES`, so surviving channels are left alone rather than +swept. + +**What a pair actually gets**: one `messaging` channel per funding context. ```typescript { channelType: "messaging", - channelId: `consultation-${consultationId}`, + channelId: getDmChannelId(consultantId, consulteeId, organizationId), members: [consultantId, consulteeId], createdById: consultantId, - additionalData: { consultation_id: consultationId } + additionalData: { + dm_consultant_user_id: consultantId, + dm_consultee_user_id: consulteeId, + }, + organizationId, } ``` -### Subscriptions - -**Format**: `subscription-{subscriptionId}` +Ten consultations and three subscriptions between the same two people in the +same context are one conversation. A personal booking and an org-funded one are +two, because ADR 19 splits dashboards by org-ness and a single thread cannot +live in both. -**Example**: `subscription-clr4h8x0j0000ab1cdcdef456` +`dm_consultant_user_id` is what decides moderation: `createChannel` grants +`channel_moderator` to that user. A DM created without it — the peer path — gets +no moderator at all, deliberately, so a consultee cannot mute or remove the +consultant (#981). -**Data**: +### Who may open one -```typescript -{ - channelType: "messaging", - channelId: `subscription-${subscriptionId}`, - members: [consultantId, consulteeId], - createdById: consultantId, - additionalData: { subscription_id: subscriptionId } -} -``` +A DM requires that the two people have transacted. `canDirectMessage` +(`lib/stream/dm-eligibility.ts`) is the only implementation of that rule: -### Webinars +- a `Consultation` or `Subscription` in `APPROVED`, + `APPROVED_PENDING_PAYMENT`, `SCHEDULED` or `COMPLETED`, in either direction; +- or a shared, non-deleted `SlotOfAppointment`. -**Format**: `webinar-{webinarId}` +Permanent once established — a lapsed subscription still leaves the thread +open. `DM_ELIGIBLE_STATUSES` is shared by the gate, the two search routes, and +`getDmPairsForUser`. **Those must move together**: the reconciler removes users +from any managed DM channel absent from the expected set it builds from that +constant, so narrowing it evicts people from live conversations. -**Example**: `webinar-clr4h8x0j0000ab1cdcdef789` +### Webinars -**Data**: +**Format**: `webinar-{webinarId}` · **Stream type**: `team` ```typescript { @@ -156,26 +196,30 @@ const channelId = getDmChannelId(currentUserId, targetUserId, organizationId); } ``` +Members come from `appointment.slotsOfAppointment[].user`, deduplicated — a +webinar's registrants are connected to every one of its slots, so the same id +appears once per slot. The host is added separately and is always a member. + ### Classes -**Format**: `class-{classId}` +**Format**: `class-{classId}` · **Stream type**: `team` -**Example**: `class-clr4h8x0j0000ab1cdcdef012` +Identical in shape; the roster walks `class.appointments[].slotsOfAppointment[].user`. -**Data**: +### Collaborators -```typescript -{ - channelType: "team", - channelId: `class-${classId}`, - channelName: classData.classPlan.title, - members: [consultantUserId, ...participantIds], - createdById: consultantUserId, - additionalData: { class_id: classId } -} -``` +**Format**: `collab-{webinar|class}-{planId}` · **Stream type**: `messaging` + +Host plus `ACCEPTED` collaborators, reconciled two-way against the collaborator +list on every accept. + +### Event channel lifecycle + +`jobs/stream/expire-event-channels.ts` freezes a webinar or class channel 7 days +after its last session ends (readable, not writable) and hard-deletes it at the +org's `streamRecordingRetentionDays`, default 90. DM channels are deliberately +excluded: the pair's thread outlives any single booking. ---- ## Creating Channels diff --git a/docs/stream/07-user-management.md b/docs/stream/07-user-management.md index 6158d9159..8c9f58c99 100644 --- a/docs/stream/07-user-management.md +++ b/docs/stream/07-user-management.md @@ -119,12 +119,57 @@ try { Stream Chat uses a role-based permission system. The `mapRoleToStream` function maps application user roles to Stream Chat roles. -**Location:** `/lib/user.ts` (lines 98-115) +**Location:** `/lib/user.ts` — `mapRoleToStream` ### Least-Privilege Mapping (#899) The mapping now follows least privilege. Only platform staff and admins receive Stream's global `admin` role; every other user, consultants included, is mapped to the plain `user` role. Consultants no longer get a blanket administrative grant. Instead, channel creation happens server-side and each host is given a channel-scoped `channel_moderator` grant on their own host channels at creation time, rather than a global moderation grant that would also cover peer direct-message channels. +Worth stating plainly how bad the previous version was, because the current +mapping reads as unremarkable and it is not: **every branch of the old switch +returned `admin`, including the `if (!role)` fallback.** Every account on the +platform held Stream global admin. The docblock said so out loud — "using admin +for consultants and consultees to ensure team channel access… can be refined +later with custom roles." + +### Decision: keep `ADMIN`/`STAFF` → Stream `admin` + +**Status: accepted.** Reviewed again while adding the DM eligibility gate; kept +as-is. + +What it costs is real and should be understood rather than forgotten: Stream's +`admin` role bypasses channel permission checks, so a staff token can read any +channel — including any private consultant↔consultee DM — from the browser. +The blast radius of a stolen staff session is every conversation on the +platform. It is a deliberate trade, not an oversight. + +Two alternatives were considered and rejected **for now**. Both remain open, and +either would be a strict improvement if the operational cost is acceptable when +someone next looks at this: + +1. **Map everyone to `user`; do moderation server-side.** Staff would hold no + special client-side role at all, and every moderation or support action would + go through the server clients in `lib/stream-client.ts`, which present the + API secret and bypass Stream's permission system anyway — so nothing is lost + operationally *unless* a support surface needs to read channels directly in + the browser. This is the least-privilege answer and removes the skeleton key + entirely. It is the option to take if a staff account is ever compromised, or + before the platform holds conversations it would be damaging to leak in bulk. + Migration cost: one `upsertUsers` sweep to re-stamp existing staff rows, + since the role is written at upsert time and the 5-minute sync cache means + stale rows linger until they next reconnect. +2. **A Stream custom role granting `ReadChannel` and nothing else.** Staff could + observe without being able to write, delete, or reconfigure. More precise + than either of the above, and Stream allows up to 25 custom roles. Rejected + for now on maintenance grounds: it is a role defined outside this repo (via + the dashboard or an API call) that must be kept in step with the code, and we + have no deployment path for chat roles yet. `scripts/stream/ensure-chat-type-grants.ts` + is the obvious place to grow one. + +Note that option 1 does **not** conflict with the grants script: that script +revokes `create-channel` from `user` and `guest`, which is orthogonal to whether +staff hold `admin`. + ### Current Implementation **Function Signature:** @@ -314,14 +359,15 @@ The background sync job maintains synchronization between your Prisma database a ### Job Overview -**Schedule:** Daily at 03:30 UTC (9:00 AM IST) +**Schedule:** Daily at 03:40 UTC (9:10 AM IST) **Execution:** GitHub Actions workflow **Location:** -- Job logic: `/jobs/stream-sync.ts` -- Workflow: `/.github/workflows/stream_sync.yml` +- Job logic: `/jobs/stream/stream-sync.ts` (implementation in + `/scripts/stream/stream-sync.ts`) +- Workflow: `/.github/workflows/stream-sync.yml` **Purpose:** @@ -440,11 +486,11 @@ Total Failed Deletions: 1 } ``` -5. **Delete Stale Users** +5. **Delete Stale Users** — soft, matching the job. ```typescript const deleteResponse = await serverStreamClient.deleteUsers( staleUsersInPage, - { user: "hard", messages: "hard" }, + { user: "soft", messages: "soft" }, ); ``` @@ -460,35 +506,73 @@ The following users are NEVER deleted: **Hardcoded Exclusions:** ```typescript -const EXCLUDED_USER_IDS = new Set(["system", "teetangh"]); +const EXCLUDED_USER_IDS = new Set(["system"]); ``` +Plus anything listed in the `STREAM_SYNC_EXCLUDED_USERS` environment variable. +This document previously showed a personal account hardcoded alongside +`"system"`; it is not in the code and must not be — an operator's own account +being un-reapable is a footgun, and the env var is the supported way to add one +temporarily. + **Reason for Exclusions:** - System accounts are required for Stream functionality - Recording egress users handle video recording and storage -- Hardcoded users are critical administrator or service accounts ### Deletion Strategy -**Hard Delete:** +**Soft delete.** This section used to document hard delete as the strategy and +soft as the alternative. The code does the opposite: ```typescript { - user: "hard", // Permanently delete user - messages: "hard" // Delete all user messages + user: "soft", // Intended 30-day grace period; expiry is NOT enforced yet + messages: "soft" } ``` +There is no hard-delete follow-up job yet — the `TODO` in +`scripts/stream/stream-sync.ts` is tracked as #535. + +**What that means concretely, for a DPDP §12 erasure request.** +`lib/compliance/erasure/scrub-user.ts` pseudonymises the local `User` row and +makes no Stream call at all. Stream-side removal is therefore incidental: the +nightly reaper notices the local row is gone and issues a *soft* delete, which +is the state the data then stays in. + +- **Retention window:** the soft delete is described in the code as a 30-day + grace period, but nothing expires it, because the job that would is #535. In + practice the retention is indefinite. +- **Who can still read it:** nobody through the app — a soft-deleted Stream user + cannot connect, and their messages are hidden from clients. It remains + readable by anything holding the API secret: the Stream dashboard, a data + export, and our own server-side clients. +- **Should erasure trigger a hard delete?** Yes, and it does not today. The + correct shape is for the erasure path to call Stream directly with + `{ user: "hard", messages: "hard" }` rather than waiting for a reaper whose + input is "absent from Postgres" — an erasure is a specific, authorised + instruction about one identified person, which is exactly the case where hard + delete is safe and the reaper's inference is not. Deferred to #535; until it + lands, an erasure request that must be provably complete has to be finished by + hand in the Stream dashboard. + **Alternative Options:** ```typescript { - user: "soft", // Mark as deleted but keep data - messages: "soft" // Mark messages as deleted + user: "hard", // Irreversible; purges the user and their data + messages: "hard" } ``` +Hard delete is what #535 will eventually run as a second pass over rows the soft +delete has already aged out. It is deliberately not what the nightly reaper does: +the reaper's input is "present in Stream, absent from Postgres", and that set +includes anyone the local database has merely failed to return — a bad migration, +a partial restore, a query bug. Soft-deleting that is a bad day; hard-deleting it +is unrecoverable. + ### Error Handling **Failed Deletions:** diff --git a/hooks/useUserData.ts b/hooks/useUserData.ts index 806de4a69..c592f4cd9 100644 --- a/hooks/useUserData.ts +++ b/hooks/useUserData.ts @@ -3,24 +3,22 @@ import { useState, useEffect } from "react"; import { reportSentryError } from "@/lib/observability/report"; import { useToast } from "@/components/ui/use-toast"; -import { TConsultantProfile } from "@/types/consultant"; -import { TConsulteeProfile } from "@/types/consultee"; -import { TStaffProfile } from "@/types/staff"; -import { User, ConsultantReview } from "@prisma/client"; -import { - fetchUserDetails, - fetchConsultantDetails, - fetchConsulteeDetails, - fetchStaffDetails, - fetchReviews, -} from "@/lib/user"; +import { User } from "@prisma/client"; +import { fetchUserDetails } from "@/lib/user"; +/** + * The signed-in user's core record, for the Stream connect. + * + * Deliberately narrow. This hook gates the chat socket — `StreamProviderImpl` + * will not connect until `isLoading` clears — so every fetch added here is + * time the Messages tab spends on a skeleton. It fetches one thing. + * + * `fetchConsultantDetails`, `fetchConsulteeDetails`, `fetchStaffDetails` and + * `fetchReviews` all still live in `@/lib/user` for callers that genuinely + * need them; they simply have no business on this path. + */ export const useUserData = (userId: string) => { const [userDetails, setUserDetails] = useState(null); - const [profileDetails, setProfileDetails] = useState< - TConsultantProfile | TConsulteeProfile | TStaffProfile | null - >(null); - const [reviews, setReviews] = useState([]); const [isLoading, setIsLoading] = useState(true); const [error, setError] = useState(null); const { toast } = useToast(); @@ -33,44 +31,34 @@ export const useUserData = (userId: string) => { const userData = await fetchUserDetails(userId); setUserDetails(userData); - switch (userData.role) { - case "CONSULTANT": - if (userData.consultantProfileId) { - const consultantData = await fetchConsultantDetails( - userData.consultantProfileId, - ); - setProfileDetails(consultantData); - const reviewsData = await fetchReviews( - userData.consultantProfileId, - ); - setReviews(reviewsData); - } - break; - case "CONSULTEE": - if (userData.consulteeProfileId) { - const consulteeData = await fetchConsulteeDetails( - userData.consulteeProfileId, - ); - setProfileDetails(consulteeData); - } - break; - case "STAFF": - if (userData.staffProfileId) { - const staffData = await fetchStaffDetails( - userData.staffProfileId, - ); - setProfileDetails(staffData); - } - break; - default: - // Handle other roles or no role - break; - } + // The per-role profile fetches that used to follow are gone. + // + // This hook has exactly one consumer — `providers/StreamProviderImpl` + // — and it destructures `{ userDetails, isLoading }` only. The + // `profileDetails` and `reviews` it also returned were read by nobody, + // yet a CONSULTANT paid for them SERIALLY (consultant details, then + // reviews) before `isLoading` cleared, and the Stream connect is hard- + // gated on `isLoading`. Two round-trips to a cold serverless function + // and a remote database, on the critical path to the chat socket, for + // data that was discarded on arrival — which is most of why the + // Messages tab sat on a skeleton. + // + // The connect reads `id`, `name`, `image` and `role`, all of which come + // from `fetchUserDetails` above. If a future caller needs the profile, + // fetch it where it is used rather than reinstating it here: this hook + // gates a socket, so anything added to it delays chat for everyone. } catch (err: unknown) { const error = err instanceof Error ? err : new Error(String(err)); setError(error); // 401 is expected after sign-out (session cleared before component unmounts) - if (err !== null && err !== undefined && typeof err === "object" && "status" in err && (err as { status: number }).status === 401) return; + if ( + err !== null && + err !== undefined && + typeof err === "object" && + "status" in err && + (err as { status: number }).status === 401 + ) + return; reportSentryError(error, { subsystem: "client" }); console.error("Error fetching user details:", err); toast({ @@ -88,5 +76,5 @@ export const useUserData = (userId: string) => { } }, [userId, toast]); - return { userDetails, profileDetails, reviews, isLoading, error }; + return { userDetails, isLoading, error }; }; diff --git a/lib/stream-channel-ids.ts b/lib/stream-channel-ids.ts index e5ac04895..16bd8da8a 100644 --- a/lib/stream-channel-ids.ts +++ b/lib/stream-channel-ids.ts @@ -6,6 +6,18 @@ // Channel ID prefixes export const DM_PREFIX = "dm-"; +/** + * Org-scoped and overflow DM forms, both minted by `getDmChannelId`. + * + * These are NOT covered by `DM_PREFIX`: `"dmo-".startsWith("dm-")` is false, so + * before they were declared here `getChannelTypeFromId` resolved every + * org-context DM as `team` — a channel created as `messaging` — and + * `MANAGED_CHANNEL_PREFIXES` skipped them entirely, so the reconciler never + * touched them. Any new DM shape must be added here as well as in + * `lib/stream-utils.ts`, or it silently falls into the `team` arm below. + */ +export const DM_ORG_PREFIX = "dmo-"; +export const DM_HASHED_PREFIX = "dmh-"; export const WEBINAR_PREFIX = "webinar-"; export const CLASS_PREFIX = "class-"; export const COLLAB_PREFIX = "collab-"; @@ -26,11 +38,24 @@ export const COLLAB_PREFIX = "collab-"; export const CONSULTATION_PREFIX = "consultation-"; export const SUBSCRIPTION_PREFIX = "subscription-"; -/** Prefixes managed by syncUserEventChannels for reconciliation */ +/** + * Prefixes managed by syncUserEventChannels for reconciliation. + * + * Adding a prefix here makes the reconciler REMOVE the user from any channel + * carrying it that is absent from `expectedChannelIds`. So a prefix only + * belongs here once `getDmPairsForUser` / `getWebinarIdsForUser` / + * `getClassIdsForUser` are guaranteed to produce every legitimate id under it — + * otherwise the sweep deletes live conversations, which is #1134 P0-7 all over + * again. `dmo-`/`dmh-` are safe here because `getDmPairsForUser` derives its + * ids through the same `getDmChannelId` helper that mints them, over the same + * `DM_ELIGIBLE_STATUSES` set the eligibility gate uses. + */ export const MANAGED_CHANNEL_PREFIXES = [ WEBINAR_PREFIX, CLASS_PREFIX, DM_PREFIX, + DM_ORG_PREFIX, + DM_HASHED_PREFIX, ] as const; /** Check if channel is a webinar or class event channel (group/team) */ @@ -42,10 +67,14 @@ export function isEventChannel(channelId: string | undefined): boolean { ); } -/** Check if channel is a direct message */ +/** Check if channel is a direct message, in any of its three id forms */ export function isDMChannel(channelId: string | undefined): boolean { if (!channelId) return false; - return channelId.startsWith(DM_PREFIX); + return ( + channelId.startsWith(DM_PREFIX) || + channelId.startsWith(DM_ORG_PREFIX) || + channelId.startsWith(DM_HASHED_PREFIX) + ); } /** Check if channel is a collaborator channel */ @@ -58,14 +87,20 @@ export function isCollaboratorChannel( /** * Infer Stream channel type from channel ID prefix. - * DM, collaborator, consultation, and subscription channels use "messaging". - * Event channels (webinar, class) use "team". + * DM (all three forms), collaborator, consultation, and subscription channels + * use "messaging". Event channels (webinar, class) use "team". + * + * The `team` return is a FALLBACK, not a match — an unrecognised prefix lands + * there. That is why `dmo-` resolved as `team` for as long as it went + * undeclared: nothing errors, the wrong type is simply handed to + * `client.channel(type, id)`, which then addresses a channel that does not + * exist and 404s or creates a second one. */ export function getChannelTypeFromId( channelId: string, ): "messaging" | "team" { if ( - channelId.startsWith(DM_PREFIX) || + isDMChannel(channelId) || channelId.startsWith(COLLAB_PREFIX) || channelId.startsWith(CONSULTATION_PREFIX) || channelId.startsWith(SUBSCRIPTION_PREFIX) diff --git a/lib/stream-utils.ts b/lib/stream-utils.ts index 0da006e54..6b241e707 100644 --- a/lib/stream-utils.ts +++ b/lib/stream-utils.ts @@ -70,6 +70,20 @@ export function getDmChannelId( userId2: string, organizationId?: string | null, ): string { + // A pair of one is not a pair. `createChannel` de-duplicates its member array + // through a Set, so `[a, a]` silently collapsed to a one-member `messaging` + // channel that rendered as its own raw id (channelUtils has no branch for + // zero counterparties) and that nothing could ever reply in. Checkout already + // refuses self-booking, so reaching here with a self-pair means a caller + // resolved the two sides wrongly — which is worth a stack trace, not a + // channel. Callers that iterate over untrusted pair lists must filter first; + // `getDmPairsForUser` does. + if (userId1 === userId2) { + throw new Error( + `getDmChannelId: refusing to derive a self-DM id for ${userId1}`, + ); + } + // Code-unit ordering, NOT localeCompare. #1134 P0-3: localeCompare sorts by // ICU collation — case-insensitive at the primary level and dependent on the // runtime's ICU build and default locale — so the same pair yielded different diff --git a/lib/stream/dm-eligibility-statuses.ts b/lib/stream/dm-eligibility-statuses.ts new file mode 100644 index 000000000..9565a5f49 --- /dev/null +++ b/lib/stream/dm-eligibility-statuses.ts @@ -0,0 +1,68 @@ +/** + * The booking statuses that constitute a direct-message relationship. + * + * Split out from `dm-eligibility.ts` deliberately: that module imports Prisma, + * and this constant is needed by callers that must NOT drag a database client + * into their module graph — most sharply the Jest suites, where importing it + * transitively evaluated `@/lib/prisma` before the hoisted `jest.mock` factory's + * `mockPrisma` binding was initialized, failing the whole suite with a temporal + * dead-zone error rather than a test failure. + * + * Keeping it here also makes the layering honest. This file is the POLICY — + * which bookings count — and it is pure data. `dm-eligibility.ts` is the + * MECHANISM that answers the question against the database. + */ +import type { AppointmentStatus } from "@prisma/client"; + +/** + * "Ever transacted" — a booking that reached any of these once is a permanent + * link. Deliberately includes COMPLETED: the conversation about a session is + * often most valuable after it, and a thread that goes read-only the moment the + * last appointment ends strands both parties mid-exchange. + * + * Deliberately EXCLUDES `PENDING`: a request the consultant has not accepted is + * not yet a relationship, or anyone could open a channel with anyone by + * requesting a booking they never intend to pay for. + * + * **Changing this changes what the reconciler deletes.** + * `syncUserEventChannels` treats every `dm-`/`dmo-`/`dmh-` channel absent from + * its expected set as stale and removes the user from it, and that set is built + * from this constant. Narrow it and you evict people from live threads; widen + * it without a create path for the new statuses and you leave search results + * that no channel backs. Every consumer — the gate, both search routes, and + * `getDmPairsForUser` — reads this and only this. + */ +export const DM_ELIGIBLE_STATUSES: readonly AppointmentStatus[] = [ + "APPROVED", + "APPROVED_PENDING_PAYMENT", + "SCHEDULED", + "COMPLETED", +] as const; + +/** + * Reusable `where` fragment so callers cannot drift from the constant. + * + * Built fresh on each call rather than exported as a shared object: Prisma + * `where` fragments get spread into query objects all over the codebase, and a + * frozen-looking module-level constant is one careless `filter.in.push()` away + * from mutating every other caller's query. + */ +export function dmEligibleStatusFilter(): { in: AppointmentStatus[] } { + return { in: [...DM_ELIGIBLE_STATUSES] }; +} + +/** + * The event (webinar/class) states whose shared team channel can still be + * opened. Lives here rather than in the two routes that need it — the search + * route decides which rows are OFFERED and the open route decides which are + * CLICKABLE, and any disagreement between those two is either a dead row or a + * wider-than-search authorization surface. + * + * Deliberately narrower than the DM set: CANCELLED events must not be + * openable, and DRAFT is author-only. + */ +export const OPENABLE_EVENT_STATUSES = [ + "SCHEDULED", + "IN_PROGRESS", + "COMPLETED", +] as const; diff --git a/lib/stream/dm-eligibility.ts b/lib/stream/dm-eligibility.ts new file mode 100644 index 000000000..acffca272 --- /dev/null +++ b/lib/stream/dm-eligibility.ts @@ -0,0 +1,280 @@ +/** + * The single answer to "may these two people hold a direct message?". + * + * Before this module the question had three different answers living in three + * places, and the disagreement was load-bearing rather than cosmetic: + * + * `checkUserRelationship` APPROVED, SCHEDULED (+ subscription window open) + * `getDmPairsForUser` APPROVED, SCHEDULED + * the two search routes APPROVED, APPROVED_PENDING_PAYMENT, SCHEDULED, COMPLETED + * + * Search was the widest, so it surfaced conversations for bookings the other two + * did not consider real. Clicking one of those rows asked the client for a + * channel that had never been created, and `channel.watch()` — which posts to + * the same endpoint `channel.create()` does — created it on the spot, with the + * caller as `created_by` and NOT as a member. That is the memberless + * `dm-…`-titled thread that accepts a message and then disappears on refresh, + * because the sidebar queries `{ members: { $in: [me] } }`. + * + * So the sets are unified here, at the widest of the three, and every consumer + * imports from this file. This is a plain module, not a `"use server"` one, on + * purpose: server-action files may only export async functions, which is why + * `DM_ELIGIBLE_STATUSES` could not previously be shared. + * + * **Widening or narrowing `DM_ELIGIBLE_STATUSES` changes what the reconciler + * deletes.** `syncUserEventChannels` treats every `dm-`/`dmo-`/`dmh-` channel + * absent from the expected set as stale and removes the user from it. The + * expected set is built from this constant. Narrow it and you evict people from + * live threads; widen it without re-running the create path and you leave rows + * the search can find but no channel backs. + */ +import prisma from "@/lib/prisma"; +import { bookingOrgId } from "@/lib/stream-utils"; +import { + DM_ELIGIBLE_STATUSES, + dmEligibleStatusFilter, +} from "@/lib/stream/dm-eligibility-statuses"; + +// Re-exported so consumers that need both the policy and the gate have one +// import. Consumers that need ONLY the constant should import it from +// `dm-eligibility-statuses` directly — this module pulls in Prisma. +export { DM_ELIGIBLE_STATUSES, dmEligibleStatusFilter }; + +type ProfileIds = { + consultantProfileId: string | null; + consulteeProfileId: string | null; +}; + +/** + * The (consultant, consultee) pairings these two people could form. + * + * Both directions, because one person may hold both profiles — a consultant who + * also books sessions as a consultee is an ordinary account shape here, not an + * edge case, and reading only one direction silently denied half of those pairs. + * Returns zero, one or two pairings; zero means no relationship is even + * expressible and the caller can skip its query entirely. + * + * Shared by both link checks. The two used to carry byte-identical copies of + * this, differing only in the Prisma model they then queried, which is precisely + * the setup where a fix lands in one copy and not the other. + */ +function buildDirections( + a: ProfileIds, + b: ProfileIds, +): Array<{ consultant: string; consultee: string }> { + const directions: Array<{ consultant: string; consultee: string }> = []; + if (a.consultantProfileId && b.consulteeProfileId) { + directions.push({ + consultant: a.consultantProfileId, + consultee: b.consulteeProfileId, + }); + } + if (b.consultantProfileId && a.consulteeProfileId) { + directions.push({ + consultant: b.consultantProfileId, + consultee: a.consulteeProfileId, + }); + } + return directions; +} + +/** A consultation links the pair if either direction holds. */ +async function hasConsultationLink( + a: ProfileIds, + b: ProfileIds, +): Promise { + const directions = buildDirections(a, b); + if (directions.length === 0) return false; + + const hit = await prisma.consultation.findFirst({ + where: { + status: dmEligibleStatusFilter(), + OR: directions.map((d) => ({ + consultationPlan: { consultantProfileId: d.consultant }, + requestedById: d.consultee, + })), + }, + select: { id: true }, + }); + return !!hit; +} + +/** + * Same shape for subscriptions, minus the scheduling window. + * + * `schedulingPeriodEndsAt: { gte: now }` used to be part of this check. Under + * "ever transacted" it must not be: a lapsed subscription is still a + * relationship that happened, and gating on the window meant a thread went + * unreachable at midnight on the renewal date — mid-conversation, with no + * notice to either party, and with the reconciler then classifying the channel + * stale and evicting them from their own history. + */ +async function hasSubscriptionLink( + a: ProfileIds, + b: ProfileIds, +): Promise { + const directions = buildDirections(a, b); + if (directions.length === 0) return false; + + const hit = await prisma.subscription.findFirst({ + where: { + status: dmEligibleStatusFilter(), + OR: directions.map((d) => ({ + subscriptionPlan: { consultantProfileId: d.consultant }, + requestedById: d.consultee, + })), + }, + select: { id: true }, + }); + return !!hit; +} + +/** + * May `userIdA` and `userIdB` hold a direct message channel? + * + * Throws rather than returning false when the lookup itself fails. The previous + * implementation caught everything and returned `false`, which reads as + * "fail closed" and is the opposite in practice: as an unused advisory flag it + * was harmless, but as the gate on channel creation a transient DB blip would + * deny a legitimate conversation and — worse, on the reconcile path — make a + * live channel look unexpected. A gate that cannot evaluate must say so. + * + * The group arm (`hasSharedSlot`) was REMOVED from this gate deliberately + * (CodeRabbit + design review on PR #1188): it made any two attendees of one + * event mutually eligible, which the safety note below used to wave off as + * "fine because no code path offers a consultee a way to open a DM" — but this + * PR itself adds exactly such a path (`POST /api/stream/channels/open`, + * reachable by any authenticated account, attendee ids readable from team + * channel member state). Peer DMs are forbidden by + * `docs/decisions/2026-07-11-moderation-enforcement-and-peer-chat-block.md`. + * Host↔attendee DMs are not offered by any surface either: the event arm of + * the open route opens the shared team channel, not a DM. If a product need + * for host↔attendee DMs appears, reintroduce the arm behind an explicit + * host-check inside `hasSharedSlot`, not as blanket co-membership. + */ +export async function canDirectMessage( + userIdA: string, + userIdB: string, +): Promise { + if (!userIdA || !userIdB) return false; + // Cheapest check first, and the one the screenshot was about. + if (userIdA === userIdB) return false; + + const [a, b] = await Promise.all([ + prisma.user.findUnique({ + where: { id: userIdA }, + select: { consultantProfileId: true, consulteeProfileId: true }, + }), + prisma.user.findUnique({ + where: { id: userIdB }, + select: { consultantProfileId: true, consulteeProfileId: true }, + }), + ]); + if (!a || !b) return false; + + const results = await Promise.all([ + hasConsultationLink(a, b), + hasSubscriptionLink(a, b), + ]); + return results.some(Boolean); +} + +/** Thrown when a DM is requested between two users with no booking link. */ +export class DmNotPermittedError extends Error { + constructor(userIdA: string, userIdB: string) { + super( + "Direct messages are only available between people who share a booking.", + ); + this.name = "DmNotPermittedError"; + this.userIdA = userIdA; + this.userIdB = userIdB; + } + readonly userIdA: string; + readonly userIdB: string; +} + +/** `canDirectMessage`, as an assertion. */ +export async function assertCanDirectMessage( + userIdA: string, + userIdB: string, +): Promise { + if (!(await canDirectMessage(userIdA, userIdB))) { + throw new DmNotPermittedError(userIdA, userIdB); + } +} + +/** + * The funding contexts (`bookingOrgId` values) under which this pair's eligible + * bookings actually live — `null` included when a personal-context booking + * exists. + * + * The open route uses this to validate the client-supplied `organizationId`. + * The channel id is re-derived server-side, but the id is a function of the + * pair AND the funding context, so a caller able to name an arbitrary org + * could mint `dmo--…` channels tagged to an organization it + * has no relation to. Contexts are read from the SAME rows the gate and the + * reconciler's expected-set use, so what the client is allowed to name is + * exactly what the reconciler will keep alive. + */ +export async function pairBookingContexts( + userIdA: string, + userIdB: string, +): Promise<{ personalAllowed: boolean; organizations: string[] }> { + const [a, b] = await Promise.all([ + prisma.user.findUnique({ + where: { id: userIdA }, + select: { consultantProfileId: true, consulteeProfileId: true }, + }), + prisma.user.findUnique({ + where: { id: userIdB }, + select: { consultantProfileId: true, consulteeProfileId: true }, + }), + ]); + if (!a || !b) return { personalAllowed: false, organizations: [] }; + + const directions = buildDirections(a, b); + if (directions.length === 0) return { personalAllowed: false, organizations: [] }; + + const [consultations, subscriptions] = await Promise.all([ + prisma.consultation.findMany({ + where: { + status: dmEligibleStatusFilter(), + OR: directions.map((d) => ({ + consultationPlan: { consultantProfileId: d.consultant }, + requestedById: d.consultee, + })), + }, + select: { + consultationPlan: { select: { organizationId: true } }, + appointment: { select: { organizationId: true } }, + }, + }), + prisma.subscription.findMany({ + where: { + status: dmEligibleStatusFilter(), + OR: directions.map((d) => ({ + subscriptionPlan: { consultantProfileId: d.consultant }, + requestedById: d.consultee, + })), + }, + select: { + subscriptionPlan: { select: { organizationId: true } }, + appointments: { select: { organizationId: true } }, + }, + }), + ]); + + const organizations = new Set(); + let personalAllowed = false; + for (const c of consultations) { + const org = bookingOrgId(c); + if (org === null) personalAllowed = true; + else organizations.add(org); + } + for (const s of subscriptions) { + const org = bookingOrgId(s); + if (org === null) personalAllowed = true; + else organizations.add(org); + } + return { personalAllowed, organizations: Array.from(organizations) }; +} diff --git a/providers/StreamProviderImpl.tsx b/providers/StreamProviderImpl.tsx index 7e7c3dd4e..b957590d6 100644 --- a/providers/StreamProviderImpl.tsx +++ b/providers/StreamProviderImpl.tsx @@ -564,7 +564,19 @@ const StreamProviderImpl = ({ } }; - // Defer the connect off the critical path (#248). + // Defer the connect off the critical path (#248) — but only briefly. + // + // The timeout was 2000ms, and on a cold load that is not a ceiling, it is + // the ACTUAL wait: the main thread is saturated by dashboard hydration and + // by evaluating the Stream Chat + Video chunk, so the browser never finds + // an idle period and fires at the deadline every time. Two seconds of the + // Messages skeleton were this line. + // + // The deferral still earns its place — it keeps the socket handshake from + // competing with first paint of the dashboard behind it — so it stays, at a + // budget that yields to hydration without becoming the dominant cost. If + // this ever needs tuning again, measure with `streamLogger.timing()` rather + // than guessing; it warns above 5s and currently has no callers. if (typeof window !== "undefined" && "requestIdleCallback" in window) { idleHandle = ( window as Window & { @@ -573,7 +585,7 @@ const StreamProviderImpl = ({ opts?: { timeout: number }, ) => number; } - ).requestIdleCallback(run, { timeout: 2000 }); + ).requestIdleCallback(run, { timeout: 300 }); } else { timeoutHandle = setTimeout(run, 0); } diff --git a/schemas/stream-search.ts b/schemas/stream-search.ts index 27a3cc599..706999229 100644 --- a/schemas/stream-search.ts +++ b/schemas/stream-search.ts @@ -30,8 +30,27 @@ export const AppointmentSearchResultSchema = z.object({ id: z.string(), type: StreamSearchKindSchema, name: z.string(), - consultantName: z.string(), - consultantImage: z.string().optional(), + /** + * The OTHER party, relative to the caller — not the consultant. + * + * These replace `consultantName`/`consultantImage`, which named the + * consultant unconditionally. On a consultant's own dashboard that is the + * consultant themselves, so every row in their search read as a conversation + * with themselves. The name is now resolved per-caller in the handler. + * + * For a group event there is no single counterparty, so this is the host. + */ + counterpartyName: z.string(), + counterpartyImage: z.string().optional(), + /** + * Present only for 1:1 rows. The client sends this to + * `POST /api/stream/channels/open`, which re-derives the channel id + * server-side — the id below is for display and cache lookup, never the + * authority on which channel gets opened. + */ + counterpartyUserId: z.string().optional(), + /** Funding context, part of the DM key. Null/absent = personal. */ + organizationId: z.string().nullable().optional(), channelId: z.string(), }); diff --git a/scripts/stream/ensure-chat-type-grants.ts b/scripts/stream/ensure-chat-type-grants.ts new file mode 100644 index 000000000..d8bc5d2f0 --- /dev/null +++ b/scripts/stream/ensure-chat-type-grants.ts @@ -0,0 +1,442 @@ +/** + * Move channel creation and membership changes off the client, on Stream's side. + * + * The chat counterpart of `ensure-call-type-grants.ts`. That script found that + * Stream's `default` call type grants `join-call` to the plain `user` role, so + * any signed-in account could join any call by id. The chat side has the same + * shape of hole and nothing in this repo has ever touched it: the only + * `updateAppSettings` call we make is `ensure-webhook-subscription.ts`, and it + * writes `event_hooks` alone. Chat channel-type permissions are therefore + * whatever Stream ships by default, which for `messaging` and `team` includes + * `create-channel` for `user`. + * + * That default is what let a browser mint a channel. `ChannelSearch` called + * `channel.watch()` on a client-computed id; `watch()` posts to the channel + * query endpoint, which creates the channel when it is absent, with the caller + * as `created_by` and no members. The result was a conversation titled with its + * own raw id, reporting "No members", that accepted a message and vanished on + * reload. `POST /api/stream/channels/open` fixes the app path. This closes the + * door behind it, so a leaked token or a future component cannot reopen it. + * + * `guest` matters as much as `user`, for the same reason the call-type script + * gives: the app has `guest_user_creation_disabled: false`, so guest sessions + * are creatable client-side with nothing but the public API key we ship as + * `NEXT_PUBLIC_STREAM_API_KEY`. Stripping only `user` would leave the hole open + * behind a fix that claims to close it. + * + * Also sets `user_search_disallowed_roles`, which stops a client-side + * `queryUsers` from enumerating the user base. Stream's own docs note that + * `queryUsers` requires no special permission by default. + * + * Existing channels are unaffected — grants are evaluated per request against + * the channel TYPE, not baked in at creation. That is the opposite of the call + * type's immutability problem, and it is why this hardens the built-in types in + * place rather than minting bespoke ones. + * + * Idempotent and reversible. Dry-run is the default. + * + * npx tsx scripts/stream/ensure-chat-type-grants.ts + * npx tsx scripts/stream/ensure-chat-type-grants.ts --apply --open-route-is-deployed + * npx tsx scripts/stream/ensure-chat-type-grants.ts --apply --restore-user-create + * npx tsx scripts/stream/ensure-chat-type-grants.ts --apply --rebaseline + * + * NOTE: dev, preview and production share one Stream app. A dry run here reads + * production. An apply writes it. + */ +import "dotenv/config"; +import { + existsSync, + mkdirSync, + readFileSync, + renameSync, + writeFileSync, +} from "node:fs"; +import { dirname, join } from "node:path"; + +import { + getStreamChatClient, + isStreamConfigured, +} from "../../lib/stream-client"; + +/** The two built-in channel types this app uses. Nothing else is in play. */ +const CHANNEL_TYPES = ["messaging", "team"] as const; + +/** + * Roles that lose the ability to create channels and rewrite membership. + * + * Not `admin`: ADMIN/STAFF map to Stream's `admin` role via `mapRoleToStream`, + * and the support surfaces rely on it. Not `channel_moderator` either — an + * event host holds it and legitimately manages their own roster through + * `addMemberToChannel`, which is server-side and bypasses these grants anyway. + */ +const REVOKED_ROLES = ["user", "guest"] as const; + +/** + * `create-channel` is the one that matters; `update-channel-members` is the + * follow-up. Without the second, a user who is already in a channel can still + * add anyone they like to it — which is how a private DM becomes a group. + * + * Stream's permission names are kebab-case in the grants arrays. + */ +const REVOKED_PERMISSIONS = [ + "create-channel", + "update-channel-members", +] as const; + +/** Roles barred from client-side `queryUsers`. Same reasoning as above. */ +const USER_SEARCH_DISALLOWED_ROLES = ["user", "guest"]; + +/** + * Where the pre-image lives. Committed to a stable, repo-relative path rather + * than a pid-suffixed temp file, because `--restore-user-create` reads it back + * on a LATER invocation — a path only the writing process could name made the + * rollback flag unusable in practice. + */ +const PRE_IMAGE_PATH = join( + process.cwd(), + ".stream-backups", + "chat-type-grants.json", +); + +interface Options { + apply: boolean; + restore: boolean; + deployConfirmed: boolean; + /** Overwrite an existing pre-image with the CURRENT live state. */ + rebaseline: boolean; +} + +function parseArgs(argv: string[]): Options { + return { + apply: argv.includes("--apply"), + restore: argv.includes("--restore-user-create"), + deployConfirmed: argv.includes("--open-route-is-deployed"), + rebaseline: argv.includes("--rebaseline"), + }; +} + +/** Code-unit ordering. Passed explicitly everywhere, never `localeCompare`. */ +function byCodeUnit(a: string, b: string): number { + return a < b ? -1 : a > b ? 1 : 0; +} + +/** + * Stable stringify for comparing two independent reads. + * + * Plain `JSON.stringify` preserves key insertion order, and the snapshots come + * from separate responses — so an identical configuration whose keys arrived in + * a different order would report as drift and tell the operator Stream + * discarded settings it never touched. A false alarm on this check is + * expensive: it is the thing that says whether a production config wipe just + * happened. + */ +function canonical(value: unknown): string { + return JSON.stringify(value, (_key, val) => + val && typeof val === "object" && !Array.isArray(val) + ? Object.fromEntries( + Object.entries(val as Record).sort(([a], [b]) => + byCodeUnit(a, b), + ), + ) + : val, + ); +} + +/** What we snapshot before writing, and read back to roll forward from. */ +interface PreImage { + capturedAt: string; + channelTypes: Record>; + userSearchDisallowedRoles: string[]; +} + +function readPreImage(): PreImage | null { + if (!existsSync(PRE_IMAGE_PATH)) return null; + try { + return JSON.parse(readFileSync(PRE_IMAGE_PATH, "utf8")) as PreImage; + } catch { + return null; + } +} + +/** + * Ordering against the deploy is load-bearing, and getting it wrong breaks chat + * for everyone. + * + * Applying revokes `create-channel` from `user`. After that, the ONLY thing + * that can bring a missing channel into existence for an ordinary account is + * `POST /api/stream/channels/open` (plus the booking-time server paths). Run + * this before that route is live and any conversation whose channel does not + * yet exist becomes unopenable — which is precisely the set of conversations + * the bug report was about. + * + * A post-apply read cannot catch it: the grants will be exactly as requested. + * What it cannot see is whether anything server-side is left that creates + * channels. So the operator asserts it, with a flag named after the assertion. + */ +function requireDeployConfirmation(opts: Options): boolean { + if (!opts.apply || opts.restore || opts.deployConfirmed) return true; + + console.error( + "\n🛑 Refusing to apply.\n" + + "\nThis revokes `create-channel` from the `user` and `guest` roles. After\n" + + "it, a channel that does not already exist can only be created server-side\n" + + "— by booking approval, payment success, or POST /api/stream/channels/open.\n" + + "If that route is not deployed and serving traffic RIGHT NOW, every search\n" + + "result whose channel was never created becomes a dead link.\n" + + "\nDeploy first. Confirm the route is live. Then re-run with:\n" + + " npx tsx scripts/stream/ensure-chat-type-grants.ts --apply --open-route-is-deployed\n" + + "\nIf you get it wrong, the rollback is:\n" + + " npx tsx scripts/stream/ensure-chat-type-grants.ts --apply --restore-user-create\n", + ); + return false; +} + +/** + * The grants this channel type should end up with. + * + * Restore returns the PRE-IMAGE verbatim, not "the current grants plus + * everything we might have revoked". The difference matters: an earlier version + * re-added every entry in `REVOKED_PERMISSIONS` to `user` and `guest` + * unconditionally, so rolling back granted permissions those roles may never + * have held. A rollback that can hand out more access than the change it is + * undoing is worse than no rollback — and this script's entire justification is + * that its production write is safely reversible. + * + * Restoring therefore requires a pre-image. If there is none, this returns null + * and the caller refuses rather than guessing. + */ +function computeGrants( + channelType: string, + existingGrants: Record, + opts: Options, + preImage: PreImage | null, +): Record | null { + if (opts.restore) { + return preImage?.channelTypes?.[channelType] ?? null; + } + + const grants: Record = Object.fromEntries( + Object.entries(existingGrants).map(([role, perms]) => [role, [...perms]]), + ); + + for (const role of REVOKED_ROLES) { + // A role absent from this type's grants map is not an error — Stream's + // built-in types do not all carry the same role keys, and inventing one + // would grant permissions rather than remove them. + if (!grants[role]) continue; + grants[role] = grants[role].filter( + (g) => !(REVOKED_PERMISSIONS as readonly string[]).includes(g), + ); + } + return grants; +} + +/** Per-role before/after table for the permissions this script touches. */ +function logGrantDiff( + channelType: string, + existingGrants: Record, + grants: Record, +): void { + console.log(`Channel type: ${channelType}`); + for (const role of REVOKED_ROLES) { + if (!existingGrants[role]) { + console.log(` ${role.padEnd(8)} (role absent on this channel type)`); + continue; + } + for (const perm of REVOKED_PERMISSIONS) { + const had = (existingGrants[role] ?? []).includes(perm); + const now = (grants[role] ?? []).includes(perm); + console.log(` ${role.padEnd(8)} ${perm.padEnd(24)} ${had} → ${now}`); + } + } +} + +/** + * App-level user-search lockdown. + * + * Restore reinstates the pre-image value, NOT `[]`. Writing an empty array on + * rollback would clear whatever the app already had before this script first + * ran — undoing someone else's setting in the name of undoing ours. + */ +async function syncUserSearchSetting( + client: ReturnType, + opts: Options, + preImage: PreImage | null, +): Promise { + // Asymmetry in stream-chat v9's types, not in the API: the field is declared + // on the READ shape (`AppSettingsAPIResponse.app`) but missing from the WRITE + // shape (`AppSettings`). Stream's own docs show it being written via + // `updateAppSettings`, so the write below carries a narrow cast. + const settings = await client.getAppSettings(); + const current = settings.app?.user_search_disallowed_roles ?? []; + + const desired = opts.restore + ? (preImage?.userSearchDisallowedRoles ?? null) + : USER_SEARCH_DISALLOWED_ROLES; + + if (desired === null) { + console.error( + "🛑 Cannot restore user_search_disallowed_roles — no pre-image on disk.", + ); + return false; + } + + if ( + canonical([...current].sort(byCodeUnit)) === + canonical([...desired].sort(byCodeUnit)) + ) { + console.log("✅ user_search_disallowed_roles already correct — no change"); + return false; + } + + console.log( + `App setting: user_search_disallowed_roles ` + + `[${current.join(", ")}] → [${desired.join(", ")}]`, + ); + if (opts.apply) { + // NOTE: `updateAppSettings` REPLACES the field it is given rather than + // merging — the trap documented at length in + // ensure-webhook-subscription.ts. Only this one key is passed, so the + // event_hooks that script manages are untouched. + await client.updateAppSettings({ + user_search_disallowed_roles: desired, + } as Parameters[0]); + console.log(" written"); + } + return true; +} + +export async function ensureChatTypeGrants(opts: Options): Promise { + // Before the read — a refusal should not depend on Stream being reachable. + if (!requireDeployConfirmation(opts)) return 1; + + if (!isStreamConfigured()) { + console.error( + "Stream is not configured — set STREAM_API_KEY and STREAM_API_SECRET", + ); + return 1; + } + + const preImage = readPreImage(); + if (opts.restore && !preImage) { + console.error( + `🛑 Cannot restore — no pre-image at ${PRE_IMAGE_PATH}.\n` + + "Restoring without one would mean guessing which permissions each role\n" + + "originally held, and guessing upward grants access nobody asked for.\n" + + "Reinstate the grants from the Stream dashboard instead.", + ); + return 1; + } + + const client = getStreamChatClient(); + + let changed = false; + const captured: PreImage = { + capturedAt: new Date().toISOString(), + channelTypes: {}, + userSearchDisallowedRoles: [], + }; + + // Snapshot everything BEFORE writing anything. The previous version wrote the + // pre-image after the last successful write, to a pid-suffixed path in + // tmpdir — so a run that failed halfway left no snapshot at all, and even a + // clean run left one that the next invocation could not find. A rollback file + // that only exists when nothing went wrong is not a rollback file. + for (const channelType of CHANNEL_TYPES) { + const existing = await client.getChannelType(channelType); + captured.channelTypes[channelType] = (existing.grants ?? {}) as Record< + string, + string[] + >; + } + const settingsProbe = await client.getAppSettings(); + captured.userSearchDisallowedRoles = + settingsProbe.app?.user_search_disallowed_roles ?? []; + + if (opts.apply && !opts.restore) { + // An existing pre-image is NEVER silently overwritten. + // + // Applying twice would otherwise capture the already-modified state as the + // rollback target — so the second run quietly redefines "before" as "after", + // and `--restore-user-create` becomes a no-op that reports success. The + // failure is invisible until the day someone actually needs to roll back. + // + // The second run does not need a new snapshot anyway: this script is + // idempotent, so by then the live state either already matches the desired + // one or the first pre-image is still the correct thing to return to. + if (existsSync(PRE_IMAGE_PATH) && !opts.rebaseline) { + console.log( + `Pre-image already exists at ${PRE_IMAGE_PATH} — keeping it.\n` + + `(pass --rebaseline to replace it with the current live state)\n`, + ); + } else { + mkdirSync(dirname(PRE_IMAGE_PATH), { recursive: true }); + // Write-then-rename, so an interrupted write cannot leave a truncated + // pre-image where a valid one used to be. Same directory, so the rename + // stays on one filesystem and is atomic. + const tmpPath = `${PRE_IMAGE_PATH}.tmp`; + writeFileSync(tmpPath, JSON.stringify(captured, null, 2)); + renameSync(tmpPath, PRE_IMAGE_PATH); + console.log(`Pre-image written to ${PRE_IMAGE_PATH}\n`); + } + } + + for (const channelType of CHANNEL_TYPES) { + const existingGrants = captured.channelTypes[channelType]; + const grants = computeGrants(channelType, existingGrants, opts, preImage); + + if (!grants) { + console.error( + `🛑 No pre-image entry for channel type "${channelType}" — skipping.`, + ); + continue; + } + + if (canonical(existingGrants) === canonical(grants)) { + console.log( + `✅ channel type "${channelType}" already correct — no change`, + ); + continue; + } + + logGrantDiff(channelType, existingGrants, grants); + changed = true; + + if (!opts.apply) continue; + + await client.updateChannelType(channelType, { grants }); + console.log(` written`); + } + + const settingChanged = await syncUserSearchSetting(client, opts, preImage); + changed = changed || settingChanged; + + if (!changed) return 0; + if (!opts.apply) { + console.log("\n(dry run — pass --apply to write)"); + // Drift detected but nothing written: a scheduled/CI consumer must be able + // to distinguish "in sync" from "drift found" by exit status. + process.exitCode = 1; + } + + return 0; +} + +async function main() { + const opts = parseArgs(process.argv.slice(2)); + console.log( + `Stream chat grants (${opts.apply ? "LIVE" : "DRY RUN"}${opts.restore ? ", RESTORE" : ""})...`, + ); + process.exit(await ensureChatTypeGrants(opts)); +} + +if ( + typeof require !== "undefined" && + typeof module !== "undefined" && + require.main === module +) { + main().catch((err) => { + console.error("Fatal error:", err); + process.exit(1); + }); +} diff --git a/scripts/stream/purge-memberless-dms.ts b/scripts/stream/purge-memberless-dms.ts new file mode 100644 index 000000000..c2b0e6f7e --- /dev/null +++ b/scripts/stream/purge-memberless-dms.ts @@ -0,0 +1,290 @@ +/** + * Delete phantom DM channels — `messaging` channels with fewer than two members. + * + * ## Where they came from + * + * `ChannelSearch` used to open a search result with + * `client.channel("messaging", id).watch()` on a browser-computed id. In + * `stream-chat`, `watch()` posts to the channel **query** endpoint, which is the + * same endpoint `create()` posts to — `channel.create()` is literally + * `query({ created_by_id })`. So watching an id that did not exist created it, + * with the caller as `created_by` and **no members**. + * + * The app paths that did that are gone, and `ensure-chat-type-grants.ts` revokes + * `create-channel` from `user`/`guest` so Stream itself refuses. Neither helps + * with the ones already on the app: a phantom is a real channel, it accepts real + * messages, and it is billed like any other. Dev, preview and production share + * one Stream app, so every environment's phantoms are in the same place. + * + * ## Why "< 2 members" is the right predicate + * + * A DM is a pair by construction. `getDmChannelId` refuses a self-pair and every + * server-side creator passes both ids in the `members` array atomically, so a + * healthy DM has exactly two. One member means the second was never added; zero + * means it was created by `watch()`. + * + * `team` channels are deliberately out of scope. A webinar channel legitimately + * sits at one member — its host — between creation and the first registration. + * + * And `messaging` alone is NOT sufficient to identify a DM. Collaborator + * channels (`collab--`) are `messaging` too, as are the + * admin-created custom ones, and a collab channel can legitimately sit at one + * member while co-host invitations are still pending. Deleting those would + * destroy live conversations in the name of cleaning up phantoms. The id prefix + * is the discriminator, via the same `isDMChannel` the app uses — which is why + * `dmo-` and `dmh-` had to be registered there first. + * + * ## Safety + * + * Dry run is the default and prints every candidate with its member count, + * message count and age. `--apply` deletes, hard, in batches. A pre-image of + * everything deleted is written first, to a stable path, so a mistake is + * reconstructible. + * + * npx tsx scripts/stream/purge-memberless-dms.ts + * npx tsx scripts/stream/purge-memberless-dms.ts --apply + * npx tsx scripts/stream/purge-memberless-dms.ts --apply --purge-with-messages + * + * NOTE: a dry run READS production. An apply DELETES from production. + */ +import "dotenv/config"; +import { mkdirSync, renameSync, writeFileSync } from "node:fs"; +import { dirname, join } from "node:path"; + +import { + getStreamChatClient, + isStreamConfigured, +} from "../../lib/stream-client"; +import { isDMChannel } from "../../lib/stream-channel-ids"; + +type StreamChatClient = ReturnType; + +/** + * Stream caps `queryChannels` at 30 per call regardless of the `limit` passed. + * A `do…while (page.length === PAGE_SIZE)` loop with PAGE_SIZE = 100 exits after + * one page and silently processes only the first 30 — the bug that made + * `syncUserEventChannels` reconcile a third of a roster. Page at the real cap. + */ +const PAGE_SIZE = 30; + +/** Stream's documented ceiling for `deleteChannels`. */ +const DELETE_BATCH = 100; + +/** Where the pre-image goes. Stable, not pid-suffixed, so a later run finds it. */ +const PRE_IMAGE_PATH = join( + process.cwd(), + ".stream-backups", + "purged-memberless-dms.json", +); + +interface Options { + apply: boolean; + /** Skip channels that hold messages, however broken they look. */ + purgeWithMessages: boolean; +} + +interface Candidate { + cid: string; + id: string; + memberIds: string[]; + createdById?: string; + createdAt?: string; + lastMessageAt?: string; +} + +function parseArgs(argv: string[]): Options { + return { + apply: argv.includes("--apply"), + purgeWithMessages: argv.includes("--purge-with-messages"), + }; +} + +/** + * Should this channel be deleted, and if so what do we record about it? + * + * Returns null for anything that must be left alone. Order matters: the prefix + * test comes before the member count, because only `dm-`/`dmo-`/`dmh-` ids are + * pair conversations. A `collab--` channel is `messaging` + * too and legitimately sits at one member while co-host invitations are pending + * — checking members first would put it on the delete list. + */ +function toCandidate( + channel: Awaited>[number], + opts: Options, +): Candidate | null { + if (!isDMChannel(channel.id)) return null; + + const memberIds = Object.keys(channel.state?.members ?? {}); + if (memberIds.length >= 2) return null; + + const messageCount = channel.state?.messages?.length ?? 0; + // Message-bearing channels are KEPT by default (PR #1188 review): a + // one-member DM can be the residue of a live conversation whose counterpart + // was evicted, and hard delete is irreversible. Only an explicit operator + // opt-in destroys history. + if (messageCount > 0 && !opts.purgeWithMessages) return null; + + const data = channel.data as Record | undefined; + return { + cid: channel.cid, + id: channel.id ?? "", + memberIds, + createdById: + typeof data?.created_by_id === "string" + ? data.created_by_id + : ((data?.created_by as { id?: string } | undefined)?.id ?? undefined), + createdAt: + typeof data?.created_at === "string" ? data.created_at : undefined, + lastMessageAt: + typeof data?.last_message_at === "string" + ? data.last_message_at + : undefined, + }; +} + +/** + * Page every `messaging` channel and collect the deletable ones. + * + * Filtered locally rather than server-side because Stream's channel filters + * have no `member_count` operator — `members: { $in: [...] }` needs ids we do + * not have, and there is no "fewer than N members" query. + */ +async function scanForCandidates( + client: StreamChatClient, + opts: Options, +): Promise<{ scanned: number; dmScanned: number; candidates: Candidate[] }> { + const candidates: Candidate[] = []; + let scanned = 0; + let dmScanned = 0; + let offset = 0; + + for (;;) { + const page = await client.queryChannels( + { type: "messaging" }, + { created_at: 1 }, + { limit: PAGE_SIZE, offset, state: true, watch: false, presence: false }, + ); + if (page.length === 0) break; + scanned += page.length; + + for (const channel of page) { + if (isDMChannel(channel.id)) dmScanned++; + const candidate = toCandidate(channel, opts); + if (candidate) candidates.push(candidate); + } + + offset += page.length; + if (page.length < PAGE_SIZE) break; + } + + return { scanned, dmScanned, candidates }; +} + +/** Print every candidate in full. This is what an operator reads before applying. */ +function reportCandidates( + candidates: Candidate[], + scanned: number, + dmScanned: number, +): void { + console.log( + `Scanned ${scanned} messaging channels (${dmScanned} were DM-prefixed).`, + ); + console.log(`Found ${candidates.length} with fewer than 2 members:\n`); + for (const c of candidates) { + console.log( + ` ${c.cid}\n` + + ` members: ${c.memberIds.length === 0 ? "(none)" : c.memberIds.join(", ")}\n` + + ` created_by: ${c.createdById ?? "unknown"} created: ${c.createdAt ?? "?"}\n` + + ` last_message_at: ${c.lastMessageAt ?? "never"}`, + ); + } +} + +/** Snapshot, then delete in batches. Returns how many were deleted. */ +async function deleteCandidates( + client: StreamChatClient, + candidates: Candidate[], +): Promise { + // Written BEFORE the delete. Unlike the grants script — where the pre-image + // only matters if a write succeeded — a delete is unrecoverable, so the + // snapshot has to exist even if the delete then fails halfway. + mkdirSync(dirname(PRE_IMAGE_PATH), { recursive: true }); + // Write-then-rename, matching ensure-chat-type-grants. An interrupted write + // would otherwise leave a truncated pre-image where a previous run's valid + // one used to be — losing the record of what an EARLIER purge deleted, which + // is the only thing that makes this reversible at all. + // Per-RUN file, not a shared path: a retry after a partial batch failure + // scans only the remaining channels, so overwriting one stable filename + // would replace the record of what the EARLIER batches deleted. Each run's + // snapshot is preserved alongside the others. + const runPreImagePath = `${PRE_IMAGE_PATH}.${new Date() + .toISOString() + .replace(/[:.]/g, "-")}.json`; + const tmpPath = `${runPreImagePath}.tmp`; + writeFileSync(tmpPath, JSON.stringify(candidates, null, 2)); + renameSync(tmpPath, runPreImagePath); + console.log(`\nPre-image written to ${runPreImagePath}`); + + let deleted = 0; + for (let i = 0; i < candidates.length; i += DELETE_BATCH) { + const batch = candidates.slice(i, i + DELETE_BATCH).map((c) => c.cid); + await client.deleteChannels(batch, { hard_delete: true }); + deleted += batch.length; + console.log(` deleted ${deleted}/${candidates.length}`); + } + return deleted; +} + +export async function purgeMemberlessDms( + opts: Options, +): Promise<{ scanned: number; candidates: number; deleted: number; ok: boolean }> { + if (!isStreamConfigured()) { + console.error( + "Stream is not configured — set STREAM_API_KEY and STREAM_API_SECRET", + ); + // A failed cleanup must not read as a completed no-op to CI or an operator. + return { scanned: 0, candidates: 0, deleted: 0, ok: false }; + } + + const client = getStreamChatClient(); + const { scanned, dmScanned, candidates } = await scanForCandidates( + client, + opts, + ); + + reportCandidates(candidates, scanned, dmScanned); + + if (candidates.length === 0) return { scanned, candidates: 0, deleted: 0, ok: true }; + + if (!opts.apply) { + console.log( + `\n(dry run — pass --apply to delete these ${candidates.length})`, + ); + return { scanned, candidates: candidates.length, deleted: 0, ok: true }; + } + + const deleted = await deleteCandidates(client, candidates); + return { scanned, candidates: candidates.length, deleted, ok: true }; +} + +async function main() { + const opts = parseArgs(process.argv.slice(2)); + console.log( + `Purging memberless DMs (${opts.apply ? "LIVE" : "DRY RUN"}` + + `${opts.purgeWithMessages ? ", INCLUDING any with messages" : ", keeping any with messages"})...`, + ); + const result = await purgeMemberlessDms(opts); + console.log("\nDone:", result); + process.exit(result.ok ? 0 : 1); +} + +if ( + typeof require !== "undefined" && + typeof module !== "undefined" && + require.main === module +) { + main().catch((err) => { + console.error("Fatal error:", err); + process.exit(1); + }); +}