From f308e2f84d00565e4846ad048d8df11d03f172fd Mon Sep 17 00:00:00 2001 From: lukinovec Date: Sun, 3 Aug 2025 23:21:03 +0200 Subject: [PATCH] [4.x] Resolve testing todos (#1361) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- .../Integrations/FortifyRouteBootstrapper.php | 1 - src/Resolvers/RequestDataTenantResolver.php | 1 - .../FortifyRouteBootstrapperTest.php | 78 +++++++--- tests/CachedTenantResolverTest.php | 133 ++++++++++++------ tests/RequestDataIdentificationTest.php | 9 +- 5 files changed, 157 insertions(+), 65 deletions(-) diff --git a/src/Bootstrappers/Integrations/FortifyRouteBootstrapper.php b/src/Bootstrappers/Integrations/FortifyRouteBootstrapper.php index fb371d6a..12b49f78 100644 --- a/src/Bootstrappers/Integrations/FortifyRouteBootstrapper.php +++ b/src/Bootstrappers/Integrations/FortifyRouteBootstrapper.php @@ -86,7 +86,6 @@ class FortifyRouteBootstrapper implements TenancyBootstrapper protected function useTenantRoutesInFortify(Tenant $tenant): void { if (static::$passQueryParameter) { - // todo@tests $tenantParameterName = RequestDataTenantResolver::queryParameterName(); $tenantParameterValue = RequestDataTenantResolver::payloadValue($tenant); } else { diff --git a/src/Resolvers/RequestDataTenantResolver.php b/src/Resolvers/RequestDataTenantResolver.php index 4d8b3277..48ee45ee 100644 --- a/src/Resolvers/RequestDataTenantResolver.php +++ b/src/Resolvers/RequestDataTenantResolver.php @@ -31,7 +31,6 @@ class RequestDataTenantResolver extends Contracts\CachedTenantResolver public function getPossibleCacheKeys(Tenant&Model $tenant): array { - // todo@tests return [ $this->formatCacheKey(static::payloadValue($tenant)), ]; diff --git a/tests/Bootstrappers/FortifyRouteBootstrapperTest.php b/tests/Bootstrappers/FortifyRouteBootstrapperTest.php index 63f0f2a0..bfc835ee 100644 --- a/tests/Bootstrappers/FortifyRouteBootstrapperTest.php +++ b/tests/Bootstrappers/FortifyRouteBootstrapperTest.php @@ -1,6 +1,5 @@ [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']); }); diff --git a/tests/CachedTenantResolverTest.php b/tests/CachedTenantResolverTest.php index 558fb345..322a1961 100644 --- a/tests/CachedTenantResolverTest.php +++ b/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}; } diff --git a/tests/RequestDataIdentificationTest.php b/tests/RequestDataIdentificationTest.php index 0f635bf1..63940ed5 100644 --- a/tests/RequestDataIdentificationTest.php +++ b/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'); }); -