From a579c648fd8b0f89264c47487b43beff3ae595b6 Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Thu, 1 Sep 2022 14:13:02 -0700 Subject: [PATCH] ejabberd: Use privileged decorator for actions Tests: - Functional tests work (backup test intermittent failure) - Initial setup works - Domain name is configured properly - FAIL: Changing hostname works (See #2276) - Adding a domain to the system works - Current list of domains shown properly in app page - Setting list of domains works - Showing TURN configuration works - Updating TURN configuration in coturn page works - Enabling/disabling MAM status works - Configure file is updated - App page shows correct status Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- plinth/modules/ejabberd/__init__.py | 53 ++-- .../modules/ejabberd/privileged.py | 240 +++++------------- .../ejabberd/tests/test_turn_config.py | 37 ++- plinth/modules/ejabberd/views.py | 20 +- 4 files changed, 109 insertions(+), 241 deletions(-) rename actions/ejabberd => plinth/modules/ejabberd/privileged.py (58%) mode change 100755 => 100644 diff --git a/plinth/modules/ejabberd/__init__.py b/plinth/modules/ejabberd/__init__.py index 1e768b019..22276b5de 100644 --- a/plinth/modules/ejabberd/__init__.py +++ b/plinth/modules/ejabberd/__init__.py @@ -1,15 +1,13 @@ # SPDX-License-Identifier: AGPL-3.0-or-later -""" -FreedomBox app to configure ejabberd server. -""" +"""FreedomBox app to configure ejabberd server.""" import json import logging +from typing import Tuple 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 @@ -25,7 +23,7 @@ from plinth.signals import (domain_added, post_hostname_change, pre_hostname_change) from plinth.utils import format_lazy -from . import manifest +from . import manifest, privileged _description = [ _('XMPP is an open and standardized communication protocol. Here ' @@ -131,14 +129,12 @@ class EjabberdApp(app_module.App): domainname = config.get_domainname() logger.info('ejabberd service domainname - %s', domainname) - actions.superuser_run('ejabberd', - ['pre-install', '--domainname', domainname]) + privileged.pre_install(domainname) # XXX: Configure all other domain names super().setup(old_version) self.get_component('letsencrypt-ejabberd').setup_certificates( [domainname]) - actions.superuser_run('ejabberd', - ['setup', '--domainname', domainname]) + privileged.setup(domainname) self.enable() # Configure STUN/TURN only if there's a valid TLS domain set for Coturn @@ -155,19 +151,14 @@ class EjabberdTurnConsumer(TurnConsumer): def on_pre_hostname_change(sender, old_hostname, new_hostname, **kwargs): - """ - Backup ejabberd database before hostname is changed. - """ + """Backup ejabberd database before hostname is changed.""" del sender # Unused del kwargs # Unused app = app_module.App.get('ejabberd') if app.needs_setup(): return - actions.superuser_run('ejabberd', [ - 'pre-change-hostname', '--old-hostname', old_hostname, - '--new-hostname', new_hostname - ]) + privileged.pre_change_hostname(old_hostname, new_hostname) def on_post_hostname_change(sender, old_hostname, new_hostname, **kwargs): @@ -178,21 +169,16 @@ def on_post_hostname_change(sender, old_hostname, new_hostname, **kwargs): if app.needs_setup(): return - actions.superuser_run('ejabberd', [ - 'change-hostname', '--old-hostname', old_hostname, '--new-hostname', - new_hostname - ], run_in_background=True) + privileged.change_hostname(_run_in_background=True) def get_domains(): - """Return the list of domains configured for ejabberd. - """ + """Return the list of domains configured for ejabberd.""" app = app_module.App.get('ejabberd') if app.needs_setup(): return [] - output = actions.superuser_run('ejabberd', ['get-domains']) - return json.loads(output) + return privileged.get_domains() def on_domain_added(sender, domain_type, name='', description='', @@ -204,7 +190,7 @@ def on_domain_added(sender, domain_type, name='', description='', domains = get_domains() if name not in domains: - actions.superuser_run('ejabberd', ['add-domain', '--domainname', name]) + privileged.add_domain(name) app.get_component('letsencrypt-ejabberd').setup_certificates() @@ -214,9 +200,7 @@ def set_domains(domains): if not domains or app.needs_setup(): return - commands = ['set-domains', '--domains'] - commands.extend(domains) - actions.superuser_run('ejabberd', commands) + privileged.set_domains(domains) app.get_component('letsencrypt-ejabberd').setup_certificates() @@ -227,14 +211,11 @@ def update_turn_configuration(config: TurnConfiguration, managed=True, if not force and app.needs_setup(): return - params = ['configure-turn'] - params += ['--managed'] if managed else [] - actions.superuser_run('ejabberd', params, input=config.to_json().encode()) + privileged.configure_turn(json.loads(config.to_json()), managed) -def get_turn_configuration() -> (TurnConfiguration, bool): +def get_turn_configuration() -> Tuple[TurnConfiguration, bool]: """Get the latest STUN/TURN configuration.""" - json_config = actions.superuser_run('ejabberd', ['get-turn-config']) - tc, managed = json.loads(json_config) - return (TurnConfiguration(tc['domain'], tc['uris'], - tc['shared_secret']), managed) + tc, managed = privileged.get_turn_config() + return TurnConfiguration(tc['domain'], tc['uris'], + tc['shared_secret']), managed diff --git a/actions/ejabberd b/plinth/modules/ejabberd/privileged.py old mode 100755 new mode 100644 similarity index 58% rename from actions/ejabberd rename to plinth/modules/ejabberd/privileged.py index 42de818f5..4e6037d1f --- a/actions/ejabberd +++ b/plinth/modules/ejabberd/privileged.py @@ -1,24 +1,23 @@ -#!/usr/bin/python3 # SPDX-License-Identifier: AGPL-3.0-or-later -""" -Configuration helper for the ejabberd service -""" +"""Configuration helper for the ejabberd service.""" -import argparse -import json +import logging import os import re import shutil import socket import subprocess -import sys from pathlib import Path +from typing import Any, Optional, Tuple from ruamel.yaml import YAML, scalarstring from plinth import action_utils +from plinth.actions import privileged from plinth.version import Version +logger = logging.getLogger(__name__) + EJABBERD_CONFIG = '/etc/ejabberd/ejabberd.yml' EJABBERD_BACKUP = '/var/log/ejabberd/ejabberd.dump' EJABBERD_BACKUP_NEW = '/var/log/ejabberd/ejabberd_new.dump' @@ -34,88 +33,9 @@ yaml.preserve_quotes = True TURN_URI_REGEX = r'(stun|turn):(.*):([0-9]{4})\?transport=(tcp|udp)' -def parse_arguments(): - """Return parsed command line arguments as dictionary""" - parser = argparse.ArgumentParser() - subparsers = parser.add_subparsers(dest='subcommand', help='Sub command') - - # Get configuration - subparsers.add_parser('get-configuration', - help='Return the current configuration') - - # Preseed debconf values before packages are installed. - pre_install = subparsers.add_parser( - 'pre-install', - help='Preseed debconf values before packages are installed.') - pre_install.add_argument( - '--domainname', - help='The domain name that will be used by the XMPP service.') - - # Setup ejabberd configuration - setup = subparsers.add_parser('setup', help='Setup ejabberd configuration') - setup.add_argument( - '--domainname', - help='The domain name that will be used by the XMPP service.') - - # Prepare ejabberd for hostname change - pre_hostname_change = subparsers.add_parser( - 'pre-change-hostname', help='Prepare ejabberd for nodename change') - pre_hostname_change.add_argument('--old-hostname', - help='Previous hostname') - pre_hostname_change.add_argument('--new-hostname', help='New hostname') - - # Update ejabberd nodename - hostname_change = subparsers.add_parser('change-hostname', - help='Update ejabberd nodename') - hostname_change.add_argument('--old-hostname', help='Previous hostname') - hostname_change.add_argument('--new-hostname', help='New hostname') - - # Manage domain names - subparsers.add_parser('get-domains', - help='Get all configured domains in JSON format') - - add_domain = subparsers.add_parser('add-domain', - help='Add a domain name to ejabberd') - add_domain.add_argument('--domainname', help='New domain name') - - set_domains = subparsers.add_parser('set-domains', - help='Set list of ejabberd domains') - set_domains.add_argument('--domains', nargs='+', - help='One or more domain names') - - # Configure STUN/TURN server for use with ejabberd - turn = subparsers.add_parser( - 'configure-turn', help='Configure a TURN server for use with ejabberd') - turn.add_argument( - '--managed', required=False, default=False, action='store_true', - help='Whether configuration is provided by user or auto-managed by ' - 'FreedomBox') - - subparsers.add_parser( - 'get-turn-config', - help='Get the latest STUN/TURN configuration in JSON format') - - # Switch/check Message Archive Management (MAM) in ejabberd config - help_MAM = 'Switch or check Message Archive Management (MAM).' - mam = subparsers.add_parser('mam', help=help_MAM) - mam.add_argument('command', choices=('enable', 'disable', 'status'), - help=help_MAM) - - subparsers.required = True - return parser.parse_args() - - -def subcommand_get_configuration(_): - """Return the current configuration, specifically domains configured.""" - with open(EJABBERD_CONFIG, 'r', encoding='utf-8') as file_handle: - conf = yaml.load(file_handle) - - print(json.dumps({'domains': conf['hosts']})) - - -def subcommand_pre_install(arguments): +@privileged +def pre_install(domainname: str): """Preseed debconf values before packages are installed.""" - domainname = arguments.domainname if not domainname: # If new domainname is blank, use hostname instead. domainname = socket.gethostname() @@ -124,8 +44,9 @@ def subcommand_pre_install(arguments): ['ejabberd ejabberd/hostname string ' + domainname]) -def subcommand_setup(arguments): - """Enabled LDAP authentication""" +@privileged +def setup(domainname: str): + """Enable LDAP authentication.""" with open(EJABBERD_CONFIG, 'r', encoding='utf-8') as file_handle: conf = yaml.load(file_handle) @@ -143,19 +64,20 @@ def subcommand_setup(arguments): with open(EJABBERD_CONFIG, 'w', encoding='utf-8') as file_handle: yaml.dump(conf, file_handle) - upgrade_config(arguments.domainname) + _upgrade_config(domainname) try: subprocess.check_output(['ejabberdctl', 'restart']) except subprocess.CalledProcessError as err: - print('Failed to restart ejabberd with new configuration: %s', err) + logger.warn('Failed to restart ejabberd with new configuration: %s', + err) -def upgrade_config(domain): - """Fix the config file by removing deprecated settings""" +def _upgrade_config(domain): + """Fix the config file by removing deprecated settings.""" current_version = _get_version() if not current_version: - print('Warning: Unable to get ejabberd version.') + logger.warn('Warning: Unable to get ejabberd version.') with open(EJABBERD_CONFIG, 'r', encoding='utf-8') as file_handle: conf = yaml.load(file_handle) @@ -194,31 +116,25 @@ def upgrade_config(domain): yaml.dump(conf, file_handle) -def subcommand_pre_change_hostname(arguments): - """Prepare ejabberd for hostname change""" +@privileged +def pre_change_hostname(old_hostname: str, new_hostname: str): + """Prepare ejabberd for hostname change.""" if not shutil.which('ejabberdctl'): - print('ejabberdctl not found. Is ejabberd installed?') + logger.info('ejabberdctl not found') return - old_hostname = arguments.old_hostname - new_hostname = arguments.new_hostname - subprocess.call(['ejabberdctl', 'backup', EJABBERD_BACKUP]) - try: - subprocess.check_output([ - 'ejabberdctl', 'mnesia-change-nodename', - 'ejabberd@' + old_hostname, 'ejabberd@' + new_hostname, - EJABBERD_BACKUP, EJABBERD_BACKUP_NEW - ]) - os.remove(EJABBERD_BACKUP) - except subprocess.CalledProcessError as err: - print('Failed to change hostname in ejabberd backup database: %s', err) + subprocess.check_output([ + 'ejabberdctl', 'mnesia-change-nodename', 'ejabberd@' + old_hostname, + 'ejabberd@' + new_hostname, EJABBERD_BACKUP, EJABBERD_BACKUP_NEW + ]) + os.remove(EJABBERD_BACKUP) -def subcommand_change_hostname(arguments): - """Update ejabberd with new hostname""" +@privileged +def change_hostname(): + """Update ejabberd with new hostname.""" if not shutil.which('ejabberdctl'): - print('ejabberdctl not found. Is ejabberd installed?') return action_utils.service_stop('ejabberd') @@ -238,35 +154,34 @@ def subcommand_change_hostname(arguments): ['ejabberdctl', 'restore', EJABBERD_BACKUP_NEW]) os.remove(EJABBERD_BACKUP_NEW) except subprocess.CalledProcessError as err: - print('Failed to restore ejabberd backup database: %s', err) + logger.error('Failed to restore ejabberd backup database: %s', err) else: - print('Could not load ejabberd backup database: %s not found' % - EJABBERD_BACKUP_NEW) + logger.error('Could not load ejabberd backup database: %s not found' % + EJABBERD_BACKUP_NEW) -def subcommand_get_domains(_): +@privileged +def get_domains() -> list[str]: """Get all configured domains.""" if not shutil.which('ejabberdctl'): - print('ejabberdctl not found. Is ejabberd installed?') - return + return [] with open(EJABBERD_CONFIG, 'r', encoding='utf-8') as file_handle: conf = yaml.load(file_handle) - print(json.dumps(conf['hosts'])) + return conf['hosts'] -def subcommand_add_domain(arguments): +@privileged +def add_domain(domainname: str): """Update ejabberd with new domainname. Restarting ejabberd is handled by letsencrypt-ejabberd component. """ if not shutil.which('ejabberdctl'): - print('ejabberdctl not found. Is ejabberd installed?') + logger.info('ejabberdctl not found') return - domainname = arguments.domainname - # Add updated domainname to ejabberd hosts list. with open(EJABBERD_CONFIG, 'r', encoding='utf-8') as file_handle: conf = yaml.load(file_handle) @@ -281,43 +196,43 @@ def subcommand_add_domain(arguments): # Restarting ejabberd is handled by letsencrypt-ejabberd component. -def subcommand_set_domains(arguments): +@privileged +def set_domains(domains: list[str]): """Set list of ejabberd domains. Restarting ejabberd is handled by letsencrypt-ejabberd component. """ + if not len(domains): + raise ValueError('No domains provided') + if not shutil.which('ejabberdctl'): - print('ejabberdctl not found. Is ejabberd installed?') return with open(EJABBERD_CONFIG, 'r', encoding='utf-8') as file_handle: conf = yaml.load(file_handle) - conf['hosts'] = arguments.domains + conf['hosts'] = domains with open(EJABBERD_CONFIG, 'w', encoding='utf-8') as file_handle: yaml.dump(conf, file_handle) -def subcommand_mam(argument): +@privileged +def mam(command: str) -> Optional[bool]: """Enable, disable, or get status of Message Archive Management (MAM).""" + if command not in ('enable', 'disable', 'status'): + raise ValueError('Invalid command') with open(EJABBERD_CONFIG, 'r', encoding='utf-8') as file_handle: conf = yaml.load(file_handle) if 'modules' not in conf: - print('Found no "modules" entry in ejabberd configuration file.') - return + return None - if argument.command == 'status': - if 'mod_mam' in conf['modules']: - print('enabled') - return - else: - print('disabled') - return + if command == 'status': + return 'mod_mam' in conf['modules'] - if argument.command == 'enable': + if command == 'enable': # Explicitly set the recommended / default settings for mod_mam, # see https://docs.ejabberd.im/admin/configuration/#mod-mam. settings_mod_mam = { @@ -332,13 +247,10 @@ def subcommand_mam(argument): } } conf['modules'].update(settings_mod_mam) - elif argument.command == 'disable': + elif command == 'disable': # disable modules by erasing from config file if 'mod_mam' in conf['modules']: conf['modules'].pop('mod_mam') - else: - print("Unknown command: %s" % argument.command) - return with open(EJABBERD_CONFIG, 'w', encoding='utf-8') as file_handle: yaml.dump(conf, file_handle) @@ -346,6 +258,8 @@ def subcommand_mam(argument): if action_utils.service_is_running('ejabberd'): subprocess.call(['ejabberdctl', 'reload_config']) + return None + def _generate_service(uri: str) -> dict: """Generate ejabberd mod_stun_disco service config from Coturn URI.""" @@ -368,7 +282,8 @@ def _generate_uris(services: list[dict]) -> list[str]: ] -def subcommand_get_turn_config(_): +@privileged +def get_turn_config() -> Tuple[dict[str, Any], bool]: """Get the latest STUN/TURN configuration in JSON format.""" with open(EJABBERD_CONFIG, 'r', encoding='utf-8') as file_handle: conf = yaml.load(file_handle) @@ -377,24 +292,18 @@ def subcommand_get_turn_config(_): managed = os.path.exists(EJABBERD_MANAGED_COTURN) if bool(mod_stun_disco_config): - print( - json.dumps([{ - 'domain': '', - 'uris': _generate_uris(mod_stun_disco_config['services']), - 'shared_secret': mod_stun_disco_config['secret'], - }, managed])) + return { + 'domain': '', + 'uris': _generate_uris(mod_stun_disco_config['services']), + 'shared_secret': mod_stun_disco_config['secret'], + }, managed else: - print( - json.dumps([{ - 'domain': None, - 'uris': [], - 'shared_secret': None - }, managed])) + return {'domain': None, 'uris': [], 'shared_secret': None}, managed -def subcommand_configure_turn(arguments): +@privileged +def configure_turn(turn_server_config: dict[str, Any], managed: bool): """Set parameters for the STUN/TURN server to use with ejabberd.""" - turn_server_config = json.loads(''.join(sys.stdin)) uris = turn_server_config['uris'] mod_stun_disco_config = {} @@ -413,7 +322,7 @@ def subcommand_configure_turn(arguments): with open(EJABBERD_CONFIG, 'w', encoding='utf-8') as file_handle: yaml.dump(conf, file_handle) - if arguments.managed: + if managed: Path(EJABBERD_MANAGED_COTURN).touch() else: Path(EJABBERD_MANAGED_COTURN).unlink(missing_ok=True) @@ -423,7 +332,7 @@ def subcommand_configure_turn(arguments): def _get_version(): - """ Get the current ejabberd version """ + """Get the current ejabberd version.""" try: output = subprocess.check_output(['ejabberdctl', 'status']).decode('utf-8') @@ -435,16 +344,3 @@ def _get_version(): version = str(version_info[1]) return Version(version) return None - - -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/ejabberd/tests/test_turn_config.py b/plinth/modules/ejabberd/tests/test_turn_config.py index 055a01db5..564418a5e 100644 --- a/plinth/modules/ejabberd/tests/test_turn_config.py +++ b/plinth/modules/ejabberd/tests/test_turn_config.py @@ -11,6 +11,7 @@ import pytest from plinth.modules import ejabberd from plinth.modules.coturn.components import TurnConfiguration +from plinth.modules.ejabberd import privileged managed_configuration = TurnConfiguration( 'freedombox.local', [], @@ -20,9 +21,9 @@ overridden_configuration = TurnConfiguration( 'public.coturn.site', [], 'BUeKvKzhAsTZ8MEwMd3yTwpr2uvbOxgWe51aiP02OAGyOlj6WGuCyqj7iaOsbkC7') -actions_name = 'ejabberd' - +pytestmark = pytest.mark.usefixtures('mock_privileged') current_directory = pathlib.Path(__file__).parent +privileged_modules_to_mock = ['plinth.modules.ejabberd.privileged'] @pytest.fixture(name='conf_file') @@ -45,31 +46,27 @@ def fixture_managed_file(tmp_path): @pytest.fixture(autouse=True) -def fixture_set_configuration_paths(actions_module, conf_file, managed_file): +def fixture_set_configuration_paths(conf_file, managed_file): """Setup configuration file paths in action module.""" - actions_module.EJABBERD_CONFIG = conf_file - actions_module.EJABBERD_MANAGED_COTURN = managed_file + privileged.EJABBERD_CONFIG = conf_file + privileged.EJABBERD_MANAGED_COTURN = managed_file @pytest.fixture(name='test_configuration', autouse=True) -def fixture_test_configuration(call_action, conf_file): +def fixture_test_configuration(conf_file): """Use a separate ejabberd configuration for tests. - Patches actions.superuser_run with the fixture call_action. The module state is patched to be 'up-to-date'. """ - with patch('plinth.actions.superuser_run', - call_action), patch('plinth.app.App.get') as app_get: + with patch('plinth.app.App.get') as app_get: app = Mock() app_get.return_value = app app.needs_setup.return_value = False yield -def _set_turn_configuration(monkeypatch, config=managed_configuration, - managed=True): - monkeypatch.setattr('sys.stdin', config.to_json()) - with patch('plinth.action_utils.service_is_running', return_value=False): +def _set_turn_configuration(config=managed_configuration, managed=True): + with patch('plinth.privileged.service.is_running', return_value=False): ejabberd.update_turn_configuration(config, managed=managed) @@ -81,17 +78,17 @@ def _assert_conf(expected_configuration, expected_managed): assert managed == expected_managed -def test_managed_turn_server_configuration(monkeypatch): - _set_turn_configuration(monkeypatch) +def test_managed_turn_server_configuration(): + _set_turn_configuration() _assert_conf(managed_configuration, True) -def test_overridden_turn_server_configuration(monkeypatch): - _set_turn_configuration(monkeypatch, overridden_configuration, False) +def test_overridden_turn_server_configuration(): + _set_turn_configuration(overridden_configuration, False) _assert_conf(overridden_configuration, False) -def test_remove_turn_configuration(monkeypatch): - _set_turn_configuration(monkeypatch) - _set_turn_configuration(monkeypatch, TurnConfiguration(), False) +def test_remove_turn_configuration(): + _set_turn_configuration() + _set_turn_configuration(TurnConfiguration(), False) _assert_conf(TurnConfiguration(), False) diff --git a/plinth/modules/ejabberd/views.py b/plinth/modules/ejabberd/views.py index cca1b102b..eb01d2b1f 100644 --- a/plinth/modules/ejabberd/views.py +++ b/plinth/modules/ejabberd/views.py @@ -1,31 +1,30 @@ # SPDX-License-Identifier: AGPL-3.0-or-later -""" -Views for the Ejabberd module -""" +"""Views for the Ejabberd app.""" from django.contrib import messages from django.utils.translation import gettext as _ -from plinth import actions from plinth.modules import coturn, ejabberd from plinth.modules.coturn.components import TurnConfiguration from plinth.views import AppView +from . import privileged from .forms import EjabberdForm class EjabberdAppView(AppView): """Show ejabberd as a service.""" + app_id = 'ejabberd' template_name = 'ejabberd.html' form_class = EjabberdForm def get_initial(self): - """Initial data to fill in the form.""" + """Return initial data to fill in the form.""" config, managed = ejabberd.get_turn_configuration() return super().get_initial() | { 'domain_names': ejabberd.get_domains(), - 'MAM_enabled': self.is_MAM_enabled(), + 'MAM_enabled': privileged.mam('status'), 'enable_managed_turn': managed, 'turn_uris': '\n'.join(config.uris), 'shared_secret': config.shared_secret @@ -65,9 +64,9 @@ class EjabberdAppView(AppView): def _handle_MAM_configuration(old_config, new_config): # note ejabberd action "enable" or "disable" restarts, if running if new_config['MAM_enabled']: - actions.superuser_run('ejabberd', ['mam', 'enable']) + privileged.mam('enable') else: - actions.superuser_run('ejabberd', ['mam', 'disable']) + privileged.mam('disable') def form_valid(self, form): """Enable/disable a service and set messages.""" @@ -96,8 +95,3 @@ class EjabberdAppView(AppView): messages.success(self.request, _('Configuration updated')) return super().form_valid(form) - - def is_MAM_enabled(self): - """Return whether Message Archive Management (MAM) is enabled.""" - output = actions.superuser_run('ejabberd', ['mam', 'status']) - return output.strip() == 'enabled'