From e3d4811f5ed18d0641e6e416531974a95f93ea8f Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Thu, 25 Aug 2022 15:05:09 -0700 Subject: [PATCH] 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 Reviewed-by: James Valleroy --- plinth/modules/openvpn/__init__.py | 18 ++--- .../modules/openvpn/privileged.py | 67 ++++--------------- .../openvpn/tests/test_configuration.py | 36 +++++----- plinth/modules/openvpn/views.py | 23 +++---- 4 files changed, 47 insertions(+), 97 deletions(-) rename actions/openvpn => plinth/modules/openvpn/privileged.py (81%) mode change 100755 => 100644 diff --git a/plinth/modules/openvpn/__init__.py b/plinth/modules/openvpn/__init__.py index 4ec97d813..93b09dd3e 100644 --- a/plinth/modules/openvpn/__init__.py +++ b/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): diff --git a/actions/openvpn b/plinth/modules/openvpn/privileged.py old mode 100755 new mode 100644 similarity index 81% rename from actions/openvpn rename to plinth/modules/openvpn/privileged.py index 088995bc5..85682ae32 --- a/actions/openvpn +++ b/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() diff --git a/plinth/modules/openvpn/tests/test_configuration.py b/plinth/modules/openvpn/tests/test_configuration.py index 35dd75d1a..11f8e8cee 100644 --- a/plinth/modules/openvpn/tests/test_configuration.py +++ b/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) diff --git a/plinth/modules/openvpn/views.py b/plinth/modules/openvpn/views.py index 903093bd6..f64ff29c1 100644 --- a/plinth/modules/openvpn/views.py +++ b/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')