Add status service for remoting host.

remoting/host/host_status_service.cc

+// Copyright (c) 2012 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 "remoting/host/host_status_service.h"
+
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/stl_util.h"
+#include "base/string_number_conversions.h"
+#include "base/string_util.h"
+#include "base/stringize_macros.h"
+#include "base/values.h"
+#include "net/base/ip_endpoint.h"
+#include "net/base/net_util.h"
+#include "remoting/host/websocket_connection.h"
+#include "remoting/host/websocket_listener.h"
+
+namespace remoting {
+
+namespace {
+
+// HostStatusService uses the first port available in the following range.
+const int kPortRangeMin = 12810;

[alexeypa (please no reviews), 2012/11/15 19:45:56]

Does this range need a firewall rule on Windows?

[Sergey Ulanov, 2012/11/16 00:52:10]

We are connecting from localhost to localhost. Are localhost->localhost
connections blocked by the firewall?

+const int kPortRangeMax = 12820;
+
+const char kChromeExtensionProtocolPrefix[] = "chrome-extension://";

[Wez, 2012/11/15 23:34:59]

You should really change this to kChromeExtensionUrlScheme (without the ://) and
use google_url to split down the origin to check the scheme.

[Sergey Ulanov, 2012/11/16 00:52:10]

I considered this, but I don't think using GURL for parsing origin is the right
thing to do: the origin is not supposed to be a full url - it's just scheme +
domain. Not using it also makes code simpler.
Renamed this const to kChromeExtensionUrlSchemePrefix.

+
+#ifdef NDEBUG

[Wez, 2012/11/15 23:34:59]

nit: #if !defined(NDEBUG)

[Sergey Ulanov, 2012/11/16 00:52:10]

Changed to "#if defined(NDEBUG)",

+const char* kAllowedWebApplicationIds[] = {
+  "gbchcmhmhahfdphkhkmpfmihenigjmpp",  // Chrome Remote Desktop
+  "kgngmbheleoaphbjbaiobfdepmghbfah",  // Pre-release Chrome Remote Desktop
+  "odkaodonbgfohohmklejpjiejmcipmib",  // Dogfood Chrome Remote Desktop
+  "ojoimpklfciegopdfgeenehpalipignm",  // Chromoting canary
+};
+#endif
+
+}  // namespace
+
+class HostStatusService::Connection : public WebsocketConnection::Delegate {
+ public:
+  Connection(HostStatusService* service,
+             scoped_ptr<WebsocketConnection> websocket)
+      : service_(service),

[alexeypa (please no reviews), 2012/11/15 19:45:56]

nit: Add a comment explaining that |service| must outlive |this|.

[Sergey Ulanov, 2012/11/16 00:52:10]

Done, but I don't think its really necessary because Connection class is
internal to HostStatusService.

+        websocket_(websocket.Pass()) {
+    websocket_->Accept(this);

[alexeypa (please no reviews), 2012/11/15 19:45:56]

I believe this constructor is too complex to define it in the class declaration.
Please move it down next of other methods of this class.

[Sergey Ulanov, 2012/11/16 00:52:10]

Done.

+  }
+  virtual ~Connection() {

[alexeypa (please no reviews), 2012/11/15 19:45:56]

nit: it is not required by the style guide but it will look nice of the
destructor's body will be defined next to all other methods of the class.

[Wez, 2012/11/15 23:34:59]

I'd prefer it be left in-line, and on a single line.

[Sergey Ulanov, 2012/11/16 00:52:10]

Done.

[Sergey Ulanov, 2012/11/16 00:52:10]

I've already moved it, so I'll leave it there. 

+  }
+
+  // WebsocketConnection::Delegate interface.
+  virtual void OnWebsocketMessage(const std::string& message) OVERRIDE;
+  virtual void OnWebsocketClosed() OVERRIDE;
+
+ private:
+  void SendHostStatusMessage();

[alexeypa (please no reviews), 2012/11/15 19:45:56]

nit: Add comments explaining what these methods are for.

[Sergey Ulanov, 2012/11/16 00:52:10]

Done.

+  void SendMessage(const std::string& method,
+                   scoped_ptr<base::DictionaryValue> data);
+
+   // Closes this connection. Destroys the object, so should be used with care.

[Wez, 2012/11/15 23:34:59]

nit: Reword e.g "Closes the connection and destroys this object."

Use with care should be implicit. ;)

[Sergey Ulanov, 2012/11/16 00:52:10]

Done.

+  void CloseOnError();

[Wez, 2012/11/15 23:34:59]

nit: CloseOnError sounds like it tells the thing to close iff there's an error;
I think you mean to close the connection and report an error?

[Sergey Ulanov, 2012/11/16 00:52:10]

It means "close connection because there was a protocol error". Renamed to just
Close()

+
+  HostStatusService* service_;
+  scoped_ptr<WebsocketConnection> websocket_;
+
+  DISALLOW_COPY_AND_ASSIGN(Connection);
+};
+
+void HostStatusService::Connection::OnWebsocketMessage(
+    const std::string& message) {
+  scoped_ptr<base::Value> json(

[alexeypa (please no reviews), 2012/11/15 19:45:56]

nit: Can anything bad happen if a malicious sender sends a bad message? What if
the message is very large? Malformed? Can it be detected early, so we will not
need to spend time parsing megabytes of text?

[Sergey Ulanov, 2012/11/16 00:52:10]

That's a good point. I think it's better to limit it on the lower level - in
WebsocketConnection. Added call set_maximum_message_size() there and which is
now called from Connection() constructor.

+      base::JSONReader::Read(message, base::JSON_ALLOW_TRAILING_COMMAS));
+
+  base::DictionaryValue* message_dict = NULL;

[Wez, 2012/11/15 23:34:59]

nit: Add a comment before this block explaining what we're doing.

[Sergey Ulanov, 2012/11/16 00:52:10]

Done.

+  std::string method;
+  base::DictionaryValue* data = NULL;

[Wez, 2012/11/15 23:34:59]

|data| doesn't seem to be used?

[Sergey Ulanov, 2012/11/16 00:52:10]

We may need it in the future when we add some new methods, e.g. client may need
to pass its version to the host. On this level this messaging protocol is
identical to how client plugin and webapp communicate.

+  if (!json.get() ||
+      !json->GetAsDictionary(&message_dict) ||
+      !message_dict->GetString("method", &method) ||
+      !message_dict->GetDictionary("data", &data)) {
+    LOG(ERROR) << "Received invalid message: " << message;
+    CloseOnError();
+    return;
+  }
+
+  if (method == "getHostStatus") {
+    SendHostStatusMessage();
+  } else {
+    LOG(ERROR) << "Received message with unknown method: " << message;

[alexeypa (please no reviews), 2012/11/15 19:45:56]

Call CloseOnError() here?

[Sergey Ulanov, 2012/11/16 00:52:10]

Actually I think it's better to keep it connected, for backward compatibility,
when we add new messages to the client. Also changed it to respond with
"unsupportedMethod" message.

+    return;
+  }
+}
+
+void HostStatusService::Connection::OnWebsocketClosed() {
+  service_->OnConnectionClosed(this);

[Wez, 2012/11/15 23:34:59]

You reset |websocket_| in CloseOnError() but not here; why not?  If
CloseOnError() deletes this object, does OnWebsocketClosed() not do the same?

[Sergey Ulanov, 2012/11/16 00:52:10]

Changed this to Close().

+}
+
+void HostStatusService::Connection::SendHostStatusMessage() {
+  SendMessage("hostStatus", service_->GetStatusMessage());
+}
+
+void HostStatusService::Connection::SendMessage(
+    const std::string& method,
+    scoped_ptr<base::DictionaryValue> data) {
+  scoped_ptr<base::DictionaryValue> message(new base::DictionaryValue());
+  message->SetString("method", method);
+  message->Set("data", data.release());
+
+  std::string message_json;
+  base::JSONWriter::Write(message.get(), &message_json);
+  websocket_->SendText(message_json);
+}
+
+void HostStatusService::Connection::CloseOnError() {
+  websocket_.reset();
+  service_->OnConnectionClosed(this);
+}
+
+HostStatusService::HostStatusService()
+    : started_(false) {
+  char ip[] = {127, 0, 0, 1};

[alexeypa (please no reviews), 2012/11/15 19:45:56]

Will it still work in presence of IPv6?

[Sergey Ulanov, 2012/11/16 00:52:10]

Yes. On Linux localhost normally resolves to IPv4 address only (and localhost6
is used for IPv6 addresses). On Mac and Windows localhost resolves to both ::1
and 127.0.0.1. If a domain is resolved to both IPv4 and IPv6 addresses then the
browser should try to connect to both, so it should still be able to connect if
we listen just on the IPv4 port.

+  net::IPAddressNumber localhost(ip, ip + sizeof(ip));
+  for (int port = kPortRangeMin; port < kPortRangeMax; ++port) {
+    net::IPEndPoint endpoint(localhost, port);
+    if (websocket_listener_.Listen(
+            endpoint, base::Bind(&HostStatusService::OnNewConnection,

[Wez, 2012/11/15 23:34:59]

nit: Moving endpoint to the previous line would make this more readable, IMHO.

[Sergey Ulanov, 2012/11/16 00:52:10]

No, it doesn't, because that would mean that the second parameter has to be
aligned with (, which makes this line longer than 80 chars. Anyway moved
|endpoint| to a separate line, but that doesn't improve readability, IMHO.

+                                 base::Unretained(this)))) {

[alexeypa (please no reviews), 2012/11/15 19:45:56]

How is it guaranteed that |this| will not be deleted before the callback is
invoked? Please add a comment. Or try to get rid of Unretained if possible.

[Sergey Ulanov, 2012/11/16 00:52:10]

We own websocket_listener_ which is guaranteed to call the callback only while
it still exists.

+      host_name_ = "localhost:" + base::UintToString(port);
+      LOG(INFO) << "Listening for WebSocket connections on localhost:" << port;
+      break;
+    }
+  }
+}
+
+HostStatusService::~HostStatusService() {
+  STLDeleteElements(&connections_);

[alexeypa (please no reviews), 2012/11/15 19:45:56]

nit: Should |websocket_| be reset first?

[Wez, 2012/11/15 23:34:59]

It's held in a scoped_ptr<> in Connection, so that should be implicit, surely?

[Sergey Ulanov, 2012/11/16 00:52:10]

It doesn't matter. websocket_listener_ is not connected to connections_ in any
way.

+}
+
+void HostStatusService::SetState(bool started, const std::string& host_id) {
+  started_ = started;
+  host_id_ = host_id;
+}
+
+bool HostStatusService::IsAllowedOrigin(const std::string& origin) {

[Wez, 2012/11/15 23:34:59]

Use GURL to break down the URL first, and then debug/non-debug enforcement based
on the results.

[Sergey Ulanov, 2012/11/16 00:52:10]

As explained above I think it would be overkill to use GURL here because we are
not parsing URLs.

+#ifndef NDEBUG

[alexeypa (please no reviews), 2012/11/15 19:45:56]

Why is the difference? If it is just a sanity check that the main check should
be outside of #ifdef

[Sergey Ulanov, 2012/11/16 00:52:10]

We want to limit which webapps can connect to the webservice. Problem is that it
doesn't work well with a locally-built client which may have some random ID, so
just to simplify debugging debug builds accept connections from any chrome
webapp. Alternative solution would be to add parameter (e.g. in the config) that
would allow whitelist specific app IDs, but it would make it harder to debug
this code, but may still be useful. Added TODO about it.

+  // Allow all chrome extensions in Debug builds.
+  return StartsWithASCII(origin, kChromeExtensionProtocolPrefix, false);
+#else
+  std::string prefix(kChromeExtensionProtocolPrefix);
+  for (int i = 0; i < arraysize(kAllowedWebApplicationIds)) {
+    if (origin == prefix + kAllowedWebApplicationIds[i]) {
+      return true;
+    }
+  }
+  return false;
+#endif
+}
+
+void HostStatusService::OnNewConnection(
+    scoped_ptr<WebsocketConnection> connection) {
+  bool accept = true;

[alexeypa (please no reviews), 2012/11/15 19:45:56]

nit: It is better to set |accept| to false by default and only set it to try
when all checks pass.

[Wez, 2012/11/15 23:34:59]

I think this would be more readable as a series of:

// Check that the request host is valid.
if (...) {
  ...
  connection->Reject();
  return;
}

clauses, rather than a sequence of if/else clauses and a flag.

[Sergey Ulanov, 2012/11/16 00:52:10]

Done.

[Sergey Ulanov, 2012/11/16 00:52:10]

Done.

+
+  if (connection->request_host() != host_name_) {
+    LOG(ERROR) << "Received connection for invalid host: "
+               << connection->request_host()
+               << ". Expected " << host_name_;
+    accept = false;
+  } else if (connection->request_path() != "/remoting_host_status") {
+    LOG(ERROR) << "Received connection for unknown path: "
+               << connection->request_path();
+    accept = false;
+  } else if (!IsAllowedOrigin(connection->origin())) {

[alexeypa (please no reviews), 2012/11/15 19:45:56]

Can connection->origin() be trusted? Can the connecting party spoof it?

[Wez, 2012/11/15 23:34:59]

The origin protects from XSS; it's assumed the request was initiated by
web-content, in which case the browser will have added the origin, so this is
fine so long as you trust the browser.

[Sergey Ulanov, 2012/11/16 00:52:10]

A web site cannot spoof it, but any locally installed code (or even a Chrome app
that has access to TCP APIs) can. The main point is to block connections from
drive-by web apps.

+    LOG(ERROR) << "Rejecting connection from unknown origin: "
+               << connection->origin();
+    accept = false;
+  }
+
+  if (accept) {
+    connections_.insert(new Connection(this, connection.Pass()));
+  } else {
+    connection->Reject();
+  }
+}
+
+void HostStatusService::OnConnectionClosed(Connection* connection) {
+  connections_.erase(connection);

[Wez, 2012/11/15 23:34:59]

This doesn't seem to delete Connection?

[Sergey Ulanov, 2012/11/16 00:52:10]

Good catch. Fixed.

+}
+
+scoped_ptr<base::DictionaryValue> HostStatusService::GetStatusMessage() {
+  scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue());
+  result->SetString("state", started_ ? "STARTED" : "STOPPED");
+  result->SetString("version", STRINGIZE(VERSION));
+  result->SetString("hostId", host_id_);
+  return result.Pass();
+}
+
+}  // namespace remoting