Closed
Bug 1368568
Opened 7 years ago
Closed 7 years ago
Remove SyncKeyBundle from keys.js
Categories
(Firefox :: Sync, enhancement, P3)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: eoger, Assigned: robcutmore, Mentored)
Details
(Keywords: good-first-bug)
Attachments
(1 file)
I probably missed this when removing the old legacy Sync code. It looks like it's only being used by tests now.
Updated•7 years ago
|
Keywords: good-first-bug
Priority: -- → P3
Reporter | ||
Updated•7 years ago
|
Mentor: eoger
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?
Reporter | ||
Comment 2•7 years 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)
Assignee | ||
Comment 3•7 years 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 hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years 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•7 years 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•7 years 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) |
Assignee | ||
Comment 11•7 years ago
|
||
Thanks Edouard, I pushed that change. I'm not familiar with Mercurial so hopefully I squashed my commits correctly.
Reporter | ||
Comment 12•7 years 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•7 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2af86a1d1f7a Remove legacy SyncKeyBundle. r=eoger
Reporter | ||
Comment 14•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2af86a1d1f7a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•6 years ago
|
Assignee: nobody → robcutmore
You need to log in
before you can comment on or make changes to this bug.
Description
•