Closed Bug 1433136 Opened 6 years ago Closed 6 years ago

Implement GPO support on the Policy Engine

Categories

(Firefox :: Enterprise Policies, enhancement)

60 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: Felipe, Assigned: mkaply)

References

Details

Attachments

(3 files)

This bug is about implementing the GPO source for policies to feed into the policy engine.

It should read from the GPO entries in the registry and generate a JS object to be given to the EnterprisePoliciesManager object, which will later validate the parameters and call the policies
Blocks: 1433173
Attached patch First passSplinter Review
First pass at some GPO support. This includes the patch from bug 1432890.

I also attach a .reg file that enables it.
Attachment #8948480 - Flags: feedback?(felipc)
Comment on attachment 8948480 [details] [diff] [review]
First pass

This looks really good. I pushed the refactoring patch to inbound last night to make it easier to build on top of it.

Two main points:

- Hopefully we won't need to do a whitelist of policies or special code for each policy to convert the registry structure into our JSON.. We can read all that is in there, and the code in https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/browser/components/enterprisepolicies/EnterprisePolicies.js#111 will ignore the policies that are not spec'ed in the schema.

And right after to it, the validateAndParseParameters function can take care of converting data types. For example, if we modify this case here:

https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/browser/components/enterprisepolicies/PoliciesValidator.jsm#102

to also accept the numbers 0 and 1, and convert that to true or false in the returned value, we can easily support the DWORD -> boolean conversion.
Attachment #8948480 - Flags: feedback?(felipc) → feedback+
Depends on: 1436396
Comment on attachment 8952557 [details]
Bug 1433136 - Basic GPO Support.

https://reviewboard.mozilla.org/r/221794/#review227690


Code analysis found 5 defects in this patch:
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/EnterprisePolicies.js:422
(Diff revision 1)
> +                        .createInstance(Components.interfaces.nsIWindowsRegKey);
> +    try {
> +      wrk.open(rootKey,
> +               "SOFTWARE\\Policies\\Mozilla\\Firefox",
> +               wrk.ACCESS_READ);
> +      for (var i=0; i < wrk.valueCount; i++) {

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/components/enterprisepolicies/EnterprisePolicies.js:428
(Diff revision 1)
> +        var name = wrk.getValueName(i);
> +        var value = this.readRegistryValue(wrk, name);
> +        this._policies[name] = value;
> +      }
> +
> +      for (var i=0; i < wrk.childCount; i++) {

Error: 'i' is already defined. [eslint: no-redeclare]

::: browser/components/enterprisepolicies/EnterprisePolicies.js:428
(Diff revision 1)
> +        var name = wrk.getValueName(i);
> +        var value = this.readRegistryValue(wrk, name);
> +        this._policies[name] = value;
> +      }
> +
> +      for (var i=0; i < wrk.childCount; i++) {

Error: Infix operators must be spaced. [eslint: space-infix-ops]

::: browser/components/enterprisepolicies/EnterprisePolicies.js:429
(Diff revision 1)
> +        var value = this.readRegistryValue(wrk, name);
> +        this._policies[name] = value;
> +      }
> +
> +      for (var i=0; i < wrk.childCount; i++) {
> +        var name = wrk.getChildName(i);

Error: 'name' is already defined. [eslint: no-redeclare]

::: browser/components/enterprisepolicies/EnterprisePolicies.js:471
(Diff revision 1)
> +  readPermissionValues(wrk, permission, action) {
> +    if (!this._policies[permission]) {
> +      this._policies[permission] = {};
> +    }
> +    this._policies[permission][action] = [];
> +    for (var i=0; i < wrk.valueCount; i++) {

Error: Infix operators must be spaced. [eslint: space-infix-ops]
Comment on attachment 8952557 [details]
Bug 1433136 - Basic GPO Support.

https://reviewboard.mozilla.org/r/221794/#review227968

Very exciting! So from what we talked about, I believe we'll be able to not need to have special code to handle any of the policies. Let me document it here:

- If a regkey has child folders:
  - if the first folder name is 0:
     read it as an array of objects, where each folder is an object, and the key/value pairs inside that are the key/values of the object
  - otherwise, read it as a nested object

- If there are no child folders:
  - if the first key name is 0:
     read it as a normal array, ignoring the key names, and each value is an entry in the array
  - otherwise, read it as a normal key/value property


With this, I believe we can have almost any arbitrary format that we might need. Note that this will make the code able to handle nested objects just fine. But that doesn't mean we need to use infinite nesting, because representing them in the ADMX templates will be the complicated part.

If the reading goes wrong for any reason, that's fine.. The Policy Validator will fail, and that specific policy will be ignored. It just means that (probably) someoned edited the registry by hand and created the wrong structure.


Examples of cases:

> \+ \\Software\\Policies\\Mozilla\\Firefox
>   \+ Bookmarks
>     \+ 0           ->  /=======================================\
>     \+ 1               | Key      | Value                      |
>  |                     | title    | "Mozilla"                  |
>  |                     | URL      | "https://www.mozilla.com"  |
>  |                     \=======================================/
>  |
>   \+ Popups
>     \+ Allow     ->    /=======================================\
>                        | Key      | Value                      |
>                        | 0        | "https://www.example.com"  |
>                        | 1        | "https://www.mozilla.com"  |
>                        \=======================================/


The resulting JSON for this will be:

> policies: {
>   "Bookmarks":
>     // array of objects
>     [
>       { title: "Mozilla", URL: "https://www.mozilla.com" },    // Object 0
>       { title: "Foo", URL: "https://www.foo.com" }             // Object 1  (not seen above)
>     ],
>
>   "Popups":
>   {
>     // nested object
>     "Allow":
>       // normal array
>       [ "https://www.example.com", "https://www.mozilla.com" ]
>   }
> }

::: browser/components/enterprisepolicies/EnterprisePolicies.js:110
(Diff revision 2)
> +      if (gpoProvider.hasPolicies) {
> +        log.error(JSON.stringify(gpoProvider.policies));
> +        return gpoProvider;
> +      }
> +    }
> +    log.error("No GPO");

nit: remove this logging

::: browser/components/enterprisepolicies/EnterprisePolicies.js:382
(Diff revision 2)
> +  constructor() {
> +    this._policies = null;
> +    this._failed = false;
> +    let wrk = Components.classes["@mozilla.org/windows-registry-key;1"]
> +                        .createInstance(Components.interfaces.nsIWindowsRegKey);
> +    wrk.open(wrk.ROOT_KEY_LOCAL_MACHINE,

As agreed to, let's read ROOT_KEY_LOCAL_MACHINE afterwards

::: browser/components/enterprisepolicies/EnterprisePolicies.js:415
(Diff revision 2)
> +
> +  get failed() {
> +    return this._failed;
> +  }
> +
> +  _readData(rootKey) {

I was hoping that the `_readData` (and its supporting functions) could live in a separate .jsm, that will only be used if GPO.hasPolicies returned true. This will make most of this code not load for the normal case, and for non-Windows.

But if that's too much to do here we can do it in a follow-up. It's certainly the least important thing now

::: browser/components/enterprisepolicies/EnterprisePolicies.js:417
(Diff revision 2)
> +    return this._failed;
> +  }
> +
> +  _readData(rootKey) {
> +    let wrk = Components.classes["@mozilla.org/windows-registry-key;1"]
> +                        .createInstance(Components.interfaces.nsIWindowsRegKey);

nit: use Cc and Ci
Attachment #8952557 - Flags: review?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #9)
> I was hoping that the `_readData` (and its supporting functions) could live
> in a separate .jsm, that will only be used if GPO.hasPolicies returned true.
> This will make most of this code not load for the normal case, and for
> non-Windows.

One advantage of doing this now is that it would make it a lot simpler to write tests, since you could just import that directly into your test and get the resulting JSON out of it to compare to a hand-written one.
Comment on attachment 8952557 [details]
Bug 1433136 - Basic GPO Support.

https://reviewboard.mozilla.org/r/221794/#review228322

Looks great. Eventually we should add more documentation in GPOParser.jsm to explain in a high-level and in more details what it is doing, but I'm fine to land this as is now so that we can focus on start creating the administrative templates and test this GPO integration.

Only open question here is if we want to handle the merging now or in a follow-up.

::: browser/components/enterprisepolicies/EnterprisePolicies.js:107
(Diff revision 4)
>  
>    _chooseProvider() {
> -    // TODO: Bug 1433136 - Add GPO provider with higher precendence here
> +    if (AppConstants.platform == "win") {
> +      let gpoProvider = new GPOPoliciesProvider();
> +      if (gpoProvider.hasPolicies) {
> +        log.error(JSON.stringify(gpoProvider.policies));

nit: remove this log.error here and below

::: browser/components/enterprisepolicies/EnterprisePolicies.js:379
(Diff revision 4)
> +    this._policies = null;
> +    this._failed = false;
> +
> +    this._policies = null;
> +    this._failed = false;

nit: declared twice

but it looks like this provider probably can't get in a failed state, so I'd just remove the `_failed` property.

::: browser/components/enterprisepolicies/EnterprisePolicies.js:385
(Diff revision 4)
> +    this._failed = false;
> +
> +    this._policies = null;
> +    this._failed = false;
> +
> +    let wrk = Cc["@mozilla.org/windows-registry-key;1"].createInstance(Ci.nsIWindowsRegKey);

Can you add a comment here explaining why this one should to be read first?

::: browser/components/enterprisepolicies/EnterprisePolicies.js:418
(Diff revision 4)
> +  }
> +
> +  _readData(wrk) {
> +    let policies = GPOParser.readPolicies(wrk);
> +    if (this._policies) {
> +      // Combine policies in an intelligent way

thinking more about it, I believe finding a good algorithm to merge the objects recursively will probably be harder than if you pass the object to readPolicies and merge it as it's built.

If you want we can leave this to be handled in a follow-up

::: browser/components/enterprisepolicies/GPOParser.jsm:9
(Diff revision 4)
> +
> +"use strict";
> +
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const PREF_LOGLEVEL           = "browser.policies.loglevel";

nit: remove this extra whitespace before the =

::: browser/components/enterprisepolicies/moz.build:26
(Diff revision 4)
>      'EnterprisePolicies.manifest',
>      'EnterprisePoliciesContent.js',
>  ]
>  
>  EXTRA_JS_MODULES.policies += [
> +    'GPOParser.jsm',

This file should be shipped only if we're building on Windows.

I'd rename it to WindowsGPOParser.jsm
Attachment #8952557 - Flags: review?(felipc) → review+
One more change that will go in. In working with the GPO tool, arrays in the registry start at 0, not 1. I'll make that change as well.
All your changes have been adopted (including the combination by passing policies).

I've test this with computer and user policies and they merged beautifully.

Working on ADMx stuff now.
Awesome. Feel free to land this whenever you want!

(nit: just change the two `var` declarations that showed up in the last version, to use `let` instead)
Hi(In reply to Mike Kaply [:mkaply] from comment #16)
> All your changes have been adopted (including the combination by passing
> policies).
> 
> I've test this with computer and user policies and they merged beautifully.
> 
> Working on ADMx stuff now.

Hi Mike,
if you don't want to start from zero on ADMX, i can give you an ADMX file with quite some in there. We use it for several years now.
Regards, Nick
https://hg.mozilla.org/mozilla-central/rev/946a3f94fc70
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee: nobody → mozilla
(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> to also accept the numbers 0 and 1, and convert that to true or false in the
> returned value, we can easily support the DWORD -> boolean conversion.

Is this not implemented yet? I've got an error "Enterprise Policies:Invalid parameters specified for BlockAboutAddons.  EnterprisePolicies.js:135" with a registry entry:

[HKEY_CURRENT_USER\Software\Policies\Mozilla\Firefox]
"BlockAboutAddons"=dword:00000001
It depends on bug 1442418 which will go in today.

FYI, the templates are here:

https://github.com/mozilla/policy-templates
Is there a complete list of options available that will be supported at release of Firefox 60?

I especially need to following options:
- app.update.enabled
- app.update.auto
- app.update.mode
- app.update.service.enabled
- browser.rights.3.shown
- browser.startup.homepage_override.mstone
- browser.shell.checkDefaultBrowser
- browser.startup.homepage
- network.proxy.ftp
- network.proxy.ftp_port
- network.proxy.http
- network.proxy.http_port
- network.proxy.socks
- network.proxy.socks_port
- network.proxy.ssl
- network.proxy.ssl_port
- network.proxy.type
- network.proxy.share_proxy_settings
- network.proxy.no_proxies_on
- signon.autologin.proxy
- datareporting.healthreport.service.enabled
- datareporting.policy.dataSubmissionEnabled
- toolkit.crashreporter.enabled

And also:
Components.classes["@mozilla.org/toolkit/crash-reporter;1"].getService(Components.interfaces.nsICrashReporter).submitReports = false;
Most of these are covered by the policies.

Any that are not, you will be able to use Autoconfig. 

I'll investigate signon.autologin.proxy which is the only interesting one on this list.

Also note that the crash reporter stuff really isn't working anyway - https://bugzilla.mozilla.org/show_bug.cgi?id=1427104
Thank you, Mike! This was one of the main blockers of Firefox adoption in the enterprise. You will make a lot of people happy with this, and help Firefox adoption.

This is fantastic news.
*applause*
*applause* *applause*
Hello,

is there also the possibility to roll out the value browser.sessionstore.interval in the group policies?

So to say extended settings.

Best regards
(In reply to Tim from comment #28)
> Hello,
> 
> is there also the possibility to roll out the value
> browser.sessionstore.interval in the group policies?
> 
> So to say extended settings.
> 
> Best regards

We're looking at adding support for a number of prefs.
You need to log in before you can comment on or make changes to this bug.