Skip to content

feat(security): RBAC M3 — per-user policy GET/PUT + dashboard editor#3229

Merged
houko merged 7 commits into
mainfrom
feat/rbac-user-policy-write-api
Apr 26, 2026
Merged

feat(security): RBAC M3 — per-user policy GET/PUT + dashboard editor#3229
houko merged 7 commits into
mainfrom
feat/rbac-user-policy-write-api

Conversation

@houko

@houko houko commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Activates UserPolicyPage by adding GET + 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 current tool_policy, tool_categories, memory_access, channel_tool_rules from UserConfig.
  • 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>
    }
    Each field is independently nullable. null clears, omitted preserves — this distinction is critical and required deserializing the body as serde_json::Map rather than the typed struct (Option<T> collapses both into None).
  • Owner-only inherited from existing is_owner_only_write middleware allowlist (/api/users/* with non-GET method) — no new gate code, just a new route under the existing umbrella.

Validation

  • Tool / namespace strings: nonempty, no whitespace-only entries
  • No duplicate entries within a single list
  • 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 config
  • Unknown top-level keys rejected so a typo (tool_polices) can't silently no-op into "preserve everything"

Dashboard

Test plan

  • Unit-style: 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.
  • Full-stack through the auth middleware: users_policy_put_owner_only — Admin caller (not Owner) blocked at the middleware → 403.
  • No regressions in surrounding tests: users_test 16/16, api_integration_test -- test_audit test_user_budget 4/4 (all M5 surface still passes).
  • cargo check -p librefang-api -p librefang-kernel -p librefang-types clean; cargo clippy -p librefang-api --tests -- -D warnings clean.
  • Dashboard pnpm build + pnpm typecheck clean (no new errors; pre-existing errors in sessions-stream.test.tsx unrelated).
  • Live: not run against a real daemon. The integration test goes through the full middleware + handler + persist_users + reload chain on a real axum server, which is the meaningful end-to-end coverage; spinning up the daemon is operator-validation rather than CI.

Out-of-scope follow-ups noted

  • UserItem doesn't yet expose policy fields, so the userKeys invalidation is defensive — no UI today reads policy off UserItem. Costs nothing to leave in.
  • Channel rules editor only exposes 4 default platforms (telegram/discord/slack/email); arbitrary channel names work in the API but the UI needs a "+ add channel" button to surface them. Not in this slice.
  • update_user_policy doesn't add its own RoleChange audit row — persist_users emits a generic config-change row. Could add a finer-grained record like update_user_budget does; stylistic, not a bug.

Refs #3054.

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.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@github-actions github-actions Bot added the size/XL 1000+ lines changed label Apr 26, 2026
@github-actions github-actions Bot added ready-for-review PR is ready for maintainer review has-conflicts PR has merge conflicts that need resolution and removed ready-for-review PR is ready for maintainer review labels Apr 26, 2026
houko and others added 2 commits April 26, 2026 21:24
…-write-api

# Conflicts:
#	crates/librefang-api/dashboard/src/lib/http/client.ts
#	crates/librefang-api/tests/api_integration_test.rs
@github-actions github-actions Bot added ready-for-review PR is ready for maintainer review has-conflicts PR has merge conflicts that need resolution and removed has-conflicts PR has merge conflicts that need resolution ready-for-review PR is ready for maintainer review labels Apr 26, 2026
…-write-api

# Conflicts:
#	crates/librefang-api/tests/api_integration_test.rs
@github-actions github-actions Bot added ready-for-review PR is ready for maintainer review and removed has-conflicts PR has merge conflicts that need resolution labels Apr 26, 2026
@github-actions github-actions Bot added has-conflicts PR has merge conflicts that need resolution and removed ready-for-review PR is ready for maintainer review labels Apr 26, 2026
…-write-api

# Conflicts:
#	crates/librefang-api/dashboard/src/lib/http/client.ts
#	crates/librefang-api/tests/api_integration_test.rs
@github-actions github-actions Bot added ready-for-review PR is ready for maintainer review and removed has-conflicts PR has merge conflicts that need resolution labels Apr 26, 2026
@houko
houko merged commit f1b5c4e into main Apr 26, 2026
20 checks passed
@houko
houko deleted the feat/rbac-user-policy-write-api branch April 26, 2026 13:37
@github-actions github-actions Bot removed the ready-for-review PR is ready for maintainer review label Apr 26, 2026
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.
@houko houko mentioned this pull request Apr 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant