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..9d90d10dce20733eb963bbe76cfbd5d059b7e07f
--- /dev/null
+++ b/components/password_manager/core/browser/affiliation_fetcher.cc
@@ -0,0 +1,216 @@
+// 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 {
+
+namespace {
+
+const char kAffilitionServiceLookupURL[] =
+    "https://www.googleapis.com/affiliation/v1/affiliation:lookup";
+
+// In the JSON dictionary of the request, the key that maps to the list of facet
+// URIs to query.
+const char kRequestFacetURIsKey[] = "facet";
+
+// In the JSON dictionary of the response, the key that maps to the the list of
+// equivalence classes, each being a dictionary itself.
+const char kResponseEquivalenceClassesKey[] = "affiliation";
+
+// In the JSON dictionary of one equivalence class, the key that maps to the
+// list of facet URIs in that class.
+const char kResponseEquivalenceClassMembersKey[] = "facet";

[Mike West, 2014/12/02 14:32:42]

Hrm. You're only using each of these keys once. While I agree that the
descriptive name is significantly better than a bare string, it's not clear that
these need to be extracted into an anonymous namespace. It's also unclear why
some string constants are extracted here, and others aren't. For instance, "key"
in BuildQueryURL().

Do you think you'll use them elsewhere in the future? If so, perhaps they could
be public constants that other classes could use? If not, it might make sense to
bring them back down into local scopes.

[engedy, 2014/12/02 18:33:55]

They shouldn't any other classes that need this, so I just moved them to local
scopes. In fact, I have removed the constant definitions altogether, and added
an sample request/response instead. WDYT? Happy to bring back the constants if
you think that is better.

+
+// If set, then this factory will be used instead of the vanilla constructor for
+// constructing AffiliationFetcher instances. Used only for testing.
+AffiliationFetcherFactory* g_factory = nullptr;
+
+}  // namespace
+
+AffiliationFetcher::AffiliationFetcher(
+    net::URLRequestContextGetter* request_context_getter,
+    const std::vector<std::string>& facet_uris,
+    AffiliationFetcherDelegate* delegate)
+    : request_context_getter_(request_context_getter),
+      requested_facet_uris_(facet_uris),
+      delegate_(delegate) {
+

[Mike West, 2014/12/02 14:32:42]

Nit: Whitespace.

[engedy, 2014/12/02 18:33:55]

Done.

+  for (const auto& uri : requested_facet_uris_) {

[Mike West, 2014/12/02 14:32:42]

Consider dropping "auto" here; the actual type isn't terribly complex.

[engedy, 2014/12/02 18:33:55]

Done.

+    DCHECK(IsValidFacetURI(uri));
+  }

[Mike West, 2014/12/02 14:32:42]

Nit: No {} for a one-line body.

[engedy, 2014/12/02 18:33:55]

Done.

+}
+
+AffiliationFetcher::~AffiliationFetcher() {
+}
+
+// static
+AffiliationFetcher* AffiliationFetcher::Create(
+    net::URLRequestContextGetter* context_getter,
+    const std::vector<std::string>& facet_uris,
+    AffiliationFetcherDelegate* delegate) {
+  if (g_factory)
+    return g_factory->CreateInstance(context_getter, facet_uris, delegate);
+  return new AffiliationFetcher(context_getter, facet_uris, delegate);
+}
+
+// static
+void AffiliationFetcher::SetFactoryForTesting(
+    AffiliationFetcherFactory* factory) {
+  g_factory = factory;
+}
+
+void AffiliationFetcher::StartRequest() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (fetcher_) {
+    NOTREACHED();

[Mike West, 2014/12/02 14:32:42]

Perhaps just `DCHECK(!fetcher_)`?

[engedy, 2014/12/02 18:33:55]

Done.

+    return;
+  }
+
+  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 |

[Mike West, 2014/12/02 14:32:42]

Probably should add net::LOAD_DO_NOT_PROMPT_FOR_LOGIN.

[engedy, 2014/12/02 18:33:55]

Done.

+                         net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE);

[Mike West, 2014/12/02 14:32:42]

Why both bypass and disable? Disabling should be enough, shouldn't it?

[engedy, 2014/12/02 18:33:55]

I think both are needed.

DISABLE alone would not be enough as only BYPASS will add "Pragma: no-cache" and
"Cache-Control: no-cache" headers to the request: otherwise a proxy may decide
to send back a cached, and possibly out-of-date, response. See:

https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...

BYPASS alone would not be enough as only DISABLE will prevent writing the
response to Chrome's cache (which would only serve to waste space). See:

https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach...
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach...
https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_cach...

The documentation in load_flags_list.h is a bit vague, but seems to be in
agreement with this, and there are also a few other call sites specifying both
flags.

+  fetcher_->SetAutomaticallyRetryOn5xx(false);
+  fetcher_->SetAutomaticallyRetryOnNetworkChanges(0);
+  fetcher_->Start();
+}
+
+GURL AffiliationFetcher::BuildQueryURL() const {
+  return net::AppendQueryParameter(GURL(kAffilitionServiceLookupURL), "key",
+                                   google_apis::GetAPIKey());
+}
+
+std::string AffiliationFetcher::PreparePayload() const {
+  scoped_ptr<base::ListValue> requested_facet_uris_list(new base::ListValue);
+  for (const auto& uri : requested_facet_uris_) {

[Mike West, 2014/12/02 14:32:42]

Consider just using the type rather than auto.

[engedy, 2014/12/02 18:33:55]

Done.

+    requested_facet_uris_list->AppendString(uri);
+  }

[Mike West, 2014/12/02 14:32:42]

Nit: No {}.

[engedy, 2014/12/02 18:33:55]

Done.

+
+  base::DictionaryValue payload_dictionary;
+  payload_dictionary.Set(kRequestFacetURIsKey,
+                         requested_facet_uris_list.release());
+
+  std::string payload_json;
+  if (!base::JSONWriter::Write(&payload_dictionary, &payload_json)) {

[Mike West, 2014/12/02 14:32:42]

Consider using OPTIONS_PRETTY_PRINT while in Debug mode (or having some other
method to inject the preference). That might make things simpler for you to
visually inspect while you're working through the implementation details.

[engedy, 2014/12/02 18:33:55]

For now, I would not complicate the code with this, as we mostly build rather
straightforward JSON outputs here.

+    // This should never really happen.

[Mike West, 2014/12/02 14:32:42]

DCHECK?

[engedy, 2014/12/02 18:33:55]

Done.

+    NOTREACHED();
+  }
+  return payload_json;
+}
+
+bool AffiliationFetcher::ParseResponse(Result* result) const {
+  std::string response_json;
+  if (!fetcher_->GetResponseAsString(&response_json))
+    return false;
+
+  scoped_ptr<const base::Value> response_value(
+      (base::JSONReader::Read(response_json)));

[Mike West, 2014/12/02 14:32:42]

Nit: Extra ().

[engedy, 2014/12/02 18:33:55]

Done.

+  if (!response_value)
+    return false;
+
+  const base::DictionaryValue* response_dict;
+  if (!response_value->GetAsDictionary(&response_dict))
+    return false;
+
+  const base::ListValue* equivalence_classes_list;
+  if (!response_dict->GetList(kResponseEquivalenceClassesKey,
+                              &equivalence_classes_list))
+    return false;

[Mike West, 2014/12/02 14:32:42]

You might find it helpful to have a bit more error reporting here. It's going to
be tough to debug as-is.

[engedy, 2014/12/02 18:33:55]

Hmm, what kind of scenario do you have in mind? The server-side suddenly sending
bad responses, or somebody updating this part of the code? That is, should we
add VLOGs and/or UMA histograms?

[Mike West, 2014/12/03 10:44:43]

I was thinking about VLOGs, as I imagine you'll be debugging the interactions
manually for at least a little while.

[engedy, 2014/12/09 10:33:45]

Done.

+
+  // 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.
+  result->reserve(requested_facet_uris_.size());
+
+  std::map<std::string, 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))
+      return false;
+
+    const base::ListValue* equivalence_class_members_list;
+    if (!equivalence_class_dict->GetList(kResponseEquivalenceClassMembersKey,
+                                         &equivalence_class_members_list))
+      return false;

[Mike West, 2014/12/03 10:44:43]

Nit: You need {} here, as the conditional is multi-line.

[engedy, 2014/12/09 10:33:45]

Done.

+
+    AffiliatedFacets affiliated_uris;
+    for (size_t j = 0; j < equivalence_class_members_list->GetSize(); ++j) {
+      std::string uri;
+      if (!equivalence_class_members_list->GetString(j, &uri))
+        return false;
+
+      // Ignore potential future kinds of facet URIs (e.g. for new platforms).
+      if (!IsValidFacetURI(uri))
+        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.
+    for (const auto& 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]])
+        return false;

[Mike West, 2014/12/03 10:44:43]

Nit: {}.

[engedy, 2014/12/09 10:33:45]

Done.

+    }
+
+    if (facet_uri_to_class_index[affiliated_uris[0]] == result->size())

[Mike West, 2014/12/03 10:44:43]

Isn't this always true if the conditions on 179-180 are met?

[engedy, 2014/12/09 10:33:45]

I have added a comment to clarify.

+      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
+  // promises to return an equivalence class for each requested facet, so create
+  // one for each each of these missing facets.
+  for (const auto& 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<Result> result(new Result);
+  if (fetcher_->GetResponseCode() == net::HTTP_OK) {
+    if (ParseResponse(result.get()))
+      delegate_->OnFetchSucceeded(result.Pass());
+    else
+      delegate_->OnMalformedResponse();
+  } else {
+    delegate_->OnFetchFailed();
+  }
+}
+
+}  // namespace password_manager