From 74f2a76d8b0f9f6a3c8c64b8872d188e2bc54e90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Fri, 14 Oct 2016 02:33:08 +0000 Subject: [PATCH 01/10] testGUIwithSecurity: cleanups and more strict test maybe a little to strict. I don't know how we can check that fields does not display value without checking the markup in a so low level way --- product/ERP5Form/tests/testGUIwithSecurity.py | 114 ++++++++---------- 1 file changed, 51 insertions(+), 63 deletions(-) diff --git a/product/ERP5Form/tests/testGUIwithSecurity.py b/product/ERP5Form/tests/testGUIwithSecurity.py index 91ec75b1ad0..f605419d4d7 100644 --- a/product/ERP5Form/tests/testGUIwithSecurity.py +++ b/product/ERP5Form/tests/testGUIwithSecurity.py @@ -28,21 +28,14 @@ ############################################################################## -import unittest - from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase from AccessControl.SecurityManagement import newSecurityManager -from zLOG import LOG from Products.ERP5Type.tests.Sequence import SequenceList -from Testing import ZopeTestCase -from DateTime import DateTime class TestGUISecurity(ERP5TypeTestCase): """ """ - quiet = 0 - run_all_test = 1 def getBusinessTemplateList(self): return ('erp5_ui_test', 'erp5_base') @@ -50,15 +43,6 @@ class TestGUISecurity(ERP5TypeTestCase): def getTitle(self): return "Security Issues in GUI" - def afterSetUp(self): - self.login() - - def login(self): - uf = self.getPortal().acl_users - uf._doAddUser('seb', '', ['Manager'], []) - user = uf.getUserById('seb').__of__(uf) - newSecurityManager(None, user) - def loginAs(self, id='user'): uf = self.getPortal().acl_users user = uf.getUser(id).__of__(uf) @@ -66,35 +50,51 @@ class TestGUISecurity(ERP5TypeTestCase): def stepCreateObjects(self, sequence = None, sequence_list = None, **kw): # Make sure that the status is clean. - portal = self.getPortal() - portal.ListBoxZuite_reset() - message = portal.foo_module.FooModule_createObjects() + self.portal.ListBoxZuite_reset() + message = self.portal.foo_module.FooModule_createObjects() self.assertTrue('Created Successfully' in message) - if not hasattr(portal.person_module, 'user'): - user = portal.person_module.newContent(portal_type='Person', id='user', reference='user') + if not hasattr(self.portal.person_module, 'user'): + user = self.portal.person_module.newContent(portal_type='Person', id='user', reference='user') user.newContent(portal_type='ERP5 Login', reference='user').validate() - asg = user.newContent(portal_type='Assignment') - asg.setStartDate(DateTime() - 100) - asg.setStopDate(DateTime() + 100) - asg.open() - self.commit() + user.newContent(portal_type='Assignment').open() def stepCreateTestFoo(self, sequence = None, sequence_list = None, **kw): - foo_module = self.getPortal().foo_module + foo_module = self.portal.foo_module foo_module.newContent(portal_type='Foo', id='foo', foo_category='a') # allow Member to view foo_module in a hard coded way as it is not required to setup complex # security for this test (by default only 5A roles + Manager can view default modules) - args = (('Manager', 'Member', 'Assignor', 'Assignee', 'Auditor', 'Associate' ), 0) - foo_module.manage_permission('Access contents information', *args) - foo_module.manage_permission('View', *args) - self.commit() + for permission in ('Access contents information', 'View'): + foo_module.manage_permission( + permission, + ('Manager', 'Member', 'Assignor', 'Assignee', 'Auditor', 'Associate' ), + 0) - def stepAccessFoo(self, sequence = None, sequence_list = None, **kw): + def stepAccessFooDoesNotRaise(self, sequence = None, sequence_list = None, **kw): """ Try to view the Foo_view form, make sure Unauthorized is not raised. """ self.loginAs() - self.getPortal().foo_module.foo.Foo_view() + self.portal.foo_module.foo.Foo_view() + self.login() + + def stepAccessFooDisplaysCategoryName(self, sequence = None, sequence_list = None, **kw): + """ + Try to view the Foo_view form, make sure our category name is displayed + """ + self.loginAs() + self.assertIn( + self.category_field_markup, + self.portal.foo_module.foo.Foo_view()) + self.login() + + def stepAccessFooDoesNotDisplayCategoryName(self, sequence = None, sequence_list = None, **kw): + """ + Try to view the Foo_view form, make sure our category name is not displayed + """ + self.loginAs() + self.assertNotIn( + self.category_field_markup, + self.portal.foo_module.foo.Foo_view()) self.login() def stepChangeCategorySecurity(self, sequence = None, sequence_list = None, **kw): @@ -102,30 +102,23 @@ class TestGUISecurity(ERP5TypeTestCase): here we change security of a category to which the "Foo" is related and which is displayed in the Foo's RelationStringField """ - category = self.getPortal().portal_categories.foo_category.a - args = (('Manager',), 0) - category.manage_permission('Access contents information', *args) - category.manage_permission('View', *args) - self.tic() + category = self.portal.portal_categories.foo_category.a + for permission in ('Access contents information', 'View'): + category.manage_permission(permission, ('Manager',), 0 ) def stepResetCategorySecurity(self, sequence = None, sequence_list = None, **kw): """ reset it back """ - category = self.getPortal().portal_categories.foo_category.a - args = ((), 1) - category.manage_permission('Access contents information', *args) - category.manage_permission('View', *args) - self.tic() + category = self.portal.portal_categories.foo_category.a + for permission in ('Access contents information', 'View'): + category.manage_permission(permission, ('Manager',), 1) - def test_01_relationFieldToInaccessibleObject(self, quiet=quiet, run=run_all_test): + def test_01_relationFieldToInaccessibleObject(self): """ This test checks if a form can be viewed when it contains a RelationStringField which links to an object the user is not authorized to view. - This fails for now. A proposed patch solving this problem is here: - http://svn.erp5.org/experimental/FSPatch/Products/ERP5Form/ERP5Form_safeRelationField.diff?view=markup - This problem can happen for example in the following situation: - a user is a member of a project P team, so he can view P - the user creates a project-related document and leaves it in "draft" state @@ -133,29 +126,24 @@ class TestGUISecurity(ERP5TypeTestCase): Then the user can not view the project, but still can view his document as he is the owner. An attempt to view the document form would raise Unauthorized. """ - self.login() - if not run: return - if not quiet: - message = 'test_01_relationFieldToInaccessibleObject' - ZopeTestCase._print('\n%s ' % message) - LOG('Testing... ', 0, message) + # this really depends on the generated markup + self.category_field_markup = ' Date: Fri, 14 Oct 2016 03:36:10 +0000 Subject: [PATCH 02/10] 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. --- .../ParentDeliveryPropertyMovementGroup.py | 6 +-- product/ERP5Type/Base.py | 27 ++++++++++--- product/ERP5Type/tests/testERP5Type.py | 38 +++++++++++++++---- 3 files changed, 55 insertions(+), 16 deletions(-) diff --git a/product/ERP5/Document/ParentDeliveryPropertyMovementGroup.py b/product/ERP5/Document/ParentDeliveryPropertyMovementGroup.py index 65cb6e6473b..fbf5bf6385e 100644 --- a/product/ERP5/Document/ParentDeliveryPropertyMovementGroup.py +++ b/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 diff --git a/product/ERP5Type/Base.py b/product/ERP5Type/Base.py index 8fd7c131522..8727da788e1 100644 --- a/product/ERP5Type/Base.py +++ b/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 diff --git a/product/ERP5Type/tests/testERP5Type.py b/product/ERP5Type/tests/testERP5Type.py index d478ec9f3e6..3f13436103a 100644 --- a/product/ERP5Type/tests/testERP5Type.py +++ b/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) -- 2.30.9 From e6d8de91862c1f7905539b9faa3454d8d4f64673 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Fri, 14 Oct 2016 05:14:35 +0000 Subject: [PATCH 03/10] ERP5Type: make PropertyManager.getProperty support `checked_permission` argument for compatibility with Base.checkProperty. --- product/ERP5Type/patches/PropertyManager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/product/ERP5Type/patches/PropertyManager.py b/product/ERP5Type/patches/PropertyManager.py index 7ebc277480c..0d08e6e088a 100644 --- a/product/ERP5Type/patches/PropertyManager.py +++ b/product/ERP5Type/patches/PropertyManager.py @@ -57,7 +57,7 @@ def PropertyManager_hasProperty(self, id, local_properties=False): return 0 def PropertyManager_getProperty(self, id, d=None, evaluate=1, - local_properties=False): + local_properties=False, checked_permission=None): """Get the property 'id', returning the optional second argument or None if no such property is found.""" property_type = self.getPropertyType(id, -- 2.30.9 From 8f26efd8fb3499f0076fde7c0515d6a64e4264a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Mon, 8 Jul 2019 10:32:45 +0200 Subject: [PATCH 04/10] ERP5Type: use constraints accessors in _filteredConstraintList Constraints are supposed to have a getReference and getConstraintType methods. --- product/ERP5Type/Base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/product/ERP5Type/Base.py b/product/ERP5Type/Base.py index 8727da788e1..32ca46e495c 100644 --- a/product/ERP5Type/Base.py +++ b/product/ERP5Type/Base.py @@ -2669,13 +2669,13 @@ class Base( CopyContainer, reference_list = filt.get('reference', None) if not isinstance(reference_list, (list, tuple)): reference_list = [reference_list] - constraints = filter(lambda x:x.getProperty('reference') in \ + constraints = filter(lambda x:x.getReference() in \ reference_list, constraints) if 'constraint_type' in filt: constraint_type_list = filt.get('constraint_type', None) if not isinstance(constraint_type_list, (list, tuple)): constraint_type_list = [constraint_type_list] - constraints = filter(lambda x:x.__of__(self).getProperty('constraint_type') in \ + constraints = filter(lambda x:x.__of__(self).getConstraintType() in \ constraint_type_list, constraints) return constraints -- 2.30.9 From eefaa8b36ebf58f8cb442457b63d3f77c6a476ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Mon, 8 Jul 2019 17:53:05 +0900 Subject: [PATCH 05/10] ui_test: Make protected properties testable - introduce protected property (ie. with a specific "read permission" / "write permission" ) on Foo - add a Foo_viewSecurity which displays this protected property --- .../property_sheet_list.xml | 1 + .../portal_property_sheets/Foo.xml | 66 +++ .../Foo/protected_property_property.xml | 40 ++ .../erp5_ui_test/Foo_viewSecurity.xml | 153 +++++ .../my_foo_category_title.xml | 540 ++++++++++++++++++ .../my_protected_property.xml | 264 +++++++++ .../template_portal_type_property_sheet_list | 3 +- .../bt/template_property_sheet_id_list | 1 + 8 files changed, 1067 insertions(+), 1 deletion(-) create mode 100644 bt5/erp5_ui_test/PropertySheetTemplateItem/portal_property_sheets/Foo.xml create mode 100644 bt5/erp5_ui_test/PropertySheetTemplateItem/portal_property_sheets/Foo/protected_property_property.xml create mode 100644 bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity.xml create mode 100644 bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity/my_foo_category_title.xml create mode 100644 bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity/my_protected_property.xml create mode 100644 bt5/erp5_ui_test/bt/template_property_sheet_id_list diff --git a/bt5/erp5_ui_test/PortalTypePropertySheetTemplateItem/property_sheet_list.xml b/bt5/erp5_ui_test/PortalTypePropertySheetTemplateItem/property_sheet_list.xml index 2d573edc0e4..4ecce9d4931 100644 --- a/bt5/erp5_ui_test/PortalTypePropertySheetTemplateItem/property_sheet_list.xml +++ b/bt5/erp5_ui_test/PortalTypePropertySheetTemplateItem/property_sheet_list.xml @@ -5,5 +5,6 @@ Amount DublinCore + Foo \ No newline at end of file diff --git a/bt5/erp5_ui_test/PropertySheetTemplateItem/portal_property_sheets/Foo.xml b/bt5/erp5_ui_test/PropertySheetTemplateItem/portal_property_sheets/Foo.xml new file mode 100644 index 00000000000..f3d2c9c7be2 --- /dev/null +++ b/bt5/erp5_ui_test/PropertySheetTemplateItem/portal_property_sheets/Foo.xml @@ -0,0 +1,66 @@ + + + + + + + + + + _count + + AAAAAAAAAAI= + + + + _mt_index + + AAAAAAAAAAM= + + + + _tree + + AAAAAAAAAAQ= + + + + description + + + + + + id + Foo + + + portal_type + Property Sheet + + + + + + + + + 0 + + + + + + + + + + + + + + + + + + diff --git a/bt5/erp5_ui_test/PropertySheetTemplateItem/portal_property_sheets/Foo/protected_property_property.xml b/bt5/erp5_ui_test/PropertySheetTemplateItem/portal_property_sheets/Foo/protected_property_property.xml new file mode 100644 index 00000000000..6aa6a962cf6 --- /dev/null +++ b/bt5/erp5_ui_test/PropertySheetTemplateItem/portal_property_sheets/Foo/protected_property_property.xml @@ -0,0 +1,40 @@ + + + + + + + + + + categories + + + elementary_type/string + + + + + description + A protected property for UI test. This is a property that only managers should be able to see and modify + + + id + protected_property_property + + + portal_type + Standard Property + + + read_permission + Manage portal + + + write_permission + Manage portal + + + + + diff --git a/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity.xml b/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity.xml new file mode 100644 index 00000000000..11de402b269 --- /dev/null +++ b/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity.xml @@ -0,0 +1,153 @@ + + + + + + + + + + _bind_names + + + + + + + + + + _asgns + + + + + + + + + + + _objects + + + + + + action + Base_edit + + + description + View with some restricted properties + + + edit_order + + + + + + encoding + UTF-8 + + + enctype + + + + group_list + + + left + right + center + bottom + hidden + + + + + groups + + + + bottom + + + + + + center + + + + + + hidden + + + + + + left + + + my_protected_property + my_foo_category_title + + + + + right + + + + + + + + + id + Foo_viewSecurity + + + method + POST + + + name + Foo_view + + + pt + form_view + + + row_length + 4 + + + stored_encoding + UTF-8 + + + title + Foo Security + + + unicode_mode + 0 + + + update_action + + + + update_action_title + + + + + + diff --git a/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity/my_foo_category_title.xml b/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity/my_foo_category_title.xml new file mode 100644 index 00000000000..b9cb5ce0bc0 --- /dev/null +++ b/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity/my_foo_category_title.xml @@ -0,0 +1,540 @@ + + + + + + + + + + id + my_foo_category_title + + + message_values + + + + external_validator_failed + The input failed the external validator. + + + line_too_long + A line was too long. + + + relation_result_ambiguous + Select appropriate document in the list. + + + relation_result_empty + No such document was found. + + + relation_result_too_long + Too many documents were found. + + + required_not_found + Input is required but no input given. + + + too_long + You entered too many characters. + + + too_many_lines + You entered too many lines. + + + + + + overrides + + + + allow_creation + + + + allow_jump + + + + alternate_name + + + + base_category + + + + catalog_index + + + + columns + + + + container_getter_id + + + + css_class + + + + default + + + + description + + + + display_maxwidth + + + + display_width + + + + editable + + + + enabled + + + + external_validator + + + + extra + + + + extra_item + + + + first_item + + + + hidden + + + + items + + + + jump_method + + + + list_method + + + + max_length + + + + max_linelength + + + + max_lines + + + + parameter_list + + + + portal_type + + + + relation_setter_id + + + + required + + + + size + + + + sort + + + + title + + + + truncate + + + + unicode + + + + update_method + + + + whitespace_preserve + + + + + + + tales + + + + allow_creation + + + + allow_jump + + + + alternate_name + + + + base_category + + + + catalog_index + + + + columns + + + + container_getter_id + + + + css_class + + + + default + + + + description + + + + display_maxwidth + + + + display_width + + + + editable + + + + enabled + + + + external_validator + + + + extra + + + + extra_item + + + + first_item + + + + hidden + + + + items + + + + jump_method + + + + list_method + + + + max_length + + + + max_linelength + + + + max_lines + + + + parameter_list + + + + portal_type + + + + relation_setter_id + + + + required + + + + size + + + + sort + + + + title + + + + truncate + + + + unicode + + + + update_method + + + + whitespace_preserve + + + + + + + values + + + + allow_creation + 0 + + + allow_jump + 1 + + + alternate_name + + + + base_category + foo_category + + + catalog_index + title + + + columns + + + + title + Title + + + int_index + Sort Index + + + relative_url + Relative URL + + + + + + container_getter_id + + + + css_class + + + + default + + + + description + + + + display_maxwidth + + + + display_width + 20 + + + editable + 1 + + + enabled + 1 + + + external_validator + + + + extra + + + + extra_item + + + + first_item + 0 + + + hidden + 0 + + + items + + + + + + jump_method + Base_jumpToRelatedDocument + + + list_method + + + + max_length + + + + max_linelength + + + + max_lines + + + + parameter_list + + + + + + portal_type + + + + Category + Category + + + + + + relation_form_id + Base_viewCustomRelatedObjectList + + + relation_setter_id + + + + required + 0 + + + size + 1 + + + sort + + + + + + title + Foo Category + + + truncate + 0 + + + unicode + 0 + + + update_method + Base_validateRelation + + + whitespace_preserve + 1 + + + + + + + + diff --git a/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity/my_protected_property.xml b/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity/my_protected_property.xml new file mode 100644 index 00000000000..7f03fa8e72c --- /dev/null +++ b/bt5/erp5_ui_test/SkinTemplateItem/portal_skins/erp5_ui_test/Foo_viewSecurity/my_protected_property.xml @@ -0,0 +1,264 @@ + + + + + + + + + + id + my_protected_property + + + message_values + + + + external_validator_failed + The input failed the external validator. + + + required_not_found + Input is required but no input given. + + + too_long + Too much input was given. + + + + + + overrides + + + + alternate_name + + + + css_class + + + + default + + + + description + + + + display_maxwidth + + + + display_width + + + + editable + + + + enabled + + + + external_validator + + + + extra + + + + hidden + + + + max_length + + + + required + + + + title + + + + truncate + + + + unicode + + + + whitespace_preserve + + + + + + + tales + + + + alternate_name + + + + css_class + + + + default + + + + description + + + + display_maxwidth + + + + display_width + + + + editable + + + + enabled + + + + external_validator + + + + extra + + + + hidden + + + + max_length + + + + required + + + + title + + + + truncate + + + + unicode + + + + whitespace_preserve + + + + + + + values + + + + alternate_name + + + + css_class + + + + default + + + + description + + + + display_maxwidth + + + + display_width + 20 + + + editable + 1 + + + enabled + 1 + + + external_validator + + + + extra + + + + hidden + 0 + + + input_type + text + + + max_length + + + + required + 0 + + + title + Protected Property + + + truncate + 0 + + + unicode + 0 + + + whitespace_preserve + 0 + + + + + + + + diff --git a/bt5/erp5_ui_test/bt/template_portal_type_property_sheet_list b/bt5/erp5_ui_test/bt/template_portal_type_property_sheet_list index 18e04ea6dea..181a41749cb 100644 --- a/bt5/erp5_ui_test/bt/template_portal_type_property_sheet_list +++ b/bt5/erp5_ui_test/bt/template_portal_type_property_sheet_list @@ -1,3 +1,4 @@ Bar | Reference Foo | Amount -Foo | DublinCore \ No newline at end of file +Foo | DublinCore +Foo | Foo \ No newline at end of file diff --git a/bt5/erp5_ui_test/bt/template_property_sheet_id_list b/bt5/erp5_ui_test/bt/template_property_sheet_id_list new file mode 100644 index 00000000000..9f26b637f01 --- /dev/null +++ b/bt5/erp5_ui_test/bt/template_property_sheet_id_list @@ -0,0 +1 @@ +Foo \ No newline at end of file -- 2.30.9 From 18edf53372f8b6f754464b5dd8b43b8e82ad18ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Tue, 9 Jul 2019 05:56:28 +0200 Subject: [PATCH 06/10] testGUIwithSecurity: test display of restricted properties --- product/ERP5Form/tests/testGUIwithSecurity.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/product/ERP5Form/tests/testGUIwithSecurity.py b/product/ERP5Form/tests/testGUIwithSecurity.py index f605419d4d7..0389824647e 100644 --- a/product/ERP5Form/tests/testGUIwithSecurity.py +++ b/product/ERP5Form/tests/testGUIwithSecurity.py @@ -60,7 +60,7 @@ class TestGUISecurity(ERP5TypeTestCase): def stepCreateTestFoo(self, sequence = None, sequence_list = None, **kw): foo_module = self.portal.foo_module - foo_module.newContent(portal_type='Foo', id='foo', foo_category='a') + foo_module.newContent(portal_type='Foo', id='foo', foo_category='a', protected_property='Protected Property') # allow Member to view foo_module in a hard coded way as it is not required to setup complex # security for this test (by default only 5A roles + Manager can view default modules) for permission in ('Access contents information', 'View'): @@ -147,3 +147,18 @@ class TestGUISecurity(ERP5TypeTestCase): sequence_list.addSequenceString(sequence_string) sequence_list.play(self) + def test_read_permission_property(self): + """ + This test checks that property defined with a `read_property` that the + logged in user does not have are not displayed. + """ + self.login() # as manager + self.stepCreateObjects() + self.stepCreateTestFoo() + + protected_property_markup = ' Date: Mon, 2 Sep 2019 03:51:21 +0200 Subject: [PATCH 07/10] testPerformance: support choosing between pprofile and cProfile --- product/ERP5Type/tests/testPerformance.py | 35 +++++++++++++++++------ 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/product/ERP5Type/tests/testPerformance.py b/product/ERP5Type/tests/testPerformance.py index d729b09ae98..7197811fbbb 100644 --- a/product/ERP5Type/tests/testPerformance.py +++ b/product/ERP5Type/tests/testPerformance.py @@ -110,8 +110,13 @@ LISTBOX_COEF=0.00173 # 0.02472 # LISTBOX_COEF : 0.02472 -> 0.001725 DO_TEST = 1 +# Profiler support. # set 1 to get profiler's result (unit_test/tests/) -PROFILE=0 +PROFILE = 0 +# set this to 'pprofile' to profile with pprofile ( https://github.com/vpelletier/pprofile ) +# instad of python's standard library profiler ( https://docs.python.org/2/library/profile.html ) +PROFILER = 'pprofile' + class TestPerformanceMixin(ERP5TypeTestCase, LogInterceptor): @@ -147,22 +152,34 @@ class TestPerformanceMixin(ERP5TypeTestCase, LogInterceptor): """ return self.portal['bar_module'] - def profile(self, func, suffix=''): + def profile(self, func, suffix='', args=(), kw=None): + """Profile `func(*args, **kw)` with selected profiler, + and dump output in a file called `func.__name__ + suffix` + """ + if not kw: + kw = {} + + if PROFILER == 'pprofile': + import pprofile + prof = pprofile.Profile() + else: from cProfile import Profile - prof_file = '%s%s' % (func.__name__, suffix) - try: - os.unlink(prof_file) - except OSError: - pass prof = Profile() - prof.runcall(func) - prof.dump_stats(prof_file) + + prof_file = '%s%s' % (func.__name__, suffix) + try: + os.unlink(prof_file) + except OSError: + pass + prof.runcall(func, *args, **kw) + prof.dump_stats(prof_file) def beforeTearDown(self): # Re-enable gc at teardown. gc.enable() self.abort() + class TestPerformance(TestPerformanceMixin): def getTitle(self): -- 2.30.9 From 82f53c12ab8d626789418e459b19047bbbeaf3d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Tue, 9 Jul 2019 05:01:00 +0200 Subject: [PATCH 08/10] testPerformance: add benchmarks for getProperty and edit --- product/ERP5Type/tests/testPerformance.py | 73 +++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/product/ERP5Type/tests/testPerformance.py b/product/ERP5Type/tests/testPerformance.py index 7197811fbbb..755885452d2 100644 --- a/product/ERP5Type/tests/testPerformance.py +++ b/product/ERP5Type/tests/testPerformance.py @@ -31,6 +31,7 @@ from time import time import gc import subprocess +from zExceptions import Unauthorized from DateTime import DateTime from Products.ERP5Type.tests.ERP5TypeTestCase import ERP5TypeTestCase from zLOG import LOG @@ -385,3 +386,75 @@ class TestPerformance(TestPerformanceMixin): MIN_OBJECT_MANY_LINES_VIEW, req_time, MAX_OBJECT_MANY_LINES_VIEW)) + + +class TestPropertyPerformance(TestPerformanceMixin): + def afterSetUp(self): + super(TestPerformanceMixin, self).afterSetUp() + self.foo = self.portal.foo_module.newContent( + portal_type='Foo', + title='Foo Test', + protected_property='Restricted Property', + ) + + # we will run the test as anonymous user. Setup permissions so that anymous can + # get and set all properties, except `protected_property` (unless test change to another user) + for permission in ('View', 'Access contents information', 'Modify portal content'): + self.foo.manage_permission(permission, ['Anonymous'], 1) + + self.tic() + self.logout() + + def _benchmark(self, nb_iterations, min_time, max_time): + def decorated(f): + before = time() + for i in xrange(nb_iterations): + f(i) + after = time() + total_time = (after - before) / 100. + + print ("time %s.%s %.4f < %.4f < %.4f\n" % \ + ( self.id(), + f.__doc__ or f.__name__, + min_time, + total_time, + max_time )) + if PROFILE: + self.profile(f, args=(i, )) + if DO_TEST: + self.assertTrue( + min_time < total_time < max_time, + '%.4f < %.4f < %.4f' % + (min_time, total_time, max_time)) + return decorated + + def test_getProperty_protected_property_refused(self): + getProperty = self.foo.getProperty + self.assertRaises(Unauthorized, getProperty, 'protected_property') + + @self._benchmark(100000, 0.0001, 1) + def getPropertyWithRestrictedPropertyRefused(_): + getProperty('protected_property', checked_permission='Access contents information') + + def test_getProperty_protected_property_allowed(self): + getProperty = self.foo.getProperty + self.login() + @self._benchmark(100000, 0.0001, 1) + def getPropertyWithRestrictedPropertyAllowed(_): + getProperty('protected_property', checked_permission='Access contents information') + + def test_getProperty_simple_property(self): + getProperty = self.foo.getProperty + @self._benchmark(100000, 0.0001, 1) + def getPropertyWithSimpleProperty(_): + getProperty('title', checked_permission='Access contents information') + + def test_edit_restricted_properties(self): + _edit = self.foo.edit + self.login() + @self._benchmark(10000, 0.0001, 1) + def edit(i): + _edit( + title=str(i), + protected_property=str(i) + ) -- 2.30.9 From 096679ba4c44f23576945a9f322655e4860747d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Tue, 9 Jul 2019 05:49:04 +0200 Subject: [PATCH 09/10] ERP5Type: micro-optimize edit/getProperty Optimize edit by calculating the set of restricted accessors only once at class generation instead of calculating it each call. Optimize edit by using the same technique to decide if getattr or guarded_getattr has to be used to get the property. TempBase had to be special-cased to keep compatibility with some existing usages of calling TempBase.edit(modification_date=...) where somewhere in CMF classes a protected setModificationDate exists. Instead of rewriting existing cases to use something lighter (why do we need TempBase anyway ?), we do minimal compatibility fix for TempBase only. Reuse the INFINITE_SET from matrix after moving it to a shared place, since this is also useful here. --- product/ERP5Type/Base.py | 58 ++++++++++++++----- product/ERP5Type/Utils.py | 4 ++ product/ERP5Type/dynamic/portal_type_class.py | 15 +++++ product/ERP5Type/mixin/matrix.py | 3 +- 4 files changed, 65 insertions(+), 15 deletions(-) diff --git a/product/ERP5Type/Base.py b/product/ERP5Type/Base.py index 32ca46e495c..2ccf1a6d2d0 100644 --- a/product/ERP5Type/Base.py +++ b/product/ERP5Type/Base.py @@ -67,6 +67,7 @@ from Products.ERP5Type.patches.CMFCoreSkinnable import SKINDATA, skinResolve from Products.ERP5Type.Utils import UpperCase from Products.ERP5Type.Utils import convertToUpperCase, convertToMixedCase from Products.ERP5Type.Utils import createExpressionContext, simple_decorator +from Products.ERP5Type.Utils import INFINITE_SET from Products.ERP5Type.Accessor.Accessor import Accessor from Products.ERP5Type.Accessor.Constant import PropertyGetter as ConstantGetter from Products.ERP5Type.Accessor.TypeDefinition import list_types @@ -755,6 +756,10 @@ class Base( CopyContainer, aq_method_generating = [] aq_portal_type = {} aq_related_generated = 0 + # These sets are generated when dynamically making a portal type class to + # short-cut guarded_getattr in edit/getProperty. For classes that are not + # dynamically generated from portal type, we always check security. + _restricted_getter_set = _restricted_setter_set = INFINITE_SET # Only generateIdList may look at this property. Anything else is unsafe. _id_generator_state = None @@ -1222,7 +1227,15 @@ class Base( CopyContainer, def _getProperty(self, key, d=_MARKER, restricted=False, **kw): __traceback_info__ = (key,) accessor_name = 'get' + UpperCase(key) - _getattr = guarded_getattr if restricted else getattr + + # for restricted properties (ie. properties protected with another permission + # that AccessContentsInformation, which was already checked when calling this + # getProperty), we use guarded_getattr, other we use a simple getattr that + # is slightly faster. + _getattr = guarded_getattr \ + if restricted and accessor_name in self.__class__._restricted_getter_set \ + else getattr + aq_self = aq_base(self) if getattr(aq_self, accessor_name, None) is not None: try: @@ -1470,14 +1483,8 @@ class Base( CopyContainer, unordered_key_list = [k for k in key_list if k not in edit_order] ordered_key_list = [k for k in edit_order if k in key_list] if restricted: - # retrieve list of accessors which doesn't use default permissions - restricted_method_set = {method - for ancestor in self.__class__.mro() - for permissions in getattr(ancestor, '__ac_permissions__', ()) - if permissions[0] not in ('Access contents information', - 'Modify portal content') - for method in permissions[1] - if method.startswith('set')} + # accessors which doesn't use default permissions + restricted_method_set = self.__class__._restricted_setter_set else: restricted_method_set = () @@ -1512,7 +1519,7 @@ class Base( CopyContainer, accessor_name = 'set' + UpperCase(key) if accessor_name in restricted_method_set: # will raise Unauthorized when not allowed - guarded_getattr(self, accessor_name) + guarded_getattr(self, accessor_name, None) modified_property_dict[key] = old_value if key != 'id': modified_object_list = _setProperty(key, kw[key]) @@ -3591,9 +3598,10 @@ def removeIContentishInterface(cls): removeIContentishInterface(Base) class TempBase(Base): - """ - If we need Base services (categories, edit, etc) in temporary objects - we shoud used TempBase + """A version of Base that does not persist in ZODB. + + This class only has the Base methods, so most of the times it is + preferable to use a temporary portal type instead. """ isIndexable = ConstantGetter('isIndexable', value=False) isTempDocument = ConstantGetter('isTempDocument', value=True) @@ -3642,3 +3650,27 @@ class TempBase(Base): # allow_class(TempBase) in ERP5Type/Document/__init__.py will trample our # ClassSecurityInfo with one that doesn't declare our public methods InitializeClass(TempBase) + +# TempBase is not a dynamic class, so it does not get _restricted_getter_set +# and _restricted_setter_set initialized ( this is only about +# Products.ERP5Type.Document.newTempBase , other temp objects are initialized ). +# Because TempBase.edit is public, there are existing cases in ERP5 code base +# where TempBase.edit is called on documents for which current user does not have +# modify portal content permission, so if we use the default behavior from Base, +# which is to check security for everything, some usages starts to raise Unauthorized. +# Also, these calls would becomes slower and the typical use case is to build a list +# of temp objects for a listbox, so we'd like to keep this fast. +TempBase._restricted_setter_set = { + method for ancestor in TempBase.mro() + for permissions in getattr(ancestor, '__ac_permissions__', ()) + if permissions[0] not in ( + Permissions.AccessContentsInformation, + Permissions.ModifyPortalContent) + for method in permissions[1] if method.startswith('set') +} +TempBase._restricted_getter_set = { + method for ancestor in TempBase.mro() + for permissions in getattr(ancestor, '__ac_permissions__', ()) + if permissions[0] not in (Permissions.AccessContentsInformation, ) + for method in permissions[1] if method.startswith('get') +} diff --git a/product/ERP5Type/Utils.py b/product/ERP5Type/Utils.py index 5074dee6e4a..855c807e885 100644 --- a/product/ERP5Type/Utils.py +++ b/product/ERP5Type/Utils.py @@ -303,6 +303,10 @@ def cartesianProduct(list_of_list): append([v] + p) return result +# Emulate an infinite-set, ie. returning containing all objects. +# At this point we don't implement full API compatibility with set. +INFINITE_SET = type('INFINITE_SET', (object,), {'__contains__': lambda *args: True})() + # Some list operations def keepIn(value_list, filter_list): # XXX this is [x for x in value_list if x in filter_list] diff --git a/product/ERP5Type/dynamic/portal_type_class.py b/product/ERP5Type/dynamic/portal_type_class.py index 49e5fb0890e..fd3fd7f175f 100644 --- a/product/ERP5Type/dynamic/portal_type_class.py +++ b/product/ERP5Type/dynamic/portal_type_class.py @@ -323,6 +323,21 @@ def generatePortalTypeClass(site, portal_type_name): if portal_type_name in core_portal_type_class_dict: core_portal_type_class_dict[portal_type_name]['generating'] = False + attribute_dict['_restricted_setter_set'] = {method + for ancestor in base_class_list + for permissions in getattr(ancestor, '__ac_permissions__', ()) + if permissions[0] not in ('Access contents information', + 'Modify portal content') + for method in permissions[1] + if method.startswith('set')} + + attribute_dict['_restricted_getter_set'] = {method + for ancestor in base_class_list + for permissions in getattr(ancestor, '__ac_permissions__', ()) + if permissions[0] not in ('Access contents information', ) + for method in permissions[1] + if method.startswith('get')} + #LOG("ERP5Type.dynamic", INFO, # "Portal type %s loaded with bases %s" \ # % (portal_type_name, repr(base_class_list))) diff --git a/product/ERP5Type/mixin/matrix.py b/product/ERP5Type/mixin/matrix.py index 8101b17b760..5952e7001a7 100644 --- a/product/ERP5Type/mixin/matrix.py +++ b/product/ERP5Type/mixin/matrix.py @@ -30,12 +30,11 @@ from Products.ERP5Type.Globals import InitializeClass, PersistentMapping from Acquisition import aq_base from AccessControl import ClassSecurityInfo from Products.ERP5Type import Permissions -from Products.ERP5Type.Utils import cartesianProduct +from Products.ERP5Type.Utils import cartesianProduct, INFINITE_SET from Products.ERP5Type.Accessor.Constant import PropertyGetter as ConstantGetter from zLOG import LOG -INFINITE_SET = type('', (object,), {'__contains__': lambda *args: True})() class Matrix(object): """A mix-in class which provides a matrix like access to objects. -- 2.30.9 From 16aa613485a7391441056b952fa591bfed6e9e60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9rome=20Perrin?= Date: Mon, 2 Sep 2019 12:21:44 +0200 Subject: [PATCH 10/10] ZSQLCatalog: use _getProperty to skip security checks During indexation we don't apply security checks, so this should be a little bit faster. With this change _getProperty becomes a more "official" API, so some small changes had to be made to classes which do not inherits from Products.ERP5Type.Base.Base, so that they also implements _getProperty: - for ERP5Site we simply use getProperty - for the test class from testERP5Catalog, the change is a bit more important, because this class never defined getProperty, so during that test we were just acquiring a getProperty from portal. --- product/ERP5/ERP5Site.py | 6 ++++++ product/ERP5Catalog/tests/testERP5Catalog.py | 8 ++++++++ product/ZSQLCatalog/SQLCatalog.py | 2 +- 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/product/ERP5/ERP5Site.py b/product/ERP5/ERP5Site.py index 77e33e7e8a5..421192bd48b 100644 --- a/product/ERP5/ERP5Site.py +++ b/product/ERP5/ERP5Site.py @@ -565,6 +565,12 @@ class ERP5Site(FolderMixIn, CMFSite, CacheCookieMixin): return self._v_version_priority_name_list + # Make sure ERP5Site follow same API as Products.ERP5Type.Base + + # _getProperty is missing, but since there are no protected properties + # on an ERP5 Site, we can just use getProperty instead. + _getProperty = CMFSite.getProperty + security.declareProtected(Permissions.AccessContentsInformation, 'getUid') def getUid(self): """ diff --git a/product/ERP5Catalog/tests/testERP5Catalog.py b/product/ERP5Catalog/tests/testERP5Catalog.py index aac6a29554f..d06edebb1ad 100644 --- a/product/ERP5Catalog/tests/testERP5Catalog.py +++ b/product/ERP5Catalog/tests/testERP5Catalog.py @@ -33,6 +33,7 @@ import unittest import httplib from AccessControl import getSecurityManager from AccessControl.SecurityManagement import newSecurityManager +from Acquisition import aq_base from DateTime import DateTime from _mysql_exceptions import ProgrammingError from OFS.ObjectManager import ObjectManager @@ -44,6 +45,8 @@ from Testing import ZopeTestCase from zLOG import LOG class IndexableDocument(ObjectManager): + # This tests uses a simple ObjectManager, but ERP5Catalog only + # support classes inherting from ERP5Type.Base. # this property is required for dummy providesIMovement __allow_access_to_unprotected_subobjects__ = 1 @@ -66,6 +69,11 @@ class IndexableDocument(ObjectManager): return lambda: 0 raise AttributeError, name + def getProperty(self, prop, default=None): + return getattr(aq_base(self), prop, default) + + _getProperty = getProperty + def getPath(self): return self._path diff --git a/product/ZSQLCatalog/SQLCatalog.py b/product/ZSQLCatalog/SQLCatalog.py index 86ebd4c95fe..f46948283eb 100644 --- a/product/ZSQLCatalog/SQLCatalog.py +++ b/product/ZSQLCatalog/SQLCatalog.py @@ -1395,7 +1395,7 @@ class Catalog(Folder, # objects but we could also use over multiple transactions # if this can improve performance significantly # ZZZ - we could find a way to compute this once only - cache_key = tuple(object.getProperty(key) for key + cache_key = tuple(object._getProperty(key) for key in expression_cache_key_list) try: if expression_result_cache[cache_key]: -- 2.30.9