Align behavior with objects raising in __getattr__ (#157)

* tests: test C and PYTHON implementations in testPermissionRole

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


Co-authored-by: Dieter Maurer <d-maurer@users.noreply.github.com>

CHANGES.rst

@@ -8,6 +8,8 @@ For changes before version 3.0, see ``HISTORY.rst``.
 
 - Respect ``PURE_PYTHON`` environment variable set to ``0`` when running tests.
 
+- 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)
 ----------------

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