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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P1)

Description

[Matthew N. [:MattN]]

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 1

[Matthew N. [:MattN]]

Attachment: Bug 1388238 - Implement waitForMasterPasswordDialog helper which handles open dialogs.

Review commit: https://reviewboard.mozilla.org/r/165908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165908/

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

Review commit: https://reviewboard.mozilla.org/r/165910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165910/

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8894734 [details]
Bug 1388238 - Implement waitForMasterPasswordDialog helper which handles open dialogs.

https://reviewboard.mozilla.org/r/165908/#review171004

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8894735 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

https://reviewboard.mozilla.org/r/165910/#review171006

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/b676d5b34892 for eslint failures, https://treeherder.mozilla.org/logviewer.html#?job_id=121866954&repo=autoland

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8894734 [details]
Bug 1388238 - Implement waitForMasterPasswordDialog helper which handles open dialogs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165908/diff/1-2/

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8894735 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165910/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/c93fa2271ee7 for xpcshell crashes, https://treeherder.mozilla.org/logviewer.html#?job_id=121879918&repo=autoland

Comment 11

[Phil Ringnalda (:philor)]

Also, and unreferenced file error, https://treeherder.mozilla.org/logviewer.html#?job_id=121889675&repo=autoland

Comment 12

[Steve Chung [:steveck]]

Attachment: Bug 1388238 - Implement waitForMasterPasswordDialog helper which handles open dialogs.

Review commit: https://reviewboard.mozilla.org/r/167456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167456/

Comment 13

[Steve Chung [:steveck]]

Attachment: Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

Review commit: https://reviewboard.mozilla.org/r/167458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/167458/

Comment 14

[Steve Chung [:steveck]]

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...

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8896182 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167458/diff/1-2/

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8896182 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167458/diff/2-3/

Comment 17

[Steve Chung [:steveck]]

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 18

[Matthew N. [:MattN]]

Comment on attachment 8896182 [details]
Bug 1388238 - Add encrypt/decrypt methods to MasterPassword.jsm.

https://reviewboard.mozilla.org/r/167458/#review174198

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8896181 [details]
Bug 1388238 - Implement waitForMasterPasswordDialog helper which handles open dialogs.

https://reviewboard.mozilla.org/r/167456/#review174202

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Matthew N. [:MattN]]

This is getting backed out since comment 11 wasn't addressed.

Comment 22

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0149c32ca8b7
Backed out changeset 02f07f0cb11d because comment 11 wasn't addressed before landing.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out part 1 as well: https://hg.mozilla.org/integration/autoland/rev/08b6dcd7d8f776c09390497b648a66fd15767aef

Comment 24

[Steve Chung [:steveck]]

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).

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f15f3ee6cc1
https://hg.mozilla.org/mozilla-central/rev/8c1ef3d6ed6d

Comment 27

[Jorg K (CEST = GMT+2)]

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"?

Comment 28

[:aceman]

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.

Comment 29

[Matthew N. [:MattN]]

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.

Comment 30

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/966d4487c72e
https://hg.mozilla.org/releases/mozilla-beta/rev/d0ea76640bfd