- 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

diff --git a/CHANGES.txt b/CHANGES.txt
index aa0b196dfc9611ffb3ecd08b0c60018f4da31f75..af4da31d8e0941fc93950cb9061b68adaef995d2 100644 (file)
--- 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 d13cd1017464d27bb8389474ba6cf53c6caa4c4d..acdef4697db074b08936759a463d4054d869f06e 100644 (file)
--- 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 2d8ea77b64ca40273cc99cfa8316644057c3d6b1..079305bfbfb95f271faf176a1f376ca4fa0f8b67 100644 (file)
--- 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 3dfa8bd442c13ec068ca90589a10e55fc9f5b9fa..9bfb0ecdd773777011f7ee4e7634a9e51745526a 100644 (file)
--- 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 '<Permission 0x%x %r,%r,%r,%r>'%(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 111a0b72442b87b849db159829dea112a1f919c2..9dda5f8aae55899b2f717baa7d8c0af6e704cff0 100644 (file)
--- 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 a0060a9586173edde5e0d35907233f7270dd9c06..af765a8650e2e8d0cd413ad5de41a9b2b13fa7ce 100644 (file)
--- 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 a994fb86938167a07480ff19e03be76d88bf7d4b..0a438ea1b41a4d1d0e72cccfb6008c19a41f7b95 100644 (file)
--- 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 d5ec2ff35dac891e7c3a29ae57cda9b439bccf2b..91878e9427582e539a544b9f79dd5bd2fd75e988 100644 (file)
--- 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 4eb7c412cab5f43017c6a17a649657e3f9214766..e88e291f45f29a3a9f5466572667cbb44b9d271c 100644 (file)
--- 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():