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)