diff --git a/CHANGELOG.rst b/CHANGELOG.rst index f5ae834..3b8324e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,27 @@ Changelog Unreleased ========== +Changed +------- +- **Raw reservation writes are now validated** (#148). + ``update_reservations()`` and ``update_reservations_confirmed()`` sent + their entry dicts unchecked, so an unknown mode such as 7, an hour of 99 + or a setpoint of 200 (100 degC) went to the device as-is. A Python + ``True`` for ``enable`` was worse: the confirmed helper read it as 1, + which means disabled. Every entry is now checked before anything is + sent, with the same rules as ``build_reservation_entry()``: the six + protocol fields present and plain integers, ``enable`` 1 or 2, ``week`` + a day bitfield, ``hour`` 0-23, ``min`` 0-59 and ``mode`` a + ``DhwOperationSetting`` id. ``param`` is held to the setpoint range the + heater reports in its feature data (``dhw_temperature_min_raw`` to + ``dhw_temperature_max_raw``), which is requested if it is not cached; + clearing the schedule with an empty list needs none. A bad entry raises + ``ParameterValidationError`` or ``RangeValidationError``, and missing + feature data raises ``DeviceCapabilityError``. Callers that wrote + entries outside these ranges and relied on the heater to clamp them now + get an error instead. The checks are also available on their own as + ``nwp500.mqtt.control.validate_reservation_entries()``. + Documentation ------------- - **New: what starts a recovery** diff --git a/docs/how-to/schedule-operation.rst b/docs/how-to/schedule-operation.rst index 2cb9f30..fc145eb 100644 --- a/docs/how-to/schedule-operation.rst +++ b/docs/how-to/schedule-operation.rst @@ -358,6 +358,16 @@ multiple entries at once: device, reservations, enabled=True ) +``update_reservations()`` and ``update_reservations_confirmed()`` check +every raw entry before anything is sent, and raise +:class:`~nwp500.exceptions.ParameterValidationError` or +:class:`~nwp500.exceptions.RangeValidationError` on a bad one. Each +``param`` must lie within the setpoint range the heater reports in its +feature data (``dhw_temperature_min_raw`` to ``dhw_temperature_max_raw``, +both in half-degrees Celsius like ``param``). The heater clamps an +out-of-range setpoint itself; the check makes a bad entry fail early and +clearly instead. + **Disable reservations** (entries are preserved on the device): .. code-block:: python diff --git a/docs/reference/python_api/mqtt_client.rst b/docs/reference/python_api/mqtt_client.rst index d23e4f4..0a7e3d9 100644 --- a/docs/reference/python_api/mqtt_client.rst +++ b/docs/reference/python_api/mqtt_client.rst @@ -490,6 +490,24 @@ update_reservations() :type reservations: Sequence[dict[str, Any]] :param enabled: Global reservation enable flag :type enabled: bool + :raises ParameterValidationError: If an entry is missing a field, has a + non-integer field (a ``bool`` or ``float`` is rejected), an ``enable`` + other than ``1``/``2``, or a ``week`` that is not a day bitfield + (``2``-``254``, bit 0 clear) + :raises RangeValidationError: If an entry's ``hour`` is outside + ``0``-``23``, ``min`` outside ``0``-``59``, ``mode`` not a + :class:`~nwp500.enums.DhwOperationSetting` id (``1``-``6``), or + ``param`` outside the setpoint range the device reports + (``dhw_temperature_min_raw``-``dhw_temperature_max_raw``, in + half-degrees Celsius) + :raises DeviceCapabilityError: If the device's feature data is not + available, so its setpoint range is unknown + + Every entry is checked before anything is sent. The setpoint range + comes from the device's feature data, which is requested if it is not + cached; an empty list needs no feature data. The same checks are + available on their own as + :func:`nwp500.mqtt.control.validate_reservation_entries`. **Example:** diff --git a/src/nwp500/mqtt/control.py b/src/nwp500/mqtt/control.py index 08d6263..7e942dc 100644 --- a/src/nwp500/mqtt/control.py +++ b/src/nwp500/mqtt/control.py @@ -27,7 +27,7 @@ """ import logging -from collections.abc import Awaitable, Callable, Sequence +from collections.abc import Awaitable, Callable, Mapping, Sequence from datetime import UTC, datetime from typing import Any @@ -120,6 +120,120 @@ def fail(message: str, parameter: str, value: Any) -> None: ) +RESERVATION_ENTRY_FIELDS = ("enable", "week", "hour", "min", "mode", "param") +# DhwOperationSetting ids are contiguous (1-6), so a range check covers them. +_DHW_MODE_MIN = min(DhwOperationSetting) +_DHW_MODE_MAX = max(DhwOperationSetting) + + +def validate_reservation_entries( + reservations: Sequence[Mapping[str, Any]], + features: DeviceFeature | None = None, +) -> None: + """Check raw reservation entries before they are written to the device. + + :class:`~nwp500.models.ReservationEntry` accepts any integers so that + device read-backs always parse; writes are held to what the device + accepts. Each entry must carry the six protocol fields as plain + integers (a ``bool`` or ``float`` is rejected): ``enable`` 1 (off) or + 2 (on), ``week`` a day bitfield (2-254, bit 0 clear), ``hour`` 0-23, + ``min`` 0-59 and ``mode`` a :class:`~nwp500.enums.DhwOperationSetting` + id. + + ``param`` is the setpoint in half-degrees Celsius. With ``features`` + it must lie within the range the heater reports, + ``dhw_temperature_min_raw`` to ``dhw_temperature_max_raw``; without + them, only within the single byte the protocol carries (0-255). + + Args: + reservations: Raw entry dicts, as passed to ``update_reservations``. + features: The device's feature data, for its setpoint limits. + + Raises: + ParameterValidationError: On a missing field, a non-integer field, + or a bad ``enable`` or ``week``. + RangeValidationError: On ``hour``, ``min``, ``mode`` or ``param`` + outside its range. + """ + for index, entry in enumerate(reservations, start=1): + _check_reservation_entry(index, entry) + if features is not None: + _check_reservation_setpoints(reservations, features) + + +def _check_reservation_entry(index: int, entry: Mapping[str, Any]) -> None: + """Check one entry's fields, except ``param`` against device limits.""" + for name in RESERVATION_ENTRY_FIELDS: + if name not in entry: + raise ParameterValidationError( + f"entry {index}: missing field {name!r}", + parameter=f"reservation[{index}].{name}", + ) + if not _is_int(entry[name]): + raise ParameterValidationError( + f"entry {index}: {name} must be an integer", + parameter=f"reservation[{index}].{name}", + value=entry[name], + ) + + enable = entry["enable"] + if enable not in (1, 2): + raise ParameterValidationError( + f"entry {index}: enable must be 1 (off) or 2 (on), got {enable}", + parameter=f"reservation[{index}].enable", + value=enable, + ) + week = entry["week"] + if not (0 < week <= 254 and week % 2 == 0): + raise ParameterValidationError( + f"entry {index}: week={week} is not a day bitfield " + "(Sun=128 .. Sat=2, at least one day, bit 0 clear)", + parameter=f"reservation[{index}].week", + value=week, + ) + + ranges = [ + ("hour", 0, 23), + ("min", 0, 59), + ("mode", int(_DHW_MODE_MIN), int(_DHW_MODE_MAX)), + ("param", 0, 255), + ] + for name, low, high in ranges: + value = entry[name] + if not low <= value <= high: + raise RangeValidationError( + f"entry {index}: {name} must be between {low} and {high}, " + f"got {value}", + field=f"reservation[{index}].{name}", + value=value, + min_value=low, + max_value=high, + ) + + +def _check_reservation_setpoints( + reservations: Sequence[Mapping[str, Any]], features: DeviceFeature +) -> None: + """Hold each entry's ``param`` to the setpoint range the heater reports. + + Both ``param`` and the feature limits are half-degrees Celsius, so they + compare without conversion. + """ + low = features.dhw_temperature_min_raw + high = features.dhw_temperature_max_raw + for index, entry in enumerate(reservations, start=1): + param = entry["param"] + if not low <= param <= high: + raise RangeValidationError( + f"entry {index}: param={param} (half-degrees C) is outside " + f"the device's setpoint range {low}-{high}", + field=f"reservation[{index}].param", + value=param, + min_value=low, + max_value=high, + ) + + class MqttDeviceController: """ Manages device control commands for Navien devices. @@ -569,6 +683,27 @@ async def set_dhw_temperature( [preferred_to_half_celsius(temperature)], ) + async def _get_reservation_limits(self, device: Device) -> DeviceFeature: + """Fetch the feature data that carries the device's setpoint range. + + Mirrors :func:`~nwp500.command_decorators.requires_capability`: a + failed fetch (a feature-response timeout, a lost connection) is + reported as :class:`DeviceCapabilityError`, like a missing result. + """ + capability = "dhw_temperature_setting_use" + message = "Unable to validate reservation temperatures" + try: + features = await self._get_device_features(device) + except DeviceCapabilityError: + raise + except Exception as e: + raise DeviceCapabilityError(capability, f"{message}: {e!s}") from e + if features is None: + raise DeviceCapabilityError( + capability, f"{message}: device features not available." + ) + return features + async def update_reservations( self, device: Device, @@ -579,6 +714,11 @@ async def update_reservations( """ Update programmed reservations for temperature/mode changes. + Every entry is checked before anything is sent (see + :func:`validate_reservation_entries`). Each ``param`` is held to + the setpoint range the heater reports in its feature data, which is + requested from the device if it is not cached yet. + Args: device: Device object reservations: List of reservation entries @@ -586,7 +726,21 @@ async def update_reservations( Returns: Publish packet ID + + Raises: + ParameterValidationError: If an entry has a missing or + non-integer field, or a bad ``enable`` or ``week``. + RangeValidationError: If an entry's ``hour``, ``min`` or + ``mode`` is out of range, or its ``param`` is outside the + device's setpoint range. + DeviceCapabilityError: If the device's feature data, and so + its setpoint range, is not available. """ + validate_reservation_entries(reservations) + if reservations: + features = await self._get_reservation_limits(device) + _check_reservation_setpoints(reservations, features) + # See docs/reference/protocol/mqtt_protocol.rst "Reservations" for the # command code (16777226) and the reservation object fields # (enable, week, hour, min, mode, param). diff --git a/src/nwp500/reservations.py b/src/nwp500/reservations.py index fe890c1..5ac90a0 100644 --- a/src/nwp500/reservations.py +++ b/src/nwp500/reservations.py @@ -17,6 +17,7 @@ from .converters import device_bool_from_python from .encoding import build_reservation_entry, encode_week_bitfield from .models import ReservationEntry, ReservationSchedule +from .mqtt.control import validate_reservation_entries if TYPE_CHECKING: from .models import Device @@ -119,7 +120,22 @@ async def update_reservations_confirmed( what was just written. This avoids resolving on a stale/unrelated ``rsv/rd`` message (e.g. from a concurrent read or a previous write) that happens to arrive in the same window. + + Entries are checked before anything is sent; see + :func:`nwp500.mqtt.control.validate_reservation_entries`. + + Raises: + ParameterValidationError: If an entry has a missing or non-integer + field, or a bad ``enable`` or ``week``. + RangeValidationError: If an entry's ``hour``, ``min`` or ``mode`` + is out of range, or its ``param`` is outside the device's + setpoint range. """ + # Structural checks first, so a bad entry neither subscribes nor builds + # a coerced expectation (``ReservationEntry`` turns ``True`` into 1). + # The setpoint range is checked by ``update_reservations`` against the + # device's feature data, still before anything is published. + validate_reservation_entries(reservations) expected = ReservationSchedule( reservationUse=device_bool_from_python(enabled), reservation=[ReservationEntry(**entry) for entry in reservations], diff --git a/tests/test_reservation_validation.py b/tests/test_reservation_validation.py new file mode 100644 index 0000000..ebd565a --- /dev/null +++ b/tests/test_reservation_validation.py @@ -0,0 +1,306 @@ +"""Raw reservation entries are checked before they reach the device (#148). + +``update_reservations`` and ``update_reservations_confirmed`` take raw entry +dicts. They used to send them unchecked, so an unknown mode, an impossible +time or a setpoint far outside the heater's range went out as-is. Entries +are now held to the protocol's field ranges, and ``param`` to the setpoint +range the heater reports in its feature data. +""" + +from typing import Any +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from nwp500.exceptions import ( + DeviceCapabilityError, + ParameterValidationError, + RangeValidationError, +) +from nwp500.models import DeviceFeature +from nwp500.mqtt.control import ( + MqttDeviceController, + validate_reservation_entries, +) +from nwp500.reservations import update_reservations_confirmed + +# The heater's reported setpoint range, in half-degrees Celsius: +# 81 = 40.5 degC (104.9 degF), 131 = 65.5 degC (149.9 degF). +DEVICE_MIN_RAW = 81 +DEVICE_MAX_RAW = 131 + + +def _features() -> DeviceFeature: + return DeviceFeature.model_validate( + { + "temperatureType": 2, + "countryCode": 3, + "modelTypeCode": 513, + "controlTypeCode": 100, + "volumeCode": 1, + "controllerSwVersion": 1, + "panelSwVersion": 1, + "wifiSwVersion": 1, + "controllerSwCode": 1, + "panelSwCode": 1, + "wifiSwCode": 1, + "recircSwVersion": 1, + "recircModelTypeCode": 0, + "controllerSerialNumber": "ABC123", + "dhwTemperatureSettingUse": 2, + "tempFormulaType": 1, + "dhwTemperatureMin": DEVICE_MIN_RAW, + "dhwTemperatureMax": DEVICE_MAX_RAW, + "freezeProtectionTempMin": 12, + "freezeProtectionTempMax": 20, + "recircTemperatureMin": 81, + "recircTemperatureMax": 120, + } + ) + + +def _entry(**overrides: Any) -> dict[str, Any]: + entry: dict[str, Any] = { + "enable": 2, + "week": 84, # Mon, Wed, Fri + "hour": 6, + "min": 30, + "mode": 3, + "param": 120, + } + entry.update(overrides) + return entry + + +@pytest.fixture +def mock_device() -> MagicMock: + device = MagicMock() + device.device_info.mac_address = "aa:bb:cc:dd:ee:ff" + device.device_info.device_type = 52 + device.device_info.additional_value = "additional" + return device + + +def _make_controller( + features: DeviceFeature | None, +) -> tuple[MqttDeviceController, AsyncMock, AsyncMock]: + publish = AsyncMock(return_value=1) + controller = MqttDeviceController( + client_id="test-client", + session_id="test-session", + publish_func=publish, + ) + get_features = AsyncMock(return_value=features) + controller._get_device_features = get_features # type: ignore[method-assign] + return controller, publish, get_features + + +class TestValidateReservationEntries: + def test_valid_entries_pass(self) -> None: + validate_reservation_entries( + [ + _entry(), + _entry(enable=1, week=2, hour=0, min=0, mode=1), + _entry(week=254, hour=23, min=59, mode=6), + ], + _features(), + ) + + def test_empty_list_passes(self) -> None: + validate_reservation_entries([], _features()) + + @pytest.mark.parametrize( + ("field", "value"), + [ + ("enable", 0), + ("enable", 3), + ("enable", True), # would be coerced to 1, i.e. disabled + ("week", 0), # no days + ("week", 1), # bit 0 is unused + ("week", 85), + ("week", 256), + ("hour", 6.0), + ("param", 120.5), + ("mode", "3"), + ], + ) + def test_bad_field_raises_parameter_error( + self, field: str, value: Any + ) -> None: + with pytest.raises(ParameterValidationError) as exc: + validate_reservation_entries([_entry(**{field: value})]) + assert exc.value.parameter == f"reservation[1].{field}" + + def test_missing_field_raises_parameter_error(self) -> None: + entry = _entry() + del entry["param"] + with pytest.raises(ParameterValidationError) as exc: + validate_reservation_entries([entry]) + assert exc.value.parameter == "reservation[1].param" + + @pytest.mark.parametrize( + ("field", "value", "low", "high"), + [ + ("hour", -1, 0, 23), + ("hour", 24, 0, 23), + ("min", -1, 0, 59), + ("min", 60, 0, 59), + ("mode", 0, 1, 6), + ("mode", 7, 1, 6), + ("param", -1, 0, 255), + ("param", 256, 0, 255), + ], + ) + def test_out_of_range_field_raises_range_error( + self, field: str, value: int, low: int, high: int + ) -> None: + with pytest.raises(RangeValidationError) as exc: + validate_reservation_entries([_entry(**{field: value})]) + assert exc.value.field == f"reservation[1].{field}" + assert exc.value.value == value + assert (exc.value.min_value, exc.value.max_value) == (low, high) + + def test_error_names_the_offending_entry(self) -> None: + with pytest.raises(RangeValidationError) as exc: + validate_reservation_entries([_entry(), _entry(hour=99)]) + assert exc.value.field == "reservation[2].hour" + + @pytest.mark.parametrize("param", [DEVICE_MIN_RAW, DEVICE_MAX_RAW]) + def test_param_at_device_limits_passes(self, param: int) -> None: + validate_reservation_entries([_entry(param=param)], _features()) + + @pytest.mark.parametrize( + "param", [0, DEVICE_MIN_RAW - 1, DEVICE_MAX_RAW + 1, 200] + ) + def test_param_outside_device_range_raises(self, param: int) -> None: + with pytest.raises(RangeValidationError) as exc: + validate_reservation_entries([_entry(param=param)], _features()) + assert exc.value.field == "reservation[1].param" + assert exc.value.min_value == DEVICE_MIN_RAW + assert exc.value.max_value == DEVICE_MAX_RAW + + def test_param_without_features_is_held_to_a_byte(self) -> None: + # 200 (100 degC) is outside any heater's range but fits the byte; + # without feature data only the byte range can be checked. + validate_reservation_entries([_entry(param=200)]) + + +class TestControllerUpdateReservations: + @pytest.mark.asyncio + async def test_valid_entries_are_published( + self, mock_device: MagicMock + ) -> None: + controller, publish, get_features = _make_controller(_features()) + entries = [_entry(), _entry(param=DEVICE_MAX_RAW, mode=4)] + + await controller.update_reservations(mock_device, entries) + + get_features.assert_awaited_once_with(mock_device) + publish.assert_awaited_once() + assert publish.await_args is not None + _, command = publish.await_args.args + assert command["request"]["reservation"] == entries + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "overrides", + [ + {"mode": 7}, + {"hour": 24}, + {"min": 60}, + {"enable": True}, + {"week": 0}, + ], + ) + async def test_bad_entry_raises_before_fetching_or_publishing( + self, mock_device: MagicMock, overrides: dict[str, Any] + ) -> None: + controller, publish, get_features = _make_controller(_features()) + + with pytest.raises((ParameterValidationError, RangeValidationError)): + await controller.update_reservations( + mock_device, [_entry(), _entry(**overrides)] + ) + + get_features.assert_not_awaited() + publish.assert_not_awaited() + + @pytest.mark.asyncio + @pytest.mark.parametrize("param", [DEVICE_MIN_RAW - 1, 200]) + async def test_param_outside_device_range_is_not_published( + self, mock_device: MagicMock, param: int + ) -> None: + controller, publish, _ = _make_controller(_features()) + + with pytest.raises(RangeValidationError) as exc: + await controller.update_reservations( + mock_device, [_entry(param=param)] + ) + + assert exc.value.max_value == DEVICE_MAX_RAW + publish.assert_not_awaited() + + @pytest.mark.asyncio + async def test_missing_features_raises( + self, mock_device: MagicMock + ) -> None: + controller, publish, _ = _make_controller(None) + + with pytest.raises(DeviceCapabilityError): + await controller.update_reservations(mock_device, [_entry()]) + + publish.assert_not_awaited() + + @pytest.mark.asyncio + async def test_feature_fetch_timeout_raises_capability_error( + self, mock_device: MagicMock + ) -> None: + """A cache miss whose device-info request times out uses the real + fetch path, which raises RuntimeError; the write reports it as a + capability error, as the capability decorator does.""" + publish = AsyncMock(return_value=1) + controller = MqttDeviceController( + client_id="test-client", + session_id="test-session", + publish_func=publish, + ) + ensure_info = AsyncMock(return_value=False) # feature reply timed out + controller.set_ensure_device_info_callback(ensure_info) + + with pytest.raises(DeviceCapabilityError) as exc: + await controller.update_reservations(mock_device, [_entry()]) + + assert isinstance(exc.value.__cause__, RuntimeError) + ensure_info.assert_awaited_once_with(mock_device) + publish.assert_not_awaited() + + @pytest.mark.asyncio + async def test_empty_schedule_needs_no_features( + self, mock_device: MagicMock + ) -> None: + """Clearing the schedule has no setpoints to check.""" + controller, publish, get_features = _make_controller(None) + + await controller.update_reservations(mock_device, [], enabled=False) + + get_features.assert_not_awaited() + publish.assert_awaited_once() + + +class TestConfirmedUpdate: + @pytest.mark.asyncio + async def test_bad_entry_raises_before_subscribing( + self, mock_device: MagicMock + ) -> None: + mqtt = MagicMock() + mqtt.subscribe_reservation_response = AsyncMock() + mqtt.unsubscribe_reservation_response = AsyncMock() + mqtt.update_reservations = AsyncMock() + + with pytest.raises(RangeValidationError): + await update_reservations_confirmed( + mqtt, mock_device, [_entry(mode=7)] + ) + + mqtt.subscribe_reservation_response.assert_not_awaited() + mqtt.update_reservations.assert_not_awaited()