Moving It2Me confirmation prompt into the Validation callback flow

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 81 matching lines @@
       host_context_->video_capture_task_runner(),
       host_context_->input_task_runner(), host_context_->ui_task_runner()));
 
   // Start monitoring configured policies.
   policy_watcher_->StartWatching(
       base::Bind(&It2MeHost::OnPolicyUpdate, this),
       base::Bind(&It2MeHost::OnPolicyError, this));
 
   // Switch to the network thread to start the actual connection.
   host_context_->network_task_runner()->PostTask(
-      FROM_HERE, base::Bind(&It2MeHost::ShowConfirmationPrompt, this));
+      FROM_HERE, base::Bind(&It2MeHost::SetState, this, kStarting, ""));

[Sergey Ulanov, 2016/09/09 18:50:40]

Call SetState() in ReadPolicyAndConnect()?

[joedow, 2016/09/10 02:50:36]

Done.

+
+  host_context_->network_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&It2MeHost::ReadPolicyAndConnect, this));
 }
 
 void It2MeHost::Disconnect() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   host_context_->network_task_runner()->PostTask(
       FROM_HERE, base::Bind(&It2MeHost::DisconnectOnNetworkThread, this));
 }
 
 void It2MeHost::DisconnectOnNetworkThread() {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
@@ skipping 30 matching lines @@
     DCHECK(task_runner_->BelongsToCurrentThread());
     host_context_->network_task_runner()->PostTask(
         FROM_HERE, base::Bind(&It2MeHost::RequestNatPolicy, this));
     return;
   }
 
   if (policy_received_)
     UpdateNatPolicy(nat_traversal_enabled_);
 }
 
-void It2MeHost::ShowConfirmationPrompt() {
-  DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
-
-  SetState(kStarting, "");
-
-  std::unique_ptr<It2MeConfirmationDialog> confirmation_dialog =
-      confirmation_dialog_factory_->Create();
-
-  // TODO(dcaiafa): Remove after dialog implementations for all platforms exist.
-  if (!confirmation_dialog) {
-    ReadPolicyAndConnect();
-    return;
-  }
-
-  confirmation_dialog_proxy_.reset(
-      new It2MeConfirmationDialogProxy(host_context_->ui_task_runner(),
-                                       std::move(confirmation_dialog)));
-
-  confirmation_dialog_proxy_->Show(
-      base::Bind(&It2MeHost::OnConfirmationResult, base::Unretained(this)));
-}
-
-void It2MeHost::OnConfirmationResult(It2MeConfirmationDialog::Result result) {
-  switch (result) {
-    case It2MeConfirmationDialog::Result::OK:
-      ReadPolicyAndConnect();
-      break;
-
-    case It2MeConfirmationDialog::Result::CANCEL:
-      DisconnectOnNetworkThread();
-      break;
-
-    default:
-      NOTREACHED();
-      return;
-  }
-}
-
 void It2MeHost::ReadPolicyAndConnect() {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
   DCHECK_EQ(kStarting, state_);
 
   // Only proceed to FinishConnect() if at least one policy update has been
-  // received.
+  // received.  Otherwise, create the policy watcher and thunk the connect.
   if (policy_received_) {
     FinishConnect();
   } else {
-    // Otherwise, create the policy watcher, and thunk the connect.
-    pending_connect_ =
-        base::Bind(&It2MeHost::FinishConnect, this);
+    pending_connect_ = base::Bind(&It2MeHost::FinishConnect, this);
   }
 }
 
 void It2MeHost::FinishConnect() {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   if (state_ != kStarting) {
     // Host has been stopped while we were fetching policy.
     return;
   }
@@ skipping 91 matching lines @@
   }
 }
 
 void It2MeHost::OnClientConnected(const std::string& jid) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   // ChromotingHost doesn't allow multiple concurrent connection and the
   // host is destroyed in OnClientDisconnected() after the first connection.
   CHECK_NE(state_, kConnected);
 
-  std::string client_username = jid;
-  size_t pos = client_username.find('/');
-  if (pos != std::string::npos)
-    client_username.replace(pos, std::string::npos, "");
+  std::string client_username;
+  if (!SplitJidResource(jid, &client_username, /*resource=*/nullptr)) {
+    LOG(WARNING) << "Incorrectly formatted JID received: " << jid;
+    client_username = jid;
+  }
 
   HOST_LOG << "Client " << client_username << " connected.";
 
   // Pass the client user name to the script object before changing state.
   task_runner_->PostTask(
       FROM_HERE, base::Bind(&It2MeHost::Observer::OnClientAuthenticated,
                             observer_, client_username));
 
   SetState(kConnected, "");
 }
@@ skipping 187 matching lines @@
   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) {
+  // 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);
+    return;
+  }
+
   // Check the client domain policy.
   if (!required_client_domain_.empty()) {
-    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(ValidationResult::ERROR_INVALID_ACCOUNT);
-      return;
-    }
     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);
       return;
     }
   }
 
-  result_callback.Run(ValidationResult::SUCCESS);
+  // Show a confirmation dialog to the user to allow them to confirm/reject it.
+  std::unique_ptr<It2MeConfirmationDialog> confirmation_dialog =
+      confirmation_dialog_factory_->Create();
+
+  if (!confirmation_dialog) {

[Sergey Ulanov, 2016/09/09 18:46:57]

Add a comment that this is only temporary until we have the dialog implemented
on all platforms and add a TODO to fix this?

[joedow, 2016/09/10 02:50:36]

Done.

+    result_callback.Run(ValidationResult::SUCCESS);
+    return;
+  }
+
+  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,
+    It2MeConfirmationDialog::Result result) {
+  switch (result) {
+    case It2MeConfirmationDialog::Result::OK:
+      result_callback.Run(ValidationResult::SUCCESS);
+      break;
+
+    case It2MeConfirmationDialog::Result::CANCEL:
+      result_callback.Run(ValidationResult::ERROR_REJECTED_BY_USER);
+      break;
+  }
 }
 
 It2MeHostFactory::It2MeHostFactory() {}
 
 It2MeHostFactory::~It2MeHostFactory() {}
 
 scoped_refptr<It2MeHost> It2MeHostFactory::CreateIt2MeHost(
     std::unique_ptr<ChromotingHostContext> context,
     policy::PolicyService* policy_service,
     base::WeakPtr<It2MeHost::Observer> observer,
     const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
     const std::string& directory_bot_jid) {
   DCHECK(context->ui_task_runner()->BelongsToCurrentThread());
 
   std::unique_ptr<It2MeConfirmationDialogFactory> confirmation_dialog_factory(
       new It2MeConfirmationDialogFactory());
   std::unique_ptr<PolicyWatcher> policy_watcher =
       PolicyWatcher::Create(policy_service, context->file_task_runner());
   return new It2MeHost(std::move(context), std::move(policy_watcher),
                        std::move(confirmation_dialog_factory), observer,
                        xmpp_server_config, directory_bot_jid);
 }
 
 }  // namespace remoting