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

[Parth Malwankar]

[Parth Malwankar (parthm)]

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:~$

[Martin Pool (mbp)]

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

[Martin Pool (mbp)]

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.

[Martin Pool (mbp)]

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.

[lp:~parthm/bzr/2.0_376388_dot_bazaar_ownership]

4736. By Parth Malwankar on 2010-02-19

added parent_dir and mkdir to osutils. osutils.mkdir optionally
supports copying ownership for a specified file or dir.

4737. By Parth Malwankar on 2010-02-19

added osutils.open which optionally takes ownership_src and
uses that to set usr/grp ownership of the opened file.

[Parth Malwankar (parthm)]

> 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.

[Parth Malwankar (parthm)]

> 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.

[Parth Malwankar (parthm)]

> 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

[Parth Malwankar (parthm)]

Does launchpad have a way of setting this as obsolete?

[John A Meinel (jameinel)]

-----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-----

Unmerged revisions

4737. By Parth Malwankar on 2010-02-19

added osutils.open which optionally takes ownership_src and
uses that to set usr/grp ownership of the opened file.

4736. By Parth Malwankar on 2010-02-19

added parent_dir and mkdir to osutils. osutils.mkdir optionally
supports copying ownership for a specified file or dir.

4735. By Parth Malwankar on 2010-02-18

updated NEWS

4734. By Parth Malwankar on 2010-02-18

fixed test cases to handle special .bzr.log files
i.e. NUL or /dev/null.
also handle the case where the caller uses os.path.dirname
and that returns ''

4733. By Parth Malwankar on 2010-02-18

default .bazaar, .bzr.log and .bazaar/bazaar.conf retain
the ownershup of the containing directory.

=== modified file 'NEWS'
--- NEWS	2010-02-18 05:38:14 +0000
+++ NEWS	2010-02-19 05:13:17 +0000
@@ -25,6 +25,11 @@
   ``UnboundLocalError``.
   (Andrew Bennetts, #423563)
 
+* ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
+  group ownership from the containing directory. This allow bzr to work
+  better with sudo.
+  (Parth Malwankar, #376388)
+
 Documentation
 *************
 

=== 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 @@
         self._write_config_file()
 
     def _write_config_file(self):
-        f = open(self._get_filename(), 'wb')
+        path = self._get_filename()
+        parent = osutils.parent_dir(path)
+        f = osutils.open(path, 'wb', ownership_src = parent)
         self._get_parser().write(f)
         f.close()
 
@@ -769,7 +771,7 @@
                 trace.mutter('creating config parent directory: %r', parent_dir)
             os.mkdir(parent_dir)
         trace.mutter('creating config directory: %r', path)
-        os.mkdir(path)
+        osutils.mkdir(path, ownership_src = osutils.parent_dir(path))
 
 
 def config_dir():

=== 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 @@
     cache_utf8,
     errors,
     win32utils,
+    trace,
     )
+
 """)
 
 # sha and md5 modules are deprecated in python2.6 but hashlib is available as
@@ -1590,6 +1592,46 @@
             real_handlers[kind](abspath, relpath)
 
 
+def parent_dir(path):
+    """same as os.path.dirname but returns '.' instead of ''
+    for paths that just have a filename in it e.g. 'foo'"""
+    pdir = os.path.dirname(path)
+    if pdir == '':
+        pdir = '.'
+    return pdir
+
+
+def copy_ownership(dst, src):
+    """copy user and group ownership from own_src file/dir to dst file/dir.
+    If own_src is None, the containing directory is used as source."""
+    if os.name != 'posix':
+        return False
+    try:
+        s = os.stat(src)
+        os.chown(dst, s.st_uid, s.st_gid)
+    except OSError, e:
+        trace.warning("IOError: %s\nUnable to copy ownership from '%s' to '%s'" % (e, src, dst))
+    return True
+
+
+def mkdir(path, ownership_src=None):
+    """creates the directory 'path'. If ownership_src is given, copies (chown)
+    usr/grp ownership from 'ownership_src' to 'path'"""
+    os.mkdir(path)
+    if ownership_src != None:
+        copy_ownership(path, ownership_src)
+
+def open(filename, mode='r', bufsize=-1, ownership_src=None):
+    """This function wraps the python builtin open. filename, mode and bufsize
+    parameters behave the same as the builtin open[1].
+    [1] http://python.org/doc/2.6.4/library/functions.html#open"""
+    import __builtin__
+    f = __builtin__.open(filename, mode, bufsize)
+    if ownership_src != None:
+        copy_ownership(filename, ownership_src)
+    return f
+
+
 def path_prefix_key(path):
     """Generate a prefix-order path key for path.
 

=== 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 @@
     _bzr_log_filename = _get_bzr_log_filename()
     _rollover_trace_maybe(_bzr_log_filename)
     try:
-        bzr_log_file = open(_bzr_log_filename, 'at', 1) # line buffered
+        buffering = 1 # line buffered
+        ownership_src = osutils.parent_dir(_bzr_log_filename)
+        bzr_log_file = osutils.open(_bzr_log_filename, 'at', buffering, ownership_src)
         # bzr_log_file.tell() on windows always return 0 until some writing done
         bzr_log_file.write('\n')
         if bzr_log_file.tell() <= 2:
             bzr_log_file.write("this is a debug log for diagnosing/reporting problems in bzr\n")
             bzr_log_file.write("you can delete or truncate this file, or include sections in\n")
             bzr_log_file.write("bug reports to https://bugs.launchpad.net/bzr/+filebug\n\n")
+
         return bzr_log_file
+
     except IOError, e:
         # If we are failing to open the log, then most likely logging has not
         # been set up yet. So we just write to stderr rather than using