Skip to content

Commit

Permalink
checkPermission: align behavior with objects raising in __getattr__
Browse files Browse the repository at this point in the history
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 `checkPermission` uses `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 raised.
This change of behavior of python causes checkPermission 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 checkPermission
raises.

This change make this scenario behave the same between python2, python3
and C implementation: Unauthorized errors raised in __getattr__ are
supported. The code is also micro-simplified by doing only one getattr
instead of hasattr and then getattr.
  • Loading branch information
perrinjerome committed Sep 24, 2024
1 parent 4779653 commit 27d88c4
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 3 deletions.
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ For changes before version 3.0, see ``HISTORY.rst``.
7.1 (unreleased)
----------------

- Make Python implementation behave same as C implementation when using
``checkPermission`` with objects raising ``Unauthorized`` in ``__getattr__``.


7.0 (2024-05-30)
----------------
Expand Down
6 changes: 5 additions & 1 deletion src/AccessControl/ImplPython.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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)
Expand Down
7 changes: 5 additions & 2 deletions src/AccessControl/cAccessControl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1847,13 +1847,16 @@ 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)
Expand Down
15 changes: 15 additions & 0 deletions src/AccessControl/tests/testZopeSecurityPolicy.py
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,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

Expand All @@ -173,6 +182,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
Expand Down Expand Up @@ -351,6 +361,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
Expand Down

0 comments on commit 27d88c4

Please sign in to comment.