Disconnect all users if too many connection requests are received for It2Me

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 33 matching lines @@
 namespace remoting {
 
 namespace {
 
 // This is used for tagging system event logs.
 const char kApplicationName[] = "chromoting";
 const int kMaxLoginAttempts = 5;
 
 using protocol::ValidatingAuthenticator;
 typedef ValidatingAuthenticator::Result ValidationResult;
+typedef ValidatingAuthenticator::ResultCallback ValidationResultCallback;
 typedef ValidatingAuthenticator::ValidationCallback ValidationCallback;
 
+bool GetUsernameFromJid(const std::string& remote_jid,

[Sergey Ulanov, 2017/03/15 22:25:33]

Maybe use base::Optional<std::string> for the result and remove client_username
argument?

[joedow, 2017/03/16 21:32:18]

Acknowledged.

+                        std::string* client_username) {
+  DCHECK(client_username);
+  if (!SplitJidResource(remote_jid, client_username, /*resource=*/nullptr)) {
+    LOG(ERROR) << "Malformed jid: '" << remote_jid << "'";
+    return false;
+  }
+
+  if (client_username->empty()) {
+    LOG(ERROR) << "Malformed jid, missing username: " << remote_jid;
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace
 
 It2MeHost::It2MeHost(
     std::unique_ptr<ChromotingHostContext> host_context,
     std::unique_ptr<PolicyWatcher> policy_watcher,
     std::unique_ptr<It2MeConfirmationDialog> confirmation_dialog,
     base::WeakPtr<It2MeHost::Observer> observer,
     std::unique_ptr<SignalStrategy> signal_strategy,
     const std::string& username,
     const std::string& directory_bot_jid)
@@ skipping 224 matching lines @@
 
 void It2MeHost::SetPolicyForTesting(
     std::unique_ptr<base::DictionaryValue> policies,
     const base::Closure& done_callback) {
   host_context_->network_task_runner()->PostTaskAndReply(
       FROM_HERE,
       base::Bind(&It2MeHost::OnPolicyUpdate, this, base::Passed(&policies)),
       done_callback);
 }
 
-ValidationCallback It2MeHost::GetValidationCallbackForTesting() {
+ValidationCallback It2MeHost::GetIncomingConnectionCallbackForTesting() {
   return base::Bind(&It2MeHost::ValidateConnectionDetails,
                     base::Unretained(this));
 }
 
+ValidationCallback It2MeHost::GetAcceptedConnectionCallbackForTesting() {
+  return base::Bind(&It2MeHost::ShowConfirmationDialog, base::Unretained(this));
+}
+
 void It2MeHost::OnPolicyUpdate(
     std::unique_ptr<base::DictionaryValue> policies) {
   // The policy watcher runs on the |ui_task_runner|.
   if (!host_context_->network_task_runner()->BelongsToCurrentThread()) {
     host_context_->network_task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&It2MeHost::OnPolicyUpdate, this, base::Passed(&policies)));
     return;
   }
 
@@ skipping 54 matching lines @@
   }
 
   required_host_domain_ = host_domain;
 }
 
 void It2MeHost::UpdateClientDomainPolicy(const std::string& client_domain) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   VLOG(2) << "UpdateClientDomainPolicy: " << client_domain;
 
-  // When setting a client  domain policy, disconnect any existing session.
+  // When setting a client domain policy, disconnect any existing session.
   if (!client_domain.empty() && IsRunning()) {
     DisconnectOnNetworkThread();
   }
 
   required_client_domain_ = client_domain;
 }
 
 void It2MeHost::SetState(It2MeHostState state,
                          const std::string& error_message) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
@@ skipping 72 matching lines @@
     LOG(ERROR) << error_message;
     SetState(kError, error_message);
     DisconnectOnNetworkThread();
     return;
   }
 
   std::unique_ptr<protocol::AuthenticatorFactory> factory(
       new protocol::It2MeHostAuthenticatorFactory(
           local_certificate, host_key_pair_, access_code_hash,
           base::Bind(&It2MeHost::ValidateConnectionDetails,
+                     base::Unretained(this)),
+          base::Bind(&It2MeHost::ShowConfirmationDialog,
                      base::Unretained(this))));
   host_->SetAuthenticatorFactory(std::move(factory));
 
   // Pass the Access Code to the script object before changing state.
   host_context_->ui_task_runner()->PostTask(
       FROM_HERE, base::Bind(&It2MeHost::Observer::OnStoreAccessCode, observer_,
                             access_code, lifetime));
 
   SetState(kReceivedAccessCode, "");
 }
 
 void It2MeHost::ValidateConnectionDetails(
     const std::string& remote_jid,
-    const protocol::ValidatingAuthenticator::ResultCallback& result_callback) {
+    const ValidationResultCallback& result_callback) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   // First ensure the JID we received is valid.
   std::string client_username;
-  if (!SplitJidResource(remote_jid, &client_username, /*resource=*/nullptr)) {
-    LOG(ERROR) << "Rejecting incoming connection from " << remote_jid
-               << ": Invalid JID.";
-    result_callback.Run(
-        protocol::ValidatingAuthenticator::Result::ERROR_INVALID_ACCOUNT);
+  if (!GetUsernameFromJid(remote_jid, &client_username)) {
+    result_callback.Run(ValidationResult::ERROR_INVALID_ACCOUNT);
     DisconnectOnNetworkThread();
     return;
   }
-
-  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.

[Sergey Ulanov, 2017/03/15 22:25:33]

Does this check need to be separate from ShowConfirmationDialog()? I.e. Could we
authenticate connection using the access code and check the domain policy after
that? That would make ValidatingAuthenticator simpler.

[joedow, 2017/03/16 21:32:18]

Done.

   if (!required_client_domain_.empty()) {
     if (!base::EndsWith(client_username,
                         std::string("@") + required_client_domain_,
                         base::CompareCase::INSENSITIVE_ASCII)) {
       LOG(ERROR) << "Rejecting incoming connection from " << remote_jid
                  << ": Domain mismatch.";
       result_callback.Run(ValidationResult::ERROR_INVALID_ACCOUNT);
       DisconnectOnNetworkThread();
       return;
     }
   }
 
+  result_callback.Run(ValidationResult::SUCCESS);
+}
+
+void It2MeHost::ShowConfirmationDialog(
+    const std::string& remote_jid,
+    const ValidationResultCallback& result_callback) {
+  std::string client_username;
+  if (!GetUsernameFromJid(remote_jid, &client_username)) {
+    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) {
+    LOG(ERROR) << "Received too many connection requests.";

[Sergey Ulanov, 2017/03/15 22:25:33]

Can we DCHECK here that state_ is kConnecting? I don't think it can be anything
else here.

[joedow, 2017/03/16 21:32:18]

Done.

+    result_callback.Run(ValidationResult::ERROR_TOO_MANY_CONNECTIONS);
+    DisconnectOnNetworkThread();
+    return;
+  }
+
   HOST_LOG << "Client " << client_username << " connecting.";
   SetState(kConnecting, std::string());
 
   // Show a confirmation dialog to the user to allow them to confirm/reject it.
   confirmation_dialog_proxy_.reset(new It2MeConfirmationDialogProxy(
       host_context_->ui_task_runner(), std::move(confirmation_dialog_)));
 
   confirmation_dialog_proxy_->Show(
       client_username, base::Bind(&It2MeHost::OnConfirmationResult,
                                   base::Unretained(this), result_callback));
 }
 
 void It2MeHost::OnConfirmationResult(
-    const protocol::ValidatingAuthenticator::ResultCallback& result_callback,
+    const ValidationResultCallback& result_callback,
     It2MeConfirmationDialog::Result result) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   switch (result) {
     case It2MeConfirmationDialog::Result::OK:
       result_callback.Run(ValidationResult::SUCCESS);
       break;
 
     case It2MeConfirmationDialog::Result::CANCEL:
       result_callback.Run(ValidationResult::ERROR_REJECTED_BY_USER);
@@ skipping 16 matching lines @@
   DCHECK(context->ui_task_runner()->BelongsToCurrentThread());
 
   std::unique_ptr<PolicyWatcher> policy_watcher =
       PolicyWatcher::Create(policy_service, context->file_task_runner());
   return new It2MeHost(std::move(context), std::move(policy_watcher),
                        It2MeConfirmationDialog::Create(), observer,
                        std::move(signal_strategy), username, directory_bot_jid);
 }
 
 }  // namespace remoting