:Felipe Gomes (needinfo for replies!) Reporter
	Description • 7 years ago

Three policies:

  - Change the default cookie setting (allow/disallow/thirdparty)
  - Change default Flash setting (is Flash allowed by default)
  - Change default popup settings (are popups allowed by default)

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 1 • 7 years ago

What I was thinking for this one is to re-use the existing policies for it (Cookies / FlashPlugin / Popups) and add a new field to them: "Default".

Note that the cookies case will actually require two fields: one for the default value, and one for the third-party option. And if we want to be really complete, maybe one to expire on the end of the session too).  Refer to the Privacy -> Cookies and Site data section in about:preferences

	Jim Mathies [:jimm]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8961078 [details]
Bug 1429169 - Facilitate testing of cookie-related enterprise policy by always firing the cookie-db-read event

https://reviewboard.mozilla.org/r/229832/#review235620

lgtm. I took a look at this event as well, doesn't appear to be in use in production.

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8961079 [details]
Bug 1429169 - Add enterprise policy to change the global cookie settings

https://reviewboard.mozilla.org/r/229834/#review235636

Make sure that the test is resilient to running as `mach test --repeat=2 browser_policy_cookie_settings.js`, otherwise it'll fail the test-verify job.

(I'm just saying it because it's hard to tell from just looking at it)

::: browser/components/enterprisepolicies/Policies.jsm:138
(Diff revision 1)
> +        }
> +
> +        if (param.Locked) {
> +          setAndLockPref("network.cookie.cookieBehavior", newCookieBehavior);
> +        } else {
> +          runOncePerModification("cookieBehavior", newCookieBehavior.toString(), () => {

Is there a reason that you chose to use runOncePerModification instead of the new pattern to use setDefaultPref in the unlocked case?

::: browser/components/enterprisepolicies/tests/browser/browser_policy_cookie_settings.js:15
(Diff revision 1)
> +const TOUCHED_PREFS = {
> +  "network.cookie.cookieBehavior": "Int",
> +  "network.cookie.lifetimePolicy": "Int",
> +  "browser.policies.runOncePerModification.cookieBehavior": "String",
> +  "browser.policies.runOncePerModification.cookieLifetimePolicy": "String",
> +};
> +let ORIGINAL_PREF_VALUE = {};
> +add_task(function read_original_pref_values() {
> +  for (let pref in TOUCHED_PREFS) {
> +    let prefType = TOUCHED_PREFS[pref];
> +    ORIGINAL_PREF_VALUE[pref] =
> +      Services.prefs[`get${prefType}Pref`](pref, undefined);
> +  }
> +});
> +function restore_prefs() {
> +  let defaults = Services.prefs.getDefaultBranch("");
> +  for (let pref in TOUCHED_PREFS) {
> +    try {
> +      Services.prefs.unlockPref(pref);
> +    } catch (ex) {
> +      // This should only throw if this pref doesn't exist. If it doesn't, there
> +      // is nothing to reset for this pref.
> +      continue;
> +    }
> +    Services.prefs.clearUserPref(pref);
> +    let originalValue = ORIGINAL_PREF_VALUE[pref];
> +    let prefType = TOUCHED_PREFS[pref];
> +    if (originalValue !== undefined) {
> +      defaults[`set${prefType}Pref`](pref, originalValue);
> +    }
> +  }
> +}
> +registerCleanupFunction(restore_prefs);

With bug 1446508 (which just landed) [1], you don't need to do most of this: reading the original values, unlocking, and reseting the original value.

All that you need to do is call clearUserPref on the prefs that got changed in the unlocked case. (But if you change it to use setDefaultPref as mentioned above, that part won't be necessary either).

Oh, and with [2], there's no need to clear the `.runOncePerModification` prefs either, even if you continue to use it.

(Note: the prefs changed through setAndLockPref and setDefaultPref are restored to their original values on every call to setupPolicyEngineWithJSON. However, the runOncePerModification prefs are only cleared at the end of test)

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/a92b420e1f7f
[2] https://hg.mozilla.org/integration/mozilla-inbound/rev/a4642c1e1d23

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 8 • 7 years ago

Mike, can you look at the schemas here and see if they will be workable on GPO?

We're adding options in the existing "Popups", "Cookies" and "FlashPlugin" policies to control their default values, and optionally lock them.

Since they are a bit different than most (as they have their own sub-folder on GPO), I'd like that you take a look too.

I suppose we can add a direct entry inside the folder to configure the "Locked" key and the other key.

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8961080 [details]
Bug 1429169 - Add enterprise policy for disabling the Flash plugin

https://reviewboard.mozilla.org/r/229836/#review235644

::: browser/components/enterprisepolicies/schemas/policies-schema.json:265
(Diff revision 1)
>            }
> +        },
> +
> +        "Disabled": {
> +          "type": "boolean"
> +        },

Since we're adding the option to lock it, I'd prefer if we also provided a option to lock the "click-to-play" setting.

So instead of a boolean Disabled, it could be a State string enum with "always-run", "click-to-play", "always-block".

BTW, I don't think that the dropdown in about:addons is correctly disabled if the pref is locked. Can you check it?

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8961081 [details]
Bug 1429169 - Add enterprise policy to disable the popup blocker

https://reviewboard.mozilla.org/r/229838/#review235646

	Mike Kaply [:mkaply]
	Comment 11 • 7 years ago

Yes, this will work fine. We already have Popups as a sub category in the registry with Allow as a child. So we can put these new values as the same level as Allow.

My only concern in all of this is that I feel like we haven't been totally consistent on naming everything.

In this case, we're using Blocked for one and Disabled for the other two. I honestly don't know what the right answer is.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 12 • 7 years ago

I didn't want to name it disabled since Popups.Disabled wouldn't really be correct. Maybe we could rename it to PopupBlocking.Disabled?

	Mike Kaply [:mkaply]
	Comment 13 • 7 years ago

> I didn't want to name it disabled since Popups.Disabled wouldn't really be correct. Maybe we could rename it to PopupBlocking.Disabled?

Good point. Honestly the pref is named horrible bad as well :)

What about just naming it either Value or Default and setting it to true/false?

I'm trying to look at chrome for some guidance, but they do things completely different:

DefaultPopupsSetting	Default popups setting
If this policy is left not set, 'BlockPopups' will be used and the user will be able to change it.

    1 = Allow all sites to show pop-ups
    2 = Do not allow any site to show popups

and then PopupsAllowedForUrls and PopupsBlockedForUrls

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 14 • 7 years ago

"Default" sounds like a good name to me

And we don't necessarily need to have it a boolean. It could be a string enum to make it more clear. Can we have string enums (that will maybe be presented as a dropdown) where it's possible to not set a value?

	Mike Kaply [:mkaply]
	Comment 15 • 7 years ago

> Can we have string enums (that will maybe be presented as a dropdown) where it's possible to not set a value?

Not in GPO. all dropdowns have to have a value.

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 16 • 7 years ago

But if it's a new entry (say, "Default") as a child of the Popups option (so, sibling of Allow), can't the Default simply be not set? (making it optional)

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 17 • 7 years ago

So, trying to homogenize it on "Default", here's my proposal:

Cookies: {
  Default: boolean, // true  (cookies are allowed),
                    // false (cookies are not allowed),
                    // undefined (no change, i.e., cookies are allowed)
  Locked: boolean,
  ...,
}

I agree that the best course of action is to rename the Popups policy to PopupBlocking

PopupBlocking: {
  Default: boolean,  // true (blocking is enabled),
                     // false (blocking is disabled),
                     // undefined (no change, i.e., blocking is enabled),
  Locked: boolean,
  ...
}

FlashPlugin: {
  Default: boolean,  // true (flash is always allow),
                     // false (flash is always block),
                     // undefined (no change, i.e., flash is click to play)
  Locked: boolean,
  ...
}

That has the extra consistency in that all the "true" values are positively enforcing the feature

And also, if someone specifies "Locked: true" for flash, but omits the Default, it means we'll lock it into click-to-play, which is also desirable.

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8961079 [details]
Bug 1429169 - Add enterprise policy to change the global cookie settings

https://reviewboard.mozilla.org/r/229834/#review235870

(clearing the flag that I forgot)

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8961079 [details]
Bug 1429169 - Add enterprise policy to change the global cookie settings

https://reviewboard.mozilla.org/r/229834/#review237210

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8961080 [details]
Bug 1429169 - Add enterprise policy for disabling the Flash plugin

https://reviewboard.mozilla.org/r/229836/#review237212

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 25 • 7 years ago
mozreview-review

Comment on attachment 8961081 [details]
Bug 1429169 - Add enterprise policy to disable the popup blocker

https://reviewboard.mozilla.org/r/229838/#review237214

We should file a separate issue on Github to mention that the name of this policy changed

	Jared Wein [:jaws] (please needinfo? me)
	Comment 26 • 7 years ago
mozreview-review

Comment on attachment 8961079 [details]
Bug 1429169 - Add enterprise policy to change the global cookie settings

https://reviewboard.mozilla.org/r/229834/#review238016

::: browser/components/enterprisepolicies/Policies.jsm:124
(Diff revision 2)
>        }
> +
> +      if (param.Default !== undefined ||
> +          param.AcceptThirdParty !== undefined ||
> +          param.Locked) {
> +        let newCookieBehavior = 0;

Can you create an enum for these integer values?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 27 • 7 years ago
mozreview-review

Comment on attachment 8961080 [details]
Bug 1429169 - Add enterprise policy for disabling the Flash plugin

https://reviewboard.mozilla.org/r/229836/#review238022

::: browser/components/enterprisepolicies/Policies.jsm:325
(Diff revision 2)
>        addAllowDenyPermissions("plugin:flash", param.Allow, param.Block);
> +
> +      let flashPrefVal;
> +      if (param.Default === undefined) {
> +        // Ask to Activate
> +        flashPrefVal = 1;

Please define an enum for these integer values too so the comments won't be as necessary.

::: browser/components/enterprisepolicies/Policies.jsm:332
(Diff revision 2)
> +        // Always Activate Flash
> +        flashPrefVal = 2;
> +      } else {
> +        // Never Activate Flash
> +        flashPrefVal = 0;
> +        setDefaultPref("plugin.state.flash", 0);

Why are you calling setDefaultPref() here and then also calling it below at line 337 (if not locked)? Looks like this line can be safely deleted.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_flash_plugin.js:17
(Diff revision 2)
> +  // eslint-disable-next-line no-shadow
> +  await ContentTask.spawn(tab.linkedBrowser, {expectedLabelText, locked}, async function({expectedLabelText, locked}) {

It's pretty easy to rename a variable so we don't have to disable the no-shadow rule. Right now it's being used to disable it for `expectedLabelText` which is pretty innocent, but once it's disabled in the future we might accidentally shadow a more useful variable name such as `tab` which could get more confusing.

You can rename `expectedLabelText` to `aExpectedLabelText` at line 14 and then continue to use `expectedLabelText` here.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8961081 [details]
Bug 1429169 - Add enterprise policy to disable the popup blocker

https://reviewboard.mozilla.org/r/229838/#review238024

::: browser/components/enterprisepolicies/schemas/policies-schema.json:319
(Diff revision 2)
>        "type": "boolean"
>      },
>  
> -    "Popups": {
> +    "PopupBlocking": {
>        "description": "Allow or deny popup usage.",
>        "first_available": "60.0",

Now that you're renaming it from Popups to PopupBlocking, doesn't that change when it is first_available (should be 61.0)? What is your plan to support configurations that are already using "Popups"?

::: browser/components/enterprisepolicies/tests/browser/browser_policy_disable_popup_blocker.js:8
(Diff revision 2)
> +registerCleanupFunction(restore_prefs);
> +
> +add_task(async function setup() {
> +  // It seems that this pref is given a special testing value for some reason.
> +  // Unset that value for this test.
> +  Services.prefs.clearUserPref("dom.disable_open_during_load");

This won't properly set the pref back to its initial user pref value. You should be caching the initial value before calling clearUserPref and the cleanupFunction should be setting the pref to the initial pref value instead of calling clearUserPref.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 29 • 7 years ago
mozreview-review-reply

Comment on attachment 8961081 [details]
Bug 1429169 - Add enterprise policy to disable the popup blocker

https://reviewboard.mozilla.org/r/229838/#review238024

> Now that you're renaming it from Popups to PopupBlocking, doesn't that change when it is first_available (should be 61.0)? What is your plan to support configurations that are already using "Popups"?

It will be available in 60.0, as this patch is expected to be uplifted to 60 with the other remaining enterprise policy patches. I believe that we are not planning to worry to much about configurations already using previous enterprise policy mechanisms because the features are still in beta and we have not started advertising their specifics yet, so there should not really be anyone using them at this point other than to test the functionality. Felipe, can you confirm?

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 34 • 7 years ago

Yeah, there aren't any release users using this yet, so there's no need to plan a transition. We want to uplift this one to 60 as it's part of the MVP

	Pulsebot
	Comment 38 • 7 years ago

Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd305f45f746
Facilitate testing of cookie-related enterprise policy by always firing the cookie-db-read event r=jimm
https://hg.mozilla.org/integration/autoland/rev/2f744fd3d77c
Add enterprise policy to change the global cookie settings r=Felipe,jaws
https://hg.mozilla.org/integration/autoland/rev/437f677d3808
Add enterprise policy for disabling the Flash plugin r=Felipe,jaws
https://hg.mozilla.org/integration/autoland/rev/a49df97d2ad9
Add enterprise policy to disable the popup blocker r=Felipe,jaws

	Andreea Pavel [:apavel]
	Comment 39 • 7 years ago

Backed out for failing browser chrome at browser/components/enterprisepolicies/tests/browser/browser_policy_cookie_settings.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a49df97d2ad98b4bfd97e60000b2b2cc1c1663cf

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=171150267&repo=autoland&lineNumber=2858

Backout: https://hg.mozilla.org/integration/autoland/rev/deee0d19a09095d3c5b8f833ae81c16f3870be0b

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 40 • 7 years ago

Hmm. This issue seems to reproduce consistently on Try, but I cannot seem to reproduce it locally. I'm not sure why yet.

	Robin Steuber (they/them) [:bytesized] Assignee
	Comment 41 • 7 years ago

Ah, it reproduces locally if I run the whole directory of tests. Investigating...

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 47 • 7 years ago
mozreview-review

Comment on attachment 8964043 [details]
Bug 1429169 - Fix test to prevent it from interfering with the new test added

https://reviewboard.mozilla.org/r/232844/#review238300

	Pulsebot
	Comment 48 • 7 years ago

Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa53e62227a9
Facilitate testing of cookie-related enterprise policy by always firing the cookie-db-read event r=jimm
https://hg.mozilla.org/integration/autoland/rev/13b2fe28f152
Add enterprise policy to change the global cookie settings r=Felipe,jaws
https://hg.mozilla.org/integration/autoland/rev/c6ed8787d259
Add enterprise policy for disabling the Flash plugin r=Felipe,jaws
https://hg.mozilla.org/integration/autoland/rev/9fcac6460feb
Add enterprise policy to disable the popup blocker r=Felipe,jaws
https://hg.mozilla.org/integration/autoland/rev/53f5d39a15d6
Fix test to prevent it from interfering with the new test added r=Felipe

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 49 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fa53e62227a9
https://hg.mozilla.org/mozilla-central/rev/13b2fe28f152
https://hg.mozilla.org/mozilla-central/rev/c6ed8787d259
https://hg.mozilla.org/mozilla-central/rev/9fcac6460feb
https://hg.mozilla.org/mozilla-central/rev/53f5d39a15d6

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 50 • 7 years ago

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policies
[User impact if declined]: Policies to set the default settings for Flash, Cookies and Popups
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: QA will handle it
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: mostly pref changes contained to these policies
[String changes made/needed]: none

	Ryan VanderMeulen [:RyanVM]
	Comment 51 • 7 years ago

Comment on attachment 8964314 [details]
Rollup patch for beta, r=jimm,jaws,felipe

Policy Engine fix needed for ESR60. Approved for 60.0b9.

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/3002e41cb4a3

	Abe - QA (:Abe_LV)
	Comment 53 • 7 years ago

We tested this on latest nightly using JSON policy formats and it is verified as fixed.
Using this policy, the default settings of cookies, flash, and popups can be set. 

This will also be tested with adm when available.
Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760

	Abe - QA (:Abe_LV)
	Comment 54 • 7 years ago

We retested this on beta builds[FX60] with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760