[4.x] Resolve testing todos (#1361)

* Test encrypted cookie identification

* Add Fortify bootstrapper custom query param passing test

* Correct Fortify route bootstrapper test (todo  refactor, convoluted)

* Clarify Fortify bootstrapper test

* Fix encrypted cookie identification test

* Move encrypted cookie assertion to "cookie identification works"

* Cover configured tenant model columns in cached resolver tests

* Refactor testing resolver with default vs custom tenant model name config

* Delete resolved todo

* Make code more concise

* Keep initial formatting (minimize diff noise)

* Make dataset/helper method parameter clearer

* Clarify fortify test

* Clarify assertions, improve comments

* Delete excessive comments, make existing comments consistent and clearer

* Make cached resolver test file clearer, update outdated comments

* Use the tenant model column term consistently

* FIx inconsistencies

* Provide more info in comment

* make comment more clear

* static property reset

---------

Co-authored-by: Samuel Štancl <samuel@archte.ch>

tests/Bootstrappers/FortifyRouteBootstrapperTest.php

@@ -1,6 +1,5 @@
 <?php
 
-use Stancl\Tenancy\Enums\Context;
 use Stancl\Tenancy\Tests\Etc\Tenant;
 use Illuminate\Support\Facades\Event;
 use Illuminate\Support\Facades\Route;
@@ -9,6 +8,8 @@ use Stancl\Tenancy\Events\TenancyInitialized;
 use Stancl\Tenancy\Listeners\BootstrapTenancy;
 use Stancl\Tenancy\Listeners\RevertToCentralContext;
 use Stancl\Tenancy\Bootstrappers\Integrations\FortifyRouteBootstrapper;
+use Stancl\Tenancy\Resolvers\RequestDataTenantResolver;
+use Stancl\Tenancy\Resolvers\PathTenantResolver;
 
 beforeEach(function () {
     Event::listen(TenancyInitialized::class, BootstrapTenancy::class);
@@ -26,16 +27,25 @@ afterEach(function () {
 test('fortify route tenancy bootstrapper updates fortify config correctly', function() {
     config(['tenancy.bootstrappers' => [FortifyRouteBootstrapper::class]]);
 
+    config([
+        // Parameter name (RequestDataTenantResolver::queryParameterName())
+        'tenancy.identification.resolvers.' . RequestDataTenantResolver::class . '.query_parameter' => 'team_query',
+        // Parameter value (RequestDataTenantResolver::payloadValue() gets the tenant model column value)
+        'tenancy.identification.resolvers.' . RequestDataTenantResolver::class . '.tenant_model_column' => 'company',
+    ]);
+
+    config([
+        // Parameter name (PathTenantResolver::tenantParameterName())
+        'tenancy.identification.resolvers.' . PathTenantResolver::class . '.tenant_parameter_name' => 'team_path',
+        // Parameter value (PathTenantResolver::tenantParameterValue() gets the tenant model column value)
+        'tenancy.identification.resolvers.' . PathTenantResolver::class . '.tenant_model_column' => 'name',
+    ]);
+
     $originalFortifyHome = config('fortify.home');
     $originalFortifyRedirects = config('fortify.redirects');
 
-    Route::get('/home', function () {
-        return true;
-    })->name($homeRouteName = 'home');
-
-    Route::get('/welcome', function () {
-        return true;
-    })->name($welcomeRouteName = 'welcome');
+    Route::get('/home', fn () => true)->name($homeRouteName = 'home');
+    Route::get('/welcome', fn () => true)->name($welcomeRouteName = 'welcome');
 
     FortifyRouteBootstrapper::$fortifyHome = $homeRouteName;
     FortifyRouteBootstrapper::$fortifyRedirectMap['login'] = $welcomeRouteName;
@@ -43,21 +53,53 @@ test('fortify route tenancy bootstrapper updates fortify config correctly', func
     expect(config('fortify.home'))->toBe($originalFortifyHome);
     expect(config('fortify.redirects'))->toBe($originalFortifyRedirects);
 
-    FortifyRouteBootstrapper::$passTenantParameter = true;
-    tenancy()->initialize($tenant = Tenant::create());
-    expect(config('fortify.home'))->toBe('http://localhost/home?tenant=' . $tenant->getTenantKey());
-    expect(config('fortify.redirects'))->toEqual(['login' => 'http://localhost/welcome?tenant=' . $tenant->getTenantKey()]);
+    /**
+     * When $passQueryParameter is true, the bootstrapper uses
+     * the RequestDataTenantResolver config for generating the Fortify URLs
+     * - tenant parameter is 'team_query'
+     * - parameter value is the tenant's company ('Acme')
+     *
+     * When $passQueryParameter is false, the bootstrapper uses
+     * the PathTenantResolver config for generating the Fortify URLs
+     * - tenant parameter is 'team_path'
+     * - parameter value is the tenant's name ('Foo')
+     */
+    $tenant = Tenant::create([
+        'company' => 'Acme',
+        'name' => 'Foo',
+    ]);
 
-    tenancy()->end();
-    expect(config('fortify.home'))->toBe($originalFortifyHome);
-    expect(config('fortify.redirects'))->toBe($originalFortifyRedirects);
+    // The bootstrapper generates and overrides the URLs in the Fortify config correctly
+    // (= the generated URLs have the correct tenant parameter + parameter value)
+    // The bootstrapper should use RequestDataTenantResolver while generating the URLs (default)
+    FortifyRouteBootstrapper::$passQueryParameter = true;
 
-    FortifyRouteBootstrapper::$passTenantParameter = false;
     tenancy()->initialize($tenant);
-    expect(config('fortify.home'))->toBe('http://localhost/home');
-    expect(config('fortify.redirects'))->toEqual(['login' => 'http://localhost/welcome']);
 
+    expect(config('fortify.home'))->toBe('http://localhost/home?team_query=Acme');
+    expect(config('fortify.redirects'))->toBe(['login' => 'http://localhost/welcome?team_query=Acme']);
+
+    // The bootstrapper restores the original Fortify config when ending tenancy
     tenancy()->end();
+
     expect(config('fortify.home'))->toBe($originalFortifyHome);
     expect(config('fortify.redirects'))->toBe($originalFortifyRedirects);
+
+    // The bootstrapper should use PathTenantResolver while generating the URLs now
+    FortifyRouteBootstrapper::$passQueryParameter = false;
+
+    tenancy()->initialize($tenant);
+
+    expect(config('fortify.home'))->toBe('http://localhost/home?team_path=Foo');
+    expect(config('fortify.redirects'))->toBe(['login' => 'http://localhost/welcome?team_path=Foo']);
+
+    tenancy()->end();
+
+    // The bootstrapper can override the home and redirects config without the tenant parameter being passed
+    FortifyRouteBootstrapper::$passTenantParameter = false;
+
+    tenancy()->initialize($tenant);
+
+    expect(config('fortify.home'))->toBe('http://localhost/home');
+    expect(config('fortify.redirects'))->toBe(['login' => 'http://localhost/welcome']);
 });

tests/CachedTenantResolverTest.php

@@ -17,59 +17,72 @@ use Stancl\Tenancy\Middleware\InitializeTenancyByPath;
 use Stancl\Tenancy\Resolvers\RequestDataTenantResolver;
 use function Stancl\Tenancy\Tests\pest;
 
-test('tenants can be resolved using cached resolvers', function (string $resolver) {
-    $tenant = Tenant::create(['id' => $tenantKey = 'acme']);
+beforeEach($cleanup = function () {
+    Tenant::$extraCustomColumns = [];
+});
 
-    $tenant->domains()->create(['domain' => $tenantKey]);
+afterEach($cleanup);
 
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+test('tenants can be resolved using cached resolvers', function (string $resolver, bool $configureTenantModelColumn) {
+    $tenant = Tenant::create([$tenantModelColumn = tenantModelColumn($configureTenantModelColumn) => 'acme']);
+
+    $tenant->createDomain($tenant->{$tenantModelColumn});
+
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
 })->with([
     DomainTenantResolver::class,
     PathTenantResolver::class,
     RequestDataTenantResolver::class,
+])->with([
+    'tenant model column is id (default)' => false,
+    'tenant model column is name (custom)' => true,
 ]);
 
-test('the underlying resolver is not touched when using the cached resolver', function (string $resolver) {
-    $tenant = Tenant::create(['id' => $tenantKey = 'acme']);
+test('the underlying resolver is not touched when using the cached resolver', function (string $resolver, bool $configureTenantModelColumn) {
+    $tenant = Tenant::create([$tenantModelColumn = tenantModelColumn($configureTenantModelColumn) => 'acme']);
 
-    $tenant->createDomain($tenantKey);
+    $tenant->createDomain($tenant->{$tenantModelColumn});
 
     DB::enableQueryLog();
 
     config(['tenancy.identification.resolvers.' . $resolver . '.cache' => false]);
 
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
     DB::flushQueryLog();
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
 
     pest()->assertNotEmpty(DB::getQueryLog()); // not empty
 
     config(['tenancy.identification.resolvers.' . $resolver . '.cache' => true]);
 
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
     DB::flushQueryLog();
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
     expect(DB::getQueryLog())->toBeEmpty(); // empty
 })->with([
     DomainTenantResolver::class,
     PathTenantResolver::class,
     RequestDataTenantResolver::class,
+])->with([
+    'tenant model column is id (default)' => false,
+    'tenant model column is name (custom)' => true,
 ]);
 
-test('cache is invalidated when the tenant is updated', function (string $resolver) {
-    $tenant = Tenant::create(['id' => $tenantKey = 'acme']);
-    $tenant->createDomain($tenantKey);
+test('cache is invalidated when the tenant is updated', function (string $resolver, bool $configureTenantModelColumn) {
+    $tenant = Tenant::create([$tenantModelColumn = tenantModelColumn($configureTenantModelColumn) => 'acme']);
+
+    $tenant->createDomain($tenant->{$tenantModelColumn});
 
     DB::enableQueryLog();
 
     config(['tenancy.identification.resolvers.' . $resolver . '.cache' => true]);
 
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
     expect(DB::getQueryLog())->not()->toBeEmpty();
 
     DB::flushQueryLog();
 
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
     expect(DB::getQueryLog())->toBeEmpty();
 
     // Tenant cache gets invalidated when the tenant is updated
@@ -77,41 +90,47 @@ test('cache is invalidated when the tenant is updated', function (string $resolv
 
     DB::flushQueryLog();
 
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
 
     expect(DB::getQueryLog())->not()->toBeEmpty(); // Cache was invalidated, so the tenant was retrieved from the DB
 })->with([
     DomainTenantResolver::class,
     PathTenantResolver::class,
     RequestDataTenantResolver::class,
+])->with([
+    'tenant model column is id (default)' => false,
+    'tenant model column is name (custom)' => true,
 ]);
 
-test('cache is invalidated when the tenant is deleted', function (string $resolver) {
+test('cache is invalidated when the tenant is deleted', function (string $resolver, bool $configureTenantModelColumn) {
     DB::statement('SET FOREIGN_KEY_CHECKS=0;'); // allow deleting the tenant
-    $tenant = Tenant::create(['id' => $tenantKey = 'acme']);
-    $tenant->createDomain($tenantKey);
+    $tenant = Tenant::create([$tenantModelColumn = tenantModelColumn($configureTenantModelColumn) => 'acme']);
+    $tenant->createDomain($tenant->{$tenantModelColumn});
 
     DB::enableQueryLog();
 
     config(['tenancy.identification.resolvers.' . $resolver . '.cache' => true]);
 
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
     expect(DB::getQueryLog())->not()->toBeEmpty();
 
     DB::flushQueryLog();
 
-    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn))))->toBeTrue();
     expect(DB::getQueryLog())->toBeEmpty();
 
     $tenant->delete();
     DB::flushQueryLog();
 
-    expect(fn () => app($resolver)->resolve(getResolverArgument($resolver, $tenantKey)))->toThrow(TenantCouldNotBeIdentifiedException::class);
+    expect(fn () => app($resolver)->resolve(getResolverArgument($resolver, $tenant, $tenantModelColumn)))->toThrow(TenantCouldNotBeIdentifiedException::class);
     expect(DB::getQueryLog())->not()->toBeEmpty(); // Cache was invalidated, so the DB was queried
 })->with([
     DomainTenantResolver::class,
     PathTenantResolver::class,
     RequestDataTenantResolver::class,
+])->with([
+    'tenant model column is id (default)' => false,
+    'tenant model column is name (custom)' => true,
 ]);
 
 test('cache is invalidated when a tenants domain is changed', function () {
@@ -307,23 +326,49 @@ test('PathTenantResolver properly separates cache for each tenant column', funct
     pest()->get("/x/bar/b")->assertSee('foo');
     expect(count(DB::getRawQueryLog()))->toBe(4); // unchanged
     expect(count($redisKeys()))->toBe(4); // unchanged
-
-    Tenant::$extraCustomColumns = []; // reset
 });
 
 /**
- * Return the argument for the resolver – tenant key, or a route instance with the tenant parameter.
+ * This method is used in generic tests to ensure that caching works correctly both with default and custom resolver config.
  *
- * PathTenantResolver uses a route instance with the tenant parameter as the argument,
- * unlike other resolvers which use a tenant key as the argument.
- *
- * This method is used in the tests where we test all the resolvers
- * to make getting the resolver arguments less repetitive (primarily because of the PathTenantResolver).
+ * If $configureTenantModelColumn is false, the tenant model column is 'id' (default) -- don't configure anything, keep the defaults.
+ * If $configureTenantModelColumn is true, the tenant model column should be 'name' (custom) -- configure tenant_model_column in the resolvers.
  */
-function getResolverArgument(string $resolver, string $tenantKey): string|Route
+function tenantModelColumn(bool $configureTenantModelColumn): string {
+    // Default tenant model column is 'id'
+    $tenantModelColumn = 'id';
+
+    if ($configureTenantModelColumn) {
+        // Use 'name' as the custom tenant model column
+        $tenantModelColumn = 'name';
+
+        Tenant::$extraCustomColumns = [$tenantModelColumn];
+
+        Schema::table('tenants', function (Blueprint $table) use ($tenantModelColumn) {
+            $table->string($tenantModelColumn)->unique();
+        });
+
+        config(['tenancy.identification.resolvers.' . PathTenantResolver::class . '.tenant_model_column' => $tenantModelColumn]);
+        config(['tenancy.identification.resolvers.' . RequestDataTenantResolver::class . '.tenant_model_column' => $tenantModelColumn]);
+    }
+
+    return $tenantModelColumn;
+}
+
+/**
+ * This method is used in generic tests where we test all the resolvers
+ * to make getting the resolver arguments less repetitive (primarily because of PathTenantResolver).
+ *
+ * For PathTenantResolver, return a route instance with the value retrieved using $tenant->{$parameterColumn} as the parameter.
+ * For RequestDataTenantResolver and DomainTenantResolver, return the value retrieved using $tenant->{$parameterColumn}.
+ *
+ * Tenant model column is 'id' by default, but in the generic tests,
+ * we also configure that to 'name' to ensure everything works both with default and custom config.
+ */
+function getResolverArgument(string $resolver, Tenant $tenant, string $parameterColumn = 'id'): string|Route
 {
-    // PathTenantResolver uses a route instance as the argument
     if ($resolver === PathTenantResolver::class) {
+        // PathTenantResolver uses a route instance as the argument
         $routeName = 'tenant-route';
 
         // Find or create a route instance for the resolver
@@ -332,17 +377,21 @@ function getResolverArgument(string $resolver, string $tenantKey): string|Route
             ->prefix('{tenant}')
             ->middleware(InitializeTenancyByPath::class);
 
-        // To make the tenant available on the route instance
-        // Make the 'tenant' route parameter the tenant key
-        // Setting the parameter on the $route->parameters property is required
-        // Because $route->setParameter() throws an exception when $route->parameters is not set yet
-        $route->parameters[PathTenantResolver::tenantParameterName()] = $tenantKey;
+        /**
+         * To make the tenant available on the route instance,
+         * set the 'tenant' route parameter to the tenant model column value ('id' or 'name').
+         *
+         * Setting the parameter on the $route->parameters property is required
+         * because $route->setParameter() throws an exception when $route->parameters isn't set yet.
+         */
+        $route->parameters['tenant'] = $tenant->{$parameterColumn};
 
-        // Return the route instance with the tenant key as the 'tenant' parameter
+        // Return the route instance with 'id' or 'name' as the 'tenant' parameter
         return $route;
     }
 
-    // Resolvers other than PathTenantResolver use the tenant key as the argument
-    // Return the tenant key
-    return $tenantKey;
+    // Assuming that:
+    // - with RequestDataTenantResolver, the tenant model column value is the payload value
+    // - with DomainTenantResolver, the tenant has a domain with name equal to the tenant model column value (see the createDomain() calls in various tests)
+    return $tenant->{$parameterColumn};
 }

tests/RequestDataIdentificationTest.php

@@ -9,6 +9,7 @@ use Stancl\Tenancy\Middleware\InitializeTenancyByRequestData;
 use Stancl\Tenancy\Resolvers\RequestDataTenantResolver;
 use Stancl\Tenancy\Tests\Etc\Tenant;
 use function Stancl\Tenancy\Tests\pest;
+use Illuminate\Cookie\CookieValuePrefix;
 
 beforeEach(function () {
     config([
@@ -88,6 +89,11 @@ test('cookie identification works', function (string|null $tenantModelColumn) {
     // Default cookie name
     $this->withoutExceptionHandling()->withUnencryptedCookie('tenant', $payload)->get('test')->assertSee($tenant->id);
 
+    // Manually encrypted cookie (encrypt the cookie exactly like MakesHttpRequests does in prepareCookiesForRequest())
+    $encryptedPayload = encrypt(CookieValuePrefix::create('tenant', app('encrypter')->getKey()) . $payload, false);
+
+    $this->withoutExceptionHandling()->withUnencryptedCookie('tenant', $encryptedPayload)->get('test')->assertSee($tenant->id);
+
     // Custom cookie name
     config(['tenancy.identification.resolvers.' . RequestDataTenantResolver::class . '.cookie' => 'custom_tenant_id']);
     $this->withoutExceptionHandling()->withUnencryptedCookie('custom_tenant_id', $payload)->get('test')->assertSee($tenant->id);
@@ -97,10 +103,7 @@ test('cookie identification works', function (string|null $tenantModelColumn) {
     expect(fn () => $this->withoutExceptionHandling()->withUnencryptedCookie('tenant', $payload)->get('test'))->toThrow(TenantCouldNotBeIdentifiedByRequestDataException::class);
 })->with([null, 'slug']);
 
-// todo@tests encrypted cookie
-
 test('an exception is thrown when no tenant data is provided in the request', function () {
     pest()->expectException(TenantCouldNotBeIdentifiedByRequestDataException::class);
     $this->withoutExceptionHandling()->get('test');
 });
-