From 7d56c9e7e69a8ed095635262d987c727ab3d1515 Mon Sep 17 00:00:00 2001
From: Cykesiopka <cykesiopka.bmo@gmail.com>
Date: Mon, 10 Oct 2016 16:08:36 +0800
Subject: [PATCH] Bug 332442 - Stop abusing window name to pass cert ref to
 editcerts.js. r=mgoodwin

editcacert.xul/editcerts.js currently requires the |dbKey| attribute of an
nsIX509Cert to be passed to it via the window name so it can get a handle to the
nsIX509Cert.

This has two problems:
1. This used to trigger warnings, and is unwise to do in any case.
2. It's unnecessary complexity - the nsIX509Cert can be passed directly.

This patch:
1. Addresses the two problems.
2. Adds a test to ensure the functionality of editcerts.js actually works.
3. Rewrites editcerts.js to better fit modern PSM style.
4. Updates the name of editcerts.js so it's more consistent with the general
   convention under security/pki/resources/content

MozReview-Commit-ID: ECxziXq5TmL

--HG--
rename : security/manager/pki/resources/content/editcerts.js => security/manager/pki/resources/content/editcacert.js
extra : rebase_source : 46a6b2ff2ee90aded61a27b21ce3d5c1a8bed5c2
---
 .../pki/resources/content/certManager.js      |   8 +-
 .../pki/resources/content/editcacert.js       |  58 +++++++++
 .../pki/resources/content/editcacert.xul      |   9 +-
 .../pki/resources/content/editcerts.js        |  71 -----------
 security/manager/pki/resources/jar.mn         |   2 +-
 .../ssl/tests/mochitest/browser/browser.ini   |   1 +
 .../browser/browser_editCACertTrust.js        | 119 ++++++++++++++++++
 7 files changed, 190 insertions(+), 78 deletions(-)
 create mode 100644 security/manager/pki/resources/content/editcacert.js
 delete mode 100644 security/manager/pki/resources/content/editcerts.js
 create mode 100644 security/manager/ssl/tests/mochitest/browser/browser_editCACertTrust.js

diff --git a/security/manager/pki/resources/content/certManager.js b/security/manager/pki/resources/content/certManager.js
index 36b257cd2e88..0b4de12b2143 100644
--- a/security/manager/pki/resources/content/certManager.js
+++ b/security/manager/pki/resources/content/certManager.js
@@ -23,6 +23,10 @@ var { Services } = Components.utils.import("resource://gre/modules/Services.jsm"
 
 var key;
 
+/**
+ * List of certs currently selected in the active tab.
+ * @type nsIX509Cert[]
+ */
 var selected_certs = [];
 var selected_tree_items = [];
 var selected_index = [];
@@ -329,8 +333,8 @@ function editCerts()
   getSelectedCerts();
 
   for (let cert of selected_certs) {
-    window.openDialog("chrome://pippki/content/editcacert.xul", cert.dbKey,
-                      "chrome,centerscreen,modal");
+    window.openDialog("chrome://pippki/content/editcacert.xul", "",
+                      "chrome,centerscreen,modal", cert);
   }
 }
 
diff --git a/security/manager/pki/resources/content/editcacert.js b/security/manager/pki/resources/content/editcacert.js
new file mode 100644
index 000000000000..405d5281a42c
--- /dev/null
+++ b/security/manager/pki/resources/content/editcacert.js
@@ -0,0 +1,58 @@
+/* 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 { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
+
+var gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
+                .getService(Ci.nsIX509CertDB);
+/**
+ * Cert to edit the trust of.
+ * @type nsIX509Cert
+ */
+var gCert;
+
+/**
+ * onload() handler.
+ */
+function onLoad() {
+  gCert = window.arguments[0];
+
+  let bundle = document.getElementById("pippki_bundle");
+  setText("certmsg",
+          bundle.getFormattedString("editTrustCA", [gCert.commonName]));
+
+  let sslCheckbox = document.getElementById("trustSSL");
+  sslCheckbox.checked = gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                              Ci.nsIX509CertDB.TRUSTED_SSL);
+
+  let emailCheckbox = document.getElementById("trustEmail");
+  emailCheckbox.checked = gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                                Ci.nsIX509CertDB.TRUSTED_EMAIL);
+
+  let objSignCheckbox = document.getElementById("trustObjSign");
+  objSignCheckbox.checked =
+    gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                          Ci.nsIX509CertDB.TRUSTED_OBJSIGN);
+}
+
+/**
+ * ondialogaccept() handler.
+ *
+ * @returns {Boolean} true to make the dialog close, false otherwise.
+ */
+function onDialogAccept() {
+  let sslCheckbox = document.getElementById("trustSSL");
+  let emailCheckbox = document.getElementById("trustEmail");
+  let objSignCheckbox = document.getElementById("trustObjSign");
+  let trustSSL = sslCheckbox.checked ? Ci.nsIX509CertDB.TRUSTED_SSL : 0;
+  let trustEmail = emailCheckbox.checked ? Ci.nsIX509CertDB.TRUSTED_EMAIL : 0;
+  let trustObjSign = objSignCheckbox.checked ? Ci.nsIX509CertDB.TRUSTED_OBJSIGN
+                                             : 0;
+
+  gCertDB.setCertTrust(gCert, Ci.nsIX509Cert.CA_CERT,
+                       trustSSL | trustEmail | trustObjSign);
+  return true;
+}
diff --git a/security/manager/pki/resources/content/editcacert.xul b/security/manager/pki/resources/content/editcacert.xul
index b3f295e40379..46ea4f1f928d 100644
--- a/security/manager/pki/resources/content/editcacert.xul
+++ b/security/manager/pki/resources/content/editcacert.xul
@@ -7,18 +7,19 @@
 
 <!DOCTYPE dialog SYSTEM "chrome://pippki/locale/certManager.dtd">
 
-<dialog id="editCaCert" 
+<dialog id="editCaCert"
         title="&certmgr.editcacert.title;"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
         buttons="accept,cancel"
-        ondialogaccept="return doOK();"
-        onload="setWindowName();"
+        ondialogaccept="return onDialogAccept();"
+        onload="onLoad();"
 >
 
   <stringbundle id="pippki_bundle" src="chrome://pippki/locale/pippki.properties"/>
 
   <script type="application/javascript" src="chrome://pippki/content/pippki.js"/>
-  <script type="application/javascript" src="chrome://pippki/content/editcerts.js"/>
+  <script type="application/javascript"
+          src="chrome://pippki/content/editcacert.js"/>
 
   <description id="certmsg"/>
   <separator/>
diff --git a/security/manager/pki/resources/content/editcerts.js b/security/manager/pki/resources/content/editcerts.js
deleted file mode 100644
index 5b210ee0a898..000000000000
--- a/security/manager/pki/resources/content/editcerts.js
+++ /dev/null
@@ -1,71 +0,0 @@
-/* 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 nsIX509Cert = Components.interfaces.nsIX509Cert;
-const nsX509CertDB = "@mozilla.org/security/x509certdb;1";
-const nsIX509CertDB = Components.interfaces.nsIX509CertDB;
-
-var certdb;
-var cert;
-
-function doPrompt(msg)
-{
-  let prompts = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].
-    getService(Components.interfaces.nsIPromptService);
-  prompts.alert(window, null, msg);
-}
-
-function setWindowName()
-{
-  var dbkey = self.name;
-
-  //  Get the cert from the cert database
-  certdb = Components.classes[nsX509CertDB].getService(nsIX509CertDB);
-  cert = certdb.findCertByDBKey(dbkey);
-
-  var bundle = document.getElementById("pippki_bundle");
-
-  var message1 = bundle.getFormattedString("editTrustCA", [cert.commonName]);
-  setText("certmsg", message1);
-
-  var ssl = document.getElementById("trustSSL");
-  if (certdb.isCertTrusted(cert, nsIX509Cert.CA_CERT,
-                           nsIX509CertDB.TRUSTED_SSL)) {
-    ssl.setAttribute("checked", "true");
-  } else {
-    ssl.setAttribute("checked", "false");
-  }
-  var email = document.getElementById("trustEmail");
-  if (certdb.isCertTrusted(cert, nsIX509Cert.CA_CERT,
-                           nsIX509CertDB.TRUSTED_EMAIL)) {
-    email.setAttribute("checked", "true");
-  } else {
-    email.setAttribute("checked", "false");
-  }
-  var objsign = document.getElementById("trustObjSign");
-  if (certdb.isCertTrusted(cert, nsIX509Cert.CA_CERT,
-                           nsIX509CertDB.TRUSTED_OBJSIGN)) {
-    objsign.setAttribute("checked", "true");
-  } else {
-    objsign.setAttribute("checked", "false");
-  }
-}
-
-function doOK()
-{
-  var ssl = document.getElementById("trustSSL");
-  var email = document.getElementById("trustEmail");
-  var objsign = document.getElementById("trustObjSign");
-  var trustssl = (ssl.checked) ? nsIX509CertDB.TRUSTED_SSL : 0;
-  var trustemail = (email.checked) ? nsIX509CertDB.TRUSTED_EMAIL : 0;
-  var trustobjsign = (objsign.checked) ? nsIX509CertDB.TRUSTED_OBJSIGN : 0;
-  //
-  //  Set the cert trust
-  //
-  certdb.setCertTrust(cert, nsIX509Cert.CA_CERT,
-                      trustssl | trustemail | trustobjsign);
-  return true;
-}
diff --git a/security/manager/pki/resources/jar.mn b/security/manager/pki/resources/jar.mn
index d1e3d5413fff..4ef3d4f2a808 100644
--- a/security/manager/pki/resources/jar.mn
+++ b/security/manager/pki/resources/jar.mn
@@ -19,7 +19,7 @@ pippki.jar:
     content/pippki/OrphanOverlay.xul         (content/OrphanOverlay.xul)
     content/pippki/viewCertDetails.xul       (content/viewCertDetails.xul)
     content/pippki/editcacert.xul            (content/editcacert.xul)
-    content/pippki/editcerts.js              (content/editcerts.js)
+    content/pippki/editcacert.js             (content/editcacert.js)
 *   content/pippki/exceptionDialog.xul       (content/exceptionDialog.xul)
     content/pippki/exceptionDialog.js        (content/exceptionDialog.js)
     content/pippki/deletecert.xul            (content/deletecert.xul)
diff --git a/security/manager/ssl/tests/mochitest/browser/browser.ini b/security/manager/ssl/tests/mochitest/browser/browser.ini
index 206911d23ad6..32ea874dd274 100644
--- a/security/manager/ssl/tests/mochitest/browser/browser.ini
+++ b/security/manager/ssl/tests/mochitest/browser/browser.ini
@@ -10,3 +10,4 @@ support-files =
 [browser_clientAuth_connection.js]
 [browser_clientAuth_ui.js]
 [browser_deleteCert_ui.js]
+[browser_editCACertTrust.js]
diff --git a/security/manager/ssl/tests/mochitest/browser/browser_editCACertTrust.js b/security/manager/ssl/tests/mochitest/browser/browser_editCACertTrust.js
new file mode 100644
index 000000000000..bc87732a5078
--- /dev/null
+++ b/security/manager/ssl/tests/mochitest/browser/browser_editCACertTrust.js
@@ -0,0 +1,119 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/publicdomain/zero/1.0/
+"use strict";
+
+// Tests that the UI for editing the trust of a CA certificate correctly
+// reflects trust in the cert DB, and correctly updates trust in the cert DB
+// when requested.
+
+var gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
+                .getService(Ci.nsIX509CertDB);
+
+/**
+ * The cert we're editing the trust of.
+ * @type nsIX509Cert
+ */
+var gCert;
+
+/**
+ * Opens the cert trust editing dialog.
+ *
+ * @returns {Promise}
+ *          A promise that resolves when the dialog has finished loading with
+ *          the window of the opened dialog.
+ */
+function openEditCertTrustDialog() {
+  let win = window.openDialog("chrome://pippki/content/editcacert.xul", "", "",
+                              gCert);
+  return new Promise((resolve, reject) => {
+    win.addEventListener("load", function onLoad() {
+      win.removeEventListener("load", onLoad);
+      resolve(win);
+    });
+  });
+}
+
+add_task(function* setup() {
+  // Initially trust ca.pem for SSL, but not e-mail or object signing.
+  gCert = yield readCertificate("ca.pem", "CT,,");
+  Assert.ok(gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                  Ci.nsIX509CertDB.TRUSTED_SSL),
+            "Sanity check: ca.pem should be trusted for SSL");
+  Assert.ok(!gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                   Ci.nsIX509CertDB.TRUSTED_EMAIL),
+            "Sanity check: ca.pem should not be trusted for e-mail");
+  Assert.ok(!gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                   Ci.nsIX509CertDB.TRUSTED_OBJSIGN),
+            "Sanity check: ca.pem should not be trusted for object signing");
+});
+
+// Tests the following:
+// 1. The checkboxes correctly reflect the trust set in setup().
+// 2. Accepting the dialog after flipping some of the checkboxes results in the
+//    correct trust being set in the cert DB.
+add_task(function* testAcceptDialog() {
+  let win = yield openEditCertTrustDialog();
+
+  let sslCheckbox = win.document.getElementById("trustSSL");
+  let emailCheckbox = win.document.getElementById("trustEmail");
+  let objSignCheckbox = win.document.getElementById("trustObjSign");
+  Assert.ok(sslCheckbox.checked,
+            "Cert should be trusted for SSL in UI");
+  Assert.ok(!emailCheckbox.checked,
+            "Cert should not be trusted for e-mail in UI");
+  Assert.ok(!objSignCheckbox.checked,
+            "Cert should not be trusted for object signing in UI");
+
+  sslCheckbox.checked = false;
+  emailCheckbox.checked = true;
+
+  info("Accepting dialog");
+  win.document.getElementById("editCaCert").acceptDialog();
+  yield BrowserTestUtils.windowClosed(win);
+
+  Assert.ok(!gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                   Ci.nsIX509CertDB.TRUSTED_SSL),
+            "Cert should no longer be trusted for SSL");
+  Assert.ok(gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                  Ci.nsIX509CertDB.TRUSTED_EMAIL),
+            "Cert should now be trusted for e-mail");
+  Assert.ok(!gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                   Ci.nsIX509CertDB.TRUSTED_OBJSIGN),
+            "Cert should still not be trusted for object signing");
+});
+
+// Tests the following:
+// 1. The checkboxes correctly reflect the trust set in testAcceptDialog().
+// 2. Canceling the dialog even after flipping the checkboxes doesn't result in
+//    a change of trust in the cert DB.
+add_task(function* testCancelDialog() {
+  let win = yield openEditCertTrustDialog();
+
+  let sslCheckbox = win.document.getElementById("trustSSL");
+  let emailCheckbox = win.document.getElementById("trustEmail");
+  let objSignCheckbox = win.document.getElementById("trustObjSign");
+  Assert.ok(!sslCheckbox.checked,
+            "Cert should not be trusted for SSL in UI");
+  Assert.ok(emailCheckbox.checked,
+            "Cert should be trusted for e-mail in UI");
+  Assert.ok(!objSignCheckbox.checked,
+            "Cert should not be trusted for object signing in UI");
+
+  sslCheckbox.checked = true;
+  emailCheckbox.checked = false;
+  objSignCheckbox.checked = true;
+
+  info("Canceling dialog");
+  win.document.getElementById("editCaCert").cancelDialog();
+  yield BrowserTestUtils.windowClosed(win);
+
+  Assert.ok(!gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                  Ci.nsIX509CertDB.TRUSTED_SSL),
+            "Cert should still not be trusted for SSL");
+  Assert.ok(gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                  Ci.nsIX509CertDB.TRUSTED_EMAIL),
+            "Cert should still be trusted for e-mail");
+  Assert.ok(!gCertDB.isCertTrusted(gCert, Ci.nsIX509Cert.CA_CERT,
+                                   Ci.nsIX509CertDB.TRUSTED_OBJSIGN),
+            "Cert should still not be trusted for object signing");
+});