From f363ac659f51a88bc2ed1e9dde8723288d4f5879 Mon Sep 17 00:00:00 2001 From: Vincent Pelletier <vincent@nexedi.com> Date: Wed, 23 Mar 2022 15:55:46 +0900 Subject: [PATCH] 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 | 72 +++++++++++++------- product/CMFActivity/tests/testCMFActivity.py | 36 ++++++++++ 2 files changed, 83 insertions(+), 25 deletions(-) diff --git a/product/CMFActivity/ActivityTool.py b/product/CMFActivity/ActivityTool.py index 8b7cb72f59..caf26b4e87 100644 --- a/product/CMFActivity/ActivityTool.py +++ b/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() diff --git a/product/CMFActivity/tests/testCMFActivity.py b/product/CMFActivity/tests/testCMFActivity.py index 2cec1738f6..8da38e14f2 100644 --- a/product/CMFActivity/tests/testCMFActivity.py +++ b/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 -- 2.30.9