-
-
Notifications
You must be signed in to change notification settings - Fork 492
[4.x] BroadcastingConfigBootstrapper: fix central instances persisting in tenant context #1448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 24 commits
7d749eb
2ecc94d
fafd082
c653c51
b1e91f1
65beecf
0b860ea
c4f4451
d939866
28b6119
0fbe1bc
b6c035c
bbe2ff0
b2add06
9e9bedc
4b1cc9c
fc45e09
6b99921
29dd23d
4937a74
c831393
ef476c5
f8528fc
dc344b7
9ab0a72
4aeaa66
b3d5197
a247bb0
20d494f
7af2b8a
a3c5568
f3652a8
a7acd07
7db486d
d885659
e592b37
ddd8c68
614344a
f20f801
9af1735
937c8c8
a19d3b5
24aeb23
2a0464d
d80fed1
af14d40
b8fddf8
128a1ad
9e6e0a9
5911619
120cec4
12e51d0
0ba588d
e6c4c31
4422df2
1409d72
31d1663
d535b28
81d41b4
c07996e
44b97ae
9efebf9
50afc49
fe7468a
2314947
64538ea
aa16736
d69c9f0
4e9f8a3
8a5f543
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| { | ||
| /** | ||
|
|
@@ -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 | ||
|
|
@@ -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; | ||
| }); | ||
|
|
||
| // 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(); | ||
| }); | ||
|
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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the argument needed here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 So I'll make this just
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (same in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 937c8c8 (also improved the comment above |
||
| } | ||
|
|
||
| public function revert(): void | ||
|
|
@@ -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(); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(). | ||
|
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). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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", We could get rid of 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)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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,
Added a regression test for this and made it pass -- a channel registered via
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed the current changes: 2a0464d
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the end, I deleted the entire (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) | ||
| { | ||
|
|
@@ -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 | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, yeah
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'sboot(), 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.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 callsextend()on an injectedBroadcastManagerinstead of the facade, but that's the same thing sinceBroadcast::extend()proxies to the manager): https://github.com/denis660/laravel-centrifugo/blob/bec36ea8323fb56dff42bfc3999600abc567afaf/src/CentrifugoServiceProvider.php#L27-L29There was a problem hiding this comment.
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 sawforgetDrivers()and that there aren't really many other properties that could hold outdated state. If I replace this whole thing with justforgetDrivers()on the boundBroadcastManager, 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
BroadcastManagerextend andforgetDrivershave 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
forgetDriversis problematic.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 theextend().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()whatbootstrap()does. If we just didforgetDrivers()on the bound manager inbootstrap(), things will work just fine in tenant context (if we ignore custom creators). Then onrevert(), if we didforgetDrivers(), we'd discard the tenant broadcaster, but nothing would copy the channel closures back to the central broadcaster. To make the fullforgetDrivers()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 (notforgetDrivers()), 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.There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added assertions for this to the credential mapping test. They fail with the forgetDrivers() thing we tested in bootstrap(), pass with extend().
9efebf9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried it, and it's like I wrote --
bootstrap()gets simpler (forgetDrivers()+ the same auth state copying), butrevert()has to re-resolve the central broadcaster and copy the auth state back onto it: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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1448 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inbootstrap()andrevert()can just be: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)->driverspart in #1448 (comment). We're keeping the current implementation either way.