From 49d73441bf4098f7f8aef1925797abe05715e839 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 22 Aug 2023 16:02:23 -0500 Subject: [PATCH] Abort ESPHome connection when both name and mac address do not match (#98787) --- homeassistant/components/esphome/manager.py | 112 +++++++--- tests/components/esphome/test_init.py | 25 --- tests/components/esphome/test_manager.py | 223 +++++++++++++++++++- 3 files changed, 298 insertions(+), 62 deletions(-) diff --git a/homeassistant/components/esphome/manager.py b/homeassistant/components/esphome/manager.py index c7433080c84..ee0d2371a56 100644 --- a/homeassistant/components/esphome/manager.py +++ b/homeassistant/components/esphome/manager.py @@ -354,51 +354,93 @@ class ESPHomeManager: async def on_connect(self) -> None: """Subscribe to states and list entities on successful API login.""" entry = self.entry + unique_id = entry.unique_id entry_data = self.entry_data reconnect_logic = self.reconnect_logic + assert reconnect_logic is not None, "Reconnect logic must be set" hass = self.hass cli = self.cli + stored_device_name = entry.data.get(CONF_DEVICE_NAME) + unique_id_is_mac_address = unique_id and ":" in unique_id try: device_info = await cli.device_info() + except APIConnectionError as err: + _LOGGER.warning("Error getting device info for %s: %s", self.host, err) + # Re-connection logic will trigger after this + await cli.disconnect() + return - # Migrate config entry to new unique ID if necessary - # This was changed in 2023.1 - if entry.unique_id != format_mac(device_info.mac_address): - hass.config_entries.async_update_entry( - entry, unique_id=format_mac(device_info.mac_address) + device_mac = format_mac(device_info.mac_address) + mac_address_matches = unique_id == device_mac + # + # Migrate config entry to new unique ID if the current + # unique id is not a mac address. + # + # This was changed in 2023.1 + if not mac_address_matches and not unique_id_is_mac_address: + hass.config_entries.async_update_entry(entry, unique_id=device_mac) + + if not mac_address_matches and unique_id_is_mac_address: + # If the unique id is a mac address + # and does not match we have the wrong device and we need + # to abort the connection. This can happen if the DHCP + # server changes the IP address of the device and we end up + # connecting to the wrong device. + _LOGGER.error( + "Unexpected device found at %s; " + "expected `%s` with mac address `%s`, " + "found `%s` with mac address `%s`", + self.host, + stored_device_name, + unique_id, + device_info.name, + device_mac, + ) + await cli.disconnect() + await reconnect_logic.stop() + # We don't want to reconnect to the wrong device + # so we stop the reconnect logic and disconnect + # the client. When discovery finds the new IP address + # for the device, the config entry will be updated + # and we will connect to the correct device when + # the config entry gets reloaded by the discovery + # flow. + return + + # Make sure we have the correct device name stored + # so we can map the device to ESPHome Dashboard config + # If we got here, we know the mac address matches or we + # did a migration to the mac address so we can update + # the device name. + if stored_device_name != device_info.name: + hass.config_entries.async_update_entry( + entry, data={**entry.data, CONF_DEVICE_NAME: device_info.name} + ) + + entry_data.device_info = device_info + assert cli.api_version is not None + entry_data.api_version = cli.api_version + entry_data.available = True + # Reset expected disconnect flag on successful reconnect + # as it will be flipped to False on unexpected disconnect. + # + # We use this to determine if a deep sleep device should + # be marked as unavailable or not. + entry_data.expected_disconnect = True + if device_info.name: + reconnect_logic.name = device_info.name + + if device_info.bluetooth_proxy_feature_flags_compat(cli.api_version): + entry_data.disconnect_callbacks.append( + await async_connect_scanner( + hass, entry, cli, entry_data, self.domain_data.bluetooth_cache ) + ) - # Make sure we have the correct device name stored - # so we can map the device to ESPHome Dashboard config - if entry.data.get(CONF_DEVICE_NAME) != device_info.name: - hass.config_entries.async_update_entry( - entry, data={**entry.data, CONF_DEVICE_NAME: device_info.name} - ) - - entry_data.device_info = device_info - assert cli.api_version is not None - entry_data.api_version = cli.api_version - entry_data.available = True - # Reset expected disconnect flag on successful reconnect - # as it will be flipped to False on unexpected disconnect. - # - # We use this to determine if a deep sleep device should - # be marked as unavailable or not. - entry_data.expected_disconnect = True - if entry_data.device_info.name: - assert reconnect_logic is not None, "Reconnect logic must be set" - reconnect_logic.name = entry_data.device_info.name - - if device_info.bluetooth_proxy_feature_flags_compat(cli.api_version): - entry_data.disconnect_callbacks.append( - await async_connect_scanner( - hass, entry, cli, entry_data, self.domain_data.bluetooth_cache - ) - ) - - self.device_id = _async_setup_device_registry(hass, entry, entry_data) - entry_data.async_update_device_state(hass) + self.device_id = _async_setup_device_registry(hass, entry, entry_data) + entry_data.async_update_device_state(hass) + try: entity_infos, services = await cli.list_entities_services() await entry_data.async_update_static_infos(hass, entry, entity_infos) await _setup_services(hass, entry_data, services) diff --git a/tests/components/esphome/test_init.py b/tests/components/esphome/test_init.py index d3d47a40d66..8e7e228e422 100644 --- a/tests/components/esphome/test_init.py +++ b/tests/components/esphome/test_init.py @@ -1,7 +1,5 @@ """ESPHome set up tests.""" -from unittest.mock import AsyncMock -from aioesphomeapi import DeviceInfo from homeassistant.components.esphome import DOMAIN from homeassistant.const import CONF_HOST, CONF_PASSWORD, CONF_PORT @@ -10,29 +8,6 @@ from homeassistant.core import HomeAssistant from tests.common import MockConfigEntry -async def test_unique_id_updated_to_mac( - hass: HomeAssistant, mock_client, mock_zeroconf: None -) -> None: - """Test we update config entry unique ID to MAC address.""" - entry = MockConfigEntry( - domain=DOMAIN, - data={CONF_HOST: "test.local", CONF_PORT: 6053, CONF_PASSWORD: ""}, - unique_id="mock-config-name", - ) - entry.add_to_hass(hass) - - mock_client.device_info = AsyncMock( - return_value=DeviceInfo( - mac_address="1122334455aa", - ) - ) - - await hass.config_entries.async_setup(entry.entry_id) - await hass.async_block_till_done() - - assert entry.unique_id == "11:22:33:44:55:aa" - - async def test_delete_entry( hass: HomeAssistant, mock_client, mock_zeroconf: None ) -> None: diff --git a/tests/components/esphome/test_manager.py b/tests/components/esphome/test_manager.py index 3bb298024f9..d297dddee4a 100644 --- a/tests/components/esphome/test_manager.py +++ b/tests/components/esphome/test_manager.py @@ -1,11 +1,20 @@ """Test ESPHome manager.""" from collections.abc import Awaitable, Callable +from unittest.mock import AsyncMock -from aioesphomeapi import APIClient, EntityInfo, EntityState, UserService +from aioesphomeapi import APIClient, DeviceInfo, EntityInfo, EntityState, UserService +import pytest -from homeassistant.components.esphome.const import DOMAIN, STABLE_BLE_VERSION_STR +from homeassistant import config_entries +from homeassistant.components import dhcp +from homeassistant.components.esphome.const import ( + CONF_DEVICE_NAME, + DOMAIN, + STABLE_BLE_VERSION_STR, +) from homeassistant.const import CONF_HOST, CONF_PASSWORD, CONF_PORT from homeassistant.core import HomeAssistant +from homeassistant.data_entry_flow import FlowResultType from homeassistant.helpers import issue_registry as ir from .conftest import MockESPHomeDevice @@ -113,3 +122,213 @@ async def test_esphome_device_with_current_bluetooth( ) is None ) + + +async def test_unique_id_updated_to_mac( + hass: HomeAssistant, mock_client, mock_zeroconf: None +) -> None: + """Test we update config entry unique ID to MAC address.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={CONF_HOST: "test.local", CONF_PORT: 6053, CONF_PASSWORD: ""}, + unique_id="mock-config-name", + ) + entry.add_to_hass(hass) + + mock_client.device_info = AsyncMock( + return_value=DeviceInfo( + mac_address="1122334455aa", + ) + ) + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.unique_id == "11:22:33:44:55:aa" + + +async def test_unique_id_not_updated_if_name_same_and_already_mac( + hass: HomeAssistant, mock_client: APIClient, mock_zeroconf: None +) -> None: + """Test we never update the entry unique ID event if the name is the same.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={ + CONF_HOST: "test.local", + CONF_PORT: 6053, + CONF_PASSWORD: "", + CONF_DEVICE_NAME: "test", + }, + unique_id="11:22:33:44:55:aa", + ) + entry.add_to_hass(hass) + + mock_client.device_info = AsyncMock( + return_value=DeviceInfo(mac_address="1122334455ab", name="test") + ) + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + # Mac should never update + assert entry.unique_id == "11:22:33:44:55:aa" + + +async def test_unique_id_updated_if_name_unset_and_already_mac( + hass: HomeAssistant, mock_client: APIClient, mock_zeroconf: None +) -> None: + """Test we never update config entry unique ID even if the name is unset.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={CONF_HOST: "test.local", CONF_PORT: 6053, CONF_PASSWORD: ""}, + unique_id="11:22:33:44:55:aa", + ) + entry.add_to_hass(hass) + + mock_client.device_info = AsyncMock( + return_value=DeviceInfo(mac_address="1122334455ab", name="test") + ) + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + # Mac should never update + assert entry.unique_id == "11:22:33:44:55:aa" + + +async def test_unique_id_not_updated_if_name_different_and_already_mac( + hass: HomeAssistant, mock_client: APIClient, mock_zeroconf: None +) -> None: + """Test we do not update config entry unique ID if the name is different.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={ + CONF_HOST: "test.local", + CONF_PORT: 6053, + CONF_PASSWORD: "", + CONF_DEVICE_NAME: "test", + }, + unique_id="11:22:33:44:55:aa", + ) + entry.add_to_hass(hass) + + mock_client.device_info = AsyncMock( + return_value=DeviceInfo(mac_address="1122334455ab", name="different") + ) + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + # Mac should not be updated because name is different + assert entry.unique_id == "11:22:33:44:55:aa" + # Name should not be updated either + assert entry.data[CONF_DEVICE_NAME] == "test" + + +async def test_name_updated_only_if_mac_matches( + hass: HomeAssistant, mock_client: APIClient, mock_zeroconf: None +) -> None: + """Test we update config entry name only if the mac matches.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={ + CONF_HOST: "test.local", + CONF_PORT: 6053, + CONF_PASSWORD: "", + CONF_DEVICE_NAME: "old", + }, + unique_id="11:22:33:44:55:aa", + ) + entry.add_to_hass(hass) + + mock_client.device_info = AsyncMock( + return_value=DeviceInfo(mac_address="1122334455aa", name="new") + ) + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.unique_id == "11:22:33:44:55:aa" + assert entry.data[CONF_DEVICE_NAME] == "new" + + +async def test_name_updated_only_if_mac_was_unset( + hass: HomeAssistant, mock_client: APIClient, mock_zeroconf: None +) -> None: + """Test we update config entry name if the old unique id was not a mac.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={ + CONF_HOST: "test.local", + CONF_PORT: 6053, + CONF_PASSWORD: "", + CONF_DEVICE_NAME: "old", + }, + unique_id="notamac", + ) + entry.add_to_hass(hass) + + mock_client.device_info = AsyncMock( + return_value=DeviceInfo(mac_address="1122334455aa", name="new") + ) + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert entry.unique_id == "11:22:33:44:55:aa" + assert entry.data[CONF_DEVICE_NAME] == "new" + + +async def test_connection_aborted_wrong_device( + hass: HomeAssistant, + mock_client: APIClient, + mock_zeroconf: None, + caplog: pytest.LogCaptureFixture, +) -> None: + """Test we abort the connection if the unique id is a mac and neither name or mac match.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={ + CONF_HOST: "192.168.43.183", + CONF_PORT: 6053, + CONF_PASSWORD: "", + CONF_DEVICE_NAME: "test", + }, + unique_id="11:22:33:44:55:aa", + ) + entry.add_to_hass(hass) + + mock_client.device_info = AsyncMock( + return_value=DeviceInfo(mac_address="1122334455ab", name="different") + ) + + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert ( + "Unexpected device found at 192.168.43.183; expected `test` " + "with mac address `11:22:33:44:55:aa`, found `different` " + "with mac address `11:22:33:44:55:ab`" in caplog.text + ) + + caplog.clear() + # Make sure discovery triggers a reconnect to the correct device + service_info = dhcp.DhcpServiceInfo( + ip="192.168.43.184", + hostname="test", + macaddress="1122334455aa", + ) + new_info = AsyncMock( + return_value=DeviceInfo(mac_address="1122334455aa", name="test") + ) + mock_client.device_info = new_info + result = await hass.config_entries.flow.async_init( + "esphome", context={"source": config_entries.SOURCE_DHCP}, data=service_info + ) + + assert result["type"] == FlowResultType.ABORT + assert result["reason"] == "already_configured" + assert entry.data[CONF_HOST] == "192.168.43.184" + await hass.async_block_till_done() + assert len(new_info.mock_calls) == 1 + assert "Unexpected device found at" not in caplog.text