Skip to content
This repository has been archived by the owner on Apr 23, 2021. It is now read-only.

Allow sending with no payload, plus some small fixes #17

Merged
merged 3 commits into from
Apr 28, 2016
Merged

Conversation

wibblymat
Copy link
Contributor

@gauntface

Fixes #10

* @return {Promise} A promise that resolves if the push was sent successfully
* with status and body.
*/
function sendWebPush(message, subscription, authToken) {
function sendWebPush(message, subscription, authToken, paddingLength) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you switch around the paddingLength with the authToken? When GCM goes away, the only change will be remove the authtoken.

I'm still pro just making a special method of GCM Auth header, simplifying sendWebPush method.

This change will also likely result in developers always sending the GCM API Key to all push services :(

@gauntface
Copy link
Contributor

cc @petele

Awesome - thanks for doing this.

Only concern is still with the auth token, I can see people doing:

subscriptions.forEach(subscription => {
    if (!subscription.aesgcm) {
        return;
    }
    pushEncryption.sendWebPush('Totes rad msg', subscription, GCM_API_KEY);
});

Which means they are sharing their API key with the world :(

If we changed back to a method, we can be smarter and only include the header for GCM requests.

pushEncryption.setGCMAPIKey(GCM_API_KEY);
subscriptions.forEach(subscription => {
    if (!subscription.aesgcm) {
        return;
    }
    pushEncryption.sendWebPush('Totes rad msg', subscription);
});

@addyosmani
Copy link

If we changed back to a method, we can be smarter and only include the header for GCM requests.

I would be strongly in favour of this if we can push for it. I had similar concerns to @gauntface about API keys being shared with the world and can see folks tripping up here :/

@wibblymat
Copy link
Contributor Author

@gauntface OK, done

@wibblymat wibblymat mentioned this pull request Apr 28, 2016
'Encryption': `salt=${ub64(payload.salt)}`,
'Crypto-Key': `dh=${ub64(payload.serverPublicKey)}`
// TODO: Make TTL variable
Ttl: '0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Mega nit, shouldn't this be TTL similar to: https://github.com/gauntface/simple-push-demo/pull/28/files

@gauntface
Copy link
Contributor

One small nit on TTL, other than that - LGTM

@wibblymat
Copy link
Contributor Author

@gauntface you're right, spec says TTL. No idea why I thought otherwise!

@wibblymat wibblymat merged commit 19a91f5 into master Apr 28, 2016
@wibblymat wibblymat deleted the no-payload branch April 28, 2016 09:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants