[MINOR BC] UserImpersonation: store auth guard in session, add $logout param to stopImpersonating() (#1437)

> Minor breaking change: `session('tenancy_impersonating')` doesn't work
anymore. Use `session('tenancy_impersonation_guard')` instead.

The 'tenancy_impersonating' session variable got replaced by
'tenancy_impersonation_guard'. `UserImpersonation::stopImpersonating()`
now calls `logout()` on the guard retrieved by
`session()->get('tenancy_impersonation_guard')` instead of calling
`logout()` on the _current_ auth guard. Now. if you create the
impersonation token with guard 'web', and call
`UserImpersonation::stopImpersonating()`, for example in a route that
has the `auth:sanctum` middleware (= the current guard in that route
would be `RequestGuard` which doesn't even have the `logout()` 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 `$logout`
parameter, which is `true` by default. If `false` is passed, the method
just forgets `tenancy_impersonation_guard` from session without logging
out.

`UserImpersonation::stopImpersonating()` now throws an exception if
impersonation wasn't active at the point of calling the method.

---------

Co-authored-by: Samuel Stancl <samuel@archte.ch>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

src/Features/UserImpersonation.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace Stancl\Tenancy\Features;
 
+use Exception;
 use Illuminate\Database\Eloquent\Model;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Support\Facades\Auth;
@@ -61,9 +62,9 @@ class UserImpersonation implements Feature
 
         Auth::guard($token->auth_guard)->loginUsingId($token->user_id, $token->remember);
 
-        $token->delete();
+        session()->put('tenancy_impersonation_guard', $token->auth_guard);
 
-        session()->put('tenancy_impersonating', true);
+        $token->delete();
 
         return redirect($token->redirect_url);
     }
@@ -76,16 +77,30 @@ class UserImpersonation implements Feature
 
     public static function isImpersonating(): bool
     {
-        return session()->has('tenancy_impersonating');
+        return session()->has('tenancy_impersonation_guard');
     }
 
     /**
-     * Logout from the current domain and forget impersonation session.
+     * Stop user impersonation by forgetting the impersonation session.
+     *
+     * When $logout is true, the user will also be logged out
+     * from the impersonation guard stored in the session.
+     *
+     * Throws an exception if impersonation is not active
+     * (= the impersonation guard is not in the session).
      */
-    public static function stopImpersonating(): void
+    public static function stopImpersonating(bool $logout = true): void
     {
-        auth()->logout();
+        if (! static::isImpersonating()) {
+            throw new Exception('Not currently impersonating any user.');
+        }
 
-        session()->forget('tenancy_impersonating');
+        if ($logout) {
+            $guard = session()->get('tenancy_impersonation_guard');
+
+            auth($guard)->logout();
+        }
+
+        session()->forget('tenancy_impersonation_guard');
     }
 }

tests/TenantUserImpersonationTest.php

@@ -89,13 +89,14 @@ test('tenant user can be impersonated on a tenant domain', function () {
         ->assertSee('You are logged in as Joe');
 
     expect(UserImpersonation::isImpersonating())->toBeTrue();
-    expect(session('tenancy_impersonating'))->toBeTrue();
+    expect(session('tenancy_impersonation_guard'))->toBe('web');
+    expect($token->auth_guard)->toBe('web');
 
     // Leave impersonation
     UserImpersonation::stopImpersonating();
 
     expect(UserImpersonation::isImpersonating())->toBeFalse();
-    expect(session('tenancy_impersonating'))->toBeNull();
+    expect(session('tenancy_impersonation_guard'))->toBeNull();
 
     // Assert can't access the tenant dashboard
     pest()->get('http://foo.localhost/dashboard')
@@ -135,19 +136,113 @@ test('tenant user can be impersonated on a tenant path', function () {
         ->assertSee('You are logged in as Joe');
 
     expect(UserImpersonation::isImpersonating())->toBeTrue();
-    expect(session('tenancy_impersonating'))->toBeTrue();
+    expect(session('tenancy_impersonation_guard'))->toBe('web');
+    expect($token->auth_guard)->toBe('web');
 
     // Leave impersonation
     UserImpersonation::stopImpersonating();
 
     expect(UserImpersonation::isImpersonating())->toBeFalse();
-    expect(session('tenancy_impersonating'))->toBeNull();
+    expect(session('tenancy_impersonation_guard'))->toBeNull();
 
     // Assert can't access the tenant dashboard
     pest()->get('/acme/dashboard')
         ->assertRedirect('/login');
 });
 
+test('stopImpersonating can keep the user authenticated', function () {
+    makeLoginRoute();
+
+    Route::middleware(InitializeTenancyByPath::class)->prefix('/{tenant}')->group(getRoutes(false));
+
+    $tenant = Tenant::create([
+        'id' => 'acme',
+        'tenancy_db_name' => 'db' . Str::random(16),
+    ]);
+
+    migrateTenants();
+
+    $user = $tenant->run(function () {
+        return ImpersonationUser::create([
+            'name' => 'Joe',
+            'email' => 'joe@local',
+            'password' => bcrypt('secret'),
+        ]);
+    });
+
+    // Impersonate the user
+    $token = tenancy()->impersonate($tenant, $user->id, '/acme/dashboard');
+
+    pest()->get('/acme/impersonate/' . $token->token)
+        ->assertRedirect('/acme/dashboard');
+
+    expect(UserImpersonation::isImpersonating())->toBeTrue();
+
+    // Stop impersonating without logging out
+    UserImpersonation::stopImpersonating(false);
+
+    // The impersonation session key should be cleared
+    expect(UserImpersonation::isImpersonating())->toBeFalse();
+    expect(session('tenancy_impersonation_guard'))->toBeNull();
+
+    // The user should still be authenticated
+    pest()->get('/acme/dashboard')
+        ->assertSuccessful()
+        ->assertSee('You are logged in as Joe');
+});
+
+test('stopImpersonating logs out the user from the impersonation guard stored in session', function () {
+    Route::middleware(InitializeTenancyByPath::class)->prefix('/{tenant}')->group(getRoutes(false));
+
+    $tenant = Tenant::create([
+        'id' => 'acme',
+        'tenancy_db_name' => 'db' . Str::random(16),
+    ]);
+
+    migrateTenants();
+
+    $user = $tenant->run(function () {
+        return ImpersonationUser::create([
+            'name' => 'Joe',
+            'email' => 'joe@local',
+            'password' => bcrypt('secret'),
+        ]);
+    });
+
+    // Impersonate the user
+    $token = tenancy()->impersonate($tenant, $user->id, '/acme/dashboard');
+
+    pest()->get('/acme/impersonate/' . $token->token)
+        ->assertRedirect('/acme/dashboard');
+
+    expect(session('tenancy_impersonation_guard'))->toBe('web');
+
+    // Impersonation logged in the user using the current guard ('web')
+    expect(auth('web')->check())->toBeTrue();
+
+    config(['auth.guards.test' => [
+        'driver' => 'session',
+        'provider' => 'users',
+    ]]);
+
+    // Manually log the user in through the 'test' guard
+    auth('test')->loginUsingId($user->id);
+
+    // Should log the user out from the guard used for impersonation ('web')
+    UserImpersonation::stopImpersonating();
+
+    expect(auth('web')->check())->toBeFalse();
+    expect(auth('test')->check())->toBeTrue();
+
+    expect(UserImpersonation::isImpersonating())->toBeFalse();
+
+    // tenancy_impersonation_guard isn't in the session anymore,
+    // stopImpersonating should throw an exception instead of logging out
+    expect(fn() => UserImpersonation::stopImpersonating())->toThrow(Exception::class);
+
+    expect(auth('test')->check())->toBeTrue();
+});
+
 test('tokens have a limited ttl', function () {
     Route::middleware(InitializeTenancyByDomain::class)->group(getRoutes());