Closed Bug 614737 Opened 10 years ago Closed 10 years ago
Simplified crypto: ensure older clients detect outdated version
From bug 603489 comment 73: The storageVersion detection code so far in Sync does not anticipate a change in the crypto structure. Upon upgrade we delete keys/pubkey and keys/privkey. This makes older Sync versions think that the passphrase is wrong instead of surfacing the "Your version is too old" error message on Sync. It doesn't even get that far because it fails on login. This is very unfortunate. I believe we should do two things about this: 1. Do not delete keys/pubkey and keys/privkey upon migration. Just leave them around, they won't do harm and instead trick old clients into logging in properly first and then detecting their version is too old. 2. Make sure that we won't run into this problem in the future. This could be the case already since we now call remoteSetup() from verifyLogin() which definitely does this check somewhere, but it would good to have a test for this.
This patch adjusts wipeServer to leave the "keys" collection by default. Tests exercise this particular functionality. The scenario in question has been manually tested: * Create two new 3.6/1.5 profiles with the same Sync credentials * Sync successfully * Update one to the current build, watch it wipe server and upgrade * Attempt to sync other client: observe "upgrade needed" notification, rather than "wrong sync key". For good measure, I then verified that upgrading the other client will allow syncing to resume. IT LIVES! A subsequent patch will address the storage version change detection logic in an automated test. (Two separate patches so we can keep things moving.)
Attachment #493772 - Flags: review?(philipp)
This test verifies login, then verifies that sync fails for the correct reason if the server version is bumped. It verifies again after wiping the server, as would be done during an upgrade. Note that the old logic for signin/passphrase/key verification is no longer in the tree, so this test doesn't exercise it -- in other words, if you take out the fix, this test still passes. This test is still valuable to avoid a future regression.
Add a line to verify that login succeeded.
Comment on attachment 493772 [details] [diff] [review] Patch and tests for changes. > // Some tests hang on OSX debug builds. See bug 604565. > let DISABLE_TESTS_BUG_604565 = false; > #ifdef XP_MACOSX > #ifdef MOZ_DEBUG_SYMBOLS >-DISABLE_TESTS_BUG_604565 = true; >+DISABLE_TESTS_BUG_604565 = false; Please remove this :)
Attachment #493772 - Flags: review?(philipp) → review+
Attachment #493805 - Flags: review?(philipp) → review+
Pushed to fx-sync.
(In reply to comment #5) > Pushed to fx-sync. https://hg.mozilla.org/services/fx-sync/rev/66175eea6c82
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
verified testing recently nightly build to force update against a 1.5.1 ext on 3.6
Status: RESOLVED → VERIFIED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.