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 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 started a PR based on the original.
What has changed since the original is that django-admin is now called with subprocess.run()
, where setting umask
is only possibly since 3.9. (Discussion on introduction of umask
and [lack of] alternatives at https://bugs.python.org/issue38417).
Would it be OK to write the tests so they are only run on >=3.9 ?
I'll try and fix the PR to reflect that first and flag it ready for review once done.
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.