diff --git a/README.md b/README.md index e8c2387..1b59c0d 100644 --- a/README.md +++ b/README.md @@ -105,11 +105,21 @@ ct get groups # JSON to stdout — pipe into jq (every page, not ct adopt campus 0 # bring ONE existing resource under management ct coverage # what the instance has that the config does not manage ct state list # what is managed +ct state rm campus mainz # un-adopt: drop it from state. Never touches ChurchTools. ct plan # diff the config against ChurchTools (read-only) ct apply # create + update in dependency order (confirm + backup first) ct refresh --group # make ChurchTools re-evaluate one auto-group now ``` +`state rm` is the inverse of `adopt`, and only of `adopt`: it removes the entry +from the state file, makes no HTTP call, and leaves the resource in place in +ChurchTools, now unmanaged. It refuses a key the config still declares — that +would make the next plan propose creating a resource that already exists — so +delete the declaration first, or pass `--force` to do both in one change. +"Declares" covers permission declarations too, not only resources: a key named +by a `ct.groupRole` domain or a group scope is just as broken to remove, and the +refusal is what keeps that from surfacing one command later as a plan error. + `apply` reconciles **creates and updates** only, saving state after each action (crash-safe / resumable). It **never deletes**: a resource dropped from the config is surfaced as a notice pointing at `destroy`. `destroy --target ` diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index f911d64..f8570ef 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/docs/handbuch/dynamic-groups.md @@ -6,7 +6,7 @@ sources: - src/engine/dynamic.ts - src/engine/synthetic.ts - src/commands/adopt-group.ts -sources_hash: 9a8c3091484e9258 +sources_hash: d67f76530e841833 reviewed: 2026-08-17 --- @@ -140,6 +140,25 @@ ct adopt group --with-dynamic # portablized (the d ct adopt group --with-dynamic --no-portable-rulesets # verbatim, this host's ids ``` +> **A re-adopt refreshes the snapshot, not the key (#123).** Re-running this over +> a list of ids is the documented way to refresh rulesets once their scope +> targets become managed — and it is the one mode where `-k` is rejected ("only +> valid when exactly one group is resolved"). Any resource whose adopted key +> differed from the derived key therefore used to be **silently re-keyed** by a +> routine refresh, so the config's declaration matched nothing in state and the +> next plan read as "one to create, one to destroy" for a resource that was fine +> and untouched. An already-managed resource now keeps its key, and says what it +> would have become: +> +> ```text +> ! merkmal_alle_2_5_mz: key would change to "alle_2_bis_5_mz" (derived from the live name). +> Keeping the adopted key. Pass --rekey to change it. +> ``` +> +> Because the ruleset filename follows the key, this also means the refresh +> overwrites `rulesets/merkmal_alle_2_5_mz.json` — the file the config already +> points at — instead of writing a second one beside it. + `ct adopt group --with-dynamic` runs the captured (normalized) ruleset through `portablizeRuleset` (`src/config/query-refs.ts`) before writing `rulesets/.json`: every numeric id sitting in a known ChurchQuery `var` @@ -147,13 +166,14 @@ position that maps to a **managed** logical key is rewritten to its `{ __ctRef } marker; every other id is left numeric. The `var → RefKind` catalog it keys off (`VAR_REF_KINDS`) is: -| ChurchQuery `var` | marker `kind` | source catalog / state | -| --------------------- | ----------------- | ---------------------------------------- | -| `ctgroup.id` | `group` | managed state (no REST catalog) | -| `ctgroup.campusId` | `campus` | `/campuses` | -| `person.campusId` | `campus` | `/campuses` | -| `ctgroup.groupTypeId` | `group-type` | `/group/grouptypes` | -| `role.id` | `group-type-role` | `/group/roles` (by `groupTypeId` + name) | +| ChurchQuery `var` | marker `kind` | source catalog / state | +| --------------------- | ----------------- | -------------------------------------------------- | +| `ctgroup.id` | `group` | managed state (no REST catalog) | +| `ctgroup.campusId` | `campus` | `/campuses` | +| `person.campusId` | `campus` | `/campuses` | +| `ctgroup.groupTypeId` | `group-type` | `/group/grouptypes` | +| `role.id` | `group-type-role` | `/group/roles` (by `groupTypeId` + name) | +| `role.id` | `role-def` | managed state — only when the pair collides (#125) | The same `group-type-role` rewrite also covers the **out-of-query** integer field `process.*.handleMembership.groupTypeRoleId` (the target role a @@ -172,7 +192,53 @@ is a **groupTypeRoleId**: a role scoped to a group **type**. Role names are marker carries the group-type key + role name, and the resolver picks the one `/group/roles` row whose `groupTypeId` matches this host's group type and whose name slugs to the role. This corrects the earlier `role-def` mapping (#86), -which was unresolvable on the real instance. +which read the role catalog by bare name and was unresolvable on the real +instance. + +##### When the pair collides too — adopt the role (#125) + +"Unique in practice" is not "unique". Two rows **can** share a name on one group +type, and then the pair is a hard error: + +```text +✗ Ambiguous group-type-role(groupType=community, role=leader) referenced at group "4_teamactive": + 2 roles on group type #30 match — "leader" (#87), "leader" (#207). +``` + +Neither obvious remedy works for a config shared across hosts. _Renaming_ +means editing ChurchTools master data to work around a config limitation — and +in the observed case one of the two rows is CT's own stock `leader`. _Passing a +numeric id_ cannot work at all: the roles have **different ids per host** +(`#127` on one, `#207` on the other), and `ConfigContext` deliberately exposes +no env or host, so there is nowhere to branch. + +The fix is the same "adopt the target to portablize it" move that already works +for groups, campuses and departments — **adopt the role**, under the same +logical key on every host: + +```bash +ct adopt group-role 127 --env prod -k community_leader +ct adopt group-role 207 --env dev -k community_leader +``` + +Capture then emits the managed form, which this host resolves from state rather +than from a name lookup: + +```json +{ "__ctRef": true, "kind": "role-def", "key": "community_leader" } +``` + +**Only when the pair actually collides, though.** The `(groupType, role)` pair +stays the default for every role whose pair is unique — even one the config +owns. Off this host `role-def` is the **weaker** reference of the two: the +resolver falls back to `resolveFromCatalog`, which keys `/group/roles` by +`slug(name)` **alone**. On a host where the role was never adopted under the +shared key, a name matching exactly one row therefore resolves _silently_ to a +role on a different group type (only a multi-row match errors) — the same bare +name lookup that #76 reverted #86 for. The pair cannot fail that way, because it +matches on `(groupTypeId, name)`. So the safe reference is never traded away for +the weaker one; `role-def` is reserved for the ids the pair genuinely cannot +name, where there is no safe reference to lose. #### What could not be portablized is REPORTED, never swallowed (#101) @@ -193,11 +259,36 @@ At **capture** time (`ct adopt … --with-dynamic`) the state file and the `/group/roles` catalog are both in hand, so the reason is a checked fact: ```text -! rulesets/jugend.json keeps 3 host-specific id(s) — NOT portable to another host: +! rulesets/jugend.json keeps 5 host-specific id(s) — NOT portable to another host: ctgroup.id: 1246 left numeric — not under management — `ct adopt group ` for each (then re-adopt) makes them portable ctgroup.groupStatusId: 1, 2 left numeric — group statuses have no REST catalog (#67) — no logical form exists + person.id: 5703, 4389 left numeric — person ids are NEVER portable — ct does not manage people, so this ruleset names DIFFERENT people on another host. Remove the clause or accept the divergence ``` +##### `person.id` is reported, and it is not fixable (#127) + +A ruleset that includes or excludes specific people by id is common — four of +five auto-groups captured in one week did it: + +```json +{ "oneof": [{ "var": "person.id" }, [5703, 4389]] } +{ "!": [{ "oneof": [{ "var": "person.id" }, [12, 1]] }] } +``` + +Those are the source host's person ids written verbatim into the other host's +ruleset, where they name entirely different people — the same failure mode as a +raw `ctgroup.id`. `person.id 1` is the unluckiest case: it exists on every +ChurchTools instance and is almost always an administrator, so an exclusion +aimed at one person on the source host lands on someone real on the target host +rather than harmlessly matching nothing. + +Unlike every other entry in the report, this one is **not fixable in config**, +and the wording says so rather than offering a command. `ct` correctly does not +manage people, so there is no person catalog to resolve against and no +`__ctRef` kind that could express it. The ask is only that the tool say so: +before #127 it was silent, and the absence of a warning actively implied there +was nothing to find. + At **plan** time the same scan runs over every declared dynamic group, but with no state, no catalogs and no network — it can prove that an id sits in an entity position and nothing more. So it says exactly that, rather than asserting a diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index 5372a2d..1144be5 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -5,7 +5,7 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: 5f9ed09f1e3793b0 +sources_hash: 87fe6c847d70d89c reviewed: 2026-08-17 --- @@ -631,18 +631,29 @@ In bulk mode: - a role instance blocked by an undeclarable scope dimension is skipped and named, with the dimension to pass to [`preserveUnknown`](#partial-ownership-preserveunknown-opt-in-102). The single-domain form still emits it, deliberately, one domain at a time; +- both of those decisions are made over the **effective** rows — the same set the + block will emit (#114/#119), not just the ones `ct` owns. That matters in both + directions: a domain carrying its rights only as _inherited_ rows is still + emitted (judging owned rows would call it empty, skip it, and leave those + rights for the other host's plan to revoke), and an inherited grant on a + dimension with no logical form still blocks the domain (judging owned rows + would hide it from the gate and emit a host-specific numeric `dataId` into a + bulk paste). Ownership itself is unchanged — that is still decided by + `normalizeActual` at plan time, never by what adoption prints; - nothing is capped silently — the run prints how many blocks it emitted **and** how many it skipped, with the reason for each. ### The single-domain form It fetches `GET /permissions//`, runs the rows through the -**same** normalization the planner uses (`normalizeActual`), and prints a -`ct.groupRole` / `ct.groupTypeRole` block whose every emitted grant is guaranteed -to be accepted by `ct plan` (the round trip is locked by tests): - -- **Excluded, as reconciliation excludes them:** the system baseline - (`meta.modifiedPid === -1`) and inherited rows. +**same** normalization the planner reconciles against (`normalizeEffective`), and +prints a `ct.groupRole` / `ct.groupTypeRole` block whose every emitted grant is +guaranteed to be accepted by `ct plan` (the round trip is locked by tests): + +- **Everything the host grants is emitted** — direct, inherited and + system-baseline rows alike (#114/#119). See [Provenance and + portability](#provenance-and-portability-114119) for why this is broader than + what `ct` owns. - **Revoke/deny rows are preserved, not emitted.** The reconciler never deletes a deny it did not author; if any exist, the block ends with a `NOTE` comment saying so (authoring denies as config is a separate, unshipped feature). @@ -673,12 +684,14 @@ to be accepted by `ct plan` (the round trip is locked by tests): - A scoped right granted **globally** in CT (row with no `dataId`) is a `WARNING` comment either way — the DSL deliberately cannot declare a global grant of a scoped right. -- **System-baseline and inherited rows are dropped, not emitted.** Adoption runs - the live rows through `normalizeActual`, which excludes the self-re-adding - system baseline (`modifiedPid === -1`) and any `isInherited` row. Admin-authored - direct grants — **including** the writable `authId >= 10000` `churchdb:+…` - member rights CT lets you set on `group_type_role` — are emitted as active - grants (no authId cutoff; #65). +- **The `-1` "alle" sentinel is not an id** (#115). A grant scoped to "alle" + comes back as `dataId: -1`, meaning _every_ value of the dimension on whatever + host reads it. It is therefore already portable, and it is emitted with a + one-line comment saying what it is — never with an adoption hint, because + `ct adopt department -1` names a resource that cannot exist. +- **Admin-authored member rights are emitted as active grants** — **including** + the writable `authId >= 10000` `churchdb:+…` member rights CT lets you set on + `group_type_role`. There is no authId cutoff (#65). - **Only `group_role` / `group_type_role` / `status`** are valid; people domains are refused (the same hard boundary as everywhere else). A `status` block is emitted with a numeric `id:` — rename it to the portable @@ -703,11 +716,13 @@ and `ct plan`. - **No authId cutoff.** Admin-authored member rights (`authId >= 10000`, the `churchdb:+…` family) ARE writable on `group_type_role` and can be declared under `ct.groupTypeRole` — verified live (eqrm prod `group_type_role/9` carries - 24 such admin-set rows). What ct never reconciles is decided by the live row's + 24 such admin-set rows). What ct never _writes_ is decided by the live row's flags, not its authId: `normalizeActual` drops the system baseline - (`modifiedPid === -1`) and every `isInherited` row, so those are neither adopted - nor revoked (#65). Earlier versions blocked `authId >= 10000` outright — that was - too broad and is removed. + (`modifiedPid === -1`) and every `isInherited` row, so those are never authored + and never revoked (#65). They are still **emitted by adoption and honoured as + satisfying a declaration** — see [Provenance and + portability](#provenance-and-portability-114119). Earlier versions blocked + `authId >= 10000` outright — that was too broad and is removed. - **Revocation is a later extension, not exposed yet.** `GrantTuple.type` is typed as `"grant" | "revoke"`, but the DSL and `desiredTuples` currently only ever _emit_ `"grant"` tuples — there is no config-level way to declare @@ -719,17 +734,53 @@ and `ct plan`. ## The baseline-tolerance model -`ct plan` and `ct apply` reconcile only the grants **you author** on a -domainId — never the platform's own bookkeeping. `normalizeActual` -(`src/permissions/grants.ts`) filters two kinds of rows out of every actual -fetch before diffing, making both invisible to reconciliation: +`ct plan` and `ct apply` **write** only the grants you author on a domainId — +never the platform's own bookkeeping. Two kinds of row are never authored and +never revoked (`normalizeActual` in `src/permissions/grants.ts`): - **System baseline rows** — any row with `meta.modifiedPid === -1`. These - are ChurchTools' own self-re-adding defaults; they are never proposed for - deletion and never conflict with a desired grant. + are ChurchTools' own self-re-adding defaults. - **Inherited rows** — any row with `isInherited: true`. These come from - hierarchy/role inheritance, not this domainId's own grant table; they are - not owned here either. + hierarchy/role inheritance, not this domainId's own grant table. + +### Provenance and portability (#114/#119) + +Those two rules decide what `ct` **owns**. They do _not_ decide whether a +declared grant is already **satisfied** — that is judged against the +**effective** set: every right the host grants, by any route +(`normalizeEffective`). + +The distinction matters because provenance is not stable across hosts of the +same instance, even when the effective permissions are identical: + +| | one host | the other host | +| ------------------------------------------------------ | ------------------------------- | --------------------------- | +| **#114** — same right, same role | `meta.modifiedPid: -1` (system) | `modifiedPid: 1` (a person) | +| **#119** — 15 group-member rights the TYPE also grants | `isInherited: true` | `isInherited: false` | + +Neither divergence is hand-made: the second host is a copy, and the copy +stamped a person id onto rows that are system rows upstream. Measured across +two hosts on CT 3.135.2: 18 of 63 `group_role` domains carried inherited rows +on one, and **zero anywhere** on the other. + +Deciding satisfaction from ownership made such a config impossible to write. +Omit the right (as adoption used to emit it) and the other host **revokes** it; +declare it and this host plans `+1 grant` forever. So the domain had to be left +out of adoption entirely, even though nothing about it is genuinely +undeclarable. + +Reconciling on the effective set removes the dilemma, because the two hosts +agree on effective permissions even when they disagree on provenance: + +- A declared right the host already grants by any route needs **no PUT**. +- A revoke is still computed from the **owned** set only, so `ct` never revokes + a baseline or inherited row. +- `ct adopt grants` emits the effective set, and names in a footer how many of + the emitted grants are inherited or baseline on this host. + +The "never fight the platform" property is unchanged. The only thing that +changed is that `ct` stopped proposing to re-author what the platform already +grants. Combined with the **managed-guard** (`buildPermissionPlan` only ever surfaces the `domainId`s you've declared — a bulk `GET /permissions/{domainType}` diff --git a/src/commands/adopt-grants.ts b/src/commands/adopt-grants.ts index 326a81c..48fc237 100644 --- a/src/commands/adopt-grants.ts +++ b/src/commands/adopt-grants.ts @@ -93,8 +93,9 @@ export function adoptGrantsCommand(): Command { const config = await resolveConfig(); const statePath = cmdEnv.statePath; const state = await loadState(statePath, config.host); - // Bulk selection runs the same declarability verdict as `ct coverage`, so it needs this - // host's catalog for the same reason (#105): under the bundled one, `--all-declarable` + // Bulk selection runs the same declarability verdict as `ct coverage` (over the effective + // rather than the authored rows — see the call below), so it needs this host's catalog for + // the same reason (#105): under the bundled one, `--all-declarable` // silently SKIPS role instances `ct plan` would manage, filed under an authId the active // catalog can name perfectly well. const hostCatalog = await loadHostCatalog(config.host); @@ -203,7 +204,12 @@ async function emitBulk( for (const role of candidates) { const rows = rowsByDomainId.get(role.domainId) ?? []; - const verdict = declarability(rows); + // Judged on the EFFECTIVE set, because that is what `buildAdoptedGrants` emits (#114/#119). An + // owned-rows verdict here would disagree with the emitter in both directions: it would skip a + // domain whose rights are all inherited on THIS host (18 of 63 group_role domains, measured) and + // leave them undeclared for the other one to revoke, and it would wave through an inherited grant + // on a dimension with no logical form, which the gate below exists to stop. + const verdict = declarability(rows, { scope: "effective" }); if (verdict.grantCount === 0) { skippedEmpty += 1; continue; // nothing authored on this domain — an empty block is not worth a paste diff --git a/src/commands/adopt-group.ts b/src/commands/adopt-group.ts index f268c75..1f18d72 100644 --- a/src/commands/adopt-group.ts +++ b/src/commands/adopt-group.ts @@ -21,7 +21,7 @@ import { RESOURCES, configSnippet, fromInformation, slug } from "../resources/re import { ReverseResolver, type RoleCatalogEntry } from "../resolve/reverse.js"; import type { RefKind } from "../resolve/refs.js"; import { formatPortablizeWarnings, portablizeRuleset, scanUnportablized } from "../config/query-refs.js"; -import { loadState, saveState, upsert, type State } from "../state/state.js"; +import { chooseAdoptKey, loadState, saveState, upsert, type State } from "../state/state.js"; import { success, info, warn, out } from "../ui.js"; interface AdoptGroupOptions { @@ -32,6 +32,8 @@ interface AdoptGroupOptions { type?: string; childrenOf?: string; withDynamic?: boolean; + /** Opt in to changing an already-managed group's logical key (#123). Never the default. */ + rekey?: boolean; /** Commander's negatable `--no-portable-rulesets`: true unless the flag was passed (#101). */ portableRulesets?: boolean; strictRulesets?: boolean; @@ -174,6 +176,10 @@ export function adoptGroupCommand(): Command { .option("-k, --key ", "logical key (only valid when exactly one group is resolved)") .option("-s, --state ", "state file path (or set CT_STATE)") .option("-e, --env ", "environment profile from ct.envs.json (host + state + token)") + .option( + "--rekey", + "let a re-adoption change an already-managed group's logical key to the derived one (#123)", + ) .option("--dry-run", "preview the config entries and state changes without writing") .option("--type ", "adopt every group of this group type (numeric id or logical key)") .option( @@ -281,11 +287,25 @@ export function adoptGroupCommand(): Command { for (const id of resolvedIds) { const resource = await client.get>(GROUP_SPEC.itemPath(id)); - const key = - (resolvedIds.length === 1 ? opts.key?.trim() : undefined) || GROUP_SPEC.deriveKey(resource); + // An already-managed group keeps its adopted key unless --rekey says otherwise (#123). This + // is the mode that made the bug bite: the documented ruleset-refresh workflow passes a LIST + // of ids, which is exactly when `-k` is rejected, so there was no way to prevent the re-key. + // Because `relPath` below is built from `key`, preserving it also makes the refresh overwrite + // the ruleset file the config already points at instead of writing a second one. + const choice = chooseAdoptKey(state, "group", id, GROUP_SPEC.deriveKey(resource), { + explicitKey: resolvedIds.length === 1 ? opts.key : undefined, + rekey: opts.rekey, + }); + const key = choice.key; if (!key) { throw new Error(`Could not derive a logical key for group #${id} — pass --key explicitly.`); } + if (choice.wouldBecome) { + warn( + `${key}: key would change to "${choice.wouldBecome}" (derived from the live name). ` + + `Keeping the adopted key. Pass --rekey to change it.`, + ); + } const fields = GROUP_SPEC.managedFields(resource); // Reverse-resolve the group's numeric ids to logical sugar for the emitted snippet; the @@ -301,11 +321,18 @@ export function adoptGroupCommand(): Command { // Managed group ids come from state (no catalog for `group`), including any group this // same run already adopted; the master-data kinds come from the catalog maps above. const groupMap = new Map(); + // Managed role definitions, likewise from STATE rather than the live catalog (#125): + // state gives a per-host id under a shared logical key, which is what makes a + // duplicate role name fixable by adopting the role instead of renaming master data. + // (`ReverseResolver.idToKeyByKind("role-def")` keys by slug(name) off the catalog and + // is exactly the ambiguous mapping this replaces.) + const roleDefMap = new Map(); for (const r of Object.values(state.resources)) { if (r.type === "group") groupMap.set(r.id, r.key); + if (r.type === "group-role") roleDefMap.set(r.id, r.key); } const { ruleset, warnings } = portablizeRuleset(captured.normalizedRuleset, { - idToKeyByKind: { ...portableCatalogMaps, group: groupMap }, + idToKeyByKind: { ...portableCatalogMaps, group: groupMap, "role-def": roleDefMap }, roleCatalog, groupTypeIdToKey, }); diff --git a/src/commands/adopt.ts b/src/commands/adopt.ts index c861136..82b89a0 100644 --- a/src/commands/adopt.ts +++ b/src/commands/adopt.ts @@ -4,7 +4,7 @@ import { resolveConfig } from "../config.js"; import { prepareEnv } from "../env/context.js"; import { resourceType, configSnippet } from "../resources/registry.js"; import { ReverseResolver } from "../resolve/reverse.js"; -import { loadState, saveState, upsert } from "../state/state.js"; +import { chooseAdoptKey, loadState, saveState, upsert } from "../state/state.js"; import { success, info, warn, out } from "../ui.js"; import { adoptGrantsCommand } from "./adopt-grants.js"; import { adoptGroupCommand } from "./adopt-group.js"; @@ -13,6 +13,8 @@ interface AdoptOptions { key?: string; state?: string; env?: string; + /** Opt in to changing an already-managed resource's logical key (#123). Never the default. */ + rekey?: boolean; dryRun?: boolean; } @@ -24,6 +26,10 @@ export function adoptCommand(): Command { .option("-k, --key ", "logical key (defaults to a slug of the resource name)") .option("-s, --state ", "state file path (or set CT_STATE)") .option("-e, --env ", "environment profile from ct.envs.json (host + state + token)") + .option( + "--rekey", + "let a re-adoption change an already-managed resource's logical key to the derived one (#123)", + ) .option("--dry-run", "preview the config entry and state change without writing") .action(async (type: string, rawId: string, opts: AdoptOptions) => { const spec = resourceType(type); @@ -50,10 +56,22 @@ export function adoptCommand(): Command { throw new Error(`No ${type} with id ${id} exists in ChurchTools.`); } - const key = opts.key?.trim() || spec.deriveKey(resource); + const derived = spec.deriveKey(resource); + // An already-managed resource keeps its adopted key unless --rekey says otherwise (#123). + const choice = chooseAdoptKey(state, type, id, derived, { + explicitKey: opts.key, + rekey: opts.rekey, + }); + const key = choice.key; if (!key) { throw new Error("Could not derive a logical key — pass --key explicitly."); } + if (choice.wouldBecome) { + warn( + `${key}: key would change to "${choice.wouldBecome}" (derived from the live name). ` + + `Keeping the adopted key. Pass --rekey to change it.`, + ); + } const fields = spec.managedFields(resource); // Reverse-resolve numeric ids (campusId/groupTypeId/groupStatusId) to logical sugar so the // emitted snippet is portable and human-readable; unresolved ids stay numeric + a TODO (#52). diff --git a/src/commands/state.ts b/src/commands/state.ts index 1365e06..6f1c7d5 100644 --- a/src/commands/state.ts +++ b/src/commands/state.ts @@ -1,14 +1,23 @@ import { Command } from "commander"; import { resolveConfig } from "../config.js"; +import { loadConfig, resolveConfigPath } from "../config/load.js"; import { prepareEnv } from "../env/context.js"; -import { loadState } from "../state/state.js"; -import { info, out } from "../ui.js"; +import { loadState, saveState } from "../state/state.js"; +import { resourceType } from "../resources/registry.js"; +import { collectRefs, isRef, type Ref } from "../resolve/refs.js"; +import { info, out, success, warn } from "../ui.js"; interface StateOptions { state?: string; env?: string; } +interface StateRmOptions extends StateOptions { + config?: string; + force?: boolean; + dryRun?: boolean; +} + export function stateCommand(): Command { const cmd = new Command("state").description("Inspect the managed-resource state file"); @@ -26,5 +35,133 @@ export function stateCommand(): Command { out(resources); }); + // `ct state rm` — the missing inverse of `ct adopt` (#122). + // + // `ct adopt` writes an entry; nothing removed one. `ct destroy` is the opposite of what un-adopting + // means (it deletes the resource IN ChurchTools), so backing out an adoption meant hand-editing + // `ct-state..json` with a text editor or a `node -e` one-liner — the very file the tool + // insists is its own. Adopt-then-declare is the documented loop, so an adoption that turns out to + // be wrong is normal rather than exotic; leaving the entry in makes `plan` report a DESTROY for a + // resource where nothing is wrong, and the offline config-matches-state check cannot be satisfied + // without editing the file the check is checking. + cmd + .command("rm") + .description("Un-adopt: remove a resource from the state file. Never touches ChurchTools.") + .argument("", "resource type, e.g. campus | group | group-role") + .argument("", "logical key of the entry to remove") + .option("-s, --state ", "state file path (or set CT_STATE)") + .option("-e, --env ", "environment profile from ct.envs.json (host + state + token)") + .option("-c, --config ", "config file to check the key against (or set CT_CONFIG)") + .option("--force", "remove even though the key is still declared in the config") + .option("--dry-run", "report what would be removed without writing") + .action(async (type: string, key: string, opts: StateRmOptions) => { + // Validate the type against the registry first, so a typo is a clear error rather than a + // confusing "not in state" for an entry that could never have existed. + resourceType(type); + + const cmdEnv = await prepareEnv(opts); + const statePath = cmdEnv.statePath; + const state = await loadState(statePath, (await resolveConfig()).host); + + const entry = state.resources[key]; + if (!entry) { + throw new Error(`No entry "${key}" in ${statePath}. List them with \`ct state list\`.`); + } + if (entry.type !== type) { + throw new Error( + `"${key}" in ${statePath} is a ${entry.type} (#${entry.id}), not a ${type}. ` + + `Pass the right type, or list them with \`ct state list\`.`, + ); + } + + // Removing an entry the config still declares turns the next plan into a CREATE for a resource + // that already exists on the host — which then 400s on a duplicate name, or worse, succeeds and + // leaves a second copy. So it is refused by default; --force is there for the case where the + // declaration is being deleted in the same change. + if (!opts.force) { + const declared = await declaredKeys(opts.config); + if (declared?.has(key)) { + throw new Error( + `"${key}" is still declared in the config, so removing it from state would make the next ` + + `plan propose CREATING a resource that already exists on this host. Remove the ` + + `declaration first, or pass --force if you are deleting both in the same change.`, + ); + } + } + + if (opts.dryRun) { + info(`Would remove ${entry.type}.${key} (#${entry.id}) from ${statePath}.`); + return; + } + + delete state.resources[key]; + await saveState(statePath, state); + success(`Removed ${entry.type}.${key} (#${entry.id}) from ${statePath}.`); + // The single most important thing to say: nothing happened in ChurchTools. Someone reaching for + // this command has usually just been told that `ct destroy` is the wrong tool. + info( + `ChurchTools was not contacted — #${entry.id} still exists there, now unmanaged. ` + + `Re-adopt it with \`ct adopt ${type} ${entry.id}\`.`, + ); + }); + return cmd; } + +/** + * The logical keys the config declares, or `undefined` when the config cannot be read. + * + * A missing/broken config must not block an un-adopt: backing out an adoption is exactly what you do + * when the config is mid-edit. So a load failure downgrades the guard to a warning rather than + * failing the command. + */ +/** + * Every logical key the config still names — as a declared resource, AND as a reference from a + * permission declaration. + * + * The permission half matters because a key can be referenced without being declared as a resource in + * the same breath: a `ct.groupRole({ group: "" })` domain, or a group-dimension + * `scope: [""]`. Removing such a key from state passes a resources-only guard and then hard-errors + * on the next `ct plan` in `resolveScope` ("does not resolve to a managed group") — which is precisely + * the broken-config outcome this guard exists to catch before the write, not after it. + * + * Embedded `{ __ctRef }` markers are collected structurally, so a new referencing position is covered + * the day it is added; compound refs contribute the key they actually name (`group-role` a group, + * `group-type-role` a group type). The scope sugar (`{ group: "x" }`) and the bare-string group key + * are NOT refs, so they are picked up explicitly — leniently, since a malformed entry is `ct plan`'s + * to report, and this guard must not turn it into a failure to read the config at all. + */ +async function declaredKeys(configOpt: string | undefined): Promise | undefined> { + try { + const { resources, permissions } = await loadConfig(resolveConfigPath(configOpt)); + const keys = new Set(resources.map((r) => r.key)); + const addRef = (r: Ref): void => { + if (r.kind === "group-role") keys.add(r.group); + else if (r.kind === "group-type-role") keys.add(r.groupType); + else keys.add(r.key); + }; + for (const ref of collectRefs(permissions)) addRef(ref); + for (const p of permissions) { + for (const g of p.grants) { + if (typeof g === "string" || !Array.isArray(g.scope)) continue; + for (const entry of g.scope) { + // A bare string is a logical GROUP key; one-field sugar names a key on its dimension. + if (typeof entry === "string" && entry.length > 0) keys.add(entry); + else if (entry !== null && typeof entry === "object" && !isRef(entry)) { + const values = Object.values(entry as Record); + if (values.length === 1 && typeof values[0] === "string" && values[0].length > 0) { + keys.add(values[0]); + } + } + } + } + } + return keys; + } catch (err) { + warn( + `Could not read the config to check whether "${configOpt ?? "the default config"}" still ` + + `declares this key (${err instanceof Error ? err.message : String(err)}) — removing anyway.`, + ); + return undefined; + } +} diff --git a/src/config/query-refs.ts b/src/config/query-refs.ts index 28c9177..0703fd8 100644 --- a/src/config/query-refs.ts +++ b/src/config/query-refs.ts @@ -12,6 +12,7 @@ */ import type { GroupTypeRoleRef, RefKind, SimpleRef } from "../resolve/refs.js"; import type { RoleCatalogEntry } from "../resolve/reverse.js"; +import { slug } from "../resources/registry.js"; export type { RoleCatalogEntry }; @@ -29,11 +30,17 @@ export type { RoleCatalogEntry }; * `role.id` is DELIBERATELY NOT here (fixed in #76, reverting #86's `role-def` mapping). A ruleset's * `role.id` is a **groupTypeRoleId** — a role scoped to a group TYPE — not a global role-catalog id. * Role NAMES are not globally unique across group types (live prod, 2026-07-11: 3 roles named "Leiter", - * 6 "Organisator", 6 "Mitglied", each on a different group type), so mapping it to `role-def` (which - * keys `/group/roles` by `slug(name)` alone) makes the resolver throw "ambiguous". Only the - * (groupTypeId, name) PAIR is unique (0 collisions across all 46 prod roles), so `role.id` needs the - * catalog-driven special case in {@link portablizeRuleset} that emits a `group-type-role` marker — a - * lone name-based table entry cannot express it. + * 6 "Organisator", 6 "Mitglied", each on a different group type), so a NAME-KEYED `role-def` mapping + * makes the resolver throw "ambiguous". So `role.id` needs the special case in + * {@link portablizeRuleset}, which a lone name-based table entry cannot express. That special case + * now emits, in order of preference: + * + * 1. `{ kind: "role-def", key }` when the role is under MANAGEMENT (#125) — a per-host id under a + * shared logical key, resolved from state, so it is unambiguous by construction. This is what + * #86 got wrong: the mapping is right, the name-keyed catalog it used to source it from was not. + * 2. `{ kind: "group-type-role", groupType, role }` otherwise. The (groupTypeId, name) PAIR is + * unique in the common case (0 collisions across all 46 prod roles) — but NOT always, and when + * it collides there is no fix available in a shared config, which is precisely why (1) exists. * * Deliberately absent (the escape hatch — unknown vars are left untouched by {@link portablizeRuleset}): * - `ctgroup.groupStatusId` — group statuses have NO REST catalog (#67; `/group/memberstatus` is a @@ -107,6 +114,23 @@ export interface PortablizeWarning { */ const UNPORTABLE_ENTITY_VARS: Readonly> = { "ctgroup.groupStatusId": "group statuses have no REST catalog (#67) — no logical form exists", + // #127. A ruleset that includes or excludes specific people by id is common — four of five + // auto-group rulesets captured in one week did it — and it was the ONE entity var the audit never + // mentioned, so the only way to find it was to read the captured JSON by hand. The absence of a + // warning actively implied there was nothing to find. + // + // Unlike every other entry here, this one is not "pending a catalog": ct correctly does not manage + // people, so there is no person catalog to resolve against and no `__ctRef` kind that could express + // it. The ask is only that the tool SAY so, and the wording has to make clear it is unfixable + // rather than not-yet-fixed — hence a remedy that is a decision ("remove the clause or accept the + // divergence"), not a command to run. + // + // `person.id` 1 is the unluckiest case: it exists on every ChurchTools instance and is almost + // always an administrator, so an exclusion clause aimed at one person on the source host lands on + // someone real on the target host rather than harmlessly matching nothing. + "person.id": + "person ids are NEVER portable — ct does not manage people, so this ruleset names DIFFERENT " + + "people on another host. Remove the clause or accept the divergence", }; /** @@ -197,6 +221,21 @@ export function portablizeRuleset( const marker = (kind: RefKind, key: string): SimpleRef => ({ __ctRef: true, kind, key }) as SimpleRef; + /** + * Does this role's (groupTypeId, slug(name)) pair match more than one catalog row — i.e. is the + * `group-type-role` marker unable to name it? Counted once per call, over the whole catalog, using + * the SAME key the resolver filters on (`resolveGroupTypeRole`), so the two agree by construction. + */ + const pairCounts = new Map(); + const pairKey = (e: RoleCatalogEntry): string => `${e.groupTypeId}${slug(e.name)}`; + if (roleCatalog) { + for (const e of roleCatalog.values()) { + const k = pairKey(e); + pairCounts.set(k, (pairCounts.get(k) ?? 0) + 1); + } + } + const roleNameAmbiguous = (e: RoleCatalogEntry): boolean => (pairCounts.get(pairKey(e)) ?? 0) > 1; + const mapScalar = (value: unknown, kind: RefKind, varName: string): unknown => { if (typeof value !== "number") return value; // booleans/strings/nulls are literals, never entity ids const key = idToKeyByKind[kind]?.get(value); @@ -212,6 +251,36 @@ export function portablizeRuleset( if (typeof value !== "number") return value; const entry = roleCatalog?.get(value); const groupTypeKey = entry ? groupTypeIdToKey?.get(entry.groupTypeId) : undefined; + + // A MANAGED role-def wins over the (group-type, role-name) pair — but ONLY when that pair is + // genuinely ambiguous (#125, narrowed). + // + // The pair is resolved by filtering `/group/roles` on (groupTypeId, slug(name)), so two rows + // sharing a name on one group type are a hard error — and neither remedy the error suggests works + // for a config shared across hosts. "Rename to disambiguate" means editing ChurchTools master + // data to work around a config limitation, and in the observed case one of the two rows is CT's + // own stock `leader`. "Pass a numeric id" cannot work at all: the roles have different ids per + // host (#127 on one, #207 on the other) and `ConfigContext` deliberately exposes no env or host, + // so there is nowhere to branch. A `role-def` ref carries a per-host id in managed state — the + // same mechanism `group` and `campus` refs already use — so it makes that case fixable IN CONFIG, + // by adopting the role under a shared key on both hosts. + // + // Why it must NOT win unconditionally: `role-def` is the WEAKER reference of the two off this + // host. `Resolver.resolveSimple` falls back to `resolveFromCatalog`, which keys `/group/roles` by + // slug(name) ALONE — the very mapping this module's header calls wrong for a `role.id`, and the + // reason #76 reverted #86. So on a host where the role was never adopted under the shared key, a + // name matching exactly one row resolves SILENTLY to a role on a different group type (only a + // multi-row match errors), and the ruleset then matches the wrong role. The `group-type-role` + // marker cannot fail that way: it resolves on the (groupTypeId, name) pair, which is unique by + // construction. So the safe pair stays the safe pair, and `role-def` is reserved for the case + // that has no safe pair to lose. + const ambiguous = entry !== undefined && roleNameAmbiguous(entry); + if (ambiguous || groupTypeKey === undefined) { + const roleDefKey = idToKeyByKind["role-def"]?.get(value); + if (roleDefKey !== undefined) { + return marker("role-def", roleDefKey); + } + } if (entry && groupTypeKey !== undefined) { return { __ctRef: true, diff --git a/src/coverage/report.ts b/src/coverage/report.ts index 66a12ac..f57f38d 100644 --- a/src/coverage/report.ts +++ b/src/coverage/report.ts @@ -19,7 +19,7 @@ * the I/O, so the whole verdict is unit-testable without a network. */ import { KNOWN_AUTH_IDS, SCOPE_FIELD_BY_AUTH_ID } from "../permissions/catalog.js"; -import { normalizeActual, type RawPermission } from "../permissions/grants.js"; +import { normalizeActual, normalizeEffective, type RawPermission } from "../permissions/grants.js"; import { ALL_SCOPE_SENTINEL, SCOPE_REF_KIND } from "../permissions/scope.js"; import { fromInformation } from "../resources/registry.js"; import type { State } from "../state/state.js"; @@ -114,11 +114,26 @@ export interface CoverageInput { * ownership default. (With `preserveUnknown` (#102) those same instances become declarable while * leaving the module grants alone; the verdict below is the strict one, and the command surfaces the * blocking dimensions precisely so they can be passed to `preserveUnknown`.) + * + * `scope` picks WHICH rows the verdict is about, and the two callers genuinely need different ones: + * - `"authored"` (default) — `ct coverage`, whose headline number is "grants an admin authored here". + * Inherited rows are somebody else's authorship and would inflate it. + * - `"effective"` — `ct adopt grants`, which since #114/#119 EMITS the effective set. The verdict has + * to be about the same rows the emitter will print, or it decides on rows that never reach the + * block: a domain carrying rights only as inherited rows counts as empty and is skipped (so the + * rights stay undeclared and the next apply on the other host revokes them), and an inherited grant + * on an unnameable dimension is invisible to the `blockedBy` gate that exists to stop exactly that + * line from being emitted as a host-specific number. */ -export function declarability(rows: RawPermission[]): DeclarabilityVerdict { - // `normalizeActual` is what makes the count right: it drops the self-re-adding system baseline and - // every inherited row, so the verdict is about ADMIN-AUTHORED grants — the only ones ct ever owns. - const tuples = normalizeActual(rows).filter((t) => t.type === "grant"); +export function declarability( + rows: RawPermission[], + { scope = "authored" }: { scope?: "authored" | "effective" } = {}, +): DeclarabilityVerdict { + // Both normalizers drop the self-re-adding system baseline; they differ only on inherited rows, + // which `normalizeActual` also drops. Neither decides ownership — that stays `normalizeActual`'s + // job at plan time, so widening the verdict here never widens what ct writes or revokes. + const normalize = scope === "effective" ? normalizeEffective : normalizeActual; + const tuples = normalize(rows).filter((t) => t.type === "grant"); const blockedBy = new Set(); const unknownAuthIds = new Set(); for (const t of tuples) { diff --git a/src/permissions/adopt.ts b/src/permissions/adopt.ts index 66f46c5..8fa8079 100644 --- a/src/permissions/adopt.ts +++ b/src/permissions/adopt.ts @@ -4,12 +4,22 @@ * instance's rights structure comes under management without hand-transcription * (issue #25). * - * The live rows are run through the SAME normalization the planner uses - * (`normalizeActual`) so what is emitted is exactly what a subsequent `ct plan` - * would consider managed: the system baseline (`modifiedPid === -1`) and - * inherited rows are dropped, and pre-existing revoke/deny rows are surfaced as - * a note rather than emitted (the reconciler preserves them; re-authoring them - * is out of scope here). + * The live rows are run through the SAME normalization the planner reconciles + * against — `normalizeEffective` (#114/#119) — so what is emitted is exactly + * what a subsequent `ct plan` would consider SATISFIED: every right the host + * grants, whether directly, by inheritance, or as system baseline. + * + * That is deliberately broader than what ct OWNS. Emitting only the owned rows + * encoded one host's provenance into a config meant to drive several, and the + * same right can be inherited on one host and direct on another — so a right + * dropped here would be undeclared there, land in `toDelete`, and be revoked. + * Declaring it keeps the block a clean no-op on both, and ct still never + * authors or revokes a row it does not own (that is `normalizeActual`'s job, + * not adoption's). A footer names how many of the emitted grants are in that + * category. + * + * Pre-existing revoke/deny rows are surfaced as a note rather than emitted (the + * reconciler preserves them; re-authoring them is out of scope here). * * Grants are NOT state-tracked resources, so adoption prints config only — it * never writes the state file. The caller makes that explicit in its output. @@ -17,8 +27,15 @@ import type { State } from "../state/state.js"; import { findByTypeId } from "../state/state.js"; import { CATALOG } from "./catalog.js"; -import { normalizeActual, type DomainType, type GrantTuple, type RawPermission } from "./grants.js"; -import { GROUP_SCOPE_FIELD, SCOPE_REF_KIND } from "./scope.js"; +import { + normalizeActual, + normalizeEffective, + unownedGrantCount, + type DomainType, + type GrantTuple, + type RawPermission, +} from "./grants.js"; +import { ALL_SCOPE_SENTINEL, GROUP_SCOPE_FIELD, SCOPE_REF_KIND } from "./scope.js"; /** DSL function name for each domain type — the call the emitted block should be pasted as. */ const DSL_FN: Record = { @@ -119,6 +136,12 @@ export interface AdoptedGrantsBlock { omitted: number; /** Pre-existing deny rows on this domain; preserved by the reconciler, never emitted. */ revokes: number; + /** + * How many emitted grants are INHERITED or system-baseline on this host (#114/#119) — declared so + * the block is a no-op on a host that materializes them as direct rows, but never authored or + * revoked by ct on this one. + */ + unowned: number; } export function emitAdoptedGrants(args: AdoptGrantsArgs): string { @@ -127,9 +150,23 @@ export function emitAdoptedGrants(args: AdoptGrantsArgs): string { export function buildAdoptedGrants(args: AdoptGrantsArgs): AdoptedGrantsBlock { const { domainType, domainId, rows, state, domain, key } = args; - const normalized = normalizeActual(rows); + // Emit the EFFECTIVE set — direct, inherited and system-baseline rows alike (#114/#119). + // + // Adoption used to emit only the rows ct owns, which encoded THIS host's provenance into a config + // meant to drive several. On a host that materializes the same rights differently — as direct rows + // rather than inherited ones — every dropped right is undeclared, so it lands in `toDelete` and + // apply strips it. Measured on two hosts of one instance: adopting one and planning the other + // proposed removing 15 group-member rights from the largest role instance in the estate. + // + // Emitting the effective set makes the block a no-op on BOTH: the reconciler now treats a right the + // host grants by any route as satisfied (see `normalizeEffective`), so a declared-but-inherited + // right causes no PUT here and no revoke there. ct still never authors or revokes a row it does not + // own — that is decided by `normalizeActual`, not by what adoption prints. + const normalized = normalizeEffective(rows); const grants = normalized.filter((t) => t.type === "grant"); - const revokes = normalized.filter((t) => t.type !== "grant"); + // Deny rows stay owned-only: an inherited deny is not this domain's to restate. + const revokes = normalizeActual(rows).filter((t) => t.type !== "grant"); + const unowned = unownedGrantCount(rows); const rev = reverseCatalog(); const body: string[] = []; @@ -151,7 +188,7 @@ export function buildAdoptedGrants(args: AdoptGrantsArgs): AdoptedGrantsBlock { } if (grants.length === 0) { - body.push(" grants: [], // no user-authored grants on this domain (baseline/inherited rows excluded)"); + body.push(" grants: [], // this domain grants nothing"); } else { body.push(" grants: ["); for (const g of collapse(grants)) { @@ -182,6 +219,21 @@ export function buildAdoptedGrants(args: AdoptGrantsArgs): AdoptedGrantsBlock { } lines.push(...body); + if (unowned > 0) { + // #114/#119 asked for exactly this: the asymmetry named UP FRONT, not discovered later as an + // unexplained revoke in the other host's plan after the config has already been written. + lines.push( + `// NOTE: ${unowned} of the grant(s) above are INHERITED or system-baseline on this host — ct never`, + ); + lines.push( + "// authors or revokes those rows. They are emitted anyway because another host of the same instance", + ); + lines.push( + "// may materialize the same rights as direct rows; declaring them is what keeps this block a clean", + ); + lines.push("// no-op on BOTH hosts instead of a 15-right revoke on one of them (#114/#119)."); + } + if (revokes.length > 0) { lines.push( `// NOTE: ${revokes.length} revoke/deny row(s) exist on this domain. The reconciler PRESERVES them (it never`, @@ -192,7 +244,7 @@ export function buildAdoptedGrants(args: AdoptGrantsArgs): AdoptedGrantsBlock { lines.push("// config is not supported yet (see issue #25 stretch goal)."); } - return { block: lines.join("\n"), omitted, revokes: revokes.length }; + return { block: lines.join("\n"), omitted, revokes: revokes.length, unowned }; } /** The lines emitted for one collapsed grant, plus whether a LIVE grant was left as a comment @@ -257,7 +309,18 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St // emitter is deliberately pure — no client, no fetch — so it cannot turn the id into a name. // Emit the number and point at the portable form the author can write by hand. const sugar = dimension?.managed ? SCOPE_SUGAR_FIELD[dimension.type] : undefined; - if (dimension && !dimension.managed) { + // #115: `-1` is not an id at all — it is ChurchTools' "alle" sentinel, meaning every value of + // the dimension on whatever host reads it. So it is ALREADY portable, and none of the + // host-specific-id advice below applies to it. Emitting `ct adopt department -1` sent people + // looking for a resource that cannot exist, and in a real adoption run this fires on most of + // the broadly-scoped grants — the interesting ones. + const realIds = g.dataIds.filter((id) => id !== ALL_SCOPE_SENTINEL); + const hasSentinel = g.dataIds.length > realIds.length; + if (hasSentinel) { + const what = dimension ? `every ${dimension.type}` : `every value of "${entry.scopeField}"`; + out.push(` // scope -1 is ChurchTools' "alle" sentinel — ${what}; host-independent.`); + } + if (dimension && !dimension.managed && realIds.length > 0) { const field = SCOPE_SUGAR_FIELD[dimension.type] ?? dimension.type; out.push( ` // NOTE: "${entry.name}" scopes by "${entry.scopeField}" (${dimension.type}), a catalog ct reads but does not manage —`, @@ -269,6 +332,11 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St const entries: string[] = []; const unmanaged: number[] = []; for (const id of [...g.dataIds].sort((a, b) => a - b)) { + if (id === ALL_SCOPE_SENTINEL) { + // Portable as-is (see above) — emitted, but never counted as an unmanaged host-specific id. + entries.push(String(id)); + continue; + } const managed = dimension && sugar ? findByTypeId(state, dimension.type, id) : undefined; if (managed && sugar) { entries.push(`{ ${sugar}: ${JSON.stringify(managed.key)} }`); @@ -313,7 +381,15 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St // bring it under management (rather than silently declaring an opaque numeric scope for it). const resolvedKeys: string[] = []; const unmanaged: number[] = []; + // #115: `-1` is the "alle" sentinel — every group on whatever host reads it — not an id that + // could ever be adopted. It is already portable, so it is emitted verbatim through the numeric + // escape hatch and never counted as an unmanaged target. + let sentinel = false; for (const id of g.dataIds) { + if (id === ALL_SCOPE_SENTINEL) { + sentinel = true; + continue; + } const group = findByTypeId(state, "group", id); if (group) resolvedKeys.push(group.key); else unmanaged.push(id); @@ -341,8 +417,14 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St out.push(` // then add its logical key to the scope array below.`); omitted = true; } - if (resolvedKeys.length > 0) { - const scope = resolvedKeys.map((k) => JSON.stringify(k)).join(", "); + if (sentinel) { + out.push(` // scope -1 is ChurchTools' "alle" sentinel — every group; host-independent.`); + } + if (resolvedKeys.length > 0 || sentinel) { + const scope = [ + ...(sentinel ? [String(ALL_SCOPE_SENTINEL)] : []), + ...resolvedKeys.map((k) => JSON.stringify(k)), + ].join(", "); out.push(` { right: ${JSON.stringify(entry.name)}, scope: [${scope}] },`); } else if (unmanaged.length > 0) { // Every scope target is unmanaged: there is no valid key to emit, so the grant itself is a diff --git a/src/permissions/grants.ts b/src/permissions/grants.ts index 005ecec..a2f676c 100644 --- a/src/permissions/grants.ts +++ b/src/permissions/grants.ts @@ -75,16 +75,72 @@ export function tupleKey(t: { return `${t.type}:${t.authId}:${scope}`; } +/** One raw row → its tuple. dataId is `[]` or a single element (CT reads scoped grants back one row + * per dataId), so there is nothing to sort — `tupleKey` sorts defensively anyway. */ +function toTuple(r: RawPermission): GrantTuple { + return { authId: r.authId, dataId: r.dataId == null ? [] : [r.dataId], type: r.type }; +} + +/** Is this row one ct may WRITE or REVOKE — i.e. a direct, user-authored row? */ +function isOwnable(r: RawPermission): boolean { + if (r.meta?.modifiedPid === -1) return false; // system baseline — ct never authors or revokes it + if (r.isInherited) return false; // granted via the type — ct never authors or revokes it + return true; +} + +/** + * The rows reconciliation OWNS: direct, user-authored grants. These are the only rows `ct` will ever + * write or revoke, and the only candidates for `toDelete`. + */ export function normalizeActual(rows: RawPermission[]): GrantTuple[] { - const out: GrantTuple[] = []; + return rows.filter(isOwnable).map(toTuple); +} + +/** + * The rows that describe what the host EFFECTIVELY grants — direct, inherited and system-baseline + * alike (#114/#119). + * + * `ct` used to decide "is this declared grant already satisfied?" from the owned set alone, which + * meant it decided from **how a row was recorded**. That differs per host for identical effective + * permissions, and it is what made a config unable to be a clean no-op on two hosts of one instance: + * + * - #114 — the same right is system-authored (`meta.modifiedPid === -1`) on one host and + * user-authored on the other, because the copy that produced the second host stamped a person id + * onto rows that are system rows upstream. Declaring the right planned `+1 grant` on the first + * host; omitting it planned a **revoke** on the second. + * - #119 — the same right is `isInherited: true` on one host and `false` on the other. Measured on + * two hosts of one instance, both CT 3.135.2: 18 of 63 `group_role` domains carried inherited + * rows on one, and **zero** anywhere on the other. Adoption dropped the inherited rows, so + * planning the result elsewhere proposed stripping 15 rights from the largest role instance in + * the estate. + * + * Reconciling against the effective set fixes both, because the two hosts agree on effective + * permissions even when they disagree on provenance. The "never fight the platform" property still + * holds: {@link normalizeActual} still decides what may be written or revoked, so ct never authors + * and never revokes a baseline or inherited row — it only stops proposing to re-author something the + * platform already grants. + */ +export function normalizeEffective(rows: RawPermission[]): GrantTuple[] { + // DEDUPED by identity: a right can appear both as a direct row and as an inherited one on the same + // domain, and "effective" is a set, not a tally. Without this, `adopt grants` would print the same + // grant twice. The OWNED row wins when both exist, so anything downstream that reads a tuple's + // provenance-adjacent fields sees the writable one. + const byKey = new Map(); for (const r of rows) { - if (r.meta?.modifiedPid === -1) continue; // system baseline — invisible to reconciliation - if (r.isInherited) continue; // inherited — not directly owned here - // dataId is [] or a single element (CT reads scoped grants back one row per dataId), so there is - // nothing to sort here — and tupleKey sorts defensively anyway when it builds the identity key. - out.push({ authId: r.authId, dataId: r.dataId == null ? [] : [r.dataId], type: r.type }); + const t = toTuple(r); + const k = tupleKey(t); + if (!byKey.has(k) || isOwnable(r)) byKey.set(k, t); } - return out; + return [...byKey.values()]; +} + +/** How many of a domain's effective grants ct does NOT own — inherited or system-baseline rows. */ +export function unownedGrantCount(rows: RawPermission[]): number { + const owned = new Set(rows.filter(isOwnable).map((r) => tupleKey(toTuple(r)))); + const unowned = new Set( + rows.filter((r) => !isOwnable(r) && r.type === "grant").map((r) => tupleKey(toTuple(r))), + ); + return [...unowned].filter((k) => !owned.has(k)).length; } export interface GrantDiff { @@ -111,6 +167,13 @@ export function diffGrants( desired: GrantTuple[], actual: GrantTuple[], preserveUnknown?: PreservePredicate, + /** + * What the host effectively grants, including rows ct does not own (inherited, system baseline) — + * see {@link normalizeEffective}. Used ONLY to decide whether a declared grant is already + * satisfied, never to decide what to revoke. Defaults to `actual`, which is the pre-#114/#119 + * behaviour and keeps every existing caller and test honest. + */ + effective: GrantTuple[] = actual, ): GrantDiff { // Reconciliation owns only user-authored GRANT rows. `desiredTuples` only ever emits // `type: "grant"`, so an explicit deny row (`type: "revoke"`) has no desired counterpart and @@ -120,7 +183,11 @@ export function diffGrants( const preserved = actual.filter((t) => t.type !== "grant"); const desiredKeys = new Map(desired.map((t) => [tupleKey(t), t])); const actualKeys = new Map(managedActual.map((t) => [tupleKey(t), t])); - const toPut = [...desiredKeys].filter(([k]) => !actualKeys.has(k)).map(([, t]) => t); + // SATISFACTION is judged against the EFFECTIVE set (#114/#119): a right the host already grants by + // ANY route — directly, by inheritance, or as system baseline — needs no PUT. REVOCATION below is + // still judged against the owned set only, so ct never revokes a row it did not author. + const effectiveKeys = new Set(effective.filter((t) => t.type === "grant").map(tupleKey)); + const toPut = [...desiredKeys].filter(([k]) => !effectiveKeys.has(k)).map(([, t]) => t); const undeclared = [...actualKeys].filter(([k]) => !desiredKeys.has(k)).map(([, t]) => t); // Opt-in partial ownership (#102). Without it, ONE live grant on a dimension this tool has no // business managing (a wiki category, an HTML template) makes the whole role instance undeclarable, diff --git a/src/permissions/plan.ts b/src/permissions/plan.ts index ff687b6..957a013 100644 --- a/src/permissions/plan.ts +++ b/src/permissions/plan.ts @@ -19,6 +19,7 @@ import { compareVersions } from "../api/version.js"; import { resolveScope, resolveScopeRefs, type ScopeRefMap } from "./scope.js"; import { normalizeActual, + normalizeEffective, diffGrants, type GrantTuple, type GrantDiff, @@ -287,7 +288,12 @@ export async function buildPermissionPlan( } 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)); + const domainRows = all.filter((r) => r.domainId === p.domainId); + const normalizedAll = normalizeActual(domainRows); + // Everything the host grants on this domain by ANY route, provenance included (#114/#119). Only + // ever consulted to answer "is this declared grant already satisfied?" — never to decide a + // revoke, which stays the owned set below. + const effectiveAll = normalizeEffective(domainRows); // Actuals are already filtered by `normalizeActual`: the self-re-adding system baseline // (`modifiedPid === -1`) and every `isInherited` row are dropped, so what remains is admin-authored // DIRECT grants — INCLUDING the writable `authId >= 10000` group-member rights Equippers curates on @@ -324,6 +330,9 @@ export async function buildPermissionPlan( desiredTuples(p, state, declaredGroupKeys, scopeRefs), knownActual, preservePredicateFor(p.preserveUnknown), + // The unknown-authId guard (#25) deliberately does NOT apply here: an unnameable right can + // still satisfy a declaration, and excluding it would only re-propose a PUT we cannot name. + effectiveAll, ), }); } diff --git a/src/resolve/resolver.ts b/src/resolve/resolver.ts index 81dfc8c..2c03127 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -399,9 +399,17 @@ export class Resolver { } if (matches.length > 1) { const list = matches.map((c) => `${JSON.stringify(c.name)} (#${c.id})`).join(", "); + // #125: the old message offered "rename to disambiguate, or pass a numeric id", and BOTH are + // dead ends for a config shared across hosts — a rename edits ChurchTools master data (often + // CT's own stock role) to work around a config limitation, and the roles have different ids per + // host with no env to branch on. Adopting the role is the remedy that actually works, and it is + // the same "adopt the target to portablize it" move groups and campuses already use. + const adopts = matches.map((c) => `\`ct adopt group-role ${c.id} -k \``).join(" / "); throw new Error( `Ambiguous ${refLabel(r)} referenced at ${site} on ${this.host}: ${matches.length} roles on group ` + - `type #${groupTypeId} match — ${list}. Rename to disambiguate, or pass a numeric id.`, + `type #${groupTypeId} match — ${list}. Adopt the one you mean under a logical key, on EVERY ` + + `host, and reference it as { kind: "role-def", key: "" } — ${adopts}. ` + + `(Renaming in ChurchTools and passing a numeric id both work on one host only.)`, ); } return matches[0]!.id; diff --git a/src/resources/registry.ts b/src/resources/registry.ts index 32c5daf..9edaba7 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -309,7 +309,16 @@ export const RESOURCES: Record = { tier: 0, // CT's campus short name is `shorty` (1–10 chars, required on create) — verified // live. `shortName` is a vestigial, usually-null sibling; do not use it for writes. - deriveKey: (r) => slug(str(r, "shorty") || str(r, "name")), + // + // The KEY comes from `name`, like every other resource in this registry (#118). It used to + // prefer `shorty`, which made campus the sole exception and produced keys nobody could read: + // Idstein → `swa` (abbreviated after the region it used to be named for), Würzburg → `wu`, + // Dietzenbach → `of` — and `{ campus: "of" }` inside a grant scope reads as a typo in the tool + // rather than as a name. `shorty` is also less stable: it is free text an admin edits in the UI + // to make a column fit, and every such edit silently changes what the derived key WOULD be, + // splitting new adoptions from the ones already in state. It is still written as a managed field + // exactly as before — this is only about the key. + deriveKey: (r) => slug(str(r, "name") || str(r, "shorty")), managedFields: (r) => ({ name: r.name, shorty: r.shorty }), }), group: define({ diff --git a/src/state/state.ts b/src/state/state.ts index 10a2fe1..105c950 100644 --- a/src/state/state.ts +++ b/src/state/state.ts @@ -154,6 +154,45 @@ export function isManaged(state: State, type: string, id: number): boolean { return findByTypeId(state, type, id) !== undefined; } +/** + * The key a re-adoption should actually use, and whether it differs from the derived one (#123). + * + * `ct adopt group --with-dynamic` is the documented way to refresh a dynamic group's ruleset + * once its scope targets become managed, and it runs over a LIST of ids — the one mode where `-k` is + * rejected ("only valid when exactly one group is resolved"). Any resource whose adopted key differed + * from the derived key was therefore silently re-keyed by that refresh: the config's declaration + * matched nothing in state, a state entry existed that nothing declared, and the next plan read as + * "one to create, one to destroy" for a resource that was fine and untouched on the host. The message + * said only that the snapshot was "refreshed", which reads as *nothing else changed*. + * + * An adopted key was chosen deliberately — very likely with `-k`, to match the config's naming — so a + * refresh touches the snapshot, not the identity. + */ +export interface AdoptKeyChoice { + /** The key to store under. */ + key: string; + /** Set when the derived key differs from the kept one — the caller warns with this. */ + wouldBecome?: string; +} + +/** + * Choose the key for an adoption. A resource already in state keeps its key unless `rekey` is set; + * an explicit `--key` is an explicit intent and always wins. New resources just take `derived`. + */ +export function chooseAdoptKey( + state: State, + type: string, + id: number, + derived: string, + opts: { explicitKey?: string; rekey?: boolean } = {}, +): AdoptKeyChoice { + const explicit = opts.explicitKey?.trim(); + if (explicit) return { key: explicit }; + const existing = findByTypeId(state, type, id); + if (!existing || opts.rekey || existing.key === derived) return { key: derived }; + return { key: existing.key, wouldBecome: derived }; +} + export interface UpsertInput { type: string; id: number; diff --git a/tests/adopt-grants-bulk.test.ts b/tests/adopt-grants-bulk.test.ts index 1a9ae2c..bfaf6d5 100644 --- a/tests/adopt-grants-bulk.test.ts +++ b/tests/adopt-grants-bulk.test.ts @@ -42,6 +42,8 @@ const groupRows = [ ], }, { id: 11, name: "Ops", information: { groupTypeId: 2 }, roles: [{ id: 200, groupTypeRoleId: 16 }] }, + // `youth` (#12, managed) carries its rights ONLY as inherited rows — the #119 shape. + { id: 12, name: "Youth", information: { groupTypeId: 2 }, roles: [{ id: 300, groupTypeRoleId: 16 }] }, ]; const roleDefRows = [ { id: 16, name: "Leiter", groupTypeId: 2 }, @@ -53,6 +55,8 @@ const permissions: RawPermission[] = [ perm(UNSCOPED, 101), perm(HTML_TEMPLATE, 101, 3), // blocks role instance 101 perm(UNSCOPED, 200), + { ...perm(UNSCOPED, 300), isInherited: true }, + { ...perm(VIEW_GROUP, 300, 12), isInherited: true }, ]; const getMock = vi.fn(async (path: string): Promise => { @@ -83,6 +87,7 @@ const state: State = { host: HOST, resources: { kids: { type: "group", id: 10, key: "kids", fields: { name: "Kids" }, adoptedAt: "t", updatedAt: "t" }, + youth: { type: "group", id: 12, key: "youth", fields: { name: "Youth" }, adoptedAt: "t", updatedAt: "t" }, }, }; @@ -165,6 +170,17 @@ describe("ct adopt grants --all-declarable (#104)", () => { expect(stdout).not.toContain("kids_mitglied"); }); + // The bulk counterpart of #119. The single-domain path emits the effective set; the bulk path used + // to decide whether to emit at all from the OWNED set, so a domain whose rights are all inherited + // here counted as "no authored grants" and was skipped. Its rights then stay undeclared — and on the + // host that materialises the same rights as DIRECT rows, the next plan puts them in toDelete. + it("emits a domain whose grants are all inherited, rather than counting it empty", async () => { + const { stdout } = await run(["grants", "--all-declarable"]); + expect(stdout).toContain('key: "youth_leiter"'); + expect(stdout).toContain('group: "youth"'); + expect(stdout).toContain("churchgroup:view group"); + }); + it("--write appends the blocks to a file instead of printing them", async () => { const target = join(workDir, "grants.ts"); writeFileSync(target, "// existing\n", "utf8"); diff --git a/tests/adopt-identity.test.ts b/tests/adopt-identity.test.ts new file mode 100644 index 0000000..2694f86 --- /dev/null +++ b/tests/adopt-identity.test.ts @@ -0,0 +1,84 @@ +/** + * Adoption identity: a re-adopt refreshes the SNAPSHOT, not the KEY (#123), and there is a supported + * way back out of an adoption (#122). + * + * `ct adopt group --with-dynamic` is the documented ruleset-refresh workflow and takes a LIST + * of ids — the one mode where `-k` is rejected — so any resource whose adopted key differed from the + * derived key was silently re-keyed by a routine refresh. The config's declaration then matched + * nothing in state, and the next plan read "one to create, one to destroy" for a resource that was + * fine and untouched on the host. Recovering meant hand-editing the state file, which had no + * supported command either. + */ +import { describe, it, expect } from "vitest"; +import { chooseAdoptKey, emptyState, upsert, type State } from "../src/state/state.js"; + +const HOST = "https://mychurch.church.tools"; + +/** State holding one group adopted long ago under a deliberately-chosen key. */ +function adopted(): State { + const state = emptyState(HOST); + state.resources.merkmal_alle_2_5_mz = { + type: "group", + id: 1990, + key: "merkmal_alle_2_5_mz", + fields: { name: "Alle 2 bis 5 MZ" }, + adoptedAt: "t", + updatedAt: "t", + }; + return state; +} + +describe("re-adopting keeps the adopted key (#123)", () => { + it("keeps the existing key when the derived one differs, and reports what it would have been", () => { + const choice = chooseAdoptKey(adopted(), "group", 1990, "alle_2_bis_5_mz"); + expect(choice.key).toBe("merkmal_alle_2_5_mz"); + expect(choice.wouldBecome).toBe("alle_2_bis_5_mz"); + }); + + it("re-keys only when --rekey is passed explicitly", () => { + const choice = chooseAdoptKey(adopted(), "group", 1990, "alle_2_bis_5_mz", { rekey: true }); + expect(choice.key).toBe("alle_2_bis_5_mz"); + expect(choice.wouldBecome).toBeUndefined(); + }); + + it("says nothing when the derived key already matches", () => { + const choice = chooseAdoptKey(adopted(), "group", 1990, "merkmal_alle_2_5_mz"); + expect(choice).toEqual({ key: "merkmal_alle_2_5_mz" }); + }); + + it("an explicit --key still wins — it is an explicit intent, not a derivation", () => { + const choice = chooseAdoptKey(adopted(), "group", 1990, "alle_2_bis_5_mz", { + explicitKey: "something_else", + }); + expect(choice.key).toBe("something_else"); + }); + + it("a NEW resource just takes the derived key", () => { + const choice = chooseAdoptKey(adopted(), "group", 4242, "brand_new"); + expect(choice).toEqual({ key: "brand_new" }); + }); + + it("does not confuse a same id of a different TYPE", () => { + // Ids are unique only within a type — the Mainz campus is id 0. A campus #1990 must not inherit + // the group's key. + expect(chooseAdoptKey(adopted(), "campus", 1990, "derived")).toEqual({ key: "derived" }); + }); + + it("the preserved key is what state ends up keyed by — no second entry, no orphan", () => { + const state = adopted(); + const choice = chooseAdoptKey(state, "group", 1990, "alle_2_bis_5_mz"); + upsert(state, { type: "group", id: 1990, key: choice.key, fields: { name: "Alle 2 bis 5 MZ" } }, "n"); + expect(Object.keys(state.resources)).toEqual(["merkmal_alle_2_5_mz"]); + expect(state.resources.merkmal_alle_2_5_mz?.id).toBe(1990); + // The stale-file half of the bug: the ruleset path is built from this key, so preserving it means + // the refresh overwrites rulesets/merkmal_alle_2_5_mz.json instead of writing a second file. + expect(`rulesets/${choice.key}.json`).toBe("rulesets/merkmal_alle_2_5_mz.json"); + }); + + it("still re-keys through upsert when --rekey was given (no orphan left behind)", () => { + const state = adopted(); + const choice = chooseAdoptKey(state, "group", 1990, "alle_2_bis_5_mz", { rekey: true }); + upsert(state, { type: "group", id: 1990, key: choice.key, fields: { name: "x" } }, "n"); + expect(Object.keys(state.resources)).toEqual(["alle_2_bis_5_mz"]); + }); +}); diff --git a/tests/adopt.test.ts b/tests/adopt.test.ts index daec5e8..082ebb7 100644 --- a/tests/adopt.test.ts +++ b/tests/adopt.test.ts @@ -49,12 +49,14 @@ describe("ct adopt", () => { it("adopts a campus (id 0) into the state file", async () => { await runAdopt(["campus", "0", "--state", statePath]); const state = await loadState(statePath, HOST); - expect(state.resources.mz).toMatchObject({ + // The key comes from `name`, like every other resource (#118) — not from `shorty` ("MZ"). + expect(state.resources.mainz).toMatchObject({ type: "campus", id: 0, - key: "mz", + key: "mainz", fields: { name: "Mainz", shorty: "MZ" }, }); + expect(state.resources.mz).toBeUndefined(); }); it("captures a campus-assigned group's campusId from information (#21)", async () => { diff --git a/tests/coverage.test.ts b/tests/coverage.test.ts index a706c94..7de3c97 100644 --- a/tests/coverage.test.ts +++ b/tests/coverage.test.ts @@ -72,6 +72,43 @@ describe("declarability (#103)", () => { }); }); +describe("declarability scope (#114/#119)", () => { + // `ct adopt grants` emits the EFFECTIVE set, so its gate has to judge the effective set too. + // Judging owned rows while emitting effective ones desynchronises the two in both directions. + it("counts inherited grants under scope: effective, so an all-inherited domain is not 'empty'", () => { + // The measured shape: a domain carrying its rights ONLY as inherited rows on this host. Under the + // authored count it looks empty, bulk adoption skips it, the rights stay undeclared — and the next + // apply on the host that materialises them as DIRECT rows puts them in toDelete and revokes them. + const rows = [ + row({ authId: VIEW_GROUP, dataId: 42, domainId: 1, isInherited: true }), + row({ authId: UNSCOPED, domainId: 1, isInherited: true }), + ]; + expect(declarability(rows).grantCount).toBe(0); + expect(declarability(rows, { scope: "effective" }).grantCount).toBe(2); + expect(declarability(rows, { scope: "effective" }).declarable).toBe(true); + }); + + it("blocks on an inherited grant with no logical form under scope: effective", () => { + // The opposite desync: the emitter prints this row as a host-specific numeric dataId, so the gate + // must see it. Under the authored verdict it is invisible and the domain passes --all-declarable. + const rows = [ + row({ authId: VIEW_GROUP, dataId: 42, domainId: 1 }), + row({ authId: HTML_TEMPLATE, dataId: 3, domainId: 1, isInherited: true }), + ]; + expect(declarability(rows)).toMatchObject({ declarable: true, blockedBy: [] }); + expect(declarability(rows, { scope: "effective" })).toMatchObject({ + declarable: false, + blockedBy: ["cc_html_template"], + }); + }); + + it("defaults to the authored set, so `ct coverage` is untouched", () => { + const rows = [row({ authId: VIEW_GROUP, dataId: 42, domainId: 1, isInherited: true })]; + expect(declarability(rows)).toEqual(declarability(rows, { scope: "authored" })); + expect(declarability(rows).grantCount).toBe(0); + }); +}); + describe("buildCoverageReport (#103)", () => { const state: State = { version: 1, diff --git a/tests/permission-adopt.test.ts b/tests/permission-adopt.test.ts index 3400e73..b613ed2 100644 --- a/tests/permission-adopt.test.ts +++ b/tests/permission-adopt.test.ts @@ -3,6 +3,7 @@ import { emitAdoptedGrants } from "../src/permissions/adopt.js"; import { diffGrants, normalizeActual, + normalizeEffective, type DomainType, type RawPermission, } from "../src/permissions/grants.js"; @@ -174,12 +175,27 @@ describe("emitAdoptedGrants", () => { expect(parseEmittedGrants(block)).toEqual([]); // comment only, no active grant line }); - it("emits an empty grants array when no user-authored grants remain", () => { + it("emits an empty grants array only when the domain grants NOTHING", () => { + const block = emitAdoptedGrants({ + domainType: "group_role", + domainId: 42, + rows: [], + state: emptyState(), + }); + expect(block).toContain("grants: [],"); + }); + + it("emits a system-baseline grant and says so, instead of silently dropping it (#114)", () => { + // The same right can be system-authored on one host and user-authored on another — the copy that + // produced the second host stamps a person id onto rows that are system rows upstream. Dropping + // it here produced a config that could not be a clean no-op on both: omitted, it REVOKED the + // right on the other host; declared by hand, it planned `+1 grant` on this one. const rows: RawPermission[] = [ - { authId: 2, dataId: null, type: "grant", domainId: 42, meta: { modifiedPid: -1 } }, // baseline only + { authId: 2, dataId: null, type: "grant", domainId: 42, meta: { modifiedPid: -1 } }, ]; const block = emitAdoptedGrants({ domainType: "group_role", domainId: 42, rows, state: emptyState() }); - expect(block).toContain("grants: [],"); + expect(block).not.toContain("grants: [],"); + expect(block).toContain("NOTE: 1 of the grant(s) above are INHERITED or system-baseline"); }); it("group_type_role admin-authored authId >= 10000 member right IS emitted as an active grant (#65)", () => { @@ -352,15 +368,58 @@ describe("emitAdoptedGrants", () => { expect(block).toContain("churchdb:+add person"); const grants = parseEmittedGrants(block); - // Paste-and-plan: diff the emitted declaration against the FULL live row set. normalizeActual - // drops the system-baseline + inherited rows, so they never appear as revokes. + // Paste-and-plan: diff the emitted declaration against the FULL live row set. const desired = grants.flatMap((g) => desiredTuples({ key: "adopted", domainType: "group_type_role", domainId: 9, grants: [g] }, state), ); const actual = normalizeActual(rows); - const diff = diffGrants(desired, actual); + const diff = diffGrants(desired, actual, undefined, normalizeEffective(rows)); + // The system-baseline and inherited rows ARE now declared (#114/#119) and are satisfied by the + // live rows, so they need no PUT... expect(diff.toPut).toEqual([]); - expect(diff.toDelete).toEqual([]); // critically: the system/inherited rows are NOT revoked + // ...and they are still never revoked: revocation is judged against the OWNED set only. + expect(diff.toDelete).toEqual([]); + }); + + it("declaring an inherited right is a no-op HERE and stops the revoke THERE (#119)", () => { + // The measured case: one role instance holds 15 group-member rights that the group's TYPE also + // grants. On prod they read `isInherited: true`; on dev, byte-identical effective permissions + // read `isInherited: false`. Adoption used to drop them on prod, so planning that config on dev + // proposed stripping all 15. + const AUTH = 10107; // churchdb:+add person — unscoped, so the block round-trips cleanly + const prodRows: RawPermission[] = [ + { authId: 1113, dataId: null, type: "grant", domainId: 9, meta: { modifiedPid: 5 } }, + { authId: AUTH, dataId: null, type: "grant", domainId: 9, isInherited: true }, + ]; + const devRows: RawPermission[] = [ + { authId: 1113, dataId: null, type: "grant", domainId: 9, meta: { modifiedPid: 5 } }, + { authId: AUTH, dataId: null, type: "grant", domainId: 9, meta: { modifiedPid: 1 } }, + ]; + + // Adopt from prod — the inherited right is emitted, not dropped. + const block = emitAdoptedGrants({ + domainType: "group_type_role", + domainId: 9, + rows: prodRows, + state: emptyState(), + }); + const grants = parseEmittedGrants(block); + const desired = grants.flatMap((g) => + desiredTuples( + { key: "adopted", domainType: "group_type_role", domainId: 9, grants: [g] }, + emptyState(), + ), + ); + + // ...and the SAME config is a clean no-op on both hosts. + for (const [host, rows] of [ + ["prod", prodRows], + ["dev", devRows], + ] as const) { + const diff = diffGrants(desired, normalizeActual(rows), undefined, normalizeEffective(rows)); + expect(diff.toPut, `${host} toPut`).toEqual([]); + expect(diff.toDelete, `${host} toDelete`).toEqual([]); + } }); it("unmanaged group-dimension scope still gets the 'ct adopt group' hint (unchanged behavior)", () => { @@ -559,3 +618,52 @@ describe("emitAdoptedGrants", () => { } }); }); + +describe('the `-1` "alle" sentinel is not a host-specific id (#115)', () => { + // `churchdb:view alldata` (authId 102) scopes by `cdb_bereich`; `churchdb:view station` (124) by + // `cdb_station`. A grant scoped to "alle" comes back as `dataId: -1`. + const sentinelRows: RawPermission[] = [ + { authId: 102, dataId: -1, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + { authId: 124, dataId: -1, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + ]; + + const block = () => + emitAdoptedGrants({ domainType: "group_role", domainId: 42, rows: sentinelRows, state: emptyState() }); + + it("never tells you to adopt it — `ct adopt department -1` cannot work", () => { + expect(block()).not.toMatch(/ct adopt \S+ -1/); + }); + + it("does not call it host-specific, because it is not", () => { + expect(block()).not.toMatch(/-1 is not managed/); + expect(block()).not.toMatch(/host-specific number/); + }); + + it("says what it actually is, once per grant", () => { + const lines = block() + .split("\n") + .filter((l) => l.includes('"alle" sentinel')); + expect(lines).toHaveLength(2); + expect(lines[0]).toContain("host-independent"); + }); + + it("still emits the scope value itself, so the declaration keeps its meaning", () => { + expect(block()).toContain("scope: [-1]"); + }); + + it("still flags a REAL unmanaged id in the same grant", () => { + // Mixed scope: -1 is fine, 77 is a genuine host-specific id that does need adopting. + const mixed: RawPermission[] = [ + { authId: 1104, dataId: -1, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + { authId: 1104, dataId: 77, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + ]; + const out = emitAdoptedGrants({ + domainType: "group_role", + domainId: 42, + rows: mixed, + state: emptyState(), + }); + expect(out).toContain("ct adopt group 77"); + expect(out).not.toContain("ct adopt group -1"); + }); +}); diff --git a/tests/query-refs.test.ts b/tests/query-refs.test.ts index 014fdd8..2b965a9 100644 --- a/tests/query-refs.test.ts +++ b/tests/query-refs.test.ts @@ -370,3 +370,178 @@ describe("portablizeRuleset (#76 Stage 2)", () => { }); }); }); + +describe("person.id is reported by the portability audit (#127)", () => { + // Four of five auto-group rulesets captured in one week included or excluded specific people by + // id. Those are the SOURCE host's person ids written verbatim into the other host's ruleset, where + // they name entirely different people — the same failure mode as a raw `ctgroup.id`, and the one + // the warning did not mention. The absence of a warning implied there was nothing to find. + + it("reports an `oneof` include-list of person ids", () => { + const w = scanUnportablized({ oneof: [{ var: "person.id" }, [5703, 4389]] }); + expect(w.map((x) => x.id).sort((a, b) => a - b)).toEqual([4389, 5703]); + expect(w.every((x) => x.var === "person.id")).toBe(true); + }); + + it("reports a NEGATED exclusion clause too — the dangerous one", () => { + // `person.id` 1 exists on every ChurchTools instance and is almost always an administrator, so + // an exclusion aimed at one person here lands on someone real there. + const w = scanUnportablized({ "!": [{ oneof: [{ var: "person.id" }, [12, 1]] }] }); + expect(w.map((x) => x.id).sort((a, b) => a - b)).toEqual([1, 12]); + }); + + it("reports an equality comparison against a single person id", () => { + expect(scanUnportablized({ "==": [{ var: "person.id" }, 5703] })).toHaveLength(1); + }); + + it("words it as unfixable — a decision, not a command that will fail", () => { + const lines = formatPortablizeWarnings(scanUnportablized({ oneof: [{ var: "person.id" }, [5703]] })); + const text = lines.join("\n"); + expect(text).toContain("person.id"); + expect(text).toContain("NEVER portable"); + expect(text).toContain("DIFFERENT people on another host"); + // There is no `ct adopt person` and there never will be — people are permanently out of scope. + expect(text).not.toMatch(/ct adopt person/); + }); + + it("does not fire on a non-id person literal", () => { + // Reporting `person.age > 18` would bury the real findings in noise. + expect(scanUnportablized({ ">": [{ var: "person.age" }, 18] })).toEqual([]); + }); + + it("reports person ids alongside the ctgroup ids it already found", () => { + const w = scanUnportablized({ + and: [{ oneof: [{ var: "ctgroup.id" }, [3090]] }, { "!": [{ oneof: [{ var: "person.id" }, [1]] }] }], + }); + expect(new Set(w.map((x) => x.var))).toEqual(new Set(["ctgroup.id", "person.id"])); + }); +}); + +describe("ruleset role refs fall back to a managed role-def only when the name pair is ambiguous (#125)", () => { + // `role.id` portablizes to a (group type, role NAME) pair, resolved by filtering `/group/roles` on + // (groupTypeId, slug(name)). Two rows sharing a name on ONE group type make that a hard error — and + // neither remedy the error offered works for a config shared across hosts: "rename" edits + // ChurchTools master data (here, CT's own stock `leader`), and "pass a numeric id" cannot work + // because the roles have different ids per host with no env to branch on. A managed `role-def` key + // is the way out of THAT case. + // + // It is only a way out of that case, though. Off this host a `role-def` ref is the WEAKER of the + // two: the resolver falls back to a `/group/roles` lookup keyed on slug(name) alone, so on a host + // where the role was never adopted under the shared key it can resolve silently to a role on a + // different group type. The pair cannot fail that way. So the pair stays the default and `role-def` + // is reserved for the ids the pair genuinely cannot name. + const groupTypeIdToKey = new Map([[30, "community"]]); + // Two rows, same group type, same name — the collision #125 is actually about. + const ambiguousCatalog = new Map([ + [207, { groupTypeId: 30, name: "leader" }], + [208, { groupTypeId: 30, name: "Leader" }], + ]); + // One row — the common case: unique across all 46 prod roles. + const uniqueCatalog = new Map([[207, { groupTypeId: 30, name: "leader" }]]); + const ruleset = { oneof: [{ var: "role.id" }, [207]] }; + + it("emits a role-def ref when the name pair is ambiguous and the role is under management", () => { + const { ruleset: out, warnings } = portablizeRuleset(ruleset, { + idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, + roleCatalog: ambiguousCatalog, + groupTypeIdToKey, + }); + expect(warnings).toEqual([]); + expect(out).toEqual({ + oneof: [{ var: "role.id" }, [{ __ctRef: true, kind: "role-def", key: "community_leader" }]], + }); + }); + + it("keeps the (group-type, role) pair when it is unambiguous, EVEN IF the role is managed", () => { + // The safety property: a `role-def` ref can mis-resolve to another group type's role on a host + // that never adopted the key, and the pair cannot. Nothing is gained by trading down here, so a + // managed role-def must not win by default. + const { ruleset: out, warnings } = portablizeRuleset(ruleset, { + idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, + roleCatalog: uniqueCatalog, + groupTypeIdToKey, + }); + expect(warnings).toEqual([]); + expect(out).toEqual({ + oneof: [ + { var: "role.id" }, + [{ __ctRef: true, kind: "group-type-role", groupType: "community", role: "leader" }], + ], + }); + }); + + it("still emits the (group-type, role) pair when the role is NOT managed", () => { + const { ruleset: out } = portablizeRuleset(ruleset, { + idToKeyByKind: {}, + roleCatalog: uniqueCatalog, + groupTypeIdToKey, + }); + expect(out).toEqual({ + oneof: [ + { var: "role.id" }, + [{ __ctRef: true, kind: "group-type-role", groupType: "community", role: "leader" }], + ], + }); + }); + + it("leaves an ambiguous, unmanaged role to the pair — which hard-errors at plan time, honestly", () => { + // Nothing portable to emit: the pair cannot name it and there is no shared key yet. Emitting the + // pair anyway is right — the resolver's ambiguity error is what tells the author to adopt it. + const { ruleset: out } = portablizeRuleset(ruleset, { + idToKeyByKind: {}, + roleCatalog: ambiguousCatalog, + groupTypeIdToKey, + }); + expect(out).toEqual({ + oneof: [ + { var: "role.id" }, + [{ __ctRef: true, kind: "group-type-role", groupType: "community", role: "leader" }], + ], + }); + }); + + it("adopting the role under a shared key is what makes the ambiguous case resolvable", () => { + // Two hosts, same logical key, different numeric ids — the whole point. Each host's capture + // produces the SAME portable marker. + const prod = portablizeRuleset( + { oneof: [{ var: "role.id" }, [127]] }, + { + idToKeyByKind: { "role-def": new Map([[127, "community_leader"]]) }, + roleCatalog: new Map([ + [127, { groupTypeId: 30, name: "leader" }], + [128, { groupTypeId: 30, name: "Leader" }], + ]), + groupTypeIdToKey, + }, + ).ruleset; + const dev = portablizeRuleset(ruleset, { + idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, + roleCatalog: ambiguousCatalog, + groupTypeIdToKey, + }).ruleset; + expect(prod).toEqual(dev); + expect(prod).toEqual({ + oneof: [{ var: "role.id" }, [{ __ctRef: true, kind: "role-def", key: "community_leader" }]], + }); + }); + + it("also portablizes the out-of-query handleMembership.groupTypeRoleId field", () => { + const { ruleset: out } = portablizeRuleset( + { process: { x: { handleMembership: { groupTypeRoleId: 207 } } } }, + { + idToKeyByKind: { "role-def": new Map([[207, "community_leader"]]) }, + roleCatalog: ambiguousCatalog, + groupTypeIdToKey, + }, + ); + expect(out).toEqual({ + process: { + x: { + handleMembership: { + groupTypeRoleId: { __ctRef: true, kind: "role-def", key: "community_leader" }, + }, + }, + }, + }); + }); +}); diff --git a/tests/registry.test.ts b/tests/registry.test.ts index 8a07741..6faa75c 100644 --- a/tests/registry.test.ts +++ b/tests/registry.test.ts @@ -32,8 +32,17 @@ describe("resourceType", () => { expect(() => resourceType("nope")).toThrow(/Adoptable types/); }); - it("derives a key from campus shorty", () => { - expect(RESOURCES.campus?.deriveKey({ name: "Mainz", shorty: "MZ" })).toBe("mz"); + it("derives a campus key from its NAME, not its shorty (#118)", () => { + // `shorty` is a display abbreviation an admin edits to make a UI column fit — often historical + // (Idstein is abbreviated "SWA" after the region it used to be named for) and sometimes actively + // misleading (Dietzenbach → "OF", which reads as a typo in a config file). + expect(RESOURCES.campus?.deriveKey({ name: "Mainz", shorty: "MZ" })).toBe("mainz"); + expect(RESOURCES.campus?.deriveKey({ name: "Dietzenbach", shorty: "OF" })).toBe("dietzenbach"); + expect(RESOURCES.campus?.deriveKey({ name: "Würzburg", shorty: "WÜ" })).toBe("wurzburg"); + }); + + it("falls back to shorty only when a campus has no name", () => { + expect(RESOURCES.campus?.deriveKey({ name: "", shorty: "MZ" })).toBe("mz"); }); it("snapshots group ids whether they are nested under information or top-level", () => { diff --git a/tests/state-rm-command.test.ts b/tests/state-rm-command.test.ts new file mode 100644 index 0000000..72e6f79 --- /dev/null +++ b/tests/state-rm-command.test.ts @@ -0,0 +1,197 @@ +/** + * `ct state rm` — the missing inverse of `ct adopt` (#122). + * + * `ct adopt` wrote a state entry and nothing removed one. `ct destroy` is the opposite of what + * un-adopting means (it deletes the resource IN ChurchTools), so backing out an adoption meant + * hand-editing `ct-state..json` — the very file the tool insists is its own. Adopt-then-declare + * is the documented loop, so a wrong adoption is normal, not exotic: leaving the entry in makes + * `plan` report a DESTROY on a host where nothing is wrong, and the offline config-matches-state + * check cannot be satisfied without editing the file the check is checking. + * + * The load-bearing property is the one asserted first: NO ChurchTools call, ever. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { emptyState, loadState, saveState } from "../src/state/state.js"; + +const authedSession = vi.fn(async () => { + throw new Error("ct state rm must never contact ChurchTools"); +}); +vi.mock("../src/api/session.js", () => ({ authedSession })); + +const { stateCommand } = await import("../src/commands/state.js"); + +const HOST = "https://mychurch.church.tools"; +let dir: string; +let statePath: string; +let configPath: string; +const originalHost = process.env.CT_HOST; +const originalConfig = process.env.CT_CONFIG; + +async function run(args: string[]): Promise { + await stateCommand().parseAsync(["rm", ...args], { from: "user" }); +} + +/** A state file holding two adopted role definitions and one campus. */ +async function seedState(): Promise { + const state = emptyState(HOST); + for (const [key, id] of [ + ["appmodule_write", 51], + ["appmodule_read", 52], + ] as const) { + state.resources[key] = { + type: "group-role", + id, + key, + fields: { name: key }, + adoptedAt: "t", + updatedAt: "t", + }; + } + state.resources.mainz = { + type: "campus", + id: 0, + key: "mainz", + fields: { name: "Mainz" }, + adoptedAt: "t", + updatedAt: "t", + }; + state.resources.youth = { + type: "group", + id: 3090, + key: "youth", + fields: { name: "Youth" }, + adoptedAt: "t", + updatedAt: "t", + }; + await saveState(statePath, state); +} + +beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), "ct-state-rm-")); + statePath = join(dir, "ct-state.json"); + configPath = join(dir, "ct.config.ts"); + process.env.CT_HOST = HOST; + process.env.CT_CONFIG = configPath; + // A config that declares NOTHING — the normal state after deleting a wrong declaration. + await writeFile(configPath, "export default () => {};"); + await seedState(); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(async () => { + vi.restoreAllMocks(); + if (originalHost === undefined) delete process.env.CT_HOST; + else process.env.CT_HOST = originalHost; + if (originalConfig === undefined) delete process.env.CT_CONFIG; + else process.env.CT_CONFIG = originalConfig; + await rm(dir, { recursive: true, force: true }); +}); + +describe("ct state rm (#122)", () => { + it("removes the entry and contacts nothing", async () => { + await run(["group-role", "appmodule_write", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeUndefined(); + expect(authedSession).not.toHaveBeenCalled(); + }); + + it("leaves every other entry untouched", async () => { + await run(["group-role", "appmodule_write", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(Object.keys(state.resources).sort()).toEqual(["appmodule_read", "mainz", "youth"]); + expect(state.resources.mainz?.id).toBe(0); + }); + + // A key can be named by a PERMISSION declaration without being declared as a resource. A + // resources-only guard waves those through, and the breakage surfaces one command later as a + // `ct plan` hard error ("does not resolve to a managed group") — after the state file was written. + it("refuses a key a permission DOMAIN still names, not just a declared resource", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.groupRole({ key: "youth_leiter", group: "youth", role: "Leiter", grants: ["churchcore:administer settings"] }); };`, + ); + await expect(run(["group", "youth", "--state", statePath])).rejects.toThrow( + /still declared in the config/, + ); + const state = await loadState(statePath, HOST); + expect(state.resources.youth).toBeDefined(); + }); + + it("refuses a key a permission SCOPE still names", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.groupRole({ key: "p", id: 77, grants: [{ right: "churchgroup:view group", scope: ["youth"] }] }); };`, + ); + await expect(run(["group", "youth", "--state", statePath])).rejects.toThrow( + /still declared in the config/, + ); + const state = await loadState(statePath, HOST); + expect(state.resources.youth).toBeDefined(); + }); + + it("still removes a key no declaration — resource or permission — mentions", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.groupRole({ key: "p", id: 77, grants: [{ right: "churchgroup:view group", scope: ["someone_else"] }] }); };`, + ); + await run(["group", "youth", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(state.resources.youth).toBeUndefined(); + }); + + it("refuses a key the config still declares — that would plan a CREATE for a live resource", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.roleDefinition({ key: "appmodule_write", name: "Write", groupTypeId: 2 }); };`, + ); + await expect(run(["group-role", "appmodule_write", "--state", statePath])).rejects.toThrow( + /still declared in the config/, + ); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeDefined(); + }); + + it("--force removes it anyway, for deleting both in one change", async () => { + await writeFile( + configPath, + `export default (ct) => { ct.roleDefinition({ key: "appmodule_write", name: "Write", groupTypeId: 2 }); };`, + ); + await run(["group-role", "appmodule_write", "--state", statePath, "--force"]); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeUndefined(); + }); + + it("--dry-run writes nothing", async () => { + await run(["group-role", "appmodule_write", "--state", statePath, "--dry-run"]); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeDefined(); + }); + + it("rejects a key that is not in state, pointing at `ct state list`", async () => { + await expect(run(["group-role", "nope", "--state", statePath])).rejects.toThrow(/ct state list/); + }); + + it("rejects a type/key mismatch instead of removing the wrong entry", async () => { + await expect(run(["campus", "appmodule_write", "--state", statePath])).rejects.toThrow( + /is a group-role \(#51\), not a campus/, + ); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeDefined(); + }); + + it("rejects an unknown resource type", async () => { + await expect(run(["widget", "appmodule_write", "--state", statePath])).rejects.toThrow(/Adoptable types/); + }); + + it("still works when the config cannot be read — that is when you need it most", async () => { + // Backing out an adoption is exactly what you do while the config is mid-edit. + await writeFile(configPath, "export default (ct) => { throw new Error('broken'); };"); + await run(["group-role", "appmodule_write", "--state", statePath]); + const state = await loadState(statePath, HOST); + expect(state.resources.appmodule_write).toBeUndefined(); + }); +});