Align behavior with objects raising in __getattr__

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

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)

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
 	*/
 

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,25 +104,67 @@ 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)
         self.testPermissive(1)
 
     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')
+
+    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):
+        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

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