From 5c6a7f0f97cc88d4ec8e8b8d44466afc54ffa8d6 Mon Sep 17 00:00:00 2001 From: Gaetan Delannay Date: Sat, 3 May 2014 22:45:51 +0200 Subject: [PATCH] [gen] Security improvements. --- fields/__init__.py | 8 ++-- fields/calendar.py | 6 ++- fields/pod.py | 4 +- fields/ref.py | 77 ++++++++++++++++++----------------- fields/workflow.py | 2 +- gen/mixins/__init__.py | 80 +++++++++++++++++++++++-------------- gen/ui/appy.css | 2 +- gen/wrappers/ToolWrapper.py | 75 ++++++++++++++++++---------------- gen/wrappers/__init__.py | 12 +++--- 9 files changed, 146 insertions(+), 120 deletions(-) diff --git a/fields/__init__.py b/fields/__init__.py index 6bcf415..0213f33 100644 --- a/fields/__init__.py +++ b/fields/__init__.py @@ -307,9 +307,9 @@ class Field: '''When displaying p_obj on a given p_layoutType, must we show this field?''' # Check if the user has the permission to view or edit the field - if layoutType == 'edit': perm = self.writePermission - else: perm = self.readPermission - if not obj.allows(perm): return False + perm = (layoutType == 'edit') and self.writePermission or \ + self.readPermission + if not obj.allows(perm): return # Evaluate self.show if callable(self.show): res = self.callMethod(obj, self.show) @@ -319,7 +319,7 @@ class Field: if type(res) in sutils.sequenceTypes: for r in res: if r == layoutType: return True - return False + return elif res in ('view', 'edit', 'result'): return res == layoutType return bool(res) diff --git a/fields/calendar.py b/fields/calendar.py index 467ca93..b6db5c3 100644 --- a/fields/calendar.py +++ b/fields/calendar.py @@ -16,7 +16,7 @@ class Calendar(Field): # Month view for a calendar. Called by pxView, and directly from the UI, # via Ajax, when the user selects another month. pxMonthView = Px(''' -
- - @@ -114,7 +113,7 @@ class Ref(Field): - + @@ -141,7 +140,7 @@ class Ref(Field): # Displays the button allowing to add a new object through a Ref field, if # it has been declared as addable and if multiplicities allow it. pxAdd = Px(''' - @@ -195,7 +193,7 @@ class Ref(Field): # PX that displays referred objects as a list. pxViewList = Px(''' -
+
:_(subLabel) (:totalNumber) :field.pxAdd @@ -215,7 +213,7 @@ class Ref(Field):

:_('no_ref')

+ if="not objects and (innerRef and mayAdd)">:_('no_ref')

+ mayView=tied.o.mayView()"> -
:field.pxNumber @@ -263,15 +261,15 @@ class Ref(Field): if="field.isShowable(zobj, 'result')">:field.pxRender - +
-
:field.pxGlobalActions
@@ -288,7 +286,6 @@ class Ref(Field): :field.getCbJsInit(zobj) -
:field.pxViewPickList
= self.multiplicity[1]: return gutils.No('max_reached') # May the user edit this Ref field? - if not obj.allows(self.writePermission): - return gutils.No('no_write_perm') + if checkMayEdit: + if not obj.mayEdit(self.writePermission): + return gutils.No('no_write_perm') # May the user create instances of the referred class? if not obj.getTool().userMayCreate(self.klass): return gutils.No('no_add_perm') @@ -943,9 +943,8 @@ class Ref(Field): m_mayAdd returns False.''' may = self.mayAdd(obj) if not may: - from AccessControl import Unauthorized - raise Unauthorized("User can't write Ref field '%s' (%s)." % \ - (self.name, may.msg)) + obj.raiseUnauthorized("User can't write Ref field '%s' (%s)." % \ + (self.name, may.msg)) def getCbJsInit(self, obj): '''When checkboxes are enabled, this method defines a JS associative @@ -1070,7 +1069,7 @@ class Ref(Field): # "link_many", "unlink_many", "delete_many". As a preamble, perform # a security check once, instead of doing it on every object-level # operation. - obj.allows(self.writePermission, raiseError=True) + obj.mayEdit(self.writePermission, raiseError=True) # Get the (un-)checked objects from the request. uids = rq['targetUid'].strip(',') or (); if uids: uids = uids.split(',') diff --git a/fields/workflow.py b/fields/workflow.py index d56690d..8c1caf1 100644 --- a/fields/workflow.py +++ b/fields/workflow.py @@ -397,7 +397,7 @@ class Transition: if not obj.isTemporary(): obj.reindex() # If we are viewing the object and if the logged user looses the # permission to view it, redirect the user to its home page. - if not obj.allows('read') and \ + if not obj.mayView() and \ (obj.absolute_url_path() in rq['HTTP_REFERER']): back = tool.getHomePage() else: diff --git a/gen/mixins/__init__.py b/gen/mixins/__init__.py index 2ce4af5..846080b 100644 --- a/gen/mixins/__init__.py +++ b/gen/mixins/__init__.py @@ -276,8 +276,7 @@ class BaseMixin: # used back/forward buttons of its browser... userId = user.login if (page in self.locks) and (userId != self.locks[page][0]): - from AccessControl import Unauthorized - raise Unauthorized('This page is locked.') + self.raiseUnauthorized('This page is locked.') # Set the lock from DateTime import DateTime self.locks[page] = (userId, DateTime()) @@ -301,8 +300,7 @@ class BaseMixin: if not force: userId = self.getTool().getUser().login if self.locks[page][0] != userId: - from AccessControl import Unauthorized - raise Unauthorized('This page was locked by someone else.') + self.raiseUnauthorized('This page was locked by someone else.') # Remove the lock del self.locks[page] @@ -444,10 +442,9 @@ class BaseMixin: # onEdit), redirect to the main site page. if not getattr(obj.getParentNode().aq_base, obj.id, None): return self.goto(tool.getSiteUrl(), msg) - # If the user can't access the object anymore, redirect him to the - # main site page. - if not obj.allows('read'): - return self.goto(tool.getSiteUrl(), msg) + # If the user can't access the object anymore, redirect him to its home + # page. + if not obj.mayView(): return self.goto(tool.getHomePage(), msg) if (buttonClicked == 'save') or saveConfirmed: obj.say(msg) if isNew and initiator: @@ -520,8 +517,7 @@ class BaseMixin: corresponding Appy wrapper and returns, as XML, its result.''' self.REQUEST.RESPONSE.setHeader('Content-Type','text/xml;charset=utf-8') # Check if the user is allowed to consult this object - if not self.allows('read'): - return XmlMarshaller().marshall('Unauthorized') + if not self.mayView(): return XmlMarshaller().marshall('Unauthorized') if not action: marshaller = XmlMarshaller(rootTag=self.getClass().__name__, dumpUnicode=True) @@ -576,6 +572,12 @@ class BaseMixin: if rq.get('appy', None) == '1': obj = obj.appy() return getattr(obj, 'on'+action)() + def raiseUnauthorized(self, msg=None): + '''Raise an error "Unauthorized access".''' + from AccessControl import Unauthorized + if msg: raise Unauthorized(msg) + raise Unauthorized() + def rememberPreviousData(self, fields): '''This method is called before updating an object and remembers, for every historized field, the previous value. Result is a dict @@ -1131,10 +1133,11 @@ class BaseMixin: return 'main' def mayAct(self): - '''May the currently logged user see column "actions" for this - object? This can be used for hiding the "edit" icon, for example: - when a user may edit only a restricted set of fields on an object, - we may avoid showing him the global "edit" icon.''' + '''m_mayAct allows to hide the whole set of actions for an object. + Indeed, beyond workflow security, it can be useful to hide controls + like "edit" icons/buttons. For example, if a user may only edit some + Ref fields with add=True on an object, when clicking on "edit", he + will see an empty edit form.''' appyObj = self.appy() if hasattr(appyObj, 'mayAct'): return appyObj.mayAct() return True @@ -1148,14 +1151,34 @@ class BaseMixin: if hasattr(appyObj, 'mayDelete'): return appyObj.mayDelete() return True - def mayEdit(self, permission='write'): - '''May the currently logged user edit this object? p_perm can be a - field-specific permission.''' - res = self.allows(permission) + def mayEdit(self, permission='write', permOnly=False, raiseError=False): + '''May the currently logged user edit this object? p_permission can be a + field-specific permission. If p_permOnly is True, the specific + user-defined condition is not evaluated. If p_raiseError is True, if + the user may not edit p_self, an error is raised.''' + res = self.allows(permission, raiseError=raiseError) + if not res: return + if permOnly: return res + # An additional, user-defined condition, may refine the base permission. + appyObj = self.appy() + if hasattr(appyObj, 'mayEdit'): + res = appyObj.mayEdit() + if not res and raiseError: self.raiseUnauthorized() + return res + return True + + def mayView(self, permission='read', raiseError=False): + '''May the currently logged user view this object? p_permission can be a + field-specific permission. If p_raiseError is True, if the user may + not view p_self, an error is raised.''' + res = self.allows(permission, raiseError=raiseError) if not res: return # An additional, user-defined condition, may refine the base permission. appyObj = self.appy() - if hasattr(appyObj, 'mayEdit'): return appyObj.mayEdit() + if hasattr(appyObj, 'mayView'): + res = appyObj.mayView() + if not res and raiseError: self.raiseUnauthorized() + return res return True def onExecuteAction(self): @@ -1512,14 +1535,12 @@ class BaseMixin: is retrieved from the request.''' name = self.REQUEST.get('name') if not name: return + # Security check if '_img_' not in name: - appyType = self.getAppyType(name) + field = self.getAppyType(name) else: - appyType = self.getAppyType(name.split('_img_')[0]) - if (not appyType.isShowable(self, 'view')) and \ - (not appyType.isShowable(self, 'result')): - from zExceptions import NotFound - raise NotFound() + field = self.getAppyType(name.split('_img_')[0]) + self.mayView(field.readPermission, raiseError=True) info = getattr(self.aq_base, name, None) if info: # Write the file in the HTTP response. @@ -1556,16 +1577,13 @@ class BaseMixin: def allows(self, permission, raiseError=False): '''Has the logged user p_permission on p_self ?''' res = self.getTool().getUser().has_permission(permission, self) - if not res and raiseError: - from AccessControl import Unauthorized - raise Unauthorized + if not res and raiseError: self.raiseUnauthorized() return res def isTemporary(self): '''Is this object temporary ?''' parent = self.getParentNode() - if not parent: # Is propably being created through code - return False + if not parent: return # Is probably being created through code return parent.getId() == 'temp_folder' def onProcess(self): @@ -1575,7 +1593,7 @@ class BaseMixin: def onCall(self): '''Calls a specific method on the corresponding wrapper.''' - self.allows('read', raiseError=True) + self.mayView(raiseError=True) method = self.REQUEST['method'] obj = self.appy() return getattr(obj, method)() diff --git a/gen/ui/appy.css b/gen/ui/appy.css index fe6bede..6cb1b13 100644 --- a/gen/ui/appy.css +++ b/gen/ui/appy.css @@ -104,7 +104,7 @@ td.search { padding-top: 8px } .dropdown a:hover { text-decoration: underline } .list { margin-bottom: 3px } .list td, .list th { border: 3px solid #ededed; color: grey; - padding: 3px 5px 0 5px } + padding: 3px 5px 3px 5px } .list th { background-color: #e5e5e5; font-style: italic; font-weight: normal } .grid th { font-style: italic; font-weight: normal; border-bottom: 5px solid #fdfdfd; padding: 3px 5px 0 5px } diff --git a/gen/wrappers/ToolWrapper.py b/gen/wrappers/ToolWrapper.py index efacad4..7f63a1b 100644 --- a/gen/wrappers/ToolWrapper.py +++ b/gen/wrappers/ToolWrapper.py @@ -302,43 +302,49 @@ class ToolWrapper(AbstractWrapper): # Show on query list or grid, the field content for a given object. pxQueryField = Px(''' - + - ::zobj.getSupTitle(navInfo) - :zobj.Title():zobj.Title()::zobj.getSubTitle() + cssClass=zobj.getCssFor('title')"> + ::zobj.getSupTitle(navInfo) + :zobj.Title():zobj.Title()::zobj.getSubTitle() - - - - - + + - - - - -
- +
+ - - - - - :targetObj.appy().pxTransitions
+ href=":zobj.getUrl(mode='edit', page=zobj.getDefaultEditPage(), \ + nav=navInfo)"> + + + + + + + + :targetObj.appy().pxTransitions + + + + + + :_('unauthorized') + - + :field.pxRender ''') @@ -361,7 +367,7 @@ class ToolWrapper(AbstractWrapper): + style="padding-top: 25px" + var2="obj=zobj.appy(); mayView=zobj.mayView()"> :tool.pxQueryField diff --git a/gen/wrappers/__init__.py b/gen/wrappers/__init__.py index fe9dd72..4dcec14 100644 --- a/gen/wrappers/__init__.py +++ b/gen/wrappers/__init__.py @@ -454,6 +454,7 @@ class AbstractWrapper(object): var="previousPage=phaseObj.getPreviousPage(page)[0]; nextPage=phaseObj.getNextPage(page)[0]; isEdit=layoutType == 'edit'; + mayAct=not isEdit and zobj.mayAct(); pageInfo=phaseObj.pagesInfo[page]"> @@ -486,7 +487,6 @@ class AbstractWrapper(object): style=":'%s; %s' % (url('save', bg=True), \ ztool.getButtonWidth(label))" /> - - + mayAct and zobj.mayEdit()"> :obj.pxTransitions + if="mayAct and \ + targetObj.showTransitions(layoutType)">:obj.pxTransitions ''') @@ -554,7 +554,7 @@ class AbstractWrapper(object): ''') pxView = Px(''' - ''', template=pxTemplate, hook='content') pxEdit = Px(''' -