Skip to content

Commit d40da20

Browse files
mulldugmulldug
authored
fix(security): revoke tokens and invalidate sessions on password change (#117)
* 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. * refactor(security): replace reason flag with RevokeUserGrants subclass hierarchy 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. * 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(). * 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. * 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.
1 parent 446696c commit d40da20

File tree

15 files changed

+557
-65
lines changed

15 files changed

+557
-65
lines changed

‎app/Http/Controllers/Api/UserApiController.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,15 @@ public function updateMe()
247247
return $this->update(Auth::user()->getId());
248248
}
249249

250+
public function revokeAllMyTokens()
251+
{
252+
if (!Auth::check())
253+
return $this->error403();
254+
255+
$this->service->revokeAllGrantsOnSessionRevocation(Auth::user()->getId());
256+
return $this->deleted();
257+
}
258+
250259
public function updateMyPic(){
251260
if (!Auth::check())
252261
return $this->error403();

‎app/Http/Controllers/UserController.php‎

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

1515
use App\Http\Controllers\OpenId\DiscoveryController;
16+
use App\Jobs\RevokeUserGrantsOnExplicitLogout;
1617
use App\Http\Controllers\OpenId\OpenIdController;
1718
use App\Http\Controllers\Traits\JsonResponses;
1819
use App\Http\Utils\CountryList;
@@ -673,7 +674,7 @@ public function getIdentity($identifier)
673674
public function logout()
674675
{
675676
$user = $this->auth_service->getCurrentUser();
676-
//RevokeUserGrantsOnExplicitLogout::dispatch($user)->afterResponse();
677+
// RevokeUserGrantsOnExplicitLogout::dispatch($user)->afterResponse();
677678
$this->auth_service->logout();
678679
Session::flush();
679680
Session::regenerate();

‎app/Jobs/RevokeUserGrants.php‎

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
<?php namespace App\Jobs;
2+
/*
3+
* Copyright 2024 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 Auth\User;
16+
use Illuminate\Bus\Queueable;
17+
use Illuminate\Contracts\Queue\ShouldQueue;
18+
use Illuminate\Foundation\Bus\Dispatchable;
19+
use Illuminate\Queue\InteractsWithQueue;
20+
use Illuminate\Queue\SerializesModels;
21+
use Illuminate\Support\Facades\Log;
22+
use OAuth2\Services\ITokenService;
23+
use Utils\IPHelper;
24+
25+
/**
26+
* Class RevokeUserGrants
27+
* @package App\Jobs
28+
*/
29+
abstract class RevokeUserGrants implements ShouldQueue
30+
{
31+
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
32+
33+
public $tries = 5;
34+
35+
public $timeout = 0;
36+
37+
/**
38+
* @var int
39+
*/
40+
private int $user_id;
41+
42+
/**
43+
* @var string|null
44+
*/
45+
private ?string $client_id;
46+
47+
/**
48+
* @var string
49+
*/
50+
private string $reason;
51+
52+
/**
53+
* @var string
54+
*/
55+
private string $client_ip;
56+
57+
/**
58+
* @param User $user
59+
* @param string|null $client_id null = revoke across all clients
60+
* @param string $reason audit message suffix
61+
*/
62+
public function __construct(User $user, ?string $client_id, string $reason)
63+
{
64+
$this->user_id = $user->getId();
65+
$this->client_id = $client_id;
66+
$this->reason = $reason;
67+
$this->client_ip = IPHelper::getUserIp();
68+
Log::debug(sprintf(
69+
"RevokeUserGrants::constructor user %s client_id %s reason %s",
70+
$this->user_id,
71+
$client_id ?? 'N/A',
72+
$reason
73+
));
74+
}
75+
76+
public function handle(ITokenService $service): void
77+
{
78+
Log::debug("RevokeUserGrants::handle");
79+
80+
try {
81+
$service->revokeUsersToken($this->user_id, $this->client_id);
82+
} catch (\Exception $ex) {
83+
Log::warning(sprintf("RevokeUserGrants::handle attempt %d failed: %s",
84+
$this->attempts(), $ex->getMessage()));
85+
throw $ex;
86+
}
87+
88+
$scope = !empty($this->client_id)
89+
? sprintf("client %s", $this->client_id)
90+
: "all clients";
91+
92+
$action = sprintf(
93+
"Revoking all grants for user %s on %s due to %s.",
94+
$this->user_id,
95+
$scope,
96+
$this->reason
97+
);
98+
99+
AddUserAction::dispatch($this->user_id, $this->client_ip, $action);
100+
101+
// Emit to OTEL audit log (Elasticsearch) if enabled
102+
if (config('opentelemetry.enabled', false)) {
103+
EmitAuditLogJob::dispatch('audit.security.tokens_revoked', [
104+
'audit.action' => 'revoke_tokens',
105+
'audit.entity' => 'User',
106+
'audit.entity_id' => (string) $this->user_id,
107+
'audit.timestamp' => now()->toISOString(),
108+
'audit.description' => $action,
109+
'audit.reason' => $this->reason,
110+
'audit.scope' => $scope,
111+
'auth.user.id' => $this->user_id,
112+
'client.ip' => $this->client_ip,
113+
'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'),
114+
]);
115+
}
116+
}
117+
118+
public function failed(\Throwable $exception): void
119+
{
120+
Log::error(sprintf("RevokeUserGrants::failed %s", $exception->getMessage()));
121+
}
122+
}

‎app/Jobs/RevokeUserGrantsOnExplicitLogout.php‎

Lines changed: 7 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -13,71 +13,18 @@
1313
**/
1414

1515
use Auth\User;
16-
use Illuminate\Bus\Queueable;
17-
use Illuminate\Contracts\Queue\ShouldQueue;
18-
use Illuminate\Foundation\Bus\Dispatchable;
19-
use Illuminate\Queue\InteractsWithQueue;
20-
use Illuminate\Queue\SerializesModels;
21-
use Illuminate\Support\Facades\Log;
22-
use OAuth2\Services\ITokenService;
23-
use Utils\IPHelper;
2416

2517
/**
26-
* Class RevokeUserGrants
18+
* Class RevokeUserGrantsOnExplicitLogout
19+
* Revokes all OAuth2 grants for a user when they explicitly log out.
2720
* @package App\Jobs
2821
*/
29-
class RevokeUserGrantsOnExplicitLogout implements ShouldQueue
22+
class RevokeUserGrantsOnExplicitLogout extends RevokeUserGrants
3023
{
31-
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
24+
const REASON = 'explicit logout';
3225

33-
public $tries = 5;
34-
35-
public $timeout = 0;
36-
37-
/**
38-
* @var int
39-
*/
40-
private $user_id;
41-
42-
/**
43-
* @var string
44-
*/
45-
private $client_id;
46-
47-
48-
/**
49-
* @param User $user
50-
* @param string|null $client_id
51-
*/
52-
public function __construct(User $user, ?string $client_id = null){
53-
$this->user_id = $user->getId();
54-
$this->client_id = $client_id;
55-
Log::debug(sprintf("RevokeUserGrants::constructor user %s client id %s", $this->user_id, !empty($client_id)? $client_id :"N/A"));
56-
}
57-
58-
public function handle(ITokenService $service){
59-
Log::debug(sprintf("RevokeUserGrants::handle"));
60-
61-
if(empty($this->client_id)) {
62-
return;
63-
}
64-
try{
65-
$action = sprintf
66-
(
67-
"Revoking all grants for user %s on %s due explicit Log out.",
68-
$this->user_id, sprintf("Client %s", $this->client_id)
69-
);
70-
71-
AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action);
72-
$service->revokeUsersToken($this->user_id, $this->client_id);
73-
}
74-
catch (\Exception $ex) {
75-
Log::error($ex);
76-
}
77-
}
78-
79-
public function failed(\Throwable $exception)
26+
public function __construct(User $user, ?string $client_id = null)
8027
{
81-
Log::error(sprintf( "RevokeUserGrants::failed %s", $exception->getMessage()));
28+
parent::__construct($user, $client_id, self::REASON);
8229
}
83-
}
30+
}

‎app/Jobs/RevokeUserGrantsOnPasswordChange.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?php namespace App\Jobs;
2+
/*
3+
* Copyright 2024 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 Auth\User;
16+
17+
/**
18+
* Class RevokeUserGrantsOnPasswordChange
19+
* Revokes all OAuth2 grants for a user across all clients after a password change.
20+
* @package App\Jobs
21+
*/
22+
class RevokeUserGrantsOnPasswordChange extends RevokeUserGrants
23+
{
24+
const REASON = 'password change';
25+
26+
public function __construct(User $user)
27+
{
28+
parent::__construct($user, null, self::REASON);
29+
}
30+
}

‎app/Jobs/RevokeUserGrantsOnSessionRevocation.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?php namespace App\Jobs;
2+
/*
3+
* Copyright 2024 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 Auth\User;
16+
17+
/**
18+
* Class RevokeUserGrantsOnSessionRevocation
19+
* Revokes all OAuth2 grants for a user when they explicitly sign out all other devices.
20+
* @package App\Jobs
21+
*/
22+
class RevokeUserGrantsOnSessionRevocation extends RevokeUserGrants
23+
{
24+
const REASON = 'user-initiated session revocation';
25+
26+
public function __construct(User $user)
27+
{
28+
parent::__construct($user, null, self::REASON);
29+
}
30+
}

‎app/Providers/EventServiceProvider.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use App\Events\UserLocked;
2020
use App\Events\UserPasswordResetRequestCreated;
2121
use App\Events\UserPasswordResetSuccessful;
22+
use App\Jobs\RevokeUserGrantsOnPasswordChange;
2223
use App\Events\UserSpamStateUpdated;
2324
use App\Audit\AuditContext;
2425
use App\libs\Auth\Repositories\IUserPasswordResetRequestRepository;
@@ -57,7 +58,7 @@ final class EventServiceProvider extends ServiceProvider
5758
'Illuminate\Database\Events\QueryExecuted' => [
5859
],
5960
'Illuminate\Auth\Events\Logout' => [
60-
//'App\Listeners\OnUserLogout',
61+
// 'App\Listeners\OnUserLogout',
6162
],
6263
'Illuminate\Auth\Events\Login' => [
6364
'App\Listeners\OnUserLogin',
@@ -169,6 +170,8 @@ public function boot()
169170
if(is_null($user)) return;
170171
if(!$user instanceof User) return;
171172
Mail::queue(new UserPasswordResetMail($user));
173+
// Revoke all OAuth2 tokens for this user across all clients
174+
RevokeUserGrantsOnPasswordChange::dispatch($user)->afterResponse();
172175
});
173176

174177
Event::listen(OAuth2ClientLocked::class, function($event){

‎app/Services/OpenId/UserService.php‎

Lines changed: 26 additions & 2 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;
@@ -33,6 +34,7 @@
3334
use Illuminate\Support\Facades\Config;
3435
use Illuminate\Support\Facades\Event;
3536
use Illuminate\Support\Facades\Log;
37+
use Illuminate\Support\Facades\Session;
3638
use Illuminate\Support\Facades\Mail;
3739
use Illuminate\Support\Facades\Storage;
3840
use models\exceptions\EntityNotFoundException;
@@ -292,7 +294,9 @@ public function create(array $payload): IEntity
292294
*/
293295
public function update(int $id, array $payload): IEntity
294296
{
295-
$user = $this->tx_service->transaction(function () use ($id, $payload) {
297+
$password_changed = false;
298+
299+
$user = $this->tx_service->transaction(function () use ($id, $payload, &$password_changed) {
296300

297301
$user = $this->repository->getById($id);
298302

@@ -372,12 +376,17 @@ public function update(int $id, array $payload): IEntity
372376

373377
if ($former_password != $user->getPassword()) {
374378
Log::warning(sprintf("UserService::update use id %s - password changed", $id));
375-
Event::dispatch(new UserPasswordResetSuccessful($user->getId()));
379+
$password_changed = true;
376380
}
377381

378382
return $user;
379383
});
380384

385+
if ($password_changed) {
386+
Session::regenerate();
387+
Event::dispatch(new UserPasswordResetSuccessful($user->getId()));
388+
}
389+
381390
try {
382391
if (Config::get("queue.enable_message_broker", false) == true)
383392
PublishUserUpdated::dispatch($user)->onConnection('message_broker');
@@ -473,6 +482,21 @@ public function updateProfilePhoto($user_id, UploadedFile $file, $max_file_size
473482
return $user;
474483
}
475484

485+
public function revokeAllGrantsOnSessionRevocation(int $user_id): void
486+
{
487+
$user = $this->tx_service->transaction(function () use ($user_id) {
488+
$user = $this->repository->getById($user_id);
489+
if (!$user instanceof User)
490+
throw new EntityNotFoundException("User not found.");
491+
492+
$user->setRememberToken(\Illuminate\Support\Str::random(60));
493+
494+
return $user;
495+
});
496+
497+
RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse();
498+
}
499+
476500
public function notifyMonitoredSecurityGroupActivity
477501
(
478502
string $action,

0 commit comments

Comments
 (0)