Skip to content

Commit 4dc0454

Browse files
committed
plugin-security: pin the catalog loader's three answers and its fail-closed warn path; the warn names the catalog
Claude-Session: https://claude.ai/code/session_01VHtcG1CjvZzd7U6F9Yg8oN Co-authored-by: Claude <noreply@anthropic.com>
1 parent d5af7e1 commit 4dc0454

4 files changed

Lines changed: 187 additions & 11 deletions

File tree

‎content/docs/permissions/authorization.mdx‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -454,9 +454,12 @@ Deactivation is an incident-response control, so what it does **not** touch is
454454
deliberate. Administration surfaces keep listing and editing a deactivated
455455
position — an admin must still be able to unbind and clean up what they just
456456
switched off — and the write gates that judge audience-anchor bindings and a
457-
delegated administrator's blast radius keep reading every row, deactivated
458-
included: dropping rows there would make a refused binding *permitted* and a
459-
delegate's boundary *narrower*, which is the opposite of switching access off.
457+
delegated administrator's blast radius do not consult the activation ledger:
458+
they judge every binding and assignment in front of them — including the
459+
permission sets a position's definition names — whether or not the ledger
460+
switched it off, because skipping a switched-off one would make a refused
461+
binding *permitted* and a delegate's boundary *narrower*, which is the
462+
opposite of switching access off.
460463

461464
## Governance: how "declared = enforced" is kept true
462465

‎packages/plugins/plugin-security/src/permission-evaluator.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -481,8 +481,8 @@ export class PermissionEvaluator {
481481
bootstrapPermissionSets: PermissionSet[] = [],
482482
/**
483483
* Optional async loader for permission set names that aren't found in
484-
* metadata or bootstrap. Lets callers query user-defined permission
485-
* sets persisted in `sys_permission_set`. Failures are swallowed
484+
* metadata or bootstrap — the security plugin passes its catalog loader
485+
* (the security catalog's definitions, ADR-0131 D3/D4). Failures are swallowed
486486
* (fail-closed: unresolvable sets grant nothing) but SURFACED via
487487
* `options.logger` — see #2565: without the warn, a transient DB error
488488
* makes custom permission sets silently vanish and the resulting 403s
@@ -533,10 +533,10 @@ export class PermissionEvaluator {
533533
}
534534
}
535535

536-
// Last-resort: query user-defined permission sets from the database.
537-
// Without this, custom permission sets (created via the admin UI as
538-
// `sys_permission_set` rows) would be silently ignored both for CRUD
539-
// enforcement and for field-level masking.
536+
// Last source: the loader the caller supplies — the security plugin's
537+
// catalog loader, which answers environment-authored and package sets the
538+
// metadata list does not (ADR-0131 D3/D4). Without it those sets would be
539+
// silently ignored both for CRUD enforcement and for field-level masking.
540540
if (dbLoader) {
541541
const unresolved = identifiers.filter((n) => !seen.has(n));
542542
if (unresolved.length > 0) {
@@ -555,7 +555,7 @@ export class PermissionEvaluator {
555555
// DB error silently drops custom permission sets and the
556556
// resulting 403s point nowhere near the cause (#2565).
557557
options.logger?.warn?.(
558-
'[security] sys_permission_set db lookup failed — unresolved sets grant nothing this request',
558+
'[security] permission-set catalog lookup failed — unresolved sets grant nothing this request',
559559
{ unresolved, error: (e as Error)?.message },
560560
);
561561
}

‎packages/plugins/plugin-security/src/security-plugin.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2950,7 +2950,7 @@ describe('PermissionEvaluator', () => {
29502950
expect(result).toEqual([psAdmin]);
29512951
// ...but the swallow is surfaced, naming the unresolved sets
29522952
expect(warns.length).toBe(1);
2953-
expect(warns[0].msg).toContain('db lookup failed');
2953+
expect(warns[0].msg).toContain('catalog lookup failed');
29542954
expect(warns[0].meta?.unresolved).toEqual(['custom_sales']);
29552955
expect(warns[0].meta?.error).toBe('db down');
29562956
});
Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [ADR-0131 D3/D4] The security plugin's third permission-set source — the
5+
* catalog loader (`SecurityPlugin.catalogSetLoader`) — pinned the way the row
6+
* loader it replaced was pinned (`engine-find-bare-array.pin.test.ts` block 7,
7+
* #15598), against the post-cutover read.
8+
*
9+
* The loader answers the names the metadata list and the bootstrap sets did
10+
* not: each name's definition in the security catalog the plugin binds to its
11+
* engine. Three facts must never leave it as one value:
12+
*
13+
* - the catalog holds the set → the definition, WHOLE;
14+
* - the catalog holds no set of that name (or no catalog is bound) → nothing;
15+
* - the catalog read did NOT answer → a THROW (`AuthzStoreUnavailableError`),
16+
* never "no such set". `PermissionEvaluator.resolvePermissionSets` catches it
17+
* one frame up: the unresolved sets grant nothing (fail closed) and the
18+
* #2565 warn names the failure, once.
19+
*
20+
* And it reads no `sys_permission_set` row at all: a set only a row carries
21+
* resolves nothing, whatever the row says.
22+
*/
23+
24+
import { describe, it, expect, vi } from 'vitest';
25+
import {
26+
AUTHZ_STORE_UNAVAILABLE_CODE,
27+
AUTHZ_STORE_UNAVAILABLE_STATUS,
28+
AuthzStoreUnavailableError,
29+
} from '@objectstack/core';
30+
import type { PermissionSet } from '@objectstack/spec/security';
31+
import { assertEngineFindOnePredicate, type EngineFindOneQueryInput } from '@objectstack/metadata-core';
32+
33+
import { SecurityPlugin } from './security-plugin.js';
34+
import { PermissionEvaluator } from './permission-evaluator.js';
35+
36+
/** An environment-authored set, as the engine registry holds it. */
37+
const CATALOG_SET: PermissionSet = {
38+
name: 'ps_catalog',
39+
label: 'Catalog authored',
40+
objects: { deal: { allowRead: true, allowEdit: true } },
41+
fields: { 'deal.amount': { readable: true, editable: false } },
42+
systemPermissions: ['setup.access'],
43+
tabPermissions: { app_crm: 'visible' },
44+
} as any;
45+
46+
interface Fixture {
47+
/** What the registry's by-name read does: answer from `CATALOG_SET`, or throw. */
48+
registryThrows?: Error;
49+
/** `sys_permission_set` rows — present to prove they are never read. */
50+
rows?: Array<Record<string, unknown>>;
51+
/** Boot with no registry on the engine: no catalog gets bound. */
52+
noRegistry?: boolean;
53+
}
54+
55+
/** Boot the real plugin over a double engine and hand back its private catalog loader. */
56+
async function loaderOver(fixture: Fixture = {}) {
57+
let rowReads = 0;
58+
const ql: any = {
59+
registerMiddleware: () => {},
60+
getSchema: () => null,
61+
findOne: async (object: string, query?: EngineFindOneQueryInput) => { assertEngineFindOnePredicate(object, query); return null; },
62+
// No table answers here: every set is the catalog's. Only a read that
63+
// asks for a set the loader was asked about counts — the boot's own
64+
// background passes (the org-admin backfill, the baseline binder) read
65+
// their own names and race the call.
66+
find: async (object: string, query?: unknown) => {
67+
if (object === 'sys_permission_set' && /ps_catalog|ps_row_only/.test(JSON.stringify(query ?? {}))) rowReads += 1;
68+
return [];
69+
},
70+
};
71+
if (!fixture.noRegistry) {
72+
ql.registry = {
73+
getItem: (type: string, name: string) => {
74+
if (fixture.registryThrows) throw fixture.registryThrows;
75+
return type === 'permission' && name === CATALOG_SET.name ? CATALOG_SET : undefined;
76+
},
77+
listItems: (type: string) => (type === 'permission' ? [CATALOG_SET] : []),
78+
isPackageDisabled: () => false,
79+
};
80+
}
81+
const warn = vi.fn();
82+
const ctx: any = {
83+
logger: { info: vi.fn(), warn, error: vi.fn(), debug: vi.fn() },
84+
registerService: vi.fn(),
85+
getService: (name: string) => {
86+
if (name === 'objectql') return ql;
87+
if (name === 'metadata') return { get: async () => null, list: async () => [] };
88+
if (name === 'manifest') return { register: vi.fn() };
89+
throw new Error(`service not registered: ${name}`);
90+
},
91+
};
92+
const plugin = new SecurityPlugin();
93+
await plugin.init(ctx);
94+
await plugin.start(ctx);
95+
const loader = (plugin as any).catalogSetLoader as ((names: string[]) => Promise<PermissionSet[]>) | undefined;
96+
expect(typeof loader, 'the catalog loader was never built — the boot bailed out').toBe('function');
97+
rowReads = 0; // boot-time reads are not the loader's
98+
return { loader: loader!, rowReads: () => rowReads };
99+
}
100+
101+
describe('[ADR-0131 D3/D4] the catalog loader — what it answers', () => {
102+
it('a set the catalog holds comes back WHOLE, and no sys_permission_set row is read', async () => {
103+
const { loader, rowReads } = await loaderOver();
104+
const sets = await loader(['ps_catalog']);
105+
expect(sets).toEqual([CATALOG_SET]);
106+
expect(rowReads()).toBe(0);
107+
});
108+
109+
it('a name the catalog does not hold answers nothing — an EMPTY answer is a real answer', async () => {
110+
const { loader } = await loaderOver();
111+
await expect(loader(['ps_nowhere'])).resolves.toEqual([]);
112+
});
113+
114+
it('a set only a sys_permission_set ROW carries resolves nothing, and the row is never read', async () => {
115+
const { loader, rowReads } = await loaderOver({
116+
rows: [{ name: 'ps_row_only', object_permissions: JSON.stringify({ deal: { allowDelete: true } }) }],
117+
});
118+
await expect(loader(['ps_row_only'])).resolves.toEqual([]);
119+
expect(rowReads()).toBe(0);
120+
});
121+
122+
it('a name asked twice is answered once', async () => {
123+
const { loader } = await loaderOver();
124+
expect((await loader(['ps_catalog', 'ps_catalog'])).map((s) => s.name)).toEqual(['ps_catalog']);
125+
});
126+
127+
it('an engine with no catalog bound answers nothing, never a guess', async () => {
128+
const { loader } = await loaderOver({ noRegistry: true });
129+
await expect(loader(['ps_catalog'])).resolves.toEqual([]);
130+
});
131+
});
132+
133+
describe('[ADR-0131 D3/D4, #2565] a catalog read that did not answer', () => {
134+
it('PROPAGATES as the reader\'s own loud failure instead of reading as "no such set"', async () => {
135+
const { loader } = await loaderOver({ registryThrows: new Error("Datasource 'primary' is declared but not connected") });
136+
// ⛔ Not `toThrow()` alone: the envelope is the assertion.
137+
const err = await loader(['ps_catalog']).then(() => null, (e: unknown) => e);
138+
expect(err).toBeInstanceOf(AuthzStoreUnavailableError);
139+
expect(err).toMatchObject({ code: AUTHZ_STORE_UNAVAILABLE_CODE, status: AUTHZ_STORE_UNAVAILABLE_STATUS });
140+
});
141+
142+
it('stays FAIL-CLOSED through the evaluator, and its #2565 warn fires exactly once', async () => {
143+
const { loader } = await loaderOver({ registryThrows: new Error('registry unreadable') });
144+
const warns: Array<{ msg: string; meta: any }> = [];
145+
const resolved = await new PermissionEvaluator().resolvePermissionSets(
146+
['ps_catalog'],
147+
{ list: async () => [] },
148+
[],
149+
loader,
150+
{ logger: { warn: (msg: string, meta?: any) => { warns.push({ msg, meta }); } } },
151+
);
152+
// The unresolved set grants nothing — fail closed, never a widening.
153+
expect(resolved).toEqual([]);
154+
// The loss is sayable: one warn, naming the catalog and the names it could not answer.
155+
expect(warns).toHaveLength(1);
156+
expect(warns[0].msg).toContain('catalog lookup failed');
157+
expect(warns[0].meta?.unresolved).toEqual(['ps_catalog']);
158+
});
159+
160+
it('CONTROL: the same request with a catalog that answers resolves the set and warns nothing', async () => {
161+
const { loader } = await loaderOver();
162+
const warns: string[] = [];
163+
const resolved = await new PermissionEvaluator().resolvePermissionSets(
164+
['ps_catalog'],
165+
{ list: async () => [] },
166+
[],
167+
loader,
168+
{ logger: { warn: (msg: string) => { warns.push(msg); } } },
169+
);
170+
expect(resolved.map((s) => s.name)).toEqual(['ps_catalog']);
171+
expect(warns).toEqual([]);
172+
});
173+
});

0 commit comments

Comments
 (0)