Skip to content

Commit 4864f50

Browse files
smarcetsmarcet
authored
fix(session):Added Session::flush() + Session::regenerate() at the end of AuthService::logout() (#118)
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
1 parent d40da20 commit 4864f50

File tree

4 files changed

+364
-5
lines changed

4 files changed

+364
-5
lines changed

‎app/Http/Controllers/HomeController.php‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
* limitations under the License.
1313
**/
1414
use Illuminate\Support\Facades\Auth;
15-
use Illuminate\Support\Facades\Session;
1615
use Illuminate\Support\Facades\View;
1716
use Illuminate\Support\Facades\Redirect;
1817
use App\Http\Controllers\OpenId\OpenIdController;
@@ -37,8 +36,6 @@ public function index()
3736
if ($this->isDiscoveryRequest())
3837
return $this->discovery->idp();
3938
if (Auth::guest()) {
40-
Session::flush();
41-
Session::regenerate();
4239
return View::make("home");
4340
}
4441
else

‎app/Http/Controllers/UserController.php‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -676,8 +676,6 @@ public function logout()
676676
$user = $this->auth_service->getCurrentUser();
677677
// RevokeUserGrantsOnExplicitLogout::dispatch($user)->afterResponse();
678678
$this->auth_service->logout();
679-
Session::flush();
680-
Session::regenerate();
681679
return Redirect::action("UserController@getLogin");
682680
}
683681

‎app/libs/Auth/AuthService.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,11 @@ public function logout(bool $clear_security_ctx = true):void
334334
$raw = false,
335335
$sameSite = 'none'
336336
);
337+
338+
// Flush all session data and regenerate the session ID to ensure no stale
339+
// data survives (OAuth2 memento, OpenID auth context, authorization responses, etc.)
340+
Session::flush();
341+
Session::regenerate();
337342
}
338343

339344
/**

‎tests/AuthServiceLogoutTest.php‎

Lines changed: 359 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,359 @@
1+
<?php namespace Tests;
2+
/**
3+
* Copyright 2026 OpenStack Foundation
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
* http://www.apache.org/licenses/LICENSE-2.0
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
**/
14+
15+
use App\libs\OAuth2\Repositories\IOAuth2OTPRepository;
16+
use Auth\AuthService;
17+
use Auth\Repositories\IUserRepository;
18+
use Mockery;
19+
use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
20+
use OAuth2\Services\IPrincipalService;
21+
use OAuth2\Services\ISecurityContextService;
22+
use OpenId\Services\IUserService;
23+
use App\Services\Auth\IUserService as IAuthUserService;
24+
use PHPUnit\Framework\TestCase as PHPUnitTestCase;
25+
use Services\IUserActionService;
26+
use Utils\Db\ITransactionService;
27+
use Utils\Services\ICacheService;
28+
use Utils\Services\IAuthService;
29+
30+
/**
31+
* Class AuthServiceLogoutTest
32+
* Tests that AuthService::logout() properly flushes all session data
33+
* and regenerates the session ID, fixing the incomplete session cleanup
34+
* that previously required callers to manually call Session::flush().
35+
*/
36+
#[\PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses]
37+
#[\PHPUnit\Framework\Attributes\PreserveGlobalState(false)]
38+
final class AuthServiceLogoutTest extends PHPUnitTestCase
39+
{
40+
use MockeryPHPUnitIntegration;
41+
42+
private AuthService $service;
43+
44+
private $mock_principal_service;
45+
private $mock_user_action_service;
46+
private $mock_cache_service;
47+
private $mock_security_context_service;
48+
49+
// Facade aliases
50+
private $auth_mock;
51+
private $session_mock;
52+
private $config_mock;
53+
private $cookie_mock;
54+
private $crypt_mock;
55+
private $log_mock;
56+
57+
protected function setUp(): void
58+
{
59+
parent::setUp();
60+
61+
$mock_user_repository = $this->createMock(IUserRepository::class);
62+
$mock_otp_repository = $this->createMock(IOAuth2OTPRepository::class);
63+
$this->mock_principal_service = $this->createMock(IPrincipalService::class);
64+
$mock_user_service = $this->createMock(IUserService::class);
65+
$this->mock_user_action_service = $this->createMock(IUserActionService::class);
66+
$this->mock_cache_service = $this->createMock(ICacheService::class);
67+
$mock_auth_user_service = $this->createMock(IAuthUserService::class);
68+
$this->mock_security_context_service = $this->createMock(ISecurityContextService::class);
69+
$mock_tx_service = $this->createMock(ITransactionService::class);
70+
71+
// Mock facades using Mockery alias (no Laravel app container needed)
72+
$this->auth_mock = Mockery::mock('alias:Illuminate\Support\Facades\Auth');
73+
$this->session_mock = Mockery::mock('alias:Illuminate\Support\Facades\Session');
74+
$this->config_mock = Mockery::mock('alias:Illuminate\Support\Facades\Config');
75+
$this->cookie_mock = Mockery::mock('alias:Illuminate\Support\Facades\Cookie');
76+
$this->crypt_mock = Mockery::mock('alias:Illuminate\Support\Facades\Crypt');
77+
$this->log_mock = Mockery::mock('alias:Illuminate\Support\Facades\Log');
78+
79+
// Log calls are always allowed
80+
$this->log_mock->shouldReceive('debug')->zeroOrMoreTimes();
81+
$this->log_mock->shouldReceive('debug_msg')->zeroOrMoreTimes();
82+
83+
$this->service = new AuthService(
84+
$mock_user_repository,
85+
$mock_otp_repository,
86+
$this->mock_principal_service,
87+
$mock_user_service,
88+
$this->mock_user_action_service,
89+
$this->mock_cache_service,
90+
$mock_auth_user_service,
91+
$this->mock_security_context_service,
92+
$mock_tx_service
93+
);
94+
}
95+
96+
private function mockGuestUser(): void
97+
{
98+
$this->auth_mock->shouldReceive('user')->andReturn(null);
99+
$this->auth_mock->shouldReceive('check')->andReturn(false);
100+
}
101+
102+
private function mockAuthenticatedUser(): Mockery\MockInterface
103+
{
104+
$user = Mockery::mock('Auth\User');
105+
$user->shouldReceive('getId')->andReturn(42);
106+
$this->auth_mock->shouldReceive('user')->andReturn($user);
107+
$this->auth_mock->shouldReceive('check')->andReturn(true);
108+
return $user;
109+
}
110+
111+
private function expectSessionInvalidation(): void
112+
{
113+
$this->session_mock->shouldReceive('getId')->once()->andReturn('test-session-id');
114+
$this->crypt_mock->shouldReceive('encrypt')->once()->with('test-session-id')->andReturn('encrypted-session-id');
115+
$this->mock_cache_service
116+
->expects($this->once())
117+
->method('addSingleValue')
118+
->with('encrypted-session-idinvalid', 'encrypted-session-id');
119+
}
120+
121+
private function expectCoreLogoutCalls(bool $clear_security_ctx = true): void
122+
{
123+
$this->mock_principal_service->expects($this->once())->method('clear');
124+
$this->auth_mock->shouldReceive('logout')->once();
125+
126+
if ($clear_security_ctx) {
127+
$this->mock_security_context_service->expects($this->once())->method('clear');
128+
} else {
129+
$this->mock_security_context_service->expects($this->never())->method('clear');
130+
}
131+
132+
$this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/');
133+
$this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com');
134+
$this->cookie_mock->shouldReceive('queue')->once();
135+
}
136+
137+
private function expectSessionFlushAndRegenerate(): void
138+
{
139+
$this->session_mock->shouldReceive('flush')->once();
140+
$this->session_mock->shouldReceive('regenerate')->once();
141+
}
142+
143+
/**
144+
* Verify that logout() calls Session::flush() and Session::regenerate()
145+
* when no user is logged in (guest context).
146+
*/
147+
public function testLogoutFlushesSessionForGuestUser(): void
148+
{
149+
$this->mockGuestUser();
150+
$this->expectSessionInvalidation();
151+
$this->expectCoreLogoutCalls();
152+
$this->expectSessionFlushAndRegenerate();
153+
154+
$this->service->logout();
155+
}
156+
157+
/**
158+
* Verify that logout() calls Session::flush() and Session::regenerate()
159+
* when an authenticated user is logged in.
160+
*/
161+
public function testLogoutFlushesSessionForAuthenticatedUser(): void
162+
{
163+
$this->mockAuthenticatedUser();
164+
$this->mock_user_action_service
165+
->expects($this->once())
166+
->method('addUserAction');
167+
168+
$this->expectSessionInvalidation();
169+
$this->expectCoreLogoutCalls();
170+
$this->expectSessionFlushAndRegenerate();
171+
172+
$this->service->logout();
173+
}
174+
175+
/**
176+
* Verify that Session::flush() is called AFTER Auth::logout() to ensure
177+
* the Laravel auth guard has already cleared its state before the session
178+
* is destroyed. This ordering prevents Auth::logout() from operating
179+
* on an empty session.
180+
*/
181+
public function testLogoutCallsFlushAfterAuthLogout(): void
182+
{
183+
$this->mockGuestUser();
184+
$this->expectSessionInvalidation();
185+
$this->mock_principal_service->expects($this->once())->method('clear');
186+
$this->mock_security_context_service->expects($this->once())->method('clear');
187+
188+
$this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/');
189+
$this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com');
190+
$this->cookie_mock->shouldReceive('queue')->once();
191+
192+
$call_order = [];
193+
194+
$this->auth_mock->shouldReceive('logout')->once()->andReturnUsing(function () use (&$call_order) {
195+
$call_order[] = 'auth_logout';
196+
});
197+
198+
$this->session_mock->shouldReceive('flush')->once()->andReturnUsing(function () use (&$call_order) {
199+
$call_order[] = 'session_flush';
200+
});
201+
202+
$this->session_mock->shouldReceive('regenerate')->once()->andReturnUsing(function () use (&$call_order) {
203+
$call_order[] = 'session_regenerate';
204+
});
205+
206+
$this->service->logout();
207+
208+
$this->assertEquals(['auth_logout', 'session_flush', 'session_regenerate'], $call_order);
209+
}
210+
211+
/**
212+
* Verify that Session::flush() is called AFTER invalidateSession()
213+
* captures the session ID. If flush happened first, the session ID
214+
* would be lost and the cache blacklist entry would be wrong.
215+
*/
216+
public function testLogoutCapturesSessionIdBeforeFlush(): void
217+
{
218+
$this->mockGuestUser();
219+
$this->mock_principal_service->expects($this->once())->method('clear');
220+
$this->mock_security_context_service->expects($this->once())->method('clear');
221+
$this->auth_mock->shouldReceive('logout')->once();
222+
223+
$this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/');
224+
$this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com');
225+
$this->cookie_mock->shouldReceive('queue')->once();
226+
227+
$session_id_captured = false;
228+
229+
$this->session_mock->shouldReceive('getId')->once()->andReturnUsing(function () use (&$session_id_captured) {
230+
$session_id_captured = true;
231+
return 'original-session-id';
232+
});
233+
234+
$this->crypt_mock->shouldReceive('encrypt')->once()->with('original-session-id')->andReturn('encrypted-id');
235+
$this->mock_cache_service
236+
->expects($this->once())
237+
->method('addSingleValue')
238+
->with('encrypted-idinvalid', 'encrypted-id');
239+
240+
$this->session_mock->shouldReceive('flush')->once()->andReturnUsing(function () use (&$session_id_captured) {
241+
$this->assertTrue($session_id_captured, 'Session::flush() was called before Session::getId()');
242+
});
243+
244+
$this->session_mock->shouldReceive('regenerate')->once();
245+
246+
$this->service->logout();
247+
}
248+
249+
/**
250+
* Verify that when clear_security_ctx is false, the security context
251+
* is NOT cleared but session flush still happens.
252+
*/
253+
public function testLogoutWithoutSecurityContextClearStillFlushesSession(): void
254+
{
255+
$this->mockGuestUser();
256+
$this->expectSessionInvalidation();
257+
$this->expectCoreLogoutCalls(clear_security_ctx: false);
258+
$this->expectSessionFlushAndRegenerate();
259+
260+
$this->service->logout(clear_security_ctx: false);
261+
}
262+
263+
/**
264+
* Verify that the rps cookie is queued for deletion during logout.
265+
* This ensures relying party tracking is cleaned up.
266+
*/
267+
public function testLogoutDeletesRpsCookie(): void
268+
{
269+
$this->mockGuestUser();
270+
$this->expectSessionInvalidation();
271+
$this->mock_principal_service->expects($this->once())->method('clear');
272+
$this->mock_security_context_service->expects($this->once())->method('clear');
273+
$this->auth_mock->shouldReceive('logout')->once();
274+
275+
$this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/test-path');
276+
$this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.test-domain.com');
277+
278+
$this->cookie_mock->shouldReceive('queue')->once()->with(
279+
IAuthService::LOGGED_RELAYING_PARTIES_COOKIE_NAME,
280+
null,
281+
-2628000,
282+
'/test-path',
283+
'.test-domain.com',
284+
true,
285+
true,
286+
false,
287+
'none'
288+
);
289+
290+
$this->expectSessionFlushAndRegenerate();
291+
292+
$this->service->logout();
293+
}
294+
295+
/**
296+
* Verify that principal_service->clear() is called during logout,
297+
* which removes the op_bs cookie and session keys (user_id, auth_time, opbs).
298+
*/
299+
public function testLogoutClearsPrincipalService(): void
300+
{
301+
$this->mockGuestUser();
302+
$this->expectSessionInvalidation();
303+
304+
$this->mock_principal_service
305+
->expects($this->once())
306+
->method('clear');
307+
308+
$this->mock_security_context_service->expects($this->once())->method('clear');
309+
$this->auth_mock->shouldReceive('logout')->once();
310+
311+
$this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/');
312+
$this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com');
313+
$this->cookie_mock->shouldReceive('queue')->once();
314+
315+
$this->expectSessionFlushAndRegenerate();
316+
317+
$this->service->logout();
318+
}
319+
320+
/**
321+
* Verify that user action logging captures the user ID and IP before
322+
* session data is destroyed.
323+
*/
324+
public function testLogoutLogsUserActionBeforeSessionDestroyed(): void
325+
{
326+
$this->mockAuthenticatedUser();
327+
328+
$action_logged = false;
329+
$session_flushed = false;
330+
331+
$user_action_mock = Mockery::mock('Models\UserAction');
332+
$this->mock_user_action_service
333+
->expects($this->once())
334+
->method('addUserAction')
335+
->willReturnCallback(function () use (&$action_logged, &$session_flushed, $user_action_mock) {
336+
$this->assertFalse($session_flushed, 'User action must be logged before session flush');
337+
$action_logged = true;
338+
return $user_action_mock;
339+
});
340+
341+
$this->expectSessionInvalidation();
342+
$this->mock_principal_service->expects($this->once())->method('clear');
343+
$this->mock_security_context_service->expects($this->once())->method('clear');
344+
$this->auth_mock->shouldReceive('logout')->once();
345+
346+
$this->config_mock->shouldReceive('get')->with('session.path')->andReturn('/');
347+
$this->config_mock->shouldReceive('get')->with('session.domain')->andReturn('.example.com');
348+
$this->cookie_mock->shouldReceive('queue')->once();
349+
350+
$this->session_mock->shouldReceive('flush')->once()->andReturnUsing(function () use (&$session_flushed) {
351+
$session_flushed = true;
352+
});
353+
$this->session_mock->shouldReceive('regenerate')->once();
354+
355+
$this->service->logout();
356+
357+
$this->assertTrue($action_logged, 'User action was never logged');
358+
}
359+
}

0 commit comments

Comments
 (0)