diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b16eaa..8730e7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -137,6 +137,24 @@ called out explicitly under **Changed** or **Removed**. `alignment`), and a centered `Image` (`position: "center"`, the only value this surface sets) now actually renders centered in `content` instead of flush left. +- **`kizen permissions group-update --settings-file `** raises or + lowers object/field/section controls on an *existing* permission group — + the same op shapes `group-create --settings-file` already accepts, now a + second consumer. Dry-run shows a `change` (current -> target) per op, read + from the live group. Object/field ops that target an object the group has + no entry for **add it at `none` and cannot raise it** (confirmed live: the + server silently corrects the requested level and reports it in the + response). Both `group-update` and `group-create --settings-file` catch + this in two ways: an `object`-op `level` outside the control's own + `allowed_access` (e.g. `associated_records: none`, which the server would + silently clamp to `view`) is rejected up front with a `PlanError`; and a + mismatch that survives that check because a *legal* value still got + adjusted by a cross-field rule (e.g. `associated_records >= all_records`) + is reported as an `adjusted` op with a plain-language message — not a + failure, since the server applied a value the design already delegates to + it — while a mismatch on a control that had **no entry at all** at plan + time (the fresh-insert case above) still surfaces as `failed`. See + `docs/specs/permission-group.md`. ### Fixed diff --git a/scripts/cli-tree-baseline.txt b/scripts/cli-tree-baseline.txt index c173e0b..734093f 100644 --- a/scripts/cli-tree-baseline.txt +++ b/scripts/cli-tree-baseline.txt @@ -2655,6 +2655,7 @@ │ group Show one permission group as a sectioned permission map (mirrors the UI). │ │ meta Show the permissions catalog / meta-data (raw). │ │ group-create Create a permission group (full default structure, optionally shaped). │ +│ group-update Raise/lower specific controls on an existing permission group. │ │ group-delete Delete a permission group. │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ @@ -2712,6 +2713,29 @@ │ --help Show this message and exit. │ ╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ + Usage: permissions group-update [OPTIONS] {group} + + Raise/lower specific controls on an existing permission group. + + Each op calls `object-update` (object/field) or a section PATCH — never + a full-group PUT — so the server normalizes cross-field rules for you. + +╭─ Arguments ──────────────────────────────────────────────────────────────────────────────────────╮ +│ * group Permission group name or UUID. [required] │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ +╭─ Options ────────────────────────────────────────────────────────────────────────────────────────╮ +│ * --settings-file JSON list of shaping ops to apply (object/field/section) — │ +│ same shape as `group-create --settings-file`. │ +│ [required] │ +│ --dry-run Show the plan without applying. │ +│ --yes -y Skip the y/N confirmation prompt. │ +│ --json Emit JSON (plan with --dry-run, results otherwise). │ +│ --help Show this message and exit. │ +╰──────────────────────────────────────────────────────────────────────────────────────────────────╯ + + Settings-file shape (a list of shaping ops): see `kizen docs show permission-group` + + Usage: permissions groups [OPTIONS] List permission groups (raw). @@ -4544,6 +4568,9 @@ ===== permissions group-delete ===== +===== permissions group-update ===== + + ===== permissions groups ===== diff --git a/src/kizen_builder/api/permissions.py b/src/kizen_builder/api/permissions.py index 0945cad..3cb1065 100644 --- a/src/kizen_builder/api/permissions.py +++ b/src/kizen_builder/api/permissions.py @@ -104,13 +104,6 @@ def delete_permission_group(client: KizenClient, group_id: str) -> None: client.delete(f"/api/permission-group/{group_id}") -def duplicate_permission_group( - client: KizenClient, group_id: str, payload: dict[str, Any] -) -> dict[str, Any]: - """POST /api/permission-group/{id}/duplicate — {name}. Copies all levels.""" - return client.post(f"/api/permission-group/{group_id}/duplicate", json=payload) - - def patch_permission_group( client: KizenClient, group_id: str, payload: dict[str, Any] ) -> dict[str, Any]: diff --git a/src/kizen_builder/cli/_mutations.py b/src/kizen_builder/cli/_mutations.py index c476af2..f0d1ac8 100644 --- a/src/kizen_builder/cli/_mutations.py +++ b/src/kizen_builder/cli/_mutations.py @@ -48,7 +48,9 @@ def _render_result(result: plan_tools.ApplyResult) -> None: table.add_column("server uuid") table.add_column("note") for r in result.results: - symbol = {"ok": "✓", "skipped": "·", "failed": "✗"}.get(r.status, "?") + symbol = {"ok": "✓", "skipped": "·", "failed": "✗", "adjusted": "~"}.get( + r.status, "?" + ) table.add_row( symbol, r.kind, diff --git a/src/kizen_builder/cli/permissions.py b/src/kizen_builder/cli/permissions.py index be10813..b780c83 100644 --- a/src/kizen_builder/cli/permissions.py +++ b/src/kizen_builder/cli/permissions.py @@ -437,6 +437,43 @@ def perms_group_create( ) +@perms_app.command( + "group-update", + epilog="Settings-file shape (a list of shaping ops): see `kizen docs show permission-group`", +) +def perms_group_update( + group: str = typer.Argument(..., help="Permission group name or UUID."), + settings_file: str = typer.Option( + ..., + "--settings-file", + help="JSON list of shaping ops to apply (object/field/section) — same " + "shape as `group-create --settings-file`.", + ), + dry_run: bool = typer.Option( + False, "--dry-run", help="Show the plan without applying." + ), + yes: bool = typer.Option( + False, "--yes", "-y", help="Skip the y/N confirmation prompt." + ), + json_out: bool = typer.Option( + False, "--json", help="Emit JSON (plan with --dry-run, results otherwise)." + ), +) -> None: + """Raise/lower specific controls on an existing permission group. + + Each op calls `object-update` (object/field) or a section PATCH — never + a full-group PUT — so the server normalizes cross-field rules for you. + """ + group_id = _resolve_group_id(group) + settings = json.loads(Path(settings_file).read_text()) + _run_mutation( + lambda: perm_planners.plan_update_permission_group(group_id, settings), + dry_run=dry_run, + yes=yes, + json_out=json_out, + ) + + @perms_app.command("group-delete") def perms_group_delete( group: str = typer.Argument(..., help="Permission group name or UUID."), diff --git a/src/kizen_builder/docs/commands.md b/src/kizen_builder/docs/commands.md index 7738e4b..97b8351 100644 --- a/src/kizen_builder/docs/commands.md +++ b/src/kizen_builder/docs/commands.md @@ -285,6 +285,7 @@ kizen roles create --name X [--group ...] [--permission ...] kizen roles update [--name Y] [--group ...] [--default/--no-default] # --group REPLACES the set kizen roles delete kizen permissions group-create --name X [--base default|clone] [--from ] [--settings-file f] +kizen permissions group-update --settings-file f # raise/lower controls on an EXISTING group; same op shapes as group-create kizen permissions group-delete # patch one automation step (GET → translate → mutate node → validate → atomic PUT) diff --git a/src/kizen_builder/docs/specs/permission-group.md b/src/kizen_builder/docs/specs/permission-group.md index 326862e..218d611 100644 --- a/src/kizen_builder/docs/specs/permission-group.md +++ b/src/kizen_builder/docs/specs/permission-group.md @@ -1,6 +1,11 @@ # Spec shape: permission-group shaping ops -**Consumed by:** `kizen permissions group-create --settings-file `. +**Consumed by:** `kizen permissions group-create --settings-file ` and +`kizen permissions group-update --settings-file ` — same op list, +same file. `group-create` applies it after building a new group; +`group-update` applies it to a group that already exists, and its dry-run +preview shows a `change` (current level -> target level) per op read from +the live group, not just the target. A group is created at a **base** (`--base default` = fresh group at Kizen's default levels, or `--base clone --from ` = copy an existing group). @@ -17,12 +22,17 @@ The `--settings-file` is an optional **JSON list of shaping ops** applied ```json [ - { "type": "object", "object_id": "", "key": "records", "level": "edit" }, + { "type": "object", "object_id": "", "key": "all_records", "level": "edit" }, { "type": "field", "object_id": "", "field_id": "", "level": "view" }, { "type": "section", "section_key": "automations", "value": true } ] ``` +`key` must be a real object control key — `all_records`, `associated_records`, +`create_record`, `unarchive_all`, `record_overview_chart_view`, +`default_for_new_field`, and others (`records` is not one; see +`kizen permissions meta` for the full list). *(confirmed live 2026-09-01)* + ```bash kizen permissions group-create --name "Sales Ops" --settings-file ops.json --dry-run ``` @@ -31,13 +41,18 @@ kizen permissions group-create --name "Sales Ops" --settings-file ops.json --dry | `type` | Keys | Effect | |--------|------|--------| -| `object` | `object_id`, `key`, `level` | Set an object-level permission (e.g. `records`, `custom_fields`) to `level`. | +| `object` | `object_id`, `key`, `level` | Set an object-level permission (e.g. `all_records`, `create_record`) to `level`. | | `field` | `object_id`, `field_id`, `level` | Set a per-field control to `level`. | | `section` | `section_key`, `value` | Toggle/set an app-section permission. | `level` is a level **name** (`none`, `view`, `edit`, `remove`, …) or its integer index — the valid range per control comes from that control's `allowed_access` -(visible in `permissions group `). +(visible in `permissions group `). Both `group-create --settings-file` +and `group-update` reject an out-of-range `level` for an `object` op at plan +time (e.g. `associated_records: none`, which has no `none` in its +`allowed_access`) with a `PlanError` naming the control and its valid +levels, instead of sending it and letting the server silently clamp it — see +"Write model" below. *(confirmed live 2026-09-01)* ## Gotchas @@ -86,7 +101,45 @@ and only resets leaf values, which is why `group-create` needs a `--base`. (`"customize_homepages: This field is required."`). - **Custom object + field perms** go through a different endpoint: `PATCH /api/permission-group/{id}/object-update` with - `{custom_object: {id}, field?: {id}, key?, permission_level: 0-3}`. + `{custom_object: {id}, field?: {id}, key?, permission_level: 0-3}`. Three + distinct outcomes when the response's `permission_level` differs from what + was requested, all reported via `response.details.message`: + 1. **Out-of-range clamp.** Requesting `associated_records: none` when + `allowed_access` is `["view","edit","remove"]` comes back + `permission_level: 1` (`view`). `group-create --settings-file` and + `group-update` never let this reach the endpoint at all (see "Op + shapes" above) — rejected as a `PlanError` at plan time instead. + 2. **Cross-field normalization on a control the group already carries.** + A *request the control's own `allowed_access` says is legal* still gets + adjusted to satisfy a rule involving another control's value — e.g. on + a group where `all_records` had just been raised to `remove`, + requesting `associated_records: view` (legal on its own — + `allowed_access` includes `view`) came back `permission_level: 3` + (`remove`), to satisfy `associated_records >= all_records`. The write + still succeeded — the server picked the nearest legal value for the + *combined* state, which is exactly the normalization these commands + delegate to `object-update` for, rather than hand-building a full + group PUT and enforcing the rules here. Neither command can predict + this at plan time without reimplementing the server's rule engine, so + it's only visible in the apply result: reported as `status: "adjusted"` + (not `"failed"`), with a message like `"requested view, server + normalized to remove (Permission level was automatically corrected by + rule.)"` — `kizen apply`'s exit code stays 0. + 3. **Insert of an object with no entry in the group at all.** Always + lands at `none`, **even when the requested level is in range** — the + defect this item exists to at least surface honestly. Not a one-time + insert quirk: a second identical apply against the now-present entry + is *also* corrected back to `none` — the exact server rule wasn't + identified, only that it isn't simply "no entry yet" (case 2's example + above shows a *present* control also being normalized). This is the + one genuine failure: reported as `status: "failed"`, and `kizen + apply`'s exit code goes non-zero. + + `group-create --settings-file`/`group-update` distinguish case 2 from + case 3 using whether the control had a live entry at plan time (recorded + on the op at plan time, since the group's state can change between ops in + the same plan — case 2's example needed `all_records` raised by an + earlier op in the same batch to reproduce). *(confirmed live 2026-09-01)* - **A full PUT** `/api/permission-group/{id}` replaces the whole structure, but is subject to cross-field **rules** — e.g. `associated_records ≥ all_records`, `unarchive_all ≤ unarchive_associated`, `create_record` needs @@ -102,10 +155,31 @@ and only resets leaf values, which is why `group-create` needs a `--base`. ## Command surface `kizen roles list|get|create|update|delete` and `kizen permissions -groups|group|meta|group-create|group-delete`. **Names are accepted anywhere a -role or group is referenced** — resolved to a UUID, with an available-list on a -miss. `kizen permissions group [--fields]` renders the sectioned slider -view that mirrors the permission editor. +groups|group|meta|group-create|group-update|group-delete`. **Names are +accepted anywhere a role or group is referenced** — resolved to a UUID, with +an available-list on a miss. `kizen permissions group [--fields]` +renders the sectioned slider view that mirrors the permission editor. + +`group-update` applies shaping ops directly — `object`/`field` ops call +`object-update`, `section` ops call the section PATCH — it never assembles a +full-group PUT, so the server's cross-field-rule normalization on those two +endpoints still applies (see "Write model" above, case 2) — deliberately: +reimplementing that rule engine client-side isn't in scope. Two consequences +in the CLI: +- A `change` preview line for an `object`/`field` op reads e.g. `"Records: + view -> edit (subject to server rules)"` when the control already has a + live entry — the target level is what was asked for, not a guarantee, since + a later op in the same plan (or the group's pre-existing state) can still + trigger a cross-field normalization that changes the outcome. +- What happens when an `object`/`field` op targets an object the group has + no entry for: **it adds the object at `none` and cannot raise it** (not + "does not yet add missing objects") — `group-update` detects this specific + case (a request that was in range but landed at `none` anyway, on a + control absent at plan time) and reports it as `failed`. A normalization + on a control that *was* present is reported as `adjusted` instead — a + successful write, not a failure — so it doesn't cost `kizen apply` a + non-zero exit for the server doing exactly what this design delegates to + it. ## See also diff --git a/src/kizen_builder/tools/permission_builder.py b/src/kizen_builder/tools/permission_builder.py index 89bf4fb..1b2c17d 100644 --- a/src/kizen_builder/tools/permission_builder.py +++ b/src/kizen_builder/tools/permission_builder.py @@ -70,7 +70,7 @@ def current_level(self) -> str: if self._wire_key is None else self._container[self._wire_key] ) - return _value_to_level(value, self.allowed_access) + return value_to_level(value, self.allowed_access) def set_level(self, level: str) -> None: if self._wire_key is None: @@ -81,7 +81,7 @@ def set_level(self, level: str) -> None: ) -def _value_to_level(value: Any, allowed: list[str]) -> str: +def value_to_level(value: Any, allowed: list[str]) -> str: """Read a wire value back to a level label.""" if isinstance(value, bool): if not value: @@ -98,6 +98,18 @@ def _value_to_level(value: Any, allowed: list[str]) -> str: return "none" +def substitute_object_label(label: str, entity: str) -> str: + """Fill an object-area control label's ``{0}`` placeholder with the + object's display name (e.g. ``"All {0} Records"`` -> ``"All Companies + Records"``). Meta only templates object labels this way — field and + section labels never carry ``{0}`` — so a label without it is returned + unchanged. + """ + if "{0}" not in label: + return label + return label.replace("{0}", entity).replace(" ", " ").strip() + + def _level_to_shape(shape: Any, level: str) -> Any: """Return a value matching ``shape``'s wire form set to ``level``.""" idx = level_index(level) diff --git a/src/kizen_builder/tools/permissions.py b/src/kizen_builder/tools/permissions.py index 7e564c2..bba19b7 100644 --- a/src/kizen_builder/tools/permissions.py +++ b/src/kizen_builder/tools/permissions.py @@ -243,9 +243,10 @@ def describe_group(group_id: str, include_fields: bool = False) -> dict[str, Any blk["enabled"] = leaf.current_level != "none" continue row_label = leaf.row_label - if blk["area"] == "object" and "{0}" in row_label: - entity = obj_entities.get(leaf.block_key, "") - row_label = row_label.replace("{0}", entity).replace(" ", " ").strip() + if blk["area"] == "object": + row_label = pb.substitute_object_label( + row_label, obj_entities.get(leaf.block_key, "") + ) if leaf.category == "custom_fields": row_label = field_names.get(leaf.row_key, leaf.row_key) category_label = ( diff --git a/src/kizen_builder/tools/planners/permissions.py b/src/kizen_builder/tools/planners/permissions.py index 7a555b9..3e55d70 100644 --- a/src/kizen_builder/tools/planners/permissions.py +++ b/src/kizen_builder/tools/planners/permissions.py @@ -15,8 +15,15 @@ from kizen_builder.api import permissions as perm_api from kizen_builder.api.client import KizenClient from kizen_builder.config import load_env_config -from kizen_builder.tools.permission_builder import build_default_group_payload -from kizen_builder.tools.permissions import LEVELS_BY_NAME +from kizen_builder.tools import objects as obj_tools +from kizen_builder.tools.permission_builder import ( + Leaf, + build_default_group_payload, + enumerate_leaves, + substitute_object_label, + value_to_level, +) +from kizen_builder.tools.permissions import LEVELS, LEVELS_BY_NAME from kizen_builder.tools.plans import Plan, PlanError, PlanOperation @@ -189,7 +196,7 @@ def plan_create_permission_group( ) ] for i, s in enumerate(settings or []): - ops.append(_setting_op(name, i, s)) + ops.append(_setting_op(name, i, s, meta=meta)) summary = f"Create permission group '{name}' (base={base})" if settings: @@ -197,8 +204,60 @@ def plan_create_permission_group( return Plan.build(env=env, summary=summary, operations=ops) -def _setting_op(group_key: str, idx: int, s: dict[str, Any]) -> PlanOperation: - """Build a permission_setting op that resolves the group id at apply time.""" +def _find_leaf( + group: dict[str, Any], meta: dict[str, Any], area: str, block_key: str, row_key: str +) -> Leaf | None: + """Locate the enumerate_leaves() leaf a setting op targets, or None. + + None means the control has no entry in the live group at all — an + object/field/section key the group's payload doesn't carry. The caller + renders that as "(not present)" rather than guessing a level. + """ + for leaf in enumerate_leaves(group, meta, include_fields=True): + if ( + leaf.area == area + and leaf.block_key == block_key + and leaf.row_key == row_key + ): + return leaf + return None + + +def _meta_control(meta: dict[str, Any], key: str) -> dict[str, Any] | None: + """The `meta["custom_objects"]` descriptor for an object-control key, or + None. Object control keys (``all_records`` etc.) match one; a field id + never does — meta has no per-field descriptors — so callers that fall + back to a field id's raw value on a `None` here are also correct: a + missing field degrading to its id is the same behavior as a found one + whose name couldn't be resolved. + """ + for desc in meta.get("custom_objects", []): + if desc.get("key") == key: + return desc + return None + + +def _setting_op( + group_key: str, + idx: int, + s: dict[str, Any], + *, + existing_group_id: str | None = None, + current_group: dict[str, Any] | None = None, + meta: dict[str, Any] | None = None, + obj_entities: dict[str, str] | None = None, +) -> PlanOperation: + """Build a permission_setting op. + + By default the op defers its parent group id to a ``create`` op earlier + in the same plan (``group_key``) — the group doesn't exist yet at plan + time. Pass ``existing_group_id`` (with the live ``current_group`` + + ``meta`` it was read from) to target an already-existing group directly: + ``parent_object_uuid`` is set immediately and the preview gains a + ``change`` string (current level -> target level), read from the live + group rather than guessed. ``obj_entities`` (object id -> display name) + fills the ``{0}`` placeholder in object-area control labels. + """ stype = s.get("type") if stype in ("object", "field"): level = s["level"] @@ -213,23 +272,158 @@ def _setting_op(group_key: str, idx: int, s: dict[str, Any]) -> PlanOperation: body["key"] = s["key"] target = s.get("field_id") or s.get("key") or s["object_id"] preview = {"target": f"{stype}:{target}", "level": level} - payload = {"mode": "object_update", "body": body} + payload: dict[str, Any] = {"mode": "object_update", "body": body} + after = LEVELS.get(level_int, str(level_int)) + # Default: no live group to check against (group-create) or meta + # unavailable — assume the control is/will-be present rather than + # risk mislabeling a server normalization as the genuine "no entry + # at all" defect. group-create's `create` op always inserts every + # object that currently exists, so its settings ops are never that + # defect either way. + payload["control_present"] = True + if meta is not None: + # Validated whenever meta is available — both group-create's + # settings (current_group is None, group doesn't exist yet) and + # group-update's (current_group is the live group) — so an + # out-of-range level (e.g. `associated_records: none`, which has + # no "none" in its allowed_access) is a PlanError before any + # write, not a server-side clamp that later looks identical to + # the fresh-insert bug this item fixes. See docs/specs/ + # permission-group.md. + row_key = ( + s["field_id"] if stype == "field" else s.get("key", s["object_id"]) + ) + leaf = ( + _find_leaf(current_group, meta, stype, s["object_id"], row_key) + if current_group is not None + else None + ) + if current_group is not None: + payload["control_present"] = leaf is not None + # Meta only describes object controls (`all_records` etc.), not + # fields, so a field with no live entry has no allowed_access + # source at plan time — it's left unvalidated, same as before. + desc = ( + None + if leaf is not None or stype == "field" + else _meta_control(meta, str(row_key)) + ) + allowed = ( + leaf.allowed_access + if leaf is not None + else (desc.get("allowed_access") if desc else None) + ) + if allowed is not None and after not in allowed: + raise PlanError( + f"{row_key!r} does not accept level {after!r} — allowed: " + f"{', '.join(allowed)}" + ) + if existing_group_id is not None: + label = ( + leaf.row_label + if leaf is not None + else (desc.get("label", str(row_key)) if desc else str(row_key)) + ) + if stype == "object": + label = substitute_object_label( + label, (obj_entities or {}).get(s["object_id"], "") + ) + before = leaf.current_level if leaf else "(not present)" + # Even a present, in-range control isn't a guaranteed outcome + # — a cross-field rule (`associated_records >= all_records` + # etc.) can still normalize it based on the group's *final* + # state, which this planner doesn't (and per this item's + # constraints, shouldn't) simulate. Only the "no entry at + # all" case renders a bare target: that one always lands at + # `none` regardless, so there's nothing to hedge. + shown_after = ( + after if leaf is None else f"{after} (subject to server rules)" + ) + preview["change"] = f"{label}: {before} -> {shown_after}" elif stype == "section": preview = {"target": f"section:{s['section_key']}", "value": s["value"]} payload = {"mode": "section", "body": {s["section_key"]: s["value"]}} + if existing_group_id is not None: + assert current_group is not None and meta is not None + parts = [] + for row_key, wire_value in s["value"].items(): + leaf = _find_leaf( + current_group, meta, "section", s["section_key"], row_key + ) + allowed = leaf.allowed_access if leaf else list(LEVELS.values()) + label = leaf.row_label if leaf else row_key + before = leaf.current_level if leaf else "(not present)" + after = value_to_level(wire_value, allowed) + parts.append(f"{label}: {before} -> {after}") + preview["change"] = "; ".join(parts) else: raise PlanError(f"unknown setting type {stype!r}") + op_kwargs: dict[str, Any] = {} + if existing_group_id is not None: + op_kwargs["parent_object_uuid"] = existing_group_id + else: + op_kwargs["deferred_parent_object_key"] = group_key + return PlanOperation( action="update", kind="permission_setting", key=f"{group_key}.setting[{idx}]", preview=preview, payload=payload, - deferred_parent_object_key=group_key, + **op_kwargs, ) +def plan_update_permission_group(group_id: str, settings: list[dict[str, Any]]) -> Plan: + """Plan shaping updates against an *existing* permission group. + + Same op shapes as ``plan_create_permission_group``'s ``settings`` + (object/field/section — see docs/specs/permission-group.md), reused + verbatim. Unlike create, the group already exists, so each op targets + its id directly (no ``deferred_parent_object_key``) and the preview + carries a ``change`` (current -> target) read from the live group, not + just the target level. Applying raises/lowers exactly those controls — + it never assembles a full-group PUT, so the cross-field rules + (``associated_records >= all_records`` etc.) are left to the server's + ``object-update``/section-PATCH normalization. + """ + if not settings: + raise PlanError("group-update requires at least one setting to apply.") + + env = load_env_config().name + with _client() as c: + group = perm_api.get_permission_group(c, group_id) + meta = perm_api.get_permissions_meta_data(c) + + # Object-area labels are templated ("All {0} Records") and meta carries no + # object names to fill them with — only fetch the object list (a second + # round trip) when an object-type op actually needs one. + obj_entities: dict[str, str] = {} + if any(s.get("type") == "object" for s in settings): + obj_entities = { + o["id"]: (o.get("entity_name") or o.get("display_name") or "") + for o in obj_tools.list_objects() + } + + ops = [ + _setting_op( + group.get("name") or group_id, + i, + s, + existing_group_id=group_id, + current_group=group, + meta=meta, + obj_entities=obj_entities, + ) + for i, s in enumerate(settings) + ] + summary = ( + f"Update permission group '{group.get('name')}' ({len(settings)} setting(s))" + ) + return Plan.build(env=env, summary=summary, operations=ops) + + def plan_delete_permission_group(group_id: str) -> Plan: env = load_env_config().name with _client() as c: diff --git a/src/kizen_builder/tools/plans.py b/src/kizen_builder/tools/plans.py index 848e89e..68d9dfb 100644 --- a/src/kizen_builder/tools/plans.py +++ b/src/kizen_builder/tools/plans.py @@ -32,6 +32,7 @@ from kizen_builder.api import saved_views as sv_api from kizen_builder.api.client import KizenAPIError, KizenClient from kizen_builder.config import load_env_config +from kizen_builder.tools.permissions import LEVELS class PlanError(ValueError): @@ -183,7 +184,10 @@ class OperationResult(BaseModel): key: str kind: Kind action: Action - status: Literal["ok", "failed", "skipped"] + # "adjusted": the write succeeded but the server normalized the value + # away from what was requested (e.g. a cross-field rule) — not a + # failure, just not exact. Excluded from all_ok the same as "ok". + status: Literal["ok", "failed", "skipped", "adjusted"] server_uuid: str | None = None message: str | None = None raw: dict[str, Any] | None = None @@ -200,7 +204,7 @@ class ApplyResult(BaseModel): @property def all_ok(self) -> bool: - return all(r.status in ("ok", "skipped") for r in self.results) + return all(r.status in ("ok", "skipped", "adjusted") for r in self.results) # --------------------------------------------------------------------------- @@ -303,14 +307,61 @@ def apply_plan(plan: Plan) -> ApplyResult: if server_uuid: results_by_key[op.key] = server_uuid message = None + status: Literal["ok", "failed", "skipped", "adjusted"] = ( + "ok" if op.action != "skip" else "skipped" + ) if op.action == "upsert" and isinstance(resp, dict) and resp.get("action"): message = resp["action"] # "created" or "updated" + elif ( + op.kind == "permission_setting" + and op.payload.get("mode") == "object_update" + and isinstance(resp, dict) + ): + # object-update silently corrects the level it's given instead + # of 4xx-ing, and reports it in the response body — but there + # are two different reasons, and only one is a failure: + # + # 1. The control had no entry in the group at plan time + # (`control_present=False` — set in `_setting_op`). Insert + # always lands at "none" regardless of what was asked, a + # follow-up apply doesn't change that, and nothing the + # caller wanted happened. Genuine failure. + # 2. The control was already present — a *legal* write that a + # cross-field rule (e.g. `associated_records >= all_records`) + # then normalized based on the group's final state, which + # this planner doesn't simulate (see docs/specs/ + # permission-group.md). The write succeeded; it just didn't + # land exactly where asked. Not a failure — reported as + # "adjusted" so it doesn't flip `kizen apply`'s exit code. + requested = op.payload["body"].get("permission_level") + returned = resp.get("permission_level") + if ( + requested is not None + and returned is not None + and returned != requested + ): + detail = (resp.get("details") or {}).get("message") + if op.payload.get("control_present") is False: + status = "failed" + message = ( + f"server set permission_level={returned}, requested " + f"{requested}" + (f" — {detail}" if detail else "") + ) + failed_keys.add(op.key) + else: + status = "adjusted" + requested_name = LEVELS.get(requested, str(requested)) + returned_name = LEVELS.get(returned, str(returned)) + message = ( + f"requested {requested_name}, server normalized to " + f"{returned_name}" + (f" ({detail})" if detail else "") + ) results.append( OperationResult( key=op.key, kind=op.kind, action=op.action, - status="ok" if op.action != "skip" else "skipped", + status=status, server_uuid=server_uuid, message=message, raw=resp if isinstance(resp, dict) else None, diff --git a/tests/test_permission_plans.py b/tests/test_permission_plans.py index 13d2af2..9acfbe8 100644 --- a/tests/test_permission_plans.py +++ b/tests/test_permission_plans.py @@ -19,13 +19,14 @@ import respx from kizen_builder.api import permissions as perm_api -from kizen_builder.api.client import KizenClient +from kizen_builder.api.client import KizenAPIError, KizenClient from kizen_builder.config import load_env_config from kizen_builder.tools.planners.permissions import ( plan_create_permission_group, plan_create_role, plan_delete_permission_group, plan_delete_role, + plan_update_permission_group, plan_update_role, ) from kizen_builder.tools.plans import PlanError @@ -43,6 +44,10 @@ GROUP_LIST = load_fixture("permissions/permission_group_list.json") GROUP_DETAIL = load_fixture("permissions/permission_group_detail.json") META = load_fixture("permissions/permissions_meta_data.json") +OBJECT_LIST = { + "results": [{"id": OBJ_ID, "name": "policies_policy", "entity_name": "Policy"}], + "next": None, +} def _mock_role_list(): @@ -75,6 +80,12 @@ def _mock_meta(): ) +def _mock_object_list(): + return respx.get(f"{FAKE_BASE_URL}/api/custom-objects").mock( + return_value=httpx.Response(200, json=OBJECT_LIST) + ) + + # --------------------------------------------------------------------------- # plan_create_role # --------------------------------------------------------------------------- @@ -356,6 +367,10 @@ def test_plan_create_permission_group_settings_build_object_field_and_section_op "permission_level": 2, "key": "records", }, + # No live group to check at create time, but the group's own `create` + # op always inserts every currently-existing object — never the + # "no entry at all" case apply_plan treats as a genuine failure. + "control_present": True, } # A "field" setting only ever carries `field`, never `key` — the branch @@ -369,6 +384,7 @@ def test_plan_create_permission_group_settings_build_object_field_and_section_op "permission_level": 1, "field": {"id": FIELD_ID}, }, + "control_present": True, } assert section_op.kind == "permission_setting" @@ -382,6 +398,37 @@ def test_plan_create_permission_group_settings_build_object_field_and_section_op assert "3 setting(s)" in plan.summary +@respx.mock +def test_plan_create_permission_group_settings_reject_level_outside_allowed_access(): + """The same out-of-range check `group-update` needs applies here too — + `--settings-file` ops are the same shapes on both commands, and the + server clamps an out-of-range level here exactly like it does on an + existing group.""" + narrow_meta = json.loads(json.dumps(META)) + narrow_meta["custom_objects"][0]["allowed_access"] = ["edit"] + _mock_group_list() + _mock_group_detail() + respx.get(f"{FAKE_BASE_URL}/api/permissions/meta-data").mock( + return_value=httpx.Response(200, json=narrow_meta) + ) + + try: + plan_create_permission_group( + name="New Group", + settings=[ + { + "type": "object", + "object_id": OBJ_ID, + "key": "records", + "level": "remove", + } + ], + ) + raise AssertionError("expected PlanError") + except PlanError as exc: + assert "'records'" in str(exc) and "edit" in str(exc) + + @respx.mock def test_plan_create_permission_group_setting_level_as_integer(): _mock_group_list() @@ -428,6 +475,258 @@ def test_plan_delete_permission_group(): assert op.key == GROUP_DETAIL["name"] +# --------------------------------------------------------------------------- +# plan_update_permission_group +# --------------------------------------------------------------------------- + + +@respx.mock +def test_plan_update_permission_group_object_op_sets_group_id_directly_with_change(): + """Unlike create, the group already exists: no + `deferred_parent_object_key` — `parent_object_uuid` is set immediately — + and the preview carries a `change` (current -> target), not just the + target level.""" + _mock_group_detail() + _mock_meta() + _mock_object_list() + + plan = plan_update_permission_group( + GROUP_ID, + settings=[ + {"type": "object", "object_id": OBJ_ID, "key": "records", "level": "edit"} + ], + ) + + (op,) = plan.operations + assert op.kind == "permission_setting" + assert op.deferred_parent_object_key is None + assert op.parent_object_uuid == GROUP_ID + assert op.payload == { + "mode": "object_update", + "body": { + "custom_object": {"id": OBJ_ID}, + "permission_level": 2, + "key": "records", + }, + # A live leaf was found -> not the "no entry at all" case. + "control_present": True, + } + # GROUP_DETAIL's records leaf for OBJ_ID is {"view": true, ...} -> "view". + # A present control's target level is still a request, not a promise — + # a cross-field rule can still normalize it. + assert op.preview["change"] == "Records: view -> edit (subject to server rules)" + + +@respx.mock +def test_plan_update_permission_group_object_op_fills_label_placeholder(): + """meta's object-control labels carry a `{0}` slot for the object's + display name (e.g. "All {0} Records") — only object ops need the extra + /api/custom-objects round trip to fill it in.""" + templated_meta = json.loads(json.dumps(META)) + templated_meta["custom_objects"][0]["label"] = "All {0} Records" + _mock_group_detail() + respx.get(f"{FAKE_BASE_URL}/api/permissions/meta-data").mock( + return_value=httpx.Response(200, json=templated_meta) + ) + _mock_object_list() + + plan = plan_update_permission_group( + GROUP_ID, + settings=[ + {"type": "object", "object_id": OBJ_ID, "key": "records", "level": "edit"} + ], + ) + + (op,) = plan.operations + assert ( + op.preview["change"] + == "All Policy Records: view -> edit (subject to server rules)" + ) + + +@respx.mock +def test_plan_update_permission_group_object_op_rejects_level_outside_allowed_access(): + """object-update silently clamps an out-of-range level + instead of 4xx-ing (e.g. requesting "none" on a control whose + allowed_access starts at "view"), and reports that clamp exactly like it + reports the real bug this item fixes (a fresh insert always landing at + "none"). Only a plan-time check against the control's own allowed_access + can tell the two apart, so `_setting_op` must reject an out-of-range + request before it ever reaches the server.""" + narrow_meta = json.loads(json.dumps(META)) + narrow_meta["custom_objects"][0]["allowed_access"] = ["view", "edit", "remove"] + _mock_group_detail() # GROUP_DETAIL has a "records" entry for OBJ_ID + respx.get(f"{FAKE_BASE_URL}/api/permissions/meta-data").mock( + return_value=httpx.Response(200, json=narrow_meta) + ) + _mock_object_list() + + try: + plan_update_permission_group( + GROUP_ID, + settings=[ + { + "type": "object", + "object_id": OBJ_ID, + "key": "records", + "level": "none", + } + ], + ) + raise AssertionError("expected PlanError") + except PlanError as exc: + assert "'records'" in str(exc) + assert "view" in str(exc) and "edit" in str(exc) + + +@respx.mock +def test_plan_update_permission_group_object_op_rejects_level_for_missing_object(): + """Same check, for an object with no entry in the group at all — the + `_find_leaf` miss falls back to meta's own control descriptor for + allowed_access, since there's no live leaf to read it from.""" + narrow_meta = json.loads(json.dumps(META)) + narrow_meta["custom_objects"][0]["allowed_access"] = ["edit"] + _mock_group_detail() + respx.get(f"{FAKE_BASE_URL}/api/permissions/meta-data").mock( + return_value=httpx.Response(200, json=narrow_meta) + ) + _mock_object_list() + + try: + plan_update_permission_group( + GROUP_ID, + settings=[ + { + "type": "object", + # not in GROUP_DETAIL's custom_objects -> no leaf + "object_id": "00000000-0000-4000-8000-000000000999", + "key": "records", + "level": "remove", + } + ], + ) + raise AssertionError("expected PlanError") + except PlanError as exc: + assert "allowed" in str(exc) and "edit" in str(exc) + + +@respx.mock +def test_plan_update_permission_group_field_op_skips_validation_without_a_leaf(): + """A field with no live entry has no allowed_access source at plan time + (meta only describes object controls) — unlike the object case, this is + left unvalidated rather than guessed at.""" + _mock_group_detail() + _mock_meta() + + plan = plan_update_permission_group( + GROUP_ID, + settings=[ + { + "type": "field", + "object_id": OBJ_ID, + "field_id": "no-such-field-id", + "level": "remove", + } + ], + ) + + (op,) = plan.operations + assert op.preview["change"] == "no-such-field-id: (not present) -> remove" + # No leaf to skip validation against also means no leaf to prove the + # control is live -> apply_plan must still treat a later mismatch here + # as the genuine "no entry at all" defect, not a normalized write. + assert op.payload["control_present"] is False + + +@respx.mock +def test_plan_update_permission_group_field_op_reads_before_from_live_group(): + _mock_group_detail() + _mock_meta() + + plan = plan_update_permission_group( + GROUP_ID, + settings=[ + { + "type": "field", + "object_id": OBJ_ID, + "field_id": FIELD_ID, + "level": "view", + } + ], + ) + + (op,) = plan.operations + assert op.parent_object_uuid == GROUP_ID + assert op.payload == { + "mode": "object_update", + "body": { + "custom_object": {"id": OBJ_ID}, + "permission_level": 1, + "field": {"id": FIELD_ID}, + }, + "control_present": True, + } + # GROUP_DETAIL's field is {"view": true, "edit": true} -> highest = "edit". + assert op.preview["change"] == f"{FIELD_ID}: edit -> view (subject to server rules)" + + +@respx.mock +def test_plan_update_permission_group_section_op_diffs_every_subkey(): + _mock_group_detail() + _mock_meta() + + plan = plan_update_permission_group( + GROUP_ID, + settings=[ + { + "type": "section", + "section_key": "dashboards_section", + "value": {"enabled": False, "view_all_dashboards": False}, + } + ], + ) + + (op,) = plan.operations + assert op.parent_object_uuid == GROUP_ID + assert op.payload == { + "mode": "section", + "body": { + "dashboards_section": {"enabled": False, "view_all_dashboards": False} + }, + } + # GROUP_DETAIL's dashboards_section is fully enabled -> both read "view". + assert op.preview["change"] == ( + "Enabled: view -> none; View All Dashboards: view -> none" + ) + + +@respx.mock +def test_plan_update_permission_group_raises_when_group_not_found(): + respx.get(f"{FAKE_BASE_URL}/api/permission-group/{UNKNOWN_GROUP_ID}").mock( + return_value=httpx.Response(404, json={"detail": "not found"}) + ) + + try: + plan_update_permission_group( + UNKNOWN_GROUP_ID, + settings=[ + {"type": "object", "object_id": OBJ_ID, "key": "records", "level": 1} + ], + ) + raise AssertionError("expected KizenAPIError") + except KizenAPIError as exc: + assert exc.status_code == 404 + + +@respx.mock +def test_plan_update_permission_group_raises_when_settings_empty(): + try: + plan_update_permission_group(GROUP_ID, settings=[]) + raise AssertionError("expected PlanError") + except PlanError as exc: + assert "setting" in str(exc) + + # --------------------------------------------------------------------------- # api.permissions write endpoints — the two dialects `_setting_op` routes # between. Nothing in the planner or tools layer calls these directly (only diff --git a/tests/test_plans.py b/tests/test_plans.py index c6af581..ee3cc39 100644 --- a/tests/test_plans.py +++ b/tests/test_plans.py @@ -317,3 +317,110 @@ def test_apply_skip_ops_never_hit_the_api(): (r,) = result.results assert r.status == "skipped" assert result.all_ok + + +def _permission_setting_op(**overrides) -> PlanOperation: + base = { + "action": "update", + "kind": "permission_setting", + "key": "Group.setting[0]", + "preview": {}, + "payload": { + "mode": "object_update", + "body": {"custom_object": {"id": "obj-uuid"}, "permission_level": 2}, + # False: no live entry at plan time — the genuine-defect case. + # The "present but normalized" test below overrides this. + "control_present": False, + }, + "parent_object_uuid": "group-uuid", + } + base.update(overrides) + return PlanOperation(**base) + + +@respx.mock +def test_apply_permission_setting_fails_when_control_was_absent(): + """object-update silently corrects the level it's given (e.g. a + freshly-inserted object always lands at "none") and reports it in the + response body instead of a 4xx — apply_plan must not report `ok` for a + write that didn't do what was asked. `control_present=False` (no live + entry at plan time) is what makes this the genuine-defect case, not a + legal server normalization.""" + respx.patch(f"{FAKE_BASE_URL}/api/permission-group/group-uuid/object-update").mock( + return_value=httpx.Response( + 200, + json={ + "key": "all_records", + "permission_level": 0, + "details": { + "message": "Permission level was automatically corrected by rule." + }, + }, + ) + ) + plan = Plan.build(env="testenv", summary="t", operations=[_permission_setting_op()]) + + result = plan_tools.apply_plan(plan) + + (r,) = result.results + assert r.status == "failed" + assert "permission_level=0, requested 2" in r.message + assert "automatically corrected by rule" in r.message + assert not result.all_ok + + +@respx.mock +def test_apply_permission_setting_adjusted_when_control_was_present(): + """A live probe found the server clamping a legal, + in-range write on a control the group already carried (Companies' + `associated_records`, normalized up to satisfy `associated_records >= + all_records` after an earlier op in the same plan raised `all_records`). + That's the server doing exactly what this item's design delegates to it + — reported as "adjusted", not "failed", and must not flip the exit code.""" + respx.patch(f"{FAKE_BASE_URL}/api/permission-group/group-uuid/object-update").mock( + return_value=httpx.Response( + 200, + json={ + "key": "associated_records", + "permission_level": 3, + "details": { + "message": "Permission level was automatically corrected by rule." + }, + }, + ) + ) + op = _permission_setting_op( + payload={ + "mode": "object_update", + "body": {"custom_object": {"id": "obj-uuid"}, "permission_level": 1}, + "control_present": True, + } + ) + plan = Plan.build(env="testenv", summary="t", operations=[op]) + + result = plan_tools.apply_plan(plan) + + (r,) = result.results + assert r.status == "adjusted" + assert r.message == ( + "requested view, server normalized to remove " + "(Permission level was automatically corrected by rule.)" + ) + assert result.all_ok + + +@respx.mock +def test_apply_permission_setting_ok_when_level_matches(): + respx.patch(f"{FAKE_BASE_URL}/api/permission-group/group-uuid/object-update").mock( + return_value=httpx.Response( + 200, json={"key": "all_records", "permission_level": 2} + ) + ) + plan = Plan.build(env="testenv", summary="t", operations=[_permission_setting_op()]) + + result = plan_tools.apply_plan(plan) + + (r,) = result.results + assert r.status == "ok" + assert r.message is None + assert result.all_ok