Make hardening work correctly even for named SQLite DBs, also make the related test test named SQLite DBs instead of just MySQL (the SQLite dataset fails when the DatabaseTenancyBootstrapper changes get reverted).
Instead of just checking the presence of the tenants table on the current connection to determine if the table is/isn't tenant, check the current database's name, and if it's the central DB name, throw the runtime exception.
This PR adds the `toRoute()` method override to `TenancyUrlGenerator`.
`toRoute()` now attempts to find a tenant equivalent of the passed route
(= a route with the same name as the passed one, but with the tenant
prefix) and generates URL for the tenant route. This behavior can be
bypassed using the bypass parameter, like with the `route()` method
override `TenancyUrlGenerator` had until now.
The primary reason for adding this is that Livewire v4 no longer uses
the `route()` helper (which automatically prefixes the passed route name
because of the override in `TenancyUrlGenerator`) in
`Livewire::getUpdateUri()`. Now, it uses `toRoute()`
(544aa3dfb8 (diff-e7609f8b0a60bde5a85067803d4e2f08f235c7cee9225a51ea67a85ff9a1d694R52)),
which didn't automatically swap the route for its 'tenant.'-prefixed
equivalent in tenant context (until now). So for the Livewire
integration to work with path identification, we need to override
`toRoute()` as described.
The `temporarySignedRoute()` override got removed because
`temporarySignedRoute()` calls `route()` under the hood, there's no need
to specifically override `temporarySignedRoute()`.
> Note: Browsing old convos, it seems like the `temporarySignedRoute()`
override was needed to make Livewire file uploads work with path
identification, but it's not needed anymore. TenancyUrlGenerator had
some changes since then, and now, I can't see the _exact_ reason why we
needed the override (`temporarySignedRoute()` uses `route()` under the
hood, so the only thing that should really matter is overriding
`route()`/`toRoute()`). It was likely a leftover from some older
implementation.
The `route()` override got simplified. Since `route()` uses `toRoute()`
under the hood, the `route()` override only has to have the prefixing
logic. The rest is delegated to `toRoute()`.
> Note: Even though we override `toRoute()` now which `route()` uses for
generating the URLs, we still need to override `route()` for its
`$this->routes->getByName($name)` call to receive the prefixed name. For
example, if `route()` wasn't overridden, and we only had one route:
`tenant.foo` (no central `foo` route), and we'd call `route('foo')`,
we'd get an exception saying that route "foo" wasn't found, even if
automatic route name prefixing was enabled and `toRoute()` was
overridden. With the `route()` override, `route('foo')` acts as if we
passed 'tenant.foo' instead of 'foo'.
Comments in TenancyUrlGenerator and UrlGeneratorBootstrapper got updated
to be more accurate. All _intentionally_ affected methods are listed in
TenancyUrlGenerator's docblock.
---------
Co-authored-by: Samuel Stancl <samuel@archte.ch>
Since It's possible to update tenant's db_name to the central DB or the DB of another tenant. Setting $harden to true prevents tenants from connecting to the wrong databases.
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.
With the previous implementation, many users would use the default
config that enables scope_sessions. They would then deploy the app
to production and get the exception there since they use the
`database` session driver which is scoped by a different mechanism.
The idea behind throwing the exception only in prod was to make it
easy to use different setups locally without getting annoying
exceptions, while notifying users that a security feature they enabled
isn't running in production.
However, a better way of doing this is to just throw the exception
consistently in all setups and use a sane default for enabling the
scope_sessions setting based on the SESSION_DRIVER env var.
Users are always encouraged to read the session scoping docs to make
sure their session scoping configuration makes sense for their specific
setup, but this is a good balance for providing solid security out of
the box for most setups without requiring users to configure things
manually.
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).
By purging stores, we "detach" existing cache stores from the
CacheManager, making them impossible to adjust in the future.
We also unnecessarily recreate them on every tenancy bootstrap/revert.
A simpler case where this causes problems is defining a RateLimiter in
a service provider. That injects a single cache store into the
rate limiter singleton, which then becomes a completely independent
object after tenancy is initialized due to the purge. This in turn
means the central and tenant contexts share the rate limiter cache
instead of using separate caches as one would expect.
Before, when using UrlGeneratorBootstrapper, and your app had a
`https://` url, in tenant context, the url would have the `http://`
scheme.
Now, the bootstrapper makes sure that the TenancyUrlGenerator inherits
the original UrlGenerator's scheme. So if your app has e.g. url
"https://some-url.test", `route('home')` in tenant context will return
"http**s**://some-url.test/home" (originally, you'd get
"http://some-url.test/home" - the original scheme - https - wouldn't be
respected in the tenant context).
This PR addresses the issue reported on Discord
(https://discord.com/channels/976506366502006874/976506736120823909/1399012794514411621).
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
The change in SQLiteDatabaseManager wasn't properly saving the
updated internal value.
The check in CacheTenancyBootstrapper wasn't handling that local tests
have a 'testing' environment, not local. However fixing only the
condition would've still added the store to $names which would throw
an exception down the line. We make sure to only throw the exception
in prod, but also make sure to only add the store to $names if it is
supported.
Notable changes:
- CreateUserWithRLSPolicies: Clarify why we're creating a custom
DatabaseConfing instance
- HasDatabase: Clarify why we're ignoring tenancy_db_connection
- DatabaseConfig: General refactor, clarify the role of the host conn
- SQLiteDatabaseManager: Handle trailing DIRECTORY_SEPARATOR
in static::$path
- DisallowSqliteAttach: Don't throw any exceptions, just silently fail
since the class isn't 100% portable
- Clean up todos that are no longer relevant
- Clean up dead code or comments in some database managers
The feature was pretty much a soft-bootstrapper -- it listened
to both Bootstrapped and Reverted. Bootstrappers have a few more
protections in terms of error handling and safe reverting, so there's
no point in (badly) re-implementing bootstrapper functionality within
TenantConfig just so it could be a Feature.
Going forward, all Features should be things that are mostly agnostic
of the tenant state, and especially they should not use bootstrapped/
reverted events. Bootstrappers are simply more appropriate and safe.
* Initial implementation (lukinovec)
* Make sure DatabaseCacheBootstrapper runs after DatabaseTenancyBootstrapper, misc wip changes
* Fix withTenantDatabases()
* Add failing test (GlobalCacheTest)
* Configure globalCache's DB stores to use central connection instead of default connection every time it's reinstantiated
* Make GlobalCache facade not cached. Even though it wasn't causing issues
in our existing tests, it likely was flaky, and making it not $cached
makes it now consistent with global_cache() - always getting a new
CacheManager from the globalCache container binding
* Add database connection assertions in GlobalCacheTest
* Run all cached resolver/global cache tests with DatabaseCacheBootstrapper
* Reset adjustCacheManagerUsing in revert() and TestCase
* Reset static $stores property
* Finalize GlobalCache-related changes
* tests: remove pointless cache TTLs
* Refactor DatabaseCacheBootstrapper
* Refactor tests
Co-authored-by: lukinovec <lukinovec@gmail.com>
* Test encrypted cookie identification
* Add Fortify bootstrapper custom query param passing test
* Correct Fortify route bootstrapper test (todo refactor, convoluted)
* Clarify Fortify bootstrapper test
* Fix encrypted cookie identification test
* Move encrypted cookie assertion to "cookie identification works"
* Cover configured tenant model columns in cached resolver tests
* Refactor testing resolver with default vs custom tenant model name config
* Delete resolved todo
* Make code more concise
* Keep initial formatting (minimize diff noise)
* Make dataset/helper method parameter clearer
* Clarify fortify test
* Clarify assertions, improve comments
* Delete excessive comments, make existing comments consistent and clearer
* Make cached resolver test file clearer, update outdated comments
* Use the tenant model column term consistently
* FIx inconsistencies
* Provide more info in comment
* make comment more clear
* static property reset
---------
Co-authored-by: Samuel Štancl <samuel@archte.ch>
* UrlGenerator: set defaults based on config; request data: move config to config file+resolver
* Claude code adjustments
* improve request data tests, simplify complex test in UrlGeneratorBootstrapperTest
* url generator test: test changing tenant parameter name
* request data identification: add tenant_model_column configuration
* defaultParameterNames -> passQueryParameter
* move comment
* minor refactor in PathIdentificationTest, expand CLAUDE.md to include early identification section
* Fix COLOR_FLAG
* improve test name
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* TenancyUrlGenerator: add a check for queryParameterName being null
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Fix code style (php-cs-fixer)
---------
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* 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>
* Make RootUrlBootstrapper run ONLY in CLI by default (add $rootUrlOverrideInTests), work with resolved UrlGenerator
* Make resolving 'url' return a pre-created generator instance instead of creating it on every app('url') call
* Take care of doubling tenant keys in TenancyUrlGenerator, add regression test for using UrlGenerator and RootUrl bootstrappers together
* Fix code style (php-cs-fixer)
* refactor RootUrlBootstrapper
* add docblock
* clarify docblock
* simplify test: use concrete values instead of overly dynamic code
* Fix bootstrapper order in test, add url('/') assertion
* Use $this->app instead of app()
* Improve TenancyUrlGenerator and RootUrlBootstrapperTest clarity
* Revert attempt to maintain compatibility between the two bootstrappers
* Delete bootstrapper combining test
* Fix code style (php-cs-fixer)
---------
Co-authored-by: lukinovec <lukinovec@gmail.com>
Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
* Declare sensitive parameters as sensitive
... just so that they don't show up in logs
* Remove unnecessary null-coalescing
* Simplify return
* Merge isset() calls
* Inline return
* Use nullsafe operator
* Simplify if-else branches
* Use direct empty string comparison instead of strlen()
* Add missing type
* Change interface as events expect a TenantWithDatabase not just a Tenant
* Narrow typehint
* Remove redundant type casts
* Fix style with php-cs-fixer
* Fix typos
* Revert unwanted if-else simplification
* fix phpstan errors
* narrow type
---------
Co-authored-by: Samuel Štancl <samuel@archte.ch>
