fix(session):Added Session::flush() + Session::regenerate() at the en… - #118
Merged
Conversation
…d of AuthService::logout() after all other cleanup operations complete. This ensures: 1. invalidateSession() captures the session ID for the cache blacklist before flush 2. principal_service->clear() and security_context_service->clear() run first (now redundant but harmless) 3. Auth::logout() clears Laravel auth state before the session is destroyed 4. Session::flush() removes ALL session data in one operation 5. Session::regenerate() creates a fresh session ID to prevent fixation
smarcet
requested review from
Copilot and
romanetar
and removed request for
Copilot
March 11, 2026 16:25
📝 WalkthroughWalkthroughSession management operations are being consolidated from multiple controllers into the AuthService. Session::flush() and Session::regenerate() calls are removed from HomeController and UserController, then centralized in AuthService::logout(). A comprehensive test suite validates the refactored behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/AuthServiceLogoutTest.php (1)
79-81: Minor:debug_msgis not a Laravel Log facade method.Line 81 mocks
debug_msgwhich doesn't exist on Laravel's Log facade. This is harmless since Mockery will just ignore calls that don't happen, but it's unnecessary noise.🧹 Suggested cleanup
// Log calls are always allowed $this->log_mock->shouldReceive('debug')->zeroOrMoreTimes(); - $this->log_mock->shouldReceive('debug_msg')->zeroOrMoreTimes();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/AuthServiceLogoutTest.php` around lines 79 - 81, Remove the unnecessary mock for the non-existent Log method by deleting the shouldReceive('debug_msg') line in the test setup where $this->log_mock is configured; keep the valid shouldReceive('debug')->zeroOrMoreTimes() call so logging remains allowed, and ensure no other tests rely on a custom debug_msg expectation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/AuthServiceLogoutTest.php`:
- Around line 79-81: Remove the unnecessary mock for the non-existent Log method
by deleting the shouldReceive('debug_msg') line in the test setup where
$this->log_mock is configured; keep the valid
shouldReceive('debug')->zeroOrMoreTimes() call so logging remains allowed, and
ensure no other tests rely on a custom debug_msg expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 302357c8-3e6c-4ae2-a3d8-2b1246bc4234
📒 Files selected for processing (4)
app/Http/Controllers/HomeController.phpapp/Http/Controllers/UserController.phpapp/libs/Auth/AuthService.phptests/AuthServiceLogoutTest.php
💤 Files with no reviewable changes (2)
- app/Http/Controllers/UserController.php
- app/Http/Controllers/HomeController.php
smarcet
added a commit
that referenced
this pull request
Aug 12, 2026
…OIDC circuit tests (#152) * fix(2fa): validate pending OAuth2 client before redeeming a recovery 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. * refactor(2fa): extract MFA error_code literals into MFAConstants 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. * refactor(2fa): return a typed DTO from getPendingState() instead of an 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. * refactor(2fa): serialize recovery-codes standing via a RecoveryCodesStatus 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). * test(2fa): prove the full OIDC consent circuit for verify2FA and verify2FARecovery 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. * test(oidc): repair OIDCProtocolTestCase login circuits (stale password + 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. * fix(auth): honor clear_security_ctx=false across logout()'s session flush 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. * test(oidc): raise testTokenResponseModePost max_age from 1 to 3200 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. * test(2fa): negative-path OIDC circuits for verify2FA and verify2FARecovery 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). * test(2fa): cover the error branches of verify2FA and verify2FARecovery - 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. * test(ci): actually run the protocol TestCase suites, stop hiding failure 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). * refactor(2fa): single home for every MFA string constant 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. * test(oidc): extract the seeded password into a SEED_PASSWORD constant 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.
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
1 participant
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 by CodeRabbit
Chores
Tests