Commit 240a8d26 authored by Vincent Pelletier's avatar Vincent Pelletier

{,Propertied}User: Reduce the overhead from Developer role processing.

getRoles is called a lot (on every restricted access, so hundreds of times
per transaction), it is definitely not the right place to do extra
computation, especially when their result does not change from one call
to the next (configuration should only change on process restart, so not
during a transaction - and even if it someday did, it should be fine to
wait for next transaction for it to take effect).
Instead, do the extra work when creating the user (typically once per
transaction).
Also, modernise python syntax (simplifications & style).
Also, reduce code duplication from ERP5Security.ERP5UserFactory.
parent e793b164
......@@ -19,7 +19,6 @@ from Products.ERP5Type.Globals import InitializeClass
from Acquisition import aq_inner, aq_parent
from AccessControl import ClassSecurityInfo
from Products.PageTemplates.PageTemplateFile import PageTemplateFile
from App.config import getConfiguration
from Products.PluggableAuthService.plugins.BasePlugin import BasePlugin
from Products.PluggableAuthService.utils import classImplements
from Products.PluggableAuthService.interfaces.plugins import IUserFactoryPlugin
......@@ -52,151 +51,6 @@ class ERP5User(PropertiedUser):
_user_path = None
_login_path = None
def getRolesInContext( self, object ):
""" Return the list of roles assigned to the user.
For ERP5, we check if a _getAcquireLocalRoles is defined on the object.
"""
user_id = self.getId()
# [ x.getId() for x in self.getGroups() ]
group_ids = self.getGroups()
principal_ids = list( group_ids )
principal_ids.insert( 0, user_id )
local = {}
object = aq_inner( object )
while 1:
local_roles = getattr( object, '__ac_local_roles__', None )
if local_roles:
if callable( local_roles ):
local_roles = local_roles()
dict = local_roles or {}
for principal_id in principal_ids:
for role in dict.get( principal_id, [] ):
local[ role ] = 1
# patch by Klaus for LocalRole blocking
if getattr(object, '_getAcquireLocalRoles', None) is not None:
if not object._getAcquireLocalRoles():
break
inner = aq_inner( object )
parent = aq_parent( inner )
if parent is not None:
object = parent
continue
new = getattr( object, 'im_self', None )
if new is not None:
object = aq_inner( new )
continue
break
# Patched: Developer role should not never be available as local role
local.pop('Developer', None)
return list( self.getRoles() ) + local.keys()
def allowed( self, object, object_roles=None ):
""" Check whether the user has access to object.
As for getRolesInContext, we take into account _getAcquireLocalRoles for
ERP5.
"""
if self.getUserName() == ERP5Security.SUPER_USER:
# super user is allowed to accesss any object
return 1
if object_roles is _what_not_even_god_should_do:
return 0
# Short-circuit the common case of anonymous access.
if object_roles is None or 'Anonymous' in object_roles:
return 1
# Check for Developer Role, see patches.User for rationale
# XXX-arnau: copy/paste
object_roles = set(object_roles)
if 'Developer' in object_roles:
object_roles.remove('Developer')
product_config = getattr(getConfiguration(), 'product_config', None)
if product_config:
config = product_config.get('erp5')
if config and self.getId() in config.developer_list:
return 1
# Provide short-cut access if object is protected by 'Authenticated'
# role and user is not nobody
if 'Authenticated' in object_roles and (
self.getUserName() != 'Anonymous User'):
return 1
# Check for ancient role data up front, convert if found.
# This should almost never happen, and should probably be
# deprecated at some point.
if 'Shared' in object_roles:
object_roles = self._shared_roles(object)
if object_roles is None or 'Anonymous' in object_roles:
return 1
# Check for a role match with the normal roles given to
# the user, then with local roles only if necessary. We
# want to avoid as much overhead as possible.
user_roles = self.getRoles()
for role in object_roles:
if role in user_roles:
if self._check_context(object):
return 1
return None
# Still have not found a match, so check local roles. We do
# this manually rather than call getRolesInContext so that
# we can incur only the overhead required to find a match.
inner_obj = aq_inner( object )
user_id = self.getId()
# [ x.getId() for x in self.getGroups() ]
group_ids = self.getGroups()
principal_ids = list( group_ids )
principal_ids.insert( 0, user_id )
while 1:
local_roles = getattr( inner_obj, '__ac_local_roles__', None )
if local_roles:
if callable( local_roles ):
local_roles = local_roles()
dict = local_roles or {}
for principal_id in principal_ids:
local_roles = dict.get( principal_id, [] )
for role in object_roles:
if role in local_roles:
if self._check_context( object ):
return 1
return 0
# patch by Klaus for LocalRole blocking
if getattr(inner_obj, '_getAcquireLocalRoles', None) is not None:
if not inner_obj._getAcquireLocalRoles():
break
inner = aq_inner( inner_obj )
parent = aq_parent( inner )
if parent is not None:
inner_obj = parent
continue
new = getattr( inner_obj, 'im_self', None )
if new is not None:
inner_obj = aq_inner( new )
continue
break
return None
def getUserValue(self):
""" -> user document
......
......@@ -17,7 +17,7 @@
# Locale roles acquisition patch for PAS
from Acquisition import aq_inner, aq_parent
from App.config import getConfiguration
try:
from Products.PluggableAuthService.PropertiedUser import PropertiedUser
from Products.PluggableAuthService.PropertiedUser import\
......@@ -25,7 +25,10 @@ try:
except ImportError:
PropertiedUser = None
def getRolesInContext( self, object ):
TRUE_LAMBDA = lambda: True
DEVELOPER_ROLE_ID = 'Developer'
def getRolesInContext(self, object):
""" Return the list of roles assigned to the user.
......@@ -37,83 +40,33 @@ def getRolesInContext( self, object ):
o Ripped off from AccessControl.User.BasicUser, which provides
no other extension mechanism. :(
"""
user_id = self.getId()
# [ x.getId() for x in self.getGroups() ]
group_ids = self.getGroups()
principal_ids = list( group_ids )
principal_ids.insert( 0, user_id )
local ={}
object = aq_inner( object )
principal_id_list = [self.getId()]
principal_id_list.extend(self.getGroups())
result = set()
object = aq_inner(object)
while 1:
local_roles = getattr( object, '__ac_local_roles__', None )
if local_roles:
if callable( local_roles ):
local_roles = local_roles()
dict = local_roles or {}
for principal_id in principal_ids:
for role in dict.get( principal_id, [] ):
local[ role ] = 1
# patch by Klaus for LocalRole blocking
_getAcquireLocalRoles = getattr(object, '_getAcquireLocalRoles', None)
if _getAcquireLocalRoles is not None:
if not _getAcquireLocalRoles():
break
inner = aq_inner( object )
parent = aq_parent( inner )
if parent is not None:
object = parent
continue
new = getattr( object, 'im_self', None )
if new is not None:
object = aq_inner( new )
continue
local_role_dict = getattr(object, '__ac_local_roles__', None)
if local_role_dict:
if callable(local_role_dict):
local_role_dict = local_role_dict() or {}
for principal_id in principal_id_list:
result.update(local_role_dict.get(principal_id, ()))
if getattr(object, '_getAcquireLocalRoles', TRUE_LAMBDA)():
parent = aq_parent(aq_inner(object))
if parent is not None:
object = parent
continue
new = getattr(object, '__self__', None)
if new is not None:
object = aq_inner(new)
continue
break
# Patched: Developer role should never be available as local role
result.discard(DEVELOPER_ROLE_ID)
result.update(self.getRoles())
return list(result)
# Patched: Developer role should not never be available as local role
local.pop('Developer', None)
return list( self.getRoles() ) + local.keys()
from App.config import getConfiguration
def getRoles( self ):
""" -> [ role ]
o Include only "global" roles.
"""
role_tuple = self._roles.keys()
if role_tuple:
product_config = getattr(getConfiguration(), 'product_config', None)
if product_config:
config = product_config.get('erp5')
if config:
role_set = set(role_tuple)
user_id = self.getId()
if config and user_id in config.developer_list:
role_set.add('Developer')
elif user_id in role_set:
role_set.remove('Developer')
return role_set
return role_tuple
def allowed(self, object, object_roles=None ):
def allowed(self, object, object_roles=None):
""" Check whether the user has access to object.
o The user must have one of the roles in object_roles to allow access.
......@@ -131,16 +84,7 @@ def allowed(self, object, object_roles=None ):
if object_roles is None or 'Anonymous' in object_roles:
return 1
# Check for Developer Role, see patches.User for rationale
# XXX-arnau: copy/paste
object_roles = set(object_roles)
if 'Developer' in object_roles:
object_roles.remove('Developer')
product_config = getattr(getConfiguration(), 'product_config', None)
if product_config:
config = product_config.get('erp5')
if config and self.getId() in config.developer_list:
return 1
# Provide short-cut access if object is protected by 'Authenticated'
# role and user is not nobody
......@@ -156,75 +100,59 @@ def allowed(self, object, object_roles=None ):
if object_roles is None or 'Anonymous' in object_roles:
return 1
# Check for a role match with the normal roles given to
# the user, then with local roles only if necessary. We
# want to avoid as much overhead as possible.
user_roles = self.getRoles()
for role in object_roles:
if role in user_roles:
if self._check_context(object):
return 1
return None
# Still have not found a match, so check local roles. We do
# this manually rather than call getRolesInContext so that
# we can incur only the overhead required to find a match.
inner_obj = aq_inner( object )
user_id = self.getId()
# [ x.getId() for x in self.getGroups() ]
group_ids = self.getGroups()
principal_ids = list( group_ids )
principal_ids.insert( 0, user_id )
# Check global roles.
if object_roles.intersection(self.getRoles()):
return self._check_context(object)
# Do not match Developer as a local role.
object_roles.discard(DEVELOPER_ROLE_ID)
check_context = self._check_context
# Check local roles.
inner_obj = aq_inner(object)
principal_id_list = [self.getId()]
principal_id_list.extend(self.getGroups())
while 1:
local_roles = getattr( inner_obj, '__ac_local_roles__', None )
if local_roles:
if callable( local_roles ):
local_roles = local_roles()
dict = local_roles or {}
for principal_id in principal_ids:
local_roles = dict.get( principal_id, [] )
for role in object_roles:
if role in local_roles:
if self._check_context( object ):
return 1
return 0
# patch by Klaus for LocalRole blocking
_getAcquireLocalRoles = getattr(object, '_getAcquireLocalRoles', None)
if _getAcquireLocalRoles is not None:
if not _getAcquireLocalRoles():
break
inner = aq_inner( inner_obj )
parent = aq_parent( inner )
local_role_dict = getattr(inner_obj, '__ac_local_roles__', None)
if local_role_dict:
if callable(local_role_dict):
local_role_dict = local_role_dict() or {}
for principal_id in principal_id_list:
for role in object_roles.intersection(
local_role_dict.get(principal_id, ()),
):
return int(bool(check_context(object)))
if getattr(inner_obj, '_getAcquireLocalRoles', TRUE_LAMBDA)():
parent = aq_parent(aq_inner(inner_obj))
if parent is not None:
inner_obj = parent
continue
new = getattr( inner_obj, 'im_self', None )
inner_obj = parent
continue
new = getattr(inner_obj, '__self__', None)
if new is not None:
inner_obj = aq_inner( new )
continue
break
inner_obj = aq_inner(new)
continue
break
return None
orig_PropertiedUser__init__ = getattr(PropertiedUser, '__init__', None)
def PropertiedUser__init__(self, id, login=None):
orig_PropertiedUser__init__(self, id, login)
if id in getattr(
getattr(
getConfiguration(),
'product_config',
{},
).get('erp5'),
'developer_list',
(),
):
self._roles[DEVELOPER_ROLE_ID] = 1
orig_PropertiedUser__addRoles = getattr(PropertiedUser, '_addRoles', None)
def PropertiedUser__addRoles(self, roles=()):
orig_PropertiedUser__addRoles(self, (x for x in roles if x != DEVELOPER_ROLE_ID))
if PropertiedUser is not None:
PropertiedUser.getRolesInContext = getRolesInContext
PropertiedUser.allowed = allowed
PropertiedUser.getRoles = getRoles
PropertiedUser.getRolesInContext = getRolesInContext
PropertiedUser.allowed = allowed
PropertiedUser.__init__ = PropertiedUser__init__
PropertiedUser._addRoles = PropertiedUser__addRoles
......@@ -13,7 +13,14 @@
#
##############################################################################
from AccessControl.User import BasicUser
from threading import local
from Acquisition import aq_inner, aq_parent
from AccessControl.PermissionRole import _what_not_even_god_should_do
from AccessControl.User import BasicUser, SimpleUser
from App.config import getConfiguration
from ..TransactionalVariable import TransactionalVariable
DEVELOPER_ROLE_ID = 'Developer'
BasicUser_allowed = BasicUser.allowed
def allowed(self, object, object_roles=None):
......@@ -22,81 +29,82 @@ def allowed(self, object, object_roles=None):
and remove it, as it should never be acquired anyhow, before calling the
original method
"""
# XXX-arnau: copy/paste (PropertiedUser)
if object_roles is not None:
object_roles = set(object_roles)
if 'Developer' in object_roles:
object_roles.remove('Developer')
product_config = getattr(getConfiguration(), 'product_config', None)
if product_config:
config = product_config.get('erp5')
if config and self.getId() in config.developer_list:
return 1
# Skip "self._check_context(object)"
if (
object_roles is not _what_not_even_god_should_do and
object_roles is not None and
DEVELOPER_ROLE_ID in set(object_roles or ()).intersection(self.getRoles())
):
return 1
return BasicUser_allowed(self, object, object_roles)
BasicUser.allowed = allowed
from App.config import getConfiguration
from AccessControl.User import SimpleUser
SimpleUser_getRoles = SimpleUser.getRoles
def getRoles(self):
def getRoles(self, _transactional_variable_pool=local()):
"""
Add Developer Role if the user has been explicitely set as Developer in Zope
configuration file
"""
role_tuple = SimpleUser_getRoles(self)
if role_tuple:
product_config = getattr(getConfiguration(), 'product_config', None)
if product_config:
config = product_config.get('erp5')
if config:
role_set = set(role_tuple)
user_id = self.getId()
if config and user_id in config.developer_list:
role_set.add('Developer')
elif user_id in role_set:
role_set.remove('Developer')
return role_set
return role_tuple
role_tuple = tuple(
x
for x in SimpleUser_getRoles(self)
if x != DEVELOPER_ROLE_ID
)
# Use our private transactional cache pool, to avoid code meddling with
# roles. Hide it in a default parameter value to make it harder to access
# than just importing it from the module.
try:
tv = _transactional_variable_pool.instance
except AttributeError:
tv = TransactionalVariable()
_transactional_variable_pool.instance = tv
try:
extra_role_tuple = tv['user_extra_role_tuple']
except KeyError:
tv['user_extra_role_tuple'] = extra_role_tuple = (
(DEVELOPER_ROLE_ID, )
if self.getId() in getattr(
getattr(
getConfiguration(),
'product_config',
{},
).get('erp5'),
'developer_list',
(),
) else
()
)
return role_tuple + extra_role_tuple
  • This code creates a cache entry whose content depends on the user but not the key. I have a problem in my ERP5 instance where I have Unauthorized while I'm zope.

    I created the following patch to have log:

    diff --git a/product/ERP5Type/patches/User.py b/product/ERP5Type/patches/User.py
    index 15733d6d7e..8dbe0c8542 100644
    --- a/product/ERP5Type/patches/User.py
    +++ b/product/ERP5Type/patches/User.py
    @@ -61,6 +61,7 @@ def getRoles(self, _transactional_variable_pool=local()):
         _transactional_variable_pool.instance = tv
       try:
         extra_role_tuple = tv['user_extra_role_tuple']
    +    print("ACCESS in cache (user %s, tv %s) : %s" % (self.getId(), id(tv), extra_role_tuple))
       except KeyError:
         tv['user_extra_role_tuple'] = extra_role_tuple = (
           (DEVELOPER_ROLE_ID, )
    @@ -75,6 +76,7 @@ def getRoles(self, _transactional_variable_pool=local()):
           ) else
           ()
         )
    +    print("ACCESS NOT in cache (user %s, tv %s)) : %s" % (self.getId(), id(tv), extra_role_tuple))
       return role_tuple + extra_role_tuple
     
     SimpleUser.getRoles = getRoles

    And here is the result of my log:

    ACCESS NOT in cache (user None, tv 140460701632912)) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user zope, tv 140460701632912) : ()
    ACCESS in cache (user zope, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ACCESS in cache (user None, tv 140460701632912) : ()
    ...

    See the 2 lines with zope in the middle...

  • /cc @jm who helped me debug

  • Oh, nice catch. Does the following improve the situation ? (you may want to apply by hand to keep your own changes)

    diff --git a/product/ERP5Type/patches/User.py b/product/ERP5Type/patches/User.py
    index 15733d6d7e..e12be532b0 100644
    --- a/product/ERP5Type/patches/User.py
    +++ b/product/ERP5Type/patches/User.py
    @@ -60,9 +60,9 @@ def getRoles(self, _transactional_variable_pool=local()):
         tv = TransactionalVariable()
         _transactional_variable_pool.instance = tv
       try:
    -    extra_role_tuple = tv['user_extra_role_tuple']
    +    extra_role_tuple = tv[('user_extra_role_tuple', self.getId())]
       except KeyError:
    -    tv['user_extra_role_tuple'] = extra_role_tuple = (
    +    tv[('user_extra_role_tuple', self.getId())] = extra_role_tuple = (
           (DEVELOPER_ROLE_ID, )
           if self.getId() in getattr(
             getattr(
  • Yes indeed, it works fine.

    ACCESS NOT in cache (user None, tv 139730790856808)) : ()
    ACCESS in cache (user None, tv 139730790856808) : ()
    ACCESS in cache (user None, tv 139730790856808) : ()
    ACCESS in cache (user None, tv 139730790856808) : ()
    ACCESS in cache (user None, tv 139730790856808) : ()
    ACCESS in cache (user None, tv 139730790856808) : ()
    ACCESS NOT in cache (user zope, tv 139730790856808)) : ('Developer',)
  • I don't understand the purpose of a cache in this function. Why not just have developer_list = ... at module level and do

    if self.getId() in developer_list:
      role_tuple += (DEVELOPER_ROLE_ID, )

    in the function.

    Edited by Julien Muchembled
  • IIRC this is a matter of initialisation order: at import time, Zope configuration would not be reachable yet.

    This said, a developer_list somewhere in this module could be filled upon first access, and used as a longer-lived cache. I guess I was being conservative compared to an initial getRoles which was accessing the configuration on every single call, in case this caused issues (...but not the kind of issue which happened here).

  • So still at module_level:

    class developer_list:
      def __contains__(self, item):
        global developer_list
        developer_list = ...
        return item in developer_list
    developer_list = developer_list()
  • Something like this, yes. Feel free to finalise and commit.

Please register or sign in to reply
SimpleUser.getRoles = getRoles
SimpleUser_getRolesInContext = SimpleUser.getRolesInContext
def getRolesInContext(self, object):
"""
Return the list of roles assigned to the user, including local roles
assigned in context of the passed in object.
"""
userid=self.getId()
roles=self.getRoles()
local={}
object=getattr(object, 'aq_inner', object)
userid = self.getId()
result = set()
object = aq_inner(object)
while 1:
local_roles = getattr(object, '__ac_local_roles__', None)
if local_roles:
if callable(local_roles):
local_roles=local_roles()
dict=local_roles or {}
for r in dict.get(userid, []):
local[r]=1
inner = getattr(object, 'aq_inner', object)
parent = getattr(inner, '__parent__', None)
local_role_dict = getattr(object, '__ac_local_roles__', None)
if local_role_dict:
if callable(local_role_dict):
local_role_dict = local_role_dict() or {}
result.update(local_role_dict.get(userid, ()))
parent = aq_parent(aq_inner(object))
if parent is not None:
object = parent
continue
if hasattr(object, 'im_self'):
object=object.im_self
object=getattr(object, 'aq_inner', object)
new = getattr(object, '__self__', None)
if new is not None:
object = aq_inner(new)
continue
break
# Patched: Developer role should never be available as local role
local.pop('Developer', None)
roles=list(roles) + local.keys()
return roles
result.discard(DEVELOPER_ROLE_ID)
result.update(self.getRoles())
  • If I read correctly, this changed the logic. Before this change, Developer role was removed from local only. If I understand the comment (# Patched: Developer role should never be available as local role) it means that if the user has Developer as a global role (because the user is listed in zope.conf) the role is returned, but not in case the Developer role was set as a local role.

    I think all this was a protection against a user adding themselves a Developer local role on a document to get the permission to edit it, but what matters for security is the result allowed check and allowed does not use getRolesInContext.

    This made getRolesInContext return less roles and has been like this for 3 years and we never noticed any problem, to me this means this SimpleUser.getRolesInContext patch was not necessary.

  • If I read correctly,

    I did not read correctly, forget about this comment

Please register or sign in to reply
return list(result)
SimpleUser.getRolesInContext = getRolesInContext
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment