From 7488bd4e3a3fbaaed13ce57ad4dfe6fa17e553c6 Mon Sep 17 00:00:00 2001 From: Gabriel Luong Date: Wed, 22 Nov 2023 13:53:17 -0500 Subject: [PATCH] Bug 1866133 - Add additional headers for Google search (cherry picked from commit 8144f31282bada16eab0a65fbc187948cf789204) --- .../mozilla/fenix/AppRequestInterceptor.kt | 34 ++- .../org/mozilla/fenix/components/Services.kt | 7 + .../fenix/components/UrlRequestInterceptor.kt | 70 ++++++ .../fenix/search/SearchDialogController.kt | 34 --- .../components/UrlRequestInterceptorTest.kt | 235 ++++++++++++++++++ .../search/SearchDialogControllerTest.kt | 77 ------ 6 files changed, 335 insertions(+), 122 deletions(-) create mode 100644 app/src/main/java/org/mozilla/fenix/components/UrlRequestInterceptor.kt create mode 100644 app/src/test/java/org/mozilla/fenix/components/UrlRequestInterceptorTest.kt diff --git a/app/src/main/java/org/mozilla/fenix/AppRequestInterceptor.kt b/app/src/main/java/org/mozilla/fenix/AppRequestInterceptor.kt index 036a37aa7..c1c00d499 100644 --- a/app/src/main/java/org/mozilla/fenix/AppRequestInterceptor.kt +++ b/app/src/main/java/org/mozilla/fenix/AppRequestInterceptor.kt @@ -27,6 +27,8 @@ class AppRequestInterceptor( this.navController = WeakReference(navController) } + override fun interceptsAppInitiatedRequests() = true + override fun onLoadRequest( engineSession: EngineSession, uri: String, @@ -37,17 +39,27 @@ class AppRequestInterceptor( isDirectNavigation: Boolean, isSubframeRequest: Boolean, ): RequestInterceptor.InterceptionResponse? { - return context.components.services.appLinksInterceptor - .onLoadRequest( - engineSession, - uri, - lastUri, - hasUserGesture, - isSameDomain, - isRedirect, - isDirectNavigation, - isSubframeRequest, - ) + val services = context.components.services + + return services.urlRequestInterceptor.onLoadRequest( + engineSession, + uri, + lastUri, + hasUserGesture, + isSameDomain, + isRedirect, + isDirectNavigation, + isSubframeRequest, + ) ?: services.appLinksInterceptor.onLoadRequest( + engineSession, + uri, + lastUri, + hasUserGesture, + isSameDomain, + isRedirect, + isDirectNavigation, + isSubframeRequest, + ) } override fun onErrorRequest( diff --git a/app/src/main/java/org/mozilla/fenix/components/Services.kt b/app/src/main/java/org/mozilla/fenix/components/Services.kt index 1623b24a8..918dfab86 100644 --- a/app/src/main/java/org/mozilla/fenix/components/Services.kt +++ b/app/src/main/java/org/mozilla/fenix/components/Services.kt @@ -11,6 +11,7 @@ import kotlinx.coroutines.launch import mozilla.components.feature.accounts.FirefoxAccountsAuthFeature import mozilla.components.feature.app.links.AppLinksInterceptor import mozilla.components.service.fxa.manager.FxaAccountManager +import org.mozilla.fenix.ext.application import org.mozilla.fenix.ext.settings import org.mozilla.fenix.perf.lazyMonitored import org.mozilla.fenix.settings.SupportUtils @@ -38,4 +39,10 @@ class Services( launchInApp = { context.settings().shouldOpenLinksInApp() }, ) } + + val urlRequestInterceptor by lazyMonitored { + UrlRequestInterceptor( + isDeviceRamAboveThreshold = context.application.isDeviceRamAboveThreshold, + ) + } } diff --git a/app/src/main/java/org/mozilla/fenix/components/UrlRequestInterceptor.kt b/app/src/main/java/org/mozilla/fenix/components/UrlRequestInterceptor.kt new file mode 100644 index 000000000..d267a6043 --- /dev/null +++ b/app/src/main/java/org/mozilla/fenix/components/UrlRequestInterceptor.kt @@ -0,0 +1,70 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.components + +import androidx.annotation.VisibleForTesting +import mozilla.components.concept.engine.EngineSession +import mozilla.components.concept.engine.EngineSession.LoadUrlFlags +import mozilla.components.concept.engine.EngineSession.LoadUrlFlags.Companion.ALLOW_ADDITIONAL_HEADERS +import mozilla.components.concept.engine.EngineSession.LoadUrlFlags.Companion.LOAD_FLAGS_BYPASS_LOAD_URI_DELEGATE +import mozilla.components.concept.engine.request.RequestInterceptor + +/** + * [RequestInterceptor] implementation for intercepting URL load requests to allow custom + * behaviour. + * + * @param isDeviceRamAboveThreshold Whether or not the device ram is above a threshold. + */ +class UrlRequestInterceptor(private val isDeviceRamAboveThreshold: Boolean) : RequestInterceptor { + + private val isGoogleSearchRequest by lazy { + Regex("^https://www\\.google\\.(?:.+)/search") + } + + @VisibleForTesting + internal fun getAdditionalHeaders(isDeviceRamAboveThreshold: Boolean): Map { + val value = if (isDeviceRamAboveThreshold) { + "1" + } else { + "0" + } + + return mapOf( + "X-Search-Subdivision" to value, + ) + } + + @VisibleForTesting + internal fun shouldInterceptRequest( + uri: String, + isSubframeRequest: Boolean, + ): Boolean { + return !isSubframeRequest && isGoogleSearchRequest.containsMatchIn(uri) + } + + override fun onLoadRequest( + engineSession: EngineSession, + uri: String, + lastUri: String?, + hasUserGesture: Boolean, + isSameDomain: Boolean, + isRedirect: Boolean, + isDirectNavigation: Boolean, + isSubframeRequest: Boolean, + ): RequestInterceptor.InterceptionResponse? { + if (!shouldInterceptRequest(uri = uri, isSubframeRequest = isSubframeRequest)) { + return null + } + + return RequestInterceptor.InterceptionResponse.Url( + url = uri, + flags = LoadUrlFlags.select( + LOAD_FLAGS_BYPASS_LOAD_URI_DELEGATE, + ALLOW_ADDITIONAL_HEADERS, + ), + additionalHeaders = getAdditionalHeaders(isDeviceRamAboveThreshold), + ) + } +} diff --git a/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt b/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt index 8a033baa4..74e830fce 100644 --- a/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt +++ b/app/src/main/java/org/mozilla/fenix/search/SearchDialogController.kt @@ -28,11 +28,9 @@ import org.mozilla.fenix.R import org.mozilla.fenix.components.Core import org.mozilla.fenix.components.metrics.MetricsUtils import org.mozilla.fenix.crashes.CrashListActivity -import org.mozilla.fenix.ext.application import org.mozilla.fenix.ext.navigateSafe import org.mozilla.fenix.ext.settings import org.mozilla.fenix.ext.telemetryName -import org.mozilla.fenix.search.awesomebar.AwesomeBarView.Companion.GOOGLE_SEARCH_ENGINE_NAME import org.mozilla.fenix.search.toolbar.SearchSelectorInteractor import org.mozilla.fenix.search.toolbar.SearchSelectorMenu import org.mozilla.fenix.settings.SupportUtils @@ -114,12 +112,6 @@ class SearchDialogController( val searchEngine = fragmentStore.state.searchEngineSource.searchEngine val isDefaultEngine = searchEngine == fragmentStore.state.defaultEngine - val additionalHeaders = getAdditionalHeaders(searchEngine) - val flags = if (additionalHeaders.isNullOrEmpty()) { - LoadUrlFlags.none() - } else { - LoadUrlFlags.select(LoadUrlFlags.ALLOW_ADDITIONAL_HEADERS) - } activity.openToBrowserAndLoad( searchTermOrURL = url, @@ -127,9 +119,7 @@ class SearchDialogController( from = BrowserDirection.FromSearchDialog, engine = searchEngine, forceSearch = !isDefaultEngine, - flags = flags, requestDesktopMode = fromHomeScreen && activity.settings().openNextTabInDesktopMode, - additionalHeaders = additionalHeaders, ) if (url.isUrl() || searchEngine == null) { @@ -195,12 +185,6 @@ class SearchDialogController( clearToolbarFocus() val searchEngine = fragmentStore.state.searchEngineSource.searchEngine - val additionalHeaders = getAdditionalHeaders(searchEngine) - val flags = if (additionalHeaders.isNullOrEmpty()) { - LoadUrlFlags.none() - } else { - LoadUrlFlags.select(LoadUrlFlags.ALLOW_ADDITIONAL_HEADERS) - } activity.openToBrowserAndLoad( searchTermOrURL = searchTerms, @@ -208,8 +192,6 @@ class SearchDialogController( from = BrowserDirection.FromSearchDialog, engine = searchEngine, forceSearch = true, - flags = flags, - additionalHeaders = additionalHeaders, ) val searchAccessPoint = when (fragmentStore.state.searchAccessPoint) { @@ -344,20 +326,4 @@ class SearchDialogController( create().withCenterAlignedButtons() } } - - private fun getAdditionalHeaders(searchEngine: SearchEngine?): Map? { - if (searchEngine?.name != GOOGLE_SEARCH_ENGINE_NAME) { - return null - } - - val value = if (activity.applicationContext.application.isDeviceRamAboveThreshold) { - "1" - } else { - "0" - } - - return mapOf( - "X-Search-Subdivision" to value, - ) - } } diff --git a/app/src/test/java/org/mozilla/fenix/components/UrlRequestInterceptorTest.kt b/app/src/test/java/org/mozilla/fenix/components/UrlRequestInterceptorTest.kt new file mode 100644 index 000000000..058c47e24 --- /dev/null +++ b/app/src/test/java/org/mozilla/fenix/components/UrlRequestInterceptorTest.kt @@ -0,0 +1,235 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.components + +import io.mockk.mockk +import mozilla.components.concept.engine.EngineSession +import mozilla.components.concept.engine.EngineSession.LoadUrlFlags +import mozilla.components.concept.engine.EngineSession.LoadUrlFlags.Companion.ALLOW_ADDITIONAL_HEADERS +import mozilla.components.concept.engine.EngineSession.LoadUrlFlags.Companion.LOAD_FLAGS_BYPASS_LOAD_URI_DELEGATE +import mozilla.components.concept.engine.request.RequestInterceptor +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mozilla.fenix.helpers.FenixRobolectricTestRunner + +@RunWith(FenixRobolectricTestRunner::class) +class UrlRequestInterceptorTest { + + private lateinit var engineSession: EngineSession + + @Before + fun setup() { + engineSession = mockk(relaxed = true) + } + + @Test + fun `GIVEN device is above threshold WHEN get additional headers is called THEN return the correct map of additional headers`() { + val isDeviceRamAboveThreshold = true + val urlRequestInterceptor = getUrlRequestInterceptor( + isDeviceRamAboveThreshold = isDeviceRamAboveThreshold, + ) + + assertEquals( + mapOf("X-Search-Subdivision" to "1"), + urlRequestInterceptor.getAdditionalHeaders(isDeviceRamAboveThreshold), + ) + } + + @Test + fun `GIVEN device is not above threshold WHEN get additional headers is called THEN return the correct map of additional headers`() { + val isDeviceRamAboveThreshold = false + val urlRequestInterceptor = getUrlRequestInterceptor( + isDeviceRamAboveThreshold = isDeviceRamAboveThreshold, + ) + + assertEquals( + mapOf("X-Search-Subdivision" to "0"), + urlRequestInterceptor.getAdditionalHeaders(isDeviceRamAboveThreshold), + ) + } + + @Test + fun `WHEN should intercept request is called THEN return the correct boolean value`() { + val urlRequestInterceptor = getUrlRequestInterceptor() + + assertFalse( + urlRequestInterceptor.shouldInterceptRequest( + uri = "https://getpocket.com", + isSubframeRequest = false, + ), + ) + assertFalse( + urlRequestInterceptor.shouldInterceptRequest( + uri = "https://www.google.com", + isSubframeRequest = false, + ), + ) + assertTrue( + urlRequestInterceptor.shouldInterceptRequest( + uri = "https://www.google.com/search?q=blue", + isSubframeRequest = false, + ), + ) + assertTrue( + urlRequestInterceptor.shouldInterceptRequest( + uri = "https://www.google.ca/search?q=red", + isSubframeRequest = false, + ), + ) + assertTrue( + urlRequestInterceptor.shouldInterceptRequest( + uri = "https://www.google.co.jp/search?q=red", + isSubframeRequest = false, + ), + ) + assertFalse( + urlRequestInterceptor.shouldInterceptRequest( + uri = "https://www.google.com/search?q=blue", + isSubframeRequest = true, + ), + ) + assertFalse( + urlRequestInterceptor.shouldInterceptRequest( + uri = "https://www.google.com/recaptcha", + isSubframeRequest = true, + ), + ) + } + + @Test + fun `WHEN a Pocket request is loaded THEN request is not intercepted`() { + val uri = "https://getpocket.com" + val response = getUrlRequestInterceptor().onLoadRequest( + uri = uri, + ) + + assertNull(response) + } + + @Test + fun `WHEN a Google request is loaded THEN request is not intercepted`() { + val uri = "https://www.google.com" + val response = getUrlRequestInterceptor().onLoadRequest( + uri = uri, + ) + + assertNull(response) + } + + @Test + fun `WHEN a Google search request is loaded THEN request is intercepted`() { + val uri = "https://www.google.com/search?q=blue" + + assertEquals( + RequestInterceptor.InterceptionResponse.Url( + url = uri, + flags = LoadUrlFlags.select( + LOAD_FLAGS_BYPASS_LOAD_URI_DELEGATE, + ALLOW_ADDITIONAL_HEADERS, + ), + additionalHeaders = mapOf( + "X-Search-Subdivision" to "1", + ), + ), + getUrlRequestInterceptor(isDeviceRamAboveThreshold = true).onLoadRequest( + uri = uri, + ), + ) + + assertEquals( + RequestInterceptor.InterceptionResponse.Url( + url = uri, + flags = LoadUrlFlags.select( + LOAD_FLAGS_BYPASS_LOAD_URI_DELEGATE, + ALLOW_ADDITIONAL_HEADERS, + ), + additionalHeaders = mapOf( + "X-Search-Subdivision" to "0", + ), + ), + getUrlRequestInterceptor(isDeviceRamAboveThreshold = false).onLoadRequest( + uri = uri, + ), + ) + } + + @Test + fun `WHEN a Google search request with a ca TLD request is loaded THEN request is intercepted`() { + val uri = "https://www.google.ca/search?q=red" + + assertEquals( + RequestInterceptor.InterceptionResponse.Url( + url = uri, + flags = LoadUrlFlags.select( + LOAD_FLAGS_BYPASS_LOAD_URI_DELEGATE, + ALLOW_ADDITIONAL_HEADERS, + ), + additionalHeaders = mapOf( + "X-Search-Subdivision" to "1", + ), + ), + getUrlRequestInterceptor(isDeviceRamAboveThreshold = true).onLoadRequest( + uri = uri, + ), + ) + + assertEquals( + RequestInterceptor.InterceptionResponse.Url( + url = uri, + flags = LoadUrlFlags.select( + LOAD_FLAGS_BYPASS_LOAD_URI_DELEGATE, + ALLOW_ADDITIONAL_HEADERS, + ), + additionalHeaders = mapOf( + "X-Search-Subdivision" to "0", + ), + ), + getUrlRequestInterceptor(isDeviceRamAboveThreshold = false).onLoadRequest( + uri = uri, + ), + ) + } + + @Test + fun `WHEN a Google subframe request is loaded THEN request is not intercepted`() { + val uri = "https://www.google.com/search?q=blue" + + assertNull( + getUrlRequestInterceptor(isDeviceRamAboveThreshold = true).onLoadRequest( + uri = uri, + isSubframeRequest = true, + ), + ) + } + + private fun getUrlRequestInterceptor(isDeviceRamAboveThreshold: Boolean = false) = + UrlRequestInterceptor( + isDeviceRamAboveThreshold = isDeviceRamAboveThreshold, + ) + + private fun UrlRequestInterceptor.onLoadRequest( + uri: String, + lastUri: String? = null, + hasUserGesture: Boolean = false, + isSameDomain: Boolean = false, + isRedirect: Boolean = false, + isDirectNavigation: Boolean = false, + isSubframeRequest: Boolean = false, + ) = this.onLoadRequest( + engineSession = engineSession, + uri = uri, + lastUri = lastUri, + hasUserGesture = hasUserGesture, + isSameDomain = isSameDomain, + isRedirect = isRedirect, + isDirectNavigation = isDirectNavigation, + isSubframeRequest = isSubframeRequest, + ) +} diff --git a/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt b/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt index d2845d61a..fcb7fb6f8 100644 --- a/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt +++ b/app/src/test/java/org/mozilla/fenix/search/SearchDialogControllerTest.kt @@ -48,11 +48,9 @@ import org.mozilla.fenix.R import org.mozilla.fenix.browser.browsingmode.BrowsingMode import org.mozilla.fenix.components.Core import org.mozilla.fenix.components.metrics.MetricsUtils -import org.mozilla.fenix.ext.application import org.mozilla.fenix.helpers.FenixRobolectricTestRunner import org.mozilla.fenix.search.SearchDialogFragmentDirections.Companion.actionGlobalAddonsManagementFragment import org.mozilla.fenix.search.SearchDialogFragmentDirections.Companion.actionGlobalSearchEngineFragment -import org.mozilla.fenix.search.awesomebar.AwesomeBarView.Companion.GOOGLE_SEARCH_ENGINE_NAME import org.mozilla.fenix.search.toolbar.SearchSelectorMenu import org.mozilla.fenix.settings.SupportUtils import org.mozilla.fenix.utils.Settings @@ -90,7 +88,6 @@ class SearchDialogControllerTest { ) every { store.state.tabId } returns "test-tab-id" every { store.state.searchEngineSource.searchEngine } returns searchEngine - every { searchEngine.name } returns "" every { searchEngine.type } returns SearchEngine.Type.BUNDLED every { navController.currentDestination } returns mockk { every { id } returns R.id.searchDialogFragment @@ -121,8 +118,6 @@ class SearchDialogControllerTest { from = BrowserDirection.FromSearchDialog, engine = searchEngine, forceSearch = false, - flags = EngineSession.LoadUrlFlags.none(), - additionalHeaders = null, ) } @@ -154,8 +149,6 @@ class SearchDialogControllerTest { from = BrowserDirection.FromSearchDialog, engine = searchEngine, forceSearch = true, - flags = EngineSession.LoadUrlFlags.none(), - additionalHeaders = null, ) } @@ -169,70 +162,6 @@ class SearchDialogControllerTest { assertEquals("false", snapshot.single().extra?.getValue("autocomplete")) } - @Test - fun `GIVEN Google search engine is selected and device ram is above threshold WHEN url is committed THEN perform search`() { - val searchTerm = "coffee" - assertNull(Events.enteredUrl.testGetValue()) - - every { searchEngine.name } returns GOOGLE_SEARCH_ENGINE_NAME - every { store.state.defaultEngine } returns searchEngine - every { activity.applicationContext.application.isDeviceRamAboveThreshold } returns true - - createController().handleUrlCommitted(searchTerm) - - browserStore.waitUntilIdle() - - verify { - activity.openToBrowserAndLoad( - searchTermOrURL = searchTerm, - newTab = false, - from = BrowserDirection.FromSearchDialog, - engine = searchEngine, - forceSearch = false, - flags = EngineSession.LoadUrlFlags.select(EngineSession.LoadUrlFlags.ALLOW_ADDITIONAL_HEADERS), - additionalHeaders = mapOf( - "X-Search-Subdivision" to "1", - ), - ) - } - - middleware.assertLastAction(AwesomeBarAction.EngagementFinished::class) { action -> - assertFalse(action.abandoned) - } - } - - @Test - fun `GIVEN Google search engine is selected and device ram is below threshold WHEN url is committed THEN perform search`() { - val searchTerm = "coffee" - assertNull(Events.enteredUrl.testGetValue()) - - every { searchEngine.name } returns GOOGLE_SEARCH_ENGINE_NAME - every { store.state.defaultEngine } returns searchEngine - every { activity.applicationContext.application.isDeviceRamAboveThreshold } returns false - - createController().handleUrlCommitted(searchTerm) - - browserStore.waitUntilIdle() - - verify { - activity.openToBrowserAndLoad( - searchTermOrURL = searchTerm, - newTab = false, - from = BrowserDirection.FromSearchDialog, - engine = searchEngine, - forceSearch = false, - flags = EngineSession.LoadUrlFlags.select(EngineSession.LoadUrlFlags.ALLOW_ADDITIONAL_HEADERS), - additionalHeaders = mapOf( - "X-Search-Subdivision" to "0", - ), - ) - } - - middleware.assertLastAction(AwesomeBarAction.EngagementFinished::class) { action -> - assertFalse(action.abandoned) - } - } - @Test fun handleBlankUrlCommitted() { val url = "" @@ -268,8 +197,6 @@ class SearchDialogControllerTest { from = BrowserDirection.FromSearchDialog, engine = searchEngine, forceSearch = true, - flags = EngineSession.LoadUrlFlags.none(), - additionalHeaders = null, ) } @@ -358,8 +285,6 @@ class SearchDialogControllerTest { newTab = false, from = BrowserDirection.FromSearchDialog, engine = searchEngine, - flags = EngineSession.LoadUrlFlags.none(), - additionalHeaders = null, ) } @@ -496,8 +421,6 @@ class SearchDialogControllerTest { from = BrowserDirection.FromSearchDialog, engine = searchEngine, forceSearch = true, - flags = EngineSession.LoadUrlFlags.none(), - additionalHeaders = null, ) }