diff --git a/packages/accounts/db.integration.test.ts b/packages/accounts/db.integration.test.ts index 61170ec..3011323 100644 --- a/packages/accounts/db.integration.test.ts +++ b/packages/accounts/db.integration.test.ts @@ -221,6 +221,25 @@ describe("org-member", () => { expect(owners.length).toBe(1); expect(owners[0]?.identityId).toBe(owner2.id); }); + + test("delete org cascades to org_member even when sole owner exists", async () => { + // Regression: previously the org_member_owner_check trigger refused + // the cascade, making it impossible to delete an org you owned. The + // invariant now lives in removeMember/updateRole so the cascade + // proceeds unimpeded. + const org = await db.createOrg({ name: "Sole Owner Cascade" }); + const identity = await db.createIdentity({ + email: "sole-owner-cascade@example.com", + name: "Sole Owner", + }); + await db.addMember(org.id, identity.id, "owner"); + + const deleted = await db.deleteOrg(org.id); + expect(deleted).toBe(true); + + const members = await db.listMembers(org.id); + expect(members.length).toBe(0); + }); }); // --------------------------------------------------------------------------- diff --git a/packages/accounts/migrate/migrations/008_drop_org_owner_trigger.sql b/packages/accounts/migrate/migrations/008_drop_org_owner_trigger.sql new file mode 100644 index 0000000..260efbf --- /dev/null +++ b/packages/accounts/migrate/migrations/008_drop_org_owner_trigger.sql @@ -0,0 +1,13 @@ +-- Remove the "must keep at least one owner" trigger on org_member. +-- +-- The trigger fired before any DELETE or UPDATE on org_member, including +-- the rows cascaded by `delete from org` (since org_member.org_id has +-- `references org on delete cascade`). When an org was being deleted in +-- its entirety, the cascade removed the org's owner row too, and the +-- trigger refused — making it impossible to delete an org that you owned. +-- +-- The invariant is now enforced at the application layer in +-- packages/accounts/ops/org-member.ts (removeMember + updateRole), where +-- it can distinguish member-management flows from a cascading org delete. +drop trigger if exists org_member_owner_check on {{schema}}.org_member; +drop function if exists {{schema}}.check_org_has_owner(); diff --git a/packages/accounts/migrate/runner.ts b/packages/accounts/migrate/runner.ts index aea8d3d..28f830a 100644 --- a/packages/accounts/migrate/runner.ts +++ b/packages/accounts/migrate/runner.ts @@ -18,6 +18,9 @@ import migration006 from "./migrations/006_ops_support.sql" with { import migration007 from "./migrations/007_device_authorization.sql" with { type: "text", }; +import migration008 from "./migrations/008_drop_org_owner_trigger.sql" with { + type: "text", +}; import { type AccountsConfig, resolveConfig, template } from "./template"; interface Migration { @@ -33,6 +36,7 @@ const migrations: Migration[] = [ { name: "005_auth", sql: migration005 }, { name: "006_ops_support", sql: migration006 }, { name: "007_device_authorization", sql: migration007 }, + { name: "008_drop_org_owner_trigger", sql: migration008 }, ]; export interface MigrateResult { diff --git a/packages/accounts/ops/org-member.ts b/packages/accounts/ops/org-member.ts index 56ca1f7..9ea7e2b 100644 --- a/packages/accounts/ops/org-member.ts +++ b/packages/accounts/ops/org-member.ts @@ -27,10 +27,37 @@ function rowToOrgMember(row: OrgMemberRow): OrgMember { } /** - * Check if a PostgreSQL error is the "org_must_have_owner" trigger exception + * Verify that removing or demoting `identityId` from `orgId` would leave at + * least one other owner in place. Throws ORG_MUST_HAVE_OWNER otherwise. + * + * The query takes `for update` row locks on every other owner row in the + * org. This closes the race where two concurrent transactions, each + * removing a different owner, both observe the other's row as still + * present and proceed — leaving the org with zero owners. With `for + * update`, the second transaction blocks on the first's row locks, sees + * the post-commit count, and correctly fails. The previous DB trigger + * had the same race; this hardens it. */ -function isOrgOwnerError(err: unknown): boolean { - return err instanceof Error && err.message?.includes("org_must_have_owner"); +async function assertAnotherOwnerExists( + sql: import("bun").SQL, + schema: string, + orgId: string, + identityId: string, +): Promise { + const rows = await sql<{ identity_id: string }[]>` + select identity_id + from ${sql.unsafe(schema)}.org_member + where org_id = ${orgId} + and role = 'owner' + and identity_id <> ${identityId} + for update + `; + if (rows.length === 0) { + throw new AccountsError( + "ORG_MUST_HAVE_OWNER", + "Cannot remove the last owner from an organization", + ); + } } export function orgMemberOps(ctx: AccountsContext) { @@ -63,21 +90,25 @@ export function orgMemberOps(ctx: AccountsContext) { async removeMember(orgId: string, identityId: string): Promise { return withTx(ctx, "removeMember", async (sql) => { - try { - const result = await sql` - delete from ${sql.unsafe(schema)}.org_member - where org_id = ${orgId} and identity_id = ${identityId} - `; - return result.count > 0; - } catch (err) { - if (isOrgOwnerError(err)) { - throw new AccountsError( - "ORG_MUST_HAVE_OWNER", - "Cannot remove the last owner from an organization", - ); - } - throw err; + // Fetch the target row with `for update` so the row stays stable + // under our feet while we decide whether to delete it. + const [target] = await sql<{ role: OrgRole }[]>` + select role + from ${sql.unsafe(schema)}.org_member + where org_id = ${orgId} and identity_id = ${identityId} + for update + `; + if (!target) return false; + + if (target.role === "owner") { + await assertAnotherOwnerExists(sql, schema, orgId, identityId); } + + const result = await sql` + delete from ${sql.unsafe(schema)}.org_member + where org_id = ${orgId} and identity_id = ${identityId} + `; + return result.count > 0; }); }, @@ -87,22 +118,26 @@ export function orgMemberOps(ctx: AccountsContext) { newRole: OrgRole, ): Promise { return withTx(ctx, "updateRole", async (sql) => { - try { - const result = await sql` - update ${sql.unsafe(schema)}.org_member - set role = ${newRole} - where org_id = ${orgId} and identity_id = ${identityId} - `; - return result.count > 0; - } catch (err) { - if (isOrgOwnerError(err)) { - throw new AccountsError( - "ORG_MUST_HAVE_OWNER", - "Cannot remove the last owner from an organization", - ); - } - throw err; + const [target] = await sql<{ role: OrgRole }[]>` + select role + from ${sql.unsafe(schema)}.org_member + where org_id = ${orgId} and identity_id = ${identityId} + for update + `; + if (!target) return false; + + // Only block the transition that would orphan the org: an owner + // being changed to anything other than owner. + if (target.role === "owner" && newRole !== "owner") { + await assertAnotherOwnerExists(sql, schema, orgId, identityId); } + + const result = await sql` + update ${sql.unsafe(schema)}.org_member + set role = ${newRole} + where org_id = ${orgId} and identity_id = ${identityId} + `; + return result.count > 0; }); },