Fix ConnectionToClient to connect stubs only after authentication.

Previously ConnectionToClient required stubs to be created before the
client is authenticated. This is no longer necessary. Fixed it so
set_*_stub() methods can be called only after authentication. Also
removed auth input filters from ClientSession as they are no longer
useful.

BUG=448838

Review URL: https://codereview.chromium.org/902613004

Cr-Commit-Position: refs/heads/master@{#316299}

remoting/host/client_session.cc

@@ -94,8 +94,6 @@ ClientSession::ClientSession(
       mouse_clamping_filter_(&remote_input_filter_),
       disable_input_filter_(mouse_clamping_filter_.input_filter()),
       disable_clipboard_filter_(clipboard_echo_filter_.host_filter()),
-      auth_input_filter_(&disable_input_filter_),
-      auth_clipboard_filter_(&disable_clipboard_filter_),
       client_clipboard_factory_(clipboard_echo_filter_.client_filter()),
       max_duration_(max_duration),
       audio_task_runner_(audio_task_runner),
@@ -105,23 +103,13 @@ ClientSession::ClientSession(
       network_task_runner_(network_task_runner),
       ui_task_runner_(ui_task_runner),
       pairing_registry_(pairing_registry),
+      is_authenticated_(false),
       pause_video_(false),
       lossless_video_encode_(false),
       lossless_video_color_(false),
       weak_factory_(this) {
   connection_->SetEventHandler(this);
 
-  // TODO(sergeyu): Currently ConnectionToClient expects stubs to be
-  // set before channels are connected. Make it possible to set stubs
-  // later and set them only when connection is authenticated.
-  connection_->set_clipboard_stub(&auth_clipboard_filter_);
-  connection_->set_host_stub(this);
-  connection_->set_input_stub(&auth_input_filter_);
-
-  // |auth_*_filter_|'s states reflect whether the session is authenticated.
-  auth_input_filter_.set_enabled(false);
-  auth_clipboard_filter_.set_enabled(false);
-
   // Create a manager for the configured extensions, if any.
   extension_manager_.reset(new HostExtensionSessionManager(extensions, this));
 
@@ -289,11 +277,7 @@ void ClientSession::OnConnectionAuthenticated(
   DCHECK(!screen_controls_);
   DCHECK(!video_frame_pump_);
 
-  auth_input_filter_.set_enabled(true);
-  auth_clipboard_filter_.set_enabled(true);
-
-  clipboard_echo_filter_.set_client_stub(connection_->client_stub());
-  mouse_clamping_filter_.set_video_stub(connection_->video_stub());
+  is_authenticated_ = true;
 
   if (max_duration_ > base::TimeDelta()) {
     // TODO(simonmorris): Let Disconnect() tell the client that the
@@ -317,6 +301,12 @@ void ClientSession::OnConnectionAuthenticated(
     return;
   }
 
+  // Connect host stub.
+  connection_->set_host_stub(this);
+
+  // Connect video stub.
+  mouse_clamping_filter_.set_video_stub(connection_->video_stub());
+
   // Collate the set of capabilities to offer the client, if it supports them.
   host_capabilities_ = desktop_environment_->GetCapabilities();
   if (!host_capabilities_.empty())
@@ -329,9 +319,14 @@ void ClientSession::OnConnectionAuthenticated(
   // Create the event executor.
   input_injector_ = desktop_environment_->CreateInputInjector();
 
-  // Connect the host clipboard and input stubs.
+  // Connect the host input stubs.
+  connection_->set_input_stub(&disable_input_filter_);
   host_input_filter_.set_input_stub(input_injector_.get());
+
+  // Connect the clipboard stubs.
+  connection_->set_clipboard_stub(&disable_clipboard_filter_);
   clipboard_echo_filter_.set_host_stub(input_injector_.get());
+  clipboard_echo_filter_.set_client_stub(connection_->client_stub());
 
   // Create a GnubbyAuthHandler to proxy gnubbyd messages.
   gnubby_auth_handler_ = desktop_environment_->CreateGnubbyAuthHandler(
@@ -345,7 +340,6 @@ void ClientSession::OnConnectionChannelsConnected(
 
   // Negotiate capabilities with the client.
   VLOG(1) << "Host capabilities: " << host_capabilities_;
-
   protocol::Capabilities capabilities;
   capabilities.set_capabilities(host_capabilities_);
   connection_->client_stub()->SetCapabilities(capabilities);
@@ -380,15 +374,9 @@ void ClientSession::OnConnectionClosed(
   weak_factory_.InvalidateWeakPtrs();
 
   // If the client never authenticated then the session failed.
-  if (!auth_input_filter_.enabled())
+  if (!is_authenticated_)
     event_handler_->OnSessionAuthenticationFailed(this);
 
-  // Block any further input events from the client.
-  // TODO(wez): Fix ChromotingHost::OnSessionClosed not to check our
-  // is_authenticated(), so that we can disable |auth_*_filter_| here.
-  disable_input_filter_.set_enabled(false);
-  disable_clipboard_filter_.set_enabled(false);
-
   // Ensure that any pressed keys or buttons are released.
   input_tracker_.ReleaseAll();
 

remoting/host/client_session.h

@@ -141,7 +141,7 @@ class ClientSession
     return connection_.get();
   }
 
-  bool is_authenticated() { return auth_input_filter_.enabled();  }
+  bool is_authenticated() { return is_authenticated_;  }
 
   const std::string* client_capabilities() const {
     return client_capabilities_.get();
@@ -185,10 +185,6 @@ class ClientSession
   protocol::InputFilter disable_input_filter_;
   protocol::ClipboardFilter disable_clipboard_filter_;
 
-  // Filters used to disable input & clipboard when we're not authenticated.
-  protocol::InputFilter auth_input_filter_;
-  protocol::ClipboardFilter auth_clipboard_filter_;
-
   // Factory for weak pointers to the client clipboard stub.
   // This must appear after |clipboard_echo_filter_|, so that it won't outlive
   // it.
@@ -240,6 +236,9 @@ class ClientSession
   // Used to manage extension functionality.
   scoped_ptr<HostExtensionSessionManager> extension_manager_;
 
+  // Set to true if the client was authenticated successfully.
+  bool is_authenticated_;
+
   // Used to store video channel pause & lossless parameters.
   bool pause_video_;
   bool lossless_video_encode_;

remoting/host/client_session_unittest.cc

@@ -42,6 +42,7 @@ using protocol::SessionConfig;
 using testing::_;
 using testing::AnyNumber;
 using testing::AtMost;
+using testing::AtLeast;
 using testing::CreateFunctor;
 using testing::DeleteArg;
 using testing::DoAll;
@@ -310,7 +311,15 @@ webrtc::MouseCursorMonitor* ClientSessionTest::CreateMouseCursorMonitor() {
 }
 
 void ClientSessionTest::ConnectClientSession() {
+  // Stubs should be set only after connection is authenticated.
+  EXPECT_FALSE(connection_->clipboard_stub());
+  EXPECT_FALSE(connection_->input_stub());
+
   client_session_->OnConnectionAuthenticated(client_session_->connection());
+
+  EXPECT_TRUE(connection_->clipboard_stub());
+  EXPECT_TRUE(connection_->input_stub());
+
   client_session_->OnConnectionChannelsConnected(client_session_->connection());
 }
 
@@ -346,126 +355,94 @@ MATCHER_P2(EqualsClipboardEvent, m, d, "") {
 TEST_F(ClientSessionTest, ClipboardStubFilter) {
   CreateClientSession();
 
-  protocol::ClipboardEvent clipboard_event1;
-  clipboard_event1.set_mime_type(kMimeTypeTextUtf8);
-  clipboard_event1.set_data("a");
-
-  protocol::ClipboardEvent clipboard_event2;
-  clipboard_event2.set_mime_type(kMimeTypeTextUtf8);
-  clipboard_event2.set_data("b");
-
-  protocol::ClipboardEvent clipboard_event3;
-  clipboard_event3.set_mime_type(kMimeTypeTextUtf8);
-  clipboard_event3.set_data("c");
-
-  Expectation authenticated =
-      EXPECT_CALL(session_event_handler_, OnSessionAuthenticated(_))
-          .WillOnce(Return(true));
-  EXPECT_CALL(*input_injector_, StartPtr(_))
-      .After(authenticated);
-  EXPECT_CALL(session_event_handler_, OnSessionChannelsConnected(_))
-      .After(authenticated);
+  EXPECT_CALL(session_event_handler_, OnSessionAuthenticated(_))
+      .WillOnce(Return(true));
+  EXPECT_CALL(*input_injector_, StartPtr(_));
+  EXPECT_CALL(session_event_handler_, OnSessionChannelsConnected(_));
 
   // Wait for the first video packet to be captured to make sure that
   // the injected input will go though. Otherwise mouse events will be blocked
   // by the mouse clamping filter.
-  Sequence s;
+  base::RunLoop run_loop;
   EXPECT_CALL(video_stub_, ProcessVideoPacketPtr(_, _))
-      .InSequence(s)
-      .After(authenticated)
-      .WillOnce(DoAll(
-          // This event should get through to the clipboard stub.
-          InjectClipboardEvent(connection_, clipboard_event2),
-          InvokeWithoutArgs(this, &ClientSessionTest::DisconnectClientSession),
-          // This event should not get through to the clipboard stub,
-          // because the client has disconnected.
-          InjectClipboardEvent(connection_, clipboard_event3),
-          InvokeWithoutArgs(this, &ClientSessionTest::StopClientSession)));
-  EXPECT_CALL(*input_injector_, InjectClipboardEvent(EqualsClipboardEvent(
-      kMimeTypeTextUtf8, "b")))
-      .InSequence(s);
-  EXPECT_CALL(session_event_handler_, OnSessionClosed(_))
-      .InSequence(s);
-
-  // This event should not get through to the clipboard stub,
-  // because the client isn't authenticated yet.
-  connection_->clipboard_stub()->InjectClipboardEvent(clipboard_event1);
+      .Times(AtLeast(1))
+      .WillOnce(testing::InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
+
+  {
+    EXPECT_CALL(*input_injector_, InjectClipboardEvent(EqualsClipboardEvent(
+                                      kMimeTypeTextUtf8, "a")));
+    EXPECT_CALL(*input_injector_, InjectKeyEvent(EqualsUsbEvent(1, true)));
+    EXPECT_CALL(*input_injector_, InjectKeyEvent(EqualsUsbEvent(1, false)));
+    EXPECT_CALL(*input_injector_, InjectMouseEvent(EqualsMouseEvent(100, 101)));
+
+    EXPECT_CALL(*input_injector_, InjectClipboardEvent(EqualsClipboardEvent(
+                                      kMimeTypeTextUtf8, "c")));
+    EXPECT_CALL(*input_injector_, InjectKeyEvent(EqualsUsbEvent(3, true)));
+    EXPECT_CALL(*input_injector_, InjectMouseEvent(EqualsMouseEvent(300, 301)));
+    EXPECT_CALL(*input_injector_, InjectKeyEvent(EqualsUsbEvent(3, false)));
+  }
 
   ConnectClientSession();
-}
 
-TEST_F(ClientSessionTest, InputStubFilter) {
-  CreateClientSession();
+  // With for the first frame.
+  run_loop.Run();
 
-  protocol::KeyEvent key_event1;
-  key_event1.set_pressed(true);
-  key_event1.set_usb_keycode(1);
+  // Inject test events that are expected to be injected.
+  protocol::ClipboardEvent clipboard_event;
+  clipboard_event.set_mime_type(kMimeTypeTextUtf8);
+  clipboard_event.set_data("a");
+  connection_->clipboard_stub()->InjectClipboardEvent(clipboard_event);
 
-  protocol::KeyEvent key_event2_down;
-  key_event2_down.set_pressed(true);
-  key_event2_down.set_usb_keycode(2);
+  protocol::KeyEvent key_event;
+  key_event.set_pressed(true);
+  key_event.set_usb_keycode(1);
+  connection_->input_stub()->InjectKeyEvent(key_event);
 
-  protocol::KeyEvent key_event2_up;
-  key_event2_up.set_pressed(false);
-  key_event2_up.set_usb_keycode(2);
+  protocol::MouseEvent mouse_event;
+  mouse_event.set_x(100);
+  mouse_event.set_y(101);
+  connection_->input_stub()->InjectMouseEvent(mouse_event);
 
-  protocol::KeyEvent key_event3;
-  key_event3.set_pressed(true);
-  key_event3.set_usb_keycode(3);
+  base::RunLoop().RunUntilIdle();
 
-  protocol::MouseEvent mouse_event1;
-  mouse_event1.set_x(100);
-  mouse_event1.set_y(101);
+  // Disable input.
+  client_session_->SetDisableInputs(true);
 
-  protocol::MouseEvent mouse_event2;
-  mouse_event2.set_x(200);
-  mouse_event2.set_y(201);
+  // These event shouldn't get though to the input injector.
+  clipboard_event.set_data("b");
+  connection_->clipboard_stub()->InjectClipboardEvent(clipboard_event);
 
-  protocol::MouseEvent mouse_event3;
-  mouse_event3.set_x(300);
-  mouse_event3.set_y(301);
+  key_event.set_pressed(true);
+  key_event.set_usb_keycode(2);
+  connection_->input_stub()->InjectKeyEvent(key_event);
+  key_event.set_pressed(false);
+  key_event.set_usb_keycode(2);
+  connection_->input_stub()->InjectKeyEvent(key_event);
 
-  Expectation authenticated =
-      EXPECT_CALL(session_event_handler_, OnSessionAuthenticated(_))
-          .WillOnce(Return(true));
-  EXPECT_CALL(*input_injector_, StartPtr(_))
-      .After(authenticated);
-  EXPECT_CALL(session_event_handler_, OnSessionChannelsConnected(_))
-      .After(authenticated);
+  mouse_event.set_x(200);
+  mouse_event.set_y(201);
+  connection_->input_stub()->InjectMouseEvent(mouse_event);
 
-  // Wait for the first video packet to be captured to make sure that
-  // the injected input will go though. Otherwise mouse events will be blocked
-  // by the mouse clamping filter.
-  Sequence s;
-  EXPECT_CALL(video_stub_, ProcessVideoPacketPtr(_, _))
-      .InSequence(s)
-      .After(authenticated)
-      .WillOnce(DoAll(
-          // These events should get through to the input stub.
-          InjectKeyEvent(connection_, key_event2_down),
-          InjectKeyEvent(connection_, key_event2_up),
-          InjectMouseEvent(connection_, mouse_event2),
-          InvokeWithoutArgs(this, &ClientSessionTest::DisconnectClientSession),
-          // These events should not get through to the input stub,
-          // because the client has disconnected.
-          InjectKeyEvent(connection_, key_event3),
-          InjectMouseEvent(connection_, mouse_event3),
-          InvokeWithoutArgs(this, &ClientSessionTest::StopClientSession)));
-  EXPECT_CALL(*input_injector_, InjectKeyEvent(EqualsUsbEvent(2, true)))
-      .InSequence(s);
-  EXPECT_CALL(*input_injector_, InjectKeyEvent(EqualsUsbEvent(2, false)))
-      .InSequence(s);
-  EXPECT_CALL(*input_injector_, InjectMouseEvent(EqualsMouseEvent(200, 201)))
-      .InSequence(s);
-  EXPECT_CALL(session_event_handler_, OnSessionClosed(_))
-      .InSequence(s);
+  base::RunLoop().RunUntilIdle();
 
-  // These events should not get through to the input stub,
-  // because the client isn't authenticated yet.
-  connection_->input_stub()->InjectKeyEvent(key_event1);
-  connection_->input_stub()->InjectMouseEvent(mouse_event1);
+  // Enable input again.
+  client_session_->SetDisableInputs(false);
 
-  ConnectClientSession();
+  clipboard_event.set_data("c");
+  connection_->clipboard_stub()->InjectClipboardEvent(clipboard_event);
+  base::RunLoop().RunUntilIdle();
+
+  key_event.set_pressed(true);
+  key_event.set_usb_keycode(3);
+  connection_->input_stub()->InjectKeyEvent(key_event);
+
+  mouse_event.set_x(300);
+  mouse_event.set_y(301);
+  connection_->input_stub()->InjectMouseEvent(mouse_event);
+
+  client_session_->DisconnectSession();
+  client_session_->OnConnectionClosed(connection_, protocol::OK);
+  client_session_.reset();
 }
 
 TEST_F(ClientSessionTest, LocalInputTest) {

remoting/protocol/connection_to_client.cc

@@ -20,9 +20,6 @@ namespace protocol {
 
 ConnectionToClient::ConnectionToClient(protocol::Session* session)
     : handler_(nullptr),
-      clipboard_stub_(nullptr),
-      host_stub_(nullptr),
-      input_stub_(nullptr),
       session_(session) {
   session_->SetEventHandler(this);
 }
@@ -74,32 +71,17 @@ ClientStub* ConnectionToClient::client_stub() {
 void ConnectionToClient::set_clipboard_stub(
     protocol::ClipboardStub* clipboard_stub) {
   DCHECK(CalledOnValidThread());
-  clipboard_stub_ = clipboard_stub;
-}
-
-ClipboardStub* ConnectionToClient::clipboard_stub() {
-  DCHECK(CalledOnValidThread());
-  return clipboard_stub_;
+  control_dispatcher_->set_clipboard_stub(clipboard_stub);
 }
 
 void ConnectionToClient::set_host_stub(protocol::HostStub* host_stub) {
   DCHECK(CalledOnValidThread());
-  host_stub_ = host_stub;
-}
-
-HostStub* ConnectionToClient::host_stub() {
-  DCHECK(CalledOnValidThread());
-  return host_stub_;
+  control_dispatcher_->set_host_stub(host_stub);
 }
 
 void ConnectionToClient::set_input_stub(protocol::InputStub* input_stub) {
   DCHECK(CalledOnValidThread());
-  input_stub_ = input_stub;
-}
-
-InputStub* ConnectionToClient::input_stub() {
-  DCHECK(CalledOnValidThread());
-  return input_stub_;
+  event_dispatcher_->set_input_stub(input_stub);
 }
 
 void ConnectionToClient::OnSessionStateChange(Session::State state) {
@@ -121,13 +103,10 @@ void ConnectionToClient::OnSessionStateChange(Session::State state) {
       control_dispatcher_.reset(new HostControlDispatcher());
       control_dispatcher_->Init(session_.get(),
                                 session_->config().control_config(), this);
-      control_dispatcher_->set_clipboard_stub(clipboard_stub_);
-      control_dispatcher_->set_host_stub(host_stub_);
 
       event_dispatcher_.reset(new HostEventDispatcher());
       event_dispatcher_->Init(session_.get(), session_->config().event_config(),
                               this);
-      event_dispatcher_->set_input_stub(input_stub_);
       event_dispatcher_->set_event_timestamp_callback(base::Bind(
           &ConnectionToClient::OnEventTimestamp, base::Unretained(this)));
 

remoting/protocol/connection_to_client.h

@@ -92,14 +92,11 @@ class ConnectionToClient : public base::NonThreadSafe,
   virtual AudioStub* audio_stub();
   virtual ClientStub* client_stub();
 
-  // Set/get the stubs which will handle messages we receive from the client.
-  // All stubs MUST be set before the session's channels become connected.
+  // Set the stubs which will handle messages we receive from the client. These
+  // must be called in EventHandler::OnConnectionAuthenticated().
   virtual void set_clipboard_stub(ClipboardStub* clipboard_stub);
-  virtual ClipboardStub* clipboard_stub();
   virtual void set_host_stub(HostStub* host_stub);
-  virtual HostStub* host_stub();
   virtual void set_input_stub(InputStub* input_stub);
-  virtual InputStub* input_stub();
 
   // Session::EventHandler interface.
   void OnSessionStateChange(Session::State state) override;
@@ -122,11 +119,6 @@ class ConnectionToClient : public base::NonThreadSafe,
   // Event handler for handling events sent from this object.
   EventHandler* handler_;
 
-  // Stubs that are called for incoming messages.
-  ClipboardStub* clipboard_stub_;
-  HostStub* host_stub_;
-  InputStub* input_stub_;
-
   // The libjingle channel used to send and receive data from the remote client.
   scoped_ptr<Session> session_;
 

remoting/protocol/connection_to_client_unittest.cc

@@ -14,6 +14,7 @@
 #include "testing/gmock/include/gmock/gmock.h"
 
 using ::testing::_;
+using ::testing::InvokeWithoutArgs;
 using ::testing::NotNull;
 using ::testing::StrictMock;
 
@@ -31,11 +32,10 @@ class ConnectionToClientTest : public testing::Test {
 
     // Allocate a ClientConnection object with the mock objects.
     viewer_.reset(new ConnectionToClient(session_));
-    viewer_->set_clipboard_stub(&clipboard_stub_);
-    viewer_->set_host_stub(&host_stub_);
-    viewer_->set_input_stub(&input_stub_);
     viewer_->SetEventHandler(&handler_);
-    EXPECT_CALL(handler_, OnConnectionAuthenticated(viewer_.get()));
+    EXPECT_CALL(handler_, OnConnectionAuthenticated(viewer_.get()))
+        .WillOnce(
+            InvokeWithoutArgs(this, &ConnectionToClientTest::ConnectStubs));
     EXPECT_CALL(handler_, OnConnectionChannelsConnected(viewer_.get()));
     session_->event_handler()->OnSessionStateChange(Session::CONNECTED);
     session_->event_handler()->OnSessionStateChange(Session::AUTHENTICATED);
@@ -47,6 +47,12 @@ class ConnectionToClientTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
+  void ConnectStubs() {
+    viewer_->set_clipboard_stub(&clipboard_stub_);
+    viewer_->set_host_stub(&host_stub_);
+    viewer_->set_input_stub(&input_stub_);
+  }
+
   base::MessageLoop message_loop_;
   MockConnectionToClientEventHandler handler_;
   MockClipboardStub clipboard_stub_;

remoting/protocol/protocol_mock_objects.cc

@@ -10,11 +10,12 @@
 namespace remoting {
 namespace protocol {
 
-MockConnectionToClient::MockConnectionToClient(
-    Session* session,
-    HostStub* host_stub)
-    : ConnectionToClient(session) {
-  set_host_stub(host_stub);
+MockConnectionToClient::MockConnectionToClient(Session* session,
+                                               HostStub* host_stub)
+    : ConnectionToClient(session),
+      clipboard_stub_(nullptr),
+      host_stub_(host_stub),
+      input_stub_(nullptr) {
 }
 
 MockConnectionToClient::~MockConnectionToClient() {}

remoting/protocol/protocol_mock_objects.h

@@ -42,7 +42,23 @@ class MockConnectionToClient : public ConnectionToClient {
   MOCK_METHOD0(session, Session*());
   MOCK_METHOD0(Disconnect, void());
 
+  void set_clipboard_stub(ClipboardStub* clipboard_stub) override {
+    clipboard_stub_ = clipboard_stub;
+  }
+  void set_host_stub(HostStub* host_stub) override { host_stub_ = host_stub; }
+  void set_input_stub(InputStub* input_stub) override {
+    input_stub_ = input_stub;
+  }
+
+  ClipboardStub* clipboard_stub() { return clipboard_stub_; }
+  HostStub* host_stub() { return host_stub_; }
+  InputStub* input_stub() { return input_stub_; }
+
  private:
+  ClipboardStub* clipboard_stub_;
+  HostStub* host_stub_;
+  InputStub* input_stub_;
+
   DISALLOW_COPY_AND_ASSIGN(MockConnectionToClient);
 };