search

gauntface / web-push-go

Apache License 2.0
99 stars 14 forks source link

Add Vapid support, mostly a subset of #11 #17

Closed wibblymat closed 6 years ago

wibblymat commented 8 years ago

I was surprised to find the other day that we never resolved the VAPID PR (#11).

Since the comment thread had stalled, I've put together this smaller counter-proposal that is hopefully less controversial.

@costinm @crhym3 @gauntface

x1ddos commented 8 years ago

Reviewed.

x1ddos commented 8 years ago

Forgot to ask: tests?

davinche commented 8 years ago

Has this been tested? Does this implementation work with chrome? I also added support for VAPID on top of this library, but it is only working in firefox. On chrome, FCM is sending back a 400 "UnauthorizedRegistration" error.

ofavre commented 7 years ago

I've just tested this with Firefox 50.0.2 and Chrome 55.0.2883.75, it works fine!

Will this be merged someday? Is this library defunct because of the raise of the web-push Node.JS lib?

@davinche: You get an UnauthorizedRegistration 400 response if the manifest contains a gcm_sender_id key for Chrome and I use applicationServerKey when subscribing and NewVapidRequest() from this pull request. I guess this comes from the fact that an Authorization: key=XYZ is expected when using a GCM SenderID and this header is used with a JWT when using VAPID. That and the end-point that does not mention the subscription id.

davinche commented 7 years ago

@ofavre The UnauthorizedRegistration error I was getting was actually caused by the JWT Header serialization. More details can be found in my reply to this stackoverflow post: http://stackoverflow.com/questions/39336523/webpushvapid-request-fails-with-400-unauthorizedregistration

The gist of it all is that json/encoding libary in Go produced a marshalled JWT header that is different than what chrome expected. Apparently order of the fields mattered ¯_(ツ)_/¯. I think this is a bug in chrome.

This PR avoids it all by including a static string: https://github.com/GoogleChrome/push-encryption-go/blob/d00644ee80b2c526918c0a6a88c63d4206fccb0a/webpush/vapid.go#L31

avinassh commented 7 years ago

Any update on this?

anaskhan96 commented 7 years ago

@avinassh I have been waiting for the vapid support too. In the meantime, this library has been helping me out.

x1ddos commented 7 years ago

@gauntface I'll take this PR and apply all suggestions. Will ping for a review.

gauntface commented 7 years ago

@x1ddos thank you Alex. I'll get to work on a PR for integration tests (expect awful Go)

@avinassh @anaskhan96 apologies for the delay on all of this. I've inherited the project and once we have tests and VAPID set up I'll be moving this over to web-push-libs org so it can get wider community eyes and support.

nicosavini commented 6 years ago

Hi any updates on this? Thanks,

gauntface commented 6 years ago

I would recommend using: https://github.com/SherClockHolmes/webpush-go

This module had a number of quirks / issues that feels wasted to develop this module when the other seems further ahead in it's implementation

gauntface commented 6 years ago

Going to deprecate this project in favor of https://github.com/SherClockHolmes/webpush-go