Create MasterPassword.jsm to provide async wrappers around crypto-SDR and handle existing dialogs

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: MattN, Assigned: steveck)

Tracking

(Blocks 2 bugs)

Trunk
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [form autofill:M4])

Attachments

(2 attachments, 2 obsolete attachments)

As a starting point for bug 1271851 and to use for Form Autofill, we can start providing cleaner APIs for a master password with helpers to avoid existing issues like triggering a new MP dialog while one is already open.

For now we'll implement this in the formautofill system extension so we can iterate quickly but eventually this module can move to toolkit and be used by password manager and other consumers like sync.
Comment on attachment 8894734 [details]
Bug 1388238 - Implement waitForMasterPasswordDialog helper which handles open dialogs.

https://reviewboard.mozilla.org/r/165908/#review171004
Attachment #8894734 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8894735 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

https://reviewboard.mozilla.org/r/165910/#review171006
Attachment #8894735 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/7b7aa4e9ae5a
Implement waitForMasterPasswordDialog helper which handles open dialogs. r=MattN
https://hg.mozilla.org/integration/autoland/rev/55f44886a839
Add encrypt/decrypt methods to MasterPassword.jsm. r=MattN
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/8e2f4f651250
Implement waitForMasterPasswordDialog helper which handles open dialogs. r=MattN
https://hg.mozilla.org/integration/autoland/rev/99a525a70041
Add encrypt/decrypt methods to MasterPassword.jsm. r=MattN
Blocks: 1389413
Attachment #8896181 - Flags: review?(MattN+bmo) → review?(lchang)
Attachment #8896182 - Flags: review?(MattN+bmo) → review?(lchang)
Comment on attachment 8896182 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

I tried to remove the token reset and needInit checking in the new patch, but it's still failed in the end... It'll return different error code(-11) with message  [unknown top frame]. I still have no idea why it's always busted in the debug build only...
Flags: needinfo?(MattN+bmo)
Attachment #8896182 - Flags: review?(lchang)
Attachment #8896181 - Flags: review?(lchang)
Attachment #8896181 - Flags: review?(MattN+bmo)
Attachment #8896182 - Flags: review?(MattN+bmo)
Comment on attachment 8896182 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

I found that assertion for the prompt case would be crashed only in the debug build, so the only way to verify the prompt safely is using the mock prompt in the xpcshell test.
Comment on attachment 8896182 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

https://reviewboard.mozilla.org/r/167458/#review174198
Attachment #8896182 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8896181 [details]
Bug 1388238 - Implement waitForMasterPasswordDialog helper which handles open dialogs.

https://reviewboard.mozilla.org/r/167456/#review174202
Attachment #8896181 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/5b4b8c71b9fb
Implement waitForMasterPasswordDialog helper which handles open dialogs. r=MattN
https://hg.mozilla.org/integration/autoland/rev/02f07f0cb11d
Add encrypt/decrypt methods to MasterPassword.jsm. r=MattN
Attachment #8894734 - Attachment is obsolete: true
Attachment #8894735 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
This is getting backed out since comment 11 wasn't addressed.
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0149c32ca8b7
Backed out changeset 02f07f0cb11d because comment 11 wasn't addressed before landing.
We'll push patch in Bug 1337314 together with this one to make sure there's no unreferenced file error(because Bug 1337314 will need API in this patch).
Flags: needinfo?(schung)
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f15f3ee6cc1
Implement waitForMasterPasswordDialog helper which handles open dialogs. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1ef3d6ed6d
Add encrypt/decrypt methods to MasterPassword.jsm. r=MattN
https://hg.mozilla.org/mozilla-central/rev/9f15f3ee6cc1
https://hg.mozilla.org/mozilla-central/rev/8c1ef3d6ed6d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1391182
Steve and Matt, sadly this hunk from your patches
+        if (xulDialog) {
+            xulDialog.setAttribute("windowtype", "prompt:" + this.args.promptType);
+        }
+
breaks Thunderbird's entire Mozmill test suite. Is it really valid to change attribution on the XUL dialogue?

Looks like you want to detect the password window
  let promptWin = Services.wm.getMostRecentWindow("prompt:promptPassword");
so why not just prepend the "prompt:" when the rest is already "promptPassword"?
Flags: needinfo?(schung)
Flags: needinfo?(MattN+bmo)
It seems more like they needed a way to find the password dialog via windowtype. Normally dialogs have no windowtype, but have an id. So the dialog code generates a windowtype unconditionally. This produces prompt:promptPassword for the password dialog, but e.g. prompt:alert (and prompt:confirmEx) for normal dialogs that only had id=commonDialog.
See bug 1391182 comment 13.

(In reply to Jorg K (GMT+2) from comment #27)
> Looks like you want to detect the password window
>   let promptWin = Services.wm.getMostRecentWindow("prompt:promptPassword");

Correct.

> so why not just prepend the "prompt:" when the rest is already
> "promptPassword"?

Because it will be useful for others in the future too.
Flags: needinfo?(schung)
Flags: needinfo?(MattN+bmo)
You need to log in before you can comment on or make changes to this bug.