Commit 6c2435cf authored by Jérome Perrin's avatar Jérome Perrin

Support properties' read permission in the GUI and in getProperty

Because unlike `getFoo()`,  `getProperty('foo')` does not checks the permission defined on the accessor, when a form contain a `my_foo` field, the property would be displayed to the user who can view the form, even if the user does not actually have the permission to get this property. This because getter for default value of fields uses getProperty ( [here](https://lab.nexedi.com/nexedi/erp5/blob/58d4ab8efef748f522b3eaaecba3dc1133c99e72/product/ERP5Form/Form.py#L275) ).

These changes modify behavior of `getProperty`, so that it enforces read permission security of properties and raise when user does not have permission to access properties.

Some notes about implementation:
 * `getProperty` now becomes a bit slower, but it was incorrect before, so I guess it's inevitable.
 * some efforts have been made to keep the impact on performance minimal. This uses the same approach of  in `edit` of computing the set of restricted properties and using `guarded_getattr` only on these properties and using `getattr` on non-restricted properties. The computation of this set was moved  to dynamic class generation time and as a result, `edit` becomes a bit faster.
 * the `expectedFailure` part of `test_PropertySheetSecurityOnAccessors` was moved to another test, but I'm not even sure we want to support this (read-protecting properties with default write permission) as, to me, such configuration does not make much sense. 
 * new performance tests were added. I don't know what to use as min/max values so I just used something that should pass.
 * implementation for `getProperty('*_list')` was changed a lot, I have no idea why this was getting the method on the class and passing self as first argument. Now it we just get method on the instance, like we do for single properties.

/reviewed-on !181
parents 55351d08 16aa6134
......@@ -5,5 +5,6 @@
<portal_type id="Foo">
<item>Amount</item>
<item>DublinCore</item>
<item>Foo</item>
</portal_type>
</property_sheet_list>
\ No newline at end of file
<?xml version="1.0"?>
<ZopeData>
<record id="1" aka="AAAAAAAAAAE=">
<pickle>
<global name="Property Sheet" module="erp5.portal_type"/>
</pickle>
<pickle>
<dictionary>
<item>
<key> <string>_count</string> </key>
<value>
<persistent> <string encoding="base64">AAAAAAAAAAI=</string> </persistent>
</value>
</item>
<item>
<key> <string>_mt_index</string> </key>
<value>
<persistent> <string encoding="base64">AAAAAAAAAAM=</string> </persistent>
</value>
</item>
<item>
<key> <string>_tree</string> </key>
<value>
<persistent> <string encoding="base64">AAAAAAAAAAQ=</string> </persistent>
</value>
</item>
<item>
<key> <string>description</string> </key>
<value>
<none/>
</value>
</item>
<item>
<key> <string>id</string> </key>
<value> <string>Foo</string> </value>
</item>
<item>
<key> <string>portal_type</string> </key>
<value> <string>Property Sheet</string> </value>
</item>
</dictionary>
</pickle>
</record>
<record id="2" aka="AAAAAAAAAAI=">
<pickle>
<global name="Length" module="BTrees.Length"/>
</pickle>
<pickle> <int>0</int> </pickle>
</record>
<record id="3" aka="AAAAAAAAAAM=">
<pickle>
<global name="OOBTree" module="BTrees.OOBTree"/>
</pickle>
<pickle>
<none/>
</pickle>
</record>
<record id="4" aka="AAAAAAAAAAQ=">
<pickle>
<global name="OOBTree" module="BTrees.OOBTree"/>
</pickle>
<pickle>
<none/>
</pickle>
</record>
</ZopeData>
<?xml version="1.0"?>
<ZopeData>
<record id="1" aka="AAAAAAAAAAE=">
<pickle>
<global name="Standard Property" module="erp5.portal_type"/>
</pickle>
<pickle>
<dictionary>
<item>
<key> <string>categories</string> </key>
<value>
<tuple>
<string>elementary_type/string</string>
</tuple>
</value>
</item>
<item>
<key> <string>description</string> </key>
<value> <string>A protected property for UI test. This is a property that only managers should be able to see and modify</string> </value>
</item>
<item>
<key> <string>id</string> </key>
<value> <string>protected_property_property</string> </value>
</item>
<item>
<key> <string>portal_type</string> </key>
<value> <string>Standard Property</string> </value>
</item>
<item>
<key> <string>read_permission</string> </key>
<value> <string>Manage portal</string> </value>
</item>
<item>
<key> <string>write_permission</string> </key>
<value> <string>Manage portal</string> </value>
</item>
</dictionary>
</pickle>
</record>
</ZopeData>
<?xml version="1.0"?>
<ZopeData>
<record id="1" aka="AAAAAAAAAAE=">
<pickle>
<global name="ERP5 Form" module="erp5.portal_type"/>
</pickle>
<pickle>
<dictionary>
<item>
<key> <string>_bind_names</string> </key>
<value>
<object>
<klass>
<global name="NameAssignments" module="Shared.DC.Scripts.Bindings"/>
</klass>
<tuple/>
<state>
<dictionary>
<item>
<key> <string>_asgns</string> </key>
<value>
<dictionary/>
</value>
</item>
</dictionary>
</state>
</object>
</value>
</item>
<item>
<key> <string>_objects</string> </key>
<value>
<tuple/>
</value>
</item>
<item>
<key> <string>action</string> </key>
<value> <string>Base_edit</string> </value>
</item>
<item>
<key> <string>description</string> </key>
<value> <string>View with some restricted properties</string> </value>
</item>
<item>
<key> <string>edit_order</string> </key>
<value>
<list/>
</value>
</item>
<item>
<key> <string>encoding</string> </key>
<value> <string>UTF-8</string> </value>
</item>
<item>
<key> <string>enctype</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>group_list</string> </key>
<value>
<list>
<string>left</string>
<string>right</string>
<string>center</string>
<string>bottom</string>
<string>hidden</string>
</list>
</value>
</item>
<item>
<key> <string>groups</string> </key>
<value>
<dictionary>
<item>
<key> <string>bottom</string> </key>
<value>
<list/>
</value>
</item>
<item>
<key> <string>center</string> </key>
<value>
<list/>
</value>
</item>
<item>
<key> <string>hidden</string> </key>
<value>
<list/>
</value>
</item>
<item>
<key> <string>left</string> </key>
<value>
<list>
<string>my_protected_property</string>
<string>my_foo_category_title</string>
</list>
</value>
</item>
<item>
<key> <string>right</string> </key>
<value>
<list/>
</value>
</item>
</dictionary>
</value>
</item>
<item>
<key> <string>id</string> </key>
<value> <string>Foo_viewSecurity</string> </value>
</item>
<item>
<key> <string>method</string> </key>
<value> <string>POST</string> </value>
</item>
<item>
<key> <string>name</string> </key>
<value> <string>Foo_view</string> </value>
</item>
<item>
<key> <string>pt</string> </key>
<value> <string>form_view</string> </value>
</item>
<item>
<key> <string>row_length</string> </key>
<value> <int>4</int> </value>
</item>
<item>
<key> <string>stored_encoding</string> </key>
<value> <string>UTF-8</string> </value>
</item>
<item>
<key> <string>title</string> </key>
<value> <string>Foo Security</string> </value>
</item>
<item>
<key> <string>unicode_mode</string> </key>
<value> <int>0</int> </value>
</item>
<item>
<key> <string>update_action</string> </key>
<value> <string></string> </value>
</item>
<item>
<key> <string>update_action_title</string> </key>
<value> <string></string> </value>
</item>
</dictionary>
</pickle>
</record>
</ZopeData>
Bar | Reference
Foo | Amount
Foo | DublinCore
\ No newline at end of file
Foo | DublinCore
Foo | Foo
\ No newline at end of file
......@@ -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
......
......@@ -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):
"""
......
......@@ -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
......
......@@ -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.newContent(portal_type='Foo', id='foo', foo_category='a')
foo_module = self.portal.foo_module
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)
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,39 @@ 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 = '<input name="field_my_foo_category_title" value="a" type="text"'
sequence_list = SequenceList()
sequence_string = '\
CreateObjects \
CreateTestFoo \
Tic \
AccessFoo \
AccessFooDoesNotRaise \
AccessFooDisplaysCategoryName \
ChangeCategorySecurity \
AccessFoo \
Tic \
AccessFooDoesNotRaise \
AccessFooDoesNotDisplayCategoryName \
ResetCategorySecurity \
AccessFoo \
Tic \
AccessFooDoesNotRaise \
'
sequence_list.addSequenceString(sequence_string)
sequence_list.play(self, quiet=quiet)
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 = '<input name="field_my_protected_property" value="Protected Property" type="text"'
self.assertIn(protected_property_markup, self.portal.foo_module.foo.Foo_viewSecurity())
def test_suite():
suite = unittest.TestSuite()
suite.addTest(unittest.makeSuite(TestGUISecurity))
return suite
self.loginAs() # user without permission to access protected property
self.assertNotIn(protected_property_markup, self.portal.foo_module.foo.Foo_viewSecurity())
......@@ -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
......@@ -1216,11 +1221,29 @@ 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)
# 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:
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 +1257,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
......@@ -1455,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 = ()
......@@ -1497,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])
......@@ -2654,13 +2676,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
......@@ -3576,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)
......@@ -3627,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)