Skip to content

Commit

Permalink
- Add explicit "Search" permissions, see Security Fix below.
Browse files Browse the repository at this point in the history
- 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!
  • Loading branch information
Ralf Schlatterbeck committed Oct 19, 2010
1 parent 00ba6c0 commit b267853
Show file tree
Hide file tree
Showing 9 changed files with 352 additions and 8 deletions.
14 changes: 13 additions & 1 deletion CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
39 changes: 39 additions & 0 deletions doc/upgrading.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
==============================

Expand Down
18 changes: 16 additions & 2 deletions roundup/cgi/templating.py
Original file line number Diff line number Diff line change
Expand Up @@ -673,13 +673,21 @@ def filter(self, request=None, filterspec={}, sort=[], group=[]):
"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 []

Expand Down Expand Up @@ -2446,12 +2454,16 @@ def _post_init(self):
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 = []
Expand Down Expand Up @@ -2481,6 +2493,8 @@ def _post_init(self):
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
Expand Down
78 changes: 78 additions & 0 deletions roundup/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,28 @@ def test(self, db, permission, classname, property, userid, itemid):
# 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)
Expand Down Expand Up @@ -175,6 +197,44 @@ def hasPermission(self, permission, userid, classname=None,
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
Expand Down Expand Up @@ -208,4 +268,22 @@ def addPermissionToRole(self, rolename, permission, classname=None,
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 :
9 changes: 8 additions & 1 deletion roundup/xmlrpc.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,15 @@ def list(self, classname, propname=None):
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)
Expand Down
2 changes: 2 additions & 0 deletions share/roundup/templates/classic/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions share/roundup/templates/devel/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit b267853

Please sign in to comment.