Merge lp:~parthm/bzr/376388-dot-bazaar-ownership into lp:bzr

[Parth Malwankar]

[Parth Malwankar (parthm)]

This patch fixes bug #376388 by ensuring the .bazaar,
.bazaar/bazaar.conf and .bzr.log inherit usr and grp ownership
from the containing folder.

Following functions are added to osutils:
* copy_ownership: copies usr/grp ownership from src file/dir to dst file/dir
* mkdir: wraps os.mkdir and adds an optional arg ownership_src. When provided,
  the newly created dir is given ownership based on ownership_src.
* open: wraps __builtin__.open and adds an optional arg ownership_src. When provided,
  the newly created file is given ownership based on ownership_src.
* parent_dir: wraps os.path.dirname handling the special case of dirname returning ''
  by returning '.' instead.

config.py and trace.py are updated to use osutils.{mkdir,open} rather than the
base functions during creation of above mentioned files.

Whitebox tests are added to test_osutils.py to verify mkdir, open and parent_dir.
Manual testing for done using "sudo bzr whoami a@b".

ChmodFeature was added to tests/__init__.py to ensure os.chmod support for test
cases.

This proposal was originally based of 2.0:
https://code.launchpad.net/~parthm/bzr/2.0_376388_dot_bazaar_ownership/+merge/19593/
but is resubmitted now as it is meant to go into trunk and testing requires
overrideAttr support that is not available in 2.0 branch.

[Parth Malwankar (parthm)]

Similar issue filed as bug #524306

[Martin Packman (gz)]

As mentioned in a different (and now deleted?) merge request, this clashes with that change as they both try and create a shadow of a builtin function in osutils, which I think is bad style.

In this case, I don't see why:

+ ownership_src = osutils.parent_dir(_bzr_log_filename)
+ bzr_log_file = osutils.open(_bzr_log_filename, 'at', buffering, ownership_src)

Is preferable to the more obvious:

+ bzr_log_file = open(_bzr_log_filename, 'at', 0) # unbuffered
+ osutils.copy_ownership(_bzr_log_filename,
+ osutils.parent_dir(_bzr_log_filename))

Would also be nice to avoid this junk entirely on windows, but that doesn't matter much.

review: Needs Fixing

[Parth Malwankar (parthm)]

> As mentioned in a different (and now deleted?) merge request, this clashes
> with that change as they both try and create a shadow of a builtin function in
> osutils, which I think is bad style.
>
> In this case, I don't see why:
>
> + ownership_src = osutils.parent_dir(_bzr_log_filename)
> + bzr_log_file = osutils.open(_bzr_log_filename, 'at', buffering,
> ownership_src)
>

Good point. Maybe we should have open_with_ownership (or something similar)
for which ownership_src is mandatory. This will keep the usage clean and
also not be confused with the builtin.

[Vincent Ladeuil (vila)]

You have some PEP8 issues (see doc/developpers/HACKING.txt section Code layout),
namely:
- no spaces around '='
27 + f = osutils.open_with_ownership(path, 'wb', ownership_src = parent)

- lines should be no more than 79 characters (watch for your tests docstrings
  in particular)

58 +def parent_dir(path):

This looks weird, why not just embed that code in copy_ownership like your docstring
seem to imply ?

87 +def open_with_ownership(filename, mode='r', bufsize=-1, ownership_src=None):
88 + """This function wraps the python builtin open. filename, mode and bufsize

From "Code Layout" cited above:
One often-missed requirement is that the first line of docstrings
should be a self-contained one-sentence summary.

135 + # we can't overrideAttr os.chown on OS that doesn't support chown
136 + if os.name == 'posix' and hasattr(os, 'chown'):
137 + self.overrideAttr(os, 'chown', self._dummy_chown)

You don't need to protect the call since your tests already depends on tests.ChownFeature.

145 + def test_mkdir_with_ownership_no_chown(self):
146 + """ensure that osutils.mkdir_with_ownership does not chown without ownership_src arg"""
147 + self.requireFeature(tests.ChownFeature)

You can avoid avoid duplicating that check for each test by doing:

class TestCreationOps(tests.TestCaseInTempDir):

    _test_needs_features = [tests.ChownFeature]

142 + def _dummy_chown(self, path, uid, gid):
143 + self.path, self.uid, self.gid = path, uid, gid

Hmm, using a dummy here is questionable since you don't really test calling chown(),
I'd prefer a recording variant like:

def _instrumented_chown(self, path, uid, gid):
    self.calls.append((path, uid, gid))
    return os.chown(path, uid, gid)

so you can both check that it was called *and* verify what was done.

180 + ownsrc = '/'

Ouch ! You're playing with fire here, I realize you may want to test with a different
uid/gid than the current user but... you'd better create such a reference file explicitly.

On the overall, I'm still a bit doubtful on the whole approach, it seems fine for root
but what happens if we use 'sudo -Ujoe' ?

review: Needs Fixing

[Parth Malwankar (parthm)]

Thanks for your comments Vincent.
I have updated the code based on your review comments.

The testing approach ( http://pastebin.com/CKhiAgL4 ) didn't work ( http://pastebin.com/ZPk2XGvv ), so it seems like we are stuck with this approach for now unless more ideas come through.

Based on the discussion vila and I had on IRC it seems that user foo cannot change ownership to user bar and chown would work only under sudo. Can such a situation arise? Not having a thorough test case or approach that works under all situations seems somewhat risky.

As discussed I am setting the status to Needs Review so that we can thrash out the solution some more as needed. I would be happy to try out any other ideas. Based on the outcome we could either update the patch or reject it.

[Vincent Ladeuil (vila)]

See https://bugs.edge.launchpad.net/bzr/+bug/376388/comments/16,
only root can use chown.

[Parth Malwankar (parthm)]

> See https://bugs.edge.launchpad.net/bzr/+bug/376388/comments/16,
> only root can use chown.

One approach could be to catch OSError during chown. That way
the ownership would be correct under sudo and in the normal
case the user won't be bothered.

[Martin Pool (mbp)]

56
57 +def copy_ownership(dst, src=None):
58 + """copy usr/grp ownership from src file/dir to dst file/dir.
59 + If src is None, the containing directory is used as source."""
60 + if os.name != 'posix':
61 + return False
62 +
63 + if src == None:
64 + src = os.path.dirname(dst)
65 + if src == '':
66 + src = '.'
67 +
68 + try:
69 + s = os.stat(src)
70 + os.chown(dst, s.st_uid, s.st_gid)
71 + except OSError, e:
72 + trace.warning("IOError: %s. Unable to copy ownership from '%s' to '%s'" % (e, src, dst))
73 + return True
74 +

Your approach of catching OSError is fne.

Normally we would write the message as:

 Unable to copy ownership from "%s" to "%s": %s

with the message at the end.

I think rather than checking os.name it would be better to check if os.chown exists, ie

chown = getattr(os, 'chown')
if chown is None: return

Why does this return a boolean? If it needs to, you should document and maybe test the value. Otherwise, don't return anything.

For consistency our docstrings have:

 * Sentence capitalization and punctuation
 * If possible a single sentence on the first line
 * A blank line between paragraphs
 * The closing quotes on a separate line

See http://www.python.org/dev/peps/pep-0008/ and the developer docs

114 +
115 +class _ChownFeature(tests.Feature):
116 + """os.chown is supported"""
117 +
118 + def _probe(self):
119 + return os.name == 'posix' and hasattr(os, 'chown')
120 +
121 +ChownFeature = _ChownFeature()
122 +

Normally the instance of the feature object should be named like an instance, ie lowercase chown_feature. Sorry this is confusing, we changed it a while ago.

Thanks!

Martin

review: Needs Fixing

[Parth Malwankar (parthm)]

Thanks for the review Martin.
I have made the relevant changes.

[Martin Pool (mbp)]

review: Approve

[Martin Pool (mbp)]

I'll resolve the conflicts, tweak the feature name to be lowercase, and submit this.

[Parth Malwankar (parthm)]

Thanks Martin.

=== modified file 'NEWS'
--- NEWS	2010-03-11 05:38:54 +0000
+++ NEWS	2010-03-11 06:39:25 +0000
@@ -120,6 +120,11 @@
 * Tolerate patches with leading noise in ``bzr-handle-patch``.
   (Toshio Kuratomi, Martin Pool, #502076)
 
+* ``.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)
+
 API Changes
 ***********
 
 
=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2010-02-23 07:43:11 +0000
+++ bzrlib/config.py	2010-03-11 06:39:25 +0000
@@ -510,7 +510,8 @@
         self._write_config_file()
 
     def _write_config_file(self):
-        f = open(self._get_filename(), 'wb')
+        path = self._get_filename()
+        f = osutils.open_with_ownership(path, 'wb')
         self._get_parser().write(f)
         f.close()
 
@@ -809,7 +810,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_with_ownership(path)
 
 
 def config_dir():
 
=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-03-11 04:33:41 +0000
+++ bzrlib/osutils.py	2010-03-11 06:39:25 +0000
@@ -51,7 +51,9 @@
     cache_utf8,
     errors,
     win32utils,
+    trace,
     )
+
 """)
 
 # sha and md5 modules are deprecated in python2.6 but hashlib is available as
@@ -1804,6 +1806,51 @@
             real_handlers[kind](abspath, relpath)
 
 
+def copy_ownership(dst, src=None):
+    """Copy usr/grp ownership from src file/dir to dst file/dir.
+
+    If src is None, the containing directory is used as source. If chown
+    fails, the error is ignored and a warning is printed.
+    """
+    has_chown = getattr(os, 'chown')
+    if has_chown is None: return
+
+    if src == None:
+        src = os.path.dirname(dst)
+        if src == '':
+            src = '.'
+
+    try:
+        s = os.stat(src)
+        os.chown(dst, s.st_uid, s.st_gid)
+    except OSError, e:
+        trace.warning("Unable to copy ownership from '%s' to '%s': IOError: %s." % (src, dst, e))
+
+
+def mkdir_with_ownership(path, ownership_src=None):
+    """Create the directory 'path' with specified ownership.
+
+    If ownership_src is given, copies (chown) usr/grp ownership
+    from 'ownership_src' to 'path'. If ownership_src is None, use the
+    containing dir ownership.
+    """
+    os.mkdir(path)
+    copy_ownership(path, ownership_src)
+
+
+def open_with_ownership(filename, mode='r', bufsize=-1, ownership_src=None):
+    """Open the file 'filename' with the specified ownership.
+
+    If ownership_src is specified, copy usr/grp ownership from ownership_src
+    to filename. If ownership_src is None, copy ownership from containing
+    directory.
+    Returns the opened file object.
+    """
+    f = open(filename, mode, bufsize)
+    copy_ownership(filename, ownership_src)
+    return f
+
+
 def path_prefix_key(path):
     """Generate a prefix-order path key for path.
 
 
=== modified file 'bzrlib/tests/features.py'
--- bzrlib/tests/features.py	2010-02-23 07:43:11 +0000
+++ bzrlib/tests/features.py	2010-03-11 06:39:25 +0000
@@ -14,6 +14,7 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
+import os
 
 from bzrlib import tests
 from bzrlib.symbol_versioning import deprecated_in
@@ -23,3 +24,12 @@
 paramiko = tests.ModuleAvailableFeature('paramiko')
 pycurl = tests.ModuleAvailableFeature('pycurl')
 subunit = tests.ModuleAvailableFeature('subunit')
+
+class _ChownFeature(tests.Feature):
+    """os.chown is supported"""
+
+    def _probe(self):
+        return os.name == 'posix' and hasattr(os, 'chown')
+
+chown_feature = _ChownFeature()
+
 
=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2010-02-23 07:43:11 +0000
+++ bzrlib/tests/test_osutils.py	2010-03-11 06:39:25 +0000
@@ -33,6 +33,7 @@
     win32utils,
     )
 from bzrlib.tests import (
+    features,
     file_utils,
     test__walkdirs_win32,
     )
@@ -1975,3 +1976,43 @@
         del os.environ['COLUMNS']
         # Whatever the result is, if we don't raise an exception, it's ok.
         osutils.terminal_width()
+
+class TestCreationOps(tests.TestCaseInTempDir):
+    _test_needs_features = [features.chown_feature]
+
+    def setUp(self):
+        tests.TestCaseInTempDir.setUp(self)
+        self.overrideAttr(os, 'chown', self._dummy_chown)
+
+        # params set by call to _dummy_chown
+        self.path = self.uid = self.gid = None
+
+    def _dummy_chown(self, path, uid, gid):
+        self.path, self.uid, self.gid = path, uid, gid
+
+    def test_mkdir_with_ownership_chown(self):
+        """Ensure that osutils.mkdir_with_ownership chowns correctly with ownership_src.
+        """
+        ownsrc = '/'
+        osutils.mkdir_with_ownership('foo', ownsrc)
+
+        s = os.stat(ownsrc)
+        self.assertEquals(self.path, 'foo')
+        self.assertEquals(self.uid, s.st_uid)
+        self.assertEquals(self.gid, s.st_gid)
+
+    def test_open_with_ownership_chown(self):
+        """Ensure that osutils.open_with_ownership chowns correctly with ownership_src.
+        """
+        ownsrc = '/'
+        f = osutils.open_with_ownership('foo', 'w', ownership_src=ownsrc)
+
+        # do a test write and close
+        f.write('hello')
+        f.close()
+
+        s = os.stat(ownsrc)
+        self.assertEquals(self.path, 'foo')
+        self.assertEquals(self.uid, s.st_uid)
+        self.assertEquals(self.gid, s.st_gid)
+
 
=== modified file 'bzrlib/trace.py'
--- bzrlib/trace.py	2010-02-24 01:20:51 +0000
+++ bzrlib/trace.py	2010-03-11 06:39:25 +0000
@@ -241,14 +241,17 @@
     _bzr_log_filename = _get_bzr_log_filename()
     _rollover_trace_maybe(_bzr_log_filename)
     try:
-        bzr_log_file = open(_bzr_log_filename, 'at', buffering=0) # unbuffered
+        buffering = 0 # unbuffered
+        bzr_log_file = osutils.open_with_ownership(_bzr_log_filename, 'at', buffering)
         # 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
 
=== modified file 'doc/developers/testing.txt'
--- doc/developers/testing.txt	2010-02-28 10:08:29 +0000
+++ doc/developers/testing.txt	2010-03-11 06:39:25 +0000
@@ -576,6 +576,8 @@
  - UnicodeFilenameFeature
  - FTPServerFeature
  - CaseInsensitiveFilesystemFeature.
+ - ChownFeature: The test can rely on OS being POSIX and python
+   supporting os.chown.
 
 
 Defining a new feature that tests can require