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 <jvalleroy@mailbox.org>

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()

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],
+        'name': app.name,
-        'version': manifest[1].version,
+        'version': app.app.version,
-        'backup': manifest[2]
+        'backup': app.manifest
-    } for manifest in packet.manifests]
+    } for app in packet.apps]
     with open(manifest_path, 'w') as manifest_file:
         json.dump(manifests, manifest_file)
 

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:
+        for app in self.apps:
-            backup = manifest[2]
             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
+        try:
-        if module.setup_helper.get_state() == 'needs-setup':
+            apps.append(BackupApp(module_name, module))
-            continue
+        except TypeError:  # Application not available for backup/restore
-
+            pass
-        # Has no backup related meta data
-        if not hasattr(module, 'backup'):
-            continue
-
-        apps.append({
-            'name': module_name,
-            'app': module,
-            'has_data': bool(module.backup)
-        })
 
     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:
+    for app in apps:
-        app.locked = lockdown
+        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()
+    state = []
-    for app_name, app, manifest in manifests:
+    for app in apps:
-        for service in manifest.get('services', []):
+        for service in app.manifest.get('services', []):
-            if service not in state:
+            state.append(ServiceHandler.create(app, service))
-                state[service] = {'app_name': app_name, 'app': app}
-
-    for service in state:
-        state[service]['was_running'] = action_utils.service_is_running(
-            service)
 
     for service in reversed(state):
-        if state[service]['was_running']:
+        service.stop()
-            actions.superuser_run('service', ['stop', service])
 
     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:
+    for service_handler in original_state:
-        if original_state[service]['was_running']:
+        service_handler.restart()
-            actions.superuser_run('service', ['start', service])
 
 
 def _run_operation(handler, packet):

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
+        name = app.app.name
-        if not app['has_data']:
+        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=(),
+        choices=(), label=_('Location'), initial='', widget=forms.Select(),
-        label=_('Location'),
+        required=True, help_text=_('Location to upload the archive to'))
-        initial='',
+    file = forms.FileField(
-        widget=forms.Select(),
+        label=_('Upload File'), required=True, validators=[
-        required=True,
+            FileExtensionValidator(['gz'],
-        help_text=_('Location to upload the archive to'))
+                                   'Backup files have to be in .tar.gz format')
-    file = forms.FileField(label=_('Upload File'), required=True,
+        ], help_text=_('Select the backup file you want to upload'))
-            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})

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 = [
+        apps = [_get_backup_app('a'), _get_backup_app('b')]
-            ('a', None, _get_test_manifest('a')),
+        packet = api.Packet('backup', 'apps', '/', apps)
-            ('b', None, _get_test_manifest('b')),
+        for app in apps:
-        ]
-        packet = api.Packet('backup', 'apps', '/', manifests)
-        for manifest in manifests:
-            backup = manifest[2]
             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 = [{
+        expected_apps = [
-            'name': 'a',
+            api.BackupApp('a', apps[0][1]),
-            'app': apps[0][1],
+            api.BackupApp('b', apps[1][1]),
-            'has_data': True
+            api.BackupApp('c', apps[2][1])
-        }, {
+        ]
-            'name': 'b',
-            'app': apps[1][1],
-            'has_data': True
-        }, {
-            'name': 'c',
-            'app': apps[2][1],
-            'has_data': False
-        }]
         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 locations
-        assert(len(locations[0]) == 2)
+        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]
+        assert apps[0].name == 'names'
-
+        assert apps[1].name == 'config'
-        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
 
     @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 = [
+        apps = [MagicMock(app=app_a), MagicMock(app=app_b)]
-            ('a', app_a),
-            ('b', 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 = [
+        apps = [_get_backup_app('a'), _get_backup_app('b')]
-            ('a', None, _get_test_manifest('a')),
+        service_is_running.return_value = True
-            ('b', None, _get_test_manifest('b')),
+        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
+        self.assertEqual(state, expected_state)
-        state = api._shutdown_services(manifests)
+
-        assert 'a' in state
+        service_is_running.assert_has_calls([call('b'), call('a')])
-        assert 'b' in state
+        webserver_is_enabled.assert_has_calls(
-        is_running.assert_any_call('a')
+            [call('b', kind='site'),
-        is_running.assert_any_call('b')
+             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 = [
-        original_state['a'] = {
+            api.SystemServiceHandler(None, 'a-service'),
-            'app_name': 'a',
+            api.SystemServiceHandler(None, 'b-service'),
-            'app': None,
+            api.ApacheServiceHandler(None, {
-            'was_running': True
+                'name': 'c-service',
-        }
+                'kind': 'site'
-        original_state['b'] = {
+            }),
-            'app_name': 'b',
+            api.ApacheServiceHandler(None, {
-            'app': None,
+                'name': 'd-service',
-            'was_running': False
+                '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())

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