[ZCM] [ZC] 1713/10 Resolve "ZCatalog getObject broken"

Tue Apr 26 06:38:05 EDT 2005

Issue #1713 Update (Resolve) "ZCatalog getObject broken"
 Status Resolved, Zope/bug critical
To followup, visit:
  http://www.zope.org/Collectors/Zope/1713

==============================================================
= Resolve - Entry #10 by efge on Apr 26, 2005 6:38 am

 Status: Accepted => Resolved

This can be resolved now that Tres has added a switch to raise exceptions, which is on by default in 2.8, and that in any case a LOG is made when something unexpected happens.

________________________________________
= Comment - Entry #9 by efge on Mar 29, 2005 6:11 pm

Chris, you are the only one I see disagreeing with this. And you said yourself that you were not using the Zope 2 feature of having subobjects with more visibility than their containers.

Anyway the best way to get feedback is on zope-dev. Please reopen the discussion there if you feel you haven't been heard.

For reference, for others, the last thread discussing this is:
http://mail.zope.org/pipermail/zope-dev/2005-March/024390.html

________________________________________
= Comment - Entry #8 by chrisw on Mar 29, 2005 5:46 pm

What's the best way to get feedback on this?

I don't want to have to roll back the commits, but is that the only way?
________________________________________
= Resubmit - Entry #7 by chrisw on Mar 26, 2005 6:06 am

 Status: Resolved => Accepted

I don't think this patch is suitable.

I strongly disagree with the docstring and the implementation.
Returning None if the object no logner exists or if the objetc is unauthorized is confusing and unnecessary.

Personally, I feel the correct implementation is:
return self.aq_parent.restrictedTraverse(self.getPath())

However, I understand I'm just about the only person who feels this way, however, ignoring that, I think the implementation is still wrong.

I think a CatalogedObjectMissing exception or some such should be raised if the brain doesn't correspond to a real object.

I think Unauthorized should be raised if whatever algorithm ends up being used decides that the object mapping ot the brain shouldn't be accessible.

The algorithm also needs to be quick, the patched version looks a lot more complex than it needs to be. I also don't think it closely enough matches url traversal to be sufficient.

In short, I reckon the changes need to be reverted and mroe discussion should take place on zope-dev.

path = self.getPath().split('/')
catalog = self.aq_parent

________________________________________
= Resolve - Entry #6 by efge on Mar 25, 2005 12:53 pm

 Status: Accepted => Resolved

Fixed on 2.7 branch and trunk.

http://svn.zope.org/Zope/trunk/lib/python/Products/ZCatalog/CatalogBrains.py?rev=29682&r1=24563&r2=29682

________________________________________
= Comment - Entry #5 by shh on Mar 10, 2005 10:36 am

I don't see the benefit of calling validate over validateValue. I mean if you are aiming for Zope 2.8 compatibility you *must* pass the name argument anyway. And it looks to me as if the value argument (the last one) was the most important...

Anyhow, I trust that whoever tackles this will provide sufficient unit tests with it ;-)
________________________________________
= Comment - Entry #4 by efge on Mar 10, 2005 8:47 am

Effects of this patch:
- gives access to more objects than it did during the 2.7.1-2.7.4 timeframe (the objects returned by the new behaviour are a superset of those from the previous behaviour),
- the objects returned are still legitimate candidates (are answers to the catalog query) and are still checked for correct access rights.

________________________________________
= Comment - Entry #3 by ajung on Mar 10, 2005 6:51 am

Before accepting this patch for Zope 2.7.5 I would to know what others think about this patch  concerning backward compatiblity and security. I have no idea what side effects this patch might have.

________________________________________
= Accept - Entry #2 by efge on Mar 10, 2005 6:41 am

 Status: Pending => Accepted

 Supporters added: efge

 -        return self.aq_parent.restrictedTraverse(self.getPath(), None)
 +        obj = self.aq_parent.unrestrictedTraverse(self.getPath(), None)
 +        if obj and securityManager.validate(obj, obj, None, None):
 +            return obj
 +        else:
 +            return None

adapted as needed to have a proper working validation.

________________________________________
= Request - Entry #1 by roche on Feb 25, 2005 3:01 pm

Last year in March the following checkin was made that changed
ZCatalog's getObject to use restrictedTraverse instead of
unrestrictedTraverse. See:

http://mail.zope.org/pipermail/zope-checkins/2004-March/026846.html

In my opininion this is wrong, and just cost me a good three hours to
figure out why big parts of one of our apps broke after an upgrade to
Zope 2.7.3. What's worse is that there is nowhere a trace of this change
in HISTORY.txt or CHANGES.txt.

Anybody with a site structure that has less restrictive access deeper
into the site would agree that getObject should do an
unrestrictedTraverse. restrictedTraverse returns None as soon as it
traverses an object a user does not have access to, regardless if the
user has access to the object referred to by the full path. To
illustrate imagine the following:

You have a site with a folder containg documents at '/documents'. Inside
that folder you have a whole bunch of documents where users have a local
role of owner to give them permission to access only their own
documents. You use a Catalog query to get the list of documents
belonging to a particular user and want to use getObject to retrieve the
objects found. But, it won't work because restrictedTraverse already
fails when traversing the 'documents' folder.

I would propose that getObject does an unrestrictedTraverse of the path
and then checks if the user has permission to access that the object.
==============================================================