diff --git a/docs/architecture.md b/docs/architecture.md index f5c89ae..956b1db 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -356,8 +356,10 @@ re-checks the proof sessions, applies `decideMove` (one pure function both store call, so memory and Postgres cannot disagree), moves the rows with credentials stripped, revokes the source's sessions, expires its pending links and unredeemed completions, and appends the row. `identity_moves` is append-only (SELECT and -INSERT only, checked at startup). An alias is derived by `resolveAccount`, never -stored. +INSERT only, checked at startup). An alias (an account with no login and an +unreversed move from it) is derived inside the transaction to refuse moves into +or out of it, never stored; nothing resolves an alias to its destination, because +no consumer needs it and a chain of moves would make one hop wrong. ### Step-up proof has two sources diff --git a/docs/service-contract.md b/docs/service-contract.md index dd2c174..6b77caa 100644 --- a/docs/service-contract.md +++ b/docs/service-contract.md @@ -793,8 +793,8 @@ re-runs the revocation receipts, so a crash between commit and receipts heals on the caller's retry. Anything else under a used `moveId` is `409 move_id_conflict`. What a move leaves: an account holding no login with an unreversed move from it -is an alias of the move's destination (derived by `resolveAccount`, never -stored; one hop). A move into or out of an alias is `already_moved`. Bearers of +is an alias: derived at move time, never stored, and no route resolves it. A +move into or out of an alias is `already_moved`. Bearers of the destination keep their old identity list until they refresh. A sign-in through a moved login lands on the destination because the row moved. diff --git a/src/store/in-memory.ts b/src/store/in-memory.ts index 401201c..f12661d 100644 --- a/src/store/in-memory.ts +++ b/src/store/in-memory.ts @@ -271,12 +271,6 @@ export class InMemoryStore implements HeimdallStore { return clone(this.identityMoves.get(moveId) ?? null); } - async resolveAccount(accountId: string): Promise { - if ([...this.linkedIdentities.values()].some((identity) => identity.accountId === accountId)) return accountId; - const unreversed = [...this.identityMoves.values()].filter((move) => move.fromAccountId === accountId && !this.isReversed(move.id)); - return unreversed.at(-1)?.toAccountId ?? accountId; - } - private isReversed(moveId: string): boolean { return [...this.identityMoves.values()].some((move) => move.reversesMoveId === moveId); } diff --git a/src/store/postgres.ts b/src/store/postgres.ts index 8482a8a..f0da059 100644 --- a/src/store/postgres.ts +++ b/src/store/postgres.ts @@ -902,8 +902,11 @@ export class PostgresStore implements HeimdallStore { try { await client.query("BEGIN"); try { - // Both accounts, in id order, before anything is read: opposite moves queue here instead of deadlocking. - await client.query("SELECT id FROM accounts WHERE id = ANY($1) ORDER BY id FOR UPDATE", [[input.fromAccountId, input.toAccountId]]); + // Both accounts, one statement each in id order, before anything is read: opposite moves queue here + // instead of deadlocking. The order is the code's own, not whatever order a plan locks a set in. + for (const id of [input.fromAccountId, input.toAccountId].sort()) { + await client.query("SELECT id FROM accounts WHERE id = $1 FOR UPDATE", [id]); + } const existing = await selectIdentityMove(client, input.moveId); let forward: MoveFacts["forward"] = null; if (input.reversesMoveId !== undefined) { @@ -996,18 +999,6 @@ export class PostgresStore implements HeimdallStore { return selectIdentityMove(this.pool, moveId); } - async resolveAccount(accountId: string): Promise { - const held = await this.pool.query("SELECT 1 FROM linked_identities WHERE account_id = $1 LIMIT 1", [accountId]); - if (held.rowCount) return accountId; - const moved = await this.pool.query<{ to_account_id: string }>( - `SELECT m.to_account_id FROM identity_moves m - WHERE m.from_account_id = $1 AND NOT EXISTS (SELECT 1 FROM identity_moves r WHERE r.reverses_move_id = m.id) - ORDER BY m.created_at DESC, m.id DESC LIMIT 1`, - [accountId] - ); - return moved.rows[0]?.to_account_id ?? accountId; - } - async updateLinkedIdentityCredential( id: string, decide: (locked: StoredLinkedIdentity) => Promise, diff --git a/src/store/types.ts b/src/store/types.ts index 68459c3..78d12a4 100644 --- a/src/store/types.ts +++ b/src/store/types.ts @@ -404,11 +404,6 @@ export interface HeimdallStore { */ moveIdentities(input: MoveIdentitiesInput): Promise; findIdentityMove(moveId: string): Promise; - /** - * One hop, derived and never stored: the destination of an unreversed move - * from `accountId` when it now holds no identity, else `accountId` itself. - */ - resolveAccount(accountId: string): Promise; /** * Serialise decisions about one row's credential. `decide` receives the row * as read under the lock (never a caller's snapshot) and the decision is diff --git a/tests/identity-moves.test.ts b/tests/identity-moves.test.ts index 9ccd1ca..ab64782 100644 --- a/tests/identity-moves.test.ts +++ b/tests/identity-moves.test.ts @@ -4,7 +4,7 @@ import { join } from "node:path"; import { afterAll, afterEach, describe, expect, it, vi } from "vitest"; import { buildApp } from "../src/app.js"; import { builtInAppProfiles } from "../src/app-profiles.js"; -import { AUTH_PROOF_TTL_SECONDS, issueAccessClaim } from "../src/claims.js"; +import { AUTH_PROOF_TTL_SECONDS, issueAccessClaim, proofTier } from "../src/claims.js"; import { type AppSlug, type Provider } from "../src/contracts.js"; import { type OAuthProviderRuntime } from "../src/oauth.js"; import { createRuntimeKeyMaterial } from "../src/signing.js"; @@ -46,6 +46,10 @@ function twitchRuntime(providerUserId: string): OAuthProviderRuntime { }; } +function discordRuntime(providerUserId: string): OAuthProviderRuntime { + return { ...twitchRuntime(providerUserId), async resolveIdentity() { return { provider: "discord", providerUserId, username: providerUserId, profile: { id: providerUserId } }; } }; +} + async function rig(twitchUser = "t-signin") { const store = new InMemoryStore(); const audit: CreateAuditEventInput[] = []; @@ -54,7 +58,7 @@ async function rig(twitchUser = "t-signin") { audit.push(structuredClone(input)); return realAudit(input); }; - const app = await buildApp({ config, store, oauthRuntimes: { twitch: twitchRuntime(twitchUser) } }); + const app = await buildApp({ config, store, oauthRuntimes: { twitch: twitchRuntime(twitchUser), discord: discordRuntime("d-linked") } }); apps.push(app); const keys = createRuntimeKeyMaterial({ ...config, bootstrapSigningPrivateKeyOnMissing: false }); return { app, store, keys, audit }; @@ -184,6 +188,18 @@ describe("a sanctioned move", () => { expect(await held(r, s.to)).toHaveLength(3); }); + it("a replay whose proofs are another app's tokens, or another account's, is 409 move_id_conflict, not the stored receipt", async () => { + const r = await rig(); + const s = await scene(r); + const body = { moveId: moveId("rp"), fromProofs: [s.fromProof.token], toProof: s.toProof.token }; + expect((await move(r, body)).statusCode).toBe(200); + const foreign = await proof(r, s.from, { app: "streampixels", stepUp: passkey }); + expect(err(await move(r, { ...body, fromProofs: [foreign.token] }))).toEqual([409, "move_id_conflict"]); + const foreignTo = await proof(r, s.to, { app: "streampixels" }); + expect(err(await move(r, { ...body, toProof: foreignTo.token }))).toEqual([409, "move_id_conflict"]); + expect((await move(r, body)).statusCode, "the genuine proofs still replay").toBe(200); + }); + it("an app without the secret is 401; streampixels with its own secret is 403 not_allowed_for_app", async () => { const r = await rig(); const s = await scene(r); @@ -434,12 +450,10 @@ describe("what a move leaves behind", () => { }); describe("aliases and reversal", () => { - it("an emptied source is an alias; moving into or out of it is 409 already_moved; resolution is one hop", async () => { + it("an emptied source is an alias; moving into or out of it is 409 already_moved", async () => { const r = await rig(); const s = await scene(r); expect((await move(r, { moveId: moveId("t"), fromProofs: [s.fromProof.token], toProof: s.toProof.token })).statusCode).toBe(200); - expect(await r.store.resolveAccount(s.from.account.id)).toBe(s.to.account.id); - expect(await r.store.resolveAccount(s.to.account.id)).toBe(s.to.account.id); // A session can outlive its logins only by being minted for an account that holds none, which is how a stale caller would look. const aliasProof = await proof(r, { account: s.from.account, rows: [s.to.rows[0]!] }, { stepUp: passkey }); const third = await acct(r, [["github"]]); @@ -469,7 +483,6 @@ describe("aliases and reversal", () => { expect(await held(r, s.from)).toEqual(sourceIds); expect(await held(r, s.to)).toHaveLength(1); expect((await r.store.listStoredLinkedIdentitiesForAccount(s.from.account.id)).every((i) => i.accessTokenEncrypted === undefined)).toBe(true); - expect(await r.store.resolveAccount(s.from.account.id)).toBe(s.from.account.id); const own2 = await proof(r, s.to, { via: 0 }); const again = await move(r, { moveId: moveId("w2"), fromProofs: [own2.token], reversesMoveId: moveId("v") }); expect(err(again)).toEqual([409, "not_reversible"]); @@ -480,6 +493,30 @@ describe("aliases and reversal", () => { }); }); +describe("proofTier", () => { + it("a proof dated in the future proves nothing, whichever claim carries it", () => { + const n = nowS(); + expect(proofTier({ auth_at: n + 5 }, n)).toBeNull(); + expect(proofTier({ step_up_at: n + 5 }, n)).toBeNull(); + expect(proofTier({ step_up_at: n + 5, auth_at: n }, n), "a future step-up does not hide a valid sign-in").toBe("weak"); + expect(proofTier({ step_up_at: n, auth_at: n - 9_999 }, n)).toBe("strong"); + }); +}); + +describe("the reversal's source must be the forward move's destination", () => { + it("is refused as not_reversible before the proofs are judged, even for a stale proof", async () => { + const r = await rig(); + const s = await scene(r); + expect((await move(r, { moveId: moveId("fw"), fromProofs: [s.fromProof.token], toProof: s.toProof.token })).statusCode).toBe(200); + const third = await acct(r, [["github"]]); + const onward = await proof(r, s.to, { stepUp: passkey }); + expect((await move(r, { moveId: moveId("on"), fromProofs: [onward.token], toProof: (await proof(r, third)).token })).statusCode).toBe(200); + const stale = await proof(r, third, { signedInAgo: 9_999 }); + const res = await move(r, { moveId: moveId("rv"), fromProofs: [stale.token], reversesMoveId: moveId("fw") }); + expect(err(res)).toEqual([409, "not_reversible"]); + }); +}); + describe("what a refusal says", () => { it("no refusal body names an account id, an email, a name or an identity the caller did not send", async () => { const r = await rig(); @@ -530,6 +567,31 @@ describe("claims: which sign-in a token came from", () => { expect(refreshed.json().claimSet.auth_identity_id).toBeUndefined(); }); + it("a link callback mints no sign-in proof: no auth_at, no auth_identity_id", async () => { + const r = await rig(); + const holder = await acct(r, [["twitch"]]); + const started = await r.app.inject({ method: "POST", url: "/v1/oauth/discord/start", headers: appAuth, payload: { appSlug: "bifrost", mode: "link", initiatingAccountId: holder.account.id, returnTo: "https://bifrost.gamecult.org/x" } }); + expect(started.statusCode).toBe(201); + const flow = await launchFlow(r.app, started.json()); + const called = await r.app.inject({ method: "GET", url: `/v1/oauth/discord/callback?code=c&state=${encodeURIComponent(flow.state)}`, headers: { cookie: flow.cookie, accept: "application/json" } }); + expect(called.statusCode).toBe(201); + expect(called.json().claimSet.identities).toHaveLength(2); + expect(called.json().claimSet.auth_at).toBeUndefined(); + expect(called.json().claimSet.auth_identity_id).toBeUndefined(); + expect(called.body).not.toMatch(/auth_at|auth_identity_id/); + }); + + it("a connect callback mints no sign-in proof either", async () => { + const r = await rig("t-connect"); + const started = await r.app.inject({ method: "POST", url: "/v1/oauth/twitch/start", headers: { "x-heimdall-app-secret": "streampixels-secret" }, payload: { appSlug: "streampixels", mode: "connect", returnTo: "https://streampixels.gamecult.org/signin/return", connection: { kind: "creator", targetId: "creator-1" } } }); + expect(started.statusCode, started.body).toBe(201); + const flow = await launchFlow(r.app, started.json()); + const called = await r.app.inject({ method: "GET", url: `/v1/oauth/twitch/callback?code=c&state=${encodeURIComponent(flow.state)}`, headers: { cookie: flow.cookie, accept: "application/json" } }); + expect(called.statusCode, called.body).toBe(201); + expect(called.body).toContain("claimSet"); + expect(called.body).not.toMatch(/auth_at|auth_identity_id/); + }); + it("an asserted step-up carries the bearer's auth claims through unchanged", async () => { const r = await rig(); const a = await acct(r, [["discord"], ["google"]]); diff --git a/tests/store.test.ts b/tests/store.test.ts index a0682dc..03b869a 100644 --- a/tests/store.test.ts +++ b/tests/store.test.ts @@ -659,7 +659,6 @@ async function exerciseStore(store: PostgresStore): Promise> { const moveId = `move-${randomUUID()}`; expect((await s.moveIdentities({ moveId, appSlug: "bifrost", fromAccountId: account.id, toAccountId: target.id, identityIds: "all", covers: "account", proofSessions: [], proof: { from: [], to: null }, at: now })).kind).toBe("moved"); expect((await s.findIdentityMove(moveId))?.id).toBe(moveId); - expect(await s.resolveAccount(account.id)).toBe(target.id); return called; } @@ -712,8 +711,6 @@ async function exerciseMoveContract(store: HeimdallStore): Promise { expect(record!.moved.map((m) => m.credentialStripped)).toEqual([true, true]); expect((await store.findSession("bifrost", a.proofSession.sessionId))!.accessRevision).toBe(2); expect((await store.findSession("bifrost", b.proofSession.sessionId))!.accessRevision).toBe(1); - expect(await store.resolveAccount(a.account.id)).toBe(b.account.id); - expect(await store.resolveAccount(b.account.id)).toBe(b.account.id); // Replay answers from the row, even though the proof session is gone; a different pair is a conflict. expect(await store.moveIdentities(first)).toEqual({ kind: "replayed", move: record }); @@ -751,9 +748,82 @@ async function exerciseMoveContract(store: HeimdallStore): Promise { expect(reversed.kind).toBe("moved"); expect(await held(a)).toEqual(a.rows.map((r) => r.id).sort()); expect(await held(b)).toEqual(b.rows.map((r) => r.id)); - expect(await store.resolveAccount(a.account.id)).toBe(a.account.id); expect((await store.moveIdentities({ ...back, moveId: `move-${randomUUID()}` })).kind).toBe("not_reversible"); expect((await store.moveIdentities(input(a, b, { reversesMoveId: back.moveId, proofSessions: [], identityIds: "all" }))).kind).toBe("not_reversible"); + + // A moveId answers only the move that used it: each of app, source, destination and reversal must match. + const k = await side(["discord"]); + const l = await side(["patreon"]); + const m = await side(["google"]); + const once = input(k, l); + expect((await store.moveIdentities(once)).kind).toBe("moved"); + expect((await store.moveIdentities({ ...once, fromAccountId: m.account.id })).kind).toBe("move_id_conflict"); + expect((await store.moveIdentities({ ...once, toAccountId: m.account.id })).kind).toBe("move_id_conflict"); + expect((await store.moveIdentities({ ...once, appSlug: "streampixels" })).kind).toBe("move_id_conflict"); + expect((await store.moveIdentities(once)).kind).toBe("replayed"); + + // Every step of a move is on Postgres too: expiry of the source's pending links and unredeemed completions, and the stripped credential's expiry and failure mark. + const n = await side(["discord", "google"]); + const o = await side(["patreon"]); + await store.upsertLinkedIdentity({ accountId: n.account.id, provider: "discord", providerUserId: (await store.listStoredLinkedIdentitiesForAccount(n.account.id))[0]!.providerUserId, tokenExpiresAt: "2999-01-01T00:00:00.000Z", scopes: [], profileJson: {}, createdAt: at, updatedAt: at }); + await store.updateLinkedIdentityCredential(n.rows[1]!.id, async () => ({ kind: "failed", failedAt: at })); + const linkOf = (accountId: string) => store.createPendingLink({ appSlug: "bifrost", accountId, provider: "discord", providerUserId: "p", identity: { accountId, provider: "discord", providerUserId: "p", scopes: [], profileJson: {}, createdAt: at, updatedAt: at }, returnTo: "https://x.test/", createdAt: at, expiresAt: later }); + const completionOf = (accountId: string) => store.createAuthCompletion({ appSlug: "bifrost", provider: "discord", mode: "sign_in", accountId, sessionId: n.proofSession.sessionId, returnTo: "https://x.test/", createdAt: at, expiresAt: later, payloadJson: {} }); + const sourceLink = await linkOf(n.account.id); + const destLink = await linkOf(o.account.id); + const sourceCompletion = await completionOf(n.account.id); + const destCompletion = await completionOf(o.account.id); + expect((await store.moveIdentities(input(n, o))).kind).toBe("moved"); + const after = "2026-11-14T12:00:01.000Z"; + expect(await store.consumePendingLink("bifrost", sourceLink.id, n.account.id, after)).toBeNull(); + expect(await store.consumeAuthCompletion("bifrost", sourceCompletion.code, after)).toBeNull(); + expect((await store.consumePendingLink("bifrost", destLink.id, o.account.id, after))?.id).toBe(destLink.id); + expect((await store.consumeAuthCompletion("bifrost", destCompletion.code, after))?.code).toBe(destCompletion.code); + for (const row of (await store.listStoredLinkedIdentitiesForAccount(o.account.id)).filter((r) => n.rows.some((x) => x.id === r.id))) { + expect([row.accessTokenEncrypted, row.refreshTokenEncrypted, row.tokenExpiresAt, row.refreshFailedAt], row.provider).toEqual([undefined, undefined, undefined, undefined]); + } + + // Each reversal guard on its own: the refusal is not_reversible, never another code, and nothing moves. + const fresh = async () => ({ p: await side(["discord", "google"]), q: await side(["patreon"]), t: await side(["patreon"]) }); + const reverse = (forwardId: string, from: { account: { id: string } }, to: { account: { id: string } }, ids: readonly string[]) => ({ + moveId: `move-${randomUUID()}`, appSlug: "bifrost" as const, fromAccountId: from.account.id, toAccountId: to.account.id, + identityIds: ids, covers: ids, proofSessions: [], reversesMoveId: forwardId, proof: { from: [], to: null }, at, + }); + // (a) already reversed, even though the logins are back on the destination by a later forward move + { + const { p, q } = await fresh(); + const ids = p.rows.map((r) => r.id); + const m1 = input(p, q); + expect((await store.moveIdentities(m1)).kind).toBe("moved"); + expect((await store.moveIdentities(reverse(m1.moveId, q, p, ids))).kind).toBe("moved"); + expect((await store.moveIdentities(input(p, q, { proofSessions: [] }))).kind).toBe("moved"); + expect((await store.moveIdentities(reverse(m1.moveId, q, p, ids))).kind, "reversed once").toBe("not_reversible"); + expect(await held(p)).toEqual([]); + } + // (d) the logins are still on the forward's destination but the reversal targets another account + { + const { p, q } = await fresh(); + const ids = p.rows.map((r) => r.id); + const m1 = input(p, q); + expect((await store.moveIdentities(m1)).kind).toBe("moved"); + const elsewhere = await side(["patreon"]); + expect((await store.moveIdentities(reverse(m1.moveId, q, elsewhere, ids))).kind, "to is not the forward's source").toBe("not_reversible"); + expect(await held(elsewhere)).toHaveLength(1); + } + // (b) the logins moved on from the destination: not held, so not_reversible (not identity_not_found) + // (c) the reversal's source is not the forward's destination, or its target is not the forward's source + { + const { p, q, t } = await fresh(); + const ids = p.rows.map((r) => r.id); + const m1 = input(p, q); + expect((await store.moveIdentities(m1)).kind).toBe("moved"); + expect((await store.moveIdentities(input(q, t, { identityIds: ids, covers: ids, proofSessions: [] }))).kind).toBe("moved"); + expect((await store.moveIdentities(reverse(m1.moveId, q, p, ids))).kind, "ids no longer held").toBe("not_reversible"); + expect((await store.moveIdentities(reverse(m1.moveId, t, p, ids))).kind, "from is not the forward's destination").toBe("not_reversible"); + const u = await side(["google"]); + expect((await store.moveIdentities(reverse(m1.moveId, t, u, ids))).kind, "to is not the forward's source").toBe("not_reversible"); + expect(await held(t)).toEqual([...ids, ...t.rows.map((r) => r.id)].sort()); + } } // Against a real Postgres, through the startup path itself: createStore, with @@ -968,6 +1038,26 @@ describe.skipIf(!adminUrl)("startup against a live Postgres (HEIMDALL_TEST_PG_AD } }); + it("one moveId over two different pairs at once: one moved, the other move_id_conflict, never a receipt for a move it did not make", async () => { + const { name, owner } = await appliedDatabase(); + const storeA = new PostgresStore(owner); + const other = fixturePool(urlFor(name)); + cleanup.push(async () => { + await other.end(); + }); + const storeB = new PostgresStore(other); + for (let round = 0; round < 6; round += 1) { + const one = await pair(storeA); + const two = await pair(storeA); + const id = `move-${randomUUID()}`; + const results = await Promise.all([storeA.moveIdentities(request(one.x, one.y, id)), storeB.moveIdentities(request(two.x, two.y, id))]); + expect(results.map((r) => r.kind).sort(), `round ${round}`).toEqual(["move_id_conflict", "moved"]); + const winner = (await storeA.findIdentityMove(id))!; + const moved = results.find((r) => r.kind === "moved") as { move: { fromAccountId: string } }; + expect(winner.fromAccountId).toBe(moved.move.fromAccountId); + } + }); + it("two concurrent identical moveIds: one moved, one replayed", async () => { const { name, owner } = await appliedDatabase(); const storeA = new PostgresStore(owner);