Closed
Bug 1382937
Opened 7 years ago
Closed 7 years ago
Don't show a master-password prompt if one is already open
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 58
People
(Reporter: markh, Assigned: lina)
References
Details
Attachments
(1 file)
Sync unconditionally attempts to unlock the masterpassword via utilities at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/services/sync/modules/util.js#475-508. We don't account for the possibility that another unlock dialog might be already open.
MattN tells me in IRC that Services.logins.uiBusy and/or the logic at https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/toolkit/components/passwordmgr/LoginManagerParent.jsm#198-222 can probably be reused to make this a better experience. He also suggests it might make sense to have a MasterPassword.jsm to put this logic in one place.
This is likely to become more important as formautofill encourages more people to use a master password.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Priority: P3 → P2
Assignee | ||
Comment 2•7 years ago
|
||
So, I think there are several things we can do here:
* Remove `mpEnabled()`. It's only used to report the `WEAVE_CONFIGURED_MASTER_PASSWORD` histogram, which I don't think we look at, and which `MASTER_PASSWORD_ENABLED` already covers.
* Change `mpLocked()` to use `nsILoginManagerCrypto.isLoggedIn` (via `@mozilla.org/login-manager/crypto/SDR;1`). It's exactly the same as what `mpLocked` does, just inverted.
* Change `ensureMPUnlocked` to use `nsILoginManagerCrypto.encrypt()`. `encrypt` will update `uiBusy` and fire the correct observer notifications. Our implementation currently goes behind the login manager, and uses SDR directly.
* In `ensureMPUnlocked`, check if `Services.logins.uiBusy`, and return false if so. IIUC, `nsISDR.encryptString` indirectly spins the event loop when it shows the prompt, so we still need to check `uiBusy` even though it looks like it's set synchronously in https://searchfox.org/mozilla-central/rev/bc6dddb88b1f34b54e22efc205846975fb4c04cb/toolkit/components/passwordmgr/crypto-SDR.js#116,139.
* Once `MasterPassword.jsm` is available in Toolkit, use `MasterPassword.prompt` instead of `ensureMPUnlocked`.
Matt, does this sound like a good plan to you?
Flags: needinfo?(MattN+bmo)
Comment 3•7 years ago
|
||
Just from reading what you wrote and not looking at the code it seems good…
I do think it's fine to move MasterPassword.jsm to toolkit now if you plan to do it within the 58 cycle since CC autofill isn't likely to be ready for 58. When it was originally landed in the extension it was because we thought we may need to make last-minute changes and wanted to be able to do that in our system add-on. I'd also welcome API feedback on it since I'd like it to be able to be the future API when/if we move away from using SDR.
Flags: needinfo?(MattN+bmo)
Assignee | ||
Comment 4•7 years ago
|
||
Sounds good, thanks. I'll post a patch now that replaces our raw SDR usage with cryptoSDR, and maybe request uplift to 57 if we think this is going to be a significant enough issue. We can convert Sync to use MasterPassword.jsm in a follow-up, and tweak the API as needed.
Assignee: nobody → kit
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8921151 [details]
Bug 1382937 - Rewrite Sync's master password functions to use the `nsILoginManagerCrypto` wrappers.
https://reviewboard.mozilla.org/r/192136/#review203536
Thanks. A test would have been good but I won't block on it.
Attachment #8921151 -
Flags: review?(MattN+bmo) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4a5450f62bc
Rewrite Sync's master password functions to use the `nsILoginManagerCrypto` wrappers. r=MattN
Comment 8•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in
before you can comment on or make changes to this bug.
Description
•