diff --git a/README.md b/README.md index 70f0be3..67e3e02 100644 --- a/README.md +++ b/README.md @@ -417,7 +417,7 @@ Shape: } ] }, - "permissions": [ /* PermissionPlanItem[]: { key, domainType, domainId, diff: { toPut, toDelete, preserved } } */ ], + "permissions": [ /* PermissionPlanItem[]: { key, domainType, domainId, pendingDomain?, diff: { toPut, toDelete, preserved } } */ ], "summary": { "resources": { "create": 0, "update": 1, "delete": 0, "no-op": 3 }, "drifted": 1, @@ -453,6 +453,19 @@ Shape: `diff.toPut`/`diff.toDelete` is honestly just desired-vs-actual — this is the one place the tool cannot make the distinction, so it doesn't pretend to. +- **A permission domain declared by reference to a same-run-created group + type** (e.g. `ct.groupTypeRole({ groupType: "struktur", ... })` against a + fresh instance where `struktur` is itself in the create-set) plans as a + **pending domain** instead of aborting. Its `domainId` is `null` and it + carries a `pendingDomain` object (the logical reference, e.g. + `{ kind: "group-type", key: "struktur", __ctRef: true }`); the human render + shows ``. Its grants land in + `diff.toPut` and count toward `summary.permissions.toPut`, `hasChanges`, + and exit code `2` — so a fresh-instance `ct plan` reports the create-set + + pending grants rather than failing. `ct apply` re-resolves the real domain + id after the group type is created and reconciles the grants in the same + run. The hard error is reserved for references that resolve to nothing at + all (a key absent from the config, state, and the live catalog — a typo). ### Posting a plan as a PR comment diff --git a/docs/permissions.md b/docs/permissions.md index 078c1bc..b5ce096 100644 --- a/docs/permissions.md +++ b/docs/permissions.md @@ -142,6 +142,33 @@ resolution, two declarations that resolve to the **same** `(domainType, domainId)` are rejected (they would otherwise diff against each other's grants forever) — even if one used a name and the other a raw id. +### Domains created in the same run (fresh-instance rehearsal, #69) + +When a `groupType` reference names a group type that is **created in this same +run** (empty/partial state — the type is part of the create-set), the domain is +handled as a **pending domain** rather than aborting the plan: + +- `ct plan` renders the grant block with a + ` (created this apply)>` marker (consistent with resource + pending refs, #20/#46) and counts its grants in `--json` + (`domainId: null` + a `pendingDomain` reference) and toward exit code `2`. +- `ct apply` runs permission reconciliation **after** the resources are + created, re-resolving the domain id from the fresh group type and granting in + the same run — so a single `ct apply` converges fully. This reuses the same + re-resolution machinery as resource pending refs. +- The hard error (`references a resource created in the same run` → now only a + genuine unresolvable) is reserved for references that resolve to **nothing**: + a key absent from the config, state, and the live catalog (a typo). + +**`group_role` is deliberately NOT symmetric here.** A `group_role` domain id is +the (group, role) **pairing** id, which only exists on +`GET /groups/{groupId}/roles` — re-resolving it needs a *live fetch* after the +group exists, not just a post-execute state lookup. So a `group_role` domain +referencing a same-run-created **group** still fails fast with its own +actionable message ("apply the group first, or pass a numeric id"). Its harder +deferral is out of scope for #69 (which targets the #23 `group_type_role` +scenario). + ## Scope resolution A scoped grant's `scope: [...]` is a list where each entry is either a diff --git a/src/permissions/apply.ts b/src/permissions/apply.ts index d770be4..091a572 100644 --- a/src/permissions/apply.ts +++ b/src/permissions/apply.ts @@ -11,6 +11,8 @@ import { mapConcurrent } from "../util/concurrency.js"; import type { PermissionPlanItem } from "./plan.js"; import type { GrantTuple } from "./grants.js"; import { reresolveTuple } from "./scope.js"; +import { pendingRef, refLabel } from "../resolve/refs.js"; +import { reresolvePendingValue } from "../resolve/resolver.js"; /** How many permission tuples to write at once. Tuples are independent rows, so a modest fan-out is safe. */ const WRITE_CONCURRENCY = 6; @@ -68,7 +70,21 @@ export async function applyPermissionPlan( // the same (domainType, domainId) — so all ops for a path come from ONE item's disjoint diff. // Programmatic callers bypassing evaluateConfig must uphold that invariant themselves. for (const item of items) { - const path = `/permissions/${item.domainType}/${item.domainId}`; + // A pending domain (#69) is a group type created THIS run: its numeric id is only known after + // executePlan, so re-resolve it against the POST-execute state now — reusing the SAME machinery + // that re-resolves resource pending refs (reresolvePendingValue). Requires `state`: a pending + // domain can never be applied statelessly. + let domainId = item.domainId; + if (item.pendingDomain) { + if (!state) { + throw new Error( + `Pending permission domain ${refLabel(item.pendingDomain)} ("${item.key}") cannot be applied ` + + `without post-execute state — it names a resource created in the same run.`, + ); + } + domainId = reresolvePendingValue(pendingRef(item.pendingDomain), state) as number; + } + const path = `/permissions/${item.domainType}/${domainId}`; assertNotPeople(path); for (const t of item.diff.toPut) ops.push({ method: "PUT", path, tuple: t }); for (const t of item.diff.toDelete) ops.push({ method: "DELETE", path, tuple: t }); diff --git a/src/permissions/plan.ts b/src/permissions/plan.ts index ff13d9d..e4af9ff 100644 --- a/src/permissions/plan.ts +++ b/src/permissions/plan.ts @@ -14,9 +14,19 @@ import { resolveScope } from "./scope.js"; import { normalizeActual, diffGrants, isInheritedOnlyRight, INHERITED_RIGHT_MIN_AUTH_ID, type GrantTuple, type GrantDiff, type DomainType, type RawPermission } from "./grants.js"; import type { DesiredPermission } from "./types.js"; import { Resolver } from "../resolve/resolver.js"; -import { isPendingRef } from "../resolve/refs.js"; +import { isPendingRef, refKey, refLabel, type Ref } from "../resolve/refs.js"; -export interface PermissionPlanItem { key: string; domainType: DomainType; domainId: number; diff: GrantDiff } +/** + * One resolved permission domain in the plan. + * + * `domainId` is the concrete numeric domain, EXCEPT when `pendingDomain` is set: the domain is a + * logical Ref to a group type created in THIS SAME run (#69), whose id is unknown until the resource + * tier applies. Then `domainId` is `null` and `pendingDomain` carries the Ref, re-resolved against + * post-execute state at apply time (see `applyPermissionPlan`) — mirroring resource pending refs + * (#20/#46) and the scope pending path (#29). A pending domain has no live grants yet, so its diff + * is `desired → toPut` against an empty actual set. + */ +export interface PermissionPlanItem { key: string; domainType: DomainType; domainId: number | null; pendingDomain?: Ref; diff: GrantDiff } /** * Fan out each grant to (authId, dataId) tuples. ChurchTools reads a scoped grant back as @@ -64,19 +74,28 @@ export function desiredTuples( }); } -/** A permission whose domainId has been resolved from a logical Ref to a concrete numeric id. */ -type ResolvedPermission = DesiredPermission & { domainId: number }; +/** + * A permission whose domainId has been resolved. Either a concrete numeric domain, or — when the + * domain is a group type created in this same run (#69) — a `pendingDomain` Ref with `domainId: null`, + * re-resolved at apply time. + */ +type ResolvedPermission = + | (DesiredPermission & { domainId: number; pendingDomain?: undefined }) + | (Omit & { domainId: null; pendingDomain: Ref }); /** - * Resolve every permission's domainId to a number (#20). A numeric domainId passes straight through; - * a Ref (e.g. `groupType: "…"`) resolves against the live catalog. A domainId that resolves to a - * same-run-created resource (PendingRef) is rejected — the permission plan needs a concrete id to - * fetch actuals and build the write path, and the permission subsystem does not defer that. A - * group_role ref resolves to its concrete (group, role) pairing id in the resolver (#25). + * Resolve every permission's domainId (#20). A numeric domainId passes straight through; a Ref + * (e.g. `groupType: "…"`) resolves against managed state ∪ the live catalog. A domainId that + * resolves to a same-run-created group type (PendingRef) is NOT rejected (#69): it is carried as a + * `pendingDomain` and re-resolved against post-execute state at apply time — this is what lets a + * fresh-instance plan render the create-set + pending grants instead of aborting. A group_role ref + * resolves to its concrete (group, role) pairing id in the resolver and never goes pending — the + * pairing id needs a live `/groups/{id}/roles` fetch, so a same-run group is a hard error there (#25). + * The hard error remains ONLY for genuinely unresolvable references (key not in config/state at all). * - * After resolution, the authoritative duplicate-target guard runs on the CONCRETE ids: two different - * refs (or a ref and a number) that collide on one (domainType, domainId) would otherwise each diff - * against the other's grants and churn forever. Mirrors the eval-time guard in config/context.ts. + * After resolution, the authoritative duplicate-target guard runs on the resolved identities (concrete + * id, or the pending Ref's key): two different refs (or a ref and a number) that collide on one domain + * would otherwise each diff against the other's grants and churn forever. Mirrors config/context.ts. */ async function resolveDomainIds( permissions: DesiredPermission[], resolver: Resolver, @@ -84,25 +103,27 @@ async function resolveDomainIds( const resolved: ResolvedPermission[] = []; for (const p of permissions) { if (typeof p.domainId === "number") { - resolved.push(p as ResolvedPermission); + resolved.push({ ...p, domainId: p.domainId }); continue; } const site = `${p.domainType} "${p.key}".domainId`; const res = await resolver.resolve(p.domainId, site); if (isPendingRef(res)) { - throw new Error( - `${site}: references a resource created in the same run — apply it first, or use a numeric id.`, - ); + resolved.push({ key: p.key, domainType: p.domainType, grants: p.grants, domainId: null, pendingDomain: res.__pendingRef }); + continue; } resolved.push({ ...p, domainId: res }); } const seen = new Map(); for (const p of resolved) { - const key = `${p.domainType}:${p.domainId}`; + const key = p.pendingDomain + ? `${p.domainType}:pending:${refKey(p.pendingDomain)}` + : `${p.domainType}:${p.domainId}`; + const label = p.pendingDomain ? `<${refLabel(p.pendingDomain)}>` : `#${p.domainId}`; const prev = seen.get(key); if (prev) { throw new Error( - `Duplicate permission target after resolution: ${p.domainType} #${p.domainId} is declared by ` + + `Duplicate permission target after resolution: ${p.domainType} ${label} is declared by ` + `both "${prev}" and "${p.key}". Merge their grants into one declaration.`, ); } @@ -133,9 +154,11 @@ export async function buildPermissionPlan( const resolved = await resolveDomainIds(permissions, resolver ?? new Resolver({ client, state, desired })); // Keys declared as groups in the config — valid scope targets even before they are created. const declaredGroupKeys = new Set(desired.filter((r) => r.type === "group").map((r) => r.key)); - // one bulk fetch per distinct domainType + // one bulk fetch per distinct domainType — but only for CONCRETE domains. A pending domain (#69) + // is a group type created this run: it has no live grants, so nothing to fetch (and on a fresh + // instance the fetch would be a pure waste, or a spurious fetchError). const byType = new Map(); - for (const dt of new Set(resolved.map((p) => p.domainType))) { + for (const dt of new Set(resolved.filter((p) => p.pendingDomain === undefined).map((p) => p.domainType))) { try { byType.set(dt, await client.get(`/permissions/${dt}`)); } catch (err) { @@ -145,6 +168,23 @@ export async function buildPermissionPlan( } } for (const p of resolved) { + if (p.pendingDomain !== undefined) { + // The domain (a group type) is created THIS run (#69), so it has no live grants yet: the + // actual set is empty and every desired grant lands in toPut as a pending grant block. Its + // numeric domainId is unknown until the resource tier applies — the pending marker is + // re-resolved against post-execute state at apply time (applyPermissionPlan). Rendered with a + // `` marker consistent with resource pending refs. + items.push({ + key: p.key, + domainType: p.domainType, + domainId: null, + pendingDomain: p.pendingDomain, + // domainId is irrelevant to desiredTuples (it only reads key/domainType/grants); pass the + // pending Ref through so the shape stays a valid DesiredPermission. + diff: diffGrants(desiredTuples({ ...p, domainId: p.pendingDomain }, state, declaredGroupKeys), []), + }); + continue; + } const all = byType.get(p.domainType); if (all == null) continue; // fetch failed for this domainType — recorded above const normalizedAll = normalizeActual(all.filter((r) => r.domainId === p.domainId)); diff --git a/src/permissions/render.ts b/src/permissions/render.ts index 94cef56..d69146d 100644 --- a/src/permissions/render.ts +++ b/src/permissions/render.ts @@ -5,6 +5,18 @@ import pc from "picocolors"; import type { PermissionPlanItem } from "./plan.js"; import type { GrantTuple } from "./grants.js"; +import { refLabel } from "../resolve/refs.js"; + +/** + * The domain identifier for a plan line: a concrete `#id`, or — when the domain is a group type + * created in this same run (#69) — a `` marker consistent with the + * resource pending-ref rendering (src/engine/render.ts). Its real id is filled in at apply time. + */ +function fmtDomain(item: PermissionPlanItem): string { + return item.pendingDomain + ? `<${refLabel(item.pendingDomain)} (created this apply)>` + : `#${item.domainId}`; +} function fmtTuple(t: GrantTuple): string { let scope = ""; @@ -37,7 +49,7 @@ export function renderPermissionPlan(items: PermissionPlanItem[]): string { totalGrant += grantCount; totalRevoke += revokeCount; lines.push( - ` ${item.domainType} #${item.domainId} (${item.key}): ${pc.green(`+${grantCount} grant(s)`)}, ${pc.red(`-${revokeCount} remove(s)`)}`, + ` ${item.domainType} ${fmtDomain(item)} (${item.key}): ${pc.green(`+${grantCount} grant(s)`)}, ${pc.red(`-${revokeCount} remove(s)`)}`, ); for (const t of item.diff.toPut) { lines.push(` ${pc.green("+")} ${fmtTuple(t)}`); diff --git a/tests/permission-pending-domain.test.ts b/tests/permission-pending-domain.test.ts new file mode 100644 index 0000000..a67e9b4 --- /dev/null +++ b/tests/permission-pending-domain.test.ts @@ -0,0 +1,155 @@ +/** + * Pending permission domains (#69): a permission domain declared BY REFERENCE to a group type that + * is created in the SAME run must NOT abort the plan. Instead it plans as a pending grant block and + * reconciles at apply time once the group type has a fresh id — mirroring resource pending refs + * (#20/#46) and the scope pending path (#29). This is the #23 fresh-instance rehearsal scenario. + * + * Exercises the REAL build → execute → apply sequence with a mock client, so convergence in ONE + * `ct apply` run is proven end-to-end. No live instance. + */ +import { describe, it, expect, vi } from "vitest"; +import { buildPermissionPlan } from "../src/permissions/plan.js"; +import { applyPermissionPlan } from "../src/permissions/apply.js"; +import { renderPermissionPlan } from "../src/permissions/render.js"; +import { executePlan } from "../src/engine/execute.js"; +import { emptyState, type State } from "../src/state/state.js"; +import { ref } from "../src/resolve/refs.js"; +import type { Plan, DesiredResource } from "../src/engine/types.js"; +import type { DesiredPermission } from "../src/permissions/types.js"; +import type { CtClient } from "../src/api/ctClient.js"; + +const HOST = "https://eqrm.church.tools"; +const STRUKTUR_TYPE_ID = 9; + +// The #23 config, in miniature: declare a group type AND a group_type_role permission domain that +// references it by name — with ZERO numeric ids. "churchgroup:administer groups" is authId 1113, +// unscoped (global). +const strukturType: DesiredResource[] = [ + { type: "group-type", key: "struktur", fields: { name: "Struktur" }, dependsOn: [] }, +]; +const strukturPerm: DesiredPermission = { + key: "struktur_roles", + domainType: "group_type_role", + domainId: ref.groupType("struktur"), + grants: ["churchgroup:administer groups"], +}; +const createStrukturPlan: Plan = { + items: [{ type: "group-type", key: "struktur", id: null, action: "create", changes: [{ field: "name", from: undefined, to: "Struktur" }] }], +}; + +/** A mock client: POST /group/grouptypes mints STRUKTUR_TYPE_ID; GETs return whatever `perms` maps. */ +function mockClient(perms: Record = {}) { + const calls: { method: string; path: string; body?: unknown }[] = []; + const request = vi.fn(async (method: string, path: string, body?: unknown) => { + calls.push({ method, path, body }); + if (method === "POST" && path === "/group/grouptypes") return { id: STRUKTUR_TYPE_ID }; + return {}; + }); + const get = vi.fn(async (path: string) => (perms[path] ?? []) as unknown[]); + return { client: { request, get } as unknown as CtClient, calls, get }; +} + +describe("pending domain: declare group type + grant by reference in one config (#69/#23)", () => { + it("plans from EMPTY state without aborting — a pending grant block, not the hard error", async () => { + const { client, get } = mockClient(); + const { items, fetchErrors } = await buildPermissionPlan(client, emptyState(HOST), [strukturPerm], strukturType); + + expect(fetchErrors).toEqual([]); + // The domain is pending: no numeric id yet, the Ref is carried for apply-time re-resolution. + expect(items[0]?.domainId).toBeNull(); + expect(items[0]?.pendingDomain).toEqual(ref.groupType("struktur")); + // Every desired grant lands in toPut against an empty actual set (the type has no live grants). + expect(items[0]?.diff.toPut).toEqual([{ authId: 1113, dataId: [], type: "grant" }]); + expect(items[0]?.diff.toDelete).toEqual([]); + // No /permissions fetch for a pending-only plan — nothing to fetch on a not-yet-created type. + expect(get).not.toHaveBeenCalled(); + // Read-only render shows the pending marker, consistent with resource pending-ref rendering. + expect(renderPermissionPlan(items)).toContain(""); + }); + + it("counts as a change for --detailed-exitcode / --json (toPut > 0)", async () => { + const { client } = mockClient(); + const { items } = await buildPermissionPlan(client, emptyState(HOST), [strukturPerm], strukturType); + const hasPermissionChanges = items.some((i) => i.diff.toPut.length > 0 || i.diff.toDelete.length > 0); + expect(hasPermissionChanges).toBe(true); + expect(items.reduce((n, i) => n + i.diff.toPut.length, 0)).toBe(1); + }); + + it("applies in ONE run — create then grant against the FRESH id — and a second plan is a no-op", async () => { + const { client, calls } = mockClient(); + const state = emptyState(HOST); + const { items } = await buildPermissionPlan(client, state, [strukturPerm], strukturType); + + // executePlan creates the group type and upserts its real id into state… + await executePlan(createStrukturPlan, { client, state, statePath: "unused", save: async () => {} }); + expect(state.resources.struktur?.id).toBe(STRUKTUR_TYPE_ID); + + // …then permission reconciliation runs against POST-execute state and writes to the fresh domain. + const res = await applyPermissionPlan(items, client, state); + expect(res.granted).toBe(1); + const put = calls.find((c) => c.method === "PUT" && c.path === `/permissions/group_type_role/${STRUKTUR_TYPE_ID}`); + expect(put?.body).toEqual({ authId: 1113, type: "grant" }); // fresh domain id in the path, not a placeholder + + // Second plan (type now in state, grant now live) converges to a no-op — domain is concrete. + const { client: c2 } = mockClient({ + "/permissions/group_type_role": [ + { domainType: "group_type_role", domainId: STRUKTUR_TYPE_ID, authId: 1113, dataId: null, type: "grant", meta: { modifiedPid: 1 } }, + ], + }); + const { items: items2, fetchErrors } = await buildPermissionPlan(c2, state, [strukturPerm], strukturType); + expect(fetchErrors).toEqual([]); + expect(items2[0]?.domainId).toBe(STRUKTUR_TYPE_ID); // now concrete, not pending + expect(items2[0]?.pendingDomain).toBeUndefined(); + expect(items2[0]?.diff.toPut).toEqual([]); + expect(items2[0]?.diff.toDelete).toEqual([]); + expect(renderPermissionPlan(items2)).toContain("No permission changes"); + }); +}); + +describe("pending domain: prod-like scenario (type already in state) is unchanged (#69)", () => { + it("resolves to the concrete domain id and reconciles idempotently — no pending path", async () => { + const state: State = { version: 1, host: HOST, resources: { + struktur: { type: "group-type", id: STRUKTUR_TYPE_ID, key: "struktur", fields: { name: "Struktur" }, adoptedAt: "t", updatedAt: "t" }, + }}; + const { client } = mockClient({ + "/permissions/group_type_role": [ + { domainType: "group_type_role", domainId: STRUKTUR_TYPE_ID, authId: 1113, dataId: null, type: "grant", meta: { modifiedPid: 1 } }, + ], + }); + const { items, fetchErrors } = await buildPermissionPlan(client, state, [strukturPerm], strukturType); + expect(fetchErrors).toEqual([]); + expect(items[0]?.domainId).toBe(STRUKTUR_TYPE_ID); + expect(items[0]?.pendingDomain).toBeUndefined(); + expect(items[0]?.diff.toPut).toEqual([]); + expect(items[0]?.diff.toDelete).toEqual([]); + }); +}); + +describe("group_role symmetry: a same-run group is NOT made pending — it stays a hard error (#69/#25)", () => { + it("keeps the specific 'apply the group first' message for a group_role domain on a declared-not-created group", async () => { + // A group_role domainId is the (group, role) PAIRING id, exposed only on GET /groups/{id}/roles — + // so re-resolution needs a LIVE fetch after the group exists, not just a state lookup. That is a + // materially harder case than group_type_role (whose domainId is the group type's OWN id, present + // in post-execute state). So group_role deliberately does NOT go pending: it fails fast at + // resolve time with its own actionable message, unchanged by this fix. + const declaredGroup: DesiredResource[] = [{ type: "group", key: "kids_area", fields: { name: "Kids" }, dependsOn: [] }]; + const grPerm: DesiredPermission = { + key: "kids_lead", domainType: "group_role", domainId: ref.groupRole("kids_area", "Leiter"), grants: [], + }; + const { client } = mockClient(); + await expect(buildPermissionPlan(client, emptyState(HOST), [grPerm], declaredGroup)).rejects.toThrow( + /group "kids_area" is declared in this config but not yet created/, + ); + }); +}); + +describe("pending domain: a TRUE typo (key absent from config AND state AND catalog) still hard-errors (#69)", () => { + it("throws the resolver's unchanged notFound message — not a pending block", async () => { + // "strucktur" is neither declared, nor in state, nor a live catalog match → genuinely unresolvable. + const typoPerm: DesiredPermission = { ...strukturPerm, domainId: ref.groupType("strucktur") }; + const { client } = mockClient({ "/group/grouptypes": [{ id: STRUKTUR_TYPE_ID, name: "Struktur" }] }); + await expect(buildPermissionPlan(client, emptyState(HOST), [typoPerm], strukturType)).rejects.toThrow( + /Cannot resolve group-type:strucktur referenced at group_type_role "struktur_roles".domainId/, + ); + }); +});