Verify that xmpp_login specified in the config matches auth token.

Previously the host would ignore the case when the oauth token doesn't
match the xmpp_login specified in the config. That would lead to situations
when host is connected with one account, but verifies incoming connections
with a different account. Fix this by verifying that the JID that we receive 
from XMPP server matches the value in the config.


Review URL: https://chromiumcodereview.appspot.com/10378110

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@136745 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 137 insertions, 23 deletions

diff --git a/remoting/host/remoting_me2me_host.cc b/remoting/host/remoting_me2me_host.cc
index bb7a7ea..195d91c 100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -335,8 +335,9 @@ class HostProcess
           new XmppSignalStrategy(context_->jingle_thread(), xmpp_login_,
                                  xmpp_auth_token_, xmpp_auth_service_));
 
-      signaling_connector_.reset(
-          new SignalingConnector(signal_strategy_.get()));
+      signaling_connector_.reset(new SignalingConnector(
+          signal_strategy_.get(),
+          base::Bind(&HostProcess::OnAuthFailed, base::Unretained(this))));
 
       if (!oauth_refresh_token_.empty()) {
         OAuthClientInfo client_info = {
@@ -360,7 +361,6 @@ class HostProcess
                 xmpp_login_, oauth_refresh_token_, client_info));
         signaling_connector_->EnableOAuth(
             oauth_credentials.Pass(),
-            base::Bind(&HostProcess::OnOAuthFailed, base::Unretained(this)),
             context_->url_request_context_getter());
       }
     }
@@ -401,7 +401,7 @@ class HostProcess
     CreateAuthenticatorFactory();
   }
 
-  void OnOAuthFailed() {
+  void OnAuthFailed() {
     Shutdown(kInvalidOauthCredentialsExitCode);
   }
 
diff --git a/remoting/host/signaling_connector.cc b/remoting/host/signaling_connector.cc
index cba384a..072eced 100644
--- a/remoting/host/signaling_connector.cc
+++ b/remoting/host/signaling_connector.cc
@@ -31,10 +31,14 @@ SignalingConnector::OAuthCredentials::OAuthCredentials(
       client_info(client_info_value) {
 }
 
-SignalingConnector::SignalingConnector(XmppSignalStrategy* signal_strategy)
+SignalingConnector::SignalingConnector(
+    XmppSignalStrategy* signal_strategy,
+    const base::Closure& auth_failed_callback)
     : signal_strategy_(signal_strategy),
+      auth_failed_callback_(auth_failed_callback),
       reconnect_attempts_(0),
       refreshing_oauth_token_(false) {
+  DCHECK(!auth_failed_callback_.is_null());
   net::NetworkChangeNotifier::AddOnlineStateObserver(this);
   net::NetworkChangeNotifier::AddIPAddressObserver(this);
   signal_strategy_->AddListener(this);
@@ -49,10 +53,8 @@ SignalingConnector::~SignalingConnector() {
 
 void SignalingConnector::EnableOAuth(
     scoped_ptr<OAuthCredentials> oauth_credentials,
-    const base::Closure& oauth_failed_callback,
     net::URLRequestContextGetter* url_context) {
   oauth_credentials_ = oauth_credentials.Pass();
-  oauth_failed_callback_ = oauth_failed_callback;
   gaia_oauth_client_.reset(new GaiaOAuthClient(kGaiaOAuth2Url, url_context));
 }
 
@@ -66,7 +68,14 @@ void SignalingConnector::OnSignalStrategyStateChange(
   } else if (state == SignalStrategy::DISCONNECTED) {
     LOG(INFO) << "Signaling disconnected.";
     reconnect_attempts_++;
-    ScheduleTryReconnect();
+
+    // If authentication failed then we have an invalid OAuth token,
+    // inform the upper layer about it.
+    if (signal_strategy_->GetError() == SignalStrategy::AUTHENTICATION_FAILED) {
+      auth_failed_callback_.Run();
+    } else {
+      ScheduleTryReconnect();
+    }
   }
 }
 
@@ -106,7 +115,7 @@ void SignalingConnector::OnOAuthError() {
   LOG(ERROR) << "OAuth: invalid credentials.";
   refreshing_oauth_token_ = false;
   reconnect_attempts_++;
-  oauth_failed_callback_.Run();
+  auth_failed_callback_.Run();
 }
 
 void SignalingConnector::OnNetworkError(int response_code) {
diff --git a/remoting/host/signaling_connector.h b/remoting/host/signaling_connector.h
index f73f744..a03ae64 100644
--- a/remoting/host/signaling_connector.h
+++ b/remoting/host/signaling_connector.h
@@ -50,13 +50,14 @@ class SignalingConnector
     OAuthClientInfo client_info;
   };
 
-  // OAuth token is updated refreshed when |oauth_credentials| is
-  // not NULL.
-  SignalingConnector(XmppSignalStrategy* signal_strategy);
+  // The |auth_failed_callback| is called when authentication fails.
+  SignalingConnector(XmppSignalStrategy* signal_strategy,
+                     const base::Closure& auth_failed_callback);
   virtual ~SignalingConnector();
 
+  // May be called immediately after the constructor to enable OAuth
+  // access token updating.
   void EnableOAuth(scoped_ptr<OAuthCredentials> oauth_credentials,
-                   const base::Closure& oauth_failed_callback,
                    net::URLRequestContextGetter* url_context);
 
   // SignalStrategy::Listener interface.
@@ -83,9 +84,9 @@ class SignalingConnector
   void RefreshOAuthToken();
 
   XmppSignalStrategy* signal_strategy_;
+  base::Closure auth_failed_callback_;
 
   scoped_ptr<OAuthCredentials> oauth_credentials_;
-  base::Closure oauth_failed_callback_;
   scoped_ptr<GaiaOAuthClient> gaia_oauth_client_;
 
   // Number of times we tried to connect without success.
diff --git a/remoting/host/simple_host_process.cc b/remoting/host/simple_host_process.cc
index 2548741..fd78413 100644
--- a/remoting/host/simple_host_process.cc
+++ b/remoting/host/simple_host_process.cc
@@ -35,6 +35,7 @@
 #include "remoting/host/capturer_fake.h"
 #include "remoting/host/chromoting_host.h"
 #include "remoting/host/chromoting_host_context.h"
+#include "remoting/host/constants.h"
 #include "remoting/host/desktop_environment.h"
 #include "remoting/host/event_executor.h"
 #include "remoting/host/heartbeat_sender.h"
@@ -94,7 +95,9 @@ class SimpleHost : public HeartbeatSender::Listener {
       : message_loop_(MessageLoop::TYPE_UI),
         context_(message_loop_.message_loop_proxy()),
         fake_(false),
-        is_it2me_(false) {
+        is_it2me_(false),
+        shutting_down_(false),
+        exit_code_(kSuccessExitCode) {
     context_.Start();
     network_change_notifier_.reset(net::NetworkChangeNotifier::Create());
   }
@@ -102,7 +105,7 @@ class SimpleHost : public HeartbeatSender::Listener {
   // Overridden from HeartbeatSender::Listener
   virtual void OnUnknownHostIdError() OVERRIDE {
     LOG(ERROR) << "Host ID not found.";
-    Shutdown();
+    Shutdown(kInvalidHostIdExitCode);
   }
 
   int Run() {
@@ -153,7 +156,7 @@ class SimpleHost : public HeartbeatSender::Listener {
 
     message_loop_.MessageLoop::Run();
 
-    return 0;
+    return exit_code_;
   }
 
   void set_config_path(const FilePath& config_path) {
@@ -214,7 +217,9 @@ class SimpleHost : public HeartbeatSender::Listener {
     signal_strategy_.reset(
         new XmppSignalStrategy(context_.jingle_thread(), xmpp_login_,
                                xmpp_auth_token_, xmpp_auth_service_));
-    signaling_connector_.reset(new SignalingConnector(signal_strategy_.get()));
+    signaling_connector_.reset(new SignalingConnector(
+        signal_strategy_.get(),
+        base::Bind(&SimpleHost::OnAuthFailed, base::Unretained(this))));
 
     if (fake_) {
       scoped_ptr<Capturer> capturer(new CapturerFake());
@@ -266,7 +271,32 @@ class SimpleHost : public HeartbeatSender::Listener {
     }
   }
 
-  void Shutdown() {
+  void OnAuthFailed() {
+    Shutdown(kInvalidOauthCredentialsExitCode);
+  }
+
+  void Shutdown(int exit_code) {
+    DCHECK(context_.network_message_loop()->BelongsToCurrentThread());
+
+    if (shutting_down_)
+      return;
+
+    shutting_down_ = true;
+    exit_code_ = exit_code;
+    host_->Shutdown(base::Bind(
+        &SimpleHost::OnShutdownFinished, base::Unretained(this)));
+  }
+
+  void OnShutdownFinished() {
+    DCHECK(context_.network_message_loop()->BelongsToCurrentThread());
+
+    // Destroy networking objects while we are on the network thread.
+    host_ = NULL;
+    log_to_server_.reset();
+    heartbeat_sender_.reset();
+    signaling_connector_.reset();
+    signal_strategy_.reset();
+
     message_loop_.PostTask(FROM_HERE, MessageLoop::QuitClosure());
   }
 
@@ -296,6 +326,9 @@ class SimpleHost : public HeartbeatSender::Listener {
   scoped_ptr<HeartbeatSender> heartbeat_sender_;
 
   scoped_refptr<ChromotingHost> host_;
+
+  bool shutting_down_;
+  int exit_code_;
 };
 
 } // namespace remoting
diff --git a/remoting/jingle_glue/fake_signal_strategy.cc b/remoting/jingle_glue/fake_signal_strategy.cc
index a854502..1ddabda 100644
--- a/remoting/jingle_glue/fake_signal_strategy.cc
+++ b/remoting/jingle_glue/fake_signal_strategy.cc
@@ -52,6 +52,10 @@ SignalStrategy::State FakeSignalStrategy::GetState() const {
   return CONNECTED;
 }
 
+SignalStrategy::Error FakeSignalStrategy::GetError() const {
+  return OK;
+}
+
 std::string FakeSignalStrategy::GetLocalJid() const {
   DCHECK(CalledOnValidThread());
   return jid_;
diff --git a/remoting/jingle_glue/fake_signal_strategy.h b/remoting/jingle_glue/fake_signal_strategy.h
index f7f8db2..9dc25cf 100644
--- a/remoting/jingle_glue/fake_signal_strategy.h
+++ b/remoting/jingle_glue/fake_signal_strategy.h
@@ -33,6 +33,7 @@ class FakeSignalStrategy : public SignalStrategy,
   virtual void Connect() OVERRIDE;
   virtual void Disconnect() OVERRIDE;
   virtual State GetState() const OVERRIDE;
+  virtual Error GetError() const OVERRIDE;
   virtual std::string GetLocalJid() const OVERRIDE;
   virtual void AddListener(Listener* listener) OVERRIDE;
   virtual void RemoveListener(Listener* listener) OVERRIDE;
diff --git a/remoting/jingle_glue/javascript_signal_strategy.cc b/remoting/jingle_glue/javascript_signal_strategy.cc
index 21cf3cc..6703c52 100644
--- a/remoting/jingle_glue/javascript_signal_strategy.cc
+++ b/remoting/jingle_glue/javascript_signal_strategy.cc
@@ -47,12 +47,21 @@ void JavascriptSignalStrategy::Disconnect() {
 }
 
 SignalStrategy::State JavascriptSignalStrategy::GetState() const {
+  DCHECK(CalledOnValidThread());
   // TODO(sergeyu): Extend XmppProxy to provide status of the
   // connection.
   return CONNECTED;
 }
 
+SignalStrategy::Error JavascriptSignalStrategy::GetError() const {
+  DCHECK(CalledOnValidThread());
+  // TODO(sergeyu): Extend XmppProxy to provide status of the
+  // connection.
+  return OK;
+}
+
 std::string JavascriptSignalStrategy::GetLocalJid() const {
+  DCHECK(CalledOnValidThread());
   return local_jid_;
 }
 
diff --git a/remoting/jingle_glue/javascript_signal_strategy.h b/remoting/jingle_glue/javascript_signal_strategy.h
index a229f52..eaab7a2 100644
--- a/remoting/jingle_glue/javascript_signal_strategy.h
+++ b/remoting/jingle_glue/javascript_signal_strategy.h
@@ -33,6 +33,7 @@ class JavascriptSignalStrategy : public SignalStrategy,
   virtual void Connect() OVERRIDE;
   virtual void Disconnect() OVERRIDE;
   virtual State GetState() const OVERRIDE;
+  virtual Error GetError() const OVERRIDE;
   virtual std::string GetLocalJid() const OVERRIDE;
   virtual void AddListener(Listener* listener) OVERRIDE;
   virtual void RemoveListener(Listener* listener) OVERRIDE;
diff --git a/remoting/jingle_glue/mock_objects.h b/remoting/jingle_glue/mock_objects.h
index 01d7117..1804628 100644
--- a/remoting/jingle_glue/mock_objects.h
+++ b/remoting/jingle_glue/mock_objects.h
@@ -18,6 +18,7 @@ class MockSignalStrategy : public SignalStrategy {
   MOCK_METHOD0(Connect, void());
   MOCK_METHOD0(Disconnect, void());
   MOCK_CONST_METHOD0(GetState, State());
+  MOCK_CONST_METHOD0(GetError, Error());
   MOCK_CONST_METHOD0(GetLocalJid, std::string());
   MOCK_METHOD1(AddListener, void(Listener* listener));
   MOCK_METHOD1(RemoveListener, void(Listener* listener));
diff --git a/remoting/jingle_glue/signal_strategy.h b/remoting/jingle_glue/signal_strategy.h
index 8971267..0651272 100644
--- a/remoting/jingle_glue/signal_strategy.h
+++ b/remoting/jingle_glue/signal_strategy.h
@@ -30,6 +30,12 @@ class SignalStrategy {
     DISCONNECTED,
   };
 
+  enum Error {
+    OK,
+    AUTHENTICATION_FAILED,
+    NETWORK_ERROR,
+  };
+
   // Callback interface for signaling event. Event handlers are not
   // allowed to destroy SignalStrategy, but may add or remove other
   // listeners.
@@ -37,7 +43,9 @@ class SignalStrategy {
    public:
     virtual ~Listener() {}
 
-    // Called after state of the connection has changed.
+    // Called after state of the connection has changed. If the state
+    // is DISCONNECTED, then GetError() can be used to get the reason
+    // for the disconnection.
     virtual void OnSignalStrategyStateChange(State state) {}
 
     // Must return true if the stanza was handled, false
@@ -63,6 +71,9 @@ class SignalStrategy {
   // Returns current state.
   virtual State GetState() const = 0;
 
+  // Returns the last error. Set when state changes to DISCONNECT.
+  virtual Error GetError() const = 0;
+
   // Returns local JID or an empty string when not connected.
   virtual std::string GetLocalJid() const = 0;
 
diff --git a/remoting/jingle_glue/xmpp_signal_strategy.cc b/remoting/jingle_glue/xmpp_signal_strategy.cc
index 1aa7d9c..6ae4f1a 100644
--- a/remoting/jingle_glue/xmpp_signal_strategy.cc
+++ b/remoting/jingle_glue/xmpp_signal_strategy.cc
@@ -4,7 +4,9 @@
 
 #include "remoting/jingle_glue/xmpp_signal_strategy.h"
 
+#include "base/bind.h"
 #include "base/logging.h"
+#include "base/string_util.h"
 #include "jingle/notifier/base/gaia_token_pre_xmpp_auth.h"
 #include "remoting/jingle_glue/jingle_thread.h"
 #include "remoting/jingle_glue/xmpp_socket_adapter.h"
@@ -20,6 +22,10 @@ const char kDefaultResourceName[] = "chromoting";
 // connections that are idle for more than a minute.
 const int kKeepAliveIntervalSeconds = 50;
 
+void DisconnectXmppClient(buzz::XmppClient* client) {
+  client->Disconnect();
+}
+
 }  // namespace
 
 namespace remoting {
@@ -34,11 +40,11 @@ XmppSignalStrategy::XmppSignalStrategy(JingleThread* jingle_thread,
      auth_token_service_(auth_token_service),
      resource_name_(kDefaultResourceName),
      xmpp_client_(NULL),
-     state_(DISCONNECTED) {
+     state_(DISCONNECTED),
+     error_(OK) {
 }
 
 XmppSignalStrategy::~XmppSignalStrategy() {
-  DCHECK_EQ(listeners_.size(), 0U);
   Disconnect();
 }
 
@@ -89,6 +95,11 @@ SignalStrategy::State XmppSignalStrategy::GetState() const {
   return state_;
 }
 
+SignalStrategy::Error XmppSignalStrategy::GetError() const {
+  DCHECK(CalledOnValidThread());
+  return error_;
+}
+
 std::string XmppSignalStrategy::GetLocalJid() const {
   DCHECK(CalledOnValidThread());
   return xmpp_client_->jid().Str();
@@ -154,7 +165,26 @@ void XmppSignalStrategy::SetResourceName(const std::string &resource_name) {
 void XmppSignalStrategy::OnConnectionStateChanged(
     buzz::XmppEngine::State state) {
   DCHECK(CalledOnValidThread());
+
   if (state == buzz::XmppEngine::STATE_OPEN) {
+    // Verify that the JID that we've received matches the username
+    // that we have. If it doesn't, then the OAuth token was probably
+    // issued for a different account, so we treat is a an auth error.
+    //
+    // TODO(sergeyu): Some user accounts may not have associated
+    // e-mail address. The check below will fail for such
+    // accounts. Make sure we can handle this case proprely.
+    if (!StartsWithASCII(GetLocalJid(), username_, false)) {
+      LOG(ERROR) << "Received JID that is different from the expected value.";
+      error_ = AUTHENTICATION_FAILED;
+      xmpp_client_->SignalStateChange.disconnect(this);
+      MessageLoop::current()->PostTask(
+          FROM_HERE, base::Bind(&DisconnectXmppClient, xmpp_client_));
+      xmpp_client_ = NULL;
+      SetState(DISCONNECTED);
+      return;
+    }
+
     keep_alive_timer_.Start(
         FROM_HERE, base::TimeDelta::FromSeconds(kKeepAliveIntervalSeconds),
         this, &XmppSignalStrategy::SendKeepAlive);
@@ -171,6 +201,18 @@ void XmppSignalStrategy::OnConnectionStateChanged(
     // Client is destroyed by the TaskRunner after the client is
     // closed. Reset the pointer so we don't try to use it later.
     xmpp_client_ = NULL;
+
+    switch (error) {
+      case buzz::XmppEngine::ERROR_UNAUTHORIZED:
+      case buzz::XmppEngine::ERROR_AUTH:
+      case buzz::XmppEngine::ERROR_MISSING_USERNAME:
+        error_ = AUTHENTICATION_FAILED;
+        break;
+
+      default:
+        error_ = NETWORK_ERROR;
+    }
+
     SetState(DISCONNECTED);
   }
 }
diff --git a/remoting/jingle_glue/xmpp_signal_strategy.h b/remoting/jingle_glue/xmpp_signal_strategy.h
index ec405a4..790b0bd 100644
--- a/remoting/jingle_glue/xmpp_signal_strategy.h
+++ b/remoting/jingle_glue/xmpp_signal_strategy.h
@@ -40,6 +40,7 @@ class XmppSignalStrategy : public base::NonThreadSafe,
   virtual void Connect() OVERRIDE;
   virtual void Disconnect() OVERRIDE;
   virtual State GetState() const OVERRIDE;
+  virtual Error GetError() const OVERRIDE;
   virtual std::string GetLocalJid() const OVERRIDE;
   virtual void AddListener(Listener* listener) OVERRIDE;
   virtual void RemoveListener(Listener* listener) OVERRIDE;
@@ -78,8 +79,9 @@ class XmppSignalStrategy : public base::NonThreadSafe,
   buzz::XmppClient* xmpp_client_;
 
   State state_;
+  Error error_;
 
-  ObserverList<Listener> listeners_;
+  ObserverList<Listener, true> listeners_;
 
   base::RepeatingTimer<XmppSignalStrategy> keep_alive_timer_;