Settings > Internet: Fix certificate updates and improve selection.

This fixes a bug where certificate updates were not triggering a refresh
in the new mojo based JS.

While testing that I discovered that the current default certificate
selection logic is broken. This is a subtle thing so was not noticed
previously. This fixes the logic, cleans up some code, and adds some
tests.


Bug: 1001598
Change-Id: Ib81d7f6e093e1e5837d5272c848a3422d6a80b5a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1825120
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700040}

chrome/test/data/webui/chromeos/fake_network_config_mojom.js

@@ -115,7 +115,10 @@ class FakeNetworkConfig {
    * @return {!Promise}
    */
   whenCalled(methodName) {
-    return this.getResolver_(methodName).promise;
+    return this.getResolver_(methodName).promise.then(() => {
+      // Support sequential calls to whenCalled by replacing the promise.
+      this.resolverMap_.set(methodName, new PromiseResolver());
+    });
   }
 
   /**

chrome/test/data/webui/cr_components/network_config_test.js

@@ -8,6 +8,10 @@ suite('network-config', function() {
   /** @type {?chromeos.networkConfig.mojom.CrosNetworkConfigRemote} */
   let mojoApi_ = null;
 
+  const kCaHash = 'CAHASH';
+  const kUserHash1 = 'USERHASH1';
+  const kUserHash2 = 'USERHASH2';
+
   suiteSetup(function() {
     mojoApi_ = new FakeNetworkConfig();
     network_config.MojoInterfaceProviderImpl.getInstance().remote_ = mojoApi_;
@@ -121,15 +125,6 @@ suite('network-config', function() {
       networkConfig.shareDefault = false;
     }
 
-    function setCertificatesForTest() {
-      const kHash1 = 'TESTHASH1', kHash2 = 'TESTHASH2';
-      var clientCert = {hash: kHash1, hardwareBacked: true, deviceWide: false};
-      var caCert = {hash: kHash2, hardwareBacked: true, deviceWide: true};
-      mojoApi_.setCertificatesForTest([caCert], [clientCert]);
-      this.selectedUserCertHash_ = kHash1;
-      this.selectedServerCaHash_ = kHash2;
-    }
-
     test('New Config: Login or guest', function() {
       // Insecure networks are always shared so test a secure config.
       setNetworkType(
@@ -238,43 +233,83 @@ suite('network-config', function() {
         assertEquals('PEAP', outer.value);
       });
     });
+  });
 
-    test('WiFi EAP TLS', function() {
-      const wifi1 = OncMojo.getDefaultManagedProperties(
-          chromeos.networkConfig.mojom.NetworkType.kWiFi, 'eaptlsguid', '');
-      wifi1.wifi.security = chromeos.networkConfig.mojom.SecurityType.kWpaEap;
-      wifi1.wifi.eap = {outer: OncMojo.createManagedString('EAP-TLS')};
-      setNetworkConfig(wifi1);
-      setCertificatesForTest();
+  suite('Certificates', function() {
+    setup(function() {
+      mojoApi_.resetForTest();
+    });
+
+    teardown(function() {
+      PolymerTest.clearBody();
+    });
+
+    function setAuthenticated() {
+      // Logged in users can share new networks.
+      networkConfig.shareAllowEnable = true;
+      // Authenticated networks default to not shared.
+      networkConfig.shareDefault = false;
+    }
+
+    test('WiFi EAP-TLS No Certs', function() {
+      setNetworkType(
+          chromeos.networkConfig.mojom.NetworkType.kWiFi,
+          chromeos.networkConfig.mojom.SecurityType.kWpaEap);
       setAuthenticated();
       initNetworkConfig();
+      networkConfig.shareNetwork_ = false;
+      networkConfig.set('eapProperties_.outer', 'EAP-TLS');
       return mojoApi_.whenCalled('getNetworkCertificates').then(() => {
         return flushAsync().then(() => {
           let outer = networkConfig.$$('#outer');
           assertEquals('EAP-TLS', outer.value);
+          // Check that with no certificates, 'do-not-check' amd 'no-certs'
+          // are selected.
+          assertEquals('do-not-check', networkConfig.selectedServerCaHash_);
+          assertEquals('no-certs', networkConfig.selectedUserCertHash_);
+        });
+      });
+    });
 
-          // check that a valid client user certificate is selected
-          let clientCert = networkConfig.$$('#userCert').$$('select').value;
-          assertTrue(!!clientCert);
-          let caCert = networkConfig.$$('#serverCa').$$('select').value;
-          assertTrue(!!caCert);
-
-          let share = networkConfig.$$('#share');
-          assertTrue(!!share);
-          // share the EAP TLS network
-          share.checked = true;
-          // trigger the onShareChanged_ event
-          var event = new Event('change');
-          share.dispatchEvent(event);
-          // check that share is enabled
-          assertTrue(share.checked);
+    test('WiFi EAP-TLS Certs', function() {
+      setNetworkType(
+          chromeos.networkConfig.mojom.NetworkType.kWiFi,
+          chromeos.networkConfig.mojom.SecurityType.kWpaEap);
+      setAuthenticated();
+      mojoApi_.setCertificatesForTest(
+          [{hash: kCaHash, hardwareBacked: true, deviceWide: true}],
+          [{hash: kUserHash1, hardwareBacked: true, deviceWide: false}]);
+      initNetworkConfig();
+      networkConfig.shareNetwork_ = false;
+      networkConfig.set('eapProperties_.outer', 'EAP-TLS');
+      return mojoApi_.whenCalled('getNetworkCertificates').then(() => {
+        return flushAsync().then(() => {
+          // The first Server CA  and User certificate should be selected.
+          assertEquals(kCaHash, networkConfig.selectedServerCaHash_);
+          assertEquals(kUserHash1, networkConfig.selectedUserCertHash_);
+        });
+      });
+    });
 
-          // check that client certificate selection is empty
-          clientCert = networkConfig.$$('#userCert').$$('select').value;
-          assertFalse(!!clientCert);
-          // check that ca device-wide cert is still selected
-          caCert = networkConfig.$$('#serverCa').$$('select').value;
-          assertTrue(!!caCert);
+    test('WiFi EAP-TLS Certs Shared', function() {
+      setNetworkType(
+          chromeos.networkConfig.mojom.NetworkType.kWiFi,
+          chromeos.networkConfig.mojom.SecurityType.kWpaEap);
+      setAuthenticated();
+      mojoApi_.setCertificatesForTest(
+          [{hash: kCaHash, hardwareBacked: true, deviceWide: true}], [
+            {hash: kUserHash1, hardwareBacked: true, deviceWide: false},
+            {hash: kUserHash2, hardwareBacked: true, deviceWide: true}
+          ]);
+      initNetworkConfig();
+      networkConfig.shareNetwork_ = true;
+      networkConfig.set('eapProperties_.outer', 'EAP-TLS');
+      return mojoApi_.whenCalled('getNetworkCertificates').then(() => {
+        return flushAsync().then(() => {
+          // The first Server CA should be selected.
+          assertEquals(kCaHash, networkConfig.selectedServerCaHash_);
+          // Second User Hash should be selected since it is a device cert.
+          assertEquals(kUserHash2, networkConfig.selectedUserCertHash_);
         });
       });
     });

ui/webui/resources/cr_components/chromeos/network/network_config.js

@@ -383,7 +383,7 @@ Polymer({
     this.hiddenNetworkWarning_ = this.showHiddenNetworkWarning_();
 
     this.updateIsConfigured_();
-    this.updateDeviceCertsOnly_();
+    this.onNetworkCertificatesChanged();
   },
 
   save: function() {
@@ -477,7 +477,7 @@ Polymer({
   },
 
   /** CrNetworkListenerBehavior override */
-  onCertificateListsChanged_: function() {
+  onNetworkCertificatesChanged: function() {
     this.networkConfig_.getNetworkCertificates().then(response => {
       const isOpenVpn = this.configProperties_.type == mojom.NetworkType.kVPN &&
           this.configProperties_.vpn.type == mojom.VpnType.kOpenVPN;
@@ -648,7 +648,7 @@ Polymer({
 
   /** @private */
   onShareChanged_: function(event) {
-    this.updateDeviceCertsOnly_();
+    this.updateSelectedCerts_();
   },
 
   /**
@@ -811,7 +811,6 @@ Polymer({
     if (managedProperties.type == mojom.NetworkType.kVPN) {
       this.vpnType_ = this.getVpnTypeFromProperties_(this.configProperties_);
     }
-    this.onCertificateListsChanged_();
   },
 
   /**
@@ -1031,7 +1030,6 @@ Polymer({
       };
     }
     this.updateCertError_();
-    this.onCertificateListsChanged_();
   },
 
   /** @private */
@@ -1145,29 +1143,53 @@ Polymer({
    * @private
    */
   updateSelectedCerts_: function() {
+    if (!this.serverCaCerts_.length || !this.userCerts_.length) {
+      return;
+    }
+    const eap = this.eapProperties_;
+
+    // Only device-wide certificates can be used for shared networks that
+    // require a certificate.
+    this.deviceCertsOnly_ =
+        this.shareNetwork_ && !!eap && eap.outer == 'EAP-TLS';
+
     // Validate selected Server CA.
-    if (!this.findCert_(this.serverCaCerts_, this.selectedServerCaHash_)) {
+    const caCert =
+        this.findCert_(this.serverCaCerts_, this.selectedServerCaHash_);
+    if (!caCert || (this.deviceCertsOnly_ && !caCert.deviceWide)) {
       this.selectedServerCaHash_ = undefined;
     }
     if (!this.selectedServerCaHash_) {
-      const eap = this.eapProperties_;
-      if (eap) {
-        this.selectedServerCaHash_ =
-            eap.useSystemCas ? DEFAULT_HASH : DO_NOT_CHECK_HASH;
+      if (eap && eap.useSystemCas) {
+        this.selectedServerCaHash_ = DEFAULT_HASH;
       } else if (!this.guid && this.serverCaCerts_[0]) {
-        // For unconfigured networks only, default to the first CA.
-        this.selectedServerCaHash_ = this.serverCaCerts_[0].hash;
+        // For unconfigured networks, default to the first available
+        // certificate, or DO_NOT_CHECK (i.e. skip DEFAULT_HASH). See
+        /// onNetworkCertificatesChanged() for how certificates are added.
+        let cert = this.serverCaCerts_[0];
+        if (cert.hash == DEFAULT_HASH && this.serverCaCerts_[1]) {
+          cert = this.serverCaCerts_[1];
+        }
+        this.selectedServerCaHash_ = cert.hash;
       } else {
         this.selectedServerCaHash_ = DO_NOT_CHECK_HASH;
       }
     }
 
     // Validate selected User cert.
-    if (!this.findCert_(this.userCerts_, this.selectedUserCertHash_)) {
+    const userCert =
+        this.findCert_(this.userCerts_, this.selectedUserCertHash_);
+    if (!userCert || (this.deviceCertsOnly_ && !userCert.deviceWide)) {
       this.selectedUserCertHash_ = undefined;
     }
-    if (!this.selectedUserCertHash_ && this.userCerts_[0]) {
-      this.selectedUserCertHash_ = this.userCerts_[0].hash;
+    if (!this.selectedUserCertHash_) {
+      for (let i = 0; i < this.userCerts_.length; ++i) {
+        const userCert = this.userCerts_[i];
+        if (userCert && (!this.deviceCertsOnly_ || userCert.deviceWide)) {
+          this.selectedUserCertHash_ = userCert.hash;
+          break;
+        }
+      }
     }
   },
 
@@ -1207,27 +1229,6 @@ Polymer({
     this.isConfigured_ = this.getIsConfigured_();
   },
 
-  /** @private */
-  updateDeviceCertsOnly_: function() {
-    // Only device-wide certificates can be used for networks that require a
-    // certificate and are shared.
-    const eap = this.eapProperties_;
-    if (!this.shareNetwork_ || !eap || eap.outer != 'EAP-TLS') {
-      this.deviceCertsOnly_ = false;
-      return;
-    }
-    // Clear selection if certificate is not device-wide.
-    let cert = this.findCert_(this.userCerts_, this.selectedUserCertHash_);
-    if (cert && !cert.deviceWide) {
-      this.selectedUserCertHash_ = undefined;
-    }
-    cert = this.findCert_(this.serverCaCerts_, this.selectedServerCaHash_);
-    if (cert && !cert.deviceWide) {
-      this.selectedServerCaHash_ = undefined;
-    }
-    this.deviceCertsOnly_ = true;
-  },
-
   /**
    * @param {mojom.NetworkType} networkType
    * @return {boolean}