From 36e070294b4b864f2d9edb6a948f07276032b64b Mon Sep 17 00:00:00 2001 From: emmanuel Date: Sat, 4 Jul 2026 22:28:35 -0700 Subject: [PATCH 1/2] fix: address high-priority evaluation findings (hotfix batch) - Fix AttributeError in configure_reservation_water_program proxy (referenced nonexistent self._control instead of _device_controller) - Remove broken 'vacation' and 'standby' choices from the CLI mode command; vacation requires a day count (use 'vacation DAYS') and standby (0) is not a writable operation mode (use 'power off') - Propagate CLI failures as non-zero exit codes (click standalone mode ignores callback return values) - Discard cached tokens when --email differs from the cached account - Write ~/.nwp500_tokens.json with 0600 permissions and tighten existing files on save - Raise APIError for OpenEI application errors returned in HTTP 200 bodies instead of reporting 'no rate plans found' - Fix broken import and deprecated datetime.utcnow() in examples/advanced/mqtt_diagnostics.py - Update CLI mode docs to match the supported choices --- CHANGELOG.rst | 34 +++++++++++ docs/reference/python_api/cli.rst | 13 ++--- examples/advanced/mqtt_diagnostics.py | 5 +- src/nwp500/cli/__main__.py | 10 +++- src/nwp500/cli/handlers.py | 4 +- src/nwp500/cli/token_storage.py | 7 ++- src/nwp500/mqtt/client.py | 3 +- src/nwp500/openei.py | 12 ++++ tests/test_cli_basic.py | 84 +++++++++++++++++++++++++++ tests/test_cli_commands.py | 15 +++++ tests/test_mqtt_client_init.py | 35 +++++++++++ tests/test_openei.py | 23 ++++++++ tests/test_token_storage.py | 61 +++++++++++++++++++ 13 files changed, 287 insertions(+), 19 deletions(-) create mode 100644 tests/test_token_storage.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 89741ba9..f45e57ef 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,40 @@ Changelog Unreleased ========== +Bug Fixes +--------- +- **Fix AttributeError in configure_reservation_water_program**: The + ``NavienMqttClient`` proxy referenced ``self._control``, which is never + assigned (the attribute is ``_device_controller``), so every call raised + ``AttributeError``. Now delegates correctly; a regression test guards all + proxies against references to the undefined attribute. +- **Fix broken CLI mode choices**: ``nwp-cli mode vacation`` always failed + because vacation mode (5) requires a day count that was never supplied, and + ``mode standby`` sent the invalid writable mode value ``0``. Both choices + were removed from the ``mode`` command; use the dedicated ``vacation DAYS`` + and ``power off`` commands instead. +- **Fix CLI exit codes**: click ignores command return values in standalone + mode, so the CLI always exited ``0`` even when a command failed. Failures + now propagate through ``ctx.exit()`` and produce a non-zero exit code for + scripts and automation. +- **Fix cached tokens being reused for a different account**: passing + ``--email`` for account B while tokens for account A were cached silently + ran commands against account A's session. Cached tokens are now discarded + when the provided email does not match the cached one. +- **Surface OpenEI application errors**: OpenEI reports errors such as an + invalid API key in the body of an HTTP 200 response; these were masked as + "no rate plans found". ``fetch_rates()`` now raises ``APIError`` with the + API's error message. +- **Fix broken example import**: ``examples/advanced/mqtt_diagnostics.py`` + imported ``MqttConnectionConfig`` from the nonexistent ``nwp500.mqtt_utils`` + module and used the deprecated ``datetime.utcnow()``. + +Security +-------- +- **Restrict token cache file permissions**: ``~/.nwp500_tokens.json`` + (refresh token and AWS credentials) was written world-readable. It is now + created with mode ``0600``, and existing files are tightened on save. + Version 8.1.3 (2026-06-15) ========================== diff --git a/docs/reference/python_api/cli.rst b/docs/reference/python_api/cli.rst index 9464aa8f..282a3ebc 100644 --- a/docs/reference/python_api/cli.rst +++ b/docs/reference/python_api/cli.rst @@ -224,12 +224,6 @@ Set operation mode. # High Demand (maximum capacity) python3 -m nwp500.cli mode high-demand - # Vacation Mode - python3 -m nwp500.cli mode vacation - - # Standby - python3 -m nwp500.cli mode standby - **Syntax:** .. code-block:: bash @@ -238,12 +232,13 @@ Set operation mode. **Available Modes:** -* ``standby`` - Device off but ready -* ``heat-pump`` - Heat pump only (0) +* ``heat-pump`` - Heat pump only (1) * ``electric`` - Electric heating only (2) * ``energy-saver`` - Hybrid/balanced mode (3) **recommended** * ``high-demand`` - Maximum heating capacity (4) -* ``vacation`` - Extended vacancy mode (5) + +For vacation mode use the ``vacation`` command (it requires a day count); +to power the unit off use the ``power`` command. **Output:** Confirmation message and updated device status. diff --git a/examples/advanced/mqtt_diagnostics.py b/examples/advanced/mqtt_diagnostics.py index b2d634ed..5a96d8b9 100755 --- a/examples/advanced/mqtt_diagnostics.py +++ b/examples/advanced/mqtt_diagnostics.py @@ -27,8 +27,7 @@ from pathlib import Path from nwp500 import NavienAuthClient, NavienMqttClient -from nwp500.mqtt import MqttDiagnosticsCollector -from nwp500.mqtt_utils import MqttConnectionConfig +from nwp500.mqtt import MqttConnectionConfig, MqttDiagnosticsCollector # Configure logging to show detailed MQTT information logging.basicConfig( @@ -275,7 +274,7 @@ async def run_example( # Final export _logger.info("Exporting final diagnostics...") - timestamp = datetime.utcnow().strftime("%Y%m%d_%H%M%S") + timestamp = datetime.now(UTC).strftime("%Y%m%d_%H%M%S") final_file = self.output_dir / f"diagnostics_final_{timestamp}.json" with open(final_file, "w") as f: f.write(self.diagnostics.export_json()) diff --git a/src/nwp500/cli/__main__.py b/src/nwp500/cli/__main__.py index 3d20691f..21c584ba 100644 --- a/src/nwp500/cli/__main__.py +++ b/src/nwp500/cli/__main__.py @@ -88,6 +88,10 @@ async def runner() -> int: # Load cached tokens if available tokens, cached_email = load_tokens() + # Cached tokens belong to cached_email; never reuse them for a + # different account. + if email and cached_email and email != cached_email: + tokens = None # If email not provided in args, try cached email email = email or cached_email @@ -162,7 +166,9 @@ async def runner() -> int: _formatter.print_error(str(e), title="Unexpected Error") return 1 - return asyncio.run(runner()) + # click ignores callback return values in standalone mode, so + # propagate failures via ctx.exit to get a non-zero exit code. + ctx.exit(asyncio.run(runner())) return wrapper @@ -288,12 +294,10 @@ async def power(mqtt: NavienMqttClient, device: Any, state: str) -> None: "mode_name", type=click.Choice( [ - "standby", "heat-pump", "electric", "energy-saver", "high-demand", - "vacation", ], case_sensitive=False, ), diff --git a/src/nwp500/cli/handlers.py b/src/nwp500/cli/handlers.py index c31f65f9..c94b0449 100644 --- a/src/nwp500/cli/handlers.py +++ b/src/nwp500/cli/handlers.py @@ -220,13 +220,13 @@ async def handle_set_mode_request( mqtt: NavienMqttClient, device: Device, mode_name: str ) -> None: """Set device operation mode.""" + # Vacation mode (5) requires a day count and has its own `vacation` + # command; power-off (6) is handled by the `power` command. mode_mapping = { - "standby": 0, "heat-pump": 1, "electric": 2, "energy-saver": 3, "high-demand": 4, - "vacation": 5, } mode_id = mode_mapping.get(mode_name.lower()) if mode_id is None: diff --git a/src/nwp500/cli/token_storage.py b/src/nwp500/cli/token_storage.py index 4b5b7633..6bb2a489 100644 --- a/src/nwp500/cli/token_storage.py +++ b/src/nwp500/cli/token_storage.py @@ -4,6 +4,7 @@ import json import logging +import os from pathlib import Path from nwp500.auth import AuthTokens @@ -22,7 +23,11 @@ def save_tokens(tokens: AuthTokens, email: str) -> None: email: User email address """ try: - with open(TOKEN_FILE, "w") as f: + # Tokens grant account access; keep the file owner-readable only. + # O_CREAT mode only applies to new files, so chmod existing ones. + fd = os.open(TOKEN_FILE, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "w") as f: + TOKEN_FILE.chmod(0o600) # Use the built-in to_dict() method for serialization token_data = tokens.to_dict() token_data["email"] = email diff --git a/src/nwp500/mqtt/client.py b/src/nwp500/mqtt/client.py index 13fa05e4..72bc1bcf 100644 --- a/src/nwp500/mqtt/client.py +++ b/src/nwp500/mqtt/client.py @@ -1268,7 +1268,8 @@ async def update_weekly_reservation( async def configure_reservation_water_program(self, device: Device) -> int: """Enable/configure water program reservation mode.""" - return await self._control.configure_reservation_water_program(device) + controller = self._device_controller + return await controller.configure_reservation_water_program(device) async def configure_recirculation_schedule( self, device: Device, schedule: RecirculationSchedule diff --git a/src/nwp500/openei.py b/src/nwp500/openei.py index f556c37f..39e95bde 100644 --- a/src/nwp500/openei.py +++ b/src/nwp500/openei.py @@ -16,6 +16,8 @@ import aiohttp +from .exceptions import APIError + __author__ = "Emmanuel Levijarvi" __copyright__ = "Emmanuel Levijarvi" __license__ = "MIT" @@ -120,6 +122,16 @@ async def fetch_rates( async with self._session.get(OPENEI_API_URL, params=params) as resp: resp.raise_for_status() data: dict[str, Any] = await resp.json() + # OpenEI reports application errors (e.g. invalid API key) in + # the body of an HTTP 200 response. + if "error" in data: + error = data["error"] + message = ( + error.get("message", str(error)) + if isinstance(error, dict) + else str(error) + ) + raise APIError(f"OpenEI API error: {message}", response=data) items: list[dict[str, Any]] = data.get("items", []) _logger.info( "Retrieved %d rate plans for zip %s", diff --git a/tests/test_cli_basic.py b/tests/test_cli_basic.py index 62216433..773e31f2 100644 --- a/tests/test_cli_basic.py +++ b/tests/test_cli_basic.py @@ -1,5 +1,6 @@ """Basic tests for CLI entry point.""" +import logging import sys from unittest.mock import patch @@ -11,6 +12,16 @@ pytest.skip("CLI dependencies not installed", allow_module_level=True) +@pytest.fixture(autouse=True) +def _restore_log_levels(): + """The CLI group callback mutates global logger levels; undo it.""" + names = (None, "nwp500", "nwp500.cli.__main__", "aiohttp") + saved = {name: logging.getLogger(name).level for name in names} + yield + for name, level in saved.items(): + logging.getLogger(name).setLevel(level) + + def test_cli_help(): """Test that CLI help command works.""" with patch.object(sys, "argv", ["nwp-cli", "--help"]): @@ -25,3 +36,76 @@ def test_cli_no_args(): with pytest.raises(SystemExit) as excinfo: run() assert excinfo.value.code != 0 + + +def test_cli_mode_rejects_vacation(): + """Vacation is not a valid mode argument (use the vacation command).""" + with patch.object(sys, "argv", ["nwp-cli", "mode", "vacation"]): + with pytest.raises(SystemExit) as excinfo: + run() + assert excinfo.value.code == 2 # click usage error + + +def test_cli_mode_rejects_standby(): + """Standby (0) is not a writable operation mode.""" + with patch.object(sys, "argv", ["nwp-cli", "mode", "standby"]): + with pytest.raises(SystemExit) as excinfo: + run() + assert excinfo.value.code == 2 # click usage error + + +def test_cli_exits_nonzero_on_missing_credentials(): + """Command failures must produce a non-zero exit code.""" + import os + + env = { + k: v + for k, v in os.environ.items() + if k not in ("NAVIEN_EMAIL", "NAVIEN_PASSWORD") + } + with ( + patch.dict(os.environ, env, clear=True), + patch("nwp500.cli.__main__.load_tokens", return_value=(None, None)), + patch.object(sys, "argv", ["nwp-cli", "mode", "heat-pump"]), + ): + with pytest.raises(SystemExit) as excinfo: + run() + assert excinfo.value.code == 1 + + +def test_cli_discards_cached_tokens_for_different_email(): + """Cached tokens for account A must not be reused for account B.""" + from unittest.mock import MagicMock + + from nwp500.exceptions import AuthenticationError + + cached_tokens = MagicMock() + mock_auth_cls = MagicMock( + side_effect=AuthenticationError("stop before network") + ) + with ( + patch( + "nwp500.cli.__main__.load_tokens", + return_value=(cached_tokens, "usera@example.com"), + ), + patch("nwp500.cli.__main__.NavienAuthClient", mock_auth_cls), + patch.object( + sys, + "argv", + [ + "nwp-cli", + "--email", + "userb@example.com", + "--password", + "pw", + "mode", + "heat-pump", + ], + ), + ): + with pytest.raises(SystemExit) as excinfo: + run() + assert excinfo.value.code == 1 + mock_auth_cls.assert_called_once_with( + "userb@example.com", "pw", stored_tokens=None + ) diff --git a/tests/test_cli_commands.py b/tests/test_cli_commands.py index e3887c2f..2e1826c9 100644 --- a/tests/test_cli_commands.py +++ b/tests/test_cli_commands.py @@ -131,6 +131,21 @@ async def test_handle_set_mode_request_invalid_mode(mock_mqtt, mock_device): mock_mqtt.set_dhw_mode.assert_not_called() +@pytest.mark.asyncio +@pytest.mark.parametrize("mode_name", ["standby", "vacation"]) +async def test_handle_set_mode_request_unsupported_modes( + mock_mqtt, mock_device, mode_name +): + """Standby/vacation are not settable via the mode command. + + Vacation requires a day count (dedicated ``vacation`` command) and + standby (0) is not a writable DhwOperationSetting value. + """ + await handle_set_mode_request(mock_mqtt, mock_device, mode_name) + + mock_mqtt.set_dhw_mode.assert_not_called() + + @pytest.mark.asyncio async def test_handle_set_dhw_temp_request_success(mock_mqtt, mock_device): """Test successful temperature setting.""" diff --git a/tests/test_mqtt_client_init.py b/tests/test_mqtt_client_init.py index 2ecc21f7..f3c0759c 100644 --- a/tests/test_mqtt_client_init.py +++ b/tests/test_mqtt_client_init.py @@ -830,3 +830,38 @@ async def mock_connect(): # Verify call order assert call_order == ["ensure_valid_token", "connect"] assert result is True + + +class TestDeviceControllerProxies: + """Regression tests for client -> device-controller delegation.""" + + @pytest.mark.asyncio + async def test_configure_reservation_water_program_delegates( + self, auth_client_with_valid_tokens + ): + """Regression: this proxy referenced a nonexistent self._control.""" + from unittest.mock import AsyncMock, MagicMock + + mqtt_client = NavienMqttClient(auth_client_with_valid_tokens) + mqtt_client._device_controller = AsyncMock() + mqtt_client._device_controller.configure_reservation_water_program = ( + AsyncMock(return_value=1) + ) + device = MagicMock() + + result = await mqtt_client.configure_reservation_water_program(device) + + assert result == 1 + proxy_target = mqtt_client._device_controller + proxy_target.configure_reservation_water_program.assert_awaited_once_with( + device + ) + + def test_no_references_to_undefined_control_attribute(self): + """No proxy may reference self._control (never assigned).""" + import inspect + + import nwp500.mqtt.client as client_module + + source = inspect.getsource(client_module) + assert "self._control." not in source diff --git a/tests/test_openei.py b/tests/test_openei.py index c68a2e47..1dddbd92 100644 --- a/tests/test_openei.py +++ b/tests/test_openei.py @@ -4,6 +4,7 @@ import pytest +from nwp500.exceptions import APIError from nwp500.openei import OpenEIClient # Sample OpenEI API response items (realistic data from HAR captures) @@ -182,6 +183,28 @@ async def test_get_rate_plan_not_found() -> None: assert plan is None +@pytest.mark.asyncio +async def test_error_body_raises_api_error() -> None: + """OpenEI reports errors in an HTTP 200 body; they must not be masked.""" + mock_resp = MagicMock() + mock_resp.raise_for_status = MagicMock() + mock_resp.json = AsyncMock( + return_value={"error": {"message": "The API_KEY provided is invalid"}} + ) + mock_resp.__aenter__ = AsyncMock(return_value=mock_resp) + mock_resp.__aexit__ = AsyncMock(return_value=False) + + mock_session = MagicMock() + mock_session.get = MagicMock(return_value=mock_resp) + + client = OpenEIClient(api_key="bad-key") + client._session = mock_session + client._owned_session = False + + with pytest.raises(APIError, match="API_KEY provided is invalid"): + await client.fetch_rates("94903") + + @pytest.mark.asyncio async def test_no_api_key_raises() -> None: """Test that missing API key raises ValueError.""" diff --git a/tests/test_token_storage.py b/tests/test_token_storage.py new file mode 100644 index 00000000..43b7b205 --- /dev/null +++ b/tests/test_token_storage.py @@ -0,0 +1,61 @@ +"""Tests for CLI token storage.""" + +import json + +import pytest + +try: + from nwp500.cli import token_storage +except ImportError: + pytest.skip("CLI dependencies not installed", allow_module_level=True) + +from nwp500.auth import AuthTokens + + +@pytest.fixture +def tokens(): + return AuthTokens( + id_token="test_id", + access_token="test_access", + refresh_token="test_refresh", + authentication_expires_in=3600, + ) + + +def test_save_tokens_owner_only_permissions(tmp_path, monkeypatch, tokens): + """Token file must not be readable by group/others.""" + token_file = tmp_path / "tokens.json" + monkeypatch.setattr(token_storage, "TOKEN_FILE", token_file) + + token_storage.save_tokens(tokens, "user@example.com") + + assert token_file.exists() + assert token_file.stat().st_mode & 0o777 == 0o600 + + +def test_save_tokens_tightens_existing_permissions( + tmp_path, monkeypatch, tokens +): + """Saving over a pre-existing world-readable file fixes its mode.""" + token_file = tmp_path / "tokens.json" + token_file.write_text("{}") + token_file.chmod(0o644) + monkeypatch.setattr(token_storage, "TOKEN_FILE", token_file) + + token_storage.save_tokens(tokens, "user@example.com") + + assert token_file.stat().st_mode & 0o777 == 0o600 + + +def test_save_and_load_roundtrip(tmp_path, monkeypatch, tokens): + token_file = tmp_path / "tokens.json" + monkeypatch.setattr(token_storage, "TOKEN_FILE", token_file) + + token_storage.save_tokens(tokens, "user@example.com") + loaded, email = token_storage.load_tokens() + + assert email == "user@example.com" + assert loaded is not None + assert loaded.refresh_token == "test_refresh" + # File content is valid JSON including the email + assert json.loads(token_file.read_text())["email"] == "user@example.com" From 30ec4e45b8eae53473b2155b05388cb1b969cd6c Mon Sep 17 00:00:00 2001 From: emmanuel Date: Sun, 5 Jul 2026 09:26:48 -0700 Subject: [PATCH 2/2] test: use AST-based check for undefined _control attribute references Addresses review feedback: a raw substring search could false-fail on comments, docstrings, or examples. The AST walk only flags actual self._control attribute access. --- tests/test_mqtt_client_init.py | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/test_mqtt_client_init.py b/tests/test_mqtt_client_init.py index f3c0759c..f92a6119 100644 --- a/tests/test_mqtt_client_init.py +++ b/tests/test_mqtt_client_init.py @@ -858,10 +858,24 @@ async def test_configure_reservation_water_program_delegates( ) def test_no_references_to_undefined_control_attribute(self): - """No proxy may reference self._control (never assigned).""" + """No code may reference self._control (never assigned). + + Uses an AST walk rather than a source substring search so that + mentions in comments, docstrings, or examples cannot cause + false failures — only actual attribute access counts. + """ + import ast import inspect import nwp500.mqtt.client as client_module - source = inspect.getsource(client_module) - assert "self._control." not in source + tree = ast.parse(inspect.getsource(client_module)) + offenders = [ + node.lineno + for node in ast.walk(tree) + if isinstance(node, ast.Attribute) + and node.attr == "_control" + and isinstance(node.value, ast.Name) + and node.value.id == "self" + ] + assert offenders == []