Bug 1312154 – Stop using nsIDialogParamBlock in downloadcert.(js|xul). r?keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Fri, 28 Oct 2016 02:13:38 +0800
changeset 430390 9352163ff95e3d57cd3d1ddb511557e8ce08af05
parent 430353 2d0a7bf026ed4aadcb17f9e1f9de0f1488b17cee
child 535197 5694a7cb28143763088e2abda69ee2fc765a63f7
push id33817
push usercykesiopka.bmo@gmail.com
push dateThu, 27 Oct 2016 18:14:15 +0000
reviewerskeeler
bugs1312154
milestone52.0a1
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");
+});