> Minor breaking change: BroadcastingConfigBootstrapper::$broadcaster property removed, we now just use the default broadcaster for setting mappings based on a preset
### Broadcasting auth route problem (`Broadcast` facade always uses
central `BroadcastManager`)
> Note: Tested with Pusher and Reverb. Also, this is the primary problem
that this PR solves.
When using `BroadcastingConfigBootstrapper` for broadcasting on private
channels with multiple broadcasting apps (= each tenant has its own
Pusher/Reverb/... app and credentials), the auth requests sent to the
broadcasting server use the central broadcasting credentials. This is
because the `Broadcast` facade (used in
`BroadcastController::authenticate` like `Broadcast::auth($request)` to
authorize the channels **and then retrieve the auth key that's then sent
to the broadcasting server**) keeps using the `BroadcastManager`
instance resolved in the central context instead of the tenant
`BroadcastManager`. Calling `withBroadcasting()` in `bootstrap/app.php`
results in calling `Broadcast::routes()` in the central context, which
resolves and stores the central `BroadcastManager` (which is never
cleared, and is used in tenant context).
Clearing the facade's resolved instance
(`Illuminate\Contracts\Broadcasting\Factory`) in
`BroadcastingConfigBootstrapper::bootstrap()` forces the facade to
re-resolve `Factory` (= `BroadcastManager`) on the next use, so the next
`Broadcast::auth()` call uses the tenant `BroadcastManager` in the
tenant context (and clearing the resolved instance in `revert()` makes
the `Broadcast` facade use the central `BroadcastManager` again), and
the credentials from the current config will be used for authentication.
### Central `BroadcastManager` doesn't pass custom driver creators to
the tenant manager
When registering a custom driver creator
(`app(BroadcastManager::class)->extend('custom-driver', fn($app,
$config) => new CustomBroadcaster(...))`) in the central context (e.g.
in `BroadcastServiceProvider`/`AppServiceProvider`, or in our case, in
the tests), the creator won't be available in the tenant context.
Calling e.g. `app(BroadcastManager::class)->driver('custom-driver')` in
the tenant context (if the creator was registered only in the central
context) would throw `InvalidArgumentException` ("Driver [custom-driver]
is not supported.").
To fix that, register the original (central) `BroadcastManager`'s custom
creators in the new `BroadcastManager` in
`BroadcastingConfigBootstrapper::bootstrap()`.
> Note: This was always a problem, the tests just never caught it. The
original test where we used a custom driver creator actually registered
the same creator on each context switch (see the
`$registerTestingBroadcaster()` calls in the removed `BroadcastingTest`
file) -- if the creator was registered just once (which is what we do
now, after the fix), the test would fail. The test registered the driver
creator repeatedly which worked around custom creators not persisting
after switching between contexts.
### Central `Broadcaster` instance bound in tenant context
> Note: This section describes the problem with resolving/injecting the
`Broadcaster` contract directly (via DI or `app(Broadcaster::class)`).
The `extend()` call that fixes it also plays a second, bigger role --
see the "How `BroadcastingConfigBootstrapper::bootstrap()` works"
section below.
The problem was that resolving
`Illuminate\Contracts\Broadcasting\Broadcaster::class` in tenant context
returned the broadcaster instance from the central context (Laravel
binds the contract as a singleton, resolved from the default driver's
config as it was at resolution time -- before tenancy was initialized).
Fixed by making `Broadcaster::class` resolve to the current
`BroadcastManager`'s default broadcaster (= the tenant broadcaster in
tenant context) via `app->extend()`.
Since the manager caches its broadcasters (see "Broadcasters are
resolved once per tenancy initialization" below),
`app(Broadcaster::class)` and `Broadcast::driver()` return the same
instance in every context -- same as Laravel's default behavior.
### How `BroadcastingConfigBootstrapper::bootstrap()` works (the
`extend()` calls)
Both `extend()` calls run immediately during `bootstrap()`, not lazily
on some later resolution -- `extend()` executes the closure right away
when the extended singleton is already resolved, and both singletons are
resolved at the top of `bootstrap()` (where we save the central
instances for `revert()`).
Since the singletons are already resolved, the container also doesn't
store the closures as extenders -- they run once during `bootstrap()`
and that's it. Once `revert()` restores the original instances,
resolving either singleton again (even after a `forgetInstance()`) goes
purely through the original bindings. So when tenancy initializes:
1. `setConfig()` maps the tenant's properties to the broadcasting
config.
2. The `BroadcastManager` extend swaps the bound manager for a fresh one
with an empty driver cache and passes the central manager's custom
driver creators to it.
3. The `Broadcaster` contract extend calls `connection()` on the tenant
manager, which resolves the default broadcaster using the updated
(tenant) config and caches it as the manager's default driver. The
central broadcaster's auth properties -- the channel auth closures,
their options, and the authenticated user callback -- are then copied
onto this tenant broadcaster.
4. `Broadcast::clearResolvedInstance()` makes the facade re-resolve on
the next call, so it returns the tenant manager instead of the stale
central one.
When `/broadcasting/auth` gets hit later, `Broadcast::auth()` goes
through the tenant manager's `driver()`, which returns the broadcaster
cached in step 3 (built with the tenant credentials, using the copied
channel auth closures). This means the `Broadcaster` contract extend
isn't just for code that resolves or injects the contract directly --
it's also what makes channel auth work in tenant context. Without it,
`app(Broadcaster::class)` would keep returning the stale central
broadcaster, and the tenant broadcaster's `$channels` would stay empty,
so `Broadcast::auth()` would throw a 403 for every channel registered in
the central context. The same goes for user authentication --
`resolveAuthenticatedUser()` has no fallback, so without the copied
authenticated user callback (registered via
`Broadcast::resolveAuthenticatedUserUsing()`), `/broadcasting/user-auth`
would throw a 403 in tenant context.
`revert()` restores the original central manager and broadcaster
instances via `instance()` (they're saved at the top of `bootstrap()`
and nothing touches them while tenancy is initialized) and clears the
facade's resolved instance again.
### Broadcasters are resolved once per tenancy initialization,
`TenancyBroadcastManager` is removed
Originally, `TenancyBroadcastManager` re-resolved the broadcasters
listed in its `$tenantBroadcasters` static property on every retrieval.
Earlier, we considered that good since it meant direct config changes in
tenant context were picked up immediately. [As discussed in the
review](https://github.com/archtechx/tenancy/pull/1448#discussion_r3555941319),
there's probably no real use case for that, and the re-resolving
actually caused a subtle bug: channels registered via
`Broadcast::channel()` in tenant context got silently lost on the next
retrieval (e.g. during a `/broadcasting/auth` request), because each
re-resolution started over from a copy of the central channels -- making
the auth request fail with a 403 as if the channel was never registered.
Now, `TenancyBroadcastManager` is removed entirely.
`BroadcastingConfigBootstrapper` binds a fresh, base `BroadcastManager`
with no cached broadcasters, so the broadcasters get resolved using the
tenant's credentials on first use and stay cached (like in the parent
manager) for the duration of the tenant's context. The central
broadcaster's auth properties (the channel auth closures, their options,
and the authenticated user callback) are copied to the tenant
broadcaster directly in the `Broadcaster` contract's `extend()` closure
-- since Laravel only ever uses the *default* broadcaster's channel auth
closures for broadcasting auth (both `Broadcast::channel()` and
`Broadcast::auth()` go through the default broadcaster), the properties
only have to be copied to the default broadcaster.
The channel auth closures are always *only* on the default broadcaster,
even in plain Laravel. Every `Broadcast::channel()` call adds an entry
to the same `$channels` array on the default broadcaster -- a channel
with its own authorization rules is just one of those entries, so it
gets copied along with the rest. The only way to register a closure on a
non-default broadcaster is calling
`Broadcast::driver('foobar')->channel(...)` explicitly, and nothing in
Laravel's auth flow ever reads those (`/broadcasting/auth` always
authenticates using the default broadcaster).
This all means that:
- `TenancyBroadcastManager` is removed. Its `$tenantBroadcasters`
property's purpose was listing the broadcasters to re-resolve (and pass
the central channel auth closures to) -- custom drivers now work without
the need to configure anything, and the closure copying is a few lines
in the bootstrapper instead of a manager override.
- Channels registered via `Broadcast::channel()` in tenant context
persist for the duration of that context. They don't leak into other
tenants' contexts or into the central context.
- For broadcasters to use updated credentials, tenancy has to be
reinitialized. Direct broadcasting config changes made in tenant context
aren't picked up by the broadcasters, and tenant property changes are
only mapped to config in `bootstrap()`. An already-injected (= stale)
`Broadcaster` instance additionally needs to be obtained again after
reinitialization -- reinitializing swaps the bound instance, so a
previously injected instance keeps using the old credentials.
### NOTE: Already-connected clients stop receiving broadcasts after a
credential update
Updating a tenant's credentials doesn't disconnect already-connected
clients (tested with Reverb) -- they stay connected using the old key,
but they stop receiving broadcasts, since broadcasts sent after the
update are sent with the new credentials.
The frontend has to reconnect using the new key (e.g. by refreshing the
page). Meaning, clients can't be notified about the credential change
through websockets themselves -- a broadcast sent after the update
already uses the new credentials, so it won't reach clients connected
with the old key. So for things like notifying clients in response to
the credential changes, a different mechanism is needed.
### Credential map: overriding presets (BroadcastingConfigBootstrapper)
Previously, credential mappings from `$mapPresets` overrode mappings
defined in `$credentialsMap`. If someone used e.g. Pusher and wanted to
override some of that preset's mappings, e.g. use 'pusher_app_key'
instead of 'pusher_key' by specifying 'pusher_app_key' in
`$credentialsMap`, the preset's mapping ('pusher_key') would still be
used.
Fixed that by reversing the `array_merge()` order in
`BroadcastingConfigBootstrapper::__construct()`.
### **MINOR BC:** `BroadcastingConfigBootstrapper::$broadcaster`
property removed
`BroadcastingConfigBootstrapper::$broadcaster` determined which
`$mapPresets` preset to apply instead of just using
`broadcasting.default`. Its only effect was applying a preset for a
connection other than the default -- a connection nothing resolves, so
it did nothing useful. Set `$credentialsMap` directly if you need a
non-default mapping.
Removing it also let us drop the static mutation from the constructor,
which used to overwrite the user-configured `$credentialsMap`. The
preset merge now happens locally in `setConfig()`.
### Tests
Deleted the BroadcastingTest file, moved the tests to appropriate
bootstrapper test files.
Added tests
- for mapping tenant properties to broadcaster credentials (including
keeping the central config values when a tenant doesn't have a mapped
property, and reverting to the central credentials after a tenant with
credential overrides)
- for the rest of the changes mentioned above (including a regression
test for the tenant-context channel registration bug)
- for copying the channel options and the authenticated user callback
along with the channel auth closures
- for the bound `Broadcaster` and the manager's default broadcaster
being the same instance in central and tenant contexts
- for broadcasters that only implement the `Broadcaster` contract
(instead of extending the abstract class), and for configuring which map
preset is used via the `$broadcaster` property
Also improved the existing tests.
---------
Co-authored-by: Samuel Stancl <samuel@archte.ch>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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>
`TriggerSyncingEvents` registered the pivot attach listener on `saved`,
which fires on both inserts and updates. So updating pivot columns on a
`TenantPivot` (e.g. via `updateExistingPivot()`) re-ran the attach flow
and tried to create the tenant resource again, causing a duplicate entry
error.
Switched the listener to `created` so we only attach when the pivot
record is first created (detaching already uses `deleting`, so using
`created` makes things a bit more consistent).
Added a regression test before the fix
(https://github.com/archtechx/tenancy/pull/1469/commits/5e3eb4322cc487d540796317e813ac2ffe076646).
The fix
(https://github.com/archtechx/tenancy/pull/1469/commits/d2fb4bc0d524cd98ec680e973b791f475afc3548)
then made the test pass.
Also improved the `saving` listener's comment a bit so that it's clear
where the "central resource not available" exception actually comes
from, since that wasn't obvious (also added the `@throws` annotation to
`getCentralResourceAndTenant()`).
Closes#1467
### 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.
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>
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.
Some bootstrappers read attributes of the tenant during bootstrap() but
don't respond to changes made to the tenant afterwards.
Therefore, when making changes to the tenant that'd affect the behavior
of a bootstrapper, it's necessary to reinitialize tenancy (if it matters
that changes are reflected immediately). This adds a convenience helper
for that purpose.
- Update ci.yml and composer.json
- Wrap single database tenancy trait scopes in whenBooted()
- Update SessionSeparationTest to use laravel-cache- prefix in L13
and laravel_cache_ in <=L12. Our own prefix remains tenant_%tenant%_
(as configured in tenancy.cache.prefix). We could update this to be
tenant-%tenant%- from now on for consistency with Laravel's prefixes
(changed in https://github.com/laravel/framework/pull/56172) but I'm
not sure yet. _ seems to read a bit better but perhaps consistency
is more important. We may change this later and it can be adjusted
in userland easily (since it's just a config option).
Using `URL::temporarySignedRoute()` in tenant context with
`UrlGeneratorBootstrapper` enabled doesn't work the same as `route()`.
The bypass parameter doesn't actually bypass the route name prefixing.
`route()` is called in the `parent::temporarySignedRoute()` call, and
because the bypass parameter is removed before calling
`parent::temporarySignedRoute()`, the underlying `route()` call doesn't
get the bypass parameter and it ends up attempting to generate URL for a
route with the name prefixed with 'tenant.'.
This PR adds the bypass parameter back after `prepareRouteInputs()`, so
that `parent::temporarySignedRoute()` receives it, and the underlying
`route()` call respects it. Also added basic tests for the
`URL::temporarySignedRoute()` behavior (the new bypass parameter test
works as a regression test).
The old names of the class and method were misleading. We don't
actually need any relation. And we don't even need a model instance
as we were returning previously -- the only use of that method was
in TriggerSyncingEvents which would immediately use ::class on the
returned value. Therefore, all we are asking for in this interface
is just the central resource class.
Also move pivot record deletion to that listener and improve tests
The 'tenant pivot records are deleted along with the tenants to which
they belong to' test is failing in this commit -- the listener
for deleting mappings when a *tenant* is deleted is only implemented
in the next commit. The only change done here is to re-add FKs
(necessary for passing *in this commit* in that specific dataset
variant) that were removed from the default test migration as we now
have the DeleteResourceMapping listener that's enabled by default.
Previously, tenant identification middleware was typically specified
for the cloned route by "inheriting" it from the central route, which
necessarily meant that the central route had to also be marked as
universal so it could continue working in the central context --
despite presumably not being usable in the tenant context, thus being
universal for no proper reason. In such cases, universal routes were
used mainly as a mechanism for specifying the tenant identification
middleware to use on the cloned tenant route.
Given that recent refactors of the cloning feature have made it more
customizable and a bit nicer to use "multiple times", i.e. run handle()
with a few different configurations of the action, letting the
developer specify the used tenant middleware using a method like this
only makes sense.
The feature also becomes more independently usable and not just a
"hack for universal routes with path identification".
In such a case, the cloned route will actually *override* the original
route, rather than being unused as the original docblock claimed.
Also adds a static make() function for convenience.
Previously, if a universal route was cloned without a
cloneRoutesWithMiddleware(['universal']) call, i.e. it had both
'clone' and 'universal' flags, with only the former triggering cloning,
the 'universal' flag would be included in the middleware of the cloned
route.
Now, we make sure to remove all context flags -- central, tenant,
universal -- in the first step of processing middleware, before adding
just 'tenant'.
`?` parameters are not supported in these statements, so we have to use
string interpolation like in other related code.
---------
Co-authored-by: Samuel Štancl <samuel@archte.ch>
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).
This PR makes the expired/invalid tenant impersonation tokens get
deleted instead of just aborting with 403.
The PR also adds a command (ClearExpiredImpersonationTokens) used like
`php artisan tenants:purge-impersonation-tokens`. As the name suggests,
it clears all expired impersonation tokens (= tokens older than
`UserImpersonation::$ttl`).
Resolves#1348
---------
Co-authored-by: Samuel Štancl <samuel@archte.ch>
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>
This method lets the user specify default values for custom
non-nullable columns. The primary use case is when the tenants table
has a column like 'slug' and createPending() is called with no
value for 'slug'. This would produce an exception due to the column
having no default value.
Here, getPendingAttributes() can set an initial dummy slug (like a
randomly generated string) before it's overwritten during a pull.
getPendingAttributes() accepts an $attributes array which corresponds
to the attributes passed to createPending(). The array returned from
getPendingAttributes() is ultimately merged with $attributes, so
the user doesn't need to use the $attributes value in
getPendingAttributes(), however it serves to provide more context when
the pending attributes might be dependent on $attributes and therefore
derived from the $attributes actually being used.
Also fixed the `finally` branch in createPending() as it was
potentially referencing the $tenant variable before it was initialized.
This commit adds support for building a docker image based on PHP 8.5
(RC). It also removes some unused code in tests that was triggering
deprecation warnings. For similar deprecation warnings coming from
testbench we have a temporary patch script until this is resolved
upstream.
This commit also adds logic to the DisallowSqliteAttach feature
leveraging the new native setAuthorizer() method, instead of loading
a compiled extension.
We also remove the unused `php` parameter from ci.yml
From the perspective of the master branch, this commit merges in a
few small breaking changes from the dev branch:
6b0066c5ef
- Make pullPendingFromPool() $firstOrCreate arg default to false
(pullPending() is now a direct alias for pullPendingFromPool() with
default $firstOrCreate=true)
- See full commit message for other changes. They shouldn't be breaking
though.
13a2209f11
- Remove $WAL static property. We instead just let Laravel use its
journal_mode config now
This merge also adds a deprecation:
b320f8f33d
- Deprecate TenantConfig feature in favor of TenantConfigBootstrapper
This commit also corrects an Event::fake() call in a separate test, as
general Event::fake() calls without specified events can lead to
incorrect (and difficult to debug) behavior in some cases, since
Tenancy depends on the event system being functional.
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.
- (BC BREAK) Remove $WAL static property. We instead just let
Laravel use its journal_mode config now
- Remove journal, wal, and shm files when deleting tenant DB
- Check that the system is 64-bit when using NoAttach (we don't
build 32 bit extensions)
- Use local static instead of a class static property for caching
loadExtensionSupported
Features are now *always* bootstrapped, even if Tenancy is not resolved
from the container.
Previous implementations include
https://github.com/tenancy-for-laravel/v4/pull/19https://github.com/archtechx/tenancy/pull/1021
Bug originally reported here
https://github.com/archtechx/tenancy/issues/949
This implementation is much simpler, we do not distinguish between
features that should be "always bootstrapped" and features that should
only be bootstrapped after Tenancy is resolved. All features should work
without issues if they're bootstrapped when TSP::boot() is called. We
also add a Tenancy::bootstrapFeatures() method that can be used to
bootstrap any features dynamically added at runtime that weren't
bootstrapped in TSP::boot(). The function keeps track of which features
were already bootstrapped so it doesn't bootstrap them again.
The only potentialy risky thing in this implementation is that we're now
resolving Tenancy in TSP::boot() (previously Tenancy was not being
resolved) but that shouldn't be causing any issues.
* Fix ViteBundler not affecting Vite static calls
Replace custom Vite class override with Vite::createAssetPathsUsing() to ensure ViteBundler works for both container and static usage when asset_helper_override is enabled.
Fixes#1388
* Remove redundant logic from tests
* Simplify test further
* Re-add file creation logic
---------
Co-authored-by: Samuel Štancl <samuel@archte.ch>
* Make the `--database` option passed to `tenants:migrate` use the passed connection as the tenant connection template
* Reset template connection regardless of process count
---------
Co-authored-by: Samuel Štancl <samuel@archte.ch>
