Add support for multiple allowed domains

remoting/host/it2me/it2me_host.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/it2me/it2me_host.h"
 
 #include <cstdint>
 #include <memory>
 #include <string>
 #include <utility>
@@ skipping 156 matching lines @@
 
 void It2MeHost::FinishConnect() {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   if (state_ != kStarting) {
     // Host has been stopped while we were fetching policy.
     return;
   }
 
   // Check the host domain policy.
-  if (!required_host_domain_.empty() &&
-      !base::EndsWith(username_, std::string("@") + required_host_domain_,
-                      base::CompareCase::INSENSITIVE_ASCII)) {
-    SetState(kInvalidDomainError, "");
-    return;
+  if (!required_host_domain_list_.empty()) {
+    bool matched = false;
+    for (const std::string& domain : required_host_domain_list_) {

[Jamie, 2017/04/19 00:29:02]

Can this be "const auto&"?

[rkjnsn, 2017/04/19 16:45:26]

If you think that's more readable.

[Jamie, 2017/04/19 16:55:47]

Being explicit about the type doesn't add to readability, and I think auto is
encouraged where applicable. I don't feel strongly about it though.

[rkjnsn, 2017/04/19 18:24:06]

The style guide says it is "permitted when it increases readability", and
especially should be used to replace long, cluttery types (like iterators) and
when iterating over a map. Otherwise, it seems to leave it up to the author's
and reviewer's discretion. I'll go ahead and use auto here.

+      if (base::EndsWith(username_, std::string("@") + domain,
+                         base::CompareCase::INSENSITIVE_ASCII)) {
+        matched = true;
+        break;
+      }
+    }
+    if (!matched) {
+      SetState(kInvalidDomainError, "");
+      return;
+    }
   }
 
   // Generate a key pair for the Host to use.
   // TODO(wez): Move this to the worker thread.
   host_key_pair_ = RsaKeyPair::Generate();
 
   // Request registration of the host for support.
   std::unique_ptr<RegisterSupportHostRequest> register_request(
       new RegisterSupportHostRequest(
           signal_strategy_.get(), host_key_pair_, directory_bot_jid_,
@@ skipping 115 matching lines @@
         FROM_HERE,
         base::Bind(&It2MeHost::OnPolicyUpdate, this, base::Passed(&policies)));
     return;
   }
 
   bool nat_policy;
   if (policies->GetBoolean(policy::key::kRemoteAccessHostFirewallTraversal,
                            &nat_policy)) {
     UpdateNatPolicy(nat_policy);
   }
-  std::string host_domain;
-  if (policies->GetString(policy::key::kRemoteAccessHostDomain, &host_domain)) {
-    UpdateHostDomainPolicy(host_domain);
+  const base::ListValue* host_domain_list;
+  if (policies->GetList(policy::key::kRemoteAccessHostDomainList,
+                        &host_domain_list)) {
+    std::vector<std::string> host_domain_list_vector;
+    for (const auto& value : *host_domain_list) {
+      host_domain_list_vector.push_back(value.GetString());
+    }
+    UpdateHostDomainListPolicy(std::move(host_domain_list_vector));

[Jamie, 2017/04/19 00:29:02]

Don't need move() if you pass by const reference.

[rkjnsn, 2017/04/19 18:24:06]

True, but that would guarantee that that vector would need to be copied by
UpdateHostDomainListPolicy, while here we can just pass it ownership of our
vector, which we don't need anymore. In general, the idea is that taking an
object by value when one wants to take ownership means the caller gets to decide
if they want to transfer ownership of an existing object or make a copy, while
taking a constant reference means that the callee must make a copy, even if the
caller was done with the object.

I found some discussion of this on the Chromium mailing list, but it didn't seem
to come to any conclusion. I'm happy to do whatever the team prefers, since
there isn't any real performance consideration here (and there probably isn't in
most cases).

(I've had this discussion once before, but it was in regards to strings, which
*do* have a codified convention for passing by const ref, so I wasn't sure if
that would apply here with a different type.)

[Jamie, 2017/04/19 19:21:06]

Thanks for the detailed follow-up. I am happy for you to leave these as
pass-by-value if no-one else objects.

   }
-  std::string client_domain;
-  if (policies->GetString(policy::key::kRemoteAccessHostClientDomain,
-                          &client_domain)) {
-    UpdateClientDomainPolicy(client_domain);
+  const base::ListValue* client_domain_list;
+  if (policies->GetList(policy::key::kRemoteAccessHostClientDomainList,
+                        &client_domain_list)) {
+    std::vector<std::string> client_domain_list_vector;
+    for (const auto& value : *client_domain_list) {
+      client_domain_list_vector.push_back(value.GetString());
+    }
+    UpdateClientDomainListPolicy(std::move(client_domain_list_vector));
   }
 
   policy_received_ = true;
 
   if (!pending_connect_.is_null()) {
     base::ResetAndReturn(&pending_connect_).Run();
   }
 }
 
 void It2MeHost::OnPolicyError() {
@@ skipping 13 matching lines @@
   }
 
   nat_traversal_enabled_ = nat_traversal_enabled;
 
   // Notify the web-app of the policy setting.
   host_context_->ui_task_runner()->PostTask(
       FROM_HERE, base::Bind(&It2MeHost::Observer::OnNatPolicyChanged, observer_,
                             nat_traversal_enabled_));
 }
 
-void It2MeHost::UpdateHostDomainPolicy(const std::string& host_domain) {
+void It2MeHost::UpdateHostDomainListPolicy(
+    std::vector<std::string> host_domain_list) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
-  VLOG(2) << "UpdateHostDomainPolicy: " << host_domain;
+  VLOG(2) << "UpdateHostDomainListPolicy: "
+          << base::JoinString(host_domain_list, ", ");
 
   // When setting a host domain policy, force disconnect any existing session.
-  if (!host_domain.empty() && IsRunning()) {
+  if (!host_domain_list.empty() && IsRunning()) {
     DisconnectOnNetworkThread();
   }
 
-  required_host_domain_ = host_domain;
+  required_host_domain_list_ = std::move(host_domain_list);
 }
 
-void It2MeHost::UpdateClientDomainPolicy(const std::string& client_domain) {
+void It2MeHost::UpdateClientDomainListPolicy(
+    std::vector<std::string> client_domain_list) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
-  VLOG(2) << "UpdateClientDomainPolicy: " << client_domain;
+  VLOG(2) << "UpdateClientDomainPolicy: "
+          << base::JoinString(client_domain_list, ", ");
 
   // When setting a client  domain policy, disconnect any existing session.
-  if (!client_domain.empty() && IsRunning()) {
+  if (!client_domain_list.empty() && IsRunning()) {
     DisconnectOnNetworkThread();
   }
 
-  required_client_domain_ = client_domain;
+  required_client_domain_list_ = client_domain_list;
 }
 
 void It2MeHost::SetState(It2MeHostState state,
                          const std::string& error_message) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   switch (state_) {
     case kDisconnected:
       DCHECK(state == kStarting ||
              state == kError) << state;
@@ skipping 103 matching lines @@
 
   if (client_username.empty()) {
     LOG(ERROR) << "Invalid user name passed in: " << remote_jid;
     result_callback.Run(
         protocol::ValidatingAuthenticator::Result::ERROR_INVALID_ACCOUNT);
     DisconnectOnNetworkThread();
     return;
   }
 
   // Check the client domain policy.
-  if (!required_client_domain_.empty()) {
-    if (!base::EndsWith(client_username,
-                        std::string("@") + required_client_domain_,
-                        base::CompareCase::INSENSITIVE_ASCII)) {
+  if (!required_client_domain_list_.empty()) {
+    bool matched = false;
+    for (const std::string& domain : required_client_domain_list_) {

[Jamie, 2017/04/19 00:29:02]

auto?

+      if (base::EndsWith(client_username, std::string("@") + domain,
+                         base::CompareCase::INSENSITIVE_ASCII)) {
+        matched = true;
+        break;
+      }
+    }
+    if (!matched) {
       LOG(ERROR) << "Rejecting incoming connection from " << remote_jid
-                 << ": Domain mismatch.";
+                 << ": Domain not allowed.";
       result_callback.Run(ValidationResult::ERROR_INVALID_ACCOUNT);
       DisconnectOnNetworkThread();
       return;
     }
   }
 
   // If we receive valid connection details multiple times, then we don't know
   // which remote user (if either) is valid so disconnect everyone.
   if (state_ != kReceivedAccessCode) {
     DCHECK_EQ(kConnecting, state_);
@@ skipping 49 matching lines @@
 
   std::unique_ptr<PolicyWatcher> policy_watcher =
       PolicyWatcher::Create(policy_service, context->file_task_runner());
   return new It2MeHost(std::move(context), std::move(policy_watcher),
                        base::MakeUnique<It2MeConfirmationDialogFactory>(),
                        observer, std::move(signal_strategy), username,
                        directory_bot_jid);
 }
 
 }  // namespace remoting