Allow specifying multiple ports for UPNP component (#9560)

* Update UPNP component

* Bump dep

* Fix flakiness in test

.coveragerc

@@ -52,7 +52,7 @@ omit =
 
     homeassistant/components/digital_ocean.py
     homeassistant/components/*/digital_ocean.py
-	
+
 	homeassistant/components/doorbird.py
 	homeassistant/components/*/doorbird.py
 
@@ -581,7 +581,6 @@ omit =
     homeassistant/components/thingspeak.py
     homeassistant/components/tts/amazon_polly.py
     homeassistant/components/tts/picotts.py
-    homeassistant/components/upnp.py
     homeassistant/components/vacuum/roomba.py
     homeassistant/components/weather/bom.py
     homeassistant/components/weather/buienradar.py

homeassistant/components/upnp.py

@@ -4,16 +4,18 @@ Will open a port in your router for Home Assistant and provide statistics.
 For more details about this component, please refer to the documentation at
 https://home-assistant.io/components/upnp/
 """
+from ipaddress import ip_address
 import logging
-from urllib.parse import urlsplit
 
 import voluptuous as vol
 
 from homeassistant.const import (EVENT_HOMEASSISTANT_STOP)
 from homeassistant.helpers import config_validation as cv
 from homeassistant.helpers import discovery
+from homeassistant.util import get_local_ip
 
-REQUIREMENTS = ['miniupnpc==1.9']
+REQUIREMENTS = ['miniupnpc==2.0.2']
+DEPENDENCIES = ['http']
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -22,9 +24,11 @@ DOMAIN = 'upnp'
 
 DATA_UPNP = 'UPNP'
 
+CONF_LOCAL_IP = 'local_ip'
 CONF_ENABLE_PORT_MAPPING = 'port_mapping'
-CONF_EXTERNAL_PORT = 'external_port'
+CONF_PORTS = 'ports'
 CONF_UNITS = 'unit'
+CONF_HASS = 'hass'
 
 NOTIFICATION_ID = 'upnp_notification'
 NOTIFICATION_TITLE = 'UPnP Setup'
@@ -39,8 +43,10 @@ UNITS = {
 CONFIG_SCHEMA = vol.Schema({
     DOMAIN: vol.Schema({
         vol.Optional(CONF_ENABLE_PORT_MAPPING, default=True): cv.boolean,
-        vol.Optional(CONF_EXTERNAL_PORT, default=0): cv.positive_int,
         vol.Optional(CONF_UNITS, default="MBytes"): vol.In(UNITS),
+        vol.Optional(CONF_LOCAL_IP): ip_address,
+        vol.Optional(CONF_PORTS):
+            vol.Schema({vol.Any(CONF_HASS, cv.positive_int): cv.positive_int})
     }),
 }, extra=vol.ALLOW_EXTRA)
 
@@ -48,6 +54,19 @@ CONFIG_SCHEMA = vol.Schema({
 # pylint: disable=import-error, no-member, broad-except
 def setup(hass, config):
     """Register a port mapping for Home Assistant via UPnP."""
+    config = config[DOMAIN]
+    host = config.get(CONF_LOCAL_IP)
+
+    if host is not None:
+        host = str(host)
+    else:
+        host = get_local_ip()
+
+    if host == '127.0.0.1':
+        _LOGGER.error(
+            'Unable to determine local IP. Add it to your configuration.')
+        return False
+
     import miniupnpc
 
     upnp = miniupnpc.UPnP()
@@ -61,40 +80,44 @@ def setup(hass, config):
         _LOGGER.exception("Error when attempting to discover an UPnP IGD")
         return False
 
-    unit = config[DOMAIN].get(CONF_UNITS)
+    unit = config.get(CONF_UNITS)
     discovery.load_platform(hass, 'sensor', DOMAIN, {'unit': unit}, config)
 
-    port_mapping = config[DOMAIN].get(CONF_ENABLE_PORT_MAPPING)
+    port_mapping = config.get(CONF_ENABLE_PORT_MAPPING)
     if not port_mapping:
         return True
 
-    base_url = urlsplit(hass.config.api.base_url)
+    internal_port = hass.http.server_port
-    host = base_url.hostname
-    internal_port = base_url.port
-    external_port = int(config[DOMAIN].get(CONF_EXTERNAL_PORT))
 
-    if external_port == 0:
+    ports = config.get(CONF_PORTS)
-        external_port = internal_port
+    if ports is None:
+        ports = {CONF_HASS: internal_port}
 
-    try:
+    registered = []
-        upnp.addportmapping(
+    for internal, external in ports.items():
-            external_port, 'TCP', host, internal_port, 'Home Assistant', '')
+        if internal == CONF_HASS:
+            internal = internal_port
+        try:
+            upnp.addportmapping(
+                external, 'TCP', host, internal, 'Home Assistant', '')
+            registered.append(external)
+        except Exception:
+            _LOGGER.exception("UPnP failed to configure port mapping for %s",
+                              external)
+            hass.components.persistent_notification.create(
+                '<b>ERROR: tcp port {} is already mapped in your router.'
+                '</b><br />Please disable port_mapping in the <i>upnp</i> '
+                'configuration section.<br />'
+                'You will need to restart hass after fixing.'
+                ''.format(external),
+                title=NOTIFICATION_TITLE,
+                notification_id=NOTIFICATION_ID)
 
-        def deregister_port(event):
+    def deregister_port(event):
-            """De-register the UPnP port mapping."""
+        """De-register the UPnP port mapping."""
-            upnp.deleteportmapping(external_port, 'TCP')
+        for external in registered:
+            upnp.deleteportmapping(external, 'TCP')
 
-        hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, deregister_port)
+    hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, deregister_port)
 
-    except Exception as ex:
-        _LOGGER.error("UPnP failed to configure port mapping: %s", str(ex))
-        hass.components.persistent_notification.create(
-            '<b>ERROR: tcp port {} is already mapped in your router.'
-            '</b><br />Please disable port_mapping in the <i>upnp</i> '
-            'configuration section.<br />'
-            'You will need to restart hass after fixing.'
-            ''.format(external_port),
-            title=NOTIFICATION_TITLE,
-            notification_id=NOTIFICATION_ID)
-        return False
     return True

requirements_all.txt

@@ -420,7 +420,7 @@ mficlient==0.3.0
 miflora==0.1.16
 
 # homeassistant.components.upnp
-miniupnpc==1.9
+miniupnpc==2.0.2
 
 # homeassistant.components.sensor.mopar
 motorparts==1.0.2

tests/components/test_upnp.py

@@ -0,0 +1,142 @@
+"""Test the UPNP component."""
+import asyncio
+from collections import OrderedDict
+from unittest.mock import patch, MagicMock
+
+import pytest
+
+from homeassistant.const import EVENT_HOMEASSISTANT_STOP
+from homeassistant.setup import async_setup_component
+
+
+@pytest.fixture
+def mock_miniupnpc():
+    """Mock miniupnpc."""
+    mock = MagicMock()
+
+    with patch.dict('sys.modules', {'miniupnpc': mock}):
+        yield mock.UPnP()
+
+
+@pytest.fixture
+def mock_local_ip():
+    """Mock get_local_ip."""
+    with patch('homeassistant.components.upnp.get_local_ip',
+               return_value='192.168.0.10'):
+        yield
+
+
+@pytest.fixture(autouse=True)
+def mock_discovery():
+    """Mock discovery of upnp sensor."""
+    with patch('homeassistant.components.upnp.discovery'):
+        yield
+
+
+@asyncio.coroutine
+def test_setup_fail_if_no_ip(hass):
+    """Test setup fails if we can't find a local IP."""
+    with patch('homeassistant.components.upnp.get_local_ip',
+               return_value='127.0.0.1'):
+        result = yield from async_setup_component(hass, 'upnp', {
+            'upnp': {}
+        })
+
+    assert not result
+
+
+@asyncio.coroutine
+def test_setup_fail_if_cannot_select_igd(hass, mock_local_ip, mock_miniupnpc):
+    """Test setup fails if we can't find an UPnP IGD."""
+    mock_miniupnpc.selectigd.side_effect = Exception
+
+    result = yield from async_setup_component(hass, 'upnp', {
+        'upnp': {}
+    })
+
+    assert not result
+
+
+@asyncio.coroutine
+def test_setup_succeeds_if_specify_ip(hass, mock_miniupnpc):
+    """Test setup succeeds if we specify IP and can't find a local IP."""
+    with patch('homeassistant.components.upnp.get_local_ip',
+               return_value='127.0.0.1'):
+        result = yield from async_setup_component(hass, 'upnp', {
+            'upnp': {
+                'local_ip': '192.168.0.10'
+            }
+        })
+
+    assert result
+
+
+@asyncio.coroutine
+def test_no_config_maps_hass_local_to_remote_port(hass, mock_miniupnpc):
+    """Test by default we map local to remote port."""
+    result = yield from async_setup_component(hass, 'upnp', {
+        'upnp': {
+            'local_ip': '192.168.0.10'
+        }
+    })
+
+    assert result
+    assert len(mock_miniupnpc.addportmapping.mock_calls) == 1
+    external, _, host, internal, _, _ = \
+        mock_miniupnpc.addportmapping.mock_calls[0][1]
+    assert host == '192.168.0.10'
+    assert external == 8123
+    assert internal == 8123
+
+
+@asyncio.coroutine
+def test_map_hass_to_remote_port(hass, mock_miniupnpc):
+    """Test mapping hass to remote port."""
+    result = yield from async_setup_component(hass, 'upnp', {
+        'upnp': {
+            'local_ip': '192.168.0.10',
+            'ports': {
+                'hass': 1000
+            }
+        }
+    })
+
+    assert result
+    assert len(mock_miniupnpc.addportmapping.mock_calls) == 1
+    external, _, host, internal, _, _ = \
+        mock_miniupnpc.addportmapping.mock_calls[0][1]
+    assert external == 1000
+    assert internal == 8123
+
+
+@asyncio.coroutine
+def test_map_internal_to_remote_ports(hass, mock_miniupnpc):
+    """Test mapping local to remote ports."""
+    ports = OrderedDict()
+    ports['hass'] = 1000
+    ports[1883] = 3883
+
+    result = yield from async_setup_component(hass, 'upnp', {
+        'upnp': {
+            'local_ip': '192.168.0.10',
+            'ports': ports
+        }
+    })
+
+    assert result
+    assert len(mock_miniupnpc.addportmapping.mock_calls) == 2
+    external, _, host, internal, _, _ = \
+        mock_miniupnpc.addportmapping.mock_calls[0][1]
+    assert external == 1000
+    assert internal == 8123
+
+    external, _, host, internal, _, _ = \
+        mock_miniupnpc.addportmapping.mock_calls[1][1]
+    assert external == 3883
+    assert internal == 1883
+
+    hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP)
+    yield from hass.async_block_till_done()
+    assert len(mock_miniupnpc.deleteportmapping.mock_calls) == 2
+    assert mock_miniupnpc.deleteportmapping.mock_calls[0][1][0] == 1000
+    assert mock_miniupnpc.deleteportmapping.mock_calls[1][1][0] == 3883