Host port range policy is no longer ignored in it2me host

remoting/host/it2me/it2me_host.cc

Index: remoting/host/it2me/it2me_host.cc
diff --git a/remoting/host/it2me/it2me_host.cc b/remoting/host/it2me/it2me_host.cc
index 17e7143541fc2d8f58256a01957bdd7da893b722..42345ec07e5ef2cddb3dd7dd29953fcc295fb592 100644
--- a/remoting/host/it2me/it2me_host.cc
+++ b/remoting/host/it2me/it2me_host.cc
@@ -191,12 +191,14 @@ void It2MeHost::FinishConnect() {
   // Beyond this point nothing can fail, so save the config and request.
   register_request_ = std::move(register_request);
 
-  // If NAT traversal is off then limit port range to allow firewall pin-holing.
-  HOST_LOG << "NAT state: " << nat_traversal_enabled_;
   protocol::NetworkSettings network_settings(
      nat_traversal_enabled_ ?
      protocol::NetworkSettings::NAT_TRAVERSAL_FULL :
      protocol::NetworkSettings::NAT_TRAVERSAL_DISABLED);
+  network_settings.port_range = udp_port_range_;
+
+  // If NAT traversal is off then limit port range to allow firewall pin-holing.
+  HOST_LOG << "NAT state: " << nat_traversal_enabled_;
   if (!nat_traversal_enabled_) {
     network_settings.port_range.min_port =

[Sergey Ulanov, 2017/05/25 22:24:38]

Setting port range here makes sense only if it isn't already set by the policy.
I.e.
  if (!udp_port_range_.is_null()) {
    network_settings.port_range = udp_port_range_;
  } else if (!nat_traversal_enabled_) {
    network_settings.port_range.min_port =
      protocol::NetworkSettings::kDefaultMinPort;
    network_settings.port_range.max_port =
      protocol::NetworkSettings::kDefaultMaxPort;    
  }

[Gus Smith, 2017/05/25 22:55:58]

Done. I don't think I fully understand what should take precedence over what,
though - I assumed that NAT traversal being disabled should force the port range
to the default port range to allow for firewall pin-holing as the comment
states. Is this not the case?

[Sergey Ulanov, 2017/05/26 00:40:08]

If an admin disables NAT traversal then they also have control of the port range
policy. They can set the policy to the default range if that's what they want.
This code was added long time ago when we didn't have PortRange policy. Now that
we have it we should respect it regardless of the NAT traversal state. This
behavior is consistent with the Me2Me host, see
https://codesearch.chromium.org/chromium/src/remoting/host/remoting_me2me_hos...

         protocol::NetworkSettings::kDefaultMinPort;
@@ -330,6 +332,12 @@ void It2MeHost::OnPolicyUpdate(
     UpdateClientDomainListPolicy(std::move(client_domain_list_vector));
   }
 
+  std::string port_range_string;
+  if (policies->GetString(policy::key::kRemoteAccessHostUdpPortRange,
+                          &port_range_string)) {
+    UpdateHostUdpPortRangePolicy(port_range_string);
+  }
+
   policy_received_ = true;
 
   if (!pending_connect_.is_null()) {
@@ -386,6 +394,22 @@ void It2MeHost::UpdateClientDomainListPolicy(
   required_client_domain_list_ = std::move(client_domain_list);
 }
 
+void It2MeHost::UpdateHostUdpPortRangePolicy(
+    const std::string& port_range_string) {
+  DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
+
+  VLOG(2) << "UpdateHostUdpPortRangePolicy: " << port_range_string;
+
+  if (IsRunning()) {
+    DisconnectOnNetworkThread();
+  }
+
+  if (!PortRange::Parse(port_range_string, &udp_port_range_)) {
+    // PolicyWatcher verifies that the value is formatted correctly.
+    LOG(FATAL) << "Invalid port range: " << port_range_string;
+  }
+}
+
 void It2MeHost::SetState(It2MeHostState state,
                          const std::string& error_message) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());