Bug 1313849 – Stop using nsIDialogParamBlock in setp12password.xul. r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Sat, 05 Nov 2016 01:23:35 +0800
changeset 321808 b504bc4ede3c3ad80b5624476bf0601c97011043
parent 321807 e886c6e03475254a8eb4d40da99502744dda28f1
child 321809 1633bd3dfd6ff73c795544da9518d16383cb18d8
push id83680
push usercbook@mozilla.com
push dateWed, 09 Nov 2016 15:39:46 +0000
treeherdermozilla-inbound@310ae43d23b7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1313849
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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");
+});