Update It2MeHost to use RemoteAccessHostAllowRelayedConnection policy

This CL adds some logic to the It2MeHost to read this policy and update
the network settings to reflect its value.  The network settings are
used to discard the entries in the ICE Config which are blocked by
policy.  With this change, the Me2MeHost and It2MeHost will now have
the same behavior (previously the It2MeHost only read the NatTraversal
policy).

This should be a safe change as the AllowRelayedConnection policy
defaults to true if not explicitly set and the description of the policy
does not indicate that it only applies to Me2Me.  Another interesting
note is that we do not refer to it in our Enterprise config doc (only
the firewall traversal policy is mentioned) so I would be surprised
if it is broadly used.

Once this lands, I'll update the website to use the new
'relayConnectionsAllowed' message value.

Change-Id: Ide4009e5e7979f7bda6dc098e5968739ed50e97e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2353931
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797935}

remoting/host/it2me/it2me_host.cc

@@ -169,12 +169,19 @@ void It2MeHost::ConnectOnNetworkThread(
       signal_strategy_.get(), host_key_pair_,
       base::BindOnce(&It2MeHost::OnReceivedSupportID, base::Unretained(this)));
 
-  HOST_LOG << "NAT state: " << nat_traversal_enabled_;
+  HOST_LOG << "NAT traversal enabled: " << nat_traversal_enabled_;
+  HOST_LOG << "Relay connections allowed: " << relay_connections_allowed_;
 
-  protocol::NetworkSettings network_settings(
-     nat_traversal_enabled_ ?
-     protocol::NetworkSettings::NAT_TRAVERSAL_FULL :
-     protocol::NetworkSettings::NAT_TRAVERSAL_DISABLED);
+  uint32_t network_flags = protocol::NetworkSettings::NAT_TRAVERSAL_DISABLED;
+  if (nat_traversal_enabled_) {
+    network_flags = protocol::NetworkSettings::NAT_TRAVERSAL_STUN |
+                    protocol::NetworkSettings::NAT_TRAVERSAL_OUTGOING;
+    if (relay_connections_allowed_) {
+      network_flags |= protocol::NetworkSettings::NAT_TRAVERSAL_RELAY;
+    }
+  }
+
+  protocol::NetworkSettings network_settings(network_flags);
 
   if (!udp_port_range_.is_null()) {
     network_settings.port_range = udp_port_range_;
@@ -289,11 +296,21 @@ void It2MeHost::OnPolicyUpdate(
     return;
   }
 
-  bool nat_policy;
-  if (policies->GetBoolean(policy::key::kRemoteAccessHostFirewallTraversal,
-                           &nat_policy)) {
-    UpdateNatPolicy(nat_policy);
+  bool nat_policy_value = false;
+  if (!policies->GetBoolean(policy::key::kRemoteAccessHostFirewallTraversal,
+                            &nat_policy_value)) {
+    HOST_LOG << "Failed to read kRemoteAccessHostFirewallTraversal policy";
+    nat_policy_value = nat_traversal_enabled_;
   }
+  bool relay_policy_value = false;
+  if (!policies->GetBoolean(
+          policy::key::kRemoteAccessHostAllowRelayedConnection,
+          &relay_policy_value)) {
+    HOST_LOG << "Failed to read kRemoteAccessHostAllowRelayedConnection policy";
+    relay_policy_value = relay_connections_allowed_;
+  }
+  UpdateNatPolicies(nat_policy_value, relay_policy_value);
+
   const base::ListValue* host_domain_list;
   if (policies->GetList(policy::key::kRemoteAccessHostDomainList,
                         &host_domain_list)) {
@@ -303,6 +320,7 @@ void It2MeHost::OnPolicyUpdate(
     }
     UpdateHostDomainListPolicy(std::move(host_domain_list_vector));
   }
+
   const base::ListValue* client_domain_list;
   if (policies->GetList(policy::key::kRemoteAccessHostClientDomainList,
                         &client_domain_list)) {
@@ -320,23 +338,30 @@ void It2MeHost::OnPolicyUpdate(
   }
 }
 
-void It2MeHost::UpdateNatPolicy(bool nat_traversal_enabled) {
+void It2MeHost::UpdateNatPolicies(bool nat_policy_value,
+                                  bool relay_policy_value) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
-  VLOG(2) << "UpdateNatPolicy: " << nat_traversal_enabled;
+  VLOG(2) << "UpdateNatPolicies: nat_policy_value: " << nat_policy_value;
+  bool nat_traversal_value_changed = nat_traversal_enabled_ != nat_policy_value;
+  nat_traversal_enabled_ = nat_policy_value;
+
+  VLOG(2) << "UpdateNatPolicies: relay_policy_value: " << relay_policy_value;
+  bool relay_value_changed = relay_connections_allowed_ != relay_policy_value;
+  relay_connections_allowed_ = relay_policy_value;
 
-  // When transitioning from enabled to disabled, force disconnect any
-  // existing session.
-  if (nat_traversal_enabled_ && !nat_traversal_enabled && IsRunning()) {
+  // Force disconnect when transitioning either policy setting to disabled.
+  if (((nat_traversal_value_changed && !nat_traversal_enabled_) ||
+       (relay_value_changed && !relay_connections_allowed_)) &&
+      IsRunning()) {
     DisconnectOnNetworkThread();
   }
 
-  nat_traversal_enabled_ = nat_traversal_enabled;
-
-  // Notify the web-app of the policy setting.
+  // Notify listeners of the policy setting change.
   host_context_->ui_task_runner()->PostTask(
-      FROM_HERE, base::BindOnce(&It2MeHost::Observer::OnNatPolicyChanged,
-                                observer_, nat_traversal_enabled_));
+      FROM_HERE,
+      base::BindOnce(&It2MeHost::Observer::OnNatPoliciesChanged, observer_,
+                     nat_traversal_enabled_, relay_connections_allowed_));
 }
 
 void It2MeHost::UpdateHostDomainListPolicy(

remoting/host/it2me/it2me_host.h

@@ -76,7 +76,8 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
     virtual void OnClientAuthenticated(const std::string& client_username) = 0;
     virtual void OnStoreAccessCode(const std::string& access_code,
                                    base::TimeDelta access_code_lifetime) = 0;
-    virtual void OnNatPolicyChanged(bool nat_traversal_enabled) = 0;
+    virtual void OnNatPoliciesChanged(bool nat_traversal_enabled,
+                                      bool relay_connections_allowed) = 0;
     virtual void OnStateChanged(It2MeHostState state,
                                 protocol::ErrorCode error_code) = 0;
   };
@@ -167,7 +168,7 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
                            protocol::ErrorCode error_code);
 
   // Handlers for NAT traversal and domain policies.
-  void UpdateNatPolicy(bool nat_traversal_enabled);
+  void UpdateNatPolicies(bool nat_policy_value, bool relay_policy_value);
   void UpdateHostDomainListPolicy(std::vector<std::string> host_domain_list);
   void UpdateClientDomainListPolicy(
       std::vector<std::string> client_domain_list);
@@ -201,9 +202,12 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   std::unique_ptr<It2MeConfirmationDialogFactory> confirmation_dialog_factory_;
   std::unique_ptr<It2MeConfirmationDialogProxy> confirmation_dialog_proxy_;
 
-  // Host the current nat traversal policy setting.
+  // Stores the current nat traversal policy value.
   bool nat_traversal_enabled_ = false;
 
+  // Stores the current relay connections allowed policy value.
+  bool relay_connections_allowed_ = false;
+
   // The client and host domain policy setting.
   std::vector<std::string> required_client_domain_list_;
   std::vector<std::string> required_host_domain_list_;

remoting/host/it2me/it2me_host_unittest.cc

@@ -152,7 +152,8 @@ class It2MeHostTest : public testing::Test, public It2MeHost::Observer {
   void OnClientAuthenticated(const std::string& client_username) override;
   void OnStoreAccessCode(const std::string& access_code,
                          base::TimeDelta access_code_lifetime) override;
-  void OnNatPolicyChanged(bool nat_traversal_enabled) override;
+  void OnNatPoliciesChanged(bool nat_traversal_enabled,
+                            bool relay_connections_allowed) override;
   void OnStateChanged(It2MeHostState state, ErrorCode error_code) override;
 
   void SetPolicies(
@@ -171,6 +172,12 @@ class It2MeHostTest : public testing::Test, public It2MeHost::Observer {
 
   ChromotingHost* GetHost() { return it2me_host_->host_.get(); }
 
+  // Stores the last nat traversal policy value received.
+  bool last_nat_traversal_enabled_value_ = false;
+
+  // Stores the last relay enabled policy value received.
+  bool last_relay_connections_allowed_value_ = false;
+
   ValidationResult validation_result_ = ValidationResult::SUCCESS;
 
   base::OnceClosure state_change_callback_;
@@ -353,7 +360,11 @@ void It2MeHostTest::OnClientAuthenticated(const std::string& client_username) {}
 void It2MeHostTest::OnStoreAccessCode(const std::string& access_code,
                                       base::TimeDelta access_code_lifetime) {}
 
-void It2MeHostTest::OnNatPolicyChanged(bool nat_traversal_enabled) {}
+void It2MeHostTest::OnNatPoliciesChanged(bool nat_traversal_enabled,
+                                         bool relay_connections_allowed) {
+  last_nat_traversal_enabled_value_ = nat_traversal_enabled;
+  last_relay_connections_allowed_value_ = relay_connections_allowed;
+}
 
 void It2MeHostTest::OnStateChanged(It2MeHostState state, ErrorCode error_code) {
   last_host_state_ = state;
@@ -408,6 +419,94 @@ TEST_F(It2MeHostTest, IceConfig) {
   ASSERT_EQ(It2MeHostState::kDisconnected, last_host_state_);
 }
 
+TEST_F(It2MeHostTest, NatTraversalPolicy_Enabled) {
+  SetPolicies(
+      {{policy::key::kRemoteAccessHostFirewallTraversal, base::Value(true)}});
+
+  StartHost();
+  ASSERT_EQ(It2MeHostState::kReceivedAccessCode, last_host_state_);
+
+  EXPECT_TRUE(last_nat_traversal_enabled_value_);
+
+  ShutdownHost();
+  ASSERT_EQ(It2MeHostState::kDisconnected, last_host_state_);
+}
+
+TEST_F(It2MeHostTest, NatTraversalPolicy_Disabled) {
+  SetPolicies(
+      {{policy::key::kRemoteAccessHostFirewallTraversal, base::Value(false)}});
+
+  StartHost();
+  ASSERT_EQ(It2MeHostState::kReceivedAccessCode, last_host_state_);
+
+  EXPECT_FALSE(last_nat_traversal_enabled_value_);
+
+  ShutdownHost();
+  ASSERT_EQ(It2MeHostState::kDisconnected, last_host_state_);
+}
+
+TEST_F(It2MeHostTest, NatTraversalPolicy_DisabledTransitionCausesDisconnect) {
+  StartHost();
+  ASSERT_EQ(It2MeHostState::kReceivedAccessCode, last_host_state_);
+
+  EXPECT_TRUE(last_nat_traversal_enabled_value_);
+  EXPECT_TRUE(last_relay_connections_allowed_value_);
+
+  SetPolicies(
+      {{policy::key::kRemoteAccessHostFirewallTraversal, base::Value(false)},
+       {policy::key::kRemoteAccessHostAllowRelayedConnection,
+        base::Value(true)}});
+  RunUntilStateChanged(It2MeHostState::kDisconnected);
+
+  EXPECT_FALSE(last_nat_traversal_enabled_value_);
+  EXPECT_TRUE(last_relay_connections_allowed_value_);
+  ASSERT_EQ(It2MeHostState::kDisconnected, last_host_state_);
+}
+
+TEST_F(It2MeHostTest, RelayPolicy_Enabled) {
+  SetPolicies({{policy::key::kRemoteAccessHostAllowRelayedConnection,
+                base::Value(true)}});
+
+  StartHost();
+  ASSERT_EQ(It2MeHostState::kReceivedAccessCode, last_host_state_);
+
+  EXPECT_TRUE(last_relay_connections_allowed_value_);
+
+  ShutdownHost();
+  ASSERT_EQ(It2MeHostState::kDisconnected, last_host_state_);
+}
+
+TEST_F(It2MeHostTest, RelayPolicy_Disabled) {
+  SetPolicies({{policy::key::kRemoteAccessHostAllowRelayedConnection,
+                base::Value(false)}});
+
+  StartHost();
+  ASSERT_EQ(It2MeHostState::kReceivedAccessCode, last_host_state_);
+
+  EXPECT_FALSE(last_relay_connections_allowed_value_);
+
+  ShutdownHost();
+  ASSERT_EQ(It2MeHostState::kDisconnected, last_host_state_);
+}
+
+TEST_F(It2MeHostTest, RelayPolicy_DisabledTransitionCausesDisconnect) {
+  StartHost();
+  ASSERT_EQ(It2MeHostState::kReceivedAccessCode, last_host_state_);
+
+  EXPECT_TRUE(last_nat_traversal_enabled_value_);
+  EXPECT_TRUE(last_relay_connections_allowed_value_);
+
+  SetPolicies(
+      {{policy::key::kRemoteAccessHostFirewallTraversal, base::Value(true)},
+       {policy::key::kRemoteAccessHostAllowRelayedConnection,
+        base::Value(false)}});
+  RunUntilStateChanged(It2MeHostState::kDisconnected);
+
+  EXPECT_TRUE(last_nat_traversal_enabled_value_);
+  EXPECT_FALSE(last_relay_connections_allowed_value_);
+  ASSERT_EQ(It2MeHostState::kDisconnected, last_host_state_);
+}
+
 TEST_F(It2MeHostTest, HostValidation_HostDomainListPolicy_MatchingDomain) {
   SetPolicies({{policy::key::kRemoteAccessHostDomainList,
                 MakeList({kMatchingDomain})}});

remoting/host/it2me/it2me_native_messaging_host.cc

@@ -484,13 +484,16 @@ void It2MeNativeMessagingHost::SetPolicyErrorClosureForTesting(
   policy_error_closure_for_testing_ = std::move(closure);
 }
 
-void It2MeNativeMessagingHost::OnNatPolicyChanged(bool nat_traversal_enabled) {
+void It2MeNativeMessagingHost::OnNatPoliciesChanged(
+    bool nat_traversal_enabled,
+    bool relay_connections_allowed) {
   DCHECK(task_runner()->BelongsToCurrentThread());
 
   std::unique_ptr<base::DictionaryValue> message(new base::DictionaryValue());
 
   message->SetString("type", "natPolicyChanged");
   message->SetBoolean("natTraversalEnabled", nat_traversal_enabled);
+  message->SetBoolean("relayConnectionsAllowed", relay_connections_allowed);
   SendMessageToClient(std::move(message));
 }
 

remoting/host/it2me/it2me_native_messaging_host.h

@@ -61,7 +61,8 @@ class It2MeNativeMessagingHost : public It2MeHost::Observer,
       override;
   void OnStoreAccessCode(const std::string& access_code,
                                  base::TimeDelta access_code_lifetime) override;
-  void OnNatPolicyChanged(bool nat_traversal_enabled) override;
+  void OnNatPoliciesChanged(bool nat_traversal_enabled,
+                            bool relay_connections_allowed) override;
   void OnStateChanged(It2MeHostState state,
                       protocol::ErrorCode error_code) override;