1453350 - Search Engine Policies should be on modify, not run once

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect, P1)

Description

[Mike Kaply [:mkaply]]

I'm thinking the search engine policies (at least default), should be based on modification, not just run once.

Also, I'm wondering about the ability to remove an engine that was added via the policy.

Comment 1

[Mike Kaply [:mkaply]]

felipe: what is your opinion on this?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

I actually thought this was already the case. But yeah, that sounds better

Comment 3

[Kanika Saini]

:felipe Can you please guide me about what we wish to achieve here?
I understood 1) Replace 'runOnce' by 'runOncePerModification' everywhere in the policy. 2) Add "Remove" attribute (an object similar to "Add" attribute) to the param of the policy. 
Thank you! :)

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Hi Kanika, for this bug, we just want to do the runOnce -> runOncePerModification change.

The Remove feature is something that we want, but it should probably be done in a separate bug. If you're interested in that, feel free to create a new bug about that and assign it to you :)

Comment 5

[Kanika Saini]

Attachment: Bug 1453350 - Search Engines Policy runs on modification

Review commit: https://reviewboard.mozilla.org/r/248842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248842/

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8982998 [details]
Bug 1453350 - Search Engines Policy runs on modification

https://reviewboard.mozilla.org/r/248842/#review255670

Comment 7

[:Felipe Gomes (needinfo for replies!)]

So this is the change expected, but the test browser_policy_search_engine.js fails with it. It just needs some adjustments (I didn't look into it to see why)

Comment 8

[Kanika Saini]

FAIL Specified search engine should be the default - Got Google, expected MozSearch
FAIL Specified search engine should be the default - Got Bing, expected MozSearch

These are the errors that we're getting. I cannot understand that in the test we're changing the policy by setting default engine as Mozsearch, so ideally setDefaultSearchEngine should be called and default engine should be set up successfully.

Can't we see the errors thrown by Policies.jsm to understand more about why it is failing?

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Ah, I do see the error when running the test. It's just hard to spot because there's some much info on the output.

Here's what I got, when I ran `mach test browser_policy_search_engine.js`:

0:06.73 INFO Console message: [JavaScript Error: "TypeError: callback is not a function" {file: "resource:///modules/policies/Policies.jsm" line: 857}]


I missed this in the review.. The prob is that the runOncePerModification requires one more parameter than the runOnce function..  You need to define what's the value to be compared  (and then the function call is the 3rd parameter)

Comment 10

[Kanika Saini]

Comment on attachment 8982998 [details]
Bug 1453350 - Search Engines Policy runs on modification

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248842/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d261d4228bd8
Search Engines Policy runs on modification r=Felipe

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d261d4228bd8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something we need for ESR60?

Comment 14

[Mike Kaply [:mkaply]]

We're not doing policy updates on ESR60 for this go cycle.

Comment 15

[Mike Kaply [:mkaply]]

Comment on attachment 8982998 [details]
Bug 1453350 - Search Engines Policy runs on modification

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Policy only change
User impact if declined: 
Fix Landed on Version: Firefox 62
Risk to taking this patch (and alternatives if risky): Low Policy Only.
String or UUID changes made by this patch:  None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8982998 [details]
Bug 1453350 - Search Engines Policy runs on modification

Policy engine fix needed on ESR60. Approved for ESR 60.2.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/97641a1614d4

Comment 18

[Emil Ghitta, QA [:emilghitta]]

This bug was covered by the overall testing efforts invested in the New Enterprise Policies feature.

Marking this as verified fixed using Firefox 62.0b16 (BuildId:20180809104529) and the ESR try build (provided in comment 17) on Windows 10 64bit and macOS 10.13.4