Hi Ralf:

In message <1714388568.86.0.772511523784.issue2551330@roundup.psfhosted.org>,
Ralf Schlatterbeck writes:
>Ralf Schlatterbeck added the comment:
>
>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:

EWDurbin fixed this on April 15th.

  https://github.com/python/psf-salt/issues/352

Email has been working for me since.

>On Fri, Apr 05, 2024 at 02:25:49AM +0000, John Rouillard wrote:
>> > 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.

I'm still not getting it.

>> 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.

Hang on. By 'role' do you mean Permission like "View" or Role as in
the user class: Admin, User, Anonymous? I am interpreting Role as
Admin ....

>> 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.

Ok, but there is no role lookup (Admin, Anonymous) at any time in
4. Roles just don't exist there.

>> 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.

This has to be an optimization unless we want to drop support for
non-sql databases. I don't like dropping support for non-sql databases
from the hyperdb.

>> 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 :-)

Good.

>> 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.?

If I have a rule "View access to username in User class" it implicitly
allows "View User". So:

   db.security.addPermission(name='View', klass='user',
    properties=('activity', 'id', 'organisation', 'phone', 'realname',
            'timezone', 'username'),
            description="User can have limited view of other users",
            props_only=False)

causes hasPermission("View", "1", "user") to return true. This is a
case of "no prop_only". The permission:

   db.security.addPermission(name='View', klass='user',
    properties=('activity', 'id', 'organisation', 'phone', 'realname',
            'timezone', 'username'),
            description="User can have limited view of other users",
            props_only=True)

would not satisfy hasPermission("View", "1", "user"). It would satisfy
hasPermission("View", "1", "user", property="phone"). This last is an
example of a permission with prop_only.

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

Agreed.

>> 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.

Fair enough. IIRC, the check functions aren't called unless there is
an exact match. So check function is called for:

  hasPermission("View", "1", "keyword")

given:

  db.security.addPermission(name='View', klass='keyword',
      check=own_keyword,
      description="User is allowed to edit what they created")

but not for:

  db.security.addPermission(name='View', klass='keyword',
      check=own_keyword, properties="name")

because the check doesn't specify any properties (but still allows
hasPermission to pass). I am a little hazy on this but I think I'm
correct. I think something like this was what led to adding
props_only.

>> I am not sure what the default order of permissions 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 :-)

As long as we preserve the ordering that sounds good to me.

>> > > 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 correct 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.

Understood.

>> 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.

As I noted above I think a permission with a check can still cause a
check to pass even without running the check.

I still find:

   db.security.addPermission(name='View', klass='user',
    properties=('activity', 'id', 'organisation', 'phone', 'realname',
            'timezone', 'username'),
            description="User can have limited view of other users")

matching hasPermission("View", "1", "user") unexpected. It makes sense
that I would need View rights on user (traversal rights) to access the
phone but...

>Thanks for looking into this!

Yeah something to look at for 2.5. I think improving the permission
data structure is doable. I am less optimistic about being able to
push this down to SQL.