On Mon, Apr 29, 2024 at 05:40:48PM +0000, John Rouillard wrote:
> >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.
...
> Email has been working for me since.

Great, thanks for pushing this!

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

Yes role in the user class. My use-cases involve lots of roles.
And we're seeing large search times even for users that have full
permission on a class because apparently permissions of other roles
(which the current user *also* has) are checked first. So the idea would
be to group permission by role.

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

They do, from roundup/security.py:

    def hasPermission(self, permission, userid, classname=None,
                      property=None, itemid=None):
        '''Look through all the Roles, and hence Permissions, and
           see if "permission" exists given the constraints of
           classname, property, itemid, and props_only.
[...]
        for rolename in self.db.user.get_roles(userid):
            if not rolename or (rolename not in self.role):
                continue
            # for each of the user's Roles, check the permissions
            for perm in self.role[rolename].permissions:

I propose to change the order of that iterations:
- First loop over the 'easy' targets, for each role check only the
  permissions without a permission method
Only if nothing is found:
- Loop over roles again this time using the costly checks *with* check
  methods

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

I want them *in addition* to the current methods.
And if we come up with a suitable formulation this can be implemented
for non-sql backends, too. But for non-sql databases there is probably
no performance improvement.

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

Thanks I forgot we had that additional prop_only flag.

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

Yes, this may be the case.
But I think when displaying the results of a search (e.g. in an index
template of the web interface) all properties are checked explicitly.

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

I do not think we need to preserve the ordering: If two permissions
(maybe even for different roles held by the same user) specify that a
user has access it doesn't matter which one triggers first.

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

Yes, me too. I would call it a bug. And I *think* the props_only flag
was added for backwards compatibility when it was discovered that the
checks are un-untuitive (or should I say buggy). And I also think that
roundup core doesn't contain any permissions on properties without the
props_only flag, at least I think I remember this from the far past.

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

Yes I think so, too. If I had an idea of how permission with check
methods could be specified more SQL-friendly I would already have
proposed it :-)

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