Closed Bug 1388238 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Form Manager, enhancement, P1)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: MattN, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4])

Attachments

(2 files, 2 obsolete files)

Bug 1388238 - Implement waitForMasterPasswordDialog helper which handles open dialogs.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
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
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)
Blocks: 1408492
You need to log in before you can comment on or make changes to this bug.