Restricted: bug fixes, support generator and collections module

Add some features to restricted python and fix problems revealed by running Zope's test suites.

# Bug fixes

 - Disallow access to old style classes without security declarations. This is not allowed in vanilla zope, but we allowed this accidentally about two years ago. This branch includes some fixes for cases where we accessed not protected classes in a way that should not have been allowed - ERP5 test suite pass, but there might be more cases in code not covered by ERP5 test suite.
 - Fix iterating on `reversed(iterable)` which was unauthorized, maybe since python 2.7
 - Disallow new style classes in container access (iteration, `{}.get` etc). Only classes had this problem, not instances, so this probably has no impact for us, but it allows running AccessControl test suite.
 - Disallow attribute names ending in `__roles__` in class name. This probably does not impact us either, but also for AccessControl tests suite.


# New features

 - Allow iterating on a generator. It's still not possible to use `yield` statement in restricted python, but iterating is now possible
 - Allow `cStringIO.StringIO("initial value")`, only `cStringIO.StringIO()` was allowed
 - Enable `collections.namedtuple` and add a few tests for other members of `collections` ( not `collections.deque` because we never used it so far )

See merge request !1090

bt5/erp5_interface_post/TestTemplateItem/portal_components/test.erp5.testInterfacePost.py

@@ -25,10 +25,10 @@
 #
 ##############################################################################
 import email
+import mock
 import time
 
 from Products.ERP5Type.tests.ERP5TypeLiveTestCase import ERP5TypeTestCase
-from Products.ERP5Type.tests.utils import createZODBPythonScript, removeZODBPythonScript
 from Products.ERP5Type.tests.Sequence import SequenceList
 from Products.ZSQLCatalog.SQLCatalog import SimpleQuery
 from DateTime import DateTime
@@ -82,14 +82,6 @@ class TestInterfacePost(ERP5TypeTestCase):
       module = getattr(self.portal, module_id)
       module.manage_delObjects(list(module.objectIds()))
 
-    custom_skin = self.portal.portal_skins.custom
-    if 'Entity_sendEmail' in custom_skin.objectIds():
-      removeZODBPythonScript(
-        custom_skin,
-        'Entity_sendEmail',
-      )
-      self.commit()
-
   def _portal_catalog(self, **kw):
     result_list = self.portal.portal_catalog(**kw)
     uid_list = [x.uid for x in result_list]
@@ -342,20 +334,19 @@ class TestInterfacePost(ERP5TypeTestCase):
     pdf_document, = pdf_document_list
     self.assertEqual(2, int(pdf_document.getContentInformation()['Pages']))
 
-
   def stepMakeEntitySendEmailFailOnce(self, sequence=None):
-    createZODBPythonScript(
-      self.portal.portal_skins.custom,
-      'Entity_sendEmail',
-      self.portal.Entity_sendEmail.params(),
-      """portal = context.getPortalObject()
-for activity in portal.portal_activities.getMessageList():
-  if activity.method_id == script.id:
-    if activity.retry == 0:
-      raise ValueError('Failure on purpose')
-    else:
-      return context.skinSuper('custom', script.id)(%s)""" % (self.portal.Entity_sendEmail.params(),)
-    )
+    def Entity_sendEmail(*args, **kw):
+      self.Entity_sendEmail_patcher.stop()
+      raise ValueError("Fail on first execution")
+    self.Entity_sendEmail_patcher = mock.patch(
+        'erp5.portal_type.Person.Entity_sendEmail',
+        create=True,
+        side_effect=Entity_sendEmail)
+    self.Entity_sendEmail_mock = self.Entity_sendEmail_patcher.start()
+    self.addCleanup(self.Entity_sendEmail_patcher.stop)
+
+  def stepCheckEntitySendEmailCalled(self, sequence=None):
+    self.Entity_sendEmail_mock.assert_called()
 
   def test_emailSendingIsPilotedByInternetMessagePost(self):
     """
@@ -433,6 +424,7 @@ for activity in portal.portal_activities.getMessageList():
       stepCheckInternetMessagePostCreated
       stepCheckOnlyOneMessageHasBeenSentFromMailHost
       stepCheckLatestMessageListFromMailHost
+      stepCheckEntitySendEmailCalled
     """
     sequence_list.addSequenceString(sequence_string)
     sequence_list.play(self)

product/ERP5/bootstrap/erp5_core/ToolComponentTemplateItem/portal_components/tool.erp5.DiffTool.py

@@ -42,6 +42,7 @@ except ImportError:
   warnings.warn("Please install unidiff, it is needed by Diff Tool",
                 DeprecationWarning)
 from AccessControl import ClassSecurityInfo
+from Acquisition import Explicit
 from Products.ERP5Type.patches.diff import DeepDiff
 from Products.ERP5Type import Permissions
 from Products.ERP5Type.Globals import InitializeClass
@@ -71,7 +72,7 @@ class DiffTool(BaseTool):
       path -- optional path to specify which property to diff
       patch_format -- optional format (rfc6902 or deepdiff)
     """
-    return PortalPatch(old_value, new_value, path, patch_format)
+    return PortalPatch(old_value, new_value, path, patch_format).__of__(self)
 
   security.declarePrivate('patchPortalObject')
   def patchPortalObject(self, old, diff_list):
@@ -89,7 +90,8 @@ class DiffTool(BaseTool):
 
     return new_obj
 
-class PortalPatch:
+
+class PortalPatch(Explicit):
   """
     Provides an abstraction to a patch that
     depends on the patch format.
@@ -283,3 +285,4 @@ class PortalPatch:
     return obj_dict
 
 InitializeClass(DiffTool)
+InitializeClass(PortalPatch)
\ No newline at end of file

product/ERP5Type/Collections.py

+##############################################################################
+#
+# Copyright (c) 2020 Nexedi SA and Contributors. All Rights Reserved.
+#
+# WARNING: This program as such is intended to be used by professional
+# programmers who take the whole responsability of assessing all potential
+# consequences resulting from its eventual inadequacies and bugs
+# End users who are looking for a ready-to-use solution with commercial
+# garantees and support are strongly adviced to contract a Free Software
+# Service Company
+#
+# This program is Free Software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+#
+##############################################################################
+"""
+Restricted collections module.
+
+From restricted python, use "import collections" (see patches/Restricted.py).
+"""
+from collections import (
+    Counter, defaultdict, deque, OrderedDict, namedtuple as _namedtuple)
+
+
+def namedtuple(typename, field_names, verbose=False, rename=False):
+  ret = _namedtuple(typename, field_names, verbose, rename)
+  ret.__allow_access_to_unprotected_subobjects__ = 1
+  return ret

product/ERP5Type/patches/Restricted.py

@@ -11,14 +11,30 @@
 #
 ##############################################################################
 
+import copy
 import sys
+import types
 
 from RestrictedPython.RestrictionMutator import RestrictionMutator
+_MARKER = []
+def checkNameLax(self, node, name=_MARKER):
+  """Verifies that a name being assigned is safe.
 
-# Unsafe attributes on protected objects are already disallowed at execution
-# and we don't want to maintain a duplicated list of exceptions.
-RestrictionMutator.checkName = RestrictionMutator.checkAttrName = \
-    lambda *args, **kw: None
+  In ERP5 we are much more lax that than in Zope's original restricted
+  python and allow to using names starting with _, because we rely on
+  runtime checks to prevent access to forbidden attributes from objects.
+
+  We don't allow defining attributes ending with __roles__ though.
+  """
+  if name is _MARKER:
+    # we use same implementation for checkName and checkAttrName which access
+    # the name in different ways ( see RestrictionMutator 3.6.0 )
+    name = node.attrname
+  if name.endswith('__roles__'):
+    self.error(node, '"%s" is an invalid variable name because '
+                     'it ends with "__roles__".' % name)
+
+RestrictionMutator.checkName = RestrictionMutator.checkAttrName = checkNameLax
 
 
 from Acquisition import aq_acquire
@@ -79,18 +95,22 @@ def allow_class_attribute(klass, access=1):
   assert(inspect.isclass(klass))
   _safe_class_attribute_dict[klass] = access
 
-def _check_type_access(name, v):
+
+class TypeAccessChecker:
+  """Check Access for class instances (whose type() is `type`).
   """
-    Create a method which checks the access if the context type is <type 'type'>s.
+  def __call__(self, name, v):
+    """
+    Create a callable which checks the access if the context type is <type 'type'>s.
     Since the 'type' can be any types of classes, we support the three ways
     defined in AccessControl/SimpleObjectPolicies.  We implement this
     as "a method which returing a method" because we can not know what is the
     type until it is actually called. So the three ways are simulated the
-    returning method inide this method.
+    function returned by this method.
     """
     def factory(inst, name):
       """
-     Check function used with ContainerAssetions checked by cAccessControl.
+      Check function used with ContainerAssertions checked by cAccessControl.
       """
       access = _safe_class_attribute_dict.get(inst, 0)
       # The next 'dict' only checks the access configuration type
@@ -108,7 +128,15 @@ def _check_type_access(name, v):
       return v
     return factory
 
-ContainerAssertions[type] = _check_type_access
+  def __nonzero__(self):
+    # If Containers(type(x)) is true, ZopeGuard checks will short circuit,
+    # thinking it's a simple type, but we don't want this for type, because
+    # type(x) is type for classes, being trueish would skip security check on
+    # classes.
+    return False
+
+ContainerAssertions[type] = TypeAccessChecker()
+
 
 class SafeIterItems(SafeIter):
 
@@ -133,6 +161,9 @@ safe_builtins['sorted'] = guarded_sorted
 def guarded_reversed(seq):
     return SafeIter(reversed(seq))
 safe_builtins['reversed'] = guarded_reversed
+ContainerAssertions[reversed] = 1
+# listreverseiterator is a special type, returned by list.__reversed__
+ContainerAssertions[type(reversed([]))] = 1
 
 def guarded_enumerate(seq, start=0):
     return NullIter(enumerate(guarded_iter(seq), start=start))
@@ -175,12 +206,17 @@ ContainerAssertions[set] = _check_access_wrapper(set, _set_white_dict)
 
 ContainerAssertions[frozenset] = 1
 
+ContainerAssertions[types.GeneratorType] = 1
+
 from collections import OrderedDict
 ModuleSecurityInfo('collections').declarePublic('OrderedDict')
 
 from collections import defaultdict
 ModuleSecurityInfo('collections').declarePublic('defaultdict')
 
+from collections import Counter
+ModuleSecurityInfo('collections').declarePublic('Counter')
+
 from AccessControl.ZopeGuards import _dict_white_list
 
 # Attributes cannot be set on defaultdict, thus modify 'safetype' dict
@@ -195,6 +231,14 @@ ContainerAssertions[OrderedDict] = _check_access_wrapper(OrderedDict, _dict_whit
 OrderedDict.__guarded_setitem__ = OrderedDict.__setitem__.__func__
 OrderedDict.__guarded_delitem__ = OrderedDict.__delitem__.__func__
 
+_counter_white_list = copy.copy(_dict_white_list)
+_counter_white_list['most_common'] = 1
+ContainerAssertions[Counter] = _check_access_wrapper(Counter, _counter_white_list)
+Counter.__guarded_setitem__ = dict.__setitem__
+Counter.__guarded_delitem__ = dict.__delitem__
+
+ModuleSecurityInfo('collections').declarePublic('namedtuple')
+
 # given as example in Products.PythonScripts.module_access_examples
 allow_module('base64')
 allow_module('binascii')
@@ -214,13 +258,14 @@ allow_type(type(re.compile('')))
 allow_type(type(re.match('x','x')))
 allow_type(type(re.finditer('x','x')))
 
-import cStringIO, StringIO
-f_cStringIO = cStringIO.StringIO()
-f_StringIO = StringIO.StringIO()
-allow_module('cStringIO')
 allow_module('StringIO')
-allow_type(type(f_cStringIO))
-allow_type(type(f_StringIO))
+import StringIO
+StringIO.StringIO.__allow_access_to_unprotected_subobjects__ = 1
+allow_module('cStringIO')
+import cStringIO
+allow_type(cStringIO.InputType)
+allow_type(cStringIO.OutputType)
+
 
 ModuleSecurityInfo('cgi').declarePublic('escape', 'parse_header')
 allow_module('datetime')
@@ -286,6 +331,7 @@ ModuleSecurityInfo('email.mime.text').declarePublic('MIMEText')
 MNAME_MAP = {
   'zipfile': 'Products.ERP5Type.ZipFile',
   'calendar': 'Products.ERP5Type.Calendar',
+  'collections': 'Products.ERP5Type.Collections',
 }
 for alias, real in MNAME_MAP.items():
   assert '.' not in alias, alias # TODO: support this