Trac LDAP plugin makes too many round trips for group membership

[Jason Dusek]

I have written a group membership routine to allow faster searches to be performed. The Trac plugin need only go to the LDAP server one time to get a user's groups listing, instead of running a separate query for each group on the server.

ldapplugin/api.py

@@ -146 +146 @@
     
     def _get_user_groups(self, username):
         """Returns a list of all groups a user belongs to"""
-        ldap_groups = self._ldap.get_groups()
-        groups = []
-        for group in ldap_groups:
-            if self._ldap.is_in_group(self.util.user_attrdn(username), group):
-                m = DN_RE.search(group)
-                if m:
-                    groupname = GROUP_PREFIX + m.group('rdn')
-                    if groupname not in groups:
-                        groups.append(groupname)
-        return groups
+        return [GROUP_PREFIX + g for g in self._ldap.groups_for_user(username)]
 
 class LdapPermissionStore(Component):
     """
@@ -541 +532 @@
         groups = self.get_dn(self.basedn, 'objectclass=' + self.groupname)
         return groups
     
+    def groups_for_user(self, uid):
+        """Obtain group listing for a particular uid."""
+        if self.groupmemberisdn:
+            dns = get_dn(self.basedn, '(uid=%s)' % uid)
+            if dns:
+                uidstr = dns[0]
+            else:
+                return False
+        else:
+            uidstr = uid
+        filter = ('(&(objectclass=%s)(%s=%s))' %
+                  (self.groupname, self.groupmember, uidstr))
+        results = self._search( self.basedn
+                              , filter
+                              , [self.groupattr]
+                              , ldap.SCOPE_SUBTREE )
+        self.log.error("Found: %s", str(results))
+        return [ attrs[self.groupattr][0] for (dn, attrs) in results ]
+
     def is_in_group(self, userdn, groupdn):
         """Tell whether the uid is member of the group"""
         if self.groupmemberisdn:

[anonymous]

I have not tested this with dns instead of uids -- I don't have that setup readily available -- but I am using it for uids on a live system and it works fine.

[Jason Dusek]

Oh, there is a pointless error logging line in my code.

ldapplugin/api.py

@@ -146 +146 @@
     
     def _get_user_groups(self, username):
         """Returns a list of all groups a user belongs to"""
-        ldap_groups = self._ldap.get_groups()
-        groups = []
-        for group in ldap_groups:
-            if self._ldap.is_in_group(self.util.user_attrdn(username), group):
-                m = DN_RE.search(group)
-                if m:
-                    groupname = GROUP_PREFIX + m.group('rdn')
-                    if groupname not in groups:
-                        groups.append(groupname)
-        return groups
+        return [GROUP_PREFIX + g for g in self._ldap.groups_for_user(username)]
 
 class LdapPermissionStore(Component):
     """
@@ -541 +532 @@
         groups = self.get_dn(self.basedn, 'objectclass=' + self.groupname)
         return groups
     
+    def groups_for_user(self, uid):
+        """Obtain group listing for a particular uid."""
+        if self.groupmemberisdn:
+            dns = get_dn(self.basedn, '(uid=%s)' % uid)
+            if dns:
+                uidstr = dns[0]
+            else:
+                return False
+        else:
+            uidstr = uid
+        filter = ('(&(objectclass=%s)(%s=%s))' %
+                  (self.groupname, self.groupmember, uidstr))
+        results = self._search( self.basedn
+                              , filter
+                              , [self.groupattr]
+                              , ldap.SCOPE_SUBTREE )
+        return [ attrs[self.groupattr][0] for (dn, attrs) in results ]
+
     def is_in_group(self, userdn, groupdn):
         """Tell whether the uid is member of the group"""
         if self.groupmemberisdn:

[jmeile@…]

Oh, there is a pointless error logging line in my code.

Thanks, your patch speed up my setup. With the original setup, I was having some LDAP errors in lots of groups to which a user didn't belonged to. It worked yes, but it was slow.

[Andrew Cooper]

Replying to jason.dusek@gmail.com:

Oh, there is a pointless error logging line in my code.

I'm using this in an Active Directory environment where uid is not the attribute for group members. So I changed that to use self.uidattr instead.

I'm not sure why I had to change an escaped ',' but the result from get_dn() needed an extra backslash to work correctly. Our user DNs are of the form "CN=Last\, First,OU=..." and the search string needed 3 backslashes, like "(member=CN=Last\\\, First,OU=...)". In any case, enjoy.

api.py

@@ -23 +23 @@
 import re
 import time
 import ldap
+import string
 
 from trac.core import *
 from trac.perm import IPermissionGroupProvider, IPermissionStore
@@ -147 +148 @@
     
     def _get_user_groups(self, username):
         """Returns a list of all groups a user belongs to"""
-        ldap_groups = self._ldap.get_groups()
-        groups = []
-        for group in ldap_groups:
-            if self._ldap.is_in_group(self.util.user_attrdn(username), group):
-                m = DN_RE.search(group)
-                if m:
-                    groupname = GROUP_PREFIX + m.group('rdn')
-                    if groupname not in groups:
-                        groups.append(groupname)
-        return groups
+        return [GROUP_PREFIX + g for g in self._ldap.groups_for_user(username)]
 
 class LdapPermissionStore(Component):
     """
@@ -554 +546 @@
         groups = self.get_dn(self.basedn, 'objectclass=' + self.groupname)
         return groups
     
+    def groups_for_user(self, uid):
+        """Obtain group listing for a particular uid."""
+        if self.groupmemberisdn:
+            dns = self.get_dn(self.basedn, '(%s=%s)' % (self.uidattr,uid) )
+            if dns:
+                uidstr = dns[0]
+            else:
+                return False
+        else:
+            uidstr = uid
+        filter = ('(&(objectclass=%s)(%s=%s))' %
+                  (self.groupname, self.groupmember, string.replace(uidstr,"\\,","\\\,") ) ) 
+    self.log.debug(filter)
+        results = self._search( self.basedn
+                              , filter
+                              , [self.groupattr]
+                              , ldap.SCOPE_SUBTREE )
+        return [ attrs[self.groupattr][0] for (dn, attrs) in results ]
+
     def is_in_group(self, userdn, groupdn):
         """Tell whether the uid is member of the group"""
         if self.groupmemberisdn: