feat(security): RBAC M3 — per-user policy GET/PUT + dashboard editor#3229
Merged
Conversation
Wires the M6 dashboard's stub `UserPolicyPage` to a live `PUT /api/users/{name}/policy`
endpoint that upserts the per-user `tool_policy`, `tool_categories`,
`memory_access`, and `channel_tool_rules` slice landed by M3 (#3205). M5
already exposes per-user budget edits the same way; this PR mirrors that
shape for the policy slice so the dashboard can stop linking to "after M3
ships".
Endpoint semantics:
- Each top-level field independently nullable: absent = preserve,
`null` = clear, object = replace. Implemented by deserializing the
body as a raw `serde_json::Map` so we can distinguish absent from
null (`Option<T>` would collapse them).
- Validates non-empty / non-duplicate string entries, rejects
unknown top-level keys, and enforces a new invariant: every
`writable_namespaces` entry must appear in `readable_namespaces`
(no upstream check exists today; the resolver assumes well-formed
config).
- Owner-only via the existing `is_owner_only_write` allowlist —
per-user policy edits change someone's authorization surface, so
they ride the same gate as create/delete.
Dashboard:
- `UserPolicyPage` becomes a real form with sections for tool
allow/deny, categories, memory access (PII / export / delete
toggles), and per-channel rules. Client-side validation mirrors
the server's so typos surface inline.
- `useUpdateUserPolicy` now invalidates `permissionPolicyKeys.detail`
plus `userKeys.detail` and `userKeys.lists` (policy fields are
part of the underlying `UserConfig`).
- `getUserPolicy` / `updateUserPolicy` rewritten to match the wire
shape; new `PermissionPolicyUpdate` distinguishes preserve from
clear via `undefined` vs `null`.
Tests:
- `users_policy_get_round_trip` — non-default seed survives GET.
- `users_policy_put_replaces_only_specified_fields` — clear one
slot, preserve another that's absent from the body.
- `users_policy_put_validates_writable_subset_of_readable` — the
new subset invariant.
- `users_policy_put_validates_no_empty_tool_strings`.
- `users_policy_put_owner_only` (full middleware stack) — Admin gets
403, Owner gets 200; pins the owner-only gate.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 tasks
…-write-api # Conflicts: # crates/librefang-api/dashboard/src/lib/http/client.ts # crates/librefang-api/tests/api_integration_test.rs
…-write-api # Conflicts: # crates/librefang-api/tests/api_integration_test.rs
…-write-api # Conflicts: # crates/librefang-api/dashboard/src/lib/http/client.ts # crates/librefang-api/tests/api_integration_test.rs
houko
added a commit
that referenced
this pull request
Apr 26, 2026
… follow-up) Closes two gaps deferred from PR #3229 (UserPolicy editor): 1. UserView gains has_policy / has_memory_access / has_budget summary booleans. The list view (`GET /api/users`) now lets the dashboard render "Policy / Memory / Budget" badges per row without an extra round-trip per user. Bodies stay behind the per-user detail endpoints — the list never echoes tool_policy / memory_access / budget contents (smaller payload, narrower disclosure surface for list-read-only callers). 2. UserPolicyPage's per-channel rules editor is no longer locked to the four hard-coded defaults. Operators can add custom channel adapter slots (e.g. `wechat`, `matrix`) inline; non-default rows gain a remove (X) control. Pre-existing extra channels already render via the form-init spread, but were undeletable until now. Tests: - users_list_summary_flags_reflect_policy_state: bare user reports all three flags false; customized user reports true on the slots it sets. - users_list_summary_does_not_leak_policy_contents: response carries the booleans but NOT tool_policy / tool_categories / memory_access / channel_tool_rules / budget / api_key_hash.
houko
added a commit
that referenced
this pull request
Apr 26, 2026
… follow-up) (#3232) * feat(rbac): user-list summary flags + custom channel rule editor (#3229 follow-up) Closes two gaps deferred from PR #3229 (UserPolicy editor): 1. UserView gains has_policy / has_memory_access / has_budget summary booleans. The list view (`GET /api/users`) now lets the dashboard render "Policy / Memory / Budget" badges per row without an extra round-trip per user. Bodies stay behind the per-user detail endpoints — the list never echoes tool_policy / memory_access / budget contents (smaller payload, narrower disclosure surface for list-read-only callers). 2. UserPolicyPage's per-channel rules editor is no longer locked to the four hard-coded defaults. Operators can add custom channel adapter slots (e.g. `wechat`, `matrix`) inline; non-default rows gain a remove (X) control. Pre-existing extra channels already render via the form-init spread, but were undeletable until now. Tests: - users_list_summary_flags_reflect_policy_state: bare user reports all three flags false; customized user reports true on the slots it sets. - users_list_summary_does_not_leak_policy_contents: response carries the booleans but NOT tool_policy / tool_categories / memory_access / channel_tool_rules / budget / api_key_hash. * style: cargo fmt users_test.rs row finder Collapses the multi-line `rows.iter().find(...).expect(...)` call onto a single line so `cargo xtask fmt` stops failing the Quality CI job. * fix(security/rbac): validate channel_tool_rules keys server-side The pre-fix `validate_channel_rules` only checked `channel.trim().is_empty()`, which let through: - Embedded control characters (`"foo\nbar".trim()` is non-empty) - 10 KB blobs that bloat the TOML round-trip - Non-ASCII or whitespace-bearing names that could never match a real channel adapter (`telegram`, `slack`, `feishu`, …) These survived the PUT and ended up in `config.toml`. Tightened the validator to: - Length cap of 64 chars (channel adapter names are short slugs) - Reject any control char (`c.is_control()`) - Charset `[a-zA-Z0-9_-]+` matching adapter naming convention New regression test `users_policy_put_validates_channel_rules_keys` pins each rejection branch (empty, newline, overlong, non-ASCII) plus a positive case for `feishu` so the new validators don't over-reject. Refs PR #3232 review item #18.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Activates
UserPolicyPageby addingGET+PUT /api/users/{name}/policy— the write side of the RBAC management surface, complementing the read-side simulator (#3228). Today the page is a stub linking to "after M3 ships". M3 (feat(security): RBAC M3 — per-user tool policy + memory namespace ACL) is merged in main; this PR connects the management plane.Surface
GET /api/users/{name}/policy— owner-only. Returns the user's currenttool_policy,tool_categories,memory_access,channel_tool_rulesfromUserConfig.PUT /api/users/{name}/policy— owner-only. Body shape:{ "tool_policy": { "allowed_tools": [...], "denied_tools": [...] } | null | <omitted>, "tool_categories": { ... } | null | <omitted>, "memory_access": { ... } | null | <omitted>, "channel_tool_rules": { "telegram": {...}, ... } | <omitted> }nullclears, omitted preserves — this distinction is critical and required deserializing the body asserde_json::Maprather than the typed struct (Option<T>collapses both intoNone).is_owner_only_writemiddleware allowlist (/api/users/*with non-GET method) — no new gate code, just a new route under the existing umbrella.Validation
memory_access.writable_namespaces ⊆ readable_namespaces— newly enforced at this layer with a comment noting there's no upstream check today and the kernel resolver assumes well-formed configtool_polices) can't silently no-op into "preserve everything"Dashboard
UserPolicyPagerewritten from stub to a 4-section editor: tool allow/deny lists, tool categories, memory access (with PII checkbox), per-channel rules. Client-side validation mirrors server.useUpdateUserPolicyinvalidatespermissionPolicyKeys.detail(name)+userKeys.detail(name)+userKeys.lists()on success.PermissionPolicytype rewritten to match the real wire shape (the previous one was an M3-stub guess written before the endpoint shipped — same root cause as feat(dashboard): activate AuditPage now that M5 audit endpoints shipped #3225 / feat(api): per-user budget write/clear endpoints + dashboard editor #3224 / feat(security): RBAC effective-permissions snapshot — wire simulator (#3054) #3228).Test plan
users_policy_get_round_trip,users_policy_put_replaces_only_specified_fields,users_policy_put_validates_writable_subset_of_readable,users_policy_put_validates_no_empty_tool_strings— 4/4 pass.users_policy_put_owner_only— Admin caller (not Owner) blocked at the middleware → 403.users_test16/16,api_integration_test -- test_audit test_user_budget4/4 (all M5 surface still passes).cargo check -p librefang-api -p librefang-kernel -p librefang-typesclean;cargo clippy -p librefang-api --tests -- -D warningsclean.pnpm build+pnpm typecheckclean (no new errors; pre-existing errors insessions-stream.test.tsxunrelated).Out-of-scope follow-ups noted
UserItemdoesn't yet expose policy fields, so theuserKeysinvalidation is defensive — no UI today reads policy offUserItem. Costs nothing to leave in.update_user_policydoesn't add its ownRoleChangeaudit row —persist_usersemits a generic config-change row. Could add a finer-grained record likeupdate_user_budgetdoes; stylistic, not a bug.Refs #3054.