Review guards on PythonScript/ExternalMethod and expose a method to check them

- Code refactoring and small optimizations.
- Really fix unwanted acquisition.
- ExternalMethod: fix security declarations.
- Fix role/permission checking for ExternalMethod called by a PythonScript
  with proxy roles.
- When editing an existing guard, modify it instead of always recreate one.
  no very useful here, but that's good practice: this is nicer for the ZODB
  and it's easier to browse the history. BT should do the same when upgrading.

product/ERP5Type/patches/ExternalMethod.py

@@ -13,13 +13,8 @@
 
 from inspect import getargs
 from Products.ExternalMethod.ExternalMethod import *
-from AccessControl import ModuleSecurityInfo
 from Products.ERP5Type.Globals import InitializeClass
-from Acquisition import aq_parent
-from Products.ERP5Type.patches.PythonScript import _guard_form, \
-     _guard_manage_options, checkGuard, getGuard, manage_guardForm, \
-     manage_setGuard
-from zExceptions import Forbidden
+from .PythonScript import addGuard
 
 if 1:
     def getFunction(self, reload=False, f=None):
@@ -89,10 +84,7 @@ if 1:
         - fix magic "self" argument when positional arguments get their values
           from kw.
         """
-        guard = getattr(self, 'guard', None)
-        if guard is not None:
-            if not checkGuard(guard, aq_parent(self)):
-                raise Forbidden, 'Calling %s %s is denied by Guard.' % (self.meta_type, self.id)
+        self.checkGuard(True)
 
         import erp5.component.extension
         component_module = erp5.component.extension.find_load_module(self._module)
@@ -147,17 +139,8 @@ if 1:
 
     ExternalMethod.__call__ = __call__
 
-security = ModuleSecurityInfo('Products.ExternalMethod.ExternalMethod.ExternalMethod')
+    ExternalMethod.security = ClassSecurityInfo()
 
-ExternalMethod.manage_options += _guard_manage_options
-ExternalMethod._guard_form = _guard_form
+    addGuard(ExternalMethod, change_external_methods)
 
-ExternalMethod.manage_guardForm = manage_guardForm
-security.declareProtected(view_management_screens, 'manage_guardForm')
-
-ExternalMethod.getGuard = getGuard
-
-ExternalMethod.manage_setGuard = manage_setGuard
-security.declareProtected(change_external_methods, 'manage_setGuard')
-
-InitializeClass(ExternalMethod)
+    InitializeClass(ExternalMethod)

product/ERP5Type/patches/PythonScript.py

@@ -13,7 +13,7 @@
 from Products.DCWorkflow.Guard import Guard
 from Products.PythonScripts.PythonScript import PythonScript
 from App.special_dtml import DTMLFile
-from Products.ERP5Type import _dtmldir
+from .. import _dtmldir
 from AccessControl import ClassSecurityInfo, getSecurityManager
 from AccessControl.class_init import InitializeClass
 from AccessControl.PermissionRole import rolesForPermissionOn
@@ -62,17 +62,17 @@ PythonScript.manage_main = manage_editForm
 PythonScript.manage_editDocument = manage_editForm
 PythonScript.manage_editForm = manage_editForm
 
+### Guards
+
 _guard_manage_options = (
   {
     'label':'Guard',
     'action':'manage_guardForm',
   },
 )
-PythonScript.manage_options += _guard_manage_options
 
 _guard_form = DTMLFile(
   'editGuardForm', _dtmldir)
-PythonScript._guard_form = _guard_form
 
 def manage_guardForm(self, REQUEST, manage_tabs_message=None):
   '''
@@ -81,64 +81,63 @@ def manage_guardForm(self, REQUEST, manage_tabs_message=None):
                           management_view='Guard',
                           manage_tabs_message=manage_tabs_message,
     )
-PythonScript.manage_guardForm = manage_guardForm
-security.declareProtected('View management screens', 'manage_guardForm')
 
 def manage_setGuard(self, props=None, REQUEST=None):
   '''
   '''
   g = Guard()
   if g.changeFromProperties(props or REQUEST):
-    self.guard = g
+    guard = self.guard
+    if guard is None:
+      self.guard = g
+    else:
+      guard._p_activate()
+      if guard.__dict__ != g.__dict__:
+        guard.__dict__.clear()
+        guard.__dict__.update(g.__dict__)
+        guard._p_changed = 1
   else:
-    self.guard = None
+    try:
+      del self.guard
+    except AttributeError:
+      pass
   if REQUEST is not None:
     return self.manage_guardForm(REQUEST, 'Properties changed.')
-PythonScript.manage_setGuard = manage_setGuard
-security.declareProtected('Change Python Scripts', 'manage_setGuard')
 
 def getGuard(self):
-  guard = getattr(self, 'guard', None)
-  if guard is not None:
-    return guard
-  else:
+  guard = self.guard
+  if guard is None:
     return Guard().__of__(self)  # Create a temporary guard.
-PythonScript.getGuard = getGuard
-
-def checkGuard(guard, ob):
+  return guard
+
+def getRoles(ob):
+  sm = getSecurityManager()
+  stack = sm._context.stack
+  if stack:
+    proxy_roles = getattr(stack[-1], '_proxy_roles', None)
+    if proxy_roles:
+      return set(proxy_roles)
+  return set(sm.getUser().getRolesInContext(ob))
+
+def _checkGuard(guard, ob):
   # returns 1 if guard passes against ob, else 0.
   # TODO : implement TALES evaluation by defining an appropriate
   # context.
-  u_roles = None
-  def getRoles():
-    sm = getSecurityManager()
-    u = sm.getUser()
-    stack = sm._context.stack
-    if stack and len(stack) > 1:
-      eo = stack[-2] # -1 is the current script.
-      proxy_roles = getattr(eo, '_proxy_roles', None)
-      if proxy_roles:
-        roles = proxy_roles
-        return proxy_roles
-    roles = u.getRolesInContext(ob)
-    return roles
   if guard.permissions:
     # Require at least one role for required roles for the given permission.
-    if u_roles is None:
-      u_roles = getRoles()
+    u_roles = getRoles(ob)
     for p in guard.permissions:
-      if set(rolesForPermissionOn(p, ob)).intersection(u_roles):
+      if not u_roles.isdisjoint(rolesForPermissionOn(p, ob)):
         break
     else:
       return 0
+  else:
+    u_roles = None
   if guard.roles:
     # Require at least one of the given roles.
     if u_roles is None:
-      u_roles = getRoles()
-    for role in guard.roles:
-      if role in u_roles:
-        break
-    else:
+      u_roles = getRoles(ob)
+    if u_roles.isdisjoint(guard.roles):
       return 0
   if guard.groups:
     # Require at least one of the specified groups.
@@ -158,15 +157,42 @@ def checkGuard(guard, ob):
       return 0
   return 1
 
-PythonScript_exec = PythonScript._exec
-def _exec(self, *args):
-  # PATCH BEGIN : check guard against context, if guard exists.
-  guard = getattr(aq_base(self), 'guard', None)
-  if guard is not None:
-    if not checkGuard(guard, aq_parent(self)):
-      raise Forbidden, 'Calling %s %s is denied by Guard.' % (self.meta_type, self.id)
-  # PATCH END
-  return PythonScript_exec(self, *args)
-PythonScript._exec = _exec
+def checkGuard(aq_parent=aq_parent, _checkGuard=_checkGuard):
+  def checkGuard(self, _exec=False):
+    guard = self.guard
+    if guard is None or _checkGuard(guard, aq_parent(self)):
+      return 1
+    if _exec:
+      raise Forbidden('Calling %s %s is denied by Guard.'
+                      % (self.meta_type, self.id))
+  return checkGuard
+checkGuard = checkGuard()
+
+def addGuard(cls, set_permission):
+  security = cls.security
+
+  cls.guard = None
+  cls.getGuard = getGuard
+  cls.checkGuard = checkGuard
+
+  cls.manage_options += _guard_manage_options
+  cls._guard_form = _guard_form
+
+  security.declareProtected('View management screens', 'manage_guardForm')
+  cls.manage_guardForm = manage_guardForm
+
+  security.declareProtected(set_permission, 'manage_setGuard')
+  cls.manage_setGuard = manage_setGuard
+
+
+addGuard(PythonScript, 'Change Python Scripts')
+
+def __call__(self, *args, **kw):
+  '''Calls the script.'''
+  self.checkGuard(True) # patch
+  return self._bindAndExec(args, kw, None)
+
+security.declarePublic("render")
+PythonScript.__call__ = PythonScript.render = __call__
 
 InitializeClass(PythonScript)