From 8d0dddf385f1c6c70fb80fe51bc67e94fb35371f Mon Sep 17 00:00:00 2001 From: jubnl Date: Tue, 22 Sep 2026 20:32:35 +0200 Subject: [PATCH] =?UTF-8?q?fix(db):=20row-out=20repository=20reads=20bypas?= =?UTF-8?q?s=20the=20identity=20map=20=E2=80=94=20stale=20write-back=20ove?= =?UTF-8?q?r=20nativeUpdate=20at=20flush;=20insertUser=20via=20em.insert;?= =?UTF-8?q?=20settings=20round-trip=20pinned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/db/repositories/Addons.repository.ts | 17 ++- .../db/repositories/AppSettings.repository.ts | 24 ++-- .../db/repositories/Categories.repository.ts | 33 +++-- .../PhotoProviderFields.repository.ts | 5 +- .../repositories/PhotoProviders.repository.ts | 5 +- .../db/repositories/Settings.repository.ts | 31 +++-- server/src/db/repositories/Tags.repository.ts | 31 +++-- .../src/db/repositories/Users.repository.ts | 127 +++++++++++------- .../db/repositories/_shared/owned-lookup.ts | 26 ++-- server/src/nest/auth/jwt-auth.guard.ts | 18 ++- server/src/nest/auth/mfa-policy.guard.ts | 4 + server/tests/integration/profile.test.ts | 30 +++++ .../AppSettings.repository.test.ts | 15 ++- .../Categories.repository.test.ts | 7 +- .../db/repositories/Tags.repository.test.ts | 12 +- .../db/repositories/Users.repository.test.ts | 68 +++++++++- .../repositories/_shared/owned-lookup.test.ts | 24 ++-- .../identity-map-writeback.test.ts | 100 ++++++++++++++ .../tests/unit/nest/auth/jwt-verify.test.ts | 25 +++- server/tests/unit/nest/files.service.test.ts | 8 +- .../unit/nest/platform.controller.test.ts | 4 +- 21 files changed, 442 insertions(+), 172 deletions(-) create mode 100644 server/tests/unit/db/repositories/identity-map-writeback.test.ts diff --git a/server/src/db/repositories/Addons.repository.ts b/server/src/db/repositories/Addons.repository.ts index 0c90af796..c5d88794d 100644 --- a/server/src/db/repositories/Addons.repository.ts +++ b/server/src/db/repositories/Addons.repository.ts @@ -20,14 +20,17 @@ export class AddonsRepository extends EntityRepository { /** * `SELECT enabled FROM addons WHERE id = ?` * - * `refresh: true` (Task 0 review, I1 — carried forward for every PK read in - * this plan): a primary-key `findOne` answers a repeat call from the - * identity map, invisible to a write on the same id inside the same - * request (`admin.service.ts`'s still-raw `UPDATE addons SET enabled = ?` - * on the same connection). + * `disableIdentityMap: true` (Plan 3b Task 1 fix round, supersedes Plan + * 3a's I1 "`refresh: true` on every PK-only `findOne`" — see + * `Users.repository.ts`'s class-level docstring and + * `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): a + * throwaway forked context always sees a write on the same id inside the + * same request (`admin.service.ts`'s still-raw `UPDATE addons SET enabled + * = ?` on the same connection), and the entity never lands in the + * request's identity map. */ async isEnabled(id: string): Promise { - const row = await this.findOne({ id }, { fields: ['enabled'], refresh: true }); + const row = await this.findOne({ id }, { fields: ['enabled'], disableIdentityMap: true }); return !!row?.enabled; } @@ -40,7 +43,7 @@ export class AddonsRepository extends EntityRepository { * for no parity benefit. */ async listEnabled(): Promise { - const rows = await this.find({ enabled: true }, { orderBy: { sort_order: 'asc' } }); + const rows = await this.find({ enabled: true }, { orderBy: { sort_order: 'asc' }, disableIdentityMap: true }); return rows.map((row) => toRow(row) as AddonRow); } } diff --git a/server/src/db/repositories/AppSettings.repository.ts b/server/src/db/repositories/AppSettings.repository.ts index 06e4315a4..7bf34eb1c 100644 --- a/server/src/db/repositories/AppSettings.repository.ts +++ b/server/src/db/repositories/AppSettings.repository.ts @@ -14,19 +14,17 @@ export class AppSettingsRepository extends EntityRepository { /** * `SELECT value FROM app_settings WHERE key = ?` * - * `refresh: true` (Task 0 review, I1): this is a primary-key lookup, so - * without it MikroORM answers a repeat call from the identity map rather - * than re-querying — invisible to a raw/native write or `deleteValue` on - * the same key inside the same request, which is exactly the coexistence - * case this plan is built on (raw SQL and the ORM sharing one connection - * while callers convert one at a time). `refresh` keeps this a single - * query either way; only a stale identity-map hit is avoided. - * Side effect of `refresh`: an UNFLUSHED in-memory change to the selected - * field on that entity is discarded (the entity reverts to the row). No - * caller mutates these entities before reading, by design (D4: rows out). + * `disableIdentityMap: true` (Plan 3b Task 1 fix round, supersedes Plan + * 3a's I1 "`refresh: true` on every PK-only `findOne`" — see + * `Users.repository.ts`'s class-level docstring for the full mechanism and + * `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): the read + * is answered from a throwaway forked context that is cleared afterwards, + * so a raw/native write or `deleteValue` on the same key inside the same + * request is always seen, and the entity never lands in the request's + * identity map to become a stale pending change at a later `flush()`. */ async getValue(key: string): Promise { - const row = await this.findOne({ key }, { fields: ['value'], refresh: true }); + const row = await this.findOne({ key }, { fields: ['value'], disableIdentityMap: true }); return row?.value ?? null; } @@ -37,7 +35,7 @@ export class AppSettingsRepository extends EntityRepository { * value" from "no row" (both read as absent). */ async getValues(keys: string[]): Promise> { - const rows = await this.find({ key: { $in: keys } }); + const rows = await this.find({ key: { $in: keys } }, { disableIdentityMap: true }); const values = new Map(); for (const row of rows) { if (row.key != null && row.value != null) values.set(row.key, row.value); @@ -82,7 +80,7 @@ export class AppSettingsRepository extends EntityRepository { * a prefix that itself contains `%`/`_`. */ async findByKeyPrefix(prefix: string): Promise { - const rows = await this.find({ key: { $like: `${prefix}%` } }); + const rows = await this.find({ key: { $like: `${prefix}%` } }, { disableIdentityMap: true }); return rows.map((row) => toRow(row) as AppSettingsRow); } diff --git a/server/src/db/repositories/Categories.repository.ts b/server/src/db/repositories/Categories.repository.ts index 9d9243038..34ee1a50b 100644 --- a/server/src/db/repositories/Categories.repository.ts +++ b/server/src/db/repositories/Categories.repository.ts @@ -17,7 +17,7 @@ const _categoryRowKeys: AssertRowKeys = true; export class CategoriesRepository extends EntityRepository { /** `SELECT * FROM categories ORDER BY name ASC` */ async list(): Promise { - const categories = await this.find({}, { orderBy: { name: 'asc' } }); + const categories = await this.find({}, { orderBy: { name: 'asc' }, disableIdentityMap: true }); return categories.map((category) => toRow(category) as CategoryRow); } @@ -30,19 +30,18 @@ export class CategoriesRepository extends EntityRepository { * global") and was deleted rather than kept as dead code (Task 0 * re-review, R6; see `_shared/owned-lookup.ts`'s docstring). * - * `refresh: true` (Task 0 review, I1 — every PK-only `findOne` in a - * repository takes it) and it is load-bearing here, not decorative: `{ id - * }` is exactly the primary key, so MikroORM answers this call from the - * identity map with zero queries whenever the row is already loaded - * (Task 3 review, Important 2's correction — the short-circuit is driven - * by the filter being PK-only, not by a `fields` restriction, which was - * the wrong mechanism this docstring stated before the fix). Without - * `refresh`, a raw write to this row earlier in the same request would - * make this method return the stale snapshot instead of the current DB - * row. + * `disableIdentityMap: true` (Plan 3b Task 1 fix round, supersedes Plan + * 3a's I1 "`refresh: true` on every PK-only `findOne`" — see + * `Users.repository.ts`'s class-level docstring and + * `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): this is + * a "rows out" read, converted via `toRow` and discarded — the entity is + * loaded in a throwaway forked context and never lands in the request's + * identity map, so a raw write to this row earlier in the same request is + * always seen, and there is nothing here for a later `flush()` to find + * dirty. */ async findById(id: number): Promise { - const category = await this.findOne({ id }, { refresh: true }); + const category = await this.findOne({ id }, { disableIdentityMap: true }); return category ? (toRow(category) as CategoryRow) : null; } @@ -103,6 +102,16 @@ export class CategoriesRepository extends EntityRepository { * managed (and now guaranteed-fresh) entity in place, and no column here * is DB-computed on UPDATE (unlike `created_at` on insert), so the * in-memory entity already holds exactly what was written. + * + * **Excluded from the Plan 3b Task 1 fix round's `disableIdentityMap: true` + * ruling, deliberately** — see `TagsRepository.patch`'s docstring for the + * full reasoning: this is not a "rows out" read, the entity must stay + * MANAGED for `assign`+`flush`, and `disableIdentityMap: true` would + * silently stop the write from persisting (a detached entity is outside + * `flush()`'s unit of work). `refresh: true` stays for the identity-map + * freshness reason above; there is no B1-shaped staleness risk because + * this is the only read of this row left un-`disableIdentityMap`-ed in the + * repository. */ async patch(id: number, changes: { name?: string; color?: string; icon?: string }): Promise { const category = await this.findOne({ id }, { refresh: true }); diff --git a/server/src/db/repositories/PhotoProviderFields.repository.ts b/server/src/db/repositories/PhotoProviderFields.repository.ts index 87c55b52c..e894cf771 100644 --- a/server/src/db/repositories/PhotoProviderFields.repository.ts +++ b/server/src/db/repositories/PhotoProviderFields.repository.ts @@ -28,9 +28,12 @@ export class PhotoProviderFieldsRepository extends EntityRepository { - const rows = await this.find({}, { orderBy: { sort_order: 'asc', id: 'asc' } }); + const rows = await this.find({}, { orderBy: { sort_order: 'asc', id: 'asc' }, disableIdentityMap: true }); return rows.map((row) => toRow(row) as PhotoProviderFieldRow); } } diff --git a/server/src/db/repositories/PhotoProviders.repository.ts b/server/src/db/repositories/PhotoProviders.repository.ts index 2b129eb73..e2cbbae3c 100644 --- a/server/src/db/repositories/PhotoProviders.repository.ts +++ b/server/src/db/repositories/PhotoProviders.repository.ts @@ -21,9 +21,12 @@ export class PhotoProvidersRepository extends EntityRepository { * entity (unlike `Addons.enabled`, which is `p.boolean()`), so it comes * back as the raw stored `0`/`1`; `AddonsService.list()` does the `!!` * coercion itself, matching the legacy `!!p.enabled`. + * + * `disableIdentityMap: true` (Plan 3b Task 1 fix round) — a "rows out" + * read, converted via `toRow` and discarded. */ async listEnabled(): Promise { - const rows = await this.find({ enabled: 1 }, { orderBy: { sort_order: 'asc', id: 'asc' } }); + const rows = await this.find({ enabled: 1 }, { orderBy: { sort_order: 'asc', id: 'asc' }, disableIdentityMap: true }); return rows.map((row) => toRow(row) as PhotoProviderRow); } } diff --git a/server/src/db/repositories/Settings.repository.ts b/server/src/db/repositories/Settings.repository.ts index 03f292f0e..52bc743f7 100644 --- a/server/src/db/repositories/Settings.repository.ts +++ b/server/src/db/repositories/Settings.repository.ts @@ -13,28 +13,33 @@ export interface SettingRow { const _settingRowKeys: AssertRowKeys = true; export class SettingsRepository extends EntityRepository { - /** `SELECT key, value FROM settings WHERE user_id = ?` */ + /** + * `SELECT key, value FROM settings WHERE user_id = ?` + * + * `disableIdentityMap: true` (Plan 3b Task 1 fix round) — a "rows out" + * read, converted via `toRow` and discarded. + */ async getForUser(userId: number): Promise { - const rows = await this.find({ user: userId }); + const rows = await this.find({ user: userId }, { disableIdentityMap: true }); return rows.map((row) => toRow(row) as SettingRow); } /** * `SELECT value FROM settings WHERE user_id = ? AND key = ?` * - * No `refresh: true`: the identity-map short-circuit (Task 0 review, I1; - * corrected by the Task 3 review — it is NOT about a `fields` restriction) - * only fires for a PRIMARY-KEY-ONLY filter (`findOne({ id })` / `findOne(pk)`): - * MikroORM answers that shape straight from the identity map with zero - * queries, invisible to a write on the same row earlier in the request. - * This filter is `(user, key)` — `Settings`'s PK is the surrogate `id`, not - * this composite — so it is never PK-only and always re-queries, the same - * guarantee a `find()` call gets. `SETTINGSREPO-012`/`013` pin this - * directly (a raw UPDATE/DELETE on the same row, then `getOne` in the same - * request, sees it) rather than assume it. + * `disableIdentityMap: true` (Plan 3b Task 1 fix round, supersedes Plan + * 3a's I1 — see `Users.repository.ts`'s class-level docstring and + * `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): this + * filter is `(user, key)` — `Settings`'s PK is the surrogate `id`, not + * this composite — so it was never PK-only and always re-queried even + * before this ruling; `disableIdentityMap: true` is applied uniformly + * regardless, matching every other read in this repository. + * `SETTINGSREPO-012`/`013` pin the always-fresh-read property directly (a + * raw UPDATE/DELETE on the same row, then `getOne` in the same request, + * sees it) rather than assume it. */ async getOne(userId: number, key: string): Promise { - const row = await this.findOne({ user: userId, key }); + const row = await this.findOne({ user: userId, key }, { disableIdentityMap: true }); return row ? (toRow(row) as SettingRow) : null; } diff --git a/server/src/db/repositories/Tags.repository.ts b/server/src/db/repositories/Tags.repository.ts index 1b5446fef..385f8230b 100644 --- a/server/src/db/repositories/Tags.repository.ts +++ b/server/src/db/repositories/Tags.repository.ts @@ -32,21 +32,12 @@ export class TagsRepository extends EntityRepository { * own write (`createTag`/`patch` refresh/return their own entity * directly). * - * `refresh: true` (Task 0 review, I1 — the ruling every Plan 3 repository - * follows for a PK-scoped read). This call's filter is `{ id, user }`, - * not PK-only, so MikroORM's identity-map short-circuit — which fires - * only when the filter is exactly the primary key, not (as an earlier - * version of this docstring said) when a `fields` restriction is present - * (Task 3 review, Important 2's correction) — never engages here anyway: - * this call already re-queries and merges fresh data on every call - * regardless of `refresh`. `refresh` stays threaded through to match the - * blanket ruling and to stay correct if a future caller narrows the - * filter to PK-only. `findOwnedByUser`'s optional `options` parameter - * (added here, not duplicated — Task 0's helper had no reads that needed - * it yet) passes straight through to the underlying `findOne`. + * `disableIdentityMap: true` per `findOwnedByUser`'s own docstring (Plan + * 3b Task 1 fix round, supersedes Plan 3a's I1) — this is a "rows out" + * read, converted via `toRow` and discarded. */ async findByIdAndUser(id: number, userId: number): Promise { - const tag = await findOwnedByUser(this, id, 'user', userId, { refresh: true }); + const tag = await findOwnedByUser(this, id, 'user', userId); return tag ? (toRow(tag) as TagRow) : null; } @@ -98,6 +89,20 @@ export class TagsRepository extends EntityRepository { * `CategoriesRepository.patch`: `assign` already mutated the managed (and * now guaranteed-fresh) entity in place, and no column here is * DB-computed on UPDATE. + * + * **Excluded from the Plan 3b Task 1 fix round's `disableIdentityMap: true` + * ruling, deliberately**: unlike every other read in this repository, this + * one is not a "rows out" read — the entity is kept MANAGED on purpose so + * `assign`+`flush` can diff and write it. `disableIdentityMap: true` would + * return a *detached* entity (per the ruling's own mechanism); `assign`ing + * it would still mutate the JS object, but the following `flush()` would + * have no effect on it (a detached entity is not in `flush()`'s unit of + * work) — the write would silently stop persisting. `refresh: true` stays + * here for exactly the reason it always was: this is the only call in the + * repository that manages this row, so there is no B1-shaped staleness + * risk (that requires a SECOND managed, differently-projected read of the + * same row still lingering in the identity map — nothing else here leaves + * one now that every other read is `disableIdentityMap: true`). */ async patch(id: number, changes: { name?: string; color?: string }): Promise { const tag = await this.findOne({ id }, { refresh: true }); diff --git a/server/src/db/repositories/Users.repository.ts b/server/src/db/repositories/Users.repository.ts index 859599f61..40720fa9b 100644 --- a/server/src/db/repositories/Users.repository.ts +++ b/server/src/db/repositories/Users.repository.ts @@ -1,4 +1,4 @@ -import type { Users } from '../entities/Users.entity'; +import { 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'; @@ -177,18 +177,23 @@ export interface ResetTargetRow { export class UsersRepository extends EntityRepository { /** - * `SELECT email FROM users WHERE id = ?` - * - * `refresh: true` (Task 0 review, I1): a primary-key `findOne` is answered - * from the identity map on a repeat call, which would hide a raw/native - * `UPDATE users SET email = ...` on the same id inside the same request. - * `refresh` keeps this a single query and always sees the current row. - * Side effect of `refresh`: an UNFLUSHED in-memory change to the selected - * field on that entity is discarded (the entity reverts to the row). No - * caller mutates these entities before reading, by design (D4: rows out). + * **Ruling (Plan 3b Task 1 fix round, supersedes Plan 3a's I1 "`refresh: + * true` on every PK-only `findOne`"):** every row-out read in this + * repository (`findOne`/`find`) passes `disableIdentityMap: true` instead. + * MikroORM forks the EM with `keepTransactionContext: true`, loads inside + * that fork (so the read still sees the transaction's own uncommitted + * writes), and clears the fork afterwards + * (`node_modules/@mikro-orm/core/EntityManager.js:807-814`) — the entity + * is never added to the *request's* identity map, so it can never become a + * stale pending change-set entry that a later `flush()` (including the + * implicit one at `UnitOfWork.transactional` commit) writes back over an + * intervening `nativeUpdate`. `refresh: true` is dropped as redundant: it + * only mattered for an entity that stayed in the map, and none does now. + * See https://mikro-orm.io/docs/entity-manager#disableidentitymap and + * `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` (B1). */ async getEmail(userId: number): Promise { - const row = await this.findOne({ id: userId }, { fields: ['email'], refresh: true }); + const row = await this.findOne({ id: userId }, { fields: ['email'], disableIdentityMap: true }); return row?.email ?? null; } @@ -198,10 +203,10 @@ export class UsersRepository extends EntityRepository { * not an interpolated column — MikroORM's `fields` option only accepts a * real property of `Users`, so this can never select an arbitrary column. * - * `refresh: true` for the same identity-map reason as `getEmail` above. + * `disableIdentityMap: true` per the class-level ruling above. */ async getApiKeyColumn(userId: number, name: InstanceApiKeyName): Promise { - const row = await this.findOne({ id: userId }, { fields: [name], refresh: true }); + const row = await this.findOne({ id: userId }, { fields: [name], disableIdentityMap: true }); return row ? (row[name] ?? null) : null; } @@ -214,12 +219,12 @@ export class UsersRepository extends EntityRepository { * * 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). + * request. `disableIdentityMap: true` per the class-level ruling above. */ async findByIdWithPasswordVersion(id: number): Promise { const row = await this.findOne( { id }, - { fields: ['id', 'username', 'email', 'role', 'password_version'], refresh: true }, + { fields: ['id', 'username', 'email', 'role', 'password_version'], disableIdentityMap: true }, ); return row ? { id: row.id, username: row.username, email: row.email, role: row.role, password_version: row.password_version } : null; } @@ -230,7 +235,7 @@ export class UsersRepository extends EntityRepository { /** `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 }); + const row = await this.findOne({ id }, { fields: ['password_version'], disableIdentityMap: true }); return row?.password_version ?? null; } @@ -249,7 +254,7 @@ export class UsersRepository extends EntityRepository { /** `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'] }); + const row = await this.findOne({ role: 'admin', must_change_password: 1 }, { fields: ['id'], disableIdentityMap: true }); return row?.id ?? null; } @@ -268,7 +273,7 @@ export class UsersRepository extends EntityRepository { $or: [{ [lower(platform, 'email')]: email.toLowerCase() }, { [lower(platform, 'username')]: username.toLowerCase() }], is_guest: 0, }, - { fields: ['id'] }, + { fields: ['id'], disableIdentityMap: true }, ); return row?.id ?? null; } @@ -287,12 +292,19 @@ export class UsersRepository extends EntityRepository { * 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` + * `em.insert` (F2, Plan 3b Task 1 fix round), never `create()` + + * `persist().flush()`: `flush()` commits the *whole* unit of work of the + * request's `EntityManager`, not just this row — the review caught it + * emitting a full-row `UPDATE users … WHERE id = ?` against an unrelated, + * previously-read user in the same EM. `em.insert` fires a single native + * INSERT with no side effects on the context/identity map (the same rule + * as `AuditLogRepository.insertEntry`). Followed by a read-back + * (`findById`, `disableIdentityMap: true` per the class-level ruling): 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({ + const id = await this.getEntityManager().insert(Users, { username: row.username, email: row.email, password_hash: row.password_hash, @@ -303,9 +315,11 @@ export class UsersRepository extends EntityRepository { 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; + const inserted = await this.findById(id as number); + if (!inserted) { + throw new Error('insertUser: read-back after insert found no row'); + } + return inserted; } // --------------------------------------------------------------------- @@ -315,7 +329,27 @@ export class UsersRepository extends EntityRepository { /** `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 }); + const user = await this.findOne( + { [lower(platform, 'email')]: email.toLowerCase(), is_guest: 0 }, + { disableIdentityMap: true }, + ); + return user ? (toRow(user) as UserRow) : null; + } + + // --------------------------------------------------------------------- + // AuthService.demoLogin (auth.service.ts:~376) — inventory-correction + // addition, Plan 3b Task 1 fix round item 7 (pre-authorised for Task 5). + // --------------------------------------------------------------------- + + /** + * `SELECT * FROM users WHERE email = ?` — case-**sensitive**, **no** guest + * filter, full row. Distinct from both `findByEmailCI` (case-insensitive + * + guest-filtered) and `findForPasswordReset` (case-sensitive but + * projected + guest-filtered): `AuthService.demoLogin` needs the exact, + * unfiltered legacy statement, including matching a guest row. + */ + async findByEmailExact(email: string): Promise { + const user = await this.findOne({ email }, { disableIdentityMap: true }); return user ? (toRow(user) as UserRow) : null; } @@ -350,7 +384,7 @@ export class UsersRepository extends EntityRepository { { id }, { fields: ['id', 'username', 'email', 'role', 'avatar', 'oidc_issuer', 'created_at', 'mfa_enabled', 'must_change_password'], - refresh: true, + disableIdentityMap: true, }, ); return row @@ -374,7 +408,7 @@ export class UsersRepository extends EntityRepository { /** `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 }); + const row = await this.findOne({ id }, { fields: ['password_hash', 'password_version'], disableIdentityMap: true }); return row ? { password_hash: row.password_hash, password_version: row.password_version } : null; } @@ -410,7 +444,7 @@ export class UsersRepository extends EntityRepository { /** `SELECT role FROM users WHERE id = ?` */ async getRole(id: number): Promise { - const row = await this.findOne({ id }, { fields: ['role'], refresh: true }); + const row = await this.findOne({ id }, { fields: ['role'], disableIdentityMap: true }); return row?.role ?? null; } @@ -430,7 +464,7 @@ export class UsersRepository extends EntityRepository { * 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 }); + const row = await this.findOne({ id }, { fields: ['mfa_enabled'], disableIdentityMap: true }); return row ? { mfa_enabled: row.mfa_enabled ?? null } : null; } @@ -460,10 +494,10 @@ export class UsersRepository extends EntityRepository { * 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). + * `disableIdentityMap: true` per the class-level ruling above. */ async findById(id: number): Promise { - const user = await this.findOne({ id }, { refresh: true }); + const user = await this.findOne({ id }, { disableIdentityMap: true }); return user ? (toRow(user) as UserRow) : null; } @@ -518,7 +552,7 @@ export class UsersRepository extends EntityRepository { async findForPasswordReset(email: string): Promise { const row = await this.findOne( { email, is_guest: 0 }, - { fields: ['id', 'email', 'password_hash', 'oidc_sub'] }, + { fields: ['id', 'email', 'password_hash', 'oidc_sub'], disableIdentityMap: true }, ); return row ? { id: row.id, email: row.email, password_hash: row.password_hash, oidc_sub: row.oidc_sub ?? null } : null; } @@ -531,7 +565,7 @@ export class UsersRepository extends EntityRepository { 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 }, + { fields: ['id', 'email', 'mfa_enabled', 'mfa_secret', 'mfa_backup_codes', 'password_version'], disableIdentityMap: true }, ); return row ? { @@ -551,7 +585,7 @@ export class UsersRepository extends EntityRepository { /** `SELECT password_hash FROM users WHERE id = ?` */ async getPasswordHash(id: number): Promise { - const row = await this.findOne({ id }, { fields: ['password_hash'], refresh: true }); + const row = await this.findOne({ id }, { fields: ['password_hash'], disableIdentityMap: true }); return row?.password_hash ?? null; } @@ -561,7 +595,7 @@ export class UsersRepository extends EntityRepository { /** `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 }); + const row = await this.findOne({ id }, { fields: ['id', 'email'], disableIdentityMap: true }); return row ? { id: row.id, email: row.email } : null; } @@ -571,7 +605,7 @@ export class UsersRepository extends EntityRepository { /** `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 }); + const user = await this.findOne({ oidc_sub: sub, oidc_issuer: issuer }, { disableIdentityMap: true }); return user ? (toRow(user) as UserRow) : null; } @@ -624,7 +658,7 @@ export class UsersRepository extends EntityRepository { const platform = this.getEntityManager().getPlatform(); const row = await this.findOne( { [lower(platform, 'username')]: username.toLowerCase(), id: { $ne: excludeId }, is_guest: 0 }, - { fields: ['id'] }, + { fields: ['id'], disableIdentityMap: true }, ); return row?.id ?? null; } @@ -638,7 +672,10 @@ export class UsersRepository extends EntityRepository { */ async findIdByUsernameCIAny(username: string): Promise { const platform = this.getEntityManager().getPlatform(); - const row = await this.findOne({ [lower(platform, 'username')]: username.toLowerCase() }, { fields: ['id'] }); + const row = await this.findOne( + { [lower(platform, 'username')]: username.toLowerCase() }, + { fields: ['id'], disableIdentityMap: true }, + ); return row?.id ?? null; } @@ -650,7 +687,7 @@ export class UsersRepository extends EntityRepository { 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 }, + { fields: ['role', 'maps_api_key', 'openweather_api_key', 'unsplash_api_key', 'amap_api_key'], disableIdentityMap: true }, ); return row ? { @@ -700,7 +737,7 @@ export class UsersRepository extends EntityRepository { { id }, { fields: ['id', 'username', 'email', 'role', 'maps_api_key', 'openweather_api_key', 'unsplash_api_key', 'amap_api_key', 'avatar', 'mfa_enabled'], - refresh: true, + disableIdentityMap: true, }, ); return row @@ -728,7 +765,7 @@ export class UsersRepository extends EntityRepository { const platform = this.getEntityManager().getPlatform(); const row = await this.findOne( { [lower(platform, 'email')]: email.toLowerCase(), id: { $ne: excludeId }, is_guest: 0 }, - { fields: ['id'] }, + { fields: ['id'], disableIdentityMap: true }, ); return row?.id ?? null; } @@ -761,7 +798,7 @@ export class UsersRepository extends EntityRepository { /** `SELECT avatar FROM users WHERE id = ?` */ async getAvatar(id: number): Promise { - const row = await this.findOne({ id }, { fields: ['avatar'], refresh: true }); + const row = await this.findOne({ id }, { fields: ['avatar'], disableIdentityMap: true }); return row?.avatar ?? null; } @@ -785,7 +822,7 @@ export class UsersRepository extends EntityRepository { /** `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 }); + const row = await this.findOne({ id }, { fields: ['id', 'username', 'email', 'role', 'avatar'], disableIdentityMap: true }); return row ? { id: row.id, username: row.username, email: row.email, role: row.role, avatar: row.avatar ?? null } : null; } @@ -797,7 +834,7 @@ export class UsersRepository extends EntityRepository { async listOthersNonGuest(excludeId: number): Promise { const rows = await this.find( { id: { $ne: excludeId }, is_guest: 0 }, - { fields: ['id', 'username', 'avatar'], orderBy: { username: 'asc' } }, + { fields: ['id', 'username', 'avatar'], orderBy: { username: 'asc' }, disableIdentityMap: true }, ); return rows.map((row) => ({ id: row.id, username: row.username, avatar: row.avatar ?? null })); } @@ -808,7 +845,7 @@ export class UsersRepository extends EntityRepository { /** `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 }); + const row = await this.findOne({ id }, { fields: ['role', 'openweather_api_key'], disableIdentityMap: true }); return row ? { role: row.role, openweather_api_key: row.openweather_api_key ?? null } : null; } } diff --git a/server/src/db/repositories/_shared/owned-lookup.ts b/server/src/db/repositories/_shared/owned-lookup.ts index d7e31956b..4a12bd783 100644 --- a/server/src/db/repositories/_shared/owned-lookup.ts +++ b/server/src/db/repositories/_shared/owned-lookup.ts @@ -36,7 +36,13 @@ import type { EntityRepository, FilterQuery, FilterValue, OrderDefinition } from * * listForOwner(tags, 'user', userId, 'name'); * findOwnedByUser(tags, id, 'user', userId); - * findOwnedByUser(tags, id, 'user', userId, { refresh: true }); + * + * Both helpers pass `disableIdentityMap: true` (Plan 3b Task 1 fix round, + * supersedes Plan 3a's I1 "`refresh: true` on every PK-only `findOne`" — see + * `Users.repository.ts`'s class-level docstring and + * `.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md` B1): both are + * "rows out" reads whose entity is converted via `toRow` and discarded, never + * assigned to nor flushed, so a throwaway forked context is exactly right. * * Typing (Task 0 review, I2): `T extends { id: unknown }` and `K extends * keyof T` tie `ownerField` to a real property of `T` and `ownerId` to @@ -64,6 +70,7 @@ export async function listForOwner { return repo.find({ [ownerField]: ownerId } as FilterQuery, { orderBy: { [orderField]: 'asc' } as OrderDefinition, + disableIdentityMap: true, }); } @@ -71,24 +78,15 @@ export async function listForOwner WHERE id = ? AND = ?`. * - * `options.refresh` passes straight through to the underlying `findOne` - * (Task 0 review, I1's PK-read ruling: a caller whose read can follow a - * raw/native write on the same row within the same request sets it). This - * filter is `{ id, [ownerField]: ownerId }` — not PK-only — so MikroORM's - * identity-map short-circuit never engages here: that short-circuit fires - * only when the filter is exactly the primary key (Task 3 review, Important - * 2's correction — `fields` has nothing to do with it, unlike what an - * earlier version of this docstring claimed). `refresh` is a harmless - * no-op on every call this helper serves today; it stays threaded through - * to match the blanket ruling and to stay correct if a future caller - * narrows the filter to PK-only. + * `disableIdentityMap: true` per the module-level docstring above — this is + * always a "rows out" read here, never a caller's staging read for + * `assign`+`flush`. */ export async function findOwnedByUser( repo: EntityRepository, id: T['id'], ownerField: K, ownerId: FilterValue, - options?: { refresh?: boolean }, ): Promise { - return repo.findOne({ id, [ownerField]: ownerId } as FilterQuery, options); + return repo.findOne({ id, [ownerField]: ownerId } as FilterQuery, { disableIdentityMap: true }); } diff --git a/server/src/nest/auth/jwt-auth.guard.ts b/server/src/nest/auth/jwt-auth.guard.ts index 016dd0143..8e9cb313e 100644 --- a/server/src/nest/auth/jwt-auth.guard.ts +++ b/server/src/nest/auth/jwt-auth.guard.ts @@ -27,9 +27,21 @@ import { extractToken, verifyJwtAndLoadUser } from './jwt-verify'; * 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. + * `platform.routes.ts::servePhoto` uses (Task 0). + * + * **Correction (Plan 3b Task 1 fix round, task-1-review.md's Ruling):** + * `this.em` here is NOT a request-scoped fork — with no `scope` on + * `MikroOrmModule.forRoot` and no `Scope.REQUEST` anywhere in `src/`, + * `@mikro-orm/nestjs`'s provider factory injects `orm.em`, the ORM's single + * GLOBAL, context-resolving `EntityManager`, the same object every request + * shares. It resolves to the request's transactional fork at QUERY TIME: + * `EntityRepository.getRepository` builds the repository holding a + * reference to `this.em` with no `getContext()` call, and + * `EntityRepository.findOne` calls `this.getEntityManager().findOne(...)`, + * which resolves the live `AsyncLocalStorage`/`TransactionContext` inside + * that call — so the repository built here still finds the request's own + * fork correctly, even though `this.em` itself is the shared global + * instance, not a per-request object. */ @Injectable() export class JwtAuthGuard implements CanActivate { diff --git a/server/src/nest/auth/mfa-policy.guard.ts b/server/src/nest/auth/mfa-policy.guard.ts index 475619177..852b2326b 100644 --- a/server/src/nest/auth/mfa-policy.guard.ts +++ b/server/src/nest/auth/mfa-policy.guard.ts @@ -79,6 +79,10 @@ export class MfaPolicyGuard implements CanActivate { // 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.) + // `=== 1`, not the legacy `row.mfa_enabled === 1 || row.mfa_enabled === true`: + // `mfa_enabled` is a SQLite INTEGER column, and `UsersRepository.getMfaEnabled` + // types it `number | null` — a JS `boolean` from this column never occurs + // through the ORM, so the `=== true` half was dead code (task-1-review.md F4). const mfaOk = row.mfa_enabled === 1; const passkeyOk = await this.webauthnCredentials.hasAny(user.id); if (mfaOk || passkeyOk) return true; diff --git a/server/tests/integration/profile.test.ts b/server/tests/integration/profile.test.ts index 35da10446..56cc85a94 100644 --- a/server/tests/integration/profile.test.ts +++ b/server/tests/integration/profile.test.ts @@ -72,6 +72,36 @@ describe('PROFILE-001 — Get current user profile', () => { }); }); +// PUT /api/auth/me/settings — F3 (task-1-review.md): the B1 regression's +// user-visible symptom was exactly this route answering 200 while silently +// discarding the write. Asserts the DB row directly, not only the response +// body, and the response body against a fresh GET /api/auth/me — either one +// alone would have missed B1 (the response body came from the pre-flush +// in-memory entity, which still looked right). +describe('PUT /api/auth/me/settings (F3)', () => { + it('PROFILE-016 — a username+email change persists: the response, a fresh GET, and the raw row all agree', async () => { + const { user } = createUser(testDb, { username: 'before-name', email: 'before@example.test' }); + + const put = await request(app) + .put('/api/auth/me/settings') + .set('Cookie', authCookie(user.id)) + .send({ username: 'after-name', email: 'after@example.test' }); + expect(put.status).toBe(200); + expect(put.body.success).toBe(true); + expect(put.body.user).toMatchObject({ username: 'after-name', email: 'after@example.test' }); + + const get = await request(app) + .get('/api/auth/me') + .set('Cookie', authCookie(user.id)); + expect(get.status).toBe(200); + expect(get.body.user).toMatchObject({ username: 'after-name', email: 'after@example.test' }); + + const row = testDb.prepare('SELECT username, email FROM users WHERE id = ?').get(user.id) as { username: string; email: string }; + expect(row.username).toBe('after-name'); + expect(row.email).toBe('after@example.test'); + }); +}); + describe('Avatar', () => { it('PROFILE-002 — upload valid JPEG avatar updates avatar_url', async () => { const { user } = createUser(testDb); diff --git a/server/tests/unit/db/repositories/AppSettings.repository.test.ts b/server/tests/unit/db/repositories/AppSettings.repository.test.ts index 2573e71b0..c7590cb20 100644 --- a/server/tests/unit/db/repositories/AppSettings.repository.test.ts +++ b/server/tests/unit/db/repositories/AppSettings.repository.test.ts @@ -27,8 +27,9 @@ function insertRaw(key: string, value: string | null): void { /** * Runs `fn` while counting queries MikroORM actually issues over the shared * connection, so a regression test can assert "one query, with this value" - * rather than only the value — the identity-map bug I1 fixed (Task 0 review) - * returned the *right* value from a stale cache with *zero* queries, which a + * rather than only the value — a regression in the `disableIdentityMap: true` + * ruling (Plan 3b Task 1 fix round, `task-1-review.md` B1) would return the + * *right* value from a stale identity-map hit with *zero* queries, which a * value-only assertion would never catch. */ async function withQueryCount(fn: () => Promise): Promise<{ value: T; queries: number }> { @@ -67,12 +68,12 @@ describe('AppSettingsRepository', () => { expect(values.has('collab_links_enabled')).toBe(false); }); - // I1 (Task 0 review): getValue is a primary-key findOne, which MikroORM - // answers from the identity map on a repeat call unless `refresh: true` is - // set — invisible to a raw write or delete on the same key inside the same - // request. These reproduce the review's two failing probes and pin the + // Plan 3b Task 1 fix round (task-1-review.md B1): getValue is a + // primary-key findOne, which MikroORM would answer from the identity map + // on a repeat call absent `disableIdentityMap: true` — invisible to a raw + // write or delete on the same key inside the same request. These pin the // fix at one query each. - describe('getValue sees a raw write/delete on the same key in the same request (I1, identity-map regression)', () => { + describe('getValue sees a raw write/delete on the same key in the same request (disableIdentityMap regression)', () => { it('APPSETREPO-013: deleteValue then getValue reads null, not the deleted row, in one query', async () => { insertRaw('bag_tracking_enabled', 'v'); expect(await appSettings.getValue('bag_tracking_enabled')).toBe('v'); // populate the identity map diff --git a/server/tests/unit/db/repositories/Categories.repository.test.ts b/server/tests/unit/db/repositories/Categories.repository.test.ts index 154a7ad03..969b47e4a 100644 --- a/server/tests/unit/db/repositories/Categories.repository.test.ts +++ b/server/tests/unit/db/repositories/Categories.repository.test.ts @@ -88,10 +88,9 @@ describe('CategoriesRepository', () => { expect(entity?.user?.isInitialized()).toBe(false); }); - // I1 (Task 0 review): findById carries `refresh: true`, the ruling every - // Plan 3 PK read follows — and it is load-bearing here (Task 3 review, - // Important 2's correction): `{ id }` is exactly the primary key, so - // without `refresh` this would be served from the identity map with + // Plan 3b Task 1 fix round (task-1-review.md B1): findById carries + // `disableIdentityMap: true` — `{ id }` is exactly the primary key, so + // without it this would risk being served from the identity map with // zero queries and the stale pre-UPDATE color. `queries === 1` proves // the real re-query happens. it('CATREPO-008: a raw UPDATE on the same row then findById reads the new value, in one query', async () => { diff --git a/server/tests/unit/db/repositories/Tags.repository.test.ts b/server/tests/unit/db/repositories/Tags.repository.test.ts index 3df590a40..eb834af0c 100644 --- a/server/tests/unit/db/repositories/Tags.repository.test.ts +++ b/server/tests/unit/db/repositories/Tags.repository.test.ts @@ -92,14 +92,10 @@ describe('TagsRepository', () => { expect(entity?.user.isInitialized()).toBe(false); }); - // I1 (Task 0 review): findByIdAndUser carries `{ refresh: true }` (via - // the shared findOwnedByUser helper), the ruling every Plan 3 PK read - // follows. This call's filter is `{ id, user }`, not PK-only, so it - // always re-queries and merges fresh data regardless of `refresh` (Task - // 3 review, Important 2's correction: the identity-map short-circuit - // fires only for an exactly-PK filter, not for a `fields` restriction) - // — this proves the (still correct) outcome in one query rather than a - // refresh-vs-no-refresh contrast. + // Plan 3b Task 1 fix round (task-1-review.md B1): findByIdAndUser + // carries `disableIdentityMap: true` (via the shared findOwnedByUser + // helper) — always re-queries and merges fresh data, in one query, and + // never lands in the request's identity map. it('TAGREPO-009: a raw UPDATE on the same row then findByIdAndUser reads the new value, in one query', async () => { const { user } = createUser(testDb); const created = createTag(testDb, user.id, { name: 'Stale', color: '#111111' }); diff --git a/server/tests/unit/db/repositories/Users.repository.test.ts b/server/tests/unit/db/repositories/Users.repository.test.ts index 4a651e1d7..8b7edb878 100644 --- a/server/tests/unit/db/repositories/Users.repository.test.ts +++ b/server/tests/unit/db/repositories/Users.repository.test.ts @@ -20,7 +20,8 @@ afterAll(async () => { await t.close(); testDb.close(); }); /** * Runs `fn` while counting queries MikroORM actually issues over the shared * connection — see `AppSettings.repository.test.ts` for why a value-only - * assertion can't catch the identity-map bug I1 fixed (Task 0 review). + * assertion can't catch a regression in the `disableIdentityMap: true` + * ruling (Plan 3b Task 1 fix round, `task-1-review.md` B1). */ async function withQueryCount(fn: () => Promise): Promise<{ value: T; queries: number }> { const connection = t.orm.em.getConnection(); @@ -70,11 +71,13 @@ describe('UsersRepository', () => { expect(await users.getApiKeyColumn(999999, 'maps_api_key')).toBeNull(); }); - // I1 (Task 0 review): getEmail/getApiKeyColumn are primary-key findOne - // calls, which MikroORM answers from the identity map on a repeat call - // unless `refresh: true` is set — invisible to a raw UPDATE on the same - // user id inside the same request. Pins the fix at one query each. - describe('getEmail/getApiKeyColumn see a raw UPDATE on the same user in the same request (I1, identity-map regression)', () => { + // Plan 3b Task 1 fix round (task-1-review.md B1): getEmail/getApiKeyColumn + // are primary-key findOne calls, which MikroORM would answer from the + // identity map on a repeat call absent `disableIdentityMap: true` — + // invisible to a raw UPDATE on the same user id inside the same request. + // Pins the fix at one query each; the property is preserved (B1's fix + // dropped `refresh: true`, not this guarantee). + describe('getEmail/getApiKeyColumn see a raw UPDATE on the same user in the same request (disableIdentityMap regression)', () => { it('USERSREPO-007: a raw UPDATE users SET email then getEmail reads the new email, in one query', async () => { const { user } = createUser(testDb, { email: 'old@example.com' }); expect(await users.getEmail(user.id)).toBe('old@example.com'); // populate the identity map @@ -200,6 +203,37 @@ describe('UsersRepository', () => { expect(row.role).toBe('admin'); expect(row.login_count).toBe(0); }); + + it('USERSREPO-057: insertUser (F2) never flushes an unrelated pending entity in the same EM', async () => { + // Simulate an unrelated pending change elsewhere in the request's EM — + // a managed entity persist()-ed but not yet flushed by its own code + // path. Before F2's fix, `insertUser`'s `persist().flush()` committed + // the WHOLE unit of work, so this row would land in the DB too. + const pending = t.em.create(Users, { + username: 'pending-unflushed', + email: 'pending@example.com', + password_hash: 'x', + role: 'user', + first_seen_version: '9.9.9', + login_count: 0, + }); + t.em.persist(pending); + + await users.insertUser({ + username: 'freshuser', + email: 'fresh@example.com', + password_hash: 'hashed', + role: 'user', + first_seen_version: '1.2.3', + }); + + const pendingRow = testDb.prepare('SELECT * FROM users WHERE username = ?').get('pending-unflushed'); + expect(pendingRow).toBeUndefined(); // insertUser must not have flushed it + const freshRow = testDb.prepare('SELECT * FROM users WHERE username = ?').get('freshuser'); + expect(freshRow).toBeDefined(); // but the intended row is there + + t.em.clear(); // discard the still-pending unflushed entity before the next test + }); }); describe('findByEmailCI (AU12/O4)', () => { @@ -218,6 +252,26 @@ describe('UsersRepository', () => { }); }); + // Plan 3b Task 1 fix round item 7 — AuthService.demoLogin's exact statement + // (`SELECT * FROM users WHERE email = ?`), missing from the inventory, + // added now so Task 5 has it. + describe('findByEmailExact (AuthService.demoLogin, inventory correction)', () => { + it('USERSREPO-057B: matches only the exact case, unlike every other email lookup here', async () => { + const { user } = createUser(testDb, { email: 'Demo.User@Example.com' }); + const row = await users.findByEmailExact('Demo.User@Example.com'); + expect(row?.id).toBe(user.id); + expect(await users.findByEmailExact('demo.user@example.com')).toBeNull(); // different case — no match + expect(await users.findByEmailExact('nope@example.com')).toBeNull(); + }); + + it('USERSREPO-057C: a guest row IS returned — no guest filter, unlike findByEmailCI/findForPasswordReset', async () => { + const { user } = createUser(testDb, { email: 'demoguest@example.com' }); + testDb.prepare('UPDATE users SET is_guest = 1 WHERE id = ?').run(user.id); + const row = await users.findByEmailExact('demoguest@example.com'); + expect(row?.id).toBe(user.id); + }); + }); + 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); @@ -315,7 +369,7 @@ describe('UsersRepository', () => { 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 () => { + it('USERSREPO-031: sees a raw UPDATE on the same id in the same request (disableIdentityMap 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); diff --git a/server/tests/unit/db/repositories/_shared/owned-lookup.test.ts b/server/tests/unit/db/repositories/_shared/owned-lookup.test.ts index b28592fdb..a3c77c221 100644 --- a/server/tests/unit/db/repositories/_shared/owned-lookup.test.ts +++ b/server/tests/unit/db/repositories/_shared/owned-lookup.test.ts @@ -60,27 +60,19 @@ describe('findOwnedByUser', () => { expect(await findOwnedByUser(tags, 999999, 'user', owner.id)).toBeNull(); }); - // Task 0 review I1's PK-read ruling, extended to this helper (Task 3): - // `options.refresh` passes through to `findOne`. This helper's filter is - // `{ id, [ownerField]: ownerId }`, not PK-only, so it always re-queries - // and merges fresh data regardless of `refresh` (proven below: a raw - // write on the same row is visible either way) — the identity-map - // short-circuit (and the staleness it can cause without `refresh`) fires - // only when the filter is exactly the primary key, which is what - // AppSettingsRepository.getValue's PK-only `findOne` hit and this - // multi-condition filter never does (Task 3 review, Important 2's - // correction — `fields` has nothing to do with it). `refresh` stays on - // this helper regardless, matching the Task 0 review's blanket PK-read - // ruling and staying correct if a future caller narrows the filter to - // PK-only. - it('OWNEDLOOKUP-006: { refresh: true } sees a raw UPDATE on the same row in the same request, in one query', async () => { + // Plan 3b Task 1 fix round (task-1-review.md B1): `findOwnedByUser` always + // passes `disableIdentityMap: true` now (no longer an opt-in `options` + // parameter) — every call answers from a throwaway forked context, so a + // raw write on the same row earlier in the request is always visible, in + // one query, and the entity never lands in the request's identity map. + it('OWNEDLOOKUP-006: sees a raw UPDATE on the same row in the same request, in one query (disableIdentityMap)', async () => { const { user: owner } = createUser(testDb); const tag = createTag(testDb, owner.id, { name: 'Old' }); - expect((await findOwnedByUser(tags, tag.id, 'user', owner.id))?.name).toBe('Old'); // populate the identity map + expect((await findOwnedByUser(tags, tag.id, 'user', owner.id))?.name).toBe('Old'); testDb.prepare('UPDATE tags SET name = ? WHERE id = ?').run('New', tag.id); const connection = t.orm.em.getConnection(); const spy = vi.spyOn(connection, 'execute'); - const found = await findOwnedByUser(tags, tag.id, 'user', owner.id, { refresh: true }); + const found = await findOwnedByUser(tags, tag.id, 'user', owner.id); expect(found?.name).toBe('New'); expect(spy.mock.calls.length).toBe(1); spy.mockRestore(); diff --git a/server/tests/unit/db/repositories/identity-map-writeback.test.ts b/server/tests/unit/db/repositories/identity-map-writeback.test.ts new file mode 100644 index 000000000..795a39214 --- /dev/null +++ b/server/tests/unit/db/repositories/identity-map-writeback.test.ts @@ -0,0 +1,100 @@ +/** + * Regression test for Plan 3b Task 1's B1 finding + * (`.superpowers/sdd/2026-09-22-orm-phase3b/task-1-review.md`): a `Users` + * row read with `fields` + `refresh: true` (no `disableIdentityMap`) left a + * *partially hydrated, managed* entity in the request `EntityManager`'s + * identity map. A second such read, with a *different* field set, on the + * *same* row re-snapshotted only its own fields — the first read's fields + * stayed on the entity object but fell out of the snapshot MikroORM's + * change-set computer diffs against, so they looked like a pending change. + * The next `flush()` (including the implicit one at `UnitOfWork.transactional` + * commit) then emitted an `UPDATE users SET …` carrying the + * STALE pre-request values, landing *after* an intervening `nativeUpdate` + * inside the same transaction and silently reverting it. + * + * No single-method test can catch this by construction (`Users.repository.test.ts` + * exercises one method per case): this file drives the two-reads-then-a-write + * interaction that is the actual trigger, inside `withRequestContext` (every + * authenticated request's shape) and `uow.transactional` (every multi-write + * request's shape). + * + * The fix (this task): every row-out read passes `disableIdentityMap: true`. + * MikroORM forks the EM with `keepTransactionContext: true`, loads inside the + * fork, then clears it — nothing is added to the *request's* identity map, so + * there is nothing for a later `flush()` to find dirty. + * See https://mikro-orm.io/docs/entity-manager#disableidentitymap and + * `node_modules/@mikro-orm/core/EntityManager.js:807-824`. + * + * Mutation proof (recorded in `task-1-fix-report.md`, not re-run here): reverting + * ONE of the two reads below to the pre-fix shape (`fields` + `refresh: true`, + * no `disableIdentityMap`) makes IMWB-001 fail with the stale write-back this + * test is built to catch. + */ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createUser } from '../../../helpers/factories'; +import { Users } from '../../../../src/db/entities/Users.entity'; +import type { UsersRepository } from '../../../../src/db/repositories/Users.repository'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; +import { withRequestContext } from '../../../../src/nest/database/request-context'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let users: UsersRepository; +let uow: UnitOfWork; + +beforeAll(async () => { + t = await createTestOrm(testDb); + users = t.repo(Users); + uow = new UnitOfWork(t.em); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +describe('identity-map write-back (task-1-review.md B1)', () => { + it('IMWB-001: findByIdWithPasswordVersion then getApiKeyColumns then patchProfile — the profile write survives the request', async () => { + const { user } = createUser(testDb, { username: 'before', email: 'before@example.test' }); + const originalPasswordVersion = (testDb.prepare('SELECT password_version FROM users WHERE id = ?').get(user.id) as { password_version: number }).password_version; + + await withRequestContext(t.orm, async () => { + // Projection A — the JwtAuthGuard/GlobalAuthGuard shape. + await users.findByIdWithPasswordVersion(user.id); + // Projection B — a second UsersRepository read with a different field set, + // the UserProfileService shape (e.g. the MfaPolicyGuard/getApiKeyColumns + // reads that run on the same request). + await users.getApiKeyColumns(user.id); + // The intended write, inside the request's own transaction. + await uow.transactional(async () => { + await users.patchProfile(user.id, { username: 'newname' }); + }); + }); + + const row = testDb + .prepare('SELECT username, email, password_version FROM users WHERE id = ?') + .get(user.id) as { username: string; email: string; password_version: number }; + expect(row.username).toBe('newname'); // the nativeUpdate must stick + expect(row.email).toBe('before@example.test'); // untouched by either read + expect(row.password_version).toBe(originalPasswordVersion); // untouched by either read + }); + + it('IMWB-002: findByIdWithPasswordVersion then getApiKeyColumns then setPassword — password_version survives (Task 5 session-invalidation case)', async () => { + const { user } = createUser(testDb, { username: 'someone' }); + + await withRequestContext(t.orm, async () => { + await users.findByIdWithPasswordVersion(user.id); + await users.getApiKeyColumns(user.id); + await uow.transactional(async () => { + await users.setPassword(user.id, 'NEWHASH', 5); + }); + }); + + const row = testDb + .prepare('SELECT password_hash, password_version, username FROM users WHERE id = ?') + .get(user.id) as { password_hash: string; password_version: number; username: string }; + expect(row.password_hash).toBe('NEWHASH'); + expect(row.password_version).toBe(5); // the session-invalidation gate — must NOT revert to the pre-change value + expect(row.username).toBe('someone'); // untouched by either read + }); +}); diff --git a/server/tests/unit/nest/auth/jwt-verify.test.ts b/server/tests/unit/nest/auth/jwt-verify.test.ts index 3ebf36d88..14d27e37e 100644 --- a/server/tests/unit/nest/auth/jwt-verify.test.ts +++ b/server/tests/unit/nest/auth/jwt-verify.test.ts @@ -37,8 +37,21 @@ function makeReq(overrides: { } as unknown as Request; } -/** A fake `UsersRepository` whose `findByIdWithPasswordVersion` returns the given row. */ -function usersRepo(row: UserWithPasswordVersion | null): UsersRepository { +/** + * A fake `UsersRepository` whose `findByIdWithPasswordVersion` returns the + * given row. + * + * `password_version` is widened to `number | null` here, unlike the real + * `UserWithPasswordVersion.password_version: number` — the column is + * `INTEGER NOT NULL DEFAULT 0`, so a real row can never carry `null`, but + * AUTH-JWT-009 (task-1-review.md F5) still needs to pin + * `verifyJwtAndLoadUser`'s `typeof row.password_version === 'number' ? … : + * 0` fallback branch, which is otherwise unreachable through the real type. + * Widening the fake, not `UserWithPasswordVersion` itself (which would + * loosen a production type for a test-only case), keeps that column's real + * NOT-NULL guarantee intact everywhere else. + */ +function usersRepo(row: (Omit & { password_version: number | null }) | null): UsersRepository { return { findByIdWithPasswordVersion: vi.fn(async () => row) } as unknown as UsersRepository; } @@ -125,8 +138,12 @@ describe('verifyJwtAndLoadUser', () => { 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 () => { - const users = usersRepo({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: 0 }); + it('AUTH-JWT-009: a pre-pv token still works against a row whose password_version reads as unset (both fall back to 0)', async () => { + // password_version: null pins the `typeof row.password_version === 'number' ? … : 0` + // fallback branch directly (task-1-review.md F5) — unreachable through + // the real UsersRepository, whose column is NOT NULL DEFAULT 0, but + // worth keeping covered rather than deleting the branch it guards. + const users = usersRepo({ id: 1, username: 'alice', email: 'alice@example.com', role: 'user', password_version: null }); const legacy = jwt.sign({ id: 1 }, 'test-secret', { algorithm: 'HS256' }); expect(await verifyJwtAndLoadUser(legacy, users)).not.toBeNull(); diff --git a/server/tests/unit/nest/files.service.test.ts b/server/tests/unit/nest/files.service.test.ts index 8f1cb235f..70ed9d42c 100644 --- a/server/tests/unit/nest/files.service.test.ts +++ b/server/tests/unit/nest/files.service.test.ts @@ -560,13 +560,17 @@ 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.anything()); + // The concrete expected argument, not expect.anything(): emStub.getRepository + // always returns a fresh `{}` (task-1-review.md F6) — deep-equal, not a + // reference match, so this pins WHICH repository (an empty object, this + // suite's stand-in for UsersRepository), not merely "a repository". + expect(verifyJwtAndLoadUser).toHaveBeenCalledWith('cookie-jwt', {}); }); 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.anything()); + expect(verifyJwtAndLoadUser).toHaveBeenCalledWith('bearer-jwt', {}); // see FILE-SVC-032's comment 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/platform.controller.test.ts b/server/tests/unit/nest/platform.controller.test.ts index 9a602638c..5f417cef2 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.anything()); + expect(h.verifyJwtAndLoadUser).toHaveBeenCalledWith('jwt123', expect.objectContaining({ findByIdWithPasswordVersion: expect.any(Function) })); 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.anything()); + expect(h.verifyJwtAndLoadUser).toHaveBeenCalledWith('qtok', expect.objectContaining({ findByIdWithPasswordVersion: expect.any(Function) })); expect(h.sendToResponse).toHaveBeenCalledWith('photos', 'a.jpg', res); });