Change merge function to only modify first argument.

[jantatje]

If the second argument is modified, then keys from one setting are added to the dict of another setting. This leads to extensions the user has not selected being added to policies.json.

Should fix #220 and #233

[allo-]

Good find!

But you're mostly reversing the logic and returning the other dict? I wonder if it changes which keys stay on conflicts and if the better solution would be to create a deep copy c (I think the merge was thought to work recursive) of a and then add b's keys to c.

[jantatje]

I see, I only made a quick change so creation of policies.json works again, so I did not consider the intention behind the original code. I have updated the PR to prefer elements from a if they exist in both. The new version does not modify a or b, but it does not create a full deep copy either, only creating a copy of those dicts that get merged, so it is not safe to modify the result in any way without doing a deep copy first, except doing another merge.

[allo-]

Yes, I'll check this later. I guess it would be reasonable not to change the inputs, but it won't need a too complicated deep copy when we can assume that it are only nested dicts and lists of basic types.

I can only check it in detail later, but probably it is fine now. It's some time from when I wrote the logic and we'll need it for merging profiles and presets later on again.

[allo-]

Sorry for the delay, but I want to have a look at the new function when I can test it and see if it matches the planned things that will need merges for profiles, existing settings, presets and so on.

Do you have a good test case? In the issues you linked, the enterprise policy seemed to work for me after fixing a thing in the form generation.

[jantatje]

I just did some print(option) debugging in forms.py line 139. If you select and save all options in addons and enterprise policies, and then deselect everything except the last option with enterprise_policies (otherwise option.get('enterprise_policy', {}) returns a new empty dict, i.e. Greasemonkey user tracking doesn't affect it because it does not contain enterprise policies), then all the enterprise policies for these addons are still enabled because they get stored in the dict of the last option with enterprise_policies.

After doing this the "Disable system addon update" option contains this:

{'addons': [],
 'config': {},
 'enterprise_policy': {'DisableBuiltinPDFViewer': True,
                       'DisableFirefoxAccounts': True,
                       'DisableFormHistory': True,
                       'DisableSystemAddonUpdate': True,
                       'DisplayMenuBar': True,
                       'ExtensionSettings': {'CanvasBlocker@kkapsner.net': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/canvasblocker/latest.xpi',
                                                                            'installation_mode': 'normal_installed'},
                                             'ClearURLs@kevinr': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/clearurls/latest.xpi',
                                                                  'installation_mode': 'normal_installed'},
                                             'CookieAutoDelete@kennydo.com': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/cookie-autodelete/latest.xpi',
                                                                              'installation_mode': 'normal_installed'},
                                             'Decentraleyes@ThomasRientjes': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/decentraleyes/latest.xpi',
                                                                              'installation_mode': 'normal_installed'},
                                             'FirefoxMulti-AccountContainers@mozilla.org': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/multi-account-containers/latest.xpi',
                                                                                            'installation_mode': 'normal_installed'},
                                             'TemporaryContainers@stoically': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/temporary-containers/latest.xpi',
                                                                               'installation_mode': 'normal_installed'},
                                             'https-everywhere@eff.org': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/https-everywhere/latest.xpi',
                                                                          'installation_mode': 'normal_installed'},
                                             'jid1-MnnxcxisBPnSXQ@jetpack': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/privacy-badger17/latest.xpi',
                                                                             'installation_mode': 'normal_installed'},
                                             'uBlock0@raymondhill.net': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/ublock-origin/latest.xpi',
                                                                         'installation_mode': 'normal_installed'},
                                             'uMatrix@raymondhill.net': {'install_url': 'https://addons.mozilla.org/firefox/downloads/latest/umatrix/latest.xpi',
                                                                         'installation_mode': 'normal_installed'}},
                       'NewTabPage': False},
 'enterprise_policy_only': True,
 'help_text': 'Disables installation and updating of system addons by '
              'Firefox.<br /><i>(enterprise policy download only)</i>',
 'initial': False,
 'label': 'Disable system addon update',
 'name': 'disable_system_addon_update',
 'type': 'boolean'}

I tested the new merge function by doing the above and observing that options are no longer modified and by passing some hand crafted input into the function.

[allo-]

I guess I merge it now. Why I am looking so deeply into it is, that I am not sure anymore why it needs the recursive function and a dict.update in the dict-branch isn't sufficient.

Maybe it is already prepared for presets adding only single keys inside settings dictionaries which are stored as dictionary items in an dict containing all settings for the selected addons. I should have documented this more.