[Zope-dev] ZopeSecurityPolicy global manipulation

[Martin Aspeli]

Hi,

AccessControl.ZopeSecurityPolicy contains this code:

from types import MethodType

# AccessControl.Implementation inserts:
#   ZopeSecurityPolicy, getRoles, rolesForPermissionOn
from AccessControl.SimpleObjectPolicies import _noroles

rolesForPermissionOn = None  # XXX:  avoid import loop

tuple_or_list = tuple, list


def getRoles(container, name, value, default):

    global rolesForPermissionOn  # XXX:  avoid import loop

    if rolesForPermissionOn is None:
        from PermissionRole import rolesForPermissionOn

    roles = getattr(value, '__roles__', _noroles)
    if roles is _noroles:
        if not name or not isinstance(name, basestring):
            return default

        if type(value) is MethodType:
            container = value.im_self

        cls = getattr(container, '__class__', None)
        if cls is None:
            return default

        roles = getattr(cls, name+'__roles__', _noroles)
        if roles is _noroles:
            return default

        value = container

    if roles is None or isinstance(roles, tuple_or_list):
        return roles

    rolesForPermissionOn = getattr(roles, 'rolesForPermissionOn', None)
    if rolesForPermissionOn is not None:
        roles = rolesForPermissionOn(value)

    return roles

Look carefully at how ``rolesForPermissionOn`` is used both at the
top, to lazily set a global to avoid an import loop, and the bottom,
as an attribute of the ``roles`` object.

I'm pretty sure this is wrong™ on many levels, but most importantly,
it seems the global is being overwritten each time execution gets down
to that last block. I know this module gets munged by Implementation,
but I'm pretty sure ImplPython doesn't define getRoles() at least, and
I'm not even sure the C implementation does either.

To prove it to myself, I made a frivolous equivalent that used
'datetime.date' as the importable. It's a bit ugly, but you get the
idea:

>>> date = None
>>> class C(object):
...     def __init__(self, d):
...             self.date = d
...
>>> c1 = C(lambda: 'x')
>>> c2 = C(lambda: 'y')
>>> def get(c):
...     global date
...     if date is None:
...             from datetime import date
...     date = getattr(c, 'date', None)
...     if date is not None:
...             print date()
...
>>> date is None
True
>>> get(c1)
x
>>> date
<function <lambda> at 0x10dac8140>
>>> get(c2)
y
>>> date
<function <lambda> at 0x10dac8cf8>
>>>

Surely, this is all evil volatile? Maybe the global bit just needs to
go away? It doesn't seem to be used in that function, and I'm pretty
sure the implementation ends up overwriting the global anyway.

Cheers,
Martin