From f691da8f7e801dc332046859d646bb88eab98f53 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 10 Aug 2026 17:42:13 +0200 Subject: [PATCH] Read online users through the facade, not a lazy require MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit admin.service.ts reached getOnlineUserIds with require('../../websocket') inside a try/catch — the last lazy require of the ws singleton in the tree, and exactly what RealtimeService exists to prevent. The facade gains the third pass-through next to broadcast and broadcastToUser, delegating at call time like its siblings so per-file vi.mock stubs keep flowing through. The catch stays at the admin call site rather than moving into the facade: an admin list that shows everyone offline is a better answer than a 500, but that judgement belongs to this caller, not to every future one. Typechecking the tests turned up two more things behind the arity errors. admin.service.test.ts built AuthService with an AtlasService in the membership slot and no mailer at all, so its last four collaborators were shifted by one. Nothing failed, because no case there reaches a path that uses them. Its getOidcSettings/updateOidcSettings wrappers referenced methods AdminService no longer has. Nothing called them: OIDC settings moved to OidcService and the two cases moved with it (ADMIN-SVC-047/048 now live in oidc.service.test.ts). Only the wrappers stayed behind. --- server/src/nest/admin/admin.bridge.ts | 1 + server/src/nest/admin/admin.service.ts | 9 ++++--- server/src/nest/realtime/realtime.service.ts | 16 +++++++++++- server/tests/unit/nest/admin.service.test.ts | 10 +++++--- .../unit/plugins/plugins-service.test.ts | 25 ++++++++++--------- 5 files changed, 42 insertions(+), 19 deletions(-) diff --git a/server/src/nest/admin/admin.bridge.ts b/server/src/nest/admin/admin.bridge.ts index a1d3d81cf..74e6dc3a0 100644 --- a/server/src/nest/admin/admin.bridge.ts +++ b/server/src/nest/admin/admin.bridge.ts @@ -49,6 +49,7 @@ const admin = new AdminService( permissions, new NotificationsService(dbs, realtime, mailer, new WebhookService(dbs), new NtfyService(dbs), notifPrefs), userCleanup, + new RealtimeService(), ); export function checkAndNotifyVersion(): Promise { diff --git a/server/src/nest/admin/admin.service.ts b/server/src/nest/admin/admin.service.ts index fd10075f2..74c28ce99 100644 --- a/server/src/nest/admin/admin.service.ts +++ b/server/src/nest/admin/admin.service.ts @@ -23,6 +23,7 @@ import { validatePassword } from '../common/passwordPolicy'; import { UserCleanupService } from '../auth/user-cleanup.service'; import { DatabaseService } from '../database/database.service'; import { AddonsService } from '../addons/addons.service'; +import { RealtimeService } from '../realtime/realtime.service'; import { PasskeyService } from '../auth/passkey.service'; import { AuthService } from '../auth/auth.service'; import { PermissionsService } from '../permissions/permissions.service'; @@ -73,6 +74,7 @@ export class AdminService { private readonly permissions: PermissionsService, private readonly notifications: NotificationsService, private readonly userCleanup: UserCleanupService, + private readonly realtime: RealtimeService, ) {} // ── User CRUD ────────────────────────────────────────────────────────────── @@ -89,9 +91,10 @@ export class AdminService { ); let onlineUserIds = new Set(); try { - // Lazy require: the ws singleton must not become a load-time dependency. - const { getOnlineUserIds } = require('../../websocket'); - onlineUserIds = getOnlineUserIds(); + // The catch stays here rather than in the facade: an admin list that shows + // everyone as offline is a better answer than a 500, and that judgement + // belongs to this caller, not to every future one. + onlineUserIds = this.realtime.getOnlineUserIds(); } catch { /* */ } diff --git a/server/src/nest/realtime/realtime.service.ts b/server/src/nest/realtime/realtime.service.ts index e8ad9468c..1b66e27e1 100644 --- a/server/src/nest/realtime/realtime.service.ts +++ b/server/src/nest/realtime/realtime.service.ts @@ -5,7 +5,7 @@ import type { TrekWsTripEventName, TrekWsUserEventName, } from '@trek/shared'; -import { broadcast, broadcastToUser } from '../../websocket'; +import { broadcast, broadcastToUser, getOnlineUserIds } from '../../websocket'; /** * Injectable facade over the websocket module singleton (roadmap Phase 0 item 3). @@ -89,4 +89,18 @@ export class RealtimeService { ): void { broadcastToUser(...args); } + + /** + * Which users currently hold an open socket. + * + * AdminService read this through a lazy `require('../../websocket')` inside a + * try/catch, which is what the facade exists to avoid. Deliberately NOT + * guarded here: a guard on the facade would swallow the failure for every + * future caller, and the one caller that wants a degraded answer rather than + * an error keeps its own catch. Delegates at call time, like its two + * siblings, so per-file vi.mock stubs keep flowing through. + */ + getOnlineUserIds(): Set { + return getOnlineUserIds(); + } } diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index 3a2edcb75..f9ad564e9 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -63,6 +63,7 @@ import { SettingsService } from '../../../src/nest/settings/settings.service'; import { AtlasService } from '../../../src/nest/atlas/atlas.service'; import { TripMembershipService } from '../../../src/nest/trip-membership/trip-membership.service'; import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service'; +import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; import { WebauthnConfigService } from '../../../src/nest/auth/webauthn-config.service'; import { AuthService } from '../../../src/nest/auth/auth.service'; import { PasskeyService } from '../../../src/nest/auth/passkey.service'; @@ -79,7 +80,11 @@ const realtime = new RealtimeService(); const permissions = new PermissionsService(dbs); const webauthn = new WebauthnConfigService(dbs); const userCleanup = new UserCleanupService(dbs); -const auth = new AuthService(dbs, permissions, new AtlasService(dbs), new TripMembershipService(dbs), webauthn, userCleanup); +// Positional and previously wrong: an AtlasService sat in the membership slot +// and the mailer was missing entirely, so `auth` was built with its last four +// collaborators shifted by one. Nothing failed, because none of the cases below +// reach a path that uses them. +const auth = new AuthService(dbs, permissions, new TripMembershipService(dbs), webauthn, userCleanup, new MailerService(dbs)); const svc = new AdminService( dbs, new AddonsService(dbs), @@ -88,6 +93,7 @@ const svc = new AdminService( permissions, makeNotificationsService(dbs, realtime), userCleanup, + realtime, ); // Legacy free-function names bound to the service, so the moved cases below read @@ -100,8 +106,6 @@ const getStats = () => svc.getStats(); const getPermissions = () => svc.getPermissions(); const savePermissions = (p: Record) => svc.savePermissions(p); const getAuditLog = (q: { limit?: string; offset?: string }) => svc.getAuditLog(q); -const getOidcSettings = () => svc.getOidcSettings(); -const updateOidcSettings = (d: Parameters[0]) => svc.updateOidcSettings(d); const saveDemoBaseline = () => svc.saveDemoBaseline(); const getGithubReleases = (perPage?: string, page?: string) => svc.getGithubReleases(perPage, page); const checkVersion = () => svc.checkVersion(); diff --git a/server/tests/unit/plugins/plugins-service.test.ts b/server/tests/unit/plugins/plugins-service.test.ts index 136e648a7..d214c76b5 100644 --- a/server/tests/unit/plugins/plugins-service.test.ts +++ b/server/tests/unit/plugins/plugins-service.test.ts @@ -20,6 +20,7 @@ vi.mock('../../../src/db/database', () => ({ db: testDb })); import { db as dbConn } from '../../../src/db/database'; import { DatabaseService } from '../../../src/nest/database/database.service'; +import { AddonsService } from '../../../src/nest/addons/addons.service'; import { PluginsService } from '../../../src/nest/plugins/plugins.service'; import { PluginsController } from '../../../src/nest/plugins/plugins.controller'; import { PluginsFeedController } from '../../../src/nest/plugins/plugins-feed.controller'; @@ -40,7 +41,7 @@ describe('PluginsService.list', () => { .run('flight', 'Flight', 'desc', 'widget', 'inactive', '1.0.0'); process.env.TREK_PLUGINS_ENABLED = 'true'; - const out = new PluginsService(new DatabaseService(dbConn)).list(); + const out = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list(); expect(out.enabled).toBe(true); expect(out.plugins).toHaveLength(1); expect(out.plugins[0]).toMatchObject({ id: 'flight', name: 'Flight', status: 'inactive' }); @@ -51,14 +52,14 @@ describe('PluginsService.list', () => { .prepare('INSERT INTO plugins (id, name, description, type, status, version) VALUES (?,?,?,?,?,?)') .run('flight', 'Flight', 'desc', 'widget', 'inactive', '1.0.0'); - const out = new PluginsService(new DatabaseService(dbConn)).list(); + const out = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list(); expect(out.enabled).toBe(true); expect(out.plugins).toHaveLength(1); }); it('reports disabled when the kill switch is off (TREK_PLUGINS_ENABLED=false)', () => { process.env.TREK_PLUGINS_ENABLED = 'false'; - const out = new PluginsService(new DatabaseService(dbConn)).list(); + const out = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list(); expect(out.enabled).toBe(false); expect(out.plugins).toEqual([]); }); @@ -78,7 +79,7 @@ describe('PluginsService.list', () => { const key = 'RWTvBn0aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789abcd'; insert('signed-one', 'acme/signed-one', key); - const p = new PluginsService(new DatabaseService(dbConn)).list().plugins[0]; + const p = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list().plugins[0]; expect(p.signed).toBe(true); // Short head…tail, for eyeballing against what the author reads out over the // phone. NOT a confidentiality measure — the key is public, and the re-trust @@ -88,7 +89,7 @@ describe('PluginsService.list', () => { it('reports unsigned for a registry plugin with no pinned key', () => { insert('plain', 'acme/plain', null); - const p = new PluginsService(new DatabaseService(dbConn)).list().plugins[0]; + const p = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list().plugins[0]; expect(p.signed).toBe(false); expect(p.keyFingerprint).toBeNull(); }); @@ -96,7 +97,7 @@ describe('PluginsService.list', () => { it('reports unsigned for a sideloaded and a dev-linked plugin (they carry no key)', () => { insert('uploaded', 'local:upload', null); insert('linked', 'local:link', null); - const plugins = new PluginsService(new DatabaseService(dbConn)).list().plugins; + const plugins = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list().plugins; expect(plugins.map((p) => [p.id, p.signed, p.source_repo])).toEqual([ ['linked', false, 'local:link'], ['uploaded', false, 'local:upload'], @@ -110,14 +111,14 @@ describe('PluginsService.list', () => { .prepare('UPDATE plugins SET update_block_code = ?, update_block_detail = ?, update_block_version = ? WHERE id = ?') .run('SIGNATURE_KEY_CHANGED', 'the key changed', '2.0.0', 'blocked'); - const byId = Object.fromEntries(new PluginsService(new DatabaseService(dbConn)).list().plugins.map((p) => [p.id, p])); + const byId = Object.fromEntries(new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list().plugins.map((p) => [p.id, p])); expect(byId.blocked.updateBlock).toEqual({ code: 'SIGNATURE_KEY_CHANGED', detail: 'the key changed', version: '2.0.0' }); expect(byId.fine.updateBlock).toBeNull(); }); it('never leaks the raw pinned key into the list response (only the fingerprint)', () => { insert('signed-one', 'acme/signed-one', 'RWTvBn0aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789abcd'); - const p = new PluginsService(new DatabaseService(dbConn)).list().plugins[0] as Record; + const p = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).list().plugins[0] as Record; expect(p.author_pubkey).toBeUndefined(); }); }); @@ -238,7 +239,7 @@ describe('PluginsService instance config', () => { testDb.prepare("INSERT INTO plugins (id, name, status, config) VALUES ('x','X','inactive','{}')").run(); testDb.prepare("INSERT INTO plugin_settings_fields (plugin_id, field_key, scope, secret) VALUES ('x','api_key','instance',1)").run(); - const svc = new PluginsService(new DatabaseService(dbConn)); + const svc = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))); const masked = svc.updateInstanceConfig('x', { api_key: 'super-secret', server: 'https://h' }); // client gets the masked view expect(masked.api_key).toBe('••••••••'); @@ -259,8 +260,8 @@ describe('PluginsService instance config', () => { }); it('throws for an unknown plugin', () => { - expect(() => new PluginsService(new DatabaseService(dbConn)).updateInstanceConfig('nope', {})).toThrow(/not found/); - expect(() => new PluginsService(new DatabaseService(dbConn)).getInstanceConfig('nope')).toThrow(/not found/); + expect(() => new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).updateInstanceConfig('nope', {})).toThrow(/not found/); + expect(() => new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))).getInstanceConfig('nope')).toThrow(/not found/); }); }); @@ -268,7 +269,7 @@ describe('PluginsService error log', () => { beforeEach(() => testDb.exec('DELETE FROM plugin_error_log')); it('lists and clears a plugin error log', () => { testDb.prepare("INSERT INTO plugin_error_log (plugin_id, level, message) VALUES ('p','error','boom')").run(); - const svc = new PluginsService(new DatabaseService(dbConn)); + const svc = new PluginsService(new DatabaseService(dbConn), new AddonsService(new DatabaseService(dbConn))); expect(svc.errors('p')).toEqual([{ ts: '2026-01-01', level: 'error', message: 'boom' }]); svc.clearErrors('p'); expect(svc.errors('p')).toEqual([]);