From 57cf1cdd118d1f95c3f26a69f8680be4b6449185 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Tue, 30 Jun 2026 14:56:36 -0500 Subject: [PATCH] fix: address code review findings from post-onboarding audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add button platform (button.py + PLATFORMS) so oven/washer Start/Pause/Stop controls actually appear in HA - Load DTLS cert chain from memory via _load_pem_chain(); eliminate all temp file writes for key material in config_flow and coordinator - Fix PEM boundary: ensure leaf and CA blocks are separated by a newline in the stored fullchain regardless of whether the user pasted a trailing newline - Dryer mode select: add static options tuple and value_fn to decode Course_XX hex strings back to human names; was permanently empty before - Remove callable branch from LocalThingsSelect (options_field covers dynamic case; callable contract was broken — called with no args vs Callable[[dict]]) - Guard native_min/max_value overrides with hasattr + super() fallback to prevent AttributeError when native_min=None and range_field returns nothing - Add async_close() public method to coordinator; __init__ now calls it instead of reaching in to _close_session directly - Update tests: rename CONF_CERT_PEM/KEY_PEM → CA/LEAF variants, add leaf_cert_pem/leaf_key_pem to mock_probe return, fix mock PEM constants (no trailing newlines, matching what .strip() stores) --- custom_components/localthings/__init__.py | 2 +- custom_components/localthings/button.py | 36 ++++++ custom_components/localthings/config_flow.py | 106 ++++++++---------- custom_components/localthings/const.py | 2 +- custom_components/localthings/coordinator.py | 44 +++----- custom_components/localthings/number.py | 8 +- .../localthings/ocf/coap_dtls.py | 37 +++++- .../ocf/registry/capabilities/dryer.py | 6 +- custom_components/localthings/select.py | 3 +- tests/localthings/conftest.py | 26 +++-- tests/localthings/test_config_flow.py | 26 ++--- 11 files changed, 176 insertions(+), 120 deletions(-) create mode 100644 custom_components/localthings/button.py diff --git a/custom_components/localthings/__init__.py b/custom_components/localthings/__init__.py index dfbc4c3..1d35120 100644 --- a/custom_components/localthings/__init__.py +++ b/custom_components/localthings/__init__.py @@ -29,5 +29,5 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: unloaded = await hass.config_entries.async_unload_platforms(entry, PLATFORMS) if unloaded: coordinator: LocalThingsCoordinator = hass.data[DOMAIN].pop(entry.entry_id) - await hass.async_add_executor_job(coordinator._close_session) + await coordinator.async_close() return unloaded diff --git a/custom_components/localthings/button.py b/custom_components/localthings/button.py new file mode 100644 index 0000000..c95dfd4 --- /dev/null +++ b/custom_components/localthings/button.py @@ -0,0 +1,36 @@ +"""Button platform for Local Things.""" +from __future__ import annotations + +from homeassistant.components.button import ButtonEntity +from homeassistant.config_entries import ConfigEntry +from homeassistant.core import HomeAssistant +from homeassistant.helpers.entity_platform import AddEntitiesCallback + +from .ocf.registry.entities import ButtonDesc + +from .const import DOMAIN +from .coordinator import LocalThingsCoordinator +from .entity import LocalThingsEntity + + +async def async_setup_entry( + hass: HomeAssistant, + entry: ConfigEntry, + async_add_entities: AddEntitiesCallback, +) -> None: + coordinator: LocalThingsCoordinator = hass.data[DOMAIN][entry.entry_id] + async_add_entities( + LocalThingsButton(coordinator, b) + for b in coordinator.bound + if isinstance(b.desc, ButtonDesc) + ) + + +class LocalThingsButton(LocalThingsEntity, ButtonEntity): + + def __init__(self, coordinator: LocalThingsCoordinator, bound) -> None: + super().__init__(coordinator, bound) + self._payload = bound.desc.payload + + async def async_press(self) -> None: + await self.coordinator.async_send_command(self._bound, self._payload) diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index 5b88f3d..167c495 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -3,11 +3,9 @@ from __future__ import annotations import datetime import logging -import os import re import socket import ssl -import tempfile from typing import Any import voluptuous as vol @@ -114,7 +112,11 @@ def _mint_leaf_cert(ca_cert_pem: str, ca_key_pem: str, uuid: str) -> tuple[str, serialization.NoEncryption(), ).decode() - fullchain_pem = leaf_cert_pem + ca_cert_pem + # Ensure a newline separates the leaf and CA blocks regardless of + # whether the user's pasted CA PEM had a trailing newline. + fullchain_pem = leaf_cert_pem.rstrip('\n') + '\n' + ca_cert_pem + if not fullchain_pem.endswith('\n'): + fullchain_pem += '\n' return fullchain_pem, leaf_key_pem @@ -143,61 +145,49 @@ def _probe_and_validate(host: str, ca_cert_pem: str, ca_key_pem: str) -> dict: raise CannotConnect(f"Failed to mint leaf cert: {exc}") from exc _LOGGER.debug("Leaf cert minted successfully") - cert_file = tempfile.NamedTemporaryFile(mode='w', suffix='.pem', delete=False) - key_file = tempfile.NamedTemporaryFile(mode='w', suffix='.pem', delete=False) - try: - cert_file.write(fullchain_pem); cert_file.flush(); cert_file.close() - key_file.write(leaf_key_pem); key_file.flush(); key_file.close() - - last_exc = None - for port in PROBE_PORTS: - sess = None - try: - sess = DtlsCoapSession( - host, port, - cert_path=cert_file.name, - key_path=key_file.name, - ) - sess.connect() - sess.start_reader() - code, payload = sess.get(['device', '0'], timeout=15.0) - if code != 0x45 or not payload: - raise CannotConnect(f"port {port}: unexpected code {code:#04x}") - body = cbor2.loads(payload) - resources = ( - parse_device0_batch(body) if isinstance(body, list) else {} - ) - serial = ( - resources - .get('/information/vs/0', {}) - .get('x.com.samsung.da.serialNum', '') - ) - if not serial: - serial = f"{host}:{port}" - return { - "port": port, - "serial": serial, - "leaf_cert_pem": fullchain_pem, - "leaf_key_pem": leaf_key_pem, - } - except CannotConnect: - raise - except Exception as exc: - last_exc = exc - _LOGGER.debug("port %d failed: %s", port, exc) - finally: - if sess is not None: - try: - sess.close() - except Exception: - pass - raise CannotConnect(f"no port responded on {host}: {last_exc}") - finally: - for path in (cert_file.name, key_file.name): - try: - os.unlink(path) - except OSError: - pass + last_exc = None + for port in PROBE_PORTS: + sess = None + try: + sess = DtlsCoapSession( + host, port, + cert_pem=fullchain_pem, + key_pem=leaf_key_pem, + ) + sess.connect() + sess.start_reader() + code, payload = sess.get(['device', '0'], timeout=15.0) + if code != 0x45 or not payload: + raise CannotConnect(f"port {port}: unexpected code {code:#04x}") + body = cbor2.loads(payload) + resources = ( + parse_device0_batch(body) if isinstance(body, list) else {} + ) + serial = ( + resources + .get('/information/vs/0', {}) + .get('x.com.samsung.da.serialNum', '') + ) + if not serial: + serial = f"{host}:{port}" + return { + "port": port, + "serial": serial, + "leaf_cert_pem": fullchain_pem, + "leaf_key_pem": leaf_key_pem, + } + except CannotConnect: + raise + except Exception as exc: + last_exc = exc + _LOGGER.debug("port %d failed: %s", port, exc) + finally: + if sess is not None: + try: + sess.close() + except Exception: + pass + raise CannotConnect(f"no port responded on {host}: {last_exc}") class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): diff --git a/custom_components/localthings/const.py b/custom_components/localthings/const.py index d15bdf9..d929e65 100644 --- a/custom_components/localthings/const.py +++ b/custom_components/localthings/const.py @@ -1,6 +1,6 @@ DOMAIN = "localthings" -PLATFORMS = ["sensor", "binary_sensor", "switch", "number", "select"] +PLATFORMS = ["sensor", "binary_sensor", "switch", "number", "select", "button"] CONF_HOST = "host" CONF_PORT = "port" diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 6ddb470..4351bef 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -2,8 +2,6 @@ from __future__ import annotations import logging -import tempfile -import os from datetime import timedelta from typing import Any @@ -68,34 +66,21 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): return self._last_resources def _connect_session(self) -> None: - host = self._entry.data[CONF_HOST] - port = self._entry.data[CONF_PORT] - cert_pem: str = self._entry.data[CONF_LEAF_CERT_PEM] - key_pem: str = self._entry.data[CONF_LEAF_KEY_PEM] + host = self._entry.data[CONF_HOST] + port = self._entry.data[CONF_PORT] + cert_pem = self._entry.data[CONF_LEAF_CERT_PEM] + key_pem = self._entry.data[CONF_LEAF_KEY_PEM] - cert_f = tempfile.NamedTemporaryFile(suffix='.pem', delete=False, mode='w') - key_f = tempfile.NamedTemporaryFile(suffix='.pem', delete=False, mode='w') + sess = DtlsCoapSession(host, port, cert_pem=cert_pem, key_pem=key_pem) + sess.connect() + sess.start_reader() + self._session = sess + _LOGGER.debug("DTLS connected to %s:%d", host, port) try: - cert_f.write(cert_pem); cert_f.flush(); cert_f.close() - key_f.write(key_pem); key_f.flush(); key_f.close() - sess = DtlsCoapSession(host, port, - cert_path=cert_f.name, - key_path=key_f.name) - sess.connect() - sess.start_reader() - self._session = sess - _LOGGER.debug("DTLS connected to %s:%d", host, port) - try: - self._identity = read_identity(sess, None) - except Exception as e: - _LOGGER.debug("read_identity failed: %s", e) - self._identity = None - finally: - for f in (cert_f.name, key_f.name): - try: - os.unlink(f) - except OSError: - pass + self._identity = read_identity(sess, None) + except Exception as e: + _LOGGER.debug("read_identity failed: %s", e) + self._identity = None def _close_session(self) -> None: sess = self._session @@ -106,6 +91,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): except Exception: pass + async def async_close(self) -> None: + await self.hass.async_add_executor_job(self._close_session) + def _poll_once(self) -> dict[str, dict]: """GET /device/0, return parsed resources. Blocking.""" if self._session is None: diff --git a/custom_components/localthings/number.py b/custom_components/localthings/number.py index 5cb1172..5f6bdf5 100644 --- a/custom_components/localthings/number.py +++ b/custom_components/localthings/number.py @@ -55,14 +55,18 @@ class LocalThingsNumber(LocalThingsEntity, NumberEntity): r = self._range_from_resource() if r is not None: return float(r[0]) - return self._attr_native_min_value + if hasattr(self, '_attr_native_min_value'): + return self._attr_native_min_value + return super().native_min_value @property def native_max_value(self) -> float: r = self._range_from_resource() if r is not None: return float(r[1]) - return self._attr_native_max_value + if hasattr(self, '_attr_native_max_value'): + return self._attr_native_max_value + return super().native_max_value @property def native_value(self): diff --git a/custom_components/localthings/ocf/coap_dtls.py b/custom_components/localthings/ocf/coap_dtls.py index ec0cbc3..8d0d45f 100644 --- a/custom_components/localthings/ocf/coap_dtls.py +++ b/custom_components/localthings/ocf/coap_dtls.py @@ -31,6 +31,33 @@ _OCF_ROOT_CA = str(Path(__file__).parent / 'ocf_root_ca.pem') import logging logger = logging.getLogger(__name__) +import re as _re +_PEM_CERT_RE = _re.compile( + rb'-----BEGIN CERTIFICATE-----.*?-----END CERTIFICATE-----', + _re.DOTALL, +) + + +def _load_pem_chain(ctx: SSL.Context, cert_pem: str, key_pem: str) -> None: + """Load a PEM cert chain and private key into an SSL context in memory. + + Parses all certificate blocks from cert_pem: the first is the leaf + (use_certificate), the rest are intermediates (add_extra_chain_cert). + No temp files are written. + """ + from OpenSSL import crypto + certs = _PEM_CERT_RE.findall(cert_pem.encode()) + if not certs: + raise ValueError("No certificates found in cert_pem") + ctx.use_certificate(crypto.load_certificate(crypto.FILETYPE_PEM, certs[0])) + for extra in certs[1:]: + ctx.add_extra_chain_cert( + crypto.load_certificate(crypto.FILETYPE_PEM, extra) + ) + ctx.use_privatekey(crypto.load_privatekey(crypto.FILETYPE_PEM, key_pem.encode())) + ctx.check_privatekey() + + # Diagnostic logging — when DEBUG_BRIDGE=1 in env, the bridge dumps # every received CoAP frame, every /operational/state/vs/0 + /oven/vs/0 # + /power/vs/0 + /mode/vs/0-options rep change, the full link tree at @@ -188,12 +215,12 @@ class DtlsCoapSession: READER_RECV_TIMEOUT_S = 1.0 # short so stop_event propagates quickly MAX_BLOCKS = 32 # safety bound for Block2 fetches - def __init__(self, host, port, cert_path, key_path, + def __init__(self, host, port, cert_pem, key_pem, on_notification=None, mtu=1200): self.host = host self.port = port - self.cert_path = str(cert_path) - self.key_path = str(key_path) + self.cert_pem = cert_pem + self.key_pem = key_pem self.on_notification = on_notification # fn(href, payload_bytes) self.mtu = mtu @@ -235,9 +262,7 @@ class DtlsCoapSession: # the OpenSSL instance cryptography bundles — ctypes and cffi bindings # do not expose SSL_CTX_set_security_level on this build. ctx.set_cipher_list(b'ECDHE-ECDSA-AES128-GCM-SHA256:@SECLEVEL=0') - ctx.use_certificate_chain_file(self.cert_path) - ctx.use_privatekey_file(self.key_path) - ctx.check_privatekey() + _load_pem_chain(ctx, self.cert_pem, self.key_pem) conn = SSL.Connection(ctx, None) conn.set_connect_state() diff --git a/custom_components/localthings/ocf/registry/capabilities/dryer.py b/custom_components/localthings/ocf/registry/capabilities/dryer.py index 911328f..2449ed9 100644 --- a/custom_components/localthings/ocf/registry/capabilities/dryer.py +++ b/custom_components/localthings/ocf/registry/capabilities/dryer.py @@ -77,7 +77,11 @@ DRYER_COURSE = Capability( entities=( SelectDesc(key='dryer_mode', field='x.com.samsung.da.st.dryerMode', name='Dryer mode', icon='mdi:tumble-dryer', - options=(), write_fn=_course_write), + options=tuple(_COURSE_CODE_BY_NAME), + value_fn=lambda v: _COURSE_NAMES.get( + int(v.split('_')[1], 16) if isinstance(v, str) and '_' in v else -1 + ), + write_fn=_course_write), ), ) diff --git a/custom_components/localthings/select.py b/custom_components/localthings/select.py index 0c8343a..dbe637d 100644 --- a/custom_components/localthings/select.py +++ b/custom_components/localthings/select.py @@ -43,8 +43,7 @@ class LocalThingsSelect(LocalThingsEntity, SelectEntity): super().__init__(coordinator, bound) desc: SelectDesc = bound.desc if not desc.options_field: - opts = desc.options - self._attr_options = list(opts() if callable(opts) else opts) + self._attr_options = list(desc.options) @property def options(self) -> list[str]: diff --git a/tests/localthings/conftest.py b/tests/localthings/conftest.py index d979e14..9bcb9b4 100644 --- a/tests/localthings/conftest.py +++ b/tests/localthings/conftest.py @@ -32,7 +32,8 @@ def auto_enable_custom_integrations(enable_custom_integrations): from custom_components.localthings.const import ( - CONF_CERT_PEM, CONF_HOST, CONF_KEY_PEM, CONF_PORT, DOMAIN, + CONF_CA_CERT_PEM, CONF_CA_KEY_PEM, + CONF_HOST, CONF_LEAF_CERT_PEM, CONF_LEAF_KEY_PEM, CONF_PORT, DOMAIN, ) FIXTURES = Path(__file__).resolve().parent.parent / 'fixtures' @@ -40,14 +41,18 @@ FIXTURES = Path(__file__).resolve().parent.parent / 'fixtures' MOCK_HOST = '10.0.0.254' MOCK_PORT = 49154 MOCK_SERIAL = 'TEST-SERIAL-001' -MOCK_CERT_PEM = '-----BEGIN CERTIFICATE-----\nTEST\n-----END CERTIFICATE-----' -MOCK_KEY_PEM = '-----BEGIN PRIVATE KEY-----\nTEST\n-----END PRIVATE KEY-----' +MOCK_CA_CERT_PEM = '-----BEGIN CERTIFICATE-----\nTEST-CA\n-----END CERTIFICATE-----' +MOCK_CA_KEY_PEM = '-----BEGIN PRIVATE KEY-----\nTEST-CA-KEY\n-----END PRIVATE KEY-----' +MOCK_LEAF_CERT_PEM = '-----BEGIN CERTIFICATE-----\nTEST-LEAF\n-----END CERTIFICATE-----' +MOCK_LEAF_KEY_PEM = '-----BEGIN PRIVATE KEY-----\nTEST-LEAF-KEY\n-----END PRIVATE KEY-----' ENTRY_DATA = { - CONF_HOST: MOCK_HOST, - CONF_PORT: MOCK_PORT, - CONF_CERT_PEM: MOCK_CERT_PEM, - CONF_KEY_PEM: MOCK_KEY_PEM, + CONF_HOST: MOCK_HOST, + CONF_PORT: MOCK_PORT, + CONF_CA_CERT_PEM: MOCK_CA_CERT_PEM, + CONF_CA_KEY_PEM: MOCK_CA_KEY_PEM, + CONF_LEAF_CERT_PEM: MOCK_LEAF_CERT_PEM, + CONF_LEAF_KEY_PEM: MOCK_LEAF_KEY_PEM, } @@ -67,7 +72,12 @@ def mock_probe(): """Patch _probe_and_validate to succeed without a real DTLS connection.""" with patch( 'custom_components.localthings.config_flow._probe_and_validate', - return_value={'port': MOCK_PORT, 'serial': MOCK_SERIAL}, + return_value={ + 'port': MOCK_PORT, + 'serial': MOCK_SERIAL, + 'leaf_cert_pem': MOCK_LEAF_CERT_PEM, + 'leaf_key_pem': MOCK_LEAF_KEY_PEM, + }, ) as m: yield m diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index baee55c..1c63836 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -9,27 +9,27 @@ from homeassistant.data_entry_flow import FlowResultType from pytest_homeassistant_custom_component.common import MockConfigEntry from custom_components.localthings.const import ( - CONF_CERT_PEM, CONF_HOST, CONF_KEY_PEM, CONF_PORT, DOMAIN, + CONF_CA_CERT_PEM, CONF_CA_KEY_PEM, CONF_HOST, CONF_PORT, DOMAIN, ) from .conftest import ( - ENTRY_DATA, MOCK_CERT_PEM, MOCK_HOST, MOCK_KEY_PEM, MOCK_PORT, MOCK_SERIAL, + ENTRY_DATA, MOCK_CA_CERT_PEM, MOCK_CA_KEY_PEM, MOCK_HOST, MOCK_PORT, MOCK_SERIAL, ) async def test_form_first_device(hass: HomeAssistant) -> None: - """First device: form asks for host, cert, and key.""" + """First device: form asks for host, CA cert, and CA key.""" result = await hass.config_entries.flow.async_init( DOMAIN, context={'source': 'user'} ) assert result['type'] == FlowResultType.FORM assert result['step_id'] == 'user' - assert CONF_CERT_PEM in result['data_schema'].schema - assert CONF_KEY_PEM in result['data_schema'].schema + assert CONF_CA_CERT_PEM in result['data_schema'].schema + assert CONF_CA_KEY_PEM in result['data_schema'].schema async def test_form_second_device_reuses_creds(hass: HomeAssistant) -> None: - """Second device: form only asks for host; cert/key schema fields absent.""" + """Second device: form only asks for host; CA cert/key schema fields absent.""" existing = MockConfigEntry(domain=DOMAIN, data=ENTRY_DATA) existing.add_to_hass(hass) @@ -37,8 +37,8 @@ async def test_form_second_device_reuses_creds(hass: HomeAssistant) -> None: DOMAIN, context={'source': 'user'} ) assert result['type'] == FlowResultType.FORM - assert CONF_CERT_PEM not in result['data_schema'].schema - assert CONF_KEY_PEM not in result['data_schema'].schema + assert CONF_CA_CERT_PEM not in result['data_schema'].schema + assert CONF_CA_KEY_PEM not in result['data_schema'].schema async def test_successful_setup(hass: HomeAssistant, mock_probe) -> None: @@ -48,12 +48,12 @@ async def test_successful_setup(hass: HomeAssistant, mock_probe) -> None: ) result = await hass.config_entries.flow.async_configure( result['flow_id'], - {CONF_HOST: MOCK_HOST, CONF_CERT_PEM: MOCK_CERT_PEM, CONF_KEY_PEM: MOCK_KEY_PEM}, + {CONF_HOST: MOCK_HOST, CONF_CA_CERT_PEM: MOCK_CA_CERT_PEM, CONF_CA_KEY_PEM: MOCK_CA_KEY_PEM}, ) assert result['type'] == FlowResultType.CREATE_ENTRY assert result['data'][CONF_HOST] == MOCK_HOST assert result['data'][CONF_PORT] == MOCK_PORT - assert result['data'][CONF_CERT_PEM] == MOCK_CERT_PEM + assert result['data'][CONF_CA_CERT_PEM] == MOCK_CA_CERT_PEM async def test_cannot_connect(hass: HomeAssistant) -> None: @@ -69,7 +69,7 @@ async def test_cannot_connect(hass: HomeAssistant) -> None: ) result = await hass.config_entries.flow.async_configure( result['flow_id'], - {CONF_HOST: MOCK_HOST, CONF_CERT_PEM: MOCK_CERT_PEM, CONF_KEY_PEM: MOCK_KEY_PEM}, + {CONF_HOST: MOCK_HOST, CONF_CA_CERT_PEM: MOCK_CA_CERT_PEM, CONF_CA_KEY_PEM: MOCK_CA_KEY_PEM}, ) assert result['type'] == FlowResultType.FORM assert result['errors']['base'] == 'cannot_connect' @@ -78,7 +78,7 @@ async def test_cannot_connect(hass: HomeAssistant) -> None: async def test_duplicate_device_aborted(hass: HomeAssistant, mock_probe) -> None: """Second add of same serial: flow aborts. - When a device already exists the form only asks for host (cert/key are + When a device already exists the form only asks for host (CA creds are reused), so we only submit CONF_HOST in the second configure call. """ existing = MockConfigEntry( @@ -91,7 +91,7 @@ async def test_duplicate_device_aborted(hass: HomeAssistant, mock_probe) -> None result = await hass.config_entries.flow.async_init( DOMAIN, context={'source': 'user'} ) - # Second-device form only has CONF_HOST; cert/key are reused from existing. + # Second-device form only has CONF_HOST; CA creds are reused from existing. result = await hass.config_entries.flow.async_configure( result['flow_id'], {CONF_HOST: MOCK_HOST},