fix(webapp): two customer-card and impersonation fixes

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
   );
 }

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() {

apps/webapp/app/routes/admin.impersonate.tsx

@@ -5,18 +5,43 @@ import {
 } from "@remix-run/server-runtime";
 import { z } from "zod";
 import { redirectWithImpersonation } from "~/models/admin.server";
-import { requireUser } from "~/services/session.server";
+import { authenticator } from "~/services/auth.server";
+import { getRealUser } from "~/services/session.server";
 import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server";
 import { logger } from "~/services/logger.server";
+import { sanitizeRedirectPath } from "~/utils";
 
 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);
+  return admin?.admin ? admin : null;
+}
+
 async function handleImpersonationRequest(request: Request, userId: string): Promise<Response> {
-  const user = await requireUser(request);
-  if (!user.admin) {
+  const admin = await requireRealAdmin(request);
+  if (!admin) {
     return redirect("/");
   }
-  return redirectWithImpersonation(request, userId, "/", user);
+  return redirectWithImpersonation(request, userId, "/");
 }
 
 export const loader = async ({ request }: LoaderFunctionArgs) => {
@@ -33,9 +58,9 @@ export const loader = async ({ request }: LoaderFunctionArgs) => {
     return redirect("/");
   }
 
-  // Check admin BEFORE consuming the one-time token
-  const user = await requireUser(request);
-  if (!user.admin) {
+  // 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("/");
   }
 
@@ -46,7 +71,7 @@ export const loader = async ({ request }: LoaderFunctionArgs) => {
     return redirect("/");
   }
 
-  return redirectWithImpersonation(request, impersonateUserId, "/", user);
+  return redirectWithImpersonation(request, impersonateUserId, "/");
 };
 
 export async function action({ request }: ActionFunctionArgs) {

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),

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;

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<ReturnType<typeof requireUser>>;
 
 export async function requireUser(request: Request) {