Convert all push recipes to use VAPID #294

slokhorst · 2018-01-28T17:34:44Z

This is a start for implementing #290

Mostly adapted the example from https://github.com/web-push-libs/web-push

Some questions/notes before I do the other push demos:

urlBase64ToUint8Array is a big and ugly, and will be needed in every push example. Should this be moved to a separate library file?
Is there a way to compare Uint8Arrays without using toString()?
I've removed the GCM functionality as it's non-standard and should not be encouraged. This means we drop support for Chrome 51 and earlier. (almost 2 years old so should be ok)

marco-c · 2018-02-06T19:31:30Z

push-payload/index.js

+    var response = await fetch('./vapidPublicKey')
+    var vapidPublicKey = await response.text();
+    // Convert the URL safe base64 string to a Uint8Array
+    var convertedVapidKey = urlBase64ToUint8Array(vapidPublicKey);


You can probably use the base64 string directly, see https://developer.mozilla.org/en-US/docs/Web/API/PushManager/subscribe

Doesn't seem to work on Chrome:
TypeError: Failed to execute 'subscribe' on 'PushManager': The provided value is not of type '(ArrayBuffer or ArrayBufferView)'

Edit: But it does work in Firefox, and it should according to the spec: (BufferSource or DOMString)? applicationServerKey = null;. So this is probably a bug in Chrome?

OK, so I guess it's not implemented yet in Chrome. Unfortunately I don't think there's a way to share the code between the recipes, so we will have to copy it everywhere.
Please also add a comment explaining why it is needed and that it can be removed once Chrome supports passing a base64 string.

Chrome bug: applicationServerKey does not accept a base64url encoded value

marco-c · 2018-02-06T19:32:40Z

push-payload/index.js

    if (subscription) {
-      return subscription;
+      var currentKey = new Uint8Array(subscription.options.applicationServerKey);


I'm not sure that we need to take into account the possibility of the vapid key changing in these examples.

If we don't store the key somewhere (as the example is right now), it will change every time the server process restarts. When the key has changed the example will no longer work: server.js will attempt to send the push message and the endpoint will return an 400 UnauthorizedRegistration response (Google) or 401 Unauthorized response with "Request did not validate missing authorization header" (Mozilla).

This obviously happens a lot when developing locally. I'm not sure what kind of infrastructure serviceworke.rs is running on, but say you try the example one day, the server restarts overnight, and you try it again the following day, it will no longer work.

We are using Heroku. I think we can store the VAPID key in a file so it doesn't get recreated every time.
It will still be recreated from time to time, but not every time you reload.

It is still going to be a pain though... Hopefully we can figure out a better solution.

I still think the current check is a pretty good solution. Even in production scenarios you might need to change the keys sometimes (when the key is compromised or lost). This seems a sensible place to check if the subscription we have is still usable. And as a bonus we don't have to extend the demo with persistent storage (or something behind-the-scenes specifically for serviceworke.rs).

I have another idea. We can put the VAPID key in a Heroku environment variable, so it will never change.

marco-c · 2018-02-06T19:33:20Z

I've removed the GCM functionality as it's non-standard and should not be encouraged. This means we drop support for Chrome 51 and earlier. (almost 2 years old so should be ok)

This is fine, we are going to remove it from the web-push library too soon.

slokhorst · 2018-02-06T23:37:53Z

By the way, if we no longer use GCM, the manifest.webmanifest is no longer required. Should I remove it?

marco-c · 2018-02-06T23:46:18Z

By the way, if we no longer use GCM, the manifest.webmanifest is no longer required. Should I remove it?

Yes, we can remove it as part of this PR.

slokhorst · 2018-02-07T12:52:29Z

b1fd19b makes things a lot more readable (from web-push-libs: "The expected format is the same output as JSON.stringify'ing a PushSubscription in the browser.")

If there aren't any more concerns (except the handling of changing server keys), I'll continue with the other recipes.

marco-c · 2018-02-07T14:05:13Z

push-payload/index.js

-
-  endpoint = subscription.endpoint;
+  // Keep the subscription for easy access
+  mySubscription = subscription;


We shouldn't need this, we can use subscription directly.

mySubscription is reused in the doIt.onclick function. If we don't save it here we need to add the following there (fine with me either way, but this adds some more code):

navigator.serviceWorker.ready.then(function(registration) { registration.pushManager.getSubscription().then(function(subscription) { ... } }

We can bring doIt.onclick in the scope here, it needs to wait for the subscription anyway.

marco-c · 2018-02-07T15:16:47Z

push-payload/index.js

+// This function is needed because Chrome doesn't accept a base64 encoded string
+// as value for applicationServerKey in pushManager.subscribe yet
+// https://bugs.chromium.org/p/chromium/issues/detail?id=802280
+function urlBase64ToUint8Array(base64String) {


Maybe we can put this function in a utils.js script in the top-directory, which we include in each recipe before index.js. What do you say?

That seems like the cleanest solution.. Unfortunately I'm not sure how to do this. If we put the file in the top-directory, it isn't served by the server process. I think it has to be specified in gulpfile.js to be served? Also not ideal.. Is there another way?

Yes, probably the gulpfile.js has to copy the file to dist/.
@delapuente any other idea how to do this?

1620dfc works, but might not be the best solution.

That looks sane enough, maybe move it to the top repo directory though instead of src/js/.
I'm still undecided whether the best solution is to have the function in a separate file or not. One one hand, it's a pain to copy and paste it in every recipe, on the other hand, the recipes are supposed to be self-contained.

(Sorry if the discussion is taking long, but once we have decided here we will apply the same rules to all recipes, so it will be way faster then).

Yep I agree that it's both not optimal.. Its a shame that Chrome hasn't even started their native implementation yet.. otherwise we could maybe leave the function out completely. But since it's not even on the horizon that's not really a possibility either.

(I've moved tools.js to the top directory in 1bb1db5)

slokhorst · 2018-02-28T18:19:06Z

So what's the next step here? I'm not sure how to handle the key with a Heroku variable (will that even work locally on my machine?). Maybe it would be easier if you do that.

marco-c · 2018-03-01T21:23:12Z

So what's the next step here? I'm not sure how to handle the key with a Heroku variable (will that even work locally on my machine?). Maybe it would be easier if you do that.

As far as the application is concerned, a Heroku variable is just an environment variable. So you can emulate everything locally by defining an env variable.

slokhorst · 2018-03-02T21:52:35Z

Ok, done in 68aebbc.

I'm still not convinced that removing the public key check in the browser is a good idea. If the server's key is (accidentally) changed it is not clear what's the problem (the webPush.sendNotification() call will get a 400 response and that's it). The only way to then fix it is to go into the browser and manually unsubscribe (and refresh the page afterwards):

navigator.serviceWorker.getRegistration()
.then(registration => registration.pushManager.getSubscription())
.then(subscription => subscription.unsubscribe());

marco-c · 2018-03-03T18:24:00Z

push-payload/index.js

    if (subscription) {
      return subscription;
    }

-    // Otherwise, subscribe the user (userVisibleOnly allows to specify that we don't plan to
+    // Subscribe the user (userVisibleOnly allows to specify that we don't plan to


Nit: don't change this comment.

marco-c · 2018-03-03T18:24:28Z

push-payload/index.js

+    // urlBase64ToUint8Array() is defined in /tools.js
+    var convertedVapidKey = urlBase64ToUint8Array(vapidPublicKey);
+
+    // See if we already have a subscription with the server
    if (subscription) {


Move this at the beginning of the function.

marco-c · 2018-03-03T18:24:46Z

push-payload/index.js

-    // If a subscription was found, return it.
+  .then(async function(subscription) {
+    // Get the server's public key
+    var response = await fetch('./vapidPublicKey')


Nit: use let and const everywhere instead of var when possible.

marco-c · 2018-03-03T18:25:23Z

LGTM overall, I think we can convert the other recipes too now.

slokhorst · 2018-03-04T15:29:26Z

Converted all recipes and rebased.

I think we should remove push-simple, as it's pretty much identical to push-payload now, see https://github.com/mozilla/serviceworker-cookbook/issues/290#issuecomment-369330773.

marco-c · 2018-03-08T19:12:29Z

So many changes!
Overall it looks good, I need to set the Heroku key before merging.

marco-c · 2018-03-17T23:55:05Z

@delapuente do you want to take a look too?

delapuente · 2018-03-19T11:09:00Z

I'll do. Moved to Windows environment and I also need to set everything up again.

delapuente

It looks great to me. Everything is working except for one recipe. I highlighted the required changes.

@marco-c are we comfortable with using async functions?

delapuente · 2018-03-26T13:46:03Z

push-get-payload/server.js

-        endpoint: req.body.endpoint,
-        TTL: req.body.ttl,
-      })
+      payloads[req.body.endpoint] = payload;


You need to index the payloads map by the endpoint which is now inside the subscription object:

payloads[req.body.subscription.endpoint] = payload;

Oops! Strange I didn't notice this. Fixed.

The client sends the endpoint as a property of the subscription object

delapuente

Working fine. @marco-c if you are OK with using async functions, then we can merge.

marco-c

Yeah, using async functions sounds OK to me.

marco-c · 2018-03-28T15:15:26Z

I've generated VAPID keys and set the two env variables on Heroku.

marco-c · 2018-03-28T15:28:03Z

push-clients/server.js

+webPush.setVapidDetails(
+  'https://serviceworke.rs/',
+  process.env.VAPID_PUBLIC_KEY,
+  process.env.VAPID_PRIVATE_KEY,


Heroku is failing because of the comma here.

aliams · 2018-04-04T19:01:42Z

Looking over the changes, I see that subscribe() is called when the service worker registration promise resolves. Considering that the service worker may not actually be ready at that point, should we change it so that the subscribe() is called when navigator.serviceWorker.ready resolves?

slokhorst · 2018-04-04T19:03:17Z

@aliams see #170 for a discussion on that. (fine with me either way)

aliams · 2018-04-04T23:12:21Z

Thanks for the extra context. Because the ready promise is meant to be resolved when the registration has an active service worker and subscribe() requires an active service worker, I believe it would make sense to call subscribe when ready resolves. Currently, as it is, it will not work in insider builds of Edge, but if it's changed to use ready, it will work.

marco-c · 2018-04-07T16:18:14Z

#170 was closed because ready wasn't enough to ensure the page was controlled by the service worker (and you actually have to wait for the controllerchange event).
For web push, I think we don't need the page to be controlled but we just need the service worker to be ready.

@aliams could you file an issue about this?

aliams · 2018-04-08T02:56:06Z

Thank you @marco-c. I've just opened #302.

marco-c reviewed Feb 6, 2018

View reviewed changes

marco-c reviewed Feb 7, 2018

View reviewed changes

marco-c reviewed Mar 3, 2018

View reviewed changes

slokhorst added 2 commits March 3, 2018 22:32

Add tools.js, required for Push recipes

896b9cc

Convert all push recipes to use VAPID

fdf0806

slokhorst changed the title ~~[WIP] Convert push-payload example to use VAPID~~ Convert all push recipes to use VAPID Mar 4, 2018

marco-c requested a review from delapuente March 17, 2018 23:54

delapuente suggested changes Mar 26, 2018

View reviewed changes

Fix push-get-payload

a4c73db

The client sends the endpoint as a property of the subscription object

delapuente approved these changes Mar 27, 2018

View reviewed changes

delapuente requested a review from marco-c March 27, 2018 07:49

marco-c approved these changes Mar 28, 2018

View reviewed changes

marco-c merged commit 62561ee into mdn:master Mar 28, 2018

marco-c reviewed Mar 28, 2018

View reviewed changes

Convert all push recipes to use VAPID #294

Convert all push recipes to use VAPID #294

Conversation

slokhorst commented Jan 28, 2018

Choose a reason for hiding this comment

slokhorst Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slokhorst Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c commented Feb 6, 2018

slokhorst commented Feb 6, 2018

marco-c commented Feb 6, 2018

slokhorst commented Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

slokhorst Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slokhorst commented Feb 28, 2018

marco-c commented Mar 1, 2018

slokhorst commented Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c commented Mar 3, 2018

slokhorst commented Mar 4, 2018

marco-c commented Mar 8, 2018

marco-c commented Mar 17, 2018

delapuente commented Mar 19, 2018

delapuente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delapuente left a comment

Choose a reason for hiding this comment

marco-c left a comment

Choose a reason for hiding this comment

marco-c commented Mar 28, 2018

Choose a reason for hiding this comment

aliams commented Apr 4, 2018

slokhorst commented Apr 4, 2018 • edited Loading

aliams commented Apr 4, 2018

marco-c commented Apr 7, 2018

aliams commented Apr 8, 2018

slokhorst Feb 6, 2018 •

edited

Loading

slokhorst Feb 6, 2018 •

edited

Loading

slokhorst commented Feb 7, 2018 •

edited

Loading

slokhorst Feb 7, 2018 •

edited

Loading

slokhorst commented Mar 2, 2018 •

edited

Loading

slokhorst commented Apr 4, 2018 •

edited

Loading