diff --git a/.changeset/26641-explain-unknown-user-refused.md b/.changeset/26641-explain-unknown-user-refused.md new file mode 100644 index 00000000000..b1e6ab62767 --- /dev/null +++ b/.changeset/26641-explain-unknown-user-refused.md @@ -0,0 +1,19 @@ +--- +'@objectstack/plugin-security': patch +--- + +fix(plugin-security)!: security explain refuses a `userId` that no `sys_user` record has with `404 RECORD_NOT_FOUND`, naming the id, instead of explaining the baseline principal the resolver builds for any id + +Clause-②: no (narrowing) — `POST` / `GET /security/explain` and the in-process `explain()` refuse a request naming another user whose id no `sys_user` record has, which they answered with a verdict before. Nothing is widened. +Metadata-protocol: none + + + +**BREAKING** accept-set narrowing at the access-explanation door (`@objectstack/plugin-security`), served at `/security/explain` on both transports. + +- **Before.** An administrator explaining another user by an id that no `sys_user` record has (a typo, a deleted user, an id from another deployment) was answered `200` with a full verdict, usually `allowed: true`: the authorization resolver gives any id the baseline position and permission sets. `GET /data/sys_user/ID` answered 404 for the same id. +- **Now.** That request is refused `404 RECORD_NOT_FOUND`, and the message names the id. The id is read through the same `sys_user` existence read the on-behalf-of delegator path uses. +- **Unchanged.** The order of checks: an anonymous caller is refused 401, and a caller who may not explain others gets the same 403 `PERMISSION_DENIED` whether the id exists or not, before any existence read. Explaining yourself (no `userId`, or your own) and explaining a user who exists answer as before. +- **Who sees it.** A caller holding `manage_users` (or a system caller) who names an id that does not exist. An organization administrator or a delegated administrator naming such an id still gets the 403: no membership or scope covers a user who does not exist. + +**The remedy.** None needed: pick the user from the directory, or check the id. diff --git a/content/docs/permissions/explain.mdx b/content/docs/permissions/explain.mdx index 65a92153b00..0891f53efdd 100644 --- a/content/docs/permissions/explain.mdx +++ b/content/docs/permissions/explain.mdx @@ -96,6 +96,11 @@ rule: `adminScope` whose business-unit subtree covers that user (ADR-0090 D12): an administrator who can rewire a user's grants may read why they resolve the way they do — and *only* a covering administrator. Anything else → 403. +- **A `userId` no user has** — once the caller may explain that user, an id + with no `sys_user` record is refused `404 RECORD_NOT_FOUND`, naming the id, + instead of being explained as a baseline member. The 403 above comes first, + so a caller who may not explain others cannot use explain to learn which ids + exist. A deployment without `@objectstack/plugin-security` answers 501. diff --git a/packages/plugins/plugin-security/src/errors.ts b/packages/plugins/plugin-security/src/errors.ts index 7d2f3fbfe90..e0a0192b6f4 100644 --- a/packages/plugins/plugin-security/src/errors.ts +++ b/packages/plugins/plugin-security/src/errors.ts @@ -341,6 +341,45 @@ export class MaskedValueWriteError extends Error { } } +/** + * [#26641] `explain` was asked about ANOTHER user, by an id no `sys_user` row + * has → `404 RECORD_NOT_FOUND`, naming the id. + * + * The explainer builds the explained principal through the authorization + * resolver, which synthesises the baseline anchor and sets for ANY id. So an + * id that names nobody (a typo, a deleted user, an id from another deployment) + * was explained as a baseline member and answered `200 allowed: true`: a + * verdict about a person who does not exist. As with an unknown object + * ({@link ExplainObjectNotFoundError}), nothing can be evaluated, so the honest + * answer is a refusal, not a verdict. + * + * `RECORD_NOT_FOUND` at 404 is the standard catalog's pair for "the record with + * this id does not exist", the answer `GET /data/sys_user/ID` gives for the + * same id, so it needs no ledger row. The REST door serves it through its + * classified-refusal arm, which keeps a declared 4xx `code` and `status`. + * + * Only a caller already admitted to explain someone else reaches this check, + * so it tells nobody new which ids exist: an anonymous caller is refused 401 + * at the door and a caller who may not explain others gets their 403 before + * any existence read. The prefix is not the `[Security] Access denied` + * matcher, and the class carries both `status` and `statusCode`, for the + * reasons the classes above record. + */ +export class ExplainUserNotFoundError extends Error { + readonly code = 'RECORD_NOT_FOUND'; + readonly status = 404; + readonly statusCode = 404; + readonly userId: string; + constructor(userId: string) { + super( + `[Security] Unknown user: explain was asked about user '${userId}', but no sys_user record has that id. ` + + `No principal was evaluated, so this is NOT an access decision.`, + ); + this.name = 'ExplainUserNotFoundError'; + this.userId = userId; + } +} + /** * [#18253] `explain` was asked about an object name that no declaration on this * runtime answers to → `404 OBJECT_NOT_FOUND`. diff --git a/packages/plugins/plugin-security/src/explain-engine.ts b/packages/plugins/plugin-security/src/explain-engine.ts index 68a18ded016..4431460519a 100644 --- a/packages/plugins/plugin-security/src/explain-engine.ts +++ b/packages/plugins/plugin-security/src/explain-engine.ts @@ -790,6 +790,24 @@ export async function buildContextForUser( }; } +/** + * [#26641] The `sys_user` row a user id names, read under the system context, + * or `null` when no row has that id. The one existence read of a user the + * explainer makes: {@link resolveDelegatorContext} asks it of a delegator, and + * the explain API (`explainAccessForCaller`) asks it of the user it was asked + * to explain. Each needs it for the same reason: {@link buildContextForUser} + * resolves a principal for ANY id, because the resolver synthesises the + * baseline anchor and sets for an id no row has. + * + * A read that throws is not absence, so this function does not catch: each + * caller decides what a failed read means. The delegator path fails closed and + * reads it as `missing`; the explain API lets it propagate as a fault rather + * than answer that the user does not exist. + */ +export async function readSysUserRow(ql: any, userId: string): Promise { + return (await ql.findOne('sys_user', { where: { id: userId }, context: SYSTEM_CTX })) ?? null; +} + /** * [ADR-0090 D10] Result of resolving the delegator behind an on-behalf-of * principal. `none` = no delegation link; `missing` = the link names a user that @@ -864,7 +882,7 @@ export async function resolveDelegatorContext( if (!oboId) return { kind: 'none' }; let user: any = null; try { - user = await ql.findOne('sys_user', { where: { id: oboId }, context: SYSTEM_CTX }); + user = await readSysUserRow(ql, oboId); } catch { user = null; } diff --git a/packages/plugins/plugin-security/src/org-admin-assignment-boundary.posture.test.ts b/packages/plugins/plugin-security/src/org-admin-assignment-boundary.posture.test.ts index d693260a09d..5da72f0ccdb 100644 --- a/packages/plugins/plugin-security/src/org-admin-assignment-boundary.posture.test.ts +++ b/packages/plugins/plugin-security/src/org-admin-assignment-boundary.posture.test.ts @@ -247,6 +247,11 @@ describe('[ADR-0090 D12, amended 2026-10-17] the organization administrator assi it('the delegable-scope report offers the org admin its in-boundary positions only, and explain admits it for its member', async () => { const h = await boot(posture); const security = h.plugin as any; + // [#26641] Explain answers only for a user a `sys_user` row carries, so + // the explained member gets one here (the boot leaves the table + // unprovisioned for the write cases above). + await h.engine.syncObjectSchema(SysUser.name); + await h.engine.insert('sys_user', { id: 'u_target', name: 'Target', email: 'u_target@example.test' }, { context: { isSystem: true } } as any); for (const context of [CALLERS.orgAdmin, CALLERS.orgAdminNoBypass]) { const sets = await security.resolvePermissionSetsForContext(context); const report = await security.delegatedAdminGate.describeDelegableScope(sets, context); diff --git a/packages/plugins/plugin-security/src/security-plugin-explain-access.ts b/packages/plugins/plugin-security/src/security-plugin-explain-access.ts index f8d950eb052..5f067ae283f 100644 --- a/packages/plugins/plugin-security/src/security-plugin-explain-access.ts +++ b/packages/plugins/plugin-security/src/security-plugin-explain-access.ts @@ -72,7 +72,7 @@ import { OWNERSHIP_QUEUE_SERVICE, type AuthoredRowWriteOperation, type IOwnershi import type { ExplainDecision, ExplainOperation, PermissionSet } from '@objectstack/spec/security'; import type { SecurityPlugin } from './security-plugin.js'; import { absentUnderCallerRead, requiredCapsForOperation, unreadableUnderCallerRead } from './security-plugin.js'; -import { PermissionDeniedError } from './errors.js'; +import { ExplainUserNotFoundError, PermissionDeniedError } from './errors.js'; import { grantRowPolicySpeaks } from './external-owd-grant-stamp.js'; import { rlsOperationForVerb } from './lifecycle-verb-rls-operation.js'; import { @@ -80,6 +80,7 @@ import { delegatedOrganizationRefusal, explainAccessForAsker, intersectDelegatedScope, + readSysUserRow, resolveDelegatorContext, } from './explain-engine.js'; @@ -164,6 +165,15 @@ export async function explainAccessForCaller( } } } + // [#26641] ...and only a user who exists is explained. The resolver + // synthesises the baseline anchor and sets for ANY id, so an id no + // `sys_user` row has was explained as a baseline member, `allowed: true`. + // Asked here, after the caller is admitted and never before, so a caller + // who may not explain others learns nothing about which ids exist; read + // through the delegator path's own existence read (`readSysUserRow`). + if (!(await readSysUserRow(this.ql, request.userId))) { + throw new ExplainUserNotFoundError(request.userId); + } // [#20515] Resolved in the CALLER's organization — the one the explain // right above was checked in. Grants scoped to any other organization do // not apply there, and with no active organization only global grants do. diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 8497e2b5af9..e576f421bb3 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -3621,6 +3621,68 @@ describe('explainAccessForCaller (ADR-0090 D6/D12)', () => { expect(self.principal.userId).toBe('u_east_1'); }); + // [#26641] Explaining ANOTHER user by an id no `sys_user` row has is refused + // `404 RECORD_NOT_FOUND`, naming the id: the resolver synthesises a baseline + // principal for any id, so it used to answer a verdict about nobody. The + // existence read runs only after the caller is admitted to explain others. + describe('[#26641] an id no sys_user row has', () => { + const GHOST = 'u_no_such_user_26641'; + /** The ids `sys_user` was asked about, in order. */ + const userReads = (plugin: any): string[] => { + const seen: string[] = []; + const ql = (plugin as any).ql; + const findOne = ql.findOne.bind(ql); + ql.findOne = async (object: string, opts: any) => { + if (object === 'sys_user') seen.push(String(opts?.where?.id)); + return findOne(object, opts); + }; + return seen; + }; + + it('a caller holding manage_users is refused 404 RECORD_NOT_FOUND naming the id', async () => { + const { plugin } = await boot(); + const caller = { userId: 'u_hr', positions: [], permissions: ['hr_admin'] }; + for (const operation of ['read', 'create']) { + const err: any = await plugin + .explainAccessForCaller({ object: 'task', operation, userId: GHOST }, caller) + .then(() => null, (e) => e); + expect(err, `${operation}: refused, not answered`).toMatchObject({ + name: 'ExplainUserNotFoundError', code: 'RECORD_NOT_FOUND', status: 404, statusCode: 404, userId: GHOST, + }); + expect(err.message).toContain(`'${GHOST}'`); + } + }); + + it('a caller who may not explain others gets their 403 first, and no existence read is made', async () => { + const { plugin } = await boot(); + const seen = userReads(plugin); + for (const caller of [ + { userId: 'u_east_1', positions: [], permissions: [] }, + { userId: 'u_delegate', positions: [], permissions: ['sub_admin'] }, + ]) { + await expect( + plugin.explainAccessForCaller({ object: 'task', operation: 'read', userId: GHOST }, caller), + `${caller.userId}: the same refusal an existing user outside their reach gets`, + ).rejects.toMatchObject({ name: 'PermissionDeniedError', code: 'PERMISSION_DENIED' }); + } + expect(seen, 'the unknown id is never looked up for a caller refused the right').not.toContain(GHOST); + }); + + it('an existing user is still explained, and a self-explanation is never refused for its id', async () => { + const { plugin } = await boot(); + const hr = { userId: 'u_hr', positions: [], permissions: ['hr_admin'] }; + const other = await plugin.explainAccessForCaller({ object: 'task', operation: 'read', userId: 'u_west_1' }, hr); + expect(other.principal.userId).toBe('u_west_1'); + // Explaining yourself is unchanged: no `userId`, or your own, explains the + // request's own context, whether or not a row carries its id. + const ghostSelf = { userId: GHOST, positions: [], permissions: [] }; + for (const request of [{}, { userId: GHOST }]) { + const self = await plugin.explainAccessForCaller({ object: 'task', operation: 'read', ...request }, ghostSelf); + expect(self.principal.userId).toBe(GHOST); + } + }); + }); + // [#20515] Explaining ANOTHER user resolves that user's grants in the CALLER's // organization — the one the explain right was checked in. A grant scoped to // an organization applies only while that organization is the one resolved diff --git a/packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts b/packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts new file mode 100644 index 00000000000..7ad7e34e193 --- /dev/null +++ b/packages/qa/dogfood/test/explain-unknown-user.dogfood.test.ts @@ -0,0 +1,122 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #26641 — `POST /security/explain` naming a `userId` that no `sys_user` row + * has is refused `404 RECORD_NOT_FOUND`, naming the id, on the showcase boot. + * + * ## The defect + * + * The explainer builds the explained principal through the authorization + * resolver, which synthesises the baseline anchor and sets for ANY id. So the + * seed admin explaining `no_such_user_qa_123` was answered `200 allowed: true` + * with principal `positions: ["everyone"]` and the member baseline sets, while + * `GET /data/sys_user/no_such_user_qa_123` answered 404: a verdict about a + * person who does not exist. + * + * ## The order of checks, pinned with the refusal + * + * The existence read runs only after the caller is admitted to explain someone + * else, so the explain door cannot be used to learn which ids exist: + * + * - `[anonymous]` no session → 401, whatever id the body names. + * - `[unprivileged]` a plain member naming the unknown id gets the same 403 + * `PERMISSION_DENIED` it gets naming a user who exists. + * - `[unknown]` the seed admin naming the unknown id → 404 `RECORD_NOT_FOUND` + * naming it, for `read` and `create`, consistent with the data door's 404. + * - `[existing]` the seed admin explaining the member who exists is answered + * as before (200, the principal is that member), and explaining yourself is + * unchanged. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { type VerifyStack } from '@objectstack/verify'; +import { bootShowcase } from './showcase-boot.js'; +import { showcaseAppDefaultSecurity } from './showcase-security.js'; + +const SYS = { isSystem: true } as const; +const OBJECT = 'showcase_task'; +const GHOST = 'no_such_user_qa_123'; +const MEMBER_EMAIL = 'explain-unknown-user.member.26641@verify.test'; + +describe('#26641: security explain refuses a userId no sys_user row has', () => { + let stack: VerifyStack; + let adminTok = ''; + let memberTok = ''; + let adminId = ''; + let memberId = ''; + + /** `POST /security/explain` as whoever holds `token` (`null` = no session). */ + const explain = async (token: string | null, body: Record) => { + const res = token === null + ? await stack.api('/security/explain', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }) + : await stack.apiAs(token, 'POST', '/security/explain', body); + const text = await res.text(); + let json: any = null; + try { json = JSON.parse(text); } catch { /* the status still reports */ } + return { status: res.status, text, json }; + }; + + beforeAll(async () => { + stack = await bootShowcase({ security: showcaseAppDefaultSecurity() }); + adminTok = await stack.signIn(); // the seeded admin first, so the sign-up below is a plain member + memberTok = await stack.signUp(MEMBER_EMAIL); + const ql: any = await stack.kernel.getServiceAsync('objectql'); + memberId = String((await ql.findOne('sys_user', { where: { email: MEMBER_EMAIL }, context: SYS }))?.id ?? ''); + expect(memberId, 'the signed-up member exists').toBeTruthy(); + expect(await ql.findOne('sys_user', { where: { id: GHOST }, context: SYS }), 'no row carries the probe id').toBeFalsy(); + const me = await explain(adminTok, { object: OBJECT, operation: 'read' }); + expect(me.status, `admin self-explain: ${me.text}`).toBe(200); + adminId = String(me.json?.principal?.userId ?? ''); + expect(adminId, 'the admin self-explanation names its principal').toBeTruthy(); + }, 300_000); + + afterAll(async () => { + await stack?.stop?.(); + }); + + it('[control] the data door answers 404 for the same id', async () => { + const res = await stack.apiAs(adminTok, 'GET', `/data/sys_user/${GHOST}`); + expect(res.status).toBe(404); + }); + + it('[unknown] the seed admin is refused 404 RECORD_NOT_FOUND naming the id, for read and create', async () => { + for (const operation of ['read', 'create']) { + const r = await explain(adminTok, { object: OBJECT, operation, userId: GHOST }); + expect(r.status, `${operation}: ${r.text}`).toBe(404); + expect(r.json?.success, r.text).toBe(false); + expect(r.json?.error?.code, r.text).toBe('RECORD_NOT_FOUND'); + expect(String(r.json?.error?.message ?? ''), r.text).toContain(GHOST); + expect(r.json?.allowed, 'no verdict is answered').toBeUndefined(); + } + }); + + it('[anonymous] no session is refused 401 before anything about the id', async () => { + const r = await explain(null, { object: OBJECT, operation: 'read', userId: GHOST }); + expect(r.status, r.text).toBe(401); + }); + + it('[unprivileged] a plain member gets the same 403 for the unknown id as for a user who exists', async () => { + const unknown = await explain(memberTok, { object: OBJECT, operation: 'read', userId: GHOST }); + const existing = await explain(memberTok, { object: OBJECT, operation: 'read', userId: adminId }); + for (const [label, r] of [['unknown id', unknown], ['existing user', existing]] as const) { + expect(r.status, `${label}: ${r.text}`).toBe(403); + expect(r.json?.error?.code, `${label}: ${r.text}`).toBe('PERMISSION_DENIED'); + } + }); + + it('[existing] the seed admin explaining the member who exists, and anyone explaining themselves, is answered', async () => { + const other = await explain(adminTok, { object: OBJECT, operation: 'read', userId: memberId }); + expect(other.status, other.text).toBe(200); + expect(other.json?.principal?.userId).toBe(memberId); + expect(typeof other.json?.allowed).toBe('boolean'); + for (const body of [{ object: OBJECT, operation: 'read' }, { object: OBJECT, operation: 'read', userId: memberId }]) { + const self = await explain(memberTok, body); + expect(self.status, self.text).toBe(200); + expect(self.json?.principal?.userId).toBe(memberId); + } + }); +}); diff --git a/packages/rest/src/security-explain-envelope.test.ts b/packages/rest/src/security-explain-envelope.test.ts index 69c69c527b9..bbbf3d8cc75 100644 --- a/packages/rest/src/security-explain-envelope.test.ts +++ b/packages/rest/src/security-explain-envelope.test.ts @@ -255,6 +255,24 @@ describe('[#8073] /security/explain — every refusal arm answers the ADR-0112 D expectNestedEnvelope(await api.explainGet(), 404, 'OBJECT_NOT_FOUND'); }); + /** + * [#26641] Explaining ANOTHER user by an id no `sys_user` row has: the + * engine REFUSES (`ExplainUserNotFoundError`, plugin-security `errors.ts`) + * instead of explaining the baseline principal the resolver synthesises + * for any id. `RECORD_NOT_FOUND` at 404 is the standard catalog's pair, + * declared with its status, so it reaches the classified-refusal arm + * (#20603) and needs no arm of its own. + */ + it('404 RECORD_NOT_FOUND — the user named does not exist', async () => { + const unknown = Object.assign( + new Error("[Security] Unknown user: explain was asked about user 'no_such_user', but no sys_user record has that id."), + { code: 'RECORD_NOT_FOUND', status: 404, statusCode: 404, name: 'ExplainUserNotFoundError', userId: 'no_such_user' }, + ); + const api = boot(throwingExplain(unknown)); + expectNestedEnvelope(await api.explainPost(), 404, 'RECORD_NOT_FOUND'); + expectNestedEnvelope(await api.explainGet(), 404, 'RECORD_NOT_FOUND'); + }); + it('404 is matched on the DECLARED code alone — no name, still not a 500', async () => { // The door does not import plugin-security (it is not a dependency), so // the thrown SHAPE is the contract (#8016). A producer that declares