summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 5f84f70)
raw | patch | inline | side by side (parent: 5f84f70)
author | schlatterbeck <schlatterbeck@57a73879-2fb5-44c3-a270-3262357dd7e2> | |
Tue, 19 Oct 2010 15:29:05 +0000 (15:29 +0000) | ||
committer | schlatterbeck <schlatterbeck@57a73879-2fb5-44c3-a270-3262357dd7e2> | |
Tue, 19 Oct 2010 15:29:05 +0000 (15:29 +0000) |
- 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
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
diff --git a/CHANGES.txt b/CHANGES.txt
index aa0b196dfc9611ffb3ecd08b0c60018f4da31f75..af4da31d8e0941fc93950cb9061b68adaef995d2 100644 (file)
--- a/CHANGES.txt
+++ b/CHANGES.txt
are given with the most recent entry first. If no other name is given,
Richard Jones did the change.
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:
Fixed:
+
- Some minor typos fixed in doc/customizing.txt (Thanks Ralf Hemmecke).
- 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)
2010-10-08 1.4.16 (r4541)
diff --git a/doc/upgrading.txt b/doc/upgrading.txt
index d13cd1017464d27bb8389474ba6cf53c6caa4c4d..acdef4697db074b08936759a463d4054d869f06e 100644 (file)
--- a/doc/upgrading.txt
+++ b/doc/upgrading.txt
.. contents::
.. 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
==============================
Migrating from 1.4.x to 1.4.12
==============================
index 2d8ea77b64ca40273cc99cfa8316644057c3d6b1..079305bfbfb95f271faf176a1f376ca4fa0f8b67 100644 (file)
"request" takes precedence over the other three arguments.
"""
"request" takes precedence over the other three arguments.
"""
+ security = self._db.security
+ userid = self._client.userid
if request is not None:
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
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 []
if not check('Web Access', userid):
return []
self.columns = handleListCGIValue(self.form[name])
break
self.show = support.TruthDict(self.columns)
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')
# 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 = []
# filtering
self.filter = []
self.filterspec[name] = handleListCGIValue(fv)
else:
self.filterspec[name] = fv.value
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
# full-text search argument
self.search_text = None
diff --git a/roundup/security.py b/roundup/security.py
index 3dfa8bd442c13ec068ca90589a10e55fc9f5b9fa..9bfb0ecdd773777011f7ee4e7634a9e51745526a 100644 (file)
--- a/roundup/security.py
+++ b/roundup/security.py
# we have a winner
return 1
# 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)
def __repr__(self):
return '<Permission 0x%x %r,%r,%r,%r>'%(id(self), self.name,
self.klass, self.properties, self.check)
return 1
return 0
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
def addPermission(self, **propspec):
''' Create a new Permission with the properties defined in
'propspec'. See the Permission class for the possible
role = self.role[rolename.lower()]
role.permissions.append(permission)
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 :
# vim: set filetype=python sts=4 sw=4 et si :
diff --git a/roundup/xmlrpc.py b/roundup/xmlrpc.py
index 111a0b72442b87b849db159829dea112a1f919c2..9dda5f8aae55899b2f717baa7d8c0af6e704cff0 100644 (file)
--- a/roundup/xmlrpc.py
+++ b/roundup/xmlrpc.py
def filter(self, classname, search_matches, filterspec,
sort=[], group=[]):
cl = self.db.getclass(classname)
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)
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)
def display(self, designator, *properties):
classname, itemid = hyperdb.splitDesignator(designator)
index a0060a9586173edde5e0d35907233f7270dd9c06..af765a8650e2e8d0cd413ad5de41a9b2b13fa7ce 100644 (file)
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='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)
p = db.security.addPermission(name='Edit', klass='query', check=edit_query,
description="User is allowed to edit their queries")
db.security.addPermissionToRole('User', p)
index a994fb86938167a07480ff19e03be76d88bf7d4b..0a438ea1b41a4d1d0e72cccfb6008c19a41f7b95 100644 (file)
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")
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,
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 d5ec2ff35dac891e7c3a29ae57cda9b439bccf2b..91878e9427582e539a544b9f79dd5bd2fd75e988 100644 (file)
--- a/test/test_cgi.py
+++ b/test/test_cgi.py
from roundup.cgi import client, actions, exceptions
from roundup.cgi.exceptions import FormError
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
from roundup.cgi.form_parser import FormParser
from roundup import init, instance, password, hyperdb, date
# SECURITY
#
# XXX test all default permissions
# 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 = 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 = []
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):
return cl
def testClassPermission(self):
self.assertRaises(exceptions.Unauthorised,
actions.EditItemAction(cl).handle)
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')
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 4eb7c412cab5f43017c6a17a649657e3f9214766..e88e291f45f29a3a9f5466572667cbb44b9d271c 100644 (file)
--- a/test/test_xmlrpc.py
+++ b/test/test_xmlrpc.py
finally:
self.db.setCurrentUser('joe')
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():
def test_suite():
suite = unittest.TestSuite()
for l in list_backends():