From 55b036ec5ef570b146a6126408da929dd66e431e Mon Sep 17 00:00:00 2001 From: Ben Randall Date: Mon, 3 Oct 2022 10:42:57 -0400 Subject: [PATCH] Improve device_automation trigger validation (#75044) * improve device_automation trigger validation Validates the trigger configuration against the device_trigger schema before trying to access any of the properties in order to provide better error messages. Updates the error message to include an explicit indication that the error is coming from a trigger configuration. The inner error message from the validator can be accessed by viewing the stack trace. Add test case for trigger missing domain. Make action and condition validation consistent with trigger. This is not strictly necessary, but should be helpful for certain use cases that bypass some of the outer validation. Removed redundant schema elements from humidifier device_trigger. **Blueprint with missing `domain`** ``` 2022-07-12 06:02:18.351 ERROR (MainThread) [homeassistant.setup] Error during setup of component automation Traceback (most recent call last): File "/workspaces/core/homeassistant/setup.py", line 235, in _async_setup_component result = await task File "/workspaces/core/homeassistant/components/automation/__init__.py", line 241, in async_setup if not await _async_process_config(hass, config, component): File "/workspaces/core/homeassistant/components/automation/__init__.py", line 648, in _async_process_config await async_validate_config_item(hass, raw_config), File "/workspaces/core/homeassistant/components/automation/config.py", line 74, in async_validate_config_item config[CONF_TRIGGER] = await async_validate_trigger_config( File "/workspaces/core/homeassistant/helpers/trigger.py", line 59, in async_validate_trigger_config conf = await platform.async_validate_trigger_config(hass, conf) File "/workspaces/core/homeassistant/components/device_automation/trigger.py", line 67, in async_validate_trigger_config hass, config[CONF_DOMAIN], DeviceAutomationType.TRIGGER KeyError: 'domain' ``` **Blueprint with missing `property` (specific to zwave_js event schema)** ``` 2022-07-12 06:09:54.206 ERROR (MainThread) [homeassistant.components.automation] Blueprint Missing Property generated invalid automation with inputs OrderedDict([('control_switch', '498be56d796836a67406e9ad373d23db')]): required key not provided @ data['property']. Got None ``` **Blueprint with missing `domain`** ``` 2022-07-12 06:12:16.080 ERROR (MainThread) [homeassistant.components.automation] Blueprint Missing Domain generated invalid automation with inputs OrderedDict([('control_switch', '498be56d796836a67406e9ad373d23db')]): invalid trigger configuration: required key not provided @ data['domain']. Got ``` **Blueprint with missing `property` (specific to zwave_js event schema)** ``` 2022-07-12 06:12:16.680 ERROR (MainThread) [homeassistant.components.automation] Blueprint Missing Property generated invalid automation with inputs OrderedDict([('control_switch', '498be56d796836a67406e9ad373d23db')]): invalid trigger configuration: required key not provided @ data['property']. Got ``` * Revert humifidier TRIGGER_SCHEMA change. --- .../components/device_automation/action.py | 6 ++- .../components/device_automation/condition.py | 4 +- .../components/device_automation/trigger.py | 5 +- .../components/rfxtrx/device_action.py | 1 - .../components/device_automation/test_init.py | 51 +++++++++++++++++-- .../components/webostv/test_device_trigger.py | 3 +- 6 files changed, 56 insertions(+), 14 deletions(-) diff --git a/homeassistant/components/device_automation/action.py b/homeassistant/components/device_automation/action.py index 081b6bb283a..432ff2fdb7d 100644 --- a/homeassistant/components/device_automation/action.py +++ b/homeassistant/components/device_automation/action.py @@ -7,6 +7,7 @@ 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 @@ -51,14 +52,15 @@ async def async_validate_action_config( ) -> ConfigType: """Validate config.""" try: + config = cv.DEVICE_ACTION_SCHEMA(config) 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 + except (vol.Invalid, InvalidDeviceAutomationConfig) as err: + raise vol.Invalid("invalid action configuration: " + str(err)) from err 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..3b0a5263f9e 100644 --- a/homeassistant/components/device_automation/condition.py +++ b/homeassistant/components/device_automation/condition.py @@ -58,8 +58,8 @@ async def async_validate_condition_config( 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 + except (vol.Invalid, InvalidDeviceAutomationConfig) as err: + raise vol.Invalid("invalid condition configuration: " + str(err)) from err async def async_condition_from_config( diff --git a/homeassistant/components/device_automation/trigger.py b/homeassistant/components/device_automation/trigger.py index bd72b24d844..aac56b39846 100644 --- a/homeassistant/components/device_automation/trigger.py +++ b/homeassistant/components/device_automation/trigger.py @@ -58,14 +58,15 @@ async def async_validate_trigger_config( ) -> ConfigType: """Validate config.""" try: + config = TRIGGER_SCHEMA(config) 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)) return await platform.async_validate_trigger_config(hass, config) - except InvalidDeviceAutomationConfig as err: - raise vol.Invalid(str(err) or "Invalid trigger configuration") from err + except (vol.Invalid, InvalidDeviceAutomationConfig) as err: + raise InvalidDeviceAutomationConfig("invalid trigger configuration") from err async def async_attach_trigger( diff --git a/homeassistant/components/rfxtrx/device_action.py b/homeassistant/components/rfxtrx/device_action.py index 15595b88cd2..7ea4ed07423 100644 --- a/homeassistant/components/rfxtrx/device_action.py +++ b/homeassistant/components/rfxtrx/device_action.py @@ -80,7 +80,6 @@ async def async_validate_action_config( hass: HomeAssistant, config: ConfigType ) -> ConfigType: """Validate config.""" - config = ACTION_SCHEMA(config) commands, _ = _get_commands(hass, config[CONF_DEVICE_ID], config[CONF_TYPE]) sub_type = config[CONF_SUBTYPE] diff --git a/tests/components/device_automation/test_init.py b/tests/components/device_automation/test_init.py index 3ead6fcb35d..71c062cf7d9 100644 --- a/tests/components/device_automation/test_init.py +++ b/tests/components/device_automation/test_init.py @@ -720,7 +720,28 @@ 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(hass, caplog): +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 ( + "Invalid config for [automation]: required key not provided @ data['condition'][0]['domain']" + in caplog.text + ) + + +async def test_automation_with_bad_condition_missing_device_id(hass, caplog): """Test automation with bad device condition.""" assert await async_setup_component( hass, @@ -735,7 +756,10 @@ async def test_automation_with_bad_condition(hass, caplog): }, ) - assert "required key not provided" in caplog.text + assert ( + "Invalid config for [automation]: required key not provided @ data['condition'][0]['device_id']" + in caplog.text + ) @pytest.fixture @@ -876,8 +900,25 @@ async def test_automation_with_bad_sub_condition(hass, caplog): assert "required key not provided" in caplog.text -async def test_automation_with_bad_trigger(hass, caplog): - """Test automation with bad device trigger.""" +async def test_automation_with_bad_trigger_missing_domain(hass, caplog): + """Test automation with device trigger this is missing domain.""" + assert await async_setup_component( + hass, + automation.DOMAIN, + { + automation.DOMAIN: { + "alias": "hello", + "trigger": {"platform": "device", "device_id": "hello.device"}, + "action": {"service": "test.automation", "entity_id": "hello.world"}, + } + }, + ) + + assert "required key not provided @ data['domain']" in caplog.text + + +async def test_automation_with_bad_trigger_missing_device_id(hass, caplog): + """Test automation with device trigger that is missing device_id.""" assert await async_setup_component( hass, automation.DOMAIN, @@ -890,7 +931,7 @@ async def test_automation_with_bad_trigger(hass, caplog): }, ) - assert "required key not provided" in caplog.text + assert "required key not provided @ data['device_id']" in caplog.text async def test_websocket_device_not_found(hass, hass_ws_client): diff --git a/tests/components/webostv/test_device_trigger.py b/tests/components/webostv/test_device_trigger.py index db15ce3a592..96914885971 100644 --- a/tests/components/webostv/test_device_trigger.py +++ b/tests/components/webostv/test_device_trigger.py @@ -128,8 +128,7 @@ async def test_get_triggers_for_invalid_device_id(hass, caplog): await hass.async_block_till_done() assert ( - "Invalid config for [automation]: Device invalid_device_id is not a valid webostv device" - in caplog.text + "Invalid config for [automation]: invalid trigger configuration" in caplog.text )