fix(db): one account per email address, whatever its case

Login, password reset and the sign-up checks all match emails
case-insensitively, but the only constraint was the column's own
UNIQUE, which SQLite compares case-sensitively. Two accounts whose
addresses differ only in case (legacy rows, two sign-ups at once) left a
login to resolve to whichever row came first, and LOWER(email) lookups
had no index to use.

A migration adds a unique index on lower(email). An install that
already holds such a pair must still start, so the migration then
leaves the index out, logs how many addresses are shared and by which
user ids (not the addresses), and records email_case_index_pending.
Since a migration runs once, db/email-case-index.ts runs on every boot
while that flag is set: it repeats the warning until the accounts are
told apart, then creates the index and clears the flag. The app-level
checks keep guarding meanwhile.

A write that passes the up-front check and still loses the race is now
refused by the database. The users repository turns that unique
violation into UserIdentityTakenError, and sign-up, admin create and
edit, and the profile update answer it with the same 409 their checks
give, instead of a 500 or an unhandled rejection (ADMIN-SVC-094 pinned
the old behaviour and now pins the 409).

The entity generator could not see an expression index (PRAGMA
index_info names its column NULL), so PARITY-009c found a db index with
no entity declaration. Rule 14 carries such an index into the entity as
a named entry with its own statement; Users.entity.ts is regenerated.
This commit is contained in:
Maurice
2026-10-08 11:59:16 +02:00
parent c04fca81ee
commit bb5cc8f795
18 changed files with 488 additions and 56 deletions
+1 -1
View File
@@ -33,7 +33,7 @@ Nest owns everything. Every domain is a DI module under `src/nest/<domain>/` (`c
- **Scheduling**: `src/nest/scheduling/` (`CronRegistrarService`) is the one way to schedule a cron. Jobs are `*.job.ts` providers in their owning domain. The registrar refuses to schedule under `NODE_ENV=test` (an integration test pins it). **Never use a `@Cron` decorator** — it bypasses that gate.
- **Plugins** (`src/nest/plugins/`): the child-process runtime (`supervisor/`, `runtime/plugin-host-entry.ts`, `protocol/envelope.ts`) is **intentionally not Nest** — it is the security boundary. `host/rpc-host.ts` registers an RPC method at spawn only if the plugin holds the unlocking permission (registration = authorization). Install-time gates live in `install/`; the manifest's `trek` version range fails closed, and `TREK_PLUGINS_IGNORE_TREK_RANGE` downgrades that to a warning. The protocol tables in `protocol/envelope.ts` (+ `src/plugin-event-sink.ts`) are **generated** into `plugin-sdk/` and `shared/` — run `gen:plugin-facts` after touching them and commit the output. `runtime/egress-policy.ts` is a hand-kept mirror guarded by a parity test in `plugin-sdk/`; change both sides together.
- **Storage**: `src/nest/storage/` is the upload driver abstraction (local / S3 / mirror behind one interface, with an admin surface and a usage job). Domain services never touch the filesystem or the S3 SDK directly.
- **DB** (`src/db/`): `better-sqlite3`, WAL, FK on. `database.ts` only opens the connection and applies pragmas — it does not build a schema. MikroORM's migrations (`db/migrations/`) are the only schema source, **including for tests**: the hand-kept `schema.ts`/`migrations.ts`/`seeds.ts` builders and the parity test that guarded them are gone. Seeding is the MikroORM seeders in `db/seeders/`, applied by `db/orm.ts` from `buildApp()` before anything reads. An install the retired positional runner migrated (a `schema_version` row, no `mikro_orm_migrations` rows) is baselined by `db/legacy-baseline.ts`: the migrations whose `Legacy migration step N` docstring is ≤ its version are recorded as executed (never replayed) in the same transaction as the migrator run, while the baseline migration itself still runs first, as the legacy runner's `createTables()` did on every boot; anything it cannot map refuses the boot. Before a boot applies pending migrations to a file database that holds anything, `db/pre-migrate-snapshot.ts` writes a `VACUUM INTO` copy next to it (`pre-migrate-<schema>-<time>.db`, `<schema>` being `legacy-<schema_version>` or the last recorded migration's timestamp; the newest `TREK_DB_PRE_MIGRATE_SNAPSHOT_KEEP` stay) and the newer-database refusal names the matching copy; a copy that cannot be written refuses the boot unless `TREK_DB_PRE_MIGRATE_SNAPSHOT=false`. It is off under `NODE_ENV=test` unless a suite sets that variable to true, and a restore's re-bootstrap skips it (the archive is the copy). `db/orm-driver.ts` binds the ORM to that same connection instead of opening a second one — with `:memory:` under test, a second connection is a second, empty database. Every read and write goes through a repository in `db/repositories/` (each extends the shared `TrekRepository` base in `db/repositories/_shared/`, which validates the request context before delegating to MikroORM) and, for multi-statement writes, `UnitOfWork.transactional(...)` (`nest/database/unit-of-work.ts`, provided by the global `OrmModule`) — there is no `DatabaseService`/`DatabaseModule` to inject any more. Entrypoints outside an HTTP request — an MCP tool call, a plugin RPC dispatch, a WebSocket message, a cron job, boot-time seeding, a backup restore — open one explicitly with `withRequestContext()` (`nest/database/request-context.ts`), since only the pathless `mikroOrmRequestContext` middleware `buildApp()` mounts (with `@mikro-orm/nestjs`'s own `registerRequestContext` turned off in `app.module.ts`, UPLOADS-P16) forks an `EntityManager` per HTTP request. `NODE_ENV=test` ⇒ isolated `:memory:` DB per vitest worker, opened from a snapshot `tests/global-setup.ts` migrates once per run; `TREK_DB_FILE` ⇒ explicit file DB (Playwright harness).
- **DB** (`src/db/`): `better-sqlite3`, WAL, FK on. `database.ts` only opens the connection and applies pragmas — it does not build a schema. MikroORM's migrations (`db/migrations/`) are the only schema source, **including for tests**: the hand-kept `schema.ts`/`migrations.ts`/`seeds.ts` builders and the parity test that guarded them are gone. Seeding is the MikroORM seeders in `db/seeders/`, applied by `db/orm.ts` from `buildApp()` before anything reads. An install the retired positional runner migrated (a `schema_version` row, no `mikro_orm_migrations` rows) is baselined by `db/legacy-baseline.ts`: the migrations whose `Legacy migration step N` docstring is ≤ its version are recorded as executed (never replayed) in the same transaction as the migrator run, while the baseline migration itself still runs first, as the legacy runner's `createTables()` did on every boot; anything it cannot map refuses the boot. Before a boot applies pending migrations to a file database that holds anything, `db/pre-migrate-snapshot.ts` writes a `VACUUM INTO` copy next to it (`pre-migrate-<schema>-<time>.db`, `<schema>` being `legacy-<schema_version>` or the last recorded migration's timestamp; the newest `TREK_DB_PRE_MIGRATE_SNAPSHOT_KEEP` stay) and the newer-database refusal names the matching copy; a copy that cannot be written refuses the boot unless `TREK_DB_PRE_MIGRATE_SNAPSHOT=false`. It is off under `NODE_ENV=test` unless a suite sets that variable to true, and a restore's re-bootstrap skips it (the archive is the copy). After the migrator, `db/email-case-index.ts` creates the case-insensitive unique index on `users.email` that its migration had to defer on an install holding two accounts whose emails differ only in case (flag `email_case_index_pending`). `db/orm-driver.ts` binds the ORM to that same connection instead of opening a second one — with `:memory:` under test, a second connection is a second, empty database. Every read and write goes through a repository in `db/repositories/` (each extends the shared `TrekRepository` base in `db/repositories/_shared/`, which validates the request context before delegating to MikroORM) and, for multi-statement writes, `UnitOfWork.transactional(...)` (`nest/database/unit-of-work.ts`, provided by the global `OrmModule`) — there is no `DatabaseService`/`DatabaseModule` to inject any more. Entrypoints outside an HTTP request — an MCP tool call, a plugin RPC dispatch, a WebSocket message, a cron job, boot-time seeding, a backup restore — open one explicitly with `withRequestContext()` (`nest/database/request-context.ts`), since only the pathless `mikroOrmRequestContext` middleware `buildApp()` mounts (with `@mikro-orm/nestjs`'s own `registerRequestContext` turned off in `app.module.ts`, UPLOADS-P16) forks an `EntityManager` per HTTP request. `NODE_ENV=test` ⇒ isolated `:memory:` DB per vitest worker, opened from a snapshot `tests/global-setup.ts` migrates once per run; `TREK_DB_FILE` ⇒ explicit file DB (Playwright harness).
## Rules for new code
+73 -1
View File
@@ -768,6 +768,41 @@ export function RULE9_addImplicitUniqueConstraints(
}
}
/** One index over an expression rather than plain columns, as Rule 14 hands it in. */
export interface ExpressionIndex {
name: string;
unique: boolean;
/** The `CREATE [UNIQUE] INDEX` statement SQLite stored for it. */
sql: string;
}
/**
* Rule 14 (metadata level; DB-dependent half in `collectExpressionIndexes`):
* an index over an expression (`idx_users_email_lower ON users (lower(email))`)
* becomes a named `indexes:`/`uniques:` entry carrying its own `expression`.
*
* `SqliteSchemaHelper.getIndexes()` reads columns from `PRAGMA index_info`,
* which reports an expression column with a NULL name, so the introspected
* index has no column the generator can map and it never reaches the entity.
* PARITY-009c would then find a db index without an entity declaration. Any
* introspected entry under the same name is replaced, so the rule holds even
* if a later MikroORM keeps the broken entry instead of dropping it.
*/
export function RULE14_addExpressionIndexes(
metadata: EntityMetadata[],
expressionIndexes: ReadonlyMap<string, ExpressionIndex[]>,
): void {
for (const meta of metadata) {
const indexes = expressionIndexes.get(meta.tableName);
if (!indexes) continue;
for (const index of indexes) {
meta.indexes = meta.indexes.filter((existing) => existing.name !== index.name);
meta.uniques = meta.uniques.filter((existing) => existing.name !== index.name);
(index.unique ? meta.uniques : meta.indexes).push({ name: index.name, expression: index.sql });
}
}
}
/** A repository-type marker to inject into one entity class — Rule 10. */
export interface RepositoryTypeMarkerFixup {
className: string;
@@ -1210,6 +1245,7 @@ export function applyRules(
_platform: Platform,
implicitUniques: ReadonlyMap<string, string[][]> = new Map(),
fkDeleteRules: ReadonlyMap<string, ReadonlyMap<string, string>> = new Map(),
expressionIndexes: ReadonlyMap<string, ExpressionIndex[]> = new Map(),
): RuleFixups {
const retypedByRule1 = RULE1_fixUnknownScalarTypes(metadata);
const jsonColumns = RULE1b_markJsonColumns(metadata);
@@ -1230,6 +1266,9 @@ export function applyRules(
RULE7_dropNoActionRules(metadata);
RULE8_bindRepositories(metadata);
RULE9_addImplicitUniqueConstraints(metadata, implicitUniques);
// Like RULE9 it only appends to `meta.indexes`/`meta.uniques`, which the
// renderer dumps verbatim; no text pass depends on it.
RULE14_addExpressionIndexes(metadata, expressionIndexes);
const defaults = RULE_normalizeLiteralDefaults(metadata);
const repositoryMarkers = RULE10_repositoryTypeMarker(metadata);
// Runs after RULE5 so it reads the FINAL (post-rename) owning-relation
@@ -1296,6 +1335,38 @@ export async function collectImplicitUniqueIndexes(
return result;
}
/**
* DB-dependent half of Rule 14: every named index whose `PRAGMA index_info`
* has an expression column (a NULL name), with the statement `sqlite_master`
* stored for it, so `RULE14_addExpressionIndexes` stays a pure function.
*/
export async function collectExpressionIndexes(
connection: Connection,
tableNames: readonly string[],
): Promise<Map<string, ExpressionIndex[]>> {
const result = new Map<string, ExpressionIndex[]>();
for (const tableName of tableNames) {
const indexList = (await connection.execute(`pragma index_list(\`${tableName}\`)`, [], 'all')) as {
name: string;
unique: number;
}[];
const perTable: ExpressionIndex[] = [];
for (const idx of indexList) {
if (idx.name.startsWith('sqlite_autoindex_')) continue;
const info = (await connection.execute(`pragma index_info(\`${idx.name}\`)`, [], 'all')) as {
name: string | null;
}[];
if (!info.some((column) => column.name === null)) continue;
const rows = (await connection.execute(`select sql from sqlite_master where type = 'index' and name = ?`, [idx.name], 'all')) as {
sql: string;
}[];
perTable.push({ name: idx.name, unique: idx.unique === 1, sql: rows[0].sql });
}
if (perTable.length > 0) result.set(tableName, perTable);
}
return result;
}
/**
* DB-dependent half of Rule 13: for every table name given, `PRAGMA
* foreign_key_list`'s `on_delete` per `from` (owning) column, lowercased —
@@ -1953,7 +2024,8 @@ export async function generateEntities(): Promise<GenerateResult> {
const tableNames = metadata.map((meta) => meta.tableName);
const implicitUniques = await collectImplicitUniqueIndexes(orm.em.getConnection(), tableNames);
const fkDeleteRules = await collectForeignKeyDeleteRules(orm.em.getConnection(), tableNames);
fixups = applyRules(metadata, platform, implicitUniques, fkDeleteRules);
const expressionIndexes = await collectExpressionIndexes(orm.em.getConnection(), tableNames);
fixups = applyRules(metadata, platform, implicitUniques, fkDeleteRules, expressionIndexes);
},
});
+38
View File
@@ -0,0 +1,38 @@
import type { Connection } from '@mikro-orm/core';
/**
* The case-insensitive unique index on users.email, when its migration had to
* leave it out.
*
* `Migration20200101042400_one_account_per_email_whatever_its_case` creates
* `idx_users_email_lower` unless two accounts already share an address in
* different case; then it records `email_case_index_pending` and the install
* boots without it. A migration runs once, so this runs on every boot while
* that flag is set: once the accounts were told apart it creates the index and
* clears the flag, until then it repeats the warning.
*/
const PENDING_KEY = 'email_case_index_pending';
export async function ensureEmailCaseIndex(connection: Connection): Promise<void> {
const pending: unknown[] = await connection.execute(`SELECT value FROM app_settings WHERE key = ?`, [PENDING_KEY]);
if (pending.length === 0) return;
const duplicates: Array<{ ids: string }> = await connection.execute(
`SELECT group_concat(id, ', ') AS ids FROM (SELECT id, email FROM users ORDER BY id) GROUP BY lower(email) HAVING COUNT(*) > 1`,
);
if (duplicates.length > 0) {
console.warn(
`[DB] ${duplicates.length} email address(es) still belong to more than one account when case is ignored ` +
`(user ids ${duplicates.map((row) => row.ids).join('; ')}), so users.email has no case-insensitive unique ` +
'index yet. Give each of those accounts its own address under Admin > Users; the next start creates it.',
);
return;
}
await connection.transactional(async (trx) => {
await connection.execute('CREATE UNIQUE INDEX IF NOT EXISTS idx_users_email_lower ON users (lower(email))', [], 'run', trx);
await connection.execute('DELETE FROM app_settings WHERE key = ?', [PENDING_KEY], 'run', trx);
});
console.log('[DB] Every account has its own email address now; created the case-insensitive unique index on users.email');
}
+4
View File
@@ -207,6 +207,10 @@ export const UsersSchema = defineEntity({
},
{ properties: ['email'] },
{ properties: ['username'] },
{
name: 'idx_users_email_lower',
expression: 'CREATE UNIQUE INDEX idx_users_email_lower ON users (lower(email))',
},
],
properties: {
id: p.integer().primary(),
@@ -0,0 +1,45 @@
import { Migration } from '@mikro-orm/migrations';
/** One email address, lowered, that more than one account holds: the ids of those accounts. */
interface DuplicateRow {
ids: string;
}
/**
* One account per email address, whatever its case.
*
* Login, password reset and the sign-up checks all match emails
* case-insensitively, but the only constraint is the column's own UNIQUE,
* which SQLite compares case-sensitively. Two accounts whose emails differ
* only in case (a legacy row, two sign-ups at once) left a login to resolve to
* whichever row came first. An index on lower(email) closes that in the
* database, and gives the case-insensitive lookups an index to use.
*
* An install that already holds such a pair must still boot, so the index is
* then left out, the pair is named in the log, and `app_settings` remembers
* that it is owed: `db/email-case-index.ts` creates it on the first boot after
* the accounts were told apart. The app-level checks keep guarding meanwhile.
*/
export class Migration20200101042400_one_account_per_email_whatever_its_case extends Migration {
override name = 'Migration20200101042400_one_account_per_email_whatever_its_case';
override async up(): Promise<void> {
const duplicates = (await this.execute(
`SELECT group_concat(id, ', ') AS ids FROM (SELECT id, email FROM users ORDER BY id) ` +
'GROUP BY lower(email) HAVING COUNT(*) > 1',
)) as DuplicateRow[];
if (duplicates.length > 0) {
console.warn(
`[migration] ${duplicates.length} email address(es) belong to more than one account when case is ignored ` +
`(user ids ${duplicates.map((row) => row.ids).join('; ')}). The case-insensitive unique ` +
'index on users.email is not created yet. Give each of those accounts its own address under ' +
'Admin > Users (or delete the one nobody uses); the next start creates the index.',
);
this.addSql(`INSERT OR REPLACE INTO app_settings (key, value) VALUES ('email_case_index_pending', 'true')`);
return;
}
this.addSql('CREATE UNIQUE INDEX IF NOT EXISTS idx_users_email_lower ON users (lower(email))');
}
}
+3
View File
@@ -1,5 +1,6 @@
import mikroOrmConfig from '../mikro-orm.config';
import { registerReinitializeHook, runDemoSeed } from './database';
import { ensureEmailCaseIndex } from './email-case-index';
import { migrateToHead, NO_SAFETY_NET } from './legacy-baseline';
import { preMigrateSnapshot } from './pre-migrate-snapshot';
import { withRequestContext } from '../nest/database/request-context';
@@ -45,6 +46,8 @@ export async function runSchemaBootstrap(orm: AnyOrm, { snapshot = true }: { sna
// run would replay the whole history over it (see legacy-baseline.ts). Every
// other database just gets its pending migrations.
await migrateToHead(connection, migrator, undefined, undefined, snapshot ? preMigrateSnapshot(connection) : NO_SAFETY_NET);
// The one index a migration may have had to defer (see email-case-index.ts).
await ensureEmailCaseIndex(connection);
const defaultSeeder = orm.config.get('seeder').defaultSeeder;
if (defaultSeeder) await orm.seeder.seedString(defaultSeeder);
+50 -19
View File
@@ -2,6 +2,7 @@ import { Users } from '../entities/Users.entity';
import { toRow, type AssertRowKeys } from './_shared/rows';
import { coalesce, coalesceOverrideWhileSame, columnIncrementedBy, currentTimestamp, lower, lowerParam } from '../dialect/sql-functions';
import { TrekRepository } from './_shared/trek-repository';
import { UniqueConstraintViolationException } from '@mikro-orm/core';
/**
* The three third-party keys that belong to the instance rather than to a
@@ -239,6 +240,32 @@ export interface NewAdminUserRow {
role: string;
}
/**
* A write the database refused because another account already holds that
* email address (in any case, `idx_users_email_lower`) or username. The
* services check first and answer 409; this is the same answer for the write
* that lost a race against another one, instead of a 500.
*/
export class UserIdentityTakenError extends Error {
constructor(readonly field: 'email' | 'username') {
super(`${field} already taken`);
this.name = 'UserIdentityTakenError';
}
}
/** Runs a users write, turning a unique violation on email or username into {@link UserIdentityTakenError}. */
async function guardIdentity<T>(write: () => Promise<T>): Promise<T> {
try {
return await write();
} catch (err) {
if (err instanceof UniqueConstraintViolationException) {
if (/users\.email|idx_users_email/.test(err.message)) throw new UserIdentityTakenError('email');
if (/users\.username/.test(err.message)) throw new UserIdentityTakenError('username');
}
throw err;
}
}
/** AD11 (`admin.service.ts#updateUser`, inside `uow.transactional`) — the columns the legacy COALESCE-shaped UPDATE can touch; a key the caller omits is left unchanged, the same net effect as binding NULL into `COALESCE(?, column)`. */
export interface AdminEditPatch {
username?: string;
@@ -429,17 +456,19 @@ export class UsersRepository extends TrekRepository<Users> {
* `created_at` the same way a legacy INSERT-then-reselect pair did.
*/
async insertUser(row: NewUserRow): Promise<UserRow> {
const id = await this.insert({
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,
});
const id = await guardIdentity(() =>
this.insert({
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,
}),
);
const inserted = await this.findById(id);
if (!inserted) {
throw new Error('insertUser: read-back after insert found no row');
@@ -968,7 +997,7 @@ export class UsersRepository extends TrekRepository<Users> {
*/
async patchProfile(id: number, changes: UserProfilePatch): Promise<void> {
const platform = this.getEntityManager().getPlatform();
await this.nativeUpdate({ id }, { ...changes, updated_at: currentTimestamp(platform) });
await guardIdentity(() => this.nativeUpdate({ id }, { ...changes, updated_at: currentTimestamp(platform) }));
}
// ---------------------------------------------------------------------
@@ -1533,12 +1562,14 @@ export class UsersRepository extends TrekRepository<Users> {
* statement handled by {@link findAdminSummary}.
*/
async insertAdminCreatedUser(row: NewAdminUserRow): Promise<number> {
return await this.insert({
username: row.username,
email: row.email,
password_hash: row.password_hash,
role: row.role,
});
return await guardIdentity(() =>
this.insert({
username: row.username,
email: row.email,
password_hash: row.password_hash,
role: row.role,
}),
);
}
/**
@@ -1584,7 +1615,7 @@ export class UsersRepository extends TrekRepository<Users> {
*/
async applyAdminEdit(id: number, patch: AdminEditPatch): Promise<void> {
const platform = this.getEntityManager().getPlatform();
await this.nativeUpdate({ id }, { ...patch, updated_at: currentTimestamp(platform) });
await guardIdentity(() => this.nativeUpdate({ id }, { ...patch, updated_at: currentTimestamp(platform) }));
}
/**
+36 -19
View File
@@ -30,7 +30,7 @@ import { PermissionsService } from '../permissions/permissions.service';
import { PERMISSION_ACTIONS } from '../permissions/permissions.service';
import { NotificationsService } from '../notifications/notifications.service';
import { Users } from '../../db/entities/Users.entity';
import type { UsersRepository, AdminEditPatch } from '../../db/repositories/Users.repository';
import { UserIdentityTakenError, type UsersRepository, type AdminEditPatch } from '../../db/repositories/Users.repository';
import { AuditLog } from '../../db/entities/AuditLog.entity';
import type { AuditLogRepository } from '../../db/repositories/AuditLog.repository';
import { AppSettings } from '../../db/entities/AppSettings.entity';
@@ -99,6 +99,11 @@ const VERSION_FAILURE_TTL = 60_000;
* and module-scoped pieces (compareVersions, isDocker, the version cache) live
* in admin.helpers.ts.
*/
/** The 409 the up-front checks give, for a write that lost the race to another one. */
function identityTakenMessage(err: UserIdentityTakenError): string {
return err.field === 'email' ? 'Email already taken' : 'Username already taken';
}
@Injectable()
export class AdminService {
constructor(
@@ -175,9 +180,16 @@ export class AdminService {
const passwordHash = bcrypt.hashSync(password, BCRYPT_COST);
const insertedId = await this.users.insertAdminCreatedUser({
username, email, password_hash: passwordHash, role: data.role || 'user',
});
let insertedId: number;
try {
insertedId = await this.users.insertAdminCreatedUser({
username, email, password_hash: passwordHash, role: data.role || 'user',
});
} catch (err) {
// Another write took the name or address between the check and the insert.
if (err instanceof UserIdentityTakenError) return { error: identityTakenMessage(err), status: 409 };
throw err;
}
const user = await this.users.findAdminSummary(insertedId);
@@ -254,22 +266,27 @@ export class AdminService {
// (R4): the users UPDATE, the mcp_tokens DELETE, the oauth_tokens revoke
// and the push_subscriptions DELETE stay inside the SAME uow.transactional
// call, never split across separate un-transacted repository calls.
await this.uow.transactional(async () => {
await this.users.applyAdminEdit(userId, patch);
try {
await this.uow.transactional(async () => {
await this.users.applyAdminEdit(userId, patch);
if (password) {
// The version bump only invalidates JWT cookies. These two stores carry
// their own credentials and are revoked separately, exactly as the
// self-service paths do it.
await this.mcpTokens.deleteAllForUser(userId);
try {
await this.oauthTokens.revokeAllForUser(userId);
} catch { /* very old installs predate oauth_tokens */ }
// Push devices outlive every session, so the intruder's browser would
// keep receiving this account's notifications. They go with the rest.
await this.pushSubscriptions.deleteAllForUser(userId);
}
});
if (password) {
// The version bump only invalidates JWT cookies. These two stores carry
// their own credentials and are revoked separately, exactly as the
// self-service paths do it.
await this.mcpTokens.deleteAllForUser(userId);
try {
await this.oauthTokens.revokeAllForUser(userId);
} catch { /* very old installs predate oauth_tokens */ }
// Push devices outlive every session, so the intruder's browser would
// keep receiving this account's notifications. They go with the rest.
await this.pushSubscriptions.deleteAllForUser(userId);
}
});
} catch (err) {
if (err instanceof UserIdentityTakenError) return { error: identityTakenMessage(err), status: 409 };
throw err;
}
if (password) {
try { revokeUserSessions(Number(id)); } catch { /* best-effort, same as elsewhere */ }
+7 -2
View File
@@ -46,7 +46,7 @@ import { splitManagedKeys } from '../common/managed';
import { emitUserDeleted } from '../../plugin-user-lifecycle';
import { verifyJwtAndLoadUser } from './jwt-verify';
import { User } from '../../types';
import type { UserRow } from '../../db/repositories/Users.repository';
import { UserIdentityTakenError, type UserRow } from '../../db/repositories/Users.repository';
import { DEMO_EMAIL_PRIMARY, DEMO_PASS, isDemoEmail } from '../common/demo';
import { avatarUrl } from '../common/avatarUrl';
import { TripMembershipService } from '../trip-membership/trip-membership.service';
@@ -520,7 +520,12 @@ export class AuthService {
auditDetails: { username, email, role },
};
});
} catch {
} catch (err) {
// The check above runs in the transaction, but a concurrent signup can
// still win the insert; the database's unique indexes refuse the second.
if (err instanceof UserIdentityTakenError) {
return { error: 'Registration failed. Please try different credentials.', status: 409 };
}
return { error: 'Error creating user', status: 500 };
}
}
+13 -5
View File
@@ -19,7 +19,7 @@ import { SEARCH_TEXT_FIELD_MASK } from '../maps/maps.helpers';
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, UserApiKeyColumns, UserProfilePatch } from '../../db/repositories/Users.repository';
import { UserIdentityTakenError, type UsersRepository, type UserApiKeyColumns, type UserProfilePatch } from '../../db/repositories/Users.repository';
/** A key check is a single request to a provider; one that hangs must not hold the settings page. */
const KEY_CHECK_TIMEOUT_MS = 10_000;
@@ -233,10 +233,18 @@ export class UserProfileService {
const changedKeys = keyLocked ? [] : await this.changedKeyNames(body, current, isAdmin, blocked);
if (Object.keys(changes).length > 0) {
await this.uow.transactional(async () => {
await this.usersRepo.patchProfile(userId, changes);
if (!keyLocked) await this.mirrorInstanceKeys(body, isAdmin);
});
try {
await this.uow.transactional(async () => {
await this.usersRepo.patchProfile(userId, changes);
if (!keyLocked) await this.mirrorInstanceKeys(body, isAdmin);
});
} catch (err) {
// Another write took the name or address between the check above and this one.
if (err instanceof UserIdentityTakenError) {
return { error: err.field === 'email' ? 'Email already taken' : 'Username already taken', status: 409 };
}
throw err;
}
}
const updated = await this.usersRepo.findProfileWithKeys(userId);
@@ -0,0 +1,99 @@
/**
* `Migration20200101042400_one_account_per_email_whatever_its_case` and the
* boot step that finishes its job (`src/db/email-case-index.ts`): one account
* per email address whatever its case, without ever failing the boot of an
* install that already holds such a pair.
*/
import { ensureEmailCaseIndex } from '../../../src/db/email-case-index';
import { createMigrationOrm, migrateTo, pendingNames, rawExec, rawQuery } from '../../helpers/migration-step';
import type { MikroORM } from '@mikro-orm/sqlite';
import { afterEach, describe, expect, it, vi } from 'vitest';
const TARGET = 'Migration20200101042400_one_account_per_email_whatever_its_case';
let orm: MikroORM | null = null;
async function ormBeforeTarget(): Promise<MikroORM> {
orm = await createMigrationOrm();
const names = await pendingNames(orm);
const idx = names.indexOf(TARGET);
expect(idx).toBeGreaterThan(0);
await migrateTo(orm, names[idx - 1]);
return orm;
}
async function addUser(db: MikroORM, id: number, email: string): Promise<void> {
await rawExec(db, 'INSERT INTO users (id, username, email, password_hash) VALUES (?, ?, ?, ?)', [id, `user${id}`, email, 'x']);
}
const hasIndex = async (db: MikroORM) =>
(await rawQuery(db, `SELECT name FROM sqlite_master WHERE type = 'index' AND name = 'idx_users_email_lower'`)).length === 1;
const pendingFlag = async (db: MikroORM) =>
rawQuery<{ value: string }>(db, `SELECT value FROM app_settings WHERE key = 'email_case_index_pending'`);
afterEach(async () => {
vi.restoreAllMocks();
await orm?.close(true);
orm = null;
});
describe('one account per email address, whatever its case', () => {
it('EMAILCI-001: without case duplicates the index is created and refuses a second account in other case', async () => {
const db = await ormBeforeTarget();
await addUser(db, 1, 'Anna@Example.test');
await addUser(db, 2, 'bob@example.test');
await migrateTo(db, TARGET);
expect(await hasIndex(db)).toBe(true);
expect(await pendingFlag(db)).toEqual([]);
await expect(addUser(db, 3, 'anna@example.TEST')).rejects.toThrow(/UNIQUE constraint failed/);
// Mixed case itself is fine: only a second account for the same address is not.
expect(await rawQuery(db, 'SELECT email FROM users ORDER BY id')).toEqual([
{ email: 'Anna@Example.test' },
{ email: 'bob@example.test' },
]);
}, 30000);
it('EMAILCI-002: with case duplicates the boot goes on, names the accounts, and the index waits', async () => {
const db = await ormBeforeTarget();
await addUser(db, 1, 'anna@example.test');
await addUser(db, 2, 'Anna@Example.test');
await addUser(db, 3, 'carl@example.test');
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
await expect(migrateTo(db, TARGET)).resolves.toBeUndefined();
expect(await hasIndex(db)).toBe(false);
expect(await pendingFlag(db)).toEqual([{ value: 'true' }]);
const message = String(warn.mock.calls[0]?.[0]);
expect(message).toContain('1 email address(es) belong to more than one account');
expect(message).toContain('user ids 1, 2');
expect(message).not.toContain('anna@example.test');
}, 30000);
it('EMAILCI-003: the boot step repeats the warning while the pair remains, then creates the index once it is gone', async () => {
const db = await ormBeforeTarget();
await addUser(db, 1, 'anna@example.test');
await addUser(db, 2, 'ANNA@example.test');
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
vi.spyOn(console, 'log').mockImplementation(() => {});
await migrateTo(db, TARGET);
warn.mockClear();
await ensureEmailCaseIndex(db.em.getConnection());
expect(await hasIndex(db)).toBe(false);
expect(String(warn.mock.calls[0]?.[0])).toContain('still belong to more than one account');
await rawExec(db, `UPDATE users SET email = 'anna.work@example.test' WHERE id = 2`);
await ensureEmailCaseIndex(db.em.getConnection());
expect(await hasIndex(db)).toBe(true);
expect(await pendingFlag(db)).toEqual([]);
// Nothing owed any more: a later boot does nothing.
warn.mockClear();
await ensureEmailCaseIndex(db.em.getConnection());
expect(warn).not.toHaveBeenCalled();
}, 30000);
});
@@ -34,6 +34,7 @@ import {
RULE11_referencedColumns,
RULE12_fixGarbledCheckExpressions,
RULE13_pinDeleteRuleDrift,
RULE14_addExpressionIndexes,
RULE_normalizeLiteralDefaults,
applyTextPasses,
checkEntities,
@@ -694,6 +695,33 @@ describe('collectForeignKeyDeleteRules', () => {
});
});
describe('RULE14_addExpressionIndexes', () => {
const sql = 'CREATE UNIQUE INDEX idx_users_email_lower ON users (lower(email))';
it('RULE14-001: a unique expression index becomes a named uniques: entry carrying its statement', () => {
const meta = fixtureMeta('Users', 'users', [fixtureProp({ name: 'email', primary: false })]);
RULE14_addExpressionIndexes([meta], new Map([['users', [{ name: 'idx_users_email_lower', unique: true, sql }]]]));
expect(meta.uniques).toEqual([{ name: 'idx_users_email_lower', expression: sql }]);
expect(meta.indexes).toEqual([]);
});
it('RULE14-002: a plain expression index goes to indexes:, and an introspected entry of the same name is replaced', () => {
const meta = fixtureMeta('X', 'x', [fixtureProp({ name: 'name', primary: false })]);
meta.indexes.push({ name: 'idx_x_name_lower', properties: [] });
const plain = 'CREATE INDEX idx_x_name_lower ON x (lower(name))';
RULE14_addExpressionIndexes([meta], new Map([['x', [{ name: 'idx_x_name_lower', unique: false, sql: plain }]]]));
expect(meta.indexes).toEqual([{ name: 'idx_x_name_lower', expression: plain }]);
expect(meta.uniques).toEqual([]);
});
it('RULE14-003: a table without expression indexes is left alone', () => {
const meta = fixtureMeta('X', 'x', [fixtureProp({ name: 'name', primary: false })]);
RULE14_addExpressionIndexes([meta], new Map([['users', [{ name: 'idx_users_email_lower', unique: true, sql }]]]));
expect(meta.uniques).toEqual([]);
expect(meta.indexes).toEqual([]);
});
});
describe('RULE9_addImplicitUniqueConstraints', () => {
it('RULE9-001: a table with a matching implicit-unique entry gets a uniques: block referencing the columns as properties — a plain (non-FK) column keeps its own name', () => {
const userId = fixtureProp({ name: 'user_id', primary: false });
@@ -35,6 +35,7 @@ const UNNUMBERED = [
'Migration20200101042100_a_trip_reminder_remembers_it_was_sent',
'Migration20200101042200_an_index_on_every_foreign_key',
'Migration20200101042300_every_tour_type_the_contract_names',
'Migration20200101042400_one_account_per_email_whatever_its_case',
];
// The last step the positional runner ever had; planLegacyBaseline refuses a
// schema_version past it.
@@ -5,6 +5,7 @@ import { createTestOrm, type TestOrm } from '../../../helpers/test-orm';
import { createUser, createAdmin, type TestUser } from '../../../helpers/factories';
import { Users } from '../../../../src/db/entities/Users.entity';
import type { UsersRepository } from '../../../../src/db/repositories/Users.repository';
import { UserIdentityTakenError } from '../../../../src/db/repositories/Users.repository';
const testDb = createSnapshotTestDb();
let t: TestOrm;
@@ -1202,3 +1203,25 @@ describe('UsersRepository — Immich settings write (IM4/IM5)', () => {
expect(read(user.id)).toEqual({ immich_url: null, immich_api_key: null, immich_allow_insecure_tls: 0 });
});
});
describe('a users write the unique indexes refuse', () => {
const row = (username: string, email: string) => ({
username, email, password_hash: 'hashed', role: 'user', first_seen_version: '1.0.0',
});
it('USERSREPO-IDENT-001: an email another account holds in different case is refused as the email', async () => {
createUser(testDb, { username: 'anna', email: 'anna@example.com' });
const refused = users.insertUser(row('anna2', 'ANNA@Example.com'));
await expect(refused).rejects.toBeInstanceOf(UserIdentityTakenError);
await expect(refused).rejects.toMatchObject({ field: 'email' });
});
it('USERSREPO-IDENT-002: a taken username is refused as the username, on insert and on update', async () => {
createUser(testDb, { username: 'anna', email: 'anna@example.com' });
const { user } = createUser(testDb, { username: 'carl', email: 'carl@example.com' });
await expect(users.insertAdminCreatedUser({ username: 'anna', email: 'new@example.com', password_hash: 'x', role: 'user' }))
.rejects.toMatchObject({ field: 'username' });
await expect(users.patchProfile(user.id, { email: 'Anna@example.com' })).rejects.toMatchObject({ field: 'email' });
await expect(users.applyAdminEdit(user.id, { username: 'anna' })).rejects.toMatchObject({ field: 'username' });
});
});
+32 -9
View File
@@ -10,6 +10,7 @@
* (it replaced ADMIN-BR-001 when the old admin bridge died with the cron move).
*/
import { describe, it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from 'vitest';
import { UsersRepository } from '../../../src/db/repositories/Users.repository';
import { ADDON_IDS, MCP_GATED_ADDON_IDS } from '../../../src/addons';
// ── DB setup ──────────────────────────────────────────────────────────────────
@@ -964,8 +965,8 @@ describe('updateUser — password-reset transaction boundary (AD11/12/13)', () =
});
});
describe('createUser — the pre-existing uniqueness-check TOCTOU window (AD2-4, R4 "report, don\'t fix")', () => {
it('ADMIN-SVC-094 — a race between the email pre-check and the INSERT is not caught by a transaction; the row-level UNIQUE constraint is what actually stops the duplicate, surfacing as a thrown error rather than a clean 409', async () => {
describe('createUser: the uniqueness-check TOCTOU window (AD2-4)', () => {
it('ADMIN-SVC-094: a race between the email pre-check and the INSERT is stopped by the row-level UNIQUE constraint and answered with the same clean 409', async () => {
const { user: existing } = createUser(testDb, { email: 'race@test.example.com' });
// Simulates the concurrent-request race AD2-4's non-transactional shape
@@ -978,15 +979,13 @@ describe('createUser — the pre-existing uniqueness-check TOCTOU window (AD2-4,
const spy = vi.spyOn(usersRepo, 'findIdByEmailCI').mockResolvedValueOnce(null);
// The pre-check said "clear," but `users.email` carries its own UNIQUE
// index (`Migration20200101000000_baseline_schema.ts`) — that's what
// actually stops the duplicate, at the SQL level, not this service's own
// TOCTOU-vulnerable pre-check. Because nothing here catches that
// failure, it propagates as an unhandled rejection rather than the
// clean `{ error: 'Email already taken', status: 409 }` a request that
// lost the race with more lead time would have gotten.
// index (`Migration20200101000000_baseline_schema.ts`), and that is what
// actually stops the duplicate, at the SQL level. The repository turns
// the refusal into UserIdentityTakenError, so the request that lost the
// race gets the same answer as one that lost it with more lead time.
await expect(
svcCreateUser({ username: 'raceuser', email: existing.email, password: 'ValidPass1!' }),
).rejects.toThrow();
).resolves.toEqual({ error: 'Email already taken', status: 409 });
spy.mockRestore();
});
@@ -1002,3 +1001,27 @@ describe('createUser — the pre-existing uniqueness-check TOCTOU window (AD2-4,
expect(existing.id).toBeGreaterThan(0);
});
});
describe('a create or edit that loses the race for an email or username', () => {
afterEach(() => {
vi.restoreAllMocks();
});
it('ADMIN-IDENT-001: createUser answers the same 409 when the database refuses what the check missed', async () => {
createUser(testDb, { username: 'anna', email: 'anna@example.com' });
// The other write lands between the check and the insert.
vi.spyOn(UsersRepository.prototype, 'findIdByEmailCI').mockResolvedValue(null);
expect(await svcCreateUser({ username: 'anna2', email: 'ANNA@example.com', password: 'Secure123!' })).toEqual({
error: 'Email already taken',
status: 409,
});
});
it('ADMIN-IDENT-002: updateUser does the same, and changes nothing', async () => {
createUser(testDb, { username: 'anna', email: 'anna@example.com' });
const { user } = createUser(testDb, { username: 'carl', email: 'carl@example.com' });
vi.spyOn(UsersRepository.prototype, 'findIdByEmailCI').mockResolvedValue(null);
expect(await updateUser(String(user.id), { email: 'Anna@Example.com' })).toEqual({ error: 'Email already taken', status: 409 });
expect(testDb.prepare('SELECT email FROM users WHERE id = ?').get(user.id)).toEqual({ email: 'carl@example.com' });
});
});
@@ -60,6 +60,7 @@ vi.mock('../../../src/mcp/sessionManager', () => ({ revokeUserSessions: vi.fn()
import { db as testDb } from '../../../src/db/database';
import { describe, it, expect, beforeAll, beforeEach, afterEach, afterAll, vi } from 'vitest';
import { UsersRepository } from '../../../src/db/repositories/Users.repository';
import { resetTestDb } from '../../helpers/test-db';
import { createUser, createAdmin, createInviteToken, createTrip, createPlace, createReservation } from '../../helpers/factories';
import { AuthService } from '../../../src/nest/auth/auth.service';
@@ -1251,3 +1252,18 @@ describe('generateToken remember claim (#1927)', () => {
expect(decoded.exp - decoded.iat).toBe(86400);
});
});
describe('registerUser loses the race for an email', () => {
afterEach(() => {
vi.restoreAllMocks();
});
it('AUTH-IDENT-001: a signup the database refuses after the check passed answers the same 409, not a 500', async () => {
createUser(testDb, { username: 'anna', email: 'anna@x.com' });
vi.spyOn(UsersRepository.prototype, 'findIdByEmailOrUsernameCI').mockResolvedValue(null);
expect(await svc.registerUser({ username: 'anna2', email: 'ANNA@x.com', password: 'Secure123!' })).toEqual({
error: 'Registration failed. Please try different credentials.',
status: 409,
});
});
});
@@ -38,6 +38,7 @@ import { createUser, createAdmin } from '../../helpers/factories';
import fs from 'node:fs';
import path from 'node:path';
import { UserProfileService } from '../../../src/nest/auth/user-profile.service';
import { UsersRepository } from '../../../src/db/repositories/Users.repository';
import { makeStorageFixture } from '../../helpers/storage-fixture';
import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow';
import { SEARCH_TEXT_FIELD_MASK } from '../../../src/nest/maps/maps.helpers';
@@ -505,3 +506,17 @@ describe('changedKeys', () => {
}
});
});
describe('updateSettings loses the race for an email', () => {
it('AUTH-IDENT-002: the database refusing what the check missed answers the same 409', async () => {
createUser(testDb, { email: 'anna@example.com' });
const { user } = createUser(testDb, { email: 'carl@example.com' });
const check = vi.spyOn(UsersRepository.prototype, 'findIdByEmailCI').mockResolvedValue(null);
try {
const result = await profile.updateSettings(user.id, { email: 'ANNA@example.com' });
expect(result).toEqual({ error: 'Email already taken', status: 409 });
} finally {
check.mockRestore();
}
});
});
+4
View File
@@ -32,6 +32,10 @@ Click the pencil icon (**Edit User**) on any row to open the edit dialog. You ca
Click **Save** to apply changes.
An email address belongs to one account only, and case does not count: `Anna@Example.com` and `anna@example.com` are the same address. Saving an address another account already holds answers **Email already taken**, and creating such an account does the same.
If an install from before 4.4 already holds two accounts whose addresses differ only in case, the update still starts, and the log names their user ids: `email address(es) belong to more than one account when case is ignored`. Give each of those accounts its own address here (or delete the one nobody uses). The next start then adds the database rule that keeps it that way.
Below the fields sits **Reset passkeys**, which removes every passkey that user has registered: the recovery path when someone loses their authenticator. It asks for confirmation in a dialog, reports how many passkeys it removed, and takes effect immediately rather than on **Save**. The user can still sign in with their password. See [Passkeys](Passkeys).
### Delete a user