diff --git a/server/src/nest/backup/backup.impl.ts b/server/src/nest/backup/backup.impl.ts index 00b8d74b6..c78f28ad6 100644 --- a/server/src/nest/backup/backup.impl.ts +++ b/server/src/nest/backup/backup.impl.ts @@ -6,7 +6,7 @@ import fs from 'fs'; import Database from 'better-sqlite3'; import { db, closeDb, reinitialize } from '../../db/database'; import { VALID_INTERVALS } from './auto-backup.settings'; -import { invalidatePermissionsCache } from '../permissions/permissions.bridge'; +import { invalidatePermissionsCache } from '../permissions/permissions-cache'; import { pluginsCodeRoot, pluginsDataRoot } from '../plugins/paths'; import { stageExtractedPluginTrees, applyStagedRestoreNow } from '../plugins/plugin-backup'; import { snapshotAllPluginDataDbs } from '../plugins/host/plugin-data.service'; diff --git a/server/src/nest/permissions/permissions-cache.ts b/server/src/nest/permissions/permissions-cache.ts new file mode 100644 index 000000000..6557b6a09 --- /dev/null +++ b/server/src/nest/permissions/permissions-cache.ts @@ -0,0 +1,27 @@ +import type { PermissionLevel } from './permissions.service'; + +/** + * The permissions cache — module state on purpose, in its own home (the + * oauth.pending-codes.ts precedent) so every path that must see one shared + * cache actually does: the container PermissionsService singleton, the + * permissions.bridge instance the MCP _shared helper reaches, and the plain + * backup restore path (backup.impl.ts), which is free functions by design and + * has nothing to hang an injected service off. A restore rewrites + * app_settings under the request path's feet; invalidating here is what keeps + * every reader consistent afterwards. + */ +let cache: Map | null = null; + +export function getPermissionsCache(): Map | null { + return cache; +} + +/** Install a fresh cache map and return it (the loader fills it in place). */ +export function setPermissionsCache(next: Map): Map { + cache = next; + return next; +} + +export function invalidatePermissionsCache(): void { + cache = null; +} diff --git a/server/src/nest/permissions/permissions.bridge.ts b/server/src/nest/permissions/permissions.bridge.ts index 16fdde12c..f19ad0257 100644 --- a/server/src/nest/permissions/permissions.bridge.ts +++ b/server/src/nest/permissions/permissions.bridge.ts @@ -9,19 +9,15 @@ import { PermissionsService } from './permissions.service'; * Inside the container, inject PermissionsService instead. Delete this file * when the MCP/OAuth mount moves behind the container. * - * The permissions cache is module-scoped in permissions.service.ts, so this - * instance and the container singleton share one cache — invalidating through - * either flushes both (backup restores depend on this). + * The permissions cache is module-scoped in permissions-cache.ts, so this + * instance and the container singleton share one cache (backup restores + * invalidate it through that module directly). * * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto * the shared better-sqlite3 singleton. */ const permissions = new PermissionsService(new DatabaseService(db)); -export function invalidatePermissionsCache(): void { - permissions.invalidatePermissionsCache(); -} - export function checkPermission( actionKey: string, userRole: string, diff --git a/server/src/nest/permissions/permissions.service.ts b/server/src/nest/permissions/permissions.service.ts index 291875ce7..e71b5ba96 100644 --- a/server/src/nest/permissions/permissions.service.ts +++ b/server/src/nest/permissions/permissions.service.ts @@ -1,6 +1,11 @@ import { Injectable } from '@nestjs/common'; import { DatabaseService } from '../database/database.service'; import { logError } from '../audit/audit-log.logger'; +import { + getPermissionsCache, + setPermissionsCache, + invalidatePermissionsCache as invalidateSharedCache, +} from './permissions-cache'; /** * Permission levels (hierarchical, higher includes lower): @@ -58,21 +63,22 @@ export const PERMISSION_ACTIONS: PermissionAction[] = [ const ACTIONS_MAP = new Map(PERMISSION_ACTIONS.map(a => [a.key, a])); -// In-memory cache, invalidated on save. Deliberately MODULE-scoped, not -// instance state: two instances exist at runtime (the container singleton and -// the permissions.bridge instance for out-of-container consumers), and legacy -// callers — backupService invalidating after a restore, integration tests — -// must flush the same cache the request path reads. Both instances wrap the -// same shared connection, so a single cache is also the correct data shape. -let cache: Map | null = null; +// The in-memory cache is deliberately MODULE-scoped, not instance state, and +// lives in ./permissions-cache: two service instances exist at runtime (the +// container singleton and the permissions.bridge instance for +// out-of-container consumers), and the backup restore path — plain functions, +// no DI — must flush the same cache the request path reads. Both instances +// wrap the same shared connection, so a single cache is also the correct +// data shape. @Injectable() export class PermissionsService { constructor(private readonly dbs: DatabaseService) {} private loadPermissions(): Map { - if (cache) return cache; - cache = new Map(); + const cached = getPermissionsCache(); + if (cached) return cached; + const cache = setPermissionsCache(new Map()); try { const rows = this.dbs.all<{ key: string; value: string }>( "SELECT key, value FROM app_settings WHERE key LIKE 'perm_%'" @@ -98,7 +104,7 @@ export class PermissionsService { } invalidatePermissionsCache(): void { - cache = null; + invalidateSharedCache(); } getPermissionLevel(actionKey: string): PermissionLevel { diff --git a/server/src/nest/system-notices/system-notices.module.ts b/server/src/nest/system-notices/system-notices.module.ts index 3a6761058..1255285a8 100644 --- a/server/src/nest/system-notices/system-notices.module.ts +++ b/server/src/nest/system-notices/system-notices.module.ts @@ -1,9 +1,11 @@ import { Module } from '@nestjs/common'; +import { AddonsModule } from '../addons/addons.module'; import { SystemNoticesController } from './system-notices.controller'; import { SystemNoticesService } from './system-notices.service'; -/** System-notices domain (L2 leaf module). Registered in AppModule. */ +/** System-notices domain. Registered in AppModule. */ @Module({ + imports: [AddonsModule], controllers: [SystemNoticesController], providers: [SystemNoticesService], }) diff --git a/server/src/nest/system-notices/system-notices.service.ts b/server/src/nest/system-notices/system-notices.service.ts index 872dd6990..f50dd64da 100644 --- a/server/src/nest/system-notices/system-notices.service.ts +++ b/server/src/nest/system-notices/system-notices.service.ts @@ -1,16 +1,21 @@ import { Injectable } from '@nestjs/common'; import type { SystemNoticeDto } from '@trek/shared'; import { getActiveNoticesFor, dismissNotice } from '../../systemNotices/service'; +import { AddonsService } from '../addons/addons.service'; /** * Thin Nest wrapper around the existing system-notices service. The condition * evaluation, version gating, sorting and dismissal persistence all stay in the - * upstream service — this only adapts it for DI, so behaviour is unchanged. + * upstream service — this only adapts it for DI (and threads the injected + * addon-enablement check into the condition context, so the plain modules + * underneath carry no bridge import), so behaviour is unchanged. */ @Injectable() export class SystemNoticesService { + constructor(private readonly addons: AddonsService) {} + getActiveFor(userId: number): SystemNoticeDto[] { - return getActiveNoticesFor(userId) as SystemNoticeDto[]; + return getActiveNoticesFor(userId, (addonId) => this.addons.isAddonEnabled(addonId)) as SystemNoticeDto[]; } dismiss(userId: number, noticeId: string): boolean { diff --git a/server/src/systemNotices/conditions.ts b/server/src/systemNotices/conditions.ts index a3a1f3c7d..2dbff5ff8 100644 --- a/server/src/systemNotices/conditions.ts +++ b/server/src/systemNotices/conditions.ts @@ -1,11 +1,15 @@ import semver from 'semver'; -import { isAddonEnabled } from '../nest/addons/addons.bridge.js'; import type { NoticeCondition, SystemNotice } from './types.js'; interface ConditionContext { user: { login_count: number; first_seen_version: string; role: string; noTrips: number }; currentAppVersion: string; now: Date; + /** + * Addon-enablement check, threaded in by the caller (the Nest service passes + * its injected AddonsService) so this plain module carries no bridge import. + */ + addonEnabled: (addonId: string) => boolean; } // Custom predicate registry — extensible without modifying this file @@ -48,7 +52,7 @@ function evaluateOne(condition: NoticeCondition, ctx: ConditionContext): boolean return condition.roles.includes(ctx.user.role as 'admin' | 'user'); case 'addonEnabled': - return isAddonEnabled(condition.addonId); + return ctx.addonEnabled(condition.addonId); case 'custom': { const fn = customPredicates.get(condition.id); diff --git a/server/src/systemNotices/service.ts b/server/src/systemNotices/service.ts index d5eb727d4..16f10f458 100644 --- a/server/src/systemNotices/service.ts +++ b/server/src/systemNotices/service.ts @@ -36,7 +36,10 @@ function severityWeight(s: string): number { return s === 'critical' ? 2 : s === 'warn' ? 1 : 0; } -export function getActiveNoticesFor(userId: number): SystemNoticeDTO[] { +export function getActiveNoticesFor( + userId: number, + addonEnabled: (addonId: string) => boolean +): SystemNoticeDTO[] { const user = db.prepare( 'SELECT login_count, first_seen_version, role FROM users WHERE id = ?' ).get(userId) as { login_count: number; first_seen_version: string; role: string } | undefined; @@ -56,7 +59,7 @@ export function getActiveNoticesFor(userId: number): SystemNoticeDTO[] { const now = new Date(); const currentAppVersion = getCurrentAppVersion(); - const ctx = { user: { ...user, noTrips: tripCount }, currentAppVersion, now }; + const ctx = { user: { ...user, noTrips: tripCount }, currentAppVersion, now, addonEnabled }; const appVer = semver.coerce(currentAppVersion)?.version ?? '0.0.0'; const isStillDismissed = (n: SystemNotice): boolean => { diff --git a/server/tests/e2e/system-notices.e2e.test.ts b/server/tests/e2e/system-notices.e2e.test.ts index 34adda5ac..cdbca93c5 100644 --- a/server/tests/e2e/system-notices.e2e.test.ts +++ b/server/tests/e2e/system-notices.e2e.test.ts @@ -30,6 +30,7 @@ vi.mock('../../src/systemNotices/service', () => ({ })); import { SystemNoticesModule } from '../../src/nest/system-notices/system-notices.module'; +import { DatabaseModule } from '../../src/nest/database/database.module'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; const notice = { @@ -42,7 +43,10 @@ describe('System-notices e2e (real auth guard + temp SQLite)', () => { let app: Awaited>; async function build() { - const moduleRef = await Test.createTestingModule({ imports: [SystemNoticesModule] }).compile(); + // DatabaseModule is @Global in the real app; a partial graph has to + // provide it for SystemNoticesModule's AddonsModule import (the + // addons.e2e precedent). + const moduleRef = await Test.createTestingModule({ imports: [DatabaseModule, SystemNoticesModule] }).compile(); const nest = moduleRef.createNestApplication(); nest.use(cookieParser()); nest.useGlobalFilters(new TrekExceptionFilter()); @@ -71,7 +75,7 @@ describe('System-notices e2e (real auth guard + temp SQLite)', () => { const res = await request(server).get('/api/system-notices/active').set('Cookie', sessionCookie(1)); expect(res.status).toBe(200); expect(res.body).toEqual([notice]); - expect(mockGetActive).toHaveBeenCalledWith(1); + expect(mockGetActive).toHaveBeenCalledWith(1, expect.any(Function)); }); it('204 with no body on a successful dismiss', async () => { diff --git a/server/tests/integration/budget.test.ts b/server/tests/integration/budget.test.ts index 92db6ed9c..a7b4c19ea 100644 --- a/server/tests/integration/budget.test.ts +++ b/server/tests/integration/budget.test.ts @@ -424,7 +424,7 @@ describe('Reorder budget items', () => { // Restrict budget_edit to trip_owner only testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('perm_budget_edit', 'trip_owner')").run(); - const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions.bridge'); + const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions-cache'); invalidatePermissionsCache(); const res = await request(app) @@ -527,7 +527,7 @@ describe('Budget edit permission enforcement', () => { const trip = createTrip(testDb, owner.id); addTripMember(testDb, trip.id, member.id); - const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions.bridge'); + const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions-cache'); testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('perm_budget_edit', 'trip_owner')").run(); invalidatePermissionsCache(); @@ -548,7 +548,7 @@ describe('Budget edit permission enforcement', () => { addTripMember(testDb, trip.id, member.id); createBudgetItem(testDb, trip.id, { name: 'Item', category: 'Transport' }); - const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions.bridge'); + const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions-cache'); testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('perm_budget_edit', 'trip_owner')").run(); invalidatePermissionsCache(); diff --git a/server/tests/integration/journey.test.ts b/server/tests/integration/journey.test.ts index 7f912d71f..af2c9f11c 100644 --- a/server/tests/integration/journey.test.ts +++ b/server/tests/integration/journey.test.ts @@ -72,7 +72,7 @@ import { addJourneyContributor, } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; -import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions.bridge'; +import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions-cache'; let nestApp: INestApplication; let app: Application; diff --git a/server/tests/integration/places.test.ts b/server/tests/integration/places.test.ts index 8a82b0b5a..7223d6ce7 100644 --- a/server/tests/integration/places.test.ts +++ b/server/tests/integration/places.test.ts @@ -56,7 +56,7 @@ import { resetTestDb, resetRateLimits } from '../helpers/test-db'; import { createUser, createAdmin, createTrip, createPlace, addTripMember } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; import { PlacesService } from '../../src/nest/places/places.service'; -import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions.bridge'; +import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions-cache'; let nestApp: INestApplication; let app: Application; diff --git a/server/tests/integration/share.test.ts b/server/tests/integration/share.test.ts index 42d89dd56..b8484d8cf 100644 --- a/server/tests/integration/share.test.ts +++ b/server/tests/integration/share.test.ts @@ -307,7 +307,7 @@ describe('Shared trip access', () => { addTripMember(testDb, trip.id, member.id); await request(app).post(`/api/trips/${trip.id}/share-link`).set('Cookie', authCookie(owner.id)).send({}); - const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions.bridge'); + const { invalidatePermissionsCache } = await import('../../src/nest/permissions/permissions-cache'); testDb.prepare("INSERT OR REPLACE INTO app_settings (key, value) VALUES ('perm_share_manage', 'trip_member')").run(); invalidatePermissionsCache(); try { diff --git a/server/tests/integration/todo.test.ts b/server/tests/integration/todo.test.ts index 168bb2449..c5bad7e9f 100644 --- a/server/tests/integration/todo.test.ts +++ b/server/tests/integration/todo.test.ts @@ -44,7 +44,7 @@ import { runMigrations } from '../../src/db/migrations'; import { resetTestDb, resetRateLimits } from '../helpers/test-db'; import { createUser, createTrip, addTripMember } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; -import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions.bridge'; +import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions-cache'; let nestApp: INestApplication; let app: Application; diff --git a/server/tests/integration/trips.test.ts b/server/tests/integration/trips.test.ts index aa473aad6..4fd5d5c6e 100644 --- a/server/tests/integration/trips.test.ts +++ b/server/tests/integration/trips.test.ts @@ -57,7 +57,7 @@ import { runMigrations } from '../../src/db/migrations'; import { resetTestDb, resetRateLimits } from '../helpers/test-db'; import { createUser, createAdmin, createTrip, addTripMember, createPlace, createReservation, createTag, createDayAccommodation, createBudgetItem, createPackingItem, createDayNote, createDayAssignment } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; -import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions.bridge'; +import { invalidatePermissionsCache } from '../../src/nest/permissions/permissions-cache'; let nestApp: INestApplication; let app: Application; diff --git a/server/tests/unit/mcp/tools-transit.test.ts b/server/tests/unit/mcp/tools-transit.test.ts index b29a3573c..2ef2cda1f 100644 --- a/server/tests/unit/mcp/tools-transit.test.ts +++ b/server/tests/unit/mcp/tools-transit.test.ts @@ -1,6 +1,6 @@ import { runMigrations } from '../../../src/db/migrations'; import { createTables } from '../../../src/db/schema'; -import { invalidatePermissionsCache } from '../../../src/nest/permissions/permissions.bridge'; +import { invalidatePermissionsCache } from '../../../src/nest/permissions/permissions-cache'; import { PermissionsService } from '../../../src/nest/permissions/permissions.service'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { addTripMember, createDay, createTrip, createUser } from '../../helpers/factories'; diff --git a/server/tests/unit/nest/permissions.service.test.ts b/server/tests/unit/nest/permissions.service.test.ts index fd9c32d23..665b8a717 100644 --- a/server/tests/unit/nest/permissions.service.test.ts +++ b/server/tests/unit/nest/permissions.service.test.ts @@ -43,10 +43,8 @@ import { runMigrations } from '../../../src/db/migrations'; import { logError } from '../../../src/nest/audit/audit-log.logger'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { PermissionsService, PERMISSION_ACTIONS } from '../../../src/nest/permissions/permissions.service'; -import { - checkPermission as bridgeCheckPermission, - invalidatePermissionsCache as bridgeInvalidatePermissionsCache, -} from '../../../src/nest/permissions/permissions.bridge'; +import { checkPermission as bridgeCheckPermission } from '../../../src/nest/permissions/permissions.bridge'; +import { invalidatePermissionsCache as invalidateSharedCache } from '../../../src/nest/permissions/permissions-cache'; const svc = new PermissionsService(new DatabaseService(testDb)); @@ -253,11 +251,12 @@ describe('permissions.bridge delegation', () => { // (checkPermission for a plain member flips with the stored level). svc.savePermissions({ trip_edit: 'trip_member' }); expect(bridgeCheckPermission('trip_edit', 'user', 10, 20, true)).toBe(true); - // Raw SQL write, then invalidate through the BRIDGE — the DI instance - // must serve the fresh value (backup-restore relies on exactly this). + // Raw SQL write, then invalidate through permissions-cache — the plain + // function backup.impl.ts calls after a restore. Both service instances + // must serve the fresh value afterwards. testDb.prepare('INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)').run('perm_trip_edit', 'trip_owner'); expect(svc.getPermissionLevel('trip_edit')).toBe('trip_member'); // still cached - bridgeInvalidatePermissionsCache(); + invalidateSharedCache(); expect(svc.getPermissionLevel('trip_edit')).toBe('trip_owner'); expect(bridgeCheckPermission('trip_edit', 'user', 10, 20, true)).toBe(false); }); diff --git a/server/tests/unit/systemNotices/conditions.test.ts b/server/tests/unit/systemNotices/conditions.test.ts index 2aa214295..2a9d108f6 100644 --- a/server/tests/unit/systemNotices/conditions.test.ts +++ b/server/tests/unit/systemNotices/conditions.test.ts @@ -25,6 +25,9 @@ const baseCtx = { user: { login_count: 5, first_seen_version: '1.0.0', role: 'user', noTrips: 1 }, currentAppVersion: '2.0.0', now: new Date('2026-06-01T00:00:00Z'), + // Threaded in by the caller since the addons.bridge import left this module; + // false keeps every case below reading as it did. + addonEnabled: () => false, }; describe('firstLogin', () => { @@ -100,3 +103,12 @@ describe('empty conditions', () => { expect(evaluate(baseNotice, baseCtx)).toBe(true); }); }); + +describe('addonEnabled', () => { + it('asks the threaded-in check with the condition addon id', () => { + const notice = noticeWith({ kind: 'addonEnabled', addonId: 'journey' }); + expect(evaluate(notice, { ...baseCtx, addonEnabled: (id) => id === 'journey' })).toBe(true); + expect(evaluate(notice, { ...baseCtx, addonEnabled: (id) => id === 'vacay' })).toBe(false); + expect(evaluate(notice, baseCtx)).toBe(false); + }); +});