Feature | UserController MFA Integration, Device Trust Cookie Management, Audit Wiring, and 2FA Rate Limiting#136
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
77c0c4f to
0fb9adf
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
0fb9adf to
84252f7
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
84252f7 to
56e1468
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
caseylocker
left a comment
caseylocker
left a comment
There was a problem hiding this comment.
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.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@matiasperrone-exo review
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
6d66d2d to
f253e7f
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
a58c823 to
50f4ad0
Compare
… Audit Wiring, and 2FA Rate Limiting
f253e7f to
5fc47a4
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
MFAGateService::requiresChallenge() had no master on/off switch, contradicting the SDS idp-mfa.md §10.1 rollout plan, which requires being able to instantly revert to password-only login without a code rollback if something goes wrong post-deploy. config/two_factor.php gains an 'enabled' key (env TWO_FACTOR_ENABLED, default true) checked first in requiresChallenge(), short-circuiting before any per-user or device-trust evaluation.
postLogin()'s mfa_required response, and the display-strategy contract it depends on, bypassed $this->login_strategy entirely: every MFA response was Response::json(...) built by hand in the controller, ignoring OAuth2 display-strategy polymorphism (native vs page/popup/touch). Native OAuth2 clients (display=native) got JSON+200 with an ad hoc shape instead of the 412 + required_params/url/method contract every other login error already returns for that display mode. - ILoginStrategy::challengeRequired() / IDisplayResponseStrategy:: getChallengeRequiredResponse(): new methods, distinct from errorLogin() since a pending MFA challenge isn't a failed attempt. - DefaultLoginStrategy: identical bytes to before (200 + JSON) - zero behavior change for the plain IdP flow. - OAuth2LoginStrategy: rebuilds the auth_request from the memento (same pattern as errorLogin()) and delegates to DisplayResponseStrategyFactory. - DisplayResponseJsonStrategy (native): 412, matching its sibling getConsentResponse/getLoginResponse/getLoginErrorResponse methods. - DisplayResponseUserAgentStrategy (page/popup/touch): 200 JSON, same live in-SPA transition as the plain flow, since both render the same login.js. - ILoginStrategy::MFA_REQUIRED constant replaces the 'mfa_required' literal duplicated across three classes. Also closes a refresh-resilience gap PR #142's frontend already expected but the backend never delivered (its login.js constructor comment reads "Two-factor state (populated from the flash redirect...)"): postLogin() now flashes flow/mfa_method/otp_length/otp_lifetime to session on mfa_required so a page refresh mid-challenge restores the 2FA screen instead of dropping back to the password form. Cleared on successful verification/recovery and on session expiry; refreshed on resend2FA() (including method switches). New: OAuth2NativeMFALoginFlowTest exercises the real /oauth2/auth -> memento -> postLogin() path for display=native and asserts 412+mfa_required. TwoFactorLoginFlowTest gains coverage for the session-flash/clear behavior.
None of the three login strategies' cancelLogin() cleared any 2FA session state - not the pre-existing 2fa_pending_user_id/2fa_pending_at/2fa_remember keys, nor the flow/mfa_method/otp_length/otp_lifetime keys added for refresh-resilience. PR #142's Cancel button resets the client's React state immediately and fires cancelLogin() as a best-effort background call, so the broken UX was masked within the same tab - but a subsequent full page load within the challenge's 300s TTL (back button, reopened tab, direct /login navigation) would restore the 2FA screen for a challenge the user explicitly abandoned, and the stale OTP could still complete it. UserController::cancelLogin() now resolves the pending strategy via the mfa_method session key (when present) and clears its pending state before delegating to the login strategy, plus clears the UI-restoration keys via the existing clearMFAUISessionState() helper. New test proves the strongest form of the property: an OTP valid before cancel returns mfa_session_expired afterward, not just that some session keys are gone.
Two related fixes to the MFA login flow: 1. Passwordless (flow=otp) login never checked shouldRequire2FA(), so an enforced-2FA user could bypass MFA entirely via emitOTP() + postLogin with flow=otp instead of flow=password (SDS idp-mfa.md §7.4 / Open Question #3 explicitly treats passwordless as single-factor). Now throws AuthenticationException before loginWithOTP(), reusing the existing errorLogin() redirect+flash path - the OTP form still submits as a native form POST, so this needed no new response contract. 2. challengeRequired()'s redirect-based implementations (DefaultLoginStrategy, DisplayResponseUserAgentStrategy) previously ignored the $params they received, silently depending on the caller having already flashed otp_length/otp_lifetime to session - an implicit contract that would silently break for any other caller. Both now flash their own $params (persistent, not one-shot, so it survives repeated refreshes) and set error_code, mirroring what DisplayResponseJsonStrategy already sends native clients in JSON. clearMFAUISessionState() now clears error_code too. The '2fa' flow value moves from a new ILoginStrategy constant to IAuthService::AuthenticationFlowMFA, alongside its siblings AuthenticationFlowPassword/AuthenticationFlowPasswordless - all three are the same session 'flow' enum (already flashed together in the AuthenticationException catch block), so splitting the third value into a different interface would have been inconsistent. New test: OAuth2NativeMFALoginFlowTest gains a non-native (page/popup/ touch) case proving the 302+session-flash contract, alongside the existing native 412+JSON case. TwoFactorLoginFlowTest covers the passwordless-bypass rejection (including that it still reuses errorLogin(), not a new JSON contract) and the error_code flash/clear.
The '2fa.rate' middleware could never gate postLogin()'s initial OTP
issuance: its before-phase reads 2fa_pending_user_id from session to know
which user to throttle, but that key is only written by issueChallenge()
- inside the very request that would need throttling. A user with valid
credentials could repeatedly POST to the plain login route and trigger
unlimited email-OTP sends, bypassing the 5-per-15-minute resend cap
entirely (SDS idp-mfa.md §4.12 explicitly requires the initial issuance
to share the same 2fa_rate:resend:{user_id} window as resend()).
Extracted the cache-key/window logic that lived only in
TwoFactorRateLimitMiddleware into ITwoFactorRateLimitService /
TwoFactorRateLimitService (same pattern as DeviceTrustService /
TwoFactorAuditService / MFAGateService, registered in
TwoFactorServiceProvider), so both the middleware (verify/recovery/resend
routes) and UserController::postLogin() (initial issuance, now knows the
user id post-validateCredentials()) share one source of truth instead of
duplicating cache-key construction.
postLogin() checks isRateLimited() before issuing a challenge and calls
increment() after a successful issue. The rejection throws
AuthenticationException, reusing the existing catch block's errorLogin()
redirect+flash path - consistent with challengeRequired() already being
redirect-based, since the password form still submits as a native form
POST. resend2FA()/verify2FA()/verifyRecoveryCode() stay JSON+429 via the
middleware, unaffected, since those are AJAX-only endpoints.
New test proves postLogin() and resend() share the same window: after
max_otp_requests postLogin() calls, the next one is rejected.
Investigated the "session fixation" finding from the PR review (SDS idp-mfa.md §9.3 asks for a test proving 2fa_pending_user_id cannot be injected). Traced actual runtime behavior via debug instrumentation before writing a fix, since pattern-matching "no explicit Session::regenerate() call" as a vulnerability turned out to be wrong. Laravel's SessionGuard::login() (invoked via Auth::login(), already called unconditionally at the end of loginUser()) already calls $session->migrate(true) internally - the session-fixation window was already closed by the framework, with no code change needed for that property specifically. An added test asserting this (comparing session ID before/after login) passed identically with or without any fix, proving it was a false positive caused by this test harness resetting the session ID between $this->action() calls regardless of production behavior - that test was written and then discarded rather than kept for false confidence. What IS real, found via the same investigation: PrincipalService::register() (called by loginUser() before this fix) hashes the CURRENT session ID into op_browser_state, used for OIDC Session Management (check-session iframe). Since register() ran BEFORE Auth::login(), its hash was computed from a session ID that Auth::login()'s own migrate(true) was about to invalidate moments later - any relying party polling the check-session iframe would see a value that no longer matched what the server would recompute, incorrectly signaling a session change. Fix: call Auth::login() first, then principal_service->clear()/register() after, so the hash uses the final, stable post-login session ID. No new Session::regenerate() call needed - Auth::login() already provides one. New tests: - AuthServiceLoginUserTest (unit, Mockery-alias facades, same pattern as AuthServiceLogoutTest): asserts the call order directly. - TwoFactorLoginFlowTest::testCompletedMFALoginKeepsOPBrowserStateConsistentWithSessionId (integration): proves op_browser_state matches a freshly-computed hash of the post-login session ID end-to-end through the real MFA verify flow. Confirmed failing against the pre-fix ordering, passing after.
Ticket CU-86ba2zc6p's TESTS list requires: "OTP redeem is persisted only on commit; a failure inside the verify transaction rolls back the redeem." No such test existed anywhere in this branch or PR #142/#146 - the two closest existing tests (testOTPCodeRejectsReuseAfterSuccessfulVerification, testRecoveryCodeRejectsReuseAfterTransactionCommit) only prove the COMMIT path (a successful verification's redeem persists and blocks reuse), not that a FAILED verification's partial redeem rolls back. Pure test-coverage gap, no production fix needed - AuthService::verifyMFAChallenge() already wraps strategy->verifyChallenge() in tx_service->transaction(), and DoctrineTransactionService already rolls back and re-throws on failure. Confirmed the test has teeth: temporarily bypassing the transaction wrapper broke the pessimistic-lock acquisition inside verifyChallenge() (which requires an open transaction), proving the test environment genuinely depends on transactional context, not just coincidentally passing. testOTPRedeemRollsBackOnMidTransactionFailure wraps the real EmailOTPMFAChallengeStrategy in a test double that lets the genuine redeem happen, then throws immediately after - inside the same transaction. Asserts the OTP is refetched from the DB (post-rollback) still unredeemed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
EventRecoveryUsed was logged unguarded after loginUser() and clearPendingState(), so an audit-sink failure at that point propagated to the outer catch(Exception) and returned a 500 to a user who was already authenticated with an already-burned recovery code — the account's last-resort login path. Mirrors the same best-effort try/catch already applied to verify2FA()'s EventChallengeSucceeded audit call. Adds testRecoveryAuditFailureDoesNotBlockLogin, the recovery-path analogue of testAuditFailureDoesNotBlockLogin, reproducing the 500 before the fix and asserting a 302 + established session after it.
testOTPCodeRejectsReuseAfterSuccessfulVerification and testRecoveryCodeRejectsReuseAfterTransactionCommit only prove sequential reuse is rejected after a transaction commits. Neither exercises the actual property refreshExclusiveLock() exists for: blocking a second, concurrent request from redeeming the same unredeemed OTP or recovery code while the first request's transaction still holds the row. Adds two tests that open a genuinely independent physical DB connection (verified via differing MySQL CONNECTION_ID()) and prove FOR UPDATE from that connection is blocked (lock wait timeout) while EmailOTPMFAChallengeStrategy/AbstractMFAChallengeStrategy's production refreshExclusiveLock() call holds the row. Verified the assertion is non-vacuous by temporarily disabling the lock call and confirming the test fails as expected, then restoring it.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
Best-effort audit logging around the MFA flows only caught Exception, which misses Error subtypes (TypeError, ArgumentCountError, etc.). An Error escaping any of these would still turn a clean response into an uncaught 500 or, worse for the two failure-path calls, drop the error_code the rate-limit middleware keys its failure counter on (TwoFactorRateLimitMiddleware::isFailure() only sees the JSON body of whatever response actually gets returned). Applies the codebase's existing convention for this exact situation (see app/Audit/AuditLoggerFactory.php, TrackRequestMiddleware.php) to all 7 best-effort audit/device-trust sites in this controller: - postLogin(): initial challenge issuance audit log (was unguarded) - verify2FA(): failure-path audit log (was unguarded) - verify2FA(): queueDeviceTrustCookie() call (was catch(Exception)) - verify2FA(): success-path audit log (was catch(Exception)) - verify2FARecovery(): failure-path audit log (was unguarded) - verify2FARecovery(): success-path audit log (was catch(Exception)) - resend2FA(): challenge-reissue audit log (was unguarded) Verified: full Two Factor Authentication Test Suite (83 tests, 241 assertions) passes unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
The passwordless-login guard called shouldRequire2FA() directly, which ignored config('two_factor.enabled'), so an enforced admin stayed blocked from passwordless login even with the kill-switch off (SDS idp-mfa.md rollout, section 10.1). Move the enabled check into shouldRequire2FA() as the single source of truth shared by both the MFA gate and the passwordless guard, and drop the now-redundant check in MFAGateService.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-136/ This page is automatically updated on each push to this PR. |
|
Resolved. As of Fixed in the follow-ups: Also resolved: |
3 participants
Task:
Ref: https://app.clickup.com/t/9014802374/86ba2zc6p
What Changed
UserController(app/Http/Controllers/UserController.php)IDeviceTrustService,ITwoFactorAuditService, andITwoFactorGateServicevia constructor.MFAGateService::requiresChallenge()returnstrue, issues a challenge (viaAuthService::issueMFAChallenge) and returns{ error_code: "mfa_required", ... }— no session created yet.resolveClientFromMemento()private helper to extract the OAuth2 client from a pending session memento (deduplicates logic previously repeated in password and OTP flows).postVerify2FA(),postResend2FA(), andpostRecovery2FA()action methods to handle the full MFA step-up login lifecycle.MFACookieManagertrait for reading/writing the trusted-device cookie.MFACookieManagertrait (app/Http/Controllers/Traits/MFACookieManager.php) — new filegetCookieToken(): ?string(reads raw token from cookie).queueDeviceTrustCookie(User $user): void(callsIDeviceTrustService::trustDeviceand queues a secure, HttpOnly cookie).TwoFactorRateLimitMiddleware(app/Http/Middleware/TwoFactorRateLimitMiddleware.php) — new fileverify,recovery, andresendMFA endpoints.verify/recovery: increments only on failed response (mfa_verification_failed,mfa_invalid_recovery).resend: increments on every request.HTTP 429witherror_code: mfa_rate_limitwhen the limit is exceeded.app/Http/Kernel.phpand wired to the new routes inroutes/web.php.AuthService(app/libs/Auth/AuthService.php) andIAuthService(app/libs/Utils/Services/IAuthService.php)validateCredentials(string $username, string $password): User— validates without creating a session.loginUser(User $user, bool $remember): void— establishes the session for an already-validated user.issueMFAChallenge(User $user, MFAChallengeStrategy $strategy, ?Client $client, bool $remember): array— triggers the challenge strategy and stores pending state.DeviceTrustServiceandTwoFactorAuditService(minor fixes)config/two_factor.php— new filecookie_name,device_trust_lifetime_days, rate-limit thresholds per action.EncryptCookiesmiddlewaredevice_trust_tokencookie from encryption (raw HMAC token must be read as-is byIDeviceTrustService).Tests
tests/TwoFactorLoginFlowTest.php(new)tests/DeviceTrustServiceTest.phptests/Unit/TwoFactorAuditServiceTest.phptests/Unit/MFA/EmailOTPMFAChallengeStrategyTest.phpissueMFAChallengepath.phpunit.xml