Implement Android GCM-based PushService protocol

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

This ticket tracks implementing a PushService protocol, analogous to PushService{Http2,WebSocket} that co-ordinates with Fennec's GCM and Autopush implementation to register, unregister, and deliver push messages.
Created attachment 8726003 [details]
MozReview Request: Bug 1214338 - Implement Android GCM-based PushService protocol. r=rnewman r?kitcambridge

Review commit: https://reviewboard.mozilla.org/r/37753/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37753/
Attachment #8726003 - Flags: review?(rnewman)
Attachment #8726003 - Flags: review?(kcambridge)
Comment on attachment 8726003 [details]
MozReview Request: Bug 1214338 - Implement Android GCM-based PushService protocol. r=rnewman r?kitcambridge

https://reviewboard.mozilla.org/r/37753/#review34493

This looks really good, Nick! Thanks so much for doing this.

::: dom/push/PushServiceAndroidGCM.jsm:76
(Diff revision 1)
> +    if (prefs.get("debug") && serverURI.scheme == "http") {

I wonder if it makes sense to have the `prefs.get("debug")` check in `PushService._findService` (https://dxr.mozilla.org/mozilla-central/rev/4ea7408b3eef059aa248f4b00328f8fdb4475112/dom/push/PushService.jsm#350), and have it skip `isURIPotentiallyTrustworthy` if that's the case.

::: dom/push/PushServiceAndroidGCM.jsm:91
(Diff revision 1)
> +        return;

Nit: please move this `return` one level out.

::: dom/push/PushServiceAndroidGCM.jsm:132
(Diff revision 1)
> +        cryptoParams = {

This will need some minor changes to handle the new encryption scheme. `PushCrypto.getCryptoParams` can help clean this up:

    cryptoParams = getCryptoParams({
      encryption_key: data.enckey,
      crypto_key: data.cryptokey,
      encryption: data.enc,
    });

::: dom/push/PushServiceAndroidGCM.jsm:193
(Diff revision 1)
> +        if (subscriptions.hasOwnProperty(record.channelID)) {

Nit: For consistency, please use `record.keyID`.

::: dom/push/PushServiceAndroidGCM.jsm:235
(Diff revision 1)
> +      return PushCrypto.generateKeys()

Ugh, sorry about all the duplication. It would be nice if `PushService.jsm` could take care of this, so we don't have to generate the keys everywhere.

::: dom/push/PushServiceAndroidGCM.jsm:244
(Diff revision 1)
> +            quota: record.maxQuota,

It's OK to remove this line; `maxQuota` is no longer used.

::: dom/push/PushServiceAndroidGCM.jsm:248
(Diff revision 1)
> +            p256dhPrivateKey: exportedKeys[1],

We'll want to generate the secret here, too: `authenticationSecret: PushCrypto.generateAuthenticationSecret()`.

::: dom/push/PushServiceAndroidGCM.jsm:257
(Diff revision 1)
> +      channelID: record.channelID,

Nit: record.keyID

::: dom/push/PushServiceAndroidGCM.jsm:275
(Diff revision 1)
> +// Should we expose the channelID in this way?  To both places?

Actually, I think you can remove both these overrides. These methods are only used to send the subscription information over IPC, which `nsIPushService` rewraps in an `nsIPushSubscription` (https://dxr.mozilla.org/mozilla-central/source/dom/push/PushComponents.js#75,119,139,353).

::: dom/push/moz.build:17
(Diff revision 1)
>      'PushServiceHttp2.jsm',

Do you think we can skip these modules for Fennec, too?

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:166
(Diff revision 1)
> +                data.put("enckey", bundle.getString("enckey"));

I think we'll want to forward `cryptokey` to Gecko here, too. autopush should send `"cryptokey"` xor `"enckey"` in the body.
Attachment #8726003 - Flags: review?(kcambridge) → review+
https://reviewboard.mozilla.org/r/37753/#review34833

::: dom/push/PushServiceAndroidGCM.jsm:76
(Diff revision 1)
> +    if (prefs.get("debug") && serverURI.scheme == "http") {

I'd be happy to unify this, but I also want to do GCM-specific things in response to "dom.push.debug" toggling, so I'm not going to lift it to the service for this ticket.

::: dom/push/PushServiceAndroidGCM.jsm:235
(Diff revision 1)
> +      return PushCrypto.generateKeys()

I agree, and previously filed Bug 1214370 to track it.  Not this ticket, though :)

::: dom/push/PushServiceAndroidGCM.jsm:275
(Diff revision 1)
> +// Should we expose the channelID in this way?  To both places?

Dropped.

::: dom/push/moz.build:17
(Diff revision 1)
>      'PushServiceHttp2.jsm',

I think we can, although I want to run tests against them.  I'll try disabling them and see how we go.

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:166
(Diff revision 1)
> +                data.put("enckey", bundle.getString("enckey"));

Updated.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49acabe9d6b6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Attachment #8726003 - Flags: review?(rnewman) → review+
You need to log in before you can comment on or make changes to this bug.