Closed Bug 1429169 Opened 3 years ago Closed 3 years ago

Policies: Change default global settings for Flash / Popups / Cookies

Categories

(Firefox :: Enterprise Policies, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox60 + verified
firefox61 --- verified

People

(Reporter: Felipe, Assigned: bytesized)

References

Details

Attachments

(6 files)

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)
Assignee: nobody → ksteuber
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
Attachment #8961078 - Flags: review?(jmathies)
Attachment #8961079 - Flags: review?(jaws)
Attachment #8961079 - Flags: review?(felipc)
Attachment #8961080 - Flags: review?(jaws)
Attachment #8961080 - Flags: review?(felipc)
Attachment #8961081 - Flags: review?(jaws)
Attachment #8961081 - Flags: review?(felipc)
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.
Attachment #8961078 - Flags: review?(jmathies) → 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
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.
Flags: needinfo?(mozilla)
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?
Attachment #8961080 - Flags: review?(felipc)
Comment on attachment 8961081 [details]
Bug 1429169 - Add enterprise policy to disable the popup blocker

https://reviewboard.mozilla.org/r/229838/#review235646
Attachment #8961081 - Flags: review?(felipc) → review+
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.
Flags: needinfo?(mozilla)
I didn't want to name it disabled since Popups.Disabled wouldn't really be correct. Maybe we could rename it to PopupBlocking.Disabled?
> 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
"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?
> 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.
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)
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.
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)
Attachment #8961079 - Flags: review?(felipc)
Attachment #8961081 - Flags: review+ → review?(felipc)
Comment on attachment 8961079 [details]
Bug 1429169 - Add enterprise policy to change the global cookie settings

https://reviewboard.mozilla.org/r/229834/#review237210
Attachment #8961079 - Flags: review?(felipc) → review+
Comment on attachment 8961080 [details]
Bug 1429169 - Add enterprise policy for disabling the Flash plugin

https://reviewboard.mozilla.org/r/229836/#review237212
Attachment #8961080 - Flags: review?(felipc) → 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
Attachment #8961081 - Flags: review?(felipc) → 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?
Attachment #8961079 - Flags: review?(jaws) → 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.
Attachment #8961080 - Flags: review?(jaws) → 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.
Attachment #8961081 - Flags: review?(jaws) → review+
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?
Flags: needinfo?(felipc)
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
Flags: needinfo?(felipc)
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
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
Flags: needinfo?(ksteuber)
Hmm. This issue seems to reproduce consistently on Try, but I cannot seem to reproduce it locally. I'm not sure why yet.
Flags: needinfo?(ksteuber)
Ah, it reproduces locally if I run the whole directory of tests. Investigating...
Attachment #8964043 - Flags: review?(felipc)
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
Attachment #8964043 - Flags: review?(felipc) → review+
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
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
Attachment #8964314 - Flags: review+
Attachment #8964314 - Flags: approval-mozilla-beta?
Comment on attachment 8964314 [details]
Rollup patch for beta, r=jimm,jaws,felipe

Policy Engine fix needed for ESR60. Approved for 60.0b9.
Attachment #8964314 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1451024
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
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.