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
=== modified file 'NEWS'
--- NEWS 2010-02-18 05:38:14 +0000
+++ NEWS 2010-02-19 05:13:17 +0000
@@ -25,6 +25,11 @@
25 ``UnboundLocalError``.25 ``UnboundLocalError``.
26 (Andrew Bennetts, #423563)26 (Andrew Bennetts, #423563)
2727
28* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
29 group ownership from the containing directory. This allow bzr to work
30 better with sudo.
31 (Parth Malwankar, #376388)
32
28Documentation33Documentation
29*************34*************
3035
3136
=== modified file 'bzrlib/config.py'
--- bzrlib/config.py 2009-08-20 04:53:23 +0000
+++ bzrlib/config.py 2010-02-19 05:13:17 +0000
@@ -473,7 +473,9 @@
473 self._write_config_file()473 self._write_config_file()
474474
475 def _write_config_file(self):475 def _write_config_file(self):
476 f = open(self._get_filename(), 'wb')476 path = self._get_filename()
477 parent = osutils.parent_dir(path)
478 f = osutils.open(path, 'wb', ownership_src = parent)
477 self._get_parser().write(f)479 self._get_parser().write(f)
478 f.close()480 f.close()
479481
@@ -769,7 +771,7 @@
769 trace.mutter('creating config parent directory: %r', parent_dir)771 trace.mutter('creating config parent directory: %r', parent_dir)
770 os.mkdir(parent_dir)772 os.mkdir(parent_dir)
771 trace.mutter('creating config directory: %r', path)773 trace.mutter('creating config directory: %r', path)
772 os.mkdir(path)774 osutils.mkdir(path, ownership_src = osutils.parent_dir(path))
773775
774776
775def config_dir():777def config_dir():
776778
=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py 2009-10-08 03:55:30 +0000
+++ bzrlib/osutils.py 2010-02-19 05:13:17 +0000
@@ -49,7 +49,9 @@
49 cache_utf8,49 cache_utf8,
50 errors,50 errors,
51 win32utils,51 win32utils,
52 trace,
52 )53 )
54
53""")55""")
5456
55# sha and md5 modules are deprecated in python2.6 but hashlib is available as57# sha and md5 modules are deprecated in python2.6 but hashlib is available as
@@ -1590,6 +1592,46 @@
1590 real_handlers[kind](abspath, relpath)1592 real_handlers[kind](abspath, relpath)
15911593
15921594
1595def parent_dir(path):
1596 """same as os.path.dirname but returns '.' instead of ''
1597 for paths that just have a filename in it e.g. 'foo'"""
1598 pdir = os.path.dirname(path)
1599 if pdir == '':
1600 pdir = '.'
1601 return pdir
1602
1603
1604def copy_ownership(dst, src):
1605 """copy user and group ownership from own_src file/dir to dst file/dir.
1606 If own_src is None, the containing directory is used as source."""
1607 if os.name != 'posix':
1608 return False
1609 try:
1610 s = os.stat(src)
1611 os.chown(dst, s.st_uid, s.st_gid)
1612 except OSError, e:
1613 trace.warning("IOError: %s\nUnable to copy ownership from '%s' to '%s'" % (e, src, dst))
1614 return True
1615
1616
1617def mkdir(path, ownership_src=None):
1618 """creates the directory 'path'. If ownership_src is given, copies (chown)
1619 usr/grp ownership from 'ownership_src' to 'path'"""
1620 os.mkdir(path)
1621 if ownership_src != None:
1622 copy_ownership(path, ownership_src)
1623
1624def open(filename, mode='r', bufsize=-1, ownership_src=None):
1625 """This function wraps the python builtin open. filename, mode and bufsize
1626 parameters behave the same as the builtin open[1].
1627 [1] http://python.org/doc/2.6.4/library/functions.html#open"""
1628 import __builtin__
1629 f = __builtin__.open(filename, mode, bufsize)
1630 if ownership_src != None:
1631 copy_ownership(filename, ownership_src)
1632 return f
1633
1634
1593def path_prefix_key(path):1635def path_prefix_key(path):
1594 """Generate a prefix-order path key for path.1636 """Generate a prefix-order path key for path.
15951637
15961638
=== modified file 'bzrlib/trace.py'
--- bzrlib/trace.py 2010-01-06 17:46:15 +0000
+++ bzrlib/trace.py 2010-02-19 05:13:17 +0000
@@ -228,14 +228,18 @@
228 _bzr_log_filename = _get_bzr_log_filename()228 _bzr_log_filename = _get_bzr_log_filename()
229 _rollover_trace_maybe(_bzr_log_filename)229 _rollover_trace_maybe(_bzr_log_filename)
230 try:230 try:
231 bzr_log_file = open(_bzr_log_filename, 'at', 1) # line buffered231 buffering = 1 # line buffered
232 ownership_src = osutils.parent_dir(_bzr_log_filename)
233 bzr_log_file = osutils.open(_bzr_log_filename, 'at', buffering, ownership_src)
232 # bzr_log_file.tell() on windows always return 0 until some writing done234 # bzr_log_file.tell() on windows always return 0 until some writing done
233 bzr_log_file.write('\n')235 bzr_log_file.write('\n')
234 if bzr_log_file.tell() <= 2:236 if bzr_log_file.tell() <= 2:
235 bzr_log_file.write("this is a debug log for diagnosing/reporting problems in bzr\n")237 bzr_log_file.write("this is a debug log for diagnosing/reporting problems in bzr\n")
236 bzr_log_file.write("you can delete or truncate this file, or include sections in\n")238 bzr_log_file.write("you can delete or truncate this file, or include sections in\n")
237 bzr_log_file.write("bug reports to https://bugs.launchpad.net/bzr/+filebug\n\n")239 bzr_log_file.write("bug reports to https://bugs.launchpad.net/bzr/+filebug\n\n")
240
238 return bzr_log_file241 return bzr_log_file
242
239 except IOError, e:243 except IOError, e:
240 # If we are failing to open the log, then most likely logging has not244 # If we are failing to open the log, then most likely logging has not
241 # been set up yet. So we just write to stderr rather than using245 # been set up yet. So we just write to stderr rather than using

Subscribers

People subscribed via source and target branches