Edgewall Software
Modify

Opened 17 years ago

Last modified 13 months ago

#5648 new defect

DefaultPermissionGroupProvider not doing its whole job

Reported by: Dave Abrahams <dave@…> Owned by:
Priority: high Milestone: next-major-releases
Component: general Version: devel
Severity: critical Keywords: user group permission
Cc: leho@…, itamarost@…, ethan.jucovy@…, Ryan J Ollos Branch:
Release Notes:
API Changes:
Internal 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 (19)

comment:1 by Alec Thomas, 17 years ago

Milestone: 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 by Dave Abrahams <dave@…>, 17 years ago

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.

in reply to:  2 comment:3 by Dave Abrahams <dave@…>, 17 years ago

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 by Dave Abrahams <dave@…>, 17 years ago

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 by Emmanuel Blot, 17 years ago

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 by Noah Kantrowitz, 17 years ago

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

in reply to:  5 comment:7 by Dave Abrahams <dave@…>, 17 years ago

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

in reply to:  4 comment:8 by Dave Abrahams <dave@…>, 17 years ago

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.

in reply to:  2 ; comment:9 by Alec Thomas, 17 years ago

Owner: changed from Christopher Lenz to 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.

in reply to:  9 ; comment:10 by Dave Abrahams <dave@…>, 17 years ago

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

in reply to:  10 comment:11 by Christian Boos, 17 years ago

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 by Christian Boos, 14 years ago

Priority: normalhigh

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 by lkraav <leho@…>, 14 years ago

Cc: leho@… added

comment:14 by Christian Boos, 14 years ago

Priority: highnormal
Severity: normalcritical

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

comment:15 by Christian Boos, 14 years ago

Owner: changed from Alec Thomas to Christian Boos
Priority: normalhigh

comment:16 by Itamar Ostricher, 13 years ago

Cc: itamarost@… added

comment:17 by ethan.jucovy@…, 10 years ago

Cc: ethan.jucovy@… added

comment:18 by Ryan J Ollos, 10 years ago

Cc: Ryan J Ollos added

comment:19 by Ryan J Ollos, 9 years ago

Owner: Christian Boos removed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.