apache: Use privileged decorator for actions

Tests:

- Initial setup works when a new container is created
- When transmission is enabled/disabled, the web configuration for it is
  enabled/disabled.
- When radicale is enabled/disabled, the uwsgi configuration for it is
  enabled/disabled.
- Sharing web configuration is disabled during backup and re-enabled.

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

actions/upgrades

@@ -468,10 +468,9 @@ def _disable_searx() -> bool:
         '/etc/uwsgi/apps-enabled/searx.ini').exists()
     if searx_is_enabled:
         print('Disabling searx...', flush=True)
-        subprocess.run([
-            '/usr/share/plinth/actions/apache', 'uwsgi-disable', '--name',
-            'searx'
-        ], check=True)
+        subprocess.run(
+            ['/usr/share/plinth/actions/actions', 'apache', 'uwsgi_disable'],
+            input='{"args": ["searx"], "kwargs": {}}'.encode(), check=True)
 
     return searx_is_enabled
 
@@ -486,9 +485,8 @@ def _update_searx(reenable=False):
         if reenable:
             print('Re-enabling searx after upgrade...', flush=True)
             subprocess.run([
-                '/usr/share/plinth/actions/apache', 'uwsgi-enable', '--name',
-                'searx'
-            ], check=True)
+                '/usr/share/plinth/actions/actions', 'apache', 'uwsgi_enable'
+            ], input='{"args": ["searx"], "kwargs": {}}'.encode(), check=True)
 
 
 def _perform_dist_upgrade():

plinth/modules/apache/__init__.py

@@ -1,12 +1,10 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
-FreedomBox app for Apache server.
-"""
+"""FreedomBox app for Apache server."""
+
 import os
 
 from django.utils.translation import gettext_lazy as _
 
-from plinth import actions
 from plinth import app as app_module
 from plinth import cfg
 from plinth.daemon import Daemon, RelatedDaemon
@@ -15,6 +13,8 @@ from plinth.modules.letsencrypt.components import LetsEncrypt
 from plinth.package import Packages
 from plinth.utils import format_lazy, is_valid_user_name
 
+from . import privileged
+
 
 class ApacheApp(app_module.App):
     """FreedomBox app for Apache web server."""
@@ -60,9 +60,7 @@ class ApacheApp(app_module.App):
     def setup(self, old_version):
         """Install and configure the app."""
         super().setup(old_version)
-        actions.superuser_run('apache',
-                              ['setup', '--old-version',
-                               str(old_version)])
+        privileged.setup(old_version)
         self.enable()
 
 
@@ -70,17 +68,17 @@ class ApacheApp(app_module.App):
 
 
 def uws_directory_of_user(user):
-    """Returns the directory of the given user's website."""
+    """Return the directory of the given user's website."""
     return '/home/{}/public_html'.format(user)
 
 
 def uws_url_of_user(user):
-    """Returns the url path of the given user's website."""
+    """Return the url path of the given user's website."""
     return '/~{}/'.format(user)
 
 
 def user_of_uws_directory(directory):
-    """Returns the user of a given user website directory."""
+    """Return the user of a given user website directory."""
     if directory.startswith('/home/'):
         pos_ini = 6
     elif directory.startswith('home/'):
@@ -97,7 +95,7 @@ def user_of_uws_directory(directory):
 
 
 def user_of_uws_url(url):
-    """Returns the user of a given user website url path."""
+    """Return the user of a given user website url path."""
     MISSING = -1
 
     pos_ini = url.find('~')
@@ -113,7 +111,7 @@ def user_of_uws_url(url):
 
 
 def uws_directory_of_url(url):
-    """Returns the directory of the user's website for the given url path.
+    """Return the directory of the user's website for the given url path.
 
     Note: It doesn't return the full OS file path to the url path!
     """
@@ -121,7 +119,7 @@ def uws_directory_of_url(url):
 
 
 def uws_url_of_directory(directory):
-    """Returns the url base path of the user's website for the given OS path.
+    """Return the url base path of the user's website for the given OS path.
 
     Note: It doesn't return the url path for the file!
     """
@@ -129,10 +127,10 @@ def uws_url_of_directory(directory):
 
 
 def get_users_with_website():
-    """Returns a dictionary of users with actual website subdirectory."""
+    """Return a dictionary of users with actual website subdirectory."""
 
     def lst_sub_dirs(directory):
-        """Returns the list of subdirectories of the given directory."""
+        """Return the list of subdirectories of the given directory."""
         return [
             name for name in os.listdir(directory)
             if os.path.isdir(os.path.join(directory, name))

plinth/modules/apache/components.py

@@ -1,7 +1,5 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
-App component for other apps to use Apache configuration functionality.
-"""
+"""App component for other apps to use Apache configuration functionality."""
 
 import re
 import subprocess
@@ -9,7 +7,9 @@ import subprocess
 from django.utils.text import format_lazy
 from django.utils.translation import gettext_lazy
 
-from plinth import action_utils, actions, app
+from plinth import action_utils, app
+
+from . import privileged
 
 
 class Webserver(app.LeaderComponent):
@@ -47,14 +47,11 @@ class Webserver(app.LeaderComponent):
 
     def enable(self):
         """Enable the Apache configuration."""
-        actions.superuser_run(
-            'apache', ['enable', '--name', self.web_name, '--kind', self.kind])
+        privileged.enable(self.web_name, self.kind)
 
     def disable(self):
         """Disable the Apache configuration."""
-        actions.superuser_run(
-            'apache',
-            ['disable', '--name', self.web_name, '--kind', self.kind])
+        privileged.disable(self.web_name, self.kind)
 
     def diagnose(self):
         """Check if the web path is accessible by clients.
@@ -99,13 +96,11 @@ class Uwsgi(app.LeaderComponent):
 
     def enable(self):
         """Enable the uWSGI configuration."""
-        actions.superuser_run('apache',
-                              ['uwsgi-enable', '--name', self.uwsgi_name])
+        privileged.uwsgi_enable(self.uwsgi_name)
 
     def disable(self):
         """Disable the uWSGI configuration."""
-        actions.superuser_run('apache',
-                              ['uwsgi-disable', '--name', self.uwsgi_name])
+        privileged.uwsgi_disable(self.uwsgi_name)
 
     def is_running(self):
         """Return whether the uWSGI daemon is running with configuration."""

plinth/modules/apache/privileged.py

@@ -1,48 +1,13 @@
-#!/usr/bin/python3
-# -*- mode: python -*-
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
-Configuration helper for Apache web server.
-"""
+"""Configure Apache web server."""
 
-import argparse
 import glob
 import os
 import re
 import subprocess
 
 from plinth import action_utils
-
-
-def parse_arguments():
-    """Return parsed command line arguments as dictionary"""
-    parser = argparse.ArgumentParser()
-    subparsers = parser.add_subparsers(dest='subcommand', help='Sub command')
-    subparser = subparsers.add_parser('setup', help='Setup for Apache')
-    subparser.add_argument(
-        '--old-version', type=int, required=True,
-        help='Earlier version of the app that is already setup.')
-    subparser = subparsers.add_parser(
-        'enable', help='Enable a site/config/module in apache')
-    subparser.add_argument('--name',
-                           help='Name of the site/config/module to enable')
-    subparser.add_argument('--kind', choices=['site', 'config', 'module'])
-    subparser = subparsers.add_parser(
-        'disable', help='Disable a site/config/module in apache')
-    subparser.add_argument('--name',
-                           help='Name of the site/config/module to disable')
-    subparser.add_argument('--kind', choices=['site', 'config', 'module'])
-    subparser = subparsers.add_parser(
-        'uwsgi-enable', help='Enable a site/config/module in UWSGI')
-    subparser.add_argument('--name',
-                           help='Name of the site/config/module to enable')
-    subparser = subparsers.add_parser(
-        'uwsgi-disable', help='Disable a site/config/module in UWSGI')
-    subparser.add_argument('--name',
-                           help='Name of the site/config/module to disable')
-
-    subparsers.required = True
-    return parser.parse_args()
+from plinth.actions import privileged
 
 
 def _get_sort_key_of_version(version):
@@ -87,14 +52,15 @@ def _disable_mod_php(webserver):
         webserver.disable('php' + version, kind='module')
 
 
-def subcommand_setup(arguments):
+@privileged
+def setup(old_version: int):
     """Setup Apache configuration."""
     # Regenerate the snakeoil self-signed SSL certificate. This is so that
     # FreedomBox images don't all have the same certificate. When FreedomBox
     # package is installed via apt, don't regenerate. When upgrading to newer
     # version of Apache FreedomBox app and setting up for the first time don't
     # regenerate.
-    if action_utils.is_disk_image() and arguments.old_version == 0:
+    if action_utils.is_disk_image() and old_version == 0:
         subprocess.run([
             'make-ssl-cert', 'generate-default-snakeoil', '--force-overwrite'
         ], check=True)
@@ -178,34 +144,33 @@ def subcommand_setup(arguments):
 
 # TODO: Check that the (name, kind) is a managed by FreedomBox before
 # performing operation.
-def subcommand_enable(arguments):
+@privileged
+def enable(name: str, kind: str):
     """Enable an Apache site/config/module."""
-    action_utils.webserver_enable(arguments.name, arguments.kind)
+    _assert_kind(kind)
+    action_utils.webserver_enable(name, kind)
 
 
-def subcommand_disable(arguments):
+@privileged
+def disable(name: str, kind: str):
     """Disable an Apache site/config/module."""
-    action_utils.webserver_disable(arguments.name, arguments.kind)
+    _assert_kind(kind)
+    action_utils.webserver_disable(name, kind)
 
 
-def subcommand_uwsgi_enable(arguments):
+def _assert_kind(kind: str):
+    """Raise and exception if kind parameter has an unexpected value."""
+    if kind not in ('site', 'config', 'module'):
+        raise ValueError('Invalid value for parameter kind')
+
+
+@privileged
+def uwsgi_enable(name: str):
     """Enable uWSGI configuration and reload."""
-    action_utils.uwsgi_enable(arguments.name)
+    action_utils.uwsgi_enable(name)
 
 
-def subcommand_uwsgi_disable(arguments):
+@privileged
+def uwsgi_disable(name: str):
     """Disable uWSGI configuration and reload."""
-    action_utils.uwsgi_disable(arguments.name)
-
-
-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()
+    action_utils.uwsgi_disable(name)

plinth/modules/apache/tests/test_components.py

@@ -47,27 +47,22 @@ def test_webserver_is_enabled(webserver_is_enabled):
     webserver_is_enabled.assert_has_calls([call('test-config', kind='module')])
 
 
-@patch('plinth.actions.superuser_run')
-def test_webserver_enable(superuser_run):
+@patch('plinth.modules.apache.privileged.enable')
+def test_webserver_enable(enable):
     """Test that enabling webserver configuration works."""
     webserver = Webserver('test-webserver', 'test-config', kind='module')
 
     webserver.enable()
-    superuser_run.assert_has_calls([
-        call('apache', ['enable', '--name', 'test-config', '--kind', 'module'])
-    ])
+    enable.assert_has_calls([call('test-config', 'module')])
 
 
-@patch('plinth.actions.superuser_run')
-def test_webserver_disable(superuser_run):
+@patch('plinth.modules.apache.privileged.disable')
+def test_webserver_disable(disable):
     """Test that disabling webserver configuration works."""
     webserver = Webserver('test-webserver', 'test-config', kind='module')
 
     webserver.disable()
-    superuser_run.assert_has_calls([
-        call('apache',
-             ['disable', '--name', 'test-config', '--kind', 'module'])
-    ])
+    disable.assert_has_calls([call('test-config', 'module')])
 
 
 @patch('plinth.modules.apache.components.diagnose_url')
@@ -132,24 +127,22 @@ def test_uwsgi_is_enabled(uwsgi_is_enabled, service_is_enabled):
     assert not uwsgi.is_enabled()
 
 
-@patch('plinth.actions.superuser_run')
-def test_uwsgi_enable(superuser_run):
+@patch('plinth.modules.apache.privileged.uwsgi_enable')
+def test_uwsgi_enable(enable):
     """Test that enabling uwsgi configuration works."""
     uwsgi = Uwsgi('test-uwsgi', 'test-config')
 
     uwsgi.enable()
-    superuser_run.assert_has_calls(
-        [call('apache', ['uwsgi-enable', '--name', 'test-config'])])
+    enable.assert_has_calls([call('test-config')])
 
 
-@patch('plinth.actions.superuser_run')
-def test_uwsgi_disable(superuser_run):
+@patch('plinth.modules.apache.privileged.uwsgi_disable')
+def test_uwsgi_disable(disable):
     """Test that disabling uwsgi configuration works."""
     uwsgi = Uwsgi('test-uwsgi', 'test-config')
 
     uwsgi.disable()
-    superuser_run.assert_has_calls(
-        [call('apache', ['uwsgi-disable', '--name', 'test-config'])])
+    disable.assert_has_calls([call('test-config')])
 
 
 @patch('plinth.action_utils.service_is_running')

plinth/modules/backups/api.py

@@ -15,6 +15,7 @@ import logging
 from plinth import action_utils, actions
 from plinth import app as app_module
 from plinth import setup
+from plinth.modules.apache import privileged as apache_privileged
 
 from .components import BackupRestore
 
@@ -340,16 +341,12 @@ class ApacheServiceHandler(ServiceHandler):
         self.was_enabled = action_utils.webserver_is_enabled(
             self.web_name, kind=self.kind)
         if self.was_enabled:
-            actions.superuser_run(
-                'apache',
-                ['disable', '--name', self.web_name, '--kind', self.kind])
+            apache_privileged.disable(self.web_name, self.kind)
 
     def restart(self):
         """Restart the service if it was earlier running."""
         if self.was_enabled:
-            actions.superuser_run(
-                'apache',
-                ['enable', '--name', self.web_name, '--kind', self.kind])
+            apache_privileged.enable(self.web_name, self.kind)
 
 
 def _shutdown_services(components):