Last Comment Bug 624552 - In Fennec w/ locked Master Password, a manual Sync "Connect" goes to JPAKE instead of Master Password dialog
: In Fennec w/ locked Master Password, a manual Sync "Connect" goes to JPAKE in...
Status: VERIFIED FIXED
[qa!]
: regression, verified-aurora
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Matt Brubeck (:mbrubeck)
:
Mentors:
https://addons.mozilla.org/en-US/mobi...
Depends on:
Blocks: 592772
  Show dependency treegraph
 
Reported: 2011-01-10 15:24 PST by Daniel Holbert [:dholbert]
Modified: 2013-12-10 10:00 PST (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP (1.47 KB, patch)
2011-08-22 15:17 PDT, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Review
patch (2.78 KB, patch)
2011-08-22 17:15 PDT, Matt Brubeck (:mbrubeck)
mark.finkle: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review

Description Daniel Holbert [:dholbert] 2011-01-10 15:24:22 PST
STEPS TO REPRODUCE:
 1. Configure your Fennec profile to use Sync, if you haven't already
 2. Install mbrubeck's Master Password extension:
   https://addons.mozilla.org/en-US/mobile/addon/270907/
 3. Restart Fennec. Wait ~20 seconds to allow Sync to init itself.
    Cancel any automatic master password prompts that appear.
 4. Open Fennec Options, and under the "Sync" area, pick "Connect".

EXPECTED RESULTS: Fennec should prompt me for master password. (to unlock my Sync credentials)

ACTUAL RESULTS: Fennec takes me to a JPAKE 3-token screen, as if this is a new device that I'm connecting for the first time.
Comment 1 Daniel Holbert [:dholbert] 2011-01-10 15:47:24 PST
(In reply to comment #0)
> EXPECTED RESULTS: Fennec should prompt me for master password. (to unlock my
> Sync credentials)

This is what happens on Desktop Firefox/Minefield, if I do Tools | Sync Now when my Master Password is locked.  (Note that the "Sync Now" button isn't available on Fennec until after you've already authenticated with the Sync server)

FURTHER NOTE:  After performing the STR -- if I then go to a page for which I have a saved login (to trigger a Master Password prompt) and unlock my master password, I'd expect that at some point later, Sync would auto-login.  However, it doesn't ever seem to do so -- instead it just stays the same with the "Connect" button being the only thing available, and that button continues to trigger a JPAKE screen instead of using my now-unlocked credentials to connect.
Comment 2 Aakash Desai [:aakashd] 2011-01-24 09:36:51 PST
ping: matt?
Comment 3 Matt Brubeck (:mbrubeck) 2011-01-24 09:52:19 PST
I can look into this, but it's not high on my priority list right now.
Comment 4 Daniel Holbert [:dholbert] 2011-08-21 09:00:51 PDT
Now that bug 592772 has landed (adding Master Password support to Fennec), this has become a bug in stock Fennec. (rather than a bug in fennec-with-an-addon)

STR from comment 0 are still valid (just ignore step 2 for installing MP addon).

In addition, when I view the Fennec config page after canceling MP dialog, the sync section says "Not connected" / "Unknown error". (and clicking Connect will trigger JPAKE)
Comment 5 Matt Brubeck (:mbrubeck) 2011-08-22 15:17:05 PDT
Created attachment 554978 [details] [diff] [review]
WIP

This fixes the bug.  If you cancel Master password login, then the "Sync enabled" toggle in the preferences flips to "Off".  Flipping the toggle back on will cause Sync to try logging in again.  Restarting the browser will also re-enable Sync and cause it to try again.

The one unsolved problem is that Sync will not retry automatically after 15 minutes, like it's supposed to (and does in desktop Firefox).
Comment 6 Matt Brubeck (:mbrubeck) 2011-08-22 17:15:42 PDT
Created attachment 554994 [details] [diff] [review]
patch

This version will do the right thing when sync tries to connect again later.

I'd also like to refactor this code to make the UI more state-based like in desktop Firefox's browser-syncui.js, but that would require touching a lot more code so I'll do that in a separate bug.

We have several different bugs in the Master Password feature (bug 592772).  I'm not sure if we want to request approval to land them all on Aurora, or disable the feature instead (and wait until Firefox 9 to ship it).
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-22 18:45:36 PDT
Comment on attachment 554994 [details] [diff] [review]
patch

># HG changeset patch
># Parent 622e4cb255f1ca0cdb5ae0af152ca5072589273b
># User Matt Brubeck <mbrubeck@mozilla.com>
>
>diff --git a/mobile/chrome/content/sync.js b/mobile/chrome/content/sync.js
>--- a/mobile/chrome/content/sync.js
>+++ b/mobile/chrome/content/sync.js
>@@ -397,16 +397,21 @@ let WeaveGlue = {
>     let autosync = this._elements.autosync;
>     let details = this._elements.details;
>     let device = this._elements.device;
>     let disconnect = this._elements.disconnect;
>     let sync = this._elements.sync;
> 
>     let syncEnabled = this._elements.autosync.value;
> 
>+    // Sync may successfully log in after it was temporarily disabled by a
>+    // canceled master password entry.  If so, then re-enable it.
>+    if (loggedIn && !syncEnabled)
>+      syncEnabled = this._elements.autosync.value = true;

We seem to make a local "autosync" a few lines above. Can we use it here and above in the | let syncEnabled = ... | line


>+      if (Weave.Status.login == "service.master_password_locked") {
>+        // Disable sync temporarily. Sync will try again after a set interval,
>+        // or if the user presses the button to enable it again.
>+        this._elements.autosync.value = false;

Here too

>     // Init the setup data if we just logged in
>-    if (!this.setupData && aTopic == "weave:service:login:finish")
>+    if (!this.setupData && aTopic == "weave:service:login:finish") {
>       this.loadSetupData();
>+      if (!this._elements.autosync.value) {
>+        this._elements.autosync.value = true;

Here too

Just a question for the future: We want to remove the "Enable Sync" toggle (tied to autosync). How does that change this problem?

r+ for the current state
Comment 8 Matt Brubeck (:mbrubeck) 2011-08-22 20:52:56 PDT
Pushed to inbound with review changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/884897e976ad

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Just a question for the future: We want to remove the "Enable Sync" toggle
> (tied to autosync). How does that change this problem?

Then we'll need a slightly different UX when the master password fails.  There just needs to be some indicator that sync login failed, and a way to retry it.
Comment 9 Mounir Lamouri (:mounir) 2011-08-23 01:43:54 PDT
http://hg.mozilla.org/mozilla-central/rev/884897e976ad
Comment 10 Matt Brubeck (:mbrubeck) 2011-08-25 16:38:37 PDT
Comment on attachment 554994 [details] [diff] [review]
patch

Requesting approval to land this in Aurora for Firefox 8.  This fixes a minor but user-visible bug in a new feature in Fennec 8 (Master Password).  The change is mobile-only and fairly well localized; it adds code that should only run if the user has Master Password enabled and cancels the Master Password dialog.  However, I won't say that it's zero-risk, since it touches some moderately complex logic.

[Alternately, we could ship Firefox 8 with this minor bug, or we could disable Master Password in Firefox 8 and wait until 9 to ship it.]
Comment 11 Aaron Train [:aaronmt] 2011-08-30 06:40:26 PDT
Verified Fixed on Nightly (9.0a1)
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110830 Firefox/9.0a1 Fennec/9.0a1
Comment 12 Matt Brubeck (:mbrubeck) 2011-08-30 20:15:43 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/4668220148dc
Comment 13 juan becerra [:juanb] 2011-09-09 15:21:26 PDT
This should be verified on aurora.
Comment 14 Aaron Train [:aaronmt] 2011-09-10 10:52:01 PDT
Samsung Galaxy SII (Android 2.3.3)
Mozilla/5.0 (Android; Linux armv7l; rv:8.0a2) Gecko/20110910 Firefox/8.0a2 Fennec/8.0a2
Comment 15 juan becerra [:juanb] 2011-09-19 15:54:10 PDT
We have done all the QA we need for this bug. Done [qa!]

Note You need to log in before you can comment on or make changes to this bug.