cubicweb #2930861 [security] update perms fire on attributes at entity creation time [validation pending]

This makes hard to define some otherwise straightforward security rules: Scenario.__permissions__ = { 'read': ('managers', 'modellers', 'users', 'agents'), 'add': ('managers', 'modellers', 'users'), 'update': ('managers', 'modellers', ERQLExpression('V has_scenario X, V is_master False')), 'delete': ('managers', ERQLExpression('NOT EXISTS(TS has_scenario X)')) } The following snippets chokes: with debugged(DBG_SEC): scen = req.execute('INSERT Scenario S: S name "BABAR"') cnx.commit() Security tracing shows this: check_perm: 'add' 'Scenario': user user matches frozenset(['modellers', 'managers', 'users']) with set([u'users']) check_perm: 'update' 'Scenario' [(ERQLExpression(Any X WHERE V has_scenario X, V is_master False, X eid %(x)s), {'eid': 5623}, False)] check_perm: 'update' 'attribute Scenario.name[String]' [(ERQLExpression(Any X WHERE U has_update_permission X, X eid %(x)s, U eid %(u)s), {'eid': 5623}, False)] check_perm: 'update' 'Scenario' [(ERQLExpression(Any X WHERE V has_scenario X, V is_master False, X eid %(x)s), {'eid': 5623}, False)] Since I didn't explicitely overrid the attribute permissions settings it should not be checked. There might be a special marker for these situations, that allows to say: if rdef.permissions is not DEFAULT: # do the attribute check else: # do nothing, the etype permissions are all we wanted to check
priority	normal
type	bug
done in	3.17.3
load	1.000
load left	0.000
closed by	#6c4ae3a06619 [hooks/security] Streamline attributes default permission check.
patch	[hooks/security] Optimize attributes permission check. [applied]

Workflow history

from state (3)	to state	date	User
done	validation pending	2013/07/09 16:14 UTC	narval
in-progress	done	2013/07/03 13:13 UTC
open	in-progress	2013/06/11 12:18 UTC

Comments

2013/06/10 19:52, written by sthenault

I don't get your later remark. if you associate an empty tuple to the 'update' key in the permissions dictionary, you don't get "the default security", you get an empty tuple. And iirc that's the target.
- 2013/06/10 19:57, written by acampeas
  
  I didn't associate anything to the Scenario.name per se and I have seen rdef.permissions.get('update') yield the default cw permission.
  - 2013/06/11 08:39, written by sthenault
    
    not specifying anything and explicitly specifying an empty tuple are two differents things. In the first case you implicitly ask for the default values.
    - 2013/06/11 09:59, written by acampeas
      
      Ticket description updated after reflections on this.
2013/06/11 09:57, written by acampeas

The two "check_perm: 'update' 'Scenario'" are quite disturbing also.
- 2013/06/11 10:20, written by acampeas
  
  I understand the second comes as a followup on the "U has_update_permission X" of the Scenario.name check.

2013/06/11 10:43, written by acampeas

an hypothetical first step:

diff --git a/hooks/security.py b/hooks/security.py
--- a/hooks/security.py
+++ b/hooks/security.py
@@ -23,6 +23,8 @@ the user connected to a session

 from logilab.common.registry import objectify_predicate

+from yams import buildobjs
+
 from cubicweb import Unauthorized
 from cubicweb.server import BEFORE_ADD_RELATIONS, ON_COMMIT_ADD_RELATIONS, hook

@@ -41,7 +43,11 @@ def check_entity_attributes(session, ent
         rdef = eschema.rdef(attr)
         if rdef.final: # non final relation are checked by other hooks
             # add/delete should be equivalent (XXX: unify them into 'update' ?)
-            if creation and not rdef.permissions.get('update'):
+            updateperm = rdef.permissions.get('update')
+            if creation and not updateperm:
+                # someone stuck an empty tuple, probably meaning immutability
+                continue
+            if updateperm == buildobjs.DEFAULT_ATTRPERMS['update']:
                 continue
             rdef.check_perm(session, 'update', eid=eid)

2013/06/25 11:16, written by sthenault

This probably won't work because of the presence of an ERQLExpression in the DEFAULT_ATTRPERMS. Also order may matters.

While I would prefer a proper solution, I would be fine with such patch in the mean time, provided an XXX line before the attribute update perms comparison.
- 2013/06/25 11:20, written by acampeas
  
  This snippet si obsolete, sorry. The patch says it all.

2013/06/25 13:14, written by acampeas

example workaround:

Scenario.__permissions__ = {
    'read': ('managers', 'modellers', 'users', 'agents'),
    'add': ('managers', 'modellers', 'users'),
    'update': ('managers', 'modellers',
               # Handled by a hook til https://www.cubicweb.org/2930861 is fixed
               # ERQLExpression('V has_scenario X, V is_master False')),
               'users'
               ),
    'delete': ('managers', ERQLExpression('NOT EXISTS(TS has_scenario X)'))
}

# look at the third 'if'
class DoNotModifyBasecaseName(Hook):
    __regid__ = 'do_not_modify_basecase_name'
    __select__ = Hook.__select__ & is_instance('Scenario',)
    events = ('before_update_entity',)
    category = 'scenarioset'

    def __call__(self):
        old_name, new_name = self.entity.cw_edited.oldnewvalue('name')
        if old_name != new_name:
            if old_name == variables.DEFAULT_SCENARIO_NAME:
                msg = _('You are not allowed to rename the BaseCase')
                raise ValidationError(self.entity.eid, {'name': msg})
            if not self._cw.user.matching_groups(('managers', 'modellers')):
                # workaround for https://www.cubicweb.org/2930861, see
                # Scenario __permissions__ in schema.py
                raise Unauthorized('update', 'Scenario.name')

As you can see it is not too nice. The UI is uninformed of the hook rule. (Also while doing this I introduced a bug wrt the spec; can you spot it ?)