Skip to content

Commit

Permalink
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 `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.
  • Loading branch information
perrinjerome committed Oct 9, 2024
1 parent 0d62399 commit 52994b3
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 3 deletions.
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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)
----------------
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
26 changes: 24 additions & 2 deletions src/AccessControl/cAccessControl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
{

Expand Down Expand Up @@ -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 =
{
Expand Down Expand Up @@ -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
*/

Expand Down
29 changes: 29 additions & 0 deletions src/AccessControl/tests/testPermissionRole.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
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 @@ -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

Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 52994b3

Please sign in to comment.