> 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 `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>
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).
* 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>
* 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 universal route logic part of tbe early ID trait
* Add requstHasTenant to prevent access MW, add todo@samuel
* Delete PathIdentificationManager, move the used methods appropriately
* Correct and refactor code related to the deleted PathIdentificationManager class
* Add docblock
* Fix code style (php-cs-fixer)
* refactor globalStackMiddleware()
* remove todos [ci skip]
* refactor routeMiddleware()
* revert bool assertions
* revert more changes
---------
Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
Co-authored-by: Samuel Štancl <samuel@archte.ch>
