45ec23cc73
<img width="2284" height="2028" alt="CleanShot 2026-05-01 at 18 53 50@2x" src="https://github.com/user-attachments/assets/4f58cbb1-0168-40fb-a523-017f2ba625a1" /> ## Performance - **Per-request DB hit**: `getUserId` runs `getEffectiveSessionDuration` (User lookup + Org `aggregate`) on *every* authenticated request, including each fetcher poll. Consider caching the effective duration in the session cookie with a short TTL (e.g. 60s) and revalidating in the background. - **Double session commit in `root.tsx`**: `getUser` already runs the expiry check; then `commitAuthenticatedSessionLazy` commits the cookie again. Fine, but doubles `Set-Cookie` headers on every page load — worth a quick perf check. ## Correctness / Edge cases - **Lazy backfill assumes a root.tsx hit first**: users whose first post-deploy request is a fetcher/API route (`/resources/*`) skip the backfill until they navigate to a page. Not a security hole, but `getUserId` could backfill itself for completeness. - **No upper bound on `Organization.maxSessionDuration`**: admin API accepts `1` second, which would instant-logout every member on next request. Add a `min(60)` (or `min(300)` to match the lowest user option) to the Zod schema. - **No clock-skew tolerance**: `isSessionExpired` is exact-millisecond. Multi-instance deploys with skewed clocks could log users out a few seconds early/late. Probably fine for the 5-min minimum, but worth noting. ## Security - **Auto-logout audit log lacks IP/orgId**: HIPAA forensics typically wants source IP and which org context. Currently logs only `userId` + path. IP isn't PII for audit purposes; orgIds help correlate. Add both. - **Cookie `Max-Age` is 1 year regardless of user's setting**: intentional (server-side `issuedAt` is the source of truth), but reviewers will ask. Add a one-line comment on the cookie config explaining why. ## API surface - **`maxSessionDuration` is admin-PAT only**: no in-app UI for org owners to set/change their own cap. If this is "Trigger staff sets it during HIPAA onboarding", say so in the PR description; otherwise add an org-settings UI. - **Auto-submit dropdown has no confirmation**: misclicking "5 minutes" immediately shortens the user's session window with no undo. Consider a save button or 3-sec undo toast. ## Schema / migration - **`User.sessionDuration NOT NULL DEFAULT 31556952`**: instant on PG 11+ (metadata-only), but call out in the PR description so reviewers don't worry about a table rewrite on the User table. - **No DB-level constraint matching `SESSION_DURATION_OPTIONS`**: if the option list changes, existing users keep orphaned values. The dropdown's tag-along behaviour hides this — fine for now, but if you ever drop an option you'll need a backfill. ## UX - **Session expiry only fires on next request**: an idle authenticated tab keeps showing UI past the cap (until SSE/polling catches it, ~60s). Add a client-side timer based on the user's effective duration that triggers a fetcher to `/account` or `/logout` at expiry. - **No "you were signed out" message on logout**: users hitting their cap are bounced to `/` with no explanation. Was intentionally reverted in this PR — call that out so reviewers don't request it. ## Tests - Unit coverage on `sessionDuration.server.ts` is solid (215 lines). Missing: integration test for `getUserId` → expired session → redirect to `/logout`, and one for the loader's clamping fix (the most recent bug). Add at least the second one to lock in the regression. --------- Co-authored-by: Matt Aitken <matt@mattaitken.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
233 lines
8.6 KiB
TypeScript
233 lines
8.6 KiB
TypeScript
import { describe, expect, it, vi } from "vitest";
|
|
|
|
// `~/db.server` eagerly calls $connect() on the singleton Prisma client at
|
|
// module load. Without this mock the test process tries to reach DATABASE_URL
|
|
// (defaults to localhost:5432) and emits an unhandled rejection that fails the
|
|
// run. Tests still get a real Prisma client via the testcontainer fixture.
|
|
vi.mock("~/db.server", () => ({
|
|
prisma: {},
|
|
$replica: {},
|
|
}));
|
|
|
|
import { containerTest } from "@internal/testcontainers";
|
|
import { createCookieSessionStorage, type Session } from "@remix-run/node";
|
|
|
|
vi.setConfig({ testTimeout: 60_000 });
|
|
import {
|
|
commitAuthenticatedSession,
|
|
DEFAULT_SESSION_DURATION_SECONDS,
|
|
getAllowedSessionOptions,
|
|
getEffectiveSessionDuration,
|
|
getOrganizationSessionCap,
|
|
isAllowedSessionDuration,
|
|
SESSION_DURATION_OPTIONS,
|
|
} from "../app/services/sessionDuration.server";
|
|
|
|
const oneHour = 60 * 60;
|
|
const oneDay = 60 * 60 * 24;
|
|
const oneYear = DEFAULT_SESSION_DURATION_SECONDS;
|
|
|
|
const sessionStorage = createCookieSessionStorage({
|
|
cookie: { name: "__test_session", secrets: ["test"] },
|
|
});
|
|
|
|
async function makeEmptySession(): Promise<Session> {
|
|
return sessionStorage.getSession();
|
|
}
|
|
|
|
describe("isAllowedSessionDuration", () => {
|
|
it("accepts every value in the dropdown options", () => {
|
|
for (const option of SESSION_DURATION_OPTIONS) {
|
|
expect(isAllowedSessionDuration(option.value)).toBe(true);
|
|
}
|
|
});
|
|
|
|
it("rejects values not in the dropdown", () => {
|
|
expect(isAllowedSessionDuration(1)).toBe(false);
|
|
expect(isAllowedSessionDuration(7 * oneDay)).toBe(false);
|
|
expect(isAllowedSessionDuration(0)).toBe(false);
|
|
expect(isAllowedSessionDuration(-1)).toBe(false);
|
|
});
|
|
});
|
|
|
|
describe("getAllowedSessionOptions", () => {
|
|
it("returns all options when there is no org cap", () => {
|
|
const options = getAllowedSessionOptions(null, oneYear);
|
|
expect(options).toEqual(SESSION_DURATION_OPTIONS);
|
|
});
|
|
|
|
it("filters out options larger than the org cap", () => {
|
|
const options = getAllowedSessionOptions(oneHour, oneHour);
|
|
expect(options.map((o) => o.value)).toEqual([60 * 5, 60 * 30, 60 * 60]);
|
|
});
|
|
|
|
it("includes the user's current value even when it exceeds the cap, so the form stays valid", () => {
|
|
const options = getAllowedSessionOptions(oneHour, oneYear);
|
|
expect(options.some((o) => o.value === oneYear)).toBe(true);
|
|
expect(options.some((o) => o.value === oneHour)).toBe(true);
|
|
});
|
|
|
|
it("does not duplicate the current value when it is already within the cap", () => {
|
|
const options = getAllowedSessionOptions(oneDay, oneHour);
|
|
const oneHourCount = options.filter((o) => o.value === oneHour).length;
|
|
expect(oneHourCount).toBe(1);
|
|
});
|
|
});
|
|
|
|
async function createUser(prisma: any, email: string, sessionDuration?: number) {
|
|
return prisma.user.create({
|
|
data: {
|
|
email,
|
|
authenticationMethod: "MAGIC_LINK",
|
|
...(sessionDuration !== undefined ? { sessionDuration } : {}),
|
|
},
|
|
});
|
|
}
|
|
|
|
async function createOrgWithMember(
|
|
prisma: any,
|
|
slug: string,
|
|
userId: string,
|
|
maxSessionDuration: number | null
|
|
) {
|
|
return prisma.organization.create({
|
|
data: {
|
|
title: `Org ${slug}`,
|
|
slug,
|
|
maxSessionDuration,
|
|
members: { create: { userId, role: "ADMIN" } },
|
|
},
|
|
});
|
|
}
|
|
|
|
describe("getOrganizationSessionCap", () => {
|
|
containerTest("returns null when the user has no orgs with a cap set", async ({ prisma }) => {
|
|
const user = await createUser(prisma, "no-cap@test.com");
|
|
await createOrgWithMember(prisma, "no-cap-org", user.id, null);
|
|
|
|
const cap = await getOrganizationSessionCap(user.id, prisma);
|
|
expect(cap).toBeNull();
|
|
});
|
|
|
|
containerTest(
|
|
"returns the most restrictive cap across orgs, ignoring nulls",
|
|
async ({ prisma }) => {
|
|
const user = await createUser(prisma, "multi-org@test.com");
|
|
await createOrgWithMember(prisma, "loose-org", user.id, oneDay);
|
|
const tight = await createOrgWithMember(prisma, "tight-org", user.id, oneHour);
|
|
await createOrgWithMember(prisma, "uncapped-org", user.id, null);
|
|
|
|
const cap = await getOrganizationSessionCap(user.id, prisma);
|
|
expect(cap).toEqual({ orgCapSeconds: oneHour, cappingOrgId: tight.id });
|
|
}
|
|
);
|
|
|
|
containerTest("ignores soft-deleted organizations", async ({ prisma }) => {
|
|
const user = await createUser(prisma, "deleted-org-user@test.com");
|
|
const tight = await createOrgWithMember(prisma, "deleted-tight", user.id, oneHour);
|
|
const loose = await createOrgWithMember(prisma, "active-loose", user.id, oneDay);
|
|
|
|
await prisma.organization.update({
|
|
where: { id: tight.id },
|
|
data: { deletedAt: new Date() },
|
|
});
|
|
|
|
const cap = await getOrganizationSessionCap(user.id, prisma);
|
|
expect(cap).toEqual({ orgCapSeconds: oneDay, cappingOrgId: loose.id });
|
|
});
|
|
});
|
|
|
|
describe("getEffectiveSessionDuration", () => {
|
|
containerTest(
|
|
"returns the user setting when no org cap is set",
|
|
async ({ prisma }) => {
|
|
const user = await createUser(prisma, "effective-no-cap@test.com", oneDay);
|
|
await createOrgWithMember(prisma, "effective-no-cap-org", user.id, null);
|
|
|
|
const result = await getEffectiveSessionDuration(user.id, prisma);
|
|
expect(result.userSettingSeconds).toBe(oneDay);
|
|
expect(result.orgCapSeconds).toBeNull();
|
|
expect(result.cappingOrgId).toBeNull();
|
|
expect(result.durationSeconds).toBe(oneDay);
|
|
}
|
|
);
|
|
|
|
containerTest("caps the user setting at the most restrictive org cap", async ({ prisma }) => {
|
|
const user = await createUser(prisma, "effective-capped@test.com", oneYear);
|
|
const org = await createOrgWithMember(prisma, "effective-capped-org", user.id, oneHour);
|
|
|
|
const result = await getEffectiveSessionDuration(user.id, prisma);
|
|
expect(result.userSettingSeconds).toBe(oneYear);
|
|
expect(result.orgCapSeconds).toBe(oneHour);
|
|
expect(result.cappingOrgId).toBe(org.id);
|
|
expect(result.durationSeconds).toBe(oneHour);
|
|
});
|
|
|
|
containerTest(
|
|
"returns the user setting when it is already smaller than the org cap",
|
|
async ({ prisma }) => {
|
|
const user = await createUser(prisma, "effective-user-smaller@test.com", 60 * 5);
|
|
await createOrgWithMember(prisma, "effective-user-smaller-org", user.id, oneHour);
|
|
|
|
const result = await getEffectiveSessionDuration(user.id, prisma);
|
|
expect(result.durationSeconds).toBe(60 * 5);
|
|
}
|
|
);
|
|
|
|
containerTest(
|
|
"uses the default when the user has no row (defensive fallback)",
|
|
async ({ prisma }) => {
|
|
const result = await getEffectiveSessionDuration("nonexistent-user-id", prisma);
|
|
expect(result.userSettingSeconds).toBe(DEFAULT_SESSION_DURATION_SECONDS);
|
|
expect(result.orgCapSeconds).toBeNull();
|
|
expect(result.cappingOrgId).toBeNull();
|
|
expect(result.durationSeconds).toBe(DEFAULT_SESSION_DURATION_SECONDS);
|
|
}
|
|
);
|
|
});
|
|
|
|
describe("commitAuthenticatedSession", () => {
|
|
containerTest(
|
|
"stamps User.nextSessionEnd at now + user setting when no org cap",
|
|
async ({ prisma }) => {
|
|
const user = await createUser(prisma, "commit-no-cap@test.com", oneHour);
|
|
const session = await makeEmptySession();
|
|
const now = 1_700_000_000_000;
|
|
|
|
await commitAuthenticatedSession(session, user.id, now, prisma);
|
|
|
|
const updated = await prisma.user.findFirstOrThrow({ where: { id: user.id } });
|
|
expect(updated.nextSessionEnd?.getTime()).toBe(now + oneHour * 1000);
|
|
}
|
|
);
|
|
|
|
containerTest(
|
|
"stamps User.nextSessionEnd against the tightest org cap when smaller than user setting",
|
|
async ({ prisma }) => {
|
|
const user = await createUser(prisma, "commit-capped@test.com", oneYear);
|
|
await createOrgWithMember(prisma, "commit-capped-org", user.id, oneHour);
|
|
const session = await makeEmptySession();
|
|
const now = 1_700_000_000_000;
|
|
|
|
await commitAuthenticatedSession(session, user.id, now, prisma);
|
|
|
|
const updated = await prisma.user.findFirstOrThrow({ where: { id: user.id } });
|
|
expect(updated.nextSessionEnd?.getTime()).toBe(now + oneHour * 1000);
|
|
}
|
|
);
|
|
|
|
containerTest("resets nextSessionEnd to a fresh window on each commit", async ({ prisma }) => {
|
|
const user = await createUser(prisma, "commit-reset@test.com", oneHour);
|
|
const session = await makeEmptySession();
|
|
|
|
await commitAuthenticatedSession(session, user.id, 1_700_000_000_000, prisma);
|
|
const first = await prisma.user.findFirstOrThrow({ where: { id: user.id } });
|
|
|
|
await commitAuthenticatedSession(session, user.id, 1_700_000_060_000, prisma);
|
|
const second = await prisma.user.findFirstOrThrow({ where: { id: user.id } });
|
|
|
|
expect(second.nextSessionEnd?.getTime()).toBeGreaterThan(first.nextSessionEnd!.getTime());
|
|
expect(second.nextSessionEnd?.getTime()).toBe(1_700_000_060_000 + oneHour * 1000);
|
|
});
|
|
});
|