Warn about upcoming change to AWS_DEFAULT_ACL; allow None in AWS_DEFAULT_ACL #535
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If bucket already exists, do not try to override permissions of objects. same for bucket creation, let defaults configured on AWS takes precedence.
46ab21e to
cf3f374
Compare
|
well done |
|
@sww314, just a nudge -- is there anything else I can do to assist with getting this merged? Thanks! |
|
@jcushman Looks ok to me, but I am not actively using the AWS storages, so I would like someone that uses the boto/boto3 to chime in. |
|
Thanks so much. I've come to the conclusion that my decision earlier on this was wrong and it should have been addressed immediately. Going to merge and cut 2.0 with this fixed on Friday. For some of the other changes I want to make we can wait to 3.0 which can always come quicker. |
|
Should we also be warning on the s3boto and gs backends? I know those are legacy but I'm not going to remove them in 1.6.7 |
cf3f374 to
6ba7202
Compare
bowans
added a commit
to bowans/tsyc-django
that referenced
this pull request
Sep 20, 2018
- Refer to jschneier/django-storages#535 - Set AWS_DEFAULT_ACL = None
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
As discussed in #381, the default setting of "public-read" for
AWS_DEFAULT_ACLis insecure -- if a user sets up a bucket that is private by default, they should have to explicitly opt in for django-storages to override that choice and upload globally readable files. By default uploads should use the bucket's settings.@jschneier suggested in that bug that the change should be made in django-storages 2.0, since it would be a breaking change for many users.
@robatwave commented that one can currently opt into the proposed behavior by setting
AWS_DEFAULT_ACL=Noneexplicitly. This does not work for me; with that setting I get'NoneType' object has no attribute 'parts'inS3Boto3StorageFile.close.@ticosax sent a pull request last year, #429, that implements the breaking change but currently has some tests failing.
It would be great to take action on this without waiting for the decision to ship django-storages 2.0. It's likely that lots of users are currently using django-storages in a way that unintentionally leaks data -- it's not something where it's harmless to wait.
This PR
This is a change that can be made immediately without waiting for 2.0. It gets
AWS_DEFAULT_ACL=Noneworking, and adds a warning that the default behavior will be changing in 2.0.This PR is based on the work @ticosax did and includes 3 commits:
Noneand tweaking a test. This getsAWS_DEFAULT_ACL=Noneworking for me.AWS_DEFAULT_ACLsetting. The warning includes instructions for how (and why) a user can either explicitly retain the existing behavior if it's actually what they want, withAWS_DEFAULT_ACL="public-read", or prepare for 2.0 by opting into the new behavior withAWS_DEFAULT_ACL=None.