On Mon, Apr 22, 2024 at 03:43:23PM +0000, John Rouillard wrote:
> Ralf, do you have more thoughts on this?

I had, yes, but was hit again by my reply being bounced. My reply was April 5th. Hmpf. My workflow here really depends on being able to reply to an email. That's what I wrote a month ago:

On Fri, Apr 05, 2024 at 02:25:49AM +0000, John Rouillard wrote:
> > Yes, and the permissions are checked in arbitrary order.
>
> Reordering the permissions can help here.

Yes, that's the idea.

>
> > My proposal would be to group permissions by role (so that we
> > need to check only the permissions where we know the user has
> > that role) *and* order the easy ones (without a check method)
> > first. That way we can - Check only the permissions for the
> > roles we have
>
> How would that work? Would you need to replace a
> hasPermission('View', 'issue' ...) in the core code with a
> hasRole() check? If so how would the code know what Role to use?
> The same permission could be in multiple roles.

No the hasPermission would stay. It just would check the permissions in
an order that has least effort first. And it would not check permissions
for roles the user doesn't have.

> Also I wonder (see below) if the permissions are grouped by role
> by default.

Yes, the implementation would store permissions grouped by role (e.g. in
a dict). In each dict we would have containers of permissions.
Permissions that are for the whole Class (not individual properties of a
class) *without* check methods would be in the first container, followed
by permissions on individual attributes (without check methods) followed
by everything with check methods, probably in different containers. So
we could check for each role the first entry (for the whole Class
without check methods), then each role the second entry, etc. The search
-- as it is now -- terminates when a matching entry is found.

> This is my understanding of how things work, do you agree?
>
> 1) user authenticates
> 2) user's roles are looked up
> 3) security object is populated with the permissions associated
>    with the user's roles
> 4) hasPermission(X) calls are made to see if the user can do X

Hmm, yes, the 3) will build data structures that can be more effectively
searched, see above.

> AFAIK access is denied only if no permission matches. A
> permission can't return a "deny", it only says "I don't
> match". Is that your understanding as well?

Yes.

> I can see a way to do some of this if we define relationships
> like:
>
>   owner
>   actor
>
> which are done via check commands currently. These map to a
> database check on 'creator' and 'actor'. But expressing a
> permission like:
>
>   If the issue is marked private the user needs to be on one of
>      two access lists (nosy and verynosy in my case) to see the
>      issue
>
> requires a like check for the id number in the two fields. Not
> insurmountable. But you would need to define some sort of method
> to implement this using a join (maybe, my SQL isn't great).

Yes, the owner checks are easy (but also require a join I think) and
other checks may be more challenging or even impossible. I think the
challenge is to come up with a user-understandable formulation of
sql-checkable permissions. So that permissions can be defined in a way
that the user understands *and* that can be translated to an SQL query.

> Since no permission can cause a 'deny' action, I think sorting it
> is safe, but I am not willing to put money on that.

The order currently is arbitrary and depends on the order of definition.
I think we would have seen a lot of bugs if this was order-dependent.

So, yes, I would put some money on that :-)

> Adding grouping by class (or None for tracker level permissions)
> to the existing grouping by permission name I think is a
> win. Then sorting within those buckets by:
>
>    no properties assigned no check function
>    no properties assigned check function
>    properties assigned and properties defined
>       without prop_only no check
>    properties assigned and properties defined
>       without prop_only with check
>    properties assigned and defined with prop_only no check function
>    properties assigned and defined with prop_only and check function

What do you mean be
 properties assigned and properties defined without prop_only
etc.?

I would put the ones with check functions last. See above where I
outlined this. The check functions are the problematic part concerning
performance.

> might be faster. I think this ordering can be expressed using a tuple
> with one position for each criteria. So a permission list item for the
> first bucket is:
>
>    (properties=0, check=0, prop-only=0, permission object)
>
> and the last bucket is
>
>    (properties=1, check=1, prop-only=1, permission object)
>
> So sorting should be doable.

I agree the principle with buckets, just the order of the checks would
be different checking permissions with check functions last.

> I am not sure what the default order of permisions is. I think the
> order of permissions assigned to a role is in the order it's assigned
> in schema.py. I would expect the order for a user with roles User,
> Role1, Role2 would be:
>
>   User permissions in order they were added in schema.py
>   Role1 permissions in order they were added in schema.py
>   Role2 permissions in order they were added in schema.py
>
> Reordering into buckets as above could help.

Yes to both, order is order of definition in schema.py and reordering
could help :-)

> > > Sorting the permissions can help as well. I am concerned that
> > > sorting could result in changing the applied permissions from
> > > the unsorted state. AFAIK, unlike detectors permissions don't
> > > have a method to set ordering which plays a role in what
> > > permission is expressed.
> >
> > I do not think the semantics change. If a person has multiple
> > permissions on a Class (and/or properties) this should be
> > equivalent to just one permission.
>
> I think you are corect but it's because a permission can't result
> in a deny. It can only result in a "I don't match" so the
> permission checker moves on to the next item.

Yes. It iterates over permissions until a match or until end of
permissions.

> Permissions defined with properties (e.g. View) match a request
> for View permissions (without properties) on the class. So:
>
>   havePermission('View', 'issue')
>
> matches:
>
>   addPermission(name='View', klass='issue', property=['title']).
>
> This may or may not make a difference. It's possible that the
> order I outlined above might be more efficient if all permisions
> with a check were moved after the permissions without
> checks. Otherwise the bucket order stays as above.

Yes. We should definitely check entries with a check function last.

Thanks for looking into this!

Ralf