diff --git a/server/src/db/dialect/sql-functions.ts b/server/src/db/dialect/sql-functions.ts index 145f26e2c..7a71cee90 100644 --- a/server/src/db/dialect/sql-functions.ts +++ b/server/src/db/dialect/sql-functions.ts @@ -74,3 +74,28 @@ export function columnIncrementedBy(platform: Platform, ref: string, amount: num } return unsupported(platform); } + +/** + * A case-insensitive column reference, for use as a filter's object key: + * `{ [lower(platform, 'email')]: value.toLowerCase() }` produces + * `WHERE LOWER(email) = ?`. This is the parity-preserving equivalent of the + * legacy `LOWER(col) = LOWER(?)` sites (`AuthService`/`OidcService`/ + * `UserProfileService`'s username/email CI lookups): the caller lowercases + * the bound value itself (every one of this migration's call sites already + * does, via `String.prototype.toLowerCase`), so the comparison is between + * two already-lowercased values either way — SQLite's `LOWER()` is ASCII-only + * exactly like `toLowerCase()` is for the identifier characters these + * columns hold (usernames/emails), so the two spellings agree on every + * input this codebase stores. + * + * Typed `RawQueryFragment & symbol` (not the bare `RawQueryFragment` the + * other helpers in this file return): TypeScript only accepts a + * `string | number | symbol` as a computed property name, and a raw + * fragment is only usable as an object key because `raw()`'s default + * generic already brands it with a `[Symbol.toPrimitive]`. The other + * helpers here are only ever used as VALUES, where that brand isn't needed. + */ +export function lower(platform: Platform, ref: string): RawQueryFragment & symbol { + if (platform instanceof SqlitePlatform) return raw(`LOWER(${column(ref)})`); + return unsupported(platform); +} diff --git a/server/src/db/repositories/InviteTokens.repository.ts b/server/src/db/repositories/InviteTokens.repository.ts index 16ac0e5dc..b92462798 100644 --- a/server/src/db/repositories/InviteTokens.repository.ts +++ b/server/src/db/repositories/InviteTokens.repository.ts @@ -37,12 +37,20 @@ export class InviteTokensRepository extends EntityRepository { /** * `SELECT * FROM invite_tokens WHERE token = ?` * + * Named `findByToken`, not `findValid` (Task 0 review addendum, LOW item + * 1): the statement has no validity filter of its own — no `expires_at`, + * no `used_count < max_uses` in the WHERE — it is a plain lookup by the + * unique `token` column, and validity (expiry, capacity) is checked in the + * caller's JS afterward, identically for `AuthService.registerUser` (AU8) + * and `OidcService.findOrCreateUser` (O11). A name promising "valid" would + * describe a filter this statement doesn't have. + * * Not a primary-key filter — `token` is a unique column, not `id` — so this * always re-queries; no `{ refresh: true }` needed (Task 3a review's * precision ruling: the identity-map short-circuit only fires for a * PK-only filter). */ - async findValid(token: string): Promise { + async findByToken(token: string): Promise { const invite = await this.findOne({ token }); return invite ? (toRow(invite) as InviteTokenRow) : null; } diff --git a/server/src/db/repositories/Users.repository.ts b/server/src/db/repositories/Users.repository.ts index 9e0e67dcf..859599f61 100644 --- a/server/src/db/repositories/Users.repository.ts +++ b/server/src/db/repositories/Users.repository.ts @@ -1,4 +1,6 @@ import type { Users } from '../entities/Users.entity'; +import { toRow, type AssertRowKeys } from './_shared/rows'; +import { columnIncrementedBy, currentTimestamp, lower } from '../dialect/sql-functions'; import { EntityRepository } from '@mikro-orm/sql'; /** @@ -13,6 +15,166 @@ import { EntityRepository } from '@mikro-orm/sql'; */ export type InstanceApiKeyName = 'maps_api_key' | 'unsplash_api_key' | 'amap_api_key'; +/** + * A full `users` row (`SELECT * FROM users WHERE ...`), as the API emits it. + * Every scalar column of the entity, hand-nullability-annotated the way + * `AppSettingsRepository`/`CategoriesRepository`'s row interfaces are — + * `AssertRowKeys` below fails `tsc` the moment this list and the entity's + * scalar columns drift. + */ +export interface UserRow { + id: number; + username: string; + email: string; + password_hash: string; + role: string; + maps_api_key: string | null; + unsplash_api_key: string | null; + amap_api_key: string | null; + openweather_api_key: string | null; + avatar: string | null; + oidc_sub: string | null; + oidc_issuer: string | null; + last_login: string | null; + mfa_enabled: number | null; + mfa_secret: string | null; + mfa_backup_codes: string | null; + immich_url: string | null; + immich_access_token: string | null; + synology_url: string | null; + synology_username: string | null; + synology_password: string | null; + synology_sid: string | null; + must_change_password: number | null; + password_version: number; + feed_token: string | null; + is_guest: number; + created_at: string | null; + updated_at: string | null; + immich_api_key: string | null; + synology_skip_ssl: number; + synology_did: string | null; + first_seen_version: string; + login_count: number; + immich_auto_upload: number; + airtrail_url: string | null; + airtrail_api_key: string | null; + airtrail_allow_insecure_tls: number | null; + airtrail_write_enabled: number | null; + display_name: string | null; +} + +const _userRowKeys: AssertRowKeys = true; + +/** `AuthService.registerUser` (AU10) / `OidcService.findOrCreateUser` (O14) — the two column sets `insertUser` covers as one method (Task 1 brief's Required set: "one method, nullable oidc/avatar fields"). */ +export interface NewUserRow { + username: string; + email: string; + password_hash: string; + role: string; + first_seen_version: string; + oidc_sub?: string | null; + oidc_issuer?: string | null; + avatar?: string | null; +} + +/** `UserProfileService.updateApiKeys` (UP3) — the four encrypted key columns, already resolved by the service. */ +export interface ApiKeysPatch { + maps_api_key: string | null; + openweather_api_key: string | null; + unsplash_api_key: string | null; + amap_api_key: string | null; +} + +/** `UserProfileService.updateSettings`'s bounded dynamic SET clause (UP7) — a closed key union, never a built SQL string. */ +export interface UserProfilePatch { + maps_api_key?: string | null; + openweather_api_key?: string | null; + unsplash_api_key?: string | null; + amap_api_key?: string | null; + username?: string; + email?: string; +} + +/** `UserProfileService.getApiKeyColumns`/`currentKeys` (UP1). */ +export interface UserApiKeyColumns { + role: string; + maps_api_key: string | null; + openweather_api_key: string | null; + unsplash_api_key: string | null; + amap_api_key: string | null; +} + +/** `UserProfileService.updateApiKeys`/`updateSettings`'s re-select (UP4/UP8). */ +export interface UserProfileWithKeys { + id: number; + username: string; + email: string; + role: string; + maps_api_key: string | null; + openweather_api_key: string | null; + unsplash_api_key: string | null; + amap_api_key: string | null; + avatar: string | null; + mfa_enabled: number | null; +} + +/** `UserProfileService.saveAvatar`'s re-select (UP11). */ +export interface UserProfileBasic { + id: number; + username: string; + email: string; + role: string; + avatar: string | null; +} + +/** `UserProfileService.listUsers` (UP14). */ +export interface UserDirectoryRow { + id: number; + username: string; + avatar: string | null; +} + +/** `AuthService.getCurrentUser` (AU14). */ +export interface CurrentUserRow { + id: number; + username: string; + email: string; + role: string; + avatar: string | null; + oidc_issuer: string | null; + created_at: string | null; + mfa_enabled: number | null; + must_change_password: number | null; +} + +/** `jwt-verify.ts#verifyJwtAndLoadUser` (JV1). */ +export interface UserWithPasswordVersion { + id: number; + username: string; + email: string; + role: string; + password_version: number; +} + +/** `AuthService.requestPasswordReset` (AU36). */ +export interface PasswordResetLookup { + id: number; + email: string; + password_hash: string; + oidc_sub: string | null; +} + +/** `AuthService.resetPassword`'s reset-target read (AU40). */ +export interface ResetTargetRow { + id: number; + email: string; + mfa_enabled: number | null; + mfa_secret: string | null; + mfa_backup_codes: string | null; + password_version: number; +} + export class UsersRepository extends EntityRepository { /** * `SELECT email FROM users WHERE id = ?` @@ -42,4 +204,611 @@ export class UsersRepository extends EntityRepository { const row = await this.findOne({ id: userId }, { fields: [name], refresh: true }); return row ? (row[name] ?? null) : null; } + + // --------------------------------------------------------------------- + // JV1 — jwt-verify.ts + // --------------------------------------------------------------------- + + /** + * `SELECT id, username, email, role, password_version FROM users WHERE id = ?` + * + * The canonical session-invalidation read: `verifyJwtAndLoadUser` compares + * the returned `password_version` against the JWT's `pv` claim on every + * request. PK-only `findOne` → `refresh: true` (I1). + */ + async findByIdWithPasswordVersion(id: number): Promise { + const row = await this.findOne( + { id }, + { fields: ['id', 'username', 'email', 'role', 'password_version'], refresh: true }, + ); + return row ? { id: row.id, username: row.username, email: row.email, role: row.role, password_version: row.password_version } : null; + } + + // --------------------------------------------------------------------- + // AU2 / TK7 / O2 — password_version (session-invalidation gate) + // --------------------------------------------------------------------- + + /** `SELECT password_version FROM users WHERE id = ?`. `?? 0` stays in the caller (AU2/TK7/O2 each fall back differently in spirit, identically in value). */ + async getPasswordVersion(id: number): Promise { + const row = await this.findOne({ id }, { fields: ['password_version'], refresh: true }); + return row?.password_version ?? null; + } + + // --------------------------------------------------------------------- + // AU4 / AU7 / O10 — first-user / admin-role decisions + // --------------------------------------------------------------------- + + /** `SELECT COUNT(*) as count FROM users WHERE COALESCE(is_guest, 0) = 0`. `is_guest` is `INTEGER NOT NULL DEFAULT 0` (verified in the baseline + guest-members migrations) — no row can store NULL there, so `{ is_guest: 0 }` is exact parity for the `COALESCE(is_guest, 0) = 0` guard, not an approximation. */ + async countNonGuest(): Promise { + return this.count({ is_guest: 0 }); + } + + // --------------------------------------------------------------------- + // AU6 — setup_complete + // --------------------------------------------------------------------- + + /** `SELECT id FROM users WHERE role = 'admin' AND must_change_password = 1 LIMIT 1`. */ + async findAdminNeedingPasswordChange(): Promise { + const row = await this.findOne({ role: 'admin', must_change_password: 1 }, { fields: ['id'] }); + return row?.id ?? null; + } + + // --------------------------------------------------------------------- + // AU9 — registration collision check + // --------------------------------------------------------------------- + + /** + * `SELECT id FROM users WHERE (LOWER(email) = LOWER(?) OR LOWER(username) = LOWER(?)) + * AND COALESCE(is_guest, 0) = 0` + */ + async findIdByEmailOrUsernameCI(email: string, username: string): Promise { + const platform = this.getEntityManager().getPlatform(); + const row = await this.findOne( + { + $or: [{ [lower(platform, 'email')]: email.toLowerCase() }, { [lower(platform, 'username')]: username.toLowerCase() }], + is_guest: 0, + }, + { fields: ['id'] }, + ); + return row?.id ?? null; + } + + // --------------------------------------------------------------------- + // AU10 / O14 — account creation + // --------------------------------------------------------------------- + + /** + * `INSERT INTO users (username, email, password_hash, role, first_seen_version, login_count) + * VALUES (?, ?, ?, ?, ?, 0)` (AU10, local registration) and + * `INSERT INTO users (username, email, password_hash, role, oidc_sub, oidc_issuer, avatar, first_seen_version, login_count) + * VALUES (?, ?, ?, ?, ?, ?, ?, ?, 0)` (O14, SSO registration) — ONE method + * (Task 1 brief's Required set), the `oidc_sub`/`oidc_issuer`/`avatar` + * columns nullable so AU10's caller simply omits them. `login_count` is + * always the literal `0` on insert, never a parameter, matching both + * statements exactly. + * + * Followed by `refresh` (D4's create rule), not relied-on `RETURNING`: + * the returned row carries the generated `id` and the `defaultRaw` + * `created_at` the same way a legacy INSERT-then-reselect pair did. + */ + async insertUser(row: NewUserRow): Promise { + const user = this.create({ + username: row.username, + email: row.email, + password_hash: row.password_hash, + role: row.role, + first_seen_version: row.first_seen_version, + login_count: 0, + oidc_sub: row.oidc_sub ?? null, + oidc_issuer: row.oidc_issuer ?? null, + avatar: row.avatar ?? null, + }); + await this.getEntityManager().persist(user).flush(); + await this.getEntityManager().refresh(user); + return toRow(user) as UserRow; + } + + // --------------------------------------------------------------------- + // AU12 / O4 — email lookup (login + OIDC email-fallback) + // --------------------------------------------------------------------- + + /** `SELECT * FROM users WHERE LOWER(email) = LOWER(?) AND COALESCE(is_guest, 0) = 0` */ + async findByEmailCI(email: string): Promise { + const platform = this.getEntityManager().getPlatform(); + const user = await this.findOne({ [lower(platform, 'email')]: email.toLowerCase(), is_guest: 0 }); + return user ? (toRow(user) as UserRow) : null; + } + + // --------------------------------------------------------------------- + // AU13 / AU34 / AU35 / PK12 / O16 — login bookkeeping + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET last_login = CURRENT_TIMESTAMP, login_count = login_count + 1 + * WHERE id = ?` — ONE statement via `nativeUpdate` with a raw sibling-column + * increment (`columnIncrementedBy`), never a read-modify-write: two + * concurrent calls both read `login_count` from the row at commit time, not + * from a value fetched earlier, so both increments land (proven by + * `USERSREPO` `touchLastLogin` concurrency test — two concurrent calls → + * `+2`, never `+1`). + */ + async touchLastLogin(id: number): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate( + { id }, + { last_login: currentTimestamp(platform), login_count: columnIncrementedBy(platform, 'login_count', 1) }, + ); + } + + // --------------------------------------------------------------------- + // AU14 — /api/auth/me + // --------------------------------------------------------------------- + + /** `SELECT id, username, email, role, avatar, oidc_issuer, created_at, mfa_enabled, must_change_password FROM users WHERE id = ?` */ + async findMeRow(id: number): Promise { + const row = await this.findOne( + { id }, + { + fields: ['id', 'username', 'email', 'role', 'avatar', 'oidc_issuer', 'created_at', 'mfa_enabled', 'must_change_password'], + refresh: true, + }, + ); + return row + ? { + id: row.id, + username: row.username, + email: row.email, + role: row.role, + avatar: row.avatar ?? null, + oidc_issuer: row.oidc_issuer ?? null, + created_at: row.created_at ?? null, + mfa_enabled: row.mfa_enabled ?? null, + must_change_password: row.must_change_password ?? null, + } + : null; + } + + // --------------------------------------------------------------------- + // AU15 — current-password check + // --------------------------------------------------------------------- + + /** `SELECT password_hash, password_version FROM users WHERE id = ?` */ + async getPasswordHashAndVersion(id: number): Promise<{ password_hash: string; password_version: number } | null> { + const row = await this.findOne({ id }, { fields: ['password_hash', 'password_version'], refresh: true }); + return row ? { password_hash: row.password_hash, password_version: row.password_version } : null; + } + + // --------------------------------------------------------------------- + // AU16 / AU43 — password change / reset + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET password_hash = ?, must_change_password = 0, + * password_version = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?` + * (AU16 `changePassword`, AU43 `resetPassword` — identical statement). + */ + async setPassword(id: number, passwordHash: string, passwordVersion: number): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate( + { id }, + { password_hash: passwordHash, must_change_password: 0, password_version: passwordVersion, updated_at: currentTimestamp(platform) }, + ); + } + + // --------------------------------------------------------------------- + // AU19 / O7 — last-admin guard + // --------------------------------------------------------------------- + + /** `SELECT COUNT(*) as count FROM users WHERE role = 'admin'` */ + async countAdmins(): Promise { + return this.count({ role: 'admin' }); + } + + // --------------------------------------------------------------------- + // AU20 / AU22 — admin gate + // --------------------------------------------------------------------- + + /** `SELECT role FROM users WHERE id = ?` */ + async getRole(id: number): Promise { + const row = await this.findOne({ id }, { fields: ['role'], refresh: true }); + return row?.role ?? null; + } + + // --------------------------------------------------------------------- + // AU23 / AU27 / MP2 — MFA state + // --------------------------------------------------------------------- + + /** + * `SELECT mfa_enabled FROM users WHERE id = ?` + * + * Returns the ROW, not the bare column: `mfa-policy.guard.ts` (MP2) needs + * to tell "no row for this id" (`null` — the user vanished mid-request, + * `MfaPolicyGuard` lets the request through) apart from "row present with + * `mfa_enabled` unset" (`{ mfa_enabled: null }` — still governed by the + * policy) — the same distinction the legacy `if (!row) return true;` + * before `row.mfa_enabled === 1` made. A bare `T | null` return would + * collapse both into the same value. + */ + async getMfaEnabled(id: number): Promise<{ mfa_enabled: number | null } | null> { + const row = await this.findOne({ id }, { fields: ['mfa_enabled'], refresh: true }); + return row ? { mfa_enabled: row.mfa_enabled ?? null } : null; + } + + // --------------------------------------------------------------------- + // AU28 — enable MFA + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET mfa_enabled = 1, mfa_secret = ?, mfa_backup_codes = ?, + * updated_at = CURRENT_TIMESTAMP WHERE id = ?`. `secret`/`codes` arrive + * already encrypted/hashed — this repository never touches either. + */ + async enableMfa(id: number, encryptedSecret: string, backupCodes: string): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate( + { id }, + { mfa_enabled: 1, mfa_secret: encryptedSecret, mfa_backup_codes: backupCodes, updated_at: currentTimestamp(platform) }, + ); + } + + // --------------------------------------------------------------------- + // AU30 / AU32 / PK4 / PK10 / O15 — full-row reads + // --------------------------------------------------------------------- + + /** + * `SELECT * FROM users WHERE id = ?` — the full row, reused by every + * caller that needs more than a handful of columns (AU30 `disableMfa`, + * AU32 `verifyMfaLogin`, PK4 `passkeyRegisterOptions`, PK10 + * `passkeyLoginVerify`, O15 `findOrCreateUser`'s post-transaction re-select). + * PK-only `findOne` → `refresh: true` (I1). + */ + async findById(id: number): Promise { + const user = await this.findOne({ id }, { refresh: true }); + return user ? (toRow(user) as UserRow) : null; + } + + // --------------------------------------------------------------------- + // AU31 — disable MFA + // --------------------------------------------------------------------- + + /** `UPDATE users SET mfa_enabled = 0, mfa_secret = NULL, mfa_backup_codes = NULL, updated_at = CURRENT_TIMESTAMP WHERE id = ?` */ + async disableMfa(id: number): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate( + { id }, + { mfa_enabled: 0, mfa_secret: null, mfa_backup_codes: null, updated_at: currentTimestamp(platform) }, + ); + } + + // --------------------------------------------------------------------- + // AU33 / AU44 — backup-code burn + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET mfa_backup_codes = ?, updated_at = CURRENT_TIMESTAMP + * WHERE id = ?` (AU33, `verifyMfaLogin`'s backup-code branch — inside the + * same transaction as `touchLastLogin`). + */ + async setBackupCodesAndTouch(id: number, backupCodes: string): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { mfa_backup_codes: backupCodes, updated_at: currentTimestamp(platform) }); + } + + /** + * `UPDATE users SET mfa_backup_codes = ? WHERE id = ?` (AU44, + * `resetPassword`'s conditional backup-code splice — no `updated_at`, + * unlike AU33's otherwise-identical statement; two methods, byte-exact + * parity per statement). + */ + async setBackupCodes(id: number, backupCodes: string): Promise { + await this.nativeUpdate({ id }, { mfa_backup_codes: backupCodes }); + } + + // --------------------------------------------------------------------- + // AU36 — password-reset request lookup + // --------------------------------------------------------------------- + + /** + * `SELECT id, email, password_hash, oidc_sub FROM users WHERE email = ? + * AND COALESCE(is_guest, 0) = 0` — case-SENSITIVE on `email`, unlike + * every other email lookup in this repository (no `LOWER()` in the legacy + * statement here); kept exactly as written, not "fixed" to match the + * others (parity is law). + */ + async findForPasswordReset(email: string): Promise { + const row = await this.findOne( + { email, is_guest: 0 }, + { fields: ['id', 'email', 'password_hash', 'oidc_sub'] }, + ); + return row ? { id: row.id, email: row.email, password_hash: row.password_hash, oidc_sub: row.oidc_sub ?? null } : null; + } + + // --------------------------------------------------------------------- + // AU40 — reset-target read + // --------------------------------------------------------------------- + + /** `SELECT id, email, mfa_enabled, mfa_secret, mfa_backup_codes, password_version FROM users WHERE id = ?` */ + async findResetTarget(id: number): Promise { + const row = await this.findOne( + { id }, + { fields: ['id', 'email', 'mfa_enabled', 'mfa_secret', 'mfa_backup_codes', 'password_version'], refresh: true }, + ); + return row + ? { + id: row.id, + email: row.email, + mfa_enabled: row.mfa_enabled ?? null, + mfa_secret: row.mfa_secret ?? null, + mfa_backup_codes: row.mfa_backup_codes ?? null, + password_version: row.password_version, + } + : null; + } + + // --------------------------------------------------------------------- + // PK15 — re-auth before passkey deletion + // --------------------------------------------------------------------- + + /** `SELECT password_hash FROM users WHERE id = ?` */ + async getPasswordHash(id: number): Promise { + const row = await this.findOne({ id }, { fields: ['password_hash'], refresh: true }); + return row?.password_hash ?? null; + } + + // --------------------------------------------------------------------- + // PK17 — admin passkey-reset target + // --------------------------------------------------------------------- + + /** `SELECT id, email FROM users WHERE id = ?` */ + async findIdAndEmail(id: number): Promise<{ id: number; email: string } | null> { + const row = await this.findOne({ id }, { fields: ['id', 'email'], refresh: true }); + return row ? { id: row.id, email: row.email } : null; + } + + // --------------------------------------------------------------------- + // O3 — OIDC primary identity match + // --------------------------------------------------------------------- + + /** `SELECT * FROM users WHERE oidc_sub = ? AND oidc_issuer = ?` */ + async findByOidcIdentity(sub: string, issuer: string): Promise { + const user = await this.findOne({ oidc_sub: sub, oidc_issuer: issuer }); + return user ? (toRow(user) as UserRow) : null; + } + + // --------------------------------------------------------------------- + // O5 / O6 — OIDC identity linking + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET oidc_sub = ?, oidc_issuer = ? WHERE id = ?` (O5, the + * email-matched/unlinked/verified-email branch; O6, the provider-switch + * branch — identical statement, two call sites, no `updated_at`). + */ + async linkOidcIdentity(id: number, sub: string, issuer: string): Promise { + await this.nativeUpdate({ id }, { oidc_sub: sub, oidc_issuer: issuer }); + } + + // --------------------------------------------------------------------- + // O8 — claims-based role change + // --------------------------------------------------------------------- + + /** `UPDATE users SET role = ? WHERE id = ?` — no `updated_at` (legacy statement). */ + async setRole(id: number, role: string): Promise { + await this.nativeUpdate({ id }, { role }); + } + + // --------------------------------------------------------------------- + // O9 — OIDC avatar sync + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET avatar = ? WHERE id = ?` — no `updated_at`, unlike + * `setAvatar` (UP10/UP13) below, which is the profile-upload path's + * otherwise-identical statement. Two methods, byte-exact parity per + * statement (same rule as `setBackupCodes`/`setBackupCodesAndTouch`). + */ + async setAvatarRaw(id: number, avatar: string | null): Promise { + await this.nativeUpdate({ id }, { avatar }); + } + + // --------------------------------------------------------------------- + // O12 / UP5 — username collision checks + // --------------------------------------------------------------------- + + /** + * `SELECT id FROM users WHERE LOWER(username) = LOWER(?) AND id != ? + * AND COALESCE(is_guest, 0) = 0` (UP5, `updateSettings`'s own-username + * collision check). + */ + async findIdByUsernameCI(username: string, excludeId: number): Promise { + const platform = this.getEntityManager().getPlatform(); + const row = await this.findOne( + { [lower(platform, 'username')]: username.toLowerCase(), id: { $ne: excludeId }, is_guest: 0 }, + { fields: ['id'] }, + ); + return row?.id ?? null; + } + + /** + * `SELECT id FROM users WHERE LOWER(username) = LOWER(?)` (O12, OIDC + * derived-username collision avoidance — no `excludeId`, no guest filter: + * a genuinely different WHERE than UP5's, so a separate method per D4 + * rather than an optional parameter that would silently change UP5's + * shape. + */ + async findIdByUsernameCIAny(username: string): Promise { + const platform = this.getEntityManager().getPlatform(); + const row = await this.findOne({ [lower(platform, 'username')]: username.toLowerCase() }, { fields: ['id'] }); + return row?.id ?? null; + } + + // --------------------------------------------------------------------- + // UP1 — API-key columns read (`currentKeys`) + // --------------------------------------------------------------------- + + /** `SELECT role, maps_api_key, openweather_api_key, unsplash_api_key, amap_api_key FROM users WHERE id = ?` */ + async getApiKeyColumns(id: number): Promise { + const row = await this.findOne( + { id }, + { fields: ['role', 'maps_api_key', 'openweather_api_key', 'unsplash_api_key', 'amap_api_key'], refresh: true }, + ); + return row + ? { + role: row.role, + maps_api_key: row.maps_api_key ?? null, + openweather_api_key: row.openweather_api_key ?? null, + unsplash_api_key: row.unsplash_api_key ?? null, + amap_api_key: row.amap_api_key ?? null, + } + : null; + } + + // --------------------------------------------------------------------- + // UP2 — single-key (maps) update + // --------------------------------------------------------------------- + + /** `UPDATE users SET maps_api_key = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?` */ + async updateMapsKey(id: number, encryptedMapsApiKey: string | null): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { maps_api_key: encryptedMapsApiKey, updated_at: currentTimestamp(platform) }); + } + + // --------------------------------------------------------------------- + // UP3 — four-key update + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET maps_api_key = ?, openweather_api_key = ?, + * unsplash_api_key = ?, amap_api_key = ?, updated_at = CURRENT_TIMESTAMP + * WHERE id = ?`. The per-column "encrypt the new value or keep the + * current one" branching (a deleted-row race degrading to a 0-row UPDATE, + * not a crash) is the service's, per D4 — this writes exactly the four + * final values it is given. + */ + async updateApiKeys(id: number, keys: ApiKeysPatch): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { ...keys, updated_at: currentTimestamp(platform) }); + } + + // --------------------------------------------------------------------- + // UP4 / UP8 — profile-with-keys re-select + // --------------------------------------------------------------------- + + /** `SELECT id, username, email, role, maps_api_key, openweather_api_key, unsplash_api_key, amap_api_key, avatar, mfa_enabled FROM users WHERE id = ?` */ + async findProfileWithKeys(id: number): Promise { + const row = await this.findOne( + { id }, + { + fields: ['id', 'username', 'email', 'role', 'maps_api_key', 'openweather_api_key', 'unsplash_api_key', 'amap_api_key', 'avatar', 'mfa_enabled'], + refresh: true, + }, + ); + return row + ? { + id: row.id, + username: row.username, + email: row.email, + role: row.role, + maps_api_key: row.maps_api_key ?? null, + openweather_api_key: row.openweather_api_key ?? null, + unsplash_api_key: row.unsplash_api_key ?? null, + amap_api_key: row.amap_api_key ?? null, + avatar: row.avatar ?? null, + mfa_enabled: row.mfa_enabled ?? null, + } + : null; + } + + // --------------------------------------------------------------------- + // UP6 — email collision check + // --------------------------------------------------------------------- + + /** `SELECT id FROM users WHERE LOWER(email) = LOWER(?) AND id != ? AND COALESCE(is_guest, 0) = 0` */ + async findIdByEmailCI(email: string, excludeId: number): Promise { + const platform = this.getEntityManager().getPlatform(); + const row = await this.findOne( + { [lower(platform, 'email')]: email.toLowerCase(), id: { $ne: excludeId }, is_guest: 0 }, + { fields: ['id'] }, + ); + return row?.id ?? null; + } + + // --------------------------------------------------------------------- + // UP7 — bounded dynamic-column profile patch + // --------------------------------------------------------------------- + + /** + * `` UPDATE users SET ${updates.join(', ')}, updated_at = CURRENT_TIMESTAMP + * WHERE id = ? `` — the SET clause is built at the CALL SITE from a fixed + * set of 6 possible literal column names, never here: `patchProfile` takes + * only the already-decided final columns as a typed partial and writes + * them in ONE `nativeUpdate` (never `assign`+`flush`, which would be a + * second statement shape for the identical intent) — the entity-API + * equivalent of the legacy's dynamic `SET` list, without ever building SQL + * text. `updated_at` is always stamped, matching the legacy: this method's + * only caller (`UserProfileService.updateSettings`) calls it only when at + * least one field changed (`updates.length > 0`), the same guard the + * legacy statement was built under. + */ + async patchProfile(id: number, changes: UserProfilePatch): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { ...changes, updated_at: currentTimestamp(platform) }); + } + + // --------------------------------------------------------------------- + // UP9 / UP12 — avatar read (before cleanup/replace) + // --------------------------------------------------------------------- + + /** `SELECT avatar FROM users WHERE id = ?` */ + async getAvatar(id: number): Promise { + const row = await this.findOne({ id }, { fields: ['avatar'], refresh: true }); + return row?.avatar ?? null; + } + + // --------------------------------------------------------------------- + // UP10 / UP13 — avatar write + // --------------------------------------------------------------------- + + /** + * `UPDATE users SET avatar = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?` + * (UP10 `saveAvatar` with a filename, UP13 `deleteAvatar` with `NULL` — + * same statement shape, `avatar` nullable either way). + */ + async setAvatar(id: number, avatar: string | null): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { avatar, updated_at: currentTimestamp(platform) }); + } + + // --------------------------------------------------------------------- + // UP11 — post-avatar-write re-select + // --------------------------------------------------------------------- + + /** `SELECT id, username, email, role, avatar FROM users WHERE id = ?` */ + async findProfileBasic(id: number): Promise { + const row = await this.findOne({ id }, { fields: ['id', 'username', 'email', 'role', 'avatar'], refresh: true }); + return row ? { id: row.id, username: row.username, email: row.email, role: row.role, avatar: row.avatar ?? null } : null; + } + + // --------------------------------------------------------------------- + // UP14 — user directory (trip member / contributor pickers) + // --------------------------------------------------------------------- + + /** `SELECT id, username, avatar FROM users WHERE id != ? AND COALESCE(is_guest, 0) = 0 ORDER BY username ASC` */ + async listOthersNonGuest(excludeId: number): Promise { + const rows = await this.find( + { id: { $ne: excludeId }, is_guest: 0 }, + { fields: ['id', 'username', 'avatar'], orderBy: { username: 'asc' } }, + ); + return rows.map((row) => ({ id: row.id, username: row.username, avatar: row.avatar ?? null })); + } + + // --------------------------------------------------------------------- + // UP15 — key-validation probe + // --------------------------------------------------------------------- + + /** `SELECT role, openweather_api_key FROM users WHERE id = ?` */ + async getRoleAndWeatherKey(id: number): Promise<{ role: string; openweather_api_key: string | null } | null> { + const row = await this.findOne({ id }, { fields: ['role', 'openweather_api_key'], refresh: true }); + return row ? { role: row.role, openweather_api_key: row.openweather_api_key ?? null } : null; + } } diff --git a/server/src/db/repositories/WebauthnCredentials.repository.ts b/server/src/db/repositories/WebauthnCredentials.repository.ts index 2c8a0bbd6..71768e484 100644 --- a/server/src/db/repositories/WebauthnCredentials.repository.ts +++ b/server/src/db/repositories/WebauthnCredentials.repository.ts @@ -1,4 +1,28 @@ import type { WebauthnCredentials } from '../entities/WebauthnCredentials.entity'; import { EntityRepository } from '@mikro-orm/sql'; -export class WebauthnCredentialsRepository extends EntityRepository {} +/** + * Plan 3b Task 1 reserves this class (empty stub since Plan 3a) and implements + * exactly the one method the AUTH cluster's Task 1 needs NOW: `mfa-policy.guard.ts` + * (MP3) and, cross-service, `AuthService.updateAppSettings` (AU24, Task 5) both + * read "does this user own at least one passkey" — a passkey satisfies the + * `require_mfa` policy the same way TOTP does. Task 3 (`PasskeyService`) + * extends this class with the rest of the WebAuthn ceremony state + * (`listByUser`, `create`, `findByCredentialId`, `updateCounterAndLastUsed`, + * `renameOwned`, `deleteOwned`, `deleteAllForUser`) — this method's name and + * shape are chosen so nothing here needs to change when that lands. + */ +export class WebauthnCredentialsRepository extends EntityRepository { + /** + * `SELECT 1 FROM webauthn_credentials WHERE user_id = ? LIMIT 1` (MP3/AU24 + * — identical statement, two callers). `count` with an implicit `LIMIT 1` + * via `findOne` mirrors the legacy existence probe without pulling a row; + * MikroORM's `count()` has no built-in short-circuit, so `findOne` with a + * narrow `fields` selection is the closer match to "does one exist" than + * `count() > 0`, which would still scan/aggregate every matching row. + */ + async hasAny(userId: number): Promise { + const row = await this.findOne({ user: userId }, { fields: ['id'] }); + return row !== null; + } +} diff --git a/server/src/nest/app.module.ts b/server/src/nest/app.module.ts index 172d14bbc..1108fc2ba 100644 --- a/server/src/nest/app.module.ts +++ b/server/src/nest/app.module.ts @@ -1,5 +1,8 @@ import { trekMcpAccessPolicy, trekMcpValidateAccess } from '../mcp/nest-mcp-policy'; import mikroOrmConfig from '../mikro-orm.config'; +import { AppSettings } from '../db/entities/AppSettings.entity'; +import { Users } from '../db/entities/Users.entity'; +import { WebauthnCredentials } from '../db/entities/WebauthnCredentials.entity'; import { McpModule } from '../nest-mcp'; import { AccommodationsModule } from './accommodations/accommodations.module'; import { AddonsModule } from './addons/addons.module'; @@ -155,6 +158,13 @@ import { APP_FILTER, APP_GUARD, APP_INTERCEPTOR, APP_PIPE } from '@nestjs/core'; // Not part of DatabaseModule: the e2e suites that compose `DatabaseModule` // without the ORM would fail on an EntityManager-dependent provider there. OrmModule, + // MfaPolicyGuard (Plan 3b Task 1) is registered directly below as this + // module's own APP_GUARD, never per-controller — so it is the one place + // that needs `forFeature` for its three repositories; `Users` is not + // otherwise global (JwtAuthGuard and friends inject `EntityManager` + // instead precisely to avoid needing this everywhere — see their own + // docstrings). + MikroOrmModule.forFeature([AppSettings, Users, WebauthnCredentials]), ], providers: [ // Default-deny: a route is authenticated unless it carries @Public() or diff --git a/server/src/nest/auth/auth.service.ts b/server/src/nest/auth/auth.service.ts index d83ca2a5d..c2cb19881 100644 --- a/server/src/nest/auth/auth.service.ts +++ b/server/src/nest/auth/auth.service.ts @@ -1097,6 +1097,6 @@ export class AuthService { * route) should go through. */ async verifyJwtToken(token: string): Promise { - return verifyJwtAndLoadUser(token); + return verifyJwtAndLoadUser(token, this.usersRepo); } } diff --git a/server/src/nest/auth/cookie-auth.guard.ts b/server/src/nest/auth/cookie-auth.guard.ts index 5867ae08c..b596c92f7 100644 --- a/server/src/nest/auth/cookie-auth.guard.ts +++ b/server/src/nest/auth/cookie-auth.guard.ts @@ -1,5 +1,7 @@ import { CanActivate, ExecutionContext, HttpException, Injectable } from '@nestjs/common'; +import { EntityManager } from '@mikro-orm/core'; import type { Request } from 'express'; +import { Users } from '../../db/entities/Users.entity'; import { verifyJwtAndLoadUser } from './jwt-verify'; /** @@ -7,16 +9,23 @@ import { verifyJwtAndLoadUser } from './jwt-verify'; * trek_session cookie (never a Bearer token), so CSRF-sensitive state-changing * OAuth endpoints (consent submit, client/session mutations) can't be driven by * a leaked Bearer. Error bodies + codes match the legacy 401 shapes exactly. + * + * `EntityManager` injection, not `@InjectRepository(Users)` — same reasoning + * as `JwtAuthGuard` (Plan 3b Task 1 RULING): the guard is instantiated per the + * HOST controller's module, and using the always-global `EntityManager` + * avoids new `forFeature` wiring in every module that applies it. */ @Injectable() export class CookieAuthGuard implements CanActivate { + constructor(private readonly em: EntityManager) {} + async canActivate(context: ExecutionContext): Promise { const req = context.switchToHttp().getRequest }>(); const cookieToken = req.cookies?.trek_session; if (!cookieToken) { throw new HttpException({ error: 'Cookie session required for this endpoint', code: 'COOKIE_AUTH_REQUIRED' }, 401); } - const user = await verifyJwtAndLoadUser(cookieToken); + const user = await verifyJwtAndLoadUser(cookieToken, this.em.getRepository(Users)); if (!user) { throw new HttpException({ error: 'Invalid or expired session', code: 'AUTH_REQUIRED' }, 401); } diff --git a/server/src/nest/auth/global-auth.guard.ts b/server/src/nest/auth/global-auth.guard.ts index 93788eeb5..8a7aa3f61 100644 --- a/server/src/nest/auth/global-auth.guard.ts +++ b/server/src/nest/auth/global-auth.guard.ts @@ -1,7 +1,9 @@ import { CanActivate, ExecutionContext, HttpException, Injectable } from '@nestjs/common'; import { GUARDS_METADATA } from '@nestjs/common/constants'; import { Reflector } from '@nestjs/core'; +import { EntityManager } from '@mikro-orm/core'; import type { Request } from 'express'; +import { Users } from '../../db/entities/Users.entity'; import { extractToken, verifyJwtAndLoadUser } from './jwt-verify'; import { IS_PUBLIC, OPTIONAL_AUTH } from './public.decorator'; @@ -28,7 +30,14 @@ import { IS_PUBLIC, OPTIONAL_AUTH } from './public.decorator'; */ @Injectable() export class GlobalAuthGuard implements CanActivate { - constructor(private readonly reflector: Reflector) {} + // EntityManager injection, not @InjectRepository(Users) — same reasoning + // as JwtAuthGuard (Plan 3b Task 1 RULING): kept uniform with the other + // three verifyJwtAndLoadUser callers even though this guard, registered + // only as AppModule's own APP_GUARD, would have tolerated either. + constructor( + private readonly reflector: Reflector, + private readonly em: EntityManager, + ) {} async canActivate(context: ExecutionContext): Promise { const handler = context.getHandler(); @@ -40,7 +49,7 @@ export class GlobalAuthGuard implements CanActivate { if (this.reflector.getAllAndOverride(OPTIONAL_AUTH, [handler, controller])) { const token = extractToken(req); - (req as { user: unknown }).user = (token ? await verifyJwtAndLoadUser(token) : null) || null; + (req as { user: unknown }).user = (token ? await verifyJwtAndLoadUser(token, this.em.getRepository(Users)) : null) || null; return true; } @@ -57,7 +66,7 @@ export class GlobalAuthGuard implements CanActivate { ]; if (declared.length > 0) { const declaredToken = extractToken(req); - (req as { user: unknown }).user = (declaredToken ? await verifyJwtAndLoadUser(declaredToken) : null) || null; + (req as { user: unknown }).user = (declaredToken ? await verifyJwtAndLoadUser(declaredToken, this.em.getRepository(Users)) : null) || null; return true; } @@ -65,7 +74,7 @@ export class GlobalAuthGuard implements CanActivate { if (!token) { throw new HttpException({ error: 'Access token required', code: 'AUTH_REQUIRED' }, 401); } - const user = await verifyJwtAndLoadUser(token); + const user = await verifyJwtAndLoadUser(token, this.em.getRepository(Users)); if (!user) { throw new HttpException({ error: 'Invalid or expired token', code: 'AUTH_REQUIRED' }, 401); } diff --git a/server/src/nest/auth/jwt-auth.guard.ts b/server/src/nest/auth/jwt-auth.guard.ts index e81acf568..016dd0143 100644 --- a/server/src/nest/auth/jwt-auth.guard.ts +++ b/server/src/nest/auth/jwt-auth.guard.ts @@ -1,5 +1,7 @@ import { CanActivate, ExecutionContext, HttpException, Injectable } from '@nestjs/common'; +import { EntityManager } from '@mikro-orm/core'; import type { Request } from 'express'; +import { Users } from '../../db/entities/Users.entity'; import { extractToken, verifyJwtAndLoadUser } from './jwt-verify'; /** @@ -9,16 +11,37 @@ import { extractToken, verifyJwtAndLoadUser } from './jwt-verify'; * and the loaded user are IDENTICAL to the Express middleware. No new tokens. * * Error bodies match the legacy 401 shape exactly so the client is unaffected. + * + * Injects `EntityManager`, not `@InjectRepository(Users)` (Plan 3b Task 1 + * RULING on `verifyJwtAndLoadUser`'s callers): this guard is applied via + * `@UseGuards(JwtAuthGuard)` on ~70 controllers across nearly every domain + * module in the app, and Nest resolves a class-referenced guard's + * constructor dependencies from the HOST CONTROLLER'S OWN module graph, not + * from a single "the guard's module" — `@InjectRepository(Users)` would need + * `MikroOrmModule.forFeature([Users])` added to every one of those ~70 + * modules (and to every partial e2e-testing-module harness that composes a + * guarded controller without the full app), which is the exact blast radius + * D5 exists to avoid. `EntityManager`/`MikroORM`, by contrast, come from + * `MikroOrmModule.forRoot`'s core module, which IS `@Global()` — already + * required wherever a JWT-guarded route is exercised for a real user (the + * ORM owns `users` everywhere else too) — so no module needs new wiring. + * `this.em.getRepository(Users)` inside `canActivate` is the same + * `orm.em.getRepository(Users)`-inside-the-request pattern + * `platform.routes.ts::servePhoto` uses (Task 0); `this.em` here is already + * the request-scoped fork, since `canActivate` only ever runs inside a real + * HTTP request Nest's `@mikro-orm/nestjs` middleware has already wrapped. */ @Injectable() export class JwtAuthGuard implements CanActivate { + constructor(private readonly em: EntityManager) {} + async canActivate(context: ExecutionContext): Promise { const req = context.switchToHttp().getRequest(); const token = extractToken(req); if (!token) { throw new HttpException({ error: 'Access token required', code: 'AUTH_REQUIRED' }, 401); } - const user = await verifyJwtAndLoadUser(token); + const user = await verifyJwtAndLoadUser(token, this.em.getRepository(Users)); if (!user) { throw new HttpException({ error: 'Invalid or expired token', code: 'AUTH_REQUIRED' }, 401); } diff --git a/server/src/nest/auth/jwt-verify.ts b/server/src/nest/auth/jwt-verify.ts index b68454d08..99e6a143c 100644 --- a/server/src/nest/auth/jwt-verify.ts +++ b/server/src/nest/auth/jwt-verify.ts @@ -1,8 +1,8 @@ import type { Request } from 'express'; import jwt from 'jsonwebtoken'; -import { db } from '../../db/database'; import { JWT_SECRET } from '../../config'; import type { User } from '../../types'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; /** * The canonical JWT session check. Every auth surface goes through here — the @@ -15,6 +15,16 @@ import type { User } from '../../types'; * would have no way to inject one. Note JWT_SECRET is deliberately a live * binding from src/config, not an app-config value: the admin panel rotates it * at runtime and `export let` is what makes a rotation take effect in-process. + * + * `verifyJwtAndLoadUser` stays a free function (Plan 3b Task 1 RULING) and + * gains an explicit `users: UsersRepository` parameter — DI over a + * module-level accessor (CLAUDE.md "DI over global mutable module state"). + * This file imports neither the ORM nor `RequestContext`: every caller + * resolves its own `UsersRepository` (through Nest DI, or — for the one + * pre-init caller, `platform.routes.ts::servePhoto` — `orm.em.getRepository(Users)` + * resolved INSIDE `applyPlatformUploads`'s `withRequestContext` wrapper, + * `orm` threaded in from `bootstrap.ts`) and passes it in. The legacy `db` + * proxy import is gone. */ export function extractToken(req: Request): string | null { // Prefer httpOnly cookie; fall back to Authorization: Bearer (MCP, API clients) @@ -54,16 +64,14 @@ export function decodeSessionClaims(token: string | undefined): SessionClaims | return decoded as SessionClaims; } -export async function verifyJwtAndLoadUser(token: string): Promise { +export async function verifyJwtAndLoadUser(token: string, users: UsersRepository): Promise { try { const decoded = jwt.verify(token, JWT_SECRET, { algorithms: ['HS256'] }) as { id: number; pv?: number; purpose?: string }; // Purpose-scoped tokens (e.g. the short-lived mfa_login token) share this // secret but are not full session tokens — only their dedicated endpoint // may accept them, so reject any token carrying a purpose claim here. if (decoded.purpose) return null; - const row = db.prepare( - 'SELECT id, username, email, role, password_version FROM users WHERE id = ?' - ).get(decoded.id) as (User & { password_version?: number }) | undefined; + const row = await users.findByIdWithPasswordVersion(decoded.id); if (!row) return null; // Session invalidation: any token whose embedded password_version // predates the user's current one is rejected. Tokens issued before diff --git a/server/src/nest/auth/mfa-policy.guard.ts b/server/src/nest/auth/mfa-policy.guard.ts index 720902452..475619177 100644 --- a/server/src/nest/auth/mfa-policy.guard.ts +++ b/server/src/nest/auth/mfa-policy.guard.ts @@ -1,11 +1,17 @@ import { CanActivate, ExecutionContext, HttpException, Injectable, SetMetadata } from '@nestjs/common'; import { Reflector } from '@nestjs/core'; +import { InjectRepository } from '@mikro-orm/nestjs'; import type { Request } from 'express'; -import { DatabaseService } from '../database/database.service'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; import { DEMO_EMAILS } from '../common/demo'; import { IS_PUBLIC } from './public.decorator'; import type { User } from '../../types'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; +import { WebauthnCredentials } from '../../db/entities/WebauthnCredentials.entity'; +import type { WebauthnCredentialsRepository } from '../../db/repositories/WebauthnCredentials.repository'; /** Metadata key `@MfaExempt()` writes. */ export const MFA_EXEMPT = 'trek:mfa-exempt'; @@ -44,7 +50,9 @@ type MfaRequest = Request & { user?: User }; @Injectable() export class MfaPolicyGuard implements CanActivate { constructor( - private readonly db: DatabaseService, + @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, + @InjectRepository(Users) private readonly users: UsersRepository, + @InjectRepository(WebauthnCredentials) private readonly webauthnCredentials: WebauthnCredentialsRepository, private readonly env: RuntimeEnvService, private readonly reflector: Reflector, ) {} @@ -60,24 +68,19 @@ export class MfaPolicyGuard implements CanActivate { // No session: whoever answers next decides, exactly as before. if (!user) return true; - const requireRow = this.db.get<{ value: string }>( - "SELECT value FROM app_settings WHERE key = 'require_mfa'", - ); - if (requireRow?.value !== 'true') return true; + const requireMfa = await this.appSettings.getValue('require_mfa'); + if (requireMfa !== 'true') return true; if (this.env.isDemoMode() && user.email && DEMO_EMAILS.has(user.email)) return true; - const row = this.db.get<{ mfa_enabled: number | boolean }>( - 'SELECT mfa_enabled FROM users WHERE id = ?', - user.id, - ); + const row = await this.users.getMfaEnabled(user.id); if (!row) return true; // A user-verified passkey is phishing-resistant and inherently two-factor, so // owning at least one satisfies require_mfa exactly like TOTP does. // (All stored passkeys were registered with userVerification required.) - const mfaOk = row.mfa_enabled === 1 || row.mfa_enabled === true; - const passkeyOk = !!this.db.get('SELECT 1 FROM webauthn_credentials WHERE user_id = ? LIMIT 1', user.id); + const mfaOk = row.mfa_enabled === 1; + const passkeyOk = await this.webauthnCredentials.hasAny(user.id); if (mfaOk || passkeyOk) return true; if (this.reflector.getAllAndOverride(MFA_EXEMPT, [handler, controller])) return true; diff --git a/server/src/nest/auth/optional-jwt.guard.ts b/server/src/nest/auth/optional-jwt.guard.ts index 8f7817dbb..88260559a 100644 --- a/server/src/nest/auth/optional-jwt.guard.ts +++ b/server/src/nest/auth/optional-jwt.guard.ts @@ -1,5 +1,7 @@ import { CanActivate, ExecutionContext, Injectable } from '@nestjs/common'; +import { EntityManager } from '@mikro-orm/core'; import type { Request } from 'express'; +import { Users } from '../../db/entities/Users.entity'; import { extractToken, verifyJwtAndLoadUser } from './jwt-verify'; /** @@ -7,13 +9,18 @@ import { extractToken, verifyJwtAndLoadUser } from './jwt-verify'; * loaded user when a valid token is present, otherwise leaves it null — and * always allows the request through (never 401). Used for endpoints whose * response varies by auth state but don't require it (e.g. /app-config). + * + * `EntityManager` injection, not `@InjectRepository(Users)` — same reasoning + * as `JwtAuthGuard` (Plan 3b Task 1 RULING). */ @Injectable() export class OptionalJwtGuard implements CanActivate { + constructor(private readonly em: EntityManager) {} + async canActivate(context: ExecutionContext): Promise { const req = context.switchToHttp().getRequest(); const token = extractToken(req); - (req as { user: unknown }).user = (token ? await verifyJwtAndLoadUser(token) : null) || null; + (req as { user: unknown }).user = (token ? await verifyJwtAndLoadUser(token, this.em.getRepository(Users)) : null) || null; return true; } } diff --git a/server/src/nest/auth/user-profile.service.ts b/server/src/nest/auth/user-profile.service.ts index d775a6d98..49fcc99b8 100644 --- a/server/src/nest/auth/user-profile.service.ts +++ b/server/src/nest/auth/user-profile.service.ts @@ -1,7 +1,6 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@mikro-orm/nestjs'; import { readEnv, getAppUrl } from '../../app-config'; -import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; import { StorageService } from '../storage/storage.service'; import { decrypt_api_key, maybe_encrypt_api_key } from '../common/crypto/apiKeyCrypto'; @@ -16,11 +15,10 @@ import { type InstanceApiKeyName, } from '../settings/instance-api-keys'; import { SEARCH_TEXT_FIELD_MASK } from '../maps/maps.helpers'; -import { User } from '../../types'; import { AppSettings } from '../../db/entities/AppSettings.entity'; import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; import { Users } from '../../db/entities/Users.entity'; -import type { UsersRepository } from '../../db/repositories/Users.repository'; +import type { UsersRepository, UserApiKeyColumns, UserProfilePatch } from '../../db/repositories/Users.repository'; /** * The account a user administers about themselves: display settings, avatar, @@ -30,20 +28,19 @@ import type { UsersRepository } from '../../db/repositories/Users.repository'; * Split out of AuthService, which had grown to 1471 lines by owning identity, * profile, settings and tokens together. None of this is identity: no password * is checked here, no session is issued, no MFA secret is touched. Everything - * moved verbatim — same SQL, same validation order, same masking, same error - * strings and status codes. + * moved verbatim — same statement shapes, same validation order, same masking, + * same error strings and status codes. * - * DatabaseService was the only injected dependency at first, which is what - * made this the second-cheapest cut after tokens; `AppSettingsRepository`/ - * `UsersRepository` (Plan 3a Task 5) were added later purely so the three - * `instance-api-keys.ts` calls below pass their own repository rather than - * that file resolving one itself — every other read/write here is still - * `DatabaseService`'s raw SQL (auth is outside Plan 3a). + * Plan 3b Task 1: every read/write here now goes through `UsersRepository` + * (`AppSettingsRepository`/`UsersRepository` were already present, Plan 3a + * Task 5, purely so `instance-api-keys.ts` could take its own repository + * rather than resolving one itself); `DatabaseService` is gone from the + * constructor entirely — nothing in this file reads or writes raw SQL any + * more. */ @Injectable() export class UserProfileService { constructor( - private readonly db: DatabaseService, private readonly storage: StorageService, private readonly uow: UnitOfWork, @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, @@ -64,12 +61,12 @@ export class UserProfileService { return readEnv().managed.enabled; } - /** The three key columns plus the role that decides where a save lands. */ - private async currentKeys(userId: number) { - return this.db.get>( - 'SELECT role, maps_api_key, openweather_api_key, unsplash_api_key, amap_api_key FROM users WHERE id = ?', - userId - ); + /** + * The three key columns plus the role that decides where a save lands. + * `SELECT role, maps_api_key, openweather_api_key, unsplash_api_key, amap_api_key FROM users WHERE id = ?` (UP1). + */ + private async currentKeys(userId: number): Promise { + return this.usersRepo.getApiKeyColumns(userId); } /** @@ -81,7 +78,7 @@ export class UserProfileService { */ private async storedKeyPlaintext( name: 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key' | 'amap_api_key', - current: Pick | undefined, + current: UserApiKeyColumns | null, isAdmin: boolean, ): Promise { if (isAdmin && (INSTANCE_API_KEY_NAMES as readonly string[]).includes(name)) { @@ -104,7 +101,7 @@ export class UserProfileService { */ private async changedKeyNames( body: Record, - current: Pick | undefined, + current: UserApiKeyColumns | null, isAdmin: boolean, skipped: string[] = [], ): Promise { @@ -143,10 +140,7 @@ export class UserProfileService { const isAdmin = current?.role === 'admin'; const changedKeys = await this.changedKeyNames({ maps_api_key }, current, isAdmin); await this.uow.transactional(async () => { - this.db.run( - 'UPDATE users SET maps_api_key = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - maybe_encrypt_api_key(maps_api_key), userId - ); + await this.usersRepo.updateMapsKey(userId, maybe_encrypt_api_key(maps_api_key)); await this.mirrorInstanceKeys({ maps_api_key }, isAdmin); }); return { success: true, maps_api_key: mask_stored_api_key(maps_api_key), changedKeys }; @@ -163,23 +157,18 @@ export class UserProfileService { await this.uow.transactional(async () => { // `?? null` instead of the former non-null assertions: a user row deleted // mid-request must degrade to a 0-row UPDATE, not a TypeError/500. - this.db.run( - 'UPDATE users SET maps_api_key = ?, openweather_api_key = ?, unsplash_api_key = ?, amap_api_key = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - body.maps_api_key !== undefined ? maybe_encrypt_api_key(body.maps_api_key) : current?.maps_api_key ?? null, - body.openweather_api_key !== undefined ? maybe_encrypt_api_key(body.openweather_api_key) : current?.openweather_api_key ?? null, - body.unsplash_api_key !== undefined ? maybe_encrypt_api_key(body.unsplash_api_key) : current?.unsplash_api_key ?? null, - body.amap_api_key !== undefined ? maybe_encrypt_api_key(body.amap_api_key) : current?.amap_api_key ?? null, - userId - ); + await this.usersRepo.updateApiKeys(userId, { + maps_api_key: body.maps_api_key !== undefined ? maybe_encrypt_api_key(body.maps_api_key) : current?.maps_api_key ?? null, + openweather_api_key: body.openweather_api_key !== undefined ? maybe_encrypt_api_key(body.openweather_api_key) : current?.openweather_api_key ?? null, + unsplash_api_key: body.unsplash_api_key !== undefined ? maybe_encrypt_api_key(body.unsplash_api_key) : current?.unsplash_api_key ?? null, + amap_api_key: body.amap_api_key !== undefined ? maybe_encrypt_api_key(body.amap_api_key) : current?.amap_api_key ?? null, + }); await this.mirrorInstanceKeys(body, isAdmin); }); - const updated = this.db.get>( - 'SELECT id, username, email, role, maps_api_key, openweather_api_key, unsplash_api_key, amap_api_key, avatar, mfa_enabled FROM users WHERE id = ?', - userId - ); + const updated = await this.usersRepo.findProfileWithKeys(userId); - const u = updated ? { ...updated, mfa_enabled: !!(updated.mfa_enabled === 1 || updated.mfa_enabled === true) } : undefined; + const u = updated ? { ...updated, mfa_enabled: !!(updated.mfa_enabled === 1) } : undefined; return { success: true, ...(blocked.length ? { managed_keys: blocked } : {}), @@ -203,7 +192,7 @@ export class UserProfileService { if (!/^[a-zA-Z0-9_.-]+$/.test(trimmed)) { return { error: 'Username can only contain letters, numbers, underscores, dots and hyphens', status: 400 }; } - const conflict = this.db.get('SELECT id FROM users WHERE LOWER(username) = LOWER(?) AND id != ? AND COALESCE(is_guest, 0) = 0', trimmed, userId); + const conflict = await this.usersRepo.findIdByUsernameCI(trimmed, userId); if (conflict) return { error: 'Username already taken', status: 409 }; } @@ -212,24 +201,26 @@ export class UserProfileService { if (!trimmed || !EMAIL_REGEX.test(trimmed)) { return { error: 'Invalid email format', status: 400 }; } - const conflict = this.db.get('SELECT id FROM users WHERE LOWER(email) = LOWER(?) AND id != ? AND COALESCE(is_guest, 0) = 0', trimmed, userId); + const conflict = await this.usersRepo.findIdByEmailCI(trimmed, userId); if (conflict) return { error: 'Email already taken', status: 409 }; } - const updates: string[] = []; - const params: (string | number | null)[] = []; - // The name and email half of this body stays the user's own in every mode; // only the three key columns answer to the operator. const { blocked } = splitManagedKeys(body, this.managed); const keyLocked = blocked.length > 0; - if (maps_api_key !== undefined && !keyLocked) { updates.push('maps_api_key = ?'); params.push(maybe_encrypt_api_key(maps_api_key)); } - if (openweather_api_key !== undefined && !keyLocked) { updates.push('openweather_api_key = ?'); params.push(maybe_encrypt_api_key(openweather_api_key)); } - if (unsplash_api_key !== undefined && !keyLocked) { updates.push('unsplash_api_key = ?'); params.push(maybe_encrypt_api_key(unsplash_api_key)); } - if (amap_api_key !== undefined && !keyLocked) { updates.push('amap_api_key = ?'); params.push(maybe_encrypt_api_key(amap_api_key)); } - if (username !== undefined) { updates.push('username = ?'); params.push(username.trim()); } - if (email !== undefined) { updates.push('email = ?'); params.push(email.trim()); } + // The bounded set of columns UP7's dynamic SET clause could touch — built + // here, at the call site, exactly as the legacy `updates`/`params` arrays + // were; `UsersRepository.patchProfile` writes this typed partial in ONE + // statement, never a built SQL string. + const changes: UserProfilePatch = {}; + if (maps_api_key !== undefined && !keyLocked) changes.maps_api_key = maybe_encrypt_api_key(maps_api_key); + if (openweather_api_key !== undefined && !keyLocked) changes.openweather_api_key = maybe_encrypt_api_key(openweather_api_key); + if (unsplash_api_key !== undefined && !keyLocked) changes.unsplash_api_key = maybe_encrypt_api_key(unsplash_api_key); + if (amap_api_key !== undefined && !keyLocked) changes.amap_api_key = maybe_encrypt_api_key(amap_api_key); + if (username !== undefined) changes.username = username.trim(); + if (email !== undefined) changes.email = email.trim(); // Read before the write, so the comparison sees the old value; the role in // the same row decides whether the two instance-wide names travel with it. @@ -237,21 +228,16 @@ export class UserProfileService { const isAdmin = current?.role === 'admin'; const changedKeys = keyLocked ? [] : await this.changedKeyNames(body, current, isAdmin, blocked); - if (updates.length > 0) { - updates.push('updated_at = CURRENT_TIMESTAMP'); - params.push(userId); + if (Object.keys(changes).length > 0) { await this.uow.transactional(async () => { - this.db.run(`UPDATE users SET ${updates.join(', ')} WHERE id = ?`, ...params); + await this.usersRepo.patchProfile(userId, changes); if (!keyLocked) await this.mirrorInstanceKeys(body, isAdmin); }); } - const updated = this.db.get>( - 'SELECT id, username, email, role, maps_api_key, openweather_api_key, unsplash_api_key, amap_api_key, avatar, mfa_enabled FROM users WHERE id = ?', - userId - ); + const updated = await this.usersRepo.findProfileWithKeys(userId); - const u = updated ? { ...updated, mfa_enabled: !!(updated.mfa_enabled === 1 || updated.mfa_enabled === true) } : undefined; + const u = updated ? { ...updated, mfa_enabled: !!(updated.mfa_enabled === 1) } : undefined; return { success: true, ...(blocked.length ? { managed_keys: blocked } : {}), @@ -261,10 +247,7 @@ export class UserProfileService { } async getSettings(userId: number): Promise<{ error?: string; status?: number; settings?: Record }> { - const user = this.db.get>( - 'SELECT role, maps_api_key, openweather_api_key, unsplash_api_key, amap_api_key FROM users WHERE id = ?', - userId - ); + const user = await this.usersRepo.getApiKeyColumns(userId); if (user?.role !== 'admin') return { error: 'Admin access required', status: 403 }; // The one endpoint in the codebase that hands back a stored key in the @@ -303,29 +286,29 @@ export class UserProfileService { // ------------------------------------------------------------------------- async saveAvatar(userId: number, filename: string) { - const current = this.db.get<{ avatar: string | null }>('SELECT avatar FROM users WHERE id = ?', userId); + const current = await this.usersRepo.getAvatar(userId); // Only a locally uploaded file has something to clean up. An OIDC picture URL // (#1399) has no storage object, so skip the delete entirely. - if (current?.avatar && !/^https:\/\//i.test(current.avatar)) { + if (current && !/^https:\/\//i.test(current)) { // Fire-and-forget parity: leftover objects are harmless; the DB update is // the source of truth for which avatar is current. The catch also // swallows a hostile stored value the central key validation rejects. - await this.storage.delete('avatars', current.avatar).catch(() => {}); + await this.storage.delete('avatars', current).catch(() => {}); } - this.db.run('UPDATE users SET avatar = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', filename, userId); + await this.usersRepo.setAvatar(userId, filename); - const updated = this.db.get>('SELECT id, username, email, role, avatar FROM users WHERE id = ?', userId); + const updated = await this.usersRepo.findProfileBasic(userId); return { success: true, avatar_url: avatarUrl(updated || {}) }; } async deleteAvatar(userId: number) { - const current = this.db.get<{ avatar: string | null }>('SELECT avatar FROM users WHERE id = ?', userId); + const current = await this.usersRepo.getAvatar(userId); // An OIDC picture URL (#1399) has no storage object — only delete an uploaded one. - if (current?.avatar && !/^https:\/\//i.test(current.avatar)) { - await this.storage.delete('avatars', current.avatar).catch(() => {}); + if (current && !/^https:\/\//i.test(current)) { + await this.storage.delete('avatars', current).catch(() => {}); } - this.db.run('UPDATE users SET avatar = NULL, updated_at = CURRENT_TIMESTAMP WHERE id = ?', userId); + await this.usersRepo.setAvatar(userId, null); return { success: true }; } @@ -336,10 +319,7 @@ export class UserProfileService { async listUsers(excludeUserId: number) { // The global user directory feeds the trip member-add / contributor pickers — // guests (#1362) are trip-scoped and must never be selectable here. - const users = this.db.all>( - 'SELECT id, username, avatar FROM users WHERE id != ? AND COALESCE(is_guest, 0) = 0 ORDER BY username ASC', - excludeUserId - ); + const users = await this.usersRepo.listOthersNonGuest(excludeUserId); return users.map(u => ({ ...u, avatar_url: avatarUrl(u) })); } @@ -348,7 +328,7 @@ export class UserProfileService { // ------------------------------------------------------------------------- async validateKeys(userId: number): Promise<{ error?: string; status?: number; maps: boolean; weather: boolean; maps_details: null | { ok: boolean; status: number | null; status_text: string | null; error_message: string | null; error_status: string | null; error_raw: string | null } }> { - const user = this.db.get>('SELECT role, openweather_api_key FROM users WHERE id = ?', userId); + const user = await this.usersRepo.getRoleAndWeatherKey(userId); if (user?.role !== 'admin') return { error: 'Admin access required', status: 403, maps: false, weather: false, maps_details: null }; const result: { diff --git a/server/src/nest/auth/webauthn-config.service.ts b/server/src/nest/auth/webauthn-config.service.ts index 36bf015a8..61d26a6b1 100644 --- a/server/src/nest/auth/webauthn-config.service.ts +++ b/server/src/nest/auth/webauthn-config.service.ts @@ -1,6 +1,8 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { getAppUrl, readEnv } from '../../app-config'; -import { DatabaseService } from '../database/database.service'; +import { AppSettings } from '../../db/entities/AppSettings.entity'; +import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; function hostOf(url: string): string | null { try { @@ -68,10 +70,11 @@ export interface WebauthnConfig { */ @Injectable() export class WebauthnConfigService { - constructor(private readonly db: DatabaseService) {} + constructor(@InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository) {} + /** `SELECT value FROM app_settings WHERE key = ?` (WC1), through `AppSettingsRepository.getValue` (Plan 3a). */ private async setting(key: string): Promise { - const raw = this.db.get<{ value: string }>('SELECT value FROM app_settings WHERE key = ?', key)?.value; + const raw = (await this.appSettings.getValue(key)) ?? undefined; const trimmed = raw?.trim(); return trimmed ? trimmed : null; } diff --git a/server/src/nest/files/files.service.ts b/server/src/nest/files/files.service.ts index 5f1a56a61..680fa7b59 100644 --- a/server/src/nest/files/files.service.ts +++ b/server/src/nest/files/files.service.ts @@ -8,6 +8,8 @@ import { PermissionsService } from '../permissions/permissions.service'; import { avatarUrl } from '../common/avatarUrl'; import { EphemeralTokenService } from '../auth/ephemeral-token.service'; import { verifyJwtAndLoadUser } from '../auth/jwt-verify'; +import { EntityManager } from '@mikro-orm/core'; +import { Users } from '../../db/entities/Users.entity'; import type { User, TripFile } from '../../types'; import { DatabaseService, type TripAccess } from '../database/database.service'; import { DEFAULT_ALLOWED_EXTENSIONS } from './files.constants'; @@ -79,6 +81,11 @@ export class FilesService { private readonly realtime: RealtimeService, private readonly tokens: EphemeralTokenService, private readonly storage: StorageService, + // EntityManager, not @InjectRepository(Users) — same reasoning as + // JwtAuthGuard (Plan 3b Task 1 RULING on verifyJwtAndLoadUser's callers): + // kept uniform with the guards rather than adding a one-off + // MikroOrmModule.forFeature([Users]) to FilesModule for this single call. + private readonly em: EntityManager, ) {} async verifyTripAccess(tripId: string | number, userId: number) { @@ -110,7 +117,7 @@ export class FilesService { // Use the shared helper so the password_version gate applies here too; // previously this bypassed the check and stolen download tokens stayed // valid across a password reset. - const user = await verifyJwtAndLoadUser(jwtToken); + const user = await verifyJwtAndLoadUser(jwtToken, this.em.getRepository(Users)); if (!user) return { error: 'Invalid or expired token', status: 401 }; return { userId: user.id }; } diff --git a/server/src/nest/platform/platform.routes.ts b/server/src/nest/platform/platform.routes.ts index 3a16fbe67..c79f971c9 100644 --- a/server/src/nest/platform/platform.routes.ts +++ b/server/src/nest/platform/platform.routes.ts @@ -8,6 +8,8 @@ import { db } from '../../db/database'; import { withRequestContext } from '../database/request-context'; import { StorageService } from '../storage/storage.service'; import { StorageInvalidKeyError, StorageNotFoundError, type StorageCategory } from '../storage/storage.types'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; // Platform / transport routes extracted verbatim from createApp() (app.ts) so they can be // mounted on either the legacy Express app or the NestJS Express instance (strangler A6/A8). @@ -72,7 +74,7 @@ export function storageStaticHandler(storage: StorageService, category: StorageC }; } -async function servePhoto(storage: StorageService, req: Request, res: Response): Promise { +async function servePhoto(storage: StorageService, req: Request, res: Response, users: UsersRepository): Promise { const safeName = path.basename(req.params.filename); // Parity: after basename(), the old resolve()+startsWith guard could only // fire when the remaining segment was '..' — keep that exact 403. @@ -104,7 +106,7 @@ async function servePhoto(storage: StorageService, req: Request, res: Response): } // JWT session path (with pv check). - const user = await verifyJwtAndLoadUser(rawToken); + const user = await verifyJwtAndLoadUser(rawToken, users); if (user) return sendPhoto(); // Share-token path: require the token to cover the exact trip the @@ -132,14 +134,19 @@ async function servePhoto(storage: StorageService, req: Request, res: Response): * mounted here, on the raw Express instance, BEFORE `app.init()` — every * `/uploads/photos/*` request runs this handler entirely outside Nest's * per-request EntityManager fork, forever, not just during a boot window. - * `verifyJwtAndLoadUser` (`jwt-verify.ts`) reads `users` through the legacy - * `db` proxy today, so nothing here needs the ORM yet — but the inventory's - * proposed conversion (`UsersRepository.findByIdWithPasswordVersion`, Task 1) - * would throw `cannotUseGlobalContext` on every photo request the instant it - * lands, because nothing forks a context for this pre-init route. Wrapping - * the handler in `withRequestContext` NOW, before any repository call exists - * on this path, means Task 1 converts `jwt-verify.ts` without touching this - * file again and without a single broken photo request in between. + * `verifyJwtAndLoadUser` (`jwt-verify.ts`, JV1) now reads `users` through + * `UsersRepository` (Plan 3b Task 1) rather than the legacy `db` proxy — the + * exact case Task 0's wrap was built for. `orm.em.getRepository(Users)` is + * resolved inside the arrow passed to `withRequestContext` below: `orm.em` + * is the global, context-resolving EntityManager, and MikroORM's repository + * object holds a reference to that same proxy rather than a snapshot, so a + * query issued through it later resolves whatever `AsyncLocalStorage` + * context is active AT QUERY TIME — not at the moment `getRepository()` was + * called. What actually gates this is `withRequestContext` wrapping the + * whole `servePhoto(...)` call (query included): removing the wrapper + * entirely reproduces Task 0's `ValidationError: Using global EntityManager + * instance...` (verified directly — mutation-proof in `task-1-report.md`), + * exactly the failure `PHOTOCTX-002`/`SEAM-002` guard against. * * Optional at the type level only, the same shape `TrekWsAdapter` uses for * its own D6 wrapper (`src/nest/realtime/trek-ws.adapter.ts`): `bootstrap.ts` @@ -184,7 +191,7 @@ export function applyPlatformUploads(app: express.Application, storage: StorageS next(new Error('applyPlatformUploads: no MikroORM available to build a request context for /uploads/photos/*')); return; } - return withRequestContext(orm, () => servePhoto(storage, req, res)).catch(next); + return withRequestContext(orm, () => servePhoto(storage, req, res, orm.em.getRepository(Users))).catch(next); }); // Block direct access to /uploads/files diff --git a/server/src/nest/plugins/plugins-proxy.controller.ts b/server/src/nest/plugins/plugins-proxy.controller.ts index 92706c366..5b860144e 100644 --- a/server/src/nest/plugins/plugins-proxy.controller.ts +++ b/server/src/nest/plugins/plugins-proxy.controller.ts @@ -1,9 +1,11 @@ import { All, Controller, Param, Req, Res } from '@nestjs/common'; +import { EntityManager } from '@mikro-orm/core'; import type { Request, Response } from 'express'; import { extractToken, verifyJwtAndLoadUser } from '../auth/jwt-verify'; import { pluginsEnabled } from './kill-switch'; import { PluginRuntimeService } from './plugin-runtime.service'; import { Public } from '../auth/public.decorator'; +import { Users } from '../../db/entities/Users.entity'; /** * Proxies a plugin's own HTTP routes at /api/plugins/:id/* (#plugins, M2). @@ -74,7 +76,12 @@ function toRelativeLocation(loc: unknown): string | null { @Public('auth is data-driven per plugin route (route.auth in the manifest), asserted in the handler') @Controller('api/plugins/:pluginId') export class PluginsProxyController { - constructor(private readonly runtime: PluginRuntimeService) {} + // EntityManager, not @InjectRepository(Users) — same reasoning as + // JwtAuthGuard (Plan 3b Task 1 RULING on verifyJwtAndLoadUser's callers). + constructor( + private readonly runtime: PluginRuntimeService, + private readonly em: EntityManager, + ) {} @All('*path') async proxy(@Param('pluginId') pluginId: string, @Req() req: Request, @Res() res: Response): Promise { @@ -95,7 +102,7 @@ export class PluginsProxyController { let user: { id: number; username: string; role?: 'admin' | 'user' } | null = null; if (route.auth) { const token = extractToken(req); - const loaded = token ? await verifyJwtAndLoadUser(token) : null; + const loaded = token ? await verifyJwtAndLoadUser(token, this.em.getRepository(Users)) : null; if (!loaded) { res.status(401).json({ error: 'Access token required', code: 'AUTH_REQUIRED' }); return; diff --git a/server/src/nest/realtime/trek-ws.adapter.ts b/server/src/nest/realtime/trek-ws.adapter.ts index fa257fd46..14cd87ee6 100644 --- a/server/src/nest/realtime/trek-ws.adapter.ts +++ b/server/src/nest/realtime/trek-ws.adapter.ts @@ -89,6 +89,15 @@ function withinBurst(socket: TrekWebSocket): boolean { * `error` event on the socket; unhandled, Node rethrows it and the process * dies. That is #1576, and it only reproduces against a hostile client, so * nothing in CI would catch its removal. + * + * All three per-connection lifecycle hooks the base adapter offers — + * `bindClientConnect` (`OnGatewayConnection`), `bindMessageHandlers` + * (`@SubscribeMessage`) and `bindClientDisconnect` (`OnGatewayDisconnect`) — + * are overridden here to run their callback inside `withRequestContext` + * (D6): none of the three has an HTTP request behind it, so none gets a + * forked EntityManager unless this adapter forks one itself. `create` + * above and `close` below are the only other overrides, and neither + * dispatches into gateway/handler code, so neither needs the wrap. */ export class TrekWsAdapter extends WsAdapter { private readonly httpServerRef: HttpServer; @@ -184,6 +193,26 @@ export class TrekWsAdapter extends WsAdapter { }); } + /** + * Dispatches Nest's `OnGatewayDisconnect` hook (`RealtimeGateway.handleDisconnect`) + * the same way `bindClientConnect` above dispatches `OnGatewayConnection` — + * the base `WsAdapter.bindClientDisconnect` just does `client.on(CLOSE_EVENT, + * callback)` with no context of any kind. `handleDisconnect` reads no + * repository today (Task 0 review addendum, LOW item 2), but it is a + * lifecycle hook with no HTTP request behind it exactly like the connect and + * message hooks are, so it gets the same wrap NOW rather than becoming the + * next "the adapter is done" surprise the moment a future change makes it + * read one. Same fail-closed shape as the other two: throw before calling. + */ + bindClientDisconnect(client: TrekWebSocket, callback: (...args: unknown[]) => void): void { + client.on('close', (...args: unknown[]) => { + if (!this.orm) { + throw new Error('TrekWsAdapter: no MikroORM available to build a request context for this disconnect'); + } + withRequestContext(this.orm, () => callback(...args)); + }); + } + bindMessageHandlers( socket: TrekWebSocket, handlers: MessageMappingProperties[], diff --git a/server/tests/e2e/feeds.e2e.test.ts b/server/tests/e2e/feeds.e2e.test.ts index 4669df472..3842650c5 100644 --- a/server/tests/e2e/feeds.e2e.test.ts +++ b/server/tests/e2e/feeds.e2e.test.ts @@ -79,6 +79,10 @@ import { GlobalAuthGuard } from '../../src/nest/auth/global-auth.guard'; import { MfaPolicyGuard } from '../../src/nest/auth/mfa-policy.guard'; import { TestUnitOfWorkModule } from '../helpers/test-uow'; import { createTestMikroOrmModule } from '../helpers/test-orm'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; +import { AppSettings } from '../../src/db/entities/AppSettings.entity'; +import { Users } from '../../src/db/entities/Users.entity'; +import { WebauthnCredentials } from '../../src/db/entities/WebauthnCredentials.entity'; const BASE = 'https://trek.example.test'; @@ -94,7 +98,19 @@ describe('Calendar-feed e2e (real auth guard + temp SQLite)', () => { // on the feed controller passed here while production 401'd every calendar // client). const moduleRef = await Test.createTestingModule({ - imports: [await TestUnitOfWorkModule.forRoot(db), await createTestMikroOrmModule(db), AppConfigModule, DatabaseModule, RealtimeModule, FeedsModule], + imports: [ + await TestUnitOfWorkModule.forRoot(db), + await createTestMikroOrmModule(db), + // MfaPolicyGuard (Plan 3b Task 1) needs these three repositories — + // mirrors app.module.ts's own forFeature, added there for the same + // reason (MfaPolicyGuard is provided only as an APP_GUARD, never + // per-controller, so this is the one place that needs it). + MikroOrmModule.forFeature([AppSettings, Users, WebauthnCredentials]), + AppConfigModule, + DatabaseModule, + RealtimeModule, + FeedsModule, + ], providers: [ { provide: APP_GUARD, useClass: GlobalAuthGuard }, { provide: APP_GUARD, useClass: MfaPolicyGuard }, diff --git a/server/tests/e2e/place-shadow.e2e.test.ts b/server/tests/e2e/place-shadow.e2e.test.ts index f18ab6270..4d195270e 100644 --- a/server/tests/e2e/place-shadow.e2e.test.ts +++ b/server/tests/e2e/place-shadow.e2e.test.ts @@ -41,6 +41,7 @@ vi.mock('../../src/db/database', () => ({ import { PlaceShadowModule } from '../../src/nest/place-shadow/place-shadow.module'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; import { TestUnitOfWorkModule } from '../helpers/test-uow'; +import { createTestMikroOrmModule } from '../helpers/test-orm'; const PICK = { query: 'kaffee bar am dobi', @@ -66,8 +67,13 @@ describe('/api/place-shadow e2e (real guards + temp SQLite)', () => { let app: Awaited>; async function build() { + // JwtAuthGuard (Plan 3b Task 1) injects EntityManager — needs + // MikroOrmModule.forRoot in the graph, same as every other e2e harness + // guarding a route with it; this suite's minimal hand-rolled `users` + // table already carries the five columns + // `findByIdWithPasswordVersion` selects. const moduleRef = await Test.createTestingModule({ - imports: [await TestUnitOfWorkModule.forRoot(db), DatabaseModule, PlaceShadowModule], + imports: [await TestUnitOfWorkModule.forRoot(db), await createTestMikroOrmModule(db), DatabaseModule, PlaceShadowModule], providers: [{ provide: APP_PIPE, useClass: ZodValidationPipe }], }).compile(); const nest = moduleRef.createNestApplication(); diff --git a/server/tests/e2e/weather.e2e.test.ts b/server/tests/e2e/weather.e2e.test.ts index 0a45985c2..8035451e0 100644 --- a/server/tests/e2e/weather.e2e.test.ts +++ b/server/tests/e2e/weather.e2e.test.ts @@ -31,13 +31,21 @@ vi.mock('../../src/nest/weather/weather.impl', async (importActual) => { import { WeatherModule } from '../../src/nest/weather/weather.module'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; import { TestUnitOfWorkModule } from '../helpers/test-uow'; +import { createTestMikroOrmModule } from '../helpers/test-orm'; describe('Weather e2e (real auth guard + temp SQLite)', () => { let server: Server; let app: Awaited>; async function build() { - const moduleRef = await Test.createTestingModule({ imports: [await TestUnitOfWorkModule.forRoot(db), WeatherModule] }).compile(); + // JwtAuthGuard (Plan 3b Task 1) injects EntityManager — needs + // MikroOrmModule.forRoot in the graph, same as every other e2e harness + // guarding a route with it; this suite's minimal hand-rolled `users` + // table already carries the five columns + // `findByIdWithPasswordVersion` selects. + const moduleRef = await Test.createTestingModule({ + imports: [await TestUnitOfWorkModule.forRoot(db), await createTestMikroOrmModule(db), WeatherModule], + }).compile(); const nest = moduleRef.createNestApplication(); nest.use(cookieParser()); nest.useGlobalFilters(new TrekExceptionFilter()); diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 66dfb48b2..49d162d83 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -148,7 +148,7 @@ export async function createMcpTestRegistry(): Promise { dbService, permissionsService, new TripMembershipService(dbService), - new WebauthnConfigService(dbService), + new WebauthnConfigService(appSettings), new UserCleanupService(dbService, budgetService, await createTestUnitOfWork(dbService.connection)), new MailerService(dbService), new EphemeralTokenService(), @@ -201,7 +201,7 @@ export async function createMcpTestRegistry(): Promise { const readModelService = new TripReadModelService( dbService, membersService, daysService, accommodationsService, budgetService, packingService, reservationsService, collabService, placesService, todoService, - new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService(), generalStorage), + new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService(), generalStorage, mcpOrm.em), ); const calendarService = new CalendarService(dbService, reservationsService); // The nine addon-gated surfaces read their toggle off an injected service now @@ -233,7 +233,7 @@ export async function createMcpTestRegistry(): Promise { new DayNotesMcp(new DayNotesService(dbService, permissionsService, realtimeService), authService, guards), new DaysMcp(daysService, authService, guards), new RoadtripMcp(new RoadtripService(dbService, realtimeService, await createTestUnitOfWork(dbService.connection)), dbService, guards, authService, addonsService), - new FilesMcp(new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService(), generalStorage), authService, guards), + new FilesMcp(new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService(), generalStorage, mcpOrm.em), authService, guards), new AccommodationsMcp(accommodationsService, dbService, placesService, authService, guards, await createTestUnitOfWork(dbService.connection)), new AssignmentsMcp(assignmentsService, daysService, authService, guards), new CollabMcp(collabService, authService, addonsService, guards), diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index 9627ba4bc..6dddd7226 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -99,7 +99,7 @@ export async function createPluginRpcHostFactory(dbs: DatabaseService): Promise< const queryHelpers = new QueryHelpersService(dbs); const todos = new TodoService(dbs, permissions, realtime, await createTestUnitOfWork(dbs.connection)); const packing = new PackingService(dbs, permissions, realtime, notificationsStub(), await createTestUnitOfWork(dbs.connection)); - const files = new FilesService(dbs, permissions, realtime, new EphemeralTokenService(), generalStorage); + const files = new FilesService(dbs, permissions, realtime, new EphemeralTokenService(), generalStorage, (await sharedTestOrm(dbs.connection)).em); const collab = new CollabService(dbs, permissions, realtime, notificationsStub(), generalStorage, new RateLimitService(), await createTestUnitOfWork(dbs.connection)); const vacay = new VacayService(dbs, realtime, notificationsStub(), await createTestUnitOfWork(dbs.connection)); const days = new DaysService(dbs, permissions, realtime, queryHelpers, await createTestUnitOfWork(dbs.connection)); diff --git a/server/tests/unit/db/dialect/sql-functions.test.ts b/server/tests/unit/db/dialect/sql-functions.test.ts index a21901cd8..e8e6f8413 100644 --- a/server/tests/unit/db/dialect/sql-functions.test.ts +++ b/server/tests/unit/db/dialect/sql-functions.test.ts @@ -5,7 +5,7 @@ import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; import { createUser } from '../../../helpers/factories'; import { Users } from '../../../../src/db/entities/Users.entity'; -import { columnIncrementedBy, columnRef, currentTimestamp, dateAdd, dateOf } from '../../../../src/db/dialect/sql-functions'; +import { columnIncrementedBy, columnRef, currentTimestamp, dateAdd, dateOf, lower } from '../../../../src/db/dialect/sql-functions'; const testDb = createSnapshotTestDb(); let t: TestOrm; @@ -73,6 +73,32 @@ describe('sql-functions (sqlite)', () => { expect(() => currentTimestamp(foreign)).toThrow(/no implementation for platform FakePlatform/); expect(() => columnRef(foreign, 'u.max_uses')).toThrow(/no implementation for platform FakePlatform/); expect(() => columnIncrementedBy(foreign, 'u.used_count', 1)).toThrow(/no implementation for platform FakePlatform/); + expect(() => lower(foreign, 'u.email')).toThrow(/no implementation for platform FakePlatform/); + }); + + // Plan 3b Task 1: UsersRepository's CI (case-insensitive) lookups + // (findByEmailCI, findIdByUsernameCI, ...) need `LOWER(col) = ?` as a + // `find`/`findOne` filter KEY, not just inside a QueryBuilder `.where()` — + // proving it works through `em.find`/`findOne` (the API the repository + // actually calls) rather than only through `createQueryBuilder`. + it('SQLF-012: lower is usable as a find() filter key, matching case-insensitively', async () => { + const { user } = createUser(testDb, { email: 'Mixed.Case@Example.com' }); + const rows = await t.em.find(Users, { [lower(t.em.getPlatform(), 'email')]: 'mixed.case@example.com' }); + expect(rows.map((r) => r.id)).toEqual([user.id]); + + // A differently-cased query value that still lowercases to the same + // string finds the same row; one that does not, finds nothing. + const none = await t.em.find(Users, { [lower(t.em.getPlatform(), 'email')]: 'nobody@example.com' }); + expect(none).toEqual([]); + }); + + it('SQLF-013: lower composes with other filter keys in one findOne call', async () => { + const { user } = createUser(testDb, { username: 'MixedCaseName' }); + const row = await t.em.findOne(Users, { + [lower(t.em.getPlatform(), 'username')]: 'mixedcasename', + id: { $ne: -1 }, + }); + expect(row?.id).toBe(user.id); }); // Plan 3b Task 0: InviteTokensRepository.incrementUsedCount needs a diff --git a/server/tests/unit/db/repositories/InviteTokens.repository.test.ts b/server/tests/unit/db/repositories/InviteTokens.repository.test.ts index fe2fba43e..1fb4b7526 100644 --- a/server/tests/unit/db/repositories/InviteTokens.repository.test.ts +++ b/server/tests/unit/db/repositories/InviteTokens.repository.test.ts @@ -22,15 +22,15 @@ function rawInvite(id: number): unknown { } describe('InviteTokensRepository', () => { - describe('findValid', () => { - it('INVREPO-001: a row read through findValid is the SELECT * row, key for key', async () => { + describe('findByToken', () => { + it('INVREPO-001: a row read through findByToken is the SELECT * row, key for key', async () => { const created = createInviteToken(testDb, { token: 'find-me' }); - const row = await invites.findValid('find-me'); + const row = await invites.findByToken('find-me'); expect(row).toStrictEqual(rawInvite(created.id)); }); it('INVREPO-002: an unknown token is null', async () => { - expect(await invites.findValid('does-not-exist')).toBeNull(); + expect(await invites.findByToken('does-not-exist')).toBeNull(); }); }); diff --git a/server/tests/unit/db/repositories/Users.repository.test.ts b/server/tests/unit/db/repositories/Users.repository.test.ts index bd4aa529b..4a651e1d7 100644 --- a/server/tests/unit/db/repositories/Users.repository.test.ts +++ b/server/tests/unit/db/repositories/Users.repository.test.ts @@ -2,7 +2,7 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vites import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; -import { createUser } from '../../../helpers/factories'; +import { createUser, createAdmin } from '../../../helpers/factories'; import { Users } from '../../../../src/db/entities/Users.entity'; import type { UsersRepository } from '../../../../src/db/repositories/Users.repository'; @@ -94,4 +94,451 @@ describe('UsersRepository', () => { expect(queries).toBe(1); }); }); + + // --------------------------------------------------------------------- + // Plan 3b Task 1 — every users-table method the auth/oidc/oauth/tokens + // cluster needs (see Users.repository.ts for the legacy-statement mapping + // per method). One `it` (or a focused group) per method, on real rows. + // --------------------------------------------------------------------- + + it('USERSREPO-009: findByIdWithPasswordVersion (JV1) reads id/username/email/role/password_version', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET password_version = 3 WHERE id = ?').run(user.id); + const row = await users.findByIdWithPasswordVersion(user.id); + expect(row).toEqual({ id: user.id, username: user.username, email: user.email, role: 'user', password_version: 3 }); + expect(await users.findByIdWithPasswordVersion(999999)).toBeNull(); + }); + + it('USERSREPO-010: getPasswordVersion reads password_version, null for a missing user', async () => { + const { user } = createUser(testDb); + expect(await users.getPasswordVersion(user.id)).toBe(0); + testDb.prepare('UPDATE users SET password_version = 7 WHERE id = ?').run(user.id); + expect(await users.getPasswordVersion(user.id)).toBe(7); + expect(await users.getPasswordVersion(999999)).toBeNull(); + }); + + describe('countNonGuest (AU4/AU7/O10) — is_guest is INTEGER NOT NULL DEFAULT 0, never observed NULL', () => { + it('USERSREPO-011: counts only non-guest users', async () => { + createUser(testDb); + createUser(testDb); + const { user: guest } = createUser(testDb); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + expect(await users.countNonGuest()).toBe(2); + }); + + it('USERSREPO-012: a NULL is_guest row cannot exist — the column is NOT NULL DEFAULT 0 (verified against the baseline + guest-members migrations), so a direct INSERT that omits the column, or an explicit NULL, both resolve to 0, never leaving a row this filter would miscount', () => { + testDb.prepare('INSERT INTO users (username, email, password_hash, role) VALUES (?, ?, ?, ?)').run('nog', 'nog@test.example.com', 'x', 'user'); + const stored = testDb.prepare("SELECT is_guest FROM users WHERE username = 'nog'").get() as { is_guest: number }; + expect(stored.is_guest).toBe(0); // the DEFAULT 0, not a nullable column silently coercing + expect(() => testDb.prepare('UPDATE users SET is_guest = NULL WHERE username = ?').run('nog')).toThrow(/NOT NULL constraint failed/); + }); + }); + + it('USERSREPO-013: findAdminNeedingPasswordChange finds an admin flagged must_change_password, ignores a matching non-admin or an unflagged admin', async () => { + expect(await users.findAdminNeedingPasswordChange()).toBeNull(); + const { user: plainAdmin } = createAdmin(testDb); + expect(await users.findAdminNeedingPasswordChange()).toBeNull(); + const { user: flaggedUser } = createUser(testDb); + testDb.prepare('UPDATE users SET must_change_password = 1 WHERE id = ?').run(flaggedUser.id); + expect(await users.findAdminNeedingPasswordChange()).toBeNull(); // not an admin + testDb.prepare('UPDATE users SET must_change_password = 1 WHERE id = ?').run(plainAdmin.id); + expect(await users.findAdminNeedingPasswordChange()).toBe(plainAdmin.id); + }); + + describe('findIdByEmailOrUsernameCI (AU9)', () => { + it('USERSREPO-014: matches by email OR username, case-insensitively, excluding guests', async () => { + const { user } = createUser(testDb, { email: 'Someone@Example.com', username: 'SomeoneName' }); + expect(await users.findIdByEmailOrUsernameCI('someone@example.com', 'no-match')).toBe(user.id); + expect(await users.findIdByEmailOrUsernameCI('no-match@example.com', 'someonename')).toBe(user.id); + expect(await users.findIdByEmailOrUsernameCI('nope@example.com', 'nope')).toBeNull(); + }); + + it('USERSREPO-015: excludes a guest even on an exact match', async () => { + const { user } = createUser(testDb, { email: 'guest1@example.com', username: 'guest1' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + expect(await users.findIdByEmailOrUsernameCI('guest1@example.com', 'guest1')).toBeNull(); + }); + }); + + describe('insertUser (AU10/O14)', () => { + it('USERSREPO-016: local registration shape (AU10) — no oidc/avatar columns', async () => { + const row = await users.insertUser({ + username: 'localuser', + email: 'local@example.com', + password_hash: 'hashed', + role: 'user', + first_seen_version: '1.2.3', + }); + expect(row.username).toBe('localuser'); + expect(row.email).toBe('local@example.com'); + expect(row.password_hash).toBe('hashed'); + expect(row.role).toBe('user'); + expect(row.first_seen_version).toBe('1.2.3'); + expect(row.login_count).toBe(0); + expect(row.oidc_sub).toBeNull(); + expect(row.oidc_issuer).toBeNull(); + expect(row.avatar).toBeNull(); + expect(row.created_at).not.toBeNull(); + const raw = testDb.prepare('SELECT * FROM users WHERE id = ?').get(row.id); + expect(raw).toMatchObject({ username: 'localuser', email: 'local@example.com', role: 'user', login_count: 0 }); + }); + + it('USERSREPO-017: SSO registration shape (O14) — oidc_sub/oidc_issuer/avatar set', async () => { + const row = await users.insertUser({ + username: 'ssouser', + email: 'sso@example.com', + password_hash: 'unusable-hash', + role: 'admin', + first_seen_version: '1.2.3', + oidc_sub: 'sub-123', + oidc_issuer: 'https://idp.example.com', + avatar: 'https://idp.example.com/pic.png', + }); + expect(row.oidc_sub).toBe('sub-123'); + expect(row.oidc_issuer).toBe('https://idp.example.com'); + expect(row.avatar).toBe('https://idp.example.com/pic.png'); + expect(row.role).toBe('admin'); + expect(row.login_count).toBe(0); + }); + }); + + describe('findByEmailCI (AU12/O4)', () => { + it('USERSREPO-018: matches case-insensitively, excludes guests, returns the full row', async () => { + const { user } = createUser(testDb, { email: 'Case.Test@Example.com' }); + const row = await users.findByEmailCI('case.test@example.com'); + expect(row?.id).toBe(user.id); + expect(row?.username).toBe(user.username); + expect(await users.findByEmailCI('nope@example.com')).toBeNull(); + }); + + it('USERSREPO-019: a guest with a matching email is not returned', async () => { + const { user } = createUser(testDb, { email: 'guest2@example.com' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + expect(await users.findByEmailCI('guest2@example.com')).toBeNull(); + }); + }); + + describe('touchLastLogin (AU13/AU34/AU35/PK12/O16)', () => { + it('USERSREPO-020: bumps login_count by exactly 1 and stamps last_login, in one statement', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET login_count = 5 WHERE id = ?').run(user.id); + await users.touchLastLogin(user.id); + const row = testDb.prepare('SELECT login_count, last_login FROM users WHERE id = ?').get(user.id) as { login_count: number; last_login: string }; + expect(row.login_count).toBe(6); + expect(row.last_login).toMatch(/^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}$/); + }); + + it('USERSREPO-021: two concurrent calls both land — +2, never +1 (no read-modify-write)', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET login_count = 10 WHERE id = ?').run(user.id); + await Promise.all([users.touchLastLogin(user.id), users.touchLastLogin(user.id)]); + const row = testDb.prepare('SELECT login_count FROM users WHERE id = ?').get(user.id) as { login_count: number }; + expect(row.login_count).toBe(12); + }); + }); + + it('USERSREPO-022: findMeRow (AU14) reads the /api/auth/me projection', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET avatar = ?, mfa_enabled = 1, must_change_password = 1 WHERE id = ?').run('pic.png', user.id); + const row = await users.findMeRow(user.id); + expect(row).toMatchObject({ id: user.id, username: user.username, email: user.email, role: 'user', avatar: 'pic.png', mfa_enabled: 1, must_change_password: 1 }); + expect(row?.created_at).not.toBeNull(); + expect(await users.findMeRow(999999)).toBeNull(); + }); + + it('USERSREPO-023: getPasswordHashAndVersion (AU15) reads both columns together', async () => { + const { user, password: _password } = createUser(testDb); + testDb.prepare('UPDATE users SET password_version = 2 WHERE id = ?').run(user.id); + const row = await users.getPasswordHashAndVersion(user.id); + expect(row?.password_hash).toBe(user.password_hash); + expect(row?.password_version).toBe(2); + }); + + it('USERSREPO-024: setPassword (AU16/AU43) writes hash+version, clears must_change_password, stamps updated_at', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET must_change_password = 1 WHERE id = ?').run(user.id); + await users.setPassword(user.id, 'new-hash', 5); + const row = testDb.prepare('SELECT password_hash, password_version, must_change_password, updated_at FROM users WHERE id = ?').get(user.id) as { + password_hash: string; password_version: number; must_change_password: number; updated_at: string; + }; + expect(row.password_hash).toBe('new-hash'); + expect(row.password_version).toBe(5); + expect(row.must_change_password).toBe(0); + expect(row.updated_at).toMatch(/^\d{4}-\d{2}-\d{2}/); + }); + + it('USERSREPO-025: countAdmins (AU19/O7) counts only role=admin', async () => { + createUser(testDb); + createAdmin(testDb); + createAdmin(testDb); + expect(await users.countAdmins()).toBe(2); + }); + + it('USERSREPO-026: getRole (AU20/AU22) reads the role column', async () => { + const { user } = createAdmin(testDb); + expect(await users.getRole(user.id)).toBe('admin'); + expect(await users.getRole(999999)).toBeNull(); + }); + + describe('getMfaEnabled (AU23/AU27/MP2) — distinguishes "no row" from "row present, mfa_enabled unset"', () => { + it('USERSREPO-027: returns the row wrapper when the user exists', async () => { + const { user } = createUser(testDb); + expect(await users.getMfaEnabled(user.id)).toEqual({ mfa_enabled: 0 }); + testDb.prepare('UPDATE users SET mfa_enabled = 1 WHERE id = ?').run(user.id); + expect(await users.getMfaEnabled(user.id)).toEqual({ mfa_enabled: 1 }); + }); + + it('USERSREPO-028: returns null (not a row with a null field) when the user does not exist', async () => { + expect(await users.getMfaEnabled(999999)).toBeNull(); + }); + }); + + it('USERSREPO-029: enableMfa (AU28) sets mfa_enabled=1, writes the already-encrypted secret and codes, stamps updated_at', async () => { + const { user } = createUser(testDb); + await users.enableMfa(user.id, 'encrypted-secret', '["hash1","hash2"]'); + const row = testDb.prepare('SELECT mfa_enabled, mfa_secret, mfa_backup_codes, updated_at FROM users WHERE id = ?').get(user.id) as { + mfa_enabled: number; mfa_secret: string; mfa_backup_codes: string; updated_at: string; + }; + expect(row.mfa_enabled).toBe(1); + expect(row.mfa_secret).toBe('encrypted-secret'); + expect(row.mfa_backup_codes).toBe('["hash1","hash2"]'); + expect(row.updated_at).toMatch(/^\d{4}-\d{2}-\d{2}/); + }); + + describe('findById (AU30/AU32/PK4/PK10/O15) — full row, PK-only findOne', () => { + it('USERSREPO-030: reads every scalar column', async () => { + const { user } = createUser(testDb); + const row = await users.findById(user.id); + expect(row?.id).toBe(user.id); + expect(row?.username).toBe(user.username); + expect(row?.password_hash).toBe(user.password_hash); + expect(await users.findById(999999)).toBeNull(); + }); + + it('USERSREPO-031: sees a raw UPDATE on the same id in the same request (I1, identity-map regression)', async () => { + const { user } = createUser(testDb, { username: 'before' }); + await users.findById(user.id); // populate the identity map + testDb.prepare('UPDATE users SET username = ? WHERE id = ?').run('after', user.id); + const { value, queries } = await withQueryCount(() => users.findById(user.id)); + expect(value?.username).toBe('after'); + expect(queries).toBe(1); + }); + }); + + it('USERSREPO-032: disableMfa (AU31) clears mfa_enabled/secret/codes, stamps updated_at', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET mfa_enabled = 1, mfa_secret = ?, mfa_backup_codes = ? WHERE id = ?').run('s', 'c', user.id); + await users.disableMfa(user.id); + const row = testDb.prepare('SELECT mfa_enabled, mfa_secret, mfa_backup_codes, updated_at FROM users WHERE id = ?').get(user.id) as { + mfa_enabled: number; mfa_secret: string | null; mfa_backup_codes: string | null; updated_at: string; + }; + expect(row.mfa_enabled).toBe(0); + expect(row.mfa_secret).toBeNull(); + expect(row.mfa_backup_codes).toBeNull(); + expect(row.updated_at).toMatch(/^\d{4}-\d{2}-\d{2}/); + }); + + describe('setBackupCodesAndTouch (AU33) vs setBackupCodes (AU44) — one stamps updated_at, the other does not', () => { + it('USERSREPO-033: setBackupCodesAndTouch writes codes and stamps updated_at', async () => { + const { user } = createUser(testDb); + testDb.prepare("UPDATE users SET updated_at = '2020-01-01 00:00:00' WHERE id = ?").run(user.id); + await users.setBackupCodesAndTouch(user.id, '["a"]'); + const row = testDb.prepare('SELECT mfa_backup_codes, updated_at FROM users WHERE id = ?').get(user.id) as { mfa_backup_codes: string; updated_at: string }; + expect(row.mfa_backup_codes).toBe('["a"]'); + expect(row.updated_at).not.toBe('2020-01-01 00:00:00'); + }); + + it('USERSREPO-034: setBackupCodes writes codes WITHOUT touching updated_at', async () => { + const { user } = createUser(testDb); + testDb.prepare("UPDATE users SET updated_at = '2020-01-01 00:00:00' WHERE id = ?").run(user.id); + await users.setBackupCodes(user.id, '["b"]'); + const row = testDb.prepare('SELECT mfa_backup_codes, updated_at FROM users WHERE id = ?').get(user.id) as { mfa_backup_codes: string; updated_at: string }; + expect(row.mfa_backup_codes).toBe('["b"]'); + expect(row.updated_at).toBe('2020-01-01 00:00:00'); + }); + }); + + describe('findForPasswordReset (AU36) — case-SENSITIVE email, unlike every other email lookup here', () => { + it('USERSREPO-035: matches the exact case only', async () => { + const { user } = createUser(testDb, { email: 'Exact@Example.com' }); + const row = await users.findForPasswordReset('Exact@Example.com'); + expect(row?.id).toBe(user.id); + expect(await users.findForPasswordReset('exact@example.com')).toBeNull(); // no LOWER() in the legacy statement + }); + + it('USERSREPO-036: excludes a guest', async () => { + const { user } = createUser(testDb, { email: 'guest3@example.com' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + expect(await users.findForPasswordReset('guest3@example.com')).toBeNull(); + }); + }); + + it('USERSREPO-037: findResetTarget (AU40) reads the reset-branch projection', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET mfa_enabled = 1, mfa_secret = ?, mfa_backup_codes = ?, password_version = 4 WHERE id = ?').run('s', 'c', user.id); + const row = await users.findResetTarget(user.id); + expect(row).toEqual({ id: user.id, email: user.email, mfa_enabled: 1, mfa_secret: 's', mfa_backup_codes: 'c', password_version: 4 }); + }); + + it('USERSREPO-038: getPasswordHash (PK15) reads password_hash', async () => { + const { user } = createUser(testDb); + expect(await users.getPasswordHash(user.id)).toBe(user.password_hash); + expect(await users.getPasswordHash(999999)).toBeNull(); + }); + + it('USERSREPO-039: findIdAndEmail (PK17) reads id+email', async () => { + const { user } = createUser(testDb); + expect(await users.findIdAndEmail(user.id)).toEqual({ id: user.id, email: user.email }); + }); + + describe('findByOidcIdentity (O3)', () => { + it('USERSREPO-040: matches on the (sub, issuer) pair', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ? WHERE id = ?').run('sub-1', 'https://idp.example.com', user.id); + const row = await users.findByOidcIdentity('sub-1', 'https://idp.example.com'); + expect(row?.id).toBe(user.id); + expect(await users.findByOidcIdentity('sub-1', 'https://other-idp.example.com')).toBeNull(); + }); + }); + + it('USERSREPO-041: linkOidcIdentity (O5/O6) writes sub+issuer WITHOUT touching updated_at', async () => { + const { user } = createUser(testDb); + testDb.prepare("UPDATE users SET updated_at = '2020-01-01 00:00:00' WHERE id = ?").run(user.id); + await users.linkOidcIdentity(user.id, 'sub-2', 'https://idp2.example.com'); + const row = testDb.prepare('SELECT oidc_sub, oidc_issuer, updated_at FROM users WHERE id = ?').get(user.id) as { + oidc_sub: string; oidc_issuer: string; updated_at: string; + }; + expect(row.oidc_sub).toBe('sub-2'); + expect(row.oidc_issuer).toBe('https://idp2.example.com'); + expect(row.updated_at).toBe('2020-01-01 00:00:00'); + }); + + it('USERSREPO-042: setRole (O8) writes role WITHOUT touching updated_at', async () => { + const { user } = createUser(testDb); + testDb.prepare("UPDATE users SET updated_at = '2020-01-01 00:00:00' WHERE id = ?").run(user.id); + await users.setRole(user.id, 'admin'); + const row = testDb.prepare('SELECT role, updated_at FROM users WHERE id = ?').get(user.id) as { role: string; updated_at: string }; + expect(row.role).toBe('admin'); + expect(row.updated_at).toBe('2020-01-01 00:00:00'); + }); + + it('USERSREPO-043: setAvatarRaw (O9) writes avatar WITHOUT touching updated_at, unlike setAvatar', async () => { + const { user } = createUser(testDb); + testDb.prepare("UPDATE users SET updated_at = '2020-01-01 00:00:00' WHERE id = ?").run(user.id); + await users.setAvatarRaw(user.id, 'https://idp.example.com/pic.png'); + const row = testDb.prepare('SELECT avatar, updated_at FROM users WHERE id = ?').get(user.id) as { avatar: string; updated_at: string }; + expect(row.avatar).toBe('https://idp.example.com/pic.png'); + expect(row.updated_at).toBe('2020-01-01 00:00:00'); + }); + + describe('findIdByUsernameCI (UP5) vs findIdByUsernameCIAny (O12) — different WHERE, two methods', () => { + it('USERSREPO-044: findIdByUsernameCI excludes the given id and any guest', async () => { + const { user: a } = createUser(testDb, { username: 'DupeName' }); + const { user: b } = createUser(testDb, { username: 'other' }); + expect(await users.findIdByUsernameCI('dupename', b.id)).toBe(a.id); + expect(await users.findIdByUsernameCI('dupename', a.id)).toBeNull(); // excludes self + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(a.id); + expect(await users.findIdByUsernameCI('dupename', b.id)).toBeNull(); // guest excluded + }); + + it('USERSREPO-045: findIdByUsernameCIAny has no exclude and no guest filter', async () => { + const { user } = createUser(testDb, { username: 'AnyName' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + expect(await users.findIdByUsernameCIAny('anyname')).toBe(user.id); // a guest still matches here + }); + }); + + it('USERSREPO-046: getApiKeyColumns (UP1) reads role + the four key columns', async () => { + const { user } = createAdmin(testDb); + testDb.prepare('UPDATE users SET maps_api_key = ?, openweather_api_key = ?, unsplash_api_key = ?, amap_api_key = ? WHERE id = ?') + .run('m', 'o', 'u', 'a', user.id); + expect(await users.getApiKeyColumns(user.id)).toEqual({ role: 'admin', maps_api_key: 'm', openweather_api_key: 'o', unsplash_api_key: 'u', amap_api_key: 'a' }); + }); + + it('USERSREPO-047: updateMapsKey (UP2) writes maps_api_key + updated_at', async () => { + const { user } = createUser(testDb); + await users.updateMapsKey(user.id, 'encrypted'); + const row = testDb.prepare('SELECT maps_api_key, updated_at FROM users WHERE id = ?').get(user.id) as { maps_api_key: string; updated_at: string }; + expect(row.maps_api_key).toBe('encrypted'); + expect(row.updated_at).toMatch(/^\d{4}-\d{2}-\d{2}/); + }); + + it('USERSREPO-048: updateApiKeys (UP3) writes all four columns + updated_at in one statement', async () => { + const { user } = createUser(testDb); + await users.updateApiKeys(user.id, { maps_api_key: 'm2', openweather_api_key: 'o2', unsplash_api_key: 'u2', amap_api_key: null }); + const row = testDb.prepare('SELECT maps_api_key, openweather_api_key, unsplash_api_key, amap_api_key, updated_at FROM users WHERE id = ?').get(user.id) as Record; + expect(row).toMatchObject({ maps_api_key: 'm2', openweather_api_key: 'o2', unsplash_api_key: 'u2', amap_api_key: null }); + expect(row.updated_at).toMatch(/^\d{4}-\d{2}-\d{2}/); + }); + + it('USERSREPO-049: findProfileWithKeys (UP4/UP8) reads the profile-with-keys projection', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET maps_api_key = ?, mfa_enabled = 1 WHERE id = ?').run('m', user.id); + const row = await users.findProfileWithKeys(user.id); + expect(row).toMatchObject({ id: user.id, username: user.username, email: user.email, role: 'user', maps_api_key: 'm', mfa_enabled: 1 }); + }); + + it('USERSREPO-050: findIdByEmailCI (UP6) matches case-insensitively, excludes self and guests', async () => { + const { user: a } = createUser(testDb, { email: 'Dup@Example.com' }); + const { user: b } = createUser(testDb, { email: 'other@example.com' }); + expect(await users.findIdByEmailCI('dup@example.com', b.id)).toBe(a.id); + expect(await users.findIdByEmailCI('dup@example.com', a.id)).toBeNull(); + }); + + it('USERSREPO-051: patchProfile (UP7) writes only the given bounded columns + updated_at, in one statement', async () => { + const { user } = createUser(testDb, { username: 'patchme', email: 'patchme@example.com' }); + await users.patchProfile(user.id, { username: 'patched', maps_api_key: 'newkey' }); + const row = testDb.prepare('SELECT username, email, maps_api_key, updated_at FROM users WHERE id = ?').get(user.id) as Record; + expect(row.username).toBe('patched'); + expect(row.email).toBe('patchme@example.com'); // untouched — not in `changes` + expect(row.maps_api_key).toBe('newkey'); + expect(row.updated_at).toMatch(/^\d{4}-\d{2}-\d{2}/); + }); + + describe('getAvatar (UP9/UP12) / setAvatar (UP10/UP13)', () => { + it('USERSREPO-052: getAvatar reads the avatar column', async () => { + const { user } = createUser(testDb); + expect(await users.getAvatar(user.id)).toBeNull(); + testDb.prepare('UPDATE users SET avatar = ? WHERE id = ?').run('pic.png', user.id); + expect(await users.getAvatar(user.id)).toBe('pic.png'); + }); + + it('USERSREPO-053: setAvatar writes avatar + updated_at (both a filename and NULL for delete)', async () => { + const { user } = createUser(testDb); + await users.setAvatar(user.id, 'new.png'); + let row = testDb.prepare('SELECT avatar, updated_at FROM users WHERE id = ?').get(user.id) as { avatar: string; updated_at: string }; + expect(row.avatar).toBe('new.png'); + expect(row.updated_at).toMatch(/^\d{4}-\d{2}-\d{2}/); + + testDb.prepare("UPDATE users SET updated_at = '2020-01-01 00:00:00' WHERE id = ?").run(user.id); + await users.setAvatar(user.id, null); + row = testDb.prepare('SELECT avatar, updated_at FROM users WHERE id = ?').get(user.id) as { avatar: string | null; updated_at: string }; + expect(row.avatar).toBeNull(); + expect(row.updated_at).not.toBe('2020-01-01 00:00:00'); + }); + }); + + it('USERSREPO-054: findProfileBasic (UP11) reads id/username/email/role/avatar', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET avatar = ? WHERE id = ?').run('a.png', user.id); + expect(await users.findProfileBasic(user.id)).toEqual({ id: user.id, username: user.username, email: user.email, role: 'user', avatar: 'a.png' }); + }); + + describe('listOthersNonGuest (UP14)', () => { + it('USERSREPO-055: excludes the given id and every guest, ordered by username', async () => { + const { user: me } = createUser(testDb, { username: 'me' }); + const { user: bravo } = createUser(testDb, { username: 'bravo' }); + const { user: alpha } = createUser(testDb, { username: 'alpha' }); + const { user: guest } = createUser(testDb, { username: 'zzz-guest' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(guest.id); + const rows = await users.listOthersNonGuest(me.id); + expect(rows.map((r) => r.id)).toEqual([alpha.id, bravo.id]); + }); + }); + + it('USERSREPO-056: getRoleAndWeatherKey (UP15) reads role + openweather_api_key', async () => { + const { user } = createAdmin(testDb); + testDb.prepare('UPDATE users SET openweather_api_key = ? WHERE id = ?').run('w', user.id); + expect(await users.getRoleAndWeatherKey(user.id)).toEqual({ role: 'admin', openweather_api_key: 'w' }); + }); }); diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index 0b817188d..df4eccd9a 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -87,7 +87,7 @@ import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); -const webauthn = new WebauthnConfigService(dbs); +let webauthn: WebauthnConfigService; // 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 @@ -99,6 +99,7 @@ let userCleanup: UserCleanupService; let auth: AuthService; let svc: AdminService; beforeAll(async () => { + webauthn = new WebauthnConfigService(await createTestAppSettingsRepo(dbs.connection)); permissions = new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)); userCleanup = new UserCleanupService(dbs, new BudgetService(dbs, permissions, new ExchangeRatesService(), realtime, await createTestUnitOfWork(dbs.connection)), await createTestUnitOfWork(dbs.connection)); auth = new AuthService(dbs, permissions, new TripMembershipService(dbs), webauthn, userCleanup, new MailerService(dbs), new EphemeralTokenService(), new AllowedFileTypesService(dbs), await createTestUnitOfWork(dbs.connection), await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection)); diff --git a/server/tests/unit/nest/admin.version-notification.test.ts b/server/tests/unit/nest/admin.version-notification.test.ts index cd16e151e..abca9725e 100644 --- a/server/tests/unit/nest/admin.version-notification.test.ts +++ b/server/tests/unit/nest/admin.version-notification.test.ts @@ -65,7 +65,7 @@ import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); -const webauthn = new WebauthnConfigService(dbs); +let webauthn: WebauthnConfigService; // 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 @@ -77,6 +77,7 @@ let userCleanup: UserCleanupService; let auth: AuthService; let svc: AdminService; beforeAll(async () => { + webauthn = new WebauthnConfigService(await createTestAppSettingsRepo(dbs.connection)); permissions = new PermissionsService(await createTestAppSettingsRepo(dbs.connection), await createTestUnitOfWork(dbs.connection)); userCleanup = new UserCleanupService(dbs, new BudgetService(dbs, permissions, new ExchangeRatesService(), realtime, await createTestUnitOfWork(dbs.connection)), await createTestUnitOfWork(dbs.connection)); auth = new AuthService(dbs, permissions, new TripMembershipService(dbs), webauthn, userCleanup, new MailerService(dbs), new EphemeralTokenService(), new AllowedFileTypesService(dbs), await createTestUnitOfWork(dbs.connection), await createTestAppSettingsRepo(dbs.connection), await createTestUsersRepo(dbs.connection)); diff --git a/server/tests/unit/nest/auth-guard.test.ts b/server/tests/unit/nest/auth-guard.test.ts index b53ce60d7..6cc6e57a1 100644 --- a/server/tests/unit/nest/auth-guard.test.ts +++ b/server/tests/unit/nest/auth-guard.test.ts @@ -7,6 +7,7 @@ vi.mock('../../../src/nest/common/cookie', () => ({ setAuthCookie: vi.fn() })); vi.mock('../../../src/nest/audit/client-ip', () => ({ getClientIp: vi.fn(() => '1.2.3.4') })); vi.mock('../../../src/nest/audit/audit-log.logger', () => ({ LOG_LEVEL: 'error', logInfo: vi.fn(), logDebug: vi.fn(), logError: vi.fn(), logWarn: vi.fn() })); +import type { EntityManager } from '@mikro-orm/core'; import { JwtAuthGuard } from '../../../src/nest/auth/jwt-auth.guard'; import { CookieAuthGuard } from '../../../src/nest/auth/cookie-auth.guard'; import { OptionalJwtGuard } from '../../../src/nest/auth/optional-jwt.guard'; @@ -41,6 +42,13 @@ import type { User } from '../../../src/types'; const user = { id: 1, username: 'u', role: 'user', email: 'u@example.test' } as User; +// JwtAuthGuard/CookieAuthGuard/OptionalJwtGuard now inject EntityManager +// (Plan 3b Task 1 RULING) rather than reading the legacy `db` proxy through +// the mocked jwt-verify.ts — verifyJwtAndLoadUser itself is fully mocked +// above, so `em.getRepository` never needs to return anything meaningful; +// it just has to not throw when the guard calls it. +const emStub = { getRepository: () => ({}) } as unknown as EntityManager; + function context(req: unknown) { return { switchToHttp: () => ({ getRequest: () => req }) } as never; } @@ -64,7 +72,7 @@ async function thrownAsync(fn: () => Promise): Promise<{ status: number beforeEach(() => vi.clearAllMocks()); describe('JwtAuthGuard', () => { - const guard = new JwtAuthGuard(); + const guard = new JwtAuthGuard(emStub); it('rejects with the legacy 401 { error, code } when no token is present', async () => { vi.mocked(extractToken).mockReturnValue(null); @@ -93,7 +101,7 @@ describe('JwtAuthGuard', () => { }); describe('CookieAuthGuard', () => { - const guard = new CookieAuthGuard(); + const guard = new CookieAuthGuard(emStub); it('401s when the trek_session cookie is missing', async () => { expect(await thrownAsync(() => guard.canActivate(context({ cookies: {} })))).toEqual({ @@ -124,7 +132,7 @@ describe('CookieAuthGuard', () => { }); describe('OptionalJwtGuard', () => { - const guard = new OptionalJwtGuard(); + const guard = new OptionalJwtGuard(emStub); it('always allows; sets req.user to null when no token', async () => { const req: Record = { headers: {} }; diff --git a/server/tests/unit/nest/auth.service.test.ts b/server/tests/unit/nest/auth.service.test.ts index 5cec768f2..f5e50c216 100644 --- a/server/tests/unit/nest/auth.service.test.ts +++ b/server/tests/unit/nest/auth.service.test.ts @@ -104,7 +104,7 @@ beforeAll(async () => { new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), membershipStub, - new WebauthnConfigService(new DatabaseService(testDb)), + new WebauthnConfigService(await createTestAppSettingsRepo(testDb)), new UserCleanupService(new DatabaseService(testDb), new BudgetService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new ExchangeRatesService(), new RealtimeService(), await createTestUnitOfWork(testDb)), await createTestUnitOfWork(testDb)), mailerStub, new EphemeralTokenService(), @@ -482,11 +482,11 @@ describe('changePassword — session invalidation', () => { const { user, password } = createUser(testDb); const stolen = await svc.generateToken({ id: user.id }); // pv=0 at mint time - expect(await verifyJwtAndLoadUser(stolen)).not.toBeNull(); + expect(await verifyJwtAndLoadUser(stolen, await createTestUsersRepo(testDb))).not.toBeNull(); await svc.changePassword(user.id, user.email, { current_password: password, new_password: 'New1234!' }); - expect(await verifyJwtAndLoadUser(stolen)).toBeNull(); // invalidated by the pv bump + expect(await verifyJwtAndLoadUser(stolen, await createTestUsersRepo(testDb))).toBeNull(); // invalidated by the pv bump }); it('AUTH-DB-036d: preserves the remember choice in the re-issued session (#1927)', async () => { diff --git a/server/tests/unit/nest/auth/jwt-verify.test.ts b/server/tests/unit/nest/auth/jwt-verify.test.ts index c795f4e3e..3ebf36d88 100644 --- a/server/tests/unit/nest/auth/jwt-verify.test.ts +++ b/server/tests/unit/nest/auth/jwt-verify.test.ts @@ -10,17 +10,21 @@ * The password_version gate gets direct cases here for the first time. It is the * reason this function exists at all: a reset bumps users.password_version and * every JWT carrying the prior value has to stop working. + * + * Plan 3b Task 1: `verifyJwtAndLoadUser` now takes an explicit `UsersRepository` + * parameter instead of reading the legacy `db` proxy — this file stubs + * `findByIdWithPasswordVersion` directly (a fake repository object, exactly + * the shape `Users.repository.test.ts` proves against real rows) rather than + * mocking `src/db/database`'s module-level `db` export, which this file no + * longer imports at all. */ import { describe, it, expect, vi, afterEach } from 'vitest'; import jwt from 'jsonwebtoken'; -vi.mock('../../../../src/db/database', () => ({ - db: { prepare: vi.fn(() => ({ get: vi.fn(), all: vi.fn() })) }, -})); vi.mock('../../../../src/config', () => ({ JWT_SECRET: 'test-secret' })); import { extractToken, verifyJwtAndLoadUser } from '../../../../src/nest/auth/jwt-verify'; -import { db } from '../../../../src/db/database'; +import type { UsersRepository, UserWithPasswordVersion } from '../../../../src/db/repositories/Users.repository'; import type { Request } from 'express'; function makeReq(overrides: { @@ -33,9 +37,9 @@ function makeReq(overrides: { } as unknown as Request; } -/** Stub the single users row the verify reads. */ -function userRow(row: Record | undefined): void { - vi.mocked(db.prepare).mockReturnValue({ get: vi.fn(() => row), all: vi.fn() } as never); +/** A fake `UsersRepository` whose `findByIdWithPasswordVersion` returns the given row. */ +function usersRepo(row: UserWithPasswordVersion | null): UsersRepository { + return { findByIdWithPasswordVersion: vi.fn(async () => row) } as unknown as UsersRepository; } afterEach(() => vi.clearAllMocks()); @@ -72,64 +76,66 @@ describe('extractToken', () => { describe('verifyJwtAndLoadUser', () => { it('AUTH-JWT-001: returns the user for a valid token, without password_version', async () => { - userRow({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 0 }); + const users = usersRepo({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 0 }); const token = jwt.sign({ id: 1 }, 'test-secret', { algorithm: 'HS256' }); - expect(await verifyJwtAndLoadUser(token)).toEqual({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user' }); + expect(await verifyJwtAndLoadUser(token, users)).toEqual({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user' }); + expect(users.findByIdWithPasswordVersion).toHaveBeenCalledWith(1); }); it('AUTH-JWT-002: returns null for a malformed token', async () => { - expect(await verifyJwtAndLoadUser('invalid.jwt.token')).toBeNull(); + expect(await verifyJwtAndLoadUser('invalid.jwt.token', usersRepo(null))).toBeNull(); }); it('AUTH-JWT-003: returns null when the user no longer exists', async () => { - userRow(undefined); - expect(await verifyJwtAndLoadUser(jwt.sign({ id: 99999 }, 'test-secret', { algorithm: 'HS256' }))).toBeNull(); + const users = usersRepo(null); + expect(await verifyJwtAndLoadUser(jwt.sign({ id: 99999 }, 'test-secret', { algorithm: 'HS256' }), users)).toBeNull(); }); it('AUTH-JWT-004: returns null for an expired token', async () => { const expired = jwt.sign({ id: 1, exp: Math.floor(Date.now() / 1000) - 3600 }, 'test-secret', { algorithm: 'HS256' }); - expect(await verifyJwtAndLoadUser(expired)).toBeNull(); + expect(await verifyJwtAndLoadUser(expired, usersRepo(null))).toBeNull(); }); it('AUTH-JWT-005: returns null for a token signed with the wrong secret', async () => { - expect(await verifyJwtAndLoadUser(jwt.sign({ id: 1 }, 'wrong-secret', { algorithm: 'HS256' }))).toBeNull(); + expect(await verifyJwtAndLoadUser(jwt.sign({ id: 1 }, 'wrong-secret', { algorithm: 'HS256' }), usersRepo(null))).toBeNull(); }); it('AUTH-JWT-006: rejects a purpose-scoped mfa_login token even when the user is valid', async () => { // Issued after the password check but before TOTP, signed with the same // secret. It must never authenticate an ordinary request. - userRow({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 0 }); + const users = usersRepo({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 0 }); const mfaToken = jwt.sign({ id: 1, purpose: 'mfa_login' }, 'test-secret', { algorithm: 'HS256' }); - expect(await verifyJwtAndLoadUser(mfaToken)).toBeNull(); + expect(await verifyJwtAndLoadUser(mfaToken, users)).toBeNull(); + expect(users.findByIdWithPasswordVersion).not.toHaveBeenCalled(); }); it('AUTH-JWT-007: rejects a token whose password_version predates the user row', async () => { - userRow({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 2 }); + const users = usersRepo({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 2 }); const stale = jwt.sign({ id: 1, pv: 1 }, 'test-secret', { algorithm: 'HS256' }); - expect(await verifyJwtAndLoadUser(stale)).toBeNull(); + expect(await verifyJwtAndLoadUser(stale, users)).toBeNull(); }); it('AUTH-JWT-008: accepts a token whose password_version matches', async () => { - userRow({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 2 }); + const users = usersRepo({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 2 }); const current = jwt.sign({ id: 1, pv: 2 }, 'test-secret', { algorithm: 'HS256' }); - expect(await verifyJwtAndLoadUser(current)).not.toBeNull(); + expect(await verifyJwtAndLoadUser(current, users)).not.toBeNull(); }); it('AUTH-JWT-009: a pre-pv token still works against a never-reset user (both read as 0)', async () => { - userRow({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: null }); + const users = usersRepo({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 0 }); const legacy = jwt.sign({ id: 1 }, 'test-secret', { algorithm: 'HS256' }); - expect(await verifyJwtAndLoadUser(legacy)).not.toBeNull(); + expect(await verifyJwtAndLoadUser(legacy, users)).not.toBeNull(); }); it('AUTH-JWT-010: but a pre-pv token stops working once the user has reset', async () => { - userRow({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 1 }); + const users = usersRepo({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 1 }); const legacy = jwt.sign({ id: 1 }, 'test-secret', { algorithm: 'HS256' }); - expect(await verifyJwtAndLoadUser(legacy)).toBeNull(); + expect(await verifyJwtAndLoadUser(legacy, users)).toBeNull(); }); }); diff --git a/server/tests/unit/nest/files.service.test.ts b/server/tests/unit/nest/files.service.test.ts index dc4c69745..8f1cb235f 100644 --- a/server/tests/unit/nest/files.service.test.ts +++ b/server/tests/unit/nest/files.service.test.ts @@ -74,10 +74,15 @@ import { import type { TripFile, User } from '../../../src/types'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; +import type { EntityManager } from '@mikro-orm/core'; const storageDelete = vi.fn(); const storageStub = { delete: storageDelete } as unknown as import('../../../src/nest/storage/storage.service').StorageService; -const svc = new FilesService(new DatabaseService(testDb), permissionsStub, new RealtimeService(), new EphemeralTokenService(), storageStub); +// EntityManager stub (Plan 3b Task 1 RULING): verifyJwtAndLoadUser is +// fully mocked above, so `em.getRepository` never needs to return +// anything meaningful; it just has to not throw when the service calls it. +const emStub = { getRepository: () => ({}) } as unknown as EntityManager; +const svc = new FilesService(new DatabaseService(testDb), permissionsStub, new RealtimeService(), new EphemeralTokenService(), storageStub, emStub); beforeAll(() => { createTables(testDb); @@ -555,13 +560,13 @@ describe('authenticateDownload', () => { verifyJwtAndLoadUser.mockReturnValue({ id: 42 }); const result = await svc.authenticateDownload(req({ cookie: 'cookie-jwt', bearer: 'bearer-jwt' })); expect(result).toEqual({ userId: 42 }); - expect(verifyJwtAndLoadUser).toHaveBeenCalledWith('cookie-jwt'); + expect(verifyJwtAndLoadUser).toHaveBeenCalledWith('cookie-jwt', expect.anything()); }); it('FILE-SVC-033: a bearer token is used when no cookie is present; invalid JWTs 401', async () => { verifyJwtAndLoadUser.mockReturnValue({ id: 7 }); expect(await svc.authenticateDownload(req({ bearer: 'bearer-jwt' }))).toEqual({ userId: 7 }); - expect(verifyJwtAndLoadUser).toHaveBeenCalledWith('bearer-jwt'); + expect(verifyJwtAndLoadUser).toHaveBeenCalledWith('bearer-jwt', expect.anything()); verifyJwtAndLoadUser.mockReturnValue(null); expect(await svc.authenticateDownload(req({ bearer: 'stale' }))).toEqual({ error: 'Invalid or expired token', status: 401 }); diff --git a/server/tests/unit/nest/mfa-policy.guard.test.ts b/server/tests/unit/nest/mfa-policy.guard.test.ts index 4b25ebac4..62684f23d 100644 --- a/server/tests/unit/nest/mfa-policy.guard.test.ts +++ b/server/tests/unit/nest/mfa-policy.guard.test.ts @@ -6,13 +6,22 @@ * /api endpoint added after those lists were written answered a logged-in user * without MFA with a 403 while answering a stranger fine. MFA-011 pins the * correction. + * + * Plan 3b Task 1 (Task 0 review flag — the one guard test needing real + * rework): `MfaPolicyGuard` now takes `AppSettingsRepository`/`UsersRepository`/ + * `WebauthnCredentialsRepository` instead of `DatabaseService`, so this file + * stubs the three REPOSITORY METHODS directly (`getValue`/`getMfaEnabled`/ + * `hasAny`) rather than hand-rolling a `DatabaseService.get` stub that + * branched on SQL text substrings. */ import { describe, it, expect, vi } from 'vitest'; import { HttpException } from '@nestjs/common'; import { Reflector } from '@nestjs/core'; import { MfaExempt, MFA_EXEMPT, MfaPolicyGuard } from '../../../src/nest/auth/mfa-policy.guard'; import { Public, IS_PUBLIC } from '../../../src/nest/auth/public.decorator'; -import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; +import type { WebauthnCredentialsRepository } from '../../../src/db/repositories/WebauthnCredentials.repository'; import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; import { DEMO_EMAIL_PRIMARY } from '../../../src/nest/common/demo'; import type { User } from '../../../src/types'; @@ -21,29 +30,34 @@ const user = { id: 7, email: 'u@example.test', role: 'user' } as User; interface Rows { requireMfa?: string; - mfaEnabled?: number | boolean | undefined; + mfaEnabled?: number | undefined; hasPasskey?: boolean; userMissing?: boolean; } function makeGuard(rows: Rows = {}, meta: { public?: boolean; exempt?: boolean } = {}, demo = false) { - const get = vi.fn((sql: string) => { - if (sql.includes('app_settings')) return rows.requireMfa === undefined ? undefined : { value: rows.requireMfa }; - if (sql.includes('FROM users')) return rows.userMissing ? undefined : { mfa_enabled: rows.mfaEnabled ?? 0 }; - if (sql.includes('webauthn_credentials')) return rows.hasPasskey ? { 1: 1 } : undefined; - return undefined; - }); + const getValue = vi.fn(async (key: string) => (key === 'require_mfa' ? (rows.requireMfa ?? null) : null)); + const appSettings = { getValue } as unknown as AppSettingsRepository; + + const getMfaEnabled = vi.fn(async () => (rows.userMissing ? null : { mfa_enabled: rows.mfaEnabled ?? 0 })); + const users = { getMfaEnabled } as unknown as UsersRepository; + + const hasAny = vi.fn(async () => !!rows.hasPasskey); + const webauthnCredentials = { hasAny } as unknown as WebauthnCredentialsRepository; + const reflector = { getAllAndOverride: vi.fn((key: string) => (key === IS_PUBLIC && meta.public) || (key === MFA_EXEMPT && meta.exempt) ? { reason: 'x' } : undefined, ), } as unknown as Reflector; const guard = new MfaPolicyGuard( - { get } as unknown as DatabaseService, + appSettings, + users, + webauthnCredentials, { isDemoMode: () => demo } as unknown as RuntimeEnvService, reflector, ); - return { guard, get }; + return { guard, getValue, getMfaEnabled, hasAny }; } function ctx(request: Record) { @@ -90,9 +104,9 @@ describe('MfaPolicyGuard', () => { }); it('MFA-004: does nothing when the policy is off, and never reads the users row', async () => { - const { guard, get } = makeGuard({ requireMfa: 'false', mfaEnabled: 0 }); + const { guard, getMfaEnabled } = makeGuard({ requireMfa: 'false', mfaEnabled: 0 }); expect(await guard.canActivate(ctx({ user }))).toBe(true); - expect(get.mock.calls.some((c) => String(c[0]).includes('FROM users'))).toBe(false); + expect(getMfaEnabled).not.toHaveBeenCalled(); }); it('MFA-005: does nothing when the setting row is absent at all', async () => { @@ -101,9 +115,9 @@ describe('MfaPolicyGuard', () => { }); it('MFA-006: an anonymous request is not the policy business', async () => { - const { guard, get } = makeGuard(ENFORCED); + const { guard, getValue } = makeGuard(ENFORCED); expect(await guard.canActivate(ctx({}))).toBe(true); - expect(get).not.toHaveBeenCalled(); + expect(getValue).not.toHaveBeenCalled(); }); it('MFA-007: the demo account is exempt while demo mode is on', async () => { @@ -128,10 +142,13 @@ describe('MfaPolicyGuard', () => { it('MFA-010: it reads req.user rather than verifying the token again', async () => { // The middleware this replaces called jwt.verify and SELECTed the users row a - // second time on every /api request, then discarded the result. - const { guard, get } = makeGuard({ requireMfa: 'true', mfaEnabled: 1 }); + // second time on every /api request, then discarded the result. Now it reads + // exactly the two repository methods the policy needs — never a third, + // token-shaped read. + const { guard, getValue, getMfaEnabled } = makeGuard({ requireMfa: 'true', mfaEnabled: 1 }); await guard.canActivate(ctx({ user, headers: { authorization: 'Bearer nonsense' } })); - expect(get.mock.calls.map((c) => c[0]).every((sql) => !String(sql).includes('password_version'))).toBe(true); + expect(getValue).toHaveBeenCalledWith('require_mfa'); + expect(getMfaEnabled).toHaveBeenCalledWith(user.id); }); it('MFA-011: a @Public route no longer 403s a logged-in user without MFA', async () => { @@ -139,9 +156,9 @@ describe('MfaPolicyGuard', () => { // routes and /api/health/features all answered a stranger fine and answered // this user with a 403, because they were added after the middleware path // lists were written and nobody noticed. - const { guard, get } = makeGuard(ENFORCED, { public: true }); + const { guard, getValue } = makeGuard(ENFORCED, { public: true }); expect(await guard.canActivate(ctx({ user }))).toBe(true); - expect(get).not.toHaveBeenCalled(); + expect(getValue).not.toHaveBeenCalled(); }); it('MFA-012: the decorators write what the guard reads', () => { diff --git a/server/tests/unit/nest/oidc.service.test.ts b/server/tests/unit/nest/oidc.service.test.ts index e3e288b23..119e6e3cc 100644 --- a/server/tests/unit/nest/oidc.service.test.ts +++ b/server/tests/unit/nest/oidc.service.test.ts @@ -125,7 +125,7 @@ beforeAll(async () => { new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), membership, - new WebauthnConfigService(new DatabaseService(testDb)), + new WebauthnConfigService(await createTestAppSettingsRepo(testDb)), new UserCleanupService(new DatabaseService(testDb), new BudgetService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new ExchangeRatesService(), new RealtimeService(), await createTestUnitOfWork(testDb)), await createTestUnitOfWork(testDb)), mailerStub, new EphemeralTokenService(), diff --git a/server/tests/unit/nest/passkey.service.test.ts b/server/tests/unit/nest/passkey.service.test.ts index b4a84b0d7..606eda771 100644 --- a/server/tests/unit/nest/passkey.service.test.ts +++ b/server/tests/unit/nest/passkey.service.test.ts @@ -118,7 +118,7 @@ beforeAll(async () => { new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new TripMembershipService(new DatabaseService(testDb)), - new WebauthnConfigService(new DatabaseService(testDb)), + new WebauthnConfigService(await createTestAppSettingsRepo(testDb)), new UserCleanupService(new DatabaseService(testDb), new BudgetService(new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), new ExchangeRatesService(), new RealtimeService(), await createTestUnitOfWork(testDb)), await createTestUnitOfWork(testDb)), mailerStub, new EphemeralTokenService(), diff --git a/server/tests/unit/nest/platform.controller.test.ts b/server/tests/unit/nest/platform.controller.test.ts index ab1b79465..9a602638c 100644 --- a/server/tests/unit/nest/platform.controller.test.ts +++ b/server/tests/unit/nest/platform.controller.test.ts @@ -178,7 +178,7 @@ describe('applyPlatformUploads', () => { res, next, ); - expect(h.verifyJwtAndLoadUser).toHaveBeenCalledWith('jwt123'); + expect(h.verifyJwtAndLoadUser).toHaveBeenCalledWith('jwt123', expect.anything()); expect(h.sendToResponse).toHaveBeenCalledWith('photos', 'a.jpg', res); }); @@ -188,7 +188,7 @@ describe('applyPlatformUploads', () => { h.verifyJwtAndLoadUser.mockReturnValue({ id: 1 }); const res = makeRes(); await photoHandler()({ params: { filename: 'a.jpg' }, headers: {}, query: { token: 'qtok' } }, res, next); - expect(h.verifyJwtAndLoadUser).toHaveBeenCalledWith('qtok'); + expect(h.verifyJwtAndLoadUser).toHaveBeenCalledWith('qtok', expect.anything()); expect(h.sendToResponse).toHaveBeenCalledWith('photos', 'a.jpg', res); }); diff --git a/server/tests/unit/nest/trek-ws-adapter.test.ts b/server/tests/unit/nest/trek-ws-adapter.test.ts index ac6242a54..4ecac6815 100644 --- a/server/tests/unit/nest/trek-ws-adapter.test.ts +++ b/server/tests/unit/nest/trek-ws-adapter.test.ts @@ -315,6 +315,54 @@ describe('TrekWsAdapter D6 request context on connect (Plan 3b Task 0)', () => { }); }); +/** + * D6 request context on the DISCONNECT dispatch (Task 0 review addendum, + * LOW item 2) — the same property WSAD-050/051 pin for the connect dispatch, + * now for `bindClientDisconnect` (`OnGatewayDisconnect` / + * `RealtimeGateway.handleDisconnect`). The base `WsAdapter.bindClientDisconnect` + * is a bare `client.on('close', callback)` with no context of any kind. + */ +describe('TrekWsAdapter D6 request context on disconnect (Task 0 review addendum)', () => { + it('WSAD-052: without MikroORM passed to the adapter, the disconnect dispatch THROWS rather than running handleDisconnect unwrapped', () => { + const ad = new TrekWsAdapter({} as HttpServer); // no orm + const socket = fakeSocket(); + let handlerRan = false; + ad.bindClientDisconnect(socket as never, () => { + handlerRan = true; + }); + expect(() => socket.emit('close', Buffer.from(''))).toThrow(/no MikroORM available/i); + // Same fail-closed shape as WSAD-050: the wrapper throws BEFORE the + // disconnect callback ever runs. + expect(handlerRan).toBe(false); + }); + + it('WSAD-053: with MikroORM passed to the adapter, a repository read inside the disconnect callback succeeds — the wrapper is load-bearing', async () => { + const t = await createTestOrm(testDb, { allowGlobalContext: false }); + try { + let result: unknown; + let caught: unknown; + const ad = new TrekWsAdapter({} as HttpServer, t.orm); + const socket = fakeSocket(); + let captured: Promise | undefined; + ad.bindClientDisconnect(socket as never, () => { + captured = (async () => { + try { + result = await t.orm.em.find(Users, {}); + } catch (e) { + caught = e; + } + })(); + }); + socket.emit('close', Buffer.from('')); + await captured; + expect(caught).toBeUndefined(); + expect(Array.isArray(result)).toBe(true); + } finally { + await t.close(); + } + }); +}); + /** * The pointer exemption (#1973). * diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index 4f5b47c19..88f7f0bc3 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -78,7 +78,7 @@ import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.serv import { makeStorageFixture } from '../../helpers/storage-fixture'; import { JourneyDomainService } from '../../../src/nest/journey/journey-domain.service'; import { TrekPhotosRepository } from '../../../src/nest/photos/trek-photos.repository'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; // Real sibling services over the same in-memory DB — the aggregation runs the // actual SQL of every domain it fans out to, so a shape change downstream shows @@ -120,7 +120,7 @@ const buildReadModel = async (database: DatabaseService, roster: TripMembersServ new CollabService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), makeStorageFixture('').storage, new RateLimitService(), await createTestUnitOfWork(dbs().connection)), placesSvc, new TodoService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), await createTestUnitOfWork(dbs().connection)), - new FilesService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), new EphemeralTokenService(), makeStorageFixture('').storage), + new FilesService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), new EphemeralTokenService(), makeStorageFixture('').storage, (await sharedTestOrm(testDb)).em), ); let svc: Awaited>; diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 0d05b0d34..518ed5fac 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -88,7 +88,7 @@ import fs from 'fs'; import path from 'path'; import { notificationsStub } from '../../helpers/notifications'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, sharedTestOrm } from '../../helpers/test-uow'; // Real sibling services over the same in-memory DB — updateTrip's date-shift // resyncs and the summary/bundle aggregation run their actual SQL. @@ -156,7 +156,7 @@ beforeAll(async () => { new CollabService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), notificationsStub(), coversFx.storage, new RateLimitService(), await createTestUnitOfWork(dbs().connection)), placesSvc, new TodoService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), await createTestUnitOfWork(dbs().connection)), - new FilesService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), new EphemeralTokenService(), coversFx.storage), + new FilesService(dbs(), new PermissionsService(await createTestAppSettingsRepo(dbs().connection), await createTestUnitOfWork(dbs().connection)), new RealtimeService(), new EphemeralTokenService(), coversFx.storage, (await sharedTestOrm(testDb)).em), ); }); diff --git a/server/tests/unit/nest/user-profile.service.test.ts b/server/tests/unit/nest/user-profile.service.test.ts index 0bb6785ad..4ff59707f 100644 --- a/server/tests/unit/nest/user-profile.service.test.ts +++ b/server/tests/unit/nest/user-profile.service.test.ts @@ -42,7 +42,6 @@ import fs from 'node:fs'; import path from 'node:path'; import { UserProfileService } from '../../../src/nest/auth/user-profile.service'; import { makeStorageFixture } from '../../helpers/storage-fixture'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; import { SEARCH_TEXT_FIELD_MASK } from '../../../src/nest/maps/maps.helpers'; @@ -53,7 +52,6 @@ beforeAll(async () => { createTables(testDb); runMigrations(testDb); profile = new UserProfileService( - new DatabaseService(testDb), avatarsFx.storage, await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), diff --git a/server/tests/unit/nest/webauthn-config.service.test.ts b/server/tests/unit/nest/webauthn-config.service.test.ts index cf6150504..22d966c47 100644 --- a/server/tests/unit/nest/webauthn-config.service.test.ts +++ b/server/tests/unit/nest/webauthn-config.service.test.ts @@ -22,22 +22,19 @@ vi.mock('../../../src/app-config', async (importOriginal) => { }); import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { DatabaseService } from '../../../src/nest/database/database.service'; +import type { AppSettingsRepository } from '../../../src/db/repositories/AppSettings.repository'; import { WebauthnConfigService, originWithinRpScope } from '../../../src/nest/auth/webauthn-config.service'; -// Injected instead of vi.mocking src/db/database: the service reads exactly one -// row shape (app_settings.value), so a stub connection keeps the suite free of -// a schema and states the only DB contact the resolver has. -const conn = { - prepare: (_sql: string) => ({ - get: (key: string) => { - const v = settingsStore.get(key); - return v === undefined ? undefined : { value: v }; - }, - }), -} as unknown as ConstructorParameters[0]; +// Injected instead of vi.mocking src/db/database (Plan 3b Task 1: the service +// now takes an AppSettingsRepository, not DatabaseService): the service reads +// exactly one method (`getValue`), so a fake repository over the same real +// Map keeps the suite free of a schema and states the only contact the +// resolver has, unchanged from the DatabaseService-stub version. +const appSettings = { + getValue: async (key: string) => settingsStore.get(key) ?? null, +} as unknown as AppSettingsRepository; -const svc = new WebauthnConfigService(new DatabaseService(conn)); +const svc = new WebauthnConfigService(appSettings); beforeEach(() => { settingsStore.clear(); diff --git a/server/tests/unit/plugins/plugins-proxy.test.ts b/server/tests/unit/plugins/plugins-proxy.test.ts index 14c619c4b..5f81e46d6 100644 --- a/server/tests/unit/plugins/plugins-proxy.test.ts +++ b/server/tests/unit/plugins/plugins-proxy.test.ts @@ -13,9 +13,18 @@ const { pluginsEnabledMock, extractTokenMock, verifyMock } = vi.hoisted(() => ({ vi.mock('../../../src/nest/plugins/kill-switch', () => ({ pluginsEnabled: pluginsEnabledMock })); vi.mock('../../../src/nest/auth/jwt-verify', () => ({ extractToken: extractTokenMock, verifyJwtAndLoadUser: verifyMock })); +import type { EntityManager } from '@mikro-orm/core'; import { PluginsProxyController } from '../../../src/nest/plugins/plugins-proxy.controller'; import type { PluginRuntimeService } from '../../../src/nest/plugins/plugin-runtime.service'; +// EntityManager injection (Plan 3b Task 1 RULING) — verifyJwtAndLoadUser is +// fully mocked above, so `em.getRepository` never needs to return anything +// meaningful; it just has to not throw when the controller calls it. +const emStub = { getRepository: () => ({}) } as unknown as EntityManager; +function ctrl(runtime: PluginRuntimeService): PluginsProxyController { + return new PluginsProxyController(runtime, emStub); +} + function fakeRes() { const res = { statusCode: 0, @@ -52,33 +61,33 @@ describe('PluginsProxyController', () => { it('404 when the runtime is disabled', async () => { pluginsEnabledMock.mockReturnValue(false); const res = fakeRes(); - await new PluginsProxyController(makeRuntime()).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(makeRuntime()).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.statusCode).toBe(404); }); it('404 when the plugin is not active', async () => { const res = fakeRes(); - await new PluginsProxyController(makeRuntime({ isActive: vi.fn(() => false) } as never)).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(makeRuntime({ isActive: vi.fn(() => false) } as never)).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.statusCode).toBe(404); }); it('404 when no declared route matches', async () => { const res = fakeRes(); - await new PluginsProxyController(makeRuntime()).proxy('p', fakeReq('GET', '/nope'), res as never); + await ctrl(makeRuntime()).proxy('p', fakeReq('GET', '/nope'), res as never); expect(res.statusCode).toBe(404); }); it('401 on an auth route without a valid session', async () => { verifyMock.mockReturnValue(null as never); const res = fakeRes(); - await new PluginsProxyController(makeRuntime()).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(makeRuntime()).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.statusCode).toBe(401); }); it('forwards an authenticated request and returns the child response', async () => { const runtime = makeRuntime(); const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(runtime).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.statusCode).toBe(200); expect(res.headers['content-type']).toBe('application/json'); expect(res.body).toBe('{"ok":true}'); @@ -96,7 +105,7 @@ describe('PluginsProxyController', () => { verifyMock.mockReturnValue({ id: 7, username: 'root', role: 'admin' } as never); const runtime = makeRuntime(); const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(runtime).proxy('p', fakeReq('GET', '/status'), res as never); expect(runtime.invoke).toHaveBeenCalledWith('p', 'invoke.route', expect.objectContaining({ req: expect.objectContaining({ user: { id: 7, username: 'root', isAdmin: true } }), }), 7); @@ -105,7 +114,7 @@ describe('PluginsProxyController', () => { it('a public (auth:false) route skips the session check', async () => { const runtime = makeRuntime({ routesOf: vi.fn(() => [{ i: 1, method: 'POST', path: '/webhook', auth: false }]) } as never); const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('POST', '/webhook'), res as never); + await ctrl(runtime).proxy('p', fakeReq('POST', '/webhook'), res as never); expect(res.statusCode).toBe(200); expect(extractTokenMock).not.toHaveBeenCalled(); // a public route has no session user → no bound acting user (undefined) @@ -117,7 +126,7 @@ describe('PluginsProxyController', () => { it('forwards the raw request bytes to a webhook (auth:false) route, but withholds them from an authenticated route', async () => { // Webhook route → the plugin gets the raw payload so it can verify an HMAC. const wh = makeRuntime({ routesOf: vi.fn(() => [{ i: 1, method: 'POST', path: '/webhook', auth: false }]) } as never); - await new PluginsProxyController(wh).proxy('p', fakeReq('POST', '/webhook', { rawBody: Buffer.from('{"a":1}') }), fakeRes() as never); + await ctrl(wh).proxy('p', fakeReq('POST', '/webhook', { rawBody: Buffer.from('{"a":1}') }), fakeRes() as never); // forwarded as base64 so a non-UTF-8 signed body survives expect(wh.invoke).toHaveBeenCalledWith('p', 'invoke.route', expect.objectContaining({ req: expect.objectContaining({ rawBodyBase64: Buffer.from('{"a":1}').toString('base64') }), @@ -125,7 +134,7 @@ describe('PluginsProxyController', () => { // Authenticated route → raw bytes are never handed to the plugin. const auth = makeRuntime(); - await new PluginsProxyController(auth).proxy('p', fakeReq('GET', '/status', { rawBody: Buffer.from('secret') }), fakeRes() as never); + await ctrl(auth).proxy('p', fakeReq('GET', '/status', { rawBody: Buffer.from('secret') }), fakeRes() as never); const fwd = (auth.invoke as unknown as { mock: { calls: unknown[][] } }).mock.calls[0][2] as { req: { rawBodyBase64?: unknown } }; expect(fwd.req.rawBodyBase64).toBeUndefined(); }); @@ -141,7 +150,7 @@ describe('PluginsProxyController', () => { authorization: 'Bearer leak', // must be dropped 'x-socket-id': 'sock-1', // must be dropped }; - await new PluginsProxyController(runtime).proxy('p', fakeReq('POST', '/webhook', { headers }), res as never); + await ctrl(runtime).proxy('p', fakeReq('POST', '/webhook', { headers }), res as never); const forwarded = (runtime.invoke as unknown as { mock: { calls: unknown[][] } }).mock.calls[0][2] as { req: { headers: Record } }; expect(forwarded.req.headers).toEqual({ 'content-type': 'application/json', 'stripe-signature': 't=1,v1=abc', 'x-hub-signature-256': 'sha256=deadbeef' }); expect(forwarded.req.headers.cookie).toBeUndefined(); @@ -152,7 +161,7 @@ describe('PluginsProxyController', () => { it('an authenticated route gets NO inbound headers (even safe ones)', async () => { const runtime = makeRuntime(); // /status, auth:true const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('GET', '/status', { headers: { 'content-type': 'application/json', 'stripe-signature': 'x' } }), res as never); + await ctrl(runtime).proxy('p', fakeReq('GET', '/status', { headers: { 'content-type': 'application/json', 'stripe-signature': 'x' } }), res as never); const forwarded = (runtime.invoke as unknown as { mock: { calls: unknown[][] } }).mock.calls[0][2] as { req: { headers: Record } }; expect(forwarded.req.headers).toEqual({}); }); @@ -162,7 +171,7 @@ describe('PluginsProxyController', () => { invoke: vi.fn(async () => ({ status: 200, headers: { 'content-type': 'text/plain', 'set-cookie': 'evil=1' }, body: 'ok' })), } as never); const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(runtime).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.headers['content-type']).toBe('text/plain'); expect(res.headers['set-cookie']).toBeUndefined(); // a plugin cannot set cookies }); @@ -170,7 +179,7 @@ describe('PluginsProxyController', () => { it('502 when the plugin invoke throws', async () => { const runtime = makeRuntime({ invoke: vi.fn(async () => { throw new Error('down'); }) } as never); const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(runtime).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.statusCode).toBe(502); }); @@ -179,7 +188,7 @@ describe('PluginsProxyController', () => { invoke: vi.fn(async () => ({ status: 302, headers: { location: '/trips/5?tab=plan' } })), } as never); const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(runtime).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.statusCode).toBe(302); // normalised to path+query, still same-origin expect(res.headers['Location']).toBe('/trips/5?tab=plan'); @@ -200,7 +209,7 @@ describe('PluginsProxyController', () => { invoke: vi.fn(async () => ({ status: 302, headers: { location: loc } })), } as never); const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(runtime).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.statusCode).toBe(502); expect(res.body).toMatchObject({ detail: 'unsafe redirect target' }); expect(res.headers['Location']).toBeUndefined(); @@ -211,7 +220,7 @@ describe('PluginsProxyController', () => { invoke: vi.fn(async () => ({ status: 301, headers: {} })), } as never); const res = fakeRes(); - await new PluginsProxyController(runtime).proxy('p', fakeReq('GET', '/status'), res as never); + await ctrl(runtime).proxy('p', fakeReq('GET', '/status'), res as never); expect(res.statusCode).toBe(502); }); });