Phase 1 — Follow-ups from review#

Captured from peer review of the Phase 1 PR (2026-09-08). Both buckets are known-and-deferred, not “surprise” work. The reviewer will drive both when they onboard full-time.

1. No committed keys#

Current state: the machine-key values from manexcloud/newSite/Web.config (dev/staging placeholder, per ADR 0001) are embedded in:

scripts/set-dev-secrets.sh (hardcoded literals)
ADR 0001 amendment text (documented example)

Production keys are held by ops and never sit in git — only the dev/staging placeholder is committed today. Still bad practice.

Immediate mitigation (can be done anytime):

Change scripts/set-dev-secrets.sh to read MACHINE_KEY_VALIDATION / MACHINE_KEY_DECRYPTION from env vars; fail loudly if unset.
Replace the hex values in ADR 0001 with <get from ops> placeholder tokens.
Update docs/getting-started/local-db-from-backup.md to say “get the machine keys from ops or from your local Web.config.”

Full solution (reviewer-driven when on): wire secrets to whichever store the team lands on — Bitbucket repository variables for CI, AWS Secrets Manager / Azure Key Vault / 1Password Connect for prod, dotnet user-secrets for local dev.

2. User / group security tightening#

Known gaps flagged by TODO comments in the code, listed here so they’re not lost. Reviewer will drive the design conversation when on full-time (especially the lockout policy — needs product input, not a guess).

Area	File	Gap	Suggested fix (subject to review)
Group write policy	`src/Manex.Web/Program.cs`	`RequireClaim("mod_admin")` allows any admin to modify any group	Add `SuperUser` claim to the JWT (ADR 0005 already lists it as a `mod_admin` source in the profile flags but doesn’t emit a distinct claim) and require it for `GroupWrite`. Alternatively a dedicated `GroupAdmin` role.
User access reads	`src/Manex.Web/Controllers/UsersController.cs`	`GET /users/{userId}/access` accepts any authenticated caller (matches legacy SP behavior, but not defensible long-term)	Restrict to callers with a `UserAdmin`/`SuperUser` claim, or to `sub == userId` for self-reads.
✅ Suspended / locked login	`src/Manex.Web/Auth/UserAuthService.cs` (`AuthenticateAsync`)	~~Login does not reject `IsApproved=false` or `IsLockedOut=true` users~~	Closed 2026-09-09. After successful password verify, `AuthenticateAsync` returns `AccountLocked` / `AccountSuspended` (403). Ordering matters: password is checked first so a locked/suspended signal only leaks to callers who already know the password.
Failed-attempt tracking	Same file	`FailedPasswordAttemptCount` / `FailedPasswordAttemptWindowStart` never get bumped from Cube 2.0	Still open. Requires a design call — do we write to legacy `aspnet_Membership` (breaks the “no writes to legacy” rule from ADR 0004) or add a Cube-2.0-owned counter alongside `MnxLoginActivity`?
✅ Login audit trail	`MnxLoginActivity`	~~No visibility into who tried to log in and when~~	Landed 2026-09-09. Every attempt (success or failure) writes one row: `UserId`, `SubmittedUsername`, `Outcome`, `ClientIp`, `UserAgent`, `RehashApplied`, `AttemptedAt`. Read endpoints: `GET /api/v1/login-activity/recent` and `/users/{userId}` (both gated by `GroupWrite` policy). SQL delta: `deploy/sql/phase-1-add-mnxloginactivity.sql`.
Lockout enforcement	Same file	No cool-off / auto-unlock logic anywhere	Policy decision. Needs Manex product input: attempts before lockout, cool-off duration, admin unlock flow, whether to email the user.
`SuperUser` claim	`src/Manex.Web/Auth/UserAuthService.cs` (`BuildUserInfoAsync`)	`aspnet_Profile.SuperUser` bit exists on the entity but isn’t emitted as a JWT claim	Emit `super=true` claim when the flag is set; unlocks the `GroupWrite` / `UserAdmin` policies above.

Rollback plans#

Per phase-1-core-security, each module in Phase 1 needs a documented rollback. Placeholder — populate as modules land:

Module	Cutover mechanism	Rollback
Auth (Login / JWT)	Direct — no legacy fallback flag at API layer	Feature-flag `FeatureFlags:UseLegacyPasswordHash=false` when rehash coverage is complete; before that, both paths coexist.
User Access Information	Read-only endpoints; no cutover risk	Delete the endpoints — legacy UI unaffected (never called them).
Group Setup	`FeatureFlags:EnableGroupWrites=false` gates all mutations	Flip flag off; reads keep working; legacy admin UI takes back writes.
Login Activity	Cube-2.0-owned table `MnxLoginActivity` — additive	Drop the table + revert the write call in `AuthenticateAsync`. Recorder failures are already swallowed so an ungraceful rollback leaves auth working.