From c505c21c694246710f14eab5e58660b5bc10c41b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 14:43:10 +0000 Subject: [PATCH] fix(security): resolve the issuer's real grants when authorizing invitation placement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scoped-invitation issuance (ADR-0105 D8) dry-runs `DelegatedAdminGate` against the `sys_user_position` rows the acceptance would write. The gate reads authority off `context.positions` / `context.permissions` (`resolvePermissionSetsForContext`) — but `beforeCreateInvitation` handed it a hand-built `{ userId, tenantId }`, which carries neither. Every delegated administrator resolved to the additive baseline alone and was refused with "requires tenant-level administration or a delegated adminScope". Fail-closed, but dead: only a tenant admin could issue a placement, which is the one caller the feature was not for. Caught by cloud's group-posture dogfood driving the real HTTP path with a real delegate. `assertIssuable` now takes `actorUserId` and resolves that user's grants itself via `@objectstack/core` `resolveUserAuthzGrants` — the userId-driven half of the single authz resolver, producing the same envelope a transport would have carried, from the same reads. A better-auth hook has no request to resolve a context from, so the id is what a caller can honestly supply and the resolution belongs behind the service boundary rather than in front of it. A principal-less call still reaches the gate with an empty context: the gate owns that refusal too, so there stays exactly one place an issuance is denied. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015FebXPaaGrLhGKw1LHPbpL --- ...0105-d8-issuance-resolves-issuer-grants.md | 31 +++++++++ .../plugin-auth/src/auth-manager.test.ts | 6 +- .../plugins/plugin-auth/src/auth-manager.ts | 11 +-- .../src/invitation-placement.test.ts | 68 ++++++++++++++++--- .../src/invitation-placement.ts | 36 ++++++++-- 5 files changed, 134 insertions(+), 18 deletions(-) create mode 100644 .changeset/adr-0105-d8-issuance-resolves-issuer-grants.md diff --git a/.changeset/adr-0105-d8-issuance-resolves-issuer-grants.md b/.changeset/adr-0105-d8-issuance-resolves-issuer-grants.md new file mode 100644 index 0000000000..5b39fdb909 --- /dev/null +++ b/.changeset/adr-0105-d8-issuance-resolves-issuer-grants.md @@ -0,0 +1,31 @@ +--- +"@objectstack/plugin-security": patch +"@objectstack/plugin-auth": patch +--- + +fix(security): resolve the ISSUER's real grants when authorizing invitation +placement (ADR-0105 D8) + +Scoped-invitation issuance dry-runs `DelegatedAdminGate` against the +`sys_user_position` rows the acceptance would write. The gate reads authority +off `context.positions` / `context.permissions` — but the invitation hook +handed it a hand-built `{ userId, tenantId }`, which carries neither. Every +delegated administrator therefore resolved to the additive baseline alone and +was refused: + +> requires tenant-level administration or a delegated adminScope (ADR-0090 D12) + +Fail-closed, but dead: only a tenant admin could ever issue a placement, which +is the one case the feature was not for. Caught by cloud's group-posture +dogfood, which exercises the real HTTP path with a real delegate. + +`assertIssuable` now takes `actorUserId` instead of a caller-built +`actorContext` and resolves that user's grants itself through the single authz +resolver (`@objectstack/core` `resolveUserAuthzGrants`) — the same envelope a +transport would have carried, from the same reads. There is no request to +resolve a context from inside a better-auth hook, so the id is what the caller +can honestly supply and the resolution belongs behind the boundary. + +A principal-less call still reaches the gate with an empty context on purpose: +the gate owns that refusal too, so the security boundary keeps exactly one +place an issuance can be denied. diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index ed298b8bc0..914f0cc9df 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -750,9 +750,13 @@ describe('AuthManager', () => { }); expect(assertIssuable).toHaveBeenCalledTimes(1); + // WHO issued, not a context: the service resolves the issuer's real + // grants through the shared authz resolver. Handing it a hand-built + // context was the #3663 defect — it carried no positions, so the gate + // refused every delegate. expect(assertIssuable).toHaveBeenCalledWith({ intent: { businessUnitId: 'bu_plant_a', positions: ['qc_inspector'] }, - actorContext: { userId: 'u_issuer', tenantId: 'org-42' }, + actorUserId: 'u_issuer', organizationId: 'org-42', }); }); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index e86dcdc26f..5f7e11e705 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -248,7 +248,8 @@ export function isOAuthEligibleBaseUrl(url: string): boolean { export interface InvitationPlacementServiceLike { assertIssuable(args: { intent: { businessUnitId: string; positions: string[] }; - actorContext: unknown; + /** The ISSUER's user id — the service resolves their grants itself. */ + actorUserId?: string | null; organizationId?: string | null; }): Promise; apply(args: { @@ -1696,9 +1697,11 @@ export class AuthManager { try { await svc.assertIssuable({ intent, - // The gate resolves the issuer's permission sets from this - // context, exactly as the CRUD middleware would. - actorContext: { userId: inviter?.id, tenantId: organizationId }, + // WHO is issuing — not a context. The service resolves this + // user's real grants through the shared authz resolver; a + // hand-built context here would carry no positions and refuse + // every delegate. + actorUserId: inviter?.id ?? inviter?.userId ?? null, organizationId, }); } catch (err: any) { diff --git a/packages/plugins/plugin-security/src/invitation-placement.test.ts b/packages/plugins/plugin-security/src/invitation-placement.test.ts index f34013a82c..b9119aa464 100644 --- a/packages/plugins/plugin-security/src/invitation-placement.test.ts +++ b/packages/plugins/plugin-security/src/invitation-placement.test.ts @@ -19,12 +19,33 @@ import { const INTENT = { businessUnitId: 'bu_plant_a', positions: ['qc_inspector', 'line_lead'] }; -function makeService(overrides: { assert?: any; ql?: any } = {}) { - const assert = overrides.assert ?? vi.fn(async () => {}); - const ql = overrides.ql ?? { +/** + * A `find`-capable engine stub. `assertIssuable` resolves the issuer's grants + * through the shared authz resolver, which reads the identity tables — so the + * stub answers per-object like the real thing (unknown tables → empty, matching + * the resolver's fail-closed reads). + */ +function makeQl(tables: Record = {}) { + return { + find: vi.fn(async (object: string, opts: any = {}) => { + const rows = tables[object] ?? []; + const where = opts?.where ?? {}; + return rows.filter((row) => + Object.entries(where).every(([k, v]) => + v && typeof v === 'object' && '$in' in (v as any) + ? (v as any).$in.includes(row[k]) + : row[k] === v, + ), + ); + }), findOne: vi.fn(async () => null), insert: vi.fn(async (_o: string, row: any) => ({ id: 'row', ...row })), }; +} + +function makeService(overrides: { assert?: any; ql?: any } = {}) { + const assert = overrides.assert ?? vi.fn(async () => {}); + const ql = overrides.ql ?? makeQl(); const svc = createInvitationPlacementService({ ql, gate: { assert }, logger: { info: vi.fn() } }); return { svc, assert, ql }; } @@ -58,15 +79,14 @@ describe('readPlacementIntent', () => { describe('assertIssuable — the gate decides, verbatim', () => { it('dry-runs the gate against the sys_user_position rows the acceptance would write', async () => { const { svc, assert } = makeService(); - const actorContext = { userId: 'u_issuer', tenantId: 'org_1' }; - await svc.assertIssuable({ intent: INTENT, actorContext, organizationId: 'org_1' }); + await svc.assertIssuable({ intent: INTENT, actorUserId: 'u_issuer', organizationId: 'org_1' }); expect(assert).toHaveBeenCalledTimes(1); const opCtx = assert.mock.calls[0][0]; expect(opCtx.object).toBe('sys_user_position'); expect(opCtx.operation).toBe('insert'); - expect(opCtx.context).toBe(actorContext); + expect(opCtx.context).toMatchObject({ userId: 'u_issuer', tenantId: 'org_1' }); // One row per requested position, each anchored to the target unit — the // exact shape `assertAssignmentWrite` boundary-checks. expect(opCtx.data).toEqual([ @@ -75,24 +95,56 @@ describe('assertIssuable — the gate decides, verbatim', () => { ]); }); + // ── Regression (#3663): the gate reads authority off `context.positions` / + // `context.permissions`. Issuance used to hand it a hand-built + // `{ userId, tenantId }`, which resolves to the additive baseline and + // nothing else — so EVERY delegate was refused and the whole feature was + // fail-closed but dead. The issuer's grants must be RESOLVED, not assumed. + it("carries the issuer's real grants — a delegate's adminScope reaches the gate", async () => { + const ql = makeQl({ + sys_user: [{ id: 'u_issuer', email: 'plant.admin@x.test' }], + sys_member: [{ user_id: 'u_issuer', organization_id: 'org_1', role: 'member' }], + sys_user_position: [ + { user_id: 'u_issuer', position: 'plant_admin', organization_id: 'org_1' }, + ], + }); + const { svc, assert } = makeService({ ql }); + + await svc.assertIssuable({ intent: INTENT, actorUserId: 'u_issuer', organizationId: 'org_1' }); + + const ctx = assert.mock.calls[0][0].context; + expect(ctx.positions).toContain('plant_admin'); + // The gate's own set resolution turns positions into permission sets — the + // wiring's job is only to make sure they are THERE to resolve. + expect(Array.isArray(ctx.permissions)).toBe(true); + }); + it('propagates the gate refusal — an unauthorized placement is not swallowed', async () => { const assert = vi.fn(async () => { throw new Error("business unit 'bu_plant_b' is outside the delegated subtree"); }); const { svc } = makeService({ assert }); await expect( - svc.assertIssuable({ intent: INTENT, actorContext: { userId: 'u' }, organizationId: 'org_1' }), + svc.assertIssuable({ intent: INTENT, actorUserId: 'u', organizationId: 'org_1' }), ).rejects.toThrow(/outside the delegated subtree/); }); it('does not stamp organization_id when the deployment has no org context (single posture)', async () => { const { svc, assert } = makeService(); - await svc.assertIssuable({ intent: INTENT, actorContext: { userId: 'u' }, organizationId: null }); + await svc.assertIssuable({ intent: INTENT, actorUserId: 'u', organizationId: null }); expect(assert.mock.calls[0][0].data[0]).toEqual({ position: 'qc_inspector', business_unit_id: 'bu_plant_a', }); }); + + it('an issuer-less call reaches the gate with no principal — it owns that refusal', async () => { + const { svc, assert } = makeService(); + await svc.assertIssuable({ intent: INTENT, actorUserId: null, organizationId: 'org_1' }); + // Empty context ⇒ the gate's principal-less branch denies. Deciding here + // instead would put a second refusal path on the security boundary. + expect(assert.mock.calls[0][0].context).toEqual({}); + }); }); describe('apply — accept-time placement', () => { diff --git a/packages/plugins/plugin-security/src/invitation-placement.ts b/packages/plugins/plugin-security/src/invitation-placement.ts index 8215a0f098..18bb3a689b 100644 --- a/packages/plugins/plugin-security/src/invitation-placement.ts +++ b/packages/plugins/plugin-security/src/invitation-placement.ts @@ -34,6 +34,8 @@ * no placement — fail closed, never a silently unchecked assignment). */ +import { resolveUserAuthzGrants } from '@objectstack/core'; + const SYSTEM_CTX = { isSystem: true } as const; /** The kernel service name plugin-auth probes. */ @@ -54,8 +56,13 @@ export interface InvitationPlacementService { */ assertIssuable(args: { intent: InvitationPlacementIntent; - /** The ISSUER's execution context — authority is judged at issuance time. */ - actorContext: unknown; + /** + * The ISSUER's user id — authority is judged at issuance time, and the + * grants behind it are resolved HERE (see below). A caller-supplied + * context is deliberately not accepted: an invitation hook has no request + * to resolve one from, and a hand-built one silently carries no grants. + */ + actorUserId?: string | null; organizationId?: string | null; }): Promise; @@ -132,16 +139,35 @@ export function createInvitationPlacementService( })); return { - async assertIssuable({ intent, actorContext, organizationId }) { + async assertIssuable({ intent, actorUserId, organizationId }) { + // Resolve the issuer's REAL grants through the one authz resolver. The + // gate judges authority from `context.positions` / `context.permissions` + // (`resolvePermissionSetsForContext`), so a hand-built `{ userId, + // tenantId }` resolves to the additive baseline and NOTHING else — every + // delegate would be refused, and the feature would be fail-closed but + // dead. There is no request here to resolve a context from, so we ask the + // userId-driven half of the shared resolver for exactly the envelope a + // transport would have carried. + const userId = typeof actorUserId === 'string' && actorUserId !== '' ? actorUserId : null; + const grants = userId + ? await resolveUserAuthzGrants(ql, userId, { + tenantId: organizationId ?? undefined, + }) + : null; + // Dry-run the REAL gate against the REAL operation shape. No user_id is // supplied — the invitee may not even have an account yet, and // `assertAssignmentWrite` judges the unit + the positions' bound sets, - // never the target principal. + // never the target principal. A principal-less call passes an empty + // context on purpose: the gate owns that refusal too, so there is exactly + // one place an issuance can be denied. await gate.assert({ object: 'sys_user_position', operation: 'insert', data: rowsFor(intent, organizationId ? { organization_id: organizationId } : {}), - context: actorContext, + context: grants + ? { ...grants, userId, ...(organizationId ? { tenantId: organizationId } : {}) } + : {}, }); },