DOCS+PLG: skill updates, ADGUARDIMP more accurate presence determination #1813

This commit is contained in:
jokob-sk committed 2026-09-26 09:41:20 +10:00
1 parent 28eced2a25
commit 127e2c92ee
6 files changed
+254 -7

No files matched your search

+7 -1
View File
@@ -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.
+7 -1
View File
@@ -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.
+7 -1
View File
@@ -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.
@@ -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()
main()
+16
View File
@@ -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",
+186
View File
@@ -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"]))