Restrict transmission of external exp ids to signed in users.

Since external experiment ids are not based on Chrome's low
entropy source, they do not have the same guarantees about
not identifying a user as Chrome's variations. As such, we
should only transmit them for signed in users, whose identity
is already known by Google so there's no risk of identifying
them through these headers.

Note: The signed-in state checking in this CL is only done for
web content area requests and not other internal requests,
like to the suggestion service, where it treats the state as
"not signed in". This is fine to do because variations service
ids are still sent, which is what the other call sites are
interested in.

BUG=672532
TBR=mpearson@chromium.org,mattm@chromium.org,donnd@chromium.org,afakhry@chromium.org

Review-Url: https://codereview.chromium.org/2558913003
Cr-Commit-Position: refs/heads/master@{#437959}

chrome/browser/android/contextualsearch/contextual_search_delegate.cc

@@ -136,10 +136,13 @@ void ContextualSearchDelegate::ContinueSearchTermResolutionRequest() {
 
   // Add Chrome experiment state to the request headers.
   net::HttpRequestHeaders headers;
+  // Note: It's fine to pass in |is_signed_in| false, which does not affect
+  // transmission of experiment ids coming from the variations server.
+  bool is_signed_in = false;
   variations::AppendVariationHeaders(
       search_term_fetcher_->GetOriginalURL(),
       false,  // Impossible to be incognito at this point.
-      false, &headers);
+      false, is_signed_in, &headers);
   search_term_fetcher_->SetExtraRequestHeaders(headers.ToString());
 
   SetDiscourseContextAndAddToHeader(*context_);

chrome/browser/android/metrics/uma_session_stats.cc

@@ -168,8 +168,11 @@ static void RegisterExternalExperiment(
   active_group.name = metrics::HashName(trial_name_utf8);
   for (int experiment_id : experiment_ids) {
     active_group.group = metrics::HashName(base::IntToString(experiment_id));
+    // Since external experiments are not based on Chrome's low entropy source,
+    // they are only sent to Google web properties for signed in users to make
+    // sure that this couldn't be used to identify a user that's not signed in.
     variations::AssociateGoogleVariationIDForceHashes(
-        variations::GOOGLE_WEB_PROPERTIES, active_group,
+        variations::GOOGLE_WEB_PROPERTIES_SIGNED_IN, active_group,
         static_cast<variations::VariationID>(experiment_id));
     group_name_hashes.push_back(active_group.group);
   }

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc

@@ -485,8 +485,11 @@ void ChromeResourceDispatcherHostDelegate::RequestBeginning(
     net::HttpRequestHeaders headers;
     headers.CopyFrom(request->extra_request_headers());
     bool is_off_the_record = io_data->IsOffTheRecord();
+    bool is_signed_in =
+        !is_off_the_record &&
+        !io_data->google_services_account_id()->GetValue().empty();
     variations::AppendVariationHeaders(
-        request->url(), is_off_the_record,
+        request->url(), is_off_the_record, is_signed_in,
         !is_off_the_record && io_data->GetMetricsEnabledStateOnIOThread(),
         &headers);
     request->SetExtraRequestHeaders(headers);

chrome/browser/safe_browsing/srt_fetcher_win.cc

@@ -632,10 +632,13 @@ class SRTFetcher : public net::URLFetcherDelegate {
     ProfileIOData* io_data = ProfileIOData::FromResourceContext(
         profile_->GetResourceContext());
     net::HttpRequestHeaders headers;
+    // Note: It's fine to pass in |is_signed_in| false, which does not affect
+    // transmission of experiment ids coming from the variations server.
+    bool is_signed_in = false;
     variations::AppendVariationHeaders(
         url_fetcher_->GetOriginalURL(), io_data->IsOffTheRecord(),
         ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled(),
-        &headers);
+        is_signed_in, &headers);
     url_fetcher_->SetExtraRequestHeaders(headers.ToString());
     url_fetcher_->Start();
   }

components/autofill/core/browser/autofill_download_manager.cc

@@ -247,8 +247,12 @@ bool AutofillDownloadManager::StartRequest(
                         net::LOAD_DO_NOT_SEND_COOKIES);
   // Add Chrome experiment state to the request headers.
   net::HttpRequestHeaders headers;
-  variations::AppendVariationHeaders(
-      fetcher->GetOriginalURL(), driver_->IsOffTheRecord(), false, &headers);
+  // Note: It's fine to pass in |is_signed_in| false, which does not affect
+  // transmission of experiment ids coming from the variations server.
+  bool is_signed_in = false;
+  variations::AppendVariationHeaders(fetcher->GetOriginalURL(),
+                                     driver_->IsOffTheRecord(), false,
+                                     is_signed_in, &headers);
   fetcher->SetExtraRequestHeaders(headers.ToString());
   fetcher->Start();
 

components/feedback/feedback_uploader_chrome.cc

@@ -60,8 +60,12 @@ void FeedbackUploaderChrome::DispatchReport(const std::string& data) {
       fetcher, data_use_measurement::DataUseUserData::FEEDBACK_UPLOADER);
   // Tell feedback server about the variation state of this install.
   net::HttpRequestHeaders headers;
-  variations::AppendVariationHeaders(
-      fetcher->GetOriginalURL(), context_->IsOffTheRecord(), false, &headers);
+  // Note: It's fine to pass in |is_signed_in| false, which does not affect
+  // transmission of experiment ids coming from the variations server.
+  bool is_signed_in = false;
+  variations::AppendVariationHeaders(fetcher->GetOriginalURL(),
+                                     context_->IsOffTheRecord(), false,
+                                     is_signed_in, &headers);
   fetcher->SetExtraRequestHeaders(headers.ToString());
 
   fetcher->SetUploadData(kProtoBufMimeType, data);

components/ntp_snippets/remote/ntp_snippets_fetcher.cc

@@ -691,10 +691,13 @@ std::string NTPSnippetsFetcher::RequestBuilder::BuildHeaders() const {
     headers.SetHeader("Authorization", auth_header_);
   }
   // Add X-Client-Data header with experiment IDs from field trials.
+  // Note: It's fine to pass in |is_signed_in| false, which does not affect
+  // transmission of experiment ids coming from the variations server.
+  bool is_signed_in = false;
   variations::AppendVariationHeaders(url_,
                                      false,  // incognito
                                      false,  // uma_enabled
-                                     &headers);
+                                     is_signed_in, &headers);
   return headers.ToString();
 }
 

components/omnibox/browser/search_provider.cc

@@ -890,8 +890,12 @@ std::unique_ptr<net::URLFetcher> SearchProvider::CreateSuggestFetcher(
   fetcher->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES);
   // Add Chrome experiment state to the request headers.
   net::HttpRequestHeaders headers;
-  variations::AppendVariationHeaders(
-      fetcher->GetOriginalURL(), client()->IsOffTheRecord(), false, &headers);
+  // Note: It's fine to pass in |is_signed_in| false, which does not affect
+  // transmission of experiment ids coming from the variations server.
+  bool is_signed_in = false;
+  variations::AppendVariationHeaders(fetcher->GetOriginalURL(),
+                                     client()->IsOffTheRecord(), false,
+                                     is_signed_in, &headers);
   fetcher->SetExtraRequestHeaders(headers.ToString());
   fetcher->Start();
   return fetcher;

components/omnibox/browser/zero_suggest_provider.cc

@@ -333,9 +333,12 @@ void ZeroSuggestProvider::Run(const GURL& suggest_url) {
     fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES);
     // Add Chrome experiment state to the request headers.
     net::HttpRequestHeaders headers;
+    // Note: It's fine to pass in |is_signed_in| false, which does not affect
+    // transmission of experiment ids coming from the variations server.
+    bool is_signed_in = false;
     variations::AppendVariationHeaders(fetcher_->GetOriginalURL(),
                                        client()->IsOffTheRecord(), false,
-                                       &headers);
+                                       is_signed_in, &headers);
     fetcher_->SetExtraRequestHeaders(headers.ToString());
     fetcher_->Start();
     LogOmniboxZeroSuggestRequest(ZERO_SUGGEST_REQUEST_SENT);

components/suggestions/suggestions_service.cc

@@ -420,8 +420,11 @@ std::unique_ptr<net::URLFetcher> SuggestionsService::CreateSuggestionsRequest(
   request->SetRequestContext(url_request_context_);
   // Add Chrome experiment state to the request headers.
   net::HttpRequestHeaders headers;
+  // Note: It's fine to pass in |is_signed_in| false, which does not affect
+  // transmission of experiment ids coming from the variations server.
+  bool is_signed_in = false;
   variations::AppendVariationHeaders(request->GetOriginalURL(), false, false,
-                                     &headers);
+                                     is_signed_in, &headers);
   request->SetExtraRequestHeaders(headers.ToString());
   if (!access_token.empty()) {
     request->AddExtraRequestHeader(

components/variations/net/variations_http_headers.cc

@@ -44,6 +44,7 @@ const char kClientData[] = "X-Client-Data";
 void AppendVariationHeaders(const GURL& url,
                             bool incognito,
                             bool uma_enabled,
+                            bool is_signed_in,
                             net::HttpRequestHeaders* headers) {
   // Note the criteria for attaching client experiment headers:
   // 1. We only transmit to Google owned domains which can evaluate experiments.
@@ -62,7 +63,8 @@ void AppendVariationHeaders(const GURL& url,
     headers->SetHeaderIfMissing(kChromeUMAEnabled, "1");
 
   const std::string variation_ids_header =
-      VariationsHttpHeaderProvider::GetInstance()->GetClientDataHeader();
+      VariationsHttpHeaderProvider::GetInstance()->GetClientDataHeader(
+          is_signed_in);
   if (!variation_ids_header.empty()) {
     // Note that prior to M33 this header was named X-Chrome-Variations.
     headers->SetHeaderIfMissing(kClientData, variation_ids_header);

components/variations/net/variations_http_headers.h

@@ -17,12 +17,16 @@ class GURL;
 namespace variations {
 
 // Adds Chrome experiment and metrics state as custom headers to |headers|.
-// Some headers may not be set given the |incognito| mode or whether
-// the user has |uma_enabled|.  Also, we never transmit headers to non-Google
-// sites, which is checked based on the destination |url|.
+// The content of the headers will depend on |incognito|, |uma_enabled| and
+// |is_signed_in| parameters. It is fine to pass false for |is_signed_in| if the
+// state is not known to the caller. This will prevent addition of ids of type
+// GOOGLE_WEB_PROPERTIES_SIGNED_IN, which is not the case for any ids that come
+// from the variations server. These headers are never transmitted to non-Google
+// web sites, which is checked based on the destination |url|.
 void AppendVariationHeaders(const GURL& url,
                             bool incognito,
                             bool uma_enabled,
+                            bool is_signed_in,
                             net::HttpRequestHeaders* headers);
 
 // Returns the HTTP header names which are added by AppendVariationHeaders().

components/variations/variations_associated_data.cc

@@ -39,13 +39,21 @@ class GroupMapAccessor {
                    const VariationID id,
                    const bool force) {
 #if !defined(NDEBUG)
-    DCHECK_EQ(3, ID_COLLECTION_COUNT);
-    // Ensure that at most one of the trigger/non-trigger web property IDs are
-    // set.
-    if (key == GOOGLE_WEB_PROPERTIES || key == GOOGLE_WEB_PROPERTIES_TRIGGER) {
-      IDCollectionKey other_key = key == GOOGLE_WEB_PROPERTIES ?
-          GOOGLE_WEB_PROPERTIES_TRIGGER : GOOGLE_WEB_PROPERTIES;
-      DCHECK_EQ(EMPTY_ID, GetID(other_key, group_identifier));
+    DCHECK_EQ(4, ID_COLLECTION_COUNT);
+    // Ensure that at most one of the trigger/non-trigger/signed-in web property
+    // IDs are set.
+    if (key == GOOGLE_WEB_PROPERTIES || key == GOOGLE_WEB_PROPERTIES_TRIGGER ||
+        key == GOOGLE_WEB_PROPERTIES_SIGNED_IN) {
+      if (key != GOOGLE_WEB_PROPERTIES)
+        DCHECK_EQ(EMPTY_ID, GetID(GOOGLE_WEB_PROPERTIES, group_identifier));
+      if (key != GOOGLE_WEB_PROPERTIES_TRIGGER) {
+        DCHECK_EQ(EMPTY_ID,
+                  GetID(GOOGLE_WEB_PROPERTIES_TRIGGER, group_identifier));
+      }
+      if (key != GOOGLE_WEB_PROPERTIES_SIGNED_IN) {
+        DCHECK_EQ(EMPTY_ID,
+                  GetID(GOOGLE_WEB_PROPERTIES_SIGNED_IN, group_identifier));
+      }
     }
 
     // Validate that all collections with this |group_identifier| have the same

components/variations/variations_associated_data.h

@@ -58,6 +58,9 @@ enum IDCollectionKey {
   // This collection is used by Google web properties, transmitted through the
   // X-Client-Data header.
   GOOGLE_WEB_PROPERTIES,
+  // This collection is used by Google web properties for signed in users only,
+  // transmitted through the X-Client-Data header.
+  GOOGLE_WEB_PROPERTIES_SIGNED_IN,
   // This collection is used by Google web properties for IDs that trigger
   // server side experimental behavior, transmitted through the
   // X-Client-Data header.

components/variations/variations_http_header_provider.h

@@ -7,6 +7,7 @@
 
 #include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
@@ -32,12 +33,15 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer,
   static VariationsHttpHeaderProvider* GetInstance();
 
   // Returns the value of the client data header, computing and caching it if
-  // necessary.
-  std::string GetClientDataHeader();
+  // necessary. If |is_signed_in| is false, variation ids that should only be
+  // sent for signed in users (i.e. GOOGLE_WEB_PROPERTIES_SIGNED_IN entries)
+  // will not be included.
+  std::string GetClientDataHeader(bool is_signed_in);
 
   // Returns a space-separated string containing the list of current active
   // variations (as would be reported in the |variation_id| repeated field of
-  // the ClientVariations proto). The returned string is guaranteed to have a
+  // the ClientVariations proto). Does not include variation ids that should be
+  // sent for signed-in users only. The returned string is guaranteed to have a
   // a leading and trailing space, e.g. " 123 234 345 ".
   std::string GetVariationsString();
 
@@ -61,6 +65,8 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer,
  private:
   friend struct base::DefaultSingletonTraits<VariationsHttpHeaderProvider>;
 
+  typedef std::pair<VariationID, IDCollectionKey> VariationIDEntry;
+
   FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest,
                            SetDefaultVariationIds_Valid);
   FRIEND_TEST_ALL_PREFIXES(VariationsHttpHeaderProviderTest,
@@ -88,35 +94,43 @@ class VariationsHttpHeaderProvider : public base::FieldTrialList::Observer,
   // new variation IDs.
   void InitVariationIDsCacheIfNeeded();
 
+  // Looks up the associated id for the given trial/group and adds an entry for
+  // it to |variation_ids_set_| if found.
+  void CacheVariationsId(const std::string& trial_name,
+                         const std::string& group_name,
+                         IDCollectionKey key);
+
   // Takes whatever is currently in |variation_ids_set_| and recreates
   // |variation_ids_header_| with it.  Assumes the the |lock_| is currently
   // held.
   void UpdateVariationIDsHeaderValue();
 
+  // Generates a base64-encoded proto to be used as a header value for the given
+  // |is_signed_in| state.
+  std::string GenerateBase64EncodedProto(bool is_signed_in);
+
   // Returns the currently active set of variation ids, which includes any
   // default values, synthetic variations and actual field trial variations.
-  std::set<VariationID> GetAllVariationIds();
+  std::set<VariationIDEntry> GetAllVariationIds();
 
-  // Guards |variation_ids_cache_initialized_|, |variation_ids_set_| and
-  // |variation_ids_header_|.
+  // Guards access to variables below.
   base::Lock lock_;
 
-  // Whether or not we've initialized the cache.
+  // Whether or not we've initialized the caches.
   bool variation_ids_cache_initialized_;
 
   // Keep a cache of variation IDs that are transmitted in headers to Google.
   // This consists of a list of valid IDs, and the actual transmitted header.
-  std::set<VariationID> variation_ids_set_;
-  std::set<VariationID> variation_trigger_ids_set_;
+  std::set<VariationIDEntry> variation_ids_set_;
 
   // Provides the google experiment ids forced from command line.
-  std::set<VariationID> default_variation_ids_set_;
-  std::set<VariationID> default_trigger_id_set_;
+  std::set<VariationIDEntry> default_variation_ids_set_;
 
   // Variations ids from synthetic field trials.
-  std::set<VariationID> synthetic_variation_ids_set_;
+  std::set<VariationIDEntry> synthetic_variation_ids_set_;
 
-  std::string variation_ids_header_;
+  std::string cached_variation_ids_header_;
+  std::string cached_variation_ids_header_signed_in_;
 
   DISALLOW_COPY_AND_ASSIGN(VariationsHttpHeaderProvider);
 };

components/variations/variations_http_header_provider_unittest.cc

@@ -67,7 +67,7 @@ TEST_F(VariationsHttpHeaderProviderTest, SetDefaultVariationIds_Valid) {
   // Valid experiment ids.
   EXPECT_TRUE(provider.SetDefaultVariationIds({"12", "456", "t789"}));
   provider.InitVariationIDsCacheIfNeeded();
-  std::string variations = provider.GetClientDataHeader();
+  std::string variations = provider.GetClientDataHeader(false);
   EXPECT_FALSE(variations.empty());
   std::set<VariationID> variation_ids;
   std::set<VariationID> trigger_ids;
@@ -86,13 +86,13 @@ TEST_F(VariationsHttpHeaderProviderTest, SetDefaultVariationIds_Invalid) {
   EXPECT_FALSE(provider.SetDefaultVariationIds(
       std::vector<std::string>{"abcd12", "456"}));
   provider.InitVariationIDsCacheIfNeeded();
-  EXPECT_TRUE(provider.GetClientDataHeader().empty());
+  EXPECT_TRUE(provider.GetClientDataHeader(false).empty());
 
   // Invalid trigger experiment id
   EXPECT_FALSE(provider.SetDefaultVariationIds(
       std::vector<std::string>{"12", "tabc456"}));
   provider.InitVariationIDsCacheIfNeeded();
-  EXPECT_TRUE(provider.GetClientDataHeader().empty());
+  EXPECT_TRUE(provider.GetClientDataHeader(false).empty());
 }
 
 TEST_F(VariationsHttpHeaderProviderTest, OnFieldTrialGroupFinalized) {
@@ -104,25 +104,44 @@ TEST_F(VariationsHttpHeaderProviderTest, OnFieldTrialGroupFinalized) {
   const std::string default_name = "default";
   scoped_refptr<base::FieldTrial> trial_1(CreateTrialAndAssociateId(
       "t1", default_name, GOOGLE_WEB_PROPERTIES, 123));
-
   ASSERT_EQ(default_name, trial_1->group_name());
 
   scoped_refptr<base::FieldTrial> trial_2(CreateTrialAndAssociateId(
       "t2", default_name, GOOGLE_WEB_PROPERTIES_TRIGGER, 456));
-
   ASSERT_EQ(default_name, trial_2->group_name());
 
+  scoped_refptr<base::FieldTrial> trial_3(CreateTrialAndAssociateId(
+      "t3", default_name, GOOGLE_WEB_PROPERTIES_SIGNED_IN, 789));
+  ASSERT_EQ(default_name, trial_3->group_name());
+
   // Run the message loop to make sure OnFieldTrialGroupFinalized is called for
   // the two field trials.
   base::RunLoop().RunUntilIdle();
 
-  std::string variations = provider.GetClientDataHeader();
-
-  std::set<VariationID> variation_ids;
-  std::set<VariationID> trigger_ids;
-  ASSERT_TRUE(ExtractVariationIds(variations, &variation_ids, &trigger_ids));
-  EXPECT_TRUE(variation_ids.find(123) != variation_ids.end());
-  EXPECT_TRUE(trigger_ids.find(456) != trigger_ids.end());
+  // Get non-signed in ids.
+  {
+    std::string variations = provider.GetClientDataHeader(false);
+    std::set<VariationID> variation_ids;
+    std::set<VariationID> trigger_ids;
+    ASSERT_TRUE(ExtractVariationIds(variations, &variation_ids, &trigger_ids));
+    EXPECT_EQ(1U, variation_ids.size());
+    EXPECT_TRUE(variation_ids.find(123) != variation_ids.end());
+    EXPECT_EQ(1U, trigger_ids.size());
+    EXPECT_TRUE(trigger_ids.find(456) != trigger_ids.end());
+  }
+
+  // Now, get signed-in ids.
+  {
+    std::string variations = provider.GetClientDataHeader(true);
+    std::set<VariationID> variation_ids;
+    std::set<VariationID> trigger_ids;
+    ASSERT_TRUE(ExtractVariationIds(variations, &variation_ids, &trigger_ids));
+    EXPECT_EQ(2U, variation_ids.size());
+    EXPECT_TRUE(variation_ids.find(123) != variation_ids.end());
+    EXPECT_TRUE(variation_ids.find(789) != variation_ids.end());
+    EXPECT_EQ(1U, trigger_ids.size());
+    EXPECT_TRUE(trigger_ids.find(456) != trigger_ids.end());
+  }
 }
 
 TEST_F(VariationsHttpHeaderProviderTest, GetVariationsString) {
@@ -131,6 +150,8 @@ TEST_F(VariationsHttpHeaderProviderTest, GetVariationsString) {
 
   CreateTrialAndAssociateId("t1", "g1", GOOGLE_WEB_PROPERTIES, 123);
   CreateTrialAndAssociateId("t2", "g2", GOOGLE_WEB_PROPERTIES, 124);
+  // SIGNED_IN ids shouldn't be included.
+  CreateTrialAndAssociateId("t3", "g3", GOOGLE_WEB_PROPERTIES_SIGNED_IN, 125);
 
   VariationsHttpHeaderProvider provider;
   std::vector<std::string> ids;