Skip to content

Commit e40aea8

Browse files
committed
fix(objectql, plugin-auth): the Default Organization's delete is counted before the first write (#24948)
Delta review 6308041940: - R1: once the last-administrator guard admits the Default Organization's delete, its record count runs in hooks.before too, leaving out only the members and invitations better-auth removes; a refusal is the same 409. - R2: the organization row counts run as the system with none of the caller's tenant scope, so a foreign tenantId cannot undercount. - R3: deciding whether the target is the Default Organization is outside the fall-through, so a failed lookup refuses. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gmigttoA9DyRNtz6SepWf
1 parent e184b51 commit e40aea8

7 files changed

Lines changed: 245 additions & 25 deletions

File tree

‎.changeset/24948-organization-delete-refuse-nonempty.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
fix(objectql, plugin-auth)!: deleting an organization that still owns records is refused with 409 and a per-table count, before anything is deleted; an empty one is deleted whole, its teams and organization-admin grants with it; the engine never clears a tenant column
77

8-
**The 18.0.0 defect this fixes.** `POST /api/v1/auth/organization/delete` ran better-auth's delete, which removes the organization's members, then its invitations, then the organization row, as three separate writes. The engine's delete cascade then applied the tenant column's `set_null` default to every row the organization still owned. On a database whose tenant column is NOT NULL (one that took the v18 organization-ownership ceremony), the delete answered 500 on `sys_team.organization_id` after the members and invitations were already gone, so the organization survived with no owner and nobody could delete it. On a database 18.0.0 created, where the column is still nullable, the delete answered 200 and left the organization's teams and records with `organization_id = NULL`, owned by nobody and visible to nobody. Every organization created through the door owns a default team, so on a NOT NULL database not even an empty organization could be deleted. Deployments that run `single` are not reached: their one organization keeps the last-administrator guard's 403, and they cannot create another.
8+
**The 18.0.0 defect this fixes.** `POST /api/v1/auth/organization/delete` ran better-auth's delete, which removes the organization's members, then its invitations, then the organization row, as three separate writes. The engine's delete cascade then applied the tenant column's `set_null` default to every row the organization still owned. On a database whose tenant column is NOT NULL (one that took the v18 organization-ownership ceremony), the delete answered 500 on `sys_team.organization_id` after the members and invitations were already gone, so the organization survived with no owner and nobody could delete it. On a database 18.0.0 created, where the column is still nullable, the delete answered 200 and left the organization's teams and records with `organization_id = NULL`, owned by nobody and visible to nobody. Every organization created through the door owns a default team, so on a NOT NULL database not even an empty organization could be deleted. Deployments that run `single` are not reached the same way: their one organization is the Default Organization, which the last-administrator guard answers first, and they cannot create another.
99

1010
**What happens now.**
1111

@@ -14,8 +14,9 @@ fix(objectql, plugin-auth)!: deleting an organization that still owns records is
1414
- Engine-owned rows (objects declared `managedBy: 'engine-owned'`) are treated the same way. The platform writes them as a consequence of records or activity, and an administrator cannot delete them over the data API: approval requests, approvers and tokens; automation runs; notifications, their receipts and deliveries; outgoing HTTP deliveries; mailboxes and mirrored mail; record shares and share links; import jobs; audience-binding suggestions; and organization-level settings values and their audit. They never refuse the delete, and they are kept, still naming the deleted organization.
1515
- So engine-owned and append-only rows outlive the organization, and go on naming it. Under tenant scoping no organization reads them.
1616
- An attribution column `organization_id` on an object whose tenancy is disabled is kept the same way: not counted, not cleared. A pointer to the organization, such as a session's or an API key's `active_organization_id`, is cleared as before.
17+
- The Default Organization (slug `default`, or the deployment's only organization, never under an organization wall) is asked the last-administrator guard's question first, and answers `403 PERMISSION_DENIED` while deleting it would leave no platform administrator. Once the guard admits, it is counted like any other organization, before better-auth deletes anything, except that only its members and invitations are left out: its teams and grants are not removed, so they count, and the delete is refused `409 DELETE_RESTRICTED` while it holds them.
1718
- An organization that owns nothing but its own auth structure is deleted whole: its members and invitations (better-auth), and its teams, their members and the organization-admin grants its owners and admins hold (plugin-auth, from `beforeDeleteOrganization`). Any other grant scoped to the organization counts as a record.
18-
- The request runs as one engine transaction and rolls back on any error answer, so a delete either removes all of that or none of it. On a datasource whose driver has no transactions, the refusals are the guarantee, because both run before the first write: the record count above, and, for the Default Organization, the last-administrator guard, whose `403` is now asked before better-auth deletes any member (it used to arrive after the members and invitations were gone).
19+
- The request runs as one engine transaction and rolls back on any error answer, so a delete either removes all of that or none of it. On a datasource whose driver has no transactions, the refusals are the guarantee, because all of them run before the first write: the record count above, and, for the Default Organization, the last-administrator guard, whose `403` is now asked before better-auth deletes any member (it used to arrive after the members and invitations were gone).
1920
- Under all of this, the engine itself refuses to delete a `sys_organization` row while any row still names it in its tenant column, whatever path reaches the delete: `409 DELETE_RESTRICTED` with the same `dependents` list, which better-auth's door now carries to the wire too. It counts each table rather than reading its rows. An organization is deleted by id only: a delete matching organizations by a filter (`multi: true`) is refused with `409 DELETE_RESTRICTED` naming the by-id route, because only the by-id delete runs this referential pass. A tenant column is never set to NULL.
2021
- `ObjectQL.organizationOwnedRowCounts(organizationId, { exclude })` answers, per object, how many rows name an organization in their tenant column. It is the question the refusal asks.
2122

‎packages/objectql/src/engine-organization-delete-refusal.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,48 @@ describe('[#24948] references the delete keeps as they are (seat decision 630599
388388
});
389389
});
390390

391+
describe('[#24948] the organization counts carry none of the caller\'s tenant scope (delta review R2)', () => {
392+
/**
393+
* A tenant layer: scopes every `acct` read whose context carries a
394+
* `tenantId` to that organization, as a tenant-scoping middleware does. A caller acting in
395+
* OTHER_ORG must not read 0 rows for ORG_ID and pass the refusal.
396+
*/
397+
const scopeByTenant = (engine: ObjectQL) =>
398+
engine.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
399+
const tenantId = opCtx.context?.tenantId;
400+
if (tenantId && opCtx.object === 'acct' && opCtx.ast) {
401+
opCtx.ast.where = { ...(opCtx.ast.where ?? {}), organization_id: tenantId };
402+
}
403+
await next();
404+
});
405+
const foreign = { userId: 'u1', tenantId: OTHER_ORG } as any;
406+
407+
it('organizationOwnedRowCounts counts the organization\'s rows under a foreign tenant scope', async () => {
408+
const { engine, store } = await makeEngine([ORGANIZATION, ACCOUNT]);
409+
scopeByTenant(engine);
410+
store.seed('acct', { id: 'a1', organization_id: ORG_ID });
411+
store.seed('acct', { id: 'a2', organization_id: ORG_ID });
412+
// CONTROL: the tenant layer is live, and scopes this caller's own count away.
413+
expect(await engine.count('acct', { where: { organization_id: ORG_ID }, context: foreign })).toBe(0);
414+
415+
expect(await engine.organizationOwnedRowCounts(ORG_ID, { context: foreign })).toEqual([{ object: 'acct', count: 2 }]);
416+
});
417+
418+
it('the engine refusal still refuses a caller scoped to another organization', async () => {
419+
const { engine, store } = await makeEngine([ORGANIZATION, ACCOUNT]);
420+
scopeByTenant(engine);
421+
store.seed('acct', { id: 'a1', organization_id: ORG_ID });
422+
423+
const err: any = await engine
424+
.delete('sys_organization', { where: { id: ORG_ID }, context: { ...foreign, isSystem: true } } as any)
425+
.then(() => null, (e: any) => e);
426+
427+
expect(err?.code).toBe('DELETE_RESTRICTED');
428+
expect(err?.dependents?.map((d: { object: string }) => d.object)).toEqual(['acct']);
429+
expect(store.writes).toEqual([]);
430+
});
431+
});
432+
391433
describe('[#24948] the predicate path is refused for an organization', () => {
392434
it('a multi delete of sys_organization is refused, by id only, before anything is written', async () => {
393435
const { engine, store } = await makeEngine([ORGANIZATION, ACCOUNT]);

‎packages/objectql/src/engine.ts‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20090,13 +20090,30 @@ export class ObjectQL implements IObjectQLEngine {
2009020090
return { where, count: probed.filter((row) => !cascadeSetHas(cascadeSet.members, childName, row?.id)).length };
2009120091
}
2009220092
try {
20093-
return { where, count: await this.count(childName, { where, context: ObjectQL.referenceCheckContext(context) }) };
20093+
return { where, count: await this.count(childName, { where, context: ObjectQL.organizationCountContext(context) }) };
2009420094
} catch (error) {
2009520095
if (isMissingTableError(error, childName)) return { where, count: 0 };
2009620096
throw error;
2009720097
}
2009820098
}
2009920099

20100+
/**
20101+
* [#24948] The context an organization's row counts run under: system
20102+
* identity, and nothing of the caller's scope. A `tenantId` carried in from
20103+
* a caller acting in another organization would scope the count to THAT
20104+
* organization and read 0 rows naming this one, so the refusal would pass on
20105+
* an undercount. Only the request's transaction and trace are kept: the
20106+
* count must see the request's own uncommitted writes. The ordinary
20107+
* reference probes keep the caller's scope ({@link referenceCheckContext}).
20108+
*/
20109+
private static organizationCountContext(context?: ExecutionContext): ExecutionContext {
20110+
return {
20111+
isSystem: true,
20112+
...(context?.transaction !== undefined ? { transaction: context.transaction } : {}),
20113+
...(context?.traceId !== undefined ? { traceId: context.traceId } : {}),
20114+
} as ExecutionContext;
20115+
}
20116+
2010020117
/**
2010120118
* [#24948] May the refusal disclose `count` to the caller? Only when the
2010220119
* caller's own identity counts the same rows: the elevated count read rows
@@ -20222,7 +20239,7 @@ export class ObjectQL implements IObjectQLEngine {
2022220239
try {
2022320240
count = await this.count(childName, {
2022420241
where: this.referenceProbeFilter(tenantField, fdef, organizationId),
20225-
context: ObjectQL.referenceCheckContext(options?.context),
20242+
context: ObjectQL.organizationCountContext(options?.context),
2022620243
});
2022720244
} catch (error) {
2022820245
if (isMissingTableError(error, childName)) continue;

‎packages/plugins/plugin-auth/src/auth-manager.ts‎

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,7 @@ import {
212212
import {
213213
ORGANIZATION_DELETE_PATH,
214214
ORGANIZATION_DELETE_REFUSAL_CODE,
215+
defaultOrganizationDeleteRefusal,
215216
isDefaultOrganizationTarget,
216217
organizationDeleteRefusal,
217218
organizationNotEmptyBody,
@@ -8112,9 +8113,11 @@ export class AuthManager {
81128113
* refusal, and no count. Everything this cannot decide falls through to the
81138114
* vendor: no session, no organization id, no membership, a lookup that
81148115
* failed. The COUNT does not fall through: it is the guard, and a guard that
8115-
* could not be evaluated must not pass, so its failure propagates. The
8116-
* Default Organization stays the last-administrator guard's to answer: the
8117-
* guard's own question is asked here, so its 403 lands before the first write.
8116+
* could not be evaluated must not pass, so its failure propagates, and so
8117+
* does a failure deciding whether the target is the Default Organization.
8118+
* The Default Organization is asked the last-administrator guard's question
8119+
* first (a 403), then its own record count (the same 409), both here, before
8120+
* the first write.
81188121
*/
81198122
private async assertOrganizationDeletable(ctx: any): Promise<void> {
81208123
const engine = this.getDataEngine();
@@ -8123,7 +8126,6 @@ export class AuthManager {
81238126
if (!organizationId) return;
81248127

81258128
let organization: any;
8126-
let defaultOrganization = false;
81278129
try {
81288130
const actor = await this.resolveActor(ctx);
81298131
if (!actor?.userId) return; // the vendor answers 401
@@ -8145,34 +8147,42 @@ export class AuthManager {
81458147
if (!permitted) return; // the vendor answers YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_ORGANIZATION
81468148
organization = await sys.findOne('sys_organization', { where: { id: organizationId } });
81478149
if (!organization) return; // the vendor answers 400
8148-
defaultOrganization = await isDefaultOrganizationTarget(engine, organization, postureEnforcesWall(resolveTenancyPosture()));
81498150
} catch {
81508151
// A lookup that did not complete hands the request back to the vendor,
81518152
// which re-reads everything it decides; the engine's own refusal still
81528153
// stands under the delete.
81538154
return;
81548155
}
81558156

8156-
// The Default Organization is the last-administrator guard's to answer,
8157-
// and it answers HERE, before the vendor's first write, so its refusal
8157+
// Which of the two refusals below applies is part of the guard, so it is
8158+
// decided outside the fall-through above: a lookup that fails here
8159+
// refuses, never hands the request to the vendor unasked.
8160+
const defaultOrganization = await isDefaultOrganizationTarget(
8161+
engine, organization, postureEnforcesWall(resolveTenancyPosture()),
8162+
);
8163+
8164+
// The Default Organization is asked the last-administrator guard's
8165+
// question first, HERE, before the vendor's first write, so its refusal
81588166
// leaves every member and invitation as it was on any driver, with or
81598167
// without transactions. The guard fails closed on a lookup it cannot
81608168
// complete, so this never passes for want of an answer. When it admits
8161-
// the delete, the vendor proceeds and the engine's own refusal still holds
8162-
// while anything names the organization.
8163-
if (defaultOrganization) {
8164-
if (!this.organizationDeleteStandingJudge) return;
8169+
// the delete, the record count below is asked before the vendor's first
8170+
// write too, leaving out only what the vendor removes (members and
8171+
// invitations): plugin-auth removes none of the Default Organization's
8172+
// structure, so its teams and grants count.
8173+
if (defaultOrganization && this.organizationDeleteStandingJudge) {
81658174
try {
81668175
await this.organizationDeleteStandingJudge(organizationId);
81678176
} catch (err) {
81688177
if ((err as { code?: unknown } | null)?.code !== 'PERMISSION_DENIED') throw err;
81698178
const { APIError } = await import('better-auth/api');
81708179
throw new APIError('FORBIDDEN', { message: (err as Error).message, code: 'PERMISSION_DENIED' });
81718180
}
8172-
return;
81738181
}
81748182

8175-
const owned = await organizationDeleteRefusal(engine, organizationId);
8183+
const owned = defaultOrganization
8184+
? await defaultOrganizationDeleteRefusal(engine, organizationId)
8185+
: await organizationDeleteRefusal(engine, organizationId);
81768186
if (owned && owned.length > 0) {
81778187
const { APIError } = await import('better-auth/api');
81788188
throw new APIError('CONFLICT', { code: ORGANIZATION_DELETE_REFUSAL_CODE, ...organizationNotEmptyBody(owned) });

‎packages/plugins/plugin-auth/src/organization-delete.test.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import { ORGANIZATION_ADMIN, ORGANIZATION_ADMIN_NO_BYPASS } from '@objectstack/s
1616
import {
1717
ORGANIZATION_AUTH_STRUCTURE,
1818
ORGANIZATION_DELETE_REFUSAL_CODE,
19+
VENDOR_REMOVED_ON_DELETE,
20+
defaultOrganizationDeleteRefusal,
1921
isDefaultOrganizationTarget,
2022
organizationDeleteRefusal,
2123
organizationNotEmptyBody,
@@ -105,6 +107,28 @@ describe('[#24948] organizationDeleteRefusal: the records the organization still
105107
});
106108
});
107109

110+
describe('[#24948] defaultOrganizationDeleteRefusal: the Default Organization\'s count, once the guard admits', () => {
111+
it('leaves out only what better-auth removes, so its teams and every grant count', async () => {
112+
const owned = [{ object: 'sys_team', count: 1 }, { object: 'sys_user_permission_set', count: 2 }];
113+
const { engine, asked, calls } = makeEngine({ sys_user_permission_set: [{ organization_id: ORG }] }, owned);
114+
expect(await defaultOrganizationDeleteRefusal(engine, ORG)).toEqual(owned);
115+
expect(asked).toEqual([{ organizationId: ORG, exclude: ['sys_member', 'sys_invitation'] }]);
116+
expect([...VENDOR_REMOVED_ON_DELETE]).toEqual(['sys_member', 'sys_invitation']);
117+
// No organization-admin grant is subtracted: plugin-auth removes none of them here.
118+
expect(calls).toEqual([]);
119+
});
120+
121+
it('answers undefined for an engine that cannot count, and propagates a count that fails', async () => {
122+
const { engine } = makeEngine({});
123+
delete engine.organizationOwnedRowCounts;
124+
expect(await defaultOrganizationDeleteRefusal(engine, ORG)).toBeUndefined();
125+
const failing = makeEngine({}).engine;
126+
const fault = new Error('connection lost');
127+
failing.organizationOwnedRowCounts = async () => { throw fault; };
128+
await expect(defaultOrganizationDeleteRefusal(failing, ORG)).rejects.toBe(fault);
129+
});
130+
});
131+
108132
describe('[#24948] organizationNotEmptyBody: the 409 body', () => {
109133
it('names every table with its count and the remedy, beside the standard DELETE_RESTRICTED code', () => {
110134
const body = organizationNotEmptyBody([

0 commit comments

Comments
 (0)