Add AffiliationFetcher to fetch authoritative affiliation information regarding facets.

components/password_manager/core/browser/affiliation_fetcher.cc

Index: components/password_manager/core/browser/affiliation_fetcher.cc
diff --git a/components/password_manager/core/browser/affiliation_fetcher.cc b/components/password_manager/core/browser/affiliation_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f4058420a36409d418f17d587ed7eb5a273844e1
--- /dev/null
+++ b/components/password_manager/core/browser/affiliation_fetcher.cc
@@ -0,0 +1,225 @@
+// 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.
+
+#include "components/password_manager/core/browser/affiliation_fetcher.h"
+
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/values.h"
+#include "components/password_manager/core/browser/affiliation_utils.h"
+#include "google_apis/google_api_keys.h"
+#include "net/base/load_flags.h"
+#include "net/base/url_util.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+namespace password_manager {
+
+AffiliationFetcher::AffiliationFetcher(
+    net::URLRequestContextGetter* request_context_getter,
+    const std::vector<FacetURI>& facet_uris,
+    AffiliationFetcherDelegate* delegate)
+    : request_context_getter_(request_context_getter),
+      requested_facet_uris_(facet_uris),
+      delegate_(delegate) {
+  for (const FacetURI& uri : requested_facet_uris_)
+    DCHECK(uri.is_valid());
+}
+
+AffiliationFetcher::~AffiliationFetcher() {
+}
+
+// static
+AffiliationFetcher* AffiliationFetcher::Create(
+    net::URLRequestContextGetter* context_getter,
+    const std::vector<FacetURI>& facet_uris,
+    AffiliationFetcherDelegate* delegate) {
+  return new AffiliationFetcher(context_getter, facet_uris, delegate);
+}
+
+void AffiliationFetcher::StartRequest() {
+  DCHECK(!fetcher_);
+
+  fetcher_.reset(
+      net::URLFetcher::Create(BuildQueryURL(), net::URLFetcher::POST, this));
+  fetcher_->SetRequestContext(request_context_getter_.get());
+  fetcher_->SetUploadData("application/json", PreparePayload());
+  fetcher_->SetLoadFlags(
+      net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES |
+      net::LOAD_DO_NOT_SEND_AUTH_DATA | net::LOAD_BYPASS_CACHE |
+      net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_PROMPT_FOR_LOGIN);

[Ryan Sleevi, 2014/12/10 19:44:36]

Why LOAD_DISABLE_CACHE? Seems emminently sensible that this could potentially be
cached for some duration, at the HTTP layer, negating some of the
application-level complexity.

[engedy, 2014/12/11 20:49:23]

Most of the time the lookups will be performed in the reverse direction. For an
example of what this means, assume that the user has a password stored for
Android app A, where A is affiliated with website B, and so the password from A
should be offered to be filled into B. Obviously, as the user visits websites
W_1, W_2, ..., W_n, Chrome should not issue a request to look up affiliates for
each website W_i, just because of the odd chance of encountering W_i = B at one
point.

Instead, it will query the affiliates of A and preload them into a "smarter"
cache that can be later queried in the reverse direction, i.e., yielding A when
queried for W_i = B. As this smart cache will be needed anyway, I do not see a
need to do caching at the HTTP layer too.

In fact, I think caching at the HTTP layer is somewhat harmful, as it makes the
freshness level of the data less clear, and we want to offer relatively strict
guarantees regarding how soon Chrome will forget that A and B are affiliated
with each other once the relation is removed from the server side database.

Please let me know if I have misunderstood something, and HTTP caching would be,
in fact, useful in this case.

+  fetcher_->SetAutomaticallyRetryOn5xx(false);
+  fetcher_->SetAutomaticallyRetryOnNetworkChanges(0);
+  fetcher_->Start();
+}
+
+GURL AffiliationFetcher::BuildQueryURL() const {
+  return net::AppendQueryParameter(
+      GURL("https://www.googleapis.com/affiliation/v1/affiliation:lookup"),
+      "key", google_apis::GetAPIKey());
+}
+
+std::string AffiliationFetcher::PreparePayload() const {
+  // The request payload should be a JSON dictionary with a single key named
+  // "facet" that maps to the list of facet URIs to query:
+  //
+  //   {
+  //     "facet": [<requested_facet_uri_1>, ..., <requested_facet_uri_N>]
+  //   }
+
+  scoped_ptr<base::ListValue> requested_facet_uris_list(new base::ListValue);
+  for (const FacetURI& uri : requested_facet_uris_)
+    requested_facet_uris_list->AppendString(uri.canonical_spec());
+
+  base::DictionaryValue payload_dictionary;
+  payload_dictionary.Set("facet", requested_facet_uris_list.release());
+
+  std::string payload_json;
+  bool serialization_suceeded =
+      base::JSONWriter::Write(&payload_dictionary, &payload_json);
+  DCHECK(serialization_suceeded);
+  return payload_json;
+}
+
+bool AffiliationFetcher::ParseResponse(
+    AffiliationFetcherDelegate::Result* result) const {
+  // The response payload will be a JSON dictionary containing the key named
+  // "affiliation" that maps to the list of equivalence classes, each of which
+  // will be a dictionary itself with a key named "facet" that maps to the list
+  // of facet URIs in that class:
+  //
+  //   {
+  //     "affiliation": [
+  //       {
+  //         "facet": [<eq_class_1_facet_1>, ..., <eq_class_1_facet_N1>]
+  //       },
+  //       ...
+  //       {
+  //         "facet": [<eq_class_M_facet_1>, ..., <eq_class_M_facet_Nm>]
+  //       }
+  //   }
+
+  std::string response_json;
+  if (!fetcher_->GetResponseAsString(&response_json))
+    NOTREACHED();
+
+  scoped_ptr<const base::Value> response_value(
+      base::JSONReader::Read(response_json));

[Ryan Sleevi, 2014/12/10 19:44:36]

SECURITY: Parsing JSON inside the Browser process, received from remote
endpoints, is DANGEROUS and NOT ENCOURAGED.

We have a utility in //chrome to defer to the browser process.

Alternatively, using protobufs is blessed for security.

Put differently, our base::JSONReader has not at all been fuzzed / vetted to
take arbitrary data. I realize this is data "from us", but it's in more of the
grey area here, so it's easier to avoid the potential red flags.

[Garrett Casto, 2014/12/11 08:18:39]

Protocol buffers also look like they would save you about 30 lines worth of
parsing code, which would be nice.

[engedy, 2014/12/11 20:49:23]

Thanks for the heads-up! The server side support protobufs as well, so...
Protobuf it is. Done. 

+  if (!response_value) {
+    VLOG(1) << "Response could not be parsed as JSON.";
+    VLOG(1) << response_json;

[Ryan Sleevi, 2014/12/10 19:44:36]

These are fairly chatty VLOGs. I want to make sure that the benefit is
justified; it's easy to miss the hidden costs of logging and suffer from death
by a thousand strings.

Does a DVLOG make more sense here? Or alternatively omitting these until they're
needed for debugging? I'm not a fan of printf style debugging in production
code, unless there's demonstrated need.

[Garrett Casto, 2014/12/11 08:18:38]

+1 to DVLOG. If you're actually worried about these error conditions, adding UMA
stats are a better way to keep track of this. VLOG only really seems useful if
you have an external reporter that is experiencing a problem that you can't
reproduce.  

[engedy, 2014/12/11 20:49:24]

Removed logging statements -- Given that the Fetcher is using protobufs now,
there is no more need, really.

+    return false;
+  }
+
+  const base::DictionaryValue* response_dict;
+  if (!response_value->GetAsDictionary(&response_dict)) {
+    VLOG(1) << "Response is not a JSON object.";
+    VLOG(1) << *response_value;
+    return false;
+  }
+
+  const base::ListValue* equivalence_classes_list;
+  if (!response_dict->GetList("affiliation", &equivalence_classes_list)) {
+    VLOG(1) << "Response contains no 'affiliation' list.";
+    VLOG(1) << *response_value;
+    return false;
+  }
+
+  // Reserve enough space to avoid costly reallocations. We will return at most
+  // one equivalence class per requested facet, so use that as an upper bound.
+  // Note that the size of |equivalence_classes_list| is not necessarily enough,
+  // as it may be missing classes artificially added by this function later.

[Ryan Sleevi, 2014/12/10 19:44:36]

This is a pretty lengthy comment for what is implicit in the code.

Further, the "costly reallocation" isn't backed up with data, and so it's a
comment that can mislead. Costly where, for who, why, etc.

It's easier to just let the idiom of "reserve what you need" apply, as a skilled
C++ developer should instantly recognize the pattern.

[engedy, 2014/12/11 20:49:23]

Done (removed comment).

+  result->reserve(requested_facet_uris_.size());
+
+  std::map<FacetURI, size_t> facet_uri_to_class_index;
+  for (size_t i = 0; i < equivalence_classes_list->GetSize(); ++i) {
+    const base::DictionaryValue* equivalence_class_dict;
+    if (!equivalence_classes_list->GetDictionary(i, &equivalence_class_dict)) {
+      VLOG(1) << "An element of the 'affiliation' list is not a JSON object.";
+      VLOG(1) << *equivalence_classes_list;
+      return false;
+    }
+
+    const base::ListValue* equivalence_class_members_list;
+    if (!equivalence_class_dict->GetList("facet",
+                                         &equivalence_class_members_list)) {
+      VLOG(1) << "Equivalence class JSON object contains no 'facet' list.";
+      VLOG(1) << *equivalence_class_dict;
+      return false;
+    }
+
+    AffiliatedFacets affiliated_uris;
+    for (size_t j = 0; j < equivalence_class_members_list->GetSize(); ++j) {
+      std::string uri_spec;
+      if (!equivalence_class_members_list->GetString(j, &uri_spec)) {
+        VLOG(1) << "An element of a 'facet' list is not string.";
+        VLOG(1) << *equivalence_class_members_list;
+        return false;
+      }
+
+      // Ignore potential future kinds of facet URIs (e.g. for new platforms).
+      FacetURI uri = FacetURI::FromPotentiallyInvalidSpec(uri_spec);
+      if (!uri.is_valid()){
+        VLOG(2) << "Ignored facet URI of unknown/invalid format: " << uri_spec;
+        continue;
+      }
+
+      affiliated_uris.push_back(uri);
+    }
+
+    // Be lenient and ignore empty (after filtering) equivalence classes.
+    if (affiliated_uris.empty())
+      continue;
+
+    // Ignore equivalence classes if they are a duplicate of an earlier one.
+    // However, bail out if we discover partial overlapping, in which case the
+    // response cannot be part of an equivalence relation.

[Ryan Sleevi, 2014/12/10 19:44:36]

Please consider rewriting this without the pronoun (see
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis...
for discussion)

// However, if a partial overlapping is discovered, terminate early, as the
response cannot be part of an equivalence relation.

^-- or whatever it means (as someone who has read your design doc, I'm still
lost on your terminology, but I'll assume that's my own ignorance than the
comment)

[engedy, 2014/12/11 20:49:23]

I have rephrased it, hopefully, for the better.

+    for (const FacetURI& uri : affiliated_uris) {
+      if (!facet_uri_to_class_index.count(uri))
+        facet_uri_to_class_index[uri] = result->size();
+      if (facet_uri_to_class_index[uri] !=
+          facet_uri_to_class_index[affiliated_uris[0]]) {
+        VLOG(1) << "Response cannot be part of an equivalence relation.";
+        VLOG(1) << *response_value;
+        return false;
+      }
+    }
+
+    // Filter out duplicate equivalence classes in the response.
+    if (facet_uri_to_class_index[affiliated_uris[0]] == result->size())
+      result->push_back(affiliated_uris);
+  }
+
+  // The server does not return at all facet URIs that are not affiliated with
+  // anything, or facet URIs that it does not know about. However, this class

[Ryan Sleevi, 2014/12/10 19:44:36]

This first comment reads weird, in particular, the "does not return at all facet
URIs"

Is it meant to be "does not return any facet URIs"? is the "at" a typo and it
should be "does not return all" ?

[engedy, 2014/12/11 20:49:23]

That was a typo, sorry. I have used you comment suggestion below.

+  // promises to return an equivalence class for each requested facet, so create
+  // one for each each of these missing facets.

[Ryan Sleevi, 2014/12/10 19:44:36]

// The API contract of this class is to return an equivalence class for all
requested facets; however, the
// server does not return any facets that are either not affiliated with
anything or which it does not
// know about. In this case, synthesize an equivalence class for all remaining
facets.

I'm still not sure what the distinction is between "facet URIs that are not
affiliated with anything" and "facet URIs that it does not know about", but I
assume the distinction is clear for you.

[engedy, 2014/12/11 20:49:23]

Thanks for the suggestion! I have rephrased along these lines.

+  for (const FacetURI& uri : requested_facet_uris_) {
+    if (!facet_uri_to_class_index.count(uri)) {
+      result->resize(result->size() + 1);
+      result->back().push_back(uri);
+    }
+  }
+
+  return true;
+}
+
+void AffiliationFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK_EQ(source, fetcher_.get());
+
+  scoped_ptr<AffiliationFetcherDelegate::Result> result(
+      new AffiliationFetcherDelegate::Result);
+  if (fetcher_->GetResponseCode() == net::HTTP_OK) {

[Randy Smith (Not in Mondays), 2014/12/10 22:13:24]

I'm a little uncomfortable with testing only this, and not testing
fetcher_->GetStatus().  It may well be that for all cases in which
fetcher_->GetResponseCode() == HTTP_OK, GetStatus() will be { Success, OK}, but
it seems unwise to count on it--for instance, if we receive http headers and
partial data and then the connection gets reset, maybe URLFetcher will return
the partial data and a failure in GetStatus().  My guess is it doesn't, but
based on the header comments it might, so defensive programming seems the right
strategy.

[engedy, 2014/12/11 20:49:24]

Done. Thanks for pointing that out!

+    if (ParseResponse(result.get()))
+      delegate_->OnFetchSucceeded(result.Pass());
+    else
+      delegate_->OnMalformedResponse();
+  } else {
+    delegate_->OnFetchFailed();
+  }
+}
+
+}  // namespace password_manager