On Thu, Oct 17, 2024 at 02:14:27PM +0000, John Rouillard wrote:
> >I would argue that if we find any permission on the given 'klass' that
> >does not have a check method, we do not need to do this: The check will
> >always tell us that we can see at least one property of *each* item in
> >the klass. So we do not need to filter and consequently do not need to
> >loop over all issues found when we establish that fact before the loop.
> 
> Your logic sounds right. The itemid AFAIK is only used in a check method.
> 
> Would the following code (properly indented) do the trick:
> 
>     if check(permission, userid, self.classname):
>       allowed = [id for id in klass.filter(matches, filterspec, sort, group)]
>     else:
>       allowed = [id for id in klass.filter(matches, filterspec, sort, group)
>                  if check(permission, userid, self.classname, itemid=id)]

Hmm, I think for the case that where we find a check on the whole class
we would simply return the result of filter without iterating, i.e.

ok = find_a_check_on_whole_class (permission, userid, self.classname)
if ok:
    allowed = klass.filter(matches, filterspec, sort, group)
elif check(permission, userid, self.classname):
    allowed = [id for id in klass.filter(matches, filterspec, sort, group)
                if check(permission, userid, self.classname, itemid=id)]

I think we need to add a method for getting a check on the whole class
to roundup/security.py

> The check(permission, userid, self.classname) should fail if all
> permissions for 'permission' have a check command or properties with a
> props_only=true.

Yes, should do the trick. But 'check' at that point in the code is the
hasPermission check, see some lines above:

check = self._client.db.security.hasPermission

So we want to group the checks into two categories, the ones with and
the others without a check method. At the point of filtering we would
completely ignore the ones with props_only=True. And we would first
check the ones in the first category and not filter at all in python
when we find any that apply.

> >When rendering the HTML, the check is performed again for each property.
> >If the property should not be shown it is rendered as '[Hidden]'.
> >
> >So no need for memoization.
> 
> I did a fast memoization change (patch attached) for hasPermission if
> the check command and item command are missing. That at least seems to
> be used for the 'Web Access' check in templating.py
> HTMLClass::is_X_ok() where X is view, edit, restore ... and in the
> HTMLClass::list() method.

I don't care much about HTMLClass: My templates edit only a handful of
things at a time. This may be different for your use-case with an
Excel-like editing form.

> >Now my second proposal comes into play: If we have check methods with
> >SQL (or some other mechanism that ultimately compiles to SQL) we can
> >*OR* all the SQL for each check method and put that with an AND into the
> >[sql generated by the] filter call. Then the loop would again only
> >return valid items and again we would not have to do it in python in the
> >loop.
> 
> We still need to figure out a method to skip the python permission
> checks only for those checks that are pushed down to the DB.

Yes.

> >Note that I think the performance hog lies only in searching, not in
> >other permission checks like creation or editing.
> 
> By searching do you mean finding issues to display on an index page?

Yes, see my log in the other mail: the permission checks are
interleaved with a *lot* of SQL.

> Also I have a spreadsheet like editing interface for issues where I
> mix an index with edit checks. So it might not always be a View
> permission.

Yes, how many do you have on a page?
Because I think *building* the page and selecting what to display may
be the performance point.

> 
> >And it probably
> >applies only to 'View' permissions (I don't think batch is ever called
> >with an explicit permission other than 'View')
> 
> I'm not sure that batch is only called with view permissions. A quick
> grep for 'batch(' doesn't return any calls to batch() in the code. I
> assume it's triggered from templates. Is there any reason to assume
> that a template can't generate a batch(..., permission="edit") call?

Hmm, yes, it's probably called from templating.

> If View is the only permission used in a batch call, why does the
> batch() method have a permission parameter?

I don't know .-)
This comes from ZTUtils so it is inherited code that has seen a lot of
use-cases which probably are not all in roundup.

Thanks
Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com