Updating It2Me to use the new ValidatingAuthenticator class

This change moves the connection validation logic for It2Me from the host
factory and into a callback in the It2Mehost class.

A subsequent change will move the confirmation dialog into this new method as
well but that is a larger change and I wanted to keep the initial change small.

BUG=617185

Review-Url: https://codereview.chromium.org/2271933002
Cr-Commit-Position: refs/heads/master@{#415774}

remoting/host/BUILD.gn

@@ -626,6 +626,7 @@ source_set("unit_tests") {
     "host_status_logger_unittest.cc",
     "ipc_desktop_environment_unittest.cc",
     "it2me/it2me_confirmation_dialog_proxy_unittest.cc",
+    "it2me/it2me_host_unittest.cc",
     "it2me/it2me_native_messaging_host_unittest.cc",
     "linux/audio_pipe_reader_unittest.cc",
     "linux/certificate_watcher_unittest.cc",

remoting/host/it2me/it2me_host.cc

@@ -4,8 +4,9 @@
 
 #include "remoting/host/it2me/it2me_host.h"
 
-#include <stddef.h>
-
+#include <cstdint>
+#include <memory>
+#include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ -37,6 +38,8 @@
 #include "remoting/protocol/jingle_session_manager.h"
 #include "remoting/protocol/network_settings.h"
 #include "remoting/protocol/transport_context.h"
+#include "remoting/protocol/validating_authenticator.h"
+#include "remoting/signaling/jid_util.h"
 #include "remoting/signaling/server_log_entry.h"
 
 namespace remoting {
@@ -47,6 +50,10 @@ namespace {
 const char kApplicationName[] = "chromoting";
 const int kMaxLoginAttempts = 5;
 
+using protocol::ValidatingAuthenticator;
+typedef ValidatingAuthenticator::Result ValidationResult;
+typedef ValidatingAuthenticator::ValidationCallback ValidationCallback;
+
 }  // namespace
 
 It2MeHost::It2MeHost(
@@ -70,6 +77,12 @@ It2MeHost::It2MeHost(
   DCHECK(task_runner_->BelongsToCurrentThread());
 }
 
+It2MeHost::~It2MeHost() {
+  // Check that resources that need to be torn down on the UI thread are gone.
+  DCHECK(!desktop_environment_factory_.get());
+  DCHECK(!policy_watcher_.get());
+}
+
 void It2MeHost::Connect() {
   if (!host_context_->ui_task_runner()->BelongsToCurrentThread()) {
     DCHECK(task_runner_->BelongsToCurrentThread());
@@ -323,6 +336,20 @@ void It2MeHost::OnClientDisconnected(const std::string& jid) {
   DisconnectOnNetworkThread();
 }
 
+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() {
+  return base::Bind(&It2MeHost::ValidateConnectionDetails,
+                    base::Unretained(this));
+}
+
 void It2MeHost::OnPolicyUpdate(
     std::unique_ptr<base::DictionaryValue> policies) {
   // The policy watcher runs on the |ui_task_runner|.
@@ -405,12 +432,6 @@ void It2MeHost::UpdateClientDomainPolicy(const std::string& client_domain) {
   required_client_domain_ = client_domain;
 }
 
-It2MeHost::~It2MeHost() {
-  // Check that resources that need to be torn down on the UI thread are gone.
-  DCHECK(!desktop_environment_factory_.get());
-  DCHECK(!policy_watcher_.get());
-}
-
 void It2MeHost::SetState(It2MeHostState state,
                          const std::string& error_message) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
@@ -490,7 +511,8 @@ void It2MeHost::OnReceivedSupportID(
   std::unique_ptr<protocol::AuthenticatorFactory> factory(
       new protocol::It2MeHostAuthenticatorFactory(
           local_certificate, host_key_pair_, access_code_hash,
-          required_client_domain_));
+          base::Bind(&It2MeHost::ValidateConnectionDetails,
+                     base::Unretained(this))));
   host_->SetAuthenticatorFactory(std::move(factory));
 
   // Pass the Access Code to the script object before changing state.
@@ -501,6 +523,31 @@ void It2MeHost::OnReceivedSupportID(
   SetState(kReceivedAccessCode, "");
 }
 
+void It2MeHost::ValidateConnectionDetails(
+    const std::string& remote_jid,
+    const protocol::ValidatingAuthenticator::ResultCallback& result_callback) {
+  // 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);
+}
+
 It2MeHostFactory::It2MeHostFactory() : policy_service_(nullptr) {
 }
 

remoting/host/it2me/it2me_host.h

@@ -14,6 +14,7 @@
 #include "remoting/host/host_status_observer.h"
 #include "remoting/host/it2me/it2me_confirmation_dialog.h"
 #include "remoting/host/it2me/it2me_confirmation_dialog_proxy.h"
+#include "remoting/protocol/validating_authenticator.h"
 #include "remoting/signaling/xmpp_signal_strategy.h"
 
 namespace base {
@@ -92,6 +93,16 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
     SetState(state, error_message);
   }
 
+  // Updates the current policies based on |policies|.  Runs |done_callback| on
+  // the calling thread once the policies have been updated.
+  void SetPolicyForTesting(std::unique_ptr<base::DictionaryValue> policies,
+                           const base::Closure& done_callback);
+
+  // Returns the callback used for validating the connection.  Do not run the
+  // returned callback after this object has been destroyed.
+  protocol::ValidatingAuthenticator::ValidationCallback
+  GetValidationCallbackForTesting();
+
  protected:
   friend class base::RefCountedThreadSafe<It2MeHost>;
 
@@ -141,6 +152,12 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
 
   void DisconnectOnNetworkThread();
 
+  // Uses details of the connection and current policies to determine if the
+  // connection should be accepted or rejected.
+  void ValidateConnectionDetails(
+      const std::string& remote_jid,
+      const protocol::ValidatingAuthenticator::ResultCallback& result_callback);
+
   // Caller supplied fields.
   std::unique_ptr<ChromotingHostContext> host_context_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
@@ -182,6 +199,10 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   // variable contains the thunk if it is necessary.
   base::Closure pending_connect_;
 
+  // Called after the client machine initiates the connection process and
+  // determines whether to reject the connection or allow it to continue.
+  protocol::ValidatingAuthenticator::ValidationCallback validation_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(It2MeHost);
 };
 

remoting/host/it2me/it2me_host_unittest.cc

+// Copyright 2016 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 <memory>
+#include <string>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/policy/policy_constants.h"
+#include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/host/chromoting_host_context.h"
+#include "remoting/host/policy_watcher.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+namespace {
+
+typedef protocol::ValidatingAuthenticator::Result ValidationResult;
+
+const char kTestClientJid[] = "ficticious_user@gmail.com/jid_resource";
+const char kTestClientUsernameNoJid[] = "completely_ficticious_user@gmail.com";
+const char kTestClientJidWithSlash[] = "fake/user@gmail.com/jid_resource";
+const char kMatchingDomain[] = "gmail.com";
+const char kMismatchedDomain1[] = "similar_to_gmail.com";
+const char kMismatchedDomain2[] = "gmail_at_the_beginning.com";
+const char kMismatchedDomain3[] = "not_even_close.com";
+
+}  // namespace
+
+class It2MeHostTest : public testing::Test {
+ public:
+  It2MeHostTest() {}
+  ~It2MeHostTest() override {}
+
+  // testing::Test interface.
+  void SetUp() override;
+  void TearDown() override;
+
+  void OnValidationComplete(const base::Closure& resume_callback,
+                            ValidationResult validation_result);
+
+ protected:
+  void SetClientDomainPolicy(const std::string& policy_value);
+
+  void RunValidationCallback(const std::string& remote_jid);
+
+  ValidationResult validation_result_ = ValidationResult::SUCCESS;
+
+ private:
+  std::unique_ptr<base::MessageLoop> message_loop_;
+  std::unique_ptr<base::RunLoop> run_loop_;
+
+  scoped_refptr<It2MeHost> it2me_host_;
+
+  std::string directory_bot_jid_;
+  XmppSignalStrategy::XmppServerConfig xmpp_server_config_;
+
+  DISALLOW_COPY_AND_ASSIGN(It2MeHostTest);
+};
+
+void It2MeHostTest::SetUp() {
+  message_loop_.reset(new base::MessageLoop());
+  run_loop_.reset(new base::RunLoop());
+
+  scoped_refptr<AutoThreadTaskRunner> auto_thread_task_runner =
+      new AutoThreadTaskRunner(base::ThreadTaskRunnerHandle::Get(),
+                               run_loop_->QuitClosure());
+  it2me_host_ = new It2MeHost(
+      ChromotingHostContext::Create(auto_thread_task_runner),
+      /*policy_watcher=*/nullptr,
+      /*confirmation_dialog_factory=*/nullptr,
+      /*observer=*/nullptr, xmpp_server_config_, directory_bot_jid_);
+}
+
+void It2MeHostTest::TearDown() {
+  it2me_host_ = nullptr;
+  run_loop_->Run();
+}
+
+void It2MeHostTest::OnValidationComplete(const base::Closure& resume_callback,
+                                         ValidationResult validation_result) {
+  validation_result_ = validation_result;
+  resume_callback.Run();
+}
+
+void It2MeHostTest::SetClientDomainPolicy(const std::string& policy_value) {
+  std::unique_ptr<base::DictionaryValue> policies(new base::DictionaryValue());
+  policies->SetString(policy::key::kRemoteAccessHostClientDomain, policy_value);
+
+  base::RunLoop run_loop;
+  it2me_host_->SetPolicyForTesting(std::move(policies), run_loop.QuitClosure());
+  run_loop.Run();
+}
+
+void It2MeHostTest::RunValidationCallback(const std::string& remote_jid) {
+  base::RunLoop run_loop;
+
+  it2me_host_->GetValidationCallbackForTesting().Run(
+      remote_jid, base::Bind(&It2MeHostTest::OnValidationComplete,
+                             base::Unretained(this), run_loop.QuitClosure()));
+
+  run_loop.Run();
+}
+
+TEST_F(It2MeHostTest, ConnectionValidation_NoClientDomainPolicy_ValidJid) {
+  RunValidationCallback(kTestClientJid);
+  ASSERT_EQ(ValidationResult::SUCCESS, validation_result_);
+}
+
+TEST_F(It2MeHostTest, ConnectionValidation_NoClientDomainPolicy_InvalidJid) {
+  RunValidationCallback(kTestClientUsernameNoJid);
+  ASSERT_EQ(ValidationResult::SUCCESS, validation_result_);
+}
+
+TEST_F(It2MeHostTest,
+       ConnectionValidation_NoClientDomainPolicy_InvalidUsername) {
+  RunValidationCallback(kTestClientJidWithSlash);
+  ASSERT_EQ(ValidationResult::SUCCESS, validation_result_);
+}
+
+TEST_F(It2MeHostTest, ConnectionValidation_ClientDomainPolicy_MatchingDomain) {
+  SetClientDomainPolicy(kMatchingDomain);
+  RunValidationCallback(kTestClientJid);
+  ASSERT_EQ(ValidationResult::SUCCESS, validation_result_);
+}
+
+TEST_F(It2MeHostTest, ConnectionValidation_ClientDomainPolicy_InvalidUserName) {
+  SetClientDomainPolicy(kMatchingDomain);
+  RunValidationCallback(kTestClientJidWithSlash);
+  ASSERT_EQ(ValidationResult::ERROR_INVALID_ACCOUNT, validation_result_);
+}
+
+TEST_F(It2MeHostTest, ConnectionValidation_ClientDomainPolicy_NoJid) {
+  SetClientDomainPolicy(kMatchingDomain);
+  RunValidationCallback(kTestClientUsernameNoJid);
+  ASSERT_EQ(ValidationResult::ERROR_INVALID_ACCOUNT, validation_result_);
+}
+
+TEST_F(It2MeHostTest, ConnectionValidation_WrongClientDomain_NoMatch) {
+  SetClientDomainPolicy(kMismatchedDomain3);
+  RunValidationCallback(kTestClientJid);
+  ASSERT_EQ(ValidationResult::ERROR_INVALID_ACCOUNT, validation_result_);
+}
+
+TEST_F(It2MeHostTest, ConnectionValidation_WrongClientDomain_MatchStart) {
+  SetClientDomainPolicy(kMismatchedDomain2);
+  RunValidationCallback(kTestClientJid);
+  ASSERT_EQ(ValidationResult::ERROR_INVALID_ACCOUNT, validation_result_);
+}
+
+TEST_F(It2MeHostTest, ConnectionValidation_WrongClientDomain_MatchEnd) {
+  SetClientDomainPolicy(kMismatchedDomain1);
+  RunValidationCallback(kTestClientJid);
+  ASSERT_EQ(ValidationResult::ERROR_INVALID_ACCOUNT, validation_result_);
+}
+
+}  // namespace remoting

remoting/host/it2me/it2me_native_messaging_host_unittest.cc

@@ -4,8 +4,9 @@
 
 #include "remoting/host/it2me/it2me_native_messaging_host.h"
 
-#include <stdint.h>
-
+#include <cstdint>
+#include <memory>
+#include <string>
 #include <utility>
 
 #include "base/compiler_specific.h"
@@ -81,7 +82,7 @@ class MockIt2MeHost : public It2MeHost {
                 const std::string& directory_bot_jid)
       : It2MeHost(std::move(context),
                   std::move(policy_watcher),
-                  nullptr,
+                  /*confirmation_dialog_factory=*/nullptr,
                   observer,
                   xmpp_server_config,
                   directory_bot_jid) {}
@@ -161,7 +162,8 @@ class MockIt2MeHostFactory : public It2MeHostFactory {
       base::WeakPtr<It2MeHost::Observer> observer,
       const XmppSignalStrategy::XmppServerConfig& xmpp_server_config,
       const std::string& directory_bot_jid) override {
-    return new MockIt2MeHost(std::move(context), nullptr, observer,
+    return new MockIt2MeHost(std::move(context),
+                             /*confirmation_dialog_factory=*/nullptr, observer,
                              xmpp_server_config, directory_bot_jid);
   }
 

remoting/protocol/it2me_host_authenticator_factory.cc

@@ -4,12 +4,15 @@
 
 #include "remoting/protocol/it2me_host_authenticator_factory.h"
 
+#include <memory>
+#include <string>
+#include <utility>
+
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
-#include "base/strings/string_util.h"
 #include "remoting/base/rsa_key_pair.h"
 #include "remoting/protocol/negotiating_host_authenticator.h"
-#include "remoting/protocol/rejecting_authenticator.h"
+#include "remoting/protocol/validating_authenticator.h"
 
 namespace remoting {
 namespace protocol {
@@ -18,11 +21,11 @@ It2MeHostAuthenticatorFactory::It2MeHostAuthenticatorFactory(
     const std::string& local_cert,
     scoped_refptr<RsaKeyPair> key_pair,
     const std::string& access_code_hash,
-    const std::string& required_client_domain)
+    const ValidatingAuthenticator::ValidationCallback& callback)
     : local_cert_(local_cert),
       key_pair_(key_pair),
       access_code_hash_(access_code_hash),
-      required_client_domain_(required_client_domain) {}
+      validation_callback_(callback) {}
 
 It2MeHostAuthenticatorFactory::~It2MeHostAuthenticatorFactory() {}
 
@@ -30,26 +33,13 @@ std::unique_ptr<Authenticator>
 It2MeHostAuthenticatorFactory::CreateAuthenticator(
     const std::string& local_jid,
     const std::string& remote_jid) {
-  // Check the client domain policy.
-  if (!required_client_domain_.empty()) {
-    std::string client_username = remote_jid;
-    size_t pos = client_username.find('/');
-    if (pos != std::string::npos) {
-      client_username.replace(pos, std::string::npos, "");
-    }
-    if (!base::EndsWith(client_username,
-                        std::string("@") + required_client_domain_,
-                        base::CompareCase::INSENSITIVE_ASCII)) {
-      LOG(ERROR) << "Rejecting incoming connection from " << remote_jid
-                 << ": Domain mismatch.";
-      return base::WrapUnique(
-          new RejectingAuthenticator(Authenticator::INVALID_ACCOUNT));
-    }
-  }
-
-  return NegotiatingHostAuthenticator::CreateWithSharedSecret(
-      local_jid, remote_jid, local_cert_, key_pair_, access_code_hash_,
-      nullptr);
+  std::unique_ptr<Authenticator> authenticator(
+      NegotiatingHostAuthenticator::CreateWithSharedSecret(
+          local_jid, remote_jid, local_cert_, key_pair_, access_code_hash_,
+          nullptr));
+
+  return base::MakeUnique<ValidatingAuthenticator>(
+      remote_jid, validation_callback_, std::move(authenticator));
 }
 
 }  // namespace protocol

remoting/protocol/it2me_host_authenticator_factory.h

@@ -12,6 +12,7 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "remoting/protocol/authenticator.h"
+#include "remoting/protocol/validating_authenticator.h"
 
 namespace remoting {
 
@@ -23,10 +24,11 @@ namespace protocol {
 // understands both the V2 and legacy V1 authentication mechanisms.
 class It2MeHostAuthenticatorFactory : public AuthenticatorFactory {
  public:
-  It2MeHostAuthenticatorFactory(const std::string& local_cert,
-                                scoped_refptr<RsaKeyPair> key_pair,
-                                const std::string& access_code,
-                                const std::string& required_client_domain);
+  It2MeHostAuthenticatorFactory(
+      const std::string& local_cert,
+      scoped_refptr<RsaKeyPair> key_pair,
+      const std::string& access_code,
+      const ValidatingAuthenticator::ValidationCallback& callback);
   ~It2MeHostAuthenticatorFactory() override;
 
   // AuthenticatorFactory interface.
@@ -38,7 +40,7 @@ class It2MeHostAuthenticatorFactory : public AuthenticatorFactory {
   std::string local_cert_;
   scoped_refptr<RsaKeyPair> key_pair_;
   std::string access_code_hash_;
-  std::string required_client_domain_;
+  ValidatingAuthenticator::ValidationCallback validation_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(It2MeHostAuthenticatorFactory);
 };

remoting/signaling/jid_util.h

@@ -13,9 +13,9 @@ namespace remoting {
 // to lower-case.
 std::string NormalizeJid(const std::string& jid);
 
-// Splits a JID into a bare JID and a resource suffix.  Either or both
-// of |full_jid| and |resource| may be null.  If |full_jid| is already
-// a bare JID, |resource| is set to the empty string.  Returns true of
+// Splits a JID into a bare JID and a resource suffix.  Either |full_jid|,
+// |resource|, or both may be null.  If |full_jid| is already
+// a bare JID, |resource| is set to the empty string.  Returns true if
 // |full_jid| has a resource, false if not.
 //
 // e.g. "user@domain/resource" -> "user@domain", "resource", true