openvpn: Use privileged decorator for actions

Tests:

- Functional tests pass.
- Initial setup completes successfully and does not take very long time.
- Profiles can be downloaded successfully and imported.
- A client an use them to connect.

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

plinth/modules/openvpn/__init__.py

@@ -1,14 +1,11 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
-FreedomBox app to configure OpenVPN server.
-"""
+"""FreedomBox app to configure OpenVPN server."""
 
 import os
 
 from django.urls import reverse_lazy
 from django.utils.translation import gettext_lazy as _
 
-from plinth import actions
 from plinth import app as app_module
 from plinth import cfg, frontpage, menu
 from plinth.daemon import Daemon
@@ -18,7 +15,7 @@ from plinth.modules.users.components import UsersAndGroups
 from plinth.package import Packages
 from plinth.utils import format_lazy
 
-from . import manifest
+from . import manifest, privileged
 
 _description = [
     format_lazy(
@@ -44,7 +41,7 @@ class OpenVPNApp(app_module.App):
     @property
     def can_be_disabled(self):
         """Return whether the app can be disabled."""
-        return is_setup()
+        return privileged.is_setup()
 
     def __init__(self):
         """Create components for the app."""
@@ -102,20 +99,15 @@ class OpenVPNApp(app_module.App):
         Return True when there are no leader components and OpenVPN setup
         is done.
         """
-        return super().is_enabled() and is_setup()
+        return super().is_enabled() and privileged.is_setup()
 
     def setup(self, old_version):
         """Install and configure the app."""
         super().setup(old_version)
-        actions.superuser_run('openvpn', ['setup'])
+        privileged.setup()
         self.enable()
 
 
-def is_setup():
-    """Return whether the service is running."""
-    return actions.superuser_run('openvpn', ['is-setup']).strip() == 'true'
-
-
 def is_using_ecc():
     """Return whether the service is using ECC."""
     if os.path.exists(SERVER_CONFIGURATION_FILE):

plinth/modules/openvpn/privileged.py

@@ -1,16 +1,13 @@
-#!/usr/bin/python3
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
-Configuration helper for OpenVPN server.
-"""
+"""Configure OpenVPN server."""
 
-import argparse
 import os
 import subprocess
 
 import augeas
 
 from plinth import action_utils
+from plinth.actions import privileged
 
 KEYS_DIRECTORY = '/etc/openvpn/freedombox-keys'
 
@@ -120,24 +117,6 @@ CERTIFICATE_CONFIGURATION_ECC = {
 COMMON_ARGS = {'env': CERTIFICATE_CONFIGURATION_ECC, 'cwd': KEYS_DIRECTORY}
 
 
-def parse_arguments():
-    """Return parsed command line arguments as dictionary."""
-    parser = argparse.ArgumentParser()
-    subparsers = parser.add_subparsers(dest='subcommand', help='Sub command')
-
-    subparsers.add_parser('is-setup', help='Return whether setup is completed')
-    subparsers.add_parser('setup', help='Setup OpenVPN server configuration')
-
-    get_profile = subparsers.add_parser(
-        'get-profile', help='Return the OpenVPN profile of a user')
-    get_profile.add_argument('username', help='User to get profile for')
-    get_profile.add_argument('remote_server',
-                             help='The server name for the user to connect')
-
-    subparsers.required = True
-    return parser.parse_args()
-
-
 def _is_using_ecc():
     """Return whether the service is using ECC."""
     if os.path.exists(SERVER_CONFIGURATION_PATH):
@@ -149,17 +128,14 @@ def _is_using_ecc():
     return False
 
 
-def _is_setup():
+@privileged
+def is_setup() -> bool:
     """Return whether setup is complete."""
     return _is_non_empty_file(DH_PARAMS) or os.path.exists(EC_PARAMS_DIR)
 
 
-def subcommand_is_setup(_):
-    """Print whether setup is complete."""
-    print('true' if _is_setup() else 'false')
-
-
-def subcommand_setup(_):
+@privileged
+def setup():
     """Setup configuration, CA and certificates."""
     _write_server_config()
     _create_certificates()
@@ -236,11 +212,9 @@ def _create_certificates():
                           **COMMON_ARGS)
 
 
-def subcommand_get_profile(arguments):
+@privileged
+def get_profile(username: str, remote_server: str) -> str:
     """Return the profile for a user."""
-    username = arguments.username
-    remote_server = arguments.remote_server
-
     if username == 'ca' or username == 'server':
         raise Exception('Invalid username')
 
@@ -264,16 +238,14 @@ def subcommand_get_profile(arguments):
     client_configuration = CLIENT_CONFIGURATION_ECC if _is_using_ecc(
     ) else CLIENT_CONFIGURATION_RSA
 
-    profile = client_configuration.format(ca=ca_string,
-                                          cert=user_certificate_string,
-                                          key=user_key_string,
-                                          remote=remote_server)
-
-    print(profile)
+    return client_configuration.format(ca=ca_string,
+                                       cert=user_certificate_string,
+                                       key=user_key_string,
+                                       remote=remote_server)
 
 
 def set_unique_subject(value):
-    """ Sets the unique_subject value to a particular value"""
+    """Set the unique_subject value to a particular value."""
     aug = load_augeas()
     aug.set('/files' + ATTR_FILE + '/unique_subject', value)
     aug.save()
@@ -300,16 +272,3 @@ def load_augeas():
     aug.set('/augeas/load/Simplevars/incl[last() + 1]', ATTR_FILE)
     aug.load()
     return aug
-
-
-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/openvpn/tests/test_configuration.py

@@ -9,8 +9,10 @@ from unittest.mock import patch
 import pytest
 
 from plinth.modules import openvpn
+from plinth.modules.openvpn import privileged
 
-actions_name = 'openvpn'
+pytestmark = pytest.mark.usefixtures('mock_privileged')
+privileged_modules_to_mock = ['plinth.modules.openvpn.privileged']
 
 
 @pytest.fixture(name='keys_directory')
@@ -19,10 +21,10 @@ def fixture_keys_directory(tmp_path):
 
 
 @pytest.fixture(autouse=True)
-def fixture_set_keys_directory(actions_module, keys_directory):
+def fixture_set_keys_directory(keys_directory):
     """Set the keys directory in the actions module."""
-    actions_module.DH_PARAMS = f'{keys_directory}/pki/dh.pem'
-    actions_module.EC_PARAMS_DIR = f'{keys_directory}/pki/ecparams'
+    privileged.DH_PARAMS = f'{keys_directory}/pki/dh.pem'
+    privileged.EC_PARAMS_DIR = f'{keys_directory}/pki/ecparams'
 
 
 @pytest.fixture(name='conf_file')
@@ -47,21 +49,19 @@ def test_identify_ecc_configuration(conf_file):
         assert openvpn.is_using_ecc()
 
 
-def test_is_setup_with_rsa(keys_directory, call_action):
+def test_is_setup_with_rsa(keys_directory):
     """is_setup should work with RSA configuration."""
-    with patch('plinth.actions.superuser_run', call_action):
-        (keys_directory / 'pki').mkdir()
-        dh_params_file = keys_directory / 'pki' / 'dh.pem'
-        dh_params_file.write_text('some content')
-        assert openvpn.is_setup()
-        os.remove(dh_params_file)
+    (keys_directory / 'pki').mkdir()
+    dh_params_file = keys_directory / 'pki' / 'dh.pem'
+    dh_params_file.write_text('some content')
+    assert privileged.is_setup()
+    os.remove(dh_params_file)
 
 
-def test_is_setup_with_ecc(keys_directory, call_action):
+def test_is_setup_with_ecc(keys_directory):
     """is_setup should work with RSA configuration."""
-    with patch('plinth.actions.superuser_run', call_action):
-        (keys_directory / 'pki' / 'ecparams').mkdir(parents=True)
-        ec_params_file = keys_directory / 'pki' / 'ecparams' / 'somecurve.pem'
-        ec_params_file.write_text('some content')
-        assert openvpn.is_setup()
-        os.remove(ec_params_file)
+    (keys_directory / 'pki' / 'ecparams').mkdir(parents=True)
+    ec_params_file = keys_directory / 'pki' / 'ecparams' / 'somecurve.pem'
+    ec_params_file.write_text('some content')
+    assert privileged.is_setup()
+    os.remove(ec_params_file)

plinth/modules/openvpn/views.py

@@ -1,7 +1,5 @@
 # SPDX-License-Identifier: AGPL-3.0-or-later
-"""
-FreedomBox app for configuring OpenVPN server.
-"""
+"""FreedomBox app for configuring OpenVPN server."""
 
 import logging
 
@@ -9,16 +7,18 @@ from django.http import HttpResponse
 from django.shortcuts import redirect
 from django.views.decorators.http import require_POST
 
-from plinth import actions
 from plinth import app as app_module
 from plinth.modules import config, openvpn
 from plinth.views import AppView
 
+from . import privileged
+
 logger = logging.getLogger(__name__)
 
 
 class OpenVPNAppView(AppView):
     """Show OpenVPN app main page."""
+
     app_id = 'openvpn'
     template_name = 'openvpn.html'
 
@@ -26,7 +26,7 @@ class OpenVPNAppView(AppView):
         """Add additional context data for template."""
         context = super().get_context_data(*args, **kwargs)
         context['status'] = {
-            'is_setup': openvpn.is_setup(),
+            'is_setup': privileged.is_setup(),
         }
         context['using_ecc'] = openvpn.is_using_ecc()
         return context
@@ -35,8 +35,8 @@ class OpenVPNAppView(AppView):
 @require_POST
 def setup(_):
     """Start the setup process."""
-    if not openvpn.is_setup():
-        actions.superuser_run('openvpn', ['setup'], run_in_background=True)
+    if not privileged.is_setup():
+        privileged.setup()
 
     app_module.App.get('openvpn').enable()
 
@@ -51,9 +51,7 @@ def profile(request):
     if not config.get_domainname():
         domainname = config.get_hostname()
 
-    profile_string = actions.superuser_run(
-        'openvpn', ['get-profile', username, domainname])
-
+    profile_string = privileged.get_profile(username, domainname)
     response = HttpResponse(profile_string,
                             content_type='application/x-openvpn-profile')
     response['Content-Disposition'] = \
@@ -65,6 +63,7 @@ def profile(request):
 @require_POST
 def ecc(_):
     """Migrate from RSA to ECC."""
-    if openvpn.is_setup():
-        actions.superuser_run('openvpn', ['setup'], run_in_background=True)
+    if privileged.is_setup():
+        privileged.setup()
+
     return redirect('openvpn:index')