From 6dd6f12f5a2a35659baa16cf5a7eda99f0cc62f0 Mon Sep 17 00:00:00 2001 From: Joseph Nuthalapati Date: Fri, 30 Aug 2024 16:17:44 +0530 Subject: [PATCH] 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 [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 Reviewed-by: Veiko Aasa --- plinth/modules/kiwix/privileged.py | 14 ++--- plinth/modules/kiwix/tests/test_privileged.py | 20 ++++-- plinth/modules/kiwix/tests/test_views.py | 62 +++++++++++-------- plinth/modules/kiwix/views.py | 27 ++++---- 4 files changed, 68 insertions(+), 55 deletions(-) diff --git a/plinth/modules/kiwix/privileged.py b/plinth/modules/kiwix/privileged.py index 43cabd5fd..cc7ae117f 100644 --- a/plinth/modules/kiwix/privileged.py +++ b/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): diff --git a/plinth/modules/kiwix/tests/test_privileged.py b/plinth/modules/kiwix/tests/test_privileged.py index 34b98707d..5342e5b31 100644 --- a/plinth/modules/kiwix/tests/test_privileged.py +++ b/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() diff --git a/plinth/modules/kiwix/tests/test_views.py b/plinth/modules/kiwix/tests/test_views.py index fc77cd22d..66ab72118 100644 --- a/plinth/modules/kiwix/tests/test_views.py +++ b/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') diff --git a/plinth/modules/kiwix/views.py b/plinth/modules/kiwix/views.py index 0f7a2dc40..a5d6503e5 100644 --- a/plinth/modules/kiwix/views.py +++ b/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)