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

S3 credentials in links missing for private buckets after upgrade to Django 5.1 #1443

Closed
GitRon opened this issue Aug 15, 2024 · 9 comments
Closed

Comments

@GitRon
Copy link

GitRon commented Aug 15, 2024

Hi @jschneier!

I found an issue with Django 5.1. After the upgrade (using django-storages 1.14.4), all GET-parameter credentials for private S3 buckets are not being added to the links in my templates anymore. This means that I don't have access to the files.

I have no idea and since I have little knowledge on how this amazing package works, I can't really contribute any suggestions. 😔 I just verified that it's really Django 5.1.

Here's my setup, it might help in reconstructing the case.

from django.conf import settings
from storages.backends.s3boto3 import S3Boto3Storage


class PrivateMediaStorage(S3Boto3Storage):
    location = settings.AWS_PRIVATE_MEDIA_LOCATION
    querystring_expire = 3600  # seconds until the generated link expires
    default_acl = "bucket-owner-full-control"
    file_overwrite = False
    custom_domain = False

Thanks so much for looking into this!
Ronny

PS: It seens unrelated to #1437 since there, Django 4.2 was used.

@christoph-teichmeister
Copy link

christoph-teichmeister commented Aug 16, 2024

I've also run in this issue 😞

@kylepollina
Copy link

kylepollina commented Aug 21, 2024

5.1 has also caused issues with AWS_S3_CUSTOM_DOMAIN. With django 5.0 + django rest, when desearializing a model with an image/file field, the key was prepended with whatever value was set for AWS_S3_CUSTOM_DOMAIN, however after the upgrade it seems to default to the hostname.

For example, if I set AWS_S3_CUSTOM_DOMAIN in my settings.py file to be www.test.com

Screenshot 2024-08-20 at 9 58 28 PM

on django==5.0 imagefield deserialization returns http://www.test.com/organizations/0a72d97f-974c-4828-85b1-0276a6c6cf2e/images/m22_ms.png

Screenshot 2024-08-20 at 9 56 52 PM

But on django==5.1 imagefield deserialization returns http://localhost:8000/organizations/0a72d97f-974c-4828-85b1-0276a6c6cf2e/images/m22_ms.png

Screenshot 2024-08-20 at 9 57 22 PM

@kylepollina
Copy link

kylepollina commented Aug 21, 2024

Upon further inspection it looks like the newer version of django is overwriting parts of the storage backend. I was debugging a bit and found that the variable stored here:

https://github.com/django/django/blob/main/django/db/models/fields/files.py#L25

This storage object seems to be the same type, but storage.url used to be pointing to the url function for the s3 storage backend, but after upgrading, it pointed to the url function for the default django filestorage backend. However I couldn't backtrack to see how or where that value is being set. It gets pretty muddled

@rory-ferguson
Copy link

Hi @kylepollina, You might of missed the django-storages config change

https://django-storages.readthedocs.io/en/latest/backends/amazon-S3.html#configuration-settings

This changed for django 4.2

@GitRon
Copy link
Author

GitRon commented Aug 21, 2024

@rory-ferguson Thanks for the investigation, we didn't use the new STORAGE yet. I'll try it out and report back.

@kylepollina
Copy link

@rory-ferguson Thank you! I updated my configurations and this fixed my issue.

@GitRon
Copy link
Author

GitRon commented Aug 22, 2024

Seems to work here, too. Should this be documented somewhere? 🤔

@kylepollina
Copy link

@GitRon it is documented on the bottom of the Django 5.1 release notes. https://docs.djangoproject.com/en/5.1/releases/5.1/

The DEFAULT_FILE_STORAGE and STATICFILES_STORAGE settings is removed.

@GitRon
Copy link
Author

GitRon commented Aug 23, 2024

@kylepollina You are right! Still don't get it, why Django works with the old settings 😆 Well, maybe really not a storages-problem 🤷‍♂️ On the other side, explicit is better than implicit. I'll leave it to the maintainers to decide this.

Thanks for the support everyone ❤️

@jschneier jschneier closed this as not planned Won't fix, can't repro, duplicate, stale Aug 26, 2024
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

No branches or pull requests

5 participants