fix(security): revoke tokens and invalidate sessions on password change#117
Open
fix(security): revoke tokens and invalidate sessions on password change#117
Conversation
- Introduce RevokeUserGrants job (replaces RevokeUserGrantsOnExplicitLogout)
with an optional client_id and a reason parameter; remove the guard
clause that silently prevented all-client revocation.
- Wire RevokeUserGrants to UserPasswordResetSuccessful so all OAuth2
tokens are revoked whenever a password is reset or changed.
- Rotate remember_token in User::setPassword() to invalidate remember-me
cookies on other devices.
- Regenerate the session in UserApiController::updateMe() after a
password change to protect against session fixation.
- Add DELETE /admin/api/v1/users/me/tokens endpoint with a corresponding
"Sign Out All Other Devices" button on the profile page.
- Add PasswordChangeRevokeTokenTest covering all eight scenarios from
the security specification.
romanetar
requested changes
Mar 10, 2026
app/Listeners/OnUserLogout.php
Outdated
| $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.
smarcet
reviewed
Mar 10, 2026
app/Jobs/RevokeUserGrants.php
Outdated
| * Class RevokeUserGrants | ||
| * @package App\Jobs | ||
| */ | ||
| class RevokeUserGrants implements ShouldQueue |
Collaborator
There was a problem hiding this comment.
@mulldug please make this class abstract
and create:
- RevokeUserGrantsOnExplicitLogout that inherits from this and set the reason at constructor
- also do the same and create an specialization called RevokeUserGrantsOnPasswordChange and
RevokeUserGrantsOnUserInitiatedRequest
abstract class RevokeUserGrants implements ShouldQueue {
//....
protected string $reason
}
final class RevokeUserGrantsOnExplicitLogout extends
RevokeUserGrants {
public function __construct($user, $client_id){
parent::__construct($user, $client_id, 'explicit logout');
}
}
final class RevokeUserGrantsOnPasswordChange extends
RevokeUserGrants {
public function __construct($user, $client_id){
parent::__construct($user, null, ''password change'');
}
}
final class RevokeUserGrantsOnUserInitiatedRequest extends
RevokeUserGrants {
public function __construct($user, $client_id){
parent::__construct($user, null, 'user-initiated session revocation');
}
}
flags are antipatterns ( code smell )
smarcet
reviewed
Mar 10, 2026
app/libs/OAuth2/OAuth2Protocol.php
Outdated
| /* | ||
| if(!is_null($user)){ | ||
| RevokeUserGrantsOnExplicitLogout::dispatch($user, $client_id)->afterResponse(); | ||
| RevokeUserGrants::dispatch($user, $client_id, 'explicit logout')->afterResponse(); |
Collaborator
There was a problem hiding this comment.
replace here RevokeUserGrants with
RevokeUserGrantsOnExplicitLogout::dispatch($user, $client_id')->afterResponse();
see https://github.com/OpenStackweb/openstackid/pull/117/changes#r2913356525 for rationale
smarcet
reviewed
Mar 10, 2026
app/Listeners/OnUserLogout.php
Outdated
| $user = $event->user; | ||
| Log::debug(sprintf("OnUserLogout::handle user %s (%s)", $user->getEmail(), $user->getId())); | ||
| RevokeUserGrantsOnExplicitLogout::dispatch($user); | ||
| RevokeUserGrants::dispatch($user, null, 'explicit logout'); |
Collaborator
There was a problem hiding this comment.
replace here RevokeUserGrants with
RevokeUserGrantsOnExplicitLogout::dispatch($user, $client_id')->afterResponse();
see https://github.com/OpenStackweb/openstackid/pull/117/changes#r2913356525 for rationale
smarcet
reviewed
Mar 10, 2026
| 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.
replace here RevokeUserGrants with
RevokeUserGrantsOnPasswordChange::dispatch($user)->afterResponse();
see https://github.com/OpenStackweb/openstackid/pull/117/changes#r2913356525 for rationale
smarcet
reviewed
Mar 10, 2026
| return $this->error403(); | ||
|
|
||
| $user = Auth::user(); | ||
| RevokeUserGrants::dispatch($user, null, 'user-initiated session revocation')->afterResponse(); |
Collaborator
There was a problem hiding this comment.
replace RevokeUserGrants
with RevokeUserGrantsOnUserInitiatedRequest::dispatch($user)
see rationale here https://github.com/OpenStackweb/openstackid/pull/117/changes#diff-c9a5faef38fdfdc240cc8783a852d995bb99a0640daa0f5de44c7ab04d7103b2R29
smarcet
reviewed
Mar 10, 2026
| { | ||
| $user = $this->auth_service->getCurrentUser(); | ||
| //RevokeUserGrantsOnExplicitLogout::dispatch($user)->afterResponse(); | ||
| RevokeUserGrants::dispatch($user, null, 'explicit logout')->afterResponse(); |
Collaborator
smarcet
reviewed
Mar 10, 2026
app/libs/OAuth2/OAuth2Protocol.php
Outdated
| /* | ||
| if(!is_null($user)){ | ||
| RevokeUserGrantsOnExplicitLogout::dispatch($user, $client_id)->afterResponse(); | ||
| RevokeUserGrants::dispatch($user, $client_id, 'explicit logout')->afterResponse(); |
Collaborator
Collaborator
|
@mulldug please review comments What a Correct Fix Would Look Like The token revocation on password change/reset is the right security feature. But it should be kept separate from the logout-triggered revocation:
Additionally, there's now double dispatch on logout:
This results in two revocation jobs for every web logout. The EventServiceProvider re-enables 'App\Listeners\OnUserLogout' (was commented out), so both paths fire. |
…s hierarchy 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.
📝 WalkthroughWalkthroughAdds a revoke-all-tokens API and UI flow plus job-driven revocation: new abstract RevokeUserGrants job and three subclasses, IUserService interface and UserService implementation update for session revocation (remember_token rotation), controller/route and JS/frontend wiring, event dispatch updates, and comprehensive tests. Changes
Sequence DiagramsequenceDiagram
participant User as "User"
participant Browser as "Browser"
participant API as "API Controller"
participant Queue as "Job Queue"
participant TokenSvc as "Token Service"
participant Audit as "Audit Service"
participant OTEL as "OTEL (EmitAuditLogJob)"
User->>Browser: Click "Sign Out All Other Devices"
Browser->>API: DELETE /admin/api/v1/users/me/tokens
API->>API: Authenticate, identify current user
API->>Queue: Dispatch RevokeUserGrantsOnSessionRevocation (afterResponse)
API->>Browser: 204 No Content
Queue->>Audit: Dispatch AddUserAction (audit entry)
Queue->>TokenSvc: ITokenService->revokeUsersToken(user_id, client_id|null)
TokenSvc->>TokenSvc: Revoke tokens/sessions
alt OTEL enabled
Queue->>OTEL: Dispatch EmitAuditLogJob
OTEL->>OTEL: Emit telemetry/audit event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
app/Jobs/RevokeUserGrants.php (2)
86-86:IPHelper::getUserIp()may return empty in queue worker context.This job runs asynchronously in a queue worker where there's no HTTP request context.
$_SERVER['HTTP_CLIENT_IP'],$_SERVER['HTTP_X_FORWARDED_FOR'], and$_SERVER['REMOTE_ADDR']will likely be unset or empty, resulting in an empty IP in audit logs.Consider capturing the IP at dispatch time (in the controller/event handler) and passing it to the constructor.
♻️ Proposed fix
- private string $reason; + private string $reason; + + /** + * `@var` string + */ + private string $ip; /** * `@param` User $user * `@param` string|null $client_id null = revoke across all clients * `@param` string $reason audit message suffix + * `@param` string $ip client IP address at dispatch time */ - public function __construct(User $user, ?string $client_id, string $reason) + public function __construct(User $user, ?string $client_id, string $reason, string $ip = '') { $this->user_id = $user->getId(); $this->client_id = $client_id; $this->reason = $reason; + $this->ip = $ip ?: IPHelper::getUserIp();Then use
$this->ipinstead ofIPHelper::getUserIp()inhandle().Also applies to: 99-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Jobs/RevokeUserGrants.php` at line 86, The job currently calls AddUserAction::dispatch($this->user_id, IPHelper::getUserIp(), $action) inside handle(), but IPHelper::getUserIp() can be empty in queue workers; instead capture the IP at dispatch time and pass it into the job via its constructor, store it as a property (e.g., $this->ip) on the job, and update the job's handle() to call AddUserAction::dispatch($this->user_id, $this->ip, $action) (ensure the constructor signature and any serialization of the job include the new $ip property).
35-35: Consider setting a reasonable timeout for the job.
$timeout = 0means the job runs indefinitely. IfrevokeUsersToken()hangs or the token service becomes unresponsive, the queue worker will be blocked. Consider setting a reasonable timeout (e.g., 30-60 seconds).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Jobs/RevokeUserGrants.php` at line 35, The job class RevokeUserGrants currently sets public $timeout = 0 which allows it to run indefinitely; change this to a reasonable timeout (e.g., 30 or 60 seconds) by updating the $timeout property on the RevokeUserGrants class so that long-running calls like revokeUsersToken() will be aborted by the queue worker if they exceed the limit, and ensure the value is documented in a comment for clarity.app/Http/Controllers/UserController.php (1)
16-16: Unused import.
RevokeUserGrantsOnExplicitLogoutis imported but the dispatch call on line 677 is commented out. If the intent is to keep logout from revoking tokens (per PR discussion), this import can be removed.♻️ Proposed fix
-use App\Jobs\RevokeUserGrantsOnExplicitLogout;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Http/Controllers/UserController.php` at line 16, Remove the unused import of RevokeUserGrantsOnExplicitLogout from the UserController by deleting the line "use App\Jobs\RevokeUserGrantsOnExplicitLogout;" since the dispatch call that would use it is commented out; if you later re-enable the revoke-on-logout behavior (the commented dispatch), re-add the import at that time.tests/PasswordChangeRevokeTokenTest.php (1)
90-114: Consider test isolation for password changes.This test modifies the user's password but doesn't restore the original password in a teardown. If tests run in a specific order and share database state, subsequent tests relying on the original password (
'1Qaz2wsx!') may fail.Consider either:
- Using database transactions that rollback after each test
- Restoring the original password in teardown
- Using a dedicated test user that gets recreated for each test
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/PasswordChangeRevokeTokenTest.php` around lines 90 - 114, The test testProfilePasswordChangePutsRevokeUserGrantsJobOnQueue changes the shared test user's password and does not restore it, risking order-dependent failures; fix by ensuring test isolation: wrap this test in a DB transaction that rolls back (or use PHPUnit/Laravel RefreshDatabase trait) or restore the original password in the test teardown (tearDown) for the $this->test_user, or create and use a dedicated ephemeral user for this test; keep the existing assertions (Queue::assertPushed for RevokeUserGrantsOnPasswordChange and the app()->terminate call) but ensure the user state is reset afterwards so other tests expecting the original password ('1Qaz2wsx!') are not affected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Http/Controllers/Api/UserApiController.php`:
- Around line 256-265: In revokeAllMyTokens(), the call to
$user->setRememberToken(...) updates the Doctrine User entity but is never
persisted; after dispatching RevokeUserGrantsOnSessionRevocation and setting the
new token you must persist the change (e.g., call the Doctrine EntityManager
flush for the User entity or invoke the existing user service method that
updates the user) so the rotated remember token is saved to the database and
"remember me" cookies are invalidated across devices.
In `@app/Jobs/RevokeUserGrants.php`:
- Around line 104-107: The catch in RevokeUserGrants (within the job handling
call to $service->revokeUsersToken($this->user_id, $this->client_id)) is
swallowing exceptions and preventing Laravel from retrying the job; modify the
handler so that after logging the exception (Log::error(...)) the exception is
re-thrown (or remove the try/catch entirely) so the thrown exception propagates
out of the job and allows the job's $tries retry behavior to work as intended.
In `@app/Providers/EventServiceProvider.php`:
- Around line 60-62: The EventServiceProvider currently maps
'Illuminate\Auth\Events\Logout' to the 'App\Listeners\OnUserLogout' listener
which dispatches RevokeUserGrantsOnExplicitLogout and therefore revokes tokens
on every standard logout; to meet the PR objective, remove or disable that
mapping (or adjust OnUserLogout) so that only explicit token-revoking flows
(password reset/change handlers and the "Sign Out All Devices" action) dispatch
RevokeUserGrantsOnExplicitLogout, and ensure UserController::logout() and
OAuth2Protocol::endSession() do not trigger token revocation.
In `@tests/PasswordChangeRevokeTokenTest.php`:
- Line 3: Update the file header in PasswordChangeRevokeTokenTest (the top
comment block) to replace the copyright year "2024" with "2026"; locate the
file-level comment at the top of PasswordChangeRevokeTokenTest.php and change
the year token to 2026.
---
Nitpick comments:
In `@app/Http/Controllers/UserController.php`:
- Line 16: Remove the unused import of RevokeUserGrantsOnExplicitLogout from the
UserController by deleting the line "use
App\Jobs\RevokeUserGrantsOnExplicitLogout;" since the dispatch call that would
use it is commented out; if you later re-enable the revoke-on-logout behavior
(the commented dispatch), re-add the import at that time.
In `@app/Jobs/RevokeUserGrants.php`:
- Line 86: The job currently calls AddUserAction::dispatch($this->user_id,
IPHelper::getUserIp(), $action) inside handle(), but IPHelper::getUserIp() can
be empty in queue workers; instead capture the IP at dispatch time and pass it
into the job via its constructor, store it as a property (e.g., $this->ip) on
the job, and update the job's handle() to call
AddUserAction::dispatch($this->user_id, $this->ip, $action) (ensure the
constructor signature and any serialization of the job include the new $ip
property).
- Line 35: The job class RevokeUserGrants currently sets public $timeout = 0
which allows it to run indefinitely; change this to a reasonable timeout (e.g.,
30 or 60 seconds) by updating the $timeout property on the RevokeUserGrants
class so that long-running calls like revokeUsersToken() will be aborted by the
queue worker if they exceed the limit, and ensure the value is documented in a
comment for clarity.
In `@tests/PasswordChangeRevokeTokenTest.php`:
- Around line 90-114: The test
testProfilePasswordChangePutsRevokeUserGrantsJobOnQueue changes the shared test
user's password and does not restore it, risking order-dependent failures; fix
by ensuring test isolation: wrap this test in a DB transaction that rolls back
(or use PHPUnit/Laravel RefreshDatabase trait) or restore the original password
in the test teardown (tearDown) for the $this->test_user, or create and use a
dedicated ephemeral user for this test; keep the existing assertions
(Queue::assertPushed for RevokeUserGrantsOnPasswordChange and the
app()->terminate call) but ensure the user state is reset afterwards so other
tests expecting the original password ('1Qaz2wsx!') are not affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bcbffd1-229b-4394-bf50-00d7a716f18c
📒 Files selected for processing (13)
app/Http/Controllers/Api/UserApiController.phpapp/Http/Controllers/UserController.phpapp/Jobs/RevokeUserGrants.phpapp/Jobs/RevokeUserGrantsOnExplicitLogout.phpapp/Jobs/RevokeUserGrantsOnPasswordChange.phpapp/Jobs/RevokeUserGrantsOnSessionRevocation.phpapp/Providers/EventServiceProvider.phpapp/libs/Auth/Models/User.phpresources/js/profile/actions.jsresources/js/profile/profile.jsresources/views/profile.blade.phproutes/web.phptests/PasswordChangeRevokeTokenTest.php
smarcet
reviewed
Mar 11, 2026
| ], | ||
| 'Illuminate\Auth\Events\Logout' => [ | ||
| //'App\Listeners\OnUserLogout', | ||
| 'App\Listeners\OnUserLogout', |
Collaborator
There was a problem hiding this comment.
@mulldug please rollback this
OnUserLogout listener revokes tokens on logout, contradicting PR objectives.
smarcet
reviewed
Mar 11, 2026
| return $this->error403(); | ||
|
|
||
| $user = Auth::user(); | ||
| RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse(); |
Collaborator
There was a problem hiding this comment.
@mulldug this need to be moved to a service layer (
) method (IUserService::RevokeUserGrantsOnSessionRevocation($user_id)) due its need a tx context in order to persist$user->setRememberToken(\Illuminate\Support\Str::random(60));
smarcet
reviewed
Mar 11, 2026
| { | ||
| public function __construct(User $user) | ||
| { | ||
| parent::__construct($user, null, 'user-initiated session revocation'); |
Collaborator
There was a problem hiding this comment.
@mulldug move 'user-initiated session revocation' to a constant
smarcet
reviewed
Mar 11, 2026
| { | ||
| public function __construct(User $user) | ||
| { | ||
| parent::__construct($user, null, 'password change'); |
smarcet
reviewed
Mar 11, 2026
app/Jobs/RevokeUserGrants.php
Outdated
|
|
||
| $service->revokeUsersToken($this->user_id, $this->client_id); | ||
| } catch (\Exception $ex) { | ||
| Log::error($ex); |
Collaborator
There was a problem hiding this comment.
@mulldug exception should be propagated in order to activate the retry mechanism
smarcet
reviewed
Mar 11, 2026
| $password_changed = request()->filled('password'); | ||
| $response = $this->update(Auth::user()->getId()); | ||
| if ($password_changed) { | ||
| request()->session()->regenerate(); |
Collaborator
There was a problem hiding this comment.
@mulldug move the session regeneration ( Session::regenerate() ) into the service layer where the password is actually changed
… fix job retries
- Add REASON constant to each RevokeUserGrants subclass so the reason
string is defined once on the class that owns it.
- Extract revokeAllMyTokens logic into IUserService::revokeAllGrantsOnSessionRevocation()
so that setRememberToken() is called inside a Doctrine transaction and
the rotated token is actually persisted.
- Move session regeneration from UserApiController::updateMe() into
UserService::update(), triggered by the real password-change condition
($former_password != $user->getPassword()) rather than the presence of
the password field in the request payload.
- Fix RevokeUserGrants retry behaviour: catch the exception from
revokeUsersToken(), log it at warning level with the attempt count,
then re-throw so the queue worker schedules the next retry. Final
failure is still logged at error level via failed().
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Jobs/RevokeUserGrants.php`:
- Around line 57-108: The job currently dispatches AddUserAction and
EmitAuditLogJob before calling revokeUsersToken, and calls IPHelper::getUserIp()
at handle time; change the constructor of RevokeUserGrants to capture and store
the initiating IP (e.g. $this->user_ip = IPHelper::getUserIp()), remove the
pre-revoke dispatches inside handle, call
$service->revokeUsersToken($this->user_id, $this->client_id) inside the try
block first, and only after it returns successfully dispatch
AddUserAction::dispatch($this->user_id, $this->user_ip, $action) and
EmitAuditLogJob::dispatch(...) using the stored $this->user_ip and existing
$scope/$action/$reason values so retries don’t produce duplicate audit entries
on failure.
In `@app/Services/OpenId/UserService.php`:
- Around line 374-377: In UserService::update, avoid calling
request()->session()->regenerate() and Event::dispatch(new
UserPasswordResetSuccessful($user->getId())) inside the Doctrine transaction
closure; instead set a local flag (e.g., $password_changed = true) when
$former_password != $user->getPassword() inside the callback, let the
transaction commit/return normally, and after the transaction completes check
that flag and then call request()->session()->regenerate() and
Event::dispatch(...) so session rotation and the UserPasswordResetSuccessful
event occur only once and only after a successful commit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92293465-c35f-49e9-abb3-faf8ac48c544
📒 Files selected for processing (9)
app/Http/Controllers/Api/UserApiController.phpapp/Jobs/RevokeUserGrants.phpapp/Jobs/RevokeUserGrantsOnExplicitLogout.phpapp/Jobs/RevokeUserGrantsOnPasswordChange.phpapp/Jobs/RevokeUserGrantsOnSessionRevocation.phpapp/Providers/EventServiceProvider.phpapp/Services/OpenId/UserService.phpapp/libs/OpenId/Services/IUserService.phptests/PasswordChangeRevokeTokenTest.php
🚧 Files skipped from review as they are similar to previous changes (4)
- app/Http/Controllers/Api/UserApiController.php
- app/Jobs/RevokeUserGrantsOnSessionRevocation.php
- app/Jobs/RevokeUserGrantsOnPasswordChange.php
- tests/PasswordChangeRevokeTokenTest.php
…keUserGrants 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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Services/OpenId/UserService.php (1)
484-497: Consider using queue dispatch instead ofafterResponse()for reliability.Using
afterResponse()runs the revocation job synchronously in the current process after the HTTP response is sent. If the job fails (e.g., token service unavailable), there's no automatic retry—tokens may remain valid while the user expects them revoked.The
remember_tokenrotation (line 491) is durable because it's in the transaction, but the actual OAuth2 token revocation inRevokeUserGrantsOnSessionRevocationlacks retry guarantees.Consider queuing the job normally to leverage the 5-retry mechanism defined in
RevokeUserGrants:Suggested change
- RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse(); + RevokeUserGrantsOnSessionRevocation::dispatch($user);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/OpenId/UserService.php` around lines 484 - 497, The revocation job is currently dispatched with afterResponse() in revokeAllGrantsOnSessionRevocation, which bypasses the queue retry semantics; change RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse() to a normal queued dispatch (e.g., RevokeUserGrantsOnSessionRevocation::dispatch($user) or dispatch(new RevokeUserGrantsOnSessionRevocation($user))) so the job runs via the queue worker and benefits from the retry/backoff logic defined on RevokeUserGrants; ensure the job class implements ShouldQueue if not already.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Services/OpenId/UserService.php`:
- Around line 484-497: The revocation job is currently dispatched with
afterResponse() in revokeAllGrantsOnSessionRevocation, which bypasses the queue
retry semantics; change
RevokeUserGrantsOnSessionRevocation::dispatch($user)->afterResponse() to a
normal queued dispatch (e.g.,
RevokeUserGrantsOnSessionRevocation::dispatch($user) or dispatch(new
RevokeUserGrantsOnSessionRevocation($user))) so the job runs via the queue
worker and benefits from the retry/backoff logic defined on RevokeUserGrants;
ensure the job class implements ShouldQueue if not already.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc083060-2736-41e3-85af-f891a43e085e
📒 Files selected for processing (2)
app/Jobs/RevokeUserGrants.phpapp/Services/OpenId/UserService.php
smarcet
reviewed
Mar 13, 2026
app/Services/OpenId/UserService.php
Outdated
| }); | ||
|
|
||
| if ($password_changed) { | ||
| request()->session()->regenerate(); |
Collaborator
There was a problem hiding this comment.
Replace request()->session()->regenerate() with Session::regenerate() to match the pattern used throughout the rest of the project, and add the missing Session facade import.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/Services/OpenId/UserService.php (1)
485-498: Implementation looks correct; consider importingStrfor consistency.The method correctly:
- Updates the
remember_tokenwithin a transaction to invalidate remember-me cookies- Dispatches the revocation job after the response using
afterResponse()Line 492 uses the fully qualified
\Illuminate\Support\Str::random(60). For consistency with typical Laravel conventions, consider importing theStrfacade.♻️ Optional: Import Str facade
Add to imports:
use Illuminate\Support\Str;Then update line 492:
- $user->setRememberToken(\Illuminate\Support\Str::random(60)); + $user->setRememberToken(Str::random(60));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/Services/OpenId/UserService.php` around lines 485 - 498, Replace the fully-qualified Str call in revokeAllGrantsOnSessionRevocation with an imported alias: add use Illuminate\Support\Str; to the top imports and change the \Illuminate\Support\Str::random(60) usage inside revokeAllGrantsOnSessionRevocation to Str::random(60); keep the rest of the logic (transaction via tx_service, repository->getById, User check, setRememberToken, and dispatch of RevokeUserGrantsOnSessionRevocation) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/Services/OpenId/UserService.php`:
- Around line 485-498: Replace the fully-qualified Str call in
revokeAllGrantsOnSessionRevocation with an imported alias: add use
Illuminate\Support\Str; to the top imports and change the
\Illuminate\Support\Str::random(60) usage inside
revokeAllGrantsOnSessionRevocation to Str::random(60); keep the rest of the
logic (transaction via tx_service, repository->getById, User check,
setRememberToken, and dispatch of RevokeUserGrantsOnSessionRevocation)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 633df0cd-c2cc-4372-8922-51852b525d8f
📒 Files selected for processing (1)
app/Services/OpenId/UserService.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://tipit.avaza.com/project/view/188885#!tab=task-pane&groupby=ProjectSection&view=vertical&task=3976826&stafilter=NotComplete&fileview=grid
Summary by CodeRabbit