firewall: Use privileged decorator, drop showing running status

- If a daemon is not-running, we already show an error message to the user. Use
that mechanism instead of the custom one.

Tests:

- Functional tests work.
- Initial setup for firewall on first boot works.
  - Default zone of the firewalld is set to external in /etc/firewalld.conf
- Status of various apps is shown properly in the app page
- If firewalld is not running, the app page is still displayed properly and
  message that firewalld is not running is shown.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

plinth/modules/firewall/__init__.py

@@ -1,14 +1,11 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
+"""FreedomBox app to configure a firewall."""
-FreedomBox app to configure a firewall.
-"""
 
 import contextlib
 import logging
 
 from django.utils.translation import gettext_lazy as _
 
-from plinth import actions
 from plinth import app as app_module
 from plinth import cfg, menu
 from plinth.daemon import Daemon
@@ -16,7 +13,7 @@ from plinth.modules.backups.components import BackupRestore
 from plinth.package import Packages, install
 from plinth.utils import Version, format_lazy, import_from_gi
 
-from . import manifest
+from . import manifest, privileged
 
 gio = import_from_gi('Gio', '2.0')
 glib = import_from_gi('GLib', '2.0')
@@ -98,7 +95,7 @@ class FirewallApp(app_module.App):
 
 def _run_setup():
     """Run firewalld setup."""
-    _run(['setup'], superuser=True)
+    privileged.setup()
     add_service('http', 'external')
     add_service('http', 'internal')
     add_service('https', 'external')
@@ -171,17 +168,8 @@ def try_with_reload(operation):
         operation()
 
 
-def get_enabled_status():
-    """Return whether firewall is enabled"""
-    output = _run(['get-status'], superuser=True)
-    if not output:
-        return False
-    else:
-        return output.split()[0] == 'running'
-
-
 def get_enabled_services(zone):
-    """Return the status of various services currently enabled"""
+    """Return the status of various services currently enabled."""
     with ignore_dbus_error(dbus_error='ServiceUnknown'):
         zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE)
         return zone_proxy.getServices('(s)', zone)
@@ -190,7 +178,7 @@ def get_enabled_services(zone):
 
 
 def get_port_details(service_port):
-    """Return the port types and numbers for a service port"""
+    """Return the port types and numbers for a service port."""
     try:
         return _port_details[service_port]
     except KeyError:
@@ -215,7 +203,7 @@ def get_interfaces(zone):
 
 
 def add_service(port, zone):
-    """Enable a service in firewall"""
+    """Enable a service in firewall."""
     with ignore_dbus_error(dbus_error='ServiceUnknown'):
         zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE)
         with ignore_dbus_error(service_error='ALREADY_ENABLED'):
@@ -229,7 +217,7 @@ def add_service(port, zone):
 
 
 def remove_service(port, zone):
-    """Remove a service in firewall"""
+    """Remove a service in firewall."""
     with ignore_dbus_error(dbus_error='ServiceUnknown'):
         zone_proxy = _get_dbus_proxy(_FIREWALLD_OBJECT, _ZONE_INTERFACE)
         with ignore_dbus_error(service_error='NOT_ENABLED'):
@@ -240,13 +228,3 @@ def remove_service(port, zone):
         config_zone = _get_dbus_proxy(zone_path, _CONFIG_ZONE_INTERFACE)
         with ignore_dbus_error(service_error='NOT_ENABLED'):
             config_zone.removeService('(s)', port)
-
-
-def _run(arguments, superuser=False):
-    """Run an given command and raise exception if there was an error"""
-    command = 'firewall'
-
-    if superuser:
-        return actions.superuser_run(command, arguments)
-    else:
-        return actions.run(command, arguments)

plinth/modules/firewall/privileged.py

@@ -1,31 +1,12 @@
-#!/usr/bin/python3
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
+"""Configuration helper for FreedomBox firewall interface."""
-Configuration helper for FreedomBox firewall interface.
-"""
 
-import argparse
 import subprocess
 
 import augeas
 
 from plinth import action_utils
-
+from plinth.actions import privileged
-
-def parse_arguments():
-    """Return parsed command line arguments as dictionary"""
-    parser = argparse.ArgumentParser()
-    subparsers = parser.add_subparsers(dest='subcommand', help='Sub command')
-
-    # Setup
-    subparsers.add_parser('setup', help='Perform basic firewall setup')
-
-    # Get status
-    subparsers.add_parser('get-status',
-                          help='Get whether firewalld is running')
-
-    subparsers.required = True
-    return parser.parse_args()
 
 
 def _flush_iptables_rules():
@@ -81,26 +62,11 @@ def set_firewall_backend(backend):
         action_utils.service_restart('firewalld')
 
 
-def subcommand_setup(_):
+@privileged
+def setup():
     """Perform basic firewalld setup."""
     action_utils.service_enable('firewalld')
-    subprocess.call(['firewall-cmd', '--set-default-zone=external'])
+    subprocess.run(['firewall-cmd', '--set-default-zone=external'],
+                   stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL,
+                   check=True)
     set_firewall_backend('nftables')
-
-
-def subcommand_get_status(_):
-    """Print status of the firewalld service"""
-    subprocess.call(['firewall-cmd', '--state'])
-
-
-def main():
-    """Parse arguments and perform all duties"""
-    arguments = parse_arguments()
-
-    subcommand = arguments.subcommand.replace('-', '_')
-    subcommand_method = globals()['subcommand_' + subcommand]
-    subcommand_method(arguments)
-
-
-if __name__ == "__main__":
-    main()

plinth/modules/firewall/templates/firewall.html

@@ -15,97 +15,82 @@
 
   <h3>{% trans "Status" %}</h3>
 
-  {% if firewall_status == 'not_running' %}
+  <div class="table-responsive">
+    <table class='table table-autowidth'>
+      <thead>
+        <th>{% trans "Service/Port" %}</th>
+        <th>{% trans "Status" %}</th>
+      </thead>
 
-    <p>
+      <tbody>
-      {% blocktrans trimmed %}
+        {% for component in components|dictsort:"name" %}
-        Firewall daemon is not running. Please run it. Firewall comes
+          {% if component.ports %}
-        enabled by default on {{ box_name }}. On any Debian based
+            <tr>
-        system (such as {{ box_name }}) you may run it using the
+              <td class="app-name">
-        command 'service firewalld start' or in case of a system with
+                <a class="dropdown-toggle" href="#"
-        systemd 'systemctl start firewalld'.
+                   data-toggle="collapse" role="button"
-      {% endblocktrans %}
+                   data-target=".{{component.component_id}}"
-    </p>
+                   aria-expanded="false"
-
+                   aria-controls="{{component.component_id}}">
-  {% else %}
+                  {{ component.name }}</a>
-
+              </td>
-    <div class="table-responsive">
+              <td class="app-status">
-      <table class='table table-autowidth'>
+                {% if component.is_enabled %}
-        <thead>
+                  <span class='badge badge-success'>
-          <th>{% trans "Service/Port" %}</th>
+                    {% trans "Enabled" %}</span>
-          <th>{% trans "Status" %}</th>
+                {% else %}
-        </thead>
+                  <span class='badge badge-warning'>
-
+                    {% trans "Disabled" %}</span>
-        <tbody>
+                {% endif %}
-          {% for component in components|dictsort:"name" %}
+              </td>
-            {% if component.ports %}
+            </tr>
-              <tr>
+            {% for port in component.ports_details %}
-                <td class="app-name">
+              <tr class="collapse {{component.component_id}}">
-                  <a class="dropdown-toggle" href="#"
+                <td class='service'>
-                     data-toggle="collapse" role="button"
+                  <span class="service-name">{{ port.name }}</span>:
-                     data-target=".{{component.component_id}}"
+                  {% for port_number, protocol in port.details %}
-                     aria-expanded="false"
+                    {{ port_number }}/{{ protocol }}
-                     aria-controls="{{component.component_id}}">
+                  {% endfor %}
-                    {{ component.name }}</a>
                 </td>
-                <td class="app-status">
+                <td class="service-status">
-                  {% if component.is_enabled %}
+                  {% if port.name in internal_enabled_ports and port.name in external_enabled_ports %}
                     <span class='badge badge-success'>
-                      {% trans "Enabled" %}</span>
+                      {% trans "Permitted" %}</span>
-                  {% else %}
+                  {% elif port.name in internal_enabled_ports %}
                     <span class='badge badge-warning'>
-                      {% trans "Disabled" %}</span>
+                      {% trans "Permitted (internal only)" %}</span>
+                  {% elif port.name in external_enabled_ports %}
+                    <span class='badge badge-warning'>
+                      {% trans "Permitted (external only)" %}</span>
+                  {% else %}
+                    <span class='badge badge-danger'>
+                      {% trans "Blocked" %}</span>
                   {% endif %}
                 </td>
               </tr>
-              {% for port in component.ports_details %}
+            {% endfor %}
-                <tr class="collapse {{component.component_id}}">
+          {% endif %}
-                  <td class='service'>
+        {% endfor %}
-                    <span class="service-name">{{ port.name }}</span>:
+      </tbody>
-                    {% for port_number, protocol in port.details %}
+    </table>
-                      {{ port_number }}/{{ protocol }}
+  </div>
-                    {% endfor %}
-                  </td>
-                  <td class="service-status">
-                    {% if port.name in internal_enabled_ports and port.name in external_enabled_ports %}
-                      <span class='badge badge-success'>
-                        {% trans "Permitted" %}</span>
-                    {% elif port.name in internal_enabled_ports %}
-                      <span class='badge badge-warning'>
-                        {% trans "Permitted (internal only)" %}</span>
-                    {% elif port.name in external_enabled_ports %}
-                      <span class='badge badge-warning'>
-                        {% trans "Permitted (external only)" %}</span>
-                    {% else %}
-                      <span class='badge badge-danger'>
-                        {% trans "Blocked" %}</span>
-                    {% endif %}
-                  </td>
-                </tr>
-              {% endfor %}
-            {% endif %}
-          {% endfor %}
-        </tbody>
-      </table>
-    </div>
 
-    <p>
+  <p>
-      <em>
+    <em>
-        {% blocktrans trimmed %}
-          The operation of the firewall is automatic. When you enable
-          a service it is also permitted in the firewall and when you
-          disable a service it is also disabled in the firewall.
-        {% endblocktrans %}
-      </em>
-    </p>
-
-    <h3>{%trans "Advanced" %} </h3>
-    <p>
       {% blocktrans trimmed %}
-        Advanced firewall operations such as opening custom ports are provided
+        The operation of the firewall is automatic. When you enable
-        by the <a href="/_cockpit/network/firewall">Cockpit</a> app.
+        a service it is also permitted in the firewall and when you
+        disable a service it is also disabled in the firewall.
       {% endblocktrans %}
-    </p>
+    </em>
-  {% endif %}
+  </p>
+
+  <h3>{%trans "Advanced" %} </h3>
+  <p>
+    {% blocktrans trimmed %}
+      Advanced firewall operations such as opening custom ports are provided
+      by the <a href="/_cockpit/network/firewall">Cockpit</a> app.
+    {% endblocktrans %}
+  </p>
 
 {% endblock %}

plinth/modules/firewall/views.py

@@ -1,7 +1,5 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
+"""FreedomBox app to configure a firewall."""
-FreedomBox app to configure a firewall.
-"""
 
 from plinth import views
 from plinth.modules import firewall
@@ -11,23 +9,16 @@ from . import components
 
 class FirewallAppView(views.AppView):
     """Serve firewall index page."""
+
     app_id = 'firewall'
     template_name = 'firewall.html'
 
     def get_context_data(self, *args, **kwargs):
         """Add additional context data for the template."""
         context = super().get_context_data(*args, **kwargs)
-
+        context['components'] = components.Firewall.list()
-        status = 'running' if firewall.get_enabled_status() else 'not_running'
+        internal_enabled_ports = firewall.get_enabled_services(zone='internal')
-        context['firewall_status'] = status
+        external_enabled_ports = firewall.get_enabled_services(zone='external')
-
+        context['internal_enabled_ports'] = internal_enabled_ports
-        if status == 'running':
+        context['external_enabled_ports'] = external_enabled_ports
-            context['components'] = components.Firewall.list()
-            internal_enabled_ports = firewall.get_enabled_services(
-                zone='internal')
-            external_enabled_ports = firewall.get_enabled_services(
-                zone='external')
-            context['internal_enabled_ports'] = internal_enabled_ports
-            context['external_enabled_ports'] = external_enabled_ports
-
         return context