[4.x] UserImpersonation: store auth guard in session, add $logout param to stopImpersonating()#1437
Conversation
…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 85.90% 85.91% +0.01%
- Complexity 1170 1172 +2
============================================
Files 185 185
Lines 3412 3416 +4
============================================
+ Hits 2931 2935 +4
Misses 481 481 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. 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)
📝 WalkthroughWalkthroughSession tracking switched from a boolean to storing the impersonation guard name in session; token deletion order adjusted. ChangesUser Impersonation Feature
Impersonation Tests
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: response/redirect
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: response/redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@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
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Updates the tenant user impersonation feature to track and use the originating auth guard when ending impersonation, avoiding incorrect logout behavior when the current request uses a different guard (e.g. Sanctum’s RequestGuard).
Changes:
- Store the impersonation guard name in session under
tenancy_impersonation_guardand use it to determine impersonation state. - Update
stopImpersonating()to log out via the stored guard (and add an optional$logoutparameter), and throw when called while not impersonating. - Extend/adjust tests to cover guard-aware logout behavior and optional “stop without logout”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Features/UserImpersonation.php |
Persists guard name in session; updates impersonation state detection and stop logic to log out the stored guard (plus optional logout + new exception behavior). |
tests/TenantUserImpersonationTest.php |
Updates session expectations and adds tests for optional logout and guard-isolated logout behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.UserImpersonation::stopImpersonating()now throws an exception if impersonation wasn't active at the point of calling the method.Summary by CodeRabbit
New Features
Tests