feat(reborn): admin user-management API and UI - #5779
Conversation
Add an end-to-end admin user-management surface to the Reborn stack, built on the existing StoredUser records in ironclaw_reborn_identity (no new user store — see .claude/rules/discovery-claims.md for the planning miss this corrects). Layers: - identity: new RebornUserDirectory trait (list/get/create/update/ status/role/last-login/delete-cascade/count-active-admins) on the same FilesystemRebornIdentityStore, kept separate from the resolver so admin CRUD can't perturb mint/link invariants. delete_user cascades over external-identity + verified-email records. CONTRACT.md documents the three persisted record shapes. - product_workflow: RebornServicesApi admin_* methods with caller authorization (admin/owner role or env-bearer operator) and last-admin protection; new AdminUserService port + DTOs. - composition: admin_user_directory / admin_secrets / admin_token adapters; AdminApiTokenMinter port for minting the one-time API bearer on user-create. Mount now grants list+delete on /tenant-shared for the identity delete cascade. - webui_v2: admin_users REST routes (GET/POST /admin/users, GET/PATCH/DELETE /admin/users/:id, status, role, GET/PUT/DELETE per-user secrets) + descriptors (body/rate limits). - serve: wires a signed-session-store-backed minter (365-day API bearer that validates under the SSO login surface's own store). - frontend: un-hide the admin nav + Users tab, wire admin-api.js to the real endpoints. Tests: identity-store unit tests, product_workflow contract tests (authz + last-admin + one-time token), webui_v2 descriptor contract, and composition HTTP e2e (full lifecycle + API-token login + last-admin over HTTP). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
⏳ IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
github-actions
Bot
added
the
risk: low
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a WebChat v2 admin user-management surface end-to-end: filesystem-backed user records and delete cascades, a fail-closed admin service and composition wiring, CLI/session-token minting, HTTP routes/handlers, frontend client/UI updates, and real-runtime contract/E2E coverage. ChangesAdmin user management
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Verdict: ❌ Changes requested
Findings: 1 blocking / 0 notes
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Head: f4fda265d016f01d7aae0bee68817f1f4eeba50b
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Found a blocking authorization regression in the new admin user-management surface.
Findings
1. ❌ [HIGH] Suspended admins still pass admin authorization
Location: crates/ironclaw_product_workflow/src/reborn_services.rs:2889
authorize_admin grants access to any persisted admin/owner role without checking AdminUserStatus::Active. The new status endpoint and last-admin logic treat suspension as removing an active admin, but a suspended admin's existing bearer can still call every admin API, including creating users, changing roles, and provisioning secrets. Require user.status == AdminUserStatus::Active in this authorization check, and add a caller-level test that suspending an admin immediately causes admin routes to return 403.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas. - Use
@ironloopai statusto check queued/running/completed/stale/stalled state while reviewers run.
| .await | ||
| .map_err(map_admin_user_error)?; | ||
| match record { | ||
| Some(user) if user.role.is_admin() => Ok(()), |
There was a problem hiding this comment.
This should also require user.status == AdminUserStatus::Active. As written, suspending an admin only changes the count used by last-admin protection; the suspended admin's existing bearer still clears this role-only check and can continue using all admin routes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive admin user-management surface, enabling user CRUD, status/role updates, and per-user secret provisioning. It defines the AdminUserService port, implements it via a composition adapter over the identity directory and secret provisioner, exposes the corresponding HTTP endpoints in the WebUI v2 router, and wires up the admin Users tab in the frontend. Feedback on these changes highlights a performance concern in list_users due to O(N) directory scans, a potential 500 Internal Server Error caused by a lack of secret handle validation at the HTTP boundary, and a bug in the frontend API client where simultaneous role and profile updates are partially ignored.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async fn list_users( | ||
| &self, | ||
| tenant_id: &TenantId, | ||
| status: Option<RebornUserStatus>, | ||
| ) -> Result<Vec<RebornUser>, RebornIdentityError> { |
There was a problem hiding this comment.
Performance & Scalability Concern: O(N) Filesystem Reads on Listing
Since user records are not tenant-partitioned in the filesystem path, list_users must list the entire global users/ directory and perform a separate read_record (filesystem read and JSON deserialization) for every single user in the system just to filter by tenant_id and status.
As the total number of users across all tenants grows, this will become a severe performance bottleneck and will not scale.
Recommendation:
Optimize filesystem-backed listing operations by using index-based queries to avoid O(N) directory scans. Maintain an index file that directly lists the paths to the relevant items to avoid costly directory traversal and scanning. Implement a fallback to directory scanning only when the backend explicitly reports the index operation as unsupported.
References
- To optimize list operations that involve scanning multiple directories and files, maintain an index file that directly lists the paths to the relevant items. This avoids costly directory traversal and scanning.
- Optimize filesystem-backed listing operations by using index-based queries to avoid O(N) directory scans. Implement a fallback to directory scanning only when the backend explicitly reports the index operation as unsupported.
There was a problem hiding this comment.
Addressed in b5e46e5 with bounded pagination (limit+cursor, default 100 / max 200) through the port, facade, directory, and JS client; the directory reads at most limit matching records per page instead of an O(N) scan-and-allocate. A tenant/status-partitioned index (to also bound the directory listing itself) is noted as a follow-up.
| // The handle is validated at the HTTP edge (webui_v2 descriptor/handler); | ||
| // a construction failure here is an internal inconsistency. | ||
| let handle = SecretHandle::new(&handle).map_err(|_| AdminUserError::Internal)?; |
There was a problem hiding this comment.
Incorrect Assumption & Potential 500 Error
The comment states that the handle is validated at the HTTP edge, but admin_put_user_secret and admin_delete_user_secret in ironclaw_webui_v2/src/handlers.rs accept handle as a raw String without any validation.
As a result, if a user passes an invalid handle in the URL path, SecretHandle::new will fail here and return AdminUserError::Internal, which the facade maps to a 500 Internal Server Error instead of a 400 Bad Request.
Recommendation:
Validate security-sensitive inputs and domain-specific types at the boundary level (e.g., the HTTP edge in ironclaw_webui_v2/src/handlers.rs) using a validating constructor like SecretHandle::new and return a proper 400 Bad Request validation error if it fails. This keeps constructors downstream infallible and centralizes validation logic at the boundary.
References
- Validate security-sensitive inputs like service base URLs at the factory or boundary level rather than in individual constructors to keep constructors infallible and centralize security logic.
- When constructing a domain-specific type from an external, untrusted source, use a validating constructor instead of a trusted constructor to ensure data is canonicalized and validated at the boundary.
There was a problem hiding this comment.
| export async function updateAdminUser(id, payload) { | ||
| if (payload && Object.prototype.hasOwnProperty.call(payload, "role")) { |
There was a problem hiding this comment.
Potential Bug: Partial Update Ignored
If payload contains both role and other profile fields (like display_name or metadata), the if block will match and return after updating only the role, silently ignoring the other updates.
While the UI may only send one or the other today, this is a fragile assumption that can easily lead to silent update failures if the UI is modified in the future.
Recommendation:
If both are present, either perform both requests (e.g., using Promise.all) or explicitly handle/reject the combined payload to prevent silent data loss.
There was a problem hiding this comment.
Acknowledged. The admin UI only ever sends { role } OR { display_name / metadata } today — never both — and the router-by-payload keeps that contract explicit. The backend also splits these into distinct endpoints (POST /role vs PATCH /). We're leaving the single-key routing as-is rather than adding speculative combined-update handling; if the UI ever sends both, the right fix is at that call site. Not changing in this pass.
There was a problem hiding this comment.
Pull request overview
This PR adds an end-to-end admin user-management surface to the Reborn stack, wiring new admin CRUD + per-user secret provisioning + one-time API token minting through identity → product_workflow → composition → webui_v2 → serve → frontend.
Changes:
- Introduces an identity-level
RebornUserDirectoryover persistedStoredUserrecords, including delete cascade and active-admin counting. - Adds a product-workflow
AdminUserServiceport + facade methods with role/operator authorization and last-admin protection. - Wires new WebUI v2 admin routes and frontend admin users tab, plus serve-layer token minting via a signed session store.
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rs | Extends route descriptor contract to cover new admin endpoints. |
| crates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.js | Replaces stub admin client with real v2 /admin/users* calls + secrets APIs. |
| crates/ironclaw_webui_v2/static/js/pages/admin/hooks/useAdminUsers.js | Hooks updated to integrate one-time token behavior and admin CRUD mutations. |
| crates/ironclaw_webui_v2/static/js/pages/admin/admin-page.js | Makes Users the default admin tab and fallback route. |
| crates/ironclaw_webui_v2/static/js/app/routes.js | Un-hides admin nav and routes only the Users tab. |
| crates/ironclaw_webui_v2/src/router.rs | Mounts the new admin routes (users + role/status + secrets). |
| crates/ironclaw_webui_v2/src/lib.rs | Re-exports new admin route IDs for consumers/tests. |
| crates/ironclaw_webui_v2/src/handlers.rs | Adds admin HTTP handlers delegating to RebornServicesApi. |
| crates/ironclaw_webui_v2/src/descriptors.rs | Defines admin route IDs/patterns and descriptors (limits, auth, audit). |
| crates/ironclaw_reborn_webui_ingress/src/signed_session_login.rs | Exposes a deterministic signed session-store constructor for token mint/verify. |
| crates/ironclaw_reborn_webui_ingress/src/lib.rs | Re-exports signed_session_store for serve/tests. |
| crates/ironclaw_reborn_identity/src/user_directory.rs | Defines the admin-facing directory trait and domain types. |
| crates/ironclaw_reborn_identity/src/lib.rs | Exports directory types and adds UserNotFound error. |
| crates/ironclaw_reborn_identity/src/filesystem_store/tests.rs | Adds unit tests covering directory CRUD, delete cascade, and legacy record defaults. |
| crates/ironclaw_reborn_identity/src/filesystem_store/record.rs | Extends persisted StoredUser shape with status/role/tenant/metadata/back-compat defaults. |
| crates/ironclaw_reborn_identity/src/filesystem_store/paths.rs | Adds helpers for enumerating users directory and walking external identity trees. |
| crates/ironclaw_reborn_identity/src/filesystem_store/directory.rs | Implements RebornUserDirectory over the filesystem store, including CAS updates and cascade delete. |
| crates/ironclaw_reborn_identity/src/filesystem_store.rs | Writes new user-record fields on SSO login. |
| crates/ironclaw_reborn_identity/CONTRACT.md | Documents persisted shapes and the new user-directory surface/invariants. |
| crates/ironclaw_reborn_composition/tests/admin_api_e2e.rs | Adds HTTP e2e tests for the composed admin surface + token login. |
| crates/ironclaw_reborn_composition/src/webui.rs | Wires AdminUserService into webui services when all dependencies are present. |
| crates/ironclaw_reborn_composition/src/test_support/local_dev_boot.rs | Updates test helper for new secret-store factory return shape. |
| crates/ironclaw_reborn_composition/src/runtime.rs | Adds runtime accessors for user directory, admin secret provisioner, and token minter. |
| crates/ironclaw_reborn_composition/src/runtime_input.rs | Adds optional admin token-minter input to runtime construction. |
| crates/ironclaw_reborn_composition/src/lib.rs | Expands mount permissions and exposes the admin token-minter port. |
| crates/ironclaw_reborn_composition/src/factory.rs | Returns secret-store crypto for admin provisioning and wires the admin secret provisioner. |
| crates/ironclaw_reborn_composition/src/admin_user_directory.rs | Composition adapter implementing product-workflow AdminUserService over identity + secrets + token minting. |
| crates/ironclaw_reborn_composition/src/admin_token.rs | Defines the AdminApiTokenMinter port used by composition/serve. |
| crates/ironclaw_reborn_composition/src/admin_secrets.rs | Implements admin per-user secret provisioning by building target-scoped secret stores. |
| crates/ironclaw_reborn_composition/Cargo.toml | Adds dev-deps needed for admin e2e (ingress + chrono + secrecy). |
| crates/ironclaw_reborn_cli/src/commands/serve.rs | Wires a session-store-backed admin API token minter into serve. |
| crates/ironclaw_product_workflow/tests/reborn_services_contract.rs | Adds contract tests for admin authorization, operator bypass, and last-admin protection. |
| crates/ironclaw_product_workflow/src/reborn_services/admin_users.rs | Introduces the admin port + HTTP wire DTOs + fail-closed default service. |
| crates/ironclaw_product_workflow/src/reborn_services.rs | Wires the port into RebornServices and implements admin facade methods. |
| crates/ironclaw_product_workflow/src/lib.rs | Re-exports admin types from reborn_services. |
| Cargo.lock | Adds ironclaw_reborn_webui_ingress to composition dev dependency graph. |
| .claude/rules/discovery-claims.md | Adds a new agent rule documenting evidence standards for load-bearing discovery claims. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // list + delete are needed by the Reborn identity store's admin | ||
| // user-directory: enumeration (`list_users`) and the delete cascade | ||
| // (removing a user's identity/verified-email records) live under | ||
| // `/tenant-shared/reborn-identity/…`. | ||
| MountPermissions::read_write_list_delete(), |
There was a problem hiding this comment.
Fixed in b5e46e5. /tenant-shared now grants only read+write+list (no delete); delete authority is scoped to a new /tenant-shared/reborn-identity sub-mount. Longest-prefix mount matching routes identity paths (which need the delete cascade) to the full grant and everything else to the delete-less one, so a compromised tenant-shared writer can't delete across unrelated subtrees.
| // Every handler delegates straight to the facade, which enforces admin | ||
| // authorization (operator token or admin/owner role) and last-admin protection. | ||
| // The `{user_id}` path segment is parsed into a `UserId` here so a malformed id | ||
| // is a 400 before the facade runs; the `{handle}` segment stays a String and is | ||
| // validated deeper (the secret store rejects a bad handle). |
There was a problem hiding this comment.
Fixed in b5e46e5. The {handle} segment is now parsed into SecretHandle at the HTTP edge (parse_admin_secret_handle → 400 on a malformed handle) and threaded as the typed value through the facade + port; the composition adapter no longer re-validates a raw string, so a bad handle is a sanitized 400, never a downstream 500.
| let user_id = parse_admin_user_id(user_id)?; | ||
| Ok(Json( | ||
| state | ||
| .services() | ||
| .put_admin_user_secret(caller, user_id, handle, body) | ||
| .await?, | ||
| )) |
There was a problem hiding this comment.
Fixed in b5e46e5. The {handle} segment is now parsed into SecretHandle at the HTTP edge (parse_admin_secret_handle → 400 on a malformed handle) and threaded as the typed value through the facade + port; the composition adapter no longer re-validates a raw string, so a bad handle is a sanitized 400, never a downstream 500.
| let user_id = parse_admin_user_id(user_id)?; | ||
| Ok(Json( | ||
| state | ||
| .services() | ||
| .delete_admin_user_secret(caller, user_id, handle) | ||
| .await?, | ||
| )) |
There was a problem hiding this comment.
Fixed in b5e46e5. The {handle} segment is now parsed into SecretHandle at the HTTP edge (parse_admin_secret_handle → 400 on a malformed handle) and threaded as the typed value through the facade + port; the composition adapter no longer re-validates a raw string, so a bad handle is a sanitized 400, never a downstream 500.
| suspendUser: suspendMut.mutateAsync, | ||
| activateUser: activateMut.mutateAsync, | ||
| createToken: (userId, name) => tokenMut.mutateAsync({ userId, name }), |
There was a problem hiding this comment.
Fixed in b5e46e5. The re-issue token controls were removed from the UI (no re-issue endpoint exists), so no caller awaits the rejecting createUserToken anymore. The dead createToken/newToken wiring was dropped from the hook.
|
Nice work on the authorization design here — One process note per The gap I'd actually flag as worth closing: tenant isolation of the admin surface is only proven at the raw |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_product_workflow/src/reborn_services.rs`:
- Around line 2998-3022: The admin write paths allow any caller that passes
authorize_admin() to create or assign an Owner role, so add a role-ceiling check
before the mutations in create_admin_user and set_admin_user_role. Use the
existing AdminUserRole and role conversion helpers (role_to_identity /
role_from_identity) to compare the caller’s admin role against the requested
target role, and reject any attempt to mint or promote Owner unless the caller
is explicitly allowed. Keep the guard close to the admin_users.create_user and
role update logic so both user creation and role changes enforce the same
hierarchy rule.
- Around line 2915-2943: The last-admin guard in ensure_not_last_admin is still
racy because it does a tenant scan after a per-user CAS path, so concurrent
demotions/deletes can both succeed. Move the protection into a single atomic
tenant-scoped identity-layer operation, or serialize the read+write with a
tenant-level lock around the whole mutation path. Update the
ensure_not_last_admin flow and the related admin_users
count_active_admins/cas_update interaction so the check and write cannot
interleave across replicas.
In `@crates/ironclaw_product_workflow/src/reborn_services/admin_users.rs`:
- Around line 96-109: AdminUserError is a plain enum without
Display/std::error::Error support, which makes it inconsistent with the rest of
the error types. Update the AdminUserError type in admin_users.rs to use
thiserror with per-variant #[error(...)] messages while keeping the coarse
taxonomy intact, and preserve the existing variants NotFound, Unavailable, and
Internal. If needed, adjust any call sites in the admin user facade/adapter that
rely on formatting or conversion so they continue mapping errors with context
cleanly.
In `@crates/ironclaw_product_workflow/tests/reborn_services_contract.rs`:
- Around line 11626-11633: The `delete_user` coverage is missing the sole-admin
case, so add a test alongside the existing `demote_user` and `suspend_user`
last-admin checks that exercises the facade’s delete path for the final
remaining admin. Update the `reborn_services_contract` test flow to attempt
deleting the only admin through the facade and assert the last-admin guard
blocks it, using the same setup/helpers already used around `delete_user`,
`demote_user`, and `suspend_user`.
In `@crates/ironclaw_reborn_cli/src/commands/serve.rs`:
- Around line 201-213: The admin bearer minting setup in serve() is creating
admin_session_store from session_signing_secret unconditionally, but the 32-byte
entropy check is still only gated by sso_startup.is_some(). Make the
entropy-floor validation unconditional, or move it directly beside
with_admin_api_token_minter(...) so SignedSessionTokenMinter cannot be wired
unless the operator secret is strong enough.
In `@crates/ironclaw_reborn_composition/src/admin_secrets.rs`:
- Around line 77-104: The admin secret mutation path in
AdminSecretProvisioner::store_for is being reached from admin_user_directory
without verifying that the target user belongs to the requested tenant. Update
the list_secrets, put_secret, and delete_secret flows to reuse
tenant_scoped_user or otherwise check the fetched RebornUser.tenant_id before
calling AdminSecretProvisioner, so secret reads/writes/deletes are only allowed
for the owning tenant.
In `@crates/ironclaw_reborn_composition/src/admin_token.rs`:
- Line 20: The admin token mint port currently returns String from mint, which
drops typed error context and violates the Rust error conventions used here.
Update the admin_token::mint signature to return a small thiserror-based error
type instead of String, then ensure the adapter layer maps that error into
AdminUserError with context; use the mint method and AdminUserError as the key
symbols when updating the composition boundary.
In `@crates/ironclaw_reborn_composition/src/admin_user_directory.rs`:
- Around line 104-125: The admin user flow in create_user currently persists the
user before token minting, then returns AdminUserError::Internal on mint
failure, leaving a tokenless account behind. Update the logic around create_user
and token_minter.mint so failures are compensated with a rollback/deletion of
the newly created user (or otherwise avoid committing the user until mint
succeeds), and keep the error mapping/logging behavior in place for the mint
failure path.
- Around line 132-180: The tenant-scoped mutation methods in AdminUserDirectory
are relying on the facade precheck and ignoring the tenant argument, which can
allow cross-tenant updates if that precheck is bypassed. Update update_profile,
set_status, and set_role to enforce tenant ownership inside the port before
calling directory.update_profile, directory.update_status, and
directory.update_role. Use the existing user lookup/validation path in this
module (or add an equivalent tenant match guard) so each mutation verifies the
UserId belongs to the provided TenantId before applying changes.
In `@crates/ironclaw_reborn_composition/src/lib.rs`:
- Around line 797-804: The MountGrant in the tenant-shared mount setup is too
broad because it gives list/delete to the entire /tenant-shared subtree via
invocation_mount_view callers. Narrow this by keeping the existing
/tenant-shared grant least-privileged and adding a separate admin-only grant or
mount view for /tenant-shared/reborn-identity to cover the identity-store admin
operations. Update the mount-building logic around MountGrant::new,
MountAlias::new, and MountPermissions::read_write_list_delete so only the
identity-specific path gets the extra permissions.
In `@crates/ironclaw_reborn_composition/tests/admin_api_e2e.rs`:
- Around line 465-501: The last-admin protection test in
admin_last_admin_protection_over_http only covers demotion and suspension, not
the delete path. Extend this test using AdminApiDriver::delete_user on the sole
admin and assert it returns StatusCode::CONFLICT, matching the existing
last_admin behavior checked via set_role and set_status. If delete_user is meant
to bypass ensure_not_last_admin, add a clear comment or documentation in
reborn_services.rs explaining that exemption.
In `@crates/ironclaw_reborn_identity/CONTRACT.md`:
- Around line 50-54: Update the persisted-records table in CONTRACT.md so the
`StoredUser` row matches the actual `StoredUser` schema used by
`filesystem_store/directory.rs`. Add the newly persisted fields (`status`,
`role`, `created_by`, `last_login_at`, `tenant_id`, `metadata`) to the
`StoredUser` field list, and keep the row aligned with the record definition so
the contract documentation reflects the current storage shape.
In `@crates/ironclaw_reborn_identity/src/filesystem_store/record.rs`:
- Around line 34-38: The legacy tenant handling for `tenant_id` is too
permissive because `None` currently gets treated as valid for every tenant.
Update the `RebornUserDirectory::list_users` path and the `record.rs`
`tenant_id` fallback so `None` is only accepted when the deployment is truly
single-tenant; otherwise fail closed or exclude those rows from admin
enumeration. If needed, gate the `Option<String>` fallback with an explicit
single-tenant check instead of unconditionally mapping `None` to true.
In `@crates/ironclaw_webui_v2/src/handlers.rs`:
- Around line 196-205: The `parse_admin_user_id` helper is discarding the
concrete `UserId::new(raw)` validation error by using `map_err(|_| ...)`. Update
this mapping so the error binding from `UserId::new` is preserved either by
carrying the cause into the `WebUiInboundValidationError`/`RebornServicesError`
chain or by logging the bound error before converting it to `WebUiV2HttpError`.
Keep the existing `parse_admin_user_id` flow and `UserId::new` call, but avoid
substituting a generic `InvalidId` without the original reason.
In `@crates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.js`:
- Around line 46-62: `updateAdminUser` currently returns immediately when
`payload.role` is present, so any accompanying `display_name` or `metadata` is
silently ignored. Update the branching in `updateAdminUser` to either reject
mixed payloads up front or send all supported fields through the appropriate
request path, and keep the behavior explicit rather than dropping data. Use the
existing `apiFetch`, `normalizeUser`, and the role/PATCH routes in
`admin-api.js` to locate and fix the mixed-payload handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c793fdfd-b35b-4e26-868c-5b54c1227884
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (36)
.claude/rules/discovery-claims.mdcrates/ironclaw_product_workflow/src/lib.rscrates/ironclaw_product_workflow/src/reborn_services.rscrates/ironclaw_product_workflow/src/reborn_services/admin_users.rscrates/ironclaw_product_workflow/tests/reborn_services_contract.rscrates/ironclaw_reborn_cli/src/commands/serve.rscrates/ironclaw_reborn_composition/Cargo.tomlcrates/ironclaw_reborn_composition/src/admin_secrets.rscrates/ironclaw_reborn_composition/src/admin_token.rscrates/ironclaw_reborn_composition/src/admin_user_directory.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime_input.rscrates/ironclaw_reborn_composition/src/test_support/local_dev_boot.rscrates/ironclaw_reborn_composition/src/webui.rscrates/ironclaw_reborn_composition/tests/admin_api_e2e.rscrates/ironclaw_reborn_identity/CONTRACT.mdcrates/ironclaw_reborn_identity/src/filesystem_store.rscrates/ironclaw_reborn_identity/src/filesystem_store/directory.rscrates/ironclaw_reborn_identity/src/filesystem_store/paths.rscrates/ironclaw_reborn_identity/src/filesystem_store/record.rscrates/ironclaw_reborn_identity/src/filesystem_store/tests.rscrates/ironclaw_reborn_identity/src/lib.rscrates/ironclaw_reborn_identity/src/user_directory.rscrates/ironclaw_reborn_webui_ingress/src/lib.rscrates/ironclaw_reborn_webui_ingress/src/signed_session_login.rscrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/src/lib.rscrates/ironclaw_webui_v2/src/router.rscrates/ironclaw_webui_v2/static/js/app/routes.jscrates/ironclaw_webui_v2/static/js/pages/admin/admin-page.jscrates/ironclaw_webui_v2/static/js/pages/admin/hooks/useAdminUsers.jscrates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.jscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rs
| async fn create_admin_user( | ||
| &self, | ||
| caller: WebUiAuthenticatedCaller, | ||
| request: RebornAdminCreateUserRequest, | ||
| ) -> Result<RebornAdminUserCreatedResponse, RebornServicesError> { | ||
| self.authorize_admin(&caller).await?; | ||
| let created = self | ||
| .admin_users | ||
| .create_user( | ||
| &caller.tenant_id, | ||
| &caller.user_id, | ||
| AdminCreateUserFields { | ||
| email: request.email, | ||
| display_name: request.display_name, | ||
| role: request.role, | ||
| }, | ||
| ) | ||
| .await | ||
| .map_err(map_admin_user_error)?; | ||
| Ok(RebornAdminUserCreatedResponse { | ||
| user: created.record, | ||
| // Exposed exactly once, here. The DTO carries it in no other path. | ||
| api_token: created.api_token.expose_secret().to_string(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- reborn_services.rs around target lines ---'
sed -n '2960,3095p' crates/ironclaw_product_workflow/src/reborn_services.rs
echo
echo '--- search AdminUserRole and admin auth helpers ---'
rg -n "enum AdminUserRole|impl AdminUserRole|is_admin\(|authorize_admin|set_admin_user_role|create_admin_user|Owner|owner" crates/ironclaw_product_workflow/src/reborn_services.rs crates/ironclaw_product_workflow/src -g '*.rs'
echo
echo '--- broader search for owner-only checks in product workflow ---'
rg -n "Owner|is_owner|role.*Owner|AdminUserRole::Owner|AdminUserRole" crates/ironclaw_product_workflow/src crates/ -g '*.rs'Repository: nearai/ironclaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- admin_users.rs role contract ---'
sed -n '1,220p' crates/ironclaw_product_workflow/src/reborn_services/admin_users.rs
echo
echo '--- role-sensitive admin-user service methods and tests ---'
rg -n "set_role\(|create_user\(|ensure_not_last_admin|AdminUserRole::Owner|AdminUserRole::Admin|AdminUserRole::Member|is_admin\(" \
crates/ironclaw_product_workflow/src/reborn_services/admin_users.rs \
crates/ironclaw_product_workflow/src/reborn_services.rs \
crates/ironclaw_product_workflow/src -g '*.rs'
echo
echo '--- tests mentioning owner/admin role transitions ---'
rg -n "Owner.*Admin|Admin.*Owner|create_admin_user|set_admin_user_role|set_role" \
crates/ironclaw_product_workflow/src -g '*.rs' | head -n 120Repository: nearai/ironclaw
Length of output: 12389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- owner/admin role usage in reborn composition identity adapter ---'
rg -n "RebornUserRole::Owner|AdminUserRole::Owner|role_to_identity|role_from_identity|Owner.*admin|admin.*Owner" \
crates/ironclaw_reborn_composition/src/admin_user_directory.rs \
crates/ironclaw_reborn_composition/src -g '*.rs'
echo
echo '--- admin-user directory mapping around role conversions ---'
sed -n '240,340p' crates/ironclaw_reborn_composition/src/admin_user_directory.rs
echo
echo '--- search for owner-specific admin-user checks across product workflow ---'
rg -n "AdminUserRole::Owner|role == .*Owner|is_owner\(|owner access required|owner-only" \
crates/ironclaw_product_workflow/src/reborn_services.rs \
crates/ironclaw_product_workflow/src/reborn_services \
crates/ironclaw_reborn_composition/src -g '*.rs'Repository: nearai/ironclaw
Length of output: 5673
No role-hierarchy cap on admin-user writes. authorize_admin() only gates on is_admin(), but AdminUserRole::Owner is still a distinct stored role (role_to_identity / role_from_identity). That lets any Admin mint or promote an Owner in create_admin_user and set_admin_user_role. Add a caller-role ceiling before those mutations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_product_workflow/src/reborn_services.rs` around lines 2998 -
3022, The admin write paths allow any caller that passes authorize_admin() to
create or assign an Owner role, so add a role-ceiling check before the mutations
in create_admin_user and set_admin_user_role. Use the existing AdminUserRole and
role conversion helpers (role_to_identity / role_from_identity) to compare the
caller’s admin role against the requested target role, and reject any attempt to
mint or promote Owner unless the caller is explicitly allowed. Keep the guard
close to the admin_users.create_user and role update logic so both user creation
and role changes enforce the same hierarchy rule.
| /// Failure modes of the admin user port. Deliberately coarse and free of | ||
| /// backend detail — the composition adapter maps identity/secret errors into | ||
| /// these, and the facade maps these into the sanitized `RebornServicesError` | ||
| /// wire taxonomy. Authorization and last-admin protection are enforced in the | ||
| /// facade, not here, so they are not modeled as port errors. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum AdminUserError { | ||
| /// The targeted user id has no record. | ||
| NotFound, | ||
| /// A transient backend failure; the caller may retry. | ||
| Unavailable, | ||
| /// A backend inconsistency or unexpected failure; not retryable. | ||
| Internal, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
AdminUserError doesn't use thiserror.
RebornIdentityError in the identity crate is a proper thiserror enum with #[error(...)] messages, but this port-level error is a bare Debug/Copy enum with no Display/std::error::Error impl. As per coding guidelines, "Use thiserror for error types in error.rs" and more broadly "Use thiserror for Rust error types and map errors with context" for **/*.rs. The "deliberately coarse, free of backend detail" design goal doesn't require dropping Error/Display — #[error("user not found")] etc. keeps it coarse while staying consistent with the rest of the stack.
♻️ Suggested fix
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)]
pub enum AdminUserError {
/// The targeted user id has no record.
+ #[error("user not found")]
NotFound,
/// A transient backend failure; the caller may retry.
+ #[error("admin user service temporarily unavailable")]
Unavailable,
/// A backend inconsistency or unexpected failure; not retryable.
+ #[error("internal admin user service error")]
Internal,
}As per coding guidelines, "Use thiserror for Rust error types and map errors with context."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Failure modes of the admin user port. Deliberately coarse and free of | |
| /// backend detail — the composition adapter maps identity/secret errors into | |
| /// these, and the facade maps these into the sanitized `RebornServicesError` | |
| /// wire taxonomy. Authorization and last-admin protection are enforced in the | |
| /// facade, not here, so they are not modeled as port errors. | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | |
| pub enum AdminUserError { | |
| /// The targeted user id has no record. | |
| NotFound, | |
| /// A transient backend failure; the caller may retry. | |
| Unavailable, | |
| /// A backend inconsistency or unexpected failure; not retryable. | |
| Internal, | |
| } | |
| /// Failure modes of the admin user port. Deliberately coarse and free of | |
| /// backend detail — the composition adapter maps identity/secret errors into | |
| /// these, and the facade maps these into the sanitized `RebornServicesError` | |
| /// wire taxonomy. Authorization and last-admin protection are enforced in the | |
| /// facade, not here, so they are not modeled as port errors. | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] | |
| pub enum AdminUserError { | |
| /// The targeted user id has no record. | |
| #[error("user not found")] | |
| NotFound, | |
| /// A transient backend failure; the caller may retry. | |
| #[error("admin user service temporarily unavailable")] | |
| Unavailable, | |
| /// A backend inconsistency or unexpected failure; not retryable. | |
| #[error("internal admin user service error")] | |
| Internal, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_product_workflow/src/reborn_services/admin_users.rs` around
lines 96 - 109, AdminUserError is a plain enum without Display/std::error::Error
support, which makes it inconsistent with the rest of the error types. Update
the AdminUserError type in admin_users.rs to use thiserror with per-variant
#[error(...)] messages while keeping the coarse taxonomy intact, and preserve
the existing variants NotFound, Unavailable, and Internal. If needed, adjust any
call sites in the admin user facade/adapter that rely on formatting or
conversion so they continue mapping errors with context cleanly.
Source: Coding guidelines
| #[tokio::test] | ||
| async fn admin_last_admin_protection_over_http() { | ||
| let harness = build_admin_harness().await; | ||
| let operator = AdminApiDriver::new(harness.router.clone(), OPERATOR_TOKEN); | ||
|
|
||
| // One admin user record → it is the sole active admin. | ||
| let (_, sole) = operator.create_user(None, "Sole Admin", "admin").await; | ||
| let sole_id = user_id_of(&sole); | ||
|
|
||
| let (status, demote) = operator.set_role(&sole_id, "member").await; | ||
| assert_eq!( | ||
| status, | ||
| StatusCode::CONFLICT, | ||
| "demoting the sole admin is blocked" | ||
| ); | ||
| assert_eq!( | ||
| demote["field"].as_str(), | ||
| Some("last_admin"), | ||
| "the block carries the stable last_admin marker" | ||
| ); | ||
| let (status, _) = operator.set_status(&sole_id, "suspended").await; | ||
| assert_eq!( | ||
| status, | ||
| StatusCode::CONFLICT, | ||
| "suspending the sole admin is blocked" | ||
| ); | ||
|
|
||
| // A second admin removes the protection. | ||
| let (_, second) = operator.create_user(None, "Second Admin", "admin").await; | ||
| let second_id = user_id_of(&second); | ||
| let (status, _) = operator.set_role(&second_id, "member").await; | ||
| assert_eq!( | ||
| status, | ||
| StatusCode::OK, | ||
| "demoting one of two admins is allowed" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Last-admin protection test doesn't cover the delete path.
This test exercises demote (set_role) and suspend (set_status) for the sole admin, but never delete_user. Deleting the sole admin is the most destructive of the three "drop to zero admins" mutations and is the one case the flagship lifecycle test (line 454-462) never applies to an admin — it only deletes a member. If ensure_not_last_admin (per the reborn_services.rs snippet) is also invoked from the delete path, add:
let (status, _) = operator.delete_user(&sole_id).await;
assert_eq!(status, StatusCode::CONFLICT, "deleting the sole admin is blocked");If delete intentionally bypasses this guard, that's worth a comment in reborn_services.rs documenting why deletion is exempt from last-admin protection while demote/suspend are not.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_composition/tests/admin_api_e2e.rs` around lines 465 -
501, The last-admin protection test in admin_last_admin_protection_over_http
only covers demotion and suspension, not the delete path. Extend this test using
AdminApiDriver::delete_user on the sole admin and assert it returns
StatusCode::CONFLICT, matching the existing last_admin behavior checked via
set_role and set_status. If delete_user is meant to bypass
ensure_not_last_admin, add a clear comment or documentation in
reborn_services.rs explaining that exemption.
| | Record | Path (opaque segments base64url-encoded) | Fields | | ||
| |---|---|---| | ||
| | `StoredUser` — the canonical **user profile** | `…/users/{user_id}.json` | `email`, `display_name`, `created_at`, `updated_at` | | ||
| | `StoredExternalIdentity` — one bound external login | `…/external/{tenant}/{surface}/{provider}/{instance}/{subject}.json` | `user_id`, `email`, `email_verified`, `created_at` | | ||
| | `StoredVerifiedEmailIndex` — cross-provider link | `…/verified-email/{tenant}/{lower_email}.json` | `user_id` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Persisted-records table is stale for the new StoredUser fields.
The table row for StoredUser still lists only email, display_name, created_at, updated_at — but this PR adds status, role, created_by, last_login_at, tenant_id, and metadata to the same record (see filesystem_store/directory.rs StoredUser usage). This is exactly the "Stale Comments After Refactors" trap: a contract doc claiming a narrower schema than the code persists is a bug report waiting to happen for the next reader who trusts the table.
📝 Suggested fix
-| `StoredUser` — the canonical **user profile** | `…/users/{user_id}.json` | `email`, `display_name`, `created_at`, `updated_at` |
+| `StoredUser` — the canonical **user profile** | `…/users/{user_id}.json` | `email`, `display_name`, `created_at`, `updated_at`, `status`, `role`, `created_by`, `last_login_at`, `tenant_id`, `metadata` |Based on learnings, "Doc strings and inline comments are part of the contract... update or delete them in the same change" (Stale Comments After Refactors, .claude/rules).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Record | Path (opaque segments base64url-encoded) | Fields | | |
| |---|---|---| | |
| | `StoredUser` — the canonical **user profile** | `…/users/{user_id}.json` | `email`, `display_name`, `created_at`, `updated_at` | | |
| | `StoredExternalIdentity` — one bound external login | `…/external/{tenant}/{surface}/{provider}/{instance}/{subject}.json` | `user_id`, `email`, `email_verified`, `created_at` | | |
| | `StoredVerifiedEmailIndex` — cross-provider link | `…/verified-email/{tenant}/{lower_email}.json` | `user_id` | | |
| | Record | Path (opaque segments base64url-encoded) | Fields | | |
| |---|---|---| | |
| | `StoredUser` — the canonical **user profile** | `…/users/{user_id}.json` | `email`, `display_name`, `created_at`, `updated_at`, `status`, `role`, `created_by`, `last_login_at`, `tenant_id`, `metadata` | | |
| | `StoredExternalIdentity` — one bound external login | `…/external/{tenant}/{surface}/{provider}/{instance}/{subject}.json` | `user_id`, `email`, `email_verified`, `created_at` | | |
| | `StoredVerifiedEmailIndex` — cross-provider link | `…/verified-email/{tenant}/{lower_email}.json` | `user_id` | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_identity/CONTRACT.md` around lines 50 - 54, Update the
persisted-records table in CONTRACT.md so the `StoredUser` row matches the
actual `StoredUser` schema used by `filesystem_store/directory.rs`. Add the
newly persisted fields (`status`, `role`, `created_by`, `last_login_at`,
`tenant_id`, `metadata`) to the `StoredUser` field list, and keep the row
aligned with the record definition so the contract documentation reflects the
current storage shape.
Source: Path instructions
| /// Tenant that owns this user. `None` on records written before the admin | ||
| /// surface existed; enumeration treats `None` as the deployment's single | ||
| /// configured tenant (see `RebornUserDirectory::list_users`). | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub(super) tenant_id: Option<String>, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and map their structure before reading slices.
git ls-files 'crates/ironclaw_reborn_identity/src/filesystem_store/*' 'crates/ironclaw_reborn_identity/src/**/directory.rs' 'CLAUDE.md' '.claude/**' | sed -n '1,200p'
echo
echo "== outline: record.rs =="
ast-grep outline crates/ironclaw_reborn_identity/src/filesystem_store/record.rs --view expanded
echo
echo "== outline: directory.rs =="
ast-grep outline crates/ironclaw_reborn_identity/src/filesystem_store/directory.rs --view expanded
echo
echo "== relevant lines in record.rs =="
sed -n '1,120p' crates/ironclaw_reborn_identity/src/filesystem_store/record.rs
echo
echo "== relevant lines in directory.rs =="
sed -n '1,220p' crates/ironclaw_reborn_identity/src/filesystem_store/directory.rs
echo
echo "== repo invariant mentions =="
rg -n 'Everything Goes Through Tools|Fail loud|LLM data is never deleted|tenant|multi-tenant|single configured tenant' CLAUDE.md .claude crates/ironclaw_reborn_identity/src -SRepository: nearai/ironclaw
Length of output: 35440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' crates/ironclaw_reborn_identity/src/user_directory.rs
echo
sed -n '240,360p' crates/ironclaw_reborn_identity/src/filesystem_store/directory.rs
echo
sed -n '790,880p' crates/ironclaw_reborn_identity/src/filesystem_store/tests.rsRepository: nearai/ironclaw
Length of output: 13920
Gate legacy tenant_id: None rows behind a real single-tenant check
None => true here, so pre-admin users are surfaced in every tenant’s admin list. The admin directory contract only treats those rows as safe in single-tenant deployments; either backfill tenant IDs or fail closed once the deployment is multi-tenant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_identity/src/filesystem_store/record.rs` around lines
34 - 38, The legacy tenant handling for `tenant_id` is too permissive because
`None` currently gets treated as valid for every tenant. Update the
`RebornUserDirectory::list_users` path and the `record.rs` `tenant_id` fallback
so `None` is only accepted when the deployment is truly single-tenant; otherwise
fail closed or exclude those rows from admin enumeration. If needed, gate the
`Option<String>` fallback with an explicit single-tenant check instead of
unconditionally mapping `None` to true.
| /// Parse a `{user_id}` path segment into a `UserId`, mapping a malformed value | ||
| /// to a sanitized `400 invalid_request` before the facade is touched. | ||
| fn parse_admin_user_id(raw: String) -> Result<UserId, WebUiV2HttpError> { | ||
| UserId::new(raw).map_err(|_| { | ||
| WebUiV2HttpError::from(RebornServicesError::from(WebUiInboundValidationError::new( | ||
| "user_id", | ||
| WebUiInboundValidationCode::InvalidId, | ||
| ))) | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
.map_err(|_| ...) drops the UserId::new validation cause.
The closure discards the error binding from UserId::new(raw) and substitutes a generic InvalidId error with no logging of the original reason. As per coding guidelines: "Do not use .map_err(|_| OtherError) or any closure that discards its error binding and substitutes a generic error; carry the cause instead or log the bound error before mapping."
🩹 Proposed fix — log the bound error before mapping
fn parse_admin_user_id(raw: String) -> Result<UserId, WebUiV2HttpError> {
- UserId::new(raw).map_err(|_| {
+ UserId::new(raw).map_err(|error| {
+ tracing::debug!(target = "ironclaw::webui_v2::admin", %error, "invalid admin user_id path segment");
WebUiV2HttpError::from(RebornServicesError::from(WebUiInboundValidationError::new(
"user_id",
WebUiInboundValidationCode::InvalidId,
)))
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Parse a `{user_id}` path segment into a `UserId`, mapping a malformed value | |
| /// to a sanitized `400 invalid_request` before the facade is touched. | |
| fn parse_admin_user_id(raw: String) -> Result<UserId, WebUiV2HttpError> { | |
| UserId::new(raw).map_err(|_| { | |
| WebUiV2HttpError::from(RebornServicesError::from(WebUiInboundValidationError::new( | |
| "user_id", | |
| WebUiInboundValidationCode::InvalidId, | |
| ))) | |
| }) | |
| } | |
| /// Parse a `{user_id}` path segment into a `UserId`, mapping a malformed value | |
| /// to a sanitized `400 invalid_request` before the facade is touched. | |
| fn parse_admin_user_id(raw: String) -> Result<UserId, WebUiV2HttpError> { | |
| UserId::new(raw).map_err(|error| { | |
| tracing::debug!(target = "ironclaw::webui_v2::admin", %error, "invalid admin user_id path segment"); | |
| WebUiV2HttpError::from(RebornServicesError::from(WebUiInboundValidationError::new( | |
| "user_id", | |
| WebUiInboundValidationCode::InvalidId, | |
| ))) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_webui_v2/src/handlers.rs` around lines 196 - 205, The
`parse_admin_user_id` helper is discarding the concrete `UserId::new(raw)`
validation error by using `map_err(|_| ...)`. Update this mapping so the error
binding from `UserId::new` is preserved either by carrying the cause into the
`WebUiInboundValidationError`/`RebornServicesError` chain or by logging the
bound error before converting it to `WebUiV2HttpError`. Keep the existing
`parse_admin_user_id` flow and `UserId::new` call, but avoid substituting a
generic `InvalidId` without the original reason.
Source: Coding guidelines
| export async function updateAdminUser(id, payload) { | ||
| if (payload && Object.prototype.hasOwnProperty.call(payload, "role")) { | ||
| const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}/role`, { | ||
| method: "POST", | ||
| body: { role: payload.role }, | ||
| }); | ||
| return normalizeUser(response?.user); | ||
| } | ||
| const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}`, { | ||
| method: "PATCH", | ||
| body: { | ||
| display_name: payload?.display_name, | ||
| metadata: payload?.metadata, | ||
| }, | ||
| }); | ||
| return normalizeUser(response?.user); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
updateAdminUser silently drops non-role fields when role is present.
If payload ever contains role together with display_name/metadata, the function returns after the /role call and never reaches the PATCH branch — those fields are silently dropped, not merged or rejected. The adjacent comment (lines 43-45) claims routing "keeps the client honest," but the current branching does the opposite for any mixed payload: it discards data with no error, warning, or partial-success signal. Currently masked because the caller only ever sends { role } alone, but that's an external invariant this file can't enforce.
🐛 Proposed fix: don't let a mixed payload silently lose fields
export async function updateAdminUser(id, payload) {
- if (payload && Object.prototype.hasOwnProperty.call(payload, "role")) {
- const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}/role`, {
- method: "POST",
- body: { role: payload.role },
- });
- return normalizeUser(response?.user);
- }
- const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}`, {
- method: "PATCH",
- body: {
- display_name: payload?.display_name,
- metadata: payload?.metadata,
- },
- });
- return normalizeUser(response?.user);
+ const hasRole = payload && Object.prototype.hasOwnProperty.call(payload, "role");
+ const hasProfileFields =
+ payload && (payload.display_name !== undefined || payload.metadata !== undefined);
+
+ let latest;
+ if (hasRole) {
+ const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}/role`, {
+ method: "POST",
+ body: { role: payload.role },
+ });
+ latest = normalizeUser(response?.user);
+ }
+ if (hasProfileFields) {
+ const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}`, {
+ method: "PATCH",
+ body: { display_name: payload?.display_name, metadata: payload?.metadata },
+ });
+ latest = normalizeUser(response?.user);
+ }
+ return latest;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function updateAdminUser(id, payload) { | |
| if (payload && Object.prototype.hasOwnProperty.call(payload, "role")) { | |
| const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}/role`, { | |
| method: "POST", | |
| body: { role: payload.role }, | |
| }); | |
| return normalizeUser(response?.user); | |
| } | |
| const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}`, { | |
| method: "PATCH", | |
| body: { | |
| display_name: payload?.display_name, | |
| metadata: payload?.metadata, | |
| }, | |
| }); | |
| return normalizeUser(response?.user); | |
| } | |
| export async function updateAdminUser(id, payload) { | |
| const hasRole = payload && Object.prototype.hasOwnProperty.call(payload, "role"); | |
| const hasProfileFields = | |
| payload && (payload.display_name !== undefined || payload.metadata !== undefined); | |
| let latest; | |
| if (hasRole) { | |
| const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}/role`, { | |
| method: "POST", | |
| body: { role: payload.role }, | |
| }); | |
| latest = normalizeUser(response?.user); | |
| } | |
| if (hasProfileFields) { | |
| const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}`, { | |
| method: "PATCH", | |
| body: { display_name: payload?.display_name, metadata: payload?.metadata }, | |
| }); | |
| latest = normalizeUser(response?.user); | |
| } | |
| return latest; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.js` around lines
46 - 62, `updateAdminUser` currently returns immediately when `payload.role` is
present, so any accompanying `display_name` or `metadata` is silently ignored.
Update the branching in `updateAdminUser` to either reject mixed payloads up
front or send all supported fields through the appropriate request path, and
keep the behavior explicit rather than dropping data. Use the existing
`apiFetch`, `normalizeUser`, and the role/PATCH routes in `admin-api.js` to
locate and fix the mixed-payload handling.
|
Follow-up on the integration-coverage note above — I verified this is writable today by building and running the three scenarios against this branch (all green, 0.10s):
Happy to push the mount-helper enabler + a ready |
|
🚅 Deployed to the ironclaw-pr-5779 environment in ironclaw-ci-preview
|
Follow-up test coverage for the admin user-management surface, plus a
frontend bug the JS test exposed.
- fix(webui-v2): `admin-api.js` passed request bodies as raw JS objects,
but `apiFetch` forwards `options.body` to `fetch` unchanged (it does
not serialize — callers must `JSON.stringify`, cf. `createThread`). In
a real browser every admin write (create/update/status/role/secret)
would have sent the string "[object Object]" and been rejected. Now
stringified. Caught by the new JS test driving the real `apiFetch`.
- test(webui-v2): `admin-api.test.js` — 14 Node `--test` cases driving
the real `apiFetch` (stubbing `globalThis.fetch`), asserting each
method/path/body and the id/token normalization. `jsonBody()` guards
the serialization fix above.
- test(e2e): repoint `test_admin_api.py` from the retired v1
`/api/admin/*` monolith surface to the v2 `/api/webchat/v2/admin/*`
routes on the real `ironclaw-reborn serve` binary (`reborn_v2_server`
fixture, operator env-bearer). Adds the flagship
`created_user_token_authenticates_as_that_user` round-trip, which
proves serve.rs's minter wiring: the one-time api_token validates AS
the new user at `/session` because the admin minter store and the SSO
login store share `session_signing_secret`. Added to
`reborn_coverage_tests.txt`.
Integration-tier note: admin coverage stays at the crate tier
(`ironclaw_reborn_composition/tests/admin_api_e2e.rs`); the
`AdminUserService` wiring is sealed `pub(crate)` in the composition
root and the `tests/integration` harness has no minter seam, so an
int-tier test would require faking the port ("wire the unwired") or
relocating the crate-tier test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.js (1)
43-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
updateAdminUserstill silently drops non-role fields whenroleis present.Same issue flagged previously: if
payloadcarriesroletogether withdisplay_name/metadata, the function returns after the/rolecall and the PATCH branch is never reached — those fields vanish with no error or warning, directly contradicting the adjacent comment's claim that routing "keeps the client honest."🐛 Proposed fix: don't let a mixed payload silently lose fields
export async function updateAdminUser(id, payload) { - if (payload && Object.prototype.hasOwnProperty.call(payload, "role")) { - const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}/role`, { - method: "POST", - body: JSON.stringify({ role: payload.role }), - }); - return normalizeUser(response?.user); - } - const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}`, { - method: "PATCH", - body: JSON.stringify({ - display_name: payload?.display_name, - metadata: payload?.metadata, - }), - }); - return normalizeUser(response?.user); + const hasRole = payload && Object.prototype.hasOwnProperty.call(payload, "role"); + const hasProfileFields = + payload && (payload.display_name !== undefined || payload.metadata !== undefined); + + let latest; + if (hasRole) { + const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}/role`, { + method: "POST", + body: JSON.stringify({ role: payload.role }), + }); + latest = normalizeUser(response?.user); + } + if (hasProfileFields) { + const response = await apiFetch(`${ADMIN_BASE}/users/${encodeURIComponent(id)}`, { + method: "PATCH", + body: JSON.stringify({ display_name: payload?.display_name, metadata: payload?.metadata }), + }); + latest = normalizeUser(response?.user); + } + return latest; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.js` around lines 43 - 62, updateAdminUser currently returns immediately after handling payload.role, so any accompanying display_name or metadata fields are silently ignored. Update the routing logic in updateAdminUser to either reject mixed payloads explicitly or send both updates in a way that preserves all fields, and make sure the role-specific /users/:id/role path does not bypass the PATCH flow for non-role changes. Use the existing normalizeUser, apiFetch, and ADMIN_BASE handling to keep the behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/scenarios/test_admin_api.py`:
- Line 35: The parameterless pytest fixtures use unnecessary empty parentheses
on the decorator, which Ruff PT001 flags. Update the fixture decorators on the
affected fixture functions to use the plain `@pytest.fixture` form instead of
`@pytest.fixture`(), and apply the same change to each matching fixture in this
test module.
---
Duplicate comments:
In `@crates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.js`:
- Around line 43-62: updateAdminUser currently returns immediately after
handling payload.role, so any accompanying display_name or metadata fields are
silently ignored. Update the routing logic in updateAdminUser to either reject
mixed payloads explicitly or send both updates in a way that preserves all
fields, and make sure the role-specific /users/:id/role path does not bypass the
PATCH flow for non-role changes. Use the existing normalizeUser, apiFetch, and
ADMIN_BASE handling to keep the behavior consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd2bb644-9a90-4ade-b9a9-9db8ef202080
📒 Files selected for processing (4)
crates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.jscrates/ironclaw_webui_v2/static/js/pages/admin/lib/admin-api.test.jstests/e2e/reborn_coverage_tests.txttests/e2e/scenarios/test_admin_api.py
| ADMIN_BASE = "/api/webchat/v2/admin" | ||
|
|
||
|
|
||
| @pytest.fixture() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drop the empty parens on @pytest.fixture().
Ruff (PT001) flags unnecessary parens on parameterless fixture decorators.
-@pytest.fixture()
+@pytest.fixture
async def admin_client(reborn_v2_server):-@pytest.fixture()
+@pytest.fixture
async def test_user(admin_client):Also applies to: 46-46
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 35-35: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/scenarios/test_admin_api.py` at line 35, The parameterless pytest
fixtures use unnecessary empty parentheses on the decorator, which Ruff PT001
flags. Update the fixture decorators on the affected fixture functions to use
the plain `@pytest.fixture` form instead of `@pytest.fixture`(), and apply the same
change to each matching fixture in this test module.
Source: Linters/SAST tools
`ironclaw-reborn serve` unconditionally wires the admin-API token minter (serve.rs) — "admin creates user" always returns a signed **session** bearer as the one-time `api_token`. But the `SessionAuthenticator` that validates session bearers was only wired on the SSO path: with no SSO provider configured, `build_webui_auth_surface` returned just the env-bearer authenticator. So in the default no-SSO deployment, an admin-created user's API token failed with 401 on every request — the feature was dead on arrival exactly where it is most likely used. Compose the env-bearer (operator) authenticator with a `SessionAuthenticator` over the same `signed_session_store` the minter writes to, in the no-SSO branch of the auth surface. Operator capabilities still follow the env token only (`CompositeAuthenticator:: mounts_operator_webui_config_routes`), so the minted session bearer stays non-operator per the ingress crate's SSO-identity-only invariant. `CompositeAuthenticator` (env-OR-session, previously used only on the SSO path) is made `pub` and reused rather than duplicating the shim. Regression: caught by the crate's binary e2e `tests/e2e/scenarios/test_admin_api.py::test_created_user_token_authenticates_as_that_user` (added in the prior commit), which now passes 10/10 against a freshly built `ironclaw-reborn` — the crate-tier `admin_api_e2e.rs` masked this because it hand-wired a `SessionAuthenticator` production serve.rs did not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_cli/src/commands/webui_auth.rs`:
- Around line 76-98: The no-SSO branch in webui_auth.rs now constructs a
CompositeAuthenticator with a SessionAuthenticator, but the test only checks
public_mount; update the relevant webui auth test to also exercise the minted
session-bearer path. Add an assertion that a session token created through the
no-SSO setup authenticates successfully via the WebuiAuthenticator, using the
same no-SSO/CompositeAuthenticator flow so the admin-API bearer case is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5666c55-401f-4673-961a-00d51fbe6a52
📒 Files selected for processing (3)
crates/ironclaw_reborn_cli/src/commands/webui_auth.rscrates/ironclaw_reborn_webui_ingress/src/lib.rscrates/ironclaw_reborn_webui_ingress/src/signed_session_login.rs
| // No SSO providers: no public login routes, and no SSO logins to seed | ||
| // local trigger access for (bootstrap config is unused here). But the | ||
| // serve layer *always* wires the admin-API token minter, which mints | ||
| // signed **session** tokens (the user-create bearer). Those validate | ||
| // only through a `SessionAuthenticator` over the same signed store — | ||
| // absent it, an admin-created user's API token would 401 on every | ||
| // request (regression caught by `tests/e2e/scenarios/test_admin_api.py`). | ||
| // Compose the env-bearer (operator) authenticator with a session | ||
| // authenticator over that store so minted tokens work without SSO; | ||
| // operator capabilities still follow the env token only, so the session | ||
| // bearer stays non-operator. | ||
| let session_authenticator: Arc<dyn WebuiAuthenticator> = Arc::new( | ||
| SessionAuthenticator::new(signed_session_store(&session_signing_secret, &tenant_id)), | ||
| ); | ||
| let authenticator: Arc<dyn WebuiAuthenticator> = Arc::new(CompositeAuthenticator::new( | ||
| session_authenticator, | ||
| env_authenticator, | ||
| )); | ||
| return Ok(WebuiAuthSurface { | ||
| authenticator: env_authenticator, | ||
| authenticator, | ||
| public_mount: None, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify minter and authenticator share secret+tenant, and inspect the no-SSO test.
rg -nP -C4 'from_operator_secret|signed_session_store|AdminApiTokenMinter' crates/ironclaw_reborn_cli/src/commands/serve.rs crates/ironclaw_reborn_composition/src/admin_token.rs
rg -nP -C6 'no_sso_keeps_env_authenticator' crates/ironclaw_reborn_cli/src/commands/webui_auth.rsRepository: nearai/ironclaw
Length of output: 3593
🏁 Script executed:
#!/bin/bash
sed -n '60,110p' crates/ironclaw_reborn_cli/src/commands/webui_auth.rs
printf '\n---\n'
sed -n '230,270p' crates/ironclaw_reborn_cli/src/commands/webui_auth.rsRepository: nearai/ironclaw
Length of output: 4546
🏁 Script executed:
#!/bin/bash
rg -n "minted session|authenticator.authenticate|signed_session_store|admin_api" crates/ironclaw_reborn_cli/src/commands/webui_auth.rs crates/ironclaw_reborn_cli/src/commands crates/ironclaw_reborn_composition/src testsRepository: nearai/ironclaw
Length of output: 2548
Cover the no-SSO session-bearer path crates/ironclaw_reborn_cli/src/commands/webui_auth.rs:234 The test still only asserts public_mount.is_none(), but this branch now builds a CompositeAuthenticator. Add a token-level assertion that a minted session bearer authenticates on the no-SSO path; otherwise the admin-API case stays untested.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/ironclaw_reborn_cli/src/commands/webui_auth.rs` around lines 76 - 98,
The no-SSO branch in webui_auth.rs now constructs a CompositeAuthenticator with
a SessionAuthenticator, but the test only checks public_mount; update the
relevant webui auth test to also exercise the minted session-bearer path. Add an
assertion that a session token created through the no-SSO setup authenticates
successfully via the WebuiAuthenticator, using the same
no-SSO/CompositeAuthenticator flow so the admin-API bearer case is covered.
Source: Path instructions
| /// `PUT /api/webchat/v2/admin/users/{user_id}/secrets/{handle}` | ||
| pub async fn admin_put_user_secret( | ||
| State(state): State<WebUiV2State>, | ||
| Extension(caller): Extension<WebUiAuthenticatedCaller>, | ||
| Path((user_id, handle)): Path<(String, String)>, | ||
| Json(body): Json<RebornAdminPutSecretRequest>, | ||
| ) -> Result<Json<RebornAdminSecretResponse>, WebUiV2HttpError> { | ||
| let user_id = parse_admin_user_id(user_id)?; | ||
| Ok(Json( | ||
| state | ||
| .services() | ||
| .put_admin_user_secret(caller, user_id, handle, body) | ||
| .await?, | ||
| )) | ||
| } |
| /// `DELETE /api/webchat/v2/admin/users/{user_id}/secrets/{handle}` | ||
| pub async fn admin_delete_user_secret( | ||
| State(state): State<WebUiV2State>, | ||
| Extension(caller): Extension<WebUiAuthenticatedCaller>, | ||
| Path((user_id, handle)): Path<(String, String)>, | ||
| ) -> Result<Json<RebornAdminSecretDeletedResponse>, WebUiV2HttpError> { | ||
| let user_id = parse_admin_user_id(user_id)?; | ||
| Ok(Json( | ||
| state | ||
| .services() | ||
| .delete_admin_user_secret(caller, user_id, handle) | ||
| .await?, | ||
| )) | ||
| } |
| // Every handler delegates straight to the facade, which enforces admin | ||
| // authorization (operator token or admin/owner role) and last-admin protection. | ||
| // The `{user_id}` path segment is parsed into a `UserId` here so a malformed id | ||
| // is a 400 before the facade runs; the `{handle}` segment stays a String and is | ||
| // validated deeper (the secret store rejects a bad handle). | ||
|
|
| /// Build a signed-token [`SessionStore`] for minting/validating bearers from an | ||
| /// operator secret + tenant. The store is stateless and deterministic in the | ||
| /// signing key, so an instance built here mints tokens that validate under any | ||
| /// other instance sharing the same operator secret + tenant (e.g. the SSO login | ||
| /// surface's own store). Used by the admin user-management surface to mint the | ||
| /// one-time API bearer on user create, which must be wired before the login | ||
| /// surface (and its own store) is composed. | ||
| pub fn signed_session_store( | ||
| operator_secret: &SecretString, | ||
| tenant_id: &TenantId, | ||
| ) -> std::sync::Arc<dyn SessionStore> { |
| /// Compose an env-bearer authenticator with a session authenticator. The | ||
| /// env token is tried first (it carries operator capabilities); a token it | ||
| /// rejects falls through to the non-operator `session` authenticator. | ||
| pub fn new( | ||
| session: Arc<dyn WebuiAuthenticator>, |
Two adversarial-testing findings on the admin user-management surface, each fixed with its regression test. 1. Suspended admin retained full admin powers. `authorize_admin` checked `role.is_admin()` only, never `status`, so a SUSPENDED admin (role still reads Admin) kept complete control of the admin API. Now authorization requires admin/owner role AND `status == Active`, read on every call (never cached) — suspending an admin revokes their access immediately. Covered by `admin_suspended_admin_is_forbidden_on_every_verb`, plus a widened `admin_member_caller_is_forbidden_on_every_verb` / `admin_unknown_caller_is_forbidden_on_every_verb` sweep that drives the 403 through EVERY admin verb (not just list) per the "test through the caller" rule, including self-privilege-escalation. 2. Last-admin protection had a TOCTOU race. `ensure_not_last_admin` re-reads the active-admin count then mutates; two concurrent demotions each read "2 admins", both pass, and both land → 0 admins, stranding the tenant. Added a per-tenant admin-mutation lock (reusing the existing weak-ref keyed-lock registry, namespaced so keyspaces can't collide) held across the check+mutation in set_role / set_status / delete. Covered by `admin_last_admin_protection_survives_concurrent_demotion` (multi- thread runtime, concurrent demotions → exactly one lands, an admin always survives). Also adds session-store characterization tests locking two intentional, security-relevant bounds of the stateless signed-session denylist: revocation does not survive a process restart, and denylist eviction can resurrect a revoked-but-unexpired token under >4096-revocation pressure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds 7 adversarial HTTP tests to the crate-tier admin e2e (real router + authenticator), and fixes a bug one of them surfaced. Fix: a malformed secret handle (path-traversal-shaped, e.g. `..%2F..`) was fail-closed (SecretHandle validation rejects it, nothing written) but the rejection mapped to `AdminUserError::Internal` → **500**. The handle is taken raw from the request path with no edge validation (a stale comment claimed otherwise), so a client-supplied bad handle is the client's fault: add `AdminUserError::InvalidInput` → **400**, and map the `SecretHandle` construction failure to it in put/delete. New e2e tests (crate `ironclaw_reborn_composition`, `admin_api_e2e.rs`): - deleted admin's token → 403 on admin routes (delete revokes admin access via no-record); documents (does not lock) that the stateless token still authenticates non-admin `/session` — a separate session-revocation gap. - suspended admin's token → 403 (exercises the status-gate fix). - minted admin *session* bearer is denied operator routes (no `operator_webui_config`) while still allowed on admin user CRUD. - forged / tampered / foreign-secret / expired tokens → 401. - oversized create body → 413 (per-route 16 KiB cap, before the facade). - secret-handle path traversal contained → now 400 (pins the fix above). - malformed user_id → 404, invalid role/status enum → 422; never 500. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.32% — 283640 / 332453 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (4 entry/entries excluded from the accounting above)
|
Summary
Adds an end-to-end admin user-management surface to the Reborn stack, threaded through all five layers (
identity → product_workflow → composition → webui_v2 → serve → frontend).The surface is built on the existing
StoredUserrecords already persisted byironclaw_reborn_identityon every SSO login — it does not stand up a new user store. (An early plan miss assumed Reborn had no user store; the correction is captured as a new agent rule,.claude/rules/discovery-claims.md.)What ships
Identity (
ironclaw_reborn_identity) — newRebornUserDirectorytrait on the sameFilesystemRebornIdentityStore, kept deliberately separate fromRebornIdentityResolverso admin CRUD can't perturb the mint/link/create invariants:list_users/get_user/create_user(admin-mint, no external identity) /update_profile/update_status/update_role/record_last_login/count_active_adminsdelete_usercascades — removes external-identity records + verified-email index (the one sanctioned unwind of the linking invariants)CONTRACT.mdnow documents the three persisted record shapes and the directory surface.Product workflow (
ironclaw_product_workflow) —RebornServicesApiadmin_*methods with caller authorization (admin/owner role or env-bearer operator) and last-admin protection enforced in the facade; newAdminUserServiceport + request/response DTOs.Composition (
ironclaw_reborn_composition) —admin_user_directory/admin_secrets/admin_tokenadapters;AdminApiTokenMinterport for the one-time API bearer minted on user-create. The/tenant-sharedmount now grants list+delete for the identity delete cascade.WebUI v2 (
ironclaw_webui_v2) — REST routes + descriptors (body/rate limits):GET|POST /admin/usersGET|PATCH|DELETE /admin/users/:idPOSTstatus,POSTroleGET|PUT|DELETEper-user secretsServe (
ironclaw-reborn serve) — wires a signed-session-store-backed minter that issues a 365-day API bearer validating under the SSO login surface's own store.Frontend — un-hides the admin nav + Users tab and wires
admin-api.jsto the real endpoints (dashboard/usage analytics tabs remain out of scope).Authorization & safety
Tests
Verification
cargo fmt --checkcleancargo clippy --all --tests --all-featuresclean (0 warnings)cargo testforironclaw_reborn_identity,ironclaw_product_workflow,ironclaw_webui_v2(--all-features) — all passcargo test -p ironclaw_reborn_composition --features webui-v2-beta,libsql— admin tests pass; 16 compute-heavyruntime::tests hit their 10s timeout only under concurrent-suite load and pass in isolation (unrelated to this change)🤖 Generated with Claude Code
Follow-up test coverage (2nd commit)
Added at reviewer request, and it surfaced a real frontend bug:
admin-api.jsrequest bodies. The client passed bodies as raw JS objects, butapiFetchforwardsoptions.bodytofetchunchanged (callers mustJSON.stringify, percreateThread). In a real browser every admin write (create/update/status/role/secret) would have sent"[object Object]"and been rejected. Now stringified. Neither the crate-tier nor the httpx e2e catches this (both send real JSON) — only a JS test through the realapiFetchdoes.admin-api.test.js. 14node --testcases driving the realapiFetch(stubbingglobalThis.fetch), asserting each method/path/body and the id/token normalization; ajsonBody()helper guards the serialization fix.test_admin_api.py. Moved from the retired v1/api/admin/*monolith surface to the v2/api/webchat/v2/admin/*routes on the realironclaw-reborn servebinary. Adds the flagshipcreated_user_token_authenticates_as_that_userround-trip: the one-timeapi_tokenvalidates AS the new user at/session, proving serve.rs's minter wiring (admin minter store and SSO login store sharesession_signing_secret). Added toreborn_coverage_tests.txt.Test-tier decision (integration vs. crate)
Admin coverage stays at the crate tier (
ironclaw_reborn_composition/tests/admin_api_e2e.rs), nottests/integration/. TheAdminUserServiceis wired in exactly one place —build_webui_services(&RebornRuntime)— and its adapter (RebornAdminUserDirectory), the secret provisioner, and the three runtime feeder accessors are allpub(crate)in the composition crate, sealed to the composition root. Thetests/integration/webui_v2_product_api.rsharness never builds aRebornRuntime(it hand-buildsRebornServices::new) and itsRebornBuildInputpath has nowith_admin_api_token_minterseam (that setter is onRebornRuntimeInput). Reaching the real path at the int tier would require either faking theAdminUserServiceport — "wire the unwired," forbidden by.claude/rules/testing.md— or relocating the exact constructionadmin_api_e2e.rsalready is; and the int-tier mount helper bypasses the bearer authenticator, so it can't exercise the operator/member authorization or the minted-token-login chain this surface is built around.Bug caught + fixed by running the binary e2e (3rd commit)
Running
test_admin_api.pyagainst a freshly builtironclaw-rebornfailed the flagship token round-trip — a real production bug:serve.rsalways wires the admin-API token minter, but theSessionAuthenticatorthat validates the minted session bearer was only wired on the SSO path. In the default no-SSOironclaw-reborn servedeployment, an admin-created user'sapi_tokengot 401 on every request — the feature was dead on arrival exactly where it's most likely used. The crate-tieradmin_api_e2e.rsmasked it (it hand-wired aSessionAuthenticatorproduction serve did not).Fix:
build_webui_auth_surface's no-SSO branch now composes the env-bearer (operator) authenticator with aSessionAuthenticatorover the samesigned_session_storethe minter writes to. Operator capabilities still follow the env token only, so the minted session bearer stays non-operator (per the ingress crate's SSO-identity-only invariant).CompositeAuthenticator(env-OR-session) is madepuband reused rather than duplicating the shim.Verification (follow-up)
node --test .../admin-api.test.js— 14/14 passironclaw-reborn(webui-v2-beta):test_admin_api.py— 10/10 pass after the no-SSO fix (was 9/10, the flagship round-trip red before the fix)cargo test -p ironclaw_reborn_webui_ingress --all-features— pass;cargo clippy -p ironclaw_reborn_webui_ingress -p ironclaw_reborn_cli --features webui-v2-beta --tests— cleanStress / adversarial testing round (commits 4–5)
Adversarial and concurrency tests across authz, tokens, last-admin protection, session revocation, and input validation. Three more real bugs found and fixed, each with its regression test.
Bugs fixed:
authorize_admincheckedroleonly, neverstatus— a suspended admin (role stillAdmin) retained complete control. Now requires role ANDstatus == Active(read every call, never cached).AdminUserError::InvalidInput→ 400.Tests added:
Documented gaps (not fixed — larger scope):
api_tokenstill authenticates non-admin routes until its 365-day expiry. Delete revokes admin access (no record) but not the session itself; the signed-session denylist is process-local and per-token, with no per-user revocation. Flagged for a follow-up (durable/per-user session revocation).