fix(webapp): clear impersonation before the consent action's admin check

Two review findings, both about "who is this request" being answered
differently in different places.

The consent page's action went straight from requireUser to the admin
check. requireUser resolves to the impersonated user whenever the
impersonation cookie is set, so user.admin was false for a legitimate
admin who already had an impersonation session: render the consent page
with nothing active, start impersonating in another tab, then press
Impersonate on the still-open page, and the POST redirected to / with no
explanation. The action now mirrors the loader's ordering — clear the
impersonation first and come back to the same URL (search included) under
the admin's own identity, where the normal flow re-authorizes them and
they can confirm. The impersonation mutation never runs on a request whose
resolved identity is the impersonated user.

The view-as-user flag had two definitions. requireUser required the
impersonated id to match the resolved user; the root loader only required
that some impersonated id was present in the session. Those disagree in
exactly one state: getUserId deliberately falls back to the real admin's
id when they are no longer an admin, so the cookie still names a target
that is not the resolved user. The client-side flag read true while the
server-computed user.isViewingAsUser read false, and client-hidden admin
UI drifted from server-computed display flags.

The strict rule now lives in one place, resolveImpersonationState, with
getImpersonationState reading the cookie and delegating to it. requireUser,
the root loader and the bulk-action loader all go through it, so the value
published to the client is the value the server computed. A de-admined
session correctly reports neither impersonating nor viewing as user, which
also restores its admin chrome — right, since it is not an admin session
any more.

The organization loader's own isImpersonating is left as it was; it is
pre-existing and nothing here depends on it.

Co-Authored-By: Claude <noreply@anthropic.com>

Author: claude

apps/webapp/app/root.tsx

@@ -19,7 +19,7 @@ import { TimezoneSetter } from "./components/TimezoneSetter";
 import { env } from "./env.server";
 import { featuresForRequest } from "./features.server";
 import { usePostHog } from "./hooks/usePostHog";
-import { getViewingAsUser } from "./services/impersonation.server";
+import { getImpersonationState } from "./services/impersonation.server";
 import { getUser } from "./services/session.server";
 import { getTimezonePreference } from "./services/preferences/uiPreferences.server";
 import { appEnvTitleTag } from "./utils";
@@ -74,7 +74,13 @@ export const loader = async ({ request }: LoaderFunctionArgs) => {
   // Display-only: while impersonating, an admin can ask to see the dashboard
   // the way the impersonated user sees it. Exposed from root so every route can
   // read it.
-  const isViewingAsUser = await getViewingAsUser(request);
+  //
+  // Resolved against the user this request authenticated as, which is the same
+  // condition `requireUser` applies — otherwise the flag the client reads and
+  // the `user.isViewingAsUser` the server computes could disagree, and the
+  // client-side admin UI would hide itself on a session that is not
+  // impersonating.
+  const { isViewingAsUser } = await getImpersonationState(request, user?.id);
 
   const headers = new Headers();
   headers.append("Set-Cookie", await commitSession(session));

apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx

@@ -107,6 +107,22 @@ export async function action({ request, params }: ActionFunctionArgs) {
 
   const user = await requireUser(request);
 
+  // Same ordering as the loader, and it matters for the same reason: while an
+  // impersonation cookie is set `requireUser` resolves to the *impersonated*
+  // user, so `user.admin` is false even for a legitimate admin. An admin who
+  // started impersonating in another tab and then submitted this page would
+  // fail the check below and be bounced to `/` with no explanation. Clear the
+  // impersonation first and come back to this same URL under their own
+  // identity, where the normal flow re-authorizes them and they can confirm.
+  // Never run the impersonation mutation on a request whose resolved identity
+  // is the impersonated user.
+  if (user.isImpersonating) {
+    const url = new URL(request.url);
+    // Keep the search for the same reason the loader does: the follow-up GET
+    // rebuilds the destination and post-back paths from it.
+    return clearImpersonation(request, `${url.pathname}${url.search}`);
+  }
+
   if (!user.admin) {
     return redirect("/");
   }

apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx

@@ -56,7 +56,7 @@ import { CreateBulkActionPresenter } from "~/presenters/v3/CreateBulkActionPrese
 import { RegionsPresenter } from "~/presenters/v3/RegionsPresenter.server";
 import { RUNS_BULK_INSPECTOR_UI_SEARCH_PARAMS } from "~/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs._index/shouldRevalidateRunsList";
 import { logger } from "~/services/logger.server";
-import { getViewingAsUser } from "~/services/impersonation.server";
+import { getImpersonationState } from "~/services/impersonation.server";
 import { dashboardAction, dashboardLoader } from "~/services/routeBuilders/dashboardBuilder";
 import { hasAdminDisplayAccess } from "~/services/session.server";
 import { checkPermissions } from "~/services/routeBuilders/permissions.server";
@@ -87,11 +87,11 @@ export const loader = dashboardLoader(
     }
 
     // Display only: hidden regions stay listed for admins, unless they've asked
-    // to see the dashboard the way the impersonated user sees it.
-    const isAdmin = hasAdminDisplayAccess({
-      ...user,
-      isViewingAsUser: await getViewingAsUser(request),
-    });
+    // to see the dashboard the way the impersonated user sees it. The route
+    // builder's `user` doesn't carry the flag, so read it off the session with
+    // the same strict condition every other consumer uses.
+    const { isViewingAsUser } = await getImpersonationState(request, user.id);
+    const isAdmin = hasAdminDisplayAccess({ ...user, isViewingAsUser });
 
     const presenter = new CreateBulkActionPresenter();
     const [data, regionsResult] = await Promise.all([

apps/webapp/app/services/impersonation.server.ts

@@ -5,6 +5,7 @@ import { singleton } from "~/utils/singleton";
 import { createRedisClient, type RedisClient } from "~/redis.server";
 import { env } from "~/env.server";
 import { logger } from "~/services/logger.server";
+import { resolveImpersonationState, type ImpersonationState } from "~/utils/impersonationState";
 
 export const impersonationSessionStorage = createCookieSessionStorage({
   cookie: {
@@ -61,16 +62,25 @@ export async function clearImpersonationId(request: Request) {
 }
 
 /**
- * Whether the admin has asked to see the dashboard the way the impersonated
- * user sees it. Only ever true inside an impersonation session.
+ * The impersonation state for a request, resolved against `resolvedUserId` — the
+ * id the request actually authenticated as (what `getUser`/`getUserId` return).
+ *
+ * This is the one place the impersonation flags come from, so the values the
+ * server computes for a route and the value the root loader publishes to the
+ * client cannot disagree. See `resolveImpersonationState` for why the
+ * impersonated id has to match the resolved user rather than merely be present.
  */
-export async function getViewingAsUser(request: Request) {
+export async function getImpersonationState(
+  request: Request,
+  resolvedUserId: string | undefined
+): Promise<ImpersonationState> {
   const session = await getImpersonationSession(request);
 
-  return (
-    typeof session.get(IMPERSONATED_USER_ID_KEY) === "string" &&
-    session.get(VIEWING_AS_USER_KEY) === true
-  );
+  return resolveImpersonationState({
+    impersonatedUserId: session.get(IMPERSONATED_USER_ID_KEY),
+    viewingAsUser: session.get(VIEWING_AS_USER_KEY),
+    resolvedUserId,
+  });
 }
 
 export async function setViewingAsUser(value: boolean, request: Request) {

apps/webapp/app/services/session.server.ts

@@ -3,7 +3,7 @@ import { getUserById } from "~/models/user.server";
 import { sanitizeRedirectPath } from "~/utils";
 import { extractClientIp } from "~/utils/extractClientIp.server";
 import { authenticator } from "./auth.server";
-import { getImpersonationId, getViewingAsUser } from "./impersonation.server";
+import { getImpersonationId, getImpersonationState } from "./impersonation.server";
 import { logger } from "./logger.server";
 import { revalidateSsoSession } from "./ssoSessionRevalidation.server";
 
@@ -135,8 +135,9 @@ export async function requireUser(request: Request) {
     throw redirect(`/login?${searchParams}`);
   }
 
-  const impersonationId = await getImpersonationId(request);
-  const isImpersonating = !!impersonationId && impersonationId === user.id;
+  // Shared with the root loader so the client never reads a different answer
+  // than the one computed here.
+  const { isImpersonating, isViewingAsUser } = await getImpersonationState(request, user.id);
   return {
     id: user.id,
     email: user.email,
@@ -150,7 +151,7 @@ export async function requireUser(request: Request) {
     confirmedBasicDetails: user.confirmedBasicDetails,
     mfaEnabledAt: user.mfaEnabledAt,
     isImpersonating,
-    isViewingAsUser: isImpersonating && (await getViewingAsUser(request)),
+    isViewingAsUser,
   };
 }
 

apps/webapp/app/utils/impersonationState.test.ts

@@ -0,0 +1,71 @@
+import { describe, expect, it } from "vitest";
+import { resolveImpersonationState } from "./impersonationState";
+
+const IMPERSONATED = "user_1";
+const ADMIN = "admin_1";
+
+describe("resolveImpersonationState", () => {
+  it("reports impersonation when the cookie's id is the resolved user", () => {
+    expect(
+      resolveImpersonationState({
+        impersonatedUserId: IMPERSONATED,
+        viewingAsUser: undefined,
+        resolvedUserId: IMPERSONATED,
+      })
+    ).toEqual({ isImpersonating: true, isViewingAsUser: false });
+  });
+
+  it("carries the view-as-user flag inside an impersonation session", () => {
+    expect(
+      resolveImpersonationState({
+        impersonatedUserId: IMPERSONATED,
+        viewingAsUser: true,
+        resolvedUserId: IMPERSONATED,
+      })
+    ).toEqual({ isImpersonating: true, isViewingAsUser: true });
+  });
+
+  // The case the strict comparison exists for: the session falls back to the
+  // real admin's id when their admin role is revoked mid-session, while the
+  // cookie still names the impersonation target. Both flags must read false so
+  // the server-side values and the value published to the client agree.
+  it("reports neither flag when the impersonated id is not the resolved user", () => {
+    expect(
+      resolveImpersonationState({
+        impersonatedUserId: IMPERSONATED,
+        viewingAsUser: true,
+        resolvedUserId: ADMIN,
+      })
+    ).toEqual({ isImpersonating: false, isViewingAsUser: false });
+  });
+
+  it("reports neither flag when there is no impersonated id", () => {
+    expect(
+      resolveImpersonationState({
+        impersonatedUserId: undefined,
+        viewingAsUser: true,
+        resolvedUserId: IMPERSONATED,
+      })
+    ).toEqual({ isImpersonating: false, isViewingAsUser: false });
+  });
+
+  it("reports neither flag when there is no resolved user", () => {
+    expect(
+      resolveImpersonationState({
+        impersonatedUserId: IMPERSONATED,
+        viewingAsUser: true,
+        resolvedUserId: undefined,
+      })
+    ).toEqual({ isImpersonating: false, isViewingAsUser: false });
+  });
+
+  it("only treats a literal true as the view-as-user flag", () => {
+    expect(
+      resolveImpersonationState({
+        impersonatedUserId: IMPERSONATED,
+        viewingAsUser: "true",
+        resolvedUserId: IMPERSONATED,
+      }).isViewingAsUser
+    ).toBe(false);
+  });
+});

apps/webapp/app/utils/impersonationState.ts

@@ -0,0 +1,47 @@
+/**
+ * The rule for reading impersonation state off the impersonation cookie.
+ *
+ * Kept pure and free of server-only imports so it can be unit tested directly,
+ * and so there is exactly one definition of "this request is impersonating" for
+ * every caller to share.
+ */
+
+export type ImpersonationState = {
+  isImpersonating: boolean;
+  isViewingAsUser: boolean;
+};
+
+/**
+ * Resolves the impersonation cookie's raw contents against the identity the
+ * request actually authenticated as.
+ *
+ * Matching the impersonated id against `resolvedUserId` is deliberate. When an
+ * admin's role is revoked mid-session the session falls back to the real admin's
+ * id while the cookie still names the impersonation target, so "an impersonated
+ * id is present" and "this request is impersonating" stop meaning the same
+ * thing. Only the strict reading is correct there: that session is no longer
+ * impersonating, and so it is not viewing as the user either.
+ *
+ * Every consumer has to agree on this, or the flags computed on the server and
+ * the flag published to the client drift apart — the admin chrome would hide
+ * itself on a session that is not impersonating at all.
+ */
+export function resolveImpersonationState(options: {
+  impersonatedUserId: unknown;
+  viewingAsUser: unknown;
+  resolvedUserId: string | undefined;
+}): ImpersonationState {
+  const { impersonatedUserId, viewingAsUser, resolvedUserId } = options;
+
+  const isImpersonating =
+    typeof impersonatedUserId === "string" &&
+    resolvedUserId !== undefined &&
+    impersonatedUserId === resolvedUserId;
+
+  return {
+    isImpersonating,
+    // Display only, and meaningless outside an impersonation session, so it
+    // never reads as on without one.
+    isViewingAsUser: isImpersonating && viewingAsUser === true,
+  };
+}

apps/webapp/test/viewAsUser.test.ts

@@ -3,7 +3,7 @@ import {
   clearImpersonationId,
   commitImpersonationSession,
   getImpersonationId,
-  getViewingAsUser,
+  getImpersonationState,
   setImpersonationId,
   setViewingAsUser,
 } from "~/services/impersonation.server";
@@ -19,27 +19,33 @@ async function requestWith(session: Session) {
   });
 }
 
+// Reads the flag the way a request does: against the user the request resolved
+// as. Every fixture below impersonates `user_1`.
+async function readViewingAsUser(request: Request) {
+  return (await getImpersonationState(request, "user_1")).isViewingAsUser;
+}
+
 // The "view as user" flag rides on the impersonation cookie, so it is scoped to
 // the impersonation session by construction.
 describe("view as user flag", () => {
   it("is off when there is no impersonation cookie", async () => {
-    expect(await getViewingAsUser(new Request("http://localhost:3030/orgs/acme"))).toBe(false);
+    expect(await readViewingAsUser(new Request("http://localhost:3030/orgs/acme"))).toBe(false);
   });
 
   it("can be turned on and back off within an impersonation session", async () => {
     const start = new Request("http://localhost:3030/orgs/acme");
 
     const impersonating = await requestWith(await setImpersonationId("user_1", start));
     expect(await getImpersonationId(impersonating)).toBe("user_1");
-    expect(await getViewingAsUser(impersonating)).toBe(false);
+    expect(await readViewingAsUser(impersonating)).toBe(false);
 
     const viewingAsUser = await requestWith(await setViewingAsUser(true, impersonating));
     expect(await getImpersonationId(viewingAsUser)).toBe("user_1");
-    expect(await getViewingAsUser(viewingAsUser)).toBe(true);
+    expect(await readViewingAsUser(viewingAsUser)).toBe(true);
 
     const showingAdminUi = await requestWith(await setViewingAsUser(false, viewingAsUser));
     expect(await getImpersonationId(showingAdminUi)).toBe("user_1");
-    expect(await getViewingAsUser(showingAdminUi)).toBe(false);
+    expect(await readViewingAsUser(showingAdminUi)).toBe(false);
   });
 
   it("is dropped when impersonation is cleared", async () => {
@@ -50,15 +56,29 @@ describe("view as user flag", () => {
     const cleared = await requestWith(await clearImpersonationId(viewingAsUser));
 
     expect(await getImpersonationId(cleared)).toBeUndefined();
-    expect(await getViewingAsUser(cleared)).toBe(false);
+    expect(await readViewingAsUser(cleared)).toBe(false);
   });
 
   it("never reads as on without an impersonated user", async () => {
     const start = new Request("http://localhost:3030/orgs/acme");
     // Set the flag with no impersonation in progress — it must not read back on.
     const flagOnly = await requestWith(await setViewingAsUser(true, start));
 
-    expect(await getViewingAsUser(flagOnly)).toBe(false);
+    expect(await readViewingAsUser(flagOnly)).toBe(false);
+  });
+
+  it("is off once the impersonated user is not who the request resolved as", async () => {
+    const start = new Request("http://localhost:3030/orgs/acme");
+    const impersonating = await requestWith(await setImpersonationId("user_1", start));
+    const viewing = await requestWith(await setViewingAsUser(true, impersonating));
+
+    // An admin who loses the admin role mid-session resolves back to their own
+    // id while the cookie still names the target. That session is not
+    // impersonating any more, so it is not viewing as the user either.
+    const state = await getImpersonationState(viewing, "admin_1");
+
+    expect(state.isImpersonating).toBe(false);
+    expect(state.isViewingAsUser).toBe(false);
   });
 });