diff --git a/server/src/db/repositories/InviteTokens.repository.ts b/server/src/db/repositories/InviteTokens.repository.ts index b92462798..69ca9f47c 100644 --- a/server/src/db/repositories/InviteTokens.repository.ts +++ b/server/src/db/repositories/InviteTokens.repository.ts @@ -17,6 +17,17 @@ export interface InviteTokenRow { const _inviteTokenRowKeys: AssertRowKeys = true; +/** + * RI1/RI5's joined projection: `invite_tokens` plus the creator's username + * and the bound trip's title (if any). `created_by_name` mirrors an admin + * invite always having a creator (INNER `JOIN users`); `trip_title` is + * `null` when the invite isn't bound to a trip (`LEFT JOIN trips`). + */ +export interface InviteWithCreatorAndTripRow extends InviteTokenRow { + created_by_name: string; + trip_title: string | null; +} + /** The column set `RegistrationInvitesService.createInvite` (Task 3) writes. */ export interface NewInviteTokenRow { token: string; @@ -45,13 +56,17 @@ export class InviteTokensRepository extends EntityRepository { * and `OidcService.findOrCreateUser` (O11). A name promising "valid" would * describe a filter this statement doesn't have. * - * Not a primary-key filter — `token` is a unique column, not `id` — so this - * always re-queries; no `{ refresh: true }` needed (Task 3a review's - * precision ruling: the identity-map short-circuit only fires for a - * PK-only filter). + * Not a primary-key filter — `token` is a unique column, not `id`. + * `disableIdentityMap: true`, not `{ refresh: true }`: a plain `refresh` + * re-snapshots only the selected fields on an already identity-mapped + * entity, which risks a stale-write-back on the request's closing + * `flush()` if this same row is (or later becomes) managed under a + * different projection in the same transaction — reproduced live on + * `users` (see `WebauthnCredentialsRepository.hasAny`'s docstring). An + * isolated, unmanaged read can never be part of that flush. */ async findByToken(token: string): Promise { - const invite = await this.findOne({ token }); + const invite = await this.findOne({ token }, { disableIdentityMap: true }); return invite ? (toRow(invite) as InviteTokenRow) : null; } @@ -63,18 +78,25 @@ export class InviteTokensRepository extends EntityRepository { * `Categories.user`/`user_id`); the repository writes them through the * `Ref`-typed relation properties, which MikroORM accepts a raw primary * key for. + * + * `EntityRepository.insert()`, not `create()` + `persist().flush()` + + * `refresh()`: a native insert with no identity-map/UnitOfWork side + * effects — `persist().flush()` flushes the ENTIRE request's UnitOfWork, + * which can write back a stale identity-mapped entity from an unrelated + * earlier read in the same request (see `WebauthnCredentialsRepository + * .hasAny`'s docstring). The re-select after `insert()` uses + * `disableIdentityMap: true` for the same reason `findByToken` does. */ async insertInvite(row: NewInviteTokenRow): Promise { - const invite = this.create({ + const id = await this.insert({ token: row.token, max_uses: row.max_uses, expires_at: row.expires_at, createdByRef: row.created_by, trip: row.trip_id ?? null, }); - await this.getEntityManager().persist(invite).flush(); - await this.getEntityManager().refresh(invite); - return toRow(invite) as InviteTokenRow; + const invite = await this.findOne({ id }, { disableIdentityMap: true }); + return toRow(invite!) as InviteTokenRow; } /** @@ -116,4 +138,76 @@ export class InviteTokensRepository extends EntityRepository { .execute('run'); return result.row ? (result.row as InviteTokenRow) : null; } + + // --------------------------------------------------------------------- + // RI1 — admin invite list (Task 3, RegistrationInvitesService.listInvites) + // --------------------------------------------------------------------- + + /** + * ```sql + * SELECT i.*, u.username as created_by_name, t.title as trip_title + * FROM invite_tokens i + * JOIN users u ON i.created_by = u.id + * LEFT JOIN trips t ON i.trip_id = t.id + * ORDER BY i.created_at DESC + * ``` + * + * A QueryBuilder join (D4's T5 tier), projection root `InviteTokens` — + * `i.createdByRef`/`i.trip` are the entity's own relation properties for + * exactly these two joins, so no hand-spelled join condition is needed. + * `mapResults: false` (the same choice `TripsRepository.findAccessible` + * makes for its own aliased join) leaves the driver's row alone: the keys + * are the column names the `AS` aliases spell, which is why the result + * type is hand-written here rather than inferred from the entity. + */ + async listWithCreatorAndTrip(): Promise { + return this.qb('i') + .join('i.createdByRef', 'u') + .leftJoin('i.trip', 't') + .select(['i.*', 'u.username as created_by_name', 't.title as trip_title']) + .orderBy({ 'i.created_at': 'desc' }) + .execute('all', false); + } + + // --------------------------------------------------------------------- + // RI5 — createInvite's re-select (same JOIN shape, filtered to one row) + // --------------------------------------------------------------------- + + /** RI1's exact JOIN shape, `WHERE i.id = ?` — the response of `createInvite`. */ + async findWithCreatorAndTrip(id: number): Promise { + const row = await this.qb('i') + .join('i.createdByRef', 'u') + .leftJoin('i.trip', 't') + .select(['i.*', 'u.username as created_by_name', 't.title as trip_title']) + .where({ 'i.id': id }) + .execute('get', false); + return row ?? null; + } + + // --------------------------------------------------------------------- + // RI6 — deleteInvite's 404 check + // --------------------------------------------------------------------- + + /** + * `SELECT id FROM invite_tokens WHERE id = ?`. Named `findIdById`, not + * `findById` — `findById` would read like it belonged next to + * `UsersRepository.findById`'s full-row shape; this is a narrow + * existence probe for one column, and neither name shadows an + * `EntityRepository` method (there is no base `findById`/`deleteById` + * to collide with — the distinct names are for a human reading two + * unrelated repositories side by side, not a compiler conflict). + */ + async findIdById(id: number): Promise { + const row = await this.findOne({ id }, { fields: ['id'], disableIdentityMap: true }); + return row ? row.id : null; + } + + // --------------------------------------------------------------------- + // RI7 — deleteInvite's delete + // --------------------------------------------------------------------- + + /** `DELETE FROM invite_tokens WHERE id = ?` */ + async deleteById(id: number): Promise { + return this.nativeDelete({ id }); + } } diff --git a/server/src/db/repositories/WebauthnChallenges.repository.ts b/server/src/db/repositories/WebauthnChallenges.repository.ts index e89bd57cf..8382be217 100644 --- a/server/src/db/repositories/WebauthnChallenges.repository.ts +++ b/server/src/db/repositories/WebauthnChallenges.repository.ts @@ -1,4 +1,98 @@ import type { WebauthnChallenges } from '../entities/WebauthnChallenges.entity'; import { EntityRepository } from '@mikro-orm/sql'; -export class WebauthnChallengesRepository extends EntityRepository {} +/** `PasskeyService.storeChallenge` (PK2). */ +export interface NewChallengeRow { + challenge: string; + user_id: number | null; + type: 'registration' | 'authentication'; + expires_at: number; +} + +/** + * The `webauthn_challenges` table shape as `claimChallenge`'s + * `em.getKysely()` call needs it — an explicit type argument rather than + * relying on `getKysely()`'s automatic entity-metadata inference (kept + * narrow to the one column this repository ever reads back). + */ +interface WebauthnChallengesKyselyDB { + webauthn_challenges: { + challenge: string; + user_id: number | null; + type: string; + expires_at: number; + }; +} + +/** The single-use, TTL'd WebAuthn ceremony challenge store — PK1–PK3. */ +export class WebauthnChallengesRepository extends EntityRepository { + /** + * `DELETE FROM webauthn_challenges WHERE expires_at < ?` — best-effort + * sweep run before minting a new challenge (no cron for this table; see + * the inventory's surprise #9). + */ + async purgeExpired(now: number): Promise { + await this.nativeDelete({ expires_at: { $lt: now } }); + } + + /** + * `INSERT INTO webauthn_challenges (challenge, user_id, type, expires_at) + * VALUES (?, ?, ?, ?)`. `EntityRepository.insert()`, not `create()` + + * `persist().flush()` — a native insert with no identity-map/UnitOfWork + * side effects (see `WebauthnCredentialsRepository.insertCredential`'s + * docstring for why `persist().flush()` is the hazard here: it flushes + * the entire request's UnitOfWork, which can write back a stale + * identity-mapped entity from an unrelated earlier read in the same + * request). + */ + async insertChallenge(row: NewChallengeRow): Promise { + await this.insert({ + challenge: row.challenge, + user: row.user_id, + type: row.type, + expires_at: row.expires_at, + }); + } + + /** + * `DELETE FROM webauthn_challenges WHERE challenge = ? AND type = ? AND + * expires_at > ? RETURNING user_id` — the anti-replay primitive: it runs + * BEFORE any async verification, so a concurrent double-submit of the + * same assertion can spend the row exactly once. + * + * Built on `em.getKysely()` (D4's T6 tier), not the QueryBuilder's own + * `.returning()` (the InviteTokensRepository.incrementUsedCount shape): + * verified empirically against the installed MikroORM 7.2.1 that the + * QueryBuilder's `processReturningStatement` (`@mikro-orm/sql`'s + * `QueryBuilder.js`) only ever honours an explicit `.returning()` hint + * for `INSERT`/`UPDATE` — a bare `DELETE` has no `data` in its internal + * state, and the very first branch of that method (`if (!data && + * !this.#state.insertSubQuery) return;`) exits before the "always + * respect explicit returning hint" branch below it is ever reached. A + * `delete().returning([...]).execute('run')` therefore silently drops + * the RETURNING clause: it deletes the row (`affectedRows: 1`) but + * `result.row`/`result.rows` come back empty every time — this is NOT + * the `update().returning()` shape Task 0 proved; it's a different, + * narrower gap the docs don't call out. `em.getKysely()` is bound to the + * ambient transaction the same way the QueryBuilder is (see its own + * doc comment — inside `em.transactional`/`uow.transactional` it reuses + * the open transaction's connection), so D6's single-connection- + * serialises-writers guarantee still holds: no SELECT→await→DELETE + * window for either caller to lose, proven by `WEBAUTHN-CHAL-REPO`'s + * concurrent-claim test asserting exactly one non-null winner. + * + * `.returning(['user_id'])` only (not `.selectAll()`/`returningAll()`), + * matching the legacy statement's own narrower `RETURNING user_id`. + */ + async claimChallenge(challenge: string, type: 'registration' | 'authentication', now: number): Promise<{ user_id: number | null } | null> { + const row = await this.getEntityManager() + .getKysely() + .deleteFrom('webauthn_challenges') + .where('challenge', '=', challenge) + .where('type', '=', type) + .where('expires_at', '>', now) + .returning(['user_id']) + .executeTakeFirst(); + return row ?? null; + } +} diff --git a/server/src/db/repositories/WebauthnCredentials.repository.ts b/server/src/db/repositories/WebauthnCredentials.repository.ts index 71768e484..e86d7419b 100644 --- a/server/src/db/repositories/WebauthnCredentials.repository.ts +++ b/server/src/db/repositories/WebauthnCredentials.repository.ts @@ -1,28 +1,260 @@ import type { WebauthnCredentials } from '../entities/WebauthnCredentials.entity'; +import { toRow, type AssertRowKeys } from './_shared/rows'; +import { currentTimestamp } from '../dialect/sql-functions'; import { EntityRepository } from '@mikro-orm/sql'; /** - * Plan 3b Task 1 reserves this class (empty stub since Plan 3a) and implements - * exactly the one method the AUTH cluster's Task 1 needs NOW: `mfa-policy.guard.ts` - * (MP3) and, cross-service, `AuthService.updateAppSettings` (AU24, Task 5) both - * read "does this user own at least one passkey" — a passkey satisfies the - * `require_mfa` policy the same way TOTP does. Task 3 (`PasskeyService`) - * extends this class with the rest of the WebAuthn ceremony state - * (`listByUser`, `create`, `findByCredentialId`, `updateCounterAndLastUsed`, - * `renameOwned`, `deleteOwned`, `deleteAllForUser`) — this method's name and - * shape are chosen so nothing here needs to change when that lands. + * A full `webauthn_credentials` row (`SELECT * ...`), as PK9 (`passkeyLoginVerify`) + * needs it — `public_key` stays a `Buffer` end to end: `BlobType` (the entity's + * `p.blob()` column type) has no `toJSON` override, so `EntityTransformer`'s + * default (return the value unchanged) applies and `toRow`'s `wrap().toObject()` + * never touches the bytes. + */ +export interface WebauthnCredentialRow { + id: number; + user_id: number; + credential_id: string; + public_key: Buffer; + counter: number; + transports: string | null; + device_type: string | null; + backed_up: number; + name: string | null; + aaguid: string | null; + created_at: string | null; + last_used_at: string | null; +} + +const _webauthnCredentialRowKeys: AssertRowKeys = true; + +/** `PasskeyService.passkeyRegisterVerify` (PK7) — only verifier-vouched fields. */ +export interface NewCredentialRow { + user_id: number; + credential_id: string; + public_key: Buffer; + counter: number; + transports: string | null; + device_type: string | null; + backed_up: number; + name: string | null; + aaguid: string | null; +} + +/** PK5's `excludeCredentials` shape and PK8/PK13's shared management-panel shape. */ +export interface CredentialTransportsRow { + credential_id: string; + transports: string | null; +} + +/** The column set PK8 (post-registration re-select) and PK13 (`listPasskeys`) both project. */ +export interface PasskeyPanelRow { + id: number; + name: string | null; + device_type: string | null; + backed_up: number; + created_at: string | null; + last_used_at: string | null; +} + +const PANEL_FIELDS = ['id', 'name', 'device_type', 'backed_up', 'created_at', 'last_used_at'] as const; + +function toPanelRow(row: { id: number; name?: string | null; device_type?: string | null; backed_up: number; created_at?: string | null; last_used_at?: string | null }): PasskeyPanelRow { + return { + id: row.id, + name: row.name ?? null, + device_type: row.device_type ?? null, + backed_up: row.backed_up, + created_at: row.created_at ?? null, + last_used_at: row.last_used_at ?? null, + }; +} + +/** + * Plan 3b Task 1 reserved this class and implemented `hasAny` (MP3/AU24 — + * shared with `mfa-policy.guard.ts` and `AuthService.updateAppSettings`). + * Task 3 (`PasskeyService`) adds everything else: the full WebAuthn + * credential-store surface (PK5–PK18, minus PK6's dup check which the + * service still gates the same insert with, race-proofed by the surrounding + * `uow.transactional`). `hasAny` is untouched — nothing here changes its + * name or shape. */ export class WebauthnCredentialsRepository extends EntityRepository { /** * `SELECT 1 FROM webauthn_credentials WHERE user_id = ? LIMIT 1` (MP3/AU24 - * — identical statement, two callers). `count` with an implicit `LIMIT 1` - * via `findOne` mirrors the legacy existence probe without pulling a row; - * MikroORM's `count()` has no built-in short-circuit, so `findOne` with a - * narrow `fields` selection is the closer match to "does one exist" than - * `count() > 0`, which would still scan/aggregate every matching row. + * — identical statement, two callers). `findOne` with a narrow `fields` + * selection mirrors the legacy existence probe without pulling a full + * row; MikroORM's `count()` has no built-in short-circuit, so this is the + * closer match to "does one exist" than `count() > 0`, which would still + * scan/aggregate every matching row. + * + * `disableIdentityMap: true` (every row-out read in this file): this + * method's caller (`mfa-policy.guard.ts`, `AuthService.updateAppSettings`) + * can run in the same request as other `webauthn_credentials`/`users` + * reads and writes; a `findOne` that joins the identity map (even for a + * `fields: ['id']` existence probe) registers a managed entity whose + * unselected columns hold whatever the query returned as undefined/stale + * — if that entity is later merged with a broader projection AND a + * sibling `nativeUpdate` changes a column out of band in the same + * transaction, the closing `flush()` can write the identity-mapped + * entity's stale in-memory snapshot back over the `nativeUpdate`'s write + * (reproduced live: `PUT /api/auth/me/settings` returning 200 while + * discarding the change). `disableIdentityMap: true` returns an isolated, + * unmanaged entity instead — nothing this method reads is ever merged + * into the request's identity map, so it can never be flushed. */ async hasAny(userId: number): Promise { - const row = await this.findOne({ user: userId }, { fields: ['id'] }); + const row = await this.findOne({ user: userId }, { fields: ['id'], disableIdentityMap: true }); return row !== null; } + + // --------------------------------------------------------------------- + // PK5 — passkeyRegisterOptions' excludeCredentials + // --------------------------------------------------------------------- + + /** `SELECT credential_id, transports FROM webauthn_credentials WHERE user_id = ?` */ + async listExcludeCredentials(userId: number): Promise { + const rows = await this.find({ user: userId }, { fields: ['credential_id', 'transports'], disableIdentityMap: true }); + return rows.map((row) => ({ credential_id: row.credential_id, transports: row.transports ?? null })); + } + + // --------------------------------------------------------------------- + // PK6 — passkeyRegisterVerify's in-transaction duplicate check + // --------------------------------------------------------------------- + + /** + * `SELECT id FROM webauthn_credentials WHERE credential_id = ?` — called + * from inside `uow.transactional` alongside `insertCredential` so the + * check-then-insert can never race a UNIQUE violation past the service's + * `DUPLICATE_CREDENTIAL` sentinel (identity-compared, not string-matched). + * `disableIdentityMap: true` per `hasAny`'s docstring above — this read + * runs inside the same transaction as PK7's write, so keeping it out of + * the identity map matters even more here. + */ + async existsByCredentialId(credentialId: string): Promise { + const row = await this.findOne({ credential_id: credentialId }, { fields: ['id'], disableIdentityMap: true }); + return row !== null; + } + + // --------------------------------------------------------------------- + // PK7 — credential enrollment (same transaction as PK6) + // --------------------------------------------------------------------- + + /** + * `INSERT INTO webauthn_credentials (user_id, credential_id, public_key, + * counter, transports, device_type, backed_up, name, aaguid, last_used_at) + * VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, NULL)` — `last_used_at` is always + * `NULL` on insert (a fresh credential has never been used yet), same as + * the legacy statement's literal. + * + * `EntityRepository.insert()`, not `create()` + `persist().flush()`: a + * native insert that commits immediately and has no identity-map/UnitOfWork + * side effects, so it can never be part of a stale-snapshot flush the way + * a persisted-and-tracked entity could; `persist().flush()` would also + * flush the ENTIRE request's UnitOfWork, not just this row, which is + * exactly the write-discarding hazard `hasAny`'s docstring describes. + */ + async insertCredential(row: NewCredentialRow): Promise { + await this.insert({ + user: row.user_id, + credential_id: row.credential_id, + public_key: row.public_key, + counter: row.counter, + transports: row.transports, + device_type: row.device_type, + backed_up: row.backed_up, + name: row.name, + aaguid: row.aaguid, + last_used_at: null, + }); + } + + // --------------------------------------------------------------------- + // PK8 — post-registration re-select (after the transaction resolves) + // --------------------------------------------------------------------- + + /** + * `SELECT id, name, device_type, backed_up, created_at, last_used_at + * FROM webauthn_credentials WHERE credential_id = ?` — a DIFFERENT + * column set from PK9's full-row read below, so a separate method (D4: + * mirror each statement's own projection, never one full-row read the + * caller trims down). + */ + async findCreatedCredential(credentialId: string): Promise { + const row = await this.findOne({ credential_id: credentialId }, { fields: [...PANEL_FIELDS], disableIdentityMap: true }); + return row ? toPanelRow(row) : null; + } + + // --------------------------------------------------------------------- + // PK9 — login-verify ceremony input (full row, Buffer public_key) + // --------------------------------------------------------------------- + + /** + * `SELECT * FROM webauthn_credentials WHERE credential_id = ?`. + * `disableIdentityMap: true`, not `{ refresh: true }` — this is not a + * primary-key filter (`credential_id` is a unique column, not `id`), and + * per `hasAny`'s docstring, a plain `refresh: true` re-snapshot risks a + * stale-write-back on flush if this same entity is already (or later) + * identity-mapped under a different projection within the same request — + * PK11's `updateCounterAndLastUsed` runs a `nativeUpdate` on this exact + * row moments later, in the same request. + */ + async findByCredentialId(credentialId: string): Promise { + const credential = await this.findOne({ credential_id: credentialId }, { disableIdentityMap: true }); + return credential ? (toRow(credential) as WebauthnCredentialRow) : null; + } + + // --------------------------------------------------------------------- + // PK11 — counter/last-used bookkeeping (same transaction as UsersRepository.touchLastLogin) + // --------------------------------------------------------------------- + + /** `UPDATE webauthn_credentials SET counter = ?, last_used_at = CURRENT_TIMESTAMP WHERE id = ?` */ + async updateCounterAndLastUsed(id: number, counter: number): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ id }, { counter, last_used_at: currentTimestamp(platform) }); + } + + // --------------------------------------------------------------------- + // PK13 — settings-panel list + // --------------------------------------------------------------------- + + /** + * `SELECT id, name, device_type, backed_up, created_at, last_used_at + * FROM webauthn_credentials WHERE user_id = ? ORDER BY created_at DESC` + */ + async listForPanel(userId: number): Promise { + const rows = await this.find({ user: userId }, { fields: [...PANEL_FIELDS], orderBy: { created_at: 'desc' }, disableIdentityMap: true }); + return rows.map((row) => toPanelRow(row)); + } + + // --------------------------------------------------------------------- + // PK14 — rename (ownership in the WHERE) + // --------------------------------------------------------------------- + + /** + * `UPDATE webauthn_credentials SET name = ? WHERE id = ? AND user_id = ?` + * — ownership enforced in the WHERE, not a separate check; returns the + * affected-row count so `renamePasskey` keeps its 404-never-403 behaviour + * (a foreign or unknown id both yield 0, indistinguishable by design). + */ + async renameOwned(id: number, userId: number, name: string): Promise { + return this.nativeUpdate({ id, user: userId }, { name }); + } + + // --------------------------------------------------------------------- + // PK16 — delete (ownership in the WHERE) + // --------------------------------------------------------------------- + + /** `DELETE FROM webauthn_credentials WHERE id = ? AND user_id = ?` */ + async deleteOwned(id: number, userId: number): Promise { + return this.nativeDelete({ id, user: userId }); + } + + // --------------------------------------------------------------------- + // PK18 — admin bulk-clear + // --------------------------------------------------------------------- + + /** `DELETE FROM webauthn_credentials WHERE user_id = ?` — returns the deleted count for `adminResetPasskeys`' response. */ + async deleteAllForUser(userId: number): Promise { + return this.nativeDelete({ user: userId }); + } } diff --git a/server/src/nest/auth/auth.module.ts b/server/src/nest/auth/auth.module.ts index bbd03f773..581e9e219 100644 --- a/server/src/nest/auth/auth.module.ts +++ b/server/src/nest/auth/auth.module.ts @@ -3,6 +3,9 @@ import { Module } from '@nestjs/common'; import { MikroOrmModule } from '@mikro-orm/nestjs'; import { AppSettings } from '../../db/entities/AppSettings.entity'; import { Users } from '../../db/entities/Users.entity'; +import { WebauthnCredentials } from '../../db/entities/WebauthnCredentials.entity'; +import { WebauthnChallenges } from '../../db/entities/WebauthnChallenges.entity'; +import { InviteTokens } from '../../db/entities/InviteTokens.entity'; import { TokensModule } from '../tokens/tokens.module'; import { AuthPublicController } from './auth-public.controller'; import { AuthController } from './auth.controller'; @@ -69,9 +72,12 @@ import { AllowedFileTypesModule } from '../files/allowed-file-types.module'; // AppSettings/Users: AuthService/UserProfileService each pass their own // AppSettingsRepository/UsersRepository to instance-api-keys.ts's // resolveApiKey/readInstanceApiKey/writeInstanceApiKey now (Plan 3a Task - // 5) — everything else either service still reads/writes is raw SQL - // through DatabaseService (auth's own conversion is a later plan). - MikroOrmModule.forFeature([AppSettings, Users])], + // 5). WebauthnCredentials/WebauthnChallenges: PasskeyService (Plan 3b + // Task 3). InviteTokens: RegistrationInvitesService (Plan 3b Tasks 0/3) + // — everything else either service still reads/writes is raw SQL + // through DatabaseService (AuthService's own conversion is a later + // task). + MikroOrmModule.forFeature([AppSettings, Users, WebauthnCredentials, WebauthnChallenges, InviteTokens])], controllers: [AuthPublicController, AuthController, PasskeyController], providers: [AuthService, UserProfileService, RegistrationInvitesService, PasskeyService, UserCleanupService, WebauthnConfigService, AuthMcp], exports: [AuthService, RegistrationInvitesService, PasskeyService, UserCleanupService], diff --git a/server/src/nest/auth/passkey.service.ts b/server/src/nest/auth/passkey.service.ts index 81f2f5a5a..669172c2d 100644 --- a/server/src/nest/auth/passkey.service.ts +++ b/server/src/nest/auth/passkey.service.ts @@ -1,4 +1,5 @@ import { Injectable, Logger } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import bcrypt from 'bcryptjs'; import { generateRegistrationOptions, @@ -11,8 +12,13 @@ import { WebauthnConfigService, originWithinRpScope, type WebauthnConfig } from import { avatarUrl } from '../common/avatarUrl'; import { stripUserForClient } from './auth.helpers'; import { AuthService } from './auth.service'; -import { DatabaseService } from '../database/database.service'; import { UnitOfWork } from '../database/unit-of-work'; +import { WebauthnCredentials } from '../../db/entities/WebauthnCredentials.entity'; +import type { WebauthnCredentialsRepository } from '../../db/repositories/WebauthnCredentials.repository'; +import { WebauthnChallenges } from '../../db/entities/WebauthnChallenges.entity'; +import type { WebauthnChallengesRepository } from '../../db/repositories/WebauthnChallenges.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; import type { User } from '../../types'; // --------------------------------------------------------------------------- @@ -38,21 +44,6 @@ const AUTH_FAILED = { error: 'Authentication failed', status: 401 } as const; // insert-failure 400 without string-matching SQLite errors. const DUPLICATE_CREDENTIAL = new Error('duplicate credential'); -interface CredentialRow { - id: number; - user_id: number; - credential_id: string; - public_key: Buffer; - counter: number; - transports: string | null; - device_type: string | null; - backed_up: number; - name: string | null; - aaguid: string | null; - created_at: string; - last_used_at: string | null; -} - function clientDataFromResponse(resp: unknown): { challenge?: unknown; origin?: unknown } | null { try { const cdj = (resp as { response?: { clientDataJSON?: unknown } })?.response?.clientDataJSON; @@ -105,10 +96,12 @@ export class PasskeyService { private readonly logger = new Logger(PasskeyService.name); constructor( - private readonly db: DatabaseService, private readonly auth: AuthService, private readonly webauthn: WebauthnConfigService, private readonly uow: UnitOfWork, + @InjectRepository(WebauthnCredentials) private readonly webauthnCredentials: WebauthnCredentialsRepository, + @InjectRepository(WebauthnChallenges) private readonly webauthnChallenges: WebauthnChallengesRepository, + @InjectRepository(Users) private readonly users: UsersRepository, ) {} // ------------------------------------------------------------------------- @@ -116,14 +109,11 @@ export class PasskeyService { // ------------------------------------------------------------------------- private async purgeExpiredChallenges(now: number): Promise { - this.db.run('DELETE FROM webauthn_challenges WHERE expires_at < ?', now); + await this.webauthnChallenges.purgeExpired(now); } private async storeChallenge(challenge: string, userId: number | null, type: 'registration' | 'authentication', now: number): Promise { - this.db.run( - 'INSERT INTO webauthn_challenges (challenge, user_id, type, expires_at) VALUES (?, ?, ?, ?)', - challenge, userId, type, now + CHALLENGE_TTL_MS, - ); + await this.webauthnChallenges.insertChallenge({ challenge, user_id: userId, type, expires_at: now + CHALLENGE_TTL_MS }); } /** @@ -133,11 +123,7 @@ export class PasskeyService { * twice (the replay window a SELECT→await→DELETE ordering would open). */ private async claimChallenge(challenge: string, type: 'registration' | 'authentication', now: number): Promise<{ user_id: number | null } | null> { - const row = this.db.get<{ user_id: number | null }>( - 'DELETE FROM webauthn_challenges WHERE challenge = ? AND type = ? AND expires_at > ? RETURNING user_id', - challenge, type, now, - ); - return row ?? null; + return this.webauthnChallenges.claimChallenge(challenge, type, now); } // ------------------------------------------------------------------------- @@ -209,7 +195,7 @@ export class PasskeyService { if (!cfg) return { ...NOT_CONFIGURED }; if (this.originCannotVerify(cfg, requestOrigin)) return { ...NOT_CONFIGURED }; - const user = this.db.get('SELECT * FROM users WHERE id = ?', userId); + const user = await this.users.findById(userId); if (!user) return { error: 'User not found', status: 404 }; // Re-authentication: a hijacked session must not be able to silently plant an @@ -219,9 +205,7 @@ export class PasskeyService { return { error: 'Incorrect password', status: 401 }; } - const existing = this.db.all<{ credential_id: string; transports: string | null }>( - 'SELECT credential_id, transports FROM webauthn_credentials WHERE user_id = ?', userId, - ); + const existing = await this.webauthnCredentials.listExcludeCredentials(userId); const now = Date.now(); await this.purgeExpiredChallenges(now); @@ -294,23 +278,20 @@ export class PasskeyService { // between them; the sentinel keeps the legacy 409-vs-400 split intact. try { await this.uow.transactional(async () => { - if (this.db.get('SELECT id FROM webauthn_credentials WHERE credential_id = ?', credential.id)) { + if (await this.webauthnCredentials.existsByCredentialId(credential.id)) { throw DUPLICATE_CREDENTIAL; } - this.db.run( - `INSERT INTO webauthn_credentials - (user_id, credential_id, public_key, counter, transports, device_type, backed_up, name, aaguid, last_used_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, NULL)`, - userId, - credential.id, - Buffer.from(credential.publicKey), - credential.counter ?? 0, - credential.transports ? JSON.stringify(credential.transports) : null, - credentialDeviceType ?? null, - credentialBackedUp ? 1 : 0, + await this.webauthnCredentials.insertCredential({ + user_id: userId, + credential_id: credential.id, + public_key: Buffer.from(credential.publicKey), + counter: credential.counter ?? 0, + transports: credential.transports ? JSON.stringify(credential.transports) : null, + device_type: credentialDeviceType ?? null, + backed_up: credentialBackedUp ? 1 : 0, name, - aaguid ?? null, - ); + aaguid: aaguid ?? null, + }); }); } catch (err) { if (err === DUPLICATE_CREDENTIAL) { @@ -319,10 +300,7 @@ export class PasskeyService { return { error: 'Could not register this passkey.', status: 400 }; } - const created = this.db.get<{ backed_up: number } & Record>( - 'SELECT id, name, device_type, backed_up, created_at, last_used_at FROM webauthn_credentials WHERE credential_id = ?', - credential.id, - ) as { backed_up: number } & Record; + const created = (await this.webauthnCredentials.findCreatedCredential(credential.id))!; return { success: true, credential: { ...created, backed_up: created.backed_up === 1 } }; } @@ -377,7 +355,7 @@ export class PasskeyService { const credId = (resp as { id?: unknown; rawId?: unknown }).id ?? (resp as { rawId?: unknown }).rawId; if (typeof credId !== 'string') return { ...AUTH_FAILED }; - const cred = this.db.get('SELECT * FROM webauthn_credentials WHERE credential_id = ?', credId); + const cred = await this.webauthnCredentials.findByCredentialId(credId); if (!cred) return { ...AUTH_FAILED }; const expectedOrigin = this.expectedOrigins(cfg, resp); @@ -414,20 +392,20 @@ export class PasskeyService { return { ...AUTH_FAILED, auditUserId: cred.user_id, auditAction: 'user.passkey_clone_suspected' }; } - const user = this.db.get('SELECT * FROM users WHERE id = ?', cred.user_id); + const user = await this.users.findById(cred.user_id); if (!user) return { ...AUTH_FAILED }; // Persist the new counter + last-used and bump login bookkeeping atomically. await this.uow.transactional(async () => { - this.db.run('UPDATE webauthn_credentials SET counter = ?, last_used_at = CURRENT_TIMESTAMP WHERE id = ?', newCounter, cred.id); - this.db.run('UPDATE users SET last_login = CURRENT_TIMESTAMP, login_count = login_count + 1 WHERE id = ?', user.id); + await this.webauthnCredentials.updateCounterAndLastUsed(cred.id, newCounter); + await this.users.touchLastLogin(user.id); }); // A user-verified passkey is phishing-resistant and inherently two-factor // (device possession + biometric/PIN), so it mints the real session directly // — the SAME path as password and OIDC login (no new token shape). const token = await this.auth.generateToken(user); - const userSafe = stripUserForClient(user) as Record; + const userSafe = stripUserForClient(user as unknown as User) as Record; return { token, user: { ...userSafe, avatar_url: avatarUrl(user) }, auditUserId: Number(user.id) }; } @@ -436,10 +414,7 @@ export class PasskeyService { // ------------------------------------------------------------------------- async listPasskeys(userId: number): Promise>> { - const rows = this.db.all<{ backed_up: number } & Record>( - 'SELECT id, name, device_type, backed_up, created_at, last_used_at FROM webauthn_credentials WHERE user_id = ? ORDER BY created_at DESC', - userId, - ); + const rows = await this.webauthnCredentials.listForPanel(userId); return rows.map((r) => ({ ...r, backed_up: r.backed_up === 1 })); } @@ -447,8 +422,8 @@ export class PasskeyService { const cleanName = sanitizeName(name); if (!cleanName) return { error: 'Name is required', status: 400 }; // Ownership enforced in SQL (404 on miss, never a 403 that leaks existence). - const result = this.db.run('UPDATE webauthn_credentials SET name = ? WHERE id = ? AND user_id = ?', cleanName, Number(id), userId); - if (result.changes === 0) return { error: 'Passkey not found', status: 404 }; + const changes = await this.webauthnCredentials.renameOwned(Number(id), userId, cleanName); + if (changes === 0) return { error: 'Passkey not found', status: 404 }; return { success: true }; } @@ -461,20 +436,20 @@ export class PasskeyService { // strip the victim's passkeys). Deleting is always allowed because every // account keeps a usable password as recovery fallback — losing all passkeys // can never lock anyone out. - const user = this.db.get<{ password_hash: string }>('SELECT password_hash FROM users WHERE id = ?', userId); - if (!user || !user.password_hash || !password || !bcrypt.compareSync(password, user.password_hash)) { + const passwordHash = await this.users.getPasswordHash(userId); + if (!passwordHash || !password || !bcrypt.compareSync(password, passwordHash)) { return { error: 'Incorrect password', status: 401 }; } - const result = this.db.run('DELETE FROM webauthn_credentials WHERE id = ? AND user_id = ?', Number(id), userId); - if (result.changes === 0) return { error: 'Passkey not found', status: 404 }; + const changes = await this.webauthnCredentials.deleteOwned(Number(id), userId); + if (changes === 0) return { error: 'Passkey not found', status: 404 }; return { success: true }; } /** Admin: clear all of a user's passkeys (e.g. on suspected compromise). */ async adminResetPasskeys(targetUserId: number): Promise<{ error?: string; status?: number; success?: boolean; deleted?: number; email?: string }> { - const target = this.db.get<{ id: number; email: string }>('SELECT id, email FROM users WHERE id = ?', targetUserId); + const target = await this.users.findIdAndEmail(targetUserId); if (!target) return { error: 'User not found', status: 404 }; - const result = this.db.run('DELETE FROM webauthn_credentials WHERE user_id = ?', targetUserId); - return { success: true, deleted: result.changes, email: target.email }; + const deleted = await this.webauthnCredentials.deleteAllForUser(targetUserId); + return { success: true, deleted, email: target.email }; } } diff --git a/server/src/nest/auth/registration-invites.service.ts b/server/src/nest/auth/registration-invites.service.ts index c0b68deed..425eb85ba 100644 --- a/server/src/nest/auth/registration-invites.service.ts +++ b/server/src/nest/auth/registration-invites.service.ts @@ -1,6 +1,9 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import crypto from 'crypto'; import { DatabaseService } from '../database/database.service'; +import { InviteTokens } from '../../db/entities/InviteTokens.entity'; +import type { InviteTokensRepository } from '../../db/repositories/InviteTokens.repository'; /** * Registration invites: the tokens an admin hands out so someone can create an @@ -16,22 +19,32 @@ import { DatabaseService } from '../database/database.service'; * because the management routes are under /api/admin. Those routes keep their * paths and their guards; AdminController now injects this instead of carrying * another domain's SQL. + * + * Plan 3b Task 3: `invite_tokens` reads/writes go through + * `InviteTokensRepository` (RI1, RI4–RI7). RI2/RI3 (`trips`) stay on + * `DatabaseService` — `trips` is `nest/trips`' table, not this domain's, and + * Plan 3c is the one that builds a `TripsRepository`; building a shim one + * here would be exactly the "manual synchronization" this migration exists + * to remove (same carve-out shape as Plan 3a's `addons.service.ts` + * `listTripsForInvite`/`createInvite`'s trip-binding validation, documented + * at the inventory's §6). */ @Injectable() export class RegistrationInvitesService { - constructor(private readonly db: DatabaseService) {} + constructor( + private readonly db: DatabaseService, + @InjectRepository(InviteTokens) private readonly inviteTokens: InviteTokensRepository, + ) {} + /** RI1 — `InviteTokensRepository.listWithCreatorAndTrip()`'s joined projection. */ async listInvites() { - return this.db.all(` - SELECT i.*, u.username as created_by_name, t.title as trip_title - FROM invite_tokens i - JOIN users u ON i.created_by = u.id - LEFT JOIN trips t ON i.trip_id = t.id - ORDER BY i.created_at DESC - `); + return this.inviteTokens.listWithCreatorAndTrip(); } - /** Trips an admin can bind an invite to — id + title only, for the picker (#1402). */ + /** + * Trips an admin can bind an invite to — id + title only, for the picker + * (#1402). RI2 — stays raw on `DatabaseService` (see the class docstring). + */ async listTripsForInvite() { return this.db.all('SELECT id, title FROM trips ORDER BY title COLLATE NOCASE ASC'); } @@ -49,6 +62,7 @@ export class RegistrationInvitesService { // Optional trip binding: only persist a trip that actually exists, so a stale // or forged id can never bind (and never auto-adds anyone on registration). + // RI3 — stays raw on `DatabaseService` (see the class docstring). let tripId: number | null = null; if (data.trip_id != null && String(data.trip_id).trim() !== '') { const parsed = Number.parseInt(String(data.trip_id)); @@ -60,27 +74,29 @@ export class RegistrationInvitesService { tripId = parsed; } - const ins = this.db.run( - 'INSERT INTO invite_tokens (token, max_uses, expires_at, created_by, trip_id) VALUES (?, ?, ?, ?, ?)', - token, uses, expiresAt, createdBy, tripId, - ); + // RI4: the write. RI5: the same joined re-select RI1 projects, filtered + // to the new row — `insertInvite`'s column set already matches this + // INSERT exactly (Task 0). + const created = await this.inviteTokens.insertInvite({ token, max_uses: uses, expires_at: expiresAt, created_by: createdBy, trip_id: tripId }); + const invite = await this.inviteTokens.findWithCreatorAndTrip(created.id); - const inviteId = Number(ins.lastInsertRowid); - const invite = this.db.get(` - SELECT i.*, u.username as created_by_name, t.title as trip_title - FROM invite_tokens i - JOIN users u ON i.created_by = u.id - LEFT JOIN trips t ON i.trip_id = t.id - WHERE i.id = ? - `, inviteId); - - return { invite, inviteId, uses, expiresInDays: data.expires_in_days ?? null, tripId }; + return { invite, inviteId: created.id, uses, expiresInDays: data.expires_in_days ?? null, tripId }; } async deleteInvite(id: string) { - const invite = this.db.get('SELECT id FROM invite_tokens WHERE id = ?', id); - if (!invite) return { error: 'Invite not found', status: 404 }; - this.db.run('DELETE FROM invite_tokens WHERE id = ?', id); + // A non-numeric id can never match an `invite_tokens.id` row — resolved + // here rather than handed to the repository as `NaN` (SQLite's driver + // has no representation for it as a bind parameter; the legacy raw + // statement tolerated a non-numeric string bind and simply matched no + // row, so the 404 below reproduces that same observable outcome without + // routing an invalid value into the query layer). + const numericId = Number(id); + // RI6 — the 404 check. + if (!Number.isInteger(numericId) || (await this.inviteTokens.findIdById(numericId)) === null) { + return { error: 'Invite not found', status: 404 }; + } + // RI7. + await this.inviteTokens.deleteById(numericId); return {}; } } diff --git a/server/tests/e2e/admin.e2e.test.ts b/server/tests/e2e/admin.e2e.test.ts index 1c215d2d9..47a3abf72 100644 --- a/server/tests/e2e/admin.e2e.test.ts +++ b/server/tests/e2e/admin.e2e.test.ts @@ -39,8 +39,16 @@ const { db } = vi.hoisted(() => { tmp.exec('CREATE TABLE trips (id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT, user_id INTEGER);'); tmp.exec('CREATE TABLE places (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER);'); tmp.exec('CREATE TABLE trip_files (id INTEGER PRIMARY KEY AUTOINCREMENT, trip_id INTEGER, message_id INTEGER);'); + // `used_count`, not `uses`: this hand-rolled fixture had drifted from the + // real migrated `invite_tokens` schema (`Migration20200101003500_create_invite_tokens`) + // — the legacy raw-SQL `RegistrationInvitesService` never named the column + // explicitly (its INSERT relied on the table's own DEFAULT, its re-select + // used `i.*`), so the drift stayed invisible. `InviteTokensRepository + // .insertInvite` (Plan 3b) writes through entity metadata, which does + // name every column explicitly — surfacing the drift as `no such column: + // used_count`. Fixed at the source (the fixture), not worked around. tmp.exec(`CREATE TABLE invite_tokens (id INTEGER PRIMARY KEY AUTOINCREMENT, token TEXT NOT NULL, - max_uses INTEGER, uses INTEGER DEFAULT 0, expires_at TEXT, created_by INTEGER NOT NULL, + max_uses INTEGER, used_count INTEGER DEFAULT 0, expires_at TEXT, created_by INTEGER NOT NULL, trip_id INTEGER, created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); tmp.exec(`CREATE TABLE packing_templates (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL, created_by INTEGER NOT NULL, created_at DATETIME DEFAULT CURRENT_TIMESTAMP);`); diff --git a/server/tests/unit/db/repositories/InviteTokens.repository.test.ts b/server/tests/unit/db/repositories/InviteTokens.repository.test.ts index 1fb4b7526..29bd1af07 100644 --- a/server/tests/unit/db/repositories/InviteTokens.repository.test.ts +++ b/server/tests/unit/db/repositories/InviteTokens.repository.test.ts @@ -109,4 +109,53 @@ describe('InviteTokensRepository', () => { expect((rawInvite(created.id) as { used_count: number }).used_count).toBe(1); }); }); + + // RI1/RI5 — the admin invite list's joined projection. + describe('listWithCreatorAndTrip / findWithCreatorAndTrip', () => { + it('INVREPO-010: projects the creator\'s username and a bound trip\'s title, newest first', async () => { + const { user: admin } = createUser(testDb, { username: 'inviter-1' }); + const trip = createTrip(testDb, admin.id, { title: 'Bound Trip' }); + const older = createInviteToken(testDb, { token: 'older', created_by: admin.id }); + testDb.prepare('UPDATE invite_tokens SET created_at = ? WHERE id = ?').run('2026-01-01 00:00:00', older.id); + const newer = createInviteToken(testDb, { token: 'newer', created_by: admin.id }); + testDb.prepare('UPDATE invite_tokens SET created_at = ?, trip_id = ? WHERE id = ?').run('2026-02-01 00:00:00', trip.id, newer.id); + + const rows = await invites.listWithCreatorAndTrip(); + + expect(rows.map((r) => r.token)).toEqual(['newer', 'older']); // ORDER BY created_at DESC + expect(rows[0]).toMatchObject({ token: 'newer', created_by_name: 'inviter-1', trip_title: 'Bound Trip', trip_id: trip.id }); + expect(rows[1]).toMatchObject({ token: 'older', created_by_name: 'inviter-1', trip_title: null, trip_id: null }); + }); + + it('INVREPO-011: findWithCreatorAndTrip filters the same projection to one id', async () => { + const { user: admin } = createUser(testDb, { username: 'inviter-2' }); + const invite = createInviteToken(testDb, { token: 'single', created_by: admin.id }); + + const row = await invites.findWithCreatorAndTrip(invite.id); + expect(row).toMatchObject({ id: invite.id, token: 'single', created_by_name: 'inviter-2', trip_title: null }); + }); + + it('INVREPO-012: findWithCreatorAndTrip is null for an unknown id', async () => { + expect(await invites.findWithCreatorAndTrip(999_999)).toBeNull(); + }); + }); + + // RI6/RI7 — deleteInvite's 404 check and the delete itself. + describe('findIdById / deleteById', () => { + it('INVREPO-013: findIdById returns the id for a known row, null otherwise', async () => { + const invite = createInviteToken(testDb, { token: 'exists' }); + expect(await invites.findIdById(invite.id)).toBe(invite.id); + expect(await invites.findIdById(999_999)).toBeNull(); + }); + + it('INVREPO-014: deleteById removes exactly that row', async () => { + const keep = createInviteToken(testDb, { token: 'keep' }); + const gone = createInviteToken(testDb, { token: 'gone' }); + + await invites.deleteById(gone.id); + + expect(rawInvite(gone.id)).toBeUndefined(); + expect(rawInvite(keep.id)).toBeDefined(); + }); + }); }); diff --git a/server/tests/unit/db/repositories/WebauthnChallenges.repository.test.ts b/server/tests/unit/db/repositories/WebauthnChallenges.repository.test.ts new file mode 100644 index 000000000..dc184f7d8 --- /dev/null +++ b/server/tests/unit/db/repositories/WebauthnChallenges.repository.test.ts @@ -0,0 +1,135 @@ +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 { WebauthnChallenges } from '../../../../src/db/entities/WebauthnChallenges.entity'; +import type { WebauthnChallengesRepository } from '../../../../src/db/repositories/WebauthnChallenges.repository'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let challenges: WebauthnChallengesRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + challenges = t.repo(WebauthnChallenges); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +function seedChallenge(challenge: string, userId: number | null, type: string, expiresAt: number): void { + testDb.prepare('INSERT INTO webauthn_challenges (challenge, user_id, type, expires_at) VALUES (?, ?, ?, ?)').run(challenge, userId, type, expiresAt); +} + +function challengeCount(): number { + return (testDb.prepare('SELECT COUNT(*) AS n FROM webauthn_challenges').get() as { n: number }).n; +} + +describe('WebauthnChallengesRepository', () => { + describe('purgeExpired', () => { + it('WEBAUTHN-CHAL-REPO-001: deletes only rows past their expiry, live rows survive', async () => { + const { user } = createUser(testDb); + const now = Date.now(); + seedChallenge('expired-1', user.id, 'registration', now - 1000); + seedChallenge('expired-2', null, 'authentication', now - 1); + seedChallenge('live', user.id, 'registration', now + 60_000); + + await challenges.purgeExpired(now); + + expect(challengeCount()).toBe(1); + const remaining = testDb.prepare('SELECT challenge FROM webauthn_challenges').all() as Array<{ challenge: string }>; + expect(remaining).toEqual([{ challenge: 'live' }]); + }); + + it('WEBAUTHN-CHAL-REPO-002: an empty table is a no-op', async () => { + await challenges.purgeExpired(Date.now()); + expect(challengeCount()).toBe(0); + }); + }); + + describe('insertChallenge', () => { + it('WEBAUTHN-CHAL-REPO-003: writes the exact column set, user-bound (registration)', async () => { + const { user } = createUser(testDb); + const expiresAt = Date.now() + 300_000; + await challenges.insertChallenge({ challenge: 'reg-chal', user_id: user.id, type: 'registration', expires_at: expiresAt }); + + const row = testDb.prepare('SELECT challenge, user_id, type, expires_at FROM webauthn_challenges WHERE challenge = ?').get('reg-chal'); + expect(row).toEqual({ challenge: 'reg-chal', user_id: user.id, type: 'registration', expires_at: expiresAt }); + }); + + it('WEBAUTHN-CHAL-REPO-004: writes a NULL user_id (anonymous authentication challenge)', async () => { + const expiresAt = Date.now() + 300_000; + await challenges.insertChallenge({ challenge: 'auth-chal', user_id: null, type: 'authentication', expires_at: expiresAt }); + + const row = testDb.prepare('SELECT user_id, type FROM webauthn_challenges WHERE challenge = ?').get('auth-chal'); + expect(row).toEqual({ user_id: null, type: 'authentication' }); + }); + }); + + describe('claimChallenge', () => { + it('WEBAUTHN-CHAL-REPO-005: claims a live, matching row and returns its user_id', async () => { + const { user } = createUser(testDb); + const now = Date.now(); + seedChallenge('claim-me', user.id, 'registration', now + 60_000); + + const result = await challenges.claimChallenge('claim-me', 'registration', now); + + expect(result).toEqual({ user_id: user.id }); + expect(challengeCount()).toBe(0); // spent + }); + + it('WEBAUTHN-CHAL-REPO-006: claims an anonymous (null user_id) row', async () => { + const now = Date.now(); + seedChallenge('anon-claim', null, 'authentication', now + 60_000); + + expect(await challenges.claimChallenge('anon-claim', 'authentication', now)).toEqual({ user_id: null }); + }); + + it('WEBAUTHN-CHAL-REPO-007: an unknown challenge is null, nothing is deleted', async () => { + expect(await challenges.claimChallenge('never-stored', 'registration', Date.now())).toBeNull(); + }); + + it('WEBAUTHN-CHAL-REPO-008: an expired row is null and stays in the table (purgeExpired\'s job, not claimChallenge\'s)', async () => { + const now = Date.now(); + seedChallenge('stale', null, 'registration', now - 1); + + expect(await challenges.claimChallenge('stale', 'registration', now)).toBeNull(); + expect(challengeCount()).toBe(1); // still there — claimChallenge only deletes a MATCHING row + }); + + it('WEBAUTHN-CHAL-REPO-009: a type mismatch is null, the row survives for its real type', async () => { + const { user } = createUser(testDb); + const now = Date.now(); + seedChallenge('wrong-type', user.id, 'registration', now + 60_000); + + expect(await challenges.claimChallenge('wrong-type', 'authentication', now)).toBeNull(); + expect(challengeCount()).toBe(1); + expect(await challenges.claimChallenge('wrong-type', 'registration', now)).toEqual({ user_id: user.id }); + }); + + it('WEBAUTHN-CHAL-REPO-010: single-use — a second claim of the same challenge is null', async () => { + const { user } = createUser(testDb); + const now = Date.now(); + seedChallenge('once-only', user.id, 'registration', now + 60_000); + + expect(await challenges.claimChallenge('once-only', 'registration', now)).toEqual({ user_id: user.id }); + expect(await challenges.claimChallenge('once-only', 'registration', now)).toBeNull(); + }); + + it('WEBAUTHN-CHAL-REPO-011: two concurrent claims of the same challenge — exactly one winner', async () => { + const { user } = createUser(testDb); + const now = Date.now(); + seedChallenge('race-me', user.id, 'registration', now + 60_000); + + const [a, b] = await Promise.all([ + challenges.claimChallenge('race-me', 'registration', now), + challenges.claimChallenge('race-me', 'registration', now), + ]); + + const winners = [a, b].filter((r) => r !== null); + expect(winners).toHaveLength(1); + expect(winners[0]).toEqual({ user_id: user.id }); + expect(challengeCount()).toBe(0); + }); + }); +}); diff --git a/server/tests/unit/db/repositories/WebauthnCredentials.repository.test.ts b/server/tests/unit/db/repositories/WebauthnCredentials.repository.test.ts new file mode 100644 index 000000000..a70961a62 --- /dev/null +++ b/server/tests/unit/db/repositories/WebauthnCredentials.repository.test.ts @@ -0,0 +1,288 @@ +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 { WebauthnCredentials } from '../../../../src/db/entities/WebauthnCredentials.entity'; +import type { WebauthnCredentialsRepository } from '../../../../src/db/repositories/WebauthnCredentials.repository'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let creds: WebauthnCredentialsRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + creds = t.repo(WebauthnCredentials); +}); +beforeEach(() => { resetTestDb(testDb); t.clear(); }); +afterAll(async () => { await t.close(); testDb.close(); }); + +let _credSeq = 0; +function insertCredential(userId: number, overrides: Record = {}): { id: number } & Record { + _credSeq++; + const row = { + credential_id: `cred-${_credSeq}`, + public_key: Buffer.from([1, 2, 3, _credSeq]), + counter: 0, + transports: null, + device_type: 'singleDevice', + backed_up: 0, + name: `Passkey ${_credSeq}`, + aaguid: null, + ...overrides, + }; + const result = testDb.prepare( + `INSERT INTO webauthn_credentials + (user_id, credential_id, public_key, counter, transports, device_type, backed_up, name, aaguid) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + ).run(userId, row.credential_id, row.public_key, row.counter, row.transports, row.device_type, row.backed_up, row.name, row.aaguid); + return { id: Number(result.lastInsertRowid), ...row }; +} + +function rawCredential(id: number): Record | undefined { + return testDb.prepare('SELECT * FROM webauthn_credentials WHERE id = ?').get(id) as Record | undefined; +} + +function rawCredentialByCredentialId(credentialId: string): Record | undefined { + return testDb.prepare('SELECT * FROM webauthn_credentials WHERE credential_id = ?').get(credentialId) as Record | undefined; +} + +describe('WebauthnCredentialsRepository', () => { + describe('hasAny', () => { + it('WEBAUTHN-CRED-REPO-001: true iff the user owns at least one credential', async () => { + const { user } = createUser(testDb); + expect(await creds.hasAny(user.id)).toBe(false); + insertCredential(user.id); + expect(await creds.hasAny(user.id)).toBe(true); + }); + }); + + describe('listExcludeCredentials', () => { + it('WEBAUTHN-CRED-REPO-002: projects credential_id + transports for one user only', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + insertCredential(user.id, { credential_id: 'mine-1', transports: JSON.stringify(['usb']) }); + insertCredential(user.id, { credential_id: 'mine-2', transports: null }); + insertCredential(other.id, { credential_id: 'theirs' }); + + const rows = await creds.listExcludeCredentials(user.id); + expect(rows).toHaveLength(2); + expect(rows).toEqual( + expect.arrayContaining([ + { credential_id: 'mine-1', transports: JSON.stringify(['usb']) }, + { credential_id: 'mine-2', transports: null }, + ]), + ); + }); + }); + + describe('existsByCredentialId', () => { + it('WEBAUTHN-CRED-REPO-003: true for a known credential_id, false otherwise', async () => { + const { user } = createUser(testDb); + insertCredential(user.id, { credential_id: 'known-cred' }); + expect(await creds.existsByCredentialId('known-cred')).toBe(true); + expect(await creds.existsByCredentialId('unknown-cred')).toBe(false); + }); + }); + + describe('insertCredential', () => { + it('WEBAUTHN-CRED-REPO-004: writes every column, last_used_at always NULL, public_key round-trips byte for byte', async () => { + const { user } = createUser(testDb); + const publicKey = Buffer.from([9, 8, 7, 6, 5]); + + await creds.insertCredential({ + user_id: user.id, + credential_id: 'new-cred', + public_key: publicKey, + counter: 3, + transports: JSON.stringify(['internal']), + device_type: 'singleDevice', + backed_up: 1, + name: 'My Key', + aaguid: 'aaguid-1', + }); + + const row = rawCredentialByCredentialId('new-cred')!; + expect(row).toMatchObject({ + user_id: user.id, + credential_id: 'new-cred', + counter: 3, + transports: JSON.stringify(['internal']), + device_type: 'singleDevice', + backed_up: 1, + name: 'My Key', + aaguid: 'aaguid-1', + last_used_at: null, + }); + expect(Buffer.isBuffer(row.public_key)).toBe(true); + expect(Buffer.from(row.public_key as Buffer).equals(publicKey)).toBe(true); + }); + + it('WEBAUTHN-CRED-REPO-005: nullable fields (transports/device_type/name/aaguid) persist as NULL', async () => { + const { user } = createUser(testDb); + await creds.insertCredential({ + user_id: user.id, + credential_id: 'sparse-cred', + public_key: Buffer.from([1]), + counter: 0, + transports: null, + device_type: null, + backed_up: 0, + name: null, + aaguid: null, + }); + const row = rawCredentialByCredentialId('sparse-cred')!; + expect(row).toMatchObject({ transports: null, device_type: null, name: null, aaguid: null }); + }); + }); + + describe('findCreatedCredential (PK8) vs findByCredentialId (PK9)', () => { + it('WEBAUTHN-CRED-REPO-006: findCreatedCredential projects only the panel columns', async () => { + const { user } = createUser(testDb); + const cred = insertCredential(user.id, { name: 'Post-Reg' }); + + const row = await creds.findCreatedCredential(cred.credential_id as string); + expect(row).toEqual({ + id: cred.id, + name: 'Post-Reg', + device_type: 'singleDevice', + backed_up: 0, + created_at: expect.any(String), + last_used_at: null, + }); + expect(row).not.toHaveProperty('public_key'); + }); + + it('WEBAUTHN-CRED-REPO-007: findCreatedCredential is null for an unknown credential_id', async () => { + expect(await creds.findCreatedCredential('no-such-cred')).toBeNull(); + }); + + it('WEBAUTHN-CRED-REPO-008: findByCredentialId returns the full row, public_key a real Buffer', async () => { + const { user } = createUser(testDb); + const pk = Buffer.from([1, 2, 3]); + const cred = insertCredential(user.id, { public_key: pk }); + + const row = await creds.findByCredentialId(cred.credential_id as string); + expect(row).not.toBeNull(); + expect(row!.id).toBe(cred.id); + expect(row!.user_id).toBe(user.id); + expect(Buffer.isBuffer(row!.public_key)).toBe(true); + expect(Buffer.from(row!.public_key).equals(pk)).toBe(true); + }); + + it('WEBAUTHN-CRED-REPO-009: findByCredentialId is null for an unknown credential_id', async () => { + expect(await creds.findByCredentialId('no-such-cred')).toBeNull(); + }); + }); + + describe('updateCounterAndLastUsed', () => { + it('WEBAUTHN-CRED-REPO-010: bumps counter and stamps last_used_at', async () => { + const { user } = createUser(testDb); + const cred = insertCredential(user.id, { counter: 5 }); + + await creds.updateCounterAndLastUsed(cred.id, 6); + + const row = rawCredential(cred.id)!; + expect(row.counter).toBe(6); + expect(row.last_used_at).not.toBeNull(); + }); + }); + + describe('listForPanel', () => { + it('WEBAUTHN-CRED-REPO-011: newest-first, scoped to one user, matches PK8\'s column set', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + testDb.prepare( + `INSERT INTO webauthn_credentials (user_id, credential_id, public_key, counter, backed_up, name, created_at) + VALUES (?, 'old', x'01', 0, 0, 'Old', '2026-01-01 00:00:00'), (?, 'new', x'02', 0, 1, 'New', '2026-02-01 00:00:00')`, + ).run(user.id, user.id); + insertCredential(other.id); + + const rows = await creds.listForPanel(user.id); + expect(rows.map((r) => r.name)).toEqual(['New', 'Old']); + expect(rows[0]).toEqual({ id: expect.any(Number), name: 'New', device_type: null, backed_up: 1, created_at: '2026-02-01 00:00:00', last_used_at: null }); + }); + }); + + describe('renameOwned', () => { + it('WEBAUTHN-CRED-REPO-012: renames when id + user_id both match, returns the affected count', async () => { + const { user } = createUser(testDb); + const cred = insertCredential(user.id); + + expect(await creds.renameOwned(cred.id, user.id, 'Renamed')).toBe(1); + expect(rawCredential(cred.id)!.name).toBe('Renamed'); + }); + + it('WEBAUTHN-CRED-REPO-013: a foreign owner or unknown id affects 0 rows (404, never 403)', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + const cred = insertCredential(user.id, { name: 'Original' }); + + expect(await creds.renameOwned(cred.id, other.id, 'Stolen')).toBe(0); + expect(await creds.renameOwned(999_999, user.id, 'Ghost')).toBe(0); + expect(rawCredential(cred.id)!.name).toBe('Original'); + }); + }); + + describe('deleteOwned', () => { + it('WEBAUTHN-CRED-REPO-014: deletes when id + user_id both match, returns the affected count', async () => { + const { user } = createUser(testDb); + const cred = insertCredential(user.id); + expect(await creds.deleteOwned(cred.id, user.id)).toBe(1); + expect(rawCredential(cred.id)).toBeUndefined(); + }); + + it('WEBAUTHN-CRED-REPO-015: a foreign owner affects 0 rows and the row survives', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + const cred = insertCredential(user.id); + expect(await creds.deleteOwned(cred.id, other.id)).toBe(0); + expect(rawCredential(cred.id)).toBeDefined(); + }); + }); + + describe('identity-map isolation (coordinator ruling — stale-write-back bug class)', () => { + it('WEBAUTHN-CRED-REPO-017: a nativeUpdate inside uow.transactional is not discarded by a stale entity read earlier under a different projection', async () => { + const { user } = createUser(testDb); + const cred = insertCredential(user.id, { counter: 1 }); + const uow = new UnitOfWork(t.em); + + // Projection A — narrow existence probe (PK6's shape: existsByCredentialId). + await creds.existsByCredentialId(cred.credential_id as string); + // Projection B — full row, same request context (PK9's shape: findByCredentialId). + const before = await creds.findByCredentialId(cred.credential_id as string); + expect(before!.counter).toBe(1); + + // The write these two reads sit alongside in the same request — PK11's + // shape: a nativeUpdate on a row this request already read twice under + // different projections, inside uow.transactional (whose closing flush + // is exactly where the coordinator's ruling says a stale identity-mapped + // entity would silently overwrite this write). + await uow.transactional(async () => { + await creds.updateCounterAndLastUsed(cred.id, 99); + }); + + // Read again through a THIRD projection after the transaction commits + // (PK8's shape) — if either prior read had joined the identity map, + // this would also risk seeing a stale merged value. + const after = await creds.findByCredentialId(cred.credential_id as string); + expect(after!.counter).toBe(99); + expect(rawCredential(cred.id)!.counter).toBe(99); // the raw row itself — no ambiguity from any repository-side caching + }); + }); + + describe('deleteAllForUser', () => { + it('WEBAUTHN-CRED-REPO-016: clears every credential for one user, returns the deleted count, other users untouched', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + insertCredential(user.id); + insertCredential(user.id); + const kept = insertCredential(other.id); + + expect(await creds.deleteAllForUser(user.id)).toBe(2); + expect(await creds.deleteAllForUser(user.id)).toBe(0); // already empty + expect((testDb.prepare('SELECT id FROM webauthn_credentials').all() as Array<{ id: number }>)).toEqual([{ id: kept.id }]); + }); + }); +}); diff --git a/server/tests/unit/nest/admin.service.test.ts b/server/tests/unit/nest/admin.service.test.ts index df4eccd9a..11f0e0f9c 100644 --- a/server/tests/unit/nest/admin.service.test.ts +++ b/server/tests/unit/nest/admin.service.test.ts @@ -82,7 +82,7 @@ import { __clearVersionCacheForTests } from '../../../src/nest/admin/admin.helpe import { makeNotificationsService, makeNotificationPreferencesService } from '../../helpers/notifications'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestWebauthnCredentialsRepo, createTestWebauthnChallengesRepo } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); @@ -106,7 +106,7 @@ beforeAll(async () => { svc = new AdminService( dbs, await createTestAddonsService(testDb, dbs), - new PasskeyService(dbs, auth, webauthn, await createTestUnitOfWork(dbs.connection)), + new PasskeyService(auth, webauthn, await createTestUnitOfWork(dbs.connection), await createTestWebauthnCredentialsRepo(dbs.connection), await createTestWebauthnChallengesRepo(dbs.connection), await createTestUsersRepo(dbs.connection)), auth, permissions, await makeNotificationsService(dbs, realtime), diff --git a/server/tests/unit/nest/admin.version-notification.test.ts b/server/tests/unit/nest/admin.version-notification.test.ts index abca9725e..8acf3c9ec 100644 --- a/server/tests/unit/nest/admin.version-notification.test.ts +++ b/server/tests/unit/nest/admin.version-notification.test.ts @@ -60,7 +60,7 @@ import { AdminService } from '../../../src/nest/admin/admin.service'; import { makeNotificationsService, makeNotificationPreferencesService } from '../../helpers/notifications'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestWebauthnCredentialsRepo, createTestWebauthnChallengesRepo } from '../../helpers/test-uow'; const dbs = new DatabaseService(testDb); const realtime = new RealtimeService(); @@ -84,7 +84,7 @@ beforeAll(async () => { svc = new AdminService( dbs, await createTestAddonsService(testDb, dbs), - new PasskeyService(dbs, auth, webauthn, await createTestUnitOfWork(dbs.connection)), + new PasskeyService(auth, webauthn, await createTestUnitOfWork(dbs.connection), await createTestWebauthnCredentialsRepo(dbs.connection), await createTestWebauthnChallengesRepo(dbs.connection), await createTestUsersRepo(dbs.connection)), auth, permissions, await makeNotificationsService(dbs, realtime), diff --git a/server/tests/unit/nest/passkey.service.test.ts b/server/tests/unit/nest/passkey.service.test.ts index 606eda771..eaed0baa3 100644 --- a/server/tests/unit/nest/passkey.service.test.ts +++ b/server/tests/unit/nest/passkey.service.test.ts @@ -89,7 +89,13 @@ import { UserCleanupService } from '../../../src/nest/auth/user-cleanup.service' import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; import { MailerService } from '../../../src/nest/notifications/mailer/mailer.service'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { + createTestUnitOfWork, + createTestAppSettingsRepo, + createTestUsersRepo, + createTestWebauthnCredentialsRepo, + createTestWebauthnChallengesRepo, +} from '../../helpers/test-uow'; // MailerService is injected since the notifications fold — a stub instead of a // module mock. sendPasswordResetEmail is the only thing auth reaches for. @@ -125,7 +131,14 @@ beforeAll(async () => { new AllowedFileTypesService(new DatabaseService(testDb)), await createTestUnitOfWork(testDb), await createTestAppSettingsRepo(testDb), await createTestUsersRepo(testDb), ); - svc = new PasskeyService(new DatabaseService(testDb), auth, webauthn, await createTestUnitOfWork(testDb)); + svc = new PasskeyService( + auth, + webauthn, + await createTestUnitOfWork(testDb), + await createTestWebauthnCredentialsRepo(testDb), + await createTestWebauthnChallengesRepo(testDb), + await createTestUsersRepo(testDb), + ); }); const CFG = { rpID: 'trek.example.com', rpName: 'TREK', origins: ['https://trek.example.com'], explicitOrigins: false }; diff --git a/server/tests/unit/nest/registration-invites.service.test.ts b/server/tests/unit/nest/registration-invites.service.test.ts index 030e91d18..1ac94d60f 100644 --- a/server/tests/unit/nest/registration-invites.service.test.ts +++ b/server/tests/unit/nest/registration-invites.service.test.ts @@ -26,10 +26,15 @@ import { resetTestDb } from '../../helpers/test-db'; import { createUser, createAdmin, createTrip, createInviteToken } from '../../helpers/factories'; import { RegistrationInvitesService } from '../../../src/nest/auth/registration-invites.service'; import { DatabaseService } from '../../../src/nest/database/database.service'; +import { createTestInviteTokensRepo } from '../../helpers/test-uow'; -const svc = new RegistrationInvitesService(new DatabaseService(testDb)); +let svc: RegistrationInvitesService; -beforeAll(() => { createTables(testDb); runMigrations(testDb); }); +beforeAll(async () => { + createTables(testDb); + runMigrations(testDb); + svc = new RegistrationInvitesService(new DatabaseService(testDb), await createTestInviteTokensRepo(testDb)); +}); beforeEach(() => { resetTestDb(testDb); vi.clearAllMocks(); }); afterAll(() => { testDb.close(); });