From adab9adbcea94c1feaf4d5ca6878d4d055bc8e5c Mon Sep 17 00:00:00 2001 From: Greg Dowling Date: Mon, 18 Jan 2021 15:00:30 +0000 Subject: [PATCH] Pyroon discovery (#44811) Co-authored-by: Paulus Schoutsen --- homeassistant/components/roon/config_flow.py | 94 ++++-- homeassistant/components/roon/const.py | 2 + homeassistant/components/roon/media_player.py | 2 +- homeassistant/components/roon/server.py | 29 +- homeassistant/components/roon/strings.json | 5 +- tests/components/roon/test_config_flow.py | 272 +++++++++++------- 6 files changed, 254 insertions(+), 150 deletions(-) diff --git a/homeassistant/components/roon/config_flow.py b/homeassistant/components/roon/config_flow.py index f8af2bac0e6..c6ed4436981 100644 --- a/homeassistant/components/roon/config_flow.py +++ b/homeassistant/components/roon/config_flow.py @@ -2,7 +2,7 @@ import asyncio import logging -from roonapi import RoonApi +from roonapi import RoonApi, RoonDiscovery import voluptuous as vol from homeassistant import config_entries, core, exceptions @@ -10,6 +10,7 @@ from homeassistant.const import CONF_API_KEY, CONF_HOST from .const import ( # pylint: disable=unused-import AUTHENTICATE_TIMEOUT, + CONF_ROON_ID, DEFAULT_NAME, DOMAIN, ROON_APPINFO, @@ -25,36 +26,79 @@ TIMEOUT = 120 class RoonHub: """Interact with roon during config flow.""" - def __init__(self, host): - """Initialize.""" - self._host = host + def __init__(self, hass): + """Initialise the RoonHub.""" + self._hass = hass + + async def discover(self): + """Try and discover roon servers.""" + + def get_discovered_servers(discovery): + servers = discovery.all() + discovery.stop() + return servers + + discovery = RoonDiscovery(None) + servers = await self._hass.async_add_executor_job( + get_discovered_servers, discovery + ) + _LOGGER.debug("Servers = %s", servers) + return servers + + async def authenticate(self, host, servers): + """Authenticate with one or more roon servers.""" + + def stop_apis(apis): + for api in apis: + api.stop() - async def authenticate(self, hass) -> bool: - """Test if we can authenticate with the host.""" token = None + core_id = None secs = 0 - roonapi = RoonApi(ROON_APPINFO, None, self._host, blocking_init=False) - while secs < TIMEOUT: - token = roonapi.token + if host is None: + apis = [ + RoonApi(ROON_APPINFO, None, server[0], server[1], blocking_init=False) + for server in servers + ] + else: + apis = [RoonApi(ROON_APPINFO, None, host, blocking_init=False)] + + while secs <= TIMEOUT: + # Roon can discover multiple devices - not all of which are proper servers, so try and authenticate with them all. + # The user will only enable one - so look for a valid token + auth_api = [api for api in apis if api.token is not None] + secs += AUTHENTICATE_TIMEOUT - if token: + if auth_api: + core_id = auth_api[0].core_id + token = auth_api[0].token break + await asyncio.sleep(AUTHENTICATE_TIMEOUT) - token = roonapi.token - roonapi.stop() - return token + await self._hass.async_add_executor_job(stop_apis, apis) + + return (token, core_id) -async def authenticate(hass: core.HomeAssistant, host): +async def discover(hass): """Connect and authenticate home assistant.""" - hub = RoonHub(host) - token = await hub.authenticate(hass) + hub = RoonHub(hass) + servers = await hub.discover() + + return servers + + +async def authenticate(hass: core.HomeAssistant, host, servers): + """Connect and authenticate home assistant.""" + + hub = RoonHub(hass) + (token, core_id) = await hub.authenticate(host, servers) if token is None: raise InvalidAuth - return {CONF_HOST: host, CONF_API_KEY: token} + return {CONF_HOST: host, CONF_ROON_ID: core_id, CONF_API_KEY: token} class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): @@ -66,20 +110,20 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): def __init__(self): """Initialize the Roon flow.""" self._host = None + self._servers = [] async def async_step_user(self, user_input=None): """Handle getting host details from the user.""" errors = {} + self._servers = await discover(self.hass) + + # We discovered one or more roon - so skip to authentication + if self._servers: + return await self.async_step_link() + if user_input is not None: self._host = user_input["host"] - existing = { - entry.data[CONF_HOST] for entry in self._async_current_entries() - } - if self._host in existing: - errors["base"] = "duplicate_entry" - return self.async_show_form(step_id="user", errors=errors) - return await self.async_step_link() return self.async_show_form( @@ -92,7 +136,7 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): errors = {} if user_input is not None: try: - info = await authenticate(self.hass, self._host) + info = await authenticate(self.hass, self._host, self._servers) except InvalidAuth: errors["base"] = "invalid_auth" except Exception: # pylint: disable=broad-except diff --git a/homeassistant/components/roon/const.py b/homeassistant/components/roon/const.py index dc11d4167a7..7c9cd6c4999 100644 --- a/homeassistant/components/roon/const.py +++ b/homeassistant/components/roon/const.py @@ -4,6 +4,8 @@ AUTHENTICATE_TIMEOUT = 5 DOMAIN = "roon" +CONF_ROON_ID = "roon_server_id" + DATA_CONFIGS = "roon_configs" DEFAULT_NAME = "Roon Labs Music Player" diff --git a/homeassistant/components/roon/media_player.py b/homeassistant/components/roon/media_player.py index 50b6c22cea6..9bdf4286291 100644 --- a/homeassistant/components/roon/media_player.py +++ b/homeassistant/components/roon/media_player.py @@ -175,7 +175,7 @@ class RoonDevice(MediaPlayerEntity): "name": self.name, "manufacturer": "RoonLabs", "model": dev_model, - "via_hub": (DOMAIN, self._server.host), + "via_hub": (DOMAIN, self._server.roon_id), } def update_data(self, player_data=None): diff --git a/homeassistant/components/roon/server.py b/homeassistant/components/roon/server.py index 0229f3492be..d5b8d81c2aa 100644 --- a/homeassistant/components/roon/server.py +++ b/homeassistant/components/roon/server.py @@ -8,7 +8,7 @@ from homeassistant.const import CONF_API_KEY, CONF_HOST from homeassistant.helpers.dispatcher import async_dispatcher_send from homeassistant.util.dt import utcnow -from .const import ROON_APPINFO +from .const import CONF_ROON_ID, ROON_APPINFO _LOGGER = logging.getLogger(__name__) FULL_SYNC_INTERVAL = 30 @@ -22,28 +22,33 @@ class RoonServer: self.config_entry = config_entry self.hass = hass self.roonapi = None + self.roon_id = None self.all_player_ids = set() self.all_playlists = [] self.offline_devices = set() self._exit = False self._roon_name_by_id = {} - @property - def host(self): - """Return the host of this server.""" - return self.config_entry.data[CONF_HOST] - async def async_setup(self, tries=0): - """Set up a roon server based on host parameter.""" - host = self.host + """Set up a roon server based on config parameters.""" hass = self.hass + # Host will be None for configs using discovery + host = self.config_entry.data[CONF_HOST] token = self.config_entry.data[CONF_API_KEY] - _LOGGER.debug("async_setup: %s %s", token, host) - self.roonapi = RoonApi(ROON_APPINFO, token, host, blocking_init=False) + # Default to None for compatibility with older configs + core_id = self.config_entry.data.get(CONF_ROON_ID) + _LOGGER.debug("async_setup: host=%s core_id=%s token=%s", host, core_id, token) + + self.roonapi = RoonApi( + ROON_APPINFO, token, host, blocking_init=False, core_id=core_id + ) self.roonapi.register_state_callback( self.roonapi_state_callback, event_filter=["zones_changed"] ) + # Default to 'host' for compatibility with older configs without core_id + self.roon_id = core_id if core_id is not None else host + # initialize media_player platform hass.async_create_task( hass.config_entries.async_forward_entry_setup( @@ -152,11 +157,11 @@ class RoonServer: new_dict = zone.copy() new_dict.update(output) new_dict.pop("outputs") - new_dict["host"] = self.host + new_dict["roon_id"] = self.roon_id new_dict["is_synced"] = len(zone["outputs"]) > 1 new_dict["zone_name"] = zone["display_name"] new_dict["display_name"] = output["display_name"] new_dict["last_changed"] = utcnow() # we don't use the zone_id or output_id for now as unique id as I've seen cases were it changes for some reason - new_dict["dev_id"] = f"roon_{self.host}_{output['display_name']}" + new_dict["dev_id"] = f"roon_{self.roon_id}_{output['display_name']}" return new_dict diff --git a/homeassistant/components/roon/strings.json b/homeassistant/components/roon/strings.json index 8164f47ade8..565a66a1320 100644 --- a/homeassistant/components/roon/strings.json +++ b/homeassistant/components/roon/strings.json @@ -2,7 +2,7 @@ "config": { "step": { "user": { - "description": "Please enter your Roon server Hostname or IP.", + "description": "Could not discover Roon server, please enter your the Hostname or IP.", "data": { "host": "[%key:common::config_flow::data::host%]" } @@ -14,8 +14,7 @@ }, "error": { "invalid_auth": "[%key:common::config_flow::error::invalid_auth%]", - "unknown": "[%key:common::config_flow::error::unknown%]", - "duplicate_entry": "That host has already been added." + "unknown": "[%key:common::config_flow::error::unknown%]" }, "abort": { "already_configured": "[%key:common::config_flow::abort::already_configured_device%]" diff --git a/tests/components/roon/test_config_flow.py b/tests/components/roon/test_config_flow.py index 6fcfc048d4b..4b4daab088d 100644 --- a/tests/components/roon/test_config_flow.py +++ b/tests/components/roon/test_config_flow.py @@ -3,49 +3,131 @@ from unittest.mock import patch from homeassistant import config_entries, setup from homeassistant.components.roon.const import DOMAIN -from homeassistant.const import CONF_HOST - -from tests.common import MockConfigEntry class RoonApiMock: - """Mock to handle returning tokens for testing the RoonApi.""" - - def __init__(self, token): - """Initialize.""" - self._token = token + """Class to mock the Roon API for testing.""" @property def token(self): - """Return the auth token from the api.""" - return self._token + """Return a good authentication key.""" + return "good_token" - def stop(self): # pylint: disable=no-self-use - """Close down the api.""" + @property + def core_id(self): + """Return the roon host.""" + return "core_id" + + def stop(self): + """Stop socket and discovery.""" return -async def test_form_and_auth(hass): - """Test we get the form.""" - await setup.async_setup_component(hass, "persistent_notification", {}) - result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": config_entries.SOURCE_USER} - ) - assert result["type"] == "form" - assert result["errors"] == {} +class RoonApiMockNoToken(RoonApiMock): + """Class to mock the Roon API for testing, with failed authorisation.""" - with patch("homeassistant.components.roon.config_flow.TIMEOUT", 0,), patch( - "homeassistant.components.roon.const.AUTHENTICATE_TIMEOUT", - 0, - ), patch( + @property + def token(self): + """Return a bad authentication key.""" + return None + + +class RoonApiMockException(RoonApiMock): + """Class to mock the Roon API for testing, throws an unexpected exception.""" + + @property + def token(self): + """Throw exception.""" + raise Exception + + +class RoonDiscoveryMock: + """Class to mock Roon Discovery for testing.""" + + def all(self): + """Return a discovered roon server.""" + return ["2.2.2.2"] + + def stop(self): + """Stop discovery running.""" + return + + +class RoonDiscoveryFailedMock(RoonDiscoveryMock): + """Class to mock Roon Discovery for testing, with no servers discovered.""" + + def all(self): + """Return no discovered roon servers.""" + return [] + + +async def test_successful_discovery_and_auth(hass): + """Test when discovery and auth both work ok.""" + + await setup.async_setup_component(hass, "persistent_notification", {}) + with patch( "homeassistant.components.roon.config_flow.RoonApi", - return_value=RoonApiMock("good_token"), + return_value=RoonApiMock(), + ), patch( + "homeassistant.components.roon.config_flow.RoonDiscovery", + return_value=RoonDiscoveryMock(), ), patch( "homeassistant.components.roon.async_setup", return_value=True - ) as mock_setup, patch( + ), patch( "homeassistant.components.roon.async_setup_entry", return_value=True, - ) as mock_setup_entry: + ): + + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + await hass.async_block_till_done() + + # Should go straight to link if server was discovered + assert result["type"] == "form" + assert result["step_id"] == "link" + assert result["errors"] == {} + + result2 = await hass.config_entries.flow.async_configure( + result["flow_id"], user_input={} + ) + await hass.async_block_till_done() + + assert result2["title"] == "Roon Labs Music Player" + assert result2["data"] == { + "host": None, + "api_key": "good_token", + "roon_server_id": "core_id", + } + + +async def test_unsuccessful_discovery_user_form_and_auth(hass): + """Test unsuccessful discover, user adding the host via the form and then successful auth.""" + + await setup.async_setup_component(hass, "persistent_notification", {}) + with patch( + "homeassistant.components.roon.config_flow.RoonApi", + return_value=RoonApiMock(), + ), patch( + "homeassistant.components.roon.config_flow.RoonDiscovery", + return_value=RoonDiscoveryFailedMock(), + ), patch( + "homeassistant.components.roon.async_setup", return_value=True + ), patch( + "homeassistant.components.roon.async_setup_entry", + return_value=True, + ): + + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} + ) + await hass.async_block_till_done() + + # Should show the form if server was not discovered + assert result["type"] == "form" + assert result["step_id"] == "user" + assert result["errors"] == {} + await hass.config_entries.flow.async_configure( result["flow_id"], {"host": "1.1.1.1"} ) @@ -54,113 +136,85 @@ async def test_form_and_auth(hass): ) await hass.async_block_till_done() - assert result2["type"] == "create_entry" assert result2["title"] == "Roon Labs Music Player" - assert result2["data"] == {"host": "1.1.1.1", "api_key": "good_token"} - assert len(mock_setup.mock_calls) == 1 - assert len(mock_setup_entry.mock_calls) == 1 + assert result2["data"] == { + "host": "1.1.1.1", + "api_key": "good_token", + "roon_server_id": "core_id", + } -async def test_form_no_token(hass): - """Test we handle no token being returned (timeout or not authorized).""" - result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": config_entries.SOURCE_USER} - ) - with patch("homeassistant.components.roon.config_flow.TIMEOUT", 0,), patch( - "homeassistant.components.roon.const.AUTHENTICATE_TIMEOUT", +async def test_successful_discovery_no_auth(hass): + """Test successful discover, but failed auth.""" + + await setup.async_setup_component(hass, "persistent_notification", {}) + with patch( + "homeassistant.components.roon.config_flow.RoonApi", + return_value=RoonApiMockNoToken(), + ), patch( + "homeassistant.components.roon.config_flow.RoonDiscovery", + return_value=RoonDiscoveryMock(), + ), patch( + "homeassistant.components.roon.config_flow.TIMEOUT", 0, ), patch( - "homeassistant.components.roon.config_flow.RoonApi", - return_value=RoonApiMock(None), + "homeassistant.components.roon.config_flow.AUTHENTICATE_TIMEOUT", + 0.01, + ), patch( + "homeassistant.components.roon.async_setup", return_value=True + ), patch( + "homeassistant.components.roon.async_setup_entry", + return_value=True, ): - await hass.config_entries.flow.async_configure( - result["flow_id"], {"host": "1.1.1.1"} + + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} ) + await hass.async_block_till_done() + + # Should go straight to link if server was discovered + assert result["type"] == "form" + assert result["step_id"] == "link" + assert result["errors"] == {} result2 = await hass.config_entries.flow.async_configure( result["flow_id"], user_input={} ) + await hass.async_block_till_done() - assert result2["type"] == "form" assert result2["errors"] == {"base": "invalid_auth"} -async def test_form_unknown_exception(hass): - """Test we handle cannot connect error.""" - result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": config_entries.SOURCE_USER} - ) - - with patch( - "homeassistant.components.roon.config_flow.RoonApi", - side_effect=Exception, - ): - await hass.config_entries.flow.async_configure( - result["flow_id"], {"host": "1.1.1.1"} - ) - - result2 = await hass.config_entries.flow.async_configure( - result["flow_id"], user_input={} - ) - - assert result2["type"] == "form" - assert result2["errors"] == {"base": "unknown"} - - -async def test_form_host_already_exists(hass): - """Test we add the host if the config exists and it isn't a duplicate.""" - - MockConfigEntry(domain=DOMAIN, data={CONF_HOST: "existing_host"}).add_to_hass(hass) +async def test_unexpected_exception(hass): + """Test successful discover, and unexpected exception during auth.""" await setup.async_setup_component(hass, "persistent_notification", {}) - result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": config_entries.SOURCE_USER} - ) - assert result["type"] == "form" - assert result["errors"] == {} - - with patch("homeassistant.components.roon.config_flow.TIMEOUT", 0,), patch( - "homeassistant.components.roon.const.AUTHENTICATE_TIMEOUT", - 0, - ), patch( + with patch( "homeassistant.components.roon.config_flow.RoonApi", - return_value=RoonApiMock("good_token"), + return_value=RoonApiMockException(), + ), patch( + "homeassistant.components.roon.config_flow.RoonDiscovery", + return_value=RoonDiscoveryMock(), ), patch( "homeassistant.components.roon.async_setup", return_value=True - ) as mock_setup, patch( + ), patch( "homeassistant.components.roon.async_setup_entry", return_value=True, - ) as mock_setup_entry: - await hass.config_entries.flow.async_configure( - result["flow_id"], {"host": "1.1.1.1"} + ): + + result = await hass.config_entries.flow.async_init( + DOMAIN, context={"source": config_entries.SOURCE_USER} ) + await hass.async_block_till_done() + + # Should go straight to link if server was discovered + assert result["type"] == "form" + assert result["step_id"] == "link" + assert result["errors"] == {} + result2 = await hass.config_entries.flow.async_configure( result["flow_id"], user_input={} ) await hass.async_block_till_done() - assert result2["type"] == "create_entry" - assert result2["title"] == "Roon Labs Music Player" - assert result2["data"] == {"host": "1.1.1.1", "api_key": "good_token"} - assert len(mock_setup.mock_calls) == 1 - assert len(mock_setup_entry.mock_calls) == 2 - - -async def test_form_duplicate_host(hass): - """Test we don't add the host if it's a duplicate.""" - - MockConfigEntry(domain=DOMAIN, data={CONF_HOST: "existing_host"}).add_to_hass(hass) - - await setup.async_setup_component(hass, "persistent_notification", {}) - result = await hass.config_entries.flow.async_init( - DOMAIN, context={"source": config_entries.SOURCE_USER} - ) - assert result["type"] == "form" - assert result["errors"] == {} - - result2 = await hass.config_entries.flow.async_configure( - result["flow_id"], {"host": "existing_host"} - ) - - assert result2["type"] == "form" - assert result2["errors"] == {"base": "duplicate_entry"} + assert result2["errors"] == {"base": "unknown"}