From 49920425e2e2cf4f8a70e28b1b2083918dc34d0a Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 11 Mar 2026 13:24:08 -0300 Subject: [PATCH] fix(session):Added Session::flush() + Session::regenerate() at the end 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 --- app/Http/Controllers/HomeController.php | 3 - app/Http/Controllers/UserController.php | 2 - app/libs/Auth/AuthService.php | 5 + tests/AuthServiceLogoutTest.php | 359 ++++++++++++++++++++++++ 4 files changed, 364 insertions(+), 5 deletions(-) create mode 100644 tests/AuthServiceLogoutTest.php diff --git a/app/Http/Controllers/HomeController.php b/app/Http/Controllers/HomeController.php index 463f0c99..8a3dfe51 100644 --- a/app/Http/Controllers/HomeController.php +++ b/app/Http/Controllers/HomeController.php @@ -12,7 +12,6 @@ * limitations under the License. **/ use Illuminate\Support\Facades\Auth; -use Illuminate\Support\Facades\Session; use Illuminate\Support\Facades\View; use Illuminate\Support\Facades\Redirect; use App\Http\Controllers\OpenId\OpenIdController; @@ -37,8 +36,6 @@ public function index() if ($this->isDiscoveryRequest()) return $this->discovery->idp(); if (Auth::guest()) { - Session::flush(); - Session::regenerate(); return View::make("home"); } else diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 69368891..c8ab3b9c 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -675,8 +675,6 @@ public function logout() $user = $this->auth_service->getCurrentUser(); //RevokeUserGrantsOnExplicitLogout::dispatch($user)->afterResponse(); $this->auth_service->logout(); - Session::flush(); - Session::regenerate(); return Redirect::action("UserController@getLogin"); } diff --git a/app/libs/Auth/AuthService.php b/app/libs/Auth/AuthService.php index 88b24fe2..fe3b0a28 100644 --- a/app/libs/Auth/AuthService.php +++ b/app/libs/Auth/AuthService.php @@ -334,6 +334,11 @@ public function logout(bool $clear_security_ctx = true):void $raw = false, $sameSite = 'none' ); + + // Flush all session data and regenerate the session ID to ensure no stale + // data survives (OAuth2 memento, OpenID auth context, authorization responses, etc.) + Session::flush(); + Session::regenerate(); } /** diff --git a/tests/AuthServiceLogoutTest.php b/tests/AuthServiceLogoutTest.php new file mode 100644 index 00000000..219d3a58 --- /dev/null +++ b/tests/AuthServiceLogoutTest.php @@ -0,0 +1,359 @@ +createMock(IUserRepository::class); + $mock_otp_repository = $this->createMock(IOAuth2OTPRepository::class); + $this->mock_principal_service = $this->createMock(IPrincipalService::class); + $mock_user_service = $this->createMock(IUserService::class); + $this->mock_user_action_service = $this->createMock(IUserActionService::class); + $this->mock_cache_service = $this->createMock(ICacheService::class); + $mock_auth_user_service = $this->createMock(IAuthUserService::class); + $this->mock_security_context_service = $this->createMock(ISecurityContextService::class); + $mock_tx_service = $this->createMock(ITransactionService::class); + + // Mock facades using Mockery alias (no Laravel app container needed) + $this->auth_mock = Mockery::mock('alias:Illuminate\Support\Facades\Auth'); + $this->session_mock = Mockery::mock('alias:Illuminate\Support\Facades\Session'); + $this->config_mock = Mockery::mock('alias:Illuminate\Support\Facades\Config'); + $this->cookie_mock = Mockery::mock('alias:Illuminate\Support\Facades\Cookie'); + $this->crypt_mock = Mockery::mock('alias:Illuminate\Support\Facades\Crypt'); + $this->log_mock = Mockery::mock('alias:Illuminate\Support\Facades\Log'); + + // Log calls are always allowed + $this->log_mock->shouldReceive('debug')->zeroOrMoreTimes(); + $this->log_mock->shouldReceive('debug_msg')->zeroOrMoreTimes(); + + $this->service = new AuthService( + $mock_user_repository, + $mock_otp_repository, + $this->mock_principal_service, + $mock_user_service, + $this->mock_user_action_service, + $this->mock_cache_service, + $mock_auth_user_service, + $this->mock_security_context_service, + $mock_tx_service + ); + } + + private function mockGuestUser(): void + { + $this->auth_mock->shouldReceive('user')->andReturn(null); + $this->auth_mock->shouldReceive('check')->andReturn(false); + } + + private function mockAuthenticatedUser(): Mockery\MockInterface + { + $user = Mockery::mock('Auth\User'); + $user->shouldReceive('getId')->andReturn(42); + $this->auth_mock->shouldReceive('user')->andReturn($user); + $this->auth_mock->shouldReceive('check')->andReturn(true); + return $user; + } + + private function expectSessionInvalidation(): void + { + $this->session_mock->shouldReceive('getId')->once()->andReturn('test-session-id'); + $this->crypt_mock->shouldReceive('encrypt')->once()->with('test-session-id')->andReturn('encrypted-session-id'); + $this->mock_cache_service + ->expects($this->once()) + ->method('addSingleValue') + ->with('encrypted-session-idinvalid', 'encrypted-session-id'); + } + + private function expectCoreLogoutCalls(bool $clear_security_ctx = true): void + { + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->auth_mock->shouldReceive('logout')->once(); + + if ($clear_security_ctx) { + $this->mock_security_context_service->expects($this->once())->method('clear'); + } else { + $this->mock_security_context_service->expects($this->never())->method('clear'); + } + + $this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/'); + $this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com'); + $this->cookie_mock->shouldReceive('queue')->once(); + } + + private function expectSessionFlushAndRegenerate(): void + { + $this->session_mock->shouldReceive('flush')->once(); + $this->session_mock->shouldReceive('regenerate')->once(); + } + + /** + * Verify that logout() calls Session::flush() and Session::regenerate() + * when no user is logged in (guest context). + */ + public function testLogoutFlushesSessionForGuestUser(): void + { + $this->mockGuestUser(); + $this->expectSessionInvalidation(); + $this->expectCoreLogoutCalls(); + $this->expectSessionFlushAndRegenerate(); + + $this->service->logout(); + } + + /** + * Verify that logout() calls Session::flush() and Session::regenerate() + * when an authenticated user is logged in. + */ + public function testLogoutFlushesSessionForAuthenticatedUser(): void + { + $this->mockAuthenticatedUser(); + $this->mock_user_action_service + ->expects($this->once()) + ->method('addUserAction'); + + $this->expectSessionInvalidation(); + $this->expectCoreLogoutCalls(); + $this->expectSessionFlushAndRegenerate(); + + $this->service->logout(); + } + + /** + * Verify that Session::flush() is called AFTER Auth::logout() to ensure + * the Laravel auth guard has already cleared its state before the session + * is destroyed. This ordering prevents Auth::logout() from operating + * on an empty session. + */ + public function testLogoutCallsFlushAfterAuthLogout(): void + { + $this->mockGuestUser(); + $this->expectSessionInvalidation(); + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->mock_security_context_service->expects($this->once())->method('clear'); + + $this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/'); + $this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com'); + $this->cookie_mock->shouldReceive('queue')->once(); + + $call_order = []; + + $this->auth_mock->shouldReceive('logout')->once()->andReturnUsing(function () use (&$call_order) { + $call_order[] = 'auth_logout'; + }); + + $this->session_mock->shouldReceive('flush')->once()->andReturnUsing(function () use (&$call_order) { + $call_order[] = 'session_flush'; + }); + + $this->session_mock->shouldReceive('regenerate')->once()->andReturnUsing(function () use (&$call_order) { + $call_order[] = 'session_regenerate'; + }); + + $this->service->logout(); + + $this->assertEquals(['auth_logout', 'session_flush', 'session_regenerate'], $call_order); + } + + /** + * Verify that Session::flush() is called AFTER invalidateSession() + * captures the session ID. If flush happened first, the session ID + * would be lost and the cache blacklist entry would be wrong. + */ + public function testLogoutCapturesSessionIdBeforeFlush(): void + { + $this->mockGuestUser(); + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->mock_security_context_service->expects($this->once())->method('clear'); + $this->auth_mock->shouldReceive('logout')->once(); + + $this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/'); + $this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com'); + $this->cookie_mock->shouldReceive('queue')->once(); + + $session_id_captured = false; + + $this->session_mock->shouldReceive('getId')->once()->andReturnUsing(function () use (&$session_id_captured) { + $session_id_captured = true; + return 'original-session-id'; + }); + + $this->crypt_mock->shouldReceive('encrypt')->once()->with('original-session-id')->andReturn('encrypted-id'); + $this->mock_cache_service + ->expects($this->once()) + ->method('addSingleValue') + ->with('encrypted-idinvalid', 'encrypted-id'); + + $this->session_mock->shouldReceive('flush')->once()->andReturnUsing(function () use (&$session_id_captured) { + $this->assertTrue($session_id_captured, 'Session::flush() was called before Session::getId()'); + }); + + $this->session_mock->shouldReceive('regenerate')->once(); + + $this->service->logout(); + } + + /** + * Verify that when clear_security_ctx is false, the security context + * is NOT cleared but session flush still happens. + */ + public function testLogoutWithoutSecurityContextClearStillFlushesSession(): void + { + $this->mockGuestUser(); + $this->expectSessionInvalidation(); + $this->expectCoreLogoutCalls(clear_security_ctx: false); + $this->expectSessionFlushAndRegenerate(); + + $this->service->logout(clear_security_ctx: false); + } + + /** + * Verify that the rps cookie is queued for deletion during logout. + * This ensures relying party tracking is cleaned up. + */ + public function testLogoutDeletesRpsCookie(): void + { + $this->mockGuestUser(); + $this->expectSessionInvalidation(); + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->mock_security_context_service->expects($this->once())->method('clear'); + $this->auth_mock->shouldReceive('logout')->once(); + + $this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/test-path'); + $this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.test-domain.com'); + + $this->cookie_mock->shouldReceive('queue')->once()->with( + IAuthService::LOGGED_RELAYING_PARTIES_COOKIE_NAME, + null, + -2628000, + '/test-path', + '.test-domain.com', + true, + true, + false, + 'none' + ); + + $this->expectSessionFlushAndRegenerate(); + + $this->service->logout(); + } + + /** + * Verify that principal_service->clear() is called during logout, + * which removes the op_bs cookie and session keys (user_id, auth_time, opbs). + */ + public function testLogoutClearsPrincipalService(): void + { + $this->mockGuestUser(); + $this->expectSessionInvalidation(); + + $this->mock_principal_service + ->expects($this->once()) + ->method('clear'); + + $this->mock_security_context_service->expects($this->once())->method('clear'); + $this->auth_mock->shouldReceive('logout')->once(); + + $this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/'); + $this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com'); + $this->cookie_mock->shouldReceive('queue')->once(); + + $this->expectSessionFlushAndRegenerate(); + + $this->service->logout(); + } + + /** + * Verify that user action logging captures the user ID and IP before + * session data is destroyed. + */ + public function testLogoutLogsUserActionBeforeSessionDestroyed(): void + { + $this->mockAuthenticatedUser(); + + $action_logged = false; + $session_flushed = false; + + $user_action_mock = Mockery::mock('Models\UserAction'); + $this->mock_user_action_service + ->expects($this->once()) + ->method('addUserAction') + ->willReturnCallback(function () use (&$action_logged, &$session_flushed, $user_action_mock) { + $this->assertFalse($session_flushed, 'User action must be logged before session flush'); + $action_logged = true; + return $user_action_mock; + }); + + $this->expectSessionInvalidation(); + $this->mock_principal_service->expects($this->once())->method('clear'); + $this->mock_security_context_service->expects($this->once())->method('clear'); + $this->auth_mock->shouldReceive('logout')->once(); + + $this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/'); + $this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com'); + $this->cookie_mock->shouldReceive('queue')->once(); + + $this->session_mock->shouldReceive('flush')->once()->andReturnUsing(function () use (&$session_flushed) { + $session_flushed = true; + }); + $this->session_mock->shouldReceive('regenerate')->once(); + + $this->service->logout(); + + $this->assertTrue($action_logged, 'User action was never logged'); + } +}