From b18a80f0f2201295d371005bf30d8d836c4e982c Mon Sep 17 00:00:00 2001 From: Joseph Nuthalapati Date: Thu, 4 Oct 2018 17:21:43 -0700 Subject: [PATCH] backups: Implement disabling web configuration during backup - Introduce a BackupApp class to store all information about application being backed up. This cleans up apps lists vs. manifest lists spread out in the code. - Introduce ServiceHandler to abstract dealing with services and web configuration. - Add enable and disable actions in apache action. Reviewed-by: James Valleroy --- actions/apache | 22 +++ plinth/modules/backups/__init__.py | 8 +- plinth/modules/backups/api.py | 209 ++++++++++++++++------- plinth/modules/backups/forms.py | 35 ++-- plinth/modules/backups/tests/test_api.py | 167 +++++++++--------- plinth/modules/backups/views.py | 4 +- 6 files changed, 279 insertions(+), 166 deletions(-) diff --git a/actions/apache b/actions/apache index 61f0ad66e..b39d5bf76 100755 --- a/actions/apache +++ b/actions/apache @@ -36,6 +36,16 @@ def parse_arguments(): 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']) subparsers.required = True return parser.parse_args() @@ -148,6 +158,18 @@ def subcommand_setup(arguments): webserver.enable('plinth-ssl', kind='site') +# TODO: Check that the (name, kind) is a managed by FreedomBox before +# performing operation. +def subcommand_enable(arguments): + """Enable an Apache site/config/module.""" + action_utils.webserver_enable(arguments.name, arguments.kind) + + +def subcommand_disable(arguments): + """Disable an Apache site/config/module.""" + action_utils.webserver_disable(arguments.name, arguments.kind) + + def main(): """Parse arguments and perform all duties""" arguments = parse_arguments() diff --git a/plinth/modules/backups/__init__.py b/plinth/modules/backups/__init__.py index 19ca99338..d9402cb25 100644 --- a/plinth/modules/backups/__init__.py +++ b/plinth/modules/backups/__init__.py @@ -86,10 +86,10 @@ def _backup_handler(packet): manifest_path = os.path.join(MANIFESTS_FOLDER, get_valid_filename(packet.label) + '.json') manifests = [{ - 'name': manifest[0], - 'version': manifest[1].version, - 'backup': manifest[2] - } for manifest in packet.manifests] + 'name': app.name, + 'version': app.app.version, + 'backup': app.manifest + } for app in packet.apps] with open(manifest_path, 'w') as manifest_file: json.dump(manifests, manifest_file) diff --git a/plinth/modules/backups/api.py b/plinth/modules/backups/api.py index b4b3016f1..43b60e7c0 100644 --- a/plinth/modules/backups/api.py +++ b/plinth/modules/backups/api.py @@ -25,8 +25,6 @@ TODO: - Implement unit tests. """ -import collections - from plinth import actions, action_utils, module_loader @@ -49,7 +47,9 @@ def validate(backup): if 'services' in backup: assert isinstance(backup['services'], list) for service in backup['services']: - assert isinstance(service, str) + assert isinstance(service, (str, dict)) + if isinstance(service, dict): + _validate_service(service) return backup @@ -67,10 +67,19 @@ def _validate_directories_and_files(section): assert isinstance(file_path, str) +def _validate_service(service): + """Validate a service manifest provided as a dictionary.""" + assert isinstance(service['name'], str) + assert isinstance(service['type'], str) + assert service['type'] in ('apache', 'uwsgi', 'system') + if service['type'] == 'apache': + assert service['kind'] in ('config', 'site', 'module') + + class Packet: """Information passed to a handlers for backup/restore operations.""" - def __init__(self, operation, scope, root, manifests=None, label=None): + def __init__(self, operation, scope, root, apps=None, label=None): """Initialize the packet. operation is either 'backup' or 'restore. @@ -86,7 +95,7 @@ class Packet: self.operation = operation self.scope = scope self.root = root - self.manifests = manifests + self.apps = apps self.label = label self.directories = [] @@ -96,12 +105,11 @@ class Packet: def _process_manifests(self): """Look at manifests and fill up the list of directories/files.""" - for manifest in self.manifests: - backup = manifest[2] + for app in self.apps: for section in ['config', 'data', 'secrets']: - self.directories += backup.get(section, {}).get( + self.directories += app.manifest.get(section, {}).get( 'directories', []) - self.files += backup.get(section, {}).get('files', []) + self.files += app.manifest.get(section, {}).get('files', []) def backup_full(backup_handler, label=None): @@ -138,19 +146,17 @@ def backup_apps(backup_handler, app_names=None, label=None): else: apps = _get_apps_in_order(app_names) - manifests = _get_manifests(apps) - if _is_snapshot_available(): snapshot = _take_snapshot() backup_root = snapshot['mount_path'] snapshotted = True else: _lockdown_apps(apps, lockdown=True) - original_state = _shutdown_services(manifests) + original_state = _shutdown_services(apps) backup_root = '/' snapshotted = False - packet = Packet('backup', 'apps', backup_root, manifests, label) + packet = Packet('backup', 'apps', backup_root, apps, label) _run_operation(backup_handler, packet) if snapshotted: @@ -168,19 +174,17 @@ def restore_apps(restore_handler, app_names=None, create_subvolume=True, else: apps = _get_apps_in_order(app_names) - manifests = _get_manifests(apps) - if _is_snapshot_available() and create_subvolume: subvolume = _create_subvolume(empty=False) restore_root = subvolume['mount_path'] subvolume = True else: _lockdown_apps(apps, lockdown=True) - original_state = _shutdown_services(manifests) + original_state = _shutdown_services(apps) restore_root = '/' subvolume = False - packet = Packet('restore', 'apps', restore_root, manifests, backup_file) + packet = Packet('restore', 'apps', restore_root, apps, backup_file) _run_operation(restore_handler, packet) if subvolume: @@ -190,23 +194,42 @@ def restore_apps(restore_handler, app_names=None, create_subvolume=True, _lockdown_apps(apps, lockdown=False) +class BackupApp: + """A application that can be backed up and its manifest.""" + + def __init__(self, name, app): + """Initialize object and load manfiest.""" + self.name = name + self.app = app + + # Not installed + if app.setup_helper.get_state() == 'needs-setup': + raise TypeError + + # Has no backup related meta data + try: + self.manifest = app.backup + except AttributeError: + raise TypeError + + self.has_data = bool(app.backup) + + def __eq__(self, other_app): + """Check if this app is same as another.""" + return self.name == other_app.name and \ + self.app == other_app.app and \ + self.manifest == other_app.manifest and \ + self.has_data == other_app.has_data + + def get_all_apps_for_backup(): """Return a list of all applications that can be backed up.""" apps = [] for module_name, module in module_loader.loaded_modules.items(): - # Not installed - if module.setup_helper.get_state() == 'needs-setup': - continue - - # Has no backup related meta data - if not hasattr(module, 'backup'): - continue - - apps.append({ - 'name': module_name, - 'app': module, - 'has_data': bool(module.backup) - }) + try: + apps.append(BackupApp(module_name, module)) + except TypeError: # Application not available for backup/restore + pass return apps @@ -216,26 +239,11 @@ def _get_apps_in_order(app_names): apps = [] for module_name, module in module_loader.loaded_modules.items(): if module_name in app_names: - apps.append((module_name, module)) + apps.append(BackupApp(module_name, module)) return apps -def _get_manifests(apps): - """Return a dictionary of apps' backup manifest data. - - Maintain the application order in returned data. - """ - manifests = [] - for app_name, app in apps: - try: - manifests.append((app_name, app, app.backup)) - except AttributeError: - pass - - return manifests - - def _lockdown_apps(apps, lockdown): """Mark apps as in/out of lockdown mode and disable all user interaction. @@ -243,8 +251,8 @@ def _lockdown_apps(apps, lockdown): will intercept all interaction and show a lockdown message. """ - for _, app in apps: - app.locked = lockdown + for app in apps: + app.app.locked = lockdown # XXX: Lockdown the application UI by implementing a middleware @@ -293,7 +301,89 @@ def _switch_to_subvolume(subvolume): raise NotImplementedError -def _shutdown_services(manifests): +class ServiceHandler: + """Abstraction to help with service shutdown/restart.""" + + @staticmethod + def create(backup_app, service): + service_type = 'system' + if isinstance(service, dict): + service_type = service['type'] + + service_map = { + 'system': SystemServiceHandler, + 'apache': ApacheServiceHandler, + } + assert service_type in service_map + return service_map[service_type](backup_app, service) + + def __init__(self, backup_app, service): + """Initialize the object.""" + self.backup_app = backup_app + self.service = service + + def stop(self): + """Stop the service.""" + raise NotImplementedError + + def restart(self): + """Stop the service.""" + raise NotImplementedError + + def __eq__(self, other_handler): + """Compare that two handlers are the same.""" + return self.backup_app == other_handler.backup_app and \ + self.service == other_handler.service + + +class SystemServiceHandler(ServiceHandler): + """Handle starting and stoping of system services for backup.""" + + def __init__(self, backup_app, service): + """Initialize the object.""" + super().__init__(backup_app, service) + self.was_running = None + + def stop(self): + """Stop the service.""" + self.was_running = action_utils.service_is_running(self.service) + if self.was_running: + actions.superuser_run('service', ['stop', self.service]) + + def restart(self): + """Restart the service if it was earlier running.""" + if self.was_running: + actions.superuser_run('service', ['start', self.service]) + + +class ApacheServiceHandler(ServiceHandler): + """Handle starting and stoping of Apache services for backup.""" + + def __init__(self, backup_app, service): + """Initialize the object.""" + super().__init__(backup_app, service) + self.was_enabled = None + self.web_name = service['name'] + self.kind = service['kind'] + + def stop(self): + """Stop the service.""" + 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]) + + 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]) + + +def _shutdown_services(apps): """Shutdown all services specified by manifests. - Services are shutdown in the reverse order of the apps listing. @@ -301,19 +391,13 @@ def _shutdown_services(manifests): Return the current state of the services so they can be restored accurately. """ - state = collections.OrderedDict() - for app_name, app, manifest in manifests: - for service in manifest.get('services', []): - if service not in state: - state[service] = {'app_name': app_name, 'app': app} - - for service in state: - state[service]['was_running'] = action_utils.service_is_running( - service) + state = [] + for app in apps: + for service in app.manifest.get('services', []): + state.append(ServiceHandler.create(app, service)) for service in reversed(state): - if state[service]['was_running']: - actions.superuser_run('service', ['stop', service]) + service.stop() return state @@ -323,9 +407,8 @@ def _restore_services(original_state): Maintain exact order of services so dependencies are satisfied. """ - for service in original_state: - if original_state[service]['was_running']: - actions.superuser_run('service', ['start', service]) + for service_handler in original_state: + service_handler.restart() def _run_operation(handler, packet): diff --git a/plinth/modules/backups/forms.py b/plinth/modules/backups/forms.py index 088ed2aba..7ef4c211c 100644 --- a/plinth/modules/backups/forms.py +++ b/plinth/modules/backups/forms.py @@ -33,12 +33,12 @@ def _get_app_choices(apps): """Return a list of check box multiple choices from list of apps.""" choices = [] for app in apps: - name = app['app'].name - if not app['has_data']: + name = app.app.name + if not app.has_data: name = ugettext('{app} (No data to backup)').format( - app=app['app'].name) + app=app.app.name) - choices.append((app['name'], name)) + choices.append((app.name, name)) return choices @@ -59,7 +59,7 @@ class CreateArchiveForm(forms.Form): super().__init__(*args, **kwargs) apps = api.get_all_apps_for_backup() self.fields['selected_apps'].choices = _get_app_choices(apps) - self.fields['selected_apps'].initial = [app['name'] for app in apps] + self.fields['selected_apps'].initial = [app.name for app in apps] class ExportArchiveForm(forms.Form): @@ -85,21 +85,18 @@ class RestoreForm(forms.Form): apps = kwargs.pop('apps') super().__init__(*args, **kwargs) self.fields['selected_apps'].choices = _get_app_choices(apps) - self.fields['selected_apps'].initial = [app['name'] for app in apps] + self.fields['selected_apps'].initial = [app.name for app in apps] class UploadForm(forms.Form): location = forms.ChoiceField( - choices=(), - label=_('Location'), - initial='', - widget=forms.Select(), - required=True, - help_text=_('Location to upload the archive to')) - file = forms.FileField(label=_('Upload File'), required=True, - validators=[FileExtensionValidator(['gz'], - 'Backup files have to be in .tar.gz format')], - help_text=_('Select the backup file you want to upload')) + choices=(), label=_('Location'), initial='', widget=forms.Select(), + required=True, help_text=_('Location to upload the archive to')) + file = forms.FileField( + label=_('Upload File'), required=True, validators=[ + FileExtensionValidator(['gz'], + 'Backup files have to be in .tar.gz format') + ], help_text=_('Select the backup file you want to upload')) def __init__(self, *args, **kwargs): """Initialize the form with location choices.""" @@ -107,7 +104,8 @@ class UploadForm(forms.Form): locations = get_export_locations() # users should only be able to select a location name -- don't # provide paths as a form input for security reasons - location_labels = [(location[1], location[1]) for location in locations] + location_labels = [(location[1], location[1]) + for location in locations] self.fields['location'].choices = location_labels def clean(self): @@ -120,6 +118,7 @@ class UploadForm(forms.Form): if (file and file.name): filepath = get_archive_path(location_path, file.name) if os.path.exists(filepath): - raise forms.ValidationError("File %s already exists" % file.name) + raise forms.ValidationError( + "File %s already exists" % file.name) else: self.cleaned_data.update({'filepath': filepath}) diff --git a/plinth/modules/backups/tests/test_api.py b/plinth/modules/backups/tests/test_api.py index c9b257482..be6e377d6 100644 --- a/plinth/modules/backups/tests/test_api.py +++ b/plinth/modules/backups/tests/test_api.py @@ -18,14 +18,14 @@ Tests for backups module API. """ -import collections -from django.core.files.uploadedfile import SimpleUploadedFile import unittest from unittest.mock import MagicMock, call, patch +from django.core.files.uploadedfile import SimpleUploadedFile + from plinth import cfg, module_loader from plinth.errors import PlinthError -from plinth.module_loader import load_modules + from .. import api, forms, get_export_locations, get_location_path # pylint: disable=protected-access @@ -45,10 +45,19 @@ def _get_test_manifest(name): 'directories': ['/etc/' + name + '/secrets.d/'], 'files': ['/etc/' + name + '/secrets'], }, - 'services': [name] + 'services': [name, { + 'type': 'apache', + 'name': name, + 'kind': 'site' + }] }) +def _get_backup_app(name): + """Return a dummy BackupApp object.""" + return api.BackupApp(name, MagicMock(backup=_get_test_manifest(name))) + + class TestBackupProcesses(unittest.TestCase): """Test cases for backup processes""" @@ -61,17 +70,13 @@ class TestBackupProcesses(unittest.TestCase): @staticmethod def test_packet_process_manifests(): """Test that directories/files are collected from manifests.""" - manifests = [ - ('a', None, _get_test_manifest('a')), - ('b', None, _get_test_manifest('b')), - ] - packet = api.Packet('backup', 'apps', '/', manifests) - for manifest in manifests: - backup = manifest[2] + apps = [_get_backup_app('a'), _get_backup_app('b')] + packet = api.Packet('backup', 'apps', '/', apps) + for app in apps: for section in ['config', 'data', 'secrets']: - for directory in backup[section]['directories']: + for directory in app.manifest[section]['directories']: assert directory in packet.directories - for file_path in backup[section]['files']: + for file_path in app.manifest[section]['files']: assert file_path in packet.files @staticmethod @@ -102,82 +107,77 @@ class TestBackupProcesses(unittest.TestCase): module_loader.load_modules() returned_apps = api.get_all_apps_for_backup() - expected_apps = [{ - 'name': 'a', - 'app': apps[0][1], - 'has_data': True - }, { - 'name': 'b', - 'app': apps[1][1], - 'has_data': True - }, { - 'name': 'c', - 'app': apps[2][1], - 'has_data': False - }] + expected_apps = [ + api.BackupApp('a', apps[0][1]), + api.BackupApp('b', apps[1][1]), + api.BackupApp('c', apps[2][1]) + ] self.assertEqual(returned_apps, expected_apps) - def test_export_locations(self): + @staticmethod + def test_export_locations(): """Check get_export_locations returns a list of tuples of length 2.""" locations = get_export_locations() - assert(len(locations)) - assert(len(locations[0]) == 2) + assert locations + assert len(locations[0]) == 2 @staticmethod - def test__get_apps_in_order(): + @patch('plinth.module_loader.loaded_modules.items') + def test__get_apps_in_order(modules): """Test that apps are listed in correct dependency order.""" + apps = [ + ('names', MagicMock(backup=_get_test_manifest('names'))), + ('config', MagicMock(backup=_get_test_manifest('config'))), + ] + modules.return_value = apps + module_loader.load_modules() app_names = ['config', 'names'] apps = api._get_apps_in_order(app_names) - ordered_app_names = [app[0] for app in apps] - - names_index = ordered_app_names.index('names') - config_index = ordered_app_names.index('config') - assert names_index < config_index - - @staticmethod - def test__get_manifests(): - """Test that manifests are collected from the apps.""" - app_a = MagicMock(backup=_get_test_manifest('a')) - app_b = MagicMock(backup=_get_test_manifest('b')) - apps = [ - ('a', app_a), - ('b', app_b), - ] - manifests = api._get_manifests(apps) - assert ('a', app_a, app_a.backup) in manifests - assert ('b', app_b, app_b.backup) in manifests + assert apps[0].name == 'names' + assert apps[1].name == 'config' @staticmethod def test__lockdown_apps(): """Test that locked flag is set for each app.""" app_a = MagicMock(locked=False) app_b = MagicMock(locked=None) - apps = [ - ('a', app_a), - ('b', app_b), - ] + apps = [MagicMock(app=app_a), MagicMock(app=app_b)] api._lockdown_apps(apps, True) assert app_a.locked is True assert app_b.locked is True - @staticmethod + @patch('plinth.action_utils.webserver_is_enabled') @patch('plinth.action_utils.service_is_running') @patch('plinth.actions.superuser_run') - def test__shutdown_services(run, is_running): + def test__shutdown_services(self, run, service_is_running, + webserver_is_enabled): """Test that services are stopped in correct order.""" - manifests = [ - ('a', None, _get_test_manifest('a')), - ('b', None, _get_test_manifest('b')), + apps = [_get_backup_app('a'), _get_backup_app('b')] + service_is_running.return_value = True + webserver_is_enabled.return_value = True + state = api._shutdown_services(apps) + + expected_state = [ + api.ServiceHandler.create(apps[0], + apps[0].manifest['services'][0]), + api.ServiceHandler.create(apps[0], + apps[0].manifest['services'][1]), + api.ServiceHandler.create(apps[1], + apps[1].manifest['services'][0]), + api.ServiceHandler.create(apps[1], apps[1].manifest['services'][1]) ] - is_running.return_value = True - state = api._shutdown_services(manifests) - assert 'a' in state - assert 'b' in state - is_running.assert_any_call('a') - is_running.assert_any_call('b') + self.assertEqual(state, expected_state) + + service_is_running.assert_has_calls([call('b'), call('a')]) + webserver_is_enabled.assert_has_calls( + [call('b', kind='site'), + call('a', kind='site')]) + calls = [ + call('apache', ['disable', '--name', 'b', '--kind', 'site']), call('service', ['stop', 'b']), + call('apache', ['disable', '--name', 'a', '--kind', 'site']), call('service', ['stop', 'a']) ] run.assert_has_calls(calls) @@ -186,19 +186,28 @@ class TestBackupProcesses(unittest.TestCase): @patch('plinth.actions.superuser_run') def test__restore_services(run): """Test that services are restored in correct order.""" - original_state = collections.OrderedDict() - original_state['a'] = { - 'app_name': 'a', - 'app': None, - 'was_running': True - } - original_state['b'] = { - 'app_name': 'b', - 'app': None, - 'was_running': False - } + original_state = [ + api.SystemServiceHandler(None, 'a-service'), + api.SystemServiceHandler(None, 'b-service'), + api.ApacheServiceHandler(None, { + 'name': 'c-service', + 'kind': 'site' + }), + api.ApacheServiceHandler(None, { + 'name': 'd-service', + 'kind': 'site' + }) + ] + original_state[0].was_running = True + original_state[1].was_running = False + original_state[2].was_enabled = True + original_state[3].was_enabled = False api._restore_services(original_state) - run.assert_called_once_with('service', ['start', 'a']) + calls = [ + call('service', ['start', 'a-service']), + call('apache', ['enable', '--name', 'c-service', '--kind', 'site']) + ] + run.assert_has_calls(calls) class TestBackupModule(unittest.TestCase): @@ -218,15 +227,15 @@ class TestBackupModule(unittest.TestCase): location_name = locations[0][1] post_data = {'location': location_name} - # posting a video should fail + # posting a video should fail video_file = SimpleUploadedFile("video.mp4", b"file_content", - content_type="video/mp4") + content_type="video/mp4") form = forms.UploadForm(post_data, {'file': video_file}) self.assertFalse(form.is_valid()) - # posting an archive file should work + # posting an archive file should work archive_file = SimpleUploadedFile("backup.tar.gz", b"file_content", - content_type="application/gzip") + content_type="application/gzip") form = forms.UploadForm(post_data, {'file': archive_file}) form.is_valid() self.assertTrue(form.is_valid()) diff --git a/plinth/modules/backups/views.py b/plinth/modules/backups/views.py index 560b4ca5d..0f8360fc4 100644 --- a/plinth/modules/backups/views.py +++ b/plinth/modules/backups/views.py @@ -60,7 +60,7 @@ class IndexView(TemplateView): context['exports'] = backups.get_export_files() context['subsubmenu'] = subsubmenu apps = api.get_all_apps_for_backup() - context['available_apps'] = [app['name'] for app in apps] + context['available_apps'] = [app.name for app in apps] return context @@ -199,7 +199,7 @@ class RestoreView(SuccessMessageMixin, FormView): included_apps = self._get_included_apps() installed_apps = api.get_all_apps_for_backup() kwargs['apps'] = [ - app for app in installed_apps if app['name'] in included_apps + app for app in installed_apps if app.name in included_apps ] return kwargs