From 08441433bcea268ffaac79f8b872483033401557 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Tue, 17 Feb 2026 12:40:34 +0100 Subject: [PATCH 01/25] feat: auto-create librenms_id custom field via post_migrate signal Add a post_migrate signal handler that automatically creates the 'librenms_id' custom field for Device, VirtualMachine, Interface, and VMInterface objects when migrations are run. This eliminates the need for manual custom field creation and ensures the field exists with correct defaults (integer type, ui_visible='if-set', ui_editable='yes') across all installations. The handler: - Uses get_or_create to be idempotent - Runs only once per migrate invocation via _executed flag - Ensures all required content types are assigned - Catches exceptions to avoid breaking startup during initial migration - Logs creation via the plugin logger Also updates documentation to reflect the automatic creation and marks the manual setup section as legacy. Includes 6 unit tests covering: - Custom field creation with correct defaults - Skip when already executed (dedup) - Existing field not recreated - Missing content types added - Exception handling (no propagation) - No logging when field already exists --- docs/usage_tips/custom_field.md | 8 +- netbox_librenms_plugin/__init__.py | 66 +++++++++ netbox_librenms_plugin/tests/test_init.py | 170 ++++++++++++++++++++++ 3 files changed, 243 insertions(+), 1 deletion(-) create mode 100644 netbox_librenms_plugin/tests/test_init.py diff --git a/docs/usage_tips/custom_field.md b/docs/usage_tips/custom_field.md index 032812ed82..7ed27a2f97 100644 --- a/docs/usage_tips/custom_field.md +++ b/docs/usage_tips/custom_field.md @@ -4,6 +4,9 @@ To enhance device identification and synchronization between NetBox and LibreNMS, this plugin supports using a custom field `librenms_id` on Device, Virtual Machine and Interface objects. While the plugin works without it, using this custom field is recommended for LibreNMS API lookups, and to assist with matching the remote device and remote interfaces for cable creation in Netbox. It can also be entered manually if no primary IP or FQDN is available. +!!! info "Automatic Creation" + As of version 0.4.2, the plugin **automatically creates** the `librenms_id` custom field when migrations are run. You no longer need to create it manually. The field is created for Device, Virtual Machine, Interface, and VM Interface objects. + For the Device and Virtual Machine objects the plugin will automatically populate the LibreNMS ID custom field when opening the LibreNMS Sync page if the device has been found in LibreNMS. For the Interface object, the plugin will automatically populate the LibreNMS ID custom field when the interface data is synced from LibreNMS. @@ -15,7 +18,10 @@ For the Interface object, the plugin will automatically populate the LibreNMS ID - **Efficient Synchronization:** Enhances the reliability of API lookups. - **Cable creation:** Allows better device identification for the creation of cables between NetBox devices. -## Suggested Custom Field Setup +## Manual Custom Field Setup (Legacy) + +!!! note + This section is only needed if you are running an older version of the plugin that does not auto-create the field, or if you need to recreate it after deletion. Follow these steps to create the `librenms_id` custom field in NetBox: diff --git a/netbox_librenms_plugin/__init__.py b/netbox_librenms_plugin/__init__.py index f1720c85d6..5a2c2532c2 100644 --- a/netbox_librenms_plugin/__init__.py +++ b/netbox_librenms_plugin/__init__.py @@ -28,6 +28,7 @@ def ready(self): super().ready() from django.conf import settings + from django.db.models.signals import post_migrate plugin_config = getattr(settings, "PLUGINS_CONFIG", {}).get(self.name, {}) @@ -37,6 +38,12 @@ def ready(self): else: self._validate_legacy_config(plugin_config) + # Auto-create the librenms_id custom field after migrations complete + post_migrate.connect( + _ensure_librenms_id_custom_field, + dispatch_uid="netbox_librenms_plugin_ensure_cf", + ) + def _validate_multi_server_config(self, servers_config): """Validate multi-server configuration.""" if not servers_config or not isinstance(servers_config, dict): @@ -61,4 +68,63 @@ def _validate_legacy_config(self, plugin_config): ) +def _ensure_librenms_id_custom_field(sender, **kwargs): + """ + Auto-create the 'librenms_id' custom field if it doesn't exist. + Runs after migrations via post_migrate signal to ensure tables exist. + Uses dispatch_uid to avoid duplicate connections. + """ + # Only run once per migrate invocation (post_migrate fires per-app). + # The _executed flag is intentionally never reset: migrations are expected to + # run in short-lived CLI processes (manage.py migrate) where the flag is + # naturally cleared on exit. Long-running processes (e.g. gunicorn workers) + # should not rely on this handler re-executing after startup. + if getattr(_ensure_librenms_id_custom_field, "_executed", False): + return + _ensure_librenms_id_custom_field._executed = True # not reset; see comment above + + try: + from django.contrib.contenttypes.models import ContentType + + from extras.models import CustomField + + cf, created = CustomField.objects.get_or_create( + name="librenms_id", + defaults={ + "type": "integer", + "label": "LibreNMS ID", + "description": "LibreNMS Device ID for synchronization (auto-created by plugin)", + "required": False, + "ui_visible": "if-set", + "ui_editable": "yes", + "is_cloneable": False, + }, + ) + + # Ensure the field is assigned to the required object types + from dcim.models import Device, Interface + from virtualization.models import VirtualMachine, VMInterface + + required_models = [Device, VirtualMachine, Interface, VMInterface] + current_types = set(cf.object_types.values_list("pk", flat=True)) + + for model in required_models: + ct = ContentType.objects.get_for_model(model) + if ct.pk not in current_types: + cf.object_types.add(ct) + + if created: + import logging + + logging.getLogger("netbox_librenms_plugin").info( + "Auto-created 'librenms_id' custom field for Device, VirtualMachine, Interface, VMInterface" + ) + except Exception as e: + # Don't break startup if custom field creation fails (e.g., during initial migration), + # but log the error so it's not silently swallowed. + import logging + + logging.getLogger("netbox_librenms_plugin").exception("Failed to auto-create 'librenms_id' custom field: %s", e) + + config = LibreNMSSyncConfig diff --git a/netbox_librenms_plugin/tests/test_init.py b/netbox_librenms_plugin/tests/test_init.py new file mode 100644 index 0000000000..ed5612f81d --- /dev/null +++ b/netbox_librenms_plugin/tests/test_init.py @@ -0,0 +1,170 @@ +"""Tests for netbox_librenms_plugin.__init__ module. + +Covers the _ensure_librenms_id_custom_field post_migrate signal handler. +""" + +from unittest.mock import MagicMock, patch + + +# ============================================================================= +# TestEnsureLibreNMSIdCustomField - 6 tests +# ============================================================================= + + +class TestEnsureLibreNMSIdCustomField: + """Test _ensure_librenms_id_custom_field signal handler.""" + + def setup_method(self): + """Reset the _executed flag before each test for consistent isolation.""" + from netbox_librenms_plugin import _ensure_librenms_id_custom_field + + _ensure_librenms_id_custom_field._executed = False + + @patch("dcim.models.Interface", new_callable=MagicMock) + @patch("dcim.models.Device", new_callable=MagicMock) + @patch("virtualization.models.VMInterface", new_callable=MagicMock) + @patch("virtualization.models.VirtualMachine", new_callable=MagicMock) + @patch("django.contrib.contenttypes.models.ContentType") + @patch("extras.models.CustomField") + def test_creates_custom_field_when_missing( + self, MockCustomField, MockContentType, mock_vm, mock_vmif, mock_device, mock_iface + ): + """Custom field is created with correct defaults when it does not exist.""" + from netbox_librenms_plugin import _ensure_librenms_id_custom_field + + mock_cf = MagicMock() + mock_cf.object_types.values_list.return_value = [] + MockCustomField.objects.get_or_create.return_value = (mock_cf, True) + + mock_ct = MagicMock() + mock_ct.pk = 1 + MockContentType.objects.get_for_model.return_value = mock_ct + + with patch("logging.getLogger") as mock_get_logger: + _ensure_librenms_id_custom_field(sender=None) + + MockCustomField.objects.get_or_create.assert_called_once_with( + name="librenms_id", + defaults={ + "type": "integer", + "label": "LibreNMS ID", + "description": "LibreNMS Device ID for synchronization (auto-created by plugin)", + "required": False, + "ui_visible": "if-set", + "ui_editable": "yes", + "is_cloneable": False, + }, + ) + + # Should have added content types for all 4 models + assert mock_cf.object_types.add.call_count == 4 + + # Should log when created + mock_get_logger.assert_called_with("netbox_librenms_plugin") + + def test_skips_when_already_executed(self): + """Handler is a no-op on second invocation (per-migrate dedup).""" + from netbox_librenms_plugin import _ensure_librenms_id_custom_field + + _ensure_librenms_id_custom_field._executed = True + + with patch("extras.models.CustomField") as MockCustomField: + _ensure_librenms_id_custom_field(sender=None) + MockCustomField.objects.get_or_create.assert_not_called() + + @patch("dcim.models.Interface", new_callable=MagicMock) + @patch("dcim.models.Device", new_callable=MagicMock) + @patch("virtualization.models.VMInterface", new_callable=MagicMock) + @patch("virtualization.models.VirtualMachine", new_callable=MagicMock) + @patch("django.contrib.contenttypes.models.ContentType") + @patch("extras.models.CustomField") + def test_existing_field_not_recreated( + self, MockCustomField, MockContentType, mock_vm, mock_vmif, mock_device, mock_iface + ): + """When custom field already exists, it is not recreated but types are checked.""" + from netbox_librenms_plugin import _ensure_librenms_id_custom_field + + mock_cf = MagicMock() + mock_cf.object_types.values_list.return_value = [1, 2, 3, 4] + MockCustomField.objects.get_or_create.return_value = (mock_cf, False) + + mock_ct = MagicMock() + mock_ct.pk = 1 + MockContentType.objects.get_for_model.return_value = mock_ct + + _ensure_librenms_id_custom_field(sender=None) + + # All pks already present, no types should be added + mock_cf.object_types.add.assert_not_called() + + @patch("dcim.models.Interface", new_callable=MagicMock) + @patch("dcim.models.Device", new_callable=MagicMock) + @patch("virtualization.models.VMInterface", new_callable=MagicMock) + @patch("virtualization.models.VirtualMachine", new_callable=MagicMock) + @patch("django.contrib.contenttypes.models.ContentType") + @patch("extras.models.CustomField") + def test_adds_missing_content_types( + self, MockCustomField, MockContentType, mock_vm, mock_vmif, mock_device, mock_iface + ): + """When some content types are missing, only those are added.""" + from netbox_librenms_plugin import _ensure_librenms_id_custom_field + + mock_cf = MagicMock() + mock_cf.object_types.values_list.return_value = [1, 2] + MockCustomField.objects.get_or_create.return_value = (mock_cf, False) + + ct_existing = MagicMock() + ct_existing.pk = 1 + ct_new = MagicMock() + ct_new.pk = 99 + MockContentType.objects.get_for_model.side_effect = [ct_existing, ct_existing, ct_new, ct_new] + + _ensure_librenms_id_custom_field(sender=None) + + assert mock_cf.object_types.add.call_count == 2 + mock_cf.object_types.add.assert_any_call(ct_new) + + @patch("extras.models.CustomField") + def test_exception_does_not_propagate(self, MockCustomField): + """Exceptions during custom field creation are caught and logged.""" + from netbox_librenms_plugin import _ensure_librenms_id_custom_field + + MockCustomField.objects.get_or_create.side_effect = Exception("DB not ready") + + with patch("logging.getLogger") as mock_get_logger: + # Should not raise + _ensure_librenms_id_custom_field(sender=None) + + # Verify the exception was logged + logger_instance = mock_get_logger.return_value + logger_instance.exception.assert_called_once() + call_args = logger_instance.exception.call_args + assert "librenms_id" in call_args[0][0] + + @patch("dcim.models.Interface", new_callable=MagicMock) + @patch("dcim.models.Device", new_callable=MagicMock) + @patch("virtualization.models.VMInterface", new_callable=MagicMock) + @patch("virtualization.models.VirtualMachine", new_callable=MagicMock) + @patch("django.contrib.contenttypes.models.ContentType") + @patch("extras.models.CustomField") + def test_no_log_when_field_already_exists( + self, MockCustomField, MockContentType, mock_vm, mock_vmif, mock_device, mock_iface + ): + """No log message when the custom field already existed.""" + from netbox_librenms_plugin import _ensure_librenms_id_custom_field + + mock_cf = MagicMock() + mock_cf.object_types.values_list.return_value = [1, 2, 3, 4] + MockCustomField.objects.get_or_create.return_value = (mock_cf, False) + + mock_ct = MagicMock() + mock_ct.pk = 1 + MockContentType.objects.get_for_model.return_value = mock_ct + + with patch("logging.getLogger") as mock_get_logger: + _ensure_librenms_id_custom_field(sender=None) + # When the field already exists (created=False), the info log should + # not be emitted. We verify via the logger instance rather than + # asserting getLogger was never called, which is fragile. + logger_instance = mock_get_logger.return_value + logger_instance.info.assert_not_called() From 8a893ca9da013494f3501778ea4db7f882ea1ef2 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Thu, 12 Mar 2026 08:18:36 +0000 Subject: [PATCH 02/25] feat(multi-server): JSON custom field librenms_id with server management and CR fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Multi-server librenms_id as JSON custom field (server_key → device_id map) - Server management UI, migration of legacy integer IDs - CR fixes: int conversion guard, VLAN dict guard, VC domain with server_key, forms has_option_only, JS server_key selector, _cancelled flag in bulk import, cache sort_keys, role recalculate, XSS escape, bool guards - Update existing tests for multi-server behavior - test_librenms_id.py: tests for new JSON CF format and migration --- .devcontainer/README.md | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/.devcontainer/README.md b/.devcontainer/README.md index 3560a93af3..7dc3ef967a 100644 --- a/.devcontainer/README.md +++ b/.devcontainer/README.md @@ -44,20 +44,20 @@ If you need to test with a LibreNMS instance on a private network (local lab, co 1. Fork and Clone: fork the plugin repo in Github and clone locally 2. Open in VS Code and choose "Reopen in Container" (or Ctrl+Shift+P → Dev Containers: Reopen in Container) -2. Wait for setup (~5min on first run or when new NetBox image is used). The container will install the plugin and prep NetBox -3. Set up GitHub access: `gh auth login` (for pushing/pulling code changes) -4. Create your plugin config — see [Plugin configuration](#plugin-configuration): +3. Wait for setup (~5min on first run or when new NetBox image is used). The container will install the plugin and prep NetBox +4. Set up GitHub access: `gh auth login` (for pushing/pulling code changes) +5. Create your plugin config — see [LibreNMS Server configuration](#librenms-server-configuration): - `cp .devcontainer/config/plugin-config.py.example .devcontainer/config/plugin-config.py` - Edit it with your server details (tokens/URLs) -5. Start NetBox with `netbox-run` (or `netbox-run-bg` in background) (see [Commands](#-commands-aliases)) -6. Access NetBox at http://localhost:8000 +6. Start NetBox with `netbox-run` (or `netbox-run-bg` in background) (see [Commands](#-commands-aliases)) +7. Access NetBox at http://localhost:8000 - Username: `admin` - Password: `admin` ### 🔄 Code changes and Committing 8. Edit code in the repo root. Check out [contributing docs](../docs/contributing.md) 9. Use `netbox-logs` to follow log output on screen -6. Commit changes and contribute as normal by submitting a PR on GitHub. +10. Commit changes and contribute as normal by submitting a PR on GitHub. ### Quick Tips - **Auto-reload**: Works for most code changes when `DEBUG=True` @@ -241,7 +241,7 @@ After any `.env` change, rebuild the dev container to apply environment updates. ### Additional packages (including other netbox plugins) - Create `.devcontainer/extra-requirements.txt` for extra Python packages. Example: `.devcontainer/extra-requirements.txt.example`. - - After changes: run `plugins-install` to install packages, then `netbox-restart` (see [Commands](#-commands-aliases)) + - After changes: run `plugin-install` to install packages, then `netbox-restart` (see [Commands](#-commands-aliases)) ## 🔧 Git Setup @@ -259,7 +259,7 @@ The dev container includes Git and GitHub CLI pre-installed. You'll need to conf git remote -v # If it shows git@github.com:..., convert to HTTPS -git remote set-url origin https://github.com/bonzo81/netbox-librenms-plugin.git +git remote set-url origin https://github.com//netbox-librenms-plugin.git ``` ### Recommended: GitHub CLI (Easiest) @@ -347,13 +347,14 @@ fatal: Could not read from remote repository. 1. **Check remote URL** - should use HTTPS, not SSH: ```bash git remote -v - # Should show: https://github.com/bonzo81/netbox-librenms-plugin.git - # NOT: git@github.com:bonzo81/netbox-librenms-plugin.git + # Should show: https://github.com//netbox-librenms-plugin.git + # NOT: git@github.com:/netbox-librenms-plugin.git ``` 2. **Fix SSH remote URL**: + ```bash - git remote set-url origin https://github.com/bonzo81/netbox-librenms-plugin.git + git remote set-url origin https://github.com//netbox-librenms-plugin.git ``` 3. **Authenticate with GitHub CLI**: From 22d08b59446873190b13787d29ef6bb2bc571943 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Thu, 12 Mar 2026 08:21:51 +0000 Subject: [PATCH 03/25] fix(multi-server): CR review fixes, production hardening, and test coverage Production fixes: - cables_view.py: None guard for port_id/port_name in local_ports_map - actions.py: remove overly strict migration gate for migrate_librenms_id - device_fields.py: test compat fixes - jobs.py: persist use_sysname/strip_domain in FilterDevicesJob.data - librenms_api.py: int conversion guard, VLAN dict isinstance guard - bulk_import.py: _cancelled flag in result, apply_role_to_validation helper - virtual_chassis.py: server_key param for VC domain namespacing - forms.py: has_option_only bool(data) guard for empty GET forms - cache.py: sort_keys=True in metadata hash for stable cache keys - JS: server_key input selector fix, closeHtmxModal abort Test coverage: 27 new coverage files + updated existing test files --- docs/development/testing.md | 2 + .../import_utils/bulk_import.py | 2 +- netbox_librenms_plugin/import_utils/cache.py | 2 +- .../import_utils/device_operations.py | 33 +- .../import_utils/virtual_chassis.py | 17 +- .../import_utils/vm_operations.py | 5 +- netbox_librenms_plugin/librenms_api.py | 2 +- .../tests/mock_librenms_server.py | 10 +- .../tests/test_background_jobs.py | 6 +- .../tests/test_cable_verify.py | 266 ++ .../tests/test_coverage_actions.py | 3842 +++++++++++++++++ .../tests/test_coverage_api.py | 1186 +++++ .../tests/test_coverage_api2.py | 699 +++ .../tests/test_coverage_base_views.py | 2139 +++++++++ .../tests/test_coverage_base_views2.py | 1997 +++++++++ .../tests/test_coverage_cache.py | 320 ++ .../tests/test_coverage_device_fields.py | 1972 +++++++++ .../tests/test_coverage_device_operations.py | 1657 +++++++ .../tests/test_coverage_filters.py | 768 ++++ .../tests/test_coverage_forms.py | 56 + .../tests/test_coverage_list.py | 1344 ++++++ .../tests/test_coverage_mixins.py | 1022 +++++ .../tests/test_coverage_sync_interfaces.py | 1188 +++++ .../tests/test_coverage_sync_view.py | 692 +++ .../tests/test_coverage_sync_views.py | 2577 +++++++++++ .../tests/test_coverage_sync_views2.py | 2230 ++++++++++ .../tests/test_coverage_sync_views3.py | 980 +++++ .../tests/test_coverage_tables.py | 2649 ++++++++++++ .../tests/test_coverage_utils.py | 561 +++ .../tests/test_coverage_virtual_chassis.py | 294 ++ .../tests/test_coverage_vlans_table.py | 367 ++ .../tests/test_import_utils.py | 16 +- .../tests/test_integration_virtual_chassis.py | 854 ++++ .../tests/test_ip_verify.py | 137 + .../tests/test_librenms_id.py | 152 +- .../tests/test_reviewer_fixes.py | 425 ++ .../tests/test_sync_devices.py | 49 - .../tests/test_sync_interfaces.py | 302 ++ .../tests/test_sync_view_mismatch.py | 183 +- .../tests/test_view_wiring.py | 19 +- .../tests/test_vm_operations.py | 681 +++ .../views/base/cables_view.py | 5 - .../views/imports/actions.py | 7 - .../views/sync/device_fields.py | 4 +- 44 files changed, 31276 insertions(+), 443 deletions(-) create mode 100644 netbox_librenms_plugin/tests/test_cable_verify.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_actions.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_api.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_api2.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_base_views.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_base_views2.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_cache.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_device_fields.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_device_operations.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_filters.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_forms.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_list.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_mixins.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_sync_view.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_sync_views.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_sync_views2.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_sync_views3.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_tables.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_utils.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_virtual_chassis.py create mode 100644 netbox_librenms_plugin/tests/test_coverage_vlans_table.py create mode 100644 netbox_librenms_plugin/tests/test_integration_virtual_chassis.py create mode 100644 netbox_librenms_plugin/tests/test_ip_verify.py create mode 100644 netbox_librenms_plugin/tests/test_reviewer_fixes.py create mode 100644 netbox_librenms_plugin/tests/test_sync_interfaces.py create mode 100644 netbox_librenms_plugin/tests/test_vm_operations.py diff --git a/docs/development/testing.md b/docs/development/testing.md index 42565cb76e..b8b1041c59 100644 --- a/docs/development/testing.md +++ b/docs/development/testing.md @@ -38,6 +38,8 @@ The test suite covers all major plugin functionality. Tests are organized by the | [test_coverage_device_fields.py](../../netbox_librenms_plugin/tests/test_coverage_device_fields.py) | Device field sync view—field update logic and device field mapping | | [test_coverage_list.py](../../netbox_librenms_plugin/tests/test_coverage_list.py) | Import list view—background job decision, job result loading, and GET handler | | [test_coverage_api.py](../../netbox_librenms_plugin/tests/test_coverage_api.py) | LibreNMS API client—malformed payload guards, error paths, and edge cases | +| [test_coverage_base_views.py](../../netbox_librenms_plugin/tests/test_coverage_base_views.py) | Base view coverage tests—sync table views, context data, and data pipeline | +| [test_coverage_base_views2.py](../../netbox_librenms_plugin/tests/test_coverage_base_views2.py) | Additional base view coverage tests | | [test_coverage_sync_view.py](../../netbox_librenms_plugin/tests/test_coverage_sync_view.py) | Sync view base class—context preparation and tab rendering | | [test_coverage_filters.py](../../netbox_librenms_plugin/tests/test_coverage_filters.py) | Import filter logic—filter form processing and device count helpers | | [test_sync_modules.py](../../netbox_librenms_plugin/tests/test_sync_modules.py) | Module sync—inventory matching, module type resolution, and normalization rules | diff --git a/netbox_librenms_plugin/import_utils/bulk_import.py b/netbox_librenms_plugin/import_utils/bulk_import.py index abc8754190..2ee408f1e6 100644 --- a/netbox_librenms_plugin/import_utils/bulk_import.py +++ b/netbox_librenms_plugin/import_utils/bulk_import.py @@ -403,7 +403,7 @@ def _refresh_existing_device(validation: dict, libre_device: dict = None, server except Exception as e: existing_id = getattr(existing, "pk", "unknown") if existing else "none" logger.error(f"Failed to refresh existing device (pk={existing_id}): {e}") - return + return # existing_device was None at cache time — check if device was imported since if not libre_device: diff --git a/netbox_librenms_plugin/import_utils/cache.py b/netbox_librenms_plugin/import_utils/cache.py index 8de881f897..6f7704ca69 100644 --- a/netbox_librenms_plugin/import_utils/cache.py +++ b/netbox_librenms_plugin/import_utils/cache.py @@ -175,7 +175,7 @@ def get_validated_device_cache_key( ) -def get_import_device_cache_key(device_id: int | str, server_key: str) -> str: +def get_import_device_cache_key(device_id: int | str, server_key: str = "default") -> str: """ Generate cache key for raw LibreNMS device data. diff --git a/netbox_librenms_plugin/import_utils/device_operations.py b/netbox_librenms_plugin/import_utils/device_operations.py index 23a7db9b8a..1426fe437f 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,6 +12,7 @@ from ..librenms_api import LibreNMSAPI from ..utils import ( + find_by_librenms_id, find_matching_platform, find_matching_site, match_librenms_hardware_to_device_type, @@ -128,11 +130,11 @@ 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, strip_domain: bool = False, - server_key: str = "default", ) -> dict: """ Validate if a LibreNMS device can be imported to NetBox. @@ -282,14 +284,10 @@ def validate_device_for_import( server_key = api.server_key if api is not None else server_key - # Check for existing VM first (by librenms_id custom field) - try: - from netbox_librenms_plugin.utils import find_by_librenms_id - - existing_vm = find_by_librenms_id(VirtualMachine, librenms_id, server_key) - except (ValueError, TypeError): - # librenms_id is not convertible to int; no match will be found - existing_vm = None + # 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})") @@ -315,15 +313,11 @@ def validate_device_for_import( result["name_sync_available"] = True result["suggested_name"] = hostname - # Check for existing Device (by librenms_id custom field) + # 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: - from netbox_librenms_plugin.utils import find_by_librenms_id - - existing_device = find_by_librenms_id(Device, librenms_id, server_key) - 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})") @@ -849,6 +843,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, @@ -856,6 +852,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": _cf_proxy.custom_field_data, } # Add optional fields @@ -880,6 +877,8 @@ 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/import_utils/virtual_chassis.py b/netbox_librenms_plugin/import_utils/virtual_chassis.py index f7a1a58b78..4f53119f2a 100644 --- a/netbox_librenms_plugin/import_utils/virtual_chassis.py +++ b/netbox_librenms_plugin/import_utils/virtual_chassis.py @@ -393,8 +393,11 @@ def create_virtual_chassis_with_members( ] """ - # original_master_name is still referenced in warning messages inside the atomic block. + # Save originals for in-memory rollback — transaction.atomic() rolls back DB but + # not in-memory model fields. original_master_name = master_device.name + original_vc = master_device.virtual_chassis + original_vc_position = master_device.vc_position # Find master's actual VC position from members_info by serial match; default to 1 _master_pos = 1 @@ -524,7 +527,10 @@ def create_virtual_chassis_with_members( [ m for m in members_info - if not (_norm_serial(m.get("serial")) and _norm_serial(m.get("serial")) == master_device.serial) + if not ( + _norm_serial(m.get("serial")) + and _norm_serial(m.get("serial")) == _norm_serial(master_device.serial) + ) and not ( not _norm_serial(m.get("serial")) and m.get("position") is not None @@ -547,10 +553,11 @@ def create_virtual_chassis_with_members( return vc except Exception as e: - # The transaction.atomic() block above will roll back all DB changes automatically. - # Manual state restoration is redundant and the save() would fail in a broken transaction. + master_device.name = original_master_name + master_device.virtual_chassis = original_vc + master_device.vc_position = original_vc_position logger.error( - f"Virtual Chassis creation failed for device {master_device.name}: {e}", + f"Virtual Chassis creation failed for device {original_master_name}: {e}", exc_info=True, ) raise diff --git a/netbox_librenms_plugin/import_utils/vm_operations.py b/netbox_librenms_plugin/import_utils/vm_operations.py index 0190c069ba..e1c43cc434 100644 --- a/netbox_librenms_plugin/import_utils/vm_operations.py +++ b/netbox_librenms_plugin/import_utils/vm_operations.py @@ -17,9 +17,10 @@ def create_vm_from_librenms( libre_device: dict, validation: dict, - server_key: str, + server_key: str = "default", use_sysname: bool = True, strip_domain: bool = False, + role=None, ): """ Create a NetBox VirtualMachine from LibreNMS device data. @@ -44,7 +45,7 @@ def create_vm_from_librenms( # Extract matched objects from validation cluster = validation["cluster"]["cluster"] platform = validation["platform"].get("platform") - role = validation.get("device_role", {}).get("role") + role = role if role is not None else validation.get("device_role", {}).get("role") # Determine VM name - use pre-computed name if available (handles strip_domain), # falling back to the validated resolved_name before recomputing from raw fields. diff --git a/netbox_librenms_plugin/librenms_api.py b/netbox_librenms_plugin/librenms_api.py index 0cceab18b7..d27152497d 100644 --- a/netbox_librenms_plugin/librenms_api.py +++ b/netbox_librenms_plugin/librenms_api.py @@ -1051,7 +1051,7 @@ def parse_port_vlan_data(self, port_data: dict, interface_name_field: str = "ifN untagged_vlan = None tagged_vlans = [] - if vlans_data: + if isinstance(vlans_data, list) and vlans_data: # Parse from detailed vlans array for vlan_entry in vlans_data: if not isinstance(vlan_entry, dict): diff --git a/netbox_librenms_plugin/tests/mock_librenms_server.py b/netbox_librenms_plugin/tests/mock_librenms_server.py index 060ba62006..11e53a4d2b 100644 --- a/netbox_librenms_plugin/tests/mock_librenms_server.py +++ b/netbox_librenms_plugin/tests/mock_librenms_server.py @@ -201,9 +201,13 @@ def auth_error_response(self, path="/api/v0/devices"): def inventory_response(self, device_id: int, items: list, status: int = 200): """Register a plain inventory response for /api/v0/inventory/{device_id}/all.""" + payload_status = "ok" if 200 <= status < 300 else "error" + payload = ( + {"status": payload_status, "inventory": items} if payload_status == "ok" else {"status": payload_status} + ) self.register( f"/api/v0/inventory/{device_id}/all", - {"status": "ok", "inventory": items}, + payload, status=status, ) @@ -225,6 +229,10 @@ def _handler(method, path, query, headers, body): if contained_in == "0": return 200, {"status": "ok", "inventory": root} if contained_in is not None: + # Only return chassis children when explicitly requesting chassis class + phy_class = query.get("entPhysicalClass", [None])[0] + if phy_class is not None and phy_class != "chassis": + return 200, {"status": "ok", "inventory": []} try: idx = int(contained_in) except (TypeError, ValueError): diff --git a/netbox_librenms_plugin/tests/test_background_jobs.py b/netbox_librenms_plugin/tests/test_background_jobs.py index 88f6931cf7..fcb3a3c260 100644 --- a/netbox_librenms_plugin/tests/test_background_jobs.py +++ b/netbox_librenms_plugin/tests/test_background_jobs.py @@ -437,9 +437,9 @@ def test_run_mixed_device_and_vm_import(self, mock_api_class, mock_bulk_devices, """Import both devices and VMs.""" from netbox_librenms_plugin.jobs import ImportDevicesJob - mock_api_instance = MagicMock() - mock_api_instance.server_key = "default" - mock_api_class.return_value = mock_api_instance + mock_api = MagicMock() + mock_api.server_key = "default" + mock_api_class.return_value = mock_api # Mock device imports mock_device = MagicMock() 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..849a1e6aba --- /dev/null +++ b/netbox_librenms_plugin/tests/test_cable_verify.py @@ -0,0 +1,266 @@ +""" +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, server_key=None): + assert link is not None + assert hostname is not None + assert device_id is not None + assert server_key == "default" + # 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, server_key=None): + 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 "' + vc.members.all.return_value = [member] + device.virtual_chassis = vc + + table = VCCableTable([], device=device) + record = {"local_port": "eth0", "local_port_id": "42"} + + with patch( + "netbox_librenms_plugin.tables.cables.get_virtual_chassis_member", + return_value=member, + ): + html = str(table.render_device_selection(None, record)) + + # The raw ' + 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, server_key=None): + 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 "' + vc.members.all.return_value = [member] + device.virtual_chassis = vc + + table = VCCableTable([], device=device) + record = {"local_port": "eth0", "local_port_id": "42"} + + with patch( + "netbox_librenms_plugin.tables.cables.get_virtual_chassis_member", + return_value=member, + ): + html = str(table.render_device_selection(None, record)) + + # The raw