DefaultPermissionGroupProvider not doing its whole job

[Dave Abrahams <dave@…>]

DefaultPermissionGroupProvider​ doesn't really return the permission groups of which a user is a member; it only handles the "anonymous" and "authenticated" groups. All of the threading through user-defined groups is actually done by DefaultPermissionStore. While I'm sure that's efficient, it's a problem when the permission_store is replaced, as with ​TracForge, if it expects to be able to extend the existing permission group system. See ​TracForgeGroupsModule for an example. This problem exists in 0.10-stable and 0.11 AFAICT.

Of course, it's entirely possible that I've completely misinterpreted the meaning of "group" and the intention behind DefaultPermissionStore here :).

[Alec Thomas]

We chatted about this on IRC and given the (justified) assumption that Trac treats user groups as a first-class concept, PermissionSystem should expose them directly rather than only through DefaultPermissionStore.

Two changes would clean this up:

1. make DefaultPermissionGroupProvider return generic groups
2. move the !IPermissionGroupProvider logic into PermissionSystem

[Dave Abrahams <dave@…>]

More insight into the functionality that should be exposed:

I was implementing get_users_with_permissions for ​TracForgePermissionModule by looping through all users and seeing if their permissions matched. It occurred to me that this approach could be really inefficient. Probably we should start with the permissions and work backwards to users, finding all of the "roles" directly granted a given permission and transitively searching for roles that have been granted those roles, stopping with the leaves.

In other words, we probably need to step back and decide what operations we need to be efficient and design the architecture to support them. Currently we have a mishmosh of APIs that probably don't work when Groups (or preferably, roles) become a first-class concept.

[Dave Abrahams <dave@…>]

Replying to Dave Abrahams <dave@boost-consulting.com>:

we probably need to step back and decide what operations we need to be efficient and design the architecture to support them. Currently we have a mishmosh of APIs that probably don't work when Groups (or preferably, roles) become a first-class concept.

One simple example is that many of these APIs return lists which the implementations convert from sets. When we compose the APIs to get a general system, we'll need sets again, so unless we change the interface we'll be converting back and forth a lot.

[Dave Abrahams <dave@…>]

Since this ticket is eventually going to result in a redesign of the permission system (I trust), I'll add another issue to it: get_all_permissions gives the impression of being a general-purpose function but it is not. It's really only used for displaying the admin panel. If you tried to use it for other things, it would be wrong (e.g. the authzgroups plugin adds permissions that aren't represented here) and if you tried to make it right for those things it would do the wrong thing for the admin panel.

[Emmanuel Blot]

I really don't think a Trac component should be redesigned right now for release 0.11. We need to stabilize Trac now, and avoid major changes.

[Noah Kantrowitz]

eblot is right, this is very much a 0.12 issue. This will dovetail nicely with a review of how to handle users and user metadata (a la IUserDirectoryProvider).

[Dave Abrahams <dave@…>]

Replying to eblot:

I really don't think a Trac component should be redesigned right now for release 0.11. We need to stabilize Trac now, and avoid major changes.

Of course; I never dreamed this was an 0.11 issue

[Dave Abrahams <dave@…>]

Replying to Dave Abrahams <dave@boost-consulting.com>:

Since this ticket is eventually going to result in a redesign of the permission system (I trust), I'll add another issue to it: get_all_permissions gives the impression of being a general-purpose function but it is not. It's really only used for displaying the admin panel.

A similar thing goes for get_users_with_permission. It is strictly used to display the list of potential ticket owners in a restrict_owner configuration.

[Alec Thomas]

Replying to Dave Abrahams <dave@boost-consulting.com>:

In other words, we probably need to step back and decide what operations we need to be efficient and design the architecture to support them. Currently we have a mishmosh of APIs that probably don't work when Groups (or preferably, roles) become a first-class concept.

I think you're overloading this ticket. The two points I mentioned, and what I understand the original request to be, can probably be completely backwards compatible. But what you're proposing in your comments would obviously require fundamental changes to the permission system.

[Dave Abrahams <dave@…>]

Replying to athomas:

I think you're overloading this ticket.

Yes.

The two points I mentioned, and what I understand the original request to be, can probably be completely backwards compatible. But what you're proposing in your comments would obviously require fundamental changes to the permission system.

Makes sense. Do we need a ticket called "Permission System Needs Fundamental Redesign?"

[Christian Boos]

Replying to Dave Abrahams <dave@boost-consulting.com>:

Replying to athomas:

I think you're overloading this ticket.

Yes.

… what you're proposing in your comments would obviously require fundamental changes to the permission system.

Makes sense. Do we need a ticket called "Permission System Needs Fundamental Redesign?"

Feel free to write a TracDev/Proposals/NewPermissionSystem (or any better name at your convenience).

hint: add some code to that proposal and that could even become a 0.12 sandbox ;-)

Be sure to review #2456 as well.

[Christian Boos]

The API for group management needs to be fixed, among other things, move:

group_providers = ExtensionPoint(IPermissionGroupProvider)

from the DefaultPermissionStore to the PermissionSystem.

It should be possible for a plugin to get all the groups to which a user belong (needed in AuthzPolicyPlugin).

[Christian Boos]

Raising severity as it's indeed an important topic on which a bunch of other ticket depends.