[Blimp Net] Add EngineAuthHandler.

blimp/net/engine_auth_handler.h

+// 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.
+
+#ifndef BLIMP_NET_ENGINE_AUTH_HANDLER_H_
+#define BLIMP_NET_ENGINE_AUTH_HANDLER_H_
+
+#include <vector>
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/timer/timer.h"
+#include "blimp/net/blimp_message_processor.h"
+#include "blimp/net/blimp_net_export.h"
+#include "blimp/net/connection_error_observer.h"
+#include "blimp/net/connection_handler.h"
+#include "net/base/completion_callback.h"
+
+namespace blimp {
+
+class BlimpConnection;
+class BlimpMessage;
+
+// Authenticates connections and hand them over to |connection_handler|
+// if it is authenticated.
+class BLIMP_NET_EXPORT EngineAuthHandler : public ConnectionHandler {
+ public:
+  // Caller is responsible for ensuring that |connection_handler| outlives
+  // |this|.
+  explicit EngineAuthHandler(ConnectionHandler* connection_handler);
+
+  ~EngineAuthHandler() override;
+
+  // ConnectionHandler implementation.
+  void HandleConnection(scoped_ptr<BlimpConnection> connection) override;
+
+ private:
+  using AuthCallback =
+      base::Callback<void(scoped_ptr<BlimpConnection> connection,
+                          bool isAuthenticated)>;

[Kevin M, 2015/12/03 19:11:36]

No camelCase for variable names.

[haibinlu, 2015/12/03 22:28:35]

Done.

+
+  // Authenticates one connection. |auth_callback| is invoked when
+  //   * the connection is authenticated.
+  //   * the connection gets into error state.
+  //   * timeout on waiting for auth message.
+  class Authenticator : public ConnectionErrorObserver,
+                        public BlimpMessageProcessor {
+   public:
+    explicit Authenticator(scoped_ptr<BlimpConnection> connection);
+    ~Authenticator() override;
+
+    void Start(const AuthCallback& auth_callback);
+
+   private:
+    // ConnectionErrorObserver implementation.

[Kevin M, 2015/12/03 19:11:36]

Move overrides to the bottom of the methods

[haibinlu, 2015/12/03 22:28:35]

Done.

+    // Used to implement reconnection logic on unexpected disconnections.
+    void OnConnectionError(int error) override;
+
+    // BlimpMessageProcessor implementation.
+    void ProcessMessage(scoped_ptr<BlimpMessage> message,
+                        const net::CompletionCallback& callback) override;
+
+    void InvokeAuthCallback(bool isAuthenticated);
+
+    // Invoked when timeout on waiting for auth message.

[Kevin M, 2015/12/03 19:11:36]

nit: on timeout while 

[haibinlu, 2015/12/03 22:28:35]

Done.

+    void FailAuthetication();
+
+    // The connection to be authenticated.
+    scoped_ptr<BlimpConnection> connection_;
+
+    // Callback for authentication result;
+    AuthCallback auth_callback_;
+
+    // A timer to fail authentication on timeout.
+    scoped_ptr<base::OneShotTimer> timer_;

[Kevin M, 2015/12/03 19:11:36]

Move this out of a scoped_ptr? It's never going to be used more than once, so no
need to allocate it as a pointer.

[haibinlu, 2015/12/03 22:28:35]

Done.

+
+    DISALLOW_COPY_AND_ASSIGN(Authenticator);
+  };
+
+  // Callback when |authenticator| has the authentication result.
+  void OnAuthResult(Authenticator* authenticator,
+                    scoped_ptr<BlimpConnection> connection,
+                    bool isAuthenticated);
+
+  void RemovePendingAuthenticator(Authenticator* authenticator);
+
+  // Handler for authenticated connections.
+  ConnectionHandler* connection_handler_;
+
+  // Pending authentications. An authenticator is removed from this list once
+  // it has the authentication result.
+  std::vector<scoped_ptr<Authenticator>> pending_auths_;

[Kevin M, 2015/12/03 19:11:36]

What's the purpose of tracking the pending authentication? I'm not seeing cases
in the code where we put this vector to good use. Can we postpone centralized
management of Authenticators until we have a solid use case?

[haibinlu, 2015/12/03 22:28:35]

Wez@, what do you think?

Since we are not doing anything special about these authenticators right now, I
am fine either way.

It is actually clean for this object to own these authenticators, and no need
for weakptr factory.  

+

[Kevin M, 2015/12/03 19:11:36]

Add WeakPtrFactory<> for callbacks to Authenticator.

+  DISALLOW_COPY_AND_ASSIGN(EngineAuthHandler);
+};
+
+}  // namespace blimp
+
+#endif  // BLIMP_NET_ENGINE_AUTH_HANDLER_H_