Closed Bug 1419102 Opened 7 years ago Closed 6 years ago

Prototype an enterprise policy manager to allow rules set on enterprise deployments

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(8 files, 11 obsolete files)

Bug 1419102 - Basis for the enterprise policy engine.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1419102 - Support passing down the information to the content process.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1419102 - Add hot-restart of the engine to support test, and create test infrastructure.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1419102 - Add param validation and parsing.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1419102 - Add proper logging.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1419102 - Move nsIEnterprisePolicies to toolkit so that platform code can include it and attempt to get the service.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1419102 - Use Cu.readUTF8File which remembers the file access and reads ahead the file in a background thread on subsequent startups.
59 bytes, text/x-review-board-request
mossop
: review+
Details
Bug 1419102 - Make initialization of the Policies manager its own step with a well-defined timing, running before the add-ons manager initializes.
59 bytes, text/x-review-board-request
mossop
: review+
Details 
The enterprise policies manager should be a module that takes a definition file (first in a JSON format, maybe other formats in the future) and applies some configuration rules to Firefox.  There will be different types of rules, such as:
- one-time settings, like adding/removing bookmarks on first run, customizing the toolbar layout, etc.
- changing defaults: setting the default homepage, search engines, default plugin or popup settings
- domain-based rules: whitelisting or blacklisting URLs to be accessed, adding some particular websites to the list of sites authorized to run plugins, display popups, etc.
- declarations of features to restrict, e.g. disable private browsing, disable access to about:config, disable installation of add-ons, etc.

These settings will be chosen by a system administrator in an enterprise context, and they are deployed by repackaging Firefox with the configuration file built-in.


The viability of this functionality itself, as well as the list of supported policies, is still being discussed. But a bug on file helps track things and prototype.
Alright, this is a good first draft for the policy engine. It implements the following:
  - a json schema describing the configuration file format
  - enforcement of only whitelisted policies with type validation
  - enforcement of documentation by requiring that any policy be defined in the schema
  - a folder structure that I'm happy with, with a place for the component, the helper functions, the Policies implementation and the schema
  - basic support for validating the types of the arguments of each policy
  - basic support for run-once policies (like the ones that need to add bookmarks, customize toolbars, etc), so that each policy doesn't need to manually keep track if it has run or not

I _might_ have to shuffle things around a little bit more as I'm starting to look into how to test all this, but hopefully it won't be much. I believe that we can seriously start implementing policies on top of this now.
Why is this Mozilla Confidential?
Flags: needinfo?(felipc)
Priority: -- → P3
> Why is this Mozilla Confidential?

1. Because we are going to have comms around announcing this (should be end of next week).
2. We didn't have full executive approval at the time so we didn't want to get everyone's hope up :)
Flags: needinfo?(felipc)
Attached patch Enterprise Policies engine, v2 (obsolete) — — Splinter Review 
The engine is approaching a point where I can start getting feedback/review. This version of the patch contains:

- support for propagating policies to the content process
- support for tests

So feature-wise it's almost complete and can be used to build other policies on top of it.

What's left to do:
- a lot of clean-up
- re-introduce the parameters validation (it was in the last patch but I removed it from this one since it's not crucial and it was getting in the way of making more progress)
- introduce a stub implementation in toolkit so that it doesn't break other products other than Firefox

Soon I'll split up the patch is smaller chunks to make review/feedback more approachable.
Comment on attachment 8932553 [details] [diff] [review]
First version, base policy engine

(obsoleting the old version)
Attachment #8932553 - Attachment is obsolete: true
Attached patch Enterprise Policies engine, v3 (obsolete) — — Splinter Review 
This version contains the parameters validation and parsing. With that, the policies implementation can be sure that they will be receiving the right parameter types.. otherwise they won't be called.

This also automatically converts a parameter specified as "URL" from the string in the json to an nsIURI object.

It supports simple types, arrays and objects, so I think it's pretty much complete now. It should allow us to implement URL-related policies like permissions, bookmarks, etc.
Attachment #8936973 - Attachment is obsolete: true
Attached patch Enterprise Policies engine, v3 (obsolete) — — Splinter Review 
(minor clean-up, and added an example usage of the array type)
Attachment #8937092 - Attachment is obsolete: true

        Attachment #8937104 -
        Flags: feedback?(dtownsend)

        Attachment #8937105 -
        Attachment is patch: true

    
    

    
  


  

        Attachment #8937108 -
        Flags: feedback?(dtownsend)

        Attachment #8937108 -
        Attachment is patch: true

    
    

    
  
Comment on attachment 8937104 [details] [diff] [review]
Part 1 - Basis for the enterprise policy engine

Review of attachment 8937104 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/enterprisepolicies/EnterprisePolicies.js
@@ +80,5 @@
> +    configFile.append(POLICIES_FILENAME);
> +
> +    let prefType = Services.prefs.getPrefType(PREF_ALTERNATE_PATH);
> +
> +    if ((prefType != Services.prefs.PREF_INVALID) && !configFile.exists()) {

I would check that it is a string pref specifically so when you call getStringPref below you don't see an exception in other cases.

@@ +95,5 @@
> +
> +    // TODO: This pattern is common in our code, but apparently not allowed
> +    // here? (is this running in strict mode?)
> +    // delete this._configurationFile;
> +    // this._configurationFile = configFile;

Given that you retrieve the value of this property in the constructor I don't think there is anything to gain by using a lazy getter here. Just calculate its value (probably in an external function) and set it from the constructor.

@@ +111,5 @@
> +      return;
> +    }
> +
> +    this.status = Ci.nsIEnterprisePolicies.ACTIVE;
> +    this._activatePolicies();

I think the ordering here is going to lead to problems. What happens is:

1. We do a synchronous file existence check.
2. We check this._file.failed which will be false since we haven't attempted to read the file yet.
3. We call _activatePolicies which attempts to synchronously load the data.

You would be better off synchronously loading the data straight off (it will fail fast if the file doesn't exist) which will then set the exists and failed properties accordingly.

@@ +125,5 @@
> +      modifiedSinceLastRun = true;
> +    }
> +    Services.prefs.setIntPref(PREF_LAST_MODIFIED_TIME, lastModifiedSeconds);
> +
> +    let schema = Cu.import("resource:///modules/policies/schema.jsm").schema;

Need to pass a second argument of {} to the import call to avoid importing the schema into the global scope.

You can also slightly simplify this line with:
    let { schema } = Cu.import(..., {});

@@ +180,5 @@
> +
> +  _runPoliciesCallbacks(timing) {
> +    let callbacks = this._callbacks[timing];
> +    while (callbacks.length > 0) {
> +      let callback = callbacks.shift();

This mutates the _callbacks array. Not a problem for the initial three but might be for IdleCallback.

@@ +183,5 @@
> +    while (callbacks.length > 0) {
> +      let callback = callbacks.shift();
> +      try {
> +        callback();
> +      } catch (ex) {}

You should probably log this exception somewhere.

@@ +222,5 @@
> +  },
> +
> +  get status() {
> +    return this._status;
> +  },

This getter and setter doesn't seem useful.

@@ +224,5 @@
> +  get status() {
> +    return this._status;
> +  },
> +
> +  isAllowed: function BG_sanitize(feature) {

I think this should throw if this.status is UNINITIALIZED

::: browser/components/enterprisepolicies/EnterprisePolicies.manifest
@@ +1,3 @@
> +component {ea4e1414-779b-458b-9d1f-d18e8efbc145} EnterprisePolicies.js process=main
> +contract @mozilla.org/browser/enterprisepolicies;1 {ea4e1414-779b-458b-9d1f-d18e8efbc145} process=main
> +category app-startup EnterprisePoliciesManager service,@mozilla.org/browser/enterprisepolicies;1 application={ec8030f7-c20a-464f-9b0e-13a3a9e97384} process=main

I don't think you should need the application flag here.

::: browser/components/enterprisepolicies/Policies.jsm
@@ +19,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["Policies", "PoliciesValidator"];
> +
> +this.PoliciesValidator = {
> +  validateAndParseParameters() {

This needs some description. It seems to be validation by returning true or false but what about the parsing bit, where is the result of that parsing returned?

::: browser/components/enterprisepolicies/nsIEnterprisePolicies.idl
@@ +6,5 @@
> +
> +[scriptable, uuid(6a568972-cc91-4bf5-963e-3768f3319b8a)]
> +interface nsIEnterprisePolicies : nsISupports
> +{
> +  const unsigned short UNITIALIZED = 0;

UNINITIALIZED :)

        Attachment #8937105 -
        Flags: feedback?(dtownsend) → feedback+

    
    

    
  
Comment on attachment 8937107 [details] [diff] [review]
Part 3 - Add hot-restart of the engine to support test, and create test infrastructure

Review of attachment 8937107 [details] [diff] [review]:
-----------------------------------------------------------------

Add a check for Cu.isInAutomation in here somewhere smart please.

::: browser/components/enterprisepolicies/EnterprisePolicies.js
@@ +198,5 @@
> +      return;
> +    }
> +    LOG("Restarting");
> +
> +    DisallowedFeatures = {};

You need to set this in the initialProcessData too.

@@ +220,5 @@
> +
> +          Services.tm.idleDispatchToMainThread(() => {
> +            Services.obs.notifyObservers(null,
> +                                         "EnterprisePolicies:Restarted",
> +                                         null);

Looks like this will cause a permanent loop. Did you mean to dispatch this to the child processes?

        Attachment #8937107 -
        Flags: feedback?(dtownsend) → feedback+

    
    

    
  
Comment on attachment 8937108 [details] [diff] [review]
Part 4 - Add param validation and parsing

Review of attachment 8937108 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/enterprisepolicies/Policies.jsm
@@ +160,5 @@
> +      try {
> +        if (param.indexOf("://")) {
> +          // Let's provide a shortcut so that people don't have
> +          // to write http:// on the URLs
> +          param = "http://" + param;

Why should insecure be the default?

        Attachment #8937108 -
        Flags: feedback?(dtownsend) → feedback+
Group: mozilla-employee-confidential

    
  
Depends on: 1426362
Blocks: 792816
Component: General → Enterprise Policies
Blocks: 1426362
No longer depends on: 1426362

    
    

    
  
Marking this as P1 because of its importance to 59 and 60.
Priority: P3 → P1

        Attachment #8937096 -
        Attachment is obsolete: true

        Attachment #8937104 -
        Attachment is obsolete: true

        Attachment #8937104 -
        Flags: feedback?(dtownsend)

        Attachment #8937105 -
        Attachment is obsolete: true

        Attachment #8937107 -
        Attachment is obsolete: true

        Attachment #8937108 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
  
I addressed the comments on the first review pass, replying to the main comments here:

(In reply to Dave Townsend [:mossop] from comment #12)
> ::: browser/components/enterprisepolicies/EnterprisePolicies.js
> > +    if ((prefType != Services.prefs.PREF_INVALID) && !configFile.exists()) {
> 
> I would check that it is a string pref specifically so when you call
> getStringPref below you don't see an exception in other cases.
> 
> @@ +95,5 @@
> 
> Given that you retrieve the value of this property in the constructor I
> don't think there is anything to gain by using a lazy getter here. Just
> calculate its value (probably in an external function) and set it from the
> constructor.

Done and done

> 
> @@ +111,5 @@
> > +      return;
> > +    }
> > +
> > +    this.status = Ci.nsIEnterprisePolicies.ACTIVE;
> > +    this._activatePolicies();
> 
> I think the ordering here is going to lead to problems. ..

That was a good catch! I was wondering why the ERROR state wasn't being properly set, and that was the reason.
I made the `_readData` function to be a "public" `readData` so that I can call it when it's needed to run.

> @@ +180,5 @@
> > +
> > +  _runPoliciesCallbacks(timing) {
> > +    let callbacks = this._callbacks[timing];
> > +    while (callbacks.length > 0) {
> > +      let callback = callbacks.shift();
> 
> This mutates the _callbacks array. Not a problem for the initial three but
> might be for IdleCallback.

I didn't end up implementing the IdleCallback timing because I think it would be unused. I left the array as is but if it becomes a problem, it's simple to change

> 
> @@ +183,5 @@
> > +    while (callbacks.length > 0) {
> > +      let callback = callbacks.shift();
> > +      try {
> > +        callback();
> > +      } catch (ex) {}
> 
> You should probably log this exception somewhere.

Done! (some logging was only added in the last patch of the series to keep the other patches clean)

> 
> @@ +222,5 @@
> > +  },
> > +
> > +  get status() {
> > +    return this._status;
> > +  },
> 
> This getter and setter doesn't seem useful.

You probably noticed that that was because I'd add more to the setter in later patches

> 
> @@ +224,5 @@
> > +  get status() {
> > +    return this._status;
> > +  },
> > +
> > +  isAllowed: function BG_sanitize(feature) {
> 
> I think this should throw if this.status is UNINITIALIZED

I'd prefer to not have a function that can throw, so I didn't make this change. I could add some console logging if this happens. In practice I don't think any code would be calling this before it is initialized, since it happens early.


(In reply to Dave Townsend [:mossop] from comment #13)
> Comment on attachment 8937107 [details] [diff] [review]
> Part 3 - Add hot-restart of the engine to support test, and create test
> infrastructure
> 
> Review of attachment 8937107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Add a check for Cu.isInAutomation in here somewhere smart please.

I didn't know about this! It was very useful. I had a manual _testing flag which I removed and replaced with this in the _restart function that is only meant for tests.

> 
> ::: browser/components/enterprisepolicies/EnterprisePolicies.js
> @@ +198,5 @@
> > +      return;
> > +    }
> > +    LOG("Restarting");
> > +
> > +    DisallowedFeatures = {};
> 
> You need to set this in the initialProcessData too.

Ah, true. Done.

> 
> @@ +220,5 @@
> > +
> > +          Services.tm.idleDispatchToMainThread(() => {
> > +            Services.obs.notifyObservers(null,
> > +                                         "EnterprisePolicies:Restarted",
> > +                                         null);
> 
> Looks like this will cause a permanent loop. Did you mean to dispatch this
> to the child processes?

It wasn't causing it because the two notifications were named "Restart" and "Restarted". But I ended up changing this slightly with a different notification when all policies are finished being applied, and it turned out cleaner.

(In reply to Dave Townsend [:mossop] from comment #14)
> > +      try {
> > +        if (param.indexOf("://")) {
> > +          // Let's provide a shortcut so that people don't have
> > +          // to write http:// on the URLs
> > +          param = "http://" + param;
> 
> Why should insecure be the default?

This was meant for cases where the protocol doesn't matter, but I realized it was a bad idea and changed it. Now the URL type requires the protocol to be given, and I added a new type called "Host" that is meant only for cases where it doesn't matter (the policies related to the Permissions service will make use of it).

    
    

    
  
I improved the test structure from the previous patch to accept dynamically adding new policies to the schema during tests (using Cu.unload to reload the schema.jsm file). This means that it's no longer necessary to polute the actual policies.json file with test-only policies.

There's also more test coverage to a lot of the basic workings of the engine, and the parameters validation and parsing.

    
    

    
  


  
renamed the const in the .idl s/ERROR/FAILED/ because ERROR was causing conflicts in unified builds.

        Attachment #8940850 -
        Attachment is obsolete: true
No longer blocks: policies-mvp

        Attachment #8940842 -
        Flags: review?(dtownsend)

        Attachment #8940843 -
        Flags: review?(dtownsend)

        Attachment #8940844 -
        Flags: review?(dtownsend)

        Attachment #8940845 -
        Flags: review?(dtownsend)

        Attachment #8940846 -
        Flags: review?(dtownsend)

    
    

    
  
The work on the specific policies desired to be implemented on the first MVP (for the next ESR) is tracked by the meta-bug https://bugzilla.mozilla.org/show_bug.cgi?id=policies-mvp
See Also:  → policies-mvp

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8940845 [details]
Bug 1419102 - Add param validation and parsing.

https://reviewboard.mozilla.org/r/211092/#review217972

::: browser/components/enterprisepolicies/Policies.jsm:141
(Diff revision 2)
> +
> +      try {
> +        parsedParam = Services.io.newURI(param);
> +
> +        let pathQueryRef = parsedParam.pathQueryRef;
> +        // Make sure that "host" types won't accept full URLs that include

drive-by nit: "origin" types.

    
    

    
  
Comment on attachment 8940842 [details]
Bug 1419102 - Basis for the enterprise policy engine.

https://reviewboard.mozilla.org/r/211086/#review218264

Have you considered adding telemetry for whether policies are in effect?

::: browser/components/enterprisepolicies/EnterprisePolicies.js:89
(Diff revision 2)
> +    if (!Services.prefs.getBoolPref(PREF_ENABLED, false)) {
> +      this.status = Ci.nsIEnterprisePolicies.INACTIVE;
> +      return;
> +    }
> +
> +    if (!this._file.exists) {

Why not just call readData first which will bail out early if the file doesn't exist saving you a stat check here.

::: browser/components/enterprisepolicies/EnterprisePolicies.js:130
(Diff revision 2)
> +        continue;
> +      }
> +
> +      if (policySchema.run_once && !modifiedSinceLastRun) {
> +        // This run-once policy already ran
> +        continue;

So this catches the case where the policy gets changed, what about if the policy code itself gets changed, do we need some kind of versioning there to detect that the method of application has changed?

        Attachment #8940842 -
        Flags: review?(dtownsend)

    
    

    
  
> So this catches the case where the policy gets changed, what about if the policy code itself gets changed, do we need some kind of versioning there to detect that the method of application has changed?

This also won't work well in the group policy world.

My hope was that we could do most of this stuff as non persistent so we wouldn't have to worry about firstrun, although things like bookmarks could be tricky. I'm still investigating how other browsers handle this.

    
    

    
  
Comment on attachment 8940843 [details]
Bug 1419102 - Support passing down the information to the content process.

https://reviewboard.mozilla.org/r/211088/#review218300

        Attachment #8940843 -
        Flags: review?(dtownsend) → review+

    
    

    
  
Comment on attachment 8940844 [details]
Bug 1419102 - Add hot-restart of the engine to support test, and create test infrastructure.

https://reviewboard.mozilla.org/r/211090/#review218304

You may want to reset the policy service at the end of each test rather than at the beginning to avoid affecting later tests.

::: browser/components/enterprisepolicies/moz.build:16
(Diff revision 2)
>      'helpers',
>      'schemas',
>  ]
>  
> +TEST_DIRS += [
> +	'tests'

Nit: 4 space indent here.

        Attachment #8940844 -
        Flags: review?(dtownsend) → review+

    
    

    
  
Comment on attachment 8940845 [details]
Bug 1419102 - Add param validation and parsing.

https://reviewboard.mozilla.org/r/211092/#review218320

::: browser/components/enterprisepolicies/schemas/policies.json:5
(Diff revision 2)
>  {
>    "$schema": "http://json-schema.org/draft-04/schema#",
>    "type": "object",
>    "properties": {
> -    "block_about_config": {
> +     "block_about_config": {

This change looks unintentional.

        Attachment #8940845 -
        Flags: review?(dtownsend) → review+

    
    

    
  
Comment on attachment 8940846 [details]
Bug 1419102 - Add proper logging.

https://reviewboard.mozilla.org/r/211094/#review218322

        Attachment #8940846 -
        Flags: review?(dtownsend) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Dave Townsend [:mossop] from comment #32)
> Comment on attachment 8940842 [details]
> Bug 1419102 - Basis for the enterprise policy engine.
> 
> https://reviewboard.mozilla.org/r/211086/#review218264
> 
> Have you considered adding telemetry for whether policies are in effect?

I intend to add some telemetry, but how much hasn't been discussed yet, so I'll leave it to a separate bug.

> 
> ::: browser/components/enterprisepolicies/EnterprisePolicies.js:89
> (Diff revision 2)
> > +    if (!Services.prefs.getBoolPref(PREF_ENABLED, false)) {
> > +      this.status = Ci.nsIEnterprisePolicies.INACTIVE;
> > +      return;
> > +    }
> > +
> > +    if (!this._file.exists) {
> 
> Why not just call readData first which will bail out early if the file
> doesn't exist saving you a stat check here.

Done!
 
> ::: browser/components/enterprisepolicies/EnterprisePolicies.js:130
> (Diff revision 2)
> > +        continue;
> > +      }
> > +
> > +      if (policySchema.run_once && !modifiedSinceLastRun) {
> > +        // This run-once policy already ran
> > +        continue;
> 
> So this catches the case where the policy gets changed, what about if the
> policy code itself gets changed, do we need some kind of versioning there to
> detect that the method of application has changed?

I ended up removing this, as it was gonna be more problematic and not be workable with Windows GPO. Each policy should handle this by itself if needed (which is probably better anyways).
My original worry here was for the bookmarks policy (to avoid creating duplicated bookmarks), but I already have a plan in place on how to solve it for the specific case of bookmarks.

    
    

    
  
Oh, FYI, another change I did was spliting the PoliciesValidator code out from Policies.jsm to its own .jsm. Policies.jsm will grow and there will be a lot of helper functions added to the bottom of it too, so it would become a messy file quickly if the validator code was also there.

    
    

    
  
Comment on attachment 8940842 [details]
Bug 1419102 - Basis for the enterprise policy engine.

https://reviewboard.mozilla.org/r/211086/#review219498

::: commit-message-ac93f:7
(Diff revision 3)
> +* * *
> +[mq]: updatepatch
> +
> +MozReview-Commit-ID: JDcrcFJlyV6
> +* * *
> +try: -b o -p linux,linux64,macosx64,win32,win64 -u none -t none

Don't forget to change this before landing.

        Attachment #8940842 -
        Flags: review?(dtownsend) → review+

    
    

    
  


  
Right now this component is being loaded on app-startup because we believe some policies might need it (specially for private browsing or safe mode).

If it turns out not needed, I'll end up moving the initialization of it later, to profile-after-change

        Attachment #8943650 -
        Flags: review?(florian)

    
    

    
  
Comment on attachment 8940842 [details]
Bug 1419102 - Basis for the enterprise policy engine.

https://reviewboard.mozilla.org/r/211084/#review219774

Drive by comments mostly focused on startup performance. I only really read EnterprisePolicies.js.

::: browser/components/enterprisepolicies/EnterprisePolicies.js:13
(Diff revision 3)
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/AppConstants.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");

Please remove this non-lazy unused import.

::: browser/components/enterprisepolicies/EnterprisePolicies.js:90
(Diff revision 3)
> +    if (!Services.prefs.getBoolPref(PREF_ENABLED, false)) {
> +      this.status = Ci.nsIEnterprisePolicies.INACTIVE;
> +      return;
> +    }
> +
> +    this._file = new JSONFileReader(getConfigurationFile());

This JSONFileReader object feels overengineered.

I don't see where you are removing the reference to this._file, despite only accessing it once in _activatePolicies.

I would do something like:

let file = this._getConfigurationFile();
try {
  let json = this.readJSONFile(file);
  if (!json) {
    this.status = ...INACTIVE;
    return;  
  }
  this._activatePolicies(json);
  this.status = ...ACTIVE;
} catch(e) {
  this.status = ...FAILED;
}

::: browser/components/enterprisepolicies/EnterprisePolicies.js:314
(Diff revision 3)
> +                          createInstance(Ci.nsIFileInputStream);
> +
> +      inputStream.init(this._file, MODE_RDONLY, PERMS_FILE, 0);
> +      this._data.exists = true;
> +
> +      let bytes = NetUtil.readInputStream(inputStream, inputStream.available());

I think this should use Cu.readUTF8File or Cu.readUTF8URI. Then you can remove the NetUtil import and the MODE_RDONLY and PERMS_FILE constants.

::: browser/components/enterprisepolicies/EnterprisePolicies.js:338
(Diff revision 3)
> +function getConfigurationFile() {
> +  let configFile = Services.dirsvc.get("XREAppDist", Ci.nsIFile);
> +  configFile.append(POLICIES_FILENAME);
> +
> +  let prefType = Services.prefs.getPrefType(PREF_ALTERNATE_PATH);
> +

let alternatePath = Services.prefs.getStringPref(PREF_ALTERNATE_PATH, "");
if (alternatePath && !configFile.exists()) {

would be more readable.

    
    

    
  
Comment on attachment 8943650 [details] [diff] [review]
Add it to browser_startup whitelist

Review of attachment 8943650 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/performance/browser_startup.js
@@ +28,5 @@
>    "before profile selection": {whitelist: {
>      components: new Set([
>        "nsBrowserGlue.js",
>        "MainProcessSingleton.js",
> +      "EnterprisePolicies.js",

per our IRC discussion, this should be in the "Bugs to fix" section, with a bug number in a comment, and that bug assigned to you.

Reluctant r=me with that done.

        Attachment #8943650 -
        Flags: review?(florian) → review+

    
    

    
  
I made some changes according to conversations I had, and also to move the .idl to toolkit so that non-Desktop builds don't break.

(The new patches go on top of the previous queue to make it easier to see the changes. Before landing I'll probably flatten everything.)

I also moved the startup to a slightly later stage, so the patch to whitelist the component for app-startup won't be needed.

It was moved to after profile-do-change and before addons-startup. This is the latest that we can go because some policies will need to be checked by code in the startup() functions of some GoFaster add-ons, like Firefox Screenshots, and also by the Add-ons Manager probably.

I believe this handles all the performance suggestions by florian and kmag. There's also some refactoring to JSONFileReader that I'll do on a separate bug when we incorporate the GPO reading code.

        Attachment #8943650 -
        Attachment is obsolete: true

        Attachment #8940867 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8943827 [details]
Bug 1419102 - Move nsIEnterprisePolicies to toolkit so that platform code can include it and attempt to get the service.

https://reviewboard.mozilla.org/r/214200/#review220076

        Attachment #8943827 -
        Flags: review?(dtownsend) → review+

    
    

    
  
Comment on attachment 8943828 [details]
Bug 1419102 - Use Cu.readUTF8File which remembers the file access and reads ahead the file in a background thread on subsequent startups.

https://reviewboard.mozilla.org/r/214202/#review220078

TIL

        Attachment #8943828 -
        Flags: review?(dtownsend) → review+

    
    

    
  
Comment on attachment 8943829 [details]
Bug 1419102 - Make initialization of the Policies manager its own step with a well-defined timing, running before the add-ons manager initializes.

https://reviewboard.mozilla.org/r/214204/#review220082

TIL!

        Attachment #8943829 -
        Flags: review?(dtownsend) → review+

    
    

    
  
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dcf8a5409c2
Implement the Enterprise Policies feature to provide enterprise users with easier control and setup of deployments of Firefox. r=Mossop

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2dcf8a5409c2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox60:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Blocks: 1433173
Blocks: 1434655

    
    

    
  
Is this usable WITHOUT a custom build?
eg: using the .cfg approach we are still using?

    
    

    
  
> Is this usable WITHOUT a custom build?
eg: using the .cfg approach we are still using?

I'm not sure what you mean. It won't work unless you have a version of Firefox that has the policy engine which will be available in Firefox 60 and Firefox 60 ESR (Note that some policies will be ESR only).

We are currently planning to deprecate the Autoconfig (.cfg) in release Firefox. It will still be available in the ESR.

So you will not be able to use the .cfg approach unless you are using the ESR.

    
    

    
  
I'm using the Nightly version ofcourse...
> v60.0a1 build:20180207100355
> Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0

What you mean by "deprecate the Autoconfig (.cfg) in release Firefox" ?! o.O
If you guys are really planning on *removing* that functionality, then this new "feature" we're talking in better have ALL the possibilities the old functionality had, else say bye-bye to enterprise usage of Firefox in *many* cases...
(yes i got a bit mad there sorry)

Let me know so we can start looking for other browsers or at least create a backup of the current state of Firefox to freeze the functionality we use...

    
    

    
  
What specific functions in AutoConfig are you using?

    
    

    
  
> What specific functions in AutoConfig are you using?
Remote preference setting/locking/clearing...

    
    

    
  
Oops add changing default values also ofcourse :) (We need a way to edit our posts here lol)

    
    

    
  
For full access to all preferences via AutoConfig, you'll need to use the ESR.

We will make available common things via the enterprise policy manager.

No other browser provides complete control of every aspect via policy mechanisms. We are providing the same level of control/customizations as other browsers.

This is not the place to discuss this, though. Please take this to the enterprise list if you like or open specific bugs for what you need once you have looked at what we have.

    
    

    
      
  

    
    

    
      
  

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: