Implementation and fixes for Blimp client/engine E2E communication.

blimp/net/engine_authentication_handler.cc

 // Copyright 2015 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 "blimp/net/engine_authentication_handler.h"
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/timer/timer.h"
 #include "blimp/common/proto/blimp_message.pb.h"
@@ skipping 46 matching lines @@
   base::OneShotTimer timeout_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(Authenticator);
 };
 
 Authenticator::Authenticator(
     scoped_ptr<BlimpConnection> connection,
     base::WeakPtr<ConnectionHandler> connection_handler)
     : connection_(std::move(connection)),
       connection_handler_(connection_handler) {
+  DVLOG(1) << "Authenticator object created.";
   connection_->SetConnectionErrorObserver(this);
   connection_->SetIncomingMessageProcessor(this);
   timeout_timer_.Start(
       FROM_HERE, base::TimeDelta::FromSeconds(kAuthTimeoutDurationInSeconds),
       this, &Authenticator::OnAuthenticationTimeout);
 }
 
 Authenticator::~Authenticator() {}
 
 void Authenticator::OnConnectionAuthenticated(bool authenticated) {
-  connection_->SetIncomingMessageProcessor(nullptr);
-  connection_->SetConnectionErrorObserver(nullptr);
-
+  DVLOG(1) << "OnConnectionAuthenticated result=" << authenticated;
   if (authenticated && connection_handler_) {
+    // We expect |connection_handler_| to provide |connection_| with its own
+    // ConnectionErrorObserver and IncomingMessageProcessor objects,

[haibinlu, 2015/12/29 00:51:46]

I prefer to set nullptr as before, rather than rely on the next connection
handler to set it synchronously. And we do not really require "HandleConnection"
to synchronously attach error observer and incoming message processor. 

[Kevin M, 2015/12/30 23:08:49]

Removed ErrorObserver from here.

I'm not sure I understand the reasoning for unsetting them before setting them
again?

+    // so there is no need to unset them beforehand.
     connection_handler_->HandleConnection(std::move(connection_));
   }
-
   delete this;
 }
 
 void Authenticator::OnAuthenticationTimeout() {
   DVLOG(1) << "Connection authentication timeout";
   OnConnectionAuthenticated(false);
 }
 
 void Authenticator::OnConnectionError(int error) {
   DVLOG(1) << "Connection error before authenticated "
            << net::ErrorToString(error);
   OnConnectionAuthenticated(false);
 }
 
 void Authenticator::ProcessMessage(scoped_ptr<BlimpMessage> message,
                                    const net::CompletionCallback& callback) {
   if (message->type() == BlimpMessage::PROTOCOL_CONTROL) {
-    // TODO(haibinlu): check client token.

[haibinlu, 2015/12/29 00:51:46]

keep TODO since you are not verifying the client token.

[Kevin M, 2015/12/30 23:08:49]

Done.

+    DVLOG(1) << "Authentication challenge received: "
+             << message->protocol_control().start_connection().client_token();
     OnConnectionAuthenticated(true);
   } else {
-    DVLOG(1) << "The first message is not START_CONNECTION";
+    DVLOG(1) << "The first message is not START_CONNECTION; got type "
+             << message->type() << " instead.";
     OnConnectionAuthenticated(false);
   }
 
   callback.Run(net::OK);
 }
 
 }  // namespace
 
 EngineAuthenticationHandler::EngineAuthenticationHandler(
     ConnectionHandler* connection_handler)
     : connection_handler_weak_factory_(connection_handler) {}
 
 EngineAuthenticationHandler::~EngineAuthenticationHandler() {}
 
 void EngineAuthenticationHandler::HandleConnection(
     scoped_ptr<BlimpConnection> connection) {
   // Authenticator manages its own lifetime.
   new Authenticator(std::move(connection),
                     connection_handler_weak_factory_.GetWeakPtr());
 }
 
 }  // namespace blimp