Make geolocation API and X-Geo header access consistent

chrome/browser/android/search_geolocation/search_geolocation_service.cc

+// Copyright 2017 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 "chrome/browser/android/search_geolocation/search_geolocation_service.h"
+
+#include "base/feature_list.h"
+#include "base/values.h"
+#include "chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.h"
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/search_engines/ui_thread_search_terms_data.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/common/pref_names.h"
+#include "components/content_settings/core/browser/content_settings_utils.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/content_settings/core/common/content_settings_types.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+#include "components/prefs/pref_service.h"
+#include "components/search_engines/template_url.h"
+#include "components/search_engines/template_url_service.h"
+#include "url/url_constants.h"
+
+namespace {
+
+const char kIsGoogleSearchEngineKey[] = "is_google_search_engine";
+const char kCCTLDKey[] = "cctld";
+const char kDSESettingKey[] = "dse_setting";
+
+}  // namespace
+
+struct SearchGeolocationService::PrefValue {
+  bool is_google_search_engine;
+  url::Origin cctld;
+  bool setting;
+};
+
+// static
+void SearchGeolocationService::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterDictionaryPref(prefs::kGoogleDSEGeolocationSetting);
+}
+
+SearchGeolocationService::SearchGeolocationService(Profile* profile)
+    : profile_(profile),
+      pref_service_(profile_->GetPrefs()),
+      host_content_settings_map_(
+          HostContentSettingsMapFactory::GetForProfile(profile_)),
+      template_url_service_(
+          TemplateURLServiceFactory::GetForProfile(profile_)) {
+  if (!UseConsistentSearchGeolocation())
+    return;
+
+  template_url_service_->AddObserver(this);
+
+  InitializeDSEGeolocationSettingIfNeeded();
+  EnsureDSEGeolocationSettingIsValid();

[raymes, 2017/01/09 05:47:26]

It might be worth adding a comment about why this is needed at startup?

[benwells, 2017/01/09 21:02:22]

Done.

+}
+
+bool SearchGeolocationService::UseDSEGeolocationSetting(
+    const url::Origin& requesting_origin) {
+  if (profile_->IsOffTheRecord())
+    return false;
+
+  if (!UseConsistentSearchGeolocation())
+    return false;

[raymes, 2017/01/09 05:47:26]

nit: should this just be the first check?

[benwells, 2017/01/09 21:02:22]

Done.

+
+  if (requesting_origin.scheme() != url::kHttpsScheme)
+    return false;
+

[raymes, 2017/01/09 05:47:26]

I think we could just add an "IsUserSettable()" check here too which would guard
against enterprise policy, though it's not strictly needed.

[benwells, 2017/01/09 21:02:22]

The enterprise policy could force geolocation on, so I think a check here could
introduce problems.

[raymes, 2017/01/10 03:19:32]

Hmm, but wouldn't we fall back to using the enterprise policy in that case?
Either way it's not a problem as it shouldn't affect most cases..

[benwells, 2017/01/11 05:35:32]

After our discussion offline, I've made it so if !IsUseSettable and content
setting is not ALLOW we return false. That will let the geolocation header
respect all enterprise policy settings - allow, ask and block.

+  return requesting_origin.IsSameOriginWith(GetDSECCTLD());
+}
+
+bool SearchGeolocationService::GetDSEGeolocationSetting() {

[raymes, 2017/01/09 05:47:26]

I vaguely remember talking about ensuring validity at this point, to guard
against the content setting changing for weird reasons, like enterprise policy
being enforced, etc. I wonder if that's still a good idea? 

[benwells, 2017/01/09 21:02:22]

I'm not sure if it's needed or not. I've added in the call with a TODO to check.

+  return GetDSEGeolocationPref().setting;
+}
+
+void SearchGeolocationService::SetDSEGeolocationSetting(bool setting) {
+  PrefValue pref = GetDSEGeolocationPref();
+  if (setting == pref.setting)
+    return;
+

[raymes, 2017/01/09 05:47:26]

As mentioned, I think I'd prefer having 
if (!IsUserSettable()) return; here and not the call to
EnsureDSEGeolocatioNSettingIsValid below. I think it makes it clearer what's
going on.

[benwells, 2017/01/09 21:02:22]

Done.

+  pref.setting = setting;
+  SetDSEGeolocationPref(pref);
+
+  ResetContentSetting();
+
+  // The UI layer currently (January 2017) has no knowledge of enterprise
+  // policy, and allows users to edit settings in this state. If the user does
+  // this they may change their DSE geolocation setting when they shouldn't be
+  // able to, so we make sure the state is valid now.
+  EnsureDSEGeolocationSettingIsValid();
+}
+
+void SearchGeolocationService::Shutdown() {
+  if (UseConsistentSearchGeolocation())
+    template_url_service_->RemoveObserver(this);
+}
+
+// static
+SearchGeolocationService*
+SearchGeolocationService::Factory::GetForBrowserContext(
+    content::BrowserContext* context) {
+  return static_cast<SearchGeolocationService*>(GetInstance()
+      ->GetServiceForBrowserContext(context, true));
+}
+
+// static
+SearchGeolocationService::Factory*
+SearchGeolocationService::Factory::GetInstance() {
+  return base::Singleton<SearchGeolocationService::Factory>::get();
+}
+
+SearchGeolocationService::Factory::Factory()
+    : BrowserContextKeyedServiceFactory(
+          "SearchGeolocationService",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+SearchGeolocationService::Factory::~Factory() {}
+
+bool SearchGeolocationService::Factory::ServiceIsCreatedWithBrowserContext()
+    const {
+  return true;
+}
+
+KeyedService* SearchGeolocationService::Factory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  return new SearchGeolocationService(Profile::FromBrowserContext(context));
+}
+
+SearchGeolocationService::~SearchGeolocationService() {}
+
+void SearchGeolocationService::OnTemplateURLServiceChanged() {
+  bool is_now_google_search_engine = IsGoogleSearchEngine();
+  PrefValue pref = GetDSEGeolocationPref();
+  ContentSetting content_setting = GetCurrentContentSetting();
+
+  if (is_now_google_search_engine) {
+    if (content_setting == CONTENT_SETTING_BLOCK && pref.setting) {
+      pref.setting = false;
+    } else if (content_setting == CONTENT_SETTING_ALLOW && !pref.setting) {
+      ResetContentSetting();
+    }
+  }
+
+  if (is_now_google_search_engine && !pref.is_google_search_engine &&
+      pref.setting) {
+    SearchGeolocationDisclosureTabHelper::ResetDisclosure(profile_);
+  }
+
+  pref.is_google_search_engine = is_now_google_search_engine;
+  pref.cctld = GetDSECCTLD();
+  SetDSEGeolocationPref(pref);
+}
+
+void SearchGeolocationService::InitializeDSEGeolocationSettingIfNeeded() {
+  // Initialize the pref if it hasn't been initialize yet.

[raymes, 2017/01/09 05:47:26]

nit: initialized

[benwells, 2017/01/09 21:02:22]

Done.

+  if (!pref_service_->HasPrefPath(prefs::kGoogleDSEGeolocationSetting)) {
+    ContentSetting content_setting = GetCurrentContentSetting();
+
+    PrefValue pref;
+    pref.is_google_search_engine = IsGoogleSearchEngine();
+    pref.cctld = GetDSECCTLD();
+    pref.setting = content_setting != CONTENT_SETTING_BLOCK;
+    SetDSEGeolocationPref(pref);
+
+    SearchGeolocationDisclosureTabHelper::ResetDisclosure(profile_);
+  }
+}
+
+void SearchGeolocationService::EnsureDSEGeolocationSettingIsValid() {
+  PrefValue pref = GetDSEGeolocationPref();
+  ContentSetting content_setting = GetCurrentContentSetting();
+  bool new_setting = pref.setting;
+
+  if (pref.setting && content_setting == CONTENT_SETTING_BLOCK) {
+    new_setting = false;
+  } else if (!pref.setting && content_setting == CONTENT_SETTING_ALLOW) {
+    new_setting = true;
+  }
+
+  if (pref.setting != new_setting) {
+    pref.setting = new_setting;
+    SetDSEGeolocationPref(pref);
+  }
+}
+
+bool SearchGeolocationService::IsGoogleSearchEngine() {
+  const TemplateURL* template_url =
+      template_url_service_->GetDefaultSearchProvider();
+  return template_url &&
+         template_url->HasGoogleBaseURLs(UIThreadSearchTermsData(profile_));
+}
+
+url::Origin SearchGeolocationService::GetDSECCTLD() {
+  if (!IsGoogleSearchEngine())
+    return url::Origin();
+
+  GURL origin_url(UIThreadSearchTermsData(profile_).GoogleBaseURLValue());
+  return url::Origin(origin_url);
+}
+
+SearchGeolocationService::PrefValue
+SearchGeolocationService::GetDSEGeolocationPref() {
+  const base::DictionaryValue* dict =
+      pref_service_->GetDictionary(prefs::kGoogleDSEGeolocationSetting);
+
+  PrefValue pref;
+  bool is_google_search_engine;
+  std::string cctld_string;
+  bool setting;
+  if (dict->GetBoolean(kIsGoogleSearchEngineKey, &is_google_search_engine) &&
+      dict->GetString(kCCTLDKey, &cctld_string) &&
+      dict->GetBoolean(kDSESettingKey, &setting)) {
+    pref.is_google_search_engine = is_google_search_engine;
+    pref.cctld = url::Origin(GURL(cctld_string));
+    pref.setting = setting;
+  }
+
+  return pref;

[raymes, 2017/01/09 05:47:26]

I think the struct will be uninitialized? We could give it a constructor to make
sure it's initialized as expected.

[benwells, 2017/01/09 21:02:22]

Done (via default values).

+}
+
+void SearchGeolocationService::SetDSEGeolocationPref(
+    const SearchGeolocationService::PrefValue& pref) {
+  base::DictionaryValue dict;
+  dict.SetBoolean(kIsGoogleSearchEngineKey, pref.is_google_search_engine);
+  dict.SetString(kCCTLDKey, pref.cctld.Serialize());
+  dict.SetBoolean(kDSESettingKey, pref.setting);
+  pref_service_->Set(prefs::kGoogleDSEGeolocationSetting, dict);
+}
+
+ContentSetting SearchGeolocationService::GetCurrentContentSetting() {
+  content_settings::SettingInfo info;
+  url::Origin origin = GetDSECCTLD();
+  std::unique_ptr<base::Value> value =
+      host_content_settings_map_->GetWebsiteSetting(

[raymes, 2017/01/09 05:47:26]

Since you're not using "info" here, you can just call GetContentSetting and
avoid the conversion below.

[benwells, 2017/01/09 21:02:22]

Done.

+          origin.GetURL(), origin.GetURL(), CONTENT_SETTINGS_TYPE_GEOLOCATION,
+          std::string(), &info);
+  return content_settings::ValueToContentSetting(value.get());
+}
+
+void SearchGeolocationService::ResetContentSetting() {
+  url::Origin origin = GetDSECCTLD();
+  host_content_settings_map_->SetContentSettingDefaultScope(
+      origin.GetURL(), origin.GetURL(), CONTENT_SETTINGS_TYPE_GEOLOCATION,
+      std::string(), CONTENT_SETTING_DEFAULT);
+}
+
+bool SearchGeolocationService::UseConsistentSearchGeolocation() {
+  return base::FeatureList::IsEnabled(features::kConsistentOmniboxGeolocation);
+}