[hooks/security] allow edition of attributes with permissive permissions

If an attribute has more permissive security rules than the entity type itself, we should be green and not deny action because of an early global entity permission check (with the more restrictive rules).

Only if one attribute with the entity-level permission rules is edited will the global check be performed.

Note:

  • the "if action == 'delete'" check at the entry of check_entity_attributes is a guard for a condition currently not happening in cubicweb itself (but application hooks could conceivably call this function with a 'delete' action)

Closes #3489895.

authorAurelien Campeas <aurelien.campeas@logilab.fr>
changeset7099bbd685aa
branchstable
phasepublic
hiddenno
parent revision#91fbd3111828 Almost backout afcd46716d6a which breaks _select_best raising an ambiguity exception in debug mode.
child revision#d91501356742 [pkg] 3.18.6
files modified by this revision
doc/book/en/devrepo/datamodel/definition.rst
hooks/security.py
server/test/data/migratedapp/schema.py
server/test/data/schema.py
server/test/unittest_msplanner.py
server/test/unittest_querier.py
server/test/unittest_security.py
# HG changeset patch
# User Aurelien Campeas <aurelien.campeas@logilab.fr>
# Date 1390919279 -3600
# Tue Jan 28 15:27:59 2014 +0100
# Branch stable
# Node ID 7099bbd685aaba8ef731c0d4ed0ba147ff29da39
# Parent 91fbd3111828e9885f5c3c7f9cf16dd863c6c784
[hooks/security] allow edition of attributes with permissive permissions

If an attribute has more permissive security rules than the entity
type itself, we should be green and not deny action because of an
early global entity permission check (with the more restrictive
rules).

Only if one attribute with the entity-level permission rules is edited
will the global check be performed.

Note:

* the "if action == 'delete'" check at the entry of
check_entity_attributes is a guard for a condition currently not
happening in cubicweb itself (but application hooks could
conceivably call this function with a 'delete' action)

Closes #3489895.

diff --git a/doc/book/en/devrepo/datamodel/definition.rst b/doc/book/en/devrepo/datamodel/definition.rst
@@ -298,11 +298,11 @@
1  The security model of `CubicWeb` is based on `Access Control List`.
2  The main principles are:
3 
4  * users and groups of users
5  * a user belongs to at least one group of user
6 -* permissions (read, update, create, delete)
7 +* permissions (`read`, `update`, `create`, `delete`)
8  * permissions are assigned to groups (and not to users)
9 
10  For *CubicWeb* in particular:
11 
12  * we associate rights at the entities/relations schema level
@@ -318,23 +318,36 @@
13      according to the context of the objects they own
14 
15    * the permissions of this group are only checked on `update`/`delete` actions
16      if all the other groups the user belongs to do not provide those permissions
17 
18 -Setting permissions is done with the attribute `__permissions__` of entities and
19 -relation definition. The value of this attribute is a dictionary where the keys
20 -are the access types (action), and the values are the authorized groups or
21 -expressions.
22 +Setting permissions is done with the class attribute `__permissions__`
23 +of entity types and relation definitions. The value of this attribute
24 +is a dictionary where the keys are the access types (action), and the
25 +values are the authorized groups or rql expressions.
26 
27  For an entity type, the possible actions are `read`, `add`, `update` and
28  `delete`.
29 
30  For a relation, the possible actions are `read`, `add`, and `delete`.
31 
32  For an attribute, the possible actions are `read`, `add` and `update`,
33  and they are a refinement of an entity type permission.
34 
35 +.. note::
36 +
37 +   By default, the permissions of an entity type attributes are
38 +   equivalent to the permissions of the entity type itself.
39 +
40 +   It is possible to provide custom attribute permissions which are
41 +   stronger than, or are more lenient than the entity type
42 +   permissions.
43 +
44 +   In a situation where all attributes were given custom permissions,
45 +   the entity type permissions would not be checked, except for the
46 +   `delete` action.
47 +
48  For each access type, a tuple indicates the name of the authorized groups and/or
49  one or multiple RQL expressions to satisfy to grant access. The access is
50  provided if the user is in one of the listed groups or if one of the RQL condition
51  is satisfied.
52 
@@ -366,10 +379,17 @@
53 
54     __permissions__ = {'read': ('managers', 'users', 'guests',),
55                        'add': ('managers', ERQLExpression('U has_add_permission X'),
56                        'update': ('managers', ERQLExpression('U has_update_permission X')),}
57 
58 +.. note::
59 +
60 +   The default permissions for attributes are not syntactically
61 +   equivalent to the default permissions of the entity types, but the
62 +   rql expressions work by delegating to the entity type permissions.
63 +
64 +
65  The standard user groups
66  ````````````````````````
67 
68  * `guests`
69 
@@ -668,11 +688,11 @@
70    In an even more remote future, it is quite possible that the
71    SubjectRelation shortcut will become deprecated, in favor of the
72    RelationType declaration which offers some advantages in the context
73    of reusable cubes.
74 
75 -  
76 +
77 
78 
79  Handling schema changes
80  ~~~~~~~~~~~~~~~~~~~~~~~
81 
diff --git a/hooks/security.py b/hooks/security.py
@@ -1,6 +1,6 @@
82 -# copyright 2003-2013 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
83 +# copyright 2003-2014 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
84  # contact http://www.logilab.fr/ -- mailto:contact@logilab.fr
85  #
86  # This file is part of CubicWeb.
87  #
88  # CubicWeb is free software: you can redistribute it and/or modify it under the
@@ -32,15 +32,19 @@
89 
90 
91  def check_entity_attributes(session, entity, action, editedattrs=None):
92      eid = entity.eid
93      eschema = entity.e_schema
94 +    if action == 'delete':
95 +        eschema.check_perm(session, action, eid=eid)
96 +        return
97      # ._cw_skip_security_attributes is there to bypass security for attributes
98      # set by hooks by modifying the entity's dictionary
99      if editedattrs is None:
100          editedattrs = entity.cw_edited
101      dontcheck = editedattrs.skip_security
102 +    etypechecked = False
103      for attr in editedattrs:
104          if attr in dontcheck:
105              continue
106          rdef = eschema.rdef(attr, takefirst=True)
107          if rdef.final: # non final relation are checked by standard hooks
@@ -52,14 +56,14 @@
108              #
109              # is deserialized in this order (groups first), and ERQLExpression
110              # implements comparison by rql expression.
111              if perms == buildobjs.DEFAULT_ATTRPERMS[action]:
112                  # The default rule is to delegate to the entity
113 -                # rule. This is an historical artefact. Hence we take
114 -                # this object as a marker saying "no specific"
115 -                # permission rule for this attribute. Thus we just do
116 -                # nothing.
117 +                # rule. This needs to be checked only once.
118 +                if not etypechecked:
119 +                    entity.cw_check_perm(action)
120 +                    etypechecked = True
121                  continue
122              if perms == ():
123                  # That means an immutable attribute; as an optimization, avoid
124                  # going through check_perm.
125                  raise Unauthorized(action, str(rdef))
@@ -69,11 +73,10 @@
126  class CheckEntityPermissionOp(hook.DataOperationMixIn, hook.LateOperation):
127      def precommit_event(self):
128          session = self.session
129          for eid, action, edited in self.get_data():
130              entity = session.entity_from_eid(eid)
131 -            entity.cw_check_perm(action)
132              check_entity_attributes(session, entity, action, edited)
133 
134 
135  class CheckRelationPermissionOp(hook.DataOperationMixIn, hook.LateOperation):
136      def precommit_event(self):
diff --git a/server/test/data/migratedapp/schema.py b/server/test/data/migratedapp/schema.py
@@ -89,10 +89,26 @@
137      shortpara = String(maxsize=64, default='hop')
138      ecrit_par = SubjectRelation('Personne', constraints=[RQLConstraint('S concerne A, O concerne A')])
139      attachment = SubjectRelation('File')
140 
141 
142 +class Frozable(EntityType):
143 +    __permissions__ = {
144 +        'read':   ('managers', 'users'),
145 +        'add':    ('managers', 'users'),
146 +        'update': ('managers', ERQLExpression('X frozen False'),),
147 +        'delete': ('managers', ERQLExpression('X frozen False'),)
148 +    }
149 +    name = String()
150 +    frozen = Boolean(default=False,
151 +                     __permissions__ = {
152 +                         'read':   ('managers', 'users'),
153 +                         'add':    ('managers', 'users'),
154 +                         'update': ('managers', 'owners')
155 +                         })
156 +
157 +
158  class Personne(EntityType):
159      __unique_together__ = [('nom', 'prenom', 'datenaiss')]
160      nom    = String(fulltextindexed=True, required=True, maxsize=64)
161      prenom = String(fulltextindexed=True, maxsize=64)
162      civility   = String(maxsize=1, default='M', fulltextindexed=True)
diff --git a/server/test/data/schema.py b/server/test/data/schema.py
@@ -1,6 +1,6 @@
163 -# copyright 2003-2013 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
164 +# copyright 2003-2014 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
165  # contact http://www.logilab.fr/ -- mailto:contact@logilab.fr
166  #
167  # This file is part of CubicWeb.
168  #
169  # CubicWeb is free software: you can redistribute it and/or modify it under the
@@ -108,10 +108,27 @@
170                                constraints=[RQLUniqueConstraint('S type T, S inline1 A1, A1 todo_by C, '
171                                                                'Y type T, Y inline1 A2, A2 todo_by C',
172                                                                 'S,Y')])
173      todo_by = SubjectRelation('CWUser')
174 
175 +
176 +class Frozable(EntityType):
177 +    __permissions__ = {
178 +        'read':   ('managers', 'users'),
179 +        'add':    ('managers', 'users'),
180 +        'update': ('managers', ERQLExpression('X frozen False'),),
181 +        'delete': ('managers', ERQLExpression('X frozen False'),)
182 +    }
183 +    name = String()
184 +    frozen = Boolean(default=False,
185 +                     __permissions__ = {
186 +                         'read':   ('managers', 'users'),
187 +                         'add':    ('managers', 'users'),
188 +                         'update': ('managers', 'owners')
189 +                         })
190 +
191 +
192  class Personne(EntityType):
193      __unique_together__ = [('nom', 'prenom', 'inline2')]
194      nom    = String(fulltextindexed=True, required=True, maxsize=64)
195      prenom = String(fulltextindexed=True, maxsize=64)
196      sexe   = String(maxsize=1, default='M', fulltextindexed=True)
diff --git a/server/test/unittest_msplanner.py b/server/test/unittest_msplanner.py
@@ -68,11 +68,11 @@
197                       {'X': 'CWSource'}, {'X': 'CWSourceHostConfig'}, {'X': 'CWSourceSchemaConfig'},
198                       {'X': 'CWUser'}, {'X': 'CWUniqueTogetherConstraint'},
199                       {'X': 'Card'}, {'X': 'Comment'}, {'X': 'Division'},
200                       {'X': 'Email'}, {'X': 'EmailAddress'}, {'X': 'EmailPart'},
201                       {'X': 'EmailThread'}, {'X': 'ExternalUri'}, {'X': 'File'},
202 -                     {'X': 'Folder'}, {'X': 'Note'}, {'X': 'Old'},
203 +                     {'X': 'Folder'}, {'X': 'Frozable'}, {'X': 'Note'}, {'X': 'Old'},
204                       {'X': 'Personne'}, {'X': 'RQLExpression'}, {'X': 'Societe'},
205                       {'X': 'State'}, {'X': 'SubDivision'}, {'X': 'SubWorkflowExitPoint'},
206                       {'X': 'Tag'}, {'X': 'TrInfo'}, {'X': 'Transition'},
207                       {'X': 'Workflow'}, {'X': 'WorkflowTransition'}])
208 
@@ -900,10 +900,11 @@
209          ueid = self.session.user.eid
210          ALL_SOLS = X_ALL_SOLS[:]
211          ALL_SOLS.remove({'X': 'CWSourceHostConfig'}) # not authorized
212          ALL_SOLS.remove({'X': 'CWSourceSchemaConfig'}) # not authorized
213          ALL_SOLS.remove({'X': 'CWDataImport'}) # not authorized
214 +        ALL_SOLS.remove({'X': 'Frozable'}) # not authorized
215          self._test('Any MAX(X)',
216                     [('FetchStep', [('Any E WHERE E type "X", E is Note', [{'E': 'Note'}])],
217                       [self.cards, self.system],  None, {'E': 'table1.C0'}, []),
218                      ('FetchStep', [('Any X WHERE X is IN(CWUser)', [{'X': 'CWUser'}])],
219                       [self.ldap, self.system], None, {'X': 'table2.C0'}, []),
@@ -957,11 +958,12 @@
220          # use a guest user
221          self.session = self.user_groups_session('guests')
222          ueid = self.session.user.eid
223          X_ET_ALL_SOLS = []
224          for s in X_ALL_SOLS:
225 -            if s in ({'X': 'CWSourceHostConfig'}, {'X': 'CWSourceSchemaConfig'}, {'X': 'CWDataImport'}):
226 +            if s in ({'X': 'CWSourceHostConfig'}, {'X': 'CWSourceSchemaConfig'},
227 +                     {'X': 'CWDataImport'}, {'X': 'Frozable'}):
228                  continue # not authorized
229              ets = {'ET': 'CWEType'}
230              ets.update(s)
231              X_ET_ALL_SOLS.append(ets)
232          self._test('Any ET, COUNT(X) GROUPBY ET ORDERBY ET WHERE X is ET',
@@ -2674,11 +2676,11 @@
233                                      [{'X': 'Note'}, {'X': 'State'}, {'X': 'Card'}])],
234                       [self.cards, self.cards2, self.system],
235                       None, {'X': 'table0.C0'}, []),
236                      ('UnionStep', None, None,
237                       [('OneFetchStep',
238 -                       [(u'Any X WHERE X owned_by U, U login "anon", U is CWUser, X is IN(Affaire, BaseTransition, Basket, Bookmark, CWAttribute, CWCache, CWConstraint, CWConstraintType, CWDataImport, CWEType, CWGroup, CWPermission, CWProperty, CWRType, CWRelation, CWSource, CWSourceHostConfig, CWSourceSchemaConfig, CWUniqueTogetherConstraint, CWUser, Division, Email, EmailAddress, EmailPart, EmailThread, ExternalUri, File, Folder, Old, Personne, RQLExpression, Societe, SubDivision, SubWorkflowExitPoint, Tag, TrInfo, Transition, Workflow, WorkflowTransition)',
239 +                       [(u'Any X WHERE X owned_by U, U login "anon", U is CWUser, X is IN(Affaire, BaseTransition, Basket, Bookmark, CWAttribute, CWCache, CWConstraint, CWConstraintType, CWDataImport, CWEType, CWGroup, CWPermission, CWProperty, CWRType, CWRelation, CWSource, CWSourceHostConfig, CWSourceSchemaConfig, CWUniqueTogetherConstraint, CWUser, Division, Email, EmailAddress, EmailPart, EmailThread, ExternalUri, File, Folder, Frozable, Old, Personne, RQLExpression, Societe, SubDivision, SubWorkflowExitPoint, Tag, TrInfo, Transition, Workflow, WorkflowTransition)',
240                           [{'U': 'CWUser', 'X': 'Affaire'},
241                            {'U': 'CWUser', 'X': 'BaseTransition'},
242                            {'U': 'CWUser', 'X': 'Basket'},
243                            {'U': 'CWUser', 'X': 'Bookmark'},
244                            {'U': 'CWUser', 'X': 'CWAttribute'},
@@ -2703,10 +2705,11 @@
245                            {'U': 'CWUser', 'X': 'EmailPart'},
246                            {'U': 'CWUser', 'X': 'EmailThread'},
247                            {'U': 'CWUser', 'X': 'ExternalUri'},
248                            {'U': 'CWUser', 'X': 'File'},
249                            {'U': 'CWUser', 'X': 'Folder'},
250 +                          {'U': 'CWUser', 'X': 'Frozable'},
251                            {'U': 'CWUser', 'X': 'Old'},
252                            {'U': 'CWUser', 'X': 'Personne'},
253                            {'U': 'CWUser', 'X': 'RQLExpression'},
254                            {'U': 'CWUser', 'X': 'Societe'},
255                            {'U': 'CWUser', 'X': 'SubDivision'},
diff --git a/server/test/unittest_querier.py b/server/test/unittest_querier.py
@@ -168,11 +168,11 @@
256                                         'I': 'Affaire',
257                                         'J': 'Affaire',
258                                         'X': 'Affaire',
259                                         'ET': 'CWEType', 'ETN': 'String'}])
260          rql, solutions = partrqls[1]
261 -        self.assertEqual(rql,  'Any ETN,X WHERE X is ET, ET name ETN, ET is CWEType, X is IN(BaseTransition, Bookmark, CWAttribute, CWCache, CWConstraint, CWConstraintType, CWEType, CWGroup, CWPermission, CWProperty, CWRType, CWRelation, CWSource, CWUniqueTogetherConstraint, CWUser, Card, Comment, Division, Email, EmailPart, EmailThread, ExternalUri, File, Folder, Note, Old, Personne, RQLExpression, Societe, State, SubDivision, SubWorkflowExitPoint, Tag, TrInfo, Transition, Workflow, WorkflowTransition)')
262 +        self.assertEqual(rql,  'Any ETN,X WHERE X is ET, ET name ETN, ET is CWEType, X is IN(BaseTransition, Bookmark, CWAttribute, CWCache, CWConstraint, CWConstraintType, CWEType, CWGroup, CWPermission, CWProperty, CWRType, CWRelation, CWSource, CWUniqueTogetherConstraint, CWUser, Card, Comment, Division, Email, EmailPart, EmailThread, ExternalUri, File, Folder, Frozable, Note, Old, Personne, RQLExpression, Societe, State, SubDivision, SubWorkflowExitPoint, Tag, TrInfo, Transition, Workflow, WorkflowTransition)')
263          self.assertListEqual(sorted(solutions),
264                                sorted([{'X': 'BaseTransition', 'ETN': 'String', 'ET': 'CWEType'},
265                                        {'X': 'Bookmark', 'ETN': 'String', 'ET': 'CWEType'},
266                                        {'X': 'Card', 'ETN': 'String', 'ET': 'CWEType'},
267                                        {'X': 'Comment', 'ETN': 'String', 'ET': 'CWEType'},
@@ -194,10 +194,11 @@
268                                        {'X': 'EmailPart', 'ETN': 'String', 'ET': 'CWEType'},
269                                        {'X': 'EmailThread', 'ETN': 'String', 'ET': 'CWEType'},
270                                        {'X': 'ExternalUri', 'ETN': 'String', 'ET': 'CWEType'},
271                                        {'X': 'File', 'ETN': 'String', 'ET': 'CWEType'},
272                                        {'X': 'Folder', 'ETN': 'String', 'ET': 'CWEType'},
273 +                                      {'X': 'Frozable', 'ETN': 'String', 'ET': 'CWEType'},
274                                        {'X': 'Note', 'ETN': 'String', 'ET': 'CWEType'},
275                                        {'X': 'Old', 'ETN': 'String', 'ET': 'CWEType'},
276                                        {'X': 'Personne', 'ETN': 'String', 'ET': 'CWEType'},
277                                        {'X': 'RQLExpression', 'ETN': 'String', 'ET': 'CWEType'},
278                                        {'X': 'Societe', 'ETN': 'String', 'ET': 'CWEType'},
@@ -574,20 +575,20 @@
279                              'WHERE RT name N, RDEF relation_type RT '
280                              'HAVING COUNT(RDEF) > 10')
281          self.assertListEqual(rset.rows,
282                                [[u'description_format', 12],
283                                 [u'description', 13],
284 -                               [u'name', 17],
285 -                               [u'created_by', 43],
286 -                               [u'creation_date', 43],
287 -                               [u'cw_source', 43],
288 -                               [u'cwuri', 43],
289 -                               [u'in_basket', 43],
290 -                               [u'is', 43],
291 -                               [u'is_instance_of', 43],
292 -                               [u'modification_date', 43],
293 -                               [u'owned_by', 43]])
294 +                               [u'name', 18],
295 +                               [u'created_by', 44],
296 +                               [u'creation_date', 44],
297 +                               [u'cw_source', 44],
298 +                               [u'cwuri', 44],
299 +                               [u'in_basket', 44],
300 +                               [u'is', 44],
301 +                               [u'is_instance_of', 44],
302 +                               [u'modification_date', 44],
303 +                               [u'owned_by', 44]])
304 
305      def test_select_aggregat_having_dumb(self):
306          # dumb but should not raise an error
307          rset = self.execute('Any U,COUNT(X) GROUPBY U '
308                              'WHERE U eid %(x)s, X owned_by U '
diff --git a/server/test/unittest_security.py b/server/test/unittest_security.py
@@ -1,6 +1,6 @@
309 -# copyright 2003-2012 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
310 +# copyright 2003-2014 LOGILAB S.A. (Paris, FRANCE), all rights reserved.
311  # contact http://www.logilab.fr/ -- mailto:contact@logilab.fr
312  #
313  # This file is part of CubicWeb.
314  #
315  # CubicWeb is free software: you can redistribute it and/or modify it under the
@@ -388,10 +388,26 @@
316              self.assertRaises(Unauthorized, self.commit)
317              cu.execute('SET X test TRUE WHERE X eid %(x)s', {'x': eid})
318              self.assertRaises(Unauthorized, self.commit)
319              cu.execute('SET X web "http://www.logilab.org" WHERE X eid %(x)s', {'x': eid})
320              self.commit()
321 +        with self.login('iaminusersgrouponly') as cu:
322 +            eid = cu.execute('INSERT Frozable F: F name "Foo"')
323 +            self.commit()
324 +            cu.execute('SET F name "Bar" WHERE F is Frozable')
325 +            self.commit()
326 +            cu.execute('SET F name "BaBar" WHERE F is Frozable')
327 +            cu.execute('SET F frozen True WHERE F is Frozable')
328 +            with self.assertRaises(Unauthorized):
329 +                self.commit()
330 +            self.rollback()
331 +            cu.execute('SET F frozen True WHERE F is Frozable')
332 +            self.commit()
333 +            cu.execute('SET F name "Bar" WHERE F is Frozable')
334 +            with self.assertRaises(Unauthorized):
335 +                self.commit()
336 +            self.rollback()
337 
338      def test_attribute_security_rqlexpr(self):
339          # Note.para attribute editable by managers or if the note is in "todo" state
340          note = self.execute("INSERT Note X: X para 'bidule'").get_entity(0, 0)
341          self.commit()