ImplPython initialisation is also required for ImplC

[Kazuhiko]

ImplC's checkPermission() just calls ImplPython's checkPermission(), but ImplPython is not initialised based on the configuration. Thus 'skip-ownership-checking on' is ignored in checkPermission() even though it is effective in validate(), that is implemented in ImplC.

I found this issue on Zope 2.12.26 but this issue exist on the current Zope and AccessControl repositories as well.

[Arnaud Fontaine (arnaud-fontaine)]

I have written two patches fixing this issue (FTR, with both patches applied, all the tests from AccessControl are passing):

1) The main patch defines getattr() in C implementation of ZopeSecurityPolicy. Thus, when checkPermission() (ImplPython) is called (from ImplC.ZopeSecurityPolicy.checkPermission()) and access _ownerous and _authenticated instance attributes, getattr() returns their expected values from ImplC ownerous and authenticated static variables. Before, it was just returning instance attributes set in ImplPython ZopeSecurity() constructor, thus ignoring completely ImplC ones. This is actually very similar to what is done with SecurityManager thread_id and context for example.

Another different implementation could have been to use PyObject for ownerous and authenticated (like C implementation of SecurityManager does actually), but I don't think that's necessary and it makes things more complicated.

I didn't define setattr() in C implementation of ZopeSecurityPolicy because I don't think that's relevant, but I initially defined it, so let me know if you want me to attach that patch as well.

2) The second smaller patch fixes a typo leading authenticated to never be set from setDefaultBehaviors() parameter value.

In order to debug this issue, I created one Python Script whose owner is 'System Processes' and with 'Manager' as Proxy Roles. This script calls checkPermission() on an object where only Manager has 'Access contents information' permission. After setting 'skip-ownership-checking' to 'on' in zope.conf and trying to access the script as Anonymous user:

- Before: checkPermission() returns 1 with ImplPython and 0 with ImplC.
- After: checkPermission() returns 1 with both ImplPython and ImplC.

[Vincent Pelletier (vincent-nexedi)]

Thanks Arnaud for working on this and posting patches.
I spotted whitespace changes which were probably not intended, especially on the second line:
- NULL, /* tp_getattro */
- NULL, /* tp_setattro */
+ (getattrofunc)ZopeSecurityPolicy_getattro,/* tp_getattro*/
+ NULL, /* tp_setattro*/

[Arnaud Fontaine (arnaud-fontaine)]

Fixed whitespace change. Thanks Vincent.

[Tres Seaver (tseaver)]

I think the attached patch adds a unit test which exercises this bug. The key to reproducing is to use an instance of the module's SecurityManager, created after calling 'setDefaultBehaviors', to check the permissions.

[Arnaud Fontaine (arnaud-fontaine)]

Thank you very much for the unit test.

When my patch is not applied, I get the following error:
$ python setup.py test -s AccessControl.tests.testZopeSecurityPolicy.C_SMTests
test__ownerous_and__authenticated_after_updating_defaults (AccessControl.tests.testZopeSecurityPolicy.C_SMTests) ... FAIL
test__ownerous_and__authenticated_defaults (AccessControl.tests.testZopeSecurityPolicy.C_SMTests) ... ok
======================================================================
FAIL: test__ownerous_and__authenticated_after_updating_defaults (AccessControl.tests.testZopeSecurityPolicy.C_SMTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/src/erp5/src/AccessControl/src/AccessControl/tests/testZopeSecurityPolicy.py", line 476, in test__ownerous_and__authenticated_after_updating_defaults
    self.assertTrue(mgr.checkPermission('testing', object()))
AssertionError: 0 is not true
----------------------------------------------------------------------
Ran 2 tests in 0.001s

Whereas AccessControl.tests.testZopeSecurityPolicy.Python_SMTests test suite passes. With my patch is applied, both test suites pass.

However, when running 'test_suite' (through 'python setup.py test -m AccessControl.tests.testZopeSecurityPolicy'), first Python Implementation test suite is executed. Then, when C Implementation test suite is executed, it fails because _ImplPython._defaultPolicy still points to Python Implementation ZopeSecurityPolicy (as set by AccessControl.SecurityManager.setSecurityPolicy()), thus when SecurityManager class is instanciated, it wrongly uses ImplPython._defaultPolicy . I have attached a patch on top of your test to fix this issue.

BTW, should I merge your unit test (and the fix I did if you think that's ok) into my commit and add a Signed-off-by with your name?

[Tres Seaver (tseaver)]

Please merge both my new test and your fix, and push your branch. We don't use the '--signoff' option.

[Tres Seaver (tseaver)]

Fix committed to 2.13 branch for 2.13.13 release, and to master for 3.0.8 release.