ActivityTool: Use UnrestrictedMethod instead of ad-hoc user for process_timer

This is required cause while running  _activeSense for portal_alarm, we switch
user to nobody so as to use system user, but at the same time in proces_timer
in ActivityTool, we use the user of portal_catalog to invoke the activities.

https://lab.nexedi.com/nexedi/erp5/blob/master/product/ERP5/Document/Alarm.py#L164

The problem comes when someone runs an alarm which create a new portal_catalog,
which might not be having the required permission to invoke all the activites.
So, its better to use unrestrictedMethod or maybe, super_user here.

product/CMFActivity/ActivityTool.py

@@ -54,6 +54,7 @@ from zExceptions import ExceptionFormatter
 from BTrees.OIBTree import OIBTree
 from Zope2 import app
 from Products.ERP5Type.UnrestrictedMethod import PrivilegedUser
+from Products.ERP5Type.UnrestrictedMethod import UnrestrictedMethod
 from zope.site.hooks import setSite
 import transaction
 from App.config import getConfiguration
@@ -993,6 +994,7 @@ class ActivityTool (Folder, UniqueObject):
           LOG('CMFActivity', INFO, "Shutdown: Activities finished.")
 
     security.declareProtected(CMFCorePermissions.ManagePortal, 'process_timer')
+    @UnrestrictedMethod
     def process_timer(self, tick, interval, prev="", next=""):
       """
       Call distribute() if we are the Distributing Node and call tic()
@@ -1011,11 +1013,6 @@ class ActivityTool (Folder, UniqueObject):
           self.setupCurrentSkin(self.REQUEST)
           old_sm = getSecurityManager()
           try:
-            # get owner of portal_catalog, so normally we should be able to
-            # have the permission to invoke all activities
-            user = self.portal_catalog.getWrappedOwner()
-            newSecurityManager(self.REQUEST, user)
-
             currentNode = self.getCurrentNode()
             self.registerNode(currentNode)
             processing_node_list = self.getNodeList(role=ROLE_PROCESSING)

[Kazuhiko Shiozaki]

@vpelletier , @jm, @jerome, @romain

This commit is somehow 'proof of concept' commit. If we use UnrestrictedMethod here, we don't need to store and restore current security manager in this method.

As well written in the commit message above, 'switching to portal_catalog's owner' here is wrong, at least it is not a good idea. I guess that here we care only permissions, that are bypassed by UnrestrictedMethod, and we don't care the user invoking the internal code of process_timer(). Can you please verify ?

[Ayush Tiwari]

I have also created a separate test suite with this commit based on master branch. Will update with the link when it finishes running all tests.

[Jérome Perrin]

@tiwariayush do you have a traceback of a failing test to make it easier for others to understand the problem ?

I remember it was an unauthorized error occuring when evaluating filters of catalog method. This unauthorized occurs during indexing. Indexing is done using grouped activity and we do not switch back to the "original user that triggered this activity" for grouped activities, we just execute them under the security context of the owner of portal catalog. So when portal catalog has no valid owner this approach does not work.

I guess that here we care only permissions

If I understand correctly, it means that with nexedi/slapos@58c9f7c3 we should not have problems coming from ownership.

We have other places where we switch to security context of owner of some documents ( Alarms and InotifyTool ), but I don't know why we just use ERP5Securtity's super user here.

[Kazuhiko Shiozaki]

In AlarmTool.tic(), we switch to the alarm's owner, then call alarm.solve() and alarm.activeSense() and we anyway use 'super_user' there, thus we no longer care at all the owner of Alarm in reality, even though we still have user switching code...

[Ayush Tiwari]

Sorry for late reply, here's the traceback of the failing test:

Last error message:
Unauthorized
You are not allowed to access 'isResourceType' in this context
Traceback (innermost last):
  Module Products.CMFActivity.ActivityTool, line 1338, in invokeGroup
    traverse(method_id)(expanded_object_list)
  Module Products.ERP5Catalog.ERP5CatalogTool, line 935, in catalogObjectList
    super(ERP5CatalogTool, self).catalogObjectList(tmp_object_list, **m.kw)
  Module Products.ZSQLCatalog.ZSQLCatalog, line 839, in catalogObjectList
    catalog.catalogObjectList(wrapped_object_list, **kw)
  Module Products.ZSQLCatalog.SQLCatalog, line 1456, in catalogObjectList
    idxs=idxs)
  Module Products.ERP5Catalog.ERP5Catalog, line 451, in _catalogObjectList
    result = expression(econtext)
  Module Products.CMFCore.Expression, line 47, in __call__
    res = compiled(econtext)
  Module Products.PageTemplates.ZRPythonExpr, line 48, in __call__
   - __traceback_info__: context.isResourceType()
    return eval(self._code, vars, {})
  Module PythonExpr, line 1, in <expression>
    
Unauthorized: You are not allowed to access 'isResourceType' in this context

This error arise while running the test for upgrader, where I had added a script constraint which was creating new portal_catalog object. The problem with this was that while indexing, the user considered was of portal_catalog.

[Ayush Tiwari]

Tests are almost finished for this commit. Test Result Link

[Ayush Tiwari]

Looks like we are switching user in __call__ function here also, which again leads to Unauthorized error.

ActivityTool.Message.call

Error Link

[Vincent Pelletier]

Looks like we are switching user in call function here also, which again leads to Unauthorized error.

This is needed so each activity runs as the user which triggered it. Grouped activities are an exception to this rule, but this is kind of an internal CMFActivity issue (maybe we should refuse grouping activities spawned by different users, but we still want to group indexation activities...).

I am not sure what the "TO BE DONE" comment is refering to (Maybe adding support for no-switch activities ? This sounds dangerous and unlikely to be really needed.). This implementation is not complete either as the activity method is accessed without checking that the activity creator user has the right to actually access that method. As typically the method is a python script and as that script will likely access context, restriction will be detected and raise an error. But if one is able to spawn arbitrary activities he could use this to call a method he should not have access to. A similar simptom is visible in the indexation issue you found because the filter expressions are (here) the first restricted python being executed.

Actually, I think it is indexation methods which should always be run as an unrestricted user: indexation result must not depend on what the user can do, and they must not expose (ex: as a return value) any data from indexed objects. Also, if some code was to trigger indexation without an activity in the middle (which is currently forbidden for reasons unrelated to security anyway) it would not benefit for a fix happening on CMFActivity level.