Feature | UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting

[matiasperrone-exo]

Task:

Ref: https://app.clickup.com/t/9014802374/86ba2zc6p

What Changed

UserController (app/Http/Controllers/UserController.php)

• Injected IDeviceTrustService, ITwoFactorAuditService, and ITwoFactorGateService via constructor.
• Password flow now validates credentials without creating a session first. If MFAGateService::requiresChallenge() returns true, issues a challenge (via
  AuthService::issueMFAChallenge) and returns { error_code: "mfa_required", ... } — no session created yet.
• Added resolveClientFromMemento() private helper to extract the OAuth2 client from a pending session memento (deduplicates logic previously repeated in password and OTP flows).
• Added postVerify2FA(), postResend2FA(), and postRecovery2FA() action methods to handle the full MFA step-up login lifecycle.
• Applied MFACookieManager trait for reading/writing the trusted-device cookie.

MFACookieManager trait (app/Http/Controllers/Traits/MFACookieManager.php) — new file

• Provides getCookieToken(): ?string (reads raw token from cookie).
• Provides queueDeviceTrustCookie(User $user): void (calls IDeviceTrustService::trustDevice and queues a secure, HttpOnly cookie).
• Contains no Doctrine/repository logic — cookie transport only.

TwoFactorRateLimitMiddleware (app/Http/Middleware/TwoFactorRateLimitMiddleware.php) — new file

• Throttles verify, recovery, and resend MFA endpoints.
• Counters stored in cache (not session) with a fixed-window TTL.
• verify / recovery: increments only on failed response (mfa_verification_failed, mfa_invalid_recovery).
• resend: increments on every request.
• Returns HTTP 429 with error_code: mfa_rate_limit when the limit is exceeded.
• Registered in app/Http/Kernel.php and wired to the new routes in routes/web.php.

AuthService (app/libs/Auth/AuthService.php) and IAuthService (app/libs/Utils/Services/IAuthService.php)

• Added validateCredentials(string $username, string $password): User — validates without creating a session.
• Added loginUser(User $user, bool $remember): void — establishes the session for an already-validated user.
• Added issueMFAChallenge(User $user, MFAChallengeStrategy $strategy, ?Client $client, bool $remember): array — triggers the challenge strategy and stores pending state.

DeviceTrustService and TwoFactorAuditService (minor fixes)

• Resolved wiring/dependency issues surfaced during integration testing.

config/two_factor.php — new file

• Centralizes MFA configuration: cookie_name, device_trust_lifetime_days, rate-limit thresholds per action.

EncryptCookies middleware

• Excluded the device_trust_token cookie from encryption (raw HMAC token must be read as-is by IDeviceTrustService).

---

Tests

File	What it covers
tests/TwoFactorLoginFlowTest.php (new)	End-to-end: password→MFA challenge→verify→session; device trust skip; rate limiting; recovery codes; resend.
tests/DeviceTrustServiceTest.php	Expanded with cookie integration assertions.
tests/Unit/TwoFactorAuditServiceTest.php	Expanded with new event types.
tests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.php	Added cases for issueMFAChallenge path.
phpunit.xml	Registered Two Factor Authentication Test Suite.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9b88570f-386b-438c-b9a9-ce74122be4dd

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/mfa---usercontroller-mfa-integration-device-trust-cookie-management-audit-wiring-and-2fa-rate-limiting---cu-86ba2zc6p

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/

This page is automatically updated on each push to this PR.

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/

This page is automatically updated on each push to this PR.

[caseylocker]

Reviewed against ClickUp 86ba2zc6p and SDS idp-mfa.md (§4.4 controller flow, §4.5 strategies, §4.9.1 AuthService MFA methods).

The transaction-context design rule is correctly implemented — the controller orchestrates only, all strategy DB-mutating calls go through AuthService::*MFA* wrappers, and no add(entity, true) self-flush remains. Every acceptance criterion is met on the happy path, and verifyChallenge now correctly loads the persisted OTP and compares its value.

However, five issues should be addressed before merge. All five are consequences of work first made live in this PR (the MFA strategies and loginUser() were previously unwired/dead code), so they are in scope here rather than the base PR. The first is a critical regression in the shared password-login path. Inline comments below.

Also, a few test gaps versus the ticket's TESTS list: concurrent OTP redeem, concurrent recovery-code redeem, client-bound MFA verification, recovery rate-limit blocking, and rollback/partial-failure after a successful redeem.

[caseylocker]

OTP redeem lacks the pessimistic row lock (TOCTOU double-redeem).

verifyChallenge fetches via getByValueConnectionAndUserName (plain SELECT) then redeem() at :62, without acquiring IOAuth2OTPRepository::refreshExclusiveLock() (DoctrineOAuth2OTPRepository.php:142). With the transaction service defaulting to READ COMMITTED (DoctrineTransactionService.php:133), two concurrent submissions of the same valid code can both read the unredeemed row and both succeed.

SDS §4.5 and the ticket gotcha explicitly say to prefer delegating to AuthService::verifyOTPChallenge() (tx-managed, value-checked, row-locked, session-bound) rather than reimplementing redemption. Either route through verifyOTPChallenge() or call refreshExclusiveLock() and re-check redemption state before redeem().

[smarcet]

@matiasperrone-exo remove all the repository->add from everywhere this is an antipattern

[caseylocker]

MFA OTP is issued with a client but verified with client = null.

The challenge is issued with the resolved client (UserController.php:480), but verifyMFAChallenge calls verifyChallenge($user, $value) with no client, so the lookup skips client scoping (DoctrineOAuth2OTPRepository.php:130). More concretely, the sibling-revocation in the strategy (EmailOTPMFAChallengeStrategy.php:65) calls getByUserNameNotRedeemed($email) without a client, so verifying an MFA OTP redeems all of the user's pending email OTPs — including unrelated passwordless-login OTPs for other clients.

Fix: carry the client id in the pending MFA state and pass the same client into verification (and into the sibling-revoke query), or delegate to verifyOTPChallenge($claim, $user, $client).

[caseylocker]

Recovery-code redemption has a double-spend race.

verifyRecoveryCode iterates getUnusedByUser (:51), Hash::checks, then markUsed() + add(false) with no pessimistic lock or conditional update, under READ COMMITTED (DoctrineTransactionService.php:133). Two concurrent requests can both observe the same code unused and both succeed.

Fix: lock the user's unused recovery rows before hash-checking, or redeem via an atomic UPDATE ... WHERE id = ? AND used_at IS NULL after identifying the match.

[smarcet]

@matiasperrone-exo this is an anti pattern , remove it

[caseylocker]

Post-verification side effects can 500 after the OTP is already burned.

By this point the OTP redeem has committed (verifyMFAChallenge, :670) and the session is established (loginUser, :686). queueDeviceTrustCookie (:689, its own tx in DeviceTrustService) and this audit log() (its own tx in TwoFactorAuditService) run afterward; if either throws, the outer catch (Exception) returns error500 even though the user is effectively logged in and the single-use OTP is spent — on retry they are locked out. If trustDevice throws, pending state is not cleared either.

Fix: make audit best-effort/non-blocking, and/or order durable side effects before loginUser(), keeping device-trust persistence consistent with the redeem boundary. Ticket Task 5 asks for exactly this decision ("redeem and device-trust persistence are atomic; audit may be best-effort").

[github-actions]

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/

This page is automatically updated on each push to this PR.

[smarcet]

@matiasperrone-exo

Review: client resolution in verify2FA duplicates state the memento already owns

resolveClientFromPendingState (app/Http/Controllers/UserController.php:642) should be replaced with the existing resolveClientFromMemento() helper. Findings:

1. Duplicated source of truth

The pending-state client_id is a session copy of the client that resolveClientFromMemento() already resolved at challenge-issue time:

postLogin (UserController.php:480)  →  resolveClientFromMemento()
  →  issueMFAChallenge  →  EmailOTPMFAChallengeStrategy::issueChallenge (line 25)
  →  storePendingState(..., $client?->getClientId())   // copies into 2fa_pending_client_id

The OAuth2 memento survives the challenge→verify round-trip — resolveClientFromMemento re-serializes after load() (UserController.php:628) and no session regeneration happens until loginUser runs after verification. The copy adds a second source of truth for a fact the session already holds canonically.

2. Internally inconsistent within this PR

• verify2FA resolves via pending state (UserController.php:688)
• resend2FA resolves via resolveClientFromMemento() (UserController.php:853)

Two endpoints of the same MFA lifecycle disagree on where the client comes from. Worse, resend's path re-runs issueChallenge → storePendingState, silently overwriting the pending-state client with the memento client — so the "snapshot the client at issue time" semantic the comment at UserController.php:634-637 claims only holds until the first resend.

Concrete drift scenario (two tabs): challenge issued for client A; user starts an authorization request for client B in another tab (memento now = B). Verify succeeds scoped to A, then login_strategy->postLogin() (UserController.php:742) resumes the memento flow — client B. The OTP verification scope and the flow that actually completes disagree. Memento-based resolution makes verify, resend, and post-login continuation agree by construction.

3. Silent degradation on stale client

resolveClientFromMemento throws ValidationException("client does not exists") when the client can't be resolved (UserController.php:626); resolveClientFromPendingState returns null (UserController.php:648) — and a null client makes the OTP lookup unscoped (DoctrineOAuth2OTPRepository.php:131 skips the client filter). A deleted/stale client id degrades silently to a wider lookup instead of failing loudly.

Recommended fix

Swap UserController.php:688 to $client = $this->resolveClientFromMemento();. That makes all of this removable:

• resolveClientFromPendingState (UserController.php:642-649)
• KEY_CLIENT_ID + the ?string $clientId param on storePendingState + client_id in getPendingState/clearPendingState (AbstractMFAChallengeStrategy.php:16,39,48,72-81)
• The scoping comments at EmailOTPMFAChallengeStrategy.php:23-24 and UserController.php:633-637

Safety of the swap is already proven in-PR: resend2FA calls resolveClientFromMemento() mid-MFA and the memento survives for postLogin because the helper re-serializes it. user_id and remember legitimately stay in pending state — they have no memento equivalent. The existing AbstractMFAChallengeStrategyTest already calls storePendingState($userId, $remember) without a client id, so the signature change is test-compatible.

[smarcet]

@matiasperrone-exo review

[smarcet]

Minor / non-blocking — pre-existing dead code surfaced while mirroring login().

Now that loginUser() mirrors login()'s post-auth principal registration: do not also copy $this->last_login_error = ""; (login(), AuthService.php:396) into the new methods — that line is dead code:

• It is the only reference to last_login_error in the entire codebase: no property declaration, no getter, no read anywhere (checked IAuthService, controllers, views).
• Because the property is never declared, the assignment creates a dynamic property — deprecated since PHP 8.2, so it emits a deprecation notice on every password login under the project's PHP 8.3 runtime.

Since this PR already reworks the login path, deleting that one line in login() is a fair drive-by cleanup here; otherwise it can go in a follow-up. Not blocking for this PR.

[smarcet]

Same category, same file: the import use Auth\Exceptions\UnverifiedEmailMemberException; (AuthService.php:22) is also unused — it is the only reference to that symbol in the whole file (no code reference, no docblock reference). Safe to drop alongside the last_login_error line.

To be clear: only the import is dead. The exception class itself is alive — thrown by CustomAuthProvider.php:132 and caught in UserController.php:562 — so don't remove the class, just this import.