Lift the plugin RPC resource gates into an injectable

A decorated handler cannot reach the router's private tripRead / requireActor / requireTripEdit, nor the canEditTripAs and requireAddon that live inside the deps factory's closure. PluginGuards holds all of them so a *.rpc.ts class can inject what it needs. Every message is copied character for character, including the two asymmetries that look like typos and are not: a trip read says "trip reads", while requireActor appends the word "writes" to whatever noun it is given, and some domains throw their own inline variant rather than calling it at all. The tests pin the exact strings, not just the refusal, because shipped plugins read them.
2026-10-18 21:16:47 +00:00 · 2026-08-09 11:36:04 +02:00
parent 7b11680cc3
commit 6b6fbbd94c
2 changed files with 264 additions and 0 deletions
@@ -0,0 +1,98 @@
+import { Injectable } from '@nestjs/common';
+import { DatabaseService } from '../../database/database.service';
+import { PermissionsService } from '../../permissions/permissions.service';
+import { AddonsService } from '../../addons/addons.service';
+import { BadParams, ForbiddenResource } from './rpc-errors';
+import { num } from './rpc-params';
+import type { PluginRpcContext } from './rpc-kit/types';
+
+/**
+ * The resource gates every plugin RPC handler needs, lifted out of PluginRpcHost's
+ * private methods and PluginHostDepsFactory's canEditTripAs / requireAddon so that
+ * decorated *.rpc.ts handlers get them by injection instead of through a closure.
+ *
+ * Every message string is character-identical to what the legacy router produced.
+ * Do not "improve" one: rpc-host.test.ts asserts them, and shipped plugins read
+ * them. Note in particular that trip READS say "trip reads" while requireActor
+ * appends the word "writes", and that some domains throw their own inline variant
+ * rather than calling requireActor at all.
+ */
+@Injectable()
+export class PluginGuards {
+  constructor(
+    private readonly db: DatabaseService,
+    private readonly permissions: PermissionsService,
+    private readonly addons: AddonsService,
+  ) {}
+
+  /**
+   * Membership-check a trip read against the HOST-bound acting user. The acting user
+   * comes from the supervisor's invocation map, never from a plugin-supplied field,
+   * so a plugin cannot read another user's trips by naming their id. A userless
+   * context (a job, or onLoad) has no acting user and is refused.
+   */
+  tripRead<T>(params: Record<string, unknown>, ctx: PluginRpcContext, read: (userId: number) => T): T {
+    const tripId = num(params.tripId, 'tripId');
+    if (ctx.actingUserId === undefined) {
+      throw new ForbiddenResource('trip reads require an authenticated user context');
+    }
+    if (!this.db.canAccessTrip(tripId, ctx.actingUserId)) {
+      throw new ForbiddenResource(`no access to trip ${tripId}`);
+    }
+    // The read runs only for a bound, membership-checked user, so hand the id through
+    // and per-user visibility filters (packing's private items) still apply.
+    return read(ctx.actingUserId);
+  }
+
+  /**
+   * Every write needs a HOST-bound acting user. `noun` is the domain word, and the
+   * message appends "writes" to it, so pass 'tag', not 'tag writes'.
+   */
+  requireActor(ctx: PluginRpcContext, noun: string): number {
+    if (ctx.actingUserId === undefined) {
+      throw new ForbiddenResource(`${noun} writes require an authenticated user context`);
+    }
+    return ctx.actingUserId;
+  }
+
+  /** A write is allowed only if the acting user can access AND edit the trip. */
+  requireTripEdit(tripId: number, userId: number, action: string): void {
+    if (!this.db.canAccessTrip(tripId, userId)) throw new ForbiddenResource(`no access to trip ${tripId}`);
+    if (!this.canEditAs(action, tripId, userId)) throw new ForbiddenResource(`no permission to edit trip ${tripId}`);
+  }
+
+  /**
+   * The trip-access + role gate every planner write uses, mirroring the app's
+   * per-domain canEdit. Returns false and never throws, so the caller decides which
+   * message the refusal carries.
+   */
+  canEditAs(action: string, tripId: number, userId: number): boolean {
+    const trip = this.db.canAccessTrip(tripId, userId);
+    if (!trip) return false;
+    const user = this.db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined;
+    if (!user) return false;
+    return this.permissions.checkPermission(action, user.role ?? 'user', trip.user_id, userId, trip.user_id !== userId);
+  }
+
+  /**
+   * A subsystem read is refused when its addon is off, matching the app, where a
+   * disabled addon means there is simply nothing to read.
+   */
+  requireAddon(addonId: string, noun: string): void {
+    if (!this.addons.isAddonEnabled(addonId)) throw new ForbiddenResource(`the ${noun} addon is disabled`);
+  }
+
+  /**
+   * The @trek/shared write schemas carry no string-length caps, so mirror the ones
+   * the REST controllers add. Without this a plugin could write a field the web app
+   * would reject with 400.
+   */
+  capStrings(input: Record<string, unknown>, limits: Record<string, number>): void {
+    for (const [field, max] of Object.entries(limits)) {
+      const value = input[field];
+      if (typeof value === 'string' && value.length > max) {
+        throw new BadParams(`${field} must be ${max} characters or fewer`);
+      }
+    }
+  }
+}
@@ -0,0 +1,166 @@
+/**
+ * PluginGuards holds the resource gates the decorated *.rpc.ts handlers will use from
+ * the first rollout PR onward. Its messages are copied character-for-character from
+ * the legacy router, because rpc-host.test.ts asserts them and shipped plugins read
+ * them, so these tests pin the exact strings rather than just the refusal.
+ */
+import { describe, it, expect, vi } from 'vitest';
+import { PluginGuards } from '../../../src/nest/plugins/host/plugin-guards.service';
+import { BadParams, ForbiddenResource } from '../../../src/nest/plugins/host/rpc-errors';
+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 { PluginRpcContext } from '../../../src/nest/plugins/host/rpc-kit/types';
+
+const ctx = (actingUserId: number | undefined): PluginRpcContext => ({
+  pluginId: 'p',
+  actingUserId,
+  data: {} as PluginRpcContext['data'],
+});
+
+/** Trip 1 belongs to user 42 and only user 42 may reach it, mirroring rpc-host.test.ts. */
+function build(overrides: { role?: string | undefined; allow?: boolean; addonOn?: boolean } = {}) {
+  const db = {
+    canAccessTrip: vi.fn((tripId: number, userId: number) =>
+      tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined,
+    ),
+    prepare: vi.fn(() => ({
+      get: () => ('role' in overrides ? (overrides.role === undefined ? undefined : { role: overrides.role }) : { role: 'user' }),
+    })),
+  } as unknown as DatabaseService;
+  const permissions = {
+    checkPermission: vi.fn(() => overrides.allow ?? true),
+  } as unknown as PermissionsService;
+  const addons = {
+    isAddonEnabled: vi.fn(() => overrides.addonOn ?? true),
+  } as unknown as AddonsService;
+  return { guards: new PluginGuards(db, permissions, addons), db, permissions, addons };
+}
+
+describe('PluginGuards — tripRead', () => {
+  it('PGUARD-001 runs the read for a member and hands it the acting user', () => {
+    const { guards } = build();
+    const read = vi.fn((userId: number) => ({ seenBy: userId }));
+    expect(guards.tripRead({ tripId: 1 }, ctx(42), read)).toEqual({ seenBy: 42 });
+    expect(read).toHaveBeenCalledWith(42);
+  });
+
+  it('PGUARD-002 a userless context is refused before the read runs', () => {
+    const { guards } = build();
+    const read = vi.fn();
+    expect(() => guards.tripRead({ tripId: 1 }, ctx(undefined), read)).toThrow(
+      new ForbiddenResource('trip reads require an authenticated user context'),
+    );
+    expect(read).not.toHaveBeenCalled();
+  });
+
+  it('PGUARD-003 a non-member is refused, naming the trip', () => {
+    const { guards } = build();
+    const read = vi.fn();
+    expect(() => guards.tripRead({ tripId: 2 }, ctx(42), read)).toThrow(
+      new ForbiddenResource('no access to trip 2'),
+    );
+    expect(read).not.toHaveBeenCalled();
+  });
+
+  it('PGUARD-004 a missing tripId is BAD_PARAMS, not a refusal', () => {
+    const { guards } = build();
+    expect(() => guards.tripRead({}, ctx(42), vi.fn())).toThrow(new BadParams('tripId must be a number'));
+  });
+
+  it('PGUARD-005 a numeric string tripId is accepted, as the wire contract allows', () => {
+    const { guards } = build();
+    expect(guards.tripRead({ tripId: '1' }, ctx(42), (u) => u)).toBe(42);
+  });
+});
+
+describe('PluginGuards — requireActor', () => {
+  it('PGUARD-006 returns the acting user', () => {
+    expect(build().guards.requireActor(ctx(42), 'tag')).toBe(42);
+  });
+
+  it('PGUARD-007 appends the word "writes" to the noun, verbatim as the router did', () => {
+    expect(() => build().guards.requireActor(ctx(undefined), 'tag')).toThrow(
+      new ForbiddenResource('tag writes require an authenticated user context'),
+    );
+  });
+});
+
+describe('PluginGuards — requireTripEdit and canEditAs', () => {
+  it('PGUARD-008 passes when the user may access and edit', () => {
+    const { guards, permissions } = build({ allow: true });
+    expect(() => guards.requireTripEdit(1, 42, 'trip_edit')).not.toThrow();
+    expect(permissions.checkPermission).toHaveBeenCalledWith('trip_edit', 'user', 42, 42, false);
+  });
+
+  it('PGUARD-009 no access wins over no permission, and names the trip', () => {
+    const { guards } = build({ allow: false });
+    expect(() => guards.requireTripEdit(2, 42, 'trip_edit')).toThrow(
+      new ForbiddenResource('no access to trip 2'),
+    );
+  });
+
+  it('PGUARD-010 access without the edit permission is a different message', () => {
+    const { guards } = build({ allow: false });
+    expect(() => guards.requireTripEdit(1, 42, 'trip_edit')).toThrow(
+      new ForbiddenResource('no permission to edit trip 1'),
+    );
+  });
+
+  it('PGUARD-011 canEditAs returns false rather than throwing when there is no access', () => {
+    expect(build().guards.canEditAs('trip_edit', 2, 42)).toBe(false);
+  });
+
+  it('PGUARD-012 canEditAs returns false when the user row is gone', () => {
+    expect(build({ role: undefined }).guards.canEditAs('trip_edit', 1, 42)).toBe(false);
+  });
+
+  it('PGUARD-013 a missing role column falls back to "user"', () => {
+    const { guards, permissions } = build({ role: null as unknown as string });
+    guards.canEditAs('trip_edit', 1, 42);
+    expect(permissions.checkPermission).toHaveBeenCalledWith('trip_edit', 'user', 42, 42, false);
+  });
+
+  it('PGUARD-014 a non-owner member is flagged as shared', () => {
+    const db = {
+      canAccessTrip: vi.fn(() => ({ id: 1, user_id: 7 })),
+      prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })),
+    } as unknown as DatabaseService;
+    const permissions = { checkPermission: vi.fn(() => true) } as unknown as PermissionsService;
+    const guards = new PluginGuards(db, permissions, {} as AddonsService);
+    guards.canEditAs('trip_edit', 1, 42);
+    expect(permissions.checkPermission).toHaveBeenCalledWith('trip_edit', 'user', 7, 42, true);
+  });
+});
+
+describe('PluginGuards — requireAddon', () => {
+  it('PGUARD-015 an enabled addon passes', () => {
+    expect(() => build({ addonOn: true }).guards.requireAddon('budget', 'costs')).not.toThrow();
+  });
+
+  it('PGUARD-016 a disabled addon is refused with the noun in the message', () => {
+    expect(() => build({ addonOn: false }).guards.requireAddon('budget', 'costs')).toThrow(
+      new ForbiddenResource('the costs addon is disabled'),
+    );
+  });
+});
+
+describe('PluginGuards — capStrings', () => {
+  it('PGUARD-017 a field within the cap passes', () => {
+    expect(() => build().guards.capStrings({ name: 'ok' }, { name: 200 })).not.toThrow();
+  });
+
+  it('PGUARD-018 an oversized field is BAD_PARAMS, naming field and cap', () => {
+    expect(() => build().guards.capStrings({ name: 'x'.repeat(201) }, { name: 200 })).toThrow(
+      new BadParams('name must be 200 characters or fewer'),
+    );
+  });
+
+  it('PGUARD-019 a non-string value is left alone', () => {
+    expect(() => build().guards.capStrings({ name: 12345 }, { name: 2 })).not.toThrow();
+  });
+
+  it('PGUARD-020 an absent field is left alone', () => {
+    expect(() => build().guards.capStrings({}, { name: 1 })).not.toThrow();
+  });
+});