Remove SyncKeyBundle from keys.js

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Sync
P3
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: eoger, Unassigned, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 56
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

8 months ago
I probably missed this when removing the old legacy Sync code.
It looks like it's only being used by tests now.
Keywords: good-first-bug
Priority: -- → P3
(Reporter)

Updated

8 months ago
Mentor: eoger@fastmail.com

Comment 1

8 months ago
Hi,

Can i work on this bug?

I think the "remove" part is easy, but what should be the replacement of new SyncKeyBundle() calls in tests?

Updated

8 months ago
Flags: needinfo?(eoger)
(Reporter)

Comment 2

8 months ago
Hello,

SyncKeyBundle has been replaced by BulkKeyBundle (defined in the same file) when we switched to Firefox accounts.
From a quick glance at the code, it appears that SyncKeyBundle is used in 3 tests.

In test_keys.js#test_sync_key_bundle_derivation it's pretty simple : Delete that test case.
For the two other tests, I would use a BulkKeyBundle instead. I think you can generate one by doing:

let key = new BulkKeyBundle(collection); // You can probably pass a bogus collection name
key.generateRandom();

Let me know if you're blocked again, thanks!
Flags: needinfo?(eoger)

Comment 3

6 months ago
Hello,

tfe,
Do you still plan to work on this?

Edouard,
I have a question about how to update the tests for this. I only see two tests with calls to SyncKeyBundle() (test_sync_key_bundle_derivation and test_keymanager). Currently test_keymanager manually generates encryption and HMAC keys and compares them to the keys generated by SyncKeyBundle. Is there an equivalent test that should be done with BulkKeyBundle?

The method to generate the keys for BulkKeyBundle doesn't take any input and they are random each time (each is generated with separate calls to Weave.Crypto.generateRandomKey()). Should we just be testing that the keys generated for two different BulkKeyBundle objects are different (with and without the same collection name) or maybe should this test be deleted as well?

Sorry for the long-winded questions. Thanks!
Flags: needinfo?(eoger)

Comment 4

6 months ago
Hi

Sorry , got no time to work on this anymore.
(Reporter)

Comment 5

6 months ago
I think these tests are safe to delete.
Flags: needinfo?(eoger)
Comment hidden (mozreview-request)

Comment 7

6 months ago
Hi Edouard,

I submitted changes to the review board: https://reviewboard.mozilla.org/r/160090/diff/1

This is my first submission so please let me know if I made any mistakes.
(Reporter)

Comment 8

6 months ago
mozreview-review
Comment on attachment 8889050 [details]
Bug 1368568 - Remove legacy SyncKeyBundle.

https://reviewboard.mozilla.org/r/160090/#review165862

Thank you! I think this is good start. There are still 2 other tests that contains SyncKeyBundle: test_service_detect_upgrade.js and test head_errorhandler_common.js. In both of these, it looks safe to replace the SyncKeyBundle construct by:

let newSyncKeyBundle = new BulkKeyBundle("crypto");
newSyncKeyBundle.generateRandom();
(Reporter)

Comment 9

6 months ago
mozreview-review
Comment on attachment 8889050 [details]
Bug 1368568 - Remove legacy SyncKeyBundle.

https://reviewboard.mozilla.org/r/160090/#review165866
Attachment #8889050 - Flags: review?(eoger)
Comment hidden (mozreview-request)

Comment 11

6 months ago
Thanks Edouard, I pushed that change. I'm not familiar with Mercurial so hopefully I squashed my commits correctly.
(Reporter)

Comment 12

6 months ago
mozreview-review
Comment on attachment 8889050 [details]
Bug 1368568 - Remove legacy SyncKeyBundle.

https://reviewboard.mozilla.org/r/160090/#review166282
Attachment #8889050 - Flags: review?(eoger) → review+

Comment 13

6 months ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2af86a1d1f7a
Remove legacy SyncKeyBundle. r=eoger
(Reporter)

Comment 14

6 months ago
Looks great thank you!
I fixed the commit message for you (it was duplicated, probably because of the squash) and pushed your commit to our servers.

Comment 15

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2af86a1d1f7a
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.