Opened 17 years ago

Closed 3 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, 14 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, 11 years ago

Owner: changed from nobody to merb
Status: newassigned

comment:10 by merb, 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:11 by Claude Paroz, 10 years ago

Comment added on the pull request.

comment:12 by Ad Timmering, 3 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, 3 years ago

Owner: merb removed
Status: assignednew

(de-assigning as not touched in 8 years)

comment:14 by Carlton Gibson, 3 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 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?)

Version 0, edited 3 years ago by Carlton Gibson (next)

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

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

comment:16 by Ad Timmering, 3 years ago

Owner: set to Ad Timmering
Status: newassigned

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

Patch needs improvement: set

comment:20 by Ad Timmering, 3 years ago

Patch needs improvement: unset

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

comment:21 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin
Type: New featureBug

comment:22 by Mariusz Felisiak <felisiak.mariusz@…>, 3 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