feat(security): security role model + moderation commands#19
Conversation
…hitecture PLAN-0005 was written before the Host->Hosting+Adapters.* split (ADR-0006) and the Ruleset.Rpg extraction (ADR-0008) - it referenced src/SharpMud.Host, src/SharpMud.Ruleset.Classic, TelnetHostContext, and HostRunner, none of which exist anymore. Re-verified every file reference against the current codebase and corrected them, added the registration call site ADR-0008 introduced (SharpMud.Ruleset.Rpg's ServiceCollectionExtensions.cs), and confirmed IThingRepository is Engine-owned so the admin commands' planned location is still architecturally sound. Also simplifies the SHARPMUD_INITIAL_ADMIN bootstrap: the original two-checkpoint design (a boot-time Program.cs check plus a separate MaybeCreateAsync check) existed because LoginFlow used to be manually parameter-threaded through a per-connection context object. LoginFlow is DI-constructed now, so both cases collapse into one check inside LoginFlow.RunAsync itself, covering fresh-server, restart, and late-set-env-var cases identically. ADR-0005 gets a correction note (not a rewrite - Decision/Context stay untouched per this repo's ADR-immutability convention) recording why the file paths changed and pointing at the plan for the simplified mechanism. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…it (PLAN-0005) First slice of ADR-0005: the SecurityRole [Flags] enum (explicit power-of-two values, not auto-numbered - see the enum's own doc comment for why that's load-bearing), the RoleGuardedCommand/MuteGuardedCommand Decorator wrappers, and ICommandRegistry's Register(ICommand) replaced with RegisterOpen/RegisterWithRole so every command's access level is a legible, intentional statement at its registration call site. PlayerBehavior gains Roles/IsMuted/IsBanned/WasBooted plus mutation methods (GrantRole/RevokeRole/Mute/Unmute/Ban/Unban/MarkBooted) - GrantRole accumulates implied lower tiers (FullAdmin also grants MinorAdmin+Player), RevokeRole enforces the same invariant symmetrically and returns a failure message rather than throwing when a revoke would break it. Roles/IsMuted/IsBanned are persisted (unlike ConnectionState); WasBooted is transient. Migrated both existing registration call sites (BuiltinCommands, SharpMud.Ruleset.Rpg's ServiceCollectionExtensions) to RegisterOpen, and wrapped say/emote specifically in MuteGuardedCommand at the Engine level so mute enforcement is universal, not per-ruleset. HelpCommand now filters role-gated commands out of its listing for actors who can't run them - RoleGuardedCommand passes Verb/Aliases straight through from the wrapped command, so without this every admin command would list unconditionally to every player. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
boot/mute/unmute/announce (MinorAdmin) and ban/unban/rolegrant/rolerevoke (FullAdmin), per ADR-0005. Shared AdminCommandHelpers covers the online-or-offline target lookup (live in World first, falling back to IThingRepository, never attaching an offline result into the world tree) and the combined online check (ConnectionState.Playing AND a connected Session - ConnectionState alone isn't enough, since it's Ignore'd by PlayerBehaviorConfiguration and defaults back to Playing on any freshly-repository-loaded PlayerBehavior), plus the grantable-role allowlist parse that rejects All/None even though both are literally named SecurityRole members. BootCommand/BanCommand call PlayerBehavior.MarkBooted() before disconnecting, so the target's own SessionLoop takes the immediate-removal path instead of Linkdead. BanCommand rejects self-targeting (no in-game recovery path); RoleRevokeCommand rejects revoking your own FullAdmin (same lockout risk) while still allowing every other revoke. AdminCommands.RegisterAll is not auto-wired into BuiltinCommands - a consumer opts in explicitly, passing their own IThingRepository. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
LoginFlow grants SHARPMUD_INITIAL_ADMIN FullAdmin on login (idempotent), rejects banned accounts. SessionLoop treats WasBooted like explicitQuit so boot/ban actually disconnect instead of resuming via Linkdead.
…c sample Registers SharpMudHostOptions (LoginFlow needs it for bootstrap) and AdminCommands.RegisterAll via the Rpg ruleset's registerConsumerCommands callback, so boot/mute/ban/rolegrant are actually reachable in-game.
LoginFlow: initial-admin grant (match/no-match/null/idempotent) and banned-account rejection. SessionLoop: WasBooted takes the same immediate- removal path as explicitQuit. SharpMudHostOptions: SHARPMUD_INITIAL_ADMIN parsing.
Updates SPEC.md, commands.md, accounts-auth.md, deployment.md (SHARPMUD_INITIAL_ADMIN), and plan/roadmap status trackers now that ADR-0005/PLAN-0005's core mechanism is implemented and tested.
All 9 verification steps (bootstrap, restart idempotency, rolegrant, mute/unmute, ban/unban, boot+reconnect regression check, announce, non-admin rejection, restart persistence) passed manually. Flips PLAN-0005/roadmap Slice 3 to Done; task checklist fully checked. Noted a pre-existing, unrelated bug found during verification: the game loop's WanderManager broadcast can crash the process writing to a Linkdead session's dead socket - not part of ADR-0005's scope.
FullBuilder/MinorBuilder now expand to include Player in ImpliedRoles, same as MinorAdmin does - a builder should always behave as an ordinary player too. Admin and builder ladders stay independent (FullAdmin still does not imply FullBuilder, by design). RevokeRole's hierarchy guard now also blocks revoking Player out from under a held builder role.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ncipollina
left a comment
There was a problem hiding this comment.
💬 Full process-conformance + correctness review of PLAN-0005/ADR-0005's implementation. No 🐛 blocking findings - ADR/plan/docs are all correctly updated in this PR (including an honest ADR correction note for the ADR-0006/0008 drift), the role-hierarchy accumulate/revoke logic is correct (including the builder→Player fix in the last commit), boot/ban session-boundary handling is correctly ordered and tested, self-lockout guards are present, and 221 tests pass. Left 3 📝 optional suggestions inline: a set of 6 new xUnit1051 warnings in the Admin command tests (main has zero today), and two small duplication/efficiency nits in BootCommand/AnnounceCommand. None block merge - your call on whether to pick them up now or later.
- Fix xUnit1051: use Arg.Any<CancellationToken>() instead of default in 6 DidNotReceiveWithAnyArgs verification calls across the admin command tests, matching the positive-path assertions in the same files. - Extract AdminCommandHelpers.FindLiveTarget so BootCommand no longer duplicates FindTargetAsync's live-lookup logic. - Add AdminCommandHelpers.GetOnlineBehavior so AnnounceCommand looks up each player's PlayerBehavior once instead of twice per iteration; IsOnline now delegates to it.
📋 Summary
Implements PLAN-0005/ADR-0005: a
SecurityRolepermission model and eight moderation commands (boot,mute/unmute,announce,ban/unban,rolegrant/rolerevoke), plus the bootstrap mechanism that grants the very first admin via an env var. The plan was written before ADR-0006/0008's Host→Hosting+Adapters split and the Ruleset scaffolding tier, so it was re-validated and corrected against the current architecture before implementation started (see the ADR's dated Correction note and the plan's "Validated against current architecture" section).📝 Changes
Permission model
SecurityRole—[Flags] enum : uintwith explicit power-of-two values, plus aSecurityRoleExtensions(C# 14 extension block) defining the role hierarchy:FullAdmin → MinorAdmin → PlayerandFullBuilder → MinorBuilder → Player. The admin and builder ladders are deliberately independent of each other.RoleGuardedCommand/MuteGuardedCommand— decorators wrappingICommandto check role/mute state before delegating, so individual commands never implement permission checks themselves.ICommandRegistry.RegisterOpen/RegisterWithRolereplace the old singleRegistermethod across all call sites.PlayerBehavior:Roles,IsMuted,IsBanned(persisted) +WasBooted(transient, never saved).GrantRoleaccumulates downward through the hierarchy;RevokeRoleenforces the hierarchy invariant and returnsstring?(not an exception) on a normal business-rule failure.Moderation commands (
src/SharpMud.Engine/Commands/Builtin/Admin/)BootCommand/MuteCommand/UnmuteCommand/AnnounceCommand(MinorAdmin),BanCommand/UnbanCommand/RoleGrantCommand/RoleRevokeCommand(FullAdmin), registered viaAdminCommands.RegisterAll.HelpCommandfilters role-gated commands out of the listing for actors who can't run them.Bootstrap + enforcement
SHARPMUD_INITIAL_ADMINenv var (SharpMudHostOptions) —LoginFlowgrants that usernameFullAdminon login, idempotently, since granting a role otherwise requires already holdingFullAdmin.LoginFlowrejects banned accounts at password verification with a distinct message.SessionLooptreatsWasBootedexactly like an explicitquit(immediate removal, notLinkdead) — without this,boot/banwould just look like a dropped connection and the player could reconnect and resume.samples/SharpMud.Samples.Classic/Program.cs(env var passthrough,SharpMudHostOptionsDI registration,AdminCommands.RegisterAllvia the Rpg ruleset'sregisterConsumerCommandscallback).Docs
docs/commands.md,docs/accounts-auth.md,docs/deployment.md(SHARPMUD_INITIAL_ADMINruntime config),SPEC.md,docs/adr/README.md/docs/plans/README.md, anddocs/plans/0001-wheelmud-reconciliation-roadmap.md(Slice 3) all updated to reflect the implemented state.🧪 Validation
dotnet test SharpMud.slnx).rolegrant+ permission boundaries, mute/unmute, ban/unban (including login rejection), boot + immediate-reconnect regression check (confirms a fresh login prompt, not aWelcome backresume), announce broadcast, non-admin rejection of all 5 gated commands, and restart persistence ofRoles/IsMuted/IsBanned(withWasBootedcorrectly not surviving).None/Allrole names, muted player'ssay/emoteblocked with a clear message, offline vs. online vs. linkdead moderation targets.💬 Notes for Reviewers
WanderManagertick can crash the whole process if it broadcasts to a room occupant whose session has already disconnected (Linkdead) — an unhandledIOException/SocketExceptionwriting to a dead socket. Noted in the plan's Verification section for a follow-up.