From bdcb21234cae9e506c37cbe90bc52803d5cc7c4e Mon Sep 17 00:00:00 2001 From: schlatterbeck Date: Tue, 19 Oct 2010 15:29:05 +0000 Subject: [PATCH] - 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! git-svn-id: http://svn.roundup-tracker.org/svnroot/roundup/roundup/trunk@4546 57a73879-2fb5-44c3-a270-3262357dd7e2 --- CHANGES.txt | 14 ++- doc/upgrading.txt | 39 +++++++ roundup/cgi/templating.py | 18 +++- roundup/security.py | 78 ++++++++++++++ roundup/xmlrpc.py | 9 +- share/roundup/templates/classic/schema.py | 2 + share/roundup/templates/devel/schema.py | 2 + test/test_cgi.py | 120 +++++++++++++++++++++- test/test_xmlrpc.py | 78 ++++++++++++++ 9 files changed, 352 insertions(+), 8 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index aa0b196..af4da31 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -2,10 +2,22 @@ This file contains the changes to the Roundup system over time. The entries 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) diff --git a/doc/upgrading.txt b/doc/upgrading.txt index d13cd10..acdef46 100644 --- a/doc/upgrading.txt +++ b/doc/upgrading.txt @@ -13,6 +13,45 @@ steps. .. 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 ============================== diff --git a/roundup/cgi/templating.py b/roundup/cgi/templating.py index 2d8ea77..079305b 100644 --- a/roundup/cgi/templating.py +++ b/roundup/cgi/templating.py @@ -673,13 +673,21 @@ class HTMLClass(HTMLInputMixin, HTMLPermissions): "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 + else: + cn = self.classname + filterspec = security.filterFilterspec(userid, cn, filterspec) + sort = security.filterSortspec(userid, cn, sort) + group = security.filterSortspec(userid, cn, group) - check = self._db.security.hasPermission - userid = self._client.userid + check = security.hasPermission if not check('Web Access', userid): return [] @@ -2446,12 +2454,16 @@ class HTMLRequest(HTMLInputMixin): 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 @@ class HTMLRequest(HTMLInputMixin): 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 diff --git a/roundup/security.py b/roundup/security.py index 3dfa8bd..9bfb0ec 100644 --- a/roundup/security.py +++ b/roundup/security.py @@ -54,6 +54,28 @@ class Permission: # 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 ''%(id(self), self.name, self.klass, self.properties, self.check) @@ -175,6 +197,44 @@ class Security: 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 @@ class Security: 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 : diff --git a/roundup/xmlrpc.py b/roundup/xmlrpc.py index 111a0b7..9dda5f8 100644 --- a/roundup/xmlrpc.py +++ b/roundup/xmlrpc.py @@ -89,8 +89,15 @@ class RoundupInstance: 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) diff --git a/share/roundup/templates/classic/schema.py b/share/roundup/templates/classic/schema.py index a0060a9..af765a8 100644 --- a/share/roundup/templates/classic/schema.py +++ b/share/roundup/templates/classic/schema.py @@ -129,6 +129,8 @@ def edit_query(db, userid, itemid): 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) diff --git a/share/roundup/templates/devel/schema.py b/share/roundup/templates/devel/schema.py index a994fb8..0a438ea 100644 --- a/share/roundup/templates/devel/schema.py +++ b/share/roundup/templates/devel/schema.py @@ -327,6 +327,8 @@ def edit_query(db, userid, itemid): 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, diff --git a/test/test_cgi.py b/test/test_cgi.py index d5ec2ff..91878e9 100644 --- a/test/test_cgi.py +++ b/test/test_cgi.py @@ -14,7 +14,7 @@ import unittest, os, shutil, errno, sys, difflib, cgi, re, StringIO 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 @@ class FormTestCase(unittest.TestCase): # 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 @@ class FormTestCase(unittest.TestCase): 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') diff --git a/test/test_xmlrpc.py b/test/test_xmlrpc.py index 4eb7c41..e88e291 100644 --- a/test/test_xmlrpc.py +++ b/test/test_xmlrpc.py @@ -115,6 +115,84 @@ class TestCase(unittest.TestCase): 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(): -- 2.30.2