fix(server): user system review — transactional last-admin guard and role disclosure#157
Merged
Merged
Conversation
…elete The count(role='admin') check and the subsequent mutation ran outside a shared transaction, so two concurrent demotions/deletions could both pass the guard and leave the system with zero admins — a state init_admin cannot recover because it only re-bootstraps an empty users table. Wrapping each path in one transaction lets SQLite serialize the writers so the second request re-reads the count and is correctly rejected; the multi-table cleanup in delete_user becomes atomic as a bonus.
Spell out in the dialog that a member sees all servers' monitoring data (security events and public IPs included) with no write access, and point at status pages for exposing selected servers to outsiders, so member accounts aren't mistaken for scoped/customer-facing access.
…nd admin recovery - Admin guide: state that member visibility is fleet-wide with no per-server scoping, and direct external exposure to status pages - Admin guide: add a lost-admin recovery procedure (wipe users table, restart to re-trigger the bootstrap banner) with its side effects - Security guide + ENV.md: warn that oauth allow_registration with a public provider grants fleet-wide read access to every provider user
Six decisions from a design grilling of the two-tier RBAC user system: member positioning, no forced password change, WS revalidation declined, oauth registration handled via docs, transactional last-admin guard (fixed), and documented admin lockout recovery.
Mirror the web create-user dialog: the Role section footer now states that a member sees all servers' monitoring data (security events and public IPs included) with no write access and points at status pages for exposing selected servers to outsiders, while the admin option spells out full-control scope. Three new String Catalog keys with zh-Hans translations. Also adds DEBUG verification hooks following the existing pattern: SB_UITEST_ADMIN=users pushes the Users screen and SB_UITEST_PRESENT=users-create auto-opens the create sheet, since the '+' navbar button is unreachable for the headless cliclick harness.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Outcome of a design review of the two-tier RBAC user system (
admin/member). Six decisions were settled and recorded indocs/superpowers/specs/2026-07-07-user-system-design-review.md; this PR ships the one code fix plus the disclosure follow-ups.Fix
crates/server/src/service/user.rs): thecount(role='admin')check and the subsequent mutation ran outside a shared transaction, so two concurrent demotions/deletions could both pass the guard and leave the system with zero admins — unrecoverable in-product, sinceinit_adminonly re-bootstraps an empty users table.update_useranddelete_usernow run guard + mutation in one transaction (anddelete_user's multi-table cleanup becomes atomic).Role disclosure (web + iOS + docs)
Member visibility is fleet-wide by design (no per-server scoping; external exposure belongs to status pages). That boundary is now stated where it matters:
SB_UITEST_ADMIN=users,SB_UITEST_PRESENT=users-create)oauth.allow_registrationwith a public provider grants fleet-wide read access to every provider userReview decisions (recorded in the spec)
Verification
cargo test --workspacegreen (~295 tests; the 5spa_*failures seen initially were a missing localapps/web/dist, resolved by building the bundle)cargo clippyclean; web vitest 93 files / 663 tests green;bun run typecheck+ ultracite clean