Skip to content

Commit eb96bd4

Browse files
committed
fix(cli): cover the unnamed-grant and walled-gate remedies, and pin the command's wiring (WIP)
Claude-Session: https://claude.ai/code/session_01Q9PZ5Pzqcus4woVvTZwWNU Co-authored-by: Claude <noreply@anthropic.com>
1 parent 02f2d16 commit eb96bd4

3 files changed

Lines changed: 78 additions & 14 deletions

File tree

‎packages/cli/src/commands/migrate/organization-ownership.fate-supplement.integration.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@ beforeEach(() => {
6868
: undefined,
6969
},
7070
tableAbsent: () => false,
71+
// What the boot's auth gate answered for the security plugin, as a boot with no auth secret records it.
72+
composition: { securityPlugin: { composed: false, reason: 'no-secret' } },
7173
shutdown: async () => {},
7274
}));
7375
apply.mockImplementation(async () => ({
@@ -175,6 +177,14 @@ describe('--apply hands the merged inventory to the apply step', () => {
175177
}, CASE_TIMEOUT_MS);
176178
});
177179

180+
describe('--apply hands the boot\'s security-plugin composition to the apply step', () => {
181+
it('the apply step receives what the boot\'s auth gate answered, so a name only that plugin declares is reported with it', async () => {
182+
await runJson(['--apply', '--backup-taken', '--plan-file', writePlan(), ...out('apply.json')]);
183+
expect(apply).toHaveBeenCalledTimes(1);
184+
expect(apply.mock.calls[0]![0]).toMatchObject({ securityPlugin: { composed: false, reason: 'no-secret' } });
185+
}, CASE_TIMEOUT_MS);
186+
});
187+
178188
describe('a refusal of the reviewed plan raised after the boot', () => {
179189
it('reaches the operator as apply_refused with its reason and both values, never as a resumable failure', async () => {
180190
const { ReviewedPlanRefusal } = await import('../../utils/organization-ownership-apply.js');

‎packages/cli/src/utils/organization-ownership-apply.test.ts‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2115,20 +2115,27 @@ describe('ADR-0131 D10 --apply — a name only the uncomposed security plugin de
21152115
expect(deletes()).toEqual([]);
21162116
});
21172117

2118-
it('control: composed, or not said, every unresolved name keeps its remedy — the composition is the boot\'s answer, never inferred', async () => {
2118+
it.each([
2119+
['composed', { composed: true } as const],
2120+
['not said', undefined],
2121+
])('control (%s): authored-conversion keeps the remedy — the composition is the boot\'s answer, never inferred', async (_label, securityPlugin) => {
2122+
db.exec(v17Bindings);
2123+
const conversion = step(await apply(catalog(), ledger(), { securityPlugin }), 'authored-conversion');
2124+
expect(blockerOf(conversion, 'position everyone ')).toMatch(/or remove the bindings/);
2125+
expect(conversion.blockers.join('\n')).not.toContain(COMPOSITION);
2126+
});
2127+
2128+
it.each([
2129+
['composed', { composed: true } as const],
2130+
['not said', undefined],
2131+
])('control (%s), under a wall: the rewrite keeps every remedy', async (_label, securityPlugin) => {
21192132
db.exec(`
21202133
INSERT INTO sys_user_permission_set VALUES ('grant_admin', 'org_default', 'usr_admin', NULL, 'admin_full_access');
21212134
${v17Bindings}
21222135
`);
2123-
for (const securityPlugin of [{ composed: true } as const, undefined]) {
2124-
writes = [];
2125-
const conversion = step(await apply(catalog(), ledger(), { securityPlugin }), 'authored-conversion');
2126-
expect(blockerOf(conversion, 'position everyone ')).toMatch(/or remove the bindings/);
2127-
const walled = await apply(catalog(), ledger(), { posture: 'isolated', securityPlugin });
2128-
const rewrite = step(walled, 'rewrite');
2129-
expect(blockerOf(rewrite, 'the permission set admin_full_access,')).toMatch(/or remove the grants/);
2130-
expect(blockerOf(rewrite, 'position everyone ')).toMatch(/resolve its blocker/);
2131-
expect(rewrite.blockers.join('\n')).not.toContain('Did not compose the security plugin');
2132-
}
2136+
const rewrite = step(await apply(catalog(), ledger(), { posture: 'isolated', securityPlugin }), 'rewrite');
2137+
expect(blockerOf(rewrite, 'the permission set admin_full_access,')).toMatch(/or remove the grants/);
2138+
expect(blockerOf(rewrite, 'position everyone ')).toMatch(/resolve its blocker/);
2139+
expect(rewrite.blockers.join('\n')).not.toContain(COMPOSITION);
21332140
});
21342141
});

‎packages/cli/src/utils/organization-ownership-apply.ts‎

Lines changed: 50 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1932,6 +1932,44 @@ interface Verification {
19321932
verified: number;
19331933
}
19341934

1935+
/**
1936+
* [#23359] Of the grants still unnamed (`ids`), those the rewrite leaves
1937+
* unnamed for one reason alone — their set row names a permission set the
1938+
* security plugin ships, which the catalog does not resolve because the boot
1939+
* did not compose that plugin — by that set's name, each with
1940+
* {@link uncomposedSecurityRemedy}'s sentence. Which grants those are is the
1941+
* rewrite's own decision (`planGrantRewrite`'s `unresolved` reason, over the
1942+
* same set rows and catalog), so a grant unnamed for any other reason (no id, a
1943+
* missing row, another organization's row) keeps the general blocker.
1944+
*/
1945+
async function unnamedForComposition(run: Run, ids: readonly string[]): Promise<Map<string, { ids: string[]; remedy: string }>> {
1946+
const out = new Map<string, { ids: string[]; remedy: string }>();
1947+
const grantCols = await columnsOf(run, GRANTS);
1948+
const setCols = await columnsOf(run, SETS);
1949+
if (!ids.length || run.opts.securityPlugin?.composed !== false || !setCols || !grantCols?.has('permission_set_id')) return out;
1950+
const sets = new Map<string, LegacySetRow>();
1951+
for (const row of await readAll(run, SETS, ['name', ...(setCols.has(ORG) ? [ORG] : [])])) {
1952+
sets.set(String(row.id), { name: text(row.name), organizationId: text(row[ORG]) });
1953+
}
1954+
const unnamed = new Set(ids);
1955+
const legacy = (await readAll(run, GRANTS, ['permission_set_id', 'user_id', ...(grantCols.has(ORG) ? [ORG] : [])]))
1956+
.filter((g) => unnamed.has(String(g.id)))
1957+
.map((g) => ({ id: String(g.id), userId: text(g.user_id), organizationId: text(g[ORG]), setId: text(g.permission_set_id) }));
1958+
const resolvable = new Map<string, boolean>();
1959+
for (const grant of legacy) {
1960+
const name = grant.setId ? sets.get(grant.setId)?.name : null;
1961+
if (name && !resolvable.has(name)) resolvable.set(name, (await resolveName(run, 'permission', name)) !== undefined);
1962+
}
1963+
const setNameOf = new Map(legacy.map((g) => [g.id, g.setId ? sets.get(g.setId)?.name : null]));
1964+
for (const id of planGrantRewrite(legacy, [], sets, (name) => resolvable.get(name) === true).reasons.unresolved) {
1965+
const name = setNameOf.get(id);
1966+
const remedy = name ? await uncomposedSecurityRemedy(run, 'permission', name) : undefined;
1967+
if (!name || remedy === undefined) continue;
1968+
out.set(name, { ids: [...(out.get(name)?.ids ?? []), id], remedy });
1969+
}
1970+
return out;
1971+
}
1972+
19351973
/** Read-only: does every reference resolve by name? The gate of fate 2. */
19361974
async function verifyReferences(run: Run): Promise<Verification> {
19371975
const out: Verification = { blockers: [], notes: [], grants: 0, verified: 0 };
@@ -1971,12 +2009,21 @@ async function verifyReferences(run: Run): Promise<Verification> {
19712009
}
19722010
out.verified++;
19732011
}
1974-
if (unnamed.length) {
2012+
const forComposition = await unnamedForComposition(run, unnamed);
2013+
const held = new Set([...forComposition.values()].flatMap((g) => g.ids));
2014+
const rest = unnamed.filter((id) => !held.has(id));
2015+
if (rest.length) {
19752016
out.blockers.push(
1976-
`${unnamed.length} grant(s) still name no permission set (${unnamed.slice(0, 20).join(', ')}${unnamed.length > 20 ? ', …' : ''}). ` +
2017+
`${rest.length} grant(s) still name no permission set (${rest.slice(0, 20).join(', ')}${rest.length > 20 ? ', …' : ''}). ` +
19772018
'The rewrite could not derive their name (see its notes); name or remove each, then re-run.',
19782019
);
19792020
}
2021+
for (const [name, { ids, remedy }] of forComposition) {
2022+
out.blockers.push(
2023+
`${ids.length} grant(s) still name no permission set (${ids.slice(0, 20).join(', ')}${ids.length > 20 ? ', …' : ''}): ` +
2024+
`their set row is ${name}, which the security catalog does not resolve. ${remedy}`,
2025+
);
2026+
}
19802027
for (const [name, ids] of unresolved) {
19812028
out.blockers.push(
19822029
`${ids.length} grant(s) name the permission set ${name}, which the security catalog does not resolve ` +
@@ -2005,7 +2052,7 @@ async function verifyReferences(run: Run): Promise<Verification> {
20052052
position.fate === 'conflicting'
20062053
? `Position ${position.name}: organizations bind different permission sets to it; the bindings cannot be carried into one definition.`
20072054
: `Binding rows give position ${position.name} sets its catalog definition does not name (${position.permissionSets.join(', ')}); ` +
2008-
'deleting them would lose a grant. The authored-conversion step carries them; resolve its blocker, then re-run.',
2055+
`deleting them would lose a grant. ${(await uncomposedSecurityRemedy(run, 'position', position.name)) ?? 'The authored-conversion step carries them; resolve its blocker, then re-run.'}`,
20092056
);
20102057
}
20112058
}

0 commit comments

Comments
 (0)