refactor(security): replace reason flag with RevokeUserGrants subclass hierarchy

mulldug · mulldug · commit feab57980f6d · 2026-03-11T10:53:53.000-05:00
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.
diff --git a/app/Http/Controllers/Api/UserApiController.php b/app/Http/Controllers/Api/UserApiController.php
@@ -13,7 +13,7 @@
  **/
 
 use App\Http\Controllers\APICRUDController;
-use App\Jobs\RevokeUserGrants;
+use App\Jobs\RevokeUserGrantsOnSessionRevocation;
 use App\Http\Controllers\Traits\RequestProcessor;
 use App\Http\Controllers\UserValidationRulesFactory;
 use App\ModelSerializers\SerializerRegistry;
@@ -259,7 +259,7 @@ public function revokeAllMyTokens()
             return $this->error403();
 
         $user = Auth::user();
-        RevokeUserGrants::dispatch($user, null, 'user-initiated session revocation')->afterResponse();
+        RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse();
         $user->setRememberToken(\Illuminate\Support\Str::random(60));
         return $this->deleted();
     }
diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php
@@ -13,7 +13,7 @@
  **/
 
 use App\Http\Controllers\OpenId\DiscoveryController;
-use App\Jobs\RevokeUserGrants;
+use App\Jobs\RevokeUserGrantsOnExplicitLogout;
 use App\Http\Controllers\OpenId\OpenIdController;
 use App\Http\Controllers\Traits\JsonResponses;
 use App\Http\Utils\CountryList;
@@ -674,7 +674,7 @@ public function getIdentity($identifier)
     public function logout()
     {
         $user = $this->auth_service->getCurrentUser();
-        RevokeUserGrants::dispatch($user, null, 'explicit logout')->afterResponse();
+        // RevokeUserGrantsOnExplicitLogout::dispatch($user)->afterResponse();
         $this->auth_service->logout();
         Session::flush();
         Session::regenerate();
diff --git a/app/Jobs/RevokeUserGrants.php b/app/Jobs/RevokeUserGrants.php
@@ -26,7 +26,7 @@
  * Class RevokeUserGrants
  * @package App\Jobs
  */
-class RevokeUserGrants implements ShouldQueue
+abstract class RevokeUserGrants implements ShouldQueue
 {
     use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
 
@@ -54,7 +54,7 @@ class RevokeUserGrants implements ShouldQueue
      * @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')
+    public function __construct(User $user, ?string $client_id, string $reason)
     {
         $this->user_id   = $user->getId();
         $this->client_id = $client_id;
diff --git a/app/Jobs/RevokeUserGrantsOnExplicitLogout.php b/app/Jobs/RevokeUserGrantsOnExplicitLogout.php
@@ -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 RevokeUserGrantsOnExplicitLogout
+ * Revokes all OAuth2 grants for a user when they explicitly log out.
+ * @package App\Jobs
+ */
+class RevokeUserGrantsOnExplicitLogout extends RevokeUserGrants
+{
+    public function __construct(User $user, ?string $client_id = null)
+    {
+        parent::__construct($user, $client_id, 'explicit logout');
+    }
+}
diff --git a/app/Jobs/RevokeUserGrantsOnPasswordChange.php b/app/Jobs/RevokeUserGrantsOnPasswordChange.php
@@ -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');
+    }
+}
diff --git a/app/Jobs/RevokeUserGrantsOnSessionRevocation.php b/app/Jobs/RevokeUserGrantsOnSessionRevocation.php
@@ -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');
+    }
+}
diff --git a/app/Listeners/OnUserLogout.php b/app/Listeners/OnUserLogout.php
@@ -12,7 +12,7 @@
  * limitations under the License.
  **/
 
-use App\Jobs\RevokeUserGrants;
+use App\Jobs\RevokeUserGrantsOnExplicitLogout;
 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()));
-        RevokeUserGrants::dispatch($user, null, 'explicit logout');
+        RevokeUserGrantsOnExplicitLogout::dispatch($user);
     }
 }
diff --git a/app/Providers/EventServiceProvider.php b/app/Providers/EventServiceProvider.php
@@ -19,7 +19,7 @@
 use App\Events\UserLocked;
 use App\Events\UserPasswordResetRequestCreated;
 use App\Events\UserPasswordResetSuccessful;
-use App\Jobs\RevokeUserGrants;
+use App\Jobs\RevokeUserGrantsOnPasswordChange;
 use App\Events\UserSpamStateUpdated;
 use App\Audit\AuditContext;
 use App\libs\Auth\Repositories\IUserPasswordResetRequestRepository;
@@ -171,7 +171,7 @@ public function boot()
             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();
+            RevokeUserGrantsOnPasswordChange::dispatch($user)->afterResponse();
         });
 
         Event::listen(OAuth2ClientLocked::class, function($event){
diff --git a/app/libs/OAuth2/OAuth2Protocol.php b/app/libs/OAuth2/OAuth2Protocol.php
@@ -13,7 +13,7 @@
  **/
 
 use App\Http\Utils\UserIPHelperProvider;
-use App\Jobs\RevokeUserGrants;
+use App\Jobs\RevokeUserGrantsOnExplicitLogout;
 use Exception;
 use Illuminate\Support\Facades\Log;
 use jwa\JSONWebSignatureAndEncryptionAlgorithms;
@@ -1562,9 +1562,11 @@ public function endSession(OAuth2Request $request = null)
                 );
             }
 
+            /*
             if(!is_null($user)){
-                RevokeUserGrants::dispatch($user, $client_id, 'explicit logout')->afterResponse();
+                RevokeUserGrantsOnExplicitLogout::dispatch($user, $client_id)->afterResponse();
             }
+            */
 
             if (!is_null($logged_user)) {
                 $this->auth_service->logout();
diff --git a/tests/PasswordChangeRevokeTokenTest.php b/tests/PasswordChangeRevokeTokenTest.php
@@ -14,7 +14,9 @@
 
 use App\Events\UserPasswordResetSuccessful;
 use App\Jobs\EmitAuditLogJob;
-use App\Jobs\RevokeUserGrants;
+use App\Jobs\RevokeUserGrantsOnExplicitLogout;
+use App\Jobs\RevokeUserGrantsOnPasswordChange;
+use App\Jobs\RevokeUserGrantsOnSessionRevocation;
 use Auth\User;
 use Illuminate\Support\Facades\Config;
 use Illuminate\Support\Facades\Event;
@@ -51,12 +53,12 @@ protected function tearDown(): void
     }
 
     // -------------------------------------------------------------------------
-    // Test 1: Dispatching UserPasswordResetSuccessful fires RevokeUserGrants
+    // Test 1: Dispatching UserPasswordResetSuccessful fires RevokeUserGrantsOnPasswordChange
     // -------------------------------------------------------------------------
 
     /**
      * When a UserPasswordResetSuccessful event is fired the EventServiceProvider
-     * listener must schedule a RevokeUserGrants job (all clients, reason = 'password change').
+     * listener must schedule a RevokeUserGrantsOnPasswordChange job for all clients.
      */
     public function testPasswordResetEventDispatchesRevokeUserGrantsJob(): void
     {
@@ -65,28 +67,25 @@ public function testPasswordResetEventDispatchesRevokeUserGrantsJob(): void
         // 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');
+        Queue::assertPushed(RevokeUserGrantsOnPasswordChange::class, function (RevokeUserGrantsOnPasswordChange $job) {
+            $ref      = new \ReflectionClass(\App\Jobs\RevokeUserGrants::class);
+            $userId   = $ref->getProperty('user_id');
+            $clientId = $ref->getProperty('client_id');
             $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';
+                && $clientId->getValue($job) === null;
         });
     }
 
     // -------------------------------------------------------------------------
-    // Test 2: PUT /admin/api/v1/users/me with password schedules RevokeUserGrants
+    // Test 2: PUT /admin/api/v1/users/me with password schedules RevokeUserGrantsOnPasswordChange
     // -------------------------------------------------------------------------
 
     /**
-     * Posting a new password via the profile API must schedule a RevokeUserGrants
-     * job so tokens from other sessions are revoked.
+     * Posting a new password via the profile API must schedule a
+     * RevokeUserGrantsOnPasswordChange job so tokens from other sessions are revoked.
      */
     public function testProfilePasswordChangePutsRevokeUserGrantsJobOnQueue(): void
     {
@@ -105,15 +104,12 @@ public function testProfilePasswordChangePutsRevokeUserGrantsJobOnQueue(): void
         // The listener for UserPasswordResetSuccessful uses afterResponse().
         app()->terminate();
 
-        Queue::assertPushed(RevokeUserGrants::class, function (RevokeUserGrants $job) {
-            $ref      = new \ReflectionObject($job);
+        Queue::assertPushed(RevokeUserGrantsOnPasswordChange::class, function (RevokeUserGrantsOnPasswordChange $job) {
+            $ref      = new \ReflectionClass(\App\Jobs\RevokeUserGrants::class);
             $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';
+            return $clientId->getValue($job) === null;
         });
     }
 
@@ -165,12 +161,12 @@ public function testCurrentSessionPreservedAfterPasswordChange(): void
     }
 
     // -------------------------------------------------------------------------
-    // Test 5: DELETE /admin/api/v1/users/me/tokens schedules RevokeUserGrants
+    // Test 5: DELETE /admin/api/v1/users/me/tokens schedules RevokeUserGrantsOnSessionRevocation
     // -------------------------------------------------------------------------
 
     /**
      * The "sign out all other devices" endpoint must respond with 204 and
-     * schedule a RevokeUserGrants job for all clients.
+     * schedule a RevokeUserGrantsOnSessionRevocation job for all clients.
      */
     public function testBulkRevokeEndpointSchedulesRevokeUserGrantsJob(): void
     {
@@ -179,23 +175,20 @@ public function testBulkRevokeEndpointSchedulesRevokeUserGrantsJob(): void
 
         app()->terminate();
 
-        Queue::assertPushed(RevokeUserGrants::class, function (RevokeUserGrants $job) {
-            $ref      = new \ReflectionObject($job);
+        Queue::assertPushed(RevokeUserGrantsOnSessionRevocation::class, function (RevokeUserGrantsOnSessionRevocation $job) {
+            $ref      = new \ReflectionClass(\App\Jobs\RevokeUserGrants::class);
             $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';
+                && $clientId->getValue($job) === null;
         });
     }
 
     // -------------------------------------------------------------------------
-    // Test 6: RevokeUserGrants::handle() calls revokeUsersToken with client_id
+    // Test 6: RevokeUserGrantsOnExplicitLogout passes client_id to token service
     // -------------------------------------------------------------------------
 
     /**
@@ -211,16 +204,16 @@ public function testRevokeUserGrantsJobPassesClientIdToTokenService(): void
                      ->once()
                      ->with($this->test_user->getId(), $client_id);
 
-        $job = new RevokeUserGrants($this->test_user, $client_id, 'unit test');
+        $job = new RevokeUserGrantsOnExplicitLogout($this->test_user, $client_id);
         $job->handle($mock_service);
     }
 
     // -------------------------------------------------------------------------
-    // Test 7: RevokeUserGrants::handle() calls revokeUsersToken with null client_id
+    // Test 7: RevokeUserGrantsOnPasswordChange passes null client_id to token service
     // -------------------------------------------------------------------------
 
     /**
-     * When constructed without a client_id the job must call
+     * RevokeUserGrantsOnPasswordChange must call
      * ITokenService::revokeUsersToken($user_id, null), revoking across all clients.
      */
     public function testRevokeUserGrantsJobPassesNullClientIdToTokenService(): void
@@ -230,7 +223,7 @@ public function testRevokeUserGrantsJobPassesNullClientIdToTokenService(): void
                      ->once()
                      ->with($this->test_user->getId(), null);
 
-        $job = new RevokeUserGrants($this->test_user, null, 'unit test');
+        $job = new RevokeUserGrantsOnPasswordChange($this->test_user);
         $job->handle($mock_service);
     }
 
@@ -239,8 +232,8 @@ public function testRevokeUserGrantsJobPassesNullClientIdToTokenService(): void
     // -------------------------------------------------------------------------
 
     /**
-     * When opentelemetry.enabled is true, RevokeUserGrants::handle() must
-     * dispatch an EmitAuditLogJob with the correct log message and audit fields.
+     * When opentelemetry.enabled is true, the job must dispatch an EmitAuditLogJob
+     * with the correct log message and audit fields.
      */
     public function testOtelAuditJobDispatchedWhenOpentelemetryEnabled(): void
     {
@@ -249,7 +242,7 @@ public function testOtelAuditJobDispatchedWhenOpentelemetryEnabled(): void
         $mock_service = Mockery::mock(ITokenService::class);
         $mock_service->shouldReceive('revokeUsersToken')->once();
 
-        $job = new RevokeUserGrants($this->test_user, null, 'password change');
+        $job = new RevokeUserGrantsOnPasswordChange($this->test_user);
         $job->handle($mock_service);
 
         Queue::assertPushed(EmitAuditLogJob::class, function (EmitAuditLogJob $emitted) {
@@ -267,8 +260,7 @@ public function testOtelAuditJobDispatchedWhenOpentelemetryEnabled(): void
     // -------------------------------------------------------------------------
 
     /**
-     * When opentelemetry.enabled is false, RevokeUserGrants::handle() must not
-     * dispatch any EmitAuditLogJob.
+     * When opentelemetry.enabled is false, the job must not dispatch any EmitAuditLogJob.
      */
     public function testOtelAuditJobNotDispatchedWhenOpentelemetryDisabled(): void
     {
@@ -277,7 +269,7 @@ public function testOtelAuditJobNotDispatchedWhenOpentelemetryDisabled(): void
         $mock_service = Mockery::mock(ITokenService::class);
         $mock_service->shouldReceive('revokeUsersToken')->once();
 
-        $job = new RevokeUserGrants($this->test_user, null, 'password change');
+        $job = new RevokeUserGrantsOnPasswordChange($this->test_user);
         $job->handle($mock_service);
 
         Queue::assertNotPushed(EmitAuditLogJob::class);

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@`
`12`	`12`	`* limitations under the License.`
`13`	`13`	`**/`
`14`	`14`
`15`		`-use App\Jobs\RevokeUserGrants;`
	`15`	`+use App\Jobs\RevokeUserGrantsOnExplicitLogout;`
`16`	`16`	`use Illuminate\Auth\Events\Logout;`
`17`	`17`	`use Illuminate\Support\Facades\Log;`
`18`	`18`
`@@ -33,6 +33,6 @@ public function handle(Logout $event)`
`33`	`33`	`{`
`34`	`34`	`$user = $event->user;`
`35`	`35`	`Log::debug(sprintf("OnUserLogout::handle user %s (%s)", $user->getEmail(), $user->getId()));`
`36`		`- RevokeUserGrants::dispatch($user, null, 'explicit logout');`
	`36`	`+ RevokeUserGrantsOnExplicitLogout::dispatch($user);`
`37`	`37`	`}`
`38`	`38`	`}`