[enh] Refactor firewall_upnp action

* make sure to save final UPnP status (fix #42)
* deprecate the 'reload' action and replace it with a new 'status'
action which retrieve current UPnP status
* allow to not refresh port forwarding with the --no-refresh argument
* some other small changes, e.g. set the action argument to one
optionnal choice, rename the UPnP cron job file, add logging

actionsmap/yunohost.yml

@@ -934,17 +934,20 @@ firewall:
 
         ### firewall_upnp()
         upnp:
-            action_help: Add uPnP cron and enable uPnP in firewall.yml, or the opposite.
+            action_help: Manage port forwarding using UPnP
             api: GET /firewall/upnp
             arguments:
                 action:
-                    help: enable/disable
                     choices:
                         - enable
                         - disable
+                        - status
                         - reload
-                        - []
+                    nargs: "?"
-                    nargs: "*"
+                    default: status
+                --no-refresh:
+                    help: Do not refresh port forwarding
+                    action: store_true
 
         ### firewall_stop()
         stop:

firewall.py

@@ -34,6 +34,7 @@ except ImportError:
     sys.exit(1)
 
 from moulinette.core import MoulinetteError
+from moulinette.utils.log import getActionLogger
 
 """ Search the ssh port in ssh config file
     If we don't find the ssh port we define 22"""
@@ -56,6 +57,11 @@ except:
   ssh_port = '22'
 
 ssh_port = int(ssh_port)
+firewall_file = '/etc/yunohost/firewall.yml'
+upnp_cron_job = '/etc/cron.d/yunohost-firewall-upnp'
+
+logger = getActionLogger('yunohost.firewall')
+
 
 def firewall_allow(port=None, protocol=['TCP'], ipv6=False, no_upnp=False):
     """
@@ -94,7 +100,7 @@ def firewall_allow(port=None, protocol=['TCP'], ipv6=False, no_upnp=False):
         else:
             msignals.display(m18n.n('port_already_opened', port), 'warning')
 
-    with open('/etc/yunohost/firewall.yml', 'w') as f:
+    with open(firewall_file, 'w') as f:
         yaml.safe_dump(firewall, f, default_flow_style=False)
 
     return firewall_reload()
@@ -134,7 +140,7 @@ def firewall_disallow(port=None, protocol=['TCP'], ipv6=False):
         else:
             msignals.display(m18n.n('port_already_closed', port), 'warning')
 
-    with open('/etc/yunohost/firewall.yml', 'w') as f:
+    with open(firewall_file, 'w') as f:
         yaml.safe_dump(firewall, f, default_flow_style=False)
 
     return firewall_reload()
@@ -148,7 +154,7 @@ def firewall_list(raw=False):
         raw -- Return the complete YAML dict
 
     """
-    with open('/etc/yunohost/firewall.yml') as f:
+    with open(firewall_file) as f:
         firewall = yaml.load(f)
 
     if raw:
@@ -172,7 +178,7 @@ def firewall_reload():
     if os.system("iptables -P INPUT ACCEPT") != 0:
         raise MoulinetteError(errno.ESRCH, m18n.n('iptables_unavailable'))
     if upnp:
-        firewall_upnp(action=['reload'])
+        firewall_upnp(no_refresh=False)
 
     os.system("iptables -F")
     os.system("iptables -X")
@@ -218,75 +224,112 @@ def firewall_reload():
     return firewall_list()
 
 
-def firewall_upnp(action=None):
+def firewall_upnp(action='status', no_refresh=False):
     """
-    Add uPnP cron and enable uPnP in firewall.yml, or the opposite.
+    Manage port forwarding using UPnP
+
+    Note: 'reload' action is deprecated and will be removed in the near
+    future. You should use 'status' instead - which retrieve UPnP status
+    and automatically refresh port forwarding if 'no_refresh' is False.
 
     Keyword argument:
-        action -- enable/disable/reload
+        action -- Action to perform
+        no_refresh -- Do not refresh port forwarding
 
     """
     firewall = firewall_list(raw=True)
+    enabled = firewall['uPnP']['enabled']
 
-    if action:
+    # Compatibility with previous version
-        action = action[0]
+    if action == 'reload':
-
+        logger.warning("'reload' action is deprecated and will be removed")
-    if action == 'enable':
-        firewall['uPnP']['enabled'] = True
-
-        with open('/etc/cron.d/yunohost-firewall', 'w+') as f:
-            f.write('PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin \
-            \n*/50 * * * * root yunohost firewall upnp reload >>/dev/null')
-
-        msignals.display(m18n.n('upnp_enabled'), 'success')
-
-    if action == 'disable':
-        firewall['uPnP']['enabled'] = False
-
         try:
-            upnpc = miniupnpc.UPnP()
+            # Remove old cron job
-            upnpc.discoverdelay = 3000
+            os.remove('/etc/cron.d/yunohost-firewall')
-            if upnpc.discover() == 1:
+        except: pass
+        action = 'status'
+        no_refresh = False
+
+    if action == 'status' and no_refresh:
+        # Only return current state
+        return { 'enabled': enabled }
+    elif action == 'enable' or (enabled and action == 'status'):
+        # Add cron job
+        with open(upnp_cron_job, 'w+') as f:
+            f.write('*/50 * * * * root '
+                    '/usr/bin/yunohost firewall upnp status >>/dev/null\n')
+        enabled = True
+    elif action == 'disable' or (not enabled and action == 'status'):
+        try:
+            # Remove cron job
+            os.remove(upnp_cron_job)
+        except: pass
+        enabled = False
+        if action == 'status':
+            no_refresh = True
+    else:
+        raise MoulinetteError(errno.EINVAL, m18n.n('action_invalid', action))
+
+    # Refresh port mapping using UPnP
+    if not no_refresh:
+        upnpc = miniupnpc.UPnP()
+        upnpc.discoverdelay = 3000
+
+        # Discover UPnP device(s)
+        logger.debug('discovering UPnP devices...')
+        nb_dev = upnpc.discover()
+        logger.debug('found %d UPnP device(s)', int(nb_dev))
+        if nb_dev < 1:
+            msignals.display(m18n.n('upnp_dev_not_found'), 'error')
+            enabled = False
+        else:
+            try:
+                # Select UPnP device
                 upnpc.selectigd()
+            except:
+                logger.exception('unable to select UPnP device')
+                enabled = False
+            else:
+                # Iterate over ports
                 for protocol in ['TCP', 'UDP']:
                     for port in firewall['uPnP'][protocol]:
+                        # Clean the mapping of this port
                         if upnpc.getspecificportmapping(port, protocol):
-                            try: upnpc.deleteportmapping(port, protocol)
+                            try:
+                                upnpc.deleteportmapping(port, protocol)
                             except: pass
-        except: pass
+                        if not enabled:
+                            continue
+                        try:
+                            # Add new port mapping
+                            upnpc.addportmapping(port, protocol, upnpc.lanaddr,
+                                port, 'yunohost firewall: port %d' % port, '')
+                        except:
+                            logger.exception('unable to add port %d using UPnP',
+                                             port)
+                            enabled = False
 
+    if enabled != firewall['uPnP']['enabled']:
+        firewall['uPnP']['enabled'] = enabled
 
-        try: os.remove('/etc/cron.d/yunohost-firewall')
+        # Make a backup and update firewall file
-        except: pass
+        os.system("cp {0} {0}.old".format(firewall_file))
-
+        with open(firewall_file, 'w') as f:
-        msignals.display(m18n.n('upnp_disabled'), 'success')
-
-    if action == 'reload':
-        upnp = firewall['uPnP']['enabled']
-
-        if upnp:
-            try:
-                upnpc = miniupnpc.UPnP()
-                upnpc.discoverdelay = 3000
-                if upnpc.discover() == 1:
-                    upnpc.selectigd()
-                    for protocol in ['TCP', 'UDP']:
-                        for port in firewall['uPnP'][protocol]:
-                            if upnpc.getspecificportmapping(port, protocol):
-                                try: upnpc.deleteportmapping(port, protocol)
-                                except: pass
-                            upnpc.addportmapping(port, protocol, upnpc.lanaddr, port, 'yunohost firewall : port %d' % port, '')
-                else:
-                    raise MoulinetteError(errno.ENXIO, m18n.n('upnp_dev_not_found'))
-            except:
-                msignals.display(m18n.n('upnp_port_open_failed'), 'warning')
-
-    if action:
-        os.system("cp /etc/yunohost/firewall.yml /etc/yunohost/firewall.yml.old")
-        with open('/etc/yunohost/firewall.yml', 'w') as f:
             yaml.safe_dump(firewall, f, default_flow_style=False)
 
-    return { "enabled": firewall['uPnP']['enabled'] }
+        if not no_refresh:
+            # Display success message if needed
+            if action == 'enable' and enabled:
+                msignals.display(m18n.n('upnp_enabled'), 'success')
+            elif action == 'disable' and not enabled:
+                msignals.display(m18n.n('upnp_disabled'), 'success')
+            # Make sure to disable UPnP
+            elif action != 'disable' and not enabled:
+                firewall_upnp('disable', no_refresh=True)
+
+    if action == 'enable' and not enabled:
+        raise MoulinetteError(errno.ENXIO, m18n.n('upnp_port_open_failed'))
+    return { 'enabled': enabled }
 
 
 def firewall_stop():
@@ -307,5 +350,5 @@ def firewall_stop():
         os.system("ip6tables -F")
         os.system("ip6tables -X")
 
-    if os.path.exists("/etc/cron.d/yunohost-firewall"):
+    if os.path.exists(upnp_cron_job):
         firewall_upnp('disable')

locales/en.json

@@ -6,6 +6,7 @@
     "installation_complete" : "Installation complete",
     "installation_failed" : "Installation failed",
     "unexpected_error" : "An unexpected error occured",
+    "action_invalid" : "Invalid action '{:s}'",
 
     "license_undefined" : "undefined",
     "no_appslist_found" : "No apps list found",
@@ -72,10 +73,10 @@
     "port_already_opened" : "Port {:d} is already opened",
     "port_already_closed" : "Port {:d} is already closed",
     "iptables_unavailable" : "You cannot play with iptables here. You are either in a container or your kernel does not support it.",
-    "upnp_dev_not_found" : "No uPnP device found",
+    "upnp_dev_not_found" : "No UPnP device found",
-    "upnp_port_open_failed" : "Unable to open uPnP ports",
+    "upnp_port_open_failed" : "Unable to open UPnP ports",
-    "upnp_enabled" : "uPnP successfully enabled",
+    "upnp_enabled" : "UPnP successfully enabled",
-    "upnp_disabled" : "uPnP successfully disabled",
+    "upnp_disabled" : "UPnP successfully disabled",
     "firewall_reloaded" : "Firewall successfully reloaded",
 
     "hook_list_by_invalid" : "Invalid property to list hook by",

locales/fr.json

@@ -6,6 +6,7 @@
     "installation_complete" : "Installation terminée",
     "installation_failed" : "Échec de l'installation",
     "unexpected_error" : "Une erreur inattendue est survenue",
+    "action_invalid" : "Action '{:s}' incorrecte",
 
     "license_undefined" : "indéfinie",
     "no_appslist_found" : "Aucune liste d'applications trouvée",
@@ -72,10 +73,10 @@
     "port_already_opened" : "Le port {:d} est déjà ouvert",
     "port_already_closed" : "Le port {:d} est déjà fermé",
     "iptables_unavailable" : "Vous ne pouvez pas faire joujou avec iptables ici. Vous êtes sûrement dans un conteneur, autrement votre noyau ne le supporte pas.",
-    "upnp_dev_not_found" : "Aucun périphérique compatible uPnP trouvé",
+    "upnp_dev_not_found" : "Aucun périphérique compatible UPnP trouvé",
-    "upnp_port_open_failed" : "Impossible d'ouvrir les ports avec uPnP",
+    "upnp_port_open_failed" : "Impossible d'ouvrir les ports avec UPnP",
-    "upnp_enabled" : "uPnP activé avec succès",
+    "upnp_enabled" : "UPnP activé avec succès",
-    "upnp_disabled" : "uPnP désactivé avec succès",
+    "upnp_disabled" : "UPnP désactivé avec succès",
     "firewall_reloaded" : "Pare-feu rechargé avec succès",
 
     "hook_list_by_invalid" : "Propriété pour lister les scripts incorrecte",

tools.py

@@ -318,12 +318,9 @@ def tools_postinstall(domain, password, ignore_dyndns=False):
     # Change LDAP admin password
     tools_adminpw(old_password='yunohost', new_password=password)
 
-    # Enable uPnP
+    # Enable UPnP silently and reload firewall
-    firewall_upnp(action=['enable'])
+    firewall_upnp('enable', no_refresh=True)
-    try:
+    firewall_reload()
-        firewall_reload()
-    except MoulinetteError:
-        firewall_upnp(action=['disable'])
 
     # Enable iptables at boot time
     os.system('update-rc.d yunohost-firewall defaults')