Repository navigation
[Enhancement]: MfaService - TOTP & Backup Codes Management #243
Description
Current source review — 2026-10-04
Commit: fc6c315 (develop). Result: Fail; full acceptance is not established.
TOTP enrollment/verification and backup generation/status APIs exist. The promised 12-character salted/versioned PBKDF2 codes and concurrent single-use enforcement are absent.
Requirement provenance: Original title/body and human discussion define the historical request. Additional engineering checks below are identified through current API contracts and repository invariants; they do not create or silently complete an original acceptance checklist.
Organization: Remain OPEN; the unresolved requirements below require acceptance or a product decision.
Open native sub-issues: #285.
Current implementation evidence
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Services/TotpMfaService.cs:26-69 — Cryptographic 160-bit Base32 secret is encrypted before storage; setup starts disabled and returns otpauth URI.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Services/TotpMfaService.cs:82-163 — Checks enabled option/setup expiry/lockout, decrypts secret, verifies configured time-step tolerance, enables MFA on first success, resets failures and records failed attempts.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Services/TotpMfaService.cs:181-236 — Generates PNG using QRCoder; otpauth URI includes issuer/email/SHA1/6 digits/configured period; code comparison uses CryptographicOperations.FixedTimeEquals.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Controllers/MfaController.cs:47-95 — Authenticated setup derives current user/email and sends InitiateMfaSetupCommand; returns secret and QR URI; completion dispatches current-user code verification.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Data/Extensions/DependencyInjection.cs:119-131 — Registers TOTP, backup-code and attempt services plus composite IMfaService and session service.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Commands/MfaMutationCommands.cs:11-31 — CQRS mutation handler delegates setup, complete, verify, regenerate and disable commands to IMfaService.
- apps/api/Source/GameGuild.API/Core/Setup/ApplicationLayerExtensions.cs:91-142 — Discovers enabled module assemblies and invokes services.AddCqrs(cqrsAssemblies); the shared CQRS scanner registers command and notification handlers.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Services/BackupCodeMfaService.cs:23-66 — Generates configured number/length of cryptographically random codes, hashes and persists them, returns plaintext only when generated.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Services/BackupCodeMfaService.cs:183-199 — HashBackupCodeAsync computes unsalted SHA256 and returns Base64; verification uses computedHash==hash. Promised versioned salted PBKDF2 is absent.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Services/BackupCodeMfaService.cs:110-144 — Consumption loads comma-separated hashes, matches, removes in memory, unconditionally updates configuration, then reports success; no atomic claim in this service.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Repositories/UserMfaConfigurationRepository.cs:12-40 — Repository reads a user configuration then applies Update+SaveChanges; no compare-and-swap consumption.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Data/Configurations/UserMfaConfigurationConfiguration.cs:23-62 — MFA mapping configures plain backup-code column and no concurrency token; user-id index is not unique.
- apps/api/Source/Modules/GameGuild.SharedKernel/Configuration/ApplicationLayer/MfaOptions.cs:16-46 — Defaults: 5 failed attempts, 15-minute lockout, 10 codes, BackupCodeLength=8, 30-second step, skew=1.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Services/MfaAttemptTrackingService.cs:296-332 — Failed attempts increment loaded configuration and set configured lockout, then UpdateAsync; sequential policy present, concurrent count protection absent.
- apps/api/Source/Modules/GameGuild.Identity.Authentication/Controllers/MfaController.cs:136-174 — Authenticated backup status returns counts; regeneration returns newly generated codes.
Requirement review
Remaining gaps
- Backup hashing is unsalted SHA256 rather than promised PBKDF2-HMAC-SHA256 600k/versioned format.
- Default length remains8 rather than recorded12.
- Concurrent successful consumption has no atomic claim or EF concurrency token; failed-attempt increments are also read/modify/write.
- Current TOTP enrollment, sequential/concurrent recovery use and lockout HTTP/storage execution absent.
This is static source inspection, including active wiring where observable. No product tests, builds, browser flows or production requests were executed in this review. Existing test definitions and historical result comments are not fresh execution evidence. Current findings supersede conflicting historical implementation assertions without deleting that history.
The original description, checklist status, title and labels are preserved. No functional criterion is marked complete by this audit.
Activity
[-]💎 Feature Request[/-][+][Enhancement]: MfaService - TOTP & Backup Codes Management[/+]mathrmartins commentedon Sep 26, 2026
Resolution and acceptance criteria
The issue had no description or checklist, so I used the title as its acceptance scope: TOTP enrollment/verification and MFA backup-code management.
Implemented on the local
feature/issue-closeoutworktree:Evidence:
BackupCodeMfaService.cs,TotpMfaService.cs,UserMfaConfigurationConfiguration.cs,BackupCodeMfaServiceSecurityTests.cs,TotpMfaServiceSecurityTests.cs, andEfConfigAndExtendedTests.UserMfaConfigurationConfiguration_ConfiguresEntity.Verification passed on 2026-09-26:
dotnet test apps/api/tests/GameGuild.Identity.Authentication.UnitTests/GameGuild.Identity.Authentication.UnitTests.csproj --no-restore— 1,757 passed, 0 failed.The work is in the local feature branch; no PR has been opened yet. Reopen with concrete additional criteria if this title was intended to include MFA methods beyond TOTP and recovery codes.
mathrmartins commentedon Oct 5, 2026
Implementation increment is now PR #686, head
38c81a5a75ed6202235f6bb047af662683a49690. This issue remains OPEN while the remaining acceptance is verified.The change restores the previously recorded 12-character/PBKDF2/600,000-iteration criteria, legacy compatibility, single use under concurrent/stale contexts and five-failure/15-minute lockout. Pending enrollment cannot use backup codes. New TOTP enrollments have a fixed deadline; successful confirmation records completion.
The previously reproduced status gap is fixed without breaking numeric v1 fields. Newly issued sets retain original totals across consumption/configuration changes. Legacy fields are lower bounds with
areUsageCountsKnown=false; original issuance remains unknown. No fabricated historical used count is claimed.Current local evidence: 2,083 authentication, 1,667 authorization, 1,371 SharedKernel, 106 selected API architecture/security/bearer, nine actual migrated PostgreSQL/HTTP recovery and three client contract executions pass. There are 35 new cases. Configured Release solution build exits 0 with zero warnings/errors; EF reports no pending model changes; typed client generation/typecheck/diff pass. All 1,296 OpenAPI paths stay unchanged; only two response schemas change. Committed evidence and limitations.
The baseline reproduced four unit defects and double consumption in real PostgreSQL. Superseded/interrupted attempts and the invalid expectation that a null token field should be absent are retained and excluded. HTTP recovery uses a test principal; real bearer validation is a separate test selection. Existing anonymous MFA verification does not issue access/refresh tokens and its complete sign-in challenge flow is not certified here.
Exact-head GitHub gates and develop merge are pending. Configured external email delivery for #223 and real authenticator QR acceptance for #285 remain unverified; #243 retains its native child. This increment does not close these issues or certify production delivery.
mathrmartins commentedon Oct 5, 2026
MFA recovery increment merged — original issue remains OPEN
PR #686 is MERGED into develop at
8e1266698a0998364ab39a34078e0fd16eaa17c8(2026-10-05T03:31:03Z). Accepted head:10e1df87c50d4f6a8280b42b6ffcdfff450b2429.All applicable matching-head checks passed: API, Web, clean migration/model compatibility, OpenAPI/client consistency, repository policy, tracked-artifact checks, PR Required Gate, Codacy and four CodeQL analyses. CI run executed 151 API integration, 1,049 API unit, 2,083 authentication and 1,371 SharedKernel cases, all passing (4,654 principal executions), followed by 14 passing repeated OpenAPI HTTP cases. Both Release builds had zero warnings/errors. Nonapplicable Testing Lab/Economy/Emception jobs were skipped.
Local evidence remains 5,239 distinct cases, including 35 new cases, with the full warning/error-clean solution build, real migrated PostgreSQL concurrency/replay/actor/lockout checks, legacy compatibility and regenerated client contracts. Repeated, superseded, interrupted and crashed attempts are excluded; the previous head's 20-minute API cancellation is retained as failure history. The API allowance was extended to 30 minutes without dropping a test or gate.
Implementation and boundary map: full original issuance counts for new sets, explicit unknown legacy history, 12-character/600,000-iteration backup criteria, guarded concurrent single use, pending-enrollment rejection and fixed new-enrollment deadline/completion state.
#223, #243 and #285 remain OPEN. Configured external recovery-email delivery and a real authenticator QR round trip remain unverified. #243 retains its native child. Existing anonymous MFA verification returns no access/refresh tokens; complete sign-in challenge/session acceptance is not certified by this increment. No automatic closure or production-delivery claim is made.
The primary develop checkout is synchronized with all 55 unrelated local files preserved byte-for-byte and by status. The integrated branch was removed locally/remotely. Two checkouts are reused and no stashes remain; password hashing/policy work under #254/#255 continues in the isolated checkout.
mathrmartins commentedon Oct 11, 2026
Verification at current develop (2026-10-11, ecfc427) — all three gaps from the 2026-10-04 review are now closed. Closing with evidence.
1. Recorded cryptographic backup-code hardening — implemented.
BackupCodeMfaService.HashBackupCodeAsyncnow derives salted, versioned PBKDF2-HMAC-SHA256 at 600,000 iterations (16-byte salt, 32-byte hash,pbkdf2-sha256$600000$…format): apps/api/Source/Modules/GameGuild.Identity.Authentication/Services/BackupCodeMfaService.cs:205-213. Legacy unsalted SHA256 rows remain verifiable exactly once via fixed-time comparison (BackupCodeMfaService.cs:226-243).MfaOptions.BackupCodeLengthdefault is now 12: apps/api/Source/Modules/GameGuild.SharedKernel/Configuration/ApplicationLayer/MfaOptions.cs:34.2. Concurrent single-use enforcement — implemented.
3. TOTP enrollment/verification execution — implemented and tested.
TOTP/backup-code scope of this issue (and landed sub-issue #285) is verified complete at develop. Broader MFA-system scope (SMS, method management UI, metrics, docs) remains tracked by #244.