Skip to content
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

Attachments support #134

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

izimobil
Copy link
Contributor

Now django-messages support attachments when composing or replying.

Attachments are stored by default in an "attachments" folder relative to the MEDIA_ROOT, but this can be changed with the new DJANGO_MESSAGES_UPLOAD_TO setting.

Fixes #109.

When not using the Django sites framework, you get a RuntimeError
because it is not installed, even if you set DJANGO_MESSAGES_NOTIFY to
False.

This can be avoided by importing the site model only when necessary.
Now django-messages support attachments when composing or replying.
Attachments are stored by default in an "attachments" folder relative to
the ``MEDIA_ROOT``, but this can be changed with the new
``DJANGO_MESSAGES_UPLOAD_TO`` setting.
@arneb
Copy link
Owner

arneb commented Sep 23, 2019

Thanks for the contribution! I like the idea.

I will need to test this a bit before merging though ...

The one thing that bothers me currently is how the attachments are stored. I'm not sure if a simple folder in MEDIA_ROOT is the best choice. This approach has at least some privacy-issues...

It might be a good idea to have a configurable storage-engine for the attachments. With that it would be possible to store attachments outside of MEDIA_ROOT, so they are not publicly available to everyone who knows the url. The default could still be MEDIA_ROOT, but we should provide a mechanism, so that it can be changed on a per-project basis.

If stored in MEDIA_ROOT we should at least do something with the filename/path. Maybe store in a folder named with the md5 hash of file contents or so...
Otherwise, if two people upload a file with the same name, the later one will be renamed, which leaks the information to the user, that another file with the original name already exists and he can then access it, because he knows the url.

Example:
User 1 uploads "test.txt". It will be available at MEDIA_ROOT/attachments/test.txt.
Then User 2 uploads "test.txt" (different content). It will be renamed and be available at MEDIA_ROOT/attachments/test_sXcftg.txt.
User 2 now knows (by looking at the URL of his uploaded attachment in sent-messages), that a different file with URL MEDIA_ROOT/attachments/test.txt must exist and he can then open it by url, even though it was never idented for him.

@izimobil
Copy link
Contributor Author

Hi,

You are totally right !

(note that the attachment code will still use the configured DEFAULT_FILE_STORAGE regardless of the upload_to argument).

What we could do at best IMO:

  1. Poor's man securing of the storage directory: MEDIA_ROOT/attachments/<some hash with sender/recipients ids and SECRET_KEY>/test.txt. This could be optional with a DJANGO_MESSAGES_SECURE_UPLOAD_DIR = False

  2. Add a note in the documentation about security

  3. Allow the user to pass a specific storage backend to the attachment.file FileField, eg:

def get_storage_backend():
    backend = getattr(settings, 'DJANGO_MESSAGES_STORAGE_BACKEND', None)
    if backend is None:
        return None
    # build backend from string here, not sure if we should handle backend args/kwargs ?
    return backend

@python_2_unicode_compatible
class Attachment(models.Model):
    """
    A message attachment. You can configure where attachments are stored with
    the ``DJANGO_MESSAGES_UPLOAD_TO`` setting, note that this setting is
    relative to your ``MEDIA_ROOT`` setting.
    """
    file = models.FileField(
        _('File'),
        upload_to=getattr(settings, 'DJANGO_MESSAGES_UPLOAD_TO', 'attachments'),
        storage=get_storage_backend(),
    )
    message = models.ForeignKey(
        Message,
        on_delete=models.CASCADE,
        verbose_name=_('Message'),
        related_name='attachments'
    )

    class Meta:
        verbose_name = _("Attachment")

What do you think ?

@arneb
Copy link
Owner

arneb commented Sep 23, 2019

I opt for the third solution: allow a custom storage backend.

The default django filesystem storage being not very secure, and thus
not appropriate for private messages attachments, now one can pass a
custom storage backend.

Also added a note on security in the relevant documentation.
@izimobil
Copy link
Contributor Author

Done !

@arneb
Copy link
Owner

arneb commented Sep 23, 2019

Looks good, thank you! I still would like to test the changes in a little example project on my dev machine. So please give me some time before it's getting merged.

@izimobil
Copy link
Contributor Author

Of course, no hurry :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

file and picture sending support?
2 participants