Merge 3e564c1f22 into e0990a438c

src/Bootstrappers/FilesystemTenancyBootstrapper.php

@@ -14,6 +14,8 @@ use Stancl\Tenancy\Contracts\Tenant;
 class FilesystemTenancyBootstrapper implements TenancyBootstrapper
 {
     public array $originalDisks = [];
+    protected array $originalCachePaths = [];
+    protected array $originalCacheLockPaths = [];
     public string|null $originalAssetUrl;
     public string $originalStoragePath;
 
@@ -191,32 +193,63 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper
             return;
         }
 
-        $storagePath = $suffix
-            ? $this->tenantStoragePath($suffix)
-            : $this->originalStoragePath;
-
-        $stores = array_filter($this->app['config']['tenancy.cache.stores'], function ($name) {
-            $store = $this->app['config']["cache.stores.{$name}"];
-
-            if ($store === null) {
-                return false;
-            }
-
-            return $store['driver'] === 'file';
-        });
+        // On revert, restore exactly the stores captured during bootstrap -- not the current
+        // (possibly mutated) tenancy.cache.stores. Otherwise, removing a store from that
+        // config in tenant context would make revert() skip it (so the store would be stuck with a tenant-scoped path).
+        $stores = $suffix !== false
+            ? $this->app['config']['tenancy.cache.stores']
+            : array_keys($this->originalCachePaths);
 
         foreach ($stores as $name) {
-            $path = $storagePath . '/framework/cache/data';
+            $store = $this->app['config']["cache.stores.{$name}"];
+
+            // Only file stores have a path to scope. Skip stores that don't exist (null) or use another driver.
+            if ($store === null || $store['driver'] !== 'file') {
+                continue;
+            }
+
+            if ($suffix !== false && ! isset($this->originalCachePaths[$name])) {
+                $this->originalCachePaths[$name] = $store['path'];
+                $this->originalCacheLockPaths[$name] = $store['lock_path'] ?? null;
+            }
+
+            $path = $suffix ? $this->tenantCachePath($this->originalCachePaths[$name], $suffix) : $this->originalCachePaths[$name];
+
+            // Unlike path, lock_path is optional -- if it's not set, FileStore::lock() falls back to path
+            // itself (see `$this->lockDirectory ?? $this->directory` in FileStore). Leave it null here rather
+            // than hardcoding it to $path ourselves, so a store that didn't configure a separate lock_path
+            // doesn't end up with one.
+            $lockPath = $this->originalCacheLockPaths[$name];
+            if ($suffix && $lockPath !== null) {
+                $lockPath = $this->tenantCachePath($lockPath, $suffix);
+            }
+
             $this->app['config']["cache.stores.{$name}.path"] = $path;
-            $this->app['config']["cache.stores.{$name}.lock_path"] = $path;
+            $this->app['config']["cache.stores.{$name}.lock_path"] = $lockPath;
 
             /** @var \Illuminate\Cache\FileStore $store */
             $store = $this->app['cache']->store($name)->getStore();
             $store->setDirectory($path);
-            $store->setLockDirectory($path);
+            $store->setLockDirectory($lockPath);
         }
     }
 
+    /** Scope a configured cache path (path or lock_path) to the tenant identified by $suffix. */
+    protected function tenantCachePath(string $configuredPath, string $suffix): string
+    {
+        if (str_starts_with($configuredPath, $this->originalStoragePath . '/')) {
+            // Swap the central storage path prefix for the tenant's.
+            // For example, storage_path('framework/cache/data') becomes storage_path('tenant1/framework/cache/data').
+            return str($configuredPath)
+                ->after($this->originalStoragePath . '/')
+                ->prepend($this->tenantStoragePath($suffix) . '/')
+                ->toString();
+        }
+
+        // Otherwise $configuredPath isn't necessarily storage_path()-based, so just append the suffix directly.
+        return rtrim($configuredPath, '/') . '/' . $suffix;
+    }
+
     public function scopeSessions(string|false $suffix): void
     {
         if (! $this->app['config']['tenancy.filesystem.scope_sessions']) {

tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php

@@ -1,6 +1,7 @@
 <?php
 
 use Stancl\JobPipeline\JobPipeline;
+use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\File;
 use Stancl\Tenancy\Tests\Etc\Tenant;
 use Illuminate\Support\Facades\Event;
@@ -298,3 +299,313 @@ test('scoped disks are scoped per tenant', function () {
     expect(file_get_contents(storage_path() . "/app/public/scoped_disk_prefix/foo.txt"))->toBe('central2');
     expect(file_get_contents(storage_path() . "/tenant{$tenant->id}/app/public/scoped_disk_prefix/foo.txt"))->toBe('tenant');
 });
+
+test('file cache stores are separated per tenant', function () {
+    config([
+        'tenancy.bootstrappers' => [
+            FilesystemTenancyBootstrapper::class,
+        ],
+        // Only stores that use the 'file' driver are scoped.
+        // The 'redis' store won't be scoped since it doesn't use the 'file' driver,
+        // and 'nonexistent_store' won't be scoped since it just doesn't exist.
+        'tenancy.cache.stores' => ['file', 'redis', 'nonexistent_store'],
+        // Laravel's default 'file' store config (set explicitly here for clarity)
+        'cache.stores.file' => [
+            'driver' => 'file',
+            'path' => storage_path('framework/cache/data'),
+            'lock_path' => storage_path('framework/cache/data'),
+        ],
+    ]);
+
+    $tenant1 = Tenant::create();
+    $tenant2 = Tenant::create();
+
+    Cache::store('file')->put('key', 'central');
+    Cache::store('redis')->put('key', 'central');
+
+    // 'redis' and 'nonexistent_store' are skipped by the driver check in scopeCache(), before it reads their path.
+    // Without the skipping logic, the `$this->originalCachePaths[$name] = $store['path']`
+    // line in scopeCache() would throw an ErrorException (with 'redis', we'd get an
+    // 'Undefined array key "path"' exception, and with 'nonexistent_store',
+    // we'd get 'Trying to access array offset on null').
+    expect(fn () => tenancy()->initialize($tenant1))->not()->toThrow(ErrorException::class);
+
+    expect(Cache::store('file')->get('key'))->toBeNull();
+    Cache::store('file')->put('key', 'tenant1');
+
+    // The 'redis' store doesn't use the file driver (no path to scope),
+    // so FilesystemTenancyBootstrapper skips it in scopeCache().
+    // The central value is retained in the tenant context.
+    expect(Cache::store('redis')->get('key'))->toBe('central');
+
+    tenancy()->initialize($tenant2);
+
+    expect(Cache::store('file')->get('key'))->toBeNull();
+    Cache::store('file')->put('key', 'tenant2');
+
+    tenancy()->initialize($tenant1);
+
+    expect(Cache::store('file')->get('key'))->toBe('tenant1');
+
+    tenancy()->end();
+
+    expect(Cache::store('file')->get('key'))->toBe('central');
+
+    // Turn FilesystemTenancyBootstrapper's cache scoping off
+    config(['tenancy.filesystem.scope_cache' => false]);
+
+    tenancy()->initialize($tenant1);
+
+    expect(Cache::store('file')->get('key'))->toBe('central');
+    Cache::store('file')->put('key', 'written in tenant context');
+
+    tenancy()->end();
+
+    expect(Cache::store('file')->get('key'))->toBe('written in tenant context');
+});
+
+test('file cache stores get their path scoped on bootstrap and restored back on revert', function () {
+    $fooPath = storage_path('framework/cache/foo_file');
+    $barPath = storage_path('framework/cache/bar_file');
+    File::deleteDirectory($fooPath);
+    File::deleteDirectory($barPath);
+
+    // Use separate 'foo_file'/'bar_file' stores rather than reconfiguring 'file'.
+    // TestCase::setUp() calls `cache:clear file`, which resolves the 'file' store and leaves it
+    // in the CacheManager with the default path. A later config() call can't mutate that,
+    // so in central context, cache would be written to 'storage/framework/cache/data' no matter how the config changes.
+    // The same applies to the other tests below that configure separate cache stores rather than using 'file'.
+    config([
+        'tenancy.bootstrappers' => [
+            FilesystemTenancyBootstrapper::class,
+        ],
+        'tenancy.cache.stores' => ['foo_file', 'bar_file'],
+        'cache.stores.foo_file' => [
+            'driver' => 'file',
+            'path' => $fooPath,
+        ],
+        'cache.stores.bar_file' => [
+            'driver' => 'file',
+            'path' => $barPath,
+        ],
+    ]);
+
+    Cache::store('foo_file')->put('key', 'central foo');
+    Cache::store('bar_file')->put('key', 'central bar');
+
+    tenancy()->initialize(Tenant::create());
+
+    Cache::store('foo_file')->put('key', 'tenant foo');
+    Cache::store('bar_file')->put('key', 'tenant bar');
+
+    // Each store uses its own scoped path.
+    // The stores don't read or overwrite each other's entries.
+    expect(Cache::store('foo_file')->get('key'))->toBe('tenant foo');
+    expect(Cache::store('bar_file')->get('key'))->toBe('tenant bar');
+
+    Cache::store('bar_file')->flush();
+
+    // Only bar_file was flushed
+    expect(Cache::store('bar_file')->get('key'))->toBeNull();
+    expect(Cache::store('foo_file')->get('key'))->toBe('tenant foo');
+
+    tenancy()->end();
+
+    // revert() points each store back at its original configured path
+    expect(Cache::store('foo_file')->get('key'))->toBe('central foo');
+    expect(Cache::store('bar_file')->get('key'))->toBe('central bar');
+
+    File::deleteDirectory($fooPath);
+    File::deleteDirectory($barPath);
+});
+
+test('scopeCache ignores changes to tenancy.cache.stores made in tenant context', function () {
+    $fooPath = storage_path('framework/cache/foo_file');
+    $barPath = storage_path('framework/cache/bar_file');
+    File::deleteDirectory($fooPath);
+    File::deleteDirectory($barPath);
+
+    config([
+        'tenancy.bootstrappers' => [
+            FilesystemTenancyBootstrapper::class,
+        ],
+        'cache.stores.foo_file' => [
+            'driver' => 'file',
+            'path' => $fooPath,
+        ],
+        'cache.stores.bar_file' => [
+            'driver' => 'file',
+            'path' => $barPath,
+        ],
+        // Only foo_file is scoped during bootstrap()
+        'tenancy.cache.stores' => ['foo_file'],
+    ]);
+
+    Cache::store('foo_file')->put('key', 'central');
+    Cache::store('bar_file')->put('key', 'central');
+
+    tenancy()->initialize(Tenant::create());
+
+    Cache::store('foo_file')->put('key', 'tenant');
+
+    // Mutate tenancy.cache.stores in tenant context (remove 'foo_file', add 'bar_file')
+    config(['tenancy.cache.stores' => ['bar_file']]);
+    // 'bar_file' wasn't in tenancy.cache.stores during bootstrap.
+    // No original path is captured for that store, so it continues to use its central path.
+    expect(Cache::store('bar_file')->get('key'))->toBe('central');
+    Cache::store('bar_file')->put('key', 'written in tenant context');
+
+    tenancy()->end();
+
+    // revert() restores the paths for the stores that were scoped during bootstrap
+    expect(Cache::store('foo_file')->get('key'))->toBe('central');
+    // 'bar_file' was never scoped, it still reads from the same path it was writing to in tenant context
+    expect(Cache::store('bar_file')->get('key'))->toBe('written in tenant context');
+
+    File::deleteDirectory($fooPath);
+    File::deleteDirectory($barPath);
+});
+
+test('a configured lock_path is scoped separately from path', function () {
+    // A store can configure path and lock_path as two different directories
+    $centralStoragePath = storage_path();
+    $path = "{$centralStoragePath}/framework/cache/foo";
+    $lockPath = "{$centralStoragePath}/framework/cache/foo_locks";
+
+    // Delete the directories before running assertions, not just after.
+    // Cache::store('foo_file')->lock(...) below defaults to a lock that never expires,
+    // and this test never releases it, so a stale lock left over from
+    // a previous run would make lock() fail forever.
+    File::deleteDirectory($path);
+    File::deleteDirectory($lockPath);
+
+    config([
+        'tenancy.bootstrappers' => [
+            FilesystemTenancyBootstrapper::class,
+        ],
+        'tenancy.cache.stores' => ['foo_file'],
+        'cache.stores.foo_file' => [
+            'driver' => 'file',
+            'path' => $path,
+            'lock_path' => $lockPath,
+        ],
+    ]);
+
+    $tenant = Tenant::create();
+    tenancy()->initialize($tenant);
+
+    $tenantPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo";
+    $tenantLockPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo_locks";
+
+    // lock('foo')->get() acquires the lock and creates the lock file at $tenantLockPath
+    expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue();
+
+    expect(File::isDirectory($tenantLockPath))->toBeTrue();
+    // The lock file was created at $tenantLockPath, not $tenantPath
+    expect(File::isDirectory($tenantPath))->toBeFalse();
+
+    tenancy()->end();
+
+    // The lock was only acquired in the tenant context, so in the central context,
+    // it's free (and the lock file doesn't exist in the central context).
+    expect(File::isDirectory($lockPath))->toBeFalse();
+    // Acquire the lock in the central context
+    expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue();
+    // The lock file was created at the original (central) lock_path
+    expect(File::isDirectory($lockPath))->toBeTrue();
+
+    File::deleteDirectory($path);
+    File::deleteDirectory($lockPath);
+});
+
+test('a file cache store without a configured lock_path defaults to using its scoped path for locks', function () {
+    // 'lock_path' is optional in the file store config.
+    // FileStore falls back to using 'path' for locks when 'lock_path' is not specified in the config (or set to null).
+    // scopeCache() respects the original config and leaves lock_path unset/null.
+    $centralStoragePath = storage_path();
+    $path = "{$centralStoragePath}/framework/cache/foo";
+
+    // $path is hardcoded here. The directory created at $path in a previous run can persist,
+    // so delete the directory at $path first.
+    File::deleteDirectory($path);
+
+    config([
+        'tenancy.bootstrappers' => [
+            FilesystemTenancyBootstrapper::class,
+        ],
+        'tenancy.cache.stores' => ['foo_file'],
+        'cache.stores.foo_file' => [
+            'driver' => 'file',
+            'path' => $path,
+            // 'lock_path' not set (same behavior as if it were set to null)
+        ],
+    ]);
+
+    $tenant = Tenant::create();
+    tenancy()->initialize($tenant);
+
+    $tenantPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo";
+
+    // Initializing tenancy scopes the store's 'path', but respects the configured
+    // (unset) 'lock_path' and leaves it alone.
+    expect(config('cache.stores.foo_file.path'))->toBe($tenantPath);
+    expect(config('cache.stores.foo_file.lock_path'))->toBeNull();
+
+    // With no 'lock_path' configured, the lock file will be created in the scoped 'path' directory
+    expect(File::isDirectory($tenantPath))->toBeFalse();
+    expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue();
+    expect(File::isDirectory($tenantPath))->toBeTrue();
+
+    // The lock is still held in the tenant context, so acquiring it again fails
+    expect(Cache::store('foo_file')->lock('foo')->get())->toBeFalse();
+
+    tenancy()->end();
+
+    // The same 'foo' lock is free in central context.
+    // revert() points the 'path' back to the original.
+    expect(File::isDirectory($path))->toBeFalse();
+    expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue();
+    expect(File::isDirectory($path))->toBeTrue();
+
+    // Clean up directory created at the hardcoded path
+    File::deleteDirectory($path);
+});
+
+test('a cache store using a path not based on storage_path() has the tenant suffix appended', function () {
+    // tenantCachePath() has no central storage path prefix to swap for the tenant's here,
+    // so it appends the tenant suffix to $path instead.
+    $path = '/tmp/tenancy-cache-test';
+    File::deleteDirectory($path);
+
+    config([
+        'tenancy.bootstrappers' => [
+            FilesystemTenancyBootstrapper::class,
+        ],
+        'tenancy.cache.stores' => ['foo_file'],
+        'cache.stores.foo_file' => [
+            'driver' => 'file',
+            'path' => $path,
+        ],
+    ]);
+
+    $tenant1 = Tenant::create();
+    $tenant2 = Tenant::create();
+
+    tenancy()->initialize($tenant1);
+    Cache::store('foo_file')->put('key', 'tenant1');
+
+    tenancy()->initialize($tenant2);
+    expect(Cache::store('foo_file')->get('key'))->toBeNull();
+
+    tenancy()->initialize($tenant1);
+    expect(Cache::store('foo_file')->get('key'))->toBe('tenant1');
+
+    // Tenant's 'foo_file' cache directory is created at $path with
+    // the tenant suffix appended, not at the tenant-scoped storage_path().
+    expect(File::isDirectory("{$path}/tenant{$tenant1->id}"))->toBeTrue();
+    expect(File::isDirectory(storage_path('framework/cache/data')))->toBeFalse();
+
+    tenancy()->end();
+
+    File::deleteDirectory($path);
+});