Skip to content

feat(security): RBAC M2 — audit user/channel attribution + stable UserId (#3054)#3196

Merged
houko merged 2 commits into
mainfrom
feat/rbac-m2-audit-attribution
Apr 26, 2026
Merged

feat(security): RBAC M2 — audit user/channel attribution + stable UserId (#3054)#3196
houko merged 2 commits into
mainfrom
feat/rbac-m2-audit-attribution

Conversation

@houko

@houko houko commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements Phase 3 (Channel Mapping & Audit) of the unified RBAC plan in #3054 by attributing every audit-log entry to the user and channel that triggered it. Stable user IDs make this attribution survive daemon restarts.

This is the next slice on top of RBAC M1 (which shipped the role model, agent access lists, and /api/authz/* CRUD). Phase 2 (tool/memory policies) and the rest of Phase 3 (Telegram/Discord channel mapping, /api/audit/query + /api/audit/export, per-user budgets) are deferred to follow-up PRs.

Changes

  • librefang-types: New UserId::from_name(name) derives a deterministic v5 UUID under a frozen namespace constant. Replaces random v4 ids in AuthManager so the same configured name yields the same id forever — across restarts, config reloads, and nodes. Renaming a user produces a new id by design (old audit history stays attached to the old identity).
  • librefang-memory: Schema migration v22 adds nullable user_id and channel columns to audit_entries plus indexes, idempotent via column_exists. Pre-M1 rows keep their original Merkle hashes because the hash function omits absent fields.
  • librefang-runtime: AuditEntry carries optional user_id / channel. New AuditLog::record_with_context(...) records attribution; legacy record(...) is now a thin wrapper passing None/None, so every existing call site stays valid. Attribution is committed to the SHA-256 chain — stripping it from a stored row breaks verify_integrity.
  • librefang-api: ApiUserAuth / AuthenticatedApiUser carry the stable UserId, pre-computed at config-load. The three kernel-mutating config endpoints (shutdown, config_reload, config_set) thread the caller through to record_with_context with channel = "api". No new public routes, so the auth is_public allowlist is unchanged.

Backward compatibility

  • Schema migration is purely additive; both new columns are nullable.
  • Pre-M1 audit rows verify with their original hash (the hash skips absent fields).
  • Existing record(...) call sites compile and run unchanged.
  • Legacy 8-column INSERT INTO audit_entries (no user_id/channel) still works after v22 — covered by an explicit test.

Phase coverage of #3054

Phase Status
Phase 1 — Role model & agent access shipped in M1
Phase 2 — Tool & memory policies deferred
Phase 3 — Channel mapping & audit this PR (audit slice + stable id)
Phase 3 — Telegram/Discord channel mapping, /api/audit/query, /api/audit/export, per-user budgets deferred
Phase 4 — Dashboard, identity linking, simulator, LDAP/OIDC deferred

Test plan

Unit tests added (all green locally):

  • librefang-types::agent::tests
    • test_user_id_from_name_is_stable — same name → same id across calls
    • test_user_id_from_name_differs_per_name — distinct names → distinct ids; case-sensitive
    • test_user_id_from_name_is_v5 — version nibble must be 5
  • librefang-kernel::auth::tests
    • test_user_ids_stable_across_manager_rebuilds — restarting AuthManager keeps ids stable
    • test_distinct_users_get_distinct_ids — sanity check
  • librefang-memory::migration::tests
    • test_migrate_v22_adds_user_id_and_channel_columns — columns present + legacy 8-column INSERT still works
    • test_migrate_v22_preserves_existing_rows — pre-existing rows survive intact with NULL attribution
    • test_migrate_v22_is_idempotent — running migrations twice is a no-op
  • librefang-runtime::audit::tests
    • test_record_with_context_round_trips_user_and_channel — attribution persists in memory + tampering with user_id breaks the chain
    • test_record_with_context_persists_user_and_channel — DB round-trip after simulated restart
    • test_user_id_from_name_is_stable_across_audit_writes — re-derived id matches recorded id

Local verification:

  • cargo check -p librefang-kernel -p librefang-types -p librefang-memory -p librefang-api -p librefang-runtime — clean
  • cargo test -p librefang-kernel auth:: — 10 passed
  • cargo test -p librefang-memory migration:: — 6 passed (3 new)
  • cargo test -p librefang-runtime audit:: — 11 passed (3 new)
  • cargo test -p librefang-types user_id — 5 passed (3 new)
  • cargo clippy -p librefang-kernel -p librefang-runtime -- -D warnings — clean

Live integration verification deferred — daemon-up testing requires a focused workspace and parallel agents are running on other worktrees on this disk.

Refs #3054

houko added 2 commits April 26, 2026 09:01
Replace random v4 UUIDs in AuthManager with deterministic v5 UUIDs
derived from `UserConfig.name` via `UserId::from_name`. The previous
behaviour minted a fresh user id on every daemon boot, which made
audit-log correlation across restarts impossible.

`from_name` uses a frozen namespace UUID so the same name maps to the
same id forever — across restarts, config reloads, and nodes — while a
rename intentionally produces a new id so old audit history stays
attached to the old identity.

Adds tests covering id stability, distinctness, version-5 encoding, and
that AuthManager exposes the same id across rebuilds (the contract the
upcoming audit-attribution wiring depends on).

Refs #3054 (Phase 3 prep)
Extend the Merkle audit trail with optional `user_id` / `channel`
attribution so security-sensitive actions can be traced back to the
caller, not just the agent that ran them.

- Adds nullable `user_id` and `channel` columns to `audit_entries`
  via schema migration v22, with indexes on both for query support.
  Columns are nullable so pre-M1 rows keep verifying with their
  original Merkle hashes — `compute_entry_hash` folds the new fields
  into the digest only when present, so a NULL-only row produces the
  same hash it always did.
- Introduces `AuditLog::record_with_context(...)` carrying the new
  attribution; the legacy `record(...)` becomes a thin wrapper that
  passes `None`/`None`, keeping every existing call site valid.
- Persists / re-loads the new columns and verifies them on chain
  walk so stripping attribution from a stored row breaks the link.

Tests cover: round-trip of attribution, that hash includes user_id
(tampering breaks the chain), DB persistence + restart, schema v22
column add + idempotency + legacy-INSERT compatibility.

Refs #3054 (Phase 3)
@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 ready-for-review PR is ready for maintainer review area/runtime Agent loop, LLM drivers, WASM sandbox area/kernel Core kernel (scheduling, RBAC, workflows) size/L 250-999 lines changed and removed ready-for-review PR is ready for maintainer review labels Apr 26, 2026
@houko
houko merged commit 9028525 into main Apr 26, 2026
22 checks passed
@houko
houko deleted the feat/rbac-m2-audit-attribution branch April 26, 2026 00:29
houko added a commit that referenced this pull request Apr 26, 2026
Lands the four endpoints requested by RBAC M5:

- GET /api/audit/query — filter the hash-chained audit log by user,
  action, agent, channel, and ISO-8601 time range. Newest-first, hard
  cap 5000 rows. Filtering is in-memory (the AuditLog::recent path
  already verifies the chain on read), so SQL injection surface is
  zero.
- GET /api/audit/export?format=json|csv — same filters, streamed via
  axum::body::Body::from_stream so the kernel never buffers a full
  result set. CSV emits a fixed header row with RFC-4180 cell
  escaping pinned by a regression test.
- GET /api/budget/users — per-user cost ranking (top N) joined with
  the user's configured limits when present so the dashboard can
  render % bars without a follow-up call.
- GET /api/budget/users/{user_id} — single-user detail with hourly /
  daily / monthly spend, alert threshold, and an alert_breach flag.
  Accepts both the canonical UUID and the raw configured name
  (re-derived via UserId::from_name) so operators can paste either.

All four endpoints are admin-only and gate in-handler against
UserRole::Admin — they stay AUTHENTICATED (not on the is_public
allowlist). A 403 also fires a PermissionDenied audit event so an
operator can spot probes via /api/audit/query.

Middleware glue:
- AuthState gains an optional audit_log handle; when a user-scoped
  role check returns FORBIDDEN we now record PermissionDenied with
  the caller's UserId so the chain shows who tripped which path.
- The dirty middleware/config glue from PR #3196 (threading
  authenticated UserId through to record_with_context) is folded in
  unchanged.

OpenAPI registers all four new operations.

Refs #3054
houko added a commit that referenced this pull request Apr 26, 2026
…3054) (#3203)

* feat(types): UserConfig.budget for per-user spend limits

RBAC M5 introduces per-user spending caps so an operator can limit
individual users on top of the existing global / per-agent / per-provider
budgets.

UserConfig gains an optional UserBudgetConfig with the same hourly /
daily / monthly window structure as BudgetConfig plus an alert_threshold
(0..=1) shared with the per-agent path. None means "no per-user cap" —
the user is still bounded by the other layers.

Refs #3054

* feat(runtime): UserLogin/RoleChange/PermissionDenied/BudgetExceeded audit variants

Append four RBAC-themed variants to AuditAction so the new admin-only
endpoints in librefang-api can correlate denials and quota breaches
with the user / channel that triggered them.

The hash chain stays intact because the new variants only add fresh
rows — pre-existing entries hash identically (their action string is
unchanged). A regression test pins the Display output of every new
variant so a casual rename surfaces as a test failure rather than a
silent invalidation of every persisted hash.

Refs #3054

* feat(memory): per-user spend rollup via usage_events schema v23

RBAC M5 needs per-user budget detail and ranking endpoints. Adding
user_id / channel columns to usage_events lets the metering pipeline
attribute every LLM call to the caller (resolved from SenderContext
via auth.identify) so the dashboard can roll spend up by user without
scanning the whole table.

- Schema v23 migration adds NULL-able user_id / channel columns and
  indexes both (idx_usage_user_time, idx_usage_channel_time). Pre-M5
  rows return NULL — they fall outside any per-user filter, which is
  the right default since they pre-date attribution.
- UsageRecord gains optional user_id / channel fields plus an
  anonymous() helper for kernel-internal call sites that don't carry
  a SenderContext (background_skill_review, ephemeral side-questions).
- New UsageStore queries: query_user_hourly / _daily / _monthly and a
  query_user_ranking that aggregates all three windows in a single
  CASE-when round-trip.
- Kernel cost-attribution wiring: execute_llm_agent and the streaming
  spawn now resolve UserId from sender_context (via auth.identify)
  and stamp every UsageRecord with it. When check_all_and_record
  returns QuotaExceeded the kernel now also records a BudgetExceeded
  audit entry so an operator can correlate denials with users.

Refs #3054

* feat(api): audit query/export and per-user budget endpoints

Lands the four endpoints requested by RBAC M5:

- GET /api/audit/query — filter the hash-chained audit log by user,
  action, agent, channel, and ISO-8601 time range. Newest-first, hard
  cap 5000 rows. Filtering is in-memory (the AuditLog::recent path
  already verifies the chain on read), so SQL injection surface is
  zero.
- GET /api/audit/export?format=json|csv — same filters, streamed via
  axum::body::Body::from_stream so the kernel never buffers a full
  result set. CSV emits a fixed header row with RFC-4180 cell
  escaping pinned by a regression test.
- GET /api/budget/users — per-user cost ranking (top N) joined with
  the user's configured limits when present so the dashboard can
  render % bars without a follow-up call.
- GET /api/budget/users/{user_id} — single-user detail with hourly /
  daily / monthly spend, alert threshold, and an alert_breach flag.
  Accepts both the canonical UUID and the raw configured name
  (re-derived via UserId::from_name) so operators can paste either.

All four endpoints are admin-only and gate in-handler against
UserRole::Admin — they stay AUTHENTICATED (not on the is_public
allowlist). A 403 also fires a PermissionDenied audit event so an
operator can spot probes via /api/audit/query.

Middleware glue:
- AuthState gains an optional audit_log handle; when a user-scoped
  role check returns FORBIDDEN we now record PermissionDenied with
  the caller's UserId so the chain shows who tripped which path.
- The dirty middleware/config glue from PR #3196 (threading
  authenticated UserId through to record_with_context) is folded in
  unchanged.

OpenAPI registers all four new operations.

Refs #3054

* feat(api): thread authenticated UserId into runtime audit context

The /api/shutdown, /api/config/reload, and /api/config/set handlers now
accept the AuthenticatedApiUser extension and pass the caller's UserId
+ a 'api' channel through to record_with_context. Combined with the
RBAC M5 audit chain, an operator can now see exactly which user
triggered each ConfigChange entry instead of an anonymous 'system'
record.

This is the dirty middleware glue originally scoped under M2 — picked
up unchanged so the M5 endpoints have a stable contract for caller
attribution.

Refs #3054

* fix(security): RBAC M5 — clippy, CSV formula guard, per-user budget gap noted

Address review of #3203:

1) UsageRecord::anonymous's 8 positional args trip clippy too_many_arguments under -D warnings, breaking the Quality CI lane. The shape mirrors the metering schema 1:1 and a builder would push call-site noise into ~20 internal kernel paths without gaining type safety, so suppress locally with explanation.

2) csv_escape did not neutralise CSV-formula injection (CWE-1236). A username or detail string starting with =, +, -, @, TAB, or CR round-trips through Excel/Google Sheets as a live formula. OWASP-recommended mitigation: prepend an apostrophe inside the quoted cell. New test test_csv_escape_neutralises_formula_injection pins the behaviour for all six sentinels and confirms inner-position sentinels are untouched.

3) UserConfig.budget is read-only display data in this slice — the metering pipeline (check_all_and_record) only enforces global / per-agent / per-provider caps, not per-user. Add an inline TODO at the kernel call site so the gap is greppable; full per-user enforcement (lookup the resolved UserConfig via attribution_user_id, call a new metering.check_user_budget arm) lands in a follow-up. PR body updated separately to reflect this.

* fix(api): regen kernel_config_schema golden after UserConfig.budget added

M5 added the UserConfig.budget field to KernelConfig but the golden JSON schema fixture was not regenerated, breaking kernel_config_schema_matches_golden_fixture across Test/Ubuntu and Test/macOS.

* fix(security): RBAC M5 review follow-ups — fail-closed admin gate, parsed-instant time filter

Addresses code review on PR #3203:

- audit/budget admin gates: drop the "anonymous = Owner" loopback trust.
  `require_admin` and `require_admin_for_user_budget` now reject when no
  `AuthenticatedApiUser` is present, so a co-resident process at
  `127.0.0.1` (or `LIBREFANG_ALLOW_NO_AUTH=1` deploy) can no longer
  exfiltrate the hash-chained audit log or per-user spend without
  configuring an admin api_key.
- /api/audit/{query,export}: parse `from`/`to` as RFC-3339 instants
  instead of doing lexicographic string compare. Prior behaviour
  silently dropped entries when the operator passed `Z` and the entries
  were stored with `+00:00` (the chrono default), because `Z` (0x5A)
  > `+` (0x2B). Malformed bounds now return 400 instead of being
  ignored. Adds `test_time_range_normalises_z_and_offset_suffix` and
  `test_parse_time_bounds_rejects_garbage`.
- /api/budget/users/{id}: add `enforced: false` to the response so the
  M6 dashboard can warn that `alert_breach` is informational until the
  M5-followup enforcement arm in `kernel/mod.rs::execute_llm_agent`
  lands. Comment links the flag back to that TODO.
- usage::query_user_ranking: bind LIMIT via `rusqlite::params!` instead
  of `format!`-ing the clamped u32 into the SQL string. Value is a
  clamped u32 so injection isn't a real risk, but keeping the
  convention uniform avoids future copy-paste landing on
  user-controlled input. `None` maps to `LIMIT -1` (SQLite "no limit").
- Drop `_USER_SPEND_FRESHNESS_SECS` (referenced nowhere, just a
  comment) and the `#[allow(clippy::type_complexity)]` on the
  `Option<Arc<AuditLog>>` field (well below the threshold).
- Honesty pass on the export-streaming comment: the body is still
  materialised in memory before chunked transmission. Real lazy
  streaming would need `async-stream`; left for a follow-up.

Verification:
- cargo check -p librefang-api -p librefang-memory --lib → clean
- cargo test -p librefang-api --lib routes::audit::tests → 8/8 passed
- cargo test -p librefang-memory --lib usage → 14/14 passed
- cargo clippy -p librefang-api -p librefang-memory --lib --tests
  -- -D warnings → clean

* fix(rbac-m5): clamp UserBudgetConfig.alert_threshold + UTC doc accuracy

Re-review of #3203 surfaced two MEDIUM issues both fixed here:

1. `UserBudgetConfig.alert_threshold` is documented "clamped to 0..=1"
   but the field is bare `f64` and TOML parsing accepts any value. With
   no clamp, `alert_threshold = 5.0` made `alert_breach` permanently
   false (the dashboard never warned) and `-1.0` made it permanently
   true. `KernelConfig::clamp_bounds` now reins both ends back to
   [0.0, 1.0] and resets NaN to the default 0.8. New regression test
   `test_clamp_bounds_user_alert_threshold` covers above-1, below-0,
   NaN, and in-range cases.

2. `query_user_daily` / `query_user_monthly` doc-comments said "calendar
   day, server-local" but SQLite's `datetime('now', 'start of day')`
   returns UTC. Operators in non-UTC zones see "today" sliced on UTC
   midnight, not local midnight. Updated the per-user rollup module
   header to spell out the UTC contract (matching the pre-existing
   `query_global_*` and `query_agent_*` rollups) and the `_daily` /
   `_monthly` doclines now say UTC.

Also corrected the PR description: `audit_export` is described as
"streamed via from_stream so the kernel never buffers". The handler
DOES materialise the full filtered Vec before chunking — wire transfer
is progressive but kernel-side memory is O(filtered_rows). Updated PR
body to reflect that, with a follow-up note that switching to
per-row pull from `AuditLog::recent` is the path to true streaming.

* test(rbac-m5): integration tests for admin-only audit/budget endpoints

Closes the integration-test gap called out in PR #3203 review:
the four new HTTP endpoints (`/api/audit/query`, `/api/audit/export`,
`/api/budget/users`, `/api/budget/users/{id}`) had unit tests for
`apply_filter` / `csv_escape` / `parse_time_bounds` in isolation but
no test that exercised the full request → middleware → handler →
`require_admin` pipeline. That gap matters because:

- The middleware role-gate (`user_role_allows_request`) lets every
  authenticated GET through regardless of role; the in-handler
  `require_admin` is the *only* thing stopping a Viewer from reading
  the hash-chained audit log. A future refactor that drops it would
  silently expose the chain.
- The `a612c09b` follow-up made the gate fail-closed for anonymous
  callers in `LIBREFANG_ALLOW_NO_AUTH=1` deployments — without an
  end-to-end test, that fix could be silently reverted.

Adds `start_test_server_with_rbac_users` helper that wires:
- `KernelConfig.users` AND `AuthState.user_api_keys` from a list of
  `(name, role, api_key)` tuples (mirrors `server.rs::build_router`).
- The audit + budget routers under `/api/`.
- `AuthState.audit_log = Some(kernel.audit().clone())` so denials
  hit the same hash chain as production.
- `allow_no_auth = true` so anonymous-rejection tests can synthesize
  Bearer-less requests without 401-ing at the middleware layer.

Tests:
- `test_audit_query_rejects_anonymous_403` — pins the fail-closed
  admin gate from `a612c09b`.
- `test_audit_query_rejects_viewer_admin_returns_200` — pins the
  in-handler `require_admin` boundary; Viewer 403, Admin 200 with
  documented JSON shape (`entries[]`, `count`, `limit`).
- `test_audit_export_csv_emits_documented_headers` — pins
  `Content-Type: text/csv` + `Content-Disposition: attachment;
  filename="audit.csv"` + the canonical CSV header row schema, so
  the dashboard download flow stays compatible.
- `test_user_budget_detail_includes_enforced_false` — pins the M6
  dashboard contract that `enforced` stays `false` until per-user
  budget enforcement (the M5-followup arm in
  `kernel/mod.rs::execute_llm_agent`) actually lands.

`cargo check -p librefang-api --tests` clean.

* fix(rbac-m3): close 2 review gaps — case-insensitive PII label, validate flag-without-namespace combos

PR #3205 (M3) shipped two paper cuts that this commit closes. Both
land on the M5 branch because M5 is already touching the same files
(`crates/librefang-types/src/config/validation.rs` for the
`UserBudgetConfig` clamp, `crates/librefang-memory/src/namespace_acl.rs`
unchanged but in scope) and a separate PR would conflict with no
benefit.

1. **`MemoryNamespaceGuard::has_pii_label` is now case-insensitive.**
   The metadata-stored label is the `Display` form of `TaintLabel::Pii`
   (literal `"Pii"`), but external writers commonly hand-stamp `"PII"`
   or `"pii"`. The previous exact-match comparison silently skipped
   those, leaving structured PII fragments to fall through to the
   regex backstop — fine for free-form text but a leak for custom
   field names that have no e-mail/phone/SSN tokens to grep. Both the
   array and scalar-string metadata shapes now lowercase-normalise the
   target *and* candidate before comparing.

2. **`KernelConfig::validate()` now warns on `pii_access` /
   `export_allowed` / `delete_allowed` set with empty
   `readable_namespaces`.** The runtime guard requires read access
   for every code path those flags gate (PII redaction only runs on
   readable items; export/delete check write access AND the flag).
   An admin who toggles a flag without declaring namespaces gets a
   silent no-op that looks like it works — exactly the
   misconfiguration `validate()` exists to surface. Warning names the
   user, lists the flags, and suggests the missing key.

Tests:
- `redact_via_metadata_label_matches_case_insensitively` covers
  `"PII"` / `"pii"` / `"Pii"` for both array and scalar-string
  metadata shapes.
- `test_validate_warns_on_memory_access_flags_without_readable_namespaces`
  pins the warning shape: warns on the typo'd user, suppresses on a
  correctly-configured user, names both the user and the flag.

`cargo test -p librefang-memory --lib namespace_acl`: 9/9.
`cargo test -p librefang-types --lib config`: 139/139 plus the new
validate test.

(A third proposed item — startup warning when memory routes are in
the auth allowlist while users have memory_access policies — turned
out to be moot: `/api/memory/*` is not in any allowlist, the only
fallback path is the loopback bypass which is deliberate and
documented in `guard_for_request`.)

* feat(security): RBAC M5 — wire per-user budget enforcement

Closes the M5-followup TODO that left UserConfig.budget as read-only
display data. Per-user spend caps now actually deny over-budget calls,
matching the global / per-agent / per-provider enforcement semantics.

Surface:
- AuthManager: UserIdentity gains an optional UserBudgetConfig (cloned
  from UserConfig.budget at populate time). New `budget_for(user_id)`
  getter mirrors `memory_acl_for` / `user_policy`.
- MeteringEngine: new `check_user_budget(user_id, &user_budget)`. Each
  window with `0.0` is treated as unlimited. On breach returns
  `QuotaExceeded` with the user id in the message so dashboard / log
  triage can grep by uid.
- kernel::execute_llm_agent (sync) and the streaming spawn now run
  the per-user check after `check_all_and_record` succeeds. Same
  post-call semantics as the existing global / agent / provider arms
  — a breach trips a BudgetExceeded audit (already attribution-tagged
  with the user / channel) so the next call from this user gets denied
  at the gate; the current response is returned unchanged because the
  tokens are already billed.
- /api/budget/users/{id} flips `enforced` from false to true; the
  earlier `enforced=false` was a UX guard while enforcement was
  deferred. Comment now explains the cap-to-denial chain.

Tests:
- 4 new tests in librefang-kernel-metering covering under-limit,
  zero-means-unlimited, hourly breach with uid in message, and
  user isolation (Bob's spend doesn't trigger Alice's gate).

Verification:
- cargo check -p librefang-kernel-metering -p librefang-kernel
  -p librefang-api --lib → clean
- cargo test -p librefang-kernel-metering --lib check_user_budget → all passing
- cargo clippy -p librefang-kernel-metering -p librefang-kernel
  -p librefang-api --lib --tests -- -D warnings → clean

* fix(rbac-m5): add missing budget field to UserConfig literal in M4 channel role test

Auto-merge missed adding `budget: None` to the `UserConfig` literal at
`auth.rs:1383` (the M4 `channel_role_explicit_user_config_wins` test).
Three other UserConfig literals in the same file (lines 852/868/883)
and the `user_with_policy` helper at 1047 already carried the field
from earlier merges; this one slipped through because it was added by
M4 after M5's field addition fanned out to its sibling literals.

CI failure: E0063 "missing field `budget` in initializer` on Test /
Ubuntu / Windows / macOS / Quality.

Caught by Test / Windows in run 24951702471.

* fix(rbac-m5): impl Default for UserConfig so test fixtures can ..Default::default()

CI Test/Ubuntu (run 24952194471) failed on
crates/librefang-api/tests/api_integration_test.rs:2565 — the new
RBAC integration test fixture `start_test_server_with_rbac_users`
uses `..Default::default()` to fill the optional RBAC fields, but
UserConfig had no Default impl.

The integration test file is a separate compilation unit from
`--lib --tests` runs targeting only the lib's unit tests, which is
why my prior local verification missed it. CI's full
`cargo test -p librefang-api` caught it.

Add a hand-written `impl Default for UserConfig` that mirrors the
per-field `#[serde(default)]` attributes — `name = ""`, `role =
default_role()` ("user"), all RBAC `Option`s = `None`, both
`HashMap`s empty. Now `serde::from_str("name = \"x\"")` and
`UserConfig { name: ..., ..Default::default() }` produce identical
values.

Verification:
- cargo test -p librefang-api --tests → all passing
- cargo clippy -p librefang-api -p librefang-types --all-targets
  -- -D warnings → clean

* fix(rbac-m5): add budget field to UserConfig literals after M6 merge

PR #3209 (RBAC M6 dashboard) landed in main and added
crates/librefang-api/src/routes/users.rs with three UserConfig
literals — create, update, CSV import — that predate the per-user
budget field this PR added to UserConfig. Compile breaks with E0063
"missing field `budget`" when the two land together.

- create_user / import_users new-row: budget=None (no per-user cap;
  global/per-agent/per-provider caps still apply). Dedicated
  /api/users/{name}/budget owns budget edits.
- update_user / import_users update-row: preserve from existing config
  alongside the M3 policy fields, same reasoning — the M6 dashboard
  doesn't surface budget yet, clobbering it on a name/role edit would
  silently wipe a user's spend cap.

Also adds budget=None to the seed in users_test.rs so the existing
M6 preservation test still compiles.

* test(rbac-m5): pin budget preservation across PUT and CSV reimport

Catches the latent bug in users.rs:568 where `..new_u.clone()` would
silently overwrite an existing user's `budget` with `None` because CSV
rows always carry no budget. Mirrors the existing M3 policy preservation
test (`users_update_and_import_preserve_rbac_m3_policy_fields`).

Seeds Carol with a non-zero UserBudgetConfig, then:
1. PUT /api/users/Carol — rename + role bump, no budget in body. Asserts
   the budget round-trips.
2. POST /api/users/import — same name, no budget in CSV row. Asserts the
   budget still round-trips through the import-update branch.

Both assertions name the failure mode in the message so a future
regression points at the exact line that broke.

* test(rbac-m5): unstale two assertions hidden by earlier compile errors

Both tests landed earlier in the M5 stack but were never actually
exercised by CI — the UserConfig.budget compile errors after the M6
merge masked them. Now that the literals compile, the tests run and
fail against the current behavior, which is correct in both cases.

1) test_audit_query_rejects_anonymous_403 → ..._anonymous (asserts 401)
   The setup asks for `allow_no_auth = true` to bypass middleware, but
   middleware.rs:501-526 only honors that flag when both `api_key` and
   `user_api_keys` are empty. The test configures both, so anonymous
   requests are 401'd at the middleware before reaching the in-handler
   `require_admin` 403 gate. Earlier rejection is the stricter outcome
   ("anonymous cannot read audit" still holds).

2) test_user_budget_detail_includes_enforced_false → ..._true
   Per-user budget enforcement landed in 4a00a64 ("RBAC M5 — wire
   per-user budget enforcement") — AuthManager::budget_for,
   MeteringEngine::check_user_budget, and the post-call arm in
   kernel::execute_llm_agent now actually deny over-budget calls. The
   handler was updated to report `enforced: true` accordingly; the
   stale assertion was missed.

No production changes — only the assertions move to track the
already-shipped behavior.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@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

area/kernel Core kernel (scheduling, RBAC, workflows) area/runtime Agent loop, LLM drivers, WASM sandbox size/L 250-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant