Opened 4 years ago

Closed 8 months ago

## #11305 closed enhancement (fixed)

Reported by: Owned by: Andras Korn Andras Korn normal DirectoryAuthPlugin normal

### Description

Currently, your code finds group memberships by searching for groups that have the user's DN as member.

This doesn't work for nested groups. It might be possible to use memberOf instead (I'm not sure; it's also not universally available); but failing that, there seem to be two approaches:

1. Build a full internal representation of the entire LDAP group hierarchy. This probably doesn't scale well if there are many groups.
1. Look up specific groups (e.g. the ones that have special permissions attached in the trac instance) and recursively obtain lists of their members (by checking whether each member is also a group, and enumerating its own members etc.). This is, I think, the better solution overall.

### comment:1 follow-up:  2 Changed 8 months ago by bebbo

Here is a third approach:

1. If a member of a group is also a group that member-group is checked recursively.

The benefit: You may also use existing groups which are not in the current tree.

### comment:2 in reply to:  1 Changed 8 months ago by Andras Korn

Here is a third approach:

1. If a member of a group is also a group that member-group is checked recursively.

The benefit: You may also use existing groups which are not in the current tree.

Unfortunately this doesn't work here, because the code needs a user->memberships mapping, not a group->members mapping.

### comment:3 follow-up:  4 Changed 8 months ago by bebbo

1. for every group containing the user x, the parent dn's are recursively added until the group_basedn is hit.

### comment:4 in reply to:  3 ; follow-up:  5 Changed 8 months ago by Andras Korn

1. for every group containing the user x, the parent dn's are recursively added until the group_basedn is hit.

If by "parent dn" you mean "every group the groups the user x is a member of are members of", then yes, this way of obtaining a transitive closure should work. However, it would need many LDAP lookups and would thus be slow.

Imagine the following relatively simple setup:

• groupA consists of groupB and groupC
• groupC consists of groupD and groupE
• x is a member of groupB and groupE (and thus, transitively, also of groupC and groupA)

To obtain the list of groups x is a member of, you first query for member=uid=x,..., and receive cn=groupB,... and cn=groupE. You then have to query for member=cn=groupB,... and member=cn=groupE. This yields cn=groupA,... and cn=groupC,.... Those have to be looked up again, and so on.

Of course it's possible to cache these lookups, but that's what I meant in "2." -- pre-cache all relevant groups and their members.

### comment:5 in reply to:  4 ; follow-up:  6 Changed 8 months ago by bebbo

1. for every group containing the user x, the parent dn's are recursively added until the group_basedn is hit.

If by "parent dn" you mean "every group the groups the user x is a member of are members of", then yes, this way of obtaining a transitive closure should work. However, it would need many LDAP lookups and would thus be slow.

Imagine the following relatively simple setup:

• groupA consists of groupB and groupC
• groupC consists of groupD and groupE
• x is a member of groupB and groupE (and thus, transitively, also of groupC and groupA)

To obtain the list of groups x is a member of, you first query for member=uid=x,..., and receive cn=groupB,... and cn=groupE. You then have to query for member=cn=groupB,... and member=cn=groupE. This yields cn=groupA,... and cn=groupC,.... Those have to be looked up again, and so on.

Of course it's possible to cache these lookups, but that's what I meant in "2." -- pre-cache all relevant groups and their members.

There is no ldap query needed to compute a parent dn - simply trim the text to next next komma.

Plus there is a cache for the full query.

=> Yes? Go for it??

### comment:6 in reply to:  5 ; follow-up:  7 Changed 8 months ago by Andras Korn

There is no ldap query needed to compute a parent dn - simply trim the text to next next komma.

OK, so you literally mean "parent dn" when you say "parent dn" -- but in that case, I don't see how what you're saying is diffrerent from my suggestion of recursively building an internal representation of LDAP group memberships, including nesting.

### comment:7 in reply to:  6 Changed 8 months ago by bebbo

There is no ldap query needed to compute a parent dn - simply trim the text to next next komma.

OK, so you literally mean "parent dn" when you say "parent dn" -- but in that case, I don't see how what you're saying is diffrerent from my suggestion of recursively building an internal representation of LDAP group memberships, including nesting.

It differs at least from what I am imaging if I read "building an internal representation of LDAP group memberships" :-)

### comment:8 Changed 8 months ago by bebbo

In 16076:

refs #11305

• the user's groups are all added from the assigned group to the top level group if the config option 'group_nested' is set to True

### comment:9 Changed 8 months ago by bebbo

Owner: changed from branson to Andras Korn new → assigned

Please test using the version 2.1.0-SNAPSHOT

### comment:10 Changed 8 months ago by bebbo

Resolution: → fixed assigned → closed

in my tests the nested groups are displayed correctly.

### Modify Ticket

Change Properties