diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index e93844dbaed..9ea767db538 100644 --- a/apps/webapp/app/models/admin.server.ts +++ b/apps/webapp/app/models/admin.server.ts @@ -1,5 +1,5 @@ import { redirect } from "@remix-run/server-runtime"; -import { $replica, prisma, type PrismaClientOrTransaction } from "~/db.server"; +import { $replica, $transaction, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { logger } from "~/services/logger.server"; import type { SearchParams } from "~/routes/admin._index"; import { @@ -9,7 +9,7 @@ import { setImpersonationId, } from "~/services/impersonation.server"; import { authenticator } from "~/services/auth.server"; -import { requireUser } from "~/services/session.server"; +import { getRealUser } from "~/services/session.server"; import { extractClientIp } from "~/utils/extractClientIp.server"; import { impersonationDestinationPath } from "~/utils/pathBuilder"; @@ -210,35 +210,76 @@ export async function adminGetOrganizations(userId: string, { page, search }: Se }; } +/** + * Starts (or switches) impersonation. + * + * The admin gate resolves the *real* authenticated user itself. `requireUser` returns the + * impersonation target while impersonating, so callers that gated on it refused an admin who was + * already impersonating someone — they had to stop first — and would have attributed the audit row + * to the target rather than the admin. + * + * `verifiedAdmin` exists only so tests can supply an admin without a session cookie. Production + * callers must not pass it: passing a `requireUser` result is exactly the bug described above. + */ export async function redirectWithImpersonation( request: Request, userId: string, path: string, - currentUser?: { id: string; admin: boolean }, + verifiedAdmin?: { id: string; admin: boolean }, prismaClient: PrismaClientOrTransaction = prisma ) { - const user = currentUser ?? (await requireUser(request)); - if (!user.admin) { + const admin = verifiedAdmin ?? (await getRealUser(request, prismaClient)); + if (!admin?.admin) { throw new Error("Unauthorized"); } const xff = request.headers.get("x-forwarded-for"); const ipAddress = extractClientIp(xff); + const previousTargetId = await getImpersonationId(request); + + // Switching straight from one target to another never passes through `clearImpersonation`, so the + // previous session is closed here, or the trail shows two overlapping STARTs. + // + // Both rows are written in one transaction: as separate statements, a failure between them could + // start an impersonation whose only audit row is the STOP for the previous target — an admin + // acting as someone with no record of it. + // + // `createdAt` is stamped explicitly rather than left to `@default(now())`, because Postgres `now()` + // is the *transaction* timestamp: inside one transaction both rows would take the same value, and + // an audit view ordered by that column couldn't tell which came first. + const startedAt = new Date(); + const closedAt = new Date(startedAt.getTime() - 1); try { - await prismaClient.impersonationAuditLog.create({ - data: { - action: "START", - adminId: user.id, - targetId: userId, - ipAddress, - }, + await $transaction(prismaClient, "startImpersonationAudit", async (tx) => { + if (previousTargetId && previousTargetId !== userId) { + await tx.impersonationAuditLog.create({ + data: { + action: "STOP", + adminId: admin.id, + targetId: previousTargetId, + ipAddress, + createdAt: closedAt, + }, + }); + } + + await tx.impersonationAuditLog.create({ + data: { + action: "START", + adminId: admin.id, + targetId: userId, + ipAddress, + createdAt: startedAt, + }, + }); }); } catch (error) { logger.error("Failed to create impersonation audit log", { error, - adminId: user.id, + adminId: admin.id, targetId: userId, + previousTargetId, }); } @@ -308,7 +349,8 @@ export async function startImpersonation( request: Request, organizationSlug: string, path: string, - currentUser: { id: string; admin: boolean }, + // Test-only, forwarded to `redirectWithImpersonation` — see its docstring. + verifiedAdmin?: { id: string; admin: boolean }, clients: { read: PrismaClientOrTransaction; write: PrismaClientOrTransaction } = { read: $replica, write: prisma, @@ -325,7 +367,7 @@ export async function startImpersonation( request, target.userId, impersonationDestinationPath(organizationSlug, path, new URL(request.url).search), - currentUser, + verifiedAdmin, clients.write ); } diff --git a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx index 2923a6fdeeb..5cecc06e56d 100644 --- a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx +++ b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx @@ -61,7 +61,7 @@ export async function loader({ request, params }: LoaderFunctionArgs) { // the consent page below instead, whose "Impersonate" button posts back from // our own page and so satisfies the same check. if (isSameOriginNavigation(request, env.LOGIN_ORIGIN)) { - throw await startImpersonation(request, organizationSlug, path, user); + throw await startImpersonation(request, organizationSlug, path); } // Expected for any link opened outside the app (address bar, bookmark, a link @@ -148,7 +148,7 @@ export async function action({ request, params }: ActionFunctionArgs) { // The consent form posts to an explicit absolute path (see // `impersonationConsentPostBackPath`), so the organization slug, the splat // path and the query string all arrive here intact. - return startImpersonation(request, organizationSlug, params["*"] ?? "", user); + return startImpersonation(request, organizationSlug, params["*"] ?? ""); } export default function Page() { diff --git a/apps/webapp/app/routes/admin.impersonate.tsx b/apps/webapp/app/routes/admin.impersonate.tsx deleted file mode 100644 index 458ed5b2a7e..00000000000 --- a/apps/webapp/app/routes/admin.impersonate.tsx +++ /dev/null @@ -1,61 +0,0 @@ -import { - redirect, - type ActionFunctionArgs, - type LoaderFunctionArgs, -} from "@remix-run/server-runtime"; -import { z } from "zod"; -import { redirectWithImpersonation } from "~/models/admin.server"; -import { requireUser } from "~/services/session.server"; -import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server"; -import { logger } from "~/services/logger.server"; - -const FormSchema = z.object({ id: z.string() }); - -async function handleImpersonationRequest(request: Request, userId: string): Promise { - const user = await requireUser(request); - if (!user.admin) { - return redirect("/"); - } - return redirectWithImpersonation(request, userId, "/", user); -} - -export const loader = async ({ request }: LoaderFunctionArgs) => { - const url = new URL(request.url); - const impersonateUserId = url.searchParams.get("impersonate"); - const impersonationToken = url.searchParams.get("impersonationToken"); - - if (!impersonateUserId) { - return redirect("/admin"); - } - - if (!impersonationToken) { - logger.warn("Impersonation request missing token"); - return redirect("/"); - } - - // Check admin BEFORE consuming the one-time token - const user = await requireUser(request); - if (!user.admin) { - return redirect("/"); - } - - const validatedUserId = await validateAndConsumeImpersonationToken(impersonationToken); - - if (!validatedUserId || validatedUserId !== impersonateUserId) { - logger.warn("Invalid or expired impersonation token"); - return redirect("/"); - } - - return redirectWithImpersonation(request, impersonateUserId, "/", user); -}; - -export async function action({ request }: ActionFunctionArgs) { - if (request.method.toLowerCase() !== "post") { - return new Response("Method not allowed", { status: 405 }); - } - - const payload = Object.fromEntries(await request.formData()); - const { id } = FormSchema.parse(payload); - - return handleImpersonationRequest(request, id); -} diff --git a/apps/webapp/app/routes/admin_.impersonate.tsx b/apps/webapp/app/routes/admin_.impersonate.tsx new file mode 100644 index 00000000000..f55d258a9f6 --- /dev/null +++ b/apps/webapp/app/routes/admin_.impersonate.tsx @@ -0,0 +1,110 @@ +import { + redirect, + type ActionFunctionArgs, + type LoaderFunctionArgs, +} from "@remix-run/server-runtime"; +import { z } from "zod"; +import { redirectWithImpersonation } from "~/models/admin.server"; +import { authenticator } from "~/services/auth.server"; +import { rbac } from "~/services/rbac.server"; +import { getRealUser } from "~/services/session.server"; +import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server"; +import { logger } from "~/services/logger.server"; +import { sanitizeRedirectPath } from "~/utils"; + +/** + * Served at `/admin/impersonate`, but the trailing `_` on `admin_` keeps it out of the `admin.tsx` + * layout on purpose. + * + * That layout's loader is `dashboardLoader({ authorization: { requireSuper: true } })`, which + * resolves the user through `getUserId` — the impersonated id while impersonating. So starting on a + * second target ran the parent gate against the target, which isn't a super admin, and it answered + * with its own `redirect("/")`. Nesting would leave this route's behaviour depending on the router + * preferring the deepest redirect; opting out removes the question. Nothing is lost — this route + * only ever redirects, so it never rendered inside the layout anyway. + */ + +const FormSchema = z.object({ id: z.string() }); + +/** + * The real authenticated user, or null when they're signed in but not an admin. + * + * Must not use `requireUser`: while impersonating it resolves to the impersonation target, whose + * `admin` is false, so an admin switching to a second target was bounced to `/` and left on the + * first one. + * + * Throws a login redirect when nobody is signed in, keeping this URL as `redirectTo` so the + * impersonation survives the round trip — the one-time token is validated after this gate, so it's + * still unconsumed when the browser comes back. Collapsing that into the non-admin `/` redirect + * would drop the link the agent clicked. + */ +async function requireRealAdmin(request: Request) { + if (!(await authenticator.isAuthenticated(request))) { + const url = new URL(request.url); + const redirectTo = sanitizeRedirectPath(`${url.pathname}${url.search}`); + throw redirect(`/login?${new URLSearchParams([["redirectTo", redirectTo]])}`); + } + + const admin = await getRealUser(request); + if (!admin) return null; + + // Same gate `dashboardLoader({ authorization: { requireSuper: true } })` applies, evaluated + // against the real admin. It can't be reached through the builder here, because the builder + // resolves its subject with `getUserId` — the impersonated id while impersonating, which is the + // bug this route exists to fix. So the ability is built explicitly for `admin.id` instead of + // trusting the raw `User.admin` column: `canSuper()` is only equal to that column in the OSS + // fallback, and a plugin is free to be stricter. requireSuper needs no org/project scope. + const auth = await rbac.authenticateSession(request, { userId: admin.id }); + if (!auth.ok || !auth.ability.canSuper()) return null; + + return admin; +} + +async function handleImpersonationRequest(request: Request, userId: string): Promise { + const admin = await requireRealAdmin(request); + if (!admin) { + return redirect("/"); + } + return redirectWithImpersonation(request, userId, "/"); +} + +export const loader = async ({ request }: LoaderFunctionArgs) => { + const url = new URL(request.url); + const impersonateUserId = url.searchParams.get("impersonate"); + const impersonationToken = url.searchParams.get("impersonationToken"); + + if (!impersonateUserId) { + return redirect("/admin"); + } + + if (!impersonationToken) { + logger.warn("Impersonation request missing token"); + return redirect("/"); + } + + // Check admin BEFORE consuming the one-time token, so a rejected request leaves the token usable. + const admin = await requireRealAdmin(request); + if (!admin) { + return redirect("/"); + } + + const validatedUserId = await validateAndConsumeImpersonationToken(impersonationToken); + + if (!validatedUserId || validatedUserId !== impersonateUserId) { + logger.warn("Invalid or expired impersonation token"); + return redirect("/"); + } + + return redirectWithImpersonation(request, impersonateUserId, "/"); +}; + +export async function action({ request }: ActionFunctionArgs) { + if (request.method.toLowerCase() !== "post") { + return new Response("Method not allowed", { status: 405 }); + } + + const payload = Object.fromEntries(await request.formData()); + const { id } = FormSchema.parse(payload); + + return handleImpersonationRequest(request, id); +} 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 5e09fb08544..cb9e60b2187 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,11 @@ 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, PlainCustomerCardRequestSchema } from "~/utils/plainCustomerCards"; function sanitizeHeaders( request: Request, @@ -141,14 +121,25 @@ export async function action({ request }: ActionFunctionArgs) { const user = where ? await prisma.user.findFirst({ where, include: userInclude }) : null; - // If user not found, return empty cards + /** + * Impersonation is offered only when the customer was 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. + */ + const canImpersonate = !!customer.externalId; + + // 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 +149,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 +243,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 +416,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/services/impersonation.server.ts b/apps/webapp/app/services/impersonation.server.ts index e69a3fb305b..7d93390f704 100644 --- a/apps/webapp/app/services/impersonation.server.ts +++ b/apps/webapp/app/services/impersonation.server.ts @@ -45,6 +45,14 @@ export async function getImpersonationId(request: Request) { export async function setImpersonationId(userId: string, request: Request) { const session = await getImpersonationSession(request); + // Switching straight to a different target begins a new impersonation session, so the view-as-user + // flag must not carry over from the previous one — it's scoped to a single impersonation, which is + // why `clearImpersonationId` drops it too. Reachable only since switching stopped requiring a stop + // first; before that, every second target arrived via `clearImpersonationId`. + if (session.get(IMPERSONATED_USER_ID_KEY) !== userId) { + session.unset(VIEWING_AS_USER_KEY); + } + session.set(IMPERSONATED_USER_ID_KEY, userId); return session; diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts index 753bc7f6a17..a34c346d433 100644 --- a/apps/webapp/app/services/session.server.ts +++ b/apps/webapp/app/services/session.server.ts @@ -1,4 +1,5 @@ import { redirect } from "@remix-run/node"; +import { prisma, type PrismaClientOrTransaction } from "~/db.server"; import { getUserById } from "~/models/user.server"; import { sanitizeRedirectPath } from "~/utils"; import { extractClientIp } from "~/utils/extractClientIp.server"; @@ -124,6 +125,44 @@ export async function requireUserId(request: Request, redirectTo?: string) { return userId; } +/** + * The user the request actually authenticated as, ignoring any impersonation cookie. + * + * `getUserId` deliberately resolves to the *impersonated* id while impersonating, so `getUser` / + * `requireUser` answer "who is this request acting as". That is the wrong question for anything + * gating on admin rights or attributing an admin action: while impersonating a customer, + * `requireUser().admin` is that customer's flag, so an admin check silently fails and an audit + * record would name the customer as the actor. + * + * Returns null when unauthenticated or the row is gone. + */ +export async function getRealUser( + request: Request, + prismaClient: PrismaClientOrTransaction = prisma +) { + const authUser = await authenticator.isAuthenticated(request); + + // Apply the same session controls `getUserId`/`getUser` apply to the real user, so this helper + // can't become a way around them: a session the IdP has revoked throws to /logout here, and one + // past its effective duration is caught below. Skipping either would let an admin whose session + // should have ended still start impersonation. + await revalidateSsoSession(request, authUser); + if (!authUser?.userId) return null; + + // Narrow select — callers need the id and the admin flag, plus `nextSessionEnd` for the deadline + // check. Takes a client so a caller already scoped to one reads the admin from the same database + // it writes to. + const user = await prismaClient.user.findFirst({ + where: { id: authUser.userId }, + select: { id: true, admin: true, nextSessionEnd: true }, + }); + if (!user) return null; + + maybeAutoLogout(request, user); + + return user; +} + export type UserFromSession = Awaited>; export async function requireUser(request: Request) { diff --git a/apps/webapp/app/utils/plainCustomerCards.test.ts b/apps/webapp/app/utils/plainCustomerCards.test.ts new file mode 100644 index 00000000000..53f45e62e36 --- /dev/null +++ b/apps/webapp/app/utils/plainCustomerCards.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, it } from "vitest"; +import { answerAllCardKeys, 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); + }); + + it("still requires one of email or externalId", () => { + const result = PlainCustomerCardRequestSchema.safeParse( + request({ customer: { id: "c_1", email: null, externalId: null } }) + ); + + expect(result.success).toBe(false); + }); + + it("rejects a body with no card keys field", () => { + expect(PlainCustomerCardRequestSchema.safeParse({ customer: { id: "c_1" } }).success).toBe( + false + ); + }); +}); + +describe("answerAllCardKeys", () => { + it("adds a no-data card for every unanswered key", () => { + expect(answerAllCardKeys(["a", "b"], [])).toEqual([ + { key: "a", components: null }, + { key: "b", components: null }, + ]); + }); + + 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 }, + { key: "c", components: null }, + ]); + }); + + 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 00000000000..e9ed1f8c4d5 --- /dev/null +++ b/apps/webapp/app/utils/plainCustomerCards.ts @@ -0,0 +1,59 @@ +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()), + customer: z + .object({ + id: z.string(), + email: z.string().nullish(), + externalId: z.string().nullish(), + }) + .refine((data) => data.email || data.externalId, { + message: "Either customer.email or customer.externalId must be provided", + path: ["customer"], + }), + thread: z + .object({ + id: z.string(), + }) + .nullish(), +}); + +export type PlainCustomerCardRequest = z.infer; + +type NoDataCard = { key: string; components: null }; + +/** + * 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, + }) + ), + ]; +}