diff --git a/.changeset/25364-last-admin-guard-read-fault-code.md b/.changeset/25364-last-admin-guard-read-fault-code.md new file mode 100644 index 0000000000..49207d96d7 --- /dev/null +++ b/.changeset/25364-last-admin-guard-read-fault-code.md @@ -0,0 +1,16 @@ +--- +'@objectstack/plugin-auth': patch +--- + +fix(plugin-auth): the last-administrator guard answers a read it could not complete with 503 `SERVICE_UNAVAILABLE`, the code the sibling guards on the same doors answer their read faults with + +Clause-②: no +Metadata-protocol: none + +The last-administrator guard fails closed: when a read its verdict depends on throws, it refuses the write. That refusal answered 403 `PERMISSION_DENIED`, the same code as a refusal on the merits, so a caller could not tell "you may not" from "try again" by the code. The organization slug guard (`auth-manager.ts`) and the organization delete admission already answer their read faults with 503 `SERVICE_UNAVAILABLE`. + +- **Every half of the guard, one shape.** The shared fail-closed envelope and the Default Organization slug-capture rule now throw the same read fault: `code: 'SERVICE_UNAVAILABLE'`, `status: 503`, the read's own failure on `cause`. The write is still refused and nothing is written. A refusal on the merits, and an enumeration that completed but returned more rows than the guard can judge, keep 403 `PERMISSION_DENIED` unchanged. +- **The data API** answers the read fault 503 `SERVICE_UNAVAILABLE` with the server-fault message, as it does every declared 5xx. +- **better-auth's endpoints** (`organization/update`, the admin ban and remove-user endpoints, and every other endpoint that writes through the adapter) answer it 503 `SERVICE_UNAVAILABLE`. The adapter now maps any engine error that declares `status: 503` and `code: 'SERVICE_UNAVAILABLE'` to that answer; before, such an error reached better-auth's router as an unhandled 500 with an empty body. The driver's message is not relayed. +- **`organization/delete`** of the Default Organization answers the guard's read fault 503 `SERVICE_UNAVAILABLE`, the code the admission's own read faults answer with. A refusal on the merits there stays 403 `PERMISSION_DENIED`. +- **For a client:** branch on the code. `SERVICE_UNAVAILABLE` means retry once the data store is reachable; `PERMISSION_DENIED` means the write itself is refused. diff --git a/packages/plugins/plugin-auth/src/auth-manager-actor-guards.ts b/packages/plugins/plugin-auth/src/auth-manager-actor-guards.ts index 40241bfe91..a2dcd90cca 100644 --- a/packages/plugins/plugin-auth/src/auth-manager-actor-guards.ts +++ b/packages/plugins/plugin-auth/src/auth-manager-actor-guards.ts @@ -289,9 +289,26 @@ export async function assertOrganizationDeletable(this: Pick m.id)).toEqual(['mem_owner']); }); + it('a read the question cannot complete refuses with the read-fault code, once: an inner envelope\'s answer passes the outer one unchanged', async () => { + const engine = await promotedSingle(); + const real = engine.find.bind(engine); + vi.spyOn(engine, 'find').mockImplementation(async (object: string, ...rest: any[]) => { + if (object === 'sys_user_permission_set') throw new Error('sys_user_permission_set is unreadable'); + return (real as any)(object, ...rest); + }); + + const err = (await guard.judgeOrganizationDelete('org_default').catch((e: unknown) => e)) as Error & { code?: unknown; status?: unknown }; + vi.restoreAllMocks(); + expect(err).toMatchObject({ code: 'SERVICE_UNAVAILABLE', status: 503 }); + expect(err.message.match(/SERVICE_UNAVAILABLE:/g)).toHaveLength(1); + expect(err.message).toContain('sys_user_permission_set is unreadable'); + }); + it('CONTROL: with another administrator standing elsewhere, the delete is admitted', async () => { const engine = await promotedSingle({ secondOrganization: true, grantOwner: 'org_b' }); await engine.insert('sys_organization', { id: 'org_c', name: 'C', slug: 'c' }, SYSTEM); diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts index 827235b124..b4ba3920fb 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.test.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.test.ts @@ -590,7 +590,9 @@ describe('[#5892] the guard fails CLOSED — an unverifiable population refuses // because the guard could not prove it. await expect(ban(engine, 'usr_admin')).rejects.toThrow(/could not be verified/i); await expect(ban(engine, 'usr_admin')).rejects.toThrow(/sys_member is unreadable/); - await expect(ban(engine, 'usr_admin')).rejects.toMatchObject({ code: 'PERMISSION_DENIED' }); + // [ADR-0112] Refused, but no verdict: the read-fault code, not the 403 a + // refusal on the merits answers. + await expect(ban(engine, 'usr_admin')).rejects.toMatchObject({ code: 'SERVICE_UNAVAILABLE', status: 503 }); expect(await bannedFlag(engine, 'usr_admin')).toBeFalsy(); }); @@ -601,6 +603,9 @@ describe('[#5892] the guard fails CLOSED — an unverifiable population refuses await seedUser(engine, 'usr_admin', { role: 'admin' }); await expect(ban(engine, 'usr_admin')).rejects.toThrow(/more than 1 rows/); + // A read that COMPLETED and proved too large is a refusal with a remedy, + // not a read fault: it keeps the 403. + await expect(ban(engine, 'usr_admin')).rejects.toMatchObject({ code: 'PERMISSION_DENIED', status: 403 }); }); it('an environment with no administrator at all is not blocked (nothing to protect)', async () => { @@ -935,7 +940,8 @@ describe('[#5941] the delete guard fails CLOSED too', () => { await expect(removeUser(engine, 'usr_admin')).rejects.toThrow(/could not be verified/i); await expect(removeUser(engine, 'usr_admin')).rejects.toThrow(/sys_member is unreadable/); await expect(removeUser(engine, 'usr_admin')).rejects.toMatchObject({ - code: 'PERMISSION_DENIED', + code: 'SERVICE_UNAVAILABLE', + status: 503, }); expect(await userExists(engine, 'usr_admin')).toBe(true); }); @@ -1557,7 +1563,7 @@ describe('[#5978] the standing halves fail CLOSED', () => { await expect( engine.update('sys_member', { id: 'mem_usr_admin', role: 'member' }, SYSTEM), - ).rejects.toMatchObject({ code: 'PERMISSION_DENIED', object: 'sys_member' }); + ).rejects.toMatchObject({ code: 'SERVICE_UNAVAILABLE', status: 503, object: 'sys_member' }); expect(await memberRole(engine, 'mem_usr_admin')).toBe('admin'); }); diff --git a/packages/plugins/plugin-auth/src/last-admin-guard.ts b/packages/plugins/plugin-auth/src/last-admin-guard.ts index 0d4b23e5c2..be205d4b1c 100644 --- a/packages/plugins/plugin-auth/src/last-admin-guard.ts +++ b/packages/plugins/plugin-auth/src/last-admin-guard.ts @@ -314,6 +314,7 @@ * reach the identity tables reach this guard. */ +import type { StandardErrorCode } from '@objectstack/spec/api'; import type { BaseEngineOptions, EngineQueryOptions } from '@objectstack/spec/data'; import { ADMIN_FULL_ACCESS, @@ -636,6 +637,44 @@ function isRefusal(err: unknown): boolean { return (err as { code?: string } | null)?.code === 'PERMISSION_DENIED'; } +/** + * [ADR-0112] The code every half of this guard answers a read it could not + * complete with: the standard catalog's member for a store that cannot answer + * right now, the code the organization slug guard's read fault + * (`auth-manager.ts`, `slugGuardReadFaultApiError`) and the organization delete + * admission's read fault answer with too. One code for the fault across every + * guard on a door, so a caller tells "you may not" (403 `PERMISSION_DENIED`) + * from "try again" by the code alone. + */ +const LAST_ADMIN_GUARD_READ_FAULT_CODE: StandardErrorCode = 'SERVICE_UNAVAILABLE'; + +/** + * The fail-closed answer to a read the guard could not complete: 503 + * {@link LAST_ADMIN_GUARD_READ_FAULT_CODE}. Still a refusal of the write — the + * hook throws, so nothing is written — but no verdict about it, so it does not + * wear {@link refuse}'s 403. The read's own failure rides `cause`. `object` + * names the table the caller was writing, as {@link refuse}'s does. + */ +function readFault(message: string, object: string, cause: unknown): Error { + const err = new Error(`${LAST_ADMIN_GUARD_READ_FAULT_CODE}: ${message}`) as Error & { + code?: string; + status?: number; + object?: string; + cause?: unknown; + }; + err.code = LAST_ADMIN_GUARD_READ_FAULT_CODE; + err.status = 503; + err.object = object; + err.cause = cause; + return err; +} + +/** Marker so a nested fail-closed wrapper re-throws an inner read fault unchanged. */ +function isReadFault(err: unknown): boolean { + const e = err as { code?: unknown; status?: unknown } | null; + return e?.code === LAST_ADMIN_GUARD_READ_FAULT_CODE && e.status === 503; +} + const BREAK_GLASS_CITATION = 'break-glass invariant, ADR-0135 D5.2 — an environment must always keep at least one ' + 'administrator who can sign in'; @@ -945,7 +984,7 @@ function touchesAny(data: Record, keys: readonly string[]): boo * slug is an engine update: better-auth's `organization/update` (its adapter * writes through the engine, and the refusal reaches the caller as 403 * `PERMISSION_DENIED`), a seed, a plugin, the data API. A read that throws - * propagates to the hook, which fails closed. + * propagates to the hook, which fails closed with 503 `SERVICE_UNAVAILABLE`. */ async function refuseDefaultOrganizationSlugCapture( engine: Pick, @@ -1573,24 +1612,30 @@ export function registerLastAdminGuard( * The fail-CLOSED envelope every half runs inside. A lookup that throws is * not "probably fine": the guard could not prove another administrator * survives, and the cost of guessing wrong is a permanently locked-out - * environment. A deliberate refusal from inside passes through unchanged. + * environment. A deliberate refusal from inside passes through unchanged, + * and so does a read fault an inner envelope already answered. + * + * The write is refused either way; only the code tells the two apart: a + * verdict is 403 `PERMISSION_DENIED` ({@link refuse}), a lookup that could + * not complete is 503 `SERVICE_UNAVAILABLE` ({@link readFault}). */ const failClosed = async (op: GuardedOp, judge: () => Promise): Promise => { const words = OP_WORDS[op]; try { await judge(); } catch (err) { - if (isRefusal(err)) throw err; + if (isRefusal(err) || isReadFault(err)) throw err; const reason = (err as Error)?.message ?? String(err); logger?.warn( `[LastAdminGuard] administrator lookup failed — ${words.noun} refused: ${reason}`, ); - throw refuse( + throw readFault( `Refusing this ${words.noun}: the remaining administrators could not be verified ` + `(${reason}). This guard fails closed — a ${words.noun} is only permitted when at ` + `least one other unbanned administrator is provably left (${BREAK_GLASS_CITATION}). ` + 'Retry once the identity tables are readable again.', words.table, + err, ); } }; @@ -1920,7 +1965,8 @@ export function registerLastAdminGuard( * ({@link refuseDefaultOrganizationSlugCapture}). A payload naming another * slug, or none, costs no reads. Fails closed like every half: the rows a * predicate write addresses, or the Default Organization itself, that cannot - * be read refuse the write. + * be read refuse the write, with the same 503 read-fault code + * ({@link readFault}) as {@link failClosed}. */ const guardOrganizationDefaultSlug = async (rawCtx: unknown): Promise => { const ctx = ctxOf(rawCtx); @@ -1931,14 +1977,15 @@ export function registerLastAdminGuard( const ids = await resolveTargetIds('organization-update', SystemObjectName.ORGANIZATION, ctx.input?.id, ctx.input?.options, data); await refuseDefaultOrganizationSlugCapture(engine, ids, data.slug); } catch (err) { - if (isRefusal(err)) throw err; + if (isRefusal(err) || isReadFault(err)) throw err; const reason = (err as Error)?.message ?? String(err); - throw refuse( + throw readFault( `Refusing this organization update: the organizations it addresses, or the Default Organization, ` + `could not be read (${reason}), so whether the slug '${DEFAULT_ORGANIZATION_SLUG}' would make ` + 'another organization the Default Organization is unknown (ADR-0131 D5). Retry once ' + `'${SystemObjectName.ORGANIZATION}' is readable again.`, SystemObjectName.ORGANIZATION, + err, ); } }; diff --git a/packages/plugins/plugin-auth/src/objectql-adapter.ts b/packages/plugins/plugin-auth/src/objectql-adapter.ts index 5ed0db8960..18a81e34a1 100644 --- a/packages/plugins/plugin-auth/src/objectql-adapter.ts +++ b/packages/plugins/plugin-auth/src/objectql-adapter.ts @@ -546,6 +546,27 @@ function isEnginePolicyRefusal(err: unknown): err is { code?: string; message?: return (err as { code?: unknown }).code === 'PERMISSION_DENIED'; } +/** + * A declared READ FAULT — an engine-level guard that could not complete a read + * its verdict depends on, and so refused the write without reaching one + * (`SERVICE_UNAVAILABLE`, 503, ADR-0112). The concrete case is the + * last-administrator guard's fail-closed envelope (`last-admin-guard.ts`, + * `readFault`): without this arm its refusal reached better-auth's router as an + * unhandled fault, a 500 with an empty body. + * + * Read by the declared `code` AND `status`, both, the way `mapDataError`'s + * declared-5xx relay reads them for the REST data routes, so the two + * transports answer the same engine error with the same status and code. The + * engine's prose is not relayed, as that relay withholds a declared fault's: + * the guard's message names the driver's own failure, which is for the server + * log, not the caller. + */ +function isEngineReadFault(err: unknown): boolean { + if (!err || typeof err !== 'object') return false; + const e = err as { code?: unknown; status?: unknown }; + return e.code === 'SERVICE_UNAVAILABLE' && e.status === 503; +} + /** * [#7724] A REFERENTIAL refusal — the engine's `cascadeDeleteRelations` found * dependent rows it may neither cascade nor null, so it vetoed the delete @@ -607,9 +628,9 @@ function wireDependents(raw: unknown): Array<{ object: string; count?: number }> /** * Re-throw `err` as a better-auth `APIError` when it is an ObjectQL validation - * failure (400), an engine policy refusal (403) or a referential delete - * restriction (409); otherwise re-throw it verbatim. Always throws — the return - * type is `never`. + * failure (400), an engine policy refusal (403), a referential delete + * restriction (409) or a declared engine read fault (503); otherwise re-throw + * it verbatim. Always throws — the return type is `never`. */ async function rethrowAsBetterAuthError(err: unknown): Promise { if (isObjectQLValidationError(err)) { @@ -634,6 +655,17 @@ async function rethrowAsBetterAuthError(err: unknown): Promise { code: 'PERMISSION_DENIED', }); } + if (isEngineReadFault(err)) { + const { APIError } = await import('better-auth/api'); + const apiErr = new APIError('SERVICE_UNAVAILABLE', { + message: + 'This request could not be completed: the data store could not answer a read it depends on. ' + + 'This is a server-side outage, not a refusal of the request: retry once the data store is reachable.', + code: 'SERVICE_UNAVAILABLE', + }); + (apiErr as { cause?: unknown }).cause = err; + throw apiErr; + } if (isReferentialDeleteRestriction(err)) { const { APIError } = await import('better-auth/api'); throw new APIError('CONFLICT', { @@ -655,9 +687,9 @@ async function rethrowAsBetterAuthError(err: unknown): Promise { /** * Wrap every function-valued method of a better-auth adapter so an ObjectQL - * `ValidationError` (400), an engine policy refusal (403) or a referential - * delete restriction (409, #7724) thrown from the underlying engine surfaces as - * a 4xx `APIError` instead of an opaque 500. Non-function properties pass + * `ValidationError` (400), an engine policy refusal (403), a referential + * delete restriction (409, #7724) or a declared engine read fault (503) thrown + * from the underlying engine surfaces as an `APIError` instead of an opaque 500. Non-function properties pass * through untouched, and every error that carries none of those signatures is * re-thrown verbatim. */ diff --git a/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts b/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts index a0016d00c7..3fc5a5c67f 100644 --- a/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts +++ b/packages/plugins/plugin-auth/src/organization-delete-read-fault.test.ts @@ -46,6 +46,7 @@ import { ObjectQL } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; import { AuthManager } from './auth-manager.js'; import { authIdentityObjects } from './manifest.js'; +import { registerLastAdminGuard, type LastAdminGuardEngine } from './last-admin-guard.js'; const BASE = 'http://localhost:3000'; const AUTH = `${BASE}/api/v1/auth`; @@ -328,3 +329,47 @@ describe('[#25059] an organization-delete read that fails refuses 503 before bet await expectNothingChanged(engine, attempted); }); }); + +describe('[#25364] the last-administrator guard\'s question about the Default Organization fails on a read: the same 503, not the verdict\'s 403', () => { + /** + * `TARGET` made the Default Organization (the only one), with the real + * last-administrator guard asked first, as `AuthPlugin` wires it. + */ + async function ownerOfDefaultOrganization() { + const setup = await ownerOfTarget(); + await setup.engine.delete('sys_organization', { where: { id: 'org_other' }, ...(SYSTEM as object) } as never); + const guard = registerLastAdminGuard(setup.engine as unknown as LastAdminGuardEngine, { packageId: 'test.25364.last-admin-guard' }); + setup.manager.setOrganizationDeleteStandingJudge(guard.judgeOrganizationDelete); + return setup; + } + + it('the guard\'s membership read fails: 503 SERVICE_UNAVAILABLE, the driver\'s words withheld, nothing attempted; CONTROL: with it back, the guard reaches its verdict, 403 PERMISSION_DENIED', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const { engine, manager, cookie } = await ownerOfDefaultOrganization(); + // The guard reads `sys_member` through `find`; the admission's own read of + // it is a `findOne`, so only the guard's read fails. + const fault = failAdmissionRead(engine, { methods: ['find'], object: 'sys_member', times: Number.POSITIVE_INFINITY }); + const attempted = recordAttemptedWrites(engine); + + const res = await post(manager, '/organization/delete', { organizationId: TARGET }, cookie); + engine.unregisterHooksByPackage(RECORDER); + fault.restore(); + + expect(fault.fired(), 'PREMISE: the guard\'s read really failed').toBeGreaterThan(0); + const text = await res.clone().text(); + expect(res.status, text).toBe(503); + expect((JSON.parse(text) as { code?: string }).code).toBe('SERVICE_UNAVAILABLE'); + expect(text).not.toContain('injected'); + await expectNothingChanged(engine, attempted); + + // CONTROL: the read back, the same door answers the verdict: this owner is + // the last administrator, a refusal on the merits, unchanged. + const again = recordAttemptedWrites(engine); + const verdict = await post(manager, '/organization/delete', { organizationId: TARGET }, cookie); + engine.unregisterHooksByPackage(RECORDER); + const verdictText = await verdict.clone().text(); + expect(verdict.status, verdictText).toBe(403); + expect((JSON.parse(verdictText) as { code?: string }).code).toBe('PERMISSION_DENIED'); + await expectNothingChanged(engine, again); + }); +}); diff --git a/packages/plugins/plugin-auth/src/single-reslug-default-anchor.test.ts b/packages/plugins/plugin-auth/src/single-reslug-default-anchor.test.ts index edf0d4fcf0..160777b92d 100644 --- a/packages/plugins/plugin-auth/src/single-reslug-default-anchor.test.ts +++ b/packages/plugins/plugin-auth/src/single-reslug-default-anchor.test.ts @@ -30,7 +30,10 @@ * - the HTTP door: 403 `PERMISSION_DENIED`, the slug unchanged, no standing * gained; the sole organization (already the Default Organization) and a * slug other than `default` are admitted; a walled posture admits it, since - * a slug anchors nothing there; a read that fails refuses; + * a slug anchors nothing there; a read that fails refuses with the read-fault + * code, 503 `SERVICE_UNAVAILABLE` — the code `auth-manager.ts`'s + * organization slug guard answers its own read fault with, so one door + * answers one code for the fault; * - the engine door, by id and by predicate: the same refusal, for a * system-context write; walled, admitted. */ @@ -217,7 +220,7 @@ describe('[ADR-0131 D5] better-auth organization/update: no re-slug makes anothe expect(await slugOf(engine, 'org_b')).toBe('default'); }); - it('a read of the Default Organization that fails refuses (the guard fails closed), and the slug is not changed', async () => { + it('a read of the Default Organization that fails refuses (the guard fails closed) with 503 SERVICE_UNAVAILABLE, not the verdict\'s 403, and the slug is not changed', async () => { const { engine, manager, cookie } = await ownerOfB(TWO_NONE_DEFAULT); // Only this rule's read (`{ slug: 'default' }`, limit 1) fails: the // break-glass half reads the same slug through a wider page, and must not @@ -228,7 +231,11 @@ describe('[ADR-0131 D5] better-auth organization/update: no re-slug makes anothe return (real as any)(object, query, ...rest); }); - expect(await answer(await reslug(manager, cookie, 'default'))).toEqual({ status: 403, code: 'PERMISSION_DENIED' }); + const res = await reslug(manager, cookie, 'default'); + expect(await answer(res)).toEqual({ status: 503, code: 'SERVICE_UNAVAILABLE' }); + // The driver's own failure is for the server log, not the caller. + expect(await res.clone().text()).not.toContain('injected'); + vi.restoreAllMocks(); expect(await slugOf(engine, 'org_b')).toBe('beta'); }); }); @@ -260,7 +267,7 @@ describe('[ADR-0131 D5] the engine door (a seed, a plugin, a script): the same r expect(await slugOf(engine, 'org_b')).toBe('beta'); }); - it('a read that fails refuses: the guard fails closed', async () => { + it('a read that fails refuses: the guard fails closed, with 503 SERVICE_UNAVAILABLE rather than the verdict\'s 403', async () => { const { engine } = await ownerOfB(TWO_NONE_DEFAULT); const real = engine.find.bind(engine); vi.spyOn(engine, 'find').mockImplementation(async (object: string, query?: any, ...rest: any[]) => { @@ -268,7 +275,7 @@ describe('[ADR-0131 D5] the engine door (a seed, a plugin, a script): the same r return (real as any)(object, query, ...rest); }); - expect(await refusalOf(engine.update('sys_organization', { id: 'org_b', slug: 'default' }, SYSTEM))).toEqual({ code: 'PERMISSION_DENIED', status: 403 }); + expect(await refusalOf(engine.update('sys_organization', { id: 'org_b', slug: 'default' }, SYSTEM))).toEqual({ code: 'SERVICE_UNAVAILABLE', status: 503 }); vi.restoreAllMocks(); expect(await slugOf(engine, 'org_b')).toBe('beta'); });