Closed Bug 1267760 Opened 3 years ago Closed 3 years ago

Send push public key and auth secret when registering/updating a device

Categories

(Firefox :: Sync, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(1 file, 4 obsolete files)

We need these keys to use push payloads on the server side.
Flags: firefox-backlog+
Extended description: We want the Fxa server to be able to send different types of notifications to the desktop client (new device joined, account verified, etc) in the future.
Sending data along a push notification helps us differentiate these commands with a single endpoint.
In order to send data payloads, we need to provide a couple of crypto keys to the server to encrypt the message when we register or update a device.
Attached patch bug-1267760.patch (obsolete) — Splinter Review
We shouldn't merge this until the FxA servers accept push keys again
Attachment #8745805 - Flags: review?(markh)
Comment on attachment 8745805 [details] [diff] [review]
bug-1267760.patch

Review of attachment 8745805 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me, but I suspect Kit better understands if this is the right thing to do.

::: services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js
@@ +106,5 @@
>            resolve({
> +            endpoint: "http://mochi.test:8888",
> +            getKey: function(type) {
> +              if (type === "auth") {
> +                return "GSsIiaD2Mr83iPqwFNK4rw";

it seems like these should be constants?
Attachment #8745805 - Flags: review?(markh) → review?(kcambridge)
Attached patch bug-1267760.patch (obsolete) — Splinter Review
I refactored the tests
Attachment #8745805 - Attachment is obsolete: true
Attachment #8745805 - Flags: review?(kcambridge)
Attachment #8745816 - Flags: review?(kcambridge)
Note to myself: I'll have to change the reviewer name on the commit message.
Comment on attachment 8745816 [details] [diff] [review]
bug-1267760.patch

Review of attachment 8745816 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks great!

::: services/fxaccounts/FxAccounts.jsm
@@ +1498,5 @@
>        if (subscription && subscription.endpoint) {
>          deviceOptions.pushCallback = subscription.endpoint;
> +        let publicKey = subscription.getKey('p256dh');
> +        let authKey = subscription.getKey('auth');
> +        let urlsafeBase64Encode = key => ChromeUtils.base64URLEncode(new Uint8Array(key), { pad: false });

Nit: This could be a top-level function, but leaving it here is OK, too.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +367,5 @@
>     *         Extra device options
>     * @param  [options.pushCallback]
>     *         `pushCallback` push endpoint callback
> +   * @param  [options.pushPublicKey]
> +   *         `pushPublicKey` push public key

Nit: A comment that these are Base64 URL-encoded would be good.
Attachment #8745816 - Flags: review?(kcambridge) → review+
Attached patch bug-1267760.patch (obsolete) — Splinter Review
Thanks kit! Carrying r+ forward
Attachment #8745816 - Attachment is obsolete: true
Attachment #8746238 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/678d2aa3a3a6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
I asked Kit to back this out since I contradicted myself (see comment 2)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8746238 [details] [diff] [review]
bug-1267760.patch

I'm fairly sure that already registered devices will not register with this new info.
Attachment #8746238 - Flags: review+
Status: REOPENED → ASSIGNED
Depends on: 1268761
Priority: -- → P2
Blocks: 1267421
No longer blocks: 1201335
Attached patch bug-1267760.patch (obsolete) — Splinter Review
Rebased and bumped the DEVICE_REGISTRATION_VERSION constant we introduced in bug 1268761
Attachment #8746238 - Attachment is obsolete: true
Attachment #8749804 - Flags: review+
Attachment #8749804 - Attachment is obsolete: true
Rebased in prevision of the new fxa-auth-server release this week (with keys support!). ni myself to remind me to land this.
Flags: needinfo?(edouard.oger)
Attachment #8756118 - Flags: review?(kcambridge) → review+
Comment on attachment 8756118 [details]
MozReview Request: Bug 1267760 - Send push public key and auth secret when registering/updating a device. r=kitcambridge

https://reviewboard.mozilla.org/r/54962/#review51632
Keywords: checkin-needed
Flags: needinfo?(edouard.oger)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/7520b940afc8
Send push public key and auth secret when registering/updating a device. r=kitcambridge
Keywords: checkin-needed
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9681002&repo=fx-team
Flags: needinfo?(edouard.oger)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e3d409d10dea
Backed out changeset 7520b940afc8 for eslint failure
Comment on attachment 8756118 [details]
MozReview Request: Bug 1267760 - Send push public key and auth secret when registering/updating a device. r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54962/diff/1-2/
My bad, here it is corrected
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/2df619ad3af0
Send push public key and auth secret when registering/updating a device. r=kitcambridge
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2df619ad3af0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.