Merge lp:~vila/bzr/1509156-stricter-readonly into lp:bzr

[Vincent Ladeuil]

Forbid more operations on ReadonlyTransportDecorator

This fixes bug #1509156 by forbidding rename() and open_write_stream() to be called on a transport decorated with ReadonlyTransportDecorator.

This ensures such transports can't be misused but so far they only allowed renaming existing files.

So this fix mostly closes the remaining holes but doesn't change the intent of the decorator.

[Richard Wilbur (richard-wilbur)]

I think this is a great idea, Vincent. 9 hours after you made the merge proposal, the diff still doesn't show! ("Updating diff...
An updated diff will be available in a few minutes. Reload to see the changes.")

Sounds like something is stuck. I don't know that approving it will be sufficient to merge it at this point when it is confused about the changes.

[Vincent Ladeuil (vila)]

The bug was embargoed for security reasons but really, after diagnosing, there is no security issue there.

[Richard Wilbur (richard-wilbur)]

Looks like this is good for security! I approve.
+2

I am curious about the change in exception type raised by
bzrlib/transport/readonly.py: get_smart_client()
- NoSmartServer
+ NoSmartMedium

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[Richard Wilbur (richard-wilbur)]

Thank you for cleaning up a bit of code rot!

=== modified file 'bzrlib/tests/per_transport.py'
--- bzrlib/tests/per_transport.py	2011-12-23 19:38:22 +0000
+++ bzrlib/tests/per_transport.py	2016-01-21 17:36:38 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2005-2011 Canonical Ltd
+# Copyright (C) 2005-2011, 2015 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -630,10 +630,13 @@
     def test_opening_a_file_stream_can_set_mode(self):
         t = self.get_transport()
         if t.is_readonly():
+            self.assertRaises((TransportNotPossible, NotImplementedError),
+                              t.open_write_stream, 'foo')
             return
         if not t._can_roundtrip_unix_modebits():
             # Can't roundtrip, so no need to run this test
             return
+
         def check_mode(name, mode, expected):
             handle = t.open_write_stream(name, mode=mode)
             handle.close()
@@ -921,7 +924,9 @@
     def test_rename_dir_succeeds(self):
         t = self.get_transport()
         if t.is_readonly():
-            raise TestSkipped("transport is readonly")
+            self.assertRaises((TransportNotPossible, NotImplementedError),
+                              t.rename, 'foo', 'bar')
+            return
         t.mkdir('adir')
         t.mkdir('adir/asubdir')
         t.rename('adir', 'bdir')
@@ -932,7 +937,9 @@
         """Attempting to replace a nonemtpy directory should fail"""
         t = self.get_transport()
         if t.is_readonly():
-            raise TestSkipped("transport is readonly")
+            self.assertRaises((TransportNotPossible, NotImplementedError),
+                              t.rename, 'foo', 'bar')
+            return
         t.mkdir('adir')
         t.mkdir('adir/asubdir')
         t.mkdir('bdir')
 
=== modified file 'bzrlib/tests/test_smart_transport.py'
--- bzrlib/tests/test_smart_transport.py	2014-02-07 10:20:38 +0000
+++ bzrlib/tests/test_smart_transport.py	2016-01-21 17:36:38 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2011 Canonical Ltd
+# Copyright (C) 2006-2015 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1640,6 +1640,21 @@
         self.assertRaises(errors.TransportNotPossible, self.transport.mkdir,
             'foo')
 
+    def test_rename_error_readonly(self):
+        """TransportNotPossible should be preserved from the backing transport."""
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
+        self.start_server(readonly=True)
+        self.assertRaises(errors.TransportNotPossible, self.transport.rename,
+                          'foo', 'bar')
+
+    def test_open_write_stream_error_readonly(self):
+        """TransportNotPossible should be preserved from the backing transport."""
+        self.overrideEnv('BZR_NO_SMART_VFS', None)
+        self.start_server(readonly=True)
+        self.assertRaises(
+            errors.TransportNotPossible, self.transport.open_write_stream,
+            'foo')
+
 
 class TestServerHooks(SmartTCPTests):
 
 
=== modified file 'bzrlib/transport/readonly.py'
--- bzrlib/transport/readonly.py	2011-12-19 13:23:58 +0000
+++ bzrlib/transport/readonly.py	2016-01-21 17:36:38 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006, 2009 Canonical Ltd
+# Copyright (C) 2006, 2007, 2009, 2010, 2011, 2015 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -41,6 +41,10 @@
         """Readonly transport decorators are invoked via 'readonly+'"""
         return 'readonly+'
 
+    def rename(self, rel_from, rel_to):
+        """See Transport.rename."""
+        raise TransportNotPossible('readonly transport')
+
     def delete(self, relpath):
         """See Transport.delete()."""
         raise TransportNotPossible('readonly transport')
@@ -61,6 +65,10 @@
         """See Transport.mkdir()."""
         raise TransportNotPossible('readonly transport')
 
+    def open_write_stream(self, relpath, mode=None):
+        """See Transport.open_write_stream()."""
+        raise TransportNotPossible('readonly transport')
+
     def is_readonly(self):
         """See Transport.is_readonly."""
         return True
@@ -74,7 +82,7 @@
         raise TransportNotPossible('readonly transport')
 
     def get_smart_client(self):
-        raise NoSmartServer(self.base)
+        raise NoSmartMedium(self)
 
     def get_smart_medium(self):
         raise NoSmartMedium(self)
@@ -83,4 +91,4 @@
 def get_test_permutations():
     """Return the permutations to be used in testing."""
     from bzrlib.tests import test_server
-    return [(ReadonlyTransportDecorator, test_server.ReadonlyServer),]
+    return [(ReadonlyTransportDecorator, test_server.ReadonlyServer)]
 
=== modified file 'doc/en/release-notes/bzr-2.7.txt'
--- doc/en/release-notes/bzr-2.7.txt	2016-01-21 11:42:23 +0000
+++ doc/en/release-notes/bzr-2.7.txt	2016-01-21 17:36:38 +0000
@@ -46,6 +46,9 @@
 * Fix pyrex version checking to be more robust.
   (Andrew Starr-Bochicchio, #1030521 )
 
+* Forbid more operations for ReadonlyTransportDecorator so no more write
+  methods can be used my mistake.  (Vincent Ladeuil, #150196)
+
 Documentation
 *************