Block event processing on host/client until the client has authentica…

…ted.

Input events:
* Client will not send them
* Host will not process them

Control events:
* Client will only process BeginSessionResponse
* Host will only process BeginSessionRequest
All other control messages will be ignored.

BUG=72466
TEST=manual+tests

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

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

remoting/client/chromoting_client.cc

@@ -233,6 +233,12 @@ void ChromotingClient::BeginSessionResponse(
     return;
   }
 
+  // Inform the connection that the client has been authenticated. This will
+  // enable the communication channels.
+  if (msg->success()) {
+    connection_->OnClientAuthenticated();
+  }
+
   view_->UpdateLoginStatus(msg->success(), msg->error_info());
   done->Run();
   delete done;

remoting/client/plugin/pepper_input_handler.cc

@@ -33,7 +33,8 @@ void PepperInputHandler::HandleCharacterEvent(
   // TODO(garykac): Coordinate key and char events.
 }
 
-void PepperInputHandler::HandleMouseMoveEvent(const PP_InputEvent_Mouse& event) {
+void PepperInputHandler::HandleMouseMoveEvent(
+    const PP_InputEvent_Mouse& event) {
   SendMouseMoveEvent(static_cast<int>(event.x),
                      static_cast<int>(event.y));
 }

remoting/host/chromoting_host.cc

@@ -383,6 +383,7 @@ void ChromotingHost::LocalLoginSucceeded() {
   connection_->client_stub()->BeginSessionResponse(
       status, new DeleteTask<protocol::LocalLoginStatus>(status));
 
+  connection_->OnClientAuthenticated();
   recorder_->Start();
 }
 

remoting/host/chromoting_host_unittest.cc

@@ -15,6 +15,16 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::remoting::protocol::LocalLoginCredentials;
+using ::remoting::protocol::MockClientStub;
+using ::remoting::protocol::MockConnectionToClient;
+using ::remoting::protocol::MockConnectionToClientEventHandler;
+using ::remoting::protocol::MockHostStub;
+using ::remoting::protocol::MockInputStub;
+using ::remoting::protocol::MockSession;
+using ::remoting::protocol::MockVideoStub;
+using ::remoting::protocol::SessionConfig;
+
 using testing::_;
 using testing::AnyNumber;
 using testing::DeleteArg;
@@ -32,8 +42,8 @@ void PostQuitTask(MessageLoop* message_loop) {
 }
 
 void BeginSessionRequest(protocol::HostStub* host_stub) {
-  protocol::LocalLoginCredentials* credentials =
-      new protocol::LocalLoginCredentials();
+  LocalLoginCredentials* credentials =
+      new LocalLoginCredentials();
   credentials->set_type(protocol::PASSWORD);
   credentials->set_username("hello");
 
@@ -42,7 +52,7 @@ void BeginSessionRequest(protocol::HostStub* host_stub) {
 
   host_stub->BeginSessionRequest(
       credentials,
-      new DeleteTask<protocol::LocalLoginCredentials>(credentials));
+      new DeleteTask<LocalLoginCredentials>(credentials));
 }
 
 // Run the task and delete it afterwards. This action is used to deal with
@@ -79,13 +89,15 @@ class ChromotingHostTest : public testing::Test {
         .Times(AnyNumber());
 
     Capturer* capturer = new CapturerFake(context_.main_message_loop());
-    input_stub_ = new protocol::MockInputStub();
+    host_stub_ = new MockHostStub();
+    input_stub_ = new MockInputStub();
     DesktopEnvironment* desktop =
         new DesktopEnvironmentFake(capturer, input_stub_);
     host_ = ChromotingHost::Create(&context_, config_, desktop);
-    connection_ = new protocol::MockConnectionToClient();
-    session_ = new protocol::MockSession();
-    session_config_.reset(protocol::SessionConfig::CreateDefault());
+    connection_ = new MockConnectionToClient(
+        &message_loop_, &handler_, host_stub_, input_stub_);
+    session_ = new MockSession();
+    session_config_.reset(SessionConfig::CreateDefault());
 
     ON_CALL(video_stub_, ProcessVideoPacket(_, _))
         .WillByDefault(
@@ -139,15 +151,17 @@ class ChromotingHostTest : public testing::Test {
 
  protected:
   MessageLoop message_loop_;
+  MockConnectionToClientEventHandler handler_;
   scoped_refptr<ChromotingHost> host_;
   scoped_refptr<InMemoryHostConfig> config_;
   MockChromotingHostContext context_;
-  scoped_refptr<protocol::MockConnectionToClient> connection_;
-  scoped_refptr<protocol::MockSession> session_;
-  scoped_ptr<protocol::SessionConfig> session_config_;
-  protocol::MockVideoStub video_stub_;
-  protocol::MockClientStub client_stub_;
-  protocol::MockInputStub* input_stub_;
+  scoped_refptr<MockConnectionToClient> connection_;
+  scoped_refptr<MockSession> session_;
+  scoped_ptr<SessionConfig> session_config_;
+  MockVideoStub video_stub_;
+  MockClientStub client_stub_;
+  MockHostStub* host_stub_;
+  MockInputStub* input_stub_;
 };
 
 TEST_F(ChromotingHostTest, StartAndShutdown) {

remoting/host/screen_recorder_unittest.cc

@@ -13,6 +13,9 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::remoting::protocol::MockConnectionToClient;
+using ::remoting::protocol::MockConnectionToClientEventHandler;
+using ::remoting::protocol::MockHostStub;
+using ::remoting::protocol::MockInputStub;
 using ::remoting::protocol::MockVideoStub;
 
 using ::testing::_;
@@ -77,14 +80,21 @@ class ScreenRecorderTest : public testing::Test {
   virtual void SetUp() {
     // Capturer and Encoder are owned by ScreenRecorder.
     encoder_ = new MockEncoder();
-    connection_ = new MockConnectionToClient();
+
+    connection_ = new MockConnectionToClient(&message_loop_, &handler_,
+                                             &host_stub_, &input_stub_);
+
     record_ = new ScreenRecorder(
         &message_loop_, &message_loop_, &message_loop_,
         &capturer_, encoder_);
   }
 
  protected:
   scoped_refptr<ScreenRecorder> record_;
+
+  MockConnectionToClientEventHandler handler_;
+  MockHostStub host_stub_;
+  MockInputStub input_stub_;
   scoped_refptr<MockConnectionToClient> connection_;
 
   // The following mock objects are owned by ScreenRecorder.

remoting/protocol/client_message_dispatcher.cc

@@ -39,18 +39,32 @@ void ClientMessageDispatcher::Initialize(
 
 void ClientMessageDispatcher::OnControlMessageReceived(
     ControlMessage* message, Task* done_task) {
-  // TODO(sergeyu): Add message validation.
-  if (message->has_notify_resolution()) {
-    client_stub_->NotifyResolution(
-        &message->notify_resolution(), done_task);
-  } else if (message->has_begin_session_response()) {
-    client_stub_->BeginSessionResponse(
-        &message->begin_session_response().login_status(), done_task);
+  if (!client_stub_->authenticated()) {
+    // When the client has not authenticated with the host, we restrict the
+    // control messages that we support.
+    if (message->has_begin_session_response()) {
+      client_stub_->BeginSessionResponse(
+          &message->begin_session_response().login_status(), done_task);
+      return;
+    } else {
+      LOG(WARNING) << "Invalid control message received "
+                   << "(client not authenticated).";
+    }
   } else {
-    LOG(WARNING) << "Invalid control message received.";
-    done_task->Run();
-    delete done_task;
+    // TODO(sergeyu): Add message validation.
+    if (message->has_notify_resolution()) {
+      client_stub_->NotifyResolution(
+          &message->notify_resolution(), done_task);
+      return;
+    } else if (message->has_begin_session_response()) {
+      LOG(WARNING) << "BeginSessionResponse sent after client already "
+                   << "authorized.";
+    } else {
+      LOG(WARNING) << "Invalid control message received.";
+    }
   }
+  done_task->Run();
+  delete done_task;
 }
 
 }  // namespace protocol

remoting/protocol/client_stub.cc

@@ -0,0 +1,30 @@
+// Copyright (c) 2010 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.
+
+// Interface of a client that receives commands from a Chromoting host.
+//
+// This interface is responsible for a subset of control messages sent to
+// the Chromoting client.
+
+#include "remoting/protocol/client_stub.h"
+
+namespace remoting {
+namespace protocol {
+
+ClientStub::ClientStub() : authenticated_(false) {
+}
+
+ClientStub::~ClientStub() {
+}
+
+void ClientStub::OnAuthenticated() {
+  authenticated_ = true;
+}
+
+bool ClientStub::authenticated() {
+  return authenticated_;
+}
+
+}  // namespace protocol
+}  // namespace remoting

remoting/protocol/client_stub.h

@@ -22,15 +22,29 @@ class NotifyResolutionRequest;
 
 class ClientStub {
  public:
-  ClientStub() {}
-  virtual ~ClientStub() {}
+  ClientStub();
+  virtual ~ClientStub();
 
   virtual void NotifyResolution(const NotifyResolutionRequest* msg,
                                 Task* done) = 0;
   virtual void BeginSessionResponse(const LocalLoginStatus* msg,
                                     Task* done) = 0;
 
+  // Called when the client has authenticated with the host to enable the
+  // host->client control channel.
+  // Before this is called, only a limited set of control messages will be
+  // processed.
+  void OnAuthenticated();
+
+  // Has the client successfully authenticated with the host?
+  // I.e., should we be processing control events?
+  bool authenticated();
+
  private:
+  // Initially false, this records whether the client has authenticated with
+  // the host.
+  bool authenticated_;
+
   DISALLOW_COPY_AND_ASSIGN(ClientStub);
 };
 

remoting/protocol/connection_to_client.cc

@@ -8,6 +8,8 @@
 #include "net/base/io_buffer.h"
 #include "remoting/protocol/client_control_sender.h"
 #include "remoting/protocol/host_message_dispatcher.h"
+#include "remoting/protocol/host_stub.h"
+#include "remoting/protocol/input_stub.h"
 
 // TODO(hclam): Remove this header once MessageDispatcher is used.
 #include "remoting/base/compound_buffer.h"
@@ -23,7 +25,8 @@ ConnectionToClient::ConnectionToClient(MessageLoop* message_loop,
                                        EventHandler* handler,
                                        HostStub* host_stub,
                                        InputStub* input_stub)
-    : loop_(message_loop),
+    : client_authenticated_(false),
+      loop_(message_loop),
       handler_(handler),
       host_stub_(host_stub),
       input_stub_(input_stub) {
@@ -73,9 +76,6 @@ ClientStub* ConnectionToClient::client_stub() {
   return client_stub_.get();
 }
 
-ConnectionToClient::ConnectionToClient() {
-}
-
 void ConnectionToClient::OnSessionStateChange(protocol::Session::State state) {
   if (state == protocol::Session::CONNECTED) {
     client_stub_.reset(new ClientControlSender(session_->control_channel()));
@@ -123,5 +123,17 @@ void ConnectionToClient::StateChangeTask(protocol::Session::State state) {
 void ConnectionToClient::OnClosed() {
 }
 
+void ConnectionToClient::OnClientAuthenticated() {
+  client_authenticated_ = true;
+
+  // Enable/disable each of the channels.
+  if (input_stub_)
+    input_stub_->OnAuthenticated();
+  if (host_stub_)
+    host_stub_->OnAuthenticated();
+  if (client_stub_.get())
+    client_stub_->OnAuthenticated();
+}
+
 }  // namespace protocol
 }  // namespace remoting

remoting/protocol/connection_to_client.h

@@ -72,9 +72,8 @@ class ConnectionToClient :
   // Return pointer to ClientStub.
   virtual ClientStub* client_stub();
 
- protected:
-  // Protected constructor used by unit test.
-  ConnectionToClient();
+  // Called when the host accepts the client authentication.
+  void OnClientAuthenticated();
 
  private:
   // Callback for protocol Session.
@@ -85,6 +84,11 @@ class ConnectionToClient :
 
   void OnClosed();
 
+  // Initially false, this is set to true once the client has authenticated
+  // properly. When this is false, many client messages (like input events)
+  // will be ignored.
+  bool client_authenticated_;
+
   // The libjingle channel used to send and receive data from the remote client.
   scoped_refptr<Session> session_;
 

remoting/protocol/connection_to_host.cc

@@ -25,7 +25,8 @@ ConnectionToHost::ConnectionToHost(
     JingleThread* thread,
     talk_base::NetworkManager* network_manager,
     talk_base::PacketSocketFactory* socket_factory)
-    : thread_(thread),
+    : client_authenticated_(false),
+      thread_(thread),
       network_manager_(network_manager),
       socket_factory_(socket_factory),
       event_callback_(NULL),
@@ -190,7 +191,6 @@ void ConnectionToHost::OnSessionStateChange(
       // Initialize reader and writer.
       video_reader_.reset(VideoReader::Create(session_->config()));
       video_reader_->Init(session_, video_stub_);
-      input_stub_.reset(new InputSender(session_->event_channel()));
       host_stub_.reset(new HostControlSender(session_->control_channel()));
       dispatcher_->Initialize(session_.get(), client_stub_);
       event_callback_->OnConnectionOpened(this);
@@ -202,5 +202,19 @@ void ConnectionToHost::OnSessionStateChange(
   }
 }
 
+void ConnectionToHost::OnClientAuthenticated() {
+  client_authenticated_ = true;
+
+  // Create and enable the input stub now that we're authenticated.
+  input_stub_.reset(new InputSender(session_->event_channel()));
+  input_stub_->OnAuthenticated();
+
+  // Enable control channel stubs.
+  if (host_stub_.get())
+    host_stub_->OnAuthenticated();
+  if (client_stub_)
+    client_stub_->OnAuthenticated();
+}
+
 }  // namespace protocol
 }  // namespace remoting

remoting/protocol/connection_to_host.h

@@ -85,6 +85,9 @@ class ConnectionToHost : public JingleClient::Callback {
   // Callback for chromotocol Session.
   void OnSessionStateChange(Session::State state);
 
+  // Called when the host accepts the client authentication.
+  void OnClientAuthenticated();
+
  private:
   // The message loop for the jingle thread this object works on.
   MessageLoop* message_loop();
@@ -101,6 +104,11 @@ class ConnectionToHost : public JingleClient::Callback {
   void OnDisconnected();
   void OnServerClosed();
 
+  // Initially false, this is set to true once the client has authenticated
+  // properly. When this is false, many messages to the host (like input events)
+  // will be suppressed.
+  bool client_authenticated_;
+
   JingleThread* thread_;
 
   scoped_ptr<talk_base::NetworkManager> network_manager_;