diff --git a/homeassistant/components/onvif/base.py b/homeassistant/components/onvif/base.py index 43a846cac37..cca84c4562a 100644 --- a/homeassistant/components/onvif/base.py +++ b/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 @@ class ONVIFBaseEntity(Entity): @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 diff --git a/homeassistant/components/onvif/camera.py b/homeassistant/components/onvif/camera.py index 570b99bfe3a..7f97e0fbea4 100644 --- a/homeassistant/components/onvif/camera.py +++ b/homeassistant/components/onvif/camera.py @@ -96,8 +96,8 @@ class ONVIFCameraEntity(ONVIFBaseEntity, Camera): 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: diff --git a/homeassistant/components/onvif/config_flow.py b/homeassistant/components/onvif/config_flow.py index 1dba697380d..29784b25429 100644 --- a/homeassistant/components/onvif/config_flow.py +++ b/homeassistant/components/onvif/config_flow.py @@ -169,10 +169,16 @@ class OnvifFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): 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 @@ class OnvifFlowHandler(config_entries.ConfigFlow, domain=DOMAIN): 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) diff --git a/homeassistant/components/onvif/device.py b/homeassistant/components/onvif/device.py index 938c960080f..a28e9c4d6c2 100644 --- a/homeassistant/components/onvif/device.py +++ b/homeassistant/components/onvif/device.py @@ -148,12 +148,12 @@ class ONVIFDevice: 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 @@ class ONVIFDevice: 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): diff --git a/homeassistant/components/onvif/event.py b/homeassistant/components/onvif/event.py index 183ad0ab532..7a0113177c4 100644 --- a/homeassistant/components/onvif/event.py +++ b/homeassistant/components/onvif/event.py @@ -62,7 +62,8 @@ class EventManager: @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 @@ class EventManager: async def async_stop(self) -> None: """Unsubscribe from events.""" + self._listeners = [] + if not self._subscription: return diff --git a/homeassistant/components/onvif/models.py b/homeassistant/components/onvif/models.py index 686d9fecbda..2a129d3bc44 100644 --- a/homeassistant/components/onvif/models.py +++ b/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 diff --git a/tests/components/onvif/test_config_flow.py b/tests/components/onvif/test_config_flow.py index c709c5e6f67..5c41349bbbe 100644 --- a/tests/components/onvif/test_config_flow.py +++ b/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 @@ PORT = 80 USERNAME = "admin" PASSWORD = "12345" MAC = "aa:bb:cc:dd:ee" +SERIAL_NUMBER = "ABCDEFGHIJK" DISCOVERY = [ { @@ -37,18 +36,25 @@ DISCOVERY = [ 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, 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) + setup_mock_onvif_camera( + mock_onvif_camera, with_interfaces=False, with_serial=False + ) result = await hass.config_entries.flow.async_init( config_flow.DOMAIN,