kiwix: Use new utility for handling uploads

Earlier, the uploaded ZIM file was being written to disk twice.

Manual Test
-----------
Without the changes in this commit, the English MediaWiki archive of
6.83 GB cannot be uploaded to the dev container of size 12 GB, since two
temporary files are created.

With the changes in this commit, the same file can be uploaded
successfully and accessed using Kiwix reader.

- Uploaded file has expected ownership and permissions.

Signed-off-by: Joseph Nuthalapati <njoseph@riseup.net>
[sunil: Handle error for uploading duplicate content.]
[sunil: Set root:root ownership on the uploaded file.]
[sunil: Use the action utility for checking that the upload file and moving it.]
Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: Veiko Aasa <veiko17@disroot.org>

plinth/modules/kiwix/privileged.py

@@ -3,7 +3,6 @@
 Privileged actions for Kiwix content server.
 """
 
-import os
 import pathlib
 import shutil
 import subprocess
@@ -20,7 +19,7 @@ CONTENT_DIR = KIWIX_HOME / 'content'
 
 
 @privileged
-def add_package(file_name: str):
+def add_package(file_name: str, temporary_file_path: str):
     """Adds a content package to Kiwix.
 
     Adding packages is idempotent.
@@ -37,14 +36,13 @@ def add_package(file_name: str):
 
     # Moving files to the Kiwix library path ensures that
     # they can't be removed by other apps or users.
-    zim_file_name = pathlib.Path(file_name).name
     CONTENT_DIR.mkdir(exist_ok=True)
-    zim_file_dest = str(CONTENT_DIR / zim_file_name)
-    shutil.chown(file_name, 'root', 'root')
-    os.chmod(file_name, 0o644)
-    shutil.move(file_name, zim_file_dest)
+    action_utils.move_uploaded_file(temporary_file_path, CONTENT_DIR,
+                                    file_name, allow_overwrite=False,
+                                    user='root', group='root',
+                                    permissions=0o644)
 
-    _kiwix_manage_add(zim_file_dest)
+    _kiwix_manage_add(str(CONTENT_DIR / file_name))
 
 
 def _kiwix_manage_add(zim_file: str):

plinth/modules/kiwix/tests/test_privileged.py

@@ -9,6 +9,7 @@ from unittest.mock import patch
 
 import pytest
 
+import plinth.settings
 from plinth.modules.kiwix import privileged
 
 pytestmark = pytest.mark.usefixtures('mock_privileged')
@@ -36,23 +37,30 @@ def fixture_kiwix_home(tmp_path):
     shutil.copy(source_file, privileged.LIBRARY_FILE)
 
 
+@pytest.fixture(name='upload_dir')
+def fixture_upload_dir(tmp_path):
+    """Overwrite the Django upload path."""
+    old_value = plinth.settings.FILE_UPLOAD_TEMP_DIR
+    plinth.settings.FILE_UPLOAD_TEMP_DIR = tmp_path
+    yield tmp_path
+    plinth.settings.FILE_UPLOAD_TEMP_DIR = old_value
+
+
 @pytest.fixture(autouse=True)
 def fixture_patch():
     """Patch some underlying methods."""
     with patch('subprocess.check_call'), patch('subprocess.run'), patch(
-            'os.chown'):
+            'shutil.chown'):
         yield
 
 
-def test_add_package(tmp_path):
+def test_add_package(upload_dir):
     """Test adding a content package to Kiwix."""
-    some_dir = tmp_path / 'some' / 'dir'
-    some_dir.mkdir(parents=True, exist_ok=True)
     zim_file_name = 'wikipedia_en_all_maxi_2022-05.zim'
-    orig_file = some_dir / zim_file_name
+    orig_file = upload_dir / zim_file_name
     orig_file.touch()
 
-    privileged.add_package(str(orig_file))
+    privileged.add_package(zim_file_name, str(orig_file))
     assert (privileged.KIWIX_HOME / 'content' / zim_file_name).exists()
     assert not orig_file.exists()
 

plinth/modules/kiwix/tests/test_views.py

@@ -3,11 +3,11 @@
 Test module for Kiwix views.
 """
 
-import pathlib
-from unittest.mock import call, patch
+from unittest.mock import call, patch, MagicMock
 
 import pytest
 from django import urls
+from django.core.files.uploadedfile import SimpleUploadedFile
 from django.contrib.messages.storage.fallback import FallbackStorage
 from django.http.response import Http404
 
@@ -19,8 +19,6 @@ pytestmark = pytest.mark.urls('plinth.urls')
 
 ZIM_ID = 'bc4f8cdf-5626-2b13-3860-0033deddfbea'
 
-_data_dir = pathlib.Path(__file__).parent / 'data'
-
 
 @pytest.fixture(autouse=True, scope='module')
 def fixture_kiwix_urls():
@@ -43,7 +41,7 @@ def make_request(request, view, **kwargs):
 
 
 @pytest.fixture(autouse=True)
-def fiture_kiwix_patch():
+def fixture_kiwix_patch():
     """Patch kiwix methods."""
     with patch(
             'plinth.modules.kiwix.privileged.list_packages') as list_libraries:
@@ -57,34 +55,46 @@ def fiture_kiwix_patch():
         yield
 
 
-@patch('tempfile.TemporaryDirectory')
+@pytest.fixture()
+def file_path(tmp_path):
+    return str(tmp_path / 'FreedomBox.zim')
+
+
+def uploaded_file():
+    return SimpleUploadedFile('FreedomBox.zim', content=b'FreedomBox rocks!',
+                              content_type='application/octet-stream')
+
+
+@pytest.fixture()
+def add_package_request(rf, file_path):
+    """Patch add_package."""
+    post_data = {'kiwix-file': uploaded_file()}
+    request = rf.post('', data=post_data)
+    request.FILES['kiwix-file'].temporary_file_path = MagicMock(
+        return_value=file_path)
+    return request
+
+
 @patch('plinth.modules.kiwix.privileged.add_package')
-def test_add_package(add_package, temp_dir_class, rf, tmp_path):
+def test_add_package(add_package, file_path, add_package_request):
     """Test that adding content view works."""
-    temp_dir_class.return_value.__enter__.return_value = str(tmp_path)
-    with open(_data_dir / 'FreedomBox.zim', 'rb') as zim_file:
-        post_data = {'kiwix-file': zim_file}
-        request = rf.post('', data=post_data)
-        response, messages = make_request(request,
-                                          views.AddPackageView.as_view())
-        assert response.status_code == 302
-        assert response.url == urls.reverse('kiwix:index')
-        assert list(messages)[0].message == 'Content package added.'
-        add_package.assert_has_calls([call(f'{tmp_path}/FreedomBox.zim')])
+    response, messages = make_request(add_package_request,
+                                      views.AddPackageView.as_view())
+    assert response.status_code == 302
+    assert response.url == urls.reverse('kiwix:index')
+    assert list(messages)[0].message == 'Content package added.'
+    add_package.assert_has_calls([call('FreedomBox.zim', file_path)])
 
 
 @patch('plinth.modules.kiwix.privileged.add_package')
-def test_add_package_failed(add_package, rf):
+def test_add_package_failed(add_package, add_package_request):
     """Test that adding content package fails in case of an error."""
     add_package.side_effect = RuntimeError('TestError')
-    with open(_data_dir / 'FreedomBox.zim', 'rb') as zim_file:
-        post_data = {'kiwix-file': zim_file}
-        request = rf.post('', data=post_data)
-        response, messages = make_request(request,
-                                          views.AddPackageView.as_view())
-        assert response.status_code == 302
-        assert response.url == urls.reverse('kiwix:index')
-        assert list(messages)[0].message == 'Failed to add content package.'
+    response, messages = make_request(add_package_request,
+                                      views.AddPackageView.as_view())
+    assert response.status_code == 302
+    assert response.url == urls.reverse('kiwix:index')
+    assert list(messages)[0].message == 'Failed to add content package.'
 
 
 @patch('plinth.app.App.get')

plinth/modules/kiwix/views.py

@@ -4,7 +4,6 @@ Views for the Kiwix module.
 """
 
 import logging
-import tempfile
 
 from django.contrib import messages
 from django.contrib.messages.views import SuccessMessageMixin
@@ -20,6 +19,7 @@ from plinth import views
 from plinth.errors import PlinthError
 from plinth.modules import storage
 from plinth.modules.kiwix import privileged
+from plinth.views import messages_error
 
 from . import forms
 
@@ -68,20 +68,17 @@ class AddPackageView(SuccessMessageMixin, FormView):
 
     def form_valid(self, form):
         """Store the uploaded file."""
-        multipart_file = self.request.FILES['kiwix-file']
-
-        with tempfile.TemporaryDirectory() as temp_dir:
-            zim_file_name = temp_dir + '/' + multipart_file.name
-            with open(zim_file_name, 'wb+') as zim_file:
-                for chunk in multipart_file.chunks():
-                    zim_file.write(chunk)
-
-            try:
-                privileged.add_package(zim_file_name)
-            except Exception:
-                messages.error(self.request,
-                               _('Failed to add content package.'))
-                return redirect(reverse_lazy('kiwix:index'))
+        try:
+            uploaded_file = self.request.FILES['kiwix-file']
+            privileged.add_package(uploaded_file.name,
+                                   uploaded_file.temporary_file_path())
+        except FileExistsError:
+            messages.error(self.request, _('Content package already exists.'))
+            return redirect(reverse_lazy('kiwix:index'))
+        except Exception as exception:
+            messages_error(self.request, _('Failed to add content package.'),
+                           exception)
+            return redirect(reverse_lazy('kiwix:index'))
 
         return super().form_valid(form)