Products.CMFActivity.ActivityTool: Store user object in activity.

When spawning an activity, store the current security context's user in
the Message object itself, so the activity security context can be
re-created with the same security during activity execution.
This allows a user to be modified (different groups, global roles, maybe
removed altogether) after they spawned activities and before these activities
could run.
It also means that any temporary custom group or global role granted to
that user (by a privilege elevation mechanism out of the scope of this
change) will still be effective during the activity execution.
This follows the principle that
  foo.activate(...).bar(...)
should be equivalent to its "immediate execution" version
  foo.bar(...)
by ensuring that the security context of the activity is the same as the
one which was applied to the code which spawned that activity,
independently of any intermediate configuration change - hence improving
(deferred and fragmentary) transaction isolation.

This also removes the need to look the user up, then looking up their
assignments (and other documents involved in group computation), etc,
saving the cost of these calls.

Also, remove redundant user_name argument of Message.changeUser method.

product/CMFActivity/ActivityTool.py

@@ -27,6 +27,7 @@ from __future__ import absolute_import
 #
 ##############################################################################
 
+import copy
 import socket
 import urllib
 import threading
@@ -195,6 +196,9 @@ class Message(BaseMessage):
   exc_type = None
   is_executed = MESSAGE_NOT_EXECUTED
   traceback = None
+  user_name = None
+  user_object = None
+  user_folder_path = None
   document_uid = None
   is_registered = False
   line = None
@@ -224,7 +228,14 @@ class Message(BaseMessage):
       # was generated.
       # Strip last stack entry, since it will always be the same.
       self.call_traceback = ''.join(format_list(extract_stack()[:-1]))
-    self.user_name = getSecurityManager().getUser().getIdOrUserName()
+    user = getSecurityManager().getUser()
+    self.user_object = copy.deepcopy(aq_base(user))
+    # Note: userfolders are not ERP5 objects, so use OFS API.
+    self.user_folder_path = getattr(
+      aq_parent(user),
+      'getPhysicalPath',
+      lambda: None,
+    )()
     # Store REQUEST Info
     self.request_info = {}
     if request is not None:
@@ -298,31 +309,42 @@ class Message(BaseMessage):
         pass
     return 1
 
-  def changeUser(self, user_name, activity_tool):
+  def changeUser(self, activity_tool):
     """restore the security context for the calling user."""
     portal = activity_tool.getPortalObject()
-    portal_uf = portal.acl_users
-    uf = portal_uf
-    user = uf.getUserById(user_name)
-    # if the user is not found, try to get it from a parent acl_users
-    # XXX this is still far from perfect, because we need to store all
-    # information about the user (like original user folder, roles) to
-    # replay the activity with exactly the same security context as if
-    # it had been executed without activity.
-    if user is None:
-      uf = portal.aq_parent.acl_users
-      user = uf.getUserById(user_name)
-    if user is None and user_name == system_user.getUserName():
-      # The following logic partly comes from unrestricted_apply()
-      # implementation in ERP5Type.UnrestrictedMethod but we get roles
-      # from the portal to have more roles.
-      uf = portal_uf
-      role_list = uf.valid_roles()
-      user = PrivilegedUser(user_name, None, role_list, ()).__of__(uf)
+    user = self.user_object
+    if user is None and self.user_name is not None: # BBB
+      user_name = self.user_name
+      user_folder = portal_user_folder = portal.acl_users
+      user = user_folder.getUserById(user_name)
+      # if the user is not found, try to get it from a parent acl_users
+      # XXX this is still far from perfect, because we need to store all
+      # information about the user (like original user folder, roles) to
+      # replay the activity with exactly the same security context as if
+      # it had been executed without activity.
+      if user is None:
+        user_folder = portal.aq_parent.acl_users
+        user = user_folder.getUserById(user_name)
+      if user is None and user_name == system_user.getUserName():
+        # The following logic partly comes from unrestricted_apply()
+        # implementation in ERP5Type.UnrestrictedMethod but we get roles
+        # from the portal to have more roles.
+        user_folder = portal_user_folder
+        user = PrivilegedUser(
+          user_name,
+          None,
+          user_folder.valid_roles(),
+          (),
+        )
+    else:
+      user_folder = portal.getPhysicalRoot().unrestrictedTraverse(
+        self.user_folder_path,
+      )
+      user_name = user.getIdOrUserName()
     if user is not None:
-      user = user.__of__(uf)
+      user = user.__of__(user_folder)
       newSecurityManager(None, user)
-      transaction.get().setUser(user_name, '/'.join(uf.getPhysicalPath()))
+      transaction.get().setUser(user_name, '/'.join(user_folder.getPhysicalPath()))
     else :
       LOG("CMFActivity", WARNING,
           "Unable to find user %r in the portal" % user_name)
@@ -347,7 +369,7 @@ class Message(BaseMessage):
         try:
           # Change user if required (TO BE DONE)
           # We will change the user only in order to execute this method
-          self.changeUser(self.user_name, activity_tool)
+          self.changeUser(activity_tool)
           # XXX: There is no check to see if user is allowed to access
           #      that method !
           method = getattr(obj, self.method_id)
@@ -420,7 +442,7 @@ Named Parameters: %r
     try:
       # Change user if required (TO BE DONE)
       # We will change the user only in order to execute this method
-      user = self.changeUser(self.user_name, activity_tool)
+      user = self.changeUser(activity_tool)
       active_obj = obj.activate(activity=activity, **self.activity_kw)
       getattr(active_obj, self.method_id)(*self.args, **self.kw)
     finally:
@@ -1664,7 +1686,7 @@ class ActivityTool (BaseTool):
               message = m._message
               if user_name != message.user_name:
                 user_name = message.user_name
-                message.changeUser(user_name, m.object)
+                message.changeUser(m.object)
               m.result = getattr(m.object, method_id)(*m.args, **m.kw)
           except Exception:
             m.raised()

product/CMFActivity/tests/testCMFActivity.py

@@ -30,6 +30,10 @@ import inspect
 import warnings
 from functools import wraps
 from itertools import product
+from AccessControl.SecurityManagement import getSecurityManager
+from AccessControl.SecurityManagement import setSecurityManager
+from AccessControl.SecurityManagement import newSecurityManager
+from Acquisition import aq_base, aq_parent
 from Products.ERP5Type.tests.utils import LogInterceptor
 from Testing import ZopeTestCase
 from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase
@@ -40,6 +44,7 @@ from Products.CMFActivity.Activity.SQLBase import INVOKE_ERROR_STATE
 from Products.CMFActivity.Activity.Queue import VALIDATION_ERROR_DELAY
 from Products.CMFActivity.Activity.SQLDict import SQLDict
 from Products.CMFActivity.Errors import ActivityPendingError, ActivityFlushError
+from Products.PluggableAuthService.PropertiedUser import PropertiedUser
 from erp5.portal_type import Organisation
 from AccessControl.SecurityManagement import newSecurityManager
 from zLOG import LOG
@@ -2798,3 +2803,34 @@ return [x.getObject() for x in context.portal_catalog(limit=100)]
       self.portal.portal_activities.manageActivitiesAdvanced()
       self.portal.portal_activities.manageLoadBalancing()
     self.assertEqual(catched_warnings, [])
+
+  @for_each_activity
+  def testSpawnTimeUserGroupAndRoleUsedDuringExecution(self, activity):
+    obj = self.portal.organisation_module.newContent(portal_type='Organisation')
+    self.tic()
+    # This user cannot be created by userfolder API, validating that activity
+    # execution does not use it.
+    # Using a PropertiedUser because it is the lowest-level class which has a
+    # groups notion.
+    artificial_user = PropertiedUser(
+      id='this user does not exist',
+      login='does not matter',
+    ).__of__(self.portal.acl_users)
+    artificial_user._addGroups(groups=('group 1', 'group 2'))
+    artificial_user._addRoles(roles=('role 1', 'role 2'))
+    initial_security_manager = getSecurityManager()
+    def checkUserGroupAndRole(organisation_self):
+      user = getSecurityManager().getUser()
+      self.assertIs(type(aq_base(user)), PropertiedUser)
+      self.assertEqual(aq_parent(user), aq_parent(artificial_user))
+      self.assertEqual(user.getId(), artificial_user.getId())
+      self.assertItemsEqual(user.getGroups(), artificial_user.getGroups())
+      self.assertItemsEqual(user.getRoles(), artificial_user.getRoles())
+    Organisation.checkUserGroupAndRole = checkUserGroupAndRole
+    try:
+      newSecurityManager(None, artificial_user)
+      obj.activate(activity=activity).checkUserGroupAndRole()
+      self.tic()
+    finally:
+      setSecurityManager(initial_security_manager)
+      del Organisation.checkUserGroupAndRole