From dd4fc057c829277df9abfc2ce11945b8ce2784ea Mon Sep 17 00:00:00 2001 From: jokob-sk Date: Thu, 10 Sep 2026 22:11:46 +1000 Subject: [PATCH] BE: SQL refactor presence --- .claude/skills/scan-pipeline/SKILL.md | 7 +- .gemini/skills/scan-pipeline/SKILL.md | 7 +- .github/skills/scan-pipeline/SKILL.md | 7 +- docs/NOTIFICATIONS.md | 4 + docs/PLUGINS_DEV_DATA_CONTRACT.md | 2 +- server/database.py | 18 ++++ server/db/db_upgrade.py | 34 +++++- server/db/schema_columns.py | 94 ++++++++++++++++ server/plugins/sync/sync.py | 6 +- server/scan/device_handling.py | 25 ++--- server/scan/presence.py | 30 ++++++ server/scan/session_events.py | 15 +-- test/db/test_schema_drift_guard.py | 148 ++++++++++++++++++++++++++ test/scan/test_presence_helper.py | 95 +++++++++++++++++ 14 files changed, 451 insertions(+), 41 deletions(-) create mode 100644 server/db/schema_columns.py create mode 100644 server/scan/presence.py create mode 100644 test/db/test_schema_drift_guard.py create mode 100644 test/scan/test_presence_helper.py diff --git a/.claude/skills/scan-pipeline/SKILL.md b/.claude/skills/scan-pipeline/SKILL.md index ab0eff0e..9781183a 100644 --- a/.claude/skills/scan-pipeline/SKILL.md +++ b/.claude/skills/scan-pipeline/SKILL.md @@ -47,13 +47,14 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Two real gotchas (not hypothetical — both surfaced live during a design review) +## Four real gotchas (not hypothetical — all surfaced live during a design review) -1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. +1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. +4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. -**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan` specifically is safe from drift because `ensure_CurrentScan()` unconditionally drops and recreates it on every startup, superseding whatever `app.sql` bootstrapped — but that safety net is unique to the four tables with an `ensure_X()`-style function (`CurrentScan`, `Parameters`, `Settings`, `Plugins_Language_Strings`). `Events`, `Sessions`, `AppEvents`, and `Notifications` have no such function and no `ensure_column()` backfill calls in `server/database.py` either (unlike `Devices`, which has ~30 of them) — for those tables, whatever `app.sql` says *is* the schema, permanently, for every fresh install. Drift there would be a real, live bug, not documentation lag — see `.gemini/internal-docs/PRDs/scan-pipeline-hardening.md` for the follow-up this motivated. Any *new* query added here should be checked the same way (`EXPLAIN QUERY PLAN` at a realistic row count) rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. +**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. **`Events`, `Sessions`, and `Notifications`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. ## When to read this vs. other docs/skills diff --git a/.gemini/skills/scan-pipeline/SKILL.md b/.gemini/skills/scan-pipeline/SKILL.md index 65352942..a2bab0f9 100644 --- a/.gemini/skills/scan-pipeline/SKILL.md +++ b/.gemini/skills/scan-pipeline/SKILL.md @@ -47,13 +47,14 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Two real gotchas (not hypothetical — both surfaced live during a design review) +## Four real gotchas (not hypothetical — all surfaced live during a design review) -1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. +1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. +4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. -**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan` specifically is safe from drift because `ensure_CurrentScan()` unconditionally drops and recreates it on every startup, superseding whatever `app.sql` bootstrapped — but that safety net is unique to the four tables with an `ensure_X()`-style function (`CurrentScan`, `Parameters`, `Settings`, `Plugins_Language_Strings`). `Events`, `Sessions`, `AppEvents`, and `Notifications` have no such function and no `ensure_column()` backfill calls in `server/database.py` either (unlike `Devices`, which has ~30 of them) — for those tables, whatever `app.sql` says *is* the schema, permanently, for every fresh install. Drift there would be a real, live bug, not documentation lag — see `.gemini/internal-docs/PRDs/scan-pipeline-hardening.md` for the follow-up this motivated. Any *new* query added here should be checked the same way (`EXPLAIN QUERY PLAN` at a realistic row count) rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. +**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. **`Events`, `Sessions`, and `Notifications`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. ## When to read this vs. other docs/skills diff --git a/.github/skills/scan-pipeline/SKILL.md b/.github/skills/scan-pipeline/SKILL.md index 6e455886..e8ba8d75 100644 --- a/.github/skills/scan-pipeline/SKILL.md +++ b/.github/skills/scan-pipeline/SKILL.md @@ -47,13 +47,14 @@ This skill covers what happens *after* a plugin's rows land in `CurrentScan` — This is the scan-pipeline-local half of a bigger attribution system — see the `database-patterns` skill for `FIELD_SOURCE_MAP` / `server/db/authoritative_handler.py`, the full `*Source` attribution model, and how SQLite triggers consume it for audit logging. Read both if touching anything that writes a `*Source` column. -## Two real gotchas (not hypothetical — both surfaced live during a design review) +## Four real gotchas (not hypothetical — all surfaced live during a design review) -1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. +1. **A "presence" check almost always exists in more than one place.** When adding a per-row signal meaning "don't count this as a live sighting" (e.g. a proposed `scanPresence` column), every query that independently re-derives "is this MAC currently present" from `CurrentScan` has to be updated together — `update_presence_from_CurrentScan()`, the `insert_events()` "New Connections"/"Device Down"/"Disconnected" queries, and the raw `INSERT INTO Sessions` inside `create_new_devices()` all encode that same question separately. Patching one and missing a sibling produces a UI where the device badge, the Events log, and the Sessions timeline each tell a different story for the same device. See `.gemini/internal-docs/PRDs/plugin-import-behavior-controls.md` for the worked example — a `scanPresence = 0` transition that never closed its session because only one of three "is it present" queries had been patched. **Update:** `current_scan_presence_condition()` (`server/scan/presence.py`) now centralizes this for five of those sites — `update_presence_from_CurrentScan()` (both statements), `update_devLastConnection_from_CurrentScan()`, and three of `insert_events()`'s four queries (both `Device Down` variants, `Disconnected`) all call it instead of writing their own `EXISTS (...)`. The remaining two ("New Connections", the raw `Sessions` insert in `create_new_devices()`) still can't use it — they need the actual `scanLastIP`/`scanVendor` *value* off the presence-asserting row via `MIN()`/`GROUP BY`, not just a boolean — so a brand-new presence-adjacent query still has to be checked against both patterns, not assumed to be a bare helper call. 2. **`CurrentScan` is deleted at the end of every cycle — a per-row flag on it cannot express a decision that needs to survive to a cycle where the row is absent.** Anything that fires specifically *because* a row is missing (`Device Down`, `Disconnected`) cannot read a flag that lived on that now-gone row. If a per-row plugin signal needs to affect behavior beyond the cycle it arrived in, persist it onto the `Devices` row at creation time (e.g. seeding `devAlertDown`/`devAlertEvents` from the row's flag instead of the global `NEWDEV_*` defaults) rather than trying to make the ephemeral table carry it forward. 3. **`CurrentScan` is not small, and it has an index now — check before assuming otherwise.** Real production users run 10,000+ devices; with the normal one-row-per-contributing-plugin pattern (see `LatestDeviceScan` above), a single cycle's `CurrentScan` is routinely 20,000-50,000+ rows, not the few hundred a homelab install might suggest. `idx_currentscan_scanmac` was added to `server/db/db_upgrade.py:ensure_CurrentScan()` (and mirrored in `server/db/schema/app.sql`) specifically because every `scanMac`-keyed lookup in this file was a full table scan without it — confirmed via `EXPLAIN QUERY PLAN` before the fix. Note `ensure_CurrentScan()` itself (the `DROP TABLE`/`CREATE TABLE`) only runs once, at app startup (`DB.initDB()`, called once from `server/__main__.py`) — don't confuse this with the per-cycle `DELETE FROM CurrentScan` in point 1 above, which clears rows but doesn't touch the table or its index. The index is built once and maintained incrementally, not rebuilt every cycle. +4. **`server/plugins/sync/sync.py` bypasses this entire pipeline on purpose, twice — a permanent exception, not a bug.** It fires its own direct `INSERT OR IGNORE INTO Events (... 'New Device' ...)` for newly-seen synced devices (hardcoded `evePendingAlertEmail = 1`, no `scanNotificationMode`/quiet awareness), and in `carbon-copy` mode its own raw `Devices` UPSERT via `ON CONFLICT(devMac) DO UPDATE` — both deliberately skipping `create_new_devices()`/`update_devices_data_from_scan()`/`can_overwrite_field()` (`sync.py`'s own comments document this as intentional: "Node is fully authoritative in this mode"). It *is* a normal `mapped_to_table: CurrentScan` plugin for its presence contribution, so `IMPORT_ON`/`scanPresence` apply to it exactly like any other plugin — but its two direct-write paths would silently ignore `scanNotificationMode = 'quiet'` or `scanCreatesDevice = 0` if `sync` ever adopted either. Keep this in mind whenever touching the generic pipeline and assuming every `Events`/`Devices` write went through it — `sync.py` is the one place that doesn't. -**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan` specifically is safe from drift because `ensure_CurrentScan()` unconditionally drops and recreates it on every startup, superseding whatever `app.sql` bootstrapped — but that safety net is unique to the four tables with an `ensure_X()`-style function (`CurrentScan`, `Parameters`, `Settings`, `Plugins_Language_Strings`). `Events`, `Sessions`, `AppEvents`, and `Notifications` have no such function and no `ensure_column()` backfill calls in `server/database.py` either (unlike `Devices`, which has ~30 of them) — for those tables, whatever `app.sql` says *is* the schema, permanently, for every fresh install. Drift there would be a real, live bug, not documentation lag — see `.gemini/internal-docs/PRDs/scan-pipeline-hardening.md` for the follow-up this motivated. Any *new* query added here should be checked the same way (`EXPLAIN QUERY PLAN` at a realistic row count) rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. +**Correction: `app.sql` is not dead code** — an earlier version of this note called it "otherwise-unused." Checked further: `install/production-filesystem/entrypoint.d/25-first-run-db.sh` pipes it straight into `sqlite3` to bootstrap a brand-new database on first install, and `scripts/db_cleanup/regenerate-database.sh` uses it too. `CurrentScan`, `Parameters`, and `Settings` are safe from drift because each has a dedicated `ensure_X()` function (`server/db/db_upgrade.py`) that unconditionally drops and recreates the table on every startup, superseding whatever `app.sql` bootstrapped. `Plugins_Language_Strings` gets the same unconditional drop/recreate, but inside the shared `ensure_plugins_tables()`, not a dedicated function of its own. `AppEvents` gets an equivalent drop/recreate too, via a different mechanism — `AppEvent_obj.__init__()` (`server/workflows/app_events.py`) drops and recreates it on every startup, independent of `db_upgrade.py`. `Devices` has no drop/recreate, but `server/database.py` has 18 explicit `ensure_column()` calls that backfill any column missing from an older `app.sql` snapshot on every startup. **`Events`, `Sessions`, and `Notifications`** — the three tables that genuinely had neither safety net — **now have the same backfill treatment**, per `scan-pipeline-hardening.md` Design §3 (implemented): `ensure_table_columns()` (`server/db/db_upgrade.py`), driven by one Python column-list constant per table (`server/db/schema_columns.py`) that's also diffed against `app.sql` in CI (`test/db/test_schema_drift_guard.py`), so drift between the two is caught rather than silently shipping. `AppEvents`/`Notifications` each also have a *second* schema-definition surface beyond `app.sql` worth knowing about — their own inline `CREATE TABLE IF NOT EXISTS` in `server/workflows/app_events.py`/`server/models/notification_instance.py` respectively — kept in sync via the same drift-check test. Any *new* query added here should still be checked with `EXPLAIN QUERY PLAN` at a realistic row count rather than assumed fine because it "looks like the existing queries" — several of those existing queries were themselves unindexed scans until this was caught. A correlated subquery re-evaluated per row (an accidental self-join) is the pattern most likely to look reasonable and be quadratic at this scale. ## When to read this vs. other docs/skills diff --git a/docs/NOTIFICATIONS.md b/docs/NOTIFICATIONS.md index 7c992e83..620fc328 100755 --- a/docs/NOTIFICATIONS.md +++ b/docs/NOTIFICATIONS.md @@ -40,6 +40,10 @@ On almost all plugins there are 2 core settings, `_WATCH` and `_ Click the **Read more in the docs.** Link at the top of each plugin to get more details on how the given plugin works. +### Plugin-level per-row overrides + +A plugin author can also mark individual rows it reports as `quiet` via the `scanNotificationMode` data column, independent of any user-facing setting above - e.g. a bulk inventory import that shouldn't spam notifications for known-offline devices. This is a plugin-authoring concept, not something configured in the UI - see [Data contract](https://docs.netalertx.com/PLUGINS_DEV_DATA_CONTRACT#import-behavior-columns) for the full behavior (when it applies, and how it combines with the **Alert Events**/**Alert Down** device settings above when multiple plugins report the same device). + ## Global settings ⚙ ![Global notification settings](./img/NOTIFICATIONS/Global-notification-settings.png) diff --git a/docs/PLUGINS_DEV_DATA_CONTRACT.md b/docs/PLUGINS_DEV_DATA_CONTRACT.md index 7493e1b8..5683c135 100644 --- a/docs/PLUGINS_DEV_DATA_CONTRACT.md +++ b/docs/PLUGINS_DEV_DATA_CONTRACT.md @@ -164,7 +164,7 @@ Three optional `CurrentScan` columns, all independent of each other, control wha | Column | Type | Default | Meaning | |---|---|---|---| | `scanCreatesDevice` | boolean | `1` | Whether this row can originate a *new* `Devices` entry. `0` lets an enrich-only plugin (e.g. a hostname resolver) update an already-existing device's fields without ever being able to create one. | -| `scanNotificationMode` | text (`normal` \| `quiet`) | `normal` | Whether creating/reconnecting this device should dispatch a notification. `quiet` still writes the `Events` row (audit trail intact) but suppresses the outbound email/push. Creation-time-only: it seeds `devAlertDown`/`devAlertEvents` to `0` on the device when it's first created, rather than being an ongoing, per-cycle re-evaluated policy — reclassifying a plugin's row later does not retroactively change an already-created device's alert settings. | +| `scanNotificationMode` | text (`normal` \| `quiet`) | `normal` | Whether this row's notifications are suppressed. `quiet` still writes the `Events` row (audit trail intact) but suppresses the outbound email/push. Checked two different ways depending on the event: **live**, as a per-cycle aggregate, for any event fired from a row that's actually present this cycle (`New Device`, `Connected`, `Down Reconnected`, `IP Changed`) — reclassifying a plugin's row does change these going forward. **Frozen**, via `devAlertDown`/`devAlertEvents` seeded onto the device at creation time, for events fired from row *absence* (`Device Down`, `Disconnected`) — there's no live `CurrentScan` row to read at that moment, so reclassifying later does not retroactively change an already-created device's alert settings for these two event types. | | `scanPresence` | boolean | `1` | Whether this row asserts the device is *currently online*. `0` means "identity/inventory data, no presence claim" — not "offline". A reservation, a lease record, or a static IPAM entry are typical `0` cases. | **Missing vs. invalid values — these behave differently, not interchangeably:** diff --git a/server/database.py b/server/database.py index 4e63ad43..f552ac2d 100755 --- a/server/database.py +++ b/server/database.py @@ -10,6 +10,7 @@ from db.db_helper import get_table_json, json_obj from workflows.app_events import AppEvent_obj from db.db_upgrade import ( ensure_column, + ensure_table_columns, ensure_CurrentScan, ensure_plugins_tables, ensure_Parameters, @@ -202,6 +203,23 @@ class DB: # before ensure_plugins_tables which uses IF NOT EXISTS with new names) migrate_to_camelcase(self.sql) + # Backfill Events/Sessions/Notifications columns that may be + # missing from an older app.sql snapshot - same drift-repair + # pattern as the Devices columns above, generalized instead of + # duplicated (see scan-pipeline-hardening.md Design §3). Must run + # after the camelCase migration above, or a pre-migration table's + # old-style column names would make every new-style column look + # "missing" and get added alongside the stale ones. + # + # AppEvents is deliberately NOT in this list: AppEvent_obj(self) + # below unconditionally drops and recreates it on every startup + # (its own equivalent of ensure_CurrentScan()'s drop/recreate + # pattern), so backfilling it here would be wasted work on a + # table about to be discarded a few lines later. + for _drift_table in ("Events", "Sessions", "Notifications"): + if not ensure_table_columns(self.sql, _drift_table): + raise RuntimeError(f"ensure_table_columns({_drift_table}) failed") + # Settings table setup ensure_Settings(self.sql) diff --git a/server/db/db_upgrade.py b/server/db/db_upgrade.py index 245c1759..1037ee14 100755 --- a/server/db/db_upgrade.py +++ b/server/db/db_upgrade.py @@ -3,6 +3,7 @@ from zoneinfo import ZoneInfo import datetime as dt from logger import mylog # noqa: E402 [flake8 lint suppression] from messaging.in_app import write_notification # noqa: E402 [flake8 lint suppression] +from db.schema_columns import TABLE_COLUMNS # noqa: E402 [flake8 lint suppression] # Define the expected Devices table columns (hardcoded base schema) [v26.1/2.XX] @@ -81,7 +82,7 @@ def ensure_column(sql, table: str, column_name: str, column_type: str) -> bool: return True # Already exists # Validate that this column is in the expected schema - expected = EXPECTED_DEVICES_COLUMNS if table == "Devices" else [] + expected = EXPECTED_DEVICES_COLUMNS if table == "Devices" else TABLE_COLUMNS.get(table, {}) if not expected or column_name not in expected: msg = ( f"[db_upgrade] ⚠ ERROR: Column '{column_name}' is not in expected schema - " @@ -102,6 +103,37 @@ def ensure_column(sql, table: str, column_name: str, column_type: str) -> bool: return False +def ensure_table_columns(sql, table: str) -> bool: + """ + Backfill every column in TABLE_COLUMNS[table] (server/db/schema_columns.py) + that's missing from the live table, via ensure_column() - the same + drift-repair pattern Devices already has, generalized instead of + hand-writing one ensure_column() call per column per table (the mirrored + duplication that pattern would otherwise reintroduce - see + scan-pipeline-hardening.md Design §3). + + Skips silently (returns True) if the table doesn't exist yet - these + four tables are normally created by app.sql's first-run bootstrap before + this ever runs, but this must not turn a not-yet-created table into a + hard failure that rolls back the whole initDB() transaction. + """ + columns = TABLE_COLUMNS.get(table) + if not columns: + mylog("none", [f"[db_upgrade] ensure_table_columns: no column list registered for '{table}'"]) + return False + + sql.execute("SELECT name FROM sqlite_master WHERE type='table' AND name=?", (table,)) + if not sql.fetchone(): + mylog("debug", [f"[db_upgrade] ensure_table_columns: '{table}' does not exist yet, skipping"]) + return True + + ok = True + for column_name, column_type in columns.items(): + if not ensure_column(sql, table, column_name, column_type): + ok = False + return ok + + def ensure_mac_lowercase_triggers(sql): """ Ensures the triggers for lowercasing MAC addresses exist on the Devices table. diff --git a/server/db/schema_columns.py b/server/db/schema_columns.py new file mode 100644 index 00000000..5371b870 --- /dev/null +++ b/server/db/schema_columns.py @@ -0,0 +1,94 @@ +""" +Single source of truth for the Events/Sessions/AppEvents/Notifications column +lists - used by both the runtime backfill loop (db_upgrade.ensure_table_columns()) +and the CI drift-check test (test/db/test_schema_drift_guard.py) against +server/db/schema/app.sql, per scan-pipeline-hardening.md Design §3. + +Unlike Devices (server/database.py's ~18 ensure_column() calls, unrelated to +this file), Events/Sessions/Notifications have no drop/recreate safety net +and previously had zero ensure_column() calls at all - app.sql was the only +definition of their schema, with nothing to catch it drifting from what the +rest of the code expects. This is additive/detection-and-backfill only; it +doesn't change any column's meaning or add new columns beyond what app.sql +already defines today. + +AppEvents is included here too (for the drift-check test's benefit, since +app.sql also defines it and workflows/app_events.py's inline CREATE TABLE is +a second definition worth keeping in sync), but is NOT part of the runtime +backfill loop in database.py's initDB() - AppEvent_obj.__init__() already +unconditionally drops and recreates the table on every startup, making its +drift harmless the same way ensure_CurrentScan() does for CurrentScan. Found +this while implementing the backfill loop, not before - the correction is +recorded in scan-pipeline-hardening.md. + +Column types are copied verbatim from server/db/schema/app.sql's CREATE +TABLE statements for these four tables - keep in sync if that file changes, +the drift-check test will fail loudly if it doesn't. +""" + +EVENTS_COLUMNS = { + "eveMac": "STRING (50)", + "eveIp": "STRING (50)", + "eveDateTime": "DATETIME", + "eveEventType": "STRING (30)", + "eveAdditionalInfo": "STRING (250)", + "evePendingAlertEmail": "BOOLEAN", + "evePairEventRowid": "INTEGER", +} + +SESSIONS_COLUMNS = { + "sesMac": "STRING (50)", + "sesIp": "STRING (50)", + "sesEventTypeConnection": "STRING (30)", + "sesDateTimeConnection": "DATETIME", + "sesEventTypeDisconnection": "STRING (30)", + "sesDateTimeDisconnection": "DATETIME", + "sesStillConnected": "BOOLEAN", + "sesAdditionalInfo": "STRING (250)", +} + +APPEVENTS_COLUMNS = { + "index": "INTEGER", + "guid": "TEXT", + "appEventProcessed": "BOOLEAN", + "dateTimeCreated": "TEXT", + "objectType": "TEXT", + "objectGuid": "TEXT", + "objectPlugin": "TEXT", + "objectPrimaryId": "TEXT", + "objectSecondaryId": "TEXT", + "objectForeignKey": "TEXT", + "objectIndex": "TEXT", + "objectIsNew": "BOOLEAN", + "objectIsArchived": "BOOLEAN", + "objectStatusColumn": "TEXT", + "objectStatus": "TEXT", + "appEventType": "TEXT", + "helper1": "TEXT", + "helper2": "TEXT", + "helper3": "TEXT", + "extra": "TEXT", +} + +NOTIFICATIONS_COLUMNS = { + "index": "INTEGER", + "guid": "TEXT", + "dateTimeCreated": "TEXT", + "dateTimePushed": "TEXT", + "status": "TEXT", + "json": "TEXT", + "text": "TEXT", + "html": "TEXT", + "publishedVia": "TEXT", + "extra": "TEXT", +} + +# Table name -> {column_name: sql_type}. Drives both the backfill loop and +# the drift-detection test - the one place these four tables' expected +# column lists are written down in Python. +TABLE_COLUMNS = { + "Events": EVENTS_COLUMNS, + "Sessions": SESSIONS_COLUMNS, + "AppEvents": APPEVENTS_COLUMNS, + "Notifications": NOTIFICATIONS_COLUMNS, +} diff --git a/server/plugins/sync/sync.py b/server/plugins/sync/sync.py index bce085a7..813743dc 100755 --- a/server/plugins/sync/sync.py +++ b/server/plugins/sync/sync.py @@ -307,6 +307,8 @@ def main(): else: # Fire "New Device" events for genuinely new MACs before the Devices # INSERT pre-seeds the table (which would block create_new_devices()). + # This bypasses the standard scan pipeline on purpose - see the + # scan-pipeline skill's sync.py bypass gotcha. if new_devices: now = timeNowUTC() cursor.executemany( @@ -341,7 +343,9 @@ def main(): # that contract by leaving devPresentLastScan to the normal pipeline. # NOTE: this raw SQL bypasses can_overwrite_field() — ALL other fields # including USER/LOCKED-sourced ones are overwritten. Node is fully - # authoritative in this mode. + # authoritative in this mode. Also bypasses the standard scan + # pipeline on purpose - see the scan-pipeline skill's sync.py + # bypass gotcha. _CARBON_COPY_SKIP = {'devMac', 'devPresentLastScan'} update_cols = [col for col in insert_cols if col not in _CARBON_COPY_SKIP] update_clause = ', '.join(f'{col}=excluded.{col}' for col in update_cols) diff --git a/server/scan/device_handling.py b/server/scan/device_handling.py index 84ea1817..f9e37d78 100755 --- a/server/scan/device_handling.py +++ b/server/scan/device_handling.py @@ -9,6 +9,7 @@ from const import vendorsPath, vendorsPathNewest, sql_generateGuid, NULL_EQUIVAL from models.device_instance import DeviceInstance from scan.name_resolution import NameResolver from scan.device_heuristics import guess_icon, guess_type +from scan.presence import current_scan_presence_condition from db.db_helper import sanitize_SQL_input, list_to_where, safe_int from db.db_upgrade import PARENT_MAC_SENTINELS from db.authoritative_handler import ( @@ -206,26 +207,18 @@ def update_presence_from_CurrentScan(db): # (scanPresence = 1). A row can exist purely as identity/inventory data # (scanPresence = 0, e.g. a DHCP reservation) without claiming the device is # online right now - "abstain, not override": any other row for the same MAC - # that does assert presence still wins via this same EXISTS check. - sql.execute(""" + # that does assert presence still wins via this same predicate. + sql.execute(f""" UPDATE Devices SET devPresentLastScan = 1 - WHERE EXISTS ( - SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) + WHERE {current_scan_presence_condition("devMac")} """) # Mark not present if no CurrentScan row for this MAC asserts presence - sql.execute(""" + sql.execute(f""" UPDATE Devices SET devPresentLastScan = 0 - WHERE NOT EXISTS ( - SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) + WHERE NOT {current_scan_presence_condition("devMac")} """) @@ -245,11 +238,7 @@ def update_devLastConnection_from_CurrentScan(db): sql.execute(f""" UPDATE Devices SET devLastConnection = '{startTime}' - WHERE EXISTS ( - SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) + WHERE {current_scan_presence_condition("devMac")} """) diff --git a/server/scan/presence.py b/server/scan/presence.py new file mode 100644 index 00000000..bd89fe82 --- /dev/null +++ b/server/scan/presence.py @@ -0,0 +1,30 @@ +import re + +_SQL_IDENTIFIER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)?$") + + +def current_scan_presence_condition(mac_column: str) -> str: + """SQL fragment: TRUE if any CurrentScan row for mac_column asserts presence. + The one and only definition of 'is this MAC present this cycle' - every + caller uses this, nobody writes their own CurrentScan presence predicate. + + mac_column MUST be a trusted, hardcoded SQL column/table.column reference + written by NetAlertX code (e.g. "devMac", "CurrentScan.scanMac") - this + function does raw string interpolation, not parameterized SQL. Never pass + plugin data, user input, or any runtime string value here. + + Not usable everywhere a presence check appears: the "New Connections" + query in session_events.py and the raw Sessions insert in + create_new_devices() (device_handling.py) both need the actual + scanLastIP/scanVendor *values* off the presence-asserting row via a + MIN()/GROUP BY aggregate, not just a boolean - see + scan-pipeline-hardening.md Design §1's correction for why those two + (plus "IP Changed", an eighth non-canonical site) keep their own + hand-written aggregation instead of calling this helper. + """ + if not _SQL_IDENTIFIER_RE.match(mac_column): + raise ValueError(f"mac_column must be a plain identifier, got: {mac_column!r}") + return f"""EXISTS ( + SELECT 1 FROM CurrentScan + WHERE scanMac = {mac_column} AND scanPresence = 1 + )""" diff --git a/server/scan/session_events.py b/server/scan/session_events.py index a26eb382..edca2fd3 100755 --- a/server/scan/session_events.py +++ b/server/scan/session_events.py @@ -14,6 +14,7 @@ from scan.device_handling import ( update_presence_from_CurrentScan ) from helper import get_setting_value +from scan.presence import current_scan_presence_condition from db.db_helper import print_table_schema from utils.datetime_utils import timeNowUTC from logger import mylog, Logger @@ -190,10 +191,7 @@ def insert_events(db): AND devCanSleep = 0 AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT EXISTS (SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) """) + AND NOT {current_scan_presence_condition("devMac")} """) # Check device down – sleeping devices whose sleep window has expired mylog("debug", "[Events] - 1b - Devices down (sleep expired)") @@ -207,9 +205,7 @@ def insert_events(db): AND devIsSleeping = 0 AND devPresentLastScan = 0 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT EXISTS (SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1) + AND NOT {current_scan_presence_condition("devMac")} AND NOT EXISTS (SELECT 1 FROM Events WHERE eveMac = devMac AND eveEventType = 'Device Down' @@ -274,10 +270,7 @@ def insert_events(db): WHERE devAlertDown = 0 AND devPresentLastScan = 1 AND {_SQL_NOT_FORCED_ONLINE} - AND NOT EXISTS (SELECT 1 FROM CurrentScan - WHERE devMac = scanMac - AND scanPresence = 1 - ) """) + AND NOT {current_scan_presence_condition("devMac")} """) # Check IP Changed mylog("debug", "[Events] - 4 - IP Changes") diff --git a/test/db/test_schema_drift_guard.py b/test/db/test_schema_drift_guard.py new file mode 100644 index 00000000..11bd79ed --- /dev/null +++ b/test/db/test_schema_drift_guard.py @@ -0,0 +1,148 @@ +""" +Tests for the Events/Sessions/AppEvents/Notifications schema drift guard +(server/db/schema_columns.py, server/db/db_upgrade.py's ensure_table_columns()) +- see scan-pipeline-hardening.md Design §3. + +Three things are tested: +1. No drift today between TABLE_COLUMNS and the real server/db/schema/app.sql. +2. The drift-detection logic can actually detect a real mismatch (not just a + check that always passes trivially) - per the prd-writing skill's rule + that a guard needs its own test, separate from "no drift found today". +3. AppEvents/Notifications specifically have a *second* schema-definition + surface beyond app.sql (their own inline CREATE TABLE IF NOT EXISTS in + application code) - keep that in sync too, using the real classes rather + than re-parsing their SQL as text. +4. ensure_table_columns() backfill: a table missing one column gets it added + with the right type. +""" + +import os +import sqlite3 +import sys +from unittest.mock import patch + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server")) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) + +from db.schema_columns import TABLE_COLUMNS # noqa: E402 +from db.db_upgrade import ensure_table_columns # noqa: E402 +from db_test_helpers import make_db, DummyDB # noqa: E402 + +_APP_SQL_PATH = os.path.join( + os.path.dirname(__file__), "..", "..", "server", "db", "schema", "app.sql" +) + + +def _columns_from_ddl(ddl_sql, table): + """Execute a CREATE TABLE (or full multi-statement schema) into a fresh + in-memory connection and return the resulting column name set - uses + SQLite's own DDL parser rather than a hand-rolled regex, so it can't be + fooled by formatting differences that a text-based diff would trip on.""" + conn = sqlite3.connect(":memory:") + try: + conn.executescript(ddl_sql) + return {row[1] for row in conn.execute(f'PRAGMA table_info("{table}")').fetchall()} + finally: + conn.close() + + +def _drift(ddl_sql, table): + """Column names present in one source but not the other. Empty = no drift.""" + expected = set(TABLE_COLUMNS[table].keys()) + actual = _columns_from_ddl(ddl_sql, table) + return expected.symmetric_difference(actual) + + +class TestNoDriftAgainstRealAppSql: + @pytest.mark.parametrize("table", list(TABLE_COLUMNS.keys())) + def test_table_matches_app_sql(self, table): + app_sql = open(_APP_SQL_PATH).read() + drift = _drift(app_sql, table) + assert not drift, ( + f"{table}: TABLE_COLUMNS (server/db/schema_columns.py) and " + f"app.sql disagree on {drift} - update whichever one is stale" + ) + + +class TestGuardActuallyDetectsDrift: + """Proves the comparison logic can fail, not just a check that always + reports success - without this, it's possible to ship a guard that + passes regardless of what it's given.""" + + def test_missing_columns_detected(self): + broken_sql = "CREATE TABLE Events (eveMac TEXT, eveIp TEXT);" + drift = _drift(broken_sql, "Events") + assert drift == { + "eveDateTime", "eveEventType", "eveAdditionalInfo", + "evePendingAlertEmail", "evePairEventRowid", + } + + def test_extra_column_detected(self): + broken_sql = "CREATE TABLE Sessions (sesMac TEXT, sesUnexpectedNewColumn TEXT);" + drift = _drift(broken_sql, "Sessions") + assert "sesUnexpectedNewColumn" in drift + + +class TestInlineDDLMatchesConstant: + """AppEvents/Notifications each have a second schema-definition surface + beyond app.sql - the inline CREATE TABLE IF NOT EXISTS in their own + Python classes (found during Design §3 implementation - see + scan-pipeline-hardening.md's correction). Exercised via the real classes, + not by re-parsing their embedded SQL as text.""" + + def test_app_events_inline_ddl_matches_constant(self): + from workflows.app_events import AppEvent_obj + + conn = make_db() + db = DummyDB(conn) + AppEvent_obj(db) # drops + recreates AppEvents with the real inline DDL + + cols = {row[1] for row in conn.execute('PRAGMA table_info("AppEvents")').fetchall()} + assert cols == set(TABLE_COLUMNS["AppEvents"].keys()) + conn.close() + + def test_notifications_inline_ddl_matches_constant(self): + from models.notification_instance import NotificationInstance + + conn = make_db() + db = DummyDB(conn) + with patch("models.notification_instance.get_setting_value", return_value=""), \ + patch("models.notification_instance.Logger"): + NotificationInstance(db) + + cols = {row[1] for row in conn.execute('PRAGMA table_info("Notifications")').fetchall()} + assert cols == set(TABLE_COLUMNS["Notifications"].keys()) + conn.close() + + +class TestEnsureTableColumnsBackfill: + """ensure_table_columns() must repair a table that's missing a column - + mirroring however Devices' existing 18 ensure_column() calls are (or + aren't currently) tested, generalized to the four registered tables.""" + + @pytest.mark.parametrize("table", list(TABLE_COLUMNS.keys())) + def test_missing_column_is_backfilled(self, table): + columns = TABLE_COLUMNS[table] + first_col, first_type = next(iter(columns.items())) + remaining = {c: t for c, t in columns.items() if c != first_col} + + conn = sqlite3.connect(":memory:") + col_defs = ", ".join(f'"{c}" {t}' for c, t in remaining.items()) + conn.execute(f"CREATE TABLE {table} ({col_defs})") + cur = conn.cursor() + + ok = ensure_table_columns(cur, table) + assert ok, f"ensure_table_columns({table}) reported failure" + + cols = {row[1] for row in conn.execute(f'PRAGMA table_info("{table}")').fetchall()} + assert cols == set(columns.keys()), f"{table}: backfill did not restore {first_col}" + conn.close() + + def test_missing_table_skips_without_error(self): + conn = sqlite3.connect(":memory:") + cur = conn.cursor() + ok = ensure_table_columns(cur, "Notifications") + assert ok, "a not-yet-created table must not be treated as a failure" + conn.close() diff --git a/test/scan/test_presence_helper.py b/test/scan/test_presence_helper.py new file mode 100644 index 00000000..c2b2ddc7 --- /dev/null +++ b/test/scan/test_presence_helper.py @@ -0,0 +1,95 @@ +""" +Tests for server/scan/presence.py's current_scan_presence_condition() - +the shared predicate helper from scan-pipeline-hardening.md Design §1. + +Two things are tested: +1. The helper itself: correct SQL fragment, and rejects anything that isn't + a plain SQL identifier (the trust-boundary check - this function does raw + string interpolation, never parameterized SQL). +2. A positive, AST-based guard (not a grep for one hand-written spelling, + which is trivially defeated by an equivalent one - scanPresence <> 0, + bare scanPresence, NOT scanPresence = 0, etc.) that each migrated + consumer function's source actually calls the helper the expected number + of times. This is what stops a future change from quietly reintroducing + a hand-written predicate instead of calling the shared one. + +Not covered here on purpose: "New Connections" (session_events.py) and the +raw Sessions insert (device_handling.py's create_new_devices()) - both need +the actual scanLastIP/scanVendor *values* off the presence-asserting row via +MIN()/GROUP BY, not just a boolean, so they keep their own hand-written +aggregation - see scan-pipeline-hardening.md Design §1's correction. +""" + +import ast +import inspect +import os +import sys +import textwrap + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "..", "server")) + +from scan.presence import current_scan_presence_condition # noqa: E402 +from scan import device_handling # noqa: E402 +from scan import session_events # noqa: E402 + + +class TestHelperCorrectness: + def test_returns_expected_sql_fragment(self): + result = current_scan_presence_condition("devMac") + assert "EXISTS (" in result + assert "SELECT 1 FROM CurrentScan" in result + assert "scanMac = devMac" in result + assert "scanPresence = 1" in result + + def test_accepts_qualified_column_reference(self): + result = current_scan_presence_condition("CurrentScan.scanMac") + assert "scanMac = CurrentScan.scanMac" in result + + @pytest.mark.parametrize("bad_value", [ + "devMac; DROP TABLE Devices--", + "devMac OR 1=1", + "'; DELETE FROM Devices; --", + "devMac)", + "", + "123devMac", + ]) + def test_rejects_non_identifier_input(self, bad_value): + with pytest.raises(ValueError): + current_scan_presence_condition(bad_value) + + +def _call_count(func, target_name="current_scan_presence_condition"): + """Count calls to target_name within func's own source (AST-based, not + a text grep - resilient to reformatting, doesn't care how a bypass + might be spelled, only whether the actual call is present).""" + source = textwrap.dedent(inspect.getsource(func)) + tree = ast.parse(source) + count = 0 + for node in ast.walk(tree): + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == target_name: + count += 1 + return count + + +class TestConsumersCallTheHelper: + """Guards the five sites that were migrated to the shared predicate.""" + + def test_update_presence_from_current_scan_calls_helper_twice(self): + assert _call_count(device_handling.update_presence_from_CurrentScan) == 2, ( + "update_presence_from_CurrentScan() has two statements (present/not-present) " + "- both must call current_scan_presence_condition()" + ) + + def test_update_dev_last_connection_calls_helper_once(self): + assert _call_count(device_handling.update_devLastConnection_from_CurrentScan) == 1 + + def test_insert_events_calls_helper_at_least_three_times(self): + """insert_events() contains four queries total - Device Down (x2), + Disconnected, and New Connections. Only the first three are plain + boolean-predicate sites; New Connections keeps its own present_agg/ + MIN(scanLastIP) aggregation on purpose (see module docstring), so + this asserts >= 3, not == 4.""" + assert _call_count(session_events.insert_events) >= 3