From 1e2f00e1867a63e89f450e4ff091d0ece083488f Mon Sep 17 00:00:00 2001 From: Erik Montnemery Date: Sat, 21 Jan 2023 00:44:17 +0100 Subject: [PATCH] Improve device automation validation (#86143) --- .../components/device_automation/action.py | 17 +-- .../components/device_automation/condition.py | 17 +-- .../components/device_automation/helpers.py | 80 ++++++++++ .../components/device_automation/trigger.py | 40 +---- .../components/zha/device_trigger.py | 2 +- .../components/device_automation/test_init.py | 144 +++++++++++++++++- tests/components/zha/test_device_trigger.py | 51 ++----- tests/helpers/test_condition.py | 10 +- tests/helpers/test_script.py | 10 +- 9 files changed, 255 insertions(+), 116 deletions(-) create mode 100644 homeassistant/components/device_automation/helpers.py diff --git a/homeassistant/components/device_automation/action.py b/homeassistant/components/device_automation/action.py index 081b6bb283a..58c124377ff 100644 --- a/homeassistant/components/device_automation/action.py +++ b/homeassistant/components/device_automation/action.py @@ -1,16 +1,17 @@ """Device action validator.""" from __future__ import annotations -from typing import Any, Protocol, cast +from typing import Any, Protocol import voluptuous as vol from homeassistant.const import CONF_DOMAIN from homeassistant.core import Context, HomeAssistant +from homeassistant.helpers import config_validation as cv from homeassistant.helpers.typing import ConfigType from . import DeviceAutomationType, async_get_device_automation_platform -from .exceptions import InvalidDeviceAutomationConfig +from .helpers import async_validate_device_automation_config class DeviceAutomationActionProtocol(Protocol): @@ -50,15 +51,9 @@ async def async_validate_action_config( hass: HomeAssistant, config: ConfigType ) -> ConfigType: """Validate config.""" - try: - platform = await async_get_device_automation_platform( - hass, config[CONF_DOMAIN], DeviceAutomationType.ACTION - ) - if hasattr(platform, "async_validate_action_config"): - return await platform.async_validate_action_config(hass, config) - return cast(ConfigType, platform.ACTION_SCHEMA(config)) - except InvalidDeviceAutomationConfig as err: - raise vol.Invalid(str(err) or "Invalid action configuration") from err + return await async_validate_device_automation_config( + hass, config, cv.DEVICE_ACTION_SCHEMA, DeviceAutomationType.ACTION + ) async def async_call_action_from_config( diff --git a/homeassistant/components/device_automation/condition.py b/homeassistant/components/device_automation/condition.py index d656908f4be..3856458c3dd 100644 --- a/homeassistant/components/device_automation/condition.py +++ b/homeassistant/components/device_automation/condition.py @@ -1,7 +1,7 @@ """Validate device conditions.""" from __future__ import annotations -from typing import TYPE_CHECKING, Any, Protocol, cast +from typing import TYPE_CHECKING, Any, Protocol import voluptuous as vol @@ -11,7 +11,7 @@ from homeassistant.helpers import config_validation as cv from homeassistant.helpers.typing import ConfigType from . import DeviceAutomationType, async_get_device_automation_platform -from .exceptions import InvalidDeviceAutomationConfig +from .helpers import async_validate_device_automation_config if TYPE_CHECKING: from homeassistant.helpers import condition @@ -50,16 +50,9 @@ async def async_validate_condition_config( hass: HomeAssistant, config: ConfigType ) -> ConfigType: """Validate device condition config.""" - try: - config = cv.DEVICE_CONDITION_SCHEMA(config) - platform = await async_get_device_automation_platform( - hass, config[CONF_DOMAIN], DeviceAutomationType.CONDITION - ) - if hasattr(platform, "async_validate_condition_config"): - return await platform.async_validate_condition_config(hass, config) - return cast(ConfigType, platform.CONDITION_SCHEMA(config)) - except InvalidDeviceAutomationConfig as err: - raise vol.Invalid(str(err) or "Invalid condition configuration") from err + return await async_validate_device_automation_config( + hass, config, cv.DEVICE_CONDITION_SCHEMA, DeviceAutomationType.CONDITION + ) async def async_condition_from_config( diff --git a/homeassistant/components/device_automation/helpers.py b/homeassistant/components/device_automation/helpers.py new file mode 100644 index 00000000000..5f844c36aa5 --- /dev/null +++ b/homeassistant/components/device_automation/helpers.py @@ -0,0 +1,80 @@ +"""Helpers for device oriented automations.""" +from __future__ import annotations + +from typing import cast + +import voluptuous as vol + +from homeassistant.const import CONF_DEVICE_ID, CONF_DOMAIN +from homeassistant.core import HomeAssistant +from homeassistant.helpers import device_registry as dr +from homeassistant.helpers.typing import ConfigType + +from . import DeviceAutomationType, async_get_device_automation_platform +from .exceptions import InvalidDeviceAutomationConfig + +DYNAMIC_VALIDATOR = { + DeviceAutomationType.ACTION: "async_validate_action_config", + DeviceAutomationType.CONDITION: "async_validate_condition_config", + DeviceAutomationType.TRIGGER: "async_validate_trigger_config", +} + +STATIC_VALIDATOR = { + DeviceAutomationType.ACTION: "ACTION_SCHEMA", + DeviceAutomationType.CONDITION: "CONDITION_SCHEMA", + DeviceAutomationType.TRIGGER: "TRIGGER_SCHEMA", +} + + +async def async_validate_device_automation_config( + hass: HomeAssistant, + config: ConfigType, + automation_schema: vol.Schema, + automation_type: DeviceAutomationType, +) -> ConfigType: + """Validate config.""" + validated_config: ConfigType = automation_schema(config) + platform = await async_get_device_automation_platform( + hass, validated_config[CONF_DOMAIN], automation_type + ) + if not hasattr(platform, DYNAMIC_VALIDATOR[automation_type]): + # Pass the unvalidated config to avoid mutating the raw config twice + return cast( + ConfigType, getattr(platform, STATIC_VALIDATOR[automation_type])(config) + ) + + # Only call the dynamic validator if the referenced device exists and the relevant + # config entry is loaded + registry = dr.async_get(hass) + if not (device := registry.async_get(validated_config[CONF_DEVICE_ID])): + # The device referenced by the device trigger does not exist + raise InvalidDeviceAutomationConfig( + f"Unknown device '{validated_config[CONF_DEVICE_ID]}'" + ) + + device_config_entry = None + for entry_id in device.config_entries: + if ( + not (entry := hass.config_entries.async_get_entry(entry_id)) + or entry.domain != validated_config[CONF_DOMAIN] + ): + continue + device_config_entry = entry + break + + if not device_config_entry: + # The config entry referenced by the device trigger does not exist + raise InvalidDeviceAutomationConfig( + f"Device '{validated_config[CONF_DEVICE_ID]}' has no config entry from " + f"domain '{validated_config[CONF_DOMAIN]}'" + ) + + if not await hass.config_entries.async_wait_component(device_config_entry): + # The component could not be loaded, skip the dynamic validation + return validated_config + + # Pass the unvalidated config to avoid mutating the raw config twice + return cast( + ConfigType, + await getattr(platform, DYNAMIC_VALIDATOR[automation_type])(hass, config), + ) diff --git a/homeassistant/components/device_automation/trigger.py b/homeassistant/components/device_automation/trigger.py index cd5b3a84c82..80e96ddaba9 100644 --- a/homeassistant/components/device_automation/trigger.py +++ b/homeassistant/components/device_automation/trigger.py @@ -1,13 +1,12 @@ """Offer device oriented automation.""" from __future__ import annotations -from typing import Any, Protocol, cast +from typing import Any, Protocol import voluptuous as vol -from homeassistant.const import CONF_DEVICE_ID, CONF_DOMAIN +from homeassistant.const import CONF_DOMAIN from homeassistant.core import CALLBACK_TYPE, HomeAssistant -from homeassistant.helpers import device_registry as dr from homeassistant.helpers.trigger import TriggerActionType, TriggerInfo from homeassistant.helpers.typing import ConfigType @@ -16,7 +15,7 @@ from . import ( DeviceAutomationType, async_get_device_automation_platform, ) -from .exceptions import InvalidDeviceAutomationConfig +from .helpers import async_validate_device_automation_config TRIGGER_SCHEMA = DEVICE_TRIGGER_BASE_SCHEMA.extend({}, extra=vol.ALLOW_EXTRA) @@ -58,36 +57,9 @@ async def async_validate_trigger_config( hass: HomeAssistant, config: ConfigType ) -> ConfigType: """Validate config.""" - try: - platform = await async_get_device_automation_platform( - hass, config[CONF_DOMAIN], DeviceAutomationType.TRIGGER - ) - if not hasattr(platform, "async_validate_trigger_config"): - return cast(ConfigType, platform.TRIGGER_SCHEMA(config)) - - # Only call the dynamic validator if the relevant config entry is loaded - registry = dr.async_get(hass) - if not (device := registry.async_get(config[CONF_DEVICE_ID])): - return config - - device_config_entry = None - for entry_id in device.config_entries: - if not (entry := hass.config_entries.async_get_entry(entry_id)): - continue - if entry.domain != config[CONF_DOMAIN]: - continue - device_config_entry = entry - break - - if not device_config_entry: - return config - - if not await hass.config_entries.async_wait_component(device_config_entry): - return config - - return await platform.async_validate_trigger_config(hass, config) - except InvalidDeviceAutomationConfig as err: - raise vol.Invalid(str(err) or "Invalid trigger configuration") from err + return await async_validate_device_automation_config( + hass, config, TRIGGER_SCHEMA, DeviceAutomationType.TRIGGER + ) async def async_attach_trigger( diff --git a/homeassistant/components/zha/device_trigger.py b/homeassistant/components/zha/device_trigger.py index 6f78aa6f858..03a13f317f3 100644 --- a/homeassistant/components/zha/device_trigger.py +++ b/homeassistant/components/zha/device_trigger.py @@ -41,7 +41,7 @@ async def async_validate_trigger_config( zha_device.device_automation_triggers is None or trigger not in zha_device.device_automation_triggers ): - raise InvalidDeviceAutomationConfig + raise InvalidDeviceAutomationConfig(f"device does not have trigger {trigger}") return config diff --git a/tests/components/device_automation/test_init.py b/tests/components/device_automation/test_init.py index ad49b8d8f7a..a2589693238 100644 --- a/tests/components/device_automation/test_init.py +++ b/tests/components/device_automation/test_init.py @@ -867,7 +867,7 @@ async def test_automation_with_device_action(hass, caplog, fake_integration): async def test_automation_with_dynamically_validated_action( - hass, caplog, fake_integration + hass, caplog, device_reg, fake_integration ): """Test device automation with an action which is dynamically validated.""" @@ -875,6 +875,14 @@ async def test_automation_with_dynamically_validated_action( module = module_cache["fake_integration.device_action"] module.async_validate_action_config = AsyncMock() + config_entry = MockConfigEntry(domain="fake_integration", data={}) + config_entry.state = config_entries.ConfigEntryState.LOADED + config_entry.add_to_hass(hass) + device_entry = device_reg.async_get_or_create( + config_entry_id=config_entry.entry_id, + connections={(device_registry.CONNECTION_NETWORK_MAC, "12:34:56:AB:CD:EF")}, + ) + assert await async_setup_component( hass, automation.DOMAIN, @@ -882,7 +890,7 @@ async def test_automation_with_dynamically_validated_action( automation.DOMAIN: { "alias": "hello", "trigger": {"platform": "event", "event_type": "test_event1"}, - "action": {"device_id": "", "domain": "fake_integration"}, + "action": {"device_id": device_entry.id, "domain": "fake_integration"}, } }, ) @@ -940,7 +948,7 @@ async def test_automation_with_device_condition(hass, caplog, fake_integration): async def test_automation_with_dynamically_validated_condition( - hass, caplog, fake_integration + hass, caplog, device_reg, fake_integration ): """Test device automation with a condition which is dynamically validated.""" @@ -948,6 +956,14 @@ async def test_automation_with_dynamically_validated_condition( module = module_cache["fake_integration.device_condition"] module.async_validate_condition_config = AsyncMock() + config_entry = MockConfigEntry(domain="fake_integration", data={}) + config_entry.state = config_entries.ConfigEntryState.LOADED + config_entry.add_to_hass(hass) + device_entry = device_reg.async_get_or_create( + config_entry_id=config_entry.entry_id, + connections={(device_registry.CONNECTION_NETWORK_MAC, "12:34:56:AB:CD:EF")}, + ) + assert await async_setup_component( hass, automation.DOMAIN, @@ -957,7 +973,7 @@ async def test_automation_with_dynamically_validated_condition( "trigger": {"platform": "event", "event_type": "test_event1"}, "condition": { "condition": "device", - "device_id": "none", + "device_id": device_entry.id, "domain": "fake_integration", }, "action": {"service": "test.automation", "entity_id": "hello.world"}, @@ -1121,6 +1137,24 @@ async def test_automation_with_bad_condition_action(hass, caplog): assert "required key not provided" in caplog.text +async def test_automation_with_bad_condition_missing_domain(hass, caplog): + """Test automation with bad device condition.""" + assert await async_setup_component( + hass, + automation.DOMAIN, + { + automation.DOMAIN: { + "alias": "hello", + "trigger": {"platform": "event", "event_type": "test_event1"}, + "condition": {"condition": "device", "device_id": "hello.device"}, + "action": {"service": "test.automation", "entity_id": "hello.world"}, + } + }, + ) + + assert "required key not provided @ data['condition'][0]['domain']" in caplog.text + + async def test_automation_with_bad_condition(hass, caplog): """Test automation with bad device condition.""" assert await async_setup_component( @@ -1305,3 +1339,105 @@ async def test_websocket_device_not_found(hass, hass_ws_client): assert msg["id"] == 1 assert not msg["success"] assert msg["error"] == {"code": "not_found", "message": "Device not found"} + + +async def test_automation_with_unknown_device(hass, caplog, fake_integration): + """Test device automation with a trigger with an unknown device.""" + + module_cache = hass.data.setdefault(loader.DATA_COMPONENTS, {}) + module = module_cache["fake_integration.device_trigger"] + module.async_validate_trigger_config = AsyncMock() + + assert await async_setup_component( + hass, + automation.DOMAIN, + { + automation.DOMAIN: { + "alias": "hello", + "trigger": { + "platform": "device", + "device_id": "no_such_device", + "domain": "fake_integration", + }, + "action": {"service": "test.automation", "entity_id": "hello.world"}, + } + }, + ) + + module.async_validate_trigger_config.assert_not_awaited() + assert ( + "Automation with alias 'hello' failed to setup triggers and has been disabled: " + "Unknown device 'no_such_device'" in caplog.text + ) + + +async def test_automation_with_device_wrong_domain( + hass, caplog, device_reg, fake_integration +): + """Test device automation where the device doesn't have the right config entry.""" + + module_cache = hass.data.setdefault(loader.DATA_COMPONENTS, {}) + module = module_cache["fake_integration.device_trigger"] + module.async_validate_trigger_config = AsyncMock() + + device_entry = device_reg.async_get_or_create( + config_entry_id="not_fake_integration_config_entry", + connections={(device_registry.CONNECTION_NETWORK_MAC, "12:34:56:AB:CD:EF")}, + ) + assert await async_setup_component( + hass, + automation.DOMAIN, + { + automation.DOMAIN: { + "alias": "hello", + "trigger": { + "platform": "device", + "device_id": device_entry.id, + "domain": "fake_integration", + }, + "action": {"service": "test.automation", "entity_id": "hello.world"}, + } + }, + ) + + module.async_validate_trigger_config.assert_not_awaited() + assert ( + "Automation with alias 'hello' failed to setup triggers and has been disabled: " + f"Device '{device_entry.id}' has no config entry from domain 'fake_integration'" + in caplog.text + ) + + +async def test_automation_with_device_component_not_loaded( + hass, caplog, device_reg, fake_integration +): + """Test device automation where the device's config entry is not loaded.""" + + module_cache = hass.data.setdefault(loader.DATA_COMPONENTS, {}) + module = module_cache["fake_integration.device_trigger"] + module.async_validate_trigger_config = AsyncMock() + module.async_attach_trigger = AsyncMock() + + config_entry = MockConfigEntry(domain="fake_integration", data={}) + config_entry.add_to_hass(hass) + device_entry = device_reg.async_get_or_create( + config_entry_id=config_entry.entry_id, + connections={(device_registry.CONNECTION_NETWORK_MAC, "12:34:56:AB:CD:EF")}, + ) + assert await async_setup_component( + hass, + automation.DOMAIN, + { + automation.DOMAIN: { + "alias": "hello", + "trigger": { + "platform": "device", + "device_id": device_entry.id, + "domain": "fake_integration", + }, + "action": {"service": "test.automation", "entity_id": "hello.world"}, + } + }, + ) + + module.async_validate_trigger_config.assert_not_awaited() diff --git a/tests/components/zha/test_device_trigger.py b/tests/components/zha/test_device_trigger.py index 31895654f4b..127c2adae12 100644 --- a/tests/components/zha/test_device_trigger.py +++ b/tests/components/zha/test_device_trigger.py @@ -297,7 +297,7 @@ async def test_device_offline_fires( async def test_exception_no_triggers(hass, mock_devices, calls, caplog): - """Test for exception on event triggers firing.""" + """Test for exception when validating device triggers.""" _, zha_device = mock_devices @@ -327,11 +327,14 @@ async def test_exception_no_triggers(hass, mock_devices, calls, caplog): }, ) await hass.async_block_till_done() - assert "Invalid trigger configuration" in caplog.text + assert ( + "Unnamed automation failed to setup triggers and has been disabled: " + "device does not have trigger ('junk', 'junk')" in caplog.text + ) async def test_exception_bad_trigger(hass, mock_devices, calls, caplog): - """Test for exception on event triggers firing.""" + """Test for exception when validating device triggers.""" zigpy_device, zha_device = mock_devices @@ -369,43 +372,7 @@ async def test_exception_bad_trigger(hass, mock_devices, calls, caplog): }, ) await hass.async_block_till_done() - assert "Invalid trigger configuration" in caplog.text - - -@pytest.mark.skip(reason="Temporarily disabled until automation validation is improved") -async def test_exception_no_device(hass, mock_devices, calls, caplog): - """Test for exception on event triggers firing.""" - - zigpy_device, zha_device = mock_devices - - zigpy_device.device_automation_triggers = { - (SHAKEN, SHAKEN): {COMMAND: COMMAND_SHAKE}, - (DOUBLE_PRESS, DOUBLE_PRESS): {COMMAND: COMMAND_DOUBLE}, - (SHORT_PRESS, SHORT_PRESS): {COMMAND: COMMAND_SINGLE}, - (LONG_PRESS, LONG_PRESS): {COMMAND: COMMAND_HOLD}, - (LONG_RELEASE, LONG_RELEASE): {COMMAND: COMMAND_HOLD}, - } - - await async_setup_component( - hass, - automation.DOMAIN, - { - automation.DOMAIN: [ - { - "trigger": { - "device_id": "no_such_device_id", - "domain": "zha", - "platform": "device", - "type": "junk", - "subtype": "junk", - }, - "action": { - "service": "test.automation", - "data": {"message": "service called"}, - }, - } - ] - }, + assert ( + "Unnamed automation failed to setup triggers and has been disabled: " + "device does not have trigger ('junk', 'junk')" in caplog.text ) - await hass.async_block_till_done() - assert "Invalid trigger configuration" in caplog.text diff --git a/tests/helpers/test_condition.py b/tests/helpers/test_condition.py index 4d779a1a4d2..19682c78b46 100644 --- a/tests/helpers/test_condition.py +++ b/tests/helpers/test_condition.py @@ -3280,14 +3280,12 @@ async def test_trigger(hass): async def test_platform_async_validate_condition_config(hass): """Test platform.async_validate_condition_config will be called if it exists.""" config = {CONF_DEVICE_ID: "test", CONF_DOMAIN: "test", CONF_CONDITION: "device"} - platform = AsyncMock() with patch( - "homeassistant.components.device_automation.condition.async_get_device_automation_platform", - return_value=platform, - ): - platform.async_validate_condition_config.return_value = config + "homeassistant.components.device_automation.condition.async_validate_condition_config", + AsyncMock(), + ) as device_automation_validate_condition_mock: await condition.async_validate_condition_config(hass, config) - platform.async_validate_condition_config.assert_awaited() + device_automation_validate_condition_mock.assert_awaited() async def test_disabled_condition(hass: HomeAssistant) -> None: diff --git a/tests/helpers/test_script.py b/tests/helpers/test_script.py index a5f3cc0cc91..0015f0437fd 100644 --- a/tests/helpers/test_script.py +++ b/tests/helpers/test_script.py @@ -4633,14 +4633,12 @@ async def test_breakpoints_2(hass): async def test_platform_async_validate_action_config(hass): """Test platform.async_validate_action_config will be called if it exists.""" config = {CONF_DEVICE_ID: "test", CONF_DOMAIN: "test"} - platform = AsyncMock() with patch( - "homeassistant.components.device_automation.action.async_get_device_automation_platform", - return_value=platform, - ): - platform.async_validate_action_config.return_value = config + "homeassistant.components.device_automation.action.async_validate_action_config", + return_value=AsyncMock(), + ) as device_automation_validate_action_mock: await script.async_validate_action_config(hass, config) - platform.async_validate_action_config.assert_awaited() + device_automation_validate_action_mock.assert_awaited() async def test_stop_action(hass, caplog):