Re: [Zope-dev] ZCatalog getObject broken

[Andreas Jung]

--On Donnerstag, 10. März 2005 12:49 Uhr +0100 Florent Guillaume 
<[EMAIL PROTECTED]> wrote:

Guys,
Dieter Maurer  <[EMAIL PROTECTED]> wrote:
Roché Compaan wrote at 2005-2-25 17:22 +0200:
> 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,
I agree with you!
Me also.
> ...
> I would propose that getObject does an unrestrictedTraverse of the path
> and then checks if the user has permission to access that the object.
I argued precisely this approach with the person who made the
change. I had the impression that I have convinced him -- but
apparently, he did not change the code accordingly :-(
Maybe, a bug report to the collector will help?
   
Roché has added http://www.zope.org/Collectors/Zope/1713
I intend to fix this before 2.7.5 final, probably today or tonight.
I feel this is sufficiently important to warrant a fix now.
I guess it'll mean an RC2.
Please see my remark on this issue in the collector.
Andreas


pgphJkbk8eW1O.pgp
Description: PGP signature
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Florent Guillaume]

Guys,

Dieter Maurer  <[EMAIL PROTECTED]> wrote:
> Roché Compaan wrote at 2005-2-25 17:22 +0200:
> >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,
> 
> I agree with you!

Me also.

> > ...
> >I would propose that getObject does an unrestrictedTraverse of the path
> >and then checks if the user has permission to access that the object.
> 
> I argued precisely this approach with the person who made the
> change. I had the impression that I have convinced him -- but
> apparently, he did not change the code accordingly :-(
> 
> Maybe, a bug report to the collector will help?
> 
>

Roché has added http://www.zope.org/Collectors/Zope/1713

I intend to fix this before 2.7.5 final, probably today or tonight.
I feel this is sufficiently important to warrant a fix now.
I guess it'll mean an RC2.

Please shout if you find problems with this approach.

Florent

-- 
Florent Guillaume, Nuxeo (Paris, France)   CTO, Director of R&D
+33 1 40 33 71 59   http://nuxeo.com   [EMAIL PROTECTED]
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Dieter Maurer]

Roché Compaan wrote at 2005-3-3 22:36 +0200:
>On Thu, 2005-03-03 at 19:36 +0100, Dieter Maurer wrote:
>> Roché Compaan wrote at 2005-3-3 09:53 +0200:
>> > ...
>> >-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):
>> 
>> I think this is not correct: "validate" needs at least a
>> "value" parameter (this is the forth parameter).
>
>I thought this much but what value? And doesn't this make the
>implementation of restrictedTraverse suspect too?
>
>When code is calling getObject on a catalog brain we don't know what
>attribute or method of that object the calling code will access. Does it
>then make any sense at all to do security checks in getObject? IMO it
>doesn't.

Value means the accessed value. In your case, this is "obj".


-- 
Dieter
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Roché Compaan]

On Thu, 2005-03-03 at 19:36 +0100, Dieter Maurer wrote:
> Roché Compaan wrote at 2005-3-3 09:53 +0200:
> > ...
> >-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):
> 
> I think this is not correct: "validate" needs at least a
> "value" parameter (this is the forth parameter).

I thought this much but what value? And doesn't this make the
implementation of restrictedTraverse suspect too?

When code is calling getObject on a catalog brain we don't know what
attribute or method of that object the calling code will access. Does it
then make any sense at all to do security checks in getObject? IMO it
doesn't.

___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Dieter Maurer]

Roché Compaan wrote at 2005-3-3 09:53 +0200:
> ...
>-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):

I think this is not correct: "validate" needs at least a
"value" parameter (this is the forth parameter).

There is a "validateValue" method (instead of "validate")
that does what you want. You find it in "AccessControl/ImplPython.py".

Drawback: it might disappear in Zope 2.8.

-- 
Dieter
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Roché Compaan]

On Thu, 2005-03-03 at 14:56 +, Chris Withers wrote:
> Roché Compaan wrote:
> > +obj = self.aq_parent.unrestrictedTraverse(self.getPath(), None)
> > +if obj and securityManager.validate(obj, obj, None, None):
> > +return obj
> > +else:
> > +return None
> 
> Urm, Roche, doesn't the above seek to do exactly what...
> 
> return self.aq_parent.restrictedTraverse(self.getPath(), None)
> 
> ...does?

No it doesn't, restrictedTraverse fails along the way. If the path
is /a/b and the user doesn't have access to /a/ restrictedTraverse will
return None even though the user has access to /a/b/. In my code above
we only do a security check on the object that the full path resolves
to.

> 
> The problem is that an error should be raised, Unauthorized in my 
> opinion, rather than None being returned.

I would be ok with raising Unauthorized but it is not backwards
compatible. I suppose changing to 'unrestrictedTraverse' is also not
backward compatible but the current 'getObject' seems to suggest that we
do not want to raise an exception when the user does not have permission
to access the object. Is there some use case for 'getObject' that we are
missing here?

> None should never be returned in place of a brain, although I'll soften 
> that to say that if it does, it means something weird has happened (used 
> to mean the object the catalog entry mapped to had gone away)

I agree.

-- 
Roché Compaan
Upfront Systems http://www.upfrontsystems.co.za

___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Chris Withers]

Roché Compaan wrote:
+obj = self.aq_parent.unrestrictedTraverse(self.getPath(), None)
+if obj and securityManager.validate(obj, obj, None, None):
+return obj
+else:
+return None
Urm, Roche, doesn't the above seek to do exactly what...
return self.aq_parent.restrictedTraverse(self.getPath(), None)
...does?
The problem is that an error should be raised, Unauthorized in my 
opinion, rather than None being returned.

None should never be returned in place of a brain, although I'll soften 
that to say that if it does, it means something weird has happened (used 
to mean the object the catalog entry mapped to had gone away)

I think:
self.aq_parent.restrictedTraverse(self.getPath())
...should be fine, no?
Chris
--
Simplistix - Content Management, Zope & Python Consulting
   - http://www.simplistix.co.uk
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
http://mail.zope.org/mailman/listinfo/zope-announce
http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Roché Compaan]

On Fri, 2005-02-25 at 21:06 +0100, Andreas Jung wrote:
> 
> --On Freitag, 25. Februar 2005 20:21 Uhr +0100 Dieter Maurer 
> <[EMAIL PROTECTED]> wrote:
> 
> > Roché Compaan wrote at 2005-2-25 17:22 +0200:
> >> 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,
> >
> > I agree with you!
> >
> >> ...
> >> I would propose that getObject does an unrestrictedTraverse of the path
> >> and then checks if the user has permission to access that the object.
> >
> > I argued precisely this approach with the person who made the
> > change. I had the impression that I have convinced him -- but
> > apparently, he did not change the code accordingly :-(
> >
> > Maybe, a bug report to the collector will help?
> >
> >
> >
> 
> Best to include a patch as well :-)
> 
> -aj

I'm unsure about the security check in the patch below - I copied the
way restrictedTraverse does it. I read through validate in the default
security policy but it is one of those methods where all the security
implications doesn't fit in your head all at once.

--- CatalogBrains.py~   2004-03-23 22:27:23.0 +0200
+++ CatalogBrains.py2005-03-03 09:43:48.0 +0200
@@ -47,7 +47,11 @@
 (i.e., it was deleted or moved without recataloging), or if the
user is
 not authorized to access an object along the path.
 """
-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
 
 def getRID(self):
 """Return the record ID for this object."""

-- 
Roché Compaan
Upfront Systems http://www.upfrontsystems.co.za

___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Andreas Jung]

--On Mittwoch, 2. März 2005 9:56 Uhr + Chris Withers 
<[EMAIL PROTECTED]> wrote:

Roché Compaan wrote:
Maybe, a bug report to the collector will help?
  
Well, I posted just such an issue a few months back. I'm working offline
so can't give you the exact number but have a search and you should find
it.
I seem to remember Andreas rejecting it without thinking, as is his way
;-)
I would appreciate it you would help resolve outstanding issues instead of
making such statement :-)
Andreas



pgpGFKGurRtBC.pgp
Description: PGP signature
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Chris Withers]

Roché Compaan wrote:
Maybe, a bug report to the collector will help?
  
Well, I posted just such an issue a few months back. I'm working offline 
so can't give you the exact number but have a search and you should find it.

I seem to remember Andreas rejecting it without thinking, as is his way ;-)
Chris
--
Simplistix - Content Management, Zope & Python Consulting
   - http://www.simplistix.co.uk
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
http://mail.zope.org/mailman/listinfo/zope-announce
http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Andreas Jung]

--On Freitag, 25. Februar 2005 20:21 Uhr +0100 Dieter Maurer 
<[EMAIL PROTECTED]> wrote:

Roché Compaan wrote at 2005-2-25 17:22 +0200:
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,
I agree with you!
...
I would propose that getObject does an unrestrictedTraverse of the path
and then checks if the user has permission to access that the object.
I argued precisely this approach with the person who made the
change. I had the impression that I have convinced him -- but
apparently, he did not change the code accordingly :-(
Maybe, a bug report to the collector will help?
   
Best to include a patch as well :-)
-aj

___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists -
http://mail.zope.org/mailman/listinfo/zope-announce
http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Roché Compaan]

On Fri, 2005-02-25 at 20:21 +0100, Dieter Maurer wrote:
> Roché Compaan wrote at 2005-2-25 17:22 +0200:
> >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,
> 
> I agree with you!

I'm surprised that a release with such a dramatic change didn't break
tons of sites running out there. Or maybe people upgrade reluctantly.

>
> > ...
> >I would propose that getObject does an unrestrictedTraverse of the path
> >and then checks if the user has permission to access that the object.
> 
> I argued precisely this approach with the person who made the
> change. I had the impression that I have convinced him -- but
> apparently, he did not change the code accordingly :-(
> 
> Maybe, a bug report to the collector will help?
> 
>

I was reluctant to post an issue on the collector since getObject has
been see-sawing on restricted- and unrestrictedTraverse for a very long
time and I thought I'd post here first as a sanity check. Before Zope
2.3 it was restricted then it changed to unrestricted and now we're back
to restricted again. But at the risk of somebody completely ignoring it
or changing it back to restricted in Zope 2.8 I'll be off to the
collector to log another issue ;-) 

-- 
Roché Compaan
Upfront Systems   http://www.upfrontsystems.co.za

___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

Re: [Zope-dev] ZCatalog getObject broken

[Dieter Maurer]

Roché Compaan wrote at 2005-2-25 17:22 +0200:
>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,

I agree with you!

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

I argued precisely this approach with the person who made the
change. I had the impression that I have convinced him -- but
apparently, he did not change the code accordingly :-(

Maybe, a bug report to the collector will help?

   

-- 
Dieter
___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )

[Zope-dev] ZCatalog getObject broken

[Roché Compaan]

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.

-- 
Roché Compaan
Upfront Systems http://www.upfrontsystems.co.za

___
Zope-Dev maillist  -  Zope-Dev@zope.org
http://mail.zope.org/mailman/listinfo/zope-dev
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope-announce
 http://mail.zope.org/mailman/listinfo/zope )