[chrome.dial] Adds chrome.dial.fetchDeviceDecription API.

chrome/browser/extensions/api/dial/device_description_fetcher.cc

+// Copyright (c) 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 "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "chrome/browser/extensions/api/dial/device_description_fetcher.h"
+#include "chrome/browser/extensions/api/dial/dial_device_data.h"
+#include "chrome/browser/profiles/profile.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_status_code.h"
+#include "net/http/http_util.h"
+#include "net/url_request/url_fetcher.h"
+
+using content::BrowserThread;
+
+constexpr char kApplicationUrlHeaderName[] = "Application-URL";
+constexpr int kMaxRetries = 3;
+constexpr int kMaxDescriptionSizeBytes = 262144;

[Wez, 2017/01/05 22:29:24]

nit: For max-size constants it is helpful if you can summarize a rationale based
on what the field might contain, e.g. in this case it can presumably be large if
the device has a lot of functions?

[mark a. foltz, 2017/01/09 21:38:07]

Done.

+
+namespace extensions {
+namespace api {
+namespace dial {
+
+DeviceDescriptionFetcher::DeviceDescriptionFetcher(
+    const GURL& device_description_url,
+    Profile* profile,
+    base::OnceCallback<void(const DialDeviceDescriptionData&)> success_cb,
+    base::OnceCallback<void(const std::string&)> error_cb)
+    : device_description_url_(device_description_url),
+      profile_(profile),
+      success_cb_(std::move(success_cb)),
+      error_cb_(std::move(error_cb)) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(profile_);
+  DCHECK(device_description_url_.is_valid());
+}
+
+DeviceDescriptionFetcher::~DeviceDescriptionFetcher() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+}
+
+void DeviceDescriptionFetcher::Start() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(!fetcher_);

[Wez, 2017/01/05 22:29:24]

nit: I usually recommend a blank line between DCHECK'd pre-conditions and actual
code.

[mark a. foltz, 2017/01/09 21:38:07]

Done.

+  fetcher_ = net::URLFetcher::Create(kURLFetcherID, device_description_url_,
+                                     net::URLFetcher::GET, this);

[Wez, 2017/01/05 22:29:24]

nit: Consider introducing this with e.g. "// uPnP returns device descriptions
via GET request."

[mark a. foltz, 2017/01/09 21:38:06]

Done.

+
+  // The request should not side effect cache or the cookie store or send auth
+  // data.  Proxies almost certainly hurt more cases than they help.

[Wez, 2017/01/05 22:29:24]

nit: It might be more readable to create a local int and |= each set of flags
into it here, e.g:

int flags = 0;

// Never use cookies in these requests.
flags |= ...

which would make it easier to provide helpful comments for each flag.

[mark a. foltz, 2017/01/09 21:38:07]

Reformatted comment to be more explicit about flags.

+  fetcher_->SetLoadFlags(net::LOAD_BYPASS_PROXY | net::LOAD_DISABLE_CACHE |
+                         net::LOAD_DO_NOT_SAVE_COOKIES |
+                         net::LOAD_DO_NOT_SEND_COOKIES |
+                         net::LOAD_DO_NOT_SEND_AUTH_DATA);
+
+  // Technically uPnP allows devices to issue HTTP 307 redirects (sec
+  // 2.1). However, we want to enforce that redirects are not followed to hosts
+  // other than the DIAL device; and there is no logical reason why a device
+  // needs to redirect a request for the device description.
+  //
+  // We can relax this restriction if there are well-behaved devices that
+  // actually need redirects.

[Wez, 2017/01/05 22:29:24]

Presumably we would only do that if we had the ability to intercept the redirect
and verify the constraint that it is to the same device?

Note that Section 5.4 of the current spec says that devices SHALL NOT redirect
this request, though.

[mark a. foltz, 2017/01/09 21:38:07]

Good catch, I was looking at the uPnP spec which does allow certain redirects. 
Updated comment to state we'll never allow them.

[Wez, 2017/01/09 22:12:58]

Acknowledged.

+  fetcher_->SetStopOnRedirect(true);
+
+  // Allow the fetcher to retry on 5XX responses and ERR_NETWORK_CHANGED.
+  fetcher_->SetMaxRetriesOn5xx(kMaxRetries);
+  fetcher_->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries);
+
+  fetcher_->SetRequestContext(profile_->GetRequestContext());
+  fetcher_->Start();
+}
+
+void DeviceDescriptionFetcher::OnURLFetchComplete(
+    const net::URLFetcher* source) {

[Wez, 2017/01/05 22:29:24]

nit: DCHECK_EQ(fetcher_.get(), source);

[mark a. foltz, 2017/01/09 21:38:07]

This code doesn't refer to fetcher_.

[Wez, 2017/01/09 22:12:58]

Doesn't this method expect that |source| is the same URLFetcher that was created
in Start(), though?  I suppose you can argue that it doesn't matter to this code
whether that is the case, or not, so seems legit not to bother checking if you
prefer not to.

[mark a. foltz, 2017/01/10 00:36:13]

Ah, I misread your earlier comment.  Done.

+  if (source->GetResponseCode() != net::HTTP_OK) {
+    ReportError(
+        base::StringPrintf("HTTP %d: Unable to fetch device description",
+                           source->GetResponseCode()));
+    return;
+  }
+
+  const net::HttpResponseHeaders* headers = source->GetResponseHeaders();
+
+  // NOTE: The uPnP spec requires devices to set a Content-Type: header of
+  // text/xml; charset="utf-8" (sec 2.11).  However Chromecast (and possibly
+  // other devices) do not comply, so not checking this header specifcially.

[Wez, 2017/01/05 22:29:24]

typo: specifically

Also, reword: so specifically do not check that header.

Is there much chance that we'd ever want to enable that check, since it seems
unlikely that device behaviours will change much?

Is it the case that devices actually set the field, but with an incorrect value,
or do the misbehaving devices simply omit it?

Have we observed devices that we definitely want to support (e.g. smart TVs?)
that set bogus Content-Type?

[mark a. foltz, 2017/01/09 21:38:07]

I know Cast devices don't comply; presumably they could get an update to fix
this.  If all other devices do comply, then we could turn it on.
So far I've observed Cast devices setting the wrong value.
I can check on  our lab network to see what they do.  We'd probably have to
write an UMA to see how many non-compliant devices there are before adding the
check.  The check doesn't add a ton of value anyway, since XML is the only
possible content that can be served for this request.

[Wez, 2017/01/09 22:12:58]

Acknowledged.

+
+  std::string app_url_header;
+  if (!headers->GetNormalizedHeader(kApplicationUrlHeaderName,
+                                    &app_url_header) ||
+      app_url_header.empty()) {
+    ReportError("Missing or empty Application-URL:");
+    return;
+  }
+
+  // Section 5.4 of the DIAL spec implies that the Application URL should not
+  // have path, query or fragment...unsure if that can be enforced.

[Wez, 2017/01/05 22:29:24]

There seem to be a number of issues of this sort - could the
DeviceDescriptionFetcher perhaps check for these conditions but report them to
the caller (e.g. via getters) rather than enforcing?  That would allow calling
code to report the correllation of usable-DIAL-device with non-compliance so we
can evaluate whether to enforce.

[mark a. foltz, 2017/01/09 21:38:07]

I would probably start with UMA to determine prevalence before investing in a
complete reporting API.  In the meantime we might be able to determine from user
feedback if there are devices that are non-compliant and non-working (and thus
should be flagged as such).

[Wez, 2017/01/09 22:12:58]

Acknowledged.

+  GURL app_url(app_url_header);
+  if (!app_url.is_valid() || !app_url.SchemeIs("http") ||
+      !app_url.HostIsIPAddress() ||

[Wez, 2017/01/05 22:29:24]

Strictly the spec says that the |host| must either resolve to an IPv4 address,
or be one; what you're enforcing here is that it is either an IPv4 or IPv6
address, instead.

I'd suggest simply omitting this check and adding a note, since you already
check that the App URL host is the same as the device description URL host - so
any restrictions on the latter implicitly apply to the former.

[mark a. foltz, 2017/01/09 21:38:07]

Yes.   We don't want domain name resolution here.
I don't think having independent checks is a bad thing.

Also, we don't have restrictions on the device description host in this API,
yet.  Filed crbug.com/679432 to track this work.

[Wez, 2017/01/09 22:12:58]

The DIAL spec appears to allow for DNS names in app URLs, though, though
admittedly it's not clear how you'd actually use those (maybe they have in mind
mDNS..?)
Acknowledged.

+      app_url.host() != device_description_url_.host()) {
+    ReportError(base::StringPrintf("Invalid Application-URL: %s",
+                                   app_url_header.c_str()));
+    return;
+  }
+
+  if (source->GetReceivedResponseContentLength() > kMaxDescriptionSizeBytes) {
+    ReportError("Response too large");
+    return;
+  }
+
+  std::string device_description;
+  if (!source->GetResponseAsString(&device_description) ||
+      device_description.empty()) {
+    ReportError("Missing or empty response");
+    return;
+  }
+
+  if (!base::IsStringUTF8(device_description)) {
+    ReportError("Invalid response encoding");
+    return;
+  }
+
+  std::move(success_cb_)
+      .Run(DialDeviceDescriptionData(device_description, app_url));

[Wez, 2017/01/05 22:29:24]

nit: |device_description| can be up to 256KB in size - std::move() it here?

[mark a. foltz, 2017/01/09 21:38:06]

Done.

+}
+
+void DeviceDescriptionFetcher::OnURLFetchDownloadProgress(
+    const net::URLFetcher* source,
+    int64_t current,
+    int64_t total,
+    int64_t current_network_bytes) {}
+
+void DeviceDescriptionFetcher::OnURLFetchUploadProgress(
+    const net::URLFetcher* source,
+    int64_t current,
+    int64_t total) {}
+
+void DeviceDescriptionFetcher::ReportError(const std::string& message) {
+  std::move(error_cb_).Run(message);

[Wez, 2017/01/05 22:29:24]

I'm not familiar with the new OnceCallback semantics.

Is std::move() now preferred over base::ReturnAndClear(&cb)?

Will std::move() clear the Callback such that a second Run() would DCHECK?  (I
think it will; just confirming :)

[mark a. foltz, 2017/01/09 21:38:07]

That's what the following says:

https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md

[Wez, 2017/01/09 22:12:58]

Acknowledged.

+}
+
+}  // namespace dial
+}  // namespace api
+}  // namespace extensions