From 0d6239912ae4526acdbdf0afc4bf6abd2ce16096 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Sun, 29 Sep 2024 14:15:06 +0000 Subject: [PATCH 1/3] tests: test C and PYTHON implementations in testPermissionRole --- src/AccessControl/tests/testPermissionRole.py | 106 ++++++++++-------- 1 file changed, 59 insertions(+), 47 deletions(-) diff --git a/src/AccessControl/tests/testPermissionRole.py b/src/AccessControl/tests/testPermissionRole.py index 8052b8e..86bd0a1 100644 --- a/src/AccessControl/tests/testPermissionRole.py +++ b/src/AccessControl/tests/testPermissionRole.py @@ -19,7 +19,7 @@ from Acquisition import Implicit from Acquisition import aq_base -from AccessControl.PermissionRole import PermissionRole +from ..Implementation import PURE_PYTHON ViewPermission = 'View' @@ -50,40 +50,39 @@ class PermissiveObject(Explicit): _Edit_Things__Permission = ['Anonymous'] -def assertPRoles(ob, permission, expect): - """ - Asserts that in the context of ob, the given permission maps to - the given roles. - """ - pr = PermissionRole(permission) - roles = pr.__of__(ob) - roles2 = aq_base(pr).__of__(ob) - assert roles == roles2 or tuple(roles) == tuple(roles2), ( - 'Different methods of checking roles computed unequal results') - same = 0 - if roles: - # When verbose security is enabled, permission names are - # embedded in the computed roles. Remove the permission - # names. - roles = [r for r in roles if not r.endswith('_Permission')] - - if roles is None or expect is None: - if (roles is None or tuple(roles) == ('Anonymous', )) and \ - (expect is None or tuple(expect) == ('Anonymous', )): - same = 1 - else: - got = {} - for r in roles: - got[r] = 1 - expected = {} - for r in expect: - expected[r] = 1 - if got == expected: # Dict compare does the Right Thing. - same = 1 - assert same, f'Expected roles: {expect!r}, got: {roles!r}' - - -class PermissionRoleTests (unittest.TestCase): +class PermissionRoleTestBase: + + def assertPRoles(self, ob, permission, expect): + """ + Asserts that in the context of ob, the given permission maps to + the given roles. + """ + pr = self._getTargetClass()(permission) + roles = pr.__of__(ob) + roles2 = aq_base(pr).__of__(ob) + assert roles == roles2 or tuple(roles) == tuple(roles2), ( + 'Different methods of checking roles computed unequal results') + same = 0 + if roles: + # When verbose security is enabled, permission names are + # embedded in the computed roles. Remove the permission + # names. + roles = [r for r in roles if not r.endswith('_Permission')] + + if roles is None or expect is None: + if (roles is None or tuple(roles) == ('Anonymous', )) and \ + (expect is None or tuple(expect) == ('Anonymous', )): + same = 1 + else: + got = {} + for r in roles: + got[r] = 1 + expected = {} + for r in expect: + expected[r] = 1 + if got == expected: # Dict compare does the Right Thing. + same = 1 + self.assertTrue(same, f'Expected roles: {expect!r}, got: {roles!r}') def testRestrictive(self, explicit=0): app = AppRoot() @@ -93,9 +92,9 @@ def testRestrictive(self, explicit=0): app.c = ImplicitContainer() app.c.o = RestrictiveObject() o = app.c.o - assertPRoles(o, ViewPermission, ('Manager', )) - assertPRoles(o, EditThingsPermission, ('Manager', 'Owner')) - assertPRoles(o, DeletePermission, ()) + self.assertPRoles(o, ViewPermission, ('Manager', )) + self.assertPRoles(o, EditThingsPermission, ('Manager', 'Owner')) + self.assertPRoles(o, DeletePermission, ()) def testPermissive(self, explicit=0): app = AppRoot() @@ -105,11 +104,11 @@ def testPermissive(self, explicit=0): app.c = ImplicitContainer() app.c.o = PermissiveObject() o = app.c.o - assertPRoles(o, ViewPermission, ('Anonymous', )) - assertPRoles(o, EditThingsPermission, ('Anonymous', - 'Manager', - 'Owner')) - assertPRoles(o, DeletePermission, ('Manager', )) + self.assertPRoles(o, ViewPermission, ('Anonymous', )) + self.assertPRoles(o, EditThingsPermission, ('Anonymous', + 'Manager', + 'Owner')) + self.assertPRoles(o, DeletePermission, ('Manager', )) def testExplicit(self): self.testRestrictive(1) @@ -117,13 +116,26 @@ def testExplicit(self): def testAppDefaults(self): o = AppRoot() - assertPRoles(o, ViewPermission, ('Anonymous', )) - assertPRoles(o, EditThingsPermission, ('Manager', 'Owner')) - assertPRoles(o, DeletePermission, ('Manager', )) + self.assertPRoles(o, ViewPermission, ('Anonymous', )) + self.assertPRoles(o, EditThingsPermission, ('Manager', 'Owner')) + self.assertPRoles(o, DeletePermission, ('Manager', )) def testPermissionRoleSupportsGetattr(self): - a = PermissionRole('a') + a = self._getTargetClass()('a') self.assertEqual(getattr(a, '__roles__'), ('Manager', )) self.assertEqual(getattr(a, '_d'), ('Manager', )) self.assertEqual(getattr(a, '__name__'), 'a') self.assertEqual(getattr(a, '_p'), '_a_Permission') + + +class Python_PermissionRoleTests(PermissionRoleTestBase, unittest.TestCase): + def _getTargetClass(self): + from AccessControl.ImplPython import PermissionRole + return PermissionRole + + +@unittest.skipIf(PURE_PYTHON, reason="Test expects C impl.") +class C__PermissionRoleTests(PermissionRoleTestBase, unittest.TestCase): + def _getTargetClass(self): + from AccessControl.ImplC import PermissionRole + return PermissionRole From 52994b3dfdaf0b964f27e94ca3dd7672f1e22f54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Mon, 30 Sep 2024 00:54:03 +0000 Subject: [PATCH 2/3] Align behavior with objects raising in `__getattr__` The observed problem was a behavior different between C and python implementation on python 3, happening with Zope python script. When the context can not be accessed by the current user, Zope binds a `Shared.DC.Scripts.Bindings.UnauthorizedBinding`, a class that raises an Unauthorized error when the context is actually accessed, in order to postpone the Unauthorized if something is actually accessed. This class does implements this by raising Unauthorized in `__getattr__`. The python implementation of `rolesForPermissionOn` used `hasattr` and `hasattr` has changed between python2 and python3, on python2 it was ignoring all exceptions, including potential Unauthorized errors and just returning False, but on python3 these errors are now raised. This change of behavior of python causes `rolesForPermissionOn` to behave differently: when using python implementation on python2 or when using C implementation, such Unauthorized errors were gracefully handled and caused `checkPermission` to return False, but on python3 the Unauthorized is raised. The C implementation of `rolesForPermissionOn` uses a construct equivalent to the python2 version of `hasattr`. For consistency - and because ignoring errors is usually not good - we also want to change it to be have like the python3 implementation. This change make this scenario behave the same between python and C implementations: - `Unauthorized` errors raised in `__getattr__` are supported on py3. - Other errors than `AttributeError` and `Unauthorized` raised in `__getattr__` are no longer ignored in the C implementation. --- CHANGES.rst | 3 ++ src/AccessControl/ImplPython.py | 6 +++- src/AccessControl/cAccessControl.c | 26 +++++++++++++++-- src/AccessControl/tests/testPermissionRole.py | 29 +++++++++++++++++++ .../tests/testZopeSecurityPolicy.py | 15 ++++++++++ 5 files changed, 76 insertions(+), 3 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index edacea0..a5916f2 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -8,6 +8,9 @@ For changes before version 3.0, see ``HISTORY.rst``. - Respect ``PURE_PYTHON`` environment variable set to ``0`` when running tests. +- Make Python implementation behave same as C implementation regarding + objects raising exceptions in ``__getattr__``. + 7.0 (2024-05-30) ---------------- diff --git a/src/AccessControl/ImplPython.py b/src/AccessControl/ImplPython.py index 1a7788b..0a9326b 100644 --- a/src/AccessControl/ImplPython.py +++ b/src/AccessControl/ImplPython.py @@ -31,6 +31,7 @@ from Acquisition import aq_parent from ExtensionClass import Base from zope.interface import implementer +from zExceptions import Unauthorized as zExceptions_Unauthorized PURE_PYTHON = int(os.environ.get('PURE_PYTHON', '0')) if PURE_PYTHON: @@ -71,8 +72,11 @@ def rolesForPermissionOn(perm, object, default=_default_roles, n=None): r = None while True: - if hasattr(object, n): + try: roles = getattr(object, n) + except (AttributeError, zExceptions_Unauthorized): + pass + else: if roles is None: if _embed_permission_in_roles: return (('Anonymous',), n) diff --git a/src/AccessControl/cAccessControl.c b/src/AccessControl/cAccessControl.c index 403ed67..bb849fe 100644 --- a/src/AccessControl/cAccessControl.c +++ b/src/AccessControl/cAccessControl.c @@ -652,6 +652,7 @@ static PyExtensionClass imPermissionRoleType = { static PyObject *Containers = NULL; static PyObject *ContainerAssertions = NULL; static PyObject *Unauthorized = NULL; +static PyObject *zExceptions_Unauthorized = NULL; static PyObject *warn= NULL; static PyObject *NoSequenceFormat = NULL; static PyObject *_what_not_even_god_should_do = NULL; @@ -1847,15 +1848,27 @@ c_rolesForPermissionOn(PyObject *perm, PyObject *object, Py_INCREF(r); /* - while 1: + while True: */ while (1) { /* - if hasattr(object, n): + try: roles = getattr(object, n) + except (AttributeError, zExceptions_Unauthorized): + pass + else: */ PyObject *roles = PyObject_GetAttr(object, n); + if (roles == NULL) + { + if (! (PyErr_ExceptionMatches(PyExc_AttributeError) + || PyErr_ExceptionMatches(zExceptions_Unauthorized))) + { + /* re-raise */ + return NULL; + } + } if (roles != NULL) { @@ -2313,6 +2326,7 @@ static struct PyMethodDef dtml_methods[] = { */ #define IMPORT(module, name) if ((module = PyImport_ImportModule(name)) == NULL) return NULL; #define GETATTR(module, name) if ((name = PyObject_GetAttrString(module, #name)) == NULL) return NULL; +#define GETATTR_AS(module, name, as_name) if ((as_name = PyObject_GetAttrString(module, name)) == NULL) return NULL; static struct PyModuleDef moduledef = { @@ -2400,6 +2414,14 @@ module_init(void) { Py_DECREF(tmp); tmp = NULL; + /*| from zExceptions import Unauthorized as zExceptions_Unauthorized + */ + + IMPORT(tmp, "zExceptions"); + GETATTR_AS(tmp, "Unauthorized", zExceptions_Unauthorized); + Py_DECREF(tmp); + tmp = NULL; + /*| from AccessControl.SecurityManagement import getSecurityManager */ diff --git a/src/AccessControl/tests/testPermissionRole.py b/src/AccessControl/tests/testPermissionRole.py index 86bd0a1..566b18b 100644 --- a/src/AccessControl/tests/testPermissionRole.py +++ b/src/AccessControl/tests/testPermissionRole.py @@ -127,6 +127,35 @@ def testPermissionRoleSupportsGetattr(self): self.assertEqual(getattr(a, '__name__'), 'a') self.assertEqual(getattr(a, '_p'), '_a_Permission') + def testErrorsDuringGetattr(self): + pr = self._getTargetClass()('View') + + class AttributeErrorObject(Implicit): + pass + self.assertEqual( + tuple(pr.__of__(AttributeErrorObject())), ('Manager', )) + + # Unauthorized errors are tolerated and equivalent to no + # permission declaration + class UnauthorizedErrorObject(Implicit): + def __getattr__(self, name): + from zExceptions import Unauthorized + if name == '_View_Permission': + raise Unauthorized(name) + raise AttributeError(name) + self.assertEqual( + tuple(pr.__of__(UnauthorizedErrorObject())), ('Manager', )) + + # other exceptions propagate + class ErrorObject(Implicit): + def __getattr__(self, name): + if name == '_View_Permission': + raise RuntimeError("Error raised during getattr") + raise AttributeError(name) + with self.assertRaisesRegex( + RuntimeError, "Error raised during getattr"): + tuple(pr.__of__(ErrorObject())) + class Python_PermissionRoleTests(PermissionRoleTestBase, unittest.TestCase): def _getTargetClass(self): diff --git a/src/AccessControl/tests/testZopeSecurityPolicy.py b/src/AccessControl/tests/testZopeSecurityPolicy.py index 068c49b..b3cd900 100644 --- a/src/AccessControl/tests/testZopeSecurityPolicy.py +++ b/src/AccessControl/tests/testZopeSecurityPolicy.py @@ -158,6 +158,15 @@ class PartlyProtectedSimpleItem3 (PartlyProtectedSimpleItem1): __roles__ = sysadmin_roles +class DynamicallyUnauthorized(SimpleItemish): + # This class raises an Unauthorized on attribute access, + # similar to Zope's Shared.DC.Scripts.Bindings.UnauthorizedBinding + __ac_local_roles__ = {} + + def __getattr__(self, name): + raise Unauthorized('Not authorized to access: %s' % name) + + class SimpleClass: attr = 1 @@ -174,6 +183,7 @@ def setUp(self): a.item1 = PartlyProtectedSimpleItem1() a.item2 = PartlyProtectedSimpleItem2() a.item3 = PartlyProtectedSimpleItem3() + a.d_item = DynamicallyUnauthorized() uf = UserFolder() a.acl_users = uf self.uf = a.acl_users @@ -352,6 +362,11 @@ def test_checkPermission_proxy_role_scope(self): r_subitem, context)) + def test_checkPermission_dynamically_unauthorized(self): + d_item = self.a.d_item + context = self.context + self.assertFalse(self.policy.checkPermission('View', d_item, context)) + def testUnicodeRolesForPermission(self): r_item = self.a.r_item context = self.context From 510e507a605bec627e7c71550fff9ac26f13365b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Wed, 9 Oct 2024 20:15:02 +0900 Subject: [PATCH 3/3] Update CHANGES.rst Co-authored-by: Dieter Maurer --- CHANGES.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index a5916f2..2bd593e 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -8,8 +8,7 @@ For changes before version 3.0, see ``HISTORY.rst``. - Respect ``PURE_PYTHON`` environment variable set to ``0`` when running tests. -- Make Python implementation behave same as C implementation regarding - objects raising exceptions in ``__getattr__``. +- Let the roles access in ``rolesForPermissionOn`` interpret ``AttributeError`` and ``Unauthorized`` as "no roles definition for this permission at this object" and report any other exception (for the Python and C implementation). We have to treat ``Unauthorized`` like ``AttributeError`` to support ``Shared.DC.Scripts.Bindings.UnauthorizedBinding`` which raises ``Unauthorized`` for any access. 7.0 (2024-05-30)