ZCatalog getObject broken
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
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? <http://www.zope.org/Collectors/Zope> -- Dieter
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
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
--On Mittwoch, 2. März 2005 9:56 Uhr +0000 Chris Withers <chris@simplistix.co.uk> 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
--On Freitag, 25. Februar 2005 20:21 Uhr +0100 Dieter Maurer <dieter@handshake.de> 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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Andreas Jung wrote: | | | --On Freitag, 25. Februar 2005 20:21 Uhr +0100 Dieter Maurer | <dieter@handshake.de> 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? |> |> <http://www.zope.org/Collectors/Zope> |> | | Best to include a patch as well :-) And a new test which fails under the current code, but succeeds with the patch. ;) Tres. - -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCH50UGqWXf00rNCgRAradAJ9/v/nU3iZEALYK+7hI2NYZCZbi0ACggAxm l4LfqI3+RYCI8VRHV9cz0rU= =4SWg -----END PGP SIGNATURE-----
On Fri, 2005-02-25 at 21:06 +0100, Andreas Jung wrote:
--On Freitag, 25. Februar 2005 20:21 Uhr +0100 Dieter Maurer <dieter@handshake.de> 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.000000000 +0200 +++ CatalogBrains.py 2005-03-03 09:43:48.000000000 +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
Roché Compaan wrote:
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.000000000 +0200 +++ CatalogBrains.py 2005-03-03 09:43:48.000000000 +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
There is a method deep down in Zope somewhere called: self.authenticated_has_access(obj) I cannot find the definition on my local Windows install, so I assume it's defined in some c code somewhere. Unfortunately there is no docs on the web either. Though there must have been at some time, as I would otherwise never have found it. Hmm... that is odd. -- hilsen/regards Max M, Denmark http://www.mxm.dk/ IT's Mad Science
On Thu, 2005-03-03 at 09:27 +0100, Max M wrote:
Roché Compaan wrote:
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.000000000 +0200 +++ CatalogBrains.py 2005-03-03 09:43:48.000000000 +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
There is a method deep down in Zope somewhere called:
self.authenticated_has_access(obj)
I cannot find the definition on my local Windows install, so I assume it's defined in some c code somewhere.
Unfortunately there is no docs on the web either. Though there must have been at some time, as I would otherwise never have found it.
Hmm... that is odd.
In this context the user does not need to be authenticated - Anonymous might have view rights in the context of the object. -- Roché Compaan Upfront Systems http://www.upfrontsystems.co.za
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
On Thu, 2005-03-03 at 14:56 +0000, 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
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
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.
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
Guys, Dieter Maurer <dieter@handshake.de> 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 fg@nuxeo.com
--On Donnerstag, 10. März 2005 12:49 Uhr +0100 Florent Guillaume <fg@nuxeo.com> wrote:
Guys,
Dieter Maurer <dieter@handshake.de> 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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Florent Guillaume wrote: | Dieter Maurer <dieter@handshake.de> 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? |> |> <http://www.zope.org/Collectors/Zope> | | | 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. Please note that calling 'validate' without passing the correct values for 'container', 'accessed', and 'name' may lead to unexpected results (it tries to guess, but may not be clever enough, especially if there is any weird wrapping / unwrapping in play). This was essentially the issue which led to the "spurious Unauthorized error" problem in Zope 2.7.3 (this point is germane or issue #1534, as well as #1713). Tres. - -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCMHBnGqWXf00rNCgRAvVZAJ9vlruC2X6Q60g8kzYpbcy8Rk8E/ACdGktW 4rPMryTLyixAABvKf/tj184= =U7gY -----END PGP SIGNATURE-----
I implemented a "publisherTraverse" function like this FWIW: def publisherTraverse(context, path): # this is a hack to get around the fact that restrictedTraverse, # unlike publisher traversal, does checks at every step of the # path. We don't want to limit access in this way (e.g. nested # shares are possible) so we reimplement restrictedTraverse in a # way that that emulates publisher traversal semantics ob = context.unrestrictedTraverse(path) user = getSecurityManager().getUser() if not user.has_permission('View', ob): raise zExceptions_Unauthorized, "cant traverse to %s" % path return ob Maybe this is better than using validate? On Thu, 2005-03-10 at 11:06, Tres Seaver wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Florent Guillaume wrote: | Dieter Maurer <dieter@handshake.de> 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? |> |> <http://www.zope.org/Collectors/Zope> | | | 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.
Please note that calling 'validate' without passing the correct values for 'container', 'accessed', and 'name' may lead to unexpected results (it tries to guess, but may not be clever enough, especially if there is any weird wrapping / unwrapping in play). This was essentially the issue which led to the "spurious Unauthorized error" problem in Zope 2.7.3 (this point is germane or issue #1534, as well as #1713).
Tres. - -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
iD8DBQFCMHBnGqWXf00rNCgRAvVZAJ9vlruC2X6Q60g8kzYpbcy8Rk8E/ACdGktW 4rPMryTLyixAABvKf/tj184= =U7gY -----END 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 )
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Chris McDonough wrote: | I implemented a "publisherTraverse" function like this FWIW: | | def publisherTraverse(context, path): | # this is a hack to get around the fact that restrictedTraverse, | # unlike publisher traversal, does checks at every step of the | # path. We don't want to limit access in this way (e.g. nested | # shares are possible) so we reimplement restrictedTraverse in a | # way that that emulates publisher traversal semantics | ob = context.unrestrictedTraverse(path) | user = getSecurityManager().getUser() | if not user.has_permission('View', ob): | raise zExceptions_Unauthorized, "cant traverse to %s" % path | return ob I don't think that the 'has_permission' check is quite right: at least, that isn't what the publisher does. 'ZPublisher.BaseRequest.traverse' collects roles during the traversal, and then calls the user folder's 'validate', passing them at the end. Note as well that 'View' may not be the permission which is protecting the traversed-to object). The "correct" emulation would probably be to call the user object's 'authorize' method, passing the proper values for accessed, container, name, value, and roles. Figuring out the proper values is left as an exercise for the reader ;). Oracular'ly, Tres. - -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.4 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCMIAxGqWXf00rNCgRAicEAJ4xoSRVeFBDNGixfenZsXK58aN/zACfUbFF SNfOYkCwgZyV30fl1H3ttko= =rdT7 -----END PGP SIGNATURE-----
On Thu, 2005-03-10 at 12:13 -0500, Tres Seaver wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Chris McDonough wrote: | I implemented a "publisherTraverse" function like this FWIW: | | def publisherTraverse(context, path): | # this is a hack to get around the fact that restrictedTraverse, | # unlike publisher traversal, does checks at every step of the | # path. We don't want to limit access in this way (e.g. nested | # shares are possible) so we reimplement restrictedTraverse in a | # way that that emulates publisher traversal semantics | ob = context.unrestrictedTraverse(path) | user = getSecurityManager().getUser() | if not user.has_permission('View', ob): | raise zExceptions_Unauthorized, "cant traverse to %s" % path | return ob
I don't think that the 'has_permission' check is quite right: at least, that isn't what the publisher does. 'ZPublisher.BaseRequest.traverse' collects roles during the traversal, and then calls the user folder's 'validate', passing them at the end. Note as well that 'View' may not be the permission which is protecting the traversed-to object).
The "correct" emulation would probably be to call the user object's 'authorize' method, passing the proper values for accessed, container, name, value, and roles. Figuring out the proper values is left as an exercise for the reader ;).
Is it at all possible to figure out the proper values? Earlier in this thread I asked if it is clear what value should be validated? It seems to me that returning None when the user does not have access to the object, imposes the same illogical constraint that the current implementation does, in that it does not allow access to a method or attribute of that object that is guarded less. One cannot predict when calling getObject what method or attribute the user wants to access. If it is possible to have access to a single method of the object but nothing else of that object, then it doesn't make sense to do *any* security checks in getObject. The object's own security assertions should guard access to its attributes. -- Roché Compaan Upfront Systems http://www.upfrontsystems.co.za
Tres Seaver <tseaver@zope.com> wrote:
Chris McDonough wrote: | I implemented a "publisherTraverse" function like this FWIW: | | def publisherTraverse(context, path): | # this is a hack to get around the fact that restrictedTraverse, | # unlike publisher traversal, does checks at every step of the | # path. We don't want to limit access in this way (e.g. nested | # shares are possible) so we reimplement restrictedTraverse in a | # way that that emulates publisher traversal semantics | ob = context.unrestrictedTraverse(path) | user = getSecurityManager().getUser() | if not user.has_permission('View', ob): | raise zExceptions_Unauthorized, "cant traverse to %s" % path | return ob
I don't think that the 'has_permission' check is quite right: at least, that isn't what the publisher does. 'ZPublisher.BaseRequest.traverse' collects roles during the traversal, and then calls the user folder's 'validate', passing them at the end. Note as well that 'View' may not be the permission which is protecting the traversed-to object).
The "correct" emulation would probably be to call the user object's 'authorize' method, passing the proper values for accessed, container, name, value, and roles. Figuring out the proper values is left as an exercise for the reader ;).
Yeah that's what I'm digging through right now :-( The publisher's collecting of the correct __roles__ from the intermediate traversed objects is quite complex. Also validation (actually trying to get a user that has access) tries user folders above if it fails. A correct publisherTraverse (which is also a method I'd like to have) is *much* more complex than what's shown above. And BTW the traverse() method should be shot in the head. In the current getObject problem that concerns us, we want to do better that restrictedTraverse, but maybe trying to emulate the full publisher rules is overly complex.
From what I see traverse():
1. does validation checks by actually trying to authenticate a user, which is done according to its credentials and from the context of the published object, and retried with above user folders if no user is found on the first one, 2. also checks for potential __roles__ found: - on an intermediate objects gotten by __bobo_traverse__, - if the published object has a __call__.__roles__. OTOH restrictedTraverse on each step: 1. does its validation using the current security manager's validate(), 2. does not collect any roles, so validate() uses its default behavior which ends up basically doing aq_acquire('__roles__'). So in a first pass that should be enough to fix our common problem, I propose to just do basically what Dieter mentionned: - unrestrictedTraverse to the parent - restrictedTraverse for the final step (I'll be testing this of course.) Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
Florent Guillaume wrote:
In the current getObject problem that concerns us, we want to do better that restrictedTraverse,
Why? As far as any problems I had go, it was purely the "returning None when the user can see the object" that was the bug. Provided getObject raises unauthorised when a user isn't allowe to see something, that works for me. Now, I'm obiously missing something, so what is the problem that you and Roché are having? Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
On Fri, 2005-03-11 at 15:47 +0000, Chris Withers wrote:
Florent Guillaume wrote:
In the current getObject problem that concerns us, we want to do better that restrictedTraverse,
Why? As far as any problems I had go, it was purely the "returning None when the user can see the object" that was the bug. Provided getObject raises unauthorised when a user isn't allowe to see something, that works for me.
Now, I'm obiously missing something, so what is the problem that you and Roché are having?
I don't get why you're not getting it :-) A, B and C are folders nested in each other i.e. A/B/C. A user does not have access to A and B but he does have access to C. If getObject uses restrictedTraverse it returns None immediately when traversing A, even though the user is allowed to access C. If getObject was working properly it would have returned C. The rest of the discussion basically boils down to figure out if the user is allowed to access C or not. -- Roché Compaan Upfront Systems http://www.upfrontsystems.co.za
Roché Compaan wrote:
The rest of the discussion basically boils down to figure out if the user is allowed to access C or not.
Hasn't it been raised allready that there is no way of knowing that? A single method might be public, but the rest of the object is hidden. What to do then? Just ignore the public method and use the objects overall visibility? -- hilsen/regards Max M, Denmark http://www.mxm.dk/ IT's Mad Science
On Fri, 2005-03-11 at 19:10 +0100, Max M wrote:
Roché Compaan wrote:
The rest of the discussion basically boils down to figure out if the user is allowed to access C or not.
Hasn't it been raised allready that there is no way of knowing that?
A single method might be public, but the rest of the object is hidden.
This is what I am arguing but I haven't had anybody agree/disagree with me yet. It is also a lot simpler to fix: < return self.aq_parent.restrictedTraverse(self.getPath(), None) ---
return self.aq_parent.unrestrictedTraverse(self.getPath(), None)
-- Roché Compaan Upfront Systems http://www.upfrontsystems.co.za
Roché Compaan wrote:
This is what I am arguing but I haven't had anybody agree/disagree with me yet. It is also a lot simpler to fix:
< return self.aq_parent.restrictedTraverse(self.getPath(), None) ---
return self.aq_parent.unrestrictedTraverse(self.getPath(), None)
I don't really mind either, provided the ,None vanishes... Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
Max M wrote at 2005-3-11 19:10 +0100:
... A single method might be public, but the rest of the object is hidden.
What to do then? Just ignore the public method and use the objects overall visibility?
The object has a "ObjectPermission" that controls handling references (!) to the object (itself) in TTW code. TTW code should not get a reference to an object when it does not have the "ObjectPermission" on it. Therefore, "getObject" tries hard to return "None" instead of the object in these cases. -- Dieter
Roché Compaan wrote:
I don't get why you're not getting it :-)
A, B and C are folders nested in each other i.e. A/B/C. A user does not have access to A and B but he does have access to C. If getObject uses restrictedTraverse it returns None immediately when traversing A, even though the user is allowed to access C. If getObject was working properly it would have returned C.
Ah, okay, I thought that's what you meant, but I hoped it wasn't. The fact that you expect this to work is a bug in Zope's security machinery, IMHO, but sadly only IMHO it appears. I would have no problem with the above behaviour if getObject raised Unauthorized rather than returned None. Your patch still had it returning None, IIRC, why did it do that?
The rest of the discussion basically boils down to figure out if the user is allowed to access C or not.
Yep, personally I reckon EVRYTHING should behave like restrictedTraverse, but as I said, that appears to just be me... Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
Chris Withers <chris@simplistix.co.uk> wrote:
A, B and C are folders nested in each other i.e. A/B/C. A user does not have access to A and B but he does have access to C. If getObject uses restrictedTraverse it returns None immediately when traversing A, even though the user is allowed to access C. If getObject was working properly it would have returned C.
Ah, okay, I thought that's what you meant, but I hoped it wasn't. The fact that you expect this to work is a bug in Zope's security machinery, IMHO, but sadly only IMHO it appears.
Huh? That's fundamental to Zope's security model.
I would have no problem with the above behaviour if getObject raised Unauthorized rather than returned None.
Your patch still had it returning None, IIRC, why did it do that?
The rest of the discussion basically boils down to figure out if the user is allowed to access C or not.
Yep, personally I reckon EVRYTHING should behave like restrictedTraverse, but as I said, that appears to just be me...
Well, you must be the only one who never had a need for security restrictions elsewhere than at the root of the site. Florent -- Florent Guillaume, Nuxeo (Paris, France) CTO, Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
Florent Guillaume wrote:
Ah, okay, I thought that's what you meant, but I hoped it wasn't. The fact that you expect this to work is a bug in Zope's security machinery, IMHO, but sadly only IMHO it appears.
Huh? That's fundamental to Zope's security model.
As I said, I appear to be the only person who thinks differently. I don't believe it's fundamental to Zope's security policy myself, and I think Zope would be just fine and less suprising to those of us who value security if it all behaved like restrictedTraverse...
Well, you must be the only one who never had a need for security restrictions elsewhere than at the root of the site.
I understand the pros and cons, don't worry ;-) Chris -- Simplistix - Content Management, Zope & Python Consulting - http://www.simplistix.co.uk
Chris McDonough wrote at 2005-3-10 11:28 -0500:
I implemented a "publisherTraverse" function like this FWIW:
def publisherTraverse(context, path): # this is a hack to get around the fact that restrictedTraverse, # unlike publisher traversal, does checks at every step of the # path. We don't want to limit access in this way (e.g. nested # shares are possible) so we reimplement restrictedTraverse in a # way that that emulates publisher traversal semantics ob = context.unrestrictedTraverse(path) user = getSecurityManager().getUser() if not user.has_permission('View', ob): raise zExceptions_Unauthorized, "cant traverse to %s" % path return ob
Maybe this is better than using validate?
No, because it does not take executable permission context into account (e.g. proxy roles). -- Dieter
participants (8)
-
Andreas Jung -
Chris McDonough -
Chris Withers -
Dieter Maurer -
Florent Guillaume -
Max M -
Roché Compaan -
Tres Seaver