-
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 2 commits
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\RevokeUserGrantsOnSessionRevocation; | ||||
| 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(); | ||||
| RevokeUserGrantsOnSessionRevocation::dispatch($user)->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. @mulldug this need to be moved to a service layer (
|
||||
| $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 |
|---|---|---|
| @@ -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 | ||
| */ | ||
| abstract class RevokeUserGrants implements ShouldQueue | ||
| { | ||
| 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, string $reason) | ||
| { | ||
| $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())); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| <?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; | ||
|
|
||
| /** | ||
| * Class RevokeUserGrantsOnPasswordChange | ||
| * Revokes all OAuth2 grants for a user across all clients after a password change. | ||
| * @package App\Jobs | ||
| */ | ||
| class RevokeUserGrantsOnPasswordChange extends RevokeUserGrants | ||
| { | ||
| public function __construct(User $user) | ||
| { | ||
| parent::__construct($user, null, 'password change'); | ||
|
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 move 'password change' to a constant |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| <?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; | ||
|
|
||
| /** | ||
| * Class RevokeUserGrantsOnSessionRevocation | ||
| * Revokes all OAuth2 grants for a user when they explicitly sign out all other devices. | ||
| * @package App\Jobs | ||
| */ | ||
| class RevokeUserGrantsOnSessionRevocation extends RevokeUserGrants | ||
| { | ||
| public function __construct(User $user) | ||
| { | ||
| parent::__construct($user, null, 'user-initiated session revocation'); | ||
|
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 move 'user-initiated session revocation' to a constant |
||
| } | ||
| } | ||
| 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\RevokeUserGrantsOnPasswordChange; | ||
| 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 | ||
| RevokeUserGrantsOnPasswordChange::dispatch($user)->afterResponse(); | ||
| }); | ||
|
|
||
| Event::listen(OAuth2ClientLocked::class, function($event){ | ||
|
|
||
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