mirror of
https://github.com/archtechx/tenancy.git
synced 2026-08-06 12:04:04 +00:00
553f57a8ad
> 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>
|
||
|---|---|---|
| .. | ||
| Console | ||
| EarlyIdentification | ||
| mail_migrations | ||
| ResourceSyncing | ||
| session_migrations | ||
| synced_resource_migrations | ||
| 2019_08_08_000001_add_custom_column.php | ||
| 2023_08_08_000001_add_domain_column.php | ||
| CacheService.php | ||
| defaultHttpKernelv6.stub | ||
| defaultHttpKernelv7.stub | ||
| ExampleSeeder.php | ||
| HasMiddlewareController.php | ||
| HttpKernel.php | ||
| modifiedHttpKernelv6.stub | ||
| modifiedHttpKernelv7.stub | ||
| SingleDomainTenant.php | ||
| SpecificCacheStoreService.php | ||
| tenant-schema.dump | ||
| Tenant.php | ||
| TestingBroadcaster.php | ||
| TestSeeder.php | ||
| User.php | ||