james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eric Charles <e...@apache.org>
Subject Re: [jira] [Commented] (JAMES-1313) more effective getUserByName(String name) in org.apache.james.user.ldap.ReadOnlyUsersLDAPRepository
Date Mon, 02 Apr 2012 09:57:47 GMT
Hi Peter,

Best to bring your comment on the JAMES-1313 so we can keep trace of it:
- For now I have to tag your mail in my folder, but there's a high risk 
it will get forgotten...
- You talk about a patch but the mailing list removes attachment. I you 
have a patch, you have to upload it via JAMES-1313 and grant ASF for th 
rights.

Doing such, the information is centralized and chance to come to a 
solution will be higher.

Thx again,

Eric


On 28/03/12 16:42, Peter Kvokacka wrote:
> Hello guys
>
> I'm not an LDAP expert. As I wrote, the patch I provided, works fine for
> me and has the restriction. As it seems it doesn't work for Kevin, I
> made quick changes to support his case as well.
> I tested it quickly on my james 3.0 beta3 setup and it works. I suggest
> these changes to beta4 code base. I don't have time to test it on beta4,
> because to get beta4 work can take some time, I suppose, because of many
> changes between beta3 and beta4.
>
> The main idea of the patch is to replace search with potencialy huge
> result-set (all user in your userBase) with single search for one user.
> Therefore I created new method searchAndBuildUser() instead buildUser()
> and I used it in getUserByName(). It makes ldap search like this
> ldapContext.search(userBase,
> "(&(${userIdAttribute}=${name})(objectClass=${userObjectClass}))")
> where userBase, userIdAttribure and userObjectClass are attributes from
> configuration and name is "mail user name" as Kevin named it. I'm not
> sure how quick would by the search on LDAP with many users, certainly
> longer then my previous suggestion, but still I think more efficient
> than original implementation.
>
> public User getUserByName(String name) throws UsersRepositoryException {
> try {
> return searchAndBuildUser(name);
> } catch (NamingException e) {
> log.error("Unable to retrieve user from ldap", e);
> throw new UsersRepositoryException("Unable to retrieve user from ldap", e);
>
> }
> }
>
> private ReadOnlyLDAPUser searchAndBuildUser(String name) throws
> NamingException {
> SearchControls sc = new SearchControls();
> sc.setSearchScope(SearchControls.SUBTREE_SCOPE);
> sc.setReturningAttributes(new String[] { userIdAttribute });
> sc.setCountLimit(1);
>
> StringBuilder builderFilter = new StringBuilder("(&(");
> builderFilter.append(userIdAttribute).append("=").append(name).append(")")
> .append("(objectClass=").append(userObjectClass).append("))");
>
> NamingEnumeration<SearchResult> sr = ldapContext.search(userBase,
> builderFilter.toString(),
> sc);
>
> if (!sr.hasMore())
> return null;
>
> SearchResult r = sr.next();
> Attribute userName = r.getAttributes().get(userIdAttribute);
>
> if (!restriction.isActivated()
> || userInGroupsMembershipList(r.getNameInNamespace(), restriction
> .getGroupMembershipLists(ldapContext)))
> return new ReadOnlyLDAPUser(userName.get().toString(),
> r.getNameInNamespace(), ldapContext);
>
> return null;
> }
>
> Another idea would be to implement some cache mechanism which will cache
> result of buildUserCollection() and do LDAP search only if necessary.
> Peter
>
> On 26. 3. 2012 14:58, Kevin Dion (Commented) (JIRA) wrote:
>> [
>> https://issues.apache.org/jira/browse/JAMES-1313?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13238345#comment-13238345
>> ]
>>
>> Kevin Dion commented on JAMES-1313:
>> -----------------------------------
>>
>> The restriction added by requiring the userIdAttribute to be the first
>> part of the DN is overly restrictive, at least in some Windows
>> environments. In Microsoft AD LDAPs, the dn is usually of the form
>> 'cn=Charlie Brown,dc=domain,dc=org' where cn is the 'Common', or full
>> name of the individual. The mail username would usually be specified
>> elsewhere in the user object, and the flexibility of allowing for
>> attributes other than the dn for login is essential in many instances.
>>
>>> 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
>>> Assignee: Norman Maurer
>>> Priority: Minor
>>> Labels: patch
>>> Fix For: 3.0-beta4
>>>
>>> Attachments: ReadOnlyUsersLDAPRepository.java.patch
>>>
>>>
>>> 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.
>> If you think it was sent incorrectly, please contact your JIRA
>> administrators:
>> https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
>> For more information on JIRA, see: http://www.atlassian.com/software/jira
>>
>>
>>
>
>

-- 
eric | http://about.echarles.net | @echarles

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message