fix(booking): use the standard visibility rule in the booking pickers

apps/webapp/app/modules/booking/service.server.load-bookings-data.test.ts

@@ -0,0 +1,192 @@
+/**
+ * Visibility regression tests for `loadBookingsData`.
+ *
+ * This is the loader that seeds the "Add to existing booking" pickers. It has
+ * to apply the SAME rules as the search endpoint those pickers switch to once
+ * the user types (`/api/model-filters`), or the list silently changes the
+ * moment someone types into the search box.
+ *
+ * There are TWO rules, and both surfaces AND both of them:
+ *
+ * 1. READ — the standard visibility rule: SELF_SERVICE / BASE users see only
+ *    bookings they are custodian of (via EITHER custody link), unless the
+ *    workspace has switched `selfServiceCanSeeBookings` /
+ *    `baseUserCanSeeBookings` on. `requirePermission` resolves that into
+ *    `canSeeAllBookings`. This loader previously gated on the role alone, so
+ *    the workspace override never reached these two dialogs.
+ * 2. WRITE — what `validateBookingOwnership` accepts on submit: creator OR
+ *    custodian, for SELF_SERVICE / BASE, independent of that toggle. A picker
+ *    exists to choose a mutation target, so offering a row the action then
+ *    rejects is a 403 dead end.
+ *
+ * Asserts on the `where` handed to Prisma, because the `where` is the boundary.
+ *
+ * @see {@link file://./service.server.ts} — `loadBookingsData`
+ * @see {@link file://./../../routes/api+/model-filters.ts}
+ * @see {@link file://./../../utils/booking-authorization.server.ts}
+ */
+import { OrganizationRoles } from "@prisma/client";
+import { db } from "~/database/db.server";
+import { loadBookingsData } from "./service.server";
+
+// @vitest-environment node
+
+// why: the subject is the `where` the loader ends up building, not what a
+// database returns. `count` is mocked because `getBookings` issues it alongside
+// `findMany`; `teamMember.findMany` because `resolveCustodianScope` reads it.
+vitest.mock("~/database/db.server", () => ({
+  db: {
+    booking: {
+      findMany: vitest.fn().mockResolvedValue([]),
+      count: vitest.fn().mockResolvedValue(0),
+    },
+    teamMember: {
+      findMany: vitest.fn().mockResolvedValue([{ id: "tm-1" }]),
+    },
+  },
+}));
+
+const findManyMock = db.booking.findMany as unknown as ReturnType<
+  typeof vitest.fn
+>;
+
+const ORGANIZATION_ID = "org-1";
+const USER_ID = "user-1";
+
+/** The READ restriction `resolveCustodianScope` produces for our fixture user. */
+const CUSTODIAN_RESTRICTION = {
+  OR: [
+    { custodianUserId: USER_ID },
+    { custodianTeamMemberId: { in: ["tm-1"] } },
+  ],
+};
+
+/** The WRITE restriction, mirroring `validateBookingOwnership`. */
+const WRITE_RESTRICTION = {
+  OR: [{ creatorId: USER_ID }, { custodianUserId: USER_ID }],
+};
+
+/**
+ * Runs the loader and returns the `where` Prisma was asked for.
+ *
+ * @param role - The caller's effective role, driving the write restriction.
+ * @param canSeeAllBookings - The resolved read-visibility flag under test.
+ * @returns The Prisma `where` from the resulting booking query.
+ */
+async function whereFor(role: OrganizationRoles, canSeeAllBookings: boolean) {
+  findManyMock.mockClear();
+
+  await loadBookingsData({
+    request: new Request(
+      "http://localhost/assets/a1/overview/add-to-existing-booking"
+    ),
+    organizationId: ORGANIZATION_ID,
+    userId: USER_ID,
+    role,
+    canSeeAllBookings,
+    ids: ["a1"],
+  });
+
+  return findManyMock.mock.calls.at(-1)?.[0]?.where;
+}
+
+/** Every combination the two dialogs can be loaded under. */
+const RESTRICTED_ROLES = [
+  OrganizationRoles.SELF_SERVICE,
+  OrganizationRoles.BASE,
+];
+const PRIVILEGED_ROLES = [OrganizationRoles.ADMIN, OrganizationRoles.OWNER];
+
+describe("loadBookingsData booking visibility", () => {
+  describe("read restriction", () => {
+    it.each(RESTRICTED_ROLES)(
+      "restricts %s to their own bookings, via EITHER custody link",
+      async (role) => {
+        const where = await whereFor(role, false);
+
+        expect(where.AND).toEqual(
+          expect.arrayContaining([
+            expect.objectContaining(CUSTODIAN_RESTRICTION),
+          ])
+        );
+      }
+    );
+
+    it.each(RESTRICTED_ROLES)(
+      "drops the custodian scope for %s when the workspace enables it",
+      async (role) => {
+        const where = await whereFor(role, true);
+
+        expect(where.AND).not.toEqual(
+          expect.arrayContaining([
+            expect.objectContaining(CUSTODIAN_RESTRICTION),
+          ])
+        );
+      }
+    );
+
+    it.each(PRIVILEGED_ROLES)("never restricts %s", async (role) => {
+      const where = await whereFor(role, true);
+
+      const restrictions = (where.AND ?? []).filter(
+        (clause: Record<string, unknown>) =>
+          JSON.stringify(clause).includes("custodianTeamMemberId") ||
+          JSON.stringify(clause).includes("custodianUserId")
+      );
+
+      expect(restrictions).toEqual([]);
+    });
+  });
+
+  describe("write restriction", () => {
+    // The dead end this exists to prevent: with the workspace setting ON, the
+    // read rule alone offered bookings `validateBookingOwnership` then 403s on.
+    it.each(RESTRICTED_ROLES)(
+      "keeps %s inside what the submitting action accepts, setting ON",
+      async (role) => {
+        const where = await whereFor(role, true);
+
+        expect(where.AND).toEqual(
+          expect.arrayContaining([expect.objectContaining(WRITE_RESTRICTION)])
+        );
+      }
+    );
+
+    it.each(RESTRICTED_ROLES)(
+      "applies to %s alongside the read rule, setting OFF",
+      async (role) => {
+        const where = await whereFor(role, false);
+
+        expect(where.AND).toEqual(
+          expect.arrayContaining([
+            expect.objectContaining(CUSTODIAN_RESTRICTION),
+            expect.objectContaining(WRITE_RESTRICTION),
+          ])
+        );
+      }
+    );
+
+    it.each(PRIVILEGED_ROLES)(
+      "does not constrain %s, who may write to every booking",
+      async (role) => {
+        const where = await whereFor(role, true);
+
+        // Asserting on the clause, not on the string "creatorId" — the
+        // draft-visibility clause legitimately carries that key.
+        expect(where.AND ?? []).not.toContainEqual(WRITE_RESTRICTION);
+      }
+    );
+  });
+
+  it("never drops the draft-visibility clause, whichever way the flags go", async () => {
+    for (const role of [...RESTRICTED_ROLES, ...PRIVILEGED_ROLES]) {
+      for (const flag of [true, false]) {
+        const where = await whereFor(role, flag);
+        const serialized = JSON.stringify(where.AND ?? []);
+
+        expect(serialized).toContain("DRAFT");
+        expect(serialized).toContain("creatorId");
+      }
+    }
+  });
+});

apps/webapp/app/modules/booking/service.server.ts

@@ -42,7 +42,10 @@
 import { lockAssetForQuantityUpdate } from "~/modules/consumption-log/quantity-lock.server";
 import { createConsumptionLog } from "~/modules/consumption-log/service.server";
 import { assetQtyMeta, formatUnitCount } from "~/utils/asset-quantity";
-import { validateBookingOwnership } from "~/utils/booking-authorization.server";
+import {
+  bookingWriteScopeClause,
+  validateBookingOwnership,
+} from "~/utils/booking-authorization.server";
 import { canUserRemoveBookingAssets } from "~/utils/bookings";
 import { getStatusClasses, isOneDayEvent } from "~/utils/calendar";
 import { getClientHint, type ClientHint } from "~/utils/client-hints";
@@ -2286,7 +2289,7 @@
   bookingFound,
   userId,
   effectiveStatus,
   effectiveBooking,
   effectiveTo,
   hints,
   organizationId,
@@ -2317,7 +2320,7 @@
 
   /** Calculate the time difference between the booking.to and the current time */
   const { hours } = calcTimeDifference(effectiveTo!, new Date());
   const lessThanOneHourToCheckin = hours < 1;
 
   /** We cancel just in case there is something pending */
   await cancelScheduler(bookingFound);
@@ -9256,6 +9259,37 @@
   };
 }
 
+/**
+ * Turns a {@link resolveCustodianScope} result into the single AND-able clause
+ * that expresses "these bookings are that person's" — custody on their user
+ * link OR on any of their team-member links.
+ *
+ * Extracted so {@link getBookings} and `/api/model-filters` cannot disagree on
+ * the shape. They previously did: the endpoint matched the user link alone, so
+ * a booking custodied through a legacy team-member row showed in the list a
+ * picker was seeded with and then vanished the moment the user typed.
+ *
+ * @param scope - Resolved custodian scope for ONE person.
+ * @returns A `Prisma.BookingWhereInput` to push into `where.AND` — never into a
+ *   top-level `OR`, where a user-supplied filter could widen it away.
+ */
+export function custodianScopeClause(scope: {
+  userId: string;
+  teamMemberIds?: string[];
+}): Prisma.BookingWhereInput {
+  const selfBranches: Prisma.BookingWhereInput[] = [
+    { custodianUserId: scope.userId },
+  ];
+
+  if (scope.teamMemberIds?.length) {
+    selfBranches.push({
+      custodianTeamMemberId: { in: scope.teamMemberIds },
+    });
+  }
+
+  return selfBranches.length === 1 ? selfBranches[0] : { OR: selfBranches };
+}
+
 /**
  * DRAFT-visibility rule shared by every booking-list query: bookings that are
  * not DRAFT are visible to everyone in the org, while DRAFT bookings are only
@@ -9404,6 +9438,16 @@
    * Accepts an array so the bookings index can filter by several team members.
    */
   custodianTeamMemberIds?: string[] | null;
+  /**
+   * RESTRICTION scoping results to bookings this person may MUTATE — see
+   * {@link bookingWriteScopeClause}. Set only by pickers whose selection feeds
+   * an action gated by `validateBookingOwnership`; omit for read-only lists.
+   *
+   * ONE object rather than two sibling params on purpose: a half-set pair
+   * (id without role, or role without id) would silently skip the restriction
+   * entirely. Both halves are required together or not at all.
+   */
+  writableBy?: { userId: string; role: OrganizationRoles } | null;
   excludeBookingIds?: Booking["id"][] | null;
   bookingFrom?: Booking["from"] | null;
   bookingTo?: Booking["to"] | null;
@@ -9431,6 +9475,7 @@
     statuses,
     custodianScope,
     custodianTeamMemberIds,
+    writableBy,
     assetIds,
     bookingTo,
     excludeBookingIds,
@@ -9529,19 +9574,25 @@
      * block also writes, so whichever ran last silently dropped the other.
      */
     if (custodianScope) {
-      const selfBranches: Prisma.BookingWhereInput[] = [
-        { custodianUserId: custodianScope.userId },
-      ];
+      andClauses.push(custodianScopeClause(custodianScope));
+    }
 
-      if (custodianScope.teamMemberIds?.length) {
-        selfBranches.push({
-          custodianTeamMemberId: { in: custodianScope.teamMemberIds },
-        });
-      }
+    /**
+     * A SECOND, independent restriction: the caller may only be offered
+     * bookings they are allowed to WRITE to. Set by mutation-target pickers
+     * (the "Add to existing booking" dialogs), never by read-only lists.
+     *
+     * Scalars rather than a where-input, so a call site cannot hand this a
+     * request-controlled predicate — the clause itself is fixed by
+     * {@link bookingWriteScopeClause}. AND-ed alongside `custodianScope`, so
+     * the two intersect and neither can widen the other.
+     */
+    if (writableBy) {
+      const writeScope = bookingWriteScopeClause(writableBy);
 
-      andClauses.push(
-        selfBranches.length === 1 ? selfBranches[0] : { OR: selfBranches }
-      );
+      if (writeScope) {
+        andClauses.push(writeScope);
+      }
     }
 
     /** The filter: independent, always AND-ed, never a restriction. */
@@ -12701,13 +12752,26 @@
   request,
   organizationId,
   userId,
-  isSelfServiceOrBase,
+  role,
+  canSeeAllBookings,
   ids,
 }: {
   request: Request;
   organizationId: string;
   userId: string;
-  isSelfServiceOrBase: boolean;
+  /**
+   * Effective role, from `requirePermission`. Drives the WRITE restriction —
+   * these pickers choose a mutation target, so they may only offer bookings
+   * the submitting action will accept.
+   */
+  role: OrganizationRoles;
+  /**
+   * Standard booking READ visibility, from `requirePermission`. Gating on the
+   * role alone ignored the workspace's `selfServiceCanSeeBookings` /
+   * `baseUserCanSeeBookings` overrides, so these pickers stayed restricted even
+   * when the workspace had switched the setting on.
+   */
+  canSeeAllBookings: boolean;
   ids?: string[];
 }): Promise<BookingLoaderResponse> {
   // Get search parameters and pagination settings
@@ -12718,10 +12782,15 @@
   // Fetch bookings with filters. Includes ONGOING/OVERDUE so assets/kits can be
   // added to active bookings (they stay AVAILABLE — progressive checkout), not
   // just to not-yet-started DRAFT/RESERVED ones.
-  // Self-service / base users may only work with their own bookings — resolve
-  // the full scope (user link + every team-member link) so legacy rows aren't
-  // hidden here while showing on the index.
-  const custodianScope = isSelfServiceOrBase
+  // TWO independent restrictions, both server-derived, both AND-ed. They must
+  // be computed identically here and in `/api/model-filters`, which takes over
+  // the moment the user types into the picker — a rule applied on only one of
+  // the two makes the list change mid-search.
+  //
+  // 1. READ — the standard booking-visibility rule. Resolve the FULL custodian
+  //    scope (user link + every team-member link) so legacy rows aren't hidden
+  //    here while showing on the index.
+  const custodianScope = !canSeeAllBookings
     ? await resolveCustodianScope({ userId, organizationId })
     : undefined;
 
@@ -12733,6 +12802,11 @@
     userId,
     statuses: ADDABLE_BOOKING_STATUSES,
     ...(custodianScope && { custodianScope }),
+    // 2. WRITE — what `validateBookingOwnership` will accept on submit. Kept
+    //    separate from the read rule because the workspace visibility toggle
+    //    does NOT grant write: without this, enabling it offers a restricted
+    //    user bookings the action then rejects with a 403.
+    writableBy: { userId, role },
   });
 
   // Set up header and model name