Closed Bug 624552 Opened 13 years ago Closed 13 years ago

In Fennec w/ locked Master Password, a manual Sync "Connect" goes to JPAKE instead of Master Password dialog

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox8+ fixed)

VERIFIED FIXED
Firefox 8
Tracking Status
firefox8 + fixed

People

(Reporter: dholbert, Assigned: mbrubeck)

References

()

Details

(Keywords: regression, verified-aurora, Whiteboard: [qa!])

Attachments

(1 file, 1 obsolete file)

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.
(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.
ping: matt?
I can look into this, but it's not high on my priority list right now.
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)
Component: Extension Compatibility → General
QA Contact: extension-compatibility → general
tracking-fennec: --- → ?
Keywords: regression
OS: Linux → All
Hardware: ARM → All
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Attached patch WIP (obsolete) — Splinter Review
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).
Attached patch patchSplinter Review
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).
Attachment #554978 - Attachment is obsolete: true
Attachment #554994 - Flags: review?(mark.finkle)
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
Attachment #554994 - Flags: review?(mark.finkle) → review+
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.
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/884897e976ad
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
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.]
Attachment #554994 - Flags: approval-mozilla-aurora?
Verified Fixed on Nightly (9.0a1)
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110830 Firefox/9.0a1 Fennec/9.0a1
Status: RESOLVED → VERIFIED
Attachment #554994 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This should be verified on aurora.
Whiteboard: [qa+]
Samsung Galaxy SII (Android 2.3.3)
Mozilla/5.0 (Android; Linux armv7l; rv:8.0a2) Gecko/20110910 Firefox/8.0a2 Fennec/8.0a2
Keywords: verified-aurora
Whiteboard: [qa+]
We have done all the QA we need for this bug. Done [qa!]
Whiteboard: [qa!]
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.