diff --git a/front/js/ui_components.js b/front/js/ui_components.js index 1614d4d6..34c2cb3e 100755 --- a/front/js/ui_components.js +++ b/front/js/ui_components.js @@ -989,14 +989,14 @@ function renderDeviceLink(data, container, useName = false) { 'data-alertdown': device.devAlertDown, 'data-sleeping': device.devIsSleeping || 0, 'data-archived': device.devIsArchived || 0, - 'data-isnew': device.devIsNew || 0, + 'data-isnew': device.devIsNew || 0, 'data-icon': device.devIcon }); return ` - ${atob(device.devIcon)} + ${device.devIcon ? atob(device.devIcon) : ''} ${useName ? encodeSpecialChars(device.devName) : data.text} (${badge.iconHtml}) diff --git a/server/database.py b/server/database.py index 63f14233..bffdde9a 100755 --- a/server/database.py +++ b/server/database.py @@ -16,6 +16,8 @@ from db.db_upgrade import ( ensure_Settings, ensure_Indexes, ensure_mac_lowercase_triggers, + ensure_dangling_parentmac_cleanup_trigger, + cleanup_existing_dangling_parentmac, migrate_to_camelcase, migrate_timestamps_to_utc, ) @@ -225,6 +227,10 @@ class DB: # Normalization triggers ensure_mac_lowercase_triggers(self.sql) + # Prevent/repair dangling devParentMAC references left by deleted devices + ensure_dangling_parentmac_cleanup_trigger(self.sql) + cleanup_existing_dangling_parentmac(self.sql) + # Device history table + audit triggers ensure_deviceshistory_table(self.sql) ensure_deviceshistory_triggers(self.sql) diff --git a/server/db/db_upgrade.py b/server/db/db_upgrade.py index 2adac94b..f2e9b55f 100755 --- a/server/db/db_upgrade.py +++ b/server/db/db_upgrade.py @@ -146,6 +146,74 @@ def ensure_mac_lowercase_triggers(sql): return False +# Sentinel devParentMAC values that are never actual device references +PARENT_MAC_SENTINELS = ("", "internet", "null") + + +def ensure_dangling_parentmac_cleanup_trigger(sql): + """ + Ensures a trigger exists that clears devParentMAC/devParentMACSource on any + device that referenced a device MAC which was just deleted, preventing + dangling Parent Node references. + + Note: this intentionally does NOT touch the NEWDEV_devParentMAC setting. + Settings are sourced from app.conf and get re-imported verbatim on every + restart (see importConfigs()), so a DB-only fix here would be silently + reverted. Stale NEWDEV_devParentMAC values are instead guarded against at + the point of use in create_new_devices() (server/scan/device_handling.py). + """ + try: + sql.execute( + "SELECT name FROM sqlite_master WHERE type='trigger' AND name='trg_clear_dangling_parentmac_on_delete'" + ) + if not sql.fetchone(): + mylog("verbose", ["[db_upgrade] Creating trigger 'trg_clear_dangling_parentmac_on_delete'"]) + sql.execute(""" + CREATE TRIGGER trg_clear_dangling_parentmac_on_delete + AFTER DELETE ON Devices + FOR EACH ROW + WHEN OLD.devMac IS NOT NULL AND OLD.devMac != '' + BEGIN + UPDATE Devices + SET devParentMAC = '', devParentMACSource = '' + WHERE LOWER(devParentMAC) = LOWER(OLD.devMac); + END; + """) + + return True + + except Exception as e: + mylog("none", [f"[db_upgrade] ERROR while ensuring dangling parentMAC trigger: {e}"]) + return False + + +def cleanup_existing_dangling_parentmac(sql) -> bool: + """ + One-time/idempotent cleanup for installations that already have devParentMAC + values pointing to a MAC no longer present in Devices. The delete trigger + only prevents new dangling references going forward, so this repairs data + left over from before the trigger existed. + """ + try: + sentinel_list = ", ".join(f"'{v}'" for v in PARENT_MAC_SENTINELS) + + sql.execute(f""" + UPDATE Devices + SET devParentMAC = '', devParentMACSource = '' + WHERE devParentMAC IS NOT NULL + AND LOWER(devParentMAC) NOT IN ({sentinel_list}) + AND LOWER(devParentMAC) NOT IN (SELECT LOWER(devMac) FROM Devices) + """) + if sql.rowcount > 0: + mylog("verbose", [f"[db_upgrade] Cleared {sql.rowcount} dangling devParentMAC reference(s)"]) + + return True + + except Exception as e: + mylog("none", [f"[db_upgrade] ERROR while cleaning up dangling parentMAC references: {e}"]) + return False + + def ensure_views(sql) -> bool: """ Ensures required views exist. diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index 1d4fa899..a604e28b 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -730,6 +730,22 @@ def create_new_devices(db): mylog("debug", f"[New Devices] Collecting New Devices Query: {query}") current_scan_data = sql.execute(query).fetchall() + # Resolve the default Parent Node setting once and guard against it pointing + # to a MAC that no longer exists (e.g. that device was since deleted) - + # falling back to unset rather than seeding new devices with a dangling reference. + default_parent_mac_setting = get_setting_value("NEWDEV_devParentMAC") + if default_parent_mac_setting: + existing_device_macs = { + str(row[0]).lower() for row in sql.execute("SELECT devMac FROM Devices").fetchall() if row[0] + } + if default_parent_mac_setting.lower() not in existing_device_macs: + mylog( + "verbose", + f"[New Devices] NEWDEV_devParentMAC '{default_parent_mac_setting}' no longer " + "exists in Devices - treating as unset", + ) + default_parent_mac_setting = "" + for row in current_scan_data: ( scanMac, @@ -771,7 +787,7 @@ def create_new_devices(db): scanParentMAC if scanParentMAC and scanMac.lower() != "internet" else ( - get_setting_value("NEWDEV_devParentMAC") + default_parent_mac_setting if scanMac.lower() != "internet" else "null" ) diff --git a/test/db/test_dangling_parentmac_cleanup.py b/test/db/test_dangling_parentmac_cleanup.py new file mode 100644 index 00000000..527d50d2 --- /dev/null +++ b/test/db/test_dangling_parentmac_cleanup.py @@ -0,0 +1,172 @@ +""" +Unit tests for dangling devParentMAC cleanup. + +Tests verify that: +- Deleting a device clears devParentMAC/devParentMACSource on devices that + referenced it as their Parent Node. +- Sentinel values ('', 'internet', 'null') are never touched. +- Valid parent references are left untouched. +- The one-time migration repairs pre-existing dangling data and is idempotent. + +Note: the NEWDEV_devParentMAC *setting* is intentionally NOT handled here. +Settings are sourced from app.conf and get re-imported verbatim on every +restart, so a DB-only fix would be silently reverted. That case is instead +guarded against at the point of use in create_new_devices() — see +test/scan/test_field_lock_scan_integration.py. +""" + +import sys +import os +import pytest +import sqlite3 +import tempfile + +INSTALL_PATH = os.getenv('NETALERTX_APP', '/app') +sys.path.extend([f"{INSTALL_PATH}/server/plugins", f"{INSTALL_PATH}/server"]) + +from db.db_upgrade import ( # noqa: E402 + ensure_dangling_parentmac_cleanup_trigger, + cleanup_existing_dangling_parentmac, +) + + +@pytest.fixture +def temp_db(): + """Create a temporary database for testing""" + fd, db_path = tempfile.mkstemp(suffix='.db') + os.close(fd) + + conn = sqlite3.connect(db_path) + cursor = conn.cursor() + + cursor.execute(""" + CREATE TABLE Devices ( + devMac TEXT PRIMARY KEY COLLATE NOCASE, + devParentMAC TEXT, + devParentMACSource TEXT + ) + """) + + conn.commit() + + yield cursor, conn + + conn.close() + os.unlink(db_path) + + +class TestDanglingParentMacTrigger: + """Test suite for the AFTER DELETE cleanup trigger""" + + def test_trigger_clears_dependent_devices_on_delete(self, temp_db): + cursor, conn = temp_db + assert ensure_dangling_parentmac_cleanup_trigger(cursor) is True + + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC, devParentMACSource) VALUES (?, ?, ?)", + ("aa:bb:cc:dd:ee:01", "", ""), + ) + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC, devParentMACSource) VALUES (?, ?, ?)", + ("aa:bb:cc:dd:ee:02", "aa:bb:cc:dd:ee:01", "NEWDEV"), + ) + conn.commit() + + cursor.execute("DELETE FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:01",)) + conn.commit() + + cursor.execute( + "SELECT devParentMAC, devParentMACSource FROM Devices WHERE devMac = ?", + ("aa:bb:cc:dd:ee:02",), + ) + row = cursor.fetchone() + assert row == ("", "") + + def test_trigger_ignores_unrelated_deletes(self, temp_db): + cursor, conn = temp_db + ensure_dangling_parentmac_cleanup_trigger(cursor) + + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)", + ("aa:bb:cc:dd:ee:01", "internet"), + ) + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)", + ("aa:bb:cc:dd:ee:02", ""), + ) + conn.commit() + + cursor.execute("DELETE FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:02",)) + conn.commit() + + cursor.execute( + "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:01",) + ) + assert cursor.fetchone() == ("internet",) + + +class TestCleanupExistingDanglingParentMac: + """Test suite for the one-time/idempotent data repair migration""" + + def test_cleanup_clears_dangling_reference(self, temp_db): + cursor, conn = temp_db + + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC, devParentMACSource) VALUES (?, ?, ?)", + ("aa:bb:cc:dd:ee:02", "aa:bb:cc:dd:ee:99", "NEWDEV"), + ) + conn.commit() + + assert cleanup_existing_dangling_parentmac(cursor) is True + + cursor.execute( + "SELECT devParentMAC, devParentMACSource FROM Devices WHERE devMac = ?", + ("aa:bb:cc:dd:ee:02",), + ) + assert cursor.fetchone() == ("", "") + + def test_cleanup_preserves_valid_and_sentinel_values(self, temp_db): + cursor, conn = temp_db + + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)", + ("aa:bb:cc:dd:ee:01", ""), + ) + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)", + ("aa:bb:cc:dd:ee:02", "aa:bb:cc:dd:ee:01"), + ) + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)", + ("aa:bb:cc:dd:ee:03", "internet"), + ) + conn.commit() + + cleanup_existing_dangling_parentmac(cursor) + + cursor.execute( + "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:02",) + ) + assert cursor.fetchone() == ("aa:bb:cc:dd:ee:01",) + + cursor.execute( + "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:03",) + ) + assert cursor.fetchone() == ("internet",) + + def test_cleanup_is_idempotent(self, temp_db): + cursor, conn = temp_db + + cursor.execute( + "INSERT INTO Devices (devMac, devParentMAC) VALUES (?, ?)", + ("aa:bb:cc:dd:ee:02", "aa:bb:cc:dd:ee:99"), + ) + conn.commit() + + assert cleanup_existing_dangling_parentmac(cursor) is True + assert cleanup_existing_dangling_parentmac(cursor) is True + + cursor.execute( + "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:02",) + ) + assert cursor.fetchone() == ("",) diff --git a/test/scan/test_field_lock_scan_integration.py b/test/scan/test_field_lock_scan_integration.py index 8036ad22..77bd47c2 100644 --- a/test/scan/test_field_lock_scan_integration.py +++ b/test/scan/test_field_lock_scan_integration.py @@ -231,6 +231,66 @@ def test_create_new_devices_sets_sources(scan_db_for_new_devices): assert row["devVlanSource"] == "NEWDEV" +def test_create_new_devices_ignores_dangling_newdev_parentmac(scan_db_for_new_devices): + """A stale NEWDEV_devParentMAC pointing to a since-deleted device is treated as unset, + instead of seeding the new device with another dangling Parent Node reference.""" + cur = scan_db_for_new_devices.cursor() + cur.execute( + """ + INSERT INTO CurrentScan ( + scanMac, scanName, scanVendor, scanSourcePlugin, scanLastIP, + scanSyncHubNode, scanParentMAC, scanParentPort, + scanSite, scanSSID, scanType + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + "aa:bb:cc:dd:ee:11", + "DeviceTwo", + "AcmeVendor", + "ARPSCAN", + "192.168.1.11", + "", + "", # no parent reported by the scan itself + "", + "", + "", + "", + ), + ) + scan_db_for_new_devices.commit() + + settings = { + "NEWDEV_devType": "default-type", + # points to a MAC that does not (and never did, in this test) exist in Devices + "NEWDEV_devParentMAC": "99:99:99:99:99:99", + "NEWDEV_devOwner": "owner", + "NEWDEV_devGroup": "group", + "NEWDEV_devComments": "", + "NEWDEV_devLocation": "", + "NEWDEV_devCustomProps": "", + "NEWDEV_devParentRelType": "uplink", + "SYNC_node_name": "SYNCNODE", + } + + db = Mock() + db.sql_connection = scan_db_for_new_devices + db.sql = cur + db.commitDB = scan_db_for_new_devices.commit + + with patch.multiple( + device_handling, + get_setting_value=Mock(side_effect=lambda key: settings.get(key, "")), + safe_int=Mock(return_value=0), + ): + device_handling.create_new_devices(db) + + row = cur.execute( + "SELECT devParentMAC FROM Devices WHERE devMac = ?", ("aa:bb:cc:dd:ee:11",) + ).fetchone() + + assert row["devParentMAC"] == "" + + def test_scan_updates_newdev_device_name(scan_db, mock_device_handlers): """Scanner discovers name for device with NEWDEV source.""" cur = scan_db.cursor()