[MGNLLDAP-76] LDAP/ADUserManager is not thread safe and can cause inconsistency in resolved user object Created: 18/Feb/13  Updated: 18/Jun/14  Resolved: 01/Mar/13

Status: Closed
Project: LDAP Connector
Component/s: None
Affects Version/s: 1.5
Fix Version/s: 1.6

Type: Task Priority: Critical
Reporter: Milan Divilek Assignee: Milan Divilek
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
dependency
relation
is related to MGNLLDAP-71 Inconsistencies in loading ldap.prope... Closed
is related to MGNLLDAP-67 Implement getAllUsers method for LDAP Closed
is related to MGNLLDAP-75 Change the access modifier on the met... Closed
Template:
Acceptance criteria:
Empty
Task DoR:
Empty
Release notes required:
Yes
Date of First Response:

 Description   

LDAP/ADUserManager is instantiated only once. So calling methods from the manager can lead to concurrent modification of global variables. We need remove global variable to avoid the issue. Unfortunately this is not possible without changing public API.

Apart from removing usage of class level variables, NameResolver.init() method needs to be modified. It has currently connection properties parameter and attributeMap parameter. AttributeMap contains all connection properties, so we don't need pass connection properties around.



 Comments   
Comment by Magnolia International [ 26/Feb/13 ]

Some comments:

  • Javadoc needs some fixing
  • update* methods should be called populate*, they're not updating anything that pre-exists to their call.
  • corollated: I don't think you need to pass groupList and roleList, just return them (in which case you might want to call those methods create*) - less arguments to pass around will help.
  • if getUserFilter(name) isn't used anymore, drop it. The way you did this, subclasses which override this method will still compile, and users won't realize their mistake. What you could do is deprecate it, make it throw an exception and mark final - subclasses won't compile. But I doubt that's ever been overridden, and there are more important changes in the API for which we're not going through the trouble, so I'd just drop it.
  • if you need to check that attributeMap isn't null (I don't think you need to, since it's passed from getUser()), do that earlier than in the getUserFilter method.
  • In this code:
  1. fix the typo in the log message ("can't initialize", not "can't initialized".
  2. be consistent between your actual variable name and how you call it in the log message.
                if (root == null) {
                    log.warn("Can't initialized initialSearchAttributes. Check your ldap/ad properties file.");
    
  • There is far too much redundancy between getUser(name) and getAllUsers(). Factor it out.

I think that's about it for now. Let me know if you need clarification or help with any of the above.

Comment by Magnolia International [ 01/Mar/13 ]

Good job, thanks. I did a few more changes, please have a look at them. You then have my blessing to close this issue

Comment by Magnolia International [ 01/Mar/13 ]

Mention in release notes - new "major" version due to some necessary API changes. (Re-enabling stuff that was accidentally broken in 1.5)

Generated at Mon Feb 12 02:21:20 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.