From cece6509b9f7c40e20bf3c936835ea9a57a9a089 Mon Sep 17 00:00:00 2001 From: jubnl Date: Sun, 26 Jul 2026 22:23:57 +0200 Subject: [PATCH] chore: move settings backend to proper DI Folds the legacy services/settingsService SQL into the Nest SettingsService (byte-identical statements; BEGIN/COMMIT blocks become db.transaction()). AdminService and ShareService inject it; llm-config.resolver becomes the injectable LlmConfigResolver (SettingsService + DatabaseService), injected by LlmParseService and PluginHostDepsFactory. No MCP surface, no bridge. Whole-file commit, so this also carries the post-migration quirk fixes that live in the same files: null serializes as '' (a stored "null" leaked out of getDecryptedUserSetting as the literal string), bulk upserts skip the masked sentinel (mirroring the single-upsert no-op) and return the written count. Unit suite moved to tests/unit/nest with SET-SVC-001..026 preserved, 027..030 pin the fixes; settings/admin e2e converted to DI-native. --- migrate.md | 2 +- server/src/nest/README.md | 12 +- server/src/nest/admin/admin.module.ts | 3 +- server/src/nest/admin/admin.service.ts | 8 +- .../src/nest/llm-parse/llm-config.resolver.ts | 108 ++++---- server/src/nest/llm-parse/llm-parse.module.ts | 12 +- .../src/nest/llm-parse/llm-parse.service.ts | 8 +- server/src/nest/plugins/DI-MIGRATION.md | 11 +- .../plugins/host/plugin-host-deps.factory.ts | 129 +++++----- server/src/nest/plugins/plugins.module.ts | 3 +- server/src/nest/settings/settings.module.ts | 2 + server/src/nest/settings/settings.service.ts | 233 +++++++++++++++++- server/src/nest/share/share.bridge.ts | 3 +- server/src/nest/share/share.module.ts | 2 + server/src/nest/share/share.service.ts | 9 +- server/src/services/settingsService.ts | 218 ---------------- server/tests/e2e/admin.e2e.test.ts | 10 +- server/tests/helpers/plugin-host.ts | 4 + .../llm-parse/llm-config.resolver.test.ts | 25 +- .../nest/llm-parse/llm-parse.service.test.ts | 9 +- .../settings.service.test.ts} | 119 ++++++--- server/tests/unit/nest/share.service.test.ts | 3 +- .../plugins/plugin-host-deps.factory.test.ts | 15 +- 23 files changed, 532 insertions(+), 416 deletions(-) delete mode 100644 server/src/services/settingsService.ts rename server/tests/unit/{services/settingsService.test.ts => nest/settings.service.test.ts} (68%) diff --git a/migrate.md b/migrate.md index 6561cf455..9d84fcc59 100644 --- a/migrate.md +++ b/migrate.md @@ -18,7 +18,7 @@ Wave 2 — cross-cutting leverage (parallel to wave 3, high payoff) Wave 3 — domain services with existing modules, low fan-in -7. dayNoteService (done 2026-07), tripInviteService (done 2026-07), assignmentService (done 2026-07), shareService (done 2026-07), settingsService, fileService, collabService, packingService (done 2026-07) — each maps 1:1 to a Nest module that's already a thin delegate; none is imported by more than one +7. dayNoteService (done 2026-07), tripInviteService (done 2026-07), assignmentService (done 2026-07), shareService (done 2026-07), settingsService (done 2026-07), fileService, collabService, packingService (done 2026-07) — each maps 1:1 to a Nest module that's already a thin delegate; none is imported by more than one other legacy service. Wave 4 — the coupled cluster (order matters here) diff --git a/server/src/nest/README.md b/server/src/nest/README.md index 423ce2668..e908898e5 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -27,8 +27,8 @@ mount to Nest and leaves the sibling trip routes (days, places, ...) on Express. categories, tags, notifications, atlas. - **Phase 2 (trip sub-domains):** vacay (addon), packing, todo. - **DI-native services (legacy `src/services/*` deleted):** tags, categories, - todo, packing, day-notes, trip-invite, assignments, share — see the migration - recipe below. + todo, packing, day-notes, trip-invite, assignments, share, settings — see the + migration recipe below. ## Cross-cutting Foundation pieces @@ -114,9 +114,13 @@ stay in `services/queryHelpers.ts`, shared with the unmigrated day/place services); share followed (never imported by the plugin host, and its three MCP tools stay in the legacy trips registrar — their `trips:share` scope gate has no declarative `access: { group, mode }` equivalent — so the port is the SQL -fold plus a 3-export `share.bridge.ts` for `mcp/tools/trips.ts`). Repeat these +fold plus a 3-export `share.bridge.ts` for `mcp/tools/trips.ts`); settings +followed (no MCP surface, no bridge, and the first migration that converted an +in-container plain-function consumer into a provider instead of bridging it: +`llm-parse/llm-config.resolver.ts` became the injectable `LlmConfigResolver`, +injected by `LlmParseService` and `PluginHostDepsFactory`). Repeat these steps per -service (next up: settingsService). This is a **pure relocation** — byte-identical +service (next up: fileService). This is a **pure relocation** — byte-identical SQL, statuses, bodies, and error strings. The plugin RPC host is **no longer a bridge consumer**: since Option A of `src/nest/plugins/DI-MIGRATION.md` it injects domain services via `PluginHostDepsFactory`, so a migrated domain adds diff --git a/server/src/nest/admin/admin.module.ts b/server/src/nest/admin/admin.module.ts index ab881b7ac..d0ad7c2d6 100644 --- a/server/src/nest/admin/admin.module.ts +++ b/server/src/nest/admin/admin.module.ts @@ -2,9 +2,10 @@ import { Module } from '@nestjs/common'; import { AdminController } from './admin.controller'; import { AdminService } from './admin.service'; import { PluginsModule } from '../plugins/plugins.module'; +import { SettingsModule } from '../settings/settings.module'; @Module({ - imports: [PluginsModule], + imports: [PluginsModule, SettingsModule], controllers: [AdminController], providers: [AdminService], }) diff --git a/server/src/nest/admin/admin.service.ts b/server/src/nest/admin/admin.service.ts index 715c28b46..d03518df4 100644 --- a/server/src/nest/admin/admin.service.ts +++ b/server/src/nest/admin/admin.service.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; import * as svc from '../../services/adminService'; -import { getAdminUserDefaults, setAdminUserDefaults } from '../../services/settingsService'; +import { SettingsService } from '../settings/settings.service'; import { invalidateMcpSessions } from '../../mcp'; import { getPreferencesMatrix, setAdminPreferences } from '../../services/notificationPreferencesService'; import { adminResetPasskeys } from '../../services/passkeyService'; @@ -13,6 +13,8 @@ import { adminResetPasskeys } from '../../services/passkeyService'; */ @Injectable() export class AdminService { + constructor(private readonly settings: SettingsService) {} + // Users listUsers() { return svc.listUsers(); } createUser(body: unknown) { return svc.createUser(body as Parameters[0]); } @@ -75,8 +77,8 @@ export class AdminService { invalidateMcpSessions() { invalidateMcpSessions(); } // Settings + notification preference helpers (non-admin-service modules) - getAdminUserDefaults() { return getAdminUserDefaults(); } - setAdminUserDefaults(body: Record) { return setAdminUserDefaults(body); } + getAdminUserDefaults() { return this.settings.getAdminUserDefaults(); } + setAdminUserDefaults(body: Record) { return this.settings.setAdminUserDefaults(body); } getPreferencesMatrix(userId: number, role: string) { return getPreferencesMatrix(userId, role, 'admin'); } setAdminPreferences(userId: number, body: unknown) { return setAdminPreferences(userId, body as Parameters[1]); } } diff --git a/server/src/nest/llm-parse/llm-config.resolver.ts b/server/src/nest/llm-parse/llm-config.resolver.ts index c1454cd70..d69784b3b 100644 --- a/server/src/nest/llm-parse/llm-config.resolver.ts +++ b/server/src/nest/llm-parse/llm-config.resolver.ts @@ -1,55 +1,75 @@ -import { db } from '../../db/database'; import { ADDON_IDS } from '../../addons'; import { isAddonEnabled } from '../../services/adminService'; -import { getUserSettings, getDecryptedUserSetting } from '../../services/settingsService'; import { decryptLlmApiKey, LLM_PROVIDERS, type LlmProvider, type ResolvedLlmConfig } from '../../services/llmConfig'; +import { DatabaseService } from '../database/database.service'; +import { SettingsService } from '../settings/settings.service'; +import { Injectable } from '@nestjs/common'; function asProvider(v: unknown): LlmProvider | null { return typeof v === 'string' && (LLM_PROVIDERS as string[]).includes(v) ? (v as LlmProvider) : null; } -function readInstanceConfig(): ResolvedLlmConfig | null { - const row = db.prepare('SELECT config FROM addons WHERE id = ?').get(ADDON_IDS.LLM_PARSING) as { config?: string } | undefined; - if (!row?.config) return null; - let cfg: Record; - try { - cfg = JSON.parse(row.config || '{}'); - } catch { - return null; - } - const provider = asProvider(cfg.provider); - const model = typeof cfg.model === 'string' ? cfg.model.trim() : ''; - if (!provider || !model) return null; - return { - provider, - model, - baseUrl: typeof cfg.baseUrl === 'string' && cfg.baseUrl.trim() ? cfg.baseUrl.trim() : undefined, - apiKey: decryptLlmApiKey(cfg.apiKey), - multimodal: cfg.multimodal === true, - }; -} - -function readUserConfig(userId: number): ResolvedLlmConfig | null { - const settings = getUserSettings(userId); - const provider = asProvider(settings.llm_provider); - const model = typeof settings.llm_model === 'string' ? settings.llm_model.trim() : ''; - if (!provider || !model) return null; - const apiKey = getDecryptedUserSetting(userId, 'llm_api_key') ?? undefined; - return { - provider, - model, - baseUrl: typeof settings.llm_base_url === 'string' && settings.llm_base_url.trim() ? settings.llm_base_url.trim() : undefined, - apiKey, - multimodal: settings.llm_multimodal === true, - }; -} - /** - * Resolve the effective LLM config for a user, gated by the addon. - * Order: addon disabled → null; admin instance config wins; else per-user config; - * else null. This is the single place the API key is decrypted. + * Resolves the effective LLM config for a user, gated by the addon. Injectable + * (settings come from SettingsService); the addon-row read and the addon gate + * still go through the legacy db/adminService seams until their waves migrate. */ -export function resolveLlmConfig(userId: number): ResolvedLlmConfig | null { - if (!isAddonEnabled(ADDON_IDS.LLM_PARSING)) return null; - return readInstanceConfig() ?? readUserConfig(userId); +@Injectable() +export class LlmConfigResolver { + constructor( + private readonly settings: SettingsService, + private readonly dbService: DatabaseService, + ) {} + + /** + * Resolve the effective LLM config for a user, gated by the addon. + * Order: addon disabled → null; admin instance config wins; else per-user config; + * else null. This is the single place the API key is decrypted. + */ + resolve(userId: number): ResolvedLlmConfig | null { + if (!isAddonEnabled(ADDON_IDS.LLM_PARSING)) return null; + return this.readInstanceConfig() ?? this.readUserConfig(userId); + } + + private readInstanceConfig(): ResolvedLlmConfig | null { + const row = this.dbService.get<{ config?: string } | undefined>( + 'SELECT config FROM addons WHERE id = ?', + ADDON_IDS.LLM_PARSING, + ); + if (!row?.config) return null; + let cfg: Record; + try { + cfg = JSON.parse(row.config || '{}'); + } catch { + return null; + } + const provider = asProvider(cfg.provider); + const model = typeof cfg.model === 'string' ? cfg.model.trim() : ''; + if (!provider || !model) return null; + return { + provider, + model, + baseUrl: typeof cfg.baseUrl === 'string' && cfg.baseUrl.trim() ? cfg.baseUrl.trim() : undefined, + apiKey: decryptLlmApiKey(cfg.apiKey), + multimodal: cfg.multimodal === true, + }; + } + + private readUserConfig(userId: number): ResolvedLlmConfig | null { + const settings = this.settings.getUserSettings(userId); + const provider = asProvider(settings.llm_provider); + const model = typeof settings.llm_model === 'string' ? settings.llm_model.trim() : ''; + if (!provider || !model) return null; + const apiKey = this.settings.getDecryptedUserSetting(userId, 'llm_api_key') ?? undefined; + return { + provider, + model, + baseUrl: + typeof settings.llm_base_url === 'string' && settings.llm_base_url.trim() + ? settings.llm_base_url.trim() + : undefined, + apiKey, + multimodal: settings.llm_multimodal === true, + }; + } } diff --git a/server/src/nest/llm-parse/llm-parse.module.ts b/server/src/nest/llm-parse/llm-parse.module.ts index acbd94a1f..42c82afe2 100644 --- a/server/src/nest/llm-parse/llm-parse.module.ts +++ b/server/src/nest/llm-parse/llm-parse.module.ts @@ -2,11 +2,17 @@ import { Module } from '@nestjs/common'; import { LlmParseService } from './llm-parse.service'; import { LlmLocalService } from './llm-local.service'; import { LlmLocalController } from './llm-local.controller'; +import { LlmConfigResolver } from './llm-config.resolver'; +import { SettingsModule } from '../settings/settings.module'; -/** Provides the LLM booking-import fallback; imported by BookingImportModule. */ +/** + * Provides the LLM booking-import fallback; imported by BookingImportModule. + * Exports LlmConfigResolver for the plugin host wiring (PluginHostDepsFactory). + */ @Module({ + imports: [SettingsModule], controllers: [LlmLocalController], - providers: [LlmParseService, LlmLocalService], - exports: [LlmParseService], + providers: [LlmParseService, LlmLocalService, LlmConfigResolver], + exports: [LlmParseService, LlmConfigResolver], }) export class LlmParseModule {} diff --git a/server/src/nest/llm-parse/llm-parse.service.ts b/server/src/nest/llm-parse/llm-parse.service.ts index e2b7ce6ae..7954da183 100644 --- a/server/src/nest/llm-parse/llm-parse.service.ts +++ b/server/src/nest/llm-parse/llm-parse.service.ts @@ -1,6 +1,6 @@ import type { KiReservation } from '../booking-import/kitinerary.types'; import { createLlmClient } from './llm-client.factory'; -import { resolveLlmConfig } from './llm-config.resolver'; +import { LlmConfigResolver } from './llm-config.resolver'; import { buildSystemPrompt, KI_RESERVATION_JSON_SCHEMA } from './llm-prompt'; import type { LlmExtractionInput } from './llm-provider.interface'; import { isPdf, extractText } from './text-extract'; @@ -26,13 +26,15 @@ export interface LlmParseResult { */ @Injectable() export class LlmParseService { + constructor(private readonly llmConfig: LlmConfigResolver) {} + /** True when the addon is enabled AND a usable config resolves for this user. */ isAvailable(userId: number): boolean { - return resolveLlmConfig(userId) !== null; + return this.llmConfig.resolve(userId) !== null; } async parse(file: { buffer: Buffer; originalName: string }, userId: number): Promise { - const config = resolveLlmConfig(userId); + const config = this.llmConfig.resolve(userId); if (!config) return { kiItems: [], warnings: ['AI parsing is not configured'] }; const warnings: string[] = []; diff --git a/server/src/nest/plugins/DI-MIGRATION.md b/server/src/nest/plugins/DI-MIGRATION.md index d21f5da94..9153a7cd2 100644 --- a/server/src/nest/plugins/DI-MIGRATION.md +++ b/server/src/nest/plugins/DI-MIGRATION.md @@ -76,9 +76,10 @@ microservice abstractions would obscure the boundary for near-zero payoff. previously `new`-ed **per call** inside the `getOAuthToken` closure). `create(id, granted, router)` returns the `PluginRpcHost` with the same ~130-member `HostDeps` object — closures over `this.*` for DI domains, still - plain imports for legacy `services/*`. `DatabaseService` is deliberately NOT - injected: after the singletons went away it had zero remaining uses (inline - SQL uses the raw `db` proxy until those domains migrate). + plain imports for legacy `services/*`. `DatabaseService` is injected too + (since 2026-07): the factory's inline SQL, the `canEditTripAs` gate and the + `HostDeps.db`/audit connection all go through it instead of the raw `db` + proxy (behavior-identical — the injected connection IS the proxy). - `host/plugin-host-state.ts` — the process-wide maps (`dataDbs`, `budgets`) and their accessors, kept module-level on purpose (see §1 table). @@ -102,7 +103,9 @@ monotonically as the recipe in `src/nest/README.md` proceeds (packing and day-notes swapped in 2026-07; trip-invite migrated without touching the factory — it was never imported here; assignments swapped in 2026-07; share migrated 2026-07 without touching the factory — it was never imported here -either; next factory swap: whichever Wave-3 domain the factory imports migrates +either; settings swapped in 2026-07 indirectly — the factory never imported it, +but its `resolveLlmConfig` import became the injected `LlmConfigResolver`; +next factory swap: whichever Wave-3 domain the factory imports migrates next — fileService/collabService are both imported here). ### Test impact (as landed) 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 7223af094..ec2f40a22 100644 --- a/server/src/nest/plugins/host/plugin-host-deps.factory.ts +++ b/server/src/nest/plugins/host/plugin-host-deps.factory.ts @@ -1,6 +1,5 @@ import { Injectable } from '@nestjs/common'; import { readEnv } from '../../../app-config'; -import { db, canAccessTrip } from '../../../db/database'; import { broadcast, broadcastToUser } from '../../../websocket'; import { listBudgetItems } from '../../../services/budgetService'; import { isUpdateConflict } from '../../../services/conflictResult'; @@ -10,7 +9,6 @@ import { createNote as createCollabNoteSvc, createPoll as createCollabPollSvc, v import { getRates as getExchangeRates } from '../../../services/exchangeRateService'; import { joinTripAsMember } from '../../../services/tripMembership'; import { send as sendNotification } from '../../../services/notificationService'; -import { resolveLlmConfig } from '../../llm-parse/llm-config.resolver'; import { createLlmClient } from '../../llm-parse/llm-client.factory'; import { readUserSettingDecrypted } from '../plugins.service'; import { PluginOAuthService } from '../plugin-oauth.service'; @@ -36,25 +34,13 @@ import { TodoService } from '../../todo/todo.service'; import { PackingService } from '../../packing/packing.service'; import { DayNotesService } from '../../days/day-notes.service'; import { AssignmentsService } from '../../assignments/assignments.service'; +import { LlmConfigResolver } from '../../llm-parse/llm-config.resolver'; +import { DatabaseService } from '../../database/database.service'; import { notifyBookingChange } from '../../../services/reservationService'; import { PluginRpcHost, ForbiddenResource, BadParams } from './rpc-host'; import { appendAudit } from './plugin-audit'; import { getPluginDataDb, budgetFor } from './plugin-host-state'; -/** - * The trip-access + role gate used by every planner write, mirroring the app's - * per-domain `canEdit` (canAccessTrip + checkPermission for the entity's *_edit - * action). Returns false — never throws — so the caller maps it to a clean - * RESOURCE_FORBIDDEN. - */ -function canEditTripAs(action: string, tripId: number, userId: number): boolean { - const trip = canAccessTrip(tripId, userId) as { user_id: number } | undefined; - if (!trip) return false; - const u = db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined; - if (!u) return false; - return checkPermission(action, u.role ?? 'user', trip.user_id, userId, trip.user_id !== userId); -} - // The booking notification the REST controller sends after a create/update/delete // is fire-and-forget, so it never blocks the plugin write. function notifyBooking(actingUserId: number, tripId: number, booking: string, type: string): void { @@ -145,15 +131,16 @@ export interface PluginCallRouter { /** * Wires a plugin's capability host to the REAL privileged modules (#plugins, - * M1). This is the ONLY plugin file that imports db/websocket — it runs in the + * M1). This is the ONLY plugin file that imports the websocket — it runs in the * host (parent), never in the child. Broadcasts are force-namespaced to * `plugin:{id}:{event}` so a plugin can't forge a core event. * * DI-native domain services (budget, reservations, tags, categories, todo, - * packing, day-notes, assignments, oauth) are constructor-injected; legacy - * `services/*` domains stay plain function imports until their own migration - * lands (DI-MIGRATION.md), at which point each swaps one import for one - * injected service. + * packing, day-notes, assignments, oauth, the LLM config resolver) and the + * DatabaseService (all inline SQL + the trip-access helper) are + * constructor-injected; legacy `services/*` domains stay plain function + * imports until their own migration lands (DI-MIGRATION.md), at which point + * each swaps one import for one injected service. */ @Injectable() export class PluginHostDepsFactory { @@ -167,8 +154,24 @@ export class PluginHostDepsFactory { private readonly oauth: PluginOAuthService, private readonly dayNotes: DayNotesService, private readonly assignments: AssignmentsService, + private readonly llmConfig: LlmConfigResolver, + private readonly db: DatabaseService, ) {} + /** + * The trip-access + role gate used by every planner write, mirroring the app's + * per-domain `canEdit` (canAccessTrip + checkPermission for the entity's *_edit + * action). Returns false — never throws — so the caller maps it to a clean + * RESOURCE_FORBIDDEN. + */ + private canEditTripAs(action: string, tripId: number, userId: number): boolean { + const trip = this.db.canAccessTrip(tripId, userId) as { user_id: number } | undefined; + if (!trip) return false; + const u = this.db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined; + if (!u) return false; + return checkPermission(action, u.role ?? 'user', trip.user_id, userId, trip.user_id !== userId); + } + create(id: string, granted: ReadonlySet, router: PluginCallRouter): PluginRpcHost { return new PluginRpcHost(id, granted, { // Resolve the data handle lazily on every access rather than capturing it once. @@ -180,14 +183,14 @@ export class PluginHostDepsFactory { // one the moment a stale dispose closes it. Safe because db.* is synchronous, so no // call is ever mid-flight when a dispose from another tick closes the handle. get data() { return getPluginDataDb(id); }, - db, - canAccessTrip: (tripId, userId) => canAccessTrip(tripId, userId), + db: this.db.connection, + canAccessTrip: (tripId, userId) => this.db.canAccessTrip(tripId, userId), // The router binds this host's plugin id as the caller/source. callPlugin: (targetId, fn, args, actingUserId) => router.callPlugin(id, targetId, fn, args, actingUserId), emitPluginEvent: (event, payload) => router.emitPluginEvent(id, event, payload), // Two users "share a trip" when both are owner-or-member of the same trip. canSeeUser: (actingUserId, targetUserId) => - !!db + !!this.db .prepare( `SELECT 1 FROM trips t LEFT JOIN trip_members m1 ON m1.trip_id = t.id AND m1.user_id = ? @@ -199,15 +202,15 @@ export class PluginHostDepsFactory { .get(actingUserId, targetUserId, actingUserId, targetUserId), broadcastToTrip: (tripId, event, payload) => broadcast(tripId, `plugin:${id}:${event}`, payload), broadcastToUser: (userId, payload) => broadcastToUser(userId, { type: `plugin:${id}`, ...payload }), - audit: (entry) => appendAudit(db, entry), + audit: (entry) => appendAudit(this.db.connection, entry), // --- Costs (budget items) --- budgetAddonEnabled: () => isAddonEnabled(ADDON_IDS.BUDGET), // Same gate as a REST/MCP budget mutation: the acting user must have trip // access AND the 'budget_edit' permission for their global role. canEditCosts: (tripId, userId) => { - const trip = canAccessTrip(tripId, userId) as { user_id: number } | undefined; + const trip = this.db.canAccessTrip(tripId, userId) as { user_id: number } | undefined; if (!trip) return false; - const u = db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined; + const u = this.db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined; if (!u) return false; return checkPermission('budget_edit', u.role ?? 'user', trip.user_id, userId, trip.user_id !== userId); }, @@ -234,16 +237,16 @@ export class PluginHostDepsFactory { // --- Files write. Bytes arrive as bounded base64; the extension is validated // against the central blocklist BEFORE anything touches disk, and link targets // must live on the same trip (findForeignLinkTarget). Same events as the app. --- - canUploadFiles: (tripId, userId) => canEditTripAs('file_upload', tripId, userId), - canEditFiles: (tripId, userId) => canEditTripAs('file_edit', tripId, userId), - canDeleteFiles: (tripId, userId) => canEditTripAs('file_delete', tripId, userId), + canUploadFiles: (tripId, userId) => this.canEditTripAs('file_upload', tripId, userId), + canEditFiles: (tripId, userId) => this.canEditTripAs('file_edit', tripId, userId), + canDeleteFiles: (tripId, userId) => this.canEditTripAs('file_delete', tripId, userId), createTripFile: (tripId, input, actingUserId) => { // Mirror the REST upload guard (files.controller): a demo user must not write // bytes to the shared demo instance, even through a plugin's db:write:files. // Only resolve the email when demo mode is actually on — keeps the hot path // (and the schema surface) untouched for self-hosted installs. if (readEnv().demo.enabled) { - const uploader = db.prepare('SELECT email FROM users WHERE id = ?').get(actingUserId) as { email?: string } | undefined; + const uploader = this.db.prepare('SELECT email FROM users WHERE id = ?').get(actingUserId) as { email?: string } | undefined; if (isDemoEmail(uploader?.email)) throw new ForbiddenResource('Uploads are disabled in demo mode.'); } const i = input as { name: string; content_base64: string; mimetype?: string; description?: string; place_id?: number; reservation_id?: number }; @@ -296,7 +299,7 @@ export class PluginHostDepsFactory { listCollabPolls: (tripId) => { requireAddon(ADDON_IDS.COLLAB, 'collab'); return listCollabPollsSvc(tripId) as unknown[]; }, listCollabMessages: (tripId, before) => { requireAddon(ADDON_IDS.COLLAB, 'collab'); return listCollabMessagesSvc(tripId, before) as unknown[]; }, // --- Collab content (collab addon). The services validate + self-report errors. --- - canEditCollab: (tripId, userId) => canEditTripAs('collab_edit', tripId, userId), + canEditCollab: (tripId, userId) => this.canEditTripAs('collab_edit', tripId, userId), createCollabNote: (tripId, input, actingUserId) => { requireAddon(ADDON_IDS.COLLAB, 'collab'); const note = createCollabNoteSvc(String(tripId), actingUserId, input as never); @@ -325,16 +328,16 @@ export class PluginHostDepsFactory { }, // --- Member add (member_manage). Grants trip access — the target must exist; // the acting user is recorded as the inviter. Owner/duplicate adds are no-ops. --- - canManageMembers: (tripId, userId) => canEditTripAs('member_manage', tripId, userId), + canManageMembers: (tripId, userId) => this.canEditTripAs('member_manage', tripId, userId), addTripMember: (tripId, targetUserId, invitedBy) => { - const target = db.prepare('SELECT id FROM users WHERE id = ?').get(targetUserId) as { id: number } | undefined; + const target = this.db.prepare('SELECT id FROM users WHERE id = ?').get(targetUserId) as { id: number } | undefined; if (!target) throw new ForbiddenResource(`no user ${targetUserId}`); return joinTripAsMember(tripId, targetUserId, invitedBy); }, removeTripMember: (tripId, targetUserId) => { // Never remove the OWNER via this path — that would orphan the trip. Ownership // transfer is a separate, deliberate action, not a member-management side effect. - const trip = db.prepare('SELECT user_id FROM trips WHERE id = ?').get(tripId) as { user_id: number } | undefined; + const trip = this.db.prepare('SELECT user_id FROM trips WHERE id = ?').get(tripId) as { user_id: number } | undefined; if (trip && trip.user_id === targetUserId) throw new ForbiddenResource('cannot remove the trip owner'); removeTripMemberSvc(tripId, targetUserId); return { removed: true }; @@ -343,7 +346,7 @@ export class PluginHostDepsFactory { // per-user preferences are all owned by notificationService.send; the plugin // supplies only the target (host-scoped by the router) + plain text. actorId is // null (no user sender), so the message body carries the plugin's content. --- - canAccessTripForNotify: (tripId, userId) => !!canAccessTrip(tripId, userId), + canAccessTripForNotify: (tripId, userId) => !!this.db.canAccessTrip(tripId, userId), sendPluginNotification: async (_pluginId, input) => { if (!budgetFor(id).take('notify', Date.now())) throw new BadParams('daily notification budget exhausted (resets at UTC midnight)'); await sendNotification({ @@ -356,13 +359,13 @@ export class PluginHostDepsFactory { }); return { sent: true }; }, - // --- Host-mediated LLM. The host holds the credential (resolveLlmConfig, encrypted + // --- Host-mediated LLM. The host holds the credential (LlmConfigResolver, encrypted // apiKey) and runs the call under the acting user's provider config; caps + the // provider-availability check live at the router. Reuses the extraction client: // complete() wraps it with a {text} schema; extract() passes the plugin's schema. --- - aiConfigured: (userId) => resolveLlmConfig(userId) !== null, + aiConfigured: (userId) => this.llmConfig.resolve(userId) !== null, aiComplete: async (userId, prompt, system) => { - const config = resolveLlmConfig(userId); + const config = this.llmConfig.resolve(userId); if (!config) throw new BadParams('no AI provider is configured for this user'); if (!budgetFor(id).take('ai', Date.now())) throw new BadParams('daily AI budget exhausted (resets at UTC midnight)'); const results = await createLlmClient(config).extract({ @@ -374,7 +377,7 @@ export class PluginHostDepsFactory { return { text: typeof first?.text === 'string' ? first.text : '' }; }, aiExtract: async (userId, text, jsonSchema, prompt) => { - const config = resolveLlmConfig(userId); + const config = this.llmConfig.resolve(userId); if (!config) throw new BadParams('no AI provider is configured for this user'); if (!budgetFor(id).take('ai', Date.now())) throw new BadParams('daily AI budget exhausted (resets at UTC midnight)'); const results = await createLlmClient(config).extract({ @@ -400,20 +403,20 @@ export class PluginHostDepsFactory { if (everyMs !== undefined && (!Number.isFinite(everyMs) || everyMs < EVERY_MIN)) throw new BadParams(`recurring interval must be >= ${EVERY_MIN} ms`); const json = JSON.stringify(payload ?? null); if (json.length > PAYLOAD_MAX) throw new BadParams(`scheduler payload too large (max ${PAYLOAD_MAX} bytes)`); - const existing = db.prepare('SELECT id FROM plugin_scheduled_tasks WHERE plugin_id = ? AND name = ?').get(id, name) as { id: number } | undefined; + const existing = this.db.prepare('SELECT id FROM plugin_scheduled_tasks WHERE plugin_id = ? AND name = ?').get(id, name) as { id: number } | undefined; if (!existing) { - const n = (db.prepare('SELECT COUNT(*) AS c FROM plugin_scheduled_tasks WHERE plugin_id = ?').get(id) as { c: number }).c; + const n = (this.db.prepare('SELECT COUNT(*) AS c FROM plugin_scheduled_tasks WHERE plugin_id = ?').get(id) as { c: number }).c; if (n >= SCHED_MAX) throw new BadParams(`too many scheduled tasks (max ${SCHED_MAX})`); } // Upsert by (plugin, name): re-scheduling the same name replaces it. - db.prepare( + this.db.prepare( `INSERT INTO plugin_scheduled_tasks (plugin_id, name, due_at, payload, every_ms) VALUES (?, ?, ?, ?, ?) ON CONFLICT (plugin_id, name) DO UPDATE SET due_at = excluded.due_at, payload = excluded.payload, every_ms = excluded.every_ms`, ).run(id, name, Math.max(dueAt, Date.now()), json, everyMs ?? null); return { scheduled: true }; }, schedulerCancel: (name) => { - const r = db.prepare('DELETE FROM plugin_scheduled_tasks WHERE plugin_id = ? AND name = ?').run(id, name); + const r = this.db.prepare('DELETE FROM plugin_scheduled_tasks WHERE plugin_id = ? AND name = ?').run(id, name); return { cancelled: r.changes > 0 }; }, listCostsForTrip: (tripId) => listBudgetItems(tripId), @@ -450,7 +453,7 @@ export class PluginHostDepsFactory { }, // --- Places (place_edit). Delegate to the same placeService the REST/MCP paths // use, then broadcast the same events so open web sessions update live. --- - canEditPlaces: (tripId, userId) => canEditTripAs('place_edit', tripId, userId), + canEditPlaces: (tripId, userId) => this.canEditTripAs('place_edit', tripId, userId), createPlace: (tripId, input) => { const place = createPlace(String(tripId), input as Parameters[1]); broadcast(tripId, 'place:created', { place }); @@ -469,7 +472,7 @@ export class PluginHostDepsFactory { return { deleted: true }; }, // --- Days (day_edit). getDay scopes the row to the trip before any write. --- - canEditDays: (tripId, userId) => canEditTripAs('day_edit', tripId, userId), + canEditDays: (tripId, userId) => this.canEditTripAs('day_edit', tripId, userId), createDay: (tripId, input) => { const i = input as { date?: string; notes?: string }; const day = createDay(tripId, i.date, i.notes); @@ -512,7 +515,7 @@ export class PluginHostDepsFactory { // --- Trip creation (trip_create; owner = acting user). No broadcast — a new trip // is only visible to its owner, who refetches (same as the REST POST). --- canCreateTrip: (userId) => { - const u = db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined; + const u = this.db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined; return checkPermission('trip_create', u?.role ?? 'user', null, userId, false); }, createTripForUser: (userId, input) => { @@ -528,18 +531,18 @@ export class PluginHostDepsFactory { getRates: (base) => getExchangeRates(base), // --- Trip (trip_edit). Only the schema-writable fields reach updateTrip; its // NotFound/Validation errors are mapped to clean RPC codes. --- - canEditTrip: (tripId, userId) => canEditTripAs('trip_edit', tripId, userId), + canEditTrip: (tripId, userId) => this.canEditTripAs('trip_edit', tripId, userId), updateTrip: (tripId, userId, input) => { // The REST controller gates two fields behind their OWN admin-configurable // permissions, separate from trip_edit — reproduce that here so a plugin (or // its member user) can't archive or re-cover a trip it may only edit. - if ('is_archived' in input && !canEditTripAs('trip_archive', tripId, userId)) { + if ('is_archived' in input && !this.canEditTripAs('trip_archive', tripId, userId)) { throw new ForbiddenResource(`no permission to archive trip ${tripId}`); } - if ('cover_image' in input && !canEditTripAs('trip_cover_upload', tripId, userId)) { + if ('cover_image' in input && !this.canEditTripAs('trip_cover_upload', tripId, userId)) { throw new ForbiddenResource(`no permission to change the cover of trip ${tripId}`); } - const u = db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined; + const u = this.db.prepare('SELECT role FROM users WHERE id = ?').get(userId) as { role?: string } | undefined; try { const result = updateTrip(tripId, userId, input as Parameters[2], u?.role ?? 'user'); broadcast(tripId, 'trip:updated', { trip: result.updatedTrip }); @@ -723,7 +726,7 @@ export class PluginHostDepsFactory { // --- Reservations (bookings, reservation_edit). Delegates to ReservationsService // so the accommodation/budget-sync/notification/broadcast side effects match the // web app EXACTLY. socketId is undefined — a plugin has no originating socket. --- - canEditReservations: (tripId, userId) => canEditTripAs('reservation_edit', tripId, userId), + canEditReservations: (tripId, userId) => this.canEditTripAs('reservation_edit', tripId, userId), createReservation: (tripId, input, actingUserId) => { const { reservation, accommodationCreated } = this.reservations.create(String(tripId), input as never); if (accommodationCreated) broadcast(tripId, 'accommodation:created', {}, undefined); @@ -757,7 +760,7 @@ export class PluginHostDepsFactory { // --- Packing (packing_edit). Reuses PackingService + replicates the #858 // privacy-scoped broadcasts (create/delete via emitPackingToViewers, update via // the four-case broadcastPackingUpdate) so a private item never leaks room-wide. --- - canEditPacking: (tripId, userId) => canEditTripAs('packing_edit', tripId, userId), + canEditPacking: (tripId, userId) => this.canEditTripAs('packing_edit', tripId, userId), createPackingItem: (tripId, input, actingUserId) => { const i = input as { name: string; category?: string; checked?: boolean; is_private?: boolean; visibility?: 'common' | 'personal' | 'shared'; recipient_ids?: number[] }; const item = this.packing.createItem(String(tripId), i, actingUserId) as PackingPrivacy; @@ -808,7 +811,7 @@ export class PluginHostDepsFactory { getWeather: (lat, lng, date) => getWeather(String(lat), String(lng), date, 'en'), listCategories: () => this.categories.list() as unknown[], tripMembers: (tripId) => - 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[], + 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), @@ -822,7 +825,7 @@ export class PluginHostDepsFactory { return { deleted: true }; }, // --- Todos (core, trip-scoped; the app's 'packing_edit' permission). --- - canEditTodos: (tripId, userId) => canEditTripAs('packing_edit', tripId, userId), + canEditTodos: (tripId, userId) => this.canEditTripAs('packing_edit', tripId, userId), listTodos: (tripId) => this.todos.listItems(String(tripId)) as unknown[], createTodo: (tripId, input) => { const item = this.todos.createItem(String(tripId), input as never); @@ -844,7 +847,7 @@ export class PluginHostDepsFactory { // to a core entity; the plugin only ever sees rows tagged with its own id. --- metaEntityTrip: (entityType, entityId) => { if (entityType === 'trip') { - return (db.prepare('SELECT id FROM trips WHERE id = ?').get(entityId) as { id: number } | undefined)?.id; + return (this.db.prepare('SELECT id FROM trips WHERE id = ?').get(entityId) as { id: number } | undefined)?.id; } // Each of these tables has a NOT NULL trip_id, so the metadata gate resolves to // the owning trip and reuses the standard canAccessTrip / *_edit checks. @@ -852,10 +855,10 @@ export class PluginHostDepsFactory { : entityType === 'day' ? 'days' : entityType === 'reservation' ? 'reservations' : 'day_accommodations'; // accommodation - return (db.prepare(`SELECT trip_id FROM ${table} WHERE id = ?`).get(entityId) as { trip_id: number } | undefined)?.trip_id; + return (this.db.prepare(`SELECT trip_id FROM ${table} WHERE id = ?`).get(entityId) as { trip_id: number } | undefined)?.trip_id; }, metaGet: (entityType, entityId, key) => { - const row = db.prepare('SELECT value FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=? AND key=?') + const row = this.db.prepare('SELECT value FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=? AND key=?') .get(id, entityType, entityId, key) as { value: string } | undefined; if (!row) return null; try { return JSON.parse(row.value); } catch { return null; } @@ -864,14 +867,14 @@ export class PluginHostDepsFactory { if (key.length > META_KEY_MAX) throw new BadParams(`metadata key too long (>${META_KEY_MAX} chars)`); const json = JSON.stringify(value ?? null); if (json.length > META_VALUE_MAX) throw new BadParams(`metadata value too large (>${META_VALUE_MAX} bytes)`); - const exists = db.prepare('SELECT 1 FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=? AND key=?') + const exists = this.db.prepare('SELECT 1 FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=? AND key=?') .get(id, entityType, entityId, key); if (!exists) { - const { n } = db.prepare('SELECT COUNT(*) AS n FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=?') + const { n } = this.db.prepare('SELECT COUNT(*) AS n FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=?') .get(id, entityType, entityId) as { n: number }; if (n >= META_KEYS_MAX) throw new BadParams(`too many metadata keys on this ${entityType} (max ${META_KEYS_MAX})`); } - db.prepare(`INSERT INTO plugin_entity_metadata (plugin_id, entity_type, entity_id, key, value, updated_at) + this.db.prepare(`INSERT INTO plugin_entity_metadata (plugin_id, entity_type, entity_id, key, value, updated_at) VALUES (?, ?, ?, ?, ?, datetime('now')) ON CONFLICT(plugin_id, entity_type, entity_id, key) DO UPDATE SET value = excluded.value, updated_at = excluded.updated_at`) @@ -879,14 +882,14 @@ export class PluginHostDepsFactory { return { key, value: value ?? null }; }, metaList: (entityType, entityId) => { - const list = db.prepare('SELECT key, value FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=? ORDER BY key') + const list = this.db.prepare('SELECT key, value FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=? ORDER BY key') .all(id, entityType, entityId) as Array<{ key: string; value: string }>; const out: Record = {}; for (const r of list) { try { out[r.key] = JSON.parse(r.value); } catch { out[r.key] = null; } } return out; }, metaDelete: (entityType, entityId, key) => { - const res = db.prepare('DELETE FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=? AND key=?') + const res = this.db.prepare('DELETE FROM plugin_entity_metadata WHERE plugin_id=? AND entity_type=? AND entity_id=? AND key=?') .run(id, entityType, entityId, key); return { deleted: res.changes > 0 }; }, diff --git a/server/src/nest/plugins/plugins.module.ts b/server/src/nest/plugins/plugins.module.ts index 1b5fd9a22..555268749 100644 --- a/server/src/nest/plugins/plugins.module.ts +++ b/server/src/nest/plugins/plugins.module.ts @@ -32,6 +32,7 @@ import { TodoModule } from '../todo/todo.module'; import { PackingModule } from '../packing/packing.module'; import { DaysModule } from '../days/days.module'; import { AssignmentsModule } from '../assignments/assignments.module'; +import { LlmParseModule } from '../llm-parse/llm-parse.module'; /** * Plugin system (#plugins). M0 read side + M2 isolated runtime + M3 frontend: @@ -43,7 +44,7 @@ import { AssignmentsModule } from '../assignments/assignments.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], + imports: [TagsModule, CategoriesModule, BudgetModule, ReservationsModule, TodoModule, PackingModule, DaysModule, AssignmentsModule, LlmParseModule], controllers: [PluginsController, PluginsFeedController, PluginsProxyController, PluginFrameController, PlaceDetailsController, TripWarningsController, ViewContributionsController, TripCardContributionsController, PluginPhotosController, PluginCalendarController, MapMarkersController, MapLayersController, PluginRoutesController, DayScheduleController, PdfSectionsController, AtlasLayersController, JournalEntryRowsController, PluginUserSettingsController, PluginOAuthController, PluginActivityController], providers: [PluginsService, PluginRuntimeService, PluginRegistryService, PluginOAuthService, PluginHostDepsFactory], // Exported so the admin addon-toggle handler can cascade-disable plugins whose diff --git a/server/src/nest/settings/settings.module.ts b/server/src/nest/settings/settings.module.ts index 63e54df50..bb3e0c688 100644 --- a/server/src/nest/settings/settings.module.ts +++ b/server/src/nest/settings/settings.module.ts @@ -2,8 +2,10 @@ import { Module } from '@nestjs/common'; import { SettingsController } from './settings.controller'; import { SettingsService } from './settings.service'; +/** Exports SettingsService for in-container consumers (admin, share, llm-parse). */ @Module({ controllers: [SettingsController], providers: [SettingsService], + exports: [SettingsService], }) export class SettingsModule {} diff --git a/server/src/nest/settings/settings.service.ts b/server/src/nest/settings/settings.service.ts index 150bb66ca..bab2f165e 100644 --- a/server/src/nest/settings/settings.service.ts +++ b/server/src/nest/settings/settings.service.ts @@ -1,13 +1,234 @@ import { Injectable } from '@nestjs/common'; -import * as svc from '../../services/settingsService'; +import { DatabaseService } from '../database/database.service'; +import { decrypt_api_key, maybe_encrypt_api_key } from '../../services/apiKeyCrypto'; +import { MASKED_SETTING_VALUE, normalizeAppearance } from '@trek/shared'; + +const ENCRYPTED_SETTING_KEYS = new Set(['webhook_url', 'ntfy_token', 'mapbox_access_token', 'llm_api_key']); +// Encrypted keys that are masked (••••••••) when returned to the client. +// Keys not in this set but in ENCRYPTED_SETTING_KEYS are decrypted and returned. +const MASKED_SETTING_KEYS = new Set(['webhook_url', 'ntfy_token', 'llm_api_key']); + +export const DEFAULTABLE_USER_SETTING_KEYS = [ + 'temperature_unit', + 'distance_unit', + 'dark_mode', + 'time_format', + // Instance-wide default currency for Costs (new users inherit it until they + // pick their own). Free-form ISO code, validated on the client. + 'default_currency', + 'blur_booking_codes', + 'map_tile_url', + // Instance-wide GL map defaults: admins can set Mapbox token/style or + // tokenless MapLibre/OpenFreeMap style defaults for new users (#920). + 'map_provider', + 'mapbox_access_token', + 'mapbox_style', + 'maplibre_style', + 'mapbox_3d_enabled', + 'mapbox_quality_mode', + // Per-user LLM fallback config for booking import (used when the admin has not + // set instance-wide config on the llm_parsing addon). See llmConfig.ts. + 'llm_provider', + 'llm_model', + 'llm_base_url', + 'llm_multimodal', + 'llm_api_key', +] as const; + +type DefaultableKey = typeof DEFAULTABLE_USER_SETTING_KEYS[number]; + +const DEFAULTABLE_USER_SETTING_KEY_SET = new Set(DEFAULTABLE_USER_SETTING_KEYS); + +const VALID_VALUES: Partial> = { + temperature_unit: ['fahrenheit', 'celsius'], + distance_unit: ['metric', 'imperial'], + time_format: ['12h', '24h'], + dark_mode: [true, false, 'light', 'dark', 'auto'], + map_provider: ['leaflet', 'mapbox-gl', 'maplibre-gl'], + llm_provider: ['local', 'openai', 'anthropic'], +}; + +const BOOLEAN_KEYS = new Set(['blur_booking_codes', 'mapbox_3d_enabled', 'mapbox_quality_mode', 'llm_multimodal']); + +function parseValue(raw: string): unknown { + try { return JSON.parse(raw); } catch { return raw; } +} + +function serializeValue(key: string, value: unknown): string { + // The appearance blob drives the DOM on the client — normalize it on the way + // in so a malformed/partial/future-versioned payload can never be stored (and + // thus never reach the writer). Unknown/invalid fields collapse to the default. + if (key === 'appearance') value = normalizeAppearance(value); + // null and undefined both mean "cleared" and store '' — the legacy code stored + // the string "null" for null, which leaked back out of getDecryptedUserSetting + // as a literal four-character "null" (e.g. as an LLM API key). + const raw = value === null || value === undefined ? '' + : typeof value === 'object' ? JSON.stringify(value) : String(value); + if (ENCRYPTED_SETTING_KEYS.has(key)) return maybe_encrypt_api_key(raw) ?? raw; + return raw; +} /** - * Thin Nest wrapper around the existing settings service. The key/value SQL and - * secret-redaction reuse the legacy code unchanged. + * Per-user key/value preferences and the admin-set instance-wide defaults + * (`default_user_setting_*` rows in app_settings). SQL, masking/merge behavior + * and secret encryption moved from the legacy services/settingsService. + * Post-migration fixes over the legacy code: null serializes as '' like + * undefined (a stored "null" leaked out of getDecryptedUserSetting as the + * literal string), and bulk upserts skip masked-sentinel values (mirroring the + * single-upsert no-op, so a bulk save echoing a masked secret can never + * destroy the stored secret) and return the count actually written. */ @Injectable() export class SettingsService { - getUserSettings(userId: number) { return svc.getUserSettings(userId); } - upsertSetting(userId: number, key: string, value: unknown) { return svc.upsertSetting(userId, key, value); } - bulkUpsertSettings(userId: number, settings: Record) { return svc.bulkUpsertSettings(userId, settings); } + constructor(private readonly db: DatabaseService) {} + + getAdminUserDefaults(): Record { + const rows = this.db.all<{ key: string; value: string }>( + "SELECT key, value FROM app_settings WHERE key LIKE 'default_user_setting_%'" + ); + const defaults: Record = {}; + for (const row of rows) { + const settingKey = row.key.slice('default_user_setting_'.length); + if (ENCRYPTED_SETTING_KEYS.has(settingKey)) { + defaults[settingKey] = row.value ? (decrypt_api_key(row.value) ?? '') : ''; + } else { + defaults[settingKey] = parseValue(row.value); + } + } + return defaults; + } + + setAdminUserDefaults(partial: Record): void { + const upsert = this.db.prepare( + `INSERT INTO app_settings (key, value) VALUES (?, ?) + ON CONFLICT(key) DO UPDATE SET value = excluded.value` + ); + const del = this.db.prepare("DELETE FROM app_settings WHERE key = ?"); + + this.db.transaction(() => { + for (const [key, value] of Object.entries(partial)) { + if (!(DEFAULTABLE_USER_SETTING_KEYS as readonly string[]).includes(key)) { + throw new Error(`Invalid setting key: ${key}`); + } + const typedKey = key as DefaultableKey; + const appKey = `default_user_setting_${key}`; + + // null/undefined means "reset to built-in default" — delete the row + if (value === null || value === undefined) { + del.run(appKey); + continue; + } + + if (BOOLEAN_KEYS.has(typedKey) && typeof value !== 'boolean') { + throw new Error(`Setting ${key} must be a boolean`); + } + const allowed = VALID_VALUES[typedKey]; + if (allowed && !allowed.includes(value)) { + throw new Error(`Invalid value for ${key}: ${value}`); + } + + // Encrypt sensitive defaults (the shared Mapbox token) at rest, like the + // per-user equivalents; everything else is stored as plain JSON. + const stored = ENCRYPTED_SETTING_KEYS.has(key) + ? (maybe_encrypt_api_key(String(value)) ?? String(value)) + : JSON.stringify(value); + upsert.run(appKey, stored); + } + }); + } + + getUserSettings(userId: number): Record { + const adminDefaults = this.getAdminUserDefaults(); + + const rows = this.db.all<{ key: string; value: string }>('SELECT key, value FROM settings WHERE user_id = ?', userId); + const userSettings: Record = {}; + for (const row of rows) { + if (MASKED_SETTING_KEYS.has(row.key)) { + userSettings[row.key] = row.value ? MASKED_SETTING_VALUE : ''; + continue; + } + if (ENCRYPTED_SETTING_KEYS.has(row.key)) { + userSettings[row.key] = row.value ? (decrypt_api_key(row.value) ?? '') : ''; + continue; + } + try { + userSettings[row.key] = JSON.parse(row.value); + } catch { + userSettings[row.key] = row.value; + } + } + + // Admin defaults fill in for keys the user hasn't explicitly set. For a + // defaultable key an *empty* user value counts as "not set": the client's + // Settings save always writes every field (a blank Mapbox token included), so + // an empty string would otherwise shadow the admin/system-wide default and the + // user could never fall back to it — even after clearing their own value + // (#1634). Non-defaultable keys keep their exact stored value. + // + // Seed from the admin defaults, but mask any encrypted secret (e.g. a shared + // llm_api_key) exactly like the per-user path above: getUserSettings is the + // client-facing accessor and must never hand back a secret in cleartext — not + // even one inherited from an admin default (the fall-through above widened the + // set of users who inherit it). The server reads the real value via + // getAdminUserDefaults / getDecryptedUserSetting, which don't mask. + const merged: Record = {}; + for (const [key, value] of Object.entries(adminDefaults)) { + merged[key] = MASKED_SETTING_KEYS.has(key) ? (value ? MASKED_SETTING_VALUE : '') : value; + } + for (const [key, value] of Object.entries(userSettings)) { + if ( + DEFAULTABLE_USER_SETTING_KEY_SET.has(key) && + (value === '' || value === null || value === undefined) && + key in adminDefaults + ) { + continue; // empty user value → keep the admin default + } + merged[key] = value; + } + return merged; + } + + upsertSetting(userId: number, key: string, value: unknown) { + this.db.run(` + INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?) + ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value + `, userId, key, serializeValue(key, value)); + } + + bulkUpsertSettings(userId: number, settings: Record) { + const upsert = this.db.prepare(` + INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?) + ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value + `); + let written = 0; + this.db.transaction(() => { + for (const [key, value] of Object.entries(settings)) { + // The client echoes redacted secrets back unchanged — skip them so a + // bulk save can never overwrite a stored secret with the mask (the + // single-upsert route has the same no-op in the controller). + if (value === MASKED_SETTING_VALUE) continue; + upsert.run(userId, key, serializeValue(key, value)); + written++; + } + }); + return written; + } + + /** + * Read a single per-user setting, decrypting it if it's an encrypted key. + * Unlike getUserSettings (which MASKS encrypted keys for the client), this + * returns the plaintext — for server-side use only (e.g. the LLM config + * resolver needs the real API key). Returns null when unset. + */ + getDecryptedUserSetting(userId: number, key: string): string | null { + const row = this.db.get<{ value: string }>('SELECT value FROM settings WHERE user_id = ? AND key = ?', userId, key); + if (!row || row.value === '' || row.value == null) return null; + if (ENCRYPTED_SETTING_KEYS.has(key)) return decrypt_api_key(row.value); + try { + const parsed = JSON.parse(row.value); + return typeof parsed === 'string' ? parsed : row.value; + } catch { + return row.value; + } + } } diff --git a/server/src/nest/share/share.bridge.ts b/server/src/nest/share/share.bridge.ts index ba6f6c524..0c4b2e4f1 100644 --- a/server/src/nest/share/share.bridge.ts +++ b/server/src/nest/share/share.bridge.ts @@ -2,6 +2,7 @@ import { db } from '../../db/database'; import { DatabaseService } from '../database/database.service'; import { ShareService } from './share.service'; import type { SharePermissions } from './share.service'; +import { SettingsService } from '../settings/settings.service'; /** * Non-Nest entry point for the share domain — for code running OUTSIDE the @@ -16,7 +17,7 @@ import type { SharePermissions } from './share.service'; * Module-level construction is safe: `db` is the reinitialize-proof Proxy onto * the shared better-sqlite3 singleton. */ -const share = new ShareService(new DatabaseService(db)); +const share = new ShareService(new DatabaseService(db), new SettingsService(new DatabaseService(db))); export function createOrUpdateShareLink(tripId: string, createdBy: number, permissions: SharePermissions) { return share.createOrUpdate(tripId, createdBy, permissions); diff --git a/server/src/nest/share/share.module.ts b/server/src/nest/share/share.module.ts index 4e147f48c..f1a47dc64 100644 --- a/server/src/nest/share/share.module.ts +++ b/server/src/nest/share/share.module.ts @@ -1,8 +1,10 @@ import { Module } from '@nestjs/common'; import { TripShareController, SharedController } from './share.controller'; import { ShareService } from './share.service'; +import { SettingsModule } from '../settings/settings.module'; @Module({ + imports: [SettingsModule], controllers: [TripShareController, SharedController], providers: [ShareService], }) diff --git a/server/src/nest/share/share.service.ts b/server/src/nest/share/share.service.ts index 3940e5bcd..126810136 100644 --- a/server/src/nest/share/share.service.ts +++ b/server/src/nest/share/share.service.ts @@ -5,7 +5,7 @@ import type { TripAccess } from '../database/database.service'; import { checkPermission } from '../../services/permissions'; import { loadTagsByPlaceIds } from '../../services/queryHelpers'; import { serveFilePath } from '../../services/placePhotoCache'; -import { getUserSettings } from '../../services/settingsService'; +import { SettingsService } from '../settings/settings.service'; import type { User } from '../../types'; type Trip = TripAccess; @@ -52,7 +52,10 @@ export interface ShareTokenInfo { */ @Injectable() export class ShareService { - constructor(private readonly dbs: DatabaseService) {} + constructor( + private readonly dbs: DatabaseService, + private readonly settings: SettingsService, + ) {} verifyTripAccess(tripId: string, userId: number) { return this.dbs.canAccessTrip(tripId, userId); @@ -275,7 +278,7 @@ export class ShareService { // (`||` on purpose: an empty-string trip currency also falls back). let baseCurrency = (trip as { currency?: string }).currency || 'EUR'; if (shareRow.created_by != null) { - const ownerDefault = getUserSettings(shareRow.created_by)['default_currency']; + const ownerDefault = this.settings.getUserSettings(shareRow.created_by)['default_currency']; if (typeof ownerDefault === 'string' && ownerDefault.trim()) { baseCurrency = ownerDefault.trim(); } diff --git a/server/src/services/settingsService.ts b/server/src/services/settingsService.ts deleted file mode 100644 index 52a47c251..000000000 --- a/server/src/services/settingsService.ts +++ /dev/null @@ -1,218 +0,0 @@ -import { db } from '../db/database'; -import { decrypt_api_key, maybe_encrypt_api_key } from './apiKeyCrypto'; -import { normalizeAppearance } from '@trek/shared'; - -const ENCRYPTED_SETTING_KEYS = new Set(['webhook_url', 'ntfy_token', 'mapbox_access_token', 'llm_api_key']); -// Encrypted keys that are masked (••••••••) when returned to the client. -// Keys not in this set but in ENCRYPTED_SETTING_KEYS are decrypted and returned. -const MASKED_SETTING_KEYS = new Set(['webhook_url', 'ntfy_token', 'llm_api_key']); - -export const DEFAULTABLE_USER_SETTING_KEYS = [ - 'temperature_unit', - 'distance_unit', - 'dark_mode', - 'time_format', - // Instance-wide default currency for Costs (new users inherit it until they - // pick their own). Free-form ISO code, validated on the client. - 'default_currency', - 'blur_booking_codes', - 'map_tile_url', - // Instance-wide GL map defaults: admins can set Mapbox token/style or - // tokenless MapLibre/OpenFreeMap style defaults for new users (#920). - 'map_provider', - 'mapbox_access_token', - 'mapbox_style', - 'maplibre_style', - 'mapbox_3d_enabled', - 'mapbox_quality_mode', - // Per-user LLM fallback config for booking import (used when the admin has not - // set instance-wide config on the llm_parsing addon). See llmConfig.ts. - 'llm_provider', - 'llm_model', - 'llm_base_url', - 'llm_multimodal', - 'llm_api_key', -] as const; - -type DefaultableKey = typeof DEFAULTABLE_USER_SETTING_KEYS[number]; - -const DEFAULTABLE_USER_SETTING_KEY_SET = new Set(DEFAULTABLE_USER_SETTING_KEYS); - -const VALID_VALUES: Partial> = { - temperature_unit: ['fahrenheit', 'celsius'], - distance_unit: ['metric', 'imperial'], - time_format: ['12h', '24h'], - dark_mode: [true, false, 'light', 'dark', 'auto'], - map_provider: ['leaflet', 'mapbox-gl', 'maplibre-gl'], - llm_provider: ['local', 'openai', 'anthropic'], -}; - -const BOOLEAN_KEYS = new Set(['blur_booking_codes', 'mapbox_3d_enabled', 'mapbox_quality_mode', 'llm_multimodal']); - -function parseValue(raw: string): unknown { - try { return JSON.parse(raw); } catch { return raw; } -} - -export function getAdminUserDefaults(): Record { - const rows = db.prepare( - "SELECT key, value FROM app_settings WHERE key LIKE 'default_user_setting_%'" - ).all() as { key: string; value: string }[]; - const defaults: Record = {}; - for (const row of rows) { - const settingKey = row.key.slice('default_user_setting_'.length); - if (ENCRYPTED_SETTING_KEYS.has(settingKey)) { - defaults[settingKey] = row.value ? (decrypt_api_key(row.value) ?? '') : ''; - } else { - defaults[settingKey] = parseValue(row.value); - } - } - return defaults; -} - -export function setAdminUserDefaults(partial: Record): void { - const upsert = db.prepare( - `INSERT INTO app_settings (key, value) VALUES (?, ?) - ON CONFLICT(key) DO UPDATE SET value = excluded.value` - ); - const del = db.prepare("DELETE FROM app_settings WHERE key = ?"); - - db.exec('BEGIN'); - try { - for (const [key, value] of Object.entries(partial)) { - if (!(DEFAULTABLE_USER_SETTING_KEYS as readonly string[]).includes(key)) { - throw new Error(`Invalid setting key: ${key}`); - } - const typedKey = key as DefaultableKey; - const appKey = `default_user_setting_${key}`; - - // null/undefined means "reset to built-in default" — delete the row - if (value === null || value === undefined) { - del.run(appKey); - continue; - } - - if (BOOLEAN_KEYS.has(typedKey) && typeof value !== 'boolean') { - throw new Error(`Setting ${key} must be a boolean`); - } - const allowed = VALID_VALUES[typedKey]; - if (allowed && !allowed.includes(value)) { - throw new Error(`Invalid value for ${key}: ${value}`); - } - - // Encrypt sensitive defaults (the shared Mapbox token) at rest, like the - // per-user equivalents; everything else is stored as plain JSON. - const stored = ENCRYPTED_SETTING_KEYS.has(key) - ? (maybe_encrypt_api_key(String(value)) ?? String(value)) - : JSON.stringify(value); - upsert.run(appKey, stored); - } - db.exec('COMMIT'); - } catch (err) { - db.exec('ROLLBACK'); - throw err; - } -} - -export function getUserSettings(userId: number): Record { - const adminDefaults = getAdminUserDefaults(); - - const rows = db.prepare('SELECT key, value FROM settings WHERE user_id = ?').all(userId) as { key: string; value: string }[]; - const userSettings: Record = {}; - for (const row of rows) { - if (MASKED_SETTING_KEYS.has(row.key)) { - userSettings[row.key] = row.value ? '••••••••' : ''; - continue; - } - if (ENCRYPTED_SETTING_KEYS.has(row.key)) { - userSettings[row.key] = row.value ? (decrypt_api_key(row.value) ?? '') : ''; - continue; - } - try { - userSettings[row.key] = JSON.parse(row.value); - } catch { - userSettings[row.key] = row.value; - } - } - - // Admin defaults fill in for keys the user hasn't explicitly set. For a - // defaultable key an *empty* user value counts as "not set": the client's - // Settings save always writes every field (a blank Mapbox token included), so - // an empty string would otherwise shadow the admin/system-wide default and the - // user could never fall back to it — even after clearing their own value - // (#1634). Non-defaultable keys keep their exact stored value. - // - // Seed from the admin defaults, but mask any encrypted secret (e.g. a shared - // llm_api_key) exactly like the per-user path above: getUserSettings is the - // client-facing accessor and must never hand back a secret in cleartext — not - // even one inherited from an admin default (the fall-through above widened the - // set of users who inherit it). The server reads the real value via - // getAdminUserDefaults / getDecryptedUserSetting, which don't mask. - const merged: Record = {}; - for (const [key, value] of Object.entries(adminDefaults)) { - merged[key] = MASKED_SETTING_KEYS.has(key) ? (value ? '••••••••' : '') : value; - } - for (const [key, value] of Object.entries(userSettings)) { - if ( - DEFAULTABLE_USER_SETTING_KEY_SET.has(key) && - (value === '' || value === null || value === undefined) && - key in adminDefaults - ) { - continue; // empty user value → keep the admin default - } - merged[key] = value; - } - return merged; -} - -function serializeValue(key: string, value: unknown): string { - // The appearance blob drives the DOM on the client — normalize it on the way - // in so a malformed/partial/future-versioned payload can never be stored (and - // thus never reach the writer). Unknown/invalid fields collapse to the default. - if (key === 'appearance') value = normalizeAppearance(value); - const raw = typeof value === 'object' ? JSON.stringify(value) : String(value !== undefined ? value : ''); - if (ENCRYPTED_SETTING_KEYS.has(key)) return maybe_encrypt_api_key(raw) ?? raw; - return raw; -} - -export function upsertSetting(userId: number, key: string, value: unknown) { - db.prepare(` - INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?) - ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value - `).run(userId, key, serializeValue(key, value)); -} - -export function bulkUpsertSettings(userId: number, settings: Record) { - const upsert = db.prepare(` - INSERT INTO settings (user_id, key, value) VALUES (?, ?, ?) - ON CONFLICT(user_id, key) DO UPDATE SET value = excluded.value - `); - db.exec('BEGIN'); - try { - for (const [key, value] of Object.entries(settings)) { - upsert.run(userId, key, serializeValue(key, value)); - } - db.exec('COMMIT'); - } catch (err) { - db.exec('ROLLBACK'); - throw err; - } - return Object.keys(settings).length; -} - -/** - * Read a single per-user setting, decrypting it if it's an encrypted key. - * Unlike getUserSettings (which MASKS encrypted keys for the client), this - * returns the plaintext — for server-side use only (e.g. the LLM config - * resolver needs the real API key). Returns null when unset. - */ -export function getDecryptedUserSetting(userId: number, key: string): string | null { - const row = db.prepare('SELECT value FROM settings WHERE user_id = ? AND key = ?').get(userId, key) as { value: string } | undefined; - if (!row || row.value === '' || row.value == null) return null; - if (ENCRYPTED_SETTING_KEYS.has(key)) return decrypt_api_key(row.value); - try { - const parsed = JSON.parse(row.value); - return typeof parsed === 'string' ? parsed : row.value; - } catch { - return row.value; - } -} diff --git a/server/tests/e2e/admin.e2e.test.ts b/server/tests/e2e/admin.e2e.test.ts index 8511f2c12..d63c600f6 100644 --- a/server/tests/e2e/admin.e2e.test.ts +++ b/server/tests/e2e/admin.e2e.test.ts @@ -1,8 +1,9 @@ /** * Admin e2e — exercises the migrated /api/admin endpoints through the real * JwtAuthGuard + AdminGuard against a temp SQLite db. The admin service + - * helpers are mocked; this focuses on auth (401), the admin gate (403 for a - * non-admin), create-201, validation 400 and the dev-only 404. + * helpers are mocked (SettingsService is DI-native and runs real SQL); this + * focuses on auth (401), the admin gate (403 for a non-admin), create-201, + * validation 400 and the dev-only 404. */ import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest'; import request from 'supertest'; @@ -19,6 +20,10 @@ const { db } = vi.hoisted(() => { tmp.exec('PRAGMA journal_mode = WAL'); tmp.exec(`CREATE TABLE users (id INTEGER PRIMARY KEY AUTOINCREMENT, username TEXT NOT NULL, email TEXT NOT NULL UNIQUE, role TEXT NOT NULL DEFAULT 'user', password_version INTEGER NOT NULL DEFAULT 0);`); + // The real SettingsService (DI-injected into AdminService, no mock) reads these. + tmp.exec(`CREATE TABLE settings (id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL, + key TEXT NOT NULL, value TEXT, UNIQUE(user_id, key));`); + tmp.exec('CREATE TABLE app_settings (key TEXT PRIMARY KEY, value TEXT);'); return { db: tmp }; }); @@ -26,7 +31,6 @@ vi.mock('../../src/db/database', () => ({ db, closeDb: () => {}, reinitialize: ( vi.mock('../../src/services/auditLog', () => ({ writeAudit: vi.fn(), getClientIp: () => '1.2.3.4', logInfo: vi.fn() })); vi.mock('../../src/mcp', () => ({ invalidateMcpSessions: vi.fn() })); vi.mock('../../src/services/notificationPreferencesService', () => ({ getPreferencesMatrix: vi.fn(() => ({})), setAdminPreferences: vi.fn() })); -vi.mock('../../src/services/settingsService', () => ({ getAdminUserDefaults: vi.fn(() => ({})), setAdminUserDefaults: vi.fn() })); vi.mock('../../src/services/notificationService', () => ({ send: vi.fn().mockResolvedValue(undefined) })); const { adminSvc } = vi.hoisted(() => ({ diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 2df1c21d3..c4e657a4b 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -11,6 +11,8 @@ import { TodoService } from '../../src/nest/todo/todo.service'; import { PackingService } from '../../src/nest/packing/packing.service'; import { DayNotesService } from '../../src/nest/days/day-notes.service'; import { AssignmentsService } from '../../src/nest/assignments/assignments.service'; +import { LlmConfigResolver } from '../../src/nest/llm-parse/llm-config.resolver'; +import { SettingsService } from '../../src/nest/settings/settings.service'; /** * Hand-wired counterpart of the PluginsModule DI graph for no-Nest tests @@ -29,6 +31,8 @@ export function createHostDepsFactory(dbs: DatabaseService): PluginHostDepsFacto new PluginOAuthService(dbs), new DayNotesService(dbs), new AssignmentsService(dbs), + new LlmConfigResolver(new SettingsService(dbs), dbs), + dbs, ); } diff --git a/server/tests/unit/nest/llm-parse/llm-config.resolver.test.ts b/server/tests/unit/nest/llm-parse/llm-config.resolver.test.ts index acb99b58c..8465fd8c9 100644 --- a/server/tests/unit/nest/llm-parse/llm-config.resolver.test.ts +++ b/server/tests/unit/nest/llm-parse/llm-config.resolver.test.ts @@ -9,13 +9,18 @@ vi.mock('../../../../src/db/database', () => ({ db: dbMock, closeDb: () => {}, r const { isAddonEnabled } = vi.hoisted(() => ({ isAddonEnabled: vi.fn() })); vi.mock('../../../../src/services/adminService', () => ({ isAddonEnabled })); -const { getUserSettings, getDecryptedUserSetting } = vi.hoisted(() => ({ - getUserSettings: vi.fn(() => ({}) as Record), - getDecryptedUserSetting: vi.fn(() => null as string | null), -})); -vi.mock('../../../../src/services/settingsService', () => ({ getUserSettings, getDecryptedUserSetting })); +import { LlmConfigResolver } from '../../../../src/nest/llm-parse/llm-config.resolver'; +import { DatabaseService } from '../../../../src/nest/database/database.service'; +import type { SettingsService } from '../../../../src/nest/settings/settings.service'; -import { resolveLlmConfig } from '../../../../src/nest/llm-parse/llm-config.resolver'; +// The resolver injects SettingsService — a stub instance instead of the old +// legacy-module path mock (same behaviors as before the DI move). The +// DatabaseService rides the same prepare/get mock the module-level db used. +const getUserSettings = vi.fn(() => ({}) as Record); +const getDecryptedUserSetting = vi.fn(() => null as string | null); +const settingsStub = { getUserSettings, getDecryptedUserSetting } as unknown as SettingsService; + +const resolver = new LlmConfigResolver(settingsStub, new DatabaseService(dbMock as never)); function setInstanceConfig(config: unknown) { dbMock._stmt.get.mockReturnValue(config === undefined ? undefined : { config: JSON.stringify(config) }); @@ -32,12 +37,12 @@ beforeEach(() => { describe('resolveLlmConfig', () => { it('returns null when the addon is disabled', () => { isAddonEnabled.mockReturnValue(false); - expect(resolveLlmConfig(1)).toBeNull(); + expect(resolver.resolve(1)).toBeNull(); }); it('uses instance config when present (and decrypts the key)', () => { setInstanceConfig({ provider: 'anthropic', model: 'claude-opus-4-8', apiKey: 'sk-plain', multimodal: true }); - expect(resolveLlmConfig(1)).toEqual({ + expect(resolver.resolve(1)).toEqual({ provider: 'anthropic', model: 'claude-opus-4-8', baseUrl: undefined, @@ -50,7 +55,7 @@ describe('resolveLlmConfig', () => { setInstanceConfig({ provider: 'anthropic' }); // no model → not usable getUserSettings.mockReturnValue({ llm_provider: 'local', llm_model: 'nuextract', llm_base_url: 'http://x/v1', llm_multimodal: true }); getDecryptedUserSetting.mockReturnValue('user-key'); - expect(resolveLlmConfig(7)).toEqual({ + expect(resolver.resolve(7)).toEqual({ provider: 'local', model: 'nuextract', baseUrl: 'http://x/v1', @@ -62,6 +67,6 @@ describe('resolveLlmConfig', () => { it('returns null when neither instance nor user config is usable', () => { getUserSettings.mockReturnValue({ llm_provider: 'openai' }); // no model - expect(resolveLlmConfig(1)).toBeNull(); + expect(resolver.resolve(1)).toBeNull(); }); }); diff --git a/server/tests/unit/nest/llm-parse/llm-parse.service.test.ts b/server/tests/unit/nest/llm-parse/llm-parse.service.test.ts index 56045320f..a2339b59d 100644 --- a/server/tests/unit/nest/llm-parse/llm-parse.service.test.ts +++ b/server/tests/unit/nest/llm-parse/llm-parse.service.test.ts @@ -1,7 +1,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { resolveLlmConfig } = vi.hoisted(() => ({ resolveLlmConfig: vi.fn() })); -vi.mock('../../../../src/nest/llm-parse/llm-config.resolver', () => ({ resolveLlmConfig })); +// LlmConfigResolver is constructor-injected — a stub instance instead of the +// old path mock (same behaviors as before the DI move). +const resolveLlmConfig = vi.fn(); const { createLlmClient, extract } = vi.hoisted(() => { const extract = vi.fn(); @@ -22,9 +23,11 @@ const { routeExtraction, detectFlightNumbers } = vi.hoisted(() => ({ vi.mock('../../../../src/nest/llm-parse/router/extraction-router', () => ({ routeExtraction, detectFlightNumbers })); import { LlmParseService } from '../../../../src/nest/llm-parse/llm-parse.service'; +import type { LlmConfigResolver } from '../../../../src/nest/llm-parse/llm-config.resolver'; const cfg = (over: Record = {}) => ({ provider: 'openai', model: 'm', multimodal: false, ...over }); -const svc = () => new LlmParseService(); +const llmConfigStub = { resolve: resolveLlmConfig } as unknown as LlmConfigResolver; +const svc = () => new LlmParseService(llmConfigStub); const file = (name: string, body = 'Flight AB123') => ({ buffer: Buffer.from(body), originalName: name }); beforeEach(() => { diff --git a/server/tests/unit/services/settingsService.test.ts b/server/tests/unit/nest/settings.service.test.ts similarity index 68% rename from server/tests/unit/services/settingsService.test.ts rename to server/tests/unit/nest/settings.service.test.ts index 6395fec31..e01280c52 100644 --- a/server/tests/unit/services/settingsService.test.ts +++ b/server/tests/unit/nest/settings.service.test.ts @@ -1,5 +1,8 @@ /** - * Unit tests for settingsService — SET-SVC-001 through SET-SVC-020. + * Unit tests for the DI-native SettingsService — SET-SVC-001 through + * SET-SVC-030 (001–026 moved 1:1 from the legacy + * tests/unit/services/settingsService.test.ts; 027–030 pin the post-migration + * quirk fixes: null-serializes-as-'' and the bulk masked-sentinel skip). * Uses a real in-memory SQLite DB; apiKeyCrypto is mocked to a passthrough * so we don't need real encryption for most tests. */ @@ -41,7 +44,10 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser } from '../../helpers/factories'; -import { getUserSettings, upsertSetting, bulkUpsertSettings } from '../../../src/services/settingsService'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { SettingsService } from '../../../src/nest/settings/settings.service'; + +const svc = new SettingsService(new DatabaseService(testDb)); beforeAll(() => { createTables(testDb); @@ -61,13 +67,13 @@ afterAll(() => { describe('getUserSettings', () => { it('SET-SVC-001 — returns empty object when user has no settings', () => { const { user } = createUser(testDb); - expect(getUserSettings(user.id)).toEqual({}); + expect(svc.getUserSettings(user.id)).toEqual({}); }); it('SET-SVC-002 — returns stored plain string values', () => { const { user } = createUser(testDb); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'theme', 'dark')").run(user.id); - const s = getUserSettings(user.id); + const s = svc.getUserSettings(user.id); expect(s.theme).toBe('dark'); }); @@ -76,7 +82,7 @@ describe('getUserSettings', () => { testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'count', '42')").run(user.id); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'flag', 'true')").run(user.id); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'obj', '{\"x\":1}')").run(user.id); - const s = getUserSettings(user.id); + const s = svc.getUserSettings(user.id); expect(s.count).toBe(42); expect(s.flag).toBe(true); expect(s.obj).toEqual({ x: 1 }); @@ -85,21 +91,21 @@ describe('getUserSettings', () => { it('SET-SVC-004 — falls back to raw string when value is not valid JSON', () => { const { user } = createUser(testDb); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'raw', 'not-json')").run(user.id); - const s = getUserSettings(user.id); + const s = svc.getUserSettings(user.id); expect(s.raw).toBe('not-json'); }); it('SET-SVC-005 — webhook_url with a value is masked as ••••••••', () => { const { user } = createUser(testDb); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'webhook_url', 'https://secret.example.com')").run(user.id); - const s = getUserSettings(user.id); + const s = svc.getUserSettings(user.id); expect(s.webhook_url).toBe('••••••••'); }); it('SET-SVC-006 — webhook_url with empty value returns empty string', () => { const { user } = createUser(testDb); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'webhook_url', '')").run(user.id); - const s = getUserSettings(user.id); + const s = svc.getUserSettings(user.id); expect(s.webhook_url).toBe(''); }); @@ -108,7 +114,7 @@ describe('getUserSettings', () => { const { user: b } = createUser(testDb); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'key_a', '\"a\"')").run(a.id); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'key_b', '\"b\"')").run(b.id); - const s = getUserSettings(a.id); + const s = svc.getUserSettings(a.id); expect(s).toHaveProperty('key_a'); expect(s).not.toHaveProperty('key_b'); }); @@ -123,34 +129,34 @@ describe('getUserSettings', () => { it('SET-SVC-020 — new user with no rows inherits the admin default token', () => { const { user } = createUser(testDb); setAdminDefault('mapbox_access_token', 'pk.admin'); - expect(getUserSettings(user.id).mapbox_access_token).toBe('pk.admin'); + expect(svc.getUserSettings(user.id).mapbox_access_token).toBe('pk.admin'); }); it('SET-SVC-021 — an empty user token falls through to the admin default', () => { const { user } = createUser(testDb); setAdminDefault('mapbox_access_token', 'pk.admin'); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'mapbox_access_token', '')").run(user.id); - expect(getUserSettings(user.id).mapbox_access_token).toBe('pk.admin'); + expect(svc.getUserSettings(user.id).mapbox_access_token).toBe('pk.admin'); }); it('SET-SVC-022 — a non-empty user token overrides the admin default', () => { const { user } = createUser(testDb); setAdminDefault('mapbox_access_token', 'pk.admin'); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'mapbox_access_token', 'pk.user')").run(user.id); - expect(getUserSettings(user.id).mapbox_access_token).toBe('pk.user'); + expect(svc.getUserSettings(user.id).mapbox_access_token).toBe('pk.user'); }); it('SET-SVC-023 — an empty value with no admin default stays empty (no regression)', () => { const { user } = createUser(testDb); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'mapbox_style', '')").run(user.id); - expect(getUserSettings(user.id).mapbox_style).toBe(''); + expect(svc.getUserSettings(user.id).mapbox_style).toBe(''); }); it('SET-SVC-024 — a non-defaultable empty value is preserved even against a same-named default', () => { const { user } = createUser(testDb); // 'theme' is not a defaultable key: an empty stored value must be returned as-is. testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'theme', '')").run(user.id); - expect(getUserSettings(user.id).theme).toBe(''); + expect(svc.getUserSettings(user.id).theme).toBe(''); }); it('SET-SVC-025 — an inherited admin llm_api_key is masked, never returned in cleartext', () => { @@ -158,14 +164,14 @@ describe('getUserSettings', () => { setAdminDefault('llm_api_key', 'sk-admin-secret'); // No user row → the value is inherited from the admin default; it is a secret // and must reach the client masked, not in cleartext. - expect(getUserSettings(user.id).llm_api_key).toBe('••••••••'); + expect(svc.getUserSettings(user.id).llm_api_key).toBe('••••••••'); }); it('SET-SVC-026 — an empty user llm_api_key falls back to the admin key, still masked', () => { const { user } = createUser(testDb); setAdminDefault('llm_api_key', 'sk-admin-secret'); testDb.prepare("INSERT INTO settings (user_id, key, value) VALUES (?, 'llm_api_key', '')").run(user.id); - expect(getUserSettings(user.id).llm_api_key).toBe('••••••••'); + expect(svc.getUserSettings(user.id).llm_api_key).toBe('••••••••'); }); }); @@ -174,41 +180,41 @@ describe('getUserSettings', () => { describe('upsertSetting', () => { it('SET-SVC-008 — inserts a new setting', () => { const { user } = createUser(testDb); - upsertSetting(user.id, 'language', 'en'); - const s = getUserSettings(user.id); + svc.upsertSetting(user.id, 'language', 'en'); + const s = svc.getUserSettings(user.id); expect(s.language).toBe('en'); }); it('SET-SVC-009 — updates an existing setting (ON CONFLICT)', () => { const { user } = createUser(testDb); - upsertSetting(user.id, 'language', 'en'); - upsertSetting(user.id, 'language', 'fr'); - const s = getUserSettings(user.id); + svc.upsertSetting(user.id, 'language', 'en'); + svc.upsertSetting(user.id, 'language', 'fr'); + const s = svc.getUserSettings(user.id); expect(s.language).toBe('fr'); }); it('SET-SVC-010 — serializes object values as JSON', () => { const { user } = createUser(testDb); - upsertSetting(user.id, 'prefs', { dark: true, size: 14 }); + svc.upsertSetting(user.id, 'prefs', { dark: true, size: 14 }); const raw = testDb.prepare("SELECT value FROM settings WHERE user_id = ? AND key = 'prefs'").get(user.id) as any; expect(raw.value).toBe('{"dark":true,"size":14}'); }); it('SET-SVC-011 — serializes boolean values as strings', () => { const { user } = createUser(testDb); - upsertSetting(user.id, 'notifications', true); + svc.upsertSetting(user.id, 'notifications', true); const raw = testDb.prepare("SELECT value FROM settings WHERE user_id = ? AND key = 'notifications'").get(user.id) as any; expect(raw.value).toBe('true'); }); it('SET-SVC-012 — webhook_url passes through maybe_encrypt_api_key', () => { const { user } = createUser(testDb); - upsertSetting(user.id, 'webhook_url', 'https://hook.example.com'); + svc.upsertSetting(user.id, 'webhook_url', 'https://hook.example.com'); // With passthrough mock, value is stored as-is const raw = testDb.prepare("SELECT value FROM settings WHERE user_id = ? AND key = 'webhook_url'").get(user.id) as any; expect(raw.value).toBe('https://hook.example.com'); // But getUserSettings masks it - const s = getUserSettings(user.id); + const s = svc.getUserSettings(user.id); expect(s.webhook_url).toBe('••••••••'); }); }); @@ -218,8 +224,8 @@ describe('upsertSetting', () => { describe('bulkUpsertSettings', () => { it('SET-SVC-013 — inserts multiple settings in one call', () => { const { user } = createUser(testDb); - bulkUpsertSettings(user.id, { a: 'alpha', b: 'beta', c: 'gamma' }); - const s = getUserSettings(user.id); + svc.bulkUpsertSettings(user.id, { a: 'alpha', b: 'beta', c: 'gamma' }); + const s = svc.getUserSettings(user.id); expect(s.a).toBe('alpha'); expect(s.b).toBe('beta'); expect(s.c).toBe('gamma'); @@ -227,28 +233,28 @@ describe('bulkUpsertSettings', () => { it('SET-SVC-014 — returns the count of settings processed', () => { const { user } = createUser(testDb); - const count = bulkUpsertSettings(user.id, { x: 1, y: 2, z: 3 }); + const count = svc.bulkUpsertSettings(user.id, { x: 1, y: 2, z: 3 }); expect(count).toBe(3); }); it('SET-SVC-015 — updates existing keys (ON CONFLICT)', () => { const { user } = createUser(testDb); - upsertSetting(user.id, 'theme', 'light'); - bulkUpsertSettings(user.id, { theme: 'dark', lang: 'en' }); - const s = getUserSettings(user.id); + svc.upsertSetting(user.id, 'theme', 'light'); + svc.bulkUpsertSettings(user.id, { theme: 'dark', lang: 'en' }); + const s = svc.getUserSettings(user.id); expect(s.theme).toBe('dark'); expect(s.lang).toBe('en'); }); it('SET-SVC-016 — returns 0 for empty settings object', () => { const { user } = createUser(testDb); - const count = bulkUpsertSettings(user.id, {}); + const count = svc.bulkUpsertSettings(user.id, {}); expect(count).toBe(0); }); it('SET-SVC-017 — all changes are committed atomically (transaction)', () => { const { user } = createUser(testDb); - bulkUpsertSettings(user.id, { p: '1', q: '2' }); + svc.bulkUpsertSettings(user.id, { p: '1', q: '2' }); const rows = testDb.prepare('SELECT key FROM settings WHERE user_id = ?').all(user.id) as any[]; const keys = rows.map((r: any) => r.key); expect(keys).toContain('p'); @@ -258,10 +264,10 @@ describe('bulkUpsertSettings', () => { it('SET-SVC-018 — settings from different users do not interfere', () => { const { user: a } = createUser(testDb); const { user: b } = createUser(testDb); - bulkUpsertSettings(a.id, { shared_key: 'from-a' }); - bulkUpsertSettings(b.id, { shared_key: 'from-b' }); - expect((getUserSettings(a.id) as any).shared_key).toBe('from-a'); - expect((getUserSettings(b.id) as any).shared_key).toBe('from-b'); + svc.bulkUpsertSettings(a.id, { shared_key: 'from-a' }); + svc.bulkUpsertSettings(b.id, { shared_key: 'from-b' }); + expect((svc.getUserSettings(a.id) as any).shared_key).toBe('from-a'); + expect((svc.getUserSettings(b.id) as any).shared_key).toBe('from-b'); }); it('SET-SVC-019 — rolls back and re-throws when DB write fails mid-transaction', () => { @@ -273,8 +279,45 @@ describe('bulkUpsertSettings', () => { intercepted = true; return { run: () => { throw new Error('forced DB error'); } } as any; }); - expect(() => bulkUpsertSettings(user.id, { k: 'v' })).toThrow('forced DB error'); + expect(() => svc.bulkUpsertSettings(user.id, { k: 'v' })).toThrow('forced DB error'); expect(intercepted).toBe(true); vi.restoreAllMocks(); }); }); + +// ── post-migration quirk fixes ──────────────────────────────────────────────── + +describe('legacy quirk fixes', () => { + it('SET-SVC-027 — null serializes as the empty string, not the string "null"', () => { + const { user } = createUser(testDb); + svc.upsertSetting(user.id, 'llm_api_key', null); + const raw = testDb.prepare("SELECT value FROM settings WHERE user_id = ? AND key = 'llm_api_key'").get(user.id) as { value: string }; + expect(raw.value).toBe(''); + // The legacy "null" storage leaked back out of getDecryptedUserSetting as + // the literal string "null"; a cleared secret must read as null. + expect(svc.getDecryptedUserSetting(user.id, 'llm_api_key')).toBeNull(); + }); + + it('SET-SVC-028 — a nulled defaultable key falls through to the admin default', () => { + const { user } = createUser(testDb); + svc.setAdminUserDefaults({ mapbox_style: 'admin-style' }); + svc.upsertSetting(user.id, 'mapbox_style', null); + expect(svc.getUserSettings(user.id).mapbox_style).toBe('admin-style'); + }); + + it('SET-SVC-029 — bulk skips the masked sentinel so a stored secret survives', () => { + const { user } = createUser(testDb); + svc.upsertSetting(user.id, 'ntfy_token', 'tok-real'); + const count = svc.bulkUpsertSettings(user.id, { ntfy_topic: 'trek', ntfy_token: '••••••••' }); + expect(count).toBe(1); + // Passthrough crypto mock: the stored value must still be the real token. + const raw = testDb.prepare("SELECT value FROM settings WHERE user_id = ? AND key = 'ntfy_token'").get(user.id) as { value: string }; + expect(raw.value).toBe('tok-real'); + expect(testDb.prepare("SELECT value FROM settings WHERE user_id = ? AND key = 'ntfy_topic'").get(user.id)).toEqual({ value: 'trek' }); + }); + + it('SET-SVC-030 — bulk returns the count of keys actually written', () => { + const { user } = createUser(testDb); + expect(svc.bulkUpsertSettings(user.id, { a: '1', b: '••••••••' })).toBe(1); + }); +}); diff --git a/server/tests/unit/nest/share.service.test.ts b/server/tests/unit/nest/share.service.test.ts index 41e03af4a..18a4a5482 100644 --- a/server/tests/unit/nest/share.service.test.ts +++ b/server/tests/unit/nest/share.service.test.ts @@ -53,10 +53,11 @@ import { } from '../../helpers/factories'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { ShareService } from '../../../src/nest/share/share.service'; +import { SettingsService } from '../../../src/nest/settings/settings.service'; import { createOrUpdateShareLink, getShareLink, deleteShareLink } from '../../../src/nest/share/share.bridge'; import type { User } from '../../../src/types'; -const svc = new ShareService(new DatabaseService(testDb)); +const svc = new ShareService(new DatabaseService(testDb), new SettingsService(new DatabaseService(testDb))); beforeAll(() => { createTables(testDb); 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 5fec0ddf7..799a3e6cd 100644 --- a/server/tests/unit/plugins/plugin-host-deps.factory.test.ts +++ b/server/tests/unit/plugins/plugin-host-deps.factory.test.ts @@ -4,8 +4,8 @@ * per-plugin data db is cached, a granted db:own call works through the wired * host, and trip broadcasts are force-namespaced to plugin:{id}:{event}. * DI-native domains (budget/reservations/tags/categories/todo/packing/ - * day-notes/assignments/oauth) are constructor-injected stubs; legacy - * services/* domains stay path-mocked until their own DI migration lands. + * day-notes/assignments/oauth/llm-config) are constructor-injected stubs; + * legacy services/* domains stay path-mocked until their own DI migration lands. */ import { describe, it, expect, beforeAll, beforeEach, afterAll, vi } from 'vitest'; import fs from 'node:fs'; @@ -208,9 +208,10 @@ vi.mock('../../../src/services/tripMembership', () => ({ const { notifySend } = vi.hoisted(() => ({ notifySend: vi.fn(async () => undefined) })); vi.mock('../../../src/services/notificationService', () => ({ send: notifySend })); // userId 7 = no provider configured; everyone else resolves to a stub config. -vi.mock('../../../src/nest/llm-parse/llm-config.resolver', () => ({ - resolveLlmConfig: vi.fn((uid: number) => (uid === 7 ? null : { provider: 'openai', model: 'gpt-x', baseUrl: undefined, apiKey: 'sekret' })), -})); +// Constructor-injected stub since the resolver became DI-native (settings migration). +const llmConfigStub = { + resolve: vi.fn((uid: number) => (uid === 7 ? null : { provider: 'openai', model: 'gpt-x', baseUrl: undefined, apiKey: 'sekret' })), +} as unknown as LlmConfigResolver; const { llmExtract } = vi.hoisted(() => ({ llmExtract: vi.fn(async (input: { text?: string }) => [{ text: `answer:${input.text ?? ''}` }]) })); vi.mock('../../../src/nest/llm-parse/llm-client.factory', () => ({ createLlmClient: vi.fn(() => ({ extract: llmExtract })) })); // The per-user settings read the runtime deps delegate to (still a path mock — @@ -299,11 +300,13 @@ import type { PackingService } from '../../../src/nest/packing/packing.service'; import type { DayNotesService } from '../../../src/nest/days/day-notes.service'; import type { AssignmentsService } from '../../../src/nest/assignments/assignments.service'; import type { PluginOAuthService } from '../../../src/nest/plugins/plugin-oauth.service'; +import type { LlmConfigResolver } from '../../../src/nest/llm-parse/llm-config.resolver'; +import { DatabaseService } from '../../../src/nest/database/database.service'; // The factory under test, wired exactly like PluginsModule does — but with the // DI-native domain services replaced by the stubs above. The shim keeps the // ~45 historical call sites unchanged and supplies a default no-op router. -const factory = new PluginHostDepsFactory(budgetStub, reservationsStub, tagsStub, categoriesStub, todoStub, packingStub, oauthStub, dayNotesStub, assignmentsStub); +const factory = new PluginHostDepsFactory(budgetStub, reservationsStub, tagsStub, categoriesStub, todoStub, packingStub, oauthStub, dayNotesStub, assignmentsStub, llmConfigStub, new DatabaseService(mockDb)); const stubRouter: PluginCallRouter = { callPlugin: async () => undefined, emitPluginEvent: () => {} }; const createRealRpcHost = (id: string, granted: ReadonlySet, router: PluginCallRouter = stubRouter) => factory.create(id, granted, router);