Abort ESPHome connection when both name and mac address do not match (#98787)

homeassistant/components/esphome/manager.py

@@ -354,23 +354,65 @@ 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
+        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 entry.unique_id != format_mac(device_info.mac_address):
+        if not mac_address_matches and not unique_id_is_mac_address:
-                hass.config_entries.async_update_entry(
+            hass.config_entries.async_update_entry(entry, unique_id=device_mac)
-                    entry, unique_id=format_mac(device_info.mac_address)
+
+        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 entry.data.get(CONF_DEVICE_NAME) != device_info.name:
+        # 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}
             )
@@ -385,9 +427,8 @@ class ESPHomeManager:
         # 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:
+        if device_info.name:
-                assert reconnect_logic is not None, "Reconnect logic must be set"
+            reconnect_logic.name = device_info.name
-                reconnect_logic.name = entry_data.device_info.name
 
         if device_info.bluetooth_proxy_feature_flags_compat(cli.api_version):
             entry_data.disconnect_callbacks.append(
@@ -399,6 +440,7 @@ class ESPHomeManager:
         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)

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:

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