-
-
Notifications
You must be signed in to change notification settings - Fork 768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Single table for file attachments #7420
Changes from all commits
0bb25b2
3fb7d18
ae5c38c
79d5bc1
9e34b67
5250508
8031a49
e50ab6e
a4f4280
4ca4638
66dcee4
3cc7dd0
b589ef0
353f1dc
04e978e
1f6d4b0
def177d
ee59ca3
6c8f36b
f390780
8f3414f
5afc259
548dd0d
78da986
0316241
5ebb9dd
b6f5398
d193945
bf82144
a3460c3
8127ba2
daa5249
6bdf372
7c4c004
3b8aec5
35e6aad
a426162
98f8c77
d2c0eae
9f09512
add38f4
ddbffd8
241ec4d
a0cccaf
471f17a
0c27705
af79d26
8e593fa
89336fe
6b748fe
a587c85
4803751
1788f96
a69713a
2389d70
5a6f9f5
ac1acb0
0d73fbb
9064e49
5517b34
2decad3
80a93ea
1c615a0
eb2716e
a46374f
f28f24a
c621b94
a89c2dc
e882bf4
c8fb905
e3f34f6
1c73b70
bc4d219
80098cc
b833da1
4e30078
e3f22ef
5450e73
c6c06d6
f93d898
da258d9
e164cd3
80f06ad
e09394d
0f867dd
e1c424c
b4767e9
3d31689
b713d3d
28d5ade
a17caa6
18e86ca
7dc5403
90bad53
fda2381
c94d901
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,7 @@ | ||
"""Generic models which provide extra functionality over base Django model types.""" | ||
|
||
import logging | ||
import os | ||
from datetime import datetime | ||
from io import BytesIO | ||
|
||
from django.conf import settings | ||
from django.contrib.auth import get_user_model | ||
|
@@ -20,11 +18,11 @@ | |
from mptt.exceptions import InvalidMove | ||
from mptt.models import MPTTModel, TreeForeignKey | ||
|
||
import common.settings | ||
import InvenTree.fields | ||
import InvenTree.format | ||
import InvenTree.helpers | ||
import InvenTree.helpers_model | ||
from InvenTree.sanitizer import sanitize_svg | ||
|
||
logger = logging.getLogger('inventree') | ||
|
||
|
@@ -304,10 +302,7 @@ def get_reference_pattern(cls): | |
if cls.REFERENCE_PATTERN_SETTING is None: | ||
return '' | ||
|
||
# import at function level to prevent cyclic imports | ||
from common.models import InvenTreeSetting | ||
|
||
return InvenTreeSetting.get_setting( | ||
return common.settings.get_global_setting( | ||
cls.REFERENCE_PATTERN_SETTING, create=False | ||
).strip() | ||
|
||
|
@@ -503,200 +498,64 @@ class Meta: | |
abstract = True | ||
|
||
|
||
def rename_attachment(instance, filename): | ||
"""Function for renaming an attachment file. The subdirectory for the uploaded file is determined by the implementing class. | ||
|
||
Args: | ||
instance: Instance of a PartAttachment object | ||
filename: name of uploaded file | ||
|
||
Returns: | ||
path to store file, format: '<subdir>/<id>/filename' | ||
""" | ||
# Construct a path to store a file attachment for a given model type | ||
return os.path.join(instance.getSubdir(), filename) | ||
|
||
|
||
class InvenTreeAttachment(InvenTreeModel): | ||
class InvenTreeAttachmentMixin: | ||
"""Provides an abstracted class for managing file attachments. | ||
|
||
An attachment can be either an uploaded file, or an external URL | ||
Links the implementing model to the common.models.Attachment table, | ||
and provides the following methods: | ||
|
||
Attributes: | ||
attachment: Upload file | ||
link: External URL | ||
comment: String descriptor for the attachment | ||
user: User associated with file upload | ||
upload_date: Date the file was uploaded | ||
- attachments: Return a queryset containing all attachments for this model | ||
""" | ||
|
||
class Meta: | ||
"""Metaclass options. Abstract ensures no database table is created.""" | ||
|
||
abstract = True | ||
|
||
def getSubdir(self): | ||
"""Return the subdirectory under which attachments should be stored. | ||
def delete(self): | ||
"""Handle the deletion of a model instance. | ||
|
||
Note: Re-implement this for each subclass of InvenTreeAttachment | ||
Before deleting the model instance, delete any associated attachments. | ||
""" | ||
return 'attachments' | ||
|
||
def save(self, *args, **kwargs): | ||
"""Provide better validation error.""" | ||
# Either 'attachment' or 'link' must be specified! | ||
if not self.attachment and not self.link: | ||
raise ValidationError({ | ||
'attachment': _('Missing file'), | ||
'link': _('Missing external link'), | ||
}) | ||
|
||
if self.attachment and self.attachment.name.lower().endswith('.svg'): | ||
self.attachment.file.file = self.clean_svg(self.attachment) | ||
|
||
super().save(*args, **kwargs) | ||
|
||
def clean_svg(self, field): | ||
"""Sanitize SVG file before saving.""" | ||
cleaned = sanitize_svg(field.file.read()) | ||
return BytesIO(bytes(cleaned, 'utf8')) | ||
|
||
def __str__(self): | ||
"""Human name for attachment.""" | ||
if self.attachment is not None: | ||
return os.path.basename(self.attachment.name) | ||
return str(self.link) | ||
|
||
attachment = models.FileField( | ||
upload_to=rename_attachment, | ||
verbose_name=_('Attachment'), | ||
help_text=_('Select file to attach'), | ||
blank=True, | ||
null=True, | ||
) | ||
|
||
link = InvenTree.fields.InvenTreeURLField( | ||
blank=True, | ||
null=True, | ||
verbose_name=_('Link'), | ||
help_text=_('Link to external URL'), | ||
) | ||
|
||
comment = models.CharField( | ||
blank=True, | ||
max_length=100, | ||
verbose_name=_('Comment'), | ||
help_text=_('File comment'), | ||
) | ||
|
||
user = models.ForeignKey( | ||
User, | ||
on_delete=models.SET_NULL, | ||
blank=True, | ||
null=True, | ||
verbose_name=_('User'), | ||
help_text=_('User'), | ||
) | ||
|
||
upload_date = models.DateField( | ||
auto_now_add=True, null=True, blank=True, verbose_name=_('upload date') | ||
) | ||
self.attachments.all().delete() | ||
super().delete() | ||
|
||
@property | ||
def basename(self): | ||
"""Base name/path for attachment.""" | ||
if self.attachment: | ||
return os.path.basename(self.attachment.name) | ||
return None | ||
|
||
@basename.setter | ||
def basename(self, fn): | ||
"""Function to rename the attachment file. | ||
|
||
- Filename cannot be empty | ||
- Filename cannot contain illegal characters | ||
- Filename must specify an extension | ||
- Filename cannot match an existing file | ||
""" | ||
fn = fn.strip() | ||
|
||
if len(fn) == 0: | ||
raise ValidationError(_('Filename must not be empty')) | ||
def attachments(self): | ||
"""Return a queryset containing all attachments for this model.""" | ||
return self.attachments_for_model().filter(model_id=self.pk) | ||
|
||
attachment_dir = settings.MEDIA_ROOT.joinpath(self.getSubdir()) | ||
old_file = settings.MEDIA_ROOT.joinpath(self.attachment.name) | ||
new_file = settings.MEDIA_ROOT.joinpath(self.getSubdir(), fn).resolve() | ||
@classmethod | ||
def check_attachment_permission(cls, permission, user) -> bool: | ||
"""Check if the user has permission to perform the specified action on the attachment. | ||
|
||
# Check that there are no directory tricks going on... | ||
if new_file.parent != attachment_dir: | ||
logger.error( | ||
"Attempted to rename attachment outside valid directory: '%s'", new_file | ||
) | ||
raise ValidationError(_('Invalid attachment directory')) | ||
The default implementation runs a permission check against *this* model class, | ||
but this can be overridden in the implementing class if required. | ||
|
||
# Ignore further checks if the filename is not actually being renamed | ||
if new_file == old_file: | ||
return | ||
Arguments: | ||
permission: The permission to check (add / change / view / delete) | ||
user: The user to check against | ||
|
||
forbidden = [ | ||
"'", | ||
'"', | ||
'#', | ||
'@', | ||
'!', | ||
'&', | ||
'^', | ||
'<', | ||
'>', | ||
':', | ||
';', | ||
'/', | ||
'\\', | ||
'|', | ||
'?', | ||
'*', | ||
'%', | ||
'~', | ||
'`', | ||
] | ||
Comment on lines
-641
to
-661
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: @SchrodingersGat where was this security feature moved? I am not able to find it in the diff There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are largely covered by django's built in file name validators. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure/have you verified this? We had a bunch of vulns in this specific area in the past with length overflows and target directory manipulation and now that huntr is basically dead we wont get a free security audit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add some more checking in to cover this. I think the the existing code is never called anyway - it is certainly not hit in our coverage report. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We did not add checks when we fixed the original issues as there was some sever time pressure. I will try to add unit tests for unsafe uploads to my todo |
||
|
||
for c in forbidden: | ||
if c in fn: | ||
raise ValidationError(_(f"Filename contains illegal character '{c}'")) | ||
|
||
if len(fn.split('.')) < 2: | ||
raise ValidationError(_('Filename missing extension')) | ||
|
||
if not old_file.exists(): | ||
logger.error( | ||
"Trying to rename attachment '%s' which does not exist", old_file | ||
) | ||
return | ||
Returns: | ||
bool: True if the user has permission, False otherwise | ||
""" | ||
perm = f'{cls._meta.app_label}.{permission}_{cls._meta.model_name}' | ||
return user.has_perm(perm) | ||
|
||
if new_file.exists(): | ||
raise ValidationError(_('Attachment with this filename already exists')) | ||
def attachments_for_model(self): | ||
"""Return all attachments for this model class.""" | ||
from common.models import Attachment | ||
|
||
try: | ||
os.rename(old_file, new_file) | ||
self.attachment.name = os.path.join(self.getSubdir(), fn) | ||
self.save() | ||
except Exception: | ||
raise ValidationError(_('Error renaming file')) | ||
model_type = self.__class__.__name__.lower() | ||
|
||
def fully_qualified_url(self): | ||
"""Return a 'fully qualified' URL for this attachment. | ||
return Attachment.objects.filter(model_type=model_type) | ||
|
||
- If the attachment is a link to an external resource, return the link | ||
- If the attachment is an uploaded file, return the fully qualified media URL | ||
""" | ||
if self.link: | ||
return self.link | ||
def create_attachment(self, attachment=None, link=None, comment='', **kwargs): | ||
"""Create an attachment / link for this model.""" | ||
from common.models import Attachment | ||
|
||
if self.attachment: | ||
media_url = InvenTree.helpers.getMediaUrl(self.attachment.url) | ||
return InvenTree.helpers_model.construct_absolute_url(media_url) | ||
kwargs['attachment'] = attachment | ||
kwargs['link'] = link | ||
kwargs['comment'] = comment | ||
kwargs['model_type'] = self.__class__.__name__.lower() | ||
kwargs['model_id'] = self.pk | ||
|
||
return '' | ||
Attachment.objects.create(**kwargs) | ||
|
||
|
||
class InvenTreeTree(MetadataMixin, PluginValidationMixin, MPTTModel): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Why is this changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value filters through from the definition of the setting itself, and does not need to be passed here.