Backup module: added some unittests; minor doc updates

- added unittests
- introduced function backups.get_location_path()
- minor docstring / comment and naming updates

Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

plinth/modules/backups/__init__.py

@@ -18,7 +18,6 @@
 FreedomBox app to manage backup archives.
 """
 
-import errno
 import json
 import os
 
@@ -26,6 +25,7 @@ from django.utils.text import get_valid_filename
 from django.utils.translation import ugettext_lazy as _
 
 from plinth import actions
+from plinth.errors import PlinthError
 from plinth.menu import main_menu
 from plinth.modules import storage
 
@@ -127,6 +127,16 @@ def get_export_locations():
     return locations
 
 
+def get_location_path(label, 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)
+
+
 def get_export_files():
     """Return a dict of exported backup archives found in storage locations."""
     locations = get_export_locations()
@@ -145,14 +155,8 @@ def get_archive_path(location, archive_name):
 
 def find_exported_archive(disk_label, archive_name):
     """Return the full path for the exported archive file."""
-    locations = get_export_locations()
+    location_path = get_location_path(disk_label)
-    for (location_path, location_name) in locations:
+    return get_archive_path(location_path, archive_name)
-        if location_name == disk_label:
-            return get_archive_path(location_path, archive_name)
-
-
-    raise FileNotFoundError(errno.ENOENT, os.strerror(errno.ENOENT),
-                            archive_name)
 
 
 def get_export_apps(filename):

plinth/modules/backups/forms.py

@@ -26,7 +26,7 @@ from django.core.validators import FileExtensionValidator
 from django.utils.translation import ugettext_lazy as _
 
 from . import backups as backups_api
-from . import get_export_locations, get_archive_path
+from . import get_export_locations, get_archive_path, get_location_path
 
 
 class CreateArchiveForm(forms.Form):
@@ -93,16 +93,21 @@ class UploadForm(forms.Form):
     def __init__(self, *args, **kwargs):
         """Initialize the form with location choices."""
         super().__init__(*args, **kwargs)
-        # TODO: write a test that assures the format of get_export_locations
+        locations = get_export_locations()
-        self.fields['location'].choices = 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]
+        self.fields['location'].choices = location_labels
 
     def clean(self):
-        """Check that the uploaded file does not exist"""
+        """Check that the uploaded file does not yet exist."""
         cleaned_data = super().clean()
         file = cleaned_data.get('file')
-        location = cleaned_data.get('location')
+        location_label = cleaned_data.get('location')
+        location_path = get_location_path(location_label)
+        # if other errors occured before, 'file' won't be in cleaned_data
         if (file and file.name):
-            filepath = get_archive_path(location, file.name)
+            filepath = get_archive_path(location_path, file.name)
             if os.path.exists(filepath):
                 raise forms.ValidationError("File %s already exists" % file.name)
             else:

plinth/modules/backups/tests/test_backups.py

@@ -19,13 +19,18 @@ Tests for backups module.
 """
 
 import collections
+from django.core.files.uploadedfile import SimpleUploadedFile
 import unittest
 from unittest.mock import call, patch, MagicMock
 
+
+from plinth.errors import PlinthError
 from plinth.module_loader import load_modules
 from ..backups import validate, Packet, backup_apps, restore_apps, \
     get_all_apps_for_backup, _get_apps_in_order, _get_manifests, \
     _lockdown_apps, _shutdown_services, _restore_services
+from .. import get_export_locations, get_location_path
+from ..forms import UploadForm
 
 
 def _get_test_manifest(name):
@@ -46,8 +51,8 @@ def _get_test_manifest(name):
     })
 
 
-class TestBackups(unittest.TestCase):
+class TestBackupProcesses(unittest.TestCase):
-    """Test cases for backups module."""
+    """Test cases for backup processes"""
 
     def test_packet_process_manifests(self):
         """Test that directories/files are collected from manifests."""
@@ -83,6 +88,12 @@ class TestBackups(unittest.TestCase):
         assert isinstance(apps, list)
         # apps may be empty, if no apps supporting backup are installed.
 
+    def test_export_locations(self):
+        """Check get_export_locations returns a list of tuples of length 2."""
+        locations = get_export_locations()
+        assert(len(locations))
+        assert(len(locations[0]) == 2)
+
     def test__get_apps_in_order(self):
         """Test that apps are listed in correct dependency order."""
         load_modules()
@@ -148,3 +159,34 @@ class TestBackups(unittest.TestCase):
             'app_name': 'b', 'app': None, 'was_running': False}
         _restore_services(original_state)
         run.assert_called_once_with('service', ['start', 'a'])
+
+
+class TestBackupModule(unittest.TestCase):
+    """Tests of the backups django module, like views or forms."""
+
+    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])
+        # verify that an unknown location raises an error
+        with self.assertRaises(PlinthError):
+            get_location_path('unknown location', locations)
+
+    def test_file_upload(self):
+        locations = get_export_locations()
+        location_name = locations[0][1]
+        post_data = {'location': location_name}
+
+	# posting a video should fail
+        video_file = SimpleUploadedFile("video.mp4", b"file_content",
+            content_type="video/mp4")
+        form = UploadForm(post_data, {'file': video_file})
+        self.assertFalse(form.is_valid())
+
+	# posting an archive file should work
+        archive_file = SimpleUploadedFile("backup.tar.gz", b"file_content",
+            content_type="application/gzip")
+        form = UploadForm(post_data, {'file': archive_file})
+        form.is_valid()
+        self.assertTrue(form.is_valid())

plinth/modules/backups/views.py

@@ -138,7 +138,7 @@ class UploadArchiveView(SuccessMessageMixin, FormView):
     prefix = 'backups'
     template_name = 'backups_upload.html'
     success_url = reverse_lazy('backups:index')
-    success_message = _('Archive uploaded.')
+    success_message = _('Backup file uploaded.')
 
     def get_context_data(self, **kwargs):
         """Return additional context for rendering the template."""