From 9b3d37ea29dde0f24f308c96b484a287bda1b18d Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 9 Aug 2026 11:36:17 +0200 Subject: [PATCH] Move the tags plugin surface onto the decorators Wires the registry into the host and takes the first domain across, so the kit lands with a worked example rather than as scaffolding nobody uses yet. The registry is an additive, optional fourth constructor parameter on PluginRpcHost, so all 107 existing construction sites keep compiling untouched; that is what makes the rest of the rollout a per-domain move instead of a big-bang one. bindInto runs LAST in the constructor, so a method still owned by one of the has() blocks collides loudly instead of being silently overridden. tags is the right pilot: four methods, a service that is already DI-native, and it exercises the read/write permission split, the userless refusal and a BadParams write. Handler bodies and messages are verbatim; only deps.foo(...) became this.tags... and uid became ctx.actingUserId. dispatch now strips the supervisor's _inv marker. The supervisor resolves the acting user from it before dispatch and then passes the request on untouched, so it has been visible to every handler and is in effect a reserved param name. Nothing reads it: auditResource takes named fields only, and the one handler that touches the params object as a whole (packing.setBagMembers) reads a named field off it. Dropping it keeps a future strict parse from rejecting calls every shipped plugin makes. Two tests carry the rollout from here. rpc-coverage.test.ts derives both owner sets from the code and proves they partition the method vocabulary exactly, so a PR that moves a method and forgets to delete the legacy registration fails in CI. And because a decorated class that no module registers is invisible to discovery, with every call to it coming back PERMISSION_DENIED and no other symptom, tags.rpc.test.ts asserts TagsRpc is actually listed in providers. --- .../plugins/host/plugin-host-deps.factory.ts | 19 +- server/src/nest/plugins/host/rpc-host.ts | 97 ++++----- server/src/nest/plugins/plugins.module.ts | 17 +- server/src/nest/tags/tags.module.ts | 6 +- server/src/nest/tags/tags.rpc.ts | 71 +++++++ server/tests/helpers/rpc-host-deps.ts | 184 +++++++++++++++++ .../plugins/plugin-host-deps.factory.test.ts | 11 +- .../tests/unit/plugins/rpc-coverage.test.ts | 64 ++++++ .../unit/plugins/rpc-host-inv-strip.test.ts | 70 +++++++ server/tests/unit/plugins/rpc-host.test.ts | 165 +--------------- server/tests/unit/tags/tags.rpc.test.ts | 186 ++++++++++++++++++ 11 files changed, 639 insertions(+), 251 deletions(-) create mode 100644 server/src/nest/tags/tags.rpc.ts create mode 100644 server/tests/helpers/rpc-host-deps.ts create mode 100644 server/tests/unit/plugins/rpc-coverage.test.ts create mode 100644 server/tests/unit/plugins/rpc-host-inv-strip.test.ts create mode 100644 server/tests/unit/tags/tags.rpc.test.ts 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 18c0da3ab..ad9b998cd 100644 --- a/server/src/nest/plugins/host/plugin-host-deps.factory.ts +++ b/server/src/nest/plugins/host/plugin-host-deps.factory.ts @@ -38,6 +38,7 @@ import { FilesService } from '../../files/files.service'; import { CollabService } from '../../collab/collab.service'; import { VacayService } from '../../vacay/vacay.service'; import { PluginRpcHost, ForbiddenResource, BadParams } from './rpc-host'; +import { PluginRpcRegistryService } from './rpc-kit/registry.service'; import { appendAudit } from './plugin-audit'; import { getPluginDataDb, budgetFor } from './plugin-host-state'; @@ -167,6 +168,10 @@ export class PluginHostDepsFactory { private readonly notifications: NotificationsService, private readonly membership: TripMembershipService, private readonly journey: JourneyDomainService, + // Appended last and OPTIONAL so the hand-wired 26-arg construction in + // plugin-host-deps.factory.test.ts keeps compiling. Handed to every host it + // builds, which is how decorated *.rpc.ts handlers reach the dispatch map. + private readonly rpcRegistry?: PluginRpcRegistryService, ) {} /** @@ -858,18 +863,6 @@ export class PluginHostDepsFactory { listCategories: () => this.categories.list() as unknown[], 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[], - // --- Tags (the acting user's own; ownership re-checked before a write). --- - listTagsForUser: (userId) => this.tags.list(userId) as unknown[], - createTagForUser: (userId, name, color) => this.tags.create(userId, name, color), - updateTagForUser: (userId, tagId, name, color) => { - if (!this.tags.getByIdAndUser(tagId, userId)) throw new ForbiddenResource(`no tag ${tagId} for this user`); - return this.tags.update(tagId, name, color); - }, - deleteTagForUser: (userId, tagId) => { - if (!this.tags.getByIdAndUser(tagId, userId)) throw new ForbiddenResource(`no tag ${tagId} for this user`); - this.tags.remove(tagId); - return { deleted: true }; - }, // --- Todos (core, trip-scoped; the app's 'packing_edit' permission). --- canEditTodos: (tripId, userId) => this.canEditTripAs('packing_edit', tripId, userId), listTodos: (tripId) => this.todos.listItems(String(tripId)) as unknown[], @@ -939,6 +932,6 @@ export class PluginHostDepsFactory { .run(id, entityType, entityId, key); return { deleted: res.changes > 0 }; }, - }); + }, this.rpcRegistry); } } diff --git a/server/src/nest/plugins/host/rpc-host.ts b/server/src/nest/plugins/host/rpc-host.ts index 3db712c32..cf7d2db60 100644 --- a/server/src/nest/plugins/host/rpc-host.ts +++ b/server/src/nest/plugins/host/rpc-host.ts @@ -21,6 +21,14 @@ import { import type { PluginDataDb } from './plugin-data.service'; import { stripEmoji } from '../text-sanitize'; import { auditResource, isAuditable } from './plugin-audit'; +import { BadParams, ForbiddenResource } from './rpc-errors'; +import { asArgs, asPayload, asTxOps, num, str } from './rpc-params'; +import type { PluginRpcRegistry } from './rpc-kit/registry'; + +// Both used to be declared here. They now live in rpc-errors.ts so decorated +// *.rpc.ts handlers can throw them without importing the router, and are re-exported +// so every existing importer keeps working unchanged. +export { BadParams, ForbiddenResource }; /** * The per-plugin capability router (#plugins, M1) — the ENFORCEMENT POINT. @@ -33,9 +41,6 @@ import { auditResource, isAuditable } from './plugin-audit'; * Runs in the HOST (parent) process. */ -/** Thrown by a handler when the acting user may not touch the requested resource. */ -export class ForbiddenResource extends Error {} - interface CoreDb { prepare(sql: string): { all(...args: unknown[]): unknown[]; @@ -240,10 +245,6 @@ export interface HostDeps { listCategories(): unknown[]; tripMembers(tripId: number): unknown[]; // --- Tags (the acting user's own; no trip) --- - listTagsForUser(userId: number): unknown[]; - createTagForUser(userId: number, name: string, color: string | undefined): unknown; - updateTagForUser(userId: number, tagId: number, name: string | undefined, color: string | undefined): unknown; - deleteTagForUser(userId: number, tagId: number): unknown; // --- Todos (core, trip-scoped; the 'packing_edit' permission, like the REST path) --- canEditTodos(tripId: number, userId: number): boolean; listTodos(tripId: number): unknown[]; @@ -261,17 +262,6 @@ export interface HostDeps { type Handler = (params: Record, actingUserId: number | undefined) => unknown; -const num = (v: unknown, name: string): number => { - const n = typeof v === 'string' ? Number(v) : v; - if (typeof n !== 'number' || !Number.isFinite(n)) throw new BadParams(`${name} must be a number`); - return n; -}; -const str = (v: unknown, name: string): string => { - if (typeof v !== 'string') throw new BadParams(`${name} must be a string`); - return v; -}; -export class BadParams extends Error {} - // Mirrors the STRING_LIMITS the places REST controller enforces (the @trek/shared // schema doesn't), so the plugin write path rejects the same oversized fields. const PLACE_STR_LIMITS: Record = { name: 200, description: 2000, address: 500, notes: 2000 }; @@ -289,6 +279,11 @@ export class PluginRpcHost { private readonly pluginId: string, granted: ReadonlySet, private readonly deps: HostDeps, + // Additive and OPTIONAL, the same convention plugin-runtime.service.ts already + // uses for its own late-added params. Every existing + // `new PluginRpcHost(id, granted, deps)` site keeps compiling untouched, which is + // what makes the decorator rollout a per-domain move instead of a big-bang one. + registry?: PluginRpcRegistry, ) { const has = (p: string) => granted.has(p); @@ -951,30 +946,7 @@ export class PluginRpcHost { // Global, read-only reference list — carries no tenant data. this.methods.set('categories.list', () => deps.listCategories()); } - if (has('db:read:tags')) { - // The acting user's own tags (not trip-scoped) — refuse a userless context. - this.methods.set('tags.list', (_p, uid) => { - if (uid === undefined) throw new ForbiddenResource('tag reads require an authenticated user context'); - return deps.listTagsForUser(uid); - }); - } - if (has('db:write:tags')) { - this.methods.set('tags.create', (p, uid) => { - if (uid === undefined) throw new ForbiddenResource('tag writes require an authenticated user context'); - const input = asPayload(p.input); - if (typeof input.name !== 'string' || input.name.trim() === '') throw new BadParams('tag name is required'); - return deps.createTagForUser(uid, input.name, typeof input.color === 'string' ? input.color : undefined); - }); - this.methods.set('tags.update', (p, uid) => { - if (uid === undefined) throw new ForbiddenResource('tag writes require an authenticated user context'); - const input = asPayload(p.input); - return deps.updateTagForUser(uid, num(p.tagId, 'tagId'), typeof input.name === 'string' ? input.name : undefined, typeof input.color === 'string' ? input.color : undefined); - }); - this.methods.set('tags.delete', (p, uid) => { - if (uid === undefined) throw new ForbiddenResource('tag writes require an authenticated user context'); - return deps.deleteTagForUser(uid, num(p.tagId, 'tagId')); - }); - } + // tags.* now lives in src/nest/tags/tags.rpc.ts, bound through the registry below. if (has('db:read:todos')) { this.methods.set('todos.list', (p, uid) => this.tripRead(p, uid, () => deps.listTodos(num(p.tripId, 'tripId')))); } @@ -1155,6 +1127,21 @@ export class PluginRpcHost { if (uid === undefined) return { value: undefined }; return { value: deps.getUserSetting(this.pluginId, uid, str(p.key, 'key')) }; }); + + // LAST on purpose: a method still owned by one of the has() blocks above then + // collides loudly in bindInto instead of being silently overridden, which is the + // failure mode a half-finished domain move would otherwise have. + // + // `deps.data` is already the lazy per-access getter the factory supplies, so + // passing it through a getter here preserves the disable/re-enable semantics + // exactly and the kit never needs to know about plugin-host-state. + registry?.bindInto(this.methods, granted, (actingUserId) => ({ + pluginId, + actingUserId, + get data() { + return deps.data; + }, + })); } /** @@ -1242,7 +1229,14 @@ export class PluginRpcHost { } async dispatch(req: RpcRequest, actingUserId?: number): Promise { - const params = (req.params ?? {}) as Record; + const raw = (req.params ?? {}) as Record; + // The supervisor resolves the acting user from `_inv` BEFORE dispatch and passes + // the request on untouched, so `_inv` is visible to every handler today and is in + // effect a reserved param name. Strip it here: no handler and no audit reads + // anything but named fields, and dropping it keeps a future strict schema parse + // from rejecting calls every shipped plugin makes. + const { _inv, ...stripped } = raw; + const params = '_inv' in raw ? stripped : raw; const res = await this.handle(req, params, actingUserId); // Audit the core-data / broadcast surface (incl. denials) at the boundary. if (this.deps.audit && isAuditable(req.method)) { @@ -1301,23 +1295,6 @@ export class PluginRpcHost { } } -function asArgs(v: unknown): unknown[] { - if (v == null) return []; - if (Array.isArray(v)) return v; - throw new BadParams('args must be an array'); -} -/** Coerce a db.tx `ops` param into a validated {sql, args}[] before it reaches the - * data db — each op must carry a string sql and, if present, an array of args. */ -function asTxOps(v: unknown): Array<{ sql: string; args?: unknown[] }> { - if (!Array.isArray(v)) throw new BadParams('ops must be an array of { sql, args }'); - return v.map((op) => { - const o = (op ?? {}) as Record; - return { sql: str(o.sql, 'ops[].sql'), args: asArgs(o.args) }; - }); -} -function asPayload(v: unknown): Record { - return v && typeof v === 'object' ? (v as Record) : { value: v }; -} /** * The reservation body is passthrough by contract, but a malformed `endpoints` * array would otherwise fail DEEP in the service (NOT-NULL mid-transaction) or be diff --git a/server/src/nest/plugins/plugins.module.ts b/server/src/nest/plugins/plugins.module.ts index a9318669a..e0deaa63f 100644 --- a/server/src/nest/plugins/plugins.module.ts +++ b/server/src/nest/plugins/plugins.module.ts @@ -1,4 +1,5 @@ import { Module } from '@nestjs/common'; +import { DiscoveryModule } from '@nestjs/core'; import { JourneyDomainModule } from '../journey/journey-domain.module'; import { PluginsController } from './plugins.controller'; import { PluginsFeedController } from './plugins-feed.controller'; @@ -26,6 +27,8 @@ import { PluginsService } from './plugins.service'; import { PluginRuntimeService } from './plugin-runtime.service'; import { PluginRegistryService } from './registry/registry.service'; import { PluginHostDepsFactory } from './host/plugin-host-deps.factory'; +import { PluginRpcRegistryService } from './host/rpc-kit/registry.service'; +import { PluginGuards } from './host/plugin-guards.service'; import { TagsModule } from '../tags/tags.module'; import { CategoriesModule } from '../categories/categories.module'; import { BudgetModule } from '../budget/budget.module'; @@ -58,11 +61,15 @@ import { TripMembershipModule } from '../trip-membership/trip-membership.module' @Module({ // The DI-native domain services the plugin host wiring injects // (PluginHostDepsFactory); DatabaseModule is @Global, so not listed. - imports: [TagsModule, CategoriesModule, BudgetModule, ReservationsModule, TodoModule, PackingModule, DaysModule, AssignmentsModule, LlmParseModule, FilesModule, CollabModule, VacayModule, TripsModule, PermissionsModule, AuditModule, AddonsModule, PlacesModule, CollectionsModule, AtlasModule, NotificationsModule, TripMembershipModule, JourneyDomainModule], + // DiscoveryModule is what lets PluginRpcRegistryService find the @PluginController + // providers at boot. + imports: [DiscoveryModule, TagsModule, CategoriesModule, BudgetModule, ReservationsModule, TodoModule, PackingModule, DaysModule, AssignmentsModule, LlmParseModule, FilesModule, CollabModule, VacayModule, TripsModule, PermissionsModule, AuditModule, AddonsModule, PlacesModule, CollectionsModule, AtlasModule, NotificationsModule, TripMembershipModule, JourneyDomainModule], controllers: [PluginsController, PluginsFeedController, PluginsProxyController, PluginFrameController, PlaceDetailsController, TripWarningsController, ViewContributionsController, TripCardContributionsController, PluginPhotosController, PluginCalendarController, MapMarkersController, MapLayersController, PluginRoutesController, DayScheduleController, DayTintsController, PdfSectionsController, AtlasLayersController, JournalEntryRowsController, PluginUserSettingsController, PluginOAuthController, PluginActivityController], - providers: [PluginsService, PluginRuntimeService, PluginRegistryService, PluginOAuthService, PluginHostDepsFactory], - // Exported so the admin addon-toggle handler can cascade-disable plugins whose - // required addon was just turned off (#plugins dependencies). - exports: [PluginRuntimeService], + providers: [PluginsService, PluginRuntimeService, PluginRegistryService, PluginOAuthService, PluginHostDepsFactory, PluginRpcRegistryService, PluginGuards], + // PluginRuntimeService is exported so the admin addon-toggle handler can + // cascade-disable plugins whose required addon was just turned off (#plugins + // dependencies). PluginGuards is exported so a domain module can inject it into + // its own .rpc.ts without importing the plugin host. + exports: [PluginRuntimeService, PluginGuards], }) export class PluginsModule {} diff --git a/server/src/nest/tags/tags.module.ts b/server/src/nest/tags/tags.module.ts index aa3d685ae..2d5fa8d0c 100644 --- a/server/src/nest/tags/tags.module.ts +++ b/server/src/nest/tags/tags.module.ts @@ -1,6 +1,7 @@ import { Module } from '@nestjs/common'; import { TagsController } from './tags.controller'; import { TagsMcp } from './tags.mcp'; +import { TagsRpc } from './tags.rpc'; import { TagsService } from './tags.service'; import { AuthModule } from '../auth/auth.module'; @@ -8,7 +9,10 @@ import { AuthModule } from '../auth/auth.module'; @Module({ imports: [AuthModule], controllers: [TagsController], - providers: [TagsService, TagsMcp], + // TagsRpc must stay in providers: the plugin RPC registry discovers marked + // PROVIDERS only, and a missing entry here would leave tags.* answering + // PERMISSION_DENIED with no other symptom. + providers: [TagsService, TagsMcp, TagsRpc], // For in-container consumers (PluginHostDepsFactory). exports: [TagsService], }) diff --git a/server/src/nest/tags/tags.rpc.ts b/server/src/nest/tags/tags.rpc.ts new file mode 100644 index 000000000..6a0e53fab --- /dev/null +++ b/server/src/nest/tags/tags.rpc.ts @@ -0,0 +1,71 @@ +import { PluginController, PluginMethod } from '../plugins/host/rpc-kit/decorators'; +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 { TagsService } from './tags.service'; + +/** + * The tags surface a plugin may reach (#plugins). Tags are the acting user's own and + * are not trip-scoped, so there is no membership gate here; what every handler does + * need is a HOST-bound acting user, and a write additionally re-checks that the tag + * belongs to that user. + * + * Moved verbatim out of the router and the deps factory. The messages differ by one + * word between the read and the writes ("tag reads" vs "tag writes"), which is why + * the read throws inline instead of going through PluginGuards.requireActor. + */ +@PluginController() +export class TagsRpc { + constructor(private readonly tags: TagsService) {} + + @PluginMethod('tags.list', { permission: 'db:read:tags' }) + list(_params: Record, ctx: PluginRpcContext): unknown[] { + if (ctx.actingUserId === undefined) { + throw new ForbiddenResource('tag reads require an authenticated user context'); + } + return this.tags.list(ctx.actingUserId) as unknown[]; + } + + @PluginMethod('tags.create', { permission: 'db:write:tags' }) + create(params: Record, ctx: PluginRpcContext): unknown { + if (ctx.actingUserId === undefined) { + throw new ForbiddenResource('tag writes require an authenticated user context'); + } + const input = asPayload(params.input); + if (typeof input.name !== 'string' || input.name.trim() === '') throw new BadParams('tag name is required'); + return this.tags.create(ctx.actingUserId, input.name, typeof input.color === 'string' ? input.color : undefined); + } + + @PluginMethod('tags.update', { permission: 'db:write:tags' }) + update(params: Record, ctx: PluginRpcContext): unknown { + if (ctx.actingUserId === undefined) { + throw new ForbiddenResource('tag writes require an authenticated user context'); + } + const input = asPayload(params.input); + const tagId = num(params.tagId, 'tagId'); + this.requireOwnTag(tagId, ctx.actingUserId); + return this.tags.update( + tagId, + typeof input.name === 'string' ? input.name : undefined, + typeof input.color === 'string' ? input.color : undefined, + ); + } + + @PluginMethod('tags.delete', { permission: 'db:write:tags' }) + delete(params: Record, ctx: PluginRpcContext): unknown { + if (ctx.actingUserId === undefined) { + throw new ForbiddenResource('tag writes require an authenticated user context'); + } + const tagId = num(params.tagId, 'tagId'); + this.requireOwnTag(tagId, ctx.actingUserId); + this.tags.remove(tagId); + return { deleted: true }; + } + + /** Ownership is re-checked per write, so a plugin cannot edit another user's tag. */ + private requireOwnTag(tagId: number, userId: number): void { + if (!this.tags.getByIdAndUser(tagId, userId)) { + throw new ForbiddenResource(`no tag ${tagId} for this user`); + } + } +} diff --git a/server/tests/helpers/rpc-host-deps.ts b/server/tests/helpers/rpc-host-deps.ts new file mode 100644 index 000000000..1fa2c67bb --- /dev/null +++ b/server/tests/helpers/rpc-host-deps.ts @@ -0,0 +1,184 @@ +import { vi } from 'vitest'; +import type { HostDeps } from '../../src/nest/plugins/host/rpc-host'; +import { TagsRpc } from '../../src/nest/tags/tags.rpc'; +import type { TagsService } from '../../src/nest/tags/tags.service'; + +/** + * The stubbed HostDeps every plugin router test builds on. It used to live inside + * rpc-host.test.ts; it moved here so the coverage ledger test can construct the same + * host without duplicating 150 lines of stubs. + * + * The fixture encodes one access story throughout: user 42 may access and edit trip 1, + * and nothing else. Tests rely on that, so widen it only with care. + */ +export function makeDeps(): HostDeps { + return { + data: { + query: vi.fn(() => [{ n: 1 }]), + exec: vi.fn(() => ({ changes: 1 })), + migrate: vi.fn(() => ({ applied: true })), + close: vi.fn(), + } as unknown as HostDeps['data'], + db: { + prepare: vi.fn((sql: string) => ({ + all: () => [{ id: 7, name: 'Place' }], + get: () => + sql.includes('FROM trips') + ? { id: 1, title: 'Japan', start_date: '2027-01-01' } + : { id: 3, username: 'ada', display_name: 'Ada', avatar: null }, + })), + }, + // trip 1 is accessible to user 42; everything else is not + canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1 } : undefined)), + // user 42 may see user 3 (they share a trip); nobody else + canSeeUser: vi.fn((actingUserId: number, targetUserId: number) => actingUserId === 42 && targetUserId === 3), + broadcastToTrip: vi.fn(), + broadcastToUser: vi.fn(), + // Costs (budget) — addon on; user 42 may edit trip 1's costs. + budgetAddonEnabled: vi.fn(() => true), + canEditCosts: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + listPackingItems: vi.fn((tripId: number, _userId: number) => [{ id: 1, trip_id: tripId, name: 'Socks' }]), + listTripFiles: vi.fn((tripId: number) => [{ id: 2, trip_id: tripId, filename: 'visa.pdf' }]), + getTripFileContent: vi.fn((tripId: number, fileId: number) => ({ name: 'visa.pdf', mimetype: 'application/pdf', size: 3, content_base64: 'aGk=', _t: tripId, _f: fileId })), + listCollabNotes: vi.fn((tripId: number) => [{ id: 1, trip_id: tripId, title: 'Note' }]), + listCollabPolls: vi.fn((tripId: number) => [{ id: 2, trip_id: tripId, question: 'Q?' }]), + listCollabMessages: vi.fn((tripId: number, before: number | undefined) => [{ id: 3, trip_id: tripId, text: 'hi', _before: before ?? null }]), + journalEntriesForUser: vi.fn((uid: number, journeyId: number) => [{ id: 10, journey_id: journeyId, author_id: uid }]), + atlasBucketForUser: vi.fn((uid: number) => [{ id: 5, user_id: uid, name: 'Kyoto' }]), + canCreateTrip: vi.fn((userId: number) => userId === 42), + createTripForUser: vi.fn((userId: number, input: unknown) => ({ id: 99, user_id: userId, ...(input as object) })), + getRates: vi.fn(async (base: string) => ({ [base]: 1, USD: 1.08 })), + listCostsForTrip: vi.fn((tripId: number) => [{ id: 5, trip_id: tripId, name: 'Hotel', total_price: 100 }]), + listCostsForUser: vi.fn(() => [ + { id: 5, trip_id: 1, name: 'Hotel' }, + { id: 6, trip_id: 2, name: 'Food' }, + ]), + createCost: vi.fn((tripId: number, input: unknown) => ({ id: 9, trip_id: tripId, ...(input as object) })), + updateCost: vi.fn((tripId: number, itemId: number, input: unknown) => ({ id: itemId, trip_id: tripId, ...(input as object) })), + deleteCost: vi.fn(() => ({ deleted: true })), + // Planner writes — user 42 may edit trip 1 only (mirrors canAccessTrip). + canEditPlaces: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + createPlace: vi.fn((tripId: number, input: unknown) => ({ id: 10, trip_id: tripId, ...(input as object) })), + updatePlace: vi.fn((tripId: number, placeId: number, input: unknown) => ({ id: placeId, trip_id: tripId, ...(input as object) })), + deletePlace: vi.fn(() => ({ deleted: true })), + canEditDays: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + createDay: vi.fn((tripId: number, input: unknown) => ({ id: 20, trip_id: tripId, ...(input as object) })), + updateDay: vi.fn((tripId: number, dayId: number, input: unknown) => ({ id: dayId, trip_id: tripId, ...(input as object) })), + deleteDay: vi.fn(() => ({ deleted: true })), + assignPlaceToDay: vi.fn((tripId: number, dayId: number, placeId: number, notes: string | null) => ({ id: 30, day_id: dayId, place_id: placeId, notes })), + unassignPlace: vi.fn(() => ({ deleted: true })), + canEditTrip: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + updateTrip: vi.fn((tripId: number, _userId: number, input: unknown) => ({ id: tripId, ...(input as object) })), + // Cross-trip reads + reservations (bookings) — user 42 may edit trip 1 only. + listTripsForUser: vi.fn(() => [{ id: 1, title: 'Japan' }, { id: 2, title: 'Peru' }]), + listReservationsForUser: vi.fn(() => [{ id: 5, trip_id: 1, title: 'Hotel' }, { id: 6, trip_id: 2, title: 'Flight' }]), + canEditReservations: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + createReservation: vi.fn((tripId: number, input: unknown) => ({ id: 40, trip_id: tripId, ...(input as object) })), + updateReservation: vi.fn((tripId: number, reservationId: number, input: unknown) => ({ id: reservationId, trip_id: tripId, ...(input as object) })), + deleteReservation: vi.fn(() => ({ deleted: true })), + // Trip-scoped hydrated reads + accommodations (lodging blocks, day_edit-gated). + listTripDays: vi.fn((tripId: number) => [{ id: 3, trip_id: tripId, day_number: 1, assignments: [], notes_items: [] }]), + listTripReservations: vi.fn((tripId: number) => [{ id: 5, trip_id: tripId, title: 'Hotel', endpoints: [], day_positions: null }]), + listTripAccommodations: vi.fn((tripId: number) => [{ id: 11, trip_id: tripId, place_name: 'Ryokan' }]), + createAccommodation: vi.fn((tripId: number, input: unknown) => ({ id: 60, trip_id: tripId, ...(input as object) })), + updateAccommodation: vi.fn((tripId: number, accommodationId: number, input: unknown) => ({ id: accommodationId, trip_id: tripId, ...(input as object) })), + deleteAccommodation: vi.fn(() => ({ deleted: true })), + // User-scoped addon reads + day notes. + listJournalsForUser: vi.fn(() => [{ id: 1, title: 'Japan 2027' }]), + atlasVisitedForUser: vi.fn(() => ({ countries: [{ country_code: 'JP' }], regions: [] })), + vacayForUser: vi.fn(() => ({ plan: { id: 1 }, entries: [] })), + listCollectionsForUser: vi.fn(() => ({ collections: [{ id: 1, name: 'Tokyo eats' }] })), + getCollectionForUser: vi.fn((_userId: number, id: number) => ({ id, name: 'Tokyo eats', places: [] })), + listDayNotes: vi.fn((tripId: number, dayId: number) => [{ id: 1, day_id: dayId, trip_id: tripId, text: 'note' }]), + createDayNote: vi.fn((_tripId: number, dayId: number, input: unknown) => ({ id: 50, day_id: dayId, ...(input as object) })), + updateDayNote: vi.fn((_tripId: number, dayId: number, noteId: number, input: unknown) => ({ id: noteId, day_id: dayId, ...(input as object) })), + deleteDayNote: vi.fn(() => ({ deleted: true })), + canEditPacking: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + createPackingItem: vi.fn((tripId: number, input: unknown) => ({ id: 70, trip_id: tripId, ...(input as object) })), + updatePackingItem: vi.fn((tripId: number, itemId: number, input: unknown) => ({ id: itemId, trip_id: tripId, ...(input as object) })), + deletePackingItem: vi.fn(() => ({ deleted: true })), + listPackingBags: vi.fn(() => [{ id: 80, name: 'Backpack' }]), + createPackingBag: vi.fn((tripId: number, input: unknown) => ({ id: 80, trip_id: tripId, ...(input as object) })), + updatePackingBag: vi.fn((_tripId: number, bagId: number, input: unknown) => ({ id: bagId, ...(input as object) })), + deletePackingBag: vi.fn(() => ({ deleted: true })), + setPackingBagMembers: vi.fn((_tripId: number, bagId: number, userIds: number[]) => ({ bagId, members: userIds })), + getWeather: vi.fn(() => ({ temp: 20 })), + listCategories: vi.fn(() => [{ id: 1, name: 'Food' }]), + tripMembers: vi.fn(() => [{ id: 5, username: 'ada' }, { id: 6, username: 'bob' }]), + canUploadFiles: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + canEditFiles: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + canDeleteFiles: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + createTripFile: vi.fn((tripId: number, input: unknown, uid: number) => ({ id: 130, trip_id: tripId, uploaded_by: uid, ...(input as object) })), + createTripFileLink: vi.fn(() => [{ file_id: 130 }]), + updateTripFile: vi.fn((_tripId: number, fileId: number, input: unknown) => ({ id: fileId, ...(input as object) })), + softDeleteTripFile: vi.fn(() => ({ deleted: true })), + canEditCollab: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + createCollabNote: vi.fn((tripId: number, input: unknown, uid: number) => ({ id: 140, trip_id: tripId, created_by: uid, ...(input as object) })), + createCollabPoll: vi.fn((tripId: number, input: unknown) => ({ id: 141, trip_id: tripId, ...(input as object) })), + voteCollabPoll: vi.fn((_tripId: number, pollId: number) => ({ id: pollId, votes: 1 })), + createCollabMessage: vi.fn((tripId: number, text: string) => ({ id: 142, trip_id: tripId, text })), + canManageMembers: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + addTripMember: vi.fn((tripId: number, targetUserId: number, invitedBy: number) => ({ joined: true, tripId, targetUserId, invitedBy })), + removeTripMember: vi.fn((tripId: number, targetUserId: number) => ({ removed: true, tripId, targetUserId })), + canAccessTripForNotify: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + sendPluginNotification: vi.fn(async (pluginId: string, input: unknown) => ({ sent: true, pluginId, ...(input as object) })), + aiConfigured: vi.fn((userId: number) => userId === 42), + aiComplete: vi.fn(async (_uid: number, prompt: string) => ({ text: `echo:${prompt}` })), + aiExtract: vi.fn(async () => ({ results: [{ ok: true }] })), + getUserSetting: vi.fn((pluginId: string, userId: number, key: string) => (userId === 42 && key === 'apiKey' ? `secret-of-${pluginId}` : undefined)), + getOAuthToken: vi.fn(async (_pluginId: string, userId: number) => (userId === 42 ? 'access-token-xyz' : null)), + schedulerSet: vi.fn((name: string, dueAt: number, everyMs: number | undefined) => ({ scheduled: true, name, dueAt, everyMs })), + schedulerCancel: vi.fn((name: string) => ({ cancelled: name !== 'ghost' })), + createCollectionForUser: vi.fn((uid: number, input: unknown) => ({ id: 100, owner_id: uid, ...(input as object) })), + updateCollectionForUser: vi.fn((_uid: number, id: number, input: unknown) => ({ id, ...(input as object) })), + saveCollectionPlace: vi.fn((uid: number, input: unknown) => ({ id: 101, saved_by: uid, ...(input as object) })), + copyCollectionToTrip: vi.fn(() => ({ copied: 2, skipped: [] })), + deleteCollectionPlace: vi.fn(() => ({ deleted: true })), + markCountryVisited: vi.fn(() => ({ visited: true })), + unmarkCountryVisited: vi.fn(() => ({ visited: false })), + markRegionVisited: vi.fn(() => ({ visited: true })), + unmarkRegionVisited: vi.fn(() => ({ visited: false })), + createBucketItem: vi.fn((uid: number, input: unknown) => ({ id: 110, user_id: uid, ...(input as object) })), + deleteBucketItem: vi.fn(() => ({ deleted: true })), + vacayToggleEntry: vi.fn(() => ({ action: 'added' })), + vacayToggleCompanyHoliday: vi.fn(() => ({ action: 'added' })), + createJournalEntry: vi.fn((uid: number, journeyId: number, input: unknown) => ({ id: 120, journey_id: journeyId, created_by: uid, ...(input as object) })), + updateJournalEntry: vi.fn((_uid: number, entryId: number, input: unknown) => ({ id: entryId, ...(input as object) })), + deleteJournalEntry: vi.fn(() => ({ deleted: true })), + createJournal: vi.fn((uid: number, input: unknown) => ({ id: 130, user_id: uid, ...(input as object) })), + deleteJournal: vi.fn(() => ({ deleted: true })), + canEditTodos: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), + listTodos: vi.fn(() => [{ id: 1, name: 'Pack' }]), + createTodo: vi.fn((tripId: number, input: unknown) => ({ id: 90, trip_id: tripId, ...(input as object) })), + updateTodo: vi.fn((_tripId: number, todoId: number, input: unknown) => ({ id: todoId, ...(input as object) })), + deleteTodo: vi.fn(() => ({ deleted: true })), + // Metadata — trip 1 and place 7 resolve to trip 1 (accessible to 42); else undefined. + metaEntityTrip: vi.fn((entityType: string, entityId: number) => + (entityType === 'trip' && entityId === 1) || (entityType === 'place' && entityId === 7) || (entityType === 'day' && entityId === 3) + || (entityType === 'reservation' && entityId === 40) || (entityType === 'accommodation' && entityId === 11) ? 1 : undefined), + metaGet: vi.fn(() => ({ hello: 'world' })), + metaSet: vi.fn((_et: string, _eid: number, key: string, value: unknown) => ({ key, value })), + metaList: vi.fn(() => ({ a: 1 })), + metaDelete: vi.fn(() => ({ deleted: true })), + }; +} + +/** + * Every @PluginController() class that owns wire methods, in the order the container + * would discover them. It grows by one entry per rollout PR, and the coverage ledger + * test (tests/unit/plugins/rpc-coverage.test.ts) is what turns a forgotten entry into + * a red build instead of a silent PERMISSION_DENIED in production. + * + * The services handed in are stubs: the ledger only binds the handlers, it never runs + * them. Behaviour is asserted in each domain's own .rpc.test.ts. + */ +export function allRpcControllers(): object[] { + const tags = { + list: vi.fn(() => []), + getByIdAndUser: vi.fn(() => undefined), + create: vi.fn(() => ({})), + update: vi.fn(() => ({})), + remove: vi.fn(), + } as unknown as TagsService; + return [new TagsRpc(tags)]; +} 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 e2161d2f2..8c81fb3fb 100644 --- a/server/tests/unit/plugins/plugin-host-deps.factory.test.ts +++ b/server/tests/unit/plugins/plugin-host-deps.factory.test.ts @@ -800,15 +800,8 @@ describe('host-deps factory — Wave 1 wiring (weather/categories/tags/todos/ros expect((await call(host('db:read:categories'), 'categories.list', {}, undefined)).ok).toBe(true) }) - it('tags: list/create/update/delete are the acting user\'s; a missing tag is RESOURCE_FORBIDDEN', async () => { - const h = host('db:read:tags', 'db:write:tags') - expect((await call(h, 'tags.list', {}, 5)).ok).toBe(true) - expect((await call(h, 'tags.create', { input: { name: 'x' } }, 5)).ok).toBe(true) - expect((await call(h, 'tags.update', { tagId: 1, input: { name: 'y' } }, 5)).ok).toBe(true) - expect((await call(h, 'tags.delete', { tagId: 1 }, 5)).ok).toBe(true) - expect(((await call(h, 'tags.update', { tagId: 404, input: {} }, 5)) as { error: { code: string } }).error.code).toBe('RESOURCE_FORBIDDEN') - expect(((await call(h, 'tags.delete', { tagId: 404 }, 5)) as { error: { code: string } }).error.code).toBe('RESOURCE_FORBIDDEN') - }) + // tags.* left this factory with the decorator migration. The same cases now run in + // tests/unit/tags/tags.rpc.test.ts against TagsRpc. it('trips.members returns the roster', async () => { const r = await call(host('db:read:trips'), 'trips.members', { tripId: 1 }, 5) diff --git a/server/tests/unit/plugins/rpc-coverage.test.ts b/server/tests/unit/plugins/rpc-coverage.test.ts new file mode 100644 index 000000000..382b3650a --- /dev/null +++ b/server/tests/unit/plugins/rpc-coverage.test.ts @@ -0,0 +1,64 @@ +/** + * The coverage ledger: the anchor for the whole decorator rollout. + * + * Every wire method is owned exactly once, either by the legacy router blocks in + * rpc-host.ts or by a decorated *.rpc.ts class in the registry. Both sets are derived + * by construction, so no list is maintained by hand and the test cannot drift. + * + * It fails the moment a migration PR moves a method and forgets to delete the legacy + * registration, deletes one without adding it, or forgets to list a new *.rpc.ts class + * in allRpcControllers(). + */ +import { describe, it, expect } from 'vitest'; +import { PluginRpcHost } from '../../../src/nest/plugins/host/rpc-host'; +import { createTestPluginRegistry } from '../../../src/nest/plugins/host/rpc-kit/testing'; +import { + KNOWN_METHODS, + KNOWN_PERMISSIONS, + UNCONDITIONAL_METHODS, +} from '../../../src/nest/plugins/protocol/envelope'; +import { allRpcControllers, makeDeps } from '../../helpers/rpc-host-deps'; + +/** Reads the router's private dispatch map. That map IS the thing under test. */ +const boundMethods = (host: PluginRpcHost): Set => + new Set((host as unknown as { methods: Map }).methods.keys()); + +describe('plugin RPC coverage ledger', () => { + const registry = createTestPluginRegistry(allRpcControllers()); + // Every permission granted, so the router registers everything it can. + const host = new PluginRpcHost('probe', new Set(KNOWN_PERMISSIONS), makeDeps(), registry); + const all = boundMethods(host); + const fromRegistry = registry.methodNames(); + const legacy = new Set([...all].filter((m) => !fromRegistry.has(m))); + + it('RPCLEDGER-001 no method is owned by both the registry and the legacy router', () => { + expect([...fromRegistry].filter((m) => legacy.has(m))).toEqual([]); + }); + + it('RPCLEDGER-002 every registered method is one the wire protocol knows', () => { + const vocabulary = new Set([...KNOWN_METHODS, ...UNCONDITIONAL_METHODS]); + expect([...all].filter((m) => !vocabulary.has(m))).toEqual([]); + }); + + it('RPCLEDGER-003 no method in the vocabulary is orphaned', () => { + const vocabulary = new Set([...KNOWN_METHODS, ...UNCONDITIONAL_METHODS]); + expect([...vocabulary].filter((m) => !all.has(m))).toEqual([]); + }); + + it('RPCLEDGER-004 the two owner sets partition the vocabulary exactly', () => { + expect(all).toEqual(new Set([...KNOWN_METHODS, ...UNCONDITIONAL_METHODS])); + expect(fromRegistry.size + legacy.size).toBe(all.size); + }); + + it('RPCLEDGER-005 the migration ledger reads as expected for this PR', () => { + // 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(4); // tags.list / create / update / delete + expect(legacy.size).toBe(total - 4); + }); + + it('RPCLEDGER-006 the migrated methods are exactly the ones this PR claims', () => { + expect([...fromRegistry].sort()).toEqual(['tags.create', 'tags.delete', 'tags.list', 'tags.update']); + }); +}); diff --git a/server/tests/unit/plugins/rpc-host-inv-strip.test.ts b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts new file mode 100644 index 000000000..b73011533 --- /dev/null +++ b/server/tests/unit/plugins/rpc-host-inv-strip.test.ts @@ -0,0 +1,70 @@ +/** + * The supervisor tags every host-bound request with `_inv`, resolves the acting user + * from it BEFORE dispatch, and then passes the request on untouched. That made `_inv` + * visible to every handler and, in effect, a reserved param name. + * + * dispatch() now strips it. These tests prove the strip is invisible: the same result, + * the same audit row, and the acting user still resolved from the host side. + */ +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'; + +const req = (method: string, params: Record): RpcRequest => ({ k: 'req', id: 'x', method, params }); + +describe('dispatch strips the supervisor _inv marker', () => { + it('RPCINV-001 the handler never sees _inv', async () => { + const deps = makeDeps(); + const host = new PluginRpcHost('p', new Set(['db:own']), deps); + await host.dispatch(req('db.query', { sql: 'SELECT 1', args: [], _inv: 'req-7' })); + expect(deps.data.query).toHaveBeenCalledWith('SELECT 1', []); + }); + + it('RPCINV-002 the result is identical with and without the marker', async () => { + const host = new PluginRpcHost('p', new Set(['db:own']), makeDeps()); + const withMarker = (await host.dispatch(req('db.query', { sql: 'SELECT 1', _inv: 'req-7' }))) as RpcResponse; + const without = (await host.dispatch(req('db.query', { sql: 'SELECT 1' }))) as RpcResponse; + expect(withMarker.ok).toBe(true); + expect(withMarker.result).toEqual(without.result); + }); + + it('RPCINV-003 the audit row is unchanged, because auditResource reads named fields only', async () => { + const deps = makeDeps(); + const audit = vi.fn(); + const host = new PluginRpcHost('p', new Set(['db:read:trips']), { ...deps, audit }); + await host.dispatch(req('trips.getById', { tripId: 1, _inv: 'req-7' }), 42); + expect(audit).toHaveBeenCalledWith({ + pluginId: 'p', + actingUserId: 42, + method: 'trips.getById', + resource: 'trip:1', + code: 'ok', + }); + }); + + it('RPCINV-004 the acting user still comes from the host, not from the params', async () => { + const deps = makeDeps(); + const host = new PluginRpcHost('p', new Set(['db:read:trips']), deps); + // No acting user bound: the trip read is refused even though _inv is present. + const res = await host.dispatch(req('trips.getById', { tripId: 1, _inv: 'req-7' })); + expect(res.ok).toBe(false); + expect((res as { error: { code: string } }).error.code).toBe('RESOURCE_FORBIDDEN'); + }); + + it('RPCINV-005 params without the marker are passed through untouched', async () => { + const deps = makeDeps(); + const host = new PluginRpcHost('p', new Set(['db:own']), deps); + await host.dispatch(req('db.exec', { sql: 'INSERT INTO t VALUES (1)', args: [1] })); + expect(deps.data.exec).toHaveBeenCalledWith('INSERT INTO t VALUES (1)', [1]); + }); + + 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. + await host.dispatch(req('packing.setBagMembers', { tripId: 1, bagId: 80, userIds: [3], _inv: 'req-7' }), 42); + expect(deps.setPackingBagMembers).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 344695e97..39c05694b 100644 --- a/server/tests/unit/plugins/rpc-host.test.ts +++ b/server/tests/unit/plugins/rpc-host.test.ts @@ -10,162 +10,8 @@ import Database from 'better-sqlite3'; import { createTables } from '../../../src/db/schema'; import { PluginRpcHost, type HostDeps } from '../../../src/nest/plugins/host/rpc-host'; import type { RpcRequest, RpcResponse, RpcError } from '../../../src/nest/plugins/protocol/envelope'; +import { makeDeps } from '../../helpers/rpc-host-deps'; -function makeDeps(): HostDeps { - return { - data: { - query: vi.fn(() => [{ n: 1 }]), - exec: vi.fn(() => ({ changes: 1 })), - migrate: vi.fn(() => ({ applied: true })), - close: vi.fn(), - } as unknown as HostDeps['data'], - db: { - prepare: vi.fn((sql: string) => ({ - all: () => [{ id: 7, name: 'Place' }], - get: () => - sql.includes('FROM trips') - ? { id: 1, title: 'Japan', start_date: '2027-01-01' } - : { id: 3, username: 'ada', display_name: 'Ada', avatar: null }, - })), - }, - // trip 1 is accessible to user 42; everything else is not - canAccessTrip: vi.fn((tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1 } : undefined)), - // user 42 may see user 3 (they share a trip); nobody else - canSeeUser: vi.fn((actingUserId: number, targetUserId: number) => actingUserId === 42 && targetUserId === 3), - broadcastToTrip: vi.fn(), - broadcastToUser: vi.fn(), - // Costs (budget) — addon on; user 42 may edit trip 1's costs. - budgetAddonEnabled: vi.fn(() => true), - canEditCosts: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - listPackingItems: vi.fn((tripId: number, _userId: number) => [{ id: 1, trip_id: tripId, name: 'Socks' }]), - listTripFiles: vi.fn((tripId: number) => [{ id: 2, trip_id: tripId, filename: 'visa.pdf' }]), - getTripFileContent: vi.fn((tripId: number, fileId: number) => ({ name: 'visa.pdf', mimetype: 'application/pdf', size: 3, content_base64: 'aGk=', _t: tripId, _f: fileId })), - listCollabNotes: vi.fn((tripId: number) => [{ id: 1, trip_id: tripId, title: 'Note' }]), - listCollabPolls: vi.fn((tripId: number) => [{ id: 2, trip_id: tripId, question: 'Q?' }]), - listCollabMessages: vi.fn((tripId: number, before: number | undefined) => [{ id: 3, trip_id: tripId, text: 'hi', _before: before ?? null }]), - journalEntriesForUser: vi.fn((uid: number, journeyId: number) => [{ id: 10, journey_id: journeyId, author_id: uid }]), - atlasBucketForUser: vi.fn((uid: number) => [{ id: 5, user_id: uid, name: 'Kyoto' }]), - canCreateTrip: vi.fn((userId: number) => userId === 42), - createTripForUser: vi.fn((userId: number, input: unknown) => ({ id: 99, user_id: userId, ...(input as object) })), - getRates: vi.fn(async (base: string) => ({ [base]: 1, USD: 1.08 })), - listCostsForTrip: vi.fn((tripId: number) => [{ id: 5, trip_id: tripId, name: 'Hotel', total_price: 100 }]), - listCostsForUser: vi.fn(() => [ - { id: 5, trip_id: 1, name: 'Hotel' }, - { id: 6, trip_id: 2, name: 'Food' }, - ]), - createCost: vi.fn((tripId: number, input: unknown) => ({ id: 9, trip_id: tripId, ...(input as object) })), - updateCost: vi.fn((tripId: number, itemId: number, input: unknown) => ({ id: itemId, trip_id: tripId, ...(input as object) })), - deleteCost: vi.fn(() => ({ deleted: true })), - // Planner writes — user 42 may edit trip 1 only (mirrors canAccessTrip). - canEditPlaces: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - createPlace: vi.fn((tripId: number, input: unknown) => ({ id: 10, trip_id: tripId, ...(input as object) })), - updatePlace: vi.fn((tripId: number, placeId: number, input: unknown) => ({ id: placeId, trip_id: tripId, ...(input as object) })), - deletePlace: vi.fn(() => ({ deleted: true })), - canEditDays: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - createDay: vi.fn((tripId: number, input: unknown) => ({ id: 20, trip_id: tripId, ...(input as object) })), - updateDay: vi.fn((tripId: number, dayId: number, input: unknown) => ({ id: dayId, trip_id: tripId, ...(input as object) })), - deleteDay: vi.fn(() => ({ deleted: true })), - assignPlaceToDay: vi.fn((tripId: number, dayId: number, placeId: number, notes: string | null) => ({ id: 30, day_id: dayId, place_id: placeId, notes })), - unassignPlace: vi.fn(() => ({ deleted: true })), - canEditTrip: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - updateTrip: vi.fn((tripId: number, _userId: number, input: unknown) => ({ id: tripId, ...(input as object) })), - // Cross-trip reads + reservations (bookings) — user 42 may edit trip 1 only. - listTripsForUser: vi.fn(() => [{ id: 1, title: 'Japan' }, { id: 2, title: 'Peru' }]), - listReservationsForUser: vi.fn(() => [{ id: 5, trip_id: 1, title: 'Hotel' }, { id: 6, trip_id: 2, title: 'Flight' }]), - canEditReservations: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - createReservation: vi.fn((tripId: number, input: unknown) => ({ id: 40, trip_id: tripId, ...(input as object) })), - updateReservation: vi.fn((tripId: number, reservationId: number, input: unknown) => ({ id: reservationId, trip_id: tripId, ...(input as object) })), - deleteReservation: vi.fn(() => ({ deleted: true })), - // Trip-scoped hydrated reads + accommodations (lodging blocks, day_edit-gated). - listTripDays: vi.fn((tripId: number) => [{ id: 3, trip_id: tripId, day_number: 1, assignments: [], notes_items: [] }]), - listTripReservations: vi.fn((tripId: number) => [{ id: 5, trip_id: tripId, title: 'Hotel', endpoints: [], day_positions: null }]), - listTripAccommodations: vi.fn((tripId: number) => [{ id: 11, trip_id: tripId, place_name: 'Ryokan' }]), - createAccommodation: vi.fn((tripId: number, input: unknown) => ({ id: 60, trip_id: tripId, ...(input as object) })), - updateAccommodation: vi.fn((tripId: number, accommodationId: number, input: unknown) => ({ id: accommodationId, trip_id: tripId, ...(input as object) })), - deleteAccommodation: vi.fn(() => ({ deleted: true })), - // User-scoped addon reads + day notes. - listJournalsForUser: vi.fn(() => [{ id: 1, title: 'Japan 2027' }]), - atlasVisitedForUser: vi.fn(() => ({ countries: [{ country_code: 'JP' }], regions: [] })), - vacayForUser: vi.fn(() => ({ plan: { id: 1 }, entries: [] })), - listCollectionsForUser: vi.fn(() => ({ collections: [{ id: 1, name: 'Tokyo eats' }] })), - getCollectionForUser: vi.fn((_userId: number, id: number) => ({ id, name: 'Tokyo eats', places: [] })), - listDayNotes: vi.fn((tripId: number, dayId: number) => [{ id: 1, day_id: dayId, trip_id: tripId, text: 'note' }]), - createDayNote: vi.fn((_tripId: number, dayId: number, input: unknown) => ({ id: 50, day_id: dayId, ...(input as object) })), - updateDayNote: vi.fn((_tripId: number, dayId: number, noteId: number, input: unknown) => ({ id: noteId, day_id: dayId, ...(input as object) })), - deleteDayNote: vi.fn(() => ({ deleted: true })), - canEditPacking: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - createPackingItem: vi.fn((tripId: number, input: unknown) => ({ id: 70, trip_id: tripId, ...(input as object) })), - updatePackingItem: vi.fn((tripId: number, itemId: number, input: unknown) => ({ id: itemId, trip_id: tripId, ...(input as object) })), - deletePackingItem: vi.fn(() => ({ deleted: true })), - listPackingBags: vi.fn(() => [{ id: 80, name: 'Backpack' }]), - createPackingBag: vi.fn((tripId: number, input: unknown) => ({ id: 80, trip_id: tripId, ...(input as object) })), - updatePackingBag: vi.fn((_tripId: number, bagId: number, input: unknown) => ({ id: bagId, ...(input as object) })), - deletePackingBag: vi.fn(() => ({ deleted: true })), - setPackingBagMembers: vi.fn((_tripId: number, bagId: number, userIds: number[]) => ({ bagId, members: userIds })), - getWeather: vi.fn(() => ({ temp: 20 })), - listCategories: vi.fn(() => [{ id: 1, name: 'Food' }]), - tripMembers: vi.fn(() => [{ id: 5, username: 'ada' }, { id: 6, username: 'bob' }]), - listTagsForUser: vi.fn((uid: number) => [{ id: 1, user_id: uid, name: 'work' }]), - createTagForUser: vi.fn((uid: number, name: string) => ({ id: 9, user_id: uid, name })), - updateTagForUser: vi.fn((_uid: number, tagId: number, name?: string) => ({ id: tagId, name })), - deleteTagForUser: vi.fn(() => ({ deleted: true })), - canUploadFiles: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - canEditFiles: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - canDeleteFiles: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - createTripFile: vi.fn((tripId: number, input: unknown, uid: number) => ({ id: 130, trip_id: tripId, uploaded_by: uid, ...(input as object) })), - createTripFileLink: vi.fn(() => [{ file_id: 130 }]), - updateTripFile: vi.fn((_tripId: number, fileId: number, input: unknown) => ({ id: fileId, ...(input as object) })), - softDeleteTripFile: vi.fn(() => ({ deleted: true })), - canEditCollab: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - createCollabNote: vi.fn((tripId: number, input: unknown, uid: number) => ({ id: 140, trip_id: tripId, created_by: uid, ...(input as object) })), - createCollabPoll: vi.fn((tripId: number, input: unknown) => ({ id: 141, trip_id: tripId, ...(input as object) })), - voteCollabPoll: vi.fn((_tripId: number, pollId: number) => ({ id: pollId, votes: 1 })), - createCollabMessage: vi.fn((tripId: number, text: string) => ({ id: 142, trip_id: tripId, text })), - canManageMembers: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - addTripMember: vi.fn((tripId: number, targetUserId: number, invitedBy: number) => ({ joined: true, tripId, targetUserId, invitedBy })), - removeTripMember: vi.fn((tripId: number, targetUserId: number) => ({ removed: true, tripId, targetUserId })), - canAccessTripForNotify: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - sendPluginNotification: vi.fn(async (pluginId: string, input: unknown) => ({ sent: true, pluginId, ...(input as object) })), - aiConfigured: vi.fn((userId: number) => userId === 42), - aiComplete: vi.fn(async (_uid: number, prompt: string) => ({ text: `echo:${prompt}` })), - aiExtract: vi.fn(async () => ({ results: [{ ok: true }] })), - getUserSetting: vi.fn((pluginId: string, userId: number, key: string) => (userId === 42 && key === 'apiKey' ? `secret-of-${pluginId}` : undefined)), - getOAuthToken: vi.fn(async (_pluginId: string, userId: number) => (userId === 42 ? 'access-token-xyz' : null)), - schedulerSet: vi.fn((name: string, dueAt: number, everyMs: number | undefined) => ({ scheduled: true, name, dueAt, everyMs })), - schedulerCancel: vi.fn((name: string) => ({ cancelled: name !== 'ghost' })), - createCollectionForUser: vi.fn((uid: number, input: unknown) => ({ id: 100, owner_id: uid, ...(input as object) })), - updateCollectionForUser: vi.fn((_uid: number, id: number, input: unknown) => ({ id, ...(input as object) })), - saveCollectionPlace: vi.fn((uid: number, input: unknown) => ({ id: 101, saved_by: uid, ...(input as object) })), - copyCollectionToTrip: vi.fn(() => ({ copied: 2, skipped: [] })), - deleteCollectionPlace: vi.fn(() => ({ deleted: true })), - markCountryVisited: vi.fn(() => ({ visited: true })), - unmarkCountryVisited: vi.fn(() => ({ visited: false })), - markRegionVisited: vi.fn(() => ({ visited: true })), - unmarkRegionVisited: vi.fn(() => ({ visited: false })), - createBucketItem: vi.fn((uid: number, input: unknown) => ({ id: 110, user_id: uid, ...(input as object) })), - deleteBucketItem: vi.fn(() => ({ deleted: true })), - vacayToggleEntry: vi.fn(() => ({ action: 'added' })), - vacayToggleCompanyHoliday: vi.fn(() => ({ action: 'added' })), - createJournalEntry: vi.fn((uid: number, journeyId: number, input: unknown) => ({ id: 120, journey_id: journeyId, created_by: uid, ...(input as object) })), - updateJournalEntry: vi.fn((_uid: number, entryId: number, input: unknown) => ({ id: entryId, ...(input as object) })), - deleteJournalEntry: vi.fn(() => ({ deleted: true })), - createJournal: vi.fn((uid: number, input: unknown) => ({ id: 130, user_id: uid, ...(input as object) })), - deleteJournal: vi.fn(() => ({ deleted: true })), - canEditTodos: vi.fn((tripId: number, userId: number) => tripId === 1 && userId === 42), - listTodos: vi.fn(() => [{ id: 1, name: 'Pack' }]), - createTodo: vi.fn((tripId: number, input: unknown) => ({ id: 90, trip_id: tripId, ...(input as object) })), - updateTodo: vi.fn((_tripId: number, todoId: number, input: unknown) => ({ id: todoId, ...(input as object) })), - deleteTodo: vi.fn(() => ({ deleted: true })), - // Metadata — trip 1 and place 7 resolve to trip 1 (accessible to 42); else undefined. - metaEntityTrip: vi.fn((entityType: string, entityId: number) => - (entityType === 'trip' && entityId === 1) || (entityType === 'place' && entityId === 7) || (entityType === 'day' && entityId === 3) - || (entityType === 'reservation' && entityId === 40) || (entityType === 'accommodation' && entityId === 11) ? 1 : undefined), - metaGet: vi.fn(() => ({ hello: 'world' })), - metaSet: vi.fn((_et: string, _eid: number, key: string, value: unknown) => ({ key, value })), - metaList: vi.fn(() => ({ a: 1 })), - metaDelete: vi.fn(() => ({ deleted: true })), - }; -} const req = (method: string, params: Record = {}): RpcRequest => ({ k: 'req', id: 'x', method, params }); const ok = (r: RpcResponse | RpcError): r is RpcResponse => r.ok === true; @@ -545,14 +391,7 @@ describe('PluginRpcHost — capability enforcement', () => { expect(ok(await c.dispatch(req('categories.list', {}), undefined))).toBe(true); }); - it('tags are the acting user\'s own; a userless context + empty name are refused', async () => { - const host = new PluginRpcHost('p', new Set(['db:read:tags', 'db:write:tags']), deps); - expect(ok(await host.dispatch(req('tags.list'), 42))).toBe(true); - expect(deps.listTagsForUser).toHaveBeenCalledWith(42); - expect(ok(await host.dispatch(req('tags.create', { input: { name: 'work' } }), 42))).toBe(true); - expect((await host.dispatch(req('tags.list'), undefined)).ok).toBe(false); - expect((await host.dispatch(req('tags.create', { input: { name: '' } }), 42)).ok).toBe(false); - }); + // The tags cases moved to tests/unit/tags/tags.rpc.test.ts with the handlers. it('trips.members + todos.list are trip-membership-gated', async () => { const host = new PluginRpcHost('p', new Set(['db:read:trips', 'db:read:todos']), deps); diff --git a/server/tests/unit/tags/tags.rpc.test.ts b/server/tests/unit/tags/tags.rpc.test.ts new file mode 100644 index 000000000..477893850 --- /dev/null +++ b/server/tests/unit/tags/tags.rpc.test.ts @@ -0,0 +1,186 @@ +/** + * The tags plugin surface, after it moved from the router's has() blocks onto + * @PluginMethod. The dispatch-level cases are the ones that used to live in + * rpc-host.test.ts: same requests, same expected codes. Only the construction line + * and the stub identity changed, which is the diff a reviewer should be reading. + * + * The rest are the per-handler cases the router-level test never had room for, in + * particular the ownership re-check on the two writes. + */ +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 { BadParams, ForbiddenResource } from '../../../src/nest/plugins/host/rpc-errors'; +import { TagsRpc } from '../../../src/nest/tags/tags.rpc'; +import { TagsModule } from '../../../src/nest/tags/tags.module'; +import type { TagsService } from '../../../src/nest/tags/tags.service'; +import type { PluginRpcContext } from '../../../src/nest/plugins/host/rpc-kit/types'; +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 }); +const ctx = (actingUserId: number | undefined): PluginRpcContext => ({ + pluginId: 'p', + actingUserId, + data: {} as PluginRpcContext['data'], +}); + +/** Tag 1 belongs to user 42, nothing else does. */ +function makeTags() { + return { + list: vi.fn((userId: number) => [{ id: 1, user_id: userId, name: 'work' }]), + getByIdAndUser: vi.fn((id: string | number, userId: number) => + Number(id) === 1 && userId === 42 ? { id: 1, user_id: 42, name: 'work' } : undefined, + ), + create: vi.fn((userId: number, name: string, color?: string) => ({ id: 9, user_id: userId, name, color })), + update: vi.fn((id: string | number, name?: string, color?: string) => ({ id, name, color })), + remove: vi.fn(), + } as unknown as TagsService & Record>; +} + +describe('TagsRpc through the router', () => { + let tags: ReturnType; + let host: PluginRpcHost; + + beforeEach(() => { + tags = makeTags(); + host = new PluginRpcHost( + 'p', + new Set(['db:read:tags', 'db:write:tags']), + makeDeps(), + createTestPluginRegistry([new TagsRpc(tags)]), + ); + }); + + it('TAGS-RPC-001 tags are the acting user\'s own; a userless context + empty name are refused', async () => { + expect((await host.dispatch(req('tags.list'), 42)).ok).toBe(true); + expect(tags.list).toHaveBeenCalledWith(42); + expect((await host.dispatch(req('tags.create', { input: { name: 'work' } }), 42)).ok).toBe(true); + expect((await host.dispatch(req('tags.list'), undefined)).ok).toBe(false); + expect((await host.dispatch(req('tags.create', { input: { name: '' } }), 42)).ok).toBe(false); + }); + + it('TAGS-RPC-002 a userless read is RESOURCE_FORBIDDEN and never touches the service', async () => { + const res = (await host.dispatch(req('tags.list'), undefined)) as RpcError; + expect(res.error.code).toBe('RESOURCE_FORBIDDEN'); + expect(res.error.message).toBe('tag reads require an authenticated user context'); + expect(tags.list).not.toHaveBeenCalled(); + }); + + it('TAGS-RPC-003 a blank name is BAD_PARAMS, not a refusal', async () => { + const res = (await host.dispatch(req('tags.create', { input: { name: ' ' } }), 42)) as RpcError; + expect(res.error.code).toBe('BAD_PARAMS'); + expect(res.error.message).toBe('tag name is required'); + expect(tags.create).not.toHaveBeenCalled(); + }); + + it('TAGS-RPC-004 editing another user\'s tag is RESOURCE_FORBIDDEN', async () => { + const res = (await host.dispatch(req('tags.update', { tagId: 2, input: { name: 'x' } }), 42)) as RpcError; + expect(res.error.code).toBe('RESOURCE_FORBIDDEN'); + expect(res.error.message).toBe('no tag 2 for this user'); + expect(tags.update).not.toHaveBeenCalled(); + }); + + it('TAGS-RPC-005 db:read:tags alone does not unlock a write', async () => { + const readOnly = new PluginRpcHost( + 'p', + new Set(['db:read:tags']), + makeDeps(), + createTestPluginRegistry([new TagsRpc(tags)]), + ); + const res = (await readOnly.dispatch(req('tags.create', { input: { name: 'work' } }), 42)) as RpcError; + expect(res.error.code).toBe('PERMISSION_DENIED'); + expect(tags.create).not.toHaveBeenCalled(); + }); + + it('TAGS-RPC-006 db:write:tags alone does not unlock the read', async () => { + const writeOnly = new PluginRpcHost( + 'p', + new Set(['db:write:tags']), + makeDeps(), + createTestPluginRegistry([new TagsRpc(tags)]), + ); + expect(((await writeOnly.dispatch(req('tags.list'), 42)) as RpcError).error.code).toBe('PERMISSION_DENIED'); + }); + + it('TAGS-RPC-007 with no tags grant at all, every method is PERMISSION_DENIED', async () => { + const none = new PluginRpcHost('p', new Set([]), makeDeps(), createTestPluginRegistry([new TagsRpc(tags)])); + for (const method of ['tags.list', 'tags.create', 'tags.update', 'tags.delete']) { + expect(((await none.dispatch(req(method, { tagId: 1 }), 42)) as RpcError).error.code).toBe('PERMISSION_DENIED'); + } + }); +}); + +describe('TagsRpc is reachable at all', () => { + it('TAGS-RPC-017 the class is listed in its module providers', () => { + // The one failure the compiler cannot see: a decorated class that no module + // registers is invisible to discovery, and every tags.* call then comes back + // PERMISSION_DENIED with no other symptom. Cheapest possible guard against it. + const providers = Reflect.getMetadata('providers', TagsModule) as unknown[] | undefined; + expect(providers).toContain(TagsRpc); + }); +}); + +describe('TagsRpc handlers', () => { + let tags: ReturnType; + let rpc: TagsRpc; + + beforeEach(() => { + tags = makeTags(); + rpc = new TagsRpc(tags); + }); + + it('TAGS-RPC-008 list returns the acting user\'s tags', () => { + expect(rpc.list({}, ctx(42))).toEqual([{ id: 1, user_id: 42, name: 'work' }]); + }); + + it('TAGS-RPC-009 create passes name and colour through', () => { + expect(rpc.create({ input: { name: 'work', color: '#abc' } }, ctx(42))).toEqual({ + id: 9, + user_id: 42, + name: 'work', + color: '#abc', + }); + }); + + it('TAGS-RPC-010 a non-string colour is dropped rather than passed on', () => { + rpc.create({ input: { name: 'work', color: 123 } }, ctx(42)); + expect(tags.create).toHaveBeenCalledWith(42, 'work', undefined); + }); + + it('TAGS-RPC-011 a non-object input is wrapped, so the name check still refuses it', () => { + expect(() => rpc.create({ input: 'work' }, ctx(42))).toThrow(new BadParams('tag name is required')); + }); + + it('TAGS-RPC-012 a userless write says "writes", not "reads"', () => { + expect(() => rpc.create({ input: { name: 'x' } }, ctx(undefined))).toThrow( + new ForbiddenResource('tag writes require an authenticated user context'), + ); + expect(() => rpc.update({ tagId: 1 }, ctx(undefined))).toThrow( + new ForbiddenResource('tag writes require an authenticated user context'), + ); + expect(() => rpc.delete({ tagId: 1 }, ctx(undefined))).toThrow( + new ForbiddenResource('tag writes require an authenticated user context'), + ); + }); + + it('TAGS-RPC-013 update only forwards the fields it was given', () => { + rpc.update({ tagId: 1, input: { name: 'renamed' } }, ctx(42)); + expect(tags.update).toHaveBeenCalledWith(1, 'renamed', undefined); + }); + + it('TAGS-RPC-014 a missing tagId is BAD_PARAMS before the ownership check runs', () => { + expect(() => rpc.update({ input: { name: 'x' } }, ctx(42))).toThrow(new BadParams('tagId must be a number')); + expect(tags.getByIdAndUser).not.toHaveBeenCalled(); + }); + + it('TAGS-RPC-015 delete re-checks ownership and reports the deletion', () => { + expect(rpc.delete({ tagId: 1 }, ctx(42))).toEqual({ deleted: true }); + expect(tags.remove).toHaveBeenCalledWith(1); + }); + + it('TAGS-RPC-016 deleting another user\'s tag never reaches remove()', () => { + expect(() => rpc.delete({ tagId: 2 }, ctx(42))).toThrow(new ForbiddenResource('no tag 2 for this user')); + expect(tags.remove).not.toHaveBeenCalled(); + }); +});