[ 
https://issues.apache.org/jira/browse/JAMES-1313?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Peter Kvokacka updated JAMES-1313:
----------------------------------

    Description: 
Hello 

I'd like to use james in my current project, but I find LDAP implementation of 
usersRepository to be not very effective.
Especially method getUserByName(String name) in 
org.apache.james.user.ldap.ReadOnlyUsersLDAPRepository seems to search all 
users in LDAP with userBase and after that it goes through the result in memory 
and looking for specific user. Which produce search like this with potencialy 
big resultset:
  SEARCH REQ conn=26 op=6 msgID=7 base="ou=people,dc=mycompany,dc=sk" 
scope=wholeSubtree filter="(objectClass=inetOrgPerson)" 
attrs="distinguishedName"
  SEARCH RES conn=26 op=6 msgID=7 result=0 nentries=438 etime=169
  SEARCH REQ conn=26 op=7 msgID=8 
base="uid=somebody,ou=people,dc=mycompany,dc=sk" scope=baseObject 
filter="(objectClass=*)" attrs="ALL"
  SEARCH RES conn=26 op=7 msgID=8 result=0 nentries=1 etime=1
  ... X more, where X is size-1 of userBase subtree

I suggest a patch that (at least in my case) does simple search instead:
  <repository name="LocalUsers"
      class="org.apache.james.user.ldap.ReadOnlyUsersLDAPRepository"
      ldapHost="ldaps://ldap.mycomapny.local:1636"
      principal="cn=admin"
      credentials="***"
      userBase="ou=people,dc=mycompany,dc=sk"
      userIdAttribute="uid"
      userObjectClass="inetOrgPerson"/>


  SEARCH REQ conn=26 op=1 msgID=2 
base="uid=test0123,ou=people,dc=mycompany,dc=sk" scope=baseObject 
filter="(objectClass=inetOrgPerson)" attrs="uid"
  SEARCH RES conn=26 op=1 msgID=2 result=0 nentries=1 etime=1

There is only one assumption that distinguishedName for each entry in userBase 
is "userIdAttribute=$name,userBase", where $name is username. I don't think of 
it as of a strong restriction, but you should consider that and decide for 
yourself. It works just fine for me.

Also it looks like getUserByNameCaseInsensitive(String name) is not used 
anywhere, so you can stick with current implementation for now.

Peter


Index: src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java
===================================================================
--- src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java 
(revision 1169673)
+++ src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java 
(working copy)
@@ -351,16 +351,23 @@
      *             Propagated by the underlying LDAP communication layer.
      */
     private ReadOnlyLDAPUser buildUser(String userDN) throws NamingException {
-        ReadOnlyLDAPUser result;
 
-        Attributes userAttributes = 
ldapConnection.getLdapContext().getAttributes(userDN);
+        SearchControls sc = new SearchControls();
+        sc.setSearchScope(SearchControls.OBJECT_SCOPE);
+        sc.setReturningAttributes(new String[] {userIdAttribute});
+        sc.setCountLimit(1);
+  
+        NamingEnumeration<SearchResult> sr = 
ldapConnection.getLdapContext().search(userDN, "(objectClass=" + 
userObjectClass + ")", sc);
+        
+        if (!sr.hasMore())
+            return null;
+  
+        Attributes userAttributes = sr.next().getAttributes();
         Attribute userName = userAttributes.get(userIdAttribute);
+        
+        return new ReadOnlyLDAPUser(userName.get().toString(), userDN, 
ldapHost);
+  }    
 
-        result = new ReadOnlyLDAPUser(userName.get().toString(), userDN, 
ldapHost);
-
-        return result;
-    }
-
     /*
      * (non-Javadoc)
      * 
@@ -425,23 +432,14 @@
      */
     public User getUserByName(String name) throws UsersRepositoryException {
         try {
-            Iterator<ReadOnlyLDAPUser> userIt = 
buildUserCollection(getValidUsers()).iterator();
-            while (userIt.hasNext()) {
-                ReadOnlyLDAPUser u = userIt.next();
-                if (u.getUserName().equals(name)) {
-                    return u;
-                }
-            }
-
+            return buildUser(userIdAttribute + "=" + name + "," + userBase); 
         } catch (NamingException e) {
             log.error("Unable to retrieve user from ldap", e);
             throw new UsersRepositoryException("Unable to retrieve user from 
ldap", e);
-
+  
         }
-        return null;
+    }        
 
-    }
-
     /*
      * (non-Javadoc)
      * 


  was:
Hello 

I'd like to use james in my current project, but I find LDAP implementation of 
usersRepository to be not very effective.
Especially method getUserByName(String name) in 
org.apache.james.user.ldap.ReadOnlyUsersLDAPRepository seems to search all user 
in LDAP with userBase and after that it goes through the result in memory and 
looking for specific user. Which produce search like this with potencialy big 
resultset:
  SEARCH REQ conn=26 op=6 msgID=7 base="ou=people,dc=mycompany,dc=sk" 
scope=wholeSubtree filter="(objectClass=inetOrgPerson)" 
attrs="distinguishedName"
  SEARCH RES conn=26 op=6 msgID=7 result=0 nentries=438 etime=169
  SEARCH REQ conn=26 op=7 msgID=8 
base="uid=somebody,ou=people,dc=mycompany,dc=sk" scope=baseObject 
filter="(objectClass=*)" attrs="ALL"
  SEARCH RES conn=26 op=7 msgID=8 result=0 nentries=1 etime=1
  ... X more, where X is size-1 of userBase subtree

I suggest a patch that (at least in my case) does simple search instead:
  <repository name="LocalUsers"
      class="org.apache.james.user.ldap.ReadOnlyUsersLDAPRepository"
      ldapHost="ldaps://ldap.mycomapny.local:1636"
      principal="cn=admin"
      credentials="***"
      userBase="ou=people,dc=mycompany,dc=sk"
      userIdAttribute="uid"
      userObjectClass="inetOrgPerson"/>


  SEARCH REQ conn=26 op=1 msgID=2 
base="uid=test0123,ou=people,dc=mycompany,dc=sk" scope=baseObject 
filter="(objectClass=inetOrgPerson)" attrs="uid"
  SEARCH RES conn=26 op=1 msgID=2 result=0 nentries=1 etime=1

There is only one assumption that distinguishedName for each entry in userBase 
is "userIdAttribute=$name,userBase", where $name is username. I don't think of 
it as of a strong restriction, but you should consider that and decide for 
yourself. It works just fine for me.

Also it looks like getUserByNameCaseInsensitive(String name) is not used 
anywhere, so you can stick with current implementation for now.

Peter


Index: src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java
===================================================================
--- src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java 
(revision 1169673)
+++ src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java 
(working copy)
@@ -351,16 +351,23 @@
      *             Propagated by the underlying LDAP communication layer.
      */
     private ReadOnlyLDAPUser buildUser(String userDN) throws NamingException {
-        ReadOnlyLDAPUser result;
 
-        Attributes userAttributes = 
ldapConnection.getLdapContext().getAttributes(userDN);
+        SearchControls sc = new SearchControls();
+        sc.setSearchScope(SearchControls.OBJECT_SCOPE);
+        sc.setReturningAttributes(new String[] {userIdAttribute});
+        sc.setCountLimit(1);
+  
+        NamingEnumeration<SearchResult> sr = 
ldapConnection.getLdapContext().search(userDN, "(objectClass=" + 
userObjectClass + ")", sc);
+        
+        if (!sr.hasMore())
+            return null;
+  
+        Attributes userAttributes = sr.next().getAttributes();
         Attribute userName = userAttributes.get(userIdAttribute);
+        
+        return new ReadOnlyLDAPUser(userName.get().toString(), userDN, 
ldapHost);
+  }    
 
-        result = new ReadOnlyLDAPUser(userName.get().toString(), userDN, 
ldapHost);
-
-        return result;
-    }
-
     /*
      * (non-Javadoc)
      * 
@@ -425,23 +432,14 @@
      */
     public User getUserByName(String name) throws UsersRepositoryException {
         try {
-            Iterator<ReadOnlyLDAPUser> userIt = 
buildUserCollection(getValidUsers()).iterator();
-            while (userIt.hasNext()) {
-                ReadOnlyLDAPUser u = userIt.next();
-                if (u.getUserName().equals(name)) {
-                    return u;
-                }
-            }
-
+            return buildUser(userIdAttribute + "=" + name + "," + userBase); 
         } catch (NamingException e) {
             log.error("Unable to retrieve user from ldap", e);
             throw new UsersRepositoryException("Unable to retrieve user from 
ldap", e);
-
+  
         }
-        return null;
+    }        
 
-    }
-
     /*
      * (non-Javadoc)
      * 



> more effective getUserByName(String name) in 
> org.apache.james.user.ldap.ReadOnlyUsersLDAPRepository 
> ----------------------------------------------------------------------------------------------------
>
>                 Key: JAMES-1313
>                 URL: https://issues.apache.org/jira/browse/JAMES-1313
>             Project: JAMES Server
>          Issue Type: Improvement
>          Components: UsersStore & UsersRepository
>    Affects Versions: 3.0-beta3
>            Reporter: Peter Kvokacka
>            Priority: Minor
>              Labels: patch
>             Fix For: 3.0-beta3
>
>
> Hello 
> I'd like to use james in my current project, but I find LDAP implementation 
> of usersRepository to be not very effective.
> Especially method getUserByName(String name) in 
> org.apache.james.user.ldap.ReadOnlyUsersLDAPRepository seems to search all 
> users in LDAP with userBase and after that it goes through the result in 
> memory and looking for specific user. Which produce search like this with 
> potencialy big resultset:
>   SEARCH REQ conn=26 op=6 msgID=7 base="ou=people,dc=mycompany,dc=sk" 
> scope=wholeSubtree filter="(objectClass=inetOrgPerson)" 
> attrs="distinguishedName"
>   SEARCH RES conn=26 op=6 msgID=7 result=0 nentries=438 etime=169
>   SEARCH REQ conn=26 op=7 msgID=8 
> base="uid=somebody,ou=people,dc=mycompany,dc=sk" scope=baseObject 
> filter="(objectClass=*)" attrs="ALL"
>   SEARCH RES conn=26 op=7 msgID=8 result=0 nentries=1 etime=1
>   ... X more, where X is size-1 of userBase subtree
> I suggest a patch that (at least in my case) does simple search instead:
>   <repository name="LocalUsers"
>       class="org.apache.james.user.ldap.ReadOnlyUsersLDAPRepository"
>       ldapHost="ldaps://ldap.mycomapny.local:1636"
>       principal="cn=admin"
>       credentials="***"
>       userBase="ou=people,dc=mycompany,dc=sk"
>       userIdAttribute="uid"
>       userObjectClass="inetOrgPerson"/>
>   SEARCH REQ conn=26 op=1 msgID=2 
> base="uid=test0123,ou=people,dc=mycompany,dc=sk" scope=baseObject 
> filter="(objectClass=inetOrgPerson)" attrs="uid"
>   SEARCH RES conn=26 op=1 msgID=2 result=0 nentries=1 etime=1
> There is only one assumption that distinguishedName for each entry in 
> userBase is "userIdAttribute=$name,userBase", where $name is username. I 
> don't think of it as of a strong restriction, but you should consider that 
> and decide for yourself. It works just fine for me.
> Also it looks like getUserByNameCaseInsensitive(String name) is not used 
> anywhere, so you can stick with current implementation for now.
> Peter
> Index: 
> src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java
> ===================================================================
> --- src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java 
> (revision 1169673)
> +++ src/main/java/org/apache/james/user/ldap/ReadOnlyUsersLDAPRepository.java 
> (working copy)
> @@ -351,16 +351,23 @@
>       *             Propagated by the underlying LDAP communication layer.
>       */
>      private ReadOnlyLDAPUser buildUser(String userDN) throws NamingException 
> {
> -        ReadOnlyLDAPUser result;
>  
> -        Attributes userAttributes = 
> ldapConnection.getLdapContext().getAttributes(userDN);
> +        SearchControls sc = new SearchControls();
> +        sc.setSearchScope(SearchControls.OBJECT_SCOPE);
> +        sc.setReturningAttributes(new String[] {userIdAttribute});
> +        sc.setCountLimit(1);
> +  
> +        NamingEnumeration<SearchResult> sr = 
> ldapConnection.getLdapContext().search(userDN, "(objectClass=" + 
> userObjectClass + ")", sc);
> +        
> +        if (!sr.hasMore())
> +            return null;
> +  
> +        Attributes userAttributes = sr.next().getAttributes();
>          Attribute userName = userAttributes.get(userIdAttribute);
> +        
> +        return new ReadOnlyLDAPUser(userName.get().toString(), userDN, 
> ldapHost);
> +  }    
>  
> -        result = new ReadOnlyLDAPUser(userName.get().toString(), userDN, 
> ldapHost);
> -
> -        return result;
> -    }
> -
>      /*
>       * (non-Javadoc)
>       * 
> @@ -425,23 +432,14 @@
>       */
>      public User getUserByName(String name) throws UsersRepositoryException {
>          try {
> -            Iterator<ReadOnlyLDAPUser> userIt = 
> buildUserCollection(getValidUsers()).iterator();
> -            while (userIt.hasNext()) {
> -                ReadOnlyLDAPUser u = userIt.next();
> -                if (u.getUserName().equals(name)) {
> -                    return u;
> -                }
> -            }
> -
> +            return buildUser(userIdAttribute + "=" + name + "," + userBase); 
>          } catch (NamingException e) {
>              log.error("Unable to retrieve user from ldap", e);
>              throw new UsersRepositoryException("Unable to retrieve user from 
> ldap", e);
> -
> +  
>          }
> -        return null;
> +    }        
>  
> -    }
> -
>      /*
>       * (non-Javadoc)
>       * 

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to