-
Notifications
You must be signed in to change notification settings - Fork 193
Fixed crash when auditing on binary data #202
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
base: master
Are you sure you want to change the base?
Changes from all commits
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,7 +1,5 @@ | ||||||
from __future__ import unicode_literals | ||||||
|
||||||
from uuid import UUID | ||||||
|
||||||
from django.conf import settings | ||||||
from django.core.exceptions import ObjectDoesNotExist | ||||||
from django.db.models import NOT_PROVIDED, DateTimeField | ||||||
|
@@ -19,15 +17,21 @@ def get_field_value(obj, field): | |||||
:return: The value of the field as a string. | ||||||
:rtype: str | ||||||
""" | ||||||
raw_value = getattr(obj, field.name, None) | ||||||
if isinstance(field, DateTimeField): | ||||||
# DateTimeFields are timezone-aware, so we need to convert the field | ||||||
# to its naive form before we can accurately compare them for changes. | ||||||
try: | ||||||
value = field.to_python(getattr(obj, field.name, None)) | ||||||
value = field.to_python(raw_value) | ||||||
if value is not None and settings.USE_TZ and not timezone.is_naive(value): | ||||||
value = timezone.make_naive(value, timezone=timezone.utc) | ||||||
except ObjectDoesNotExist: | ||||||
value = field.default if field.default is not NOT_PROVIDED else None | ||||||
elif isinstance(raw_value, bytes): | ||||||
if len(raw_value) > 100: | ||||||
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'm not saying this is a magic number, but it would be helpful to have a comment regarding this (and possibly make it a variable since it is referenced multiple times in this function scope). 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'm actually quite confused why this value is being truncated, and why at 100 bytes. What is this doing? This PR doesn't link an issue explaining the problem. 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. It's being truncated because the audit log can't by default store a full copy of binary files that can be gigabytes in size. 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. Would it be worth (possible even?) storing a hash of the binary data instead of the first 100 bytes? 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. Hmm.. that's an idea for sure. The advantage is that you can actually check if some data is the exact data. The downsides are that you don't get something immediately useful in the log, and that if you don't have the binary data you want to compare to anymore then the hash is useless. 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 would worry about the speed of hashing very large binary data. Say for a 1GB file, I'd expect at least 5 seconds for a hash to be generated if the implementation was purely C. |
||||||
return repr(raw_value[:100]) + '[truncated {} bytes]'.format(len(raw_value) - 100) | ||||||
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.
Suggested change
|
||||||
else: | ||||||
return repr(raw_value) | ||||||
else: | ||||||
try: | ||||||
value = smart_str(getattr(obj, field.name, None)) | ||||||
|
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.
well placed!