From 67b18aef5b9b1877b304ef692e6642a7f64bc73a Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Fri, 10 Jul 2020 09:37:36 -0700 Subject: [PATCH] Fix Hue homekit discovery (#37694) Co-authored-by: Franck Nijhof --- homeassistant/components/hue/__init__.py | 30 ++++++- homeassistant/components/hue/config_flow.py | 12 --- tests/components/hue/test_config_flow.py | 24 +----- tests/components/hue/test_init.py | 88 +++++++++++++++------ 4 files changed, 96 insertions(+), 58 deletions(-) diff --git a/homeassistant/components/hue/__init__.py b/homeassistant/components/hue/__init__.py index a466405094a..1131d68baec 100644 --- a/homeassistant/components/hue/__init__.py +++ b/homeassistant/components/hue/__init__.py @@ -161,11 +161,37 @@ async def async_setup_entry( config = bridge.api.config # For backwards compat + unique_id = normalize_bridge_id(config.bridgeid) if entry.unique_id is None: - hass.config_entries.async_update_entry( - entry, unique_id=normalize_bridge_id(config.bridgeid) + hass.config_entries.async_update_entry(entry, unique_id=unique_id) + + # For recovering from bug where we incorrectly assumed homekit ID = bridge ID + elif entry.unique_id != unique_id: + # Find entries with this unique ID + other_entry = next( + ( + entry + for entry in hass.config_entries.async_entries(DOMAIN) + if entry.unique_id == unique_id + ), + None, ) + if other_entry is None: + # If no other entry, update unique ID of this entry ID. + hass.config_entries.async_update_entry(entry, unique_id=unique_id) + + elif other_entry.source == config_entries.SOURCE_IGNORE: + # There is another entry but it is ignored, delete that one and update this one + hass.async_create_task( + hass.config_entries.async_remove(other_entry.entry_id) + ) + hass.config_entries.async_update_entry(entry, unique_id=unique_id) + else: + # There is another entry that already has the right unique ID. Delete this entry + hass.async_create_task(hass.config_entries.async_remove(entry.entry_id)) + return False + device_registry = await dr.async_get_registry(hass) device_registry.async_get_or_create( config_entry_id=entry.entry_id, diff --git a/homeassistant/components/hue/config_flow.py b/homeassistant/components/hue/config_flow.py index 63cd5ec80b4..66c93ead846 100644 --- a/homeassistant/components/hue/config_flow.py +++ b/homeassistant/components/hue/config_flow.py @@ -203,18 +203,6 @@ class HueFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): self.bridge = bridge return await self.async_step_link() - async def async_step_homekit(self, homekit_info): - """Handle HomeKit discovery.""" - bridge = self._async_get_bridge( - homekit_info["host"], homekit_info["properties"]["id"] - ) - - await self.async_set_unique_id(bridge.id) - self._abort_if_unique_id_configured(updates={CONF_HOST: bridge.host}) - - self.bridge = bridge - return await self.async_step_link() - async def async_step_import(self, import_info): """Import a new bridge as a config entry. diff --git a/tests/components/hue/test_config_flow.py b/tests/components/hue/test_config_flow.py index 06a0bcedc81..42a6f525319 100644 --- a/tests/components/hue/test_config_flow.py +++ b/tests/components/hue/test_config_flow.py @@ -538,8 +538,10 @@ async def test_creating_entry_removes_entries_for_same_host_or_bridge(hass): assert new_entry.unique_id == "id-1234" -async def test_bridge_homekit(hass): +async def test_bridge_homekit(hass, aioclient_mock): """Test a bridge being discovered via HomeKit.""" + aioclient_mock.get(URL_NUPNP, json=[{"internalipaddress": "1.2.3.4", "id": "bla"}]) + result = await hass.config_entries.flow.async_init( const.DOMAIN, context={"source": "homekit"}, @@ -552,7 +554,7 @@ async def test_bridge_homekit(hass): ) assert result["type"] == "form" - assert result["step_id"] == "link" + assert result["step_id"] == "init" async def test_bridge_import_already_configured(hass): @@ -609,24 +611,6 @@ async def test_ssdp_discovery_update_configuration(hass): assert entry.data["host"] == "1.1.1.1" -async def test_homekit_discovery_update_configuration(hass): - """Test if a discovered bridge is configured and updated with new host.""" - entry = MockConfigEntry( - domain="hue", unique_id="aabbccddeeff", data={"host": "0.0.0.0"} - ) - entry.add_to_hass(hass) - - result = await hass.config_entries.flow.async_init( - const.DOMAIN, - context={"source": "homekit"}, - data={"host": "1.1.1.1", "properties": {"id": "aa:bb:cc:dd:ee:ff"}}, - ) - - assert result["type"] == "abort" - assert result["reason"] == "already_configured" - assert entry.data["host"] == "1.1.1.1" - - async def test_options_flow(hass): """Test options config flow.""" entry = MockConfigEntry( diff --git a/tests/components/hue/test_init.py b/tests/components/hue/test_init.py index 033b5ae056a..b1dc024998e 100644 --- a/tests/components/hue/test_init.py +++ b/tests/components/hue/test_init.py @@ -1,11 +1,23 @@ """Test Hue setup process.""" from unittest.mock import Mock +import pytest + +from homeassistant import config_entries from homeassistant.components import hue from homeassistant.setup import async_setup_component from tests.async_mock import AsyncMock, patch -from tests.common import MockConfigEntry, mock_coro +from tests.common import MockConfigEntry + + +@pytest.fixture +def mock_bridge_setup(): + """Mock bridge setup.""" + with patch.object(hue, "HueBridge") as mock_bridge: + mock_bridge.return_value.async_setup = AsyncMock(return_value=True) + mock_bridge.return_value.api.config = Mock(bridgeid="mock-id") + yield mock_bridge.return_value async def test_setup_with_no_config(hass): @@ -23,7 +35,7 @@ async def test_setup_defined_hosts_known_auth(hass): """Test we don't initiate a config entry if config bridge is known.""" MockConfigEntry(domain="hue", data={"host": "0.0.0.0"}).add_to_hass(hass) - with patch.object(hue, "async_setup_entry", return_value=mock_coro(True)): + with patch.object(hue, "async_setup_entry", return_value=True): assert ( await async_setup_component( hass, @@ -143,43 +155,71 @@ async def test_config_passed_to_config_entry(hass): } -async def test_unload_entry(hass): +async def test_unload_entry(hass, mock_bridge_setup): """Test being able to unload an entry.""" entry = MockConfigEntry(domain=hue.DOMAIN, data={"host": "0.0.0.0"}) entry.add_to_hass(hass) - with patch.object(hue, "HueBridge") as mock_bridge, patch( - "homeassistant.helpers.device_registry.async_get_registry", - return_value=mock_coro(Mock()), - ): - mock_bridge.return_value.async_setup = AsyncMock(return_value=True) - mock_bridge.return_value.api.config = Mock(bridgeid="aabbccddeeff") - assert await async_setup_component(hass, hue.DOMAIN, {}) is True + assert await async_setup_component(hass, hue.DOMAIN, {}) is True + assert len(mock_bridge_setup.mock_calls) == 1 - assert len(mock_bridge.return_value.mock_calls) == 1 - - mock_bridge.return_value.async_reset = AsyncMock(return_value=True) + mock_bridge_setup.async_reset = AsyncMock(return_value=True) assert await hue.async_unload_entry(hass, entry) - assert len(mock_bridge.return_value.async_reset.mock_calls) == 1 + assert len(mock_bridge_setup.async_reset.mock_calls) == 1 assert hass.data[hue.DOMAIN] == {} -async def test_setting_unique_id(hass): +async def test_setting_unique_id(hass, mock_bridge_setup): """Test we set unique ID if not set yet.""" entry = MockConfigEntry(domain=hue.DOMAIN, data={"host": "0.0.0.0"}) entry.add_to_hass(hass) - - with patch.object(hue, "HueBridge") as mock_bridge, patch( - "homeassistant.helpers.device_registry.async_get_registry", - return_value=mock_coro(Mock()), - ): - mock_bridge.return_value.async_setup = AsyncMock(return_value=True) - mock_bridge.return_value.api.config = Mock(bridgeid="mock-id") - assert await async_setup_component(hass, hue.DOMAIN, {}) is True - + assert await async_setup_component(hass, hue.DOMAIN, {}) is True assert entry.unique_id == "mock-id" +async def test_fixing_unique_id_no_other(hass, mock_bridge_setup): + """Test we set unique ID if not set yet.""" + entry = MockConfigEntry( + domain=hue.DOMAIN, data={"host": "0.0.0.0"}, unique_id="invalid-id" + ) + entry.add_to_hass(hass) + assert await async_setup_component(hass, hue.DOMAIN, {}) is True + assert entry.unique_id == "mock-id" + + +async def test_fixing_unique_id_other_ignored(hass, mock_bridge_setup): + """Test we set unique ID if not set yet.""" + MockConfigEntry( + domain=hue.DOMAIN, + data={"host": "0.0.0.0"}, + unique_id="mock-id", + source=config_entries.SOURCE_IGNORE, + ).add_to_hass(hass) + entry = MockConfigEntry( + domain=hue.DOMAIN, data={"host": "0.0.0.0"}, unique_id="invalid-id", + ) + entry.add_to_hass(hass) + assert await async_setup_component(hass, hue.DOMAIN, {}) is True + await hass.async_block_till_done() + assert entry.unique_id == "mock-id" + assert hass.config_entries.async_entries() == [entry] + + +async def test_fixing_unique_id_other_correct(hass, mock_bridge_setup): + """Test we remove config entry if another one has correct ID.""" + correct_entry = MockConfigEntry( + domain=hue.DOMAIN, data={"host": "0.0.0.0"}, unique_id="mock-id", + ) + correct_entry.add_to_hass(hass) + entry = MockConfigEntry( + domain=hue.DOMAIN, data={"host": "0.0.0.0"}, unique_id="invalid-id", + ) + entry.add_to_hass(hass) + assert await async_setup_component(hass, hue.DOMAIN, {}) is True + await hass.async_block_till_done() + assert hass.config_entries.async_entries() == [correct_entry] + + async def test_security_vuln_check(hass): """Test that we report security vulnerabilities.""" assert await async_setup_component(hass, "persistent_notification", {})