From 127e2c92ee51dbea305ad13d86841e5eda13f3a1 Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Sat, 26 Sep 2026 09:41:20 +1000 Subject: [PATCH] DOCS+PLG: skill updates, ADGUARDIMP more accurate presence determination #1813 --- .claude/skills/plugin-review/SKILL.md | 8 +- .gemini/skills/plugin-review/SKILL.md | 8 +- .github/skills/plugin-review/SKILL.md | 8 +- .../plugins/adguard_import/adguard_import.py | 35 +++- server/plugins/adguard_import/config.json | 16 ++ test/plugins/test_adguard_import.py | 186 ++++++++++++++++++ 6 files changed, 254 insertions(+), 7 deletions(-) create mode 100644 test/plugins/test_adguard_import.py diff --git a/.claude/skills/plugin-review/SKILL.md b/.claude/skills/plugin-review/SKILL.md index 8fbf0880f..c274e1529 100644 --- a/.claude/skills/plugin-review/SKILL.md +++ b/.claude/skills/plugin-review/SKILL.md @@ -1,6 +1,6 @@ --- name: plugin-review -description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers three checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, suspicious/attacker-influenced plugin data not being logged, and a new dependency not reaching every build target - plus worked real-PR examples. +description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers four checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, suspicious/attacker-influenced plugin data not being logged, a new dependency not reaching every build target, and scanPresence correctness for a roster/inventory-style plugin - plus worked real-PR examples. --- # Plugin Review @@ -56,3 +56,9 @@ If a PR adds a system package (`apk add` in the root `Dockerfile`) or a Python d For the latter two, `scripts/check_dependency_mirroring.py` (wired into the non-blocking `check-dependency-mirroring` CI job) flags a PR that touches `Dockerfile` without `Dockerfile.debian`, or `requirements.txt` without both `install/*` copies - it only knows the sibling file wasn't touched at all, not whether the specific package was actually needed there, so treat a flag as a prompt to check, not a verdict. A worked example: a WiFi-scanning plugin PR added `iw` plus its `setcap` grant to the root `Dockerfile` only. The devcontainer's checked-in Dockerfile went stale (missing `iw` until someone regenerates it), and `Dockerfile.debian` never got `iw` or a `setcap` line for it at all - the plugin silently can't scan on either target, caught only because the plugin's own error handling logs "not found" rather than crashing. + +## The fourth check this skill adds: scanPresence correctness for a roster/inventory-style plugin + +`scanPresence` defaults to `1` (`server/db/schema/app.sql`) when a plugin's `config.json` never maps it - every row that plugin reports asserts "currently online." That's correct for a plugin that only reports a device when it actually observed it *this cycle* (arp_scan, icmp_scan). It's wrong for a plugin whose data source is a **roster of known/historical entries** the upstream service remembers regardless of current connectivity (a client list built from DNS query history, a full device inventory export, anything that persists once discovered rather than expiring when the device goes quiet). Such a plugin re-reports the same entries every run whether or not the device is actually reachable, so the default `1` silently asserts permanent presence - read the actual data source (the API/file the script pulls from), not just the plugin's name, to tell which shape it is. `docs/PLUGINS_IMPORT_BEHAVIOR.md` already documents the fix (`scanPresence = 0` for "a reservation, a lease record, or a static IPAM entry"); this check exists because that guidance wasn't being verified against actual plugin behavior at review time. + +If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-seen timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastSeen`-vs-threshold check and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes. diff --git a/.gemini/skills/plugin-review/SKILL.md b/.gemini/skills/plugin-review/SKILL.md index 8fbf0880f..c274e1529 100644 --- a/.gemini/skills/plugin-review/SKILL.md +++ b/.gemini/skills/plugin-review/SKILL.md @@ -1,6 +1,6 @@ --- name: plugin-review -description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers three checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, suspicious/attacker-influenced plugin data not being logged, and a new dependency not reaching every build target - plus worked real-PR examples. +description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers four checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, suspicious/attacker-influenced plugin data not being logged, a new dependency not reaching every build target, and scanPresence correctness for a roster/inventory-style plugin - plus worked real-PR examples. --- # Plugin Review @@ -56,3 +56,9 @@ If a PR adds a system package (`apk add` in the root `Dockerfile`) or a Python d For the latter two, `scripts/check_dependency_mirroring.py` (wired into the non-blocking `check-dependency-mirroring` CI job) flags a PR that touches `Dockerfile` without `Dockerfile.debian`, or `requirements.txt` without both `install/*` copies - it only knows the sibling file wasn't touched at all, not whether the specific package was actually needed there, so treat a flag as a prompt to check, not a verdict. A worked example: a WiFi-scanning plugin PR added `iw` plus its `setcap` grant to the root `Dockerfile` only. The devcontainer's checked-in Dockerfile went stale (missing `iw` until someone regenerates it), and `Dockerfile.debian` never got `iw` or a `setcap` line for it at all - the plugin silently can't scan on either target, caught only because the plugin's own error handling logs "not found" rather than crashing. + +## The fourth check this skill adds: scanPresence correctness for a roster/inventory-style plugin + +`scanPresence` defaults to `1` (`server/db/schema/app.sql`) when a plugin's `config.json` never maps it - every row that plugin reports asserts "currently online." That's correct for a plugin that only reports a device when it actually observed it *this cycle* (arp_scan, icmp_scan). It's wrong for a plugin whose data source is a **roster of known/historical entries** the upstream service remembers regardless of current connectivity (a client list built from DNS query history, a full device inventory export, anything that persists once discovered rather than expiring when the device goes quiet). Such a plugin re-reports the same entries every run whether or not the device is actually reachable, so the default `1` silently asserts permanent presence - read the actual data source (the API/file the script pulls from), not just the plugin's name, to tell which shape it is. `docs/PLUGINS_IMPORT_BEHAVIOR.md` already documents the fix (`scanPresence = 0` for "a reservation, a lease record, or a static IPAM entry"); this check exists because that guidance wasn't being verified against actual plugin behavior at review time. + +If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-seen timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastSeen`-vs-threshold check and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes. diff --git a/.github/skills/plugin-review/SKILL.md b/.github/skills/plugin-review/SKILL.md index ea7a5d4b9..59ae30823 100644 --- a/.github/skills/plugin-review/SKILL.md +++ b/.github/skills/plugin-review/SKILL.md @@ -1,6 +1,6 @@ --- name: netalertx-plugin-review -description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers three checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, suspicious/attacker-influenced plugin data not being logged, and a new dependency not reaching every build target - plus worked real-PR examples. +description: Read when reviewing a plugin PR or auditing an existing plugin script (server/plugins/*/script.py or equivalent). Covers four checks not already mechanically enforced by test_plugin_conventions.py - plugin scripts embedding their own raw SQL instead of an existing/new model method, suspicious/attacker-influenced plugin data not being logged, a new dependency not reaching every build target, and scanPresence correctness for a roster/inventory-style plugin - plus worked real-PR examples. --- # Plugin Review @@ -56,3 +56,9 @@ If a PR adds a system package (`apk add` in the root `Dockerfile`) or a Python d For the latter two, `scripts/check_dependency_mirroring.py` (wired into the non-blocking `check-dependency-mirroring` CI job) flags a PR that touches `Dockerfile` without `Dockerfile.debian`, or `requirements.txt` without both `install/*` copies - it only knows the sibling file wasn't touched at all, not whether the specific package was actually needed there, so treat a flag as a prompt to check, not a verdict. A worked example: a WiFi-scanning plugin PR added `iw` plus its `setcap` grant to the root `Dockerfile` only. The devcontainer's checked-in Dockerfile went stale (missing `iw` until someone regenerates it), and `Dockerfile.debian` never got `iw` or a `setcap` line for it at all - the plugin silently can't scan on either target, caught only because the plugin's own error handling logs "not found" rather than crashing. + +## The fourth check this skill adds: scanPresence correctness for a roster/inventory-style plugin + +`scanPresence` defaults to `1` (`server/db/schema/app.sql`) when a plugin's `config.json` never maps it - every row that plugin reports asserts "currently online." That's correct for a plugin that only reports a device when it actually observed it *this cycle* (arp_scan, icmp_scan). It's wrong for a plugin whose data source is a **roster of known/historical entries** the upstream service remembers regardless of current connectivity (a client list built from DNS query history, a full device inventory export, anything that persists once discovered rather than expiring when the device goes quiet). Such a plugin re-reports the same entries every run whether or not the device is actually reachable, so the default `1` silently asserts permanent presence - read the actual data source (the API/file the script pulls from), not just the plugin's name, to tell which shape it is. `docs/PLUGINS_IMPORT_BEHAVIOR.md` already documents the fix (`scanPresence = 0` for "a reservation, a lease record, or a static IPAM entry"); this check exists because that guidance wasn't being verified against actual plugin behavior at review time. + +If the plugin's own data does contain *some* real recency signal (an active DHCP lease, a last-seen timestamp within a threshold), prefer computing `scanPresence` per-row from that over a blanket static value - see `pihole_api_scan`'s `lastSeen`-vs-threshold check and `adguard_import`'s active-dynamic-lease check (`server/plugins/adguard_import/adguard_import.py`) for two different real implementations of the same idea. A `scanPresence = 0` row is an *abstention*, not a vote for offline - `update_presence_from_CurrentScan()`/`current_scan_presence_condition()` (`server/scan/presence.py`) only ever require *some* row asserting `1` to mark a device present, so a plugin correctly reporting "no evidence of presence" never suppresses a real scanner's positive signal for the same device. When writing the comment explaining this on a plugin's own `scanPresence` logic, state the plugin's own contract ("this value means no active lease was found, not that the device is offline") rather than asserting how the core presence-aggregation code behaves internally - that couples a leaf plugin file to framework internals it doesn't own and won't be updated if that logic ever changes. diff --git a/server/plugins/adguard_import/adguard_import.py b/server/plugins/adguard_import/adguard_import.py index 3fb79066f..b626bf2cf 100644 --- a/server/plugins/adguard_import/adguard_import.py +++ b/server/plugins/adguard_import/adguard_import.py @@ -83,6 +83,8 @@ def main(): plugin_objects.write_result_file() return 1 + mylog("debug", [f"[{pluginName}] /control/clients raw response: {clients_json}"]) + raw_clients = clients_json.get("auto_clients", []) or [] # ------------------------------------------- @@ -93,22 +95,32 @@ def main(): server, port, protocol, auth, timeout ) + mylog("debug", [f"[{pluginName}] /control/dhcp/status raw response: {dhcp_json}"]) + dhcp_leases = [] static_leases = [] - + if dhcp_json: dhcp_leases = dhcp_json.get("leases", []) or [] static_leases = dhcp_json.get("static_leases", []) or [] # Build MAC lookup table for DHCP (combining dynamic and static leases) dhcp_mac_map = {} - + # MACs currently holding an active dynamic DHCP lease - the closest proxy + # for "recently connected" this plugin has, NOT a reachability check: a + # lease can outlive the device being powered off. A static reservation is + # a permanent binding regardless of connection state, and auto_clients is + # AdGuard's historical DNS-seen roster, not a live feed - neither implies + # the device is connected now. See has_active_lease below. + dynamic_lease_macs = set() + # Process dynamic leases first for lease in dhcp_leases: ip = lease.get("ip") mac = lease.get("mac") if ip and mac: dhcp_mac_map[ip] = mac.upper() + dynamic_lease_macs.add(mac.upper()) # Process static leases (overriding or adding to the map) for lease in static_leases: @@ -139,13 +151,27 @@ def main(): mylog("verbose", [f"[{pluginName}] Skipping device with {ip} as no MAC supplied and ADGUARDIMP_FAKE_MAC set to False"]) continue + # has_active_lease means exactly "AdGuard currently considers this + # MAC's dynamic lease active" - not "this device is reachable right + # now". A false value means there is no active dynamic lease + # reported by AdGuard; it does NOT mean the device is offline. The + # value is therefore not intended to override presence reported by + # other scanners. + has_active_lease = mac in dynamic_lease_macs + device_data.append({ "mac_address": mac, "ip_address": ip, "hostname": hostname, - "device_type": dsource + "device_type": dsource, + "has_active_lease": has_active_lease, }) + mylog("debug", [ + f"[{pluginName}] {mac} ({ip}): active_dhcp_lease={has_active_lease} " + f"(dynamic_lease_macs contains {len(dynamic_lease_macs)} entries)" + ]) + # ------------------------------------------- # Write plugin objects # ------------------------------------------- @@ -159,6 +185,7 @@ def main(): watched4 = '', extra = '', foreignKey = dev["mac_address"], + helpVal4 = '1' if dev["has_active_lease"] else '0', ) mylog("verbose", [f"[{pluginName}] New entries: {len(device_data)}"]) @@ -167,4 +194,4 @@ def main(): if __name__ == "__main__": - main() \ No newline at end of file + main() diff --git a/server/plugins/adguard_import/config.json b/server/plugins/adguard_import/config.json index b6901ad1a..f113af1c5 100644 --- a/server/plugins/adguard_import/config.json +++ b/server/plugins/adguard_import/config.json @@ -505,6 +505,22 @@ } ] }, + { + "column": "helpVal4", + "mapped_to_column": "scanPresence", + "css_classes": "col-sm-2", + "show": false, + "type": "none", + "default_value": "", + "options": [], + "localized": ["name"], + "name": [ + { + "language_code": "en_us", + "string": "N/A" + } + ] + }, { "column": "dateTimeCreated", "css_classes": "col-sm-2", diff --git a/test/plugins/test_adguard_import.py b/test/plugins/test_adguard_import.py new file mode 100644 index 000000000..ce10f88d9 --- /dev/null +++ b/test/plugins/test_adguard_import.py @@ -0,0 +1,186 @@ +"""Tests for the adguard_import (ADGUARDIMP) plugin's presence detection. + +script.py is loaded with its NetAlertX-internal dependencies (plugin_helper, +logger, helper, const, conf, pytz, utils.crypto_utils) stubbed out - same +approach test_wificanary.py/test_dockerdisc.py use - so these run without the +full devcontainer environment. `ag_request()` is mocked per test rather than +actually calling AdGuard Home's API. + +Regression coverage for GitHub issue #1813: devices imported from AdGuard +Home showed as permanently Online. Root cause: scanPresence was never mapped, +so it defaulted to 1 (server/db/schema/app.sql) for every row on every run - +but auto_clients (from /control/clients) is AdGuard's historical DNS-seen +roster, not a live-presence feed. Fix: scanPresence is now computed per +device from whether it currently holds an active *dynamic* DHCP lease +(/control/dhcp/status's `leases`, not `static_leases` - a static reservation +is a permanent binding, not evidence of current connectivity), passed via +helpVal4 (config.json maps helpVal4 -> scanPresence). +""" + +import os +import sys +import types +from unittest.mock import MagicMock, patch + +import pytest + +INSTALL_PATH = os.getenv("NETALERTX_APP", "/app") +sys.path.extend([f"{INSTALL_PATH}/server/plugins", f"{INSTALL_PATH}/server"]) + + +def _load_adguard_import_module(): + missing_module = object() + previous_modules = {} + + def stub(name, **attributes): + previous_modules[name] = sys.modules.get(name, missing_module) + module = types.ModuleType(name) + for attribute, value in attributes.items(): + setattr(module, attribute, value) + sys.modules[name] = module + + stub("plugin_helper", Plugin_Objects=MagicMock, string_to_fake_mac=lambda ip: f"fake:{ip}") + stub("logger", mylog=MagicMock(), Logger=MagicMock()) + stub("helper", get_setting_value=MagicMock(return_value="UTC")) + stub("const", logPath="/tmp") + stub("utils.crypto_utils", string_to_fake_mac=lambda ip: f"fake:{ip}") + stub("conf", tz=None) + stub("pytz", timezone=MagicMock(return_value="UTC")) + + import importlib.util + from pathlib import Path + module_path = Path(__file__).resolve().parents[2] / "server" / "plugins" / "adguard_import" / "adguard_import.py" + spec = importlib.util.spec_from_file_location("adguard_import_script", module_path) + module = importlib.util.module_from_spec(spec) + try: + spec.loader.exec_module(module) + finally: + for name, previous_module in previous_modules.items(): + if previous_module is missing_module: + sys.modules.pop(name, None) + else: + sys.modules[name] = previous_module + + return module + + +adguard_import = _load_adguard_import_module() + + +def _settings(overrides=None): + base = { + "ADGUARDIMP_SERVER": "adguard.local", + "ADGUARDIMP_PORT": 80, + "ADGUARDIMP_PROTOCOL": "http", + "ADGUARDIMP_USER": "", + "ADGUARDIMP_PASS": "", + "ADGUARDIMP_FAKE_MAC": False, + "ADGUARDIMP_RUN_TIMEOUT": 30, + } + base.update(overrides or {}) + return base + + +def _run_main(clients_response, dhcp_response, settings_overrides=None): + """Run main() with ag_request mocked to return the given API payloads, + and capture every add_object() call's kwargs, keyed by primaryId (MAC).""" + settings = _settings(settings_overrides) + calls = {} + + def fake_add_object(**kwargs): + calls[kwargs["primaryId"]] = kwargs + + def fake_ag_request(path, *args, **kwargs): + if path == "/control/clients": + return clients_response + if path == "/control/dhcp/status": + return dhcp_response + raise AssertionError(f"unexpected path: {path}") + + with patch.object(adguard_import, "get_setting_value", side_effect=lambda k: settings.get(k)): + with patch.object(adguard_import, "ag_request", side_effect=fake_ag_request): + adguard_import.plugin_objects.add_object = fake_add_object + adguard_import.plugin_objects.write_result_file = MagicMock() + adguard_import.main() + + return calls + + +class TestPresenceFromDynamicLease: + def test_device_with_active_dynamic_lease_is_present(self): + clients = {"auto_clients": [{"ip": "192.168.1.10", "name": "laptop", "source": "DHCP"}]} + dhcp = {"leases": [{"ip": "192.168.1.10", "mac": "aa:bb:cc:dd:ee:01"}], "static_leases": []} + + calls = _run_main(clients, dhcp) + + assert calls["AA:BB:CC:DD:EE:01"]["helpVal4"] == "1" + + def test_device_known_only_from_auto_clients_is_not_present(self): + """The exact bug from #1813: a device AdGuard has historically seen + via DNS traffic, with no current DHCP lease, must not be asserted + as currently online.""" + clients = {"auto_clients": [{"ip": "192.168.1.20", "name": "old-phone", "source": "RDNS"}]} + dhcp = {"leases": [], "static_leases": [{"ip": "192.168.1.20", "mac": "aa:bb:cc:dd:ee:02"}]} + + calls = _run_main(clients, dhcp) + + assert calls["AA:BB:CC:DD:EE:02"]["helpVal4"] == "0" + + def test_static_reservation_alone_is_not_present(self): + """A static DHCP reservation is a permanent binding, not evidence the + device is currently connected - only an active entry in the dynamic + `leases` array counts.""" + clients = {"auto_clients": [{"ip": "192.168.1.30", "name": "printer", "source": "DHCP"}]} + dhcp = {"leases": [], "static_leases": [{"ip": "192.168.1.30", "mac": "aa:bb:cc:dd:ee:03"}]} + + calls = _run_main(clients, dhcp) + + assert calls["AA:BB:CC:DD:EE:03"]["helpVal4"] == "0" + + def test_device_with_both_static_and_dynamic_entry_is_present(self): + """A device can have a static reservation *and* currently be + connected (holding the active lease that reservation grants it) - + the dynamic-lease signal should still mark it present.""" + clients = {"auto_clients": [{"ip": "192.168.1.40", "name": "server", "source": "DHCP"}]} + dhcp = { + "leases": [{"ip": "192.168.1.40", "mac": "aa:bb:cc:dd:ee:04"}], + "static_leases": [{"ip": "192.168.1.40", "mac": "aa:bb:cc:dd:ee:04"}], + } + + calls = _run_main(clients, dhcp) + + assert calls["AA:BB:CC:DD:EE:04"]["helpVal4"] == "1" + + def test_fake_mac_device_is_never_present(self): + """A device with no real MAC (identified only via a synthesized fake + MAC) can never match a real DHCP lease's MAC, so it can't be + asserted present.""" + clients = {"auto_clients": [{"ip": "192.168.1.50", "name": "unknown", "source": "RDNS"}]} + dhcp = {"leases": [], "static_leases": []} + + calls = _run_main(clients, dhcp, settings_overrides={"ADGUARDIMP_FAKE_MAC": True}) + + assert len(calls) == 1 + (only_call,) = calls.values() + assert only_call["helpVal4"] == "0" + + def test_multiple_devices_mixed_presence(self): + clients = { + "auto_clients": [ + {"ip": "192.168.1.60", "name": "online-device", "source": "DHCP"}, + {"ip": "192.168.1.61", "name": "offline-device", "source": "RDNS"}, + ] + } + dhcp = { + "leases": [{"ip": "192.168.1.60", "mac": "aa:bb:cc:dd:ee:60"}], + "static_leases": [{"ip": "192.168.1.61", "mac": "aa:bb:cc:dd:ee:61"}], + } + + calls = _run_main(clients, dhcp) + + assert calls["AA:BB:CC:DD:EE:60"]["helpVal4"] == "1" + assert calls["AA:BB:CC:DD:EE:61"]["helpVal4"] == "0" + + +if __name__ == "__main__": + sys.exit(pytest.main([__file__, "-v"]))