[Django] #21219: The FILE_UPLOAD_PERMISSIONS should not be used when deploying static files
Django
--------------------------------------+--------------------
Reporter: dblack+django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
The FILE_UPLOAD_PERMISSIONS permission is used in the
django.contrib.staticfiles.storage.staticfiles_storage backend *and* it
really shouldn't be. The use of the FILE_UPLOAD_PERMISSIONS permission to
govern the deployed permission of static files is not documented so it
should be possible to make a new STATIC_FILE_PERMISSIONS setting which is
used instead of FILE_UPLOAD_PERMISSIONS during collectstatic :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/21219>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: dblack+django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* component: File uploads/storage => contrib.staticfiles
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:1>
Django
Reporter: dblack+django@… | Owner: nobody
Type: New feature | Status: new
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* has_patch: 0 => 1
* type: Cleanup/optimization => New feature
Comment:
@vajrasky - any chance you can make a pull request on github so we can
more easily comment on the patch?
Also don't forget to tick "Has patch" so the ticket shows up for review.
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:2>
Django
Reporter: dblack+django@… | Owner: nobody
Type: New feature | Status: new
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vajrasky):
$ git push origin ticket_21219
error: The requested URL returned error: 403 Forbidden while accessing
https://vajr...@github.com/django/django.git/info/refs?service=git-
receive-pack
fatal: HTTP request failed
@timo, I am working on it. Probably it takes longer time since I need to
learn how to solve this problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:3>
Django
Reporter: dblack+django@… | Owner: nobody
Type: New feature | Status: new
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vajrasky):
Apparently I did not fork the django repo first from my github account.
Now I managed to make a pull request!
https://github.com/django/django/pull/1747
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:4>
Django
Reporter: dblack+django@… | Owner: nobody
Type: New feature | Status: new
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
Rather than adding a setting for this (we are trying to avoid settings),
should we instead suggest subclassing the `STATICFILES_STORAGE` class and
overriding the new `permissions_mode` attribute this patch offers?
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:5>
Django
Reporter: dblack+django@… | Owner: vajrasky
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by vajrasky):
* status: new => assigned
* owner: nobody => vajrasky
Comment:
Let me get this straight.
So FileSystemStorage has self.file_permissions_mode and
self.directory_permissions_mode. Their values come from
settings.FILE_UPLOAD_PERMISSIONS and
settings.FILE_UPLOAD_DIRECTORY_PERMISSIONS.
For StaticFilesStorage, the situation is same. But if you dislike the
permissions, all you have to do is...
{{{
class CustomStaticFilesStorage(StaticFilesStorage):
def __init__(self, location=None, base_url=None, *args, **kwargs):
self.file_permissions_mode = 0o644
self.directory_permissions_mode = 0o600
super(CustomStaticFilesStorage, self).__init__(location, base_url,
*args, **kwargs)
settings.STATICFILES_STORAGE = CustomStaticFilesStorage()
}}}
Is that what is in your mind?
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:6>
Django
Reporter: dblack+django@… | Owner: vajrasky
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
Correct, although I would expect the subclass to simply look like this:
{{{
class CustomStaticFilesStorage(StaticFilesStorage):
file_permissions_mode = 0o644
directory_permissions_mode = 0o600
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:7>
Django
Reporter: dblack+django@… | Owner: vajrasky
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vajrasky):
This is the new pull request. https://github.com/django/django/pull/1779
Some notes:
1. There is no way to set permission to the directories of static files
either with FILE_UPLOAD_PERMISSIONS and FILE_DIRECTORY_UPLOAD_PERMISSIONS.
Therefore I can not override the directories permission by subclassing
StaticFilesStorage without a lot of works. I prefer to create a separate
ticket for this bug. One step at at time.
2. At first, I want to add file_permissions_mode class attribute in
Storage or FileSystemStorage class and give the default value of
FILE_UPLOAD_PERMISSIONS to this class attribute. However some tests depend
on the behaviour of FileSystemStorage checking the value of
FILE_UPLOAD_PERMISSIONS at the *last minute* before setting the
permissions to uploaded files. So from engineering perspective, this
behaviour is a little bit inconsistent with the StaticFilesStorage. But
there is nothing I can do.
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:8>
Django
Reporter: dblack+django@… | Owner: vajrasky
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
Actually in receiving some feedback on another ticket, I think it would be
better to use an instance attribute. I've sent an
[https://github.com/django/django/pull/1803 updated pull request]. Please
take a look and let me know if it looks good.
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:9>
Django
Reporter: dblack+django@… | Owner: vajrasky
Type: New feature | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vajrasky):
Aside from 0o000 permission issue, LGTM.
Actually before the fix {{{file_permissions_mode if file_permissions_mode
is not None else settings.FILE_UPLOAD_PERMISSIONS}}}, the PR breaks
file_upload test. Now it works again.
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:10>
Django
Reporter: dblack+django@… | Owner: vajrasky
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"9eecb9169566db263e243e4522b08ea1403ee95f"]:
{{{
#!CommitTicketReference repository=""
revision="9eecb9169566db263e243e4522b08ea1403ee95f"
Fixed #21219 -- Added a way to set different permission for static files.
Previously, when collecting static files, the files would receive
permission
from FILE_UPLOAD_PERMISSIONS. Now, there's an option to give different
permission from uploaded files permission by subclassing any of the static
files storage classes and setting the file_permissions_mode parameter.
Thanks dblack at atlassian.com for the suggestion.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21219#comment:11>