Add gnubby authentication to remoting host

remoting/host/gnubby_auth_handler.h

+// Copyright 2014 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 REMOTING_HOST_GNUBBY_AUTH_HANDLER_H_
+#define REMOTING_HOST_GNUBBY_AUTH_HANDLER_H_
+
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/files/file_path.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/threading/non_thread_safe.h"
+#include "net/socket/stream_listen_socket.h"
+
+namespace base {
+class SingleThreadTaskRunner;
+}  // namespace base
+
+namespace remoting {
+namespace protocol {
+class ClientStub;
+}  // namespace protocol
+
+// Class responsible for proxying authentication data between a local gnubbyd
+// and the client.
+

[Sergey Ulanov, 2014/02/09 22:29:54]

nit: don't need this empty line.

[psj, 2014/02/10 22:57:22]

Done.

+class GnubbyAuthHandler : public base::NonThreadSafe,
+                          public net::StreamListenSocket::Delegate {
+ public:
+  explicit GnubbyAuthHandler(protocol::ClientStub* client_stub);
+  virtual ~GnubbyAuthHandler();
+
+  // StreamListenSocket::Delegate interface.

[Sergey Ulanov, 2014/02/09 22:29:54]

nit: If this interface is not intended to be used externally then it's better to
move this declaration to the private section (inheritance is still public). Not
all code in chromium follows that style, but some does, and it's considered the
right thing to do. In either case it's better to move it after all other public
methods. 

[psj, 2014/02/10 22:57:22]

Done.

+  virtual void DidAccept(net::StreamListenSocket* server,
+                         scoped_ptr<net::StreamListenSocket> socket);

[Sergey Ulanov, 2014/02/09 22:29:54]

add OVERRIDE at then end of overridden method.

[psj, 2014/02/10 22:57:22]

Done.

+  virtual void DidRead(net::StreamListenSocket* socket,
+                       const char* data,
+                       int len);
+  virtual void DidClose(net::StreamListenSocket* socket);
+
+  // A message was received from the client.
+  virtual void DeliverClientMessage(const std::string message);

[Sergey Ulanov, 2014/02/09 22:29:54]

do these methods need to be virtual?

[Sergey Ulanov, 2014/02/09 22:29:54]

use const reference to pass strings. Same for the methods below.

[psj, 2014/02/10 22:57:22]

Done.

[psj, 2014/02/10 22:57:22]

They are virtual for test mocks

+
+  // Send control message to client.
+  virtual void DeliverHostControlMessage(const std::string control_type,
+                                         const std::string data) const;
+
+  // Send data to client.
+  virtual void DeliverHostDataMessage(int connection_id,
+                                      const std::string data) const;
+
+  // Specify the name of the socket to listen to gnubby requests on.
+  static void SetGnubbySocketName(base::FilePath gnubby_socket_name);

[Sergey Ulanov, 2014/02/09 22:29:54]

const base::FilePath&

[Sergey Ulanov, 2014/02/09 22:29:54]

nit: move this after the destructor before all  public, non-static methods.

[psj, 2014/02/10 22:57:22]

Done.

[psj, 2014/02/10 22:57:22]

Done.

+
+  // Returns true if the socket is being tracked as active.
+  bool HasActiveSocketForTesting(net::StreamListenSocket*) const;
+ private:

[Sergey Ulanov, 2014/02/09 22:29:54]

nit: empty line above this one.

[psj, 2014/02/10 22:57:22]

Done.

+  // Create socket for authorization.
+  void CreateAuthorizationSocket();
+
+  // Interfaces through which communication with the client occurs.
+  protocol::ClientStub* client_stub_;
+
+  // Socket used to listen for authorization requests.
+  scoped_ptr<net::StreamListenSocket> auth_socket_;
+
+  // The current gnubby connection id.
+  int current_connection_id_;

[Sergey Ulanov, 2014/02/09 22:29:54]

Maybe last_connection_id_? To make it clear that this is used to generate new
Ids

[psj, 2014/02/10 22:57:22]

Done.

+
+  // Socket and connection id pairs used to process gnubbyd requests
+  typedef std::vector<std::pair<net::StreamListenSocket*, int> > ActiveSockets;

[Sergey Ulanov, 2014/02/09 22:29:54]

Can this be std::map? Then you wouldn't need both CompareSocket() and
CompareConnection()

[psj, 2014/02/10 22:57:22]

No, I need to search both by socket (for incoming request data) and connection
id (for response from webapp)

[Sergey Ulanov, 2014/02/11 08:20:37]

Right, but you can still use find_if() with map<> to search in the reverse
direction direction. It's just seems more natural to use map<> because it's
essentially a map.

[psj, 2014/02/12 09:01:01]

Done.

+  ActiveSockets active_sockets_;
+
+  DISALLOW_COPY_AND_ASSIGN(GnubbyAuthHandler);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_GNUBBY_AUTH_HANDLER_H_