[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 kevan):

 I've looked through the changes to behavior (with the exception of those
 to the webapi) and documentation, and have the following comments so far.
 I'll probably have more after I start working on the remaining parts, but
 posting what I have now might make this patchset more likely to make it
 into trunk by Thursday.

 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?

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

 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?

 {{{
 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? From what I can understand by reading the code in
 UnknownURI, it is possible to get that result in either case. Apologies if
 this is a case of me being dense.

 {{{
 if deep_immutable:
             assert self.rw_uri is None
             # strengthen the constraint on ro_uri to
 ALLEGED_IMMUTABLE_PREFIX
             if given_ro_uri is not None:
                 if given_ro_uri.startswith(ALLEGED_IMMUTABLE_PREFIX):
                     self.ro_uri = given_ro_uri
                 elif given_ro_uri.startswith(ALLEGED_READONLY_PREFIX):
                     self.ro_uri = ALLEGED_IMMUTABLE_PREFIX +
 given_ro_uri[len(ALLEGED_READONLY_PREFIX):]
                 else:
                     self.ro_uri = ALLEGED_IMMUTABLE_PREFIX + given_ro_uri
 }}}
 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.

 Any reason for implementing {{{is_unknown}}} in {{{_BaseURI}}}? Is it
 anything that might need to be added to the {{{IURI}}} interface? 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 spacing on your changes to SSKVerifierURI is inconsistent with your
 changes in the other URI classes.

 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.

 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.

 I have a block of free time tomorrow, so I'm hoping I will have the rest
 of the review done by then.

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