Skip to content
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
7d749eb
BroadcastingConfigBootstrapper: test mapping credentials
lukinovec Mar 31, 2026
2ecc94d
BroadcastingConfigBootstrapper: test persistence of custom driver cre…
lukinovec Mar 31, 2026
fafd082
Fix typo in test
lukinovec Mar 31, 2026
c653c51
BroadcastingConfigBootstrapper: make tenant manager inherit central m…
lukinovec Mar 31, 2026
b1e91f1
BroadcastingConfigBootstrapper: make `Broadcaster::class` resolve to …
lukinovec Mar 31, 2026
65beecf
BroadcastingConfigBootstrapper: clear the `Broadcast` facade's resolv…
lukinovec Mar 31, 2026
0b860ea
Fix code style (php-cs-fixer)
github-actions[bot] Mar 31, 2026
c4f4451
Add assertions for the config of the bound manager's driver
lukinovec Mar 31, 2026
d939866
Fix custom creator assertions
lukinovec Mar 31, 2026
28b6119
TenancyBroadcastManager: update docblocks
lukinovec Apr 1, 2026
0fbe1bc
TenancyBroadcastManager: delete `Broadcaster` singleton binding
lukinovec Apr 2, 2026
b6c035c
Improve comments
lukinovec Apr 2, 2026
bbe2ff0
BroadcastingTest: update channel inheritance test
lukinovec Apr 2, 2026
b2add06
Delete BroadcastingTest
lukinovec Apr 2, 2026
9e9bedc
Fix code style (php-cs-fixer)
github-actions[bot] Apr 2, 2026
4b1cc9c
Improve comments
lukinovec Apr 2, 2026
fc45e09
Update BroadcastingConfigBootstrapperTest
lukinovec Apr 3, 2026
6b99921
BroadcastingConfigBootstrapper: correct `$credentialsMap` array_merge…
lukinovec Apr 3, 2026
29dd23d
BroadcastingConfigBootstrapperTest: add 'reverb' driver to datasets
lukinovec Apr 3, 2026
4937a74
BroadcastingConfigBootstrapper and TenancyBroadcastManager: comments
lukinovec Apr 3, 2026
c831393
Update comment
lukinovec Apr 3, 2026
ef476c5
Polish comments
lukinovec Apr 3, 2026
f8528fc
Add 'reverb' to `TenancyBroadcastManager::$tenantBroadcasters`
lukinovec Apr 3, 2026
dc344b7
Merge branch 'master' into broadcasting-fixes
stancl Apr 12, 2026
9ab0a72
Fix typo
lukinovec Apr 13, 2026
4aeaa66
Remove unused `$broadcaster` parameter
lukinovec Apr 13, 2026
b3d5197
Correct channel prefix test comments
lukinovec Apr 13, 2026
a247bb0
Correct config bootstrapper test comment
lukinovec Apr 13, 2026
20d494f
Reset `BroadcastingConfigBootstrapper::$broadcaster` in tests
lukinovec Apr 14, 2026
7af2b8a
Improve BroadcastManager instance checks
lukinovec Apr 14, 2026
a3c5568
Add regression test for credentialsMap merge order
lukinovec Apr 15, 2026
f3652a8
Make `with()` formatting consistent
lukinovec Apr 15, 2026
a7acd07
Reset `BroadcastingConfigBootstrapper::$mapPresets` in tests
lukinovec Apr 15, 2026
7db486d
Extract cleanup in broadcasting config test file
lukinovec Apr 15, 2026
d885659
Use toEqualCanonicalizing instead of toBe for custom creators
lukinovec Apr 15, 2026
e592b37
Merge branch 'master' into broadcasting-fixes
lukinovec Apr 15, 2026
ddd8c68
Improve comments
lukinovec Apr 21, 2026
614344a
Add test for reverting the bound broadcaster when tenancy ends
lukinovec Jun 29, 2026
f20f801
Merge branch 'master' into broadcasting-fixes
lukinovec Jun 29, 2026
9af1735
Explain why we swap the bound Broadcaster singleton better, remove th…
lukinovec Jul 10, 2026
937c8c8
Delete the redundant BroadcastingFactory::class arg from the Broadcas…
lukinovec Jul 10, 2026
a19d3b5
Merge branch 'master' into broadcasting-fixes
lukinovec Jul 10, 2026
24aeb23
Remove unused `$app` param and `?BroadcastManager` typehint (`singlet…
lukinovec Jul 10, 2026
2a0464d
Resolve (and cache) tenant broadcasters once per tenancy initializati…
lukinovec Jul 10, 2026
d80fed1
Remove stale $tenantBroadcasters mention
lukinovec Jul 10, 2026
af14d40
Delete extra newline, fix comment inconsistency
lukinovec Jul 10, 2026
b8fddf8
Retrieve the re-registered channel closure to assert what tenant_chan…
lukinovec Jul 11, 2026
128a1ad
Improve BroadcastingConfigBootstrapper docblock
lukinovec Jul 11, 2026
9e6e0a9
Remove TenancyBroadcastManager, pass the channel auth closures direct…
lukinovec Jul 12, 2026
5911619
Simplify channel-passing test
lukinovec Jul 12, 2026
120cec4
Add spaces after `fn`/`function` for consistency
lukinovec Jul 12, 2026
12e51d0
Test that when a tenant doesn't have the property that should be mapp…
lukinovec Jul 12, 2026
0ba588d
Improve channel closure inheritance test
lukinovec Jul 12, 2026
e6c4c31
Test that using broadcasters that only implement the Broadcaster cont…
lukinovec Jul 12, 2026
4422df2
Test configuring BroadcastingConfigBootstrapper::$broadcaster
lukinovec Jul 12, 2026
1409d72
Make test name more specific
lukinovec Jul 12, 2026
31d1663
Fix typo in comment
lukinovec Jul 12, 2026
d535b28
Update src/Bootstrappers/BroadcastingConfigBootstrapper.php
lukinovec Jul 13, 2026
81d41b4
Use `$centralBroadcaster->getChannels()` to get the channels instead …
lukinovec Jul 13, 2026
c07996e
Explain extending BroadcasterContract better
lukinovec Jul 13, 2026
44b97ae
phpstan fix: comment ignore
stancl Jul 15, 2026
9efebf9
Test reverting to central credentials after a tenant with overrides
lukinovec Jul 16, 2026
50afc49
Test that the authenticated user resolvers get copied to tenant broad…
lukinovec Jul 16, 2026
fe7468a
Copy the broadcaster auth state directly
lukinovec Jul 16, 2026
2314947
Assert that the bound broadcaster and the manager's default driver ar…
lukinovec Jul 17, 2026
64538ea
Rename `copyAuthState` to `copyAuthProperties`
lukinovec Jul 17, 2026
aa16736
Merge the BroadcastManager binding test into the broadcaster instance…
lukinovec Jul 17, 2026
d69c9f0
minor test improvements
stancl Jul 21, 2026
4e9f8a3
Delete redundant second tenant from auth prop copying test
lukinovec Jul 21, 2026
8a5f543
Delete $broadcaster property from the config bootstrapper, stop overr…
lukinovec Jul 21, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions src/Bootstrappers/BroadcastingConfigBootstrapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,19 @@
use Illuminate\Broadcasting\BroadcastManager;
use Illuminate\Config\Repository;
use Illuminate\Contracts\Broadcasting\Broadcaster;
use Illuminate\Contracts\Broadcasting\Factory as BroadcastingFactory;
use Illuminate\Foundation\Application;
use Illuminate\Support\Facades\Broadcast;
use Stancl\Tenancy\Contracts\TenancyBootstrapper;
use Stancl\Tenancy\Contracts\Tenant;
use Stancl\Tenancy\Overrides\TenancyBroadcastManager;

/**
* Maps tenant properties to broadcasting config and overrides
* the BroadcastManager binding with TenancyBroadcastManager.
*
* @see TenancyBroadcastManager
*/
class BroadcastingConfigBootstrapper implements TenancyBootstrapper
{
/**
Expand Down Expand Up @@ -54,7 +62,7 @@ public function __construct(
protected Application $app
) {
static::$broadcaster ??= $config->get('broadcasting.default');
static::$credentialsMap = array_merge(static::$credentialsMap, static::$mapPresets[static::$broadcaster] ?? []);
static::$credentialsMap = array_merge(static::$mapPresets[static::$broadcaster] ?? [], static::$credentialsMap);
}

public function bootstrap(Tenant $tenant): void
Expand All @@ -64,10 +72,35 @@ public function bootstrap(Tenant $tenant): void

$this->setConfig($tenant);

// Make BroadcastManager resolve to a custom BroadcastManager which makes the broadcasters use the tenant credentials
// Make BroadcastManager resolve to TenancyBroadcastManager which always re-resolves the used broadcasters so that
// the credentials used by broadcasters are always up-to-date with the config when retrieving the broadcasters using
// the manager and gives the channels of the broadcaster from central context to the newly resolved broadcasters in tenant context.
$this->app->extend(BroadcastManager::class, function (BroadcastManager $broadcastManager) {
return new TenancyBroadcastManager($this->app);
$originalCustomCreators = invade($broadcastManager)->customCreators;
$tenantBroadcastManager = new TenancyBroadcastManager($this->app);

// TenancyBroadcastManager inherits the custom driver creators registered in the central context so that
// custom drivers work in tenant context without having to re-register the creators manually.
foreach ($originalCustomCreators as $driver => $closure) {
$tenantBroadcastManager->extend($driver, $closure);
}

return $tenantBroadcastManager;
});
Comment on lines +72 to +82

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reason for creating the instance manually like this that if we simply forgot the bound instance from the container, the custom creators that were already registered would be lost?

To confirm - when would BroadcastManager::extend() be used? Is this change simply because of how we do things in our tests or would real applications also extend the manager before tenancy bootstrap?

And what are some common use cases.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reason for creating the instance manually like this that if we simply forgot the bound instance from the container, the custom creators that were already registered would be lost?

Exactly, yeah

To confirm - when would BroadcastManager::extend() be used? Is this change simply because of how we do things in our tests or would real applications also extend the manager before tenancy bootstrap?

I made the change because when I reviewed the tests, I realized that this could be a real problem with any custom driver.

Custom drivers are registered via Broadcast::extend('foobar', ...) in a service provider's boot(), which runs before tenancy is initialized, so in a real app the creators are always on the central manager before bootstrap. Without copying them, a tenant using a custom driver as the default broadcaster would get "Driver [foobar] is not supported" in the tenant context.

And what are some common use cases

Writing your own driver, or installing a package that ships one. I see that for example, the Centrifugo package registers its driver exactly like this, in its provider's boot() (it calls extend() on an injected BroadcastManager instead of the facade, but that's the same thing since Broadcast::extend() proxies to the manager): https://github.com/denis660/laravel-centrifugo/blob/bec36ea8323fb56dff42bfc3999600abc567afaf/src/CentrifugoServiceProvider.php#L27-L29

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we are recreating the manager from scratch, and then copying existing stuff into the new manager, instead of trying to reset parts of the manager - if possible. The ones that contain state that becomes utdated in the tenant context.

As a brief test, I looked at what forget* methods there are in the manager and what properties exist there. I saw forgetDrivers() and that there aren't really many other properties that could hold outdated state. If I replace this whole thing with just forgetDrivers() on the bound BroadcastManager, all tests except two assertions that are coupled to this specific implementation seem to pass.

Have we tried that before and was it insufficient to make everything work? Wondering why we'd be re-instantiating managers and copying private properties to new instances and things like that in this bootstrapper when we normally just try to reset state.

Thought of this when I looked at the invade()-related review since I checked where we use invade and it's only this bootstrapper and some tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a brief test ... If I replace this whole thing with just forgetDrivers() on the bound BroadcastManager, all tests except two assertions that are coupled to this specific implementation seem to pass.

Tested this too -- these tests are coupled to the specific implementation, so yeah, these fail. Other tests pass because we don't deal with the custom creator inheritance, the BroadcastManager extend and forgetDrivers have similar primary purpose (= make the manager resolve fresh broadcasters).

(actually, I think there's also a gap in the tests -- we don't have a test for whether revert restores the central credentials after a tenant with overrides, and that one would fail with forgetDrivers() -- more on that below)

But forgetDrivers is problematic.

Have we tried that before and was it insufficient to make everything work? Wondering why we'd be re-instantiating managers and copying private properties to new instances and things like that in this bootstrapper when we normally just try to reset state.

I don't think we have tried that at all because before, we were swapping the bound broadcast manager for the custom TenancyBroadcastManager (so there was no reason to try that). And after we removed that class, it didn't occur to me to test using just forgetDrivers() instead of the extend().

forgetDrivers() keeps the custom creators (since it only clears $drivers), so custom drivers would still work without the need to copy anything. The only custom creator difference is that a creator registered inside a tenant context would leak into later contexts instead of being isolated per tenant (that's why one of the tests fails), but that's an edge case.

But the worse thing is that we have to properly revert() what bootstrap() does. If we just did forgetDrivers() on the bound manager in bootstrap(), things will work just fine in tenant context (if we ignore custom creators). Then on revert(), if we did forgetDrivers(), we'd discard the tenant broadcaster, but nothing would copy the channel closures back to the central broadcaster. To make the full forgetDrivers() implementation actually correct, we'd have to copy the channel closures onto the rebuilt central broadcaster on revert too.

And if instead we kept instance() on revert (not forgetDrivers()), the manager's cached default driver would still hold the previous (tenant) broadcaster, so /broadcasting/auth in central context would use the old tenant's key (the channel closures were copied over during bootstrap, so /broadcasting/auth wouldn't 403, but the response would be signed using the tenant's key, and the websocket server would just reject it).

So both revert solutions need copying extra state to be correct, which is why extend() (restoring the untouched central instances via instance()) probably ends up cleaner. I'll try how that implementation could look, but like I wrote, I think the extend() may be cleaner in the end -- I'll let you know.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Keeping all the changes local for now since this review is tied to #1448 (comment), and we'll be doing more testing/code changes)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I think there's also a gap in the tests -- we don't have a test for whether revert restores the central credentials after a tenant with overrides, and that one would fail with forgetDrivers()

Added assertions for this to the credential mapping test. They fail with the forgetDrivers() thing we tested in bootstrap(), pass with extend().

9efebf9

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So both revert solutions need copying extra state to be correct, which is why extend() (restoring the untouched central instances via instance()) probably ends up cleaner. I'll try how that implementation could look

Tried it, and it's like I wrote -- bootstrap() gets simpler (forgetDrivers() + the same auth state copying), but revert() has to re-resolve the central broadcaster and copy the auth state back onto it:

public function revert(): void
{
    $this->unsetConfig();

    // Clear the tenant broadcasters so they get re-resolved using the central config again
    $this->app->make(BroadcastManager::class)->forgetDrivers();

    // A freshly resolved broadcaster has no auth state, so copy it back from the original broadcaster
    $centralBroadcaster = $this->app->make(BroadcastManager::class)->connection();
    $this->copyAuthState($this->originalBroadcaster, $centralBroadcaster);
    $this->app->instance(BroadcasterContract::class, $centralBroadcaster);

    Broadcast::clearResolvedInstance();
}

So the copying happens in both directions instead of once (meaning invade() stays too), the original broadcaster isn't restored on revert -- a fresh instance replaces it, so instances resolved before initializing tenancy end up stale. This all would be a bit more messy than the current implementation, so imo, switching to this won't be worth it.

(About the new assertions I mentioned in the previous comment -- with the full implementation, these assertions pass too, since revert() re-resolves the broadcaster with the central config -- only the instance check assertions fail)

Also tested the "resetting the existing object" idea from the other review, that's more interesting. I'll comment on that in a minute.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also tested the "resetting the existing object" idea from the other review, that's more interesting. I'll comment on that

See #1448 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1448 (comment)

One correction from the Discord discussion: revert() doesn't have to rebuild the central broadcaster and copy the properties back. Since nothing touches the central broadcaster while tenancy is initialized, it can be backed up in bootstrap() and revert() can just be:

$manager->forgetDrivers();
$this->app->instance(BroadcasterContract::class, $this->originalBroadcaster);
invade($manager)->drivers = [$default => $this->originalBroadcaster];

With that, only the custom creator isolation and manager instance check assertions fail (both coupled to the current implementation, and the creator isolation is a non-issue anyway since creators aren't context-specific state). More on the invade($manager)->drivers part in #1448 (comment). We're keeping the current implementation either way.


// Swap currently bound Broadcaster instance for one that's resolved through the tenant BroadcastManager.
// Note that updating broadcasting config (credentials) in tenant context doesn't update the credentials
// used by the bound Broadcaster instance. If you need to e.g. send a notification in response to
// updating tenant's broadcasting credentials in tenant context, it's recommended to
// reinitialize tenancy after updating the credentials.
$this->app->extend(Broadcaster::class, function (Broadcaster $broadcaster) {
return $this->app->make(BroadcastManager::class)->connection();
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

// Clear the resolved Broadcast facade's Illuminate\Contracts\Broadcasting\Factory instance
// so that it gets re-resolved as TenancyBroadcastManager instead of the central BroadcastManager
// when used. E.g. the Broadcast::auth() call in BroadcastController::authenticate (/broadcasting/auth).
Broadcast::clearResolvedInstance(BroadcastingFactory::class);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the argument needed here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I added the arg for additional clarity (though now I see it doesn't help in that regard at all). Confirmed that it's absolutely not needed:

public static function clearResolvedInstance($name = null)
{
    unset(static::$resolvedInstance[$name ?? static::getFacadeAccessor()]);
}

Since Broadcast:: getFacadeAccessor() returns Illuminate\Contracts\Broadcasting\Factory::class, which is exactly what we're passing to clearResolvedInstance (just aliased to BroadcastingFactory), we can just use Broadcast::clearResolvedInstance(). The behavior will be identical.

So I'll make this just Broadcast::clearResolvedInstance()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same in bootstrap())

@lukinovec lukinovec Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 937c8c8 (also improved the comment above clearResolvedInstance in bootstrap() -- it didn't fully explain why we need to clear the resolved instance).

}

public function revert(): void
Expand All @@ -76,6 +109,9 @@ public function revert(): void
$this->app->singleton(BroadcastManager::class, fn (Application $app) => $this->originalBroadcastManager);
$this->app->singleton(Broadcaster::class, fn (Application $app) => $this->originalBroadcaster);

// Clear the resolved Broadcast facade instance so that it gets re-resolved as the central BroadcastManager
Broadcast::clearResolvedInstance(BroadcastingFactory::class);

$this->unsetConfig();
}

Expand Down
54 changes: 34 additions & 20 deletions src/Overrides/TenancyBroadcastManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,37 @@
use Illuminate\Broadcasting\Broadcasters\Broadcaster;
use Illuminate\Broadcasting\BroadcastManager;
use Illuminate\Contracts\Broadcasting\Broadcaster as BroadcasterContract;
use Illuminate\Contracts\Foundation\Application;

/**
* BroadcastManager override that always re-resolves the broadcasters in static::$tenantBroadcasters
* when attempting to retrieve them and passes the channels of the original (central) broadcaster
* to the newly resolved (tenant) broadcasters.
*
* Affects calls that use app(BroadcastManager::class)->get().
Comment thread
lukinovec marked this conversation as resolved.
Outdated
*
* @see Stancl\Tenancy\Bootstrappers\BroadcastingConfigBootstrapper
*/
class TenancyBroadcastManager extends BroadcastManager
{
/**
* Names of broadcasters to always recreate using $this->resolve() (even when they're
* cached and available in the $broadcasters property).
*
* The reason for recreating the broadcasters is
* to make your app use the correct broadcaster credentials when tenancy is initialized.
* Names of broadcasters that
* - should always be recreated using $this->resolve(), even when they're cached and available
* in $this->drivers so that when you update broadcasting config in the tenant context,
* the updated config/credentials will be used for broadcasting immediately.
* Note that in cases like this, only direct config changes are reflected right away.
* For the broadcasters to reflect tenant property changes made in tenant context,
* you still have to reinitialize tenancy after updating the tenant properties intended
* to be mapped to broadcasting config, since the properties are only mapped to config
* on BroadcastingConfigBootstrapper::bootstrap().
* - should inherit the original broadcaster's channels (= the channels registered in
* the central context, e.g. in routes/channels.php, before this manager overrides the bound BroadcastManager).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking if there is a reason for breaking the cache like this. When would someone update the config? Only when tenancy is being initialized - the config is loaded from the tenant and then stays the same. There aren't many use cases for someone updating broadcasting config long after tenancy is initialized.

If they would make any change, it'd be through changes on the tenant itself which aren't reflected in the config anyway, so a reinitialization would be needed.

The only benefit of doing this that I can see is that it makes our tests a little easier maybe? Fewer reinit calls needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I agree that there probably aren't many use cases for updating the config after tenancy is initialized, so I guess we can forget about it entirely.

Even with this re-resolving logic, after direct broadcasting config updates, a page reload is necessary for the frontend to reconnect to Echo using the new credentials. The re-resolving logic here loses its purpose, the channel-passing logic is the thing we care about here.

Now I found out that when the drivers are re-resolved, only the central channels get passed to them. So when someone registers a channel in tenant context (pretty unlikely but possible), and the driver gets re-resolved later e.g. during the /broadcasting/auth request (during Broadcast::auth()), the driver will only have the channels registered in the central context again (since the driver got re-resolved, its channel list is fresh, until we pass the central channels to it -- and then, it'll only have the central channels in the list). So the /broadcasting/auth request would throw a 403 as if the channel never got registered because the driver just doesn't have it in its list at this point due to the re-resolution. This is probably not a huge problem, but it's not right.

If we don't care about direct config updates and just establish that "for broadcasters to use the up-to-date credentials/config, tenancy has to be reinitialized", TenancyBroadcastManager could actually get simplified a lot. Passing the channels to freshly resolved broadcasters could be its only purpose (after initializing tenancy, all drivers would get freshly resolved because we create a new TenancyBroadcastManager instance with no drivers cached). Instead of overriding get() (which we do now just so that the broadcasters are always re-resolved), we could override resolve() just so that the freshly resolved broadcasters receive the central channels (and if someone registers a channel in the tenant context, that'll work without a problem because we won't re-resolve the broadcasters).

We could get rid of TenancyBroadcastManager::$tenantBroadcasters entirely since its purpose was to list names of broadcasters that should get re-resolved and receive the central channels (the coupling here was weird anyway). Since in this theoretical implementation, we'd just be passing central channels to broadcasters newly resolved (and then cached) by TenancyBroadcastManager, I think it'd be pretty safe to just do this for all broadcasters, not just some specific ones.

Anyways, these are pretty big changes. I'll try changing the code around and see if the changes would make sense in the end.

(Also, yeah, the tests would have to get updated)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try changing the code around and see if the changes would make sense in the end.

Tried that, and it turned out good. Will push the changes after polishing everything.

Worth noting that now that there's no re-resolving going on, app(BroadcasterContract::class) and Broadcast::driver() will be the same instances in tenant context (which is consistent with Laravel's behavior).

Now I found out that when the drivers are re-resolved, only the central channels get passed to them. So when someone registers a channel in tenant context (pretty unlikely but possible), and the driver gets re-resolved later e.g. during the /broadcasting/auth request (during Broadcast::auth()), the driver will only have the channels registered in the central context again

Added a regression test for this and made it pass -- a channel registered via Broadcast::channel() in tenant context now survives subsequent driver() calls (the test fails with the original get() override, passes with the new resolve() one). Also tested that the channels registered in tenant context only live for the duration of that context, so the channels don't leak from tenant to central context, and they don't leak from one tenant's context to another's.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed the current changes: 2a0464d

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end, I deleted the entire TenancyBroadcastManager class in 9e6e0a9 as its only purpose was to pass the central channel auth closures to each newly resolved broadcaster in the tenant context. Only the default broadcaster's channel auth closures matter.

(see the PR's description)

*/
public static array $tenantBroadcasters = ['pusher', 'ably'];
public static array $tenantBroadcasters = ['pusher', 'ably', 'reverb'];

/**
* Override the get method so that the broadcasters in $tenantBroadcasters
* always get freshly resolved even when they're cached and available in the $broadcasters property,
* and that the resolved broadcaster will override the BroadcasterContract::class singleton.
*
* If there's a cached broadcaster with the same name as $name,
* give its channels to the newly resolved bootstrapper.
* Override the get method so that the broadcasters in static::$tenantBroadcasters
* - receive the original (central) broadcaster's channels
* - always get freshly resolved.
*/
protected function get($name)
{
Expand All @@ -35,24 +46,27 @@ protected function get($name)
$originalBroadcaster = $this->app->make(BroadcasterContract::class);
$newBroadcaster = $this->resolve($name);

// If there is a current broadcaster, give its channels to the newly resolved one
// Give the channels of the original (central) broadcaster to the newly resolved one.
//
// Broadcasters only have to implement the Illuminate\Contracts\Broadcasting\Broadcaster contract
// Which doesn't require the channels property
// So passing the channels is only needed for Illuminate\Broadcasting\Broadcasters\Broadcaster instances
// which doesn't require the channels property, so passing the channels is only needed for
// Illuminate\Broadcasting\Broadcasters\Broadcaster instances (= all the default broadcasters, e.g. PusherBroadcaster).
if ($originalBroadcaster instanceof Broadcaster && $newBroadcaster instanceof Broadcaster) {
$this->passChannelsFromOriginalBroadcaster($originalBroadcaster, $newBroadcaster);
}

$this->app->singleton(BroadcasterContract::class, fn (Application $app) => $newBroadcaster);

return $newBroadcaster;
}

return parent::get($name);
}

// Because, unlike the original broadcaster, the newly resolved broadcaster won't have the channels registered using routes/channels.php
// Using it for broadcasting won't work, unless we make it have the original broadcaster's channels
/**
* The newly resolved broadcasters don't automatically receive the channels registered
* in central context (e.g. Broadcast::channel() in routes/channels.php), so the channels
* have to be obtained from the original (central) broadcaster and manually passed to the new broadcasters
* (broadcasting using a broadcaster with no channels results in a 403 error on Broadcast::auth()).
*/
protected function passChannelsFromOriginalBroadcaster(Broadcaster $originalBroadcaster, Broadcaster $newBroadcaster): void
{
// invade() because channels can't be retrieved through any of the broadcaster's public methods
Expand Down
96 changes: 96 additions & 0 deletions tests/Bootstrappers/BroadcastChannelPrefixBootstrapperTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
use Stancl\Tenancy\Bootstrappers\DatabaseTenancyBootstrapper;
use Stancl\Tenancy\Bootstrappers\BroadcastChannelPrefixBootstrapper;
use function Stancl\Tenancy\Tests\pest;
use Illuminate\Broadcasting\Broadcasters\NullBroadcaster;
use Illuminate\Support\Facades\Broadcast;
use Illuminate\Support\Collection;

beforeEach(function () {
Event::listen(TenancyInitialized::class, BootstrapTenancy::class);
Expand Down Expand Up @@ -137,3 +140,96 @@ protected function formatChannels(array $channels)
expect(app(BroadcastManager::class)->driver())->toBe($broadcaster);
expect(invade(app(BroadcastManager::class)->driver())->formatChannels($channelNames))->toEqual($channelNames);
});

test('broadcasting channel helpers register channels correctly', function() {
config([
'broadcasting.default' => $driver = 'testing',
'broadcasting.connections.testing.driver' => $driver,
]);

config(['tenancy.bootstrappers' => [DatabaseTenancyBootstrapper::class]]);

Schema::create('users', function (Blueprint $table) {
$table->increments('id');
$table->string('name');
$table->string('email')->unique();
$table->string('password');
$table->rememberToken();
$table->timestamps();
});

$centralUser = User::create(['name' => 'central', 'email' => 'test@central.cz', 'password' => 'test']);
$tenant = Tenant::create();

migrateTenants();

tenancy()->initialize($tenant);

// Same ID as $centralUser
$tenantUser = User::create(['name' => 'tenant', 'email' => 'test@tenant.cz', 'password' => 'test']);

tenancy()->end();

/** @var BroadcastManager $broadcastManager */
$broadcastManager = app(BroadcastManager::class);

// Use a driver with no channels
$broadcastManager->extend($driver, fn () => new NullBroadcaster);

$getChannels = fn (): Collection => $broadcastManager->driver($driver)->getChannels();

expect($getChannels())->toBeEmpty();

// Basic channel registration
Broadcast::channel($channelName = 'user.{userName}', $channelClosure = function ($user, $userName) {
return User::firstWhere('name', $userName)?->is($user) ?? false;
});

// Check if the channel is registered
$centralChannelClosure = $getChannels()->first(fn ($closure, $name) => $name === $channelName);
expect($centralChannelClosure)->not()->toBeNull();

// Channel closures work as expected (running in central context)
expect($centralChannelClosure($centralUser, $centralUser->name))->toBeTrue();
expect($centralChannelClosure($centralUser, $tenantUser->name))->toBeFalse();

// Register a tenant broadcasting channel (almost identical to the original channel, just able to accept the tenant key)
tenant_channel($channelName, $channelClosure);

// Tenant channel registered – its name is correctly prefixed ("{tenant}.user.{userId}")
$tenantChannelClosure = $getChannels()->first(fn ($closure, $name) => $name === "{tenant}.$channelName");
expect($tenantChannelClosure)->toBe($centralChannelClosure);

// The tenant channels are prefixed with '{tenant}.'
// They accept the tenant key, but their closures only run in tenant context when tenancy is initialized
// The regular channels don't accept the tenant key, but they also respect the current context
// The tenant key is used solely for the name prefixing – the closures can still run in the central context
tenant_channel($channelName, $tenantChannelClosure = function ($user, $tenant, $userName) {
return User::firstWhere('name', $userName)?->is($user) ?? false;
});

expect($tenantChannelClosure)->not()->toBe($centralChannelClosure);

expect($tenantChannelClosure($centralUser, $tenant->getTenantKey(), $centralUser->name))->toBeTrue();
expect($tenantChannelClosure($centralUser, $tenant->getTenantKey(), $tenantUser->name))->toBeFalse();

tenancy()->initialize($tenant);

// The channel closure runs in the central context
// Only the central user is available
expect($tenantChannelClosure($centralUser, $tenant->getTenantKey(), $tenantUser->name))->toBeFalse();
expect($tenantChannelClosure($tenantUser, $tenant->getTenantKey(), $tenantUser->name))->toBeTrue();
Comment thread
lukinovec marked this conversation as resolved.
Outdated
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Use a new channel instance to delete the previously registered channels before testing the universal_channel helper
Comment thread
lukinovec marked this conversation as resolved.
Outdated
$broadcastManager->purge($driver);
$broadcastManager->extend($driver, fn () => new NullBroadcaster);

expect($getChannels())->toBeEmpty();

// Global channel helper prefixes the channel name with 'global__'
global_channel($channelName, $channelClosure);

// Channel prefixed with 'global__' found
$foundChannelClosure = $getChannels()->first(fn ($closure, $name) => $name === 'global__' . $channelName);
expect($foundChannelClosure)->not()->toBeNull();
});
Loading
Loading