Bazaar

Merge lp:~parthm/bzr/2.0_376388_dot_bazaar_ownership into lp:bzr/2.0

  1. 2.0_376388_dot_bazaar_ownership
  2. Merge into 2.0
Proposed by Parth Malwankar
Status: Rejected
Rejected by: Martin Pool
Proposed branch: lp:~parthm/bzr/2.0_376388_dot_bazaar_ownership
Merge into: lp:bzr/2.0
Diff against target: 124 lines (+56/-3)
4 files modified
NEWS (+5/-0)
bzrlib/config.py (+4/-2)
bzrlib/osutils.py (+42/-0)
bzrlib/trace.py (+5/-1)
To merge this branch: bzr merge lp:~parthm/bzr/2.0_376388_dot_bazaar_ownership
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Review via email: mp+19593@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Parth Malwankar (parthm) wrote :

This patch fixes bug #376388
It ensure that .bazaar, .bazaar/bazaar.conf and .bzr.log when created
by bzr run under sudo, have the same usr/grp ownership as the containing
folder and not root.

copy_ownership(src, dst) function copies user and group ownership
from src to dst. copy_ownership does nothing on windows.

This function is used during creation of .bazaar, .bazaar/bazaar.conf
and .bzr.log.

.bzr.log location of /dev/null and NUL are ignored.

This was tested by creating a user 'sandbox' and running
'sudo bzr whoami a@b'. The latter creates the above files with
the correct ownership.

I am not sure what would be a good way to write a automated test case
for this. Any ideas/suggestion are welcome. I would be happy to
implement them.

Below is the log of the manual test run:

sandbox@parthm-laptop:~$ ls -la .bazaar .bzr.log .bazaar/bazaar.conf
ls: cannot access .bazaar: No such file or directory
ls: cannot access .bzr.log: No such file or directory
ls: cannot access .bazaar/bazaar.conf: No such file or directory

sandbox@parthm-laptop:~$ sudo /storage/parth/src/bzr.dev/2.0_376388_dot_bazaar_ownership/bzr whoami a@b
[sudo] password for sandbox:

sandbox@parthm-laptop:~$ ls -la .bazaar .bzr.log .bazaar/bazaar.conf-rw-r--r-- 1 sandbox sandbox 22 2010-02-18 18:13 .bazaar/bazaar.conf
-rw-r--r-- 1 sandbox sandbox 727 2010-02-18 18:13 .bzr.log

.bazaar:
total 12
drwxr-xr-x 2 sandbox sandbox 4096 2010-02-18 18:13 .
drwxr-xr-x 4 sandbox sandbox 4096 2010-02-18 18:13 ..
-rw-r--r-- 1 sandbox sandbox 22 2010-02-18 18:13 bazaar.conf
sandbox@parthm-laptop:~$

Revision history for this message
Martin Pool (mbp) wrote :

Having a special case for /dev/null and NUL is a bit gross and probably unnecessary.

I think we should set the permissions only when the files are first created; changing the perms on existing files is more dangerous and more likely to cause nasty surprises.

I think we should catch errors in doing the chmod and turn them into user warnings to avoid bzr crashing entirely if you do have a strange situation.

I'd like to see the 'create a thing inheriting permissions' factored out; probably you need a variation for mkdir and for making a file.

Once that is split out, the cleanest way to test it is probably to monkeypatch os.chmod and then run that function. You can use overrideAttr to do this.

review: Needs Fixing
Revision history for this message
Martin Pool (mbp) wrote :

An interesting followon from this would be that we could also do this when creating a .bzr directory.

Also, you should perhaps do the chown and chgrp separately, as the chown may fail when the chgrp can succeed. (Common for non-root users.)

I think overall this is a reasonable change though it might be considered a bit magical.

Revision history for this message
Martin Pool (mbp) wrote :

btw I don't think this is appropriate for 2.0: it's not a severe bug and there is some risk of it having surprising fallout. trunk only.

Revision history for this message
Parth Malwankar (parthm) wrote :

> Having a special case for /dev/null and NUL is a bit gross and probably
> unnecessary.
>
> I think we should set the permissions only when the files are first created;
> changing the perms on existing files is more dangerous and more likely to
> cause nasty surprises.
>
> I think we should catch errors in doing the chmod and turn them into user
> warnings to avoid bzr crashing entirely if you do have a strange situation.
>
> I'd like to see the 'create a thing inheriting permissions' factored out;
> probably you need a variation for mkdir and for making a file.
>
> Once that is split out, the cleanest way to test it is probably to monkeypatch
> os.chmod and then run that function. You can use overrideAttr to do this.

Thanks for your comments Martin. I have updated the patch.

Revision history for this message
Parth Malwankar (parthm) wrote :

> An interesting followon from this would be that we could also do this when
> creating a .bzr directory.
>
> Also, you should perhaps do the chown and chgrp separately, as the chown may
> fail when the chgrp can succeed. (Common for non-root users.)
>
> I think overall this is a reasonable change though it might be considered a
> bit magical.

Filed bug #524306 to track this.

Revision history for this message
Parth Malwankar (parthm) wrote :

> btw I don't think this is appropriate for 2.0: it's not a severe bug and there
> is some risk of it having surprising fallout. trunk only.

I have proposed a merge based of trunk due to the above and also because overrideAttr was not available in the 2.0 line.

Please consider this mp obsolte.
The new merge proposal is https://code.launchpad.net/~parthm/bzr/376388-dot-bazaar-ownership/+merge/19691

Revision history for this message
Parth Malwankar (parthm) wrote :

Does launchpad have a way of setting this as obsolete?

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Parth Malwankar wrote:
> Does launchpad have a way of setting this as obsolete?
>

Change the setting to Rejected.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkt+0WsACgkQJdeBCYSNAAPnqACfR887K5Hby0OkuZs/C0pg9EB4
ZmsAoNWf7m3aOVKAGMfxgjCc747FN51b
=WOeF
-----END PGP SIGNATURE-----

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-02-18 05:38:14 +0000
3+++ NEWS 2010-02-19 05:13:17 +0000
4@@ -25,6 +25,11 @@
5 ``UnboundLocalError``.
6 (Andrew Bennetts, #423563)
7
8+* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
9+ group ownership from the containing directory. This allow bzr to work
10+ better with sudo.
11+ (Parth Malwankar, #376388)
12+
13 Documentation
14 *************
15
16
17=== modified file 'bzrlib/config.py'
18--- bzrlib/config.py 2009-08-20 04:53:23 +0000
19+++ bzrlib/config.py 2010-02-19 05:13:17 +0000
20@@ -473,7 +473,9 @@
21 self._write_config_file()
22
23 def _write_config_file(self):
24- f = open(self._get_filename(), 'wb')
25+ path = self._get_filename()
26+ parent = osutils.parent_dir(path)
27+ f = osutils.open(path, 'wb', ownership_src = parent)
28 self._get_parser().write(f)
29 f.close()
30
31@@ -769,7 +771,7 @@
32 trace.mutter('creating config parent directory: %r', parent_dir)
33 os.mkdir(parent_dir)
34 trace.mutter('creating config directory: %r', path)
35- os.mkdir(path)
36+ osutils.mkdir(path, ownership_src = osutils.parent_dir(path))
37
38
39 def config_dir():
40
41=== modified file 'bzrlib/osutils.py'
42--- bzrlib/osutils.py 2009-10-08 03:55:30 +0000
43+++ bzrlib/osutils.py 2010-02-19 05:13:17 +0000
44@@ -49,7 +49,9 @@
45 cache_utf8,
46 errors,
47 win32utils,
48+ trace,
49 )
50+
51 """)
52
53 # sha and md5 modules are deprecated in python2.6 but hashlib is available as
54@@ -1590,6 +1592,46 @@
55 real_handlers[kind](abspath, relpath)
56
57
58+def parent_dir(path):
59+ """same as os.path.dirname but returns '.' instead of ''
60+ for paths that just have a filename in it e.g. 'foo'"""
61+ pdir = os.path.dirname(path)
62+ if pdir == '':
63+ pdir = '.'
64+ return pdir
65+
66+
67+def copy_ownership(dst, src):
68+ """copy user and group ownership from own_src file/dir to dst file/dir.
69+ If own_src is None, the containing directory is used as source."""
70+ if os.name != 'posix':
71+ return False
72+ try:
73+ s = os.stat(src)
74+ os.chown(dst, s.st_uid, s.st_gid)
75+ except OSError, e:
76+ trace.warning("IOError: %s\nUnable to copy ownership from '%s' to '%s'" % (e, src, dst))
77+ return True
78+
79+
80+def mkdir(path, ownership_src=None):
81+ """creates the directory 'path'. If ownership_src is given, copies (chown)
82+ usr/grp ownership from 'ownership_src' to 'path'"""
83+ os.mkdir(path)
84+ if ownership_src != None:
85+ copy_ownership(path, ownership_src)
86+
87+def open(filename, mode='r', bufsize=-1, ownership_src=None):
88+ """This function wraps the python builtin open. filename, mode and bufsize
89+ parameters behave the same as the builtin open[1].
90+ [1] http://python.org/doc/2.6.4/library/functions.html#open"""
91+ import __builtin__
92+ f = __builtin__.open(filename, mode, bufsize)
93+ if ownership_src != None:
94+ copy_ownership(filename, ownership_src)
95+ return f
96+
97+
98 def path_prefix_key(path):
99 """Generate a prefix-order path key for path.
100
101
102=== modified file 'bzrlib/trace.py'
103--- bzrlib/trace.py 2010-01-06 17:46:15 +0000
104+++ bzrlib/trace.py 2010-02-19 05:13:17 +0000
105@@ -228,14 +228,18 @@
106 _bzr_log_filename = _get_bzr_log_filename()
107 _rollover_trace_maybe(_bzr_log_filename)
108 try:
109- bzr_log_file = open(_bzr_log_filename, 'at', 1) # line buffered
110+ buffering = 1 # line buffered
111+ ownership_src = osutils.parent_dir(_bzr_log_filename)
112+ bzr_log_file = osutils.open(_bzr_log_filename, 'at', buffering, ownership_src)
113 # bzr_log_file.tell() on windows always return 0 until some writing done
114 bzr_log_file.write('\n')
115 if bzr_log_file.tell() <= 2:
116 bzr_log_file.write("this is a debug log for diagnosing/reporting problems in bzr\n")
117 bzr_log_file.write("you can delete or truncate this file, or include sections in\n")
118 bzr_log_file.write("bug reports to https://bugs.launchpad.net/bzr/+filebug\n\n")
119+
120 return bzr_log_file
121+
122 except IOError, e:
123 # If we are failing to open the log, then most likely logging has not
124 # been set up yet. So we just write to stderr rather than using

Subscribers

People subscribed via source and target branches