diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..e2379a0 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,25 @@ +version: 2 +updates: + - package-ecosystem: "nuget" + directory: "/" + schedule: + interval: "weekly" + day: "wednesday" + open-pull-requests-limit: 25 + cooldown: + default-days: 7 + commit-message: + prefix: "chore" + include: "scope" + groups: + dotnet-minor-patch: + update-types: + - "minor" + - "patch" + patterns: + - "*" + dotnet-major: + update-types: + - "major" + patterns: + - "*" diff --git a/.github/release-drafter.yml b/.github/release-drafter.yml new file mode 100644 index 0000000..a686049 --- /dev/null +++ b/.github/release-drafter.yml @@ -0,0 +1,95 @@ +name-template: 'v$RESOLVED_VERSION' +tag-template: 'v$RESOLVED_VERSION' +commitish: main + +version-resolver: + major: + labels: + - 'breaking-change' + minor: + labels: + - 'type: feat' + patch: + labels: + - 'type: fix' + - 'type: docs' + - 'type: refactor' + - 'type: test' + - 'type: chore' + - 'type: ci' + - 'type: revert' + default: patch + +autolabeler: + - label: 'type: feat' + title: + - '/^feat(\(.+\))?(!)?:/i' + - label: 'type: fix' + title: + - '/^fix(\(.+\))?(!)?:/i' + - label: 'type: docs' + title: + - '/^docs(\(.+\))?(!)?:/i' + - label: 'type: refactor' + title: + - '/^refactor(\(.+\))?(!)?:/i' + - label: 'type: test' + title: + - '/^test(\(.+\))?(!)?:/i' + - label: 'type: chore' + title: + - '/^chore(\(.+\))?(!)?:/i' + - label: 'type: ci' + title: + - '/^ci(\(.+\))?(!)?:/i' + - label: 'type: revert' + title: + - '/^revert(\(.+\))?(!)?:/i' + - label: 'breaking-change' + title: + - '/^(feat|fix|docs|refactor|test|chore|ci|revert)(\([^)]*\))?!:/i' + +categories: + - title: '⚠️ Breaking Changes' + labels: + - 'breaking-change' + - title: 'πŸš€ Features' + labels: + - 'type: feat' + - title: 'πŸ› Bug Fixes' + labels: + - 'type: fix' + - title: 'πŸ“š Documentation' + labels: + - 'type: docs' + - title: 'πŸ”„ Refactoring' + labels: + - 'type: refactor' + - title: 'βœ… Tests' + labels: + - 'type: test' + - title: 'πŸ”§ Maintenance' + labels: + - 'type: chore' + - 'type: ci' + - 'type: revert' + +include-labels: + - 'type: feat' + - 'type: fix' + - 'type: docs' + - 'type: refactor' + - 'type: test' + - 'type: chore' + - 'type: ci' + - 'type: revert' + - 'breaking-change' + +exclude-labels: + - 'skip-changelog' + - 'internal' + +template: | + ## Changes + + $CHANGES \ No newline at end of file diff --git a/.github/workflows/pr-title-check.yaml b/.github/workflows/pr-title-check.yaml new file mode 100644 index 0000000..6180520 --- /dev/null +++ b/.github/workflows/pr-title-check.yaml @@ -0,0 +1,13 @@ +name: PR Title Check + +on: + pull_request: + types: [opened, edited, synchronize, reopened] + +permissions: + pull-requests: read + statuses: write + +jobs: + validate: + uses: LayeredCraft/devops-templates/.github/workflows/pr-title-check.yml@v10.1 \ No newline at end of file diff --git a/.github/workflows/release-drafter.yaml b/.github/workflows/release-drafter.yaml new file mode 100644 index 0000000..7db3a98 --- /dev/null +++ b/.github/workflows/release-drafter.yaml @@ -0,0 +1,20 @@ +name: Release Drafter + +on: + push: + branches: + - main + pull_request: + types: [opened, edited, synchronize, reopened, ready_for_review] + workflow_dispatch: + +permissions: + contents: write + pull-requests: write + +jobs: + draft: + uses: LayeredCraft/devops-templates/.github/workflows/release-drafter.yml@v10.1 + with: + event_name: ${{ github.event_name }} + pr_draft: ${{ github.event.pull_request.draft == true }} \ No newline at end of file diff --git a/SPEC.md b/SPEC.md index b8b76dd..046ab3d 100644 --- a/SPEC.md +++ b/SPEC.md @@ -179,9 +179,13 @@ configuration and open items. Explicitly out of scope for v1, to revisit later: -- **Moderation/admin tooling**: permission levels (player/builder/admin), - mute/kick/ban, admin commands, audit logging. Known future need, not - designed yet. +- **Moderation/admin tooling**: permission levels, mute/kick/ban, admin + commands β€” designed, see + [ADR-0005](docs/adr/0005-security-role-model-and-moderation-commands.md) + and + [PLAN-0005](docs/plans/0005-security-role-model-and-moderation-commands.md); + not yet implemented. Audit logging remains undesigned, tracked as an + open item in PLAN-0005. - **Soft-code/scripting engine**: revisit once data/config-driven NPC and room behavior proves insufficient. - **Procedural frontier generation algorithm**: choice of generation approach diff --git a/docs/accounts-auth.md b/docs/accounts-auth.md index 9428fea..2d0739c 100644 --- a/docs/accounts-auth.md +++ b/docs/accounts-auth.md @@ -147,8 +147,15 @@ silently skipped). "non-empty" is enforced. - Password reset flow β€” with no email/OAuth identity backing the account, there's no "forgot password" recovery path; not designed. Likely - admin-assisted reset only (ties into deferred moderation tooling, see - `SPEC.md`). + admin-assisted reset only (ties into the moderation tooling designed in + [ADR-0005](adr/0005-security-role-model-and-moderation-commands.md), not + yet implemented). +- Ban enforcement β€” designed in + [ADR-0005](adr/0005-security-role-model-and-moderation-commands.md)/ + [PLAN-0005](plans/0005-security-role-model-and-moderation-commands.md) + (a banned `PlayerBehavior.IsBanned` rejects login at password + verification), not yet implemented β€” today, login has no concept of a + banned user at all. - The password itself travels in cleartext over Telnet (only the on-screen *display* is suppressed) β€” no transport encryption exists yet; SSH (see [networking.md](networking.md)) would be the natural place this gets diff --git a/docs/adr/0005-security-role-model-and-moderation-commands.md b/docs/adr/0005-security-role-model-and-moderation-commands.md new file mode 100644 index 0000000..278b4b0 --- /dev/null +++ b/docs/adr/0005-security-role-model-and-moderation-commands.md @@ -0,0 +1,343 @@ +# [ADR-0005] Security Role Model + Moderation Commands + +**Status:** Accepted + +**Date:** 2026-07-17 + +**Decision Makers:** solo (design dive conducted with the user) + +## Context + +Per ADR-0001, this is Slice 3 of the WheelMUD reconciliation roadmap. +Verified by direct research (grep across `src/`/`tests/`, full read of +`docs/commands.md`, `docs/accounts-auth.md`, `ICommand.cs`, +`ICommandRegistry.cs`, `CommandGuards.cs`, `PlayerBehavior.cs`): sharp-mud +has **no authorization concept at all** today. Every command any connected +player types is unconditionally resolved and executed β€” +`CommandGuards.cs` has exactly one guard (`RequireArgsAsync`, an +arg-presence check), nothing role-related. `SPEC.md`'s Deferred/Open Items +lists "Moderation/admin tooling... Known future need, not designed yet." + +WheelMUD's own mechanism, full source dive recorded in +[wheelmud-findings.md Β§11](../research/wheelmud-findings.md#11-security-roles--moderation--actionsecurityattribute-actionsadmin): +a `[Flags] enum SecurityRole` (`mobile`/`item`/`room`/`tutorialPlayer`/ +`player`/`helper`/`married`/`minorBuilder`/`fullBuilder`/`minorAdmin`/ +`fullAdmin`/`all`, 12 real values), an `[ActionSecurity(SecurityRole.x)]` +class attribute on each Action, reflected off at registration time into a +`Command.SecurityRole` field, gated at dispatch by a bitwise-AND check +(`command.SecurityRole & user.SecurityRoles`) in `CommandGuard.cs`. +`Actions/Admin/` (`Announce`, `Ban`, `Boot`, `Buff`, `Clone`, `Control`, +`Find`, `GoTo`, `Jail`, `Locate`, `Mute`, `Relinquish`, `RoleGrant`, +`RoleRevoke`, `Spawn`, `Unmute`) are ordinary Actions decorated with +high-tier roles β€” no separate mechanism from any other command. + +## Decision Drivers + +- No new `Thing` subtype (`design-decisions.md` rule 2) β€” role/mute/ban + state lives on `PlayerBehavior`, the existing identity `Behavior`. +- This repo has twice already (ADR-0002, ADR-0004) chosen a leaner + reimplementation over a faithful WheelMUD structural port when the + extra structure doesn't earn its keep here β€” evaluated directly against + WheelMUD's attribute+reflection mechanism as part of this dive. +- The user has a compile-time decorator-generation package + (`LayeredCraft.DecoWeaver`) and asked whether it fit. Verified against + its own docs (`https://decoweaver.layeredcraft.dev/`, + `https://github.com/LayeredCraft/decoweaver`): it only intercepts + `Microsoft.Extensions.DependencyInjection` registration calls + (`AddScoped()` etc. via C# 11 interceptors). + sharp-mud's commands are registered directly into a custom + `ICommandRegistry` (`BuiltinCommands.RegisterAll`/ + `ClassicCommands.RegisterAll` call `registry.Register(...)`), never + through `IServiceCollection` β€” DecoWeaver cannot see them without first + re-plumbing command registration through the DI container, a real, + separate architecture change with no driver of its own right now + (`code-of-conduct.md`: don't bundle an unrelated change into this one). +- A missing guard on a security boundary is a materially worse failure + mode than a missing guard on a UX check (the existing `RequireArgsAsync` + precedent) β€” an admin command silently callable by any player is a real + vulnerability, not a confusing error message. This pushed the decision + toward something structurally harder to forget than "paste a guard call + at the top of `ExecuteAsync`." +- `docs/persistence.md`/`WearableBehaviorConfiguration.cs`'s existing + precedent: `WearableBehavior.Slot` (backed by `EquipSlot`) is a **plain** + C# enum with EF Core's default int mapping, not a + `LayeredCraft.OptimizedEnums` instance β€” confirmed, `OptimizedEnum` in + this repo is for small non-combinable state machines (`Race`, + `CharacterClass`, `ConnectionState`). Corrected during PR review: + `EquipSlot` is **not** itself `[Flags]` (it's an ordinary enum, not a + bitmask), and there is no existing `[Flags]` enum anywhere in this repo + today β€” `SecurityRole` follows `Slot`'s "plain enum over `OptimizedEnum`" + precedent, but is the first genuinely bitwise-combinable `[Flags]` enum + in the codebase, not a second instance of an existing pattern. +- Bootstrap problem, surfaced during the dive: if granting a role itself + requires `FullAdmin`, and every new character defaults to `Player`, + nothing in-game can ever produce the first admin. + +## Considered Options + +1. **Explicit per-command guard call**, mirroring the existing + `CommandGuards.RequireArgsAsync` pattern β€” each gated command's + `ExecuteAsync` starts with `if (await CommandGuards.RequireRoleAsync(...)) + return;`. +2. **`ICommand.RequiredRole` property**, checked centrally by `SessionLoop` + before `ExecuteAsync` ever runs β€” every `ICommand` implementation must + supply it (compile error otherwise). +3. **Attribute + reflection**, a close port of WheelMUD's + `[ActionSecurity(...)]` + registration-time reflection. +4. **Compile-time decorator via `LayeredCraft.DecoWeaver`** β€” requires + first re-plumbing command registration through + `Microsoft.Extensions.DependencyInjection`. +5. **Hand-rolled Decorator pattern**: `RoleGuardedCommand : ICommand` wraps + an inner `ICommand`, checks the actor's roles against a required mask, + and delegates or rejects. `ICommandRegistry`'s single generic + `Register(ICommand)` is replaced with exactly two intentional entry + points, `RegisterOpen(ICommand)` and `RegisterWithRole(ICommand, + SecurityRole)` β€” the latter always applies the wrapper, so there is no + third, accidental way to register a command without declaring its + access level. + +## Decision Outcome + +Chosen option: **"5 β€” hand-rolled Decorator pattern with an intentional +two-method registry API,"** arrived at through the dive: option 1 was +rejected once framed as a security boundary specifically (forgettable by +construction); option 2 works but forces every existing `ICommand` +(~14 classes, including harmless ones like `look`) to declare a role it +doesn't need, just to satisfy the interface; option 3 repeats this repo's +now-established pattern of not faithfully porting WheelMUD's structure +when it doesn't earn its keep; option 4 doesn't fit sharp-mud's actual +command-registration model without an unrelated prerequisite change. The +Decorator pattern is the *actual* thing the user was reaching for with +DecoWeaver β€” DecoWeaver is one implementation strategy for it (compile-time +codegen over DI-registered services), not the pattern itself; hand-writing +it here gets the real benefit (composable, independently-testable +cross-cutting command wrappers β€” the next one, e.g. a cooldown check, is +just another wrapper of the same shape) without DecoWeaver's registration +-model requirement. + +**Mechanism**, in full: +- `SecurityRole` (`SharpMud.Engine.Commands`): a plain `[Flags] enum`, + full WheelMUD 12-value set ported (PascalCase: `None = 0`, `Mobile = 1 + << 0`, `Item = 1 << 1`, `Room = 1 << 2`, `TutorialPlayer = 1 << 3`, + `Player = 1 << 4`, `Helper = 1 << 5`, `Married = 1 << 6`, `MinorBuilder + = 1 << 7`, `FullBuilder = 1 << 8`, `MinorAdmin = 1 << 9`, `FullAdmin = + 1 << 10`, `All = Mobile | Item | Room | TutorialPlayer | Player | + Helper | Married | MinorBuilder | FullBuilder | MinorAdmin | + FullAdmin`). **Explicit values are load-bearing, not stylistic** β€” + caught in PR review: without them, C# auto-numbers enum members + sequentially (`0, 1, 2, 3...`), which are *not* distinct bits (`Room` + would auto-number to `3`, silently equal to `Mobile | Item` combined, + and the `RoleGuardedCommand` bitwise-AND check would grant unrelated + permissions on overlapping bits). `All` is defined as the union of the + individual flags, not a separate hardcoded value (e.g. `uint.MaxValue`) + β€” so it can't drift out of sync if a flag is ever added later. Adopted + in full even though several values (`Mobile`/`Item`/`Room`: sharp-mud + has no non-player command issuer today; `Married`: no marriage system; + `TutorialPlayer`/builder tiers: not yet used) are inert now, so future + slices (world-building/OLC is Slice 4, explicitly bundled with this one + in ADR-0001) already have a role to reach for instead of extending the + enum again. +- `PlayerBehavior` gains `SecurityRole Roles` (persisted β€” unlike + `ConnectionState`/`Session`, a role assignment must survive a restart or + it's useless; default `Player` for new characters), plus `bool IsMuted` + and `bool IsBanned` (also persisted; separate from `Roles` β€” a + restriction, not a capability). +- **Roles accumulate at grant time, matching WheelMUD's own behavior** + (confirmed during research: WheelMUD's `UserControlledBehavior + .SecurityRoles` is a bitwise-OR accumulation, e.g. "a promoted builder + keeps `player | minorBuilder | fullBuilder`," not just the newest tier's + bit alone). `PlayerBehavior.GrantRole(SecurityRole role)` ORs in the + requested role *and* every tier it implies: `FullAdmin` implies + `MinorAdmin` implies `Player`; `FullBuilder` implies `MinorBuilder`. + This matters because `RoleGuardedCommand`'s check stays a simple + bitwise AND with no hierarchy logic of its own β€” without accumulation, + a user granted only `FullAdmin` would fail every `MinorAdmin`-gated + command (`boot`/`mute`/`unmute`/`announce`), since `FullAdmin` and + `MinorAdmin` are independent bits with no inherent relationship. Caught + during PR review (the bootstrap admin would otherwise be unable to run + day-to-day moderation commands) β€” see `SHARPMUD_INITIAL_ADMIN` below, + which relies on this same accumulation. The same invariant has to hold + on the way out, not just the way in β€” also caught during PR review: + `PlayerBehavior.RevokeRole(SecurityRole role)` rejects (rather than + silently applying) a revoke of a tier still implied by a higher one the + target currently holds β€” e.g. revoking `MinorAdmin` from someone who + still has `FullAdmin` would otherwise leave `FullAdmin` set with + `MinorAdmin` cleared, breaking "`FullAdmin` implies `MinorAdmin`" for + that user going forward. The rejection names the blocking higher tier + so the admin knows to revoke that one first (or instead). +- `RoleGuardedCommand` checks `(actor.Roles & requiredRole) != + SecurityRole.None` (any-of semantics, matching WheelMUD's own bitwise + check) before delegating to the inner command. The same Decorator shape + is reused for mute enforcement (`MuteGuardedCommand`, wrapping `say`/ + `emote`, checking the *actor's own* `IsMuted` β€” it's the speaker being + blocked from speaking, not anything about a target) β€” validating the + pattern's reusability for a cross-cutting concern that isn't role-based + at all. +- `ICommandRegistry.Register(ICommand)` is removed from the public + interface; `RegisterOpen(ICommand)` and `RegisterWithRole(ICommand, + SecurityRole)` are the only ways in. Every existing command's + registration call site changes from `Register` to `RegisterOpen` + (mechanical, no behavior change for them). +- Ban is enforced in `LoginFlow` at successful password verification + (`IsBanned` β†’ reject, distinct message). + +**Command set for this slice** β€” the subset of WheelMUD's 16 Admin +actions that map onto systems sharp-mud already has; the rest need +prerequisite infrastructure this slice doesn't build (Find/Locate/GoTo/ +Control need world/NPC lookup and puppeting; Clone/Spawn need item/NPC +creation tooling; Jail needs a cell-room concept; Buff needs a generic +stat-modification system; Relinquish is builder-role-specific and tied to +Slice 4): + +| Command | Required role | Notes | +|---|---|---| +| `Boot` | `MinorAdmin` | Disconnects a currently-online target. | +| `Mute` / `Unmute` | `MinorAdmin` | Sets/clears `IsMuted`; enforced on `say`/`emote` via `MuteGuardedCommand`. | +| `Announce` | `MinorAdmin` | Broadcasts to every connected session. | +| `Ban` / `Unban` | `FullAdmin` | Sets/clears `IsBanned`, enforced in `LoginFlow`. `Unban` has no WheelMUD equivalent β€” added here because a ban with no in-game reversal is an operability trap, not because WheelMUD has one. | +| `RoleGrant` / `RoleRevoke` | `FullAdmin` | Mutates a target's `Roles`. Gated at `FullAdmin` specifically so a `MinorAdmin` can never self-escalate. | + +Day-to-day moderation (`Boot`/`Mute`/`Unmute`/`Announce`) sits at +`MinorAdmin`; harder-to-reverse or privilege-affecting actions +(`Ban`/`Unban`/`RoleGrant`/`RoleRevoke`) require `FullAdmin`. Target lookup +(online or not) mirrors `LoginFlow.FindAndAttachExistingAsync`'s existing +live-then-repository pattern; an offline target is loaded, mutated, and +saved without being attached into the live world tree (no need to, unlike +login). + +**Bootstrap**: `HostOptions` gains `SHARPMUD_INITIAL_ADMIN` (env var, +matching the existing `SHARPMUD_MODE`/`SHARPMUD_TELNET_PORT`/ +`SHARPMUD_DB_PATH` precedent). The grant is checked in **two** places, not +just one β€” caught during PR review that checking only once at boot is a +no-op on a genuinely fresh server, since the target character doesn't +exist yet at boot time and only gets created later through the normal +login flow: +1. At boot (after world load), if a character with that username already + exists (a restart of an existing world) β€” the original case. +2. At the moment a character with that username is actually created + (`LoginFlow.MaybeCreateAsync`), covering the fresh-server case. + +Both paths are idempotent and go through the same `GrantRole(FullAdmin)` +call (which accumulates `MinorAdmin`/`Player` too, per the accumulation +rule above) β€” safe to leave `SHARPMUD_INITIAL_ADMIN` set permanently, or +unset after first use. Solves "how does the first admin ever get +`FullAdmin`" without an in-game path that would otherwise be a +chicken-and-egg dead end. + +### Positive Consequences + +- Closes a real, previously-total gap: no command in this codebase was + ever gated by anything before this. +- The Decorator mechanism is reusable for the *next* cross-cutting + command concern (a cooldown, a "must not be in combat" check) without + inventing a new approach each time β€” validated within this same slice + by reusing it for mute enforcement, not just role-gating. +- `RegisterOpen`/`RegisterWithRole` replacing a single generic `Register` + means every command registration is a legible, intentional statement of + its own access level β€” there's no third, silent way in. +- Adopting the full WheelMUD role set now means Slice 4 (world-building, + already flagged in ADR-0001 as needing security gating) has + `MinorBuilder`/`FullBuilder` ready rather than needing another enum + change. + +### Negative Consequences + +- `RegisterWithRole` still depends on the person registering a command + choosing the right entry point β€” not literally impossible to misuse + (e.g. calling `RegisterOpen` on something that should have been gated), + just much harder to do by accident than a scattered guard call, since + the two entry points sit side by side at every registration call site. +- Most of the 12-value `SecurityRole` set (`Mobile`/`Item`/`Room`/ + `TutorialPlayer`/`Married`) has no consumer yet β€” accepted as the cost + of not re-deriving the enum later, but it is real unused surface area + today. +- `SHARPMUD_INITIAL_ADMIN` is a standing env var that, if left set and a + malicious actor ever created a character with that exact username on a + fresh world, would hand them `FullAdmin` β€” acceptable for a + solo/small-group server at this stage, worth revisiting if/when this + goes to a wider public deployment (`docs/deployment.md`). + +## Pros and Cons of the Options + +### Option 1: Explicit per-command guard call + +- Good, because it requires no interface or registry changes at all. +- Good, because it matches an already-established repo pattern + (`RequireArgsAsync`). +- Bad, because forgetting it is silent and the failure mode is a real + security hole, not a UX inconvenience β€” the exact risk profile that + makes this the wrong pattern to reuse here. + +### Option 2: `ICommand.RequiredRole` property + +- Good, because it's structurally impossible to omit β€” a compile error, + not a runtime gap. +- Good, because the check lives in exactly one place (`SessionLoop`'s + dispatch), not scattered per-command. +- Bad, because it forces every existing `ICommand` (including commands + that need no gating at all) to declare a role, touching ~14 files for + no functional benefit to those commands. + +### Option 3: Attribute + reflection (WheelMUD-faithful) + +- Good, because it's a direct, low-judgment translation of prior art. +- Bad, because this repo has already twice (ADR-0002, ADR-0004) rejected + faithfully porting WheelMUD's structure in favor of something leaner + fit to sharp-mud's actual needs, and the same reasoning applies here: + reflection-based registration-time discovery solves a + plugin-discoverability problem sharp-mud's fixed, hand-wired + `RegisterAll` calls don't have. + +### Option 4: Compile-time decorator via DecoWeaver + +- Good, because it's genuinely zero-runtime-overhead and would give + broader decoration for free if the app already used DI-container-based + registration everywhere. +- Bad, because sharp-mud's commands aren't DI-container-registered today, + and re-plumbing that is a separate, unrelated architecture change with + no driver of its own yet. + +### Option 5: Hand-rolled Decorator pattern (chosen) + +- Good, because it needs zero `ICommand` interface changes and zero + registration-model changes β€” a genuinely additive change. +- Good, because it's the actual design pattern the user was reaching for, + independent of any specific codegen tool, and it's proven reusable + within this same slice (mute enforcement, not just role-gating). +- Good, because the `RegisterOpen`/`RegisterWithRole` split closes almost + all of option 1's "forgettable" risk without option 2's forced, + unnecessary interface changes. +- Bad, because enforcement is still a registration-time discipline, not a + compile-time guarantee β€” mitigated, not eliminated, by there being only + two, clearly-named ways to register anything. + +## Links + +- [ADR-0001](0001-wheelmud-reconciliation-roadmap.md) β€” WheelMUD + Reconciliation Roadmap (this is Slice 3; Slice 4 world-building is + explicitly bundled with this one and will consume the same mechanism). +- [PLAN-0005](../plans/0005-security-role-model-and-moderation-commands.md) + β€” execution plan for this decision. +- `SPEC.md` β€” "Moderation/admin tooling" Deferred/Open Item this ADR + resolves (not yet implemented β€” see the plan). +- `docs/accounts-auth.md` β€” the existing forward-reference to "deferred + moderation tooling" this ADR makes concrete; ban enforcement lands in + `LoginFlow`. +- `docs/commands.md` β€” command pipeline (`ICommand`/`ICommandRegistry`) + this ADR extends. +- `docs/deployment.md` β€” Runtime Configuration table `SHARPMUD_INITIAL_ADMIN` + joins (caught in PR review β€” it's the only bootstrap path to a + `FullAdmin` in a fresh deployment, and this table is the documented + list of every `HostOptions` env var). +- `docs/persistence.md` / `WearableBehaviorConfiguration.cs` β€” the + existing plain-enum-with-default-EF-mapping precedent `SecurityRole` + follows for "plain enum, not `OptimizedEnum`" (`EquipSlot` itself is not + `[Flags]` β€” corrected during PR review). +- `WheelMUD/src/Core/Attributes/ActionSecurityAttribute.cs`, + `WheelMUD/src/Core/ManagerSystems/CommandManager.cs`, + `WheelMUD/src/Core/CommandSystem/CommandGuard.cs`, + `WheelMUD/src/Core/Behaviors/UserControlledBehavior.cs`, + `WheelMUD/src/Actions/Admin/` β€” source researched for this decision. +- `https://decoweaver.layeredcraft.dev/`, + `https://github.com/LayeredCraft/decoweaver` β€” DecoWeaver documentation + consulted and ruled out for this slice (registration-model mismatch). diff --git a/docs/adr/README.md b/docs/adr/README.md index df0ee0f..681673f 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -64,3 +64,4 @@ the mechanics: numbering, status, and the index. | [0002](0002-telnet-protocol-negotiation.md) | Telnet Protocol Negotiation (IAC/Q-Method core + NAWS) | Accepted | | [0003](0003-allow-appsettingsjson-for-non-secret-config.md) | Allow `appsettings.json` for Non-Secret Configuration | Accepted | | [0004](0004-session-state-machine-and-reconnect.md) | Session State Machine + Linkdead Reconnect | Accepted | +| [0005](0005-security-role-model-and-moderation-commands.md) | Security Role Model + Moderation Commands | Accepted | diff --git a/docs/commands.md b/docs/commands.md index 48396a8..fd123f2 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -124,9 +124,14 @@ actually implemented as of the inventory/items build-order phase: - **Character**: `score`/`stats` (display derived stats β€” see [character.md](character.md)) is **not implemented yet**. - **Meta** βœ…: `help`, `quit`. -- **Builder/admin verbs** (`@dig`, `@describe`, etc.) explicitly excluded β€” +- **Builder/OLC verbs** (`@dig`, `@describe`, etc.) explicitly excluded β€” those belong to the deferred in-game building phase (see - [world-model.md](world-model.md)). + [world-model.md](world-model.md)). **Moderation/admin verbs** + (`boot`/`mute`/`unmute`/`announce`/`ban`/`unban`/`rolegrant`/ + `rolerevoke`) are a separate thing β€” designed in + [ADR-0005](adr/0005-security-role-model-and-moderation-commands.md), not + yet implemented (see + [PLAN-0005](plans/0005-security-role-model-and-moderation-commands.md)). ## Open Items diff --git a/docs/plans/0001-wheelmud-reconciliation-roadmap.md b/docs/plans/0001-wheelmud-reconciliation-roadmap.md index abc713c..e738f8d 100644 --- a/docs/plans/0001-wheelmud-reconciliation-roadmap.md +++ b/docs/plans/0001-wheelmud-reconciliation-roadmap.md @@ -29,7 +29,8 @@ reconciliation effort stands. Accepted, PLAN-0004 Done. See [PLAN-0004](0004-session-state-machine-and-reconnect.md). - [ ] **Slice 3 β€” Permission/security-role model + moderation commands.** - Not yet designed. + ADR-0005 Accepted, PLAN-0005 Not Started. See + [PLAN-0005](0005-security-role-model-and-moderation-commands.md). - [ ] **Slice 4 β€” World-building/OLC command surface.** Not yet designed; bundles with Slice 3. - [ ] **Slice 5 β€” Help system.** Not yet designed. diff --git a/docs/plans/0005-security-role-model-and-moderation-commands.md b/docs/plans/0005-security-role-model-and-moderation-commands.md new file mode 100644 index 0000000..2894efd --- /dev/null +++ b/docs/plans/0005-security-role-model-and-moderation-commands.md @@ -0,0 +1,499 @@ +# [PLAN-0005] Security Role Model + Moderation Commands + +**Implements:** [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md) + +**Status:** Not Started + +**Last updated:** 2026-07-17 + +## Goal + +A `MinorAdmin`/`FullAdmin`-gated set of moderation commands +(`boot`/`mute`/`unmute`/`announce`/`ban`/`unban`/`rolegrant`/`rolerevoke`) +works end-to-end over real Telnet, no command can be registered without an +explicit access-level declaration, mute/ban are enforced against the +right targets, and a `SHARPMUD_INITIAL_ADMIN` env var bootstraps the first +`FullAdmin` without an in-game path. + +## Scope + +Per ADR-0005's Decision Outcome. In scope: `SecurityRole` enum, +`RoleGuardedCommand`/`MuteGuardedCommand` decorators, the +`RegisterOpen`/`RegisterWithRole` registry split (and updating every +existing registration call site), `PlayerBehavior.Roles`/`IsMuted`/ +`IsBanned`, the 8 moderation commands above, `LoginFlow` ban enforcement, +`SHARPMUD_INITIAL_ADMIN` bootstrap. + +Explicitly deferred (per ADR-0005): `Find`/`Locate`/`GoTo`/`Control` +(need world/NPC lookup + puppeting), `Clone`/`Spawn` (need item/NPC +creation tooling), `Jail` (needs a cell-room concept), `Buff` (needs a +generic stat-modification system), `Relinquish` (Slice 4/builder-specific). +Audit logging of moderation actions β€” not in ADR-0005's scope, flagged as +an open item below. + +## Tasks + +### `SecurityRole` + registry + +- [ ] New `src/SharpMud.Engine/Commands/SecurityRole.cs` β€” plain + `[Flags] enum SecurityRole : uint` with **explicit power-of-two + values on every member** (`None = 0`, `Mobile = 1 << 0`, `Item = 1 + << 1`, `Room = 1 << 2`, `TutorialPlayer = 1 << 3`, `Player = 1 << + 4`, `Helper = 1 << 5`, `Married = 1 << 6`, `MinorBuilder = 1 << 7`, + `FullBuilder = 1 << 8`, `MinorAdmin = 1 << 9`, `FullAdmin = 1 << + 10`, `All = Mobile | Item | Room | TutorialPlayer | Player | Helper + | Married | MinorBuilder | FullBuilder | MinorAdmin | FullAdmin`). + **Do not rely on C#'s default sequential auto-numbering** β€” caught + in PR review: unnumbered members would auto-assign `0, 1, 2, 3...`, + which are not distinct bits (e.g. `Room` would silently equal + `Mobile | Item` combined), breaking `RoleGuardedCommand`'s bitwise + -AND check into granting unrelated permissions on overlapping bits. + `All` is a union expression, not a separate hardcoded value, so it + can't drift out of sync if a flag is added later. XML doc comments + per `documentation.md`'s new-public-member rule. +- [ ] New `src/SharpMud.Engine/Commands/RoleGuardedCommand.cs` β€” wraps an + inner `ICommand`, checks `(actor.Roles & requiredRole) != + SecurityRole.None` before delegating; generic rejection message. + Exposes `public SecurityRole RequiredRole { get; }` β€” needed by + `HelpCommand`'s filtering below, not just internally. +- [ ] New `src/SharpMud.Engine/Commands/MuteGuardedCommand.cs` β€” wraps an + inner `ICommand`, checks the *actor's own* `IsMuted` (not a target's + β€” this gates the muted player's own `say`/`emote`, not something + they're doing to someone else) before delegating. +- [ ] `ICommandRegistry.cs` / `CommandRegistry.cs`: remove `Register + (ICommand)` from the public surface; add `RegisterOpen(ICommand)` + and `RegisterWithRole(ICommand, SecurityRole)` (the latter wraps in + `RoleGuardedCommand` before storing). +- [ ] Update every existing registration call site (`BuiltinCommands + .RegisterAll`, `ClassicCommands.RegisterAll`) from `Register` to + `RegisterOpen` β€” mechanical, no behavior change. +- [ ] `HelpCommand.cs`: filter `registry.Commands` by the actor's roles + before listing them β€” caught in self-review. `RoleGuardedCommand` + passes `Verb`/`Aliases` straight through from the wrapped command, so + without this, `help` lists every admin command (`ban`, `boot`, + `rolegrant`, ...) to every player. Not an exploit (the gate still + blocks execution) but a real, unpolished info leak β€” skip a command + in the listing if it's a `RoleGuardedCommand` and `(ctx.Actor`'s + `Roles & command.RequiredRole) == SecurityRole.None`; anything not + role-guarded still lists unconditionally. + +### `PlayerBehavior` + persistence + `SessionLoop` + +- [ ] `PlayerBehavior.cs`: add `SecurityRole Roles { get; private set; } = + SecurityRole.Player`, `bool IsMuted { get; private set; }`, `bool + IsBanned { get; private set; }`, plus mutation methods + (`GrantRole`/`RevokeRole`, `Mute`/`Unmute`, `Ban`/`Unban`) rather than + public setters, matching `ConnectionState`'s existing + transition-method style. + - [ ] Also add `bool WasBooted { get; private set; }` (transient, + like `Session`/`ConnectionState` β€” `Ignore`d in + `PlayerBehaviorConfiguration`) and a `MarkBooted()` method. + **Real gap caught in PR review**: `BootCommand` runs inside + the *admin's* `SessionLoop`, calling `DisconnectAsync` on the + *target's* session β€” but `SessionLoop`'s `explicitQuit` flag + is a local variable scoped to each connection's own + `RunAsync` call. The target's own loop never sees an + admin-triggered disconnect as a "quit," so today it would + take the `Linkdead` branch (per ADR-0004) β€” the booted player + could just reconnect within the grace window and resume + exactly where they were, making `boot` a no-op as a + moderation tool. `WasBooted` is the signal that crosses that + boundary: `BootCommand` sets it on the target's + `PlayerBehavior` *before* calling `DisconnectAsync`; the + target's own `SessionLoop.RunAsync` (a completely separate + call stack) checks it in its `finally` block. + - [ ] `SessionLoop.cs`: in the `finally` block, treat `WasBooted` + exactly like `explicitQuit` β€” both mean "this disconnect was + intentional, skip `Linkdead` and remove immediately" (same + save-then-remove ordering `explicitQuit` already uses, not + `EnterLinkdead`'s mutate-before-save ordering). Concretely: + replace the bare `explicitQuit` checks in both branches with + `explicitQuit || (playerBehavior?.WasBooted ?? false)`. + - [ ] `GrantRole(SecurityRole role)`: ORs in `role` *and* every tier + it implies (`FullAdmin` β†’ also `MinorAdmin` + `Player`; + `FullBuilder` β†’ also `MinorBuilder`) per ADR-0005's + accumulation rule β€” a plain `Roles |= role` is not enough on + its own. + - [ ] A static `SecurityRole.Implies(SecurityRole role)` (or + equivalent lookup) expressing the same hierarchy + (`FullAdmin`β†’`MinorAdmin`β†’`Player`, `FullBuilder`β†’ + `MinorBuilder`) β€” used by both `GrantRole` (to accumulate + downward) and `RevokeRole` (to check upward, see next) so the + hierarchy is defined once, not duplicated between the two + directions. + - [ ] `RevokeRole(SecurityRole role)`: **enforces the same + invariant symmetrically** (caught in PR review β€” clearing + only the exact bit passed in can leave a higher tier set with + a lower tier it implies cleared, e.g. `FullAdmin` present but + `MinorAdmin` cleared after revoking just `MinorAdmin`). Before + clearing, check whether any *other* currently-held role + implies `role`; if so, **return a failure, don't throw** β€” + per `coding-standards.md`'s Error Handling section, this is a + normal, directly-user-triggerable business-rule outcome (an + admin typed a `rolerevoke` that doesn't make sense given the + target's current roles), not a bug or an invariant violation, + so it must be a return value the caller checks (caught in PR + review β€” the original wording said "throw/reject," which + contradicts the standard). Signature: + `string? RevokeRole(SecurityRole role)` β€” `null` on success, + a message naming the blocking higher tier on failure ("still + has FullAdmin, which includes MinorAdmin β€” revoke FullAdmin + instead"). Corrected during self-review: earlier wording cited + this as mirroring "`MoveRequest.CancelReason`," but no + `MoveRequest` type exists β€” the actual precedent + (`UseExitEvent.CancelReason`, `src/SharpMud.Engine/Core + /Events.cs`) is a *property* set via `.Cancel(reason)` on a + published cancellable event, a different shape (pub/sub + event-object mutation, not a direct method return). A plain + nullable-string return is simpler and doesn't need that + machinery here β€” it just needs to be a return value, per + `coding-standards.md`, not a citation to a nonexistent type. + `RoleRevokeCommand` relays a non-null return straight to the + admin; never wraps this call in a try/catch. +- [ ] `PlayerBehaviorConfiguration.cs`: map `Roles` with the plain-enum + default EF conversion (matching `WearableBehaviorConfiguration`'s + `Slot` precedent β€” no custom value converter needed); map `IsMuted`/ + `IsBanned` as plain persisted columns (NOT `Ignore`d β€” unlike + `ConnectionState`, these must survive a restart). `Ignore(x => + x.WasBooted)` alongside `Session`/`ConnectionState` β€” transient, + same category, never meaningful across a restart. + +### Moderation commands (`src/SharpMud.Engine/Commands/Builtin/Admin/`) + +`Mute`/`Unmute`/`Ban`/`Unban`/`RoleGrant`/`RoleRevoke` need `IThingRepository` +for offline target lookup + immediate saves β€” `CommandContext` only carries +`World`/`Session`, not the repository, and today the repository is only +available inside `SessionLoop`. Rather than extending `CommandContext` +(bigger blast radius, touches every command's context), these six commands +take `IThingRepository` via their own constructor β€” the same shape +`ClassicCommands.RegisterAll` already uses for `combatManager`/`random`. +`Boot`/`Announce` don't need it (online-only / broadcast-only). + +**"Online" means `ConnectionState == Playing` *and* `Session is { +IsConnected: true }` β€” both, not either alone.** `WhoCommand`'s iteration +(`world.AllWithBehavior()`, no further filter) was +originally cited here as the pattern to copy, but `WhoCommand` itself has +no liveness filter at all, and since ADR-0004 that call also returns +**`Linkdead`** players (disconnected but not yet swept) β€” `WhoCommand` +mislabeling those as "online" is a separate, pre-existing gap, out of +scope for this slice to fix. `BootCommand`/`AnnounceCommand` must *not* +copy that omission. + +An earlier version of this note said `ConnectionState == Playing` alone +was sufficient β€” **wrong, caught in PR review**: `ConnectionState` is +`Ignore`d by `PlayerBehaviorConfiguration` (runtime-only, not persisted), +so it defaults back to `Playing` on any freshly-constructed +`PlayerBehavior` β€” including a player just reloaded from the repository +with no live session at all. This isn't hypothetical: +`LoginFlow.FindAndAttachExistingAsync` already registers a +repository-loaded player into `World` *before password verification even +runs* β€” during that window (and after a server restart, before that +player reconnects) the Thing sits in `World` with `ConnectionState +.Playing` and `Session == null`. Checking `ConnectionState` alone would +make `Boot`/`Announce` treat that player as online and attempt a +null-session disconnect/write. The fix: use the exact same combined check +`LoginFlow.LoginExistingAsync` already established for this +(`playerBehavior.ConnectionState == ConnectionState.Playing && +playerBehavior.Session is { IsConnected: true }`) β€” don't invent a +narrower one. + +- [ ] `BootCommand` (`MinorAdmin`, no repository dependency) β€” disconnects + a currently-online (`ConnectionState == Playing && Session is { + IsConnected: true }`) target by username; "not online" message + otherwise (not found at all, found only `Linkdead`, or found but + with a null/disconnected `Session`). **Calls + `target.FindBehavior()!.MarkBooted()` before** + `target's session.DisconnectAsync(...)` β€” without this the boot is + cosmetic (caught in PR review; see the `PlayerBehavior`/ + `SessionLoop` task above for why). Writes a message to the target's + session first (mirrors `QuitCommand`'s "Goodbye!" before + disconnecting), e.g. "You have been disconnected by an + administrator." +- [ ] `MuteCommand`/`UnmuteCommand` (`MinorAdmin`, `IThingRepository`) β€” + sets/clears `IsMuted` on a target (online-or-not, mirrors + `LoginFlow`'s live-then-repository lookup), saves immediately. +- [ ] `AnnounceCommand` (`MinorAdmin`, no repository dependency) β€” + broadcasts to every `world.AllWithBehavior()` entry + whose `ConnectionState == Playing && Session is { IsConnected: true + }` β€” explicitly **not** every entry `WhoCommand`-style, and + explicitly **not** `ConnectionState` alone (see above). +- [ ] `BanCommand` (`FullAdmin`, `IThingRepository`) β€” sets `IsBanned`, + online-or-not lookup, saves immediately. **If the target is + currently online (`ConnectionState == Playing && Session is { + IsConnected: true }`), also disconnects them the same way + `BootCommand` does** β€” `MarkBooted()` then session write + `Disconn + ectAsync(...)` (caught in PR review: `SessionLoop` never re-checks + `IsBanned` mid-session, so without this an already-connected banned + player keeps issuing commands until an admin separately remembers + to `boot` them). **Rejects self-targeting** + (caught in self-review: `Ban` has no in-game recovery β€” + `SHARPMUD_INITIAL_ADMIN` only re-grants roles, it doesn't clear + `IsBanned` β€” so an admin banning themselves is locked out short of a + manual DB edit; `boot`/`mute` self-targeting is left alone, both are + harmless and trivially reversible by the same admin). `UnbanCommand` + needs no such guard (undoing your own ban isn't reachable β€” you + can't be logged in while banned). +- [ ] `RoleGrantCommand`/`RoleRevokeCommand` (`FullAdmin`, + `IThingRepository`) β€” mutates a target's `Roles` via + `GrantRole`/`RevokeRole` (accumulation/hierarchy-invariant + enforcement happens inside `PlayerBehavior` itself, not here), + online-or-not lookup, saves immediately. Validate the role name + argument against an **explicit allowlist of individually-grantable + roles β€” not just "is this a real `SecurityRole` name."** Caught in + PR review: a plain `Enum.TryParse` would also accept + `All` (every current *and future* flag β€” not a real assignable + tier, a severe over-grant) and `None` (a meaningless no-op + sentinel), since both are literally named enum members. Reject + either with a clear message rather than silently persisting them. + **`RoleRevokeCommand` rejects revoking your own `FullAdmin`** (caught + in self-review β€” same class of lockout risk as self-`Ban`: a sole + `FullAdmin` revoking their own tier has no in-game path back without + another `FullAdmin` already present to re-grant it). Revoking any + other role from yourself, or revoking `FullAdmin` from someone + *else*, is unaffected. `RoleRevokeCommand` checks `RevokeRole`'s + `string?` return (not a + caught exception β€” see the `PlayerBehavior` task above) and relays + a non-null failure straight to the admin as the rejection message. +- [ ] Register all 8 via `RegisterWithRole` in a new + `AdminCommands.RegisterAll(registry, repository)` (mirrors + `BuiltinCommands`/`ClassicCommands`'s shape β€” `ClassicCommands + .RegisterAll` already takes extra constructed dependencies the same + way), called from `Program.cs` alongside the existing `RegisterAll` + calls, passing the already-constructed `repository`. Wrap `say`/ + `emote`'s existing registrations in `MuteGuardedCommand` at the same + call site. + +### Login-flow + bootstrap + +- [ ] `LoginFlow.LoginExistingAsync`: after password verification + succeeds, check `IsBanned` β†’ reject with a distinct message before + the `ConnectionState` branch. +- [ ] `HostOptions.cs`: add `string? InitialAdminUsername`, parsed from + `SHARPMUD_INITIAL_ADMIN`. +- [ ] `Program.cs`: add `["SHARPMUD_INITIAL_ADMIN"] = + Environment.GetEnvironmentVariable("SHARPMUD_INITIAL_ADMIN")` to the + `env` dictionary built before `HostOptions.Parse(args, env)` β€” caught + in PR review. `Program.cs` builds a **fixed** dictionary of only the + three existing env vars (`SHARPMUD_MODE`/`SHARPMUD_TELNET_PORT`/ + `SHARPMUD_DB_PATH`), not a full environment pass-through, so adding + the parse logic to `HostOptions.cs` alone isn't enough β€” + `SHARPMUD_INITIAL_ADMIN` needs its own entry here or the real host + never sees it, even with the env var actually set in production. +- [ ] Bootstrap the grant in **two** places, not just one β€” a boot-time-only + check is a no-op on a genuinely fresh server, since the target + character doesn't exist yet at boot and only gets created later + through the normal login flow (caught in PR review): + - [ ] `Program.cs`: after world load/build, if `InitialAdminUsername` + is set and that username's character already exists (live or + via repository β€” the "restart of an existing world" case), + idempotently `GrantRole(FullAdmin)` + save if not already + present. + - [ ] `LoginFlow.MaybeCreateAsync`: after a new character is created + and saved, if its username matches `InitialAdminUsername`, + `GrantRole(FullAdmin)` + save again (the fresh-server case). + **This needs `InitialAdminUsername` threaded all the way down + to `MaybeCreateAsync` β€” not hidden global state** (caught in + PR review): the actual Telnet call chain is + `HostRunner.RunTelnetAsync` β†’ `HandleConnectionAsync` β†’ + `LoginFlow.RunAsync(session, context.World, context.Repository, + context.StartingRoom, ct)` β†’ `MaybeCreateAsync`, and neither + `TelnetHostContext` nor `LoginFlow.RunAsync`'s signature + carries `HostOptions`/`InitialAdminUsername` today. Concretely: + - [ ] `TelnetHostContext` (`src/SharpMud.Host + /TelnetHostContext.cs`) gains a `string? + InitialAdminUsername` field. + - [ ] `LoginFlow.RunAsync`/`MaybeCreateAsync` gain an + `InitialAdminUsername` parameter (already at/near the + 4-param limit β€” a small parameter object may be + warranted here too, matching `TelnetHostContext`'s own + precedent per `coding-standards.md`'s 4-param rule, + rather than pushing a 5th positional parameter through). + - [ ] `HostRunner.HandleConnectionAsync` passes + `context.InitialAdminUsername` through to + `LoginFlow.RunAsync`. + - [ ] `Program.cs`'s `TelnetHostContext` construction passes + `hostOptions.InitialAdminUsername`. + Both paths call the same `PlayerBehavior.GrantRole(SecurityRole + .FullAdmin)`, so both get the accumulation behavior (also granting + `MinorAdmin`/`Player`) for free. + +### Docs + +- [ ] `docs/commands.md`: describe the new admin command set as current + state, link ADR-0005. +- [ ] `docs/accounts-auth.md`: describe ban enforcement in the login flow + as current state, update the existing "deferred moderation tooling" + forward-reference to point at ADR-0005/this plan. +- [ ] `docs/deployment.md`: add `SHARPMUD_INITIAL_ADMIN` to the Runtime + Configuration table (caught in PR review β€” this table is the + documented list of every `HostOptions` env var, and + `SHARPMUD_INITIAL_ADMIN` is the *only* bootstrap path to a + `FullAdmin` in a fresh deployment; omitting it here means deploying + a container with no discoverable way to administer it). +- [ ] `SPEC.md`: update the "Moderation/admin tooling" Deferred/Open Item + to reflect what's actually implemented vs. still deferred (Find/ + GoTo/Control/Clone/Spawn/Jail/Buff/Relinquish, audit logging). +- [ ] `docs/adr/README.md` / `docs/plans/README.md`: index rows for + ADR-0005/PLAN-0005. +- [ ] `docs/plans/0001-wheelmud-reconciliation-roadmap.md`: check off + Slice 3. + +## Critical files + +New: +- `src/SharpMud.Engine/Commands/SecurityRole.cs` +- `src/SharpMud.Engine/Commands/RoleGuardedCommand.cs` +- `src/SharpMud.Engine/Commands/MuteGuardedCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/BootCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/MuteCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/UnmuteCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/AnnounceCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/BanCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/UnbanCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/RoleGrantCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/RoleRevokeCommand.cs` +- `src/SharpMud.Engine/Commands/Builtin/Admin/AdminCommands.cs` +- Matching test files under `tests/SharpMud.Engine.Tests/Commands/...` + +Modified: +- `src/SharpMud.Engine/Commands/ICommandRegistry.cs`, + `CommandRegistry.cs`, `Builtin/HelpCommand.cs` +- `src/SharpMud.Engine/Commands/BuiltinCommands.cs` (or wherever + `RegisterAll` lives), `src/SharpMud.Ruleset.Classic/ClassicCommands.cs` +- `src/SharpMud.Engine/Behaviors/PlayerBehavior.cs` +- `src/SharpMud.Persistence/Configurations/PlayerBehaviorConfiguration.cs` +- `src/SharpMud.Host/LoginFlow.cs`, `HostOptions.cs`, `Program.cs`, + `HostRunner.cs`, `TelnetHostContext.cs`, `SessionLoop.cs` +- `docs/commands.md`, `docs/accounts-auth.md`, `docs/deployment.md`, + `SPEC.md`, `docs/adr/README.md`, `docs/plans/README.md`, + `docs/plans/0001-wheelmud-reconciliation-roadmap.md` + +## Test plan + +- Unit: `SecurityRole` β€” every named member (excluding `None`/`All`) is a + distinct power of two, and no two members share a bit + (`Enum.GetValues()` pairwise-AND'd should all be `None` + except `All`/self-comparisons). `All` equals the OR of every individual + flag. The regression test for the undefined-values gap caught in PR + review β€” this is the one test that would have caught it immediately if + the enum were ever implemented with auto-numbered members. +- Unit: `RoleGuardedCommand` β€” actor with the required role reaches the + inner command; actor without it doesn't, gets the rejection message. + Cover the any-of/bitwise semantics (actor has one of several required + flags). +- Unit: `MuteGuardedCommand` β€” muted actor blocked, unmuted actor passes + through to the inner command. +- Unit: `CommandRegistry` β€” `RegisterOpen` resolves unconditionally; + `RegisterWithRole` resolves to a `RoleGuardedCommand` wrapping the given + command with the given role. +- Unit: each of the 8 admin commands β€” happy path (role holder, valid + target) and the online/offline target-lookup branches. +- Unit: `BootCommand`/`AnnounceCommand` β€” a `Linkdead` player is treated as + not-online (`Boot` reports "not online," `Announce` doesn't attempt a + write to their stale session); only `ConnectionState == Playing && + Session is { IsConnected: true }` targets/recipients count. Specifically + cover a `Playing`-but-`Session == null` player (the repository-reload + case β€” `ConnectionState` defaults to `Playing` because it isn't + persisted) being correctly treated as *not* online β€” the regression test + for both the online/live ambiguity caught in self-review and the + reload-defaults-to-Playing gap caught in PR review. +- Unit: `SessionLoop` β€” a session ending with `WasBooted` set (but not + `explicitQuit`) takes the same immediate-removal path `explicitQuit` + takes, not `EnterLinkdead`. The regression test for the + boot-is-cosmetic gap caught in PR review: without this, a booted player + would just resume via the `Linkdead` reconnect path, making `boot` a + no-op as a moderation tool. +- Unit: `HelpCommand` β€” a role-gated command is omitted from the listing + for an actor without the required role, and included for one with it; + non-gated commands always list regardless of role. The regression test + for the admin-command-visibility gap caught in self-review. +- Unit: `BanCommand` β€” self-targeting is rejected with a clear message, + `IsBanned` unchanged; targeting another player still works normally. +- Unit: `RoleRevokeCommand` β€” revoking your own `FullAdmin` is rejected; + revoking a different role from yourself, or `FullAdmin` from someone + else, still works normally. Both are the regression test for the + self-lockout risk caught in self-review. +- Unit: `RoleGrantCommand`/`RoleRevokeCommand` β€” `all` and `none` (any + casing) are rejected with a clear message and never reach + `GrantRole`/`RevokeRole`; every other individually-grantable role name + is accepted. The regression test for the `All`/sentinel-value gap + caught in PR review. +- Unit: `LoginFlow` β€” banned user rejected at password verification with + the correct message, not silently falling through. +- Unit: `PlayerBehavior.GrantRole`/`RevokeRole`/`Mute`/`Unmute`/`Ban`/ + `Unban` β€” state mutates as expected. Specifically cover accumulation: + `GrantRole(FullAdmin)` results in `Roles` containing `FullAdmin`, + `MinorAdmin`, *and* `Player`; `GrantRole(FullBuilder)` results in + `FullBuilder` + `MinorBuilder`. +- Unit: a `FullAdmin`-only actor (post-accumulation) successfully passes a + `MinorAdmin`-gated `RoleGuardedCommand` β€” the regression test for the + bootstrap-admin-can't-moderate gap caught in PR review. +- Unit: `RevokeRole(MinorAdmin)` on an actor who also holds `FullAdmin` + returns a non-null failure message and leaves `Roles` unchanged β€” the + regression test for the revoke-side hierarchy gap caught in PR review + (and, separately, that the failure is a return value, not a thrown + exception, per `coding-standards.md`'s Error Handling section). + `RevokeRole(FullAdmin)` on that same actor returns `null` (success) and + leaves `MinorAdmin`/`Player` intact (demotion, not a full reset β€” + revoking the top tier doesn't cascade-clear what it implied). + `RevokeRole(MinorAdmin)` on an actor who does *not* also hold + `FullAdmin` returns `null` (success) normally. +- Unit: `HostOptions.Parse` β€” `SHARPMUD_INITIAL_ADMIN` parses correctly, + absent env var leaves it null. +- Unit: bootstrap grants `FullAdmin` via both paths independently β€” the + `Program.cs` existing-character path, and `LoginFlow.MaybeCreateAsync`'s + newly-created-character path β€” since a boot-time-only check was the + fresh-server gap caught in PR review. +- Unit: `LoginFlow.MaybeCreateAsync` (or its `RunAsync` entry point) + actually receives and uses `InitialAdminUsername` β€” a character created + with a username matching it gets `FullAdmin`; a character created with a + non-matching (or null/absent) `InitialAdminUsername` does not. The + regression test for the parameter-threading gap caught in PR review + (`TelnetHostContext`/`LoginFlow.RunAsync` never carried this value + before). + +## Verification + +Real manual check over Telnet (this repo's established pattern for +session/persistence-facing changes): + +1. **Fresh-server case** (the gap caught in PR review): boot a brand-new + world with `SHARPMUD_INITIAL_ADMIN=` set, and only *then* + create that character over Telnet β€” confirm the grant happens at + creation time, not just at boot. Confirm the resulting admin can run + *both* a `FullAdmin`-gated command (e.g. `ban`) *and* a + `MinorAdmin`-gated one (e.g. `boot`) without a separate grant β€” + validates the accumulation fix. +2. **Restart case**: restart the server (same world, same + `SHARPMUD_INITIAL_ADMIN`); confirm the existing admin's roles are + unaffected (idempotent, no duplicate grants/errors). +3. As the admin, `rolegrant minoradmin` on a second + character; confirm the second character can now run `boot`/`mute`/ + `announce` but not `ban`/`rolegrant`. +4. `mute `; confirm that player's `say`/`emote` are blocked with a + clear message, `unmute` restores them. +5. `ban `; confirm that player can no longer log in (distinct + message, not the generic "incorrect" one); `unban` restores login. +6. `boot `; confirm their session is disconnected immediately, then + **immediately reconnect as that player** (within the `Linkdead` grace + window) and confirm login goes through the normal username/password + prompt from scratch, not a "Welcome back" resume β€” the regression check + for `WasBooted` actually crossing the `SessionLoop` boundary (PR review + gap: without it, `boot` would be a no-op since the target could just + reconnect and resume where they were). +7. `announce `; confirm every currently-connected session + receives it. +8. Confirm a non-admin attempting any of the above gets a clear rejection, + not a crash or a silent no-op. +9. Restart the server again; confirm `Roles`/`IsMuted`/`IsBanned` all + survived (unlike `ConnectionState`, which is intentionally + runtime-only). + +## Open questions / blockers + +- Audit logging of moderation actions (who banned/muted whom, when) is + explicitly not in ADR-0005's scope β€” flagged here as a likely near-term + follow-up once this lands, not designed yet. +- Exact rejection message wording for gated commands is a placeholder + ("You don't have permission to do that.") β€” not a considered final + string. diff --git a/docs/plans/README.md b/docs/plans/README.md index 4ef1919..aae3dcd 100644 --- a/docs/plans/README.md +++ b/docs/plans/README.md @@ -59,3 +59,4 @@ isn't settled yet β€” don't do that. | [0001](0001-wheelmud-reconciliation-roadmap.md) | [ADR-0001](../adr/0001-wheelmud-reconciliation-roadmap.md) | In Progress | | [0002](0002-telnet-protocol-negotiation.md) | [ADR-0002](../adr/0002-telnet-protocol-negotiation.md) | Done | | [0004](0004-session-state-machine-and-reconnect.md) | [ADR-0004](../adr/0004-session-state-machine-and-reconnect.md) | Done | +| [0005](0005-security-role-model-and-moderation-commands.md) | [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md) | Not Started | diff --git a/docs/research/wheelmud-findings.md b/docs/research/wheelmud-findings.md index d979c27..e86732a 100644 --- a/docs/research/wheelmud-findings.md +++ b/docs/research/wheelmud-findings.md @@ -335,6 +335,24 @@ to regress here. --- +## 11. Security roles / moderation β€” `ActionSecurityAttribute`, `Actions/Admin/` + +Source dive conducted for [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md) +(Slice 3 of the reconciliation roadmap). `Core/Attributes/ActionSecurityAttribute.cs` ++ `Actions/Admin/` (16 files): a `[Flags] enum SecurityRole` (`mobile` / +`item` / `room` / `tutorialPlayer` / `player` / `helper` / `married` / +`minorBuilder` / `fullBuilder` / `minorAdmin` / `fullAdmin` / `all`, 12 real +values), an `[ActionSecurity(SecurityRole.x)]` class attribute on each +Action, reflected off at registration time into a `Command.SecurityRole` +field, gated at dispatch by a bitwise-AND check +(`command.SecurityRole & user.SecurityRoles`) in `CommandGuard.cs`. +`Actions/Admin/` (`Announce`, `Ban`, `Boot`, `Buff`, `Clone`, `Control`, +`Find`, `GoTo`, `Jail`, `Locate`, `Mute`, `Relinquish`, `RoleGrant`, +`RoleRevoke`, `Spawn`, `Unmute`) are ordinary Actions decorated with +high-tier roles β€” no separate mechanism from any other command. + +--- + ## Decisions for sharp-mud Two real forks, resolved as follows (see the "Engine vs. Ruleset" architecture @@ -379,6 +397,19 @@ don't require adding new delegate pairs to a growing interface. one sequential `await` chain per connection. MCCP/MXP/TermType remain unreviewed beyond the survey in Β§Networking above. +4. **The `[Flags] SecurityRole` bitmask and its bitwise-AND gating check are + adopted from Β§11's `ActionSecurityAttribute`/`Actions/Admin/` dive; + attribute+reflection dispatch is not.** sharp-mud gates via a + hand-rolled Decorator (`RoleGuardedCommand`/`MuteGuardedCommand` + wrapping an inner `ICommand` at registration time) instead, since + sharp-mud's `ICommandRegistry.Register(ICommand)` model has no + DI-container registration step for an attribute-scanning approach (or + `LayeredCraft.DecoWeaver`, which only intercepts `IServiceCollection` + registrations) to hook into. Full rationale, the grant/revoke + hierarchy-accumulation rule, and the command set actually built are in + [ADR-0005](../adr/0005-security-role-model-and-moderation-commands.md)/ + [PLAN-0005](../plans/0005-security-role-model-and-moderation-commands.md). + Going forward, further reconciliation against WheelMUD (moderation/admin tooling, session reconnect, world-building commands, and more) is tracked as a sequenced roadmap in [ADR-0001](../adr/0001-wheelmud-reconciliation-roadmap.md)