-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
@gauntface Sorry about the number of commits. I sort of let this one get a way from me. |
Hey @jpmedley - this is going into master, is it also ported into next-version? |
Doing that after LGTMs. |
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.
Sorry for the number of comments.
Feel free to ignore the GCM comments for this PR if you want to leave it as is or do seperately.
Only other thing to note is that the gif has two '5' steps and it would be awesome if it looped.
|
||
* The `push` event handler. | ||
* A call to `getNotifications()`. | ||
* reusing an existing `tag` value. | ||
* Setting the `renotify` flag in the call to `showNotification()`. |
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.
This section raises a lot of questions and doesn't really feel like it answer a lot but I think its because it's not written in a fact of metter way.
Maybe something along the lines of:
"When you want to combine notifications, the following events and steps need to be taken:
1.) A push message arrives in the push event handler.
2.) You call self.registration.getNotifications() <NOTE: CHECK THIS IS THE CORRECT API> to see if there are any notifications you want to combine, this is commonly done by checking the tag of the nofication.
3.) Finally show your new notification by calling self.registration.showNotification() making sure you set the renotify parameter to true in the options (See below for an example).
"
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.
Done.
@@ -178,7 +181,7 @@ self.addEventListener('push', function(event) { | |||
}); | |||
{% endhighlight %} | |||
|
|||
Check for notifications that match `data.tag` with a call to `getNotifications()`. | |||
Once we have the message data, call `getNotifications()` using `data.tag`. |
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.
data.tag is only a thing if the push message has a tag attribute.......Developers may determine tags from something completely else. This may introduce a confusion.
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.
I'm going to take this as a separate PR. I need to figure out how tag is added to a message and put it in sending-messages.md.
that all push messages sent the client will be shown to the user as a | ||
notification. We're also including an `applicationServerKey` converted to an | ||
integer array. | ||
that all push messages sent to the client will be shown to the user as a |
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.
This feels like an awkward phrasing. It's required by browsers and means that you must show a notification for every push message.
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.
Done.
@@ -102,6 +101,10 @@ This is the result in Chrome. | |||
|
|||
![Chrome prompts for permissions.](images/news-permissions.png){:width="296px"} | |||
|
|||
### What about the applicationServerKey? {#applicationserverkey} | |||
|
|||
You probably noticed that the call to `subscribe()` contains a parameter called `applicationServerKey`. Its value is generated by your server. We were saving all server side issue for the next section. For now, there's one thing you need to know about the `applicationServerKey`: your app should pass key to the server as an array of eight-bit unsigned integers. |
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.
The last paragraph could do with some tweaking.
"For now, there's one thing you need to know about the applicationServerKey: When passing in the key to a subscribe() call, make sure it's a Uint8Array (an array of eight-bit-unsigned integers)."
Main change - "should pass key to the server" <- This is incorrect. The browser passes it to the push service.
Secondary change is to simplify the eight-bit unsigned integers, it's not a specific as it could be.
Probably worth linking Uint8Array to: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Uint8Array
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.
Fixed.
@@ -180,3 +183,34 @@ if ('showNotification' in ServiceWorkerRegistration.prototype) { | |||
}); | |||
} | |||
{% endhighlight %} | |||
|
|||
### Upgrading from GCM to the web push protocal {#upgrad} |
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.
NIT: Change hash to 'upgrade'
Create the signature by first concatenating the JWT header and the payload with | ||
a dot, the encrypting the private key you created earlier. We're not going to | ||
show you how to do this, but there are a number of encryption libraries | ||
available. The result will look something like the following: |
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.
Maybe link to the JWT site with libraries available: https://jwt.io/
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.
Done.
show you how to do this, but there are a number of encryption libraries | ||
available. The result will look something like the following: | ||
|
||
<pre>Bearer eyJ0eXAiOiJKV1QiLCJhbGciOiJFUzI1NiJ9.eyJhdWQiOiJodHRwczovL2ZjbS5nb29nbGVhcGlzLmNvbSIsImV4cCI6MTQ2NjY2ODU5NCwic3ViIjoibWFpbHRvOnNpbXBsZS1wdXNoLWRlbW9AZ2F1bnRmYWNlLmNvLnVrIn0.Ec0VR8dtf5qb8Fb5Wk91br-evfho9sZT6jBRuQwxVMFyK5S8bhOjk8kuxvilLqTBmDXJM5l3uVrVOQirSsjq0A</pre> |
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.
WebPush
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 above.
browser doesn't support payloads, the subscription object won't contain keys. | ||
|
||
## Sending messages {#sending-messages} | ||
The payload, regardless of how you get it to the client, should be encrypted. |
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.
NIT: "has to be encrypted"
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.
Done.
The payload, regardless of how you get it to the client, should be encrypted. | ||
For payloads sent through the message server you must encrypt it using the | ||
publicKey and the authentication secret. It must also be padded, and salted with | ||
random 16 bytes, urlBase64 encoded. Finally, it's added to the body of the |
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.
this doesn't read well. "and salted with 16 random bytes and then url safe base64 encoded."
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.
I can't recall the specifics the body isn't base64 url encoded, but the encrypted bytes and sent to the web push service.
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.
Changed to: "It must also be salted with 16 random bytes that are unique to the message." I found that bit about it being unique when I looked it up in the spec.
Content-Encoding: aesgcm128 | ||
Encryption: keyid=p256dh;salt=xxxxxxx | ||
Crypto-Key: keyid=p256dh;dh=xxxxxxxx | ||
At some point the request is send to the message server. The web-push node |
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.
NIT: sent
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.
Fixed.
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.
Among all of the responses that say, "Done" are a few further questions. Can you please answer them.
specific endpoints on the messaging server. The messaging server handles the | ||
routing. </p> | ||
<p class="intro"> | ||
It's easy to assume that sending messages must involve building a complex |
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.
Removed.
specific endpoints on the messaging server. The messaging server handles the | ||
routing. </p> | ||
<p class="intro"> | ||
It's easy to assume that sending messages must involve building a complex |
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.
Started paragraph at "There are two...".
Note: You don't need to know the URL of the message server. Each browser vender manages it's own message server for its browser. | ||
|
||
1. The messaging server returns 201 to indicate that the subscription was successful. | ||
1. After processing the subscription flow, your app passes a subscription object back to your app server. |
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.
Reworked the diagram description a bit.
|
||
**endpoint**—Contains two parts: the URL of the messaging service used by the | ||
subscribing browser followed by unique identifier for the user. This is called a | ||
subscription ID or a registration ID. This tells your server how to identify |
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.
Done.
messages. It contains the following: | ||
|
||
* **auth**-A 16 byte authentication secret generated by the browser. | ||
* **p256dh**-A 65 byte containing the vapidPublicKey created on your app server. |
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.
Done.
|
||
* The `push` event handler. | ||
* A call to `getNotifications()`. | ||
* reusing an existing `tag` value. | ||
* Setting the `renotify` flag in the call to `showNotification()`. |
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.
Done.
@@ -178,7 +181,7 @@ self.addEventListener('push', function(event) { | |||
}); | |||
{% endhighlight %} | |||
|
|||
Check for notifications that match `data.tag` with a call to `getNotifications()`. | |||
Once we have the message data, call `getNotifications()` using `data.tag`. |
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.
I'm going to take this as a separate PR. I need to figure out how tag is added to a message and put it in sending-messages.md.
that all push messages sent the client will be shown to the user as a | ||
notification. We're also including an `applicationServerKey` converted to an | ||
integer array. | ||
that all push messages sent to the client will be shown to the user as a |
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.
Done.
@@ -102,6 +101,10 @@ This is the result in Chrome. | |||
|
|||
![Chrome prompts for permissions.](images/news-permissions.png){:width="296px"} | |||
|
|||
### What about the applicationServerKey? {#applicationserverkey} | |||
|
|||
You probably noticed that the call to `subscribe()` contains a parameter called `applicationServerKey`. Its value is generated by your server. We were saving all server side issue for the next section. For now, there's one thing you need to know about the `applicationServerKey`: your app should pass key to the server as an array of eight-bit unsigned integers. |
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.
Fixed.
@@ -180,3 +183,34 @@ if ('showNotification' in ServiceWorkerRegistration.prototype) { | |||
}); | |||
} | |||
{% endhighlight %} | |||
|
|||
### Upgrading from GCM to the web push protocal {#upgrad} |
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.
Removed the section.
@gauntface Lost in all of my response of 'done' are a few rebuttals and questions. Can you look at them today or tomorrow? |
Just responded to the questions, a few things that I think would be worth changing, otherwise just answers to questions. |
I'm not sure why that would be, but the point is moot. I've transferred the https://github.com/google/WebFundamentals/tree/push-hackathon Joe Medley | Technical Writer, Chrome DevRel | jmedley@google.com | On Tue, Sep 27, 2016 at 4:07 PM, Matthew Gaunt notifications@github.com
|
@petele everything in here has been addressed and moved to the new branch. https://github.com/google/WebFundamentals/tree/push-hackathon |
No description provided.