[4.x] UserImpersonation: store auth guard in session, add $logout param to stopImpersonating()#1437
Open
[4.x] UserImpersonation: store auth guard in session, add $logout param to stopImpersonating()#1437
$logout param to stopImpersonating()#1437Conversation
…e auth guard in 'tenancy_impersonation_guard' Also make `stopImpersonating()` able to keep the user logged in.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1437 +/- ##
============================================
+ Coverage 86.03% 86.05% +0.01%
- Complexity 1156 1158 +2
============================================
Files 184 184
Lines 3381 3385 +4
============================================
+ Hits 2909 2913 +4
Misses 472 472 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced the boolean impersonation session flag with storing the impersonation auth guard name; reordered session/token handling in response creation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App
participant Session
participant TokenStore
participant Auth
Client->>App: request impersonation
App->>Session: set tenancy_impersonation_guard = token.auth_guard
App->>TokenStore: delete impersonation token
App-->>Client: redirect/response
Client->>App: request stopImpersonating(logout = true/false)
App->>Session: read guard = tenancy_impersonation_guard
alt guard exists and logout == true
App->>Auth: auth(guard).logout()
end
App->>Session: forget tenancy_impersonation_guard
App-->>Client: redirect/response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/Features/UserImpersonation.php`:
- Line 79: The session check and cleanup currently only look for
'tenancy_impersonation_guard', which breaks existing sessions using the legacy
'tenancy_impersonating' key; update the UserImpersonation logic so read
operations (e.g., isImpersonating or the method that returns
session()->has(...)) check for either session key ('tenancy_impersonation_guard'
OR 'tenancy_impersonating'), and update any cleanup/clear method (the method
that forgets/clears the impersonation at line ~93) to remove both keys so legacy
sessions get cleaned up during upgrade; keep writing the new key going forward
but accept and clear the old key for one upgrade cycle.
In `@tests/TenantUserImpersonationTest.php`:
- Around line 153-192: Update the test to cover the case where the current
request guard differs from the stored impersonation guard: after creating the
impersonation (tenancy()->impersonate) and asserting impersonation is active,
simulate a different current guard (for example call Auth::shouldUse('api') or
Auth::guard('api')->loginUsingId($user->id)) while leaving the session key
tenancy_impersonation_guard set to the original guard (e.g. 'web'), then call
UserImpersonation::stopImpersonating(false) and assert impersonation was cleared
(UserImpersonation::isImpersonating() is false and
session('tenancy_impersonation_guard') is null) and that the user remains
authenticated under the current guard and can access the dashboard; reference
tenancy()->impersonate, UserImpersonation::stopImpersonating(false), and the
session key 'tenancy_impersonation_guard' when adding these steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04d2c13e-3479-445b-87f6-cb5505d10f55
📒 Files selected for processing (2)
src/Features/UserImpersonation.phptests/TenantUserImpersonationTest.php
Starting impersonation using 'web', then using a different guard and calling UserImpersonation::stopImpersonating() should log out the user from the 'web' guard stored in `session('tenancy_impersonation_guard')`.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/TenantUserImpersonationTest.php (1)
228-232:⚠️ Potential issue | 🟠 MajorSwitch the active guard before calling
stopImpersonating().
auth('test')->loginUsingId($user->id)authenticates that guard, but it does not maketestthe guard resolved byauth()here. WithoutAuth::shouldUse('test')(or equivalent), the oldauth()->logout()behavior would still log outweb, so this test does not actually pin the regression this PR is fixing.Proposed change
// Manually log in the user using a different guard auth('test')->loginUsingId($user->id); + Auth::shouldUse('test'); // Should log out the user from the guard used for impersonation ('web') UserImpersonation::stopImpersonating();In Laravel, does calling `auth('test')->loginUsingId($id)` change the guard used by subsequent `auth()->logout()` calls, or is `Auth::shouldUse('test')` required to switch the current/default guard?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/TenantUserImpersonationTest.php` around lines 228 - 232, Test currently logs in with auth('test')->loginUsingId($user->id) but doesn't switch the active guard, so stopImpersonating() still operates on the default guard; call Auth::shouldUse('test') (or Auth::shouldUse($previousGuard) to restore later) before logging in or before invoking UserImpersonation::stopImpersonating() so the active guard matches auth('test')->loginUsingId and the test exercises the logout behavior on the intended guard (reference auth('test')->loginUsingId, Auth::shouldUse, and UserImpersonation::stopImpersonating).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/TenantUserImpersonationTest.php`:
- Around line 228-232: Test currently logs in with
auth('test')->loginUsingId($user->id) but doesn't switch the active guard, so
stopImpersonating() still operates on the default guard; call
Auth::shouldUse('test') (or Auth::shouldUse($previousGuard) to restore later)
before logging in or before invoking UserImpersonation::stopImpersonating() so
the active guard matches auth('test')->loginUsingId and the test exercises the
logout behavior on the intended guard (reference auth('test')->loginUsingId,
Auth::shouldUse, and UserImpersonation::stopImpersonating).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48ae8e55-1100-4ba5-bef0-3fa70abe0b8e
📒 Files selected for processing (1)
tests/TenantUserImpersonationTest.php
Originally, we just authenticated the user using a different guard, without actually switching to that guard. Now, we fully switch to the 'test' guard.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/TenantUserImpersonationTest.php`:
- Around line 194-237: Add a defensive check to
UserImpersonation::stopImpersonating() so it does nothing if there is no stored
tenancy_impersonation_guard in session (i.e. when not impersonating) —
specifically, read the guard via session('tenancy_impersonation_guard') and if
null/empty return early instead of calling auth(null)->logout(); then add the
edge-case test that calls UserImpersonation::stopImpersonating() when
UserImpersonation::isImpersonating() is false to assert the current
authenticated user remains logged in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d8079f0-763a-43ae-969e-d4f2d7021257
📒 Files selected for processing (1)
tests/TenantUserImpersonationTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/UserImpersonation.php`:
- Around line 88-90: Replace the generic Exception thrown in the
UserImpersonation::isImpersonating check with a specific exception type: create
a new NotImpersonatingException (e.g., class NotImpersonatingException extends
\Exception with the existing message) under the Stancl\Tenancy\Exceptions
namespace, import that exception in src/Features/UserImpersonation.php, and
change the throw in the block that currently calls throw new Exception('Not
currently impersonating any user.'); to throw new NotImpersonatingException();
so callers can catch the specific error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfd17528-30ae-4366-8600-28a387fabe14
📒 Files selected for processing (2)
src/Features/UserImpersonation.phptests/TenantUserImpersonationTest.php
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/UserImpersonation.php`:
- Around line 83-86: Update the docblock for the stopImpersonating method to
accurately document the optional logout behavior: mention the $logout parameter
(bool $logout = true) and clarify that when $logout is false the method will
only forget the impersonation session without logging the user out; include a
short description of both behaviors and the parameter purpose next to the method
signature (stopImpersonating).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3c06c11-0bcc-4c6f-8367-3798c71a8362
📒 Files selected for processing (2)
src/Features/UserImpersonation.phptests/TenantUserImpersonationTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/UserImpersonation.php`:
- Around line 98-104: The session cleanup
(session()->forget('tenancy_impersonation_guard')) can be skipped if
auth($guard)->logout() throws; update the impersonation cleanup so that the
logout call is executed inside a try/finally (or try/catch/finally) and
guarantee session()->forget('tenancy_impersonation_guard') runs in the finally
block; catch and optionally log any exception from auth($guard)->logout() (using
your logger) but always clear the session key obtained via
session()->get('tenancy_impersonation_guard') to avoid leaving impersonation
state stuck.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8469df72-6b29-41c0-8459-82986ee98dde
📒 Files selected for processing (1)
src/Features/UserImpersonation.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
2 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.
The 'tenancy_impersonating' session variable got replaced by 'tenancy_impersonation_guard'.
UserImpersonation::stopImpersonating()now callslogout()on the guard retrieved bysession()->get('tenancy_impersonation_guard')instead of callinglogout()on the current auth guard. Now. if you create the impersonation token with guard 'web', and callUserImpersonation::stopImpersonating(), for example in a route that has theauth:sanctummiddleware (= the current guard in that route would beRequestGuardwhich doesn't even have thelogout()method -- not the guard for which the impersonation token was created), the method will correctly log the user out of the 'web' guard using which he was actually authenticated instead of the current guard of the visited route (which doesn't have to be the same guard for which impersonation started).UserImpersonation::stopImpersonating()now also accepts the$logoutparameter, which istrueby default. Iffalseis passed, the method just forgetstenancy_impersonation_guardfrom session without logging out.Summary by CodeRabbit
New Features
Tests