From 3518a6e3b08e8d8a0bfe1bb20f4458b45d50027f Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sun, 8 Mar 2026 11:56:48 +0100 Subject: [PATCH 01/19] tests + device_fields: transaction.atomic, full_clean, select_for_update, wiring tests CreateAndAssignPlatformView: - Add transaction.atomic() wrapping platform creation + device assignment - Call platform.full_clean() before platform.save() to surface ValidationError - Fetch device with select_for_update() inside transaction (TOCTOU guard) - Add logger.exception() for both ValidationError and IntegrityError - Better error messages (include actual error, not hard-coded 'slug collision') New independent test files (multi-server tests excluded): - test_view_wiring.py: MRO/mixin wiring, CacheMixin, permissions, template syntax - test_reviewer_fixes.py: vc_member_name_pattern, XSS escape, GenerateVcMemberName, lazy validation api passthrough, CreateAndAssignPlatformView full_clean - test_mixins.py: LibreNMSAPIMixin lazy init, get_server_info, CacheMixin key format - test_sync_devices.py: AddDeviceToLibreNMSView, UpdateDeviceLocation, field view wiring --- netbox_librenms_plugin/tests/test_view_wiring.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/netbox_librenms_plugin/tests/test_view_wiring.py b/netbox_librenms_plugin/tests/test_view_wiring.py index 7aa00229a5..0c55d86228 100644 --- a/netbox_librenms_plugin/tests/test_view_wiring.py +++ b/netbox_librenms_plugin/tests/test_view_wiring.py @@ -229,6 +229,7 @@ class DummyView(LibreNMSAPIMixin): # After init, the backing attribute must be None (lazy, not eager) assert dummy._librenms_api is None +<<<<<<< HEAD def test_sync_interfaces_has_librenms_api_property_via_class(self): """BaseLibreNMSSyncView must expose librenms_api through its MRO. @@ -240,6 +241,8 @@ def test_sync_interfaces_has_librenms_api_property_via_class(self): assert any("librenms_api" in vars(cls) for cls in BaseLibreNMSSyncView.__mro__) +======= +>>>>>>> 946109a (tests + device_fields: transaction.atomic, full_clean, select_for_update, wiring tests) # ── Template syntax smoke tests ────────────────────────────────────────────── From f1b1f45b09dc06db0c485310ff4fe873125f36ac Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sun, 8 Mar 2026 23:23:21 +0100 Subject: [PATCH 02/19] fix: use validated-cache namespace for BG job gate + unconditional VC sync delegation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit list.py: Check validated-cache metadata (which includes use_sysname and strip_domain in its key) instead of raw device cache when deciding whether to short-circuit the background job. A naming preference change now correctly shows the cache as cold and routes through the background path instead of doing synchronous re-validation. librenms_sync_view.py: Always delegate VC device resolution to get_librenms_sync_device() instead of first checking get_librenms_device_id(). In a partially migrated VC, a member with an explicit per-server dict is preferred over one with a legacy bare-int — the previous guard could pick the wrong member. Includes 5 new tests covering both fixes. --- netbox_librenms_plugin/tests/test_utils.py | 47 ++++++++++------------ 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/netbox_librenms_plugin/tests/test_utils.py b/netbox_librenms_plugin/tests/test_utils.py index 609d514818..50577dcfdd 100644 --- a/netbox_librenms_plugin/tests/test_utils.py +++ b/netbox_librenms_plugin/tests/test_utils.py @@ -346,67 +346,64 @@ def test_get_librenms_sync_device_with_librenms_id(self): assert result == mock_member_with_id - def test_get_librenms_sync_device_member_with_id_preferred(self): - """A VC member with librenms_id set is preferred over members without.""" + def test_get_librenms_sync_device_dict_preferred_over_legacy_bare_int(self): + """In a partially migrated VC, a member with per-server dict format + is preferred over a member with legacy bare-int format.""" from netbox_librenms_plugin.utils import get_librenms_sync_device + # Member A: legacy bare-int librenms_id (not yet migrated) member_a = MagicMock() - member_a.cf = {} + member_a.cf = {"librenms_id": 42} + # Member B: migrated per-server dict format member_b = MagicMock() - member_b.cf = {"librenms_id": 42} + member_b.cf = {"librenms_id": {"default": 42}} mock_device = MagicMock() mock_device.virtual_chassis = MagicMock() + # member_a listed first — the function should still prefer member_b mock_device.virtual_chassis.members.all.return_value = [member_a, member_b] - result = get_librenms_sync_device(mock_device) + result = get_librenms_sync_device(mock_device, server_key="default") assert result == member_b - def test_get_librenms_sync_device_fallback_to_master_with_ip(self): - """When no member has librenms_id, fall back to master with primary IP.""" + def test_get_librenms_sync_device_legacy_fallback_when_no_dict(self): + """When no member has a per-server dict, fall back to legacy bare-int.""" from netbox_librenms_plugin.utils import get_librenms_sync_device member_a = MagicMock() - member_a.cf = {} - member_a.primary_ip = MagicMock() - + member_a.cf = {"librenms_id": 42} member_b = MagicMock() member_b.cf = {} - member_b.primary_ip = None mock_device = MagicMock() mock_device.virtual_chassis = MagicMock() - mock_device.virtual_chassis.master = member_a - mock_device.virtual_chassis.master.primary_ip = MagicMock() - mock_device.virtual_chassis.members.all.return_value = [member_a, member_b] + mock_device.virtual_chassis.members.all.return_value = [member_b, member_a] - result = get_librenms_sync_device(mock_device) + result = get_librenms_sync_device(mock_device, server_key="default") assert result == member_a - def test_get_librenms_sync_device_fallback_to_any_member_with_ip(self): - """When no member has librenms_id and master has no IP, fall back to any member with IP.""" + def test_get_librenms_sync_device_dict_for_different_server_falls_through(self): + """Per-server dict with a different key does not match; legacy bare-int resolves instead.""" from netbox_librenms_plugin.utils import get_librenms_sync_device + # Member A: legacy bare-int (universal fallback) member_a = MagicMock() - member_a.cf = {} - member_a.primary_ip = None + member_a.cf = {"librenms_id": 42} + # Member B: dict but only for "production", not "default" member_b = MagicMock() - member_b.cf = {} - member_b.primary_ip = MagicMock() + member_b.cf = {"librenms_id": {"production": 99}} mock_device = MagicMock() mock_device.virtual_chassis = MagicMock() - mock_device.virtual_chassis.master = member_a - mock_device.virtual_chassis.master.primary_ip = None mock_device.virtual_chassis.members.all.return_value = [member_a, member_b] - result = get_librenms_sync_device(mock_device) + result = get_librenms_sync_device(mock_device, server_key="default") - assert result == member_b + assert result == member_a # ============================================================================= From b943969df5172c16460ad5e3bc9fc2b04d62ea7e Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Mon, 9 Mar 2026 13:56:07 +0100 Subject: [PATCH 03/19] fix: replace multi-server VC tests with branch-appropriate variants The cherry-picked VC sync delegation tests passed server_key= to get_librenms_sync_device(), which doesn't exist on this pre-id branch. Replaced with tests that exercise the actual priority order on this branch: member with librenms_id > master with IP > any member with IP. --- netbox_librenms_plugin/tests/test_utils.py | 47 ++++++++++++---------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/netbox_librenms_plugin/tests/test_utils.py b/netbox_librenms_plugin/tests/test_utils.py index 50577dcfdd..609d514818 100644 --- a/netbox_librenms_plugin/tests/test_utils.py +++ b/netbox_librenms_plugin/tests/test_utils.py @@ -346,64 +346,67 @@ def test_get_librenms_sync_device_with_librenms_id(self): assert result == mock_member_with_id - def test_get_librenms_sync_device_dict_preferred_over_legacy_bare_int(self): - """In a partially migrated VC, a member with per-server dict format - is preferred over a member with legacy bare-int format.""" + def test_get_librenms_sync_device_member_with_id_preferred(self): + """A VC member with librenms_id set is preferred over members without.""" from netbox_librenms_plugin.utils import get_librenms_sync_device - # Member A: legacy bare-int librenms_id (not yet migrated) member_a = MagicMock() - member_a.cf = {"librenms_id": 42} + member_a.cf = {} - # Member B: migrated per-server dict format member_b = MagicMock() - member_b.cf = {"librenms_id": {"default": 42}} + member_b.cf = {"librenms_id": 42} mock_device = MagicMock() mock_device.virtual_chassis = MagicMock() - # member_a listed first — the function should still prefer member_b mock_device.virtual_chassis.members.all.return_value = [member_a, member_b] - result = get_librenms_sync_device(mock_device, server_key="default") + result = get_librenms_sync_device(mock_device) assert result == member_b - def test_get_librenms_sync_device_legacy_fallback_when_no_dict(self): - """When no member has a per-server dict, fall back to legacy bare-int.""" + def test_get_librenms_sync_device_fallback_to_master_with_ip(self): + """When no member has librenms_id, fall back to master with primary IP.""" from netbox_librenms_plugin.utils import get_librenms_sync_device member_a = MagicMock() - member_a.cf = {"librenms_id": 42} + member_a.cf = {} + member_a.primary_ip = MagicMock() + member_b = MagicMock() member_b.cf = {} + member_b.primary_ip = None mock_device = MagicMock() mock_device.virtual_chassis = MagicMock() - mock_device.virtual_chassis.members.all.return_value = [member_b, member_a] + mock_device.virtual_chassis.master = member_a + mock_device.virtual_chassis.master.primary_ip = MagicMock() + mock_device.virtual_chassis.members.all.return_value = [member_a, member_b] - result = get_librenms_sync_device(mock_device, server_key="default") + result = get_librenms_sync_device(mock_device) assert result == member_a - def test_get_librenms_sync_device_dict_for_different_server_falls_through(self): - """Per-server dict with a different key does not match; legacy bare-int resolves instead.""" + def test_get_librenms_sync_device_fallback_to_any_member_with_ip(self): + """When no member has librenms_id and master has no IP, fall back to any member with IP.""" from netbox_librenms_plugin.utils import get_librenms_sync_device - # Member A: legacy bare-int (universal fallback) member_a = MagicMock() - member_a.cf = {"librenms_id": 42} + member_a.cf = {} + member_a.primary_ip = None - # Member B: dict but only for "production", not "default" member_b = MagicMock() - member_b.cf = {"librenms_id": {"production": 99}} + member_b.cf = {} + member_b.primary_ip = MagicMock() mock_device = MagicMock() mock_device.virtual_chassis = MagicMock() + mock_device.virtual_chassis.master = member_a + mock_device.virtual_chassis.master.primary_ip = None mock_device.virtual_chassis.members.all.return_value = [member_a, member_b] - result = get_librenms_sync_device(mock_device, server_key="default") + result = get_librenms_sync_device(mock_device) - assert result == member_a + assert result == member_b # ============================================================================= From c8fd7a7b24076e79f8b0094b3322a882a0f81d22 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 7 Mar 2026 12:24:37 +0100 Subject: [PATCH 04/19] feat: multi-server librenms_id with JSON custom field and server management Replace bare-integer librenms_id custom field with a per-server JSON dict format (e.g., {"primary": 42, "secondary": 99}) to support multi-server LibreNMS deployments. Key changes: - Add get/set/find/migrate helpers for server-scoped librenms_id in utils.py - Server-aware cache keys across all sync views (interfaces, cables, VLANs, IPs) - Legacy bare-int IDs act as universal fallback for any server_key - Convert ID badge on sync page for migrating legacy IDs with serial verification - RemoveServerMappingView and ConvertLegacyLibreNMSIdView with collision checks - Bool rejection guards on all librenms_id helpers (bool is subclass of int) - CSRF token and response.ok guard in cable verify JS/HTML - Normalize server_key to "default" early in verify/VLAN-override views - DoesNotExist guard on select_for_update in CreateAndAssignPlatformView - Comprehensive tests: test_librenms_id, test_mixins, test_sync_devices, test_sync_interfaces, test_sync_view_mismatch, test_permissions, test_view_wiring smoke tests for new views --- netbox_librenms_plugin/forms.py | 8 +- netbox_librenms_plugin/import_utils/cache.py | 7 +- .../import_utils/device_operations.py | 49 ++- netbox_librenms_plugin/librenms_api.py | 19 +- .../js/librenms_sync.js | 6 +- .../tables/device_status.py | 13 +- netbox_librenms_plugin/tables/interfaces.py | 6 +- .../_interface_sync_content.html | 1 + .../htmx/device_validation_details.html | 39 +- .../librenms_sync_base.html | 96 ++++- netbox_librenms_plugin/tests/conftest.py | 53 ++- .../tests/test_import_utils.py | 252 +++++++++-- .../tests/test_librenms_id.py | 372 ++++++++++++++++ netbox_librenms_plugin/tests/test_mixins.py | 51 +++ .../tests/test_sync_devices.py | 58 +++ .../tests/test_sync_interfaces.py | 297 +++++++++++++ .../tests/test_sync_view_mismatch.py | 91 +++- .../tests/test_view_wiring.py | 37 +- netbox_librenms_plugin/urls.py | 12 + netbox_librenms_plugin/utils.py | 207 ++++++++- netbox_librenms_plugin/views/__init__.py | 2 + .../views/base/cables_view.py | 92 ++-- .../views/base/interfaces_view.py | 17 +- .../views/base/ip_addresses_view.py | 7 +- .../views/base/librenms_sync_view.py | 102 ++++- .../views/base/vlan_table_view.py | 12 +- .../views/imports/actions.py | 401 +++++++++++++----- netbox_librenms_plugin/views/mixins.py | 24 +- .../views/object_sync/devices.py | 41 +- .../views/object_sync/vms.py | 8 +- netbox_librenms_plugin/views/sync/cables.py | 39 +- .../views/sync/device_fields.py | 217 +++++++++- .../views/sync/interfaces.py | 20 +- .../views/sync/ip_addresses.py | 12 +- netbox_librenms_plugin/views/sync/vlans.py | 11 +- 35 files changed, 2348 insertions(+), 331 deletions(-) create mode 100644 netbox_librenms_plugin/tests/test_librenms_id.py create mode 100644 netbox_librenms_plugin/tests/test_sync_interfaces.py diff --git a/netbox_librenms_plugin/forms.py b/netbox_librenms_plugin/forms.py index f3e3d075e5..4116eea15b 100644 --- a/netbox_librenms_plugin/forms.py +++ b/netbox_librenms_plugin/forms.py @@ -577,19 +577,19 @@ def _populate_librenms_locations(self): """Fetch and populate LibreNMS locations in the dropdown.""" from django.core.cache import cache + from netbox_librenms_plugin.import_utils.cache import get_location_choices_cache_key from netbox_librenms_plugin.librenms_api import LibreNMSAPI try: - # Use caching to avoid repeated API calls - cache_key = "librenms_locations_choices" + # Instantiate the API client to resolve the authoritative server_key + api = LibreNMSAPI() + cache_key = get_location_choices_cache_key(api.server_key) cached_choices = cache.get(cache_key) - if cached_choices: self.fields["librenms_location"].choices = cached_choices return # Fetch locations from LibreNMS - api = LibreNMSAPI() success, locations = api.get_locations() if success and locations: diff --git a/netbox_librenms_plugin/import_utils/cache.py b/netbox_librenms_plugin/import_utils/cache.py index fe0896a302..6cc914e4d9 100644 --- a/netbox_librenms_plugin/import_utils/cache.py +++ b/netbox_librenms_plugin/import_utils/cache.py @@ -7,6 +7,11 @@ logger = logging.getLogger(__name__) +def get_location_choices_cache_key(server_key: str) -> str: + """Return the cache key for LibreNMS location choices for a given server.""" + return f"librenms_locations_choices:{server_key}" + + def get_cache_metadata_key(server_key: str, filters: dict, vc_enabled: bool) -> str: """ Generate a consistent cache metadata key from filter parameters. @@ -62,7 +67,7 @@ def get_active_cached_searches(server_key: str) -> list[dict]: } # Get cached location choices for enrichment - location_cache_key = "librenms_locations_choices" + location_cache_key = get_location_choices_cache_key(server_key) cached_locations = cache.get(location_cache_key) if cached_locations: location_choices = dict(cached_locations) diff --git a/netbox_librenms_plugin/import_utils/device_operations.py b/netbox_librenms_plugin/import_utils/device_operations.py index c25fff7b8e..fd81abe2f7 100644 --- a/netbox_librenms_plugin/import_utils/device_operations.py +++ b/netbox_librenms_plugin/import_utils/device_operations.py @@ -1,6 +1,7 @@ """Device validation, import, and fetch operations.""" import logging +from types import SimpleNamespace from dcim.models import Device, DeviceRole, DeviceType, Rack, Site from django.core.cache import cache @@ -11,9 +12,11 @@ from ..librenms_api import LibreNMSAPI from ..utils import ( + find_by_librenms_id, find_matching_platform, find_matching_site, match_librenms_hardware_to_device_type, + set_librenms_device_id, ) from .cache import get_import_device_cache_key from .virtual_chassis import ( @@ -125,6 +128,7 @@ def validate_device_for_import( import_as_vm: bool = False, api: "LibreNMSAPI" = None, *, + server_key: str = "default", include_vc_detection: bool = True, force_vc_refresh: bool = False, use_sysname: bool = True, @@ -207,6 +211,7 @@ def validate_device_for_import( "name_sync_available": False, # True when existing device name differs from sysName "suggested_name": None, # sysName to suggest when name_sync_available is True "device_type_mismatch": False, # True when existing device's type differs from LibreNMS + "librenms_id_needs_migration": False, # True when existing device has legacy bare-int ID "issues": [], "warnings": [], "virtual_chassis": empty_virtual_chassis_data(), @@ -275,13 +280,12 @@ def validate_device_for_import( from virtualization.models import VirtualMachine - # Check for existing VM first (by librenms_id custom field) - # Always query with int to match custom field type - try: - existing_vm = VirtualMachine.objects.filter(custom_field_data__librenms_id=int(librenms_id)).first() - except (ValueError, TypeError): - # librenms_id is not convertible to int; no match will be found - existing_vm = None + server_key = api.server_key if api is not None else server_key + + # Check for existing VM first (by librenms_id custom field). + # find_by_librenms_id() covers both the new per-server JSON format + # and legacy bare-integer values so neither is missed. + existing_vm = find_by_librenms_id(VirtualMachine, librenms_id, server_key) if existing_vm: logger.info(f"Found existing VM: {existing_vm.name} (matched by librenms_id={librenms_id})") @@ -290,6 +294,11 @@ def validate_device_for_import( result["import_as_vm"] = True # Force VM mode since VM exists result["can_import"] = False + # Flag legacy bare-int or string-digit librenms_id for migration to per-server dict format + _vm_cf_id = existing_vm.custom_field_data.get("librenms_id") + if isinstance(_vm_cf_id, int) or (isinstance(_vm_cf_id, str) and _vm_cf_id.isdigit()): + result["librenms_id_needs_migration"] = True + # Check if name matches sysName # Note: name_sync_available/suggested_name are intentionally not set for VMs # because UpdateDeviceNameView only supports Device objects; VM name-sync @@ -297,14 +306,11 @@ def validate_device_for_import( if hostname and existing_vm.name == hostname: result["name_matches"] = True - # Check for existing Device (by librenms_id custom field) - # Always query with int to match custom field type + # Check for existing Device (by librenms_id custom field). + # find_by_librenms_id() covers both the new per-server JSON format + # and legacy bare-integer values so neither is missed. if not result["existing_device"]: - try: - existing_device = Device.objects.filter(custom_field_data__librenms_id=int(librenms_id)).first() - except (ValueError, TypeError): - # librenms_id is not convertible to int; no match will be found - existing_device = None + existing_device = find_by_librenms_id(Device, librenms_id, server_key) if existing_device: logger.info(f"Found existing device: {existing_device.name} (matched by librenms_id={librenms_id})") @@ -312,6 +318,13 @@ def validate_device_for_import( result["existing_match_type"] = "librenms_id" result["can_import"] = False + # Detect legacy bare-integer or string-digit format so UI can offer a migration action. + _dev_cf_id = existing_device.custom_field_data.get("librenms_id") + if (isinstance(_dev_cf_id, int) and not isinstance(_dev_cf_id, bool)) or ( + isinstance(_dev_cf_id, str) and _dev_cf_id.isdigit() + ): + result["librenms_id_needs_migration"] = True + # Check if name matches resolved name (VC-aware: compare against VC member name) if hostname and existing_device.virtual_chassis and existing_device.vc_position: incoming_serial = libre_device.get("serial") or "" @@ -726,6 +739,7 @@ def import_single_device( api=api, use_sysname=use_sysname_opt, strip_domain=strip_domain_opt, + server_key=api.server_key, ) # Check if device already exists @@ -804,6 +818,8 @@ def import_single_device( # Generate import timestamp comment import_time = timezone.now().strftime("%Y-%m-%d %H:%M:%S %Z") + _cf_proxy = SimpleNamespace(custom_field_data={}) + set_librenms_device_id(_cf_proxy, device_id, api.server_key) device_data = { "name": device_name, "site": site, @@ -811,7 +827,7 @@ def import_single_device( "role": device_role, "status": "active" if libre_device.get("status") == 1 else "offline", "comments": f"Imported from LibreNMS by netbox-librenms-plugin on {import_time}", - "custom_field_data": {"librenms_id": int(device_id)}, + "custom_field_data": _cf_proxy.custom_field_data, } # Add optional fields @@ -836,6 +852,9 @@ def import_single_device( # Create the device device = Device(**device_data) + # Store librenms_id in per-server dict format before validation so the + # mapping is present on the instance when full_clean() runs. + set_librenms_device_id(device, device_id, api.server_key) device.full_clean() device.save() diff --git a/netbox_librenms_plugin/librenms_api.py b/netbox_librenms_plugin/librenms_api.py index 02f7e3459e..8e2b2b3ac0 100644 --- a/netbox_librenms_plugin/librenms_api.py +++ b/netbox_librenms_plugin/librenms_api.py @@ -190,16 +190,11 @@ def get_librenms_id(self, obj): If found via API, stores ID in custom field if available, otherwise caches the value. """ - librenms_id = obj.cf.get("librenms_id") - if librenms_id is not None: - if isinstance(librenms_id, str): - try: - librenms_id = int(librenms_id) - self._store_librenms_id(obj, librenms_id) - except (ValueError, TypeError): - librenms_id = None # empty or invalid string — fall through to discovery - if librenms_id: - return librenms_id + from netbox_librenms_plugin.utils import get_librenms_device_id + + librenms_id = get_librenms_device_id(obj, self.server_key) + if librenms_id: + return librenms_id # Check cache cache_key = self._get_cache_key(obj) @@ -261,7 +256,9 @@ def _store_librenms_id(self, obj, librenms_id): None """ if "librenms_id" in obj.cf: - obj.custom_field_data["librenms_id"] = librenms_id + from netbox_librenms_plugin.utils import set_librenms_device_id + + set_librenms_device_id(obj, librenms_id, self.server_key) obj.save() else: # Use cache as fallback diff --git a/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js b/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js index c56f9dee97..a8783ca575 100644 --- a/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js +++ b/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js @@ -624,7 +624,8 @@ function initializeVlanModalSave() { }, body: JSON.stringify({ device_id: deviceId, - vid_group_map: vidGroupMap + vid_group_map: vidGroupMap, + server_key: document.getElementById('current-server-key')?.value || null }) }).then(response => { if (!response.ok) { @@ -813,7 +814,8 @@ function handleInterfaceChange(select, value) { body: JSON.stringify({ device_id: value, interface_name: select.dataset.interface, - interface_name_field: document.querySelector('input[name="interface_name_field"]:checked')?.value || null + interface_name_field: document.querySelector('input[name="interface_name_field"]:checked')?.value || null, + server_key: document.getElementById('current-server-key')?.value || null }) }) .then(response => { diff --git a/netbox_librenms_plugin/tables/device_status.py b/netbox_librenms_plugin/tables/device_status.py index da5b02d0bf..f6dcc97e49 100644 --- a/netbox_librenms_plugin/tables/device_status.py +++ b/netbox_librenms_plugin/tables/device_status.py @@ -481,17 +481,24 @@ def render_actions(self, value, record): btn_class = "btn-outline-warning" btn_icon = "mdi-information-outline" btn_label = " Details" - btn_title = "View details" elif match_type == "librenms_id" and validation.get("librenms_id_needs_migration"): btn_class = "btn-outline-warning" btn_icon = "mdi-database-alert" btn_label = " Legacy ID" - btn_title = "Migrate Legacy ID" else: btn_class = "btn-outline-success" btn_icon = "mdi-check-circle" btn_label = "" - btn_title = "View details" + + btn_title = ( + "Resolve conflict" + if (has_actions or has_mismatch) + else ( + "Migrate Legacy ID" + if match_type == "librenms_id" and validation.get("librenms_id_needs_migration") + else "View details" + ) + ) aria_attr = f'aria-label="{btn_title}" ' buttons.append( f' + + + {% endif %} {% elif validation.existing_match_type == 'hostname' %}
diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html index 82fd96ff03..a756c29a9f 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html @@ -29,7 +29,72 @@ {% block content %} -{% if librenms_server_info %} +{% if all_server_mappings %} +
+
+ LibreNMS Connections + {% if librenms_server_info and not librenms_server_info.is_legacy %} + + Change Server + + {% endif %} +
+
+ + + {% for mapping in all_server_mappings %} + + + + + + {% endfor %} + +
+ {% if mapping.is_active %} + + {% elif mapping.is_configured %} + + {% else %} + + {% endif %} + {% if mapping.is_configured %} + {{ mapping.display_name }} + {% else %} + {{ mapping.server_key }} + Not configured + {% endif %} + + {% if mapping.device_url %} + + ID {{ mapping.device_id }} + + + {% else %} + ID {{ mapping.device_id }} + {% endif %} + + {% if not mapping.is_configured %} + {% if lookup_device_model_name == "device" or lookup_device_model_name == "virtualmachine" %} +
+ {% csrf_token %} + + + +
+ {% endif %} + {% endif %} +
+
+
+{% elif librenms_server_info %}
@@ -88,7 +153,29 @@
ID - {{ librenms_device_id }} + + {{ librenms_device_id }} + {% if librenms_id_is_legacy %} + {% with model_name=object|meta:"model_name" %} +
+ {% csrf_token %} + + {% if librenms_id_serial_confirmed %} + + {% else %} + + {% endif %} +
+ {% endwith %} + {% endif %} + @@ -249,7 +336,7 @@
Device Information Sync
{{ object.name }}
- {% if sysName and sysName != object.name %} + {% if sysName and sysName != "-" and sysName != object.name %}
@@ -259,7 +346,7 @@
Device Information Sync
Sync to NetBox
- {% elif sysName %} + {% elif sysName and sysName != "-" %} @@ -574,7 +661,6 @@
Device Information Sync
{% endwith %}
- {% else %}
diff --git a/netbox_librenms_plugin/tests/conftest.py b/netbox_librenms_plugin/tests/conftest.py index c897c5ef56..a6bba487f7 100644 --- a/netbox_librenms_plugin/tests/conftest.py +++ b/netbox_librenms_plugin/tests/conftest.py @@ -49,12 +49,10 @@ def mock_librenms_api(mock_multi_server_config): """Pre-configured LibreNMSAPI instance with mocked dependencies.""" with patch("netbox_librenms_plugin.librenms_api.get_plugin_config") as mock_config: mock_config.return_value = mock_multi_server_config - with patch("netbox_librenms_plugin.librenms_api.LibreNMSSettings") as mock_settings: - mock_settings.objects.filter.return_value.first.return_value = None - from netbox_librenms_plugin.librenms_api import LibreNMSAPI + from netbox_librenms_plugin.librenms_api import LibreNMSAPI - api = LibreNMSAPI(server_key="default") - yield api + api = LibreNMSAPI(server_key="default") + yield api # ============================================================================= @@ -291,3 +289,48 @@ def mock_netbox_rack(): rack.name = "Rack A1" rack.site = MagicMock(id=1, name="DC1") return rack + + +# ============================================================================= +# Server Mapping Fixtures (used by test_sync_view_mismatch.py) +# ============================================================================= + + +@pytest.fixture +def mock_plugins_config_single_server(): + """PLUGINS_CONFIG with a single 'production' server (for _build_all_server_mappings tests).""" + return { + "netbox_librenms_plugin": { + "servers": { + "production": { + "display_name": "Production LibreNMS", + "librenms_url": "https://librenms.example.com", + }, + } + } + } + + +@pytest.fixture +def mock_plugins_config_empty_servers(): + """PLUGINS_CONFIG with no configured servers (simulates all orphaned).""" + return {"netbox_librenms_plugin": {"servers": {}}} + + +@pytest.fixture +def mock_plugins_config_multi_server_mapping(): + """PLUGINS_CONFIG with 'production' and 'mock-dev' servers (for multi-server mapping tests).""" + return { + "netbox_librenms_plugin": { + "servers": { + "production": { + "display_name": "Production LibreNMS", + "librenms_url": "https://librenms.example.com", + }, + "mock-dev": { + "display_name": "Mock", + "librenms_url": "http://mock.example.com", + }, + } + } + } diff --git a/netbox_librenms_plugin/tests/test_import_utils.py b/netbox_librenms_plugin/tests/test_import_utils.py index a63f49d1e3..4d4475bdad 100644 --- a/netbox_librenms_plugin/tests/test_import_utils.py +++ b/netbox_librenms_plugin/tests/test_import_utils.py @@ -1343,22 +1343,36 @@ def _configure_standard_mocks(self): self.mock_site_model.objects.all.return_value = [] def _setup_librenms_id_match(self, existing_device, as_vm=False): - """Configure mocks so that a device is found by librenms_id.""" + """Configure mocks so that a device is found by librenms_id. + + Uses a Q-aware side_effect so only filter() calls targeting a + ``librenms_id`` field return the existing device; other filter() calls + (e.g. name lookups, serial lookups) return an empty queryset. + """ + from unittest.mock import MagicMock + + def _librenms_id_filter_side_effect(hit): + def side_effect(*args, **kwargs): + mock_qs = MagicMock() + # Match when the first positional arg is a Q that references librenms_id + if args: + q = args[0] + if hasattr(q, "children") and any( + isinstance(child, tuple) and "librenms_id" in child[0] for child in q.children + ): + mock_qs.first.return_value = hit + return mock_qs + mock_qs.first.return_value = None + return mock_qs + + return side_effect + if as_vm: - self.mock_vm.objects.filter.return_value.first.return_value = existing_device - self.mock_device.objects.filter.return_value.first.return_value = None + self.mock_vm.objects.filter.side_effect = _librenms_id_filter_side_effect(existing_device) + self.mock_device.objects.filter.side_effect = _librenms_id_filter_side_effect(None) else: - self.mock_vm.objects.filter.return_value.first.return_value = None - - def device_filter(**kwargs): - result = MagicMock() - if "custom_field_data__librenms_id" in kwargs: - result.first.return_value = existing_device - else: - result.first.return_value = None - return result - - self.mock_device.objects.filter.side_effect = device_filter + self.mock_device.objects.filter.side_effect = _librenms_id_filter_side_effect(existing_device) + self.mock_vm.objects.filter.side_effect = _librenms_id_filter_side_effect(None) def setup_method(self): """Set up common patches.""" @@ -1647,7 +1661,7 @@ def test_serial_match_blocks_import(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() if "serial" in kwargs: result.first.return_value = existing @@ -1674,7 +1688,7 @@ def test_serial_match_same_hostname_offers_link(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() if "serial" in kwargs: result.first.return_value = existing @@ -1701,7 +1715,7 @@ def test_serial_match_diff_hostname_offers_hostname_differs(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() if "serial" in kwargs: result.first.return_value = existing @@ -1728,7 +1742,7 @@ def test_hostname_match_diff_serial_offers_update(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() if "name__iexact" in kwargs: result.first.return_value = existing @@ -1809,7 +1823,7 @@ def test_hostname_match_serial_conflict_warns(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() if "name__iexact" in kwargs: result.first.return_value = hostname_device @@ -1839,9 +1853,9 @@ def test_librenms_id_match_shows_serial_confirmed(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() - if "custom_field_data__librenms_id" in kwargs: + if args: # Q-object call from find_by_librenms_id result.first.return_value = existing else: result.first.return_value = None @@ -1879,9 +1893,9 @@ def test_librenms_id_match_detects_serial_drift(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() - if "custom_field_data__librenms_id" in kwargs: + if args: # Q-object call from find_by_librenms_id result.first.return_value = existing elif "serial" in kwargs: result.first.return_value = None @@ -1921,9 +1935,9 @@ def test_librenms_id_match_still_validates_site(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() - if "custom_field_data__librenms_id" in kwargs: + if args: # Q-object call from find_by_librenms_id result.first.return_value = existing else: result.first.return_value = None @@ -1964,7 +1978,7 @@ def test_existing_device_role_populated(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() if "serial" in kwargs: result.first.return_value = existing @@ -2016,7 +2030,7 @@ def test_device_type_mismatch_flagged(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() if "serial" in kwargs: result.first.return_value = existing @@ -2066,7 +2080,7 @@ def test_no_device_type_mismatch_when_types_match(self): self.mock_vm.objects.filter.return_value.first.return_value = None - def device_filter(**kwargs): + def device_filter(*args, **kwargs): result = MagicMock() if "serial" in kwargs: result.first.return_value = existing @@ -2107,7 +2121,7 @@ def _create_view(self): """Create a DeviceConflictActionView instance with mocked dependencies.""" from netbox_librenms_plugin.views.imports.actions import DeviceConflictActionView - view = object.__new__(DeviceConflictActionView) + view = DeviceConflictActionView() view._librenms_api = MagicMock() view._librenms_api.server_key = "default" view.request = MagicMock() @@ -2117,11 +2131,14 @@ def _create_view(self): def _create_request(self, action, existing_device_id, use_sysname=False, strip_domain=False): """Create a mock request with POST data.""" request = MagicMock() - post_data = {"action": action, "existing_device_id": str(existing_device_id)} - if use_sysname: - post_data["use-sysname-toggle"] = "on" - if strip_domain: - post_data["strip-domain-toggle"] = "on" + # Always include both toggles so _resolve_naming_preferences never falls through + # to the user-pref/settings DB path, which would hit the real database. + post_data = { + "action": action, + "existing_device_id": str(existing_device_id), + "use-sysname-toggle": "on" if use_sysname else "off", + "strip-domain-toggle": "on" if strip_domain else "off", + } request.POST = post_data return request @@ -2152,18 +2169,67 @@ def test_link_action_sets_librenms_id_and_name(self, mock_cache_key, mock_cache) patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, patch.object(DeviceConflictActionView, "render_device_row") as mock_render, patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, ): + mock_tx.atomic.return_value = MagicMock() mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.first.return_value = None mock_device_cls.objects.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() view.post(request, device_id=10) - assert existing_device.custom_field_data["librenms_id"] == 10 + assert existing_device.custom_field_data["librenms_id"] == {"default": 10} assert existing_device.name == "switch-01.example.com" existing_device.save.assert_called_once() + @patch("netbox_librenms_plugin.views.imports.actions.cache") + @patch("netbox_librenms_plugin.views.imports.actions.get_import_device_cache_key") + def test_link_action_uses_non_default_server_key(self, mock_cache_key, mock_cache): + """Link action should store librenms_id under the active server_key, not always 'default'.""" + from netbox_librenms_plugin.views.imports.actions import DeviceConflictActionView + + view = self._create_view() + view._librenms_api.server_key = "production" + existing_device = MagicMock() + existing_device.pk = 42 + existing_device.custom_field_data = {} + existing_device.name = "84.116.251.35" + + libre_device = { + "device_id": 10, + "hostname": "84.116.251.35", + "sysName": "switch-01.example.com", + "serial": "ABC123", + } + validation = {"can_import": False, "existing_device": existing_device} + selections = {} + + request = self._create_request("link", 42, use_sysname=True) + + with ( + patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, + patch.object(DeviceConflictActionView, "render_device_row") as mock_render, + patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, + ): + mock_tx.atomic.return_value = MagicMock() + mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.filter.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False + mock_validate.return_value = (libre_device, validation, selections) + mock_render.return_value = MagicMock() + + view.post(request, device_id=10) + + assert existing_device.custom_field_data["librenms_id"] == {"production": 10} + @patch("netbox_librenms_plugin.views.imports.actions.cache") @patch("netbox_librenms_plugin.views.imports.actions.get_import_device_cache_key") def test_update_action_sets_hostname_serial_and_librenms_id(self, mock_cache_key, mock_cache): @@ -2192,19 +2258,72 @@ def test_update_action_sets_hostname_serial_and_librenms_id(self, mock_cache_key patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, patch.object(DeviceConflictActionView, "render_device_row") as mock_render, patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, ): + mock_tx.atomic.return_value = MagicMock() mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.first.return_value = None mock_device_cls.objects.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() view.post(request, device_id=10) - assert existing_device.custom_field_data["librenms_id"] == 10 + assert existing_device.custom_field_data["librenms_id"] == {"default": 10} assert existing_device.serial == "NEW-SERIAL" assert existing_device.name == "new-name.example.com" existing_device.save.assert_called_once() + @patch("netbox_librenms_plugin.views.imports.actions.cache") + @patch("netbox_librenms_plugin.views.imports.actions.get_import_device_cache_key") + def test_update_action_uses_non_default_server_key(self, mock_cache_key, mock_cache): + """Update action should store librenms_id under the active server key.""" + from netbox_librenms_plugin.views.imports.actions import DeviceConflictActionView + + view = self._create_view() + view._librenms_api = MagicMock() + view._librenms_api.server_key = "production" + existing_device = MagicMock() + existing_device.pk = 42 + existing_device.custom_field_data = {} + existing_device.name = "old-name" + existing_device.serial = "OLD-SERIAL" + + libre_device = { + "device_id": 10, + "hostname": "switch-01", + "sysName": "switch-01", + "serial": "NEW-SERIAL", + "resolved_name": "switch-01", + } + validation = {"can_import": False, "existing_device": existing_device, "resolved_name": "switch-01"} + selections = {} + + request = self._create_request("update", 42) + + with ( + patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, + patch.object(DeviceConflictActionView, "render_device_row") as mock_render, + patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, + ): + mock_tx.atomic.return_value = MagicMock() + mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False + mock_validate.return_value = (libre_device, validation, selections) + mock_render.return_value = MagicMock() + + view.post(request, device_id=10) + + assert existing_device.custom_field_data["librenms_id"] == {"production": 10} + @patch("netbox_librenms_plugin.views.imports.actions.cache") @patch("netbox_librenms_plugin.views.imports.actions.get_import_device_cache_key") def test_update_serial_action_updates_serial_only(self, mock_cache_key, mock_cache): @@ -2228,15 +2347,21 @@ def test_update_serial_action_updates_serial_only(self, mock_cache_key, mock_cac patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, patch.object(DeviceConflictActionView, "render_device_row") as mock_render, patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, ): + mock_tx.atomic.return_value = MagicMock() mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.first.return_value = None mock_device_cls.objects.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() view.post(request, device_id=10) - assert existing_device.custom_field_data["librenms_id"] == 10 + assert existing_device.custom_field_data["librenms_id"] == {"default": 10} assert existing_device.serial == "NEW-SERIAL" # Name should NOT be changed by update_serial assert existing_device.name == "switch-01" @@ -2265,8 +2390,13 @@ def test_update_skips_dash_serial(self, mock_cache_key, mock_cache): patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, patch.object(DeviceConflictActionView, "render_device_row") as mock_render, patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, ): + mock_tx.atomic.return_value = MagicMock() mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() @@ -2292,14 +2422,20 @@ def test_unknown_action_returns_400(self): request = self._create_request("invalid_action", 42) existing_device = MagicMock() + existing_device.pk = 42 libre_device = {"device_id": 10, "hostname": "switch-01", "serial": "ABC"} with ( patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, + patch.object(DeviceConflictActionView, "require_object_permissions", return_value=None), patch("dcim.models.Device") as mock_device_cls, ): mock_device_cls.objects.get.return_value = existing_device - mock_validate.return_value = (libre_device, {}, {}) + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + # Include existing_device so the validated-conflict-target guard passes; + # we want to exercise the unknown-action branch, not the missing-device guard. + mock_validate.return_value = (libre_device, {"existing_device": existing_device}, {}) response = view.post(request, device_id=10) @@ -2334,6 +2470,8 @@ def test_sync_name_action_updates_name(self, mock_cache_key, mock_cache): patch("dcim.models.Device") as mock_device_cls, ): mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() @@ -2341,6 +2479,7 @@ def test_sync_name_action_updates_name(self, mock_cache_key, mock_cache): assert existing_device.name == "switch-01.example.com" existing_device.save.assert_called_once() + assert existing_device.custom_field_data["librenms_id"] == 10 @patch("netbox_librenms_plugin.views.imports.actions.cache") @patch("netbox_librenms_plugin.views.imports.actions.get_import_device_cache_key") @@ -2364,6 +2503,8 @@ def test_device_type_mismatch_blocked_without_force(self, mock_cache_key, mock_c patch("dcim.models.Device") as mock_device_cls, ): mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None mock_validate.return_value = (libre_device, validation, selections) response = view.post(request, device_id=10) @@ -2399,15 +2540,21 @@ def test_device_type_mismatch_allowed_with_force(self, mock_cache_key, mock_cach patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, patch.object(DeviceConflictActionView, "render_device_row") as mock_render, patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, ): + mock_tx.atomic.return_value = MagicMock() mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.first.return_value = None mock_device_cls.objects.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() view.post(request, device_id=10) - assert existing_device.custom_field_data["librenms_id"] == 10 + assert existing_device.custom_field_data["librenms_id"] == {"default": 10} existing_device.save.assert_called_once() @patch("netbox_librenms_plugin.views.imports.actions.cache") @@ -2445,16 +2592,22 @@ def test_force_with_mismatch_updates_device_type(self, mock_cache_key, mock_cach patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, patch.object(DeviceConflictActionView, "render_device_row") as mock_render, patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, ): + mock_tx.atomic.return_value = MagicMock() mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.first.return_value = None mock_device_cls.objects.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() view.post(request, device_id=10) assert existing_device.device_type == librenms_device_type - assert existing_device.custom_field_data["librenms_id"] == 10 + assert existing_device.custom_field_data["librenms_id"] == {"default": 10} existing_device.save.assert_called_once() @patch("netbox_librenms_plugin.views.imports.actions.cache") @@ -2495,6 +2648,8 @@ def test_update_type_action_changes_device_type(self, mock_cache_key, mock_cache patch("dcim.models.Device") as mock_device_cls, ): mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() @@ -2502,6 +2657,7 @@ def test_update_type_action_changes_device_type(self, mock_cache_key, mock_cache assert existing_device.device_type == new_device_type existing_device.save.assert_called_once() + assert existing_device.custom_field_data["librenms_id"] == 10 @patch("netbox_librenms_plugin.views.imports.actions.cache") @patch("netbox_librenms_plugin.views.imports.actions.get_import_device_cache_key") @@ -2522,9 +2678,15 @@ def test_sync_serial_action(self, mock_cache_key, mock_cache): patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, patch.object(DeviceConflictActionView, "render_device_row") as mock_render, patch("dcim.models.Device") as mock_device_cls, + patch("netbox_librenms_plugin.views.imports.actions.transaction") as mock_tx, ): + mock_tx.atomic.return_value = MagicMock() mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.first.return_value = None mock_device_cls.objects.filter.return_value.exclude.return_value.first.return_value = None + mock_device_cls.objects.filter.return_value.exclude.return_value.exists.return_value = False mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() @@ -2553,10 +2715,14 @@ def test_sync_platform_action(self, mock_cache_key, mock_cache): patch.object(DeviceConflictActionView, "get_validated_device_with_selections") as mock_validate, patch.object(DeviceConflictActionView, "render_device_row") as mock_render, patch("dcim.models.Device") as mock_device_cls, - patch("dcim.models.Platform") as mock_platform_cls, + # Patch find_matching_platform at the utility module level — the action imports + # it from netbox_librenms_plugin.utils, so that is the correct seam to mock. + patch("netbox_librenms_plugin.utils.find_matching_platform") as mock_find_platform, ): mock_device_cls.objects.get.return_value = existing_device - mock_platform_cls.objects.get.return_value = mock_platform + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None + mock_find_platform.return_value = {"found": True, "platform": mock_platform, "match_type": "exact"} mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() @@ -2587,6 +2753,8 @@ def test_sync_device_type_action(self, mock_cache_key, mock_cache): patch("netbox_librenms_plugin.utils.match_librenms_hardware_to_device_type") as mock_hw_match, ): mock_device_cls.objects.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.get.return_value = existing_device + mock_device_cls.objects.select_for_update.return_value.filter.return_value.exclude.return_value.first.return_value = None mock_hw_match.return_value = {"matched": True, "device_type": new_device_type} mock_validate.return_value = (libre_device, validation, selections) mock_render.return_value = MagicMock() diff --git a/netbox_librenms_plugin/tests/test_librenms_id.py b/netbox_librenms_plugin/tests/test_librenms_id.py new file mode 100644 index 0000000000..71c0b40992 --- /dev/null +++ b/netbox_librenms_plugin/tests/test_librenms_id.py @@ -0,0 +1,372 @@ +"""Tests for multi-server librenms_id helpers. + +Covers get_librenms_device_id, set_librenms_device_id, find_by_librenms_id, +and migrate_legacy_librenms_id. +""" + +from unittest.mock import MagicMock + + +class TestGetLibreNMSDeviceId: + """Tests for get_librenms_device_id().""" + + def test_returns_none_when_cf_missing(self): + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {} + result = get_librenms_device_id(obj, "default") + assert result is None + + def test_returns_int_for_legacy_bare_integer(self): + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": 42} + result = get_librenms_device_id(obj, "default") + assert result == 42 + + def test_legacy_bare_int_returned_for_any_server_key(self): + """Legacy bare integers are returned as a universal fallback for any server_key. + + Devices imported before multi-server support store a bare integer in + librenms_id. These must remain discoverable regardless of which server is + active, so the bare-int is returned as-is for any server_key. + """ + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": 99} + assert get_librenms_device_id(obj, "default") == 99 + assert get_librenms_device_id(obj, "production") == 99 + assert get_librenms_device_id(obj, "secondary") == 99 + + def test_returns_value_for_matching_server_key(self): + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": {"production": 7, "secondary": 12}} + assert get_librenms_device_id(obj, "production") == 7 + + def test_returns_none_for_missing_server_key_in_dict(self): + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": {"production": 7}} + result = get_librenms_device_id(obj, "secondary") + assert result is None + + def test_returns_none_for_unexpected_type(self): + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": "not-an-int-or-dict"} + result = get_librenms_device_id(obj, "default") + assert result is None + + def test_legacy_string_int_returned_for_any_server_key(self): + """A bare string integer ('42') is coerced and returned for any server_key.""" + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": "42"} + assert get_librenms_device_id(obj, "default") == 42 + assert get_librenms_device_id(obj, "production") == 42 + + def test_returns_none_for_bare_boolean(self): + """bool is a subclass of int; bare True/False must not be treated as a valid ID.""" + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": True} + assert get_librenms_device_id(obj, "default") is None + + obj.cf = {"librenms_id": False} + assert get_librenms_device_id(obj, "default") is None + + def test_returns_none_for_boolean_inside_dict(self): + """Boolean values inside the JSON dict must be rejected.""" + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": {"default": True}} + assert get_librenms_device_id(obj, "default") is None + + def test_default_server_key_is_default(self): + from netbox_librenms_plugin.utils import get_librenms_device_id + + obj = MagicMock() + obj.cf = {"librenms_id": {"default": 5}} + assert get_librenms_device_id(obj) == 5 + + +class TestFindByLibreNMSId: + """Tests for find_by_librenms_id().""" + + def test_queries_server_key_and_legacy_integer(self): + """find_by_librenms_id() issues a Q that covers both the JSON server-key branch + and the legacy bare-int branch in a single filter() call. + + We inspect the Q object's children directly because the two branches must + coexist — matching only one would silently miss devices stored in the other + format. + """ + from unittest.mock import MagicMock + from django.db.models import Q + from netbox_librenms_plugin.utils import find_by_librenms_id + + mock_model = MagicMock() + mock_qs = MagicMock() + mock_model.objects.filter.return_value = mock_qs + mock_qs.first.return_value = None + + find_by_librenms_id(mock_model, 42, "default") + + mock_model.objects.filter.assert_called_once() + # Verify the Q predicate covers both the server-key JSON branch and legacy bare-int/string branches + call_args = mock_model.objects.filter.call_args + q_arg = call_args[0][0] + assert isinstance(q_arg, Q) + assert q_arg.connector == "OR" + # The combined Q should contain four children: JSON key (int), JSON key (str), bare-int, bare-string + assert len(q_arg.children) == 4 + children_keys = {child[0] for child in q_arg.children} + children_values = {child[1] for child in q_arg.children} + assert "custom_field_data__librenms_id__default" in children_keys + assert "custom_field_data__librenms_id" in children_keys + assert 42 in children_values + assert "42" in children_values + + def test_returns_first_matching_object(self): + from netbox_librenms_plugin.utils import find_by_librenms_id + + expected = MagicMock() + mock_model = MagicMock() + mock_qs = MagicMock() + mock_model.objects.filter.return_value = mock_qs + mock_qs.first.return_value = expected + + result = find_by_librenms_id(mock_model, 42, "default") + assert result is expected + + def test_returns_none_when_not_found(self): + from unittest.mock import MagicMock + from django.db.models import Q + from netbox_librenms_plugin.utils import find_by_librenms_id + + mock_model = MagicMock() + mock_qs = MagicMock() + mock_model.objects.filter.return_value = mock_qs + mock_qs.first.return_value = None + + result = find_by_librenms_id(mock_model, 999, "production") + assert result is None + + # Any server_key must include legacy bare-int/string fallback conditions + # so that devices imported before multi-server support are still found. + call_args = mock_model.objects.filter.call_args + q_arg = call_args[0][0] + assert isinstance(q_arg, Q) + keys = [child[0] for child in q_arg.children] + assert "custom_field_data__librenms_id__production" in keys + assert "custom_field_data__librenms_id" in keys + + def test_default_server_key_is_default(self): + """find_by_librenms_id() uses "default" as the server key when no key is passed. + + We inspect the Q predicate's children to confirm the key embedded in the + JSON path is exactly "default", not some other fallback value. + """ + from unittest.mock import MagicMock + from django.db.models import Q + from netbox_librenms_plugin.utils import find_by_librenms_id + + mock_model = MagicMock() + mock_qs = MagicMock() + mock_model.objects.filter.return_value = mock_qs + mock_qs.first.return_value = None + + find_by_librenms_id(mock_model, 42) + + mock_model.objects.filter.assert_called_once() + call_args = mock_model.objects.filter.call_args + q_arg = call_args[0][0] + assert isinstance(q_arg, Q) + assert q_arg.connector == "OR" + assert len(q_arg.children) == 4 + children_keys = {child[0] for child in q_arg.children} + children_values = {child[1] for child in q_arg.children} + # The JSON-path branch must use "default" as the server key + assert "custom_field_data__librenms_id__default" in children_keys + assert "custom_field_data__librenms_id" in children_keys + assert 42 in children_values + assert "42" in children_values + + +class TestMigrateLegacyLibreNMSId: + """Tests for migrate_legacy_librenms_id().""" + + def test_returns_true_when_migrated(self): + from netbox_librenms_plugin.utils import migrate_legacy_librenms_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": 42} + result = migrate_legacy_librenms_id(obj, "default") + assert result is True + + def test_migrates_integer_to_dict_format(self): + from netbox_librenms_plugin.utils import migrate_legacy_librenms_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": 42} + migrate_legacy_librenms_id(obj, "production") + assert obj.custom_field_data["librenms_id"] == {"production": 42} + + def test_returns_false_when_already_dict(self): + from netbox_librenms_plugin.utils import migrate_legacy_librenms_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": {"default": 42}} + result = migrate_legacy_librenms_id(obj, "default") + assert result is False + + def test_returns_false_when_value_is_none(self): + from netbox_librenms_plugin.utils import migrate_legacy_librenms_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": None} + result = migrate_legacy_librenms_id(obj, "default") + assert result is False + + def test_returns_false_for_boolean_value(self): + """bool is a subclass of int; True/False must not be migrated.""" + from netbox_librenms_plugin.utils import migrate_legacy_librenms_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": True} + assert migrate_legacy_librenms_id(obj, "default") is False + assert obj.custom_field_data["librenms_id"] is True # unchanged + + def test_does_not_call_save(self): + """migrate_legacy_librenms_id must NOT call obj.save() — caller is responsible.""" + from netbox_librenms_plugin.utils import migrate_legacy_librenms_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": 7} + migrate_legacy_librenms_id(obj, "default") + obj.save.assert_not_called() + + def test_preserves_value_in_migrated_dict(self): + from netbox_librenms_plugin.utils import migrate_legacy_librenms_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": 99} + migrate_legacy_librenms_id(obj, "secondary") + assert obj.custom_field_data["librenms_id"]["secondary"] == 99 + + +class TestLibreNMSIdRoundtrip: + """get_librenms_device_id should see the value set by set_librenms_device_id.""" + + def test_set_then_get_returns_same_value(self): + from netbox_librenms_plugin.utils import get_librenms_device_id, set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {} + obj.cf = obj.custom_field_data # make cf a live view of custom_field_data + + set_librenms_device_id(obj, 42, "production") + result = get_librenms_device_id(obj, "production") + assert result == 42 + + def test_set_multiple_servers_get_correct_each(self): + from netbox_librenms_plugin.utils import get_librenms_device_id, set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {} + obj.cf = obj.custom_field_data + + set_librenms_device_id(obj, 10, "primary") + set_librenms_device_id(obj, 20, "secondary") + + assert get_librenms_device_id(obj, "primary") == 10 + assert get_librenms_device_id(obj, "secondary") == 20 + + def test_migrate_then_get_returns_value(self): + from netbox_librenms_plugin.utils import get_librenms_device_id, migrate_legacy_librenms_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": 55} + obj.cf = obj.custom_field_data + + migrate_legacy_librenms_id(obj, "default") + result = get_librenms_device_id(obj, "default") + assert result == 55 + + +class TestSetLibreNMSDeviceId: + """Tests for set_librenms_device_id in utils.py.""" + + def test_stores_int_for_valid_device_id(self): + """Valid integer device_id is stored under server_key.""" + from netbox_librenms_plugin.utils import set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": None} + set_librenms_device_id(obj, 42, server_key="primary") + assert obj.custom_field_data["librenms_id"] == {"primary": 42} + + def test_invalid_device_id_not_stored(self): + """Non-integer device_id is rejected and nothing is written.""" + from netbox_librenms_plugin.utils import set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {} + set_librenms_device_id(obj, "not-an-int", server_key="primary") + assert "librenms_id" not in obj.custom_field_data + + def test_invalid_device_id_does_not_overwrite_existing(self): + """Existing valid value is preserved when new device_id is invalid.""" + from netbox_librenms_plugin.utils import set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": {"primary": 10}} + set_librenms_device_id(obj, None, server_key="primary") + assert obj.custom_field_data["librenms_id"] == {"primary": 10} + + def test_legacy_bare_int_blocks_write(self): + """Legacy bare-integer value blocks the write (no silent migration).""" + from netbox_librenms_plugin.utils import set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": 7} + set_librenms_device_id(obj, 99, server_key="secondary") + # Write must be skipped; user must use the migration workflow. + assert obj.custom_field_data["librenms_id"] == 7 + + def test_adds_new_server_key_to_existing_dict(self): + """Adding a new server key preserves existing keys.""" + from netbox_librenms_plugin.utils import set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": {"primary": 5}} + set_librenms_device_id(obj, 20, server_key="secondary") + assert obj.custom_field_data["librenms_id"] == {"primary": 5, "secondary": 20} + + def test_string_integer_is_coerced(self): + """String '42' is coerced to int 42.""" + from netbox_librenms_plugin.utils import set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {} + set_librenms_device_id(obj, "42", server_key="primary") + assert obj.custom_field_data["librenms_id"] == {"primary": 42} + + def test_unexpected_cf_type_reset_to_empty(self): + """If custom_field_data has unexpected type for librenms_id, it is reset.""" + from netbox_librenms_plugin.utils import set_librenms_device_id + + obj = MagicMock() + obj.custom_field_data = {"librenms_id": "unexpected-string"} + set_librenms_device_id(obj, 5, server_key="primary") + assert obj.custom_field_data["librenms_id"] == {"primary": 5} diff --git a/netbox_librenms_plugin/tests/test_mixins.py b/netbox_librenms_plugin/tests/test_mixins.py index fb40e690ae..de5cd8f245 100644 --- a/netbox_librenms_plugin/tests/test_mixins.py +++ b/netbox_librenms_plugin/tests/test_mixins.py @@ -110,6 +110,21 @@ def test_get_cache_key_format(self): key = mixin.get_cache_key(obj, "ports") assert key == "librenms_ports_device_5" + def test_get_cache_key_includes_server_key(self): + """Cache keys must be namespaced per server so two servers' data never collide. + + Without server_key isolation a second server's stale ports list could be + returned to the wrong sync session. + """ + mixin = self._make_mixin() + obj = MagicMock() + obj._meta.model_name = "device" + obj.pk = 5 + + key = mixin.get_cache_key(obj, "ports", server_key="srv1") + assert "srv1" in key + assert key == "librenms_ports_device_5_srv1" + def test_get_cache_key_includes_model_name(self): mixin = self._make_mixin() obj = MagicMock() @@ -120,6 +135,16 @@ def test_get_cache_key_includes_model_name(self): assert "virtualmachine" in key assert "10" in key + def test_get_cache_key_different_data_types(self): + mixin = self._make_mixin() + obj = MagicMock() + obj._meta.model_name = "device" + obj.pk = 1 + + key_ports = mixin.get_cache_key(obj, "ports", server_key="prod") + key_ips = mixin.get_cache_key(obj, "ips", server_key="prod") + assert key_ports != key_ips + def test_get_last_fetched_key_format(self): mixin = self._make_mixin() obj = MagicMock() @@ -129,6 +154,20 @@ def test_get_last_fetched_key_format(self): key = mixin.get_last_fetched_key(obj, "ports") assert key == "librenms_ports_last_fetched_device_3" # exact string + def test_get_last_fetched_key_includes_server_key(self): + """The last-fetched timestamp key must also be server-scoped. + + If two servers share the same key the cache countdown would reflect the + wrong server's fetch time. + """ + mixin = self._make_mixin() + obj = MagicMock() + obj._meta.model_name = "device" + obj.pk = 3 + + key = mixin.get_last_fetched_key(obj, "ports", server_key="srv1") + assert key == "librenms_ports_last_fetched_device_3_srv1" # exact string + def test_cache_key_different_pks_differ(self): mixin = self._make_mixin() obj1 = MagicMock() @@ -152,3 +191,15 @@ def test_get_vlan_overrides_key_exists_and_differs_from_data_key(self): assert vlan_key == "librenms_vlan_group_overrides_device_7" data_key = mixin.get_cache_key(obj, "vlans") assert vlan_key != data_key + + def test_get_vlan_overrides_key_server_scoped(self): + """VLAN overrides key includes server_key to avoid cross-server leakage.""" + mixin = self._make_mixin() + obj = MagicMock() + obj._meta.model_name = "device" + obj.pk = 7 + + key_no_server = mixin.get_vlan_overrides_key(obj) + key_with_server = mixin.get_vlan_overrides_key(obj, server_key="prod") + assert key_with_server == "librenms_vlan_group_overrides_device_7_prod" + assert key_no_server != key_with_server diff --git a/netbox_librenms_plugin/tests/test_sync_devices.py b/netbox_librenms_plugin/tests/test_sync_devices.py index e877a9b0c1..0804a424d9 100644 --- a/netbox_librenms_plugin/tests/test_sync_devices.py +++ b/netbox_librenms_plugin/tests/test_sync_devices.py @@ -213,3 +213,61 @@ def test_requires_change_device_permission(self): perms = UpdateDeviceSerialView.required_object_permissions assert "POST" in perms assert any(action == "change" and model == Device for action, model in perms["POST"]) + + +class TestRemoveServerMappingViewWiring: + def test_does_not_have_librenms_api_mixin(self): + """RemoveServerMappingView does not call LibreNMS API — it only modifies NetBox.""" + from netbox_librenms_plugin.views.sync.device_fields import RemoveServerMappingView + from netbox_librenms_plugin.views.mixins import LibreNMSAPIMixin + + assert LibreNMSAPIMixin not in RemoveServerMappingView.__mro__ + + def test_has_permission_mixin(self): + from netbox_librenms_plugin.views.sync.device_fields import RemoveServerMappingView + from netbox_librenms_plugin.views.mixins import LibreNMSPermissionMixin, NetBoxObjectPermissionMixin + + assert LibreNMSPermissionMixin in RemoveServerMappingView.__mro__ + assert NetBoxObjectPermissionMixin in RemoveServerMappingView.__mro__ + + def test_post_with_virtualmachine_sets_vm_permissions_and_redirects(self): + """post() with object_type='virtualmachine' sets VirtualMachine permissions and redirects to VM URL.""" + from netbox_librenms_plugin.views.sync.device_fields import RemoveServerMappingView + from virtualization.models import VirtualMachine + + view = object.__new__(RemoveServerMappingView) + + permissions_at_check = {} + + def capture_perms(method): + permissions_at_check[method] = list(view.required_object_permissions.get(method, [])) + return None # permission passes + + mock_vm = MagicMock() + mock_vm.pk = 10 + mock_vm.custom_field_data = {"librenms_id": {"orphaned-server": 42}} + + # Use a mock model class so the select_for_update().get() call doesn't hit the DB + mock_model = MagicMock() + mock_model.objects.select_for_update.return_value.get.return_value = mock_vm + + request = MagicMock() + request.POST = {"object_type": "virtualmachine", "server_key": "orphaned-server"} + + with ( + patch.object(view, "require_all_permissions", side_effect=capture_perms), + patch.object(view, "_get_object", return_value=(mock_vm, mock_model)), + patch("netbox_librenms_plugin.views.sync.device_fields.messages"), + patch("netbox_librenms_plugin.views.sync.device_fields.redirect") as mock_redirect, + patch("netbox_librenms_plugin.views.sync.device_fields.transaction"), + patch( + "django.conf.settings", + PLUGINS_CONFIG={"netbox_librenms_plugin": {}}, + ), + ): + view.post(request, pk=10) + + # required_object_permissions must be scoped to VirtualMachine, not Device + assert ("change", VirtualMachine) in permissions_at_check.get("POST", []) + # Response must redirect to the VM-specific sync URL + mock_redirect.assert_called_with("plugins:netbox_librenms_plugin:vm_librenms_sync", pk=10) diff --git a/netbox_librenms_plugin/tests/test_sync_interfaces.py b/netbox_librenms_plugin/tests/test_sync_interfaces.py new file mode 100644 index 0000000000..ea13627e12 --- /dev/null +++ b/netbox_librenms_plugin/tests/test_sync_interfaces.py @@ -0,0 +1,297 @@ +"""Unit tests for SyncInterfacesView: update_interface_attributes and handle_mac_address.""" + +import pytest +from unittest.mock import MagicMock, patch + + +class TestUpdateInterfaceAttributes: + """update_interface_attributes() must set fields respecting exclude_columns.""" + + @pytest.fixture + def view(self, mock_librenms_api): + from netbox_librenms_plugin.views.sync.interfaces import SyncInterfacesView + + v = object.__new__(SyncInterfacesView) + v._librenms_api = mock_librenms_api + v.request = MagicMock() + v._lookup_maps = {} + return v + + def _make_device_interface(self, **extra): + """Return a MagicMock mimicking a dcim.Interface.""" + from dcim.models import Interface # noqa: F401 + + iface = MagicMock( + spec=[ + "name", + "type", + "speed", + "description", + "mtu", + "enabled", + "save", + "cf", + "custom_field_data", + "mac_addresses", + "primary_mac_address", + ] + ) + iface.cf = {"librenms_id": {"default": 1}} + iface.__class__ = Interface + for k, v in extra.items(): + setattr(iface, k, v) + return iface + + def test_sets_speed_via_convert(self, view): + iface = self._make_device_interface() + librenms_data = {"ifName": "eth0", "ifSpeed": 1_000_000_000} + + with patch( + "netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=1_000_000 + ) as mock_convert: + with patch("netbox_librenms_plugin.views.sync.interfaces.set_librenms_device_id"): + view.update_interface_attributes(iface, librenms_data, "1000base-t", set(), "ifName") + + mock_convert.assert_called_once_with(1_000_000_000) + assert iface.speed == 1_000_000 + + def test_skips_excluded_columns(self, view): + speed_sentinel = object() + iface = self._make_device_interface(speed=speed_sentinel) + librenms_data = {"ifName": "eth0", "ifSpeed": 1_000_000_000, "ifAlias": "uplink"} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=1_000_000): + with patch("netbox_librenms_plugin.views.sync.interfaces.set_librenms_device_id"): + view.update_interface_attributes(iface, librenms_data, "1000base-t", {"speed"}, "ifName") + + # speed should NOT have been mutated (excluded) + assert iface.speed is speed_sentinel + + def test_sets_type_for_device_interface(self, view): + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {} + iface.mac_addresses = MagicMock() + librenms_data = {"ifName": "eth0", "ifType": "ethernetCsmacd"} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + view.update_interface_attributes(iface, librenms_data, "1000base-t", set(), "ifName") + + assert iface.type == "1000base-t" + + def test_does_not_set_type_for_vm_interface(self, view): + from virtualization.models import VMInterface + + iface = MagicMock() + iface.__class__ = VMInterface + iface.cf = {} + iface.mac_addresses = MagicMock() + original_type = "some_type" + iface.type = original_type + librenms_data = {"ifName": "eth0", "ifType": "ethernetCsmacd"} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + view.update_interface_attributes(iface, librenms_data, "1000base-t", set(), "ifName") + + # type is NOT in the mapping for non-device interfaces (type set only if is_device_interface) + assert iface.type == original_type + + def test_sets_description_only_when_alias_differs_from_name(self, view): + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {} + iface.mac_addresses = MagicMock() + + # ifAlias == interface name field value → description should NOT be set + librenms_data = {"ifName": "eth0", "ifAlias": "eth0"} + desc_sentinel = object() + iface.description = desc_sentinel + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + view.update_interface_attributes(iface, librenms_data, None, {"type", "speed", "mtu"}, "ifName") + + assert iface.description is desc_sentinel # untouched: alias == name, no update + + def test_sets_description_when_alias_differs(self, view): + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {} + iface.mac_addresses = MagicMock() + + librenms_data = {"ifName": "eth0", "ifAlias": "uplink-port"} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + view.update_interface_attributes(iface, librenms_data, None, {"type", "speed", "mtu"}, "ifName") + + assert iface.description == "uplink-port" + + def test_sets_librenms_id_when_port_id_present(self, view): + """set_librenms_device_id() is called unconditionally when port_id is not None. + + Previously the call was guarded by ``"librenms_id" in interface.cf``, which + prevented the mapping from being created for brand-new interfaces. The fix + (this PR) drops that guard so first-time writes are handled correctly. + """ + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {} # empty — first-time write, no existing mapping + iface.mac_addresses = MagicMock() + librenms_data = {"ifName": "eth0", "port_id": 77} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + with patch("netbox_librenms_plugin.views.sync.interfaces.set_librenms_device_id") as mock_set: + view.update_interface_attributes(iface, librenms_data, None, {"type", "speed", "mtu"}, "ifName") + + mock_set.assert_called_once_with(iface, 77, view._librenms_api.server_key) + + def test_does_not_set_librenms_id_when_port_id_none(self, view): + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {"librenms_id": {"default": 1}} + iface.mac_addresses = MagicMock() + librenms_data = {"ifName": "eth0", "port_id": None} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + with patch("netbox_librenms_plugin.views.sync.interfaces.set_librenms_device_id") as mock_set: + view.update_interface_attributes(iface, librenms_data, None, {"type", "speed", "mtu"}, "ifName") + + mock_set.assert_not_called() + + def test_sets_enabled_true_when_admin_status_none(self, view): + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {} + iface.mac_addresses = MagicMock() + librenms_data = {"ifName": "eth0", "ifAdminStatus": None} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + view.update_interface_attributes(iface, librenms_data, None, {"type", "speed", "mtu"}, "ifName") + + assert iface.enabled is True + + def test_sets_enabled_based_on_admin_status_string(self, view): + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {} + iface.mac_addresses = MagicMock() + librenms_data = {"ifName": "eth0", "ifAdminStatus": "down"} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + view.update_interface_attributes(iface, librenms_data, None, {"type", "speed", "mtu"}, "ifName") + + assert iface.enabled is False + + def test_calls_save_at_end(self, view): + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {} + iface.mac_addresses = MagicMock() + librenms_data = {"ifName": "eth0"} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + view.update_interface_attributes(iface, librenms_data, None, {"type", "speed", "mtu"}, "ifName") + + iface.save.assert_called_once() + + def test_excludes_mac_address_when_in_excluded(self, view): + from dcim.models import Interface + + iface = MagicMock() + iface.__class__ = Interface + iface.cf = {} + iface.mac_addresses = MagicMock() + librenms_data = {"ifName": "eth0", "ifPhysAddress": "aa:bb:cc:dd:ee:ff"} + + with patch("netbox_librenms_plugin.views.sync.interfaces.convert_speed_to_kbps", return_value=None): + with patch.object(view, "handle_mac_address") as mock_mac: + view.update_interface_attributes(iface, librenms_data, None, {"mac_address"}, "ifName") + + mock_mac.assert_not_called() + + +class TestHandleMacAddress: + """handle_mac_address() must work for both Interface (has primary_mac_address) + and VMInterface (does not have primary_mac_address).""" + + @pytest.fixture + def view(self, mock_librenms_api): + from netbox_librenms_plugin.views.sync.interfaces import SyncInterfacesView + + v = object.__new__(SyncInterfacesView) + v._librenms_api = mock_librenms_api + v.request = MagicMock() + v._lookup_maps = {} + return v + + def test_creates_new_mac_and_adds_to_interface(self, view): + iface = MagicMock() + iface.mac_addresses = MagicMock() + iface.mac_addresses.filter.return_value.first.return_value = None + new_mac = MagicMock() + + with patch("netbox_librenms_plugin.views.sync.interfaces.MACAddress") as mock_cls: + mock_cls.objects.create.return_value = new_mac + view.handle_mac_address(iface, "aa:bb:cc:dd:ee:ff") + + mock_cls.objects.create.assert_called_once_with(mac_address="aa:bb:cc:dd:ee:ff") + iface.mac_addresses.add.assert_called_once_with(new_mac) + + def test_reuses_existing_mac(self, view): + existing_mac = MagicMock() + iface = MagicMock() + iface.mac_addresses = MagicMock() + iface.mac_addresses.filter.return_value.first.return_value = existing_mac + + with patch("netbox_librenms_plugin.views.sync.interfaces.MACAddress") as mock_cls: + view.handle_mac_address(iface, "aa:bb:cc:dd:ee:ff") + + mock_cls.objects.create.assert_not_called() + iface.mac_addresses.add.assert_called_once_with(existing_mac) + + def test_sets_primary_mac_when_attribute_present(self, view): + mac_obj = MagicMock() + iface = MagicMock(spec=["mac_addresses", "primary_mac_address"]) + iface.mac_addresses = MagicMock() + iface.mac_addresses.filter.return_value.first.return_value = None + + with patch("netbox_librenms_plugin.views.sync.interfaces.MACAddress") as mock_cls: + mock_cls.objects.create.return_value = mac_obj + view.handle_mac_address(iface, "aa:bb:cc:dd:ee:ff") + + assert iface.primary_mac_address is mac_obj + + def test_no_error_when_primary_mac_attribute_absent(self, view): + """VMInterface does not have primary_mac_address — handle_mac_address must not raise.""" + mac_obj = MagicMock() + iface = MagicMock(spec=["mac_addresses"]) # no primary_mac_address attr + iface.mac_addresses = MagicMock() + iface.mac_addresses.filter.return_value.first.return_value = None + + with patch("netbox_librenms_plugin.views.sync.interfaces.MACAddress") as mock_cls: + mock_cls.objects.create.return_value = mac_obj + # Must not raise AttributeError + view.handle_mac_address(iface, "aa:bb:cc:dd:ee:ff") + + def test_noop_when_mac_address_is_falsy(self, view): + iface = MagicMock() + with patch("netbox_librenms_plugin.views.sync.interfaces.MACAddress") as mock_cls: + view.handle_mac_address(iface, "") + view.handle_mac_address(iface, None) + + mock_cls.objects.create.assert_not_called() diff --git a/netbox_librenms_plugin/tests/test_sync_view_mismatch.py b/netbox_librenms_plugin/tests/test_sync_view_mismatch.py index 55ca65e592..d4a4336ae0 100644 --- a/netbox_librenms_plugin/tests/test_sync_view_mismatch.py +++ b/netbox_librenms_plugin/tests/test_sync_view_mismatch.py @@ -341,7 +341,7 @@ def test_vc_member_with_own_id_delegates_to_sync_device(self, mock_sync_device, # Viewed device: member A with its own librenms_id member_a = MagicMock() member_a.pk = 1 - member_a.cf = {"librenms_id": 42} + member_a.cf = {"librenms_id": {"default": 42}} member_a.virtual_chassis = MagicMock() # Sync device: member B (returned by get_librenms_sync_device) @@ -354,6 +354,7 @@ def test_vc_member_with_own_id_delegates_to_sync_device(self, mock_sync_device, view = object.__new__(BaseLibreNMSSyncView) view.model = MagicMock() api = MagicMock() + api.server_key = "default" api.get_librenms_id.return_value = 42 view._librenms_api = api view.tab = MagicMock() @@ -364,7 +365,7 @@ def test_vc_member_with_own_id_delegates_to_sync_device(self, mock_sync_device, view.get(request, pk=1) # get_librenms_sync_device must be called unconditionally for VC members - mock_sync_device.assert_called_once_with(member_a) + mock_sync_device.assert_called_once_with(member_a, server_key="default") # get_librenms_id should be called on the sync device (member_b) api.get_librenms_id.assert_called_once_with(member_b) @@ -385,6 +386,7 @@ def test_non_vc_device_skips_sync_device_lookup(self, mock_sync_device, mock_get view = object.__new__(BaseLibreNMSSyncView) view.model = MagicMock() api = MagicMock() + api.server_key = "default" api.get_librenms_id.return_value = 42 view._librenms_api = api view.tab = MagicMock() @@ -395,3 +397,88 @@ def test_non_vc_device_skips_sync_device_lookup(self, mock_sync_device, mock_get view.get(request, pk=1) mock_sync_device.assert_not_called() + + +# --------------------------------------------------------------------------- +# Tests for _build_all_server_mappings +# --------------------------------------------------------------------------- + + +class TestBuildAllServerMappings: + """Tests for BaseLibreNMSSyncView._build_all_server_mappings.""" + + def test_returns_none_for_legacy_int(self, mock_netbox_device): + from netbox_librenms_plugin.views.base.librenms_sync_view import BaseLibreNMSSyncView + + mock_netbox_device.custom_field_data = {"librenms_id": 42} + result = BaseLibreNMSSyncView._build_all_server_mappings(mock_netbox_device, "production") + assert result is None + + def test_returns_none_for_missing_cf(self, mock_netbox_device): + from netbox_librenms_plugin.views.base.librenms_sync_view import BaseLibreNMSSyncView + + mock_netbox_device.custom_field_data = {"librenms_id": None} + result = BaseLibreNMSSyncView._build_all_server_mappings(mock_netbox_device, "production") + assert result is None + + def test_single_configured_server(self, mock_netbox_device, mock_plugins_config_single_server): + from unittest.mock import patch + + from netbox_librenms_plugin.views.base.librenms_sync_view import BaseLibreNMSSyncView + + mock_netbox_device.custom_field_data = {"librenms_id": {"production": 42}} + with patch("netbox_librenms_plugin.views.base.librenms_sync_view.django_settings") as mock_settings: + mock_settings.PLUGINS_CONFIG = mock_plugins_config_single_server + result = BaseLibreNMSSyncView._build_all_server_mappings(mock_netbox_device, "production") + + assert result is not None + assert len(result) == 1 + entry = result[0] + assert entry["server_key"] == "production" + assert entry["device_id"] == 42 + assert entry["display_name"] == "Production LibreNMS" + assert entry["is_configured"] is True + assert entry["is_active"] is True + assert entry["device_url"] == "https://librenms.example.com/device/device=42/" + + def test_orphaned_server_is_not_configured(self, mock_netbox_device, mock_plugins_config_empty_servers): + from unittest.mock import patch + + from netbox_librenms_plugin.views.base.librenms_sync_view import BaseLibreNMSSyncView + + mock_netbox_device.custom_field_data = {"librenms_id": {"deleted-server": 77}} + with patch("netbox_librenms_plugin.views.base.librenms_sync_view.django_settings") as mock_settings: + mock_settings.PLUGINS_CONFIG = mock_plugins_config_empty_servers + result = BaseLibreNMSSyncView._build_all_server_mappings(mock_netbox_device, "production") + + assert result is not None + assert len(result) == 1 + entry = result[0] + assert entry["server_key"] == "deleted-server" + assert entry["device_id"] == 77 + assert entry["is_configured"] is False + assert entry["is_active"] is False + assert entry["device_url"] is None + + def test_multiple_servers_sorted_active_first(self, mock_netbox_device, mock_plugins_config_multi_server_mapping): + from unittest.mock import patch + + from netbox_librenms_plugin.views.base.librenms_sync_view import BaseLibreNMSSyncView + + mock_netbox_device.custom_field_data = {"librenms_id": {"mock-dev": 99, "production": 42, "old-server": 11}} + with patch("netbox_librenms_plugin.views.base.librenms_sync_view.django_settings") as mock_settings: + mock_settings.PLUGINS_CONFIG = mock_plugins_config_multi_server_mapping + result = BaseLibreNMSSyncView._build_all_server_mappings(mock_netbox_device, "production") + + assert result is not None + assert len(result) == 3 + # Active (production) first + assert result[0]["server_key"] == "production" + assert result[0]["is_active"] is True + # Configured (mock-dev) second + assert result[1]["server_key"] == "mock-dev" + assert result[1]["is_configured"] is True + assert result[1]["is_active"] is False + # Orphaned last + assert result[2]["server_key"] == "old-server" + assert result[2]["is_configured"] is False diff --git a/netbox_librenms_plugin/tests/test_view_wiring.py b/netbox_librenms_plugin/tests/test_view_wiring.py index 0c55d86228..f62339d3a4 100644 --- a/netbox_librenms_plugin/tests/test_view_wiring.py +++ b/netbox_librenms_plugin/tests/test_view_wiring.py @@ -63,6 +63,11 @@ def test_assign_vc_serial_has_librenms_api_mixin(self): self._assert_has_api_mixin(AssignVCSerialView) + def test_convert_legacy_id_has_librenms_api_mixin(self): + from netbox_librenms_plugin.views.sync.device_fields import ConvertLegacyLibreNMSIdView + + self._assert_has_api_mixin(ConvertLegacyLibreNMSIdView) + class TestCacheMixinWiring: """Views that cache LibreNMS data must have CacheMixin and expose get_cache_key.""" @@ -192,6 +197,18 @@ def test_update_device_serial_has_required_object_permissions(self): self._assert_has_mixins(UpdateDeviceSerialView) assert "POST" in UpdateDeviceSerialView.required_object_permissions + def test_remove_server_mapping_has_required_object_permissions(self): + from netbox_librenms_plugin.views.sync.device_fields import RemoveServerMappingView + + self._assert_has_mixins(RemoveServerMappingView) + assert "POST" in RemoveServerMappingView.required_object_permissions + + def test_convert_legacy_id_has_required_object_permissions(self): + from netbox_librenms_plugin.views.sync.device_fields import ConvertLegacyLibreNMSIdView + + self._assert_has_mixins(ConvertLegacyLibreNMSIdView) + assert "POST" in ConvertLegacyLibreNMSIdView.required_object_permissions + def test_delete_interfaces_has_required_object_permissions(self): from dcim.models import Interface from virtualization.models import VMInterface @@ -253,17 +270,6 @@ def test_sync_interfaces_has_librenms_api_property_via_class(self): class TestTemplateSyntax: """Compile every plugin template to catch syntax errors early.""" - @pytest.fixture(autouse=True, scope="class") - def _django_engine(self): - """Ensure Django is set up once and expose the template engine.""" - os.environ.setdefault("DJANGO_SETTINGS_MODULE", "netbox.settings") - import django - - django.setup() - from django.template import engines - - self.__class__._engine = engines["django"] - @pytest.mark.parametrize( "template_path", _TEMPLATE_FILES, @@ -271,6 +277,13 @@ def _django_engine(self): ) def test_template_compiles(self, template_path): """Each template must parse without TemplateSyntaxError.""" + os.environ.setdefault("DJANGO_SETTINGS_MODULE", "netbox.settings") + import django + + django.setup() + from django.template import engines + + engine = engines["django"] source = template_path.read_text() # Compile the template — raises TemplateSyntaxError on bad tags - self._engine.from_string(source) + engine.from_string(source) diff --git a/netbox_librenms_plugin/urls.py b/netbox_librenms_plugin/urls.py index 9eafdb1dca..7634e130b8 100644 --- a/netbox_librenms_plugin/urls.py +++ b/netbox_librenms_plugin/urls.py @@ -30,6 +30,7 @@ InterfaceTypeMappingView, LibreNMSImportView, LibreNMSSettingsView, + RemoveServerMappingView, SaveUserPrefView, SingleCableVerifyView, SingleInterfaceVerifyView, @@ -52,6 +53,7 @@ VMIPAddressTableView, VMLibreNMSSyncView, VMStatusListView, + ConvertLegacyLibreNMSIdView, ) urlpatterns = [ @@ -221,6 +223,16 @@ AssignVCSerialView.as_view(), name="assign_vc_serial", ), + path( + "devices//remove-server-mapping/", + RemoveServerMappingView.as_view(), + name="remove_server_mapping", + ), + path( + "devices//convert-legacy-id/", + ConvertLegacyLibreNMSIdView.as_view(), + name="convert_legacy_librenms_id", + ), path( "device-status/", DeviceStatusListView.as_view(), diff --git a/netbox_librenms_plugin/utils.py b/netbox_librenms_plugin/utils.py index 00dd425a01..12b26aa6d7 100644 --- a/netbox_librenms_plugin/utils.py +++ b/netbox_librenms_plugin/utils.py @@ -4,6 +4,7 @@ from dcim.models import Device from django.core.exceptions import ObjectDoesNotExist +from django.db.models import Q from django.http import HttpRequest from netbox.config import get_config from netbox.plugins import get_plugin_config @@ -76,7 +77,7 @@ def get_virtual_chassis_member(device: Device, port_name: str) -> Device: return device -def get_librenms_sync_device(device: Device) -> Optional[Device]: +def get_librenms_sync_device(device: Device, server_key: str = "default") -> Optional[Device]: """ Determine which Virtual Chassis member should handle LibreNMS sync operations. @@ -84,13 +85,14 @@ def get_librenms_sync_device(device: Device) -> Optional[Device]: should have the librenms_id custom field set and be used for sync operations. Priority order for selecting the sync device: - 1. Any member with librenms_id custom field set (highest priority - already configured) + 1. Any member with librenms_id custom field set for *server_key* (highest priority) 2. Master device with primary IP (if master is designated) 3. Any member with primary IP (fallback when no master or master lacks IP) 4. Member with lowest vc_position (for error messages when no IPs configured) Args: device (Device): Any device in the virtual chassis. + server_key: LibreNMS server key used to resolve the correct librenms_id mapping. Returns: Optional[Device]: The device that should handle LibreNMS sync, or None if @@ -102,12 +104,19 @@ def get_librenms_sync_device(device: Device) -> Optional[Device]: vc = device.virtual_chassis all_members = vc.members.all() - # Priority 1: Check if ANY member has librenms_id configured. - # LibreNMS device IDs are auto-incremented from 1 (MySQL default), so 0 is never valid. - # Using a truthy check safely excludes 0, None, "", and other falsy non-IDs. + # Priority 1: Prefer member with an explicit per-server dict mapping for server_key. + # This ensures a migrated device is preferred over one with a legacy bare-int ID. for member in all_members: raw_cf = member.cf.get("librenms_id") - if raw_cf and not isinstance(raw_cf, bool): + if isinstance(raw_cf, dict): + val = raw_cf.get(server_key) + if val is not None and not isinstance(val, bool): + return member + + # Priority 2 (legacy fallback): Any member whose librenms_id resolves for this server + # (includes bare-int legacy IDs that are a universal fallback). + for member in all_members: + if get_librenms_device_id(member, server_key, auto_save=False): return member # Priority 2: Use master device if it has primary IP @@ -453,3 +462,189 @@ def check_vlan_group_matches( netbox_gid = netbox_tagged_group_ids.get(vid) return netbox_gid == selected_group_id return True + + +def get_librenms_device_id(obj, server_key: str = "default", *, auto_save: bool = True): + """ + Get the LibreNMS device/port ID for a specific server from the JSON custom field. + + Supports both the legacy integer format and the new multi-server JSON format:: + + Legacy: librenms_id = 42 → returned as universal fallback for any server_key + New: librenms_id = {"primary": 42} → returns 42 only for server_key="primary" + Legacy: librenms_id = 42 → returns 42 for any server_key (universal fallback) + New: librenms_id = {"primary": 42} → returns 42 only for server_key="primary" + + If the stored value (or the dict entry for server_key) is a string it is + normalised to ``int``. When *auto_save* is ``True`` (the default) the + normalised value is written back so that subsequent DB queries can use a + plain integer without defensive ``str()`` casting. Pass ``auto_save=False`` + in read-only contexts (e.g. table renderers) to avoid triggering unintended + DB writes or signals. + + Args: + obj: NetBox object with a ``librenms_id`` custom field. + server_key: LibreNMS server key (from plugin ``servers`` config). + auto_save: When True (default), persist any normalised value back to the DB. + + Returns: + int or None + """ + cf_value = obj.cf.get("librenms_id") + if cf_value is None: + return None + if isinstance(cf_value, int) and not isinstance(cf_value, bool): + # Legacy bare integer — universal fallback for any server to ensure + # devices imported before multi-server support remain discoverable. + return cf_value + if isinstance(cf_value, str): + # Someone stored a bare string (e.g., via NetBox UI/API) — normalise to int. + # Treated as a legacy universal fallback for any server. + try: + int_id = int(cf_value) + except (ValueError, TypeError): + return None + obj.custom_field_data["librenms_id"] = int_id + if auto_save: + obj.save() + return int_id + if isinstance(cf_value, dict): + value = cf_value.get(server_key) + if isinstance(value, bool): + return None + if isinstance(value, str): + # Normalise string-stored ID inside JSON dict and write back. + try: + value = int(value) + except (ValueError, TypeError): + return None + cf_value[server_key] = value + obj.custom_field_data["librenms_id"] = cf_value + if auto_save: + obj.save() + return value + return None + + +def set_librenms_device_id(obj, device_id, server_key: str = "default"): + """ + Set the LibreNMS device/port ID for a specific server on the JSON custom field. + + Does NOT silently migrate legacy bare-integer values to the dict format. + If the field contains a legacy bare integer (or a string that parses as an integer), + a warning is logged and the write is skipped; use the migration workflow instead. + + Args: + obj: NetBox object with a ``librenms_id`` custom field. + device_id: LibreNMS device ID (integer). + server_key: LibreNMS server key (from plugin ``servers`` config). + """ + cf_value = obj.custom_field_data.get("librenms_id") or {} + if isinstance(cf_value, int) and not isinstance(cf_value, bool): + logger.warning( + "librenms_id on %r has legacy bare integer %r; skipping write to prevent " + "silent migration. Use the migration workflow to convert.", + obj, + cf_value, + ) + return + elif isinstance(cf_value, str): + try: + int(cf_value) + logger.warning( + "librenms_id on %r has legacy bare integer string %r; skipping write to " + "prevent silent migration. Use the migration workflow to convert.", + obj, + cf_value, + ) + return + except (ValueError, TypeError): + logger.warning( + "librenms_id custom field has unexpected string %r on %r; resetting to empty dict.", + cf_value, + obj, + ) + cf_value = {} + elif not isinstance(cf_value, dict): + logger.warning( + "librenms_id custom field has unexpected type %s on %r; resetting to empty dict.", + type(cf_value).__name__, + obj, + ) + cf_value = {} + try: + cf_value[server_key] = int(device_id) + except (TypeError, ValueError): + logger.warning( + "librenms_id device_id %r is not a valid integer on %r; not storing.", + device_id, + obj, + ) + return # Don't persist an invalid entry + obj.custom_field_data["librenms_id"] = cf_value + + +def find_by_librenms_id(model, librenms_id, server_key: str = "default"): + """ + Return the first object of *model* whose ``librenms_id`` JSON field contains + *librenms_id* under *server_key*. + + Also matches legacy records stored as a bare ``librenms_id`` integer or string + in ``custom_field_data``—these predate multi-server support and act as a + universal fallback for any *server_key*. + + Args: + model: A Django model class (Device, VirtualMachine, Interface, …). + librenms_id: The LibreNMS device/port ID to look up. + server_key: LibreNMS server key (from plugin ``servers`` config). + + Returns: + Model instance or None + """ + q = Q(**{f"custom_field_data__librenms_id__{server_key}": librenms_id}) + # Also match when the namespaced value was stored as a string (e.g. {"production": "42"}). + q |= Q(**{f"custom_field_data__librenms_id__{server_key}": str(librenms_id)}) + # Always include legacy bare-integer and bare-string IDs as a universal fallback. + # Legacy records were created before multi-server support; they should be visible + # regardless of which server is currently active. + q |= Q(custom_field_data__librenms_id=librenms_id) + q |= Q(custom_field_data__librenms_id=str(librenms_id)) + return model.objects.filter(q).first() + + +def migrate_legacy_librenms_id(obj, server_key: str = "default") -> bool: + """ + Migrate a legacy bare-integer ``librenms_id`` custom field to the JSON dict format, + scoped to *server_key*. + + Only performs the migration when the current value is a bare integer, i.e. a record + created before the multi-server JSON refactor. The integer is assumed to belong to + the server identified by *server_key* (the caller must verify this, e.g. by confirming + that the LibreNMS device ID and serial number both match). + + Does **not** call ``obj.save()`` — the caller is responsible for persisting the change. + + Args: + obj: NetBox object with a ``librenms_id`` custom field. + server_key: LibreNMS server key the legacy integer should be scoped to. + + Returns: + True if the value was migrated, False if it was already in the correct format. + """ + cf_value = obj.custom_field_data.get("librenms_id") + if isinstance(cf_value, str): + try: + cf_value = int(cf_value) # normalize int-like string before migration + except (ValueError, TypeError): + return False + if not isinstance(cf_value, int) or isinstance(cf_value, bool): + return False + obj.custom_field_data["librenms_id"] = {server_key: cf_value} + logger.info( + "Migrated legacy librenms_id %d → {%r: %d} on %r", + cf_value, + server_key, + cf_value, + obj, + ) + return True diff --git a/netbox_librenms_plugin/views/__init__.py b/netbox_librenms_plugin/views/__init__.py index df3beac79b..103900c6c0 100644 --- a/netbox_librenms_plugin/views/__init__.py +++ b/netbox_librenms_plugin/views/__init__.py @@ -52,7 +52,9 @@ from .sync.cables import SyncCablesView # noqa: F401 from .sync.device_fields import ( # noqa: F401 AssignVCSerialView, + ConvertLegacyLibreNMSIdView, CreateAndAssignPlatformView, + RemoveServerMappingView, UpdateDeviceNameView, UpdateDevicePlatformView, UpdateDeviceSerialView, diff --git a/netbox_librenms_plugin/views/base/cables_view.py b/netbox_librenms_plugin/views/base/cables_view.py index cff041c64e..247dceb90c 100644 --- a/netbox_librenms_plugin/views/base/cables_view.py +++ b/netbox_librenms_plugin/views/base/cables_view.py @@ -4,6 +4,7 @@ from django.contrib import messages from django.core.cache import cache from django.core.exceptions import MultipleObjectsReturned +from django.db.models import Q from django.http import JsonResponse from django.middleware.csrf import get_token from django.shortcuts import get_object_or_404, render @@ -14,11 +15,32 @@ from netbox_librenms_plugin.utils import ( get_interface_name_field, + get_librenms_sync_device, get_virtual_chassis_member, ) from netbox_librenms_plugin.views.mixins import CacheMixin, LibreNMSAPIMixin, LibreNMSPermissionMixin +def _librenms_id_q(server_key: str, value) -> Q: + """Return a combined Q matching JSON-field and legacy bare-int librenms_id. + + Matches both integer and string representations to handle any stored format. + """ + q = Q(**{f"custom_field_data__librenms_id__{server_key}": value}) | Q(custom_field_data__librenms_id=value) + try: + int_val = int(value) + str_val = str(int_val) + if int_val != value: # value was a string; also add the integer variant + q |= Q(**{f"custom_field_data__librenms_id__{server_key}": int_val}) + q |= Q(custom_field_data__librenms_id=int_val) + if str_val != value: # value was an integer; also add the string variant + q |= Q(**{f"custom_field_data__librenms_id__{server_key}": str_val}) + q |= Q(custom_field_data__librenms_id=str_val) + except (TypeError, ValueError): + pass + return q + + class BaseCableTableView(LibreNMSPermissionMixin, LibreNMSAPIMixin, CacheMixin, View): """ Base view for synchronizing cable information from LibreNMS. @@ -39,7 +61,8 @@ def get_ip_address(self, obj): def get_ports_data(self, obj): """Get ports data without affecting cache""" - cached_data = cache.get(self.get_cache_key(obj, "ports")) + server_key = self.librenms_api.server_key + cached_data = cache.get(self.get_cache_key(obj, "ports", server_key)) if cached_data: return cached_data success, data = self.librenms_api.get_ports(self.librenms_id) @@ -77,10 +100,11 @@ def get_links_data(self, obj): def get_device_by_id_or_name(self, remote_device_id, hostname): """Try to find device in NetBox first by librenms_id custom field, then by name""" + server_key = self.librenms_api.server_key # First try matching by LibreNMS ID if remote_device_id: try: - device = Device.objects.get(custom_field_data__librenms_id=remote_device_id) + device = Device.objects.get(_librenms_id_q(server_key, remote_device_id)) return device, True, None except Device.DoesNotExist: pass @@ -115,13 +139,14 @@ def enrich_local_port(self, link, obj): if local_port := link.get("local_port"): interface = None local_port_id = link.get("local_port_id") + server_key = self.librenms_api.server_key if hasattr(obj, "virtual_chassis") and obj.virtual_chassis: chassis_member = get_virtual_chassis_member(obj, local_port) # First try to find interface by librenms_id if local_port_id: - interface = chassis_member.interfaces.filter(custom_field_data__librenms_id=local_port_id).first() + interface = chassis_member.interfaces.filter(_librenms_id_q(server_key, local_port_id)).first() # Only if librenms_id match fails, try matching by name if not interface: @@ -129,7 +154,7 @@ def enrich_local_port(self, link, obj): else: # First try to find interface by librenms_id if local_port_id: - interface = obj.interfaces.filter(custom_field_data__librenms_id=local_port_id).first() + interface = obj.interfaces.filter(_librenms_id_q(server_key, local_port_id)).first() # Only if librenms_id match fails, try matching by name if not interface: @@ -144,6 +169,7 @@ def enrich_remote_port(self, link, device): if remote_port := link.get("remote_port"): netbox_remote_interface = None librenms_remote_port_id = link.get("remote_port_id") + server_key = self.librenms_api.server_key # Handle virtual chassis case if hasattr(device, "virtual_chassis") and device.virtual_chassis: @@ -153,7 +179,7 @@ def enrich_remote_port(self, link, device): # First try to find interface by librenms_id if librenms_remote_port_id: netbox_remote_interface = chassis_member.interfaces.filter( - custom_field_data__librenms_id=librenms_remote_port_id + _librenms_id_q(server_key, librenms_remote_port_id) ).first() # If not found by librenms_id, fall back to name matching on the correct chassis member @@ -164,7 +190,7 @@ def enrich_remote_port(self, link, device): # First try to find interface by librenms_id if librenms_remote_port_id: netbox_remote_interface = device.interfaces.filter( - custom_field_data__librenms_id=librenms_remote_port_id + _librenms_id_q(server_key, librenms_remote_port_id) ).first() # If not found by librenms_id, fall back to name matching @@ -254,6 +280,7 @@ def get_table(self, data, obj): def _prepare_context(self, request, obj, fetch_fresh=False): """Helper method to prepare the context data for cable sync views.""" cache_expiry = None + server_key = self.librenms_api.server_key if fetch_fresh: # Always fetch new data when requested @@ -262,7 +289,7 @@ def _prepare_context(self, request, obj, fetch_fresh=False): return None else: # Try to use cached data - cached_links_data = cache.get(self.get_cache_key(obj, "links")) + cached_links_data = cache.get(self.get_cache_key(obj, "links", server_key)) if cached_links_data: links_data = cached_links_data.get("links", []) else: @@ -286,7 +313,7 @@ def _prepare_context(self, request, obj, fetch_fresh=False): links_data = self.enrich_links_data(links_data, obj) # Cache after enrichment so verify/sync views read current NetBox state - cache_key = self.get_cache_key(obj, "links") + cache_key = self.get_cache_key(obj, "links", server_key) if fetch_fresh: cache.set( cache_key, @@ -363,18 +390,13 @@ def post(self, request): if selected_device_id: selected_device = get_object_or_404(Device, pk=selected_device_id) - # Get the primary device (master or first with IP) if part of virtual chassis - if selected_device.virtual_chassis: - primary_device = selected_device.virtual_chassis.master - if not primary_device or not primary_device.primary_ip: - primary_device = next( - (member for member in selected_device.virtual_chassis.members.all() if member.primary_ip), - None, - ) - else: - primary_device = selected_device + # Use the same sync-device resolution as the GET path so the cache + # key matches what _prepare_context wrote. + primary_device = ( + get_librenms_sync_device(selected_device, server_key=self.librenms_api.server_key) or selected_device + ) - cached_links = cache.get(self.get_cache_key(primary_device, "links")) + cached_links = cache.get(self.get_cache_key(primary_device, "links", self.librenms_api.server_key)) if cached_links: link_data = next( @@ -386,38 +408,24 @@ def post(self, request): None, ) if link_data: - # Strip derived fields from cached data to avoid stale - # IDs/URLs when NetBox objects are deleted after caching. - _raw_keys = { - "local_port", - "local_port_id", - "remote_port", - "remote_device", - "remote_port_id", - "remote_device_id", - } - link_data = {k: v for k, v in link_data.items() if k in _raw_keys} - - # Re-enrich remote side from current NetBox state - remote_hostname = link_data.get("remote_device", "") - if remote_hostname: - link_data = self.process_remote_device( - link_data, remote_hostname, link_data.get("remote_device_id") - ) - local_port = link_data.get("local_port", "") formatted_row["local_port"] = local_port + # Resolve the VC member that owns this port (mirrors enrich_local_port). + _sk = self.librenms_api.server_key + if hasattr(selected_device, "virtual_chassis") and selected_device.virtual_chassis: + _member = get_virtual_chassis_member(selected_device, local_port) + else: + _member = selected_device + # First try to find interface by librenms_id interface = None if local_port_id: - interface = selected_device.interfaces.filter( - custom_field_data__librenms_id=local_port_id - ).first() + interface = _member.interfaces.filter(_librenms_id_q(_sk, local_port_id)).first() # If not found by librenms_id, try matching by name if not interface and local_port: - interface = selected_device.interfaces.filter(name=local_port).first() + interface = _member.interfaces.filter(name=local_port).first() if interface: link_data["netbox_local_interface_id"] = interface.pk diff --git a/netbox_librenms_plugin/views/base/interfaces_view.py b/netbox_librenms_plugin/views/base/interfaces_view.py index 80e413a8f3..7868295755 100644 --- a/netbox_librenms_plugin/views/base/interfaces_view.py +++ b/netbox_librenms_plugin/views/base/interfaces_view.py @@ -93,15 +93,16 @@ def post(self, request, pk): enriched_ports = self._enrich_ports_with_vlan_data(ports, interface_name_field) librenms_data["ports"] = enriched_ports - # Store data in cache + _server_key = self.librenms_api.server_key + # Store data in cache using server-scoped key to prevent cross-server collisions. cache.set( - self.get_cache_key(obj, "ports"), + self.get_cache_key(obj, "ports", _server_key), librenms_data, timeout=self.librenms_api.cache_timeout, ) last_fetched = timezone.now() cache.set( - self.get_last_fetched_key(obj, "ports"), + self.get_last_fetched_key(obj, "ports", _server_key), last_fetched, timeout=self.librenms_api.cache_timeout, ) @@ -147,15 +148,16 @@ def get_context_data(self, request, obj, interface_name_field): if interface_name_field is None: interface_name_field = get_interface_name_field(request) - cached_data = cache.get(self.get_cache_key(obj, "ports")) - last_fetched = cache.get(self.get_last_fetched_key(obj, "ports")) + _server_key = self.librenms_api.server_key + cached_data = cache.get(self.get_cache_key(obj, "ports", _server_key)) + last_fetched = cache.get(self.get_last_fetched_key(obj, "ports", _server_key)) # Get VLAN groups for dropdown vlan_groups = self.get_vlan_groups_for_device(obj) lookup_maps = self._build_vlan_lookup_maps(vlan_groups) # Load any user VLAN group overrides from cache (set by "apply to all") - vlan_group_overrides = cache.get(self.get_vlan_overrides_key(obj)) or {} + vlan_group_overrides = cache.get(self.get_vlan_overrides_key(obj, _server_key)) or {} if cached_data: ports_data = cached_data.get("ports", []) @@ -244,7 +246,7 @@ def get_context_data(self, request, obj, interface_name_field): if hasattr(obj, "virtual_chassis") and obj.virtual_chassis: virtual_chassis_members = obj.virtual_chassis.members.all() - cache_ttl = cache.ttl(self.get_cache_key(obj, "ports")) + cache_ttl = cache.ttl(self.get_cache_key(obj, "ports", _server_key)) cache_expiry = timezone.now() + timezone.timedelta(seconds=cache_ttl) if cache_ttl is not None else None return { @@ -256,6 +258,7 @@ def get_context_data(self, request, obj, interface_name_field): "virtual_chassis_members": virtual_chassis_members, "interface_name_field": interface_name_field, "netbox_only_interfaces": netbox_only_interfaces, + "server_key": _server_key, } def _add_vlan_group_selection(self, port, lookup_maps, device, vlan_group_overrides=None): diff --git a/netbox_librenms_plugin/views/base/ip_addresses_view.py b/netbox_librenms_plugin/views/base/ip_addresses_view.py index 22f4b49742..2b90c23179 100644 --- a/netbox_librenms_plugin/views/base/ip_addresses_view.py +++ b/netbox_librenms_plugin/views/base/ip_addresses_view.py @@ -229,6 +229,7 @@ def _prepare_context(self, request, obj, interface_name_field, fetch_fresh=False """Helper method to prepare the context data for IP address sync views.""" table = None cache_expiry = None + server_key = self.librenms_api.server_key if interface_name_field is None: interface_name_field = get_interface_name_field(request) @@ -236,7 +237,7 @@ def _prepare_context(self, request, obj, interface_name_field, fetch_fresh=False if fetch_fresh: success, ip_data = self.get_ip_addresses(obj) else: - cached_ip_data = cache.get(self.get_cache_key(obj, "ip_addresses")) + cached_ip_data = cache.get(self.get_cache_key(obj, "ip_addresses", server_key)) if cached_ip_data: ip_data = cached_ip_data.get("ip_addresses", []) else: @@ -248,13 +249,13 @@ def _prepare_context(self, request, obj, interface_name_field, fetch_fresh=False if fetch_fresh: # Cache the fresh data after enrichment cache.set( - self.get_cache_key(obj, "ip_addresses"), + self.get_cache_key(obj, "ip_addresses", server_key), {"ip_addresses": ip_data}, timeout=self.librenms_api.cache_timeout, ) # Calculate cache expiry - cache_ttl = cache.ttl(self.get_cache_key(obj, "ip_addresses")) + cache_ttl = cache.ttl(self.get_cache_key(obj, "ip_addresses", server_key)) if cache_ttl is not None: cache_expiry = timezone.now() + timezone.timedelta(seconds=cache_ttl) diff --git a/netbox_librenms_plugin/views/base/librenms_sync_view.py b/netbox_librenms_plugin/views/base/librenms_sync_view.py index aaaaab4081..1c475c15b4 100644 --- a/netbox_librenms_plugin/views/base/librenms_sync_view.py +++ b/netbox_librenms_plugin/views/base/librenms_sync_view.py @@ -1,5 +1,6 @@ import re +from django.conf import settings as django_settings from django.shortcuts import get_object_or_404, render from netbox.views import generic @@ -31,12 +32,13 @@ def get(self, request, pk, context=None): # order (explicit per-server dict > legacy bare-int > master with IP > any IP > position). librenms_lookup_device = obj if hasattr(obj, "virtual_chassis") and obj.virtual_chassis: - sync_device = get_librenms_sync_device(obj) + sync_device = get_librenms_sync_device(obj, server_key=self.librenms_api.server_key) if sync_device: librenms_lookup_device = sync_device # Get librenms_id using the determined lookup device self.librenms_id = self.librenms_api.get_librenms_id(librenms_lookup_device) + self._librenms_lookup_device = librenms_lookup_device context = self.get_context_data(request, obj) @@ -58,14 +60,14 @@ def get_context_data(self, request, obj): if hasattr(obj, "virtual_chassis") and obj.virtual_chassis: # Use helper function to determine the sync device - librenms_sync_device = get_librenms_sync_device(obj) + librenms_sync_device = get_librenms_sync_device(obj, server_key=self.librenms_api.server_key) # Determine sync device status sync_device_has_librenms_id = False sync_device_has_primary_ip = False if librenms_sync_device: - sync_device_has_librenms_id = bool(librenms_sync_device.cf.get("librenms_id")) + sync_device_has_librenms_id = bool(self.librenms_api.get_librenms_id(librenms_sync_device)) sync_device_has_primary_ip = bool(librenms_sync_device.primary_ip) context.update( @@ -83,6 +85,7 @@ def get_context_data(self, request, obj): cable_context = self.get_cable_context(request, obj) ip_context = self.get_ip_context(request, obj) vlan_context = self.get_vlan_context(request, obj) + module_context = self.get_module_context(request, obj) interface_name_field = get_interface_name_field(request) @@ -94,12 +97,25 @@ def get_context_data(self, request, obj): manufacturers = Manufacturer.objects.all().order_by("name") + # Detect legacy bare-int librenms_id format for conversion badge + _lookup_device = getattr(self, "_librenms_lookup_device", obj) + _raw_cf = _lookup_device.cf.get("librenms_id") if _lookup_device else None + librenms_id_is_legacy = isinstance(_raw_cf, (int, str)) and not isinstance(_raw_cf, bool) + + # Determine if serial match allows legacy ID conversion + _librenms_serial = librenms_info["librenms_device_details"].get("librenms_device_serial", "-") + _netbox_serial = getattr(_lookup_device, "serial", "") or "" + librenms_id_serial_confirmed = bool( + _librenms_serial and _librenms_serial != "-" and _netbox_serial and _librenms_serial == _netbox_serial + ) + context.update( { "interface_sync": interface_context, "cable_sync": cable_context, "ip_sync": ip_context, "vlan_sync": vlan_context, + "module_sync": module_context, "v1v2form": AddToLIbreSNMPV1V2(prefix="v1v2"), "v3form": AddToLIbreSNMPV3(prefix="v3"), "librenms_device_id": self.librenms_id, @@ -111,11 +127,84 @@ def get_context_data(self, request, obj): "platform_info": platform_info, "vc_inventory_serials": librenms_info["librenms_device_details"].get("vc_inventory_serials", []), "manufacturers": manufacturers, + "all_server_mappings": self._build_all_server_mappings(_lookup_device, self.librenms_api.server_key), + "librenms_id_is_legacy": librenms_id_is_legacy, + "librenms_id_serial_confirmed": librenms_id_serial_confirmed, + # Lookup device may differ from object (e.g. VC master vs member). + # Used by the Remove server mapping form to post to the correct device. + "lookup_device_pk": _lookup_device.pk if _lookup_device else obj.pk, + "lookup_device_model_name": ( + _lookup_device._meta.model_name if _lookup_device else obj._meta.model_name + ), } ) return context + @staticmethod + def _build_all_server_mappings(obj, active_server_key): + """Build a list of all LibreNMS server mappings for the given device. + + Each entry describes one server<->ID mapping stored in the ``librenms_id`` + custom field: + + * ``server_key`` – the key as stored in the CF dict. + * ``display_name`` – human-readable name from PLUGINS_CONFIG, or the key. + * ``librenms_url`` – base URL of that server (``None`` when not configured). + * ``device_id`` – the integer device ID on that server. + * ``device_url`` – direct URL to the device page on that server (or ``None``). + * ``is_configured`` – True when the server key exists in current plugin config. + * ``is_active`` – True when this is the currently active server. + + Returns ``None`` for legacy bare-int format (no per-server info to show) + and ``None`` when the CF is absent/invalid. + """ + cf_value = obj.custom_field_data.get("librenms_id") + if not isinstance(cf_value, dict) or not cf_value: + return None + + plugins_cfg = getattr(django_settings, "PLUGINS_CONFIG", {}).get("netbox_librenms_plugin", {}) + servers_config = plugins_cfg.get("servers") or {} + if not isinstance(servers_config, dict): + servers_config = {} + + result = [] + for sk, did in cf_value.items(): + srv_cfg = servers_config.get(sk) + # Legacy single-server config: "default" key with no matching servers entry — + # fall back to root-level librenms_url/display_name in plugins_cfg. + # Only do this when no "servers" section is configured (i.e., legacy mode). + if srv_cfg is None and sk == "default" and not servers_config: + legacy_url = plugins_cfg.get("librenms_url") + if legacy_url: + srv_cfg = { + "librenms_url": legacy_url, + "display_name": plugins_cfg.get("display_name") or sk, + } + is_configured = srv_cfg is not None + # Treat malformed (non-dict) server config entries as unconfigured + if srv_cfg is not None and not isinstance(srv_cfg, dict): + srv_cfg = None + is_configured = False + librenms_url = srv_cfg.get("librenms_url") if srv_cfg else None + display_name = (srv_cfg.get("display_name") or sk) if srv_cfg else sk + device_url = f"{librenms_url}/device/device={did}/" if librenms_url else None + result.append( + { + "server_key": sk, + "display_name": display_name, + "librenms_url": librenms_url, + "device_id": did, + "device_url": device_url, + "is_configured": is_configured, + "is_active": sk == active_server_key, + } + ) + + # Sort: active first, then configured, then orphaned + result.sort(key=lambda e: 0 if e["is_active"] else (1 if e["is_configured"] else 2)) + return result or None + def get_librenms_device_info(self, obj): """Get the LibreNMS device information for the given object.""" found_in_librenms = False @@ -265,6 +354,13 @@ def get_vlan_context(self, request, obj): """ return None + def get_module_context(self, request, obj): + """ + Get the context data for module sync. + Subclasses should override this method if applicable. + """ + return None + @staticmethod def _strip_vc_pattern(name): """Strip the VC member naming suffix from a device name. diff --git a/netbox_librenms_plugin/views/base/vlan_table_view.py b/netbox_librenms_plugin/views/base/vlan_table_view.py index 78bbfb0fca..fb8ab627c9 100644 --- a/netbox_librenms_plugin/views/base/vlan_table_view.py +++ b/netbox_librenms_plugin/views/base/vlan_table_view.py @@ -64,13 +64,14 @@ def _fetch_and_cache_vlan_data(self, obj): return False, f"Failed to fetch VLANs: {vlans_data}" # Cache VLANs + server_key = self.librenms_api.server_key cache.set( - self.get_cache_key(obj, "vlans"), + self.get_cache_key(obj, "vlans", server_key), vlans_data, timeout=self.librenms_api.cache_timeout, ) cache.set( - self.get_last_fetched_key(obj, "vlans"), + self.get_last_fetched_key(obj, "vlans", server_key), timezone.now(), timeout=self.librenms_api.cache_timeout, ) @@ -88,8 +89,9 @@ def get_vlan_context(self, request, obj): vlan_table = None # Get cached data - cached_vlans = cache.get(self.get_cache_key(obj, "vlans")) - last_fetched = cache.get(self.get_last_fetched_key(obj, "vlans")) + server_key = getattr(self.librenms_api, "server_key", None) + cached_vlans = cache.get(self.get_cache_key(obj, "vlans", server_key)) + last_fetched = cache.get(self.get_last_fetched_key(obj, "vlans", server_key)) # Get available VLAN groups for this device vlan_groups = self.get_vlan_groups_for_device(obj) @@ -105,7 +107,7 @@ def get_vlan_context(self, request, obj): vlan_table.configure(request) # Calculate cache TTL - cache_ttl = cache.ttl(self.get_cache_key(obj, "vlans")) + cache_ttl = cache.ttl(self.get_cache_key(obj, "vlans", server_key)) cache_expiry = timezone.now() + timezone.timedelta(seconds=cache_ttl) if cache_ttl else None return { diff --git a/netbox_librenms_plugin/views/imports/actions.py b/netbox_librenms_plugin/views/imports/actions.py index 1c6bc3a1f0..880b14f6cf 100644 --- a/netbox_librenms_plugin/views/imports/actions.py +++ b/netbox_librenms_plugin/views/imports/actions.py @@ -6,6 +6,8 @@ from django.contrib import messages from django.core.cache import cache from django.core.exceptions import PermissionDenied, ValidationError +from django.db import transaction +from django.db.models import Q from django.http import HttpResponse, JsonResponse from django.shortcuts import redirect, render from django.utils.html import escape @@ -30,7 +32,7 @@ fetch_model_by_id, ) from netbox_librenms_plugin.tables.device_status import DeviceImportTable -from netbox_librenms_plugin.utils import get_user_pref, save_user_pref +from netbox_librenms_plugin.utils import get_user_pref, save_user_pref, set_librenms_device_id from netbox_librenms_plugin.views.mixins import LibreNMSAPIMixin, LibreNMSPermissionMixin, NetBoxObjectPermissionMixin logger = logging.getLogger(__name__) @@ -208,6 +210,7 @@ def get_validated_device_with_selections(self, device_id: int, request) -> tuple include_vc_detection=enable_vc, use_sysname=use_sysname, strip_domain=strip_domain, + server_key=self.librenms_api.server_key, ) validation["import_as_vm"] = is_vm @@ -354,6 +357,7 @@ def post(self, request): api=self.librenms_api, use_sysname=use_sysname, strip_domain=strip_domain, + server_key=self.librenms_api.server_key, ) # Mark validation with VC detection flag for proper URL generation in table @@ -722,6 +726,7 @@ def post(self, request): # noqa: PLR0912 - branching keeps responses explicit import_as_vm=is_vm, api=None, # No VC detection needed for already-imported devices include_vc_detection=False, + server_key=self.librenms_api.server_key, use_sysname=sync_options.get("use_sysname", True), strip_domain=sync_options.get("strip_domain", False), ) @@ -804,12 +809,15 @@ def get(self, request, device_id): "validation": validation, "use_sysname": use_sysname, "strip_domain": strip_domain, + "server_key": self.librenms_api.server_key, } # Add sync comparison data for existing devices existing = validation.get("existing_device") if existing: context["sync_info"] = self._build_sync_info(libre_device, existing) + context["existing_id_servers"] = self._build_id_server_info(existing) + context["existing_device_model_name"] = existing._meta.model_name return render( request, @@ -877,6 +885,33 @@ def _build_sync_info(libre_device, existing_device): "all_synced": all_synced, } + @staticmethod + def _build_id_server_info(existing_device): + """Return per-server ID mappings for the existing device's librenms_id custom field. + + Returns a list of dicts with server_key, display_name, and device_id — one entry + per server the device is linked to. Returns None when the format is legacy (bare int) + or when the field is absent/invalid. + """ + from django.conf import settings + + cf_value = existing_device.custom_field_data.get("librenms_id") + if not isinstance(cf_value, dict): + return None + + plugins_config = settings.PLUGINS_CONFIG.get("netbox_librenms_plugin") or {} + servers_config = plugins_config.get("servers") or {} + if not isinstance(servers_config, dict): + servers_config = {} + result = [] + for sk, did in cf_value.items(): + srv_cfg = servers_config.get(sk) or {} + if not isinstance(srv_cfg, dict): + srv_cfg = {} + display_name = srv_cfg.get("display_name") or sk + result.append({"server_key": sk, "display_name": display_name, "device_id": did}) + return result or None + class DeviceRoleUpdateView(LibreNMSPermissionMixin, LibreNMSAPIMixin, DeviceImportHelperMixin, View): """HTMX view to update a table row when a role is selected.""" @@ -928,20 +963,37 @@ def post(self, request, device_id): return error from dcim.models import Device + from netbox_librenms_plugin.librenms_api import LibreNMSAPI action = request.POST.get("action") existing_device_id = request.POST.get("existing_device_id") + existing_device_type = request.POST.get("existing_device_type", "device") + + # If the form submitted a specific server_key, honour it so the handler uses + # the same server context as the import page when the user clicked the button. + post_server_key = request.POST.get("server_key", "").strip() + if post_server_key: + self._librenms_api = LibreNMSAPI(server_key=post_server_key) if not action or not existing_device_id: return HttpResponse("Missing action or existing_device_id", status=400) + # VirtualMachine is supported for migrate_librenms_id only; all other actions + # operate on Device-specific fields (serial, device_type) and remain Device-only. + if existing_device_type == "virtualmachine" and action == "migrate_librenms_id": + from virtualization.models import VirtualMachine as NetBoxVM + + existing_model: type = NetBoxVM + else: + existing_model = Device + try: - existing_device = Device.objects.get(pk=int(existing_device_id)) - except (Device.DoesNotExist, ValueError): + existing_device = existing_model.objects.get(pk=int(existing_device_id)) + except (existing_model.DoesNotExist, ValueError): return HttpResponse("Existing device not found", status=404) - # Object-level change permission for the specific device being mutated. - self.required_object_permissions = {"POST": [("change", Device)]} + # Object-level change permission for the specific model being mutated. + self.required_object_permissions = {"POST": [("change", existing_model)]} if error := self.require_object_permissions("POST"): return error @@ -955,7 +1007,7 @@ def post(self, request, device_id): validated_existing = validation.get("existing_device") if validation else None if validated_existing is None: return HttpResponse("Missing validated conflict target", status=400) - if validated_existing.pk != existing_device.pk: + if validated_existing.pk != existing_device.pk or type(validated_existing) is not type(existing_device): return HttpResponse("Device ID mismatch: existing_device_id does not match validated device", status=400) # Require force flag when device type mismatches, but only for actions that use it @@ -972,108 +1024,119 @@ def post(self, request, device_id): librenms_device_type = validation.get("device_type", {}).get("device_type") librenms_id = libre_device.get("device_id") - - # Check for LibreNMS ID collision before any linking action + try: + librenms_id = int(librenms_id) + except (TypeError, ValueError): + return HttpResponse("Invalid or missing LibreNMS device_id in payload", status=400) + + # Wrap the LibreNMS-ID collision check and subsequent write in a single + # transaction so the read-then-write is atomic for link/update/update_serial. + # NOTE: A fully race-free guarantee would require a DB-unique constraint on + # (server_key, librenms_id) — e.g., a dedicated DeviceLibreNMSIDMapping model. + # That is deferred to a future schema migration. Until then, we acquire a + # row-level lock on the target device before re-checking for conflicts, which + # serializes concurrent operations on the SAME device and greatly reduces the + # window for assigning the same ID to two DIFFERENT devices. if action in {"link", "update", "update_serial"}: - id_conflict = ( - Device.objects.filter(custom_field_data__librenms_id=int(librenms_id)) - .exclude(pk=existing_device.pk) - .first() - ) - if id_conflict: - return HttpResponse( - f"LibreNMS ID conflict: ID {escape(str(librenms_id))} is already assigned to device " - f"'{escape(id_conflict.name)}' (ID: {id_conflict.pk})", - status=409, - ) - - if action == "link": - # Link to LibreNMS and update name from LibreNMS data - resolved_name = validation.get("resolved_name") - hostname = ( - resolved_name - if resolved_name - else _determine_device_name( - libre_device, - use_sysname=request.POST.get("use-sysname-toggle") == "on", - strip_domain=request.POST.get("strip-domain-toggle") == "on", - ) - ) - existing_device.custom_field_data["librenms_id"] = int(librenms_id) - existing_device.name = hostname - if librenms_device_type: - existing_device.device_type = librenms_device_type - if err := _save_device(existing_device): - return err - logger.info(f"Linked device '{existing_device.name}' to LibreNMS ID {librenms_id}") - - elif action == "update": - # Update hostname, serial, and link to LibreNMS - resolved_name = validation.get("resolved_name") - incoming_serial = libre_device.get("serial") or "" - hostname = ( - resolved_name - if resolved_name - else _determine_device_name( - libre_device, - use_sysname=request.POST.get("use-sysname-toggle") == "on", - strip_domain=request.POST.get("strip-domain-toggle") == "on", - ) - ) - existing_device.custom_field_data["librenms_id"] = int(librenms_id) - if incoming_serial and incoming_serial != "-": - conflict_device = Device.objects.filter(serial=incoming_serial).exclude(pk=existing_device.pk).first() - if conflict_device: + from netbox_librenms_plugin.utils import find_by_librenms_id + + with transaction.atomic(): + server_key = self.librenms_api.server_key + # Lock the target device row so concurrent requests for the same + # device are serialized. The conflict check below is still a + # best-effort guard for different devices; a DB unique constraint + # would be needed for full protection. + try: + existing_device = Device.objects.select_for_update().get(pk=existing_device.pk) + except Device.DoesNotExist: return HttpResponse( - f"Serial conflict: '{escape(incoming_serial)}' is already assigned to device " - f"'{escape(conflict_device.name)}' (ID: {conflict_device.pk})", + "Device no longer exists; it may have been deleted concurrently.", status=409, ) - existing_device.serial = incoming_serial - existing_device.name = hostname - if librenms_device_type: - existing_device.device_type = librenms_device_type - if err := _save_device(existing_device): - return err - logger.info( - f"Updated device '{existing_device.name}': serial={incoming_serial}, " - f"linked to LibreNMS ID {librenms_id}" - ) - - elif action == "update_serial": - # Update only the serial and link to LibreNMS - incoming_serial = libre_device.get("serial") or "" - existing_device.custom_field_data["librenms_id"] = int(librenms_id) - if incoming_serial and incoming_serial != "-": - conflict_device = Device.objects.filter(serial=incoming_serial).exclude(pk=existing_device.pk).first() - if conflict_device: + id_conflict = find_by_librenms_id(Device, int(librenms_id), server_key) + if id_conflict and id_conflict.pk != existing_device.pk: return HttpResponse( - f"Serial conflict: '{escape(incoming_serial)}' is already assigned to device " - f"'{escape(conflict_device.name)}' (ID: {conflict_device.pk})", + f"LibreNMS ID conflict: ID {escape(str(librenms_id))} is already assigned to device " + f"'{escape(id_conflict.name)}' (ID: {id_conflict.pk})", status=409, ) - existing_device.serial = incoming_serial - if librenms_device_type: - existing_device.device_type = librenms_device_type - if err := _save_device(existing_device): - return err - logger.info( - f"Updated serial on device '{existing_device.name}' to {incoming_serial}, " - f"linked to LibreNMS ID {librenms_id}" - ) + + if action == "link": + # Link to LibreNMS and update name from LibreNMS data + hostname = _get_hostname_for_action(request, validation, libre_device) + set_librenms_device_id(existing_device, librenms_id, self.librenms_api.server_key) + existing_device.name = hostname + if librenms_device_type: + existing_device.device_type = librenms_device_type + if err := _save_device(existing_device): + return err + logger.info(f"Linked device '{existing_device.name}' to LibreNMS ID {librenms_id}") + + elif action == "update": + # Update hostname, serial, and link to LibreNMS + hostname = _get_hostname_for_action(request, validation, libre_device) + incoming_serial = libre_device.get("serial") or "" + if incoming_serial and incoming_serial != "-": + # Lock any conflicting device under the same transaction to reduce + # the serial-assignment race window (best-effort; a DB unique + # constraint on serial would give full protection). + conflict_device = ( + Device.objects.select_for_update() + .filter(serial=incoming_serial) + .exclude(pk=existing_device.pk) + .first() + ) + if conflict_device: + return HttpResponse( + f"Serial conflict: '{escape(incoming_serial)}' is already assigned to device " + f"'{escape(conflict_device.name)}' (ID: {conflict_device.pk})", + status=409, + ) + existing_device.serial = incoming_serial + existing_device.name = hostname + if librenms_device_type: + existing_device.device_type = librenms_device_type + set_librenms_device_id(existing_device, librenms_id, self.librenms_api.server_key) + if err := _save_device(existing_device): + return err + logger.info( + f"Updated device '{existing_device.name}': serial={incoming_serial}, " + f"linked to LibreNMS ID {librenms_id}" + ) + + elif action == "update_serial": + # Update only the serial and link to LibreNMS + incoming_serial = libre_device.get("serial") or "" + if incoming_serial and incoming_serial != "-": + # Lock any conflicting device under the same transaction to reduce + # the serial-assignment race window (best-effort; a DB unique + # constraint on serial would give full protection). + conflict_device = ( + Device.objects.select_for_update() + .filter(serial=incoming_serial) + .exclude(pk=existing_device.pk) + .first() + ) + if conflict_device: + return HttpResponse( + f"Serial conflict: '{escape(incoming_serial)}' is already assigned to device " + f"'{escape(conflict_device.name)}' (ID: {conflict_device.pk})", + status=409, + ) + existing_device.serial = incoming_serial + if librenms_device_type: + existing_device.device_type = librenms_device_type + set_librenms_device_id(existing_device, librenms_id, self.librenms_api.server_key) + if err := _save_device(existing_device): + return err + logger.info( + f"Updated serial on device '{existing_device.name}' to {incoming_serial}, " + f"linked to LibreNMS ID {librenms_id}" + ) elif action == "sync_name": # Sync device name from LibreNMS (e.g., IP → sysName) - resolved_name = validation.get("resolved_name") - hostname = ( - resolved_name - if resolved_name - else _determine_device_name( - libre_device, - use_sysname=request.POST.get("use-sysname-toggle") == "on", - strip_domain=request.POST.get("strip-domain-toggle") == "on", - ) - ) + hostname = _get_hostname_for_action(request, validation, libre_device) existing_device.name = hostname if err := _save_device(existing_device): return err @@ -1090,25 +1153,41 @@ def post(self, request, device_id): return HttpResponse("No LibreNMS device type available to update", status=400) elif action == "sync_serial": - # Sync serial number from LibreNMS + # Sync serial number from LibreNMS. + # Wrap conflict-check-and-write in a transaction with a row lock so + # concurrent requests cannot both pass the serial uniqueness guard. incoming_serial = libre_device.get("serial") or "" if incoming_serial and incoming_serial != "-": - # Check for serial ownership conflict - conflict_device = Device.objects.filter(serial=incoming_serial).exclude(pk=existing_device.pk).first() - if conflict_device: - logger.warning( - f"Serial sync blocked: '{incoming_serial}' already assigned to " - f"'{conflict_device.name}' (pk={conflict_device.pk})" - ) - return HttpResponse( - f"Serial conflict: '{escape(incoming_serial)}' is already assigned to device " - f"'{escape(conflict_device.name)}' (ID: {conflict_device.pk})", - status=409, - ) - existing_device.serial = incoming_serial - if err := _save_device(existing_device): - return err - logger.info(f"Synced serial on '{existing_device.name}' to {incoming_serial}") + with transaction.atomic(): + try: + locked_device = Device.objects.select_for_update().get(pk=existing_device.pk) + except Device.DoesNotExist: + return HttpResponse( + "Device no longer exists; it may have been deleted concurrently.", + status=409, + ) + # Re-check for serial ownership conflict under lock. + # Note: We intentionally do NOT enforce a DB-level uniqueness constraint on + # Device.serial. During device moves/replacements, multiple devices may + # temporarily share a serial (old record gets updated later). A unique + # constraint would block those valid workflows. Instead, we rely on this + # in-transaction row-lock check to guard concurrent sync of the SAME serial, + # and flag conflicts via a 409 response for the user to resolve manually. + conflict_device = Device.objects.filter(serial=incoming_serial).exclude(pk=locked_device.pk).first() + if conflict_device: + logger.warning( + f"Serial sync blocked: '{incoming_serial}' already assigned to " + f"'{conflict_device.name}' (pk={conflict_device.pk})" + ) + return HttpResponse( + f"Serial conflict: '{escape(incoming_serial)}' is already assigned to device " + f"'{escape(conflict_device.name)}' (ID: {conflict_device.pk})", + status=409, + ) + locked_device.serial = incoming_serial + if err := _save_device(locked_device): + return err + logger.info(f"Synced serial on '{locked_device.name}' to {incoming_serial}") else: return HttpResponse("No valid serial from LibreNMS", status=400) @@ -1143,6 +1222,98 @@ def post(self, request, device_id): else: return HttpResponse(f"No matching device type for '{escape(hardware)}'", status=400) + elif action == "migrate_librenms_id": + # Migrate legacy bare-integer librenms_id to the JSON dict format. + # Only safe when the integer matches the LibreNMS device ID for this server, + # confirmed by serial match (or explicit force). + from netbox_librenms_plugin.utils import migrate_legacy_librenms_id + + # Verify that the import workflow flagged this device as needing migration, + # preventing direct POST bypass of the modal validation gate. + if not validation.get("librenms_id_needs_migration"): + return HttpResponse( + "Device is not flagged for librenms_id migration.", + status=400, + ) + # Direct access needed to detect legacy integer format for migration prompt: + # LibreNMSAPI.get_librenms_id() returns an int in both formats; only the raw + # type check on custom_field_data reveals whether migration is needed. + cf_value = existing_device.custom_field_data.get("librenms_id") + if isinstance(cf_value, bool) or not ( + isinstance(cf_value, int) or (isinstance(cf_value, str) and cf_value.isdigit()) + ): + return HttpResponse( + "Device librenms_id is already in JSON format; no migration needed.", + status=400, + ) + # Normalise string-digit to int for consistent comparison + cf_int = int(cf_value) if isinstance(cf_value, str) else cf_value + # Verify the stored legacy ID matches the active LibreNMS device_id so we don't + # migrate a stale/incorrect association to the wrong server mapping. + if cf_int != librenms_id: + return HttpResponse( + f"Legacy librenms_id ({cf_int}) does not match the active device ID " + f"({librenms_id}); cannot migrate safely.", + status=400, + ) + if not validation.get("serial_confirmed") and not force: + return HttpResponse( + "Serial number not confirmed. Check the force checkbox to migrate without serial verification.", + status=400, + ) + with transaction.atomic(): + try: + locked_device = existing_model.objects.select_for_update().get(pk=existing_device.pk) + except existing_model.DoesNotExist: + return HttpResponse( + "Object no longer exists; it may have been deleted concurrently.", + status=409, + ) + # Re-check under lock — another request may have already migrated it + cf_locked = locked_device.custom_field_data.get("librenms_id") + if isinstance(cf_locked, bool) or not ( + isinstance(cf_locked, int) or (isinstance(cf_locked, str) and cf_locked.isdigit()) + ): + return HttpResponse( + "Device librenms_id is already in JSON format; no migration needed.", + status=400, + ) + cf_locked_int = int(cf_locked) if isinstance(cf_locked, str) else cf_locked + if cf_locked_int != librenms_id: + return HttpResponse( + f"Legacy librenms_id changed under lock ({cf_locked_int} != {librenms_id}); cannot migrate safely.", + status=400, + ) + # Check that no other object already owns this ID on this server + # (both new namespaced format — int and string — and legacy integer format) + server_key = self.librenms_api.server_key + conflict = ( + existing_model.objects.filter( + Q(**{f"custom_field_data__librenms_id__{server_key}": cf_locked_int}) + | Q(**{f"custom_field_data__librenms_id__{server_key}": str(cf_locked_int)}) + | Q(custom_field_data__librenms_id=cf_locked_int) + | Q(custom_field_data__librenms_id=str(cf_locked_int)) + ) + .exclude(pk=locked_device.pk) + .exists() + ) + if conflict: + return HttpResponse( + f"Another device already has librenms_id {cf_locked_int} for server '{server_key}'; cannot migrate.", + status=409, + ) + if not migrate_legacy_librenms_id(locked_device, self.librenms_api.server_key): + return HttpResponse( + "Migration failed: librenms_id could not be converted.", + status=400, + ) + if err := _save_device(locked_device): + return err + logger.info( + f"Migrated legacy librenms_id on '{locked_device.name}' " + f"to {{{self.librenms_api.server_key!r}: {cf_locked_int}}}" + ) + else: return HttpResponse(f"Unknown action: {escape(action)}", status=400) diff --git a/netbox_librenms_plugin/views/mixins.py b/netbox_librenms_plugin/views/mixins.py index 73513f88df..99b045c6e5 100644 --- a/netbox_librenms_plugin/views/mixins.py +++ b/netbox_librenms_plugin/views/mixins.py @@ -287,32 +287,42 @@ class CacheMixin: A mixin class that provides caching functionality. """ - def get_cache_key(self, obj, data_type="ports"): + def get_cache_key(self, obj, data_type="ports", server_key=None): """ Get the cache key for the object. Args: obj: The object to cache data for - data_type: Type of data being cached ('ports' or 'links') + data_type: Type of data being cached ('ports', 'links', 'inventory', etc.) + server_key: Optional LibreNMS server key for namespacing per-server data """ model_name = obj._meta.model_name - return f"librenms_{data_type}_{model_name}_{obj.pk}" + base = f"librenms_{data_type}_{model_name}_{obj.pk}" + if server_key: + return f"{base}_{server_key}" + return base - def get_last_fetched_key(self, obj, data_type="ports"): + def get_last_fetched_key(self, obj, data_type="ports", server_key=None): """ Get the cache key for the last fetched time of the object. """ model_name = obj._meta.model_name - return f"librenms_{data_type}_last_fetched_{model_name}_{obj.pk}" + base = f"librenms_{data_type}_last_fetched_{model_name}_{obj.pk}" + if server_key: + return f"{base}_{server_key}" + return base - def get_vlan_overrides_key(self, obj): + def get_vlan_overrides_key(self, obj, server_key=None): """ Get the cache key for user VLAN group override selections. Stores a {vid_str: group_id_str} map so that "apply to all" VLAN - group choices persist across table pages. + group choices persist across table pages. Including server_key scopes + overrides per-server to avoid leakage when multiple servers are configured. """ model_name = obj._meta.model_name + if server_key: + return f"librenms_vlan_group_overrides_{model_name}_{obj.pk}_{server_key}" return f"librenms_vlan_group_overrides_{model_name}_{obj.pk}" diff --git a/netbox_librenms_plugin/views/object_sync/devices.py b/netbox_librenms_plugin/views/object_sync/devices.py index 97584f8319..3cdaa8c4e1 100644 --- a/netbox_librenms_plugin/views/object_sync/devices.py +++ b/netbox_librenms_plugin/views/object_sync/devices.py @@ -19,6 +19,7 @@ ) from netbox_librenms_plugin.utils import ( get_interface_name_field, + get_librenms_sync_device, get_missing_vlan_warning, get_tagged_vlan_css_class, get_untagged_vlan_css_class, @@ -79,13 +80,22 @@ def get_redirect_url(self, obj): def get_table(self, data, obj, interface_name_field, vlan_groups=None): """Return the appropriate interface table, selecting VC variant if needed.""" + server_key = self.librenms_api.server_key if hasattr(obj, "virtual_chassis") and obj.virtual_chassis: table = VCInterfaceTable( - data, device=obj, interface_name_field=interface_name_field, vlan_groups=vlan_groups + data, + device=obj, + interface_name_field=interface_name_field, + vlan_groups=vlan_groups, + server_key=server_key, ) else: table = LibreNMSInterfaceTable( - data, device=obj, interface_name_field=interface_name_field, vlan_groups=vlan_groups + data, + device=obj, + interface_name_field=interface_name_field, + vlan_groups=vlan_groups, + server_key=server_key, ) table.htmx_url = f"{self.request.path}?tab=interfaces" return table @@ -100,23 +110,19 @@ def post(self, request): selected_device_id = data.get("device_id") interface_name = data.get("interface_name") interface_name_field = data.get("interface_name_field") or get_interface_name_field() + server_key = data.get("server_key") or "default" if not selected_device_id: return JsonResponse({"status": "error", "message": "No device ID provided"}, status=400) selected_device = get_object_or_404(Device, pk=selected_device_id) - if selected_device.virtual_chassis: - primary_device = selected_device.virtual_chassis.master - if not primary_device or not primary_device.primary_ip: - primary_device = next( - (member for member in selected_device.virtual_chassis.members.all() if member.primary_ip), - None, - ) - else: + # Normalise to the VC sync device so cache keys match what the sync view stored + primary_device = get_librenms_sync_device(selected_device, server_key=server_key) + if primary_device is None: primary_device = selected_device - cached_data = cache.get(self.get_cache_key(primary_device, "ports")) + cached_data = cache.get(self.get_cache_key(primary_device, "ports", server_key)) if cached_data: port_data = next( @@ -130,6 +136,7 @@ def post(self, request): [], device=selected_device, interface_name_field=interface_name_field, + server_key=server_key, ) formatted_row = table.format_interface_data(port_data, selected_device) return JsonResponse({"status": "success", "formatted_row": formatted_row}) @@ -330,14 +337,20 @@ def post(self, request): data = json.loads(request.body) device_id = data.get("device_id") vid_group_map = data.get("vid_group_map", {}) + server_key = data.get("server_key") or "default" if not device_id: return JsonResponse({"status": "error", "message": "No device ID provided"}, status=400) device = get_object_or_404(Device, pk=device_id) + # Normalise to the VC sync device so cache keys match what the sync view stored + sync_device = get_librenms_sync_device(device, server_key=server_key) + if sync_device is None: + sync_device = device + # Use the remaining TTL of the ports cache so both expire together - ports_ttl = cache.ttl(self.get_cache_key(device, "ports")) + ports_ttl = cache.ttl(self.get_cache_key(sync_device, "ports", server_key)) if ports_ttl is None or ports_ttl <= 0: return JsonResponse( {"status": "error", "message": "No cached port data; refresh interfaces first"}, @@ -345,10 +358,10 @@ def post(self, request): ) # Merge with any existing overrides (user may save multiple times) - existing = cache.get(self.get_vlan_overrides_key(device)) or {} + existing = cache.get(self.get_vlan_overrides_key(sync_device, server_key)) or {} existing.update(vid_group_map) - cache.set(self.get_vlan_overrides_key(device), existing, timeout=ports_ttl) + cache.set(self.get_vlan_overrides_key(sync_device, server_key), existing, timeout=ports_ttl) return JsonResponse({"status": "success"}) diff --git a/netbox_librenms_plugin/views/object_sync/vms.py b/netbox_librenms_plugin/views/object_sync/vms.py index 354e1a3b5e..7e03f8de4e 100644 --- a/netbox_librenms_plugin/views/object_sync/vms.py +++ b/netbox_librenms_plugin/views/object_sync/vms.py @@ -53,7 +53,13 @@ class VMInterfaceTableView(BaseInterfaceTableView): def get_table(self, data, obj, interface_name_field, vlan_groups=None): """Return a VM interface table for the given data.""" - return LibreNMSVMInterfaceTable(data, device=obj, vlan_groups=vlan_groups) + return LibreNMSVMInterfaceTable( + data, + device=obj, + interface_name_field=interface_name_field, + vlan_groups=vlan_groups, + server_key=self.librenms_api.server_key, + ) def get_interfaces(self, obj): """Return all interfaces for the virtual machine.""" diff --git a/netbox_librenms_plugin/views/sync/cables.py b/netbox_librenms_plugin/views/sync/cables.py index 93139b1f63..95b6cea602 100644 --- a/netbox_librenms_plugin/views/sync/cables.py +++ b/netbox_librenms_plugin/views/sync/cables.py @@ -1,3 +1,5 @@ +import logging + from dcim.models import Cable, Device, Interface from django.contrib import messages from django.core.cache import cache @@ -7,10 +9,17 @@ from django.urls import reverse from django.views import View -from netbox_librenms_plugin.views.mixins import CacheMixin, LibreNMSPermissionMixin, NetBoxObjectPermissionMixin +from netbox_librenms_plugin.views.mixins import ( + CacheMixin, + LibreNMSAPIMixin, + LibreNMSPermissionMixin, + NetBoxObjectPermissionMixin, +) + +logger = logging.getLogger(__name__) -class SyncCablesView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, CacheMixin, View): +class SyncCablesView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, LibreNMSAPIMixin, CacheMixin, View): """Create NetBox cables using cached LibreNMS link data.""" required_object_permissions = { @@ -40,13 +49,18 @@ def get_selected_interfaces(self, request, initial_device): def get_cached_links_data(self, request, obj): """Return cached LibreNMS link data for the given object.""" - cached_data = cache.get(self.get_cache_key(obj, "links")) + server_key = self.librenms_api.server_key + cached_data = cache.get(self.get_cache_key(obj, "links", server_key)) if not cached_data: return None return cached_data.get("links", []) def create_cable(self, local_interface, remote_interface, request): - """Create a cable between local and remote interfaces.""" + """Create a cable between local and remote interfaces. + + Returns: + True on success, False on failure. + """ try: Cable.objects.create( a_terminations=[local_interface], @@ -92,7 +106,6 @@ def verify_cable_creation_requirements(self, link_data): """Return True if all required NetBox IDs are present in link data.""" required_fields = [ "netbox_local_interface_id", - "netbox_remote_device_id", "netbox_remote_interface_id", ] @@ -119,13 +132,21 @@ def handle_cable_creation(self, link_data, interface): return {"status": "missing_remote", "interface": display_name} def process_interface_sync(self, selected_interfaces, cached_links): - """Process cable sync for all selected interfaces and return results.""" + """Process cable sync for all selected interfaces and return results. + + Each interface is processed in its own atomic block so individual + failures roll back only that cable without affecting others. + """ results = {"valid": [], "invalid": [], "duplicate": [], "missing_remote": []} - with transaction.atomic(): - for interface in selected_interfaces: - result = self.process_single_interface(interface, cached_links) + for interface in selected_interfaces: + try: + with transaction.atomic(): + result = self.process_single_interface(interface, cached_links) results[result["status"]].append(result.get("interface", "")) + except Exception: + logger.exception("Failed to sync cable for port_id %s", interface.get("local_port_id", "")) + results["invalid"].append(interface.get("local_port_id", "")) return results diff --git a/netbox_librenms_plugin/views/sync/device_fields.py b/netbox_librenms_plugin/views/sync/device_fields.py index 445aa01434..4bdb8f9b55 100644 --- a/netbox_librenms_plugin/views/sync/device_fields.py +++ b/netbox_librenms_plugin/views/sync/device_fields.py @@ -4,10 +4,13 @@ from django.contrib import messages from django.core.exceptions import ValidationError from django.db import IntegrityError, transaction +from django.db.models import Q +from django.http import HttpResponse from django.shortcuts import get_object_or_404, redirect from django.views import View +from virtualization.models import VirtualMachine -from netbox_librenms_plugin.utils import match_librenms_hardware_to_device_type +from netbox_librenms_plugin.utils import match_librenms_hardware_to_device_type, migrate_legacy_librenms_id from netbox_librenms_plugin.views.mixins import LibreNMSAPIMixin, LibreNMSPermissionMixin, NetBoxObjectPermissionMixin logger = logging.getLogger(__name__) @@ -340,7 +343,6 @@ def post(self, request, pk): f"Platform '{platform_name}' could not be created: {e}", ) return redirect("plugins:netbox_librenms_plugin:device_librenms_sync", pk=pk) - messages.success( request, f"Created platform '{platform}' and assigned to device", @@ -423,3 +425,214 @@ def post(self, request, pk): messages.info(request, "No serial assignments were made") return redirect("plugins:netbox_librenms_plugin:device_librenms_sync", pk=pk) + + +class RemoveServerMappingView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, View): + """Remove a single server entry from the device's (or VM's) librenms_id custom field dict.""" + + required_object_permissions = { + "POST": [("change", Device), ("change", VirtualMachine)], + } + + def _get_object(self, object_type, pk): + """Return the Device or VirtualMachine for the given pk.""" + model = VirtualMachine if object_type == "vm" else Device + return get_object_or_404(model, pk=pk), model + + def _sync_url_name(self, object_type): + if object_type == "vm": + return "plugins:netbox_librenms_plugin:vm_librenms_sync" + return "plugins:netbox_librenms_plugin:device_librenms_sync" + + def _normalize_librenms_mapping(self, value): + if isinstance(value, int): + return {"default": value} + if isinstance(value, str): + try: + return {"default": int(value)} + except (TypeError, ValueError): + return {} + return value if isinstance(value, dict) else {} + + def post(self, request, pk): + # Scope required permissions to the specific model being modified before checking. + object_type = request.POST.get("object_type", "device") + if object_type == "virtualmachine": + object_type = "vm" + if object_type not in ("device", "vm"): + return HttpResponse(f"Invalid object_type: {object_type!r}", status=400) + target_model = VirtualMachine if object_type == "vm" else Device + self.required_object_permissions = {"POST": [("change", target_model)]} + + if error := self.require_all_permissions("POST"): + return error + + obj, model = self._get_object(object_type, pk) + sync_url = self._sync_url_name(object_type) + server_key = request.POST.get("server_key", "").strip() + + if not server_key: + messages.error(request, "No server_key provided.") + return redirect(sync_url, pk=pk) + + cf_value = self._normalize_librenms_mapping(obj.custom_field_data.get("librenms_id")) + if not isinstance(cf_value, dict) or server_key not in cf_value: + messages.warning(request, f"No mapping found for server '{server_key}'.") + return redirect(sync_url, pk=pk) + + # Refuse to remove mappings for servers that are still configured in the plugin. + # Only orphaned (unconfigured) mappings may be removed via this endpoint. + # Guard both multi-server mode (servers dict) and legacy single-server mode + # (top-level librenms_url in plugin config, which implicitly defines "default") + # but only when no servers section is configured (pure legacy mode). + from django.conf import settings as django_settings + + plugins_cfg = django_settings.PLUGINS_CONFIG.get("netbox_librenms_plugin", {}) + configured_servers = plugins_cfg.get("servers") or {} + if not isinstance(configured_servers, dict): + configured_servers = {} + legacy_url_configured = bool(plugins_cfg.get("librenms_url")) + if server_key in configured_servers or ( + legacy_url_configured and not configured_servers and server_key == "default" + ): + messages.error( + request, + f"Cannot remove mapping for configured server '{server_key}'. " + "Remove the server from plugin configuration first, then retry.", + ) + return redirect(sync_url, pk=pk) + + with transaction.atomic(): + try: + obj_locked = model.objects.select_for_update().get(pk=pk) + except model.DoesNotExist: + messages.error(request, f"{model.__name__} no longer exists.") + return redirect(sync_url, pk=pk) + cf = self._normalize_librenms_mapping(obj_locked.custom_field_data.get("librenms_id")) + # Re-check after acquiring lock; mirror the pre-transaction protection logic + _is_protected = server_key in configured_servers or ( + legacy_url_configured and not configured_servers and server_key == "default" + ) + if isinstance(cf, dict) and server_key in cf and not _is_protected: + del cf[server_key] + obj_locked.custom_field_data["librenms_id"] = cf if cf else None + try: + obj_locked.full_clean() + obj_locked.save() + except ValidationError as exc: + transaction.set_rollback(True) + logger.error("Validation error removing LibreNMS mapping for server %r: %s", server_key, exc) + messages.error(request, "Validation error removing LibreNMS mapping.") + return redirect(sync_url, pk=pk) + except Exception: + transaction.set_rollback(True) + logger.exception("Unexpected error removing LibreNMS mapping for server %r", server_key) + messages.error(request, "An unexpected error occurred while removing the LibreNMS mapping.") + return redirect(sync_url, pk=pk) + messages.success(request, f"Removed LibreNMS mapping for server '{server_key}'.") + else: + messages.warning(request, f"Mapping for server '{server_key}' was already removed.") + + return redirect(sync_url, pk=pk) + + +class ConvertLegacyLibreNMSIdView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, LibreNMSAPIMixin, View): + """Convert a legacy bare-integer librenms_id to the server-scoped JSON dict format. + + Only allowed when the NetBox serial matches the LibreNMS serial, so the + association can be verified before scoping the ID to the active server. + """ + + required_object_permissions = { + "POST": [("change", Device), ("change", VirtualMachine)], + } + + def _get_model_and_object(self, object_type, pk): + model = VirtualMachine if object_type == "vm" else Device + return model, get_object_or_404(model, pk=pk) + + def _sync_url(self, object_type, pk): + name = "vm_librenms_sync" if object_type == "vm" else "device_librenms_sync" + return redirect(f"plugins:netbox_librenms_plugin:{name}", pk=pk) + + def post(self, request, pk): + object_type = request.POST.get("object_type", "device") + if object_type == "virtualmachine": + object_type = "vm" + if object_type not in ("device", "vm"): + return HttpResponse(f"Invalid object_type: {object_type!r}", status=400) + + target_model = VirtualMachine if object_type == "vm" else Device + self.required_object_permissions = {"POST": [("change", target_model)]} + if error := self.require_all_permissions("POST"): + return error + + model, obj = self._get_model_and_object(object_type, pk) + server_key = self.librenms_api.server_key + + # Verify the device actually has a legacy bare-int librenms_id + cf_value = obj.custom_field_data.get("librenms_id") + if not isinstance(cf_value, (int, str)) or isinstance(cf_value, bool): + messages.warning(request, "librenms_id is already in the server-scoped JSON format.") + return self._sync_url(object_type, pk) + if isinstance(cf_value, str): + if not cf_value.isdigit(): + messages.error(request, "librenms_id is not a valid integer; cannot convert.") + return self._sync_url(object_type, pk) + + # Verify serial match before converting + librenms_id = int(cf_value) if isinstance(cf_value, str) else cf_value + success, device_info = self.librenms_api.get_device_info(librenms_id) + if not success or not device_info: + messages.error(request, "Could not retrieve device info from LibreNMS to verify serial.") + return self._sync_url(object_type, pk) + + librenms_serial = (device_info.get("serial") or "").strip() + netbox_serial = (obj.serial or "").strip() + if not netbox_serial or not librenms_serial or netbox_serial != librenms_serial: + messages.error( + request, + "Serial number mismatch — cannot convert legacy ID without serial confirmation.", + ) + return self._sync_url(object_type, pk) + + with transaction.atomic(): + try: + locked = model.objects.select_for_update().get(pk=pk) + except model.DoesNotExist: + messages.error(request, f"{model.__name__} no longer exists.") + return self._sync_url(object_type, pk) + # Check that no other object already owns this ID on this server + conflict = ( + model.objects.filter( + Q(**{f"custom_field_data__librenms_id__{server_key}": librenms_id}) + | Q(**{f"custom_field_data__librenms_id__{server_key}": str(librenms_id)}) + ) + .exclude(pk=locked.pk) + .exists() + ) + if conflict: + transaction.set_rollback(True) + messages.error( + request, + f"Another {model.__name__} already has librenms_id {librenms_id} " + f"for server '{server_key}'; cannot convert.", + ) + return self._sync_url(object_type, pk) + migrated = migrate_legacy_librenms_id(locked, server_key) + if not migrated: + messages.warning(request, "librenms_id is already in the server-scoped JSON format.") + return self._sync_url(object_type, pk) + try: + locked.full_clean() + locked.save() + except (ValidationError, Exception) as exc: + transaction.set_rollback(True) + messages.error(request, f"Failed to save converted librenms_id: {exc}") + return self._sync_url(object_type, pk) + + messages.success( + request, + f"Converted legacy librenms_id {librenms_id} → {{'{server_key}': {librenms_id}}}.", + ) + return self._sync_url(object_type, pk) diff --git a/netbox_librenms_plugin/views/sync/interfaces.py b/netbox_librenms_plugin/views/sync/interfaces.py index 29269742b3..e255199d9a 100644 --- a/netbox_librenms_plugin/views/sync/interfaces.py +++ b/netbox_librenms_plugin/views/sync/interfaces.py @@ -9,7 +9,7 @@ from virtualization.models import VirtualMachine, VMInterface from netbox_librenms_plugin.models import InterfaceTypeMapping -from netbox_librenms_plugin.utils import convert_speed_to_kbps, get_interface_name_field +from netbox_librenms_plugin.utils import convert_speed_to_kbps, get_interface_name_field, set_librenms_device_id from netbox_librenms_plugin.views.mixins import ( CacheMixin, LibreNMSAPIMixin, @@ -52,6 +52,10 @@ def post(self, request, object_type, object_id): obj = self.get_object(object_type, object_id) self.object = obj # Store for use in sync methods + # Read server_key from POST so we use the exact server the user was viewing + server_key = request.POST.get("server_key") or self.librenms_api.server_key + self._post_server_key = server_key + interface_name_field = get_interface_name_field(request) self.interface_name_field = interface_name_field selected_interfaces = self.get_selected_interfaces(request, interface_name_field) @@ -63,7 +67,7 @@ def post(self, request, object_type, object_id): + f"?tab=interfaces&interface_name_field={interface_name_field}" ) - ports_data = self.get_cached_ports_data(request, obj) + ports_data = self.get_cached_ports_data(request, obj, server_key) if ports_data is None: return redirect( reverse(url_name, kwargs={"pk": object_id}) @@ -98,9 +102,11 @@ def get_selected_interfaces(self, request, interface_name_field): return None return selected_interfaces - def get_cached_ports_data(self, request, obj): + def get_cached_ports_data(self, request, obj, server_key=None): """Return cached LibreNMS port data for the given object.""" - cached_data = cache.get(self.get_cache_key(obj, "ports")) + if server_key is None: + server_key = self.librenms_api.server_key + cached_data = cache.get(self.get_cache_key(obj, "ports", server_key)) if not cached_data: messages.warning( request, @@ -232,8 +238,10 @@ def update_interface_attributes( else: setattr(interface, netbox_key, librenms_interface.get(librenms_key)) - if "librenms_id" in interface.cf: - interface.custom_field_data["librenms_id"] = librenms_interface.get("port_id") + port_id = librenms_interface.get("port_id") + if port_id is not None: + server_key = getattr(self, "_post_server_key", None) or self.librenms_api.server_key + set_librenms_device_id(interface, port_id, server_key) if "enabled" not in exclude_columns: admin_status = librenms_interface.get("ifAdminStatus") diff --git a/netbox_librenms_plugin/views/sync/ip_addresses.py b/netbox_librenms_plugin/views/sync/ip_addresses.py index 474a3a446c..ddaf15de30 100644 --- a/netbox_librenms_plugin/views/sync/ip_addresses.py +++ b/netbox_librenms_plugin/views/sync/ip_addresses.py @@ -9,10 +9,15 @@ from ipam.models import VRF, IPAddress from virtualization.models import VirtualMachine, VMInterface -from netbox_librenms_plugin.views.mixins import CacheMixin, LibreNMSPermissionMixin, NetBoxObjectPermissionMixin +from netbox_librenms_plugin.views.mixins import ( + CacheMixin, + LibreNMSAPIMixin, + LibreNMSPermissionMixin, + NetBoxObjectPermissionMixin, +) -class SyncIPAddressesView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, CacheMixin, View): +class SyncIPAddressesView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, LibreNMSAPIMixin, CacheMixin, View): """Synchronize IP addresses from LibreNMS cache into NetBox.""" required_object_permissions = { @@ -40,7 +45,8 @@ def get_vrf_selection(self, request, ip_address): def get_cached_ip_data(self, request, obj): """Return cached LibreNMS IP address data for the given object.""" - cached_data = cache.get(self.get_cache_key(obj, "ip_addresses")) + server_key = self.librenms_api.server_key + cached_data = cache.get(self.get_cache_key(obj, "ip_addresses", server_key)) if not cached_data: return None return cached_data.get("ip_addresses", []) diff --git a/netbox_librenms_plugin/views/sync/vlans.py b/netbox_librenms_plugin/views/sync/vlans.py index c5fae91530..948e68c8f1 100644 --- a/netbox_librenms_plugin/views/sync/vlans.py +++ b/netbox_librenms_plugin/views/sync/vlans.py @@ -8,10 +8,15 @@ from django.views import View from ipam.models import VLAN, VLANGroup -from netbox_librenms_plugin.views.mixins import CacheMixin, LibreNMSPermissionMixin, NetBoxObjectPermissionMixin +from netbox_librenms_plugin.views.mixins import ( + CacheMixin, + LibreNMSAPIMixin, + LibreNMSPermissionMixin, + NetBoxObjectPermissionMixin, +) -class SyncVLANsView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, CacheMixin, View): +class SyncVLANsView(LibreNMSPermissionMixin, NetBoxObjectPermissionMixin, LibreNMSAPIMixin, CacheMixin, View): """ Handle POST requests to create/update VLANs in NetBox from LibreNMS data. """ @@ -73,7 +78,7 @@ def _handle_create_vlans(self, request, obj, object_type, object_id): return self._redirect(object_type, object_id) # Get cached VLAN data - cached_vlans = cache.get(self.get_cache_key(obj, "vlans")) + cached_vlans = cache.get(self.get_cache_key(obj, "vlans", self.librenms_api.server_key)) if not cached_vlans: messages.error(request, "No cached VLAN data. Please refresh VLANs first.") return self._redirect(object_type, object_id) From 5e3ad5cd28fa24a03b13af1d7ac80d4fa348ecf1 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 7 Mar 2026 12:55:47 +0100 Subject: [PATCH 05/19] fix: strip stale derived fields in cable verify POST, preserve server_key in sync redirects SingleCableVerifyView.post() now strips derived fields from cached link data and re-enriches remote side from current NetBox state, preventing DoesNotExist when remote devices/interfaces are deleted after caching. Sync redirect URLs for interfaces, cables, IP addresses, and VLANs now preserve the server_key query parameter so users return to the correct multi-server tab. --- .../views/base/cables_view.py | 19 ++++++++++++++++++ netbox_librenms_plugin/views/sync/cables.py | 14 +++++++------ .../views/sync/interfaces.py | 20 +++++++++---------- .../views/sync/ip_addresses.py | 6 +++++- netbox_librenms_plugin/views/sync/vlans.py | 6 +++++- 5 files changed, 46 insertions(+), 19 deletions(-) diff --git a/netbox_librenms_plugin/views/base/cables_view.py b/netbox_librenms_plugin/views/base/cables_view.py index 247dceb90c..dc478f9317 100644 --- a/netbox_librenms_plugin/views/base/cables_view.py +++ b/netbox_librenms_plugin/views/base/cables_view.py @@ -408,6 +408,25 @@ def post(self, request): None, ) if link_data: + # Strip derived fields from cached data to avoid stale + # IDs/URLs when NetBox objects are deleted after caching. + _raw_keys = { + "local_port", + "local_port_id", + "remote_port", + "remote_device", + "remote_port_id", + "remote_device_id", + } + link_data = {k: v for k, v in link_data.items() if k in _raw_keys} + + # Re-enrich remote side from current NetBox state + remote_hostname = link_data.get("remote_device", "") + if remote_hostname: + link_data = self.process_remote_device( + link_data, remote_hostname, link_data.get("remote_device_id") + ) + local_port = link_data.get("local_port", "") formatted_row["local_port"] = local_port diff --git a/netbox_librenms_plugin/views/sync/cables.py b/netbox_librenms_plugin/views/sync/cables.py index 95b6cea602..0e9dc884fa 100644 --- a/netbox_librenms_plugin/views/sync/cables.py +++ b/netbox_librenms_plugin/views/sync/cables.py @@ -157,20 +157,22 @@ def post(self, request, pk): return error initial_device = get_object_or_404(Device, pk=pk) + server_key = self.librenms_api.server_key + redirect_url = ( + f"{reverse('plugins:netbox_librenms_plugin:device_librenms_sync', args=[initial_device.pk])}?tab=cables" + + (f"&server_key={server_key}" if server_key else "") + ) + selected_interfaces = self.get_selected_interfaces(request, initial_device) cached_links = self.get_cached_links_data(request, initial_device) if not self.validate_prerequisites(cached_links, selected_interfaces): - return redirect( - f"{reverse('plugins:netbox_librenms_plugin:device_librenms_sync', args=[initial_device.pk])}?tab=cables" - ) + return redirect(redirect_url) results = self.process_interface_sync(selected_interfaces, cached_links) self.display_sync_results(request, results) - return redirect( - f"{reverse('plugins:netbox_librenms_plugin:device_librenms_sync', args=[initial_device.pk])}?tab=cables" - ) + return redirect(redirect_url) def display_sync_results(self, request, results): """Display flash messages summarizing the cable sync results.""" diff --git a/netbox_librenms_plugin/views/sync/interfaces.py b/netbox_librenms_plugin/views/sync/interfaces.py index e255199d9a..3f0c93fa4c 100644 --- a/netbox_librenms_plugin/views/sync/interfaces.py +++ b/netbox_librenms_plugin/views/sync/interfaces.py @@ -61,18 +61,18 @@ def post(self, request, object_type, object_id): selected_interfaces = self.get_selected_interfaces(request, interface_name_field) exclude_columns = request.POST.getlist("exclude_columns") + redirect_url = ( + reverse(url_name, kwargs={"pk": object_id}) + + f"?tab=interfaces&interface_name_field={interface_name_field}" + + (f"&server_key={server_key}" if server_key else "") + ) + if selected_interfaces is None: - return redirect( - reverse(url_name, kwargs={"pk": object_id}) - + f"?tab=interfaces&interface_name_field={interface_name_field}" - ) + return redirect(redirect_url) ports_data = self.get_cached_ports_data(request, obj, server_key) if ports_data is None: - return redirect( - reverse(url_name, kwargs={"pk": object_id}) - + f"?tab=interfaces&interface_name_field={interface_name_field}" - ) + return redirect(redirect_url) # Prepare VLAN lookup maps if VLAN sync is enabled vlan_groups = self.get_vlan_groups_for_device(obj) @@ -82,9 +82,7 @@ def post(self, request, object_type, object_id): self.sync_selected_interfaces(obj, selected_interfaces, ports_data, exclude_columns, interface_name_field) messages.success(request, "Selected interfaces synced successfully.") - return redirect( - reverse(url_name, kwargs={"pk": object_id}) + f"?tab=interfaces&interface_name_field={interface_name_field}" - ) + return redirect(redirect_url) def get_object(self, object_type, object_id): """Return the Device or VirtualMachine for the given type and ID.""" diff --git a/netbox_librenms_plugin/views/sync/ip_addresses.py b/netbox_librenms_plugin/views/sync/ip_addresses.py index ddaf15de30..ff8c9a1a6c 100644 --- a/netbox_librenms_plugin/views/sync/ip_addresses.py +++ b/netbox_librenms_plugin/views/sync/ip_addresses.py @@ -65,7 +65,11 @@ def get_ip_tab_url(self, obj): url_name = "plugins:netbox_librenms_plugin:device_librenms_sync" else: url_name = "plugins:netbox_librenms_plugin:vm_librenms_sync" - return f"{reverse(url_name, args=[obj.pk])}?tab=ipaddresses" + server_key = self.librenms_api.server_key + url = f"{reverse(url_name, args=[obj.pk])}?tab=ipaddresses" + if server_key: + url += f"&server_key={server_key}" + return url def post(self, request, object_type, pk): """Sync selected IP addresses from LibreNMS into NetBox.""" diff --git a/netbox_librenms_plugin/views/sync/vlans.py b/netbox_librenms_plugin/views/sync/vlans.py index 948e68c8f1..b4c2e63c67 100644 --- a/netbox_librenms_plugin/views/sync/vlans.py +++ b/netbox_librenms_plugin/views/sync/vlans.py @@ -63,7 +63,11 @@ def _redirect(self, object_type: str, object_id: int): if object_type == "device" else "plugins:netbox_librenms_plugin:vm_librenms_sync" ) - return redirect(reverse(url_name, kwargs={"pk": object_id}) + "?tab=vlans") + server_key = self.librenms_api.server_key + url = reverse(url_name, kwargs={"pk": object_id}) + "?tab=vlans" + if server_key: + url += f"&server_key={server_key}" + return redirect(url) def _handle_create_vlans(self, request, obj, object_type, object_id): """ From 5c6f9a9ac5d5e10f2719ecdbbe54432b1b730c17 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 7 Mar 2026 13:01:41 +0100 Subject: [PATCH 06/19] fix: escape LibreNMS labels in cable verify HTML to prevent XSS Add regression tests for stale-field stripping and XSS escaping in SingleCableVerifyView.post(). --- .../tests/test_cable_verify.py | 261 ++++++++++++++++++ 1 file changed, 261 insertions(+) create mode 100644 netbox_librenms_plugin/tests/test_cable_verify.py diff --git a/netbox_librenms_plugin/tests/test_cable_verify.py b/netbox_librenms_plugin/tests/test_cable_verify.py new file mode 100644 index 0000000000..be4bb0d603 --- /dev/null +++ b/netbox_librenms_plugin/tests/test_cable_verify.py @@ -0,0 +1,261 @@ +"""Regression tests for SingleCableVerifyView.post(). + +Covers: +- Stale derived fields are stripped before re-enrichment (prevents + DoesNotExist when remote objects are deleted after caching). +- LibreNMS-sourced labels are HTML-escaped to prevent XSS. +""" + +import json +from unittest.mock import MagicMock, patch + + +def _make_view(server_key="default"): + """Create a SingleCableVerifyView instance without database access.""" + from netbox_librenms_plugin.views.base.cables_view import SingleCableVerifyView + + view = object.__new__(SingleCableVerifyView) + view._librenms_api = MagicMock() + view._librenms_api.server_key = server_key + view.request = MagicMock() + return view + + +def _make_request(body_dict): + """Create a mock POST request with JSON body.""" + request = MagicMock() + request.method = "POST" + request.body = json.dumps(body_dict).encode() + request.META = {"HTTP_X_REQUESTED_WITH": "XMLHttpRequest"} + return request + + +class TestStaleFieldStripping: + """Cached link data with stale derived fields must be stripped before use.""" + + def test_stale_remote_fields_stripped_before_enrichment(self): + """Stale netbox_remote_device_id / remote_device_url must not reach check_cable_status().""" + view = _make_view() + + # Cached link with stale derived fields (from a previous enrichment) + cached_link = { + "local_port": "eth0", + "local_port_id": 100, + "remote_port": "eth1", + "remote_device": "switch-remote", + "remote_port_id": 200, + "remote_device_id": 42, + # Stale derived fields — remote device was deleted after caching + "netbox_remote_device_id": 999, + "remote_device_url": "/dcim/devices/999/", + "netbox_remote_interface_id": 888, + "remote_port_url": "/dcim/interfaces/888/", + "cable_status": "No Cable", + "can_create_cable": True, + } + + cached_data = {"links": [cached_link]} + + device = MagicMock() + device.pk = 1 + device.id = 1 + device.virtual_chassis = None + interface_mock = MagicMock() + interface_mock.pk = 10 + + # Track what link_data check_cable_status receives + received_link_data = {} + + def fake_check_cable_status(link): + received_link_data.update(link) + link["cable_status"] = "No Cable" + link["can_create_cable"] = True + return link + + def fake_process_remote_device(link, hostname, device_id): + # Simulate successful remote enrichment with fresh IDs + link["remote_device_url"] = "/dcim/devices/777/" + link["netbox_remote_device_id"] = 777 + link["remote_port_url"] = "/dcim/interfaces/666/" + link["netbox_remote_interface_id"] = 666 + link["remote_port_name"] = "eth1" + return link + + request = _make_request({"device_id": 1, "local_port_id": 100}) + + with ( + patch("netbox_librenms_plugin.views.base.cables_view.get_object_or_404", return_value=device), + patch("netbox_librenms_plugin.views.base.cables_view.cache") as mock_cache, + patch.object(view, "get_cache_key", return_value="test_key"), + patch.object(view, "check_cable_status", side_effect=fake_check_cable_status), + patch.object(view, "process_remote_device", side_effect=fake_process_remote_device), + patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device", return_value=device), + patch("netbox_librenms_plugin.views.base.cables_view.get_virtual_chassis_member", return_value=device), + patch("netbox_librenms_plugin.views.base.cables_view._librenms_id_q", return_value=MagicMock()), + patch("netbox_librenms_plugin.views.base.cables_view.get_token", return_value="csrf123"), + patch("netbox_librenms_plugin.views.base.cables_view.reverse", return_value="/fake/"), + ): + mock_cache.get.return_value = cached_data + # Make the interface filter return our mock + device.interfaces.filter.return_value.first.return_value = interface_mock + + view.post(request) + + # check_cable_status should have received fresh IDs from process_remote_device, + # NOT the stale 999/888 from cache + assert received_link_data.get("netbox_remote_device_id") == 777 + assert received_link_data.get("netbox_remote_interface_id") == 666 + + def test_raw_keys_match_prepare_context(self): + """The _raw_keys set in post() must match the one in _prepare_context().""" + import inspect + + from netbox_librenms_plugin.views.base.cables_view import BaseCableTableView, SingleCableVerifyView + + # Extract _raw_keys from _prepare_context source + prepare_src = inspect.getsource(BaseCableTableView._prepare_context) + post_src = inspect.getsource(SingleCableVerifyView.post) + + # Both should contain the same set of raw keys + expected_keys = { + "local_port", + "local_port_id", + "remote_port", + "remote_device", + "remote_port_id", + "remote_device_id", + } + for key in expected_keys: + assert f'"{key}"' in prepare_src, f"{key} missing from _prepare_context _raw_keys" + assert f'"{key}"' in post_src, f"{key} missing from post() _raw_keys" + + +class TestXSSEscaping: + """LibreNMS-sourced labels must be HTML-escaped in cable verify output.""" + + def test_xss_in_local_port_name_escaped(self): + """A malicious local_port name must be escaped in the HTML output.""" + view = _make_view() + + xss_port_name = '' + cached_link = { + "local_port": xss_port_name, + "local_port_id": 100, + "remote_port": "eth1", + "remote_device": "safe-switch", + "remote_port_id": 200, + "remote_device_id": 42, + } + + cached_data = {"links": [cached_link]} + + device = MagicMock() + device.pk = 1 + device.id = 1 + device.virtual_chassis = None + interface_mock = MagicMock() + interface_mock.pk = 10 + + def fake_process_remote_device(link, hostname, device_id): + link["remote_device_url"] = "/dcim/devices/2/" + link["netbox_remote_device_id"] = 2 + link["remote_port_url"] = "/dcim/interfaces/20/" + link["netbox_remote_interface_id"] = 20 + link["remote_port_name"] = "eth1" + return link + + def fake_check_cable_status(link): + link["cable_status"] = "No Cable" + link["can_create_cable"] = False + return link + + request = _make_request({"device_id": 1, "local_port_id": 100}) + + with ( + patch("netbox_librenms_plugin.views.base.cables_view.get_object_or_404", return_value=device), + patch("netbox_librenms_plugin.views.base.cables_view.cache") as mock_cache, + patch.object(view, "get_cache_key", return_value="test_key"), + patch.object(view, "check_cable_status", side_effect=fake_check_cable_status), + patch.object(view, "process_remote_device", side_effect=fake_process_remote_device), + patch("netbox_librenms_plugin.views.base.cables_view.get_librenms_sync_device", return_value=device), + patch("netbox_librenms_plugin.views.base.cables_view._librenms_id_q", return_value=MagicMock()), + patch("netbox_librenms_plugin.views.base.cables_view.get_token", return_value="csrf123"), + patch("netbox_librenms_plugin.views.base.cables_view.reverse", return_value="/fake/"), + ): + mock_cache.get.return_value = cached_data + device.interfaces.filter.return_value.first.return_value = interface_mock + + response = view.post(request) + + content = json.loads(response.content) + row = content.get("formatted_row", {}) + local_port_html = row.get("local_port", "") + + # The raw script tag must NOT appear unescaped + assert "