the main change here is adding similar assertions related to
*re-resolving* channels to the 'stack channels that include any
configured channel are re-resolved' test, similar to the 'channels are
forgotten and re-resolved during bootstrap and revert' test
we use FilesystemTenancyBootstrapper::getBoundCentralStoragePath() in
LogTenancyBootstrapper to simplify the logic for extracting a path
inside the central storage directory
we could use this method in more places, such as DeleteTenantStorage:
$centralStoragePath = tenancy()->central(fn () => storage_path());
$tenantStoragePath = tenancy()->run($this->tenant, fn () => storage_path());
however for consistency (we're getting both the central and tenant
paths there) we're keeping it simple as it is now
using the newly added method would also introduce more coupling to
FilesystemTenancyBootstrapper - its specific implementation. this isn't
a big issue in LogTenancyBootstrapper which already has several
dependencies on the FS bootstrapper, and we do use app(static::class)
in the added method so extending the bootstrapper should work
old ignore no longer necessary, two new ignores are needed (the added
ignores are not necessary in PHP 8.5 but at this time our CI still runs
on PHP 8.4)
### Test for DatabaseTenancyBootstrapper throwing an exception when
`DB_URL` is set
The test meant to cover this set `central.url` before creating a tenant.
That made the `CreateDatabase` job fail with a QueryException (it tried
to use the URL host as a database name). The bootstrapper's check was
never reached, and the exception that the test intended to cover wasn't
ever thrown in the end. The test passed for the wrong reason.
Fixed by creating the tenant first, setting the url only after that, and
asserting that `tenancy()->initialize()` throws the bootstrapper's
specific exception.
### Reference env variable that Laravel uses now (`DB_URL`) instead of
`DATABASE_URL`
Changed all `DATABASE_URL` references in Tenancy to `DB_URL`. [In
Laravel
11](96508d43ec (diff-e4382b565f69d02de16eef89c8d5ca2b60a679ffca90ab8b7d85fbd78f30075bR35)),
the `DATABASE_URL` env variable got renamed to `DB_URL` in
`config/database.php`. The env var got renamed quite a while ago, so
referencing `DATABASE_URL` in Tenancy's comments and tests was a bit
confusing.
### Minor cleanup
The `harden prevents tenants from using the database of another tenant`
test now reads the central connection name from
`config('tenancy.database.central_connection')` instead of hardcoding
`'central'` for consistency with the other tests.
The bootstrapper should be able to map nested tenant attributes to config, and `getAttribute()` doesn't support dot notation, so e.g. `$tenant->getAttribute('logging.slackUrl')` would return `null` instead of the actual attribute, so the test failed.
`data_get()` supports dot notation, so we'll use that.
Resubmission of #1369 (by @lordofthebrain), changes adapted to v4. Also
added a test (passes with the MigrateFresh changes, fails without them).
---------
Co-authored-by: lordofthebrain <f.mangelsdorf@gmail.com>
Co-authored-by: Samuel Stancl <samuel@archte.ch>
### Parameter validation
In `statement()` calls of `TenantDatabaseManager`s, use parameter
binding when possible. When that's not possible, validate the parameters
using `validateParameter()` or `validatePassword()`.
Passwords use a less strict allowlist than other parameters (e.g. DB
names), since passwords tend to use more special characters but we can
afford to be more restrictive in the generic `validateParameter()`.
In `SQLiteDatabaseManager`, names of file-based databases are validated
(in `createDatabase`, `deleteDatabase` and `databaseExists`) using a
similar allowlist to the (non-password) parameters in other DB managers,
with an additional character: `.` (this addition is necessary since
file-based SQLite databases end with `.sqlite`).
### DatabaseTenancyBootstrapper - harden() and the lost test file
While checking for more places that could use validation, I realized
that it's possible to update tenant's db_name to the central DB or the
DB of another tenant. Added the `DatabaseTenancyBootstrapper::$harden`
property -- setting it to true prevents tenants from connecting to the
wrong databases (`RuntimeException` is thrown after connecting to the
wrong database).
Also, the DatabaseTenancyBootstrapper test file was ignored while
running tests because it lacked the `Test` suffix. Added the suffix and
fixed the broken `DATABASE_URL` test in the file.
### SQLiteDatabaseManager - respect static $path property in
makeConnectionConfig()
SQLiteDatabaseManager had a bug in `makeConnectionConfig`: the method
didn't respect the static `$path` property, it used `database_path()`
instead. Added a regression test for that. Also recognizing in-memory
SQLite databases (using `isInMemory()`) is more strict now so that
simply having a db_name with `_tenancy_inmemory_` somewhere in the name
doesn't make a file-based database considered in-memory.
### MySQLDatabaseManager - charset and collation defaulting
Creating databases with `null` charsets and collations resulted in a
`QueryException`, since null isn't a valid charset/collation. To solve
that, in the `CREATE DATABASE` statement in MySQLDatabaseManager, only
add charset/collation to the statement if they are not null.
MySQL defaults to the server's charset and collation, so it's safe to
not pass any charset/collation in the `CREATE DATABASE` statement and
let MySQL choose. Also, if e.g. collation is non-null and charset is
null, MySQL will use a charset compatible with the used collation, and
this works both ways.
---------
Co-authored-by: Samuel Stancl <samuel@archte.ch>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
`globalCache` should always use the central connection, but when using a
`database`-driver cache store with `DatabaseTenancyBootstrapper`, it
does not (with the exception of `DatabaseCacheBootstrapper`, explained
below).
`globalCache` creates a fresh `CacheManager` each time it's resolved
(it's a `bind`, not a `singleton`). A freshly-created manager builds its
database stores using the current default DB connection. When
`DatabaseTenancyBootstrapper` is active, that default is `tenant`. So
`globalCache` in tenant context points at the tenant DB. Specifically,
`CachedTenantResolver` stores cached tenant lookups via `globalCache`.
When a domain is deleted in tenant context, the invalidation logic calls
`globalCache->forget(...)`, but that hits the tenant DB, while the
resolver cache entry is in the central DB. `globalCache->forget(...)`
doesn't actually do anything in that case.
With `DatabaseCacheBootstrapper`, this is already handled. `globalCache`
is always central because it sets
`TenancyServiceProvider::$adjustCacheManagerUsing` to a callback that
explicitly restores the central connection on `globalCache`'s stores.
To fix this, after constructing the fresh CacheManager in the
globalCache binding, explicitly set the connection of every
database-driver store to its configured connection value, **falling back
to central_connection** when the config value is null (null value =
inherits whatever the current default DB connection is).
This is sufficient for `CacheTenancyBootstrapper` and any other
bootstrapper that doesn't explicitly set the store's DB connection. For
`DatabaseCacheBootstrapper` specifically, this alone is not enough since
it explicitly sets the store's config connection to 'tenant'. That's why
DatabaseCacheBootstrapper's `$adjustCacheManagerUsing` callback runs
after and overrides those stores back to the original (central)
connection.
> In short: `makeDatabaseCacheStoresCentral()` handles stores with a
`null` connection config (falls back to central).
`$adjustCacheManagerUsing` handles the `DatabaseCacheBootstrapper` case
where the config is explicitly set to 'tenant'.
Added datasets that use the database cache store +
CacheTenancyBootstrapper to the relevant tests (globalCache and
invalidation) to test regression
(0cf7043b73),
and the changes mentioned above
(https://github.com/archtechx/tenancy/pull/1462/commits/5e65c67ea0daf98f57f2a6a7b0e1937bbc397a56)
make these tests pass.
---------
Co-authored-by: Samuel Stancl <samuel@archte.ch>
> Minor breaking change: clearing pending_since no longer fires Eloquent
events, PullingPendingTenant is now fired at a different point in the
lifecycle and does not guarantee the tenant will actually be pulled.
`pullPendingFromPool` had a race condition when user A attempted to pull
a tenant at the same time as user B. Both could end up grabbing the same
tenant, and the result was unexpected, e.g. one of them ending up with
no pending tenant pulled at all even though there was a pending tenant
in the pool.
instead of selecting a pending tenant and updating the same model, we
now run `update()` conditionally -- it clears `pending_since` _only_ if
the tenant is still pending, and we check the affected row count. Only
one process can get a row back, the other gets 0 and retries with the
next pending candidate in the pool. The loop always terminates since
every lost claim means the pool shrank by one. Eventually it's empty and
we create a new tenant (or return null).
The claim and the attribute update happen in a single transaction now,
so if updating `$attributes` fails, the claim rolls back and the tenant
stays in the pool.
Added a regression test that simulates a concurrent "steal"
synchronously via a PullingPendingTenant listener. Fails with the old
code, passes with the HasPending changes.
Very minor BC:
- Clearing `pending_since` no longer fires model updating/updated events
(since the update goes through query builder). `PendingTenantPulled`
still fires the same as before and is the listener you'd want to use
anyway.
- `PullingPendingTenant` now fires before the claim (and outside the
transaction), so it can fire more than once with concurrent pulls (e.g.
when a tenant gets claimed by someone else). `PendingTenantPulled` is
still the one that fires exactly once for the actually pulled tenant.
---------
Co-authored-by: Samuel Stancl <samuel@archte.ch>
Adds a --skip-tenants option to all tenant artisan commands
(`tenants:run`, `tenants:migrate`, `tenants:rollback`, `tenants:seed`,
`tenants:up`, `tenants:down`).
The option is the complement of the existing `--tenants` option instead
of specifying which tenants to include, you specify which to exclude.
---------
Co-authored-by: Jimish Gamit <unique.jimish@gmail.com>
Co-authored-by: Samuel Stancl <samuel@archte.ch>
Co-authored-by: lukinovec <lukinovec@gmail.com>
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>
> Minor breaking change: Pending tenants would previously go through the
creation pipeline as *not* pending and would only be marked as pending
after full creation. Now, pending tenants go through the creation
process with pending_since set from the start.
Pending tenants aren't getting their `pending_since` set until they're
created completely (e.g. their DB was created, migrated and seeded --
first, the tenant is created fully, and only after that, the tenant is
updated to have `pending_since`). This is a problem if someone wants to
e.g. add a job to the `DatabaseCreated` job pipeline that would check
`$this->tenant->pending()`. Since at the point of `DatabaseCreated`, the
tenant's `pending_since` isn't set yet, `$this->tenant->pending()`
returns `false`, even for tenants created using `createPending()`. So
instead of letting the pending tenant get fully created, and only after
that, setting its `pending_since` (using `update()`), we now set
`pending_since` in `create()`. `CreatingPendingTenant` is now dispatched
from the `static::creating` hook, and `PendingTenantCreated` is
dispatched from `static::created` for consistency.
Setting `pending_since` right in `create()` made the `MigrateDatabase`
and `SeedDatabase` jobs exclude the pending tenants during their
creation if the `tenancy.pending.include_in_queries` config was set to
`false` -- in that case, these jobs would never migrate or seed the
databases of pending tenants. So these jobs now pass `--with-pending` to
their underlying commands, with the value set in their `$includePending`
static property (`true` by default). This overrides the
`tenancy.pending.include_in_queries` config -- unless the
`$includePending` properties are set to `false`, these jobs will always
include pending tenants.
The `--with-pending` tenant command option originally worked just to
opt-in for including pending tenants in the command. Now,
`--with-pending` can accept values (`true`/`1` or `false`/`0`), so e.g.
- `tenants:run foo` with
`--with-pending`/`--with-pending=true`/`--with-pending=1` includes
pending tenants
- `tenants:run foo` with `--with-pending=false`/`--with-pending=0`
**excludes** pending tenants (also `--with-pending=foobar` -- invalid
input, considered `false`)
Passing `--with-pending` makes the command bypass the
`tenancy.pending.include_in_queries` config (so e.g. if
`tenancy.pending.include_in_queries` is set to `true`, and
`--with-pending=false` is passed to a command, the command will exclude
pending tenants). When `--with-pending` is not passed, the command will
include or exclude pending tenants based on the
`tenancy.pending.include_in_queries` config.
---------
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
> Minor breaking change: `session('tenancy_impersonating')` doesn't work
anymore. Use `session('tenancy_impersonation_guard')` instead.
The 'tenancy_impersonating' session variable got replaced by
'tenancy_impersonation_guard'. `UserImpersonation::stopImpersonating()`
now calls `logout()` on the guard retrieved by
`session()->get('tenancy_impersonation_guard')` instead of calling
`logout()` on the _current_ auth guard. Now. if you create the
impersonation token with guard 'web', and call
`UserImpersonation::stopImpersonating()`, for example in a route that
has the `auth:sanctum` middleware (= the current guard in that route
would be `RequestGuard` which doesn't even have the `logout()` method --
not the guard for which the impersonation token was created), the method
will correctly log the user out of the 'web' guard using which he was
actually authenticated instead of the current guard of the visited route
(which doesn't have to be the same guard for which impersonation
started).
`UserImpersonation::stopImpersonating()` now also accepts the `$logout`
parameter, which is `true` by default. If `false` is passed, the method
just forgets `tenancy_impersonation_guard` from session without logging
out.
`UserImpersonation::stopImpersonating()` now throws an exception if
impersonation wasn't active at the point of calling the method.
---------
Co-authored-by: Samuel Stancl <samuel@archte.ch>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
The PreventAcessFromUnwantedDomains MW had the
`tenancy()->routeIsUniversal($route)` check either for returning early,
or it was a leftover from some older implementation, so I removed it.
The middleware aborts if the
`$this->accessingTenantRouteFromCentralDomain($request, $route) ||
$this->accessingCentralRouteFromTenantDomain($request, $route)` check
passes. Meaning, **for the middleware to abort, the route has to be
either in central or tenant mode**. When the route is in universal mode,
the middleware will never reach `return $abortRequest()`. `return
$next($request)` will always get reached, even when the `||
tenancy()->routeIsUniversal($route)` check is deleted from the previous
condition, so that check was basically useless.
Since the docblock for the class does mention the behavior for universal
routes explicitly, we've instead added a comment documenting that things
work this way. That's probably the most reasonable way to have this
explicit behavior for universal routes easily understandable in this
fairly complex logic without redundant code.
Resolves#1418
---------
Co-authored-by: Samuel Štancl <samuel@archte.ch>
Added a failing test for determining if a host is a subdomain, then
fixed `DomainTenantResolver::isSubdomain()` (similar fix as in #1423)
and a related assertion.
Previously, while having `tenancy.identification.central_domains` set to
e.g. `['site.com']`, the `isSubdomain()` check consider `tenantsite.com`
a subdomain because it ends with `site.com`. Now, instead of the
`endsWith()` check, the method checks if the passed domain is in the
configured central domains. If it is, it returns `false`. Otherwise,
loop through all the central domains and check if the passed domain
matches any of the central domains prefixed with a dot (e.g.
`tenant.site.com` would be considered a subdomain, `tenant.site.com`
wouldn't).
Because in InitializeTenancyByDomainOrSubdomain, if tenancy fails to
initialize using a subdomain (before this PR's changes, e.g.
`tenantsite.com` would be considered a subdomain, and `tenantsite` would
be used for initializing tenancy), it'll catch the exception and use the
whole domain for identification instead, this error will likely never be
noticed in real-world usage. So this PR corrects the subdomain detection
logic, but the real-world impact of that is negligible.
> Note: The subdomain error catching logic in domainOrSubdomain ID MW
was added in v4. If we applied this change in v3, it'd fix a real issue
where domainOrSubdomain ID MW would just fail at the subdomain
initialization, without attempting domain initialization after the
failure.
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
Database deletion is now skipped by default if the tenant has the
`create_database` internal attribute set to false, meaning it was likely
created without a database. This skip can be opted out of by changing a
static property.
It also adds an opt-in static property for ignoring any other failures
during database deletion, to allow continuing execution of the delete
pipeline.
---------
Co-authored-by: Samuel Štancl <samuel@archte.ch>
The `CreateTenantStorage` and `DeleteTenantStorage` listeners were used
alongside JobPipelines. When the `TenantCreated` JobPipeline had
`shouldBeQueued(true)` and the `Listeners\CreateTenantStorage` was
uncommented, the listener would throw an exception
(`Stancl\Tenancy\Database\Exceptions\TenantDatabaseDoesNotExistException
Database tenantX.sqlite does not exist.`) because at the time of
executing the listener, the tenant DB wasn't created yet.
The same issue could likely also occur in the `DeleteTenantStorage`
listener as it uses `tenancy()->run()` to resolve the tenant's storage
path which wouldn't work if the tenant's database (or other resources)
was already deleted, making initialization impossible.
This PR changes `DeleteTenantStorage` into a job and puts it (commented)
into the job pipeline, so that it can be queued with the rest of the
jobs. It also removes `CreateTenantStorage` because it should be
redundant with the FilesystemTenancyBootstrapper creating the same paths
automatically when storage path is suffixed.
The old classes are kept but deprecated for backwards compatibility.
We've also added some edge case hardening to `DeleteTenantStorage` to
make sure it never deletes the central storage path directory, which
previously could in theory occur due to a misconfiguration if a user
enabled this job/listener but disabled storage path suffixing.
Co-authored-by: Samuel Štancl <samuel@archte.ch>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
At the moment, `where()` cannot be used correctly while using
`withoutPending()`. For example, if we have a single non-pending tenant
in our DB (with ID 'foo'), queries like
`Tenant::withoutPending()->where('id', 'nonexistent')->first()`will
incorrectly return the non-pending tenant ('foo').
This is because `withoutPending()` does
`$builder->whereNull('data->pending_since')->orWhereNull('data')`. These
two aren't grouped, so `withoutPending()->where('id', 'nonexistent')`
basically translates to "WHERE data->pending_since IS NULL **OR (data IS
NULL AND id = 'nonexistent')**". So the query will include all tenants
whose `pending_since` is null (= all non-pending tenants).
Grouping `->whereNull('data->pending_since')->orWhereNull('data')` in a
closure passed to a separate `where()` fixes this issue.
phpstan started failing with '... implements generic interface
Illuminate\Database\Eloquent\Scope but does not specify its types:
TModel'. We solve this by adding an implements docblock to the scopes
implementing that interface. They're fairly generic - we just use the
Model type itself in the code - so we use Model for the type parameter.
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
