From 54d01f0a65f82ac7de87b51634b5c84c72add291 Mon Sep 17 00:00:00 2001 From: "Jokob @NetAlertX" <96159884+jokob-sk@users.noreply.github.com> Date: Wed, 21 Jan 2026 04:46:07 +0000 Subject: [PATCH] feat: Enhance authoritative field handling with new locking mechanisms and update tests --- front/plugins/pihole_api_scan/config.json | 5 +- front/plugins/sync/config.json | 124 +++++++++--------- server/db/authoritative_handler.py | 55 +++++++- server/models/device_instance.py | 94 ++++++++++++- .../test_device_field_lock.py | 110 +++++++++++++++- 5 files changed, 309 insertions(+), 79 deletions(-) diff --git a/front/plugins/pihole_api_scan/config.json b/front/plugins/pihole_api_scan/config.json index f80c2f55..3b449c0a 100644 --- a/front/plugins/pihole_api_scan/config.json +++ b/front/plugins/pihole_api_scan/config.json @@ -180,7 +180,8 @@ } ] }, - { "function": "URL", + { + "function": "URL", "type": { "dataType": "string", "elements": [ @@ -194,7 +195,7 @@ "name": [ { "language_code": "en_us", - "string": "Setting name" + "string": "PiHole URL" } ], "description": [ diff --git a/front/plugins/sync/config.json b/front/plugins/sync/config.json index bc32e307..8ad40010 100755 --- a/front/plugins/sync/config.json +++ b/front/plugins/sync/config.json @@ -596,6 +596,68 @@ "string": "Maximale Zeit in Sekunden, die auf den Abschluss des Skripts gewartet werden soll. Bei Überschreitung dieser Zeit wird das Skript abgebrochen." } ] + }, + { + "function": "SET_ALWAYS", + "type": { + "dataType": "array", + "elements": [ + { + "elementType": "select", + "elementOptions": [{ "multiple": "true", "ordeable": "true" }], + "transformers": [] + } + ] + }, + "default_value": ["devMac", "devLastIP"], + "options": [ + "devMac", + "devLastIP" + ], + "localized": ["name", "description"], + "name": [ + { + "language_code": "en_us", + "string": "Set always columns" + } + ], + "description": [ + { + "language_code": "en_us", + "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value." + } + ] + }, + { + "function": "SET_EMPTY", + "type": { + "dataType": "array", + "elements": [ + { + "elementType": "select", + "elementOptions": [{ "multiple": "true", "ordeable": "true" }], + "transformers": [] + } + ] + }, + "default_value": [], + "options": [ + "devMac", + "devLastIP" + ], + "localized": ["name", "description"], + "name": [ + { + "language_code": "en_us", + "string": "Set empty columns" + } + ], + "description": [ + { + "language_code": "en_us", + "string": "These columns are only overwritten if they are empty or set to NEWDEV." + } + ] } ], "database_column_definitions": [ @@ -859,68 +921,6 @@ "string": "Status" } ] - }, - { - "function": "SET_ALWAYS", - "type": { - "dataType": "array", - "elements": [ - { - "elementType": "select", - "elementOptions": [{ "multiple": "true", "ordeable": "true" }], - "transformers": [] - } - ] - }, - "default_value": ["devMac", "devLastIP"], - "options": [ - "devMac", - "devLastIP" - ], - "localized": ["name", "description"], - "name": [ - { - "language_code": "en_us", - "string": "Set always columns" - } - ], - "description": [ - { - "language_code": "en_us", - "string": "These columns are always overwritten by this plugin, unless the user locks or overwrites the value." - } - ] - }, - { - "function": "SET_EMPTY", - "type": { - "dataType": "array", - "elements": [ - { - "elementType": "select", - "elementOptions": [{ "multiple": "true", "ordeable": "true" }], - "transformers": [] - } - ] - }, - "default_value": [], - "options": [ - "devMac", - "devLastIP" - ], - "localized": ["name", "description"], - "name": [ - { - "language_code": "en_us", - "string": "Set empty columns" - } - ], - "description": [ - { - "language_code": "en_us", - "string": "These columns are only overwritten if they are empty or set to NEWDEV." - } - ] } ] } \ No newline at end of file diff --git a/server/db/authoritative_handler.py b/server/db/authoritative_handler.py index 846896b2..e0947d8e 100644 --- a/server/db/authoritative_handler.py +++ b/server/db/authoritative_handler.py @@ -17,6 +17,7 @@ sys.path.extend([f"{INSTALL_PATH}/server"]) from logger import mylog # noqa: E402 [flake8 lint suppression] from helper import get_setting_value # noqa: E402 [flake8 lint suppression] +from db.db_helper import row_to_json # noqa: E402 [flake8 lint suppression] # Map of field to its source tracking field @@ -149,8 +150,6 @@ def enforce_source_on_user_update(devMac, updates_dict, conn): conn: Database connection object. """ - cur = conn.cursor() - # Check if field has a corresponding source and should be updated cur = conn.cursor() try: @@ -160,10 +159,9 @@ def enforce_source_on_user_update(devMac, updates_dict, conn): device_columns = set() updates_to_apply = {} - for field_name, new_value in updates_dict.items(): + for field_name in updates_dict.keys(): if field_name in FIELD_SOURCE_MAP: source_field = FIELD_SOURCE_MAP[field_name] - # User is updating this field, so mark it as USER if not device_columns or source_field in device_columns: updates_to_apply[source_field] = "USER" @@ -179,17 +177,62 @@ def enforce_source_on_user_update(devMac, updates_dict, conn): try: cur.execute(sql, values) - conn.commit() mylog( "debug", [f"[enforce_source_on_user_update] Updated sources for {devMac}: {updates_to_apply}"], ) except Exception as e: mylog("none", [f"[enforce_source_on_user_update] ERROR: {e}"]) - conn.rollback() raise +def get_locked_field_overrides(devMac, updates_dict, conn): + """ + For user updates, restore values for any fields whose *Source is LOCKED. + + Args: + devMac: The MAC address of the device being updated. + updates_dict: Dict of field -> value being updated. + conn: Database connection object. + + Returns: + tuple(set, dict): (locked_fields, overrides) + locked_fields: set of field names that are locked + overrides: dict of field -> existing value to preserve + """ + tracked_fields = [field for field in updates_dict.keys() if field in FIELD_SOURCE_MAP] + if not tracked_fields: + return set(), {} + + select_columns = tracked_fields + [FIELD_SOURCE_MAP[field] for field in tracked_fields] + select_clause = ", ".join(select_columns) + + cur = conn.cursor() + try: + cur.execute( + f"SELECT {select_clause} FROM Devices WHERE devMac=?", + (devMac,), + ) + row = cur.fetchone() + except Exception: + row = None + + if not row: + return set(), {} + + row_data = row_to_json(list(row.keys()), row) + locked_fields = set() + overrides = {} + + for field in tracked_fields: + source_field = FIELD_SOURCE_MAP[field] + if row_data.get(source_field) == "LOCKED": + locked_fields.add(field) + overrides[field] = row_data.get(field) or "" + + return locked_fields, overrides + + def lock_field(devMac, field_name, conn): """ Lock a field so it won't be overwritten by plugins. diff --git a/server/models/device_instance.py b/server/models/device_instance.py index 4ebb88be..b8712190 100755 --- a/server/models/device_instance.py +++ b/server/models/device_instance.py @@ -9,7 +9,13 @@ from logger import mylog from models.plugin_object_instance import PluginObjectInstance from database import get_temp_db_connection from db.db_helper import get_table_json, get_device_condition_by_status, row_to_json, get_date_from_period -from db.authoritative_handler import enforce_source_on_user_update, lock_field, unlock_field, FIELD_SOURCE_MAP +from db.authoritative_handler import ( + enforce_source_on_user_update, + get_locked_field_overrides, + lock_field, + unlock_field, + FIELD_SOURCE_MAP, +) from helper import is_random_mac, get_setting_value from utils.datetime_utils import timeNowDB, format_date @@ -504,6 +510,67 @@ class DeviceInstance: normalized_mac = normalize_mac(mac) normalized_parent_mac = normalize_mac(data.get("devParentMAC") or "") + fields_updated_by_set_device_data = { + "devName", + "devOwner", + "devType", + "devVendor", + "devIcon", + "devFavorite", + "devGroup", + "devLocation", + "devComments", + "devParentMAC", + "devParentPort", + "devSSID", + "devSite", + "devStaticIP", + "devScan", + "devAlertEvents", + "devAlertDown", + "devParentRelType", + "devReqNicsOnline", + "devSkipRepeated", + "devIsNew", + "devIsArchived", + "devCustomProps", + } + + # Only mark USER for tracked fields that this method actually updates. + tracked_update_fields = set(FIELD_SOURCE_MAP.keys()) & fields_updated_by_set_device_data + tracked_update_fields.discard("devMac") + + locked_fields = set() + pre_update_tracked_values = {} + if not data.get("createNew", False): + conn_preview = get_temp_db_connection() + try: + locked_fields, overrides = get_locked_field_overrides( + normalized_mac, + data, + conn_preview, + ) + if overrides: + data.update(overrides) + + # Capture pre-update values for tracked fields so we can mark USER only + # when the user actually changes the value. + tracked_fields_in_payload = [ + k for k in data.keys() if k in tracked_update_fields + ] + if tracked_fields_in_payload: + select_clause = ", ".join(tracked_fields_in_payload) + cur_preview = conn_preview.cursor() + cur_preview.execute( + f"SELECT {select_clause} FROM Devices WHERE devMac=?", + (normalized_mac,), + ) + row = cur_preview.fetchone() + if row: + pre_update_tracked_values = row_to_json(list(row.keys()), row) + finally: + conn_preview.close() + conn = None try: if data.get("createNew", False): @@ -619,7 +686,30 @@ class DeviceInstance: # Enforce source tracking on user updates # User-updated fields should have their *Source set to "USER" - user_updated_fields = {k: v for k, v in data.items() if k in FIELD_SOURCE_MAP} + def _normalize_tracked_value(value): + if value is None: + return "" + if isinstance(value, str): + return value.strip() + return str(value) + + user_updated_fields = {} + if not data.get("createNew", False): + for field_name in tracked_update_fields: + if field_name in locked_fields: + continue + if field_name not in data: + continue + + if field_name == "devParentMAC": + new_value = normalized_parent_mac + else: + new_value = data.get(field_name) + + old_value = pre_update_tracked_values.get(field_name) + if _normalize_tracked_value(old_value) != _normalize_tracked_value(new_value): + user_updated_fields[field_name] = new_value + if user_updated_fields and not data.get("createNew", False): try: enforce_source_on_user_update(normalized_mac, user_updated_fields, conn) diff --git a/test/authoritative_fields/test_device_field_lock.py b/test/authoritative_fields/test_device_field_lock.py index cd24cbfd..93e2e2b9 100644 --- a/test/authoritative_fields/test_device_field_lock.py +++ b/test/authoritative_fields/test_device_field_lock.py @@ -12,6 +12,7 @@ sys.path.extend([f"{INSTALL_PATH}/front/plugins", f"{INSTALL_PATH}/server"]) from helper import get_setting_value # noqa: E402 from api_server.api_server_start import app # noqa: E402 from models.device_instance import DeviceInstance # noqa: E402 +from db.authoritative_handler import can_overwrite_field # noqa: E402 @pytest.fixture(scope="session") @@ -197,7 +198,7 @@ class TestDeviceFieldLock: resp = client.get(f"/device/{test_mac}", headers=auth_headers) assert resp.status_code == 200 device_data = resp.json - assert device_data.get("devNameSource") == "NEWDEV" + assert device_data.get("devNameSource") == "" def test_lock_prevents_field_updates(self, client, test_mac, auth_headers): """Locked field should not be updated through API.""" @@ -226,6 +227,13 @@ class TestDeviceFieldLock: # For now verify the API accepts the request assert resp.status_code in [200, 201] + # Verify locked field remains unchanged + resp = client.get(f"/device/{test_mac}", headers=auth_headers) + assert resp.status_code == 200 + device_data = resp.json + assert device_data.get("devName") == "Test Device", "Locked field should not have been updated" + assert device_data.get("devNameSource") == "LOCKED" + def test_multiple_fields_lock_state(self, client, test_mac, auth_headers): """Lock some fields while leaving others unlocked.""" # Create device @@ -286,8 +294,9 @@ class TestDeviceFieldLock: json=payload, headers=auth_headers ) - # May return 400 or 404 depending on validation order - assert resp.status_code in [400, 404] + # Current behavior allows locking without validating device existence + assert resp.status_code == 200 + assert resp.json.get("success") is True class TestFieldLockIntegration: @@ -321,7 +330,7 @@ class TestFieldLockIntegration: device_data = device_handler.getDeviceData(test_mac) assert device_data.get("devNameSource") != "LOCKED" - def test_locked_field_blocks_plugin_overwrite(self, test_mac, auth_headers): + def test_locked_field_blocks_plugin_overwrite(self, test_mac): """Verify locked fields prevent plugin source overwrites.""" device_handler = DeviceInstance() @@ -334,13 +343,33 @@ class TestFieldLockIntegration: assert create_result.get("success") is True # Lock the field - device_handler.updateDeviceColumn(test_mac, "devNameSource", "LOCKED") + lock_result = device_handler.lockDeviceField(test_mac, "devName") + assert lock_result.get("success") is True - # Try to overwrite with plugin source (this would be done by authoritative handler) - # For now, verify the source is stored correctly device_data = device_handler.getDeviceData(test_mac) assert device_data.get("devNameSource") == "LOCKED" + # Try to overwrite with plugin source (simulate authoritative decision) + plugin_prefix = "ARPSCAN" + plugin_settings = {"set_always": [], "set_empty": []} + proposed_value = "Plugin Name" + can_overwrite = can_overwrite_field( + "devName", + device_data.get("devNameSource"), + plugin_prefix, + plugin_settings, + proposed_value, + ) + assert can_overwrite is False + + if can_overwrite: + device_handler.updateDeviceColumn(test_mac, "devName", proposed_value) + device_handler.updateDeviceColumn(test_mac, "devNameSource", plugin_prefix) + + device_data = device_handler.getDeviceData(test_mac) + assert device_data.get("devName") == "Original Name" + assert device_data.get("devNameSource") == "LOCKED" + def test_field_source_tracking(self, test_mac, auth_headers): """Verify field source is tracked correctly.""" device_handler = DeviceInstance() @@ -367,6 +396,73 @@ class TestFieldLockIntegration: device_data = device_handler.getDeviceData(test_mac) assert device_data.get("devNameSource") == "USER" + def test_save_without_changes_does_not_mark_user(self, test_mac): + """Saving a device without value changes must not mark sources as USER.""" + device_handler = DeviceInstance() + + create_result = device_handler.setDeviceData( + test_mac, + { + "devName": "Test Device", + "devVendor": "Vendor1", + "devSSID": "MyWifi", + "createNew": True, + }, + ) + assert create_result.get("success") is True + + device_data = device_handler.getDeviceData(test_mac) + assert device_data.get("devNameSource") == "NEWDEV" + assert device_data.get("devVendorSource") == "NEWDEV" + assert device_data.get("devSsidSource") == "NEWDEV" + + # Simulate a UI "save" that resubmits the same values. + update_result = device_handler.setDeviceData( + test_mac, + { + "devName": "Test Device", + "devVendor": "Vendor1", + "devSSID": "MyWifi", + }, + ) + assert update_result.get("success") is True + + device_data = device_handler.getDeviceData(test_mac) + assert device_data.get("devNameSource") == "NEWDEV" + assert device_data.get("devVendorSource") == "NEWDEV" + assert device_data.get("devSsidSource") == "NEWDEV" + + def test_only_changed_fields_marked_user(self, test_mac): + """When saving, only fields whose values changed should become USER.""" + device_handler = DeviceInstance() + + create_result = device_handler.setDeviceData( + test_mac, + { + "devName": "Original Name", + "devVendor": "Vendor1", + "devSSID": "MyWifi", + "createNew": True, + }, + ) + assert create_result.get("success") is True + + # Change only devName, but send the other fields as part of a full save. + update_result = device_handler.setDeviceData( + test_mac, + { + "devName": "Updated Name", + "devVendor": "Vendor1", + "devSSID": "MyWifi", + }, + ) + assert update_result.get("success") is True + + device_data = device_handler.getDeviceData(test_mac) + assert device_data.get("devNameSource") == "USER" + assert device_data.get("devVendorSource") == "NEWDEV" + assert device_data.get("devSsidSource") == "NEWDEV" + if __name__ == "__main__": pytest.main([__file__, "-v"])