Skip to content

Commit f1c6eb3

Browse files
committed
fix(webapp): write the impersonation audit pair atomically
Closing the previous impersonation and opening the new one were two separate statements sharing one error handler, so a failure between them could start an impersonation whose only record was the STOP for the previous target — an admin acting as someone with no trace of it. Both rows now go through the $transaction helper. createdAt is stamped explicitly because Postgres now() is the transaction timestamp: inside one transaction the default would give both rows the same value, leaving an audit view ordered by that column unable to tell which came first.
1 parent 4aadcd5 commit f1c6eb3

1 file changed

Lines changed: 31 additions & 21 deletions

File tree

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

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { redirect } from "@remix-run/server-runtime";
2-
import { $replica, prisma, type PrismaClientOrTransaction } from "~/db.server";
2+
import { $replica, $transaction, prisma, type PrismaClientOrTransaction } from "~/db.server";
33
import { logger } from "~/services/logger.server";
44
import type { SearchParams } from "~/routes/admin._index";
55
import {
@@ -237,32 +237,42 @@ export async function redirectWithImpersonation(
237237
const ipAddress = extractClientIp(xff);
238238
const previousTargetId = await getImpersonationId(request);
239239

240+
// Switching straight from one target to another never passes through `clearImpersonation`, so the
241+
// previous session is closed here, or the trail shows two overlapping STARTs.
242+
//
243+
// Both rows are written in one transaction: as separate statements, a failure between them could
244+
// start an impersonation whose only audit row is the STOP for the previous target — an admin
245+
// acting as someone with no record of it.
246+
//
247+
// `createdAt` is stamped explicitly rather than left to `@default(now())`, because Postgres `now()`
248+
// is the *transaction* timestamp: inside one transaction both rows would take the same value, and
249+
// an audit view ordered by that column couldn't tell which came first.
250+
const startedAt = new Date();
251+
const closedAt = new Date(startedAt.getTime() - 1);
252+
240253
try {
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({
254+
await $transaction(prismaClient, "startImpersonationAudit", async (tx) => {
255+
if (previousTargetId && previousTargetId !== userId) {
256+
await tx.impersonationAuditLog.create({
257+
data: {
258+
action: "STOP",
259+
adminId: admin.id,
260+
targetId: previousTargetId,
261+
ipAddress,
262+
createdAt: closedAt,
263+
},
264+
});
265+
}
266+
267+
await tx.impersonationAuditLog.create({
250268
data: {
251-
action: "STOP",
269+
action: "START",
252270
adminId: admin.id,
253-
targetId: previousTargetId,
271+
targetId: userId,
254272
ipAddress,
273+
createdAt: startedAt,
255274
},
256275
});
257-
}
258-
259-
await prismaClient.impersonationAuditLog.create({
260-
data: {
261-
action: "START",
262-
adminId: admin.id,
263-
targetId: userId,
264-
ipAddress,
265-
},
266276
});
267277
} catch (error) {
268278
logger.error("Failed to create impersonation audit log", {

0 commit comments

Comments
 (0)