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

[Parth Malwankar]

Fix for .bazaar ownership regression for bug #376388.

This patch fixes a regression in an earlier fix for bug #376388
A test case is also added to ensure that IniBasedConfig._write_config_file calls chown.

[Vincent Ladeuil (vila)]

Thanks for the prompt reply !
The test scope is a bit tight, but the important point is that it will fail if the implementation is changed and could be updated or tweaked then.

review: Approve

[John A Meinel (jameinel)]

Is this really what we want to do? It looks like we grab the info for the file from its containing directory. I think what we would really want is to have the permissions be obtained from the file that is being replaced.

Which would hint to a fix in AtomicFile itself, possibly with an optional flag.

review: Needs Fixing

[Parth Malwankar (parthm)]

Thanks for the review John.

> Is this really what we want to do? It looks like we grab the info for the file
> from its containing directory. I think what we would really want is to have
> the permissions be obtained from the file that is being replaced.
>

There is one specific case which requires that the ownership gets copied from the containing directory. If the config file doesn't exist and is created by bzr under sudo we need to be sure that the user has ownership.

> Which would hint to a fix in AtomicFile itself, possibly with an optional
> flag.

[Vincent Ladeuil (vila)]

> Is this really what we want to do?

It's what we did before.

> It looks like we grab the info for the file
> from its containing directory. I think what we would really want is to have
> the permissions be obtained from the file that is being replaced.

Isn't atomicfile ensuring the permissions are preserved ?

copy_ownership only applies when running as root anyway...

[Vincent Ladeuil (vila)]

> > Is this really what we want to do?
>
> It's what we did before.

To clarify: it's what we did before the regression, which isn't shown by this submission.

[Vincent Ladeuil (vila)]

> > Is this really what we want to do?
>
> It's what we did before.

To clarify: it's what we did before the regression, which isn't shown by this submission.

I think we should land this submission as is and address enhancements in atomicfile and/or
copy_ownership in a further submission if needed.

[John A Meinel (jameinel)]

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

Parth Malwankar wrote:
> Thanks for the review John.
>
>> Is this really what we want to do? It looks like we grab the info for the file
>> from its containing directory. I think what we would really want is to have
>> the permissions be obtained from the file that is being replaced.
>>
>
> There is one specific case which requires that the ownership gets copied from the containing directory. If the config file doesn't exist and is created by bzr under sudo we need to be sure that the user has ownership.

That is true, but if the file already exists, shouldn't we use the
permissions it already has?

John
=:->

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

iEYEARECAAYFAkxFy8QACgkQJdeBCYSNAAO8PQCcDpwk6fEc3MMCS7NQS3XkoMah
pl0An3s5gtO13PdV9AIFsrF+JvR3CYnr
=dFGi
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

> > There is one specific case which requires that the ownership gets copied
> from the containing directory. If the config file doesn't exist and is created
> by bzr under sudo we need to be sure that the user has ownership.
>
> That is true, but if the file already exists, shouldn't we use the
> permissions it already has?

The patch is about ownership of the file not about its permissions. atomicfile preserve existing permissions right ?

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
>>> There is one specific case which requires that the ownership gets copied
>> from the containing directory. If the config file doesn't exist and is created
>> by bzr under sudo we need to be sure that the user has ownership.
>>
>> That is true, but if the file already exists, shouldn't we use the
>> permissions it already has?
>
> The patch is about ownership of the file not about its permissions. atomicfile preserve existing permissions right ?
>

Well, shouldn't we use the *ownership* it already has? (yes you are
right that AF preserves permissions.)

John
=:->

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

iEYEARECAAYFAkxGFxQACgkQJdeBCYSNAAMbOACgnu6fZTi9i02xG9R0ik27KPAm
BoIAoJgNO0KpmnIaAeF7X1EVwLPEBkV2
=sFss
-----END PGP SIGNATURE-----

[Parth Malwankar (parthm)]

So I tried to copy ownership conditionally:

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py 2010-07-14 14:31:38 +0000
+++ bzrlib/config.py 2010-07-21 02:36:39 +0000
@@ -480,11 +480,13 @@

     def _write_config_file(self):
         filename = self._get_filename()
+ file_existed = os.path.isfile(filename)
         atomic_file = atomicfile.AtomicFile(filename)
         self._get_parser().write(atomic_file)
         atomic_file.commit()
         atomic_file.close()
- osutils.copy_ownership_from_path(filename)
+ if not file_existed:
+ osutils.copy_ownership_from_path(filename)

I noticed that in case of a second operation under sudo (when the file already exists) the write operation changes the ownership to root. The operation I experimented with was 'sudo bzr whoami a@b && sudo bzr whoami b@c'.
I suspect this was the reason it wasn't conditional in the first place but the original patch was created quite a few months ago.

[Vincent Ladeuil (vila)]

I think we still have grey areas when working under sudo, but this patch doesn't make things worse in that respect and we need to fix the regression.

A further submission may create a specific test suite that we can run under sudo (depending on a feature and/or using the test_suite_factory parameter of bt.selftest) so we can run it on a dedicated slave on babune. I've filed bug #608216 for that.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

> Well, shouldn't we use the *ownership* it already has?

Keep in mind that only root can change ownership, this code is targeted as usages under sudo, we may want to try being more fancy here like respecting the current owner if the file owner is not root nor the containing directory owner but that sounds weird.

We are talking of the user config files here and I can't see a good reason to have different owners for the config file and its containing directory.

Anyway, if we want to explore these scenarios, we need to write the corresponding tests, hence bug #608216.

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2010-07-13 16:10:20 +0000
+++ bzrlib/config.py	2010-07-21 03:17:41 +0000
@@ -479,10 +479,12 @@
         return self.get_user_option('nickname')
 
     def _write_config_file(self):
-        atomic_file = atomicfile.AtomicFile(self._get_filename())
+        filename = self._get_filename()
+        atomic_file = atomicfile.AtomicFile(filename)
         self._get_parser().write(atomic_file)
         atomic_file.commit()
         atomic_file.close()
+        osutils.copy_ownership_from_path(filename)
 
 
 class GlobalConfig(IniBasedConfig):

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2010-04-23 07:15:23 +0000
+++ bzrlib/tests/test_config.py	2010-07-21 03:17:41 +0000
@@ -35,6 +35,7 @@
     trace,
     transport,
     )
+from bzrlib.tests import features
 from bzrlib.util.configobj import configobj
 
 
@@ -367,7 +368,7 @@
             '/home/bogus/.cache')
 
 
-class TestIniConfig(tests.TestCase):
+class TestIniConfig(tests.TestCaseInTempDir):
 
     def make_config_parser(self, s):
         conf = config.IniBasedConfig(None)
@@ -393,6 +394,22 @@
         parser = my_config._get_parser(file=config_file)
         self.failUnless(my_config._get_parser() is parser)
 
+    def _dummy_chown(self, path, uid, gid):
+        self.path, self.uid, self.gid = path, uid, gid
+
+    def test_ini_config_ownership(self):
+        """Ensure that chown is happening during _write_config_file.
+        """
+        self.requireFeature(features.chown_feature)
+        self.overrideAttr(os, 'chown', self._dummy_chown)
+        self.path = self.uid = self.gid = None
+        def get_filename():
+            return 'foo.conf'
+        conf = config.IniBasedConfig(get_filename)
+        conf._write_config_file()
+        self.assertEquals(self.path, 'foo.conf')
+        self.assertTrue(isinstance(self.uid, int))
+        self.assertTrue(isinstance(self.gid, int))
 
 class TestGetUserOptionAs(TestIniConfig):