Allow fetching personalized snippets from ChromeReader.

components/ntp_snippets/ntp_snippets_fetcher.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 "components/ntp_snippets/ntp_snippets_fetcher.h"
 
+#include <stdlib.h>
+
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
+#include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/switches.h"
+#include "components/signin/core/browser/profile_oauth2_token_service.h"
+#include "components/signin/core/browser/signin_manager.h"
+#include "components/signin/core/browser/signin_manager_base.h"
+#include "components/signin/core/browser/signin_tracker.h"

[Marc Treib, 2016/05/09 12:33:33]

Is this needed?

[jkrcal, 2016/05/09 15:30:12]

Done.

+#include "components/variations/variations_associated_data.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
+#include "third_party/icu/source/common/unicode/uloc.h"
+#include "third_party/icu/source/common/unicode/utypes.h"
 
 using net::URLFetcher;
 using net::URLRequestContextGetter;
 using net::HttpRequestHeaders;
 using net::URLRequestStatus;
 
 namespace ntp_snippets {
 
 namespace {
 
 const char kStatusMessageEmptyHosts[] = "Cannot fetch for empty hosts list.";
 const char kStatusMessageURLRequestErrorFormat[] = "URLRequestStatus error %d";
 const char kStatusMessageHTTPErrorFormat[] = "HTTP error %d";
 const char kStatusMessageJsonErrorFormat[] = "Received invalid JSON (error %s)";
 const char kStatusMessageInvalidList[] = "Invalid / empty list.";
 
-const char kContentSnippetsServerFormat[] =
-    "https://chromereader-pa.googleapis.com/v1/fetch?key=%s";
+const char kApiScope[] = "https://www.googleapis.com/auth/webhistory";
+const char kSnippetsServerAuthorized[] =

[Marc Treib, 2016/05/09 12:33:33]

Just kSnippetsServer, it's used for both cases.

[jkrcal, 2016/05/09 15:30:13]

Done.

+    "https://chromereader-pa.googleapis.com/v1/fetch";
+const char kSnippetsServerNonAuthorizedFormat[] =
+    "%s?key=%s";

[Marc Treib, 2016/05/09 12:33:33]

Fits on the previous line

[jkrcal, 2016/05/09 15:30:13]

Done.

+const char kAuthorizationRequestHeaderFormat[] = "Bearer %s";
+
+// Variation parameter for the variant of fetching to use.
+const char kVariantName[] = "fetching_variant";
+
+// Constants listing possible values of the "fetching_variant" parameter.
+const char kVariantRestrictedString[] = "restricted";
+const char kVariantPersonalizedString[] = "personalized";
+const char kVariantRestrictedPersonalizedString[] = "restricted_personalized";
 
 const char kRequestParameterFormat[] =
     "{"
     "  \"response_detail_level\": \"STANDARD\","
+    "%s"  // If authenticated - an obfuscated Gaia ID will be inserted here
     "  \"advanced_options\": {"
     "    \"local_scoring_params\": {"
     "      \"content_params\": {"
     "        \"only_return_personalized_results\": false"
+    "%s"  // If authenticated - user segment (lang code) will be inserted here
     "      },"
     "      \"content_restricts\": {"
     "        \"type\": \"METADATA\","
     "        \"value\": \"TITLE\""
     "      },"
     "      \"content_restricts\": {"
     "        \"type\": \"METADATA\","
     "        \"value\": \"SNIPPET\""
     "      },"
     "      \"content_restricts\": {"
     "        \"type\": \"METADATA\","
     "        \"value\": \"THUMBNAIL\""
     "      }"
-    "%s"
+    "%s"  // If host restricted - host restrictions will be inserted here
     "    },"
     "    \"global_scoring_params\": {"
-    "      \"num_to_return\": %i"
+    "      \"num_to_return\": %i,"
+    "      \"sort_type\": 1"
     "    }"
     "  }"
     "}";
 
+const char kAuthorizationFormat[] = "  \"obfuscated_gaia_id\": \"%s\",";

[Marc Treib, 2016/05/09 12:33:33]

This isn't really authorization... kGaiaIdFormat?

[jkrcal, 2016/05/09 15:30:12]

Done.

+const char kUserSegmentFormat[] = "        \"user_segment\": \"%s\"";
 const char kHostRestrictFormat[] =
     "      ,\"content_selectors\": {"
     "        \"type\": \"HOST_RESTRICT\","
     "        \"value\": \"%s\""
     "      }";
 
 }  // namespace
 
 NTPSnippetsFetcher::NTPSnippetsFetcher(
+    SigninManagerBase* signin_manager,
+    OAuth2TokenService* token_service,
     scoped_refptr<URLRequestContextGetter> url_request_context_getter,
     const ParseJSONCallback& parse_json_callback,
     bool is_stable_channel)
-    : url_request_context_getter_(url_request_context_getter),
+    : OAuth2TokenService::Consumer("NTP_snippets"),

[Marc Treib, 2016/05/09 12:33:33]

I'd make this "ntp_snippets", most of the other implementations seem to use
lower-case strings here.

[jkrcal, 2016/05/09 15:30:13]

Done.

+      signin_manager_(signin_manager),
+      token_service_(token_service),
+      waiting_for_refresh_token_(false),
+      url_request_context_getter_(url_request_context_getter),
       parse_json_callback_(parse_json_callback),
       is_stable_channel_(is_stable_channel),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  // Parse the variation parameters and set the defaults if missing.
+  std::string variant = variations::GetVariationParamValue(
+      ntp_snippets::kStudyName, kVariantName);
+  if (variant == kVariantRestrictedString) {
+    variant_ = kRestricted;
+  } else if (variant == kVariantPersonalizedString) {
+    variant_ = kPersonalized;
+  } else {
+    variant_ = kRestrictedPersonalized;
+    if (!variant.empty() && variant != kVariantRestrictedPersonalizedString)
+      DLOG(WARNING) << "Unknown fetching variant provided: " << variant;

[Marc Treib, 2016/05/09 12:33:33]

I think this might be worth a non-D LOG. Bernhard, WDYT?

[jkrcal, 2016/05/09 15:30:13]

Done.

(And happy to change it back if Bernhard is of a different opinion.)

[Bernhard Bauer, 2016/05/09 17:45:06]

Nah, just make it LOG_IF if you're not doing anything else.

[jkrcal, 2016/05/09 19:04:16]

Done.

+  }
+}
 
-NTPSnippetsFetcher::~NTPSnippetsFetcher() {}
+NTPSnippetsFetcher::~NTPSnippetsFetcher() {
+  if (waiting_for_refresh_token_)
+    token_service_->RemoveObserver(this);
+}
 
 void NTPSnippetsFetcher::SetCallback(
     const SnippetsAvailableCallback& callback) {
   snippets_available_callback_ = callback;
 }
 
 void NTPSnippetsFetcher::FetchSnippetsFromHosts(
-    const std::set<std::string>& hosts, int count) {
-  std::string host_restricts;
+    const std::set<std::string>& hosts,
+    const std::string& language_code,
+    int count) {
+  hosts_ = hosts;
+
   if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDontRestrict)) {
-    if (hosts.empty()) {
+    if (hosts_.empty()) {
       if (!snippets_available_callback_.is_null()) {
         snippets_available_callback_.Run(NTPSnippet::PtrVector(),
                                          kStatusMessageEmptyHosts);
       }
       return;
     }
-    for (const std::string& host : hosts)
-      host_restricts += base::StringPrintf(kHostRestrictFormat, host.c_str());
   }
-  const std::string& key = is_stable_channel_
-                               ? google_apis::GetAPIKey()
-                               : google_apis::GetNonStableAPIKey();
-  std::string url =
-      base::StringPrintf(kContentSnippetsServerFormat, key.c_str());
-  url_fetcher_ = URLFetcher::Create(GURL(url), URLFetcher::POST, this);
+
+  // Translate the BCP 47 |language_code| into a posix locale string

[Marc Treib, 2016/05/09 12:33:33]

nit: Period after comment, also in a few other places in this file.

[jkrcal, 2016/05/09 15:30:12]

Done.

+  char locale[ULOC_FULLNAME_CAPACITY];
+  UErrorCode error;
+  uloc_forLanguageTag(language_code.c_str(), locale, ULOC_FULLNAME_CAPACITY,
+                      nullptr, &error);
+  DLOG_IF(WARNING, U_ZERO_ERROR != error) <<
+      "Error in translating language code to a locale string: " << error;
+  locale_ = locale;
+
+  count_ = count;
+
+  bool use_authentication = UseAuthentication();
+
+  if (use_authentication && signin_manager_->IsAuthenticated()) {
+    // Signed-in: get OAuth token --> fetch snippets.
+    StartTokenRequest();
+  } else if (use_authentication && signin_manager_->AuthInProgress()) {
+    // Currently signing in: wait for auth to finish (the refresh token) -->
+    //     get OAuth token --> fetch snippets.
+    if (!waiting_for_refresh_token_) {
+      // Wait until we get a refresh token.
+      waiting_for_refresh_token_ = true;
+      token_service_->AddObserver(this);
+    }
+  } else {
+    // Not signed in: fetch snippets (without authentication).
+    FetchSnippetsNonAuthenticated();
+  }
+}
+
+void NTPSnippetsFetcher::FetchSnippetsImpl(const GURL& url,
+                                           const std::string& auth_header,
+                                           const std::string& request) {
+  url_fetcher_ = URLFetcher::Create(url, URLFetcher::POST, this);
+
   url_fetcher_->SetRequestContext(url_request_context_getter_.get());
   url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                              net::LOAD_DO_NOT_SAVE_COOKIES);
+
   data_use_measurement::DataUseUserData::AttachToFetcher(
       url_fetcher_.get(), data_use_measurement::DataUseUserData::NTP_SNIPPETS);
+
   HttpRequestHeaders headers;
+  if (!auth_header.empty())
+    headers.SetHeader("Authorization", auth_header);
   headers.SetHeader("Content-Type", "application/json; charset=UTF-8");
   url_fetcher_->SetExtraRequestHeaders(headers.ToString());
-  url_fetcher_->SetUploadData("application/json",
-                              base::StringPrintf(kRequestParameterFormat,
-                                                 host_restricts.c_str(),
-                                                 count));
-
+  url_fetcher_->SetUploadData("application/json", request);
   // Fetchers are sometimes cancelled because a network change was detected.
   url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(3);
   // Try to make fetching the files bit more robust even with poor connection.
   url_fetcher_->SetMaxRetriesOn5xx(3);
   url_fetcher_->Start();
 }
 
+std::string NTPSnippetsFetcher::GetHostRestricts() const {
+  std::string host_restricts;
+  if (variant_ == kRestricted || variant_ == kRestrictedPersonalized) {
+    for (const std::string& host : hosts_)
+      host_restricts += base::StringPrintf(kHostRestrictFormat, host.c_str());
+  }
+  return host_restricts;
+}
+
+bool NTPSnippetsFetcher::UseAuthentication() {
+  return (variant_ == kPersonalized ||
+      (variant_ == kRestrictedPersonalized && !hosts_.empty()));

[Marc Treib, 2016/05/09 12:33:33]

Why are we checking hosts_ here?

[jkrcal, 2016/05/09 15:30:13]

You are right, this is weird 
(especially w.r.t. changes that happened to the code since I started on this).

I removed it (and added another function dealing with host restriction
generally).

+}
+
+void NTPSnippetsFetcher::FetchSnippetsNonAuthenticated() {
+  // When not providing OAuth token, we need to pass the Google API key
+  const std::string& key = is_stable_channel_
+                               ? google_apis::GetAPIKey()
+                               : google_apis::GetNonStableAPIKey();
+  GURL url(base::StringPrintf(kSnippetsServerNonAuthorizedFormat,
+                              kSnippetsServerAuthorized,
+                              key.c_str()));
+
+  FetchSnippetsImpl(url, std::string(),
+                    base::StringPrintf(kRequestParameterFormat,
+                                       std::string().c_str(),

[Marc Treib, 2016/05/09 12:33:33]

I think in this case, "" would be preferred.

[jkrcal, 2016/05/09 15:30:13]

Okay :)

+                                       std::string().c_str(),
+                                       GetHostRestricts().c_str(),
+                                       count_));
+}
+
+void NTPSnippetsFetcher::FetchSnippetsAuthenticated(
+    const std::string& account_id,
+    const std::string& oauth_access_token) {
+  std::string auth =
+        base::StringPrintf(kAuthorizationFormat, account_id.c_str());
+  std::string user_segment =
+          base::StringPrintf(kUserSegmentFormat, locale_.c_str());
+
+  FetchSnippetsImpl(GURL(kSnippetsServerAuthorized),
+                    base::StringPrintf(kAuthorizationRequestHeaderFormat,
+                                       oauth_access_token.c_str()),
+                    base::StringPrintf(kRequestParameterFormat,
+                                       auth.c_str(),
+                                       user_segment.c_str(),
+                                       GetHostRestricts().c_str(),
+                                       count_));
+}
+
+void NTPSnippetsFetcher::StartTokenRequest() {
+  OAuth2TokenService::ScopeSet scopes;
+  scopes.insert(kApiScope);
+  oauth_request_ = token_service_->StartRequest(
+      signin_manager_->GetAuthenticatedAccountId(), scopes, this);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// OAuth2TokenService::Consumer overrides
+void NTPSnippetsFetcher::OnGetTokenSuccess(
+    const OAuth2TokenService::Request* request,
+    const std::string& access_token,
+    const base::Time& expiration_time) {
+  oauth_request_.reset();

[Marc Treib, 2016/05/09 12:33:33]

Mmh, this is actually the same as the |request| parameter, right? If so, then
GetAccountId below will be a use-after-free.

[jkrcal, 2016/05/09 15:30:13]

Done.
 (Thanks for pointing it out!)

+
+  FetchSnippetsAuthenticated(request->GetAccountId(), access_token);
+}
+
+void NTPSnippetsFetcher::OnGetTokenFailure(
+    const OAuth2TokenService::Request* request,
+    const GoogleServiceAuthError& error) {
+  oauth_request_.reset();
+  DLOG(ERROR) << "Unable to get token: " << error.ToString()
+              << " - fetching the snippets without authentication.";
+
+  // Fallback to fetching non-authenticated tokens.
+  FetchSnippetsNonAuthenticated();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// OAuth2TokenService::Observer overrides
+void NTPSnippetsFetcher::OnRefreshTokenAvailable(
+  const std::string& account_id) {

[Marc Treib, 2016/05/09 12:33:33]

Misaligned, but should fit on the previous line?
Also, should we do anything with the account_id? Like, check it's the one we
expect?

[jkrcal, 2016/05/09 15:30:13]

Thanks for pointing out the misalignment. (No, it does not fit on the previous
line.)

I do not think we should do anything about the account_id. There is no
particular account_id we should expect - the one the user signs in with is the
one we pass to ChromeReader. This callback has a unary meaning to us - now it is
time to send the request for the access token.

[Marc Treib, 2016/05/09 16:48:24]

Can we only ever have one refresh token? I think with multi-signin on Android,
we can actually have multiple. I'm not completely sure though, Bernhard might
know more.
If we *can* have multiple refresh tokens, then we should probably check that
account_id matches signin_manager_->GetAuthenticatedAccountId()

[Bernhard Bauer, 2016/05/09 17:45:07]

Yeah, I'm pretty certain this can happen.
Yup. (Also, if it couldn't happen, we should DCHECK that they're the same.)

[jkrcal, 2016/05/09 19:04:16]

Now I get your point. I was not aware this listens to tokens for all accounts.

Done.

+  token_service_->RemoveObserver(this);
+  waiting_for_refresh_token_ = false;
+  StartTokenRequest();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // URLFetcherDelegate overrides
 void NTPSnippetsFetcher::OnURLFetchComplete(const URLFetcher* source) {
   DCHECK_EQ(url_fetcher_.get(), source);
 
   std::string message;
   const URLRequestStatus& status = source->GetStatus();
 
   UMA_HISTOGRAM_SPARSE_SLOWLY(
       "NewTabPage.Snippets.FetchHttpResponseOrErrorCode",
       status.is_success() ? source->GetResponseCode() : status.error());
 
   if (!status.is_success()) {
     message = base::StringPrintf(kStatusMessageURLRequestErrorFormat,
                                  status.error());
   } else if (source->GetResponseCode() != net::HTTP_OK) {
+    // TODO(jkrcal): https://crbug.com/609084
+    // We need to deal with the edge case again where the auth
+    // token expires just before we send the request (in which case we need to
+    // fetch a new auth token). We should extract that into a common class
+    // instead of adding it to every single class that uses auth tokens.
     message = base::StringPrintf(kStatusMessageHTTPErrorFormat,
                                  source->GetResponseCode());
   }
 
   if (!message.empty()) {
+    std::string error_response;
+    source->GetResponseAsString(&error_response);
     DLOG(WARNING) << message << " while trying to download "
-                  << source->GetURL().spec();
+                  << source->GetURL().spec() << ": " << error_response;
     if (!snippets_available_callback_.is_null())
       snippets_available_callback_.Run(NTPSnippet::PtrVector(), message);
   } else {
     bool stores_result_to_string = source->GetResponseAsString(
         &last_fetch_json_);
     DCHECK(stores_result_to_string);
 
     parse_json_callback_.Run(
         last_fetch_json_,
         base::Bind(&NTPSnippetsFetcher::OnJsonParsed,
@@ skipping 25 matching lines @@
   LOG(WARNING) << "Received invalid JSON (" << error << "): "
                << last_fetch_json_;
   if (!snippets_available_callback_.is_null()) {
     snippets_available_callback_.Run(
         NTPSnippet::PtrVector(),
         base::StringPrintf(kStatusMessageJsonErrorFormat, error.c_str()));
   }
 }
 
 }  // namespace ntp_snippets