Skip to content
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

[9.x] User authentication for Pusher #42531

Merged
merged 6 commits into from
Jun 1, 2022

Conversation

rennokki
Copy link
Contributor

@rennokki rennokki commented May 26, 2022

Purpose

Pusher recently launched a feature that allows their users to enable authentication for connections, alongside the channel authorizations: https://pusher.com/docs/channels/using_channels/user-authentication/

Shortly, it's a feature that will make sure that anyone connecting to the websockets should be an authenticated user within the app. This will increase the trust of your connected users and you can broadcast events to user's connections by their database ID, instead of the usual Socket ID.

This is also a gateway step to implement Authorized Connections for the clients, to prevent developers from having attackers abuse the connection quota by creating fake websocket connections without subscribing to any channel: https://pusher.com/docs/channels/using_channels/authorized-connections/

Approach

In Laravel, this requires a new endpoint that Pusher can call. This was implemented as /broadcasting/user-auth (Pusher default is /pusher/user-auth) which receives a socket_id as request parameter. The issue here is that we cannot find a correlation between socket ID and user, so the requests from the frontend need to be authenticated via the already-injected session by Echo, or with Sanctum with a custom authorizer that will automatically bind the session to the request, in order to be able to retrieve the user.

In BroadcastServiceProvider, developers can provide a callback for the user data to send back to Pusher, in the same manner it's done for presence channels:

Broadcast::resolveUserAuthentication(function (Request $request) {
    if (! $request->user()) {
        return;
    }

    return [
        'id' => $request->user()->id,
        'name' => $request->user()->name,
    ];
});

Currently, this feature is enabled only for Pusher.

Breaking Changes

There are no breaking changes to be considered. The /user-auth endpoint is going to return 404 for Laravel versions prior to this PR and 401 in case the users don't implement the resolveUserAuthentication method.

Additional steps

Extra, this implies:

  • Docs about feature
  • Additional changes to docs to include the endpoint for user auth (same as channel auth)
  • Laravel Echo changes (optional)

Read more about the feature:

@rennokki rennokki marked this pull request as draft May 26, 2022 11:20
@rennokki rennokki changed the title [9.x] Added user connection authentication for Pusher [9.x] User authentication for Pusher May 26, 2022
@rennokki rennokki marked this pull request as ready for review May 26, 2022 11:27
@taylorotwell taylorotwell merged commit 5d38cc2 into laravel:9.x Jun 1, 2022
@taylorotwell
Copy link
Member

Renamed methods... new names:

resolveAuthenticatedUser
resolveAuthenticatedUserUsing

@taylorotwell
Copy link
Member

Are you able to send a documentation PR for this?

@rennokki rennokki deleted the feature/pusher-user-auth branch June 2, 2022 15:24
@RTippin
Copy link

RTippin commented Jun 3, 2022

Not sure if this is a bug or an inconvenience, but with the addition of a new endpoint in the route group within BroadcastManager, I can no longer "name" the broadcast endpoint using the Broadcast::routes() helper, eg:

Broadcast::routes([
    'domain' => null,
    'prefix' => 'api/v1',
    'middleware' => ['api', 'auth:api'],
    'as' => 'api.broadcasting.auth',
]);

I give the endpoint a name for internal documentation and as a helper for use in my test suite, eg:

/** @test */
public function it_is_forbidden_to_join_another_users_channel()
{
    $this->actingAs($this->tippin, 'api');

    $this->postJson(route('api.broadcasting.auth'), [
        'channel_name' => (string) $this->doe->getPrivateChannel(),
    ])->assertForbidden();
}

/** @test */
public function it_joins_user_channel()
{
    $this->actingAs($this->tippin, 'api');

    $this->postJson(route('api.broadcasting.auth'), [
        'channel_name' => (string) $this->tippin->getPrivateChannel(),
    ])->assertSuccessful();
}

Unless I replace the use of the route helper with the exact endpoint /api/v1/broadcasting/auth, or register the single route myself with the router directly, then my api.broadcasting.auth has the router now giving me the new /broadcasting/user-auth endpoint, causing my tests to fail.

@rennokki
Copy link
Contributor Author

rennokki commented Jun 4, 2022

@RTippin Indeed there were issues. Sent a new PR for this fix.

After the PR, you can do something like this for both endpoints, as you can individually tell which one to receive whichever options you want:

$channelOpts = [
    'domain' => null,
    'prefix' => 'api/v1',
    'middleware' => ['api', 'auth:api'],
    'as' => 'api.broadcasting.auth',
];

$authOpts = [
    'domain' => null,
    'prefix' => 'api/v1',
    'middleware' => ['api', 'auth:api'],
    'as' => 'api.broadcasting.auth-user',
];

Broadcast::routes($channelOpts, $authOpts);

Or keep the old configuration as you currently have, the new endpoint won't receive the same name as the channel authorization endpoint.

@RTippin
Copy link

RTippin commented Jun 4, 2022

@rennokki I do appreciate the response! For now, I ended up defining the route directly:

private function loadRoutes(): void
{
    if ($this->app instanceof CachesRoutes
        && $this->app->routesAreCached()) {
        return;
    }

    Route::middleware(['api', 'auth:api'])
        ->withoutMiddleware([VerifyCsrfToken::class])
        ->prefix('api/v1/broadcasting')
        ->name('api.broadcasting.')
        ->group(function () {
            Route::match(['get', 'post'], '/auth', [BroadcastController::class, 'authenticate'])->name('auth');
    });
}

I would not mind using your PR'd fix, though pretty sure it would have to be put in 10.x as I want to say method signature changes are considered BCs, even when they are optional.

@rennokki
Copy link
Contributor Author

rennokki commented Jun 4, 2022

Hm, maybe defining separate routing calls to avoid BC?

Broadcast::routes();
Broadcast::authenticationRoutes();

// Maybe having an alias for ::routes() as this
Broadcast::channelAuthorizationRoutes();

@RTippin
Copy link

RTippin commented Jun 5, 2022

@rennokki I just checked your recent updates to the PR #42664 and I like that approach. Naming wise, I'm probably the wrong person to ask, but unsure if it should have pusher in the name, or if other broadcasters will also support a similar feature.

@rennokki
Copy link
Contributor Author

rennokki commented Jun 5, 2022

@RTippin Not sure exactly if they should be named based on the drivers. You could only have one broadcaster driver activated per request cycle, without taking into consideration the config rewrite during the request cycle.

Having this would break if I'd have the broadcasting driver set to log, as the LogBroadcaster might not implement it, and it's too specific to have it in all drivers:

Broadcaster::pusherAuthenticationRoutes();

I've seen the broadcasting module's implementation's names are pretty much driver agnostic, with the exception of the actual broadcasters that are specific (PusherBroadcaster, RedisBroadcaster, etc.) for the drivers.

But this is up to @taylorotwell :)

@Wulfheart
Copy link

Is this also supported by soketi?

@rennokki
Copy link
Contributor Author

rennokki commented Jun 9, 2022

@Wulfheart Starting from 1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants