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

Proposed by Vincent Ladeuil on 2016-01-21
Status: Merged
Approved by: Vincent Ladeuil on 2016-01-21
Approved revision: 6609
Merged at revision: 6612
Proposed branch: lp:~vila/bzr/1509156-stricter-readonly
Merge into: lp:bzr
Diff against target: 137 lines (+40/-7)
4 files modified
bzrlib/tests/per_transport.py (+10/-3)
bzrlib/tests/test_smart_transport.py (+16/-1)
bzrlib/transport/readonly.py (+11/-3)
doc/en/release-notes/bzr-2.7.txt (+3/-0)
To merge this branch: bzr merge lp:~vila/bzr/1509156-stricter-readonly
Reviewer Review Type Date Requested Status
Richard Wilbur 2016-01-21 Approve on 2016-01-21
Review via email: mp+283512@code.launchpad.net

This proposal supersedes a proposal from 2016-01-21.

Commit message

Forbid more operations on ReadonlyTransportDecorator

Description of the change

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.

To post a comment you must log in.
Richard Wilbur (richard-wilbur) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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

6609. By Vincent Ladeuil on 2016-01-21

Complete the explanation.

Richard Wilbur (richard-wilbur) wrote :

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) :
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Richard Wilbur (richard-wilbur) wrote :

Thank you for cleaning up a bit of code rot!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1# Copyright (C) 2005-2011 Canonical Ltd1# Copyright (C) 2005-2011, 2015 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -630,10 +630,13 @@
630 def test_opening_a_file_stream_can_set_mode(self):630 def test_opening_a_file_stream_can_set_mode(self):
631 t = self.get_transport()631 t = self.get_transport()
632 if t.is_readonly():632 if t.is_readonly():
633 self.assertRaises((TransportNotPossible, NotImplementedError),
634 t.open_write_stream, 'foo')
633 return635 return
634 if not t._can_roundtrip_unix_modebits():636 if not t._can_roundtrip_unix_modebits():
635 # Can't roundtrip, so no need to run this test637 # Can't roundtrip, so no need to run this test
636 return638 return
639
637 def check_mode(name, mode, expected):640 def check_mode(name, mode, expected):
638 handle = t.open_write_stream(name, mode=mode)641 handle = t.open_write_stream(name, mode=mode)
639 handle.close()642 handle.close()
@@ -921,7 +924,9 @@
921 def test_rename_dir_succeeds(self):924 def test_rename_dir_succeeds(self):
922 t = self.get_transport()925 t = self.get_transport()
923 if t.is_readonly():926 if t.is_readonly():
924 raise TestSkipped("transport is readonly")927 self.assertRaises((TransportNotPossible, NotImplementedError),
928 t.rename, 'foo', 'bar')
929 return
925 t.mkdir('adir')930 t.mkdir('adir')
926 t.mkdir('adir/asubdir')931 t.mkdir('adir/asubdir')
927 t.rename('adir', 'bdir')932 t.rename('adir', 'bdir')
@@ -932,7 +937,9 @@
932 """Attempting to replace a nonemtpy directory should fail"""937 """Attempting to replace a nonemtpy directory should fail"""
933 t = self.get_transport()938 t = self.get_transport()
934 if t.is_readonly():939 if t.is_readonly():
935 raise TestSkipped("transport is readonly")940 self.assertRaises((TransportNotPossible, NotImplementedError),
941 t.rename, 'foo', 'bar')
942 return
936 t.mkdir('adir')943 t.mkdir('adir')
937 t.mkdir('adir/asubdir')944 t.mkdir('adir/asubdir')
938 t.mkdir('bdir')945 t.mkdir('bdir')
939946
=== 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 @@
1# Copyright (C) 2006-2011 Canonical Ltd1# Copyright (C) 2006-2015 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -1640,6 +1640,21 @@
1640 self.assertRaises(errors.TransportNotPossible, self.transport.mkdir,1640 self.assertRaises(errors.TransportNotPossible, self.transport.mkdir,
1641 'foo')1641 'foo')
16421642
1643 def test_rename_error_readonly(self):
1644 """TransportNotPossible should be preserved from the backing transport."""
1645 self.overrideEnv('BZR_NO_SMART_VFS', None)
1646 self.start_server(readonly=True)
1647 self.assertRaises(errors.TransportNotPossible, self.transport.rename,
1648 'foo', 'bar')
1649
1650 def test_open_write_stream_error_readonly(self):
1651 """TransportNotPossible should be preserved from the backing transport."""
1652 self.overrideEnv('BZR_NO_SMART_VFS', None)
1653 self.start_server(readonly=True)
1654 self.assertRaises(
1655 errors.TransportNotPossible, self.transport.open_write_stream,
1656 'foo')
1657
16431658
1644class TestServerHooks(SmartTCPTests):1659class TestServerHooks(SmartTCPTests):
16451660
16461661
=== 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 @@
1# Copyright (C) 2006, 2009 Canonical Ltd1# Copyright (C) 2006, 2007, 2009, 2010, 2011, 2015 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -41,6 +41,10 @@
41 """Readonly transport decorators are invoked via 'readonly+'"""41 """Readonly transport decorators are invoked via 'readonly+'"""
42 return 'readonly+'42 return 'readonly+'
4343
44 def rename(self, rel_from, rel_to):
45 """See Transport.rename."""
46 raise TransportNotPossible('readonly transport')
47
44 def delete(self, relpath):48 def delete(self, relpath):
45 """See Transport.delete()."""49 """See Transport.delete()."""
46 raise TransportNotPossible('readonly transport')50 raise TransportNotPossible('readonly transport')
@@ -61,6 +65,10 @@
61 """See Transport.mkdir()."""65 """See Transport.mkdir()."""
62 raise TransportNotPossible('readonly transport')66 raise TransportNotPossible('readonly transport')
6367
68 def open_write_stream(self, relpath, mode=None):
69 """See Transport.open_write_stream()."""
70 raise TransportNotPossible('readonly transport')
71
64 def is_readonly(self):72 def is_readonly(self):
65 """See Transport.is_readonly."""73 """See Transport.is_readonly."""
66 return True74 return True
@@ -74,7 +82,7 @@
74 raise TransportNotPossible('readonly transport')82 raise TransportNotPossible('readonly transport')
7583
76 def get_smart_client(self):84 def get_smart_client(self):
77 raise NoSmartServer(self.base)85 raise NoSmartMedium(self)
7886
79 def get_smart_medium(self):87 def get_smart_medium(self):
80 raise NoSmartMedium(self)88 raise NoSmartMedium(self)
@@ -83,4 +91,4 @@
83def get_test_permutations():91def get_test_permutations():
84 """Return the permutations to be used in testing."""92 """Return the permutations to be used in testing."""
85 from bzrlib.tests import test_server93 from bzrlib.tests import test_server
86 return [(ReadonlyTransportDecorator, test_server.ReadonlyServer),]94 return [(ReadonlyTransportDecorator, test_server.ReadonlyServer)]
8795
=== 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 @@
46* Fix pyrex version checking to be more robust.46* Fix pyrex version checking to be more robust.
47 (Andrew Starr-Bochicchio, #1030521 )47 (Andrew Starr-Bochicchio, #1030521 )
4848
49* Forbid more operations for ReadonlyTransportDecorator so no more write
50 methods can be used my mistake. (Vincent Ladeuil, #150196)
51
49Documentation52Documentation
50*************53*************
5154