Add AUTHENTICATED session state.

Also removed AUTHENTICATED state from ConnectionToHost - We no longer need it there.

BUG=105214


Review URL: http://codereview.chromium.org/8774017

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

11 files changed, 76 insertions, 56 deletions

diff --git a/remoting/client/chromoting_client.cc b/remoting/client/chromoting_client.cc
index f07b5e8..60bb4e6 100644
--- a/remoting/client/chromoting_client.cc
+++ b/remoting/client/chromoting_client.cc
@@ -162,8 +162,7 @@ void ChromotingClient::OnConnectionState(
     protocol::ConnectionToHost::Error error) {
   DCHECK(message_loop()->BelongsToCurrentThread());
   VLOG(1) << "ChromotingClient::OnConnectionState(" << state << ")";
-  if (state == protocol::ConnectionToHost::CONNECTED ||
-      state == protocol::ConnectionToHost::AUTHENTICATED)
+  if (state == protocol::ConnectionToHost::CONNECTED)
     Initialize();
   view_->SetConnectionState(state, error);
 }
diff --git a/remoting/client/plugin/pepper_view.cc b/remoting/client/plugin/pepper_view.cc
index 3d6a71d..04f8f98 100644
--- a/remoting/client/plugin/pepper_view.cc
+++ b/remoting/client/plugin/pepper_view.cc
@@ -238,9 +238,6 @@ void PepperView::SetConnectionState(protocol::ConnectionToHost::State state,
       break;
 
     case protocol::ConnectionToHost::CONNECTED:
-      break;
-
-    case protocol::ConnectionToHost::AUTHENTICATED:
       UnsetSolidFill();
       scriptable_obj->SetConnectionStatus(
           ChromotingScriptableObject::STATUS_CONNECTED,
diff --git a/remoting/protocol/connection_to_client.cc b/remoting/protocol/connection_to_client.cc
index e247770..de5b401 100644
--- a/remoting/protocol/connection_to_client.cc
+++ b/remoting/protocol/connection_to_client.cc
@@ -88,16 +88,18 @@ void ConnectionToClient::set_input_stub(protocol::InputStub* input_stub) {
   input_stub_ = input_stub;
 }
 
-void ConnectionToClient::OnSessionStateChange(protocol::Session::State state) {
+void ConnectionToClient::OnSessionStateChange(Session::State state) {
   DCHECK(CalledOnValidThread());
 
   DCHECK(handler_);
   switch(state) {
-    case protocol::Session::CONNECTING:
-      // Don't care about this message.
+    case Session::INITIALIZING:
+    case Session::CONNECTING:
+    case Session::CONNECTED:
+      // Don't care about these events.
       break;
 
-    case protocol::Session::CONNECTED:
+    case Session::AUTHENTICATED:
       // Initialize channels.
       control_dispatcher_.reset(new HostControlDispatcher());
       control_dispatcher_->Init(session_.get(), base::Bind(
@@ -118,18 +120,14 @@ void ConnectionToClient::OnSessionStateChange(protocol::Session::State state) {
 
       break;
 
-    case protocol::Session::CLOSED:
+    case Session::CLOSED:
       CloseChannels();
       handler_->OnConnectionClosed(this);
       break;
 
-    case protocol::Session::FAILED:
+    case Session::FAILED:
       CloseOnError();
       break;
-
-    default:
-      // We shouldn't receive other states.
-      NOTREACHED();
   }
 }
 
diff --git a/remoting/protocol/connection_to_client_unittest.cc b/remoting/protocol/connection_to_client_unittest.cc
index 41f299c..83cf225 100644
--- a/remoting/protocol/connection_to_client_unittest.cc
+++ b/remoting/protocol/connection_to_client_unittest.cc
@@ -37,6 +37,8 @@ class ConnectionToClientTest : public testing::Test {
     EXPECT_CALL(handler_, OnConnectionOpened(viewer_.get()));
     session_->state_change_callback().Run(
         protocol::Session::CONNECTED);
+    session_->state_change_callback().Run(
+        protocol::Session::AUTHENTICATED);
     message_loop_.RunAllPending();
   }
 
diff --git a/remoting/protocol/connection_to_host.cc b/remoting/protocol/connection_to_host.cc
index c1145a7..50a59d9 100644
--- a/remoting/protocol/connection_to_host.cc
+++ b/remoting/protocol/connection_to_host.cc
@@ -159,6 +159,33 @@ void ConnectionToHost::OnSessionStateChange(
   DCHECK(event_callback_);
 
   switch (state) {
+    case Session::INITIALIZING:
+    case Session::CONNECTING:
+    case Session::CONNECTED:
+      // Don't care about these events.
+      break;
+
+    case Session::AUTHENTICATED:
+      video_reader_.reset(VideoReader::Create(
+          message_loop_, session_->config()));
+      video_reader_->Init(session_.get(), video_stub_, base::Bind(
+          &ConnectionToHost::OnChannelInitialized, base::Unretained(this)));
+
+      control_dispatcher_.reset(new ClientControlDispatcher());
+      control_dispatcher_->Init(session_.get(), base::Bind(
+          &ConnectionToHost::OnChannelInitialized, base::Unretained(this)));
+      control_dispatcher_->set_client_stub(client_stub_);
+
+      event_dispatcher_.reset(new ClientEventDispatcher());
+      event_dispatcher_->Init(session_.get(), base::Bind(
+          &ConnectionToHost::OnChannelInitialized, base::Unretained(this)));
+      break;
+
+    case Session::CLOSED:
+      CloseChannels();
+      SetState(CLOSED, OK);
+      break;
+
     case Session::FAILED:
       switch (session_->error()) {
         case Session::PEER_IS_OFFLINE:
@@ -179,31 +206,6 @@ void ConnectionToHost::OnSessionStateChange(
           CloseOnError(NETWORK_FAILURE);
       }
       break;
-
-    case Session::CLOSED:
-      CloseChannels();
-      SetState(CLOSED, OK);
-      break;
-
-    case Session::CONNECTED:
-      video_reader_.reset(VideoReader::Create(
-          message_loop_, session_->config()));
-      video_reader_->Init(session_.get(), video_stub_, base::Bind(
-          &ConnectionToHost::OnChannelInitialized, base::Unretained(this)));
-
-      control_dispatcher_.reset(new ClientControlDispatcher());
-      control_dispatcher_->Init(session_.get(), base::Bind(
-          &ConnectionToHost::OnChannelInitialized, base::Unretained(this)));
-      control_dispatcher_->set_client_stub(client_stub_);
-
-      event_dispatcher_.reset(new ClientEventDispatcher());
-      event_dispatcher_->Init(session_.get(), base::Bind(
-          &ConnectionToHost::OnChannelInitialized, base::Unretained(this)));
-      break;
-
-    default:
-      // Ignore the other states by default.
-      break;
   }
 }
 
@@ -223,7 +225,6 @@ void ConnectionToHost::NotifyIfChannelsReady() {
       video_reader_.get() && video_reader_->is_connected() &&
       state_ == CONNECTING) {
     SetState(CONNECTED, OK);
-    SetState(AUTHENTICATED, OK);
   }
 }
 
diff --git a/remoting/protocol/connection_to_host.h b/remoting/protocol/connection_to_host.h
index c590da4..bba17fd 100644
--- a/remoting/protocol/connection_to_host.h
+++ b/remoting/protocol/connection_to_host.h
@@ -45,11 +45,7 @@ class ConnectionToHost : public SignalStrategy::StatusObserver,
  public:
   enum State {
     CONNECTING,
-    // TODO(sergeyu): Currently CONNECTED state is not used and state
-    // is set to AUTHENTICATED after we are connected. Remove it and
-    // renamed AUTHENTICATED to CONNECTED?
     CONNECTED,
-    AUTHENTICATED,
     FAILED,
     CLOSED,
   };
diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc
index dd66e04..40f0657 100644
--- a/remoting/protocol/jingle_session.cc
+++ b/remoting/protocol/jingle_session.cc
@@ -325,6 +325,9 @@ void JingleSession::OnAccept() {
   }
 
   SetState(CONNECTED);
+
+  if (authenticator_->state() == Authenticator::ACCEPTED)
+    SetState(AUTHENTICATED);
 }
 
 void JingleSession::OnTerminate() {
diff --git a/remoting/protocol/jingle_session_unittest.cc b/remoting/protocol/jingle_session_unittest.cc
index 8e7043b1..8757029 100644
--- a/remoting/protocol/jingle_session_unittest.cc
+++ b/remoting/protocol/jingle_session_unittest.cc
@@ -349,6 +349,9 @@ class JingleSessionTest : public testing::Test {
 
       EXPECT_CALL(host_connection_callback_,
                   OnStateChange(Session::CONNECTED))
+          .Times(1);
+      EXPECT_CALL(host_connection_callback_,
+                  OnStateChange(Session::AUTHENTICATED))
           .Times(1)
           .WillOnce(QuitThreadOnCounter(&not_connected_peers));
       // Expect that the connection will be closed eventually.
@@ -365,6 +368,9 @@ class JingleSessionTest : public testing::Test {
           .Times(1);
       EXPECT_CALL(client_connection_callback_,
                   OnStateChange(Session::CONNECTED))
+          .Times(1);
+      EXPECT_CALL(client_connection_callback_,
+                  OnStateChange(Session::AUTHENTICATED))
           .Times(1)
           .WillOnce(QuitThreadOnCounter(&not_connected_peers));
       // Expect that the connection will be closed eventually.
diff --git a/remoting/protocol/pepper_session.cc b/remoting/protocol/pepper_session.cc
index 735d7ad..e22287d 100644
--- a/remoting/protocol/pepper_session.cc
+++ b/remoting/protocol/pepper_session.cc
@@ -163,7 +163,7 @@ void PepperSession::set_config(const SessionConfig& config) {
 void PepperSession::Close() {
   DCHECK(CalledOnValidThread());
 
-  if (state_ == CONNECTING || state_ == CONNECTED) {
+  if (state_ == CONNECTING || state_ == CONNECTED || state_ == AUTHENTICATED) {
     // Send session-terminate message.
     JingleMessage message(peer_jid_, JingleMessage::SESSION_TERMINATE,
                           session_id_);
@@ -237,6 +237,9 @@ void PepperSession::OnAccept(const JingleMessage& message,
 
   SetState(CONNECTED);
 
+  if (authenticator_->state() == Authenticator::ACCEPTED)
+    SetState(AUTHENTICATED);
+
    // In case there is transport information in the accept message.
   ProcessTransportInfo(message);
 }
@@ -274,7 +277,7 @@ void PepperSession::OnTerminate(const JingleMessage& message,
     return;
   }
 
-  if (state_ == CONNECTED) {
+  if (state_ == CONNECTED || state_ == AUTHENTICATED) {
     if (message.reason == JingleMessage::GENERAL_ERROR) {
       OnError(CHANNEL_CONNECTION_ERROR);
     } else {
diff --git a/remoting/protocol/session.h b/remoting/protocol/session.h
index 4640fad..aceb39d 100644
--- a/remoting/protocol/session.h
+++ b/remoting/protocol/session.h
@@ -36,11 +36,15 @@ class Session : public base::NonThreadSafe {
 
     // Sent or received session-initiate, but haven't sent or received
     // session-accept.
+    // TODO(sergeyu): Do we really need this state?
     CONNECTING,
 
-    // Session has been accepted.
+    // Session has been accepted and is pending authentication.
     CONNECTED,
 
+    // Session has been connected and authenticated.
+    AUTHENTICATED,
+
     // Session has been closed.
     CLOSED,
 
@@ -58,7 +62,10 @@ class Session : public base::NonThreadSafe {
     CHANNEL_CONNECTION_ERROR,
   };
 
-  typedef base::Callback<void(State)> StateChangeCallback;
+  // State change callbacks are called after session state has
+  // changed. It is not safe to destroy the session from within the
+  // handler unless |state| is CLOSED or FAILED.
+  typedef base::Callback<void(State state)> StateChangeCallback;
 
   // TODO(sergeyu): Specify connection error code when channel
   // connection fails.
@@ -80,8 +87,8 @@ class Session : public base::NonThreadSafe {
   // callback is called with NULL if connection failed for any reason.
   // Ownership of the channel socket is given to the caller when the
   // callback is called. All channels must be destroyed before the
-  // session is destroyed. Can be called only when in CONNECTING or
-  // CONNECTED state.
+  // session is destroyed. Can be called only when in CONNECTING,
+  // CONNECTED or AUTHENTICATED states.
   virtual void CreateStreamChannel(
       const std::string& name, const StreamChannelCallback& callback) = 0;
   virtual void CreateDatagramChannel(
diff --git a/remoting/protocol/session_manager.h b/remoting/protocol/session_manager.h
index 7a02c3d..ad484fa 100644
--- a/remoting/protocol/session_manager.h
+++ b/remoting/protocol/session_manager.h
@@ -18,12 +18,20 @@
 // The callback function decides whether the session should be accepted or
 // rejected.
 //
+// AUTHENTICATION
+// Implementations of the Session and SessionManager interfaces
+// delegate authentication to an Authenticator implementation. For
+// incoming connections authenticators are created using an
+// AuthenticatorFactory set via the set_authenticator_factory()
+// method. For outgoing sessions authenticator must be passed to the
+// Connect() method. The Session's state changes to AUTHENTICATED once
+// authentication succeeds.
+//
 // SESSION OWNERSHIP AND SHUTDOWN
-// SessionManager owns all Sessions it creates. The manager must not
-// be closed or destroyed before all sessions created by that
-// SessionManager are destroyed. Caller owns Sessions created by a
-// SessionManager (except rejected sessions). Sessions must outlive
-// SessionManager, and SignalStrategy must outlive SessionManager.
+// The SessionManager must not be closed or destroyed before all sessions
+// created by that SessionManager are destroyed. Caller owns Sessions
+// created by a SessionManager (except rejected
+// sessions). The SignalStrategy must outlive the SessionManager.
 //
 // PROTOCOL VERSION NEGOTIATION
 // When client connects to a host it sends a session-initiate stanza with list