From b9eda4de6a4e1d3ece6040d7eae2cbefdbfb71ac Mon Sep 17 00:00:00 2001 From: david-sarah Date: Wed, 27 Jan 2010 15:06:42 -0800 Subject: [PATCH] Address comments by Kevan on 833 and add test for stripping spaces --- docs/frontends/webapi.txt | 14 ++++-- src/allmydata/dirnode.py | 16 +++--- src/allmydata/test/test_dirnode.py | 77 +++++++++++++++++++++++++++++ src/allmydata/test/test_filenode.py | 36 +++++++------- src/allmydata/test/test_web.py | 70 ++++++++++++++++++++------ 5 files changed, 170 insertions(+), 43 deletions(-) diff --git a/docs/frontends/webapi.txt b/docs/frontends/webapi.txt index 12435f4e..6e698990 100644 --- a/docs/frontends/webapi.txt +++ b/docs/frontends/webapi.txt @@ -743,8 +743,14 @@ PUT /uri/$DIRCAP/[SUBDIRS../]CHILDNAME?t=uri Note that this operation does not take its child cap in the form of separate "rw_uri" and "ro_uri" fields. Therefore, it cannot accept a - child cap in a format unknown to the webapi server, because the server - is not able to attenuate an unknown write cap to a read cap. + child cap in a format unknown to the webapi server, unless its URI + starts with "ro." or "imm.". This restriction is necessary because the + server is not able to attenuate an unknown write cap to a read cap. + Unknown URIs starting with "ro." or "imm.", on the other hand, are + assumed to represent read caps. The client should not prefix a write + cap with "ro." or "imm." and pass it to this operation, since that + would result in granting the cap's write authority to holders of the + directory read cap. === Adding multiple files or directories to a parent directory at once === @@ -1028,7 +1034,9 @@ POST /uri/$DIRCAP/[SUBDIRS../]?t=uri&name=CHILDNAME&uri=CHILDCAP This attaches a given read- or write- cap "CHILDCAP" to the designated directory, with a specified child name. This behaves much like the PUT t=uri - operation, and is a lot like a UNIX hardlink. + operation, and is a lot like a UNIX hardlink. It is subject to the same + restrictions as that operation on the use of cap formats unknown to the + webapi server. This will create additional intermediate directories as necessary, although since it is expected to be triggered by a form that was retrieved by "GET diff --git a/src/allmydata/dirnode.py b/src/allmydata/dirnode.py index 2c1f0dd8..ebdc0fa7 100644 --- a/src/allmydata/dirnode.py +++ b/src/allmydata/dirnode.py @@ -265,23 +265,23 @@ class DirectoryNode: while position < len(data): entries, position = split_netstring(data, 1, position) entry = entries[0] - (name, ro_uri, rwcapdata, metadata_s), subpos = split_netstring(entry, 4) + (name_utf8, ro_uri, rwcapdata, metadata_s), subpos = split_netstring(entry, 4) if not mutable and len(rwcapdata) > 0: raise ValueError("the rwcapdata field of a dirnode in an immutable directory was not empty") - name = name.decode("utf-8") + name = name_utf8.decode("utf-8") rw_uri = "" if writeable: rw_uri = self._decrypt_rwcapdata(rwcapdata) # Since the encryption uses CTR mode, it currently leaks the length of the # plaintext rw_uri -- and therefore whether it is present, i.e. whether the - # dirnode is writeable (ticket #925). By stripping spaces in Tahoe >= 1.6.0, - # we may make it easier for future versions to plug this leak. + # dirnode is writeable (ticket #925). By stripping trailing spaces in + # Tahoe >= 1.6.0, we may make it easier for future versions to plug this leak. # ro_uri is treated in the same way for consistency. # rw_uri and ro_uri will be either None or a non-empty string. - rw_uri = rw_uri.strip(' ') or None - ro_uri = ro_uri.strip(' ') or None + rw_uri = rw_uri.rstrip(' ') or None + ro_uri = ro_uri.rstrip(' ') or None try: child = self._create_and_validate_node(rw_uri, ro_uri, name) @@ -292,11 +292,11 @@ class DirectoryNode: children.set_with_aux(name, (child, metadata), auxilliary=entry) else: log.msg(format="mutable cap for child '%(name)s' unpacked from an immutable directory", - name=name.encode("utf-8"), + name=name_utf8, facility="tahoe.webish", level=log.UNUSUAL) except CapConstraintError, e: log.msg(format="unmet constraint on cap for child '%(name)s' unpacked from a directory:\n" - "%(message)s", message=e.args[0], name=name.encode("utf-8"), + "%(message)s", message=e.args[0], name=name_utf8, facility="tahoe.webish", level=log.UNUSUAL) return children diff --git a/src/allmydata/test/test_dirnode.py b/src/allmydata/test/test_dirnode.py index 1acfdb5f..6adf91ab 100644 --- a/src/allmydata/test/test_dirnode.py +++ b/src/allmydata/test/test_dirnode.py @@ -13,6 +13,7 @@ from allmydata.interfaces import IImmutableFileNode, IMutableFileNode, \ from allmydata.mutable.filenode import MutableFileNode from allmydata.mutable.common import UncoordinatedWriteError from allmydata.util import hashutil, base32 +from allmydata.util.netstring import split_netstring from allmydata.monitor import Monitor from allmydata.test.common import make_chk_file_uri, make_mutable_file_uri, \ ErrorMixin @@ -48,6 +49,7 @@ class Dirnode(GridTestMixin, unittest.TestCase, self.set_up_grid() c = self.g.clients[0] nm = c.nodemaker + setup_py_uri = "URI:CHK:n7r3m6wmomelk4sep3kw5cvduq:os7ijw5c3maek7pg65e5254k2fzjflavtpejjyhshpsxuqzhcwwq:3:20:14861" one_uri = "URI:LIT:n5xgk" # LIT for "one" mut_write_uri = "URI:SSK:vfvcbdfbszyrsaxchgevhmmlii:euw4iw7bbnkrrwpzuburbhppuxhc3gwxv26f6imekhz7zyw2ojnq" @@ -115,6 +117,8 @@ class Dirnode(GridTestMixin, unittest.TestCase, bad_future_node = UnknownNode(future_write_uri, None) bad_kids1 = {u"one": (bad_future_node, {})} + # This should fail because we don't know how to diminish the future_write_uri + # cap (given in a write slot and not prefixed with "ro." or "imm.") to a readcap. d.addCallback(lambda ign: self.shouldFail(MustNotBeUnknownRWError, "bad_kids1", "cannot attach unknown", @@ -133,6 +137,7 @@ class Dirnode(GridTestMixin, unittest.TestCase, self.set_up_grid() c = self.g.clients[0] nm = c.nodemaker + setup_py_uri = "URI:CHK:n7r3m6wmomelk4sep3kw5cvduq:os7ijw5c3maek7pg65e5254k2fzjflavtpejjyhshpsxuqzhcwwq:3:20:14861" one_uri = "URI:LIT:n5xgk" # LIT for "one" mut_write_uri = "URI:SSK:vfvcbdfbszyrsaxchgevhmmlii:euw4iw7bbnkrrwpzuburbhppuxhc3gwxv26f6imekhz7zyw2ojnq" @@ -280,6 +285,75 @@ class Dirnode(GridTestMixin, unittest.TestCase, d.addCallback(_made_parent) return d + def test_spaces_are_stripped_on_the_way_out(self): + self.basedir = "dirnode/Dirnode/test_spaces_are_stripped_on_the_way_out" + self.set_up_grid() + c = self.g.clients[0] + nm = c.nodemaker + + # This test checks that any trailing spaces in URIs are retained in the + # encoded directory, but stripped when we get them out of the directory. + # See ticket #925 for why we want that. + + stripped_write_uri = "lafs://from_the_future\t" + stripped_read_uri = "lafs://readonly_from_the_future\t" + spacedout_write_uri = stripped_write_uri + " " + spacedout_read_uri = stripped_read_uri + " " + + child = nm.create_from_cap(spacedout_write_uri, spacedout_read_uri) + self.failUnlessEqual(child.get_write_uri(), spacedout_write_uri) + self.failUnlessEqual(child.get_readonly_uri(), "ro." + spacedout_read_uri) + + kids = {u"child": (child, {})} + d = c.create_dirnode(kids) + + def _created(dn): + self.failUnless(isinstance(dn, dirnode.DirectoryNode)) + self.failUnless(dn.is_mutable()) + self.failIf(dn.is_readonly()) + dn.raise_error() + self.cap = dn.get_cap() + self.rootnode = dn + return dn._node.download_best_version() + d.addCallback(_created) + + def _check_data(data): + # Decode the netstring representation of the directory to check that the + # spaces are retained when the URIs are stored. + position = 0 + numkids = 0 + while position < len(data): + entries, position = split_netstring(data, 1, position) + entry = entries[0] + (name_utf8, ro_uri, rwcapdata, metadata_s), subpos = split_netstring(entry, 4) + name = name_utf8.decode("utf-8") + rw_uri = self.rootnode._decrypt_rwcapdata(rwcapdata) + self.failUnless(name in kids) + (expected_child, ign) = kids[name] + self.failUnlessEqual(rw_uri, expected_child.get_write_uri()) + self.failUnlessEqual("ro." + ro_uri, expected_child.get_readonly_uri()) + numkids += 1 + + self.failUnlessEqual(numkids, 1) + return self.rootnode.list() + d.addCallback(_check_data) + + # Now when we use the real directory listing code, the trailing spaces + # should have been stripped (and "ro." should have been prepended to the + # ro_uri, since it's unknown). + def _check_kids(children): + self.failUnlessEqual(sorted(children.keys()), [u"child"]) + child_node, child_metadata = children[u"child"] + + self.failUnlessEqual(child_node.get_write_uri(), stripped_write_uri) + self.failUnlessEqual(child_node.get_readonly_uri(), "ro." + stripped_read_uri) + d.addCallback(_check_kids) + + d.addCallback(lambda ign: nm.create_from_cap(self.cap.to_string())) + d.addCallback(lambda n: n.list()) + d.addCallback(_check_kids) # again with dirnode recreated from cap + return d + def test_check(self): self.basedir = "dirnode/Dirnode/test_check" self.set_up_grid() @@ -1121,6 +1195,9 @@ class FakeMutableFile: def is_allowed_in_immutable_directory(self): return False + def raise_error(self): + pass + def modify(self, modifier): self.data = modifier(self.data, None, True) return defer.succeed(None) diff --git a/src/allmydata/test/test_filenode.py b/src/allmydata/test/test_filenode.py index 6106a2f8..983896f4 100644 --- a/src/allmydata/test/test_filenode.py +++ b/src/allmydata/test/test_filenode.py @@ -39,10 +39,10 @@ class Node(unittest.TestCase): self.failUnlessEqual(fn1.get_uri(), u.to_string()) self.failUnlessEqual(fn1.get_cap(), u) self.failUnlessEqual(fn1.get_readcap(), u) - self.failUnlessEqual(fn1.is_readonly(), True) - self.failUnlessEqual(fn1.is_mutable(), False) - self.failUnlessEqual(fn1.is_unknown(), False) - self.failUnlessEqual(fn1.is_allowed_in_immutable_directory(), True) + self.failUnless(fn1.is_readonly()) + self.failIf(fn1.is_mutable()) + self.failIf(fn1.is_unknown()) + self.failUnless(fn1.is_allowed_in_immutable_directory()) self.failUnlessEqual(fn1.get_write_uri(), None) self.failUnlessEqual(fn1.get_readonly_uri(), u.to_string()) self.failUnlessEqual(fn1.get_size(), 1000) @@ -54,8 +54,8 @@ class Node(unittest.TestCase): v = fn1.get_verify_cap() self.failUnless(isinstance(v, uri.CHKFileVerifierURI)) self.failUnlessEqual(fn1.get_repair_cap(), v) - self.failUnlessEqual(v.is_readonly(), True) - self.failUnlessEqual(v.is_mutable(), False) + self.failUnless(v.is_readonly()) + self.failIf(v.is_mutable()) def test_literal_filenode(self): @@ -69,10 +69,10 @@ class Node(unittest.TestCase): self.failUnlessEqual(fn1.get_uri(), u.to_string()) self.failUnlessEqual(fn1.get_cap(), u) self.failUnlessEqual(fn1.get_readcap(), u) - self.failUnlessEqual(fn1.is_readonly(), True) - self.failUnlessEqual(fn1.is_mutable(), False) - self.failUnlessEqual(fn1.is_unknown(), False) - self.failUnlessEqual(fn1.is_allowed_in_immutable_directory(), True) + self.failUnless(fn1.is_readonly()) + self.failIf(fn1.is_mutable()) + self.failIf(fn1.is_unknown()) + self.failUnless(fn1.is_allowed_in_immutable_directory()) self.failUnlessEqual(fn1.get_write_uri(), None) self.failUnlessEqual(fn1.get_readonly_uri(), u.to_string()) self.failUnlessEqual(fn1.get_size(), len(DATA)) @@ -122,10 +122,10 @@ class Node(unittest.TestCase): self.failUnlessEqual(n.get_readonly_uri(), u.get_readonly().to_string()) self.failUnlessEqual(n.get_cap(), u) self.failUnlessEqual(n.get_readcap(), u.get_readonly()) - self.failUnlessEqual(n.is_mutable(), True) - self.failUnlessEqual(n.is_readonly(), False) - self.failUnlessEqual(n.is_unknown(), False) - self.failUnlessEqual(n.is_allowed_in_immutable_directory(), False) + self.failUnless(n.is_mutable()) + self.failIf(n.is_readonly()) + self.failIf(n.is_unknown()) + self.failIf(n.is_allowed_in_immutable_directory()) n.raise_error() n2 = MutableFileNode(None, None, client.get_encoding_parameters(), @@ -144,10 +144,10 @@ class Node(unittest.TestCase): self.failUnlessEqual(nro.get_readonly(), nro) self.failUnlessEqual(nro.get_cap(), u.get_readonly()) self.failUnlessEqual(nro.get_readcap(), u.get_readonly()) - self.failUnlessEqual(nro.is_mutable(), True) - self.failUnlessEqual(nro.is_readonly(), True) - self.failUnlessEqual(nro.is_unknown(), False) - self.failUnlessEqual(nro.is_allowed_in_immutable_directory(), False) + self.failUnless(nro.is_mutable()) + self.failUnless(nro.is_readonly()) + self.failIf(nro.is_unknown()) + self.failIf(nro.is_allowed_in_immutable_directory()) nro_u = nro.get_uri() self.failUnlessEqual(nro_u, nro.get_readonly_uri()) self.failUnlessEqual(nro_u, u.get_readonly().to_string()) diff --git a/src/allmydata/test/test_web.py b/src/allmydata/test/test_web.py index 416459b6..ad633384 100644 --- a/src/allmydata/test/test_web.py +++ b/src/allmydata/test/test_web.py @@ -2376,7 +2376,7 @@ class Web(WebMixin, WebErrorMixin, testutil.StallMixin, unittest.TestCase): def test_POST_set_children_with_hyphen(self): return self.test_POST_set_children(command_name="set-children") - def test_POST_put_uri(self): + def test_POST_link_uri(self): contents, n, newuri = self.makefile(8) d = self.POST(self.public_url + "/foo", t="uri", name="new.txt", uri=newuri) d.addCallback(self.failUnlessURIMatchesROChild, self._foo_node, u"new.txt") @@ -2385,7 +2385,7 @@ class Web(WebMixin, WebErrorMixin, testutil.StallMixin, unittest.TestCase): contents)) return d - def test_POST_put_uri_replace(self): + def test_POST_link_uri_replace(self): contents, n, newuri = self.makefile(8) d = self.POST(self.public_url + "/foo", t="uri", name="bar.txt", uri=newuri) d.addCallback(self.failUnlessURIMatchesROChild, self._foo_node, u"bar.txt") @@ -2394,12 +2394,33 @@ class Web(WebMixin, WebErrorMixin, testutil.StallMixin, unittest.TestCase): contents)) return d - def test_POST_put_uri_no_replace_queryarg(self): + def test_POST_link_uri_unknown_bad(self): + newuri = "lafs://from_the_future" + d = self.POST(self.public_url + "/foo", t="uri", name="future.txt", uri=newuri) + d.addBoth(self.shouldFail, error.Error, + "POST_link_uri_unknown_bad", + "400 Bad Request", + "unknown cap in a write slot") + return d + + def test_POST_link_uri_unknown_ro_good(self): + newuri = "ro.lafs://readonly_from_the_future" + d = self.POST(self.public_url + "/foo", t="uri", name="future-ro.txt", uri=newuri) + d.addCallback(self.failUnlessURIMatchesROChild, self._foo_node, u"future-ro.txt") + return d + + def test_POST_link_uri_unknown_imm_good(self): + newuri = "imm.lafs://immutable_from_the_future" + d = self.POST(self.public_url + "/foo", t="uri", name="future-imm.txt", uri=newuri) + d.addCallback(self.failUnlessURIMatchesROChild, self._foo_node, u"future-imm.txt") + return d + + def test_POST_link_uri_no_replace_queryarg(self): contents, n, newuri = self.makefile(8) d = self.POST(self.public_url + "/foo?replace=false", t="uri", name="bar.txt", uri=newuri) d.addBoth(self.shouldFail, error.Error, - "POST_put_uri_no_replace_queryarg", + "POST_link_uri_no_replace_queryarg", "409 Conflict", "There was already a child by that name, and you asked me " "to not replace it") @@ -2407,12 +2428,12 @@ class Web(WebMixin, WebErrorMixin, testutil.StallMixin, unittest.TestCase): d.addCallback(self.failUnlessIsBarDotTxt) return d - def test_POST_put_uri_no_replace_field(self): + def test_POST_link_uri_no_replace_field(self): contents, n, newuri = self.makefile(8) d = self.POST(self.public_url + "/foo", t="uri", replace="false", name="bar.txt", uri=newuri) d.addBoth(self.shouldFail, error.Error, - "POST_put_uri_no_replace_field", + "POST_link_uri_no_replace_field", "409 Conflict", "There was already a child by that name, and you asked me " "to not replace it") @@ -2680,6 +2701,29 @@ class Web(WebMixin, WebErrorMixin, testutil.StallMixin, unittest.TestCase): "to not replace it") return d + def test_PUT_NEWFILEURL_uri_unknown_bad(self): + new_uri = "lafs://from_the_future" + d = self.PUT(self.public_url + "/foo/put-future.txt?t=uri", new_uri) + d.addBoth(self.shouldFail, error.Error, + "POST_put_uri_unknown_bad", + "400 Bad Request", + "unknown cap in a write slot") + return d + + def test_PUT_NEWFILEURL_uri_unknown_ro_good(self): + new_uri = "ro.lafs://readonly_from_the_future" + d = self.PUT(self.public_url + "/foo/put-future-ro.txt?t=uri", new_uri) + d.addCallback(self.failUnlessURIMatchesROChild, self._foo_node, + u"put-future-ro.txt") + return d + + def test_PUT_NEWFILEURL_uri_unknown_imm_good(self): + new_uri = "imm.lafs://immutable_from_the_future" + d = self.PUT(self.public_url + "/foo/put-future-imm.txt?t=uri", new_uri) + d.addCallback(self.failUnlessURIMatchesROChild, self._foo_node, + u"put-future-imm.txt") + return d + def test_PUT_NEWFILE_URI(self): file_contents = "New file contents here\n" d = self.PUT("/uri", file_contents) @@ -3360,15 +3404,13 @@ class Grid(GridTestMixin, WebErrorMixin, unittest.TestCase, ShouldFailMixin): while position < len(data): entries, position = split_netstring(data, 1, position) entry = entries[0] - (name, ro_uri, rwcapdata, metadata_s), subpos = split_netstring(entry, 4) - name = name.decode("utf-8") + (name_utf8, ro_uri, rwcapdata, metadata_s), subpos = split_netstring(entry, 4) + name = name_utf8.decode("utf-8") self.failUnless(rwcapdata == "") - ro_uri = ro_uri.strip() - if name in kids: - self.failIfEqual(ro_uri, "") - (expected_child, ign) = kids[name] - self.failUnlessEqual(ro_uri, expected_child.get_readonly_uri()) - numkids += 1 + self.failUnless(name in kids) + (expected_child, ign) = kids[name] + self.failUnlessEqual(ro_uri, expected_child.get_readonly_uri()) + numkids += 1 self.failUnlessEqual(numkids, 3) return self.rootnode.list() -- 2.45.2