Mercurial > p > roundup > code
changeset 4437:261c9f913ff7
- Add explicit "Search" permissions, see Security Fix below.
- Security Fix: Add a check for search-permissions: now we allow
searching for properties only if the property is readable without a
check method or if an explicit search permission (see above unter
"Features) is given for the property. This fixes cases where a user
doesn't have access to a property but can deduce the content by
crafting a clever search, group or sort query.
see doc/upgrading.txt for how to fix your trackers!
| author | Ralf Schlatterbeck <schlatterbeck@users.sourceforge.net> |
|---|---|
| date | Tue, 19 Oct 2010 15:29:05 +0000 |
| parents | 528ace81fd16 |
| children | 222efa59ee6c |
| files | CHANGES.txt doc/upgrading.txt roundup/cgi/templating.py roundup/security.py roundup/xmlrpc.py share/roundup/templates/classic/schema.py share/roundup/templates/devel/schema.py test/test_cgi.py test/test_xmlrpc.py |
| diffstat | 9 files changed, 353 insertions(+), 9 deletions(-) [+] |
line wrap: on
line diff
--- a/CHANGES.txt Tue Oct 19 00:41:29 2010 +0000 +++ b/CHANGES.txt Tue Oct 19 15:29:05 2010 +0000 @@ -2,10 +2,22 @@ are given with the most recent entry first. If no other name is given, Richard Jones did the change. -20X0-XX-XX +20XX-XX-XX 1.4.17 (rXXXX) + +Features: + +- Add explicit "Search" permissions, see Security Fix below. Fixed: + - Some minor typos fixed in doc/customizing.txt (Thanks Ralf Hemmecke). +- Security Fix: Add a check for search-permissions: now we allow + searching for properties only if the property is readable without a + check method or if an explicit search permission (see above unter + "Features) is given for the property. This fixes cases where a user + doesn't have access to a property but can deduce the content by + crafting a clever search, group or sort query. + see doc/upgrading.txt for how to fix your trackers! 2010-10-08 1.4.16 (r4541)
--- a/doc/upgrading.txt Tue Oct 19 00:41:29 2010 +0000 +++ b/doc/upgrading.txt Tue Oct 19 15:29:05 2010 +0000 @@ -13,6 +13,45 @@ .. contents:: +Migrating from 1.4.x to 1.4.17 +============================== + +Searching now requires either read-permission without a check method, or +you will have to add a "Search" permission for a class or a list of +properties for a class (if you want to allow searching). For the classic +template (or other templates derived from it) you want to add the +following lines to your `schema.py` file:: + + p = db.security.addPermission(name='Search', klass='query') + db.security.addPermissionToRole('User', p) + +This is needed, because for the `query` class users may view only their +own queries (or public queries). This is implemented with a `check` +method, therefore the default search permissions will not allow +searching and you'll have to add an explicit search permission. +If you have modified your schema, you can check if you're missing any +search permissions with the following script, run it in your tracker +directory, it will list for each Class and Property the roles that may +search for this property:: + + #!/usr/bin/python + import os + from roundup import instance + + tracker = instance.open(os.getcwd ()) + db = tracker.open('admin') + + for cl in sorted(db.getclasses()): + print "Class:", cl + for p in sorted(db.getclass(cl).properties.keys()): + print " Property:", p + roles = [] + for role in sorted(db.security.role.iterkeys()): + if db.security.roleHasSearchPermission(role,cl,p): + roles.append(role) + print " roles may search:", ', '.join(roles) + + Migrating from 1.4.x to 1.4.12 ==============================
--- a/roundup/cgi/templating.py Tue Oct 19 00:41:29 2010 +0000 +++ b/roundup/cgi/templating.py Tue Oct 19 15:29:05 2010 +0000 @@ -673,13 +673,21 @@ "request" takes precedence over the other three arguments. """ + security = self._db.security + userid = self._client.userid if request is not None: + # for a request we asume it has already been + # security-filtered filterspec = request.filterspec sort = request.sort group = request.group - - check = self._db.security.hasPermission - userid = self._client.userid + else: + cn = self.classname + filterspec = security.filterFilterspec(userid, cn, filterspec) + sort = security.filterSortspec(userid, cn, sort) + group = security.filterSortspec(userid, cn, group) + + check = security.hasPermission if not check('Web Access', userid): return [] @@ -2446,12 +2454,16 @@ self.columns = handleListCGIValue(self.form[name]) break self.show = support.TruthDict(self.columns) + security = self._client.db.security + userid = self._client.userid # sorting and grouping self.sort = [] self.group = [] self._parse_sort(self.sort, 'sort') self._parse_sort(self.group, 'group') + self.sort = security.filterSortspec(userid, self.classname, self.sort) + self.group = security.filterSortspec(userid, self.classname, self.group) # filtering self.filter = [] @@ -2481,6 +2493,8 @@ self.filterspec[name] = handleListCGIValue(fv) else: self.filterspec[name] = fv.value + self.filterspec = security.filterFilterspec(userid, self.classname, + self.filterspec) # full-text search argument self.search_text = None
--- a/roundup/security.py Tue Oct 19 00:41:29 2010 +0000 +++ b/roundup/security.py Tue Oct 19 15:29:05 2010 +0000 @@ -54,6 +54,28 @@ # we have a winner return 1 + def searchable(self, db, permission, classname, property): + """ A Permission is searchable for the given permission if it + doesn't include a check method and otherwise matches the + given parameters. + """ + if permission != self.name: + return 0 + + # are we checking the correct class + if self.klass != classname: + return 0 + + # what about property? + if not self._properties_dict[property]: + return 0 + + if self.check: + return 0 + + return 1 + + def __repr__(self): return '<Permission 0x%x %r,%r,%r,%r>'%(id(self), self.name, self.klass, self.properties, self.check) @@ -175,6 +197,44 @@ return 1 return 0 + def roleHasSearchPermission(self, rolename, classname, property): + """ for each of the user's Roles, check the permissions + """ + for perm in self.role[rolename].permissions: + # permission match? + for p in 'View', 'Search': + if perm.searchable(self.db, p, classname, property): + return 1 + return 0 + + def hasSearchPermission(self, userid, classname, property): + '''Look through all the Roles, and hence Permissions, and + see if "permission" exists given the constraints of + classname and property. + + A search permission is granted if we find a 'View' or + 'Search' permission for the user which does *not* include + a check function. If such a permission is found, the user may + search for the given property in the given class. + + Note that classname *and* property are mandatory arguments. + + Contrary to hasPermission, the search will *not* match if + there are additional constraints (namely a search function) + on a Permission found. + + Concerning property, the Permission matched must have + either no properties listed or the property must appear in + the list. + ''' + for rolename in self.db.user.get_roles(userid): + if not rolename or not self.role.has_key(rolename): + continue + # for each of the user's Roles, check the permissions + if self.roleHasSearchPermission (rolename, classname, property): + return 1 + return 0 + def addPermission(self, **propspec): ''' Create a new Permission with the properties defined in 'propspec'. See the Permission class for the possible @@ -208,4 +268,22 @@ role = self.role[rolename.lower()] role.permissions.append(permission) + # Convenience methods for removing non-allowed properties from a + # filterspec or sort/group list + + def filterFilterspec(self, userid, classname, filterspec): + """ Return a filterspec that has all non-allowed properties removed. + """ + return dict ([(k, v) for k, v in filterspec.iteritems() + if self.hasSearchPermission(userid,classname,k)]) + + def filterSortspec(self, userid, classname, sort): + """ Return a sort- or group-list that has all non-allowed properties + removed. + """ + if isinstance(sort, tuple) and sort[0] in '+-': + sort = [sort] + return [(d, p) for d, p in sort + if self.hasSearchPermission(userid,classname,p)] + # vim: set filetype=python sts=4 sw=4 et si :
--- a/roundup/xmlrpc.py Tue Oct 19 00:41:29 2010 +0000 +++ b/roundup/xmlrpc.py Tue Oct 19 15:29:05 2010 +0000 @@ -89,8 +89,15 @@ def filter(self, classname, search_matches, filterspec, sort=[], group=[]): cl = self.db.getclass(classname) + uid = self.db.getuid() + security = self.db.security + filterspec = security.filterFilterspec (uid, classname, filterspec) + sort = security.filterSortspec (uid, classname, sort) + group = security.filterSortspec (uid, classname, group) result = cl.filter(search_matches, filterspec, sort=sort, group=group) - return result + check = security.hasPermission + x = [id for id in result if check('View', uid, classname, itemid=id)] + return x def display(self, designator, *properties): classname, itemid = hyperdb.splitDesignator(designator)
--- a/share/roundup/templates/classic/schema.py Tue Oct 19 00:41:29 2010 +0000 +++ b/share/roundup/templates/classic/schema.py Tue Oct 19 15:29:05 2010 +0000 @@ -129,6 +129,8 @@ p = db.security.addPermission(name='View', klass='query', check=view_query, description="User is allowed to view their own and public queries") db.security.addPermissionToRole('User', p) +p = db.security.addPermission(name='Search', klass='query') +db.security.addPermissionToRole('User', p) p = db.security.addPermission(name='Edit', klass='query', check=edit_query, description="User is allowed to edit their queries") db.security.addPermissionToRole('User', p)
--- a/share/roundup/templates/devel/schema.py Tue Oct 19 00:41:29 2010 +0000 +++ b/share/roundup/templates/devel/schema.py Tue Oct 19 15:29:05 2010 +0000 @@ -327,6 +327,8 @@ return userid == db.query.get(itemid, 'creator') p = db.security.addPermission(name='View', klass='query', check=view_query, description="User is allowed to view their own and public queries") +p = db.security.addPermission(name='Search', klass='query') +db.security.addPermissionToRole('User', p) for r in 'User', 'Developer', 'Coordinator': db.security.addPermissionToRole(r, p) p = db.security.addPermission(name='Edit', klass='query', check=edit_query,
--- a/test/test_cgi.py Tue Oct 19 00:41:29 2010 +0000 +++ b/test/test_cgi.py Tue Oct 19 15:29:05 2010 +0000 @@ -14,7 +14,7 @@ from roundup.cgi import client, actions, exceptions from roundup.cgi.exceptions import FormError -from roundup.cgi.templating import HTMLItem +from roundup.cgi.templating import HTMLItem, HTMLRequest from roundup.cgi.form_parser import FormParser from roundup import init, instance, password, hyperdb, date @@ -616,17 +616,18 @@ # SECURITY # # XXX test all default permissions - def _make_client(self, form, classname='user', nodeid='1', userid='2'): + def _make_client(self, form, classname='user', nodeid='1', + userid='2', template='item'): cl = client.Client(self.instance, None, {'PATH_INFO':'/', 'REQUEST_METHOD':'POST'}, makeForm(form)) - cl.classname = 'user' + cl.classname = classname if nodeid is not None: cl.nodeid = nodeid cl.db = self.db cl.userid = userid cl.language = ('en',) cl.error_message = [] - cl.template = 'item' + cl.template = template return cl def testClassPermission(self): @@ -724,6 +725,117 @@ self.assertRaises(exceptions.Unauthorised, actions.EditItemAction(cl).handle) + def testSearchPermission(self): + # this checks if we properly check for search permissions + self.db.security.permissions = {} + self.db.security.addRole(name='User') + self.db.security.addRole(name='Project') + self.db.security.addPermissionToRole('User', 'Web Access') + self.db.security.addPermissionToRole('Project', 'Web Access') + # Allow viewing department + p = self.db.security.addPermission(name='View', klass='department') + self.db.security.addPermissionToRole('User', p) + # Allow viewing interesting things (but not department) on iss + # But users might only view issues where they are on nosy + # (so in the real world the check method would be better) + p = self.db.security.addPermission(name='View', klass='iss', + properties=("title", "status"), check=lambda x,y,z: True) + self.db.security.addPermissionToRole('User', p) + # Allow role "Project" access to whole iss + p = self.db.security.addPermission(name='View', klass='iss') + self.db.security.addPermissionToRole('Project', p) + + department = self.instance.backend.Class(self.db, "department", + name=hyperdb.String()) + status = self.instance.backend.Class(self.db, "stat", + name=hyperdb.String()) + issue = self.instance.backend.Class(self.db, "iss", + title=hyperdb.String(), status=hyperdb.Link('stat'), + department=hyperdb.Link('department')) + + d1 = department.create(name='d1') + d2 = department.create(name='d2') + open = status.create(name='open') + closed = status.create(name='closed') + issue.create(title='i1', status=open, department=d2) + issue.create(title='i2', status=open, department=d1) + issue.create(title='i2', status=closed, department=d1) + + chef = self.db.user.lookup('Chef') + mary = self.db.user.lookup('mary') + self.db.user.set(chef, roles = 'User, Project') + + perm = self.db.security.hasPermission + search = self.db.security.hasSearchPermission + self.assert_(perm('View', chef, 'iss', 'department', '1')) + self.assert_(perm('View', chef, 'iss', 'department', '2')) + self.assert_(perm('View', chef, 'iss', 'department', '3')) + self.assert_(search(chef, 'iss', 'department')) + + self.assert_(not perm('View', mary, 'iss', 'department')) + self.assert_(perm('View', mary, 'iss', 'status')) + # Conditionally allow view of whole iss (check is False here, + # this might check for department owner in the real world) + p = self.db.security.addPermission(name='View', klass='iss', + check=lambda x,y,z: False) + self.db.security.addPermissionToRole('User', p) + self.assert_(perm('View', mary, 'iss', 'department')) + self.assert_(not perm('View', mary, 'iss', 'department', '1')) + self.assert_(not search(mary, 'iss', 'department')) + + self.assert_(perm('View', mary, 'iss', 'status')) + self.assert_(not search(mary, 'iss', 'status')) + # Allow user to search for iss.status + p = self.db.security.addPermission(name='Search', klass='iss', + properties=("status",)) + self.db.security.addPermissionToRole('User', p) + self.assert_(search(mary, 'iss', 'status')) + + dep = {'@action':'search','columns':'id','@filter':'department', + 'department':'1'} + stat = {'@action':'search','columns':'id','@filter':'status', + 'status':'1'} + depsort = {'@action':'search','columns':'id','@sort':'department'} + depgrp = {'@action':'search','columns':'id','@group':'department'} + + # Filter on department ignored for role 'User': + cl = self._make_client(dep, classname='iss', nodeid=None, userid=mary, + template='index') + h = HTMLRequest(cl) + self.assertEqual([x.id for x in h.batch()],['1', '2', '3']) + # Filter on department works for role 'Project': + cl = self._make_client(dep, classname='iss', nodeid=None, userid=chef, + template='index') + h = HTMLRequest(cl) + self.assertEqual([x.id for x in h.batch()],['2', '3']) + # Filter on status works for all: + cl = self._make_client(stat, classname='iss', nodeid=None, userid=mary, + template='index') + h = HTMLRequest(cl) + self.assertEqual([x.id for x in h.batch()],['1', '2']) + cl = self._make_client(stat, classname='iss', nodeid=None, userid=chef, + template='index') + h = HTMLRequest(cl) + self.assertEqual([x.id for x in h.batch()],['1', '2']) + # Sorting and grouping for class Project works: + cl = self._make_client(depsort, classname='iss', nodeid=None, + userid=chef, template='index') + h = HTMLRequest(cl) + self.assertEqual([x.id for x in h.batch()],['2', '3', '1']) + cl = self._make_client(depgrp, classname='iss', nodeid=None, + userid=chef, template='index') + h = HTMLRequest(cl) + self.assertEqual([x.id for x in h.batch()],['2', '3', '1']) + # Sorting and grouping for class User fails: + cl = self._make_client(depsort, classname='iss', nodeid=None, + userid=mary, template='index') + h = HTMLRequest(cl) + self.assertEqual([x.id for x in h.batch()],['1', '2', '3']) + cl = self._make_client(depgrp, classname='iss', nodeid=None, + userid=mary, template='index') + h = HTMLRequest(cl) + self.assertEqual([x.id for x in h.batch()],['1', '2', '3']) + def testRoles(self): cl = self._make_client({}) self.db.user.set('1', roles='aDmin, uSer')
--- a/test/test_xmlrpc.py Tue Oct 19 00:41:29 2010 +0000 +++ b/test/test_xmlrpc.py Tue Oct 19 15:29:05 2010 +0000 @@ -115,6 +115,84 @@ finally: self.db.setCurrentUser('joe') + def testAuthFilter(self): + # this checks if we properly check for search permissions + self.db.security.permissions = {} + self.db.security.addRole(name='User') + self.db.security.addRole(name='Project') + self.db.security.addPermissionToRole('User', 'Web Access') + self.db.security.addPermissionToRole('Project', 'Web Access') + # Allow viewing keyword + p = self.db.security.addPermission(name='View', klass='keyword') + self.db.security.addPermissionToRole('User', p) + # Allow viewing interesting things (but not keyword) on issue + # But users might only view issues where they are on nosy + # (so in the real world the check method would be better) + p = self.db.security.addPermission(name='View', klass='issue', + properties=("title", "status"), check=lambda x,y,z: True) + self.db.security.addPermissionToRole('User', p) + # Allow role "Project" access to whole issue + p = self.db.security.addPermission(name='View', klass='issue') + self.db.security.addPermissionToRole('Project', p) + + keyword = self.db.keyword + status = self.db.status + issue = self.db.issue + + d1 = keyword.create(name='d1') + d2 = keyword.create(name='d2') + open = status.create(name='open') + closed = status.create(name='closed') + issue.create(title='i1', status=open, keyword=[d2]) + issue.create(title='i2', status=open, keyword=[d1]) + issue.create(title='i2', status=closed, keyword=[d1]) + + chef = self.db.user.create(username = 'chef', roles='User, Project') + joe = self.db.user.lookup('joe') + + # Conditionally allow view of whole issue (check is False here, + # this might check for keyword owner in the real world) + p = self.db.security.addPermission(name='View', klass='issue', + check=lambda x,y,z: False) + self.db.security.addPermissionToRole('User', p) + # Allow user to search for issue.status + p = self.db.security.addPermission(name='Search', klass='issue', + properties=("status",)) + self.db.security.addPermissionToRole('User', p) + + keyw = {'keyword':self.db.keyword.lookup('d1')} + stat = {'status':self.db.status.lookup('open')} + keygroup = keysort = [('+', 'keyword')] + self.db.commit() + + # Filter on keyword ignored for role 'User': + r = self.server.filter('issue', None, keyw) + self.assertEqual(r, ['1', '2', '3']) + # Filter on status works for all: + r = self.server.filter('issue', None, stat) + self.assertEqual(r, ['1', '2']) + # Sorting and grouping for class User fails: + r = self.server.filter('issue', None, {}, sort=keysort) + self.assertEqual(r, ['1', '2', '3']) + r = self.server.filter('issue', None, {}, group=keygroup) + self.assertEqual(r, ['1', '2', '3']) + + self.db.close() + self.db = self.instance.open('chef') + self.server = RoundupInstance(self.db, self.instance.actions, None) + + # Filter on keyword works for role 'Project': + r = self.server.filter('issue', None, keyw) + self.assertEqual(r, ['2', '3']) + # Filter on status works for all: + r = self.server.filter('issue', None, stat) + self.assertEqual(r, ['1', '2']) + # Sorting and grouping for class Project works: + r = self.server.filter('issue', None, {}, sort=keysort) + self.assertEqual(r, ['2', '3', '1']) + r = self.server.filter('issue', None, {}, group=keygroup) + self.assertEqual(r, ['2', '3', '1']) + def test_suite(): suite = unittest.TestSuite() for l in list_backends():