Bug 1313849 – Stop using nsIDialogParamBlock in setp12password.xul. r?keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Sat, 05 Nov 2016 01:23:35 +0800
changeset 433997 7da8996a9a328c888c7919298f1ff25d069cfe6b
parent 433996 a163bb8144c98d81a382e28ad44e4c1b08140233
child 536002 b61d9c8d740f2ea23b2302c698fc340afec6b37b
push id34701
push usercykesiopka.bmo@gmail.com
push dateFri, 04 Nov 2016 17:27:20 +0000
reviewerskeeler
bugs1313849
milestone52.0a1
Bug 1313849 – Stop using nsIDialogParamBlock in setp12password.xul. r?keeler 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. MozReview-Commit-ID: A1GlnIFl8h
security/manager/pki/nsNSSDialogs.cpp
security/manager/pki/resources/content/changepassword.js
security/manager/pki/resources/content/changepassword.xul
security/manager/pki/resources/content/password.js
security/manager/pki/resources/content/setp12password.js
security/manager/pki/resources/content/setp12password.xul
security/manager/pki/resources/jar.mn
security/manager/ssl/tests/mochitest/browser/browser.ini
security/manager/ssl/tests/mochitest/browser/browser_exportP12_passwordUI.js
--- a/security/manager/pki/nsNSSDialogs.cpp
+++ b/security/manager/pki/nsNSSDialogs.cpp
@@ -265,48 +265,46 @@ nsNSSDialogs::ChooseCertificate(nsIInter
     if (NS_FAILED(rv)) {
       return rv;
     }
   }
 
   return NS_OK;
 }
 
-NS_IMETHODIMP 
-nsNSSDialogs::SetPKCS12FilePassword(nsIInterfaceRequestor *ctx, 
-                                    nsAString &_password,
-                                    bool *_retval)
+NS_IMETHODIMP
+nsNSSDialogs::SetPKCS12FilePassword(nsIInterfaceRequestor* ctx,
+                            /*out*/ nsAString& password,
+                            /*out*/ bool* confirmedPassword)
 {
-  nsresult rv;
-  *_retval = true;
+  // |ctx| is allowed to be null.
+  NS_ENSURE_ARG(confirmedPassword);
+
   // Get the parent window for the dialog
   nsCOMPtr<mozIDOMWindowProxy> parent = do_GetInterface(ctx);
-  nsCOMPtr<nsIDialogParamBlock> block =
-           do_CreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID);
-  if (!block) return NS_ERROR_FAILURE;
-  // open up the window
-  rv = nsNSSDialogHelper::openDialog(parent,
+  nsCOMPtr<nsIWritablePropertyBag2> retVals = new nsHashPropertyBag();
+  nsresult rv =
+    nsNSSDialogHelper::openDialog(parent,
                                   "chrome://pippki/content/setp12password.xul",
-                                  block);
-  if (NS_FAILED(rv)) return rv;
-  // see if user canceled
-  int32_t status;
-  rv = block->GetInt(1, &status);
-  if (NS_FAILED(rv)) return rv;
-  *_retval = (status == 0) ? false : true;
-  if (*_retval) {
-    // retrieve the password
-    char16_t *pw;
-    rv = block->GetString(2, &pw);
-    if (NS_SUCCEEDED(rv)) {
-      _password = pw;
-      free(pw);
-    }
+                                  retVals);
+  if (NS_FAILED(rv)) {
+    return rv;
   }
-  return rv;
+
+  rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("confirmedPassword"),
+                                  confirmedPassword);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  if (!*confirmedPassword) {
+    return NS_OK;
+  }
+
+  return retVals->GetPropertyAsAString(NS_LITERAL_STRING("password"), password);
 }
 
 NS_IMETHODIMP
 nsNSSDialogs::GetPKCS12FilePassword(nsIInterfaceRequestor* ctx,
                                     nsAString& _password,
                                     bool* _retval)
 {
   *_retval = false;
rename from security/manager/pki/resources/content/password.js
rename to security/manager/pki/resources/content/changepassword.js
--- a/security/manager/pki/resources/content/password.js
+++ b/security/manager/pki/resources/content/changepassword.js
@@ -119,25 +119,16 @@ function process()
   if (params) {
     // Return value 0 means "canceled"
     params.SetInt(1, 0);
   }
 
   checkPasswords();
 }
 
-function onP12Load(disableOkButton)
-{
-  document.documentElement.getButton("accept").disabled = disableOkButton;
-  pw1 = document.getElementById("pw1");
-  params = window.arguments[0].QueryInterface(nsIDialogParamBlock);
-  // Select first password field
-  document.getElementById('pw1').focus();
-}
-
 function setPassword()
 {
   var pk11db = Components.classes[nsPK11TokenDB].getService(nsIPK11TokenDB);
   var token = pk11db.findTokenByName(tokenName);
 
   var oldpwbox = document.getElementById("oldpw");
   var initpw = oldpwbox.getAttribute("inited");
   var bundle = document.getElementById("pippki_bundle");
@@ -201,26 +192,16 @@ function setPassword()
     // Return value 1 means "successfully executed ok"
     params.SetInt(1, 1);
   }
 
   // Terminate dialog
   return success;
 }
 
-function setP12Password()
-{
-  // grab what was entered
-  params.SetString(2, pw1.value);
-  // Return value
-  params.SetInt(1, 1);
-  // Terminate dialog
-  return true;
-}
-
 function setPasswordStrength()
 {
   // We weigh the quality of the password by checking the number of:
   //  - Characters
   //  - Numbers
   //  - Non-alphanumeric chars
   //  - Upper and lower case characters
 
--- a/security/manager/pki/resources/content/changepassword.xul
+++ b/security/manager/pki/resources/content/changepassword.xul
@@ -10,17 +10,18 @@
 <dialog id="set_password" title="&setPassword.title;"
   xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"      
   buttons="accept,cancel"
   ondialogaccept="return setPassword();"
   onload="onLoad();">
 
 <stringbundle id="pippki_bundle" src="chrome://pippki/locale/pippki.properties"/>
 
-<script type="application/javascript" src="chrome://pippki/content/password.js"/>
+<script type="application/javascript"
+        src="chrome://pippki/content/changepassword.js"/>
 
 <hbox align="center">
   <label value="&setPassword.tokenName.label;: "/>
   <label id="tokenName" />
   <menulist id="tokenMenu" oncommand="onMenuChange()">
      <menupopup/>
   </menulist>
 </hbox>
new file mode 100644
--- /dev/null
+++ b/security/manager/pki/resources/content/setp12password.js
@@ -0,0 +1,128 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+/**
+ * @file Implements the functionality of setp12password.xul: a dialog that lets
+ *       the user confirm the password to set on a PKCS #12 file.
+ * @argument {nsISupports} window.arguments[0]
+ *           Object to set the return values of calling the dialog on, queryable
+ *           to the underlying type of SetP12PasswordReturnValues.
+ */
+
+/**
+ * @typedef SetP12PasswordReturnValues
+ * @type nsIWritablePropertyBag2
+ * @property {Boolean} confirmedPassword
+ *           Set to true if the user entered two matching passwords and
+ *           confirmed the dialog.
+ * @property {String} password
+ *           The password the user entered. Undefined value if
+ *           |confirmedPassword| is not true.
+ */
+
+const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
+
+/**
+ * onload() handler.
+ */
+function onLoad() {
+  // Ensure the first password textbox has focus.
+  document.getElementById("pw1").focus();
+}
+
+/**
+ * ondialogaccept() handler.
+ *
+ * @returns {Boolean} true to make the dialog close, false otherwise.
+ */
+function onDialogAccept() {
+  let password = document.getElementById("pw1").value;
+
+  let retVals = window.arguments[0].QueryInterface(Ci.nsIWritablePropertyBag2);
+  retVals.setPropertyAsBool("confirmedPassword", true);
+  retVals.setPropertyAsAString("password", password);
+  return true;
+}
+
+/**
+ * ondialogcancel() handler.
+ *
+ * @returns {Boolean} true to make the dialog close, false otherwise.
+ */
+function onDialogCancel() {
+  let retVals = window.arguments[0].QueryInterface(Ci.nsIWritablePropertyBag2);
+  retVals.setPropertyAsBool("confirmedPassword", false);
+  return true;
+}
+
+/**
+ * Calculates the strength of the given password, suitable for use in updating
+ * a progress bar that represents said strength.
+ *
+ * The strength of the password is calculated by checking the number of:
+ *   - Characters
+ *   - Numbers
+ *   - Non-alphanumeric chars
+ *   - Upper case characters
+ *
+ * @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) {
+  let lengthStrength = password.length;
+  if (lengthStrength > 5) {
+    lengthStrength = 5;
+  }
+
+  let nonNumericChars = password.replace(/[0-9]/g, "");
+  let numericStrength = password.length - nonNumericChars.length;
+  if (numericStrength > 3) {
+    numericStrength = 3;
+  }
+
+  let nonSymbolChars = password.replace(/\W/g, "");
+  let symbolStrength = password.length - nonSymbolChars.length;
+  if (symbolStrength > 3) {
+    symbolStrength = 3;
+  }
+
+  let nonUpperAlphaChars = password.replace(/[A-Z]/g, "");
+  let upperAlphaStrength = password.length - nonUpperAlphaChars.length;
+  if (upperAlphaStrength > 3) {
+    upperAlphaStrength = 3;
+  }
+
+  let strength = (lengthStrength * 10) - 20 + (numericStrength * 10) +
+                 (symbolStrength * 15) + (upperAlphaStrength * 10);
+  if (strength < 0) {
+    strength = 0;
+  }
+  if (strength > 100) {
+    strength = 100;
+  }
+
+  return strength;
+}
+
+/**
+ * oninput() handler for both password textboxes.
+ *
+ * @param {Boolean} recalculatePasswordStrength
+ *                  Whether to recalculate the strength of the first password.
+ */
+function onPasswordInput(recalculatePasswordStrength) {
+  let pw1 = document.getElementById("pw1").value;
+
+  if (recalculatePasswordStrength) {
+    document.getElementById("pwmeter").value = getPasswordStrength(pw1);
+  }
+
+  // Disable the accept button if the two passwords don't match, and enable it
+  // if the passwords do match.
+  let pw2 = document.getElementById("pw2").value;
+  document.documentElement.getButton("accept").disabled = (pw1 != pw2);
+}
--- a/security/manager/pki/resources/content/setp12password.xul
+++ b/security/manager/pki/resources/content/setp12password.xul
@@ -2,50 +2,50 @@
 <!-- This Source Code Form is subject to the terms of the Mozilla Public
    - License, v. 2.0. If a copy of the MPL was not distributed with this
    - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
 
 <?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
 
 <!DOCTYPE dialog SYSTEM "chrome://pippki/locale/pippki.dtd">
 
-<dialog id="setp12password" title="&pkcs12.setpassword.title;"
-  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"      
-  style="width: 48em;"
-  buttons="accept,cancel"
-  ondialogaccept="return setP12Password();"
-  onload="onP12Load(true);">
+<dialog id="setp12password"
+        title="&pkcs12.setpassword.title;"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+        style="width: 48em;"
+        buttons="accept,cancel"
+        ondialogaccept="return onDialogAccept();"
+        ondialogcancel="return onDialogCancel();"
+        onload="onLoad();">
 
-  <script type="application/javascript" src="chrome://pippki/content/password.js"/>
+  <script type="application/javascript"
+          src="chrome://pippki/content/setp12password.js"/>
 
   <description>&pkcs12.setpassword.message;</description>
   <separator />
   <grid>
     <columns> <column/> <column/> </columns>
     <rows>
       <row>
-        <label value="&pkcs12.setpassword.label1;"/> 
-        <textbox id="pw1" type="password" 
-                 oninput="setPasswordStrength(); checkPasswords();"/> 
+        <label value="&pkcs12.setpassword.label1;"/>
+        <textbox id="pw1" type="password" oninput="onPasswordInput(true);"/>
       </row>
       <row>
-        <label value="&pkcs12.setpassword.label2;"/> 
-        <textbox id="pw2" type="password" 
-                 oninput="checkPasswords();"/>  
+        <label value="&pkcs12.setpassword.label2;"/>
+        <textbox id="pw2" type="password" oninput="onPasswordInput(false);"/>
       </row>
     </rows>
   </grid>
   <separator/>
   <description>&pkcs12.setpassword.reminder;</description>
   <separator/>
   <label value="&setPassword.meter.label;"/>
   <grid style="margin: 4px;">
     <rows> <row/> </rows>
     <columns>
       <column style="margin: 5px;">
-        <progressmeter flex="1" id="pwmeter" mode="determined" value="0%"
+        <progressmeter flex="1" id="pwmeter" mode="determined" value="0"
                        orient="horizontal"
-                       style="width: 20em; foreground-color: red"/> 
+                       style="width: 20em; foreground-color: red"/>
       </column>
     </columns>
   </grid>
-  
 </dialog>
--- a/security/manager/pki/resources/jar.mn
+++ b/security/manager/pki/resources/jar.mn
@@ -9,16 +9,17 @@ pippki.jar:
     content/pippki/OrphanOverlay.xul         (content/OrphanOverlay.xul)
     content/pippki/OthersOverlay.xul         (content/OthersOverlay.xul)
     content/pippki/WebSitesOverlay.xul       (content/WebSitesOverlay.xul)
     content/pippki/certDump.xul              (content/certDump.xul)
     content/pippki/certManager.js            (content/certManager.js)
     content/pippki/certManager.xul           (content/certManager.xul)
     content/pippki/certViewer.js             (content/certViewer.js)
     content/pippki/certViewer.xul            (content/certViewer.xul)
+    content/pippki/changepassword.js         (content/changepassword.js)
     content/pippki/changepassword.xul        (content/changepassword.xul)
     content/pippki/choosetoken.js            (content/choosetoken.js)
     content/pippki/choosetoken.xul           (content/choosetoken.xul)
     content/pippki/clientauthask.js          (content/clientauthask.js)
     content/pippki/clientauthask.xul         (content/clientauthask.xul)
     content/pippki/createCertInfo.js         (content/createCertInfo.js)
     content/pippki/createCertInfo.xul        (content/createCertInfo.xul)
     content/pippki/deletecert.js             (content/deletecert.js)
@@ -27,16 +28,16 @@ pippki.jar:
     content/pippki/device_manager.xul        (content/device_manager.xul)
     content/pippki/downloadcert.js           (content/downloadcert.js)
     content/pippki/downloadcert.xul          (content/downloadcert.xul)
     content/pippki/editcacert.js             (content/editcacert.js)
     content/pippki/editcacert.xul            (content/editcacert.xul)
     content/pippki/exceptionDialog.js        (content/exceptionDialog.js)
 *   content/pippki/exceptionDialog.xul       (content/exceptionDialog.xul)
     content/pippki/load_device.xul           (content/load_device.xul)
-    content/pippki/password.js               (content/password.js)
     content/pippki/pippki.js                 (content/pippki.js)
     content/pippki/protectedAuth.js          (content/protectedAuth.js)
     content/pippki/protectedAuth.xul         (content/protectedAuth.xul)
     content/pippki/resetpassword.js          (content/resetpassword.js)
     content/pippki/resetpassword.xul         (content/resetpassword.xul)
+    content/pippki/setp12password.js         (content/setp12password.js)
     content/pippki/setp12password.xul        (content/setp12password.xul)
     content/pippki/viewCertDetails.xul       (content/viewCertDetails.xul)
--- a/security/manager/ssl/tests/mochitest/browser/browser.ini
+++ b/security/manager/ssl/tests/mochitest/browser/browser.ini
@@ -7,11 +7,12 @@ support-files =
 [browser_bug627234_perwindowpb.js]
 [browser_certificateManagerLeak.js]
 [browser_certViewer.js]
 [browser_clientAuth_connection.js]
 [browser_clientAuth_ui.js]
 [browser_deleteCert_ui.js]
 [browser_downloadCert_ui.js]
 [browser_editCACertTrust.js]
-# An earlier attempting at landing this test resulted in frequent intermittent
-# failures, almost entirely on Linux.
+# An earlier attempt at landing this test resulted in frequent intermittent
+# failures, almost entirely on Linux. See Bug 1309519.
 skip-if = os == "linux"
+[browser_exportP12_passwordUI.js]
new file mode 100644
--- /dev/null
+++ b/security/manager/ssl/tests/mochitest/browser/browser_exportP12_passwordUI.js
@@ -0,0 +1,142 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/publicdomain/zero/1.0/
+"use strict";
+
+// Tests that the UI for setting the password on a to be exported PKCS #12 file:
+//   1. Correctly requires the password to be typed in twice as confirmation.
+//   2. Calculates and displays the strength of said password.
+
+/**
+ * @typedef {TestCase}
+ * @type Object
+ * @property {String} name
+ *           The name of the test case for display purposes.
+ * @property {String} password1
+ *           The password to enter into the first password textbox.
+ * @property {String} password2
+ *           The password to enter into the second password textbox.
+ * @property {String} strength
+ *           The expected strength of the password in the range [0, 100].
+ */
+
+/**
+ * A list of test cases representing various inputs to the password textboxes.
+ * @type TestCase[]
+ */
+const TEST_CASES = [
+  { name: "empty",
+    password1: "",
+    password2: "",
+    strength: "0" },
+  { name: "match-weak",
+    password1: "foo",
+    password2: "foo",
+    strength: "10" },
+  { name: "match-medium",
+    password1: "foo123",
+    password2: "foo123",
+    strength: "60" },
+  { name: "match-strong",
+    password1: "fooBARBAZ 1234567890`~!@#$%^&*()-_=+{[}]|\\:;'\",<.>/?一二三",
+    password2: "fooBARBAZ 1234567890`~!@#$%^&*()-_=+{[}]|\\:;'\",<.>/?一二三",
+    strength: "100" },
+  { name: "mismatch-weak",
+    password1: "foo",
+    password2: "bar",
+    strength: "10" },
+  { name: "mismatch-medium",
+    password1: "foo123",
+    password2: "bar",
+    strength: "60" },
+  { name: "mismatch-strong",
+    password1: "fooBARBAZ 1234567890`~!@#$%^&*()-_=+{[}]|\\:;'\",<.>/?一二三",
+    password2: "bar",
+    strength: "100" },
+];
+
+/**
+ * Opens the dialog shown to set the password on a PKCS #12 file being exported.
+ *
+ * @returns {Promise}
+ *          A promise that resolves when the dialog has finished loading, with
+ *          an array consisting of:
+ *            1. The window of the opened dialog.
+ *            2. The return value nsIWritablePropertyBag2 passed to the dialog.
+ */
+function openSetP12PasswordDialog() {
+  let returnVals = Cc["@mozilla.org/hash-property-bag;1"]
+                     .createInstance(Ci.nsIWritablePropertyBag2);
+  let win = window.openDialog("chrome://pippki/content/setp12password.xul", "",
+                              "", returnVals);
+  return new Promise((resolve, reject) => {
+    win.addEventListener("load", function onLoad() {
+      win.removeEventListener("load", onLoad);
+      resolve([win, returnVals]);
+    });
+  });
+}
+
+// Tests that the first password textbox is the element that is initially
+// focused.
+add_task(function* testFocus() {
+  let [win, retVals] = yield openSetP12PasswordDialog();
+  Assert.equal(win.document.activeElement,
+               win.document.getElementById("pw1").inputField,
+               "First password textbox should have focus");
+  yield BrowserTestUtils.closeWindow(win);
+});
+
+// Tests that the password strength algorithm used is reasonable, and that the
+// Accept button is only enabled if the two passwords match.
+add_task(function* testPasswordStrengthAndEquality() {
+  let [win, retVals] = yield openSetP12PasswordDialog();
+  let password1Textbox = win.document.getElementById("pw1");
+  let password2Textbox = win.document.getElementById("pw2");
+  let strengthProgressBar = win.document.getElementById("pwmeter");
+
+  for (let testCase of TEST_CASES) {
+    password1Textbox.value = testCase.password1;
+    password2Textbox.value = testCase.password2;
+    // Setting the value of the password textboxes via |.value| apparently
+    // doesn't cause the oninput handlers to be called, so we do it here.
+    password1Textbox.oninput();
+    password2Textbox.oninput();
+
+    Assert.equal(win.document.documentElement.getButton("accept").disabled,
+                 password1Textbox.value != password2Textbox.value,
+                 "Actual and expected accept button disable state should " +
+                 `match for ${testCase.name}`);
+    Assert.equal(strengthProgressBar.value, testCase.strength,
+                 "Actual and expected strength value should match for" +
+                 `${testCase.name}`);
+  }
+
+  yield BrowserTestUtils.closeWindow(win);
+});
+
+// Test that the right values are returned when the dialog is accepted.
+add_task(function* testAcceptDialogReturnValues() {
+  let [win, retVals] = yield openSetP12PasswordDialog();
+  const password = "fooBAR 1234567890`~!@#$%^&*()-_=+{[}]|\\:;'\",<.>/?一二三";
+  win.document.getElementById("pw1").value = password;
+  win.document.getElementById("pw2").value = password;
+  info("Accepting dialog");
+  win.document.getElementById("setp12password").acceptDialog();
+  yield BrowserTestUtils.windowClosed(win);
+
+  Assert.ok(retVals.get("confirmedPassword"),
+            "Return value should signal user confirmed a password");
+  Assert.equal(retVals.get("password"), password,
+               "Actual and expected password should match");
+});
+
+// Test that the right values are returned when the dialog is canceled.
+add_task(function* testCancelDialogReturnValues() {
+  let [win, retVals] = yield openSetP12PasswordDialog();
+  info("Canceling dialog");
+  win.document.getElementById("setp12password").cancelDialog();
+  yield BrowserTestUtils.windowClosed(win);
+
+  Assert.ok(!retVals.get("confirmedPassword"),
+            "Return value should signal user didn't confirm a password");
+});