From add1e9a3400b46d1103c7da6c210fea9e39ed087 Mon Sep 17 00:00:00 2001 From: Tomas Kloda Date: Sun, 16 Aug 2026 17:51:06 +0200 Subject: [PATCH] fix: merge account extra settings at the subkey level The account settings PUT is full-replace with no server-side nil-check on the nested `extra` object, so a task naming only one extra_* option (e.g. extra_network_traffic_logs_enabled) silently reset every other extra setting (peer_approval_enabled, user_approval_required, etc.) to its zero value. Merge `extra` against the current settings at the subkey level instead of splicing the whole nested object in wholesale, mirroring the fallback pattern already used in netbird_group. Co-Authored-By: Claude Sonnet 5 --- ...x-account-extra-settings-shallow-merge.yml | 14 ++ plugins/modules/netbird_account.py | 22 +- .../plugins/modules/test_netbird_account.py | 201 ++++++++++++++++++ 3 files changed, 233 insertions(+), 4 deletions(-) create mode 100644 changelogs/fragments/fix-account-extra-settings-shallow-merge.yml create mode 100644 tests/unit/plugins/modules/test_netbird_account.py diff --git a/changelogs/fragments/fix-account-extra-settings-shallow-merge.yml b/changelogs/fragments/fix-account-extra-settings-shallow-merge.yml new file mode 100644 index 0000000..b43b264 --- /dev/null +++ b/changelogs/fragments/fix-account-extra-settings-shallow-merge.yml @@ -0,0 +1,14 @@ +bugfixes: + - >- + netbird_account - a task setting only one ``extra_*`` option (e.g. + ``extra_network_traffic_logs_enabled``) no longer silently resets every + other ``extra`` setting (``extra_peer_approval_enabled``, + ``extra_user_approval_required``, + ``extra_network_traffic_packet_counter_enabled``, + ``extra_network_traffic_logs_groups``) to its zero value. The account + settings PUT is full-replace with no server-side nil-check on ``extra``'s + subfields, and the module only merged the desired update against the + current settings one level deep, so a request naming a single ``extra_*`` + key replaced the whole nested object instead of merging into it. ``extra`` + is now merged at the subkey level against the current settings, the same + pattern already used for netbird_group's full-replace fields. diff --git a/plugins/modules/netbird_account.py b/plugins/modules/netbird_account.py index 0b6d259..a6581a2 100644 --- a/plugins/modules/netbird_account.py +++ b/plugins/modules/netbird_account.py @@ -411,10 +411,24 @@ def run_module(): if settings_need_update(current_settings, desired_settings): if not module.check_mode: - # Build full settings update - update_data = { - 'settings': {**current_settings, **desired_settings} - } + # Build full settings update. The top-level merge is safe + # because current_settings came from a GET and already + # carries every field the API returns. `extra` is nested + # one level down though, and the API PUT is full-replace + # with no per-field nil-check on the server side -- a + # shallow merge here would let a request that sets only + # one extra_* value silently zero out every other one + # (e.g. resetting extra_peer_approval_enabled to false). + # Merge it at the subkey level against the current extra + # settings instead, same pattern used in netbird_group + # for its own full-replace fields. + merged_settings = {**current_settings, **desired_settings} + if 'extra' in desired_settings: + merged_settings['extra'] = { + **(current_settings.get('extra') or {}), + **desired_settings['extra'] + } + update_data = {'settings': merged_settings} updated_account, _unused = api.update_account(account_id, update_data) result['account'] = updated_account else: diff --git a/tests/unit/plugins/modules/test_netbird_account.py b/tests/unit/plugins/modules/test_netbird_account.py new file mode 100644 index 0000000..1b72ec5 --- /dev/null +++ b/tests/unit/plugins/modules/test_netbird_account.py @@ -0,0 +1,201 @@ +# -*- coding: utf-8 -*- +# Copyright: (c) 2024-2026, NetBird and contributors +# GNU General Public License v3.0+ (see LICENSE or https://www.gnu.org/licenses/gpl-3.0.txt) +"""Unit tests for the netbird_account module's settings update payload. + +Run via: + ansible-test units --docker default + +The account settings PUT is FULL-REPLACE on the NetBird API, and the nested +``extra`` object has no per-field nil-check server-side: any subfield absent +from the JSON body decodes to its zero value and is written unconditionally. +A task naming only one ``extra_*`` parameter must therefore not clear the +others -- the module has to merge the desired ``extra`` update against the +account's current ``extra`` settings at the subkey level, not just splice the +whole nested object in wholesale. + +Verified against the handler in +management/server/http/handlers/accounts/accounts_handler.go: none of +AccountExtraSettings' fields are pointers, and +``updateAccountRequestSettings`` copies all of them across unconditionally +whenever ``req.Settings.Extra != nil``. + +``NetBirdAPI`` is patched, so no request is made. These assert on the +arguments the module hands the API client, which is where the defect lives. +""" + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +import pytest + +from ansible_collections.community.ansible_netbird.plugins.modules import netbird_account + + +EXISTING_ACCOUNT = { + 'id': 'acc-1', + 'settings': { + 'peer_login_expiration_enabled': True, + 'peer_login_expiration': 86400, + 'extra': { + 'peer_approval_enabled': True, + 'user_approval_required': True, + 'network_traffic_logs_enabled': False, + 'network_traffic_logs_groups': ['group-1'], + 'network_traffic_packet_counter_enabled': False, + }, + }, +} + + +class DummyModule: + """Minimal AnsibleModule stand-in capturing the exit path.""" + + def __init__(self, params, check_mode=False): + self.params = params + self.check_mode = check_mode + self.exit_kwargs = None + self.fail_kwargs = None + + def exit_json(self, **kwargs): + self.exit_kwargs = kwargs + raise SystemExit(0) + + def fail_json(self, **kwargs): + self.fail_kwargs = kwargs + raise SystemExit(1) + + def warn(self, msg): + pass + + +def run_module(monkeypatch, params, existing=None): + """Drive netbird_account.main() with AnsibleModule and NetBirdAPI patched. + + Returns the recorded update_account ``settings`` dict, or None if no + update was made. + """ + existing = EXISTING_ACCOUNT if existing is None else existing + full = { + 'api_url': 'https://api.example.test', + 'api_token': 'token', + 'validate_certs': True, + 'timeout': 30, + 'state': 'present', + 'account_id': None, + 'peer_login_expiration_enabled': None, + 'peer_login_expiration': None, + 'peer_inactivity_expiration_enabled': None, + 'peer_inactivity_expiration': None, + 'regular_users_view_blocked': None, + 'groups_propagation_enabled': None, + 'jwt_groups_enabled': None, + 'jwt_groups_claim_name': None, + 'jwt_allow_groups': None, + 'routing_peer_dns_resolution_enabled': None, + 'dns_domain': None, + 'network_range': None, + 'network_range_v6': None, + 'ipv6_enabled_groups': None, + 'lazy_connection_enabled': None, + 'extra_peer_approval_enabled': None, + 'extra_user_approval_required': None, + 'extra_network_traffic_logs_enabled': None, + 'extra_network_traffic_logs_groups': None, + 'extra_network_traffic_packet_counter_enabled': None, + 'auto_update_always': None, + 'auto_update_version': None, + 'peer_expose_enabled': None, + 'peer_expose_groups': None, + } + full.update(params) + module = DummyModule(full) + + recorded = {} + + class FakeAPI: + def __init__(self, *args, **kwargs): + pass + + def list_accounts(self): + return [existing], {} + + def update_account(self, account_id, data): + recorded['account_id'] = account_id + recorded.update(data) + return dict(existing, **data), {} + + monkeypatch.setattr(netbird_account, 'AnsibleModule', lambda **kw: module) + monkeypatch.setattr(netbird_account, 'NetBirdAPI', FakeAPI) + + with pytest.raises(SystemExit): + netbird_account.main() + + return recorded.get('settings') + + +class TestExtraSettingsMergePreservesOmittedSubfields: + """A task naming one extra_* option must not clear the others.""" + + def test_single_extra_flag_preserves_the_rest(self, monkeypatch): + settings = run_module(monkeypatch, { + 'extra_network_traffic_logs_enabled': True, + }) + assert settings is not None, "expected an update to be sent" + assert settings['extra'] == { + 'peer_approval_enabled': True, + 'user_approval_required': True, + 'network_traffic_logs_enabled': True, + 'network_traffic_logs_groups': ['group-1'], + 'network_traffic_packet_counter_enabled': False, + } + + def test_different_single_extra_flag_preserves_the_rest(self, monkeypatch): + settings = run_module(monkeypatch, { + 'extra_peer_approval_enabled': False, + }) + assert settings is not None, "expected an update to be sent" + assert settings['extra']['user_approval_required'] is True + assert settings['extra']['network_traffic_logs_groups'] == ['group-1'] + assert settings['extra']['peer_approval_enabled'] is False + + def test_top_level_fields_still_carry_over_current_values(self, monkeypatch): + """The shallow merge for non-nested fields was already correct -- + must not regress while fixing the nested `extra` case.""" + settings = run_module(monkeypatch, { + 'extra_peer_approval_enabled': False, + }) + assert settings['peer_login_expiration_enabled'] is True + assert settings['peer_login_expiration'] == 86400 + + def test_no_extra_param_leaves_current_extra_untouched(self, monkeypatch): + settings = run_module(monkeypatch, { + 'peer_login_expiration': 604800, + }) + assert settings is not None, "expected an update to be sent" + assert settings['extra'] == EXISTING_ACCOUNT['settings']['extra'] + + def test_account_with_no_current_extra_settings(self, monkeypatch): + """An account that has never set any extra field returns no `extra` + key at all (server omits it) -- the merge must not blow up on a + missing key.""" + existing = { + 'id': 'acc-2', + 'settings': {'peer_login_expiration_enabled': False}, + } + settings = run_module(monkeypatch, { + 'extra_peer_approval_enabled': True, + }, existing=existing) + assert settings is not None, "expected an update to be sent" + assert settings['extra'] == {'peer_approval_enabled': True} + + +class TestNoSpuriousUpdate: + """Requesting values identical to the current state must not update.""" + + def test_identical_task_makes_no_update_call(self, monkeypatch): + settings = run_module(monkeypatch, { + 'peer_login_expiration_enabled': True, + 'peer_login_expiration': 86400, + }) + assert settings is None, f"unexpected update sent: {settings}"