Skip to content

Commit 8656d67

Browse files
hotlongclaude
andauthored
fix(plugin-security): count and warn the derived capability skip that leaves the platform bucket unseeded (#8749)
* fix(plugin-security): count and warn the derived skip that leaves the platform bucket unseeded (#8536) The DERIVED half of bootstrapSystemCapabilities keeps declining an authored row (#5876, reaffirmed by #8552 — no adoption, no backfill). What it did not do was say what the decline leaves behind: since #8461 made sys_capability.name unique per organization, the cross-organization lookup can be satisfied by an organization's row while the platform (NULL-organization) bucket is never written at all, and `continue` runs before any insert is attempted, so nothing reported the empty bucket. The skip now reads the platform bucket once, on that branch only, and warns with the curated half's provenance-naming shape — but only where the platform's own placeholder is genuinely absent, so the warning means exactly one thing. New counter `unseededDerived` is a documented SUBSET of `skippedAuthored`, which keeps its meaning and value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 * chore(changeset): plugin-security patch for the derived-skip diagnostic (#8536) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7246d63 commit 8656d67

3 files changed

Lines changed: 351 additions & 6 deletions

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
"@objectstack/plugin-security": patch
3+
---
4+
5+
fix(plugin-security): the derived capability seeder's skip is counted and warned instead of leaving the platform bucket silently unseeded (#8536)
6+
7+
**This does not change what the seeder does. It changes whether an operator can
8+
tell what it did.** No adoption, no backfill, no new writes — the #5876 guard
9+
keeps declining an authored row, which is the ruled behaviour (#8552 settled the
10+
posture on an occupied platform bucket: keep declining, loudly).
11+
12+
`bootstrapSystemCapabilities` derives a placeholder `sys_capability` row for any
13+
capability a bootstrap permission set grants by name. Its lookup runs under the
14+
system context, which carries no `tenantId`, so it reads **across
15+
organizations** — and when the row it finds is one it does not own, the #5876
16+
guard `continue`s before any insert is attempted.
17+
18+
Before #8461 that was harmless, because `name` was unique installation-wide: "a
19+
row resolves this name" and "the platform holds a row for this name" were one
20+
statement, which is exactly what #5876's reasoning rests on ("the capability
21+
resolves and the authored copy is the better one"). Per-organization uniqueness
22+
(ADR-0120 D1) separated them. An organization's row now satisfies the lookup
23+
while the platform's NULL-organization bucket is **never written at all**, and
24+
nothing said so: `skippedAuthored` moved, and that counter cannot distinguish
25+
"an authored copy was left alone" from "the platform's definition exists
26+
nowhere".
27+
28+
The skip now reads the platform bucket once — on that branch only, the same cost
29+
the curated half already accepted — and warns with the curated half's
30+
provenance-naming shape: it names the `managed_by` and organization it **read**
31+
off the blocking row rather than asserting an ownership verdict, states which of
32+
the three bucket observations it saw (free / held by an unstamped row / held by
33+
a row with a named provenance), and carries the #8552 hand-resolution line only
34+
where a row an operator may legitimately rename is what blocks the bucket. Where
35+
an organization's row is what stands in the way, the message says there is
36+
nothing to remove — that row is a supported ADR-0066 D1 extension.
37+
38+
The warning fires only where the platform's own placeholder is genuinely
39+
**absent**, so it means one thing. A skip that declines a mere refresh — the
40+
placeholder is present and simply was not the row the cross-organization lookup
41+
selected — stays summary-only, as #4632 decided.
42+
43+
`CapabilitySeedResult` gains `unseededDerived`, a documented **subset** of
44+
`skippedAuthored` rather than a split of it: the existing counter keeps its
45+
meaning and its value, because the two facts are separable only since #8461 and
46+
neither should be inferred from the other.

packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,3 +528,183 @@ describe('[#8470] the curated half owns its row, not whichever row shares the na
528528
expect(ql.rows.filter((r) => r.name === 'manage_users')).toHaveLength(3);
529529
});
530530
});
531+
532+
// ───────────────────────────────────────────────────────────────────────────
533+
// [#8536] The DERIVED half's skip is REPORTED, never silent.
534+
//
535+
// The #5876 guard keeps skipping — that is the ruled behaviour and none of the
536+
// pins below change it. What changes is that a skip which leaves the platform
537+
// (NULL-organization) bucket with no platform-owned row is COUNTED and WARNED
538+
// with the same provenance-naming shape the curated half emits, instead of
539+
// being visible only as a number in the boot summary.
540+
//
541+
// Two facts have to be told apart, and #8461 is why they are no longer the same
542+
// statement. The derived lookup is `{ name }` and runs with no `tenantId`, so it
543+
// reads ACROSS organizations: an organization's row can satisfy it while the
544+
// platform bucket is never written at all. So "the guard skipped" does NOT imply
545+
// "the platform's definition is missing", and the last two pins here are the
546+
// opposite-direction controls that force the difference — an implementation that
547+
// warned on every skip, or that warned unconditionally, fails them.
548+
// ───────────────────────────────────────────────────────────────────────────
549+
describe('[#8536] the derived skip is counted and warned when it leaves the bucket unseeded', () => {
550+
const GRANTS = [{ systemPermissions: ['showcase.export_data'] }];
551+
const AUTHORED = { label: 'Admin Made', description: 'Admin wrote this.' };
552+
/** Deliberately distinctive: a message that merely prints a placeholder cannot contain it. */
553+
const ORG = 'org_jia_9f2';
554+
const NAME = 'showcase.export_data';
555+
556+
const platformRowFor = (ql: ReturnType<typeof makeQl>, name: string) =>
557+
ql.rows.find((r) => r.name === name && r.organization_id == null);
558+
559+
/**
560+
* An organization's authored row for the derived name. `aaa_` sorts before
561+
* every `cap_…`, so the `{ name }` lookup selects it under the #4363
562+
* tie-breaker the double models.
563+
*
564+
* `managed_by` is REQUIRED, with no default value: a parameter defaulted to
565+
* `'admin'` would silently rebuild the `undefined` row of the provenance
566+
* matrix below as an admin row (JS defaults fire on an explicitly passed
567+
* `undefined`), collapsing that case into its neighbour while staying green.
568+
*/
569+
const orgRow = (managed_by: string | undefined) => ({
570+
id: 'aaa_org_derived',
571+
organization_id: ORG,
572+
name: NAME,
573+
...AUTHORED,
574+
scope: 'org',
575+
active: true,
576+
...(managed_by === undefined ? {} : { managed_by }),
577+
});
578+
579+
// ── The headline state: the bucket is FREE and stays empty ──
580+
it('counts and warns when an ORGANIZATION row blocks the derivation', async () => {
581+
const ql = makeQl();
582+
ql.rows.push(orgRow('admin'));
583+
const warn = vi.fn();
584+
const out = await bootstrapSystemCapabilities(ql, GRANTS, { logger: { warn } });
585+
586+
// BEHAVIOUR IS UNCHANGED — this card is observability only. The guard still
587+
// skips, the organization's copy is untouched, and nothing is adopted into
588+
// the platform's identity or backfilled into its bucket (#8552).
589+
expect(out.skippedAuthored).toBe(1);
590+
expect(ql.rows.find((r) => r.id === 'aaa_org_derived')).toEqual(orgRow('admin'));
591+
expect(platformRowFor(ql, NAME)).toBeUndefined();
592+
expect(out.seeded).toBe(KNOWN_CAPABILITIES.length);
593+
594+
// …and the gap the skip leaves behind is now stated out loud.
595+
expect(out.unseededDerived).toBe(1);
596+
expect(warn).toHaveBeenCalledTimes(1);
597+
const [message, meta] = warn.mock.calls[0];
598+
expect(message).toContain(NAME);
599+
expect(message).toContain("managed_by='admin'");
600+
expect(message).toContain(ORG); // the blocking row's REAL organization, not a placeholder
601+
expect(message).toContain('NO row holds the name in that bucket at all');
602+
// The organization's row is a supported ADR-0066 D1 extension, so the
603+
// hand-resolution line — which tells an operator to rename or delete the
604+
// blocking row — must NOT print here. It belongs to a blocked PLATFORM
605+
// bucket only; printing it here would advise removing a legitimate row.
606+
expect(message).not.toContain('To resolve by hand');
607+
expect(meta).toEqual({
608+
name: NAME,
609+
blockingRowId: 'aaa_org_derived',
610+
blockingManagedBy: 'admin',
611+
blockingOrganizationId: ORG,
612+
platformRowId: undefined,
613+
});
614+
});
615+
616+
// The message must name the provenance it READ. Same fixture, same grant —
617+
// only `managed_by` differs, so a diagnostic that printed a fixed string (or
618+
// asserted an ownership verdict) cannot pass all three rows.
619+
it.each([
620+
['admin — Setup-authored inside the organization', 'admin'],
621+
['package — declared by an installed package', 'package'],
622+
['(absent) — not engine-reachable; the message must still be truthful', undefined],
623+
])('names the provenance it READ (%s)', async (_label, managedBy) => {
624+
const ql = makeQl();
625+
ql.rows.push(orgRow(managedBy));
626+
const warn = vi.fn();
627+
const out = await bootstrapSystemCapabilities(ql, GRANTS, { logger: { warn } });
628+
629+
expect(out.unseededDerived).toBe(1);
630+
expect(warn).toHaveBeenCalledTimes(1);
631+
const message = warn.mock.calls[0][0] as string;
632+
if (managedBy === undefined) {
633+
expect(message).toContain('a row carrying no managed_by value');
634+
// never `managed_by='undefined'` / `managed_by='null'` — the field is absent.
635+
expect(message).not.toContain('managed_by=');
636+
} else {
637+
expect(message).toContain(`managed_by='${managedBy}'`);
638+
expect(message).not.toContain('carrying no managed_by value');
639+
// …and never the OTHER row's provenance: the value is read, not guessed.
640+
expect(message).not.toContain(`managed_by='${managedBy === 'admin' ? 'package' : 'admin'}'`);
641+
}
642+
expect(warn.mock.calls[0][1]).toMatchObject({ blockingManagedBy: managedBy ?? null });
643+
});
644+
645+
// ── The other unseeded state: the PLATFORM bucket itself is occupied ──
646+
// This is #5876's own fixture (no `organization_id` at all), which post-#8461
647+
// means the platform bucket. The platform's placeholder is just as absent
648+
// here, so it warns too — and here the blocking row IS one an operator may
649+
// legitimately rename, so the #8552 hand-resolution line applies.
650+
it('counts and warns when the PLATFORM bucket itself is held by an authored row', async () => {
651+
const ql = makeQl();
652+
ql.rows.push({
653+
id: 'cap_existing', name: NAME, ...AUTHORED, scope: 'org', managed_by: 'admin', active: true,
654+
});
655+
const warn = vi.fn();
656+
const out = await bootstrapSystemCapabilities(ql, GRANTS, { logger: { warn } });
657+
658+
expect(out.skippedAuthored).toBe(1);
659+
expect(out.unseededDerived).toBe(1);
660+
expect(ql.rows.find((r) => r.id === 'cap_existing')).toMatchObject({ ...AUTHORED, managed_by: 'admin' });
661+
expect(warn).toHaveBeenCalledTimes(1);
662+
const [message, meta] = warn.mock.calls[0];
663+
expect(message).toContain("managed_by='admin'");
664+
expect(message).toContain('that row is itself the one holder the bucket admits');
665+
expect(message).toContain('To resolve by hand');
666+
expect(meta).toEqual({
667+
name: NAME,
668+
blockingRowId: 'cap_existing',
669+
blockingManagedBy: 'admin',
670+
blockingOrganizationId: null,
671+
platformRowId: 'cap_existing',
672+
});
673+
});
674+
675+
// ── Opposite-direction control #1: the ordinary path ──
676+
// Without this an unconditional warn would pass every pin above.
677+
it('OPPOSITE-DIRECTION CONTROL: an ordinary derivation warns nothing and moves no counter', async () => {
678+
const ql = makeQl();
679+
const warn = vi.fn();
680+
const out = await bootstrapSystemCapabilities(ql, GRANTS, { logger: { warn } });
681+
682+
expect(out.seeded).toBe(KNOWN_CAPABILITIES.length + 1);
683+
expect(platformRowFor(ql, NAME)).toMatchObject({ managed_by: 'platform' });
684+
expect(out.skippedAuthored).toBe(0);
685+
expect(out.unseededDerived).toBe(0);
686+
expect(warn).not.toHaveBeenCalled();
687+
});
688+
689+
// ── Opposite-direction control #2: a skip that is NOT a gap ──
690+
// The sharp one. The guard fires, `skippedAuthored` moves — and nothing is
691+
// missing, because the platform's placeholder is sitting in its bucket; the
692+
// cross-organization lookup simply selected the organization's row first.
693+
// "Warn on every skip" fails here, which is what makes the warning mean one
694+
// specific thing: the platform's definition for this name is absent.
695+
it('OPPOSITE-DIRECTION CONTROL: a skip whose platform placeholder EXISTS is not reported', async () => {
696+
const ql = makeQl();
697+
await bootstrapSystemCapabilities(ql, GRANTS); // boot 1 derives the placeholder
698+
expect(platformRowFor(ql, NAME)).toMatchObject({ managed_by: 'platform' });
699+
// An organization then authors its own row, with an id that sorts FIRST — so
700+
// the `{ name }` lookup selects it and the #5876 guard fires on boot 2.
701+
ql.rows.push(orgRow('admin'));
702+
const warn = vi.fn();
703+
const out = await bootstrapSystemCapabilities(ql, GRANTS, { logger: { warn } });
704+
705+
expect(out.skippedAuthored).toBe(1); // the guard really did fire…
706+
expect(out.unseededDerived).toBe(0); // …and there is no missing definition to report
707+
expect(warn).not.toHaveBeenCalled();
708+
expect(platformRowFor(ql, NAME)).toMatchObject({ managed_by: 'platform' });
709+
});
710+
});

0 commit comments

Comments
 (0)