From b1441511b6347266f348c2f4d4cb427b42766e4c Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Thu, 11 Apr 2024 19:28:41 +0200 Subject: [PATCH 01/16] Scan a file with clamdscan --- .../0005_alter_filevalidation_validation.py | 18 +++++++++++++ files/models.py | 25 ++++++++++++++++-- files/tests/files/Win.Test.EICAR_HDB-1.zip | 1 + files/tests/test_models.py | 26 ++++++++++++++++++- files/utils.py | 8 ++++++ 5 files changed, 75 insertions(+), 3 deletions(-) create mode 100644 files/migrations/0005_alter_filevalidation_validation.py create mode 100644 files/tests/files/Win.Test.EICAR_HDB-1.zip diff --git a/files/migrations/0005_alter_filevalidation_validation.py b/files/migrations/0005_alter_filevalidation_validation.py new file mode 100644 index 00000000..873b11bc --- /dev/null +++ b/files/migrations/0005_alter_filevalidation_validation.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.11 on 2024-04-11 17:13 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('files', '0004_alter_file_status'), + ] + + operations = [ + migrations.AlterField( + model_name='filevalidation', + name='validation', + field=models.JSONField(), + ), + ] diff --git a/files/models.py b/files/models.py index 246542cd..22ec4163 100644 --- a/files/models.py +++ b/files/models.py @@ -1,12 +1,14 @@ from pathlib import Path from typing import Dict, Any import logging +import os.path +from django.conf import settings from django.contrib.auth import get_user_model from django.db import models from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin -from files.utils import get_sha256, guess_mimetype_from_ext +from files.utils import get_sha256, guess_mimetype_from_ext, scan from constants.base import ( FILE_STATUS_CHOICES, FILE_TYPE_CHOICES, @@ -204,6 +206,25 @@ class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mode def get_submit_url(self) -> str: return self.extension.get_draft_url() + def scan(self) -> 'FileValidation': + """Run a scanner on the source file and save its output as a FileValidation record.""" + abs_path = os.path.join(settings.MEDIA_ROOT, self.source.path) + completed_process = scan(abs_path) + validation = { + 'args': completed_process.args, + 'stdout': completed_process.stdout.decode(), + 'stderr': completed_process.stderr.decode(), + 'returncode': completed_process.returncode, + } + file_validation, is_new = FileValidation.objects.get_or_create( + file=self, defaults={'validation': validation} + ) + file_validation.is_valid = completed_process.returncode == 0 + # FIXME: do we need `errors`/`warnings`/`notices` counters at all? + file_validation.errors = 1 if not file_validation.is_valid else 0 + file_validation.save() + return file_validation + class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): track_changes_to_fields = {'is_valid', 'errors', 'warnings', 'notices', 'validation'} @@ -213,4 +234,4 @@ class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): errors = models.IntegerField(default=0) warnings = models.IntegerField(default=0) notices = models.IntegerField(default=0) - validation = models.TextField() + validation = models.JSONField() diff --git a/files/tests/files/Win.Test.EICAR_HDB-1.zip b/files/tests/files/Win.Test.EICAR_HDB-1.zip new file mode 100644 index 00000000..a2463df6 --- /dev/null +++ b/files/tests/files/Win.Test.EICAR_HDB-1.zip @@ -0,0 +1 @@ +X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H* \ No newline at end of file diff --git a/files/tests/test_models.py b/files/tests/test_models.py index e8066179..6913cec2 100644 --- a/files/tests/test_models.py +++ b/files/tests/test_models.py @@ -1,7 +1,9 @@ import json +import shutil +import tempfile from django.contrib.auth import get_user_model -from django.test import TestCase +from django.test import TestCase, override_settings from common.admin import get_admin_change_path from common.log_entries import entries_for @@ -76,3 +78,25 @@ class FileTest(TestCase): response = self.client.get(path) self.assertEqual(response.status_code, 200, path) + + +@override_settings(MEDIA_ROOT='./files/tests/files') +class FileScanTest(TestCase): + def setUp(self): + super().setUp() + self.temp_directory = tempfile.mkdtemp() + + def tearDown(self): + super().tearDown() + shutil.rmtree(self.temp_directory) + + def test_scan(self): + # TODO: write the test files on the fly + file = FileFactory(source='Win.Test.EICAR_HDB-1.zip') + + file_validation = file.scan() + + self.assertEqual(file_validation.validation['returncode'], 1) + stdout_lines = file_validation.validation['stdout'].split('\n') + self.assertIn(f'{file.source.name}: Win.Test.EICAR_HDB-1 FOUND', stdout_lines[0]) + self.assertEqual(file_validation.validation['stderr'], '') diff --git a/files/utils.py b/files/utils.py index 3c4f413f..9a6e8ad3 100644 --- a/files/utils.py +++ b/files/utils.py @@ -4,6 +4,8 @@ import io import logging import mimetypes import os +import os.path +import subprocess import toml import typing import zipfile @@ -161,3 +163,9 @@ def guess_mimetype_from_content(file_obj) -> str: # This file might be read again by validation or other utilities file_obj.seek(0) return mimetype_from_bytes + + +def scan(abs_path: str) -> 'subprocess.CompletedProcess': + scan_args = ['clamdscan', '--fdpass', abs_path] + logger.info('Running %s', scan_args) + return subprocess.run(scan_args, capture_output=True) -- 2.30.2 From 9f7c917e23c8d7eb58e2711ab488f81d723c787e Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 12:27:21 +0200 Subject: [PATCH 02/16] Background tasks for the scan --- extensions/signals.py | 1 - extensions/tasks.py | 0 .../0005_alter_filevalidation_validation.py | 18 ------ ...idation_filevalidation_results_and_more.py | 35 ++++++++++ files/models.py | 28 +------- files/signals.py | 18 +++++- files/tasks.py | 26 ++++++++ files/tests/test_models.py | 64 ++++++++++++++++--- files/utils.py | 2 +- 9 files changed, 136 insertions(+), 56 deletions(-) delete mode 100644 extensions/tasks.py delete mode 100644 files/migrations/0005_alter_filevalidation_validation.py create mode 100644 files/migrations/0005_rename_validation_filevalidation_results_and_more.py create mode 100644 files/tasks.py diff --git a/extensions/signals.py b/extensions/signals.py index e6f2d682..780abdd9 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -5,7 +5,6 @@ from django.dispatch import receiver import django.dispatch import extensions.models -import extensions.tasks import files.models version_changed = django.dispatch.Signal() diff --git a/extensions/tasks.py b/extensions/tasks.py deleted file mode 100644 index e69de29b..00000000 diff --git a/files/migrations/0005_alter_filevalidation_validation.py b/files/migrations/0005_alter_filevalidation_validation.py deleted file mode 100644 index 873b11bc..00000000 --- a/files/migrations/0005_alter_filevalidation_validation.py +++ /dev/null @@ -1,18 +0,0 @@ -# Generated by Django 4.2.11 on 2024-04-11 17:13 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ('files', '0004_alter_file_status'), - ] - - operations = [ - migrations.AlterField( - model_name='filevalidation', - name='validation', - field=models.JSONField(), - ), - ] diff --git a/files/migrations/0005_rename_validation_filevalidation_results_and_more.py b/files/migrations/0005_rename_validation_filevalidation_results_and_more.py new file mode 100644 index 00000000..5297ecb7 --- /dev/null +++ b/files/migrations/0005_rename_validation_filevalidation_results_and_more.py @@ -0,0 +1,35 @@ +# Generated by Django 4.2.11 on 2024-04-12 09:05 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('files', '0004_alter_file_status'), + ] + + operations = [ + migrations.RenameField( + model_name='filevalidation', + old_name='validation', + new_name='results', + ), + migrations.AlterField( + model_name='filevalidation', + name='results', + field=models.JSONField(), + ), + migrations.RemoveField( + model_name='filevalidation', + name='errors', + ), + migrations.RemoveField( + model_name='filevalidation', + name='notices', + ), + migrations.RemoveField( + model_name='filevalidation', + name='warnings', + ), + ] diff --git a/files/models.py b/files/models.py index 22ec4163..124cf0ca 100644 --- a/files/models.py +++ b/files/models.py @@ -1,14 +1,12 @@ from pathlib import Path from typing import Dict, Any import logging -import os.path -from django.conf import settings from django.contrib.auth import get_user_model from django.db import models from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin -from files.utils import get_sha256, guess_mimetype_from_ext, scan +from files.utils import get_sha256, guess_mimetype_from_ext from constants.base import ( FILE_STATUS_CHOICES, FILE_TYPE_CHOICES, @@ -206,32 +204,10 @@ class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mode def get_submit_url(self) -> str: return self.extension.get_draft_url() - def scan(self) -> 'FileValidation': - """Run a scanner on the source file and save its output as a FileValidation record.""" - abs_path = os.path.join(settings.MEDIA_ROOT, self.source.path) - completed_process = scan(abs_path) - validation = { - 'args': completed_process.args, - 'stdout': completed_process.stdout.decode(), - 'stderr': completed_process.stderr.decode(), - 'returncode': completed_process.returncode, - } - file_validation, is_new = FileValidation.objects.get_or_create( - file=self, defaults={'validation': validation} - ) - file_validation.is_valid = completed_process.returncode == 0 - # FIXME: do we need `errors`/`warnings`/`notices` counters at all? - file_validation.errors = 1 if not file_validation.is_valid else 0 - file_validation.save() - return file_validation - class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): track_changes_to_fields = {'is_valid', 'errors', 'warnings', 'notices', 'validation'} file = models.OneToOneField(File, related_name='validation', on_delete=models.CASCADE) is_valid = models.BooleanField(default=False) - errors = models.IntegerField(default=0) - warnings = models.IntegerField(default=0) - notices = models.IntegerField(default=0) - validation = models.JSONField() + results = models.JSONField() diff --git a/files/signals.py b/files/signals.py index 239506c9..eecf3962 100644 --- a/files/signals.py +++ b/files/signals.py @@ -1,7 +1,12 @@ -from django.db.models.signals import pre_save +import logging + +from django.db.models.signals import pre_save, post_save from django.dispatch import receiver import files.models +import files.tasks + +logger = logging.getLogger(__name__) @receiver(pre_save, sender=files.models.File) @@ -9,3 +14,14 @@ def _record_changes(sender: object, instance: files.models.File, **kwargs: objec was_changed, old_state = instance.pre_save_record() instance.record_status_change(was_changed, old_state, **kwargs) + + +@receiver(post_save, sender=files.models.File) +def _scan_new_file( + sender: object, instance: files.models.File, created: bool, **kwargs: object +) -> None: + if not created: + return + + logger.info('Initiating a scan for file pk=%s', instance.pk) + files.tasks.scan(file_id=instance.pk, creator=instance) diff --git a/files/tasks.py b/files/tasks.py new file mode 100644 index 00000000..69115c55 --- /dev/null +++ b/files/tasks.py @@ -0,0 +1,26 @@ +import os.path + +from background_task import background +from django.conf import settings + +import files.models +import files.utils + + +@background() +def scan(file_id: int): + """Run a scan on a given file and save its output as a FileValidation record.""" + file = files.models.File.objects.get(pk=file_id) + abs_path = os.path.join(settings.MEDIA_ROOT, file.source.path) + completed_process = files.utils.run_clamdscan(abs_path) + scan_result = { + 'args': completed_process.args, + 'stdout': completed_process.stdout.decode(), + 'stderr': completed_process.stderr.decode(), + 'returncode': completed_process.returncode, + } + file_validation, is_new = files.models.FileValidation.objects.get_or_create( + file=file, defaults={'results': {completed_process.args[0]: scan_result}} + ) + file_validation.is_valid = completed_process.returncode == 0 + file_validation.save() diff --git a/files/tests/test_models.py b/files/tests/test_models.py index 6913cec2..a0ba4017 100644 --- a/files/tests/test_models.py +++ b/files/tests/test_models.py @@ -1,7 +1,11 @@ import json +import os import shutil import tempfile +import unittest +from background_task.models import Task +from django.conf import settings from django.contrib.auth import get_user_model from django.test import TestCase, override_settings @@ -9,6 +13,7 @@ from common.admin import get_admin_change_path from common.log_entries import entries_for from common.tests.factories.files import FileFactory from files.models import File +import files.tasks User = get_user_model() @@ -80,23 +85,64 @@ class FileTest(TestCase): self.assertEqual(response.status_code, 200, path) -@override_settings(MEDIA_ROOT='./files/tests/files') +@unittest.skipUnless(shutil.which('clamdscan'), 'requires clamdscan') +@override_settings(MEDIA_ROOT='/tmp/') class FileScanTest(TestCase): def setUp(self): super().setUp() - self.temp_directory = tempfile.mkdtemp() + self.temp_directory = tempfile.mkdtemp(prefix=settings.MEDIA_ROOT) def tearDown(self): super().tearDown() shutil.rmtree(self.temp_directory) - def test_scan(self): - # TODO: write the test files on the fly - file = FileFactory(source='Win.Test.EICAR_HDB-1.zip') + def test_scan_flags_found_invalid(self): + test_file_path = os.path.join(self.temp_directory, 'test_file.zip') + test_content = ( + b'X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*' # noqa: W605 + ) + with open(test_file_path, 'wb+') as test_file: + test_file.write(test_content) - file_validation = file.scan() + file = FileFactory(source=test_file_path) - self.assertEqual(file_validation.validation['returncode'], 1) - stdout_lines = file_validation.validation['stdout'].split('\n') + # A background task should have been created + task = Task.objects.created_by(creator=file).first() + self.assertIsNotNone(task) + self.assertEqual(task.task_name, 'files.tasks.scan') + self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') + + # Actually run the task as if by background runner + task_args, task_kwargs = task.params() + files.tasks.scan.task_function(*task_args, **task_kwargs) + + self.assertFalse(file.validation.is_valid) + result = file.validation.results['clamdscan'] + self.assertEqual(result['returncode'], 1) + stdout_lines = result['stdout'].split('\n') self.assertIn(f'{file.source.name}: Win.Test.EICAR_HDB-1 FOUND', stdout_lines[0]) - self.assertEqual(file_validation.validation['stderr'], '') + self.assertEqual(result['stderr'], '') + + def test_scan_flags_nothing_found_valid(self): + test_file_path = os.path.join(self.temp_directory, 'test_file.zip') + with open(test_file_path, 'wb+') as test_file: + test_file.write(b'some file') + + file = FileFactory(source=test_file_path) + + # A background task should have been created + task = Task.objects.created_by(creator=file).first() + self.assertIsNotNone(task) + self.assertEqual(task.task_name, 'files.tasks.scan') + self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') + + # Actually run the task as if by background runner + task_args, task_kwargs = task.params() + files.tasks.scan.task_function(*task_args, **task_kwargs) + + self.assertTrue(file.validation.is_valid) + result = file.validation.results['clamdscan'] + self.assertEqual(result['returncode'], 0) + stdout_lines = result['stdout'].split('\n') + self.assertIn(f'{file.source.name}: OK', stdout_lines[0]) + self.assertEqual(result['stderr'], '') diff --git a/files/utils.py b/files/utils.py index 9a6e8ad3..04c20f2a 100644 --- a/files/utils.py +++ b/files/utils.py @@ -165,7 +165,7 @@ def guess_mimetype_from_content(file_obj) -> str: return mimetype_from_bytes -def scan(abs_path: str) -> 'subprocess.CompletedProcess': +def run_clamdscan(abs_path: str) -> 'subprocess.CompletedProcess': scan_args = ['clamdscan', '--fdpass', abs_path] logger.info('Running %s', scan_args) return subprocess.run(scan_args, capture_output=True) -- 2.30.2 From da1522599eb28ed5cc127fa94b4bd64dabe3107d Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 12:30:13 +0200 Subject: [PATCH 03/16] Minor changes --- files/tests/files/Win.Test.EICAR_HDB-1.zip | 1 - files/tests/test_models.py | 72 +-------------------- files/tests/test_signals.py | 74 ++++++++++++++++++++++ 3 files changed, 75 insertions(+), 72 deletions(-) delete mode 100644 files/tests/files/Win.Test.EICAR_HDB-1.zip create mode 100644 files/tests/test_signals.py diff --git a/files/tests/files/Win.Test.EICAR_HDB-1.zip b/files/tests/files/Win.Test.EICAR_HDB-1.zip deleted file mode 100644 index a2463df6..00000000 --- a/files/tests/files/Win.Test.EICAR_HDB-1.zip +++ /dev/null @@ -1 +0,0 @@ -X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H* \ No newline at end of file diff --git a/files/tests/test_models.py b/files/tests/test_models.py index a0ba4017..e8066179 100644 --- a/files/tests/test_models.py +++ b/files/tests/test_models.py @@ -1,19 +1,12 @@ import json -import os -import shutil -import tempfile -import unittest -from background_task.models import Task -from django.conf import settings from django.contrib.auth import get_user_model -from django.test import TestCase, override_settings +from django.test import TestCase from common.admin import get_admin_change_path from common.log_entries import entries_for from common.tests.factories.files import FileFactory from files.models import File -import files.tasks User = get_user_model() @@ -83,66 +76,3 @@ class FileTest(TestCase): response = self.client.get(path) self.assertEqual(response.status_code, 200, path) - - -@unittest.skipUnless(shutil.which('clamdscan'), 'requires clamdscan') -@override_settings(MEDIA_ROOT='/tmp/') -class FileScanTest(TestCase): - def setUp(self): - super().setUp() - self.temp_directory = tempfile.mkdtemp(prefix=settings.MEDIA_ROOT) - - def tearDown(self): - super().tearDown() - shutil.rmtree(self.temp_directory) - - def test_scan_flags_found_invalid(self): - test_file_path = os.path.join(self.temp_directory, 'test_file.zip') - test_content = ( - b'X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*' # noqa: W605 - ) - with open(test_file_path, 'wb+') as test_file: - test_file.write(test_content) - - file = FileFactory(source=test_file_path) - - # A background task should have been created - task = Task.objects.created_by(creator=file).first() - self.assertIsNotNone(task) - self.assertEqual(task.task_name, 'files.tasks.scan') - self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') - - # Actually run the task as if by background runner - task_args, task_kwargs = task.params() - files.tasks.scan.task_function(*task_args, **task_kwargs) - - self.assertFalse(file.validation.is_valid) - result = file.validation.results['clamdscan'] - self.assertEqual(result['returncode'], 1) - stdout_lines = result['stdout'].split('\n') - self.assertIn(f'{file.source.name}: Win.Test.EICAR_HDB-1 FOUND', stdout_lines[0]) - self.assertEqual(result['stderr'], '') - - def test_scan_flags_nothing_found_valid(self): - test_file_path = os.path.join(self.temp_directory, 'test_file.zip') - with open(test_file_path, 'wb+') as test_file: - test_file.write(b'some file') - - file = FileFactory(source=test_file_path) - - # A background task should have been created - task = Task.objects.created_by(creator=file).first() - self.assertIsNotNone(task) - self.assertEqual(task.task_name, 'files.tasks.scan') - self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') - - # Actually run the task as if by background runner - task_args, task_kwargs = task.params() - files.tasks.scan.task_function(*task_args, **task_kwargs) - - self.assertTrue(file.validation.is_valid) - result = file.validation.results['clamdscan'] - self.assertEqual(result['returncode'], 0) - stdout_lines = result['stdout'].split('\n') - self.assertIn(f'{file.source.name}: OK', stdout_lines[0]) - self.assertEqual(result['stderr'], '') diff --git a/files/tests/test_signals.py b/files/tests/test_signals.py new file mode 100644 index 00000000..2bf1c63c --- /dev/null +++ b/files/tests/test_signals.py @@ -0,0 +1,74 @@ +import os +import shutil +import tempfile +import unittest + +from background_task.models import Task +from django.conf import settings +from django.test import TestCase, override_settings + +from common.tests.factories.files import FileFactory +import files.tasks + + +@unittest.skipUnless(shutil.which('clamdscan'), 'requires clamdscan') +@override_settings(MEDIA_ROOT='/tmp/') +class FileScanTest(TestCase): + def setUp(self): + super().setUp() + self.temp_directory = tempfile.mkdtemp(prefix=settings.MEDIA_ROOT) + + def tearDown(self): + super().tearDown() + shutil.rmtree(self.temp_directory) + + def test_scan_flags_found_invalid(self): + test_file_path = os.path.join(self.temp_directory, 'test_file.zip') + test_content = ( + b'X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*' # noqa: W605 + ) + with open(test_file_path, 'wb+') as test_file: + test_file.write(test_content) + + file = FileFactory(source=test_file_path) + + # A background task should have been created + task = Task.objects.created_by(creator=file).first() + self.assertIsNotNone(task) + self.assertEqual(task.task_name, 'files.tasks.scan') + self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') + + # Actually run the task as if by background runner + task_args, task_kwargs = task.params() + files.tasks.scan.task_function(*task_args, **task_kwargs) + + self.assertFalse(file.validation.is_valid) + result = file.validation.results['clamdscan'] + self.assertEqual(result['returncode'], 1) + stdout_lines = result['stdout'].split('\n') + self.assertIn(f'{file.source.name}: Win.Test.EICAR_HDB-1 FOUND', stdout_lines[0]) + self.assertEqual(result['stderr'], '') + + def test_scan_flags_nothing_found_valid(self): + test_file_path = os.path.join(self.temp_directory, 'test_file.zip') + with open(test_file_path, 'wb+') as test_file: + test_file.write(b'some file') + + file = FileFactory(source=test_file_path) + + # A background task should have been created + task = Task.objects.created_by(creator=file).first() + self.assertIsNotNone(task) + self.assertEqual(task.task_name, 'files.tasks.scan') + self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') + + # Actually run the task as if by background runner + task_args, task_kwargs = task.params() + files.tasks.scan.task_function(*task_args, **task_kwargs) + + self.assertTrue(file.validation.is_valid) + result = file.validation.results['clamdscan'] + self.assertEqual(result['returncode'], 0) + stdout_lines = result['stdout'].split('\n') + self.assertIn(f'{file.source.name}: OK', stdout_lines[0]) + self.assertEqual(result['stderr'], '') -- 2.30.2 From d04ce7b3adca84ace2dad0361404ad1f08411d0a Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 12:39:03 +0200 Subject: [PATCH 04/16] More logging --- files/tasks.py | 4 ++++ files/utils.py | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/files/tasks.py b/files/tasks.py index 69115c55..f4dbe2a8 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -1,3 +1,4 @@ +import logging import os.path from background_task import background @@ -6,6 +7,8 @@ from django.conf import settings import files.models import files.utils +logger = logging.getLogger(__name__) + @background() def scan(file_id: int): @@ -19,6 +22,7 @@ def scan(file_id: int): 'stderr': completed_process.stderr.decode(), 'returncode': completed_process.returncode, } + logger.info('File pk=%s scanned: exit code %s', file.pk, completed_process.returncode) file_validation, is_new = files.models.FileValidation.objects.get_or_create( file=file, defaults={'results': {completed_process.args[0]: scan_result}} ) diff --git a/files/utils.py b/files/utils.py index 04c20f2a..c0bb26ec 100644 --- a/files/utils.py +++ b/files/utils.py @@ -166,6 +166,9 @@ def guess_mimetype_from_content(file_obj) -> str: def run_clamdscan(abs_path: str) -> 'subprocess.CompletedProcess': + logger.info('Scanning file at path=%s', abs_path) scan_args = ['clamdscan', '--fdpass', abs_path] logger.info('Running %s', scan_args) - return subprocess.run(scan_args, capture_output=True) + completed_process = subprocess.run(scan_args, capture_output=True) + logger.info('File at path=%s scanned: exit code %s', abs_path, completed_process.returncode) + return completed_process -- 2.30.2 From bd1d6acc52921bead396a4bb1ba47e9f1508e15b Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 14:14:46 +0200 Subject: [PATCH 05/16] Admin action for scanning selected files --- files/admin.py | 64 +++++++++++++++++++++++++++++++++++++++++++++++- files/models.py | 2 +- files/signals.py | 8 ++++-- files/tasks.py | 3 ++- 4 files changed, 72 insertions(+), 5 deletions(-) diff --git a/files/admin.py b/files/admin.py index e6306157..62b632a0 100644 --- a/files/admin.py +++ b/files/admin.py @@ -1,6 +1,20 @@ from django.contrib import admin +import background_task.admin +import background_task.models -from .models import File +from .models import File, FileValidation +import files.signals + + +def scan_selected_files(self, request, queryset): + """Scan selected files.""" + for instance in queryset: + files.signals._initiate_scan(instance) + + +class FileValidationInlineAdmin(admin.TabularInline): + model = FileValidation + readonly_fields = ('date_created', 'date_modified', 'is_valid', 'results') @admin.register(File) @@ -77,3 +91,51 @@ class FileAdmin(admin.ModelAdmin): }, ), ) + + inlines = [FileValidationInlineAdmin] + actions = [scan_selected_files] + + +try: + admin.site.unregister(background_task.models.Task) + admin.site.unregister(background_task.models.CompletedTask) +except admin.site.NotRegistered: + pass + + +class TaskMixin: + """Modify a few properties of background tasks displayed in admin.""" + + def no_errors(self, obj): + """Replace background_task's "has_error". + + Make Django's red/green boolean icons less confusing + in the context of "there's an error during task run". + """ + return not bool(obj.last_error) + + no_errors.boolean = True + + +@admin.register(background_task.models.Task) +@admin.register(background_task.models.CompletedTask) +class TaskAdmin(background_task.admin.TaskAdmin, TaskMixin): + date_hierarchy = 'run_at' + list_display = [ + 'run_at', + 'task_name', + 'task_params', + 'attempts', + 'no_errors', + 'locked_by', + 'locked_by_pid_running', + ] + list_filter = ( + 'task_name', + 'run_at', + 'failed_at', + 'locked_at', + 'attempts', + 'creator_content_type', + ) + search_fields = ['task_name', 'task_params', 'last_error', 'verbose_name'] diff --git a/files/models.py b/files/models.py index 124cf0ca..2437a9a2 100644 --- a/files/models.py +++ b/files/models.py @@ -206,7 +206,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mode class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): - track_changes_to_fields = {'is_valid', 'errors', 'warnings', 'notices', 'validation'} + track_changes_to_fields = {'is_valid', 'results'} file = models.OneToOneField(File, related_name='validation', on_delete=models.CASCADE) is_valid = models.BooleanField(default=False) diff --git a/files/signals.py b/files/signals.py index eecf3962..580ad34d 100644 --- a/files/signals.py +++ b/files/signals.py @@ -16,6 +16,11 @@ def _record_changes(sender: object, instance: files.models.File, **kwargs: objec instance.record_status_change(was_changed, old_state, **kwargs) +def _initiate_scan(file: files.models.File) -> None: + logger.info('Initiating a scan for file pk=%s', file.pk) + files.tasks.scan(file_id=file.pk, creator=file, verbose_name=file.source.name) + + @receiver(post_save, sender=files.models.File) def _scan_new_file( sender: object, instance: files.models.File, created: bool, **kwargs: object @@ -23,5 +28,4 @@ def _scan_new_file( if not created: return - logger.info('Initiating a scan for file pk=%s', instance.pk) - files.tasks.scan(file_id=instance.pk, creator=instance) + _initiate_scan(instance) diff --git a/files/tasks.py b/files/tasks.py index f4dbe2a8..b3e0da68 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -2,6 +2,7 @@ import logging import os.path from background_task import background +from background_task.tasks import TaskSchedule from django.conf import settings import files.models @@ -10,7 +11,7 @@ import files.utils logger = logging.getLogger(__name__) -@background() +@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) def scan(file_id: int): """Run a scan on a given file and save its output as a FileValidation record.""" file = files.models.File.objects.get(pk=file_id) -- 2.30.2 From fc8100b9aebbf53e1122c2b2c73d8b0242206615 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 14:24:09 +0200 Subject: [PATCH 06/16] Playbooks: packages for clamav daemon and client Use ./ansible.sh -i environments/staging install.yaml --diff --tags=deps ./ansible.sh -i environments/production install.yaml --diff --tags=deps to install these on staging/production. --- playbooks/install.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/playbooks/install.yaml b/playbooks/install.yaml index 44840ccd..c625ae40 100644 --- a/playbooks/install.yaml +++ b/playbooks/install.yaml @@ -9,6 +9,8 @@ - name: Installing required packages ansible.builtin.apt: name={{ item }} state=present with_items: + - clamav-daemon + - clamdscan - git - libpq-dev - nginx-full -- 2.30.2 From 21b74c28afa1259e7333f246e1c766ea173b46d5 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 14:26:23 +0200 Subject: [PATCH 07/16] Wording --- files/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/files/tasks.py b/files/tasks.py index b3e0da68..c6de7ee9 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -13,7 +13,7 @@ logger = logging.getLogger(__name__) @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) def scan(file_id: int): - """Run a scan on a given file and save its output as a FileValidation record.""" + """Run a scan of a given file and save its output as a FileValidation record.""" file = files.models.File.objects.get(pk=file_id) abs_path = os.path.join(settings.MEDIA_ROOT, file.source.path) completed_process = files.utils.run_clamdscan(abs_path) -- 2.30.2 From b143091ccfe4ecec48c6f62e72a902188b964d19 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 15:29:19 +0200 Subject: [PATCH 08/16] Remove unused utility copy-pasta --- common/templatetags/common.py | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/common/templatetags/common.py b/common/templatetags/common.py index 6792b812..88a9040b 100644 --- a/common/templatetags/common.py +++ b/common/templatetags/common.py @@ -1,7 +1,6 @@ from urllib.parse import urljoin, urlparse import json import logging -import os from django.conf import settings from django.contrib.sites.shortcuts import get_current_site @@ -41,31 +40,6 @@ def absolute_url(context, path: str) -> str: return absolutify(path, request=request) -# A (temporary?) copy of this is in services/utils.py. See bug 1055654. -def user_media_path(what): - """Make it possible to override storage paths in settings. - - By default, all storage paths are in the MEDIA_ROOT. - - This is backwards compatible. - - """ - default = os.path.join(settings.MEDIA_ROOT, what) - key = f'{what.upper()}_PATH' - return getattr(settings, key, default) - - -# A (temporary?) copy of this is in services/utils.py. See bug 1055654. -def user_media_url(what): - """ - Generate default media url, and make possible to override it from - settings. - """ - default = f'{settings.MEDIA_URL}{what}/' - key = '{}_URL'.format(what.upper().replace('-', '_')) - return getattr(settings, key, default) - - class PaginationRenderer: def __init__(self, pager): self.pager = pager -- 2.30.2 From 57bc866b2f4efaf0317536920011c9fb92979ef7 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 17:53:01 +0200 Subject: [PATCH 09/16] Show scan-related alerts in various places --- extensions/templates/extensions/detail.html | 6 ++++++ files/admin.py | 20 +++++++++++++++--- ...idation_filevalidation_results_and_more.py | 5 +++++ files/models.py | 8 +++++-- files/tasks.py | 2 +- .../files/components/scan_details.html | 21 +++++++++++++++++++ .../files/components/scan_details_flag.html | 10 +++++++++ files/tests/test_signals.py | 4 ++-- .../components/review_list_item.html | 1 + 9 files changed, 69 insertions(+), 8 deletions(-) create mode 100644 files/templates/files/components/scan_details.html create mode 100644 files/templates/files/components/scan_details_flag.html diff --git a/extensions/templates/extensions/detail.html b/extensions/templates/extensions/detail.html index 912d9e70..7c0ed8e4 100644 --- a/extensions/templates/extensions/detail.html +++ b/extensions/templates/extensions/detail.html @@ -5,6 +5,12 @@ {% block page_title %}{{ extension.name }}{% endblock page_title %} {% block content %} + {% if extension.latest_version %} + {% with latest=extension.latest_version %} + {% include "files/components/scan_details.html" with file=latest.file %} + {% endwith %} + {% endif %} + {% has_maintainer extension as is_maintainer %} {% with latest=extension.latest_version %} diff --git a/files/admin.py b/files/admin.py index 62b632a0..bb956aa7 100644 --- a/files/admin.py +++ b/files/admin.py @@ -12,9 +12,17 @@ def scan_selected_files(self, request, queryset): files.signals._initiate_scan(instance) -class FileValidationInlineAdmin(admin.TabularInline): +class FileValidationInlineAdmin(admin.StackedInline): model = FileValidation - readonly_fields = ('date_created', 'date_modified', 'is_valid', 'results') + readonly_fields = ('date_created', 'date_modified', 'is_ok', 'results') + extra = 0 + + def _nope(self, request, obj): + return False + + has_add_permission = _nope + has_change_permission = _nope + has_delete_permission = _nope @admin.register(File) @@ -23,13 +31,14 @@ class FileAdmin(admin.ModelAdmin): save_on_top = True list_filter = ( + 'validation__is_ok', 'type', 'status', 'date_status_changed', 'date_approved', 'date_deleted', ) - list_display = ('original_name', 'extension', 'user', 'date_created', 'type', 'status') + list_display = ('original_name', 'extension', 'user', 'date_created', 'type', 'status', 'is_ok') list_select_related = ('version__extension', 'user') @@ -95,6 +104,11 @@ class FileAdmin(admin.ModelAdmin): inlines = [FileValidationInlineAdmin] actions = [scan_selected_files] + def is_ok(self, obj): + return obj.validation.is_ok if hasattr(obj, 'validation') else None + + is_ok.boolean = True + try: admin.site.unregister(background_task.models.Task) diff --git a/files/migrations/0005_rename_validation_filevalidation_results_and_more.py b/files/migrations/0005_rename_validation_filevalidation_results_and_more.py index 5297ecb7..21d01245 100644 --- a/files/migrations/0005_rename_validation_filevalidation_results_and_more.py +++ b/files/migrations/0005_rename_validation_filevalidation_results_and_more.py @@ -32,4 +32,9 @@ class Migration(migrations.Migration): model_name='filevalidation', name='warnings', ), + migrations.RenameField( + model_name='filevalidation', + old_name='is_valid', + new_name='is_ok', + ), ] diff --git a/files/models.py b/files/models.py index 2437a9a2..8957e8c8 100644 --- a/files/models.py +++ b/files/models.py @@ -204,10 +204,14 @@ class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mode def get_submit_url(self) -> str: return self.extension.get_draft_url() + @property + def is_ok(self): + return self.validation.is_ok if hasattr(self, 'validation') else None + class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): - track_changes_to_fields = {'is_valid', 'results'} + track_changes_to_fields = {'is_ok', 'results'} file = models.OneToOneField(File, related_name='validation', on_delete=models.CASCADE) - is_valid = models.BooleanField(default=False) + is_ok = models.BooleanField(default=False) results = models.JSONField() diff --git a/files/tasks.py b/files/tasks.py index c6de7ee9..d325436e 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -27,5 +27,5 @@ def scan(file_id: int): file_validation, is_new = files.models.FileValidation.objects.get_or_create( file=file, defaults={'results': {completed_process.args[0]: scan_result}} ) - file_validation.is_valid = completed_process.returncode == 0 + file_validation.is_ok = completed_process.returncode == 0 file_validation.save() diff --git a/files/templates/files/components/scan_details.html b/files/templates/files/components/scan_details.html new file mode 100644 index 00000000..9904ddd7 --- /dev/null +++ b/files/templates/files/components/scan_details.html @@ -0,0 +1,21 @@ +{% load common i18n %} +{# FIXME: we might want to rephrase is_moderator is terms of Django's (group) permissions #} +{% if perms.files.view_file or request.user.is_moderator %} + {% with file_validation=file.validation %} + {% if file_validation and not file_validation.is_ok %} +
+
+

⚠ Suspicious upload

+ {% blocktrans asvar alert_text %}Scan of the {{ file }} indicates malicious content.{% endblocktrans %} +

+ {{ alert_text }} + {% if perms.files.view_file %}{# Moderators don't necessarily have access to the admin #} + {% url 'admin:files_file_change' file.pk as admin_file_url %} + See details + {% endif %} +

+
+
+ {% endif %} + {% endwith %} +{% endif %} diff --git a/files/templates/files/components/scan_details_flag.html b/files/templates/files/components/scan_details_flag.html new file mode 100644 index 00000000..db5fc6bc --- /dev/null +++ b/files/templates/files/components/scan_details_flag.html @@ -0,0 +1,10 @@ +{% load common i18n %} +{# FIXME: we might want to rephrase is_moderator is terms of Django's (group) permissions #} +{% if perms.files.view_file or request.user.is_moderator %} + {% with file_validation=file.validation %} + {% if file_validation and not file_validation.is_ok %} + {% blocktrans asvar alert_text %}Scan of the {{ file }} indicates malicious content.{% endblocktrans %} + ⚠ + {% endif %} + {% endwith %} +{% endif %} diff --git a/files/tests/test_signals.py b/files/tests/test_signals.py index 2bf1c63c..5807e4d1 100644 --- a/files/tests/test_signals.py +++ b/files/tests/test_signals.py @@ -42,7 +42,7 @@ class FileScanTest(TestCase): task_args, task_kwargs = task.params() files.tasks.scan.task_function(*task_args, **task_kwargs) - self.assertFalse(file.validation.is_valid) + self.assertFalse(file.validation.is_ok) result = file.validation.results['clamdscan'] self.assertEqual(result['returncode'], 1) stdout_lines = result['stdout'].split('\n') @@ -66,7 +66,7 @@ class FileScanTest(TestCase): task_args, task_kwargs = task.params() files.tasks.scan.task_function(*task_args, **task_kwargs) - self.assertTrue(file.validation.is_valid) + self.assertTrue(file.validation.is_ok) result = file.validation.results['clamdscan'] self.assertEqual(result['returncode'], 0) stdout_lines = result['stdout'].split('\n') diff --git a/reviewers/templates/reviewers/components/review_list_item.html b/reviewers/templates/reviewers/components/review_list_item.html index 0bbe5b15..8de1cfb5 100644 --- a/reviewers/templates/reviewers/components/review_list_item.html +++ b/reviewers/templates/reviewers/components/review_list_item.html @@ -22,6 +22,7 @@ {{ extension.review_activity.all.last.date_created|naturaltime_compact }} {% endif %} + {% include "files/components/scan_details_flag.html" with file=extension.latest_version.file %} -- 2.30.2 From 3a1877cb7e2da0542064b9f85d7791b5b3b5251d Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 17:58:39 +0200 Subject: [PATCH 10/16] Remove unused property --- files/models.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/files/models.py b/files/models.py index 8957e8c8..1d6f2d02 100644 --- a/files/models.py +++ b/files/models.py @@ -204,10 +204,6 @@ class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mode def get_submit_url(self) -> str: return self.extension.get_draft_url() - @property - def is_ok(self): - return self.validation.is_ok if hasattr(self, 'validation') else None - class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): track_changes_to_fields = {'is_ok', 'results'} -- 2.30.2 From 1cdabee21450d9de748e852f458b3caf4333ffa4 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 18:05:44 +0200 Subject: [PATCH 11/16] Name task accordingly --- files/signals.py | 2 +- files/tasks.py | 4 ++-- files/tests/test_signals.py | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/files/signals.py b/files/signals.py index 580ad34d..13d0ce09 100644 --- a/files/signals.py +++ b/files/signals.py @@ -18,7 +18,7 @@ def _record_changes(sender: object, instance: files.models.File, **kwargs: objec def _initiate_scan(file: files.models.File) -> None: logger.info('Initiating a scan for file pk=%s', file.pk) - files.tasks.scan(file_id=file.pk, creator=file, verbose_name=file.source.name) + files.tasks.clamdscan(file_id=file.pk, creator=file, verbose_name=file.source.name) @receiver(post_save, sender=files.models.File) diff --git a/files/tasks.py b/files/tasks.py index d325436e..4b942a1d 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -12,7 +12,7 @@ logger = logging.getLogger(__name__) @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) -def scan(file_id: int): +def clamdscan(file_id: int): """Run a scan of a given file and save its output as a FileValidation record.""" file = files.models.File.objects.get(pk=file_id) abs_path = os.path.join(settings.MEDIA_ROOT, file.source.path) @@ -25,7 +25,7 @@ def scan(file_id: int): } logger.info('File pk=%s scanned: exit code %s', file.pk, completed_process.returncode) file_validation, is_new = files.models.FileValidation.objects.get_or_create( - file=file, defaults={'results': {completed_process.args[0]: scan_result}} + file=file, defaults={'results': {'clamdscan': scan_result}} ) file_validation.is_ok = completed_process.returncode == 0 file_validation.save() diff --git a/files/tests/test_signals.py b/files/tests/test_signals.py index 5807e4d1..8619ad2f 100644 --- a/files/tests/test_signals.py +++ b/files/tests/test_signals.py @@ -35,12 +35,12 @@ class FileScanTest(TestCase): # A background task should have been created task = Task.objects.created_by(creator=file).first() self.assertIsNotNone(task) - self.assertEqual(task.task_name, 'files.tasks.scan') + self.assertEqual(task.task_name, 'files.tasks.clamdscan') self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') # Actually run the task as if by background runner task_args, task_kwargs = task.params() - files.tasks.scan.task_function(*task_args, **task_kwargs) + files.tasks.clamdscan.task_function(*task_args, **task_kwargs) self.assertFalse(file.validation.is_ok) result = file.validation.results['clamdscan'] @@ -59,12 +59,12 @@ class FileScanTest(TestCase): # A background task should have been created task = Task.objects.created_by(creator=file).first() self.assertIsNotNone(task) - self.assertEqual(task.task_name, 'files.tasks.scan') + self.assertEqual(task.task_name, 'files.tasks.clamdscan') self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') # Actually run the task as if by background runner task_args, task_kwargs = task.params() - files.tasks.scan.task_function(*task_args, **task_kwargs) + files.tasks.clamdscan.task_function(*task_args, **task_kwargs) self.assertTrue(file.validation.is_ok) result = file.validation.results['clamdscan'] -- 2.30.2 From a110c8532a37c9f1ef48ab021258da2e009de0ab Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 18:14:55 +0200 Subject: [PATCH 12/16] Name things according to their meaning --- files/admin.py | 2 +- files/signals.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/files/admin.py b/files/admin.py index bb956aa7..8c8fa1bc 100644 --- a/files/admin.py +++ b/files/admin.py @@ -9,7 +9,7 @@ import files.signals def scan_selected_files(self, request, queryset): """Scan selected files.""" for instance in queryset: - files.signals._initiate_scan(instance) + files.signals.schedule_scan(instance) class FileValidationInlineAdmin(admin.StackedInline): diff --git a/files/signals.py b/files/signals.py index 13d0ce09..3388efad 100644 --- a/files/signals.py +++ b/files/signals.py @@ -16,8 +16,9 @@ def _record_changes(sender: object, instance: files.models.File, **kwargs: objec instance.record_status_change(was_changed, old_state, **kwargs) -def _initiate_scan(file: files.models.File) -> None: - logger.info('Initiating a scan for file pk=%s', file.pk) +def schedule_scan(file: files.models.File) -> None: + """Schedule a scan of a given file.""" + logger.info('Scheduling a scan for file pk=%s', file.pk) files.tasks.clamdscan(file_id=file.pk, creator=file, verbose_name=file.source.name) @@ -28,4 +29,4 @@ def _scan_new_file( if not created: return - _initiate_scan(instance) + schedule_scan(instance) -- 2.30.2 From 34982b3f16bede1aedb070278419951ad93b164b Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 18:27:29 +0200 Subject: [PATCH 13/16] Minor changes --- files/tasks.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/files/tasks.py b/files/tasks.py index 4b942a1d..107e5adb 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -17,15 +17,20 @@ def clamdscan(file_id: int): file = files.models.File.objects.get(pk=file_id) abs_path = os.path.join(settings.MEDIA_ROOT, file.source.path) completed_process = files.utils.run_clamdscan(abs_path) - scan_result = { - 'args': completed_process.args, - 'stdout': completed_process.stdout.decode(), - 'stderr': completed_process.stderr.decode(), - 'returncode': completed_process.returncode, - } logger.info('File pk=%s scanned: exit code %s', file.pk, completed_process.returncode) + results = { + 'clamdscan': { + 'args': completed_process.args, + 'stdout': completed_process.stdout.decode(), + 'stderr': completed_process.stderr.decode(), + 'returncode': completed_process.returncode, + } + } + is_ok = completed_process.returncode == 0 file_validation, is_new = files.models.FileValidation.objects.get_or_create( - file=file, defaults={'results': {'clamdscan': scan_result}} + file=file, defaults={'results': results, 'is_ok': is_ok} ) - file_validation.is_ok = completed_process.returncode == 0 - file_validation.save() + if not is_new: + file_validation.results = results + file_validation.is_ok = is_ok + file_validation.save(update_fields={'results', 'is_ok'}) -- 2.30.2 From 8aa25be6e62f177ea2220042ecf0388d179ac248 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 18:52:04 +0200 Subject: [PATCH 14/16] Make sure scan result is actually save when record exists already --- files/tasks.py | 6 +++--- files/tests/test_signals.py | 38 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/files/tasks.py b/files/tasks.py index 107e5adb..8468d384 100644 --- a/files/tasks.py +++ b/files/tasks.py @@ -18,7 +18,7 @@ def clamdscan(file_id: int): abs_path = os.path.join(settings.MEDIA_ROOT, file.source.path) completed_process = files.utils.run_clamdscan(abs_path) logger.info('File pk=%s scanned: exit code %s', file.pk, completed_process.returncode) - results = { + scan_result = { 'clamdscan': { 'args': completed_process.args, 'stdout': completed_process.stdout.decode(), @@ -28,9 +28,9 @@ def clamdscan(file_id: int): } is_ok = completed_process.returncode == 0 file_validation, is_new = files.models.FileValidation.objects.get_or_create( - file=file, defaults={'results': results, 'is_ok': is_ok} + file=file, defaults={'results': scan_result, 'is_ok': is_ok} ) if not is_new: - file_validation.results = results + file_validation.results = scan_result file_validation.is_ok = is_ok file_validation.save(update_fields={'results', 'is_ok'}) diff --git a/files/tests/test_signals.py b/files/tests/test_signals.py index 8619ad2f..a2d73122 100644 --- a/files/tests/test_signals.py +++ b/files/tests/test_signals.py @@ -8,6 +8,7 @@ from django.conf import settings from django.test import TestCase, override_settings from common.tests.factories.files import FileFactory +import files.models import files.tasks @@ -31,6 +32,39 @@ class FileScanTest(TestCase): test_file.write(test_content) file = FileFactory(source=test_file_path) + self.assertFalse(hasattr(file, 'validation')) + + # A background task should have been created + task = Task.objects.created_by(creator=file).first() + self.assertIsNotNone(task) + self.assertEqual(task.task_name, 'files.tasks.clamdscan') + self.assertEqual(task.task_params, f'[[], {{"file_id": {file.pk}}}]') + + # Actually run the task as if by background runner + task_args, task_kwargs = task.params() + files.tasks.clamdscan.task_function(*task_args, **task_kwargs) + + file.refresh_from_db() + self.assertFalse(file.validation.is_ok) + result = file.validation.results['clamdscan'] + self.assertEqual(result['returncode'], 1) + stdout_lines = result['stdout'].split('\n') + self.assertIn(f'{file.source.name}: Win.Test.EICAR_HDB-1 FOUND', stdout_lines[0]) + self.assertEqual(result['stderr'], '') + + def test_scan_flags_found_invalid_updates_existing_validation(self): + test_file_path = os.path.join(self.temp_directory, 'test_file.zip') + test_content = ( + b'X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*' # noqa: W605 + ) + with open(test_file_path, 'wb+') as test_file: + test_file.write(test_content) + + file = FileFactory(source=test_file_path) + # Make sure validation record exists before scanner runs + existing_validation = files.models.FileValidation(file=file, results={}) + existing_validation.save() + self.assertTrue(hasattr(file, 'validation')) # A background task should have been created task = Task.objects.created_by(creator=file).first() @@ -43,11 +77,13 @@ class FileScanTest(TestCase): files.tasks.clamdscan.task_function(*task_args, **task_kwargs) self.assertFalse(file.validation.is_ok) + file.validation.refresh_from_db() result = file.validation.results['clamdscan'] self.assertEqual(result['returncode'], 1) stdout_lines = result['stdout'].split('\n') self.assertIn(f'{file.source.name}: Win.Test.EICAR_HDB-1 FOUND', stdout_lines[0]) self.assertEqual(result['stderr'], '') + self.assertEqual(existing_validation.pk, file.validation.pk) def test_scan_flags_nothing_found_valid(self): test_file_path = os.path.join(self.temp_directory, 'test_file.zip') @@ -55,6 +91,7 @@ class FileScanTest(TestCase): test_file.write(b'some file') file = FileFactory(source=test_file_path) + self.assertFalse(hasattr(file, 'validation')) # A background task should have been created task = Task.objects.created_by(creator=file).first() @@ -66,6 +103,7 @@ class FileScanTest(TestCase): task_args, task_kwargs = task.params() files.tasks.clamdscan.task_function(*task_args, **task_kwargs) + file.refresh_from_db() self.assertTrue(file.validation.is_ok) result = file.validation.results['clamdscan'] self.assertEqual(result['returncode'], 0) -- 2.30.2 From 2288f80227129c62c8afaf072bd3c1836f568361 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 19:01:28 +0200 Subject: [PATCH 15/16] Missing translation tag --- files/templates/files/components/scan_details.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/files/templates/files/components/scan_details.html b/files/templates/files/components/scan_details.html index 9904ddd7..4a38ef4f 100644 --- a/files/templates/files/components/scan_details.html +++ b/files/templates/files/components/scan_details.html @@ -5,13 +5,13 @@ {% if file_validation and not file_validation.is_ok %}
-

⚠ Suspicious upload

+

⚠ {% trans "Suspicious upload" %}

{% blocktrans asvar alert_text %}Scan of the {{ file }} indicates malicious content.{% endblocktrans %}

{{ alert_text }} {% if perms.files.view_file %}{# Moderators don't necessarily have access to the admin #} {% url 'admin:files_file_change' file.pk as admin_file_url %} - See details + {% trans "See details" %} {% endif %}

-- 2.30.2 From b7a32344c60999ab16600f7731dc24d6df763373 Mon Sep 17 00:00:00 2001 From: Anna Sirota Date: Fri, 12 Apr 2024 19:04:08 +0200 Subject: [PATCH 16/16] Typos --- files/templates/files/components/scan_details.html | 2 +- files/templates/files/components/scan_details_flag.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/files/templates/files/components/scan_details.html b/files/templates/files/components/scan_details.html index 4a38ef4f..4a83d374 100644 --- a/files/templates/files/components/scan_details.html +++ b/files/templates/files/components/scan_details.html @@ -1,5 +1,5 @@ {% load common i18n %} -{# FIXME: we might want to rephrase is_moderator is terms of Django's (group) permissions #} +{# FIXME: we might want to rephrase is_moderator in terms of Django's (group) permissions #} {% if perms.files.view_file or request.user.is_moderator %} {% with file_validation=file.validation %} {% if file_validation and not file_validation.is_ok %} diff --git a/files/templates/files/components/scan_details_flag.html b/files/templates/files/components/scan_details_flag.html index db5fc6bc..7b896fce 100644 --- a/files/templates/files/components/scan_details_flag.html +++ b/files/templates/files/components/scan_details_flag.html @@ -1,5 +1,5 @@ {% load common i18n %} -{# FIXME: we might want to rephrase is_moderator is terms of Django's (group) permissions #} +{# FIXME: we might want to rephrase is_moderator in terms of Django's (group) permissions #} {% if perms.files.view_file or request.user.is_moderator %} {% with file_validation=file.validation %} {% if file_validation and not file_validation.is_ok %} -- 2.30.2