Add method to PermissionSystem for retrieving user groups

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

Committed to trunk in r16326.

[Jun Omae]

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

[Ryan J Ollos]

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.

[anonymous]

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:

• ​LDAPPlugin: Implements the dictionary. This probably still works in practice, since iteration over the dictionary iterates over the keys.
• ​TracForgePlugin: Also a list.
• ​TracSuperUserPlugin: Also a list.

[Jun Omae]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12915_add_get_permission_groups.2.

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

[Jun Omae]

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

[Jun Omae]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

Changes committed:

• 1.0-stable: r16531
• 1.2-stable: r16532
• trunk: [16533:16537]

Follow-on ticket is #13014.