From 81dbc6fa03bfaae80eef8b72d674bb50186b63de Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Mon, 17 Aug 2026 05:24:22 +0000 Subject: [PATCH] Key devices on the OCF device ID instead of the serial number Two Samsung air purifiers of the same model report the identical, well formed serialNum `BS7SP9AW400114A` (issue #381). Since the entry's unique_id, the device registry identifiers and every entity unique_id were all minted from that string, the second unit was refused as already configured, and would have collided entity-for-entity even if it hadn't been. This is the third firmware family to ship an unusable serialNum, after `Nothing(SVC)` (#83) and the flash-unset sentinel (#189), and the first one no heuristic can catch: the value is well formed, it's just shared. `is_placeholder_serial` was a dead end. So identity moves onto /oic/d's `di`, falling back to /oic/p's `pi`, then the serial, then the host. `di` is what the protocol already uses to address the endpoint -- if it were wrong or shared, OCF discovery and the DTLS association wouldn't work at all -- and it's device-scoped, where `pi` is platform-scoped and would be shared by a board hosting several logical devices. Both units in #381 report a distinct `di`. A board that answers neither resource lands exactly where it did before, so no existing hardware regresses. The re-key can't happen in async_migrate_entry: the UUID is only readable from the device, and an entry can load entirely from its snapshot while the appliance is off (#295). So v3 -> v4 only records the legacy key, and the coordinator adopts the UUID on the first live poll, rewriting the entity registry, the device registry (including subdevice identifiers) and the entry's unique_id together. Rewriting rather than recreating is what lets a user keep entity_ids, names, areas, statistics and every automation that references them. Three rules keep that adoption from misfiring: - A poll that reads no UUID never demotes a UUID-keyed entry back onto its serial, so one failed reconnect doesn't re-key every entity. - A changed UUID is followed only when the serial still corroborates it (a factory reset may regenerate `di`) or when the entry was keyed on its IP, which was never an identity to defend. - When the identity is rejected as a different appliance, the serial isn't adopted either -- otherwise the intruder would gain exactly the corroboration needed to win the next poll. Also stop redacting `di`/`pi` from diagnostics. They're randomly assigned per-unit UUIDs, not account data, and blanking them is what made the first #381 diagnostics download unable to answer the only question it was requested to answer. The owner-set device name stays redacted. Fixes #381 --- README.md | 5 +- custom_components/localthings/__init__.py | 90 ++--- custom_components/localthings/config_flow.py | 31 +- custom_components/localthings/const.py | 10 + custom_components/localthings/coordinator.py | 186 +++++++-- custom_components/localthings/entity.py | 2 +- .../localthings/registry/identity.py | 85 +++++ .../localthings/registry/redact.py | 25 +- custom_components/localthings/rekey.py | 110 ++++++ custom_components/localthings/sensor.py | 2 +- tests/localthings/conftest.py | 11 +- tests/localthings/test_config_flow.py | 94 ++++- tests/localthings/test_coordinator.py | 24 +- tests/localthings/test_device_removal.py | 2 +- tests/localthings/test_diagnostics.py | 11 +- tests/localthings/test_migration.py | 359 ++++++++++++++++-- .../localthings/test_statistics_migration.py | 24 +- tests/test_air_purifier_airflow_fan.py | 2 +- tests/test_air_purifier_vtww_fan.py | 2 +- tests/test_airconditioner_artik051_krac.py | 2 +- .../test_airconditioner_tp1x_rac_01001_fan.py | 2 +- tests/test_climate_ac_modes.py | 2 +- tests/test_entity_naming.py | 2 +- tests/test_fridge_capabilities.py | 2 +- tests/test_identity.py | 118 +++++- tests/test_range_hood_fan.py | 2 +- tests/test_redact.py | 23 +- tests/test_select_options.py | 2 +- tests/test_sensor_enum_options.py | 2 +- tests/test_sensor_hysteresis.py | 2 +- tests/test_sensor_sticky.py | 2 +- tests/test_subdevice_discovery.py | 6 +- tests/test_water_heater_ehs.py | 2 +- 33 files changed, 1073 insertions(+), 171 deletions(-) create mode 100644 custom_components/localthings/rekey.py diff --git a/README.md b/README.md index 671effc..ea6ebd9 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ This repo doesn't include the needed CA bundle. For an example of how to obtain 5. The flow sends a DTLS `ClientHello` to every port in the `49152-49160` range at once and keeps the one that answers -- a real DTLS server identifies itself in about one round trip, and the probe stops there, so nothing is left behind on the appliance. Only that port is then given a real certificate handshake: it fetches the current UUID from Samsung's cloud gateway, mints a leaf cert signed by your CA, and reads the device's identity and `/device/0`. On success it creates the config entry, already knowing the appliance's serial, model, and type. 6. Every subsequent device only asks for the host IP. The stored CA credentials are reused, and so is the leaf cert itself -- every appliance accepts the same one -- so adding a second appliance doesn't depend on Samsung's cloud being reachable at all. If a device rejects the reused cert (the UUID behind it does rotate), the flow mints a fresh one and retries by itself. -Entities appear under one HA device per appliance, named for the appliance's type and model. Rename freely: the device is keyed on its serial, not its name. +Entities appear under one HA device per appliance, named for the appliance's type and model. Rename freely: the device is keyed on the appliance's own OCF device ID, not its name. (Some Samsung models ship the same serial number on every unit of a model, so the serial can't tell two of them apart -- the OCF device ID can.) --- @@ -237,7 +237,8 @@ docker-compose.yml / ha_config/ Local HA dev environment If your appliance's type isn't recognized, or it exposes resources this integration doesn't model yet, a Repairs issue appears under Settings > System > Repairs pointing you at Settings > Devices & Services > this device > the menu > Download diagnostics. That download is already redacted of account/network identifiers (Bixby login -email, access tokens, device IDs, MAC addresses, serial numbers) before it's generated, so it's safe to attach +email, access tokens, hashed device IDs, MAC addresses, serial numbers, and the owner-set device name) before it's +generated, so it's safe to attach directly to a new issue using the linked device-support template. This is the fastest way to help add or expand support for hardware the maintainers don't have. diff --git a/custom_components/localthings/__init__.py b/custom_components/localthings/__init__.py index 79bc444..3f82537 100644 --- a/custom_components/localthings/__init__.py +++ b/custom_components/localthings/__init__.py @@ -19,6 +19,7 @@ from homeassistant.helpers.typing import ConfigType from .const import CONF_DEVICE_TYPE, CONF_HOST, CONF_PORT, CONF_SERIAL, DOMAIN, PLATFORMS from .coordinator import LocalThingsCoordinator, snapshot_store from .registry.identity import resolve_serial +from .rekey import rekey_entry from .services import async_setup_services _LOGGER = logging.getLogger(__name__) @@ -82,67 +83,20 @@ def _repair_placeholder_keys(hass: HomeAssistant, entry: ConfigEntry, serial: st """Re-key registry entries this entry minted from the placeholder identity. Before the identity moved onto the config entry, the coordinator seeded - `device_serial` with the host and only replaced it after the first poll. + its device key with the host and only replaced it after the first poll. Anything that registered in between -- the connection-mode sensor especially, added unconditionally rather than from `bound` -- was written into the registry keyed on the IP permanently, orphaned the moment the serial-keyed identity appeared (issue #236). Deleting the orphans by hand didn't help: the next restart that lost the same race recreated them. - - Rewriting beats deleting where possible -- an entity keeps its - entity_id, name, area and automations. Only possible when the - serial-keyed key is still free; where both exist the placeholder-keyed - one is the dead duplicate (unavailable since the restart that created - it), so it goes. """ host = entry.data[CONF_HOST] if serial == host: # A board with no usable serial resolves to the host, so its keys # were never placeholders. return - - ent_reg = er.async_get(hass) - stale_prefix = f"{DOMAIN}_{host}_" - for entity in list(er.async_entries_for_config_entry(ent_reg, entry.entry_id)): - if not entity.unique_id.startswith(stale_prefix): - continue - new_unique_id = f"{DOMAIN}_{serial}_{entity.unique_id[len(stale_prefix) :]}" - if ent_reg.async_get_entity_id(entity.domain, DOMAIN, new_unique_id): - _LOGGER.debug("removing orphaned entity %s", entity.entity_id) - ent_reg.async_remove(entity.entity_id) - else: - _LOGGER.debug("re-keying entity %s to %s", entity.entity_id, new_unique_id) - ent_reg.async_update_entity(entity.entity_id, new_unique_id=new_unique_id) - - dev_reg = dr.async_get(hass) - for device in list(dr.async_entries_for_config_entry(dev_reg, entry.entry_id)): - # `host` for the master, `host_` for a subdevice (device_info_for). - stale = { - ident - for ident in device.identifiers - if ident[0] == DOMAIN and (ident[1] == host or ident[1].startswith(f"{host}_")) - } - if not stale: - continue - fresh = {(DOMAIN, f"{serial}{ident[1][len(host) :]}") for ident in stale} - existing = dev_reg.async_get_device(identifiers=fresh) - if existing is not None and existing.id != device.id: - # Removing a device takes its entities with it. Anything still - # attached here was re-keyed rather than removed above -- the - # surviving copy, not a duplicate -- so move it onto the device - # it now belongs to before the removal destroys it too. - for entity in er.async_entries_for_device( - ent_reg, device.id, include_disabled_entities=True - ): - ent_reg.async_update_entity(entity.entity_id, device_id=existing.id) - _LOGGER.debug("removing orphaned device %s", device.id) - dev_reg.async_remove_device(device.id) - else: - _LOGGER.debug("re-keying device %s to %s", device.id, fresh) - dev_reg.async_update_device( - device.id, new_identifiers=(device.identifiers - stale) | fresh - ) + rekey_entry(hass, entry, host, serial) # Registries whose Dust/FineDust/SuperFineDust sensors gained pm10/pm25/pm1 @@ -249,8 +203,22 @@ async def async_migrate_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: v2 -> v3 relabels the recorded statistics for the particulate sensors, which gained a device_class/unit in the same release (issue #325). + + v3 -> v4 moves the entry off the serialNum as its identity and onto the + OCF device UUID (issue #381). Deliberately almost a no-op here: the + UUID lives on the device, and this runs before any I/O -- and before + the device is even known to be reachable, since an entry can load + entirely from its snapshot while the appliance is off (issue #295). + So the migration only guarantees CONF_SERIAL is populated, which is + what the coordinator rewrites *from* once a live poll finally hands it + a UUID to rewrite *to*. + + The version is still bumped now rather than at that point, because the + bump's real job is the `entry.version > 4` downgrade guard below: an + entry re-keyed onto a UUID and then loaded by a release that reads + CONF_SERIAL as the key would silently orphan every entity it has. """ - if entry.version > 3: + if entry.version > 4: return False # downgrade: this release doesn't know the newer shape if entry.version == 1: @@ -268,6 +236,28 @@ async def async_migrate_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: hass.config_entries.async_update_entry(entry, version=3) _LOGGER.debug("migrated entry %s to version 3", entry.entry_id) + if entry.version == 3: + # `or host` mirrors what the coordinator has always fallen back to, + # so the recorded legacy key is the one this entry's registry + # entries were actually minted under even if CONF_SERIAL never got + # written (a v1 entry whose migration predates it). Both being + # absent shouldn't happen for an entry the config flow created, but + # an exception raised here fails the whole entry -- so it bumps the + # version and leaves the data alone rather than taking that risk + # for a value the coordinator re-derives on its next poll anyway. + legacy_key = entry.data.get(CONF_SERIAL) or entry.data.get(CONF_HOST) + hass.config_entries.async_update_entry( + entry, + data={**entry.data, CONF_SERIAL: legacy_key} if legacy_key else entry.data, + version=4, + ) + _LOGGER.debug( + "migrated entry %s to version 4 (legacy key=%s, awaiting a poll to adopt " + "the OCF device id)", + entry.entry_id, + legacy_key, + ) + return True diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index 04a0122..304e83a 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -43,6 +43,7 @@ from .const import ( CONF_CA_CERT_PEM, CONF_CA_KEY_PEM, CONF_CLOUD_COURSES_ENABLED, + CONF_DEVICE_KEY, CONF_DEVICE_TYPE, CONF_FINISH_TIME_HYSTERESIS_MINUTES, CONF_HOST, @@ -662,7 +663,12 @@ def _read_device(sess, host: str, port: int) -> dict: from .registry.batch import parse_device0_batch from .registry.by_type import resolve as resolve_registry - from .registry.identity import read_identity, resolve_model, resolve_serial + from .registry.identity import ( + read_identity, + resolve_device_key, + resolve_model, + resolve_serial, + ) identity = read_identity(sess, None) @@ -680,12 +686,19 @@ def _read_device(sess, host: str, port: int) -> dict: info = resources.get("/information/vs/0", {}) registry = resolve_registry(resources, device_types=identity.device_types) + raw_serial = info.get("x.com.samsung.da.serialNum") return { "port": port, # Resolved through the same helpers _run_discovery uses, so the device # the coordinator registers up front is the one discovery would have # produced -- no rename, and no re-key, once the first poll lands. - "serial": resolve_serial(info.get("x.com.samsung.da.serialNum"), host), + # + # `device_key` is what the entry is actually keyed on; the serial is + # kept alongside it because the coordinator corroborates a later + # change of key against it (issue #381). read_identity has already + # fetched /oic/p and /oic/d above, so this costs no extra round trip. + "device_key": resolve_device_key(identity, raw_serial, host), + "serial": resolve_serial(raw_serial, host), "model": resolve_model(info.get("x.com.samsung.da.modelNum", ""), identity), "manufacturer": identity.manufacturer or "Samsung", "device_type_name": registry.name if registry is not None else None, @@ -798,7 +811,10 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): # v3 relabels the particulate sensors' recorded statistics; a freshly # created entry has none to relabel, so it starts at the migrated # version rather than walking through v2 (see async_migrate_entry). - VERSION = 3 + # v4 keys the entry on the OCF device UUID (issue #381), which the probe + # below resolves up front -- so a new entry is already on the v4 shape + # and has nothing to re-key either. + VERSION = 4 def __init__(self) -> None: self._host: str = "" @@ -817,7 +833,7 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): """Persist everything the probe resolved, identity included. The identity fields aren't decoration: the coordinator seeds - `device_serial` and its DeviceInfo from them at construction time, + `device_key` and its DeviceInfo from them at construction time, so entity unique_ids are correct from the first entity that registers, even if the first poll is slow or fails (issue #236). """ @@ -832,6 +848,7 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): CONF_CA_KEY_PEM: self._ca_key_pem, CONF_LEAF_CERT_PEM: info["leaf_cert_pem"], CONF_LEAF_KEY_PEM: info["leaf_key_pem"], + CONF_DEVICE_KEY: info["device_key"], CONF_SERIAL: info["serial"], CONF_MODEL: info["model"], CONF_MANUFACTURER: info["manufacturer"], @@ -878,7 +895,11 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): _LOGGER.exception("Unexpected error during device probe") errors["base"] = "unknown" else: - await self.async_set_unique_id(f"localthings_{info['serial']}") + # Keyed on the OCF device UUID rather than the serialNum + # (issue #381): two units of a model that ship the same + # well-formed serial are indistinguishable here otherwise, + # and the second one is turned away as already configured. + await self.async_set_unique_id(f"localthings_{info['device_key']}") self._abort_if_unique_id_configured() if info["device_type_recognized"]: return self._create_entry(info) diff --git a/custom_components/localthings/const.py b/custom_components/localthings/const.py index bd4eb9b..67d3c44 100644 --- a/custom_components/localthings/const.py +++ b/custom_components/localthings/const.py @@ -30,6 +30,16 @@ CONF_LEAF_KEY_PEM = "leaf_key_pem" # output, the host itself for a placeholder-serial board -- issues # #83/#189), so it matches what _run_discovery computes on the first poll. CONF_SERIAL = "serial" +# The identity this entry's devices and entities are actually keyed on -- +# registry.identity.resolve_device_key's output, normally the OCF device +# UUID (issue #381). Distinct from CONF_SERIAL, which stays as (a) the key +# a pre-v4 entry was minted with, so the one-time re-key knows what to +# rewrite from, and (b) the corroborating identity that tells a device +# whose `di` rotated across a factory reset apart from a different +# appliance that moved onto this address. Absent on an entry that has not +# polled since upgrading: the OCF resources are only readable from the +# device, so the coordinator adopts this on the first live poll. +CONF_DEVICE_KEY = "device_key" CONF_MODEL = "model" CONF_MANUFACTURER = "manufacturer" CONF_DEVICE_TYPE = "device_type" diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index dc80977..6c85954 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -31,6 +31,7 @@ from .const import ( CONF_BYPASS_REMOTE_CONTROL, CONF_CLOUD_COURSES, CONF_CLOUD_COURSES_ENABLED, + CONF_DEVICE_KEY, CONF_DEVICE_TYPE, CONF_HOST, CONF_LEAF_CERT_PEM, @@ -66,6 +67,7 @@ from .registry.entities import ClimateDesc from .registry.identity import ( DeviceIdentity, device_display_name, + ocf_device_key, read_identity, resolve_model, resolve_serial, @@ -78,6 +80,7 @@ from .registry.subdevices import ( enumerate_subdevices, normalize_seed_batch, ) +from .rekey import rekey_entry # Sentinel for apply_cloud_courses: "leave this field as it is", # distinct from None which means "clear it". @@ -205,7 +208,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): bound: list[BoundEntity] device_info: DeviceInfo - device_serial: str + device_key: str # Class-level so tests can shrink these via patch.object() without # touching the production defaults. @@ -310,15 +313,22 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._push_pending = False self._push_pending_lock = threading.Lock() # Identity is resolved once by the config flow's probe (issue #236). - # device_serial mints permanent registry keys, so it must be correct + # device_key mints permanent registry keys, so it must be correct # before the first entity registers -- a placeholder corrected once # the first poll lands orphans the first device/entity pair instead. - # The host fallback covers a pre-migration entry and matches what - # resolve_serial itself returns for a placeholder-serial board - # (issues #83/#189). - self.device_serial = entry.data.get(CONF_SERIAL) or entry.data[CONF_HOST] + # + # CONF_DEVICE_KEY is what a v4 entry stores (the OCF device UUID, + # issue #381); CONF_SERIAL is both the pre-v4 key and the fallback + # for a board that answers no OCF identity resource; and the host + # covers a pre-migration entry, matching what resolve_serial itself + # returns for a placeholder-serial board (issues #83/#189). The + # chain is ordered so an entry that has not polled since upgrading + # keeps loading under the key its registry entries already carry. + self.device_key = ( + entry.data.get(CONF_DEVICE_KEY) or entry.data.get(CONF_SERIAL) or entry.data[CONF_HOST] + ) self.device_info = DeviceInfo( - identifiers={(DOMAIN, self.device_serial)}, + identifiers={(DOMAIN, self.device_key)}, name=device_display_name( entry.data.get(CONF_DEVICE_TYPE), entry.data.get(CONF_MODEL) or "" ), @@ -768,8 +778,8 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): else "Secondary Subdevice" ) return DeviceInfo( - identifiers={(DOMAIN, f"{self.device_serial}_{subdevice.key}")}, - via_device=(DOMAIN, self.device_serial), + identifiers={(DOMAIN, f"{self.device_key}_{subdevice.key}")}, + via_device=(DOMAIN, self.device_key), name=f"{base_name} {label}", manufacturer=self.device_info.get("manufacturer") or "Samsung", model=model or None, @@ -1093,8 +1103,120 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._skipped_subdevice_resources = skipped return kept + def _resolve_identity(self, polled_serial: str, *, from_snapshot: bool) -> tuple[str, str]: + """The (key, serial) this entry should be registered under, re-keying + the registries if the key has moved. + + Returns both together because they have to agree: the serial is what + corroborates a later change of key, so writing this poll's serial + while *defending* the stored key against a different appliance would + quietly hand that appliance the corroboration it needs to win the + next poll. The serial is therefore only adopted alongside a key we + accepted -- never on a poll whose identity we just rejected. + + The stored key wins by default: re-keying an entry that already has + registry entries orphans them unless the rewrite goes with it + (issue #236), so nothing here changes a key without calling + `rekey_entry` in the same breath. + + Four things can happen: + + * A snapshot replay (issue #295) never re-keys. That path did not + reach the device, so its "reported" identity is only whatever the + snapshot happened to preserve. + * Nothing stored (a pre-v4 entry, or a legacy migration that could + not recover an identity) -- adopt what the device reports and + rewrite the registries off the legacy key onto it. This is the + one-time move onto the OCF device UUID (issue #381), deferred to + here because the UUID is only readable from the device itself. + * A key is stored and this poll produced no UUID -- keep it. The + device saying nothing is not the device saying something + different, and demoting a UUID-keyed entry back onto its serial + because one reconnect couldn't read /oic/d would re-key every + entity the user has for the duration of an outage. + * A key is stored and the UUID differs. Either this appliance's + `di` rotated (an OCF hard reset is allowed to regenerate it) or a + *different* appliance now answers on this address. The serialNum + tells them apart: a usable one that still matches what this entry + was registered with means same unit, new UUID, so follow it. + Anything else keeps the registered identity and warns -- re-adding + is the user's call, not something a poll gets to decide. + """ + host = self._entry.data[CONF_HOST] + stored_serial = self._entry.data.get(CONF_SERIAL) + if from_snapshot: + return self.device_key, stored_serial or polled_serial + + polled_ocf = ocf_device_key(self._identity) + stored_key = self._entry.data.get(CONF_DEVICE_KEY) + + if stored_key is None: + # `polled_serial` is already resolve_serial's output, so this is + # exactly resolve_device_key's chain: the UUID where the device + # reports one, today's serial/host answer where it doesn't. + polled_key = polled_ocf or polled_serial + legacy_key = stored_serial or host + if legacy_key != polled_key: + self._log.info( + "moving this device's registry entries from %r onto %r", + legacy_key, + polled_key, + ) + rekey_entry(self.hass, self._entry, legacy_key, polled_key) + return polled_key, polled_serial + + if polled_ocf == stored_key: + # Confirmed the registered appliance, so this poll's serial is + # authoritative -- firmware is allowed to correct it. + return stored_key, polled_serial + if polled_ocf is None: + # Nothing to compare against: this may or may not be the + # registered appliance, so neither half is updated. + return stored_key, stored_serial or polled_serial + polled_key = polled_ocf + + # Excluding the host answer is what keeps this from firing on two + # *different* placeholder-serial units: `polled_serial` is already + # resolved, so anything that fell back to the address is not an + # identity and can't corroborate anything. + same_unit = polled_serial == stored_serial and polled_serial != host + # An entry keyed on the host never made an identity claim to defend + # -- it is registered against "whatever answers at this address" + # (issues #83/#189). Any real UUID is a strict improvement on that, + # so it is adopted without needing the serial to corroborate it; + # requiring corroboration would strand exactly the placeholder-serial + # boards this whole change is meant to rescue, since their serial + # resolves to the host and can never match. + if not (stored_key == host or same_unit): + self._log.warning( + "device at %s identifies as %r but this entry is registered as %r " + "(serial %r, registered %r); keeping the registered identity", + host, + polled_key, + stored_key, + polled_serial, + stored_serial, + ) + return stored_key, stored_serial or polled_serial + + # Corroborated: same unit with a new UUID (a factory reset is + # allowed to regenerate `di`), or an address-keyed entry finally + # learning a real identity. Following it keeps the user's + # entity_ids, history and automations rather than stranding them on + # a key the device will never report again. + self._log.info( + "device %s (serial %r) changed key from %r to %r; following it", + host, + polled_serial, + stored_key, + polled_key, + ) + rekey_entry(self.hass, self._entry, stored_key, polled_key) + return polled_key, polled_serial + def _persist_identity( self, + device_key: str | None, serial: str, model: str, manufacturer: str, @@ -1108,9 +1230,16 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): means the next restart names the device fully instead of renaming it again once a poll lands. + `device_key` is what the registries are keyed on; `serial` is stored + alongside it rather than replaced by it, because it is what + `_resolve_key` corroborates a changed UUID against on a later poll. + None leaves whatever key is already stored untouched -- see the + caller for why a snapshot replay must not write one. + Runs on the event loop, which async_update_entry requires. """ identity = { + **({CONF_DEVICE_KEY: device_key} if device_key is not None else {}), CONF_SERIAL: serial, CONF_MODEL: model, CONF_MANUFACTURER: manufacturer, @@ -1195,6 +1324,11 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): model=ident.get("model") or "", name=ident.get("name") or "", serial=ident.get("serial"), + # Absent from a snapshot written before these fields + # existed; _resolve_key never re-keys from a replay, so + # a missing UUID here costs nothing beyond diagnostics. + device_id=ident.get("device_id"), + platform_id=ident.get("platform_id"), device_types=tuple(ident.get("device_types") or ()), raw=ident.get("raw") or {}, ) @@ -1346,26 +1480,11 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._unbound_hrefs = unbound self._refresh_learnable_hrefs() - # The entry's stored identity wins; this poll's answer is only - # adopted when nothing is stored (a legacy migration couldn't - # recover it), then written back. Re-keying an entry with existing - # registry entries orphans them (issue #236). polled_serial = resolve_serial( info.get("x.com.samsung.da.serialNum"), self._entry.data[CONF_HOST] ) - serial = self._entry.data.get(CONF_SERIAL) or polled_serial - if serial != polled_serial: - # Same IP, different appliance (or firmware that changed what it - # reports) -- keep the registered identity; re-adding is the - # user's call. - self._log.warning( - "device at %s reports serial %r but this entry is registered " - "as %r; keeping the registered identity", - self._entry.data[CONF_HOST], - polled_serial, - serial, - ) - self.device_serial = serial + key, serial = self._resolve_identity(polled_serial, from_snapshot=from_snapshot) + self.device_key = key ident = self._identity model = resolve_model(model_num, ident) @@ -1373,12 +1492,19 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): mfr = (ident.manufacturer if ident else "") or "Samsung" self.device_info = DeviceInfo( - identifiers={(DOMAIN, serial)}, + identifiers={(DOMAIN, key)}, name=name, manufacturer=mfr, model=model, ) - self._persist_identity(serial, model, mfr, device_type_name) + # A snapshot replay passes None: it never reached the device, so it + # has no standing to write a device key. Persisting `self.device_key` + # there would freeze a pre-v4 entry's legacy key into CONF_DEVICE_KEY + # as if a poll had confirmed it, and _resolve_key would then treat + # the real UUID -- when a live poll finally produced one -- as a + # changed identity to defend against rather than the one-time + # adoption it is. + self._persist_identity(None if from_snapshot else key, serial, model, mfr, device_type_name) if not from_snapshot: # A coverage gap is a claim about what the device reports, so only # a live poll gets to make it. Replaying a snapshot would restate @@ -1392,9 +1518,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): self._discovered = True self._log.info( - "discovered %d entities (serial=%s) hot=%s warm=%s subdevices=%s", + "discovered %d entities (key=%s) hot=%s warm=%s subdevices=%s", len(bound), - serial, + key, self._hot_hrefs, self._warm_hrefs, [su.key for su in self.subdevices], diff --git a/custom_components/localthings/entity.py b/custom_components/localthings/entity.py index a6fc65c..c8a97b5 100644 --- a/custom_components/localthings/entity.py +++ b/custom_components/localthings/entity.py @@ -88,7 +88,7 @@ class LocalThingsEntity(CoordinatorEntity[LocalThingsCoordinator]): super().__init__(coordinator) self._bound = bound self._state_key = _key(bound) - self._attr_unique_id = f"{DOMAIN}_{coordinator.device_serial}_{self._state_key}" + self._attr_unique_id = f"{DOMAIN}_{coordinator.device_key}_{self._state_key}" if bound.desc.translation_placeholders is not None: self._attr_translation_placeholders = dict(bound.desc.translation_placeholders) elif bound.desc.use_instance_name: diff --git a/custom_components/localthings/registry/identity.py b/custom_components/localthings/registry/identity.py index c53bcdb..c9b5dcc 100644 --- a/custom_components/localthings/registry/identity.py +++ b/custom_components/localthings/registry/identity.py @@ -13,6 +13,12 @@ class DeviceIdentity: model: str name: str serial: str | None + # /oic/d's `di` and /oic/p's `pi` -- OCF's own device and platform + # UUIDs. Promoted out of `raw` into named fields because + # resolve_device_key mints permanent registry keys from them; see its + # docstring for why `di` leads. + device_id: str | None = None + platform_id: str | None = None device_types: tuple[str, ...] = () raw: dict[str, dict | list] = field(default_factory=dict) @@ -57,6 +63,83 @@ def resolve_serial(raw_serial: str | None, host: str) -> str: return s +def is_usable_device_id(value: str | None) -> bool: + """True for an OCF `di`/`pi` that actually identifies one unit. + + Firmware that never had a UUID assigned reports OCF's nil UUID + (all-zero, dashes aside), which is identical on every unit of the + family -- the #189 failure mode transplanted onto a new field, and not + something `is_placeholder_serial`'s repeated-hex-digit rule catches, + because the dashes make more than one distinct character. + + Past that, the same known-junk rules that disqualify a serialNum + disqualify a UUID: a board firmware-flashed with 'Nothing(SVC)' in one + identity field is not a board to trust in another. Anything else is + accepted as-is. A *shared but well-formed* `di` would be undetectable + here, exactly as issue #381's shared-but-well-formed serialNum is -- + heuristics can't see that, which is the whole reason this moves onto a + field the protocol itself has to keep distinct. + """ + s = (value or "").strip() + if not s: + return False + if not set(s) - {"0", "-"}: + return False + return not is_placeholder_serial(s) + + +def ocf_device_key(identity: DeviceIdentity | None) -> str | None: + """The OCF-derived half of resolve_device_key's chain, or None when the + device reported no usable UUID. + + Split out because "the device has no UUID" and "the device has this + UUID" are different answers to a caller holding an existing key: the + coordinator must never demote an entry from a UUID back onto a + serialNum just because one poll couldn't read /oic/d (a reconnect, a + timeout), which is exactly what it would do if it only saw the + collapsed string resolve_device_key returns. + + Normalized because the result is compared against its stored form on + every poll; firmware that changes case between reads would otherwise + look like a different appliance. + """ + if identity is None: + return None + for candidate in (identity.device_id, identity.platform_id): + if candidate is not None and is_usable_device_id(candidate): + return candidate.strip().lower() + return None + + +def resolve_device_key(identity: DeviceIdentity | None, raw_serial: str | None, host: str) -> str: + """The identity to mint this device's permanent registry keys from. + + Tried in order: /oic/d's `di`, /oic/p's `pi`, the serialNum, the host. + + `di` leads because it is the identifier the protocol we are already + speaking uses to address this endpoint -- if it were wrong or shared, + OCF discovery and the DTLS association would not work in the first + place. serialNum, by contrast, is a vendor-populated string no part of + the stack depends on, which is why three separate firmware families + have shipped it unusable: 'Nothing(SVC)' (#83), a flash-unset sentinel + (#189), and -- unfixable by any heuristic -- a well-formed serial + duplicated across every unit of the model (#381). + + `pi` is only the fallback despite the OCF spec calling it immutable: + it identifies the *platform*, so a board hosting more than one logical + OCF device shares one `pi` across all of them, reintroducing the very + collision this exists to prevent. `di` is device-scoped, which is the + granularity of a config entry. + + The serial stays in the chain below both so a board that answers + neither OCF resource lands exactly where it did before this existed, + and the host stays last for the same reason -- it is an address rather + than an identity (a new DHCP lease silently makes it someone else's), + so it is strictly a last resort. + """ + return ocf_device_key(identity) or resolve_serial(raw_serial, host) + + def resolve_model(model_num: str, identity: DeviceIdentity | None) -> str: """The model string to name and register a device under. @@ -143,6 +226,8 @@ def read_identity(sess, serial: str | None) -> DeviceIdentity: model=p.get("mnmo") or "", name=d.get("n") or "", serial=serial, + device_id=d.get("di") if isinstance(d.get("di"), str) else None, + platform_id=p.get("pi") if isinstance(p.get("pi"), str) else None, device_types=_device_types(d), # Kept whole rather than field-by-field: outside the /device/0 dump # diagnostics already captures, and we don't yet know which fields diff --git a/custom_components/localthings/registry/redact.py b/custom_components/localthings/registry/redact.py index 1ed8e47..a4dda45 100644 --- a/custom_components/localthings/registry/redact.py +++ b/custom_components/localthings/registry/redact.py @@ -30,13 +30,24 @@ _SENSITIVE_SUBSTRINGS = ( "secret", ) -# Matched whole, not as substrings: OCF's /oic/d and /oic/p identify the -# unit with bare one/two-letter keys too short for the substring rules above -# ('di' is a substring of 'condition', 'display', ...). 'di'/'pi' are the -# device/platform UUIDs; 'n' is /oic/d's free-text device name, which may -# carry a person's name -- the device-type signal we actually want from -# that resource is `rt`, which is not redacted. -_SENSITIVE_EXACT = frozenset({"di", "pi", "n"}) +# Matched whole, not as substrings: these are bare one/two-letter keys too +# short for the substring rules above ('n' is a substring of very nearly +# everything). 'n' is /oic/d's free-text device name, which the owner sets +# from the SmartThings app and can carry a person's name -- the device-type +# signal we actually want from that resource is `rt`, which is not redacted. +# +# OCF's /oic/d `di` and /oic/p `pi` used to be redacted here too. They are +# not account data: they're randomly-assigned per-unit UUIDs, carrying no +# more about their owner than the appliance-internal subdeviceIdList +# diagnostics already reports for the same reason (see diagnostics.py). +# Redacting them cost more than it bought -- issue #381 was two units +# colliding on a duplicated serialNum, and the first diagnostics download +# asking whether their `di`/`pi` differed came back with both values +# blanked, so the question could only be answered by walking the reporter +# through a manual read_resource call. They are now also what +# resolve_device_key mints registry keys from, so a report that hides them +# hides the identity every entity in it is named after. +_SENSITIVE_EXACT = frozenset({"n"}) def _is_sensitive_key(key: str) -> bool: diff --git a/custom_components/localthings/rekey.py b/custom_components/localthings/rekey.py new file mode 100644 index 0000000..db04631 --- /dev/null +++ b/custom_components/localthings/rekey.py @@ -0,0 +1,110 @@ +"""Move an entry's registry entries from one device key to another. + +The key a device's registry entries are minted from (see +registry.identity.resolve_device_key) appears in three permanent places: +the config entry's unique_id, the device registry's identifiers, and every +entity's unique_id. Changing it therefore can't be a matter of writing a +new value and restarting -- everything already in the registries would be +orphaned, and the user would find a duplicate device whose entities all +carry a `_2` suffix, with their history, area and automations attached to +the dead copy. + +So the key change is performed *on* the registries instead. Rewriting +beats deleting: an entity keeps its entity_id, and with it its name, area, +long-term statistics and every automation and dashboard that references +it. + +Lives in its own module rather than in __init__.py because both callers +need it and they sit on opposite sides of an import edge: the v1 -> v2 +migration in __init__.py, and the coordinator's first-poll adoption of the +OCF device UUID (issue #381), which can only happen once the device has +actually been reached. +""" + +from __future__ import annotations + +import logging + +from homeassistant.config_entries import ConfigEntry +from homeassistant.core import HomeAssistant, callback +from homeassistant.helpers import device_registry as dr +from homeassistant.helpers import entity_registry as er + +from .const import DOMAIN + +_LOGGER = logging.getLogger(__name__) + + +@callback +def rekey_entry(hass: HomeAssistant, entry: ConfigEntry, old_key: str, new_key: str) -> None: + """Rewrite everything this entry registered under `old_key` to `new_key`. + + Covers all three places the key is permanent: the entity registry + (unique_ids are f"{DOMAIN}_{key}_{state_key}"), the device registry + (identifiers are (DOMAIN, key) for the device itself and + (DOMAIN, f"{key}_{subdevice}") for each subdevice of a composite + appliance -- see coordinator.device_info_for), and the config entry's + own unique_id. + + Leaving the entry's unique_id behind would half-migrate it: the config + flow's duplicate check (_abort_if_unique_id_configured) would still be + comparing new devices against the key this entry no longer uses, so + re-adding this very appliance would be waved through as a second entry. + + Idempotent: a second call finds nothing left under `old_key` and does + nothing, which is what makes it safe to attempt on every poll rather + than having to track whether it has already run. + + Where both keys somehow already exist, the `old_key` copy is the dead + one -- unavailable since whichever restart created the split -- so it + is removed rather than rewritten over the live entry. + + Must run on the event loop; the registry helpers require it. + """ + if old_key == new_key: + return + + new_unique_id = f"{DOMAIN}_{new_key}" + if entry.unique_id != new_unique_id: + hass.config_entries.async_update_entry(entry, unique_id=new_unique_id) + + ent_reg = er.async_get(hass) + stale_prefix = f"{DOMAIN}_{old_key}_" + for entity in list(er.async_entries_for_config_entry(ent_reg, entry.entry_id)): + if not entity.unique_id.startswith(stale_prefix): + continue + new_unique_id = f"{DOMAIN}_{new_key}_{entity.unique_id[len(stale_prefix) :]}" + if ent_reg.async_get_entity_id(entity.domain, DOMAIN, new_unique_id): + _LOGGER.debug("removing orphaned entity %s", entity.entity_id) + ent_reg.async_remove(entity.entity_id) + else: + _LOGGER.debug("re-keying entity %s to %s", entity.entity_id, new_unique_id) + ent_reg.async_update_entity(entity.entity_id, new_unique_id=new_unique_id) + + dev_reg = dr.async_get(hass) + for device in list(dr.async_entries_for_config_entry(dev_reg, entry.entry_id)): + stale = { + ident + for ident in device.identifiers + if ident[0] == DOMAIN and (ident[1] == old_key or ident[1].startswith(f"{old_key}_")) + } + if not stale: + continue + fresh = {(DOMAIN, f"{new_key}{ident[1][len(old_key) :]}") for ident in stale} + existing = dev_reg.async_get_device(identifiers=fresh) + if existing is not None and existing.id != device.id: + # Removing a device takes its entities with it. Anything still + # attached here was re-keyed rather than removed above -- the + # surviving copy, not a duplicate -- so move it onto the device + # it now belongs to before the removal destroys it too. + for entity in er.async_entries_for_device( + ent_reg, device.id, include_disabled_entities=True + ): + ent_reg.async_update_entity(entity.entity_id, device_id=existing.id) + _LOGGER.debug("removing orphaned device %s", device.id) + dev_reg.async_remove_device(device.id) + else: + _LOGGER.debug("re-keying device %s to %s", device.id, fresh) + dev_reg.async_update_device( + device.id, new_identifiers=(device.identifiers - stale) | fresh + ) diff --git a/custom_components/localthings/sensor.py b/custom_components/localthings/sensor.py index a7086d5..283afef 100644 --- a/custom_components/localthings/sensor.py +++ b/custom_components/localthings/sensor.py @@ -187,7 +187,7 @@ class LocalThingsConnectionModeSensor(CoordinatorEntity[LocalThingsCoordinator], def __init__(self, coordinator: LocalThingsCoordinator) -> None: super().__init__(coordinator) - self._attr_unique_id = f"{DOMAIN}_{coordinator.device_serial}_connection_mode" + self._attr_unique_id = f"{DOMAIN}_{coordinator.device_key}_connection_mode" @property def device_info(self) -> DeviceInfo: diff --git a/tests/localthings/conftest.py b/tests/localthings/conftest.py index 8fe0f3f..6c682f8 100644 --- a/tests/localthings/conftest.py +++ b/tests/localthings/conftest.py @@ -14,6 +14,7 @@ from pytest_homeassistant_custom_component.common import MockConfigEntry from custom_components.localthings.const import ( CONF_CA_CERT_PEM, CONF_CA_KEY_PEM, + CONF_DEVICE_KEY, CONF_DEVICE_TYPE, CONF_HOST, CONF_LEAF_CERT_PEM, @@ -82,6 +83,10 @@ MOCK_PORT = 49154 # what mock_coordinator_session polls -- so an entry built from ENTRY_DATA and # the device it "reaches" agree on who they are, the same as in production. MOCK_SERIAL = "TEST-SERIAL-0000" +# The OCF device UUID (/oic/d's `di`) the probe resolves the entry's key from +# (issue #381). Distinct from MOCK_SERIAL so a test that confuses the two +# fails rather than passing by coincidence. +MOCK_DEVICE_KEY = "7b1f0c9e-2a44-4d6b-9f10-4c8e2b5a0d31" MOCK_MODEL = "TEST-MODEL" MOCK_DEVICE_TYPE = "refrigerator" MOCK_CA_CERT_PEM = "-----BEGIN CERTIFICATE-----\nTEST-CA\n-----END CERTIFICATE-----" @@ -98,6 +103,7 @@ ENTRY_DATA = { CONF_LEAF_KEY_PEM: MOCK_LEAF_KEY_PEM, # Identity the config flow's probe resolved (issue #236) -- what the # coordinator keys its devices and entities on from construction. + CONF_DEVICE_KEY: MOCK_DEVICE_KEY, CONF_SERIAL: MOCK_SERIAL, CONF_MODEL: MOCK_MODEL, CONF_MANUFACTURER: "Samsung", @@ -131,6 +137,7 @@ def fridge_resources(): def _probe_result(*, recognized: bool) -> dict: return { "port": MOCK_PORT, + "device_key": MOCK_DEVICE_KEY, "serial": MOCK_SERIAL, "model": MOCK_MODEL, "manufacturer": "Samsung", @@ -244,8 +251,8 @@ def mock_entry(hass): entry = MockConfigEntry( domain=DOMAIN, data=ENTRY_DATA, - unique_id=f"localthings_{MOCK_SERIAL}", - version=2, + unique_id=f"localthings_{MOCK_DEVICE_KEY}", + version=4, ) entry.add_to_hass(hass) return entry diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index 0a89c76..afd6596 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -16,11 +16,13 @@ from custom_components.localthings.const import ( CONF_CA_CERT_PEM, CONF_CA_KEY_PEM, CONF_CLOUD_COURSES_ENABLED, + CONF_DEVICE_KEY, CONF_HOST, CONF_LEAF_CERT_PEM, CONF_LEARN_MODES, CONF_LEARNED_MODES, CONF_PORT, + CONF_SERIAL, DOMAIN, ) @@ -28,11 +30,13 @@ from .conftest import ( ENTRY_DATA, MOCK_CA_CERT_PEM, MOCK_CA_KEY_PEM, + MOCK_DEVICE_KEY, MOCK_HOST, MOCK_LEAF_CERT_PEM, MOCK_MODEL, MOCK_PORT, MOCK_SERIAL, + _probe_result, ) @@ -1047,7 +1051,11 @@ async def test_unknown_type_step_description_makes_no_version_claim( async def test_duplicate_device_aborted(hass: HomeAssistant, mock_probe) -> None: - """Second add of same serial: flow aborts. + """Second add of the same *device key*: flow aborts. + + Keyed on the OCF device UUID rather than the serialNum (issue #381), so + this is now the check that two genuinely distinct units can no longer + trip -- see test_same_serial_on_two_units_is_not_a_duplicate. 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. @@ -1055,7 +1063,7 @@ async def test_duplicate_device_aborted(hass: HomeAssistant, mock_probe) -> None existing = MockConfigEntry( domain=DOMAIN, data=ENTRY_DATA, - unique_id=f"localthings_{MOCK_SERIAL}", + unique_id=f"localthings_{MOCK_DEVICE_KEY}", ) existing.add_to_hass(hass) @@ -1069,6 +1077,88 @@ async def test_duplicate_device_aborted(hass: HomeAssistant, mock_probe) -> None assert result["reason"] == "already_configured" +async def test_same_serial_on_two_units_is_not_a_duplicate(hass: HomeAssistant, mock_probe) -> None: + """Issue #381: two Samsung air purifiers of one model ship the identical, + well-formed serialNum, so keying the entry on it turned the second one + away as already configured. The OCF device UUID differs between them, + and it is what the entry is keyed on now, so both can be added. + + Deliberately holds the serial *constant* across the two probes and varies + only the device key -- the exact shape of the bug report. + """ + first = MockConfigEntry( + domain=DOMAIN, + data={**ENTRY_DATA, CONF_HOST: "192.168.0.3"}, + unique_id=f"localthings_{MOCK_DEVICE_KEY}", + ) + first.add_to_hass(hass) + + second_probe = { + **_probe_result(recognized=True), + "device_key": "3771f8bf-c184-3a2d-d885-e4c9818736d2", + "serial": MOCK_SERIAL, + } + with patch( + "custom_components.localthings.config_flow._probe_and_validate", + return_value=second_probe, + ): + result = await hass.config_entries.flow.async_init(DOMAIN, context={"source": "user"}) + result = await hass.config_entries.flow.async_configure( + result["flow_id"], {CONF_HOST: "192.168.0.14"} + ) + + assert result["type"] == FlowResultType.CREATE_ENTRY + assert result["data"][CONF_DEVICE_KEY] == "3771f8bf-c184-3a2d-d885-e4c9818736d2" + # Both entries exist, and the shared serial is still recorded on each -- + # it is what corroborates a later change of key. + assert len(hass.config_entries.async_entries(DOMAIN)) == 2 + assert result["data"][CONF_SERIAL] == first.data[CONF_SERIAL] + + +def test_probe_reads_the_device_key_from_oic_d_without_an_extra_round_trip(monkeypatch): + """`_read_device` already fetches /oic/p and /oic/d for the device-type + signal, so keying on the OCF UUID costs no additional GET -- it reads + the identity that call already returned.""" + from custom_components.localthings.config_flow import _read_device + + device0 = [ + {"rt": ["x.com.samsung.devcol"]}, + { + "href": "/information/vs/0", + "rep": { + "x.com.samsung.da.modelNum": "AVT-WW-TP1-23-AXX500|10251941", + "x.com.samsung.da.serialNum": "BS7SP9AW400114A", + }, + }, + ] + + class _Session: + def __init__(self): + self.paths = [] + + def get(self, path, timeout=10.0): + self.paths.append(tuple(path)) + table = { + ("oic", "p"): {"mnmn": "Samsung Electronics", "pi": "PLATFORM-UUID"}, + ("oic", "d"): {"di": "CCFD73B3-AEB4-792A-1100-68F06F5D603B"}, + ("device", "0"): device0, + } + body = table.get(tuple(path)) + if body is None: + return 0x84, b"" + import cbor2 + + return 0x45, cbor2.dumps(body) + + sess = _Session() + info = _read_device(sess, "192.168.0.3", MOCK_PORT) + + assert info["device_key"] == "ccfd73b3-aeb4-792a-1100-68f06f5d603b" + assert info["serial"] == "BS7SP9AW400114A" + # Exactly the three reads the probe already made before this change. + assert sess.paths == [("oic", "p"), ("oic", "d"), ("oic", "res"), ("device", "0")] + + def test_probe_marks_washer_as_recognized(monkeypatch): """A washer reports no oneUiVersion at all -- its consumer-model code must still resolve so setup doesn't warn about an unrecognized type.""" diff --git a/tests/localthings/test_coordinator.py b/tests/localthings/test_coordinator.py index 2429e3c..ed1844e 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -33,7 +33,13 @@ from custom_components.localthings.registry.capabilities.common import ( remote_control_required_for_write, ) -from .conftest import ENTRY_DATA, MOCK_MODEL, MOCK_SERIAL, FakeObserveSession +from .conftest import ( + ENTRY_DATA, + MOCK_DEVICE_KEY, + MOCK_MODEL, + MOCK_SERIAL, + FakeObserveSession, +) from .conftest import _load_fridge_resources as _load_fridge @@ -150,7 +156,7 @@ def test_run_discovery_falls_back_to_host_for_placeholder_serial( ) -> None: """Issue #83: the ARTIK051_DONGLE_REF firmware family reports the literal string 'Nothing(SVC)' as serialNum on every unit. Left as-is, - two such units get the same device_serial (which feeds both the HA + two such units get the same device_key (which feeds both the HA device-registry identifier and every entity's unique_id), so the second one's entities silently collide and get dropped. It must be treated the same as an empty serial and fall back to the host.""" @@ -164,7 +170,7 @@ def test_run_discovery_falls_back_to_host_for_placeholder_serial( } coordinator = LocalThingsCoordinator(hass, legacy_entry) coordinator._run_discovery(resources) - assert coordinator.device_serial == legacy_entry.data[CONF_HOST] + assert coordinator.device_key == legacy_entry.data[CONF_HOST] def test_run_discovery_falls_back_to_host_for_all_f_placeholder_serial( @@ -175,7 +181,7 @@ def test_run_discovery_falls_back_to_host_for_all_f_placeholder_serial( character the same repeated hex digit. A washer and a dryer, two different physical units, both reported the literal serialNum 'FFFFFFFFFFFFFFF', so without this fallback they'd collide on - device_serial exactly like the #83 case above.""" + device_key exactly like the #83 case above.""" resources = { "/information/vs/0": { "x.com.samsung.da.modelNum": "DA_WM_A51_20_COMMON|20221341|30010102001211000103000000000000", # noqa: E501 @@ -186,7 +192,7 @@ def test_run_discovery_falls_back_to_host_for_all_f_placeholder_serial( } coordinator = LocalThingsCoordinator(hass, legacy_entry) coordinator._run_discovery(resources) - assert coordinator.device_serial == legacy_entry.data[CONF_HOST] + assert coordinator.device_key == legacy_entry.data[CONF_HOST] # --------------------------------------------------------------------------- @@ -198,7 +204,7 @@ def test_identity_is_resolved_before_any_poll(hass: HomeAssistant, mock_entry) - """The coordinator mints registry keys from the entry's stored identity at construction time. - `device_serial` is what entity unique_ids and device identifiers are built + `device_key` is what entity unique_ids and device identifiers are built from, and those are permanent. Seeding it with the host meant anything that registered before the first poll returned -- the connection-mode sensor especially, added unconditionally rather than from `bound` -- was written @@ -207,8 +213,8 @@ def test_identity_is_resolved_before_any_poll(hass: HomeAssistant, mock_entry) - """ coordinator = LocalThingsCoordinator(hass, mock_entry) - assert coordinator.device_serial == MOCK_SERIAL - assert coordinator.device_info["identifiers"] == {(DOMAIN, MOCK_SERIAL)} + assert coordinator.device_key == MOCK_DEVICE_KEY + assert coordinator.device_info["identifiers"] == {(DOMAIN, MOCK_DEVICE_KEY)} assert coordinator.device_info["model"] == MOCK_MODEL assert coordinator.device_info["name"] == f"Samsung Refrigerator ({MOCK_MODEL})" assert mock_entry.data[CONF_HOST] not in str(coordinator.device_info["identifiers"]) @@ -239,7 +245,7 @@ def test_discovery_keeps_the_registered_identity(hass: HomeAssistant, mock_entry coordinator = LocalThingsCoordinator(hass, mock_entry) coordinator._run_discovery(resources) - assert coordinator.device_serial == MOCK_SERIAL + assert coordinator.device_key == MOCK_DEVICE_KEY def test_discovery_backfills_a_legacy_entry_identity(hass: HomeAssistant, legacy_entry) -> None: diff --git a/tests/localthings/test_device_removal.py b/tests/localthings/test_device_removal.py index 8affe32..27d6fdb 100644 --- a/tests/localthings/test_device_removal.py +++ b/tests/localthings/test_device_removal.py @@ -39,7 +39,7 @@ async def test_stale_device_can_be_removed( stale = _device( hass, mock_entry, - {(DOMAIN, f"{coordinator.device_serial}_1")}, + {(DOMAIN, f"{coordinator.device_key}_1")}, ) assert await async_remove_config_entry_device(hass, mock_entry, stale) is True diff --git a/tests/localthings/test_diagnostics.py b/tests/localthings/test_diagnostics.py index 677813b..e35a351 100644 --- a/tests/localthings/test_diagnostics.py +++ b/tests/localthings/test_diagnostics.py @@ -76,8 +76,11 @@ async def test_diagnostics_include_ocf_identity( # fields identify a device type, so nothing is dropped up front beyond # what redaction takes out. assert identity["resources"]["/oic/p"]["mnmn"] == "Samsung Electronics" - assert identity["resources"]["/oic/d"]["di"] == REDACTED - assert identity["resources"]["/oic/p"]["pi"] == REDACTED + # The OCF UUIDs are reported rather than redacted: they are what this + # entry is keyed on (issue #381), and blanking them is what made the + # first duplicate-serial report unanswerable. + assert identity["resources"]["/oic/d"]["di"] == "ab-cd-ef" + assert identity["resources"]["/oic/p"]["pi"] == "12-34-56" # The owner-settable device name is redacted; `rt` -- the reason this # block exists -- is not. assert identity["resources"]["/oic/d"]["n"] == REDACTED @@ -125,9 +128,9 @@ async def test_diagnostics_include_oic_res_links( links = diag["identity"]["resources"]["/oic/res"] assert len(links) == 2 - assert links[0]["di"] == REDACTED + assert links[0]["di"] == "aaaa-1111" assert links[0]["href"] == "/device/0" - assert links[1]["di"] == REDACTED + assert links[1]["di"] == "bbbb-2222" assert links[1]["href"] == "/device/1" assert links[1]["rt"] == ["x.com.samsung.devcol", "oic.wk.col"] diff --git a/tests/localthings/test_migration.py b/tests/localthings/test_migration.py index 49a9152..8c4788a 100644 --- a/tests/localthings/test_migration.py +++ b/tests/localthings/test_migration.py @@ -1,13 +1,17 @@ -"""Config-entry migration and the placeholder-identity repair (issue #236).""" +"""Config-entry migration, the placeholder-identity repair (issue #236), and +the move onto the OCF device UUID (issue #381).""" from __future__ import annotations +from unittest.mock import patch + from homeassistant.core import HomeAssistant from homeassistant.helpers import device_registry as dr from homeassistant.helpers import entity_registry as er from pytest_homeassistant_custom_component.common import MockConfigEntry from custom_components.localthings.const import ( + CONF_DEVICE_KEY, CONF_HOST, CONF_SERIAL, DOMAIN, @@ -39,36 +43,37 @@ async def test_migration_recovers_serial_from_unique_id( await hass.async_block_till_done() # Straight through to the current version: v2 -> v3 is a statistics - # relabel that no-ops for a family without particulate sensors. - assert entry.version == 3 + # relabel that no-ops for a family without particulate sensors, and + # v3 -> v4 only records the legacy key for the coordinator to re-key + # from once a poll produces an OCF device id. + assert entry.version == 4 assert entry.data[CONF_SERIAL] == MOCK_SERIAL -async def test_migration_collapses_the_host_port_unique_id( - hass: HomeAssistant, mock_coordinator_session -) -> None: +async def test_migration_collapses_the_host_port_unique_id(hass: HomeAssistant) -> None: """A board with no usable serial (issues #83/#189) used to be keyed two different ways at once: `host:port` on the config entry, `host` in the device and entity registries. Migration collapses the entry onto the registry's form, so the two finally name the same thing.""" + from custom_components.localthings import async_migrate_entry + entry = _legacy_entry(hass, f"{DOMAIN}_{MOCK_HOST}:{MOCK_PORT}") - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() + assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == MOCK_HOST assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" -async def test_migration_resolves_a_placeholder_serial_unique_id( - hass: HomeAssistant, mock_coordinator_session -) -> None: +async def test_migration_resolves_a_placeholder_serial_unique_id(hass: HomeAssistant) -> None: """An entry created before the placeholder rules landed was keyed on the placeholder itself (issues #83/#189), while the coordinator has been resolving those boards to the host ever since. The unique_id records what the flow believed then, not what the registry holds -- taking it at face value would re-key working devices back onto a string every unit of the family reports, which is the collision those issues are about.""" + from custom_components.localthings import async_migrate_entry + entry = _legacy_entry(hass, f"{DOMAIN}_Nothing(SVC)") dev_reg = dr.async_get(hass) device = dev_reg.async_get_or_create( @@ -76,8 +81,7 @@ async def test_migration_resolves_a_placeholder_serial_unique_id( identifiers={(DOMAIN, MOCK_HOST)}, ) - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() + assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == MOCK_HOST assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" @@ -86,14 +90,13 @@ async def test_migration_resolves_a_placeholder_serial_unique_id( assert unchanged.identifiers == {(DOMAIN, MOCK_HOST)} -async def test_migration_resolves_an_all_hex_placeholder_unique_id( - hass: HomeAssistant, mock_coordinator_session -) -> None: +async def test_migration_resolves_an_all_hex_placeholder_unique_id(hass: HomeAssistant) -> None: """The issue #189 flash-unset sentinel, same reasoning.""" + from custom_components.localthings import async_migrate_entry + entry = _legacy_entry(hass, f"{DOMAIN}_FFFFFFFFFFFFFFF") - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() + assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == MOCK_HOST @@ -231,13 +234,13 @@ async def test_migration_removes_an_orphan_that_is_already_duplicated( assert dev_reg.async_get(real_device.id) is not None -async def test_migration_leaves_a_host_identity_device_alone( - hass: HomeAssistant, mock_coordinator_session -) -> None: +async def test_migration_leaves_a_host_identity_device_alone(hass: HomeAssistant) -> None: """A board whose serial resolves *to* the host was never keyed on a placeholder -- its host-keyed device is the real one, and re-keying or removing it would orphan a working device to fix a problem it doesn't have.""" + from custom_components.localthings import async_migrate_entry + entry = _legacy_entry(hass, f"{DOMAIN}_{MOCK_HOST}") dev_reg = dr.async_get(hass) device = dev_reg.async_get_or_create( @@ -245,8 +248,7 @@ async def test_migration_leaves_a_host_identity_device_alone( identifiers={(DOMAIN, MOCK_HOST)}, ) - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() + assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == MOCK_HOST unchanged = dev_reg.async_get(device.id) @@ -259,7 +261,7 @@ async def test_migration_rejects_a_future_entry_version(hass: HomeAssistant) -> written by a newer release.""" from custom_components.localthings import async_migrate_entry - entry = MockConfigEntry(domain=DOMAIN, data=LEGACY_ENTRY_DATA, version=4) + entry = MockConfigEntry(domain=DOMAIN, data=LEGACY_ENTRY_DATA, version=5) entry.add_to_hass(hass) assert await async_migrate_entry(hass, entry) is False @@ -277,3 +279,312 @@ async def test_migration_without_a_unique_id_falls_back_to_host(hass: HomeAssist assert await async_migrate_entry(hass, entry) is True assert entry.data[CONF_SERIAL] == entry.data[CONF_HOST] assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" + + +# --------------------------------------------------------------------------- +# v3 -> v4: onto the OCF device UUID (issue #381) +# --------------------------------------------------------------------------- + +# The two purifiers from issue #381: one serialNum, two device UUIDs. +SHARED_SERIAL = "BS7SP9AW400114A" +UUID_A = "ccfd73b3-aeb4-792a-1100-68f06f5d603b" +UUID_B = "3771f8bf-c184-3a2d-d885-e4c9818736d2" + + +def _identity_reporting(device_id: str | None): + """A patched _connect_session that hands the coordinator the identity a + real DTLS connect would have read off /oic/p and /oic/d.""" + from custom_components.localthings.registry.identity import DeviceIdentity + + def _connect(self): + self._identity = ( + None + if device_id is None + else DeviceIdentity( + manufacturer="Samsung Electronics", + model="AVT-WW-TP1-23-AXX500", + name="Samsung AirPurifier", + serial=None, + device_id=device_id, + ) + ) + + return patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._connect_session", + _connect, + ) + + +def _v3_entry(hass: HomeAssistant, key: str, *, serial: str | None = None) -> MockConfigEntry: + """An entry as it sits on disk before this release: keyed on CONF_SERIAL, + with no CONF_DEVICE_KEY.""" + data = {**LEGACY_ENTRY_DATA, CONF_SERIAL: serial if serial is not None else key} + entry = MockConfigEntry(domain=DOMAIN, data=data, unique_id=f"{DOMAIN}_{key}", version=3) + entry.add_to_hass(hass) + return entry + + +async def test_v3_entry_moves_onto_the_device_uuid_keeping_its_entity_ids( + hass: HomeAssistant, fridge_resources +) -> None: + """The whole migration promise for an existing user, asserted end to end. + + The re-key happens on the first live poll rather than in + async_migrate_entry, because the UUID is only readable from the device + and an entry can load entirely from its snapshot while the appliance is + off (issue #295). What the user must keep across it: the entity_id (and + with it the entity's name, area, long-term statistics, and every + automation and dashboard that references it), and the device row. + """ + entry = _v3_entry(hass, MOCK_SERIAL) + dev_reg = dr.async_get(hass) + ent_reg = er.async_get(hass) + + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_SERIAL)}, + ) + existing = ent_reg.async_get_or_create( + "sensor", + DOMAIN, + f"{DOMAIN}_{MOCK_SERIAL}_connection_mode", + config_entry=entry, + device_id=device.id, + suggested_object_id="kitchen_purifier_connection", + ) + + with ( + _identity_reporting(UUID_A), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.version == 4 + assert entry.data[CONF_DEVICE_KEY] == UUID_A + # The serial is kept alongside the key, not replaced by it: it is what + # corroborates a later change of UUID. + assert entry.data[CONF_SERIAL] == MOCK_SERIAL + # All three permanent places moved together. + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" + rekeyed_device = dev_reg.async_get(device.id) + assert rekeyed_device is not None + assert rekeyed_device.identifiers == {(DOMAIN, UUID_A)} + kept = ent_reg.async_get(existing.entity_id) + assert kept is not None + assert kept.entity_id == "sensor.kitchen_purifier_connection" + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + # And nothing is left behind on the old key. + assert dev_reg.async_get_device(identifiers={(DOMAIN, MOCK_SERIAL)}) is None + + +async def test_a_host_keyed_entry_adopts_a_real_identity( + hass: HomeAssistant, fridge_resources +) -> None: + """A placeholder-serial board (issues #83/#189) was keyed on its IP, which + is an address rather than an identity -- a new DHCP lease silently makes + it someone else's. Such an entry never made an identity claim to defend, + so a real UUID is adopted without needing the serial to corroborate it; + requiring corroboration would strand exactly these boards, since their + serial resolves to the host and can never match.""" + entry = _v3_entry(hass, MOCK_HOST) + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_HOST)}, + ) + + with ( + _identity_reporting(UUID_B), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_B + rekeyed = dev_reg.async_get(device.id) + assert rekeyed is not None + assert rekeyed.identifiers == {(DOMAIN, UUID_B)} + + +async def test_a_poll_that_reads_no_uuid_does_not_demote_a_keyed_entry( + hass: HomeAssistant, fridge_resources +) -> None: + """The device saying nothing is not the device saying something different. + + A reconnect that can't read /oic/d (a timeout, a firmware hiccup) must + leave the key alone -- demoting back onto the serial would re-key every + entity the user has for the duration of an outage, and re-key them all + back afterwards.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={**LEGACY_ENTRY_DATA, CONF_SERIAL: MOCK_SERIAL, CONF_DEVICE_KEY: UUID_A}, + unique_id=f"{DOMAIN}_{UUID_A}", + version=4, + ) + entry.add_to_hass(hass) + + with ( + _identity_reporting(None), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + coordinator = hass.data[DOMAIN][entry.entry_id] + assert coordinator.device_key == UUID_A + assert entry.data[CONF_DEVICE_KEY] == UUID_A + + +async def test_a_rotated_uuid_on_the_same_serial_is_followed( + hass: HomeAssistant, fridge_resources +) -> None: + """OCF permits a hard factory reset to regenerate `di`. The serialNum is + what tells that apart from a different appliance moving onto the address, + and following it keeps the user's history rather than stranding it on a + UUID the device will never report again.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={**LEGACY_ENTRY_DATA, CONF_SERIAL: MOCK_SERIAL, CONF_DEVICE_KEY: UUID_A}, + unique_id=f"{DOMAIN}_{UUID_A}", + version=4, + ) + entry.add_to_hass(hass) + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, UUID_A)}, + ) + + # fridge_resources reports MOCK_SERIAL, matching what the entry stored. + with ( + _identity_reporting(UUID_B), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_B + rekeyed = dev_reg.async_get(device.id) + assert rekeyed is not None + assert rekeyed.identifiers == {(DOMAIN, UUID_B)} + + +async def test_a_different_appliance_on_the_same_address_keeps_the_registered_identity( + hass: HomeAssistant, fridge_resources +) -> None: + """Neither the UUID nor the serial matches what this entry was registered + with, so this is a different appliance answering at this address -- not a + reset of the registered one. Re-keying here would hand one appliance's + entities, history and automations to another; re-adding is the user's + call.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={ + **LEGACY_ENTRY_DATA, + CONF_SERIAL: "SOME-OTHER-APPLIANCE", + CONF_DEVICE_KEY: UUID_A, + }, + unique_id=f"{DOMAIN}_{UUID_A}", + version=4, + ) + entry.add_to_hass(hass) + dev_reg = dr.async_get(hass) + device = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, UUID_A)}, + ) + + with ( + _identity_reporting(UUID_B), + patch( + "custom_components.localthings.coordinator.LocalThingsCoordinator._poll_once", + return_value=fridge_resources, + ), + patch("custom_components.localthings.coordinator.LocalThingsCoordinator._close_session"), + ): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.data[CONF_DEVICE_KEY] == UUID_A + unchanged = dev_reg.async_get(device.id) + assert unchanged is not None + assert unchanged.identifiers == {(DOMAIN, UUID_A)} + # The rejected appliance's serial is not written either. The serial is + # what corroborates a later change of key, so adopting it here would + # hand the intruder exactly the corroboration it needs to win the *next* + # poll -- defending the identity once and then surrendering it on the + # following cycle. + assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" + + coordinator = hass.data[DOMAIN][entry.entry_id] + coordinator._run_discovery(fridge_resources) + + assert coordinator.device_key == UUID_A + assert entry.data[CONF_DEVICE_KEY] == UUID_A + assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" + + +async def test_rekey_moves_subdevice_identifiers_too(hass: HomeAssistant) -> None: + """A composite appliance (issue #177) registers one device per logical + subdevice, keyed f"{key}_{subdevice}" and linked via_device to the + master's bare key. Rewriting only the exact-match identifier would strand + every sibling, so the prefix form moves with it. + + Calls rekey_entry directly: what it leaves behind is the contract, and + going through a setup would let the platforms re-adding their entities + hide a row that had in fact been orphaned. + """ + from custom_components.localthings.rekey import rekey_entry + + entry = _v3_entry(hass, MOCK_SERIAL) + dev_reg = dr.async_get(hass) + master = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, MOCK_SERIAL)}, + ) + sub = dev_reg.async_get_or_create( + config_entry_id=entry.entry_id, + identifiers={(DOMAIN, f"{MOCK_SERIAL}_subdevice_1")}, + ) + + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + + assert dev_reg.async_get(master.id).identifiers == {(DOMAIN, UUID_A)} + assert dev_reg.async_get(sub.id).identifiers == {(DOMAIN, f"{UUID_A}_subdevice_1")} + + +async def test_rekey_is_idempotent(hass: HomeAssistant) -> None: + """Safe to attempt on every poll rather than having to track whether it + has already run -- a second call finds nothing under the old key.""" + from custom_components.localthings.rekey import rekey_entry + + entry = _v3_entry(hass, MOCK_SERIAL) + ent_reg = er.async_get(hass) + existing = ent_reg.async_get_or_create( + "sensor", DOMAIN, f"{DOMAIN}_{MOCK_SERIAL}_connection_mode", config_entry=entry + ) + + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + rekey_entry(hass, entry, MOCK_SERIAL, UUID_A) + + kept = ent_reg.async_get(existing.entity_id) + assert kept is not None + assert kept.unique_id == f"{DOMAIN}_{UUID_A}_connection_mode" + assert entry.unique_id == f"{DOMAIN}_{UUID_A}" diff --git a/tests/localthings/test_statistics_migration.py b/tests/localthings/test_statistics_migration.py index 27fd7cd..d0da6e7 100644 --- a/tests/localthings/test_statistics_migration.py +++ b/tests/localthings/test_statistics_migration.py @@ -72,7 +72,7 @@ async def test_relabels_every_particulate_sensor(hass: HomeAssistant) -> None: # µg/m³ has a converter, so the class must be named, not None -- # passing neither is deprecated and breaks in HA Core 2026.11. assert call.kwargs["new_unit_class"] == "concentration" - assert entry.version == 3 + assert entry.version == 4 async def test_leaves_other_sensors_on_the_same_device_alone(hass: HomeAssistant) -> None: @@ -102,7 +102,7 @@ async def test_skips_families_that_did_not_gain_the_unit(hass: HomeAssistant) -> assert await async_migrate_entry(hass, entry) is True assert relabel.call_args_list == [], device_type - assert entry.version == 3 + assert entry.version == 4 async def test_defers_rather_than_consuming_the_migration_without_the_recorder( @@ -127,7 +127,7 @@ async def test_defers_rather_than_consuming_the_migration_without_the_recorder( assert await async_migrate_entry(hass, entry) is True assert len(relabel.call_args_list) == 1 - assert entry.version == 3 + assert entry.version == 4 async def test_omits_unit_class_on_an_older_home_assistant(hass: HomeAssistant) -> None: @@ -151,7 +151,7 @@ async def test_omits_unit_class_on_an_older_home_assistant(hass: HomeAssistant) assert await async_migrate_entry(hass, entry) is True assert seen == [{"new_unit_of_measurement": CONCENTRATION_MICROGRAMS_PER_CUBIC_METER}] - assert entry.version == 3 + assert entry.version == 4 async def test_a_relabel_failure_never_fails_the_entry(hass: HomeAssistant) -> None: @@ -165,7 +165,7 @@ async def test_a_relabel_failure_never_fails_the_entry(hass: HomeAssistant) -> N with patch(RELABEL, autospec=True, side_effect=TypeError("older HA signature")): assert await async_migrate_entry(hass, entry) is True - assert entry.version == 3 + assert entry.version == 4 async def test_follows_a_renamed_entity_rather_than_rebuilding_its_id( @@ -214,8 +214,16 @@ async def test_matches_subdevice_prefixed_and_instanced_keys(hass: HomeAssistant async def test_a_fresh_entry_starts_at_the_migrated_version(hass: HomeAssistant) -> None: - """A newly created entry has no statistics to relabel, so the config flow - mints v3 directly rather than walking through the migration.""" + """A newly created entry has nothing either migration step needs to do -- + no statistics to relabel, and the probe already resolved its device key + (issue #381) -- so the config flow mints the current version directly + rather than walking through them. + + Pinned rather than compared to a constant on purpose: the two must be + bumped together, and a migration step added without moving the flow's + VERSION never runs at all, because Home Assistant only calls + async_migrate_entry for an entry *behind* the flow's version. + """ from custom_components.localthings.config_flow import LocalThingsConfigFlow - assert LocalThingsConfigFlow.VERSION == 3 + assert LocalThingsConfigFlow.VERSION == 4 diff --git a/tests/test_air_purifier_airflow_fan.py b/tests/test_air_purifier_airflow_fan.py index b884261..cfab918 100644 --- a/tests/test_air_purifier_airflow_fan.py +++ b/tests/test_air_purifier_airflow_fan.py @@ -12,7 +12,7 @@ from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-AIRFLOW-SERIAL" + device_key = "TEST-AIRFLOW-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_air_purifier_vtww_fan.py b/tests/test_air_purifier_vtww_fan.py index ac17ef3..71d2b44 100644 --- a/tests/test_air_purifier_vtww_fan.py +++ b/tests/test_air_purifier_vtww_fan.py @@ -21,7 +21,7 @@ from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-VTWW-SERIAL" + device_key = "TEST-VTWW-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_airconditioner_artik051_krac.py b/tests/test_airconditioner_artik051_krac.py index 4f887d2..f6487c6 100644 --- a/tests/test_airconditioner_artik051_krac.py +++ b/tests/test_airconditioner_artik051_krac.py @@ -44,7 +44,7 @@ MODEL = "ARTIK051_KRAC_18K|10193441|60010119001111010100000000000000" class _FakeCoordinator: - device_serial = "TEST-KRAC-SERIAL" + device_key = "TEST-KRAC-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_airconditioner_tp1x_rac_01001_fan.py b/tests/test_airconditioner_tp1x_rac_01001_fan.py index 9d53da6..6b44135 100644 --- a/tests/test_airconditioner_tp1x_rac_01001_fan.py +++ b/tests/test_airconditioner_tp1x_rac_01001_fan.py @@ -28,7 +28,7 @@ FIXTURE = "airconditioner_tp1x_rac_01001" class _FakeCoordinator: - device_serial = "TEST-RAC-01001-SERIAL" + device_key = "TEST-RAC-01001-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_climate_ac_modes.py b/tests/test_climate_ac_modes.py index 49c29ef..91fd39c 100644 --- a/tests/test_climate_ac_modes.py +++ b/tests/test_climate_ac_modes.py @@ -104,7 +104,7 @@ def test_fac_bora_wind_strength_codes_fit_the_standard_scale(): from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-FAC-BORA-SERIAL" + device_key = "TEST-FAC-BORA-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_entity_naming.py b/tests/test_entity_naming.py index 74b95ed..7c3ba97 100644 --- a/tests/test_entity_naming.py +++ b/tests/test_entity_naming.py @@ -10,7 +10,7 @@ from custom_components.localthings.registry.entities import BinarySensorDesc class _FakeCoordinator: - device_serial = "TEST-SERIAL" + device_key = "TEST-SERIAL" def __init__(self, last_resources=None): self.last_resources = last_resources or {} diff --git a/tests/test_fridge_capabilities.py b/tests/test_fridge_capabilities.py index 6ad4e4b..267c834 100644 --- a/tests/test_fridge_capabilities.py +++ b/tests/test_fridge_capabilities.py @@ -463,7 +463,7 @@ class TestKimchiZone: ) class _FakeCoordinator: - device_serial = "TEST-SERIAL" + device_key = "TEST-SERIAL" def __init__(self, resources, data): self.last_resources = resources diff --git a/tests/test_identity.py b/tests/test_identity.py index 654e0c7..36d17ea 100644 --- a/tests/test_identity.py +++ b/tests/test_identity.py @@ -1,6 +1,12 @@ import cbor2 -from custom_components.localthings.registry.identity import read_identity +from custom_components.localthings.registry.identity import ( + DeviceIdentity, + is_usable_device_id, + ocf_device_key, + read_identity, + resolve_device_key, +) class FakeSession: @@ -124,3 +130,113 @@ def test_read_identity_tolerates_malformed_oic_res(): _device_types' handling of a malformed /oic/d rt.""" ident = read_identity(FakeSession({("oic", "res"): {"not": "a list"}}), serial=None) assert ident.raw["/oic/res"] == [] + + +# --------------------------------------------------------------------------- +# The device key: which identity field registry keys are minted from (#381) +# --------------------------------------------------------------------------- + + +def _identity(**kwargs) -> DeviceIdentity: + base = {"manufacturer": "Samsung", "model": "M", "name": "N", "serial": None} + return DeviceIdentity(**{**base, **kwargs}) + + +def test_read_identity_captures_the_ocf_uuids_as_named_fields(): + """`di`/`pi` are what resolve_device_key mints keys from, so they are + lifted out of `raw` rather than dug back out of it at every call site.""" + sess = FakeSession( + { + ("oic", "p"): {"pi": "ccfd73b3-aeb4-792a-1100-68f06f5d603b"}, + ("oic", "d"): {"di": "3771f8bf-c184-3a2d-d885-e4c9818736d2"}, + } + ) + ident = read_identity(sess, serial=None) + assert ident.device_id == "3771f8bf-c184-3a2d-d885-e4c9818736d2" + assert ident.platform_id == "ccfd73b3-aeb4-792a-1100-68f06f5d603b" + + +def test_read_identity_ignores_non_string_uuids(): + """Firmware answering with a number or a map must not put a non-string + into a field that goes on to be string-formatted into a unique_id.""" + ident = read_identity(FakeSession({("oic", "d"): {"di": 42}, ("oic", "p"): {"pi": {}}}), None) + assert ident.device_id is None + assert ident.platform_id is None + + +def test_two_units_sharing_a_serial_get_distinct_keys(): + """Issue #381 exactly: two Samsung air purifiers of the same model ship + the identical, well-formed serialNum 'BS7SP9AW400114A', so keying on it + collapsed them onto one identity and the second was refused as already + configured. Their `di` differs, which is what makes them separable.""" + shared_serial = "BS7SP9AW400114A" + first = resolve_device_key( + _identity(device_id="ccfd73b3-aeb4-792a-1100-68f06f5d603b"), shared_serial, "192.168.0.3" + ) + second = resolve_device_key( + _identity(device_id="3771f8bf-c184-3a2d-d885-e4c9818736d2"), shared_serial, "192.168.0.14" + ) + assert first != second + assert shared_serial not in (first, second) + + +def test_platform_id_is_the_fallback_when_oic_d_is_unreadable(): + ident = _identity(device_id=None, platform_id="ccfd73b3-aeb4-792a-1100-68f06f5d603b") + assert resolve_device_key(ident, "REAL-SERIAL", "10.0.0.1") == ( + "ccfd73b3-aeb4-792a-1100-68f06f5d603b" + ) + + +def test_device_id_wins_over_platform_id(): + """`pi` is platform-scoped, so a board hosting more than one logical OCF + device shares it -- the collision this exists to prevent.""" + ident = _identity(device_id="dddddddd-0000-1111-2222-333333333333", platform_id="shared-plat") + assert resolve_device_key(ident, "REAL-SERIAL", "10.0.0.1") == ( + "dddddddd-0000-1111-2222-333333333333" + ) + + +def test_falls_back_to_the_serial_then_the_host(): + """A board that answers neither OCF resource lands exactly where it did + before any of this existed -- no regression for existing hardware.""" + assert resolve_device_key(None, "REAL-SERIAL", "10.0.0.1") == "REAL-SERIAL" + assert resolve_device_key(_identity(), "REAL-SERIAL", "10.0.0.1") == "REAL-SERIAL" + # ...and a placeholder serial still resolves to the host (#83/#189). + assert resolve_device_key(_identity(), "Nothing(SVC)", "10.0.0.1") == "10.0.0.1" + + +def test_the_key_is_case_normalized(): + """The stored key is compared against a freshly polled one on every + poll; firmware that changed case between reads would otherwise look + like a different appliance every time.""" + ident = _identity(device_id="CCFD73B3-AEB4-792A-1100-68F06F5D603B") + assert resolve_device_key(ident, None, "10.0.0.1") == "ccfd73b3-aeb4-792a-1100-68f06f5d603b" + + +def test_the_nil_uuid_is_not_an_identity(): + """OCF's unset UUID is identical on every unit that never had one + assigned -- the #189 failure mode on a new field. Its dashes stop + is_placeholder_serial's repeated-digit rule from seeing it, so it needs + its own check; falling through to the serial is the right answer.""" + ident = _identity(device_id="00000000-0000-0000-0000-000000000000") + assert resolve_device_key(ident, "REAL-SERIAL", "10.0.0.1") == "REAL-SERIAL" + assert not is_usable_device_id("00000000-0000-0000-0000-000000000000") + + +def test_known_junk_disqualifies_a_uuid_the_same_way_it_does_a_serial(): + """A board firmware-flashed with 'Nothing(SVC)' in one identity field is + not a board to trust in another.""" + assert not is_usable_device_id("Nothing(SVC)") + assert not is_usable_device_id("FFFFFFFFFFFFFFF") + assert not is_usable_device_id("") + assert not is_usable_device_id(None) + assert is_usable_device_id("3771f8bf-c184-3a2d-d885-e4c9818736d2") + + +def test_ocf_device_key_reports_absence_rather_than_collapsing_to_the_serial(): + """The coordinator needs "the device said nothing" and "the device said + this" to be different answers, so it never demotes a UUID-keyed entry + onto a serial because one poll couldn't read /oic/d.""" + assert ocf_device_key(None) is None + assert ocf_device_key(_identity(serial="REAL-SERIAL")) is None + assert ocf_device_key(_identity(device_id="abc-123")) == "abc-123" diff --git a/tests/test_range_hood_fan.py b/tests/test_range_hood_fan.py index 4eafe92..d102408 100644 --- a/tests/test_range_hood_fan.py +++ b/tests/test_range_hood_fan.py @@ -11,7 +11,7 @@ from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-HOOD-SERIAL" + device_key = "TEST-HOOD-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {} diff --git a/tests/test_redact.py b/tests/test_redact.py index 733972d..0585e95 100644 --- a/tests/test_redact.py +++ b/tests/test_redact.py @@ -75,9 +75,18 @@ def test_redact_resources_does_not_mutate_input(): assert resources["/information/vs/0"]["x.com.samsung.da.serialNum"] == original_serial -def test_redacts_bare_ocf_identity_keys(): - """/oic/d and /oic/p identify the unit with two-letter keys ('di', 'pi') - that the substring rules can't see.""" +def test_keeps_ocf_identity_uuids_but_redacts_the_owner_set_name(): + """/oic/d's `di` and /oic/p's `pi` survive redaction. + + They are randomly-assigned per-unit UUIDs, not account data, and they + are what the entry's registry keys are minted from (issue #381) -- a + report that blanks them hides the identity every entity in it is named + after, and can't answer the one question a duplicate-serial report + exists to ask: whether two units differ here at all. + + `n` is the opposite case and stays redacted: free text the owner sets + from the SmartThings app, so it can carry a person's name. + """ redacted = redact_resources( { "/oic/d": { @@ -89,12 +98,10 @@ def test_redacts_bare_ocf_identity_keys(): } ) - assert redacted["/oic/d"]["di"] == REDACTED - assert redacted["/oic/p"]["pi"] == REDACTED - # 'n' is free text the owner sets from the SmartThings app, so it can - # carry a person's name -- redacted too. `rt`, the device-type signal - # we actually want out of /oic/d, is not. + assert redacted["/oic/d"]["di"] == "ab-cd-ef" + assert redacted["/oic/p"]["pi"] == "12-34-56" assert redacted["/oic/d"]["n"] == REDACTED + # `rt`, the device-type signal we actually want out of /oic/d, is kept. assert redacted["/oic/d"]["rt"] == ["oic.wk.d", "oic.d.refrigerator"] assert redacted["/oic/p"]["mnmo"] == "RF9000B" diff --git a/tests/test_select_options.py b/tests/test_select_options.py index 1e4ba98..93448d7 100644 --- a/tests/test_select_options.py +++ b/tests/test_select_options.py @@ -18,7 +18,7 @@ from custom_components.localthings.select import LocalThingsSelect class _FakeCoordinator: - device_serial = "TEST-SERIAL" + device_key = "TEST-SERIAL" def __init__(self, last_resources): self.last_resources = last_resources diff --git a/tests/test_sensor_enum_options.py b/tests/test_sensor_enum_options.py index 5c8c652..a98ab9f 100644 --- a/tests/test_sensor_enum_options.py +++ b/tests/test_sensor_enum_options.py @@ -46,7 +46,7 @@ class _FakeConfigEntry: class _FakeCoordinator: def __init__(self): - self.device_serial = "TEST-SERIAL" + self.device_key = "TEST-SERIAL" self.config_entry = _FakeConfigEntry() self.resources: dict[str, dict] = {} diff --git a/tests/test_sensor_hysteresis.py b/tests/test_sensor_hysteresis.py index e4b5794..eecfe60 100644 --- a/tests/test_sensor_hysteresis.py +++ b/tests/test_sensor_hysteresis.py @@ -23,7 +23,7 @@ class _FakeCoordinator: """Just enough surface for LocalThingsEntity/LocalThingsSensor.""" def __init__(self, threshold_minutes): - self.device_serial = "TEST-SERIAL" + self.device_key = "TEST-SERIAL" self.config_entry = _FakeConfigEntry( { CONF_FINISH_TIME_HYSTERESIS_MINUTES: threshold_minutes, diff --git a/tests/test_sensor_sticky.py b/tests/test_sensor_sticky.py index 9790edf..207af1d 100644 --- a/tests/test_sensor_sticky.py +++ b/tests/test_sensor_sticky.py @@ -50,7 +50,7 @@ class _FakeCoordinator: """ def __init__(self): - self.device_serial = "TEST-SERIAL" + self.device_key = "TEST-SERIAL" self.config_entry = _FakeConfigEntry() self.resources: dict[str, dict] = {} diff --git a/tests/test_subdevice_discovery.py b/tests/test_subdevice_discovery.py index a8dcb6f..01b320c 100644 --- a/tests/test_subdevice_discovery.py +++ b/tests/test_subdevice_discovery.py @@ -147,7 +147,7 @@ async def test_pattern_a_sub1_device_info_links_via_device_to_master(hass: HomeA sub1 = next(su for su in coordinator.subdevices if su.key == "1") info = coordinator.device_info_for(sub1) - master_serial = coordinator.device_serial + master_serial = coordinator.device_key assert info["identifiers"] == {(DOMAIN, f"{master_serial}_1")} assert info["via_device"] == (DOMAIN, master_serial) # The subdevice's own /information/vs/1 (real, ARTIK051_DONGLE_FAC_RAC_18K) @@ -245,7 +245,7 @@ async def test_fac_bora_2in1_subdevice_device_info(hass: HomeAssistant): subdevice = coordinator.subdevices[0] info = coordinator.device_info_for(subdevice) - master_serial = coordinator.device_serial + master_serial = coordinator.device_key assert info["identifiers"] == {(DOMAIN, f"{master_serial}_{_SUB_UUID}")} assert info["via_device"] == (DOMAIN, master_serial) # Confirmed live by the reporter (DESIGN-177.md section 1): the wall @@ -266,7 +266,7 @@ async def test_fac_bora_2in1_unique_ids_include_subdevice_prefix(hass: HomeAssis entity = LocalThingsEntity(coordinator, sub_climate) expected_slug = _SUB_UUID.replace("-", "") assert entity._attr_unique_id == ( - f"{DOMAIN}_{coordinator.device_serial}_subdevice_{expected_slug}_climate" + f"{DOMAIN}_{coordinator.device_key}_subdevice_{expected_slug}_climate" ) diff --git a/tests/test_water_heater_ehs.py b/tests/test_water_heater_ehs.py index d3dc935..5b0615b 100644 --- a/tests/test_water_heater_ehs.py +++ b/tests/test_water_heater_ehs.py @@ -20,7 +20,7 @@ from tests.conftest import _load_device class _FakeCoordinator: - device_serial = "TEST-EHS-SERIAL" + device_key = "TEST-EHS-SERIAL" device_info: ClassVar[dict] = {} data: ClassVar[dict] = {}