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

Author: isshaddad

apps/webapp/app/models/admin.server.ts

@@ -9,7 +9,7 @@ import {
   setImpersonationId,
 } from "~/services/impersonation.server";
 import { authenticator } from "~/services/auth.server";
-import { requireUser } from "~/services/session.server";
+import { getRealUser, requireUser } from "~/services/session.server";
 import { extractClientIp } from "~/utils/extractClientIp.server";
 import { impersonationDestinationPath } from "~/utils/pathBuilder";
 
@@ -210,35 +210,62 @@ 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);
 
   try {
-    await prismaClient.impersonationAuditLog.create({
-      data: {
-        action: "START",
-        adminId: user.id,
-        targetId: userId,
-        ipAddress,
-      },
+    await prismaClient.impersonationAuditLog.createMany({
+      data: [
+        // Switching straight from one target to another never passes through `clearImpersonation`,
+        // so close the previous session here or the trail shows two overlapping STARTs.
+        ...(previousTargetId && previousTargetId !== userId
+          ? [
+              {
+                action: "STOP" as const,
+                adminId: admin.id,
+                targetId: previousTargetId,
+                ipAddress,
+              },
+            ]
+          : []),
+        {
+          action: "START" as const,
+          adminId: admin.id,
+          targetId: userId,
+          ipAddress,
+        },
+      ],
     });
   } catch (error) {
     logger.error("Failed to create impersonation audit log", {
       error,
-      adminId: user.id,
+      adminId: admin.id,
       targetId: userId,
+      previousTargetId,
     });
   }
 
@@ -308,7 +335,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 +353,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,30 @@ import {
 } from "@remix-run/server-runtime";
 import { z } from "zod";
 import { redirectWithImpersonation } from "~/models/admin.server";
-import { requireUser } from "~/services/session.server";
+import { getRealUser } from "~/services/session.server";
 import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server";
 import { logger } from "~/services/logger.server";
 
 const FormSchema = z.object({ id: z.string() });
 
+/**
+ * The real authenticated user, or null when they aren't 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.
+ */
+async function requireRealAdmin(request: Request) {
+  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 +45,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 +58,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/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,32 @@ 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);
+  if (!authUser?.userId) return null;
+
+  // Narrow select: callers only ever need the id and the admin flag. Takes a client so a caller
+  // already scoped to one reads the admin from the same database it writes to.
+  return prismaClient.user.findFirst({
+    where: { id: authUser.userId },
+    select: { id: true, admin: true },
+  });
+}
+
 export type UserFromSession = Awaited<ReturnType<typeof requireUser>>;
 
 export async function requireUser(request: Request) {