Edgewall Software
Modify

Opened 7 years ago

Last modified 2 months ago

#5648 new defect

DefaultPermissionGroupProvider not doing its whole job

Reported by: Dave Abrahams <dave@…> Owned by: cboos
Priority: high Milestone: next-major-releases
Component: general Version: devel
Severity: critical Keywords: user group permission
Cc: leho@…, itamarost@…, ethan.jucovy@…, rjollos
Release Notes:
API Changes:

Description

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

Attachments (0)

Change History (18)

comment:1 Changed 7 years ago by athomas

  • Milestone set to 0.11

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

comment:2 follow-ups: Changed 7 years ago by 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.

comment:3 in reply to: ↑ 2 Changed 7 years ago by 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.

comment:4 follow-up: Changed 7 years ago by 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.

comment:5 follow-up: Changed 7 years ago by 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.

comment:6 Changed 7 years ago by nkantrowitz

  • Milestone changed from 0.11 to 0.12

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

comment:7 in reply to: ↑ 5 Changed 7 years ago by 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

comment:8 in reply to: ↑ 4 Changed 7 years ago by 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.

comment:9 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by athomas

  • Owner changed from cmlenz to athomas

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.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by 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?"

comment:11 in reply to: ↑ 10 Changed 7 years ago by cboos

  • Keywords user group permission added

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.

comment:12 Changed 5 years ago by cboos

  • Priority changed from normal to high

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

comment:13 Changed 5 years ago by lkraav <leho@…>

  • Cc leho@… added

comment:14 Changed 4 years ago by cboos

  • Priority changed from high to normal
  • Severity changed from normal to critical

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

comment:15 Changed 4 years ago by cboos

  • Owner changed from athomas to cboos
  • Priority changed from normal to high

comment:16 Changed 4 years ago by itamaro

  • Cc itamarost@… added

comment:17 Changed 8 months ago by ethan.jucovy@…

  • Cc ethan.jucovy@… added

comment:18 Changed 2 months ago by rjollos

  • Cc rjollos added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new The owner will remain cboos.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from cboos to anonymous. Next status will be 'assigned'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.