Fix Synology NAS discovered multiple times (#35094)

homeassistant/components/synology_dsm/__init__.py

@@ -11,6 +11,7 @@ from homeassistant.config_entries import SOURCE_IMPORT, ConfigEntry
 from homeassistant.const import (
     CONF_DISKS,
     CONF_HOST,
+    CONF_MAC,
     CONF_PASSWORD,
     CONF_PORT,
     CONF_SSL,
@@ -77,6 +78,13 @@ async def async_setup_entry(hass: HomeAssistantType, entry: ConfigEntry):
     hass.data.setdefault(DOMAIN, {})
     hass.data[DOMAIN][entry.unique_id] = api
 
+    # For SSDP compat
+    if not entry.data.get(CONF_MAC):
+        network = await hass.async_add_executor_job(getattr, api.dsm, "network")
+        hass.config_entries.async_update_entry(
+            entry, data={**entry.data, CONF_MAC: network.macs}
+        )
+
     hass.async_create_task(
         hass.config_entries.async_forward_entry_setup(entry, "sensor")
     )
@@ -115,7 +123,7 @@ class SynoApi:
         self._device_token = device_token
         self.temp_unit = temp_unit
 
-        self._dsm: SynologyDSM = None
+        self.dsm: SynologyDSM = None
         self.information: SynoDSMInformation = None
         self.utilisation: SynoCoreUtilization = None
         self.storage: SynoStorage = None
@@ -129,7 +137,7 @@ class SynoApi:
 
     async def async_setup(self):
         """Start interacting with the NAS."""
-        self._dsm = SynologyDSM(
+        self.dsm = SynologyDSM(
             self._host,
             self._port,
             self._username,
@@ -147,9 +155,9 @@ class SynoApi:
 
     def _fetch_device_configuration(self):
         """Fetch initial device config."""
-        self.information = self._dsm.information
+        self.information = self.dsm.information
-        self.utilisation = self._dsm.utilisation
+        self.utilisation = self.dsm.utilisation
-        self.storage = self._dsm.storage
+        self.storage = self.dsm.storage
 
     async def async_unload(self):
         """Stop interacting with the NAS and prepare for removal from hass."""
@@ -157,5 +165,5 @@ class SynoApi:
 
     async def update(self, now=None):
         """Update function for updating API information."""
-        await self._hass.async_add_executor_job(self._dsm.update)
+        await self._hass.async_add_executor_job(self.dsm.update)
         async_dispatcher_send(self._hass, self.signal_sensor_update)

homeassistant/components/synology_dsm/config_flow.py

@@ -17,6 +17,7 @@ from homeassistant.components import ssdp
 from homeassistant.const import (
     CONF_DISKS,
     CONF_HOST,
+    CONF_MAC,
     CONF_NAME,
     CONF_PASSWORD,
     CONF_PORT,
@@ -145,6 +146,7 @@ class SynologyDSMFlowHandler(config_entries.ConfigFlow, domain=DOMAIN):
             CONF_SSL: use_ssl,
             CONF_USERNAME: username,
             CONF_PASSWORD: password,
+            CONF_MAC: api.network.macs,
         }
         if otp_code:
             config_data["device_token"] = api.device_token
@@ -162,16 +164,11 @@ class SynologyDSMFlowHandler(config_entries.ConfigFlow, domain=DOMAIN):
             discovery_info[ssdp.ATTR_UPNP_FRIENDLY_NAME].split("(", 1)[0].strip()
         )
 
-        if self._host_already_configured(parsed_url.hostname):
+        # Synology NAS can broadcast on multiple IP addresses, since they can be connected to multiple ethernets.
+        # The serial of the NAS is actually its MAC address.
+        if self._mac_already_configured(discovery_info[ssdp.ATTR_UPNP_SERIAL].upper()):
             return self.async_abort(reason="already_configured")
 
-        if ssdp.ATTR_UPNP_SERIAL in discovery_info:
-            # Synology can broadcast on multiple IP addresses
-            await self.async_set_unique_id(
-                discovery_info[ssdp.ATTR_UPNP_SERIAL].upper()
-            )
-            self._abort_if_unique_id_configured()
-
         self.discovered_conf = {
             CONF_NAME: friendly_name,
             CONF_HOST: parsed_url.hostname,
@@ -205,12 +202,14 @@ class SynologyDSMFlowHandler(config_entries.ConfigFlow, domain=DOMAIN):
 
         return await self.async_step_user(user_input)
 
-    def _host_already_configured(self, hostname):
+    def _mac_already_configured(self, mac):
-        """See if we already have a host matching user input configured."""
+        """See if we already have configured a NAS with this MAC address."""
-        existing_hosts = {
+        existing_macs = [
-            entry.data[CONF_HOST] for entry in self._async_current_entries()
+            mac.replace("-", "")
-        }
+            for entry in self._async_current_entries()
-        return hostname in existing_hosts
+            for mac in entry.data.get(CONF_MAC, [])
+        ]
+        return mac in existing_macs
 
 
 def _login_and_fetch_syno_info(api, otp_code):
@@ -221,10 +220,11 @@ def _login_and_fetch_syno_info(api, otp_code):
     storage = api.storage
 
     if (
-        api.information.serial is None
+        not api.information.serial
         or utilisation.cpu_user_load is None
-        or storage.disks_ids is None
+        or not storage.disks_ids
-        or storage.volumes_ids is None
+        or not storage.volumes_ids
+        or not api.network.macs
     ):
         raise InvalidData
 

tests/components/synology_dsm/test_config_flow.py

@@ -25,6 +25,7 @@ from homeassistant.config_entries import SOURCE_IMPORT, SOURCE_SSDP, SOURCE_USER
 from homeassistant.const import (
     CONF_DISKS,
     CONF_HOST,
+    CONF_MAC,
     CONF_PASSWORD,
     CONF_PORT,
     CONF_SSL,
@@ -47,6 +48,8 @@ USERNAME = "Home_Assistant"
 PASSWORD = "password"
 DEVICE_TOKEN = "Dév!cè_T0k€ñ"
 
+MACS = ["00-11-32-XX-XX-59", "00-11-32-XX-XX-5A"]
+
 
 @pytest.fixture(name="service")
 def mock_controller_service():
@@ -56,8 +59,9 @@ def mock_controller_service():
     ) as service_mock:
         service_mock.return_value.information.serial = SERIAL
         service_mock.return_value.utilisation.cpu_user_load = 1
-        service_mock.return_value.storage.disks_ids = []
+        service_mock.return_value.storage.disks_ids = ["sda", "sdb", "sdc"]
-        service_mock.return_value.storage.volumes_ids = []
+        service_mock.return_value.storage.volumes_ids = ["volume_1"]
+        service_mock.return_value.network.macs = MACS
         yield service_mock
 
 
@@ -72,8 +76,9 @@ def mock_controller_service_2sa():
         )
         service_mock.return_value.information.serial = SERIAL
         service_mock.return_value.utilisation.cpu_user_load = 1
-        service_mock.return_value.storage.disks_ids = []
+        service_mock.return_value.storage.disks_ids = ["sda", "sdb", "sdc"]
-        service_mock.return_value.storage.volumes_ids = []
+        service_mock.return_value.storage.volumes_ids = ["volume_1"]
+        service_mock.return_value.network.macs = MACS
         yield service_mock
 
 
@@ -85,8 +90,9 @@ def mock_controller_service_failed():
     ) as service_mock:
         service_mock.return_value.information.serial = None
         service_mock.return_value.utilisation.cpu_user_load = None
-        service_mock.return_value.storage.disks_ids = None
+        service_mock.return_value.storage.disks_ids = []
-        service_mock.return_value.storage.volumes_ids = None
+        service_mock.return_value.storage.volumes_ids = []
+        service_mock.return_value.network.macs = []
         yield service_mock
 
 
@@ -118,6 +124,7 @@ async def test_user(hass: HomeAssistantType, service: MagicMock):
     assert result["data"][CONF_SSL] == SSL
     assert result["data"][CONF_USERNAME] == USERNAME
     assert result["data"][CONF_PASSWORD] == PASSWORD
+    assert result["data"][CONF_MAC] == MACS
     assert result["data"].get("device_token") is None
     assert result["data"].get(CONF_DISKS) is None
     assert result["data"].get(CONF_VOLUMES) is None
@@ -142,6 +149,7 @@ async def test_user(hass: HomeAssistantType, service: MagicMock):
     assert not result["data"][CONF_SSL]
     assert result["data"][CONF_USERNAME] == USERNAME
     assert result["data"][CONF_PASSWORD] == PASSWORD
+    assert result["data"][CONF_MAC] == MACS
     assert result["data"].get("device_token") is None
     assert result["data"].get(CONF_DISKS) is None
     assert result["data"].get(CONF_VOLUMES) is None
@@ -183,6 +191,7 @@ async def test_user_2sa(hass: HomeAssistantType, service_2sa: MagicMock):
     assert result["data"][CONF_SSL] == DEFAULT_SSL
     assert result["data"][CONF_USERNAME] == USERNAME
     assert result["data"][CONF_PASSWORD] == PASSWORD
+    assert result["data"][CONF_MAC] == MACS
     assert result["data"].get("device_token") == DEVICE_TOKEN
     assert result["data"].get(CONF_DISKS) is None
     assert result["data"].get(CONF_VOLUMES) is None
@@ -204,6 +213,7 @@ async def test_import(hass: HomeAssistantType, service: MagicMock):
     assert result["data"][CONF_SSL] == DEFAULT_SSL
     assert result["data"][CONF_USERNAME] == USERNAME
     assert result["data"][CONF_PASSWORD] == PASSWORD
+    assert result["data"][CONF_MAC] == MACS
     assert result["data"].get("device_token") is None
     assert result["data"].get(CONF_DISKS) is None
     assert result["data"].get(CONF_VOLUMES) is None
@@ -231,6 +241,7 @@ async def test_import(hass: HomeAssistantType, service: MagicMock):
     assert result["data"][CONF_SSL] == SSL
     assert result["data"][CONF_USERNAME] == USERNAME
     assert result["data"][CONF_PASSWORD] == PASSWORD
+    assert result["data"][CONF_MAC] == MACS
     assert result["data"].get("device_token") is None
     assert result["data"][CONF_DISKS] == ["sda", "sdb", "sdc"]
     assert result["data"][CONF_VOLUMES] == ["volume_1"]
@@ -329,8 +340,13 @@ async def test_form_ssdp_already_configured(
 
     MockConfigEntry(
         domain=DOMAIN,
-        data={CONF_HOST: HOST, CONF_USERNAME: USERNAME, CONF_PASSWORD: PASSWORD},
+        data={
-        unique_id=SERIAL.upper(),
+            CONF_HOST: HOST,
+            CONF_USERNAME: USERNAME,
+            CONF_PASSWORD: PASSWORD,
+            CONF_MAC: MACS,
+        },
+        unique_id=SERIAL,
     ).add_to_hass(hass)
 
     result = await hass.config_entries.flow.async_init(
@@ -339,7 +355,7 @@ async def test_form_ssdp_already_configured(
         data={
             ssdp.ATTR_SSDP_LOCATION: "http://192.168.1.5:5000",
             ssdp.ATTR_UPNP_FRIENDLY_NAME: "mydsm",
-            ssdp.ATTR_UPNP_SERIAL: SERIAL,
+            ssdp.ATTR_UPNP_SERIAL: "001132XXXX59",  # Existing in MACS[0], but SSDP does not have `-`
         },
     )
     assert result["type"] == data_entry_flow.RESULT_TYPE_ABORT
@@ -355,7 +371,7 @@ async def test_form_ssdp(hass: HomeAssistantType, service: MagicMock):
         data={
             ssdp.ATTR_SSDP_LOCATION: "http://192.168.1.5:5000",
             ssdp.ATTR_UPNP_FRIENDLY_NAME: "mydsm",
-            ssdp.ATTR_UPNP_SERIAL: SERIAL,
+            ssdp.ATTR_UPNP_SERIAL: "001132XXXX99",  # MAC address, but SSDP does not have `-`
         },
     )
     assert result["type"] == data_entry_flow.RESULT_TYPE_FORM
@@ -373,6 +389,7 @@ async def test_form_ssdp(hass: HomeAssistantType, service: MagicMock):
     assert result["data"][CONF_SSL] == DEFAULT_SSL
     assert result["data"][CONF_USERNAME] == USERNAME
     assert result["data"][CONF_PASSWORD] == PASSWORD
+    assert result["data"][CONF_MAC] == MACS
     assert result["data"].get("device_token") is None
     assert result["data"].get(CONF_DISKS) is None
     assert result["data"].get(CONF_VOLUMES) is None