Skip to content

Commit 99065f4

Browse files
committed
Merge permission-performance branch
This fixes issue2551330: Add an optional 'filter' function to the Permission objects and the addPermission method. This is used to optimize search performance by not checking items returned from a database query one-by-one (using the check function) but instead offload the permission checks to the database. For SQL backends this performs the filtering in the database.
2 parents 28a7dc3 + d485ecc commit 99065f4

19 files changed

+574
-134
lines changed

CHANGES.txt

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ Features:
3939
- issue2551315 - Document use of
4040
RestfulInstance.max_response_row_size to limit data returned
4141
from rest request.
42+
- issue2551330 Add an optional 'filter' function to the Permission
43+
objects and the addPermission method. This is used to optimize search
44+
performance by not checking items returned from a database query
45+
one-by-one (using the check function) but instead offload the
46+
permission checks to the database. For SQL backends this performs the
47+
filtering in the database. (Ralf Schlatterbeck)
4248

4349
2024-07-13 2.4.0
4450

doc/design.txt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1524,6 +1524,7 @@ The security module defines::
15241524
- klass (optional)
15251525
- properties (optional)
15261526
- check function (optional)
1527+
- filter function (optional)
15271528

15281529
The klass may be unset, indicating that this permission is
15291530
not locked to a particular hyperdb class. There may be
@@ -1536,6 +1537,16 @@ The security module defines::
15361537
If check function is set, permission is granted only when
15371538
the function returns value interpreted as boolean true.
15381539
The function is called with arguments db, userid, itemid.
1540+
1541+
A filter function complements a check function: It is used
1542+
when searching for viewable items. The filter function
1543+
allows to filter in SQL rather than calling the check
1544+
function for each item after a query. It must return a list
1545+
of dictionaries containing parameters for the hyperdb.Class.filter
1546+
method. An empty list indicates no access. The signature of
1547+
the filter function is::
1548+
1549+
def filter(db, userid, klass):
15391550
'''
15401551

15411552
class Role:

doc/reference.txt

Lines changed: 93 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1484,7 +1484,7 @@ When adding a new Permission, you need to:
14841484
4. check it in the appropriate hasPermission methods in your tracker's
14851485
extensions/detectors/interfaces.py modules
14861486

1487-
The ``addPermission`` method takes a three optional parameters:
1487+
The ``addPermission`` method takes a four optional parameters:
14881488

14891489
**check**
14901490
A function to be executed which returns boolean determining whether
@@ -1520,6 +1520,98 @@ The ``addPermission`` method takes a three optional parameters:
15201520
permission. When searching there is no item defined, so a check
15211521
function does not make any sense.
15221522

1523+
**filter**
1524+
This optional function returns parameters for the ``filter`` method
1525+
when getting ``Class`` items (users, issues etc.) from the
1526+
database. It filters items at the database level (using SQL where
1527+
possible). This pre-filters the number of items returned from the
1528+
database when displaying results in an ``index`` template. This
1529+
filtering is usually faster than calling a ``check`` method (see
1530+
previous argument) on *each individual result*.
1531+
1532+
The ``filter`` method has the signature::
1533+
1534+
filter(db, userid, klass)
1535+
1536+
where ``db`` is the database handle, ``userid`` is the user attempting
1537+
access and ``klass`` is the ``Class`` in the schema.
1538+
1539+
The ``filter`` function must return a *list* of dictionaries of
1540+
parameters of the
1541+
`Class.filter <design.html#:~:text=search_matches>`_ call.
1542+
This includes filterspec, retired and exact_match_specs.
1543+
Note that sort and group parameters of the filter call should
1544+
not be set by filter method (they will be overwritten) and the
1545+
parameter search_matches must not be set.
1546+
1547+
The query executed by an index template is modified by the
1548+
parameters computed by the ``filter`` function. An
1549+
empty list of filter parameters (``[]``) indicates no access. When
1550+
using a filter, a check function is still needed to test each
1551+
individual item for visibility. When the filter function is defined
1552+
but a check function is not defined, a check function is
1553+
manufactured automatically from the ``filter`` function.
1554+
1555+
Note that the filter option is not supported for the Search
1556+
permission. Since the filter function is called *after* the search was
1557+
already performed a filter function does not make any sense.
1558+
1559+
An example ``filter`` function for the ``view_query`` check function
1560+
in the query checks above would look like::
1561+
1562+
def filter_query(db, userid, klass):
1563+
return [{'filterspec': {
1564+
'private_for': ['-1', userid]
1565+
}}]
1566+
1567+
This would be called by the framework for all queries found when
1568+
displaying queries. It filters for all queries where the
1569+
``private_for`` field is the userid or empty. This matches the
1570+
definition of the ``view_query`` function above where permission is
1571+
granted if the ``private_for`` field indicates the query is owned by
1572+
the user, or the ``private_for`` field is empty indicating that the
1573+
query is public. If we want to modify the check to also allow acess if
1574+
the user is the ``creator`` of a query we would change the filter
1575+
function to::
1576+
1577+
def filter_query(db, userid, klass):
1578+
f1 = {'filterspec': {'private_for': ['-1', userid]}}
1579+
f2 = {'filterspec': {'creator': userid}}
1580+
return [f1, f2]
1581+
1582+
This is an example where we need multiple filter calls to model an
1583+
"or" condition, the user has access if either the ``private_for``
1584+
check passes *or* the user is the creator of the query.
1585+
1586+
Consider an example where we have a class structure where both the
1587+
``issue`` class and the ``user`` class include a reference to an
1588+
``organization`` class. Users are permitted to view only those
1589+
issues that are associated with their respective organizations. A
1590+
check function or this could look like::
1591+
1592+
def view_issue(db, userid, itemid):
1593+
user = db.user.getnode(userid)
1594+
if not user.organisation:
1595+
return False
1596+
issue = db.issue.getnode(itemid)
1597+
if user.organisation == issue.organisation:
1598+
return True
1599+
1600+
The corresponding ``filter`` function::
1601+
1602+
def filter_issue(db, userid, klass):
1603+
user = db.user.getnode(userid)
1604+
if not user.organisation:
1605+
return []
1606+
return [{'filterspec': {
1607+
'organisation': user.organisation
1608+
}}]
1609+
1610+
This filters for all issues where the organisation is the same as the
1611+
organisation of the user. Note how the filter fails early returning an
1612+
empty list (meaning "no access") if the user happens to not have an
1613+
organisation.
1614+
15231615
**properties**
15241616
A sequence of property names that are the only properties to apply the
15251617
new Permission to (eg. ``... klass='user', properties=('name',

roundup/admin.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1920,7 +1920,7 @@ def do_security(self, args):
19201920
roles.sort()
19211921
for _rolename, role in roles:
19221922
sys.stdout.write(_('Role "%(name)s":\n') % role.__dict__)
1923-
for permission in role.permissions:
1923+
for permission in role.permission_list():
19241924
d = permission.__dict__
19251925
if permission.klass:
19261926
if permission.properties:

roundup/backends/back_anydbm.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,11 +408,13 @@ def getnode(self, classname, nodeid, db=None, cache=1):
408408
if db is None:
409409
db = self.getclassdb(classname)
410410
if nodeid not in db:
411+
db.close()
411412
raise IndexError("no such %s %s" % (classname, nodeid))
412413

413414
# check the uncommitted, destroyed nodes
414415
if (classname in self.destroyednodes and
415416
nodeid in self.destroyednodes[classname]):
417+
db.close()
416418
raise IndexError("no such %s %s" % (classname, nodeid))
417419

418420
# decode

roundup/cgi/actions.py

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1706,13 +1706,9 @@ def fct(arg):
17061706
# generate the CSV output
17071707
self.client._socket_op(writer.writerow, columns)
17081708
# and search
1709-
for itemid in klass.filter(matches, filterspec, sort, group):
1709+
filter = klass.filter_with_permissions
1710+
for itemid in filter(matches, filterspec, sort, group):
17101711
row = []
1711-
# don't put out a row of [hidden] fields if the user has
1712-
# no access to the issue.
1713-
if not self.hasPermission(self.permissionType, itemid=itemid,
1714-
classname=request.classname):
1715-
continue
17161712
for name in columns:
17171713
# check permission for this property on this item
17181714
# TODO: Permission filter doesn't work for the 'user' class
@@ -1807,15 +1803,9 @@ def handle(self):
18071803
self.client._socket_op(writer.writerow, columns)
18081804

18091805
# and search
1810-
for itemid in klass.filter(matches, filterspec, sort, group):
1806+
filter = klass.filter_with_permissions
1807+
for itemid in filter(matches, filterspec, sort, group):
18111808
row = []
1812-
# FIXME should this code raise an exception if an item
1813-
# is included that can't be accessed? Enabling this
1814-
# check will just skip the row for the inaccessible item.
1815-
# This makes it act more like the web interface.
1816-
# if not self.hasPermission(self.permissionType, itemid=itemid,
1817-
# classname=request.classname):
1818-
# continue
18191809
for name in columns:
18201810
# check permission to view this property on this item
18211811
if not self.hasPermission(self.permissionType, itemid=itemid,

roundup/cgi/templating.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3427,7 +3427,7 @@ def batch(self, permission='View'):
34273427
return Batch(self.client, [], self.pagesize, self.startwith,
34283428
classname=self.classname)
34293429

3430-
filterspec = self.filterspec
3430+
fspec = self.filterspec
34313431
sort = self.sort
34323432
group = self.group
34333433

@@ -3453,9 +3453,9 @@ def batch(self, permission='View'):
34533453
matches = None
34543454

34553455
# filter for visibility
3456-
allowed = [itemid for itemid in klass.filter(matches, filterspec,
3457-
sort, group)
3458-
if check(permission, userid, self.classname, itemid=itemid)]
3456+
allowed = klass.filter_with_permissions(
3457+
matches, fspec, sort, group, permission=permission, userid=userid
3458+
)
34593459

34603460
# return the batch object, using IDs only
34613461
return Batch(self.client, allowed, self.pagesize, self.startwith,

roundup/configuration.py

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1462,6 +1462,17 @@ def str2value(self, value):
14621462
("rdbms", (
14631463
(DatabaseBackend, 'backend', NODEFAULT,
14641464
"Database backend."),
1465+
(BooleanOption, "debug_filter", "no",
1466+
"Filter debugging: Permissions can define additional filter\n"
1467+
"functions that are used when checking permissions on results\n"
1468+
"returned by the database. This is done to improve\n"
1469+
"performance since the filtering is done in the database\n"
1470+
"backend, not in python (at least for the SQL backends). The\n"
1471+
"user is responsible for making the filter return the same\n"
1472+
"set of results as the check function for a permission. So it\n"
1473+
"makes sense to aid in debugging (and performance\n"
1474+
"measurements) to allow turning off the usage of filter\n"
1475+
"functions using only the check functions."),
14651476
(Option, 'name', 'roundup',
14661477
"Name of the database to use. For Postgresql, this can\n"
14671478
"be database.schema to use a specific schema within\n"
@@ -1545,8 +1556,8 @@ def str2value(self, value):
15451556
"Set the database cursor for filter queries to serverside\n"
15461557
"cursor, this avoids caching large amounts of data in the\n"
15471558
"client. This option only applies for the postgresql backend."),
1548-
), "Settings in this section (except for backend) are used\n"
1549-
" by RDBMS backends only.",
1559+
), "Most settings in this section (except for backend and debug_filter)\n"
1560+
"are used by RDBMS backends only.",
15501561
),
15511562
("sessiondb", (
15521563
(SessiondbBackendOption, "backend", "",

roundup/hyperdb.py

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1796,6 +1796,60 @@ def filter(self, search_matches, filterspec, sort=[], group=[],
17961796
# anyway).
17971797
filter_iter = filter
17981798

1799+
def filter_with_permissions(self, search_matches, filterspec, sort=[],
1800+
group=[], retired=False, exact_match_spec={},
1801+
limit=None, offset=None,
1802+
permission='View', userid=None):
1803+
""" Do the same as filter but return only the items the user is
1804+
entitled to see, running the results through security checks.
1805+
The userid defaults to the current database user.
1806+
"""
1807+
if userid is None:
1808+
userid = self.db.getuid()
1809+
cn = self.classname
1810+
sec = self.db.security
1811+
filterspec = sec.filterFilterspec(userid, cn, filterspec)
1812+
if exact_match_spec:
1813+
exact_match_spec = sec.filterFilterspec(userid, cn,
1814+
exact_match_spec)
1815+
sort = sec.filterSortspec(userid, cn, sort)
1816+
group = sec.filterSortspec(userid, cn, group)
1817+
item_ids = self.filter(search_matches, filterspec, sort, group,
1818+
retired, exact_match_spec, limit, offset)
1819+
check = sec.hasPermission
1820+
if check(permission, userid, cn, skip_permissions_with_check = True):
1821+
allowed = item_ids
1822+
else:
1823+
debug = self.db.config.RDBMS_DEBUG_FILTER
1824+
# Note that is_filterable returns True if no permissions are
1825+
# found. This makes it fail early (with an empty allowed list)
1826+
# instead of running through all ids with an empty
1827+
# permission list.
1828+
if not debug and sec.is_filterable(permission, userid, cn):
1829+
new_ids = set(item_ids)
1830+
confirmed = set()
1831+
for perm in sec.filter_iter(permission, userid, cn):
1832+
fargs = perm.filter(self.db, userid, self)
1833+
for farg in fargs:
1834+
farg.update(sort=[], group=[], retired=None)
1835+
result = self.filter(list(new_ids), **farg)
1836+
new_ids.difference_update(result)
1837+
confirmed.update(result)
1838+
# all allowed?
1839+
if not new_ids:
1840+
break
1841+
# all allowed?
1842+
if not new_ids:
1843+
break
1844+
# Need to sort again in database
1845+
allowed = self.filter(confirmed, {}, sort=sort, group=group,
1846+
retired=None)
1847+
else: # Last resort: filter in python
1848+
allowed = [id for id in item_ids
1849+
if check(permission, userid, cn, itemid=id)]
1850+
return allowed
1851+
1852+
17991853
def count(self):
18001854
"""Get the number of nodes in this class.
18011855

roundup/rest.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -953,7 +953,7 @@ def get_collection(self, class_name, input):
953953
kw['limit'] = self.max_response_row_size
954954
if page['index'] is not None and page['index'] > 1:
955955
kw['offset'] = (page['index'] - 1) * page['size']
956-
obj_list = class_obj.filter(None, *l, **kw)
956+
obj_list = class_obj.filter_with_permissions(None, *l, **kw)
957957

958958
# Have we hit the max number of returned rows?
959959
# If so there may be more data that the client
@@ -973,10 +973,9 @@ def get_collection(self, class_name, input):
973973
result['collection'] = []
974974
for item_id in obj_list:
975975
r = {}
976-
if self.db.security.hasPermission(
977-
'View', uid, class_name, itemid=item_id, property='id',
978-
):
979-
r = {'id': item_id, 'link': class_path + item_id}
976+
# No need to check permission on id here, as we have only
977+
# security-checked results
978+
r = {'id': item_id, 'link': class_path + item_id}
980979
if display_props:
981980
# format_item does the permission checks
982981
r.update(self.format_item(class_obj.getnode(item_id),

0 commit comments

Comments
 (0)