ERP5Type: honor read_property in Base.getProperty

This introduce a `_getProperty` method that's not really part of public
API, but could be called to get a property without security check.
Because some classes were already using internally a _getProperty method
for a different meaning, these methods have been renamed.

This also re-enable parts of test_PropertySheetSecurityOnAccessors that
are supposed to work (security is checked on accessors) and split
what's currently not supposed to work (security on accessors does not
work in the corner case of using the default read permission, 'Access
contents information' as a write permission) to another expected failure
test.

product/ERP5/Document/ParentDeliveryPropertyMovementGroup.py

@@ -45,7 +45,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup):
     parent_delivery = self._getParentDelivery(movement)
     if parent_delivery is not None:
       for prop in self.getTestedPropertyList():
-        property_dict['_%s' % prop] = self._getProperty(parent_delivery, prop)
+        property_dict['_%s' % prop] = self._getPropertyFromDocument(parent_delivery, prop)
     return property_dict
 
   def test(self, document, property_dict, property_list=None, **kw):
@@ -64,7 +64,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup):
         return False, {}
       document = self._getParentDelivery(movement)
     for prop in target_property_list:
-      if property_dict['_%s' % prop] != self._getProperty(document, prop):
+      if property_dict['_%s' % prop] != self._getPropertyFromDocument(document, prop):
         return False, {}
     return True, {}
 
@@ -78,7 +78,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup):
       delivery = movement.getDeliveryValue()
     return delivery
 
-  def _getProperty(self, document, property_id):
+  def _getPropertyFromDocument(self, document, property_id):
     if document is None:
       return None
     # XXX here we don't use Base.getProperty() but try to call accessor

product/ERP5Type/Base.py

@@ -1216,11 +1216,21 @@ class Base( CopyContainer,
     If an accessor exists for this property, the accessor will be called,
     default value will be passed to the accessor as first positional argument.
     """
+    kw['restricted'] = True
+    return self._getProperty(key, d=d, **kw)
+
+  def _getProperty(self, key, d=_MARKER, restricted=False, **kw):
     __traceback_info__ = (key,)
     accessor_name = 'get' + UpperCase(key)
+    _getattr = guarded_getattr if restricted else getattr
     aq_self = aq_base(self)
     if getattr(aq_self, accessor_name, None) is not None:
-      method = getattr(self, accessor_name)
+      try:
+        method = _getattr(self, accessor_name)
+      except Unauthorized:
+        if not kw.get('checked_permission'):
+          raise
+        return None if d is _MARKER else d
       if d is not _MARKER:
         try:
           # here method is a method defined on the class, we don't know if the
@@ -1234,16 +1244,21 @@ class Base( CopyContainer,
     # and return it as a list
     if accessor_name.endswith('List'):
       mono_valued_accessor_name = accessor_name[:-4]
-      method = getattr(self.__class__, mono_valued_accessor_name, None)
-      if method is not None:
+      if hasattr(self.__class__, mono_valued_accessor_name):
+        try:
+          method = _getattr(self, mono_valued_accessor_name)
+        except Unauthorized:
+          if not kw.get('checked_permission'):
+            raise
+          return [] if d is _MARKER else d
         # We have a monovalued property
         if d is _MARKER:
-          result = method(self, **kw)
+          result = method(**kw)
         else:
           try:
-            result = method(self, d, **kw)
+            result = method(d, **kw)
           except TypeError:
-            result = method(self, **kw)
+            result = method(**kw)
         if not isinstance(result, (list, tuple)):
           result = [result]
         return result

product/ERP5Type/tests/testERP5Type.py

@@ -2676,8 +2676,6 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
       self.assertFalse(guarded_hasattr(obj, 'getRegionValueList'))
       self.assertFalse(guarded_hasattr(obj, 'getRegionRelatedValueList'))
 
-    # Permission definition on Accessor is buggy. TO BE FIXED!
-    @expectedFailure
     def test_PropertySheetSecurityOnAccessors(self):
       # Test accessors are protected correctly when you specify the permission
       # in the property sheet.
@@ -2688,7 +2686,7 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
           write_permission='Set own password',
           read_permission='Manage users',
           portal_type='Standard Property')
-      obj = self.getPersonModule().newContent(portal_type='Person')
+      obj = self.getPersonModule().newContent(portal_type='Person', foo_bar='value')
       self.assertTrue(guarded_hasattr(obj, 'setFooBar'))
       self.assertTrue(guarded_hasattr(obj, 'getFooBar'))
 
@@ -2700,9 +2698,35 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
       obj.manage_permission('Manage users', [], 0)
       self.assertTrue(guarded_hasattr(obj, 'setFooBar'))
       self.assertFalse(guarded_hasattr(obj, 'getFooBar'))
+      # getProperty also raises
+      self.assertRaises(Unauthorized, obj.getProperty, 'foo_bar')
+      # ... unless called with checked_permission=
+      self.assertEqual(None,
+        obj.getProperty('foo_bar', checked_permission='Access content information'))
+
+      # When a document has protected properties, PropertyManager API does not
+      # "leak" protected properties when user cannot access.
+      self.assertRaises(Unauthorized, obj.propertyItems)
+      self.assertRaises(Unauthorized, obj.propertyValues)
+      # Other ERP5 APIs do not allow accessing private properties either.
+      self.assertRaises(Unauthorized, obj.asXML)
 
-      # Make sure that we can use 'Access contents information' as
-      # write permission and 'Modify portal content' as read permission.
+    @expectedFailure
+    def test_PropertySheetSecurityOnAccessors_nonstandard_permissions(self):
+      """Make sure that we can use 'Access contents information' as
+      write permission and 'Modify portal content' as read permission.
+
+      note about the expectedFailure:
+      `Access contents information` and `Modify portal content` are
+      special cased and currently cannot be applied for other cases as
+      getter use access and setter use modify.
+
+      This is done in AccessorHolderType._skip_permission_set from
+      product/ERP5Type/dynamic/accessor_holder.py . Maybe we could be more
+      specific and define the skip permission on the accessor class, so that
+      we can define separatly the skipped permission for setter and getter.
+      For now I (Jerome) feel it's not important.
+      """
       self._addProperty('Person',
           'test_PropertySheetSecurityOnAccessors',
           'hoge_hoge',
@@ -2725,7 +2749,7 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
 
       # Make sure that getProperty and setProperty respect accessor's
       # security protection.
-      createZODBPythonScript(portal.portal_skins.custom,
+      createZODBPythonScript(self.portal.portal_skins.custom,
                              'Base_callAccessorHogeHoge',
                              'mode',
                              '''\
@@ -2736,7 +2760,7 @@ elif mode == 'getProperty':
 elif mode == 'setter':
   context.setHogeHoge('waa')
 elif mode == 'setProperty':
-  context.setProperty('waa')
+  context.setProperty('hoge_hoge', 'waa')
 return True''')
       # test accessors
       obj.manage_permission('Access contents information', ['Manager'], 1)