Implement HttpIceConfigRequest

remoting/protocol/http_ice_config_request.cc

+// Copyright 2016 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/protocol/http_ice_config_request.h"
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/json/json_reader.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/values.h"
+#include "net/base/url_util.h"
+#include "remoting/protocol/ice_config.h"
+
+namespace remoting {
+namespace protocol {
+
+namespace {
+
+// Ensure they are active at least one hour after session starts.

[Jamie, 2016/02/18 19:37:38]

s/they/<whatever "they" refers to in this context>/ :)

[Sergey Ulanov, 2016/02/19 20:08:32]

Done.

+const int kMinimumConfigLifetimeSeconds = 3600;
+
+// See draft-petithuguenin-behave-turn-uris-01.
+const int kDefaultStunTurnPort = 3478;
+const int kDefaultTurnsPort = 5349;
+
+base::TimeDelta ParseLifetime(const std::string& string) {
+  double seconds;
+  if (!base::EndsWith(string, "s", base::CompareCase::INSENSITIVE_ASCII) ||
+      !base::StringToDouble(string.substr(0, string.size() - 1), &seconds)) {
+    LOG(ERROR) << "Received invalid lifetimeDuration value: " << string;

[Jamie, 2016/02/18 19:37:38]

For consistency with the other error messages, I think it would be better to log
this in the caller.

[Sergey Ulanov, 2016/02/19 20:08:32]

Done.

+    return base::TimeDelta();
+  }
+  return base::TimeDelta::FromSecondsD(seconds);
+}
+
+// Parses url in form of <stun|turn|turns>:<host>[:<port>][?transport=<udp|tcp>]
+// and adds an entry to the |config|.
+bool AddServerToConfig(std::string url,
+                       const std::string& username,
+                       const std::string& password,
+                       IceConfig* config) {
+  cricket::ProtocolType turn_transport_type = cricket::PROTO_LAST;
+
+  const char kTcpTransportSuffix[] = "?transport=tcp";
+  const char kUdpTransportSuffix[] = "?transport=udp";
+  if (base::EndsWith(url, kTcpTransportSuffix,
+                     base::CompareCase::INSENSITIVE_ASCII)) {
+    turn_transport_type = cricket::PROTO_TCP;
+    url.resize(url.size() - strlen(kTcpTransportSuffix));
+  } else if (base::EndsWith(url, kUdpTransportSuffix,
+                            base::CompareCase::INSENSITIVE_ASCII)) {
+    turn_transport_type = cricket::PROTO_UDP;
+    url.resize(url.size() - strlen(kUdpTransportSuffix));
+  }
+
+  size_t colon_pos = url.find(':');
+  if (colon_pos == std::string::npos)
+    return false;
+
+  std::string protocol = url.substr(0, colon_pos);
+
+  std::string host;
+  int port;
+  if (!net::ParseHostAndPort(url.substr(colon_pos + 1), &host, &port))
+    return false;

[Jamie, 2016/02/18 19:37:37]

Optional: This might be more readable using StringSplit, since you have either
two or three colon-separated components.

[Sergey Ulanov, 2016/02/19 20:08:32]

StringSplit wouldn't work for IPv6 addresses. IPv6 addresses contain colons and
are wrapped with square brackets, e.g. "turn:[2001:4860:4860::8888]:333"

+
+  if (protocol == "stun") {
+    if (port == -1)
+      port = kDefaultStunTurnPort;
+    config->stun_servers.push_back(rtc::SocketAddress(host, port));
+  } else if (protocol == "turn") {
+    if (port == -1)
+      port = kDefaultStunTurnPort;
+    if (turn_transport_type == cricket::PROTO_LAST)
+      turn_transport_type = cricket::PROTO_UDP;
+    config->turn_servers.push_back(cricket::RelayServerConfig(
+        host, port, username, password, turn_transport_type, false));
+  } else if (protocol == "turns") {
+    if (port == -1)
+      port = kDefaultTurnsPort;
+    if (turn_transport_type == cricket::PROTO_LAST)
+      turn_transport_type = cricket::PROTO_TCP;
+    config->turn_servers.push_back(cricket::RelayServerConfig(
+        host, port, username, password, turn_transport_type, true));
+  } else {
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace
+
+HttpIceConfigRequest::HttpIceConfigRequest(
+    UrlRequestFactory* url_request_factory,
+    const std::string& url)
+    : url_(url) {
+  url_request_ =
+      url_request_factory->CreateUrlRequest(UrlRequest::Type::POST, url_);
+  url_request_->SetPostData("application/json", "");
+}
+
+HttpIceConfigRequest::~HttpIceConfigRequest() {}
+
+void HttpIceConfigRequest::Send(const OnIceConfigCallback& callback) {
+  on_ice_config_callback_ = callback;

[Jamie, 2016/02/18 19:37:38]

DCHECK it isn't already set (and that it is set in OnResponse)?

[Sergey Ulanov, 2016/02/19 20:08:31]

Done.

+  url_request_->Start(
+      base::Bind(&HttpIceConfigRequest::OnResponse, base::Unretained(this)));
+}
+
+void HttpIceConfigRequest::OnResponse(const UrlRequest::Result& result) {
+  IceConfig ice_config;
+  if (!result.success) {
+    LOG(ERROR) << "Failed to fetch " << url_;
+    base::ResetAndReturn(&on_ice_config_callback_).Run(ice_config);
+    return;
+  }
+
+  if (result.status != 200) {
+    LOG(ERROR) << "Received status code " << result.status << " from " << url_
+               << ": " << result.response_body;
+    base::ResetAndReturn(&on_ice_config_callback_).Run(ice_config);

[Jamie, 2016/02/18 19:37:38]

In case of error, you seem to be returning as much of the config as you were
able to parse, which means that the caller can't tell anything went wrong. I
think it would make more sense to return an empty config in this case, or
provide some other mechanism to detect errors.

[Sergey Ulanov, 2016/02/19 20:08:32]

In this particular case we are returning a null IceConfig (IceConfig::is_null()
is true), which indicates an error.

+    return;
+  }
+
+  scoped_ptr<base::Value> json = base::JSONReader::Read(result.response_body);
+  base::DictionaryValue* dictionary;
+  base::ListValue* ice_servers_list;
+  std::string lifetime_str;
+  if (!json || !json->GetAsDictionary(&dictionary) ||
+      !dictionary->GetList("iceServers", &ice_servers_list) ||
+      !dictionary->GetString("lifetimeDuration", &lifetime_str)) {
+    LOG(ERROR) << "Received invalid response from " << url_ << ": "
+               << result.response_body;
+    base::ResetAndReturn(&on_ice_config_callback_).Run(ice_config);
+    return;
+  }
+
+  // Parse lifetimeDuration field.
+  base::TimeDelta lifetime = ParseLifetime(lifetime_str);
+  if (lifetime == base::TimeDelta()) {
+    base::ResetAndReturn(&on_ice_config_callback_).Run(ice_config);
+    return;
+  }
+  ice_config.expiration_time =
+      base::Time::Now() + lifetime -
+      base::TimeDelta::FromSeconds(kMinimumConfigLifetimeSeconds);
+
+  // Parse iceServers list.
+  bool errors_found = false;

[Jamie, 2016/02/18 19:37:38]

Why not just return early in case of error? Particularly if you take my
suggestion of returning an empty config instead of parsing as much as you can.

[Sergey Ulanov, 2016/02/19 20:08:32]

When we fail to fetch a config we will still want try to continue with the
connection, but without using STUN/TURN. If we get a config that's we could
parsed only partially then I think it's best try to use parts that we were able
to parse correctly. 

Let's say there is a new TURN version and the server starts returning urls in
the form of "turn2:host:port". AddServerToConfig() will fail to parse such urls.
But we still want to continue using stun and turn servers that were in the
config instead of ignoring the whole config.

+  for (base::Value* server : *ice_servers_list) {
+    base::DictionaryValue* server_dict;
+    if (!server->GetAsDictionary(&server_dict)) {
+      errors_found = true;
+      continue;
+    }
+
+    base::ListValue* urls_list;
+    if (!server_dict->GetList("urls", &urls_list)) {
+      errors_found = true;
+      continue;
+    }
+
+    std::string username;
+    server_dict->GetString("username", &username);
+
+    std::string password;
+    server_dict->GetString("credential", &password);
+
+    for (base::Value* url : *urls_list) {
+      std::string url_str;
+      if (!url->GetAsString(&url_str)) {
+        errors_found = true;
+        continue;
+      }
+      if (!AddServerToConfig(url_str, username, password, &ice_config)) {
+        LOG(ERROR) << "Invalid ICE server URL: " << url_str;

[Jamie, 2016/02/18 19:37:38]

errors_found = true?

[Sergey Ulanov, 2016/02/19 20:08:31]

error_found is there only for structural errors, e.g. when there are no urls
specified for a server. In that case we want to dump the whole config for logs
to make it easier to debug. In this case, when an URL is invalid it's enough to
just display that one URL.

+      }
+    }
+  }
+
+  if (errors_found) {
+    LOG(ERROR) << "Received ICE config from the server that contained errors: "
+               << result.response_body;
+  }
+
+  base::ResetAndReturn(&on_ice_config_callback_).Run(ice_config);
+}
+
+}  // namespace protocol
+}  // namespace remoting