Code review improvements

Author: samejr

apps/webapp/app/routes/admin.api.v1.orgs.$organizationId.session-duration.ts

@@ -2,6 +2,10 @@ import { type ActionFunctionArgs, json } from "@remix-run/server-runtime";
 import { z } from "zod";
 import { prisma } from "~/db.server";
 import { requireAdminApiRequest } from "~/services/personalAccessToken.server";
+import {
+  ALLOWED_SESSION_DURATION_VALUES,
+  isAllowedSessionDuration,
+} from "~/services/sessionDuration.server";
 
 const ParamsSchema = z.object({
   organizationId: z.string(),
@@ -12,15 +16,33 @@ const RequestBodySchema = z.object({
    * Maximum session lifetime (seconds) for members of this organization, or
    * null to remove the cap. When set, this caps each member's
    * `User.sessionDuration` and is enforced on the user's next request.
+   *
+   * Must be one of the values in `SESSION_DURATION_OPTIONS` so the cap always
+   * maps to a labeled dropdown option for users — otherwise users see fallback
+   * labels like "7200 seconds" in the UI. To allow a new value, add it to
+   * `SESSION_DURATION_OPTIONS`.
    */
-  maxSessionDuration: z.number().int().positive().nullable(),
+  maxSessionDuration: z
+    .number()
+    .int()
+    .positive()
+    .nullable()
+    .refine((v) => v === null || isAllowedSessionDuration(v), {
+      message: `maxSessionDuration must be one of: ${[...ALLOWED_SESSION_DURATION_VALUES]
+        .sort((a, b) => a - b)
+        .join(", ")}`,
+    }),
 });
 
 export async function action({ request, params }: ActionFunctionArgs) {
   await requireAdminApiRequest(request);
 
   const { organizationId } = ParamsSchema.parse(params);
-  const body = RequestBodySchema.parse(await request.json());
+  const parseResult = RequestBodySchema.safeParse(await request.json());
+  if (!parseResult.success) {
+    return json({ success: false, errors: parseResult.error.flatten() }, { status: 400 });
+  }
+  const body = parseResult.data;
 
   const organization = await prisma.organization.update({
     where: { id: organizationId },

apps/webapp/app/services/session.server.ts

@@ -2,6 +2,7 @@ import { redirect } from "@remix-run/node";
 import { $replica } from "~/db.server";
 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 { logger } from "./logger.server";
@@ -30,22 +31,26 @@ async function enforceSessionExpiry(
   // when one is configured (falls back to primary). Stale-by-replica-lag is
   // acceptable here because the worst case is a session living a few seconds
   // past its cap on the very first request after a cap change.
-  const { durationSeconds, orgCapSeconds, userSettingSeconds } =
+  const { durationSeconds, orgCapSeconds, cappingOrgId, userSettingSeconds } =
     await getEffectiveSessionDuration(userId, $replica);
   if (!isSessionExpired(session, durationSeconds)) return;
 
   const issuedAt = getSessionIssuedAt(session);
   // HIPAA audit trail: structured log lands in CloudWatch via stdout. Use
   // the stable `event` field to filter/aggregate auto-logout events.
+  // `sourceIp` uses ALB's appended (last) X-Forwarded-For element, not the
+  // first one, since the leading element is client-supplied and spoofable.
   logger.info("Auto-logout: session exceeded effective duration", {
     event: "session.auto_logout",
     userId,
     impersonatedUserId,
+    cappingOrgId,
     effectiveDurationSeconds: durationSeconds,
     userSettingSeconds,
     orgCapSeconds,
     sessionAgeMs: issuedAt === null ? null : Date.now() - issuedAt,
     requestPath: new URL(request.url).pathname,
+    sourceIp: extractClientIp(request.headers.get("x-forwarded-for")),
   });
   throw redirect("/logout");
 }

apps/webapp/app/services/sessionDuration.server.ts

@@ -35,31 +35,42 @@ export function isAllowedSessionDuration(value: number): boolean {
   return ALLOWED_SESSION_DURATION_VALUES.has(value);
 }
 
+export type OrganizationSessionCap = {
+  /** The org cap in seconds. */
+  orgCapSeconds: number;
+  /** The id of the org whose cap is currently the most restrictive. */
+  cappingOrgId: string;
+};
+
 /**
- * Returns the most restrictive max session duration (in seconds) across all of
- * the user's organizations, ignoring orgs where it's null. Returns null when
- * no org has set a cap.
+ * Returns the most restrictive max session duration across the user's orgs
+ * along with the id of the org that owns it, ignoring orgs where the cap is
+ * null. Returns null when no org has set a cap.
  */
 export async function getOrganizationSessionCap(
   userId: string,
   client: PrismaClientOrTransaction = prisma
-): Promise<number | null> {
-  const result = await client.organization.aggregate({
+): Promise<OrganizationSessionCap | null> {
+  const tightest = await client.organization.findFirst({
     where: {
       members: { some: { userId } },
       maxSessionDuration: { not: null },
       deletedAt: null,
     },
-    _min: { maxSessionDuration: true },
+    orderBy: { maxSessionDuration: "asc" },
+    select: { id: true, maxSessionDuration: true },
   });
-  return result._min.maxSessionDuration ?? null;
+  if (!tightest || tightest.maxSessionDuration === null) return null;
+  return { orgCapSeconds: tightest.maxSessionDuration, cappingOrgId: tightest.id };
 }
 
 export type EffectiveSessionDuration = {
   /** Effective session duration in seconds = min(user.sessionDuration, orgCap?). */
   durationSeconds: number;
   /** The org cap in seconds, or null if no org caps the user. */
   orgCapSeconds: number | null;
+  /** The id of the org whose cap is currently in effect, or null. */
+  cappingOrgId: string | null;
   /** The raw user setting in seconds. */
   userSettingSeconds: number;
 };
@@ -83,11 +94,12 @@ export async function getEffectiveSessionDuration(
 
   const userSettingSeconds = user?.sessionDuration ?? DEFAULT_SESSION_DURATION_SECONDS;
   const durationSeconds =
-    orgCap === null ? userSettingSeconds : Math.min(userSettingSeconds, orgCap);
+    orgCap === null ? userSettingSeconds : Math.min(userSettingSeconds, orgCap.orgCapSeconds);
 
   return {
     durationSeconds,
-    orgCapSeconds: orgCap,
+    orgCapSeconds: orgCap?.orgCapSeconds ?? null,
+    cappingOrgId: orgCap?.cappingOrgId ?? null,
     userSettingSeconds,
   };
 }

apps/webapp/test/sessionDuration.test.ts

@@ -145,26 +145,26 @@ describe("getOrganizationSessionCap", () => {
     async ({ prisma }) => {
       const user = await createUser(prisma, "multi-org@test.com");
       await createOrgWithMember(prisma, "loose-org", user.id, oneDay);
-      await createOrgWithMember(prisma, "tight-org", user.id, oneHour);
+      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).toBe(oneHour);
+      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);
-    await createOrgWithMember(prisma, "active-loose", user.id, oneDay);
+    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).toBe(oneDay);
+    expect(cap).toEqual({ orgCapSeconds: oneDay, cappingOrgId: loose.id });
   });
 });
 
@@ -178,17 +178,19 @@ describe("getEffectiveSessionDuration", () => {
       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);
-    await createOrgWithMember(prisma, "effective-capped-org", user.id, oneHour);
+    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);
   });
 
@@ -209,6 +211,7 @@ describe("getEffectiveSessionDuration", () => {
       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);
     }
   );