Skip to content

Commit c797473

Browse files
os-zhuangclaude
andauthored
fix(plugin-auth): organization/remove-member answers a permission denial as 403, not as the only-owner 400 (#8316)
* test(plugin-auth): failing repro for #8289 remove-member denial envelope Pins the measured defect before any fix: better-auth 1.7.0-rc.2's removeMember orders its owner-target branch AHEAD of hasPermission, so a non-owner caller gets 400 YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER instead of a permission refusal. 5 assertions red, 6 green (the must-not-break set). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 * fix(plugin-auth): remove-member answers its permission denial as 403 better-auth 1.7.0-rc.2 orders removeMember's 'only an owner may remove an owner' rule ahead of its real permission check and reports it with the sole-owner invariant's code and a 400. Answer the permission class in the global before-hook instead, with the 403 YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER envelope the sibling endpoints use. The guard stays silent on the self-removal path, so the genuine sole-owner invariant remains the vendor's; the permission half is decided by the vendor's own exported hasPermission, so there is no second spelling of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 * chore(changeset): remove-member permission denial answers 403 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 2a18012 commit c797473

5 files changed

Lines changed: 771 additions & 0 deletions

File tree

.changeset/tidy-pugs-invite.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
---
2+
'@objectstack/plugin-auth': patch
3+
---
4+
5+
`POST /api/v1/auth/organization/remove-member` now answers a permission denial
6+
as `403 YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER`, matching its sibling
7+
endpoints (`organization/update-member-role`, `organization/update`,
8+
`organization/delete`, `organization/invite-member`).
9+
10+
It previously answered `400 YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER`
11+
— a message whose every clause could be false at once: the caller was not
12+
leaving, was not an owner, and the organization could hold any number of owners.
13+
better-auth orders its "only an owner may remove an owner" rule ahead of the
14+
route's real permission check and reports it with the sole-owner invariant's
15+
code and status, so the invariant answered a question it was never asked. The
16+
removal itself was always correctly refused; only the response was wrong.
17+
18+
The genuine sole-owner refusal is unchanged and still fires when a sole owner
19+
removes themselves or calls `organization/leave`, and every legitimate
20+
owner-removes-owner / owner-removes-member path still returns `200`.

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

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@ import {
3434
isPlainMemberInvitation,
3535
isOrgAdminGrade,
3636
} from './invitation-role-cap.js';
37+
import {
38+
DEFAULT_CREATOR_ROLE,
39+
REMOVE_MEMBER_DENIAL_CODE,
40+
REMOVE_MEMBER_DENIAL_MESSAGE,
41+
isSoleOwnerGuardTerritory,
42+
removalBlockedByOwnerTarget,
43+
} from './remove-member-permission-guard.js';
3744
import { isPlaceholderEmail } from './placeholder-email.js';
3845
import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js';
3946
import type { TenancyService } from './tenancy-service.js';
@@ -835,6 +842,15 @@ async function smsQuotaExceededApiError(message: string): Promise<Error> {
835842
export class AuthManager {
836843
private auth: Auth<any> | null = null;
837844
private config: AuthManagerOptions;
845+
/**
846+
* [#8289] The org-role ac map handed to the `organization` plugin as `roles`
847+
* (`undefined` → the plugin runs on better-auth's `defaultRoles`). Stashed at
848+
* plugin-build time because `assertRemoveMemberPermitted` has to ask the
849+
* vendor's own `hasPermission` the same question the route will, and the
850+
* global before-hook runs BEFORE the org plugin shims `ctx.context.orgOptions`
851+
* into scope — so the map is not reachable from `ctx` at that point.
852+
*/
853+
private orgRolesMap: Record<string, any> | undefined;
838854
// ADR-0069 — cached "does any org require MFA" flag (per-org tightening).
839855
// Refreshed lazily with a TTL so isAuthGateActive() stays synchronous + cheap.
840856
private _orgMfaCache: { value: boolean; at: number } = { value: false, at: 0 };
@@ -1362,6 +1378,23 @@ export class AuthManager {
13621378
}
13631379
}
13641380

1381+
// ── #8289: remove-member answers its PERMISSION denial itself ──
1382+
// better-auth's `removeMember` orders "only an owner may remove an
1383+
// owner" AHEAD of its real permission check and reports it with the
1384+
// sole-owner invariant's code and a 400, so a caller who merely lacks
1385+
// permission is told they "cannot leave the organization as the only
1386+
// owner" — every clause false, and a 400 where every sibling denial is
1387+
// a 403. Answer the permission class here instead; the sole-owner
1388+
// invariant and every 200 path stay the vendor's, untouched.
1389+
// `remove-member-permission-guard.ts` carries the full reading,
1390+
// including why this MUST be a before-hook (an after-hook cannot
1391+
// change the status) and why the guard's refusal set is exactly the
1392+
// vendor's.
1393+
if (ctx?.path === '/organization/remove-member') {
1394+
await this.assertRemoveMemberPermitted(ctx);
1395+
// fall through — the vendor still re-decides everything it owns
1396+
}
1397+
13651398
// ── ADR-0024: admin-gate self-service SSO provider registration ──
13661399
// `@better-auth/sso`'s POST /sso/register only checks org-admin when
13671400
// `body.organizationId` is present (index.mjs: `if (ctx.body
@@ -2080,6 +2113,9 @@ export class AuthManager {
20802113
} catch {
20812114
customOrgRoles = undefined;
20822115
}
2116+
// [#8289] Same map, same request lifetime — see the field's doc for why
2117+
// the before-hook cannot read it back off `ctx`.
2118+
this.orgRolesMap = customOrgRoles;
20832119
return organization({
20842120
schema: buildOrganizationPluginSchema(),
20852121
// Enable the team sub-feature so the framework's `sys_team` /
@@ -4092,6 +4128,129 @@ export class AuthManager {
40924128
}
40934129
}
40944130

4131+
/**
4132+
* [#8289] Answer `/organization/remove-member`'s PERMISSION denial with the
4133+
* `403 YOU_ARE_NOT_ALLOWED_TO_*` envelope its siblings use, ahead of the
4134+
* vendor handler that would answer it with the sole-owner invariant's `400`.
4135+
*
4136+
* `remove-member-permission-guard.ts` carries the full reading of the vendor
4137+
* defect and of the two properties that make pre-empting it safe. The shape
4138+
* here follows from them:
4139+
*
4140+
* - **Silent on the sole-owner path.** A caller removing THEMSELVES while
4141+
* carrying the creator role is the one reading under which the vendor's
4142+
* message is true, so the guard returns and lets the vendor answer.
4143+
* - **FAIL-OPEN on anything unresolvable.** Unlike the `/sso/register` gate,
4144+
* this is not a security boundary — better-auth still enforces the whole
4145+
* policy after us, and refuses everything it refused before. The guard only
4146+
* RESTATES a refusal the vendor is already going to make, so a lookup that
4147+
* cannot be completed must fall back to today's behaviour (the vendor's own
4148+
* answer), never to an invented refusal. Failing closed here would turn an
4149+
* engine hiccup into a 403 on a legitimate owner's removal.
4150+
* - **The permission half is the vendor's own `hasPermission`**, called with
4151+
* the same roles map we hand the org plugin, so this never becomes a second
4152+
* spelling of the authorization question.
4153+
*/
4154+
private async assertRemoveMemberPermitted(ctx: any): Promise<void> {
4155+
const engine = this.getDataEngine();
4156+
if (!engine) return;
4157+
4158+
const memberIdOrEmail =
4159+
typeof ctx?.body?.memberIdOrEmail === 'string' ? ctx.body.memberIdOrEmail : '';
4160+
if (!memberIdOrEmail) return;
4161+
4162+
try {
4163+
const actor = await this.resolveActor(ctx);
4164+
// No resolvable session → better-auth's `sessionMiddleware` issues the
4165+
// 401. Not ours to pre-empt.
4166+
if (!actor?.userId) return;
4167+
4168+
const orgId =
4169+
(typeof ctx?.body?.organizationId === 'string' && ctx.body.organizationId) ||
4170+
actor.activeOrgId;
4171+
// No org in play → the vendor answers NO_ACTIVE_ORGANIZATION.
4172+
if (!orgId) return;
4173+
4174+
const sys = withSystemReadContext(engine);
4175+
4176+
const callerRow: any = await sys.findOne('sys_member', {
4177+
where: { organization_id: orgId, user_id: actor.userId },
4178+
});
4179+
if (!callerRow) return; // vendor answers MEMBER_NOT_FOUND
4180+
4181+
// Resolve the target exactly the way better-auth's org adapter does:
4182+
// an `@` means "by email" (lower-cased), anything else is a member id.
4183+
let targetRow: any = null;
4184+
if (memberIdOrEmail.includes('@')) {
4185+
const user: any = await sys.findOne('sys_user', {
4186+
where: { email: memberIdOrEmail.toLowerCase() },
4187+
});
4188+
if (user?.id) {
4189+
targetRow = await sys.findOne('sys_member', {
4190+
where: { organization_id: orgId, user_id: user.id },
4191+
});
4192+
}
4193+
} else {
4194+
targetRow = await sys.findOne('sys_member', { where: { id: memberIdOrEmail } });
4195+
}
4196+
if (!targetRow) return; // vendor answers MEMBER_NOT_FOUND
4197+
4198+
const creatorRole =
4199+
(typeof ctx?.context?.orgOptions?.creatorRole === 'string' &&
4200+
ctx.context.orgOptions.creatorRole) ||
4201+
DEFAULT_CREATOR_ROLE;
4202+
4203+
// (1) The sole-owner invariant's territory — never answer over it.
4204+
if (
4205+
isSoleOwnerGuardTerritory(
4206+
String(actor.userId),
4207+
String(targetRow.user_id ?? ''),
4208+
callerRow.role,
4209+
creatorRole,
4210+
)
4211+
) {
4212+
return;
4213+
}
4214+
4215+
// (2) The vendor's (3a) predicate: an owner target and a non-owner
4216+
// caller. A permission refusal — say so, with the right status.
4217+
if (removalBlockedByOwnerTarget(callerRow.role, targetRow.role, creatorRole)) {
4218+
const { APIError } = await import('better-auth/api');
4219+
throw new APIError('FORBIDDEN', {
4220+
message: REMOVE_MEMBER_DENIAL_MESSAGE,
4221+
code: REMOVE_MEMBER_DENIAL_CODE,
4222+
});
4223+
}
4224+
4225+
// (3) The vendor's (4): the real `member: ['delete']` check, decided by
4226+
// the vendor's own function so there is only ever one answer to it. Only
4227+
// the envelope differs — better-auth reports this one as 401.
4228+
const { hasPermission } = await import('better-auth/plugins/organization');
4229+
const permitted = await hasPermission(
4230+
{
4231+
role: callerRow.role,
4232+
options: (this.orgRolesMap ? { roles: this.orgRolesMap } : {}) as any,
4233+
permissions: { member: ['delete'] },
4234+
organizationId: orgId,
4235+
} as any,
4236+
ctx,
4237+
);
4238+
if (!permitted) {
4239+
const { APIError } = await import('better-auth/api');
4240+
throw new APIError('FORBIDDEN', {
4241+
message: REMOVE_MEMBER_DENIAL_MESSAGE,
4242+
code: REMOVE_MEMBER_DENIAL_CODE,
4243+
});
4244+
}
4245+
} catch (error) {
4246+
// Our own refusal must propagate; anything else is a lookup that did not
4247+
// complete, and per the fail-open contract above that hands the request
4248+
// back to better-auth unchanged.
4249+
const { isAPIError } = await import('better-auth/api');
4250+
if (isAPIError(error)) throw error;
4251+
}
4252+
}
4253+
40954254
/**
40964255
* [#3697] The issuer's own better-auth membership role in `orgId` — the
40974256
* input to the invitation role cap.

packages/plugins/plugin-auth/src/managed-extension-fields.test.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,6 +402,18 @@ const AUTH_MANAGER_PLUGINS: Record<string, { construct: () => unknown } | { skip
402402
oauthProvider: {
403403
construct: () => oauthProvider({ loginPage: '/login', consentPage: '/oauth/consent' }),
404404
},
405+
// [#8289] NOT a plugin factory — the scanner's regex cannot tell the two
406+
// apart, because both are a one-name destructure off `better-auth/plugins/*`.
407+
// `hasPermission` is the organization plugin's exported permission PREDICATE
408+
// (`(input, ctx) => Promise<boolean>`, `has-permission.mjs`); it declares no
409+
// schema, contributes no model and no column, so there is nothing here for
410+
// the collision loop to compare. `assertRemoveMemberPermitted` calls it so the
411+
// remove-member gate asks the vendor's own authorization question rather than
412+
// keeping a second spelling of it. The `stale` assertion below removes this
413+
// entry's licence the moment that import goes away.
414+
hasPermission: {
415+
skip: 'permission predicate exported by the organization plugin — declares no schema',
416+
},
405417
};
406418

407419
/** The plugin set the auth manager actually assembles (`buildPluginList()`). */

0 commit comments

Comments
 (0)