|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
I probably missed this when removing the old legacy Sync code. It looks like it's only being used by tests now.
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?
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!
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!
Hi Sorry , got no time to work on this anymore.
I think these tests are safe to delete.
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.
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();
Comment on attachment 8889050 [details] Bug 1368568 - Remove legacy SyncKeyBundle. https://reviewboard.mozilla.org/r/160090/#review165866
Attachment #8889050 - Flags: review?(eoger)
Thanks Edouard, I pushed that change. I'm not familiar with Mercurial so hopefully I squashed my commits correctly.
Comment on attachment 8889050 [details] Bug 1368568 - Remove legacy SyncKeyBundle. https://reviewboard.mozilla.org/r/160090/#review166282
Attachment #8889050 - Flags: review?(eoger) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2af86a1d1f7a Remove legacy SyncKeyBundle. r=eoger
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.
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.