Refactor updating enterprise policies #321

1rneh · 2026-01-08T09:42:32Z

Refactor updating remote policies
Fix merging policies of a combined provider when remote policies are updated

…d providers

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

gcp · 2026-01-09T19:49:30Z

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

+      );
+
+      if (!isValid) {
+        continue;


This will cause the policy to be removed because it doesn't go in parsedPolicies, unclear if that is what you want.

Good point! I didn't change that behavior. But let's re-think this :)

We have two options when encountering invalid parameters:

Policy is currently active: We can remove the policy or keep it active with unchanged parameters. I think I prefer the latter. It's seems more intentional. In both ways we can warn in the logs about invalid parameters.

Policy is currently inactive: Do nothing.

This function is only called during initialization though so the policy cannot already be active here. The question is what happens later when the policies are updated if this invalid policy isn't listed in _parsedPolicies. It looks to me like you don't want it in _parsedPolicies because that would then cause us to try to call an onRemove handler for the policy even though we haven't actually applied it.

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

…emote-policies-update

1rneh · 2026-01-22T17:21:09Z

Update: This patch includes:

Refactor updating remote policies
Fix merging policies of a combined provider when remote policies are updated
Test merging local and remote policies

Question:
What should we do with the tests under browser/components/enterprisepolicies? Is it enough to run them with local policies?

toolkit/modules/Preferences.sys.mjs

.prettierignore

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

gcp · 2026-01-23T10:52:30Z

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

  },

  get status() {
    return this._status;


Given how error prone updating _status is, maybe we can add some sanity checks here for inconsistent status?

Why is _status error prone?

lissyx · 2026-01-23T17:56:44Z

Question: What should we do with the tests under browser/components/enterprisepolicies? Is it enough to run them with local policies?

(I have not had a look at the PR yet so this may be invalid) From past experience I ran several times in regressions I introduced with remote fetching that broke some policies. So I think we should try and run with both, but maybe not right now, and maybe we'd like to scope them in a flagged suite, maybe enterprise subsuite for mochtest browser and xpcshell ?

…or-remote-policies-update

1rneh · 2026-02-09T14:01:10Z

What should we do with the tests under browser/components/enterprisepolicies? Is it enough to run them with local policies?

From an offline conversation with @mkaply:

Tests under /browser/components/enterprisepolicies/ cover each policy logic. Tests under /toolkit/components/enterprisepolicies/ test the policy engine. We should test whether each policy is applied correctly when provided remotely after startup (not during engine initialization).

…or-remote-policies-update

1rneh · 2026-02-12T09:19:05Z

This PR is growing and entails quite a lot of entangled changes and therefore difficult to split into multiple patches:

Bug 2015809 - Fix merging local and remote policies
Add preference to control policy provision for Bug 2012440 - Repack disables local policies.json and GPO
Bug 2015833 - Fetch remote policies during policy engine initialisation
Removing test suite /browser/components/enterprisepolicies/**/live because it's not testing the policies in a different way except that they are getting applied remotely. That irrelevant for the policy logic. We should aim for test that test the policies when applied remotely and after startup
General refactoring

Note:
Leave out the tests in your review for now. I just noticed that I have removed Testing.servePolicyWithJson but it's still used in the /browser/components/enterprisepolicies/ tests. I will have to fix this first.

Mossop · 2026-02-12T11:25:13Z

toolkit/components/enterprisepolicies/tests/browser/remote/browser_remote_policies_diffing.js

+  },
+};
+
+let policyValue = POLICY_PARAM_STATE.DEFAULT;


Throughout this test you verify the state of the active policies but you should sprinkle in checks that policyValue has the expected state after updates.

Mossop · 2026-02-12T11:34:00Z

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

+    // Combine policies with primaryProvider taking precedence.
    // We only do this for top level policies.
-    this._policies = this._primary._policies;
-    for (let policyName of Object.keys(this._secondary.policies)) {
+    this._policies = structuredClone(this._primaryProvider.policies);
+    for (let [policyName, policyParams] of Object.entries(
+      this._secondaryProvider.policies || {}
+    )) {
      if (!(policyName in this._policies)) {
-        this._policies[policyName] = this._secondary.policies[policyName];
+        this._policies[policyName] = policyParams;
      }
    }


Suggested change

// Combine policies with primaryProvider taking precedence.

// We only do this for top level policies.

this._policies = this._primary._policies;

for (let policyName of Object.keys(this._secondary.policies)) {

this._policies = structuredClone(this._primaryProvider.policies);

for (let [policyName, policyParams] of Object.entries(

this._secondaryProvider.policies || {}

)) {

if (!(policyName in this._policies)) {

this._policies[policyName] = this._secondary.policies[policyName];

this._policies[policyName] = policyParams;

}

}

// Combine policies with primaryProvider taking precedence.

// We only do this for top level policies.

return Object.assign({}, this._secondaryProvider.policies ?? {}, this._primaryProvider.policies);

I think this is just a manual implementation of Object.assign

Mossop · 2026-02-12T11:39:28Z

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

+  _chooseProvider() {
    let platformProvider = null;
-    if (AppConstants.platform == "win" && AppConstants.MOZ_SYSTEM_POLICIES) {
-      platformProvider = new WindowsGPOPoliciesProvider();
-      platformProvider.onPoliciesChanges(handler);
-    } else if (
-      AppConstants.platform == "macosx" &&
-      AppConstants.MOZ_SYSTEM_POLICIES
-    ) {
-      platformProvider = new macOSPoliciesProvider();
-      platformProvider.onPoliciesChanges(handler);
+    if (AppConstants.MOZ_SYSTEM_POLICIES) {
+      if (AppConstants.platform == "win") {
+        platformProvider = new WindowsGPOPoliciesProvider();
+      } else if (AppConstants.platform == "macosx") {
+        platformProvider = new macOSPoliciesProvider();
+      }
    }
-
    let jsonProvider = new JSONPoliciesProvider();
-    jsonProvider.onPoliciesChanges(handler);
-    let remoteProvider = RemotePoliciesProvider.createInstance();
-    remoteProvider.onPoliciesChanges(handler);
    if (platformProvider && platformProvider.hasPolicies) {
      if (jsonProvider.hasPolicies) {
-        return new CombinedProvider(
-          new CombinedProvider(remoteProvider, platformProvider),
-          jsonProvider
-        );
+        return new CombinedProvider(platformProvider, jsonProvider);
      }
-      return new CombinedProvider(remoteProvider, platformProvider);
+      return platformProvider;
    }
-    if (jsonProvider.hasPolicies) {
-      return new CombinedProvider(remoteProvider, jsonProvider);
+    return jsonProvider;
+  },


You don't have to do it here but I think things would make a lot more sense if instead of having CombinedProvider merge two policies it could instead merge any number of policies. Then instead of a binary tree of policy providers there would just be a single CombinedProfiler holding all the real providers and it would be able to merge them all in a single operation.

Mossop · 2026-02-12T11:42:03Z

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

+      );
+
+      if (!isValid) {
+        continue;


This function is only called during initialization though so the policy cannot already be active here. The question is what happens later when the policies are updated if this invalid policy isn't listed in _parsedPolicies. It looks to me like you don't want it in _parsedPolicies because that would then cause us to try to call an onRemove handler for the policy even though we haven't actually applied it.

Mossop · 2026-02-12T11:52:31Z

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

+      );
+
+      if (!isValid) {
+        console.warn(`Parameters for policy ${policyName} are invalid`);


_validatePolicyParams already logs warnings in the event of invalid data so we don't need to do it here.

Mossop · 2026-02-12T11:55:41Z

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

        break;

+      case "EnterprisePolicies:Reset":
+        this._resetEngine().then(null, console.error);


Suggested change

this._resetEngine().then(null, console.error);

this._resetEngine().catch(console.error);

Mossop · 2026-02-12T12:01:07Z

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs

+  /**
+   * Update engine status after parsing a new set of policies
+   */
+  _updateStatus() {


This should set the status to FAILED if this._provider.failed

mkaply

I've asked some questions and made a few suggestions, but assuming this pasts the tests (and you've addressed Mossop's questions), this seems good.

mkaply · 2026-02-12T19:39:24Z

toolkit/components/enterprisepolicies/tests/EnterprisePolicyTesting.sys.mjs

      // of setAndLockPref, it wasn't locked, but calling
      // unlockPref is harmless
-      Preferences.unlock(prefName);
+      if (Services.prefs.prefIsLocked(prefName)) {


Why was this added? As said above, Preference.unlock is harmless?

mkaply · 2026-02-12T19:40:14Z

browser/extensions/felt/content/FeltProcessParent.sys.mjs

  isTesting: "resource:///modules/enterprise/EnterpriseCommon.sys.mjs",
  FeltCommon: "chrome://felt/content/FeltCommon.sys.mjs",
  FeltStorage: "resource:///modules/FeltStorage.sys.mjs",
+  PREF_REMOTE_POLICIES_ENABLED:


What's the reason the same JS is loaded into two different identifiers?

mkaply · 2026-02-12T19:40:59Z

...t/components/enterprisepolicies/tests/browser/remote/browser_remote_policies_missing_live.js

    "DisableDefaultBrowserAgent",
    "DisableForgetButton",
    "DisableFormHistory",
+    "DisableRemoteImprovements",


This is unrelated to this patch.

Refactor updating remote policies and fix merging policies of combine…

3e7f935

…d providers

1rneh requested a review from a team January 8, 2026 09:42

1rneh temporarily deployed to Credentials January 8, 2026 09:42 — with GitHub Actions Inactive

1rneh temporarily deployed to Credentials January 8, 2026 09:56 — with GitHub Actions Inactive

1rneh temporarily deployed to Credentials January 8, 2026 09:57 — with GitHub Actions Inactive

1rneh temporarily deployed to Credentials January 8, 2026 10:00 — with GitHub Actions Inactive

1rneh temporarily deployed to Credentials January 8, 2026 10:06 — with GitHub Actions Inactive

gcp reviewed Jan 9, 2026

View reviewed changes

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs Outdated Show resolved Hide resolved

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs Outdated Show resolved Hide resolved

gcp reviewed Jan 9, 2026

View reviewed changes

toolkit/components/enterprisepolicies/EnterprisePoliciesParent.sys.mjs Show resolved Hide resolved

1rneh added 5 commits January 22, 2026 12:35

Refactor enterprisepolicies tests for remote policies

19bc165

Add comment in firefox-enterprise.js

a8500ba

Test merging local and remote policies

ce4e88e

Spin event loop until remote policies are fetched

ed9a580

Fix lint issues

b0686c7

1rneh requested a review from Mossop January 22, 2026 15:12

Correct setupPolicyEngineWithJson call

d3de4b6

1rneh requested a review from gcp January 22, 2026 16:00

Merge remote-tracking branch 'origin/enterprise-main' into refactor-r…

9f35333

…emote-policies-update

1rneh requested a review from mkaply January 22, 2026 17:21

Mossop reviewed Jan 23, 2026

View reviewed changes

toolkit/modules/Preferences.sys.mjs Show resolved Hide resolved

Mossop requested changes Jan 23, 2026

View reviewed changes

gcp requested changes Jan 23, 2026

View reviewed changes

1rneh added 12 commits February 6, 2026 11:36

Merge remote-tracking branch 'enterprise/enterprise-main' into refact…

bf5c46f

…or-remote-policies-update

Remove EnterprisePoliciesParent from prettierignore

093e5dc

Only set status inactive once

50288a9

Set policy status after parsing policies

99058f7

Correct this.isRemotePoliciesSupported() call

d3e874e

Lint changes

f872f21

Add DisableRemoteImprovements to set of "not live" policies

1ecc8be

Undo pref duplication

dd3fffb

Let felt set pref to disable local policies

eeedb7f

Call spinResolve from observer

76bb370

Correct policies engine status

6be5189

Lint changes

063260d

1rneh requested review from Mossop and gcp February 9, 2026 13:52

1rneh added 7 commits February 12, 2026 08:47

Remove MOZ_AUTOMATION test env

5b4393a

Remove isRemotePoliciesSupported from the policies xpcom interface

e131908

Only parse local policies if local policies supported

c1afb99

Remove browser/**/enterprisepolicies/**/live

015ae5a

Lint changes

ca18fa9

Merge remote-tracking branch 'enterprise/enterprise-main' into refact…

62248a6

…or-remote-policies-update

Remove description update in firefox-enterprise.js

ab0eca5

Mossop requested changes Feb 12, 2026

View reviewed changes

mkaply approved these changes Feb 12, 2026

View reviewed changes

	this._resetEngine().then(null, console.error);
	this._resetEngine().catch(console.error);

Refactor updating enterprise policies #321

Are you sure you want to change the base?

Refactor updating enterprise policies #321

Uh oh!

Conversation

1rneh commented Jan 8, 2026

Uh oh!

Uh oh!

Uh oh!

gcp Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

1rneh commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lissyx commented Jan 23, 2026

Uh oh!

1rneh commented Feb 9, 2026

Uh oh!

1rneh commented Feb 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkaply left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gcp Jan 9, 2026 •

edited

Loading

1rneh commented Jan 22, 2026 •

edited

Loading