Traversal issue which affects Five
Hi all, It seems that the way OFS.Traversable.restrictedTraverse() handles security checking on objects with __bobo_traverse__ methods is considerably different from the way it normally checks security. The result is that traversal cannot obtain attributes using acquisition from objects that are marked <five:traversable>. In the normal case, security is checked using guarded_getattr, which gets an attribute and checks the permissions on the retrieved object in its original context. For __bobo_traverse__methods which return simple properties (say strings), it is impossible to determine the container from which the returned attribute originates, so unless the attribute was not acquired, an Unauthorized error will always be raised. Objects that are Five Traversable always have __bobo_traverse__ methods which attempt to mimic normal traversal, which effectively means that the security checks end up preventing acquisition of simple properties using traversal from ever working on these objects (say using a TAL path expression 'context/attribute' which you expect to be acquired). Unfortunately, because Five has no control over the security checks done during traversal, this cannot be fixed directly in Five. However, IMHO fixing this makes sense for Zope itself, provided there aren't any undesirable consequences. I propose that if the validation of a __bobo_traverse result raises Unauthorized, that we make one last check to see if the result o 'guarded_getattr(obj, name)' is identical to the result of the __bobo_traverse__ call and allow it if that's the case. Here is my proposed patch against Zope 2.9 trunk: --- Traversable.py (revision 66323) +++ Traversable.py (working copy) @@ -201,9 +201,18 @@ else: # Can't determine container container = _none - if not securityManager.validate( - obj, container, name, next): - raise Unauthorized, name + try: + if not securityManager.validate( + obj, container, name, next): + raise Unauthorized, name + except Unauthorized: + # If next is a simple unwrapped property, it's + # parentage is indeterminate, but it may have been + # acquired safely. In this case validate will raise + # an error, and we can check that our value was + # acquired safely. + if guarded_getattr(obj, name, marker) is not next: + raise Unauthorized, name else: if restricted: next = guarded_getattr(obj, name, marker) At the moment Plone 2.5 is really struggling with this issue, and it would be wonderful if a fix for this could go into Zope 2.8 and 2.9 soon. I'm happy to write tests for this, I just want to make sure that I'm not proposing something really wrong/inappropriate here. Generally, the validate() call should return a True or False value, so this change should have little performance impact except in the case where 'container == _none' and validate would otherwise raise a very unhelpful unauthorized error. Your feedback is much appreciated. Alec Mitchell
Hi Alec, Five traversal is definitely very touchy, and the interactions with Zope numerous. So I'm sure the problem you observed is real and that a solution must be found. The unit tests you propose would go a long way to having a fix committed asap, so yes please, provide one. Florent Alec Mitchell wrote:
It seems that the way OFS.Traversable.restrictedTraverse() handles security checking on objects with __bobo_traverse__ methods is considerably different from the way it normally checks security. The result is that traversal cannot obtain attributes using acquisition from objects that are marked <five:traversable>. In the normal case, security is checked using guarded_getattr, which gets an attribute and checks the permissions on the retrieved object in its original context. For __bobo_traverse__methods which return simple properties (say strings), it is impossible to determine the container from which the returned attribute originates, so unless the attribute was not acquired, an Unauthorized error will always be raised.
Objects that are Five Traversable always have __bobo_traverse__ methods which attempt to mimic normal traversal, which effectively means that the security checks end up preventing acquisition of simple properties using traversal from ever working on these objects (say using a TAL path expression 'context/attribute' which you expect to be acquired). Unfortunately, because Five has no control over the security checks done during traversal, this cannot be fixed directly in Five. However, IMHO fixing this makes sense for Zope itself, provided there aren't any undesirable consequences. I propose that if the validation of a __bobo_traverse result raises Unauthorized, that we make one last check to see if the result o 'guarded_getattr(obj, name)' is identical to the result of the __bobo_traverse__ call and allow it if that's the case. Here is my proposed patch against Zope 2.9 trunk:
--- Traversable.py (revision 66323) +++ Traversable.py (working copy) @@ -201,9 +201,18 @@ else: # Can't determine container container = _none - if not securityManager.validate( - obj, container, name, next): - raise Unauthorized, name + try: + if not securityManager.validate( + obj, container, name, next): + raise Unauthorized, name + except Unauthorized: + # If next is a simple unwrapped property, it's + # parentage is indeterminate, but it may have been + # acquired safely. In this case validate will raise + # an error, and we can check that our value was + # acquired safely. + if guarded_getattr(obj, name, marker) is not next: + raise Unauthorized, name else: if restricted: next = guarded_getattr(obj, name, marker)
At the moment Plone 2.5 is really struggling with this issue, and it would be wonderful if a fix for this could go into Zope 2.8 and 2.9 soon. I'm happy to write tests for this, I just want to make sure that I'm not proposing something really wrong/inappropriate here. Generally, the validate() call should return a True or False value, so this change should have little performance impact except in the case where 'container == _none' and validate would otherwise raise a very unhelpful unauthorized error. Your feedback is much appreciated.
Alec Mitchell _______________________________________________ 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 )
-- Florent Guillaume, Nuxeo (Paris, France) Director of R&D +33 1 40 33 71 59 http://nuxeo.com fg@nuxeo.com
On 4/17/06, Florent Guillaume <fg@nuxeo.com> wrote:
Hi Alec,
Five traversal is definitely very touchy, and the interactions with Zope numerous. So I'm sure the problem you observed is real and that a solution must be found. The unit tests you propose would go a long way to having a fix committed asap, so yes please, provide one. ... Alec Mitchell wrote:
It seems that the way OFS.Traversable.restrictedTraverse() handles security checking on objects with __bobo_traverse__ methods is considerably different from the way it normally checks security. The result is that traversal cannot obtain attributes using acquisition from objects that are marked <five:traversable>. In the normal case, security is checked using guarded_getattr, which gets an attribute and checks the permissions on the retrieved object in its original context. For __bobo_traverse__methods which return simple properties (say strings), it is impossible to determine the container from which the returned attribute originates, so unless the attribute was not acquired, an Unauthorized error will always be raised.
Objects that are Five Traversable always have __bobo_traverse__ methods which attempt to mimic normal traversal, which effectively means that the security checks end up preventing acquisition of simple properties using traversal from ever working on these objects (say using a TAL path expression 'context/attribute' which you expect to be acquired). Unfortunately, because Five has no control over the security checks done during traversal, this cannot be fixed directly in Five. However, IMHO fixing this makes sense for Zope itself, provided there aren't any undesirable consequences. I propose that if the validation of a __bobo_traverse result raises Unauthorized, that we make one last check to see if the result o 'guarded_getattr(obj, name)' is identical to the result of the __bobo_traverse__ call and allow it if that's the case. Here is my proposed patch against Zope 2.9 trunk:
<snip old patch>
At the moment Plone 2.5 is really struggling with this issue, and it would be wonderful if a fix for this could go into Zope 2.8 and 2.9 soon. I'm happy to write tests for this, I just want to make sure that I'm not proposing something really wrong/inappropriate here. Generally, the validate() call should return a True or False value, so this change should have little performance impact except in the case where 'container == _none' and validate would otherwise raise a very unhelpful unauthorized error. Your feedback is much appreciated.
Ok, I've attached a refined patch with tests. Only one of these will fail with the original version of Traversable.py (testBoboTraverseToAcquiredAttribute), the other three just make sure that expected security restrictions and behavior are preserved. I'll reiterate that fixing this issue is quite essential for Plone, and likely for any other reasonably complex Zope 2 project which wishes to use Five/Zope3 extensively. I'll file a collector issue later today. Alec
Alec Mitchell wrote at 2006-4-16 12:28 -0700:
... It seems that the way OFS.Traversable.restrictedTraverse() handles security checking on objects with __bobo_traverse__ methods is considerably different from the way it normally checks security. The result is that traversal cannot obtain attributes using acquisition from objects that are marked <five:traversable>. In the normal case, security is checked using guarded_getattr, which gets an attribute and checks the permissions on the retrieved object in its original context. For __bobo_traverse__methods which return simple properties (say strings), it is impossible to determine the container from which the returned attribute originates, so unless the attribute was not acquired, an Unauthorized error will always be raised. ... However, IMHO fixing this makes sense for Zope itself, provided there aren't any undesirable consequences. I propose that if the validation of a __bobo_traverse result raises Unauthorized, that we make one last check to see if the result o 'guarded_getattr(obj, name)' is identical to the result of the __bobo_traverse__ call and allow it if that's the case. Here is my proposed patch against Zope 2.9 trunk:
In our local Zope version, I implemented a solution that is (in my opinion) superior: Define an exception "UseTraversalDefault" that can be used by "__bobo_traverse__" to tell the traversal process (either URL traversal in the publisher or restricted/unrestricted/CMF traversal) to use the default lookup. One big advantage is that e.g. "Archetypes.BaseObject.BaseObject.__bobo_traverse__" no longer need this horrible dance to decide whether it should or must not create a "NullResource". -- Dieter
On 4/17/06, Dieter Maurer <dieter@handshake.de> wrote:
Alec Mitchell wrote at 2006-4-16 12:28 -0700:
... It seems that the way OFS.Traversable.restrictedTraverse() handles security checking on objects with __bobo_traverse__ methods is considerably different from the way it normally checks security. The result is that traversal cannot obtain attributes using acquisition from objects that are marked <five:traversable>. In the normal case, security is checked using guarded_getattr, which gets an attribute and checks the permissions on the retrieved object in its original context. For __bobo_traverse__methods which return simple properties (say strings), it is impossible to determine the container from which the returned attribute originates, so unless the attribute was not acquired, an Unauthorized error will always be raised. ... However, IMHO fixing this makes sense for Zope itself, provided there aren't any undesirable consequences. I propose that if the validation of a __bobo_traverse result raises Unauthorized, that we make one last check to see if the result o 'guarded_getattr(obj, name)' is identical to the result of the __bobo_traverse__ call and allow it if that's the case. Here is my proposed patch against Zope 2.9 trunk:
In our local Zope version, I implemented a solution that is (in my opinion) superior:
Define an exception "UseTraversalDefault" that can be used by "__bobo_traverse__" to tell the traversal process (either URL traversal in the publisher or restricted/unrestricted/CMF traversal) to use the default lookup.
One big advantage is that e.g. "Archetypes.BaseObject.BaseObject.__bobo_traverse__" no longer need this horrible dance to decide whether it should or must not create a "NullResource".
Yes, it does sound like a better solution. However, the issue I see with it is that it is essentially adding new functionality, rather than fixing a problem with the existing behavior. That would seem to make it a much less likely candidate for getting into zope 2.8.7 (which is important IMHO), despite the fact that it is a bit more elegant. In fact, I would say that even with this clever exception handling, there may still be cases where one wants to return an acquired attribute directly from a __bobo_traverse__, without simply falling back on the default behavior (say if one wants to limit the acquirable names or some other silly thing). So the two solutions can coexist peacefully; though the exception based solution is likely to allow a lot of nice simplifications to some of the hacky __bobo_traverses__ out there. Alec
Alec Mitchell wrote at 2006-4-17 14:53 -0700:
... Yes, it does sound like a better solution. However, the issue I see with it is that it is essentially adding new functionality, rather than fixing a problem with the existing behavior. That would seem to make it a much less likely candidate for getting into zope 2.8.7 (which is important IMHO), despite the fact that it is a bit more elegant
It is a new functionality -- but one that does nothing until it is used and only for those applications (like "five:traversal") that use it. Thus, even a strict release manager might accept it. -- Dieter
--On 18. April 2006 18:52:10 +0200 Dieter Maurer <dieter@handshake.de> wrote:
Alec Mitchell wrote at 2006-4-17 14:53 -0700:
... Yes, it does sound like a better solution. However, the issue I see with it is that it is essentially adding new functionality, rather than fixing a problem with the existing behavior. That would seem to make it a much less likely candidate for getting into zope 2.8.7 (which is important IMHO), despite the fact that it is a bit more elegant
It is a new functionality -- but one that does nothing until it is used and only for those applications (like "five:traversal") that use it.
Thus, even a strict release manager might accept it.
Am I strict? :-) Andreas
Andreas Jung wrote at 2006-4-18 20:54 +0200:
... --On 18. April 2006 18:52:10 +0200 Dieter Maurer <dieter@handshake.de> wrote:
Alec Mitchell wrote at 2006-4-17 14:53 -0700:
... Yes, it does sound like a better solution. However, the issue I see with it is that it is essentially adding new functionality, rather than fixing a problem with the existing behavior. That would seem to make it a much less likely candidate for getting into zope 2.8.7 (which is important IMHO), despite the fact that it is a bit more elegant
It is a new functionality -- but one that does nothing until it is used and only for those applications (like "five:traversal") that use it.
Thus, even a strict release manager might accept it.
Am I strict? :-)
Do you accept it? ;-) -- Dieter
--On 19. April 2006 18:38:05 +0200 Dieter Maurer <dieter@handshake.de> wrote:
Thus, even a strict release manager might accept it.
Am I strict? :-)
Do you accept it? ;-)
As release manager I don't have to dig into every problem. Patches + tests are of course accepted if there is some consensus that a proposed solution solves the open problem. But I don't have to care about the progress of every ongoing issue. Andreas -- ZOPYX Ltd. & Co. KG - Charlottenstr. 37/1 - 72070 Tübingen - Germany Web: www.zopyx.com - Email: info@zopyx.com - Phone +49 - 7071 - 793376 E-Publishing, Python, Zope & Plone development, Consulting
Andreas Jung wrote at 2006-4-19 20:13 +0200:
--On 19. April 2006 18:38:05 +0200 Dieter Maurer <dieter@handshake.de> wrote: ...
Do you accept it? ;-)
As release manager I don't have to dig into every problem. Patches + tests are of course accepted if there is some consensus that a proposed solution solves the open problem. But I don't have to care about the progress of every ongoing issue.
In this case, you may want to say something: I proposed the introduction of a new feature ("__bobo_traverse__" can tell the containing framework that it should use the default by raising a special exception). Someone feared that it cannot be added to a micro release as it is a new feature (and not only a bug fix). I argued that this new feature could be acceptable nevertheless, as it affects only components that know about it and use it. Otherwise, it is completely inoffensive. Up to you to decide whether we may break the general rule ("no new features in micro releases") for features of this kind. -- Dieter
--On 20. April 2006 20:29:30 +0200 Dieter Maurer <dieter@handshake.de> wrote:
Up to you to decide whether we may break the general rule ("no new features in micro releases") for features of this kind.
We've always included minor features that aren't obviously critical micro release. The general rule basically applies applies to really new features. If this patch is acceptable and useful for everyone affected (Alec?). Since I am not directly (or no more) affected by this issue others should vote for or against this change. So we're more pragmatic than pedantic. Andreas -- ZOPYX Ltd. & Co. KG - Charlottenstr. 37/1 - 72070 Tübingen - Germany Web: www.zopyx.com - Email: info@zopyx.com - Phone +49 - 7071 - 793376 E-Publishing, Python, Zope & Plone development, Consulting
Andreas Jung wrote:
--On 20. April 2006 20:29:30 +0200 Dieter Maurer <dieter@handshake.de> wrote:
Up to you to decide whether we may break the general rule ("no new features in micro releases") for features of this kind.
We've always included minor features that aren't obviously critical micro release. The general rule basically applies applies to really new features. If this patch is acceptable and useful for everyone affected (Alec?).
I don't have time to comment on the specific issue. In general, I think that new features in bug-fix releases are a bad idea. Of course, the line between bug-fix and feature is not always crisp. Something that is really a new feature should wait for a feature release. Note that the next feature release should happen in a couple of months. Jim -- Jim Fulton mailto:jim@zope.com Python Powered! CTO (540) 361-1714 http://www.python.org Zope Corporation http://www.zope.com http://www.zope.org
--On 21. April 2006 07:09:45 -0400 Jim Fulton <jim@zope.com> wrote:
In general, I think that new features in bug-fix releases are a bad idea. Of course, the line between bug-fix and feature is not always crisp. Something that is really a new feature should wait for a feature release.
Sure, in this particular case the issue is bug for Alec and Philipp, others consider it a feature :-) A behavior introduced because by some implementation bug is likely to be considered as a bug (this justifies the change in Five 1.3.3).... Andreas
On 4/17/06, Dieter Maurer <dieter@handshake.de> wrote: ...
In our local Zope version, I implemented a solution that is (in my opinion) superior:
Define an exception "UseTraversalDefault" that can be used by "__bobo_traverse__" to tell the traversal process (either URL traversal in the publisher or restricted/unrestricted/CMF traversal) to use the default lookup.
One big advantage is that e.g. "Archetypes.BaseObject.BaseObject.__bobo_traverse__" no longer need this horrible dance to decide whether it should or must not create a "NullResource".
OK, I'm bringing an old thread back to life to propose implementing this change for Zope 2.10. It would be quite nice to get rid of this "horrible dance" in the next version of Archetypes, and the change would generally allow people to make simpler __bobo_traverse__ methods that don't have to reimplement the traversal machinery themselves, especially since the traversal machinery is a bit in flux these days. Of course, if the plan is to make __bobo_traverse__ obsolete in the near future (provided reasonable alternatives exist), then making it nicer shouldn't really be too much of a priority. Thoughts? Alec
On 8/20/06, Alec Mitchell <apm13@columbia.edu> wrote:
On 4/17/06, Dieter Maurer <dieter@handshake.de> wrote: ...
In our local Zope version, I implemented a solution that is (in my opinion) superior:
Define an exception "UseTraversalDefault" that can be used by "__bobo_traverse__" to tell the traversal process (either URL traversal in the publisher or restricted/unrestricted/CMF traversal) to use the default lookup.
One big advantage is that e.g. "Archetypes.BaseObject.BaseObject.__bobo_traverse__" no longer need this horrible dance to decide whether it should or must not create a "NullResource".
OK, I'm bringing an old thread back to life to propose implementing this change for Zope 2.10. It would be quite nice to get rid of this "horrible dance" in the next version of Archetypes, and the change would generally allow people to make simpler __bobo_traverse__ methods that don't have to reimplement the traversal machinery themselves, especially since the traversal machinery is a bit in flux these days. Of course, if the plan is to make __bobo_traverse__ obsolete in the near future (provided reasonable alternatives exist), then making it nicer shouldn't really be too much of a priority. Thoughts?
Well, as I mentioned in April, i was going to in Zope 2.10 refactor this and get rid of five:traversable, so I asked for specific usecases so I could test it. No use cases popped up and the refactoring is now finished. What effect that has on your usecase I don't know. However, five objects no longer have a __bobo_traverse__ by default, which should make things simpler. -- Lennart Regebro, Nuxeo http://www.nuxeo.com/ CPS Content Management http://www.cps-project.org/
On 8/21/06, Lennart Regebro <regebro@gmail.com> wrote:
On 8/20/06, Alec Mitchell <apm13@columbia.edu> wrote:
On 4/17/06, Dieter Maurer <dieter@handshake.de> wrote: ...
In our local Zope version, I implemented a solution that is (in my opinion) superior:
Define an exception "UseTraversalDefault" that can be used by "__bobo_traverse__" to tell the traversal process (either URL traversal in the publisher or restricted/unrestricted/CMF traversal) to use the default lookup.
One big advantage is that e.g. "Archetypes.BaseObject.BaseObject.__bobo_traverse__" no longer need this horrible dance to decide whether it should or must not create a "NullResource".
OK, I'm bringing an old thread back to life to propose implementing this change for Zope 2.10. It would be quite nice to get rid of this "horrible dance" in the next version of Archetypes, and the change would generally allow people to make simpler __bobo_traverse__ methods that don't have to reimplement the traversal machinery themselves, especially since the traversal machinery is a bit in flux these days. Of course, if the plan is to make __bobo_traverse__ obsolete in the near future (provided reasonable alternatives exist), then making it nicer shouldn't really be too much of a priority. Thoughts?
Well, as I mentioned in April, i was going to in Zope 2.10 refactor this and get rid of five:traversable, so I asked for specific usecases so I could test it. No use cases popped up and the refactoring is now finished.
What effect that has on your usecase I don't know. However, five objects no longer have a __bobo_traverse__ by default, which should make things simpler.
Indeed it does, the issue is that writing __bobo_traverse__ methods which try to fallback on the normal traversal mechanisms has always been a pain (you have to reimplement the normal traversal mechanisms yourself, including some funny WebDAVisms). If instead the __bobo_traverse__ mechanism could explicitly tell its caller (via an exception) to continue with the standard traversal, it would be a nice improvement. Of course, the real fix may be for products to stop using __bobo_traverse__ and start sing BeforeTraverseEvent subscribers, or IPublshTraverse adapters. This is something I hope to look into in the next couple days. Alec
On 8/21/06, Alec Mitchell <apm13@columbia.edu> wrote:
Indeed it does, the issue is that writing __bobo_traverse__ methods which try to fallback on the normal traversal mechanisms has always been a pain (you have to reimplement the normal traversal mechanisms yourself, including some funny WebDAVisms). If instead the __bobo_traverse__ mechanism could explicitly tell its caller (via an exception) to continue with the standard traversal, it would be a nice improvement. Of course, the real fix may be for products to stop using __bobo_traverse__ and start sing BeforeTraverseEvent subscribers, or IPublshTraverse adapters. This is something I hope to look into in the next couple days.
Yeah, that's probably a solution that doesn't require us to increse the complexity of the already overly complex traversal. :) -- Lennart Regebro, Nuxeo http://www.nuxeo.com/ CPS Content Management http://www.cps-project.org/
participants (6)
-
Alec Mitchell -
Andreas Jung -
Dieter Maurer -
Florent Guillaume -
Jim Fulton -
Lennart Regebro