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); + } +}