From 65237c899e692213492927167364e90e2e3ba293 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 09:53:12 +0000 Subject: [PATCH 1/2] fix(plugin-auth): serve /admin/ban-user and /admin/unban-user with the ADR-0068 platform-admin gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit better-auth's admin plugin authorizes on the legacy `user.role === 'admin'` scalar that ADR-0068 D2 stopped synthesizing, and its option surface at the installed 1.7.1 cannot be pointed at ObjectStack's predicate. Mount both routes as ObjectStack raw routes ahead of the catch-all, carrying the platform-admin gate — the create-user / set-user-password pattern. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .../src/admin-ban-endpoints.test.ts | 236 ++++++++++++++++++ .../plugin-auth/src/admin-ban-endpoints.ts | 208 +++++++++++++++ .../plugins/plugin-auth/src/auth-manager.ts | 46 ++-- .../plugins/plugin-auth/src/auth-plugin.ts | 138 ++++++---- packages/plugins/plugin-auth/src/index.ts | 3 + .../plugin-auth/src/last-local-credential.ts | 84 +++++++ .../plugin-auth/src/platform-admin-gate.ts | 107 ++++++++ 7 files changed, 749 insertions(+), 73 deletions(-) create mode 100644 packages/plugins/plugin-auth/src/admin-ban-endpoints.test.ts create mode 100644 packages/plugins/plugin-auth/src/admin-ban-endpoints.ts create mode 100644 packages/plugins/plugin-auth/src/last-local-credential.ts create mode 100644 packages/plugins/plugin-auth/src/platform-admin-gate.ts diff --git a/packages/plugins/plugin-auth/src/admin-ban-endpoints.test.ts b/packages/plugins/plugin-auth/src/admin-ban-endpoints.test.ts new file mode 100644 index 0000000000..ad17e502ea --- /dev/null +++ b/packages/plugins/plugin-auth/src/admin-ban-endpoints.test.ts @@ -0,0 +1,236 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #9652 — ban / unban as ObjectStack raw mounts with the ADR-0068 gate. + * + * ⛔ NOTE FOR THE NEXT AUTHOR: nothing in this file patches `role = 'admin'` + * onto a user row. The pre-existing hand-patch in `remove-user-atomicity.test.ts` + * exists precisely because the better-auth admin plugin authorizes on that + * legacy scalar; these handlers do not, so the workaround is not needed and + * must not be reintroduced (re-synthesizing the scalar is permanently vetoed, + * maintainer ruling 2026-08-18). + * + * Rejection cases assert `code` AND `status` per ADR-0112 — a bare "it threw" + * would pass against a handler that refuses everyone. + */ + +import { describe, it, expect } from 'vitest'; +import { runAdminBanUser, runAdminUnbanUser, type AuthBanContextLike } from './admin-ban-endpoints.js'; +import { judgePlatformAdmin, isPlatformAdminUser } from './platform-admin-gate.js'; +import { isLastLocalCredentialHolder } from './last-local-credential.js'; + +const ADMIN = { id: 'usr_admin', email: 'admin@example.com' }; + +interface Recorded { + updates: Array<{ id: string; data: Record }>; + sessionsDeletedFor: string[]; +} + +/** + * A fake `$context` slice. `credentialHolders` drives the break-glass guard: + * the ids that hold a local password account. + */ +function fakeContext(opts: { + users?: string[]; + credentialHolders?: string[]; +}): { ctx: AuthBanContextLike; rec: Recorded } { + const users = new Set(opts.users ?? ['usr_target']); + const holders = opts.credentialHolders ?? []; + const rec: Recorded = { updates: [], sessionsDeletedFor: [] }; + + const ctx: AuthBanContextLike = { + internalAdapter: { + findUserById: async (id) => (users.has(id) ? { id } : null), + updateUser: async (id, data) => { + rec.updates.push({ id, data }); + return { id, ...data }; + }, + deleteUserSessions: async (userId) => { + rec.sessionsDeletedFor.push(userId); + return true; + }, + }, + adapter: { + findOne: async ({ where }) => { + const userId = where.find((w) => w.field === 'userId')?.value; + return userId && holders.includes(userId) ? { userId } : null; + }, + findMany: async () => holders.map((userId) => ({ userId })), + }, + }; + return { ctx, rec }; +} + +const post = (body: unknown): Request => + new Request('http://local/api/v1/auth/admin/ban-user', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }); + +const depsFor = (ctx: AuthBanContextLike) => ({ getAuthContext: async () => ctx }); + +describe('#9652 runAdminBanUser', () => { + it('bans the target and revokes its sessions, mirroring the vendor write', async () => { + const { ctx, rec } = fakeContext({ credentialHolders: ['usr_target', 'usr_admin'] }); + + const res = await runAdminBanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_target', banReason: 'abuse' })); + + expect(res.status).toBe(200); + expect(res.body.success).toBe(true); + expect(rec.updates).toHaveLength(1); + expect(rec.updates[0].id).toBe('usr_target'); + expect(rec.updates[0].data.banned).toBe(true); + expect(rec.updates[0].data.banReason).toBe('abuse'); + // Without this the banned user keeps a live session until it expires. + expect(rec.sessionsDeletedFor).toEqual(['usr_target']); + }); + + it("defaults the reason to the vendor's 'No reason'", async () => { + const { ctx, rec } = fakeContext({ credentialHolders: ['usr_target', 'usr_admin'] }); + await runAdminBanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_target' })); + expect(rec.updates[0].data.banReason).toBe('No reason'); + expect(rec.updates[0].data.banExpires).toBeUndefined(); + }); + + it('converts banExpiresIn seconds into an expiry date', async () => { + const { ctx, rec } = fakeContext({ credentialHolders: ['usr_target', 'usr_admin'] }); + const before = Date.now(); + await runAdminBanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_target', banExpiresIn: 3600 })); + const expires = rec.updates[0].data.banExpires as Date; + expect(expires).toBeInstanceOf(Date); + expect(expires.getTime()).toBeGreaterThanOrEqual(before + 3600_000); + }); + + it('refuses a missing userId with INVALID_REQUEST 400', async () => { + const { ctx, rec } = fakeContext({}); + const res = await runAdminBanUser(depsFor(ctx), ADMIN, post({})); + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('INVALID_REQUEST'); + expect(rec.updates).toHaveLength(0); + }); + + it('refuses an unknown user with RESOURCE_NOT_FOUND 404', async () => { + const { ctx, rec } = fakeContext({ users: ['usr_target'] }); + const res = await runAdminBanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_ghost' })); + expect(res.status).toBe(404); + expect(res.body.error?.code).toBe('RESOURCE_NOT_FOUND'); + expect(rec.updates).toHaveLength(0); + }); + + it('refuses self-ban with INVALID_REQUEST 400', async () => { + const { ctx, rec } = fakeContext({ users: ['usr_admin'] }); + const res = await runAdminBanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_admin' })); + expect(res.status).toBe(400); + expect(res.body.error?.code).toBe('INVALID_REQUEST'); + expect(rec.updates).toHaveLength(0); + }); + + it('keeps the break-glass guard the shadowed vendor route used to inherit', async () => { + // The target is the ONLY holder of a local password: banning it would sign + // the last break-glass account out of a deployment under enforced SSO. + const { ctx, rec } = fakeContext({ credentialHolders: ['usr_target'] }); + const res = await runAdminBanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_target' })); + expect(res.status).toBe(409); + expect(res.body.error?.code).toBe('LAST_LOCAL_CREDENTIAL'); + expect(rec.updates).toHaveLength(0); + expect(rec.sessionsDeletedFor).toEqual([]); + }); + + it('permits banning a credential-less (managed) user even when it is alone', async () => { + const { ctx, rec } = fakeContext({ credentialHolders: [] }); + const res = await runAdminBanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_target' })); + expect(res.status).toBe(200); + expect(rec.updates).toHaveLength(1); + }); +}); + +describe('#9652 runAdminUnbanUser', () => { + it('clears every ban field the vendor handler clears', async () => { + const { ctx, rec } = fakeContext({}); + const res = await runAdminUnbanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_target' })); + expect(res.status).toBe(200); + expect(rec.updates[0].data).toMatchObject({ banned: false, banReason: null, banExpires: null }); + }); + + it('refuses an unknown user with RESOURCE_NOT_FOUND 404', async () => { + const { ctx } = fakeContext({ users: ['usr_target'] }); + const res = await runAdminUnbanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_ghost' })); + expect(res.status).toBe(404); + expect(res.body.error?.code).toBe('RESOURCE_NOT_FOUND'); + }); + + it('does NOT apply the break-glass guard — lifting a ban cannot cause lockout', async () => { + const { ctx, rec } = fakeContext({ credentialHolders: ['usr_target'] }); + const res = await runAdminUnbanUser(depsFor(ctx), ADMIN, post({ userId: 'usr_target' })); + expect(res.status).toBe(200); + expect(rec.updates).toHaveLength(1); + }); +}); + +describe('#9652 the shared ADR-0068 platform-admin gate', () => { + it('admits a platform admin carrying positions[] and NO role scalar', () => { + // This is the identity a real deployment produces after ADR-0068 D2 — the + // exact shape better-auth refuses. + const verdict = judgePlatformAdmin({ + user: { id: 'usr_admin', email: 'a@b.c', positions: ['user', 'platform_admin'], role: 'user' }, + }); + expect(verdict.ok).toBe(true); + expect(verdict.ok && verdict.actor.id).toBe('usr_admin'); + }); + + it('admits on the derived isPlatformAdmin alias alone', () => { + expect(judgePlatformAdmin({ user: { id: 'u', isPlatformAdmin: true } }).ok).toBe(true); + }); + + it('refuses an anonymous caller 401 UNAUTHENTICATED', () => { + const verdict = judgePlatformAdmin(null); + expect(verdict.ok).toBe(false); + expect(!verdict.ok && verdict.refusal.status).toBe(401); + expect(!verdict.ok && verdict.refusal.body.error.code).toBe('UNAUTHENTICATED'); + }); + + it('refuses a signed-in plain member 403 PERMISSION_DENIED', () => { + const verdict = judgePlatformAdmin({ user: { id: 'usr_member', positions: ['user'], role: 'user' } }); + expect(verdict.ok).toBe(false); + expect(!verdict.ok && verdict.refusal.status).toBe(403); + expect(!verdict.ok && verdict.refusal.body.error.code).toBe('PERMISSION_DENIED'); + }); + + it('an org admin is NOT a platform admin', () => { + expect(isPlatformAdminUser({ id: 'u', positions: ['user', 'org_admin', 'org_owner'] })).toBe(false); + }); + + it('still reads a pre-ADR-0068 role scalar as the legacy fallback', () => { + expect(isPlatformAdminUser({ id: 'u', role: 'admin' })).toBe(true); + }); +}); + +describe('#9652 isLastLocalCredentialHolder', () => { + const adapterFor = (holders: string[]) => ({ + findOne: async ({ where }: { where: Array<{ field: string; value: string }> }) => { + const userId = where.find((w) => w.field === 'userId')?.value; + return userId && holders.includes(userId) ? { userId } : null; + }, + findMany: async () => holders.map((userId) => ({ userId })), + }); + + it('is true only for the sole credential holder', async () => { + expect(await isLastLocalCredentialHolder(adapterFor(['a']), 'a')).toBe(true); + expect(await isLastLocalCredentialHolder(adapterFor(['a', 'b']), 'a')).toBe(false); + }); + + it('is false for a user holding no credential account', async () => { + expect(await isLastLocalCredentialHolder(adapterFor(['a']), 'b')).toBe(false); + }); + + it('fails OPEN on a lookup error — a transient failure must not block an admin', async () => { + const broken = { + findOne: async () => { + throw new Error('db down'); + }, + findMany: async () => [], + }; + expect(await isLastLocalCredentialHolder(broken, 'a')).toBe(false); + }); +}); diff --git a/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts b/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts new file mode 100644 index 0000000000..2c992572ce --- /dev/null +++ b/packages/plugins/plugin-auth/src/admin-ban-endpoints.ts @@ -0,0 +1,208 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Admin ban / unban endpoints — ObjectStack raw mounts carrying the ADR-0068 + * platform-admin gate, shadowing better-auth's native `/admin/ban-user` and + * `/admin/unban-user`. + * + * POST /api/v1/auth/admin/ban-user — ban a user, revoking their sessions + * POST /api/v1/auth/admin/unban-user — lift a ban + * + * They join `create-user` / `set-user-password` / `unlock-user` / + * `import-users` / `oauth2/toggle-disabled`, which are ObjectStack mounts for + * the same reason. + * + * ── WHY these are re-implemented rather than configured (the measurement) ──── + * + * better-auth's `admin` plugin authorizes every `/admin/*` route through + * `hasPermission({ userId, role, options, permissions })`, whose only two + * authorization inputs are: + * + * 1. `options.adminUserIds` — a `string[]` frozen into the plugin's options + * object at CONSTRUCTION time; and + * 2. `session.user.role` — the persisted legacy scalar. + * + * ObjectStack's platform-admin predicate is neither: it is a per-request read + * of `sys_user_permission_set` for a row pointing at `admin_full_access` with + * `organization_id = null`. A construction-time id array cannot express a + * predicate that changes while the process runs, and every trick to make the + * array dynamic either grants a demoted admin a stale pass (fail-OPEN — the + * one direction that turns a broken-capability bug into a security bug) or + * needs an async answer where the vendor calls a synchronous one. + * + * And the remaining door — synthesizing the role onto the SESSION-scoped user + * object at request time, without persisting anything — is mechanically shut + * on the installed version: every one of these routes mounts `adminMiddleware`, + * which calls `getAuthoritativeSessionFromCtx`, which on any deployment with a + * `database` (i.e. every ObjectStack deployment) sets `ctx.context.session = + * null` and re-reads the session from the DB with the cookie cache disabled. + * Anything ObjectStack writes onto the in-memory session user is discarded + * before `hasPermission` ever sees it. `customSession` is not a door either — + * it overrides the `/get-session` ENDPOINT, not the session the admin routes + * resolve internally. + * + * So the vendor cannot be pointed at the predicate, and ADR-0068 D2 forbids + * producing the scalar it can be pointed at. Re-implementation is what is left + * (maintainer ruling 2026-08-18: Option 1 if the vendor can express it, else + * Option 2; re-synthesizing `user.role = 'admin'` is permanently vetoed). + * + * ── Fidelity to the handler being shadowed ────────────────────────────────── + * + * The writes mirror better-auth's own handlers field for field — `banned` / + * `banReason` / `banExpires` / `updatedAt`, then `deleteUserSessions` — so a + * banned user is signed out and refused at sign-in by the vendor's OWN session + * hook (`BANNED_USER`), which is untouched. The default ban reason is + * `'No reason'` because ObjectStack configures no `defaultBanReason`. + * + * ⚠️ Shadowing a vendor route detaches every better-auth hook keyed on its + * path. `/admin/ban-user` carried one: the break-glass last-local-credential + * guard in `auth-manager.ts`. It is re-run here from the shared module + * (`last-local-credential.ts`) rather than reimplemented — see that file's + * header for why it is a module now. + * + * The refusal envelope is ObjectStack's (ADR-0112 `{success,error:{code}}`), + * NOT better-auth's flat `{message,code}`: these routes are ObjectStack's, and + * the dogfood sweep distinguishes the two envelopes on purpose. + */ + +import { + isLastLocalCredentialHolder, + LAST_LOCAL_CREDENTIAL_CODE, + LAST_LOCAL_CREDENTIAL_MESSAGE, + type CredentialAccountAdapter, +} from './last-local-credential.js'; +import type { AdminActor, EndpointResult } from './admin-user-endpoints.js'; + +/** + * Minimal better-auth `$context` surface these two routes touch. Mirrors what + * the stock handlers use, minus their role check (the mount gates instead). + */ +export interface AuthBanContextLike { + internalAdapter: { + findUserById(id: string): Promise; + updateUser(id: string, data: Record): Promise; + deleteUserSessions(userId: string): Promise; + }; + adapter: CredentialAccountAdapter; +} + +export interface AdminBanEndpointDeps { + getAuthContext(): Promise; +} + +const invalid = (message: string): EndpointResult => ({ + status: 400, + body: { success: false, error: { code: 'INVALID_REQUEST', message } }, +}); + +const notFound = (): EndpointResult => ({ + status: 404, + body: { success: false, error: { code: 'RESOURCE_NOT_FOUND', message: 'User not found' } }, +}); + +async function parseJson(request: Request): Promise> { + try { + const parsed = await request.json(); + return parsed && typeof parsed === 'object' ? (parsed as Record) : {}; + } catch { + return {}; + } +} + +/** + * Read the target user id. Both spellings are accepted because the console's + * `recordIdParam: 'userId'` sends the camelCase one while the shadowed + * ObjectStack siblings (`unlock-user`) have always also read `user_id`. + */ +function readUserId(body: Record): string | undefined { + const raw = body.userId ?? body.user_id; + return typeof raw === 'string' && raw.length > 0 ? raw : undefined; +} + +/** `POST /api/v1/auth/admin/ban-user` — the caller is already gated. */ +export async function runAdminBanUser( + deps: AdminBanEndpointDeps, + actor: AdminActor, + request: Request, +): Promise { + const body = await parseJson(request); + const userId = readUserId(body); + if (!userId) return invalid('userId is required'); + + // Same refusal better-auth makes (`YOU_CANNOT_BAN_YOURSELF`): an admin who + // bans themselves is immediately signed out and cannot undo it. + if (userId === actor.id) return invalid('You cannot ban yourself'); + + const banReason = + typeof body.banReason === 'string' && body.banReason.length > 0 ? body.banReason : 'No reason'; + + // Seconds until the ban lifts; absent means it never expires. + let banExpires: Date | undefined; + if (body.banExpiresIn !== undefined) { + const seconds = Number(body.banExpiresIn); + if (!Number.isFinite(seconds) || seconds <= 0) { + return invalid('banExpiresIn must be a positive number of seconds'); + } + banExpires = new Date(Date.now() + seconds * 1000); + } + + const ctx = await deps.getAuthContext(); + if (!(await ctx.internalAdapter.findUserById(userId))) return notFound(); + + // Break-glass — the guard the shadowed vendor route used to inherit from + // `auth-manager.ts`'s before-hook, which a raw mount no longer reaches. + if (await isLastLocalCredentialHolder(ctx.adapter, userId)) { + return { + status: 409, + body: { + success: false, + error: { code: LAST_LOCAL_CREDENTIAL_CODE, message: LAST_LOCAL_CREDENTIAL_MESSAGE }, + }, + }; + } + + await ctx.internalAdapter.updateUser(userId, { + banned: true, + banReason, + ...(banExpires ? { banExpires } : {}), + updatedAt: new Date(), + }); + // Sign the banned user out everywhere, exactly as the vendor handler does. + await ctx.internalAdapter.deleteUserSessions(userId); + + return { + status: 200, + body: { + success: true, + data: { + userId, + banned: true, + banReason, + ...(banExpires ? { banExpires: banExpires.toISOString() } : {}), + }, + }, + }; +} + +/** `POST /api/v1/auth/admin/unban-user` — the caller is already gated. */ +export async function runAdminUnbanUser( + deps: AdminBanEndpointDeps, + _actor: AdminActor, + request: Request, +): Promise { + const body = await parseJson(request); + const userId = readUserId(body); + if (!userId) return invalid('userId is required'); + + const ctx = await deps.getAuthContext(); + if (!(await ctx.internalAdapter.findUserById(userId))) return notFound(); + + await ctx.internalAdapter.updateUser(userId, { + banned: false, + banReason: null, + banExpires: null, + updatedAt: new Date(), + }); + + return { status: 200, body: { success: true, data: { userId, banned: false } } }; +} diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 1f65947095..7646c234a2 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -57,6 +57,11 @@ import { import type { TenancyService } from './tenancy-service.js'; import { OtpSendGuard, assertOtpCooldownSeconds } from './otp-send-guard.js'; import type { CounterStore } from './rate-limit-storage.js'; +import { + isLastLocalCredentialHolder, + LAST_LOCAL_CREDENTIAL_CODE, + LAST_LOCAL_CREDENTIAL_MESSAGE, +} from './last-local-credential.js'; import { PHONE_SMS_TOPICS, builtinPhoneSmsBody, @@ -1550,9 +1555,15 @@ export class AuthManager { ctx?.path === '/admin/remove-user' || ctx?.path === '/admin/ban-user' ) { + // ⚠️ `/admin/ban-user` no longer reaches this hook — #9652 mounts an + // ObjectStack raw route on that path ahead of the catch-all (the + // ADR-0068 gate better-auth cannot express), so the request never + // enters better-auth's router. The path stays listed here because + // the mount is conditional on the admin plugin, and because the + // guard itself now lives in ONE module both call sites share — + // `last-local-credential.ts`, whose header records this trap. let isLastLocalCredential = false; try { - const adapter = ctx.context.adapter; let targetId: string | undefined = ctx?.body?.userId ?? ctx?.body?.user_id; if (!targetId && ctx.path === '/delete-user') { const { getSessionFromCtx } = await import('better-auth/api'); @@ -1560,27 +1571,10 @@ export class AuthManager { targetId = s?.user?.id ?? s?.session?.userId; } if (targetId) { - // Only guard when the target actually holds a local credential — - // removing a credential-less (managed) user can't cause lockout. - const targetCred = await adapter.findOne({ - model: 'account', - where: [ - { field: 'userId', value: targetId }, - { field: 'providerId', value: 'credential' }, - ], - }); - if (targetCred) { - const creds: any[] = await adapter.findMany({ - model: 'account', - where: [{ field: 'providerId', value: 'credential' }], - }); - const otherHolders = new Set( - (creds ?? []) - .map((a: any) => a?.userId ?? a?.user_id) - .filter((id: any) => id && id !== targetId), - ); - isLastLocalCredential = otherHolders.size === 0; - } + isLastLocalCredential = await isLastLocalCredentialHolder( + ctx.context.adapter, + targetId, + ); } } catch { // Fail-open — never block a legitimate op on a lookup error. @@ -1588,12 +1582,8 @@ export class AuthManager { if (isLastLocalCredential) { const { APIError } = await import('better-auth/api'); throw new APIError('CONFLICT', { - message: - 'Cannot remove the last local password login. At least one ' + - 'break-glass account with a password must remain so an identity-' + - 'provider outage can never lock the organization out. Add another ' + - 'local password first, then retry.', - code: 'LAST_LOCAL_CREDENTIAL', + message: LAST_LOCAL_CREDENTIAL_MESSAGE, + code: LAST_LOCAL_CREDENTIAL_CODE, }); } // fall through to better-auth's own handler diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index ad03e27e64..9a502aae96 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -57,6 +57,12 @@ import { authPluginManifestHeader, } from './manifest.js'; import { scheduleLegacySsoSecretMigration } from './sso-client-secret.js'; +import { judgePlatformAdmin, type PlatformAdminActor } from './platform-admin-gate.js'; +import { + runAdminBanUser, + runAdminUnbanUser, + type AdminBanEndpointDeps, +} from './admin-ban-endpoints.js'; /** @@ -1751,25 +1757,12 @@ export class AuthPlugin implements Plugin { return c.json({ success: false, error: { code: 'INVALID_REQUEST', message: 'disabled must be a boolean' } }, 400); } + // Platform-admin gate (ADR-0068 D2) — one shared judge for every + // ObjectStack `/admin/*` mount; see platform-admin-gate.ts. const authApi = await this.authManager!.getApi(); const session = await authApi.getSession({ headers: c.req.raw.headers }); - if (!session?.user?.id) { - return c.json({ success: false, error: { code: 'UNAUTHENTICATED', message: 'Sign in first' } }, 401); - } - // Platform-admin gate. ADR-0068 removed the `user.role = 'admin'` - // synthesis, so a stale `role === 'admin'` check now rejects even - // platform admins. Accept the canonical signals customSession carries - // (the derived `isPlatformAdmin` alias / `platform_admin` in roles[]), - // with the legacy admin-plugin `role` scalar as a fallback. Mirrors the - // /admin/unlock-user gate below. - const u: any = session.user; - const isAdmin = - u?.isPlatformAdmin === true || - (Array.isArray(u?.positions) && u.positions.includes('platform_admin')) || - u?.role === 'admin'; - if (!isAdmin) { - return c.json({ success: false, error: { code: 'PERMISSION_DENIED', message: 'Admin role required' } }, 403); - } + const verdict = judgePlatformAdmin(session); + if (!verdict.ok) return c.json(verdict.refusal.body, verdict.refusal.status); // Write through the same ObjectQL data engine that better-auth's // adapter uses. We target the snake_case table name (`sys_oauth_application`, @@ -1858,23 +1851,11 @@ export class AuthPlugin implements Plugin { return c.json({ success: false, error: { code: 'INVALID_REQUEST', message: 'userId is required' } }, 400); } + // Platform-admin gate (ADR-0068 D2) — see platform-admin-gate.ts. const authApi = await this.authManager!.getApi(); const session = await authApi.getSession({ headers: c.req.raw.headers }); - if (!session?.user?.id) { - return c.json({ success: false, error: { code: 'UNAUTHENTICATED', message: 'Sign in first' } }, 401); - } - // Platform-admin gate. Accept any of the equivalent signals the - // customSession plugin may carry (ADR-0068): the derived - // `isPlatformAdmin` alias, the canonical `platform_admin` in roles[], - // or the legacy admin-plugin `role` scalar. - const u: any = session.user; - const isAdmin = - u?.isPlatformAdmin === true || - (Array.isArray(u?.positions) && u.positions.includes('platform_admin')) || - u?.role === 'admin'; - if (!isAdmin) { - return c.json({ success: false, error: { code: 'PERMISSION_DENIED', message: 'Admin role required' } }, 403); - } + const verdict = judgePlatformAdmin(session); + if (!verdict.ok) return c.json(verdict.refusal.body, verdict.refusal.status); const ok = await this.authManager!.unlockUser(userId); if (!ok) { @@ -1919,21 +1900,12 @@ export class AuthPlugin implements Plugin { getTenancy: () => this.tenancy ?? undefined, logger: ctx.logger, }); - const gateAdmin = async (c: any): Promise<{ id: string; email?: string } | Response> => { + const gateAdmin = async (c: any): Promise => { const authApi = await this.authManager!.getApi(); const session = await (authApi as any).getSession({ headers: c.req.raw.headers }); - if (!session?.user?.id) { - return c.json({ success: false, error: { code: 'UNAUTHENTICATED', message: 'Sign in first' } }, 401); - } - const u: any = session.user; - const isAdmin = - u?.isPlatformAdmin === true || - (Array.isArray(u?.positions) && u.positions.includes('platform_admin')) || - u?.role === 'admin'; - if (!isAdmin) { - return c.json({ success: false, error: { code: 'PERMISSION_DENIED', message: 'Admin role required' } }, 403); - } - return { id: String(u.id), email: typeof u.email === 'string' ? u.email : undefined }; + const verdict = judgePlatformAdmin(session); + if (!verdict.ok) return c.json(verdict.refusal.body, verdict.refusal.status); + return verdict.actor; }; rawApp.post(`${basePath}/admin/create-user`, async (c: any) => { @@ -1967,6 +1939,82 @@ export class AuthPlugin implements Plugin { } }); + // ── #9652: ban / unban, re-mounted with the ADR-0068 gate ──────────── + // + // These SHADOW better-auth's native `/admin/ban-user` and + // `/admin/unban-user`. The vendor's `admin` plugin authorizes on the + // legacy `user.role === 'admin'` scalar that ADR-0068 D2 stopped + // synthesizing, and it exposes no option that can be pointed at + // ObjectStack's predicate — a MEASURED property of the installed + // version, written up in admin-ban-endpoints.ts. The result was that the + // `sys_user` Ban / Unban buttons 403'd for every platform admin on any + // deployment with the admin plugin on (SCIM forces it, ADR-0071). + // + // ⚠️ Shadowing detaches better-auth hooks keyed on the path: the + // break-glass last-local-credential guard used to fire on + // `/admin/ban-user` from `auth-manager.ts`. `runAdminBanUser` re-runs it + // from the shared module. + const adminBanDeps = (): AdminBanEndpointDeps => ({ + getAuthContext: () => this.authManager!.getAuthContext(), + }); + + /** + * Refuse when the admin plugin is off, exactly as create-user does. + * + * Without this the shadowing mounts would answer 200 on a deployment + * that has no admin plugin — writing `banned: true` while the vendor's + * session hook that ENFORCES a ban is not loaded. That is a + * declared-but-not-enforced ban: the console reports success and the + * banned user signs straight back in. `banUser` on the api surface is + * the same seam create-user tests for. + */ + const adminPluginMissing = async (c: any): Promise => { + const authApi: any = await this.authManager!.getApi(); + if (typeof authApi.banUser === 'function') return undefined; + return c.json( + { success: false, error: { code: 'NOT_IMPLEMENTED', message: 'The better-auth admin plugin is not enabled (auth.plugins.admin)' } }, + 501, + ); + }; + + rawApp.post(`${basePath}/admin/ban-user`, async (c: any) => { + try { + const actor = await gateAdmin(c); + if (actor instanceof Response) return actor; + const unavailable = await adminPluginMissing(c); + if (unavailable) return unavailable; + // Attributed to the admin for the same reason create-user is: the + // write is driven server-side and never passes through + // `AuthManager.handleRequest`, so the request-scoped actor seam is + // not open unless we open it here. + const { status, body } = await runAttributedToUser(actor.id, () => + runAdminBanUser(adminBanDeps(), actor, c.req.raw), + ); + return c.json(body, status as any); + } catch (error) { + const err = error instanceof Error ? error : new Error(String(error)); + ctx.logger.error('[AuthPlugin] admin/ban-user failed', err); + return c.json({ success: false, error: { code: 'INTERNAL_ERROR', message: err.message } }, 500); + } + }); + + rawApp.post(`${basePath}/admin/unban-user`, async (c: any) => { + try { + const actor = await gateAdmin(c); + if (actor instanceof Response) return actor; + const unavailable = await adminPluginMissing(c); + if (unavailable) return unavailable; + const { status, body } = await runAttributedToUser(actor.id, () => + runAdminUnbanUser(adminBanDeps(), actor, c.req.raw), + ); + return c.json(body, status as any); + } catch (error) { + const err = error instanceof Error ? error : new Error(String(error)); + ctx.logger.error('[AuthPlugin] admin/unban-user failed', err); + return c.json({ success: false, error: { code: 'INTERNAL_ERROR', message: err.message } }, 500); + } + }); + rawApp.post(`${basePath}/admin/set-user-password`, async (c: any) => { try { const actor = await gateAdmin(c); diff --git a/packages/plugins/plugin-auth/src/index.ts b/packages/plugins/plugin-auth/src/index.ts index 042bcf5753..5b4f096b5f 100644 --- a/packages/plugins/plugin-auth/src/index.ts +++ b/packages/plugins/plugin-auth/src/index.ts @@ -17,6 +17,9 @@ export * from './ensure-default-organization.js'; export * from './backfill-account-issuer.js'; export * from './set-initial-password.js'; export * from './admin-user-endpoints.js'; +export * from './admin-ban-endpoints.js'; +export * from './platform-admin-gate.js'; +export * from './last-local-credential.js'; export * from './placeholder-email.js'; export * from './admin-import-users.js'; export * from './identity-write-guard.js'; diff --git a/packages/plugins/plugin-auth/src/last-local-credential.ts b/packages/plugins/plugin-auth/src/last-local-credential.ts new file mode 100644 index 0000000000..8278e8b517 --- /dev/null +++ b/packages/plugins/plugin-auth/src/last-local-credential.ts @@ -0,0 +1,84 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Break-glass guard: never remove the LAST local-password login. + * + * Under enforced SSO the managed team holds no local credential; the env owner + * / a local admin keeps one as the break-glass escape hatch so an IdP outage + * can never lock the org out. Deleting or banning the last user holding a + * `credential` account is refused. + * + * ── Why this is a module and not an inline check ───────────────────────────── + * + * It used to live inline in `auth-manager.ts`'s better-auth `before` hook, + * matched on `ctx.path`. That is the correct seam for routes better-auth + * serves — and it silently STOPS BEING A SEAM the moment ObjectStack mounts a + * raw Hono route on the same path ahead of the catch-all, because the request + * then never enters better-auth's router at all. `/admin/ban-user` is now such + * a route (the ADR-0068 platform-admin gate the vendor cannot express), so the + * guard has two call sites and must be ONE implementation: a raw mount that + * forgot to re-run it would drop a lockout protection with no test, no gate + * and no diff to read. + * + * Fail-open on lookup errors is deliberate and unchanged: a transient query + * error must never block a legitimate admin operation. + */ + +/** The slice of better-auth's DB adapter this guard needs. */ +export interface CredentialAccountAdapter { + findOne(query: { + model: string; + where: Array<{ field: string; value: string }>; + }): Promise; + findMany(query: { + model: string; + where: Array<{ field: string; value: string }>; + }): Promise; +} + +/** The error code + message both call sites answer with. */ +export const LAST_LOCAL_CREDENTIAL_CODE = 'LAST_LOCAL_CREDENTIAL'; +export const LAST_LOCAL_CREDENTIAL_MESSAGE = + 'Cannot remove the last local password login. At least one break-glass ' + + 'account with a password must remain so an identity-provider outage can ' + + 'never lock the organization out. Add another local password first, then retry.'; + +/** + * Would removing/banning `targetId` leave zero users holding a local password? + * + * Returns `false` — permit — when the target holds no credential account at + * all (removing a credential-less, managed user cannot cause lockout), and + * `false` on any lookup failure (fail-open, see the header). + */ +export async function isLastLocalCredentialHolder( + adapter: CredentialAccountAdapter, + targetId: string, +): Promise { + if (!targetId) return false; + try { + // Only guard when the target actually holds a local credential. + const targetCred = await adapter.findOne({ + model: 'account', + where: [ + { field: 'userId', value: targetId }, + { field: 'providerId', value: 'credential' }, + ], + }); + if (!targetCred) return false; + + const creds = (await adapter.findMany({ + model: 'account', + where: [{ field: 'providerId', value: 'credential' }], + })) as Array>; + + const otherHolders = new Set( + (creds ?? []) + .map((a) => (a?.userId ?? a?.user_id) as string | undefined) + .filter((id): id is string => Boolean(id) && id !== targetId), + ); + return otherHolders.size === 0; + } catch { + // Fail-open — never block a legitimate op on a lookup error. + return false; + } +} diff --git a/packages/plugins/plugin-auth/src/platform-admin-gate.ts b/packages/plugins/plugin-auth/src/platform-admin-gate.ts new file mode 100644 index 0000000000..1fcb18a1c2 --- /dev/null +++ b/packages/plugins/plugin-auth/src/platform-admin-gate.ts @@ -0,0 +1,107 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The ADR-0068 D2 platform-admin gate, in ONE place. + * + * Every ObjectStack raw `/admin/*` mount owns its own authorization, because + * better-auth's `admin` plugin cannot be pointed at ObjectStack's predicate. + * That is a MEASURED property of the installed vendor version, not an + * assumption — see `admin-ban-endpoints.ts` for the measurement, and + * `auth-manager.ts` for the `positions[]` derivation this reads. + * + * Before this module the gate existed as four near-identical inline copies + * (`/admin/oauth2/toggle-disabled`, `/admin/unlock-user`, and the shared + * `gateAdmin` behind `/admin/create-user` + `/admin/set-user-password` + + * `/admin/import-users`). N copies of an authorization predicate is the shape + * that drifts: the next mount is written by copying whichever copy the author + * happened to open. One exported judge, called by every mount, is the fix — + * and it is what lets a new signal be added in a single edit. + * + * ⛔ `user.role === 'admin'` is accepted ONLY as the legacy fallback it has + * always been here. It is NOT synthesized, and nothing in ObjectStack writes + * it for a platform admin (ADR-0068 D2; re-synthesizing it is permanently + * vetoed by the maintainer's 2026-08-18 ruling). It stays readable so a + * deployment that still carries the scalar from before D2 is not locked out. + */ + +/** The caller a passing gate hands back to the route. */ +export interface PlatformAdminActor { + id: string; + email?: string; +} + +/** ADR-0112 refusal envelope — the ObjectStack shape, not better-auth's. */ +export interface PlatformAdminRefusal { + status: 401 | 403; + body: { success: false; error: { code: string; message: string } }; +} + +export type PlatformAdminVerdict = + | { ok: true; actor: PlatformAdminActor } + | { ok: false; refusal: PlatformAdminRefusal }; + +/** + * Is this session user a platform admin under ADR-0068 D2? + * + * Reads the canonical signals `customSession` contributes — the derived + * `isPlatformAdmin` alias and `platform_admin` in `positions[]` — plus the + * legacy `role` scalar as the back-compat fallback described above. + * + * Exported separately from `judgePlatformAdmin` so a caller that already holds + * a session (a test, a hook) can ask the question without building an + * envelope. + */ +export function isPlatformAdminUser(sessionUser: unknown): boolean { + const u = sessionUser as Record | null | undefined; + if (!u) return false; + if (u.isPlatformAdmin === true) return true; + if (Array.isArray(u.positions) && u.positions.includes('platform_admin')) return true; + return u.role === 'admin'; +} + +/** + * Judge a resolved session for a platform-admin-only route. + * + * `session` is what `auth.api.getSession({ headers })` returned — that call + * goes through the `customSession` override, which is why `positions[]` and + * `isPlatformAdmin` are present on `session.user` at all. + * + * The two refusals are deliberately distinct and are asserted as such by the + * dogfood sweep: anonymous is 401 `UNAUTHENTICATED` (we do not know who you + * are), a signed-in non-admin is 403 `PERMISSION_DENIED` (we do, and the + * answer is no). Collapsing them into one status would make the sweep unable + * to tell "the payload never reached the gate" from "the gate said no". + */ +export function judgePlatformAdmin(session: unknown): PlatformAdminVerdict { + const user = (session as { user?: unknown } | null | undefined)?.user as + | Record + | undefined; + + if (!user?.id) { + return { + ok: false, + refusal: { + status: 401, + body: { success: false, error: { code: 'UNAUTHENTICATED', message: 'Sign in first' } }, + }, + }; + } + + if (!isPlatformAdminUser(user)) { + return { + ok: false, + refusal: { + status: 403, + body: { success: false, error: { code: 'PERMISSION_DENIED', message: 'Admin role required' } }, + }, + }; + } + + return { + ok: true, + actor: { + id: String(user.id), + email: typeof user.email === 'string' ? user.email : undefined, + }, + }; +} From d9a099951478b643d754a998856924dd6642105c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 10:20:42 +0000 Subject: [PATCH 2/2] test(qa): pin the platform-admin 2xx side on the re-mounted ban/unban routes Moves /admin/ban-user and /admin/unban-user from the better-auth-gate bucket (refusal side only) into objectstack-gate, which asserts the full contrast: anon 401 UNAUTHENTICATED, member 403 PERMISSION_DENIED, platform admin NOT refused. Adds the changeset. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --- .changeset/lucky-pugs-shave.md | 17 ++++++ ...min-route-nonadmin-refusal.dogfood.test.ts | 53 ++++++++++++++++--- 2 files changed, 64 insertions(+), 6 deletions(-) create mode 100644 .changeset/lucky-pugs-shave.md diff --git a/.changeset/lucky-pugs-shave.md b/.changeset/lucky-pugs-shave.md new file mode 100644 index 0000000000..cd40aa9277 --- /dev/null +++ b/.changeset/lucky-pugs-shave.md @@ -0,0 +1,17 @@ +--- +'@objectstack/plugin-auth': patch +--- + +Platform admins can ban and unban users again. + +`POST /api/v1/auth/admin/ban-user` and `POST /api/v1/auth/admin/unban-user` are +now served by ObjectStack with the ADR-0068 platform-admin gate instead of +better-auth's `admin` plugin, which authorizes on the legacy +`user.role === 'admin'` scalar that ADR-0068 D2 stopped synthesizing. On any +deployment with the admin plugin on (SCIM forces it, ADR-0071) the `sys_user` +Ban / Unban actions returned `403 YOU_ARE_NOT_ALLOWED_TO_BAN_USERS` for every +platform admin; they now succeed, and refuse a plain member with +`403 PERMISSION_DENIED` and an anonymous caller with `401 UNAUTHENTICATED`. + +The break-glass guard that refuses to ban the last local-password login is +unchanged and still applies. diff --git a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts index e4138be401..2723fa5676 100644 --- a/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts +++ b/packages/qa/dogfood/test/admin-route-nonadmin-refusal.dogfood.test.ts @@ -15,7 +15,9 @@ // HALF A — `honoApp.routes`, the ObjectStack raw mounts. `auth-plugin.ts` // registers these directly on the Hono app AHEAD of better-auth's catch-all // (`rawApp.all(`${basePath}/*`)`), so they shadow the vendor's handler and -// never appear in `auth.api`. Measured here: 9 routes. +// may or may not also appear in `auth.api` — `ban-user` / `unban-user` +// (#9652) shadow a vendor route of the same name and so appear in BOTH +// halves, which the union deduplicates. Measured here: 11 routes. // // HALF B — `auth.api`, the better-auth endpoint table. The catch-all publishes // whatever the vendor registers, so there are no per-route registration @@ -23,7 +25,7 @@ // `.options.method`, which is why `auth-route-ledger.conformance.test.ts` // uses the same seam. Measured here: 24 routes. // -// Union: 31 routes at the configuration this file boots (5 + 11 + 2 + 4 + 9). Both halves are +// Union: 31 routes at the configuration this file boots (7 + 9 + 2 + 4 + 9). Both halves are // asserted non-empty, and the union is cross-checked against a small ANCHOR set, // so a derivation that silently returns nothing cannot make the sweep vacuous. // @@ -63,14 +65,14 @@ // A refusal-only suite stays green if a route starts refusing EVERYONE, so each // bucket that can carry an allowed side does: // -// `objectstack-gate` (5 routes) — the full contrast. A plain member is refused +// `objectstack-gate` (7 routes) — the full contrast. A plain member is refused // 403 PERMISSION_DENIED, an anonymous caller 401 UNAUTHENTICATED, and the // platform admin is NOT refused: the same request reaches the handler and // comes back 2xx (unlock-user) or a SEMANTIC error (404 RESOURCE_NOT_FOUND // for a missing OAuth client). That is what proves the member's 403 is a // gate verdict and not a payload the server rejects for everyone. // -// `better-auth-gate` (11 routes) — refusal side only, DELIBERATELY. On this +// `better-auth-gate` (9 routes) — refusal side only, DELIBERATELY. On this // stack the platform admin is refused these routes too, with the same // `YOU_ARE_NOT_ALLOWED_TO_*` code as the member. That is not a harness // artifact: better-auth's admin plugin authorizes on the legacy @@ -91,6 +93,17 @@ // admin-is-also-refused behaviour would turn the fix red, and pinning the // fixed behaviour would be red today. #9482's report carries the finding. // +// #9652 measured the vendor's option surface at the installed 1.7.1 and +// found nothing that can express ObjectStack's predicate — `adminUserIds` +// is frozen at plugin construction, `adminRoles` matches only the +// persisted scalar, and `adminMiddleware` re-reads the session from the DB +// (`getAuthoritativeSessionFromCtx` nulls `ctx.context.session` first), so +// a session-scoped synthesis is discarded before the check runs. It moved +// the two routes `sys_user` actions call — `ban-user` / `unban-user` — +// onto ObjectStack mounts, where the allowed side IS pinned above. The +// nine below still answer the platform admin with the vendor's own +// `YOU_ARE_NOT_ALLOWED_*`; that is a known, filed gap, not drift. +// // `self-scoped` (2 routes) — `has-permission` and `stop-impersonating` answer // a non-admin without a refusal BY DESIGN, and the invariant is asserted in // the shape that actually holds: they must not leak a privileged result. @@ -231,9 +244,37 @@ function expectationsFor(targetUserId: string): Record body: { providerId: 'refusal-probe-oidc' }, }, + // ── #9652: ban / unban moved from the vendor to an ObjectStack mount ──── + // + // These two carried the `better-auth-gate` bucket — refusal side only, + // because the platform admin was refused too and pinning EITHER side would + // have been wrong. #9652 mounts them as ObjectStack raw routes with the + // ADR-0068 gate, so the allowed side now exists and IS pinned: this is the + // assertion that goes red if the gate ever stops admitting a platform + // admin whose `sys_user.role` is `'user'` — the identity a real deployment + // produces and the one the vendor plugin refuses. + // + // ORDER MATTERS and is load-bearing: the bucket loop walks its routes + // sorted, so `ban-user` fires before `unban-user` and the admin-side probe + // leaves the target unbanned again. The target holds a local credential + // and is not the only holder (the seeded admin and the probe member hold + // one each), so the break-glass guard permits the ban. + 'POST /api/v1/auth/admin/ban-user': { + bucket: 'objectstack-gate', + body: { userId: targetUserId, banReason: 'probe' }, + }, + 'POST /api/v1/auth/admin/unban-user': { + bucket: 'objectstack-gate', + body: { userId: targetUserId }, + }, + // ── better-auth admin plugin (legacy `role` scalar gate) ──────────────── - 'POST /api/v1/auth/admin/ban-user': { bucket: 'better-auth-gate', body: { userId: targetUserId, banReason: 'probe' } }, - 'POST /api/v1/auth/admin/unban-user': { bucket: 'better-auth-gate', body: { userId: targetUserId } }, + // + // Still refusal-side only, and still for the reason in the header: the + // platform admin is refused these too. #9652 fixed the two routes above + // (the ones `sys_user` actions call and whose handlers are faithfully + // re-implementable); the rest stay on the vendor's gate pending the + // maintainer's call on the remaining surface. 'POST /api/v1/auth/admin/set-role': { bucket: 'better-auth-gate', body: { userId: targetUserId, role: 'admin' } }, 'POST /api/v1/auth/admin/remove-user': { bucket: 'better-auth-gate', body: { userId: targetUserId } }, 'POST /api/v1/auth/admin/impersonate-user': { bucket: 'better-auth-gate', body: { userId: targetUserId } },