From 692eeb3687bd053abb74dbbcba7b8d1f273d9041 Mon Sep 17 00:00:00 2001 From: Jc2k Date: Thu, 16 May 2019 13:32:13 +0100 Subject: [PATCH] Fix ecobee 3 homekit pairing (#23882) --- .../homekit_controller/config_flow.py | 81 ++++- .../homekit_controller/test_config_flow.py | 306 ++++++++---------- 2 files changed, 209 insertions(+), 178 deletions(-) diff --git a/homeassistant/components/homekit_controller/config_flow.py b/homeassistant/components/homekit_controller/config_flow.py index 6c534bb0c64..deefb596310 100644 --- a/homeassistant/components/homekit_controller/config_flow.py +++ b/homeassistant/components/homekit_controller/config_flow.py @@ -66,14 +66,16 @@ class HomekitControllerFlowHandler(config_entries.ConfigFlow): def __init__(self): """Initialize the homekit_controller flow.""" + import homekit # pylint: disable=import-error + self.model = None self.hkid = None self.devices = {} + self.controller = homekit.Controller() + self.finish_pairing = None async def async_step_user(self, user_input=None): """Handle a flow start.""" - import homekit - errors = {} if user_input is not None: @@ -82,9 +84,8 @@ class HomekitControllerFlowHandler(config_entries.ConfigFlow): self.model = self.devices[key]['md'] return await self.async_step_pair() - controller = homekit.Controller() all_hosts = await self.hass.async_add_executor_job( - controller.discover, 5 + self.controller.discover, 5 ) self.devices = {} @@ -189,7 +190,11 @@ class HomekitControllerFlowHandler(config_entries.ConfigFlow): self.model = model self.hkid = hkid - return await self.async_step_pair() + + # We want to show the pairing form - but don't call async_step_pair + # directly as it has side effects (will ask the device to show a + # pairing code) + return self._async_step_pair_show_form() async def async_import_legacy_pairing(self, discovery_props, pairing_data): """Migrate a legacy pairing to config entries.""" @@ -216,45 +221,91 @@ class HomekitControllerFlowHandler(config_entries.ConfigFlow): """Pair with a new HomeKit accessory.""" import homekit # pylint: disable=import-error + # If async_step_pair is called with no pairing code then we do the M1 + # phase of pairing. If this is successful the device enters pairing + # mode. + + # If it doesn't have a screen then the pin is static. + + # If it has a display it will display a pin on that display. In + # this case the code is random. So we have to call the start_pairing + # API before the user can enter a pin. But equally we don't want to + # call start_pairing when the device is discovered, only when they + # click on 'Configure' in the UI. + + # start_pairing will make the device show its pin and return a + # callable. We call the callable with the pin that the user has typed + # in. + errors = {} if pair_info: code = pair_info['pairing_code'] - controller = homekit.Controller() try: await self.hass.async_add_executor_job( - controller.perform_pairing, self.hkid, self.hkid, code + self.finish_pairing, code ) - pairing = controller.pairings.get(self.hkid) + pairing = self.controller.pairings.get(self.hkid) if pairing: return await self._entry_from_accessory( pairing) errors['pairing_code'] = 'unable_to_pair' except homekit.AuthenticationError: + # PairSetup M4 - SRP proof failed + # PairSetup M6 - Ed25519 signature verification failed + # PairVerify M4 - Decryption failed + # PairVerify M4 - Device not recognised + # PairVerify M4 - Ed25519 signature verification failed errors['pairing_code'] = 'authentication_error' except homekit.UnknownError: + # An error occured on the device whilst performing this + # operation. errors['pairing_code'] = 'unknown_error' - except homekit.MaxTriesError: - errors['pairing_code'] = 'max_tries_error' - except homekit.BusyError: - errors['pairing_code'] = 'busy_error' except homekit.MaxPeersError: + # The device can't pair with any more accessories. errors['pairing_code'] = 'max_peers_error' except homekit.AccessoryNotFoundError: + # Can no longer find the device on the network return self.async_abort(reason='accessory_not_found_error') - except homekit.UnavailableError: - return self.async_abort(reason='already_paired') except Exception: # pylint: disable=broad-except _LOGGER.exception( "Pairing attempt failed with an unhandled exception" ) errors['pairing_code'] = 'pairing_failed' + start_pairing = self.controller.start_pairing + try: + self.finish_pairing = await self.hass.async_add_executor_job( + start_pairing, self.hkid, self.hkid + ) + except homekit.BusyError: + # Already performing a pair setup operation with a different + # controller + errors['pairing_code'] = 'busy_error' + except homekit.MaxTriesError: + # The accessory has received more than 100 unsuccessful auth + # attempts. + errors['pairing_code'] = 'max_tries_error' + except homekit.UnavailableError: + # The accessory is already paired - cannot try to pair again. + return self.async_abort(reason='already_paired') + except homekit.AccessoryNotFoundError: + # Can no longer find the device on the network + return self.async_abort(reason='accessory_not_found_error') + except Exception: # pylint: disable=broad-except + _LOGGER.exception( + "Pairing attempt failed with an unhandled exception" + ) + errors['pairing_code'] = 'pairing_failed' + + return self._async_step_pair_show_form(errors) + + def _async_step_pair_show_form(self, errors=None): return self.async_show_form( step_id='pair', - errors=errors, + errors=errors or {}, data_schema=vol.Schema({ vol.Required('pairing_code'): vol.All(str, vol.Strip), }) diff --git a/tests/components/homekit_controller/test_config_flow.py b/tests/components/homekit_controller/test_config_flow.py index 33160abaa55..9c869809544 100644 --- a/tests/components/homekit_controller/test_config_flow.py +++ b/tests/components/homekit_controller/test_config_flow.py @@ -13,14 +13,25 @@ from tests.components.homekit_controller.common import ( ) -ERROR_MAPPING_FORM_FIXTURE = [ - (homekit.MaxPeersError, 'max_peers_error'), +PAIRING_START_FORM_ERRORS = [ (homekit.BusyError, 'busy_error'), (homekit.MaxTriesError, 'max_tries_error'), (KeyError, 'pairing_failed'), ] -ERROR_MAPPING_ABORT_FIXTURE = [ +PAIRING_START_ABORT_ERRORS = [ + (homekit.AccessoryNotFoundError, 'accessory_not_found_error'), + (homekit.UnavailableError, 'already_paired'), +] + +PAIRING_FINISH_FORM_ERRORS = [ + (homekit.MaxPeersError, 'max_peers_error'), + (homekit.AuthenticationError, 'authentication_error'), + (homekit.UnknownError, 'unknown_error'), + (KeyError, 'pairing_failed'), +] + +PAIRING_FINISH_ABORT_ERRORS = [ (homekit.AccessoryNotFoundError, 'accessory_not_found_error'), ] @@ -29,6 +40,10 @@ def _setup_flow_handler(hass): flow = config_flow.HomekitControllerFlowHandler() flow.hass = hass flow.context = {} + + flow.controller = mock.Mock() + flow.controller.pairings = {} + return flow @@ -48,11 +63,18 @@ async def test_discovery_works(hass): flow = _setup_flow_handler(hass) + # Device is discovered result = await flow.async_step_discovery(discovery_info) assert result['type'] == 'form' assert result['step_id'] == 'pair' assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} + # User initiates pairing - device enters pairing mode and displays code + result = await flow.async_step_pair({}) + assert result['type'] == 'form' + assert result['step_id'] == 'pair' + assert flow.controller.start_pairing.call_count == 1 + pairing = mock.Mock(pairing_data={ 'AccessoryPairingID': '00:00:00:00:00:00', }) @@ -68,17 +90,13 @@ async def test_discovery_works(hass): }] }] - controller = mock.Mock() - controller.pairings = { + # Pairing doesn't error error and pairing results + flow.controller.pairings = { '00:00:00:00:00:00': pairing, } - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) - + result = await flow.async_step_pair({ + 'pairing_code': '111-22-33', + }) assert result['type'] == 'create_entry' assert result['title'] == 'Koogeek-LS1-20833F' assert result['data'] == pairing.pairing_data @@ -100,11 +118,18 @@ async def test_discovery_works_upper_case(hass): flow = _setup_flow_handler(hass) + # Device is discovered result = await flow.async_step_discovery(discovery_info) assert result['type'] == 'form' assert result['step_id'] == 'pair' assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} + # User initiates pairing - device enters pairing mode and displays code + result = await flow.async_step_pair({}) + assert result['type'] == 'form' + assert result['step_id'] == 'pair' + assert flow.controller.start_pairing.call_count == 1 + pairing = mock.Mock(pairing_data={ 'AccessoryPairingID': '00:00:00:00:00:00', }) @@ -120,17 +145,12 @@ async def test_discovery_works_upper_case(hass): }] }] - controller = mock.Mock() - controller.pairings = { + flow.controller.pairings = { '00:00:00:00:00:00': pairing, } - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) - + result = await flow.async_step_pair({ + 'pairing_code': '111-22-33', + }) assert result['type'] == 'create_entry' assert result['title'] == 'Koogeek-LS1-20833F' assert result['data'] == pairing.pairing_data @@ -151,11 +171,18 @@ async def test_discovery_works_missing_csharp(hass): flow = _setup_flow_handler(hass) + # Device is discovered result = await flow.async_step_discovery(discovery_info) assert result['type'] == 'form' assert result['step_id'] == 'pair' assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} + # User initiates pairing - device enters pairing mode and displays code + result = await flow.async_step_pair({}) + assert result['type'] == 'form' + assert result['step_id'] == 'pair' + assert flow.controller.start_pairing.call_count == 1 + pairing = mock.Mock(pairing_data={ 'AccessoryPairingID': '00:00:00:00:00:00', }) @@ -171,17 +198,13 @@ async def test_discovery_works_missing_csharp(hass): }] }] - controller = mock.Mock() - controller.pairings = { + flow.controller.pairings = { '00:00:00:00:00:00': pairing, } - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) - + result = await flow.async_step_pair({ + 'pairing_code': '111-22-33', + }) assert result['type'] == 'create_entry' assert result['title'] == 'Koogeek-LS1-20833F' assert result['data'] == pairing.pairing_data @@ -342,26 +365,28 @@ async def test_pair_unable_to_pair(hass): flow = _setup_flow_handler(hass) + # Device is discovered result = await flow.async_step_discovery(discovery_info) assert result['type'] == 'form' assert result['step_id'] == 'pair' assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} - controller = mock.Mock() - controller.pairings = {} - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) + # User initiates pairing - device enters pairing mode and displays code + result = await flow.async_step_pair({}) + assert result['type'] == 'form' + assert result['step_id'] == 'pair' + assert flow.controller.start_pairing.call_count == 1 + # Pairing doesn't error but no pairing object is generated + result = await flow.async_step_pair({ + 'pairing_code': '111-22-33', + }) assert result['type'] == 'form' assert result['errors']['pairing_code'] == 'unable_to_pair' -@pytest.mark.parametrize("exception,expected", ERROR_MAPPING_ABORT_FIXTURE) -async def test_pair_abort_errors(hass, exception, expected): +@pytest.mark.parametrize("exception,expected", PAIRING_START_ABORT_ERRORS) +async def test_pair_abort_errors_on_start(hass, exception, expected): """Test various pairing errors.""" discovery_info = { 'name': 'TestDevice', @@ -377,28 +402,24 @@ async def test_pair_abort_errors(hass, exception, expected): flow = _setup_flow_handler(hass) + # Device is discovered result = await flow.async_step_discovery(discovery_info) assert result['type'] == 'form' assert result['step_id'] == 'pair' assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} - controller = mock.Mock() - controller.pairings = {} - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - controller.perform_pairing.side_effect = exception('error') - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) + # User initiates pairing - device refuses to enter pairing mode + with mock.patch.object(flow.controller, 'start_pairing') as start_pairing: + start_pairing.side_effect = exception('error') + result = await flow.async_step_pair({}) assert result['type'] == 'abort' assert result['reason'] == expected assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} -@pytest.mark.parametrize("exception,expected", ERROR_MAPPING_FORM_FIXTURE) -async def test_pair_form_errors(hass, exception, expected): +@pytest.mark.parametrize("exception,expected", PAIRING_START_FORM_ERRORS) +async def test_pair_form_errors_on_start(hass, exception, expected): """Test various pairing errors.""" discovery_info = { 'name': 'TestDevice', @@ -414,28 +435,25 @@ async def test_pair_form_errors(hass, exception, expected): flow = _setup_flow_handler(hass) + # Device is discovered result = await flow.async_step_discovery(discovery_info) assert result['type'] == 'form' assert result['step_id'] == 'pair' assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} - controller = mock.Mock() - controller.pairings = {} - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - controller.perform_pairing.side_effect = exception('error') - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) + # User initiates pairing - device refuses to enter pairing mode + with mock.patch.object(flow.controller, 'start_pairing') as start_pairing: + start_pairing.side_effect = exception('error') + result = await flow.async_step_pair({}) assert result['type'] == 'form' assert result['errors']['pairing_code'] == expected assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} -async def test_pair_authentication_error(hass): - """Pairing code is incorrect.""" +@pytest.mark.parametrize("exception,expected", PAIRING_FINISH_ABORT_ERRORS) +async def test_pair_abort_errors_on_finish(hass, exception, expected): + """Test various pairing errors.""" discovery_info = { 'name': 'TestDevice', 'host': '127.0.0.1', @@ -450,96 +468,65 @@ async def test_pair_authentication_error(hass): flow = _setup_flow_handler(hass) + # Device is discovered result = await flow.async_step_discovery(discovery_info) assert result['type'] == 'form' assert result['step_id'] == 'pair' assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} - controller = mock.Mock() - controller.pairings = {} - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - exc = homekit.AuthenticationError('Invalid pairing code') - controller.perform_pairing.side_effect = exc - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) - - assert result['type'] == 'form' - assert result['errors']['pairing_code'] == 'authentication_error' - - -async def test_pair_unknown_error(hass): - """Pairing failed for an unknown rason.""" - discovery_info = { - 'name': 'TestDevice', - 'host': '127.0.0.1', - 'port': 8080, - 'properties': { - 'md': 'TestDevice', - 'id': '00:00:00:00:00:00', - 'c#': 1, - 'sf': 1, - } - } - - flow = _setup_flow_handler(hass) - - result = await flow.async_step_discovery(discovery_info) + # User initiates pairing - device enters pairing mode and displays code + result = await flow.async_step_pair({}) assert result['type'] == 'form' assert result['step_id'] == 'pair' - assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} - - controller = mock.Mock() - controller.pairings = {} - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - exc = homekit.UnknownError('Unknown error') - controller.perform_pairing.side_effect = exc - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) - - assert result['type'] == 'form' - assert result['errors']['pairing_code'] == 'unknown_error' - - -async def test_pair_already_paired(hass): - """Device is already paired.""" - discovery_info = { - 'name': 'TestDevice', - 'host': '127.0.0.1', - 'port': 8080, - 'properties': { - 'md': 'TestDevice', - 'id': '00:00:00:00:00:00', - 'c#': 1, - 'sf': 1, - } - } - - flow = _setup_flow_handler(hass) - - result = await flow.async_step_discovery(discovery_info) - assert result['type'] == 'form' - assert result['step_id'] == 'pair' - assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} - - controller = mock.Mock() - controller.pairings = {} - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - exc = homekit.UnavailableError('Unavailable error') - controller.perform_pairing.side_effect = exc - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) + assert flow.controller.start_pairing.call_count == 1 + # User submits code - pairing fails but can be retried + flow.finish_pairing.side_effect = exception('error') + result = await flow.async_step_pair({ + 'pairing_code': '111-22-33', + }) assert result['type'] == 'abort' - assert result['reason'] == 'already_paired' + assert result['reason'] == expected + assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} + + +@pytest.mark.parametrize("exception,expected", PAIRING_FINISH_FORM_ERRORS) +async def test_pair_form_errors_on_finish(hass, exception, expected): + """Test various pairing errors.""" + discovery_info = { + 'name': 'TestDevice', + 'host': '127.0.0.1', + 'port': 8080, + 'properties': { + 'md': 'TestDevice', + 'id': '00:00:00:00:00:00', + 'c#': 1, + 'sf': 1, + } + } + + flow = _setup_flow_handler(hass) + + # Device is discovered + result = await flow.async_step_discovery(discovery_info) + assert result['type'] == 'form' + assert result['step_id'] == 'pair' + assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} + + # User initiates pairing - device enters pairing mode and displays code + result = await flow.async_step_pair({}) + assert result['type'] == 'form' + assert result['step_id'] == 'pair' + assert flow.controller.start_pairing.call_count == 1 + + # User submits code - pairing fails but can be retried + flow.finish_pairing.side_effect = exception('error') + result = await flow.async_step_pair({ + 'pairing_code': '111-22-33', + }) + assert result['type'] == 'form' + assert result['errors']['pairing_code'] == expected + assert flow.context == {'title_placeholders': {'name': 'TestDevice'}} async def test_import_works(hass): @@ -647,19 +634,16 @@ async def test_user_works(hass): }] }] - controller = mock.Mock() - controller.pairings = { + flow = _setup_flow_handler(hass) + + flow.controller.pairings = { '00:00:00:00:00:00': pairing, } - controller.discover.return_value = [ + flow.controller.discover.return_value = [ discovery_info, ] - flow = _setup_flow_handler(hass) - - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - result = await flow.async_step_user() + result = await flow.async_step_user() assert result['type'] == 'form' assert result['step_id'] == 'user' @@ -669,11 +653,9 @@ async def test_user_works(hass): assert result['type'] == 'form' assert result['step_id'] == 'pair' - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value = controller - result = await flow.async_step_pair({ - 'pairing_code': '111-22-33', - }) + result = await flow.async_step_pair({ + 'pairing_code': '111-22-33', + }) assert result['type'] == 'create_entry' assert result['title'] == 'Koogeek-LS1-20833F' assert result['data'] == pairing.pairing_data @@ -683,9 +665,8 @@ async def test_user_no_devices(hass): """Test user initiated pairing where no devices discovered.""" flow = _setup_flow_handler(hass) - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value.discover.return_value = [] - result = await flow.async_step_user() + flow.controller.discover.return_value = [] + result = await flow.async_step_user() assert result['type'] == 'abort' assert result['reason'] == 'no_devices' @@ -705,11 +686,10 @@ async def test_user_no_unpaired_devices(hass): 'sf': 0, } - with mock.patch('homekit.Controller') as controller_cls: - controller_cls.return_value.discover.return_value = [ - discovery_info, - ] - result = await flow.async_step_user() + flow.controller.discover.return_value = [ + discovery_info, + ] + result = await flow.async_step_user() assert result['type'] == 'abort' assert result['reason'] == 'no_devices'