Opened 6 years ago

Closed 3 years ago

#13518 closed New feature (fixed)


Reported by: jacob@… Owned by: nobody
Component: File uploads/storage Version: 1.1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


In core/files/, FILE_UPLOAD_PERMISSIONS is used to set the permissions of created files. It also makes the containing directory if it doesn't exist, but doesn't set the permissions for this directory. One possible patch is attached.

I can imagine cases where one would want the directory permissions to be different from file permissions. I'm not sure how objectional new setting names are.


Attachments (1)

storage_perms.patch (570 bytes) - added by jacob@… 6 years ago.

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by jacob@…

comment:1 Changed 6 years ago by russellm

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Good suggestion, but:

  1. Patch should be generated as a diff against the root of the source tree
  2. Patch requires tests
  3. Patch probably also needs a note added to documentation describing the permissions that new directories will be created with

As for introducing a new FILE_UPLOAD_DIRECTORY_PERMISSIONS setting; while we generally try to avoid introducing new settings, in this case it makes sense to me. However, this also means extra documentation for point (3).

comment:2 Changed 5 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:3 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:4 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:5 Changed 3 years ago by Raumkraut

The lack of this feature hit me a few days ago, so I've worked up a fix, taking into account the given suggestions.

The commit in question:

Since this is my first Django commit, and the ticket is somewhat old, there are some things I'm not certain are "correct":

  • The documentation sections I've added defer explanation of the intricacies of file permissions to the existing FILE_UPLOAD_PERMISSIONS entries, in preference to duplicating the information.
  • Since os.makedirs accounts for the system umask, and os.chmod doesn't, I've added code to temporarily zero out the umask while the directories are created. The intent is to maintain consistency of behaviour between the two settings.

Are these acceptable implementations, or do I need to tweak further before I send out a pull request?

comment:6 Changed 3 years ago by Raumkraut

I've gone ahead and created a pull request with my patch:
Hopefully someone can take a look at it?

comment:7 Changed 3 years ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Summary changed from FILE_UPLOAD_PERMISSIONS used for created files, but not created directories to Add settings.FILE_UPLOAD_DIRECTORY_PERMISSIONS

Comments on PR.

comment:8 Changed 3 years ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 6bdb3b1135d1bd7b2dc24131b9d26ac19ebdba67:


This setting does for new directories what FILE_UPLOAD_PERMISSIONS
does for new files.

Thanks jacob@ for the suggestion.

Note: See TracTickets for help on using tickets.
Back to Top