Fix ONVIF config entry unique ID (#36008)

* fallback to device serial number if no mac available

* make password optional to fix #35904

* update tests to reflect new flow

* fix snake case and AsyncMock

* add comments around why weird things are being done

homeassistant/components/onvif/base.py

@@ -2,6 +2,7 @@
 from homeassistant.helpers.device_registry import CONNECTION_NETWORK_MAC
 from homeassistant.helpers.entity import Entity
 
+from .const import DOMAIN
 from .device import ONVIFDevice
 from .models import Profile
 
@@ -11,8 +12,8 @@ class ONVIFBaseEntity(Entity):
 
     def __init__(self, device: ONVIFDevice, profile: Profile = None) -> None:
         """Initialize the ONVIF entity."""
-        self.device = device
-        self.profile = profile
+        self.device: ONVIFDevice = device
+        self.profile: Profile = profile
 
     @property
     def available(self):
@@ -22,10 +23,25 @@ def available(self):
     @property
     def device_info(self):
         """Return a device description for device registry."""
-        return {
-            "connections": {(CONNECTION_NETWORK_MAC, self.device.info.mac)},
+        device_info = {
             "manufacturer": self.device.info.manufacturer,
             "model": self.device.info.model,
             "name": self.device.name,
             "sw_version": self.device.info.fw_version,
         }
+
+        # MAC address is not always available, and given the number
+        # of non-conformant ONVIF devices we have historically supported,
+        # we can not guarantee serial number either.  Due to this, we have
+        # adopted an either/or approach in the config entry setup, and can
+        # guarantee that one or the other will be populated.
+        # See: https://github.com/home-assistant/core/issues/35883
+        if self.device.info.serial_number:
+            device_info["identifiers"] = {(DOMAIN, self.device.info.serial_number)}
+
+        if self.device.info.mac:
+            device_info["connections"] = {
+                (CONNECTION_NETWORK_MAC, self.device.info.mac)
+            }
+
+        return device_info

homeassistant/components/onvif/camera.py

@@ -96,8 +96,8 @@ def name(self) -> str:
     def unique_id(self) -> str:
         """Return a unique ID."""
         if self.profile.index:
-            return f"{self.device.info.mac}_{self.profile.index}"
-        return self.device.info.mac
+            return f"{self.device.info.mac or self.device.info.serial_number}_{self.profile.index}"
+        return self.device.info.mac or self.device.info.serial_number
 
     @property
     def entity_registry_enabled_default(self) -> bool:

homeassistant/components/onvif/config_flow.py

@@ -169,10 +169,16 @@ async def async_step_auth(self, user_input=None):
             self.onvif_config[CONF_PASSWORD] = user_input[CONF_PASSWORD]
             return await self.async_step_profiles()
 
+        # Password is optional and default empty due to some cameras not
+        # allowing you to change ONVIF user settings.
+        # See https://github.com/home-assistant/core/issues/35904
         return self.async_show_form(
             step_id="auth",
             data_schema=vol.Schema(
-                {vol.Required(CONF_USERNAME): str, vol.Required(CONF_PASSWORD): str}
+                {
+                    vol.Required(CONF_USERNAME): str,
+                    vol.Optional(CONF_PASSWORD, default=""): str,
+                }
             ),
         )
 
@@ -195,15 +201,21 @@ async def async_step_profiles(self, user_input=None):
         await device.update_xaddrs()
 
         try:
+            device_mgmt = device.create_devicemgmt_service()
+
             # Get the MAC address to use as the unique ID for the config flow
             if not self.device_id:
-                devicemgmt = device.create_devicemgmt_service()
-                network_interfaces = await devicemgmt.GetNetworkInterfaces()
+                network_interfaces = await device_mgmt.GetNetworkInterfaces()
                 for interface in network_interfaces:
                     if interface.Enabled:
                         self.device_id = interface.Info.HwAddress
 
-            if self.device_id is None:
+            # If no network interfaces are exposed, fallback to serial number
+            if not self.device_id:
+                device_info = await device_mgmt.GetDeviceInformation()
+                self.device_id = device_info.SerialNumber
+
+            if not self.device_id:
                 return self.async_abort(reason="no_mac")
 
             await self.async_set_unique_id(self.device_id, raise_on_progress=False)

homeassistant/components/onvif/device.py

@@ -148,12 +148,12 @@ async def async_stop(self, event=None):
     async def async_check_date_and_time(self) -> None:
         """Warns if device and system date not synced."""
         LOGGER.debug("Setting up the ONVIF device management service")
-        devicemgmt = self.device.create_devicemgmt_service()
+        device_mgmt = self.device.create_devicemgmt_service()
 
         LOGGER.debug("Retrieving current device date/time")
         try:
             system_date = dt_util.utcnow()
-            device_time = await devicemgmt.GetSystemDateAndTime()
+            device_time = await device_mgmt.GetSystemDateAndTime()
             if not device_time:
                 LOGGER.debug(
                     """Couldn't get device '%s' date/time.
@@ -212,13 +212,22 @@ async def async_check_date_and_time(self) -> None:
 
     async def async_get_device_info(self) -> DeviceInfo:
         """Obtain information about this device."""
-        devicemgmt = self.device.create_devicemgmt_service()
-        device_info = await devicemgmt.GetDeviceInformation()
+        device_mgmt = self.device.create_devicemgmt_service()
+        device_info = await device_mgmt.GetDeviceInformation()
+
+        # Grab the last MAC address for backwards compatibility
+        mac = None
+        network_interfaces = await device_mgmt.GetNetworkInterfaces()
+        for interface in network_interfaces:
+            if interface.Enabled:
+                mac = interface.Info.HwAddress
+
         return DeviceInfo(
             device_info.Manufacturer,
             device_info.Model,
             device_info.FirmwareVersion,
-            self.config_entry.unique_id,
+            device_info.SerialNumber,
+            mac,
         )
 
     async def async_get_capabilities(self):

homeassistant/components/onvif/event.py

@@ -62,7 +62,8 @@ def remove_listener() -> None:
     @callback
     def async_remove_listener(self, update_callback: CALLBACK_TYPE) -> None:
         """Remove data update."""
-        self._listeners.remove(update_callback)
+        if update_callback in self._listeners:
+            self._listeners.remove(update_callback)
 
         if not self._listeners and self._unsub_refresh:
             self._unsub_refresh()
@@ -93,6 +94,8 @@ async def async_start(self) -> bool:
 
     async def async_stop(self) -> None:
         """Unsubscribe from events."""
+        self._listeners = []
+
         if not self._subscription:
             return
 

homeassistant/components/onvif/models.py

@@ -10,6 +10,7 @@ class DeviceInfo:
     manufacturer: str = None
     model: str = None
     fw_version: str = None
+    serial_number: str = None
     mac: str = None
 
 

tests/components/onvif/test_config_flow.py

@@ -1,13 +1,11 @@
 """Test ONVIF config flow."""
-from asyncio import Future
-
-from asynctest import MagicMock, patch
 from onvif.exceptions import ONVIFError
 from zeep.exceptions import Fault
 
 from homeassistant import config_entries, data_entry_flow
 from homeassistant.components.onvif import config_flow
 
+from tests.async_mock import AsyncMock, MagicMock, patch
 from tests.common import MockConfigEntry
 
 URN = "urn:uuid:123456789"
@@ -17,6 +15,7 @@
 USERNAME = "admin"
 PASSWORD = "12345"
 MAC = "aa:bb:cc:dd:ee"
+SERIAL_NUMBER = "ABCDEFGHIJK"
 
 DISCOVERY = [
     {
@@ -37,18 +36,25 @@
 
 
 def setup_mock_onvif_camera(
-    mock_onvif_camera, with_h264=True, two_profiles=False, with_interfaces=True
+    mock_onvif_camera,
+    with_h264=True,
+    two_profiles=False,
+    with_interfaces=True,
+    with_serial=True,
 ):
     """Prepare mock onvif.ONVIFCamera."""
     devicemgmt = MagicMock()
 
+    device_info = MagicMock()
+    device_info.SerialNumber = SERIAL_NUMBER if with_serial else None
+    devicemgmt.GetDeviceInformation = AsyncMock(return_value=device_info)
+
     interface = MagicMock()
     interface.Enabled = True
     interface.Info.HwAddress = MAC
 
-    devicemgmt.GetNetworkInterfaces.return_value = Future()
-    devicemgmt.GetNetworkInterfaces.return_value.set_result(
-        [interface] if with_interfaces else []
+    devicemgmt.GetNetworkInterfaces = AsyncMock(
+        return_value=[interface] if with_interfaces else []
     )
 
     media_service = MagicMock()
@@ -58,11 +64,9 @@ def setup_mock_onvif_camera(
     profile2 = MagicMock()
     profile2.VideoEncoderConfiguration.Encoding = "H264" if two_profiles else "MJPEG"
 
-    media_service.GetProfiles.return_value = Future()
-    media_service.GetProfiles.return_value.set_result([profile1, profile2])
+    media_service.GetProfiles = AsyncMock(return_value=[profile1, profile2])
 
-    mock_onvif_camera.update_xaddrs.return_value = Future()
-    mock_onvif_camera.update_xaddrs.return_value.set_result(True)
+    mock_onvif_camera.update_xaddrs = AsyncMock(return_value=True)
     mock_onvif_camera.create_devicemgmt_service = MagicMock(return_value=devicemgmt)
     mock_onvif_camera.create_media_service = MagicMock(return_value=media_service)
 
@@ -116,8 +120,7 @@ def setup_mock_discovery(
 
 def setup_mock_device(mock_device):
     """Prepare mock ONVIFDevice."""
-    mock_device.async_setup.return_value = Future()
-    mock_device.async_setup.return_value.set_result(True)
+    mock_device.async_setup = AsyncMock(return_value=True)
 
     def mock_constructor(hass, config):
         """Fake the controller constructor."""
@@ -390,11 +393,48 @@ async def test_flow_manual_entry(hass):
 
 
 async def test_flow_import_no_mac(hass):
-    """Test that config flow fails when no MAC available."""
+    """Test that config flow uses Serial Number when no MAC available."""
     with patch(
         "homeassistant.components.onvif.config_flow.get_device"
-    ) as mock_onvif_camera:
+    ) as mock_onvif_camera, patch(
+        "homeassistant.components.onvif.ONVIFDevice"
+    ) as mock_device:
         setup_mock_onvif_camera(mock_onvif_camera, with_interfaces=False)
+        setup_mock_device(mock_device)
+
+        result = await hass.config_entries.flow.async_init(
+            config_flow.DOMAIN,
+            context={"source": config_entries.SOURCE_IMPORT},
+            data={
+                config_flow.CONF_NAME: NAME,
+                config_flow.CONF_HOST: HOST,
+                config_flow.CONF_PORT: PORT,
+                config_flow.CONF_USERNAME: USERNAME,
+                config_flow.CONF_PASSWORD: PASSWORD,
+            },
+        )
+
+        await hass.async_block_till_done()
+
+        assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY
+        assert result["title"] == f"{NAME} - {SERIAL_NUMBER}"
+        assert result["data"] == {
+            config_flow.CONF_NAME: NAME,
+            config_flow.CONF_HOST: HOST,
+            config_flow.CONF_PORT: PORT,
+            config_flow.CONF_USERNAME: USERNAME,
+            config_flow.CONF_PASSWORD: PASSWORD,
+        }
+
+
+async def test_flow_import_no_mac_or_serial(hass):
+    """Test that config flow fails when no MAC or Serial Number available."""
+    with patch(
+        "homeassistant.components.onvif.config_flow.get_device"
+    ) as mock_onvif_camera:
+        setup_mock_onvif_camera(
+            mock_onvif_camera, with_interfaces=False, with_serial=False
+        )
 
         result = await hass.config_entries.flow.async_init(
             config_flow.DOMAIN,