If I cancel Sync's timed/automatic "master password" dialog, it pops up 2 warnings at bottom of browser

RESOLVED FIXED

Status

Cloud Services
Firefox Sync: Backend
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dholbert, Assigned: rnewman)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 501769 [details]
screenshot

STEPS TO REPRODUCE:
 1. Set up Firefox Sync in a profile with a Master Password
 2. (Re)start Firefox. Don't unlock your master password.
 3. After a few minutes (5? 10?), a master password dialog will suddenly pop up -- from Sync.  Click 'Cancel'.

ACTUAL RESULTS: Two scary-looking notification bars appear, as shown in screenshot.

I think this behavior changed semi-recently, though I'm not sure.
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b9pre) Gecko/20110106 Firefox/4.0b9pre
(Reporter)

Comment 1

7 years ago
NOTE: It may or may not be appropriate to issue *some* notification when the user refuses to unlock their master password, to let them know that Sync isn't able to run.

I'm filing this bug on the fact that we issue two differently-colored notifications simultaneously.
(Reporter)

Comment 2

7 years ago
>  3. After a few minutes (5? 10?), a master password dialog will suddenly pop up

At this point, I checked my log and noticed these lines:
>2011-01-06 12:32:11	Service.Main         DEBUG	Global Score threshold hit, triggering sync.
>2011-01-06 12:32:11	Service.Main         DEBUG	Syncing as soon as we're idle.
>2011-01-06 12:32:11	Service.Main         DEBUG	Idle timer created for sync, will sync after 5 seconds of inactivity.

From this point on, after every 5 seconds of inactivity I get a Master Password dialog spammed at me (which triggers this bug's ACTUAL RESULTS each time).

(That dialog-spamming-frequency is unacceptable IMHO, but that's a different issue, and I think (?) there might already be a bug filed about that.)
(Assignee)

Comment 3

7 years ago
Thanks for the report.

Sync should certainly back off and have different warning behavior in this case.
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Component: Firefox Sync: UI → Firefox Sync: Backend
OS: Linux → All
QA Contact: sync-ui → sync-backend
Hardware: x86_64 → All
(Reporter)

Comment 4

7 years ago
(In reply to comment #2)
> From this point on, after every 5 seconds of inactivity I get a Master Password
> dialog spammed at me (which triggers this bug's ACTUAL RESULTS each time).

(Actually it's not quite as bad as that, but it's still pretty bad -- I filed bug 623702 on the master-password-prompt-spamming frequency, with more details over there)
(Assignee)

Comment 5

7 years ago
With latest sync build I get one error (just the bottom one) if I force sync. Log is:

2011-01-06 12:58:01	Service.Main         INFO	Logging in user 4cjkw5itifp5otqnpfta2gemi2ao3w6q
2011-01-06 12:58:03	Service.Main         DEBUG	verifyLogin failed: User canceled master password entry JS Stack trace: Res_get()@resource.js:376 < ()@service.js:830 < WrappedNotify()@util.js:151 < verifyLogin()@service.js:812 < ()@service.js:1067 < WrappedNotify()@util.js:151 < WrappedLock()@util.js:119 < WrappedCatch()@util.js:97 < WeaveSvc_login()@service.js:1043 < SUI_doSync([object XULCommandEvent])@browser.js:5213 < oncommand([object XULCommandEvent])@browser.xul:1
2011-01-06 12:58:03	Service.Main         DEBUG	Exception: Login failed: error.login.reason.network No traceback available


Timed sync might enter login in a different way. Should be easy enough to fix; I'll get to this this afternoon.
(Assignee)

Comment 6

7 years ago
Timed syncs throw two errors:

2011-01-06 13:02:20	Service.Main         DEBUG	Idle timer created for sync, will sync after 5 seconds of inactivity.
2011-01-06 13:02:54	Service.Main         INFO	Logging in user 4cjkw5itifp5otqnpfta2gemi2ao3w6q
2011-01-06 13:03:00	Service.Main         DEBUG	verifyLogin failed: User canceled master password entry JS Stack trace: Res_get()@resource.js:376 < ()@service.js:830 < WrappedNotify()@util.js:151 < verifyLogin()@service.js:812 < ()@service.js:1067 < WrappedNotify()@util.js:151 < WrappedLock()@util.js:119 < WrappedCatch()@util.js:97 < WeaveSvc_login()@service.js:1043 < sync(false)@service.js:1572 < ([object Object])@service.js:558 < notify([object XPCWrappedNative_NoHelper])@util.js:1130
2011-01-06 13:03:00	Service.Main         DEBUG	Exception: Login failed: error.login.reason.network No traceback available
2011-01-06 13:03:00	Service.Main         INFO	In sync().
2011-01-06 13:03:00	Service.Main         DEBUG	Syncing as soon as we're idle.
2011-01-06 13:03:00	Service.Main         DEBUG	Idle timer created for sync, will sync after 5 seconds of inactivity.
2011-01-06 13:03:00	Service.Main         DEBUG	Exception: Can't sync: User is not logged in No traceback available
(Assignee)

Updated

7 years ago
Assignee: nobody → rnewman
(Assignee)

Comment 7

7 years ago
OK, the lesson in all of this is that Password Manager *throws a string exception* if the user cancels the MP dialog.

This causes Sync's error handling to think there's a network error.

My proposed solution:

* Try a password retrieval in verifyLogin. If an exception is thrown...
* Set Status.login to a new constant (e.g., MASTER_PASSWORD_LOCKED).
* In timed syncs, check for this status and back off if the MP is still locked.
* In Sync Now syncs, ignore the status and attempt to log in, which will try to unlock the MP.

The only issue then is to decide whether we should occasionally prompt, and if so how often...
(Reporter)

Comment 8

7 years ago
(In reply to comment #7)
> The only issue then is to decide whether we should occasionally prompt, and if
> so how often...

If possible, it might be simpler to just fix the network error / notification issue here, and then decide on the prompt-frequency in bug 623702.  Maybe it's all tied up together, though.

(I think mconnor spent some time making Master Password interactions more sane at some point in the past, but I don't remember if there was a final decision about when / how often to prompt.)
(Assignee)

Comment 9

7 years ago
(In reply to comment #8)

> If possible, it might be simpler to just fix the network error / notification
> issue here, 

Not really. The Password Manager throws an exception if you cancel that dialog, so we have to do *something* to avoid that appearing as a Big Bad Error.

> and then decide on the prompt-frequency in bug 623702.  Maybe it's
> all tied up together, though.

To me it seems like there are several different parts to the notification story:

* Something unexpected happened (e.g., network or server error), and the user might be able to do something about it... or it could be transient. How often do we give an informational popup?

* Your configuration is wrong (wrong sync key). You might not have time to fix it now, but we do need to tell you.

* The user made a choice that affects syncing (cancel master password dialog), and might want to be reminded again later. 

Incidentally, the old state of Sync (prior to Bug 543784) was users like zandr complaining about going a week without a Sync completing, because we quietly waited until the master password was unlocked...

> (I think mconnor spent some time making Master Password interactions more sane
> at some point in the past, but I don't remember if there was a final decision
> about when / how often to prompt.)

Bug 543784 covers some of this, which I committed a little while ago.
(Reporter)

Comment 10

7 years ago
(In reply to comment #9)
> Incidentally, the old state of Sync (prior to Bug 543784) was users like zandr
> complaining about going a week without a Sync completing, because we quietly
> waited until the master password was unlocked...

(Yeah -- as noted in comment 1, I think makes sense to do at least one automatic master-password-prompt-inducing sync, and to subtly notify the user of the consequences of canceling that dialog, so that users don't unwittingly go unsynced for weeks at a time.)
(Assignee)

Comment 11

7 years ago
Created attachment 501937 [details] [diff] [review]
Proposed patch. v1

The attached does the following:

* Extracts the custom lock-detecting catch method, and uses it for login as well as sync. This is to preserve our logging behavior when login fails within a sync -- we still want to log "already syncing?".

* Introduces a constant for the state MASTER_PASSWORD_LOCKED -- that state in which we've tried to get protected data, and have been refused. The combination of this state and Utils.mpLocked() together dictate whether we should throw a warning or try a sync -- for so long as they both are in effect, we stay quiet.

* Attempts to fetch passphrase *before* fetching info/collections. The latter requires accessing the user's password; if they hit Cancel, we get an error inside Res_get. A test retrieval beforehand gives us the opportunity to detect the error from Password Manager.

* Augments _checkSync to include the additional reason (kSyncMasterPasswordLocked). Neither this nor MASTER_PASSWORD_LOCKED is user-visible, and so no localization is needed.

* Checks Status.service in login(), returning false if verifyLogin failed because the master password was locked.

* Extends sync() and syncOnIdle() to be aware of master password locking and the result of login().

* Necessary changes to Utils.catch and Utils.isLockException.

Tests and crossweave pass. I verified behavior in Minefield; a master password prompt occurs once, and if dismissed does not reoccur until either Sync Now is clicked, or Minefield is restarted.
Attachment #501937 - Flags: review?(philipp)
(Assignee)

Comment 12

7 years ago
See Bug 623940 to add CrossWeave (2) tests around this. I don't think there's much logic in this patch that we can test in xpcshell, but I'll add some tests for the utilities later today.
(Assignee)

Comment 13

7 years ago
Created attachment 502076 [details] [diff] [review]
Proposed patch. v2

Here's a version with a pile of tests.
Attachment #501937 - Attachment is obsolete: true
Attachment #502076 - Flags: review?(philipp)
Attachment #501937 - Flags: review?(philipp)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 623702
Comment on attachment 502076 [details] [diff] [review]
Proposed patch. v2

>+          this.passphrase;
>+        } catch (ex) {
>+          this._log.debug("Fetching passphrase threw " + ex +
>+                          "; assuming master password locked.");
>+          Status.login   = STATUS_DISABLED;
>+          Status.service = MASTER_PASSWORD_LOCKED;
>+          return false;
>+        }

So far STATUS_DISABLED has only been used as a Status.service constant. Is there a reason we're not using MASTER_PASSWORD_LOCKED for Status.login? It seems more appropriate there than on Status.service. Setting Status.login to anything that's not LOGIN_SUCCEEDED will automatically set Status.service to LOGIN_FAILED.

>+    // Unstub.
>+    Utils.mpLocked = mpLockedF;
>+    Service._clearSyncTriggers = oldClearSyncTriggers;
>+    Service._lockedSync = oldLockedSync;

It's nice of you but not really necessary. The process ends about 4 lines down from here. (And *if* it were necessary, you would find out that you forgot to unstub Service.passphrase here ;))

r=me but we should sort out the status constants before landing
Attachment #502076 - Flags: review?(philipp) → review+
(Assignee)

Comment 16

7 years ago
(In reply to comment #15)
> Comment on attachment 502076 [details] [diff] [review]
> Proposed patch. v2
> 
> >+          this.passphrase;
> >+        } catch (ex) {
> >+          this._log.debug("Fetching passphrase threw " + ex +
> >+                          "; assuming master password locked.");
> >+          Status.login   = STATUS_DISABLED;
> >+          Status.service = MASTER_PASSWORD_LOCKED;
> >+          return false;
> >+        }
> 
> So far STATUS_DISABLED has only been used as a Status.service constant. Is
> there a reason we're not using MASTER_PASSWORD_LOCKED for Status.login?

If you're addressing "why set both": well, otherwise Status.login is SUCCESS, and that ain't right :)

I opted to use STATUS_DISABLED for Status.login just for the sake of accuracy -- "login is disabled because MP is locked" -- but M_P_L would do the job too.

If you're addressing the broader point of ".login versus .service": I wrestled with that myself. I went with service because all of the login statuses were around failures, and this isn't a failure -- it's opting not to even try to login. From that perspective it's just like CLIENT_NOT_CONFIGURED or STATUS_DISABLED.

(See also below.)

> seems more appropriate there than on Status.service. Setting Status.login to
> anything that's not LOGIN_SUCCEEDED will automatically set Status.service to
> LOGIN_FAILED.

This was another motivation for using Status.service -- login didn't fail, we're opting not to bother. The existing logic for setting Status.login implies that there's been an attempted login and it failed.

> It's nice of you but not really necessary. The process ends about 4 lines down
> from here. (And *if* it were necessary, you would find out that you forgot to
> unstub Service.passphrase here ;))

Heh.

Yes, this is a little over-thorough... but we've been bitten before by cross-test state (and even later-additions-to-the-test-suite state!). I mainly wanted to leave logic there in anticipation of later need.
(In reply to comment #16)
> If you're addressing "why set both": well, otherwise Status.login is SUCCESS,
> and that ain't right :)

Of course :)

> I opted to use STATUS_DISABLED for Status.login just for the sake of accuracy
> -- "login is disabled because MP is locked" -- but M_P_L would do the job too.
> 
> If you're addressing the broader point of ".login versus .service": I wrestled
> with that myself. I went with service because all of the login statuses were
> around failures, and this isn't a failure -- it's opting not to even try to
> login. From that perspective it's just like CLIENT_NOT_CONFIGURED or
> STATUS_DISABLED.

Or more concrete: MASTER_PASSWORD_LOCKED.

My reasoning is this: Usually the Service or the UI calls login(). If that returns false, login has failed and we're not logged in. Then whoever called login() can use the Status.login value to find out why it failed. I think this exact scenario applies here as well. login() returns false if the MP is locked. We failed to log in. Whether that's due to the client or the server doesn't matter much IMHO.

So to be clear, I think we should just set Status.login = MASTER_PASSWORD_LOCKED. Status.service will automatically be set to LOGIN_FAILED then.

> > seems more appropriate there than on Status.service. Setting Status.login to
> > anything that's not LOGIN_SUCCEEDED will automatically set Status.service to
> > LOGIN_FAILED.
> 
> This was another motivation for using Status.service -- login didn't fail,
> we're opting not to bother. The existing logic for setting Status.login implies
> that there's been an attempted login and it failed.

I would say that we did attempt and failed. At least that's what it looks like to the caller of login().

> Yes, this is a little over-thorough... but we've been bitten before by
> cross-test state (and even later-additions-to-the-test-suite state!). I mainly
> wanted to leave logic there in anticipation of later need.

I would suggest not doing that but adding a comment that this test stanza has to be the last test run in the process because there's no clean up.
(Assignee)

Comment 18

7 years ago
Created attachment 502630 [details] [diff] [review]
Proposed patch. v3

Here's a better version. This incorporates your suggestions, Philipp, and additionally handles sync retries more intelligently (i.e., at all).

Per IRC I'm requesting another review to check my logic changes.

The old code relied on an exception to prompt the next sync. Once login() and sync() attempts are quiet (as they are for MP situations -- so as to not throw a yellow bar), these exceptions don't cause a timed sync.

Changes from the last patch:

* Introduce a MASTER_PASSWORD_LOCKED_RETRY_INTERVAL (15 mins).

* More logging (as always).

* Ignoring another _checkSync result code for flow reasons.

* Rejigging the way scheduled/idle log output is written to make it make sense!

* Refactor out the backoff/schedule logic into a new _scheduleAtInterval function, with a minimum time input to avoid needing to log sync errors in order to prompt backoff.

* Add scheduling at the above interval in two places:
  - In syncOnIdle() when we decide not to because MP is locked;
  - In sync() where we check login(). Yes, login() can fail for many other reasons... but because we're returning, we need to schedule anyway. I think this is a reasonable compromise to get useful behavior.

* Amended tests.


Tests pass, and I manually verified behavior around timed/forced syncs.
Attachment #502076 - Attachment is obsolete: true
Attachment #502630 - Flags: review?(philipp)
Comment on attachment 502630 [details] [diff] [review]
Proposed patch. v3

>   sync: function sync() {
>-    try {
>+    this._log.debug("In wrapping sync().");
>+    this._catch(function () {
>       // Make sure we're logged in.
>       if (this._shouldLogin()) {
>-        this._log.trace("In sync: should login.");
>-        this.login();
>+        this._log.debug("In sync: should login.");
>+        if (!this.login()) {
>+          this._log.debug("Not syncing: login returned false.");
>+          this._clearSyncTriggers();    // No more pending syncs, please.
>+          
>+          // Try again later, just as if we threw an error... only without the
>+          // error count.
>+          this._scheduleAtInterval(MASTER_PASSWORD_LOCKED_RETRY_INTERVAL);
>+          return;

As discussed on IRC, I think we only want to call _scheduleAtInterval() if the login failure wasn't due to LOGIN_FAILED_INVALID_PASSPHRASE or LOGIN_FAILED_LOGIN_REJECTED.

Looks great otherwise!
Attachment #502630 - Flags: review?(philipp) → review+
(Assignee)

Comment 20

7 years ago
Pushed:

http://hg.mozilla.org/services/fx-sync/rev/eda8ed4b8db3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

7 years ago
N.B., I didn't push this to 1.6.x; the patch doesn't apply cleanly, and I don't want to spent the time if we'll be rolling up 1.7.0 soon.
Post-mortem blocking-. It's a non-default scenario (master password) which is odd, but doesn't cause dataloss, and doesn't break any major functionality. Doesn't break anything at all, from the comments.
blocking2.0: ? → -

Comment 23

7 years ago
As the other bug was duped to this, is there a follow up bug for doing something more sane with the prompting? Or should there be one?

Prompting for the master password at a seemingly random point shortly after the opening Firefox seems annoying. May as well just prompt at start up, IMHO.

I understand that people may not be happy if sync doesn't happen for a week because the password is locked. But on the other hand, if I synced a few hours ago, I'd be happy if I could use Firefox for 10 minutes without getting prompted.
You need to log in before you can comment on or make changes to this bug.