From 1a239e1e9558b491d92fa9d273f65bf0c72a057a Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 21 Sep 2026 15:26:48 -0700 Subject: [PATCH 1/6] feat(accounts): resolve accounts by (issuer, subject) An account is found by how it signed in: a new identity-bindings table maps (issuer, subject) to an account_id. The pair is the table's key, so a subject binds to one account per issuer and nothing more. AccountsTable.fetchByIdentity(issuer, subject) resolves any account type through it. fetchByOryId keeps its signature and becomes the wrapper that supplies this environment's Ory issuer; while the backfill lands it still falls back to the identity_id index for an account with no binding yet, and logs that fallback so the unbacked accounts are countable. create() binds an individual's Ory identity in the same breath, and removes the row again if the identity is already bound elsewhere. The table is declared in CDK, the local-dev script, and the CDK grant list; the local-dev script seeds bindings from the fixture accounts. scripts/backfill-identity-bindings.ts backfills every individual (MODE= backfill, idempotent) and verifies the counts match (MODE=verify). Removing the fallback and the identity_id index follows once verify passes in production. Part of #543 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z --- deploy/lib/api-stack.ts | 1 + deploy/lib/database-construct.ts | 18 ++ scripts/backfill-identity-bindings.ts | 206 ++++++++++++++++++ scripts/init-local.ts | 78 ++++++- src/lib/api/oidc.ts | 5 +- .../clients/database/accounts.create.test.ts | 61 +++++- .../database/accounts.identity.test.ts | 111 ++++++++++ src/lib/clients/database/accounts.ts | 66 +++++- .../database/identity-bindings.test.ts | 58 +++++ src/lib/clients/database/identity-bindings.ts | 72 ++++++ src/lib/clients/database/index.ts | 1 + src/types/identity-binding.ts | 20 ++ src/types/index.ts | 1 + 13 files changed, 684 insertions(+), 14 deletions(-) create mode 100644 scripts/backfill-identity-bindings.ts create mode 100644 src/lib/clients/database/accounts.identity.test.ts create mode 100644 src/lib/clients/database/identity-bindings.test.ts create mode 100644 src/lib/clients/database/identity-bindings.ts create mode 100644 src/types/identity-binding.ts diff --git a/deploy/lib/api-stack.ts b/deploy/lib/api-stack.ts index 414d818d0..bff342ec2 100644 --- a/deploy/lib/api-stack.ts +++ b/deploy/lib/api-stack.ts @@ -55,6 +55,7 @@ export class ApiStack extends cdk.Stack { database.dataConnectionsTable, database.apiKeysTable, database.membershipsTable, + database.identityBindingsTable, ]) { table.grantReadWriteData(vercel.vercelRole); } diff --git a/deploy/lib/database-construct.ts b/deploy/lib/database-construct.ts index 01ad92046..2e88866b7 100644 --- a/deploy/lib/database-construct.ts +++ b/deploy/lib/database-construct.ts @@ -26,6 +26,7 @@ export class DatabaseConstruct extends Construct { public readonly dataConnectionsTable: dynamodb.Table; public readonly apiKeysTable: dynamodb.Table; public readonly membershipsTable: dynamodb.Table; + public readonly identityBindingsTable: dynamodb.Table; constructor( scope: Construct, @@ -60,6 +61,23 @@ export class DatabaseConstruct extends Construct { removalPolicy, }); + this.identityBindingsTable = this.createTable({ + name: "identity-bindings", + stage, + // (issuer, subject) is the key, so a subject binds to one account per + // issuer and nothing more. + partitionKey: "issuer", + sortKey: "subject", + indexes: [ + { + // fetch the bindings of an account + name: "account_id", + partitionKey: "account_id", + }, + ], + removalPolicy, + }); + this.apiKeysTable = this.createTable({ name: "api-keys", stage, diff --git a/scripts/backfill-identity-bindings.ts b/scripts/backfill-identity-bindings.ts new file mode 100644 index 000000000..5651c21a3 --- /dev/null +++ b/scripts/backfill-identity-bindings.ts @@ -0,0 +1,206 @@ +#!/usr/bin/env tsx +/** + * Give every individual account an identity binding — `(issuer, subject) -> + * account_id` in the identity-bindings table — for its Ory identity, so the + * app can resolve accounts through bindings and retire the `identity_id` index. + * + * ZERO-DOWNTIME, TWO-PHASE DESIGN + * ------------------------------- + * The app resolves an Ory identity through its binding first and falls back to + * the `identity_id` index for any account without one, logging the fallback. + * A missed account is a person who cannot sign in once the fallback goes, so + * the fallback stays until this script's verify phase says the counts match. + * + * MODE=backfill (default) — ADDITIVE. Writes one binding per individual + * account under the given issuer. Guarded by `attribute_not_exists(issuer)`, + * so it is idempotent and never clobbers a binding the app already wrote. + * + * MODE=verify — READ-ONLY. Counts individual accounts with an identity_id and + * bindings under the issuer, and exits non-zero if they differ. + * + * Rollout order: deploy the table + dual-read code -> run backfill -> run + * verify -> remove the fallback and the index in a follow-up. + * + * Usage: + * npx tsx scripts/backfill-identity-bindings.ts + * + * Examples: + * npx tsx scripts/backfill-identity-bindings.ts sc-dev-accounts sc-dev-identity-bindings https://auth.dev.source.coop + * MODE=verify npx tsx scripts/backfill-identity-bindings.ts sc-prod-accounts sc-prod-identity-bindings https://auth.source.coop + * + * Environment variables: + * MODE - "backfill" (default, additive) or "verify" (read-only) + * AWS_REGION - AWS region (default: us-east-1) + * AWS_PROFILE - AWS profile to use (optional) + * DRY_RUN - Set (to anything) to preview backfill writes without writing + */ + +import { DynamoDBClient } from "@aws-sdk/client-dynamodb"; +import { + DynamoDBDocumentClient, + PutCommand, + QueryCommand, + ScanCommand, +} from "@aws-sdk/lib-dynamodb"; + +type Mode = "backfill" | "verify"; + +interface AccountItem { + account_id: string; + type: string; + identity_id?: string; + created_at?: string; +} + +function isConditionalCheckFailed(err: unknown): boolean { + return err instanceof Error && err.name === "ConditionalCheckFailedException"; +} + +async function* individuals( + client: DynamoDBDocumentClient, + accountsTable: string +): AsyncGenerator { + let lastEvaluatedKey: Record | undefined; + do { + const result = await client.send( + new ScanCommand({ + TableName: accountsTable, + ProjectionExpression: "account_id, #type, identity_id, created_at", + ExpressionAttributeNames: { "#type": "type" }, + ExclusiveStartKey: lastEvaluatedKey, + }) + ); + lastEvaluatedKey = result.LastEvaluatedKey; + for (const item of (result.Items || []) as AccountItem[]) { + if (item.type === "individual" && item.identity_id) yield item; + } + } while (lastEvaluatedKey); +} + +async function countBindings( + client: DynamoDBDocumentClient, + bindingsTable: string, + issuer: string +): Promise { + let count = 0; + let lastEvaluatedKey: Record | undefined; + do { + const result = await client.send( + new QueryCommand({ + TableName: bindingsTable, + KeyConditionExpression: "issuer = :issuer", + ExpressionAttributeValues: { ":issuer": issuer }, + Select: "COUNT", + ExclusiveStartKey: lastEvaluatedKey, + }) + ); + count += result.Count ?? 0; + lastEvaluatedKey = result.LastEvaluatedKey; + } while (lastEvaluatedKey); + return count; +} + +async function run( + accountsTable: string, + bindingsTable: string, + issuer: string, + mode: Mode, + dryRun: boolean +) { + const region = process.env.AWS_REGION || "us-east-1"; + console.log(`Mode: ${mode}`); + console.log(`Accounts: ${accountsTable}`); + console.log(`Bindings: ${bindingsTable}`); + console.log(`Issuer: ${issuer}`); + console.log(`Region: ${region}`); + console.log(`Dry run: ${dryRun}`); + console.log(""); + + const client = DynamoDBDocumentClient.from(new DynamoDBClient({ region }), { + marshallOptions: { removeUndefinedValues: true }, + }); + + if (mode === "verify") { + let accounts = 0; + for await (const _ of individuals(client, accountsTable)) accounts++; + const bindings = await countBindings(client, bindingsTable, issuer); + console.log(`Individual accounts with an identity: ${accounts}`); + console.log(`Bindings under the issuer: ${bindings}`); + if (accounts !== bindings) { + console.error("MISMATCH — keep the identity_id fallback in place."); + process.exit(1); + } + console.log("Counts match. The fallback can go."); + return; + } + + let scanned = 0; + let written = 0; + let guarded = 0; + let errors = 0; + for await (const account of individuals(client, accountsTable)) { + scanned++; + const binding = { + issuer, + subject: account.identity_id, + account_id: account.account_id, + created_at: account.created_at ?? new Date().toISOString(), + }; + if (dryRun) { + console.log(`[DRY RUN] Would bind ${account.identity_id} -> ${account.account_id}`); + written++; + continue; + } + try { + await client.send( + new PutCommand({ + TableName: bindingsTable, + Item: binding, + ConditionExpression: "attribute_not_exists(issuer)", + }) + ); + written++; + } catch (err) { + if (isConditionalCheckFailed(err)) { + // Already bound — by an earlier run or by the app at signup. + guarded++; + continue; + } + errors++; + console.error(`Error binding ${account.account_id}:`, err); + } + } + + console.log(""); + console.log("backfill complete."); + console.log(` Individuals scanned: ${scanned}`); + console.log(` Bindings written: ${written}`); + console.log(` Already bound: ${guarded}`); + console.log(` Errors: ${errors}`); +} + +function usage() { + console.error( + "Usage: [MODE=backfill|verify] npx tsx scripts/backfill-identity-bindings.ts " + ); +} + +const [accountsTable, bindingsTable, issuer] = process.argv.slice(2); +if (!accountsTable || !bindingsTable || !issuer) { + usage(); + process.exit(1); +} + +const mode = (process.env.MODE || "backfill") as Mode; +if (mode !== "backfill" && mode !== "verify") { + console.error(`Invalid MODE: ${mode} (expected "backfill" or "verify")`); + usage(); + process.exit(1); +} + +run(accountsTable, bindingsTable, issuer, mode, process.env.DRY_RUN !== undefined).catch( + (err) => { + console.error(`${mode} failed:`, err); + process.exit(1); + } +); diff --git a/scripts/init-local.ts b/scripts/init-local.ts index d078945f2..f3d8fd5a3 100644 --- a/scripts/init-local.ts +++ b/scripts/init-local.ts @@ -15,6 +15,7 @@ import { // Import mock data import { accounts, apiKeys, memberships, products, dataConnections } from "../src/lib/api/utils.mock"; +import { AccountType } from "../src/types"; const DYNAMODB_ENDPOINT = process.env.DYNAMODB_ENDPOINT || "http://localhost:8000"; @@ -49,7 +50,8 @@ async function tablesExist() { tables.TableNames?.includes(getTableName("products")) && tables.TableNames?.includes(getTableName("api-keys")) && tables.TableNames?.includes(getTableName("data-connections")) && - tables.TableNames?.includes(getTableName("memberships")) + tables.TableNames?.includes(getTableName("memberships")) && + tables.TableNames?.includes(getTableName("identity-bindings")) ); } @@ -77,6 +79,7 @@ async function createTables() { await deleteTable(getTableName("api-keys")); await deleteTable(getTableName("data-connections")); await deleteTable(getTableName("memberships")); + await deleteTable(getTableName("identity-bindings")); // Wait for tables to be fully deleted await new Promise((resolve) => setTimeout(resolve, 2000)); @@ -143,6 +146,45 @@ async function createTables() { throw e; } + // Create identity-bindings table + try { + await client.send( + new CreateTableCommand({ + TableName: getTableName("identity-bindings"), + AttributeDefinitions: [ + { AttributeName: "issuer", AttributeType: "S" }, + { AttributeName: "subject", AttributeType: "S" }, + { AttributeName: "account_id", AttributeType: "S" }, + ], + KeySchema: [ + { AttributeName: "issuer", KeyType: "HASH" }, + { AttributeName: "subject", KeyType: "RANGE" }, + ], + GlobalSecondaryIndexes: [ + { + IndexName: "account_id", + KeySchema: [{ AttributeName: "account_id", KeyType: "HASH" }], + Projection: { + ProjectionType: "ALL", + }, + ProvisionedThroughput: { + ReadCapacityUnits: 5, + WriteCapacityUnits: 5, + }, + }, + ], + ProvisionedThroughput: { + ReadCapacityUnits: 5, + WriteCapacityUnits: 5, + }, + }) + ); + console.log(`✓ Created ${getTableName("identity-bindings")} table`); + } catch (e) { + console.error(`✗ Error creating ${getTableName("identity-bindings")} table:`, e); + throw e; + } + // Create api-keys table try { await client.send( @@ -356,6 +398,40 @@ async function loadFixtureData() { `Account insertion complete! Processed ${processedAccounts} accounts.` ); + // Every individual is reachable by its Ory identity under this + // environment's Ory issuer — the same binding the app writes at signup. + const issuer = process.env.NEXT_PUBLIC_ORY_SDK_URL || "http://localhost:4000"; + const bindings = accounts.flatMap((account) => + account.type === AccountType.INDIVIDUAL + ? [ + { + issuer, + subject: account.identity_id, + account_id: account.account_id, + created_at: account.created_at, + }, + ] + : [] + ); + console.log( + `Inserting ${bindings.length} identity bindings into ${getTableName( + "identity-bindings" + )} table...` + ); + for (const binding of bindings) { + try { + await docClient.send( + new PutCommand({ + TableName: getTableName("identity-bindings"), + Item: binding, + }) + ); + } catch (error) { + console.error(`Error inserting binding for ${binding.account_id}:`, error); + } + } + console.log(`Identity binding insertion complete!`); + // Insert API keys into DynamoDB console.log( `Inserting ${apiKeys.length} API keys into ${getTableName( diff --git a/src/lib/api/oidc.ts b/src/lib/api/oidc.ts index cbe8650ea..26b1d911c 100644 --- a/src/lib/api/oidc.ts +++ b/src/lib/api/oidc.ts @@ -147,9 +147,8 @@ export async function authenticateWithOidcToken( // The token subject is the caller's Ory identity id (the data proxy signs // tokens with the authenticated principal's Ory id as `sub`), so resolve the - // account via the identity_id index — NOT fetchById, which keys on the - // human-readable account_id. Only individual accounts have an Ory identity; - // org accounts are never the subject of a proxy-issued token. + // account through its identity binding under the Ory issuer — NOT fetchById, + // which keys on the human-readable account_id. const account = await accountsTable.fetchByOryId(oryId); if (!account) { // Verified token, but no account is indexed under this Ory id. This is the diff --git a/src/lib/clients/database/accounts.create.test.ts b/src/lib/clients/database/accounts.create.test.ts index e3de028c0..3134c6b11 100644 --- a/src/lib/clients/database/accounts.create.test.ts +++ b/src/lib/clients/database/accounts.create.test.ts @@ -1,4 +1,7 @@ import { AccountsTable } from "./accounts"; +import { IdentityBindingsTable } from "./identity-bindings"; +import { createMemoizedRead } from "./request-cache"; +import { fakeReactCache } from "./__test-helpers__/fake-react-cache"; import { DynamoDBDocumentClient } from "@aws-sdk/lib-dynamodb"; import { AccountType, type Account } from "@/types"; @@ -6,6 +9,7 @@ jest.mock("@/lib/config", () => ({ CONFIG: { environment: { stage: "test" }, database: {}, + auth: { api: { backendUrl: "http://ory.test" } }, }, })); @@ -54,7 +58,11 @@ function fakeDynamo(seed: Account[] = []) { const send = jest.fn(async (command: { input: Record }) => { const item = command.input.Item as Account | undefined; - if (!item) return {}; + if (!item) { + const key = command.input.Key as { account_id: string } | undefined; + if (key) store.delete(key.account_id); + return {}; + } const condition = command.input.ConditionExpression as string | undefined; const guardsExistence = @@ -79,12 +87,38 @@ function fakeDynamo(seed: Account[] = []) { }; } +/** + * A bindings table whose store keys on the (issuer, subject) pair and honours + * `attribute_not_exists(issuer)` the way the real table does. + */ +function fakeBindings(seed: Array<{ issuer: string; subject: string }> = []) { + const store = new Set(seed.map((b) => `${b.issuer}\u0000${b.subject}`)); + const send = jest.fn(async (command: { input: Record }) => { + const item = command.input.Item as { issuer: string; subject: string }; + const key = `${item.issuer}\u0000${item.subject}`; + if (store.has(key)) { + const error = new Error("The conditional request failed") as Error & { + name: string; + }; + error.name = "ConditionalCheckFailedException"; + throw error; + } + store.add(key); + return {}; + }); + const table = new IdentityBindingsTable({ + client: { send } as unknown as DynamoDBDocumentClient, + memoizedRead: createMemoizedRead(fakeReactCache), + }); + return { store, table }; +} + describe("AccountsTable.create", () => { it("refuses to overwrite an account that already exists", async () => { // The victim already holds `victim`, bound to their Ory identity. const victim = makeAccount(); const { store, client } = fakeDynamo([victim]); - const table = new AccountsTable({ client }); + const table = new AccountsTable({ client, bindings: fakeBindings().table }); // An attacker who has registered an Ory identity but not yet created an // account submits the create form naming the victim's account_id. This is @@ -109,7 +143,8 @@ describe("AccountsTable.create", () => { it("still creates an account when the id is unused", async () => { const { store, client } = fakeDynamo([makeAccount()]); - const table = new AccountsTable({ client }); + const bindings = fakeBindings(); + const table = new AccountsTable({ client, bindings: bindings.table }); const newcomer = makeAccount({ account_id: "newcomer", @@ -120,5 +155,25 @@ describe("AccountsTable.create", () => { await expect(table.create(newcomer)).resolves.toEqual(newcomer); expect(store.get("newcomer")?.identity_id).toBe("ory-identity-newcomer"); expect(store.size).toBe(2); + // ...and binds the Ory identity under this environment's issuer. + expect(bindings.store.has("http://ory.test\u0000ory-identity-newcomer")).toBe(true); + }); + + it("does not leave an account behind when its identity is already bound", async () => { + // The identity already resolves to some other account. A second account for + // it must not be created half-way: the row is written, the binding fails, + // and the row is removed again. + const { store, client } = fakeDynamo([makeAccount()]); + const bindings = fakeBindings([ + { issuer: "http://ory.test", subject: "ory-identity-victim" }, + ]); + const table = new AccountsTable({ client, bindings: bindings.table }); + + const second = makeAccount({ account_id: "second", name: "Second" }); + + await expect(table.create(second)).rejects.toThrow( + expect.objectContaining({ name: "ConditionalCheckFailedException" }) + ); + expect(store.has("second")).toBe(false); }); }); diff --git a/src/lib/clients/database/accounts.identity.test.ts b/src/lib/clients/database/accounts.identity.test.ts new file mode 100644 index 000000000..904805351 --- /dev/null +++ b/src/lib/clients/database/accounts.identity.test.ts @@ -0,0 +1,111 @@ +import { type DynamoDBDocumentClient } from "@aws-sdk/lib-dynamodb"; +import { AccountsTable } from "./accounts"; +import { IdentityBindingsTable } from "./identity-bindings"; +import { createMemoizedRead } from "./request-cache"; +import { fakeReactCache } from "./__test-helpers__/fake-react-cache"; +import { LOGGER } from "@/lib/logging"; +import { AccountType, type Account, type IdentityBinding } from "@/types"; + +jest.mock("@/lib/config", () => ({ + CONFIG: { + environment: { stage: "test" }, + database: {}, + auth: { api: { backendUrl: "http://ory.test" } }, + }, +})); +jest.mock("@/lib/logging", () => ({ + LOGGER: { error: jest.fn(), warn: jest.fn(), debug: jest.fn(), info: jest.fn() }, +})); + +const account = (fields: Partial): Account => + ({ type: AccountType.INDIVIDUAL, disabled: false, ...fields }) as Account; + +const jane = account({ account_id: "jane-doe", identity_id: "ory-jane" }); +const acme = account({ account_id: "acme", type: AccountType.ORGANIZATION }); +const unbacked = account({ account_id: "old-timer", identity_id: "ory-old" }); + +/** Answers the key query and the identity_id index query from `items`. */ +function fakeAccounts(items: Account[]) { + const send = jest.fn(async (command: { input: Record }) => { + const values = command.input.ExpressionAttributeValues as Record; + if (values[":account_id"] !== undefined) { + return { Items: items.filter((a) => a.account_id === values[":account_id"]) }; + } + return { Items: items.filter((a) => a.identity_id === values[":identity_id"]) }; + }); + return { send, client: { send } as unknown as DynamoDBDocumentClient }; +} + +function fakeBindings(bindings: IdentityBinding[]) { + const send = jest.fn(async (command: { input: Record }) => { + const key = command.input.Key as { issuer: string; subject: string }; + return { + Item: bindings.find((b) => b.issuer === key.issuer && b.subject === key.subject), + }; + }); + return new IdentityBindingsTable({ + client: { send } as unknown as DynamoDBDocumentClient, + memoizedRead: createMemoizedRead(fakeReactCache), + }); +} + +function tableFor(items: Account[], bindings: IdentityBinding[]) { + const accounts = fakeAccounts(items); + const table = new AccountsTable({ + client: accounts.client, + memoizedRead: createMemoizedRead(fakeReactCache), + bindings: fakeBindings(bindings), + }); + return { table, send: accounts.send }; +} + +const bound = (subject: string, account_id: string, issuer = "http://ory.test") => + ({ issuer, subject, account_id, created_at: "2024-01-01T00:00:00Z" }) as IdentityBinding; + +describe("AccountsTable identity resolution", () => { + beforeEach(() => jest.clearAllMocks()); + + it("resolves an Ory identity through its binding, without touching the index", async () => { + const { table, send } = tableFor([jane, unbacked], [bound("ory-jane", "jane-doe")]); + + expect(await table.fetchByOryId("ory-jane")).toEqual(jane); + expect(send.mock.calls.map((c) => c[0].input.IndexName)).toEqual([undefined]); + expect(LOGGER.warn).not.toHaveBeenCalled(); + }); + + it("falls back to the identity_id index for an account with no binding yet, and says so", async () => { + const { table } = tableFor([jane, unbacked], [bound("ory-jane", "jane-doe")]); + + expect(await table.fetchByOryId("ory-old")).toEqual(unbacked); + expect(LOGGER.warn).toHaveBeenCalledWith( + expect.stringContaining("identity_id index"), + expect.objectContaining({ metadata: { identity_id: "ory-old", account_id: "old-timer" } }) + ); + }); + + it("returns nothing for an unknown identity", async () => { + const { table } = tableFor([jane], [bound("ory-jane", "jane-doe")]); + expect(await table.fetchByOryId("ory-nobody")).toBeNull(); + }); + + it("keeps fetchByOryId to individuals, while fetchByIdentity resolves any account type", async () => { + const bindings = [bound("ci-subject", "acme", "https://ci.example")]; + const { table } = tableFor([acme], bindings); + + expect(await table.fetchByIdentity("https://ci.example", "ci-subject")).toEqual(acme); + // The same pair under the Ory issuer is not bound... + expect(await table.fetchByOryId("ci-subject")).toBeNull(); + // ...and a non-individual bound under Ory is not an Ory account either. + const { table: odd } = tableFor([acme], [bound("ory-acme", "acme")]); + expect(await odd.fetchByOryId("ory-acme")).toBeNull(); + }); + + it("tells issuers apart", async () => { + const { table } = tableFor( + [jane, acme], + [bound("shared", "jane-doe"), bound("shared", "acme", "https://ci.example")] + ); + expect(await table.fetchByIdentity("http://ory.test", "shared")).toEqual(jane); + expect(await table.fetchByIdentity("https://ci.example", "shared")).toEqual(acme); + }); +}); diff --git a/src/lib/clients/database/accounts.ts b/src/lib/clients/database/accounts.ts index b847171b6..ee2a07316 100644 --- a/src/lib/clients/database/accounts.ts +++ b/src/lib/clients/database/accounts.ts @@ -17,6 +17,11 @@ import { } from "@/types"; import { BaseTable } from "./base"; +import { + IdentityBindingsTable, + identityBindingsTable, + oryIssuer, +} from "./identity-bindings"; import { LOGGER } from "@/lib/logging"; /** @@ -31,6 +36,17 @@ export interface AccountSuggestion { export class AccountsTable extends BaseTable { model = "accounts"; + private readonly bindings: IdentityBindingsTable; + + constructor({ + bindings, + ...base + }: { + bindings?: IdentityBindingsTable; + } & ConstructorParameters[0] = {}) { + super(base); + this.bindings = bindings ?? identityBindingsTable; + } async fetchById(account_id: string): Promise { try { @@ -105,13 +121,27 @@ export class AccountsTable extends BaseTable { return accountBatches; } + /** The account `subject` resolves to, as named by `issuer`. Any type. */ + async fetchByIdentity( + issuer: string, + subject: string + ): Promise { + const binding = await this.bindings.resolve(issuer, subject); + return binding ? this.fetchById(binding.account_id) : null; + } + + /** + * The individual an Ory identity resolves to. Ory identities are bound under + * this environment's Ory issuer. While the backfill lands (#543), an account + * with no binding yet is still found through the `identity_id` index, and + * that fallback is logged so the unbacked accounts are countable. Remove the + * fallback, and the index with it, once the backfill is verified. + */ async fetchByOryId(identity_id: string): Promise { + const bound = await this.fetchByIdentity(oryIssuer(), identity_id); + if (bound) return isIndividualAccount(bound) ? bound : null; + try { - LOGGER.debug(`Trying to fetch account by Ory ID`, { - operation: "AccountsTable.fetchByOryId", - context: "database operation", - metadata: { identity_id }, - }); const result = await this.cachedSend( new QueryCommand({ TableName: this.table, @@ -129,10 +159,10 @@ export class AccountsTable extends BaseTable { if (!account) return null; - LOGGER.debug(`Found account by Ory ID`, { + LOGGER.warn("Account resolved through the identity_id index, not a binding", { operation: "AccountsTable.fetchByOryId", context: "database operation", - metadata: { identity_id, account }, + metadata: { identity_id, account_id: account.account_id }, }); return account as IndividualAccount; } catch (error) { @@ -228,6 +258,28 @@ export class AccountsTable extends BaseTable { throw error; } + // An individual is reachable by its Ory identity, so it is bound in the + // same breath. If that identity is already bound elsewhere, the account + // row must not survive without a binding of its own. + if (isIndividualAccount(account)) { + try { + await this.bindings.create({ + issuer: oryIssuer(), + subject: account.identity_id, + account_id: account.account_id, + created_at: account.created_at, + }); + } catch (error) { + await this.client.send( + new DeleteCommand({ + TableName: this.table, + Key: { account_id: account.account_id }, + }) + ); + throw error; + } + } + return account; } diff --git a/src/lib/clients/database/identity-bindings.test.ts b/src/lib/clients/database/identity-bindings.test.ts new file mode 100644 index 000000000..0e4f6815f --- /dev/null +++ b/src/lib/clients/database/identity-bindings.test.ts @@ -0,0 +1,58 @@ +import { type DynamoDBDocumentClient } from "@aws-sdk/lib-dynamodb"; +import { ResourceNotFoundException } from "@aws-sdk/client-dynamodb"; +import { IdentityBindingsTable } from "./identity-bindings"; +import { createMemoizedRead } from "./request-cache"; +import { fakeReactCache } from "./__test-helpers__/fake-react-cache"; + +jest.mock("@/lib/config", () => ({ + CONFIG: { environment: { stage: "test" }, database: {} }, +})); +jest.mock("@/lib/logging", () => ({ + LOGGER: { error: jest.fn(), warn: jest.fn(), debug: jest.fn(), info: jest.fn() }, +})); + +function tableWith(send: jest.Mock) { + return new IdentityBindingsTable({ + client: { send } as unknown as DynamoDBDocumentClient, + memoizedRead: createMemoizedRead(fakeReactCache), + }); +} + +const binding = { + issuer: "https://ci.example", + subject: "repo:o/r:ref:refs/heads/main", + account_id: "o-bot", + created_at: "2024-01-01T00:00:00Z", +}; + +describe("IdentityBindingsTable", () => { + it("writes a binding only if the pair is not already bound", async () => { + const send = jest.fn(async () => ({})); + await tableWith(send).create(binding); + + const input = send.mock.calls[0][0].input; + expect(input.TableName).toBe("sc-test-identity-bindings"); + expect(input.Item).toEqual(binding); + expect(input.ConditionExpression).toBe("attribute_not_exists(issuer)"); + }); + + it("resolves by the (issuer, subject) key", async () => { + const send = jest.fn(async () => ({ Item: binding })); + const table = tableWith(send); + + expect(await table.resolve(binding.issuer, binding.subject)).toEqual(binding); + expect(send.mock.calls[0][0].input.Key).toEqual({ + issuer: binding.issuer, + subject: binding.subject, + }); + expect(await tableWith(jest.fn(async () => ({}))).resolve("x", "y")).toBeNull(); + }); + + it("treats a table that does not exist yet as holding no bindings", async () => { + // The app may deploy before the table does; nothing is bound until then. + const send = jest.fn(async () => { + throw new ResourceNotFoundException({ message: "no table", $metadata: {} }); + }); + expect(await tableWith(send).resolve("x", "y")).toBeNull(); + }); +}); diff --git a/src/lib/clients/database/identity-bindings.ts b/src/lib/clients/database/identity-bindings.ts new file mode 100644 index 000000000..95fcd702a --- /dev/null +++ b/src/lib/clients/database/identity-bindings.ts @@ -0,0 +1,72 @@ +import { + DeleteCommand, + GetCommand, + PutCommand, + QueryCommand, +} from "@aws-sdk/lib-dynamodb"; +import { ResourceNotFoundException } from "@aws-sdk/client-dynamodb"; +import { CONFIG } from "@/lib/config"; +import type { IdentityBinding } from "@/types"; +import { BaseTable } from "./base"; + +/** The issuer every Ory identity is bound under: this environment's Ory project. */ +export function oryIssuer(): string { + return CONFIG.auth.api.backendUrl ?? ""; +} + +export class IdentityBindingsTable extends BaseTable { + model = "identity-bindings"; + + /** The binding for `subject` as named by `issuer`, or null. */ + async resolve( + issuer: string, + subject: string + ): Promise { + try { + const result = await this.cachedSend( + new GetCommand({ TableName: this.table, Key: { issuer, subject } }) + ); + return (result.Item as IdentityBinding | undefined) ?? null; + } catch (error) { + // The app may deploy before the table does; until then nothing is bound. + if (error instanceof ResourceNotFoundException) return null; + this.logError("resolve", error, { issuer, subject }); + throw error; + } + } + + async listByAccount(account_id: string): Promise { + const result = await this.cachedSend( + new QueryCommand({ + TableName: this.table, + IndexName: "account_id", + KeyConditionExpression: "account_id = :account_id", + ExpressionAttributeValues: { ":account_id": account_id }, + }) + ); + return (result.Items ?? []) as IdentityBinding[]; + } + + /** + * Binds the pair, or throws `ConditionalCheckFailedException` if it is + * already bound — to this account or any other. + */ + async create(binding: IdentityBinding): Promise { + await this.client.send( + new PutCommand({ + TableName: this.table, + Item: binding, + ConditionExpression: "attribute_not_exists(issuer)", + }) + ); + return binding; + } + + async delete(issuer: string, subject: string): Promise { + await this.client.send( + new DeleteCommand({ TableName: this.table, Key: { issuer, subject } }) + ); + } +} + +export const identityBindingsTable = new IdentityBindingsTable({}); diff --git a/src/lib/clients/database/index.ts b/src/lib/clients/database/index.ts index 13c619e5c..7571ec44f 100644 --- a/src/lib/clients/database/index.ts +++ b/src/lib/clients/database/index.ts @@ -1,5 +1,6 @@ export * from "./accounts"; export * from "./api-keys"; export * from "./data-connections"; +export * from "./identity-bindings"; export * from "./memberships"; export * from "./products"; diff --git a/src/types/identity-binding.ts b/src/types/identity-binding.ts new file mode 100644 index 000000000..68ffa612e --- /dev/null +++ b/src/types/identity-binding.ts @@ -0,0 +1,20 @@ +import { extendZodWithOpenApi } from "@asteasolutions/zod-to-openapi"; +import { z } from "zod"; + +extendZodWithOpenApi(z); + +/** + * One way an account may authenticate: a subject as named by an issuer, and + * the account it resolves to. The pair is the key, so a subject binds to one + * account per issuer and nothing more. + */ +export const IdentityBindingSchema = z + .object({ + issuer: z.string().min(1).openapi({ example: "https://auth.source.coop" }), + subject: z.string().min(1).openapi({ example: "identity-id" }), + account_id: z.string().openapi({ example: "account-id" }), + created_at: z.string().datetime(), + }) + .openapi("IdentityBinding"); + +export type IdentityBinding = z.infer; diff --git a/src/types/index.ts b/src/types/index.ts index e372fd7d8..a4142c69b 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -1,5 +1,6 @@ export * from "./shared"; export * from "./account"; +export * from "./identity-binding"; export * from "./api-key"; export * from "./data-connection"; export * from "./membership"; From d7ff7c2a3bda5468c62c8ad5f6ced95c93c60af3 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 21 Sep 2026 15:32:13 -0700 Subject: [PATCH 2/6] fix(accounts): remove an account's identity bindings when it is deleted An orphaned (issuer, subject) row would keep that identity from ever binding to an account again. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z --- .../database/accounts.identity.test.ts | 27 +++++++++++++++---- src/lib/clients/database/accounts.ts | 4 +++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/lib/clients/database/accounts.identity.test.ts b/src/lib/clients/database/accounts.identity.test.ts index 904805351..75fe2cd03 100644 --- a/src/lib/clients/database/accounts.identity.test.ts +++ b/src/lib/clients/database/accounts.identity.test.ts @@ -27,7 +27,8 @@ const unbacked = account({ account_id: "old-timer", identity_id: "ory-old" }); /** Answers the key query and the identity_id index query from `items`. */ function fakeAccounts(items: Account[]) { const send = jest.fn(async (command: { input: Record }) => { - const values = command.input.ExpressionAttributeValues as Record; + const values = command.input.ExpressionAttributeValues as Record | undefined; + if (!values) return {}; // a Delete of the row itself if (values[":account_id"] !== undefined) { return { Items: items.filter((a) => a.account_id === values[":account_id"]) }; } @@ -36,12 +37,18 @@ function fakeAccounts(items: Account[]) { return { send, client: { send } as unknown as DynamoDBDocumentClient }; } +/** Answers Get by key, Query on the account_id index, and Delete, over `bindings`. */ function fakeBindings(bindings: IdentityBinding[]) { const send = jest.fn(async (command: { input: Record }) => { - const key = command.input.Key as { issuer: string; subject: string }; - return { - Item: bindings.find((b) => b.issuer === key.issuer && b.subject === key.subject), - }; + const key = command.input.Key as { issuer: string; subject: string } | undefined; + const at = (b: IdentityBinding) => b.issuer === key?.issuer && b.subject === key?.subject; + if (key && command.constructor.name === "DeleteCommand") { + bindings.splice(bindings.findIndex(at), 1); + return {}; + } + if (key) return { Item: bindings.find(at) }; + const values = command.input.ExpressionAttributeValues as Record; + return { Items: bindings.filter((b) => b.account_id === values[":account_id"]) }; }); return new IdentityBindingsTable({ client: { send } as unknown as DynamoDBDocumentClient, @@ -100,6 +107,16 @@ describe("AccountsTable identity resolution", () => { expect(await odd.fetchByOryId("ory-acme")).toBeNull(); }); + it("removes an account's bindings when the account is deleted", async () => { + // Otherwise the orphaned pair would keep that identity from ever binding again. + const bindings = [bound("ory-jane", "jane-doe"), bound("ci", "jane-doe", "https://ci.example")]; + const { table } = tableFor([jane], bindings); + + await table.delete({ account_id: "jane-doe", type: AccountType.INDIVIDUAL }); + + expect(bindings).toEqual([]); + }); + it("tells issuers apart", async () => { const { table } = tableFor( [jane, acme], diff --git a/src/lib/clients/database/accounts.ts b/src/lib/clients/database/accounts.ts index ee2a07316..815c0d3fc 100644 --- a/src/lib/clients/database/accounts.ts +++ b/src/lib/clients/database/accounts.ts @@ -362,6 +362,10 @@ export class AccountsTable extends BaseTable { } async delete(Key: { account_id: string; type: AccountType }): Promise { + // An orphaned binding would keep the identity from ever binding again. + for (const binding of await this.bindings.listByAccount(Key.account_id)) { + await this.bindings.delete(binding.issuer, binding.subject); + } await this.client.send( new DeleteCommand({ TableName: this.table, From 5c2296737374e972c37c5fe9be84d7c4245ec789 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Mon, 21 Sep 2026 15:42:37 -0700 Subject: [PATCH 3/6] fix(accounts): name a bound-identity conflict, and tolerate a missing bindings table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IdentityBindingsTable.create converts the conditional-write failure into IdentityAlreadyBoundError, so createAccount can tell "this sign-in already has an account" apart from "that account id is taken" — the two conflicts arrived under the same exception name. And it treats a table that does not exist yet as "not written": the identity_id fallback still resolves the account and the backfill writes the binding later, so the app and the CDK change deploy in either order for writes as well as reads. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z --- src/lib/actions/account.ts | 13 ++++++ .../clients/database/accounts.create.test.ts | 21 ++++++++- src/lib/clients/database/accounts.ts | 3 +- .../database/identity-bindings.test.ts | 11 ++++- src/lib/clients/database/identity-bindings.ts | 44 +++++++++++++++---- 5 files changed, 80 insertions(+), 12 deletions(-) diff --git a/src/lib/actions/account.ts b/src/lib/actions/account.ts index 362856a9d..a6d219ca3 100644 --- a/src/lib/actions/account.ts +++ b/src/lib/actions/account.ts @@ -131,6 +131,19 @@ export async function createAccount( try { account = await accountsTable.create(newAccount); } catch (error) { + if ((error as { name?: string })?.name === "IdentityAlreadyBoundError") { + LOGGER.warn("Account creation rejected: identity already has an account", { + operation: "createAccount", + context: "account creation", + metadata: { identity_id: session.identity_id }, + }); + return { + fieldErrors: {}, + data: formData, + message: "This sign-in already has an account", + success: false, + }; + } if ((error as { name?: string })?.name === "ConditionalCheckFailedException") { LOGGER.warn("Account creation rejected: account_id already taken", { operation: "createAccount", diff --git a/src/lib/clients/database/accounts.create.test.ts b/src/lib/clients/database/accounts.create.test.ts index 3134c6b11..da6c9b80a 100644 --- a/src/lib/clients/database/accounts.create.test.ts +++ b/src/lib/clients/database/accounts.create.test.ts @@ -3,6 +3,7 @@ import { IdentityBindingsTable } from "./identity-bindings"; import { createMemoizedRead } from "./request-cache"; import { fakeReactCache } from "./__test-helpers__/fake-react-cache"; import { DynamoDBDocumentClient } from "@aws-sdk/lib-dynamodb"; +import { ResourceNotFoundException } from "@aws-sdk/client-dynamodb"; import { AccountType, type Account } from "@/types"; jest.mock("@/lib/config", () => ({ @@ -171,9 +172,27 @@ describe("AccountsTable.create", () => { const second = makeAccount({ account_id: "second", name: "Second" }); + // ...and the caller learns which conflict this was, not "account_id taken". await expect(table.create(second)).rejects.toThrow( - expect.objectContaining({ name: "ConditionalCheckFailedException" }) + expect.objectContaining({ name: "IdentityAlreadyBoundError" }) ); expect(store.has("second")).toBe(false); }); + + it("still creates the account when the bindings table does not exist yet", async () => { + // The app may deploy before the table does; signups must not fail in the + // window. The identity_id fallback resolves the account until the backfill. + const { store, client } = fakeDynamo(); + const send = jest.fn(async () => { + throw new ResourceNotFoundException({ message: "no table", $metadata: {} }); + }); + const bindings = new IdentityBindingsTable({ + client: { send } as unknown as DynamoDBDocumentClient, + memoizedRead: createMemoizedRead(fakeReactCache), + }); + const table = new AccountsTable({ client, bindings }); + + await expect(table.create(makeAccount())).resolves.toBeDefined(); + expect(store.has("victim")).toBe(true); + }); }); diff --git a/src/lib/clients/database/accounts.ts b/src/lib/clients/database/accounts.ts index 815c0d3fc..12bb45cd5 100644 --- a/src/lib/clients/database/accounts.ts +++ b/src/lib/clients/database/accounts.ts @@ -260,7 +260,8 @@ export class AccountsTable extends BaseTable { // An individual is reachable by its Ory identity, so it is bound in the // same breath. If that identity is already bound elsewhere, the account - // row must not survive without a binding of its own. + // row must not survive without a binding of its own; the + // IdentityAlreadyBoundError propagates for the caller to name. if (isIndividualAccount(account)) { try { await this.bindings.create({ diff --git a/src/lib/clients/database/identity-bindings.test.ts b/src/lib/clients/database/identity-bindings.test.ts index 0e4f6815f..f19e83a62 100644 --- a/src/lib/clients/database/identity-bindings.test.ts +++ b/src/lib/clients/database/identity-bindings.test.ts @@ -1,6 +1,6 @@ import { type DynamoDBDocumentClient } from "@aws-sdk/lib-dynamodb"; import { ResourceNotFoundException } from "@aws-sdk/client-dynamodb"; -import { IdentityBindingsTable } from "./identity-bindings"; +import { IdentityAlreadyBoundError, IdentityBindingsTable } from "./identity-bindings"; import { createMemoizedRead } from "./request-cache"; import { fakeReactCache } from "./__test-helpers__/fake-react-cache"; @@ -36,6 +36,15 @@ describe("IdentityBindingsTable", () => { expect(input.ConditionExpression).toBe("attribute_not_exists(issuer)"); }); + it("names a conflict on the pair for what it is", async () => { + const send = jest.fn(async () => { + const error = new Error("The conditional request failed") as Error & { name: string }; + error.name = "ConditionalCheckFailedException"; + throw error; + }); + await expect(tableWith(send).create(binding)).rejects.toBeInstanceOf(IdentityAlreadyBoundError); + }); + it("resolves by the (issuer, subject) key", async () => { const send = jest.fn(async () => ({ Item: binding })); const table = tableWith(send); diff --git a/src/lib/clients/database/identity-bindings.ts b/src/lib/clients/database/identity-bindings.ts index 95fcd702a..56e7b30d6 100644 --- a/src/lib/clients/database/identity-bindings.ts +++ b/src/lib/clients/database/identity-bindings.ts @@ -6,9 +6,18 @@ import { } from "@aws-sdk/lib-dynamodb"; import { ResourceNotFoundException } from "@aws-sdk/client-dynamodb"; import { CONFIG } from "@/lib/config"; +import { LOGGER } from "@/lib/logging"; import type { IdentityBinding } from "@/types"; import { BaseTable } from "./base"; +/** The (issuer, subject) pair already resolves to an account. */ +export class IdentityAlreadyBoundError extends Error { + constructor(issuer: string, subject: string) { + super(`Identity already bound: ${issuer} ${subject}`); + this.name = "IdentityAlreadyBoundError"; + } +} + /** The issuer every Ory identity is bound under: this environment's Ory project. */ export function oryIssuer(): string { return CONFIG.auth.api.backendUrl ?? ""; @@ -48,17 +57,34 @@ export class IdentityBindingsTable extends BaseTable { } /** - * Binds the pair, or throws `ConditionalCheckFailedException` if it is - * already bound — to this account or any other. + * Binds the pair, or throws `IdentityAlreadyBoundError` if it is already + * bound — to this account or any other. */ async create(binding: IdentityBinding): Promise { - await this.client.send( - new PutCommand({ - TableName: this.table, - Item: binding, - ConditionExpression: "attribute_not_exists(issuer)", - }) - ); + try { + await this.client.send( + new PutCommand({ + TableName: this.table, + Item: binding, + ConditionExpression: "attribute_not_exists(issuer)", + }) + ); + } catch (error) { + if ((error as { name?: string })?.name === "ConditionalCheckFailedException") { + throw new IdentityAlreadyBoundError(binding.issuer, binding.subject); + } + // The app may deploy before the table does. The identity_id fallback + // still resolves the account, and the backfill writes this binding later. + if (error instanceof ResourceNotFoundException) { + LOGGER.warn("Identity binding not written: table does not exist yet", { + operation: "IdentityBindingsTable.create", + metadata: { account_id: binding.account_id }, + }); + return binding; + } + this.logError("create", error, { account_id: binding.account_id }); + throw error; + } return binding; } From 226d15a7c5a755a6f88182f71a0c1751b7222087 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Tue, 22 Sep 2026 22:56:58 -0700 Subject: [PATCH 4/6] refactor(accounts): keep Ory identities on the account row; bind only other issuers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An individual's Ory identity is not a binding. identity_id cannot leave the row — the session cookie, the Ory email lookup, proxy credentials and key minting all read it — so backfilling it into the bindings table would have kept a second copy of the same fact in step, in exchange for one index. The issuer it would have been keyed under is the Ory SDK URL from config, which no token carries as iss, so a change to that variable would have orphaned every person's login row at once. And the conditional write only buys something for attacker-chosen subjects, which an Ory subject never is. So fetchByOryId is as it was on main, create writes no binding, and the backfill, the dual-read fallback and the missing-table tolerance are gone. The table holds identities the platform does not own, and refuses an empty issuer or subject before touching DynamoDB. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z --- scripts/backfill-identity-bindings.ts | 206 ------------------ scripts/init-local.ts | 35 --- src/lib/actions/account.ts | 13 -- src/lib/api/oidc.ts | 5 +- .../clients/database/accounts.create.test.ts | 80 +------ .../database/accounts.identity.test.ts | 76 +++---- src/lib/clients/database/accounts.ts | 54 ++--- .../database/identity-bindings.test.ts | 16 +- src/lib/clients/database/identity-bindings.ts | 35 +-- 9 files changed, 77 insertions(+), 443 deletions(-) delete mode 100644 scripts/backfill-identity-bindings.ts diff --git a/scripts/backfill-identity-bindings.ts b/scripts/backfill-identity-bindings.ts deleted file mode 100644 index 5651c21a3..000000000 --- a/scripts/backfill-identity-bindings.ts +++ /dev/null @@ -1,206 +0,0 @@ -#!/usr/bin/env tsx -/** - * Give every individual account an identity binding — `(issuer, subject) -> - * account_id` in the identity-bindings table — for its Ory identity, so the - * app can resolve accounts through bindings and retire the `identity_id` index. - * - * ZERO-DOWNTIME, TWO-PHASE DESIGN - * ------------------------------- - * The app resolves an Ory identity through its binding first and falls back to - * the `identity_id` index for any account without one, logging the fallback. - * A missed account is a person who cannot sign in once the fallback goes, so - * the fallback stays until this script's verify phase says the counts match. - * - * MODE=backfill (default) — ADDITIVE. Writes one binding per individual - * account under the given issuer. Guarded by `attribute_not_exists(issuer)`, - * so it is idempotent and never clobbers a binding the app already wrote. - * - * MODE=verify — READ-ONLY. Counts individual accounts with an identity_id and - * bindings under the issuer, and exits non-zero if they differ. - * - * Rollout order: deploy the table + dual-read code -> run backfill -> run - * verify -> remove the fallback and the index in a follow-up. - * - * Usage: - * npx tsx scripts/backfill-identity-bindings.ts - * - * Examples: - * npx tsx scripts/backfill-identity-bindings.ts sc-dev-accounts sc-dev-identity-bindings https://auth.dev.source.coop - * MODE=verify npx tsx scripts/backfill-identity-bindings.ts sc-prod-accounts sc-prod-identity-bindings https://auth.source.coop - * - * Environment variables: - * MODE - "backfill" (default, additive) or "verify" (read-only) - * AWS_REGION - AWS region (default: us-east-1) - * AWS_PROFILE - AWS profile to use (optional) - * DRY_RUN - Set (to anything) to preview backfill writes without writing - */ - -import { DynamoDBClient } from "@aws-sdk/client-dynamodb"; -import { - DynamoDBDocumentClient, - PutCommand, - QueryCommand, - ScanCommand, -} from "@aws-sdk/lib-dynamodb"; - -type Mode = "backfill" | "verify"; - -interface AccountItem { - account_id: string; - type: string; - identity_id?: string; - created_at?: string; -} - -function isConditionalCheckFailed(err: unknown): boolean { - return err instanceof Error && err.name === "ConditionalCheckFailedException"; -} - -async function* individuals( - client: DynamoDBDocumentClient, - accountsTable: string -): AsyncGenerator { - let lastEvaluatedKey: Record | undefined; - do { - const result = await client.send( - new ScanCommand({ - TableName: accountsTable, - ProjectionExpression: "account_id, #type, identity_id, created_at", - ExpressionAttributeNames: { "#type": "type" }, - ExclusiveStartKey: lastEvaluatedKey, - }) - ); - lastEvaluatedKey = result.LastEvaluatedKey; - for (const item of (result.Items || []) as AccountItem[]) { - if (item.type === "individual" && item.identity_id) yield item; - } - } while (lastEvaluatedKey); -} - -async function countBindings( - client: DynamoDBDocumentClient, - bindingsTable: string, - issuer: string -): Promise { - let count = 0; - let lastEvaluatedKey: Record | undefined; - do { - const result = await client.send( - new QueryCommand({ - TableName: bindingsTable, - KeyConditionExpression: "issuer = :issuer", - ExpressionAttributeValues: { ":issuer": issuer }, - Select: "COUNT", - ExclusiveStartKey: lastEvaluatedKey, - }) - ); - count += result.Count ?? 0; - lastEvaluatedKey = result.LastEvaluatedKey; - } while (lastEvaluatedKey); - return count; -} - -async function run( - accountsTable: string, - bindingsTable: string, - issuer: string, - mode: Mode, - dryRun: boolean -) { - const region = process.env.AWS_REGION || "us-east-1"; - console.log(`Mode: ${mode}`); - console.log(`Accounts: ${accountsTable}`); - console.log(`Bindings: ${bindingsTable}`); - console.log(`Issuer: ${issuer}`); - console.log(`Region: ${region}`); - console.log(`Dry run: ${dryRun}`); - console.log(""); - - const client = DynamoDBDocumentClient.from(new DynamoDBClient({ region }), { - marshallOptions: { removeUndefinedValues: true }, - }); - - if (mode === "verify") { - let accounts = 0; - for await (const _ of individuals(client, accountsTable)) accounts++; - const bindings = await countBindings(client, bindingsTable, issuer); - console.log(`Individual accounts with an identity: ${accounts}`); - console.log(`Bindings under the issuer: ${bindings}`); - if (accounts !== bindings) { - console.error("MISMATCH — keep the identity_id fallback in place."); - process.exit(1); - } - console.log("Counts match. The fallback can go."); - return; - } - - let scanned = 0; - let written = 0; - let guarded = 0; - let errors = 0; - for await (const account of individuals(client, accountsTable)) { - scanned++; - const binding = { - issuer, - subject: account.identity_id, - account_id: account.account_id, - created_at: account.created_at ?? new Date().toISOString(), - }; - if (dryRun) { - console.log(`[DRY RUN] Would bind ${account.identity_id} -> ${account.account_id}`); - written++; - continue; - } - try { - await client.send( - new PutCommand({ - TableName: bindingsTable, - Item: binding, - ConditionExpression: "attribute_not_exists(issuer)", - }) - ); - written++; - } catch (err) { - if (isConditionalCheckFailed(err)) { - // Already bound — by an earlier run or by the app at signup. - guarded++; - continue; - } - errors++; - console.error(`Error binding ${account.account_id}:`, err); - } - } - - console.log(""); - console.log("backfill complete."); - console.log(` Individuals scanned: ${scanned}`); - console.log(` Bindings written: ${written}`); - console.log(` Already bound: ${guarded}`); - console.log(` Errors: ${errors}`); -} - -function usage() { - console.error( - "Usage: [MODE=backfill|verify] npx tsx scripts/backfill-identity-bindings.ts " - ); -} - -const [accountsTable, bindingsTable, issuer] = process.argv.slice(2); -if (!accountsTable || !bindingsTable || !issuer) { - usage(); - process.exit(1); -} - -const mode = (process.env.MODE || "backfill") as Mode; -if (mode !== "backfill" && mode !== "verify") { - console.error(`Invalid MODE: ${mode} (expected "backfill" or "verify")`); - usage(); - process.exit(1); -} - -run(accountsTable, bindingsTable, issuer, mode, process.env.DRY_RUN !== undefined).catch( - (err) => { - console.error(`${mode} failed:`, err); - process.exit(1); - } -); diff --git a/scripts/init-local.ts b/scripts/init-local.ts index f3d8fd5a3..c0b74866f 100644 --- a/scripts/init-local.ts +++ b/scripts/init-local.ts @@ -15,7 +15,6 @@ import { // Import mock data import { accounts, apiKeys, memberships, products, dataConnections } from "../src/lib/api/utils.mock"; -import { AccountType } from "../src/types"; const DYNAMODB_ENDPOINT = process.env.DYNAMODB_ENDPOINT || "http://localhost:8000"; @@ -398,40 +397,6 @@ async function loadFixtureData() { `Account insertion complete! Processed ${processedAccounts} accounts.` ); - // Every individual is reachable by its Ory identity under this - // environment's Ory issuer — the same binding the app writes at signup. - const issuer = process.env.NEXT_PUBLIC_ORY_SDK_URL || "http://localhost:4000"; - const bindings = accounts.flatMap((account) => - account.type === AccountType.INDIVIDUAL - ? [ - { - issuer, - subject: account.identity_id, - account_id: account.account_id, - created_at: account.created_at, - }, - ] - : [] - ); - console.log( - `Inserting ${bindings.length} identity bindings into ${getTableName( - "identity-bindings" - )} table...` - ); - for (const binding of bindings) { - try { - await docClient.send( - new PutCommand({ - TableName: getTableName("identity-bindings"), - Item: binding, - }) - ); - } catch (error) { - console.error(`Error inserting binding for ${binding.account_id}:`, error); - } - } - console.log(`Identity binding insertion complete!`); - // Insert API keys into DynamoDB console.log( `Inserting ${apiKeys.length} API keys into ${getTableName( diff --git a/src/lib/actions/account.ts b/src/lib/actions/account.ts index a6d219ca3..362856a9d 100644 --- a/src/lib/actions/account.ts +++ b/src/lib/actions/account.ts @@ -131,19 +131,6 @@ export async function createAccount( try { account = await accountsTable.create(newAccount); } catch (error) { - if ((error as { name?: string })?.name === "IdentityAlreadyBoundError") { - LOGGER.warn("Account creation rejected: identity already has an account", { - operation: "createAccount", - context: "account creation", - metadata: { identity_id: session.identity_id }, - }); - return { - fieldErrors: {}, - data: formData, - message: "This sign-in already has an account", - success: false, - }; - } if ((error as { name?: string })?.name === "ConditionalCheckFailedException") { LOGGER.warn("Account creation rejected: account_id already taken", { operation: "createAccount", diff --git a/src/lib/api/oidc.ts b/src/lib/api/oidc.ts index 26b1d911c..cbe8650ea 100644 --- a/src/lib/api/oidc.ts +++ b/src/lib/api/oidc.ts @@ -147,8 +147,9 @@ export async function authenticateWithOidcToken( // The token subject is the caller's Ory identity id (the data proxy signs // tokens with the authenticated principal's Ory id as `sub`), so resolve the - // account through its identity binding under the Ory issuer — NOT fetchById, - // which keys on the human-readable account_id. + // account via the identity_id index — NOT fetchById, which keys on the + // human-readable account_id. Only individual accounts have an Ory identity; + // org accounts are never the subject of a proxy-issued token. const account = await accountsTable.fetchByOryId(oryId); if (!account) { // Verified token, but no account is indexed under this Ory id. This is the diff --git a/src/lib/clients/database/accounts.create.test.ts b/src/lib/clients/database/accounts.create.test.ts index da6c9b80a..e3de028c0 100644 --- a/src/lib/clients/database/accounts.create.test.ts +++ b/src/lib/clients/database/accounts.create.test.ts @@ -1,16 +1,11 @@ import { AccountsTable } from "./accounts"; -import { IdentityBindingsTable } from "./identity-bindings"; -import { createMemoizedRead } from "./request-cache"; -import { fakeReactCache } from "./__test-helpers__/fake-react-cache"; import { DynamoDBDocumentClient } from "@aws-sdk/lib-dynamodb"; -import { ResourceNotFoundException } from "@aws-sdk/client-dynamodb"; import { AccountType, type Account } from "@/types"; jest.mock("@/lib/config", () => ({ CONFIG: { environment: { stage: "test" }, database: {}, - auth: { api: { backendUrl: "http://ory.test" } }, }, })); @@ -59,11 +54,7 @@ function fakeDynamo(seed: Account[] = []) { const send = jest.fn(async (command: { input: Record }) => { const item = command.input.Item as Account | undefined; - if (!item) { - const key = command.input.Key as { account_id: string } | undefined; - if (key) store.delete(key.account_id); - return {}; - } + if (!item) return {}; const condition = command.input.ConditionExpression as string | undefined; const guardsExistence = @@ -88,38 +79,12 @@ function fakeDynamo(seed: Account[] = []) { }; } -/** - * A bindings table whose store keys on the (issuer, subject) pair and honours - * `attribute_not_exists(issuer)` the way the real table does. - */ -function fakeBindings(seed: Array<{ issuer: string; subject: string }> = []) { - const store = new Set(seed.map((b) => `${b.issuer}\u0000${b.subject}`)); - const send = jest.fn(async (command: { input: Record }) => { - const item = command.input.Item as { issuer: string; subject: string }; - const key = `${item.issuer}\u0000${item.subject}`; - if (store.has(key)) { - const error = new Error("The conditional request failed") as Error & { - name: string; - }; - error.name = "ConditionalCheckFailedException"; - throw error; - } - store.add(key); - return {}; - }); - const table = new IdentityBindingsTable({ - client: { send } as unknown as DynamoDBDocumentClient, - memoizedRead: createMemoizedRead(fakeReactCache), - }); - return { store, table }; -} - describe("AccountsTable.create", () => { it("refuses to overwrite an account that already exists", async () => { // The victim already holds `victim`, bound to their Ory identity. const victim = makeAccount(); const { store, client } = fakeDynamo([victim]); - const table = new AccountsTable({ client, bindings: fakeBindings().table }); + const table = new AccountsTable({ client }); // An attacker who has registered an Ory identity but not yet created an // account submits the create form naming the victim's account_id. This is @@ -144,8 +109,7 @@ describe("AccountsTable.create", () => { it("still creates an account when the id is unused", async () => { const { store, client } = fakeDynamo([makeAccount()]); - const bindings = fakeBindings(); - const table = new AccountsTable({ client, bindings: bindings.table }); + const table = new AccountsTable({ client }); const newcomer = makeAccount({ account_id: "newcomer", @@ -156,43 +120,5 @@ describe("AccountsTable.create", () => { await expect(table.create(newcomer)).resolves.toEqual(newcomer); expect(store.get("newcomer")?.identity_id).toBe("ory-identity-newcomer"); expect(store.size).toBe(2); - // ...and binds the Ory identity under this environment's issuer. - expect(bindings.store.has("http://ory.test\u0000ory-identity-newcomer")).toBe(true); - }); - - it("does not leave an account behind when its identity is already bound", async () => { - // The identity already resolves to some other account. A second account for - // it must not be created half-way: the row is written, the binding fails, - // and the row is removed again. - const { store, client } = fakeDynamo([makeAccount()]); - const bindings = fakeBindings([ - { issuer: "http://ory.test", subject: "ory-identity-victim" }, - ]); - const table = new AccountsTable({ client, bindings: bindings.table }); - - const second = makeAccount({ account_id: "second", name: "Second" }); - - // ...and the caller learns which conflict this was, not "account_id taken". - await expect(table.create(second)).rejects.toThrow( - expect.objectContaining({ name: "IdentityAlreadyBoundError" }) - ); - expect(store.has("second")).toBe(false); - }); - - it("still creates the account when the bindings table does not exist yet", async () => { - // The app may deploy before the table does; signups must not fail in the - // window. The identity_id fallback resolves the account until the backfill. - const { store, client } = fakeDynamo(); - const send = jest.fn(async () => { - throw new ResourceNotFoundException({ message: "no table", $metadata: {} }); - }); - const bindings = new IdentityBindingsTable({ - client: { send } as unknown as DynamoDBDocumentClient, - memoizedRead: createMemoizedRead(fakeReactCache), - }); - const table = new AccountsTable({ client, bindings }); - - await expect(table.create(makeAccount())).resolves.toBeDefined(); - expect(store.has("victim")).toBe(true); }); }); diff --git a/src/lib/clients/database/accounts.identity.test.ts b/src/lib/clients/database/accounts.identity.test.ts index 75fe2cd03..172526f89 100644 --- a/src/lib/clients/database/accounts.identity.test.ts +++ b/src/lib/clients/database/accounts.identity.test.ts @@ -3,15 +3,10 @@ import { AccountsTable } from "./accounts"; import { IdentityBindingsTable } from "./identity-bindings"; import { createMemoizedRead } from "./request-cache"; import { fakeReactCache } from "./__test-helpers__/fake-react-cache"; -import { LOGGER } from "@/lib/logging"; import { AccountType, type Account, type IdentityBinding } from "@/types"; jest.mock("@/lib/config", () => ({ - CONFIG: { - environment: { stage: "test" }, - database: {}, - auth: { api: { backendUrl: "http://ory.test" } }, - }, + CONFIG: { environment: { stage: "test" }, database: {} }, })); jest.mock("@/lib/logging", () => ({ LOGGER: { error: jest.fn(), warn: jest.fn(), debug: jest.fn(), info: jest.fn() }, @@ -22,7 +17,7 @@ const account = (fields: Partial): Account => const jane = account({ account_id: "jane-doe", identity_id: "ory-jane" }); const acme = account({ account_id: "acme", type: AccountType.ORGANIZATION }); -const unbacked = account({ account_id: "old-timer", identity_id: "ory-old" }); +const bot = account({ account_id: "acme-bot", type: AccountType.SERVICE }); /** Answers the key query and the identity_id index query from `items`. */ function fakeAccounts(items: Account[]) { @@ -50,79 +45,70 @@ function fakeBindings(bindings: IdentityBinding[]) { const values = command.input.ExpressionAttributeValues as Record; return { Items: bindings.filter((b) => b.account_id === values[":account_id"]) }; }); - return new IdentityBindingsTable({ + const table = new IdentityBindingsTable({ client: { send } as unknown as DynamoDBDocumentClient, memoizedRead: createMemoizedRead(fakeReactCache), }); + return { table, send }; } function tableFor(items: Account[], bindings: IdentityBinding[]) { const accounts = fakeAccounts(items); + const bound = fakeBindings(bindings); const table = new AccountsTable({ client: accounts.client, memoizedRead: createMemoizedRead(fakeReactCache), - bindings: fakeBindings(bindings), + bindings: bound.table, }); - return { table, send: accounts.send }; + return { table, send: accounts.send, bindingsSend: bound.send }; } -const bound = (subject: string, account_id: string, issuer = "http://ory.test") => +const CI = "https://ci.example"; +const PROXY = "https://data.example"; + +const bound = (issuer: string, subject: string, account_id: string) => ({ issuer, subject, account_id, created_at: "2024-01-01T00:00:00Z" }) as IdentityBinding; describe("AccountsTable identity resolution", () => { beforeEach(() => jest.clearAllMocks()); - it("resolves an Ory identity through its binding, without touching the index", async () => { - const { table, send } = tableFor([jane, unbacked], [bound("ory-jane", "jane-doe")]); + it("resolves an Ory identity through the identity_id index, never the bindings table", async () => { + // Ory identities are not bindings: a person's identity_id is on the row, + // and the session, the email lookup and the proxy credentials read it there. + const { table, bindingsSend } = tableFor([jane], [bound(PROXY, "ory-jane", "acme-bot")]); expect(await table.fetchByOryId("ory-jane")).toEqual(jane); - expect(send.mock.calls.map((c) => c[0].input.IndexName)).toEqual([undefined]); - expect(LOGGER.warn).not.toHaveBeenCalled(); - }); - - it("falls back to the identity_id index for an account with no binding yet, and says so", async () => { - const { table } = tableFor([jane, unbacked], [bound("ory-jane", "jane-doe")]); - - expect(await table.fetchByOryId("ory-old")).toEqual(unbacked); - expect(LOGGER.warn).toHaveBeenCalledWith( - expect.stringContaining("identity_id index"), - expect.objectContaining({ metadata: { identity_id: "ory-old", account_id: "old-timer" } }) - ); - }); - - it("returns nothing for an unknown identity", async () => { - const { table } = tableFor([jane], [bound("ory-jane", "jane-doe")]); expect(await table.fetchByOryId("ory-nobody")).toBeNull(); + expect(bindingsSend).not.toHaveBeenCalled(); }); - it("keeps fetchByOryId to individuals, while fetchByIdentity resolves any account type", async () => { - const bindings = [bound("ci-subject", "acme", "https://ci.example")]; - const { table } = tableFor([acme], bindings); + it("resolves any account type through a binding, while fetchByOryId stays individual-only", async () => { + const bindings = [bound(CI, "ci-subject", "acme"), bound(PROXY, "acme-bot", "acme-bot")]; + const { table } = tableFor([acme, bot], bindings); - expect(await table.fetchByIdentity("https://ci.example", "ci-subject")).toEqual(acme); - // The same pair under the Ory issuer is not bound... + expect(await table.fetchByIdentity(CI, "ci-subject")).toEqual(acme); + expect(await table.fetchByIdentity(PROXY, "acme-bot")).toEqual(bot); + // A bound subject is not an Ory identity; nothing on the row carries it. expect(await table.fetchByOryId("ci-subject")).toBeNull(); - // ...and a non-individual bound under Ory is not an Ory account either. - const { table: odd } = tableFor([acme], [bound("ory-acme", "acme")]); - expect(await odd.fetchByOryId("ory-acme")).toBeNull(); + expect(await table.fetchByIdentity(CI, "unknown")).toBeNull(); }); it("removes an account's bindings when the account is deleted", async () => { - // Otherwise the orphaned pair would keep that identity from ever binding again. - const bindings = [bound("ory-jane", "jane-doe"), bound("ci", "jane-doe", "https://ci.example")]; - const { table } = tableFor([jane], bindings); + // Otherwise the orphaned pair would keep that subject from ever binding again. + const bindings = [bound(PROXY, "acme-bot", "acme-bot"), bound(CI, "ci", "acme-bot")]; + const { table } = tableFor([bot], bindings); - await table.delete({ account_id: "jane-doe", type: AccountType.INDIVIDUAL }); + await table.delete({ account_id: "acme-bot", type: AccountType.SERVICE }); expect(bindings).toEqual([]); }); it("tells issuers apart", async () => { const { table } = tableFor( - [jane, acme], - [bound("shared", "jane-doe"), bound("shared", "acme", "https://ci.example")] + [acme, bot], + [bound(CI, "shared", "acme"), bound(PROXY, "shared", "acme-bot")] ); - expect(await table.fetchByIdentity("http://ory.test", "shared")).toEqual(jane); - expect(await table.fetchByIdentity("https://ci.example", "shared")).toEqual(acme); + expect(await table.fetchByIdentity(CI, "shared")).toEqual(acme); + expect(await table.fetchByIdentity(PROXY, "shared")).toEqual(bot); }); }); diff --git a/src/lib/clients/database/accounts.ts b/src/lib/clients/database/accounts.ts index 12bb45cd5..064b90dec 100644 --- a/src/lib/clients/database/accounts.ts +++ b/src/lib/clients/database/accounts.ts @@ -17,11 +17,7 @@ import { } from "@/types"; import { BaseTable } from "./base"; -import { - IdentityBindingsTable, - identityBindingsTable, - oryIssuer, -} from "./identity-bindings"; +import { IdentityBindingsTable, identityBindingsTable } from "./identity-bindings"; import { LOGGER } from "@/lib/logging"; /** @@ -121,7 +117,11 @@ export class AccountsTable extends BaseTable { return accountBatches; } - /** The account `subject` resolves to, as named by `issuer`. Any type. */ + /** + * The account `subject` resolves to, as named by `issuer` — for identities + * the platform does not own. An Ory identity is `fetchByOryId`'s to resolve, + * from the row itself. + */ async fetchByIdentity( issuer: string, subject: string @@ -130,18 +130,13 @@ export class AccountsTable extends BaseTable { return binding ? this.fetchById(binding.account_id) : null; } - /** - * The individual an Ory identity resolves to. Ory identities are bound under - * this environment's Ory issuer. While the backfill lands (#543), an account - * with no binding yet is still found through the `identity_id` index, and - * that fallback is logged so the unbacked accounts are countable. Remove the - * fallback, and the index with it, once the backfill is verified. - */ async fetchByOryId(identity_id: string): Promise { - const bound = await this.fetchByIdentity(oryIssuer(), identity_id); - if (bound) return isIndividualAccount(bound) ? bound : null; - try { + LOGGER.debug(`Trying to fetch account by Ory ID`, { + operation: "AccountsTable.fetchByOryId", + context: "database operation", + metadata: { identity_id }, + }); const result = await this.cachedSend( new QueryCommand({ TableName: this.table, @@ -159,10 +154,10 @@ export class AccountsTable extends BaseTable { if (!account) return null; - LOGGER.warn("Account resolved through the identity_id index, not a binding", { + LOGGER.debug(`Found account by Ory ID`, { operation: "AccountsTable.fetchByOryId", context: "database operation", - metadata: { identity_id, account_id: account.account_id }, + metadata: { identity_id, account }, }); return account as IndividualAccount; } catch (error) { @@ -258,29 +253,6 @@ export class AccountsTable extends BaseTable { throw error; } - // An individual is reachable by its Ory identity, so it is bound in the - // same breath. If that identity is already bound elsewhere, the account - // row must not survive without a binding of its own; the - // IdentityAlreadyBoundError propagates for the caller to name. - if (isIndividualAccount(account)) { - try { - await this.bindings.create({ - issuer: oryIssuer(), - subject: account.identity_id, - account_id: account.account_id, - created_at: account.created_at, - }); - } catch (error) { - await this.client.send( - new DeleteCommand({ - TableName: this.table, - Key: { account_id: account.account_id }, - }) - ); - throw error; - } - } - return account; } diff --git a/src/lib/clients/database/identity-bindings.test.ts b/src/lib/clients/database/identity-bindings.test.ts index f19e83a62..08d392e37 100644 --- a/src/lib/clients/database/identity-bindings.test.ts +++ b/src/lib/clients/database/identity-bindings.test.ts @@ -1,5 +1,4 @@ import { type DynamoDBDocumentClient } from "@aws-sdk/lib-dynamodb"; -import { ResourceNotFoundException } from "@aws-sdk/client-dynamodb"; import { IdentityAlreadyBoundError, IdentityBindingsTable } from "./identity-bindings"; import { createMemoizedRead } from "./request-cache"; import { fakeReactCache } from "./__test-helpers__/fake-react-cache"; @@ -57,11 +56,14 @@ describe("IdentityBindingsTable", () => { expect(await tableWith(jest.fn(async () => ({}))).resolve("x", "y")).toBeNull(); }); - it("treats a table that does not exist yet as holding no bindings", async () => { - // The app may deploy before the table does; nothing is bound until then. - const send = jest.fn(async () => { - throw new ResourceNotFoundException({ message: "no table", $metadata: {} }); - }); - expect(await tableWith(send).resolve("x", "y")).toBeNull(); + it("refuses an empty issuer or subject, before touching the table", async () => { + // An issuer read from unset config is "", and every binding written under + // it would share one partition. + const send = jest.fn(async () => ({})); + const table = tableWith(send); + await expect(table.resolve("", "subject")).rejects.toThrow(/issuer and a subject/); + await expect(table.create({ ...binding, issuer: "" })).rejects.toThrow(/issuer and a subject/); + await expect(table.create({ ...binding, subject: "" })).rejects.toThrow(/issuer and a subject/); + expect(send).not.toHaveBeenCalled(); }); }); diff --git a/src/lib/clients/database/identity-bindings.ts b/src/lib/clients/database/identity-bindings.ts index 56e7b30d6..2b157f414 100644 --- a/src/lib/clients/database/identity-bindings.ts +++ b/src/lib/clients/database/identity-bindings.ts @@ -4,9 +4,6 @@ import { PutCommand, QueryCommand, } from "@aws-sdk/lib-dynamodb"; -import { ResourceNotFoundException } from "@aws-sdk/client-dynamodb"; -import { CONFIG } from "@/lib/config"; -import { LOGGER } from "@/lib/logging"; import type { IdentityBinding } from "@/types"; import { BaseTable } from "./base"; @@ -18,11 +15,24 @@ export class IdentityAlreadyBoundError extends Error { } } -/** The issuer every Ory identity is bound under: this environment's Ory project. */ -export function oryIssuer(): string { - return CONFIG.auth.api.backendUrl ?? ""; +/** + * Both halves of the key must be real. An issuer read from unset config would + * be `""`, and every binding written under it would share one partition and + * answer for each other. + */ +function requireKey(issuer: string, subject: string): void { + if (!issuer || !subject) { + throw new Error("An identity binding needs both an issuer and a subject"); + } } +/** + * Identities the platform does not own — a service account's subject under the + * data proxy's issuer, a GitHub Actions workflow — resolved to the account they + * belong to. An individual's Ory identity is not one of them: it stays on the + * account row as `identity_id`, where the session, the email lookup and the + * proxy credentials already read it. + */ export class IdentityBindingsTable extends BaseTable { model = "identity-bindings"; @@ -31,14 +41,13 @@ export class IdentityBindingsTable extends BaseTable { issuer: string, subject: string ): Promise { + requireKey(issuer, subject); try { const result = await this.cachedSend( new GetCommand({ TableName: this.table, Key: { issuer, subject } }) ); return (result.Item as IdentityBinding | undefined) ?? null; } catch (error) { - // The app may deploy before the table does; until then nothing is bound. - if (error instanceof ResourceNotFoundException) return null; this.logError("resolve", error, { issuer, subject }); throw error; } @@ -61,6 +70,7 @@ export class IdentityBindingsTable extends BaseTable { * bound — to this account or any other. */ async create(binding: IdentityBinding): Promise { + requireKey(binding.issuer, binding.subject); try { await this.client.send( new PutCommand({ @@ -73,15 +83,6 @@ export class IdentityBindingsTable extends BaseTable { if ((error as { name?: string })?.name === "ConditionalCheckFailedException") { throw new IdentityAlreadyBoundError(binding.issuer, binding.subject); } - // The app may deploy before the table does. The identity_id fallback - // still resolves the account, and the backfill writes this binding later. - if (error instanceof ResourceNotFoundException) { - LOGGER.warn("Identity binding not written: table does not exist yet", { - operation: "IdentityBindingsTable.create", - metadata: { account_id: binding.account_id }, - }); - return binding; - } this.logError("create", error, { account_id: binding.account_id }); throw error; } From 0d678d009ab0b90feb7f87b7a03231b3cc657305 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Tue, 22 Sep 2026 23:32:08 -0700 Subject: [PATCH 5/6] docs(accounts): say what the bindings table is for now that keys write none Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z --- src/lib/clients/database/identity-bindings.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lib/clients/database/identity-bindings.ts b/src/lib/clients/database/identity-bindings.ts index 2b157f414..6fea58904 100644 --- a/src/lib/clients/database/identity-bindings.ts +++ b/src/lib/clients/database/identity-bindings.ts @@ -27,11 +27,12 @@ function requireKey(issuer: string, subject: string): void { } /** - * Identities the platform does not own — a service account's subject under the - * data proxy's issuer, a GitHub Actions workflow — resolved to the account they + * Subjects the platform cannot derive from an account — a GitHub Actions + * workflow, whatever platform IdP comes next — resolved to the account they * belong to. An individual's Ory identity is not one of them: it stays on the * account row as `identity_id`, where the session, the email lookup and the - * proxy credentials already read it. + * proxy credentials already read it. Nor is an API key's subject, which is the + * service account's own id and resolves by id. */ export class IdentityBindingsTable extends BaseTable { model = "identity-bindings"; From 788c22d7611d0991cfe642e8f66260f8a1263826 Mon Sep 17 00:00:00 2001 From: Anthony Lukach Date: Tue, 22 Sep 2026 23:48:40 -0700 Subject: [PATCH 6/6] fix(accounts): delete an account and its bindings in one transaction, keyed as the table is AccountsTable.delete sent a key with a type attribute the accounts table is not keyed on, which DynamoDB rejects; the fake in its test accepted any key, and its delete handler removed the last binding on a miss, so nothing caught it. The bindings and the row now go in one TransactWriteCommand, so a failure leaves nothing half-done, and the bindings are read past the request cache so a binding written earlier in the request is not missed. IdentityBindingsTable.create validates the row with the schema instead of a duplicate check, and the schema's examples show a GitHub Actions binding rather than the Ory one this table does not hold. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01REZWKgQy2PDETn6j9YpM4z --- .../database/accounts.identity.test.ts | 36 ++++++------ src/lib/clients/database/accounts.ts | 29 +++++++--- .../database/identity-bindings.test.ts | 24 ++++++-- src/lib/clients/database/identity-bindings.ts | 55 +++++++++---------- src/types/identity-binding.ts | 6 +- 5 files changed, 89 insertions(+), 61 deletions(-) diff --git a/src/lib/clients/database/accounts.identity.test.ts b/src/lib/clients/database/accounts.identity.test.ts index 172526f89..7426c2fdc 100644 --- a/src/lib/clients/database/accounts.identity.test.ts +++ b/src/lib/clients/database/accounts.identity.test.ts @@ -19,11 +19,11 @@ const jane = account({ account_id: "jane-doe", identity_id: "ory-jane" }); const acme = account({ account_id: "acme", type: AccountType.ORGANIZATION }); const bot = account({ account_id: "acme-bot", type: AccountType.SERVICE }); -/** Answers the key query and the identity_id index query from `items`. */ +/** Answers the key query and the identity_id index query from `items`; records writes. */ function fakeAccounts(items: Account[]) { const send = jest.fn(async (command: { input: Record }) => { const values = command.input.ExpressionAttributeValues as Record | undefined; - if (!values) return {}; // a Delete of the row itself + if (!values) return {}; // a write if (values[":account_id"] !== undefined) { return { Items: items.filter((a) => a.account_id === values[":account_id"]) }; } @@ -32,16 +32,11 @@ function fakeAccounts(items: Account[]) { return { send, client: { send } as unknown as DynamoDBDocumentClient }; } -/** Answers Get by key, Query on the account_id index, and Delete, over `bindings`. */ +/** Answers Get by key and Query on the account_id index over `bindings`. */ function fakeBindings(bindings: IdentityBinding[]) { const send = jest.fn(async (command: { input: Record }) => { const key = command.input.Key as { issuer: string; subject: string } | undefined; - const at = (b: IdentityBinding) => b.issuer === key?.issuer && b.subject === key?.subject; - if (key && command.constructor.name === "DeleteCommand") { - bindings.splice(bindings.findIndex(at), 1); - return {}; - } - if (key) return { Item: bindings.find(at) }; + if (key) return { Item: bindings.find((b) => b.issuer === key.issuer && b.subject === key.subject) }; const values = command.input.ExpressionAttributeValues as Record; return { Items: bindings.filter((b) => b.account_id === values[":account_id"]) }; }); @@ -93,14 +88,23 @@ describe("AccountsTable identity resolution", () => { expect(await table.fetchByIdentity(CI, "unknown")).toBeNull(); }); - it("removes an account's bindings when the account is deleted", async () => { - // Otherwise the orphaned pair would keep that subject from ever binding again. + it("removes an account and its bindings in one transaction, keyed as the tables are", async () => { + // Otherwise an orphaned pair would keep that subject from ever binding + // again — and a half-done cascade would leave an account some of whose + // sign-in paths are gone. const bindings = [bound(PROXY, "acme-bot", "acme-bot"), bound(CI, "ci", "acme-bot")]; - const { table } = tableFor([bot], bindings); - - await table.delete({ account_id: "acme-bot", type: AccountType.SERVICE }); - - expect(bindings).toEqual([]); + const { table, send, bindingsSend } = tableFor([bot], bindings); + + await table.delete("acme-bot"); + + // The list is read fresh, not from the request cache. + expect(bindingsSend.mock.calls[0][0].constructor.name).toBe("QueryCommand"); + const write = send.mock.calls.find((c) => c[0].constructor.name === "TransactWriteCommand")!; + expect(write[0].input.TransactItems).toEqual([ + { Delete: { TableName: "sc-test-identity-bindings", Key: { issuer: PROXY, subject: "acme-bot" } } }, + { Delete: { TableName: "sc-test-identity-bindings", Key: { issuer: CI, subject: "ci" } } }, + { Delete: { TableName: "sc-test-accounts", Key: { account_id: "acme-bot" } } }, + ]); }); it("tells issuers apart", async () => { diff --git a/src/lib/clients/database/accounts.ts b/src/lib/clients/database/accounts.ts index 08c7e314a..2f00a2e51 100644 --- a/src/lib/clients/database/accounts.ts +++ b/src/lib/clients/database/accounts.ts @@ -4,7 +4,7 @@ import { ScanCommand, type ScanCommandOutput, UpdateCommand, - DeleteCommand, + TransactWriteCommand, PutCommand, BatchGetCommand, } from "@aws-sdk/lib-dynamodb"; @@ -344,15 +344,26 @@ export class AccountsTable extends BaseTable { ) as ServiceAccount[]; } - async delete(Key: { account_id: string; type: AccountType }): Promise { - // An orphaned binding would keep the identity from ever binding again. - for (const binding of await this.bindings.listByAccount(Key.account_id)) { - await this.bindings.delete(binding.issuer, binding.subject); - } + /** + * Removes the account and its bindings together. An orphaned binding would + * keep its subject from ever binding again, and a cascade that stopped + * half-way would leave an account some of whose sign-in paths are gone, so + * the deletes are one transaction. The bindings are read fresh: a memoized + * list from earlier in the request could predate one of them. + */ + async delete(account_id: string): Promise { + const bindings = await this.bindings.listByAccount(account_id, true); await this.client.send( - new DeleteCommand({ - TableName: this.table, - Key, + new TransactWriteCommand({ + TransactItems: [ + ...bindings.map((b) => ({ + Delete: { + TableName: this.bindings.table, + Key: { issuer: b.issuer, subject: b.subject }, + }, + })), + { Delete: { TableName: this.table, Key: { account_id } } }, + ], }) ); } diff --git a/src/lib/clients/database/identity-bindings.test.ts b/src/lib/clients/database/identity-bindings.test.ts index 08d392e37..316fdcf64 100644 --- a/src/lib/clients/database/identity-bindings.test.ts +++ b/src/lib/clients/database/identity-bindings.test.ts @@ -56,14 +56,28 @@ describe("IdentityBindingsTable", () => { expect(await tableWith(jest.fn(async () => ({}))).resolve("x", "y")).toBeNull(); }); - it("refuses an empty issuer or subject, before touching the table", async () => { + it("refuses a malformed row before touching the table", async () => { // An issuer read from unset config is "", and every binding written under - // it would share one partition. + // it would share one partition; an empty account_id cannot be indexed. const send = jest.fn(async () => ({})); const table = tableWith(send); - await expect(table.resolve("", "subject")).rejects.toThrow(/issuer and a subject/); - await expect(table.create({ ...binding, issuer: "" })).rejects.toThrow(/issuer and a subject/); - await expect(table.create({ ...binding, subject: "" })).rejects.toThrow(/issuer and a subject/); + await expect(table.create({ ...binding, issuer: "" })).rejects.toThrow(); + await expect(table.create({ ...binding, subject: "" })).rejects.toThrow(); + await expect(table.create({ ...binding, account_id: "" })).rejects.toThrow(); + await expect(table.create({ ...binding, created_at: "yesterday" })).rejects.toThrow(); expect(send).not.toHaveBeenCalled(); }); + + it("lists an account's bindings from the request cache unless told not to", async () => { + const send = jest.fn(async () => ({ Items: [binding] })); + const table = tableWith(send); + + expect(await table.listByAccount("o-bot")).toEqual([binding]); + expect(await table.listByAccount("o-bot")).toEqual([binding]); + expect(send).toHaveBeenCalledTimes(1); + + // A destructive caller reads what the table holds now. + expect(await table.listByAccount("o-bot", true)).toEqual([binding]); + expect(send).toHaveBeenCalledTimes(2); + }); }); diff --git a/src/lib/clients/database/identity-bindings.ts b/src/lib/clients/database/identity-bindings.ts index 6fea58904..951dcab81 100644 --- a/src/lib/clients/database/identity-bindings.ts +++ b/src/lib/clients/database/identity-bindings.ts @@ -4,7 +4,7 @@ import { PutCommand, QueryCommand, } from "@aws-sdk/lib-dynamodb"; -import type { IdentityBinding } from "@/types"; +import { IdentityBindingSchema, type IdentityBinding } from "@/types"; import { BaseTable } from "./base"; /** The (issuer, subject) pair already resolves to an account. */ @@ -15,17 +15,6 @@ export class IdentityAlreadyBoundError extends Error { } } -/** - * Both halves of the key must be real. An issuer read from unset config would - * be `""`, and every binding written under it would share one partition and - * answer for each other. - */ -function requireKey(issuer: string, subject: string): void { - if (!issuer || !subject) { - throw new Error("An identity binding needs both an issuer and a subject"); - } -} - /** * Subjects the platform cannot derive from an account — a GitHub Actions * workflow, whatever platform IdP comes next — resolved to the account they @@ -42,7 +31,6 @@ export class IdentityBindingsTable extends BaseTable { issuer: string, subject: string ): Promise { - requireKey(issuer, subject); try { const result = await this.cachedSend( new GetCommand({ TableName: this.table, Key: { issuer, subject } }) @@ -54,40 +42,51 @@ export class IdentityBindingsTable extends BaseTable { } } - async listByAccount(account_id: string): Promise { - const result = await this.cachedSend( - new QueryCommand({ - TableName: this.table, - IndexName: "account_id", - KeyConditionExpression: "account_id = :account_id", - ExpressionAttributeValues: { ":account_id": account_id }, - }) - ); + /** + * An account's bindings. A destructive caller passes `bypassCache` so it + * acts on what the table holds now, not on a read memoized earlier in the + * request that may predate a binding. + */ + async listByAccount( + account_id: string, + bypassCache = false + ): Promise { + const command = new QueryCommand({ + TableName: this.table, + IndexName: "account_id", + KeyConditionExpression: "account_id = :account_id", + ExpressionAttributeValues: { ":account_id": account_id }, + }); + const result = bypassCache + ? await this.client.send(command) + : await this.cachedSend(command); return (result.Items ?? []) as IdentityBinding[]; } /** * Binds the pair, or throws `IdentityAlreadyBoundError` if it is already - * bound — to this account or any other. + * bound — to this account or any other. The row is validated first: an + * issuer read from unset config would be `""`, and every binding written + * under it would share one partition. */ async create(binding: IdentityBinding): Promise { - requireKey(binding.issuer, binding.subject); + const row = IdentityBindingSchema.parse(binding); try { await this.client.send( new PutCommand({ TableName: this.table, - Item: binding, + Item: row, ConditionExpression: "attribute_not_exists(issuer)", }) ); } catch (error) { if ((error as { name?: string })?.name === "ConditionalCheckFailedException") { - throw new IdentityAlreadyBoundError(binding.issuer, binding.subject); + throw new IdentityAlreadyBoundError(row.issuer, row.subject); } - this.logError("create", error, { account_id: binding.account_id }); + this.logError("create", error, { account_id: row.account_id }); throw error; } - return binding; + return row; } async delete(issuer: string, subject: string): Promise { diff --git a/src/types/identity-binding.ts b/src/types/identity-binding.ts index 68ffa612e..6e49a1c42 100644 --- a/src/types/identity-binding.ts +++ b/src/types/identity-binding.ts @@ -10,9 +10,9 @@ extendZodWithOpenApi(z); */ export const IdentityBindingSchema = z .object({ - issuer: z.string().min(1).openapi({ example: "https://auth.source.coop" }), - subject: z.string().min(1).openapi({ example: "identity-id" }), - account_id: z.string().openapi({ example: "account-id" }), + issuer: z.string().min(1).openapi({ example: "https://token.actions.githubusercontent.com" }), + subject: z.string().min(1).openapi({ example: "repo:acme/data:ref:refs/heads/main" }), + account_id: z.string().min(1).openapi({ example: "acme-nightly-sync" }), created_at: z.string().datetime(), }) .openapi("IdentityBinding");