From 6e5fee32b8f3e45bd84e4a14e5a12960204647f3 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Thu, 23 Jul 2026 02:14:40 +0000 Subject: [PATCH 1/2] feat: reject writes when a device's remote control is disabled Devices with a /remotectrl href already surface it as a read-only "Smart Control" binary sensor, but writes weren't checking it before now. async_send_command now blocks every write (any platform) with a ServiceValidationError telling the user to enable remote control via the appliance's manual, ahead of any per-description validate_fn. --- custom_components/localthings/coordinator.py | 29 +++- tests/localthings/test_coordinator.py | 138 ++++++++++++++++++- 2 files changed, 164 insertions(+), 3 deletions(-) diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 39c401b..359a598 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -37,6 +37,26 @@ _LOGGER = logging.getLogger(__name__) _SEED_PATH = ['device', '0'] +_REMOTE_CONTROL_DISABLED_MESSAGE = ( + "Remote control is turned off on this device. Check your appliance's " + "manual for how to enable remote control before Home Assistant can " + "control it." +) + + +def _remote_control_disabled(resources: dict) -> bool: + """True only when the device explicitly reports remote control off. + Mirrors REMOTE_CONTROL_GENERIC/_VS_FALLBACK's href/field pair in + registry/capabilities/common.py. Fails open (False) when neither href + is present -- most device types don't report this capability at all.""" + generic = resources.get('/remotectrl/0') + if generic is not None: + return not bool(generic.get('value')) + fallback = resources.get('/remotectrl/vs/0') + if fallback is not None: + return str(fallback.get('x.com.samsung.da.remoteControlEnabled')).lower() != 'true' + return False + class _NoOpDescriptor: """StateCache requires a descriptor with an on_observation hook. This @@ -515,16 +535,21 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): A description-level validate_fn (currently SwitchDesc only) runs here rather than per-platform, so rejecting a write with a user-facing message -- as opposed to write_fn's silent no-op below - -- is available to every platform for free.""" + -- is available to every platform for free. The remote-control + check runs first and applies to every platform unconditionally, + ahead of any description-specific validate_fn.""" desc = bound_entity.desc write_fn = getattr(desc, 'write_fn', None) if write_fn is None: return href = bound_entity.href rep = self._cache.get(href or '') or {} + resources = self._cache.snapshot() + if _remote_control_disabled(resources): + raise ServiceValidationError(_REMOTE_CONTROL_DISABLED_MESSAGE) validate_fn = getattr(desc, 'validate_fn', None) if validate_fn is not None: - error = validate_fn(payload, rep, self._cache.snapshot()) + error = validate_fn(payload, rep, resources) if error: raise ServiceValidationError(error) result = write_fn(payload, rep, href) diff --git a/tests/localthings/test_coordinator.py b/tests/localthings/test_coordinator.py index 7739da5..ed199e2 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -7,12 +7,15 @@ from unittest.mock import AsyncMock, patch import cbor2 import pytest from homeassistant.core import HomeAssistant +from homeassistant.exceptions import ServiceValidationError from homeassistant.helpers import issue_registry as ir from custom_components.localthings.const import ( CONF_HOST, DOMAIN, SUMMARY_INTERVAL_S, ) -from custom_components.localthings.coordinator import LocalThingsCoordinator +from custom_components.localthings.coordinator import ( + LocalThingsCoordinator, _remote_control_disabled, +) from custom_components.localthings.observe import MODE_OBSERVE, MODE_POLL, PUSH_HEALTH_WINDOW_S from .conftest import ENTRY_DATA, MOCK_SERIAL @@ -634,3 +637,136 @@ async def test_write_marks_href_pending_before_post( await coordinator.async_send_command(bound, 5) assert coordinator._observe._settle_until.get('/test/vs/0') is not None + + +class TestRemoteControlDisabled: + """_remote_control_disabled mirrors REMOTE_CONTROL_GENERIC/_VS_FALLBACK's + OCF-standard-preferred read (registry/capabilities/common.py) directly + on a raw resources snapshot.""" + + def test_vs_fallback_href_true_is_enabled(self): + resources = {'/remotectrl/vs/0': {'x.com.samsung.da.remoteControlEnabled': 'true'}} + assert _remote_control_disabled(resources) is False + + def test_vs_fallback_href_false_is_disabled(self): + resources = {'/remotectrl/vs/0': {'x.com.samsung.da.remoteControlEnabled': 'false'}} + assert _remote_control_disabled(resources) is True + + def test_generic_href_true_is_enabled(self): + resources = {'/remotectrl/0': {'value': True}} + assert _remote_control_disabled(resources) is False + + def test_generic_href_false_is_disabled(self): + resources = {'/remotectrl/0': {'value': False}} + assert _remote_control_disabled(resources) is True + + def test_generic_href_wins_when_both_present(self): + resources = { + '/remotectrl/0': {'value': True}, + '/remotectrl/vs/0': {'x.com.samsung.da.remoteControlEnabled': 'false'}, + } + assert _remote_control_disabled(resources) is False + + def test_fails_open_when_capability_absent(self): + assert _remote_control_disabled({}) is False + + +async def test_send_command_blocked_when_remote_control_disabled( + hass: HomeAssistant, mock_entry, mock_coordinator_observe_session +) -> None: + """A device reporting remote control off must reject every write with + a user-facing message, ahead of write_fn's silent-no-op path and any + description-level validate_fn.""" + from custom_components.localthings.registry.discovery import BoundEntity + from custom_components.localthings.registry.entities import NumberDesc + + fake = mock_coordinator_observe_session + await hass.config_entries.async_setup(mock_entry.entry_id) + await hass.async_block_till_done() + coordinator: LocalThingsCoordinator = hass.data[DOMAIN][mock_entry.entry_id] + coordinator._cache.apply_rep( + '/remotectrl/vs/0', + {'x.com.samsung.da.remoteControlEnabled': 'false'}, + source='test', + ) + + def _write_fn(payload, rep, href): + return (['some', 'path'], {'value': payload}) + + desc = NumberDesc(key='test', field='value', write_fn=_write_fn) + bound = BoundEntity(href='/test/vs/0', capability=coordinator.bound[0].capability, desc=desc) + + posted = False + + def _post(*a, **k): + nonlocal posted + posted = True + return (0x44, b'') + + with patch.object(fake, 'subscribe'): + fake.post = _post + with pytest.raises(ServiceValidationError, match="Remote control is turned off"): + await coordinator.async_send_command(bound, 5) + + assert posted is False + + +async def test_send_command_allowed_when_remote_control_enabled( + hass: HomeAssistant, mock_entry, mock_coordinator_observe_session +) -> None: + """The same write goes through once remote control reports enabled.""" + from custom_components.localthings.registry.discovery import BoundEntity + from custom_components.localthings.registry.entities import NumberDesc + + fake = mock_coordinator_observe_session + await hass.config_entries.async_setup(mock_entry.entry_id) + await hass.async_block_till_done() + coordinator: LocalThingsCoordinator = hass.data[DOMAIN][mock_entry.entry_id] + coordinator._cache.apply_rep( + '/remotectrl/vs/0', + {'x.com.samsung.da.remoteControlEnabled': 'true'}, + source='test', + ) + + def _write_fn(payload, rep, href): + return (['some', 'path'], {'value': payload}) + + desc = NumberDesc(key='test', field='value', write_fn=_write_fn) + bound = BoundEntity(href='/test/vs/0', capability=coordinator.bound[0].capability, desc=desc) + + with patch.object(fake, 'subscribe'): + fake.post = lambda *a, **k: (0x44, b'') + await coordinator.async_send_command(bound, 5) + + assert coordinator._observe._settle_until.get('/test/vs/0') is not None + + +async def test_send_command_remote_control_check_precedes_validate_fn( + hass: HomeAssistant, mock_entry, mock_coordinator_observe_session +) -> None: + """When both a disabled remote-control state and a failing validate_fn + apply to the same write, the remote-control message wins -- it's the + more actionable of the two and is checked first.""" + from custom_components.localthings.registry.discovery import BoundEntity + from custom_components.localthings.registry.entities import SwitchDesc + + await hass.config_entries.async_setup(mock_entry.entry_id) + await hass.async_block_till_done() + coordinator: LocalThingsCoordinator = hass.data[DOMAIN][mock_entry.entry_id] + coordinator._cache.apply_rep( + '/remotectrl/vs/0', + {'x.com.samsung.da.remoteControlEnabled': 'false'}, + source='test', + ) + + def _write_fn(payload, rep, href=None): + return (['some', 'path'], {'value': payload}) + + def _validate_fn(payload, rep, resources): + return "always rejected" + + desc = SwitchDesc(key='test', field='value', write_fn=_write_fn, validate_fn=_validate_fn) + bound = BoundEntity(href='/test/vs/0', capability=coordinator.bound[0].capability, desc=desc) + + with pytest.raises(ServiceValidationError, match="Remote control is turned off"): + await coordinator.async_send_command(bound, 'On') From 2f6b7d54e328242ab3b0c27318020a74748da9f1 Mon Sep 17 00:00:00 2001 From: Marc Billow Date: Thu, 23 Jul 2026 02:29:03 +0000 Subject: [PATCH 2/2] refactor: share remote-control read between sensor and write guard, poll it warm Move the on/off interpretation into a single remote_control_enabled() in registry/capabilities/common.py so the write-guard added in the previous commit can't silently drift from the Smart Control binary sensor's own reading of the same hrefs. Also promotes both /remotectrl hrefs to poll_tier='warm' so the coordinator's cached state backing that write guard doesn't lag up to a full 30s cold summary poll behind the device's actual toggle state. --- custom_components/localthings/coordinator.py | 17 ++--------- .../registry/capabilities/common.py | 23 +++++++++++++++ tests/localthings/test_coordinator.py | 28 ++++++++++--------- tests/test_common_capabilities.py | 8 ++++++ 4 files changed, 48 insertions(+), 28 deletions(-) diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 359a598..cb2f8b1 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -22,6 +22,7 @@ from smartthings_local.ocf.state_cache import StateCache from .registry.batch import parse_device0_batch from .registry.by_type import for_device, for_device_by_model +from .registry.capabilities.common import remote_control_enabled from .registry.discovery import discover, BoundEntity from .registry import CAPABILITIES from .registry.adapter import flatten @@ -44,20 +45,6 @@ _REMOTE_CONTROL_DISABLED_MESSAGE = ( ) -def _remote_control_disabled(resources: dict) -> bool: - """True only when the device explicitly reports remote control off. - Mirrors REMOTE_CONTROL_GENERIC/_VS_FALLBACK's href/field pair in - registry/capabilities/common.py. Fails open (False) when neither href - is present -- most device types don't report this capability at all.""" - generic = resources.get('/remotectrl/0') - if generic is not None: - return not bool(generic.get('value')) - fallback = resources.get('/remotectrl/vs/0') - if fallback is not None: - return str(fallback.get('x.com.samsung.da.remoteControlEnabled')).lower() != 'true' - return False - - class _NoOpDescriptor: """StateCache requires a descriptor with an on_observation hook. This integration doesn't use per-capability observation hooks, so this is a @@ -545,7 +532,7 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): href = bound_entity.href rep = self._cache.get(href or '') or {} resources = self._cache.snapshot() - if _remote_control_disabled(resources): + if not remote_control_enabled(resources): raise ServiceValidationError(_REMOTE_CONTROL_DISABLED_MESSAGE) validate_fn = getattr(desc, 'validate_fn', None) if validate_fn is not None: diff --git a/custom_components/localthings/registry/capabilities/common.py b/custom_components/localthings/registry/capabilities/common.py index ef181fc..bb1775b 100644 --- a/custom_components/localthings/registry/capabilities/common.py +++ b/custom_components/localthings/registry/capabilities/common.py @@ -119,8 +119,30 @@ KIDS_LOCK_VS_FALLBACK = Capability( ), ) + +def remote_control_enabled(resources: dict) -> bool: + """Single source of truth for the /remotectrl on/off signal, mirroring + REMOTE_CONTROL_GENERIC/_VS_FALLBACK's href/field pair and precedence + below. Used both to render the read-only Smart Control binary_sensor + (via those two descriptors) and, from coordinator.async_send_command, + to block writes outright when remote control is off. Both hrefs are + poll_tier='warm' below so that gate reads recent state (subscribed + when observe is live, subpolled every ~6s otherwise) rather than a + once-per-30s cold summary poll. True (assume enabled) when neither + href is present -- most device types don't report this capability + at all.""" + generic = resources.get('/remotectrl/0') + if generic is not None: + return bool(generic.get('value')) + fallback = resources.get('/remotectrl/vs/0') + if fallback is not None: + return str(fallback.get('x.com.samsung.da.remoteControlEnabled')).lower() == 'true' + return True + + REMOTE_CONTROL_GENERIC = Capability( href='/remotectrl/0', + poll_tier='warm', entities=( BinarySensorDesc(key='remote_control', field='value', name='Smart Control', device_class='connectivity', @@ -131,6 +153,7 @@ REMOTE_CONTROL_GENERIC = Capability( REMOTE_CONTROL_VS_FALLBACK = Capability( href='/remotectrl/vs/0', match_fn=lambda rep, resources: '/remotectrl/0' not in resources, + poll_tier='warm', entities=( BinarySensorDesc(key='remote_control', field='x.com.samsung.da.remoteControlEnabled', diff --git a/tests/localthings/test_coordinator.py b/tests/localthings/test_coordinator.py index ed199e2..0a5c2cb 100644 --- a/tests/localthings/test_coordinator.py +++ b/tests/localthings/test_coordinator.py @@ -13,8 +13,9 @@ from homeassistant.helpers import issue_registry as ir from custom_components.localthings.const import ( CONF_HOST, DOMAIN, SUMMARY_INTERVAL_S, ) -from custom_components.localthings.coordinator import ( - LocalThingsCoordinator, _remote_control_disabled, +from custom_components.localthings.coordinator import LocalThingsCoordinator +from custom_components.localthings.registry.capabilities.common import ( + remote_control_enabled, ) from custom_components.localthings.observe import MODE_OBSERVE, MODE_POLL, PUSH_HEALTH_WINDOW_S @@ -639,36 +640,37 @@ async def test_write_marks_href_pending_before_post( assert coordinator._observe._settle_until.get('/test/vs/0') is not None -class TestRemoteControlDisabled: - """_remote_control_disabled mirrors REMOTE_CONTROL_GENERIC/_VS_FALLBACK's - OCF-standard-preferred read (registry/capabilities/common.py) directly - on a raw resources snapshot.""" +class TestRemoteControlEnabled: + """remote_control_enabled (registry/capabilities/common.py) is the + single source of truth for the /remotectrl on/off signal, shared by + the Smart Control binary_sensor descriptors and the coordinator's + write guard alike.""" def test_vs_fallback_href_true_is_enabled(self): resources = {'/remotectrl/vs/0': {'x.com.samsung.da.remoteControlEnabled': 'true'}} - assert _remote_control_disabled(resources) is False + assert remote_control_enabled(resources) is True def test_vs_fallback_href_false_is_disabled(self): resources = {'/remotectrl/vs/0': {'x.com.samsung.da.remoteControlEnabled': 'false'}} - assert _remote_control_disabled(resources) is True + assert remote_control_enabled(resources) is False def test_generic_href_true_is_enabled(self): resources = {'/remotectrl/0': {'value': True}} - assert _remote_control_disabled(resources) is False + assert remote_control_enabled(resources) is True def test_generic_href_false_is_disabled(self): resources = {'/remotectrl/0': {'value': False}} - assert _remote_control_disabled(resources) is True + assert remote_control_enabled(resources) is False def test_generic_href_wins_when_both_present(self): resources = { '/remotectrl/0': {'value': True}, '/remotectrl/vs/0': {'x.com.samsung.da.remoteControlEnabled': 'false'}, } - assert _remote_control_disabled(resources) is False + assert remote_control_enabled(resources) is True - def test_fails_open_when_capability_absent(self): - assert _remote_control_disabled({}) is False + def test_assumes_enabled_when_capability_absent(self): + assert remote_control_enabled({}) is True async def test_send_command_blocked_when_remote_control_disabled( diff --git a/tests/test_common_capabilities.py b/tests/test_common_capabilities.py index fdd1150..473d64d 100644 --- a/tests/test_common_capabilities.py +++ b/tests/test_common_capabilities.py @@ -84,6 +84,14 @@ class TestRemoteControlFallback: assert common.REMOTE_CONTROL_VS_FALLBACK.match_fn( {}, {'/remotectrl/0': {}, '/remotectrl/vs/0': {}}) is False + def test_polled_warm_so_write_gating_stays_fresh(self): + """coordinator.async_send_command blocks writes on this signal, so + it can't sit in the default 'cold' tier (refreshed only once per + 30s summary poll) -- it needs the subscribe/subpoll cadence 'warm' + and 'hot' hrefs get instead.""" + assert common.REMOTE_CONTROL_GENERIC.poll_tier == 'warm' + assert common.REMOTE_CONTROL_VS_FALLBACK.poll_tier == 'warm' + # --------------------------------------------------------------------------- # Energy meter. instantaneousPower clamps negatives to 0, but the constant