diff --git a/apps/webapp/app/models/admin.server.ts b/apps/webapp/app/models/admin.server.ts index e93844dbaed..9ea767db538 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 { $replica, prisma, type PrismaClientOrTransaction } from "~/db.server"; +import { $replica, $transaction, prisma, type PrismaClientOrTransaction } from "~/db.server"; import { logger } from "~/services/logger.server"; import type { SearchParams } from "~/routes/admin._index"; import { @@ -9,7 +9,7 @@ import { setImpersonationId, } from "~/services/impersonation.server"; import { authenticator } from "~/services/auth.server"; -import { requireUser } from "~/services/session.server"; +import { getRealUser } from "~/services/session.server"; import { extractClientIp } from "~/utils/extractClientIp.server"; import { impersonationDestinationPath } from "~/utils/pathBuilder"; @@ -210,35 +210,76 @@ export async function adminGetOrganizations(userId: string, { page, search }: Se }; } +/** + * Starts (or switches) impersonation. + * + * The admin gate resolves the *real* authenticated user itself. `requireUser` returns the + * impersonation target while impersonating, so callers that gated on it refused an admin who was + * already impersonating someone — they had to stop first — and would have attributed the audit row + * to the target rather than the admin. + * + * `verifiedAdmin` exists only so tests can supply an admin without a session cookie. Production + * callers must not pass it: passing a `requireUser` result is exactly the bug described above. + */ export async function redirectWithImpersonation( request: Request, userId: string, path: string, - currentUser?: { id: string; admin: boolean }, + verifiedAdmin?: { id: string; admin: boolean }, prismaClient: PrismaClientOrTransaction = prisma ) { - const user = currentUser ?? (await requireUser(request)); - if (!user.admin) { + const admin = verifiedAdmin ?? (await getRealUser(request, prismaClient)); + if (!admin?.admin) { throw new Error("Unauthorized"); } const xff = request.headers.get("x-forwarded-for"); const ipAddress = extractClientIp(xff); + const previousTargetId = await getImpersonationId(request); + + // Switching straight from one target to another never passes through `clearImpersonation`, so the + // previous session is closed here, or the trail shows two overlapping STARTs. + // + // Both rows are written in one transaction: as separate statements, a failure between them could + // start an impersonation whose only audit row is the STOP for the previous target — an admin + // acting as someone with no record of it. + // + // `createdAt` is stamped explicitly rather than left to `@default(now())`, because Postgres `now()` + // is the *transaction* timestamp: inside one transaction both rows would take the same value, and + // an audit view ordered by that column couldn't tell which came first. + const startedAt = new Date(); + const closedAt = new Date(startedAt.getTime() - 1); try { - await prismaClient.impersonationAuditLog.create({ - data: { - action: "START", - adminId: user.id, - targetId: userId, - ipAddress, - }, + await $transaction(prismaClient, "startImpersonationAudit", async (tx) => { + if (previousTargetId && previousTargetId !== userId) { + await tx.impersonationAuditLog.create({ + data: { + action: "STOP", + adminId: admin.id, + targetId: previousTargetId, + ipAddress, + createdAt: closedAt, + }, + }); + } + + await tx.impersonationAuditLog.create({ + data: { + action: "START", + adminId: admin.id, + targetId: userId, + ipAddress, + createdAt: startedAt, + }, + }); }); } catch (error) { logger.error("Failed to create impersonation audit log", { error, - adminId: user.id, + adminId: admin.id, targetId: userId, + previousTargetId, }); } @@ -308,7 +349,8 @@ export async function startImpersonation( request: Request, organizationSlug: string, path: string, - currentUser: { id: string; admin: boolean }, + // Test-only, forwarded to `redirectWithImpersonation` — see its docstring. + verifiedAdmin?: { id: string; admin: boolean }, clients: { read: PrismaClientOrTransaction; write: PrismaClientOrTransaction } = { read: $replica, write: prisma, @@ -325,7 +367,7 @@ export async function startImpersonation( request, target.userId, impersonationDestinationPath(organizationSlug, path, new URL(request.url).search), - currentUser, + verifiedAdmin, clients.write ); } diff --git a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx index 2923a6fdeeb..5cecc06e56d 100644 --- a/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx +++ b/apps/webapp/app/routes/_app.@.orgs.$organizationSlug.$.tsx @@ -61,7 +61,7 @@ export async function loader({ request, params }: LoaderFunctionArgs) { // the consent page below instead, whose "Impersonate" button posts back from // our own page and so satisfies the same check. if (isSameOriginNavigation(request, env.LOGIN_ORIGIN)) { - throw await startImpersonation(request, organizationSlug, path, user); + throw await startImpersonation(request, organizationSlug, path); } // Expected for any link opened outside the app (address bar, bookmark, a link @@ -148,7 +148,7 @@ export async function action({ request, params }: ActionFunctionArgs) { // The consent form posts to an explicit absolute path (see // `impersonationConsentPostBackPath`), so the organization slug, the splat // path and the query string all arrive here intact. - return startImpersonation(request, organizationSlug, params["*"] ?? "", user); + return startImpersonation(request, organizationSlug, params["*"] ?? ""); } export default function Page() { diff --git a/apps/webapp/app/routes/admin.impersonate.tsx b/apps/webapp/app/routes/admin.impersonate.tsx deleted file mode 100644 index 458ed5b2a7e..00000000000 --- a/apps/webapp/app/routes/admin.impersonate.tsx +++ /dev/null @@ -1,61 +0,0 @@ -import { - redirect, - type ActionFunctionArgs, - type LoaderFunctionArgs, -} from "@remix-run/server-runtime"; -import { z } from "zod"; -import { redirectWithImpersonation } from "~/models/admin.server"; -import { requireUser } from "~/services/session.server"; -import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server"; -import { logger } from "~/services/logger.server"; - -const FormSchema = z.object({ id: z.string() }); - -async function handleImpersonationRequest(request: Request, userId: string): Promise { - const user = await requireUser(request); - if (!user.admin) { - return redirect("/"); - } - return redirectWithImpersonation(request, userId, "/", user); -} - -export const loader = async ({ request }: LoaderFunctionArgs) => { - const url = new URL(request.url); - const impersonateUserId = url.searchParams.get("impersonate"); - const impersonationToken = url.searchParams.get("impersonationToken"); - - if (!impersonateUserId) { - return redirect("/admin"); - } - - if (!impersonationToken) { - logger.warn("Impersonation request missing token"); - return redirect("/"); - } - - // Check admin BEFORE consuming the one-time token - const user = await requireUser(request); - if (!user.admin) { - return redirect("/"); - } - - const validatedUserId = await validateAndConsumeImpersonationToken(impersonationToken); - - if (!validatedUserId || validatedUserId !== impersonateUserId) { - logger.warn("Invalid or expired impersonation token"); - return redirect("/"); - } - - return redirectWithImpersonation(request, impersonateUserId, "/", user); -}; - -export async function action({ request }: ActionFunctionArgs) { - if (request.method.toLowerCase() !== "post") { - return new Response("Method not allowed", { status: 405 }); - } - - const payload = Object.fromEntries(await request.formData()); - const { id } = FormSchema.parse(payload); - - return handleImpersonationRequest(request, id); -} diff --git a/apps/webapp/app/routes/admin_.impersonate.tsx b/apps/webapp/app/routes/admin_.impersonate.tsx new file mode 100644 index 00000000000..f55d258a9f6 --- /dev/null +++ b/apps/webapp/app/routes/admin_.impersonate.tsx @@ -0,0 +1,110 @@ +import { + redirect, + type ActionFunctionArgs, + type LoaderFunctionArgs, +} from "@remix-run/server-runtime"; +import { z } from "zod"; +import { redirectWithImpersonation } from "~/models/admin.server"; +import { authenticator } from "~/services/auth.server"; +import { rbac } from "~/services/rbac.server"; +import { getRealUser } from "~/services/session.server"; +import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server"; +import { logger } from "~/services/logger.server"; +import { sanitizeRedirectPath } from "~/utils"; + +/** + * Served at `/admin/impersonate`, but the trailing `_` on `admin_` keeps it out of the `admin.tsx` + * layout on purpose. + * + * That layout's loader is `dashboardLoader({ authorization: { requireSuper: true } })`, which + * resolves the user through `getUserId` — the impersonated id while impersonating. So starting on a + * second target ran the parent gate against the target, which isn't a super admin, and it answered + * with its own `redirect("/")`. Nesting would leave this route's behaviour depending on the router + * preferring the deepest redirect; opting out removes the question. Nothing is lost — this route + * only ever redirects, so it never rendered inside the layout anyway. + */ + +const FormSchema = z.object({ id: z.string() }); + +/** + * The real authenticated user, or null when they're signed in but not an admin. + * + * Must not use `requireUser`: while impersonating it resolves to the impersonation target, whose + * `admin` is false, so an admin switching to a second target was bounced to `/` and left on the + * first one. + * + * Throws a login redirect when nobody is signed in, keeping this URL as `redirectTo` so the + * impersonation survives the round trip — the one-time token is validated after this gate, so it's + * still unconsumed when the browser comes back. Collapsing that into the non-admin `/` redirect + * would drop the link the agent clicked. + */ +async function requireRealAdmin(request: Request) { + if (!(await authenticator.isAuthenticated(request))) { + const url = new URL(request.url); + const redirectTo = sanitizeRedirectPath(`${url.pathname}${url.search}`); + throw redirect(`/login?${new URLSearchParams([["redirectTo", redirectTo]])}`); + } + + const admin = await getRealUser(request); + if (!admin) return null; + + // Same gate `dashboardLoader({ authorization: { requireSuper: true } })` applies, evaluated + // against the real admin. It can't be reached through the builder here, because the builder + // resolves its subject with `getUserId` — the impersonated id while impersonating, which is the + // bug this route exists to fix. So the ability is built explicitly for `admin.id` instead of + // trusting the raw `User.admin` column: `canSuper()` is only equal to that column in the OSS + // fallback, and a plugin is free to be stricter. requireSuper needs no org/project scope. + const auth = await rbac.authenticateSession(request, { userId: admin.id }); + if (!auth.ok || !auth.ability.canSuper()) return null; + + return admin; +} + +async function handleImpersonationRequest(request: Request, userId: string): Promise { + const admin = await requireRealAdmin(request); + if (!admin) { + return redirect("/"); + } + return redirectWithImpersonation(request, userId, "/"); +} + +export const loader = async ({ request }: LoaderFunctionArgs) => { + const url = new URL(request.url); + const impersonateUserId = url.searchParams.get("impersonate"); + const impersonationToken = url.searchParams.get("impersonationToken"); + + if (!impersonateUserId) { + return redirect("/admin"); + } + + if (!impersonationToken) { + logger.warn("Impersonation request missing token"); + return redirect("/"); + } + + // Check admin BEFORE consuming the one-time token, so a rejected request leaves the token usable. + const admin = await requireRealAdmin(request); + if (!admin) { + return redirect("/"); + } + + const validatedUserId = await validateAndConsumeImpersonationToken(impersonationToken); + + if (!validatedUserId || validatedUserId !== impersonateUserId) { + logger.warn("Invalid or expired impersonation token"); + return redirect("/"); + } + + return redirectWithImpersonation(request, impersonateUserId, "/"); +}; + +export async function action({ request }: ActionFunctionArgs) { + if (request.method.toLowerCase() !== "post") { + return new Response("Method not allowed", { status: 405 }); + } + + const payload = Object.fromEntries(await request.formData()); + const { id } = FormSchema.parse(payload); + + return handleImpersonationRequest(request, id); +} diff --git a/apps/webapp/app/services/impersonation.server.ts b/apps/webapp/app/services/impersonation.server.ts index e69a3fb305b..7d93390f704 100644 --- a/apps/webapp/app/services/impersonation.server.ts +++ b/apps/webapp/app/services/impersonation.server.ts @@ -45,6 +45,14 @@ export async function getImpersonationId(request: Request) { export async function setImpersonationId(userId: string, request: Request) { const session = await getImpersonationSession(request); + // Switching straight to a different target begins a new impersonation session, so the view-as-user + // flag must not carry over from the previous one — it's scoped to a single impersonation, which is + // why `clearImpersonationId` drops it too. Reachable only since switching stopped requiring a stop + // first; before that, every second target arrived via `clearImpersonationId`. + if (session.get(IMPERSONATED_USER_ID_KEY) !== userId) { + session.unset(VIEWING_AS_USER_KEY); + } + session.set(IMPERSONATED_USER_ID_KEY, userId); return session; diff --git a/apps/webapp/app/services/session.server.ts b/apps/webapp/app/services/session.server.ts index 753bc7f6a17..a34c346d433 100644 --- a/apps/webapp/app/services/session.server.ts +++ b/apps/webapp/app/services/session.server.ts @@ -1,4 +1,5 @@ import { redirect } from "@remix-run/node"; +import { prisma, type PrismaClientOrTransaction } from "~/db.server"; import { getUserById } from "~/models/user.server"; import { sanitizeRedirectPath } from "~/utils"; import { extractClientIp } from "~/utils/extractClientIp.server"; @@ -124,6 +125,44 @@ export async function requireUserId(request: Request, redirectTo?: string) { return userId; } +/** + * The user the request actually authenticated as, ignoring any impersonation cookie. + * + * `getUserId` deliberately resolves to the *impersonated* id while impersonating, so `getUser` / + * `requireUser` answer "who is this request acting as". That is the wrong question for anything + * gating on admin rights or attributing an admin action: while impersonating a customer, + * `requireUser().admin` is that customer's flag, so an admin check silently fails and an audit + * record would name the customer as the actor. + * + * Returns null when unauthenticated or the row is gone. + */ +export async function getRealUser( + request: Request, + prismaClient: PrismaClientOrTransaction = prisma +) { + const authUser = await authenticator.isAuthenticated(request); + + // Apply the same session controls `getUserId`/`getUser` apply to the real user, so this helper + // can't become a way around them: a session the IdP has revoked throws to /logout here, and one + // past its effective duration is caught below. Skipping either would let an admin whose session + // should have ended still start impersonation. + await revalidateSsoSession(request, authUser); + if (!authUser?.userId) return null; + + // Narrow select — callers need the id and the admin flag, plus `nextSessionEnd` for the deadline + // check. Takes a client so a caller already scoped to one reads the admin from the same database + // it writes to. + const user = await prismaClient.user.findFirst({ + where: { id: authUser.userId }, + select: { id: true, admin: true, nextSessionEnd: true }, + }); + if (!user) return null; + + maybeAutoLogout(request, user); + + return user; +} + export type UserFromSession = Awaited>; export async function requireUser(request: Request) {