If you change your passphrase on one device, then you should not be able to continue syncing from other devices. So, basically this means Change Passphrase goes away and you can only Reset Passphrase. The reason we are doing this is because your passphrase is essentially your key and only you have access to it.

We discussed that perhaps we should just replace change with reset, which currently actively tells other clients to log out. But that remote command isn't necessary because resetPassphrase will also wipe the server and should eventually result in syncID changing, and remote clients seeing a syncID change will drop their cached keys and fail to sync.

Plan of record: Combine Reset Passphrase and Change Passphrase to use the reset passphrase codepath. (wipe server, generate new keys, re-upload from scratch, force other clients to log out) Existing, known codepaths, just need to design and implement some UI.

we should also check the code paths of where passwords and pass phrases might get cached. places like the password manager(s), and any internal caches. I was seeing many inconsistent results with testing the steps in the blocking bug 553756 and that suggests internal caches of both passwords and passphrases were not being cleared consistently and reliably.

In general, the passphrase caching isn't the issue, it's the caching of the encryption key, which this bug will address. I don't see anything about the password being improperly cached after a reset, can you file a bug on any password issues you have?

This works, not sure it's perfect, but wanted feedback ASAP.

Comment on attachment 436400 [details] [diff] [review] v1 >+++ b/source/chrome/content/generic-change.js > switch (Weave.Utils._genericDialogType) { >- case "ResetPassphrase": >+ case "ChangePassphrase": The UI bits in fx-prefs need to be updated to only have the one ChangePassphrase now. Right now resetPassphrase is provided conditionally only if the passphrase fails and changePassphrase is available on login. The conditional reset also appears when setting up a second machine and you enter the wrong passphrase. >+ validate: function () { >+ else if (val1 && val2 && val1 == val2 && Shouldn't this be a separate check to inform the user that the passwords aren't the same? >+ val1.length >= Weave.MIN_PASS_LENGTH) Did we want the password to be this long? It used to be 8. The passphrase is 12 I think. But now there's only one value and both need to be 12? >+++ b/source/chrome/content/generic-change.xul >+ <description flex="1"> >+ <html:p style="margin-top: 2px" id="introText"/> >+ <html:p id="introText2"/> >+ </description> Is the description not sufficient and html:p are needed? o.O? >+++ b/source/chrome/content/sync.js >- // Don't notify on missing passphrase errors >- if (!Weave.Service.passphrase) >- return; >+ // if login fails, any other notifications are essentially moot >+ Weave.Notifications.removeAll(); This silent passphrase failure is to prevent notifications from showing up when creating a new account and signing in on a second machine. The secret phrase panel is separate, so it tries logging in first with just the password. So I guess extra logic will be needed to clear out the notification from those interfaces. >+ buttons.push(new Weave.NotificationButton( >+ this._stringBundle.getString("error.login.prefs.label"), >+ this._stringBundle.getString("error.login.prefs.accesskey"), >+ function() { gWeaveWin.openPrefs(); return true; } >+ )); Good. Curious, any idea how this will interact with zpao's "weave needs to be setup" status text where clicking it will bring up the pref window? Should we just be opening the pref window on failure? >+++ b/source/modules/service.js > case "weave:service:sync:error": > this._handleSyncError(); >+ if (this._needToReLogin) { >+ this.logout(); >+ Utils.delay(function() this.login(), 0, this); >+ this._needToReLogin = false; What does this accomplish? Are you just trying to trigger the login:error notification? The needToReLogin was set because login failed on syncid change and it's a bad password/passphrase. You could just send the login:error notification here instead.. but I suppose you lose out on the reason for login failure.... Does Status.login get reset by this point? >+ changePassphrase: function WeaveSvc_changePassphrase(newphrase) >+ this._catch(this._notify("changepph", "", function() { > /* Make remote commands ready so we have a list of clients beforehand */ > this.prepCommand("logout", []); > let clientsBackup = Clients._store.clients; > > /* Wipe */ > this.wipeServer(); > PubKeys.clearCache(); > PrivKeys.clearCache(); Do we still need the prepCommand logout stuff? The syncID change + failed verifyLogin should do the trick. This code should just call resetService or actually _freshStart to do reset + wipe. Probably need to persistLogin() too.. >+ // bug 545725 - re-verify creds and fail sanely >+ if (!this._verifyLogin()) { >+ this._needToReLogin = true; >+ return false; Should be able to reuse Status.sync and set a new constant.. CREDENTIALS_CHANGED instead of adding a new _needToReLogin property.

(In reply to comment #9) > The UI bits in fx-prefs need to be updated to only have the one > ChangePassphrase now. Right, fixed. > >+ validate: function () { > >+ else if (val1 && val2 && val1 == val2 && > Shouldn't this be a separate check to inform the user that the passwords aren't > the same? I don't really want to do that, since that will basically always be true. > >+ val1.length >= Weave.MIN_PASS_LENGTH) > Did we want the password to be this long? It used to be 8. The passphrase is 12 > I think. But now there's only one value and both need to be 12? Mmm, right, fixed. We should probably look at hooking up a pw strength indicator. > >+++ b/source/chrome/content/generic-change.xul > >+ <description flex="1"> > >+ <html:p style="margin-top: 2px" id="introText"/> > >+ <html:p id="introText2"/> > >+ </description> > Is the description not sufficient and html:p are needed? o.O? Yeah, description can't be set dynamically _and_ wrap... <description> is such a dumb tag. > >+++ b/source/chrome/content/sync.js > >- // Don't notify on missing passphrase errors > >- if (!Weave.Service.passphrase) > >- return; > >+ // if login fails, any other notifications are essentially moot > >+ Weave.Notifications.removeAll(); > This silent passphrase failure is to prevent notifications from showing up when > creating a new account and signing in on a second machine. The secret phrase > panel is separate, so it tries logging in first with just the password. So I > guess extra logic will be needed to clear out the notification from those > interfaces. Yeah, but that means if your pp goes missing, there's no indication something is wrong. Not exactly awesome. > >+ buttons.push(new Weave.NotificationButton( > >+ this._stringBundle.getString("error.login.prefs.label"), > >+ this._stringBundle.getString("error.login.prefs.accesskey"), > >+ function() { gWeaveWin.openPrefs(); return true; } > >+ )); > Good. Curious, any idea how this will interact with zpao's "weave needs to be > setup" status text where clicking it will bring up the pref window? That'll trigger in other cases, this would be "you've set up the client, but you have a problem" > Should we just be opening the pref window on failure? No, except possibly in response to a user action. > >+++ b/source/modules/service.js > > case "weave:service:sync:error": > > this._handleSyncError(); > >+ if (this._needToReLogin) { > >+ this.logout(); > >+ Utils.delay(function() this.login(), 0, this); > >+ this._needToReLogin = false; > What does this accomplish? Are you just trying to trigger the login:error > notification? The needToReLogin was set because login failed on syncid change > and it's a bad password/passphrase. You could just send the login:error > notification here instead.. but I suppose you lose out on the reason for login > failure.... > > Does Status.login get reset by this point? I don't see why we'd fake login failure instead of just actually retrying login and not worrying about manually setting various other states... we know it's going to fail, but logging out/logging in means we do not have to duplicate functionality. > >+ changePassphrase: function WeaveSvc_changePassphrase(newphrase) > >+ this._catch(this._notify("changepph", "", function() { > > /* Make remote commands ready so we have a list of clients beforehand */ > > this.prepCommand("logout", []); > > let clientsBackup = Clients._store.clients; > > > > /* Wipe */ > > this.wipeServer(); > > PubKeys.clearCache(); > > PrivKeys.clearCache(); > Do we still need the prepCommand logout stuff? The syncID change + failed > verifyLogin should do the trick. the failed verifyLogin is because the logout stuff doesn't work (encrypted commands with a different key == useless!) > This code should just call resetService or actually _freshStart to do reset + > wipe. Probably need to persistLogin() too.. Let's fix that separately, trying to not conflate too much here... > >+ // bug 545725 - re-verify creds and fail sanely > >+ if (!this._verifyLogin()) { > >+ this._needToReLogin = true; > >+ return false; > Should be able to reuse Status.sync and set a new constant.. > CREDENTIALS_CHANGED instead of adding a new _needToReLogin property. Yeah, good idea. Fixed.

Comment on attachment 436761 [details] [diff] [review] v1.1 (In reply to comment #10) > the failed verifyLogin is because the logout stuff doesn't work (encrypted > commands with a different key == useless!) We should at least not call it and do the clients store saving which shouldn't work right now anyway.

Comment on attachment 436761 [details] [diff] [review] v1.1 >+++ b/source/chrome/content/preferences/fx-setup.js >- if (state == 0 && (val.length < 12 || valConfirm.length < val.length)) >+ if (state == 0 && (val.length < Weave.MIN_PASS_LENGTH || valConfirm.length < val.length)) This should be MIN_PP_LENGTH

http://hg.mozilla.org/labs/weave/rev/eb8ef75539b9 Followup to bug 545725 to have creating new account passphrase check for 12 instead of 8.

verified with 1.2b4 changing passphrase on client A prevents client B from sync until the new passphrase is entered on client B (which weave on client B prompts for)