From c8128126314f4f714c1ce1bf10571f41a8aaf3cb Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Tue, 6 Oct 2020 13:57:36 +0200 Subject: [PATCH] Don't set upnp config_entry.unique_id from setup entry (#40988) * Don't set config_entry.unique_id from setup entry. Fixes #40168 * Ensure entry has a unique_id * Add test test_flow_import_incomplete * Add test test_flow_import_duplicate * Re-add testing import_info * Simplify import flow * Remove not needed line Co-authored-by: Martin Hjelmare --- homeassistant/components/upnp/__init__.py | 12 ++-- homeassistant/components/upnp/config_flow.py | 32 ++++----- tests/components/upnp/test_config_flow.py | 70 ++++++++++++++++++-- 3 files changed, 87 insertions(+), 27 deletions(-) diff --git a/homeassistant/components/upnp/__init__.py b/homeassistant/components/upnp/__init__.py index 52cada89333..773a872f33f 100644 --- a/homeassistant/components/upnp/__init__.py +++ b/homeassistant/components/upnp/__init__.py @@ -56,7 +56,9 @@ async def async_discover_and_construct( filtered = [di for di in discovery_infos if di[DISCOVERY_ST] == st] if not filtered: _LOGGER.warning( - 'Wanted UPnP/IGD device with UDN "%s" not found, aborting', udn + 'Wanted UPnP/IGD device with UDN/ST "%s"/"%s" not found, aborting', + udn, + st, ) return None @@ -104,7 +106,7 @@ async def async_setup_entry(hass: HomeAssistantType, config_entry: ConfigEntry) """Set up UPnP/IGD device from a config entry.""" _LOGGER.debug("async_setup_entry, config_entry: %s", config_entry.data) - # discover and construct + # Discover and construct. udn = config_entry.data.get(CONFIG_ENTRY_UDN) st = config_entry.data.get(CONFIG_ENTRY_ST) # pylint: disable=invalid-name try: @@ -116,11 +118,11 @@ async def async_setup_entry(hass: HomeAssistantType, config_entry: ConfigEntry) _LOGGER.info("Unable to create UPnP/IGD, aborting") raise ConfigEntryNotReady - # Save device + # Save device. hass.data[DOMAIN][DOMAIN_DEVICES][device.udn] = device - # Ensure entry has proper unique_id. - if config_entry.unique_id != device.unique_id: + # Ensure entry has a unique_id. + if not config_entry.unique_id: hass.config_entries.async_update_entry( entry=config_entry, unique_id=device.unique_id, diff --git a/homeassistant/components/upnp/config_flow.py b/homeassistant/components/upnp/config_flow.py index 016a5a25017..e7c16ef0df9 100644 --- a/homeassistant/components/upnp/config_flow.py +++ b/homeassistant/components/upnp/config_flow.py @@ -104,19 +104,10 @@ class UpnpFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): """ _LOGGER.debug("async_step_import: import_info: %s", import_info) - if import_info is None: - # Landed here via configuration.yaml entry. - # Any device already added, then abort. - if self._async_current_entries(): - _LOGGER.debug("aborting, already configured") - return self.async_abort(reason="already_configured") - - # Test if import_info isn't already configured. - if import_info is not None and any( - import_info["udn"] == entry.data[CONFIG_ENTRY_UDN] - and import_info["st"] == entry.data[CONFIG_ENTRY_ST] - for entry in self._async_current_entries() - ): + # Landed here via configuration.yaml entry. + # Any device already added, then abort. + if self._async_current_entries(): + _LOGGER.debug("Already configured, aborting") return self.async_abort(reason="already_configured") # Discover devices. @@ -127,8 +118,17 @@ class UpnpFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): _LOGGER.info("No UPnP devices discovered, aborting") return self.async_abort(reason="no_devices_found") - discovery = self._discoveries[0] - return await self._async_create_entry_from_discovery(discovery) + # Ensure complete discovery. + discovery_info = self._discoveries[0] + if DISCOVERY_USN not in discovery_info: + _LOGGER.debug("Incomplete discovery, ignoring") + return self.async_abort(reason="incomplete_discovery") + + # Ensure not already configuring/configured. + usn = discovery_info[DISCOVERY_USN] + await self.async_set_unique_id(usn) + + return await self._async_create_entry_from_discovery(discovery_info) async def async_step_ssdp(self, discovery_info: Mapping): """Handle a discovered UPnP/IGD device. @@ -191,7 +191,7 @@ class UpnpFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): ): """Create an entry from discovery.""" _LOGGER.debug( - "_async_create_entry_from_data: discovery: %s", + "_async_create_entry_from_discovery: discovery: %s", discovery, ) # Get name from device, if not found already. diff --git a/tests/components/upnp/test_config_flow.py b/tests/components/upnp/test_config_flow.py index 98df710d8fe..997811a835f 100644 --- a/tests/components/upnp/test_config_flow.py +++ b/tests/components/upnp/test_config_flow.py @@ -98,10 +98,9 @@ async def test_flow_user(hass: HomeAssistantType): """Test config flow: discovered + configured through user.""" udn = "uuid:device_1" mock_device = MockDevice(udn) - usn = f"{mock_device.udn}::{mock_device.device_type}" discovery_infos = [ { - DISCOVERY_USN: usn, + DISCOVERY_USN: mock_device.unique_id, DISCOVERY_ST: mock_device.device_type, DISCOVERY_UDN: mock_device.udn, DISCOVERY_LOCATION: "dummy", @@ -121,7 +120,7 @@ async def test_flow_user(hass: HomeAssistantType): # Confirmed via step user. result = await hass.config_entries.flow.async_configure( result["flow_id"], - user_input={"usn": usn}, + user_input={"usn": mock_device.unique_id}, ) assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY @@ -132,14 +131,13 @@ async def test_flow_user(hass: HomeAssistantType): } -async def test_flow_config(hass: HomeAssistantType): +async def test_flow_import(hass: HomeAssistantType): """Test config flow: discovered + configured through configuration.yaml.""" udn = "uuid:device_1" mock_device = MockDevice(udn) - usn = f"{mock_device.udn}::{mock_device.device_type}" discovery_infos = [ { - DISCOVERY_USN: usn, + DISCOVERY_USN: mock_device.unique_id, DISCOVERY_ST: mock_device.device_type, DISCOVERY_UDN: mock_device.udn, DISCOVERY_LOCATION: "dummy", @@ -162,6 +160,66 @@ async def test_flow_config(hass: HomeAssistantType): } +async def test_flow_import_duplicate(hass: HomeAssistantType): + """Test config flow: discovered, but already configured.""" + udn = "uuid:device_1" + mock_device = MockDevice(udn) + discovery_infos = [ + { + DISCOVERY_USN: mock_device.unique_id, + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + } + ] + + # Existing entry. + config_entry = MockConfigEntry( + domain=DOMAIN, + data={ + CONFIG_ENTRY_UDN: mock_device.udn, + CONFIG_ENTRY_ST: mock_device.device_type, + }, + options={CONFIG_ENTRY_SCAN_INTERVAL: DEFAULT_SCAN_INTERVAL}, + ) + config_entry.add_to_hass(hass) + + with patch.object( + Device, "async_create_device", AsyncMock(return_value=mock_device) + ), patch.object(Device, "async_discover", AsyncMock(return_value=discovery_infos)): + # Discovered via step import. + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_IMPORT} + ) + + assert result["type"] == data_entry_flow.RESULT_TYPE_ABORT + assert result["reason"] == "already_configured" + + +async def test_flow_import_incomplete(hass: HomeAssistantType): + """Test config flow: incomplete discovery, configured through configuration.yaml.""" + udn = "uuid:device_1" + mock_device = MockDevice(udn) + discovery_infos = [ + { + DISCOVERY_ST: mock_device.device_type, + DISCOVERY_UDN: mock_device.udn, + DISCOVERY_LOCATION: "dummy", + } + ] + + with patch.object( + Device, "async_create_device", AsyncMock(return_value=mock_device) + ), patch.object(Device, "async_discover", AsyncMock(return_value=discovery_infos)): + # Discovered via step import. + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_IMPORT} + ) + + assert result["type"] == data_entry_flow.RESULT_TYPE_ABORT + assert result["reason"] == "incomplete_discovery" + + async def test_options_flow(hass: HomeAssistantType): """Test options flow.""" # Set up config entry.