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
--- 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");
+});