Closed Bug 1428948 Opened 6 years ago Closed 6 years ago

Search engine policies: add / remove / set default / block web API

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox60 + verified
firefox61 --- verified

People

(Reporter: Felipe, Assigned: bytesized)

References

Details

Attachments

(1 file)

Bug 1428948 - Add policies to modify the available search engines
59 bytes, text/x-review-board-request
Felipe
: review+
florian
: review+
jcristau
: approval-mozilla-beta+
Details 
There are requests for policies to do these 4 tasks related to search engines:

- add an engine
- remove engines
- choose the default one
- block installation of new ones via the web api
Assignee: nobody → ksteuber
Note: Removing search engines is not part of the MVP. That functionality will probably not be part of this patch and will probably not be available in Firefox 60.
Felipe- I have been running into some implementation problems with the policy. However, I think I have figured out a solution and I want to know what you think.

The problems:
  Search engines can be added by config url or by description. Do we want a separate policy for both?
  Order is a problem. We don't want to run "set default search engine" before we run "add search engine". Nor do we want to run "block installation of search engines" before we run "add search engines". Currently, there is no real way to ensure that policies are run in any particular order.

Possible solution:
  They can all be one policy!

It would probably look something like this:

"SearchEngines": {
  "AddByConfigURL": [/* array of objects describing search engines */],
  "AddByDescription": [/* array of objects describing search engines */],
  "SetDefault": "Engine Name",
  "PreventInstalls: true
}

This seems like it neatly solves all of the problems as long as we are ok with the policy looking like this. What do you think?
Flags: needinfo?(felipc)
(In reply to Kirk Steuber [:bytesized] from comment #2)
> The problems:
>   Search engines can be added by config url or by description. Do we want a
> separate policy for both?

I don't know what's the difference here. Can you give an example of what a config url vs. a description looks like?

Needinfo'ing mkaply to check what does CCK2 support

If "description" means "a js object with the details for the engine", I imagine supporting just this format should be enough. Unless mkaply says that both are important

> Possible solution:
>   They can all be one policy!

Yeah, sounds like a plan!
Flags: needinfo?(felipc) → needinfo?(mozilla)
That idea sounds good to me.

I was trying to figure out how specifying the engine would work in practice. I think it's probably a URL (or possibly a path?) and you would use addEngine with that URL to add the engine. I can't find any good details in GPO of how you would point to a file. And even in the JSON case,, I'm not sure.

Add by Description is a great idea although I might name it differently. I'm not sure how much of the stuff we pass to add EngineWithDetails we would want. I guess name, URL and icon? So:

engine {
   name: "My Google",
   url: "http://www.google.com/search?q={searchTerms}
   icon: "http://www.google.com/favicon.ico"
}

I think that would be enough. I don't picture folks wanting search other things.

As I think about this more, I'm wondering if this is the only method we support. Not allow to specify XML files at all. This seems like it would meet the need.

As far as removing goes, the primary ask I ever received was removing all engines. There was never a requirement to remove individual engines.
Flags: needinfo?(mozilla)
The "prevent installation" policy description in the spreadsheet reads "Don't allow search engines to be installed from web pages". I just wanted to verify that this is exactly what the policy should do. The policy should not prevent search engine installation via, for example, addons, right?
Flags: needinfo?(mozilla)
It does two things. It makes it so that installing engines on web pages that have opensearch doesn't work and that the AddSearchProvider API doesn't work.

See:

https://github.com/mkaply/cck2wizard/blob/master/cck2/modules/CCK2Framescript.js#L13
and
https://github.com/mkaply/cck2wizard/blob/461023f4f8ac38a030a16db6edc7c2d3b6e52809/cck2/modules/CCK2.jsm#L800

Mike
Flags: needinfo?(mozilla)
Attachment #8956565 - Flags: review?(florian)
Attachment #8956565 - Flags: review?(felipc)
Comment on attachment 8956565 [details]
Bug 1428948 - Add policies to modify the available search engines

https://reviewboard.mozilla.org/r/225468/#review231394

I just reviewed policies-schema.json. I'll delegate the rest of the review to Florian

::: browser/components/enterprisepolicies/schemas/policies-schema.json:275
(Diff revision 1)
> +            "required": ["Name", "URLTemplate"],
> +

can you make Method required too?

::: browser/components/enterprisepolicies/schemas/policies-schema.json:282
(Diff revision 1)
> +            "properties": {
> +              "Name": {
> +                "type": "string"
> +              },
> +              "IconURL": {
> +                "type": "URL"

so it turns out that, due to the way that GPO works, any optional URL will have to be of type URLOrEmpty
Attachment #8956565 - Flags: review?(felipc) → review+
> can you make Method required too?

99.99% of search engines are GET so I'm not sure we need to make that required.

> so it turns out that, due to the way that GPO works, any optional URL will have to be of type URLOrEmpty

URLorEmpty.

Maybe we should change that :)
(In reply to Mike Kaply [:mkaply] from comment #9)
> > can you make Method required too?
> 
> 99.99% of search engines are GET so I'm not sure we need to make that
> required.

Since Method is an enum, I said that because I think it might be needed for the GPO template. But if you think it can be implemented without requiring that, fine by me

> 
> > so it turns out that, due to the way that GPO works, any optional URL will have to be of type URLOrEmpty
> 
> URLorEmpty.

I keep making that mistake :S
> Since Method is an enum, I said that because I think it might be needed for the GPO template. But if you think it can be implemented without requiring that, fine by me

Yes, it can. You can make it optional, but the GPO enum will always have POST.
Comment on attachment 8956565 [details]
Bug 1428948 - Add policies to modify the available search engines

https://reviewboard.mozilla.org/r/225468/#review231674

::: browser/components/enterprisepolicies/Policies.jsm:274
(Diff revision 2)
>        setAndLockPref("signon.rememberSignons", param);
>      }
>    },
> +
> +  "SearchEngines": {
> +    onBeforeUIStartup(manager, param) {

Can this run after first paint? Initializing the search service is currently delayed until after first paint.

::: browser/components/enterprisepolicies/Policies.jsm:291
(Diff revision 2)
> +              description: newEngine.Description,
> +              method:      newEngine.Method,
> +              suggestURL:  newEngine.SuggestURLTemplate
> +            };
> +            try {
> +              Services.search.addEngineWithDetails(newEngine.Name,

Looks a lot like this is forcing a sync initialization of the search service. Any reason why this can't wait until the search service is done initializing itself asynchronously?

::: browser/components/enterprisepolicies/Policies.jsm:294
(Diff revision 2)
> +            };
> +            try {
> +              Services.search.addEngineWithDetails(newEngine.Name,
> +                                                   newEngineParameters);
> +            } catch (ex) {
> +              log.error("Unable to add search engine", ex);

You'll be in that failure case whenever we attempt to update engines. Is that ok?

::: browser/components/enterprisepolicies/Policies.jsm:303
(Diff revision 2)
> +      }
> +      if (param.Default) {
> +        runOnce("setDefaultSearchEngine", () => {
> +          let defaultEngine;
> +          try {
> +            defaultEngine = Services.search.getEngineByName(param.Default);

Same here, can't this wait until the async init is done?

::: browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:33
(Diff revision 2)
> +      }
> +    }
> +  });
> +});
> +
> +add_task(async function test_install_and_set_default() {

There's no reason to split this in a separate add_task call, it's part of the same test as lines 10-31.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:40
(Diff revision 2)
> +  // *and* was properly set as the default.
> +  is(Services.search.currentEngine.name, "MozSearch",
> +     "Specified search engine should be the default");
> +
> +  // Clean up
> +  Services.search.removeEngine(Services.search.currentEngine);

Should deactivating a policy automatically remove the engines it had added? If so, should we test it here?

::: browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:43
(Diff revision 2)
> +
> +  // Clean up
> +  Services.search.removeEngine(Services.search.currentEngine);
> +});
> +
> +add_task(async function setup_prevent_installs() {

Should the test for preventing installs be moved to its own file?

::: browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:59
(Diff revision 2)
> +  // Check that about:preferences does not prompt user to install search engines
> +  // if that feature is disabled
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:preferences");
> +  await ContentTask.spawn(tab.linkedBrowser, null, async function() {
> +    let linkContainer = content.document.getElementById("addEnginesBox");
> +    if (!linkContainer.hidden) {

Why is this test expecting to (sometimes?) not have this link hidden immediately? Is this going to visibly flicker when users open preferences?

::: browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:80
(Diff revision 2)
> +  // Check that search engines cannot be added via opensearch
> +  await SpecialPowers.pushPrefEnv({ set: [
> +    ["browser.search.widget.inNavBar", true],
> +  ]});
> +  let rootDir = getRootDirectory(gTestPath);
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, rootDir + "opensearch.html");

I'm afraid this test is a bit fragile. If somehow we fail to detect the open search engine provided by the page, the whole test would still pass despite not testing anything. I'm wondering if this test code should be moved to a helper method taking a boolean, and run once before applying the policy, and once after, to ensure the result is different.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:82
(Diff revision 2)
> +    ["browser.search.widget.inNavBar", true],
> +  ]});
> +  let rootDir = getRootDirectory(gTestPath);
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, rootDir + "opensearch.html");
> +  let searchPopup = document.getElementById("PopupSearchAutoComplete");
> +  let searchBar = document.getElementById("searchbar");

We should verify here that the searchBar doesn't have the 'addengines' attribute. It's what triggers showing the green (+) badge.
The next few lines opening the panel and checking that we don't have any 'add engine' items don't hurt, but I don't think they are really useful, as you are blocking this at the time we detect these engines on the page's HTML markup. We we don't show the green badge, that confirms we have found no engine on the page.

::: browser/components/enterprisepolicies/tests/browser/opensearch.html:5
(Diff revision 2)
> +<!DOCTYPE html>
> +<html>
> +<head>
> +<meta charset="UTF-8">
> +<link rel="search" type="application/opensearchdescription+xml" title="newEngine" href="http://mochi.test:8888/browser/browser/components/enterprisepolicies/tests/browser/opensearchEngine.xml">

Would this still work with a relative url in the href attribute? It would look nicer.

::: browser/modules/ContentLinkHandler.jsm:341
(Diff revision 2)
>            iconAdded = handleFaviconLink(link, isRichIcon, chromeGlobal, faviconLoads);
>            break;
>          case "search":
> +          if (Services.policies &&
> +              !Services.policies.isAllowed("installSearchEngine")) {
> +            Cu.reportError("Search Engine installation blocked by the Enterprise Policy Manager.");

Are you sure you want to spam the error console each time a user visits a site offering open search plugins (eg. developer.mozilla.org or amazon.com)?
Attachment #8956565 - Flags: review?(florian) → review-
Comment on attachment 8956565 [details]
Bug 1428948 - Add policies to modify the available search engines

https://reviewboard.mozilla.org/r/225468/#review231674

> Looks a lot like this is forcing a sync initialization of the search service. Any reason why this can't wait until the search service is done initializing itself asynchronously?

I moved the call to the `onAllWindowsRestored` callback. I trust that this is late enough that it should be done initialing.

> You'll be in that failure case whenever we attempt to update engines. Is that ok?

Yes. Changing installed engines is not planned for Firefox 60

> Should deactivating a policy automatically remove the engines it had added? If so, should we test it here?

Deactivating the policy should not automatically remove added engines. If administrators want to do this, they will want to use the "remove search engine" policy (which is not scheduled for inclusion in Firefox 60.

> Why is this test expecting to (sometimes?) not have this link hidden immediately? Is this going to visibly flicker when users open preferences?

The code that hides the link runs at the same time as the code that sets the href attribute of the link. No flickering problems would be introduced by this patch unless they already existed due to that code.

> Would this still work with a relative url in the href attribute? It would look nicer.

It seems that it does not work. Not sure why but I don't see a compelling reason to delve into it.
(In reply to :Felipe Gomes (needinfo me!) from comment #0)
> There are requests for policies to do these 4 tasks related to search
> engines:
> 
> - add an engine
> - remove engines
> - choose the default one
> - block installation of new ones via the web api

Can you add a 5th task?

Would like to be able to perform a context-sensitive search on a clicked-on item on a web page. Would this sort of thing be possible using this technology? Thank you.



new:
 - add an engine
 - remove engines
 - choose the default one
 - block installation of new ones via the web api
 - allow context sensitive searching
> Would like to be able to perform a context-sensitive search on a clicked-on item on a web page. Would this sort of thing be possible using this technology? Thank you.

Firefox already provides this functionality using the default search or it can be done via a WebExtension. It's not in scope for this bug.
(In reply to Kirk Steuber [:bytesized] from comment #16)
> Comment on attachment 8956565 [details]
> Bug 1428948 - Add policies to modify the available search engines
> 
> https://reviewboard.mozilla.org/r/225468/#review231674
> 
> > Looks a lot like this is forcing a sync initialization of the search service. Any reason why this can't wait until the search service is done initializing itself asynchronously?
> 
> I moved the call to the `onAllWindowsRestored` callback. I trust that this
> is late enough that it should be done initialing.

Moving to 'onAllWindowsRestored' is good to avoid delaying first paint. It doesn't help with avoiding main thread I/O. You really need to wait for the end of the search service's async initialization. (Eventually we would like to remove the synchronous initialization code path of the search service; currently it's only deprecated.)

> > You'll be in that failure case whenever we attempt to update engines. Is that ok?
> 
> Yes. Changing installed engines is not planned for Firefox 60
> 
> > Should deactivating a policy automatically remove the engines it had added? If so, should we test it here?
> 
> Deactivating the policy should not automatically remove added engines. If
> administrators want to do this, they will want to use the "remove search
> engine" policy (which is not scheduled for inclusion in Firefox 60.

This means it'll be impossible for administrators to update engines (eg. to change an url). Expect to see complaints about it once someone makes a typo ;-).
onAllWindowsRestored was good, to wait for the search service async init, you can do:
Services.search.init(() => {
  ... your code
});

Your code using browser-search-service doesn't work, as some of these notifications will be fired during initialization. If you really wanted to use that notification, you would need to check its data parameter, and wait for it to be "init-complete".
Comment on attachment 8956565 [details]
Bug 1428948 - Add policies to modify the available search engines

https://reviewboard.mozilla.org/r/225468/#review232024

Looks good to me now, thanks!

::: browser/components/enterprisepolicies/Policies.jsm:274
(Diff revision 5)
>        setAndLockPref("signon.rememberSignons", param);
>      }
>    },
> +
> +  "SearchEngines": {
> +    onAllWindowsRestored (manager, param) {

nit: no space before '('.

::: browser/components/enterprisepolicies/Policies.jsm:289
(Diff revision 5)
> +                template:    newEngine.URLTemplate,
> +                iconURL:     newEngine.IconURL,
> +                alias:       newEngine.Alias,
> +                description: newEngine.Description,
> +                method:      newEngine.Method,
> +                suggestURL:  newEngine.SuggestURLTemplate

I wonder if we should provide an extensionID here (maybe just "policies") to be able to distinguish these engines in telemetry data.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_search_engine.js:34
(Diff revision 5)
> +  let engineListElement = document.getAnonymousElementByAttribute(oneOffsContainer,
> +                                                                  "anonid",
> +                                                                  "add-engines");
> +  if (shouldWork) {
> +    isnot(engineListElement.firstChild, null,
> +       "There should be search engines available to add");

nit: the indent is wrong here.

alternatively, instead of isnot(..., null), this could just be ok(engineListElement.firstChild);
Attachment #8956565 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] from comment #23)

> ::: browser/components/enterprisepolicies/Policies.jsm:289
> (Diff revision 5)
> > +                template:    newEngine.URLTemplate,
> > +                iconURL:     newEngine.IconURL,
> > +                alias:       newEngine.Alias,
> > +                description: newEngine.Description,
> > +                method:      newEngine.Method,
> > +                suggestURL:  newEngine.SuggestURLTemplate
> 
> I wonder if we should provide an extensionID here (maybe just "policies") to
> be able to distinguish these engines in telemetry data.

Mike, any opinion about this?
Flags: needinfo?(mozilla)
> Mike, any opinion about this?

Yes. That would definitely be helpful.
Flags: needinfo?(mozilla)
I'm not sure what an appropriate extension ID to provide here might be. Robert- Would you be able to suggest something?

I also know that some things that are controlled by extensions show a UI element indicating such in the about:preferences page. Is this something that exists/will exist for search engines? Will any of that misbehave by setting an extension id for a non-existent extension like this?
Flags: needinfo?(rhelmer)
(In reply to Kirk Steuber [:bytesized] from comment #26)
> I'm not sure what an appropriate extension ID to provide here might be.
> Robert- Would you be able to suggest something?
> 
> I also know that some things that are controlled by extensions show a UI
> element indicating such in the about:preferences page. Is this something
> that exists/will exist for search engines? Will any of that misbehave by
> setting an extension id for a non-existent extension like this?
Flags: needinfo?(rhelmer) → needinfo?(aswan)
This is a question about how the search service handles the extension ID property, which I don't really know anything about.  In particular, I have no idea if something will break if you put a string like "policies" that is not a valid extension id there.
I'm pretty sure the UI elements in about:preferences don't refer to that field, it is used only within the search service.
Flags: needinfo?(aswan)
The search service really only uses it for reporting right now, so I think something like "policy" is fine.
I think we actually want the thing we put there to not be a valid extension id, so that there can be no ambiguity when reading the resulting value on Telemetry data.
> I think we actually want the thing we put there to not be a valid extension id, so that there can be no ambiguity when reading the resulting value on Telemetry data.

Totally agree. Also an extension can't arbitrarily set it.

It could be a string like set-via-policy
Depends on: 1445134
Here's what to change in this bug due to bug bug 1445134:

- simply add a `memoryOnly: true` in the newEngineParameters
- unwrap that part from the runOncePerModification call (it should run on every startup)
- make this run on onBeforeAddons instead of onAllWindowsRestored


Note: a policy can use more than one of the callback timings..  So you can put the param.Add part on a onBeforeAddons function, while leaving the parts that handle param.Default and param.PreventInstalls inside the onAllWindowsRestored function.


e.g:

"SearchEngines": {
  onBeforeAddons(manager, param) {
    ...
  },

  onAllWindowsRestored(manager, param) {
    ...
  }
}
It looks like the in-memory search engines are not going to make it into beta in time for this.
No longer depends on: 1445134
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s dd42e24b79c1db37bb44546aa7e86b9d4ee02634 -d f01eb749b3f9: rebasing 454064:dd42e24b79c1 "Bug 1428948 - Add policies to modify the available search engines r=Felipe,florian" (tip)
merging browser/components/enterprisepolicies/Policies.jsm
merging browser/components/enterprisepolicies/schemas/policies-schema.json
merging browser/components/enterprisepolicies/tests/browser/browser.ini
merging browser/components/preferences/in-content/search.js
warning: conflicts while merging browser/components/enterprisepolicies/tests/browser/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ebad80a76266
Add policies to modify the available search engines r=Felipe,florian
https://hg.mozilla.org/mozilla-central/rev/ebad80a76266
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment on attachment 8956565 [details]
Bug 1428948 - Add policies to modify the available search engines

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policy Engine
[User impact if declined]: Policies to modify available search engines will not be possible
[Is this code covered by automated tests?]: Test is included in patch
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: To be verified by Abe
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal Risk
[Why is the change risky/not risky?]: Changes only affect enterprise policy users
[String changes made/needed]: None
Attachment #8956565 - Flags: approval-mozilla-beta?
Comment on attachment 8956565 [details]
Bug 1428948 - Add policies to modify the available search engines

enterprise policies for search, beta60+
Attachment #8956565 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1449681
We verified this on the latest nightly using JSON policy format and it is verified as fixed.
Using this policy, search engines can be added, set to default or (and) blocked from installation via a web API.

This will also be tested with adm format when available. 
  
Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
We retested this on beta builds with ADM and JSON policy formats and it is verified as fixed.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: