Bad interaction between Zope 2.7.3 and CMF 1.4
While testing a large-ish customer project under Zope 2.7.3 we found that when an object with setDefaultAccess('deny') is used as the context for a PythonScript, the script can no longer aquire tools from the portal root. Because a test says more than a thousand words, I added one to CMFDefault. To reproduce: - get Zope-2_7-branch - get CMF-1_4-branch - run tests of CMFDefault, notably test_RestrictedAcquisition.py Rolling back this checkin restores functionality: http://mail.zope.org/pipermail/zope-checkins/2004-August/028152.html Note that I was unable to reproduce the issue with CMF 1.5 (or plain Zope, for that matter). What has changed? Beats me! Note that this issue has the potential to break each and every Plone site out there. Ultimately, I feel that unless there is a *very* good reason for removing the aq_acquire call from cAccessControl/ImplPython it should be restored. Stefan -- The time has come to start talking about whether the emperor is as well dressed as we are supposed to think he is. /Pete McBreen/
Stefan H. Holek wrote:
While testing a large-ish customer project under Zope 2.7.3 we found that when an object with setDefaultAccess('deny') is used as the context for a PythonScript, the script can no longer aquire tools from the portal root.
*By definition*, anybody who has declared 'setDefaultAccess('deny') *wants* the behavior you describe: that declaration says, "unless I give you explicit permission for using a name, refuse." If Plone has classes which make such assertions, then either the authors *meant* them, or they need to be removed. This is (literally) the same thing as declaring '__allow_access_to_unprotected_subobjects__ = 0' in your class. You could still acquire explicit objects from above, e.g.: - Make the class derive from Acquisition.Explicit (might not be necessary; I can't recall whether the 'Acquired' bit works also for Acquisition.Implicit instances). - For each attribute you want to acquire, add 'foo = Acquired()', to the class - Make security assertions about each attribute.
Because a test says more than a thousand words, I added one to CMFDefault.
Your test doesn't really belong in CMF, as you are arguing that the current implemtation in Zope is broken. Please *don't* check such a test in on the HEAD (or branch head) until after this discussion is resolved.
To reproduce: - get Zope-2_7-branch - get CMF-1_4-branch - run tests of CMFDefault, notably test_RestrictedAcquisition.py
Rolling back this checkin restores functionality: http://mail.zope.org/pipermail/zope-checkins/2004-August/028152.html
Note that I was unable to reproduce the issue with CMF 1.5 (or plain Zope, for that matter). What has changed? Beats me! Note that this issue has the potential to break each and every Plone site out there.
Ultimately, I feel that unless there is a *very* good reason for removing the aq_acquire call from cAccessControl/ImplPython it should be restored.
Thank you for making the case reproducible; Richard Jones had reported this issue earlier, but couldn't cut it down to a simple case. I will work on adding tests to AccessControl which make the intent clear (we can still argue about whether to keep the change). Tres. -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com
On 09.10.2004, at 18:04, Tres Seaver wrote:
*By definition*, anybody who has declared 'setDefaultAccess('deny') *wants* the behavior you describe: that declaration says, "unless I give you explicit permission for using a name, refuse."
If Plone has classes which make such assertions, then either the authors *meant* them, or they need to be removed. This is (literally) the same thing as declaring '__allow_access_to_unprotected_subobjects__ = 0' in your class.
Plone itself doesn't AFAICS. Third party applications may, like the one I was talking about. The unfortunate coincidence is that these apps work fine with Zope up to 2.7.2. I am of the impression that using aq_acquire in guarded_getattr does the right thing (by accident?). I certainly lack the Fu though.
Your test doesn't really belong in CMF, as you are arguing that the current implemtation in Zope is broken.
Please *don't* check such a test in on the HEAD (or branch head) until after this discussion is resolved.
Right, but I couldn't make it break anyplace else. Sorry. Feel free to remove it.
Thank you for making the case reproducible; Richard Jones had reported this issue earlier, but couldn't cut it down to a simple case. I will work on adding tests to AccessControl which make the intent clear (we can still argue about whether to keep the change).
Thank you! Stefan -- The time has come to start talking about whether the emperor is as well dressed as we are supposed to think he is. /Pete McBreen/
Stefan H. Holek wrote:
On 09.10.2004, at 18:04, Tres Seaver wrote:
*By definition*, anybody who has declared 'setDefaultAccess('deny') *wants* the behavior you describe: that declaration says, "unless I give you explicit permission for using a name, refuse."
If Plone has classes which make such assertions, then either the authors *meant* them, or they need to be removed. This is (literally) the same thing as declaring '__allow_access_to_unprotected_subobjects__ = 0' in your class.
Plone itself doesn't AFAICS. Third party applications may, like the one I was talking about. The unfortunate coincidence is that these apps work fine with Zope up to 2.7.2.
This would be a good time for those apps to convert to usubg 'getToolByName'. E.g., instead of: tool = context.portal_sometool they should be doing: from Products.CMFCore.utils import getToolByName tool = getToolByName(context, 'portal_sometool') 'getToolByName' already does the Right Thing here, because it uses 'aq_get' to find the tool. Tres. -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com
Tres Seaver wrote at 2004-10-9 12:04 -0400:
Stefan H. Holek wrote:
While testing a large-ish customer project under Zope 2.7.3 we found that when an object with setDefaultAccess('deny') is used as the context for a PythonScript, the script can no longer aquire tools from the portal root.
*By definition*, anybody who has declared 'setDefaultAccess('deny') *wants* the behavior you describe: that declaration says, "unless I give you explicit permission for using a name, refuse."
I do *NOT* think that this is the intended semantics of "setDefaultAccess('deny')". In my view, "setDefaultAccess(XXX)" should only affect objects that do not have security declarations themselves. Acquired tools have security declarations -- they should not be affected by "setDefaultAccess".
If Plone has classes which make such assertions, then either the authors *meant* them, or they need to be removed. This is (literally) the same thing as declaring '__allow_access_to_unprotected_subobjects__ = 0' in your class.
With this name, it becomes even clearer: Acquires tools are neither "unprotected" nor "subobjects". -- Dieter
Dieter Maurer wrote:
Tres Seaver wrote at 2004-10-9 12:04 -0400:
Stefan H. Holek wrote:
While testing a large-ish customer project under Zope 2.7.3 we found that when an object with setDefaultAccess('deny') is used as the context for a PythonScript, the script can no longer aquire tools from the portal root.
*By definition*, anybody who has declared 'setDefaultAccess('deny') *wants* the behavior you describe: that declaration says, "unless I give you explicit permission for using a name, refuse."
I do *NOT* think that this is the intended semantics of "setDefaultAccess('deny')".
In my view, "setDefaultAccess(XXX)" should only affect objects that do not have security declarations themselves. Acquired tools have security declarations -- they should not be affected by "setDefaultAccess".
If Plone has classes which make such assertions, then either the authors *meant* them, or they need to be removed. This is (literally) the same thing as declaring '__allow_access_to_unprotected_subobjects__ = 0' in your class.
With this name, it becomes even clearer:
Acquires tools are neither "unprotected" nor "subobjects".
Hmm, I tried writing a test te verify that my change (after 2.7.2) had modified the security policy's behavior. Here is the test, which I expected to pass on the head of the 2.7 branch, but fail on 2.7.2: --- lib/python/AccessControl/tests/testZopeSecurityPolicy.py 27 Jan 2004 19:22:36 -0000 1.6.2.6 +++ lib/python/AccessControl/tests/testZopeSecurityPolicy.py 12 Oct 2004 17:15:05 -0000 @@ -336,6 +336,16 @@ else: self.fail('Policy accepted bad __roles__') + def test_no_acquisition_through_paranoid(self): + + # Check that acquisition through a paranoid object is not allowed. + app = App() + app.wanted = UnprotectedSimpleItem() + app.enabling = UnprotectedSimpleItem() + app.blocking = RestrictedSimpleItem() + self.assertPolicyAllows(app.enabling, 'wanted') + self.assertPolicyDenies(app.blocking, 'wanted') + def test_suite(): return unittest.makeSuite(ZopeSecurityPolicyTests) However, it passes for both. Anyone care to speculate why? Tres. -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com
Tres Seaver wrote:
Dieter Maurer wrote:
Tres Seaver wrote at 2004-10-9 12:04 -0400:
Stefan H. Holek wrote:
While testing a large-ish customer project under Zope 2.7.3 we found that when an object with setDefaultAccess('deny') is used as the context for a PythonScript, the script can no longer aquire tools from the portal root.
*By definition*, anybody who has declared 'setDefaultAccess('deny') *wants* the behavior you describe: that declaration says, "unless I give you explicit permission for using a name, refuse."
I do *NOT* think that this is the intended semantics of "setDefaultAccess('deny')".
In my view, "setDefaultAccess(XXX)" should only affect objects that do not have security declarations themselves. Acquired tools have security declarations -- they should not be affected by "setDefaultAccess".
If Plone has classes which make such assertions, then either the authors *meant* them, or they need to be removed. This is (literally) the same thing as declaring '__allow_access_to_unprotected_subobjects__ = 0' in your class.
With this name, it becomes even clearer:
Acquires tools are neither "unprotected" nor "subobjects".
Hmm, I tried writing a test te verify that my change (after 2.7.2) had modified the security policy's behavior. Here is the test, which I expected to pass on the head of the 2.7 branch, but fail on 2.7.2:
--- lib/python/AccessControl/tests/testZopeSecurityPolicy.py 27 Jan 2004 19:22:36 -0000 1.6.2.6 +++ lib/python/AccessControl/tests/testZopeSecurityPolicy.py 12 Oct 2004 17:15:05 -0000 @@ -336,6 +336,16 @@ else: self.fail('Policy accepted bad __roles__')
+ def test_no_acquisition_through_paranoid(self): + + # Check that acquisition through a paranoid object is not allowed. + app = App() + app.wanted = UnprotectedSimpleItem() + app.enabling = UnprotectedSimpleItem() + app.blocking = RestrictedSimpleItem() + self.assertPolicyAllows(app.enabling, 'wanted') + self.assertPolicyDenies(app.blocking, 'wanted') +
def test_suite(): return unittest.makeSuite(ZopeSecurityPolicyTests)
However, it passes for both. Anyone care to speculate why?
Tres.
OK, so my change was specifically to 'guarded_getattr', so the test was exercising the wrong thing. Note that this means that the old behaviour of guarded_getattr was allowing objects to be returned which wouldn't be validated (I think). Here is a test for 'guarded_getattr', which *fails* both before and after my change: --- lib/python/AccessControl/tests/testZopeGuards.py 16 Jan 2004 21:04:54 -0000 1.1.4.3 +++ lib/python/AccessControl/tests/testZopeGuards.py 12 Oct 2004 17:51:13 -0000 @@ -24,11 +24,14 @@ import AccessControl.SecurityManagement from AccessControl.SimpleObjectPolicies import ContainerAssertions from zExceptions import Unauthorized +from Acquisition import Explicit from AccessControl.ZopeGuards \ import guarded_getattr, get_dict_get, get_dict_pop, get_list_pop, \ get_iter, guarded_min, guarded_max, safe_builtins, guarded_enumerate, \ guarded_sum, guarded_apply +from testZopeSecurityPolicy \ + import App, UnprotectedSimpleItem, RestrictedSimpleItem try: __file__ except NameError: @@ -112,6 +115,17 @@ self.assertEqual(values.args, (d, 'values')) finally: ContainerAssertions[_dict] = old + + def test_no_acquisition_through_paranoid(self): + + # Check that acquisition through a paranoid object is not allowed. + app = App() + app.wanted = UnprotectedSimpleItem() + app.enabling = UnprotectedSimpleItem() + app.blocking = RestrictedSimpleItem() + ok = self.guarded_getattr(app.enabling, 'wanted') + self.assertRaises(Unauthorized, + self.guarded_getattr, app.blocking, 'wanted') class TestGuardedGetattrC(TestGuardedGetattrPy): Aargh! Tres. -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com
Note that I found it to be relevant which object I want to acquire (don't ask me why, though). E.g. going back to my CMFDefault examples, I *can* acquire portal_workflow and portal_url, but I can *not* acquire portal_membership and acl_users from a denied context. Go figure. If I change the test below to "app.wanted = PartlyProtectedSimpleItem3()" the test fails on current 2_7-branch ... Stefan On 12.10.2004, at 19:14, Tres Seaver wrote:
+ def test_no_acquisition_through_paranoid(self): + + # Check that acquisition through a paranoid object is not allowed. + app = App() + app.wanted = UnprotectedSimpleItem() + app.enabling = UnprotectedSimpleItem() + app.blocking = RestrictedSimpleItem() + self.assertPolicyAllows(app.enabling, 'wanted') + self.assertPolicyDenies(app.blocking, 'wanted') +
-- The time has come to start talking about whether the emperor is as well dressed as we are supposed to think he is. /Pete McBreen/
Stefan H. Holek wrote:
Note that I found it to be relevant which object I want to acquire (don't ask me why, though).
Because the policy checks the roles on the acquired object? As Dieter points out, 'setDefaultAccess(deny)' should only apply to subobjects which do *not* have their own roles.
E.g. going back to my CMFDefault examples, I *can* acquire portal_workflow and portal_url, but I can *not* acquire portal_membership and acl_users from a denied context. Go figure.
If I change the test below to "app.wanted = PartlyProtectedSimpleItem3()" the test fails on current 2_7-branch ...
But that test fails on 2.7.2, as well. My change was actually to the implementation of 'guarded_getattr', which is not tested in 'testZopeSecurityPolicy' (the location of my original patch); rather, its tests are in 'testZopeGuards' (where my second patch applies). I have not yet bee able to write a good test there yet (one which either passes on the 2.7 head and fails for 2.7.2, or vice versa). Tres. -- =============================================================== Tres Seaver tseaver@zope.com Zope Corporation "Zope Dealers" http://www.zope.com
participants (3)
-
Dieter Maurer -
Stefan H. Holek -
Tres Seaver