diff --git a/apps/webapp/app/modules/booking/service.server.load-bookings-data.test.ts b/apps/webapp/app/modules/booking/service.server.load-bookings-data.test.ts new file mode 100644 index 000000000..a69cc9521 --- /dev/null +++ b/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) => + 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"); + } + } + }); +}); diff --git a/apps/webapp/app/modules/booking/service.server.ts b/apps/webapp/app/modules/booking/service.server.ts index e4ac40ff8..3bbad9669 100644 --- a/apps/webapp/app/modules/booking/service.server.ts +++ b/apps/webapp/app/modules/booking/service.server.ts @@ -42,7 +42,10 @@ import { checkAndNotifyLowStock } from "~/modules/consumption-log/low-stock.serv 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"; @@ -9256,6 +9259,37 @@ export async function getBookingsFilterData({ }; } +/** + * 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 @@ export async function getBookings(params: { * 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 @@ export async function getBookings(params: { statuses, custodianScope, custodianTeamMemberIds, + writableBy, assetIds, bookingTo, excludeBookingIds, @@ -9529,19 +9574,25 @@ export async function getBookings(params: { * 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 @@ export async function loadBookingsData({ 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 { // Get search parameters and pagination settings @@ -12718,10 +12782,15 @@ export async function loadBookingsData({ // 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 @@ export async function loadBookingsData({ 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 diff --git a/apps/webapp/app/routes/_layout+/assets.$assetId.overview.add-to-existing-booking.tsx b/apps/webapp/app/routes/_layout+/assets.$assetId.overview.add-to-existing-booking.tsx index dfaf2eaab..0bd10d8c3 100644 --- a/apps/webapp/app/routes/_layout+/assets.$assetId.overview.add-to-existing-booking.tsx +++ b/apps/webapp/app/routes/_layout+/assets.$assetId.overview.add-to-existing-booking.tsx @@ -62,12 +62,14 @@ export async function loader({ context, request, params }: LoaderFunctionArgs) { }); try { - const { organizationId, isSelfServiceOrBase } = await requirePermission({ - userId: authSession?.userId, - request, - entity: PermissionEntity.booking, - action: PermissionAction.create, - }); + const { organizationId, role, canSeeAllBookings } = await requirePermission( + { + userId: authSession?.userId, + request, + entity: PermissionEntity.booking, + action: PermissionAction.create, + } + ); // loadBookingsData + the asset lookup are independent (both only // need organizationId from requirePermission above), so parallelise @@ -77,7 +79,8 @@ export async function loader({ context, request, params }: LoaderFunctionArgs) { request, organizationId, userId: authSession?.userId, - isSelfServiceOrBase, + role, + canSeeAllBookings, ids: assetId ? [assetId] : undefined, }), db.asset.findFirst({ @@ -286,10 +289,6 @@ export default function ExistingBooking() { // `loadBookingsData` seeds the list with — otherwise searching // returns bookings this dialog then refuses to render. status: ADDABLE_BOOKING_STATUSES.join(","), - // Keep the typed list inside the same custodian scope - // `loadBookingsData` seeds it with, so SELF_SERVICE / BASE users - // are not offered bookings that submit would then reject. - scopeToCustodian: true, }} fieldName="bookingId" contentLabel="Existing Bookings" diff --git a/apps/webapp/app/routes/_layout+/kits.$kitId.assets.add-to-existing-booking.tsx b/apps/webapp/app/routes/_layout+/kits.$kitId.assets.add-to-existing-booking.tsx index 851bc153d..b8b44f59f 100644 --- a/apps/webapp/app/routes/_layout+/kits.$kitId.assets.add-to-existing-booking.tsx +++ b/apps/webapp/app/routes/_layout+/kits.$kitId.assets.add-to-existing-booking.tsx @@ -68,18 +68,21 @@ export async function loader({ context, request, params }: LoaderFunctionArgs) { }); try { - const { organizationId, isSelfServiceOrBase } = await requirePermission({ - userId: authSession?.userId, - request, - entity: PermissionEntity.booking, - action: PermissionAction.create, - }); + const { organizationId, role, canSeeAllBookings } = await requirePermission( + { + userId: authSession?.userId, + request, + entity: PermissionEntity.booking, + action: PermissionAction.create, + } + ); const loaderData = await loadBookingsData({ request, organizationId, userId: authSession?.userId, - isSelfServiceOrBase, + role, + canSeeAllBookings, ids: kitId ? [kitId] : undefined, }); @@ -303,10 +306,6 @@ export default function ExistingBooking() { // `loadBookingsData` seeds the list with — otherwise searching // returns bookings this dialog then refuses to render. status: ADDABLE_BOOKING_STATUSES.join(","), - // Keep the typed list inside the same custodian scope - // `loadBookingsData` seeds it with, so SELF_SERVICE / BASE users - // are not offered bookings that submit would then reject. - scopeToCustodian: true, }} fieldName="bookingId" contentLabel=" Existing Bookings" diff --git a/apps/webapp/app/routes/api+/model-filters.ts b/apps/webapp/app/routes/api+/model-filters.ts index 77139e1fd..050802186 100644 --- a/apps/webapp/app/routes/api+/model-filters.ts +++ b/apps/webapp/app/routes/api+/model-filters.ts @@ -1,17 +1,18 @@ -import type { Prisma } from "@prisma/client"; import { BookingStatus, TagUseFor } from "@prisma/client"; import { data, type LoaderFunctionArgs } from "react-router"; import { z } from "zod"; import { db } from "~/database/db.server"; import { bookingDraftVisibilityClause, + custodianScopeClause, resolveCustodianScope, } from "~/modules/booking/service.server"; import { getSelectedOrganization } from "~/modules/organization/context.server"; +import { bookingWriteScopeClause } from "~/utils/booking-authorization.server"; import { makeShelfError } from "~/utils/error"; import { payload, error, parseData } from "~/utils/http.server"; import { - isSelfServiceOrBaseRole, + resolveCanSeeAllBookings, resolveEffectiveRole, } from "~/utils/roles.server"; @@ -89,25 +90,6 @@ export const ModelFiltersSchema = z.discriminatedUnion("name", [ ), { message: "Invalid booking status" } ), - - /** - * Opt in to the same custodian restriction the seeding loader applies. - * - * Set by the "Add to existing booking" dialogs, whose loader runs - * `loadBookingsData` → `getBookings({ custodianScope })` for SELF_SERVICE / - * BASE callers. Without it, typing replaces their custodian-scoped list with - * bookings they do not own, which `validateBookingOwnership` then rejects on - * submit — a dead end. - * - * The asset-index advanced filter does NOT set it, so that surface keeps - * seeing the same rows before and after typing. - * - * This is a request-controlled *toggle*, never a request-controlled *value*: - * the ids it restricts to are resolved server-side from the session user, so - * it cannot be used to widen or retarget the scope (the hazard documented on - * `getMinimalBookings`). It is also a no-op for ADMIN / OWNER. - */ - scopeToCustodian: z.coerce.boolean().optional(), }), BasicModelFilters.extend({ name: z.literal("assetModel"), @@ -123,9 +105,8 @@ export async function loader({ context, request }: LoaderFunctionArgs) { const { userId } = authSession; try { - const { organizationId, userOrganizations } = await getSelectedOrganization( - { userId, request } - ); + const { organizationId, userOrganizations, currentOrganization } = + await getSelectedOrganization({ userId, request }); /** Getting all the query parameters from url */ const url = new URL(request.url); @@ -208,35 +189,51 @@ export async function loader({ context, request }: LoaderFunctionArgs) { */ where.AND = [...(where.AND ?? []), bookingDraftVisibilityClause(userId)]; + const role = resolveEffectiveRole({ userOrganizations, organizationId }); + /** - * Callers that opted in get the seeding loader's custodian restriction, - * resolved here from the session — see the `scopeToCustodian` docs above. + * Standard booking READ visibility: SELF_SERVICE / BASE users only see + * bookings they are custodian of, unless the workspace has switched the + * setting on. + * + * Resolved from the session role plus the organization's settings, never + * from a request param, and AND-ed so the search `OR` cannot widen it. + * The restriction used to be opt-in via a `scopeToCustodian` query param, + * which a caller could simply omit — every booking row in the workspace + * came back to a restricted user with the setting off. + * + * Shares `custodianScopeClause` with `getBookings` so the shape matches + * the loader that seeded the picker; matching only `custodianUserId` here + * dropped bookings custodied through a legacy team-member row as soon as + * the user typed. */ - if ( - modelFilters.scopeToCustodian && - isSelfServiceOrBaseRole( - resolveEffectiveRole({ userOrganizations, organizationId }) - ) - ) { - const custodianScope = await resolveCustodianScope({ - userId, - organizationId, - }); - - const selfBranches: Prisma.BookingWhereInput[] = [ - { custodianUserId: custodianScope.userId }, - ]; - - if (custodianScope.teamMemberIds.length) { - selfBranches.push({ - custodianTeamMemberId: { in: custodianScope.teamMemberIds }, - }); - } - + if (!resolveCanSeeAllBookings({ role, currentOrganization })) { where.AND.push( - selfBranches.length === 1 ? selfBranches[0] : { OR: selfBranches } + custodianScopeClause( + await resolveCustodianScope({ userId, organizationId }) + ) ); } + + /** + * Standard booking WRITE authorization, AND-ed on top. + * + * Every reachable consumer of a booking search for a restricted role is a + * mutation-target picker — the two "Add to existing booking" dialogs. The + * asset-index advanced filter is the only read-only consumer, and + * `assets._index.tsx` refuses ADVANCED mode to SELF_SERVICE / BASE + * outright, so this never narrows a list they can otherwise reach. + * + * Independent of the visibility toggle on purpose: it mirrors + * `validateBookingOwnership`, which ignores that toggle. Offering rows the + * action rejects turns the picker into a 403 dead end. A future read-only + * booking search for these roles needs a purpose distinction here. + */ + const writeScope = bookingWriteScopeClause({ userId, role }); + + if (writeScope) { + where.AND.push(writeScope); + } } if (modelFilters.name === "tag" && modelFilters.useFor) { diff --git a/apps/webapp/app/utils/booking-authorization.server.test.ts b/apps/webapp/app/utils/booking-authorization.server.test.ts index e0d165c16..5de226a61 100644 --- a/apps/webapp/app/utils/booking-authorization.server.test.ts +++ b/apps/webapp/app/utils/booking-authorization.server.test.ts @@ -14,9 +14,14 @@ * * @see {@link file://./booking-authorization.server.ts} */ +import { OrganizationRoles } from "@prisma/client"; import { describe, expect, it } from "vitest"; -import { canSeeBooking } from "./booking-authorization.server"; +import { + bookingWriteScopeClause, + canSeeBooking, + validateBookingOwnership, +} from "./booking-authorization.server"; const ME = "user-me"; const SOMEONE_ELSE = "user-victim"; @@ -132,3 +137,160 @@ describe("canSeeBooking", () => { }); }); }); + +/** + * A booking reduced to the two columns both the clause and the gate read. + */ +type BookingRow = { creatorId: string | null; custodianUserId: string | null }; + +/** + * Evaluates the clause against a row, the way Postgres would. + * + * Deliberately narrow: it understands ONLY the `{ OR: [{ field: value }, …] }` + * shape {@link bookingWriteScopeClause} emits, and throws on anything else. If + * the clause grows a construct this cannot evaluate, the equivalence test below + * fails loudly instead of quietly passing on an unchecked predicate. + * + * @param clause - The where-input under test. + * @param row - The candidate booking. + * @returns Whether the row would be returned by a query carrying the clause. + */ +function rowMatches( + clause: Record | undefined, + row: BookingRow +): boolean { + if (!clause) { + return true; // No restriction — every row qualifies. + } + + const branches = clause.OR; + + if (!Array.isArray(branches)) { + throw new Error( + `Unsupported clause shape: ${JSON.stringify( + clause + )}. Extend rowMatches to cover it.` + ); + } + + return branches.some((branch: Record) => + Object.entries(branch).every(([field, value]) => { + if (typeof value !== "string") { + throw new Error(`Unsupported branch: ${JSON.stringify(branch)}`); + } + return row[field as keyof BookingRow] === value; + }) + ); +} + +/** + * Runs the submit-time gate and reports whether it let the caller through. + * + * @param row - The candidate booking. + * @param role - The caller's effective role. + * @returns `true` when {@link validateBookingOwnership} does not throw. + */ +function gateAllows(row: BookingRow, role: OrganizationRoles): boolean { + try { + validateBookingOwnership({ + booking: row, + userId: ME, + role, + action: "add items to", + }); + return true; + } catch { + return false; + } +} + +/** + * The point of the clause: it is the query-side mirror of the submit-time gate. + * Any row a picker offers must be one the action will accept, or the user hits + * a 403 dead end — so these two must agree on EVERY row, for EVERY role. + */ +describe("bookingWriteScopeClause", () => { + const ROWS: Array<{ label: string; row: BookingRow }> = [ + { + label: "created and custodied by me", + row: { creatorId: ME, custodianUserId: ME }, + }, + { + label: "created by me, custodied by someone else", + row: { creatorId: ME, custodianUserId: SOMEONE_ELSE }, + }, + { + label: "created by someone else, custodied by me", + row: { creatorId: SOMEONE_ELSE, custodianUserId: ME }, + }, + { + label: "entirely someone else's", + row: { creatorId: SOMEONE_ELSE, custodianUserId: SOMEONE_ELSE }, + }, + { + label: "unassigned with no creator", + row: { creatorId: null, custodianUserId: null }, + }, + ]; + + const ROLES = [ + OrganizationRoles.SELF_SERVICE, + OrganizationRoles.BASE, + OrganizationRoles.ADMIN, + OrganizationRoles.OWNER, + ]; + + for (const role of ROLES) { + for (const { label, row } of ROWS) { + it(`agrees with validateBookingOwnership for ${role} on a booking ${label}`, () => { + const clause = bookingWriteScopeClause({ userId: ME, role }) as + | Record + | undefined; + + expect(rowMatches(clause, row)).toBe(gateAllows(row, role)); + }); + } + } + + it.each([OrganizationRoles.ADMIN, OrganizationRoles.OWNER])( + "returns no restriction for %s", + (role) => { + expect(bookingWriteScopeClause({ userId: ME, role })).toBeUndefined(); + } + ); + + it.each([OrganizationRoles.SELF_SERVICE, OrganizationRoles.BASE])( + "restricts %s to bookings they created or hold", + (role) => { + expect(bookingWriteScopeClause({ userId: ME, role })).toEqual({ + OR: [{ creatorId: ME }, { custodianUserId: ME }], + }); + } + ); + + /** + * The clause allow-lists ADMIN / OWNER rather than deny-listing the two + * restricted roles, so a role added to `OrganizationRoles` later lands in the + * RESTRICTED branch by default. That direction is the safe one: the picker + * under-offers, which someone notices, instead of offering rows no rule + * covered. + */ + it("restricts an unrecognised role rather than waving it through", () => { + const futureRole = "AUDITOR" as OrganizationRoles; + + expect(bookingWriteScopeClause({ userId: ME, role: futureRole })).toEqual({ + OR: [{ creatorId: ME }, { custodianUserId: ME }], + }); + }); + + it("covers every role in the enum, so a new one cannot slip past unreviewed", () => { + // Fails the moment `OrganizationRoles` grows a member: whoever adds it has + // to decide which side of this clause it belongs on. + expect(Object.values(OrganizationRoles).sort()).toEqual([ + OrganizationRoles.ADMIN, + OrganizationRoles.BASE, + OrganizationRoles.OWNER, + OrganizationRoles.SELF_SERVICE, + ]); + }); +}); diff --git a/apps/webapp/app/utils/booking-authorization.server.ts b/apps/webapp/app/utils/booking-authorization.server.ts index 255258274..52df6dd15 100644 --- a/apps/webapp/app/utils/booking-authorization.server.ts +++ b/apps/webapp/app/utils/booking-authorization.server.ts @@ -1,3 +1,4 @@ +import type { Prisma } from "@prisma/client"; import { OrganizationRoles } from "@prisma/client"; import { ShelfError } from "./error"; @@ -61,6 +62,58 @@ export function canSeeBooking({ ); } +/** + * The query-side mirror of {@link validateBookingOwnership}'s default check: + * the set of bookings a caller may MUTATE (add assets/kits to, edit, …). + * + * `validateBookingOwnership` is a per-row gate that runs at submit time. A + * picker whose whole purpose is to choose a mutation target has to offer that + * SAME set, or the user selects a row the action then 403s on. Sharing the + * predicate is what keeps the two from drifting: change the rule below and the + * gate, and every picker follows. + * + * Deliberately independent of `canSeeAllBookings`. That workspace toggle + * governs READ visibility only — `validateBookingOwnership` ignores it, so a + * SELF_SERVICE user in a workspace with the toggle on can view another user's + * booking but still cannot write to it. Gating a mutation-target picker on the + * read rule is what produced the dead-end this mirrors away. + * + * KNOWN GAP, intentionally mirrored rather than fixed here: like + * `validateBookingOwnership`, this matches only `custodianUserId` and NOT the + * team-member custody link, so a legacy booking whose custody sits solely on + * `custodianTeamMemberId` is excluded. That is a faithful reflection of what + * the action accepts today — offering those rows would just restore the 403. + * Widening both together (as {@link canSeeBooking} already does for reads) is a + * separate change that has to sweep every `validateBookingOwnership` call site. + * + * @param params.userId - The caller. + * @param params.role - The caller's effective role in the workspace. + * @returns A `Prisma.BookingWhereInput` to AND into the query, or `undefined` + * for ADMIN / OWNER, who may write to every booking in the workspace. + */ +export function bookingWriteScopeClause({ + userId, + role, +}: { + userId: string; + role: OrganizationRoles; +}): Prisma.BookingWhereInput | undefined { + // ALLOW-list, not a deny-list on SELF_SERVICE/BASE. A role added to the enum + // later defaults to RESTRICTED here, so the picker under-offers (a visible + // gap) rather than offering rows nobody checked. The gate below still + // deny-lists, matching what it has always enforced — so for a hypothetical + // new role this clause is deliberately the stricter of the two. + const canWriteToEveryBooking = + role === OrganizationRoles.ADMIN || role === OrganizationRoles.OWNER; + + if (canWriteToEveryBooking) { + return undefined; + } + + // Mirrors the `checkCustodianOnly: false` branch below: creator OR custodian. + return { OR: [{ creatorId: userId }, { custodianUserId: userId }] }; +} + interface ValidateBookingOwnershipParams { booking: { creatorId: string | null; diff --git a/apps/webapp/app/utils/roles.server.ts b/apps/webapp/app/utils/roles.server.ts index 2c6b7e749..e76c3a116 100644 --- a/apps/webapp/app/utils/roles.server.ts +++ b/apps/webapp/app/utils/roles.server.ts @@ -88,6 +88,44 @@ export function isSelfServiceOrBaseRole(role: OrganizationRoles): boolean { ); } +/** + * Whether the caller may see bookings they are not the custodian of. + * + * ADMIN / OWNER always can. SELF_SERVICE and BASE only can when the workspace + * has switched the corresponding setting on. This is the standard visibility + * rule for bookings; every read path that can surface someone else's booking + * gates on it (`/bookings`, the command palette, CSV export). + * + * Exported so callers outside {@link requirePermission} resolve it identically. + * A surface that invents its own rule ends up disagreeing with the loader that + * seeded it, which is how a picker's list changes the moment a user types. + * + * @param args.role - Effective role from {@link resolveEffectiveRole}. + * @param args.currentOrganization - Workspace whose override settings apply. + * @returns `true` when bookings should NOT be restricted to the caller's own. + */ +export function resolveCanSeeAllBookings({ + role, + currentOrganization, +}: { + role: OrganizationRoles; + currentOrganization: { + selfServiceCanSeeBookings: boolean; + baseUserCanSeeBookings: boolean; + }; +}): boolean { + return ( + // Admin/Owner always can see all + !isSelfServiceOrBaseRole(role) || + // SELF_SERVICE can see all if org setting allows + (role === OrganizationRoles.SELF_SERVICE && + currentOrganization.selfServiceCanSeeBookings) || + // BASE can see all if org setting allows + (role === OrganizationRoles.BASE && + currentOrganization.baseUserCanSeeBookings) + ); +} + export async function requirePermission({ userId, request, @@ -141,15 +179,10 @@ export async function requirePermission({ * This checks the organization settings permissions overrides for BASE and SELF_SERVICE roles * If the user is in a BASE or SELF_SERVICE role, we check if they can see all bookings */ - const canSeeAllBookings = - // Admin/Owner always can see all - !isSelfServiceOrBase || - // SELF_SERVICE can see all if org setting allows - (role === OrganizationRoles.SELF_SERVICE && - currentOrganization.selfServiceCanSeeBookings) || - // BASE can see all if org setting allows - (role === OrganizationRoles.BASE && - currentOrganization.baseUserCanSeeBookings); + const canSeeAllBookings = resolveCanSeeAllBookings({ + role, + currentOrganization, + }); // Determine if user can see all custody information const canSeeAllCustody = diff --git a/apps/webapp/test/routes-tests/api+/model-filters.test.ts b/apps/webapp/test/routes-tests/api+/model-filters.test.ts index 6bd992c45..5aa014451 100644 --- a/apps/webapp/test/routes-tests/api+/model-filters.test.ts +++ b/apps/webapp/test/routes-tests/api+/model-filters.test.ts @@ -74,20 +74,33 @@ const clause = vi.hoisted(() => ({ { AND: [{ status: "DRAFT" }, { creatorId: userId }] }, ], }), + buildCustodianScope: (scope: { userId: string; teamMemberIds?: string[] }) => + scope.teamMemberIds?.length + ? { + OR: [ + { custodianUserId: scope.userId }, + { custodianTeamMemberId: { in: scope.teamMemberIds } }, + ], + } + : { custodianUserId: scope.userId }, })); const bookingMocks = vi.hoisted(() => ({ resolveCustodianScope: vi.fn(), })); -// why: service.server pulls in the whole booking domain (schedulers, emails); -// the clause itself is pure, so a local equivalent keeps the test fast. +// why: service.server pulls in the whole booking domain (schedulers, emails). +// `custodianScopeClause` is pure, so a local equivalent keeps the test fast; +// `resolveCustodianScope` is the one DB read, stubbed to a fixed scope. vi.mock("~/modules/booking/service.server", () => ({ bookingDraftVisibilityClause: clause.buildDraftVisibility, + custodianScopeClause: clause.buildCustodianScope, resolveCustodianScope: bookingMocks.resolveCustodianScope, })); const ORG_ID = "org-1"; +/** Team-member rows the fixture user holds in the workspace. */ +const TEAM_MEMBER_IDS = ["tm-1"]; /** * Builds the loader args for a given query string. @@ -133,11 +146,38 @@ async function readFilters(response: Response) { /** The clause every booking read path AND-s in — drafts are creator-only. */ const DRAFT_VISIBILITY = clause.buildDraftVisibility("user-1"); -/** Points the session at a workspace where the caller holds `role`. */ -function setRole(role: OrganizationRoles) { +/** + * READ restriction — the standard visibility rule, matching custody on EITHER + * link. Must be the same shape `getBookings` builds for the loader that seeded + * the picker, or the list changes the moment the user types. + */ +const CUSTODIAN_SCOPE = clause.buildCustodianScope({ + userId: "user-1", + teamMemberIds: TEAM_MEMBER_IDS, +}); + +/** + * WRITE restriction — mirrors `validateBookingOwnership`, which is what the + * "Add to existing booking" actions enforce on submit. + */ +const WRITE_SCOPE = { + OR: [{ creatorId: "user-1" }, { custodianUserId: "user-1" }], +}; + +/** + * Points the session at a workspace where the caller holds `role`. + * + * @param role - Caller's role in the workspace. + * @param canSeeAllOverride - Workspace override settings, both off by default. + */ +function setRole(role: OrganizationRoles, canSeeAllOverride = false) { orgMocks.getSelectedOrganization.mockResolvedValue({ organizationId: ORG_ID, userOrganizations: [{ organization: { id: ORG_ID }, roles: [role] }], + currentOrganization: { + selfServiceCanSeeBookings: canSeeAllOverride, + baseUserCanSeeBookings: canSeeAllOverride, + }, }); } @@ -145,6 +185,10 @@ describe("GET /api/model-filters", () => { beforeEach(() => { vi.clearAllMocks(); setRole(OrganizationRoles.OWNER); + bookingMocks.resolveCustodianScope.mockResolvedValue({ + userId: "user-1", + teamMemberIds: TEAM_MEMBER_IDS, + }); }); describe("result shape", () => { @@ -267,54 +311,122 @@ describe("GET /api/model-filters", () => { }); }); - describe("scopeToCustodian", () => { + describe("booking visibility (standard rule)", () => { beforeEach(() => { dbMocks.dynamicFindMany.mockResolvedValue([]); - bookingMocks.resolveCustodianScope.mockResolvedValue({ - userId: "user-1", - teamMemberIds: ["tm-1"], - }); }); it.each([OrganizationRoles.SELF_SERVICE, OrganizationRoles.BASE])( - "restricts %s callers that opt in to bookings they are custodian of", + "restricts %s users to their own bookings when the setting is off", async (role) => { - setRole(role); + setRole(role, false); - await loader( - buildArgs("name=booking&queryKey=name&scopeToCustodian=true") - ); + await loader(buildArgs("name=booking&queryKey=name&queryValue=x")); expect(lastWhere().AND).toEqual([ DRAFT_VISIBILITY, - { - OR: [ - { custodianUserId: "user-1" }, - { custodianTeamMemberId: { in: ["tm-1"] } }, - ], - }, + CUSTODIAN_SCOPE, + WRITE_SCOPE, ]); } ); - it("is a no-op for admins and owners", async () => { - setRole(OrganizationRoles.ADMIN); + it.each([OrganizationRoles.SELF_SERVICE, OrganizationRoles.BASE])( + "matches custody on EITHER link, as the seeding loader does", + async (role) => { + setRole(role, false); + + await loader(buildArgs("name=booking&queryKey=name&queryValue=x")); + + // The regression: matching `custodianUserId` alone dropped bookings + // custodied through a legacy team-member row, so they showed in the + // seeded list and vanished the moment the user typed. + expect(lastWhere().AND).toContainEqual({ + OR: [ + { custodianUserId: "user-1" }, + { custodianTeamMemberId: { in: TEAM_MEMBER_IDS } }, + ], + }); + } + ); + it.each([OrganizationRoles.SELF_SERVICE, OrganizationRoles.BASE])( + "lifts the read restriction for %s when the workspace enables it", + async (role) => { + setRole(role, true); + + await loader(buildArgs("name=booking&queryKey=name&queryValue=x")); + + expect(lastWhere().AND).not.toContainEqual(CUSTODIAN_SCOPE); + expect(bookingMocks.resolveCustodianScope).not.toHaveBeenCalled(); + } + ); + + it.each([OrganizationRoles.ADMIN, OrganizationRoles.OWNER])( + "never restricts %s", + async (role) => { + setRole(role, false); + + await loader(buildArgs("name=booking&queryKey=name&queryValue=x")); + + expect(lastWhere().AND).toEqual([DRAFT_VISIBILITY]); + } + ); + + it("cannot be widened by a request param, whatever the caller sends", async () => { + setRole(OrganizationRoles.SELF_SERVICE, false); + + // `scopeToCustodian` used to be the ONLY thing applying this restriction, + // so omitting it returned every booking row in the workspace. Sending it + // — or its negation — must now make no difference at all. await loader( - buildArgs("name=booking&queryKey=name&scopeToCustodian=true") + buildArgs( + "name=booking&queryKey=name&queryValue=&scopeToCustodian=false&selectedValues=someone-elses-booking" + ) ); - expect(lastWhere().AND).toEqual([DRAFT_VISIBILITY]); - expect(bookingMocks.resolveCustodianScope).not.toHaveBeenCalled(); + expect(lastWhere().AND).toEqual([ + DRAFT_VISIBILITY, + CUSTODIAN_SCOPE, + WRITE_SCOPE, + ]); }); + }); + + describe("booking write scope", () => { + beforeEach(() => { + dbMocks.dynamicFindMany.mockResolvedValue([]); + }); + + it.each([OrganizationRoles.SELF_SERVICE, OrganizationRoles.BASE])( + "keeps %s inside what the submitting action accepts, setting ON", + async (role) => { + setRole(role, true); - it("leaves callers that do not opt in unscoped (advanced filter)", async () => { - setRole(OrganizationRoles.SELF_SERVICE); + await loader(buildArgs("name=booking&queryKey=name&queryValue=x")); + + // Without this the picker offers bookings `validateBookingOwnership` + // then rejects with a 403 — a dead end for the user. + expect(lastWhere().AND).toEqual([DRAFT_VISIBILITY, WRITE_SCOPE]); + } + ); + + it("does not constrain roles that may write to every booking", async () => { + setRole(OrganizationRoles.ADMIN, false); await loader(buildArgs("name=booking&queryKey=name&queryValue=x")); - expect(lastWhere().AND).toEqual([DRAFT_VISIBILITY]); - expect(bookingMocks.resolveCustodianScope).not.toHaveBeenCalled(); + // Asserting on the clause, not on the string "creatorId" — the + // draft-visibility clause legitimately carries that key. + expect(lastWhere().AND).not.toContainEqual(WRITE_SCOPE); + }); + + it("does not leak into non-booking searches", async () => { + setRole(OrganizationRoles.SELF_SERVICE, false); + + await loader(buildArgs("name=kit&queryKey=name&queryValue=x")); + + expect(lastWhere().AND).toBeUndefined(); }); }); });