[tahoe-dev] [tahoe-lafs] #833: reject mutable children when *reading* an immutable dirnode

[tahoe-lafs]

#833: reject mutable children when *reading* an immutable dirnode
--------------------------------------------------------------------------------------------------+
 Reporter:  warner                                                                                |           Owner:  kevan   
     Type:  defect                                                                                |          Status:  assigned
 Priority:  critical                                                                              |       Milestone:  1.6.0   
Component:  code-dirnodes                                                                         |         Version:  1.5.0   
 Keywords:  integrity forward-compatibility backward-compatibility confidentiality review-needed  |   Launchpad_bug:          
--------------------------------------------------------------------------------------------------+

Comment(by davidsarah):

 > interfaces.py looks okay, but I was a bit confused when I read "...and
 then used" at the end of MustBeDeepImmutableError and MustBeReadOnlyError.
 This is possibly me being dense after work, but maybe you could clarify
 the context in which those are being used to trigger that error?

 An UnknownNode may have one of these error objects in its .error property.
 They will only be thrown if node.raise_error() is called. This happens
 when we try to put the node into a directory (besides tests, the relevant
 calls to raise_error are in dirnode.py and nodemaker.py).

 >  The docstring for {{{strip_prefix_for_ro}}} should be inside the
 function, not before it.

 OK.

 >  In the {{{__init__}}} method of UnknownNode, you say {{{if x is not
 None:}}} instead of {{{if x}}}. I'm assuming that that's to deal with
 empty string arguments/other arguments that evaluate to False in a
 comparison?

 Yes. I'll recheck what happens to falsy URIs.

 {{{
 >  if given_ro_uri is not None:
 >              read_cap = uri.from_string(given_ro_uri,
 >  deep_immutable=deep_immutable, name=name)
 >              if isinstance(read_cap, uri.UnknownURI):
 >                  self.error = read_cap.get_error()
 >                  if self.error:
 >                      assert self.rw_uri is None and self.ro_uri is None
 >                      return
 }}}

 >  How does this know the difference between an UnknownURI that is an
 UnknownURI because we don't know what it is, and an UnknownURI that is
 unknown because we know what it is but it is prefixed with something that
 makes it nonsensical?

 In either case, isinstance(read_cap, uri.UnknownURI) will be true. But
 only in the latter case will read_cap.get_error() be truthy.

 {{{
 >  if deep_immutable:
 >              assert self.rw_uri is None
 ...
 }}}
 >  What is this assertion meant to do? I don't think that anything so far
 in the code would have assigned anything other than none to self.rw_uri. I
 don't suppose that it hurts to have it there, aside from maybe meaning
 that the calling code needs to be more complicated if it wants to handle
 all of the error conditions that UnknownNode can enter. It also serves as
 self-documentation, I guess.

 It's intended as documentation. I normally use asserts only for
 documentation, not for validity checks.

 >  Any reason for implementing {{{is_unknown}}} in {{{_BaseURI}}}? Is it
 anything that might need to be added to the {{{IURI}}} interface?

 No, I'll remove that. (I originally had an is_unknown method on URIs but
 changed to using isinstance(..., UnknownURI).)

 >  What about {{{is_readonly}}}, {{{is_mutable}}}, {{{get_readonly}}},
 {{{get_verify_cap}}}, which were added to a number of URIs in uri.py.
 Should they be added to one of the URI interfaces?

 The existing code was inconsistent; it had these methods for some URI
 classes and not others. They were already declared in IURI.

 >  The spacing on your changes to SSKVerifierURI is inconsistent with your
 changes in the other URI classes.

 Fixed.

 >  I didn't see any problems in nodemaker.py or dirnode.py.

 >  If you're correcting things in webapi.txt, you might try replacing
 Tahoe with Tahoe-LAFS, which (I gather) is more correct. But maybe that's
 out of scope for this ticket.

 Out of scope (there are > 60 uses of "Tahoe" in webapi.txt!)

 >  I think it would be easier to read if you changed the from_string_
 methods in uri.py to use explicit named keyword arguments instead of the
 **kwargs form.

 Will do.

-- 
Ticket URL: <http://allmydata.org/trac/tahoe/ticket/833#comment:49>
tahoe-lafs <http://allmydata.org>
secure decentralized file storage grid