[Blimp Net] Add EngineAuthHandler.

blimp/net/engine_authentication_handler.cc

Index: blimp/net/engine_authentication_handler.cc
diff --git a/blimp/net/engine_authentication_handler.cc b/blimp/net/engine_authentication_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a9527eb8fb9eccbd4b062124814ea3feb472b181
--- /dev/null
+++ b/blimp/net/engine_authentication_handler.cc
@@ -0,0 +1,124 @@
+// 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"
+#include "blimp/net/blimp_connection.h"
+#include "blimp/net/blimp_message_processor.h"
+#include "blimp/net/blimp_transport.h"
+#include "blimp/net/connection_error_observer.h"
+#include "net/base/completion_callback.h"
+#include "net/base/net_errors.h"
+
+namespace blimp {
+
+namespace {
+// Expect authentication to be done in 10 seconds.

[Wez, 2015/12/08 00:31:41]

nit: I think you can be more explicit "Expect Client to send the StartConnection
within ten seconds of becoming connected."

[haibinlu, 2015/12/08 01:54:50]

Done.

+const double kAuthTimeoutDurationInSeconds = 10;
+
+// Authenticates one connection. It deletes itself when
+//   * the connection is authenticated and passed to |connection_handler|.
+//   * the connection gets into error state.

[Wez, 2015/12/08 00:31:41]

nit: into an error state

[haibinlu, 2015/12/08 01:54:50]

Done.

+//   * timeout on waiting for auth message.

[Wez, 2015/12/08 00:31:41]

nit: the auth message does not arrive within a reasonable time.

[haibinlu, 2015/12/08 01:54:50]

Done.

+class Authenticator : public ConnectionErrorObserver,
+                      public BlimpMessageProcessor {
+ public:
+  explicit Authenticator(scoped_ptr<BlimpConnection> connection,
+                         base::WeakPtr<ConnectionHandler> connection_handler);
+  ~Authenticator() override;
+
+ private:
+  // Processes authentication result and delete this object.

[Wez, 2015/12/08 00:31:41]

nit: delete->deletes

nit: You can also write "this object" as "|this|" in comments of this sort.

[haibinlu, 2015/12/08 01:54:50]

Done.

+  void OnConnectionAuthenticated(bool authenticated);
+
+  // Invoked on timeout while waiting for auth message.

[Wez, 2015/12/08 00:31:41]

nit: Reword this similarly to the processes one, above, e.g. "Handles timeout
waiting for auth message, and deletes |this|."

[haibinlu, 2015/12/08 01:54:50]

Done.

+  void OnAuthenticationTimeout();
+
+  // ConnectionErrorObserver implementation.
+  void OnConnectionError(int error) override;
+
+  // BlimpMessageProcessor implementation.
+  void ProcessMessage(scoped_ptr<BlimpMessage> message,
+                      const net::CompletionCallback& callback) override;
+
+  // The connection to be authenticated.
+  scoped_ptr<BlimpConnection> connection_;
+
+  // Handler for authenticated connections.

[Wez, 2015/12/08 00:31:41]

nit: Handler to pass successfully authentication connections to.

[haibinlu, 2015/12/08 01:54:50]

Done.

+  base::WeakPtr<ConnectionHandler> connection_handler_;
+
+  // A timer to fail authentication on timeout.
+  base::OneShotTimer timer_;

[Wez, 2015/12/08 00:31:41]

nit: timeout_ or timeout_timer_?

[haibinlu, 2015/12/08 01:54:50]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(Authenticator);
+};
+
+Authenticator::Authenticator(
+    scoped_ptr<BlimpConnection> connection,
+    base::WeakPtr<ConnectionHandler> connection_handler)
+    : connection_(std::move(connection)),
+      connection_handler_(connection_handler) {
+  connection_->SetConnectionErrorObserver(this);
+  connection_->SetIncomingMessageProcessor(this);
+  timer_.Start(FROM_HERE,
+               base::TimeDelta::FromSecondsD(kAuthTimeoutDurationInSeconds),

[Wez, 2015/12/08 00:31:41]

nit: Why are we using FromSecondsD() here, rather than having an int64
auth-timeout duration constant? Do we need sub-second granularity..?

[haibinlu, 2015/12/08 01:54:50]

No, we do not right now. Kevin prefered FromSecondsD. Updated to FromSeconds.

+               this, &Authenticator::OnAuthenticationTimeout);
+}
+
+Authenticator::~Authenticator() {}
+
+void Authenticator::OnConnectionAuthenticated(bool authenticated) {
+  timer_.Stop();

[Wez, 2015/12/08 00:31:41]

nit: We are about to delete |this|, which will delete |timer_|, which will
StopAndAbandon() it - do we need to explicitly Stop() it here?

[haibinlu, 2015/12/08 01:54:50]

right. no need to stop timer here.

+  connection_->SetIncomingMessageProcessor(nullptr);
+  connection_->SetConnectionErrorObserver(nullptr);
+
+  if (authenticated) {
+    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";

[Wez, 2015/12/08 00:31:41]

nit: Log the actual error code.

[haibinlu, 2015/12/08 01:54:50]

Done.

+  OnConnectionAuthenticated(false);
+}
+
+void Authenticator::ProcessMessage(scoped_ptr<BlimpMessage> message,
+                                   const net::CompletionCallback& callback) {
+  if (message->type() == BlimpMessage::PROTOCOL_CONTROL) {
+    // TODO(haibinlu): check client token.
+    OnConnectionAuthenticated(true);
+  } else {
+    OnConnectionAuthenticated(false);

[Wez, 2015/12/08 00:31:41]

nit: Perhaps a DVLOG(1) here, as you have above, would be appropriate?

[haibinlu, 2015/12/08 01:54:50]

Done.

+  }
+
+  callback.Run(net::OK);

[Wez, 2015/12/08 00:31:41]

nit: We should update MessageProcessor (separate CL) to clarify that it's OK for
ProcessMessage() to invoke the callback synchronously.

[haibinlu, 2015/12/08 01:54:50]

Acknowledged. 

+}
+
+}  // 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