From cc617abea21cae45dd13e87ae391b63ecf433130 Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Sun, 31 May 2020 12:19:18 +0000 Subject: [PATCH] Bug 1641261 - Handle the case when an app provided engine is hidden/removed, and a WebExtension wants to select as default. r=daleharvey Differential Revision: https://phabricator.services.mozilla.com/D77429 --- .../parent/ext-chrome-settings-overrides.js | 5 ++- ...r_ext_settings_overrides_default_search.js | 42 +++++++++++++++++++ toolkit/components/search/SearchService.jsm | 36 ++++++++++++---- .../components/search/nsISearchService.idl | 7 +++- .../tests/xpcshell/test_override_allowlist.js | 20 ++++++++- 5 files changed, 99 insertions(+), 11 deletions(-) diff --git a/browser/components/extensions/parent/ext-chrome-settings-overrides.js b/browser/components/extensions/parent/ext-chrome-settings-overrides.js index 8f6a3c87370f..319d45580e0b 100644 --- a/browser/components/extensions/parent/ext-chrome-settings-overrides.js +++ b/browser/components/extensions/parent/ext-chrome-settings-overrides.js @@ -379,8 +379,11 @@ this.chrome_settings_overrides = class extends ExtensionAPI { let engineName = searchProvider.name.trim(); if (searchProvider.is_default) { - if (await Services.search.maybeSetAndOverrideDefault(extension)) { + let result = await Services.search.maybeSetAndOverrideDefault(extension); + if (result.canChangeToAppProvided) { await this.setDefault(engineName); + } + if (!result.canInstallEngine) { return; } } diff --git a/browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js b/browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js index 1bd671244150..29257cd169de 100644 --- a/browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js +++ b/browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js @@ -63,6 +63,48 @@ add_task(async function test_extension_setting_default_engine() { ); }); +/* This tests what happens when the engine you're setting it to is hidden. */ +add_task(async function test_extension_setting_default_engine_hidden() { + let engine = Services.search.getEngineByName("DuckDuckGo"); + engine.hidden = true; + + let ext1 = ExtensionTestUtils.loadExtension({ + manifest: { + chrome_settings_overrides: { + search_provider: { + name: "DuckDuckGo", + search_url: "https://example.com/?q={searchTerms}", + is_default: true, + }, + }, + }, + useAddonManager: "temporary", + }); + + await ext1.startup(); + await AddonTestUtils.waitForSearchProviderStartup(ext1); + + is( + (await Services.search.getDefault()).name, + defaultEngineName, + "Default engine should have remained as the default" + ); + is( + ExtensionSettingsStore.getSetting("default_search", "defaultSearch"), + null, + "The extension should not have been recorded as having set the default search" + ); + + await ext1.unload(); + + is( + (await Services.search.getDefault()).name, + defaultEngineName, + `Default engine is ${defaultEngineName}` + ); + engine.hidden = false; +}); + // Test the popup displayed when trying to add a non-built-in default // search engine. add_task(async function test_extension_setting_default_engine_external() { diff --git a/toolkit/components/search/SearchService.jsm b/toolkit/components/search/SearchService.jsm index 4e8ad8db45e8..8e8d5e42ab12 100644 --- a/toolkit/components/search/SearchService.jsm +++ b/toolkit/components/search/SearchService.jsm @@ -717,8 +717,15 @@ SearchService.prototype = { let searchProvider = extension.manifest.chrome_settings_overrides.search_provider; let engine = this._engines.get(searchProvider.name); - if (!engine || !engine.isAppProvided) { - return false; + if (!engine || !engine.isAppProvided || engine.hidden) { + // If the engine is not application provided, then we shouldn't simply + // set default to it. + // If the engine is application provided, but hidden, then we don't + // switch to it, nor do we try to install it. + return { + canChangeToAppProvided: false, + canInstallEngine: !engine?.hidden, + }; } let params = this.getEngineParams( extension, @@ -736,7 +743,10 @@ SearchService.prototype = { ) { // Don't allow an extension to set the default if it is already the default. if (this.defaultEngine.name == searchProvider.name) { - return false; + return { + canChangeToAppProvided: false, + canInstallEngine: false, + }; } if ( !(await this._defaultOverrideAllowlist.canOverride( @@ -750,7 +760,10 @@ SearchService.prototype = { ); // We don't allow overriding the engine in this case, but we can allow // the extension to change the default engine. - return true; + return { + canChangeToAppProvided: true, + canInstallEngine: false, + }; } // We're ok to override. engine.overrideWithExtension(params); @@ -758,7 +771,10 @@ SearchService.prototype = { "Allowing default engine to be set to app-provided and overridden.", extension.id ); - return true; + return { + canChangeToAppProvided: true, + canInstallEngine: false, + }; } if ( @@ -773,10 +789,16 @@ SearchService.prototype = { "Re-enabling overriding of core extension by", extension.id ); - return true; + return { + canChangeToAppProvided: true, + canInstallEngine: false, + }; } - return false; + return { + canChangeToAppProvided: false, + canInstallEngine: false, + }; }, /** diff --git a/toolkit/components/search/nsISearchService.idl b/toolkit/components/search/nsISearchService.idl index 9f047cc0b627..bb52a9b36d0b 100644 --- a/toolkit/components/search/nsISearchService.idl +++ b/toolkit/components/search/nsISearchService.idl @@ -500,8 +500,11 @@ interface nsISearchService : nsISupports * * @param extension * The extension to load from. - * @returns A boolean that indicates if the WebExtension engine may set the - * named engine as default (because it is application provided). + * @returns An object with two booleans: + * - canChangeToAppProvided: indicates if the WebExtension engine may + * set the named engine as default e.g. it is application provided. + * - canInstallEngine: indicates if the WebExtension engine may be + * installed, e.g. it is not an app-provided engine. */ Promise maybeSetAndOverrideDefault(in jsval extension); diff --git a/toolkit/components/search/tests/xpcshell/test_override_allowlist.js b/toolkit/components/search/tests/xpcshell/test_override_allowlist.js index c4be56882dcd..5e262bef71bf 100644 --- a/toolkit/components/search/tests/xpcshell/test_override_allowlist.js +++ b/toolkit/components/search/tests/xpcshell/test_override_allowlist.js @@ -29,6 +29,7 @@ const tests = [ }, expected: { switchToDefaultAllowed: false, + canInstallEngine: true, overridesEngine: false, }, }, @@ -43,6 +44,7 @@ const tests = [ }, expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: false, }, }, @@ -57,6 +59,7 @@ const tests = [ }, expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: false, }, }, @@ -76,6 +79,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: true, searchUrl: kSearchEngineURL, }, @@ -96,6 +100,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: true, searchUrl: kSearchEngineURL, }, @@ -116,6 +121,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: false, }, }, @@ -137,6 +143,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: true, searchUrl: `${kBaseURL}?q={searchTerms}&enc=UTF-8`, }, @@ -159,6 +166,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: false, }, }, @@ -180,6 +188,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: true, searchUrl: `${kBaseURL}`, postData: "q={searchTerms}&enc=UTF-8", @@ -203,6 +212,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: false, }, }, @@ -224,6 +234,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: true, searchUrl: `${kBaseURL}`, searchForm: "https://example.com/form", @@ -247,6 +258,7 @@ const tests = [ ], expected: { switchToDefaultAllowed: true, + canInstallEngine: false, overridesEngine: false, }, }, @@ -300,11 +312,17 @@ for (const test of tests) { ]); } + let result = await Services.search.maybeSetAndOverrideDefault(extension); Assert.equal( - await Services.search.maybeSetAndOverrideDefault(extension), + result.canChangeToAppProvided, test.expected.switchToDefaultAllowed, "Should have returned the correct value for allowing switch to default or not." ); + Assert.equal( + result.canInstallEngine, + test.expected.canInstallEngine, + "Should have returned the correct value for allowing to install the engine or not." + ); let engine = await Services.search.getEngineByName(kOverriddenEngineName); Assert.equal(