Edgewall Software
Modify

Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#12915 closed enhancement (fixed)

Add method to PermissionSystem for retrieving user groups

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.3.3
Component: general Version:
Severity: normal Keywords: permissions
Cc: Branch:
Release Notes:
API Changes:
  • Added PermissionSystem.get_permission_groups(username), which returns a sorted list of all groups that username belongs to.
  • DefaultPermissionStore implements IPermissionGroupProvider.
  • Added skip argument to PermissionSystem.get_actions_dict, for skipping an IPermissionRequestor, for consistency with PermissionSystem.get_actions method arguments (applied to 1.0-stable).
Internal Changes:

Description

I have wanted a method of PermissionSystem for retrieving all groups that a user belongs to, both those defined in the permissions table and in IPermissionGroupProviders.

Attachments (1)

privateticketsplugin.diff (2.1 KB ) - added by Ryan J Ollos 7 years ago.

Download all attachments as: .zip

Change History (19)

by Ryan J Ollos, 7 years ago

Attachment: privateticketsplugin.diff added

comment:1 by Ryan J Ollos, 7 years ago

Proposed changes in [3fdd6a387/rjollos.git]. Example use: privateticketsplugin.diff.

  1. get_user_permissions is similarly named, but returns a dictionary. I have never understood the usefulness of mapping all the keys to True, so I chose to return a sorted list from get_user_groups.
  2. get_user_permissions returns all permissions when username is None. The same can't easily be done for get_user_permissions because IPermissionGroupProviders don't return all groups when username is None. I'm not even sure we'd want that behavior for DefaultPermissionGroupProvider. We'd probably need to add a get_all_groups method to the IPermissionGroupProvider interface.
  3. get_groups_dict doesn't really return all groups, because it doesn't account for IPermissionGroupProviders. We'd probably need the DefaultPermissionGroupProvider.get_all_groups method for it to return all groups. This could be considered a defect in group expansion of the set_owner field.
Version 2, edited 7 years ago by Ryan J Ollos (previous) (next) (diff)

comment:2 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in r16326.

comment:3 by Jun Omae, 7 years ago

Resolution: fixed
Status: closedreopened

When group has cyclic reference, maximum recursion depth would be exceeded in get_user_groups.

I think the expanding-group logic might be able to use the same logic in get_user_permissions.

>>> env = EnvironmentStub(default_data=True)
>>> permsys = PermissionSystem(env)
>>> permsys.grant_permission('anonymous', 'group1')
>>> permsys.grant_permission('group1', 'group2')  # or permsys.grant_permission('group1', 'group1')
>>> permsys.grant_permission('group2', 'group1')
>>> for row in env.db_query('SELECT * FROM permission ORDER BY 1, 2'): print(row)
...
(u'anonymous', u'BROWSER_VIEW')
(u'anonymous', u'CHANGESET_VIEW')
(u'anonymous', u'FILE_VIEW')
(u'anonymous', u'LOG_VIEW')
(u'anonymous', u'MILESTONE_VIEW')
(u'anonymous', u'REPORT_SQL_VIEW')
(u'anonymous', u'REPORT_VIEW')
(u'anonymous', u'ROADMAP_VIEW')
(u'anonymous', u'SEARCH_VIEW')
(u'anonymous', u'TICKET_VIEW')
(u'anonymous', u'TIMELINE_VIEW')
(u'anonymous', u'WIKI_VIEW')
(u'anonymous', u'group1')
(u'authenticated', u'TICKET_CREATE')
(u'authenticated', u'TICKET_MODIFY')
(u'authenticated', u'WIKI_CREATE')
(u'authenticated', u'WIKI_MODIFY')
(u'group1', u'group2')
(u'group2', u'group1')
>>> permsys.get_user_groups('user1')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/perm.py", line 486, in get_user_groups
    expand_members(group, members)
  File "trac/perm.py", line 483, in expand_members
    expand_members(group, groups_dict[m])
  ...
    expand_members(group, groups_dict[m])
  File "trac/perm.py", line 480, in expand_members
    if m == username:
RuntimeError: maximum recursion depth exceeded in cmp

comment:4 by Ryan J Ollos, 7 years ago

Recursion is fixed in r16329.

I can see how the logic is similar to DefaultPermissionStore.get_user_permissions. subjects, as calculated in that method, is the set of permission groups possessed by the user. We could just copy/paste and modify that method: [4e719b32d/rjollos.git].

However, another thing to consider is that DefaultPermissionStore is storing permission groups, so maybe it should implement IPermissionGroupProvider: [bf3fe0818/rjollos.git].

In r16314 I fixed the docstring for DefaultPermissionStore.get_user_permissions. The docstring for IPermissionStore.get_user_permissions is either incorrect, or DefaultPermissionStore.get_user_permissions is incorrectly implementing the interface. The interface claims a dictionary is returned, but DefaultPermissionStore returns a list.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

in reply to:  4 comment:5 by anonymous, 7 years ago

Replying to Ryan J Ollos:

The docstring for IPermissionStore.get_user_permissions is either incorrect, or DefaultPermissionStore.get_user_permissions is incorrectly implementing the interface. The interface claims a dictionary is returned, but DefaultPermissionStore returns a list.

Looks like this was already wrong since changeset:1860#file8 (get_permissions was later renamed to get_user_permissions).

Other IPermissionStore implementations:

comment:6 by Jun Omae, 7 years ago

Another case can lead the recursion.

>>> from trac.perm import PermissionSystem
>>> env = EnvironmentStub(default_data=True)
>>> permsys = PermissionSystem(env)
>>> permsys.grant_permission('anonymous', 'group1')
>>> permsys.grant_permission('group1', 'anonymous')
>>> permsys.grant_permission('group1', 'group1')
>>> permsys.get_user_groups('anonymous')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "trac/perm.py", line 486, in get_user_groups
    expand_members(group, members)
  ...
  File "trac/perm.py", line 480, in expand_members
    if m == username:
RuntimeError: maximum recursion depth exceeded in cmp

comment:7 by Ryan J Ollos, 7 years ago

Status: reopenedassigned

comment:8 by Ryan J Ollos, 6 years ago

HideValsSystem._get_groups is another example where get_user_groups would be useful.

comment:9 by Ryan J Ollos, 6 years ago

PrivateReports._get_user_groups is another example where get_user_groups would be useful.

comment:10 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Release Notes: modified (diff)

comment:11 by Ryan J Ollos, 6 years ago

I'd like to propose [151bf993/rjollos.git] for 1.0-stable and 1.2-stable as well. I think it will make it easier to see what permission checks are being done when debugging permissions.

in reply to:  4 ; comment:12 by Jun Omae, 6 years ago

Replying to Ryan J Ollos:

However, another thing to consider is that DefaultPermissionStore is storing permission groups, so maybe it should implement IPermissionGroupProvider: [bf3fe0818/rjollos.git].

I think DefaultPermissionStore.get_permission_groups() should return groups provided by only permission table rather than IPermissionGroupProvider components except itself. (e.g. DefaultPermissionStore.get_permission_groups() shouldn't return anonymous and authenticated groups).

in reply to:  11 comment:13 by Jun Omae, 6 years ago

Replying to Ryan J Ollos:

I'd like to propose [151bf993/rjollos.git] for 1.0-stable and 1.2-stable as well. I think it will make it easier to see what permission checks are being done when debugging permissions.

Sounds good.

comment:14 by Ryan J Ollos, 6 years ago

Committed [151bf993/rjollos.git] to 1.0-stable in r16482, merged in r16483, r16484.

in reply to:  12 comment:15 by Ryan J Ollos, 6 years ago

Replying to Jun Omae:

I think DefaultPermissionStore.get_permission_groups() should return groups provided by only permission table rather than IPermissionGroupProvider components except itself. (e.g. DefaultPermissionStore.get_permission_groups() shouldn't return anonymous and authenticated groups).

Yeah, that makes sense.

With that in mind, DefaultPermissionStore.get_user_permissions probably should not get user permissions provided by all of the user's groups: tags/trac-1.2.2/trac/perm.py@:188-190#L169. Put another way, DefaultPermissionStore should not concern itself with IPermissionGroupProviders. Instead, that work should be left to PermissionSystem.get_user_permissions. However, making that change leads to more extensive changes, including API changes, so I'll leave as-is for now and possibly propose changes in another ticket.

Revised in rjollos.git:t12915_add_get_permission_groups.3.

comment:16 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)
Resolution: fixed
Status: assignedclosed

Changes committed:

Follow-on ticket is #13014.

comment:17 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)

comment:18 by Ryan J Ollos, 4 years ago

API Changes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


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