From c33b192f71275193af851dcd9ed19882095bddd5 Mon Sep 17 00:00:00 2001 From: Joseph Nuthalapati Date: Mon, 22 Oct 2018 13:34:05 -0700 Subject: [PATCH] backups: Don't rely on disk labels during export/restore Disk labels are unreliable. They may not exist. There are not unique and two disks in the system may have the same label. Instead use the device path of the disk/partition. Reviewed-by: James Valleroy --- functional_tests/support/system.py | 4 +- plinth/modules/backups/__init__.py | 45 ++++++++++++------- plinth/modules/backups/forms.py | 9 ++-- plinth/modules/backups/templates/backups.html | 10 ++--- plinth/modules/backups/tests/test_api.py | 28 ++++++++---- plinth/modules/backups/urls.py | 4 +- plinth/modules/backups/views.py | 14 +++--- 7 files changed, 70 insertions(+), 44 deletions(-) diff --git a/functional_tests/support/system.py b/functional_tests/support/system.py index 02a01ef5c..9efde69c5 100644 --- a/functional_tests/support/system.py +++ b/functional_tests/support/system.py @@ -213,8 +213,8 @@ def backup_restore(browser, app_name): browser.visit(default_url) nav_to_module(browser, 'backups') browser.find_link_by_href( - '/plinth/sys/backups/restore/Root%2520Filesystem/_functional_test_' + - app_name + '.tar.gz/').first.click() + '/plinth/sys/backups/restore/%252F/_functional_test_' + app_name + + '.tar.gz/').first.click() submit(browser) diff --git a/plinth/modules/backups/__init__.py b/plinth/modules/backups/__init__.py index 5552016d3..bb00be006 100644 --- a/plinth/modules/backups/__init__.py +++ b/plinth/modules/backups/__init__.py @@ -109,7 +109,9 @@ def delete_archive(name): def export_archive(name, location): - filepath = get_archive_path(location, get_valid_filename(name) + '.tar.gz') + location_path = get_location_path(location) + filepath = get_archive_path(location_path, + get_valid_filename(name) + '.tar.gz') # TODO: that's a full path, not a filename; rename argument actions.superuser_run('backups', ['export', '--name', name, '--filename', filepath]) @@ -117,35 +119,46 @@ def export_archive(name, location): def get_export_locations(): """Return a list of storage locations for exported backup archives.""" - locations = [('/var/lib/freedombox/', _('Root Filesystem'))] + locations = [{ + 'path': '/var/lib/freedombox/', + 'label': _('Root Filesystem'), + 'device': '/' + }] if storage.is_running(): devices = storage.udisks2.list_devices() for device in devices: if 'mount_points' in device and len(device['mount_points']) > 0: - name = device['label'] or device['device'] - locations.append((device['mount_points'][0], name)) + location = { + 'path': device['mount_points'][0], + 'label': device['label'] or device['device'], + 'device': device['device'] + } + locations.append(location) return locations -def get_location_path(label, locations=None): +def get_location_path(device, locations=None): """Returns the location path given a disk label""" if locations is None: locations = get_export_locations() - for (location_path, location_label) in locations: - if location_label == label: - return location_path - raise PlinthError("Could not find path of location %s" % label) + + for location in locations: + if location['device'] == device: + return location['path'] + + raise PlinthError('Could not find path of location %s' % device) def get_export_files(): """Return a dict of exported backup archives found in storage locations.""" locations = get_export_locations() - export_files = {} + export_files = [] for location in locations: output = actions.superuser_run( - 'backups', ['list-exports', '--location', location[0]]) - export_files[location[1]] = json.loads(output) + 'backups', ['list-exports', '--location', location['path']]) + location['files'] = json.loads(output) + export_files.append(location) return export_files @@ -154,9 +167,9 @@ def get_archive_path(location, archive_name): return os.path.join(location, BACKUP_FOLDER_NAME, archive_name) -def find_exported_archive(disk_label, archive_name): +def find_exported_archive(device, archive_name): """Return the full path for the exported archive file.""" - location_path = get_location_path(disk_label) + location_path = get_location_path(device) return get_archive_path(location_path, archive_name) @@ -175,8 +188,8 @@ def _restore_handler(packet): input=locations_data.encode()) -def restore_exported(label, archive_name, apps=None): +def restore_exported(device, archive_name, apps=None): """Restore files from exported backup archive.""" - filename = find_exported_archive(label, archive_name) + filename = find_exported_archive(device, archive_name) api.restore_apps(_restore_handler, app_names=apps, create_subvolume=False, backup_file=filename) diff --git a/plinth/modules/backups/forms.py b/plinth/modules/backups/forms.py index 7ef4c211c..0e41e825b 100644 --- a/plinth/modules/backups/forms.py +++ b/plinth/modules/backups/forms.py @@ -71,7 +71,8 @@ class ExportArchiveForm(forms.Form): def __init__(self, *args, **kwargs): """Initialize the form with disk choices.""" super().__init__(*args, **kwargs) - self.fields['disk'].choices = get_export_locations() + self.fields['disk'].choices = [(location['device'], location['label']) + for location in get_export_locations()] class RestoreForm(forms.Form): @@ -104,7 +105,7 @@ 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]) + location_labels = [(location['device'], location['label']) for location in locations] self.fields['location'].choices = location_labels @@ -112,8 +113,8 @@ class UploadForm(forms.Form): """Check that the uploaded file does not yet exist.""" cleaned_data = super().clean() file = cleaned_data.get('file') - location_label = cleaned_data.get('location') - location_path = get_location_path(location_label) + location_device = cleaned_data.get('location') + location_path = get_location_path(location_device) # if other errors occured before, 'file' won't be in cleaned_data if (file and file.name): filepath = get_archive_path(location_path, file.name) diff --git a/plinth/modules/backups/templates/backups.html b/plinth/modules/backups/templates/backups.html index e38c28f12..3797bd3ac 100644 --- a/plinth/modules/backups/templates/backups.html +++ b/plinth/modules/backups/templates/backups.html @@ -113,18 +113,18 @@ - {% for label, file_list in exports.items %} - {% for name in file_list %} + {% for export in exports %} + {% for name in export.files %} - {{ label }} + {{ export.label }} {{ name }} + href="{% url 'backups:download' export.device|urlencode:'' name|urlencode:'' %}"> {% trans "Download" %} + href="{% url 'backups:restore' export.device|urlencode:'' name|urlencode:'' %}"> {% trans "Restore" %} diff --git a/plinth/modules/backups/tests/test_api.py b/plinth/modules/backups/tests/test_api.py index 73ae04bdd..f2cb71812 100644 --- a/plinth/modules/backups/tests/test_api.py +++ b/plinth/modules/backups/tests/test_api.py @@ -60,6 +60,7 @@ def _get_backup_app(name): class TestBackupApp(unittest.TestCase): """Test the BackupApp class.""" + def test_run_hook(self): """Test running a hook on an application.""" packet = api.Packet('backup', 'apps', '/', []) @@ -73,7 +74,8 @@ class TestBackupApp(unittest.TestCase): app.testhook_pre.reset_mock() app.testhook_pre.side_effect = Exception() backup_app.run_hook(hook, packet) - self.assertEqual(packet.errors, [api.BackupError('hook', app, hook=hook)]) + self.assertEqual(packet.errors, + [api.BackupError('hook', app, hook=hook)]) del app.testhook_pre backup_app.run_hook(hook, packet) @@ -137,10 +139,14 @@ class TestBackupProcesses(unittest.TestCase): @staticmethod def test_export_locations(): - """Check get_export_locations returns a list of tuples of length 2.""" + """Check get_export_locations returns a list of locations.""" locations = get_export_locations() assert locations - assert len(locations[0]) == 2 + for location in locations: + assert isinstance(location, dict) + assert isinstance(location['path'], str) + assert isinstance(location['device'], str) + assert str(location['label']) @staticmethod @patch('plinth.module_loader.loaded_modules.items') @@ -251,16 +257,22 @@ class TestBackupModule(unittest.TestCase): def test_get_location_path(self): """Test the 'get_location_path' method""" - locations = [('/var/www', 'dummy location'), ('/etc', 'dangerous')] - location = get_location_path('dummy location', locations) - self.assertEquals(location, locations[0][0]) + locations = [{ + 'path': '/var/www', + 'device': '/dummy/device' + }, { + 'path': '/etc', + 'device': '/dangerous' + }] + location_path = get_location_path('/dummy/device', locations) + self.assertEqual(location_path, locations[0]['path']) # verify that an unknown location raises an error with self.assertRaises(PlinthError): - get_location_path('unknown location', locations) + get_location_path('/unknown/device', locations) def test_file_upload(self): locations = get_export_locations() - location_name = locations[0][1] + location_name = locations[0]['device'] post_data = {'location': location_name} # posting a video should fail diff --git a/plinth/modules/backups/urls.py b/plinth/modules/backups/urls.py index 8b24714f5..e81b7eb9d 100644 --- a/plinth/modules/backups/urls.py +++ b/plinth/modules/backups/urls.py @@ -28,11 +28,11 @@ urlpatterns = [ url(r'^sys/backups/create/$', CreateArchiveView.as_view(), name='create'), url(r'^sys/backups/export/(?P[^/]+)/$', ExportArchiveView.as_view(), name='export'), - url(r'^sys/backups/download/(?P