Base._getAcquireLocalRoles: Optimise.

This is called when checking access permission on objects, which happens
very often. CachingMethod has a hit cost which is too high for this use.
Instead, generate this method as part of the portal type class, removing
all call-time logic.

product/ERP5/bootstrap/erp5_core/WorkflowTemplateItem/portal_workflow/dynamic_class_generation_interaction_workflow/interactions/BaseType_resetDynamicClasses.xml

@@ -61,6 +61,7 @@
                 <string>_setTypeInterface.*</string>
                 <string>_setTypePropertySheet.*</string>
                 <string>_setTypeBaseCategory.*</string>
+                <string>_setTypeAcquireLocalRole</string>
               </list>
             </value>
         </item>

product/ERP5Catalog/tests/testERP5Catalog.py

@@ -1609,8 +1609,8 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
 
     # Check that Owner is not catalogued when he can't view the
     # object and when the portal type does not acquire the local roles.
-    sub_portal_type.acquire_local_roles = False
-    self.portal.portal_caches.clearAllCache()
+    sub_portal_type.setTypeAcquireLocalRole(False)
+    self.commit() # So dynamic class gets updated for setTypeAcquireLocalRole change
     logout()
     self.loginByUserName('super_owner')
     obj = folder.newContent(portal_type='Organisation')
@@ -1625,8 +1625,8 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
 
     # Check that Owner is catalogued when he can view the
     # object and when the portal type does not acquire the local roles.
-    sub_portal_type.acquire_local_roles = False
-    self.portal.portal_caches.clearAllCache()
+    sub_portal_type.setTypeAcquireLocalRole(False)
+    self.commit() # So dynamic class gets updated for setTypeAcquireLocalRole change
     logout()
     self.loginByUserName('super_owner')
     obj = folder.newContent(portal_type='Organisation')
@@ -1642,8 +1642,8 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
     # Check that Owner is catalogued when he can view the
     # object because permissions are acquired and when the portal type does not
     # acquire the local roles.
-    sub_portal_type.acquire_local_roles = False
-    self.portal.portal_caches.clearAllCache()
+    sub_portal_type.setTypeAcquireLocalRole(False)
+    self.commit() # So dynamic class gets updated for setTypeAcquireLocalRole change
     logout()
     self.loginByUserName('super_owner')
     obj = folder.newContent(portal_type='Organisation')
@@ -1658,8 +1658,8 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
 
     # Check that Owner is not catalogued when he can't view the
     # object and when the portal type acquires the local roles.
-    sub_portal_type.acquire_local_roles = True
-    self.portal.portal_caches.clearAllCache()
+    sub_portal_type.setTypeAcquireLocalRole(True)
+    self.commit() # So dynamic class gets updated for setTypeAcquireLocalRole change
     logout()
     self.loginByUserName('super_owner')
     obj = folder.newContent(portal_type='Organisation')
@@ -1674,8 +1674,8 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
 
     # Check that Owner is catalogued when he can view the
     # object and when the portal type acquires the local roles.
-    sub_portal_type.acquire_local_roles = True
-    self.portal.portal_caches.clearAllCache()
+    sub_portal_type.setTypeAcquireLocalRole(True)
+    self.commit() # So dynamic class gets updated for setTypeAcquireLocalRole change
     logout()
     self.loginByUserName('super_owner')
     obj = folder.newContent(portal_type='Organisation')
@@ -1691,8 +1691,8 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
     # Check that Owner is catalogued when he can view the
     # object because permissions are acquired and when the portal type
     # acquires the local roles.
-    sub_portal_type.acquire_local_roles = True
-    self.portal.portal_caches.clearAllCache()
+    sub_portal_type.setTypeAcquireLocalRole(True)
+    self.commit() # So dynamic class gets updated for setTypeAcquireLocalRole change
     logout()
     self.loginByUserName('super_owner')
     obj = folder.newContent(portal_type='Organisation')
@@ -1840,14 +1840,13 @@ class TestERP5Catalog(ERP5TypeTestCase, LogInterceptor):
     person_module = portal.person_module
     person = 'Person'
     person_portal_type = portal_types._getOb(person)
-    person_portal_type.acquire_local_roles = False
+    person_portal_type.setTypeAcquireLocalRole(False)
     # Organisation stuff
     organisation_module = portal.organisation_module
     organisation = 'Organisation'
     organisation_portal_type = portal_types._getOb(organisation)
-    organisation_portal_type.acquire_local_roles = True
-
-    self.portal.portal_caches.clearAllCache()
+    organisation_portal_type.setTypeAcquireLocalRole(True)
+    self.commit() # So dynamic class gets updated for setTypeAcquireLocalRole change
 
     def newContent(container, portal_type, acquire_view_permission, view_role_list, local_role_dict):
       document = container.newContent(portal_type=portal_type)

product/ERP5Security/tests/testERP5Security.py

@@ -1008,7 +1008,8 @@ class TestLocalRoleManagement(RoleManagementTestCase):
     """Tests that document does not acquire loal roles from their parents if
     "acquire local roles" is not checked."""
     ti = self._getTypeInfo()
-    ti.acquire_local_roles = False
+    ti.setTypeAcquireLocalRole(False)
+    self.commit() # So dynamic class gets updated for setTypeAcquireLocalRole change
     self._getModuleTypeInfo().newContent(portal_type='Role Information',
       role_name='Assignor',
       description='desc.',

product/ERP5Type/Base.py

@@ -3042,16 +3042,13 @@ class Base( CopyContainer,
       - False means that the role acquisition chain is cut.
 
     The code to support this is on the user class, see
-    ERP5Security.ERP5UserFactory.ERP5User
-    """
-    def cached_getAcquireLocalRoles(portal_type):
-      ti = self._getTypesTool().getTypeInfo(portal_type)
-      return ti is None or ti.getTypeAcquireLocalRole()
+    ERP5Type.patches.User and ERP5Type.patches.PropertiedUser .
 
-    cached_getAcquireLocalRoles = CachingMethod(cached_getAcquireLocalRoles,
-                                                id='Base__getAcquireLocalRoles',
-                                                cache_factory='erp5_content_short')
-    return cached_getAcquireLocalRoles(portal_type=self.getPortalType())
+    This specific implementation of this method should only be reached when
+    processing an object with a missing portal type, otherwise portal type
+    class should hold such accessor.
+    """
+    return True
 
   security.declareProtected(Permissions.AccessContentsInformation,
                             'get_local_permissions')

product/ERP5Type/ERP5Type.py

@@ -534,12 +534,12 @@ class ERP5TypeInformation(XMLObject,
       return getPropertySheetValueList(self.getPortalObject(),
                                        property_sheet_name_set)
 
-    # XXX these methods, _baseGetTypeClass, getTypeMixinList, and
-    # getTypeInterfaceList, are required for a bootstrap issue that
-    # the portal type class Base Type is required for _aq_dynamic on
-    # Base Type. So surpress calling _aq_dynamic when obtaining information
-    # required for generating a portal type class by declaring these methods
-    # explicitly.
+    # XXX these methods, _baseGetTypeClass, getTypeMixinList,
+    # getTypeAcquireLocalRole and getTypeInterfaceList, are required for a
+    # bootstrap issue that the portal type class Base Type is required for
+    # _aq_dynamic on Base Type. So surpress calling _aq_dynamic when obtaining
+    # information required for generating a portal type class by declaring
+    # these methods explicitly.
     def _baseGetTypeClass(self):
       return getattr(aq_base(self), 'type_class', None)
 
@@ -553,6 +553,11 @@ class ERP5TypeInformation(XMLObject,
     def getTypeMixinList(self):
       return getattr(aq_base(self), 'type_mixin', ())
 
+    security.declareProtected(Permissions.AccessContentsInformation,
+                              'getTypeAcquireLocalRole')
+    def getTypeAcquireLocalRole(self):
+      return getattr(aq_base(self), 'acquire_local_roles', None)
+
     security.declareProtected(Permissions.AccessContentsInformation,
                               'getTypeInterfaceList')
     def getTypeInterfaceList(self):

product/ERP5Type/dynamic/portal_type_class.py

@@ -39,10 +39,13 @@ from Products.ERP5Type.Globals import InitializeClass
 from Products.ERP5Type.Utils import setDefaultClassProperties
 from Products.ERP5Type import document_class_registry, mixin_class_registry
 from Products.ERP5Type.dynamic.accessor_holder import createAllAccessorHolderList
+from Products.ERP5Type.Accessor.Constant import Getter as ConstantGetter
 from Products.ERP5Type.TransactionalVariable import TransactionalResource
 
 from zLOG import LOG, ERROR, INFO, WARNING, PANIC
 
+ACQUIRE_LOCAL_ROLE_GETTER_ID = '_getAcquireLocalRoles'
+
 def _importClass(classpath):
   try:
     module_path, class_name = classpath.rsplit('.', 1)
@@ -153,6 +156,11 @@ def generatePortalTypeClass(site, portal_type_name):
     interface_list = portal_type.getTypeInterfaceList()
     portal_type_category_list = portal_type.getTypeBaseCategoryList()
     attribute_dict['_categories'] = portal_type_category_list[:]
+    attribute_dict[ACQUIRE_LOCAL_ROLE_GETTER_ID] = ConstantGetter(
+      id=ACQUIRE_LOCAL_ROLE_GETTER_ID,
+      key=None,
+      value=bool(portal_type.getTypeAcquireLocalRole()),
+    )
   else:
     LOG("ERP5Type.dynamic", WARNING,
         "Cannot find a portal type definition for '%s', trying to guess..."

product/ERP5Type/tests/testERP5Type.py

@@ -146,8 +146,7 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
 
       # set Person.acquire_local_roles back.
       if getattr(self, 'person_acquire_local_roles', None) is not None:
-        self.getTypesTool().getTypeInfo('Person').acquire_local_roles = self.person_acquire_local_roles
-        self.portal.portal_caches.clearAllCache()
+        self.getTypesTool().getTypeInfo('Person').setTypeAcquireLocalRole(self.person_acquire_local_roles)
 
       # restore workflows for other tests
       self.getWorkflowTool().setChainForPortalTypes(
@@ -1653,9 +1652,8 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
 
       # turn on Person.acquire_local_roles
       person = self.getTypesTool().getTypeInfo('Person')
-      self.person_acquire_local_roles = person.acquire_local_roles
-      person.acquire_local_roles = True
-      portal.portal_caches.clearAllCache()
+      self.person_acquire_local_roles = person.getTypeAcquireLocalRole()
+      person.setTypeAcquireLocalRole(True)
 
       # Make a plain user.
       uf = portal.acl_users

[Sebastien Robin]

I have a project overriding for some portal types like "PDF" or "Image" the method _getAcquireLocalRoles:

  def _getAcquireLocalRoles(self):
    """
    Override for MyProject default method to acquire local roles only if document is
    embedded
    """
    result = False
    if not(self.getRelativeUrl().startswith("document_module/")):
      result = True
    return result

So it was able to have different behavior depending if we had the portal type embedded in another type or not (in document module, documents are not embedded, in other places they are).

So with this patch, _getAcquireLocalRoles is not retrieved at the class level any more, thus my code is no longer called. So we looks like to have now way to make dynamic the choice of having or not acquisition of roles.

In my case, some documents like PDF are embedded inside other types of objects (like Mail Message), each having it's own security setup. So this method was very convenient. I can hardly find a way to solve my issue, and I don't want to patch portal_type_class itself.

@nexedi No one else having trouble with this ?

[Vincent Pelletier]

So with this patch, _getAcquireLocalRoles is not retrieved at the class level any more, thus my code is no longer called.

I think a generic fix can be:

• Remove Base._getAcquireLocalRoles: anyway a missing portal type will likely not work properly, and I see generatePortalTypeClass actually raises when it fails to find one.
• Make generatePortalTypeClass only set _getAcquireLocalRoles constant getter from Portal Type configuration if this method is not already implemented by a superclass.

Another idea would be to change/add the possibility of inputing a TALES expression at portal_type level, but I am worried this will have a non-trivial run-time cost: every single restricted action on that object will cause a TALES environment to be setup (I do not think we can cache it in this case), and TALES evaluator involved instead of just 4 lines of nice, native python... There can be a lot of such accesses because of zope security magic, so even a tiny per-call slowdown can cause a much bigger effect at whole-transaction level.

Also, I think your use-case is a very good one, and I'm surprised this is not in generic code (maybe I missed a discussion about this ?).

[Jérome Perrin]

Also, I think your use-case is a very good one, and I'm surprised this is not in generic code (maybe I missed a discussion about this ?).

Generic code address this by using two different portal types, Embedded File (acquire local roles) vs File (does not acquire local roles).

[Vincent Pelletier]

Oh, that's a good fix too indeed. But I think I can also make _getAcquireLocalRoles ConstantGetter less disruptive.

[Vincent Pelletier]

I opened !857 (closed) with a tentative fix.

[Kazuhiko Shiozaki]

I vote for using Embedded File or introducing yet another Embedded Xxx portal types. Maybe Embedded File can have more intelligent 'preview' action by checking content_type ?

[Sebastien Robin]

So I was not so happy with the "Embedded" dedicated portal type solution. For Mail Message I wanted to use various types instead of only "Embedded File", because "PDF", "Image", "Text", "Drawing", etc have nice preview and are not just files. This is not the only issue of preview, we might also think about cases where we would like also the benefit of dedicated gadget editors depending on types. So I would need "Embedded PDF", "Embedded Image", etc. This makes a lot of duplication while the only difference I need is this property of acquisition of roles. All security setting is dynamic and very flexible (all roles), except that property.

So I think it is nice to keep flexibility.

Thanks Vincent for patch, I'm going to try.

[Jérome Perrin]

In my understanding, File has this special semantic that ERP5 does not try to perform any conversion and the only thing one can do with a File is download it. I think it's intentionally not clever, because we have DMS for "clever" types.

@seb what was the reason not to use document module to store the documents ?

[Kazuhiko Shiozaki]

@seb You can create a dedicated Role definition, whose condition is 'parent is NOT document_module' and base category script returns 'calculated result based on parent document'. Isn't it enough ?

[Sebastien Robin]

@jerome my usage is to see attachments of mail message in a user friendly way. It is still possible to create document in document module and display a listbox showing related documents. But email attachments are not supposed to be used outside of email, and all their security is strictly linked to the security of mail message. In document module, I prefer in this project to have documents created by users, not whatever mess can come from emails. Then they are free to create a document if they really need to share something that was received by mail message. So if at the end I need to create an "hidden document module", this is still possible, but not sure it sounds nice.

@kazuhiko probably possible, but I need in such case to duplicate all role definition of Mail Message, and then also add interactions every time security is calculated on Mail Message itself. And this would need to be done for all types PDF, Image, etc... I clearly prefer to have the flexibility to define a 4 lines of code method like I was doing up to now.

@vpelletier many thanks, my failing tests checking security are now passing with your patch

[Vincent Pelletier]

my failing tests checking security are now passing with your patch

Great to hear.

So I'll rebase & push to public master once testrunner 2 is happy (which likely means: beginning of next week).

[Vincent Pelletier]

(which likely means: beginning of next week).

Sadly, some tests failed because we have at least 2 "document" types which do not have a portal type, so they do not go through portal type class generation and raise when indexed:

• testERP5Promise.TestERP5Promise.test_promise_certificate_autority_tool : Certificate Authority Tool has no portal type.
• ERP5Form has no portal type. Or does. Sometimes. But apparently not when created the ZMI way (which creates Products.ERP5Form.Form.ERP5Form which is mistaken by catalog as a document type), but still somehow someone could create them as document objects, because they are committed as instance of erp5.portal_types.ERP5 Form. And the portal type gets created when migrating ERP5 Catalog from old version to latest version by installing it from a non-bootstrap business template by using a bootstrap-only method which still somehow works in that case... What a mess.

I suspect there are more cases, but so far these are the ones I found by checking failing tests. And I also found a few more failing tests (one because of my ZSQLCatalog fixes).

I cannot spend more time on this now, so I sadly have to give up. Please, someone familiar with ERP5 Form vs. ERP5Form:

• move the portal type into erp5_core, with all essential portal types
• alias ERP5Form product class to the portal type class somehow, so that all existing instances are automatically migrated on instanciation

[Vincent Pelletier]

And I could find some time today and got ideas of simpler fixes for the 2 classes requiring special handling. The result passes tests. I closed the merge request.