Opened 17 years ago

Closed 2 years ago

#4282 closed Bug (fixed)

startproject should honor umask

Reported by: talex5+django@… 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)

permissions.patch (1.4 KB ) - added by talex5+django@… 17 years ago.
Fix.
django-4282.diff (1.5 KB ) - added by Fredrik Lundh <fredrik@…> 17 years ago.
Updated patch, against revision 6281.
4282_against_16741.diff (1.6 KB ) - added by Peter Baumgartner 13 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 by Simon G. <dev@…>, 17 years ago

Summary: startproject doesn't honour umaskstartproject should honour umask
Triage Stage: UnreviewedAccepted

comment:2 by Adrian Holovaty, 17 years ago

Summary: startproject should honour umaskstartproject should honor umask

Seems like a good improvement to me. A patch would be great.

by talex5+django@…, 17 years ago

Attachment: permissions.patch added

Fix.

comment:3 by anonymous, 17 years ago

Has patch: set

comment:4 by anonymous, 17 years ago

Any comments on this patch? Is it OK?

comment:5 by Fredrik Lundh <fredrik@…>, 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.

by Fredrik Lundh <fredrik@…>, 17 years ago

Attachment: django-4282.diff added

Updated patch, against revision 6281.

comment:6 by Adam Nelson, 14 years ago

Patch needs improvement: set

Patch needs to be updated again.

comment:7 by Łukasz Rekucki, 13 years ago

Severity: Normal
Type: New feature

by Peter Baumgartner, 13 years ago

Attachment: 4282_against_16741.diff added

comment:8 by Peter Baumgartner, 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 merb, 10 years ago

Owner: changed from nobody to merb
Status: newassigned

comment:10 by merb, 10 years ago

I uploaded & updated the pull request. https://github.com/django/django/pull/2061
would be great if somebody will making suggestions.

comment:11 by Claude Paroz, 10 years ago

Comment added on the pull request.

comment:12 by Ad Timmering, 2 years ago

Cc: Ad Timmering added

Browsing some ancient tickets, wondering if this ticket can perhaps be closed as obsolete? If not - what is still missing?

Ticket isn't very clear on what the issue is and how to reproduce is, but a lot has changed on permission handling between the time of the ticket and #17042, #26494, #27628.

comment:13 by Ad Timmering, 2 years ago

Owner: merb removed
Status: assignednew

(de-assigning as not touched in 8 years)

comment:14 by Carlton Gibson, 2 years ago

Cc: Claude Paroz 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?)

Last edited 2 years ago by Carlton Gibson (previous) (diff)

comment:15 by Ad Timmering, 2 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.

Last edited 2 years ago by Ad Timmering (previous) (diff)

comment:16 by Ad Timmering, 2 years ago

Owner: set to Ad Timmering
Status: newassigned

comment:17 by Ad Timmering, 2 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 Ad Timmering, 2 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 Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:20 by Ad Timmering, 2 years ago

Patch needs improvement: unset

Thanks for the review Mariusz, learning from your comments. Applied the changes.

comment:21 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin
Type: New featureBug

comment:22 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 59f47969:

Fixed #4282 -- Made startapp/startproject management commands honor umask.

Co-authored-by: Christian Schmitt <c.schmitt@…>

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