From 51dc17451788de079ceca3e9580d82247568e738 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Tue, 25 Aug 2026 19:29:18 +0200 Subject: [PATCH 1/3] feat(resources): promote comment-viewer to a declarable resource (#151) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cdb_comment_viewer` was the last scope dimension a config could not express portably. It had a reference form since #102 but nothing to declare the viewers themselves, so a config granting `churchdb:view comments` had to write a raw, host-specific `dataId` — and that id silently means something different, or nothing, on another host. Nothing detects it: a plan compares the declared id against the live id and they match; the id is just meaningless on the target. Switching to a name ref made it worse rather than better, because the second host lacks the NAMES too, so the ref hard-errors instead of resolving wrongly. Declaring the viewer is what makes the name exist on both hosts, which is what makes the reference portable. - registry: `comment-viewer` at `/person/commentviewers`, tier 0, PUT updates, managing `name` + `sortKey` (the whole editable surface of this 3-column master-data table). No new machinery: CT mints the id (unlike `security-level`) and the writes are REST (unlike `department`). Reads of a single viewer filter the collection — a GET on the item path was never probed, and guessing it has the #108 failure mode (a 404 reads as "vanished", so every plan re-creates it). - DSL: `ct.commentViewer({ key, name, sortKey })`; `ct adopt comment-viewer ` falls out of the registry. - resolution: `{ commentViewer: "…" }` now resolves managed-state-first, live catalog second, so a viewer declared in the same run resolves pending and is re-resolved at apply time — and a ref to a viewer the config does not own keeps working exactly as before. - grant adoption: an unmanaged viewer dataId now earns the `ct adopt comment-viewer ` hint instead of the weaker "portable form" note, and a managed one is emitted as `{ commentViewer: "" }`. Docs: permissions.md's catalog-only section is replaced by the declarable one (no scope dimension with a logical form is catalog-only any more), plus rows in api-coverage.md and README. Claude-Session: https://claude.ai/code/session_011DU9pubAh7dGWyojwd1smn --- README.md | 7 +- docs/api-coverage.md | 11 +- docs/handbuch/permissions.md | 100 ++++++---- src/config/context.ts | 18 ++ src/permissions/adopt.ts | 17 +- src/permissions/scope.ts | 27 +-- src/resolve/refs.ts | 9 +- src/resolve/resolver.ts | 5 + src/resources/registry.ts | 56 ++++++ tests/comment-viewer-resource.test.ts | 277 ++++++++++++++++++++++++++ tests/permission-adopt.test.ts | 34 +++- 11 files changed, 488 insertions(+), 73 deletions(-) create mode 100644 tests/comment-viewer-resource.test.ts diff --git a/README.md b/README.md index 5a8e511..62833af 100644 --- a/README.md +++ b/README.md @@ -256,10 +256,11 @@ confirmation — and a declaration marked `preventDestroy: true` blocks even tha | Auto-groups (dynamic groups) | the `dynamic` block on a group | [dynamic groups](docs/handbuch/dynamic-groups.md) | | Group member field definitions (group-scoped, never deleted implicitly) | the `memberFields` block on a group | [group member fields](docs/handbuch/group-member-fields.md) | | Repeated structure, parametrized | a plain function over the DSL | [blueprints](docs/handbuch/blueprints.md) | +| Permission scope master data (Bereiche, security levels, comment viewers) | `ct.department`, `ct.securityLevel`, `ct.commentViewer` | [permissions](docs/handbuch/permissions.md) | -Read-only by design: the person master-data model, security levels and -custom-field _definitions_ (`ct get person-masterdata`, `ct get data-fields`) — -schema in scope, per-record values never. See +Read-only by design: the person master-data model and custom-field +_definitions_ (`ct get person-masterdata`, `ct get data-fields`) — schema in +scope, per-record values never. See [field definitions](docs/handbuch/field-definitions.md). References are logical throughout: `groupType: "ministry_team"` resolves to that diff --git a/docs/api-coverage.md b/docs/api-coverage.md index bdbde1d..dc496dd 100644 --- a/docs/api-coverage.md +++ b/docs/api-coverage.md @@ -88,11 +88,12 @@ API-client libraries (5pm-HDH `churchtools-api` @ CT 3.104, bensteUEM `src/api/schema.d.ts` is git-ignored and was not available offline. Re-verify per the runbook's re-audit procedure once the schema is regenerated. -| Resource | Matched path(s) | GET | POST/PUT/PATCH/DELETE | Verdict | -| ------------------------------------ | ----------------------------------------- | ------------------ | ---------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Person master-data model | `/person/masterdata` | ✅ (single object) | ❌ | **Read-only.** Versionable master-data model incl. the `securityLevels` enumeration. No write endpoint (edited in the CT master-data admin UI). | -| Data-field definitions (Datenfelder) | `/dbfields`, `/dbfields/{id}` | list ✅ / by-id ✅ | ❌ | **Read-only.** Unified person + group field definitions, discriminated by `fieldCategory`. Mutation only via legacy churchdb AJAX (`db_insert/update/deletefields`), not REST. | -| Security levels | `/securitylevels`, `/securitylevels/{id}` | list ✅ / by-id ✅ | ✅ POST, PATCH, DELETE | **Full CRUD — but not on the usual paths.** Live-probed 2026-08-14 (eqrm-dev, CT 3.135.2): `/securitylevels` is GET-only, and **create is `POST /securitylevels/{id}`** (body `{name}`), update is `PATCH /securitylevels/{id}` (body `{name, newid, forcereorder}` — reordering is first-class), delete is `DELETE /securitylevels/{id}`. **Managed since #110** — the registry gained a `createPath` hook and a `callerAssignedId` flag for it; `ct.securityLevel({ key, id, name })` declares one, `ct adopt security-level ` adopts one. Changing a declared id is refused at plan time (that is a renumber, which rewrites what every numeric `cc_securitylevel` scope means). Resolvable by name as a scope ref since #110. The ids are NOT protocol constants — `cc_securitylevel` is an editable master-data table with an auto-increment id. | +| Resource | Matched path(s) | GET | POST/PUT/PATCH/DELETE | Verdict | +| ------------------------------------ | ------------------------------------------------------- | ------------------ | ---------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Person master-data model | `/person/masterdata` | ✅ (single object) | ❌ | **Read-only.** Versionable master-data model incl. the `securityLevels` enumeration. No write endpoint (edited in the CT master-data admin UI). | +| Data-field definitions (Datenfelder) | `/dbfields`, `/dbfields/{id}` | list ✅ / by-id ✅ | ❌ | **Read-only.** Unified person + group field definitions, discriminated by `fieldCategory`. Mutation only via legacy churchdb AJAX (`db_insert/update/deletefields`), not REST. | +| Security levels | `/securitylevels`, `/securitylevels/{id}` | list ✅ / by-id ✅ | ✅ POST, PATCH, DELETE | **Full CRUD — but not on the usual paths.** Live-probed 2026-08-14 (eqrm-dev, CT 3.135.2): `/securitylevels` is GET-only, and **create is `POST /securitylevels/{id}`** (body `{name}`), update is `PATCH /securitylevels/{id}` (body `{name, newid, forcereorder}` — reordering is first-class), delete is `DELETE /securitylevels/{id}`. **Managed since #110** — the registry gained a `createPath` hook and a `callerAssignedId` flag for it; `ct.securityLevel({ key, id, name })` declares one, `ct adopt security-level ` adopts one. Changing a declared id is refused at plan time (that is a renumber, which rewrites what every numeric `cc_securitylevel` scope means). Resolvable by name as a scope ref since #110. The ids are NOT protocol constants — `cc_securitylevel` is an editable master-data table with an auto-increment id. | +| Comment viewers (Kommentare-Viewer) | `/person/commentviewers`, `/person/commentviewers/{id}` | list ✅ / by-id ❓ | ✅ POST, PUT, DELETE | **Full CRUD on conventional paths.** Live-probed 2026-08-14 (CT 3.135.2): `GET /person/commentviewers` returns a flat `[{id, name, sortKey}]` (`id: 0` — "Alle" — is a real row), and the item path takes POST/PUT/DELETE. **Managed since #151** — `ct.commentViewer({ key, name, sortKey })` declares one, `ct adopt comment-viewer ` adopts one; no registry machinery was needed (CT mints the id, and the writes are REST). `cdb_comment_viewer` is the scope dimension of `churchdb:view comments`, and it was the last one a config could not express portably: the ids differ across hosts of the same deployment, and before #151 a config had only the raw numeric `dataId`. A GET on the item path was never probed, so reads of a single viewer filter the collection instead of guessing it. | See [`docs/handbuch/field-definitions.md`](handbuch/field-definitions.md) for the full writability decision, evidence, and the schema/values boundary. diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index 9bb49d2..f533682 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -463,14 +463,14 @@ ct.groupRole({ `{ campus: "koblenz" }` is sugar for `ref.campus("koblenz")` — the same `Ref` the rest of the DSL uses — so both spellings are interchangeable. -| `scopeField` | Reference form | Resolved against | -| -------------------- | ----------------------------------------- | ---------------------------------------------------------- | -| `cdb_gruppe` | `{ group: "" }` (or the bare string) | managed groups | -| `cdb_station` | `{ campus: "" }` | managed campuses, then `GET /campuses` | -| `cdb_gruppentyp` | `{ groupType: "" }` | managed group types, then `GET /group/grouptypes` | -| `cdb_bereich` | `{ department: "" }` | managed Bereiche, then `GET /departments` (#108) | -| `cc_securitylevel` | `{ securityLevel: "" }` | managed security levels, then `GET /securitylevels` (#110) | -| `cdb_comment_viewer` | `{ commentViewer: "" }` | `GET /person/commentviewers` **only** (#102) | +| `scopeField` | Reference form | Resolved against | +| -------------------- | ----------------------------------------- | ----------------------------------------------------------------- | +| `cdb_gruppe` | `{ group: "" }` (or the bare string) | managed groups | +| `cdb_station` | `{ campus: "" }` | managed campuses, then `GET /campuses` | +| `cdb_gruppentyp` | `{ groupType: "" }` | managed group types, then `GET /group/grouptypes` | +| `cdb_bereich` | `{ department: "" }` | managed Bereiche, then `GET /departments` (#108) | +| `cc_securitylevel` | `{ securityLevel: "" }` | managed security levels, then `GET /securitylevels` (#110) | +| `cdb_comment_viewer` | `{ commentViewer: "" }` | managed comment viewers, then `GET /person/commentviewers` (#151) | Two `Ref` kinds are deliberately **not** in that table because no permission dimension scopes by them: `group-type-role` (a `groupTypeRoleId`, addressed by @@ -508,23 +508,46 @@ Three things are hard errors at **plan** time, never a guessed `dataId`: group", so on e.g. a `cdb_station` right it would either fail confusingly or — worse — match an unrelated group that happens to carry that key. -### Catalog dimensions: referenceable, not declarable +### Comment viewers: the last catalog-only dimension (#151) -`cdb_comment_viewer` (Kommentare-Viewer) is the one remaining dimension `ct` -**reads but does not manage**, and not because ChurchTools cannot do it: +`cdb_comment_viewer` (Kommentare-Viewer) had a reference form since #102 but no +declaration behind it, and that combination turned out to be the worst of both +worlds. Every other option was worse still: -- **Comment viewers have plain REST CRUD** — `/person/commentviewers` plus - `POST`/`PUT`/`DELETE` on the item path, which would fit the resource registry - unchanged. They are catalog-only purely because nothing has needed to declare - one yet ([#102](https://github.com/eqrm/ct-cli/issues/102)). +- a **raw `dataId`** is host-specific. Read on two hosts of the same deployment + on 2026-08-24, three of the six viewer ids on one host did not exist on the + other at all, and the two ids present on both named _different_ categories on + each. An apply duly wrote a grant pointing at nothing, and nothing caught it — + `ct plan` compares the declared id against the live id, and they match; the id + is simply meaningless on the target host. +- a **name reference** did not rescue it, because the second host was missing the + NAMES as well. The reference hard-errored instead of resolving wrongly — safer, + but still not a config that applies to both hosts. +- **creating the viewers by hand** on each host is unmanaged master data, which + is the thing this tool exists not to need. -So the reference resolves by name on every host — which is what makes -`churchdb:view comments` declarable at all — but there is no `ct.commentViewer` -resource, no `ct adopt` for it, and a name that matches nothing is a **hard -error** rather than a create: +So comment viewers are a managed resource now: -(Security levels left this list in #110 and Bereiche in #108. Both are managed -resources now — see below.) +```ts +ct.commentViewer({ key: "dienstbereich", name: "Dienstbereich", sortKey: 40 }); +... +{ right: "churchdb:view comments", scope: [{ commentViewer: "dienstbereich" }] } +``` + +Declaring the viewer is what makes the NAME exist on both hosts, which is what +makes the reference portable — a name reference is only worth anything once +something guarantees the row is there. + +- **`ct adopt comment-viewer `** puts an existing viewer under management and + emits the declaration; `ct get comment-viewers` lists the names. +- **`/person/commentviewers` is conventional REST** (`[{id, name, sortKey}]` plus + POST/PUT/DELETE), so this type needs none of the special machinery the other + two awkward master-data types did: ChurchTools mints the id (unlike a security + level) and the writes are REST (unlike a Bereich). +- **Resolution is managed-state first, live catalog second.** An existing + reference to a viewer your config does not own keeps resolving exactly as it + did in #102, and a name that matches nothing anywhere is still a hard error + rather than a create: ``` Cannot resolve comment-viewer:nope referenced at … : no managed resource and @@ -532,11 +555,14 @@ no live comment-viewer at /person/commentviewers matches key "nope". Declare/adopt it, fix the key/name, or use a numeric id. ``` -Discover the names with `ct get comment-viewers`. Because -the id always comes from the live catalog, such a scope is never "pending" and is -never re-resolved at apply time — it is host-correct the moment it resolves. A -managed resource that happens to share the key does **not** shadow it (that would -be exactly the misgrant this feature exists to prevent). +- **`ct apply` never deletes**, as everywhere else. `ct destroy` can, and warns: + deleting a viewer reaches every person comment restricted to it and every grant + scoped to it. + +With this, every scope dimension `ct` can name is also one it can declare — +Bereiche left the catalog-only list in #108, security levels in #110, and comment +viewers here. What remains numeric-only (`ccm_data_category`, `oauth_client`, +`cc_calcategory`, …) belongs to modules outside this tool's mandate. ### Bereiche: managed, but written through a non-REST endpoint (#108) @@ -631,11 +657,11 @@ For these, a `dataId` like `1`, `2`, `3` names a security level or a comment-viewer bucket — **not** a group — so `GET /groups/{1,2,3}` 404s. A `scope` array entry may therefore be a plain number instead of a string: -(Security levels gained a name-based form in #110 — see -[Catalog dimensions](#catalog-dimensions-referenceable-not-declarable). -Comment viewers gained one too, in #102. Numerics keep working for both; -calendar categories, OAuth clients and the other module dimensions still have -nothing but numerics.) +(Both of those dimensions gained a name-based form and are declarable resources +now — comment viewers in [#151](#comment-viewers-the-last-catalog-only-dimension-151), +security levels in [#110](#security-levels-a-managed-resource-with-a-declared-id-110). +Numerics keep working for both; calendar categories, OAuth clients and the other +module dimensions still have nothing but numerics.) ```ts { right: "churchdb:security level view own data", scope: [1, 2, 3, 5] }, @@ -739,11 +765,13 @@ guaranteed to be accepted by `ct plan` (the round trip is locked by tests): clearly-marked placeholder comment telling you to `ct adopt group ` first — scope keys must be state keys (see [Scope resolution](#scope-resolution)). - - **A catalog dimension `ct` reads but does not manage** (`cdb_bereich`, - `cc_securitylevel`): the emitter is pure — no client, no fetch — so it cannot - turn the id into a name. It emits the number plus a `NOTE` giving the portable - form (`{ department: "" }` / `{ securityLevel: "" }`) to write by - hand. + - **Another dimension with a logical form** (`cdb_station`, `cdb_gruppentyp`, + `cdb_bereich`, `cc_securitylevel`, `cdb_comment_viewer`): a `dataId` that + matches a resource **managed in your state file** is emitted as the portable + reference (`scope: [{ commentViewer: "dienstbereich" }]`). An unmanaged one + keeps its number and earns a `NOTE` naming the one command that fixes it — + `ct adopt comment-viewer `, then re-adopt the grants. The emitter is pure + (no client, no fetch), so it cannot turn an unmanaged id into a name itself. - **Any other scope dimension** (`cc_calcategory`, `oauth_client`, …): there is no group to adopt, so the `dataId`(s) are emitted directly as a numeric `scope: [1, 2, 3]` — always an active line, with a comment diff --git a/src/config/context.ts b/src/config/context.ts index 76db652..c5593c0 100644 --- a/src/config/context.ts +++ b/src/config/context.ts @@ -322,6 +322,23 @@ export interface ConfigContext { * `destroyWarning`: a delete reaches every person field and grant referencing the level. */ securityLevel(input: ResourceInput): void; + /** + * A COMMENT VIEWER (`/person/commentviewers` — "Kommentare-Viewer"), #151 — the `cdb_comment_viewer` + * scope dimension of `churchdb:view comments`. + * + * ```ts + * ct.commentViewer({ key: "dienstbereich", name: "Dienstbereich", sortKey: 40 }); + * ... + * { right: "churchdb:view comments", scope: [{ commentViewer: "dienstbereich" }] } + * ``` + * + * The last scope dimension a config could not express portably: before this, a `view comments` + * grant had to name a raw numeric `dataId`, which means a different viewer — or nothing — on + * another host, and neither `ct plan` nor a row count can see that. Declaring the viewer makes the + * NAME exist on every host, which is what makes `{ commentViewer: "…" }` resolve to the same thing + * everywhere. Master data (the enumeration), never a person or a comment. + */ + commentViewer(input: ResourceInput): void; /** * A BEREICH / department (`/departments`), #108 — the `cdb_bereich` scope dimension of * `churchdb:view alldata` ("Personen eines Bereiches sehen"). @@ -772,6 +789,7 @@ export function createContext(): { relationshipType: define("relationship-type"), personStatus: define("person-status"), securityLevel: define("security-level"), + commentViewer: define("comment-viewer"), department: define("department"), roleDefinition: define("group-role"), groupRole: definePermission("group_role"), diff --git a/src/permissions/adopt.ts b/src/permissions/adopt.ts index 8fa8079..38e44db 100644 --- a/src/permissions/adopt.ts +++ b/src/permissions/adopt.ts @@ -288,7 +288,7 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St // — the whole point of #98, since these ids are host-specific (dev Mainz = 6, prod Mainz = 0), // so a numeric literal adopted from prod is a misgrant when replayed on dev. An unmanaged // dataId keeps its number and earns a NOTE pointing at the one command that fixes it. - // b) The dimension has none (`cc_securitylevel`, `cdb_bereich`, `oauth_client`, …) — its dataIds + // b) The dimension has none (`ccm_data_category`, `oauth_client`, …) — its dataIds // name something this tool has no managed representation for, so the numeric escape hatch // (#49) is the only honest output and the grant is always emitted as an ACTIVE line. const out: string[] = []; @@ -305,9 +305,11 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St } if (g.dataIds.length > 0) { const dimension = SCOPE_REF_KIND[entry.scopeField]; - // A catalog-only dimension (departments) has no managed resource to look an id up in, and this - // 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. + // A catalog-only dimension (`managed: false`) has no managed resource to look an id up in, and + // this 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. No + // dimension is catalog-only today (comment viewers, the last one, became managed in #151); + // the branch stays because the flag is what a future read-only dimension would set. const sugar = dimension?.managed ? SCOPE_SUGAR_FIELD[dimension.type] : undefined; // #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 @@ -361,9 +363,10 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St `and re-adopt to make ${one ? "it" : "them all"} portable.`, ); } else if (!dimension) { - // Reached only for a dimension with NO logical form at all (`cc_securitylevel`, `oauth_client`, - // …). A catalog-only dimension (`cdb_bereich`) already got its own NOTE above — emitting this - // line there too would contradict it ("not a group, use numbers" vs "portable form exists"). + // Reached only for a dimension with NO logical form at all (`ccm_data_category`, + // `oauth_client`, …). A catalog-only dimension would already have got its own NOTE above — + // emitting this line there too would contradict it ("not a group, use numbers" vs "portable + // form exists"). out.push( ` // "${entry.name}" scopes by "${entry.scopeField}", not a group — using its numeric dataId(s) directly.`, ); diff --git a/src/permissions/scope.ts b/src/permissions/scope.ts index e5142ef..211eb04 100644 --- a/src/permissions/scope.ts +++ b/src/permissions/scope.ts @@ -59,16 +59,16 @@ export const GROUP_SCOPE_FIELD = "cdb_gruppe"; * - `cc_securitylevel` → security levels (`/securitylevels`, #110) — e.g. `churchdb:+see persons` * - `cdb_comment_viewer` → comment viewers (`/person/commentviewers`, #102) — `churchdb:view comments` * - * `managed` separates the MANAGED resource kinds — groups, campuses, group types and (since #110) - * security levels — from the read-only catalogs. For a managed kind a scope target can be declared in - * the same config (resolving pending, then re-resolved at apply time) and a state-backed id is worth - * re-resolving. For a catalog kind the reference always resolves by NAME against the live catalog and - * hard-errors when the name is absent — strictly better than a silent numeric misgrant, but never a - * create. Neither remaining catalog kind is unmanaged because "ChurchTools cannot": - * - `cdb_bereich` has no REST write at all; the admin UI writes it through the legacy master-data - * endpoint (#108/#109). - * - `cdb_comment_viewer` has conventional REST CRUD that WOULD fit the registry unchanged (#102) — - * it is catalog-only purely because nothing has needed to declare one yet. + * `managed` separates the MANAGED resource kinds from the read-only catalogs. For a managed kind a + * scope target can be declared in the same config (resolving pending, then re-resolved at apply time) + * and a state-backed id is worth re-resolving. For a catalog kind the reference always resolves by + * NAME against the live catalog and hard-errors when the name is absent — strictly better than a + * silent numeric misgrant, but never a create. + * + * Every dimension listed here is managed today: departments joined in #108 (writes go through the + * legacy master-data endpoint), security levels in #110, and comment viewers in #151 — the last one + * a config could not express portably at all, because a `churchdb:view comments` grant had no choice + * but a raw host-specific `dataId`. * * The remaining scoped dimensions (`ccm_data_category`, `oauth_client`, `cc_calcategory`, …) stay * numeric-only because this tool has no way to address their values by a host-independent name today — @@ -82,7 +82,7 @@ export const SCOPE_REF_KIND: Readonly ({ __ctRef: true, diff --git a/src/resolve/resolver.ts b/src/resolve/resolver.ts index f2e6c20..66e1bb2 100644 --- a/src/resolve/resolver.ts +++ b/src/resolve/resolver.ts @@ -74,6 +74,11 @@ const REF_KIND_TYPE: Partial> = { // ordering is what lets a config declare a level and scope a grant to it in the same run (the ref // resolves to a PendingRef, carried as a pending scope). "security-level": "security-level", + // Comment viewers became a managed resource in #151 — same ordering as security levels above, and + // for the sharper reason: their ids genuinely differ across hosts of the same deployment, so a + // config that declares its viewers must resolve `{ commentViewer: "…" }` against what IT owns + // before falling back to `/person/commentviewers` for a viewer it does not. + "comment-viewer": "comment-viewer", "role-def": "group-role", group: "group", }; diff --git a/src/resources/registry.ts b/src/resources/registry.ts index 9edaba7..5819b93 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -514,6 +514,62 @@ export const RESOURCES: Record = { deriveKey: (r) => slug(str(r, "name")), managedFields: (r) => ({ id: r.id, name: r.name }), }), + /** + * COMMENT VIEWERS (#151) — `cdb_comment_viewer`, the scope dimension of `churchdb:view comments` + * ("Kommentare-Viewer"). Managed so a `view comments` grant means the same thing on every host. + * + * It was catalog-only until now, and that is exactly what made the dimension unportable: a config + * granting `churchdb:view comments` had to write a raw, host-specific `dataId`, and the same + * number names a different viewer (or nothing) on another instance. Read on two hosts of the same + * deployment 2026-08-24: three of prod's six ids do not exist on dev at all, and the two that do + * exist on both name different categories on each. Nothing detected the + * mismatch — a plan compares the declared id against the live id, and they match; the id is simply + * meaningless on the target host. Switching to a name ref did not fix it either: dev lacks the + * NAMES too, so the ref failed to resolve rather than resolving wrongly. Only a declarable + * resource makes the names exist on both hosts, which is what makes a name ref portable. + * + * `/person/commentviewers` is conventional REST (`[{id, name, sortKey}]` plus POST/PUT/DELETE, + * live-probed on eqrm-dev CT 3.135.2, 2026-08-14), so this needs none of the machinery the other + * two awkward master-data types did: CT mints the id (unlike `security-level`) and writes are REST + * (unlike `department`). + * + * `name` and `sortKey` are the whole editable surface — `cdb_comment_viewer` is one of the 3-column + * tables in #109's list (`id`, `bezeichnung`, `sortkey`) — so the managed set is complete and a PUT + * carrying it cannot blank an unmanaged sibling. + */ + "comment-viewer": define({ + collectionPath: "/person/commentviewers", + updateMethod: "PUT", + // Tier 0: grants scope by comment viewer, and permissions apply after every resource tier. + tier: 0, + destroyWarning: + "deleting a comment viewer reaches every person comment restricted to it and every grant " + + "scoped to it (`churchdb:view comments`) — the viewer id is referenced instance-wide. Verify " + + "it is unused first (`ct get comment-viewers`, `ct report permissions`).", + deriveKey: (r) => slug(str(r, "name")), + managedFields: (r) => ({ name: r.name, sortKey: r.sortKey }), + // `sortKey` is managed but not mandatory in a hand-authored declaration; CT's create validator + // for the 3-column master-data tables rejects a missing integer column, so supply a neutral one + // (a declared value still wins — createDefaults merges UNDER the body). + createDefaults: () => ({ sortKey: 0 }), + /** + * Read one viewer out of the COLLECTION rather than through `GET {itemPath}`. + * + * The live probe behind this type recorded the collection read plus POST/PUT/DELETE on the item + * path; a GET on the item path was never probed. Guessing it is the one mistake with a genuinely + * bad failure mode (#108): a read that 404s reads as "vanished in ChurchTools", so every plan + * would propose creating the same viewer again and every apply would duplicate it. The + * collection read is the endpoint this tool has actually verified, and with ≤ a handful of rows + * per instance it costs nothing. NB `id: 0` ("Alle") is a real row — match on `=== id`, never on + * truthiness. + */ + fetchOne: async (client, id) => { + const rows = client.getAll + ? (await client.getAll>("/person/commentviewers")).data + : ((await client.get?.[]>("/person/commentviewers")) ?? []); + return rows.find((r) => r.id === id) ?? null; + }, + }), "group-role": define({ collectionPath: "/group/roles", updateMethod: "PUT", diff --git a/tests/comment-viewer-resource.test.ts b/tests/comment-viewer-resource.test.ts new file mode 100644 index 0000000..cee9232 --- /dev/null +++ b/tests/comment-viewer-resource.test.ts @@ -0,0 +1,277 @@ +/** + * Comment viewers as a managed resource (#151) — the last scope dimension a config could not express + * portably. + * + * `cdb_comment_viewer` had a reference form (`{ commentViewer: "…" }`, #102) but no declaration + * behind it, so a config granting `churchdb:view comments` had to write a raw numeric `dataId`. That + * id is host-specific: read on two hosts of the same deployment on 2026-08-24, three of prod's six + * viewer ids did not exist on dev at all, and the two present on both named different categories on + * each. Nothing detected it — `ct plan` compares the declared id against the live id and they match; + * the id is simply meaningless on the target host. The name ref was no escape either, because the + * NAMES were missing on the second host too, so the ref hard-errored rather than resolving wrongly. + * + * Declaring the viewer is what makes the name exist on both hosts, which is what makes the ref + * portable. `/person/commentviewers` is conventional REST (`[{id, name, sortKey}]` plus + * POST/PUT/DELETE, live-probed CT 3.135.2, 2026-08-14), so this type needs none of the machinery the + * other two awkward master-data types did: CT mints the id (unlike `security-level`) and the writes + * are REST (unlike `department`). + */ +import { describe, it, expect, vi } from "vitest"; +import { evaluateConfig } from "../src/config/context.js"; +import { executePlan } from "../src/engine/execute.js"; +import { computePlan } from "../src/engine/plan.js"; +import { tierOf, isKnownType } from "../src/engine/graph.js"; +import { applyPermissionPlan } from "../src/permissions/apply.js"; +import { buildPermissionPlan } from "../src/permissions/plan.js"; +import { RESOURCES, configSnippet, knownFields } from "../src/resources/registry.js"; +import { emptyState, type State } from "../src/state/state.js"; +import type { DesiredPermission } from "../src/permissions/types.js"; +import type { DesiredResource, Plan } from "../src/engine/types.js"; +import type { CtClient } from "../src/api/ctClient.js"; + +const HOST = "https://mychurch.church.tools"; +const PATH = "/person/commentviewers"; +const SPEC = RESOURCES["comment-viewer"]!; + +/** `churchdb:view comments` — authId 113, the one right scoped by `cdb_comment_viewer`. */ +const VIEW_COMMENTS = "churchdb:view comments"; + +const noSave: (path: string, state: State) => Promise = async () => {}; + +function recorder(responses: Record = {}) { + const calls: { method: string; path: string; body?: unknown }[] = []; + const client = { + request: async (method: string, path: string, body?: unknown): Promise => { + calls.push({ method, path, body }); + return (responses[`${method} ${path}`] ?? {}) as T; + }, + }; + return { client, calls }; +} + +/** State holding one viewer under key `dienstbereich`, as a prior apply would have left it. */ +function stateWithViewer(id: number): State { + return { + version: 1, + host: HOST, + resources: { + dienstbereich: { + type: "comment-viewer", + id, + key: "dienstbereich", + fields: { name: "Dienstbereich", sortKey: 40 }, + adoptedAt: "t", + updatedAt: "t", + }, + }, + }; +} + +/** A grant scoped by a comment viewer, declared with the portable ref form. */ +const viewerScoped: DesiredPermission = { + key: "pastoral_care_grants", + domainType: "group_role", + domainId: 900, + grants: [{ right: VIEW_COMMENTS, scope: [{ commentViewer: "dienstbereich" }] }], +}; + +describe("declaring, creating and updating a comment viewer (#151)", () => { + it("is a plain collection POST — CT mints the id, unlike a security level", async () => { + const state = emptyState(HOST); + const { client, calls } = recorder({ [`POST ${PATH}`]: { id: 7 } }); + const plan: Plan = { + items: [ + { + type: "comment-viewer", + key: "dienstbereich", + id: null, + action: "create", + changes: [ + { field: "name", from: undefined, to: "Dienstbereich" }, + { field: "sortKey", from: undefined, to: 40 }, + ], + }, + ], + }; + + const res = await executePlan(plan, { client, state, statePath: "unused", save: noSave }); + expect(res.failed).toBeUndefined(); + expect(calls).toEqual([{ method: "POST", path: PATH, body: { name: "Dienstbereich", sortKey: 40 } }]); + expect(state.resources.dienstbereich).toMatchObject({ type: "comment-viewer", id: 7 }); + // Pinned against a future refactor that makes the caller-assigned-id path unconditional: this + // type must NOT post to `/person/commentviewers/{id}` the way a security level does. + expect(SPEC.createPath).toBeUndefined(); + expect(SPEC.callerAssignedId).toBeUndefined(); + }); + + it("supplies a neutral sortKey at create when the declaration omits it", () => { + // `sortkey` is a non-nullable integer column on these 3-column master-data tables, so a + // hand-authored declaration carrying only `name` still needs a valid create body. A DECLARED + // value wins (createDefaults merges UNDER the body) — that ordering is the executor's, so all + // this pins is that the default exists and is the neutral one. + expect(SPEC.createDefaults?.({ name: "Dienstbereich" })).toEqual({ sortKey: 0 }); + }); + + it("updates with a PUT carrying the whole managed set", async () => { + const state = stateWithViewer(4); + const { client, calls } = recorder(); + const plan: Plan = { + items: [ + { + type: "comment-viewer", + key: "dienstbereich", + id: 4, + action: "update", + actual: { name: "Dienstbereich", sortKey: 40 }, + changes: [{ field: "name", from: "Dienstbereich", to: "Dienstbereiche" }], + }, + ], + }; + + const res = await executePlan(plan, { client, state, statePath: "unused", save: noSave }); + expect(res.failed).toBeUndefined(); + // PUT is a full replace, so the unchanged sibling has to travel with it — `name` and `sortKey` + // are the whole editable surface, so nothing outside the managed set can be blanked. + expect(calls).toEqual([ + { method: "PUT", path: `${PATH}/4`, body: { name: "Dienstbereiche", sortKey: 40 } }, + ]); + }); + + it("applies in the master-data tier, before permissions", () => { + expect(isKnownType("comment-viewer")).toBe(true); + expect(tierOf("comment-viewer")).toBe(0); + }); + + it("names what a delete reaches, the way person-status and security-level do", () => { + expect(SPEC.destroyWarning).toMatch(/churchdb:view comments/); + expect(SPEC.destroyWarning).toMatch(/ct get comment-viewers/); + }); +}); + +describe("adopt → config → plan round-trips to a no-op (#151)", () => { + it("reads the row out of the COLLECTION, not a guessed item path", async () => { + // The probe behind this type recorded the collection read plus POST/PUT/DELETE on the item path; + // a GET on the item path was never probed. Guessing it has a genuinely bad failure mode (#108): + // a 404 reads as "vanished in ChurchTools", so every plan proposes creating the viewer again. + const rows = [ + { id: 0, name: "Alle", sortKey: 0 }, + { id: 4, name: "Dienstbereich", sortKey: 40 }, + ]; + const getAll = vi.fn(async () => ({ data: rows })); + const client = { request: async () => ({}), getAll } as never; + + expect(await SPEC.fetchOne!(client, 4)).toEqual(rows[1]); + expect(getAll).toHaveBeenCalledWith(PATH); + // `id: 0` ("Alle") is a real row on every instance — a lookup that tests truthiness would drop it. + expect(await SPEC.fetchOne!(client, 0)).toEqual(rows[0]); + expect(await SPEC.fetchOne!(client, 99)).toBeNull(); + }); + + it("emits a declaration that diffs clean against the row it was adopted from", async () => { + const live = { id: 4, name: "Dienstbereich", sortKey: 40 }; + const fields = SPEC.managedFields(live); + expect(fields).toEqual({ name: "Dienstbereich", sortKey: 40 }); + + const snippet = configSnippet("comment-viewer", "dienstbereich", fields); + expect(snippet).toContain("commentViewer("); + expect(snippet).toContain('name: "Dienstbereich"'); + + const { resources } = await evaluateConfig((ct) => { + ct.commentViewer({ key: "dienstbereich", name: "Dienstbereich", sortKey: 40 }); + }); + expect(resources[0]).toMatchObject({ type: "comment-viewer", key: "dienstbereich" }); + const plan = computePlan(resources, stateWithViewer(4), new Map([["dienstbereich", live]])); + expect(plan.items.map((i) => i.action)).toEqual(["no-op"]); + }); + + it("accepts `name` and `sortKey` as known fields — no unknown-field warning", () => { + expect(knownFields("comment-viewer")).toEqual(new Set(["name", "sortKey"])); + }); +}); + +describe("the same config means the same thing on two hosts (#151)", () => { + /** A client whose permission reads are empty and whose viewer catalog is host-specific. */ + function mockClient(viewers: { id: number; name: string }[] = [], newId = 555) { + const calls: { method: string; path: string; body?: unknown }[] = []; + const get = vi.fn(async (path: string) => (path === PATH ? viewers : [])); + const request = vi.fn(async (method: string, path: string, body?: unknown) => { + calls.push({ method, path, body }); + if (method === "POST" && path === PATH) return { id: newId }; + return {}; + }); + return { client: { get, request } as unknown as CtClient, calls }; + } + + it("resolves the ref from MANAGED state, so the dataId differs per host and the config does not", async () => { + // The exact failure #151 exists to close: the declared viewer is id 4 on one host and id 2 on the + // other. Written as a raw `scope: [4]` the second host gets a grant pointing at a different + // viewer — or at nothing — and no plan, row count or diff can see it. + for (const id of [4, 2]) { + const { client } = mockClient(); + const { items, fetchErrors } = await buildPermissionPlan(client, stateWithViewer(id), [viewerScoped]); + expect(fetchErrors).toEqual([]); + expect(items[0]?.diff.toPut).toEqual([ + { authId: 113, dataId: [id], type: "grant", scopeKey: "dienstbereich", scopeType: "comment-viewer" }, + ]); + } + }); + + it("a viewer declared in the SAME run is pending at plan time and gets its real id at apply time", async () => { + // The fresh-host case: nothing exists yet, so the viewer and the grant that scopes to it have to + // land in one run. Before #151 this could not be expressed at all — the viewer had to be created + // by hand on every target instance first. + const desired: DesiredResource[] = [ + { + type: "comment-viewer", + key: "dienstbereich", + fields: { name: "Dienstbereich", sortKey: 40 }, + dependsOn: [], + }, + ]; + const { client, calls } = mockClient([], 555); + const state = emptyState(HOST); + const { items } = await buildPermissionPlan(client, state, [viewerScoped], desired); + expect(items[0]?.diff.toPut).toEqual([ + { + authId: 113, + dataId: [], + type: "grant", + scopeKey: "dienstbereich", + scopeType: "comment-viewer", + pending: true, + }, + ]); + + const createViewer: Plan = { + items: [ + { + type: "comment-viewer", + key: "dienstbereich", + id: null, + action: "create", + changes: [ + { field: "name", from: undefined, to: "Dienstbereich" }, + { field: "sortKey", from: undefined, to: 40 }, + ], + }, + ], + }; + await executePlan(createViewer, { client, state, statePath: "unused", save: noSave }); + expect(state.resources.dienstbereich?.id).toBe(555); + + const res = await applyPermissionPlan(items, client, state); + expect(res.granted).toBe(1); + const put = calls.find((c) => c.method === "PUT" && c.path === "/permissions/group_role/900"); + expect(put?.body).toEqual({ authId: 113, type: "grant", dataId: [555] }); + }); + + it("still falls back to the live catalog for a viewer this config does not own", async () => { + // The compatibility half of the promotion: a name ref against an unmanaged viewer keeps working + // exactly as it did in #102, so no existing config has to change. + const { client } = mockClient([{ id: 2, name: "Dienstbereich" }]); + const { items, fetchErrors } = await buildPermissionPlan(client, emptyState(HOST), [viewerScoped]); + expect(fetchErrors).toEqual([]); + // Catalog-resolved: already host-correct, so no managed identity is carried for re-resolution. + expect(items[0]?.diff.toPut).toEqual([{ authId: 113, dataId: [2], type: "grant" }]); + }); +}); diff --git a/tests/permission-adopt.test.ts b/tests/permission-adopt.test.ts index b613ed2..e849fec 100644 --- a/tests/permission-adopt.test.ts +++ b/tests/permission-adopt.test.ts @@ -491,21 +491,43 @@ describe("emitAdoptedGrants", () => { expect(block).toContain('scope: [{ campus: "koblenz" }, 99]'); }); - it("a catalog-only dimension gets ONE note — the portable form, not the 'not a group' line", () => { + it("points an UNMANAGED comment-viewer scope at `ct adopt comment-viewer` (#151)", () => { + // This dimension used to be the last catalog-only one, and got the weaker "portable form: + // { commentViewer: \"\" }" note — honest then, useless in practice, because the NAME did + // not exist on the other host either. Comment viewers are a managed resource now, so the advice + // is the same as for every other dimension: adopt the viewer, then re-adopt the grants. const rows: RawPermission[] = [ - // cdb_comment_viewer — a catalog ct reads but does not manage (#102). Bereiche used to be the - // example here; they became a managed resource in #108, so they now take the adopt-hint path. { authId: 113, dataId: 4, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, ]; const block = emitAdoptedGrants({ domainType: "group_role", domainId: 42, rows, state: emptyState() }); - expect(block).toContain('Portable form: { commentViewer: "" }'); - // The numeric-escape-hatch line is for dimensions with NO logical form; emitting it here too - // would contradict the portable-form note directly above it. + expect(block).toContain("ct adopt comment-viewer 4"); + // The numeric-escape-hatch line is for dimensions with NO logical form at all. expect(block).not.toContain("not a group"); expect(block).toContain('{ right: "churchdb:view comments", scope: [4] }'); }); + it("emits a MANAGED comment-viewer scope as the portable ref form (#151)", () => { + const state = emptyState(); + state.resources.dienstbereich = { + type: "comment-viewer", + id: 4, + key: "dienstbereich", + fields: { name: "Dienstbereich", sortKey: 40 }, + adoptedAt: "t", + updatedAt: "t", + }; + const rows: RawPermission[] = [ + { authId: 113, dataId: 4, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + ]; + const block = emitAdoptedGrants({ domainType: "group_role", domainId: 42, rows, state }); + + // The whole point of #151: what used to be a raw host-specific 4 is now a name that means the + // same thing on the host this config is replayed against. + expect(block).toContain('scope: [{ commentViewer: "dienstbereich" }]'); + expect(block).not.toContain("ct adopt comment-viewer"); + }); + it("points an unmanaged Bereich scope at `ct adopt department` now that Bereiche are managed (#108)", () => { const rows: RawPermission[] = [ { authId: 102, dataId: 4, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, // cdb_bereich From e9b03aca50359cb33755d45dd7f5c64a333f3513 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Wed, 26 Aug 2026 11:08:29 +0200 Subject: [PATCH 2/3] fix(comment-viewer): probe the API for real, and never adopt the built-in "Alle" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #152 raised five findings. A live probe on eqrm-dev (CT 3.135.2, 2026-08-26 — one throwaway row created, read, updated, deleted, instance left as found) settled the two that needed evidence. - The create path was described three different ways: "conventional REST", "the item path takes POST/PUT/DELETE", and a plain collection POST in the code. The probe confirms the code: POST on the COLLECTION, CT mints the id. Replaced the vague prose in registry.ts, api-coverage.md and permissions.md with the actual verb table, and recorded why a viewer can never be callerAssignedId — the minted id skips deleted rows and moved 5 -> 8 -> 11 on a three-row instance. - Dropped `fetchOne`. It filtered the whole collection because `GET {itemPath}` was unprobed and guessing wrong has #108's failure mode. The probe retired the guess: a present id returns the row, an absent one a clean 404 error.notfound. That is what the default read needs, and it removes one full collection read per managed viewer per plan/apply/destroy — `fetchActual` fans out concurrently, so the hook cost N round-trips against a rate-limited API. - `dataId: 0` (the built-in "Alle" viewer) is no longer advertised as adoptable. Only `-1` was filtered, so adoption printed `ct adopt comment-viewer 0`; following that hint duplicates "Alle" on any second host, misgrants to the duplicate, and makes `{ commentViewer: "alle" }` permanently ambiguous. Added BUILTIN_SCOPE_IDS, keyed by dimension so `0` elsewhere stays an ordinary host-specific id, and emitted as a number (an admin can rename the row). - Corrected the stale ScopeEntry doc (departments and security levels have been declarable since #108/#110; comment viewers since #151) and a section heading that stated the opposite of its own section. The registry's "3-column table, so the managed set is complete" reasoning was wrong — rows carry a fourth column, `nameTranslated`. The conclusion holds for a different reason: the probe's PUT sent only {name, sortKey} and nameTranslated followed name, so it is derived, not a sibling a PUT could blank. Claude-Session: https://claude.ai/code/session_011DU9pubAh7dGWyojwd1smn --- docs/api-coverage.md | 12 +++--- docs/handbuch/blueprints.md | 4 +- docs/handbuch/group-member-fields.md | 4 +- docs/handbuch/permissions.md | 54 ++++++++++++++++++++++---- src/permissions/adopt.ts | 31 +++++++++++++-- src/permissions/scope.ts | 34 ++++++++++++++++ src/permissions/types.ts | 8 ++-- src/resources/registry.ts | 50 ++++++++++++------------ tests/comment-viewer-resource.test.ts | 37 ++++++++---------- tests/permission-adopt.test.ts | 56 +++++++++++++++++++++++++++ 10 files changed, 221 insertions(+), 69 deletions(-) diff --git a/docs/api-coverage.md b/docs/api-coverage.md index dc496dd..d85a98c 100644 --- a/docs/api-coverage.md +++ b/docs/api-coverage.md @@ -88,12 +88,12 @@ API-client libraries (5pm-HDH `churchtools-api` @ CT 3.104, bensteUEM `src/api/schema.d.ts` is git-ignored and was not available offline. Re-verify per the runbook's re-audit procedure once the schema is regenerated. -| Resource | Matched path(s) | GET | POST/PUT/PATCH/DELETE | Verdict | -| ------------------------------------ | ------------------------------------------------------- | ------------------ | ---------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Person master-data model | `/person/masterdata` | ✅ (single object) | ❌ | **Read-only.** Versionable master-data model incl. the `securityLevels` enumeration. No write endpoint (edited in the CT master-data admin UI). | -| Data-field definitions (Datenfelder) | `/dbfields`, `/dbfields/{id}` | list ✅ / by-id ✅ | ❌ | **Read-only.** Unified person + group field definitions, discriminated by `fieldCategory`. Mutation only via legacy churchdb AJAX (`db_insert/update/deletefields`), not REST. | -| Security levels | `/securitylevels`, `/securitylevels/{id}` | list ✅ / by-id ✅ | ✅ POST, PATCH, DELETE | **Full CRUD — but not on the usual paths.** Live-probed 2026-08-14 (eqrm-dev, CT 3.135.2): `/securitylevels` is GET-only, and **create is `POST /securitylevels/{id}`** (body `{name}`), update is `PATCH /securitylevels/{id}` (body `{name, newid, forcereorder}` — reordering is first-class), delete is `DELETE /securitylevels/{id}`. **Managed since #110** — the registry gained a `createPath` hook and a `callerAssignedId` flag for it; `ct.securityLevel({ key, id, name })` declares one, `ct adopt security-level ` adopts one. Changing a declared id is refused at plan time (that is a renumber, which rewrites what every numeric `cc_securitylevel` scope means). Resolvable by name as a scope ref since #110. The ids are NOT protocol constants — `cc_securitylevel` is an editable master-data table with an auto-increment id. | -| Comment viewers (Kommentare-Viewer) | `/person/commentviewers`, `/person/commentviewers/{id}` | list ✅ / by-id ❓ | ✅ POST, PUT, DELETE | **Full CRUD on conventional paths.** Live-probed 2026-08-14 (CT 3.135.2): `GET /person/commentviewers` returns a flat `[{id, name, sortKey}]` (`id: 0` — "Alle" — is a real row), and the item path takes POST/PUT/DELETE. **Managed since #151** — `ct.commentViewer({ key, name, sortKey })` declares one, `ct adopt comment-viewer ` adopts one; no registry machinery was needed (CT mints the id, and the writes are REST). `cdb_comment_viewer` is the scope dimension of `churchdb:view comments`, and it was the last one a config could not express portably: the ids differ across hosts of the same deployment, and before #151 a config had only the raw numeric `dataId`. A GET on the item path was never probed, so reads of a single viewer filter the collection instead of guessing it. | +| Resource | Matched path(s) | GET | POST/PUT/PATCH/DELETE | Verdict | +| ------------------------------------ | ------------------------------------------------------- | ------------------ | ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| Person master-data model | `/person/masterdata` | ✅ (single object) | ❌ | **Read-only.** Versionable master-data model incl. the `securityLevels` enumeration. No write endpoint (edited in the CT master-data admin UI). | +| Data-field definitions (Datenfelder) | `/dbfields`, `/dbfields/{id}` | list ✅ / by-id ✅ | ❌ | **Read-only.** Unified person + group field definitions, discriminated by `fieldCategory`. Mutation only via legacy churchdb AJAX (`db_insert/update/deletefields`), not REST. | +| Security levels | `/securitylevels`, `/securitylevels/{id}` | list ✅ / by-id ✅ | ✅ POST, PATCH, DELETE | **Full CRUD — but not on the usual paths.** Live-probed 2026-08-14 (eqrm-dev, CT 3.135.2): `/securitylevels` is GET-only, and **create is `POST /securitylevels/{id}`** (body `{name}`), update is `PATCH /securitylevels/{id}` (body `{name, newid, forcereorder}` — reordering is first-class), delete is `DELETE /securitylevels/{id}`. **Managed since #110** — the registry gained a `createPath` hook and a `callerAssignedId` flag for it; `ct.securityLevel({ key, id, name })` declares one, `ct adopt security-level ` adopts one. Changing a declared id is refused at plan time (that is a renumber, which rewrites what every numeric `cc_securitylevel` scope means). Resolvable by name as a scope ref since #110. The ids are NOT protocol constants — `cc_securitylevel` is an editable master-data table with an auto-increment id. | +| Comment viewers (Kommentare-Viewer) | `/person/commentviewers`, `/person/commentviewers/{id}` | list ✅ / by-id ✅ | ✅ POST, PUT, DELETE | **Full CRUD on conventional paths.** Fully live-probed 2026-08-26 (eqrm-dev, CT 3.135.2 — one throwaway row created, read, updated and deleted): `GET /person/commentviewers` returns a flat `[{id, name, nameTranslated, sortKey}]` (`id: 0` — "Alle" — is a real, built-in row), **create is a plain `POST /person/commentviewers`** with CT minting the id, and the item path takes GET/PUT/DELETE (an absent id gives a clean 404 `error.notfound`). `nameTranslated` is derived from `name`, not independently writable. **Managed since #151** — `ct.commentViewer({ key, name, sortKey })` declares one, `ct adopt comment-viewer ` adopts one; no registry machinery was needed (CT mints the id, and the writes are REST). `cdb_comment_viewer` is the scope dimension of `churchdb:view comments`, and it was the last one a config could not express portably: the ids differ across hosts of the same deployment, and before #151 a config had only the raw numeric `dataId`. The minted id is an auto-increment that does not reuse deleted rows, so it can never be caller-assigned. | See [`docs/handbuch/field-definitions.md`](handbuch/field-definitions.md) for the full writability decision, evidence, and the schema/values boundary. diff --git a/docs/handbuch/blueprints.md b/docs/handbuch/blueprints.md index d0447a8..867536c 100644 --- a/docs/handbuch/blueprints.md +++ b/docs/handbuch/blueprints.md @@ -4,8 +4,8 @@ sources: - src/config/context.ts - src/engine/graph.ts - src/engine/hierarchy.ts -sources_hash: ed5171960e9206fa -reviewed: 2026-08-17 +sources_hash: bb6f89956ace833d +reviewed: 2026-08-26 --- # Blueprints (parametrized, reusable config) diff --git a/docs/handbuch/group-member-fields.md b/docs/handbuch/group-member-fields.md index 3159af3..0bd29f7 100644 --- a/docs/handbuch/group-member-fields.md +++ b/docs/handbuch/group-member-fields.md @@ -1,5 +1,5 @@ --- -sources_hash: 0dd7d04768dda2d2 +sources_hash: 777badbcc7fbbd7f title: Group member fields sources: - src/engine/member-fields.ts @@ -7,7 +7,7 @@ sources: - src/config/context.ts - src/commands/adopt-group.ts - src/commands/destroy.ts -reviewed: 2026-08-24 +reviewed: 2026-08-26 --- # Group member fields (#135) diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index f533682..08e7ac6 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -7,8 +7,8 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: eaea3f87de51a46f -reviewed: 2026-08-25 +sources_hash: bd920e2a1991c0d8 +reviewed: 2026-08-26 --- # Permissions (`ct.groupRole` / `ct.groupTypeRole` / `ct.status`) @@ -508,7 +508,7 @@ Three things are hard errors at **plan** time, never a guessed `dataId`: group", so on e.g. a `cdb_station` right it would either fail confusingly or — worse — match an unrelated group that happens to carry that key. -### Comment viewers: the last catalog-only dimension (#151) +### Comment viewers: from catalog-only to declarable (#151) `cdb_comment_viewer` (Kommentare-Viewer) had a reference form since #102 but no declaration behind it, and that combination turned out to be the worst of both @@ -540,10 +540,27 @@ something guarantees the row is there. - **`ct adopt comment-viewer `** puts an existing viewer under management and emits the declaration; `ct get comment-viewers` lists the names. -- **`/person/commentviewers` is conventional REST** (`[{id, name, sortKey}]` plus - POST/PUT/DELETE), so this type needs none of the special machinery the other - two awkward master-data types did: ChurchTools mints the id (unlike a security - level) and the writes are REST (unlike a Bereich). +- **`/person/commentviewers` is conventional REST**, so this type needs none of + the special machinery the other two awkward master-data types did: ChurchTools + mints the id (unlike a security level) and the writes are REST (unlike a + Bereich). Fully live-probed on eqrm-dev, CT 3.135.2 (2026-08-26) — one + throwaway row created, read, updated and deleted, instance left as found: + + | Call | Result | + | ------------------------------------ | ------------------------------------------------- | + | `GET /person/commentviewers` | flat `[{id, name, nameTranslated, sortKey}]` | + | `POST /person/commentviewers` | 200 — **CT mints the id**; body `{name, sortKey}` | + | `GET /person/commentviewers/{id}` | 200; an absent id → clean 404 `error.notfound` | + | `PUT /person/commentviewers/{id}` | 200 | + | `DELETE /person/commentviewers/{id}` | 200 | + + The minted id is an auto-increment that does **not** reuse deleted rows and + moves faster than the visible row count (three consecutive probe creates on a + three-row instance minted 5, 8 and 11), which is why a viewer can never carry a + declared id the way a security level does — nothing can predict it. + `nameTranslated` is derived from `name`, not independently writable, so the + managed set (`name`, `sortKey`) is complete and a `PUT` cannot blank a sibling. + - **Resolution is managed-state first, live catalog second.** An existing reference to a viewer your config does not own keeps resolving exactly as it did in #102, and a name that matches nothing anywhere is still a hard error @@ -558,6 +575,19 @@ Declare/adopt it, fix the key/name, or use a numeric id. - **`ct apply` never deletes**, as everywhere else. `ct destroy` can, and warns: deleting a viewer reaches every person comment restricted to it and every grant scoped to it. +- **`dataId: 0` — the built-in "Alle" viewer — is the one id you should _not_ + adopt.** ChurchTools ships it on every instance, so the number already means the + same thing everywhere, and it is the likely `dataId` on a real + `churchdb:view comments` grant. Adopting it would be actively harmful rather + than merely pointless: the emitted + `ct.commentViewer({ key: "alle", name: "Alle" })` finds no state entry on a + second host, so `ct apply` creates a **second** "Alle" with a fresh id, scopes + the grant to the duplicate, and leaves a catalog where + `{ commentViewer: "alle" }` is ambiguous forever after. The adopter therefore + treats it like the `-1` sentinel — emitted as the bare number with a comment + saying what it is, never with an adoption hint. It stays a **number** rather + than a name reference on purpose: an admin can rename the row, and the id is + what does not move. With this, every scope dimension `ct` can name is also one it can declare — Bereiche left the catalog-only list in #108, security levels in #110, and comment @@ -658,7 +688,7 @@ comment-viewer bucket — **not** a group — so `GET /groups/{1,2,3}` 404s. A `scope` array entry may therefore be a plain number instead of a string: (Both of those dimensions gained a name-based form and are declarable resources -now — comment viewers in [#151](#comment-viewers-the-last-catalog-only-dimension-151), +now — comment viewers in [#151](#comment-viewers-from-catalog-only-to-declarable-151), security levels in [#110](#security-levels-a-managed-resource-with-a-declared-id-110). Numerics keep working for both; calendar categories, OAuth clients and the other module dimensions still have nothing but numerics.) @@ -785,6 +815,14 @@ guaranteed to be accepted by `ct plan` (the round trip is locked by tests): 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. +- **Built-in rows are already portable too** (#151). Same treatment as `-1`, for a + different reason: `cdb_comment_viewer` `0` ("Alle") _is_ a real row, but one every + instance has, so the number means the same thing on every host. It is emitted + verbatim with a comment naming it, never counted as an unmanaged host-specific + id, and never offered for adoption — see + [comment viewers](#comment-viewers-from-catalog-only-to-declarable-151) for what + adopting it would do to the second host. This is keyed **by dimension**: `0` on + any other scope field is an ordinary host-specific id and is still flagged. - **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). diff --git a/src/permissions/adopt.ts b/src/permissions/adopt.ts index 38e44db..1b78bd6 100644 --- a/src/permissions/adopt.ts +++ b/src/permissions/adopt.ts @@ -35,7 +35,13 @@ import { type GrantTuple, type RawPermission, } from "./grants.js"; -import { ALL_SCOPE_SENTINEL, GROUP_SCOPE_FIELD, SCOPE_REF_KIND } from "./scope.js"; +import { + ALL_SCOPE_SENTINEL, + builtinScopeIdName, + GROUP_SCOPE_FIELD, + isBuiltinScopeId, + 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 = { @@ -316,12 +322,29 @@ function grantLines(g: CollapsedGrant, rev: Map, state: St // 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; + // + // #151 generalised that: a dataId is ALREADY PORTABLE when it means the same thing on every + // host, which covers the `-1` sentinel AND a BUILT-IN row of the dimension (`cdb_comment_viewer` + // 0 = "Alle"). Both are emitted verbatim and neither counts as an unmanaged host-specific id. + // They differ in one respect worth keeping straight: `-1` is not an id at all, while a built-in + // IS a real row — just one every instance has. See `isBuiltinScopeId` for why adopting one is + // actively harmful rather than merely useless. + const portable = (id: number): boolean => + id === ALL_SCOPE_SENTINEL || isBuiltinScopeId(entry.scopeField!, id); + const realIds = g.dataIds.filter((id) => !portable(id)); + const hasSentinel = g.dataIds.includes(ALL_SCOPE_SENTINEL); if (hasSentinel) { const what = dimension ? `every ${dimension.type}` : `every value of "${entry.scopeField}"`; out.push(` // scope -1 is ChurchTools' "alle" sentinel — ${what}; host-independent.`); } + for (const id of g.dataIds) { + const builtin = builtinScopeIdName(entry.scopeField!, id); + if (builtin === null) continue; + out.push( + ` // scope ${id} is the built-in "${builtin}" ${dimension?.type ?? entry.scopeField} — ` + + `present on every instance, so the number is already portable (do not adopt it).`, + ); + } if (dimension && !dimension.managed && realIds.length > 0) { const field = SCOPE_SUGAR_FIELD[dimension.type] ?? dimension.type; out.push( @@ -334,7 +357,7 @@ 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) { + if (portable(id)) { // Portable as-is (see above) — emitted, but never counted as an unmanaged host-specific id. entries.push(String(id)); continue; diff --git a/src/permissions/scope.ts b/src/permissions/scope.ts index 211eb04..2b9bff1 100644 --- a/src/permissions/scope.ts +++ b/src/permissions/scope.ts @@ -37,6 +37,40 @@ export interface ScopeResolution { */ export const ALL_SCOPE_SENTINEL = -1; +/** + * Per-dimension dataIds that are BUILT IN to every ChurchTools instance, and therefore already + * portable — a config may write the number and mean the same thing on every host. + * + * Unlike {@link ALL_SCOPE_SENTINEL} these are REAL rows: `cdb_comment_viewer` `0` is the "Alle" + * comment viewer, which CT ships on every instance (verified live on two hosts of the same + * deployment, 2026-08-26). That difference matters — a built-in row can be renamed or deleted by an + * admin, so it is emitted as its NUMBER (exact, and immune to a rename) rather than resolved to a + * name, and it is never counted as an unmanaged host-specific id. + * + * Adopting one would be actively harmful, which is why this table exists: `ct adopt comment-viewer 0` + * yields `ct.commentViewer({ key: "alle", name: "Alle" })`, and replaying THAT config on a second + * host with a fresh state file finds no state entry, POSTs a SECOND "Alle" (CT mints a fresh id), + * scopes the grant to the duplicate instead of the built-in — the exact host-specific misgrant #151 + * exists to prevent — and leaves a catalog with two "Alle" rows, which makes `{ commentViewer: "alle" }` + * fail `resolveFromCatalog`'s ambiguity check from then on. + * + * Deliberately keyed BY DIMENSION, not a blanket "`0` is portable" rule: on any other scope field `0` + * is an ordinary host-specific id, and a global rule would silently stop flagging it. + */ +const BUILTIN_SCOPE_IDS: Readonly> = { + cdb_comment_viewer: [0], // "Alle" +}; + +/** Whether `id` on `scopeField` is a built-in row present on every host — see {@link BUILTIN_SCOPE_IDS}. */ +export function isBuiltinScopeId(scopeField: string, id: number): boolean { + return BUILTIN_SCOPE_IDS[scopeField]?.includes(id) ?? false; +} + +/** The human name of a built-in scope id, for the note the adopter emits next to the number. */ +export function builtinScopeIdName(scopeField: string, id: number): string | null { + return scopeField === "cdb_comment_viewer" && id === 0 ? "Alle" : null; +} + /** * The catalog `scopeField` naming the GROUP dimension. The only dimension a bare string scope entry * may address — every other one needs a typed ref (see {@link SCOPE_REF_KIND}) or a numeric dataId. diff --git a/src/permissions/types.ts b/src/permissions/types.ts index f700c81..e126ad8 100644 --- a/src/permissions/types.ts +++ b/src/permissions/types.ts @@ -27,11 +27,13 @@ export type ScopeSugar = * - a **logical group key** (`string`) — a group managed by this tool. Group dimension only. * - a **typed logical ref** ({@link ScopeSugar} / {@link Ref}, #98) — the portable form for the other * dimensions this tool can name by key: campuses (`cdb_station`), group types (`cdb_gruppentyp`), - * departments (`cdb_bereich`) and security levels (`cc_securitylevel`, #110). The last two are - * catalogs `ct` reads but does not manage: referenceable by name on any host, never declarable. + * departments (`cdb_bereich`, #108), security levels (`cc_securitylevel`, #110) and comment viewers + * (`cdb_comment_viewer`, #151). All of them are DECLARABLE resources — every dimension with a + * logical form is managed today, so a ref can resolve against the config's own state as well as + * against the live catalog. * - a **raw numeric dataId** (`number`, #49 escape hatch) — still supported everywhere, and the only * form for dimensions this tool cannot yet address by a host-independent name - * (`ccm_data_category`, `cdb_comment_viewer`, `oauth_client`, …). + * (`ccm_data_category`, `oauth_client`, `cc_calcategory`, …). */ export type ScopeEntry = string | number | Ref | ScopeSugar; diff --git a/src/resources/registry.ts b/src/resources/registry.ts index 5819b93..7cefb32 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -528,14 +528,25 @@ export const RESOURCES: Record = { * NAMES too, so the ref failed to resolve rather than resolving wrongly. Only a declarable * resource makes the names exist on both hosts, which is what makes a name ref portable. * - * `/person/commentviewers` is conventional REST (`[{id, name, sortKey}]` plus POST/PUT/DELETE, - * live-probed on eqrm-dev CT 3.135.2, 2026-08-14), so this needs none of the machinery the other - * two awkward master-data types did: CT mints the id (unlike `security-level`) and writes are REST - * (unlike `department`). + * `/person/commentviewers` is conventional REST, so this needs none of the machinery the other two + * awkward master-data types did: CT mints the id (unlike `security-level`) and writes are REST + * (unlike `department`). FULLY live-probed on eqrm-dev, CT 3.135.2 (2026-08-26) — one throwaway + * row created, read, updated and deleted, instance left as found: * - * `name` and `sortKey` are the whole editable surface — `cdb_comment_viewer` is one of the 3-column - * tables in #109's list (`id`, `bezeichnung`, `sortkey`) — so the managed set is complete and a PUT - * carrying it cannot blank an unmanaged sibling. + * GET /person/commentviewers → flat `[{id, name, nameTranslated, sortKey}]` + * POST /person/commentviewers → 200, CT MINTS the id (body `{name, sortKey}`) + * GET /person/commentviewers/{id} → 200; an absent id → clean 404 `error.notfound` + * PUT /person/commentviewers/{id} → 200 + * DELETE /person/commentviewers/{id} → 200 + * + * The minted id is a bare auto-increment that does NOT reuse deleted rows and moves faster than the + * visible row count (three consecutive probe creates on a 3-row instance minted 5, 8, 11), which is + * the concrete reason a comment viewer can never be `callerAssignedId`: nothing can predict it. + * + * `name` and `sortKey` are the whole editable surface. NB the row carries a fourth column the #109 + * list does not mention, `nameTranslated` — but CT DERIVES it from `name` (the probe's PUT sent only + * `{name, sortKey}` and `nameTranslated` followed `name`), so it is not an unmanaged sibling a PUT + * could blank, and the managed set is complete. */ "comment-viewer": define({ collectionPath: "/person/commentviewers", @@ -552,23 +563,14 @@ export const RESOURCES: Record = { // for the 3-column master-data tables rejects a missing integer column, so supply a neutral one // (a declared value still wins — createDefaults merges UNDER the body). createDefaults: () => ({ sortKey: 0 }), - /** - * Read one viewer out of the COLLECTION rather than through `GET {itemPath}`. - * - * The live probe behind this type recorded the collection read plus POST/PUT/DELETE on the item - * path; a GET on the item path was never probed. Guessing it is the one mistake with a genuinely - * bad failure mode (#108): a read that 404s reads as "vanished in ChurchTools", so every plan - * would propose creating the same viewer again and every apply would duplicate it. The - * collection read is the endpoint this tool has actually verified, and with ≤ a handful of rows - * per instance it costs nothing. NB `id: 0` ("Alle") is a real row — match on `=== id`, never on - * truthiness. - */ - fetchOne: async (client, id) => { - const rows = client.getAll - ? (await client.getAll>("/person/commentviewers")).data - : ((await client.get?.[]>("/person/commentviewers")) ?? []); - return rows.find((r) => r.id === id) ?? null; - }, + // No `fetchOne`: `GET /person/commentviewers/{id}` exists and behaves (probed 2026-08-26, see + // above), so the DEFAULT item read is correct here. This type briefly carried a collection- + // filtering hook while that path was unverified — guessing it would have risked #108's failure + // mode, where a 404 reads as "vanished in ChurchTools" and every apply duplicates the viewer. + // The probe retired the guess: a present id returns the row, an absent one returns a clean 404, + // which is exactly the distinction the default read needs. It also drops one full collection + // read per managed viewer per plan/apply/destroy — `fetchActual` fans out concurrently, so the + // hook cost N round-trips against a rate-limited API where the default costs N cheap item reads. }), "group-role": define({ collectionPath: "/group/roles", diff --git a/tests/comment-viewer-resource.test.ts b/tests/comment-viewer-resource.test.ts index cee9232..12f2fe5 100644 --- a/tests/comment-viewer-resource.test.ts +++ b/tests/comment-viewer-resource.test.ts @@ -11,10 +11,13 @@ * NAMES were missing on the second host too, so the ref hard-errored rather than resolving wrongly. * * Declaring the viewer is what makes the name exist on both hosts, which is what makes the ref - * portable. `/person/commentviewers` is conventional REST (`[{id, name, sortKey}]` plus - * POST/PUT/DELETE, live-probed CT 3.135.2, 2026-08-14), so this type needs none of the machinery the - * other two awkward master-data types did: CT mints the id (unlike `security-level`) and the writes - * are REST (unlike `department`). + * portable. `/person/commentviewers` is conventional REST, so this type needs none of the machinery + * the other two awkward master-data types did: CT mints the id (unlike `security-level`) and the + * writes are REST (unlike `department`). Fully live-probed on eqrm-dev, CT 3.135.2 (2026-08-26): + * collection GET/POST, item GET/PUT/DELETE, absent item id → clean 404. + * + * The one dataId that is NOT host-specific is `0`, the built-in "Alle" viewer CT ships everywhere — + * so it is emitted as a number and never offered for adoption (see the "Alle" test below). */ import { describe, it, expect, vi } from "vitest"; import { evaluateConfig } from "../src/config/context.js"; @@ -149,22 +152,16 @@ describe("declaring, creating and updating a comment viewer (#151)", () => { }); describe("adopt → config → plan round-trips to a no-op (#151)", () => { - it("reads the row out of the COLLECTION, not a guessed item path", async () => { - // The probe behind this type recorded the collection read plus POST/PUT/DELETE on the item path; - // a GET on the item path was never probed. Guessing it has a genuinely bad failure mode (#108): - // a 404 reads as "vanished in ChurchTools", so every plan proposes creating the viewer again. - const rows = [ - { id: 0, name: "Alle", sortKey: 0 }, - { id: 4, name: "Dienstbereich", sortKey: 40 }, - ]; - const getAll = vi.fn(async () => ({ data: rows })); - const client = { request: async () => ({}), getAll } as never; - - expect(await SPEC.fetchOne!(client, 4)).toEqual(rows[1]); - expect(getAll).toHaveBeenCalledWith(PATH); - // `id: 0` ("Alle") is a real row on every instance — a lookup that tests truthiness would drop it. - expect(await SPEC.fetchOne!(client, 0)).toEqual(rows[0]); - expect(await SPEC.fetchOne!(client, 99)).toBeNull(); + it("uses the DEFAULT item read — `GET {itemPath}` exists here (probed 2026-08-26)", () => { + // This type briefly carried a collection-filtering `fetchOne` because a GET on the item path was + // unprobed, and guessing it has a genuinely bad failure mode (#108): a 404 reads as "vanished in + // ChurchTools", so every plan proposes creating the viewer again. The live probe retired the + // guess — `GET /person/commentviewers/{id}` returns the row, and an ABSENT id returns a clean + // 404 `error.notfound`, which is exactly the distinction the default read needs. Keeping the hook + // would cost one full collection read per managed viewer per plan/apply/destroy (`fetchActual` + // fans out concurrently) against a rate-limited API. `department` is the only type that still + // needs the hook, because `/departments/{id}` genuinely does not exist. + expect(SPEC.fetchOne).toBeUndefined(); }); it("emits a declaration that diffs clean against the row it was adopted from", async () => { diff --git a/tests/permission-adopt.test.ts b/tests/permission-adopt.test.ts index e849fec..08db9bb 100644 --- a/tests/permission-adopt.test.ts +++ b/tests/permission-adopt.test.ts @@ -641,6 +641,62 @@ describe("emitAdoptedGrants", () => { }); }); +describe('the built-in "Alle" comment viewer (dataId 0) is not a host-specific id (#151)', () => { + // `churchdb:view comments` (authId 113) scopes by `cdb_comment_viewer`. Unlike `-1`, `0` is a REAL + // row — but it is one CT ships on every instance, so the number already means the same thing on + // every host. Adopting it is the trap: `ct adopt comment-viewer 0` yields + // `ct.commentViewer({ key: "alle", name: "Alle" })`, and replaying that on a second host with a + // fresh state finds no state entry, POSTs a SECOND "Alle", and scopes the grant to the duplicate — + // the exact misgrant #151 exists to prevent, plus a catalog with two "Alle" rows that makes + // `{ commentViewer: "alle" }` permanently ambiguous. + const alleRows: RawPermission[] = [ + { authId: 113, dataId: 0, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + ]; + + const block = (rows: RawPermission[] = alleRows) => + emitAdoptedGrants({ domainType: "group_role", domainId: 42, rows, state: emptyState() }); + + it("never tells you to adopt it", () => { + expect(block()).not.toMatch(/ct adopt comment-viewer 0/); + }); + + it("does not call it host-specific, because it is not", () => { + expect(block()).not.toMatch(/0 is not managed/); + expect(block()).not.toMatch(/host-specific number/); + }); + + it("says what it actually is", () => { + expect(block()).toMatch(/scope 0 is the built-in "Alle" comment-viewer/); + expect(block()).toContain("present on every instance"); + }); + + it("emits the NUMBER, not a name — a built-in row can still be renamed by an admin", () => { + expect(block()).toContain("scope: [0]"); + expect(block()).not.toContain("{ commentViewer:"); + }); + + it("still flags a REAL unmanaged viewer id in the same grant", () => { + const mixed: RawPermission[] = [ + { authId: 113, dataId: 0, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + { authId: 113, dataId: 4, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + ]; + const out = block(mixed); + expect(out).toContain("scope: [0, 4]"); + expect(out).toMatch(/ct adopt comment-viewer 4/); + expect(out).not.toMatch(/ct adopt comment-viewer 0/); + // The "4 is not managed" note must name 4 alone, never "0, 4". + expect(out).not.toMatch(/0, 4 (is|are) not managed/); + }); + + it("is scoped to the comment-viewer dimension — `0` elsewhere is an ordinary id", () => { + // `churchdb:view alldata` (102) scopes by `cdb_bereich`, where 0 has no special meaning. + const other: RawPermission[] = [ + { authId: 102, dataId: 0, type: "grant", domainId: 42, meta: { modifiedPid: 5 } }, + ]; + expect(block(other)).toMatch(/ct adopt department 0/); + }); +}); + 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`. From 19c696a05860cbe7f7dc7a4b5c262a5feba19c9d Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Wed, 26 Aug 2026 11:19:24 +0200 Subject: [PATCH 3/3] fix(scope): keep built-in id and name in one table; correct two claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up review of e9b03ac found three minor issues, no correctness defects. - `builtinScopeIdName` hardcoded the comment-viewer case instead of deriving from BUILTIN_SCOPE_IDS, so the two could drift: adding a dimension to the table but not the function would suppress the adoption hint while emitting no comment explaining the suppression — a bare `scope: [0]` with nothing saying why it was not flagged. The name now lives in the table and `isBuiltinScopeId` derives from it, which makes the two impossible to desync. - The "verified live on two hosts, 2026-08-26" date conflated two probes: the two-host read was 2026-08-24, and 2026-08-26 was eqrm-dev only. - The duplicate-"Alle" story overstated the ambiguity error. Resolution is managed-state-first, so on the host that created the duplicate the ref quietly resolves to it and never reaches resolveFromCatalog's ambiguity check. The hard error only reaches a config reading that catalog without a state entry, so the failure surfaces somewhere other than where it was caused — which is worse than the original wording, not better. Claude-Session: https://claude.ai/code/session_011DU9pubAh7dGWyojwd1smn --- docs/handbuch/permissions.md | 12 ++++++++---- src/permissions/scope.ts | 30 +++++++++++++++++++----------- 2 files changed, 27 insertions(+), 15 deletions(-) diff --git a/docs/handbuch/permissions.md b/docs/handbuch/permissions.md index 08e7ac6..2bda944 100644 --- a/docs/handbuch/permissions.md +++ b/docs/handbuch/permissions.md @@ -7,7 +7,7 @@ sources: - src/resolve/resolver.ts - src/resolve/refs.ts - src/config/context.ts -sources_hash: bd920e2a1991c0d8 +sources_hash: becf78e8e94dbfed reviewed: 2026-08-26 --- @@ -581,9 +581,13 @@ Declare/adopt it, fix the key/name, or use a numeric id. `churchdb:view comments` grant. Adopting it would be actively harmful rather than merely pointless: the emitted `ct.commentViewer({ key: "alle", name: "Alle" })` finds no state entry on a - second host, so `ct apply` creates a **second** "Alle" with a fresh id, scopes - the grant to the duplicate, and leaves a catalog where - `{ commentViewer: "alle" }` is ambiguous forever after. The adopter therefore + second host, so `ct apply` creates a **second** "Alle" with a fresh id and + scopes the grant to the duplicate. Worse, that host never complains: + resolution is managed-state-first, so `{ commentViewer: "alle" }` quietly + resolves to the config's own duplicate. The two identically named rows only + hard-error for a config reading the catalog _without_ a state entry — a third + host, or the same one after a state reset — so the damage surfaces somewhere + other than where it was caused. The adopter therefore treats it like the `-1` sentinel — emitted as the bare number with a comment saying what it is, never with an adoption hint. It stays a **number** rather than a name reference on purpose: an admin can rename the row, and the id is diff --git a/src/permissions/scope.ts b/src/permissions/scope.ts index 2b9bff1..d81d9ba 100644 --- a/src/permissions/scope.ts +++ b/src/permissions/scope.ts @@ -42,33 +42,41 @@ export const ALL_SCOPE_SENTINEL = -1; * portable — a config may write the number and mean the same thing on every host. * * Unlike {@link ALL_SCOPE_SENTINEL} these are REAL rows: `cdb_comment_viewer` `0` is the "Alle" - * comment viewer, which CT ships on every instance (verified live on two hosts of the same - * deployment, 2026-08-26). That difference matters — a built-in row can be renamed or deleted by an - * admin, so it is emitted as its NUMBER (exact, and immune to a rename) rather than resolved to a - * name, and it is never counted as an unmanaged host-specific id. + * comment viewer, which CT ships on every instance (present on both hosts of the same deployment in + * the 2026-08-24 two-host read; re-read on eqrm-dev 2026-08-26). That difference matters — a built-in + * row can be renamed or deleted by an admin, so it is emitted as its NUMBER (exact, and immune to a + * rename) rather than resolved to a name, and it is never counted as an unmanaged host-specific id. * * Adopting one would be actively harmful, which is why this table exists: `ct adopt comment-viewer 0` * yields `ct.commentViewer({ key: "alle", name: "Alle" })`, and replaying THAT config on a second - * host with a fresh state file finds no state entry, POSTs a SECOND "Alle" (CT mints a fresh id), + * host with a fresh state file finds no state entry, POSTs a SECOND "Alle" (CT mints a fresh id) and * scopes the grant to the duplicate instead of the built-in — the exact host-specific misgrant #151 - * exists to prevent — and leaves a catalog with two "Alle" rows, which makes `{ commentViewer: "alle" }` - * fail `resolveFromCatalog`'s ambiguity check from then on. + * exists to prevent. Note the duplicate does NOT announce itself on the host that created it: + * resolution is managed-state-first (`Resolver.resolve`), so `{ commentViewer: "alle" }` there + * silently resolves to the config's own duplicate and never reaches `resolveFromCatalog`'s ambiguity + * check. The two "Alle" rows only hard-error for a config reading that catalog WITHOUT a state entry + * — so the failure surfaces on a third host, or after a state reset, not where it was caused. * * Deliberately keyed BY DIMENSION, not a blanket "`0` is portable" rule: on any other scope field `0` * is an ordinary host-specific id, and a global rule would silently stop flagging it. + * + * The NAME lives in this same table rather than in a parallel lookup: `isBuiltinScopeId` suppresses + * the adoption hint and `builtinScopeIdName` supplies the comment that explains the suppression, so + * two tables that disagreed would emit a bare `scope: [0]` with nothing saying why it was not flagged + * — precisely the confusion this exists to prevent. */ -const BUILTIN_SCOPE_IDS: Readonly> = { - cdb_comment_viewer: [0], // "Alle" +const BUILTIN_SCOPE_IDS: Readonly>>> = { + cdb_comment_viewer: { 0: "Alle" }, }; /** Whether `id` on `scopeField` is a built-in row present on every host — see {@link BUILTIN_SCOPE_IDS}. */ export function isBuiltinScopeId(scopeField: string, id: number): boolean { - return BUILTIN_SCOPE_IDS[scopeField]?.includes(id) ?? false; + return builtinScopeIdName(scopeField, id) !== null; } /** The human name of a built-in scope id, for the note the adopter emits next to the number. */ export function builtinScopeIdName(scopeField: string, id: number): string | null { - return scopeField === "cdb_comment_viewer" && id === 0 ? "Alle" : null; + return BUILTIN_SCOPE_IDS[scopeField]?.[id] ?? null; } /**