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

RESOLVED FIXED in Firefox 49

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: eoger, Assigned: eoger)

Tracking

unspecified
Firefox 49
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 years ago
We need these keys to use push payloads on the server side.
Flags: firefox-backlog+
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8745805 [details] [diff] [review]
bug-1267760.patch

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)
(Assignee)

Comment 4

2 years ago
Created attachment 8745816 [details] [diff] [review]
bug-1267760.patch

I refactored the tests
Attachment #8745805 - Attachment is obsolete: true
Attachment #8745805 - Flags: review?(kcambridge)
Attachment #8745816 - Flags: review?(kcambridge)
(Assignee)

Comment 5

2 years ago
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+
(Assignee)

Comment 7

2 years ago
Created attachment 8746238 [details] [diff] [review]
bug-1267760.patch

Thanks kit! Carrying r+ forward
Attachment #8745816 - Attachment is obsolete: true
Attachment #8746238 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/678d2aa3a3a6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(Assignee)

Comment 10

2 years ago
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+

Updated

2 years ago
Status: REOPENED → ASSIGNED

Updated

2 years ago
Depends on: 1268761
Priority: -- → P2

Updated

2 years ago
Blocks: 1267421
No longer blocks: 1201335
(Assignee)

Comment 13

2 years ago
Created attachment 8749804 [details] [diff] [review]
bug-1267760.patch

Rebased and bumped the DEVICE_REGISTRATION_VERSION constant we introduced in bug 1268761
Attachment #8746238 - Attachment is obsolete: true
Attachment #8749804 - Flags: review+
(Assignee)

Comment 14

2 years ago
Created attachment 8756118 [details]
MozReview Request: Bug 1267760 - Send push public key and auth secret when registering/updating a device. r=kitcambridge

Review commit: https://reviewboard.mozilla.org/r/54962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54962/
Attachment #8756118 - Flags: review?(kcambridge)
(Assignee)

Updated

2 years ago
Attachment #8749804 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
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
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Flags: needinfo?(edouard.oger)

Comment 17

2 years ago
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)

Comment 19

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/e3d409d10dea
Backed out changeset 7520b940afc8 for eslint failure
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
My bad, here it is corrected
Flags: needinfo?(edouard.oger)
Keywords: checkin-needed

Comment 22

2 years ago
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

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2df619ad3af0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.