-
Notifications
You must be signed in to change notification settings - Fork 0
fix(security): revoke tokens and invalidate sessions on password change #117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
3d6cd09
feab579
1afebad
630d325
f61514b
5381c79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. replace RevokeUserGrants |
||
| $user->setRememberToken(\Illuminate\Support\Str::random(60)); | ||
| return $this->deleted(); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| public function updateMyPic(){ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
smarcet marked this conversation as resolved.
Outdated
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| $this->auth_service->logout(); | ||
| Session::flush(); | ||
| Session::regenerate(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| <?php namespace App\Jobs; | ||
| /* | ||
| * Copyright 2024 OpenStack Foundation | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| **/ | ||
|
|
||
| use Auth\User; | ||
| use Illuminate\Bus\Queueable; | ||
| use Illuminate\Contracts\Queue\ShouldQueue; | ||
| use Illuminate\Foundation\Bus\Dispatchable; | ||
| use Illuminate\Queue\InteractsWithQueue; | ||
| use Illuminate\Queue\SerializesModels; | ||
| use Illuminate\Support\Facades\Log; | ||
| use OAuth2\Services\ITokenService; | ||
| use Utils\IPHelper; | ||
|
|
||
| /** | ||
| * Class RevokeUserGrants | ||
| * @package App\Jobs | ||
| */ | ||
| class RevokeUserGrants implements ShouldQueue | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mulldug please make this class abstract
flags are antipatterns ( code smell ) |
||
| { | ||
| use Dispatchable, InteractsWithQueue, Queueable, SerializesModels; | ||
|
|
||
| public $tries = 5; | ||
|
|
||
| public $timeout = 0; | ||
|
|
||
| /** | ||
| * @var int | ||
| */ | ||
| private int $user_id; | ||
|
|
||
| /** | ||
| * @var string|null | ||
| */ | ||
| private ?string $client_id; | ||
|
|
||
| /** | ||
| * @var string | ||
| */ | ||
| private string $reason; | ||
|
|
||
| /** | ||
| * @param User $user | ||
| * @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') | ||
| { | ||
| $this->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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mulldug exception should be propagated in order to activate the retry mechanism |
||
| } | ||
| } | ||
|
|
||
| public function failed(\Throwable $exception): void | ||
| { | ||
| Log::error(sprintf("RevokeUserGrants::failed %s", $exception->getMessage())); | ||
| } | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. replace here RevokeUserGrants with see https://github.com/OpenStackweb/openstackid/pull/117/changes#r2913356525 for rationale |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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', | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mulldug please rollback this |
||
| ], | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| '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(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. replace here RevokeUserGrants with see https://github.com/OpenStackweb/openstackid/pull/117/changes#r2913356525 for rationale |
||
| }); | ||
|
|
||
| Event::listen(OAuth2ClientLocked::class, function($event){ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
smarcet marked this conversation as resolved.
Outdated
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. replace here RevokeUserGrants with see https://github.com/OpenStackweb/openstackid/pull/117/changes#r2913356525 for rationale
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
| */ | ||
|
|
||
| if (!is_null($logged_user)) { | ||
| $this->auth_service->logout(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mulldug move the session regeneration ( Session::regenerate() ) into the service layer where the password is actually changed
openstackid/app/Services/OpenId/UserService.php
Line 375 in 7b8a392