Fix X-Geo header not sent despite user explicitly allowing geolocation.

After the DSE automatic permissions are revoked, this is happening
because GeolocationHeader.java calls `IsPermissionControlledByDSE`
which checks whether the permission was auto-granted instead of checking
if the permission was granted.

Bug: 1242895
Change-Id: I247c8037bb48f2c407cb24fb7dce9f2d1cb446c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3116265
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Commit-Queue: Krishna Govind <govind@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Filip Gorski <fgorski@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/main@{#914887}

chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderTest.java

@@ -26,6 +26,7 @@ import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
 import org.chromium.chrome.test.util.browser.Features.DisableFeatures;
+import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
 import org.chromium.components.browser_ui.site_settings.PermissionInfo;
 import org.chromium.components.content_settings.ContentSettingValues;
 import org.chromium.components.content_settings.ContentSettingsType;
@@ -36,7 +37,6 @@ import org.chromium.content_public.browser.test.util.TestThreadUtils;
  */
 @RunWith(ChromeJUnit4ClassRunner.class)
 @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
-@DisableFeatures({ChromeFeatureList.REVERT_DSE_AUTOMATIC_PERMISSIONS})
 public class GeolocationHeaderTest {
     @Rule
     public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
@@ -60,6 +60,7 @@ public class GeolocationHeaderTest {
     @Feature({"Location"})
     @CommandLineFlags.Add({GOOGLE_BASE_URL_SWITCH})
     public void testConsistentHeader() {
+        setPermission(ContentSettingValues.ALLOW);
         long now = setMockLocationNow();
 
         // X-Geo should be sent for Google search results page URLs.
@@ -86,7 +87,8 @@ public class GeolocationHeaderTest {
     @SmallTest
     @Feature({"Location"})
     @CommandLineFlags.Add({GOOGLE_BASE_URL_SWITCH})
-    public void testPermission() {
+    @DisableFeatures(ChromeFeatureList.REVERT_DSE_AUTOMATIC_PERMISSIONS)
+    public void testPermissionWithAutogrant() {
         long now = setMockLocationNow();
 
         // X-Geo shouldn't be sent when location is disallowed for the origin.
@@ -98,10 +100,26 @@ public class GeolocationHeaderTest {
         checkHeaderWithPermission(ContentSettingValues.DEFAULT, now, false);
     }
 
+    @Test
+    @SmallTest
+    @Feature({"Location"})
+    @CommandLineFlags.Add({GOOGLE_BASE_URL_SWITCH})
+    @EnableFeatures(ChromeFeatureList.REVERT_DSE_AUTOMATIC_PERMISSIONS)
+    public void testPermissionWithoutAutogrant() {
+        long now = setMockLocationNow();
+
+        // X-Geo should be sent if DSE autogrant is enabled only if the user has explicitly allowed
+        // geolocation.
+        checkHeaderWithPermission(ContentSettingValues.ALLOW, now, false);
+        checkHeaderWithPermission(ContentSettingValues.BLOCK, now, true);
+        checkHeaderWithPermission(ContentSettingValues.DEFAULT, now, true);
+    }
+
     @Test
     @SmallTest
     @Feature({"Location"})
     public void testProtoEncoding() {
+        setPermission(ContentSettingValues.ALLOW);
         long now = setMockLocationNow();
 
         // X-Geo should be sent for Google search results page URLs using proto encoding.
@@ -112,6 +130,7 @@ public class GeolocationHeaderTest {
     @SmallTest
     @Feature({"Location"})
     public void testGpsFallback() {
+        setPermission(ContentSettingValues.ALLOW);
         // Only GPS location, should be sent when flag is on.
         long now = System.currentTimeMillis();
         Location gpsLocation = generateMockLocation(LocationManager.GPS_PROVIDER, now);
@@ -124,6 +143,7 @@ public class GeolocationHeaderTest {
     @SmallTest
     @Feature({"Location"})
     public void testGpsFallbackYounger() {
+        setPermission(ContentSettingValues.ALLOW);
         long now = System.currentTimeMillis();
         // GPS location is younger.
         Location gpsLocation = generateMockLocation(LocationManager.GPS_PROVIDER, now + 100);
@@ -139,6 +159,7 @@ public class GeolocationHeaderTest {
     @SmallTest
     @Feature({"Location"})
     public void testGpsFallbackOlder() {
+        setPermission(ContentSettingValues.ALLOW);
         long now = System.currentTimeMillis();
         // GPS location is older.
         Location gpsLocation = generateMockLocation(LocationManager.GPS_PROVIDER, now - 100);
@@ -246,4 +267,12 @@ public class GeolocationHeaderTest {
         String expectedHeader = "X-Geo: w " + locationProto;
         Assert.assertEquals(expectedHeader, header);
     }
+
+    private void setPermission(final @ContentSettingValues int setting) {
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            PermissionInfo infoHttps =
+                    new PermissionInfo(ContentSettingsType.GEOLOCATION, SEARCH_URL_1, null, false);
+            infoHttps.setContentSetting(Profile.getLastUsedRegularProfile(), setting);
+        });
+    }
 }

chrome/browser/android/search_permissions/search_permissions_service.cc

@@ -195,6 +195,11 @@ bool SearchPermissionsService::IsPermissionControlledByDSE(
   return true;
 }
 
+bool SearchPermissionsService::IsDseOrigin(const url::Origin& origin) {
+  return origin.scheme() == url::kHttpsScheme &&
+         origin.IsSameOriginWith(delegate_->GetDSEOrigin());
+}
+
 void SearchPermissionsService::ResetDSEPermission(ContentSettingsType type) {
   url::Origin dse_origin = delegate_->GetDSEOrigin();
   GURL dse_url = dse_origin.GetURL();

chrome/browser/android/search_permissions/search_permissions_service.h

@@ -80,6 +80,9 @@ class SearchPermissionsService : public KeyedService {
   bool IsPermissionControlledByDSE(ContentSettingsType type,
                                    const url::Origin& requesting_origin);
 
+  // Returns whether the given origin matches the DSE origin.
+  bool IsDseOrigin(const url::Origin& origin);
+
   // Resets the DSE permission for a single ContentSettingsType.
   void ResetDSEPermission(ContentSettingsType type);
 

chrome/browser/permissions/chrome_permissions_client.cc

@@ -340,6 +340,14 @@ bool ChromePermissionsClient::IsPermissionControlledByDse(
          search_helper->IsPermissionControlledByDSE(type, origin);
 }
 
+bool ChromePermissionsClient::IsDseOrigin(
+    content::BrowserContext* browser_context,
+    const url::Origin& origin) {
+  SearchPermissionsService* search_helper =
+      SearchPermissionsService::Factory::GetForBrowserContext(browser_context);
+  return search_helper && search_helper->IsDseOrigin(origin);
+}
+
 bool ChromePermissionsClient::ResetPermissionIfControlledByDse(
     content::BrowserContext* browser_context,
     ContentSettingsType type,

chrome/browser/permissions/chrome_permissions_client.h

@@ -72,6 +72,8 @@ class ChromePermissionsClient : public permissions::PermissionsClient {
   bool IsPermissionControlledByDse(content::BrowserContext* browser_context,
                                    ContentSettingsType type,
                                    const url::Origin& origin) override;
+  bool IsDseOrigin(content::BrowserContext* browser_context,
+                   const url::Origin& origin) override;
   bool ResetPermissionIfControlledByDse(
       content::BrowserContext* browser_context,
       ContentSettingsType type,

chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java

@@ -447,11 +447,9 @@ public class GeolocationHeader {
      */
     static boolean isLocationDisabledForUrl(Profile profile, Uri uri) {
         boolean enabled =
-                // TODO(raymes): The call to isPermissionControlledByDSE is only needed if this
-                // could be called for an origin that isn't the default search engine. Otherwise
-                // remove this line.
-                WebsitePreferenceBridge.isPermissionControlledByDSE(
-                        profile, ContentSettingsType.GEOLOCATION, uri.toString())
+                // TODO(raymes): The call to isDSEOrigin is only needed if this could be called for
+                // an origin that isn't the default search engine. Otherwise remove this line.
+                WebsitePreferenceBridge.isDSEOrigin(profile, uri.toString())
                 && locationContentSettingForUrl(profile, uri) == ContentSettingValues.ALLOW;
         return !enabled;
     }

chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeaderUnitTest.java

@@ -141,6 +141,9 @@ public class GeolocationHeaderUnitTest {
         when(mWebsitePreferenceBridgeJniMock.isPermissionControlledByDSE(
                      any(BrowserContextHandle.class), anyInt(), anyString()))
                 .thenReturn(true);
+        when(mWebsitePreferenceBridgeJniMock.isDSEOrigin(
+                     any(BrowserContextHandle.class), anyString()))
+                .thenReturn(true);
         when(mUrlUtilitiesJniMock.isGoogleSearchUrl(anyString())).thenReturn(true);
         when(mProfileJniMock.fromWebContents(any(WebContents.class))).thenReturn(mProfileMock);
         when(mProfileMock.isOffTheRecord()).thenReturn(false);

components/browser_ui/site_settings/android/java/src/org/chromium/components/browser_ui/site_settings/WebsitePreferenceBridge.java

@@ -153,6 +153,13 @@ public class WebsitePreferenceBridge {
                 browserContextHandle, contentSettingsType, origin);
     }
 
+    /**
+     * Returns whether the DSE (Default Search Engine) origin matches the given origin.
+     */
+    public static boolean isDSEOrigin(BrowserContextHandle browserContextHandle, String origin) {
+        return WebsitePreferenceBridgeJni.get().isDSEOrigin(browserContextHandle, origin);
+    }
+
     /**
      * Returns whether this origin is activated for ad blocking, and will have resources blocked
      * unless they are explicitly allowed via a permission.
@@ -355,6 +362,7 @@ public class WebsitePreferenceBridge {
                 int value);
         boolean isPermissionControlledByDSE(BrowserContextHandle browserContextHandle,
                 @ContentSettingsType int contentSettingsType, String origin);
+        boolean isDSEOrigin(BrowserContextHandle browserContextHandle, String origin);
         boolean getAdBlockingActivated(BrowserContextHandle browserContextHandle, String origin);
         boolean isContentSettingEnabled(
                 BrowserContextHandle browserContextHandle, int contentSettingType);

components/browser_ui/site_settings/android/website_preference_bridge.cc

@@ -716,6 +716,15 @@ static jboolean JNI_WebsitePreferenceBridge_IsPermissionControlledByDSE(
       url::Origin::Create(GURL(ConvertJavaStringToUTF8(env, jorigin))));
 }
 
+static jboolean JNI_WebsitePreferenceBridge_IsDSEOrigin(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& jbrowser_context_handle,
+    const JavaParamRef<jstring>& jorigin) {
+  return permissions::PermissionsClient::Get()->IsDseOrigin(
+      unwrap(jbrowser_context_handle),
+      url::Origin::Create(GURL(ConvertJavaStringToUTF8(env, jorigin))));
+}
+
 static jboolean JNI_WebsitePreferenceBridge_GetAdBlockingActivated(
     JNIEnv* env,
     const JavaParamRef<jobject>& jbrowser_context_handle,

components/permissions/permissions_client.cc

@@ -122,6 +122,11 @@ bool PermissionsClient::IsPermissionControlledByDse(
   return false;
 }
 
+bool PermissionsClient::IsDseOrigin(content::BrowserContext* browser_context,
+                                    const url::Origin& origin) {
+  return false;
+}
+
 bool PermissionsClient::ResetPermissionIfControlledByDse(
     content::BrowserContext* browser_context,
     ContentSettingsType type,

components/permissions/permissions_client.h

@@ -189,6 +189,11 @@ class PermissionsClient {
       ContentSettingsType type,
       const url::Origin& origin);
 
+  // Returns whether the given origin matches the default search engine (DSE)
+  // origin.
+  virtual bool IsDseOrigin(content::BrowserContext* browser_context,
+                           const url::Origin& origin);
+
   // Resets the permission if it's controlled by the default search
   // engine (DSE). The return value is true if the permission was reset.
   virtual bool ResetPermissionIfControlledByDse(

weblayer/browser/default_search_engine.cc

@@ -24,6 +24,10 @@ bool IsPermissionControlledByDse(ContentSettingsType type,
   return type == ContentSettingsType::GEOLOCATION && GetDseOrigin() == origin;
 }
 
+bool IsDseOrigin(const url::Origin& origin) {
+  return GetDseOrigin() == origin;
+}
+
 void ResetDsePermissions(content::BrowserContext* browser_context) {
   // Incognito should still have to prompt for permissions.
   if (browser_context->IsOffTheRecord())

weblayer/browser/default_search_engine.h

@@ -25,6 +25,10 @@ const url::Origin& GetDseOrigin();
 bool IsPermissionControlledByDse(ContentSettingsType type,
                                  const url::Origin& origin);
 
+// Returns whether the provided |origin| matches the Weblayer's default search
+// engine logic.
+bool IsDseOrigin(const url::Origin& origin);
+
 // Resets all permissions managed by WebLayer for the default search engine.
 // TODO(crbug.com/1063433): If this logic gets more complicated consider
 // refactoring SearchPermissionsService to be used in WebLayer.

weblayer/browser/permissions/weblayer_permissions_client.cc

@@ -74,6 +74,12 @@ bool WebLayerPermissionsClient::IsPermissionControlledByDse(
   return weblayer::IsPermissionControlledByDse(type, origin);
 }
 
+bool WebLayerPermissionsClient::IsDseOrigin(
+    content::BrowserContext* browser_context,
+    const url::Origin& origin) {
+  return weblayer::IsDseOrigin(origin);
+}
+
 bool WebLayerPermissionsClient::ResetPermissionIfControlledByDse(
     content::BrowserContext* browser_context,
     ContentSettingsType type,

weblayer/browser/permissions/weblayer_permissions_client.h

@@ -37,6 +37,8 @@ class WebLayerPermissionsClient : public permissions::PermissionsClient {
   bool IsPermissionControlledByDse(content::BrowserContext* browser_context,
                                    ContentSettingsType type,
                                    const url::Origin& origin) override;
+  bool IsDseOrigin(content::BrowserContext* browser_context,
+                   const url::Origin& origin) override;
   bool ResetPermissionIfControlledByDse(
       content::BrowserContext* browser_context,
       ContentSettingsType type,