1313849 - Stop using nsIDialogParamBlock in setp12password.xul

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[:Cykesiopka]

nsIDialogParamBlock isn't a great API, and is best avoided.

Comment 1

[:Cykesiopka]

Attachment: Bug 1313849 - Alphabetically sort security/manager/pki/resources/jar.mn.

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

Comment 2

[:Cykesiopka]

Attachment: Bug 1313849 – Stop using nsIDialogParamBlock in setp12password.xul.

nsIDialogParamBlock isn't a great API, and is best avoided.

This patch also splits password.js into two files that implement the
functionality of changepassword.xul and setp12password.xul
respectively, and adds a test.

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

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8807650 [details]
Bug 1313849 - Alphabetically sort security/manager/pki/resources/jar.mn.

https://reviewboard.mozilla.org/r/90736/#review90982

Seems like a reasonable thing to do.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8807651 [details]
Bug 1313849 – Stop using nsIDialogParamBlock in setp12password.xul.

https://reviewboard.mozilla.org/r/90738/#review90998

LGTM.

::: security/manager/pki/resources/content/setp12password.js:75
(Diff revision 1)
> + * @param {String} password
> + *        The password to calculate the strength of.
> + * @returns {Number}
> + *          The strength of the password in the range [0, 100].
> + */
> +function getPasswordStrength(password) {

We should revisit this password strength function. For one thing, it looks like this duplicates code from changepassword.js. More fundamentally, though, it's not that great. It gives "###1AA" the maximum strength, which I think it shouldn't. We should look at the literature here and fix this in all the places we do something like this.

Comment 5

[:Cykesiopka]

Comment on attachment 8807651 [details]
Bug 1313849 – Stop using nsIDialogParamBlock in setp12password.xul.

https://reviewboard.mozilla.org/r/90738/#review90998

Thanks!

> We should revisit this password strength function. For one thing, it looks like this duplicates code from changepassword.js. More fundamentally, though, it's not that great. It gives "###1AA" the maximum strength, which I think it shouldn't. We should look at the literature here and fix this in all the places we do something like this.

Right - I plan on deduping this code when I deal with changepassword.js.
I didn't bother originally because I wanted to avoid changing changepassword.js too much without test coverage.

I'll file a follow-up for making the function better later.

Comment 6

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d524f215d857e7f965acc8539afe6765a2b534c6
The orange looks like an intermittent, or something from my choice of base commit.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e886c6e03475
Alphabetically sort security/manager/pki/resources/jar.mn. r=keeler
https://hg.mozilla.org/integration/autoland/rev/b504bc4ede3c
– Stop using nsIDialogParamBlock in setp12password.xul. r=keeler

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e886c6e03475
https://hg.mozilla.org/mozilla-central/rev/b504bc4ede3c