From bf89869dabbd18cae2f9b8c8e33843c9970d516f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 04:41:01 +0000 Subject: [PATCH 1/3] 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 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- .../remove-member-permission-guard.test.ts | 398 ++++++++++++++++++ 1 file changed, 398 insertions(+) create mode 100644 packages/plugins/plugin-auth/src/remove-member-permission-guard.test.ts diff --git a/packages/plugins/plugin-auth/src/remove-member-permission-guard.test.ts b/packages/plugins/plugin-auth/src/remove-member-permission-guard.test.ts new file mode 100644 index 0000000000..c6e8f324f1 --- /dev/null +++ b/packages/plugins/plugin-auth/src/remove-member-permission-guard.test.ts @@ -0,0 +1,398 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#8289] Regression suite for "`organization/remove-member` answers a +// PERMISSION denial with `400 YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER`". +// +// ## Where the wrong answer is minted (measured, not assumed) +// +// NOT in our packages. better-auth `1.7.0-rc.2`, +// `dist/plugins/organization/routes/crud-members.mjs`, the `removeMember` +// handler, runs its checks in this order: +// +// 1. resolve the caller's own member row → 400 MEMBER_NOT_FOUND +// 2. resolve the target member row → 400 MEMBER_NOT_FOUND +// 3. `if (targetRoles.includes('owner')) {` +// a. `if (!callerRoles.includes('owner'))` +// throw 400 YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER ← the defect +// b. `if (ownerCount <= 1)` +// throw 400 YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER ← genuine +// `}` +// 4. `hasPermission({ member: ['delete'] })` +// → throw 401 UNAUTHORIZED YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER +// +// Step 3a is a PERMISSION rule ("only an owner may remove an owner") wearing the +// only-owner invariant's error code and status. It is also ordered AHEAD of the +// real permission check at step 4, so for any caller who is not an owner and any +// target who is, the permission check never runs and the invariant answers a +// question it was never asked. That is the filer's step 1 and step 2 exactly — +// and it is why adding a second owner does not change the answer: step 3a +// short-circuits before the owner COUNT at 3b is ever consulted. +// +// Step 4 carries a second, smaller parity defect: `UNAUTHORIZED` (401), where +// every sibling denial (`update-member-role`, `organization/update`, +// `organization/delete`) uses `FORBIDDEN` (403). +// +// ## The fix shape this pins +// +// We do NOT fork better-auth. `remove-member-permission-guard.ts` answers the +// permission class ITSELF, in the global before-hook, ahead of the vendor +// handler — so the two vendor branches above become unreachable for exactly the +// inputs they get wrong, while the genuine sole-owner invariant at 3b and every +// legitimate 200 path stay with the vendor, untouched. +// +// Real better-auth pipeline throughout, through a real `AuthManager` — the +// #5233 / #7725 precedent. The 400 in the field came out of the mounted route, +// so the mounted route is what has to answer here. + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { AuthManager } from './auth-manager'; + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const BASE = 'http://localhost:3000'; +const ORG = 'org_acme'; +const PASSWORD = 'S3cure!Passw0rd-8289'; + +/** + * The same minimal in-memory `IDataEngine` double the other end-to-end + * auth-manager suites use, with `delete` pinned to ObjectQL's own dispatch + * predicate ({@link assertEngineDeleteDispatch}) rather than a hand-written + * copy — a fake that accepts a call the real engine refuses is how a dead route + * ships with its suite green (#4550). + */ +const createMemoryEngine = () => { + const tables = new Map(); + const rows = (name: string) => { + if (!tables.has(name)) tables.set(name, []); + return tables.get(name)!; + }; + const eq = (a: any, b: any) => + a instanceof Date || b instanceof Date + ? new Date(a as any).getTime() === new Date(b as any).getTime() + : a === b; + const matches = (row: any, where: Record = {}) => + Object.entries(where).every(([k, v]) => { + const actual = row[k]; + if (v && typeof v === 'object' && !Array.isArray(v) && !(v instanceof Date)) { + if ('$ne' in v) return !eq(actual, v.$ne); + if ('$in' in v) return (v.$in as any[]).some((x) => eq(actual, x)); + } + return eq(actual, v); + }); + const project = (row: any, fields?: string[]) => { + if (!Array.isArray(fields) || fields.length === 0) return { ...row }; + const out: any = {}; + for (const f of ['id', ...fields]) if (f in row) out[f] = row[f]; + return out; + }; + let seq = 0; + return { + tables, + async insert(name: string, data: any) { + const row = { id: data.id ?? `row_${++seq}`, ...data }; + rows(name).push(row); + return { ...row }; + }, + async findOne(name: string, q: any = {}) { + const row = rows(name).find((r) => matches(r, q.where)); + return row ? project(row, q.fields) : null; + }, + async find(name: string, q: any = {}) { + let out = rows(name).filter((r) => matches(r, q.where)); + if (q.offset) out = out.slice(q.offset); + if (q.limit) out = out.slice(0, q.limit); + return out.map((r) => project(r, q.fields)); + }, + async count(name: string, q: any = {}) { + return rows(name).filter((r) => matches(r, q.where)).length; + }, + async update(name: string, patch: any) { + const row = rows(name).find((r) => r.id === patch.id); + if (!row) return null; + Object.assign(row, patch); + return { ...row }; + }, + async delete(name: string, q: any = {}) { + assertEngineDeleteDispatch(q); + const table = rows(name); + const keep = table.filter((r) => !matches(r, q.where)); + tables.set(name, keep); + return table.length - keep.length; + }, + }; +}; + +type MemoryEngine = ReturnType; + +const makeManager = (engine: MemoryEngine) => + new AuthManager({ + secret: SECRET, + baseUrl: BASE, + dataEngine: engine as any, + plugins: { organization: true }, + } as any); + +const cookieFrom = (response: Response): string => + (response.headers.getSetCookie?.() ?? [response.headers.get('set-cookie') ?? '']) + .map((c) => c.split(';')[0]) + .filter(Boolean) + .join('; '); + +const post = (manager: AuthManager, path: string, body: unknown, cookie?: string) => + manager.handleRequest( + new Request(`${BASE}/api/v1/auth${path}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...(cookie ? { cookie } : {}) }, + body: JSON.stringify(body), + }), + ); + +const signUp = async (manager: AuthManager, engine: MemoryEngine, email: string) => { + const res = await post(manager, '/sign-up/email', { email, password: PASSWORD, name: email }); + expect(res.status, await res.clone().text()).toBe(200); + const user = (engine.tables.get('sys_user') ?? []).find((u) => u.email === email); + expect(user, `sign-up did not create ${email}`).toBeDefined(); + return { cookie: cookieFrom(res), userId: String(user!.id), email }; +}; + +/** The membership rows for an org, as the database holds them. */ +const membersOf = (engine: MemoryEngine, organizationId = ORG) => + (engine.tables.get('sys_member') ?? []).filter((m) => m.organization_id === organizationId); + +/** + * The filer's fixture: a workspace whose roles are seeded directly, the way an + * operator's data looks. Returns the actors by the names the issue uses. + */ +const bootWorkspace = async (roles: Record) => { + const engine = createMemoryEngine(); + const manager = makeManager(engine); + await engine.insert('sys_organization', { id: ORG, name: 'Acme', slug: 'acme' }); + + const actors: Record = {}; + let n = 0; + for (const [who, role] of Object.entries(roles)) { + const actor = await signUp(manager, engine, `${who}@example.com`); + actors[who] = actor; + await engine.insert('sys_member', { + id: `mem_${++n}`, + organization_id: ORG, + user_id: actor.userId, + role, + created_at: new Date(), + }); + } + return { engine, manager, actors }; +}; + +/** `POST organization/remove-member`, exactly as the console makes it. */ +const removeMember = ( + manager: AuthManager, + cookie: string, + memberIdOrEmail: string, +) => post(manager, '/organization/remove-member', { memberIdOrEmail, organizationId: ORG }, cookie); + +const answer = async (response: Response) => { + const body = (await response.clone().json().catch(() => null)) as any; + return { status: response.status, code: body?.code ?? null, message: body?.message ?? null }; +}; + +beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => vi.restoreAllMocks()); + +describe('#8289 organization/remove-member — a permission denial answers as one', () => { + // ── The defect, exactly as filed ──────────────────────────────────────── + it("step 1: a plain member removing the owner is refused as a PERMISSION denial, not as 'only owner'", async () => { + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + zhangsan: 'member', + }); + + const res = await removeMember(manager, actors.zhangsan!.cookie, actors.lisi!.email); + + expect(await answer(res)).toEqual({ + status: 403, + code: 'YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER', + message: 'You are not allowed to delete this member', + }); + // The refusal is real — it always was; this card is about the ANSWER. + expect(membersOf(engine)).toHaveLength(2); + }); + + it('step 2: adding a SECOND owner does not change the answer — the reason was never the owner count', async () => { + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + outsider: 'owner', + zhangsan: 'member', + }); + + const res = await removeMember(manager, actors.zhangsan!.cookie, actors.outsider!.email); + + // Before the fix this returned the only-owner 400 even though TWO owners + // exist — the clause was false and the status was wrong at the same time. + expect(await answer(res)).toEqual({ + status: 403, + code: 'YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER', + message: 'You are not allowed to delete this member', + }); + expect(membersOf(engine)).toHaveLength(3); + }); + + it('a plain member removing another plain member is refused as 403, not 401', async () => { + // The vendor reaches its OWN permission check here and answers it with + // `UNAUTHORIZED` (401) — right code, wrong status, and 401 tells a client + // "you are not signed in" when they plainly are. + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + zhangsan: 'member', + wangwu: 'member', + }); + + const res = await removeMember(manager, actors.zhangsan!.cookie, actors.wangwu!.email); + + expect(await answer(res)).toEqual({ + status: 403, + code: 'YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER', + message: 'You are not allowed to delete this member', + }); + expect(membersOf(engine)).toHaveLength(3); + }); + + it('an ADMIN removing an owner is refused as a permission denial too', async () => { + // An admin passes better-auth's `member: ['delete']` permission but is not + // an owner, so it is branch 3a's other inhabitant — and the one a + // guard written as "plain members may not remove" would miss. + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + admin: 'admin', + }); + + const res = await removeMember(manager, actors.admin!.cookie, actors.lisi!.email); + + expect(await answer(res)).toEqual({ + status: 403, + code: 'YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER', + message: 'You are not allowed to delete this member', + }); + expect(membersOf(engine)).toHaveLength(2); + }); + + // ── The invariant that must NOT be masked ─────────────────────────────── + it('the genuine sole-owner refusal still fires on the self-removal path', async () => { + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + zhangsan: 'member', + }); + + const res = await removeMember(manager, actors.lisi!.cookie, actors.lisi!.email); + + // Unchanged, and still the vendor's — this one is TRUE: the caller is + // leaving, is an owner, and is the only owner. + expect(await answer(res)).toEqual({ + status: 400, + code: 'YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER', + message: 'You cannot leave the organization as the only owner', + }); + expect(membersOf(engine)).toHaveLength(2); + }); + + it('the sole-owner guard also still fires through organization/leave', async () => { + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + zhangsan: 'member', + }); + + const res = await post(manager, '/organization/leave', { organizationId: ORG }, actors.lisi!.cookie); + + expect(await answer(res)).toEqual({ + status: 400, + code: 'YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER', + message: 'You cannot leave the organization as the only owner', + }); + expect(membersOf(engine)).toHaveLength(2); + }); + + it('an owner who is NOT the last owner may still leave', async () => { + // The other side of the invariant: the guard is about the LAST owner, so a + // second owner leaving must succeed. A remap that swallowed the 400 would + // not show up here, but a guard that over-refuses would. + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + outsider: 'owner', + }); + + const res = await post(manager, '/organization/leave', { organizationId: ORG }, actors.outsider!.cookie); + + expect(res.status, await res.clone().text()).toBe(200); + expect(membersOf(engine)).toHaveLength(1); + }); + + // ── The legitimate paths, still 200 ───────────────────────────────────── + it('step 4a: an owner may remove the other owner', async () => { + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + outsider: 'owner', + zhangsan: 'member', + }); + + const res = await removeMember(manager, actors.lisi!.cookie, actors.outsider!.email); + + expect(res.status, await res.clone().text()).toBe(200); + expect(membersOf(engine).map((m) => m.role).sort()).toEqual(['member', 'owner']); + }); + + it('step 4b: an owner may remove a plain member', async () => { + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + zhangsan: 'member', + }); + + const res = await removeMember(manager, actors.lisi!.cookie, actors.zhangsan!.email); + + expect(res.status, await res.clone().text()).toBe(200); + expect(membersOf(engine).map((m) => m.role)).toEqual(['owner']); + }); + + it('an admin may still remove a plain member', async () => { + // The guard answers only the owner-target branch, so an admin's ordinary + // authority is untouched — proof it did not over-refuse. + const { manager, engine, actors } = await bootWorkspace({ + lisi: 'owner', + admin: 'admin', + zhangsan: 'member', + }); + + const res = await removeMember(manager, actors.admin!.cookie, actors.zhangsan!.email); + + expect(res.status, await res.clone().text()).toBe(200); + expect(membersOf(engine).map((m) => m.role).sort()).toEqual(['admin', 'owner']); + }); + + // ── The parity target the card names ──────────────────────────────────── + it('answers in the same envelope shape as its sibling update-member-role', async () => { + const { manager, actors, engine } = await bootWorkspace({ + lisi: 'owner', + zhangsan: 'member', + }); + const [ownerRow] = membersOf(engine).filter((m) => m.user_id === actors.lisi!.userId); + + const sibling = await post( + manager, + '/organization/update-member-role', + { memberId: String(ownerRow!.id), role: 'member', organizationId: ORG }, + actors.zhangsan!.cookie, + ); + const removal = await removeMember(manager, actors.zhangsan!.cookie, actors.lisi!.email); + + const siblingAnswer = await answer(sibling); + const removalAnswer = await answer(removal); + + // The parity the issue asks for: same status, same code FAMILY. + expect(siblingAnswer.status).toBe(403); + expect(siblingAnswer.code).toBe('YOU_ARE_NOT_ALLOWED_TO_UPDATE_THIS_MEMBER'); + expect(removalAnswer.status).toBe(siblingAnswer.status); + expect(removalAnswer.code).toMatch(/^YOU_ARE_NOT_ALLOWED_TO_/); + }); +}); From f5ddb067e6bd72a63019815aed2c7b033de28044 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 04:50:24 +0000 Subject: [PATCH 2/3] 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 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- .../plugins/plugin-auth/src/auth-manager.ts | 159 ++++++++++++++++++ .../src/managed-extension-fields.test.ts | 12 ++ .../remove-member-permission-guard.test.ts | 41 ++++- .../src/remove-member-permission-guard.ts | 155 +++++++++++++++++ 4 files changed, 360 insertions(+), 7 deletions(-) create mode 100644 packages/plugins/plugin-auth/src/remove-member-permission-guard.ts diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index ca2a2d7a93..ba62d4024c 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -34,6 +34,13 @@ import { isPlainMemberInvitation, isOrgAdminGrade, } from './invitation-role-cap.js'; +import { + DEFAULT_CREATOR_ROLE, + REMOVE_MEMBER_DENIAL_CODE, + REMOVE_MEMBER_DENIAL_MESSAGE, + isSoleOwnerGuardTerritory, + removalBlockedByOwnerTarget, +} from './remove-member-permission-guard.js'; import { isPlaceholderEmail } from './placeholder-email.js'; import { reconcileMembership, type MembershipPolicy } from './reconcile-membership.js'; import type { TenancyService } from './tenancy-service.js'; @@ -835,6 +842,15 @@ async function smsQuotaExceededApiError(message: string): Promise { export class AuthManager { private auth: Auth | null = null; private config: AuthManagerOptions; + /** + * [#8289] The org-role ac map handed to the `organization` plugin as `roles` + * (`undefined` → the plugin runs on better-auth's `defaultRoles`). Stashed at + * plugin-build time because `assertRemoveMemberPermitted` has to ask the + * vendor's own `hasPermission` the same question the route will, and the + * global before-hook runs BEFORE the org plugin shims `ctx.context.orgOptions` + * into scope — so the map is not reachable from `ctx` at that point. + */ + private orgRolesMap: Record | undefined; // ADR-0069 — cached "does any org require MFA" flag (per-org tightening). // Refreshed lazily with a TTL so isAuthGateActive() stays synchronous + cheap. private _orgMfaCache: { value: boolean; at: number } = { value: false, at: 0 }; @@ -1362,6 +1378,23 @@ export class AuthManager { } } + // ── #8289: remove-member answers its PERMISSION denial itself ── + // better-auth's `removeMember` orders "only an owner may remove an + // owner" AHEAD of its real permission check and reports it with the + // sole-owner invariant's code and a 400, so a caller who merely lacks + // permission is told they "cannot leave the organization as the only + // owner" — every clause false, and a 400 where every sibling denial is + // a 403. Answer the permission class here instead; the sole-owner + // invariant and every 200 path stay the vendor's, untouched. + // `remove-member-permission-guard.ts` carries the full reading, + // including why this MUST be a before-hook (an after-hook cannot + // change the status) and why the guard's refusal set is exactly the + // vendor's. + if (ctx?.path === '/organization/remove-member') { + await this.assertRemoveMemberPermitted(ctx); + // fall through — the vendor still re-decides everything it owns + } + // ── ADR-0024: admin-gate self-service SSO provider registration ── // `@better-auth/sso`'s POST /sso/register only checks org-admin when // `body.organizationId` is present (index.mjs: `if (ctx.body @@ -2080,6 +2113,9 @@ export class AuthManager { } catch { customOrgRoles = undefined; } + // [#8289] Same map, same request lifetime — see the field's doc for why + // the before-hook cannot read it back off `ctx`. + this.orgRolesMap = customOrgRoles; return organization({ schema: buildOrganizationPluginSchema(), // Enable the team sub-feature so the framework's `sys_team` / @@ -4092,6 +4128,129 @@ export class AuthManager { } } + /** + * [#8289] Answer `/organization/remove-member`'s PERMISSION denial with the + * `403 YOU_ARE_NOT_ALLOWED_TO_*` envelope its siblings use, ahead of the + * vendor handler that would answer it with the sole-owner invariant's `400`. + * + * `remove-member-permission-guard.ts` carries the full reading of the vendor + * defect and of the two properties that make pre-empting it safe. The shape + * here follows from them: + * + * - **Silent on the sole-owner path.** A caller removing THEMSELVES while + * carrying the creator role is the one reading under which the vendor's + * message is true, so the guard returns and lets the vendor answer. + * - **FAIL-OPEN on anything unresolvable.** Unlike the `/sso/register` gate, + * this is not a security boundary — better-auth still enforces the whole + * policy after us, and refuses everything it refused before. The guard only + * RESTATES a refusal the vendor is already going to make, so a lookup that + * cannot be completed must fall back to today's behaviour (the vendor's own + * answer), never to an invented refusal. Failing closed here would turn an + * engine hiccup into a 403 on a legitimate owner's removal. + * - **The permission half is the vendor's own `hasPermission`**, called with + * the same roles map we hand the org plugin, so this never becomes a second + * spelling of the authorization question. + */ + private async assertRemoveMemberPermitted(ctx: any): Promise { + const engine = this.getDataEngine(); + if (!engine) return; + + const memberIdOrEmail = + typeof ctx?.body?.memberIdOrEmail === 'string' ? ctx.body.memberIdOrEmail : ''; + if (!memberIdOrEmail) return; + + try { + const actor = await this.resolveActor(ctx); + // No resolvable session → better-auth's `sessionMiddleware` issues the + // 401. Not ours to pre-empt. + if (!actor?.userId) return; + + const orgId = + (typeof ctx?.body?.organizationId === 'string' && ctx.body.organizationId) || + actor.activeOrgId; + // No org in play → the vendor answers NO_ACTIVE_ORGANIZATION. + if (!orgId) return; + + const sys = withSystemReadContext(engine); + + const callerRow: any = await sys.findOne('sys_member', { + where: { organization_id: orgId, user_id: actor.userId }, + }); + if (!callerRow) return; // vendor answers MEMBER_NOT_FOUND + + // Resolve the target exactly the way better-auth's org adapter does: + // an `@` means "by email" (lower-cased), anything else is a member id. + let targetRow: any = null; + if (memberIdOrEmail.includes('@')) { + const user: any = await sys.findOne('sys_user', { + where: { email: memberIdOrEmail.toLowerCase() }, + }); + if (user?.id) { + targetRow = await sys.findOne('sys_member', { + where: { organization_id: orgId, user_id: user.id }, + }); + } + } else { + targetRow = await sys.findOne('sys_member', { where: { id: memberIdOrEmail } }); + } + if (!targetRow) return; // vendor answers MEMBER_NOT_FOUND + + const creatorRole = + (typeof ctx?.context?.orgOptions?.creatorRole === 'string' && + ctx.context.orgOptions.creatorRole) || + DEFAULT_CREATOR_ROLE; + + // (1) The sole-owner invariant's territory — never answer over it. + if ( + isSoleOwnerGuardTerritory( + String(actor.userId), + String(targetRow.user_id ?? ''), + callerRow.role, + creatorRole, + ) + ) { + return; + } + + // (2) The vendor's (3a) predicate: an owner target and a non-owner + // caller. A permission refusal — say so, with the right status. + if (removalBlockedByOwnerTarget(callerRow.role, targetRow.role, creatorRole)) { + const { APIError } = await import('better-auth/api'); + throw new APIError('FORBIDDEN', { + message: REMOVE_MEMBER_DENIAL_MESSAGE, + code: REMOVE_MEMBER_DENIAL_CODE, + }); + } + + // (3) The vendor's (4): the real `member: ['delete']` check, decided by + // the vendor's own function so there is only ever one answer to it. Only + // the envelope differs — better-auth reports this one as 401. + const { hasPermission } = await import('better-auth/plugins/organization'); + const permitted = await hasPermission( + { + role: callerRow.role, + options: (this.orgRolesMap ? { roles: this.orgRolesMap } : {}) as any, + permissions: { member: ['delete'] }, + organizationId: orgId, + } as any, + ctx, + ); + if (!permitted) { + const { APIError } = await import('better-auth/api'); + throw new APIError('FORBIDDEN', { + message: REMOVE_MEMBER_DENIAL_MESSAGE, + code: REMOVE_MEMBER_DENIAL_CODE, + }); + } + } catch (error) { + // Our own refusal must propagate; anything else is a lookup that did not + // complete, and per the fail-open contract above that hands the request + // back to better-auth unchanged. + const { isAPIError } = await import('better-auth/api'); + if (isAPIError(error)) throw error; + } + } + /** * [#3697] The issuer's own better-auth membership role in `orgId` — the * input to the invitation role cap. diff --git a/packages/plugins/plugin-auth/src/managed-extension-fields.test.ts b/packages/plugins/plugin-auth/src/managed-extension-fields.test.ts index ccacda2f14..6f976240f4 100644 --- a/packages/plugins/plugin-auth/src/managed-extension-fields.test.ts +++ b/packages/plugins/plugin-auth/src/managed-extension-fields.test.ts @@ -402,6 +402,18 @@ const AUTH_MANAGER_PLUGINS: Record unknown } | { skip oauthProvider: { construct: () => oauthProvider({ loginPage: '/login', consentPage: '/oauth/consent' }), }, + // [#8289] NOT a plugin factory — the scanner's regex cannot tell the two + // apart, because both are a one-name destructure off `better-auth/plugins/*`. + // `hasPermission` is the organization plugin's exported permission PREDICATE + // (`(input, ctx) => Promise`, `has-permission.mjs`); it declares no + // schema, contributes no model and no column, so there is nothing here for + // the collision loop to compare. `assertRemoveMemberPermitted` calls it so the + // remove-member gate asks the vendor's own authorization question rather than + // keeping a second spelling of it. The `stale` assertion below removes this + // entry's licence the moment that import goes away. + hasPermission: { + skip: 'permission predicate exported by the organization plugin — declares no schema', + }, }; /** The plugin set the auth manager actually assembles (`buildPluginList()`). */ diff --git a/packages/plugins/plugin-auth/src/remove-member-permission-guard.test.ts b/packages/plugins/plugin-auth/src/remove-member-permission-guard.test.ts index c6e8f324f1..3ce7307ec6 100644 --- a/packages/plugins/plugin-auth/src/remove-member-permission-guard.test.ts +++ b/packages/plugins/plugin-auth/src/remove-member-permission-guard.test.ts @@ -45,7 +45,7 @@ // so the mounted route is what has to answer here. import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { assertEngineDeleteDispatch } from '@objectstack/objectql'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/objectql'; import { AuthManager } from './auth-manager'; const SECRET = 'test-secret-at-least-32-chars-long!!'; @@ -54,11 +54,19 @@ const ORG = 'org_acme'; const PASSWORD = 'S3cure!Passw0rd-8289'; /** - * The same minimal in-memory `IDataEngine` double the other end-to-end - * auth-manager suites use, with `delete` pinned to ObjectQL's own dispatch - * predicate ({@link assertEngineDeleteDispatch}) rather than a hand-written - * copy — a fake that accepts a call the real engine refuses is how a dead route - * ships with its suite green (#4550). + * The in-memory `IDataEngine` double the other end-to-end auth-manager suites + * use, with three places where it is deliberately NO more forgiving than the + * real engine — a fake that accepts a call ObjectQL refuses is how #4434 + * shipped a dead REST route with its suite green (#4550): + * + * - `delete` routes through {@link assertEngineDeleteDispatch}; + * - `update` routes through {@link assertEngineUpdateDispatch}; + * - `sys_member`'s declared `{ organization_id, user_id }` UNIQUE index is + * ENFORCED. That one matters specifically here: every assertion in this file + * reads the membership rows back to prove a removal did or did not happen, so + * a fake that silently tolerated a duplicate membership would let a + * miscounted org read as a correct one — and the owner COUNT is exactly the + * fact the vendor's sole-owner branch turns on. */ const createMemoryEngine = () => { const tables = new Map(); @@ -66,6 +74,22 @@ const createMemoryEngine = () => { if (!tables.has(name)) tables.set(name, []); return tables.get(name)!; }; + /** `sys_member` declares `{ organization_id, user_id }` UNIQUE. */ + const assertMemberUnique = (name: string, row: any, ignoreId?: string) => { + if (name !== 'sys_member') return; + const clash = rows(name).some( + (r) => + r.id !== ignoreId && + r.organization_id === row.organization_id && + r.user_id === row.user_id, + ); + if (clash) { + throw new Error( + 'insert into sys_member … UNIQUE constraint failed: ' + + 'sys_member.organization_id, sys_member.user_id', + ); + } + }; const eq = (a: any, b: any) => a instanceof Date || b instanceof Date ? new Date(a as any).getTime() === new Date(b as any).getTime() @@ -90,6 +114,7 @@ const createMemoryEngine = () => { tables, async insert(name: string, data: any) { const row = { id: data.id ?? `row_${++seq}`, ...data }; + assertMemberUnique(name, row); rows(name).push(row); return { ...row }; }, @@ -106,9 +131,11 @@ const createMemoryEngine = () => { async count(name: string, q: any = {}) { return rows(name).filter((r) => matches(r, q.where)).length; }, - async update(name: string, patch: any) { + async update(name: string, patch: any, options?: any) { + assertEngineUpdateDispatch(patch, options); const row = rows(name).find((r) => r.id === patch.id); if (!row) return null; + assertMemberUnique(name, { ...row, ...patch }, row.id); Object.assign(row, patch); return { ...row }; }, diff --git a/packages/plugins/plugin-auth/src/remove-member-permission-guard.ts b/packages/plugins/plugin-auth/src/remove-member-permission-guard.ts new file mode 100644 index 0000000000..8c65ed3c3b --- /dev/null +++ b/packages/plugins/plugin-auth/src/remove-member-permission-guard.ts @@ -0,0 +1,155 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8289] `POST /organization/remove-member` — answer a PERMISSION denial as a + * permission denial. + * + * ## The defect, and where it is minted + * + * Not here, and not anywhere in our packages: the wrong answer comes out of the + * pinned vendor. better-auth `1.7.0-rc.2`, + * `dist/plugins/organization/routes/crud-members.mjs`, `removeMember` runs: + * + * ```js + * const roles = toBeRemovedMember.role.split(","); + * const creatorRole = ctx.context.orgOptions?.creatorRole || "owner"; + * if (roles.includes(creatorRole)) { + * // (3a) a PERMISSION rule, wearing the invariant's code and status + * if (!member.role.split(",").map(r => r.trim()).includes(creatorRole)) + * throw APIError.from("BAD_REQUEST", YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER); + * // (3b) the genuine invariant + * if (owners.length <= 1) + * throw APIError.from("BAD_REQUEST", YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER); + * } + * // (4) the real permission check — ordered AFTER the two above + * if (!await hasPermission({ role: member.role, permissions: { member: ["delete"] }, … })) + * throw APIError.from("UNAUTHORIZED", YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER); + * ``` + * + * (3a) is "only an owner may remove an owner" — a permission rule — reported + * with the sole-owner invariant's message and a `400`. It is also ordered ahead + * of (4), so whenever the target is an owner and the caller is not, the actual + * permission check never runs and the invariant answers a question nobody asked. + * That is the filed reproduction exactly, and it is why adding a second owner + * does not change the response: (3a) short-circuits before the owner COUNT at + * (3b) is ever consulted, so every clause of "you cannot leave the organization + * as the only owner" can be false while it is still what comes back. + * + * (4) carries a second, smaller defect: `UNAUTHORIZED` (401) where every sibling + * denial — `update-member-role`, `organization/update`, `organization/delete`, + * `organization/invite-member` — answers `FORBIDDEN` (403). + * + * ## Why the fix is a pre-emptive gate and not a response remap + * + * The vendor is not ours to edit (no fork, no vendoring), so the correction has + * to happen at our bridge. It has to happen in the **before**-hook specifically: + * better-auth pins the HTTP status at the moment the handler throws + * (`dispatch.mjs` calls `toResponse(response, { status: result.status })` with + * the ORIGINAL error's `statusCode`, and `better-call`'s `toResponse` resolves + * `init?.status ?? data.statusCode` — init WINS). An after-hook can therefore + * replace the body but NOT the status, which would produce a `400` carrying a + * `YOU_ARE_NOT_ALLOWED_TO_*` code — a worse answer than the one being fixed. + * A before-hook throw is dispatched through `toResponse(before, { headers })` + * with no `status` in init, so the thrown error's own `403` stands. That is the + * same mechanism the `/sso/register` FORBIDDEN gate already relies on. + * + * ## What this guard is allowed to decide, and what it must never touch + * + * It answers the PERMISSION question and nothing else. Two properties make that + * safe, and both are pinned by `remove-member-permission-guard.test.ts`: + * + * 1. **It never speaks on the sole-owner path.** When the caller is removing + * THEMSELVES and carries the creator role, the guard returns silently and + * lets the vendor answer — that is branch (3b)'s territory, the one reading + * of the message that is true. The exemption is exact: (3b) can only fire + * when caller and target are the same person (it requires both to carry the + * creator role while at most one such member exists), so exempting the + * self-removal path removes the guard from the invariant's way completely + * without exempting anything else. + * 2. **It never widens a refusal.** The role test it applies is the vendor's + * own predicate at (3a), reproduced literally — including the asymmetry + * where the target's roles are split WITHOUT `trim()` and the caller's WITH + * it. Reproducing that asymmetry is deliberate: a "cleaned up" predicate + * would refuse inputs the vendor lets through (a `sys_member.role` of + * `' owner'` reads as an owner to a trimming test and as a non-owner to the + * vendor), which would be a policy change smuggled in under an envelope fix. + * The permission half is decided by calling the vendor's OWN exported + * `hasPermission`, never a local re-derivation of it — the org role + * vocabulary is closed (ADR-0108) but the ac map is still the vendor's to + * interpret, and two spellings of one authorization question cannot be kept + * in agreement. + * + * So the refusal SET is byte-for-byte the vendor's; only the envelope changes. + */ + +/** better-auth's own code for this denial — the `YOU_ARE_NOT_ALLOWED_TO_*` family. */ +export const REMOVE_MEMBER_DENIAL_CODE = 'YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER'; + +/** better-auth's own message for {@link REMOVE_MEMBER_DENIAL_CODE}. */ +export const REMOVE_MEMBER_DENIAL_MESSAGE = 'You are not allowed to delete this member'; + +/** better-auth's default `creatorRole` when the org plugin does not set one. */ +export const DEFAULT_CREATOR_ROLE = 'owner'; + +/** + * Does this role value carry `creatorRole`, read the way the vendor's (3a) + * reads the CALLER's — `split(',')` then `trim()` each part. + */ +export function callerCarriesCreatorRole(raw: unknown, creatorRole: string): boolean { + const flat = Array.isArray(raw) ? raw.join(',') : raw; + if (typeof flat !== 'string') return false; + return flat + .split(',') + .map((r) => r.trim()) + .includes(creatorRole); +} + +/** + * Does this role value carry `creatorRole`, read the way the vendor's (3a) + * reads the TARGET's — `split(',')` with NO `trim()`. + * + * The missing `trim()` is the vendor's, not a mistake here: see the header for + * why the asymmetry is reproduced rather than corrected. Correcting it would + * change WHO is refused, which this guard must not do. + */ +export function targetCarriesCreatorRole(raw: unknown, creatorRole: string): boolean { + const flat = Array.isArray(raw) ? raw.join(',') : raw; + if (typeof flat !== 'string') return false; + return flat.split(',').includes(creatorRole); +} + +/** + * The role half of the decision — the vendor's (3a) predicate, and only it. + * + * `true` means "the vendor is about to refuse this at (3a) with the only-owner + * message"; the caller of this function turns that into the `403` the refusal + * should always have been. + */ +export function removalBlockedByOwnerTarget( + callerRole: unknown, + targetRole: unknown, + creatorRole: string = DEFAULT_CREATOR_ROLE, +): boolean { + return ( + targetCarriesCreatorRole(targetRole, creatorRole) && + !callerCarriesCreatorRole(callerRole, creatorRole) + ); +} + +/** + * Is this request the sole-owner invariant's territory — i.e. must the guard + * stay silent and let the vendor answer? + * + * True exactly when the caller is removing themselves AND carries the creator + * role. See header property (1) for why that is the precise exemption. + */ +export function isSoleOwnerGuardTerritory( + callerUserId: string, + targetUserId: string, + callerRole: unknown, + creatorRole: string = DEFAULT_CREATOR_ROLE, +): boolean { + return ( + callerUserId === targetUserId && callerCarriesCreatorRole(callerRole, creatorRole) + ); +} From 96f797fbb96e77a4211e05188aa843bbf5dc6f35 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 04:54:57 +0000 Subject: [PATCH 3/3] chore(changeset): remove-member permission denial answers 403 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 --- .changeset/tidy-pugs-invite.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .changeset/tidy-pugs-invite.md diff --git a/.changeset/tidy-pugs-invite.md b/.changeset/tidy-pugs-invite.md new file mode 100644 index 0000000000..08797b22c5 --- /dev/null +++ b/.changeset/tidy-pugs-invite.md @@ -0,0 +1,20 @@ +--- +'@objectstack/plugin-auth': patch +--- + +`POST /api/v1/auth/organization/remove-member` now answers a permission denial +as `403 YOU_ARE_NOT_ALLOWED_TO_DELETE_THIS_MEMBER`, matching its sibling +endpoints (`organization/update-member-role`, `organization/update`, +`organization/delete`, `organization/invite-member`). + +It previously answered `400 YOU_CANNOT_LEAVE_THE_ORGANIZATION_AS_THE_ONLY_OWNER` +— a message whose every clause could be false at once: the caller was not +leaving, was not an owner, and the organization could hold any number of owners. +better-auth orders its "only an owner may remove an owner" rule ahead of the +route's real permission check and reports it with the sole-owner invariant's +code and status, so the invariant answered a question it was never asked. The +removal itself was always correctly refused; only the response was wrong. + +The genuine sole-owner refusal is unchanged and still fires when a sole owner +removes themselves or calls `organization/leave`, and every legitimate +owner-removes-owner / owner-removes-member path still returns `200`.