Mercurial > p > roundup > code
changeset 8125:b358da7c89e5 permission-performance
Optimize filtering of search results
Now the Permission class constructor takes an optional argument
'filter'. Now if we do not find a permission on the whole class *and*
all permission objects on the current class with a check method also
have a filter method we can improve search performance by filtering in
the database (instead of in python).
| author | Ralf Schlatterbeck <rsc@runtux.com> |
|---|---|
| date | Mon, 21 Oct 2024 16:11:13 +0200 |
| parents | 2a4d0413bd20 |
| children | f7bd22bdef9d |
| files | roundup/cgi/templating.py roundup/security.py |
| diffstat | 2 files changed, 83 insertions(+), 5 deletions(-) [+] |
line wrap: on
line diff
--- a/roundup/cgi/templating.py Fri Oct 18 18:04:46 2024 +0200 +++ b/roundup/cgi/templating.py Mon Oct 21 16:11:13 2024 +0200 @@ -3421,7 +3421,8 @@ def batch(self, permission='View'): """ Return a batch object for results from the "current search" """ - check = self._client.db.security.hasPermission + sec = self._client.db.security + check = sec.hasPermission userid = self._client.userid if not check('Web Access', userid): return Batch(self.client, [], self.pagesize, self.startwith, @@ -3454,11 +3455,34 @@ # filter for visibility item_ids = klass.filter(matches, filterspec, sort, group) - if check(permission, userid, self.classname, only_no_check = True): + cn = self.classname + if check(permission, userid, cn, only_no_check = True): allowed = item_ids else: - allowed = [id for id in item_ids - if check(permission, userid, self.classname, itemid=id)] + # Note that is_filterable returns True if no permissions are + # found. This makes it fail early (with an empty allowed list) + # instead of running through all ids with an empty + # permission list. + if sec.is_filterable(permission, userid, cn): + new_ids = set(item_ids) + confirmed = set() + for perm in sec.filter_iter(permission, userid, cn): + fargs = perm.filter(self._client.db, userid, klass) + for farg in fargs: + farg.update(sort=sort, group=group, retired=False) + result = klass.filter(list(new_ids), **farg) + new_ids.difference_update(result) + confirmed.update(result) + # all allowed? + if not new_ids: + break + # all allowed? + if not new_ids: + break + allowed = list(confirmed) + else: + allowed = [id for id in item_ids + if check(permission, userid, cn, itemid=id)] # return the batch object, using IDs only return Batch(self.client, allowed, self.pagesize, self.startwith,
--- a/roundup/security.py Fri Oct 18 18:04:46 2024 +0200 +++ b/roundup/security.py Mon Oct 21 16:11:13 2024 +0200 @@ -18,6 +18,20 @@ - properties (optional) - check function (optional) - props_only (optional, internal field is limit_perm_to_props_only) + - filter function (optional) returns filter arguments for + determining which records are visible by the user. The filter + function comes into play when determining if a set of nodes + found via a filter call of a class can be seen by the user -- + the normal way would be to call the permissions for each item + found, the filter call performs this on the database for all + nodes. + Signature of the filter function: + filter(db, userid, klass) + The filter must return a list of dictionaries with filter parameters. + Note that sort and group parameters of the filter call should + not be set by filter method (they will be overwritten) and the + parameter search_matches must not be set. + An empty list returned means no access for this filter method. The klass may be unset, indicating that this permission is not locked to a particular class. That means there may be multiple @@ -47,7 +61,7 @@ limit_perm_to_props_only = False def __init__(self, name='', description='', klass=None, - properties=None, check=None, props_only=None): + properties=None, check=None, props_only=None, filter=None): from roundup.anypy import findargspec self.name = name self.description = description @@ -55,6 +69,7 @@ self.properties = properties self._properties_dict = support.TruthDict(properties) self.check = check + self.filter = filter if properties is not None: # Set to None unless properties are defined. # This means that: @@ -211,6 +226,21 @@ self._permissions[pn][cn] = dict (((False, []), (True, []))) self._permissions[pn][cn][bool(p.check)].append(p) + def filter_iter (self, permission, classname): + """ Loop over all permissions for the current role on the class + with a check method (and props_only False). + """ + if permission not in self._permissions: + return + for c in (None, classname): + if c not in self._permissions[permission]: + continue + perms = self._permissions[permission][c][True] + for p in perms: + if p.limit_perm_to_props_only and p.properties: + continue + yield p + def hasPermission (self, db, perm, uid, classname, property, itemid, chk): # if itemid is given a classname must, too, checked in caller assert not itemid or classname @@ -283,6 +313,17 @@ from roundup import mailgw mailgw.initialiseSecurity(self) + def filter_iter(self, permission, userid, classname): + """ Loop over all permissions for the current user on the class + with a check method (and props_only False). + """ + for rolename in self.db.user.get_roles(userid): + if not rolename or (rolename not in self.role): + continue + r = self.role[rolename] + for perm in r.filter_iter(permission, classname): + yield perm + def getPermission(self, permission, classname=None, properties=None, check=None, props_only=None): ''' Find the Permission matching the name and for the class, if the @@ -359,6 +400,19 @@ return v return False + def is_filterable(self, permission, userid, classname): + """ Check if all permissions for the current user on the class + with a check method (and props_only False) also have a + filter method. We only consider permissions with props_only + set to False. Note that this will return True if there are + no permissions with a check method found, the performed + checks later will find no matching records. + """ + for perm in self.filter_iter (permission, userid, classname): + if not perm.filter: + return False + return True + def roleHasSearchPermission(self, classname, property, *rolenames): """ For each of the given roles, check the permissions. Property can be a transitive property.
