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.
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 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 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>
This PR adds the LogChannelBootstrapper to provide tenant-specific
logging configuration. The bootstrapper automatically configures storage
path channels to use tenant-specific directories (NOTE: for this to work
correctly, the bootstrapper has to run AFTER
FilesystemTenancyBootstrapper, otherwise, the logs still won't be
separated, unless you use overrides) and supports custom channel
overrides for custom logging scenarios -- mapping tenant properties to
the channel config, or using custom closures an array with the logging
config, e.g. for making the slack channel (that's not handled by the
bootstrapper by default) tenant-specific.
The bootstrapper first modifies the channel config, then forgets the
channel from LogManager so that on the next logging attempt, the channel
is re-resolved with the modified config. Otherwise, the channel would
just use the initial config **if the channel was resolved before**. If
the channel wasn't resolved before, it'll always be resolved with the
updated (tenant) config, unless the configuration fails. In that case,
the config will be reverted (the central config will be restored) and
the error will be logged using the original channel.
When using a channel stack, the `stack` channel itself also has to be
forgotten, since the LogManager could retain e.g. the original `stack`
channel's webhook URL, while the underlying `slack` channel would use
the updated one, and while logging, the app would actually use the
initial webhook URL instead of the updated one (encountered this issue
while testing).
Note that **all** channels in `$storagePathChannels` and
`$channelOverrides` are affected.
Also, adding `'attachment' => 'false'` to the slack channel's config
makes the slack channel work with Discord webhooks (just a cool thing we
figured out whlle testing the bootstrapper).
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
This prevents race conditions that may occur if there are two concurrent
processes trying to create the storage path for the tenant. The
storagePath() method runs during bootstrap() which can easily happen
in two places at once. The race condition specifically occurs in between
the is_dir() check and the mkdir() call, the latter producing an
exception if the dir already exist. We simply ignore any error coming
out of mkdir() and then check for success separately.
We could omit that success check since failure is unlikely and would
only occur due to a server misconfiguration that would manifest itself
in other ways as well, but this way the simple TOC/TOU race condition
is prevented while other errors are still reported.
We apply the same change to the mkdir() in scopeSessions() as the logic
is similar.
Resolves#1452
This change was prompted by a phpstan failure after a recent update.
While making this change, I noticed we don't need the macro anymore
as useAssetOrigin() was added to the UrlGenerator earlier this year,
simplifying our implementation.
This is because the CreateTenantStorage listener only runs when
a tenant is created, but in multi-server setups the directory may
need to be created each time a tenant is *used*, not just created.
Also changed the listeners to use TenantEvent instead of specific
events, to make it possible to use them with other events, such as
TenancyBootstrapped.
Also update permission bits in a few mkdir() calls to better scope
data to the current OS user.
Also fix a typo in CacheTenancyBootstrapper (exception message).
* cleanup, resolve todos, add immediate todos
* Improve path_identification_middleware docblock
* rename leave() method in tests
* wip fix hardcoded values making assumptions about the parameters used in routing
* defaultParameterNames
* fix CreatesDatabaseUsers return values
* $tenant -> tenant()
* resolve more todos
* make comment block a complete block
* Correct useTenantRoutesInFortify(), delete unused import
* test fixes
* remove todos
* remove JobPipeline todo
* simplify comment example
* remove todo
* fix VERSION_PREFIX in queue.yml
---------
Co-authored-by: lukinovec <lukinovec@gmail.com>
* Run cache tests on all supported drivers
* update ci healthcheck for memcached
* remove memcached healthcheck
* fix typos in test comments, expand internal.md [ci skip]
* add empty line [ci skip]
* switch to using $store->setPrefix()
* add dynamodb
* refactor try-finally to try-catch
* remove unnecessary clearResolvedInstances() call
* add dual Cache:: and cache() assertions
* add apc
* Flush APCu cache in test setup
* Revert "add dual Cache:: and cache() assertions"
This reverts commit a0bab162fbe2dd0d25e7056ceca4fb7ce54efc77.
* phpstan fix
* Add logic for scoping 'file' disks to FilesystemTenancyBootstrapper
* minor changes, add todos
* refactor how the session.connection is used in the DB session bootstrapper
* add session forgery prevention logic to the db session bootstrapper
* only use the fs bootstrapper for file disk in 'cache data is separated' dataset
* minor session scoping test changes
* Add session scoping logic to FilesystemTenancyBootstrapper, correctly update disk roots even with storage_path_tenancy disabled
* Fix code style (php-cs-fixer)
* update docblock
* make not-null check more explicit
* separate bootstrapper tests, fix swapped test names for two tests
* refactor cache bootstrapper tests
* resolve global cache todo
* expand tests: session separation tests, more filesystem separation assertions; change prefix_base-type config keys to templates/formats
* add apc session scoping test, various session separation bugfixes
* phpstan + minor logic fixes
* prefix_format -> prefix
* fix database session separation test
* revert composer.json changes, update laravel dependencies to expected next release
* only run session scoping logic in cache bootstrapper for redis, memcached, dynamodb, apc; update gitattributes
* tenancy.central_domains -> tenancy.identification.central_domains
* db session separation test: add datasets
---------
Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
* 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>
* Test if Storage::url() works correctly after reverting to central context
* Fix typo in code
* Access originalPaths using dot notation using `data_get()`
* This adds support for tenancy aware Storage::url() method
* Trigger CI build
* Fixed Link command for Laravel v6, added StorageLink Events, more StorageLink tests, added RemoveStorageSymlinks Job, added Storage Jobs to TenancyServiceProvider stub, renamed misleading config example.
* Fix typo
* Fix code style (php-cs-fixer)
* Update config comments
* Format code in Link command, make writing more concise
* Change "symLinks" to "symlinks"
* Refactor Link command
* Fix test name typo
* Test fetching files using the public URL
* Extract Link command logic into actions
* Fix code style (php-cs-fixer)
* Check if closure is null in CreateStorageSymlinksAction
* Stop using command terminology in CreateStorageSymlinksAction
* Separate the Storage::url() test cases
* Update url_override comments
* Remove afterLink closures, add types, move actions, add usage explanation to the symlink trait
* Fix code style (php-cs-fixer)
* Update public storage URL test
* Fix issue with using str()
* Improve url_override comment, add todos
* add todo comment
* fix docblock style
* Add link command tests back
* Add types to $tenants in the action handle() methods
* Fix typo, update variable name formatting
* Add tests for the symlink actions
* Change possibleTenantSymlinks not to prefix the paths twice while tenancy is initialized
* Fix code style (php-cs-fixer)
* Stop testing storage directory existence in symlink test
* Don't specify full namespace for Tenant model annotation
* Don't specify full namespace in ActionTest
* Remove "change to DI" todo
* Remove possibleTenantSymlinks return annotation
* Remove symlink-related jobs, instantiate and use actions
* Revert "Remove symlink-related jobs, instantiate and use actions"
This reverts commit 547440c887.
* Add a comment line about the possible tenant symlinks
* Correct storagePath and publicPath variables
* Revert "Correct storagePath and publicPath variables"
This reverts commit e3aa8e2086.
* add a todo
Co-authored-by: Martin Vlcek <martin@dontfreakout.eu>
Co-authored-by: lukinovec <lukinovec@gmail.com>
Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
* Let the user pass the tenant suffix by %tenant%
In this PR we let the user pass an additional parameter using `%tenant%` so the user can additionally pass the folder corresponding to each tenant.
This is my proposal, because if I try to use %storage_path% within Linux, I get the full path to the project when I use Google Cloud Storage
* Missing missing updates
Moving from $subject to $root when %storage_path% has been replaced
* Test on Laravel 9
* Don't extend final Kernel class
* Make FilesystemTenancyBootstrapper compatible with Flysystem v3
Co-authored-by: George <jiri.zizka@funfirst.cz>
* Update tenant maintenance mode te be in line with Laravel
* Exclude PHP 7.4 <> L9 combination from testing
* add root_override-related assertions
* getPrefix -> getPathPrefix
* handle / inconsistency in s3 prefix
* Refactor Storage facade changes
Co-authored-by: George <jiri.zizka@funfirst.cz>
Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com>
* PHP 8 support, test workflow syntax
* Try using a custom data structure in the workflow
* Try using matrix include
* Default PHP target
* single include
* fix composer package name
* [WIP] Try using dev-master for valuestore
* Update spatie/valuestore constraint to be stable
