Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/accounts/db.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

// ---------------------------------------------------------------------------
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
4 changes: 4 additions & 0 deletions packages/accounts/migrate/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
99 changes: 67 additions & 32 deletions packages/accounts/ops/org-member.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
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) {
Expand Down Expand Up @@ -63,21 +90,25 @@ export function orgMemberOps(ctx: AccountsContext) {

async removeMember(orgId: string, identityId: string): Promise<boolean> {
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;
});
},

Expand All @@ -87,22 +118,26 @@ export function orgMemberOps(ctx: AccountsContext) {
newRole: OrgRole,
): Promise<boolean> {
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;
});
},

Expand Down