Refrain from dealing with the impossible "self-referencing" scoped disk case. Instead of that, test the inline parent behavior.
Also update the exception message in forgetDisks() so that it's a bit less vague.
In FSBootstrapper::forgetDisks():
- `tenancy.filesystem.disks => ['scoped']` throws
- `tenancy.filesystem.disks => ['scoped', 'parent']` does NOT throw
- `tenancy.filesystem.disks => ['scoped_with_scoped_parent', 'scoped_parent']` (invalid config where a scoped disk's base disk doesn't actually exist because the scoped disks just reference themselves) throws
At first glance, it could look weird that $attemptedPath uses "/" but the check in abortIf below uses DIRECTORY_SEPARATOR. Add comment that explains this.
If diskRoot() somehow ended up receiving a scoped disk (e.g. in case the scoped disk was listed in tenancy.filesystem.disks), its root would get configured, and it'd be completely redundant. It wouldn't break anything since scoped disk's configured root is ignored -- its parent's root is always used. Even though not adding this skipping code would essentially do no harm, it prevents the method from doing redundant work and defines the behavior a bit more clearly.
diskUrl() is similar in that regard, but that method already has a strict "disk driver has to be 'local'" -- scoped disks never made it through so nothing to change there.
This includes moving the TenantAssetController baseDiskName() method to FSBootstrapper and making it public static, since the same logic is used in two places now. Also cover the edge case where a scoped disk A has a scoped disk B as its parent, and B has A as its parent -- in that case, the method would be stuck in an infinite loop (also added separate test for this, commenting out the $visited-related code in baseDiskName will make the test fail).
Also updated the assetRoot's unnamed disk exception message.
The test fails with the nested disk dataset because we resolve a disk first, then initialize tenancy, and because the nested scoped disks aren't forgotten, so the disk config changes that the FS bootstrapper applies aren't reflected on the already-resolved disk instance.
Instead of just saying that the publicDisk *should* be listed in tenancy.filesystem.disks, enforce that -- if the disk isn't tenant-aware, throw an exception.
Also update comments accordingly. E.g. since scoped disks don't have to have a single "parent disk" (the parent can also be a scoped disk and have another parent, and so on), use "base disk".
Since the tenant storage path can now be grabbed using FilesystemTenancyBootstrapper::getBoundTenantStoragePath(), the log bootstrapper doesn't need to depend on the FSBootstrapper being enabled and storage_path() being suffixed.
Instead of adding a regression test, just delete FilesystemTenancyBootstrapper from the config settings in the log bootstrapper tests (and in tests that did use storage_path() in tenant context assertions, use explicitly "hardcoded" paths instead).
Also, instead of throwing the "no root path configured" exception, just throw an exception if the disk is not local (i.e. is not instanceof LocalFilesystemAdapter). A local disk HAS to have a string root, otherwise, Laravel throws an exception while instantiating that disk.
Test that tenant assets can be served from scoped disks, and that tenant asset roots respect the disk's configured prefix.
Currently, the tests fail because TenantAssetController grabs the root from the config instead of resolving it via Storage::disk(...)->path('').
The docblock said that the FSBootstrapper was required for this job to work at all, but that's not fully true since the job just uses the FSBootstrapper's public static methods to get the storage paths.
Correct misleading ones, add ones that were missing (e.g. the TenantAssetController's docblock, the FSBootstrapper dependency should be mentioned there)
Briefly document the root_override placeholders, make the links point to v4 docs instead of the v3 ones. Also in the url_override comments, mention that local disks must have a valid root in order for the override to work correctly.
Previously, we only skipped disks with `null` override. But an override with an empty string is also incorrect, and simply checking if $this->app['config']["tenancy.filesystem.url_override.{$disk}"]) is falsy instead of strictly null takes care of that.
Update the existing "create storage symlinks action skips disks with a null url_override" test so that it covers disks with NO url_override (unset/null and empty string). The test fails with the empty string override at the moment.
getBoundTenantStoragePath() and DeleteTenantStorage both claimed the tenant storage directory is where disks, cache and sessions are scoped to. That's only true when root_override points there and scope_cache/scope_sessions are enabled -- a root_override using %original_storage_path% puts the disk root outside it entirely.
The symlink target used to be built from the root_override template, so a disk without an entry there had nothing to resolve.
The symlink target is now the disk's tenant-context root, which the bootstrapper sets either way -- with a root_override it expands
the template, without one, it appends the suffix to the disk's own root. Disks that only have a url_override now get a working symlink
instead of being skipped while their URL was still overridden.
Skipping disks with a null url_override is now explicit. The root_override check used to cover that by accident, and without it str_replace() gets null and throws a TypeError.
The comment said that disks, cache and sessions are scoped ot the tenant's storage dir either way, but that's only true if the disks are included in tenancy.filesystem.disks, and for cache and sessions, scope_cache and scope_sessions have to be enabled. This might be obvious, but it'll be better to make this completely clear from the comment.
When a disk in url_override and root_override is absent from tenancy.filesystem.disks, FilesystemTenancyBootstrapper leaves its root unchanged, possibleTenantSymlinks allows creating a symlink for that unscoped disk (= a disk with a central root), which can expose shared files.
Fixed by throwing an exception in possibleTenantSymlinks saying that the disk should be tenant-aware (= included in the tenancy.filesystem.disks config).
The resolved path was compared to the asset root using a plain string prefix, so a
directory whose name just starts with the asset root's name passed the check. This
didn't matter while the asset root was hardcoded to app/public, but $publicDisk lets
it be any disk root.
Currently this fails because the controller checks that the requested file is inside the
asset root using a plain string prefix, so with the root set to
'%storage_path%/app/media/', a request for '../media-originals/photo.jpg' is served
from the sibling 'app/media-originals' directory.
Disabling the config doesn't break local disk tenancy -- it only affects the
storage_path() helper. Disks, cache and sessions are scoped either way, so the
tradeoff is that files accessed using storage_path() are shared by all tenants.
The check compares the tenant's storage path with the bootstrapper's central storage
path, so unlike the original one, it doesn't depend on storage_path().
With the current path resolution, the two can only be the same if suffix_base and the
tenant's key are both empty, so this is just a safety net for weird configurations.
TenantAssetController::$publicDisk is null by default, which keeps serving the
assets from app/public inside the tenant's storage directory. Setting it to a disk
name serves the assets from that disk's root instead.
A disk with no root path throws instead of resolving to an empty path. realpath('')
returns the current working directory, so the controller would end up treating the
whole app directory as the allowed root.
The only thing the controller now depends on is that FilesystemTenancyBootstrapper needs to be enabled (so basically, the same dependency as before, but before this, there was the extra "suffix_storage_path === true" dependency -- not literally, storage_path() just had to be suffixed in tenant context, otherwise, the controller would read from the central storage in tenant context).
possibleTenantSymlinks() resolved the root_override template on its own, using
storage_path() for %storage_path% and leaving %original_storage_path% and
%tenant% unreplaced. Let the FS bootstrapper resolve the placeholders tenant instead, so the symlinks point where the disks actually write.
The symlinks should point to the tenant's disk root regardless of the
suffix_storage_path config and of which root_override placeholders are used.
Currently, the storage_path() helper is used for generating the symlink path,
so the two new datasets fail.
The job depended on storage_path(), which is only suffixed when
suffix_storage_path is enabled, so with it disabled the tenant's files were left
behind. It now uses the bootstrapper's own suffix logic via a new
getBoundTenantStoragePath() method..
The job should delete the tenant storage regardless of the suffix_storage_path config. Now, the job depends on that config, so currently, this test fails.
Also remove the "FS bootstrapper disabled" assertions. The job clearly depends on the bootstrapper being enabled, so I don't think these assertions matter in the end.
> The cache part of this is specific to `file`-driver stores that are
listed in `tenancy.cache.stores`, while `tenancy.filesystem.scope_cache`
is set to `true`. The session part applies to the `file` session driver,
while `tenancy.filesystem.scope_sessions` is set to `true`.
>
> Ran into this while checking whether we could drop the separate
'parallel' cache store from our boilerplate's testing setup and instead
just give the 'file' store a per-process path
(`framework/cache/data_<parallel testing token>`), so each test process
gets its own cache directory. Turns out `scopeCache()` discards
configured paths entirely, so that has no effect (see below).
Using a different directory for the `file` cache store by setting
`cache.stores.file.path` (either using `config([...])`, or directly in
`config/cache.php` -- doesn't matter) has no effect --
`FilesystemTenancyBootstrapper::scopeCache()` ignores `path`/`lock_path`
entirely and rewrites both to a hardcoded
`<storage>/framework/cache/data` path on every
`tenancy()->initialize()`/`tenancy()->end()`:
```php
// In `FilesystemTenancyBootstrapper::scopeCache()` (called both in `bootstrap()` and in `revert()`)
foreach ($stores as $name) {
$path = $storagePath . '/framework/cache/data';
$this->app['config']["cache.stores.{$name}.path"] = $path;
$this->app['config']["cache.stores.{$name}.lock_path"] = $path;
...
}
```
Specific issues with hardcoding the path like this:
- a store with a configured (non-default) path gets scoped to use the
default one (what I described above)
- `lock_path` is always overwritten with `path`, so a store with a
separate lock directory loses that separation
- `revert()` runs the same code, so it doesn't restore what the store
was configured with before tenancy initialized -- it just re-applies the
same hardcoded default. Central cache ends up using the wrong path after
ending tenancy.
**`scopeSessions()` has the same bug.** It never reads `session.files`,
it hardcodes `<storage>/framework/sessions` on both bootstrap and
revert. So a configured session path gets discarded when tenancy
initializes, and it doesn't get reverted back to what it was when
tenancy ends. For example, with `session.files` set to
`/tmp/foo-sessions`:
```
In tenant context: session.files = .../storage/tenant<key>/framework/sessions
After ending tenancy: session.files = .../storage/framework/sessions
```
So after ending tenancy, sessions don't go back to the configured
`/tmp/foo-sessions`. They use the hardcoded
`<storage>/framework/sessions` path, which was never configured
anywhere.
### The fix
In `bootstrap()`, `scopeCache()` captures the original configured paths
and scopes those instead of using a hardcoded default.
- If the configured path is under the central storage path, that central
part gets swapped for the tenant's storage path, keeping everything
after it the same (e.g. `storage/framework/cache/data` becomes
`storage/tenant1/framework/cache/data`).
- If the path isn't `storage_path()`-based, there's nothing to swap, so
the tenant's suffix just gets appended to the end of the path instead.
On `revert()`, `scopeCache(false)` puts the captured paths back into
`cache.stores.{$name}.path`/`lock_path` and into the resolved store
instance, so central cache uses the path it was configured with again.
> The paths are captured just once, during `scopeCache()` at
`bootstrap()`. If `bootstrap()` fails after `scopeCache()` (e.g. when
`scopeSessions()` can't create its directory), `revert()` never runs and
the config is left with the scoped paths, so capturing a second time
would lose the central ones. Also, `revert()` iterates the stores whose
paths were captured during bootstrap rather than
`config('tenancy.cache.stores')`. Removing a store from that config in
tenant context would otherwise make `revert()` skip it and leave it
stuck with a tenant-scoped path, and adding one would make
`tenancy()->end()` throw because its path was never captured (see the
'scopeCache ignores changes to tenancy.cache.stores made in tenant
context' test). These are edge cases most users wouldn't notice, but
still, worth mentioning.
`lock_path` stays `null` when a store doesn't configure it, rather than
us making it default to the scoped path -- `FileStore` already falls
back to `path` for locks in that case, so we can just respect the
store's original config.
`scopeSessions()` does the same for `session.files`. It captures the
configured path during `bootstrap()` (once, for the same reason as
above), scopes it, and puts it back on `revert()` (the default
`storage/framework/sessions` still ends up as
`storage/tenant1/framework/sessions`, so nothing changes for the default
config).
Also added tests that cover each of the issues above (+ a test for
handling paths that aren't `storage_path()`-based, and one for a custom
`session.files` path).
**POSSIBLE MINOR BC:** Someone with `'path' => '/var/cache/foobar'`
currently gets tenant cache in `storage/tenant1/framework/cache/data`.
After this fix, they get `/var/cache/foobar/tenant1`, so whatever is
already cached in the old directory is orphaned. The same applies to a
non-default `storage_path()`-based path, e.g. `'path' =>
storage_path('framework/cache/data_' . env('TEST_TOKEN', 'default'))`.
The config is now respected while scoping. The same goes for sessions --
with a non-default `session.files`, tenant sessions move from
`storage/tenant1/framework/sessions` to the configured path scoped for
the tenant, so the sessions in the old directory are orphaned.
## Note about directory separators
While implementing a method that centralizes scoping a path to the
tenant (`tenantScopedPath()`), we looked into which code should use
`DIRECTORY_SEPARATOR` instead of plain `/` (for Windows compatibility,
overall correctness and consistency).
In short, the separators only matter in code that compares paths --
there, both sides of the check have to use the same separators (it
doesn't matter whether that's `DIRECTORY_SEPARATOR` or `/`). Strings
that only get passed to the filesystem are fine with plain `/` -- the
filesystem handles these just fine (for example, Laravel uses
`storage_path('framework/cache/data')` as the default `file` cache
store's path, and that works just fine on Windows).
A thing related to this are the `rtrim()` calls. In `diskRoot()`'s "disk
present in `tenancy.filesystem.disks`, but not in
`tenancy.filesystem.root_override`" code branch, `rtrim()` only trimmed
the `/` separator. So if a Windows user used a local disk like that, and
configured that disk's path to use a trailing separator, the method
would set the disk root to a path like `C:\app\uploads\/tenant1`. In
practice, this shouldn't be an issue, but trimming both `/` and `\`
prevents that code from setting the root to a weird path like that (so a
very low impact change).
The `tenantStoragePath()` method got the same treatment as `diskRoot()`
mentioned above: the original storage path _could_ be configured with a
trailing slash. Again, this was a non-issue, but only because the
method's output didn't get compared to other strings in a way where this
_could_ be an issue. The `rtrim` there is a _slight_ improvement, but
primarily, this got changed for consistency with the change in
`diskRoot()`.
---------
Co-authored-by: Samuel Stancl <samuel@archte.ch>
