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)
Firefox for Android Graveyard
General
Tracking
(firefox8+ fixed)
VERIFIED
FIXED
Firefox 8
People
(Reporter: dholbert, Assigned: mbrubeck)
References
()
Details
(Keywords: regression, verified-aurora, Whiteboard: [qa!])
Attachments
(1 file, 1 obsolete file)
2.78 KB,
patch
|
mfinkle
:
review+
christian
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
(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•13 years ago
|
||
ping: matt?
Assignee | ||
Comment 3•13 years ago
|
||
I can look into this, but it's not high on my priority list right now.
Reporter | ||
Comment 4•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
tracking-fennec: --- → ?
status-firefox8:
--- → affected
Keywords: regression
OS: Linux → All
Hardware: ARM → All
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•13 years ago
|
||
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).
Assignee | ||
Comment 6•13 years ago
|
||
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 7•13 years ago
|
||
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+
Assignee | ||
Comment 8•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
Comment 9•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/884897e976ad
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
Assignee | ||
Comment 10•13 years ago
|
||
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?
Comment 11•13 years ago
|
||
Verified Fixed on Nightly (9.0a1) Mozilla/5.0 (Android; Linux armv7l; rv:9.0a1) Gecko/20110830 Firefox/9.0a1 Fennec/9.0a1
Assignee | ||
Updated•13 years ago
|
Status: RESOLVED → VERIFIED
Attachment #554994 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox8:
--- → +
Assignee | ||
Comment 12•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4668220148dc
Target Milestone: Firefox 9 → Firefox 8
Comment 14•13 years ago
|
||
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+]
Updated•10 years ago
|
tracking-fennec: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•