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
This commit is contained in:
Jason Hunter 2020-05-24 15:50:50 -04:00 committed by GitHub
parent ed7ac3c1f7
commit bd8848e57a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 112 additions and 31 deletions

View File

@ -2,6 +2,7 @@
from homeassistant.helpers.device_registry import CONNECTION_NETWORK_MAC from homeassistant.helpers.device_registry import CONNECTION_NETWORK_MAC
from homeassistant.helpers.entity import Entity from homeassistant.helpers.entity import Entity
from .const import DOMAIN
from .device import ONVIFDevice from .device import ONVIFDevice
from .models import Profile from .models import Profile
@ -11,8 +12,8 @@ class ONVIFBaseEntity(Entity):
def __init__(self, device: ONVIFDevice, profile: Profile = None) -> None: def __init__(self, device: ONVIFDevice, profile: Profile = None) -> None:
"""Initialize the ONVIF entity.""" """Initialize the ONVIF entity."""
self.device = device self.device: ONVIFDevice = device
self.profile = profile self.profile: Profile = profile
@property @property
def available(self): def available(self):
@ -22,10 +23,25 @@ class ONVIFBaseEntity(Entity):
@property @property
def device_info(self): def device_info(self):
"""Return a device description for device registry.""" """Return a device description for device registry."""
return { device_info = {
"connections": {(CONNECTION_NETWORK_MAC, self.device.info.mac)},
"manufacturer": self.device.info.manufacturer, "manufacturer": self.device.info.manufacturer,
"model": self.device.info.model, "model": self.device.info.model,
"name": self.device.name, "name": self.device.name,
"sw_version": self.device.info.fw_version, "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

View File

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

View File

@ -169,10 +169,16 @@ class OnvifFlowHandler(config_entries.ConfigFlow, domain=DOMAIN):
self.onvif_config[CONF_PASSWORD] = user_input[CONF_PASSWORD] self.onvif_config[CONF_PASSWORD] = user_input[CONF_PASSWORD]
return await self.async_step_profiles() 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( return self.async_show_form(
step_id="auth", step_id="auth",
data_schema=vol.Schema( 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() await device.update_xaddrs()
try: try:
device_mgmt = device.create_devicemgmt_service()
# Get the MAC address to use as the unique ID for the config flow # Get the MAC address to use as the unique ID for the config flow
if not self.device_id: if not self.device_id:
devicemgmt = device.create_devicemgmt_service() network_interfaces = await device_mgmt.GetNetworkInterfaces()
network_interfaces = await devicemgmt.GetNetworkInterfaces()
for interface in network_interfaces: for interface in network_interfaces:
if interface.Enabled: if interface.Enabled:
self.device_id = interface.Info.HwAddress 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") return self.async_abort(reason="no_mac")
await self.async_set_unique_id(self.device_id, raise_on_progress=False) await self.async_set_unique_id(self.device_id, raise_on_progress=False)

View File

@ -148,12 +148,12 @@ class ONVIFDevice:
async def async_check_date_and_time(self) -> None: async def async_check_date_and_time(self) -> None:
"""Warns if device and system date not synced.""" """Warns if device and system date not synced."""
LOGGER.debug("Setting up the ONVIF device management service") 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") LOGGER.debug("Retrieving current device date/time")
try: try:
system_date = dt_util.utcnow() system_date = dt_util.utcnow()
device_time = await devicemgmt.GetSystemDateAndTime() device_time = await device_mgmt.GetSystemDateAndTime()
if not device_time: if not device_time:
LOGGER.debug( LOGGER.debug(
"""Couldn't get device '%s' date/time. """Couldn't get device '%s' date/time.
@ -212,13 +212,22 @@ class ONVIFDevice:
async def async_get_device_info(self) -> DeviceInfo: async def async_get_device_info(self) -> DeviceInfo:
"""Obtain information about this device.""" """Obtain information about this device."""
devicemgmt = self.device.create_devicemgmt_service() device_mgmt = self.device.create_devicemgmt_service()
device_info = await devicemgmt.GetDeviceInformation() 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( return DeviceInfo(
device_info.Manufacturer, device_info.Manufacturer,
device_info.Model, device_info.Model,
device_info.FirmwareVersion, device_info.FirmwareVersion,
self.config_entry.unique_id, device_info.SerialNumber,
mac,
) )
async def async_get_capabilities(self): async def async_get_capabilities(self):

View File

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

View File

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

View File

@ -1,13 +1,11 @@
"""Test ONVIF config flow.""" """Test ONVIF config flow."""
from asyncio import Future
from asynctest import MagicMock, patch
from onvif.exceptions import ONVIFError from onvif.exceptions import ONVIFError
from zeep.exceptions import Fault from zeep.exceptions import Fault
from homeassistant import config_entries, data_entry_flow from homeassistant import config_entries, data_entry_flow
from homeassistant.components.onvif import config_flow from homeassistant.components.onvif import config_flow
from tests.async_mock import AsyncMock, MagicMock, patch
from tests.common import MockConfigEntry from tests.common import MockConfigEntry
URN = "urn:uuid:123456789" URN = "urn:uuid:123456789"
@ -17,6 +15,7 @@ PORT = 80
USERNAME = "admin" USERNAME = "admin"
PASSWORD = "12345" PASSWORD = "12345"
MAC = "aa:bb:cc:dd:ee" MAC = "aa:bb:cc:dd:ee"
SERIAL_NUMBER = "ABCDEFGHIJK"
DISCOVERY = [ DISCOVERY = [
{ {
@ -37,18 +36,25 @@ DISCOVERY = [
def setup_mock_onvif_camera( 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.""" """Prepare mock onvif.ONVIFCamera."""
devicemgmt = MagicMock() 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 = MagicMock()
interface.Enabled = True interface.Enabled = True
interface.Info.HwAddress = MAC interface.Info.HwAddress = MAC
devicemgmt.GetNetworkInterfaces.return_value = Future() devicemgmt.GetNetworkInterfaces = AsyncMock(
devicemgmt.GetNetworkInterfaces.return_value.set_result( return_value=[interface] if with_interfaces else []
[interface] if with_interfaces else []
) )
media_service = MagicMock() media_service = MagicMock()
@ -58,11 +64,9 @@ def setup_mock_onvif_camera(
profile2 = MagicMock() profile2 = MagicMock()
profile2.VideoEncoderConfiguration.Encoding = "H264" if two_profiles else "MJPEG" profile2.VideoEncoderConfiguration.Encoding = "H264" if two_profiles else "MJPEG"
media_service.GetProfiles.return_value = Future() media_service.GetProfiles = AsyncMock(return_value=[profile1, profile2])
media_service.GetProfiles.return_value.set_result([profile1, profile2])
mock_onvif_camera.update_xaddrs.return_value = Future() mock_onvif_camera.update_xaddrs = AsyncMock(return_value=True)
mock_onvif_camera.update_xaddrs.return_value.set_result(True)
mock_onvif_camera.create_devicemgmt_service = MagicMock(return_value=devicemgmt) mock_onvif_camera.create_devicemgmt_service = MagicMock(return_value=devicemgmt)
mock_onvif_camera.create_media_service = MagicMock(return_value=media_service) 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): def setup_mock_device(mock_device):
"""Prepare mock ONVIFDevice.""" """Prepare mock ONVIFDevice."""
mock_device.async_setup.return_value = Future() mock_device.async_setup = AsyncMock(return_value=True)
mock_device.async_setup.return_value.set_result(True)
def mock_constructor(hass, config): def mock_constructor(hass, config):
"""Fake the controller constructor.""" """Fake the controller constructor."""
@ -390,11 +393,48 @@ async def test_flow_manual_entry(hass):
async def test_flow_import_no_mac(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( with patch(
"homeassistant.components.onvif.config_flow.get_device" "homeassistant.components.onvif.config_flow.get_device"
) as mock_onvif_camera: ) 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( result = await hass.config_entries.flow.async_init(
config_flow.DOMAIN, config_flow.DOMAIN,