Opened 17 years ago
Closed 3 years ago
#4282 closed Bug (fixed)
startproject should honor umask
Reported by: | Owned by: | Ad Timmering | |
---|---|---|---|
Component: | Core (Management commands) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Ad Timmering, Claude Paroz | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Ticket #1651 fixed the problem of manage.py not being made executable by copying *all* the permission bits (not just the executable flags). This means that the user's umask doesn't work, e.g.:
$ umask 077 $ touch foo $ ls -l foo -rw------- 1 talex talex 0 2007-05-12 13:27 foo $ PYTHONPATH=trunk ./trunk/django/bin/django-admin.py startproject mysite $ ls -l mysite/settings.py -rw-r--r-- 1 talex talex 2804 2007-05-12 13:28 mysite/settings.py
I discovered this whilst trying to make a Zero Install package for Django. Everything in the Zero Install cache is read-only, so startproject fails with:
File "/var/cache/0install.net/implementations/sha1new=262c95b5a7cc34f525408b675106e4e4ae3494cc/django/core/management.py", line 799, in startproject fp = open(main_settings_file, 'w') IOError: [Errno 13] Permission denied: '.../site/settings.py'
Thanks,
Attachments (3)
Change History (25)
comment:1 by , 17 years ago
Summary: | startproject doesn't honour umask → startproject should honour umask |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 17 years ago
Summary: | startproject should honour umask → startproject should honor umask |
---|
comment:3 by , 17 years ago
Has patch: | set |
---|
comment:5 by , 17 years ago
The patch doesn't apply any more (the relevant code is now in django/core/management/base.py). I've adapted the patch, but I'm not sure how this should interact with the _make_writeable call. I'll upload the updated patch anyway, just in case.
comment:7 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
by , 13 years ago
Attachment: | 4282_against_16741.diff added |
---|
comment:8 by , 13 years ago
Easy pickings: | unset |
---|---|
UI/UX: | unset |
Confirmed issue still exists in r16741. This patch applies cleanly and fixes the issue.
comment:9 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:10 by , 11 years ago
I uploaded & updated the pull request. https://github.com/django/django/pull/2061
would be great if somebody will making suggestions.
comment:12 by , 3 years ago
Cc: | added |
---|
comment:13 by , 3 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
(de-assigning as not touched in 8 years)
comment:14 by , 3 years ago
Cc: | added |
---|
Hey Adam — thanks for looking!
Seems like the behaviour is unchanged (to 4.0b1):
% mkdir issue4282 % cd issue4282 issue4282 % umask 077 issue4282 % touch foo issue4282 % ls -l foo -rw------- 1 carlton staff 0 Nov 16 10:06 foo issue4282 % django-admin startproject mysite issue4282 % ls -l mysite/mysite/settings.py -rw-r--r-- 1 carlton staff 3221 Nov 16 10:06 mysite/mysite/settings.py issue4282 % django-admin --version 4.0b1
Expected behaviour is that the created files have the restricted permissions, I think.
The original patch didn't look too complex… 🤔 (We do similar in storage.py — I didn't look into Claude's comments on the PR there.)
It may be that if we're not going to fix this, we can mark it wontfix. (Is this something Django needs to handle that a find ... | xargs chmod
wouldn't be more appropriate for?)
comment:15 by , 3 years ago
Thanks for context Claude.
I have updated the original PR - however got stuck on the fact that subprocess.run()
does not support umask
(at least not prior to 3.9), and just setting umask
has no effect - which renders this untestable. If you (or anyone reading this) has any suggestions please lmk.
PR that does not passes the tests (because of umask
):
https://github.com/django/django/pull/15102
comment:16 by , 3 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:17 by , 3 years ago
Still need to fix the PR for Windows (checking hasattr(os, 'umask')
doesn't cut it), but keen to know if PR is directionally fine.
comment:18 by , 3 years ago
Patch needs improvement: | unset |
---|
Modified and tests pass- but as said in previous comment, tests are skipped on Python 3.8 as it does not support setting umask
in subprocess.run()
so we can't test.
comment:19 by , 3 years ago
Patch needs improvement: | set |
---|
comment:20 by , 3 years ago
Patch needs improvement: | unset |
---|
Thanks for the review Mariusz, learning from your comments. Applied the changes.
comment:21 by , 3 years ago
Triage Stage: | Accepted → Ready for checkin |
---|---|
Type: | New feature → Bug |
Seems like a good improvement to me. A patch would be great.