diff --git a/apps/api/test/enrolment.test.ts b/apps/api/test/enrolment.test.ts index f4a3c8e2..a201f68b 100644 --- a/apps/api/test/enrolment.test.ts +++ b/apps/api/test/enrolment.test.ts @@ -149,12 +149,33 @@ describe('POST /machine/enrolment', () => { test('a user nobody has heard of is refused rather than crashing', async () => { const host = await registeredHost('enrol-ghost'); - const res = await enrol(host, { userId: 'usr_nosuchuseratall', steamId: steamId(6) }); + // Well-formed and simply absent, which is the case the foreign key + // catches. A malformed one never reaches the database at all. + const res = await enrol(host, { + userId: Identifier.ascending('user'), + steamId: steamId(6) + }); expect(res.status).toBe(404); const body = (await res.json()) as any; expect(body.type).toBe('not_found'); }); + test('a userId of the wrong shape is bad input, not a server fault', async () => { + // Ids live in a fixed-width column, so an overlong one is refused by + // the database rather than merely not found — and that refusal used to + // reach the host as a 500, which tells it to retry something that can + // never succeed. The width is checked where the input arrives. + const host = await registeredHost('enrol-misshapen'); + const malformed = [`usr_${'a'.repeat(40)}`, 'usr_short', `mch_${'a'.repeat(26)}`, 'nonsense']; + for (const userId of malformed) { + // eslint-disable-next-line no-await-in-loop + const res = await enrol(host, { userId, steamId: steamId(15) }); + expect(res.status).toBe(400); + // eslint-disable-next-line no-await-in-loop + expect(((await res.json()) as any).type).toBe('validation'); + } + }); + test('a Steam id has to look like one', async () => { const host = await registeredHost('enrol-badsteam'); const res = await enrol(host, { userId: host.userId, steamId: 'not-a-steam-id' }); @@ -165,7 +186,7 @@ describe('POST /machine/enrolment', () => { const res = await app.request('/machine/enrolment', { method: 'POST', headers: { 'x-nestri-admin-token': TEST_ADMIN_SECRET, 'content-type': 'application/json' }, - body: JSON.stringify({ userId: 'usr_x', steamId: steamId(7) }) + body: JSON.stringify({ userId: Identifier.ascending('user'), steamId: steamId(7) }) }); expect(res.status).toBe(403); expect(((await res.json()) as any).message).toContain('Machine credentials'); @@ -220,7 +241,7 @@ describe('POST /machine/enrolment/stale', () => { const res = await app.request('/machine/enrolment/stale', { method: 'POST', headers: { 'x-nestri-admin-token': TEST_ADMIN_SECRET, 'content-type': 'application/json' }, - body: JSON.stringify({ userId: 'usr_x' }) + body: JSON.stringify({ userId: Identifier.ascending('user') }) }); expect(res.status).toBe(403); }); diff --git a/packages/core/migrations/0012_steam_enrolment_without_a_token.sql b/packages/core/migrations/0012_steam_enrolment_without_a_token.sql index 38648778..f55c1964 100644 --- a/packages/core/migrations/0012_steam_enrolment_without_a_token.sql +++ b/packages/core/migrations/0012_steam_enrolment_without_a_token.sql @@ -25,6 +25,12 @@ -- itself only goes away when the machine or the user does, which the foreign -- keys already do. -- +-- The key begins with the machine, so it answers "what does this host hold" and +-- nothing else. `user_id` gets its own index because the two things that read +-- by user cannot use the key: deleting a user cascades into this table by that +-- column alone, and asking which hosts hold a token for one person is the +-- obvious next reader. +-- -- `last_ok_at` has no writer yet. A successful logon happens inside the -- workload, which holds no control-plane credential, so the report has to come -- back out through the host and nothing carries it today. The column exists @@ -44,4 +50,5 @@ CREATE TABLE "steam_enrolment" ( ); --> statement-breakpoint ALTER TABLE "steam_enrolment" ADD CONSTRAINT "steam_enrolment_machine_id_machine_id_fk" FOREIGN KEY ("machine_id") REFERENCES "public"."machine"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint -ALTER TABLE "steam_enrolment" ADD CONSTRAINT "steam_enrolment_user_id_user_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."user"("id") ON DELETE cascade ON UPDATE no action; \ No newline at end of file +ALTER TABLE "steam_enrolment" ADD CONSTRAINT "steam_enrolment_user_id_user_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."user"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint +CREATE INDEX "steam_enrolment_user_idx" ON "steam_enrolment" USING btree ("user_id"); \ No newline at end of file diff --git a/packages/core/migrations/meta/0012_snapshot.json b/packages/core/migrations/meta/0012_snapshot.json index abecf8dc..31297a46 100644 --- a/packages/core/migrations/meta/0012_snapshot.json +++ b/packages/core/migrations/meta/0012_snapshot.json @@ -1,5 +1,5 @@ { - "id": "5c6f5a6c-74ff-4b9d-95ac-bef2c2603273", + "id": "d519bc2f-25f8-46e1-b7df-67bf6b5a927a", "prevId": "6fdda454-c7ee-42c1-931b-fa595385aac0", "version": "7", "dialect": "postgresql", @@ -1860,7 +1860,23 @@ "notNull": false } }, - "indexes": {}, + "indexes": { + "steam_enrolment_user_idx": { + "name": "steam_enrolment_user_idx", + "columns": [ + { + "expression": "user_id", + "isExpression": false, + "asc": true, + "nulls": "last" + } + ], + "isUnique": false, + "concurrently": false, + "method": "btree", + "with": {} + } + }, "foreignKeys": { "steam_enrolment_machine_id_machine_id_fk": { "name": "steam_enrolment_machine_id_machine_id_fk", diff --git a/packages/core/migrations/meta/_journal.json b/packages/core/migrations/meta/_journal.json index b7a37435..dfae72e2 100644 --- a/packages/core/migrations/meta/_journal.json +++ b/packages/core/migrations/meta/_journal.json @@ -89,7 +89,7 @@ { "idx": 12, "version": "7", - "when": 1788690115352, + "when": 1788691753961, "tag": "0012_steam_enrolment_without_a_token", "breakpoints": true } diff --git a/packages/core/src/id.ts b/packages/core/src/id.ts index 83d000e7..9773885f 100644 --- a/packages/core/src/id.ts +++ b/packages/core/src/id.ts @@ -27,8 +27,24 @@ export namespace Identifier { refreshToken: 'rft' } as const; + /** + * An id as this control plane issues them: the right prefix, and the exact + * width the column has. + * + * The width is the half that matters at an API boundary. Ids are stored in + * a fixed-width column, so an overlong string is *refused by the database* + * rather than simply not matching anything — which surfaces to the caller + * as a server fault instead of the validation error it actually is. + * Checking it where the input arrives is what keeps the two apart. + * + * The separator is part of the prefix check for the same reason: without + * it, `usrsomething` reads as a user id. + */ export function schema(prefix: keyof typeof prefixes) { - return z.string().startsWith(prefixes[prefix]); + return z + .string() + .startsWith(`${prefixes[prefix]}_`) + .length(prefixes[prefix].length + 1 + LENGTH); } const LENGTH = 26; diff --git a/packages/core/src/steam/enrolment.sql.ts b/packages/core/src/steam/enrolment.sql.ts index 27e9ce7d..afd6d4f8 100644 --- a/packages/core/src/steam/enrolment.sql.ts +++ b/packages/core/src/steam/enrolment.sql.ts @@ -1,4 +1,4 @@ -import { pgEnum, pgTable, primaryKey, text } from 'drizzle-orm/pg-core'; +import { index, pgEnum, pgTable, primaryKey, text } from 'drizzle-orm/pg-core'; import { ulid, utc } from '../db/types.js'; import { MachineTable } from '../machine/machine.sql.js'; @@ -61,5 +61,12 @@ export const SteamEnrolmentTable = pgTable( lastOkAt: utc('last_ok_at'), revokedAt: utc('revoked_at') }, - (t) => [primaryKey({ columns: [t.machineId, t.userId] })] + (t) => [ + primaryKey({ columns: [t.machineId, t.userId] }), + // The key starts with the machine, which answers "what does this host + // hold" and nothing else. Deleting a user cascades into this table by + // `user_id` alone, and asking which hosts hold a token for one person + // is the obvious next reader — neither can use the key. + index('steam_enrolment_user_idx').on(t.userId) + ] ); diff --git a/packages/core/src/steam/enrolment.test.ts b/packages/core/src/steam/enrolment.test.ts index b1185752..2af5b031 100644 --- a/packages/core/src/steam/enrolment.test.ts +++ b/packages/core/src/steam/enrolment.test.ts @@ -2,6 +2,7 @@ import { afterAll, describe, expect, test } from 'bun:test'; import { Fixtures } from '../db/fixtures.js'; import { testDb } from '../db/test.js'; +import { Identifier } from '../id.js'; import { Enrolment } from './enrolment.js'; const sql = testDb(); @@ -121,6 +122,22 @@ describe('Enrolment.markStale', () => { }); }); +describe('the user foreign key has its own index', () => { + test('deleting a user, and asking by user, do not scan the table', async () => { + // The primary key starts with the machine, so neither of the two things + // that read by user alone can use it: the cascade behind a user + // deletion, and the question "which hosts hold a token for me". + const indexes = await sql<{ indexdef: string }[]>` + select indexdef from pg_indexes + where schemaname = 'public' + and tablename = 'steam_enrolment' + and indexname = 'steam_enrolment_user_idx' + `; + expect(indexes).toHaveLength(1); + expect(indexes[0]!.indexdef).toContain('user_id'); + }); +}); + describe('Enrolment.listByMachine', () => { test('every enrolment for one host, oldest first', async () => { const h = await host('core-list'); @@ -140,6 +157,6 @@ describe('Enrolment.listByMachine', () => { }); test('an unknown host has no enrolments rather than an error', async () => { - expect(await Enrolment.listByMachine('mch_nosuchmachine')).toEqual([]); + expect(await Enrolment.listByMachine(Identifier.ascending('machine'))).toEqual([]); }); }); diff --git a/packages/core/src/steam/enrolment.ts b/packages/core/src/steam/enrolment.ts index e79c667e..f46b224e 100644 --- a/packages/core/src/steam/enrolment.ts +++ b/packages/core/src/steam/enrolment.ts @@ -5,6 +5,7 @@ import { Database } from '../db/index.js'; import { ErrorCodes, VisibleError } from '../error.js'; import { Examples } from '../examples.js'; import { fn } from '../fn.js'; +import { Identifier } from '../id.js'; import { SteamEnrolmentState, SteamEnrolmentTable } from './enrolment.sql.js'; import { STEAM_ID_RE } from './index.js'; @@ -31,11 +32,15 @@ function isForeignKeyViolation(err: unknown): boolean { export namespace Enrolment { export const Info = z .object({ - machineId: z.string().meta({ + // Shaped, not merely non-empty. Both are foreign keys into + // fixed-width columns, so a string of the wrong width is rejected + // by the database itself — and a database refusal reaches a caller + // as a server fault rather than as the bad input it is. + machineId: Identifier.schema('machine').meta({ description: 'The host that holds a token for this user', example: Examples.SteamEnrolment.machineId }), - userId: z.string().meta({ + userId: Identifier.schema('user').meta({ description: 'The person the host signed in as', example: Examples.SteamEnrolment.userId }),