Closed Bug 1382937 Opened 2 years ago Closed 2 years ago
Don't show a master-password prompt if one is already open
59 bytes, text/x-review-board-request
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.
Priority: P3 → P2
See Also: → 1405917
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?
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.
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 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 email@example.com: https://hg.mozilla.org/integration/autoland/rev/b4a5450f62bc Rewrite Sync's master password functions to use the `nsILoginManagerCrypto` wrappers. r=MattN
You need to log in before you can comment on or make changes to this bug.