fix(webapp): let an admin switch impersonation target without stopping first

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

@@ -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<Response> {
+  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);
+}

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