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.

Author: mulldug

app/Jobs/RevokeUserGrants.php

@@ -49,16 +49,22 @@ abstract class RevokeUserGrants implements ShouldQueue
      */
     private string $reason;
 
+    /**
+     * @var string
+     */
+    private string $client_ip;
+
     /**
      * @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;
+        $this->user_id    = $user->getId();
+        $this->client_id  = $client_id;
+        $this->reason     = $reason;
+        $this->client_ip  = IPHelper::getUserIp();
         Log::debug(sprintf(
             "RevokeUserGrants::constructor user %s client_id %s reason %s",
             $this->user_id,
@@ -71,6 +77,14 @@ public function handle(ITokenService $service): void
     {
         Log::debug("RevokeUserGrants::handle");
 
+        try {
+            $service->revokeUsersToken($this->user_id, $this->client_id);
+        } catch (\Exception $ex) {
+            Log::warning(sprintf("RevokeUserGrants::handle attempt %d failed: %s",
+                $this->attempts(), $ex->getMessage()));
+            throw $ex;
+        }
+
         $scope = !empty($this->client_id)
             ? sprintf("client %s", $this->client_id)
             : "all clients";
@@ -82,7 +96,7 @@ public function handle(ITokenService $service): void
             $this->reason
         );
 
-        AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action);
+        AddUserAction::dispatch($this->user_id, $this->client_ip, $action);
 
         // Emit to OTEL audit log (Elasticsearch) if enabled
         if (config('opentelemetry.enabled', false)) {
@@ -95,18 +109,10 @@ public function handle(ITokenService $service): void
                 'audit.reason'      => $this->reason,
                 'audit.scope'       => $scope,
                 'auth.user.id'      => $this->user_id,
-                'client.ip'         => IPHelper::getUserIp(),
+                'client.ip'         => $this->client_ip,
                 'elasticsearch.index' => config('opentelemetry.logs.elasticsearch_index', 'logs-audit'),
             ]);
         }
-
-        try {
-            $service->revokeUsersToken($this->user_id, $this->client_id);
-        } catch (\Exception $ex) {
-            Log::warning(sprintf("RevokeUserGrants::handle attempt %d failed: %s",
-                $this->attempts(), $ex->getMessage()));
-            throw $ex;
-        }
     }
 
     public function failed(\Throwable $exception): void

app/Services/OpenId/UserService.php

@@ -293,7 +293,9 @@ public function create(array $payload): IEntity
      */
     public function update(int $id, array $payload): IEntity
     {
-        $user = $this->tx_service->transaction(function () use ($id, $payload) {
+        $password_changed = false;
+
+        $user = $this->tx_service->transaction(function () use ($id, $payload, &$password_changed) {
 
             $user = $this->repository->getById($id);
 
@@ -373,13 +375,17 @@ public function update(int $id, array $payload): IEntity
 
             if ($former_password != $user->getPassword()) {
                 Log::warning(sprintf("UserService::update use id %s - password changed", $id));
-                request()->session()->regenerate();
-                Event::dispatch(new UserPasswordResetSuccessful($user->getId()));
+                $password_changed = true;
             }
 
             return $user;
         });
 
+        if ($password_changed) {
+            request()->session()->regenerate();
+            Event::dispatch(new UserPasswordResetSuccessful($user->getId()));
+        }
+
         try {
             if (Config::get("queue.enable_message_broker", false) == true)
                 PublishUserUpdated::dispatch($user)->onConnection('message_broker');