Correct asset helpers, make asset helpers work with path identification (#6)

* 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 <phpcsfixer@example.com>
Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com>

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;

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,
     ],
 
     /**

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');

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;
             }
 

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']);

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);
 

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]);
     }
 }

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'));