1368568 - Remove SyncKeyBundle from keys.js

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

[Edouard Oger [:eoger]]

I probably missed this when removing the old legacy Sync code.
It looks like it's only being used by tests now.

Comment 1

[tfe]

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?

Comment 2

[Edouard Oger [:eoger]]

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!

Comment 3

[Rob Cutmore]

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!

Comment 4

[tfe]

Hi

Sorry , got no time to work on this anymore.

Comment 5

[Edouard Oger [:eoger]]

I think these tests are safe to delete.

Comment 6

[Rob Cutmore]

Attachment: Bug 1368568 - Remove legacy SyncKeyBundle.

Review commit: https://reviewboard.mozilla.org/r/160090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160090/

Comment 7

[Rob Cutmore]

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 8

[Edouard Oger [:eoger]]

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 9

[Edouard Oger [:eoger]]

Comment on attachment 8889050 [details]
Bug 1368568 - Remove legacy SyncKeyBundle.

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

Comment 10

[Rob Cutmore]

Comment on attachment 8889050 [details]
Bug 1368568 - Remove legacy SyncKeyBundle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160090/diff/1-2/

Comment 11

[Rob Cutmore]

Thanks Edouard, I pushed that change. I'm not familiar with Mercurial so hopefully I squashed my commits correctly.

Comment 12

[Edouard Oger [:eoger]]

Comment on attachment 8889050 [details]
Bug 1368568 - Remove legacy SyncKeyBundle.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2af86a1d1f7a
Remove legacy SyncKeyBundle. r=eoger

Comment 14

[Edouard Oger [: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.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2af86a1d1f7a