From 3d6cd09b646cdd45d3f2d1b9500136d573d7a685 Mon Sep 17 00:00:00 2001 From: gbutler Date: Mon, 9 Mar 2026 12:46:29 -0500 Subject: [PATCH 1/5] fix(security): revoke tokens and invalidate sessions on password change - Introduce RevokeUserGrants job (replaces RevokeUserGrantsOnExplicitLogout) with an optional client_id and a reason parameter; remove the guard clause that silently prevented all-client revocation. - Wire RevokeUserGrants to UserPasswordResetSuccessful so all OAuth2 tokens are revoked whenever a password is reset or changed. - Rotate remember_token in User::setPassword() to invalidate remember-me cookies on other devices. - Regenerate the session in UserApiController::updateMe() after a password change to protect against session fixation. - Add DELETE /admin/api/v1/users/me/tokens endpoint with a corresponding "Sign Out All Other Devices" button on the profile page. - Add PasswordChangeRevokeTokenTest covering all eight scenarios from the security specification. --- .../Controllers/Api/UserApiController.php | 19 +- app/Http/Controllers/UserController.php | 3 +- app/Jobs/RevokeUserGrants.php | 114 +++++++ app/Jobs/RevokeUserGrantsOnExplicitLogout.php | 83 ----- app/Listeners/OnUserLogout.php | 4 +- app/Providers/EventServiceProvider.php | 5 +- app/libs/Auth/Models/User.php | 2 + app/libs/OAuth2/OAuth2Protocol.php | 6 +- resources/js/profile/actions.js | 4 + resources/js/profile/profile.js | 35 ++- resources/views/profile.blade.php | 1 + routes/web.php | 1 + tests/PasswordChangeRevokeTokenTest.php | 285 ++++++++++++++++++ 13 files changed, 469 insertions(+), 93 deletions(-) create mode 100644 app/Jobs/RevokeUserGrants.php delete mode 100644 app/Jobs/RevokeUserGrantsOnExplicitLogout.php create mode 100644 tests/PasswordChangeRevokeTokenTest.php diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index 12fc34aa..7e2ac09b 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -13,6 +13,7 @@ **/ use App\Http\Controllers\APICRUDController; +use App\Jobs\RevokeUserGrants; use App\Http\Controllers\Traits\RequestProcessor; use App\Http\Controllers\UserValidationRulesFactory; use App\ModelSerializers\SerializerRegistry; @@ -244,7 +245,23 @@ public function updateMe() if (!Auth::check()) return $this->error403(); - return $this->update(Auth::user()->getId()); + $password_changed = request()->filled('password'); + $response = $this->update(Auth::user()->getId()); + if ($password_changed) { + request()->session()->regenerate(); + } + return $response; + } + + public function revokeAllMyTokens() + { + if (!Auth::check()) + return $this->error403(); + + $user = Auth::user(); + RevokeUserGrants::dispatch($user, null, 'user-initiated session revocation')->afterResponse(); + $user->setRememberToken(\Illuminate\Support\Str::random(60)); + return $this->deleted(); } public function updateMyPic(){ diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 69368891..a2efe87c 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -13,6 +13,7 @@ **/ use App\Http\Controllers\OpenId\DiscoveryController; +use App\Jobs\RevokeUserGrants; use App\Http\Controllers\OpenId\OpenIdController; use App\Http\Controllers\Traits\JsonResponses; use App\Http\Utils\CountryList; @@ -673,7 +674,7 @@ public function getIdentity($identifier) public function logout() { $user = $this->auth_service->getCurrentUser(); - //RevokeUserGrantsOnExplicitLogout::dispatch($user)->afterResponse(); + RevokeUserGrants::dispatch($user, null, 'explicit logout')->afterResponse(); $this->auth_service->logout(); Session::flush(); Session::regenerate(); diff --git a/app/Jobs/RevokeUserGrants.php b/app/Jobs/RevokeUserGrants.php new file mode 100644 index 00000000..c5483634 --- /dev/null +++ b/app/Jobs/RevokeUserGrants.php @@ -0,0 +1,114 @@ +user_id = $user->getId(); + $this->client_id = $client_id; + $this->reason = $reason; + Log::debug(sprintf( + "RevokeUserGrants::constructor user %s client_id %s reason %s", + $this->user_id, + $client_id ?? 'N/A', + $reason + )); + } + + public function handle(ITokenService $service): void + { + Log::debug("RevokeUserGrants::handle"); + + try { + $scope = !empty($this->client_id) + ? sprintf("client %s", $this->client_id) + : "all clients"; + + $action = sprintf( + "Revoking all grants for user %s on %s due to %s.", + $this->user_id, + $scope, + $this->reason + ); + + AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action); + + // Emit to OTEL audit log (Elasticsearch) if enabled + if (config('opentelemetry.enabled', false)) { + EmitAuditLogJob::dispatch('audit.security.tokens_revoked', [ + 'audit.action' => 'revoke_tokens', + 'audit.entity' => 'User', + 'audit.entity_id' => (string) $this->user_id, + 'audit.timestamp' => now()->toISOString(), + 'audit.description' => $action, + 'audit.reason' => $this->reason, + 'audit.scope' => $scope, + 'auth.user.id' => $this->user_id, + 'client.ip' => IPHelper::getUserIp(), + 'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'), + ]); + } + + $service->revokeUsersToken($this->user_id, $this->client_id); + } catch (\Exception $ex) { + Log::error($ex); + } + } + + public function failed(\Throwable $exception): void + { + Log::error(sprintf("RevokeUserGrants::failed %s", $exception->getMessage())); + } +} diff --git a/app/Jobs/RevokeUserGrantsOnExplicitLogout.php b/app/Jobs/RevokeUserGrantsOnExplicitLogout.php deleted file mode 100644 index 33f139e8..00000000 --- a/app/Jobs/RevokeUserGrantsOnExplicitLogout.php +++ /dev/null @@ -1,83 +0,0 @@ -user_id = $user->getId(); - $this->client_id = $client_id; - Log::debug(sprintf("RevokeUserGrants::constructor user %s client id %s", $this->user_id, !empty($client_id)? $client_id :"N/A")); - } - - public function handle(ITokenService $service){ - Log::debug(sprintf("RevokeUserGrants::handle")); - - if(empty($this->client_id)) { - return; - } - try{ - $action = sprintf - ( - "Revoking all grants for user %s on %s due explicit Log out.", - $this->user_id, sprintf("Client %s", $this->client_id) - ); - - AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action); - $service->revokeUsersToken($this->user_id, $this->client_id); - } - catch (\Exception $ex) { - Log::error($ex); - } - } - - public function failed(\Throwable $exception) - { - Log::error(sprintf( "RevokeUserGrants::failed %s", $exception->getMessage())); - } -} \ No newline at end of file diff --git a/app/Listeners/OnUserLogout.php b/app/Listeners/OnUserLogout.php index 12a5e318..c3a1e904 100644 --- a/app/Listeners/OnUserLogout.php +++ b/app/Listeners/OnUserLogout.php @@ -12,7 +12,7 @@ * limitations under the License. **/ -use App\Jobs\RevokeUserGrantsOnExplicitLogout; +use App\Jobs\RevokeUserGrants; use Illuminate\Auth\Events\Logout; use Illuminate\Support\Facades\Log; @@ -33,6 +33,6 @@ public function handle(Logout $event) { $user = $event->user; Log::debug(sprintf("OnUserLogout::handle user %s (%s)", $user->getEmail(), $user->getId())); - RevokeUserGrantsOnExplicitLogout::dispatch($user); + RevokeUserGrants::dispatch($user, null, 'explicit logout'); } } diff --git a/app/Providers/EventServiceProvider.php b/app/Providers/EventServiceProvider.php index 810289a1..155f5e3d 100644 --- a/app/Providers/EventServiceProvider.php +++ b/app/Providers/EventServiceProvider.php @@ -19,6 +19,7 @@ use App\Events\UserLocked; use App\Events\UserPasswordResetRequestCreated; use App\Events\UserPasswordResetSuccessful; +use App\Jobs\RevokeUserGrants; use App\Events\UserSpamStateUpdated; use App\Audit\AuditContext; use App\libs\Auth\Repositories\IUserPasswordResetRequestRepository; @@ -57,7 +58,7 @@ final class EventServiceProvider extends ServiceProvider 'Illuminate\Database\Events\QueryExecuted' => [ ], 'Illuminate\Auth\Events\Logout' => [ - //'App\Listeners\OnUserLogout', + 'App\Listeners\OnUserLogout', ], 'Illuminate\Auth\Events\Login' => [ 'App\Listeners\OnUserLogin', @@ -169,6 +170,8 @@ public function boot() if(is_null($user)) return; if(!$user instanceof User) return; Mail::queue(new UserPasswordResetMail($user)); + // Revoke all OAuth2 tokens for this user across all clients + RevokeUserGrants::dispatch($user, null, 'password change')->afterResponse(); }); Event::listen(OAuth2ClientLocked::class, function($event){ diff --git a/app/libs/Auth/Models/User.php b/app/libs/Auth/Models/User.php index 919a0efa..c1d70d96 100644 --- a/app/libs/Auth/Models/User.php +++ b/app/libs/Auth/Models/User.php @@ -1578,6 +1578,8 @@ public function setPassword(string $password): void $this->password_salt = AuthHelper::generateSalt(self::SaltLen, $this->password_enc); $this->password = AuthHelper::encrypt_password($password, $this->password_salt, $this->password_enc); + $this->setRememberToken(\Illuminate\Support\Str::random(60)); + $action = 'User set new password.'; $current_user = Auth::user(); if($current_user instanceof User) { diff --git a/app/libs/OAuth2/OAuth2Protocol.php b/app/libs/OAuth2/OAuth2Protocol.php index 2c064dd5..13b53eed 100644 --- a/app/libs/OAuth2/OAuth2Protocol.php +++ b/app/libs/OAuth2/OAuth2Protocol.php @@ -13,7 +13,7 @@ **/ use App\Http\Utils\UserIPHelperProvider; -use App\Jobs\RevokeUserGrantsOnExplicitLogout; +use App\Jobs\RevokeUserGrants; use Exception; use Illuminate\Support\Facades\Log; use jwa\JSONWebSignatureAndEncryptionAlgorithms; @@ -1562,11 +1562,9 @@ public function endSession(OAuth2Request $request = null) ); } - /* if(!is_null($user)){ - RevokeUserGrantsOnExplicitLogout::dispatch($user, $client_id)->afterResponse(); + RevokeUserGrants::dispatch($user, $client_id, 'explicit logout')->afterResponse(); } - */ if (!is_null($logged_user)) { $this->auth_service->logout(); diff --git a/resources/js/profile/actions.js b/resources/js/profile/actions.js index b7fa2ba1..c1c6c2f0 100644 --- a/resources/js/profile/actions.js +++ b/resources/js/profile/actions.js @@ -83,6 +83,10 @@ export const revokeToken = async (value, hint) => { )({'X-CSRF-TOKEN': window.CSFR_TOKEN}); } +export const revokeAllTokens = async () => { + return deleteRawRequest(window.REVOKE_ALL_TOKENS_ENDPOINT)({'X-CSRF-TOKEN': window.CSFR_TOKEN}); +} + const normalizeEntity = (entity) => { entity.public_profile_show_photo = entity.public_profile_show_photo ? 1 : 0; entity.public_profile_show_fullname = entity.public_profile_show_fullname ? 1 : 0; diff --git a/resources/js/profile/profile.js b/resources/js/profile/profile.js index c9e89c87..a530d04a 100644 --- a/resources/js/profile/profile.js +++ b/resources/js/profile/profile.js @@ -20,7 +20,7 @@ import RichTextEditor from "../components/rich_text_editor"; import FormControlLabel from "@material-ui/core/FormControlLabel"; import UserAccessTokensGrid from "../components/user_access_tokens_grid"; import UserActionsGrid from "../components/user_actions_grid"; -import {getUserActions, getUserAccessTokens, PAGE_SIZE, revokeToken, save} from "./actions"; +import {getUserActions, getUserAccessTokens, PAGE_SIZE, revokeToken, revokeAllTokens, save} from "./actions"; import ProfileImageUploader from "./components/profile_image_uploader/profile_image_uploader"; import Navbar from "../components/navbar/navbar"; import Divider from "@material-ui/core/Divider"; @@ -82,6 +82,30 @@ const ProfilePage = ({ }, }); + const confirmRevokeAll = () => { + Swal({ + title: 'Sign out all other devices?', + html: '', + showCancelButton: true, + confirmButtonColor: '#3085d6', + cancelButtonColor: '#d33', + confirmButtonText: 'Yes, sign out all devices!' + }).then((result) => { + if (result.value) { + revokeAllTokens().then(() => { + Swal('Signed out', 'All other sessions and tokens have been revoked.', 'success'); + setAccessTokensListRefresh(!accessTokensListRefresh); + }).catch((err) => { + handleErrorResponse(err); + }); + } + }); + }; + const confirmRevocation = (value) => { Swal({ title: 'Are you sure to revoke this token?', @@ -840,6 +864,15 @@ const ProfilePage = ({ onRevoke={confirmRevocation} /> + + + diff --git a/resources/views/profile.blade.php b/resources/views/profile.blade.php index 811648d0..335094e7 100644 --- a/resources/views/profile.blade.php +++ b/resources/views/profile.blade.php @@ -109,6 +109,7 @@ window.GET_USER_ACTIONS_ENDPOINT = '{{URL::action("Api\UserActionApiController@getActionsByCurrentUser")}}'; window.GET_USER_ACCESS_TOKENS_ENDPOINT = '{{URL::action("Api\ClientApiController@getAccessTokensByCurrentUser")}}'; window.REVOKE_ACCESS_TOKENS_ENDPOINT = '{!!URL::action("Api\UserApiController@revokeMyToken", ["value"=>"@value", "hint"=>"@hint"])!!}'; + window.REVOKE_ALL_TOKENS_ENDPOINT = '{{URL::action("Api\UserApiController@revokeAllMyTokens")}}'; window.SAVE_PROFILE_ENDPOINT = '{!!URL::action("Api\UserApiController@updateMe")!!}'; window.SAVE_PIC_ENDPOINT = '{!!URL::action("Api\UserApiController@updateMyPic")!!}'; window.CSFR_TOKEN = document.head.querySelector('meta[name="csrf-token"]').content; diff --git a/routes/web.php b/routes/web.php index f8473c4c..b49a3547 100644 --- a/routes/web.php +++ b/routes/web.php @@ -187,6 +187,7 @@ Route::post('', ['middleware' => ['openstackid.currentuser.serveradmin.json'], 'uses' => "UserApiController@create"]); Route::group(['prefix' => 'me'], function () { + Route::delete('tokens', "UserApiController@revokeAllMyTokens"); Route::delete('tokens/{value}', "UserApiController@revokeMyToken"); Route::put('', "UserApiController@updateMe"); Route::put('pic', "UserApiController@updateMyPic"); diff --git a/tests/PasswordChangeRevokeTokenTest.php b/tests/PasswordChangeRevokeTokenTest.php new file mode 100644 index 00000000..a0f8f9a1 --- /dev/null +++ b/tests/PasswordChangeRevokeTokenTest.php @@ -0,0 +1,285 @@ +test_user = $user_repository->findOneBy(['identifier' => 'sebastian.marcet']); + $this->be($this->test_user, 'web'); + } + + protected function tearDown(): void + { + $this->addToAssertionCount(Mockery::getContainer()->mockery_getExpectationCount()); + Mockery::close(); + parent::tearDown(); + } + + // ------------------------------------------------------------------------- + // Test 1: Dispatching UserPasswordResetSuccessful fires RevokeUserGrants + // ------------------------------------------------------------------------- + + /** + * When a UserPasswordResetSuccessful event is fired the EventServiceProvider + * listener must schedule a RevokeUserGrants job (all clients, reason = 'password change'). + */ + public function testPasswordResetEventDispatchesRevokeUserGrantsJob(): void + { + Event::dispatch(new UserPasswordResetSuccessful($this->test_user->getId())); + + // afterResponse() registers a terminating callback; fire it now. + app()->terminate(); + + Queue::assertPushed(RevokeUserGrants::class, function (RevokeUserGrants $job) { + $ref = new \ReflectionObject($job); + $userId = $ref->getProperty('user_id'); + $clientId = $ref->getProperty('client_id'); + $reason = $ref->getProperty('reason'); + $userId->setAccessible(true); + $clientId->setAccessible(true); + $reason->setAccessible(true); + + return $userId->getValue($job) === $this->test_user->getId() + && $clientId->getValue($job) === null + && $reason->getValue($job) === 'password change'; + }); + } + + // ------------------------------------------------------------------------- + // Test 2: PUT /admin/api/v1/users/me with password schedules RevokeUserGrants + // ------------------------------------------------------------------------- + + /** + * Posting a new password via the profile API must schedule a RevokeUserGrants + * job so tokens from other sessions are revoked. + */ + public function testProfilePasswordChangePutsRevokeUserGrantsJobOnQueue(): void + { + $payload = [ + 'first_name' => $this->test_user->getFirstName(), + 'last_name' => $this->test_user->getLastName(), + 'email' => $this->test_user->getEmail(), + 'current_password' => '1Qaz2wsx!', + 'password' => 'NewP@ssw0rd!99', + 'password_confirmation' => 'NewP@ssw0rd!99', + ]; + + $this->put('/admin/api/v1/users/me', $payload) + ->assertResponseStatus(201); + + // The listener for UserPasswordResetSuccessful uses afterResponse(). + app()->terminate(); + + Queue::assertPushed(RevokeUserGrants::class, function (RevokeUserGrants $job) { + $ref = new \ReflectionObject($job); + $clientId = $ref->getProperty('client_id'); + $reason = $ref->getProperty('reason'); + $clientId->setAccessible(true); + $reason->setAccessible(true); + + return $clientId->getValue($job) === null + && $reason->getValue($job) === 'password change'; + }); + } + + // ------------------------------------------------------------------------- + // Test 3: setPassword() rotates the remember_token + // ------------------------------------------------------------------------- + + /** + * Calling User::setPassword() must rotate the remember_token so that + * "remember me" cookies issued to other devices become invalid. + */ + public function testSetPasswordRotatesRememberToken(): void + { + $original_token = $this->test_user->getRememberToken(); + + $this->test_user->setPassword('AnotherS3cur3!Pass'); + + $new_token = $this->test_user->getRememberToken(); + + $this->assertNotEmpty($new_token); + $this->assertNotEquals($original_token, $new_token); + } + + // ------------------------------------------------------------------------- + // Test 4: Current session is preserved (regenerated) after password change + // ------------------------------------------------------------------------- + + /** + * After a profile password change the API response must be successful and + * the user must remain authenticated (session is regenerated, not destroyed). + */ + public function testCurrentSessionPreservedAfterPasswordChange(): void + { + $payload = [ + 'first_name' => $this->test_user->getFirstName(), + 'last_name' => $this->test_user->getLastName(), + 'email' => $this->test_user->getEmail(), + 'current_password' => '1Qaz2wsx!', + 'password' => 'Preserved@Sess10n!', + 'password_confirmation' => 'Preserved@Sess10n!', + ]; + + $this->put('/admin/api/v1/users/me', $payload) + ->assertResponseStatus(201); + + // The currently authenticated user must still be set after the call. + $this->assertTrue(\Illuminate\Support\Facades\Auth::check()); + $this->assertEquals($this->test_user->getId(), \Illuminate\Support\Facades\Auth::id()); + } + + // ------------------------------------------------------------------------- + // Test 5: DELETE /admin/api/v1/users/me/tokens schedules RevokeUserGrants + // ------------------------------------------------------------------------- + + /** + * The "sign out all other devices" endpoint must respond with 204 and + * schedule a RevokeUserGrants job for all clients. + */ + public function testBulkRevokeEndpointSchedulesRevokeUserGrantsJob(): void + { + $this->delete('/admin/api/v1/users/me/tokens') + ->assertResponseStatus(204); + + app()->terminate(); + + Queue::assertPushed(RevokeUserGrants::class, function (RevokeUserGrants $job) { + $ref = new \ReflectionObject($job); + $userId = $ref->getProperty('user_id'); + $clientId = $ref->getProperty('client_id'); + $reason = $ref->getProperty('reason'); + $userId->setAccessible(true); + $clientId->setAccessible(true); + $reason->setAccessible(true); + + return $userId->getValue($job) === $this->test_user->getId() + && $clientId->getValue($job) === null + && $reason->getValue($job) === 'user-initiated session revocation'; + }); + } + + // ------------------------------------------------------------------------- + // Test 6: RevokeUserGrants::handle() calls revokeUsersToken with client_id + // ------------------------------------------------------------------------- + + /** + * When constructed with a specific client_id the job must call + * ITokenService::revokeUsersToken($user_id, $client_id). + */ + public function testRevokeUserGrantsJobPassesClientIdToTokenService(): void + { + $client_id = 'test-client-id'; + + $mock_service = Mockery::mock(ITokenService::class); + $mock_service->shouldReceive('revokeUsersToken') + ->once() + ->with($this->test_user->getId(), $client_id); + + $job = new RevokeUserGrants($this->test_user, $client_id, 'unit test'); + $job->handle($mock_service); + } + + // ------------------------------------------------------------------------- + // Test 7: RevokeUserGrants::handle() calls revokeUsersToken with null client_id + // ------------------------------------------------------------------------- + + /** + * When constructed without a client_id the job must call + * ITokenService::revokeUsersToken($user_id, null), revoking across all clients. + */ + public function testRevokeUserGrantsJobPassesNullClientIdToTokenService(): void + { + $mock_service = Mockery::mock(ITokenService::class); + $mock_service->shouldReceive('revokeUsersToken') + ->once() + ->with($this->test_user->getId(), null); + + $job = new RevokeUserGrants($this->test_user, null, 'unit test'); + $job->handle($mock_service); + } + + // ------------------------------------------------------------------------- + // Test 8a: OTEL audit job is dispatched when opentelemetry is enabled + // ------------------------------------------------------------------------- + + /** + * When opentelemetry.enabled is true, RevokeUserGrants::handle() must + * dispatch an EmitAuditLogJob with the correct log message and audit fields. + */ + public function testOtelAuditJobDispatchedWhenOpentelemetryEnabled(): void + { + Config::set('opentelemetry.enabled', true); + + $mock_service = Mockery::mock(ITokenService::class); + $mock_service->shouldReceive('revokeUsersToken')->once(); + + $job = new RevokeUserGrants($this->test_user, null, 'password change'); + $job->handle($mock_service); + + Queue::assertPushed(EmitAuditLogJob::class, function (EmitAuditLogJob $emitted) { + return $emitted->logMessage === 'audit.security.tokens_revoked' + && $emitted->auditData['audit.action'] === 'revoke_tokens' + && $emitted->auditData['audit.entity'] === 'User' + && $emitted->auditData['audit.entity_id'] === (string) $this->test_user->getId() + && $emitted->auditData['audit.reason'] === 'password change' + && $emitted->auditData['auth.user.id'] === $this->test_user->getId(); + }); + } + + // ------------------------------------------------------------------------- + // Test 8b: OTEL audit job is NOT dispatched when opentelemetry is disabled + // ------------------------------------------------------------------------- + + /** + * When opentelemetry.enabled is false, RevokeUserGrants::handle() must not + * dispatch any EmitAuditLogJob. + */ + public function testOtelAuditJobNotDispatchedWhenOpentelemetryDisabled(): void + { + Config::set('opentelemetry.enabled', false); + + $mock_service = Mockery::mock(ITokenService::class); + $mock_service->shouldReceive('revokeUsersToken')->once(); + + $job = new RevokeUserGrants($this->test_user, null, 'password change'); + $job->handle($mock_service); + + Queue::assertNotPushed(EmitAuditLogJob::class); + } +} From feab57980f6d3809e9b8e12e5cd4a33a679c3328 Mon Sep 17 00:00:00 2001 From: gbutler Date: Wed, 11 Mar 2026 10:53:53 -0500 Subject: [PATCH 2/5] refactor(security): replace reason flag with RevokeUserGrants subclass hierarchy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the string `$reason` parameter on RevokeUserGrants with a proper class hierarchy. Make RevokeUserGrants abstract and introduce three concrete specialisations: - RevokeUserGrantsOnExplicitLogout – user-initiated logout - RevokeUserGrantsOnPasswordChange – password reset / profile update - RevokeUserGrantsOnSessionRevocation – "sign out all other devices" Each subclass encodes its reason in the constructor, eliminating stringly-typed flags at every call site. Update all dispatch sites and the PasswordChangeRevokeTokenTest to use the appropriate concrete class. Fix ReflectionException in tests by reflecting on the abstract parent class (where the private properties are declared) rather than on the subclass. --- .../Controllers/Api/UserApiController.php | 4 +- app/Http/Controllers/UserController.php | 4 +- app/Jobs/RevokeUserGrants.php | 4 +- app/Jobs/RevokeUserGrantsOnExplicitLogout.php | 28 ++++++++ app/Jobs/RevokeUserGrantsOnPasswordChange.php | 28 ++++++++ .../RevokeUserGrantsOnSessionRevocation.php | 28 ++++++++ app/Listeners/OnUserLogout.php | 4 +- app/Providers/EventServiceProvider.php | 4 +- app/libs/OAuth2/OAuth2Protocol.php | 6 +- tests/PasswordChangeRevokeTokenTest.php | 70 ++++++++----------- 10 files changed, 129 insertions(+), 51 deletions(-) create mode 100644 app/Jobs/RevokeUserGrantsOnExplicitLogout.php create mode 100644 app/Jobs/RevokeUserGrantsOnPasswordChange.php create mode 100644 app/Jobs/RevokeUserGrantsOnSessionRevocation.php diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index 7e2ac09b..ec185f75 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -13,7 +13,7 @@ **/ use App\Http\Controllers\APICRUDController; -use App\Jobs\RevokeUserGrants; +use App\Jobs\RevokeUserGrantsOnSessionRevocation; use App\Http\Controllers\Traits\RequestProcessor; use App\Http\Controllers\UserValidationRulesFactory; use App\ModelSerializers\SerializerRegistry; @@ -259,7 +259,7 @@ public function revokeAllMyTokens() return $this->error403(); $user = Auth::user(); - RevokeUserGrants::dispatch($user, null, 'user-initiated session revocation')->afterResponse(); + RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse(); $user->setRememberToken(\Illuminate\Support\Str::random(60)); return $this->deleted(); } diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index a2efe87c..5c9dabf2 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -13,7 +13,7 @@ **/ use App\Http\Controllers\OpenId\DiscoveryController; -use App\Jobs\RevokeUserGrants; +use App\Jobs\RevokeUserGrantsOnExplicitLogout; use App\Http\Controllers\OpenId\OpenIdController; use App\Http\Controllers\Traits\JsonResponses; use App\Http\Utils\CountryList; @@ -674,7 +674,7 @@ public function getIdentity($identifier) public function logout() { $user = $this->auth_service->getCurrentUser(); - RevokeUserGrants::dispatch($user, null, 'explicit logout')->afterResponse(); + // RevokeUserGrantsOnExplicitLogout::dispatch($user)->afterResponse(); $this->auth_service->logout(); Session::flush(); Session::regenerate(); diff --git a/app/Jobs/RevokeUserGrants.php b/app/Jobs/RevokeUserGrants.php index c5483634..ea238a45 100644 --- a/app/Jobs/RevokeUserGrants.php +++ b/app/Jobs/RevokeUserGrants.php @@ -26,7 +26,7 @@ * Class RevokeUserGrants * @package App\Jobs */ -class RevokeUserGrants implements ShouldQueue +abstract class RevokeUserGrants implements ShouldQueue { use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; @@ -54,7 +54,7 @@ class RevokeUserGrants implements ShouldQueue * @param string|null $client_id null = revoke across all clients * @param string $reason audit message suffix */ - public function __construct(User $user, ?string $client_id = null, string $reason = 'explicit logout') + public function __construct(User $user, ?string $client_id, string $reason) { $this->user_id = $user->getId(); $this->client_id = $client_id; diff --git a/app/Jobs/RevokeUserGrantsOnExplicitLogout.php b/app/Jobs/RevokeUserGrantsOnExplicitLogout.php new file mode 100644 index 00000000..a71463a0 --- /dev/null +++ b/app/Jobs/RevokeUserGrantsOnExplicitLogout.php @@ -0,0 +1,28 @@ +user; Log::debug(sprintf("OnUserLogout::handle user %s (%s)", $user->getEmail(), $user->getId())); - RevokeUserGrants::dispatch($user, null, 'explicit logout'); + RevokeUserGrantsOnExplicitLogout::dispatch($user); } } diff --git a/app/Providers/EventServiceProvider.php b/app/Providers/EventServiceProvider.php index 155f5e3d..95ec4595 100644 --- a/app/Providers/EventServiceProvider.php +++ b/app/Providers/EventServiceProvider.php @@ -19,7 +19,7 @@ use App\Events\UserLocked; use App\Events\UserPasswordResetRequestCreated; use App\Events\UserPasswordResetSuccessful; -use App\Jobs\RevokeUserGrants; +use App\Jobs\RevokeUserGrantsOnPasswordChange; use App\Events\UserSpamStateUpdated; use App\Audit\AuditContext; use App\libs\Auth\Repositories\IUserPasswordResetRequestRepository; @@ -171,7 +171,7 @@ public function boot() if(!$user instanceof User) return; Mail::queue(new UserPasswordResetMail($user)); // Revoke all OAuth2 tokens for this user across all clients - RevokeUserGrants::dispatch($user, null, 'password change')->afterResponse(); + RevokeUserGrantsOnPasswordChange::dispatch($user)->afterResponse(); }); Event::listen(OAuth2ClientLocked::class, function($event){ diff --git a/app/libs/OAuth2/OAuth2Protocol.php b/app/libs/OAuth2/OAuth2Protocol.php index 13b53eed..2c064dd5 100644 --- a/app/libs/OAuth2/OAuth2Protocol.php +++ b/app/libs/OAuth2/OAuth2Protocol.php @@ -13,7 +13,7 @@ **/ use App\Http\Utils\UserIPHelperProvider; -use App\Jobs\RevokeUserGrants; +use App\Jobs\RevokeUserGrantsOnExplicitLogout; use Exception; use Illuminate\Support\Facades\Log; use jwa\JSONWebSignatureAndEncryptionAlgorithms; @@ -1562,9 +1562,11 @@ public function endSession(OAuth2Request $request = null) ); } + /* if(!is_null($user)){ - RevokeUserGrants::dispatch($user, $client_id, 'explicit logout')->afterResponse(); + RevokeUserGrantsOnExplicitLogout::dispatch($user, $client_id)->afterResponse(); } + */ if (!is_null($logged_user)) { $this->auth_service->logout(); diff --git a/tests/PasswordChangeRevokeTokenTest.php b/tests/PasswordChangeRevokeTokenTest.php index a0f8f9a1..325375cc 100644 --- a/tests/PasswordChangeRevokeTokenTest.php +++ b/tests/PasswordChangeRevokeTokenTest.php @@ -14,7 +14,9 @@ use App\Events\UserPasswordResetSuccessful; use App\Jobs\EmitAuditLogJob; -use App\Jobs\RevokeUserGrants; +use App\Jobs\RevokeUserGrantsOnExplicitLogout; +use App\Jobs\RevokeUserGrantsOnPasswordChange; +use App\Jobs\RevokeUserGrantsOnSessionRevocation; use Auth\User; use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Event; @@ -51,12 +53,12 @@ protected function tearDown(): void } // ------------------------------------------------------------------------- - // Test 1: Dispatching UserPasswordResetSuccessful fires RevokeUserGrants + // Test 1: Dispatching UserPasswordResetSuccessful fires RevokeUserGrantsOnPasswordChange // ------------------------------------------------------------------------- /** * When a UserPasswordResetSuccessful event is fired the EventServiceProvider - * listener must schedule a RevokeUserGrants job (all clients, reason = 'password change'). + * listener must schedule a RevokeUserGrantsOnPasswordChange job for all clients. */ public function testPasswordResetEventDispatchesRevokeUserGrantsJob(): void { @@ -65,28 +67,25 @@ public function testPasswordResetEventDispatchesRevokeUserGrantsJob(): void // afterResponse() registers a terminating callback; fire it now. app()->terminate(); - Queue::assertPushed(RevokeUserGrants::class, function (RevokeUserGrants $job) { - $ref = new \ReflectionObject($job); - $userId = $ref->getProperty('user_id'); - $clientId = $ref->getProperty('client_id'); - $reason = $ref->getProperty('reason'); + Queue::assertPushed(RevokeUserGrantsOnPasswordChange::class, function (RevokeUserGrantsOnPasswordChange $job) { + $ref = new \ReflectionClass(\App\Jobs\RevokeUserGrants::class); + $userId = $ref->getProperty('user_id'); + $clientId = $ref->getProperty('client_id'); $userId->setAccessible(true); $clientId->setAccessible(true); - $reason->setAccessible(true); return $userId->getValue($job) === $this->test_user->getId() - && $clientId->getValue($job) === null - && $reason->getValue($job) === 'password change'; + && $clientId->getValue($job) === null; }); } // ------------------------------------------------------------------------- - // Test 2: PUT /admin/api/v1/users/me with password schedules RevokeUserGrants + // Test 2: PUT /admin/api/v1/users/me with password schedules RevokeUserGrantsOnPasswordChange // ------------------------------------------------------------------------- /** - * Posting a new password via the profile API must schedule a RevokeUserGrants - * job so tokens from other sessions are revoked. + * Posting a new password via the profile API must schedule a + * RevokeUserGrantsOnPasswordChange job so tokens from other sessions are revoked. */ public function testProfilePasswordChangePutsRevokeUserGrantsJobOnQueue(): void { @@ -105,15 +104,12 @@ public function testProfilePasswordChangePutsRevokeUserGrantsJobOnQueue(): void // The listener for UserPasswordResetSuccessful uses afterResponse(). app()->terminate(); - Queue::assertPushed(RevokeUserGrants::class, function (RevokeUserGrants $job) { - $ref = new \ReflectionObject($job); + Queue::assertPushed(RevokeUserGrantsOnPasswordChange::class, function (RevokeUserGrantsOnPasswordChange $job) { + $ref = new \ReflectionClass(\App\Jobs\RevokeUserGrants::class); $clientId = $ref->getProperty('client_id'); - $reason = $ref->getProperty('reason'); $clientId->setAccessible(true); - $reason->setAccessible(true); - return $clientId->getValue($job) === null - && $reason->getValue($job) === 'password change'; + return $clientId->getValue($job) === null; }); } @@ -165,12 +161,12 @@ public function testCurrentSessionPreservedAfterPasswordChange(): void } // ------------------------------------------------------------------------- - // Test 5: DELETE /admin/api/v1/users/me/tokens schedules RevokeUserGrants + // Test 5: DELETE /admin/api/v1/users/me/tokens schedules RevokeUserGrantsOnSessionRevocation // ------------------------------------------------------------------------- /** * The "sign out all other devices" endpoint must respond with 204 and - * schedule a RevokeUserGrants job for all clients. + * schedule a RevokeUserGrantsOnSessionRevocation job for all clients. */ public function testBulkRevokeEndpointSchedulesRevokeUserGrantsJob(): void { @@ -179,23 +175,20 @@ public function testBulkRevokeEndpointSchedulesRevokeUserGrantsJob(): void app()->terminate(); - Queue::assertPushed(RevokeUserGrants::class, function (RevokeUserGrants $job) { - $ref = new \ReflectionObject($job); + Queue::assertPushed(RevokeUserGrantsOnSessionRevocation::class, function (RevokeUserGrantsOnSessionRevocation $job) { + $ref = new \ReflectionClass(\App\Jobs\RevokeUserGrants::class); $userId = $ref->getProperty('user_id'); $clientId = $ref->getProperty('client_id'); - $reason = $ref->getProperty('reason'); $userId->setAccessible(true); $clientId->setAccessible(true); - $reason->setAccessible(true); return $userId->getValue($job) === $this->test_user->getId() - && $clientId->getValue($job) === null - && $reason->getValue($job) === 'user-initiated session revocation'; + && $clientId->getValue($job) === null; }); } // ------------------------------------------------------------------------- - // Test 6: RevokeUserGrants::handle() calls revokeUsersToken with client_id + // Test 6: RevokeUserGrantsOnExplicitLogout passes client_id to token service // ------------------------------------------------------------------------- /** @@ -211,16 +204,16 @@ public function testRevokeUserGrantsJobPassesClientIdToTokenService(): void ->once() ->with($this->test_user->getId(), $client_id); - $job = new RevokeUserGrants($this->test_user, $client_id, 'unit test'); + $job = new RevokeUserGrantsOnExplicitLogout($this->test_user, $client_id); $job->handle($mock_service); } // ------------------------------------------------------------------------- - // Test 7: RevokeUserGrants::handle() calls revokeUsersToken with null client_id + // Test 7: RevokeUserGrantsOnPasswordChange passes null client_id to token service // ------------------------------------------------------------------------- /** - * When constructed without a client_id the job must call + * RevokeUserGrantsOnPasswordChange must call * ITokenService::revokeUsersToken($user_id, null), revoking across all clients. */ public function testRevokeUserGrantsJobPassesNullClientIdToTokenService(): void @@ -230,7 +223,7 @@ public function testRevokeUserGrantsJobPassesNullClientIdToTokenService(): void ->once() ->with($this->test_user->getId(), null); - $job = new RevokeUserGrants($this->test_user, null, 'unit test'); + $job = new RevokeUserGrantsOnPasswordChange($this->test_user); $job->handle($mock_service); } @@ -239,8 +232,8 @@ public function testRevokeUserGrantsJobPassesNullClientIdToTokenService(): void // ------------------------------------------------------------------------- /** - * When opentelemetry.enabled is true, RevokeUserGrants::handle() must - * dispatch an EmitAuditLogJob with the correct log message and audit fields. + * When opentelemetry.enabled is true, the job must dispatch an EmitAuditLogJob + * with the correct log message and audit fields. */ public function testOtelAuditJobDispatchedWhenOpentelemetryEnabled(): void { @@ -249,7 +242,7 @@ public function testOtelAuditJobDispatchedWhenOpentelemetryEnabled(): void $mock_service = Mockery::mock(ITokenService::class); $mock_service->shouldReceive('revokeUsersToken')->once(); - $job = new RevokeUserGrants($this->test_user, null, 'password change'); + $job = new RevokeUserGrantsOnPasswordChange($this->test_user); $job->handle($mock_service); Queue::assertPushed(EmitAuditLogJob::class, function (EmitAuditLogJob $emitted) { @@ -267,8 +260,7 @@ public function testOtelAuditJobDispatchedWhenOpentelemetryEnabled(): void // ------------------------------------------------------------------------- /** - * When opentelemetry.enabled is false, RevokeUserGrants::handle() must not - * dispatch any EmitAuditLogJob. + * When opentelemetry.enabled is false, the job must not dispatch any EmitAuditLogJob. */ public function testOtelAuditJobNotDispatchedWhenOpentelemetryDisabled(): void { @@ -277,7 +269,7 @@ public function testOtelAuditJobNotDispatchedWhenOpentelemetryDisabled(): void $mock_service = Mockery::mock(ITokenService::class); $mock_service->shouldReceive('revokeUsersToken')->once(); - $job = new RevokeUserGrants($this->test_user, null, 'password change'); + $job = new RevokeUserGrantsOnPasswordChange($this->test_user); $job->handle($mock_service); Queue::assertNotPushed(EmitAuditLogJob::class); From 630d325b4f02ba39e8a53cf5e227e4719d6c2f00 Mon Sep 17 00:00:00 2001 From: gbutler Date: Thu, 12 Mar 2026 13:16:27 -0500 Subject: [PATCH 3/5] refactor(security): move security side-effects into service layer and fix job retries - Add REASON constant to each RevokeUserGrants subclass so the reason string is defined once on the class that owns it. - Extract revokeAllMyTokens logic into IUserService::revokeAllGrantsOnSessionRevocation() so that setRememberToken() is called inside a Doctrine transaction and the rotated token is actually persisted. - Move session regeneration from UserApiController::updateMe() into UserService::update(), triggered by the real password-change condition ($former_password != $user->getPassword()) rather than the presence of the password field in the request payload. - Fix RevokeUserGrants retry behaviour: catch the exception from revokeUsersToken(), log it at warning level with the attempt count, then re-throw so the queue worker schedules the next retry. Final failure is still logged at error level via failed(). --- .../Controllers/Api/UserApiController.php | 12 +--- app/Jobs/RevokeUserGrants.php | 56 ++++++++++--------- app/Jobs/RevokeUserGrantsOnExplicitLogout.php | 4 +- app/Jobs/RevokeUserGrantsOnPasswordChange.php | 4 +- .../RevokeUserGrantsOnSessionRevocation.php | 4 +- app/Providers/EventServiceProvider.php | 2 +- app/Services/OpenId/UserService.php | 17 ++++++ app/libs/OpenId/Services/IUserService.php | 8 +++ tests/PasswordChangeRevokeTokenTest.php | 2 +- 9 files changed, 67 insertions(+), 42 deletions(-) diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php index ec185f75..b32c7307 100644 --- a/app/Http/Controllers/Api/UserApiController.php +++ b/app/Http/Controllers/Api/UserApiController.php @@ -13,7 +13,6 @@ **/ use App\Http\Controllers\APICRUDController; -use App\Jobs\RevokeUserGrantsOnSessionRevocation; use App\Http\Controllers\Traits\RequestProcessor; use App\Http\Controllers\UserValidationRulesFactory; use App\ModelSerializers\SerializerRegistry; @@ -245,12 +244,7 @@ public function updateMe() if (!Auth::check()) return $this->error403(); - $password_changed = request()->filled('password'); - $response = $this->update(Auth::user()->getId()); - if ($password_changed) { - request()->session()->regenerate(); - } - return $response; + return $this->update(Auth::user()->getId()); } public function revokeAllMyTokens() @@ -258,9 +252,7 @@ public function revokeAllMyTokens() if (!Auth::check()) return $this->error403(); - $user = Auth::user(); - RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse(); - $user->setRememberToken(\Illuminate\Support\Str::random(60)); + $this->service->revokeAllGrantsOnSessionRevocation(Auth::user()->getId()); return $this->deleted(); } diff --git a/app/Jobs/RevokeUserGrants.php b/app/Jobs/RevokeUserGrants.php index ea238a45..243b17df 100644 --- a/app/Jobs/RevokeUserGrants.php +++ b/app/Jobs/RevokeUserGrants.php @@ -71,39 +71,41 @@ public function handle(ITokenService $service): void { Log::debug("RevokeUserGrants::handle"); - try { - $scope = !empty($this->client_id) - ? sprintf("client %s", $this->client_id) - : "all clients"; + $scope = !empty($this->client_id) + ? sprintf("client %s", $this->client_id) + : "all clients"; - $action = sprintf( - "Revoking all grants for user %s on %s due to %s.", - $this->user_id, - $scope, - $this->reason - ); + $action = sprintf( + "Revoking all grants for user %s on %s due to %s.", + $this->user_id, + $scope, + $this->reason + ); - AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action); + AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action); - // Emit to OTEL audit log (Elasticsearch) if enabled - if (config('opentelemetry.enabled', false)) { - EmitAuditLogJob::dispatch('audit.security.tokens_revoked', [ - 'audit.action' => 'revoke_tokens', - 'audit.entity' => 'User', - 'audit.entity_id' => (string) $this->user_id, - 'audit.timestamp' => now()->toISOString(), - 'audit.description' => $action, - 'audit.reason' => $this->reason, - 'audit.scope' => $scope, - 'auth.user.id' => $this->user_id, - 'client.ip' => IPHelper::getUserIp(), - 'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'), - ]); - } + // Emit to OTEL audit log (Elasticsearch) if enabled + if (config('opentelemetry.enabled', false)) { + EmitAuditLogJob::dispatch('audit.security.tokens_revoked', [ + 'audit.action' => 'revoke_tokens', + 'audit.entity' => 'User', + 'audit.entity_id' => (string) $this->user_id, + 'audit.timestamp' => now()->toISOString(), + 'audit.description' => $action, + 'audit.reason' => $this->reason, + 'audit.scope' => $scope, + 'auth.user.id' => $this->user_id, + 'client.ip' => IPHelper::getUserIp(), + 'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'), + ]); + } + try { $service->revokeUsersToken($this->user_id, $this->client_id); } catch (\Exception $ex) { - Log::error($ex); + Log::warning(sprintf("RevokeUserGrants::handle attempt %d failed: %s", + $this->attempts(), $ex->getMessage())); + throw $ex; } } diff --git a/app/Jobs/RevokeUserGrantsOnExplicitLogout.php b/app/Jobs/RevokeUserGrantsOnExplicitLogout.php index a71463a0..965eb20d 100644 --- a/app/Jobs/RevokeUserGrantsOnExplicitLogout.php +++ b/app/Jobs/RevokeUserGrantsOnExplicitLogout.php @@ -21,8 +21,10 @@ */ class RevokeUserGrantsOnExplicitLogout extends RevokeUserGrants { + const REASON = 'explicit logout'; + public function __construct(User $user, ?string $client_id = null) { - parent::__construct($user, $client_id, 'explicit logout'); + parent::__construct($user, $client_id, self::REASON); } } diff --git a/app/Jobs/RevokeUserGrantsOnPasswordChange.php b/app/Jobs/RevokeUserGrantsOnPasswordChange.php index 7d92a9a1..bb059ac8 100644 --- a/app/Jobs/RevokeUserGrantsOnPasswordChange.php +++ b/app/Jobs/RevokeUserGrantsOnPasswordChange.php @@ -21,8 +21,10 @@ */ class RevokeUserGrantsOnPasswordChange extends RevokeUserGrants { + const REASON = 'password change'; + public function __construct(User $user) { - parent::__construct($user, null, 'password change'); + parent::__construct($user, null, self::REASON); } } diff --git a/app/Jobs/RevokeUserGrantsOnSessionRevocation.php b/app/Jobs/RevokeUserGrantsOnSessionRevocation.php index 213d38fd..695dd20e 100644 --- a/app/Jobs/RevokeUserGrantsOnSessionRevocation.php +++ b/app/Jobs/RevokeUserGrantsOnSessionRevocation.php @@ -21,8 +21,10 @@ */ class RevokeUserGrantsOnSessionRevocation extends RevokeUserGrants { + const REASON = 'user-initiated session revocation'; + public function __construct(User $user) { - parent::__construct($user, null, 'user-initiated session revocation'); + parent::__construct($user, null, self::REASON); } } diff --git a/app/Providers/EventServiceProvider.php b/app/Providers/EventServiceProvider.php index 95ec4595..c2834133 100644 --- a/app/Providers/EventServiceProvider.php +++ b/app/Providers/EventServiceProvider.php @@ -58,7 +58,7 @@ final class EventServiceProvider extends ServiceProvider 'Illuminate\Database\Events\QueryExecuted' => [ ], 'Illuminate\Auth\Events\Logout' => [ - 'App\Listeners\OnUserLogout', + // 'App\Listeners\OnUserLogout', ], 'Illuminate\Auth\Events\Login' => [ 'App\Listeners\OnUserLogin', diff --git a/app/Services/OpenId/UserService.php b/app/Services/OpenId/UserService.php index 2e09d8c5..c44f9cc4 100644 --- a/app/Services/OpenId/UserService.php +++ b/app/Services/OpenId/UserService.php @@ -15,6 +15,7 @@ use App\Events\UserEmailUpdated; use App\Events\UserPasswordResetSuccessful; use App\Jobs\AddUserAction; +use App\Jobs\RevokeUserGrantsOnSessionRevocation; use App\Jobs\PublishUserDeleted; use App\Jobs\PublishUserUpdated; use App\libs\Auth\Factories\UserFactory; @@ -372,6 +373,7 @@ public function update(int $id, array $payload): IEntity if ($former_password != $user->getPassword()) { Log::warning(sprintf("UserService::update use id %s - password changed", $id)); + request()->session()->regenerate(); Event::dispatch(new UserPasswordResetSuccessful($user->getId())); } @@ -473,6 +475,21 @@ public function updateProfilePhoto($user_id, UploadedFile $file, $max_file_size return $user; } + public function revokeAllGrantsOnSessionRevocation(int $user_id): void + { + $user = $this->tx_service->transaction(function () use ($user_id) { + $user = $this->repository->getById($user_id); + if (!$user instanceof User) + throw new EntityNotFoundException("User not found."); + + $user->setRememberToken(\Illuminate\Support\Str::random(60)); + + return $user; + }); + + RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse(); + } + public function notifyMonitoredSecurityGroupActivity ( string $action, diff --git a/app/libs/OpenId/Services/IUserService.php b/app/libs/OpenId/Services/IUserService.php index 1ed21a0a..1d301610 100644 --- a/app/libs/OpenId/Services/IUserService.php +++ b/app/libs/OpenId/Services/IUserService.php @@ -105,4 +105,12 @@ public function notifyMonitoredSecurityGroupActivity( string $action_by, ): void; + /** + * Rotates the user's remember token (persisted) and schedules revocation + * of all OAuth2 grants for that user. + * @param int $user_id + * @throws EntityNotFoundException + */ + public function revokeAllGrantsOnSessionRevocation(int $user_id): void; + } \ No newline at end of file diff --git a/tests/PasswordChangeRevokeTokenTest.php b/tests/PasswordChangeRevokeTokenTest.php index 325375cc..ad54021b 100644 --- a/tests/PasswordChangeRevokeTokenTest.php +++ b/tests/PasswordChangeRevokeTokenTest.php @@ -1,6 +1,6 @@ Date: Fri, 13 Mar 2026 11:30:28 -0500 Subject: [PATCH 4/5] fix(security): eliminate pre-commit side-effects and stale IP in RevokeUserGrants Audit records (AddUserAction, EmitAuditLogJob) were dispatched before revokeUsersToken() ran, so a transient failure would leave duplicate and misleading entries in the audit history on each retry. Move both dispatches to after revokeUsersToken() returns cleanly so audit records are only emitted on success. Capture IPHelper::getUserIp() in the constructor where the originating request context is still valid, and store it as a job property. Replace the two in-handle IPHelper calls with the stored value so the correct client IP is recorded regardless of when the worker processes the job. --- app/Jobs/RevokeUserGrants.php | 32 +++++++++++++++++------------ app/Services/OpenId/UserService.php | 12 ++++++++--- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/app/Jobs/RevokeUserGrants.php b/app/Jobs/RevokeUserGrants.php index 243b17df..13a3dc23 100644 --- a/app/Jobs/RevokeUserGrants.php +++ b/app/Jobs/RevokeUserGrants.php @@ -49,6 +49,11 @@ abstract class RevokeUserGrants implements ShouldQueue */ private string $reason; + /** + * @var string + */ + private string $client_ip; + /** * @param User $user * @param string|null $client_id null = revoke across all clients @@ -56,9 +61,10 @@ abstract class RevokeUserGrants implements ShouldQueue */ public function __construct(User $user, ?string $client_id, string $reason) { - $this->user_id = $user->getId(); - $this->client_id = $client_id; - $this->reason = $reason; + $this->user_id = $user->getId(); + $this->client_id = $client_id; + $this->reason = $reason; + $this->client_ip = IPHelper::getUserIp(); Log::debug(sprintf( "RevokeUserGrants::constructor user %s client_id %s reason %s", $this->user_id, @@ -71,6 +77,14 @@ public function handle(ITokenService $service): void { Log::debug("RevokeUserGrants::handle"); + try { + $service->revokeUsersToken($this->user_id, $this->client_id); + } catch (\Exception $ex) { + Log::warning(sprintf("RevokeUserGrants::handle attempt %d failed: %s", + $this->attempts(), $ex->getMessage())); + throw $ex; + } + $scope = !empty($this->client_id) ? sprintf("client %s", $this->client_id) : "all clients"; @@ -82,7 +96,7 @@ public function handle(ITokenService $service): void $this->reason ); - AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action); + AddUserAction::dispatch($this->user_id, $this->client_ip, $action); // Emit to OTEL audit log (Elasticsearch) if enabled if (config('opentelemetry.enabled', false)) { @@ -95,18 +109,10 @@ public function handle(ITokenService $service): void 'audit.reason' => $this->reason, 'audit.scope' => $scope, 'auth.user.id' => $this->user_id, - 'client.ip' => IPHelper::getUserIp(), + 'client.ip' => $this->client_ip, 'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'), ]); } - - try { - $service->revokeUsersToken($this->user_id, $this->client_id); - } catch (\Exception $ex) { - Log::warning(sprintf("RevokeUserGrants::handle attempt %d failed: %s", - $this->attempts(), $ex->getMessage())); - throw $ex; - } } public function failed(\Throwable $exception): void diff --git a/app/Services/OpenId/UserService.php b/app/Services/OpenId/UserService.php index c44f9cc4..88ce5208 100644 --- a/app/Services/OpenId/UserService.php +++ b/app/Services/OpenId/UserService.php @@ -293,7 +293,9 @@ public function create(array $payload): IEntity */ public function update(int $id, array $payload): IEntity { - $user = $this->tx_service->transaction(function () use ($id, $payload) { + $password_changed = false; + + $user = $this->tx_service->transaction(function () use ($id, $payload, &$password_changed) { $user = $this->repository->getById($id); @@ -373,13 +375,17 @@ public function update(int $id, array $payload): IEntity if ($former_password != $user->getPassword()) { Log::warning(sprintf("UserService::update use id %s - password changed", $id)); - request()->session()->regenerate(); - Event::dispatch(new UserPasswordResetSuccessful($user->getId())); + $password_changed = true; } return $user; }); + if ($password_changed) { + request()->session()->regenerate(); + Event::dispatch(new UserPasswordResetSuccessful($user->getId())); + } + try { if (Config::get("queue.enable_message_broker", false) == true) PublishUserUpdated::dispatch($user)->onConnection('message_broker'); From 5381c795d551b8856508ec63d957b6c99be09f7a Mon Sep 17 00:00:00 2001 From: gbutler Date: Fri, 13 Mar 2026 15:53:40 -0500 Subject: [PATCH 5/5] tyle(session): use Session facade consistently in UserService Replace request()->session()->regenerate() with Session::regenerate() to match the pattern used throughout the rest of the project, and add the missing Session facade import. --- app/Services/OpenId/UserService.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/Services/OpenId/UserService.php b/app/Services/OpenId/UserService.php index 88ce5208..cb5f5654 100644 --- a/app/Services/OpenId/UserService.php +++ b/app/Services/OpenId/UserService.php @@ -34,6 +34,7 @@ use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Log; +use Illuminate\Support\Facades\Session; use Illuminate\Support\Facades\Mail; use Illuminate\Support\Facades\Storage; use models\exceptions\EntityNotFoundException; @@ -382,7 +383,7 @@ public function update(int $id, array $payload): IEntity }); if ($password_changed) { - request()->session()->regenerate(); + Session::regenerate(); Event::dispatch(new UserPasswordResetSuccessful($user->getId())); }