Merge lp:~jelmer/bzr/config-file-permdenied into lp:bzr

[Jelmer Vernooij]

Print a warning rather than an error when the current user does not have permission to read a configuration file.

If the current user is not allowed to read a configuration file,
warn and continue without it rather than raising an exception.

[Vincent Ladeuil (vila)]

store.load() is used both for reading a config file and before saving it, the behavior should be different.

I've tweaked your submission at lp:~vila/bzr/config-file-permdenied

Also, I think configobj use os.path.isfile and as such should be immune to the issue, but your patch doesn't hurt either so let's land that.

review: Needs Fixing

[Martin Packman (gz)]

+ trace.warning("Permission denied while trying to open "
+ "configuration file %s.", urlutils.unescape_for_display(
+ urlutils.join(self._transport.base, self._filename), "utf-8"))

Again, this results in mangled path on non-utf-8 terminals.

[Jelmer Vernooij (jelmer)]

On 08/31/2011 01:27 AM, Martin [gz] wrote:
> + trace.warning("Permission denied while trying to open "
> + "configuration file %s.", urlutils.unescape_for_display(
> + urlutils.join(self._transport.base, self._filename), "utf-8"))
>
> Again, this results in mangled path on non-utf-8 terminals.
I think that's a separate issue we need to address. trace is defined as
using utf8 at the moment so this is the only sensible thing to do right now.

Ideally we should have trace recode the utf8 to whatever the terminal is
using when writing to stdout, or perhaps have it accept unicode too.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

> On 08/31/2011 01:27 AM, Martin [gz] wrote:
> > Again, this results in mangled path on non-utf-8 terminals.
> I think that's a separate issue we need to address. trace is defined as
> using utf8 at the moment so this is the only sensible thing to do right now.

I agree with mgz that encoding issues need to be addressed too ;)

I agree with jelmer that we need to go forward there ;)

@mgz: Is there a bug tracking this particular issue ?

I want my cake and eat it too: this fix addresses a specific
issue in a highly unsual env which deserves a better *error*
reporting, i.e.: something is already wrong in the setup.

If this env *also* provides a broken terminal, well, I don't mind
giving a mangled path until we are able to give a meaningful
one. If trying to report this error *fails*, then that's more
concerning, but 'mangled', I can live with that (for now ;).

Summary: I think we shouldn't block here. mgz, is that ok ?

[Jelmer Vernooij (jelmer)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-08-29 12:01:49 +0000
+++ bzrlib/config.py	2011-08-30 12:59:32 +0000
@@ -1397,7 +1397,7 @@
             return (self.branch._transport.get_bytes("email")
                     .decode(osutils.get_user_encoding())
                     .rstrip("\r\n"))
-        except errors.NoSuchFile, e:
+        except (errors.NoSuchFile, errors.PermissionDenied), e:
             pass
 
         return self._get_best_value('_get_user_id')
@@ -2278,6 +2278,11 @@
             return f
         except errors.NoSuchFile:
             return StringIO()
+        except errors.PermissionDenied, e:
+            trace.warning("Permission denied while trying to open "
+                "configuration file %s.", urlutils.unescape_for_display(
+                urlutils.join(self._transport.base, self._filename), "utf-8"))
+            return StringIO()
 
     def _external_url(self):
         return urlutils.join(self._transport.external_url(), self._filename)
@@ -2690,7 +2695,12 @@
         """Load the store from the associated file."""
         if self.is_loaded():
             return
-        content = self.transport.get_bytes(self.file_name)
+        try:
+            content = self.transport.get_bytes(self.file_name)
+        except errors.PermissionDenied:
+            trace.warning("Permission denied while trying to load "
+                          "configuration store %s.", self.external_url())
+            raise
         self._load_from_string(content)
         for hook in ConfigHooks['load']:
             hook(self)
@@ -2738,8 +2748,8 @@
         # We need a loaded store
         try:
             self.load()
-        except errors.NoSuchFile:
-            # If the file doesn't exist, there is no sections
+        except (errors.NoSuchFile, errors.PermissionDenied):
+            # If the file can't be read, there is no sections
             return
         cobj = self._config_obj
         if cobj.scalars:

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-08-29 12:01:49 +0000
+++ bzrlib/tests/test_config.py	2011-08-30 12:59:32 +0000
@@ -33,18 +33,14 @@
     errors,
     osutils,
     mail_client,
-    mergetools,
     ui,
     urlutils,
-    registry,
     remote,
     tests,
     trace,
-    transport,
     )
 from bzrlib.symbol_versioning import (
     deprecated_in,
-    deprecated_method,
     )
 from bzrlib.transport import remote as transport_remote
 from bzrlib.tests import (
@@ -1968,6 +1964,29 @@
         conf = config.TransportConfig(t, 'foo.conf')
         self.assertRaises(errors.ParseConfigError, conf._get_configobj)
 
+    def test_load_permission_denied(self):
+        """Ensure we get an empty config file if the file is inaccessible."""
+        warnings = []
+        def warning(*args):
+            warnings.append(args[0] % args[1:])
+        self.overrideAttr(trace, 'warning', warning)
+
+        class DenyingTransport(object):
+
+            def __init__(self, base):
+                self.base = base
+
+            def get_bytes(self, relpath):
+                raise errors.PermissionDenied(relpath, "")
+
+        cfg = config.TransportConfig(
+            DenyingTransport("nonexisting://"), 'control.conf')
+        self.assertIs(None, cfg.get_option('non-existant', 'SECTION'))
+        self.assertEquals(
+            warnings,
+            [u'Permission denied while trying to open configuration file '
+             u'nonexisting:///control.conf.'])
+
     def test_get_value(self):
         """Test that retreiving a value from a section is possible"""
         bzrdir_config = config.TransportConfig(self.get_transport('.'),
@@ -2584,6 +2603,25 @@
         store = config.IniFileStore(t, 'foo.conf')
         self.assertRaises(errors.ParseConfigError, store.load)
 
+    def test_load_permission_denied(self):
+        """Ensure we get warned when trying to load an inaccessible file."""
+        warnings = []
+        def warning(*args):
+            warnings.append(args[0] % args[1:])
+        self.overrideAttr(trace, 'warning', warning)
+
+        t = self.get_transport()
+
+        def get_bytes(relpath):
+            raise errors.PermissionDenied(relpath, "")
+        t.get_bytes = get_bytes
+        store = config.IniFileStore(t, 'foo.conf')
+        self.assertRaises(errors.PermissionDenied, store.load)
+        self.assertEquals(
+            warnings,
+            [u'Permission denied while trying to load configuration store %s.'
+             % store.external_url()])
+
 
 class TestIniConfigContent(tests.TestCaseWithTransport):
     """Simulate loading a IniBasedConfig without content of various encodings.

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-08-27 19:08:54 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-08-30 12:59:32 +0000
@@ -136,6 +136,10 @@
 * Decode ``BZR_HOME`` with fs encoding on posix platforms to avoid unicode
   errors.  (Vincent Ladeuil, #822571)
 
+* Rather than an error being raised, a warning is now printed when the
+  current user does not have permission to read a configuration file.
+  (Jelmer Vernooij, #837324)
+
 * TreeTransformBase.fixup_new_roots no longer forces trees to have a root, so
   operations that use it, like merge, can now create trees without a root.
   (Aaron Bentley)