Commit 0f4d6bf4 authored by Jérome Perrin's avatar Jérome Perrin

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.
parent f6b4ea3c
...@@ -45,7 +45,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup): ...@@ -45,7 +45,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup):
parent_delivery = self._getParentDelivery(movement) parent_delivery = self._getParentDelivery(movement)
if parent_delivery is not None: if parent_delivery is not None:
for prop in self.getTestedPropertyList(): 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 return property_dict
def test(self, document, property_dict, property_list=None, **kw): def test(self, document, property_dict, property_list=None, **kw):
...@@ -64,7 +64,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup): ...@@ -64,7 +64,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup):
return False, {} return False, {}
document = self._getParentDelivery(movement) document = self._getParentDelivery(movement)
for prop in target_property_list: 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 False, {}
return True, {} return True, {}
...@@ -78,7 +78,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup): ...@@ -78,7 +78,7 @@ class ParentDeliveryPropertyMovementGroup(PropertyMovementGroup):
delivery = movement.getDeliveryValue() delivery = movement.getDeliveryValue()
return delivery return delivery
def _getProperty(self, document, property_id): def _getPropertyFromDocument(self, document, property_id):
if document is None: if document is None:
return None return None
# XXX here we don't use Base.getProperty() but try to call accessor # XXX here we don't use Base.getProperty() but try to call accessor
......
...@@ -1216,11 +1216,21 @@ class Base( CopyContainer, ...@@ -1216,11 +1216,21 @@ class Base( CopyContainer,
If an accessor exists for this property, the accessor will be called, If an accessor exists for this property, the accessor will be called,
default value will be passed to the accessor as first positional argument. 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,) __traceback_info__ = (key,)
accessor_name = 'get' + UpperCase(key) accessor_name = 'get' + UpperCase(key)
_getattr = guarded_getattr if restricted else getattr
aq_self = aq_base(self) aq_self = aq_base(self)
if getattr(aq_self, accessor_name, None) is not None: 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: if d is not _MARKER:
try: try:
# here method is a method defined on the class, we don't know if the # here method is a method defined on the class, we don't know if the
...@@ -1234,16 +1244,21 @@ class Base( CopyContainer, ...@@ -1234,16 +1244,21 @@ class Base( CopyContainer,
# and return it as a list # and return it as a list
if accessor_name.endswith('List'): if accessor_name.endswith('List'):
mono_valued_accessor_name = accessor_name[:-4] mono_valued_accessor_name = accessor_name[:-4]
method = getattr(self.__class__, mono_valued_accessor_name, None) if getattr(self.__class__, mono_valued_accessor_name, None) is not None:
if method is not None: 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 # We have a monovalued property
if d is _MARKER: if d is _MARKER:
result = method(self, **kw) result = method(**kw)
else: else:
try: try:
result = method(self, d, **kw) result = method(d, **kw)
except TypeError: except TypeError:
result = method(self, **kw) result = method(**kw)
if not isinstance(result, (list, tuple)): if not isinstance(result, (list, tuple)):
result = [result] result = [result]
return result return result
......
...@@ -2676,8 +2676,6 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor): ...@@ -2676,8 +2676,6 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
self.assertFalse(guarded_hasattr(obj, 'getRegionValueList')) self.assertFalse(guarded_hasattr(obj, 'getRegionValueList'))
self.assertFalse(guarded_hasattr(obj, 'getRegionRelatedValueList')) self.assertFalse(guarded_hasattr(obj, 'getRegionRelatedValueList'))
# Permission definition on Accessor is buggy. TO BE FIXED!
@expectedFailure
def test_PropertySheetSecurityOnAccessors(self): def test_PropertySheetSecurityOnAccessors(self):
# Test accessors are protected correctly when you specify the permission # Test accessors are protected correctly when you specify the permission
# in the property sheet. # in the property sheet.
...@@ -2688,7 +2686,7 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor): ...@@ -2688,7 +2686,7 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
write_permission='Set own password', write_permission='Set own password',
read_permission='Manage users', read_permission='Manage users',
portal_type='Standard Property') 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, 'setFooBar'))
self.assertTrue(guarded_hasattr(obj, 'getFooBar')) self.assertTrue(guarded_hasattr(obj, 'getFooBar'))
...@@ -2700,9 +2698,28 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor): ...@@ -2700,9 +2698,28 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
obj.manage_permission('Manage users', [], 0) obj.manage_permission('Manage users', [], 0)
self.assertTrue(guarded_hasattr(obj, 'setFooBar')) self.assertTrue(guarded_hasattr(obj, 'setFooBar'))
self.assertFalse(guarded_hasattr(obj, 'getFooBar')) 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'))
# Make sure that we can use 'Access contents information' as @expectedFailure
# write permission and 'Modify portal content' as read permission. 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', self._addProperty('Person',
'test_PropertySheetSecurityOnAccessors', 'test_PropertySheetSecurityOnAccessors',
'hoge_hoge', 'hoge_hoge',
...@@ -2725,7 +2742,7 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor): ...@@ -2725,7 +2742,7 @@ class TestERP5Type(PropertySheetTestCase, LogInterceptor):
# Make sure that getProperty and setProperty respect accessor's # Make sure that getProperty and setProperty respect accessor's
# security protection. # security protection.
createZODBPythonScript(portal.portal_skins.custom, createZODBPythonScript(self.portal.portal_skins.custom,
'Base_callAccessorHogeHoge', 'Base_callAccessorHogeHoge',
'mode', 'mode',
'''\ '''\
...@@ -2736,7 +2753,7 @@ elif mode == 'getProperty': ...@@ -2736,7 +2753,7 @@ elif mode == 'getProperty':
elif mode == 'setter': elif mode == 'setter':
context.setHogeHoge('waa') context.setHogeHoge('waa')
elif mode == 'setProperty': elif mode == 'setProperty':
context.setProperty('waa') context.setProperty('hoge_hoge', 'waa')
return True''') return True''')
# test accessors # test accessors
obj.manage_permission('Access contents information', ['Manager'], 1) obj.manage_permission('Access contents information', ['Manager'], 1)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment