Skip to content

Add support for webpush pusher as defined in MSC4174#1201

Open
MatMaul wants to merge 2 commits intoelement-hq:masterfrom
MatMaul:msc4174-vapid
Open

Add support for webpush pusher as defined in MSC4174#1201
MatMaul wants to merge 2 commits intoelement-hq:masterfrom
MatMaul:msc4174-vapid

Conversation

@MatMaul
Copy link
Copy Markdown

@MatMaul MatMaul commented Dec 2, 2024

Copy link
Copy Markdown
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Needs a few updates with respect to the latest draft of MSC4174 (f59cb97551216812d1ca492fafba05d057cc1298)

const subscriptionData = subscription.toJSON();
const pushkey = subscriptionData.keys.p256dh;
const data = {
endpoint: subscriptionData.endpoint,
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.

This has been replaced with a url field in the latest version of the MSC.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The new endpoint has to go there: https://github.com/element-hq/hydrogen-web/pull/1201/changes#r2923213109

This endpoint field is used for the current http pusher. That's OK to keep it with the new pusher, it will simply be ignored. To make things a little more evident, we can do supportDirectWebPush ? null : subscriptionData.endpoint

}

async enablePush(pusherFactory, defaultPayload) {
async enablePush(pusherFactory, defaultPayload, hsApi) {
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.

Does this code handle the 201 response code that the MSC asks for?

If the request creates a new pusher or modifies values under pushkey , PusherData.url, or PusherData.auth, then the server MUST respond with 201, ...

as well as handling the subsequent validation push containing app_id and ack_token?

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.

We seem to also be missing the bit where the client calls POST /_matrix/client/v3/pushers/ack after receiving the validation push.

The client needs to handle the new M_EXPIRED_ACTIVATION_TOKEN and M_UNKNOWN_ACTIVATION_TOKEN error types.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

To be done on the SW 👍

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.

SW?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

service worker

);
if (supportDirectWebPush) {
return pusherFactory.webpushPusher(
this._pushConfig.appId,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
this._pushConfig.appId,
subscriptionData.endpoint,
this._pushConfig.appId,

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.

3 participants