diff --git a/custom_components/localthings/const.py b/custom_components/localthings/const.py index 67d3c44..d5a0607 100644 --- a/custom_components/localthings/const.py +++ b/custom_components/localthings/const.py @@ -30,15 +30,10 @@ 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. +# What this entry's devices and entities are keyed on -- normally the OCF +# device UUID (issue #381). CONF_SERIAL stays alongside it as the pre-v4 +# key to re-key from, and as what corroborates a later change of UUID. +# Absent until the first live poll, since only the device can report it. CONF_DEVICE_KEY = "device_key" CONF_MODEL = "model" CONF_MANUFACTURER = "manufacturer" diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 4a7dd09..a7da859 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -316,14 +316,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # 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. - # - # 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. + # Ordered so an entry that has not polled since upgrading still + # loads under the key its registry rows already carry: the v4 UUID + # (issue #381), else the pre-v4 serial, else the host (#83/#189). self.device_key = ( entry.data.get(CONF_DEVICE_KEY) or entry.data.get(CONF_SERIAL) or entry.data[CONF_HOST] ) @@ -1107,44 +1102,25 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): """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. + Key and serial are returned together because they have to agree: the + serial corroborates a later change of key, so adopting it while + *defending* the stored key would hand a different appliance the + corroboration it needs to win the next poll. - 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. + Nothing changes a key without calling `rekey_entry` in the same + breath, or the existing registry rows are orphaned (issue #236). + Two things are therefore never treated as a change of identity: a + snapshot replay (issue #295), which never reached the device, and a + poll that read no UUID, since the device saying nothing is not the + device saying something different. - 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. - * The polled identity matches what this entry is already keyed on -- - the overwhelmingly common case, and a no-op beyond recording it. - * 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. - * The identity differs. Either this is the registered appliance - under a new identity -- the one-time move onto the OCF device UUID - (issue #381), or a `di` regenerated by an OCF hard reset -- or a - *different* appliance now answers on this address. The serialNum - is what tells those apart. - - That last decision is deliberately the same whether or not this - entry has adopted a UUID yet. A pre-v4 entry is the population this - change exists to move, but it is also the population that has been - running longest, so it is the last one that should lose its - registry rows to an appliance that merely happens to share its - address -- a guard `_run_discovery` has had since issue #236 and - which an unconditional "adopt whatever the device says" would have - quietly dropped for exactly those users. + A genuine difference is either the registered appliance under a new + identity -- the move onto the OCF device UUID (issue #381), or a + `di` regenerated by a hard reset -- or a different appliance on this + address, and the serialNum is what tells them apart. That test is + deliberately the same before and after an entry has adopted a UUID: + a pre-v4 entry has been running longest, so it is the last one that + should lose its rows to whatever now answers at its address. """ host = self._entry.data[CONF_HOST] stored_serial = self._entry.data.get(CONF_SERIAL) @@ -1153,40 +1129,28 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): polled_ocf = ocf_device_key(self._identity) stored_key = self._entry.data.get(CONF_DEVICE_KEY) - # What this entry's registry rows actually carry today. A pre-v4 - # entry has no CONF_DEVICE_KEY, so it is whatever the coordinator - # has always fallen back to -- see this class's __init__. + # What this entry's rows carry today: a pre-v4 entry has no + # CONF_DEVICE_KEY, so it is whatever __init__ fell back to. current_key = stored_key if stored_key is not None else (stored_serial or host) - # `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 if polled_key == current_key: # Returned rather than short-circuited so a pre-v4 entry records - # the key it has always had, which is what stops the next poll - # from treating this same answer as a change. + # the key it has always had, and the next poll sees no change. return current_key, polled_serial if stored_key is not None and 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 - # 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. + # Excluding the host answer keeps this from firing on two *different* + # placeholder-serial units: an 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 identity 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. An - # entry with no stored serial at all has nothing to corroborate - # against either, and is treated the same way. + # A host-keyed entry (issues #83/#189) is registered against whatever + # answers at this address, so it has no claim to defend and needs no + # corroboration -- demanding it would strand exactly the + # placeholder-serial boards this exists to rescue. Same for an entry + # with no stored serial to compare against. if not (current_key == host or same_unit or stored_serial is None): self._log.warning( "device at %s identifies as %r but this entry is registered as %r " @@ -1200,10 +1164,8 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): return current_key, stored_serial or polled_serial # Corroborated: a pre-v4 entry moving onto its UUID, the same unit - # with a regenerated `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. + # with a regenerated `di`, or a host-keyed entry learning a real + # identity. Following it keeps the user's entity_ids and history. self._log.info( "device %s (serial %r) changed key from %r to %r; following it", host, @@ -1499,13 +1461,10 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): manufacturer=mfr, model=model, ) - # 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_identity 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. + # A snapshot replay passes None: it never reached the device, so + # writing a key here would freeze a pre-v4 entry's legacy key in as + # if a poll had confirmed it, and the real UUID would later look + # like an identity to defend against rather than one to adopt. 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 diff --git a/custom_components/localthings/registry/identity.py b/custom_components/localthings/registry/identity.py index c9b5dcc..94181a3 100644 --- a/custom_components/localthings/registry/identity.py +++ b/custom_components/localthings/registry/identity.py @@ -66,19 +66,12 @@ def resolve_serial(raw_serial: str | None, host: str) -> str: 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. + Rejects OCF's nil UUID, which firmware that never had one assigned + reports on every unit of the family -- the #189 failure mode on a new + field, and one `is_placeholder_serial`'s repeated-digit rule misses + because the dashes make more than one distinct character. Past that the + same known-junk rules apply: a board flashed with 'Nothing(SVC)' in one + identity field is not one to trust in another. """ s = (value or "").strip() if not s: @@ -92,16 +85,11 @@ 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. + Split out because "no UUID" and "this UUID" are different answers to a + caller holding an existing key: the coordinator must never demote an + entry off its UUID just because one poll couldn't read /oic/d. + Normalized so firmware that changes case between reads doesn't look + like a different appliance. """ if identity is None: return None @@ -116,26 +104,18 @@ def resolve_device_key(identity: DeviceIdentity | None, raw_serial: str | None, 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). + `di` leads because it is what the protocol already uses to address this + endpoint: if it were wrong or shared, OCF discovery and the DTLS + association would not work at all. serialNum is a vendor-populated + string nothing depends on, which is why three firmware families have + shipped it unusable -- 'Nothing(SVC)' (#83), a flash-unset sentinel + (#189), and a well-formed serial duplicated across every unit (#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. + `pi` is only the fallback despite the spec calling it immutable: it is + *platform*-scoped, so a board hosting several logical OCF devices + shares one across all of them. `di` is device-scoped, the granularity + of a config entry. The serial and host stay below both so a board + answering neither resource lands where it always did. """ return ocf_device_key(identity) or resolve_serial(raw_serial, host) diff --git a/custom_components/localthings/registry/redact.py b/custom_components/localthings/registry/redact.py index a4dda45..c65b3de 100644 --- a/custom_components/localthings/registry/redact.py +++ b/custom_components/localthings/registry/redact.py @@ -36,17 +36,11 @@ _SENSITIVE_SUBSTRINGS = ( # 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. +# /oic/d's `di` and /oic/p's `pi` are deliberately not redacted: they're +# randomly-assigned per-unit UUIDs rather than account data, and they are +# what registry keys are minted from (issue #381), so blanking them hides +# the identity every entity in a report is named after -- which is exactly +# what made #381's first diagnostics download unable to answer it. _SENSITIVE_EXACT = frozenset({"n"}) diff --git a/custom_components/localthings/rekey.py b/custom_components/localthings/rekey.py index 415b878..7b7a51e 100644 --- a/custom_components/localthings/rekey.py +++ b/custom_components/localthings/rekey.py @@ -1,24 +1,13 @@ """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. +The key (registry.identity.resolve_device_key) is permanent in three +places, so changing it means rewriting the registries rather than storing +a new value -- anything left behind is orphaned. Rewriting rather than +recreating is what keeps an entity's entity_id, and with it its history, +area and automations. -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. +Its own module because both callers need it: the v1 -> v2 migration in +__init__.py and the coordinator's first-poll adoption (issue #381). """ from __future__ import annotations @@ -39,25 +28,16 @@ _LOGGER = logging.getLogger(__name__) 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. + All three permanent places move together: entity unique_ids + (f"{DOMAIN}_{key}_{state_key}"), device identifiers ((DOMAIN, key), plus + (DOMAIN, f"{key}_{subdevice}") per subdevice -- see device_info_for), + and the entry's own unique_id. Leaving that last one behind would let + the config flow's duplicate check wave through a re-add of this very + appliance. - 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. + Idempotent, so it is safe to attempt on every poll rather than tracking + whether it has run. Where both keys already exist the `old_key` copy is + the dead one, so it is removed rather than rewritten over the live entry. Must run on the event loop; the registry helpers require it. """ diff --git a/tests/localthings/test_identity_migration.py b/tests/localthings/test_identity_migration.py index 5d3710c..6592b44 100644 --- a/tests/localthings/test_identity_migration.py +++ b/tests/localthings/test_identity_migration.py @@ -1,24 +1,16 @@ """Moving an existing install onto the OCF device UUID (issue #381). -Every other migration this integration has done finishes inside -`async_migrate_entry`. This one can't: the UUID is only readable from the -appliance, and an entry can load entirely from its stored snapshot while -the appliance is off (issue #295). So the config-entry version bumps up -front and the coordinator adopts the UUID on the first *live* poll, -rewriting the entity registry, the device registry and the entry's own +This migration can't finish inside `async_migrate_entry` -- the UUID is +only readable from the appliance, and an entry can load from its snapshot +while that appliance is off (issue #295) -- so the coordinator adopts it +on the first live poll, rewriting both registries and the entry's unique_id together. -That makes this the riskiest migration in the codebase: it rewrites the -identity of registry rows a user's automations, dashboards, history and -areas all hang off. The suite is therefore organised around what an -existing user must not lose, rather than around the functions involved: - -* the entity_id, and every customization carried on that registry row -* long-term statistics (keyed by entity_id -- see the end-to-end test in - tests/test_rekey_statistics_end_to_end.py, which drives a real recorder) -* the device row, its area, and a composite appliance's subdevice links -* other config entries' rows, which this must never touch -* stability: an upgrade that happens twice, or offline, must not churn +That makes it the riskiest migration here: it rewrites the identity of +rows a user's automations, history and areas hang off. These tests are +organised around what must not break rather than around the functions +involved. Statistics are covered separately, against a real recorder, in +tests/test_rekey_statistics_end_to_end.py. """ from __future__ import annotations