Fixed #21219 -- Added STATIC_FILE_PERMISSIONS setting. #1747

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

vajrasky commented Oct 15, 2013

Now STATIC_FILE_PERMISSIONS is used in the
django.contrib.staticfiles.storage.StaticFilesStorage backend, instead of
FILE_UPLOAD_PERMISSIONS. Added test and documentation as well.

@vajrasky vajrasky Fixed #21219 -- Added STATIC_FILE_PERMISSIONS setting.
Now STATIC_FILE_PERMISSIONS is used in the
django.contrib.staticfiles.storage.StaticFilesStorage backend, instead of
FILE_UPLOAD_PERMISSIONS. Added test and documentation as well.
6875bc1

@timgraham timgraham commented on an outdated diff Oct 15, 2013

docs/releases/1.7.txt
@@ -337,6 +337,12 @@ Signals
* The ``enter`` argument was added to the
:data:`~django.test.signals.setting_changed` signal.
+Static File
@timgraham

timgraham Oct 15, 2013

Owner

should be django.contrib.staticfiles for consistency with the other contrib headings

@timgraham timgraham commented on the diff Oct 15, 2013

django/core/files/storage.py
@@ -171,7 +172,7 @@ def _save(self, name, content):
try:
if settings.FILE_UPLOAD_DIRECTORY_PERMISSIONS is not None:
@timgraham

timgraham Oct 15, 2013

Owner

do you it makes sense to use FILE_UPLOAD_DIRECTORY_PERMISSIONS here or should we have another setting specific to static files?

@vajrasky

vajrasky Oct 15, 2013

Contributor

For fine-tuning, it is better to have a separate STATIC_FILE_DIRECTORY_PERMISSIONS. I omitted it because of practicality reason. I try to minimize the code changes for this ticket. Maybe a separate ticket for that setting? The FILE_UPLOAD_DIRECTORY_PERMISSIONS setting was added later than FILE_UPLOAD_PERMISSIONS setting anyway.

@timgraham timgraham and 1 other commented on an outdated diff Oct 15, 2013

django/core/files/storage.py
@@ -232,7 +233,9 @@ def _save(self, name, content):
# OK, the file save worked. Break out of the loop.
break
- if settings.FILE_UPLOAD_PERMISSIONS is not None:
+ if self.permissions_mode is not None:
+ os.chmod(full_path, self.permissions_mode)
@timgraham

timgraham Oct 15, 2013

Owner

I suggest having just 1 os.chmod call:

os.chmod(full_path, self.permissions_mode if self.permissions_mode is not None else settings.FILE_UPLOAD_PERMISSIONS)
@vajrasky

vajrasky Oct 15, 2013

Contributor
  1. The line is too long (more than 80 chars) and it is still ugly to break this too long lines with "\n".
  2. The meaning of that line differs. If FILE_UPLOAD_PERMISSIONS and STATIC_FILE_PERMISSIONS both are empty (None), then we are in trouble.
    os.chmod(full_path, None) -> error.

What about this:

        if self.permissions_mode is not None:
            mode = self.permissions_mode
        else:
            mode = settings.FILE_UPLOAD_PERMISSIONS
        if mode:
            os.chmod(full_path, mode)

@timgraham timgraham commented on the diff Oct 15, 2013

docs/ref/settings.txt
@@ -2678,6 +2678,27 @@ setting.
Static file finders are currently considered a private interface, and this
interface is thus undocumented.
+STATIC_FILE_PERMISSIONS
@timgraham

timgraham Oct 15, 2013

Owner

needs: .. setting:: STATIC_FILE_PERMISSIONS so that links are properly created

@timgraham timgraham commented on an outdated diff Oct 15, 2013

docs/ref/contrib/staticfiles.txt
@@ -322,6 +323,14 @@ files:
.. _staticfiles-development-view:
+Permissions
+===========
+
+When running :djadmin:`collectstatic` management command, the newly-collected
@timgraham

timgraham Oct 15, 2013

Owner

.. versionadded:: 1.7

@timgraham timgraham commented on an outdated diff Oct 15, 2013

docs/ref/settings.txt
@@ -2678,6 +2678,27 @@ setting.
Static file finders are currently considered a private interface, and this
interface is thus undocumented.
+STATIC_FILE_PERMISSIONS
+-----------------------
+
+Default: ``None``
@timgraham

timgraham Oct 15, 2013

Owner

.. versionadded:: 1.7

@timgraham timgraham commented on an outdated diff Oct 15, 2013

docs/ref/settings.txt
@@ -2678,6 +2678,27 @@ setting.
Static file finders are currently considered a private interface, and this
interface is thus undocumented.
+STATIC_FILE_PERMISSIONS
+-----------------------
+
+Default: ``None``
+
+The numeric mode (i.e. ``0644``) to set newly-collected static files to. For
+more information about what these modes mean, see the documentation for
+:func:`os.chmod`.
+
+If this isn't given or is ``None``, you'll get FILE_UPLOAD_PERMISSIONS
@timgraham

timgraham Oct 15, 2013

Owner

:setting:FILE_UPLOAD_PERMISSIONS

rather than "you'll get ..." I'd say "FILE_UPLOAD_PERMISSIONS will be used instead."

@vajrasky vajrasky Made changes asked in review by Tim Graham.
- Fixed an English sentence.
- Added addedversion 1.7.
- Added link to static contrib.
- Changed header.
- Removed extra line of os.chmod.

Test mode for None specifically in case someone is giving permission 0o000.
50dcee4

vajrasky closed this Oct 19, 2013

vajrasky deleted the vajrasky:ticket_21219 branch Oct 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment