diff --git a/.server-changes/plain-customer-card-external-id.md b/.server-changes/plain-customer-card-external-id.md new file mode 100644 index 0000000000..250ee1c06e --- /dev/null +++ b/.server-changes/plain-customer-card-external-id.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +Fixed support threads showing no account details for some customers, so the team can see your plan, organizations and projects when you get in touch. diff --git a/apps/webapp/app/routes/api.v1.plain.customer-cards.ts b/apps/webapp/app/routes/api.v1.plain.customer-cards.ts index 5e09fb0854..bfb9988bee 100644 --- a/apps/webapp/app/routes/api.v1.plain.customer-cards.ts +++ b/apps/webapp/app/routes/api.v1.plain.customer-cards.ts @@ -1,31 +1,15 @@ import { json, type ActionFunctionArgs } from "@remix-run/server-runtime"; import { timingSafeEqual } from "crypto"; import { uiComponent } from "@team-plain/ui-components"; -import { z } from "zod"; import { prisma } from "~/db.server"; import { env } from "~/env.server"; import { logger } from "~/services/logger.server"; import { generateImpersonationToken } from "~/services/impersonation.server"; - -// Schema for the request body from Plain -const PlainCustomerCardRequestSchema = z.object({ - cardKeys: z.array(z.string()), - customer: z - .object({ - id: z.string(), - email: z.string().optional(), - externalId: z.string().optional(), - }) - .refine((data) => data.email || data.externalId, { - message: "Either customer.email or customer.externalId must be provided", - path: ["customer"], - }), - thread: z - .object({ - id: z.string(), - }) - .optional(), -}); +import { + answerAllCardKeys, + emailLookupCandidates, + PlainCustomerCardRequestSchema, +} from "~/utils/plainCustomerCards"; function sanitizeHeaders( request: Request, @@ -133,22 +117,55 @@ export async function action({ request }: ActionFunctionArgs) { }, }; - const where = customer.externalId - ? { id: customer.externalId } - : customer.email - ? { email: customer.email } - : null; + // The external id is ours (`User.id`), so it's tried first. Falling back to email when it + // doesn't resolve covers a stale id — one naming a user row that no longer exists — instead of + // leaving the card blank for a customer we could still identify. + const byExternalId = customer.externalId + ? await prisma.user.findFirst({ where: { id: customer.externalId }, include: userInclude }) + : null; + + // Emails aren't stored consistently cased, so try the address as sent and then its lowercased + // form — see `emailLookupCandidates`. Both are exact matches on the unique index. + const findByEmail = async () => { + for (const email of emailLookupCandidates(customer.email)) { + const match = await prisma.user.findFirst({ where: { email }, include: userInclude }); + if (match) return match; + } + return null; + }; - const user = where ? await prisma.user.findFirst({ where, include: userInclude }) : null; + const user = byExternalId ?? (await findByEmail()); + + // An external id we set ourselves that no longer resolves is an anomaly worth seeing, even + // though the email match keeps the card useful — otherwise the stale link stays invisible. + if (customer.externalId && !byExternalId) { + logger.warn("Plain customer card external id did not resolve", { + resolvedByEmail: !!user, + }); + } - // If user not found, return empty cards + /** + * Impersonation is offered only when the customer matched on `externalId` — a value we set + * ourselves from `User.id`. + * + * Matching on email is a weaker claim: the address on a Plain customer isn't verified, and for + * customers created outside our own writes it comes from whoever sent the message. Offering a + * one-click impersonation link off the back of that would let an unverified address stand in + * for an account, so email-matched customers get the account rows without it. + * + * Derived from which lookup actually matched, not from whether an external id was *sent* — an + * id that misses and falls through to email must not unlock impersonation. + */ + const canImpersonate = Boolean(byExternalId); + + // No matching user: still answer every requested key, with no data so Plain hides the cards. if (!user) { + // Presence flags only — the identifiers themselves don't need to persist in log storage. logger.info("User not found for Plain customer card request", { - customerId: customer.id, - externalId: customer.externalId, + hasExternalId: !!customer.externalId, hasEmail: !!customer.email, }); - return json({ cards: [] }); + return json({ cards: answerAllCardKeys(cardKeys, []) }); } // Build cards based on requested cardKeys @@ -158,10 +175,21 @@ export async function action({ request }: ActionFunctionArgs) { for (const cardKey of cardKeys) { switch (cardKey) { case accountDetailsKey: { - // Generate a signed one-time token for impersonation - const impersonationToken = await generateImpersonationToken(user.id); - // Build the impersonate URL with token for CSRF protection - const impersonateUrl = `${env.APP_ORIGIN}/admin/impersonate?impersonate=${user.id}&impersonationToken=${encodeURIComponent(impersonationToken)}`; + // Only mint a token when the button will actually be rendered — see `canImpersonate`. + const impersonationComponents = canImpersonate + ? [ + uiComponent.spacer({ size: "M" }), + uiComponent.divider({ spacingSize: "M" }), + uiComponent.spacer({ size: "M" }), + uiComponent.linkButton({ + label: "Impersonate User", + // The one-time token is what protects this link against CSRF. + url: `${env.APP_ORIGIN}/admin/impersonate?impersonate=${user.id}&impersonationToken=${encodeURIComponent( + await generateImpersonationToken(user.id) + )}`, + }), + ] + : []; cards.push({ key: accountDetailsKey, @@ -241,13 +269,7 @@ export async function action({ request }: ActionFunctionArgs) { }), ], }), - uiComponent.spacer({ size: "M" }), - uiComponent.divider({ spacingSize: "M" }), - uiComponent.spacer({ size: "M" }), - uiComponent.linkButton({ - label: "Impersonate User", - url: impersonateUrl, - }), + ...impersonationComponents, ], }), ], @@ -420,13 +442,13 @@ export async function action({ request }: ActionFunctionArgs) { } default: - // Unknown card key - skip it + // Unknown card key - answered with no data by answerAllCardKeys below. logger.info("Unknown card key requested", { cardKey }); break; } } - return json({ cards }); + return json({ cards: answerAllCardKeys(cardKeys, cards) }); } catch (error) { logger.error("Error processing Plain customer card request", { error: error instanceof Error ? error.message : String(error), diff --git a/apps/webapp/app/utils/plainCustomerCards.test.ts b/apps/webapp/app/utils/plainCustomerCards.test.ts new file mode 100644 index 0000000000..4a9fcb4301 --- /dev/null +++ b/apps/webapp/app/utils/plainCustomerCards.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it } from "vitest"; +import { + answerAllCardKeys, + emailLookupCandidates, + PlainCustomerCardRequestSchema, +} from "./plainCustomerCards"; + +const request = (overrides: Record = {}) => ({ + cardKeys: ["account-details"], + customer: { id: "c_1", email: "dev@example.com", externalId: "user_1" }, + ...overrides, +}); + +describe("PlainCustomerCardRequestSchema", () => { + it("accepts a fully populated request", () => { + expect( + PlainCustomerCardRequestSchema.safeParse(request({ thread: { id: "th_1" } })).success + ).toBe(true); + }); + + // Plain sends explicit nulls rather than omitting these keys. Rejecting them meant every + // customer created outside our own writes got a 400 instead of a card. + it("accepts a null externalId when there is an email", () => { + const result = PlainCustomerCardRequestSchema.safeParse( + request({ customer: { id: "c_1", email: "dev@example.com", externalId: null } }) + ); + + expect(result.success).toBe(true); + }); + + it("accepts a null email when there is an externalId", () => { + const result = PlainCustomerCardRequestSchema.safeParse( + request({ customer: { id: "c_1", email: null, externalId: "user_1" } }) + ); + + expect(result.success).toBe(true); + }); + + it("accepts a null thread", () => { + expect(PlainCustomerCardRequestSchema.safeParse(request({ thread: null })).success).toBe(true); + }); + + it("accepts an omitted thread", () => { + expect(PlainCustomerCardRequestSchema.safeParse(request()).success).toBe(true); + }); + + // A contact created by an integration can have neither identifier. There's nothing to look up, + // but rejecting it would make Plain record an integration error rather than hide the card. + it("accepts a customer with neither email nor externalId", () => { + const result = PlainCustomerCardRequestSchema.safeParse( + request({ customer: { id: "c_1", email: null, externalId: null } }) + ); + + expect(result.success).toBe(true); + }); + + it("rejects a body with no card keys field", () => { + expect(PlainCustomerCardRequestSchema.safeParse({ customer: { id: "c_1" } }).success).toBe( + false + ); + }); +}); + +// `User.email` casing depends on the signup path: the SSO upsert lowercases, magic-link and OAuth +// store what the provider gave. Either candidate alone misses one of those populations. +describe("emailLookupCandidates", () => { + it("tries the address as sent before its lowercased form", () => { + // Finds a magic-link user stored with capitals, then an SSO user stored lowercased. + expect(emailLookupCandidates("Dev@Example.com")).toEqual([ + "Dev@Example.com", + "dev@example.com", + ]); + }); + + it("yields a single candidate when the address is already lowercase", () => { + expect(emailLookupCandidates("dev@example.com")).toEqual(["dev@example.com"]); + }); + + it("trims before comparing, so padding doesn't produce a duplicate candidate", () => { + expect(emailLookupCandidates(" dev@example.com ")).toEqual(["dev@example.com"]); + }); + + it("is empty for absent or blank addresses, so the lookup can be skipped", () => { + expect(emailLookupCandidates(null)).toEqual([]); + expect(emailLookupCandidates(undefined)).toEqual([]); + expect(emailLookupCandidates("")).toEqual([]); + expect(emailLookupCandidates(" ")).toEqual([]); + }); +}); + +describe("answerAllCardKeys", () => { + it("adds a no-data card for every unanswered key", () => { + expect(answerAllCardKeys(["a", "b"], [])).toEqual([ + { key: "a", components: null, timeToLiveSeconds: 60 }, + { key: "b", components: null, timeToLiveSeconds: 60 }, + ]); + }); + + it("leaves answered cards untouched", () => { + const answered = { key: "a", components: [{ componentText: { text: "hi" } }] }; + + expect(answerAllCardKeys(["a"], [answered])).toEqual([answered]); + }); + + it("fills only the gaps, keeping answered cards first", () => { + const answered = { key: "b", components: [] }; + + expect(answerAllCardKeys(["a", "b", "c"], [answered])).toEqual([ + answered, + { key: "a", components: null, timeToLiveSeconds: 60 }, + { key: "c", components: null, timeToLiveSeconds: 60 }, + ]); + }); + + // Omitting the TTL would fall back to the card's configured default, keeping an empty card in + // Plain's cache after the customer becomes resolvable. + it("caps how long an empty card is cached", () => { + const [filler] = answerAllCardKeys(["a"], []); + + expect(filler).toMatchObject({ timeToLiveSeconds: 60 }); + }); + + it("ignores extra cards that were not requested", () => { + const extra = { key: "unrequested", components: [] }; + + expect(answerAllCardKeys([], [extra])).toEqual([extra]); + }); +}); diff --git a/apps/webapp/app/utils/plainCustomerCards.ts b/apps/webapp/app/utils/plainCustomerCards.ts new file mode 100644 index 0000000000..96303b0dec --- /dev/null +++ b/apps/webapp/app/utils/plainCustomerCards.ts @@ -0,0 +1,92 @@ +import { z } from "zod"; + +/** + * The request Plain sends to a customer card endpoint. + * + * `email`, `externalId` and `thread` are nullish rather than optional because Plain sends these + * keys as explicit nulls rather than omitting them — `externalId` whenever the customer was + * created outside our own writes (its Slack integration, for one), `thread` when the card is + * loaded on the customer page rather than in a thread. `.optional()` accepts `undefined` but + * rejects `null`, which failed the whole request before any lookup could run. + */ +export const PlainCustomerCardRequestSchema = z.object({ + cardKeys: z.array(z.string()), + // A customer with neither an email nor an external id is valid input, not a malformed request: + // a contact created by an integration can legitimately have neither. There's nothing to look up, + // so the route answers every key with no data — rejecting it would make Plain record an + // integration error, which is the failure this schema change exists to remove. + customer: z.object({ + id: z.string(), + email: z.string().nullish(), + externalId: z.string().nullish(), + }), + thread: z + .object({ + id: z.string(), + }) + .nullish(), +}); + +export type PlainCustomerCardRequest = z.infer; + +/** + * The values to try, in order, when looking a user up by email. + * + * `User.email` is not stored consistently cased: the SSO upsert writes + * `email.toLowerCase().trim()`, while `findOrCreateMagicLinkUser` and the OAuth paths store + * whatever the provider gave us. So neither an exact match nor a lowercased one finds everybody — + * exact misses an SSO user whose address arrives capitalised, lowercased misses a magic-link user + * stored with capitals. + * + * Hence two candidates: the address as sent (trimmed), then its lowercased form. Both are exact + * matches, so each uses the unique index on `email` — a case-insensitive query would not, and this + * table is far too big to scan. The common case hits on the first. + * + * Empty when there's no usable address, so callers can skip the lookup entirely. + */ +export function emailLookupCandidates(email: string | null | undefined): string[] { + const asSent = email?.trim(); + if (!asSent) return []; + + const lowercased = asSent.toLowerCase(); + return asSent === lowercased ? [asSent] : [asSent, lowercased]; +} + +type NoDataCard = { key: string; components: null; timeToLiveSeconds: number }; + +/** + * How long Plain may cache a card we had no data for. + * + * Explicit rather than omitted: omitting the field falls back to the TTL configured for that card + * in Plain's settings, so a customer who becomes resolvable — an external id gets set, or someone + * signs up with that address — would keep showing an empty card for however long that default is. + * Short enough to recover promptly, long enough not to re-ask on every glance at a thread. + */ +const NO_DATA_TTL_SECONDS = 60; + +/** + * Fills in a `components: null` card for every requested key that wasn't answered. + * + * Plain records an integration error against any key it asked for and didn't get back, so a + * partial response surfaces in the support app as a broken card. `components: null` is how you + * say "this card has no data" and have Plain hide it instead. + */ +export function answerAllCardKeys( + cardKeys: string[], + cards: TCard[] +): (TCard | NoDataCard)[] { + const answered = new Set(cards.map((card) => card.key)); + + return [ + ...cards, + ...cardKeys + .filter((key) => !answered.has(key)) + .map( + (key): NoDataCard => ({ + key, + components: null, + timeToLiveSeconds: NO_DATA_TTL_SECONDS, + }) + ), + ]; +}