Martin Aspeli wrote: Martin Aspeli wrote:

Martin Aspeli wrote:

...

Hi,

In my travails through the ZPublisher and WebDAV, I've come up with another fun thing.

As far as I can tell, traversal via acquisition doesn't make any sense for a WebDAV request. If I have /foo and /bar, but not /bar/foo, then traversal to /bar/foo over WebDAV should not acquire /foo and wrap it in /bar.

One reason for this (apart from being utterly confusing) is that it breaks PUT requests to a NullResource: If I try to PUT to /bar/foo it should create a new object there, not overwrite /foo.

There is actually some convoluted support for detecting this in ZPublisher.BaseRequest:

# Note - no_acquire_flag is necessary to support # things like DAV. We have to make sure # that the target object is not acquired # if the request_method is other than GET # or POST. Otherwise, you could never use # PUT to add a new object named 'test' if # an object 'test' existed above it in the # heirarchy -- you'd always get the # existing object :( if (no_acquire_flag and hasattr(parents[1], 'aq_base') and not hasattr(parents[1],'__bobo_traverse__')): if not (hasattr(parents[1].aq_base, entry_name) or parents[1].aq_base.has_key(entry_name)): raise AttributeError, entry_name

This doesn't really work, though. The object is acquired, and then all it does is to check that the PUT() method object is a true attribute of the acquired object. But even then, raising AttributeError is wrong.

What should happen, I think, is that in DefaultPublishTraverse.publishTraverse() we should *not* attempt to acquire for webdav requests. Instead, we should try direct attribute access and then __getitem__() access (which is the next thing it tries after getattr() acquisition). This would then allow the folder to return a NullResource (as OFS.ObjectManager does, for example).

Any objections? It's a relatively simple condition in DfaultPublishTraverse.publishTraverse().

Actually, I found some code that may be trying to address this (although I think it's probably likely that not attempting to acquire in the first place is better).

Basically, it allows acquisition during traversal, but when you get to the end of the path, it does this:

if (no_acquire_flag and hasattr(object, 'aq_base') and not hasattr(object,'__bobo_traverse__')): if object.aq_parent is not object.aq_inner.aq_parent: from webdav.NullResource import NullResource object = NullResource(parents[-2], object.getId(), self).__of__(parents[-2])

This would fix the problem, except in my case, the traversed-to object (/foo in the example above) happens to have a __bobo_traverse__(), as do all CMF types, so the third caluse of the first if statement is false.

I can't see the logic here, though: why would it matter if the acquired object happens to have a __bobo_traverse__?

Removing that condition fixes it for me. This would be an even simpler bug fix (though I still find it fishy that you can acquire folders through WebDAV).

Can anyone explain why that condition is there? Otherwise, I'll rip it out. ;-)

Martin

-- Author of `Professional Plone Development`, a book for developers who want to work with Plone. See http://martinaspeli.net/plone-book

Martin Aspeli wrote:

...

Hi,

In my travails through the ZPublisher and WebDAV, I've come up with another fun thing.

As far as I can tell, traversal via acquisition doesn't make any sense for a WebDAV request. If I have /foo and /bar, but not /bar/foo, then traversal to /bar/foo over WebDAV should not acquire /foo and wrap it in /bar.

One reason for this (apart from being utterly confusing) is that it breaks PUT requests to a NullResource: If I try to PUT to /bar/foo it should create a new object there, not overwrite /foo.

There is actually some convoluted support for detecting this in ZPublisher.BaseRequest:

# Note - no_acquire_flag is necessary to support # things like DAV. We have to make sure # that the target object is not acquired # if the request_method is other than GET # or POST. Otherwise, you could never use # PUT to add a new object named 'test' if # an object 'test' existed above it in the # heirarchy -- you'd always get the # existing object :( if (no_acquire_flag and hasattr(parents[1], 'aq_base') and not hasattr(parents[1],'__bobo_traverse__')): if not (hasattr(parents[1].aq_base, entry_name) or parents[1].aq_base.has_key(entry_name)): raise AttributeError, entry_name

This doesn't really work, though. The object is acquired, and then all it does is to check that the PUT() method object is a true attribute of the acquired object. But even then, raising AttributeError is wrong.

What should happen, I think, is that in DefaultPublishTraverse.publishTraverse() we should *not* attempt to acquire for webdav requests. Instead, we should try direct attribute access and then __getitem__() access (which is the next thing it tries after getattr() acquisition). This would then allow the folder to return a NullResource (as OFS.ObjectManager does, for example).

Any objections? It's a relatively simple condition in DfaultPublishTraverse.publishTraverse().

Actually, I found some code that may be trying to address this (although I think it's probably likely that not attempting to acquire in the first place is better).

Basically, it allows acquisition during traversal, but when you get to the end of the path, it does this:

if (no_acquire_flag and hasattr(object, 'aq_base') and not hasattr(object,'__bobo_traverse__')): if object.aq_parent is not object.aq_inner.aq_parent: from webdav.NullResource import NullResource object = NullResource(parents[-2], object.getId(), self).__of__(parents[-2])

This would fix the problem, except in my case, the traversed-to object (/foo in the example above) happens to have a __bobo_traverse__(), as do all CMF types, so the third caluse of the first if statement is false.

I can't see the logic here, though: why would it matter if the acquired object happens to have a __bobo_traverse__?

Removing that condition fixes it for me. This would be an even simpler bug fix (though I still find it fishy that you can acquire folders through WebDAV).

Can anyone explain why that condition is there? Otherwise, I'll rip it out. ;-)

One more thing to note: __bobo_traverse__() is eventually called on the (wrongly acquired) object, to traverse to the PUT() HTTP verb handler method. I suppose it's possible it could want to do something wildly different, thus making the "don't acquire" condition that returns a NullResource above incorrect but this still seems very convoluted. It would need to do a check like "if aq_parent(self) is not aq_parent(aq_inner(self)) and is_webdav_request: return NullResource(aq_parent(self), self.getId(), request)" even if the name is actually available on the object (in this case, the PUT() method). Demanding that everyone that implements a __bobo_traverse__ does this seems really onerous and error prone. I can't see any situations where you'd want to be able to do this on a resource that was acquired from somewhere other than its containment parent. Martin -- Author of `Professional Plone Development`, a book for developers who want to work with Plone. See http://martinaspeli.net/plone-book

Shane Hathaway <shane <at> hathawaymix.org> writes:

Martin Aspeli wrote:

...

...

Can anyone explain why that condition is there? Otherwise, I'll rip it out.

As I recall, this code is convoluted because it's hard to tell whether an HTTP request is a WebDAV request. If there is now a way to clearly distinguish WebDAV requests, then I imagine this code can be greatly simplified. This code had to deal with Windows 95 and such.

Well, at the very least, you can use the check that says: if request.maybe_webdav_client and \ request['HTTP_REQUEST_METHOD'] not in ('GET', 'POST')": This wouldn't solve the problem for WebDAV GET or POST requests, but that's actually fine. The problem seen here mainly pertain to PUT and PROPFIND requests. This would meant that there's a chance people could get a weird acquisition chain on a GET request that then wouldn't resolve properly (giving a 404) on a subsequent PROPFIND or PUT, but this is very unlikely to happen in practice unless someone (a) overrode listDAVObjects() to return acquired objects or (b) manually entered a WebDAV URL that resulted in acquisition, and which was only used for a GET. Compared to the situation right now where PUT is broken for any file that has the same name as an object higher up in the acquisition chain, not supporting these arguably-invalid edge cases seems a lot better. :) Martin

Tres Seaver <tseaver <at> palladion.com> writes:

There is no way to tell the difference between a WebDAV GET and a "normal" browser GET, period: the specs explicitly, deliberately overload the GET verb.

Hence the IANA-assigned "WebDAV source port"[1] (9800) (which *we* requested) in order to disambiguate those requests.

Heh, nice. Unfortuantely, there's no way to guarantee people will only use this port for Zope's WebDAV server. That said, the two problems (WebDAV requests result in a browserDefault lookup, and folder contents) are not really an issue in everyday use for GET request. They merely cause things to explode on PUT requests to a null resource. We *can* identify PUT requests, obviously. So any comments on my proposal to skip the browserDefault lookup and the acquisition of resources for PUT/PROPFIND/PROPPATCH requests? This is the IPublishTraverse adapter I've had to register for this stuff to work right now. As you can see, I have to "undo" certain things the default traversal does (nice I didn't want to copy all that code and only modify the one condition needed). http://svn.plone.org/svn/plone/plone.dexterity/trunk/plone/dexterity/browser... (ignore the DAV_FOLDER_DATA_ID stuff - that's application specific). Martin

Tres Seaver wrote:

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Martin Aspeli wrote:

...

Tres Seaver <tseaver <at> palladion.com> writes:

...

There is no way to tell the difference between a WebDAV GET and a "normal" browser GET, period: the specs explicitly, deliberately overload the GET verb.

Hence the IANA-assigned "WebDAV source port"[1] (9800) (which *we* requested) in order to disambiguate those requests. Heh, nice.

That said, though: we know which port Zope is listening to for WebDAV. Even if it's 80 or 81 or whatever, we should be able to detect a DAV request by correlating the port on which the request was received with the address of the <webdav> server in zope.conf. True, we probably also allow DAV over the "http" port, but if that's a bit broken, I don't see a huge problem telling people to use a dedicated port. Do you see any problems with this?

...

Unfortuantely, there's no way to guarantee people will only use this port for Zope's WebDAV server.

That said, the two problems (WebDAV requests result in a browserDefault lookup, and folder contents) are not really an issue in everyday use for GET request. They merely cause things to explode on PUT requests to a null resource. We *can* identify PUT requests, obviously.

Strictly, PUT is not WebDAV-specific; however, it might be reasonable to apply the policy you are requesting for any PUT.

True.

...

So any comments on my proposal to skip the browserDefault lookup and the acquisition of resources for PUT/PROPFIND/PROPPATCH requests?

+.5, I guess. I'd like to make sure that we aren't breaking some other use first.

I'll run the tests? :) Martin -- Author of `Professional Plone Development`, a book for developers who want to work with Plone. See http://martinaspeli.net/plone-book