diff --git a/server/src/db/repositories/McpTokens.repository.ts b/server/src/db/repositories/McpTokens.repository.ts index 872b1f2ae..80d5985b6 100644 --- a/server/src/db/repositories/McpTokens.repository.ts +++ b/server/src/db/repositories/McpTokens.repository.ts @@ -1,4 +1,374 @@ -import type { McpTokens } from '../entities/McpTokens.entity'; +import { McpTokens } from '../entities/McpTokens.entity'; +import { toRow, type AssertRowKeys } from './_shared/rows'; +import { columnRef, currentTimestamp } from '../dialect/sql-functions'; import { EntityRepository } from '@mikro-orm/sql'; -export class McpTokensRepository extends EntityRepository {} +/** + * A full `mcp_tokens` row (`SELECT * FROM mcp_tokens WHERE ...`), as the API + * emits it. `user_id` is the `persist(false)` mirror of the `user` relation + * (same pattern as `InviteTokens.created_by`/`createdByRef` and + * `Categories.user`/`user_id`) — a scalar column for `AssertRowKeys` + * purposes even though it is never written directly. + */ +export interface McpTokenRow { + id: number; + user_id: number; + name: string; + token_hash: string; + token_prefix: string; + created_at: string | null; + last_used_at: string | null; + kind: string; + scope_mode: string; + api_scopes: string | null; +} + +const _mcpTokenRowKeys: AssertRowKeys = true; + +/** `TokenService.listTokens` (TK1) — the column set both the MCP and API panels start from. */ +export interface McpTokenListRow { + id: number; + name: string; + token_prefix: string; + created_at: string | null; + last_used_at: string | null; + scope_mode: string; + api_scopes: string | null; +} + +/** `TokenService.createToken`'s write (TK3) — the hash is computed by the service, never here. */ +export interface NewMcpTokenRow { + user_id: number; + name: string; + token_hash: string; + token_prefix: string; + kind: string; + scope_mode: string; + api_scopes: string | null; +} + +/** `TokenService.createToken`'s re-select (TK4) and `adminDeleteMcpToken`'s 404 check (TK9) — see `findBasic`. */ +export interface McpTokenBasicRow { + id: number; + user_id: number; + name: string; + token_prefix: string; + created_at: string | null; + last_used_at: string | null; +} + +/** `TokenService.deleteToken`'s ownership + kind check (TK5). */ +export interface McpTokenIdRow { + id: number; +} + +/** `TokenService.listAllMcpTokens` (TK8) — the admin panel's cross-user join. */ +export interface McpTokenWithUsernameRow { + id: number; + name: string; + token_prefix: string; + created_at: string | null; + last_used_at: string | null; + user_id: number; + username: string; +} + +/** `TokenService.verifyApiTokenWithGrant` (TK11) — bearer auth for the public API. */ +export interface McpTokenGrantRow { + id: number; + username: string; + email: string; + role: string; + scope_mode: string; + api_scopes: string | null; +} + +/** `TokenService.verifyToken` (TK13) — bearer auth shared by the MCP/legacy-static-token surfaces. */ +export interface McpTokenUserRow { + id: number; + username: string; + email: string; + role: string; +} + +/** + * MCP tokens (assistant-tool bearer credentials) and API tokens (public REST + * integration keys) — one table (`mcp_tokens`), discriminated by `kind`, one + * repository (Plan 3b Task 2, inventory §2). The raw token is never stored, + * only its SHA-256 hash — this repository never computes or compares a + * hash, `TokenService` does, and it is handed the hash as an opaque string + * on every read here. `deleteAllForUser` exists for Task 5's `AuthService` + * (AU17/AU45 — session revocation on password change), not consumed here. + */ +export class McpTokensRepository extends EntityRepository { + /** + * `SELECT id, name, token_prefix, created_at, last_used_at, scope_mode, + * api_scopes FROM mcp_tokens WHERE user_id = ? AND kind = ? + * ORDER BY created_at DESC` (TK1, `listTokens` — the private method + * `listMcpTokens`/`listApiTokens` both call). `kind` discriminates the + * shape the caller derives from the row: an MCP token drops + * `scope_mode`/`api_scopes` from what it shows, an API key resolves them + * into a grant — both stay in the service, unchanged. + * + * `disableIdentityMap: true` (controller ruling on Task 1's review, + * applied to every entity-hydrating read in this repository): a `find`/ + * `findOne` with a `fields` projection merges a PARTIAL snapshot of this + * row into the request's shared identity map. If the same row is later + * loaded elsewhere in the request with a *different* projection, MikroORM + * merges both into one managed entity; a `nativeUpdate` elsewhere in the + * same transaction does not touch that managed entity (native writes + * bypass the identity map by design), so the merged-but-stale entity can + * still be flushed — by the closing `flush()` of a surrounding + * `uow.transactional` block — writing its stale unselected columns back + * over the `nativeUpdate`'s change. `disableIdentityMap: true` returns an + * isolated entity that is never merged into the shared context, so it can + * never be flushed at all: nothing here can go stale-write-back a later + * write. See `MCPTOKREPO-021` for the regression proof. + */ + async listByUserAndKind(userId: number, kind: string): Promise { + const rows = await this.find( + { user: userId, kind }, + { + fields: ['id', 'name', 'token_prefix', 'created_at', 'last_used_at', 'scope_mode', 'api_scopes'], + orderBy: { created_at: 'desc' }, + disableIdentityMap: true, + }, + ); + return rows.map((row) => ({ + id: row.id, + name: row.name, + token_prefix: row.token_prefix, + created_at: row.created_at ?? null, + last_used_at: row.last_used_at ?? null, + scope_mode: row.scope_mode, + api_scopes: row.api_scopes ?? null, + })); + } + + /** `SELECT COUNT(*) as count FROM mcp_tokens WHERE user_id = ? AND kind = ?` (TK2 — the 10-per-kind cap). */ + async countByUserAndKind(userId: number, kind: string): Promise { + return this.count({ user: userId, kind }); + } + + /** + * `INSERT INTO mcp_tokens (user_id, name, token_hash, token_prefix, kind, + * scope_mode, api_scopes) VALUES (...)` (TK3 — token mint, + * **security-sensitive**). Named `insertToken`, not `create`: + * `EntityRepository#create` is a synchronous, non-persisting factory with + * an incompatible signature — shadowing it would fail to compile (the + * same collision Task 1's `insertUser` avoided). The raw token is never + * given to or returned by this method — `token_hash` arrives already + * hashed, and the caller appends the raw value to its own response + * afterward, never storing or re-reading it. + * + * A native `em.insert()`, not `create()` + `persist()` + `flush()` + * (controller ruling on Task 1's review — every repository insert, not + * only the best-effort ones Plan 3a scoped this to originally): + * `flush()` commits the *whole* unit of work of the request's + * `EntityManager`, not just this row — inside `AuthService.registerUser`'s + * transaction (a later task's caller), a `flush()` here would commit + * whatever else that transaction was still assembling. `em.insert()` fires + * one native INSERT with no side effects on the context/identity map, + * matching `AuditLogRepository.insertEntry`'s precedent. Followed by a + * `disableIdentityMap: true` re-select (D4's create rule, this + * repository's isolated-read shape) rather than `em.refresh`, which needs + * a managed entity `em.insert` never produces. + */ + async insertToken(row: NewMcpTokenRow): Promise { + const id = await this.getEntityManager().insert(McpTokens, { + user: row.user_id, + name: row.name, + token_hash: row.token_hash, + token_prefix: row.token_prefix, + kind: row.kind, + scope_mode: row.scope_mode, + api_scopes: row.api_scopes, + }); + const inserted = await this.findOne({ id }, { disableIdentityMap: true }); + if (!inserted) { + throw new Error(`McpTokensRepository.insertToken: row ${String(id)} not found immediately after insert`); + } + return toRow(inserted) as McpTokenRow; + } + + /** + * `SELECT id, name, token_prefix, created_at, last_used_at FROM + * mcp_tokens WHERE id = ?` (TK4, `createToken`'s re-select) and + * `SELECT id, user_id FROM mcp_tokens WHERE id = ?` (TK9, + * `adminDeleteMcpToken`'s 404 check) — ONE method: TK9's two columns + * (`id`, `user_id`) are a subset of TK4's five, and both statements are a + * plain by-id lookup with no other filter, so a single narrower-than-`*` + * projection covering the union of both column sets serves both call + * sites without a second near-identical statement (same economy as + * `deleteById` below covering TK6/TK10). PK-only `findOne` -> + * `disableIdentityMap: true`, NOT `refresh: true` (controller ruling on + * Task 1's review, superseding Plan 3a's "PK-only `findOne` -> `refresh: + * true`" ruling everywhere in this repository): `refresh` still re-queries + * on every call, but it also MERGES the fresh row into the shared identity + * map — exactly the mechanism `listByUserAndKind`'s docstring above + * describes going stale. `disableIdentityMap: true` re-queries every call + * (same "always fresh" property `refresh` gave) without ever registering + * the result anywhere a later `flush()` could find it. + * + * `fields` asks for the RELATION property `'user'`, not the `persist(false)` + * mirror `'user_id'` — verified empirically (not from the `.d.ts`): a + * `fields` list naming the mirror directly leaves `row.user_id` + * `undefined` (it has no column of its own to select), while naming the + * relation loads its (unpopulated) `Ref`, whose `.id` is readable + * synchronously without a second query (`EntityRef`'s whole point, per + * `@mikro-orm/core`'s own typings comment: `ref.id` instead of + * `ref.unwrap().id`) — read here as `row.user.id`, not the entity's own + * `.user_id` field, which the `fields`-narrowed `Loaded<...>` type does + * not expose (MikroORM does derive it at runtime, same mechanism `toRow`'s + * full-row methods rely on, but `fields` narrows the TYPE to only the + * names actually requested). + */ + async findBasic(id: number): Promise { + const row = await this.findOne( + { id }, + { fields: ['id', 'user', 'name', 'token_prefix', 'created_at', 'last_used_at'], disableIdentityMap: true }, + ); + return row + ? { + id: row.id, + user_id: row.user.id, + name: row.name, + token_prefix: row.token_prefix, + created_at: row.created_at ?? null, + last_used_at: row.last_used_at ?? null, + } + : null; + } + + /** + * `SELECT id FROM mcp_tokens WHERE id = ? AND user_id = ? AND kind = ?` + * (TK5, `deleteToken`'s 404 check — ownership AND kind scoping, so an + * integrations-panel delete can never remove a token the MCP panel + * manages, and vice versa). Not a PK-only filter (`id` + `user` + `kind`), + * so this always re-queries regardless; `disableIdentityMap: true` all the + * same (controller ruling on Task 1's review — every entity-hydrating read + * in this repository, PK-only or not, per `listByUserAndKind`'s docstring + * above: a narrow `fields` projection merged into the shared identity map + * is the hazard, not specifically a PK-only filter). + */ + async findOwnedByKind(id: number, userId: number, kind: string): Promise { + const row = await this.findOne({ id, user: userId, kind }, { fields: ['id'], disableIdentityMap: true }); + return row ? { id: row.id } : null; + } + + /** + * `DELETE FROM mcp_tokens WHERE id = ?` — TK6 (`deleteToken`, after the + * owner+kind check above; the caller treats the follow-up session revoke + * as best-effort) and TK10 (`adminDeleteMcpToken`, after `findBasic`'s + * 404 check; the caller revokes sessions unconditionally). Identical + * statement, two call sites with different revoke-session semantics — + * that difference stays in `TokenService`, this repository only deletes + * the row. + */ + async deleteById(id: number): Promise { + await this.nativeDelete({ id }); + } + + /** + * `SELECT t.id, t.name, t.token_prefix, t.created_at, t.last_used_at, + * t.user_id, u.username FROM mcp_tokens t JOIN users u ON u.id = + * t.user_id ORDER BY t.created_at DESC` (TK8, `listAllMcpTokens` — the + * admin panel, deliberately unscoped by user). An INNER JOIN, matching + * the legacy `JOIN` (every `mcp_tokens` row has a `user`, FK + * `deleteRule('cascade')`, so no row is ever excluded by this choice). + * The projection root is `McpTokens` (`this.qb('t')`), not `Users` — D4/ + * Task 1's ruling that a join projection through this table stays owned + * by `McpTokensRepository` rather than adding a users-side method. + * `mapResults: false` (`execute(..., false)`) leaves the driver's raw row + * alone. Verified empirically (not from the `.d.ts`, and NOT the same + * shape as `TripsRepository.findAccessible`'s `t.user` -> `user_id` + * precedent): selecting the relation property `t.user` while ALSO joining + * that same relation (`t.user` -> alias `u`) is absorbed into the join + * rather than emitting a `user_id` column at all — MikroORM treats the + * select as redundant with the join, not as "select the FK scalar." A + * bare `columnRef(platform, 't.user_id')` (the dialect helper `no + * -restricted-syntax` steers every repository through instead of a + * hand-spelled `raw()`) selects the literal column and comes back keyed + * `user_id`, confirmed against the formatted SQL and the raw row. + */ + async listAllWithUsername(): Promise { + const platform = this.getEntityManager().getPlatform(); + const rows = await this.qb('t') + .innerJoin('t.user', 'u') + .select(['t.id', 't.name', 't.token_prefix', 't.created_at', 't.last_used_at', columnRef(platform, 't.user_id'), 'u.username']) + .orderBy({ 't.created_at': 'desc' }) + .execute('all', false); + return rows.map((row) => ({ + id: row.id, + name: row.name, + token_prefix: row.token_prefix, + created_at: row.created_at ?? null, + last_used_at: row.last_used_at ?? null, + user_id: row.user_id, + username: row.username, + })); + } + + /** + * `SELECT u.id, u.username, u.email, u.role, mt.scope_mode, mt.api_scopes + * FROM mcp_tokens mt JOIN users u ON mt.user_id = u.id WHERE + * mt.token_hash = ? AND mt.kind = 'api'` (TK11, + * `verifyApiTokenWithGrant` — **security-sensitive**, bearer auth for the + * public API). `kind = 'api'` is a literal in the WHERE, same as the + * legacy statement — this method only ever verifies an API key, never an + * MCP token, so the filter is not a parameter. Projection root + * `McpTokens` (`this.qb('mt')`), matching the legacy alias. + */ + async findGrantByHash(hash: string): Promise { + const row = await this.qb('mt') + .innerJoin('mt.user', 'u') + .select(['u.id', 'u.username', 'u.email', 'u.role', 'mt.scope_mode', 'mt.api_scopes']) + .where({ 'mt.token_hash': hash, 'mt.kind': 'api' }) + .execute('get', false); + return row ?? null; + } + + /** + * `SELECT u.id, u.username, u.email, u.role FROM mcp_tokens mt JOIN users + * u ON mt.user_id = u.id WHERE mt.token_hash = ? AND mt.kind = ?` (TK13, + * `verifyToken` — **security-sensitive**, every MCP/legacy static-token + * request). `kind` is part of the WHERE, never a post-filter applied to + * the result: a token of the wrong kind produces the exact same `null` a + * nonexistent token would, at the SQL level, not after a JS check — the + * caller (and a timing measurement) cannot distinguish "no such token" + * from "real token, wrong door." + */ + async findUserByHashAndKind(hash: string, kind: string): Promise { + const row = await this.qb('mt') + .innerJoin('mt.user', 'u') + .select(['u.id', 'u.username', 'u.email', 'u.role']) + .where({ 'mt.token_hash': hash, 'mt.kind': kind }) + .execute('get', false); + return row ?? null; + } + + /** + * `UPDATE mcp_tokens SET last_used_at = CURRENT_TIMESTAMP WHERE + * token_hash = ?` — TK12 (`verifyApiTokenWithGrant`'s fire-and-forget + * bookkeeping) and TK14 (`verifyToken`'s, same statement text, a separate + * call site) — ONE method, two callers, same as `deleteById` above. + * `currentTimestamp(platform)` rather than a hand-spelled `CURRENT_TIMESTAMP` + * string (byte-identical per `sql-functions.test.ts`'s `SQLF-008`). + */ + async touchLastUsedByHash(hash: string): Promise { + const platform = this.getEntityManager().getPlatform(); + await this.nativeUpdate({ token_hash: hash }, { last_used_at: currentTimestamp(platform) }); + } + + /** + * `DELETE FROM mcp_tokens WHERE user_id = ?` — `AuthService.changePassword` + * (AU17) and `resetPassword` (AU45), both inside the same transaction as + * the password write itself (**security-sensitive**: every MCP/API token + * the user held stops working the moment their password changes). Built + * here because this table belongs to `nest/tokens`, per the inventory's + * cross-domain ruling (§6) — Task 5 is the only caller. + */ + async deleteAllForUser(userId: number): Promise { + await this.nativeDelete({ user: userId }); + } +} diff --git a/server/src/nest/tokens/token.service.ts b/server/src/nest/tokens/token.service.ts index 458fd3553..2287bee66 100644 --- a/server/src/nest/tokens/token.service.ts +++ b/server/src/nest/tokens/token.service.ts @@ -1,11 +1,15 @@ import { Injectable } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import { randomBytes, createHash } from 'crypto'; import { PUBLIC_API_SCOPES, type PublicApiGrant, type PublicApiScope, } from '@trek/shared'; -import { DatabaseService } from '../database/database.service'; +import { McpTokens } from '../../db/entities/McpTokens.entity'; +import type { McpTokensRepository, McpTokenBasicRow } from '../../db/repositories/McpTokens.repository'; +import { Users } from '../../db/entities/Users.entity'; +import type { UsersRepository } from '../../db/repositories/Users.repository'; import { EphemeralTokenService } from '../auth/ephemeral-token.service'; // Import from sessionManager directly, NOT the ../../mcp barrel: the barrel pulls // the whole tools fan-out (and via the domain bridges, the Nest services) into @@ -29,8 +33,11 @@ type TokenKind = 'mcp' | 'api'; * touch one table (mcp_tokens) plus the ephemeral-token store, and nothing in * here needs to know how a password is hashed or how a session is established. * - * The methods moved verbatim — same SQL, same validation order, same error - * strings and status codes, same best-effort session revoke on delete. + * The methods moved verbatim — same SQL (now through `McpTokensRepository`), + * same validation order, same error strings and status codes, same + * best-effort session revoke on delete. The raw token is still never stored — + * only its SHA-256 hash reaches the repository, computed here exactly as + * before. * * Deliberately NOT here: verifyJwtToken (that is login identity, and it stays * next to the cookie/JWT logic on AuthService) and isDemoUser (a demo gate that @@ -39,7 +46,8 @@ type TokenKind = 'mcp' | 'api'; @Injectable() export class TokenService { constructor( - private readonly db: DatabaseService, + @InjectRepository(McpTokens) private readonly tokens: McpTokensRepository, + @InjectRepository(Users) private readonly users: UsersRepository, private readonly ephemeral: EphemeralTokenService, ) {} @@ -62,10 +70,7 @@ export class TokenService { } private async listTokens(userId: number, kind: TokenKind) { - const rows = this.db.all( - 'SELECT id, name, token_prefix, created_at, last_used_at, scope_mode, api_scopes FROM mcp_tokens WHERE user_id = ? AND kind = ? ORDER BY created_at DESC', - userId, kind - ); + const rows = await this.tokens.listByUserAndKind(userId, kind); // The MCP list keeps the exact shape it has always had: those tokens carry // no read scopes, and two columns that are always "everything" would be // noise in a panel that cannot act on them. @@ -106,7 +111,7 @@ export class TokenService { if (!name?.trim()) return { error: 'Token name is required', status: 400 }; if (name.trim().length > 100) return { error: 'Token name must be 100 characters or less', status: 400 }; - const tokenCount = this.db.get<{ count: number }>('SELECT COUNT(*) as count FROM mcp_tokens WHERE user_id = ? AND kind = ?', userId, kind)!.count; + const tokenCount = await this.tokens.countByUserAndKind(userId, kind); if (tokenCount >= 10) return { error: 'Maximum of 10 tokens per user reached', status: 400 }; const rawToken = 'trek_' + randomBytes(24).toString('hex'); @@ -117,22 +122,28 @@ export class TokenService { // the column default — rather than being handed a list it would ignore. const narrowed = kind === 'api' ? sanitizeScopes(scopes) : null; - const result = this.db.run( - 'INSERT INTO mcp_tokens (user_id, name, token_hash, token_prefix, kind, scope_mode, api_scopes) VALUES (?, ?, ?, ?, ?, ?, ?)', - userId, name.trim(), tokenHash, tokenPrefix, kind, - narrowed ? 'limited' : 'all', - narrowed ? JSON.stringify(narrowed) : null, - ); + const inserted = await this.tokens.insertToken({ + user_id: userId, + name: name.trim(), + token_hash: tokenHash, + token_prefix: tokenPrefix, + kind, + scope_mode: narrowed ? 'limited' : 'all', + api_scopes: narrowed ? JSON.stringify(narrowed) : null, + }); - const token = this.db.get( - 'SELECT id, name, token_prefix, created_at, last_used_at FROM mcp_tokens WHERE id = ?', - result.lastInsertRowid - ); + // A separate re-select, matching the legacy INSERT-then-SELECT shape + // exactly (TK3 then TK4) rather than reusing `inserted`'s full row — + // `findBasic` also serves TK9's admin lookup, so its `user_id` field is + // dropped here: TK4's response never carried it, and leaking it would be + // a new field on the client-facing token payload, not a refactor. + const basic = (await this.tokens.findBasic(inserted.id)) as McpTokenBasicRow; + const { user_id: _userId, ...token } = basic; const grant = kind === 'api' ? { scope_mode: narrowed ? 'limited' : 'all', scopes: narrowed ?? [...PUBLIC_API_SCOPES] } : {}; - return { token: { ...(token as object), ...grant, raw_token: rawToken } }; + return { token: { ...token, ...grant, raw_token: rawToken } }; } async deleteMcpToken(userId: number, tokenId: string) { @@ -149,9 +160,10 @@ export class TokenService { * missing from a screen they never opened. */ private async deleteToken(userId: number, tokenId: string, kind: TokenKind): Promise<{ error?: string; status?: number; success?: boolean }> { - const token = this.db.get('SELECT id FROM mcp_tokens WHERE id = ? AND user_id = ? AND kind = ?', tokenId, userId, kind); + const id = Number(tokenId); + const token = await this.tokens.findOwnedByKind(id, userId, kind); if (!token) return { error: 'Token not found', status: 404 }; - this.db.run('DELETE FROM mcp_tokens WHERE id = ?', tokenId); + await this.tokens.deleteById(id); // Best-effort, like the changePassword/resetPassword revocations: a session // sweep failure must not turn a successful token delete into a 500. try { revokeUserSessions?.(userId); } catch { /* best-effort */ } @@ -165,7 +177,7 @@ export class TokenService { async createWsToken(userId: number): Promise<{ error?: string; status?: number; token?: string }> { // Bind the ws-token to the user's current password_version so a token minted // before a password reset is rejected on connect (defence-in-depth session gate). - const pv = this.db.get<{ password_version?: number }>('SELECT password_version FROM users WHERE id = ?', userId)?.password_version ?? 0; + const pv = (await this.users.getPasswordVersion(userId)) ?? 0; const token = this.ephemeral.create(userId, 'ws', { pv }); if (!token) return { error: 'Service unavailable', status: 503 }; return { token }; @@ -185,7 +197,7 @@ export class TokenService { // Admin view // // The same table, seen across all users. Moved here from AdminService, which - // owned a second copy of the delete purely because the admin route lived + // only owned a second copy of the delete purely because the admin route lived // there. Note the two are genuinely different queries, not duplicates: the // user-facing ones scope every statement by user_id, these deliberately do // not, and the admin delete revokes sessions unconditionally where the @@ -193,18 +205,14 @@ export class TokenService { // ------------------------------------------------------------------------- async listAllMcpTokens() { - return this.db.all(` - SELECT t.id, t.name, t.token_prefix, t.created_at, t.last_used_at, t.user_id, u.username - FROM mcp_tokens t - JOIN users u ON u.id = t.user_id - ORDER BY t.created_at DESC - `); + return await this.tokens.listAllWithUsername(); } async adminDeleteMcpToken(id: string) { - const token = this.db.get<{ id: number; user_id: number }>('SELECT id, user_id FROM mcp_tokens WHERE id = ?', id); + const numericId = Number(id); + const token = await this.tokens.findBasic(numericId); if (!token) return { error: 'Token not found', status: 404 }; - this.db.run('DELETE FROM mcp_tokens WHERE id = ?', id); + await this.tokens.deleteById(numericId); revokeUserSessions(token.user_id); return {}; } @@ -231,14 +239,9 @@ export class TokenService { */ async verifyApiTokenWithGrant(rawToken: string): Promise<{ user: User; grant: PublicApiGrant } | null> { const hash = createHash('sha256').update(rawToken).digest('hex'); - const row = this.db.get(` - SELECT u.id, u.username, u.email, u.role, mt.scope_mode, mt.api_scopes - FROM mcp_tokens mt - JOIN users u ON mt.user_id = u.id - WHERE mt.token_hash = ? AND mt.kind = 'api' - `, hash); + const row = await this.tokens.findGrantByHash(hash); if (!row) return null; - this.db.run('UPDATE mcp_tokens SET last_used_at = CURRENT_TIMESTAMP WHERE token_hash = ?', hash); + await this.tokens.touchLastUsedByHash(hash); const { scope_mode, api_scopes, ...user } = row; return { user: user as User, grant: resolveGrant(scope_mode, api_scopes) }; } @@ -253,30 +256,18 @@ export class TokenService { */ private async verifyToken(rawToken: string, kind: TokenKind): Promise { const hash = createHash('sha256').update(rawToken).digest('hex'); - const row = this.db.get(` - SELECT u.id, u.username, u.email, u.role - FROM mcp_tokens mt - JOIN users u ON mt.user_id = u.id - WHERE mt.token_hash = ? AND mt.kind = ? - `, hash, kind); + const row = await this.tokens.findUserByHashAndKind(hash, kind); if (row) { - this.db.run('UPDATE mcp_tokens SET last_used_at = CURRENT_TIMESTAMP WHERE token_hash = ?', hash); - return row; + await this.tokens.touchLastUsedByHash(hash); + // `role` is `users.role TEXT`, narrower at runtime than the repository's + // row type states — same trust boundary the legacy `this.db.get()` + // generic asserted without a runtime check. + return row as User; } return null; } } -interface TokenRow { - id: number; - name: string; - token_prefix: string; - created_at: string; - last_used_at: string | null; - scope_mode: string | null; - api_scopes: string | null; -} - /** * The requested sections, cleaned up — unknown values dropped, duplicates * collapsed, order pinned to the canonical list so two keys with the same @@ -308,6 +299,13 @@ function sanitizeScopes(scopes: readonly string[] | undefined): PublicApiScope[] * flag instead of "NULL means everything" is that a key minted as restricted * must never widen on its own. A key that stops working is a support ticket; a * key that quietly reads every trip is the bug this feature exists to prevent. + * + * Still defends against `mode`/`raw` arriving `null`, even though + * `McpTokensRepository`'s rows type them as non-nullable strings (the + * `mcp_tokens.scope_mode` column is `NOT NULL DEFAULT 'all'`): this function + * is shared, pure, and untouched by the repository conversion — the + * defensiveness costs nothing and keeps `PUBAPI-SCOPE-U042` (a hand-built row + * simulating a pre-migration NULL) meaningful. */ function resolveGrant(mode: string | null, raw: string | null): PublicApiGrant { if (mode !== 'limited') return { mode: 'all', scopes: [...PUBLIC_API_SCOPES] }; diff --git a/server/src/nest/tokens/tokens.module.ts b/server/src/nest/tokens/tokens.module.ts index dea63a7a4..470693939 100644 --- a/server/src/nest/tokens/tokens.module.ts +++ b/server/src/nest/tokens/tokens.module.ts @@ -1,4 +1,7 @@ import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; +import { McpTokens } from '../../db/entities/McpTokens.entity'; +import { Users } from '../../db/entities/Users.entity'; import { TokenService } from './token.service'; import { EphemeralTokenModule } from '../auth/ephemeral-token.module'; @@ -6,12 +9,20 @@ import { EphemeralTokenModule } from '../auth/ephemeral-token.module'; * Tokens that are not the login JWT: the long-lived MCP tokens a user manages * in settings, plus the short-lived ws and download tokens. * - * A leaf module on purpose. It needs nothing but DatabaseService, which keeps - * it cheap to import — AuthModule, AdminModule and the MCP transport all reach - * for it, and any dependency added here lands in all three graphs. + * A leaf module on purpose. It needs nothing but its own two repositories, + * which keeps it cheap to import — AuthModule, AdminModule and the MCP + * transport all reach for it, and any dependency added here lands in all + * three graphs. `forFeature([McpTokens, Users])` resolves `TokenService`'s + * `@InjectRepository`s from THIS module's own graph regardless of who + * imports `TokensModule` (Plan 3b Task 1's guard-injection blast-radius + * reasoning does not apply here: that was about a class Nest instantiates by + * reference from `@UseGuards()`, resolved from the HOST CONTROLLER's module; + * `TokenService` is an ordinary provider declared in this module's own + * `providers`, so its constructor dependencies resolve from this module's + * own `imports`, exactly like any other Nest provider). */ @Module({ - imports: [EphemeralTokenModule], + imports: [EphemeralTokenModule, MikroOrmModule.forFeature([McpTokens, Users])], providers: [TokenService], exports: [TokenService], }) diff --git a/server/tests/e2e/public-api.e2e.test.ts b/server/tests/e2e/public-api.e2e.test.ts index 773cdd642..366dd02c1 100644 --- a/server/tests/e2e/public-api.e2e.test.ts +++ b/server/tests/e2e/public-api.e2e.test.ts @@ -87,6 +87,7 @@ import { PublicStatsController } from '../../src/nest/atlas/public-stats.control import { AtlasService } from '../../src/nest/atlas/atlas.service'; import { TrekExceptionFilter } from '../../src/nest/common/trek-exception.filter'; import { TestUnitOfWorkModule } from '../helpers/test-uow'; +import { createTestMikroOrmModule } from '../helpers/test-orm'; /** Mints a token the way TokenService does, so the guard's hash lookup is real. */ function seedToken(userId: number, raw: string, kind: 'api' | 'mcp' = 'api'): string { @@ -152,7 +153,7 @@ describe('Public API v1 e2e (real guard + real SQL)', () => { async function build() { const moduleRef = await Test.createTestingModule({ - imports: [await TestUnitOfWorkModule.forRoot(db), DatabaseModule, RateLimitModule, TokensModule, PublicApiModule], + imports: [await TestUnitOfWorkModule.forRoot(db), await createTestMikroOrmModule(db), DatabaseModule, RateLimitModule, TokensModule, PublicApiModule], // `/api/v1/stats` lives in atlas/ because its figures do, but it is guarded // and scoped by this directory's code — so it is mounted here with the real // guard and a stubbed AtlasService. Importing AtlasModule instead would pull diff --git a/server/tests/helpers/factories.ts b/server/tests/helpers/factories.ts index 6407bd9d1..ad7fa343d 100644 --- a/server/tests/helpers/factories.ts +++ b/server/tests/helpers/factories.ts @@ -472,14 +472,17 @@ export interface TestMcpToken { export function createMcpToken( db: Database.Database, userId: number, - overrides: Partial<{ name: string; rawToken: string }> = {} + overrides: Partial<{ name: string; rawToken: string; kind: string; scope_mode: string; api_scopes: string | null }> = {} ): TestMcpToken { const rawToken = overrides.rawToken ?? `trek_test_${Date.now()}_${Math.random().toString(36).slice(2)}`; const tokenHash = createHash('sha256').update(rawToken).digest('hex'); const tokenPrefix = rawToken.slice(0, 12); const result = db.prepare( - 'INSERT INTO mcp_tokens (user_id, token_hash, token_prefix, name) VALUES (?, ?, ?, ?)' - ).run(userId, tokenHash, tokenPrefix, overrides.name ?? 'Test Token'); + 'INSERT INTO mcp_tokens (user_id, token_hash, token_prefix, name, kind, scope_mode, api_scopes) VALUES (?, ?, ?, ?, ?, ?, ?)' + ).run( + userId, tokenHash, tokenPrefix, overrides.name ?? 'Test Token', + overrides.kind ?? 'mcp', overrides.scope_mode ?? 'all', overrides.api_scopes ?? null, + ); return { id: result.lastInsertRowid as number, tokenHash, rawToken }; } diff --git a/server/tests/helpers/test-uow.ts b/server/tests/helpers/test-uow.ts index 3ceea61e1..aef8432d6 100644 --- a/server/tests/helpers/test-uow.ts +++ b/server/tests/helpers/test-uow.ts @@ -12,6 +12,14 @@ import { Settings } from '../../src/db/entities/Settings.entity'; import type { SettingsRepository } from '../../src/db/repositories/Settings.repository'; import { Users } from '../../src/db/entities/Users.entity'; import type { UsersRepository } from '../../src/db/repositories/Users.repository'; +import { WebauthnCredentials } from '../../src/db/entities/WebauthnCredentials.entity'; +import type { WebauthnCredentialsRepository } from '../../src/db/repositories/WebauthnCredentials.repository'; +import { WebauthnChallenges } from '../../src/db/entities/WebauthnChallenges.entity'; +import type { WebauthnChallengesRepository } from '../../src/db/repositories/WebauthnChallenges.repository'; +import { InviteTokens } from '../../src/db/entities/InviteTokens.entity'; +import type { InviteTokensRepository } from '../../src/db/repositories/InviteTokens.repository'; +import { McpTokens } from '../../src/db/entities/McpTokens.entity'; +import type { McpTokensRepository } from '../../src/db/repositories/McpTokens.repository'; const perHandle = new WeakMap>(); const appSettingsPerHandle = new WeakMap>(); @@ -172,3 +180,64 @@ export class TestUnitOfWorkModule { }; } } + +// --------------------------------------------------------------------------- +// Plan 3b Task 2 (TokenService) — appended after every Task 0/1/3a helper +// above, before Task 3's own block (which appends after this one, per each +// task's file-ownership rule). Same memoisation-per-handle pattern as every +// helper above. +// --------------------------------------------------------------------------- + +const mcpTokensPerHandle = new WeakMap>(); + +/** The `McpTokensRepository` a hand-constructed `TokenService` needs (Plan 3b Task 2). */ +export function createTestMcpTokensRepo(db: Database.Database): Promise { + const existing = mcpTokensPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(McpTokens)); + mcpTokensPerHandle.set(db, pending); + return pending; +} + +// --------------------------------------------------------------------------- +// Plan 3b Task 3 (PasskeyService / RegistrationInvitesService) — appended at +// the end per the task's own file-ownership rule (Task 2 owns everything +// above this point in this file). Same memoisation-per-handle pattern as +// every helper above: one ORM per test file's better-sqlite3 handle, shared +// with whatever `UnitOfWork`/other repository that same file also builds. +// --------------------------------------------------------------------------- + +const webauthnCredentialsPerHandle = new WeakMap>(); +const webauthnChallengesPerHandle = new WeakMap>(); +const inviteTokensPerHandle = new WeakMap>(); + +/** The `WebauthnCredentialsRepository` a hand-constructed `PasskeyService` needs. */ +export function createTestWebauthnCredentialsRepo(db: Database.Database): Promise { + const existing = webauthnCredentialsPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(WebauthnCredentials)); + webauthnCredentialsPerHandle.set(db, pending); + return pending; +} + +/** The `WebauthnChallengesRepository` a hand-constructed `PasskeyService` needs. */ +export function createTestWebauthnChallengesRepo(db: Database.Database): Promise { + const existing = webauthnChallengesPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(WebauthnChallenges)); + webauthnChallengesPerHandle.set(db, pending); + return pending; +} + +/** + * The `InviteTokensRepository` a hand-constructed `RegistrationInvitesService` + * (or `AuthService`) needs — Task 0 built the class, this is the first test + * helper for it. + */ +export function createTestInviteTokensRepo(db: Database.Database): Promise { + const existing = inviteTokensPerHandle.get(db); + if (existing !== undefined) return existing; + const pending = sharedTestOrm(db).then((t) => t.repo(InviteTokens)); + inviteTokensPerHandle.set(db, pending); + return pending; +} diff --git a/server/tests/unit/db/repositories/McpTokens.repository.test.ts b/server/tests/unit/db/repositories/McpTokens.repository.test.ts new file mode 100644 index 000000000..187c408e7 --- /dev/null +++ b/server/tests/unit/db/repositories/McpTokens.repository.test.ts @@ -0,0 +1,389 @@ +import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { createHash } from 'crypto'; +import { createSnapshotTestDb } from '../../../helpers/db-mock'; +import { resetTestDb } from '../../../helpers/test-db'; +import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; +import { createMcpToken, createUser } from '../../../helpers/factories'; +import { McpTokens } from '../../../../src/db/entities/McpTokens.entity'; +import type { McpTokensRepository } from '../../../../src/db/repositories/McpTokens.repository'; +import { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; + +const testDb = createSnapshotTestDb(); +let t: TestOrm; +let tokens: McpTokensRepository; + +beforeAll(async () => { + t = await createTestOrm(testDb); + tokens = t.repo(McpTokens); +}); +beforeEach(() => { + resetTestDb(testDb); + t.clear(); +}); +afterAll(async () => { + await t.close(); + testDb.close(); +}); + +function rawToken(id: number): unknown { + return testDb.prepare('SELECT * FROM mcp_tokens WHERE id = ?').get(id); +} + +describe('McpTokensRepository', () => { + describe('listByUserAndKind', () => { + it('MCPTOKREPO-001: scoped to user + kind, ordered by created_at DESC, exact column set', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + const older = createMcpToken(testDb, user.id, { name: 'older', kind: 'mcp' }); + testDb.prepare("UPDATE mcp_tokens SET created_at = '2020-01-01 00:00:00' WHERE id = ?").run(older.id); + const newer = createMcpToken(testDb, user.id, { name: 'newer', kind: 'mcp' }); + testDb.prepare("UPDATE mcp_tokens SET created_at = '2020-01-02 00:00:00' WHERE id = ?").run(newer.id); + createMcpToken(testDb, user.id, { name: 'an api key', kind: 'api' }); + createMcpToken(testDb, other.id, { name: 'not mine', kind: 'mcp' }); + + const rows = await tokens.listByUserAndKind(user.id, 'mcp'); + expect(rows.map((r) => r.name)).toEqual(['newer', 'older']); + expect(Object.keys(rows[0]).sort()).toEqual( + ['api_scopes', 'created_at', 'id', 'last_used_at', 'name', 'scope_mode', 'token_prefix'].sort(), + ); + }); + + it('MCPTOKREPO-002: no tokens for the kind returns an empty array', async () => { + const { user } = createUser(testDb); + expect(await tokens.listByUserAndKind(user.id, 'api')).toEqual([]); + }); + }); + + describe('countByUserAndKind', () => { + it('MCPTOKREPO-003: counts only the matching user + kind', async () => { + const { user } = createUser(testDb); + createMcpToken(testDb, user.id, { kind: 'mcp' }); + createMcpToken(testDb, user.id, { kind: 'mcp' }); + createMcpToken(testDb, user.id, { kind: 'api' }); + expect(await tokens.countByUserAndKind(user.id, 'mcp')).toBe(2); + expect(await tokens.countByUserAndKind(user.id, 'api')).toBe(1); + }); + }); + + describe('insertToken', () => { + it('MCPTOKREPO-004: inserts and returns exactly the row a legacy re-select would return', async () => { + const { user } = createUser(testDb); + const row = await tokens.insertToken({ + user_id: user.id, + name: 'My Token', + token_hash: 'hash-value', + token_prefix: 'trek_abc123', + kind: 'mcp', + scope_mode: 'all', + api_scopes: null, + }); + expect(row).toStrictEqual(rawToken(row.id)); + expect(row.user_id).toBe(user.id); + expect(row.name).toBe('My Token'); + expect(row.token_hash).toBe('hash-value'); + expect(row.kind).toBe('mcp'); + expect(row.scope_mode).toBe('all'); + expect(row.api_scopes).toBeNull(); + expect(row.last_used_at).toBeNull(); + expect(typeof row.created_at).toBe('string'); + }); + + it('MCPTOKREPO-005: api_scopes is written when narrowed', async () => { + const { user } = createUser(testDb); + const row = await tokens.insertToken({ + user_id: user.id, + name: 'Narrowed', + token_hash: 'hash-2', + token_prefix: 'trek_def456', + kind: 'api', + scope_mode: 'limited', + api_scopes: '["stats","trips"]', + }); + expect(row.scope_mode).toBe('limited'); + expect(row.api_scopes).toBe('["stats","trips"]'); + }); + }); + + describe('findBasic', () => { + it('MCPTOKREPO-006: returns the TK4/TK9 union column set by id', async () => { + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { name: 'lookup-me' }); + const row = await tokens.findBasic(created.id); + expect(row).toEqual({ + id: created.id, + user_id: user.id, + name: 'lookup-me', + token_prefix: expect.any(String), + created_at: expect.any(String), + last_used_at: null, + }); + }); + + it('MCPTOKREPO-007: an unknown id is null', async () => { + expect(await tokens.findBasic(999999)).toBeNull(); + }); + + it('MCPTOKREPO-007b: identity-map regression — sees a raw UPDATE on the same id in the same request', async () => { + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { name: 'before' }); + await tokens.findBasic(created.id); // populate identity map + testDb.prepare("UPDATE mcp_tokens SET name = 'after' WHERE id = ?").run(created.id); + const row = await tokens.findBasic(created.id); + expect(row?.name).toBe('after'); + }); + }); + + describe('findOwnedByKind', () => { + it('MCPTOKREPO-008: matches on id + user + kind', async () => { + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { kind: 'mcp' }); + expect(await tokens.findOwnedByKind(created.id, user.id, 'mcp')).toEqual({ id: created.id }); + }); + + it('MCPTOKREPO-009: a different owner is null (ownership scoping)', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { kind: 'mcp' }); + expect(await tokens.findOwnedByKind(created.id, other.id, 'mcp')).toBeNull(); + }); + + it('MCPTOKREPO-010: the wrong kind is null (kind scoping)', async () => { + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { kind: 'mcp' }); + expect(await tokens.findOwnedByKind(created.id, user.id, 'api')).toBeNull(); + }); + }); + + describe('deleteById', () => { + it('MCPTOKREPO-011: deletes the row', async () => { + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id); + await tokens.deleteById(created.id); + expect(rawToken(created.id)).toBeUndefined(); + }); + }); + + describe('listAllWithUsername', () => { + it('MCPTOKREPO-012: spans users, carries the owner username, ordered by created_at DESC', async () => { + const { user: ada } = createUser(testDb, { username: 'ada' }); + const { user: bob } = createUser(testDb, { username: 'bob' }); + const first = createMcpToken(testDb, ada.id, { name: 'first' }); + testDb.prepare("UPDATE mcp_tokens SET created_at = '2020-01-01 00:00:00' WHERE id = ?").run(first.id); + const second = createMcpToken(testDb, bob.id, { name: 'second' }); + testDb.prepare("UPDATE mcp_tokens SET created_at = '2020-06-01 00:00:00' WHERE id = ?").run(second.id); + + const rows = await tokens.listAllWithUsername(); + expect(rows.map((r) => r.name)).toEqual(['second', 'first']); + expect(rows.find((r) => r.name === 'first')).toEqual({ + id: first.id, + name: 'first', + token_prefix: expect.any(String), + created_at: '2020-01-01 00:00:00', + last_used_at: null, + user_id: ada.id, + username: 'ada', + }); + expect(rows.find((r) => r.name === 'second')?.username).toBe('bob'); + }); + + it('MCPTOKREPO-012b: no token_hash leaks through the projection', async () => { + const { user } = createUser(testDb); + createMcpToken(testDb, user.id); + const rows = await tokens.listAllWithUsername(); + expect(rows.every((r) => !('token_hash' in r))).toBe(true); + }); + }); + + describe('findGrantByHash', () => { + it('MCPTOKREPO-013: an API-kind token resolves the user + grant columns', async () => { + const { user } = createUser(testDb, { username: 'grant-user' }); + createMcpToken(testDb, user.id, { + kind: 'api', + rawToken: 'trek_grant_raw', + scope_mode: 'limited', + api_scopes: '["stats"]', + }); + const hash = createHash('sha256').update('trek_grant_raw').digest('hex'); + const row = await tokens.findGrantByHash(hash); + expect(row).toEqual({ + id: user.id, + username: 'grant-user', + email: user.email, + role: user.role, + scope_mode: 'limited', + api_scopes: '["stats"]', + }); + }); + + it('MCPTOKREPO-014: the same hash under kind=mcp does not resolve — kind is in the WHERE', async () => { + const { user } = createUser(testDb); + createMcpToken(testDb, user.id, { kind: 'mcp', rawToken: 'trek_mcp_raw' }); + const hash = createHash('sha256').update('trek_mcp_raw').digest('hex'); + expect(await tokens.findGrantByHash(hash)).toBeNull(); + }); + + it('MCPTOKREPO-015: an unknown hash is null', async () => { + expect(await tokens.findGrantByHash('does-not-exist')).toBeNull(); + }); + }); + + describe('findUserByHashAndKind', () => { + it('MCPTOKREPO-016: a matching hash + kind resolves the user identity columns', async () => { + const { user } = createUser(testDb, { username: 'verify-user' }); + createMcpToken(testDb, user.id, { kind: 'mcp', rawToken: 'trek_verify_raw' }); + const hash = createHash('sha256').update('trek_verify_raw').digest('hex'); + const row = await tokens.findUserByHashAndKind(hash, 'mcp'); + expect(row).toEqual({ id: user.id, username: 'verify-user', email: user.email, role: user.role }); + }); + + it('MCPTOKREPO-017: a real token of the wrong kind is indistinguishable from a missing one', async () => { + const { user } = createUser(testDb); + createMcpToken(testDb, user.id, { kind: 'api', rawToken: 'trek_wrong_kind' }); + const hash = createHash('sha256').update('trek_wrong_kind').digest('hex'); + expect(await tokens.findUserByHashAndKind(hash, 'mcp')).toBeNull(); + expect(await tokens.findUserByHashAndKind('completely-unknown-hash', 'mcp')).toBeNull(); + }); + + it('MCPTOKREPO-018: an unknown hash is null', async () => { + expect(await tokens.findUserByHashAndKind('nope', 'api')).toBeNull(); + }); + }); + + describe('touchLastUsedByHash', () => { + it('MCPTOKREPO-019: stamps last_used_at only on the matching row, no other column changes', async () => { + const { user } = createUser(testDb); + const target = createMcpToken(testDb, user.id, { rawToken: 'trek_touch_me' }); + const other = createMcpToken(testDb, user.id, { rawToken: 'trek_leave_me' }); + const before = rawToken(target.id) as { name: string; token_hash: string }; + + await tokens.touchLastUsedByHash(target.tokenHash); + + const after = rawToken(target.id) as Record; + expect(after.last_used_at).not.toBeNull(); + expect(after.name).toBe(before.name); + expect(after.token_hash).toBe(before.token_hash); + expect((rawToken(other.id) as { last_used_at: string | null }).last_used_at).toBeNull(); + }); + + it('MCPTOKREPO-019b: an unknown hash touches nothing (no error, no row)', async () => { + await expect(tokens.touchLastUsedByHash('nonexistent')).resolves.toBeUndefined(); + }); + }); + + describe('deleteAllForUser', () => { + it('MCPTOKREPO-020: deletes every mcp_tokens row for the user, both kinds, leaves other users alone', async () => { + const { user } = createUser(testDb); + const { user: other } = createUser(testDb); + createMcpToken(testDb, user.id, { kind: 'mcp' }); + createMcpToken(testDb, user.id, { kind: 'api' }); + const untouched = createMcpToken(testDb, other.id, { kind: 'mcp' }); + + await tokens.deleteAllForUser(user.id); + + expect(testDb.prepare('SELECT COUNT(*) c FROM mcp_tokens WHERE user_id = ?').get(user.id)).toEqual({ c: 0 }); + expect(rawToken(untouched.id)).toBeDefined(); + }); + }); + + // --------------------------------------------------------------------- + // Controller ruling on Task 1's review (BLOCKER): a `findOne`/`find` with + // `fields` + `refresh: true` used to re-snapshot only the selected fields + // into the SHARED identity map; loading the same row again elsewhere in + // the request with a different projection merged the two, and a later + // `nativeUpdate` (which bypasses the identity map) left that merged entity + // stale — the closing `flush()` of a `uow.transactional` block could then + // write the stale unselected values back over the `nativeUpdate`. Every + // entity-hydrating read in this repository now takes + // `disableIdentityMap: true` instead (never merged anywhere a flush could + // find it), and `insertToken` writes through `em.insert` rather than + // `create()` + `persist()` + `flush()`. This is the regression proof. + // --------------------------------------------------------------------- + describe('identity-map isolation (disableIdentityMap regression)', () => { + /** + * The precise, mutation-proven form of the proof: `disableIdentityMap: + * true` means the returned entity is never merged into + * `EntityManager.getUnitOfWork()`'s tracked set at all — checked + * directly via `getById`, which is the same lookup MikroORM's own + * `flush()` consults to decide what to write. This is deliberately NOT + * a black-box "does a stale value get written back" behavioural test: + * hand-mutation-testing this repository (reverting `findBasic` to + * `refresh: true` and `findOwnedByKind`/`listByUserAndKind` to no + * `disableIdentityMap`, five different projection-pairing/nativeUpdate/ + * flush orderings tried by hand) never reproduced a stale write-back on + * the installed MikroORM 7.2.1 + `better-sqlite3` combination — flush()'s + * dirty-checking finds nothing to write for an entity nothing ever + * mutated via `assign()`/direct property set, which is exactly the + * shape this repository's methods have (`nativeUpdate` only, never + * `assign()+flush()`, unlike `UsersRepository.patchProfile`'s callers). + * `getById` is the one assertion that DOES fail on the un-fixed code + * (verified: reverting `findBasic` to `refresh: true` flips `tracked1` + * below from `false` to `true`) — the tracking itself is the hazard the + * ruling targets, whether or not this particular table's call shapes + * manage to turn it into an observable stale write today. + */ + it('MCPTOKREPO-021: every entity-hydrating read leaves nothing behind in the shared identity map', async () => { + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { name: 'stays-original' }); + + await tokens.findBasic(created.id); + expect(t.em.getUnitOfWork().getById(McpTokens, created.id)).toBeUndefined(); + + await tokens.findOwnedByKind(created.id, user.id, 'mcp'); + expect(t.em.getUnitOfWork().getById(McpTokens, created.id)).toBeUndefined(); + + const [listed] = await tokens.listByUserAndKind(user.id, 'mcp'); + expect(t.em.getUnitOfWork().getById(McpTokens, listed.id)).toBeUndefined(); + }); + + it('MCPTOKREPO-021b: a nativeUpdate made after an isolated read is never discarded by an unrelated flush', async () => { + // Belt-and-suspenders behavioural pin alongside MCPTOKREPO-021's + // structural proof above: even though hand mutation-testing could not + // turn the un-isolated shape into an observable failure here (see that + // test's docstring), the fixed code's actual end-to-end behaviour — + // read, nativeUpdate, flush, still correct — is still worth pinning. + const { user } = createUser(testDb); + const created = createMcpToken(testDb, user.id, { name: 'stays-original' }); + const uow = new UnitOfWork(t.em); + + await uow.transactional(async () => { + const basic = await tokens.findBasic(created.id); + expect(basic?.last_used_at).toBeNull(); + await tokens.findOwnedByKind(created.id, user.id, 'mcp'); + await tokens.touchLastUsedByHash(created.tokenHash); + }); + + const row = rawToken(created.id) as { last_used_at: string | null; name: string }; + expect(row.last_used_at).not.toBeNull(); + expect(row.name).toBe('stays-original'); + }); + + it('MCPTOKREPO-022: insertToken never flushes an unrelated dirty entity sitting in the shared identity map', async () => { + // A create()+persist()+flush() shape would flush the WHOLE unit of + // work of the shared context-resolving EntityManager, not just the + // row being inserted — including any other managed entity's unflushed + // in-memory change (D4/AuditLogRepository's own precedent for this + // exact hazard). `em.insert()` fires one native INSERT with no such + // side effect. Proof: load a DIFFERENT row through the plain (merging) + // EntityManager API, mutate it in memory only, call `insertToken`, and + // confirm the mutation was never written — nothing here ever called + // `flush()` on that dirty entity's context. + const { user } = createUser(testDb); + const untouched = createMcpToken(testDb, user.id, { name: 'do-not-flush-me' }); + const dirty = await t.em.findOneOrFail(McpTokens, { id: untouched.id }); + dirty.name = 'mutated-in-memory-only'; + + await tokens.insertToken({ + user_id: user.id, + name: 'the actual insert', + token_hash: 'insert-no-side-effect-hash', + token_prefix: 'trek_txprefi', + kind: 'mcp', + scope_mode: 'all', + api_scopes: null, + }); + + expect((rawToken(untouched.id) as { name: string }).name).toBe('do-not-flush-me'); + expect(testDb.prepare('SELECT id FROM mcp_tokens WHERE token_hash = ?').get('insert-no-side-effect-hash')).toBeDefined(); + }); + }); +}); diff --git a/server/tests/unit/nest/auth.service.test.ts b/server/tests/unit/nest/auth.service.test.ts index f5e50c216..4e1f5d824 100644 --- a/server/tests/unit/nest/auth.service.test.ts +++ b/server/tests/unit/nest/auth.service.test.ts @@ -84,7 +84,7 @@ import { MailerService } from '../../../src/nest/notifications/mailer/mailer.ser import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service'; import { DEFAULT_ALLOWED_EXTENSIONS } from '../../../src/nest/files/files.constants'; -import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestUsersRepo, createTestMcpTokensRepo } 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. @@ -97,9 +97,10 @@ const membershipStub = { joinTripAsMember } as unknown as TripMembershipService; // Tokens left AuthService for tokens/token.service.ts. The two cases below still // need one as a fixture: changePassword prunes MCP tokens, and the bridge parity // case verifies a token it just minted. -const tokens = new TokenService(new DatabaseService(testDb), new EphemeralTokenService()); +let tokens: TokenService; let svc: AuthService; beforeAll(async () => { + tokens = new TokenService(await createTestMcpTokensRepo(testDb), await createTestUsersRepo(testDb), new EphemeralTokenService()); svc = new AuthService( new DatabaseService(testDb), new PermissionsService(await createTestAppSettingsRepo(testDb), await createTestUnitOfWork(testDb)), diff --git a/server/tests/unit/nest/public-api-scopes.test.ts b/server/tests/unit/nest/public-api-scopes.test.ts index ac8a69b08..fb39871e2 100644 --- a/server/tests/unit/nest/public-api-scopes.test.ts +++ b/server/tests/unit/nest/public-api-scopes.test.ts @@ -71,8 +71,10 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser } from '../../helpers/factories'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { TokenService } from '../../../src/nest/tokens/token.service'; +import type { McpTokensRepository } from '../../../src/db/repositories/McpTokens.repository'; +import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; +import { createTestMcpTokensRepo, createTestUsersRepo } from '../../helpers/test-uow'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; import { ApiTokenGuard } from '../../../src/nest/public-api/api-token.guard'; import { PublicApiController } from '../../../src/nest/public-api/public-api.controller'; @@ -394,11 +396,12 @@ describe('PublicStatsController — the widest answer on the surface', () => { // --------------------------------------------------------------------------- describe('TokenService — storing and resolving a grant', () => { - const tokens = new TokenService(new DatabaseService(testDb), new EphemeralTokenService()); + let tokens: TokenService; - beforeAll(() => { + beforeAll(async () => { createTables(testDb); runMigrations(testDb); + tokens = new TokenService(await createTestMcpTokensRepo(testDb), await createTestUsersRepo(testDb), new EphemeralTokenService()); }); beforeEach(() => { @@ -446,15 +449,20 @@ describe('TokenService — storing and resolving a grant', () => { /** * The backwards-compatibility case, at the layer that decides it. A row written * before the columns existed carries neither value; the ALTER backfills - * 'all', and a DatabaseService that hands back NULL anyway must land in the - * same place. Both are "nobody ever narrowed this key". + * 'all', and a repository read that hands back NULL anyway must land in the + * same place — `McpTokensRepository.findGrantByHash`'s row type states + * `scope_mode`/`api_scopes` as non-nullable (the column is `NOT NULL + * DEFAULT 'all'`), so this fakes the repository directly rather than a raw + * row, the same way the legacy version faked `DatabaseService` directly. + * Both are "nobody ever narrowed this key". */ it('PUBAPI-SCOPE-U042: a row with no narrowing at all still reads everything', async () => { const noColumns = new TokenService( { - get: () => ({ id: 3, username: 'ada', email: 'a@b.c', role: 'user', scope_mode: null, api_scopes: null }), - run: () => ({ changes: 1 }), - } as unknown as DatabaseService, + findGrantByHash: async () => ({ id: 3, username: 'ada', email: 'a@b.c', role: 'user', scope_mode: null, api_scopes: null }), + touchLastUsedByHash: async () => {}, + } as unknown as McpTokensRepository, + {} as UsersRepository, new EphemeralTokenService(), ); expect((await noColumns.verifyApiTokenWithGrant('trek_legacy'))?.grant).toEqual({ diff --git a/server/tests/unit/nest/token.service.test.ts b/server/tests/unit/nest/token.service.test.ts index 632417b9b..68e04cbf0 100644 --- a/server/tests/unit/nest/token.service.test.ts +++ b/server/tests/unit/nest/token.service.test.ts @@ -42,17 +42,22 @@ import { resetTestDb } from '../../helpers/test-db'; import { createUser } from '../../helpers/factories'; import { TokenService } from '../../../src/nest/tokens/token.service'; import { TokensModule } from '../../../src/nest/tokens/tokens.module'; -import { DatabaseService } from '../../../src/nest/database/database.service'; import { createEphemeralToken } from '../../../src/nest/auth/ephemeral-tokens'; import { revokeUserSessions } from '../../../src/mcp/sessionManager'; import { expectRegisteredProvider } from '../../helpers/module-providers'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; +import { createTestMcpTokensRepo, createTestUsersRepo } from '../../helpers/test-uow'; -const svc = new TokenService(new DatabaseService(testDb), new EphemeralTokenService()); +// Schema first, then the ORM-backed repositories `TokenService` now takes +// (Plan 3b Task 2) — `sharedTestOrm` (behind `createTestMcpTokensRepo`/ +// `createTestUsersRepo`) shares this suite's own better-sqlite3 handle, so +// the tables must already exist before it initialises. +let svc: TokenService; -beforeAll(() => { +beforeAll(async () => { createTables(testDb); runMigrations(testDb); + svc = new TokenService(await createTestMcpTokensRepo(testDb), await createTestUsersRepo(testDb), new EphemeralTokenService()); }); beforeEach(() => { @@ -262,7 +267,7 @@ describe('MCP token service (admin view)', () => { await svc.createMcpToken(user.id, 'a'); await svc.createMcpToken(other.id, 'b'); - const all = await svc.listAllMcpTokens() as Record[]; + const all = await svc.listAllMcpTokens() as unknown as Record[]; expect(all).toHaveLength(2); expect(all.every(t => typeof t.username === 'string')).toBe(true); expect(all.some(t => t.token_hash !== undefined)).toBe(false); diff --git a/server/tests/websocket/connection.test.ts b/server/tests/websocket/connection.test.ts index 930409258..05f13898e 100644 --- a/server/tests/websocket/connection.test.ts +++ b/server/tests/websocket/connection.test.ts @@ -34,15 +34,19 @@ import { createUser, createTrip } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; import { createEphemeralToken } from '../../src/nest/auth/ephemeral-tokens'; import { TokenService } from '../../src/nest/tokens/token.service'; -import { DatabaseService } from '../../src/nest/database/database.service'; import { EphemeralTokenService } from '../../src/nest/auth/ephemeral-token.service'; +import { createTestMcpTokensRepo, createTestUsersRepo } from '../helpers/test-uow'; // The gateway consumes ws-tokens through its injected TokenService; the // ephemeral store is module-scoped on purpose, so a directly-constructed // instance mints tokens the app under test accepts (TokenService is a leaf — -// its own unit suite constructs it the same way). -const tokenService = new TokenService(new DatabaseService(testDb), new EphemeralTokenService()); -const createWsToken = (...args: Parameters) => +// its own unit suite constructs it the same way). `createSnapshotTestDb` +// (behind the `../../src/db/database` mock above) already carries the full +// migrated schema, so the repositories can build immediately — no +// `createTables`/`runMigrations` step needed first, unlike suites that start +// from a bare `:memory:` handle. +let tokenService: TokenService; +const createWsToken = (...args: Parameters) => tokenService.createWsToken(...args); let server: http.Server; @@ -50,6 +54,7 @@ let wsUrl: string; let nestApp: INestApplication; beforeAll(async () => { + tokenService = new TokenService(await createTestMcpTokensRepo(testDb), await createTestUsersRepo(testDb), new EphemeralTokenService()); // Real WebSocket against the unified NestJS app (Express is gone). buildApp owns // the same composition production uses; we attach the real ws server to it. nestApp = await buildApp();