diff --git a/assets/TenancyServiceProvider.stub.php b/assets/TenancyServiceProvider.stub.php index 1cb358de..915e80c2 100644 --- a/assets/TenancyServiceProvider.stub.php +++ b/assets/TenancyServiceProvider.stub.php @@ -53,8 +53,6 @@ class TenancyServiceProvider extends ServiceProvider ])->send(function (Events\TenantCreated $event) { return $event->tenant; })->shouldBeQueued(false), - - // Listeners\CreateTenantStorage::class, ], Events\SavingTenant::class => [], Events\TenantSaved::class => [], @@ -63,12 +61,11 @@ class TenancyServiceProvider extends ServiceProvider Events\DeletingTenant::class => [ JobPipeline::make([ Jobs\DeleteDomains::class, + // Jobs\DeleteTenantStorage::class, // Jobs\RemoveStorageSymlinks::class, ])->send(function (Events\DeletingTenant $event) { return $event->tenant; })->shouldBeQueued(false), - - // Listeners\DeleteTenantStorage::class, ], Events\TenantDeleted::class => [ JobPipeline::make([ diff --git a/docker-compose.yml b/docker-compose.yml index 70a68019..6cc03e12 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -80,8 +80,8 @@ services: mssql: image: mcr.microsoft.com/mssql/server:2022-latest environment: - - ACCEPT_EULA=Y - - SA_PASSWORD=P@ssword # must be the same as TENANCY_TEST_SQLSRV_PASSWORD + ACCEPT_EULA: "Y" + SA_PASSWORD: "P@ssword" # must be the same as TENANCY_TEST_SQLSRV_PASSWORD healthcheck: # https://github.com/Microsoft/mssql-docker/issues/133#issuecomment-1995615432 test: timeout 2 bash -c 'cat < /dev/null > /dev/tcp/127.0.0.1/1433' interval: 10s diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index af2b809f..56091600 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Stancl\Tenancy\Bootstrappers; +use Exception; use Illuminate\Foundation\Application; use Illuminate\Session\FileSessionHandler; use Illuminate\Support\Facades\Storage; @@ -75,8 +76,13 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper : $this->originalStoragePath . '/framework/cache'; if (! is_dir($path)) { - // Create tenant framework/cache directory if it does not exist - mkdir($path, 0750, true); + // Create tenant framework/cache directory if it does not exist. + // We ignore errors due to TOCTOU race conditions, instead we check for success below. + @mkdir($path, 0750, true); + + if (! is_dir($path)) { + throw new Exception("Unable to create tenant storage directory [{$path}]."); + } } if ($suffix === false) { @@ -222,8 +228,13 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper : $this->originalStoragePath . '/framework/sessions'; if (! is_dir($path)) { - // Create tenant framework/sessions directory if it does not exist - mkdir($path, 0750, true); + // Create tenant framework/sessions directory if it does not exist. + // We ignore errors due to TOCTOU race conditions, instead we check for success below. + @mkdir($path, 0750, true); + + if (! is_dir($path)) { + throw new Exception("Unable to create tenant session directory [{$path}]."); + } } $this->app['config']['session.files'] = $path; diff --git a/src/Database/Concerns/PendingScope.php b/src/Database/Concerns/PendingScope.php index 712de6c7..e8805d8a 100644 --- a/src/Database/Concerns/PendingScope.php +++ b/src/Database/Concerns/PendingScope.php @@ -8,12 +8,13 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Scope; +/** @implements Scope */ class PendingScope implements Scope { /** * Apply the scope to a given Eloquent query builder. * - * @param Builder $builder + * @param Builder $builder * * @return void */ @@ -57,8 +58,10 @@ class PendingScope implements Scope { $builder->macro('withoutPending', function (Builder $builder) { $builder->withoutGlobalScope(static::class) - ->whereNull($builder->getModel()->getColumnForQuery('pending_since')) - ->orWhereNull($builder->getModel()->getDataColumn()); + ->where(function (Builder $query) { + $query->whereNull($query->getModel()->getColumnForQuery('pending_since')) + ->orWhereNull($query->getModel()->getDataColumn()); + }); return $builder; }); diff --git a/src/Database/ParentModelScope.php b/src/Database/ParentModelScope.php index ee1a200e..9268fea9 100644 --- a/src/Database/ParentModelScope.php +++ b/src/Database/ParentModelScope.php @@ -8,10 +8,11 @@ use Illuminate\Database\Eloquent\Builder; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Scope; +/** @implements Scope */ class ParentModelScope implements Scope { /** - * @param Builder $builder + * @param Builder $builder */ public function apply(Builder $builder, Model $model): void { diff --git a/src/Database/TenantScope.php b/src/Database/TenantScope.php index 050da365..c6ce5f09 100644 --- a/src/Database/TenantScope.php +++ b/src/Database/TenantScope.php @@ -9,10 +9,11 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Scope; use Stancl\Tenancy\Tenancy; +/** @implements Scope */ class TenantScope implements Scope { /** - * @param Builder $builder + * @param Builder $builder */ public function apply(Builder $builder, Model $model) { diff --git a/src/Jobs/DeleteTenantStorage.php b/src/Jobs/DeleteTenantStorage.php new file mode 100644 index 00000000..36a0d326 --- /dev/null +++ b/src/Jobs/DeleteTenantStorage.php @@ -0,0 +1,43 @@ +central(fn () => storage_path()); + $tenantStoragePath = tenancy()->run($this->tenant, fn () => storage_path()); + + if ($tenantStoragePath === $centralStoragePath) { + // Check again to ensure the tenant storage path is distinct from the central storage path + // to avoid any accidental central storage path deletion + return; + } + + if (is_dir($tenantStoragePath)) { + File::deleteDirectory($tenantStoragePath); + } + } +} diff --git a/src/Listeners/CreateTenantStorage.php b/src/Listeners/CreateTenantStorage.php index 3bebb731..0ffdef60 100644 --- a/src/Listeners/CreateTenantStorage.php +++ b/src/Listeners/CreateTenantStorage.php @@ -7,11 +7,7 @@ namespace Stancl\Tenancy\Listeners; use Stancl\Tenancy\Events\Contracts\TenantEvent; /** - * Can be used to manually create framework directories in the tenant storage when storage_path() is scoped. - * - * Useful when using real-time facades which use the framework/cache directory. - * - * Generally not needed anymore as the directory is also created by the FilesystemTenancyBootstrapper. + * @deprecated FilesystemTenancyBootstrapper creates the path automatically when suffix_storage_path is enabled. */ class CreateTenantStorage { diff --git a/src/Listeners/DeleteTenantStorage.php b/src/Listeners/DeleteTenantStorage.php index ec360073..06f20454 100644 --- a/src/Listeners/DeleteTenantStorage.php +++ b/src/Listeners/DeleteTenantStorage.php @@ -7,14 +7,29 @@ namespace Stancl\Tenancy\Listeners; use Illuminate\Support\Facades\File; use Stancl\Tenancy\Events\Contracts\TenantEvent; +/** + * @deprecated Use Stancl\Tenancy\Jobs\DeleteTenantStorage in a job pipeline instead. + */ class DeleteTenantStorage { public function handle(TenantEvent $event): void { - $path = tenancy()->run($event->tenant, fn () => storage_path()); + if (config('tenancy.filesystem.suffix_storage_path') === false) { + // Skip storage deletion if path suffixing is disabled + return; + } - if (is_dir($path)) { - File::deleteDirectory($path); + $centralStoragePath = tenancy()->central(fn () => storage_path()); + $tenantStoragePath = tenancy()->run($event->tenant, fn () => storage_path()); + + if ($tenantStoragePath === $centralStoragePath) { + // Check again to ensure the tenant storage path is distinct from the central storage path + // to avoid any accidental central storage path deletion + return; + } + + if (is_dir($tenantStoragePath)) { + File::deleteDirectory($tenantStoragePath); } } } diff --git a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php index 628b974e..4e834917 100644 --- a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php @@ -13,7 +13,7 @@ use Stancl\Tenancy\Events\TenancyInitialized; use Stancl\Tenancy\Jobs\CreateStorageSymlinks; use Stancl\Tenancy\Jobs\RemoveStorageSymlinks; use Stancl\Tenancy\Listeners\BootstrapTenancy; -use Stancl\Tenancy\Listeners\DeleteTenantStorage; +use Stancl\Tenancy\Jobs\DeleteTenantStorage; use Stancl\Tenancy\Listeners\RevertToCentralContext; use Stancl\Tenancy\Bootstrappers\FilesystemTenancyBootstrapper; use function Stancl\Tenancy\Tests\pest; @@ -184,21 +184,63 @@ test('create and delete storage symlinks jobs work', function() { $this->assertDirectoryDoesNotExist(public_path("public-$tenantKey")); }); -test('tenant storage can get deleted after the tenant when DeletingTenant listens to DeleteTenantStorage', function() { - Event::listen(DeletingTenant::class, DeleteTenantStorage::class); +test('tenant storage gets deleted during tenant deletion when the DeletingTenant pipeline contains DeleteTenantStorage', function() { + Event::listen(DeletingTenant::class, + JobPipeline::make([DeleteTenantStorage::class])->send(function (DeletingTenant $event) { + return $event->tenant; + })->shouldBeQueued(false)->toListener() + ); + + $centralStoragePath = storage_path(); + tenancy()->initialize(Tenant::create()); + + // FilesystemTenancyBootstrapper not enabled, + // tenant and central storage path is the same, + // the storage deletion will be skipped. + $tenantStoragePath = storage_path(); + expect($tenantStoragePath)->toBe($centralStoragePath); + expect(File::isDirectory($centralStoragePath))->toBeTrue(); + tenant()->delete(); + + expect(File::isDirectory($centralStoragePath))->toBeTrue(); + + config([ + 'tenancy.bootstrappers' => [FilesystemTenancyBootstrapper::class], + 'tenancy.filesystem.suffix_storage_path' => false, + ]); + + tenancy()->initialize(Tenant::create()); + + $tenantStoragePath = storage_path(); + + // FilesystemTenancyBootstrapper enabled, + // but tenant and central storage path is still the same + // because suffix_storage_path is false. + // The storage deletion will be skipped. + expect($tenantStoragePath)->toBe($centralStoragePath); + expect(File::isDirectory($centralStoragePath))->toBeTrue(); + tenant()->delete(); + + expect(File::isDirectory($centralStoragePath))->toBeTrue(); + + config([ + 'tenancy.bootstrappers' => [FilesystemTenancyBootstrapper::class], + 'tenancy.filesystem.suffix_storage_path' => true, + ]); tenancy()->initialize(Tenant::create()); $tenantStoragePath = storage_path(); - Storage::fake('test'); - + // FilesystemTenancyBootstrapper enabled, + // suffix_storage_path enabled, so the two paths are distinct. + // Tenant storage will be deleted. + expect($tenantStoragePath)->not()->toBe($centralStoragePath); expect(File::isDirectory($tenantStoragePath))->toBeTrue(); - Storage::put('test.txt', 'testing file'); - tenant()->delete(); expect(File::isDirectory($tenantStoragePath))->toBeFalse(); + expect(File::isDirectory($centralStoragePath))->toBeTrue(); }); test('the framework/cache directory is created when storage_path is scoped', function (bool $suffixStoragePath) { @@ -256,4 +298,3 @@ 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'); }); - diff --git a/tests/PendingTenantsTest.php b/tests/PendingTenantsTest.php index a90aceed..433b85fb 100644 --- a/tests/PendingTenantsTest.php +++ b/tests/PendingTenantsTest.php @@ -111,6 +111,18 @@ test('a new tenant gets created while pulling a pending tenant if the pending po expect(Tenant::withPending()->get()->count())->toBe(1); // All tenants }); +test('withoutPending chained with where clauses returns correct results', function () { + $tenant = Tenant::create(); + $pendingTenant = Tenant::createPending(); + + // The query returned the correct tenant + expect(Tenant::withoutPending()->where('id', $tenant->id)->first()->id)->toBe($tenant->id); + // No tenant with this ID exists, the query returns null + expect(Tenant::withoutPending()->where('id', Str::random(8) . 'nonexistent-id')->first())->toBeNull(); + // withoutPending() correctly excludes the pending tenant from the query + expect(Tenant::withoutPending()->where('id', $pendingTenant->id)->first())->toBeNull(); +}); + test('pending tenants are included in all queries based on the include_in_queries config', function () { Tenant::createPending();