Opened 7 years ago
Last modified 4 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: |
|
||
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 IPermissionGroupProvider
s.
Change History (11)
by , 7 years ago
Attachment: | privateticketsplugin.diff added |
---|
comment:2 by , 7 years ago
Release Notes: | modified (diff) |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
Committed to trunk in r16326.
comment:3 by , 7 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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
follow-up: 5 comment:4 by , 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.
comment:5 by , 7 years ago
Replying to Ryan J Ollos:
The docstring for
IPermissionStore.get_user_permissions
is either incorrect, orDefaultPermissionStore.get_user_permissions
is incorrectly implementing the interface. The interface claims a dictionary is returned, butDefaultPermissionStore
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.
comment:6 by , 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 , 7 years ago
Status: | reopened → assigned |
---|
comment:8 by , 7 years ago
HideValsSystem._get_groups is another example where get_user_groups
would be useful.
comment:9 by , 7 years ago
PrivateReports._get_user_groups is another example where get_user_groups
would be useful.
comment:10 by , 6 years ago
Proposed changes in log:rjollos.git:t12915_add_get_permission_groups.2.
Proposed changes in [3fdd6a387/rjollos.git]. Example use: privateticketsplugin.diff.
True
, so I chose to return a sorted list fromget_user_groups
.get_user_permissions
returns all permissions whenusername
isNone
. The same can't easily be done forget_user_groups
becauseIPermissionGroupProvider
s don't return all groups whenusername
isNone
. I'm not even sure we'd want that behavior for DefaultPermissionGroupProvider. We'd probably need to add aget_all_groups
method to theIPermissionGroupProvider
interface.IPermissionGroupProvider
s. We'd probably need theDefaultPermissionGroupProvider.get_all_groups
method for it to return all groups. This could be considered a defect in group expansion of theset_owner
field.