Bug 1312152 - Stop using nsIDialogParamBlock in the client auth UI. r?keeler draft
authorCykesiopka <cykesiopka.bmo@gmail.com>
Sun, 23 Oct 2016 12:57:41 +0800
changeset 428354 ee09f4fefd0b157eb10c4de871000101334f8cca
parent 428353 795b9e63823c3750fd9c1a573cdbb0961c994bad
child 534718 923c9c8b6153408940524ffbf370214ac1dd3343
push id33292
push usercykesiopka.bmo@gmail.com
push dateSun, 23 Oct 2016 05:07:56 +0000
reviewerskeeler
bugs1312152
milestone52.0a1
Bug 1312152 - Stop using nsIDialogParamBlock in the client auth UI. r?keeler nsIDialogParamBlock isn't a great API, and is best avoided. MozReview-Commit-ID: 2B0HkKNJizo
security/manager/pki/nsNSSDialogs.cpp
security/manager/pki/resources/content/clientauthask.js
security/manager/ssl/tests/mochitest/browser/browser_clientAuth_ui.js
--- a/security/manager/pki/nsNSSDialogs.cpp
+++ b/security/manager/pki/nsNSSDialogs.cpp
@@ -6,31 +6,31 @@
 
 /*
  * Dialog services for PIP.
  */
 
 #include "nsNSSDialogs.h"
 
 #include "mozIDOMWindow.h"
-#include "mozilla/Assertions.h"
-#include "mozilla/Casting.h"
 #include "nsArray.h"
 #include "nsEmbedCID.h"
+#include "nsHashPropertyBag.h"
 #include "nsIDialogParamBlock.h"
 #include "nsIInterfaceRequestor.h"
 #include "nsIInterfaceRequestorUtils.h"
 #include "nsIKeygenThread.h"
 #include "nsIPromptService.h"
 #include "nsIProtectedAuthThread.h"
 #include "nsIWindowWatcher.h"
 #include "nsIX509CertDB.h"
 #include "nsIX509Cert.h"
 #include "nsNSSDialogHelper.h"
 #include "nsString.h"
+#include "nsVariant.h"
 
 #define PIPSTRING_BUNDLE_URL "chrome://pippki/locale/pippki.properties"
 
 nsNSSDialogs::nsNSSDialogs()
 {
 }
 
 nsNSSDialogs::~nsNSSDialogs()
@@ -160,96 +160,100 @@ nsNSSDialogs::ChooseCertificate(nsIInter
 {
   NS_ENSURE_ARG_POINTER(ctx);
   NS_ENSURE_ARG_POINTER(certList);
   NS_ENSURE_ARG_POINTER(selectedIndex);
   NS_ENSURE_ARG_POINTER(certificateChosen);
 
   *certificateChosen = false;
 
-  nsCOMPtr<nsIDialogParamBlock> block =
-    do_CreateInstance(NS_DIALOGPARAMBLOCK_CONTRACTID);
-  if (!block) {
+  nsCOMPtr<nsIMutableArray> argArray = nsArrayBase::Create();
+  if (!argArray) {
     return NS_ERROR_FAILURE;
   }
 
-  // SetObjects() expects an nsIMutableArray, which is why we can't directly use
-  // |certList| and have to add an extra layer of indirection.
-  nsCOMPtr<nsIMutableArray> paramBlockArray = nsArrayBase::Create();
-  if (!paramBlockArray) {
-    return NS_ERROR_FAILURE;
-  }
-  nsresult rv = paramBlockArray->AppendElement(certList, false);
+  nsCOMPtr<nsIWritableVariant> hostnameVariant = new nsVariant();
+  nsresult rv = hostnameVariant->SetAsAUTF8String(hostname);
   if (NS_FAILED(rv)) {
     return rv;
   }
-  rv = block->SetObjects(paramBlockArray);
+  rv = argArray->AppendElement(hostnameVariant, false);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  nsCOMPtr<nsIWritableVariant> organizationVariant = new nsVariant();
+  rv = organizationVariant->SetAsAUTF8String(organization);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  rv = argArray->AppendElement(organizationVariant, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  rv = block->SetNumberStrings(3);
+  nsCOMPtr<nsIWritableVariant> issuerOrgVariant = new nsVariant();
+  rv = issuerOrgVariant->SetAsAUTF8String(issuerOrg);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  rv = argArray->AppendElement(issuerOrgVariant, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  rv = block->SetString(0, NS_ConvertUTF8toUTF16(hostname).get());
+  nsCOMPtr<nsIWritableVariant> portVariant = new nsVariant();
+  rv = portVariant->SetAsInt32(port);
   if (NS_FAILED(rv)) {
     return rv;
   }
-  rv = block->SetString(1, NS_ConvertUTF8toUTF16(organization).get());
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-  rv = block->SetString(2, NS_ConvertUTF8toUTF16(issuerOrg).get());
+  rv = argArray->AppendElement(portVariant, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
-  rv = block->SetInt(0, port);
+  rv = argArray->AppendElement(certList, false);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+
+  nsCOMPtr<nsIWritablePropertyBag2> retVals = new nsHashPropertyBag();
+  rv = argArray->AppendElement(retVals, false);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   rv = nsNSSDialogHelper::openDialog(nullptr,
                                      "chrome://pippki/content/clientauthask.xul",
-                                     block);
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-
-  int32_t status;
-  rv = block->GetInt(0, &status);
+                                     argArray);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   nsCOMPtr<nsIClientAuthUserDecision> extraResult = do_QueryInterface(ctx);
   if (extraResult) {
-    int32_t rememberSelection;
-    rv = block->GetInt(2, &rememberSelection);
+    bool rememberSelection = false;
+    rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("rememberSelection"),
+                                    &rememberSelection);
     if (NS_SUCCEEDED(rv)) {
-      extraResult->SetRememberClientAuthCertificate(rememberSelection!=0);
+      extraResult->SetRememberClientAuthCertificate(rememberSelection);
     }
   }
 
-  *certificateChosen = (status != 0);
+  rv = retVals->GetPropertyAsBool(NS_LITERAL_STRING("certChosen"),
+                                  certificateChosen);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
   if (*certificateChosen) {
-    int32_t index = 0;
-    rv = block->GetInt(1, &index);
+    rv = retVals->GetPropertyAsUint32(NS_LITERAL_STRING("selectedIndex"),
+                                      selectedIndex);
     if (NS_FAILED(rv)) {
       return rv;
     }
-
-    if (index < 0) {
-      MOZ_ASSERT_UNREACHABLE("Selected index should never be negative");
-      return NS_ERROR_FAILURE;
-    }
-
-    *selectedIndex = mozilla::AssertedCast<uint32_t>(index);
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP 
 nsNSSDialogs::SetPKCS12FilePassword(nsIInterfaceRequestor *ctx, 
                                     nsAString &_password,
--- a/security/manager/pki/resources/content/clientauthask.js
+++ b/security/manager/pki/resources/content/clientauthask.js
@@ -1,68 +1,94 @@
 /* -*- tab-width: 2; indent-tabs-mode: nil; js-indent-level: 2 -*-
  *
  * 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";
 
+/**
+ * @file Implements the functionality of clientauthask.xul: a dialog that allows
+ *       a user pick a client certificate for TLS client authentication.
+ * @argument {String} window.arguments[0]
+ *           The hostname of the server requesting client authentication.
+ * @argument {String} window.arguments[1]
+ *           The Organization of the server cert.
+ * @argument {String} window.arguments[2]
+ *           The Organization of the issuer of the server cert.
+ * @argument {Number} window.arguments[3]
+ *           The port of the server.
+ * @argument {nsISupports} window.arguments[4]
+ *           List of certificates the user can choose from, queryable to
+ *           nsIArray<nsIX509Cert>.
+ * @argument {nsISupports} window.arguments[5]
+ *           Object to set the return values of calling the dialog on, queryable
+ *           to the underlying type of ClientAuthAskReturnValues.
+ */
+
+/**
+ * @typedef ClientAuthAskReturnValues
+ * @type nsIWritablePropertyBag2
+ * @property {Boolean} certChosen
+ *           Set to true if the user chose a cert and accepted the dialog, false
+ *           otherwise.
+ * @property {Boolean} rememberSelection
+ *           Set to true if the user wanted their cert selection to be
+ *           remembered, false otherwise.
+ * @property {Number} selectedIndex
+ *           The index the chosen cert is at for the given cert list. Undefined
+ *           value if |certChosen| is not true.
+ */
+
 const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
 
 const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
 
 /**
  * The pippki <stringbundle> element.
  * @type <stringbundle>
  */
 var bundle;
 /**
  * The array of certs the user can choose from.
  * @type nsIArray<nsIX509Cert>
  */
 var certArray;
 /**
- * The param block to get params from and set results on.
- * @type nsIDialogParamBlock
- */
-var dialogParams;
-/**
  * The checkbox storing whether the user wants to remember the selected cert.
  * @type nsIDOMXULCheckboxElement
  */
 var rememberBox;
 
 function onLoad() {
-  dialogParams = window.arguments[0].QueryInterface(Ci.nsIDialogParamBlock);
-
   bundle = document.getElementById("pippki_bundle");
   let rememberSetting =
     Services.prefs.getBoolPref("security.remember_cert_checkbox_default_setting");
 
   rememberBox = document.getElementById("rememberBox");
   rememberBox.label = bundle.getString("clientAuthRemember");
   rememberBox.checked = rememberSetting;
 
-  let hostname = dialogParams.GetString(0);
-  let org = dialogParams.GetString(1);
-  let issuerOrg = dialogParams.GetString(2);
-  let port = dialogParams.GetInt(0);
+  let hostname = window.arguments[0];
+  let org = window.arguments[1];
+  let issuerOrg = window.arguments[2];
+  let port = window.arguments[3];
   let formattedOrg = bundle.getFormattedString("clientAuthMessage1", [org]);
   let formattedIssuerOrg = bundle.getFormattedString("clientAuthMessage2",
                                                      [issuerOrg]);
   let formattedHostnameAndPort =
     bundle.getFormattedString("clientAuthHostnameAndPort",
                               [hostname, port.toString()]);
   setText("hostname", formattedHostnameAndPort);
   setText("organization", formattedOrg);
   setText("issuer", formattedIssuerOrg);
 
   let selectElement = document.getElementById("nicknames");
-  certArray = dialogParams.objects.queryElementAt(0, Ci.nsIArray);
+  certArray = window.arguments[4].QueryInterface(Ci.nsIArray);
   for (let i = 0; i < certArray.length; i++) {
     let menuItemNode = document.createElement("menuitem");
     let cert = certArray.queryElementAt(i, Ci.nsIX509Cert);
     let nickAndSerial =
       bundle.getFormattedString("clientAuthNickAndSerial",
                                 [cert.nickname, cert.serialNumber]);
     menuItemNode.setAttribute("value", i);
     menuItemNode.setAttribute("label", nickAndSerial); // This is displayed.
@@ -108,26 +134,22 @@ function setDetails() {
   document.getElementById("details").value = detailLines.join("\n");
 }
 
 function onCertSelected() {
   setDetails();
 }
 
 function doOK() {
-  // Signal that the user accepted.
-  dialogParams.SetInt(0, 1);
+  let retVals = window.arguments[5].QueryInterface(Ci.nsIWritablePropertyBag2);
+  retVals.setPropertyAsBool("certChosen", true);
   let index = parseInt(document.getElementById("nicknames").value);
-  // Signal the index of the selected cert in the list of cert nicknames
-  // provided.
-  dialogParams.SetInt(1, index);
-  // Signal whether the user wanted to remember the selection.
-  dialogParams.SetInt(2, rememberBox.checked);
+  retVals.setPropertyAsUint32("selectedIndex", index);
+  retVals.setPropertyAsBool("rememberSelection", rememberBox.checked);
   return true;
 }
 
 function doCancel() {
-  // Signal that the user cancelled.
-  dialogParams.SetInt(0, 0);
-  // Signal whether the user wanted to remember the "selection".
-  dialogParams.SetInt(2, rememberBox.checked);
+  let retVals = window.arguments[5].QueryInterface(Ci.nsIWritablePropertyBag2);
+  retVals.setPropertyAsBool("certChosen", false);
+  retVals.setPropertyAsBool("rememberSelection", rememberBox.checked);
   return true;
 }
--- a/security/manager/ssl/tests/mochitest/browser/browser_clientAuth_ui.js
+++ b/security/manager/ssl/tests/mochitest/browser/browser_clientAuth_ui.js
@@ -22,39 +22,31 @@ var cert;
 /**
  * Opens the client auth cert chooser 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 nsIDialogParamBlock passed to the dialog.
+ *            2. The return value nsIWritablePropertyBag2 passed to the dialog.
  */
 function openClientAuthDialog(cert) {
-  let params = Cc["@mozilla.org/embedcomp/dialogparam;1"]
-                 .createInstance(Ci.nsIDialogParamBlock);
-
   let certList = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
   certList.appendElement(cert, false);
-  let array = Cc["@mozilla.org/array;1"].createInstance(Ci.nsIMutableArray);
-  array.appendElement(certList, false);
-  params.objects = array;
 
-  params.SetString(0, TEST_HOSTNAME);
-  params.SetString(1, TEST_ORG);
-  params.SetString(2, TEST_ISSUER_ORG);
-  params.SetInt(0, TEST_PORT);
-
+  let returnVals = Cc["@mozilla.org/hash-property-bag;1"]
+                     .createInstance(Ci.nsIWritablePropertyBag2);
   let win = window.openDialog("chrome://pippki/content/clientauthask.xul", "",
-                              "", params);
+                              "", TEST_HOSTNAME, TEST_ORG, TEST_ISSUER_ORG,
+                              TEST_PORT, certList, returnVals);
   return new Promise((resolve, reject) => {
     win.addEventListener("load", function onLoad() {
       win.removeEventListener("load", onLoad);
-      resolve([win, params]);
+      resolve([win, returnVals]);
     });
   });
 }
 
 /**
  * Checks that the contents of the given cert chooser dialog match the details
  * of build/pgo/certs/mochitest.client.
  *
@@ -101,45 +93,45 @@ function checkDialogContents(win, notBef
 add_task(function* setup() {
   cert = certDB.findCertByNickname("test client certificate");
   Assert.notEqual(cert, null, "Should be able to find the test client cert");
 });
 
 // Test that the contents of the dialog correspond to the details of the
 // provided cert.
 add_task(function* testContents() {
-  let [win, params] = yield openClientAuthDialog(cert);
+  let [win, retVals] = yield openClientAuthDialog(cert);
   checkDialogContents(win, cert.validity.notBeforeLocalTime,
                       cert.validity.notAfterLocalTime);
   yield BrowserTestUtils.closeWindow(win);
 });
 
 // Test that the right values are returned when the dialog is accepted.
 add_task(function* testAcceptDialogReturnValues() {
-  let [win, params] = yield openClientAuthDialog(cert);
+  let [win, retVals] = yield openClientAuthDialog(cert);
   win.document.getElementById("rememberBox").checked = true;
   info("Accepting dialog");
   win.document.getElementById("certAuthAsk").acceptDialog();
   yield BrowserTestUtils.windowClosed(win);
 
-  Assert.equal(params.GetInt(0), 1,
-               "1 should be returned to signal user accepted");
-  Assert.equal(params.GetInt(1), 0,
+  Assert.ok(retVals.get("certChosen"),
+            "Return value should signal user chose a certificate");
+  Assert.equal(retVals.get("selectedIndex"), 0,
                "0 should be returned as the selected index");
-  Assert.equal(params.GetInt(2), 1,
-               "1 should be returned as the state of the 'Remember this " +
-               "decision' checkbox");
+  Assert.ok(retVals.get("rememberSelection"),
+            "Return value should signal 'Remember this decision' checkbox was" +
+            "checked");
 });
 
 // Test that the right values are returned when the dialog is canceled.
 add_task(function* testCancelDialogReturnValues() {
-  let [win, params] = yield openClientAuthDialog(cert);
+  let [win, retVals] = yield openClientAuthDialog(cert);
   win.document.getElementById("rememberBox").checked = false;
   info("Canceling dialog");
   win.document.getElementById("certAuthAsk").cancelDialog();
   yield BrowserTestUtils.windowClosed(win);
 
-  Assert.equal(params.GetInt(0), 0,
-               "0 should be returned to signal user canceled");
-  Assert.equal(params.GetInt(2), 0,
-               "0 should be returned as the state of the 'Remember this " +
-               "decision' checkbox");
+  Assert.ok(!retVals.get("certChosen"),
+            "Return value should signal user did not choose a certificate");
+  Assert.ok(!retVals.get("rememberSelection"),
+            "Return value should signal 'Remember this decision' checkbox was" +
+            "unchecked");
 });