Reland "cros-geo-apikey: Update ChromeOS surfaces to use new API keys"

This reverts commit 31a89b88e6.

Reason for revert: google_internal needed a manual roll up - https://chromium-review.googlesource.com/c/chromium/src/+/6401294

Original change's description:
> Revert "cros-geo-apikey: Update ChromeOS surfaces to use new API keys"
>
> This reverts commit f1341b6f55.
>
> Reason for revert: many crashing tests when is_chrome_branded is set
> b/406579271
>
> Original change's description:
> > cros-geo-apikey: Update ChromeOS surfaces to use new API keys
> >
> > Introduce new methods for ChromeOS to access the ChromeOS-specific GCP
> > API keys. Update the `SimpleGeolocationRequest` and
> > `ChromeContentBrowserClient` to use these new keys instead.
> >
> > Bug: 394051078
> > Change-Id: I36b6995b3bfb7edd81ac63d785c0a8946101137b
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6361503
> > Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
> > Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> > Commit-Queue: Zauri Meshveliani <zauri@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1438300}
>
> Bug: 394051078, 406579271
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Change-Id: Ie920746087812a1fd118b66031e7bd42cf8c210f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6399493
> Auto-Submit: Joel Hockey <joelhockey@chromium.org>
> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Owners-Override: Joel Hockey <joelhockey@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1438546}

Bug: 394051078, 406579271
Change-Id: I53f5a383a367c1a21b32ce899dbb9e635049a11a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6401295
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1438987}

chrome/browser/chrome_content_browser_client.cc

@@ -3776,6 +3776,11 @@ ChromeContentBrowserClient::GetSystemNetworkContext() {
 }
 
 std::string ChromeContentBrowserClient::GetGeolocationApiKey() {
+#if BUILDFLAG(IS_CHROMEOS)
+  if (ash::features::IsCrosSeparateGeoApiKeyEnabled()) {
+    return google_apis::GetCrosChromeGeoAPIKey();
+  }
+#endif
   return google_apis::GetAPIKey();
 }
 

chrome/browser/chrome_content_browser_client_unittest.cc

@@ -147,6 +147,9 @@
 #include "chromeos/components/kiosk/kiosk_utils.h"
 #include "components/user_manager/scoped_user_manager.h"
 #include "content/public/test/scoped_web_ui_controller_factory_registration.h"
+#include "google_apis/api_key_cache.h"
+#include "google_apis/default_api_keys.h"
+#include "google_apis/google_api_keys.h"
 #endif  // BUILDFLAG(IS_CHROMEOS)
 
 #if BUILDFLAG(IS_WIN)
@@ -1378,6 +1381,50 @@ TEST_F(ChromeContentBrowserClientTest, RequestFileAccessDeny) {
                             continuation_callback.GetCallback());
   EXPECT_FALSE(continuation_callback.Take().is_allowed());
 }
+
+namespace override_geo_api_keys {
+
+// We start every test by creating a clean environment for the
+// preprocessor defines used in define_baked_in_api_keys-inc.cc
+#undef GOOGLE_API_KEY
+#undef GOOGLE_API_KEY_CROS_SYSTEM_GEO
+#undef GOOGLE_API_KEY_CROS_CHROME_GEO
+
+// Set Geolocation-specific keys.
+#define GOOGLE_API_KEY "bogus_api_key"
+#define GOOGLE_API_KEY_CROS_SYSTEM_GEO "bogus_cros_system_geo_api_key"
+#define GOOGLE_API_KEY_CROS_CHROME_GEO "bogus_cros_chrome_geo_api_key"
+
+// This file must be included after the internal files defining official keys.
+#include "google_apis/default_api_keys-inc.cc"
+
+}  // namespace override_geo_api_keys
+
+// Test that when `kCrosSeparateGeoApiKey` feature is enabled,
+// Chrome-on-ChromeOS switches to using a separate (ChromeOS-specific) API Key
+// for the location requests.
+TEST_F(ChromeContentBrowserClientTest, UseCorrectGeoAPIKey) {
+  auto default_key_values =
+      override_geo_api_keys::GetDefaultApiKeysFromDefinedValues();
+  default_key_values.allow_unset_values = true;
+  google_apis::ApiKeyCache api_key_cache(default_key_values);
+  auto scoped_override =
+      google_apis::SetScopedApiKeyCacheForTesting(&api_key_cache);
+
+  // Check that by default Chrome-on-ChromeOS uses shared API key for
+  // geolocation requests.
+  ChromeContentBrowserClient client;
+  EXPECT_EQ(client.GetGeolocationApiKey(), google_apis::GetAPIKey());
+
+  // Check that when the `kCrosSeparateGeoApiKey` feature is enabled,
+  // Chrome-on-ChromeOS uses ChromeOS-specific API key for geolocation.
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      ash::features::kCrosSeparateGeoApiKey);
+  EXPECT_EQ(client.GetGeolocationApiKey(),
+            google_apis::GetCrosChromeGeoAPIKey());
+}
+
 #endif  // BUILDFLAG(IS_CHROMEOS)
 
 class ChromeContentBrowserClientSwitchTest

chromeos/ash/components/geolocation/BUILD.gn

@@ -36,6 +36,7 @@ source_set("unit_tests") {
     "//base/test:test_support",
     "//chromeos/ash/components/dbus/shill",
     "//chromeos/ash/components/network:test_support",
+    "//google_apis",
     "//net",
     "//services/network:test_support",
     "//services/network/public/cpp",

chromeos/ash/components/geolocation/simple_geolocation_request.cc

@@ -11,6 +11,7 @@
 #include <string>
 #include <utility>
 
+#include "ash/constants/ash_features.h"
 #include "base/functional/bind.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
@@ -153,7 +154,12 @@ GURL GeolocationRequestURL(const GURL& url) {
   if (url != SimpleGeolocationProvider::DefaultGeolocationProviderURL())
     return url;
 
-  std::string api_key = google_apis::GetAPIKey();
+  std::string api_key;
+  if (features::IsCrosSeparateGeoApiKeyEnabled()) {
+    api_key = google_apis::GetCrosSystemGeoAPIKey();
+  } else {
+    api_key = google_apis::GetAPIKey();
+  }
   if (api_key.empty())
     return url;
 
@@ -478,6 +484,10 @@ std::string SimpleGeolocationRequest::FormatRequestBodyForTesting() const {
   return FormatRequestBody();
 }
 
+GURL SimpleGeolocationRequest::GetServiceURLForTesting() const {
+  return request_url_;
+}
+
 void SimpleGeolocationRequest::Retry(bool server_error) {
   base::TimeDelta delay(server_error ? retry_sleep_on_server_error_
                                      : retry_sleep_on_bad_response_);

chromeos/ash/components/geolocation/simple_geolocation_request.h

@@ -85,6 +85,7 @@ class COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_GEOLOCATION)
   static void SetTestMonitor(SimpleGeolocationRequestTestMonitor* monitor);
 
   std::string FormatRequestBodyForTesting() const;
+  GURL GetServiceURLForTesting() const;
 
  private:
   void OnSimpleURLLoaderComplete(std::unique_ptr<std::string> response_body);

chromeos/ash/components/geolocation/simple_geolocation_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 
+#include "ash/constants/ash_features.h"
 #include "ash/constants/geolocation_access_level.h"
 #include "base/containers/contains.h"
 #include "base/functional/bind.h"
@@ -14,6 +15,7 @@
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "base/time/time.h"
 #include "chromeos/ash/components/dbus/shill/shill_manager_client.h"
@@ -21,6 +23,9 @@
 #include "chromeos/ash/components/geolocation/simple_geolocation_request_test_monitor.h"
 #include "chromeos/ash/components/network/geolocation_handler.h"
 #include "chromeos/ash/components/network/network_handler_test_helper.h"
+#include "google_apis/api_key_cache.h"
+#include "google_apis/default_api_keys.h"
+#include "google_apis/google_api_keys.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
@@ -117,7 +122,12 @@ class TestGeolocationAPILoaderFactory : public network::TestURLLoaderFactory {
   }
 
   void Intercept(const network::ResourceRequest& request) {
-    EXPECT_EQ(url_, request.url);
+    // Drop the query component potentially appended by the
+    // `SimpleGeolocationRequest` class.
+    GURL::Replacements replacements;
+    replacements.ClearQuery();
+    EXPECT_EQ(url_.ReplaceComponents(replacements),
+              request.url.ReplaceComponents(replacements));
 
     SimpleGeolocationProvider* provider =
         SimpleGeolocationProvider::GetInstance();
@@ -188,14 +198,17 @@ class WirelessTestMonitor : public SimpleGeolocationRequestTestMonitor {
   void OnRequestCreated(SimpleGeolocationRequest* request) override {}
   void OnStart(SimpleGeolocationRequest* request) override {
     last_request_body_ = request->FormatRequestBodyForTesting();
+    last_request_url_ = request->GetServiceURLForTesting();
     ++requests_count_;
   }
 
   const std::string& last_request_body() const { return last_request_body_; }
+  GURL last_request_url() const { return last_request_url_; }
   unsigned int requests_count() const { return requests_count_; }
 
  private:
   std::string last_request_body_;
+  GURL last_request_url_;
   unsigned int requests_count_ = 0;
 };
 
@@ -399,6 +412,93 @@ TEST_F(SimpleGeolocationTest, SystemGeolocationPermissionDenied) {
   }
 }
 
+namespace override_geo_api_keys {
+
+// We start every test by creating a clean environment for the
+// preprocessor defines used in define_baked_in_api_keys-inc.cc
+#undef GOOGLE_API_KEY
+#undef GOOGLE_API_KEY_CROS_SYSTEM_GEO
+#undef GOOGLE_API_KEY_CROS_CHROME_GEO
+
+// Set Geolocation-specific keys.
+#define GOOGLE_API_KEY "bogus_api_key"
+#define GOOGLE_API_KEY_CROS_SYSTEM_GEO "bogus_cros_system_geo_api_key"
+#define GOOGLE_API_KEY_CROS_CHROME_GEO "bogus_cros_chrome_geo_api_key"
+
+// This file must be included after the internal files defining official keys.
+#include "google_apis/default_api_keys-inc.cc"
+
+}  // namespace override_geo_api_keys
+
+class SimpleGeolocationAPIKeyTest : public SimpleGeolocationTest,
+                                    public testing::WithParamInterface<bool> {
+ public:
+  SimpleGeolocationAPIKeyTest() : is_separate_api_keys_enabled_(GetParam()) {
+    if (is_separate_api_keys_enabled_) {
+      feature_list_.InitAndEnableFeature(features::kCrosSeparateGeoApiKey);
+    } else {
+      feature_list_.InitAndDisableFeature(features::kCrosSeparateGeoApiKey);
+    }
+  }
+
+  void SetUp() override {
+    SimpleGeolocationTest::SetUp();
+
+    // Set URL to the production value to let the `SimpleGeolocationRequest`
+    // extend it with the API keys.
+    SimpleGeolocationProvider::GetInstance()
+        ->SetGeolocationProviderUrlForTesting(
+            SimpleGeolocationProvider::DefaultGeolocationProviderURL()
+                .spec()
+                .c_str());
+    url_factory_.Configure(
+        SimpleGeolocationProvider::DefaultGeolocationProviderURL(),
+        net::HTTP_OK, kSimpleResponseBody, 0);
+  }
+
+  const bool is_separate_api_keys_enabled_;
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_P(SimpleGeolocationAPIKeyTest, TestCorrectAPIKeysAreUsed) {
+  GeolocationReceiver receiver;
+  WirelessTestMonitor requests_monitor;
+  SimpleGeolocationRequest::SetTestMonitor(&requests_monitor);
+
+  // Override the `ApiKeyCache` with the bogus values from above.
+  auto default_key_values =
+      override_geo_api_keys::GetDefaultApiKeysFromDefinedValues();
+  default_key_values.allow_unset_values = true;
+  google_apis::ApiKeyCache api_key_cache(default_key_values);
+  auto scoped_key_cache_override =
+      google_apis::SetScopedApiKeyCacheForTesting(&api_key_cache);
+
+  // Request geolocation and wait for the response.
+  EnableGeolocationUsage();
+  SimpleGeolocationProvider::GetInstance()->RequestGeolocation(
+      base::Seconds(1), false, false,
+      base::BindOnce(&GeolocationReceiver::OnRequestDone,
+                     base::Unretained(&receiver)),
+      SimpleGeolocationProvider::ClientId::kForTesting);
+  receiver.WaitUntilRequestDone();
+
+  // Check that the appropriate API key was used depending on the
+  // `CrosSeparateGeoApiKey` feature status.
+  const GURL request_url = requests_monitor.last_request_url();
+  ASSERT_TRUE(request_url.has_query());
+  EXPECT_EQ(is_separate_api_keys_enabled_,
+            request_url.query().find(GOOGLE_API_KEY_CROS_SYSTEM_GEO) !=
+                std::string::npos);
+  EXPECT_EQ(is_separate_api_keys_enabled_,
+            request_url.query().find(GOOGLE_API_KEY) == std::string::npos);
+  EXPECT_EQ(1U, url_factory_.attempts());
+}
+
+// GetParam() - `ash::features::kCrosSeparateGeoApiKey` feature state.
+INSTANTIATE_TEST_SUITE_P(All, SimpleGeolocationAPIKeyTest, testing::Bool());
+
 // Test sending of WiFi Access points and Cell Towers.
 // (This is mostly derived from GeolocationHandlerTest.)
 class SimpleGeolocationWirelessTest : public SimpleGeolocationTestBase,

google_apis/api_key_cache.cc

@@ -180,6 +180,21 @@ ApiKeyCache::ApiKeyCache(const DefaultApiKeys& default_api_keys) {
       environment.get(), command_line, gaia_config,
       default_api_keys.allow_override_via_environment,
       default_api_keys.allow_unset_values);
+
+  api_key_cros_system_geo_ = CalculateKeyValue(
+      default_api_keys.google_api_key_cros_system_geo_,
+      STRINGIZE_NO_EXPANSION(GOOGLE_API_KEY_CROS_SYSTEM_GEO), nullptr,
+      std::string(), environment.get(), command_line, gaia_config,
+      default_api_keys.allow_override_via_environment,
+      default_api_keys.allow_unset_values);
+
+  api_key_cros_chrome_geo_ = CalculateKeyValue(
+      default_api_keys.google_api_key_cros_chrome_geo_,
+      STRINGIZE_NO_EXPANSION(GOOGLE_API_KEY_CROS_CHROME_GEO), nullptr,
+      std::string(), environment.get(), command_line, gaia_config,
+      default_api_keys.allow_override_via_environment,
+      default_api_keys.allow_unset_values);
+
 #endif
 
   metrics_key_ = CalculateKeyValue(

google_apis/api_key_cache.h

@@ -38,6 +38,12 @@ class COMPONENT_EXPORT(GOOGLE_APIS) ApiKeyCache {
   const std::string& api_key_read_aloud() const { return api_key_read_aloud_; }
   const std::string& api_key_fresnel() const { return api_key_fresnel_; }
   const std::string& api_key_boca() const { return api_key_boca_; }
+  const std::string& api_key_cros_system_geo() const {
+    return api_key_cros_system_geo_;
+  }
+  const std::string& api_key_cros_chrome_geo() const {
+    return api_key_cros_chrome_geo_;
+  }
 #endif
 
   const std::string& metrics_key() const { return metrics_key_; }
@@ -67,6 +73,8 @@ class COMPONENT_EXPORT(GOOGLE_APIS) ApiKeyCache {
   std::string api_key_read_aloud_;
   std::string api_key_fresnel_;
   std::string api_key_boca_;
+  std::string api_key_cros_system_geo_;
+  std::string api_key_cros_chrome_geo_;
 #endif
 
   std::string metrics_key_;

google_apis/default_api_keys-inc.cc

@@ -92,6 +92,19 @@
 #if !defined(GOOGLE_API_KEY_BOCA)
 #define GOOGLE_API_KEY_BOCA google_apis::DefaultApiKeys::kUnsetApiToken
 #endif
+
+// API key for ChromeOS system services to resolve location.
+#if !defined(GOOGLE_API_KEY_CROS_SYSTEM_GEO)
+#define GOOGLE_API_KEY_CROS_SYSTEM_GEO \
+  google_apis::DefaultApiKeys::kUnsetApiToken
+#endif
+
+// API key for ChromeOS Chrome to resolve location.
+#if !defined(GOOGLE_API_KEY_CROS_CHROME_GEO)
+#define GOOGLE_API_KEY_CROS_CHROME_GEO \
+  google_apis::DefaultApiKeys::kUnsetApiToken
+#endif
+
 #endif  // BUILDFLAG(IS_CHROMEOS)
 
 // These are used as shortcuts for developers and users providing
@@ -127,6 +140,8 @@ constexpr ::google_apis::DefaultApiKeys GetDefaultApiKeysFromDefinedValues() {
       .google_api_key_read_aloud = GOOGLE_API_KEY_READ_ALOUD,
       .google_api_key_fresnel = GOOGLE_API_KEY_FRESNEL,
       .google_api_key_boca = GOOGLE_API_KEY_BOCA,
+      .google_api_key_cros_system_geo_ = GOOGLE_API_KEY_CROS_SYSTEM_GEO,
+      .google_api_key_cros_chrome_geo_ = GOOGLE_API_KEY_CROS_CHROME_GEO,
 #endif
       .google_client_id_main = GOOGLE_CLIENT_ID_MAIN,
       .google_client_secret_main = GOOGLE_CLIENT_SECRET_MAIN,

google_apis/default_api_keys.h

@@ -34,6 +34,8 @@ struct DefaultApiKeys {
   const char* google_api_key_read_aloud;
   const char* google_api_key_fresnel;
   const char* google_api_key_boca;
+  const char* google_api_key_cros_system_geo_;
+  const char* google_api_key_cros_chrome_geo_;
 #endif
 
   const char* google_client_id_main;

google_apis/google_api_keys.cc

@@ -114,6 +114,13 @@ const std::string& GetFresnelAPIKey() {
 const std::string& GetBocaAPIKey() {
   return GetApiKeyCacheInstance().api_key_boca();
 }
+
+const std::string& GetCrosSystemGeoAPIKey() {
+  return GetApiKeyCacheInstance().api_key_cros_system_geo();
+}
+const std::string& GetCrosChromeGeoAPIKey() {
+  return GetApiKeyCacheInstance().api_key_cros_chrome_geo();
+}
 #endif
 
 const std::string& GetMetricsKey() {

google_apis/google_api_keys.h

@@ -113,6 +113,14 @@ COMPONENT_EXPORT(GOOGLE_APIS) const std::string& GetFresnelAPIKey();
 
 // Retrieves the Boca API Key.
 COMPONENT_EXPORT(GOOGLE_APIS) const std::string& GetBocaAPIKey();
+
+// Retrieves the ChromeOS-specific Geolocation API Key used by the System
+// Services.
+COMPONENT_EXPORT(GOOGLE_APIS) const std::string& GetCrosSystemGeoAPIKey();
+// Retrieves the ChromeOS-specific Geolocation API Key used by the Chrome
+// browser.
+COMPONENT_EXPORT(GOOGLE_APIS) const std::string& GetCrosChromeGeoAPIKey();
+
 #endif
 
 // Retrieves the key used to sign metrics (UMA/UKM) uploads.