Skip to content

Commit 4aadcd5

Browse files
committed
fix(webapp): keep the impersonation link through login, order audit rows
Signing in from an impersonation link dropped the destination: the admin gate answered both "not signed in" and "signed in but not an admin" with a redirect to /, so an agent who clicked Impersonate while logged out landed on the dashboard afterwards. Unauthenticated requests now redirect to login carrying the original URL as redirectTo — the one-time token is validated after the gate, so it survives the round trip. The paired STOP/START audit rows were written with createMany, a single insert, so both took the same createdAt and a view ordered by that column could show the new START ahead of the STOP closing the previous session. They are now two statements so the sequence is unambiguous.
1 parent 3c90ce3 commit 4aadcd5

2 files changed

Lines changed: 36 additions & 19 deletions

File tree

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

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -238,27 +238,31 @@ export async function redirectWithImpersonation(
238238
const previousTargetId = await getImpersonationId(request);
239239

240240
try {
241-
await prismaClient.impersonationAuditLog.createMany({
242-
data: [
243-
// Switching straight from one target to another never passes through `clearImpersonation`,
244-
// so close the previous session here or the trail shows two overlapping STARTs.
245-
...(previousTargetId && previousTargetId !== userId
246-
? [
247-
{
248-
action: "STOP" as const,
249-
adminId: admin.id,
250-
targetId: previousTargetId,
251-
ipAddress,
252-
},
253-
]
254-
: []),
255-
{
256-
action: "START" as const,
241+
// Switching straight from one target to another never passes through `clearImpersonation`, so
242+
// close the previous session here or the trail shows two overlapping STARTs.
243+
//
244+
// Two statements rather than one `createMany`: `createdAt` defaults to `now()`, which is fixed
245+
// for the duration of a statement, so a single insert would stamp both rows identically and an
246+
// audit view ordered by `createdAt` couldn't tell which came first — the very ambiguity the
247+
// STOP row exists to remove.
248+
if (previousTargetId && previousTargetId !== userId) {
249+
await prismaClient.impersonationAuditLog.create({
250+
data: {
251+
action: "STOP",
257252
adminId: admin.id,
258-
targetId: userId,
253+
targetId: previousTargetId,
259254
ipAddress,
260255
},
261-
],
256+
});
257+
}
258+
259+
await prismaClient.impersonationAuditLog.create({
260+
data: {
261+
action: "START",
262+
adminId: admin.id,
263+
targetId: userId,
264+
ipAddress,
265+
},
262266
});
263267
} catch (error) {
264268
logger.error("Failed to create impersonation audit log", {

apps/webapp/app/routes/admin.impersonate.tsx

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,33 @@ import {
55
} from "@remix-run/server-runtime";
66
import { z } from "zod";
77
import { redirectWithImpersonation } from "~/models/admin.server";
8+
import { authenticator } from "~/services/auth.server";
89
import { getRealUser } from "~/services/session.server";
910
import { validateAndConsumeImpersonationToken } from "~/services/impersonation.server";
1011
import { logger } from "~/services/logger.server";
12+
import { sanitizeRedirectPath } from "~/utils";
1113

1214
const FormSchema = z.object({ id: z.string() });
1315

1416
/**
15-
* The real authenticated user, or null when they aren't an admin.
17+
* The real authenticated user, or null when they're signed in but not an admin.
1618
*
1719
* Must not use `requireUser`: while impersonating it resolves to the impersonation target, whose
1820
* `admin` is false, so an admin switching to a second target was bounced to `/` and left on the
1921
* first one.
22+
*
23+
* Throws a login redirect when nobody is signed in, keeping this URL as `redirectTo` so the
24+
* impersonation survives the round trip — the one-time token is validated after this gate, so it's
25+
* still unconsumed when the browser comes back. Collapsing that into the non-admin `/` redirect
26+
* would drop the link the agent clicked.
2027
*/
2128
async function requireRealAdmin(request: Request) {
29+
if (!(await authenticator.isAuthenticated(request))) {
30+
const url = new URL(request.url);
31+
const redirectTo = sanitizeRedirectPath(`${url.pathname}${url.search}`);
32+
throw redirect(`/login?${new URLSearchParams([["redirectTo", redirectTo]])}`);
33+
}
34+
2235
const admin = await getRealUser(request);
2336
return admin?.admin ? admin : null;
2437
}

0 commit comments

Comments
 (0)