MFA hardening: OAuth2 memento guard on recovery, DTO refactors, full OIDC circuit tests - #152
Open
smarcet wants to merge 13 commits into
Open
Conversation
…code verify2FARecovery() skipped the resolveClientFromMemento() guard that verify2FA() applies, so with a pending OAuth2 authorization request whose client no longer exists the single-use recovery code was burned and an IDP session established for an authorization request that could only fail at the /oauth2/auth hop. Apply the same guard before redemption; recovery-code checking itself stays client-agnostic.
The error_code values emitted by UserController's MFA endpoints were hardcoded strings, duplicated in TwoFactorRateLimitMiddleware::FAILURE_CODES where a silent drift would break the rate-limit failure counting. Tests keep asserting the literal wire values on purpose, pinning the contract.
…n array MFAPendingState (getUserId / getPendingAt / shouldRemember) replaces the string-keyed array, so callers stop scattering 'user_id'/'remember' literals and casts, and the shape is enforced by the type system instead of by convention.
…tatus DTO verify2FARecovery() and getProfile() each hand-built the recovery_codes_remaining/total/low_threshold payload with their own config() reads and magic defaults. IRecoveryCodeService::getStatus() now returns a RecoveryCodesStatus DTO whose toArray() owns the wire keys, so both call sites merge the same serialized shape. Side effect: the recovery XHR response now also carries recovery_codes_total (additive, ignored by the SPA).
…fy2FARecovery authorize -> login -> MFA challenge -> verify (OTP / recovery code) -> redirect_url back to the authorization endpoint (rebuilt from the session memento) -> consent screen -> AllowOnce -> authorization code delivered to the client redirect_uri. Locks in that the XHR verify contract composes with the interactive grant's memento round-trip. Note: OIDCProtocolTestCase's password-login circuits (e.g. testAuthCode) predate the MFA gate and post a wrong seed password - broken independently of this change.
…d + MFA gate) Two stacked breakages, both predating and unrelated to each individual test: - 021bee3 (jul 2024) changed the TestSeeder passwords from '1qaz2wsx' to '1Qaz2wsx!' without updating this class, so every password login leg has silently failed since - errorLogin() also answers 302, so the post-login assertion kept passing and tests died downstream instead. - The MFA gate now challenges the seeded login user (SuperAdminGroup is in two_factor.enforced_groups), so even a correct password stops at the 2FA challenge. This class exercises the OIDC protocol, not the gate - enforced groups are cleared in prepareForTests(); the gate plus the full authorize -> MFA -> consent -> code circuit live in TwoFactorLoginFlowTest. Result: 29 broken -> 3 (32/35 green). The 3 residuals have distinct pre-existing causes: testConsentLogin and testGetRefreshTokenWithPromptSetToConsentLogin lose the login hint because AuthService::logout()'s Session::flush() (4864f50 / #118) wipes the session-backed security context even when called with clear_security_ctx = false (prompt=login path); testTokenResponseModePost uses max_age=1 and the multi-request dance now takes longer than 1s, forcing a re-login.
…lush The Session::flush() hardening added in #118 wipes the whole session at the end of logout(), including the session-backed security context - even when the caller passed clear_security_ctx = false (the prompt=login re-authentication path in InteractiveGrantType::mustAuthenticateUser()), which broke the login-hint prefill on the login screen for prompt=login OIDC requests. Capture the context before the flush and re-save it after the session ID regenerate; everything else is still flushed, so the #118 hardening stands.
The test exercises response_mode=form_post, not max_age expiry (testMaxAge1AndWait2 owns that) - with max_age=1 the multi-request login+consent dance takes longer than 1s and the final authorize hop forced a re-login instead of delivering the form post. 3200 matches the sibling circuits. OIDCProtocolTestCase is now fully green: 35/35.
|
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:
📝 WalkthroughWalkthroughThe PR introduces typed MFA pending state, centralized MFA error codes, recovery-code status responses, OAuth2 client validation, logout security-context preservation, and expanded MFA/OIDC test coverage. ChangesMFA authentication and recovery flow
| Sequence Diagram(s)sequenceDiagram
participant UserController
participant OAuth2Client
participant MFAChallengeStrategy
participant RecoveryCodeService
UserController->>MFAChallengeStrategy: Read pending MFA state
UserController->>OAuth2Client: Validate OAuth2 client
UserController->>RecoveryCodeService: Redeem recovery code
RecoveryCodeService-->>UserController: Return recovery status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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-152/ This page is automatically updated on each push to this PR. |
…overy Six tests inside a pending OIDC authorization-code flow, three per endpoint: - wrong code then correct code: the rejection keeps the pending challenge and the OAuth2 memento alive, and the retry completes the full circuit (consent -> authorization code). - consecutive wrong codes up to the rate-limit threshold: every attempt is 401 without a session, and once the window closes even the CORRECT code answers 429 - brute-forcing inside a pending flow buys no extra attempts. - burned single-use code (used recovery code / redeemed OTP): rejected like any invalid code, and the flow still completes afterwards with a fresh code (new recovery code / resent OTP).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-152/ This page is automatically updated on each push to this PR. |
- validator 412s (malformed request, no otp_value / recovery_code) - vanished pending user -> mfa_session_expired + pending state cleared - recovery without a pending challenge -> mfa_session_expired - stale OAuth2 client guard on verify2FA (parity with the recovery test): 412 before the OTP is redeemed - audit failure on the FAILED-verify path stays a clean 401 with the error_code the rate-limit middleware keys on, for both endpoints verify2FA line coverage 82.3% -> 95.2%, verify2FARecovery 82.7% -> 94.2%; the only uncovered lines left are the generic Exception -> 500 catches.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-152/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/OIDCProtocolTestCase.php (1)
135-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the seeded password into a class constant.
The literal
1Qaz2wsx!now appears at about 26 call sites in this file. This PR had to edit every one of them. A private constant, asTwoFactorLoginFlowTest::SEED_PASSWORDalready does, reduces the next seed change to one edit. Keep the trailing-space form at this line explicit, because that spacing is the subject under test.♻️ Proposed refactor
Add the constant near the top of the class:
final class OIDCProtocolTestCase extends OpenStackIDBaseTestCase { private const SEED_PASSWORD = '1Qaz2wsx!';Then replace the literals:
'username' => ' sebastian@tipit.net ', - 'password' => ' 1Qaz2wsx! ', + 'password' => ' ' . self::SEED_PASSWORD . ' ','username' => 'sebastian@tipit.net', - 'password' => '1Qaz2wsx!', + 'password' => self::SEED_PASSWORD,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/OIDCProtocolTestCase.php` at line 135, Extract the repeated seeded password into a private OIDCProtocolTestCase::SEED_PASSWORD class constant and replace the other exact password literals with that constant. Keep the password value with trailing spaces explicit at the shown password-field call site, since that test must continue verifying whitespace handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/libs/Auth/MFAConstants.php`:
- Around line 29-31: Add ERROR_CODE_VERIFICATION_FAILED and
ERROR_CODE_INVALID_RECOVERY to the MFA_ERROR_CODE definition, alongside the
existing ERROR_CODE_SESSION_EXPIRED entry, so all three MFA error codes are
exposed to the login SPA.
---
Nitpick comments:
In `@tests/OIDCProtocolTestCase.php`:
- Line 135: Extract the repeated seeded password into a private
OIDCProtocolTestCase::SEED_PASSWORD class constant and replace the other exact
password literals with that constant. Keep the password value with trailing
spaces explicit at the shown password-field call site, since that test must
continue verifying whitespace handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 95e6cae3-bc85-47e5-befa-d941108e62e7
📒 Files selected for processing (14)
app/Http/Controllers/UserController.phpapp/Http/Middleware/TwoFactorRateLimitMiddleware.phpapp/Services/Auth/IRecoveryCodeService.phpapp/Services/Auth/RecoveryCodeService.phpapp/Services/Auth/RecoveryCodesStatus.phpapp/Strategies/MFA/AbstractMFAChallengeStrategy.phpapp/Strategies/MFA/IMFAChallengeStrategy.phpapp/Strategies/MFA/MFAPendingState.phpapp/libs/Auth/AuthService.phpapp/libs/Auth/MFAConstants.phptests/OIDCProtocolTestCase.phptests/TwoFactorLoginFlowTest.phptests/unit/AuthServiceLogoutTest.phptests/unit/MFA/AbstractMFAChallengeStrategyTest.php
…ure breadth Two changes to phpunit.xml: - The Application suite's <directory> scan only picks up *Test.php (PHPUnit's default suffix), so the four concrete *TestCase.php protocol suites (OAuth2Protocol, OIDCProtocol, OIDCPasswordless, OpenIdProtocol - 93 tests) were NEVER executed by CI. That is how OIDCProtocolTestCase stayed broken for two years with green builds. They are now listed explicitly. - stopOnFailure=false so a run reports every failure instead of dying on the first one. Also fixes the one test the newly-wired suites surfaced: testResourceServerIntrospectionNotValidIP expected an unconditional 400, but the resource-server IP check became opt-in in #98 (oauth2.validate_resource_server_ip, default off) - the test now enables the flag before asserting the rejection. Full-suite evidence (523 tests): green except 8 pre-existing environment-dependent Turnstile tests that need TEST_USER_EMAIL / TEST_USER_PASSWORD and the Turnstile secrets CI injects (they pass in CI; locally their markTestSkipped guard is defeated by a typed-property TypeError when the env vars are absent).
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-152/ This page is automatically updated on each push to this PR. |
MFAConstants now owns all of them:
- error codes: the existing three plus mfa_rate_limit and mfa_required.
ITwoFactorRateLimitService::RATE_LIMIT_ERROR_CODE and
ILoginStrategy::MFA_REQUIRED alias it, so consumers keep their names while
the value is defined once.
- 2fa_* session keys: previously defined TWICE in production
(AbstractMFAChallengeStrategy's private consts and
ITwoFactorRateLimitService::PENDING_USER_SESSION_KEY) - both now alias
MFAConstants.
Also promotes the rate-limit cache-key prefix ('2fa_rate:', previously a
sprintf literal in TwoFactorRateLimitService duplicated by the test flush
helper) to ITwoFactorRateLimitService::RATE_LIMIT_CACHE_KEY_PREFIX.
All ~50 hardcoded literals across TwoFactorLoginFlowTest,
AbstractMFAChallengeStrategyTest and EmailOTPMFAChallengeStrategyTest now
reference the constants.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-152/ This page is automatically updated on each push to this PR. |
The literal appeared at 26 call sites; a seed password change is now a one-line edit, matching TwoFactorLoginFlowTest. The trailing-space login test keeps its spacing explicit around the constant, since that spacing is the subject under test. Suite re-run in idp-app: 35/35, 506 assertions.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-152/ This page is automatically updated on each push to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardening and cleanup on top of the 2FA feature (#126), plus repairs to the OIDC protocol test suite.
Fixes
resolveClientFromMemento()guard thatverify2FA()applies, so with a pending OAuth2 authorization request whose client no longer exists, the single-use recovery code was burned and an IDP session established for an authorization request that could only fail at the/oauth2/authhop. Covered by a red-green regression test.clear_security_ctx = falseacross the session flush. TheSession::flush()hardening from fix(session):Added Session::flush() + Session::regenerate() at the en… #118 wiped the session-backed security context even when the caller asked to keep it (theprompt=loginre-authentication path), breaking the login-hint prefill on the login screen. The context is now captured before the flush and re-saved after the session ID regenerate; everything else is still flushed. Covered byAuthServiceLogoutTest(red-green verified).Refactors
MFAConstants: the MFAerror_codewire literals, previously duplicated betweenUserControllerandTwoFactorRateLimitMiddleware::FAILURE_CODES(where silent drift would break rate-limit failure counting).MFAPendingStateDTO:IMFAChallengeStrategy::getPendingState()returns a typed object instead of a string-keyed array.RecoveryCodesStatusDTO:IRecoveryCodeService::getStatus()owns therecovery_codes_remaining/total/low_thresholdwire shape consumed byverify2FARecovery()andgetProfile()(which each hand-built it with inlineconfig()reads). Additive contract change: the recovery XHR response now also carriesrecovery_codes_total.Tests
OIDCProtocolTestCaserepaired: 29 broken → 0 (35/35 green). Two stacked pre-existing causes: seed passwords went stale in 021bee3 (jul 2024) and every login leg silently failed since (errorLogin()also answers 302); and the MFA gate now challenges the seeded super-admin, so enforced groups are cleared in this class (the gate is covered byTwoFactorLoginFlowTest). Also raisedtestTokenResponseModePost'smax_agefrom 1 to 3200 — it testsresponse_mode=form_post, not max_age expiry, and the login+consent dance takes longer than 1s.Test evidence (run inside the idp-app container)
TwoFactorLoginFlowTest: 43/43 (237 assertions)OIDCProtocolTestCase: 35/35 (506 assertions, stop-on-failure disabled)tests/unit/: 80/80 (2 pre-existing deprecations)Summary by CodeRabbit
New Features
Bug Fixes
Tests