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] 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