Fix ecobee 3 homekit pairing (#23882)

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),
             })

tests/components/homekit_controller/test_config_flow.py

@@ -13,14 +13,25 @@ from tests.components.homekit_controller.common import (
 )
 
 
-ERROR_MAPPING_FORM_FIXTURE = [
+PAIRING_START_FORM_ERRORS = [
-    (homekit.MaxPeersError, 'max_peers_error'),
     (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()
+    # Pairing doesn't error error and pairing results
-    controller.pairings = {
+    flow.controller.pairings = {
         '00:00:00:00:00:00': pairing,
     }
-
+    result = await flow.async_step_pair({
-    with mock.patch('homekit.Controller') as controller_cls:
+        'pairing_code': '111-22-33',
-        controller_cls.return_value = controller
+    })
-        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()
+    flow.controller.pairings = {
-    controller.pairings = {
         '00:00:00:00:00:00': pairing,
     }
-
+    result = await flow.async_step_pair({
-    with mock.patch('homekit.Controller') as controller_cls:
+        'pairing_code': '111-22-33',
-        controller_cls.return_value = controller
+    })
-        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()
+    flow.controller.pairings = {
-    controller.pairings = {
         '00:00:00:00:00:00': pairing,
     }
 
-    with mock.patch('homekit.Controller') as controller_cls:
+    result = await flow.async_step_pair({
-        controller_cls.return_value = controller
+        '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()
+    # User initiates pairing - device enters pairing mode and displays code
-    controller.pairings = {}
+    result = await flow.async_step_pair({})
-
+    assert result['type'] == 'form'
-    with mock.patch('homekit.Controller') as controller_cls:
+    assert result['step_id'] == 'pair'
-        controller_cls.return_value = controller
+    assert flow.controller.start_pairing.call_count == 1
-        result = await flow.async_step_pair({
-            'pairing_code': '111-22-33',
-        })
 
+    # 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)
+@pytest.mark.parametrize("exception,expected", PAIRING_START_ABORT_ERRORS)
-async def test_pair_abort_errors(hass, exception, expected):
+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()
+    # User initiates pairing - device refuses to enter pairing mode
-    controller.pairings = {}
+    with mock.patch.object(flow.controller, 'start_pairing') as start_pairing:
-
+        start_pairing.side_effect = exception('error')
-    with mock.patch('homekit.Controller') as controller_cls:
+        result = await flow.async_step_pair({})
-        controller_cls.return_value = controller
-        controller.perform_pairing.side_effect = exception('error')
-        result = await flow.async_step_pair({
-            'pairing_code': '111-22-33',
-        })
 
     assert result['type'] == 'abort'
     assert result['reason'] == expected
     assert flow.context == {'title_placeholders': {'name': 'TestDevice'}}
 
 
-@pytest.mark.parametrize("exception,expected", ERROR_MAPPING_FORM_FIXTURE)
+@pytest.mark.parametrize("exception,expected", PAIRING_START_FORM_ERRORS)
-async def test_pair_form_errors(hass, exception, expected):
+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()
+    # User initiates pairing - device refuses to enter pairing mode
-    controller.pairings = {}
+    with mock.patch.object(flow.controller, 'start_pairing') as start_pairing:
-
+        start_pairing.side_effect = exception('error')
-    with mock.patch('homekit.Controller') as controller_cls:
+        result = await flow.async_step_pair({})
-        controller_cls.return_value = controller
-        controller.perform_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_pair_authentication_error(hass):
+@pytest.mark.parametrize("exception,expected", PAIRING_FINISH_ABORT_ERRORS)
-    """Pairing code is incorrect."""
+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()
+    # User initiates pairing - device enters pairing mode and displays code
-    controller.pairings = {}
+    result = await flow.async_step_pair({})
-
-    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)
     assert result['type'] == 'form'
     assert result['step_id'] == 'pair'
-    assert flow.context == {'title_placeholders': {'name': 'TestDevice'}}
+    assert flow.controller.start_pairing.call_count == 1
-
-    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',
-        })
 
+    # 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()
+    flow = _setup_flow_handler(hass)
-    controller.pairings = {
+
+    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)
+    result = await flow.async_step_user()
-
-    with mock.patch('homekit.Controller') as controller_cls:
-        controller_cls.return_value = controller
-        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:
+    result = await flow.async_step_pair({
-        controller_cls.return_value = controller
+        '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:
+    flow.controller.discover.return_value = []
-        controller_cls.return_value.discover.return_value = []
+    result = await flow.async_step_user()
-        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:
+    flow.controller.discover.return_value = [
-        controller_cls.return_value.discover.return_value = [
+        discovery_info,
-            discovery_info,
+    ]
-        ]
+    result = await flow.async_step_user()
-        result = await flow.async_step_user()
 
     assert result['type'] == 'abort'
     assert result['reason'] == 'no_devices'