storage: Directory selection form improvements

- Action script:
  - must not be root when validating directory
  - return only first validation error
- Directory selection form, transmission, deluge:
  show the download path as it is in the configuration,
  the path is resolved only on form submit.
- Tests: add relative path checks, refactor parametrize code

Signed-off-by: Veiko Aasa <veiko17@disroot.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

actions/storage

@@ -333,6 +333,9 @@ def subcommand_usage_info(_):
 
 def subcommand_validate_directory(arguments):
     """Validate a directory"""
+    if os.geteuid() == 0:
+        raise RuntimeError('You must not be root to run this command')
+
     directory = arguments.path
 
     def part_exists(path):
@@ -349,14 +352,15 @@ def subcommand_validate_directory(arguments):
         if not os.path.exists(directory):
             # doesn't exist
             print('ValidationError: 1')
+            return
 
     if not os.path.isdir(directory):
         # is not a directory
         print('ValidationError: 2')
-    if not os.access(directory, os.R_OK):
+    elif not os.access(directory, os.R_OK):
         # is not readable
         print('ValidationError: 3')
-    if arguments.check_writable or arguments.check_creatable:
+    elif arguments.check_writable or arguments.check_creatable:
         if not os.access(directory, os.W_OK):
             # is not writable
             print('ValidationError: 4')

plinth/modules/deluge/forms.py

@@ -18,5 +18,5 @@ class DelugeForm(DirectorySelectForm):
                                        check_creatable=True)
         super(DelugeForm, self).__init__(
             title=_('Download directory'),
-            default='/var/lib/deluged/Downloads/', validator=validator, *args,
+            default='/var/lib/deluged/Downloads', validator=validator, *args,
             **kw)

plinth/modules/deluge/views.py

@@ -4,7 +4,6 @@ Django views for Deluge.
 """
 
 import json
-import os
 
 from django.contrib import messages
 from django.utils.translation import ugettext as _
@@ -25,8 +24,7 @@ class DelugeAppView(views.AppView):
         status = super().get_initial()
         configuration = json.loads(
             actions.superuser_run('deluge', ['get-configuration']))
-        status['storage_path'] = os.path.normpath(
-            configuration['download_location'])
+        status['storage_path'] = configuration['download_location']
         return status
 
     def form_valid(self, form):

plinth/modules/storage/forms.py

@@ -103,7 +103,7 @@ class DirectorySelectForm(AppForm):
         if title:
             self.fields['storage_dir'].label = title
         self.validator = validator
-        self.default = os.path.normpath(default)
+        self.default = default
         self.set_form_data()
 
     def clean(self):

plinth/modules/storage/tests/test_storage.py

@@ -250,45 +250,25 @@ class TestActions:
             assert output == error
 
     @pytest.mark.usefixtures('needs_not_root')
-    @pytest.mark.parametrize('directory', [{
-        'path': '/missing',
-        'error': '1'
-    }, {
-        'path': '/etc/os-release',
-        'error': '2'
-    }, {
-        'path': '/root',
-        'error': '3'
-    }, {
-        'path': '/',
-        'error': ''
-    }])
-    def test_validate_directory(self, directory):
+    @pytest.mark.parametrize('path,error', [('/missing', '1'),
+                                            ('/etc/os-release', '2'),
+                                            ('/root', '3'), ('/', ''),
+                                            ('/etc/..', '')])
+    def test_validate_directory(self, path, error):
         """Test that directory validation returns expected output."""
-        self.assert_validate_directory(directory['path'], directory['error'])
+        self.assert_validate_directory(path, error)
 
     @pytest.mark.usefixtures('needs_not_root')
-    @pytest.mark.parametrize('directory', [{
-        'path': '/',
-        'error': '4'
-    }, {
-        'path': '/tmp',
-        'error': ''
-    }])
-    def test_validate_directory_writable(self, directory):
+    @pytest.mark.parametrize('path,error', [('/', '4'), ('/tmp', '')])
+    def test_validate_directory_writable(self, path, error):
         """Test that directory writable validation returns expected output."""
-        self.assert_validate_directory(directory['path'], directory['error'],
-                                       check_writable=True)
+        self.assert_validate_directory(path, error, check_writable=True)
 
     @pytest.mark.usefixtures('needs_not_root')
-    @pytest.mark.parametrize('directory', [{
-        'path': '/var/lib/plinth_storage_test_not_exists',
-        'error': '4'
-    }, {
-        'path': '/tmp/plint_storage_test_not_exists',
-        'error': ''
-    }])
-    def test_validate_directory_creatable(self, directory):
+    @pytest.mark.parametrize(
+        'path,error', [('/var/lib/plinth_storage_test_not_exists', '4'),
+                       ('/tmp/plint_storage_test_not_exists', ''),
+                       ('/var/../tmp/plint_storage_test_not_exists', '')])
+    def test_validate_directory_creatable(self, path, error):
         """Test that directory creatable validation returns expected output."""
-        self.assert_validate_directory(directory['path'], directory['error'],
-                                       check_creatable=True)
+        self.assert_validate_directory(path, error, check_creatable=True)

plinth/modules/transmission/forms.py

@@ -18,5 +18,5 @@ class TransmissionForm(DirectorySelectForm):
             username=reserved_usernames[0], check_writable=True)
         super(TransmissionForm, self).__init__(
             title=_('Download directory'),
-            default='/var/lib/transmission-daemon/downloads/',
+            default='/var/lib/transmission-daemon/downloads',
             validator=validator, *args, **kw)

plinth/modules/transmission/views.py

@@ -5,7 +5,6 @@ FreedomBox app for configuring Transmission Server.
 
 import json
 import logging
-import os
 import socket
 
 from django.contrib import messages
@@ -29,8 +28,7 @@ class TransmissionAppView(views.AppView):
         configuration = actions.superuser_run('transmission',
                                               ['get-configuration'])
         configuration = json.loads(configuration)
-        status['storage_path'] = os.path.normpath(
-            configuration['download-dir'])
+        status['storage_path'] = configuration['download-dir']
         status['hostname'] = socket.gethostname()
 
         return status