From e7f9633fc2c9b77119e73ddb8231432b84e9f25b Mon Sep 17 00:00:00 2001 From: John Willis Date: Mon, 13 Sep 2021 15:36:16 -0400 Subject: [PATCH 01/12] Added ClamAVFileScan model to track all file scans performed by ClamAV, added new security app to house auditing models --- tdrs-backend/tdpservice/core/admin.py | 1 + .../tdpservice/data_files/serializers.py | 29 ++++++-- .../tdpservice/data_files/validators.py | 12 +--- tdrs-backend/tdpservice/security/__init__.py | 0 tdrs-backend/tdpservice/security/admin.py | 19 ++++++ tdrs-backend/tdpservice/security/apps.py | 7 ++ .../tdpservice/{ => security}/clients.py | 26 +++++-- .../security/migrations/0001_initial.py | 29 ++++++++ .../security/migrations/__init__.py | 0 tdrs-backend/tdpservice/security/models.py | 68 +++++++++++++++++++ .../tdpservice/security/test/__init__.py | 0 .../{ => security}/test/test_clamav.py | 2 +- tdrs-backend/tdpservice/settings/common.py | 1 + 13 files changed, 171 insertions(+), 23 deletions(-) create mode 100644 tdrs-backend/tdpservice/security/__init__.py create mode 100644 tdrs-backend/tdpservice/security/admin.py create mode 100644 tdrs-backend/tdpservice/security/apps.py rename tdrs-backend/tdpservice/{ => security}/clients.py (76%) create mode 100644 tdrs-backend/tdpservice/security/migrations/0001_initial.py create mode 100644 tdrs-backend/tdpservice/security/migrations/__init__.py create mode 100644 tdrs-backend/tdpservice/security/models.py create mode 100644 tdrs-backend/tdpservice/security/test/__init__.py rename tdrs-backend/tdpservice/{ => security}/test/test_clamav.py (96%) diff --git a/tdrs-backend/tdpservice/core/admin.py b/tdrs-backend/tdpservice/core/admin.py index 9902c9d0a6..83929df67a 100644 --- a/tdrs-backend/tdpservice/core/admin.py +++ b/tdrs-backend/tdpservice/core/admin.py @@ -31,6 +31,7 @@ class LogEntryAdmin(admin.ModelAdmin): 'content_type', 'object_link', 'action_flag', + 'change_message', ] def object_link(self, obj): diff --git a/tdrs-backend/tdpservice/data_files/serializers.py b/tdrs-backend/tdpservice/data_files/serializers.py index 647fec1a15..c334fe3f0e 100644 --- a/tdrs-backend/tdpservice/data_files/serializers.py +++ b/tdrs-backend/tdpservice/data_files/serializers.py @@ -4,7 +4,11 @@ from tdpservice.data_files.errors import ImmutabilityError from tdpservice.data_files.models import DataFile -from tdpservice.data_files.validators import validate_data_file +from tdpservice.data_files.validators import ( + validate_file_extension, + validate_file_infection, +) +from tdpservice.security.models import ClamAVFileScan from tdpservice.stts.models import STT from tdpservice.users.models import User @@ -12,10 +16,7 @@ class DataFileSerializer(serializers.ModelSerializer): """Serializer for Data files.""" - file = serializers.FileField( - write_only=True, - validators=[validate_data_file] - ) + file = serializers.FileField(write_only=True) stt = serializers.PrimaryKeyRelatedField(queryset=STT.objects.all()) user = serializers.PrimaryKeyRelatedField(queryset=User.objects.all()) @@ -39,8 +40,24 @@ class Meta: def create(self, validated_data): """Create a new entry with a new version number.""" - return DataFile.create_new_version(validated_data) + data_file = DataFile.create_new_version(validated_data) + av_scan = ClamAVFileScan.objects.filter( + file_name=data_file.original_filename, + uploaded_by=data_file.user + ).last() + if av_scan is not None: + av_scan.data_file = data_file + av_scan.save() + + return data_file def update(self, instance, validated_data): """Throw an error if a user tries to update a data_file.""" raise ImmutabilityError(instance, validated_data) + + def validate(self, attrs): + """Perform all validation steps on a given file.""" + file, user = attrs['file'], attrs['user'] + validate_file_extension(file.name) + validate_file_infection(file, user) + return attrs diff --git a/tdrs-backend/tdpservice/data_files/validators.py b/tdrs-backend/tdpservice/data_files/validators.py index 07deb87550..bfdba82c03 100644 --- a/tdrs-backend/tdpservice/data_files/validators.py +++ b/tdrs-backend/tdpservice/data_files/validators.py @@ -5,7 +5,7 @@ from django.core.exceptions import ValidationError from inflection import pluralize -from tdpservice.clients import ClamAVClient +from tdpservice.security.clients import ClamAVClient logger = logging.getLogger(__name__) @@ -46,11 +46,11 @@ def validate_file_extension(file_name: str): raise ValidationError(msg) -def validate_file_infection(file): +def validate_file_infection(file, uploaded_by): """Validate file is not infected by scanning with ClamAV.""" av_client = ClamAVClient() try: - is_file_clean = av_client.scan_file(file, file.name) + is_file_clean = av_client.scan_file(file, file.name, uploaded_by) except ClamAVClient.ServiceUnavailable: raise ValidationError( 'Unable to complete security inspection, please try again or ' @@ -61,9 +61,3 @@ def validate_file_infection(file): raise ValidationError( 'Rejected: uploaded file did not pass security inspection' ) - - -def validate_data_file(file): - """Perform all validation steps on a given file.""" - validate_file_extension(file.name) - validate_file_infection(file) diff --git a/tdrs-backend/tdpservice/security/__init__.py b/tdrs-backend/tdpservice/security/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tdrs-backend/tdpservice/security/admin.py b/tdrs-backend/tdpservice/security/admin.py new file mode 100644 index 0000000000..013b388766 --- /dev/null +++ b/tdrs-backend/tdpservice/security/admin.py @@ -0,0 +1,19 @@ +from django.contrib import admin + +from tdpservice.security.models import ClamAVFileScan + + +@admin.register(ClamAVFileScan) +class ClamAVFileScanAdmin(admin.ModelAdmin): + + list_display = [ + 'file_name', + 'file_shasum', + 'result', + 'uploaded_by', + ] + + list_filter = [ + 'result', + 'uploaded_by', + ] diff --git a/tdrs-backend/tdpservice/security/apps.py b/tdrs-backend/tdpservice/security/apps.py new file mode 100644 index 0000000000..cc49655d2f --- /dev/null +++ b/tdrs-backend/tdpservice/security/apps.py @@ -0,0 +1,7 @@ +from django.apps import AppConfig + + +class SecurityConfig(AppConfig): + default_auto_field = 'django.db.models.BigAutoField' + name = 'tdpservice.security' + verbose_name = 'Security' diff --git a/tdrs-backend/tdpservice/clients.py b/tdrs-backend/tdpservice/security/clients.py similarity index 76% rename from tdrs-backend/tdpservice/clients.py rename to tdrs-backend/tdpservice/security/clients.py index 5b96d06dca..433ff64c41 100644 --- a/tdrs-backend/tdpservice/clients.py +++ b/tdrs-backend/tdpservice/security/clients.py @@ -7,6 +7,8 @@ from django.conf import settings +from tdpservice.security.models import ClamAVFileScan + logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) @@ -44,15 +46,19 @@ def init_session(self): session.mount(self.endpoint_url, HTTPAdapter(max_retries=retries)) return session - def scan_file(self, file, file_name) -> bool: + def scan_file(self, file, file_name, uploaded_by) -> bool: """Scan a file for virus infections. :param file: The file or file-like object that should be scanned :param file_name: The name of the target file (str). + :param uploaded_by: + The User that uploaded the given file. :returns is_file_clean: A boolean indicating whether or not the file passed the ClamAV scan + :returns msg: + A string containing details about the scan result. """ logger.debug(f'Initiating virus scan for file: {file_name}') try: @@ -67,16 +73,22 @@ def scan_file(self, file, file_name) -> bool: raise self.ServiceUnavailable() if scan_response.status_code in self.SCAN_CODES['CLEAN']: - logger.debug( - f'File scan marked as CLEAN for file: {file_name}' + msg = f'File scan marked as CLEAN for file: {file_name}' + logger.debug(msg) + ClamAVFileScan.objects.log_result( + file, + file_name, + msg, + ClamAVFileScan.Result.CLEAN, + uploaded_by ) return True if scan_response.status_code in self.SCAN_CODES['INFECTED']: - logger.debug( - f'File scan marked as INFECTED for file: {file_name}' - ) + msg = f'File scan marked as INFECTED for file: {file_name}' + logger.debug(msg) return False - logger.debug(f'Unable to scan file with name: {file_name}') + msg = f'Unable to scan file with name: {file_name}' + logger.debug(msg) return False diff --git a/tdrs-backend/tdpservice/security/migrations/0001_initial.py b/tdrs-backend/tdpservice/security/migrations/0001_initial.py new file mode 100644 index 0000000000..14ebda266e --- /dev/null +++ b/tdrs-backend/tdpservice/security/migrations/0001_initial.py @@ -0,0 +1,29 @@ +# Generated by Django 3.2.5 on 2021-09-13 19:05 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ('data_files', '0009_update_log_entry_content_type'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.CreateModel( + name='ClamAVFileScan', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('file_name', models.TextField()), + ('file_shasum', models.TextField(help_text='The SHA256 checksum of the uploaded file')), + ('result', models.CharField(choices=[('CLEAN', 'Clean'), ('INFECTED', 'Infected'), ('ERROR', 'Error')], help_text='Scan result for uploaded file', max_length=12)), + ('data_file', models.ForeignKey(blank=True, help_text='The resulting DataFile object if this scan was clean', null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='av_scans', to='data_files.datafile')), + ('uploaded_by', models.ForeignKey(help_text='The user that uploaded the scanned file', null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='av_scans', to=settings.AUTH_USER_MODEL)), + ], + ), + ] diff --git a/tdrs-backend/tdpservice/security/migrations/__init__.py b/tdrs-backend/tdpservice/security/migrations/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tdrs-backend/tdpservice/security/models.py b/tdrs-backend/tdpservice/security/models.py new file mode 100644 index 0000000000..be1edc0480 --- /dev/null +++ b/tdrs-backend/tdpservice/security/models.py @@ -0,0 +1,68 @@ +from hashlib import sha256 + +from django.contrib.admin.models import ContentType, LogEntry, ADDITION +from django.db import models + +from tdpservice.data_files.models import DataFile +from tdpservice.users.models import User + + +class ClamAVFileScanManager(models.Manager): + """Extends object manager functionality with common operations.""" + + def log_result(self, file, file_name, msg, result, uploaded_by): + """TODO.""" + scan_result = self.model.objects.create( + file_name=file_name, + file_shasum=sha256(file.read()).hexdigest(), + result=result, + uploaded_by=uploaded_by + ) + content_type = ContentType.objects.get_for_model(ClamAVFileScan) + LogEntry.objects.log_action( + user_id=uploaded_by.pk, + content_type_id=content_type.pk, + object_id=scan_result.pk, + object_repr=str(scan_result), + action_flag=ADDITION, + change_message=msg + ) + + return scan_result + + +class ClamAVFileScan(models.Model): + """Represents a ClamAV virus scan performed for an uploaded file.""" + + class Result(models.TextChoices): + CLEAN = 'CLEAN' + INFECTED = 'INFECTED' + ERROR = 'ERROR' + + file_name = models.TextField() + file_shasum = models.TextField( + help_text='The SHA256 checksum of the uploaded file' + ) + result = models.CharField( + choices=Result.choices, + help_text='Scan result for uploaded file', + max_length=12 + ) + uploaded_by = models.ForeignKey( + User, + help_text='The user that uploaded the scanned file', + null=True, + on_delete=models.SET_NULL, + related_name='av_scans' + ) + + data_file = models.ForeignKey( + DataFile, + blank=True, + help_text='The resulting DataFile object if this scan was clean', + null=True, + on_delete=models.SET_NULL, + related_name='av_scans' + ) + + objects = ClamAVFileScanManager() diff --git a/tdrs-backend/tdpservice/security/test/__init__.py b/tdrs-backend/tdpservice/security/test/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tdrs-backend/tdpservice/test/test_clamav.py b/tdrs-backend/tdpservice/security/test/test_clamav.py similarity index 96% rename from tdrs-backend/tdpservice/test/test_clamav.py rename to tdrs-backend/tdpservice/security/test/test_clamav.py index b2e427094b..10ea2a234a 100644 --- a/tdrs-backend/tdpservice/test/test_clamav.py +++ b/tdrs-backend/tdpservice/security/test/test_clamav.py @@ -2,7 +2,7 @@ from django.conf import settings import pytest -from tdpservice.clients import ClamAVClient +from tdpservice.security.clients import ClamAVClient @pytest.fixture diff --git a/tdrs-backend/tdpservice/settings/common.py b/tdrs-backend/tdpservice/settings/common.py index a127f167b3..9ee09ae2ff 100755 --- a/tdrs-backend/tdpservice/settings/common.py +++ b/tdrs-backend/tdpservice/settings/common.py @@ -53,6 +53,7 @@ class Common(Configuration): "tdpservice.users", "tdpservice.stts", "tdpservice.data_files", + "tdpservice.security", ) # https://docs.djangoproject.com/en/2.0/topics/http/middleware/ From 647de7ddc14dc8f87f59649877f2139bfffe8c25 Mon Sep 17 00:00:00 2001 From: John Willis Date: Mon, 13 Sep 2021 17:12:18 -0400 Subject: [PATCH 02/12] Updated ClamAVClient to log scan results for infected and error states, added permissions for ClamAVFileScan model --- tdrs-backend/tdpservice/security/admin.py | 1 + tdrs-backend/tdpservice/security/clients.py | 43 +++++++++------- .../security/migrations/0001_initial.py | 3 +- tdrs-backend/tdpservice/security/models.py | 1 + .../migrations/0018_ofa_admin_permissions.py | 19 +------ .../0020_ofa_system_admin_permissions.py | 2 - .../0022_add_clam_av_file_scan_perms.py | 49 +++++++++++++++++++ tdrs-backend/tdpservice/users/permissions.py | 18 +++++++ 8 files changed, 98 insertions(+), 38 deletions(-) create mode 100644 tdrs-backend/tdpservice/users/migrations/0022_add_clam_av_file_scan_perms.py diff --git a/tdrs-backend/tdpservice/security/admin.py b/tdrs-backend/tdpservice/security/admin.py index 013b388766..fbb359d10d 100644 --- a/tdrs-backend/tdpservice/security/admin.py +++ b/tdrs-backend/tdpservice/security/admin.py @@ -7,6 +7,7 @@ class ClamAVFileScanAdmin(admin.ModelAdmin): list_display = [ + 'scanned_at', 'file_name', 'file_shasum', 'result', diff --git a/tdrs-backend/tdpservice/security/clients.py b/tdrs-backend/tdpservice/security/clients.py index 433ff64c41..507edd7fcd 100644 --- a/tdrs-backend/tdpservice/security/clients.py +++ b/tdrs-backend/tdpservice/security/clients.py @@ -6,8 +6,10 @@ import logging from django.conf import settings +from django.core.files.base import File from tdpservice.security.models import ClamAVFileScan +from tdpservice.users.models import User logger = logging.getLogger(__name__) logger.setLevel(logging.DEBUG) @@ -46,7 +48,12 @@ def init_session(self): session.mount(self.endpoint_url, HTTPAdapter(max_retries=retries)) return session - def scan_file(self, file, file_name, uploaded_by) -> bool: + def scan_file( + self, + file: File, + file_name: str, + uploaded_by: User + ) -> bool: """Scan a file for virus infections. :param file: @@ -57,8 +64,7 @@ def scan_file(self, file, file_name, uploaded_by) -> bool: The User that uploaded the given file. :returns is_file_clean: A boolean indicating whether or not the file passed the ClamAV scan - :returns msg: - A string containing details about the scan result. + :raises ClamAVClient.ServiceUnavailable: """ logger.debug(f'Initiating virus scan for file: {file_name}') try: @@ -74,21 +80,24 @@ def scan_file(self, file, file_name, uploaded_by) -> bool: if scan_response.status_code in self.SCAN_CODES['CLEAN']: msg = f'File scan marked as CLEAN for file: {file_name}' - logger.debug(msg) - ClamAVFileScan.objects.log_result( - file, - file_name, - msg, - ClamAVFileScan.Result.CLEAN, - uploaded_by - ) - return True + scan_result = ClamAVFileScan.Result.CLEAN - if scan_response.status_code in self.SCAN_CODES['INFECTED']: + elif scan_response.status_code in self.SCAN_CODES['INFECTED']: msg = f'File scan marked as INFECTED for file: {file_name}' - logger.debug(msg) - return False + scan_result = ClamAVFileScan.Result.INFECTED + + else: + msg = f'Unable to scan file with name: {file_name}' + scan_result = ClamAVFileScan.Result.ERROR - msg = f'Unable to scan file with name: {file_name}' + # Log and create audit records with the results of this scan logger.debug(msg) - return False + ClamAVFileScan.objects.log_result( + file, + file_name, + msg, + scan_result, + uploaded_by + ) + + return True if scan_result == ClamAVFileScan.Result.CLEAN else False diff --git a/tdrs-backend/tdpservice/security/migrations/0001_initial.py b/tdrs-backend/tdpservice/security/migrations/0001_initial.py index 14ebda266e..6553ae923b 100644 --- a/tdrs-backend/tdpservice/security/migrations/0001_initial.py +++ b/tdrs-backend/tdpservice/security/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.5 on 2021-09-13 19:05 +# Generated by Django 3.2.5 on 2021-09-13 20:27 from django.conf import settings from django.db import migrations, models @@ -19,6 +19,7 @@ class Migration(migrations.Migration): name='ClamAVFileScan', fields=[ ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('scanned_at', models.DateTimeField(auto_now_add=True)), ('file_name', models.TextField()), ('file_shasum', models.TextField(help_text='The SHA256 checksum of the uploaded file')), ('result', models.CharField(choices=[('CLEAN', 'Clean'), ('INFECTED', 'Infected'), ('ERROR', 'Error')], help_text='Scan result for uploaded file', max_length=12)), diff --git a/tdrs-backend/tdpservice/security/models.py b/tdrs-backend/tdpservice/security/models.py index be1edc0480..1fd0157910 100644 --- a/tdrs-backend/tdpservice/security/models.py +++ b/tdrs-backend/tdpservice/security/models.py @@ -39,6 +39,7 @@ class Result(models.TextChoices): INFECTED = 'INFECTED' ERROR = 'ERROR' + scanned_at = models.DateTimeField(auto_now_add=True) file_name = models.TextField() file_shasum = models.TextField( help_text='The SHA256 checksum of the uploaded file' diff --git a/tdrs-backend/tdpservice/users/migrations/0018_ofa_admin_permissions.py b/tdrs-backend/tdpservice/users/migrations/0018_ofa_admin_permissions.py index 6a47834022..a0144537e8 100644 --- a/tdrs-backend/tdpservice/users/migrations/0018_ofa_admin_permissions.py +++ b/tdrs-backend/tdpservice/users/migrations/0018_ofa_admin_permissions.py @@ -1,32 +1,15 @@ # Generated by Django 3.2.5 on 2021-08-16 14:10 -from django.contrib.auth.management import create_permissions from django.db import migrations from tdpservice.users.permissions import ( add_permissions_q, + create_perms, delete_permissions_q, get_permission_ids_for_model, view_permissions_q ) -def create_perms(apps, schema_editor): - """Creates permissions for all installed apps. - - This is needed because Django does not actually create any Content Types - or Permissions until a post_migrate signal is raised after the completion - of `manage.py migrate`. When this migration is run as part of a set for a - freshly created database, that signal will not run until all migrations are - complete - resulting in no permissions for any Group. - - For more info: https://code.djangoproject.com/ticket/29843 - """ - for app_config in apps.get_app_configs(): - app_config.models_module = True - create_permissions(app_config, apps=apps, verbosity=0) - app_config.models_module = None - - def set_ofa_admin_permissions(apps, schema_editor): """Set relevant Group Permissions for OFA Admin group.""" ofa_admin = apps.get_model('auth', 'Group').objects.get(name='OFA Admin') diff --git a/tdrs-backend/tdpservice/users/migrations/0020_ofa_system_admin_permissions.py b/tdrs-backend/tdpservice/users/migrations/0020_ofa_system_admin_permissions.py index b66bb84b3e..61d00cc918 100644 --- a/tdrs-backend/tdpservice/users/migrations/0020_ofa_system_admin_permissions.py +++ b/tdrs-backend/tdpservice/users/migrations/0020_ofa_system_admin_permissions.py @@ -7,8 +7,6 @@ def set_ofa_system_admin_permissions(apps, schema_editor): """Set relevant Group Permissions for OFA System Admin group.""" ofa_system_admin, _ = ( - # TODO: Merge this with the migration generated for: - # https://github.com/raft-tech/TANF-app/pull/1228 apps.get_model('auth', 'Group') .objects .get_or_create(name='OFA System Admin') diff --git a/tdrs-backend/tdpservice/users/migrations/0022_add_clam_av_file_scan_perms.py b/tdrs-backend/tdpservice/users/migrations/0022_add_clam_av_file_scan_perms.py new file mode 100644 index 0000000000..d4f5658e21 --- /dev/null +++ b/tdrs-backend/tdpservice/users/migrations/0022_add_clam_av_file_scan_perms.py @@ -0,0 +1,49 @@ +# Generated by Django 3.2.5 on 2021-09-13 20:53 + +from django.db import migrations + +from tdpservice.users.permissions import ( + create_perms, + get_permission_ids_for_model, + view_permissions_q +) + + +def set_clamav_file_scan_permissions(apps, schema_editor): + """Set permissions for the ClamAVFileScan model for relevant groups.""" + ofa_admin = apps.get_model('auth', 'Group').objects.get(name='OFA Admin') + ofa_system_admin = ( + apps.get_model('auth', 'Group') + .objects + .get(name='OFA System Admin') + ) + + av_scan_permissions = get_permission_ids_for_model( + 'security', + 'clamavfilescan', + filters=[view_permissions_q] + ) + + # Only OFA Admin and OFA System Admin need access to this model + ofa_admin.permissions.add(*av_scan_permissions) + ofa_system_admin.permissions.add(*av_scan_permissions) + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0021_add_dev_group'), + ] + + operations = [ + # We need to call this again to ensure permissions are created for + # the newly added ClamAVFileScan model. + migrations.RunPython( + create_perms, + reverse_code=migrations.RunPython.noop + ), + migrations.RunPython( + set_clamav_file_scan_permissions, + reverse_code=migrations.RunPython.noop + ) + ] diff --git a/tdrs-backend/tdpservice/users/permissions.py b/tdrs-backend/tdpservice/users/permissions.py index 7e414d6057..cab87ee987 100644 --- a/tdrs-backend/tdpservice/users/permissions.py +++ b/tdrs-backend/tdpservice/users/permissions.py @@ -4,6 +4,7 @@ from typing import List, Optional, TYPE_CHECKING from django.apps import apps +from django.contrib.auth.management import create_permissions from django.db.models import Q, QuerySet from rest_framework import permissions @@ -19,6 +20,23 @@ view_permissions_q = Q(codename__startswith='view_') +def create_perms(_apps, *_): + """Creates permissions for all installed apps. + + This is needed because Django does not actually create any Content Types + or Permissions until a post_migrate signal is raised after the completion + of `manage.py migrate`. When this migration is run as part of a set for a + freshly created database, that signal will not run until all migrations are + complete - resulting in no permissions for any Group. + + For more info: https://code.djangoproject.com/ticket/29843 + """ + for app_config in _apps.get_app_configs(): + app_config.models_module = True + create_permissions(app_config, apps=_apps, verbosity=0) + app_config.models_module = None + + def get_permission_ids_for_model( app_label: str, model_name: str, From 121d8c82db520af6e9c4a0199f050072d5734ff2 Mon Sep 17 00:00:00 2001 From: John Willis Date: Tue, 14 Sep 2021 13:50:01 -0400 Subject: [PATCH 03/12] Handle uncaught exceptions, linting/comment fixes --- .../tdpservice/data_files/serializers.py | 4 ++ .../tdpservice/data_files/validators.py | 2 +- tdrs-backend/tdpservice/security/clients.py | 39 +++++++------ .../security/migrations/0001_initial.py | 8 ++- tdrs-backend/tdpservice/security/models.py | 57 ++++++++++++++++--- tdrs-backend/tdpservice/users/permissions.py | 6 +- 6 files changed, 86 insertions(+), 30 deletions(-) diff --git a/tdrs-backend/tdpservice/data_files/serializers.py b/tdrs-backend/tdpservice/data_files/serializers.py index c334fe3f0e..6b6c2ed31b 100644 --- a/tdrs-backend/tdpservice/data_files/serializers.py +++ b/tdrs-backend/tdpservice/data_files/serializers.py @@ -41,10 +41,14 @@ class Meta: def create(self, validated_data): """Create a new entry with a new version number.""" data_file = DataFile.create_new_version(validated_data) + + # Determine the matching ClamAVFileScan for this DataFile. av_scan = ClamAVFileScan.objects.filter( file_name=data_file.original_filename, uploaded_by=data_file.user ).last() + + # Link the newly created DataFile to the relevant ClamAVFileScan. if av_scan is not None: av_scan.data_file = data_file av_scan.save() diff --git a/tdrs-backend/tdpservice/data_files/validators.py b/tdrs-backend/tdpservice/data_files/validators.py index bfdba82c03..098bd689f6 100644 --- a/tdrs-backend/tdpservice/data_files/validators.py +++ b/tdrs-backend/tdpservice/data_files/validators.py @@ -50,7 +50,7 @@ def validate_file_infection(file, uploaded_by): """Validate file is not infected by scanning with ClamAV.""" av_client = ClamAVClient() try: - is_file_clean = av_client.scan_file(file, file.name, uploaded_by) + is_file_clean = av_client.scan_file(file, uploaded_by) except ClamAVClient.ServiceUnavailable: raise ValidationError( 'Unable to complete security inspection, please try again or ' diff --git a/tdrs-backend/tdpservice/security/clients.py b/tdrs-backend/tdpservice/security/clients.py index 507edd7fcd..2dcc8b8052 100644 --- a/tdrs-backend/tdpservice/security/clients.py +++ b/tdrs-backend/tdpservice/security/clients.py @@ -48,53 +48,56 @@ def init_session(self): session.mount(self.endpoint_url, HTTPAdapter(max_retries=retries)) return session - def scan_file( - self, - file: File, - file_name: str, - uploaded_by: User - ) -> bool: + def scan_file(self, file: File, uploaded_by: User) -> bool: """Scan a file for virus infections. :param file: The file or file-like object that should be scanned - :param file_name: - The name of the target file (str). :param uploaded_by: The User that uploaded the given file. :returns is_file_clean: A boolean indicating whether or not the file passed the ClamAV scan :raises ClamAVClient.ServiceUnavailable: """ - logger.debug(f'Initiating virus scan for file: {file_name}') + logger.debug(f'Initiating virus scan for file: {file.name}') try: scan_response = self.session.post( self.endpoint_url, - data={'name': file_name}, + data={'name': file.name}, files={'file': file}, timeout=settings.AV_SCAN_TIMEOUT ) + except ConnectionError as err: - logger.debug(f'Encountered error scanning file: {err}') + logger.error(f'ClamAV connection failure: {err}') raise self.ServiceUnavailable() - if scan_response.status_code in self.SCAN_CODES['CLEAN']: - msg = f'File scan marked as CLEAN for file: {file_name}' + except UnicodeEncodeError as err: + logger.debug(f'Unable to decode file: {err}') + scan_response = None + + if ( + scan_response is not None and + scan_response.status_code in self.SCAN_CODES['CLEAN'] + ): + msg = f'File scan marked as CLEAN for file: {file.name}' scan_result = ClamAVFileScan.Result.CLEAN - elif scan_response.status_code in self.SCAN_CODES['INFECTED']: - msg = f'File scan marked as INFECTED for file: {file_name}' + elif ( + scan_response is not None and + scan_response.status_code in self.SCAN_CODES['INFECTED'] + ): + msg = f'File scan marked as INFECTED for file: {file.name}' scan_result = ClamAVFileScan.Result.INFECTED else: - msg = f'Unable to scan file with name: {file_name}' + msg = f'Unable to scan file with name: {file.name}' scan_result = ClamAVFileScan.Result.ERROR # Log and create audit records with the results of this scan logger.debug(msg) - ClamAVFileScan.objects.log_result( + ClamAVFileScan.objects.record_scan( file, - file_name, msg, scan_result, uploaded_by diff --git a/tdrs-backend/tdpservice/security/migrations/0001_initial.py b/tdrs-backend/tdpservice/security/migrations/0001_initial.py index 6553ae923b..119de88354 100644 --- a/tdrs-backend/tdpservice/security/migrations/0001_initial.py +++ b/tdrs-backend/tdpservice/security/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.5 on 2021-09-13 20:27 +# Generated by Django 3.2.5 on 2021-09-14 15:47 from django.conf import settings from django.db import migrations, models @@ -10,8 +10,8 @@ class Migration(migrations.Migration): initial = True dependencies = [ - ('data_files', '0009_update_log_entry_content_type'), migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('data_files', '0009_update_log_entry_content_type'), ] operations = [ @@ -21,10 +21,14 @@ class Migration(migrations.Migration): ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), ('scanned_at', models.DateTimeField(auto_now_add=True)), ('file_name', models.TextField()), + ('file_size', models.PositiveBigIntegerField(help_text='The file size in bytes')), ('file_shasum', models.TextField(help_text='The SHA256 checksum of the uploaded file')), ('result', models.CharField(choices=[('CLEAN', 'Clean'), ('INFECTED', 'Infected'), ('ERROR', 'Error')], help_text='Scan result for uploaded file', max_length=12)), ('data_file', models.ForeignKey(blank=True, help_text='The resulting DataFile object if this scan was clean', null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='av_scans', to='data_files.datafile')), ('uploaded_by', models.ForeignKey(help_text='The user that uploaded the scanned file', null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='av_scans', to=settings.AUTH_USER_MODEL)), ], + options={ + 'verbose_name': 'Clam AV File Scan', + }, ), ] diff --git a/tdrs-backend/tdpservice/security/models.py b/tdrs-backend/tdpservice/security/models.py index 1fd0157910..f9be68b6f2 100644 --- a/tdrs-backend/tdpservice/security/models.py +++ b/tdrs-backend/tdpservice/security/models.py @@ -1,46 +1,72 @@ from hashlib import sha256 +import logging from django.contrib.admin.models import ContentType, LogEntry, ADDITION +from django.core.files.base import File from django.db import models from tdpservice.data_files.models import DataFile from tdpservice.users.models import User +logger = logging.getLogger(__name__) + class ClamAVFileScanManager(models.Manager): """Extends object manager functionality with common operations.""" - def log_result(self, file, file_name, msg, result, uploaded_by): - """TODO.""" - scan_result = self.model.objects.create( - file_name=file_name, - file_shasum=sha256(file.read()).hexdigest(), + def record_scan( + self, + file: File, + msg: str, + result: 'ClamAVFileScan.Result', + uploaded_by: User + ) -> 'ClamAVFileScan': + """Create a new ClamAVFileScan instance with associated LogEntry.""" + try: + file_shasum = sha256(file.read()).hexdigest() + except (TypeError, ValueError) as err: + logger.error(f'Encountered error deriving file hash: {err}') + file_shasum = 'INVALID' + + av_scan = self.model.objects.create( + file_name=file.name, + file_size=file.size, + file_shasum=file_shasum, result=result, uploaded_by=uploaded_by ) + + # Create a new LogEntry that is tied to this model instance. content_type = ContentType.objects.get_for_model(ClamAVFileScan) LogEntry.objects.log_action( user_id=uploaded_by.pk, content_type_id=content_type.pk, - object_id=scan_result.pk, - object_repr=str(scan_result), + object_id=av_scan.pk, + object_repr=str(av_scan), action_flag=ADDITION, change_message=msg ) - return scan_result + return av_scan class ClamAVFileScan(models.Model): """Represents a ClamAV virus scan performed for an uploaded file.""" + class Meta: + verbose_name = 'Clam AV File Scan' + class Result(models.TextChoices): + """Represents the possible results from a completed ClamAV scan.""" CLEAN = 'CLEAN' INFECTED = 'INFECTED' ERROR = 'ERROR' scanned_at = models.DateTimeField(auto_now_add=True) file_name = models.TextField() + file_size = models.PositiveBigIntegerField( + help_text='The file size in bytes' + ) file_shasum = models.TextField( help_text='The SHA256 checksum of the uploaded file' ) @@ -67,3 +93,18 @@ class Result(models.TextChoices): ) objects = ClamAVFileScanManager() + + def __str__(self) -> str: + """Return string representation of model instance.""" + return f'{self.file_name} ({self.file_size_humanized}) - {self.result}' + + @property + def file_size_humanized(self) -> str: + """Convert the file size into the largest human readable unit.""" + size = self.file_size + for unit in ['B', 'KB', 'MB', 'GB', 'TB']: + if size < 1024.0: + break + size /= 1024.0 + + return f'{size:.{2}f}{unit}' diff --git a/tdrs-backend/tdpservice/users/permissions.py b/tdrs-backend/tdpservice/users/permissions.py index cab87ee987..b040879b1a 100644 --- a/tdrs-backend/tdpservice/users/permissions.py +++ b/tdrs-backend/tdpservice/users/permissions.py @@ -23,9 +23,11 @@ def create_perms(_apps, *_): """Creates permissions for all installed apps. + Intended for use in data migrations that add/edit/remove group permissions. + This is needed because Django does not actually create any Content Types or Permissions until a post_migrate signal is raised after the completion - of `manage.py migrate`. When this migration is run as part of a set for a + of `manage.py migrate`. When a migration is run as part of a set for a freshly created database, that signal will not run until all migrations are complete - resulting in no permissions for any Group. @@ -45,6 +47,8 @@ def get_permission_ids_for_model( ) -> QuerySet['Permission']: """Retrieve the permissions associated with a given model. + Intended for use in data migrations that add/edit/remove group permissions. + Optionally apply a list of Q objects as filters or exclusions that will be chained together via an OR clause to the database. From a95300f5908239467e8507b795c3b5a138f6b0b9 Mon Sep 17 00:00:00 2001 From: John Willis Date: Tue, 14 Sep 2021 15:22:51 -0400 Subject: [PATCH 04/12] Linter fixes --- tdrs-backend/tdpservice/security/admin.py | 12 ++++++++++-- tdrs-backend/tdpservice/security/apps.py | 3 +++ tdrs-backend/tdpservice/security/clients.py | 2 +- tdrs-backend/tdpservice/security/models.py | 4 ++++ tdrs-backend/tdpservice/users/permissions.py | 2 +- 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/tdrs-backend/tdpservice/security/admin.py b/tdrs-backend/tdpservice/security/admin.py index fbb359d10d..ddc9548c20 100644 --- a/tdrs-backend/tdpservice/security/admin.py +++ b/tdrs-backend/tdpservice/security/admin.py @@ -1,3 +1,4 @@ +"""Admin classes for the tdpservice.security app.""" from django.contrib import admin from tdpservice.security.models import ClamAVFileScan @@ -5,16 +6,23 @@ @admin.register(ClamAVFileScan) class ClamAVFileScanAdmin(admin.ModelAdmin): + """Admin interface for Clam AV File Scan instances.""" list_display = [ 'scanned_at', - 'file_name', - 'file_shasum', 'result', 'uploaded_by', + 'file_name', + 'file_size_human', + 'file_shasum', ] list_filter = [ 'result', 'uploaded_by', ] + + @admin.display(description='File Size') + def file_size_human(self, obj): + """Return human friendly file size, converted to appropriate unit.""" + return obj.file_size_humanized diff --git a/tdrs-backend/tdpservice/security/apps.py b/tdrs-backend/tdpservice/security/apps.py index cc49655d2f..e02d7dfba0 100644 --- a/tdrs-backend/tdpservice/security/apps.py +++ b/tdrs-backend/tdpservice/security/apps.py @@ -1,7 +1,10 @@ +"""App configuration for tdpservice.security.""" from django.apps import AppConfig class SecurityConfig(AppConfig): + """App configuration.""" + default_auto_field = 'django.db.models.BigAutoField' name = 'tdpservice.security' verbose_name = 'Security' diff --git a/tdrs-backend/tdpservice/security/clients.py b/tdrs-backend/tdpservice/security/clients.py index 2dcc8b8052..e7f3d70bf8 100644 --- a/tdrs-backend/tdpservice/security/clients.py +++ b/tdrs-backend/tdpservice/security/clients.py @@ -1,4 +1,4 @@ -"""Globally available external client services.""" +"""External client services related to security auditing.""" from requests.adapters import HTTPAdapter from requests.exceptions import ConnectionError from requests.packages.urllib3.util.retry import Retry diff --git a/tdrs-backend/tdpservice/security/models.py b/tdrs-backend/tdpservice/security/models.py index f9be68b6f2..cafc5cf3ee 100644 --- a/tdrs-backend/tdpservice/security/models.py +++ b/tdrs-backend/tdpservice/security/models.py @@ -1,3 +1,4 @@ +"""Models for the tdpservice.security app.""" from hashlib import sha256 import logging @@ -54,10 +55,13 @@ class ClamAVFileScan(models.Model): """Represents a ClamAV virus scan performed for an uploaded file.""" class Meta: + """Model Meta options.""" + verbose_name = 'Clam AV File Scan' class Result(models.TextChoices): """Represents the possible results from a completed ClamAV scan.""" + CLEAN = 'CLEAN' INFECTED = 'INFECTED' ERROR = 'ERROR' diff --git a/tdrs-backend/tdpservice/users/permissions.py b/tdrs-backend/tdpservice/users/permissions.py index b040879b1a..d8fb7667e6 100644 --- a/tdrs-backend/tdpservice/users/permissions.py +++ b/tdrs-backend/tdpservice/users/permissions.py @@ -21,7 +21,7 @@ def create_perms(_apps, *_): - """Creates permissions for all installed apps. + """Create permissions for all installed apps. Intended for use in data migrations that add/edit/remove group permissions. From b6437921fc67231f8bc8dab98fe7e373cf54ac45 Mon Sep 17 00:00:00 2001 From: John Willis Date: Tue, 14 Sep 2021 16:51:22 -0400 Subject: [PATCH 05/12] Fix tests --- .../tdpservice/data_files/serializers.py | 8 ++-- .../data_files/test/test_serializers.py | 42 +++++++++++++++---- .../tdpservice/data_files/validators.py | 5 +-- tdrs-backend/tdpservice/data_files/views.py | 6 +++ tdrs-backend/tdpservice/security/clients.py | 15 ++++--- tdrs-backend/tdpservice/security/models.py | 36 +++++++++++++--- .../tdpservice/security/test/test_clamav.py | 27 ++++++++++-- .../0022_add_clam_av_file_scan_perms.py | 10 +++++ .../tdpservice/users/test/test_permissions.py | 2 + 9 files changed, 121 insertions(+), 30 deletions(-) diff --git a/tdrs-backend/tdpservice/data_files/serializers.py b/tdrs-backend/tdpservice/data_files/serializers.py index 6b6c2ed31b..c8492e05cb 100644 --- a/tdrs-backend/tdpservice/data_files/serializers.py +++ b/tdrs-backend/tdpservice/data_files/serializers.py @@ -59,9 +59,9 @@ def update(self, instance, validated_data): """Throw an error if a user tries to update a data_file.""" raise ImmutabilityError(instance, validated_data) - def validate(self, attrs): + def validate_file(self, file): """Perform all validation steps on a given file.""" - file, user = attrs['file'], attrs['user'] + user = self.context.get('user') validate_file_extension(file.name) - validate_file_infection(file, user) - return attrs + validate_file_infection(file, file.name, user) + return file diff --git a/tdrs-backend/tdpservice/data_files/test/test_serializers.py b/tdrs-backend/tdpservice/data_files/test/test_serializers.py index 09ede9392d..97e09c3274 100644 --- a/tdrs-backend/tdpservice/data_files/test/test_serializers.py +++ b/tdrs-backend/tdpservice/data_files/test/test_serializers.py @@ -4,25 +4,41 @@ from tdpservice.data_files.errors import ImmutabilityError from tdpservice.data_files.serializers import DataFileSerializer -from tdpservice.data_files.validators import validate_file_extension +from tdpservice.data_files.validators import ( + validate_file_extension, + validate_file_infection +) @pytest.mark.django_db -def test_serializer_with_valid_data(data_file_data): +def test_serializer_with_valid_data(data_file_data, user): """If a serializer has valid data it will return a valid object.""" - create_serializer = DataFileSerializer(data=data_file_data) + create_serializer = DataFileSerializer( + context={'user': user}, + data=data_file_data + ) create_serializer.is_valid(raise_exception=True) assert create_serializer.is_valid() is True @pytest.mark.django_db -def test_serializer_increment_create(data_file_data, other_data_file_data): +def test_serializer_increment_create( + data_file_data, + other_data_file_data, + user +): """Test serializer produces data_files with correct version.""" - serializer_1 = DataFileSerializer(data=data_file_data) + serializer_1 = DataFileSerializer( + context={'user': user}, + data=data_file_data + ) assert serializer_1.is_valid() is True data_file_1 = serializer_1.save() - serializer_2 = DataFileSerializer(data=other_data_file_data) + serializer_2 = DataFileSerializer( + context={'user': user}, + data=other_data_file_data + ) assert serializer_2.is_valid() is True data_file_2 = serializer_2.save() @@ -45,9 +61,12 @@ def test_immutability_of_data_file(data_file_instance): @pytest.mark.django_db -def test_created_at(data_file_data): +def test_created_at(data_file_data, user): """If a serializer has valid data it will return a valid object.""" - create_serializer = DataFileSerializer(data=data_file_data) + create_serializer = DataFileSerializer( + context={'user': user}, + data=data_file_data + ) assert create_serializer.is_valid() is True data_file = create_serializer.save() @@ -87,3 +106,10 @@ def test_rejects_invalid_file_extensions(file_name): """Test invalid file names are rejected by serializer validation.""" with pytest.raises(ValidationError): validate_file_extension(file_name) + + +@pytest.mark.django_db +def test_rejects_infected_file(infected_file, fake_file_name, user): + """Test infected files are rejected by serializer validation.""" + with pytest.raises(ValidationError): + validate_file_infection(infected_file, fake_file_name, user) diff --git a/tdrs-backend/tdpservice/data_files/validators.py b/tdrs-backend/tdpservice/data_files/validators.py index 098bd689f6..bf7a40684e 100644 --- a/tdrs-backend/tdpservice/data_files/validators.py +++ b/tdrs-backend/tdpservice/data_files/validators.py @@ -46,11 +46,10 @@ def validate_file_extension(file_name: str): raise ValidationError(msg) -def validate_file_infection(file, uploaded_by): +def validate_file_infection(file, file_name, uploaded_by): """Validate file is not infected by scanning with ClamAV.""" - av_client = ClamAVClient() try: - is_file_clean = av_client.scan_file(file, uploaded_by) + is_file_clean = ClamAVClient().scan_file(file, file_name, uploaded_by) except ClamAVClient.ServiceUnavailable: raise ValidationError( 'Unable to complete security inspection, please try again or ' diff --git a/tdrs-backend/tdpservice/data_files/views.py b/tdrs-backend/tdpservice/data_files/views.py index e1ed2e1757..9a478c81ca 100644 --- a/tdrs-backend/tdpservice/data_files/views.py +++ b/tdrs-backend/tdpservice/data_files/views.py @@ -55,6 +55,12 @@ def filter_queryset(self, queryset): self.filterset_class = None return super().filter_queryset(queryset) + def get_serializer_context(self): + """Retrieve additional context required by serializer.""" + context = super().get_serializer_context() + context['user'] = self.request.user + return context + @action(methods=["get"], detail=True) def download(self, request, pk=None): """Retrieve a file from s3 then stream it to the client.""" diff --git a/tdrs-backend/tdpservice/security/clients.py b/tdrs-backend/tdpservice/security/clients.py index e7f3d70bf8..44af156b20 100644 --- a/tdrs-backend/tdpservice/security/clients.py +++ b/tdrs-backend/tdpservice/security/clients.py @@ -48,22 +48,24 @@ def init_session(self): session.mount(self.endpoint_url, HTTPAdapter(max_retries=retries)) return session - def scan_file(self, file: File, uploaded_by: User) -> bool: + def scan_file(self, file: File, file_name: str, uploaded_by: User) -> bool: """Scan a file for virus infections. :param file: The file or file-like object that should be scanned + :param file_name: + The string name of the file. :param uploaded_by: The User that uploaded the given file. :returns is_file_clean: A boolean indicating whether or not the file passed the ClamAV scan :raises ClamAVClient.ServiceUnavailable: """ - logger.debug(f'Initiating virus scan for file: {file.name}') + logger.debug(f'Initiating virus scan for file: {file_name}') try: scan_response = self.session.post( self.endpoint_url, - data={'name': file.name}, + data={'name': file_name}, files={'file': file}, timeout=settings.AV_SCAN_TIMEOUT ) @@ -80,24 +82,25 @@ def scan_file(self, file: File, uploaded_by: User) -> bool: scan_response is not None and scan_response.status_code in self.SCAN_CODES['CLEAN'] ): - msg = f'File scan marked as CLEAN for file: {file.name}' + msg = f'File scan marked as CLEAN for file: {file_name}' scan_result = ClamAVFileScan.Result.CLEAN elif ( scan_response is not None and scan_response.status_code in self.SCAN_CODES['INFECTED'] ): - msg = f'File scan marked as INFECTED for file: {file.name}' + msg = f'File scan marked as INFECTED for file: {file_name}' scan_result = ClamAVFileScan.Result.INFECTED else: - msg = f'Unable to scan file with name: {file.name}' + msg = f'Unable to scan file with name: {file_name}' scan_result = ClamAVFileScan.Result.ERROR # Log and create audit records with the results of this scan logger.debug(msg) ClamAVFileScan.objects.record_scan( file, + file_name, msg, scan_result, uploaded_by diff --git a/tdrs-backend/tdpservice/security/models.py b/tdrs-backend/tdpservice/security/models.py index cafc5cf3ee..f2b328e550 100644 --- a/tdrs-backend/tdpservice/security/models.py +++ b/tdrs-backend/tdpservice/security/models.py @@ -1,5 +1,7 @@ """Models for the tdpservice.security app.""" from hashlib import sha256 +from io import StringIO +from typing import Union import logging from django.contrib.admin.models import ContentType, LogEntry, ADDITION @@ -12,26 +14,50 @@ logger = logging.getLogger(__name__) +def get_file_shasum(file: File) -> str: + """Derive the SHA256 checksum of the file.""" + f = file.open('rb') + _hash = sha256() + + # For large files we need to read it in by chunks to prevent invalid hashes + if f.multiple_chunks(): + for chunk in f.chunks(): + _hash.update(chunk) + else: + _hash.update(f.read()) + + # Ensure to reset the file so it can be read in further operations. + f.seek(0) + + return _hash.hexdigest() + + class ClamAVFileScanManager(models.Manager): """Extends object manager functionality with common operations.""" def record_scan( self, - file: File, + file: Union[File, StringIO], + file_name: str, msg: str, result: 'ClamAVFileScan.Result', uploaded_by: User ) -> 'ClamAVFileScan': """Create a new ClamAVFileScan instance with associated LogEntry.""" try: - file_shasum = sha256(file.read()).hexdigest() - except (TypeError, ValueError) as err: + file_shasum = get_file_shasum(file) + except (AttributeError, TypeError, ValueError) as err: logger.error(f'Encountered error deriving file hash: {err}') file_shasum = 'INVALID' + # Create the ClamAVFileScan instance. av_scan = self.model.objects.create( - file_name=file.name, - file_size=file.size, + file_name=file_name, + file_size=( + file.size + if isinstance(file, File) + else len(file.getvalue()) + ), file_shasum=file_shasum, result=result, uploaded_by=uploaded_by diff --git a/tdrs-backend/tdpservice/security/test/test_clamav.py b/tdrs-backend/tdpservice/security/test/test_clamav.py index 10ea2a234a..34745781e0 100644 --- a/tdrs-backend/tdpservice/security/test/test_clamav.py +++ b/tdrs-backend/tdpservice/security/test/test_clamav.py @@ -3,6 +3,7 @@ import pytest from tdpservice.security.clients import ClamAVClient +from tdpservice.security.models import ClamAVFileScan @pytest.fixture @@ -23,29 +24,47 @@ def assert_clamav_url(clamav_url): assert clamav_url is not None +@pytest.mark.django_db def test_clamav_accepts_files( clamav_client, clamav_url, fake_file, - fake_file_name + fake_file_name, + user ): """Test that ClamAV is configured and accessible by this application.""" assert_clamav_url(clamav_url) # Send a fake file to ClamAV to ensure it is accessible and accepts files. - is_file_clean = clamav_client.scan_file(fake_file, fake_file_name) + is_file_clean = clamav_client.scan_file(fake_file, fake_file_name, user) assert is_file_clean is True + av_scan = ClamAVFileScan.objects.get( + file_name=fake_file_name, + uploaded_by=user + ) + assert av_scan is not None + assert av_scan.result == ClamAVFileScan.Result.CLEAN + +@pytest.mark.django_db def test_clamav_rejects_infected_files( clamav_client, clamav_url, infected_file, - fake_file_name + fake_file_name, + user ): """Test that ClamAV will reject files that match infection signatures.""" assert_clamav_url(clamav_url) # Send a test file that will be treated as "infected" - is_file_clean = clamav_client.scan_file(infected_file, fake_file_name) + is_file_clean = clamav_client.scan_file(infected_file, fake_file_name, user) assert is_file_clean is False + + av_scan = ClamAVFileScan.objects.get( + file_name=fake_file_name, + uploaded_by=user + ) + assert av_scan is not None + assert av_scan.result == ClamAVFileScan.Result.INFECTED diff --git a/tdrs-backend/tdpservice/users/migrations/0022_add_clam_av_file_scan_perms.py b/tdrs-backend/tdpservice/users/migrations/0022_add_clam_av_file_scan_perms.py index d4f5658e21..f51c72fac0 100644 --- a/tdrs-backend/tdpservice/users/migrations/0022_add_clam_av_file_scan_perms.py +++ b/tdrs-backend/tdpservice/users/migrations/0022_add_clam_av_file_scan_perms.py @@ -28,6 +28,16 @@ def set_clamav_file_scan_permissions(apps, schema_editor): ofa_admin.permissions.add(*av_scan_permissions) ofa_system_admin.permissions.add(*av_scan_permissions) + # On new systems the previous migration for OFA System Admin will add extra + # permissions, remove these if so. + unwanted_permissions = get_permission_ids_for_model( + 'security', + 'clamavfilescan', + exclusions=[view_permissions_q] + ) + + ofa_system_admin.permissions.remove(*unwanted_permissions) + class Migration(migrations.Migration): diff --git a/tdrs-backend/tdpservice/users/test/test_permissions.py b/tdrs-backend/tdpservice/users/test/test_permissions.py index a229d89898..ba49e9da7f 100644 --- a/tdrs-backend/tdpservice/users/test/test_permissions.py +++ b/tdrs-backend/tdpservice/users/test/test_permissions.py @@ -10,6 +10,7 @@ def test_ofa_admin_permissions(ofa_admin): 'auth.view_group', 'data_files.add_datafile', 'data_files.view_datafile', + 'security.view_clamavfilescan', 'stts.view_region', 'stts.view_stt', 'users.add_user', @@ -51,6 +52,7 @@ def test_ofa_system_admin_permissions(ofa_system_admin): 'data_files.add_datafile', 'data_files.change_datafile', 'data_files.view_datafile', + 'security.view_clamavfilescan', 'sessions.add_session', 'sessions.change_session', 'sessions.view_session', From 47bdc15c99c644f67a4f76c6637f572791182856 Mon Sep 17 00:00:00 2001 From: John Willis Date: Wed, 15 Sep 2021 11:50:08 -0400 Subject: [PATCH 06/12] Increased code coverage --- tdrs-backend/tdpservice/security/clients.py | 14 +--- tdrs-backend/tdpservice/security/models.py | 21 +++++- .../tdpservice/security/test/test_clamav.py | 73 ++++++++++++++----- 3 files changed, 74 insertions(+), 34 deletions(-) diff --git a/tdrs-backend/tdpservice/security/clients.py b/tdrs-backend/tdpservice/security/clients.py index 44af156b20..3734929d29 100644 --- a/tdrs-backend/tdpservice/security/clients.py +++ b/tdrs-backend/tdpservice/security/clients.py @@ -74,21 +74,11 @@ def scan_file(self, file: File, file_name: str, uploaded_by: User) -> bool: logger.error(f'ClamAV connection failure: {err}') raise self.ServiceUnavailable() - except UnicodeEncodeError as err: - logger.debug(f'Unable to decode file: {err}') - scan_response = None - - if ( - scan_response is not None and - scan_response.status_code in self.SCAN_CODES['CLEAN'] - ): + if scan_response.status_code in self.SCAN_CODES['CLEAN']: msg = f'File scan marked as CLEAN for file: {file_name}' scan_result = ClamAVFileScan.Result.CLEAN - elif ( - scan_response is not None and - scan_response.status_code in self.SCAN_CODES['INFECTED'] - ): + elif scan_response.status_code in self.SCAN_CODES['INFECTED']: msg = f'File scan marked as INFECTED for file: {file_name}' scan_result = ClamAVFileScan.Result.INFECTED diff --git a/tdrs-backend/tdpservice/security/models.py b/tdrs-backend/tdpservice/security/models.py index f2b328e550..095978f75c 100644 --- a/tdrs-backend/tdpservice/security/models.py +++ b/tdrs-backend/tdpservice/security/models.py @@ -15,16 +15,29 @@ def get_file_shasum(file: File) -> str: - """Derive the SHA256 checksum of the file.""" - f = file.open('rb') + """Derive the SHA256 checksum of a file.""" _hash = sha256() + # If the file has the `open` method it needs to be called, otherwise this + # input is a file-like object (ie. StringIO) and doesn't need to be opened. + if hasattr(file, 'open'): + f = file.open('rb') + else: + f = file + # For large files we need to read it in by chunks to prevent invalid hashes - if f.multiple_chunks(): + if hasattr(f, 'multiple_chunks') and f.multiple_chunks(): for chunk in f.chunks(): _hash.update(chunk) else: - _hash.update(f.read()) + content = f.read() + + # If the content is returned as a string we must encode it to bytes + # or an error will be raised. + if isinstance(content, str): + content = content.encode('utf-8') + + _hash.update(content) # Ensure to reset the file so it can be read in further operations. f.seek(0) diff --git a/tdrs-backend/tdpservice/security/test/test_clamav.py b/tdrs-backend/tdpservice/security/test/test_clamav.py index 34745781e0..428473d9a0 100644 --- a/tdrs-backend/tdpservice/security/test/test_clamav.py +++ b/tdrs-backend/tdpservice/security/test/test_clamav.py @@ -1,22 +1,29 @@ """Integration test(s) for clamav-rest operations.""" -from django.conf import settings import pytest +from rest_framework.status import HTTP_400_BAD_REQUEST + from tdpservice.security.clients import ClamAVClient from tdpservice.security.models import ClamAVFileScan @pytest.fixture -def clamav_client(clamav_url): +def clamav_client(): """HTTP Client used to send files to ClamAV-REST.""" - av_client = ClamAVClient(endpoint_url=clamav_url) + av_client = ClamAVClient() return av_client @pytest.fixture -def clamav_url(): - """URL that can be used to reach ClamAV-REST.""" - return settings.AV_SCAN_URL +def mock_clamav_response(mocker): + mock_post = mocker.patch('requests.sessions.Session.post') + mock_post.return_value.status_code = HTTP_400_BAD_REQUEST + return mock_post + + +@pytest.fixture +def patch_invalid_clamav_url(settings): + settings.AV_SCAN_URL = 'http://invalid' def assert_clamav_url(clamav_url): @@ -27,44 +34,74 @@ def assert_clamav_url(clamav_url): @pytest.mark.django_db def test_clamav_accepts_files( clamav_client, - clamav_url, fake_file, fake_file_name, + settings, user ): """Test that ClamAV is configured and accessible by this application.""" - assert_clamav_url(clamav_url) + assert_clamav_url(settings.AV_SCAN_URL) # Send a fake file to ClamAV to ensure it is accessible and accepts files. is_file_clean = clamav_client.scan_file(fake_file, fake_file_name, user) assert is_file_clean is True - av_scan = ClamAVFileScan.objects.get( + assert ClamAVFileScan.objects.filter( file_name=fake_file_name, + result=ClamAVFileScan.Result.CLEAN, uploaded_by=user - ) - assert av_scan is not None - assert av_scan.result == ClamAVFileScan.Result.CLEAN + ).exists() @pytest.mark.django_db def test_clamav_rejects_infected_files( clamav_client, - clamav_url, infected_file, fake_file_name, + settings, user ): """Test that ClamAV will reject files that match infection signatures.""" - assert_clamav_url(clamav_url) + assert_clamav_url(settings.AV_SCAN_URL) # Send a test file that will be treated as "infected" is_file_clean = clamav_client.scan_file(infected_file, fake_file_name, user) assert is_file_clean is False - av_scan = ClamAVFileScan.objects.get( + assert ClamAVFileScan.objects.filter( file_name=fake_file_name, + result=ClamAVFileScan.Result.INFECTED, uploaded_by=user - ) - assert av_scan is not None - assert av_scan.result == ClamAVFileScan.Result.INFECTED + ).exists() + + +@pytest.mark.django_db +def test_clamav_scan_error( + clamav_client, + fake_file, + fake_file_name, + mock_clamav_response, + user +): + """Test that ClamAV records scan errors caused by file/user errors.""" + is_file_clean = clamav_client.scan_file(fake_file, fake_file_name, user) + assert is_file_clean is False + + assert ClamAVFileScan.objects.filter( + file_name=fake_file_name, + result=ClamAVFileScan.Result.ERROR, + uploaded_by=user + ).exists() + + +@pytest.mark.django_db +def test_clamav_connection_error( + patch_invalid_clamav_url, + clamav_client, + fake_file, + fake_file_name, + user +): + """Test that the appropriate error is raised when ClamAV is inaccessible.""" + with pytest.raises(ClamAVClient.ServiceUnavailable): + clamav_client.scan_file(fake_file, fake_file_name, user) From 8acb8bcaa9b02414443082745413297e125add87 Mon Sep 17 00:00:00 2001 From: John Willis Date: Wed, 15 Sep 2021 12:29:24 -0400 Subject: [PATCH 07/12] Additional code coverage, linter fixes --- tdrs-backend/tdpservice/security/models.py | 2 +- .../tdpservice/security/test/test_clamav.py | 35 ++++++++++--------- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/tdrs-backend/tdpservice/security/models.py b/tdrs-backend/tdpservice/security/models.py index 095978f75c..0451226164 100644 --- a/tdrs-backend/tdpservice/security/models.py +++ b/tdrs-backend/tdpservice/security/models.py @@ -14,7 +14,7 @@ logger = logging.getLogger(__name__) -def get_file_shasum(file: File) -> str: +def get_file_shasum(file: Union[File, StringIO]) -> str: """Derive the SHA256 checksum of a file.""" _hash = sha256() diff --git a/tdrs-backend/tdpservice/security/test/test_clamav.py b/tdrs-backend/tdpservice/security/test/test_clamav.py index 428473d9a0..a52c7f409e 100644 --- a/tdrs-backend/tdpservice/security/test/test_clamav.py +++ b/tdrs-backend/tdpservice/security/test/test_clamav.py @@ -1,4 +1,5 @@ """Integration test(s) for clamav-rest operations.""" +from requests.sessions import Session import pytest from rest_framework.status import HTTP_400_BAD_REQUEST @@ -10,25 +11,29 @@ @pytest.fixture def clamav_client(): """HTTP Client used to send files to ClamAV-REST.""" - av_client = ClamAVClient() - return av_client + return ClamAVClient() @pytest.fixture def mock_clamav_response(mocker): + """Mock the ClamAV post request to return a 400 Bad Request.""" mock_post = mocker.patch('requests.sessions.Session.post') mock_post.return_value.status_code = HTTP_400_BAD_REQUEST return mock_post @pytest.fixture -def patch_invalid_clamav_url(settings): - settings.AV_SCAN_URL = 'http://invalid' +def invalid_clamav_client(): + """Override the ClamAVClient endpoint_url with an invalid value.""" + return ClamAVClient(endpoint_url='http://invalid') -def assert_clamav_url(clamav_url): - """Ensure that the provided setting for AV_SCAN_URL is configured.""" - assert clamav_url is not None +def test_clamav_endpoint_url(clamav_client, settings): + """Test that the client has the appropriate endpoint URL set.""" + actual_url = clamav_client.endpoint_url + expected_url = settings.AV_SCAN_URL + assert actual_url == expected_url + assert isinstance(clamav_client.session, Session) @pytest.mark.django_db @@ -36,16 +41,14 @@ def test_clamav_accepts_files( clamav_client, fake_file, fake_file_name, - settings, user ): """Test that ClamAV is configured and accessible by this application.""" - assert_clamav_url(settings.AV_SCAN_URL) - # Send a fake file to ClamAV to ensure it is accessible and accepts files. is_file_clean = clamav_client.scan_file(fake_file, fake_file_name, user) assert is_file_clean is True + # Ensure that a CLEAN file scan result is recorded in the database. assert ClamAVFileScan.objects.filter( file_name=fake_file_name, result=ClamAVFileScan.Result.CLEAN, @@ -58,16 +61,14 @@ def test_clamav_rejects_infected_files( clamav_client, infected_file, fake_file_name, - settings, user ): """Test that ClamAV will reject files that match infection signatures.""" - assert_clamav_url(settings.AV_SCAN_URL) - # Send a test file that will be treated as "infected" is_file_clean = clamav_client.scan_file(infected_file, fake_file_name, user) assert is_file_clean is False + # Ensure that an INFECTED scan result is recorded in the database. assert ClamAVFileScan.objects.filter( file_name=fake_file_name, result=ClamAVFileScan.Result.INFECTED, @@ -76,17 +77,18 @@ def test_clamav_rejects_infected_files( @pytest.mark.django_db +@pytest.mark.usefixtures('mock_clamav_response') def test_clamav_scan_error( clamav_client, fake_file, fake_file_name, - mock_clamav_response, user ): """Test that ClamAV records scan errors caused by file/user errors.""" is_file_clean = clamav_client.scan_file(fake_file, fake_file_name, user) assert is_file_clean is False + # Ensure that an ERROR scan result is recorded in the database. assert ClamAVFileScan.objects.filter( file_name=fake_file_name, result=ClamAVFileScan.Result.ERROR, @@ -96,12 +98,11 @@ def test_clamav_scan_error( @pytest.mark.django_db def test_clamav_connection_error( - patch_invalid_clamav_url, - clamav_client, + invalid_clamav_client, fake_file, fake_file_name, user ): """Test that the appropriate error is raised when ClamAV is inaccessible.""" with pytest.raises(ClamAVClient.ServiceUnavailable): - clamav_client.scan_file(fake_file, fake_file_name, user) + invalid_clamav_client.scan_file(fake_file, fake_file_name, user) From 4e1e545fe88545162c34e52eeb5ce176a75ccf83 Mon Sep 17 00:00:00 2001 From: John Willis Date: Wed, 15 Sep 2021 13:36:44 -0400 Subject: [PATCH 08/12] Code coverage --- .../data_files/test/test_serializers.py | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tdrs-backend/tdpservice/data_files/test/test_serializers.py b/tdrs-backend/tdpservice/data_files/test/test_serializers.py index 97e09c3274..af642fc549 100644 --- a/tdrs-backend/tdpservice/data_files/test/test_serializers.py +++ b/tdrs-backend/tdpservice/data_files/test/test_serializers.py @@ -8,6 +8,7 @@ validate_file_extension, validate_file_infection ) +from tdpservice.security.clients import ClamAVClient @pytest.mark.django_db @@ -71,6 +72,34 @@ def test_created_at(data_file_data, user): data_file = create_serializer.save() assert data_file.created_at + assert data_file.av_scans.exists() + + +@pytest.mark.django_db +def test_data_file_still_created_if_av_scan_fails_to_create( + data_file_data, + mocker, + user +): + """Test valid DataFile is still created if ClamAV scan isn't recorded. + + In this scenario all validation would have already passed but in the event + the ClamAVFileScan isn't found in the database due to an error or race + condition we should still store the DataFile. + """ + mocker.patch( + 'tdpservice.security.models.ClamAVFileScanManager.record_scan', + return_value=None + ) + create_serializer = DataFileSerializer( + context={'user': user}, + data=data_file_data + ) + assert create_serializer.is_valid() is True + data_file = create_serializer.save() + + assert data_file is not None + assert data_file.av_scans.count() == 0 @pytest.mark.parametrize("file_name", [ @@ -113,3 +142,19 @@ def test_rejects_infected_file(infected_file, fake_file_name, user): """Test infected files are rejected by serializer validation.""" with pytest.raises(ValidationError): validate_file_infection(infected_file, fake_file_name, user) + + +@pytest.mark.django_db +def test_rejects_uploads_on_clamav_connection_error( + fake_file, + fake_file_name, + mocker, + user +): + """Test that DataFiles cannot pass validation if ClamAV is down.""" + mocker.patch( + 'tdpservice.security.clients.ClamAVClient.scan_file', + side_effect=ClamAVClient.ServiceUnavailable() + ) + with pytest.raises(ValidationError): + validate_file_infection(fake_file, fake_file_name, user) From 5bc46a6f66c15c155eb06bda2a87ac509b6b5e30 Mon Sep 17 00:00:00 2001 From: John Willis Date: Wed, 15 Sep 2021 15:29:28 -0400 Subject: [PATCH 09/12] More code coverage --- .../tdpservice/security/test/test_clamav.py | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tdrs-backend/tdpservice/security/test/test_clamav.py b/tdrs-backend/tdpservice/security/test/test_clamav.py index a52c7f409e..37f8b9a84d 100644 --- a/tdrs-backend/tdpservice/security/test/test_clamav.py +++ b/tdrs-backend/tdpservice/security/test/test_clamav.py @@ -2,8 +2,10 @@ from requests.sessions import Session import pytest +from django.contrib.admin import site as admin_site from rest_framework.status import HTTP_400_BAD_REQUEST +from tdpservice.security.admin import ClamAVFileScanAdmin from tdpservice.security.clients import ClamAVClient from tdpservice.security.models import ClamAVFileScan @@ -106,3 +108,43 @@ def test_clamav_connection_error( """Test that the appropriate error is raised when ClamAV is inaccessible.""" with pytest.raises(ClamAVClient.ServiceUnavailable): invalid_clamav_client.scan_file(fake_file, fake_file_name, user) + + +@pytest.mark.django_db +def test_clamav_hash_errors(fake_file, fake_file_name, mocker, user): + """Test that a failure to derive sha256 hash of file is handled.""" + mocker.patch( + 'tdpservice.security.models.get_file_shasum', + side_effect=AttributeError() + ) + + av_scan = ClamAVFileScan.objects.record_scan( + file=fake_file, + file_name=fake_file_name, + msg='CLEAN', + result=ClamAVFileScan.Result.CLEAN, + uploaded_by=user + ) + + assert av_scan.file_shasum == 'INVALID' + + +@pytest.mark.django_db +def test_clamav_file_scan_admin_file_size_display( + fake_file, + fake_file_name, + user +): + """Test that the ClamAVFileScan Admin page displays formatted file size.""" + av_scan = ClamAVFileScan.objects.record_scan( + file=fake_file, + file_name=fake_file_name, + msg='CLEAN', + result=ClamAVFileScan.Result.CLEAN, + uploaded_by=user + ) + + av_admin = ClamAVFileScanAdmin(ClamAVFileScan, admin_site) + admin_file_size = av_admin.file_size_human(av_scan) + + assert admin_file_size == av_scan.file_size_humanized From b7a534b1ec4085b8c66fa8e7b27f6018a62498d0 Mon Sep 17 00:00:00 2001 From: John Willis Date: Wed, 15 Sep 2021 17:26:11 -0400 Subject: [PATCH 10/12] More code coverage --- .../tdpservice/security/test/test_clamav.py | 79 +++++++++++++++---- 1 file changed, 64 insertions(+), 15 deletions(-) diff --git a/tdrs-backend/tdpservice/security/test/test_clamav.py b/tdrs-backend/tdpservice/security/test/test_clamav.py index 37f8b9a84d..93a8f03028 100644 --- a/tdrs-backend/tdpservice/security/test/test_clamav.py +++ b/tdrs-backend/tdpservice/security/test/test_clamav.py @@ -1,8 +1,10 @@ """Integration test(s) for clamav-rest operations.""" +from os import remove from requests.sessions import Session import pytest from django.contrib.admin import site as admin_site +from django.core.files.base import File from rest_framework.status import HTTP_400_BAD_REQUEST from tdpservice.security.admin import ClamAVFileScanAdmin @@ -10,12 +12,39 @@ from tdpservice.security.models import ClamAVFileScan +@pytest.fixture +def chunky_file(chunky_file_name) -> File: + """Return a file that is large enough to be broken into chunks on read.""" + with open(chunky_file_name, 'wb') as f: + num_chars = 1024 * 1024 # 1 MB + content = ('0' * num_chars).encode('utf-8') + f.write(content) + file = File(f) + + yield file + + # Clean up the generated file after the test completes + remove(chunky_file_name) + + +@pytest.fixture +def chunky_file_name() -> str: + """Return a static string to allow safely removing file after test runs.""" + return 'chunky_file_test.txt' + + @pytest.fixture def clamav_client(): """HTTP Client used to send files to ClamAV-REST.""" return ClamAVClient() +@pytest.fixture +def invalid_clamav_client(): + """Override the ClamAVClient endpoint_url with an invalid value.""" + return ClamAVClient(endpoint_url='http://invalid') + + @pytest.fixture def mock_clamav_response(mocker): """Mock the ClamAV post request to return a 400 Bad Request.""" @@ -24,12 +53,6 @@ def mock_clamav_response(mocker): return mock_post -@pytest.fixture -def invalid_clamav_client(): - """Override the ClamAVClient endpoint_url with an invalid value.""" - return ClamAVClient(endpoint_url='http://invalid') - - def test_clamav_endpoint_url(clamav_client, settings): """Test that the client has the appropriate endpoint URL set.""" actual_url = clamav_client.endpoint_url @@ -111,7 +134,24 @@ def test_clamav_connection_error( @pytest.mark.django_db -def test_clamav_hash_errors(fake_file, fake_file_name, mocker, user): +def test_clamav_shasum_large_file(chunky_file, chunky_file_name, user): + """Test that the file_shasum is correctly generated for large files.""" + # The file should be large enough to split into multiple chunks. + assert chunky_file.multiple_chunks() + + av_scan = ClamAVFileScan.objects.record_scan( + file=chunky_file, + file_name=chunky_file_name, + msg='CLEAN', + result=ClamAVFileScan.Result.CLEAN, + uploaded_by=user + ) + + assert av_scan.file_shasum != 'INVALID' + + +@pytest.mark.django_db +def test_clamav_shasum_invalid(fake_file, fake_file_name, mocker, user): """Test that a failure to derive sha256 hash of file is handled.""" mocker.patch( 'tdpservice.security.models.get_file_shasum', @@ -130,21 +170,30 @@ def test_clamav_hash_errors(fake_file, fake_file_name, mocker, user): @pytest.mark.django_db -def test_clamav_file_scan_admin_file_size_display( - fake_file, +@pytest.mark.parametrize('file_size, expected_value', [ + (1, '1.00B'), + (1024, '1.00KB'), + (1048576, '1.00MB'), + (1073741824, '1.00GB'), + (1099511627776, '1.00TB') +]) +def test_clamav_file_scan_file_size_display( + expected_value, fake_file_name, + file_size, user ): - """Test that the ClamAVFileScan Admin page displays formatted file size.""" - av_scan = ClamAVFileScan.objects.record_scan( - file=fake_file, + """Test that the admin displays a human readable file size.""" + av_scan = ClamAVFileScan.objects.create( file_name=fake_file_name, - msg='CLEAN', + file_size=file_size, + file_shasum='TEST', result=ClamAVFileScan.Result.CLEAN, uploaded_by=user ) + assert av_scan.file_size_humanized == expected_value + # Ensure that the Admin site returns the expected value av_admin = ClamAVFileScanAdmin(ClamAVFileScan, admin_site) admin_file_size = av_admin.file_size_human(av_scan) - - assert admin_file_size == av_scan.file_size_humanized + assert admin_file_size == expected_value From 772cbe43e5b99aa2f8a2ae3c256f1db128971b0b Mon Sep 17 00:00:00 2001 From: John Willis Date: Fri, 17 Sep 2021 12:47:53 -0400 Subject: [PATCH 11/12] Cast AV scan settings to integer --- tdrs-backend/tdpservice/settings/common.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tdrs-backend/tdpservice/settings/common.py b/tdrs-backend/tdpservice/settings/common.py index 9ee09ae2ff..d061878f5d 100755 --- a/tdrs-backend/tdpservice/settings/common.py +++ b/tdrs-backend/tdpservice/settings/common.py @@ -299,13 +299,13 @@ class Common(Configuration): # The factor used to determine how long to wait before retrying failed scans # NOTE: This value exponentially increases up to the maximum retries allowed # algo: {backoff factor} * (2 ** ({number of total retries} - 1)) - AV_SCAN_BACKOFF_FACTOR = os.getenv('AV_SCAN_BACKOFF_FACTOR', 1) + AV_SCAN_BACKOFF_FACTOR = int(os.getenv('AV_SCAN_BACKOFF_FACTOR', 1)) # The maximum number of times to retry failed virus scans - AV_SCAN_MAX_RETRIES = os.getenv('AV_SCAN_MAX_RETRIES', 5) + AV_SCAN_MAX_RETRIES = int(os.getenv('AV_SCAN_MAX_RETRIES', 5)) # The number of seconds to wait for socket response from clamav-rest - AV_SCAN_TIMEOUT = os.getenv('AV_SCAN_TIMEOUT', 30) + AV_SCAN_TIMEOUT = int(os.getenv('AV_SCAN_TIMEOUT', 30)) s3_src = "s3-us-gov-west-1.amazonaws.com" From 92cf09e4a1d300dd597094ccbc1d71a3c3591ac7 Mon Sep 17 00:00:00 2001 From: John Willis Date: Fri, 24 Sep 2021 10:01:50 -0400 Subject: [PATCH 12/12] Update tdrs-backend/tdpservice/security/models.py Co-authored-by: Alex P. <63075587+ADPennington@users.noreply.github.com> --- tdrs-backend/tdpservice/security/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tdrs-backend/tdpservice/security/models.py b/tdrs-backend/tdpservice/security/models.py index 0451226164..5dec7b6ad7 100644 --- a/tdrs-backend/tdpservice/security/models.py +++ b/tdrs-backend/tdpservice/security/models.py @@ -129,7 +129,7 @@ class Result(models.TextChoices): data_file = models.ForeignKey( DataFile, blank=True, - help_text='The resulting DataFile object if this scan was clean', + help_text='The resulting DataFile object, if this scan was clean', null=True, on_delete=models.SET_NULL, related_name='av_scans'