1214338 - Implement Android GCM-based PushService protocol

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: 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/

Comment 2

[Lina Butler [:lina]]

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.

Comment 3

[Nick Alexander :nalexander [he/him]]

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.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/49acabe9d6b6

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49acabe9d6b6