diff --git a/server/src/nest/packing/packing.controller.ts b/server/src/nest/packing/packing.controller.ts index fc2f92259..a2bbb5f85 100644 --- a/server/src/nest/packing/packing.controller.ts +++ b/server/src/nest/packing/packing.controller.ts @@ -109,21 +109,10 @@ export class PackingController { this.requireEdit(trip, user); // checked arrives as boolean or legacy 0/1 — the service coerces by truthiness. const item = this.packing.createItem(tripId, { name: body.name, category: body.category, checked: body.checked === undefined ? undefined : !!body.checked, is_private: body.is_private, visibility: body.visibility, recipient_ids: body.recipient_ids }, user.id); - this.emitToViewers(tripId, 'packing:created', { item }, item, socketId); + this.packing.emitToViewers(tripId, 'packing:created', { item }, item, socketId); return { item }; } - /** Deliver an item event to exactly the people who can see it (#858): the whole - * room for a Common item, or owner + recipients for a restricted one. */ - private emitToViewers(tripId: string, event: E, payload: TrekWsPayload, item: PackingItemRow, socketId: string | undefined): void { - const viewers = this.packing.viewersOf(item); - if (viewers === null) { - this.packing.broadcast(tripId, event, payload, socketId); - } else { - this.packing.broadcastToViewers(tripId, event, payload, viewers, socketId); - } - } - @Put('reorder') reorder( @CurrentUser() user: User, @@ -163,41 +152,10 @@ export class PackingController { if (isUpdateConflict(updated)) { throw new HttpException({ error: 'conflict', server: updated.server }, 409); } - this.broadcastUpdate(tripId, id, updated as PackingItemRow, !!before?.is_private, socketId); + this.packing.broadcastUpdate(tripId, id, updated as PackingItemRow, !!before?.is_private, socketId); return { item: updated }; } - /** - * Routes a packing-item update over WebSocket so private items (#858) stay - * scoped to their owner across the four public↔private transitions: - * - stays private → owner-only update - * - public→private → drop it from the whole room, re-add for the owner - * - private→public → create for members who lacked it, then update for all - * - stays public → plain update to all - */ - private broadcastUpdate( - tripId: string, - id: string, - item: PackingItemRow, - wasPrivate: boolean, - socketId: string | undefined, - ): void { - const nowPrivate = !!item.is_private; - if (nowPrivate) { - if (wasPrivate) { - this.packing.broadcastItem(tripId, 'packing:updated', { item }, item, socketId); - } else { - this.packing.broadcast(tripId, 'packing:deleted', { itemId: Number(id) }, socketId); - this.packing.broadcastItem(tripId, 'packing:created', { item }, item, socketId); - } - } else { - if (wasPrivate) { - this.packing.broadcast(tripId, 'packing:created', { item }, socketId); - } - this.packing.broadcast(tripId, 'packing:updated', { item }, socketId); - } - } - @Delete(':id') remove( @CurrentUser() user: User, @@ -212,7 +170,7 @@ export class PackingController { throw new HttpException({ error: 'Item not found' }, 404); } // Scope the delete to the people who could see it (owner + recipients, #858). - this.emitToViewers(tripId, 'packing:deleted', { itemId: Number(id) }, deleted as PackingItemRow, socketId); + this.packing.emitToViewers(tripId, 'packing:deleted', { itemId: Number(id) }, deleted as PackingItemRow, socketId); return { success: true }; } @@ -236,7 +194,7 @@ export class PackingController { // The viewer set just changed: drop the item from the whole room, then re-add // it for whoever can now see it (owner + recipients, or everyone if Common). this.packing.broadcast(tripId, 'packing:deleted', { itemId: Number(id) }, socketId); - this.emitToViewers(tripId, 'packing:created', { item: updated }, updated as PackingItemRow, socketId); + this.packing.emitToViewers(tripId, 'packing:created', { item: updated }, updated as PackingItemRow, socketId); return { item: updated }; } @@ -255,7 +213,7 @@ export class PackingController { throw new HttpException({ error: 'Item not found' }, 404); } // The clone is personal to the caller — only their sockets need it. - this.emitToViewers(tripId, 'packing:created', { item }, item, socketId); + this.packing.emitToViewers(tripId, 'packing:created', { item }, item, socketId); return { item }; } diff --git a/server/src/nest/packing/packing.module.ts b/server/src/nest/packing/packing.module.ts index 75b3cad90..324a677ab 100644 --- a/server/src/nest/packing/packing.module.ts +++ b/server/src/nest/packing/packing.module.ts @@ -2,6 +2,9 @@ import { Module } from '@nestjs/common'; import { PackingController } from './packing.controller'; import { PackingMcp } from './packing.mcp'; import { PackingService } from './packing.service'; +import { PackingRpc } from './packing.rpc'; +import { PluginGuardsModule } from '../plugins/host/plugin-guards.module'; +import { RealtimeModule } from '../realtime/realtime.module'; import { PermissionsModule } from '../permissions/permissions.module'; import { AuthModule } from '../auth/auth.module'; @@ -9,9 +12,9 @@ import { AuthModule } from '../auth/auth.module'; * Exports PackingService for in-container consumers (TripsService bundle, * PluginHostDepsFactory). */ @Module({ - imports: [PermissionsModule, AuthModule], + imports: [PermissionsModule, AuthModule, RealtimeModule, PluginGuardsModule], controllers: [PackingController], - providers: [PackingService, PackingMcp], + providers: [PackingService, PackingMcp, PackingRpc], exports: [PackingService], }) export class PackingModule {} diff --git a/server/src/nest/packing/packing.rpc.ts b/server/src/nest/packing/packing.rpc.ts new file mode 100644 index 000000000..fdd71d6e2 --- /dev/null +++ b/server/src/nest/packing/packing.rpc.ts @@ -0,0 +1,144 @@ +import { packingCreateItemRequestSchema, packingUpdateItemRequestSchema } from '@trek/shared'; +import { PluginController, PluginMethod } from '../plugins/host/rpc-kit/decorators'; +import { PluginGuards } from '../plugins/host/plugin-guards.service'; +import { BadParams, ForbiddenResource } from '../plugins/host/rpc-errors'; +import { asPayload, num } from '../plugins/host/rpc-params'; +import type { PluginRpcContext } from '../plugins/host/rpc-kit/types'; +import { RealtimeService } from '../realtime/realtime.service'; +import { PackingService } from './packing.service'; +import { isUpdateConflict } from '../common/conflictResult'; + +/** Packing rides on the app's own 'packing_edit' permission, exactly like the REST path. */ +const PACKING_EDIT_ACTION = 'packing_edit'; + +type PrivacyItem = { is_private?: number; owner_id?: number | null; recipients?: { user_id: number }[] }; + +/** + * The packing surface a plugin may reach (#plugins). + * + * The privacy-scoped broadcasts (#858) go through PackingService, the same methods + * the REST controller calls. They used to be duplicated inside the plugin deps + * factory, with a comment asking for both copies to be kept in lockstep by hand; + * that copy is gone. Getting this wrong leaks a private item to the whole trip room, + * which is why `wasPrivate` is read BEFORE the write and why the four transitions + * live in exactly one place. + * + * Bags carry no privacy, so they broadcast to the room unfiltered. + */ +@PluginController() +export class PackingRpc { + constructor( + private readonly packing: PackingService, + private readonly realtime: RealtimeService, + private readonly guards: PluginGuards, + ) {} + + @PluginMethod('packing.list', { permission: 'db:read:packing' }) + list(params: Record, ctx: PluginRpcContext): unknown { + // Scoped to the acting user so the #858 visibility filter applies: a plugin must + // not see another member's private items. + return this.guards.tripRead(params, ctx, (userId) => this.packing.listItems(num(params.tripId, 'tripId'), userId)); + } + + @PluginMethod('packing.create', { permission: 'db:write:packing' }) + create(params: Record, ctx: PluginRpcContext): unknown { + const tripId = num(params.tripId, 'tripId'); + const actor = this.guards.requireActor(ctx, 'packing item'); + const parsed = packingCreateItemRequestSchema.safeParse(params.input); + if (!parsed.success) throw new BadParams(`invalid packing item: ${parsed.error.issues[0]?.message ?? 'bad input'}`); + this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); + const item = this.packing.createItem(String(tripId), parsed.data as never, actor) as PrivacyItem; + this.packing.emitToViewers(String(tripId), 'packing:created', { item }, item, undefined); + return item; + } + + @PluginMethod('packing.update', { permission: 'db:write:packing' }) + update(params: Record, ctx: PluginRpcContext): unknown { + const tripId = num(params.tripId, 'tripId'); + const itemId = num(params.itemId, 'itemId'); + const actor = this.guards.requireActor(ctx, 'packing item'); + const parsed = packingUpdateItemRequestSchema.safeParse(params.input); + if (!parsed.success) throw new BadParams(`invalid packing item: ${parsed.error.issues[0]?.message ?? 'bad input'}`); + this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); + // Read the privacy BEFORE the write, so a public/private toggle routes correctly. + const before = this.packing.getItemPrivacy(tripId, itemId); + const input = parsed.data as Record; + const updated = this.packing.updateItem(String(tripId), String(itemId), input as never, Object.keys(input), undefined, actor); + if (!updated) throw new ForbiddenResource(`no packing item ${itemId} on trip ${tripId}`); + if (isUpdateConflict(updated)) throw new BadParams('packing item was modified concurrently'); + this.packing.broadcastUpdate(String(tripId), itemId, updated as PrivacyItem, !!before?.is_private, undefined); + return updated; + } + + @PluginMethod('packing.delete', { permission: 'db:write:packing' }) + delete(params: Record, ctx: PluginRpcContext): unknown { + const tripId = num(params.tripId, 'tripId'); + const itemId = num(params.itemId, 'itemId'); + const actor = this.guards.requireActor(ctx, 'packing item'); + this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); + const deleted = this.packing.deleteItem(String(tripId), String(itemId)) as PrivacyItem | null; + if (!deleted) throw new ForbiddenResource(`no packing item ${itemId} on trip ${tripId}`); + this.packing.emitToViewers(String(tripId), 'packing:deleted', { itemId }, deleted, undefined); + return { deleted: true }; + } + + @PluginMethod('packing.listBags', { permission: 'db:write:packing' }) + listBags(params: Record, ctx: PluginRpcContext): unknown[] { + // Note the permission: the envelope really does gate this READ on the write + // grant. The decorator makes the oddity visible instead of burying it. + return this.guards.tripRead(params, ctx, () => this.packing.listBags(String(num(params.tripId, 'tripId'))) as unknown[]); + } + + @PluginMethod('packing.createBag', { permission: 'db:write:packing' }) + createBag(params: Record, ctx: PluginRpcContext): unknown { + const tripId = num(params.tripId, 'tripId'); + const actor = this.guards.requireActor(ctx, 'packing bag'); + const input = asPayload(params.input); + if (typeof input.name !== 'string' || input.name.trim() === '') throw new BadParams('bag name is required'); + this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); + const bag = this.packing.createBag(String(tripId), { name: input.name, color: typeof input.color === 'string' ? input.color : undefined }); + this.realtime.broadcast(tripId, 'packing:bag-created', { bag }, undefined); + return bag; + } + + @PluginMethod('packing.updateBag', { permission: 'db:write:packing' }) + updateBag(params: Record, ctx: PluginRpcContext): unknown { + const tripId = num(params.tripId, 'tripId'); + const bagId = num(params.bagId, 'bagId'); + const actor = this.guards.requireActor(ctx, 'packing bag'); + this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); + const input = asPayload(params.input); + const bag = this.packing.updateBag(String(tripId), String(bagId), input as never, Object.keys(input)); + if (!bag) throw new ForbiddenResource(`no packing bag ${bagId} on trip ${tripId}`); + this.realtime.broadcast(tripId, 'packing:bag-updated', { bag }, undefined); + return bag; + } + + @PluginMethod('packing.deleteBag', { permission: 'db:write:packing' }) + deleteBag(params: Record, ctx: PluginRpcContext): unknown { + const tripId = num(params.tripId, 'tripId'); + const bagId = num(params.bagId, 'bagId'); + const actor = this.guards.requireActor(ctx, 'packing bag'); + this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); + if (!this.packing.deleteBag(String(tripId), String(bagId))) { + throw new ForbiddenResource(`no packing bag ${bagId} on trip ${tripId}`); + } + this.realtime.broadcast(tripId, 'packing:bag-deleted', { bagId }, undefined); + return { deleted: true }; + } + + @PluginMethod('packing.setBagMembers', { permission: 'db:write:packing' }) + setBagMembers(params: Record, ctx: PluginRpcContext): unknown { + const tripId = num(params.tripId, 'tripId'); + const bagId = num(params.bagId, 'bagId'); + const actor = this.guards.requireActor(ctx, 'packing bag'); + this.guards.requireTripEdit(tripId, actor, PACKING_EDIT_ACTION); + // userIds sits on the params object itself, not under `input`. + const raw = asPayload(params).userIds; + const userIds = Array.isArray(raw) ? raw.filter((x): x is number => typeof x === 'number') : []; + const members = this.packing.setBagMembers(String(tripId), String(bagId), userIds); + if (!members) throw new ForbiddenResource(`no packing bag ${bagId} on trip ${tripId}`); + this.realtime.broadcast(tripId, 'packing:bag-members-updated', { bagId, members }, undefined); + return members; + } +} diff --git a/server/src/nest/packing/packing.service.ts b/server/src/nest/packing/packing.service.ts index bfc590425..9f0fe1b2e 100644 --- a/server/src/nest/packing/packing.service.ts +++ b/server/src/nest/packing/packing.service.ts @@ -84,6 +84,49 @@ export class PackingService { return ids; } + /** Deliver an item event to exactly the people who can see it (#858): the whole + * room for a Common item, or owner + recipients for a restricted one. */ + emitToViewers(tripId: string, event: E, payload: TrekWsPayload, item: PrivacyFields | null | undefined, socketId: string | undefined): void { + const viewers = this.viewersOf(item); + if (viewers === null) { + this.broadcast(tripId, event, payload, socketId); + } else { + this.broadcastToViewers(tripId, event, payload, viewers, socketId); + } + } + + /** + * The four public/private transitions after an update (#858). `wasPrivate` must be + * read BEFORE the write: getting it wrong leaks a freshly-privatized item to the + * whole room. + * + * - private -> private: owner-only update + * - public -> private: drop it from the room, then re-add it for the owner + * - private -> public: add it for the members who did not have it, then update + * - public -> public: a plain update to everyone + * + * Both the REST controller and the plugin RPC handler call this. It used to exist + * three times over (here, in the controller, and as a standalone copy inside the + * plugin deps factory), with a comment asking for all of them to be kept in + * lockstep by hand. + */ + broadcastUpdate(tripId: string, id: string | number, item: PrivacyFields, wasPrivate: boolean, socketId: string | undefined): void { + const nowPrivate = !!item.is_private; + if (nowPrivate) { + if (wasPrivate) { + this.broadcastItem(tripId, 'packing:updated', { item } as TrekWsPayload<'packing:updated'>, item, socketId); + } else { + this.broadcast(tripId, 'packing:deleted', { itemId: Number(id) }, socketId); + this.broadcastItem(tripId, 'packing:created', { item } as TrekWsPayload<'packing:created'>, item, socketId); + } + } else { + if (wasPrivate) { + this.broadcast(tripId, 'packing:created', { item } as TrekWsPayload<'packing:created'>, socketId); + } + this.broadcast(tripId, 'packing:updated', { item } as TrekWsPayload<'packing:updated'>, socketId); + } + } + // ── Items ────────────────────────────────────────────────────────────────── /** diff --git a/server/src/nest/plugins/host/plugin-host-deps.factory.ts b/server/src/nest/plugins/host/plugin-host-deps.factory.ts index cc9ba3f7f..a1a538ecf 100644 --- a/server/src/nest/plugins/host/plugin-host-deps.factory.ts +++ b/server/src/nest/plugins/host/plugin-host-deps.factory.ts @@ -1,9 +1,6 @@ import { Injectable } from '@nestjs/common'; -import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { readEnv } from '../../../app-config'; import { RealtimeService } from '../../realtime/realtime.service'; -import { isUpdateConflict } from '../../common/conflictResult'; -import { getWeather } from '../../weather/weather.impl'; import { BLOCKED_EXTENSIONS, filesDir } from '../../files/files.constants'; import { TripMembershipService } from '../../trip-membership/trip-membership.service'; import { NotificationsService } from '../../notifications/notifications.service'; @@ -64,54 +61,10 @@ function mirrorJourneys(run: () => void): void { try { run(); } catch { /* non-fatal */ } } -// --- #858 packing privacy: viewer-scoped fan-out mirrored from -// packing.controller.broadcastUpdate/emitToViewers and PackingService's -// broadcastItem/viewersOf/broadcastToViewers. A private item's events reach ONLY -// its owner (+ recipients for a shared item), never the whole trip room; passing the -// wrong onlyUserId — or forgetting to drop a freshly-privatized item from the room — -// leaks it. Kept as a local copy (rather than delegating to the injected -// PackingService) so the factory's unit test can assert the fan-out through the -// websocket mock while PackingService stays a plain data stub. Keep in lockstep -// with packing.controller.ts + packing.service.ts. --- -type PackingPrivacy = { is_private?: number; owner_id?: number | null; recipients?: { user_id: number }[] }; - -function packingViewersOf(item: PackingPrivacy | null | undefined): number[] | null { - if (!item || !item.is_private) return null; // Common — visible to the whole room - return [item.owner_id, ...(item.recipients || []).map((r) => r.user_id)].filter((x): x is number => x != null); -} - -/** CREATE/DELETE fan-out: whole room for a Common item, else owner + recipients only. */ -function emitPackingToViewers(realtime: RealtimeService, tripId: number, event: E, payload: TrekWsPayload, item: PackingPrivacy): void { - const viewers = packingViewersOf(item); - if (viewers === null) { - realtime.broadcast(tripId, event, payload, undefined); - return; - } - for (const uid of new Set(viewers)) if (uid != null) realtime.broadcast(tripId, event, payload, undefined, uid); -} - -/** An item event delivered owner-only when the item is private (else to the room). */ -function broadcastPackingItem(realtime: RealtimeService, tripId: number, event: E, payload: TrekWsPayload, item: PackingPrivacy): void { - const onlyUserId = item?.is_private && item.owner_id != null ? item.owner_id : undefined; - realtime.broadcast(tripId, event, payload, undefined, onlyUserId); -} - -/** The four public<->private transitions (packing.controller.broadcastUpdate). `wasPrivate` - * is read BEFORE the write — getting this wrong LEAKS a freshly-privatized item. */ -function broadcastPackingUpdate(realtime: RealtimeService, tripId: number, itemId: number, item: PackingPrivacy, wasPrivate: boolean): void { - const nowPrivate = !!item.is_private; - if (nowPrivate) { - if (wasPrivate) { - broadcastPackingItem(realtime, tripId, 'packing:updated', { item }, item); // stays private -> owner-only - } else { - realtime.broadcast(tripId, 'packing:deleted', { itemId }, undefined); // public->private: drop from the room... - broadcastPackingItem(realtime, tripId, 'packing:created', { item }, item); // ...then re-add for the owner - } - } else { - if (wasPrivate) realtime.broadcast(tripId, 'packing:created', { item }, undefined); // private->public: add for members who lacked it - realtime.broadcast(tripId, 'packing:updated', { item }, undefined); - } -} +// The #858 privacy-scoped packing broadcasts used to be copied here, with a comment +// asking for the copy to be kept in lockstep with packing.controller.ts and +// packing.service.ts by hand. They now live once, in PackingService, and both the +// REST controller and PackingRpc call them there. // Quotas for plugin entity metadata (db:meta) — a cheap disk-DoS guard on the // shared trek.db volume. Generous for real use, small enough to bound abuse. @@ -243,7 +196,6 @@ export class PluginHostDepsFactory { }, // --- Read scopes (packing/files). Membership is checked by the host (tripRead); // these just delegate to the same services the REST paths use. --- - listPackingItems: (tripId, userId) => this.packing.listItems(tripId, userId), listTripFiles: (tripId) => this.files.listFiles(tripId, false), // A file's bytes as base64, size-capped BEFORE the read so a 500MB video can't // be pulled (~667MB base64) through the IPC pipe. 10MB matches the plugin upload @@ -788,53 +740,7 @@ export class PluginHostDepsFactory { // --- Packing (packing_edit). Reuses PackingService + replicates the #858 // privacy-scoped broadcasts (create/delete via emitPackingToViewers, update via // the four-case broadcastPackingUpdate) so a private item never leaks room-wide. --- - canEditPacking: (tripId, userId) => this.canEditTripAs('packing_edit', tripId, userId), - createPackingItem: (tripId, input, actingUserId) => { - const i = input as { name: string; category?: string; checked?: boolean; is_private?: boolean; visibility?: 'common' | 'personal' | 'shared'; recipient_ids?: number[] }; - const item = this.packing.createItem(String(tripId), i, actingUserId) as PackingPrivacy; - emitPackingToViewers(this.realtime, tripId, 'packing:created', { item }, item); - return item; - }, - updatePackingItem: (tripId, itemId, input, actingUserId) => { - // Privacy BEFORE the write, so a public<->private toggle routes correctly. - const before = this.packing.getItemPrivacy(tripId, itemId); - const updated = this.packing.updateItem(String(tripId), String(itemId), input as never, Object.keys(input), undefined, actingUserId); - if (!updated) throw new ForbiddenResource(`no packing item ${itemId} on trip ${tripId}`); - if (isUpdateConflict(updated)) throw new BadParams('packing item was modified concurrently'); - broadcastPackingUpdate(this.realtime, tripId, itemId, updated as PackingPrivacy, !!before?.is_private); - return updated; - }, - deletePackingItem: (tripId, itemId) => { - const deleted = this.packing.deleteItem(String(tripId), String(itemId)) as PackingPrivacy | null; - if (!deleted) throw new ForbiddenResource(`no packing item ${itemId} on trip ${tripId}`); - emitPackingToViewers(this.realtime, tripId, 'packing:deleted', { itemId }, deleted); - return { deleted: true }; - }, // --- Packing bags (no privacy — plain room broadcasts). --- - listPackingBags: (tripId) => this.packing.listBags(String(tripId)) as unknown[], - createPackingBag: (tripId, input) => { - const i = input as { name: string; color?: string }; - const bag = this.packing.createBag(String(tripId), { name: i.name, color: i.color }); - this.realtime.broadcast(tripId, 'packing:bag-created', { bag }, undefined); - return bag; - }, - updatePackingBag: (tripId, bagId, input) => { - const bag = this.packing.updateBag(String(tripId), String(bagId), input as never, Object.keys(input)); - if (!bag) throw new ForbiddenResource(`no packing bag ${bagId} on trip ${tripId}`); - this.realtime.broadcast(tripId, 'packing:bag-updated', { bag }, undefined); - return bag; - }, - deletePackingBag: (tripId, bagId) => { - if (!this.packing.deleteBag(String(tripId), String(bagId))) throw new ForbiddenResource(`no packing bag ${bagId} on trip ${tripId}`); - this.realtime.broadcast(tripId, 'packing:bag-deleted', { bagId }, undefined); - return { deleted: true }; - }, - setPackingBagMembers: (tripId, bagId, userIds) => { - const members = this.packing.setBagMembers(String(tripId), String(bagId), userIds); - if (!members) throw new ForbiddenResource(`no packing bag ${bagId} on trip ${tripId}`); - this.realtime.broadcast(tripId, 'packing:bag-members-updated', { bagId, members }, undefined); - return members; - }, // --- Read-convenience: weather (host cache, tenant-free), categories (global), roster --- tripMembers: (tripId) => this.db.prepare('SELECT u.id, u.username, u.display_name, u.avatar FROM trip_members tm JOIN users u ON u.id = tm.user_id WHERE tm.trip_id = ?').all(tripId) as unknown[], diff --git a/server/src/nest/plugins/host/rpc-host.ts b/server/src/nest/plugins/host/rpc-host.ts index 193ab5700..0c15c09b6 100644 --- a/server/src/nest/plugins/host/rpc-host.ts +++ b/server/src/nest/plugins/host/rpc-host.ts @@ -91,7 +91,6 @@ export interface HostDeps { /** True if the acting user may create costs on the trip (the 'budget_edit' permission). */ canEditCosts(tripId: number, userId: number): boolean; /** A trip's packing items visible to `userId` (#858 private-item filter), for `packing.list`. */ - listPackingItems(tripId: number, userId: number): unknown[]; /** A trip's files (trash excluded), for `files.list`. */ listTripFiles(tripId: number): unknown[]; /** One trip file's bytes as base64 (size-capped), for `files.getContent`. Throws if it's not on the trip or exceeds the cap. */ @@ -222,19 +221,10 @@ export interface HostDeps { /** Delete a reservation from a trip (same side effects); returns { deleted: true }. */ deleteReservation(tripId: number, reservationId: number, actingUserId: number): unknown; // --- Packing (the 'packing_edit' permission; #858 privacy-scoped broadcasts) --- - canEditPacking(tripId: number, userId: number): boolean; /** Create a packing item (owner = acting user); privacy-scoped packing:created broadcast; returns it. */ - createPackingItem(tripId: number, input: Record, actingUserId: number): unknown; /** Update a packing item; four-case public<->private broadcast; throws if not on the trip. */ - updatePackingItem(tripId: number, itemId: number, input: Record, actingUserId: number): unknown; /** Delete a packing item; owner+recipients-scoped packing:deleted broadcast; returns { deleted: true }. */ - deletePackingItem(tripId: number, itemId: number): unknown; // --- Packing bags (packing_edit; no privacy — broadcast to the whole room) --- - listPackingBags(tripId: number): unknown[]; - createPackingBag(tripId: number, input: Record): unknown; - updatePackingBag(tripId: number, bagId: number, input: Record): unknown; - deletePackingBag(tripId: number, bagId: number): unknown; - setPackingBagMembers(tripId: number, bagId: number, userIds: number[]): unknown; // --- Read-convenience: weather (tenant-free), categories (global), the trip roster --- tripMembers(tripId: number): unknown[]; // --- Tags (the acting user's own; no trip) --- @@ -325,13 +315,6 @@ export class PluginRpcHost { // The trip's member roster (ids + display fields only), membership-checked. this.methods.set('trips.members', (p, uid) => this.tripRead(p, uid, () => deps.tripMembers(num(p.tripId, 'tripId')))); } - if (has('db:read:packing')) { - // Delegate to the packing service, scoped to the acting user so its #858 private- - // item visibility filter applies (a plugin must not see other members' private items). - this.methods.set('packing.list', (p, uid) => - this.tripRead(p, uid, (userId) => deps.listPackingItems(num(p.tripId, 'tripId'), userId)), - ); - } if (has('db:read:files')) { // Trip files, trash excluded — same view the files tab shows. this.methods.set('files.list', (p, uid) => @@ -823,69 +806,7 @@ export class PluginRpcHost { }); } - if (has('db:write:packing')) { - // Packing list write. Gated exactly like the packing REST path — trip access + - // the 'packing_edit' permission for the HOST-bound acting user. The deps reuse - // packingService and replicate the #858 privacy-scoped broadcasts 1:1, so a - // private item is never leaked to the whole trip room. - this.methods.set('packing.create', (p, uid) => { - const tripId = num(p.tripId, 'tripId'); - const actor = this.requireActor(uid, 'packing item'); - const parsed = packingCreateItemRequestSchema.safeParse(p.input); - if (!parsed.success) throw new BadParams(`invalid packing item: ${parsed.error.issues[0]?.message ?? 'bad input'}`); - this.requireTripEdit(tripId, actor, deps.canEditPacking); - return deps.createPackingItem(tripId, parsed.data as Record, actor); - }); - this.methods.set('packing.update', (p, uid) => { - const tripId = num(p.tripId, 'tripId'); - const itemId = num(p.itemId, 'itemId'); - const actor = this.requireActor(uid, 'packing item'); - const parsed = packingUpdateItemRequestSchema.safeParse(p.input); - if (!parsed.success) throw new BadParams(`invalid packing item: ${parsed.error.issues[0]?.message ?? 'bad input'}`); - this.requireTripEdit(tripId, actor, deps.canEditPacking); - return deps.updatePackingItem(tripId, itemId, parsed.data as Record, actor); - }); - this.methods.set('packing.delete', (p, uid) => { - const tripId = num(p.tripId, 'tripId'); - const itemId = num(p.itemId, 'itemId'); - const actor = this.requireActor(uid, 'packing item'); - this.requireTripEdit(tripId, actor, deps.canEditPacking); - return deps.deletePackingItem(tripId, itemId); - }); - // Bags carry no privacy — a plain packing:bag-* broadcast to the whole room. - this.methods.set('packing.listBags', (p, uid) => this.tripRead(p, uid, () => deps.listPackingBags(num(p.tripId, 'tripId')))); - this.methods.set('packing.createBag', (p, uid) => { - const tripId = num(p.tripId, 'tripId'); - const actor = this.requireActor(uid, 'packing bag'); - const input = asPayload(p.input); - if (typeof input.name !== 'string' || input.name.trim() === '') throw new BadParams('bag name is required'); - this.requireTripEdit(tripId, actor, deps.canEditPacking); - return deps.createPackingBag(tripId, input); - }); - this.methods.set('packing.updateBag', (p, uid) => { - const tripId = num(p.tripId, 'tripId'); - const bagId = num(p.bagId, 'bagId'); - const actor = this.requireActor(uid, 'packing bag'); - this.requireTripEdit(tripId, actor, deps.canEditPacking); - return deps.updatePackingBag(tripId, bagId, asPayload(p.input)); - }); - this.methods.set('packing.deleteBag', (p, uid) => { - const tripId = num(p.tripId, 'tripId'); - const bagId = num(p.bagId, 'bagId'); - const actor = this.requireActor(uid, 'packing bag'); - this.requireTripEdit(tripId, actor, deps.canEditPacking); - return deps.deletePackingBag(tripId, bagId); - }); - this.methods.set('packing.setBagMembers', (p, uid) => { - const tripId = num(p.tripId, 'tripId'); - const bagId = num(p.bagId, 'bagId'); - const actor = this.requireActor(uid, 'packing bag'); - this.requireTripEdit(tripId, actor, deps.canEditPacking); - const raw = asPayload(p).userIds; - const userIds = Array.isArray(raw) ? raw.filter((x): x is number => typeof x === 'number') : []; - return deps.setPackingBagMembers(tripId, bagId, userIds); - }); - } + // packing.* now lives in src/nest/packing/packing.rpc.ts. // weather.get and categories.list now live in their own domains' *.rpc.ts. // tags.* now lives in src/nest/tags/tags.rpc.ts, bound through the registry below. diff --git a/server/tests/helpers/rpc-host-deps.ts b/server/tests/helpers/rpc-host-deps.ts index 4b2eaf9f1..cc2eed9be 100644 --- a/server/tests/helpers/rpc-host-deps.ts +++ b/server/tests/helpers/rpc-host-deps.ts @@ -6,6 +6,7 @@ import { WeatherRpc } from '../../src/nest/weather/weather.rpc'; import { ExchangeRatesRpc } from '../../src/nest/budget/exchange-rates.rpc'; import { TodoRpc } from '../../src/nest/todo/todo.rpc'; import { DayNotesRpc } from '../../src/nest/days/day-notes.rpc'; +import { PackingRpc } from '../../src/nest/packing/packing.rpc'; /** * The stubbed HostDeps every plugin router test builds on. It used to live inside @@ -186,5 +187,6 @@ export function allRpcControllers(): object[] { new ExchangeRatesRpc(anyService()), new TodoRpc(anyService(), anyService(), anyService()), new DayNotesRpc(anyService(), anyService(), anyService()), + new PackingRpc(anyService(), anyService(), anyService()), ]; } diff --git a/server/tests/unit/nest/packing.controller.test.ts b/server/tests/unit/nest/packing.controller.test.ts index 0b4f32716..56c34a75d 100644 --- a/server/tests/unit/nest/packing.controller.test.ts +++ b/server/tests/unit/nest/packing.controller.test.ts @@ -10,7 +10,7 @@ const trip = { id: 5, user_id: 1 }; /** Service mock with trip access granted + edit allowed by default. */ function makeService(overrides: Partial = {}): PackingService { - return { + const base = { verifyTripAccess: vi.fn().mockReturnValue(trip), canEdit: vi.fn().mockReturnValue(true), broadcast: vi.fn(), @@ -23,6 +23,30 @@ function makeService(overrides: Partial = {}): PackingService { notifyTagged: vi.fn(), ...overrides, } as unknown as PackingService; + // emitToViewers and broadcastUpdate moved from the controller into the service, so + // the mock reproduces their routing on top of whatever broadcast stubs a test + // supplied. That keeps every #858 assertion below pointed at the same three + // primitives it always was. + const svc = base as unknown as PackingService & Record unknown>; + svc.emitToViewers = ((tripId, event, payload, item, socketId) => { + const viewers = svc.viewersOf(item); + if (viewers === null) svc.broadcast(tripId, event, payload, socketId); + else svc.broadcastToViewers(tripId, event, payload, viewers, socketId); + }) as PackingService['emitToViewers']; + svc.broadcastUpdate = ((tripId, id, item, wasPrivate, socketId) => { + if (item.is_private) { + if (wasPrivate) { + svc.broadcastItem(tripId, 'packing:updated', { item }, item, socketId); + } else { + svc.broadcast(tripId, 'packing:deleted', { itemId: Number(id) }, socketId); + svc.broadcastItem(tripId, 'packing:created', { item }, item, socketId); + } + } else { + if (wasPrivate) svc.broadcast(tripId, 'packing:created', { item }, socketId); + svc.broadcast(tripId, 'packing:updated', { item }, socketId); + } + }) as PackingService['broadcastUpdate']; + return svc; } function thrown(fn: () => unknown): { status: number; body: unknown } { diff --git a/server/tests/unit/packing/packing.rpc.test.ts b/server/tests/unit/packing/packing.rpc.test.ts new file mode 100644 index 000000000..01a23abb4 --- /dev/null +++ b/server/tests/unit/packing/packing.rpc.test.ts @@ -0,0 +1,203 @@ +/** + * The packing plugin surface after it moved onto @PluginMethod. + * + * The four public/private transitions (#858) are the reason this file is long. Each + * one is asserted separately, because getting any of them wrong leaks a private item + * to the whole trip room, and because the logic now lives once in PackingService + * instead of three times over. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { PluginRpcHost } from '../../../src/nest/plugins/host/rpc-host'; +import { createTestPluginRegistry } from '../../../src/nest/plugins/host/rpc-kit/testing'; +import { PluginGuards } from '../../../src/nest/plugins/host/plugin-guards.service'; +import { PackingRpc } from '../../../src/nest/packing/packing.rpc'; +import { PackingModule } from '../../../src/nest/packing/packing.module'; +import { PackingService } from '../../../src/nest/packing/packing.service'; +import type { RealtimeService } from '../../../src/nest/realtime/realtime.service'; +import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { PermissionsService } from '../../../src/nest/permissions/permissions.service'; +import type { AddonsService } from '../../../src/nest/addons/addons.service'; +import type { RpcRequest, RpcError } from '../../../src/nest/plugins/protocol/envelope'; +import { makeDeps } from '../../helpers/rpc-host-deps'; + +const req = (method: string, params: Record = {}): RpcRequest => ({ k: 'req', id: 'x', method, params }); + +type Item = { id: number; name?: string; is_private?: number; owner_id?: number | null; recipients?: { user_id: number }[] }; + +/** + * A REAL PackingService over a fake realtime, so the broadcast fan-out under test is + * the production one; only the data methods are stubbed. + */ +function build(opts: { canEdit?: boolean; before?: Item | undefined; updated?: Item | null } = {}) { + const realtime = { broadcast: vi.fn() } as unknown as RealtimeService & { broadcast: ReturnType }; + const packing = new PackingService({} as never, {} as never, realtime); + const data = { + listItems: vi.fn(() => [{ id: 70, name: 'Socks' }]), + createItem: vi.fn((_t: string, i: Record) => ({ id: 70, ...i })), + getItemPrivacy: vi.fn(() => opts.before), + updateItem: vi.fn(() => (opts.updated === undefined ? { id: 70, is_private: 0 } : opts.updated)), + deleteItem: vi.fn((_t: string, id: string) => (id === '70' ? { id: 70, is_private: 0 } : null)), + listBags: vi.fn(() => [{ id: 80, name: 'Backpack' }]), + createBag: vi.fn((_t: string, b: Record) => ({ id: 80, ...b })), + updateBag: vi.fn((_t: string, id: string) => (id === '80' ? { id: 80 } : null)), + deleteBag: vi.fn((_t: string, id: string) => id === '80'), + setBagMembers: vi.fn((_t: string, id: string, ids: number[]) => (id === '80' ? { bagId: 80, members: ids } : null)), + }; + Object.assign(packing, data); + const guards = new PluginGuards( + { + canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)), + prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })), + } as unknown as DatabaseService, + { checkPermission: vi.fn(() => opts.canEdit ?? true) } as unknown as PermissionsService, + { isAddonEnabled: vi.fn(() => true) } as unknown as AddonsService, + ); + const rpc = new PackingRpc(packing, realtime, guards); + const host = (...grants: string[]) => new PluginRpcHost('p', new Set(grants), makeDeps(), createTestPluginRegistry([rpc])); + return { packing, data, realtime, rpc, host }; +} + +/** Every (event, onlyUserId) pair the fan-out produced, in order. */ +const fanout = (realtime: { broadcast: ReturnType }) => + realtime.broadcast.mock.calls.map((c) => [c[1], c[4]]); + +describe('PackingRpc through the router', () => { + it('PACKING-RPC-001 packing.list is membership-checked and scoped to the acting user', async () => { + const f = build(); + const host = f.host('db:read:packing'); + expect((await host.dispatch(req('packing.list', { tripId: 1 }), 42)).ok).toBe(true); + // The user id is handed through so the #858 visibility filter applies. + expect(f.data.listItems).toHaveBeenCalledWith(1, 42); + expect(((await host.dispatch(req('packing.list', { tripId: 2 }), 42)) as RpcError).error.code).toBe('RESOURCE_FORBIDDEN'); + }); + + it('PACKING-RPC-002 create needs the write grant, the edit right and a bound user', async () => { + const f = build(); + const host = f.host('db:write:packing'); + expect((await host.dispatch(req('packing.create', { tripId: 1, input: { name: 'Socks' } }), 42)).ok).toBe(true); + expect((await host.dispatch(req('packing.create', { tripId: 1, input: { name: '' } }), 42)).ok).toBe(false); + expect(((await host.dispatch(req('packing.create', { tripId: 2, input: { name: 'x' } }), 42)) as RpcError).error.code).toBe('RESOURCE_FORBIDDEN'); + const noUser = (await host.dispatch(req('packing.create', { tripId: 1, input: { name: 'x' } }), undefined)) as RpcError; + expect(noUser.error.message).toBe('packing item writes require an authenticated user context'); + }); + + it('PACKING-RPC-003 db:read:packing does not unlock a write', async () => { + const f = build(); + const res = (await f.host('db:read:packing').dispatch(req('packing.create', { tripId: 1, input: { name: 'x' } }), 42)) as RpcError; + expect(res.error.code).toBe('PERMISSION_DENIED'); + }); + + it('PACKING-RPC-004 a missing item is RESOURCE_FORBIDDEN, naming it', async () => { + const f = build({ updated: null }); + const res = (await f.host('db:write:packing').dispatch(req('packing.update', { tripId: 1, itemId: 404, input: { name: 'x' } }), 42)) as RpcError; + expect(res.error.message).toBe('no packing item 404 on trip 1'); + }); + + it('PACKING-RPC-005 bags are gated on the write grant too, listBags included', async () => { + const f = build(); + const host = f.host('db:write:packing'); + expect((await host.dispatch(req('packing.listBags', { tripId: 1 }), 42)).ok).toBe(true); + expect((await host.dispatch(req('packing.createBag', { tripId: 1, input: { name: 'Bag' } }), 42)).ok).toBe(true); + expect((await host.dispatch(req('packing.setBagMembers', { tripId: 1, bagId: 80, userIds: [5, 6] }), 42)).ok).toBe(true); + expect(f.data.setBagMembers).toHaveBeenCalledWith('1', '80', [5, 6]); + }); + + it('PACKING-RPC-006 a bag name is required, and a missing bag is named', async () => { + const f = build(); + const host = f.host('db:write:packing'); + expect(((await host.dispatch(req('packing.createBag', { tripId: 1, input: { name: ' ' } }), 42)) as RpcError).error.message).toBe('bag name is required'); + expect(((await host.dispatch(req('packing.updateBag', { tripId: 1, bagId: 404, input: {} }), 42)) as RpcError).error.message).toBe('no packing bag 404 on trip 1'); + }); + + it('PACKING-RPC-007 non-numeric entries in userIds are dropped', async () => { + const f = build(); + await f.host('db:write:packing').dispatch(req('packing.setBagMembers', { tripId: 1, bagId: 80, userIds: [5, 'six', null, 6] }), 42); + expect(f.data.setBagMembers).toHaveBeenCalledWith('1', '80', [5, 6]); + }); + + it('PACKING-RPC-008 the class is listed in its module providers', () => { + expect(Reflect.getMetadata('providers', PackingModule) as unknown[]).toContain(PackingRpc); + }); +}); + +describe('PackingRpc keeps private items off the room (#858)', () => { + it('PACKING-RPC-009 a common item is created for the whole room', async () => { + const f = build(); + await f.host('db:write:packing').dispatch(req('packing.create', { tripId: 1, input: { name: 'Socks' } }), 42); + expect(fanout(f.realtime)).toEqual([['packing:created', undefined]]); + }); + + it('PACKING-RPC-010 a private item is created only for its owner and recipients', async () => { + const f = build(); + f.data.createItem.mockReturnValueOnce({ id: 70, is_private: 1, owner_id: 42, recipients: [{ user_id: 7 }] }); + await f.host('db:write:packing').dispatch(req('packing.create', { tripId: 1, input: { name: 'Gift', is_private: true } }), 42); + expect(fanout(f.realtime)).toEqual([ + ['packing:created', 42], + ['packing:created', 7], + ]); + }); + + it('PACKING-RPC-011 private stays private: an owner-only update, never a room event', async () => { + const f = build({ before: { id: 70, is_private: 1 }, updated: { id: 70, is_private: 1, owner_id: 42 } }); + await f.host('db:write:packing').dispatch(req('packing.update', { tripId: 1, itemId: 70, input: { name: 'x' } }), 42); + expect(fanout(f.realtime)).toEqual([['packing:updated', 42]]); + }); + + it('PACKING-RPC-012 public to private: dropped from the room FIRST, then re-added for the owner', async () => { + const f = build({ before: { id: 70, is_private: 0 }, updated: { id: 70, is_private: 1, owner_id: 42 } }); + await f.host('db:write:packing').dispatch(req('packing.update', { tripId: 1, itemId: 70, input: { is_private: true } }), 42); + // The order matters: a member who already had the item must lose it before the + // owner gets the private copy. + expect(fanout(f.realtime)).toEqual([ + ['packing:deleted', undefined], + ['packing:created', 42], + ]); + }); + + it('PACKING-RPC-013 private to public: created for the members who lacked it, then updated for all', async () => { + const f = build({ before: { id: 70, is_private: 1 }, updated: { id: 70, is_private: 0 } }); + await f.host('db:write:packing').dispatch(req('packing.update', { tripId: 1, itemId: 70, input: { is_private: false } }), 42); + expect(fanout(f.realtime)).toEqual([ + ['packing:created', undefined], + ['packing:updated', undefined], + ]); + }); + + it('PACKING-RPC-014 public stays public: one plain update to everyone', async () => { + const f = build({ before: { id: 70, is_private: 0 }, updated: { id: 70, is_private: 0 } }); + await f.host('db:write:packing').dispatch(req('packing.update', { tripId: 1, itemId: 70, input: { name: 'x' } }), 42); + expect(fanout(f.realtime)).toEqual([['packing:updated', undefined]]); + }); + + it('PACKING-RPC-015 the privacy is read BEFORE the write, not after', async () => { + const f = build({ before: { id: 70, is_private: 0 }, updated: { id: 70, is_private: 1, owner_id: 42 } }); + await f.host('db:write:packing').dispatch(req('packing.update', { tripId: 1, itemId: 70, input: { is_private: true } }), 42); + // Reading it afterwards would report "already private" and skip the room delete, + // leaving the item on every member's screen. + expect(f.data.getItemPrivacy.mock.invocationCallOrder[0]).toBeLessThan(f.data.updateItem.mock.invocationCallOrder[0]); + }); + + it('PACKING-RPC-016 deleting a private item tells only its viewers', async () => { + const f = build(); + f.data.deleteItem.mockReturnValueOnce({ id: 70, is_private: 1, owner_id: 42, recipients: [{ user_id: 7 }] }); + await f.host('db:write:packing').dispatch(req('packing.delete', { tripId: 1, itemId: 70 }), 42); + expect(fanout(f.realtime)).toEqual([ + ['packing:deleted', 42], + ['packing:deleted', 7], + ]); + }); + + it('PACKING-RPC-016b a stale-write conflict is BAD_PARAMS and broadcasts nothing', async () => { + const f = build({ before: { id: 70, is_private: 0 }, updated: { conflict: true, server: { id: 70 } } as never }); + const res = (await f.host('db:write:packing').dispatch(req('packing.update', { tripId: 1, itemId: 70, input: { name: 'x' } }), 42)) as RpcError; + expect(res.error.code).toBe('BAD_PARAMS'); + expect(res.error.message).toBe('packing item was modified concurrently'); + expect(f.realtime.broadcast).not.toHaveBeenCalled(); + }); + + it('PACKING-RPC-017 a refused write broadcasts nothing at all', async () => { + const f = build({ canEdit: false }); + await f.host('db:write:packing').dispatch(req('packing.create', { tripId: 1, input: { name: 'x' } }), 42); + expect(f.realtime.broadcast).not.toHaveBeenCalled(); + }); +}); diff --git a/server/tests/unit/plugins/plugin-host-deps.factory.test.ts b/server/tests/unit/plugins/plugin-host-deps.factory.test.ts index 420b08e68..d5c01d5ef 100644 --- a/server/tests/unit/plugins/plugin-host-deps.factory.test.ts +++ b/server/tests/unit/plugins/plugin-host-deps.factory.test.ts @@ -585,13 +585,6 @@ describe('host-deps factory — planner write + metadata deps', () => { expect((await call(h, 'costs.delete', { tripId: 1, itemId: 404 })).error.code).toBe('RESOURCE_FORBIDDEN'); }); - it('packing/files read deps delegate to their services (trash excluded for files)', async () => { - const h = host('db:read:packing', 'db:read:files'); - // acting user 5 is threaded to the packing service (#858 private-item filter); _uid proves it - expect((await call(h, 'packing.list', { tripId: 1 })).result).toEqual([{ id: 1, trip_id: 1, name: 'Socks', _uid: 5 }]); - expect((await call(h, 'files.list', { tripId: 1 })).result).toEqual([{ id: 2, trip_id: 1, trash: false }]); - }); - it('users.getById is scoped to people the acting user shares a trip with', async () => { const h = host('db:read:users'); expect((await call(h, 'users.getById', { id: 6 }, 5)).ok).toBe(true); // 5 (owner) + 6 (member) share trip 1 @@ -714,68 +707,9 @@ describe('host-deps factory — reservations, day notes, cross-trip + addon read }); }); -describe('host-deps factory — packing write with #858 privacy-scoped broadcasts', () => { - const host = () => createRealRpcHost('pk', new Set(['db:write:packing'])) - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const call = async (method: string, params: Record, uid = 5): Promise => - host().dispatch({ k: 'req', id: 'x', method, params }, uid) - // the onlyUserId (5th arg) of every broadcast for `event` since the last clear: - // undefined = whole trip room, a number = that user's sockets only. - const fanout = (event: string) => broadcast.mock.calls.filter((c) => c[1] === event).map((c) => c[4]) - beforeEach(() => { checkPermission.mockReset(); checkPermission.mockReturnValue(true); isAddonEnabled.mockReset(); isAddonEnabled.mockReturnValue(true); broadcast.mockClear() }) - afterAll(() => closePluginDataDb('pk')) - - it('create: Common -> whole room; Personal -> owner-only; Shared -> owner + recipients', async () => { - expect((await call('packing.create', { tripId: 1, input: { name: 'Common', visibility: 'common' } })).ok).toBe(true) - expect(fanout('packing:created')).toEqual([undefined]) // whole room - - broadcast.mockClear() - expect((await call('packing.create', { tripId: 1, input: { name: 'Mine', visibility: 'personal' } })).ok).toBe(true) - expect(fanout('packing:created')).toEqual([5]) // owner-only - - broadcast.mockClear() - expect((await call('packing.create', { tripId: 1, input: { name: 'Ours', visibility: 'shared', recipient_ids: [6] } })).ok).toBe(true) - expect([...fanout('packing:created')].sort()).toEqual([5, 6]) // owner + recipient, never the room - }) - - it('update: the four public<->private transitions route correctly (never leaks a privatized item)', async () => { - await call('packing.update', { tripId: 1, itemId: 71, input: { is_private: true } }) // stays private (71 seeded private) - expect(fanout('packing:updated')).toEqual([5]) - expect(fanout('packing:deleted')).toEqual([]) - expect(fanout('packing:created')).toEqual([]) - - broadcast.mockClear() - await call('packing.update', { tripId: 1, itemId: 70, input: { is_private: true } }) // public -> private (70 seeded public) - expect(fanout('packing:deleted')).toEqual([undefined]) // drop from the room FIRST (the anti-leak) - expect(fanout('packing:created')).toEqual([5]) // then re-add owner-only - - broadcast.mockClear() - await call('packing.update', { tripId: 1, itemId: 71, input: { is_private: false } }) // private -> public - expect(fanout('packing:created')).toEqual([undefined]) - expect(fanout('packing:updated')).toEqual([undefined]) - - broadcast.mockClear() - await call('packing.update', { tripId: 1, itemId: 70, input: { is_private: false } }) // stays public - expect(fanout('packing:updated')).toEqual([undefined]) - expect(fanout('packing:deleted')).toEqual([]) - }) - - it('update: a stale-write conflict is BAD_PARAMS and never broadcasts', async () => { - const res = await call('packing.update', { tripId: 1, itemId: 99, input: { name: 'x' } }) - expect((res as { error: { code: string } }).error.code).toBe('BAD_PARAMS') - expect(broadcast).not.toHaveBeenCalled() - }) - - it('delete: a private item is owner-scoped; a missing one is RESOURCE_FORBIDDEN', async () => { - await call('packing.delete', { tripId: 1, itemId: 71 }) - expect(fanout('packing:deleted')).toEqual([5]) // owner-only (recipients get no packing:deleted) - broadcast.mockClear() - await call('packing.delete', { tripId: 1, itemId: 70 }) - expect(fanout('packing:deleted')).toEqual([undefined]) // common -> room - const missing = await call('packing.delete', { tripId: 1, itemId: 404 }) - expect((missing as { error: { code: string } }).error.code).toBe('RESOURCE_FORBIDDEN') - }) -}) +// The packing write deps left this factory with the decorator migration. The #858 +// transitions are asserted in tests/unit/packing/packing.rpc.test.ts, against the +// PackingService methods the REST controller uses too, so there is one copy now. describe('host-deps factory — Wave 1 wiring (weather/categories/tags/todos/roster/bags)', () => { const host = (...perms: string[]) => createRealRpcHost('w1', new Set(perms)) @@ -800,15 +734,6 @@ describe('host-deps factory — Wave 1 wiring (weather/categories/tags/todos/ros // todos.* left this factory with the decorator migration; the cases now run in // tests/unit/todo/todo.rpc.test.ts against TodoRpc. - it('packing bags list/create/update/delete/setMembers run the wiring', async () => { - const h = host('db:write:packing') - expect((await call(h, 'packing.listBags', { tripId: 1 }, 5)).ok).toBe(true) - expect((await call(h, 'packing.createBag', { tripId: 1, input: { name: 'Bag' } }, 5)).ok).toBe(true) - expect((await call(h, 'packing.updateBag', { tripId: 1, bagId: 80, input: { name: 'X' } }, 5)).ok).toBe(true) - expect((await call(h, 'packing.setBagMembers', { tripId: 1, bagId: 80, userIds: [5] }, 5)).ok).toBe(true) - expect((await call(h, 'packing.deleteBag', { tripId: 1, bagId: 80 }, 5)).ok).toBe(true) - expect(((await call(h, 'packing.deleteBag', { tripId: 1, bagId: 404 }, 5)) as { error: { code: string } }).error.code).toBe('RESOURCE_FORBIDDEN') - }) }) describe('host-deps factory — Wave 2 wiring (atlas/vacay/journal/collections writes)', () => { diff --git a/server/tests/unit/plugins/rpc-coverage.test.ts b/server/tests/unit/plugins/rpc-coverage.test.ts index 7c539384d..62b9d4c6f 100644 --- a/server/tests/unit/plugins/rpc-coverage.test.ts +++ b/server/tests/unit/plugins/rpc-coverage.test.ts @@ -54,8 +54,8 @@ describe('plugin RPC coverage ledger', () => { // Update these two numbers in every rollout PR. They make the diff state the size // of the move out loud, which is the number a reviewer wants to see. const total = KNOWN_METHODS.length + UNCONDITIONAL_METHODS.length; - expect(fromRegistry.size).toBe(15); - expect(legacy.size).toBe(total - 15); + expect(fromRegistry.size).toBe(24); + expect(legacy.size).toBe(total - 24); }); it('RPCLEDGER-006 the migrated methods are exactly the ones this PR claims', () => { @@ -65,6 +65,15 @@ describe('plugin RPC coverage ledger', () => { 'daynotes.delete', 'daynotes.list', 'daynotes.update', + 'packing.create', + 'packing.createBag', + 'packing.delete', + 'packing.deleteBag', + 'packing.list', + 'packing.listBags', + 'packing.setBagMembers', + 'packing.update', + 'packing.updateBag', 'rates.get', 'tags.create', 'tags.delete', diff --git a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts index b73011533..dd46b54cb 100644 --- a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts +++ b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts @@ -10,6 +10,11 @@ import { describe, it, expect, vi } from 'vitest'; import { PluginRpcHost } from '../../../src/nest/plugins/host/rpc-host'; import type { RpcRequest, RpcResponse } from '../../../src/nest/plugins/protocol/envelope'; import { makeDeps } from '../../helpers/rpc-host-deps'; +import { createTestPluginRegistry } from '../../../src/nest/plugins/host/rpc-kit/testing'; +import { PackingRpc } from '../../../src/nest/packing/packing.rpc'; +import type { PackingService } from '../../../src/nest/packing/packing.service'; +import type { PluginGuards } from '../../../src/nest/plugins/host/plugin-guards.service'; +import type { RealtimeService } from '../../../src/nest/realtime/realtime.service'; const req = (method: string, params: Record): RpcRequest => ({ k: 'req', id: 'x', method, params }); @@ -60,11 +65,23 @@ describe('dispatch strips the supervisor _inv marker', () => { }); it('RPCINV-006 a handler reading the whole payload object no longer finds _inv in it', async () => { - const deps = makeDeps(); - const host = new PluginRpcHost('p', new Set(['db:write:packing']), deps); // packing.setBagMembers is the one handler that reads the params object as a - // whole (asPayload(p).userIds) rather than a named field off it. + // whole (asPayload(p).userIds) rather than a named field off it, which makes it + // the case where a stray _inv could actually change behaviour. + const setBagMembers = vi.fn(() => ({ bagId: 80, members: [3] })); + const packing = { setBagMembers } as unknown as PackingService; + const guards = { + requireActor: () => 42, + requireTripEdit: () => undefined, + } as unknown as PluginGuards; + const realtime = { broadcast: vi.fn() } as unknown as RealtimeService; + const host = new PluginRpcHost( + 'p', + new Set(['db:write:packing']), + makeDeps(), + createTestPluginRegistry([new PackingRpc(packing, realtime, guards)]), + ); await host.dispatch(req('packing.setBagMembers', { tripId: 1, bagId: 80, userIds: [3], _inv: 'req-7' }), 42); - expect(deps.setPackingBagMembers).toHaveBeenCalledWith(1, 80, [3]); + expect(setBagMembers).toHaveBeenCalledWith('1', '80', [3]); }); }); diff --git a/server/tests/unit/plugins/rpc-host.test.ts b/server/tests/unit/plugins/rpc-host.test.ts index 32598a4f5..1b47c2e3d 100644 --- a/server/tests/unit/plugins/rpc-host.test.ts +++ b/server/tests/unit/plugins/rpc-host.test.ts @@ -77,23 +77,6 @@ describe('PluginRpcHost — capability enforcement', () => { expect((allowed as RpcResponse).result).toEqual([{ id: 7, name: 'Place' }]); }); - it('db:read:packing / db:read:files delegate to the service, membership-checked, and stay separate scopes', async () => { - const packing = new PluginRpcHost('p', new Set(['db:read:packing']), deps); - // no access to trip 2 → refused before the service is called - expect(((await packing.dispatch(req('packing.list', { tripId: 2 }), 42)) as RpcError).error.code).toBe('RESOURCE_FORBIDDEN'); - expect(deps.listPackingItems).not.toHaveBeenCalled(); - const okP = await packing.dispatch(req('packing.list', { tripId: 1 }), 42); - expect(ok(okP)).toBe(true); - // the acting user is threaded through so packing's #858 private-item filter applies - expect(deps.listPackingItems).toHaveBeenCalledWith(1, 42); - // the packing scope does NOT unlock files - expect(((await packing.dispatch(req('files.list', { tripId: 1 }), 42)) as RpcError).error.code).toBe('PERMISSION_DENIED'); - - const files = new PluginRpcHost('p', new Set(['db:read:files']), deps); - const okF = await files.dispatch(req('files.list', { tripId: 1 }), 42); - expect((okF as RpcResponse).result).toEqual([{ id: 2, trip_id: 1, filename: 'visa.pdf' }]); - }); - it('db:read:users returns only the public projection for a visible user', async () => { const host = new PluginRpcHost('p', new Set(['db:read:users']), deps); const res = await host.dispatch(req('users.getById', { id: 3 }), 42); @@ -334,26 +317,6 @@ describe('PluginRpcHost — capability enforcement', () => { expect((await host.dispatch(req('collections.listMine'), undefined)).ok).toBe(false); }); - it('packing.create needs db:write:packing + packing_edit, membership-checked, name required', async () => { - const host = new PluginRpcHost('p', new Set(['db:write:packing']), deps); - const good = await host.dispatch(req('packing.create', { tripId: 1, input: { name: 'Socks' } }), 42); - expect(ok(good)).toBe(true); - expect(deps.createPackingItem).toHaveBeenCalledWith(1, expect.objectContaining({ name: 'Socks' }), 42); - expect((await host.dispatch(req('packing.create', { tripId: 1, input: { name: '' } }), 42)).ok).toBe(false); // schema: name min 1 - expect(((await host.dispatch(req('packing.create', { tripId: 2, input: { name: 'x' } }), 42)) as RpcError).error.code).toBe('RESOURCE_FORBIDDEN'); - expect((await host.dispatch(req('packing.create', { tripId: 1, input: { name: 'x' } }), undefined)).ok).toBe(false); // no acting user - }); - - it('packing.update/delete are gated the same way; nothing without the grant', async () => { - const host = new PluginRpcHost('p', new Set(['db:write:packing']), deps); - expect(ok(await host.dispatch(req('packing.update', { tripId: 1, itemId: 70, input: { checked: true } }), 42))).toBe(true); - expect(deps.updatePackingItem).toHaveBeenCalledWith(1, 70, expect.objectContaining({ checked: true }), 42); - expect(ok(await host.dispatch(req('packing.delete', { tripId: 1, itemId: 70 }), 42))).toBe(true); - expect(deps.deletePackingItem).toHaveBeenCalledWith(1, 70); - const noGrant = new PluginRpcHost('p', new Set(['db:read:packing']), deps); - expect((await noGrant.dispatch(req('packing.create', { tripId: 1, input: { name: 'x' } }), 42)).ok).toBe(false); - }); - // weather.get, categories.list and rates.get moved to tests/unit/plugins/tenant-free.rpc.test.ts. // The tags cases moved to tests/unit/tags/tags.rpc.test.ts with the handlers. @@ -364,14 +327,6 @@ describe('PluginRpcHost — capability enforcement', () => { expect(((await host.dispatch(req('trips.members', { tripId: 2 }), 42)) as RpcError).error.code).toBe('RESOURCE_FORBIDDEN'); }); - it('packing-bags writes need the grant + edit right + a bound user', async () => { - const host = new PluginRpcHost('p', new Set(['db:write:packing']), deps); - expect(ok(await host.dispatch(req('packing.createBag', { tripId: 1, input: { name: 'Bag' } }), 42))).toBe(true); - expect(ok(await host.dispatch(req('packing.setBagMembers', { tripId: 1, bagId: 80, userIds: [5, 6] }), 42))).toBe(true); - expect(deps.setPackingBagMembers).toHaveBeenCalledWith(1, 80, [5, 6]); - expect((await host.dispatch(req('packing.createBag', { tripId: 1, input: { name: 'Bag' } }), undefined)).ok).toBe(false); - }); - it('atlas writes are uid-bound (code-validated, userless refused)', async () => { const host = new PluginRpcHost('p', new Set(['db:write:atlas']), deps); expect(ok(await host.dispatch(req('atlas.markCountry', { code: 'jp' }), 42))).toBe(true);