Bug 1312154 – Stop using nsIDialogParamBlock in downloadcert.(js|xul). r=keeler
authorCykesiopka <cykesiopka.bmo@gmail.com>
Fri, 28 Oct 2016 02:13:38 +0800
changeset 320166 52df9f0848ce92e04f8a41ca2a8de66d68bc2745
parent 320165 568c403cde43c611d3befebd5a8fe1cc1cb81582
child 320167 8739d8f536d0d37f117a17492127ff17517c659c
push id33688
push userryanvm@gmail.com
push dateSun, 30 Oct 2016 02:12:08 +0000
treeherderautoland@52df9f0848ce [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1312154
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 1312154 – Stop using nsIDialogParamBlock in downloadcert.(js|xul). r=keeler nsIDialogParamBlock isn't a great API, and is best avoided. This patch also updates downloadcert.js to match modern PSM style, and adds a test. MozReview-Commit-ID: J2g2H0iBAn4
security/manager/locales/en-US/chrome/pippki/pippki.properties
security/manager/pki/nsNSSDialogs.cpp
security/manager/pki/resources/content/downloadcert.js
security/manager/pki/resources/content/downloadcert.xul
security/manager/ssl/tests/mochitest/browser/browser.ini
security/manager/ssl/tests/mochitest/browser/browser_downloadCert_ui.js
--- a/security/manager/locales/en-US/chrome/pippki/pippki.properties
+++ b/security/manager/locales/en-US/chrome/pippki/pippki.properties
@@ -4,16 +4,18 @@
 
 CertPassPrompt=Please enter the Personal Security Password for the PSM Private Keys security device.
 
 # LOCALIZATION NOTE(certWithSerial): Used for semi-uniquely representing a cert.
 # %1$S is the serial number of the cert in AA:BB:CC hex format.
 certWithSerial=Certificate with serial number: %1$S
 
 # Download Cert dialog
+# LOCALIZATION NOTE(newCAMessage1):
+# %S is a string representative of the certificate being downloaded/imported.
 newCAMessage1=Do you want to trust “%S” for the following purposes?
 unnamedCA=Certificate Authority (unnamed)
 
 # For editing cert trust
 editTrustCA=The certificate “%S” represents a Certificate Authority.
 
 # For Deleting Certificates
 deleteSslCertConfirm3=Are you sure you want to delete these server exceptions?
--- a/security/manager/pki/nsNSSDialogs.cpp
+++ b/security/manager/pki/nsNSSDialogs.cpp
@@ -86,71 +86,87 @@ nsNSSDialogs::SetPassword(nsIInterfaceRe
   rv = block->GetInt(1, &status);
   if (NS_FAILED(rv)) return rv;
 
   *_canceled = (status == 0)?true:false;
 
   return rv;
 }
 
-NS_IMETHODIMP 
-nsNSSDialogs::ConfirmDownloadCACert(nsIInterfaceRequestor *ctx, 
-                                    nsIX509Cert *cert,
-                                    uint32_t *_trust,
-                                    bool *_retval)
+NS_IMETHODIMP
+nsNSSDialogs::ConfirmDownloadCACert(nsIInterfaceRequestor* ctx,
+                                    nsIX509Cert* cert,
+                            /*out*/ uint32_t* trust,
+                            /*out*/ bool* importConfirmed)
 {
-  nsresult rv;
+  // |ctx| is allowed to be null.
+  NS_ENSURE_ARG(cert);
+  NS_ENSURE_ARG(trust);
+  NS_ENSURE_ARG(importConfirmed);
 
-  nsCOMPtr<nsIMutableArray> dlgArray = nsArrayBase::Create();
-  if (!dlgArray) {
+  nsCOMPtr<nsIMutableArray> argArray = nsArrayBase::Create();
+  if (!argArray) {
     return NS_ERROR_FAILURE;
   }
-  rv = dlgArray->AppendElement(cert, false);
+
+  nsresult rv = argArray->AppendElement(cert, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
-  nsCOMPtr<nsIDialogParamBlock> dlgParamBlock(
-    do_CreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID));
-  if (!dlgParamBlock) {
-    return NS_ERROR_FAILURE;
-  }
-  rv = dlgParamBlock->SetObjects(dlgArray);
+
+  nsCOMPtr<nsIWritablePropertyBag2> retVals = new nsHashPropertyBag();
+  rv = argArray->AppendElement(retVals, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   // Get the parent window for the dialog
   nsCOMPtr<mozIDOMWindowProxy> parent = do_GetInterface(ctx);
-  rv = nsNSSDialogHelper::openDialog(parent, 
+  rv = nsNSSDialogHelper::openDialog(parent,
                                      "chrome://pippki/content/downloadcert.xul",
-                                     dlgParamBlock);
+                                     argArray);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("importConfirmed"),
+                                  importConfirmed);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  int32_t status;
-  int32_t ssl, email, objsign;
+  *trust = nsIX509CertDB::UNTRUSTED;
+  if (!*importConfirmed) {
+    return NS_OK;
+  }
 
-  rv = dlgParamBlock->GetInt(1, &status);
-  if (NS_FAILED(rv)) return rv;
-  rv = dlgParamBlock->GetInt(2, &ssl);
-  if (NS_FAILED(rv)) return rv;
-  rv = dlgParamBlock->GetInt(3, &email);
-  if (NS_FAILED(rv)) return rv;
-  rv = dlgParamBlock->GetInt(4, &objsign);
-  if (NS_FAILED(rv)) return rv;
- 
-  *_trust = nsIX509CertDB::UNTRUSTED;
-  *_trust |= (ssl) ? nsIX509CertDB::TRUSTED_SSL : 0;
-  *_trust |= (email) ? nsIX509CertDB::TRUSTED_EMAIL : 0;
-  *_trust |= (objsign) ? nsIX509CertDB::TRUSTED_OBJSIGN : 0;
+  bool trustForSSL = false;
+  rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("trustForSSL"),
+                                  &trustForSSL);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  bool trustForEmail = false;
+  rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("trustForEmail"),
+                                  &trustForEmail);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  bool trustForObjSign = false;
+  rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("trustForObjSign"),
+                                  &trustForObjSign);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
 
-  *_retval = (status != 0);
+  *trust |= trustForSSL ? nsIX509CertDB::TRUSTED_SSL : 0;
+  *trust |= trustForEmail ? nsIX509CertDB::TRUSTED_EMAIL : 0;
+  *trust |= trustForObjSign ? nsIX509CertDB::TRUSTED_OBJSIGN : 0;
 
-  return rv;
+  return NS_OK;
 }
 
 NS_IMETHODIMP
 nsNSSDialogs::ChooseCertificate(nsIInterfaceRequestor* ctx,
                                 const nsACString& hostname,
                                 int32_t port,
                                 const nsACString& organization,
                                 const nsACString& issuerOrg,
--- a/security/manager/pki/resources/content/downloadcert.js
+++ b/security/manager/pki/resources/content/downloadcert.js
@@ -1,55 +1,92 @@
 /* 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/. */
 /* import-globals-from pippki.js */
 "use strict";
 
-const nsIDialogParamBlock = Components.interfaces.nsIDialogParamBlock;
-const nsIX509Cert = Components.interfaces.nsIX509Cert;
-
-var params;
-var caName;
-var cert;
+/**
+ * @file Implements the functionality of downloadcert.xul: a dialog that allows
+ *       a user to confirm whether to import a certificate, and if so what trust
+ *       to give it.
+ * @argument {nsISupports} window.arguments[0]
+ *           Certificate to confirm import of, queryable to nsIX509Cert.
+ * @argument {nsISupports} window.arguments[1]
+ *           Object to set the return values of calling the dialog on, queryable
+ *           to the underlying type of DownloadCertReturnValues.
+ */
 
-function onLoad()
-{
-  params = window.arguments[0].QueryInterface(nsIDialogParamBlock);
-  cert = params.objects.queryElementAt(0, nsIX509Cert);
+/**
+ * @typedef DownloadCertReturnValues
+ * @type nsIWritablePropertyBag2
+ * @property {Boolean} importConfirmed
+ *           Set to true if the user confirmed import of the cert and accepted
+ *           the dialog, false otherwise.
+ * @property {Boolean} trustForSSL
+ *           Set to true if the cert should be trusted for SSL, false otherwise.
+ *           Undefined value if |importConfirmed| is not true.
+ * @property {Boolean} trustForEmail
+ *           Set to true if the cert should be trusted for e-mail, false
+ *           otherwise. Undefined value if |importConfirmed| is not true.
+ * @property {Boolean} trustForObjSign
+ *           Set to true if the cert should be trusted for object signing, false
+ *           otherwise. Undefined value if |importConfirmed| is not true.
+ */
 
-  var bundle = document.getElementById("pippki_bundle");
+const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
+
+/**
+ * The cert to potentially import.
+ * @type nsIX509Cert
+ */
+var gCert;
 
-  caName = cert.commonName;
+/**
+ * onload() handler.
+ */
+function onLoad() {
+  gCert = window.arguments[0].QueryInterface(Ci.nsIX509Cert);
+
+  let bundle = document.getElementById("pippki_bundle");
+  let caName = gCert.commonName;
   if (caName.length == 0) {
     caName = bundle.getString("unnamedCA");
   }
 
-  var message2 = bundle.getFormattedString("newCAMessage1", [caName]);
-  setText("message2", message2);
+  setText("trustHeader", bundle.getFormattedString("newCAMessage1", [caName]));
 }
 
-function viewCert()
-{
-  viewCertHelper(window, cert);
+/**
+ * Handler for the "View Cert" button.
+ */
+function viewCert() {
+  viewCertHelper(window, gCert);
 }
 
-function doOK()
-{
+/**
+ * ondialogaccept() handler.
+ *
+ * @returns {Boolean} true to make the dialog close, false otherwise.
+ */
+function onDialogAccept() {
   let checkSSL = document.getElementById("trustSSL");
   let checkEmail = document.getElementById("trustEmail");
   let checkObjSign = document.getElementById("trustObjSign");
 
-  // Signal which trust bits the user wanted to enable.
-  params.SetInt(2, checkSSL.checked ? 1 : 0);
-  params.SetInt(3, checkEmail.checked ? 1 : 0);
-  params.SetInt(4, checkObjSign.checked ? 1 : 0);
-
-  // Signal that the user accepted.
-  params.SetInt(1, 1);
+  let retVals = window.arguments[1].QueryInterface(Ci.nsIWritablePropertyBag2);
+  retVals.setPropertyAsBool("importConfirmed", true);
+  retVals.setPropertyAsBool("trustForSSL", checkSSL.checked);
+  retVals.setPropertyAsBool("trustForEmail", checkEmail.checked);
+  retVals.setPropertyAsBool("trustForObjSign", checkObjSign.checked);
   return true;
 }
 
-function doCancel()
-{
-  params.SetInt(1, 0); // Signal that the user cancelled.
+/**
+ * ondialogcancel() handler.
+ *
+ * @returns {Boolean} true to make the dialog close, false otherwise.
+ */
+function onDialogCancel() {
+  let retVals = window.arguments[1].QueryInterface(Ci.nsIWritablePropertyBag2);
+  retVals.setPropertyAsBool("importConfirmed", false);
   return true;
 }
--- a/security/manager/pki/resources/content/downloadcert.xul
+++ b/security/manager/pki/resources/content/downloadcert.xul
@@ -2,23 +2,24 @@
 <!-- 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="download_cert" title="&downloadCert.title;"
-  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"      
-  style="width: 46em;"
-  buttons="accept,cancel"
-  ondialogaccept="return doOK();"
-  ondialogcancel="return doCancel();"
-  onload="onLoad();">
+<dialog id="download_cert"
+        title="&downloadCert.title;"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
+        style="width: 46em;"
+        buttons="accept,cancel"
+        ondialogaccept="return onDialogAccept();"
+        ondialogcancel="return onDialogCancel();"
+        onload="onLoad();">
 
 <stringbundle id="pippki_bundle" src="chrome://pippki/locale/pippki.properties"/>
 
 <script type="application/javascript" src="chrome://pippki/content/downloadcert.js"/>
 <script type="application/javascript" src="chrome://pippki/content/pippki.js"/>
 
 
   <!--  Let 'em know what they're doing -->
@@ -30,44 +31,39 @@
 
   <!--  checkboxes for trust bits
      -  "do you want to?"
      -  * trust for SSL
      -  * trust for email
      -  * trust for object signing
     -->
   <vbox>
-    <description id="message2"/>
+    <description id="trustHeader"/>
     <checkbox label="&downloadCert.trustSSL;"
               id="trustSSL"/>
     <checkbox label="&downloadCert.trustEmail;"
               id="trustEmail"/>
     <checkbox label="&downloadCert.trustObjSign;"
               id="trustObjSign"/>
   </vbox>
 
   <separator/>
 
-  <!--  buttons for viewing cert and policies
-     -  "suggested you view the following:"
-     -  <>  view cert
-     -  <>  view policy
-    -->
   <vbox>
     <description>&downloadCert.message3;</description>
     <separator/>
     <grid>
       <columns>
         <column/>
         <column/>
       </columns>
       <rows>
         <row>
           <button id="viewC-button"
                   label="&downloadCert.viewCert.label;"
-                  oncommand="viewCert();"/> 
+                  oncommand="viewCert();"/>
           <description style="margin: 4px;">&downloadCert.viewCert.text;</description>
         </row>
       </rows>
     </grid>
   </vbox>
 
 </dialog>
--- a/security/manager/ssl/tests/mochitest/browser/browser.ini
+++ b/security/manager/ssl/tests/mochitest/browser/browser.ini
@@ -5,12 +5,13 @@ support-files =
   *.pem
 
 [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.
 skip-if = os == "linux"
new file mode 100644
--- /dev/null
+++ b/security/manager/ssl/tests/mochitest/browser/browser_downloadCert_ui.js
@@ -0,0 +1,150 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/publicdomain/zero/1.0/
+"use strict";
+
+// Tests that the cert download/import UI correctly identifies the cert being
+// downloaded, and allows the trust of the cert to be specified.
+
+const { MockRegistrar } =
+  Cu.import("resource://testing-common/MockRegistrar.jsm", {});
+
+/**
+ * @typedef {TestCase}
+ * @type Object
+ * @property {String} certFilename
+ *           Filename of the cert for this test case.
+ * @property {String} expectedDisplayString
+ *           The string we expect the UI to display to represent the given cert.
+ * @property {nsIX509Cert} cert
+ *           Handle to the cert once read in setup().
+ */
+
+/**
+ * A list of test cases representing certs that get "downloaded".
+ * @type TestCase[]
+ */
+const TEST_CASES = [
+  { certFilename: "has-cn.pem",
+    expectedDisplayString: "Foo",
+    cert: null },
+  { certFilename: "has-empty-subject.pem",
+    expectedDisplayString: "Certificate Authority (unnamed)",
+    cert: null },
+];
+
+/**
+ * Opens the cert download dialog.
+ *
+ * @param {nsIX509Cert} cert
+ *        The cert to pass to the dialog for display.
+ * @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 openCertDownloadDialog(cert) {
+  let returnVals = Cc["@mozilla.org/hash-property-bag;1"]
+                     .createInstance(Ci.nsIWritablePropertyBag2);
+  let win = window.openDialog("chrome://pippki/content/downloadcert.xul", "",
+                              "", cert, returnVals);
+  return new Promise((resolve, reject) => {
+    win.addEventListener("load", function onLoad() {
+      win.removeEventListener("load", onLoad);
+      resolve([win, returnVals]);
+    });
+  });
+}
+
+// Mock implementation of nsICertificateDialogs.
+const gCertificateDialogs = {
+  expectedCert: null,
+  viewCertCallCount: 0,
+  confirmDownloadCACert(ctx, cert, trust) {
+    Assert.ok(false, "confirmDownloadCACert() should not have been called");
+  },
+  setPKCS12FilePassword(ctx, password) {
+    Assert.ok(false, "setPKCS12FilePassword() should not have been called");
+  },
+  getPKCS12FilePassword(ctx, password) {
+    Assert.ok(false, "getPKCS12FilePassword() should not have been called");
+  },
+  viewCert(ctx, cert) {
+    this.viewCertCallCount++;
+    Assert.notEqual(cert, null, "Cert to view should not be null");
+    Assert.equal(cert, this.expectedCert,
+                 "Actual and expected cert should match");
+  },
+
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsICertificateDialogs])
+};
+
+add_task(function* setup() {
+  for (let testCase of TEST_CASES) {
+    testCase.cert = yield readCertificate(testCase.certFilename, ",,");
+    Assert.notEqual(testCase.cert, null,
+                    `'${testCase.certFilename}' should have been read`);
+  }
+
+  let certificateDialogsCID =
+    MockRegistrar.register("@mozilla.org/nsCertificateDialogs;1",
+                           gCertificateDialogs);
+  registerCleanupFunction(() => {
+    MockRegistrar.unregister(certificateDialogsCID);
+  });
+});
+
+// Test that the trust header message corresponds to the provided cert, and that
+// the View Cert button launches the cert viewer for the provided cert.
+add_task(function* testTrustHeaderAndViewCertButton() {
+  for (let testCase of TEST_CASES) {
+    let [win, retVals] = yield openCertDownloadDialog(testCase.cert);
+    let expectedTrustHeaderString =
+      `Do you want to trust \u201C${testCase.expectedDisplayString}\u201D ` +
+      "for the following purposes?";
+    Assert.equal(win.document.getElementById("trustHeader").textContent,
+                 expectedTrustHeaderString,
+                 "Actual and expected trust header text should match for " +
+                 `${testCase.certFilename}`);
+
+    gCertificateDialogs.viewCertCallCount = 0;
+    gCertificateDialogs.expectedCert = testCase.cert;
+    info("Pressing View Cert button");
+    win.document.getElementById("viewC-button").doCommand();
+    Assert.equal(gCertificateDialogs.viewCertCallCount, 1,
+                 "viewCert() should've been called once");
+
+    yield BrowserTestUtils.closeWindow(win);
+  }
+});
+
+// Test that the right values are returned when the dialog is accepted.
+add_task(function* testAcceptDialogReturnValues() {
+  let [win, retVals] = yield openCertDownloadDialog(TEST_CASES[0].cert);
+  win.document.getElementById("trustSSL").checked = true;
+  win.document.getElementById("trustEmail").checked = false;
+  win.document.getElementById("trustObjSign").checked = true;
+  info("Accepting dialog");
+  win.document.getElementById("download_cert").acceptDialog();
+  yield BrowserTestUtils.windowClosed(win);
+
+  Assert.ok(retVals.get("importConfirmed"),
+            "Return value should signal user chose to import the cert");
+  Assert.ok(retVals.get("trustForSSL"),
+            "Return value should signal SSL trust checkbox was checked");
+  Assert.ok(!retVals.get("trustForEmail"),
+            "Return value should signal E-mail trust checkbox was unchecked");
+  Assert.ok(retVals.get("trustForObjSign"),
+            "Return value should signal Obj Sign trust checkbox was checked");
+});
+
+// Test that the right values are returned when the dialog is canceled.
+add_task(function* testCancelDialogReturnValues() {
+  let [win, retVals] = yield openCertDownloadDialog(TEST_CASES[0].cert);
+  info("Canceling dialog");
+  win.document.getElementById("download_cert").cancelDialog();
+  yield BrowserTestUtils.windowClosed(win);
+
+  Assert.ok(!retVals.get("importConfirmed"),
+            "Return value should signal user chose not to import the cert");
+});