Edgewall Software

Opened 5 years ago

Last modified 2 years ago

#12915 closed enhancement

Add method to PermissionSystem for retrieving user groups — at Version 10

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

Change History (11)

by Ryan J Ollos, 5 years ago

Attachment: privateticketsplugin.diff added

comment:1 by Ryan J Ollos, 5 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_groups 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.
Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:2 by Ryan J Ollos, 5 years ago

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

Committed to trunk in r16326.

comment:3 by Jun Omae, 5 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, 5 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 5 years ago by Ryan J Ollos (previous) (diff)

in reply to:  4 comment:5 by anonymous, 5 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, 5 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, 5 years ago

Status: reopenedassigned

comment:8 by Ryan J Ollos, 5 years ago

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

comment:9 by Ryan J Ollos, 5 years ago

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

comment:10 by Ryan J Ollos, 4 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Note: See TracTickets for help on using tickets.