diff --git a/db/init.sql b/db/init.sql index d8a99db..659b0d7 100644 --- a/db/init.sql +++ b/db/init.sql @@ -85,5 +85,11 @@ CREATE TABLE IF NOT EXISTS repacss_environment.unknown_devices ( mac macaddr PRIMARY KEY, first_seen timestamptz NOT NULL, last_seen timestamptz NOT NULL, - message_count_24h int NOT NULL + -- Running total since first_seen, not a rolling window: nothing trims it. + -- The 24 hour summary the README describes is a query over this plus + -- last_seen, not a maintained counter. + hit_count int NOT NULL, + -- Last payload seen, truncated by the writer. Helps identify what a stray + -- device actually is before deciding whether to register or block it. + last_payload text ); \ No newline at end of file diff --git a/host/app/current_state.py b/host/app/current_state.py index 3393aa8..299236d 100644 --- a/host/app/current_state.py +++ b/host/app/current_state.py @@ -27,7 +27,7 @@ def get_recent(conn) -> dict: cur.execute( """ SELECT upper(replace(mac::text, ':', '')), running_firmware_version - FROM repacss_enviroment.current_status + FROM repacss_environment.current_status WHERE last_seen > now() - interval '1 minutes' """ ) diff --git a/host/app/unknown_devices.py b/host/app/unknown_devices.py index f3f53d9..858eb6b 100644 --- a/host/app/unknown_devices.py +++ b/host/app/unknown_devices.py @@ -9,7 +9,7 @@ def record(conn, mac: str, payload_txt) -> None: with conn.cursor() as cur: cur.execute( """ - INSERT INTO repacss_enviroment.unknown_devices + INSERT INTO repacss_environment.unknown_devices (mac, first_seen, last_seen, hit_count, last_payload) VALUES (%s, now(), now(), 1, %s) ON CONFLICT (mac) DO UPDATE diff --git a/host/tests/test_current_state.py b/host/tests/test_current_state.py new file mode 100644 index 0000000..6daa05e --- /dev/null +++ b/host/tests/test_current_state.py @@ -0,0 +1,58 @@ +"""Tests for app.current_state (the current_status table). + +upsert(conn, mac, fw) +- Runs the current_status upsert with (mac, fw) parameters. +- Swallows DB errors (logs, does not raise) so a hello is never lost to a + write blip. + +get_recent(conn) +- Maps rows to {mac: {firmware_version}}, with the mac normalised to the + compact uppercase form the devices report. +- Returns {} on a DB error. +""" + +import app.current_state as cs + + +# --- upsert ------------------------------------------------------------------- + +def test_upsert_runs_upsert(cursor_conn): + conn, cur = cursor_conn + cs.upsert(conn, "MAC", "v1.0") + cur.execute.assert_called_once() + sql, params = cur.execute.call_args.args + assert "INSERT INTO repacss_environment.current_status" in sql + assert "ON CONFLICT (mac) DO UPDATE" in sql + assert params == ("MAC", "v1.0") + + +def test_upsert_swallows_error(cursor_conn): + conn, cur = cursor_conn + cur.execute.side_effect = Exception("boom") + cs.upsert(conn, "MAC", "v1.0") # must not raise + + +# --- get_recent --------------------------------------------------------------- + +def test_get_recent_maps_rows(cursor_conn): + conn, cur = cursor_conn + cur.fetchall.return_value = [("MAC1", "v0.0.2"), ("MAC2", "v0.0.3")] + assert cs.get_recent(conn) == { + "MAC1": {"firmware_version": "v0.0.2"}, + "MAC2": {"firmware_version": "v0.0.3"}, + } + + +def test_get_recent_queries_the_right_schema(cursor_conn): + # Guards the repacss_enviroment typo that made this silently return {}. + conn, cur = cursor_conn + cur.fetchall.return_value = [] + cs.get_recent(conn) + sql = cur.execute.call_args.args[0] + assert "repacss_environment.current_status" in sql + + +def test_get_recent_returns_empty_on_error(cursor_conn): + conn, cur = cursor_conn + cur.execute.side_effect = Exception("db down") + assert cs.get_recent(conn) == {} diff --git a/host/tests/test_provisioning_service.py b/host/tests/test_provisioning_service.py index a8fb53c..b06270b 100644 --- a/host/tests/test_provisioning_service.py +++ b/host/tests/test_provisioning_service.py @@ -2,30 +2,24 @@ Functions covered and their cases: -is_enabled(mac, role) -- Primary -> True, Standby -> True, any other role -> False. - build_config(mac, device) -- Returns the full config dict with configured=True and enabled from is_enabled. +- Returns the full config dict, with enabled taken straight from the registry + row (the old is_enabled role filter is gone: device_map.enabled is the truth). - Missing a required device key raises KeyError (surfaces bad registry data). -get_device_state() -- Maps DB rows to {mac: {firmware_version}}. -- Returns {} on a DB error. - -upsert_device_state(mac, fw) -- Runs the current_status upsert with (mac, fw) parameters. -- Swallows DB errors (logs, does not raise) so a hello is never lost to a write blip. - -(The device_map cache moved to app.device_registry; see test_device_registry.py.) - on_connect(...) - Subscribes to the hello topic. on_message(...) -- Known device -> upserts state and publishes a config. -- Unknown device -> publishes configured:false with reason "unknown mac". +- Known device -> upserts current_status and publishes a config. +- Unknown device -> records it in unknown_devices and publishes NOTHING. + The silence is intended: the device stays unconfigured and keeps helloing, + and the host keeps a record instead of replying. - Invalid JSON -> does nothing (no upsert, no publish). +- Oversized payload -> dropped before parsing. + +The per-table SQL lives in app.current_state / app.unknown_devices / +app.device_registry and is tested in their own files. """ import json @@ -36,24 +30,11 @@ import app.provisioning_service as prov -# --- is_enabled --------------------------------------------------------------- - -@pytest.mark.parametrize("role,expected", [ - ("Primary", True), - ("Standby", True), - ("Unknown", False), - ("controllerA", False), - ("", False), -]) -def test_is_enabled(role, expected): - assert prov.is_enabled("AABBCCDDEEFF", role) is expected - - # --- build_config ------------------------------------------------------------- -def test_build_config_primary_full_shape(): +def test_build_config_full_shape(): # rack_id is free-form text (rack codenames like "rpg93"), not a number. - cfg = prov.build_config("MAC", {"rack_id": "rpg93", "role": "Primary"}) + cfg = prov.build_config("MAC", {"rack_id": "rpg93", "role": "Primary", "enabled": True}) assert cfg == { "message_type": "config", "mac": "MAC", @@ -64,48 +45,16 @@ def test_build_config_primary_full_shape(): } -def test_build_config_missing_key_raises_keyerror(): - with pytest.raises(KeyError): - prov.build_config("MAC", {"role": "Primary"}) # no rack_id - - -# --- get_device_state --------------------------------------------------------- - -def test_get_device_state_maps_rows(monkeypatch, cursor_conn): - conn, cur = cursor_conn - cur.fetchall.return_value = [("MAC1", "v0.0.2"), ("MAC2", "v0.0.3")] - monkeypatch.setattr(prov, "conn", conn) - assert prov.get_device_state() == { - "MAC1": {"firmware_version": "v0.0.2"}, - "MAC2": {"firmware_version": "v0.0.3"}, - } - - -def test_get_device_state_returns_empty_on_error(monkeypatch, cursor_conn): - conn, cur = cursor_conn - cur.execute.side_effect = Exception("db down") - monkeypatch.setattr(prov, "conn", conn) - assert prov.get_device_state() == {} +def test_build_config_passes_enabled_false_through(): + # enabled:false is a valid applied config (remote disable), not a rejection. + cfg = prov.build_config("MAC", {"rack_id": "rpg93", "role": "Standby", "enabled": False}) + assert cfg["enabled"] is False + assert cfg["configured"] is True -# --- upsert_device_state ------------------------------------------------------ - -def test_upsert_device_state_runs_upsert(monkeypatch, cursor_conn): - conn, cur = cursor_conn - monkeypatch.setattr(prov, "conn", conn) - prov.upsert_device_state("MAC", "v1.0") - cur.execute.assert_called_once() - sql, params = cur.execute.call_args.args - assert "INSERT INTO repacss_environment.current_status" in sql - assert "ON CONFLICT (mac) DO UPDATE" in sql - assert params == ("MAC", "v1.0") - - -def test_upsert_device_state_swallows_error(monkeypatch, cursor_conn): - conn, cur = cursor_conn - cur.execute.side_effect = Exception("boom") - monkeypatch.setattr(prov, "conn", conn) - prov.upsert_device_state("MAC", "v1.0") # must not raise +def test_build_config_missing_key_raises_keyerror(): + with pytest.raises(KeyError): + prov.build_config("MAC", {"role": "Primary", "enabled": True}) # no rack_id # --- on_connect --------------------------------------------------------------- @@ -119,10 +68,11 @@ def test_on_connect_subscribes_to_hello(): # --- on_message --------------------------------------------------------------- def test_on_message_known_device_publishes_config(monkeypatch, make_msg): - monkeypatch.setattr(prov, "upsert_device_state", MagicMock()) + upsert = MagicMock() + monkeypatch.setattr(prov.current_state, "upsert", upsert) monkeypatch.setattr( prov.device_registry, "lookup", - MagicMock(return_value={"rack_id": "rpg93", "role": "Primary"}), + MagicMock(return_value={"rack_id": "rpg93", "role": "Primary", "enabled": True}), ) client = MagicMock() msg = make_msg( @@ -132,37 +82,54 @@ def test_on_message_known_device_publishes_config(monkeypatch, make_msg): prov.on_message(client, None, msg) - prov.upsert_device_state.assert_called_once_with("ECE3347C07D0", "v0.0.2") + assert upsert.call_args.args[1:] == ("ECE3347C07D0", "v0.0.2") client.publish.assert_called_once() topic, payload = client.publish.call_args.args[0], client.publish.call_args.args[1] assert topic == "repacss/devices/ECE3347C07D0/config" body = json.loads(payload) assert body["configured"] is True assert body["role"] == "Primary" + assert body["rack_id"] == "rpg93" -def test_on_message_unknown_device_publishes_not_configured(monkeypatch, make_msg): - monkeypatch.setattr(prov, "upsert_device_state", MagicMock()) +def test_on_message_unknown_device_records_and_stays_silent(monkeypatch, make_msg): + monkeypatch.setattr(prov.current_state, "upsert", MagicMock()) monkeypatch.setattr(prov.device_registry, "lookup", MagicMock(return_value=None)) + record = MagicMock() + monkeypatch.setattr(prov.unknown_devices, "record", record) client = MagicMock() - msg = make_msg( - "repacss/devices/AABBCCDDEEFF/hello", - {"message_type": "hello", "mac": "AABBCCDDEEFF"}, - ) + payload = {"message_type": "hello", "mac": "AABBCCDDEEFF"} + msg = make_msg("repacss/devices/AABBCCDDEEFF/hello", payload) prov.on_message(client, None, msg) - body = json.loads(client.publish.call_args.args[1]) - assert body["configured"] is False - assert body["reason"] == "unknown mac" + # Recorded with the raw payload text, and deliberately NOT answered: + # the device keeps helloing rather than being told it is unknown. + record.assert_called_once() + assert record.call_args.args[1] == "AABBCCDDEEFF" + assert json.loads(record.call_args.args[2]) == payload + client.publish.assert_not_called() def test_on_message_invalid_json_does_nothing(monkeypatch, make_msg): - monkeypatch.setattr(prov, "upsert_device_state", MagicMock()) + upsert = MagicMock() + monkeypatch.setattr(prov.current_state, "upsert", upsert) client = MagicMock() msg = make_msg("repacss/devices/ECE3347C07D0/hello", "not-json{") prov.on_message(client, None, msg) client.publish.assert_not_called() - prov.upsert_device_state.assert_not_called() + upsert.assert_not_called() + + +def test_on_message_oversized_payload_dropped(monkeypatch, make_msg): + upsert = MagicMock() + monkeypatch.setattr(prov.current_state, "upsert", upsert) + client = MagicMock() + msg = make_msg("repacss/devices/ECE3347C07D0/hello", "x" * (prov.MAX_HELLO_BYTES + 1)) + + prov.on_message(client, None, msg) + + upsert.assert_not_called() + client.publish.assert_not_called() diff --git a/host/tests/test_unknown_devices.py b/host/tests/test_unknown_devices.py new file mode 100644 index 0000000..cf75c0e --- /dev/null +++ b/host/tests/test_unknown_devices.py @@ -0,0 +1,53 @@ +"""Tests for app.unknown_devices (the unknown_devices table). + +record(conn, mac, payload_txt) +- Upserts one row per mac: inserts on first sight, bumps hit_count and + last_seen on repeat. +- Truncates the stored payload to _MAX_PAYLOAD_CHARS so a chatty or hostile + device cannot write unbounded text. +- Passes mac and payload as query parameters, never interpolated (the mac + comes off the MQTT topic and is untrusted). +- Swallows DB errors (logs, does not raise). +""" + +import app.unknown_devices as ud + + +def test_record_upserts_row(cursor_conn): + conn, cur = cursor_conn + ud.record(conn, "AABBCCDDEEFF", '{"message_type":"hello"}') + cur.execute.assert_called_once() + sql, params = cur.execute.call_args.args + assert "INSERT INTO repacss_environment.unknown_devices" in sql + assert "ON CONFLICT (mac) DO UPDATE" in sql + assert params == ("AABBCCDDEEFF", '{"message_type":"hello"}') + + +def test_record_bumps_hit_count_on_conflict(cursor_conn): + conn, cur = cursor_conn + ud.record(conn, "AABBCCDDEEFF", "{}") + sql = cur.execute.call_args.args[0] + assert "hit_count = unknown_devices.hit_count + 1" in sql + + +def test_record_truncates_payload(cursor_conn): + conn, cur = cursor_conn + ud.record(conn, "AABBCCDDEEFF", "x" * (ud._MAX_PAYLOAD_CHARS + 500)) + _, params = cur.execute.call_args.args + assert len(params[1]) == ud._MAX_PAYLOAD_CHARS + + +def test_record_uses_parameters_not_interpolation(cursor_conn): + # The mac comes off the MQTT topic and is untrusted, so it must never be + # baked into the SQL string. + conn, cur = cursor_conn + ud.record(conn, "'; DROP TABLE unknown_devices; --", "{}") + sql, params = cur.execute.call_args.args + assert "DROP TABLE" not in sql + assert params[0] == "'; DROP TABLE unknown_devices; --" + + +def test_record_swallows_error(cursor_conn): + conn, cur = cursor_conn + cur.execute.side_effect = Exception("boom") + ud.record(conn, "AABBCCDDEEFF", "{}") # must not raise