crostini: Prevent adding duplicate ports in the port forwarding UI

Signalling to the user that they can't add a port that's already being
stored in the port forwarding settings for Crostini. Also, touched up
some of the browser tests.

Tests: Added browser tests and tested changes locally
Bug: 848127
Change-Id: Ia001380f6a08970f20f43029b6dc78acc47ccadc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2210339
Commit-Queue: Daniel Ng <danielng@google.com>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Julian Watson <juwa@google.com>
Cr-Commit-Position: refs/heads/master@{#772580}

chrome/app/os_settings_strings.grdp

@@ -992,6 +992,9 @@
   <message name="IDS_SETTINGS_CROSTINI_PORT_FORWARDING_ADD_ERROR" desc="Description for the error when adding an invalid port.">
     Error forwarding port
   </message>
+  <message name="IDS_SETTINGS_CROSTINI_PORT_FORWARDING_ADD_EXISTING" desc="Description for the error when trying to add a port that already exists.">
+    Port already exists in settings
+  </message>
   <message name="IDS_SETTINGS_CROSTINI_PORT_FORWARDING_REMOVE_ALL_PORTS" desc="Label for the button to deallocate all ports currently being forwarded in Crostini and clear the list of ports in the Crostini port forwarding settings page.">
     Remove all ports
   </message>

chrome/app/os_settings_strings_grdp/IDS_SETTINGS_CROSTINI_PORT_FORWARDING_ADD_EXISTING.png.sha1

+2e164b48a1a46e227d870ade182f3cde916ff2b6
\ No newline at end of file

chrome/browser/chromeos/crostini/crostini_port_forwarder.cc

@@ -241,6 +241,7 @@ void CrostiniPortForwarder::TryDeactivatePort(
     case Protocol::UDP:
       client->ReleaseUdpPortForward(key.port_number, current_interface_,
                                     std::move(result_callback));
+      break;
   }
 }
 

chrome/browser/resources/settings/chromeos/crostini_page/BUILD.gn

@@ -12,6 +12,7 @@ js_type_check("closure_compile") {
     ":crostini_export_import",
     ":crostini_page",
     ":crostini_port_forwarding",
+    ":crostini_port_forwarding_add_port_dialog",
     ":crostini_shared_paths",
     ":crostini_shared_usb_devices",
     ":crostini_subpage",
@@ -92,6 +93,14 @@ js_library("crostini_port_forwarding") {
   ]
 }
 
+js_library("crostini_port_forwarding_add_port_dialog") {
+  deps = [
+    ":crostini_browser_proxy",
+    "..:metrics_recorder",
+    "//ui/webui/resources/js:cr",
+  ]
+}
+
 js_library("crostini_subpage") {
   deps = [
     ":crostini_browser_proxy",

chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding.html

@@ -121,7 +121,9 @@
       </div>
     </template>
     <template is="dom-if" if="[[showAddPortDialog_]]" restamp>
-      <settings-crostini-add-port-dialog on-close="onAddPortDialogClose_">
+      <settings-crostini-add-port-dialog
+          on-close="onAddPortDialogClose_"
+          all-ports="[[allPorts_]]">
       </settings-crostini-add-port-dialog>
     </template>
     <cr-lazy-render id="removeAllPortsMenu">

chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding_add_port_dialog.html

@@ -11,6 +11,11 @@
 <dom-module id="settings-crostini-add-port-dialog">
   <template>
     <style include="settings-shared md-select">
+      #errorText {
+        color: var(--settings-error-color);
+        float: left;
+      }
+
       #portNumberInput {
         padding-inline-end: 20px;
         width: 376px;
@@ -25,11 +30,6 @@
         max-width: 96px;
       }
 
-      #errorText {
-        color: var(--settings-error-color);
-        float: left;
-      }
-
       .custom-body {
         padding-bottom: 20px;
       }
@@ -83,11 +83,9 @@
         </div>
       </div>
       <div slot="body" class="custom-body">
-        <template is="dom-if" if="[[invalidPort_]]">
-          <span id="errorText">
-            $i18n{crostiniPortForwardingAddError}
-          </span>
-        </template>
+        <span id="errorText">
+          [[portState_]]
+        </span>
         <div slot="button-container" class="custom-button-container">
           <cr-button id="cancel" class="cancel-button"
               on-click="onCancelTap_">$i18n{cancel}</cr-button>

chrome/browser/resources/settings/chromeos/crostini_page/crostini_port_forwarding_add_port_dialog.js

@@ -7,6 +7,16 @@
  * user to start forwarding a different port by filling in the appropriate
  * fields and clicking add.
  */
+
+/**
+ * @enum {string}
+ */
+const PortState = {
+  VALID: '',
+  INVALID: loadTimeData.getString('crostiniPortForwardingAddError'),
+  DUPLICATE: loadTimeData.getString('crostiniPortForwardingAddExisting'),
+};
+
 const MIN_VALID_PORT_NUMBER = 1024;   // Minimum 16-bit integer value.
 const MAX_VALID_PORT_NUMBER = 65535;  // Maximum 16-bit integer value.
 
@@ -39,11 +49,22 @@ Polymer({
     },
 
     /**
-     * @private {boolean}
+     * @private {!PortState}
+     */
+    portState_: {
+      type: String,
+      value: PortState.VALID,
+    },
+
+    /**
+     * List of ports that are already stored in the settings.
+     * @type {!Array<!CrostiniPortSetting>}
      */
-    invalidPort_: {
-      type: Boolean,
-      value: false,
+    allPorts: {
+      type: Array,
+      value() {
+        return [];
+      },
     },
   },
 
@@ -51,7 +72,7 @@ Polymer({
   attached: function() {
     this.$.dialog.showModal();
     this.async(() => {
-      this.portNumberInput.focus();
+      this.$.portNumberInput.focus();
     }, 1);
   },
 
@@ -59,25 +80,26 @@ Polymer({
     this.inputPortLabel_ = '';
     this.inputPortNumber_ = null;
     this.inputProtocolIndex_ = 0;
+    this.portState_ = PortState.VALID;
   },
 
   /**
-   * @returns {string} input for the port number.
+   * @return {string} input for the port number.
    */
   get portNumberInput() {
     return this.$.portNumberInput;
   },
 
   /**
-   * @returns {string} input for the optional port label.
+   * @return {string} input for the optional port label.
    */
   get portLabelInput() {
     return this.$.portLabelInput;
   },
 
   /**
-   * @param {string} to verify.
-   * @returns {boolean} if the input string is a valid port number.
+   * @param {string} input The port input to verify.
+   * @return {?boolean} if the input string is a valid port number.
    */
   isValidPortNumber: function(input) {
     const numberRegex = /^[0-9]+$/;
@@ -85,6 +107,23 @@ Polymer({
         Number(input) <= MAX_VALID_PORT_NUMBER;
   },
 
+  /**
+   * @return {!PortState}
+   * @private
+   */
+  computePortState_: function() {
+    if (!this.isValidPortNumber(this.$.portNumberInput.value)) {
+      return PortState.INVALID;
+    }
+    if (this.allPorts.find(
+            portSetting =>
+                portSetting.port_number == this.$.portNumberInput.value &&
+                portSetting.protocol_type == this.inputProtocolIndex_)) {
+      return PortState.DUPLICATE;
+    }
+    return PortState.VALID;
+  },
+
   /**
    * @param {!Event} e
    * @private
@@ -101,17 +140,18 @@ Polymer({
 
   /** @private */
   onAddTap_: function() {
-    if (!this.isValidPortNumber(this.portNumberInput.value)) {
-      this.invalidPort_ = true;
+    this.portState_ = this.computePortState_();
+    if (!this.portState_ == PortState.VALID) {
       return;
     }
-    const portNumber = +this.portNumberInput.value;
-    const portLabel = this.portLabelInput.value;
+    const portNumber = +this.$.portNumberInput.value;
+    const portLabel = this.$.portLabelInput.value;
     this.invalidPort_ = false;
     settings.CrostiniBrowserProxyImpl.getInstance()
         .addCrostiniPortForward(
             DEFAULT_CROSTINI_VM, DEFAULT_CROSTINI_CONTAINER, portNumber,
-            this.inputProtocolIndex_, portLabel)
+            /** @type {!CrostiniPortProtocol} */ (this.inputProtocolIndex_),
+            portLabel)
         .then(result => {
           // TODO(crbug.com/848127): Error handling for result
           this.$.dialog.close();

chrome/browser/ui/webui/settings/chromeos/crostini_section.cc

@@ -326,6 +326,8 @@ void CrostiniSection::AddLoadTimeData(content::WebUIDataSource* html_source) {
       {"crostiniPortForwardingUDP", IDS_SETTINGS_CROSTINI_PORT_FORWARDING_UDP},
       {"crostiniPortForwardingAddError",
        IDS_SETTINGS_CROSTINI_PORT_FORWARDING_ADD_ERROR},
+      {"crostiniPortForwardingAddExisting",
+       IDS_SETTINGS_CROSTINI_PORT_FORWARDING_ADD_EXISTING},
       {"crostiniPortForwardingRemoveAllPorts",
        IDS_SETTINGS_CROSTINI_PORT_FORWARDING_REMOVE_ALL_PORTS},
       {"crostiniPortForwardingRemovePort",

chrome/test/data/webui/settings/chromeos/crostini_page_test.js

@@ -418,7 +418,7 @@ suite('CrostiniPageTests', function() {
         assertTrue(!!subpage);
       });
 
-      test('Forwarded ports are shown', function() {
+      test('DisplayPorts', function() {
         // Extra list item for the titles.
         assertEquals(
             3, subpage.shadowRoot.querySelectorAll('.list-item').length);
@@ -432,7 +432,7 @@ suite('CrostiniPageTests', function() {
         await flushAsync();
         subpage = subpage.$$('settings-crostini-add-port-dialog');
         const portNumberInput = subpage.$$('#portNumberInput');
-        portNumberInput.value = '5000';
+        portNumberInput.value = '5002';
         const portLabelInput = subpage.$$('#portLabelInput');
         portLabelInput.value = 'Some Label';
         subpage.$$('cr-dialog cr-button[id="continue"]').click();
@@ -447,6 +447,8 @@ suite('CrostiniPageTests', function() {
 
         await flushAsync();
         subpage = subpage.$$('settings-crostini-add-port-dialog');
+        const errorText = subpage.$$('#errorText');
+        assertEquals(errorText.innerText, '');
         const portNumberInput = subpage.$$('#portNumberInput');
         portNumberInput.value = 'INVALID_PORT_NUMBER';
         const portLabelInput = subpage.$$('#portLabelInput');
@@ -454,14 +456,23 @@ suite('CrostiniPageTests', function() {
         subpage.$$('cr-dialog cr-button[id="continue"]').click();
         assertEquals(
             0, crostiniBrowserProxy.getCallCount('addCrostiniPortForward'));
+        assertEquals(
+            errorText.innerText,
+            loadTimeData.getString('crostiniPortForwardingAddError'));
         portNumberInput.value = '65536';
         subpage.$$('cr-dialog cr-button[id="continue"]').click();
         assertEquals(
             0, crostiniBrowserProxy.getCallCount('addCrostiniPortForward'));
-        portNumberInput.value = '65535';
+        assertEquals(
+            errorText.innerText,
+            loadTimeData.getString('crostiniPortForwardingAddError'));
+        portNumberInput.value = '5000';
         subpage.$$('cr-dialog cr-button[id="continue"]').click();
         assertEquals(
-            1, crostiniBrowserProxy.getCallCount('addCrostiniPortForward'));
+            0, crostiniBrowserProxy.getCallCount('addCrostiniPortForward'));
+        assertEquals(
+            errorText.innerText,
+            loadTimeData.getString('crostiniPortForwardingAddExisting'));
       });
 
       test('AddPortCancel', async function() {
@@ -568,6 +579,49 @@ suite('CrostiniPageTests', function() {
         assertFalse(crToggle.checked);
       });
 
+      test('PortPrefsChange', async function() {
+        setCrostiniPrefs(true, {
+          forwardedPorts: [
+            {
+              port_number: 5000,
+              protocol_type: 0,
+              label: 'Label1',
+            },
+            {
+              port_number: 5001,
+              protocol_type: 0,
+              label: 'Label1',
+            },
+          ]
+        });
+        assertEquals(
+            3, subpage.shadowRoot.querySelectorAll('.list-item').length);
+        setCrostiniPrefs(true, {
+          forwardedPorts: [
+            {
+              port_number: 5000,
+              protocol_type: 0,
+              label: 'Label1',
+            },
+            {
+              port_number: 5001,
+              protocol_type: 0,
+              label: 'Label1',
+            },
+            {
+              port_number: 5002,
+              protocol_type: 0,
+              label: 'Label1',
+            },
+          ]
+        });
+        assertEquals(
+            4, subpage.shadowRoot.querySelectorAll('.list-item').length);
+        setCrostiniPrefs(true, {forwardedPorts: []});
+        assertEquals(
+            0, subpage.shadowRoot.querySelectorAll('.list-item').length);
+      });
+
       test('CrostiniStopAndStart', function() {
         const crToggle = subpage.$$('#toggleActivationButton0');
         assertFalse(crToggle.disabled);