From 862799062ae22c36a4a2612a6cbf4d8a8bbbe48d Mon Sep 17 00:00:00 2001 From: jubnl Date: Thu, 24 Sep 2026 01:46:09 +0200 Subject: [PATCH] refactor(admin): user CRUD, stats, audit log, version check and addon config through repositories --- .../src/db/repositories/Addons.repository.ts | 43 +++ .../DocumentProviders.repository.ts | 43 ++- .../PhotoProviderFields.repository.ts | 44 +++ .../repositories/PhotoProviders.repository.ts | 37 ++ .../src/db/repositories/Users.repository.ts | 178 +++++++++ server/src/nest/admin/admin.module.ts | 29 +- server/src/nest/admin/admin.service.ts | 345 ++++++++---------- server/tests/e2e/admin.e2e.test.ts | 17 +- .../addons-config-encryption.test.ts | 75 ++++ server/tests/unit/nest/admin.service.test.ts | 137 ++++++- .../nest/admin.version-notification.test.ts | 22 +- 11 files changed, 779 insertions(+), 191 deletions(-) create mode 100644 server/tests/unit/db/repositories/addons-config-encryption.test.ts diff --git a/server/src/db/repositories/Addons.repository.ts b/server/src/db/repositories/Addons.repository.ts index 30ae6f593..ff490979f 100644 --- a/server/src/db/repositories/Addons.repository.ts +++ b/server/src/db/repositories/Addons.repository.ts @@ -47,4 +47,47 @@ export class AddonsRepository extends TrekRepository { const rows = await this.find({ enabled: true }, { orderBy: { sort_order: 'asc' } }); return rows.map((row) => toRow(row) as AddonRow); } + + // --------------------------------------------------------------------- + // Plan 3i Task 1 (`AdminService` — AD26/AD30/AD33/AD36/AD39, admin's + // addon-shelf listing + config/enabled writes). + // --------------------------------------------------------------------- + + /** + * AD26 (`admin.service.ts#listAddons`) — `SELECT * FROM addons ORDER BY + * sort_order, id`, UNFILTERED (unlike {@link listEnabled}'s `WHERE + * enabled = 1`) — admin's own catalog listing, filtered afterward in JS + * for managed-instance hiding. + */ + async listAllOrdered(): Promise { + const rows = await this.find({}, { orderBy: { sort_order: 'asc', id: 'asc' } }); + return rows.map((row) => toRow(row) as AddonRow); + } + + /** AD30/AD39 (`admin.service.ts#updateAddon`'s pre-write read and post-TX re-select, byte-identical text at both sites) — `SELECT * FROM addons WHERE id = ?`. */ + async findById(id: string): Promise { + const row = await this.findOne({ id }); + return row ? (toRow(row) as AddonRow) : null; + } + + /** AD33 (`admin.service.ts#updateAddon`, inside `uow.transactional`) — `UPDATE addons SET enabled = ? WHERE id = ?`. */ + async setEnabled(id: string, enabled: boolean): Promise { + await this.nativeUpdate({ id }, { enabled: !!enabled }); + } + + /** + * AD36 (`admin.service.ts#updateAddon`, inside `uow.transactional`) — + * `UPDATE addons SET config = ? WHERE id = ?`. The LLM-parsing addon's API + * key is already encrypted by `AdminService` + * (`prepareLlmAddonConfigForWrite`) before this call — this repository + * never touches the crypto, only persists whatever JSON blob it is + * handed (3e R6's encrypted-column precedent: the service encrypts, the + * repository passes the ciphertext through untouched). `config` is a + * `p.json()` column — MikroORM serializes the object to the stored TEXT + * column itself, the same JSON text the legacy statement's hand-called + * `JSON.stringify` produced. + */ + async setConfig(id: string, config: AddonConfig): Promise { + await this.nativeUpdate({ id }, { config }); + } } diff --git a/server/src/db/repositories/DocumentProviders.repository.ts b/server/src/db/repositories/DocumentProviders.repository.ts index 3be73b077..cfb419148 100644 --- a/server/src/db/repositories/DocumentProviders.repository.ts +++ b/server/src/db/repositories/DocumentProviders.repository.ts @@ -1,5 +1,5 @@ import type { DocumentProviders } from '../entities/DocumentProviders.entity'; -import { type AssertRowKeys } from './_shared/rows'; +import { toRow, type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; /** DSCTRL2's wider catalog row — id/name/description/icon, no `enabled`/`sort_order` (those only gate/order the read, they are never returned to the client). */ @@ -69,4 +69,45 @@ export class DocumentProvidersRepository extends TrekRepository ({ id: r.id as string, name: r.name, description: r.description ?? null, icon: r.icon ?? null })); } + + // --------------------------------------------------------------------- + // Plan 3i Task 1 (`AdminService` — AD29/AD32/AD35/AD38, admin's + // addon-shelf listing + `enabled` toggle — R6: this repository is 3h's + // doc-sync task's to own; these four are additive, on top of its three + // read shapes above, for admin's reuse). + // --------------------------------------------------------------------- + + /** + * AD29 (`admin.service.ts#listAddons`) — `SELECT id, name, description, + * icon, enabled, sort_order FROM document_providers ORDER BY sort_order, + * id`, UNFILTERED (unlike {@link listEnabledCatalog}'s `WHERE enabled = + * 1`). + */ + async listAllOrdered(): Promise { + const rows = await this.find({}, { orderBy: { sort_order: 'asc', id: 'asc' } }); + return rows.map((row) => toRow(row) as DocumentProviderRow); + } + + /** AD32 (`admin.service.ts#updateAddon`'s pre-write read) — `SELECT * FROM document_providers WHERE id = ?`. */ + async findById(id: string): Promise { + const row = await this.findOne({ id }); + return row ? (toRow(row) as DocumentProviderRow) : null; + } + + /** + * AD35 (`admin.service.ts#updateAddon`, inside `uow.transactional`, the + * Documents-off cascade) — `UPDATE document_providers SET enabled = 0`, + * deliberately UNSCOPED (every row, no WHERE). Named `disableAll`, not a + * generic `setEnabled` overload that could be called with an + * accidentally omitted id (R4), same reasoning as + * `PhotoProvidersRepository.disableAll` (AD34). + */ + async disableAll(): Promise { + await this.nativeUpdate({}, { enabled: 0 }); + } + + /** AD38 (`admin.service.ts#updateAddon`, inside `uow.transactional`) — `UPDATE document_providers SET enabled = ? WHERE id = ?`. */ + async setEnabled(id: string, enabled: number): Promise { + await this.nativeUpdate({ id }, { enabled }); + } } diff --git a/server/src/db/repositories/PhotoProviderFields.repository.ts b/server/src/db/repositories/PhotoProviderFields.repository.ts index 557961a94..06fa7589b 100644 --- a/server/src/db/repositories/PhotoProviderFields.repository.ts +++ b/server/src/db/repositories/PhotoProviderFields.repository.ts @@ -37,4 +37,48 @@ export class PhotoProviderFieldsRepository extends TrekRepository toRow(row) as PhotoProviderFieldRow); } + + /** + * AD28 (`admin.service.ts#listAddons`) — `SELECT provider_id, field_key, + * label, input_type, placeholder, required, secret, settings_key, + * payload_key, sort_order FROM photo_provider_fields ORDER BY sort_order, + * id`. A NARROWER projection than {@link listAllOrdered} — no `id`, no + * `hint` — a genuinely different column set for a different caller, not a + * superset reuse (Task 0's report: parity is column-exact here). + */ + async listAllOrderedForAdminShelf(): Promise { + const rows = await this.find( + {}, + { + fields: ['provider_id', 'field_key', 'label', 'input_type', 'placeholder', 'required', 'secret', 'settings_key', 'payload_key', 'sort_order'], + orderBy: { sort_order: 'asc', id: 'asc' }, + }, + ); + return rows.map((row) => ({ + provider_id: row.provider_id, + field_key: row.field_key, + label: row.label, + input_type: row.input_type, + placeholder: row.placeholder ?? null, + required: row.required ?? null, + secret: row.secret ?? null, + settings_key: row.settings_key ?? null, + payload_key: row.payload_key ?? null, + sort_order: row.sort_order ?? null, + })); + } +} + +/** AD28's own row shape — see {@link PhotoProviderFieldsRepository.listAllOrderedForAdminShelf}. */ +export interface PhotoProviderFieldForAdminRow { + provider_id: string; + field_key: string; + label: string; + input_type: string; + placeholder: string | null; + required: number | null; + secret: number | null; + settings_key: string | null; + payload_key: string | null; + sort_order: number | null; } diff --git a/server/src/db/repositories/PhotoProviders.repository.ts b/server/src/db/repositories/PhotoProviders.repository.ts index 03a795ee9..e703c7816 100644 --- a/server/src/db/repositories/PhotoProviders.repository.ts +++ b/server/src/db/repositories/PhotoProviders.repository.ts @@ -55,4 +55,41 @@ export class PhotoProvidersRepository extends TrekRepository { const row = await this.findOne({ id }, { fields: ['enabled'] }); return row ? { enabled: row.enabled ?? null } : null; } + + // --------------------------------------------------------------------- + // Plan 3i Task 1 (`AdminService` — AD27/AD31/AD34/AD37/AD40, admin's + // addon-shelf listing + enabled writes). + // --------------------------------------------------------------------- + + /** + * AD27 (`admin.service.ts#listAddons`) — `SELECT id, name, description, + * icon, enabled, sort_order FROM photo_providers ORDER BY sort_order, + * id`, UNFILTERED (unlike {@link listEnabled}'s `WHERE enabled = 1`). + */ + async listAllOrdered(): Promise { + const rows = await this.find({}, { orderBy: { sort_order: 'asc', id: 'asc' } }); + return rows.map((row) => toRow(row) as PhotoProviderRow); + } + + /** AD31/AD40 (`admin.service.ts#updateAddon`'s pre-write read and post-TX re-select, byte-identical text at both sites) — `SELECT * FROM photo_providers WHERE id = ?`. */ + async findById(id: string): Promise { + const row = await this.findOne({ id }); + return row ? (toRow(row) as PhotoProviderRow) : null; + } + + /** + * AD34 (`admin.service.ts#updateAddon`, inside `uow.transactional`, the + * Journey-off cascade — "Journey off takes its providers with it") — + * `UPDATE photo_providers SET enabled = 0`, deliberately UNSCOPED (every + * row, no WHERE). Named `disableAll`, not a generic `setEnabled` overload + * that could be called with an accidentally omitted id (R4). + */ + async disableAll(): Promise { + await this.nativeUpdate({}, { enabled: 0 }); + } + + /** AD37 (`admin.service.ts#updateAddon`, inside `uow.transactional`) — `UPDATE photo_providers SET enabled = ? WHERE id = ?`. `enabled` stays the raw stored int (this entity's column is `p.integer()`, not `p.boolean()` — see {@link listEnabled}'s docstring), matching the legacy `data.enabled ? 1 : 0` bind exactly. */ + async setEnabled(id: string, enabled: number): Promise { + await this.nativeUpdate({ id }, { enabled }); + } } diff --git a/server/src/db/repositories/Users.repository.ts b/server/src/db/repositories/Users.repository.ts index e29715e6a..b20ce9f80 100644 --- a/server/src/db/repositories/Users.repository.ts +++ b/server/src/db/repositories/Users.repository.ts @@ -191,6 +191,52 @@ export interface ResetTargetRow { password_version: number; } +/** AD1 (`admin.service.ts#listUsers`) — the admin user-list projection. */ +export interface AdminUserListRow { + id: number; + username: string; + email: string; + role: string; + avatar: string | null; + created_at: string | null; + updated_at: string | null; + last_login: string | null; +} + +/** AD5/AD14 (`admin.service.ts#createUser`/`updateUser`'s post-write re-select), byte-identical text at both sites. */ +export interface AdminUserSummaryRow { + id: number; + username: string; + email: string; + role: string; + created_at: string | null; + updated_at: string | null; +} + +/** AD16 (`admin.service.ts#resetUserMfa`'s pre-write read). */ +export interface AdminUserMfaRow { + id: number; + email: string; + mfa_enabled: number | null; +} + +/** AD4 (`admin.service.ts#createUser`'s write) — narrower than {@link NewUserRow}: no `first_seen_version`/`login_count`/oidc/avatar columns named, letting the entity's own defaults (`'0.0.0'`/`0`) fill them, the same values the legacy INSERT's omitted columns fell back to via the schema's `DEFAULT`. */ +export interface NewAdminUserRow { + username: string; + email: string; + password_hash: string; + role: string; +} + +/** AD11 (`admin.service.ts#updateUser`, inside `uow.transactional`) — the columns the legacy COALESCE-shaped UPDATE can touch; a key the caller omits is left unchanged, the same net effect as binding NULL into `COALESCE(?, column)`. */ +export interface AdminEditPatch { + username?: string; + email?: string; + role?: string; + password_hash?: string; + password_version?: number; +} + export class UsersRepository extends TrekRepository { /** * **Ruling (Plan 3b Task 1 fix round, supersedes Plan 3a's I1 "`refresh: @@ -1329,6 +1375,138 @@ export class UsersRepository extends TrekRepository { async clearAirtrailApiKey(id: number): Promise { await this.nativeUpdate({ id }, { airtrail_api_key: null }); } + + // --------------------------------------------------------------------- + // Plan 3i Task 1 (`AdminService` — AD1-AD17, admin's user-CRUD surface). + // Every statement not already covered by an existing method above + // (`findById` = AD6, `getRole` = AD9, `countAdmins` = AD10, `disableMfa` + // = AD17, `findIdAndEmail` = AD15 — all reused as-is, no new method). + // --------------------------------------------------------------------- + + /** + * AD1 (`listUsers`) — `SELECT id, username, email, role, avatar, + * created_at, updated_at, last_login FROM users WHERE COALESCE(is_guest, + * 0) = 0 ORDER BY created_at DESC`. `is_guest: 0` is exact parity for the + * COALESCE guard (same reasoning as `countNonGuest` above — the column is + * `NOT NULL DEFAULT 0`). + */ + async listForAdmin(): Promise { + const rows = await this.find( + { is_guest: 0 }, + { + fields: ['id', 'username', 'email', 'role', 'avatar', 'created_at', 'updated_at', 'last_login'], + orderBy: { created_at: 'desc' }, + }, + ); + return rows.map((row) => ({ + id: row.id, + username: row.username, + email: row.email, + role: row.role, + avatar: row.avatar ?? null, + created_at: row.created_at ?? null, + updated_at: row.updated_at ?? null, + last_login: row.last_login ?? null, + })); + } + + /** + * AD2 (`createUser`) — `SELECT id FROM users WHERE username = ? AND + * COALESCE(is_guest, 0) = 0`. Case-**sensitive** (plain equality, no + * `LOWER()`) — distinct from `findIdByUsernameCIAny`'s folded comparison, + * which is a genuinely different legacy statement. + */ + async findIdByUsernameExact(username: string): Promise { + const row = await this.findOne({ username, is_guest: 0 }, { fields: ['id'] }); + return row?.id ?? null; + } + + /** AD3 (`createUser`) — `SELECT id FROM users WHERE email = ? AND COALESCE(is_guest, 0) = 0`. Case-sensitive, same reasoning as {@link findIdByUsernameExact}. */ + async findIdByEmailExact(email: string): Promise { + const row = await this.findOne({ email, is_guest: 0 }, { fields: ['id'] }); + return row?.id ?? null; + } + + /** + * AD4 (`createUser`'s write) — `INSERT INTO users (username, email, + * password_hash, role) VALUES (?, ?, ?, ?)`. Only these four columns are + * named, matching the legacy statement's own column list exactly; every + * other column (`first_seen_version`, `login_count`, …) is left to the + * entity's own default the same way the legacy INSERT left them to the + * schema's `DEFAULT` — not reused from {@link insertUser} (AU10/O14), + * whose `NewUserRow` requires `first_seen_version` as a caller-supplied + * value, a column this statement never named at all. Returns the + * generated id only (no re-select) — AD5 is a separate, distinct + * statement handled by {@link findAdminSummary}. + */ + async insertAdminCreatedUser(row: NewAdminUserRow): Promise { + return await this.insert({ + username: row.username, + email: row.email, + password_hash: row.password_hash, + role: row.role, + }); + } + + /** + * AD5/AD14 (`createUser`'s post-insert re-select, `updateUser`'s + * post-update re-select) — `SELECT id, username, email, role, created_at, + * updated_at FROM users WHERE id = ?`, byte-identical text at both sites, + * one method (D4). + */ + async findAdminSummary(id: number): Promise { + const row = await this.findOne({ id }, { fields: ['id', 'username', 'email', 'role', 'created_at', 'updated_at'] }); + return row + ? { id: row.id, username: row.username, email: row.email, role: row.role, created_at: row.created_at ?? null, updated_at: row.updated_at ?? null } + : null; + } + + /** + * AD7 (`updateUser`) — `SELECT id FROM users WHERE username = ? AND id != + * ? AND COALESCE(is_guest, 0) = 0`. Case-**sensitive**, excludes the + * user's own row — distinct from `findIdByUsernameCI`'s `LOWER()`-folded, + * otherwise identically-shaped statement (UP5's own-username check). + */ + async findIdByUsernameExactExcluding(username: string, excludeId: number): Promise { + const row = await this.findOne({ username, id: { $ne: excludeId }, is_guest: 0 }, { fields: ['id'] }); + return row?.id ?? null; + } + + /** AD8 (`updateUser`) — `SELECT id FROM users WHERE email = ? AND id != ? AND COALESCE(is_guest, 0) = 0`. Case-sensitive, excludes self, same reasoning as {@link findIdByUsernameExactExcluding}. */ + async findIdByEmailExactExcluding(email: string, excludeId: number): Promise { + const row = await this.findOne({ email, id: { $ne: excludeId }, is_guest: 0 }, { fields: ['id'] }); + return row?.id ?? null; + } + + /** + * AD11 (`updateUser`, inside `uow.transactional` alongside AD12/AD13) — + * `UPDATE users SET username=COALESCE(?,username), email=COALESCE(?,email), + * role=COALESCE(?,role), password_hash=COALESCE(?,password_hash), + * password_version=COALESCE(?,password_version), updated_at=CURRENT_TIMESTAMP + * WHERE id=?`. The legacy binds a NULL parameter for "no change" — a + * `nativeUpdate` that simply omits an unchanged key from `patch` has the + * identical net effect (coalesceParam: the new value wins when the + * caller supplies a key, the existing column wins when it is left out) — + * the caller decides which keys to include, the same discipline + * `patchProfile` (UP7) already uses for this table. `updated_at` is + * ALWAYS stamped, even when `patch` is `{}` — matching the legacy + * statement's own unconditional `updated_at = CURRENT_TIMESTAMP` (every + * `updateUser` call runs this UPDATE regardless of what changed). + */ + async applyAdminEdit(id: number, patch: AdminEditPatch): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { ...patch, updated_at: currentTimestamp(platform) }); + } + + /** + * AD16 (`resetUserMfa`'s pre-write read) — `SELECT id, email, mfa_enabled + * FROM users WHERE id = ?`. Distinct from `getMfaEnabled` (MP2/AU23/AU27), + * which selects only `mfa_enabled` for a different caller. + */ + async findIdEmailMfaEnabled(id: number): Promise { + const row = await this.findOne({ id }, { fields: ['id', 'email', 'mfa_enabled'] }); + return row ? { id: row.id, email: row.email, mfa_enabled: row.mfa_enabled ?? null } : null; + } } /** diff --git a/server/src/nest/admin/admin.module.ts b/server/src/nest/admin/admin.module.ts index f26738318..18b59c789 100644 --- a/server/src/nest/admin/admin.module.ts +++ b/server/src/nest/admin/admin.module.ts @@ -1,4 +1,5 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; import { TokensModule } from '../tokens/tokens.module'; import { OauthModule } from '../oauth/oauth.module'; import { KitineraryExtractorModule } from '../booking-import/kitinerary-extractor.module'; @@ -23,9 +24,35 @@ import { PackingModule } from '../packing/packing.module'; // not @Global, so the import must be explicit. import { PermissionsModule } from '../permissions/permissions.module'; import { AppConfigModule } from '../app-config/app-config.module'; +import { Users } from '../../db/entities/Users.entity'; +import { AuditLog } from '../../db/entities/AuditLog.entity'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import { Addons } from '../../db/entities/Addons.entity'; +import { PhotoProviders } from '../../db/entities/PhotoProviders.entity'; +import { PhotoProviderFields } from '../../db/entities/PhotoProviderFields.entity'; +import { DocumentProviders } from '../../db/entities/DocumentProviders.entity'; +import { McpTokens } from '../../db/entities/McpTokens.entity'; +import { OauthTokens } from '../../db/entities/OauthTokens.entity'; +import { Trips } from '../../db/entities/Trips.entity'; +import { Places } from '../../db/entities/Places.entity'; +import { TripFiles } from '../../db/entities/TripFiles.entity'; +/** + * MikroOrmModule.forFeature registers the ten repositories `AdminService`'s + * `@InjectRepository` constructor needs (Plan 3i Task 1 — AD1-AD40's + * conversion off `DatabaseService`): `Users`/`AuditLog`/`AppSettings` for the + * user-CRUD + audit-log + instance-settings surface, `Addons`/ + * `PhotoProviders`/`PhotoProviderFields`/`DocumentProviders` for the addon + * shelf, `McpTokens`/`OauthTokens` for the password-reset session revoke, and + * `Trips`/`Places`/`TripFiles` (owned by other domains, 3c/3e) for + * `getStats`'s three bare cross-domain counts — the same forFeature + + * @InjectRepository wiring pattern `AddonsModule` already uses. + */ @Module({ - imports: [AppConfigModule, PluginsRuntimeModule, SettingsModule, AuditModule, AddonsModule, AuthModule, NotificationsModule, PackingModule, PermissionsModule, TokensModule, OauthModule, SchedulingModule, KitineraryExtractorModule], + imports: [ + MikroOrmModule.forFeature([Users, AuditLog, AppSettings, Addons, PhotoProviders, PhotoProviderFields, DocumentProviders, McpTokens, OauthTokens, Trips, Places, TripFiles]), + AppConfigModule, PluginsRuntimeModule, SettingsModule, AuditModule, AddonsModule, AuthModule, NotificationsModule, PackingModule, PermissionsModule, TokensModule, OauthModule, SchedulingModule, KitineraryExtractorModule, + ], controllers: [AdminController], providers: [AdminService, VersionCheckJob, DemoResetJob], }) diff --git a/server/src/nest/admin/admin.service.ts b/server/src/nest/admin/admin.service.ts index 20fb50c45..6c44ba37b 100644 --- a/server/src/nest/admin/admin.service.ts +++ b/server/src/nest/admin/admin.service.ts @@ -1,4 +1,5 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import bcrypt from 'bcryptjs'; import crypto from 'crypto'; import fs from 'fs'; @@ -14,14 +15,12 @@ import { updateJwtSecret } from '../../config'; import { revokeUserSessions, revokeUserSessionsForClient } from '../../mcp/sessionManager'; import { invalidateMcpSessions } from '../../mcp'; import { emitUserDeleted } from '../../plugin-user-lifecycle'; -import type { User, Addon } from '../../types'; import { maybe_encrypt_api_key, decrypt_api_key } from '../common/crypto/apiKeyCrypto'; import { avatarUrl } from '../common/avatarUrl'; import { prepareLlmAddonConfigForWrite, maskLlmAddonConfig } from '../llm-parse/llm-config'; import { getPhotoProviderConfig } from '../memories/memories.helpers'; import { validatePassword } from '../common/passwordPolicy'; import { UserCleanupService } from '../auth/user-cleanup.service'; -import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { AddonsService } from '../addons/addons.service'; import { RealtimeService } from '../realtime/realtime.service'; @@ -30,6 +29,31 @@ import { AuthService } from '../auth/auth.service'; import { PermissionsService } from '../permissions/permissions.service'; import { PERMISSION_ACTIONS } from '../permissions/permissions.service'; import { NotificationsService } from '../notifications/notifications.service'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository, AdminEditPatch } from '../../db/repositories/Users.repository'; +import { AuditLog } from '../../db/entities/AuditLog.entity'; +import type { AuditLogRepository } from '../../db/repositories/AuditLog.repository'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Addons } from '../../db/entities/Addons.entity'; +import type { AddonsRepository } from '../../db/repositories/Addons.repository'; +import { PhotoProviders } from '../../db/entities/PhotoProviders.entity'; +import type { PhotoProvidersRepository } from '../../db/repositories/PhotoProviders.repository'; +import { PhotoProviderFields } from '../../db/entities/PhotoProviderFields.entity'; +import type { PhotoProviderFieldsRepository } from '../../db/repositories/PhotoProviderFields.repository'; +import { DocumentProviders } from '../../db/entities/DocumentProviders.entity'; +import type { DocumentProvidersRepository } from '../../db/repositories/DocumentProviders.repository'; +import { McpTokens } from '../../db/entities/McpTokens.entity'; +import type { McpTokensRepository } from '../../db/repositories/McpTokens.repository'; +import { OauthTokens } from '../../db/entities/OauthTokens.entity'; +import type { OauthTokensRepository } from '../../db/repositories/OauthTokens.repository'; +import { Trips } from '../../db/entities/Trips.entity'; +import type { TripsRepository } from '../../db/repositories/Trips.repository'; +import { Places } from '../../db/entities/Places.entity'; +import type { PlacesRepository } from '../../db/repositories/Places.repository'; +import { TripFiles } from '../../db/entities/TripFiles.entity'; +import type { TripFilesRepository } from '../../db/repositories/TripFiles.repository'; +import type { AddonConfig } from '../../db/entities/Addons.entity'; import { BCRYPT_COST, compareVersions, @@ -48,18 +72,25 @@ const GITHUB_MAX_BYTES = 2_000_000; const VERSION_FAILURE_TTL = 60_000; /** - * Admin domain service — owns the admin SQL (folded from the legacy - * services/adminService.ts with the 2026-08 migration): user CRUD, instance - * stats, the permission matrix, the audit-log read side, OIDC settings, the - * demo baseline, GitHub release/version checks, invite tokens, the three - * places feature toggles, addons + photo providers, MCP tokens, OAuth sessions - * and JWT rotation. + * Admin domain service — owns admin's data access (folded from the legacy + * services/adminService.ts with the 2026-08 migration, converted onto + * repositories by Plan 3i Task 1): user CRUD, instance stats, the permission + * matrix, the audit-log read side, OIDC settings, the demo baseline, GitHub + * release/version checks, invite tokens, the three places feature toggles, + * addons + photo providers, MCP tokens, OAuth sessions and JWT rotation. * - * Every quirk relocated byte-for-byte: the `||` falsy defaults (never `??`), - * post-insert/post-update re-selects instead of RETURNING, the COALESCE partial - * update, the #1362 guest exclusions and the exact error strings. The legacy - * `{ error, status }` envelope is the return contract — the controller's `ok()` - * helper turns it into an HttpException, so nothing here throws. + * Every quirk stays byte-for-byte: the `||` falsy defaults (never `??`), + * post-insert/post-update re-selects instead of RETURNING, the COALESCE-shaped + * partial update (now a repository call that omits unchanged keys — the same + * net effect), the #1362 guest exclusions and the exact error strings. The + * legacy `{ error, status }` envelope is the return contract — the + * controller's `ok()` helper turns it into an HttpException, so nothing here + * throws. + * + * The three self-protection invariants (own-account delete, own-MFA reset, + * last-admin demote) stay exactly where the legacy had them: plain JS + * comparisons in this service, never folded into a repository method's WHERE + * clause (R4). * * The bag-tracking/collab-feature toggles, user defaults, passkey reset and * packing templates delegate to the services that own those tables. The pure @@ -69,7 +100,18 @@ const VERSION_FAILURE_TTL = 60_000; @Injectable() export class AdminService { constructor( - private readonly db: DatabaseService, + @InjectRepository(Users) private readonly users: UsersRepository, + @InjectRepository(AuditLog) private readonly auditLog: AuditLogRepository, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Addons) private readonly addonsRepo: AddonsRepository, + @InjectRepository(PhotoProviders) private readonly photoProviders: PhotoProvidersRepository, + @InjectRepository(PhotoProviderFields) private readonly photoProviderFields: PhotoProviderFieldsRepository, + @InjectRepository(DocumentProviders) private readonly documentProviders: DocumentProvidersRepository, + @InjectRepository(McpTokens) private readonly mcpTokens: McpTokensRepository, + @InjectRepository(OauthTokens) private readonly oauthTokens: OauthTokensRepository, + @InjectRepository(Trips) private readonly trips: TripsRepository, + @InjectRepository(Places) private readonly places: PlacesRepository, + @InjectRepository(TripFiles) private readonly tripFiles: TripFilesRepository, private readonly addons: AddonsService, private readonly passkeys: PasskeyService, private readonly auth: AuthService, @@ -85,13 +127,7 @@ export class AdminService { async listUsers() { // Guests (#1362) are accountless trip participants, not real users — keep them out // of admin user management entirely. - const users = this.db.all< - Pick & { - avatar?: string | null; - } - >( - 'SELECT id, username, email, role, avatar, created_at, updated_at, last_login FROM users WHERE COALESCE(is_guest, 0) = 0 ORDER BY created_at DESC', - ); + const users = await this.users.listForAdmin(); let onlineUserIds = new Set(); try { // The catch stays here rather than in the facade: an admin list that shows @@ -128,36 +164,33 @@ export class AdminService { } // Guests (#1362) live in a reserved synthetic namespace; never let one block a real account. - const existingUsername = this.db.get('SELECT id FROM users WHERE username = ? AND COALESCE(is_guest, 0) = 0', username); + const existingUsername = await this.users.findIdByUsernameExact(username); if (existingUsername) return { error: 'Username already taken', status: 409 }; - const existingEmail = this.db.get('SELECT id FROM users WHERE email = ? AND COALESCE(is_guest, 0) = 0', email); + const existingEmail = await this.users.findIdByEmailExact(email); if (existingEmail) return { error: 'Email already taken', status: 409 }; const passwordHash = bcrypt.hashSync(password, BCRYPT_COST); - const result = this.db.run( - 'INSERT INTO users (username, email, password_hash, role) VALUES (?, ?, ?, ?)', - username, email, passwordHash, data.role || 'user', - ); + const insertedId = await this.users.insertAdminCreatedUser({ + username, email, password_hash: passwordHash, role: data.role || 'user', + }); - const user = this.db.get( - 'SELECT id, username, email, role, created_at, updated_at FROM users WHERE id = ?', - result.lastInsertRowid, - ); + const user = await this.users.findAdminSummary(insertedId); return { user, - insertedId: Number(result.lastInsertRowid), + insertedId, auditDetails: { username, email, role: data.role || 'user' }, }; } async updateUser(id: string, data: { username?: string; email?: string; role?: string; password?: string }) { + const userId = Number(id); const username = typeof data.username === 'string' ? data.username.trim() : data.username; const email = typeof data.email === 'string' ? data.email.trim() : data.email; const { role, password } = data; - const user = this.db.get('SELECT * FROM users WHERE id = ?', id); + const user = await this.users.findById(userId); if (!user) return { error: 'User not found', status: 404 }; @@ -171,11 +204,11 @@ export class AdminService { if (email === '') return { error: 'Email cannot be empty', status: 400 }; if (username && username !== user.username) { - const conflict = this.db.get('SELECT id FROM users WHERE username = ? AND id != ? AND COALESCE(is_guest, 0) = 0', username, id); + const conflict = await this.users.findIdByUsernameExactExcluding(username, userId); if (conflict) return { error: 'Username already taken', status: 409 }; } if (email && email !== user.email) { - const conflict = this.db.get('SELECT id FROM users WHERE email = ? AND id != ? AND COALESCE(is_guest, 0) = 0', email, id); + const conflict = await this.users.findIdByEmailExactExcluding(email, userId); if (conflict) return { error: 'Email already taken', status: 409 }; } @@ -187,10 +220,13 @@ export class AdminService { // Don't let the admin UI demote the last remaining admin — that would leave the // instance with no one able to manage it (and on OIDC-only setups, no recovery). #1274 + // SECURITY: plain JS comparisons, deliberately never folded into a + // repository method's WHERE clause (R4) — two repository reads, one + // service-level `if`. if (role && role !== 'admin') { - const current = this.db.get<{ role?: string }>('SELECT role FROM users WHERE id = ?', id); - if (current?.role === 'admin') { - const adminCount = this.db.get<{ count: number }>("SELECT COUNT(*) as count FROM users WHERE role = 'admin'")!.count; + const currentRole = await this.users.getRole(userId); + if (currentRole === 'admin') { + const adminCount = await this.users.countAdmins(); if (adminCount <= 1) return { error: 'Cannot remove the last admin', status: 400 }; } } @@ -200,33 +236,31 @@ export class AdminService { // accepting every cookie the intruder already holds, so the one action taken // to lock them out was the one action that did not. Both self-service paths // (changePassword, resetPassword) have always done this; this one had not. - const newPv = password ? ((user as User & { password_version?: number }).password_version ?? 0) + 1 : null; + const newPv = password ? (user.password_version ?? 0) + 1 : null; + const patch: AdminEditPatch = {}; + if (username) patch.username = username; + if (email) patch.email = email; + if (role) patch.role = role; + if (password) { + patch.password_hash = passwordHash!; + patch.password_version = newPv!; + } + + // The password-reset transaction's CURRENT boundary, preserved exactly + // (R4): the users UPDATE, the mcp_tokens DELETE and the oauth_tokens + // revoke stay inside the SAME uow.transactional call, never split across + // separate un-transacted repository calls. await this.uow.transactional(async () => { - this.db.run( - ` - UPDATE users SET - username = COALESCE(?, username), - email = COALESCE(?, email), - role = COALESCE(?, role), - password_hash = COALESCE(?, password_hash), - password_version = COALESCE(?, password_version), - updated_at = CURRENT_TIMESTAMP - WHERE id = ? - `, - username || null, email || null, role || null, passwordHash, newPv, id, - ); + await this.users.applyAdminEdit(userId, patch); if (password) { // The version bump only invalidates JWT cookies. These two stores carry // their own credentials and are revoked separately, exactly as the // self-service paths do it. - this.db.run('DELETE FROM mcp_tokens WHERE user_id = ?', id); + await this.mcpTokens.deleteAllForUser(userId); try { - this.db.run( - 'UPDATE oauth_tokens SET revoked_at = CURRENT_TIMESTAMP WHERE user_id = ? AND revoked_at IS NULL', - id, - ); + await this.oauthTokens.revokeAllForUser(userId); } catch { /* very old installs predate oauth_tokens */ } } }); @@ -235,7 +269,7 @@ export class AdminService { try { revokeUserSessions(Number(id)); } catch { /* best-effort, same as elsewhere */ } } - const updated = this.db.get('SELECT id, username, email, role, created_at, updated_at FROM users WHERE id = ?', id); + const updated = await this.users.findAdminSummary(userId); const changed: string[] = []; if (username) changed.push('username'); @@ -251,11 +285,13 @@ export class AdminService { } async deleteUser(id: string, currentUserId: number) { + // SECURITY: plain JS comparison, deliberately kept exactly here — never + // folded into a repository method's WHERE clause (R4). if (Number.parseInt(id) === currentUserId) { return { error: 'Cannot delete own account', status: 400 }; } - const userToDel = this.db.get<{ id: number; email: string }>('SELECT id, email FROM users WHERE id = ?', id); + const userToDel = await this.users.findIdAndEmail(Number(id)); if (!userToDel) return { error: 'User not found', status: 404 }; await this.userCleanup.deleteUserCompletely(userToDel.id); @@ -280,21 +316,17 @@ export class AdminService { */ async resetUserMfa(id: string, actingUserId: number): Promise<{ error?: string; status?: number; success?: boolean; email?: string }> { const targetId = Number(id); + // SECURITY: plain JS comparison, deliberately kept exactly here — never + // folded into a repository method's WHERE clause (R4). if (targetId === actingUserId) { return { error: 'Use Settings to change your own two-factor setup', status: 400 }; } - const target = this.db.get<{ id: number; email: string; mfa_enabled: number | boolean }>( - 'SELECT id, email, mfa_enabled FROM users WHERE id = ?', - targetId, - ); + const target = await this.users.findIdEmailMfaEnabled(targetId); if (!target) return { error: 'User not found', status: 404 }; // Same three columns disableMfa clears, so an admin reset and a self-service // disable leave the account in exactly one state rather than two. - this.db.run( - 'UPDATE users SET mfa_enabled = 0, mfa_secret = NULL, mfa_backup_codes = NULL, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - targetId, - ); + await this.users.disableMfa(targetId); return { success: true, email: target.email }; } @@ -302,10 +334,10 @@ export class AdminService { // ── Stats ────────────────────────────────────────────────────────────────── async getStats() { - const totalUsers = this.db.get<{ count: number }>('SELECT COUNT(*) as count FROM users WHERE COALESCE(is_guest, 0) = 0')!.count; - const totalTrips = this.db.get<{ count: number }>('SELECT COUNT(*) as count FROM trips')!.count; - const totalPlaces = this.db.get<{ count: number }>('SELECT COUNT(*) as count FROM places')!.count; - const totalFiles = this.db.get<{ count: number }>('SELECT COUNT(*) as count FROM trip_files')!.count; + const totalUsers = await this.users.countNonGuest(); + const totalTrips = await this.trips.count(); + const totalPlaces = await this.places.count(); + const totalFiles = await this.tripFiles.count(); return { totalUsers, totalTrips, totalPlaces, totalFiles }; } @@ -335,30 +367,8 @@ export class AdminService { const limit = Math.min(Math.max(Number.isFinite(limitRaw) ? limitRaw : 100, 1), 500); const offset = Math.max(Number.isFinite(offsetRaw) ? offsetRaw : 0, 0); - type Row = { - id: number; - created_at: string; - user_id: number | null; - username: string | null; - user_email: string | null; - action: string; - resource: string | null; - details: string | null; - ip: string | null; - }; - - const rows = this.db.all( - ` - SELECT a.id, a.created_at, a.user_id, u.username, u.email as user_email, a.action, a.resource, a.details, a.ip - FROM audit_log a - LEFT JOIN users u ON u.id = a.user_id - ORDER BY a.id DESC - LIMIT ? OFFSET ? - `, - limit, offset, - ); - - const total = this.db.get<{ c: number }>('SELECT COUNT(*) as c FROM audit_log')!.c; + const rows = await this.auditLog.listPage(limit, offset); + const total = await this.auditLog.count(); const entries = rows.map((r) => { // Unparseable details fall back to the raw string rather than the old @@ -516,15 +526,14 @@ export class AdminService { const result = await this.checkVersion(); if (!result.update_available) return; - const lastNotified = this.db.get<{ value: string }>( - 'SELECT value FROM app_settings WHERE key = ?', 'last_notified_version', - )?.value; + const lastNotified = await this.appSettings.getValue('last_notified_version'); if (lastNotified === result.latest) return; - this.db.run( - 'INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)', - 'last_notified_version', result.latest, - ); + // INSERT OR REPLACE shape — the SAME dialect storage's `upsertOrReplace` + // (Plan 3i Task 2, SS1) already added to AppSettingsRepository, reused + // here rather than adding a second method for the identical SQL text + // (`INSERT OR REPLACE INTO app_settings (key, value) VALUES (?, ?)`). + await this.appSettings.upsertOrReplace('last_notified_version', result.latest); await this.notifications.send({ event: 'version_available', @@ -541,8 +550,7 @@ export class AdminService { // ── Addons ───────────────────────────────────────────────────────────────── async listAddons() { - const addons = this.db - .all('SELECT * FROM addons ORDER BY sort_order, id') + const addons = (await this.addonsRepo.listAllOrdered()) // Hidden rather than shown-and-refused, because a toggle that answers 403 // is worse than no toggle. // @@ -561,38 +569,12 @@ export class AdminService { a.id === ADDON_IDS.DAWARICH) ), ); - const providers = this.db.all<{ - id: string; - name: string; - description?: string | null; - icon: string; - enabled: number; - sort_order: number; - }>(` - SELECT id, name, description, icon, enabled, sort_order - FROM photo_providers - ORDER BY sort_order, id - `) + const providers = (await this.photoProviders.listAllOrdered()) // Immich and Synology Photos are servers the admin runs at home. A managed // instance cannot reach one (its egress does not go there, and it should // not), so offering the connection would only produce a timeout. .filter(() => !readEnv().managed.enabled); - const fields = this.db.all<{ - provider_id: string; - field_key: string; - label: string; - input_type: string; - placeholder?: string | null; - required: number; - secret: number; - settings_key?: string | null; - payload_key?: string | null; - sort_order: number; - }>(` - SELECT provider_id, field_key, label, input_type, placeholder, required, secret, settings_key, payload_key, sort_order - FROM photo_provider_fields - ORDER BY sort_order, id - `); + const fields = await this.photoProviderFields.listAllOrderedForAdminShelf(); const fieldsByProvider = new Map(); for (const field of fields) { const arr = fieldsByProvider.get(field.provider_id) || []; @@ -609,18 +591,8 @@ export class AdminService { // not belong to a user here but to a trip, so they are entered in the trip // rather than in settings. The admin decides only whether a provider may be // offered at all. - const docProviders = this.db.all<{ - id: string; - name: string; - description?: string | null; - icon: string; - enabled: number; - sort_order: number; - }>(` - SELECT id, name, description, icon, enabled, sort_order - FROM document_providers - ORDER BY sort_order, id - `).filter(() => !readEnv().managed.enabled); + const docProviders = (await this.documentProviders.listAllOrdered()) + .filter(() => !readEnv().managed.enabled); return [ ...addons.map((a) => ({ @@ -628,8 +600,8 @@ export class AdminService { enabled: !!a.enabled, config: a.id === ADDON_IDS.LLM_PARSING - ? maskLlmAddonConfig(JSON.parse(a.config || '{}')) - : JSON.parse(a.config || '{}'), + ? maskLlmAddonConfig(a.config ?? {}) + : (a.config ?? {}), })), ...providers.map((p) => ({ id: p.id, @@ -638,8 +610,8 @@ export class AdminService { type: 'photo_provider', icon: p.icon, enabled: !!p.enabled, - config: getPhotoProviderConfig(p.id), - fields: (fieldsByProvider.get(p.id) || []).map((f) => ({ + config: getPhotoProviderConfig(p.id!), + fields: (fieldsByProvider.get(p.id!) || []).map((f) => ({ key: f.field_key, label: f.label, input_type: f.input_type, @@ -667,10 +639,9 @@ export class AdminService { } async updateAddon(id: string, data: { enabled?: boolean; config?: Record }) { - type ProviderRow = { id: string; name: string; description?: string | null; icon: string; enabled: number; sort_order: number }; - const addon = this.db.get('SELECT * FROM addons WHERE id = ?', id); - const provider = this.db.get('SELECT * FROM photo_providers WHERE id = ?', id); - const docProvider = this.db.get('SELECT * FROM document_providers WHERE id = ?', id); + const addon = await this.addonsRepo.findById(id); + const provider = await this.photoProviders.findById(id); + const docProvider = await this.documentProviders.findById(id); if (!addon && !provider && !docProvider) return { error: 'Addon not found', status: 404 }; // The whole addon, not just its config: on a centrally administered install @@ -696,46 +667,46 @@ export class AdminService { } await this.uow.transactional(async () => { - if (addon) { - if (data.enabled !== undefined) { - this.db.run('UPDATE addons SET enabled = ? WHERE id = ?', data.enabled ? 1 : 0, id); - // Journey off takes its providers with it: a row left enabled would - // resurface the moment journey returns, which nobody switched on. - if (id === ADDON_IDS.JOURNEY && !data.enabled) - this.db.run('UPDATE photo_providers SET enabled = 0'); - // Documents off takes its providers with it, for the reason above: a - // row left enabled would resurface the moment the addon returns. - if (id === ADDON_IDS.DOCUMENTS && !data.enabled) - this.db.run('UPDATE document_providers SET enabled = 0'); + if (addon) { + if (data.enabled !== undefined) { + await this.addonsRepo.setEnabled(id, !!data.enabled); + // Journey off takes its providers with it: a row left enabled would + // resurface the moment journey returns, which nobody switched on. + if (id === ADDON_IDS.JOURNEY && !data.enabled) + await this.photoProviders.disableAll(); + // Documents off takes its providers with it, for the reason above: a + // row left enabled would resurface the moment the addon returns. + if (id === ADDON_IDS.DOCUMENTS && !data.enabled) + await this.documentProviders.disableAll(); + } + if (data.config !== undefined) { + // The AI-parsing addon holds an API key — encrypt it at rest and preserve + // the stored key when the client echoes the mask sentinel (see llmConfig.ts). + const configToStore = + id === ADDON_IDS.LLM_PARSING + ? prepareLlmAddonConfigForWrite(data.config, addon.config ?? {}) + : data.config; + await this.addonsRepo.setConfig(id, configToStore as AddonConfig); + } + } else if (provider) { + if (data.enabled !== undefined) + await this.photoProviders.setEnabled(id, data.enabled ? 1 : 0); + } else { + if (data.enabled !== undefined) + await this.documentProviders.setEnabled(id, data.enabled ? 1 : 0); } - if (data.config !== undefined) { - // The AI-parsing addon holds an API key — encrypt it at rest and preserve - // the stored key when the client echoes the mask sentinel (see llmConfig.ts). - const configToStore = - id === ADDON_IDS.LLM_PARSING - ? prepareLlmAddonConfigForWrite(data.config, JSON.parse(addon.config || '{}')) - : data.config; - this.db.run('UPDATE addons SET config = ? WHERE id = ?', JSON.stringify(configToStore), id); - } - } else if (provider) { - if (data.enabled !== undefined) - this.db.run('UPDATE photo_providers SET enabled = ? WHERE id = ?', data.enabled ? 1 : 0, id); - } else { - if (data.enabled !== undefined) - this.db.run('UPDATE document_providers SET enabled = ? WHERE id = ?', data.enabled ? 1 : 0, id); - } }); - const updatedAddon = this.db.get('SELECT * FROM addons WHERE id = ?', id); - const updatedProvider = this.db.get('SELECT * FROM photo_providers WHERE id = ?', id); + const updatedAddon = await this.addonsRepo.findById(id); + const updatedProvider = await this.photoProviders.findById(id); const updated = updatedAddon ? { ...updatedAddon, enabled: !!updatedAddon.enabled, config: updatedAddon.id === ADDON_IDS.LLM_PARSING - ? maskLlmAddonConfig(JSON.parse(updatedAddon.config || '{}')) - : JSON.parse(updatedAddon.config || '{}'), + ? maskLlmAddonConfig(updatedAddon.config ?? {}) + : (updatedAddon.config ?? {}), } : updatedProvider ? { @@ -745,7 +716,7 @@ export class AdminService { type: 'photo_provider', icon: updatedProvider.icon, enabled: !!updatedProvider.enabled, - config: getPhotoProviderConfig(updatedProvider.id), + config: getPhotoProviderConfig(updatedProvider.id!), sort_order: updatedProvider.sort_order, } : null; @@ -761,7 +732,11 @@ export class AdminService { // after an admin switched them off, while the REST half answered 404 for the // same user in the same moment. const MCP_RELEVANT_ADDONS = new Set(MCP_GATED_ADDON_IDS); - const enabledChanged = !!addon && data.enabled !== undefined && (data.enabled ? 1 : 0) !== addon.enabled; + // `addon.enabled` is a JS boolean now (the repository's `AddonRow.enabled` + // shape, not the raw stored int the legacy `SELECT *` returned) — comparing + // two booleans for a real flip is the same predicate the legacy int + // comparison (`(data.enabled ? 1 : 0) !== addon.enabled`) expressed. + const enabledChanged = !!addon && data.enabled !== undefined && !!data.enabled !== addon.enabled; return { addon: updated, diff --git a/server/tests/e2e/admin.e2e.test.ts b/server/tests/e2e/admin.e2e.test.ts index 47a3abf72..1dd787d6b 100644 --- a/server/tests/e2e/admin.e2e.test.ts +++ b/server/tests/e2e/admin.e2e.test.ts @@ -22,12 +22,25 @@ const { db } = vi.hoisted(() => { const tmp = new Database(':memory:'); tmp.exec('PRAGMA journal_mode = WAL'); // `users` carries the columns listUsers/createUser/updateUser select, plus the - // is_guest flag the #1362 COALESCE guards read. + // is_guest flag the #1362 COALESCE guards read. Plan 3i Task 1 (admin's + // repository conversion) surfaced the SAME class of drift the + // `invite_tokens`/`used_count` comment below already documents: `UsersRepository + // .insertAdminCreatedUser` writes through entity metadata (MikroORM's native + // insert), which — unlike the legacy raw `INSERT INTO users (username, email, + // password_hash, role) VALUES (...)` — also applies every OTHER column's + // class-level JS default (`mfa_enabled = 0`, `first_seen_version = '0.0.0'`, …), + // even though this repository's own method never names them. Fixed at the + // source (the fixture, adding the columns the real migrated schema has), not + // worked around in the repository. 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, password_hash TEXT, avatar TEXT, is_guest INTEGER DEFAULT 0, created_at DATETIME DEFAULT CURRENT_TIMESTAMP, updated_at DATETIME DEFAULT CURRENT_TIMESTAMP, - last_login DATETIME);`); + last_login DATETIME, mfa_enabled INTEGER DEFAULT 0, mfa_secret TEXT, mfa_backup_codes TEXT, + must_change_password INTEGER DEFAULT 0, synology_skip_ssl INTEGER DEFAULT 0, + first_seen_version TEXT DEFAULT '0.0.0', login_count INTEGER DEFAULT 0, + immich_auto_upload INTEGER DEFAULT 0, airtrail_allow_insecure_tls INTEGER DEFAULT 0, + airtrail_write_enabled INTEGER DEFAULT 0);`); 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);'); diff --git a/server/tests/unit/db/repositories/addons-config-encryption.test.ts b/server/tests/unit/db/repositories/addons-config-encryption.test.ts new file mode 100644 index 000000000..cb4e55e6d --- /dev/null +++ b/server/tests/unit/db/repositories/addons-config-encryption.test.ts @@ -0,0 +1,75 @@ +/** + * AD36 (`admin.service.ts#updateAddon`, the LLM-parsing addon's encrypted + * API key) — Plan 3i Task 1's encryption-boundary proof. Deliberately does + * NOT mock `apiKeyCrypto` (unlike `admin.service.test.ts`, which stubs it + * to identity functions) — this file exercises the REAL + * `maybe_encrypt_api_key`/`decrypt_api_key` so the round trip is genuine, + * not a no-op through a mock. `ENCRYPTION_KEY` comes from + * `tests/global-setup.ts`. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { Addons } from '../../../../src/db/entities/Addons.entity'; +import type { AddonsRepository } from '../../../../src/db/repositories/Addons.repository'; +import { prepareLlmAddonConfigForWrite } from '../../../../src/nest/llm-parse/llm-config'; +import { decrypt_api_key } from '../../../../src/nest/common/crypto/apiKeyCrypto'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let addons: AddonsRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + addons = t.repo(Addons); +}); +beforeEach(() => { + resetTestDb(testDb); + testDb.exec('DELETE FROM addons'); + testDb + .prepare(`INSERT INTO addons (id, name, description, type, icon, enabled, sort_order) VALUES ('llm_parsing', 'AI Parsing', NULL, 'global', 'Sparkles', 0, 0)`) + .run(); + t.clear(); +}); +afterAll(async () => { + await t.close(); + testDb.close(); +}); + +describe('AddonsRepository.setConfig — AD36 encryption boundary', () => { + it('ADDONSREPO-010: the stored apiKey is never the plaintext, but decrypt_api_key always round-trips it back', async () => { + const plaintext = 'sk-live-abc123-a-real-looking-secret'; + // Mirrors admin.service.ts#updateAddon's own call: the SERVICE encrypts + // before handing the repository a config object — the repository never + // calls maybe_encrypt_api_key/decrypt_api_key itself. + const configToStore = prepareLlmAddonConfigForWrite({ provider: 'openai', apiKey: plaintext }, undefined); + + await addons.setConfig('llm_parsing', configToStore as { apiKey?: string; [k: string]: unknown }); + + const row = await addons.findById('llm_parsing'); + const storedApiKey = (row?.config as { apiKey?: string } | null)?.apiKey; + + expect(storedApiKey).toBeDefined(); + // Never assert a literal ciphertext string (the IV is random on every + // encrypt call, so the exact bytes are non-deterministic) — only that it + // differs from the plaintext and that the real decrypt function recovers it. + expect(storedApiKey).not.toBe(plaintext); + expect(decrypt_api_key(storedApiKey)).toBe(plaintext); + }); + + it('ADDONSREPO-011: a second write of the SAME plaintext produces a different stored ciphertext (random IV), both still decrypting to the same plaintext', async () => { + const plaintext = 'sk-live-same-secret-both-times'; + const first = prepareLlmAddonConfigForWrite({ apiKey: plaintext }, undefined); + await addons.setConfig('llm_parsing', first as { apiKey?: string; [k: string]: unknown }); + const firstStored = ((await addons.findById('llm_parsing'))?.config as { apiKey?: string } | null)?.apiKey; + + const second = prepareLlmAddonConfigForWrite({ apiKey: plaintext }, undefined); + await addons.setConfig('llm_parsing', second as { apiKey?: string; [k: string]: unknown }); + const secondStored = ((await addons.findById('llm_parsing'))?.config as { apiKey?: string } | null)?.apiKey; + + expect(firstStored).not.toBe(secondStored); + expect(decrypt_api_key(firstStored)).toBe(plaintext); + expect(decrypt_api_key(secondStored)).toBe(plaintext); + }); +}); diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index bc12195f0..92468d718 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -95,7 +95,17 @@ import { createTestTripsRepo, createTestTripMembersRepo, createTestSettingsRepo, + createTestPlacesRepo, + sharedTestOrm, } from '../../helpers/test-uow'; +import { AuditLog } from '../../../src/db/entities/AuditLog.entity'; +import type { AuditLogRepository } from '../../../src/db/repositories/AuditLog.repository'; +import { Addons } from '../../../src/db/entities/Addons.entity'; +import { PhotoProviders } from '../../../src/db/entities/PhotoProviders.entity'; +import { PhotoProviderFields } from '../../../src/db/entities/PhotoProviderFields.entity'; +import { DocumentProviders } from '../../../src/db/entities/DocumentProviders.entity'; +import { TripFiles } from '../../../src/db/entities/TripFiles.entity'; +import type { McpTokensRepository } from '../../../src/db/repositories/McpTokens.repository'; import { budgetRepoArgs } from '../../helpers/budget-repos'; import { createTestShareTokensRepo } from '../../helpers/share-repos'; import { createTestBudgetItemsRepo } from '../../helpers/files-repos'; @@ -116,6 +126,8 @@ let permissions: PermissionsService; let userCleanup: UserCleanupService; let auth: AuthService; let svc: AdminService; +let mcpTokensRepo: McpTokensRepository; +let auditLogRepo: AuditLogRepository; beforeAll(async () => { webauthn = new WebauthnConfigService(await createTestAppSettingsRepo(dbs.connection)); permissions = new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)); @@ -125,8 +137,22 @@ beforeAll(async () => { await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection), await createTestInviteTokensRepo(dbs.connection), await createTestMcpTokensRepo(dbs.connection), await createTestOauthTokensRepo(dbs.connection), await createTestWebauthnCredentialsRepo(dbs.connection), await createTestPasswordResetTokensRepo(dbs.connection), ); + const t = await sharedTestOrm(testDb); + mcpTokensRepo = await createTestMcpTokensRepo(dbs.connection); + auditLogRepo = t.repo(AuditLog); svc = new AdminService( - dbs, + await createTestUsersRepo(dbs.connection), + auditLogRepo, + await createTestAppSettingsRepo(dbs.connection), + t.repo(Addons), + t.repo(PhotoProviders), + t.repo(PhotoProviderFields), + t.repo(DocumentProviders), + mcpTokensRepo, + await createTestOauthTokensRepo(dbs.connection), + await createTestTripsRepo(dbs.connection), + await createTestPlacesRepo(dbs.connection), + t.repo(TripFiles), await createTestAddonsService(testDb, dbs), new PasskeyService(auth, webauthn, await createTestUnitOfWork(dbs.connection), await createTestWebauthnCredentialsRepo(dbs.connection), await createTestWebauthnChallengesRepo(dbs.connection), await createTestUsersRepo(dbs.connection)), auth, @@ -738,3 +764,112 @@ describe('checkVersion on a centrally administered install', () => { }); }); }); + +// ── Plan 3i Task 1 — R4's guard-location + TX-boundary + AD22 parity proofs ── + +describe('updateUser — last-admin guard (AD9/AD10, R4)', () => { + it('ADMIN-SVC-090 — refuses to demote the sole remaining admin, row untouched', async () => { + const { user: soleAdmin } = createAdmin(testDb); + + const result = (await updateUser(String(soleAdmin.id), { role: 'user' })) as { error?: string; status?: number }; + + expect(result).toEqual({ error: 'Cannot remove the last admin', status: 400 }); + const row = testDb.prepare('SELECT role FROM users WHERE id = ?').get(soleAdmin.id) as { role: string }; + expect(row.role).toBe('admin'); + }); + + it('ADMIN-SVC-091 — demoting one of SEVERAL admins is allowed', async () => { + createAdmin(testDb); + const { user: secondAdmin } = createAdmin(testDb); + + const result = (await updateUser(String(secondAdmin.id), { role: 'user' })) as { user?: { role: string }; error?: string }; + + expect(result.error).toBeUndefined(); + const row = testDb.prepare('SELECT role FROM users WHERE id = ?').get(secondAdmin.id) as { role: string }; + expect(row.role).toBe('user'); + }); +}); + +describe('getAuditLog — AD22 parity through the service (LEFT JOIN survives a deleted user, rule 16)', () => { + it('ADMIN-SVC-092 — a deleted user\'s audit row keeps its row (not dropped) with username/user_email both null, matching the legacy raw LEFT JOIN', async () => { + const { user: liveUser } = createUser(testDb); + const { user: doomedUser } = createUser(testDb); + + testDb.prepare('INSERT INTO audit_log (user_id, action, resource, details, ip) VALUES (?, ?, ?, ?, ?)') + .run(liveUser.id, 'live_user_action', 'trip', null, '127.0.0.1'); + testDb.prepare('INSERT INTO audit_log (user_id, action, resource, details, ip) VALUES (?, ?, ?, ?, ?)') + .run(doomedUser.id, 'about_to_be_deleted_action', 'trip', null, '127.0.0.1'); + + // audit_log.user_id is ON DELETE SET NULL (Task 0's report) — deleting the + // user directly (not through the service) makes the row's user_id + // genuinely NULL, the exact LEFT JOIN shape rule 16 warns about. + testDb.prepare('DELETE FROM users WHERE id = ?').run(doomedUser.id); + + // The legacy statement, run raw on the SAME seeded rows — the parity anchor. + const legacyRows = testDb + .prepare( + `SELECT a.id, a.created_at, a.user_id, u.username, u.email as user_email, a.action, a.resource, a.details, a.ip + FROM audit_log a + LEFT JOIN users u ON u.id = a.user_id + ORDER BY a.id DESC + LIMIT ? OFFSET ?`, + ) + .all(500, 0) as Array<{ + id: number; created_at: string; user_id: number | null; username: string | null; + user_email: string | null; action: string; resource: string | null; details: string | null; ip: string | null; + }>; + + const result = (await getAuditLog({ limit: '500', offset: '0' })) as { + entries: Array<{ user_id: number | null; username: string | null; user_email: string | null; action: string }>; + total: number; + }; + + expect(result.entries.length).toBe(legacyRows.length); + + const deletedEntry = result.entries.find((e) => e.action === 'about_to_be_deleted_action'); + const legacyDeletedRow = legacyRows.find((r) => r.action === 'about_to_be_deleted_action'); + expect(deletedEntry).toBeDefined(); + expect(legacyDeletedRow).toBeDefined(); + // The row survives the LEFT JOIN (never dropped): user_id is preserved, + // username/user_email both null — never `undefined` (rule 16: T | null). + expect(deletedEntry!.user_id).toBe(legacyDeletedRow!.user_id); + expect(deletedEntry!.username).toBeNull(); + expect(deletedEntry!.user_email).toBeNull(); + expect(legacyDeletedRow!.username).toBeNull(); + expect(legacyDeletedRow!.user_email).toBeNull(); + + const liveEntry = result.entries.find((e) => e.action === 'live_user_action'); + const legacyLiveRow = legacyRows.find((r) => r.action === 'live_user_action'); + expect(liveEntry!.username).toBe(legacyLiveRow!.username); + expect(liveEntry!.user_email).toBe(legacyLiveRow!.user_email); + }); +}); + +describe('updateUser — password-reset transaction boundary (AD11/12/13)', () => { + it('ADMIN-SVC-093 — a failure on the mcp_tokens delete (AD12, not try/caught) rolls back the already-run users UPDATE (AD11) too', async () => { + const { user } = createUser(testDb); + const before = testDb + .prepare('SELECT username, password_version FROM users WHERE id = ?') + .get(user.id) as { username: string; password_version: number }; + + const spy = vi.spyOn(mcpTokensRepo, 'deleteAllForUser').mockRejectedValueOnce(new Error('simulated mcp_tokens failure')); + + await expect( + updateUser(String(user.id), { username: 'should-roll-back', password: 'ANewStrongPass123!' }), + ).rejects.toThrow('simulated mcp_tokens failure'); + + const after = testDb + .prepare('SELECT username, password_version FROM users WHERE id = ?') + .get(user.id) as { username: string; password_version: number }; + + // The users UPDATE (AD11) ran FIRST, inside the SAME uow.transactional + // boundary as the failing mcp_tokens delete — proves the TX boundary + // survived conversion (not just that each statement individually works): + // an uncaught failure anywhere in the block rolls the whole thing back, + // including a statement that already "succeeded" earlier in the same TX. + expect(after.username).toBe(before.username); + expect(after.password_version).toBe(before.password_version); + + spy.mockRestore(); + }); +}); diff --git a/server/tests/unit/nest/admin.version-notification.test.ts b/server/tests/unit/nest/admin.version-notification.test.ts index 80dd93c8d..c282dc9ef 100644 --- a/server/tests/unit/nest/admin.version-notification.test.ts +++ b/server/tests/unit/nest/admin.version-notification.test.ts @@ -73,7 +73,15 @@ import { createTestTripsRepo, createTestTripMembersRepo, createTestSettingsRepo, + createTestPlacesRepo, + sharedTestOrm, } from '../../helpers/test-uow'; +import { AuditLog } from '../../../src/db/entities/AuditLog.entity'; +import { Addons } from '../../../src/db/entities/Addons.entity'; +import { PhotoProviders } from '../../../src/db/entities/PhotoProviders.entity'; +import { PhotoProviderFields } from '../../../src/db/entities/PhotoProviderFields.entity'; +import { DocumentProviders } from '../../../src/db/entities/DocumentProviders.entity'; +import { TripFiles } from '../../../src/db/entities/TripFiles.entity'; import { budgetRepoArgs } from '../../helpers/budget-repos'; import { createTestShareTokensRepo } from '../../helpers/share-repos'; import { createTestBudgetItemsRepo } from '../../helpers/files-repos'; @@ -103,8 +111,20 @@ beforeAll(async () => { await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection), await createTestInviteTokensRepo(dbs.connection), await createTestMcpTokensRepo(dbs.connection), await createTestOauthTokensRepo(dbs.connection), await createTestWebauthnCredentialsRepo(dbs.connection), await createTestPasswordResetTokensRepo(dbs.connection), ); + const t = await sharedTestOrm(testDb); svc = new AdminService( - dbs, + await createTestUsersRepo(dbs.connection), + t.repo(AuditLog), + await createTestAppSettingsRepo(dbs.connection), + t.repo(Addons), + t.repo(PhotoProviders), + t.repo(PhotoProviderFields), + t.repo(DocumentProviders), + await createTestMcpTokensRepo(dbs.connection), + await createTestOauthTokensRepo(dbs.connection), + await createTestTripsRepo(dbs.connection), + await createTestPlacesRepo(dbs.connection), + t.repo(TripFiles), await createTestAddonsService(testDb, dbs), new PasskeyService(auth, webauthn, await createTestUnitOfWork(dbs.connection), await createTestWebauthnCredentialsRepo(dbs.connection), await createTestWebauthnChallengesRepo(dbs.connection), await createTestUsersRepo(dbs.connection)), auth,