Skip to content

Commit 735f850

Browse files
os-zhuangclaude
andauthored
fix(security): resolve the issuer's real grants when authorizing invitation placement (#3695)
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. Claude-Session: https://claude.ai/code/session_015FebXPaaGrLhGKw1LHPbpL Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3177d51 commit 735f850

5 files changed

Lines changed: 134 additions & 18 deletions

File tree

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
"@objectstack/plugin-security": patch
3+
"@objectstack/plugin-auth": patch
4+
---
5+
6+
fix(security): resolve the ISSUER's real grants when authorizing invitation
7+
placement (ADR-0105 D8)
8+
9+
Scoped-invitation issuance dry-runs `DelegatedAdminGate` against the
10+
`sys_user_position` rows the acceptance would write. The gate reads authority
11+
off `context.positions` / `context.permissions` — but the invitation hook
12+
handed it a hand-built `{ userId, tenantId }`, which carries neither. Every
13+
delegated administrator therefore resolved to the additive baseline alone and
14+
was refused:
15+
16+
> requires tenant-level administration or a delegated adminScope (ADR-0090 D12)
17+
18+
Fail-closed, but dead: only a tenant admin could ever issue a placement, which
19+
is the one case the feature was not for. Caught by cloud's group-posture
20+
dogfood, which exercises the real HTTP path with a real delegate.
21+
22+
`assertIssuable` now takes `actorUserId` instead of a caller-built
23+
`actorContext` and resolves that user's grants itself through the single authz
24+
resolver (`@objectstack/core` `resolveUserAuthzGrants`) — the same envelope a
25+
transport would have carried, from the same reads. There is no request to
26+
resolve a context from inside a better-auth hook, so the id is what the caller
27+
can honestly supply and the resolution belongs behind the boundary.
28+
29+
A principal-less call still reaches the gate with an empty context on purpose:
30+
the gate owns that refusal too, so the security boundary keeps exactly one
31+
place an issuance can be denied.

packages/plugins/plugin-auth/src/auth-manager.test.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -750,9 +750,13 @@ describe('AuthManager', () => {
750750
});
751751

752752
expect(assertIssuable).toHaveBeenCalledTimes(1);
753+
// WHO issued, not a context: the service resolves the issuer's real
754+
// grants through the shared authz resolver. Handing it a hand-built
755+
// context was the #3663 defect — it carried no positions, so the gate
756+
// refused every delegate.
753757
expect(assertIssuable).toHaveBeenCalledWith({
754758
intent: { businessUnitId: 'bu_plant_a', positions: ['qc_inspector'] },
755-
actorContext: { userId: 'u_issuer', tenantId: 'org-42' },
759+
actorUserId: 'u_issuer',
756760
organizationId: 'org-42',
757761
});
758762
});

packages/plugins/plugin-auth/src/auth-manager.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,8 @@ export function isOAuthEligibleBaseUrl(url: string): boolean {
248248
export interface InvitationPlacementServiceLike {
249249
assertIssuable(args: {
250250
intent: { businessUnitId: string; positions: string[] };
251-
actorContext: unknown;
251+
/** The ISSUER's user id — the service resolves their grants itself. */
252+
actorUserId?: string | null;
252253
organizationId?: string | null;
253254
}): Promise<void>;
254255
apply(args: {
@@ -1696,9 +1697,11 @@ export class AuthManager {
16961697
try {
16971698
await svc.assertIssuable({
16981699
intent,
1699-
// The gate resolves the issuer's permission sets from this
1700-
// context, exactly as the CRUD middleware would.
1701-
actorContext: { userId: inviter?.id, tenantId: organizationId },
1700+
// WHO is issuing — not a context. The service resolves this
1701+
// user's real grants through the shared authz resolver; a
1702+
// hand-built context here would carry no positions and refuse
1703+
// every delegate.
1704+
actorUserId: inviter?.id ?? inviter?.userId ?? null,
17021705
organizationId,
17031706
});
17041707
} catch (err: any) {

packages/plugins/plugin-security/src/invitation-placement.test.ts

Lines changed: 60 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,33 @@ import {
1919

2020
const INTENT = { businessUnitId: 'bu_plant_a', positions: ['qc_inspector', 'line_lead'] };
2121

22-
function makeService(overrides: { assert?: any; ql?: any } = {}) {
23-
const assert = overrides.assert ?? vi.fn(async () => {});
24-
const ql = overrides.ql ?? {
22+
/**
23+
* A `find`-capable engine stub. `assertIssuable` resolves the issuer's grants
24+
* through the shared authz resolver, which reads the identity tables — so the
25+
* stub answers per-object like the real thing (unknown tables → empty, matching
26+
* the resolver's fail-closed reads).
27+
*/
28+
function makeQl(tables: Record<string, any[]> = {}) {
29+
return {
30+
find: vi.fn(async (object: string, opts: any = {}) => {
31+
const rows = tables[object] ?? [];
32+
const where = opts?.where ?? {};
33+
return rows.filter((row) =>
34+
Object.entries(where).every(([k, v]) =>
35+
v && typeof v === 'object' && '$in' in (v as any)
36+
? (v as any).$in.includes(row[k])
37+
: row[k] === v,
38+
),
39+
);
40+
}),
2541
findOne: vi.fn(async () => null),
2642
insert: vi.fn(async (_o: string, row: any) => ({ id: 'row', ...row })),
2743
};
44+
}
45+
46+
function makeService(overrides: { assert?: any; ql?: any } = {}) {
47+
const assert = overrides.assert ?? vi.fn(async () => {});
48+
const ql = overrides.ql ?? makeQl();
2849
const svc = createInvitationPlacementService({ ql, gate: { assert }, logger: { info: vi.fn() } });
2950
return { svc, assert, ql };
3051
}
@@ -58,15 +79,14 @@ describe('readPlacementIntent', () => {
5879
describe('assertIssuable — the gate decides, verbatim', () => {
5980
it('dry-runs the gate against the sys_user_position rows the acceptance would write', async () => {
6081
const { svc, assert } = makeService();
61-
const actorContext = { userId: 'u_issuer', tenantId: 'org_1' };
6282

63-
await svc.assertIssuable({ intent: INTENT, actorContext, organizationId: 'org_1' });
83+
await svc.assertIssuable({ intent: INTENT, actorUserId: 'u_issuer', organizationId: 'org_1' });
6484

6585
expect(assert).toHaveBeenCalledTimes(1);
6686
const opCtx = assert.mock.calls[0][0];
6787
expect(opCtx.object).toBe('sys_user_position');
6888
expect(opCtx.operation).toBe('insert');
69-
expect(opCtx.context).toBe(actorContext);
89+
expect(opCtx.context).toMatchObject({ userId: 'u_issuer', tenantId: 'org_1' });
7090
// One row per requested position, each anchored to the target unit — the
7191
// exact shape `assertAssignmentWrite` boundary-checks.
7292
expect(opCtx.data).toEqual([
@@ -75,24 +95,56 @@ describe('assertIssuable — the gate decides, verbatim', () => {
7595
]);
7696
});
7797

98+
// ── Regression (#3663): the gate reads authority off `context.positions` /
99+
// `context.permissions`. Issuance used to hand it a hand-built
100+
// `{ userId, tenantId }`, which resolves to the additive baseline and
101+
// nothing else — so EVERY delegate was refused and the whole feature was
102+
// fail-closed but dead. The issuer's grants must be RESOLVED, not assumed.
103+
it("carries the issuer's real grants — a delegate's adminScope reaches the gate", async () => {
104+
const ql = makeQl({
105+
sys_user: [{ id: 'u_issuer', email: 'plant.admin@x.test' }],
106+
sys_member: [{ user_id: 'u_issuer', organization_id: 'org_1', role: 'member' }],
107+
sys_user_position: [
108+
{ user_id: 'u_issuer', position: 'plant_admin', organization_id: 'org_1' },
109+
],
110+
});
111+
const { svc, assert } = makeService({ ql });
112+
113+
await svc.assertIssuable({ intent: INTENT, actorUserId: 'u_issuer', organizationId: 'org_1' });
114+
115+
const ctx = assert.mock.calls[0][0].context;
116+
expect(ctx.positions).toContain('plant_admin');
117+
// The gate's own set resolution turns positions into permission sets — the
118+
// wiring's job is only to make sure they are THERE to resolve.
119+
expect(Array.isArray(ctx.permissions)).toBe(true);
120+
});
121+
78122
it('propagates the gate refusal — an unauthorized placement is not swallowed', async () => {
79123
const assert = vi.fn(async () => {
80124
throw new Error("business unit 'bu_plant_b' is outside the delegated subtree");
81125
});
82126
const { svc } = makeService({ assert });
83127
await expect(
84-
svc.assertIssuable({ intent: INTENT, actorContext: { userId: 'u' }, organizationId: 'org_1' }),
128+
svc.assertIssuable({ intent: INTENT, actorUserId: 'u', organizationId: 'org_1' }),
85129
).rejects.toThrow(/outside the delegated subtree/);
86130
});
87131

88132
it('does not stamp organization_id when the deployment has no org context (single posture)', async () => {
89133
const { svc, assert } = makeService();
90-
await svc.assertIssuable({ intent: INTENT, actorContext: { userId: 'u' }, organizationId: null });
134+
await svc.assertIssuable({ intent: INTENT, actorUserId: 'u', organizationId: null });
91135
expect(assert.mock.calls[0][0].data[0]).toEqual({
92136
position: 'qc_inspector',
93137
business_unit_id: 'bu_plant_a',
94138
});
95139
});
140+
141+
it('an issuer-less call reaches the gate with no principal — it owns that refusal', async () => {
142+
const { svc, assert } = makeService();
143+
await svc.assertIssuable({ intent: INTENT, actorUserId: null, organizationId: 'org_1' });
144+
// Empty context ⇒ the gate's principal-less branch denies. Deciding here
145+
// instead would put a second refusal path on the security boundary.
146+
expect(assert.mock.calls[0][0].context).toEqual({});
147+
});
96148
});
97149

98150
describe('apply — accept-time placement', () => {

packages/plugins/plugin-security/src/invitation-placement.ts

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@
3434
* no placement — fail closed, never a silently unchecked assignment).
3535
*/
3636

37+
import { resolveUserAuthzGrants } from '@objectstack/core';
38+
3739
const SYSTEM_CTX = { isSystem: true } as const;
3840

3941
/** The kernel service name plugin-auth probes. */
@@ -54,8 +56,13 @@ export interface InvitationPlacementService {
5456
*/
5557
assertIssuable(args: {
5658
intent: InvitationPlacementIntent;
57-
/** The ISSUER's execution context — authority is judged at issuance time. */
58-
actorContext: unknown;
59+
/**
60+
* The ISSUER's user id — authority is judged at issuance time, and the
61+
* grants behind it are resolved HERE (see below). A caller-supplied
62+
* context is deliberately not accepted: an invitation hook has no request
63+
* to resolve one from, and a hand-built one silently carries no grants.
64+
*/
65+
actorUserId?: string | null;
5966
organizationId?: string | null;
6067
}): Promise<void>;
6168

@@ -132,16 +139,35 @@ export function createInvitationPlacementService(
132139
}));
133140

134141
return {
135-
async assertIssuable({ intent, actorContext, organizationId }) {
142+
async assertIssuable({ intent, actorUserId, organizationId }) {
143+
// Resolve the issuer's REAL grants through the one authz resolver. The
144+
// gate judges authority from `context.positions` / `context.permissions`
145+
// (`resolvePermissionSetsForContext`), so a hand-built `{ userId,
146+
// tenantId }` resolves to the additive baseline and NOTHING else — every
147+
// delegate would be refused, and the feature would be fail-closed but
148+
// dead. There is no request here to resolve a context from, so we ask the
149+
// userId-driven half of the shared resolver for exactly the envelope a
150+
// transport would have carried.
151+
const userId = typeof actorUserId === 'string' && actorUserId !== '' ? actorUserId : null;
152+
const grants = userId
153+
? await resolveUserAuthzGrants(ql, userId, {
154+
tenantId: organizationId ?? undefined,
155+
})
156+
: null;
157+
136158
// Dry-run the REAL gate against the REAL operation shape. No user_id is
137159
// supplied — the invitee may not even have an account yet, and
138160
// `assertAssignmentWrite` judges the unit + the positions' bound sets,
139-
// never the target principal.
161+
// never the target principal. A principal-less call passes an empty
162+
// context on purpose: the gate owns that refusal too, so there is exactly
163+
// one place an issuance can be denied.
140164
await gate.assert({
141165
object: 'sys_user_position',
142166
operation: 'insert',
143167
data: rowsFor(intent, organizationId ? { organization_id: organizationId } : {}),
144-
context: actorContext,
168+
context: grants
169+
? { ...grants, userId, ...(organizationId ? { tenantId: organizationId } : {}) }
170+
: {},
145171
});
146172
},
147173

0 commit comments

Comments
 (0)