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

getUserId resolves to the impersonated user id while impersonating, by design, so
requireUser answers "who is this request acting as". Every impersonation entry
point gated on it, so while impersonating a customer user.admin was that
customer's flag and starting on a second target silently redirected to / — you
had to stop impersonating first.

- New getRealUser resolves the authenticated user, ignoring the impersonation
  cookie, and applies the same session controls getUserId does for the real user
  (SSO revalidation and the auto-logout deadline) so this can't become a way
  around them.
- redirectWithImpersonation gates on it rather than taking a user from the
  caller, and attributes the audit row to the real admin.
- The route moves to admin_.impersonate.tsx to opt out of the admin layout, whose
  requireSuper gate resolves the same impersonated identity. It checks
  canSuper() against the real admin directly, since the raw User.admin column
  only equals canSuper() in the OSS fallback.
- Unauthenticated requests redirect to login carrying the original URL, so the
  impersonation link survives the round trip.
- The view-as-user flag is cleared when the target changes; it is scoped to a
  single impersonation session.

Switching straight between targets never passes through clearImpersonation, so a
STOP for the previous target is written alongside the new START, both in one
transaction with explicit timestamps — Postgres now() is the transaction
timestamp, so the default would stamp both rows identically.

Author: isshaddad

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/routes/admin_.impersonate.tsx

@@ -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/impersonation.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) {