Skip to content

Commit 630d325

Browse files
committed
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().
1 parent 1afebad commit 630d325

9 files changed

Lines changed: 67 additions & 42 deletions

app/Http/Controllers/Api/UserApiController.php

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
**/
1414

1515
use App\Http\Controllers\APICRUDController;
16-
use App\Jobs\RevokeUserGrantsOnSessionRevocation;
1716
use App\Http\Controllers\Traits\RequestProcessor;
1817
use App\Http\Controllers\UserValidationRulesFactory;
1918
use App\ModelSerializers\SerializerRegistry;
@@ -245,22 +244,15 @@ public function updateMe()
245244
if (!Auth::check())
246245
return $this->error403();
247246

248-
$password_changed = request()->filled('password');
249-
$response = $this->update(Auth::user()->getId());
250-
if ($password_changed) {
251-
request()->session()->regenerate();
252-
}
253-
return $response;
247+
return $this->update(Auth::user()->getId());
254248
}
255249

256250
public function revokeAllMyTokens()
257251
{
258252
if (!Auth::check())
259253
return $this->error403();
260254

261-
$user = Auth::user();
262-
RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse();
263-
$user->setRememberToken(\Illuminate\Support\Str::random(60));
255+
$this->service->revokeAllGrantsOnSessionRevocation(Auth::user()->getId());
264256
return $this->deleted();
265257
}
266258

app/Jobs/RevokeUserGrants.php

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -71,39 +71,41 @@ public function handle(ITokenService $service): void
7171
{
7272
Log::debug("RevokeUserGrants::handle");
7373

74-
try {
75-
$scope = !empty($this->client_id)
76-
? sprintf("client %s", $this->client_id)
77-
: "all clients";
74+
$scope = !empty($this->client_id)
75+
? sprintf("client %s", $this->client_id)
76+
: "all clients";
7877

79-
$action = sprintf(
80-
"Revoking all grants for user %s on %s due to %s.",
81-
$this->user_id,
82-
$scope,
83-
$this->reason
84-
);
78+
$action = sprintf(
79+
"Revoking all grants for user %s on %s due to %s.",
80+
$this->user_id,
81+
$scope,
82+
$this->reason
83+
);
8584

86-
AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action);
85+
AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action);
8786

88-
// Emit to OTEL audit log (Elasticsearch) if enabled
89-
if (config('opentelemetry.enabled', false)) {
90-
EmitAuditLogJob::dispatch('audit.security.tokens_revoked', [
91-
'audit.action' => 'revoke_tokens',
92-
'audit.entity' => 'User',
93-
'audit.entity_id' => (string) $this->user_id,
94-
'audit.timestamp' => now()->toISOString(),
95-
'audit.description' => $action,
96-
'audit.reason' => $this->reason,
97-
'audit.scope' => $scope,
98-
'auth.user.id' => $this->user_id,
99-
'client.ip' => IPHelper::getUserIp(),
100-
'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'),
101-
]);
102-
}
87+
// Emit to OTEL audit log (Elasticsearch) if enabled
88+
if (config('opentelemetry.enabled', false)) {
89+
EmitAuditLogJob::dispatch('audit.security.tokens_revoked', [
90+
'audit.action' => 'revoke_tokens',
91+
'audit.entity' => 'User',
92+
'audit.entity_id' => (string) $this->user_id,
93+
'audit.timestamp' => now()->toISOString(),
94+
'audit.description' => $action,
95+
'audit.reason' => $this->reason,
96+
'audit.scope' => $scope,
97+
'auth.user.id' => $this->user_id,
98+
'client.ip' => IPHelper::getUserIp(),
99+
'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'),
100+
]);
101+
}
103102

103+
try {
104104
$service->revokeUsersToken($this->user_id, $this->client_id);
105105
} catch (\Exception $ex) {
106-
Log::error($ex);
106+
Log::warning(sprintf("RevokeUserGrants::handle attempt %d failed: %s",
107+
$this->attempts(), $ex->getMessage()));
108+
throw $ex;
107109
}
108110
}
109111

app/Jobs/RevokeUserGrantsOnExplicitLogout.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@
2121
*/
2222
class RevokeUserGrantsOnExplicitLogout extends RevokeUserGrants
2323
{
24+
const REASON = 'explicit logout';
25+
2426
public function __construct(User $user, ?string $client_id = null)
2527
{
26-
parent::__construct($user, $client_id, 'explicit logout');
28+
parent::__construct($user, $client_id, self::REASON);
2729
}
2830
}

app/Jobs/RevokeUserGrantsOnPasswordChange.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@
2121
*/
2222
class RevokeUserGrantsOnPasswordChange extends RevokeUserGrants
2323
{
24+
const REASON = 'password change';
25+
2426
public function __construct(User $user)
2527
{
26-
parent::__construct($user, null, 'password change');
28+
parent::__construct($user, null, self::REASON);
2729
}
2830
}

app/Jobs/RevokeUserGrantsOnSessionRevocation.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@
2121
*/
2222
class RevokeUserGrantsOnSessionRevocation extends RevokeUserGrants
2323
{
24+
const REASON = 'user-initiated session revocation';
25+
2426
public function __construct(User $user)
2527
{
26-
parent::__construct($user, null, 'user-initiated session revocation');
28+
parent::__construct($user, null, self::REASON);
2729
}
2830
}

app/Providers/EventServiceProvider.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ final class EventServiceProvider extends ServiceProvider
5858
'Illuminate\Database\Events\QueryExecuted' => [
5959
],
6060
'Illuminate\Auth\Events\Logout' => [
61-
'App\Listeners\OnUserLogout',
61+
// 'App\Listeners\OnUserLogout',
6262
],
6363
'Illuminate\Auth\Events\Login' => [
6464
'App\Listeners\OnUserLogin',

app/Services/OpenId/UserService.php

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use App\Events\UserEmailUpdated;
1616
use App\Events\UserPasswordResetSuccessful;
1717
use App\Jobs\AddUserAction;
18+
use App\Jobs\RevokeUserGrantsOnSessionRevocation;
1819
use App\Jobs\PublishUserDeleted;
1920
use App\Jobs\PublishUserUpdated;
2021
use App\libs\Auth\Factories\UserFactory;
@@ -372,6 +373,7 @@ public function update(int $id, array $payload): IEntity
372373

373374
if ($former_password != $user->getPassword()) {
374375
Log::warning(sprintf("UserService::update use id %s - password changed", $id));
376+
request()->session()->regenerate();
375377
Event::dispatch(new UserPasswordResetSuccessful($user->getId()));
376378
}
377379

@@ -473,6 +475,21 @@ public function updateProfilePhoto($user_id, UploadedFile $file, $max_file_size
473475
return $user;
474476
}
475477

478+
public function revokeAllGrantsOnSessionRevocation(int $user_id): void
479+
{
480+
$user = $this->tx_service->transaction(function () use ($user_id) {
481+
$user = $this->repository->getById($user_id);
482+
if (!$user instanceof User)
483+
throw new EntityNotFoundException("User not found.");
484+
485+
$user->setRememberToken(\Illuminate\Support\Str::random(60));
486+
487+
return $user;
488+
});
489+
490+
RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse();
491+
}
492+
476493
public function notifyMonitoredSecurityGroupActivity
477494
(
478495
string $action,

app/libs/OpenId/Services/IUserService.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,4 +105,12 @@ public function notifyMonitoredSecurityGroupActivity(
105105
string $action_by,
106106
): void;
107107

108+
/**
109+
* Rotates the user's remember token (persisted) and schedules revocation
110+
* of all OAuth2 grants for that user.
111+
* @param int $user_id
112+
* @throws EntityNotFoundException
113+
*/
114+
public function revokeAllGrantsOnSessionRevocation(int $user_id): void;
115+
108116
}

tests/PasswordChangeRevokeTokenTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
<?php namespace Tests;
22
/**
3-
* Copyright 2024 OpenStack Foundation
3+
* Copyright 2026 OpenStack Foundation
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
66
* You may obtain a copy of the License at

0 commit comments

Comments
 (0)