From 8d38f42cd0d4e0aec811cbf782c1e7a083e784d0 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Thu, 31 Aug 2023 15:44:26 +0200 Subject: [PATCH] Correct asset helpers, make asset helpers work with path identification (#6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Make asset_helper_tenancy false by default * Make tenant_asset() respect ASSET_URL * Set asset helper tenancy to true in tests where needed * If the `asset_helper_tenancy` key is missing, default to false in filesystem bootstrapper * Make temporary clone action changes * Make tenancy asset route universal * Make the asset controller's asset method behave differently if path ID MW is the default * Test that asset helper works with path identification * Fix code style (php-cs-fixer) * Delete path traversal attack prevention * Fix code style (php-cs-fixer) * Skip cloning of stancl.tenancy.asset route in some tests * Fix code style (php-cs-fixer) * Clone asset route in TSP stub * Add cloning only the passed route * Clone asset route in tenant asset test beforeEach * Skip asset route cloning by default * Fix typo * Change public method back to protected * Remove cloning of specific routes, skip cloning routes flagged as tenant * Delete constructor from asset controiler, change asset method to invoke * Update asset route registration, add prefixed asest route for path identification * Use default middleware from config instead of `tenancy()->defaultMiddleware()` * Delete old code from TSP stub * Revert TSP stub change * Revert FilesystemTenancyBootstrapper changes * Suffix asset url in tenant_asset() * Simplify `tenant_asset()` * Ensure the base asset url is always suffixed with '/' * remove unnecessary ?? false --------- Co-authored-by: PHP CS Fixer Co-authored-by: Samuel Štancl --- assets/TenancyServiceProvider.stub.php | 1 + assets/config.php | 6 +-- assets/routes.php | 10 +++- src/Actions/CloneRoutesAsTenant.php | 10 +++- .../FilesystemTenancyBootstrapper.php | 2 +- src/Controllers/TenantAssetController.php | 8 +-- src/helpers.php | 11 +++- tests/TenantAssetTest.php | 52 ++++++++++++++++++- 8 files changed, 82 insertions(+), 18 deletions(-) diff --git a/assets/TenancyServiceProvider.stub.php b/assets/TenancyServiceProvider.stub.php index 7dee869d..5bd85ce0 100644 --- a/assets/TenancyServiceProvider.stub.php +++ b/assets/TenancyServiceProvider.stub.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Providers; +use Illuminate\Routing\Route; use Stancl\Tenancy\Jobs; use Stancl\Tenancy\Events; use Stancl\Tenancy\Listeners; diff --git a/assets/config.php b/assets/config.php index 35fee7f0..3dfea4fe 100644 --- a/assets/config.php +++ b/assets/config.php @@ -272,13 +272,13 @@ return [ 'suffix_storage_path' => true, /** - * By default, asset() calls are made multi-tenant too. You can use global_asset() and mix() + * Setting this to true makes asset() calls multi-tenant. You can use global_asset() and mix() * for global, non-tenant-specific assets. However, you might have some issues when using * packages that use asset() calls inside the tenant app. To avoid such issues, you can - * disable asset() helper tenancy and explicitly use tenant_asset() calls in places + * leave asset() helper tenancy disabled and explicitly use tenant_asset() calls in places * where you want to use tenant-specific assets (product images, avatars, etc). */ - 'asset_helper_tenancy' => true, + 'asset_helper_tenancy' => false, ], /** diff --git a/assets/routes.php b/assets/routes.php index a9c09797..52fc071b 100644 --- a/assets/routes.php +++ b/assets/routes.php @@ -3,9 +3,15 @@ declare(strict_types=1); use Illuminate\Support\Facades\Route; +use Stancl\Tenancy\PathIdentificationManager; use Stancl\Tenancy\Controllers\TenantAssetController; -// todo make this work with path identification -Route::get('/tenancy/assets/{path?}', [TenantAssetController::class, 'asset']) +Route::get('/tenancy/assets/{path?}', TenantAssetController::class) ->where('path', '(.*)') + ->middleware(config('tenancy.identification.default_middleware')) // todo@features Use tenancy()->defaultMiddleware() after merging #1021 ->name('stancl.tenancy.asset'); + +Route::prefix('/{' . PathIdentificationManager::getTenantParameterName() . '}/')->get('/tenancy/assets/{path?}', TenantAssetController::class) + ->where('path', '(.*)') + ->middleware('tenant') + ->name(PathIdentificationManager::getTenantRouteNamePrefix() . 'stancl.tenancy.asset'); diff --git a/src/Actions/CloneRoutesAsTenant.php b/src/Actions/CloneRoutesAsTenant.php index 30313a40..97ad817c 100644 --- a/src/Actions/CloneRoutesAsTenant.php +++ b/src/Actions/CloneRoutesAsTenant.php @@ -37,7 +37,9 @@ use Stancl\Tenancy\PathIdentificationManager; class CloneRoutesAsTenant { protected array $cloneRouteUsing = []; - protected array $skippedRoutes = []; + protected array $skippedRoutes = [ + 'stancl.tenancy.asset', + ]; public function __construct( protected Router $router, @@ -87,7 +89,11 @@ class CloneRoutesAsTenant * universal routes will be available in both contexts. */ return collect($this->router->getRoutes()->get())->filter(function (Route $route) use ($tenantParameterName) { - if (in_array($tenantParameterName, $route->parameterNames(), true) || in_array($route->getName(), $this->skippedRoutes, true)) { + if ( + tenancy()->routeHasMiddleware($route, 'tenant') || + in_array($route->getName(), $this->skippedRoutes, true) || + in_array($tenantParameterName, $route->parameterNames(), true) + ) { return false; } diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index 2dbba369..b64ff9ed 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -45,7 +45,7 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper } // asset() - if ($this->app['config']['tenancy.filesystem.asset_helper_tenancy'] ?? true) { + if ($this->app['config']['tenancy.filesystem.asset_helper_tenancy']) { if ($this->originalPaths['asset_url']) { $this->app['config']['app.asset_url'] = $this->originalPaths['asset_url'] . "/$suffix"; $this->app['url']->setAssetRoot($this->app['config']['app.asset_url']); diff --git a/src/Controllers/TenantAssetController.php b/src/Controllers/TenantAssetController.php index 7a95dffe..198292bb 100644 --- a/src/Controllers/TenantAssetController.php +++ b/src/Controllers/TenantAssetController.php @@ -5,21 +5,15 @@ declare(strict_types=1); namespace Stancl\Tenancy\Controllers; use Illuminate\Routing\Controller; -use Stancl\Tenancy\Tenancy; use Symfony\Component\HttpFoundation\BinaryFileResponse; use Throwable; class TenantAssetController extends Controller // todo@docs this was renamed from TenantAssetsController { - public function __construct() - { - $this->middleware(Tenancy::defaultMiddleware()); - } - /** * @throws \Symfony\Component\HttpKernel\Exception\NotFoundHttpException */ - public function asset(string $path = null): BinaryFileResponse + public function __invoke(string $path = null): BinaryFileResponse { abort_if($path === null, 404); diff --git a/src/helpers.php b/src/helpers.php index c50da940..d38e6e53 100644 --- a/src/helpers.php +++ b/src/helpers.php @@ -35,9 +35,18 @@ if (! function_exists('tenant')) { if (! function_exists('tenant_asset')) { // todo docblock - // todo add an option to generate paths respecting the ASSET_URL function tenant_asset(string|null $asset): string { + if ($assetUrl = config('app.asset_url')) { + $assetUrl = str($assetUrl)->rtrim('/')->append('/'); + + if (tenant()) { + $assetUrl .= config('tenancy.filesystem.suffix_base') . tenant()->getTenantKey(); + } + + return $assetUrl . $asset; + } + return route('stancl.tenancy.asset', ['path' => $asset]); } } diff --git a/tests/TenantAssetTest.php b/tests/TenantAssetTest.php index a1cd0f5b..94e733b8 100644 --- a/tests/TenantAssetTest.php +++ b/tests/TenantAssetTest.php @@ -2,20 +2,31 @@ declare(strict_types=1); +use Stancl\Tenancy\Tests\Etc\Tenant; +use Illuminate\Contracts\Http\Kernel; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Route; use Illuminate\Support\Facades\Storage; -use Stancl\Tenancy\Bootstrappers\FilesystemTenancyBootstrapper; +use Stancl\Tenancy\Actions\CloneRoutesAsTenant; use Stancl\Tenancy\Events\TenancyInitialized; use Stancl\Tenancy\Listeners\BootstrapTenancy; +use Stancl\Tenancy\Middleware\InitializeTenancyByPath; use Stancl\Tenancy\Middleware\InitializeTenancyByRequestData; -use Stancl\Tenancy\Tests\Etc\Tenant; +use Stancl\Tenancy\Bootstrappers\FilesystemTenancyBootstrapper; +use Stancl\Tenancy\Bootstrappers\UrlBindingBootstrapper; +use Stancl\Tenancy\Overrides\TenancyUrlGenerator; beforeEach(function () { config(['tenancy.bootstrappers' => [ FilesystemTenancyBootstrapper::class, ]]); + TenancyUrlGenerator::$prefixRouteNames = false; + + /** @var CloneRoutesAsTenant $cloneAction */ + $cloneAction = app(CloneRoutesAsTenant::class); + $cloneAction->handle(Route::getRoutes()->getByName('stancl.tenancy.asset')); + Event::listen(TenancyInitialized::class, BootstrapTenancy::class); }); @@ -47,6 +58,7 @@ test('asset can be accessed using the url returned by the tenant asset helper', test('asset helper returns a link to tenant asset controller when asset url is null', function () { config(['app.asset_url' => null]); + config(['tenancy.filesystem.asset_helper_tenancy' => true]); $tenant = Tenant::create(); tenancy()->initialize($tenant); @@ -56,6 +68,7 @@ test('asset helper returns a link to tenant asset controller when asset url is n test('asset helper returns a link to an external url when asset url is not null', function () { config(['app.asset_url' => 'https://an-s3-bucket']); + config(['tenancy.filesystem.asset_helper_tenancy' => true]); $tenant = Tenant::create(); tenancy()->initialize($tenant); @@ -63,6 +76,41 @@ test('asset helper returns a link to an external url when asset url is not null' expect(asset('foo'))->toBe("https://an-s3-bucket/tenant{$tenant->id}/foo"); }); +test('asset helper works correctly with path identification', function (bool $kernelIdentification) { + TenancyUrlGenerator::$prefixRouteNames = true; + config(['tenancy.filesystem.asset_helper_tenancy' => true]); + config(['tenancy.identification.default_middleware' => InitializeTenancyByPath::class]); + config(['tenancy.bootstrappers' => array_merge([UrlBindingBootstrapper::class], config('tenancy.bootstrappers'))]); + + $tenantAssetRoute = Route::prefix('{tenant}')->get('/tenant_helper', function () { + return tenant_asset('foo'); + })->name('tenant.helper.tenant'); + + $assetRoute = Route::prefix('{tenant}')->get('/asset_helper', function () { + return asset('foo'); + })->name('tenant.helper.asset'); + + if ($kernelIdentification) { + app(Kernel::class)->pushMiddleware(InitializeTenancyByPath::class); + } else { + $assetRoute->middleware(InitializeTenancyByPath::class); + $tenantAssetRoute->middleware(InitializeTenancyByPath::class); + } + + /** @var CloneRoutesAsTenant $cloneAction */ + $cloneAction = app(CloneRoutesAsTenant::class); + + $cloneAction->handle(); + + tenancy()->initialize(Tenant::create()); + + expect(pest()->get(route('tenant.helper.asset'))->getContent())->toBe(route('stancl.tenancy.asset', ['path' => 'foo'])); + expect(pest()->get(route('tenant.helper.tenant'))->getContent())->toBe(route('stancl.tenancy.asset', ['path' => 'foo'])); +})->with([ + 'kernel identification' => true, + 'route-level identification' => false, +]); + test('global asset helper returns the same url regardless of tenancy initialization', function () { $original = global_asset('foobar'); expect(global_asset('foobar'))->toBe(asset('foobar'));