From dcd20a7ba4ef07b77be6425b5dc89c2286488301 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 19:23:23 +0000 Subject: [PATCH 1/8] feat(webapp): impersonation consent page and a view-as-user toggle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a `/@/orgs//…` link from outside the dashboard (address bar, bookmark, a link shared elsewhere) used to bounce back to /admin, because starting impersonation requires a same-origin navigation. Keep that requirement for the state change, but render a consent interstitial instead of blocking: the page names the organization and destination, and its "Impersonate" button posts back from our own page, so the same-origin check still holds. In-app admin links are unchanged and still impersonate in one click. Also add a display-only "View as user" toggle for impersonation sessions. It lives on the impersonation cookie, so it disappears when impersonation is cleared, and it hides admin-only UI both client-side (via useHasAdminAccess) and in the loaders that compute what to show. It never touches authorization. The escape hatches — the impersonation border, "Stop impersonating", the global shortcut and the toggle itself — stay visible while it's on. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01G5rpYGm4SVqxSNXx6sXtpu --- .../impersonation-consent-and-view-as-user.md | 6 + .../app/components/navigation/SideMenu.tsx | 80 +++++-- apps/webapp/app/hooks/useUser.ts | 16 +- apps/webapp/app/models/admin.server.ts | 7 +- apps/webapp/app/root.tsx | 6 + .../routes/_app.@.orgs.$organizationSlug.$.ts | 82 ------- .../_app.@.orgs.$organizationSlug.$.tsx | 216 ++++++++++++++++++ .../route.tsx | 4 +- .../route.tsx | 4 +- .../route.tsx | 6 +- .../route.tsx | 10 +- .../route.tsx | 4 +- .../route.tsx | 9 +- .../routes/resources.impersonation.view-as.ts | 36 +++ ...ectParam.env.$envParam.runs.bulkaction.tsx | 11 +- .../app/services/impersonation.server.ts | 43 +++- apps/webapp/app/services/session.server.ts | 21 +- apps/webapp/test/impersonationConsent.test.ts | 149 ++++++++++++ apps/webapp/test/viewAsUser.test.ts | 89 ++++++++ 19 files changed, 671 insertions(+), 128 deletions(-) create mode 100644 .server-changes/impersonation-consent-and-view-as-user.md delete mode 100644 apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.ts create mode 100644 apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx create mode 100644 apps/webapp/app/routes/resources.impersonation.view-as.ts create mode 100644 apps/webapp/test/impersonationConsent.test.ts create mode 100644 apps/webapp/test/viewAsUser.test.ts diff --git a/.server-changes/impersonation-consent-and-view-as-user.md b/.server-changes/impersonation-consent-and-view-as-user.md new file mode 100644 index 00000000000..db78d9c375e --- /dev/null +++ b/.server-changes/impersonation-consent-and-view-as-user.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: improvement +--- + +Admins opening an impersonation link from outside the dashboard now get a confirmation page naming the organization and destination instead of being bounced back, and while impersonating they can switch to "View as user" to see the dashboard without any admin-only UI. diff --git a/apps/webapp/app/components/navigation/SideMenu.tsx b/apps/webapp/app/components/navigation/SideMenu.tsx index 29eb1fc8520..625ee3d74e9 100644 --- a/apps/webapp/app/components/navigation/SideMenu.tsx +++ b/apps/webapp/app/components/navigation/SideMenu.tsx @@ -4,7 +4,14 @@ import { ExclamationTriangleIcon, } from "@heroicons/react/24/outline"; import { EllipsisHorizontalIcon } from "@heroicons/react/20/solid"; -import { useFetcher, useNavigation, useRevalidator, useSubmit } from "@remix-run/react"; +import { + Form, + useFetcher, + useLocation, + useNavigation, + useRevalidator, + useSubmit, +} from "@remix-run/react"; import { LayoutGroup, motion } from "framer-motion"; import { type CSSProperties, @@ -33,6 +40,8 @@ import { DeploymentsIcon } from "~/assets/icons/DeploymentsIcon"; import { DialIcon } from "~/assets/icons/DialIcon"; import { DropdownIcon } from "~/assets/icons/DropdownIcon"; import { BranchEnvironmentIconSmall } from "~/assets/icons/EnvironmentIcons"; +import { EyeClosedIcon } from "~/assets/icons/EyeClosedIcon"; +import { EyeOpenIcon } from "~/assets/icons/EyeOpenIcon"; import { FolderClosedIcon } from "~/assets/icons/FolderClosedIcon"; import { FolderOpenIcon } from "~/assets/icons/FolderOpenIcon"; import { GlobeLinesIcon } from "~/assets/icons/GlobeLinesIcon"; @@ -69,7 +78,7 @@ import { type MatchedOrganization } from "~/hooks/useOrganizations"; import { type MatchedProject } from "~/hooks/useProject"; import { useShortcutKeys } from "~/hooks/useShortcutKeys"; import { useShowSelfServe } from "~/hooks/useShowSelfServe"; -import { useHasAdminAccess } from "~/hooks/useUser"; +import { useHasAdminAccess, useIsViewingAsUser } from "~/hooks/useUser"; import { type UserWithDashboardPreferences } from "~/models/user.server"; import { useCurrentPlan, @@ -785,7 +794,7 @@ export function SideMenu({ // user's saved order/hidden preferences are applied at render below. const staticSections: SideMenuSectionConfig[] = []; - if (user.admin || user.isImpersonating || featureFlags.hasAiAccess) { + if (isAdmin || featureFlags.hasAiAccess) { staticSections.push({ id: "ai", title: "AI", @@ -813,12 +822,12 @@ export function SideMenu({ }); } - if (user.admin || user.isImpersonating || featureFlags.hasQueryAccess) { + if (isAdmin || featureFlags.hasQueryAccess) { staticSections.push({ id: "metrics", title: "Observability", items: [ - ...(user.admin || user.isImpersonating || featureFlags.hasLogsPageAccess + ...(isAdmin || featureFlags.hasLogsPageAccess ? [ { id: "logs", @@ -1832,24 +1841,29 @@ function AccountMenuItems({ return ( <> - {isAdmin && ( + {/* "Stop impersonating" and the view-as-user toggle key off raw impersonation, not `isAdmin`: + with "view as user" on, `isAdmin` is false and these are the only ways back out. */} + {(isImpersonating || isAdmin) && (
{isImpersonating ? ( - - Stop impersonating - -
- } - icon={UserCrossIcon} - onClick={stopImpersonating} - leadingIconClassName={cn(SIDE_MENU_POPOVER_ITEM_ICON, IMPERSONATION_ACCENT.text)} - className={SIDE_MENU_POPOVER_ITEM_LABEL} - /> + <> + + Stop impersonating + + + } + icon={UserCrossIcon} + onClick={stopImpersonating} + leadingIconClassName={cn(SIDE_MENU_POPOVER_ITEM_ICON, IMPERSONATION_ACCENT.text)} + className={SIDE_MENU_POPOVER_ITEM_LABEL} + /> + + ) : ( + + + + + ); +} + function AccountMenu({ isAdmin, isImpersonating }: { isAdmin: boolean; isImpersonating: boolean }) { const [isOpen, setIsOpen] = useState(false); const navigation = useNavigation(); diff --git a/apps/webapp/app/hooks/useUser.ts b/apps/webapp/app/hooks/useUser.ts index fd9938fdb9a..aa86ba63865 100644 --- a/apps/webapp/app/hooks/useUser.ts +++ b/apps/webapp/app/hooks/useUser.ts @@ -30,9 +30,23 @@ export function useUserChanged(callback: (user: User | undefined) => void) { useChanged(useOptionalUser, callback); } +/** + * Whether the admin has switched to "view as user" for the current + * impersonation session. Display only — see `hasAdminDisplayAccess`. + */ +export function useIsViewingAsUser(matches?: UIMatch[]): boolean { + const routeMatch = useTypedMatchesData({ + id: "root", + matches, + }); + + return routeMatch?.isViewingAsUser === true; +} + export function useHasAdminAccess(matches?: UIMatch[]): boolean { const user = useOptionalUser(matches); const isImpersonating = useIsImpersonating(matches); + const isViewingAsUser = useIsViewingAsUser(matches); - return Boolean(user?.admin) || isImpersonating; + return (Boolean(user?.admin) || isImpersonating) && !isViewingAsUser; } diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index 09811c99c2d..e6305dc235b 100644 --- a/apps/webapp/app/models/admin.server.ts +++ b/apps/webapp/app/models/admin.server.ts @@ -1,5 +1,5 @@ import { redirect } from "@remix-run/server-runtime"; -import { prisma } from "~/db.server"; +import { prisma, type PrismaClientOrTransaction } from "~/db.server"; import { logger } from "~/services/logger.server"; import type { SearchParams } from "~/routes/admin._index"; import { @@ -213,7 +213,8 @@ export async function redirectWithImpersonation( request: Request, userId: string, path: string, - currentUser?: { id: string; admin: boolean } + currentUser?: { id: string; admin: boolean }, + prismaClient: PrismaClientOrTransaction = prisma ) { const user = currentUser ?? (await requireUser(request)); if (!user.admin) { @@ -224,7 +225,7 @@ export async function redirectWithImpersonation( const ipAddress = extractClientIp(xff); try { - await prisma.impersonationAuditLog.create({ + await prismaClient.impersonationAuditLog.create({ data: { action: "START", adminId: user.id, diff --git a/apps/webapp/app/root.tsx b/apps/webapp/app/root.tsx index 839f44b4bbc..100d7d0c9a6 100644 --- a/apps/webapp/app/root.tsx +++ b/apps/webapp/app/root.tsx @@ -19,6 +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 { getUser } from "./services/session.server"; import { getTimezonePreference } from "./services/preferences/uiPreferences.server"; import { appEnvTitleTag } from "./utils"; @@ -70,6 +71,10 @@ export const loader = async ({ request }: LoaderFunctionArgs) => { }; const user = await getUser(request); + // 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); const headers = new Headers(); headers.append("Set-Cookie", await commitSession(session)); @@ -77,6 +82,7 @@ export const loader = async ({ request }: LoaderFunctionArgs) => { return typedjson( { user, + isViewingAsUser, toastMessage, posthogProjectKey, posthogUiHost, diff --git a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.ts b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.ts deleted file mode 100644 index a47deeac6aa..00000000000 --- a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.ts +++ /dev/null @@ -1,82 +0,0 @@ -import type { LoaderFunctionArgs } from "@remix-run/server-runtime"; -import { redirect } from "remix-typedjson"; -import { $replica } from "~/db.server"; -import { clearImpersonation, redirectWithImpersonation } from "~/models/admin.server"; -import { env } from "~/env.server"; -import { logger } from "~/services/logger.server"; -import { requireUser } from "~/services/session.server"; -import { isSameOriginNavigation } from "~/utils/sameOriginNavigation"; - -export async function loader({ request, params }: LoaderFunctionArgs) { - const user = await requireUser(request); - - // If already impersonating, we need to clear the impersonation - if (user.isImpersonating) { - const url = new URL(request.url); - return clearImpersonation(request, url.pathname); - } - - // Only admins can impersonate - if (!user.admin) { - return redirect("/"); - } - - const path = params["*"]; - const organizationSlug = params.organizationSlug; - - logger.debug("Impersonating user", { path, organizationSlug }); - - if (!organizationSlug) { - logger.debug("Exiting impersonation mode"); - return clearImpersonation(request, "/admin"); - } - - // CSRF gate for the SET-impersonation path. Clearing impersonation - // above is benign and stays reachable without the check. - if (!isSameOriginNavigation(request, env.LOGIN_ORIGIN)) { - logger.warn("Refusing cross-site impersonation entry", { - userId: user.id, - organizationSlug, - referer: request.headers.get("referer"), - secFetchSite: request.headers.get("sec-fetch-site"), - }); - return redirect("/admin"); - } - - const org = await $replica.organization.findFirst({ - where: { - slug: organizationSlug, - deletedAt: null, - }, - select: { - members: { - select: { - user: { - select: { - id: true, - confirmedBasicDetails: true, - }, - }, - }, - }, - }, - }); - - if (!org) { - logger.debug("Organization not found", { organizationSlug }); - return clearImpersonation(request, "/admin"); - } - - const firstValidMember = org.members.find((m) => m.user.confirmedBasicDetails); - - if (!firstValidMember) { - logger.debug("No valid members found", { organizationSlug }); - return clearImpersonation(request, "/admin"); - } - - return redirectWithImpersonation( - request, - firstValidMember.user.id, - `/orgs/${organizationSlug}/${path}` - ); -} diff --git a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx new file mode 100644 index 00000000000..1858f89d401 --- /dev/null +++ b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx @@ -0,0 +1,216 @@ +import { Form } from "@remix-run/react"; +import type { ActionFunctionArgs, LoaderFunctionArgs } from "@remix-run/server-runtime"; +import { redirect, typedjson, useTypedLoaderData } from "remix-typedjson"; +import { MainCenteredContainer } from "~/components/layout/AppLayout"; +import { Button } from "~/components/primitives/Buttons"; +import { Callout } from "~/components/primitives/Callout"; +import { Header1 } from "~/components/primitives/Headers"; +import { Paragraph } from "~/components/primitives/Paragraph"; +import { $replica, prisma, type PrismaClientOrTransaction } from "~/db.server"; +import { env } from "~/env.server"; +import { clearImpersonation, redirectWithImpersonation } from "~/models/admin.server"; +import { logger } from "~/services/logger.server"; +import { requireUser } from "~/services/session.server"; +import { isSameOriginNavigation } from "~/utils/sameOriginNavigation"; + +type ImpersonationTarget = + | { success: true; userId: string; organizationName: string } + | { success: false; reason: "org-not-found" | "no-confirmed-member" }; + +/** + * Read-only lookup of who a `/@/orgs//…` link would impersonate: the + * first organization member who has confirmed their basic details. Writes + * nothing, so it is safe to call while only rendering the consent page. + */ +export async function findImpersonationTarget( + organizationSlug: string, + prismaClient: PrismaClientOrTransaction = $replica +): Promise { + const org = await prismaClient.organization.findFirst({ + where: { + slug: organizationSlug, + deletedAt: null, + }, + select: { + title: true, + members: { + select: { + user: { + select: { + id: true, + confirmedBasicDetails: true, + }, + }, + }, + }, + }, + }); + + if (!org) { + return { success: false, reason: "org-not-found" }; + } + + const firstValidMember = org.members.find((m) => m.user.confirmedBasicDetails); + + if (!firstValidMember) { + return { success: false, reason: "no-confirmed-member" }; + } + + return { success: true, userId: firstValidMember.user.id, organizationName: org.title }; +} + +/** + * Starts impersonating the organization's first confirmed member and lands on + * the requested path with the `/@` prefix stripped. Shared by the same-origin + * loader path and the consent page's POST so there is one implementation. + */ +export async function startImpersonation( + request: Request, + organizationSlug: string, + path: string, + currentUser: { id: string; admin: boolean }, + clients: { read: PrismaClientOrTransaction; write: PrismaClientOrTransaction } = { + read: $replica, + write: prisma, + } +) { + const target = await findImpersonationTarget(organizationSlug, clients.read); + + if (!target.success) { + logger.debug("Cannot impersonate organization", { organizationSlug, reason: target.reason }); + return clearImpersonation(request, "/admin"); + } + + return redirectWithImpersonation( + request, + target.userId, + `/orgs/${organizationSlug}/${path}`, + currentUser, + clients.write + ); +} + +export async function loader({ request, params }: LoaderFunctionArgs) { + const user = await requireUser(request); + + // If already impersonating, we need to clear the impersonation. Redirects are + // thrown, not returned, so the consent page below is the loader's only data + // shape. + if (user.isImpersonating) { + const url = new URL(request.url); + throw await clearImpersonation(request, url.pathname); + } + + // Only admins can impersonate + if (!user.admin) { + throw redirect("/"); + } + + const path = params["*"] ?? ""; + const organizationSlug = params.organizationSlug; + + logger.debug("Impersonating user", { path, organizationSlug }); + + if (!organizationSlug) { + logger.debug("Exiting impersonation mode"); + throw await clearImpersonation(request, "/admin"); + } + + // Starting impersonation is a state change, so it only happens straight away + // for an unambiguously same-origin navigation — that is what stops a + // cross-site navigation from silently starting impersonation. Links opened + // from outside the app (address bar, bookmark, a link shared elsewhere) get + // 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); + } + + logger.warn("Cross-site impersonation entry, showing consent page", { + userId: user.id, + organizationSlug, + referer: request.headers.get("referer"), + secFetchSite: request.headers.get("sec-fetch-site"), + }); + + // Read-only on purpose: nothing is written and no impersonation cookie is set + // until the admin confirms with the POST below. + const target = await findImpersonationTarget(organizationSlug); + + return typedjson({ + organizationSlug, + organizationName: target.success ? target.organizationName : undefined, + destinationPath: `/orgs/${organizationSlug}/${path}`, + canImpersonate: target.success, + }); +} + +export async function action({ request, params }: ActionFunctionArgs) { + if (request.method.toLowerCase() !== "post") { + return new Response("Method not allowed", { status: 405 }); + } + + const user = await requireUser(request); + + if (!user.admin) { + return redirect("/"); + } + + // The consent page posts from our own origin, so this holds. Re-applied here + // so another site cannot drive the POST either. + if (!isSameOriginNavigation(request, env.LOGIN_ORIGIN)) { + logger.warn("Refusing cross-site impersonation submission", { + userId: user.id, + organizationSlug: params.organizationSlug, + referer: request.headers.get("referer"), + secFetchSite: request.headers.get("sec-fetch-site"), + }); + return redirect("/admin"); + } + + const organizationSlug = params.organizationSlug; + + if (!organizationSlug) { + return clearImpersonation(request, "/admin"); + } + + // The form has no `action`, so it posts to the current URL and the + // organization slug plus the splat path arrive here unchanged. + return startImpersonation(request, organizationSlug, params["*"] ?? "", user); +} + +export default function Page() { + const { organizationSlug, organizationName, destinationPath, canImpersonate } = + useTypedLoaderData(); + + return ( + +
+ Impersonate + {canImpersonate ? ( + <> + + Continue to impersonate a member of{" "} + {organizationName ?? organizationSlug} and + open {destinationPath}. + +
+ +
+ + Only continue if you meant to open this link. You'll be signed in as a member of this + organization until you stop impersonating. + + + ) : ( + + There's no organization {organizationSlug}{" "} + with a member you can impersonate. + + )} +
+ + ); +} diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.custom.$dashboardId/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.custom.$dashboardId/route.tsx index 153004813a8..6f7c1d2609c 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.custom.$dashboardId/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.custom.$dashboardId/route.tsx @@ -49,7 +49,7 @@ import { getTaskIdentifiers } from "~/models/task.server"; import { MetricDashboardPresenter } from "~/presenters/v3/MetricDashboardPresenter.server"; import { QueryPresenter } from "~/presenters/v3/QueryPresenter.server"; import { removeFavoritesByUrlSubstring } from "~/services/dashboardPreferences.server"; -import { requireUser } from "~/services/session.server"; +import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; import { EnvironmentParamSchema, queryPath, @@ -98,7 +98,7 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { ]); // Admins and impersonating users can use EXPLAIN - const isAdmin = user.admin || user.isImpersonating; + const isAdmin = hasAdminDisplayAccess(user); // Compute widget count from dashboard layout const widgetCount = Object.keys(dashboard.layout.widgets).length; diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.playground/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.playground/route.tsx index da486898d4f..95f7b0ec946 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.playground/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.playground/route.tsx @@ -22,7 +22,7 @@ import { findProjectBySlug } from "~/models/project.server"; import { findEnvironmentBySlug } from "~/models/runtimeEnvironment.server"; import { playgroundPresenter } from "~/presenters/v3/PlaygroundPresenter.server"; import { RegionsPresenter } from "~/presenters/v3/RegionsPresenter.server"; -import { requireUser } from "~/services/session.server"; +import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; import { docsPath, EnvironmentParamSchema, v3PlaygroundAgentPath } from "~/utils/pathBuilder"; export const meta: MetaFunction = () => { @@ -57,7 +57,7 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { new RegionsPresenter().call({ userId: user.id, projectSlug: projectParam, - isAdmin: user.admin || user.isImpersonating, + isAdmin: hasAdminDisplayAccess(user), }), ]); diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx index 848546a518f..6aecd22be81 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx @@ -11,7 +11,7 @@ import { findProjectBySlug } from "~/models/project.server"; import { findEnvironmentBySlug } from "~/models/runtimeEnvironment.server"; import { QueryPresenter } from "~/presenters/v3/QueryPresenter.server"; import { executeQuery, getDefaultPeriod } from "~/services/queryService.server"; -import { requireUser } from "~/services/session.server"; +import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; import { EnvironmentParamSchema, queryPath } from "~/utils/pathBuilder"; import { canAccessQuery } from "~/v3/canAccessQuery.server"; import { useCurrentPlan } from "../_app.orgs.$organizationSlug/route"; @@ -63,7 +63,7 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { }); // Admins and impersonating users can use EXPLAIN - const isAdmin = user.admin || user.isImpersonating; + const isAdmin = hasAdminDisplayAccess(user); return typedjson({ defaultQuery, @@ -176,7 +176,7 @@ export const action = async ({ request, params }: ActionFunctionArgs) => { const { query, scope, explain: explainParam, period, from, to } = parsed.data; // Only allow explain for admins/impersonating users - const isAdmin = user.admin || user.isImpersonating; + const isAdmin = hasAdminDisplayAccess(user); const explain = explainParam === "true" && isAdmin; try { diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx index 588647f22a7..7059ff6f9c9 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx @@ -54,7 +54,8 @@ import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/m import { resolveOrgIdFromSlug } from "~/models/organization.server"; import { findProjectBySlug } from "~/models/project.server"; import { type Region, RegionsPresenter } from "~/presenters/v3/RegionsPresenter.server"; -import { requireUser } from "~/services/session.server"; +import { getViewingAsUser } from "~/services/impersonation.server"; +import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder"; import { docsPath, @@ -74,7 +75,7 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { presenter.call({ userId: user.id, projectSlug: projectParam, - isAdmin: user.admin || user.isImpersonating, + isAdmin: hasAdminDisplayAccess(user), }) ); @@ -135,7 +136,10 @@ export const action = dashboardAction( service.call({ projectId: project.id, regionId: parsedFormData.data.regionId, - isAdmin: user.admin || user.isImpersonating, + isAdmin: hasAdminDisplayAccess({ + ...user, + isViewingAsUser: await getViewingAsUser(request), + }), }) ); diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx index 321d6391179..fc8ed9fb2d1 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx @@ -67,7 +67,7 @@ import { import { type loader as queuesLoader } from "~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues"; import { clickhouseFactory } from "~/services/clickhouse/clickhouseFactoryInstance.server"; import { logger } from "~/services/logger.server"; -import { requireUser } from "~/services/session.server"; +import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; import { cn } from "~/utils/cn"; import { docsPath, v3RunSpanPath, v3TaskParamsSchema, v3TestPath } from "~/utils/pathBuilder"; import { DeleteTaskRunTemplateService } from "~/v3/services/deleteTaskRunTemplate.server"; @@ -120,7 +120,7 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { new RegionsPresenter().call({ userId: user.id, projectSlug: projectParam, - isAdmin: user.admin || user.isImpersonating, + isAdmin: hasAdminDisplayAccess(user), }), ]); diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam/route.tsx index d22883caa12..9a2127489d4 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam/route.tsx @@ -5,7 +5,7 @@ import { DashboardAgent } from "~/components/dashboard-agent/DashboardAgent"; import { prisma } from "~/db.server"; import { updateCurrentProjectEnvironmentId } from "~/services/dashboardPreferences.server"; import { logger } from "~/services/logger.server"; -import { requireUser } from "~/services/session.server"; +import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; import { tenantContext } from "~/services/tenantContext.server"; import { EnvironmentParamSchema, v3ProjectPath } from "~/utils/pathBuilder"; import { canAccessDashboardAgent } from "~/v3/canAccessDashboardAgent.server"; @@ -91,10 +91,13 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { // the launcher button is hidden when it's not enabled. The org's featureFlags // came from the membership-checked project query above, so we pass them in to // avoid a second org lookup. + // Display-only, so it respects the "view as user" toggle: while that's on we + // hide the launcher an impersonated-into user wouldn't have. + const showAdminUi = hasAdminDisplayAccess(user); const hasDashboardAgentAccess = await canAccessDashboardAgent({ userId: user.id, - isAdmin: user.admin, - isImpersonating: user.isImpersonating, + isAdmin: showAdminUi && user.admin, + isImpersonating: showAdminUi && user.isImpersonating, organizationSlug, orgFeatureFlags: (project.organization.featureFlags as Record) ?? {}, }); diff --git a/apps/webapp/app/routes/resources.impersonation.view-as.ts b/apps/webapp/app/routes/resources.impersonation.view-as.ts new file mode 100644 index 00000000000..4875c1bb53f --- /dev/null +++ b/apps/webapp/app/routes/resources.impersonation.view-as.ts @@ -0,0 +1,36 @@ +import { redirect, type ActionFunctionArgs } from "@remix-run/server-runtime"; +import { z } from "zod"; +import { commitImpersonationSession, setViewingAsUser } from "~/services/impersonation.server"; +import { requireUser } from "~/services/session.server"; +import { sanitizeRedirectPath } from "~/utils"; + +const FormSchema = z.object({ + viewAsUser: z.enum(["true", "false"]), + redirectTo: z.string().optional(), +}); + +export async function action({ request }: ActionFunctionArgs) { + if (request.method.toLowerCase() !== "post") { + return new Response("Method not allowed", { status: 405 }); + } + + const user = await requireUser(request); + + const payload = Object.fromEntries(await request.formData()); + const parsed = FormSchema.safeParse(payload); + const redirectTo = sanitizeRedirectPath(parsed.success ? parsed.data.redirectTo : undefined); + + // Display-only toggle scoped to an impersonation session — outside one there + // is nothing to toggle. + if (!user.isImpersonating || !parsed.success) { + return redirect(redirectTo); + } + + const session = await setViewingAsUser(parsed.data.viewAsUser === "true", request); + + return redirect(redirectTo, { + headers: { + "Set-Cookie": await commitImpersonationSession(session), + }, + }); +} diff --git a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx index 42631d5ff45..9bada394f26 100644 --- a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx +++ b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx @@ -56,7 +56,9 @@ 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 { dashboardAction, dashboardLoader } from "~/services/routeBuilders/dashboardBuilder"; +import { hasAdminDisplayAccess } from "~/services/session.server"; import { checkPermissions } from "~/services/routeBuilders/permissions.server"; import { cn } from "~/utils/cn"; import { EnvironmentParamSchema, v3BulkActionPath } from "~/utils/pathBuilder"; @@ -84,6 +86,13 @@ export const loader = dashboardLoader( throw new Response("Not Found", { status: 404 }); } + // 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), + }); + const presenter = new CreateBulkActionPresenter(); const [data, regionsResult] = await Promise.all([ presenter.call({ @@ -96,7 +105,7 @@ export const loader = dashboardLoader( new RegionsPresenter().call({ userId: user.id, projectSlug: projectParam, - isAdmin: user.admin || user.isImpersonating, + isAdmin, }) ), ]); diff --git a/apps/webapp/app/services/impersonation.server.ts b/apps/webapp/app/services/impersonation.server.ts index a4f3ee59c9f..dd5127c65dd 100644 --- a/apps/webapp/app/services/impersonation.server.ts +++ b/apps/webapp/app/services/impersonation.server.ts @@ -18,6 +18,15 @@ export const impersonationSessionStorage = createCookieSessionStorage({ }, }); +const IMPERSONATED_USER_ID_KEY = "impersonatedUserId"; + +/** + * Display-only "view as user" flag. It lives on the impersonation cookie so it + * is scoped to the impersonation session by construction: stop impersonating + * and the flag goes with it. + */ +const VIEWING_AS_USER_KEY = "viewingAsUser"; + export function getImpersonationSession(request: Request) { return impersonationSessionStorage.getSession(request.headers.get("Cookie")); } @@ -29,13 +38,13 @@ export function commitImpersonationSession(session: Session) { export async function getImpersonationId(request: Request) { const session = await getImpersonationSession(request); - return session.get("impersonatedUserId") as string | undefined; + return session.get(IMPERSONATED_USER_ID_KEY) as string | undefined; } export async function setImpersonationId(userId: string, request: Request) { const session = await getImpersonationSession(request); - session.set("impersonatedUserId", userId); + session.set(IMPERSONATED_USER_ID_KEY, userId); return session; } @@ -43,7 +52,35 @@ export async function setImpersonationId(userId: string, request: Request) { export async function clearImpersonationId(request: Request) { const session = await getImpersonationSession(request); - session.unset("impersonatedUserId"); + session.unset(IMPERSONATED_USER_ID_KEY); + // The view-as-user flag only means anything inside an impersonation session, + // so it never outlives one. + session.unset(VIEWING_AS_USER_KEY); + + return session; +} + +/** + * Whether the admin has asked to see the dashboard the way the impersonated + * user sees it. Only ever true inside an impersonation session. + */ +export async function getViewingAsUser(request: Request) { + const session = await getImpersonationSession(request); + + return ( + typeof session.get(IMPERSONATED_USER_ID_KEY) === "string" && + session.get(VIEWING_AS_USER_KEY) === true + ); +} + +export async function setViewingAsUser(value: boolean, request: Request) { + const session = await getImpersonationSession(request); + + if (value) { + session.set(VIEWING_AS_USER_KEY, true); + } else { + session.unset(VIEWING_AS_USER_KEY); + } return session; } diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts index bdd565cf2f9..1aa90b76a5f 100644 --- a/apps/webapp/app/services/session.server.ts +++ b/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 } from "./impersonation.server"; +import { getImpersonationId, getViewingAsUser } from "./impersonation.server"; import { logger } from "./logger.server"; import { revalidateSsoSession } from "./ssoSessionRevalidation.server"; @@ -136,6 +136,7 @@ export async function requireUser(request: Request) { } const impersonationId = await getImpersonationId(request); + const isImpersonating = !!impersonationId && impersonationId === user.id; return { id: user.id, email: user.email, @@ -148,10 +149,26 @@ export async function requireUser(request: Request) { dashboardPreferences: user.dashboardPreferences, confirmedBasicDetails: user.confirmedBasicDetails, mfaEnabledAt: user.mfaEnabledAt, - isImpersonating: !!impersonationId && impersonationId === user.id, + isImpersonating, + isViewingAsUser: isImpersonating && (await getViewingAsUser(request)), }; } +/** + * Whether admin-only UI should be rendered for this user. + * + * Display only. The "view as user" toggle is cosmetic and must never widen or + * narrow a real security boundary — authorization stays on `user.admin`, the + * route builder's `authorization` block and the per-feature access checks. + */ +export function hasAdminDisplayAccess(user: { + admin: boolean; + isImpersonating: boolean; + isViewingAsUser: boolean; +}): boolean { + return (user.admin || user.isImpersonating) && !user.isViewingAsUser; +} + export async function logout(request: Request) { return redirect("/logout"); } diff --git a/apps/webapp/test/impersonationConsent.test.ts b/apps/webapp/test/impersonationConsent.test.ts new file mode 100644 index 00000000000..4139d3c5235 --- /dev/null +++ b/apps/webapp/test/impersonationConsent.test.ts @@ -0,0 +1,149 @@ +import { containerTest } from "@internal/testcontainers"; +import { describe, expect, vi } from "vitest"; +import { + findImpersonationTarget, + startImpersonation, +} from "~/routes/_app.@.orgs.$organizationSlug.$"; + +vi.setConfig({ testTimeout: 30_000 }); + +function suffix() { + return Math.random().toString(36).slice(2, 10); +} + +// A cross-site `/@/orgs//…` navigation renders a consent page instead of +// starting impersonation, so the only work its loader does is the read-only +// target lookup. Lock that the lookup writes nothing, and that the explicit +// POST is what actually starts impersonation. +describe("impersonation consent page", () => { + containerTest( + "resolving who a link would impersonate is read-only, and the explicit POST starts impersonation", + async ({ prisma }) => { + const admin = await prisma.user.create({ + data: { + email: `admin-${suffix()}@test.local`, + authenticationMethod: "MAGIC_LINK", + admin: true, + }, + }); + + // First member has never confirmed their details, so it must be skipped. + const unconfirmed = await prisma.user.create({ + data: { + email: `unconfirmed-${suffix()}@test.local`, + authenticationMethod: "MAGIC_LINK", + confirmedBasicDetails: false, + }, + }); + const confirmed = await prisma.user.create({ + data: { + email: `confirmed-${suffix()}@test.local`, + authenticationMethod: "MAGIC_LINK", + confirmedBasicDetails: true, + }, + }); + + const slug = `acme-${suffix()}`; + const org = await prisma.organization.create({ + data: { + title: "Acme Inc", + slug, + members: { create: [{ userId: unconfirmed.id }, { userId: confirmed.id }] }, + }, + }); + + // What the consent-page loader does: look up the target, nothing else. + const target = await findImpersonationTarget(org.slug, prisma); + expect(target).toEqual({ + success: true, + userId: confirmed.id, + organizationName: "Acme Inc", + }); + + // Read-only: no impersonation was recorded by rendering the page. + expect(await prisma.impersonationAuditLog.count()).toBe(0); + + // The consent page's POST: same-origin, and it does start impersonation. + const response = await startImpersonation( + new Request(`http://localhost:3030/@/orgs/${org.slug}/projects/p/runs/run_123`, { + method: "POST", + headers: { "sec-fetch-site": "same-origin" }, + }), + org.slug, + "projects/p/runs/run_123", + { id: admin.id, admin: true }, + { read: prisma, write: prisma } + ); + + expect(response.status).toBe(302); + // The splat path is preserved, with the `/@` prefix stripped. + expect(response.headers.get("location")).toBe(`/orgs/${org.slug}/projects/p/runs/run_123`); + expect(response.headers.get("set-cookie")).toContain("__impersonate="); + + const auditLogs = await prisma.impersonationAuditLog.findMany(); + expect(auditLogs).toHaveLength(1); + expect(auditLogs[0]).toMatchObject({ + action: "START", + adminId: admin.id, + targetId: confirmed.id, + }); + } + ); + + containerTest("an unknown organization slug cannot be impersonated", async ({ prisma }) => { + expect(await findImpersonationTarget(`missing-${suffix()}`, prisma)).toEqual({ + success: false, + reason: "org-not-found", + }); + }); + + containerTest( + "an organization with no confirmed members cannot be impersonated", + async ({ prisma }) => { + const user = await prisma.user.create({ + data: { + email: `unconfirmed-${suffix()}@test.local`, + authenticationMethod: "MAGIC_LINK", + confirmedBasicDetails: false, + }, + }); + + const org = await prisma.organization.create({ + data: { + title: "Nobody Inc", + slug: `nobody-${suffix()}`, + members: { create: [{ userId: user.id }] }, + }, + }); + + expect(await findImpersonationTarget(org.slug, prisma)).toEqual({ + success: false, + reason: "no-confirmed-member", + }); + } + ); + + containerTest("a deleted organization cannot be impersonated", async ({ prisma }) => { + const user = await prisma.user.create({ + data: { + email: `confirmed-${suffix()}@test.local`, + authenticationMethod: "MAGIC_LINK", + confirmedBasicDetails: true, + }, + }); + + const org = await prisma.organization.create({ + data: { + title: "Gone Inc", + slug: `gone-${suffix()}`, + deletedAt: new Date(), + members: { create: [{ userId: user.id }] }, + }, + }); + + expect(await findImpersonationTarget(org.slug, prisma)).toEqual({ + success: false, + reason: "org-not-found", + }); + }); +}); diff --git a/apps/webapp/test/viewAsUser.test.ts b/apps/webapp/test/viewAsUser.test.ts new file mode 100644 index 00000000000..299aaf49449 --- /dev/null +++ b/apps/webapp/test/viewAsUser.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it } from "vitest"; +import { + clearImpersonationId, + commitImpersonationSession, + getImpersonationId, + getViewingAsUser, + setImpersonationId, + setViewingAsUser, +} from "~/services/impersonation.server"; +import { hasAdminDisplayAccess } from "~/services/session.server"; +import type { Session } from "@remix-run/node"; + +// `commitSession` returns a full Set-Cookie value; a request only needs the +// name=value pair back. +async function requestWith(session: Session) { + const setCookie = await commitImpersonationSession(session); + return new Request("http://localhost:3030/orgs/acme", { + headers: { Cookie: setCookie.split(";")[0] }, + }); +} + +// 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); + }); + + 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); + + const viewingAsUser = await requestWith(await setViewingAsUser(true, impersonating)); + expect(await getImpersonationId(viewingAsUser)).toBe("user_1"); + expect(await getViewingAsUser(viewingAsUser)).toBe(true); + + const showingAdminUi = await requestWith(await setViewingAsUser(false, viewingAsUser)); + expect(await getImpersonationId(showingAdminUi)).toBe("user_1"); + expect(await getViewingAsUser(showingAdminUi)).toBe(false); + }); + + it("is dropped when impersonation is cleared", async () => { + const start = new Request("http://localhost:3030/orgs/acme"); + const impersonating = await requestWith(await setImpersonationId("user_1", start)); + const viewingAsUser = await requestWith(await setViewingAsUser(true, impersonating)); + + const cleared = await requestWith(await clearImpersonationId(viewingAsUser)); + + expect(await getImpersonationId(cleared)).toBeUndefined(); + expect(await getViewingAsUser(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); + }); +}); + +describe("hasAdminDisplayAccess", () => { + it("shows admin UI to admins and to impersonating sessions", () => { + expect( + hasAdminDisplayAccess({ admin: true, isImpersonating: false, isViewingAsUser: false }) + ).toBe(true); + expect( + hasAdminDisplayAccess({ admin: false, isImpersonating: true, isViewingAsUser: false }) + ).toBe(true); + }); + + it("hides admin UI to everyone else", () => { + expect( + hasAdminDisplayAccess({ admin: false, isImpersonating: false, isViewingAsUser: false }) + ).toBe(false); + }); + + it("hides admin UI while viewing as the user", () => { + expect( + hasAdminDisplayAccess({ admin: true, isImpersonating: true, isViewingAsUser: true }) + ).toBe(false); + expect( + hasAdminDisplayAccess({ admin: false, isImpersonating: true, isViewingAsUser: true }) + ).toBe(false); + }); +}); From 5612c332c83b6af77a55a2518003c3dc49543714 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 19:40:15 +0000 Subject: [PATCH 2/8] fix(webapp): keep server-only impersonation helpers out of the route module The consent route exported `findImpersonationTarget` and `startImpersonation` alongside its loader and action. Remix only strips `loader`, `action` and `headers` from a route module when building the browser bundle, so those extra exports kept `~/db.server` in the client graph and the build failed with "Server-only module referenced by client". Move both helpers into `~/models/admin.server`, next to `redirectWithImpersonation` and `clearImpersonation`, and import them from the route. The route now only exports `loader`, `action` and its component, so every `.server` import it has is removed for the client. --- apps/webapp/app/models/admin.server.ts | 79 ++++++++++++++++- .../_app.@.orgs.$organizationSlug.$.tsx | 87 ++----------------- apps/webapp/test/impersonationConsent.test.ts | 5 +- 3 files changed, 88 insertions(+), 83 deletions(-) diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index e6305dc235b..cf533cb7170 100644 --- a/apps/webapp/app/models/admin.server.ts +++ b/apps/webapp/app/models/admin.server.ts @@ -1,5 +1,5 @@ import { redirect } from "@remix-run/server-runtime"; -import { prisma, type PrismaClientOrTransaction } from "~/db.server"; +import { $replica, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { logger } from "~/services/logger.server"; import type { SearchParams } from "~/routes/admin._index"; import { @@ -248,6 +248,83 @@ export async function redirectWithImpersonation( }); } +type ImpersonationTarget = + | { success: true; userId: string; organizationName: string } + | { success: false; reason: "org-not-found" | "no-confirmed-member" }; + +/** + * Read-only lookup of who a `/@/orgs//…` link would impersonate: the + * first organization member who has confirmed their basic details. Writes + * nothing, so it is safe to call while only rendering the consent page. + */ +export async function findImpersonationTarget( + organizationSlug: string, + prismaClient: PrismaClientOrTransaction = $replica +): Promise { + const org = await prismaClient.organization.findFirst({ + where: { + slug: organizationSlug, + deletedAt: null, + }, + select: { + title: true, + members: { + select: { + user: { + select: { + id: true, + confirmedBasicDetails: true, + }, + }, + }, + }, + }, + }); + + if (!org) { + return { success: false, reason: "org-not-found" }; + } + + const firstValidMember = org.members.find((m) => m.user.confirmedBasicDetails); + + if (!firstValidMember) { + return { success: false, reason: "no-confirmed-member" }; + } + + return { success: true, userId: firstValidMember.user.id, organizationName: org.title }; +} + +/** + * Starts impersonating the organization's first confirmed member and lands on + * the requested path with the `/@` prefix stripped. Shared by the same-origin + * loader path and the consent page's POST so there is one implementation. + */ +export async function startImpersonation( + request: Request, + organizationSlug: string, + path: string, + currentUser: { id: string; admin: boolean }, + clients: { read: PrismaClientOrTransaction; write: PrismaClientOrTransaction } = { + read: $replica, + write: prisma, + } +) { + const target = await findImpersonationTarget(organizationSlug, clients.read); + + if (!target.success) { + logger.debug("Cannot impersonate organization", { organizationSlug, reason: target.reason }); + return clearImpersonation(request, "/admin"); + } + + return redirectWithImpersonation( + request, + target.userId, + `/orgs/${organizationSlug}/${path}`, + currentUser, + clients.write + ); +} + export async function clearImpersonation(request: Request, path: string) { const authUser = await authenticator.isAuthenticated(request); const targetId = await getImpersonationId(request); diff --git a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx index 1858f89d401..418cc10c23b 100644 --- a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx +++ b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx @@ -6,89 +6,20 @@ import { Button } from "~/components/primitives/Buttons"; import { Callout } from "~/components/primitives/Callout"; import { Header1 } from "~/components/primitives/Headers"; import { Paragraph } from "~/components/primitives/Paragraph"; -import { $replica, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { env } from "~/env.server"; -import { clearImpersonation, redirectWithImpersonation } from "~/models/admin.server"; +import { + clearImpersonation, + findImpersonationTarget, + startImpersonation, +} from "~/models/admin.server"; import { logger } from "~/services/logger.server"; import { requireUser } from "~/services/session.server"; import { isSameOriginNavigation } from "~/utils/sameOriginNavigation"; -type ImpersonationTarget = - | { success: true; userId: string; organizationName: string } - | { success: false; reason: "org-not-found" | "no-confirmed-member" }; - -/** - * Read-only lookup of who a `/@/orgs//…` link would impersonate: the - * first organization member who has confirmed their basic details. Writes - * nothing, so it is safe to call while only rendering the consent page. - */ -export async function findImpersonationTarget( - organizationSlug: string, - prismaClient: PrismaClientOrTransaction = $replica -): Promise { - const org = await prismaClient.organization.findFirst({ - where: { - slug: organizationSlug, - deletedAt: null, - }, - select: { - title: true, - members: { - select: { - user: { - select: { - id: true, - confirmedBasicDetails: true, - }, - }, - }, - }, - }, - }); - - if (!org) { - return { success: false, reason: "org-not-found" }; - } - - const firstValidMember = org.members.find((m) => m.user.confirmedBasicDetails); - - if (!firstValidMember) { - return { success: false, reason: "no-confirmed-member" }; - } - - return { success: true, userId: firstValidMember.user.id, organizationName: org.title }; -} - -/** - * Starts impersonating the organization's first confirmed member and lands on - * the requested path with the `/@` prefix stripped. Shared by the same-origin - * loader path and the consent page's POST so there is one implementation. - */ -export async function startImpersonation( - request: Request, - organizationSlug: string, - path: string, - currentUser: { id: string; admin: boolean }, - clients: { read: PrismaClientOrTransaction; write: PrismaClientOrTransaction } = { - read: $replica, - write: prisma, - } -) { - const target = await findImpersonationTarget(organizationSlug, clients.read); - - if (!target.success) { - logger.debug("Cannot impersonate organization", { organizationSlug, reason: target.reason }); - return clearImpersonation(request, "/admin"); - } - - return redirectWithImpersonation( - request, - target.userId, - `/orgs/${organizationSlug}/${path}`, - currentUser, - clients.write - ); -} +// Everything this route's loader and action touch on the server lives in +// `~/models/admin.server` on purpose: Remix only strips `loader`, `action` and +// `headers` from a route module for the browser bundle, so any other export +// here would drag server-only modules into the client build. export async function loader({ request, params }: LoaderFunctionArgs) { const user = await requireUser(request); diff --git a/apps/webapp/test/impersonationConsent.test.ts b/apps/webapp/test/impersonationConsent.test.ts index 4139d3c5235..b8f09afa1c2 100644 --- a/apps/webapp/test/impersonationConsent.test.ts +++ b/apps/webapp/test/impersonationConsent.test.ts @@ -1,9 +1,6 @@ import { containerTest } from "@internal/testcontainers"; import { describe, expect, vi } from "vitest"; -import { - findImpersonationTarget, - startImpersonation, -} from "~/routes/_app.@.orgs.$organizationSlug.$"; +import { findImpersonationTarget, startImpersonation } from "~/models/admin.server"; vi.setConfig({ testTimeout: 30_000 }); From 753d2adaa3744c2b113cbe5aef34f44d00193c6a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 20:42:57 +0000 Subject: [PATCH 3/8] fix(webapp): keep the impersonation deep link and honor the view-as-user toggle The consent page's form had no `action`, so React Router resolved it to the matched route's `pathnameBase`. Without `future.v3_relativeSplatPath` that excludes the splat, so the form posted to `/@/orgs/`, the action saw an empty splat, and the admin landed on the organization root instead of the deep link the page had just named. The form now posts to an explicit absolute path. The query string was dropped too. A `/@/runs/` link redirects to a run path carrying `?span=`, which selects the span to open, so both the displayed destination and the final redirect now carry the incoming search. The debug tooltip and the "Debug run" button still checked `!hasAdminAccess && !isImpersonating`, which stays true with the toggle on, so the two most visible admin-only affordances kept rendering. `useHasAdminAccess` already folds in impersonation and the toggle, so the extra check is gone. The replay dialog's region picker was likewise still keyed off raw impersonation and now uses `hasAdminDisplayAccess`. Also: gate the view-as-user route on a same-origin navigation like the other impersonation routes, stop it flat-route-nesting under `resources.impersonation` (the URL is unchanged), and log consent-page entry at info with only the referer's origin, since it is expected for any link opened outside the app. --- apps/webapp/app/components/admin/debugRun.tsx | 6 +- .../app/components/admin/debugTooltip.tsx | 7 +- apps/webapp/app/models/admin.server.ts | 7 +- .../_app.@.orgs.$organizationSlug.$.tsx | 36 ++++-- ...ts => resources.impersonation_.view-as.ts} | 14 +++ .../resources.taskruns.$runParam.replay.ts | 4 +- .../app/utils/impersonationPaths.test.ts | 119 ++++++++++++++++++ apps/webapp/app/utils/pathBuilder.ts | 35 ++++++ apps/webapp/test/impersonationConsent.test.ts | 19 ++- 9 files changed, 225 insertions(+), 22 deletions(-) rename apps/webapp/app/routes/{resources.impersonation.view-as.ts => resources.impersonation_.view-as.ts} (67%) create mode 100644 apps/webapp/app/utils/impersonationPaths.test.ts diff --git a/apps/webapp/app/components/admin/debugRun.tsx b/apps/webapp/app/components/admin/debugRun.tsx index 705e3169a8b..049c5cd08c3 100644 --- a/apps/webapp/app/components/admin/debugRun.tsx +++ b/apps/webapp/app/components/admin/debugRun.tsx @@ -1,4 +1,3 @@ -import { useIsImpersonating } from "~/hooks/useOrganizations"; import { useHasAdminAccess } from "~/hooks/useUser"; import { Button } from "../primitives/Buttons"; import { Dialog, DialogContent, DialogHeader, DialogTrigger } from "../primitives/Dialog"; @@ -12,10 +11,11 @@ import * as Property from "~/components/primitives/PropertyTable"; import { ClipboardField } from "../primitives/ClipboardField"; export function AdminDebugRun({ friendlyId }: { friendlyId: string }) { + // `useHasAdminAccess` already folds in impersonation and the "view as user" + // toggle, so this one check is enough. const hasAdminAccess = useHasAdminAccess(); - const isImpersonating = useIsImpersonating(); - if (!hasAdminAccess && !isImpersonating) { + if (!hasAdminAccess) { return null; } diff --git a/apps/webapp/app/components/admin/debugTooltip.tsx b/apps/webapp/app/components/admin/debugTooltip.tsx index 4157f898163..1729bfa740c 100644 --- a/apps/webapp/app/components/admin/debugTooltip.tsx +++ b/apps/webapp/app/components/admin/debugTooltip.tsx @@ -8,15 +8,16 @@ import { TooltipTrigger, } from "~/components/primitives/Tooltip"; import { useOptionalEnvironment } from "~/hooks/useEnvironment"; -import { useIsImpersonating, useOptionalOrganization } from "~/hooks/useOrganizations"; +import { useOptionalOrganization } from "~/hooks/useOrganizations"; import { useOptionalProject } from "~/hooks/useProject"; import { useHasAdminAccess, useUser } from "~/hooks/useUser"; export function AdminDebugTooltip({ children }: { children?: React.ReactNode }) { + // `useHasAdminAccess` already folds in impersonation and the "view as user" + // toggle, so this one check is enough. const hasAdminAccess = useHasAdminAccess(); - const isImpersonating = useIsImpersonating(); - if (!hasAdminAccess && !isImpersonating) { + if (!hasAdminAccess) { return null; } diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index cf533cb7170..e93844dbaed 100644 --- a/apps/webapp/app/models/admin.server.ts +++ b/apps/webapp/app/models/admin.server.ts @@ -11,6 +11,7 @@ import { import { authenticator } from "~/services/auth.server"; import { requireUser } from "~/services/session.server"; import { extractClientIp } from "~/utils/extractClientIp.server"; +import { impersonationDestinationPath } from "~/utils/pathBuilder"; const pageSize = 20; @@ -298,6 +299,10 @@ export async function findImpersonationTarget( * Starts impersonating the organization's first confirmed member and lands on * the requested path with the `/@` prefix stripped. Shared by the same-origin * loader path and the consent page's POST so there is one implementation. + * + * The destination keeps the incoming query string: both entry points are served + * at the `/@`-prefixed URL, so `request.url` carries the same search the link + * arrived with (for example the `?span=` a `/@/runs/` link redirects with). */ export async function startImpersonation( request: Request, @@ -319,7 +324,7 @@ export async function startImpersonation( return redirectWithImpersonation( request, target.userId, - `/orgs/${organizationSlug}/${path}`, + impersonationDestinationPath(organizationSlug, path, new URL(request.url).search), currentUser, clients.write ); diff --git a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx index 418cc10c23b..6b3319d8e8e 100644 --- a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx +++ b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx @@ -14,6 +14,10 @@ import { } from "~/models/admin.server"; import { logger } from "~/services/logger.server"; import { requireUser } from "~/services/session.server"; +import { + impersonationConsentPostBackPath, + impersonationDestinationPath, +} from "~/utils/pathBuilder"; import { isSameOriginNavigation } from "~/utils/sameOriginNavigation"; // Everything this route's loader and action touch on the server lives in @@ -57,10 +61,14 @@ export async function loader({ request, params }: LoaderFunctionArgs) { throw await startImpersonation(request, organizationSlug, path, user); } - logger.warn("Cross-site impersonation entry, showing consent page", { + // Expected for any link opened outside the app (address bar, bookmark, a link + // shared elsewhere), so this is routine rather than suspicious. Only the + // referer's origin is logged — the full referer can carry another site's path + // and query string. + logger.info("Impersonation entry outside the app, showing consent page", { userId: user.id, organizationSlug, - referer: request.headers.get("referer"), + refererOrigin: refererOrigin(request), secFetchSite: request.headers.get("sec-fetch-site"), }); @@ -68,14 +76,27 @@ export async function loader({ request, params }: LoaderFunctionArgs) { // until the admin confirms with the POST below. const target = await findImpersonationTarget(organizationSlug); + const search = new URL(request.url).search; + return typedjson({ organizationSlug, organizationName: target.success ? target.organizationName : undefined, - destinationPath: `/orgs/${organizationSlug}/${path}`, + destinationPath: impersonationDestinationPath(organizationSlug, path, search), + postBackPath: impersonationConsentPostBackPath(organizationSlug, path, search), canImpersonate: target.success, }); } +function refererOrigin(request: Request): string | undefined { + const referer = request.headers.get("referer"); + if (!referer) return undefined; + try { + return new URL(referer).origin; + } catch { + return undefined; + } +} + export async function action({ request, params }: ActionFunctionArgs) { if (request.method.toLowerCase() !== "post") { return new Response("Method not allowed", { status: 405 }); @@ -105,13 +126,14 @@ export async function action({ request, params }: ActionFunctionArgs) { return clearImpersonation(request, "/admin"); } - // The form has no `action`, so it posts to the current URL and the - // organization slug plus the splat path arrive here unchanged. + // 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); } export default function Page() { - const { organizationSlug, organizationName, destinationPath, canImpersonate } = + const { organizationSlug, organizationName, destinationPath, postBackPath, canImpersonate } = useTypedLoaderData(); return ( @@ -125,7 +147,7 @@ export default function Page() { {organizationName ?? organizationSlug} and open {destinationPath}. -
+ diff --git a/apps/webapp/app/routes/resources.impersonation.view-as.ts b/apps/webapp/app/routes/resources.impersonation_.view-as.ts similarity index 67% rename from apps/webapp/app/routes/resources.impersonation.view-as.ts rename to apps/webapp/app/routes/resources.impersonation_.view-as.ts index 4875c1bb53f..ab1ea68baf6 100644 --- a/apps/webapp/app/routes/resources.impersonation.view-as.ts +++ b/apps/webapp/app/routes/resources.impersonation_.view-as.ts @@ -1,8 +1,11 @@ import { redirect, type ActionFunctionArgs } from "@remix-run/server-runtime"; import { z } from "zod"; +import { env } from "~/env.server"; import { commitImpersonationSession, setViewingAsUser } from "~/services/impersonation.server"; +import { logger } from "~/services/logger.server"; import { requireUser } from "~/services/session.server"; import { sanitizeRedirectPath } from "~/utils"; +import { isSameOriginNavigation } from "~/utils/sameOriginNavigation"; const FormSchema = z.object({ viewAsUser: z.enum(["true", "false"]), @@ -16,6 +19,17 @@ export async function action({ request }: ActionFunctionArgs) { const user = await requireUser(request); + // The toggle is submitted from our own side menu, so this holds. Applied here + // for the same reason as the other impersonation routes: no other site gets to + // drive this state change. + if (!isSameOriginNavigation(request, env.LOGIN_ORIGIN)) { + logger.warn("Refusing cross-site view-as-user submission", { + userId: user.id, + secFetchSite: request.headers.get("sec-fetch-site"), + }); + return redirect("/"); + } + const payload = Object.fromEntries(await request.formData()); const parsed = FormSchema.safeParse(payload); const redirectTo = sanitizeRedirectPath(parsed.success ? parsed.data.redirectTo : undefined); diff --git a/apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts b/apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts index d6fe04be99f..f113824075d 100644 --- a/apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts +++ b/apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts @@ -8,7 +8,7 @@ import { $replica, prisma } from "~/db.server"; import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/message.server"; import { displayableEnvironment } from "~/models/runtimeEnvironment.server"; import { logger } from "~/services/logger.server"; -import { requireUser } from "~/services/session.server"; +import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder"; import { sortEnvironments } from "~/utils/environmentSort"; import { v3RunSpanPath } from "~/utils/pathBuilder"; @@ -169,7 +169,7 @@ export async function loader({ request, params }: LoaderFunctionArgs) { new RegionsPresenter().call({ userId, projectSlug, - isAdmin: user.admin || user.isImpersonating, + isAdmin: hasAdminDisplayAccess(user), }), ]); diff --git a/apps/webapp/app/utils/impersonationPaths.test.ts b/apps/webapp/app/utils/impersonationPaths.test.ts new file mode 100644 index 00000000000..e3dba8c8241 --- /dev/null +++ b/apps/webapp/app/utils/impersonationPaths.test.ts @@ -0,0 +1,119 @@ +import { matchRoutes, resolveTo } from "@remix-run/router"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { impersonationConsentPostBackPath, impersonationDestinationPath } from "./pathBuilder"; + +// The route that renders the impersonation consent page, as the flat-route +// convention compiles `_app.@.orgs.$organizationSlug.$.tsx`. +const CONSENT_ROUTES = [ + { + id: "routes/_app", + children: [ + { + id: "routes/_app.@.orgs.$organizationSlug.$", + path: "@/orgs/:organizationSlug/*", + }, + ], + }, +]; + +/** + * What the consent route's `action` would receive for a POST to `pathname`: + * the organization slug and the splat, straight off the router. + */ +function actionParamsFor(pathname: string) { + const matches = matchRoutes(CONSENT_ROUTES, pathname); + const leaf = matches?.[matches.length - 1]; + return { + organizationSlug: leaf?.params.organizationSlug, + splat: leaf?.params["*"], + }; +} + +/** + * What `` with NO `action` prop resolves to, reproducing + * `useFormAction()`: `resolveTo(".", …)` over the matched routes' contributing + * pathnames. This app does not enable `future.v3_relativeSplatPath`, so each + * contributing match supplies its `pathnameBase`. + */ +function relativeFormAction(pathname: string) { + const matches = matchRoutes(CONSENT_ROUTES, pathname) ?? []; + const contributing = matches.filter((m, i) => i === 0 || Boolean(m.route.path)); + const routePathnames = contributing.map((m) => m.pathnameBase); + return resolveTo(".", routePathnames, pathname, false).pathname; +} + +describe("impersonation consent post-back path", () => { + const slug = "acme-inc"; + // A `/@/runs/` link redirects here: a deep run path plus the `?span=` + // that selects which span to open. + const splat = "projects/proj_123/env/prod/runs/run_abc"; + const search = "?span=span_xyz"; + + it("keeps the splat and the query string", () => { + expect(impersonationConsentPostBackPath(slug, splat, search)).toBe( + `/@/orgs/${slug}/${splat}${search}` + ); + expect(impersonationDestinationPath(slug, splat, search)).toBe( + `/orgs/${slug}/${splat}${search}` + ); + }); + + it("omits the query string when there isn't one", () => { + expect(impersonationConsentPostBackPath(slug, splat)).toBe(`/@/orgs/${slug}/${splat}`); + expect(impersonationDestinationPath(slug, splat)).toBe(`/orgs/${slug}/${splat}`); + }); + + it("round-trips the slug and splat back to the action", () => { + const postBackPath = impersonationConsentPostBackPath(slug, splat, search); + + expect(actionParamsFor(new URL(postBackPath, "http://localhost").pathname)).toEqual({ + organizationSlug: slug, + splat, + }); + }); + + it("the destination the action redirects to matches the one the page displayed", () => { + const postBackPath = impersonationConsentPostBackPath(slug, splat, search); + const url = new URL(postBackPath, "http://localhost"); + const params = actionParamsFor(url.pathname); + + // What `startImpersonation` builds from the action's params + request URL. + expect(impersonationDestinationPath(params.organizationSlug!, params.splat!, url.search)).toBe( + impersonationDestinationPath(slug, splat, search) + ); + }); + + // The regression this file exists for. A relative `` action drops the + // splat, so the action would start impersonation and land the admin on the + // organization root rather than the deep link the consent page promised. + it("a relative form action would drop the splat, so the path must be explicit", () => { + const consentUrl = `/@/orgs/${slug}/${splat}`; + + expect(relativeFormAction(consentUrl)).toBe(`/@/orgs/${slug}`); + expect(actionParamsFor(relativeFormAction(consentUrl))).toEqual({ + organizationSlug: slug, + splat: "", + }); + + // The explicit path does not. + expect(impersonationConsentPostBackPath(slug, splat)).toBe(consentUrl); + }); + + // Ideally this would render the route and read the emitted `action` + // attribute, but the route module imports `~/env.server`, which validates the + // full server environment at import time and so cannot be pulled into a unit + // test. Asserting on the source is the next best guard: it fails if the + // `action` prop is ever dropped, which is the whole bug. + it("the consent form names its action explicitly", () => { + const source = readFileSync( + join(__dirname, "../routes/_app.@.orgs.$organizationSlug.$.tsx"), + "utf8" + ); + + const form = source.match(/]*>/); + expect(form).not.toBeNull(); + expect(form![0]).toMatch(/action=\{postBackPath\}/); + }); +}); diff --git a/apps/webapp/app/utils/pathBuilder.ts b/apps/webapp/app/utils/pathBuilder.ts index edd65f8bde4..d50d9e10f30 100644 --- a/apps/webapp/app/utils/pathBuilder.ts +++ b/apps/webapp/app/utils/pathBuilder.ts @@ -62,6 +62,41 @@ export function impersonate(path: string) { return `/@${path}`; } +/** + * Where a `/@/orgs//` impersonation link lands once impersonation + * has started: the same deep link with the `/@` prefix stripped. + * + * `search` must be carried through explicitly. A `/@/runs/` link redirects + * to a `v3RunSpanPath`, whose `?span=` selects the span to open, so + * dropping it lands the admin on the run with nothing selected. + */ +export function impersonationDestinationPath( + organizationSlug: string, + splatPath: string, + search: string = "" +) { + return `/orgs/${organizationSlug}/${splatPath}${search}`; +} + +/** + * Where the impersonation consent page's form must POST back to. + * + * The form has to name this path explicitly. A `` with no `action` + * resolves to `useResolvedPath(".")`, and because this app does not enable + * `future.v3_relativeSplatPath`, that resolves to the matched route's + * `pathnameBase` — which excludes the splat. The form would post to + * `/@/orgs/`, the action would see an empty splat, and the admin would + * land on the organization root instead of the deep link the consent page just + * promised them. + */ +export function impersonationConsentPostBackPath( + organizationSlug: string, + splatPath: string, + search: string = "" +) { + return impersonate(impersonationDestinationPath(organizationSlug, splatPath, search)); +} + export function accountPath() { return `/account`; } diff --git a/apps/webapp/test/impersonationConsent.test.ts b/apps/webapp/test/impersonationConsent.test.ts index b8f09afa1c2..a481338e245 100644 --- a/apps/webapp/test/impersonationConsent.test.ts +++ b/apps/webapp/test/impersonationConsent.test.ts @@ -61,11 +61,16 @@ describe("impersonation consent page", () => { expect(await prisma.impersonationAuditLog.count()).toBe(0); // The consent page's POST: same-origin, and it does start impersonation. + // The `?span=` is what a `/@/runs/` link redirects with, so it has to + // survive to the destination or the run opens with no span selected. const response = await startImpersonation( - new Request(`http://localhost:3030/@/orgs/${org.slug}/projects/p/runs/run_123`, { - method: "POST", - headers: { "sec-fetch-site": "same-origin" }, - }), + new Request( + `http://localhost:3030/@/orgs/${org.slug}/projects/p/runs/run_123?span=span_abc`, + { + method: "POST", + headers: { "sec-fetch-site": "same-origin" }, + } + ), org.slug, "projects/p/runs/run_123", { id: admin.id, admin: true }, @@ -73,8 +78,10 @@ describe("impersonation consent page", () => { ); expect(response.status).toBe(302); - // The splat path is preserved, with the `/@` prefix stripped. - expect(response.headers.get("location")).toBe(`/orgs/${org.slug}/projects/p/runs/run_123`); + // The splat path and query string are preserved, `/@` prefix stripped. + expect(response.headers.get("location")).toBe( + `/orgs/${org.slug}/projects/p/runs/run_123?span=span_abc` + ); expect(response.headers.get("set-cookie")).toContain("__impersonate="); const auditLogs = await prisma.impersonationAuditLog.findMany(); From 9d12b5f9a6229520b72c59ab6307f958c847d71f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 21:03:57 +0000 Subject: [PATCH 4/8] test(webapp): add hasAdminDisplayAccess to the run-GET guard's session mock The replay loader now derives the region picker's `isAdmin` from `hasAdminDisplayAccess`, but this file's `vi.mock` of `~/services/session.server` is a factory that replaces the whole module, and it only returned the three functions the route graph used before. Accessing an export a factory mock does not define throws, so every case in the file failed once the loader was reached: Error: [vitest] No "hasAdminDisplayAccess" export is defined on the "~/services/session.server" mock. Did you forget to return it from "vi.mock"? The stub mirrors the real predicate rather than importing it, since pulling the original module in would evaluate the server-only import graph this file deliberately keeps out. The fixture user gains `isViewingAsUser: false`, so the flag stays false exactly as it did before, and the presenter that reads it is mocked out anyway. Co-Authored-By: Claude --- .../test/runGetRoutes.replicaLag.guard.test.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/apps/webapp/test/runGetRoutes.replicaLag.guard.test.ts b/apps/webapp/test/runGetRoutes.replicaLag.guard.test.ts index 8d844f550b3..02c1e19f3ab 100644 --- a/apps/webapp/test/runGetRoutes.replicaLag.guard.test.ts +++ b/apps/webapp/test/runGetRoutes.replicaLag.guard.test.ts @@ -36,7 +36,12 @@ vi.setConfig({ testTimeout: 120_000, hookTimeout: 120_000 }); // `authUser`, `resolvedEnv`, `logDetail`, `project`, `environment` feed the mocked fix-orthogonal deps. const cp = vi.hoisted(() => ({ client: undefined as any })); const router = vi.hoisted(() => ({ store: undefined as any })); -const authUser = vi.hoisted(() => ({ id: "user_rrg_guard", admin: false, isImpersonating: false })); +const authUser = vi.hoisted(() => ({ + id: "user_rrg_guard", + admin: false, + isImpersonating: false, + isViewingAsUser: false, +})); const resolved = vi.hoisted(() => ({ authEnv: undefined as any, env: undefined as any, @@ -104,11 +109,20 @@ vi.mock("~/v3/runStore.server", () => ({ ), })); -// Auth/session (orthogonal): fixed user id. +// Auth/session (orthogonal): fixed user id. `hasAdminDisplayAccess` mirrors the real predicate +// rather than importing it: a factory mock replaces the whole module, and pulling the original in +// would evaluate session.server's server-only import graph, which this test deliberately keeps out. +// Only the replay loader's region-picker flag reads it, and RegionsPresenter is mocked below, so the +// value is orthogonal to every proof here — it just has to exist, or the mock throws on access. vi.mock("~/services/session.server", () => ({ requireUserId: async () => authUser.id, requireUser: async () => authUser, getUserId: async () => authUser.id, + hasAdminDisplayAccess: (user: { + admin: boolean; + isImpersonating: boolean; + isViewingAsUser: boolean; + }) => (user.admin || user.isImpersonating) && !user.isViewingAsUser, })); // Control-plane env resolution (a downstream cross-DB lookup, orthogonal to the run-store read): From a24b49f780909fd93416a7cf741244d93f8344cf Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 08:29:37 +0000 Subject: [PATCH 5/8] fix(webapp): drop the impersonation accent in view-as-user mode, keep capability gates raw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The yellow side-menu border is the loudest tell that you are not the user, so "view as user" now hides it too — the point of the mode is a dashboard that looks exactly like the user's. The ways out stay on raw impersonation: "Stop impersonating" and the toggle itself in the account menu, the Cmd+Opt+A shortcut, and the routes that clear the impersonation cookie. The write suppressions (side-menu preferences, favorites) also stay raw — they keep an admin from writing into the impersonated account and are not display chrome. Two more review findings, same area: - The already-impersonating branch of the impersonation deep link kept only the pathname, so an admin who was already impersonating and opened a run link lost the ?span= selection and the follow-up GET built its destination and post-back paths from an empty search. Keep the search. - hasAdminDisplayAccess is display-only, but it had reached two gates that decide what a request may do: the default-region service call and the query page's EXPLAIN flag. With the toggle on, an admin's own form submissions were rejected. Both are back on raw user.admin || user.isImpersonating, and the helper now states the rule: the toggle changes what is shown, never what is permitted. Every loader and rendering site stays on the helper. Co-Authored-By: Claude --- .../impersonation-consent-and-view-as-user.md | 2 +- apps/webapp/app/components/navigation/SideMenu.tsx | 9 ++++++++- .../app/routes/_app.@.orgs.$organizationSlug.$.tsx | 5 ++++- .../route.tsx | 7 ++++--- .../route.tsx | 9 ++++----- apps/webapp/app/services/session.server.ts | 6 ++++++ 6 files changed, 27 insertions(+), 11 deletions(-) diff --git a/.server-changes/impersonation-consent-and-view-as-user.md b/.server-changes/impersonation-consent-and-view-as-user.md index db78d9c375e..232e97e8b90 100644 --- a/.server-changes/impersonation-consent-and-view-as-user.md +++ b/.server-changes/impersonation-consent-and-view-as-user.md @@ -3,4 +3,4 @@ area: webapp type: improvement --- -Admins opening an impersonation link from outside the dashboard now get a confirmation page naming the organization and destination instead of being bounced back, and while impersonating they can switch to "View as user" to see the dashboard without any admin-only UI. +Admins opening an impersonation link from outside the dashboard now get a confirmation page naming the organization and destination instead of being bounced back, and while impersonating they can switch to "View as user" to see the dashboard exactly as that user sees it, with the admin-only UI and the impersonation highlight both hidden. Stopping impersonation is still one click away in the account menu. diff --git a/apps/webapp/app/components/navigation/SideMenu.tsx b/apps/webapp/app/components/navigation/SideMenu.tsx index 625ee3d74e9..ebedc77fc6e 100644 --- a/apps/webapp/app/components/navigation/SideMenu.tsx +++ b/apps/webapp/app/components/navigation/SideMenu.tsx @@ -398,6 +398,7 @@ export function SideMenu({ const { isConnected } = useDevPresence(); const isFreeUser = currentPlan?.v3Subscription?.isPaying === false; const isAdmin = useHasAdminAccess(); + const isViewingAsUser = useIsViewingAsUser(); const { isManagedCloud } = useFeatures(); const featureFlags = useFeatureFlags(); const incidentStatus = useIncidentStatus(); @@ -1057,7 +1058,13 @@ export function SideMenu({ style={initialStyleRef.current} className={cn( "relative h-full border-r bg-background-bright", - user.isImpersonating ? IMPERSONATION_ACCENT.border : "border-grid-bright" + // The accent is the loudest "you are not this user" tell, so "view as user" drops it too — + // the point of the mode is a dashboard that looks exactly like the user's. The account + // menu's "Stop impersonating" and the toggle itself stay on raw impersonation, so there is + // still a way back out (as does the ⌘⌥A shortcut in ). + user.isImpersonating && !isViewingAsUser + ? IMPERSONATION_ACCENT.border + : "border-grid-bright" )} > ` links redirect here carrying `?span=`, and the + // follow-up GET builds the destination and post-back paths from it. Dropping it would land the + // admin on the run with no span selected. + throw await clearImpersonation(request, `${url.pathname}${url.search}`); } // Only admins can impersonate diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx index 6aecd22be81..9aec240cb9d 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/route.tsx @@ -175,9 +175,10 @@ export const action = async ({ request, params }: ActionFunctionArgs) => { } const { query, scope, explain: explainParam, period, from, to } = parsed.data; - // Only allow explain for admins/impersonating users - const isAdmin = hasAdminDisplayAccess(user); - const explain = explainParam === "true" && isAdmin; + // Only allow explain for admins/impersonating users. Raw impersonation, not + // `hasAdminDisplayAccess`: this decides what the request may run, and "view as user" only changes + // what is shown — the loader is what hides the EXPLAIN control. + const explain = explainParam === "true" && (user.admin || user.isImpersonating); try { const queryResult = await executeQuery({ diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx index 7059ff6f9c9..f5b4e4901db 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.regions/route.tsx @@ -54,7 +54,6 @@ import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/m import { resolveOrgIdFromSlug } from "~/models/organization.server"; import { findProjectBySlug } from "~/models/project.server"; import { type Region, RegionsPresenter } from "~/presenters/v3/RegionsPresenter.server"; -import { getViewingAsUser } from "~/services/impersonation.server"; import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder"; import { @@ -136,10 +135,10 @@ export const action = dashboardAction( service.call({ projectId: project.id, regionId: parsedFormData.data.regionId, - isAdmin: hasAdminDisplayAccess({ - ...user, - isViewingAsUser: await getViewingAsUser(request), - }), + // Raw impersonation, not `hasAdminDisplayAccess`: this decides whether a restricted or + // hidden region may be set as the default, which is a capability. "View as user" only + // changes what is shown. + isAdmin: user.admin || user.isImpersonating, }) ); diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts index 1aa90b76a5f..74293e2c676 100644 --- a/apps/webapp/app/services/session.server.ts +++ b/apps/webapp/app/services/session.server.ts @@ -160,6 +160,12 @@ export async function requireUser(request: Request) { * Display only. The "view as user" toggle is cosmetic and must never widen or * narrow a real security boundary — authorization stays on `user.admin`, the * route builder's `authorization` block and the per-feature access checks. + * + * The rule: the toggle changes what is *shown*, never what is *permitted*. So + * call this from loaders and rendering only. Anything deciding what a request + * may do — an action, a service call that enforces something — stays on raw + * `user.admin || user.isImpersonating`, or the admin's own form submissions + * start failing the moment they flip the toggle on. */ export function hasAdminDisplayAccess(user: { admin: boolean; From 6807b682ca8e9443558f06b1646abdc6ebdd1da5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 08:58:40 +0000 Subject: [PATCH 6/8] fix(webapp): clear impersonation before the consent action's admin check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/webapp/app/root.tsx | 10 ++- .../_app.@.orgs.$organizationSlug.$.tsx | 16 +++++ ...ectParam.env.$envParam.runs.bulkaction.tsx | 12 ++-- .../app/services/impersonation.server.ts | 24 +++++-- apps/webapp/app/services/session.server.ts | 9 +-- .../app/utils/impersonationState.test.ts | 71 +++++++++++++++++++ apps/webapp/app/utils/impersonationState.ts | 47 ++++++++++++ apps/webapp/test/viewAsUser.test.ts | 34 +++++++-- 8 files changed, 197 insertions(+), 26 deletions(-) create mode 100644 apps/webapp/app/utils/impersonationState.test.ts create mode 100644 apps/webapp/app/utils/impersonationState.ts diff --git a/apps/webapp/app/root.tsx b/apps/webapp/app/root.tsx index 100d7d0c9a6..f6c403b8c11 100644 --- a/apps/webapp/app/root.tsx +++ b/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)); diff --git a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx index 1c48da2cb04..2923a6fdeeb 100644 --- a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx +++ b/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("/"); } diff --git a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx index 9bada394f26..a9ca662248d 100644 --- a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx +++ b/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([ diff --git a/apps/webapp/app/services/impersonation.server.ts b/apps/webapp/app/services/impersonation.server.ts index dd5127c65dd..e69a3fb305b 100644 --- a/apps/webapp/app/services/impersonation.server.ts +++ b/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 { 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) { diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts index 74293e2c676..b6101684720 100644 --- a/apps/webapp/app/services/session.server.ts +++ b/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, }; } diff --git a/apps/webapp/app/utils/impersonationState.test.ts b/apps/webapp/app/utils/impersonationState.test.ts new file mode 100644 index 00000000000..b6fe24bf7e0 --- /dev/null +++ b/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); + }); +}); diff --git a/apps/webapp/app/utils/impersonationState.ts b/apps/webapp/app/utils/impersonationState.ts new file mode 100644 index 00000000000..2eca8efefe9 --- /dev/null +++ b/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, + }; +} diff --git a/apps/webapp/test/viewAsUser.test.ts b/apps/webapp/test/viewAsUser.test.ts index 299aaf49449..d21d53f6bc6 100644 --- a/apps/webapp/test/viewAsUser.test.ts +++ b/apps/webapp/test/viewAsUser.test.ts @@ -3,7 +3,7 @@ import { clearImpersonationId, commitImpersonationSession, getImpersonationId, - getViewingAsUser, + getImpersonationState, setImpersonationId, setViewingAsUser, } from "~/services/impersonation.server"; @@ -19,11 +19,17 @@ 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 () => { @@ -31,15 +37,15 @@ describe("view as user flag", () => { 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,7 +56,7 @@ 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 () => { @@ -58,7 +64,21 @@ describe("view as user flag", () => { // 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); }); }); From 60a20ddd3ceb2e579ac602086dcbed90b8e1db76 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 09:31:03 +0000 Subject: [PATCH 7/8] fix(webapp): keep the region pickers on the raw admin check, not the display one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four of the hasAdminDisplayAccess sites feed a region picker that submits, and an option list a form posts from is not display. Shrinking it changes what the submitted form is able to do, so an admin who flipped "view as user" on quietly lost the ability to send a run to a hidden or restricted region from the replay dialog, the test form, the playground and the bulk-action override. All four are back on raw user.admin || user.isImpersonating. The replay dialog is the only one where the list and the current value can disagree, because the run's own region is returned separately from the list. It degrades better than it looks: the Select's hidden native select synthesises an option for a value that is not among the registered items, so an out-of-list region is displayed and posted unchanged rather than falling back to the first option — and when the list drops to a single entry the field is not rendered at all, at which point the replay service falls back to the run's own region. Nothing was silently re-routed; the cost was the narrowed choice. The regions page listing stays on hasAdminDisplayAccess. It is a read-only table, and each row's "set as default" form posts that row's own id, so a shorter list can only offer fewer rows, never a wrong value — and the action behind it already re-checks raw impersonation. The helper's comment now draws the sharper line, since "display" was reading as broader than intended: display is rendering and read-only listings, while anything a request handler reads, or that feeds a control which submits, stays raw. The run-GET guard test's session mock drops the hasAdminDisplayAccess stub it needed only for the replay loader. No module in that test's graph reaches the export now, and the file still fails only on the container runtime it cannot reach in a sandbox, with no missing-export error. Co-Authored-By: Claude --- .../route.tsx | 7 +++++-- .../route.tsx | 7 +++++-- ...ojectParam.env.$envParam.runs.bulkaction.tsx | 13 +++++-------- .../resources.taskruns.$runParam.replay.ts | 9 +++++++-- apps/webapp/app/services/session.server.ts | 17 ++++++++++++----- .../test/runGetRoutes.replicaLag.guard.test.ts | 14 ++++---------- 6 files changed, 38 insertions(+), 29 deletions(-) diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.playground/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.playground/route.tsx index 95f7b0ec946..ddb9f2a63eb 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.playground/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.playground/route.tsx @@ -22,7 +22,7 @@ import { findProjectBySlug } from "~/models/project.server"; import { findEnvironmentBySlug } from "~/models/runtimeEnvironment.server"; import { playgroundPresenter } from "~/presenters/v3/PlaygroundPresenter.server"; import { RegionsPresenter } from "~/presenters/v3/RegionsPresenter.server"; -import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; +import { requireUser } from "~/services/session.server"; import { docsPath, EnvironmentParamSchema, v3PlaygroundAgentPath } from "~/utils/pathBuilder"; export const meta: MetaFunction = () => { @@ -54,10 +54,13 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { orderBy: { createdAt: "desc" }, take: 20, }), + // Raw impersonation, not `hasAdminDisplayAccess`: this list is the + // playground's region picker, so it decides which region a submitted run + // can be sent to. "View as user" only changes what is shown. new RegionsPresenter().call({ userId: user.id, projectSlug: projectParam, - isAdmin: hasAdminDisplayAccess(user), + isAdmin: user.admin || user.isImpersonating, }), ]); diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx index fc8ed9fb2d1..ffe6dbcd038 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx @@ -67,7 +67,7 @@ import { import { type loader as queuesLoader } from "~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.queues"; import { clickhouseFactory } from "~/services/clickhouse/clickhouseFactoryInstance.server"; import { logger } from "~/services/logger.server"; -import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; +import { requireUser } from "~/services/session.server"; import { cn } from "~/utils/cn"; import { docsPath, v3RunSpanPath, v3TaskParamsSchema, v3TestPath } from "~/utils/pathBuilder"; import { DeleteTaskRunTemplateService } from "~/v3/services/deleteTaskRunTemplate.server"; @@ -117,10 +117,13 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { taskIdentifier: taskParam, environment: environment, }), + // Raw impersonation, not `hasAdminDisplayAccess`: this list is the test + // form's region picker, so it decides which region a submitted test run + // can be sent to. "View as user" only changes what is shown. new RegionsPresenter().call({ userId: user.id, projectSlug: projectParam, - isAdmin: hasAdminDisplayAccess(user), + isAdmin: user.admin || user.isImpersonating, }), ]); diff --git a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx index a9ca662248d..5bef068b994 100644 --- a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx +++ b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsx @@ -56,9 +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 { 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"; import { cn } from "~/utils/cn"; import { EnvironmentParamSchema, v3BulkActionPath } from "~/utils/pathBuilder"; @@ -86,12 +84,11 @@ export const loader = dashboardLoader( throw new Response("Not Found", { status: 404 }); } - // Display only: hidden regions stay listed for admins, unless they've asked - // 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 }); + // Raw impersonation, not `hasAdminDisplayAccess`: this list is the bulk + // action's "override region" picker, so it decides which region a submitted + // bulk replay can re-route runs to. "View as user" only changes what is + // shown. + const isAdmin = user.admin || user.isImpersonating; const presenter = new CreateBulkActionPresenter(); const [data, regionsResult] = await Promise.all([ diff --git a/apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts b/apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts index f113824075d..e91ac2c5451 100644 --- a/apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts +++ b/apps/webapp/app/routes/resources.taskruns.$runParam.replay.ts @@ -8,7 +8,7 @@ import { $replica, prisma } from "~/db.server"; import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/message.server"; import { displayableEnvironment } from "~/models/runtimeEnvironment.server"; import { logger } from "~/services/logger.server"; -import { hasAdminDisplayAccess, requireUser } from "~/services/session.server"; +import { requireUser } from "~/services/session.server"; import { dashboardAction } from "~/services/routeBuilders/dashboardBuilder"; import { sortEnvironments } from "~/utils/environmentSort"; import { v3RunSpanPath } from "~/utils/pathBuilder"; @@ -166,10 +166,15 @@ export async function loader({ request, params }: LoaderFunctionArgs) { const [payload, regionsResult] = await Promise.all([ prettyPrintPacket(run.payload, run.payloadType), + // Raw impersonation, not `hasAdminDisplayAccess`: this list is the replay + // dialog's region picker, so it decides what the submitted form can select + // — and the run's own region is returned separately, so dropping entries + // can leave the current value off the list. "View as user" only changes + // what is shown. new RegionsPresenter().call({ userId, projectSlug, - isAdmin: hasAdminDisplayAccess(user), + isAdmin: user.admin || user.isImpersonating, }), ]); diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts index b6101684720..753bc7f6a17 100644 --- a/apps/webapp/app/services/session.server.ts +++ b/apps/webapp/app/services/session.server.ts @@ -162,11 +162,18 @@ export async function requireUser(request: Request) { * narrow a real security boundary — authorization stays on `user.admin`, the * route builder's `authorization` block and the per-feature access checks. * - * The rule: the toggle changes what is *shown*, never what is *permitted*. So - * call this from loaders and rendering only. Anything deciding what a request - * may do — an action, a service call that enforces something — stays on raw - * `user.admin || user.isImpersonating`, or the admin's own form submissions - * start failing the moment they flip the toggle on. + * The rule: the toggle changes what is *shown*, never what is *permitted or + * what happens*. "Shown" is narrow — rendering and read-only listings. A badge, + * a debug tooltip, an extra table column, a control that is merely hidden while + * the handler behind it re-checks the raw flags: all fine. + * + * It does NOT extend to a value a request handler reads, nor to the option set + * of a control that submits. An option list feeding a mutation is not display: + * shrinking it changes what a submitted form is able to do, and where the + * current value is not among the remaining options it can change which value + * the form carries. Those stay on raw `user.admin || user.isImpersonating`, or + * the admin's own submissions start behaving differently — or failing — the + * moment they flip the toggle on. */ export function hasAdminDisplayAccess(user: { admin: boolean; diff --git a/apps/webapp/test/runGetRoutes.replicaLag.guard.test.ts b/apps/webapp/test/runGetRoutes.replicaLag.guard.test.ts index 02c1e19f3ab..f6597980f39 100644 --- a/apps/webapp/test/runGetRoutes.replicaLag.guard.test.ts +++ b/apps/webapp/test/runGetRoutes.replicaLag.guard.test.ts @@ -109,20 +109,14 @@ vi.mock("~/v3/runStore.server", () => ({ ), })); -// Auth/session (orthogonal): fixed user id. `hasAdminDisplayAccess` mirrors the real predicate -// rather than importing it: a factory mock replaces the whole module, and pulling the original in -// would evaluate session.server's server-only import graph, which this test deliberately keeps out. -// Only the replay loader's region-picker flag reads it, and RegionsPresenter is mocked below, so the -// value is orthogonal to every proof here — it just has to exist, or the mock throws on access. +// Auth/session (orthogonal): fixed user id. This factory replaces the whole module, so it has to +// return every export the route graph under test actually reaches — accessing one it does not +// define throws. Keep it to that set; importing the original would evaluate session.server's +// server-only import graph, which this test deliberately keeps out. vi.mock("~/services/session.server", () => ({ requireUserId: async () => authUser.id, requireUser: async () => authUser, getUserId: async () => authUser.id, - hasAdminDisplayAccess: (user: { - admin: boolean; - isImpersonating: boolean; - isViewingAsUser: boolean; - }) => (user.admin || user.isImpersonating) && !user.isViewingAsUser, })); // Control-plane env resolution (a downstream cross-DB lookup, orthogonal to the run-store read): From 3a07360ed63b1069f7f20a2f53bceb8cba13fe95 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 09:47:39 +0000 Subject: [PATCH 8/8] chore(webapp): delete unused ImpersonationBanner component It was rendered nowhere; the live impersonation affordances live in the side menu's account popover. Co-Authored-By: Claude --- .../app/components/ImpersonationBanner.tsx | 30 ------------------- 1 file changed, 30 deletions(-) delete mode 100644 apps/webapp/app/components/ImpersonationBanner.tsx diff --git a/apps/webapp/app/components/ImpersonationBanner.tsx b/apps/webapp/app/components/ImpersonationBanner.tsx deleted file mode 100644 index c16e822df1e..00000000000 --- a/apps/webapp/app/components/ImpersonationBanner.tsx +++ /dev/null @@ -1,30 +0,0 @@ -import { Form } from "@remix-run/react"; -import { UserCrossIcon } from "~/assets/icons/UserCrossIcon"; -import { Button } from "./primitives/Buttons"; -import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from "./primitives/Tooltip"; - -export function ImpersonationBanner() { - return ( -
- - - - -
- ); -}