fix(sws12500): harden credential validation, URL fallback and push timeouts
- Reject blank or whitespace-only PWS credentials in both the config flow and the options flow. INVALID_CREDENTIALS only held placeholder strings, so an empty API ID or key passed validation, the entry was created, and _validate_credentials then rejected every incoming packet. - Fall back to "UNKNOWN" host/port when get_url raises NoURLAvailableError. The values are shown as Ecowitt setup instructions only, so an unresolvable Home Assistant URL must not make the step unreachable. - Catch TimeoutError alongside ClientError in the Windy and Pocasi push paths. A timeout is not a ClientError and used to escape into the webhook handler, answering the station with HTTP 500 even though the measured data was already stored. - Add regression tests for all three and pin that 0-5 battery descriptions really reach the WSLink sensor platform.ecowitt_support
SchiZzA
parent
5adc684558
commit
7c9343f90d
|
|
@ -9,9 +9,9 @@ import voluptuous as vol
|
|||
from yarl import URL
|
||||
|
||||
from homeassistant.config_entries import ConfigEntry, ConfigFlow, ConfigFlowResult, OptionsFlow
|
||||
from homeassistant.core import callback
|
||||
from homeassistant.core import HomeAssistant, callback
|
||||
from homeassistant.helpers import selector
|
||||
from homeassistant.helpers.network import get_url
|
||||
from homeassistant.helpers.network import NoURLAvailableError, get_url
|
||||
|
||||
from .conflicts import ERROR_MUTUALLY_EXCLUSIVE
|
||||
from .const import (
|
||||
|
|
@ -42,6 +42,31 @@ from .const import (
|
|||
_PASSWORD_SELECTOR = selector.TextSelector(selector.TextSelectorConfig(type=selector.TextSelectorType.PASSWORD))
|
||||
|
||||
|
||||
def _is_invalid_credential(value: Any) -> bool:
|
||||
"""Return True when a PWS/WSLink credential is unusable.
|
||||
|
||||
Blank (or whitespace-only) values are as useless as the placeholder strings in
|
||||
`INVALID_CREDENTIALS`: `_validate_credentials` rejects every incoming packet if
|
||||
either option is empty, so the entry would be created but never receive data.
|
||||
"""
|
||||
return not isinstance(value, str) or not value.strip() or value in INVALID_CREDENTIALS
|
||||
|
||||
|
||||
def _ha_url_placeholders(hass: HomeAssistant) -> tuple[str, str]:
|
||||
"""Return (host, port) of the Home Assistant URL for the Ecowitt instructions.
|
||||
|
||||
`get_url` raises `NoURLAvailableError` when HA cannot resolve any of its own
|
||||
URLs. These values are shown as setup instructions only, so fall back to a
|
||||
placeholder instead of letting the config flow abort.
|
||||
"""
|
||||
try:
|
||||
url: URL = URL(get_url(hass))
|
||||
except NoURLAvailableError:
|
||||
return "UNKNOWN", "UNKNOWN"
|
||||
|
||||
return url.host or "UNKNOWN", str(url.port)
|
||||
|
||||
|
||||
class ConfigOptionsFlowHandler(OptionsFlow):
|
||||
"""Handle WeatherStation ConfigFlow."""
|
||||
|
||||
|
|
@ -160,9 +185,9 @@ class ConfigOptionsFlowHandler(OptionsFlow):
|
|||
# legacy one while Ecowitt is active would corrupt those entities.
|
||||
if self.ecowitt.get(ECOWITT_ENABLED):
|
||||
errors["base"] = ERROR_MUTUALLY_EXCLUSIVE
|
||||
elif user_input[API_ID] in INVALID_CREDENTIALS or user_input.get(API_ID, "") == "":
|
||||
elif _is_invalid_credential(user_input.get(API_ID)):
|
||||
errors[API_ID] = "valid_credentials_api"
|
||||
elif user_input[API_KEY] in INVALID_CREDENTIALS or user_input.get(API_KEY, "") == "":
|
||||
elif _is_invalid_credential(user_input.get(API_KEY)):
|
||||
errors[API_KEY] = "valid_credentials_key"
|
||||
elif user_input[API_KEY] == user_input[API_ID]:
|
||||
errors["base"] = "valid_credentials_match"
|
||||
|
|
@ -258,8 +283,7 @@ class ConfigOptionsFlowHandler(OptionsFlow):
|
|||
else:
|
||||
return self.async_create_entry(title=DOMAIN, data=self.retain_data(user_input))
|
||||
|
||||
url: URL = URL(get_url(self.hass))
|
||||
host = url.host or "UNKNOWN"
|
||||
host, port = _ha_url_placeholders(self.hass)
|
||||
|
||||
ecowitt_schema = {
|
||||
vol.Required(
|
||||
|
|
@ -277,7 +301,7 @@ class ConfigOptionsFlowHandler(OptionsFlow):
|
|||
data_schema=vol.Schema(ecowitt_schema),
|
||||
description_placeholders={
|
||||
"url": host,
|
||||
"port": str(url.port),
|
||||
"port": port,
|
||||
"webhook_id": webhook,
|
||||
},
|
||||
errors=errors,
|
||||
|
|
@ -358,9 +382,9 @@ class ConfigFlowHandler(ConfigFlow, domain=DOMAIN):
|
|||
if user_input is None:
|
||||
return self.async_show_form(step_id="pws", data_schema=vol.Schema(self.pws_schema), errors=errors)
|
||||
|
||||
if user_input[API_ID] in INVALID_CREDENTIALS:
|
||||
if _is_invalid_credential(user_input.get(API_ID)):
|
||||
errors[API_ID] = "valid_credentials_api"
|
||||
elif user_input[API_KEY] in INVALID_CREDENTIALS:
|
||||
elif _is_invalid_credential(user_input.get(API_KEY)):
|
||||
errors[API_KEY] = "valid_credentials_key"
|
||||
elif user_input[API_KEY] == user_input[API_ID]:
|
||||
errors["base"] = "valid_credentials_match"
|
||||
|
|
@ -383,8 +407,7 @@ class ConfigFlowHandler(ConfigFlow, domain=DOMAIN):
|
|||
|
||||
if user_input is None:
|
||||
webhook = secrets.token_hex(8)
|
||||
url: URL = URL(get_url(self.hass))
|
||||
host = url.host or "UNKNOWN"
|
||||
host, port = _ha_url_placeholders(self.hass)
|
||||
|
||||
ecowitt_schema = {
|
||||
vol.Required(ECOWITT_WEBHOOK_ID, default=webhook): str,
|
||||
|
|
@ -396,7 +419,7 @@ class ConfigFlowHandler(ConfigFlow, domain=DOMAIN):
|
|||
data_schema=vol.Schema(ecowitt_schema),
|
||||
description_placeholders={
|
||||
"url": host,
|
||||
"port": str(url.port),
|
||||
"port": port,
|
||||
"webhook_id": webhook,
|
||||
},
|
||||
)
|
||||
|
|
|
|||
|
|
@ -180,7 +180,10 @@ class PocasiPush:
|
|||
_LOGGER.critical(POCASI_CZ_UNEXPECTED)
|
||||
await self._disable_pocasi(POCASI_CZ_UNEXPECTED)
|
||||
|
||||
except ClientError as ex:
|
||||
# TimeoutError is not a ClientError: an `async_timeout`/`asyncio` timeout would
|
||||
# otherwise escape into the webhook handler and answer the station with HTTP 500,
|
||||
# even though the measured data was already stored.
|
||||
except (ClientError, TimeoutError) as ex:
|
||||
self.last_status = "client_error"
|
||||
# Store only the exception class - last_error is surfaced via entity
|
||||
# attributes; str(ex) could embed the request URL.
|
||||
|
|
|
|||
|
|
@ -311,7 +311,10 @@ class WindyPush:
|
|||
reason=f"Unable to send data to Windy ({WINDY_MAX_RETRIES} times). Disabling resend option for now. Please check your Windy configuration and enable this feature afterwards."
|
||||
)
|
||||
|
||||
except ClientError as ex:
|
||||
# TimeoutError is not a ClientError: an `async_timeout`/`asyncio` timeout would
|
||||
# otherwise escape into the webhook handler and answer the station with HTTP 500,
|
||||
# even though the measured data was already stored.
|
||||
except (ClientError, TimeoutError) as ex:
|
||||
self.last_status = "client_error"
|
||||
# Store only the exception class - last_error is surfaced via entity
|
||||
# attributes; str(ex) could embed the request URL.
|
||||
|
|
|
|||
|
|
@ -81,13 +81,35 @@ def test_level_descriptions_generated_from_battery_non_binary() -> None:
|
|||
|
||||
|
||||
def test_level_batteries_are_not_also_plain_sensors() -> None:
|
||||
"""A 0-5 battery must appear exactly once in each platform's description set."""
|
||||
"""A 0-5 battery must not be hand-defined on top of its generated description.
|
||||
|
||||
`BATTERY_LEVEL_SENSORS` is spliced into `SENSOR_TYPES_WSLINK`, so one occurrence
|
||||
there is the generated one and is expected; `SENSOR_TYPES_WEATHER_API` does not
|
||||
splice them in, so zero is expected there. "At most once" is therefore the
|
||||
invariant that holds for both - a second entry is a duplicate entity.
|
||||
|
||||
Complete *absence* of a description is caught by
|
||||
`test_level_descriptions_generated_from_battery_non_binary`, which pins the
|
||||
generated set to `BATTERY_NON_BINARY` exactly.
|
||||
"""
|
||||
for sensor_types in (SENSOR_TYPES_WSLINK, SENSOR_TYPES_WEATHER_API):
|
||||
keys = [desc.key for desc in sensor_types]
|
||||
for key in BATTERY_NON_BINARY:
|
||||
assert keys.count(key) <= 1, f"{key} defined more than once"
|
||||
|
||||
|
||||
def test_level_batteries_reach_the_sensor_platform() -> None:
|
||||
"""Every 0-5 battery must actually be spliced into the WSLink platform set.
|
||||
|
||||
The generated tuple existing is not enough - if the `*BATTERY_LEVEL_SENSORS`
|
||||
splice were dropped, the classification tests would still pass while the
|
||||
percentage entities silently disappeared.
|
||||
"""
|
||||
keys = [desc.key for desc in SENSOR_TYPES_WSLINK]
|
||||
for key in BATTERY_NON_BINARY:
|
||||
assert key in keys, f"{key} is classified as a 0-5 battery but has no sensor description"
|
||||
|
||||
|
||||
def test_binary_batteries_are_not_plain_sensors_too() -> None:
|
||||
"""0/1 batteries belong to the binary platform only.
|
||||
|
||||
|
|
|
|||
|
|
@ -29,6 +29,7 @@ from custom_components.sws12500.const import (
|
|||
WSLINK_ADDON_PORT,
|
||||
)
|
||||
from homeassistant import config_entries
|
||||
from homeassistant.helpers.network import NoURLAvailableError
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -127,6 +128,81 @@ async def test_config_flow_user_invalid_credentials_api_key(
|
|||
assert result2["errors"][API_KEY] == "valid_credentials_key"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("blank", ["", " "])
|
||||
@pytest.mark.parametrize("field", [API_ID, API_KEY])
|
||||
@pytest.mark.asyncio
|
||||
async def test_config_flow_user_rejects_blank_credentials(
|
||||
hass, enable_custom_integrations, field, blank
|
||||
) -> None:
|
||||
"""Blank PWS credentials must not create an entry.
|
||||
|
||||
`INVALID_CREDENTIALS` holds placeholder strings only, so an empty (or
|
||||
whitespace-only) value used to pass validation. The entry was created but
|
||||
`_validate_credentials` then rejected every incoming packet, leaving the
|
||||
integration permanently without data.
|
||||
"""
|
||||
result = await hass.config_entries.flow.async_init(
|
||||
DOMAIN, context={"source": config_entries.SOURCE_USER}
|
||||
)
|
||||
form = await hass.config_entries.flow.async_configure(
|
||||
result["flow_id"], user_input={"next_step_id": "pws"}
|
||||
)
|
||||
assert form["step_id"] == "pws"
|
||||
|
||||
user_input = {
|
||||
API_ID: "ok_id",
|
||||
API_KEY: "ok_key",
|
||||
WSLINK: False,
|
||||
DEV_DBG: False,
|
||||
}
|
||||
user_input[field] = blank
|
||||
|
||||
result2 = await hass.config_entries.flow.async_configure(
|
||||
form["flow_id"], user_input=user_input
|
||||
)
|
||||
assert result2["type"] == "form"
|
||||
assert result2["step_id"] == "pws"
|
||||
expected = "valid_credentials_api" if field is API_ID else "valid_credentials_key"
|
||||
assert result2["errors"][field] == expected
|
||||
|
||||
|
||||
@pytest.mark.parametrize("blank", ["", " "])
|
||||
@pytest.mark.parametrize("field", [API_ID, API_KEY])
|
||||
@pytest.mark.asyncio
|
||||
async def test_options_flow_basic_rejects_blank_credentials(
|
||||
hass, enable_custom_integrations, field, blank
|
||||
) -> None:
|
||||
"""Same blank-credential rule applies when the legacy endpoint is re-enabled."""
|
||||
entry = MockConfigEntry(
|
||||
domain=DOMAIN,
|
||||
data={},
|
||||
options={LEGACY_ENABLED: False, ECOWITT_ENABLED: False},
|
||||
)
|
||||
entry.add_to_hass(hass)
|
||||
|
||||
init = await hass.config_entries.options.async_init(entry.entry_id)
|
||||
form = await hass.config_entries.options.async_configure(
|
||||
init["flow_id"], user_input={"next_step_id": "basic"}
|
||||
)
|
||||
assert form["step_id"] == "basic"
|
||||
|
||||
user_input = {
|
||||
API_ID: "ok_id",
|
||||
API_KEY: "ok_key",
|
||||
WSLINK: False,
|
||||
DEV_DBG: False,
|
||||
LEGACY_ENABLED: True,
|
||||
}
|
||||
user_input[field] = blank
|
||||
|
||||
result = await hass.config_entries.options.async_configure(
|
||||
init["flow_id"], user_input=user_input
|
||||
)
|
||||
assert result["type"] == "form"
|
||||
expected = "valid_credentials_api" if field is API_ID else "valid_credentials_key"
|
||||
assert result["errors"][field] == expected
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_config_flow_user_invalid_credentials_match(
|
||||
hass, enable_custom_integrations
|
||||
|
|
@ -410,6 +486,70 @@ async def test_options_flow_ecowitt_uses_get_url_placeholders_and_webhook_defaul
|
|||
assert done["data"][ECOWITT_ENABLED] is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_options_flow_ecowitt_survives_no_url_available(
|
||||
hass, enable_custom_integrations
|
||||
) -> None:
|
||||
"""`get_url` raising must not abort the Ecowitt options step.
|
||||
|
||||
The host/port are shown as setup instructions only. HA can fail to resolve any
|
||||
of its own URLs (no internal/external URL configured), and letting that escape
|
||||
made the Ecowitt setup unreachable.
|
||||
"""
|
||||
entry = MockConfigEntry(
|
||||
domain=DOMAIN,
|
||||
data={},
|
||||
options={ECOWITT_WEBHOOK_ID: "", ECOWITT_ENABLED: False, LEGACY_ENABLED: False},
|
||||
)
|
||||
entry.add_to_hass(hass)
|
||||
|
||||
init = await hass.config_entries.options.async_init(entry.entry_id)
|
||||
with patch(
|
||||
"custom_components.sws12500.config_flow.get_url",
|
||||
side_effect=NoURLAvailableError,
|
||||
):
|
||||
form = await hass.config_entries.options.async_configure(
|
||||
init["flow_id"], user_input={"next_step_id": "ecowitt"}
|
||||
)
|
||||
assert form["type"] == "form"
|
||||
placeholders = form.get("description_placeholders") or {}
|
||||
assert placeholders["url"] == "UNKNOWN"
|
||||
assert placeholders["port"] == "UNKNOWN"
|
||||
assert placeholders["webhook_id"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_config_flow_ecowitt_survives_no_url_available(
|
||||
hass, enable_custom_integrations
|
||||
) -> None:
|
||||
"""The initial Ecowitt step stays usable when HA cannot resolve its own URL."""
|
||||
result = await hass.config_entries.flow.async_init(
|
||||
DOMAIN, context={"source": config_entries.SOURCE_USER}
|
||||
)
|
||||
with patch(
|
||||
"custom_components.sws12500.config_flow.get_url",
|
||||
side_effect=NoURLAvailableError,
|
||||
):
|
||||
form = await hass.config_entries.flow.async_configure(
|
||||
result["flow_id"], user_input={"next_step_id": "ecowitt"}
|
||||
)
|
||||
assert form["type"] == "form"
|
||||
placeholders = form.get("description_placeholders") or {}
|
||||
assert placeholders["url"] == "UNKNOWN"
|
||||
assert placeholders["port"] == "UNKNOWN"
|
||||
|
||||
# The step is still completable without a resolvable URL.
|
||||
done = await hass.config_entries.flow.async_configure(
|
||||
form["flow_id"],
|
||||
user_input={
|
||||
ECOWITT_WEBHOOK_ID: placeholders["webhook_id"],
|
||||
ECOWITT_ENABLED: True,
|
||||
},
|
||||
)
|
||||
assert done["type"] == "create_entry"
|
||||
assert done["data"][ECOWITT_ENABLED] is True
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_options_flow_wslink_port_setup(hass, enable_custom_integrations) -> None:
|
||||
"""The WSLink add-on port step shows a form and stores the port."""
|
||||
|
|
|
|||
|
|
@ -338,6 +338,40 @@ async def test_push_data_to_server_client_error_increments_and_disables_after_th
|
|||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_push_data_to_server_timeout_is_handled_like_client_error(
|
||||
monkeypatch, hass
|
||||
):
|
||||
"""A network timeout must not escape into the webhook handler.
|
||||
|
||||
`TimeoutError` is not a subclass of `ClientError`, so it used to propagate out
|
||||
of `push_data_to_server`, through the awaiting coordinator, and answer the
|
||||
station with HTTP 500 - even though the measured data was already stored.
|
||||
"""
|
||||
entry = _make_entry()
|
||||
pp = PocasiPush(hass, entry)
|
||||
|
||||
monkeypatch.setattr(
|
||||
"custom_components.sws12500.pocasti_cz.update_options",
|
||||
_write_through_update_options(entry),
|
||||
)
|
||||
monkeypatch.setattr("custom_components.sws12500.pocasti_cz._LOGGER.critical", MagicMock())
|
||||
|
||||
session = _FakeSession(exc=TimeoutError("timed out"))
|
||||
monkeypatch.setattr(
|
||||
"custom_components.sws12500.pocasti_cz.async_get_clientsession",
|
||||
lambda _h: session,
|
||||
)
|
||||
|
||||
pp.next_update = dt_util.utcnow() - timedelta(seconds=1)
|
||||
await pp.push_data_to_server({"x": 1}, "WU")
|
||||
|
||||
assert pp.last_status == "client_error"
|
||||
assert pp.last_error == "TimeoutError"
|
||||
assert pp.invalid_response_count == 1
|
||||
assert pp.enabled is True
|
||||
|
||||
|
||||
def test_verify_response_logs_debug_when_logger_enabled(monkeypatch, hass):
|
||||
entry = _make_entry(logger=True)
|
||||
pp = PocasiPush(hass, entry)
|
||||
|
|
|
|||
|
|
@ -449,6 +449,43 @@ async def test_push_data_to_windy_client_error_increments_and_disables_after_thr
|
|||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_push_data_to_windy_timeout_is_handled_like_client_error(
|
||||
monkeypatch, hass
|
||||
):
|
||||
"""A network timeout must not escape into the webhook handler.
|
||||
|
||||
`TimeoutError` is not a subclass of `ClientError`, so it used to propagate out
|
||||
of `push_data_to_windy`, through the awaiting coordinator, and answer the
|
||||
station with HTTP 500 - even though the measured data was already stored.
|
||||
"""
|
||||
entry = _make_entry()
|
||||
wp = WindyPush(hass, entry)
|
||||
wp.next_update = dt_util.utcnow() - timedelta(seconds=1)
|
||||
|
||||
monkeypatch.setattr(
|
||||
"custom_components.sws12500.windy_func.update_options", AsyncMock(return_value=True)
|
||||
)
|
||||
monkeypatch.setattr("custom_components.sws12500.windy_func._LOGGER.critical", MagicMock())
|
||||
monkeypatch.setattr(
|
||||
"custom_components.sws12500.windy_func.persistent_notification.async_create",
|
||||
MagicMock(),
|
||||
)
|
||||
|
||||
session = _FakeSession(exc=TimeoutError("timed out"))
|
||||
monkeypatch.setattr(
|
||||
"custom_components.sws12500.windy_func.async_get_clientsession",
|
||||
lambda _h: session,
|
||||
)
|
||||
|
||||
ok = await wp.push_data_to_windy({"a": "b"})
|
||||
|
||||
assert ok is True
|
||||
assert wp.last_status == "client_error"
|
||||
assert wp.last_error == "TimeoutError"
|
||||
assert wp.invalid_response_count == 1
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_push_data_to_windy_client_error_disable_failure_logs_debug(
|
||||
monkeypatch, hass
|
||||
|
|
|
|||
Loading…
Reference in New Issue