Closed Bug 1313849 Opened 3 years ago Closed 3 years ago

Stop using nsIDialogParamBlock in setp12password.xul

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files)

nsIDialogParamBlock isn't a great API, and is best avoided.
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.
Attachment #8807650 - Flags: review?(dkeeler) → review+
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.
Attachment #8807651 - Flags: review?(dkeeler) → review+
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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d524f215d857e7f965acc8539afe6765a2b534c6
The orange looks like an intermittent, or something from my choice of base commit.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e886c6e03475
https://hg.mozilla.org/mozilla-central/rev/b504bc4ede3c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.