Refactor updating enterprise policies

[1rneh]

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

[gcp]

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

[1rneh]

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.

[Mossop]

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.

[1rneh]

Yes, it's also to avoid calling the onRemove callback when it's not applied. Additionally we probably want to avoid removing a valid policy if a remote policy update includes that policy with invalid parameters.

Instead we could add another property valid : true/false to each policy entry in _parsedPolicies and take that into account when activating/updating policies.

[gcp]

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

[1rneh]

Why is _status error prone?

[gcp]

IIRC part of the review pointed out mistakes in updating _status. So the idea is to add some checks/assertions here to make catching any bugs easier, by checking if _status matches with the state of the rest of the object?

[gcp]

The trigger was this: #321 (comment)

[gcp]

Also this: #321 (comment)

[lissyx]

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 ?

[1rneh]

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).

[1rneh]

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]

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.

[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.