Merged V3.2 to HEAD

18846: ETHREEOH-4233: LDAP sync now synchronizes group display names
      - New ldap.synchronization.groupDisplayNameAttributeName property provides name of LDAP attribute


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@18856 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

config/alfresco/subsystems/Authentication/common-ldap-context.xml

@@ -290,7 +290,7 @@
         the repository, the value is the attribute name from the user/inetOrgPerson/.. object in the
         LDAP repository.     
         -->
-        <property name="attributeMapping">
+        <property name="personAttributeMapping">
             <map>
                 <entry key="cm:userName">
                     <!-- Must match the same attribute as userIdAttributeName -->
@@ -324,7 +324,7 @@
         </property>
         <!-- Set a default home folder provider -->
         <!-- Defaults only apply for values above -->
-        <property name="attributeDefaults">
+        <property name="personAttributeDefaults">
             <map>
                 <entry key="cm:homeFolderProvider">
                     <value>${ldap.synchronization.defaultHomeFolderProvider}</value>
@@ -332,6 +332,25 @@
             </map>
         </property>
     
+        <!--
+        This property defines a mapping between attributes held on LDAP group objects and
+        the properties of authorities held in the repository. The key is the QName of an attribute in
+        the repository, the value is the attribute name from the group object in the LDAP repository.     
+        -->
+        <property name="groupAttributeMapping">
+            <map>
+                <entry key="cm:authorityName">
+                    <!-- Must match the same attribute as groupIdAttributeName -->
+                    <value>${ldap.synchronization.groupIdAttributeName}</value>
+                </entry>
+                <entry key="cm:authorityDisplayName">
+                    <!-- OpenLDAP: "description" -->
+                    <!-- Active Directory: "displayName" -->
+                    <value>${ldap.synchronization.groupDisplayNameAttributeName}</value>
+                </entry>
+            </map>
+        </property>
+
         <property name="enableProgressEstimation">
             <value>${ldap.synchronization.enableProgressEstimation}</value>
         </property>

config/alfresco/subsystems/Authentication/ldap-ad/ldap-ad-authentication.properties

@@ -98,9 +98,12 @@ ldap.synchronization.userOrganizationalIdAttributeName=company
 # The default home folder provider to use for people created via LDAP import
 ldap.synchronization.defaultHomeFolderProvider=userHomesHomeFolderProvider
 
-# The attribute on LDAP group objects to map to the gid property in Alfrecso
+# The attribute on LDAP group objects to map to the authority name property in Alfresco
 ldap.synchronization.groupIdAttributeName=cn
 
+# The attribute on LDAP group objects to map to the authority display name property in Alfresco
+ldap.synchronization.groupDisplayNameAttributeName=displayName
+
 # The group type in LDAP
 ldap.synchronization.groupType=group
 

config/alfresco/subsystems/Authentication/ldap/ldap-authentication.properties

@@ -104,9 +104,12 @@ ldap.synchronization.userOrganizationalIdAttributeName=o
 # The default home folder provider to use for people created via LDAP import
 ldap.synchronization.defaultHomeFolderProvider=userHomesHomeFolderProvider
 
-# The attribute on LDAP group objects to map to the gid property in Alfrecso
+# The attribute on LDAP group objects to map to the authority name property in Alfresco
 ldap.synchronization.groupIdAttributeName=cn
 
+# The attribute on LDAP group objects to map to the authority display name property in Alfresco
+ldap.synchronization.groupDisplayNameAttributeName=description
+
 # The group type in LDAP
 ldap.synchronization.groupType=groupOfNames
 

source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizer.java

@@ -549,7 +549,7 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
         class Analyzer implements Worker<NodeDescription>
         {
             private final Set<String> allZoneAuthorities = new TreeSet<String>();
-            private final Set<String> groupsToCreate = new TreeSet<String>();
+            private final Map<String, String> groupsToCreate = new TreeMap<String, String>();
             private final Map<String, Set<String>> groupAssocsToCreate = new TreeMap<String, Set<String>>();
             private final Map<String, Set<String>> groupAssocsToDelete = new TreeMap<String, Set<String>>();
             private long latestTime;
@@ -569,7 +569,7 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                 return this.allZoneAuthorities;
             }
 
-            public Set<String> getGroupsToCreate()
+            public Map<String, String> getGroupsToCreate()
             {
                 return this.groupsToCreate;
             }
@@ -608,7 +608,7 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                 if (groupZones == null)
                 {
                     // The group did not exist at all
-                    addAssociations(groupName, group.getChildAssociations(), false);
+                    addGroup(group);
                 }
                 else
                 {
@@ -632,7 +632,7 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                     if (groupZones.contains(zoneId) || intersection.isEmpty())
                     {
                         // The group already existed in this zone or no valid zone: update the group
-                        updateAssociations(group, groupName);
+                        updateGroup(group);
                     }
                     else
                     {
@@ -652,8 +652,9 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                                             + "'. This group was previously created through synchronization with a lower priority user registry.");
                         }
                         ChainingUserRegistrySynchronizer.this.authorityService.deleteAuthority(groupName);
+
                         // create the group
-                        addAssociations(groupName, group.getChildAssociations(), false);
+                        addGroup(group);
                     }
                 }
 
@@ -668,8 +669,20 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                 }
             }
 
-            private synchronized void updateAssociations(NodeDescription group, String groupName)
+            private void updateGroup(NodeDescription group)
             {
+                PropertyMap groupProperties = group.getProperties();
+                String groupName = (String) groupProperties.get(ContentModel.PROP_AUTHORITY_NAME);
+                String groupDisplayName = (String) groupProperties.get(ContentModel.PROP_AUTHORITY_DISPLAY_NAME);
+                if (groupDisplayName == null)
+                {
+                    groupDisplayName = groupName;
+                }
+                // Update the display name now
+                ChainingUserRegistrySynchronizer.this.authorityService.setAuthorityDisplayName(groupName,
+                        groupDisplayName);
+
+                // Work out the association differences
                 Set<String> oldChildren = ChainingUserRegistrySynchronizer.this.authorityService
                         .getContainedAuthorities(null, groupName, true);
                 Set<String> newChildren = group.getChildAssociations();
@@ -677,17 +690,33 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                 Set<String> toAdd = new TreeSet<String>(newChildren);
                 toDelete.removeAll(newChildren);
                 toAdd.removeAll(oldChildren);
-                addAssociations(groupName, toAdd, true);
+                synchronized (this)
-                deleteAssociations(groupName, toDelete);
+                {
+                    addAssociations(groupName, toAdd);
+                    deleteAssociations(groupName, toDelete);
+                }
             }
 
-            private synchronized void addAssociations(String groupName, Set<String> children, boolean existed)
+            private void addGroup(NodeDescription group)
+            {
+                PropertyMap groupProperties = group.getProperties();
+                String groupName = (String) groupProperties.get(ContentModel.PROP_AUTHORITY_NAME);
+                String groupDisplayName = (String) groupProperties.get(ContentModel.PROP_AUTHORITY_DISPLAY_NAME);
+                if (groupDisplayName == null)
+                {
+                    groupDisplayName = groupName;
+                }
+
+                synchronized (this)
+                {
+                    this.groupsToCreate.put(groupName, groupDisplayName);
+                    addAssociations(groupName, group.getChildAssociations());
+                }
+            }
+
+            private synchronized void addAssociations(String groupName, Set<String> children)
             {
                 this.allZoneAuthorities.add(groupName);
-                if (!existed)
-                {
-                    this.groupsToCreate.add(groupName);
-                }
                 // Add an entry for the parent itself, in case it is a root group
                 Set<String> parents = this.groupAssocsToCreate.get(groupName);
                 if (parents == null)
@@ -712,7 +741,7 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                 for (String child : children)
                 {
                     // Make sure each child features as a key in the creation map
-                    addAssociations(child, Collections.<String> emptySet(), true);
+                    addAssociations(child, Collections.<String> emptySet());
 
                     Set<String> parents = this.groupAssocsToDelete.get(child);
                     if (parents == null)
@@ -775,7 +804,7 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                 }
 
                 // Add the groups and their parent associations in depth-first order
-                final Set<String> groupsToCreate = groupAnalyzer.getGroupsToCreate();
+                final Map<String, String> groupsToCreate = groupAnalyzer.getGroupsToCreate();
                 BatchProcessor<Map.Entry<String, Set<String>>> groupCreator = new BatchProcessor<Map.Entry<String, Set<String>>>(
                         ChainingUserRegistrySynchronizer.logger, this.retryingTransactionHelper, this.ruleService,
                         this.applicationEventPublisher, sortedGroupAssociations.entrySet(), zone
@@ -793,7 +822,8 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                         Set<String> parents = entry.getValue();
                         String child = entry.getKey();
 
-                        if (groupsToCreate.contains(child))
+                        String groupDisplayName = groupsToCreate.get(child);
+                        if (groupDisplayName != null)
                         {
                             String groupShortName = ChainingUserRegistrySynchronizer.this.authorityService
                                     .getShortName(child);
@@ -804,7 +834,7 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                             }
                             // create the group
                             ChainingUserRegistrySynchronizer.this.authorityService.createAuthority(AuthorityType
-                                    .getAuthorityType(child), groupShortName, groupShortName, zoneSet);
+                                    .getAuthorityType(child), groupShortName, groupDisplayName, zoneSet);
                         }
                         if (!parents.isEmpty())
                         {
@@ -829,7 +859,7 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                                 // Let's force a transaction retry if a parent doesn't exist. It may be because we are
                                 // waiting for another worker thread to create it
                                 throw new BatchUpdateException().initCause(e);
-                            }                            
+                            }
                         }
                         Set<String> parentsToDelete = groupAssocsToDelete.get(child);
                         if (parentsToDelete != null && !parentsToDelete.isEmpty())

source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizerTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005-2009 Alfresco Software Limited.
+ * Copyright (C) 2005-2010 Alfresco Software Limited.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -257,7 +257,7 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
             newPerson("U1", "changeofemail@alfresco.com"), newPerson("U6")
         }, new NodeDescription[]
         {
-            newGroup("G1", "U1", "U6"), newGroup("G2", "U1"), newGroup("G5", "U6")
+            newGroup("G1", "U1", "U6"), newGroup("G2", "U1"), newGroupWithDisplayName("G5", "Amazing Group", "U6")
         }), new MockUserRegistry("Z2", new NodeDescription[]
         {
             newPerson("U1", "shouldbeignored@alfresco.com"), newPerson("U5", "u5email@alfresco.com"), newPerson("U6")
@@ -282,6 +282,7 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
                 assertExists("Z1", "G3", "U2", "G4", "G5");
                 assertExists("Z1", "G4");
                 assertExists("Z1", "G5", "U6");
+                assertGroupDisplayNameEquals("G5", "Amazing Group");
                 assertExists("Z2", "U3");
                 assertExists("Z2", "U4");
                 assertExists("Z2", "U5");
@@ -330,7 +331,7 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
             newPerson("U1", "somenewemail@alfresco.com"), newPerson("U3"), newPerson("U6")
         }, new NodeDescription[]
         {
-            newGroup("G2", "U1", "U3", "U4", "U6"), newGroup("G6", "U3", "U4", "G7"), newGroup("G7", "U4", "U5")
+            newGroup("G2", "U1", "U3", "U4", "U6"), newGroup("G6", "U3", "U4", "G7"), newGroupWithDisplayName("G7", "Late Arrival", "U4", "U5")
         }));
         this.synchronizer.synchronize(true, true, true);
         this.retryingTransactionHelper.doInTransaction(new RetryingTransactionCallback<Object>()
@@ -352,6 +353,7 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
                 assertNotExists("U4");
                 assertNotExists("U5");
                 assertExists("Z2", "G7");
+                assertGroupDisplayNameEquals("G7", "Late Arrival");
                 return null;
             }
         }, false, true);
@@ -408,11 +410,28 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
      * @return the node description
      */
     private NodeDescription newGroup(String name, String... members)
+    {
+        return newGroupWithDisplayName(name, name, members);
+    }
+
+    /**
+     * Constructs a description of a test group with a display name.
+     * 
+     * @param name
+     *            the name
+     * @param displayName
+     *            the display name
+     * @param members
+     *            the members
+     * @return the node description
+     */
+    private NodeDescription newGroupWithDisplayName(String name, String displayName, String... members)
     {
         String longName = longName(name);
         NodeDescription group = new NodeDescription(longName);
         PropertyMap properties = group.getProperties();
         properties.put(ContentModel.PROP_AUTHORITY_NAME, longName);
+        properties.put(ContentModel.PROP_AUTHORITY_DISPLAY_NAME, displayName);
         if (members.length > 0)
         {
             Set<String> assocs = group.getChildAssociations();
@@ -424,7 +443,7 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
         group.setLastModified(new Date());
         return group;
     }
-
+    
     /**
      * Constructs a description of a test person with default email (userName@alfresco.com)
      * 
@@ -529,6 +548,19 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
         assertEquals(email, this.nodeService.getProperty(personRef, ContentModel.PROP_EMAIL));
     }
 
+    /**
+     * Asserts that a group's display name has the expected value.
+     * 
+     * @param personName
+     *            the person name
+     * @param email
+     *            the email
+     */
+    private void assertGroupDisplayNameEquals(String name, String displayName)
+    {
+        assertEquals(displayName, this.authorityService.getAuthorityDisplayName(longName(name)));
+    }
+
     /**
      * Converts the given short name to a full authority name, assuming that those short names beginning with 'G'
      * correspond to groups and all others correspond to users.

source/java/org/alfresco/repo/security/sync/ldap/LDAPUserRegistry.java

@@ -29,8 +29,11 @@ import java.text.MessageFormat;
 import java.text.ParseException;
 import java.text.SimpleDateFormat;
 import java.util.AbstractCollection;
+import java.util.Arrays;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Date;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Locale;
@@ -79,7 +82,7 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
 
     /** The logger. */
     private static Log logger = LogFactory.getLog(LDAPUserRegistry.class);
-    
+
     /** The regular expression that will match the attribute at the end of a range. */
     private static final Pattern PATTERN_RANGE_END = Pattern.compile(";range=[0-9]+-\\*");
 
@@ -128,14 +131,20 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
     /** The ldap initial context factory. */
     private LDAPInitialDirContextFactory ldapInitialContextFactory;
 
-    /** The attribute mapping. */
-    private Map<String, String> attributeMapping;
-
     /** The namespace service. */
     private NamespaceService namespaceService;
 
-    /** The attribute defaults. */
+    /** The person attribute mapping. */
-    private Map<String, String> attributeDefaults;
+    private Map<String, String> personAttributeMapping;
+
+    /** The person attribute defaults. */
+    private Map<String, String> personAttributeDefaults = Collections.emptyMap();
+
+    /** The group attribute mapping. */
+    private Map<String, String> groupAttributeMapping;
+
+    /** The group attribute defaults. */
+    private Map<String, String> groupAttributeDefaults = Collections.emptyMap();
 
     /**
      * The query batch size. If positive, indicates that RFC 2696 paged results should be used to split query results
@@ -406,17 +415,6 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
         this.ldapInitialContextFactory = ldapInitialDirContextFactory;
     }
 
-    /**
-     * Sets the attribute defaults.
-     * 
-     * @param attributeDefaults
-     *            the attribute defaults
-     */
-    public void setAttributeDefaults(Map<String, String> attributeDefaults)
-    {
-        this.attributeDefaults = attributeDefaults;
-    }
-
     /**
      * Sets the namespace service.
      * 
@@ -429,14 +427,47 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
     }
 
     /**
-     * Sets the attribute mapping.
+     * Sets the person attribute defaults.
      * 
-     * @param attributeMapping
+     * @param personAttributeDefaults
-     *            the attribute mapping
+     *            the person attribute defaults
      */
-    public void setAttributeMapping(Map<String, String> attributeMapping)
+    public void setPersonAttributeDefaults(Map<String, String> personAttributeDefaults)
     {
-        this.attributeMapping = attributeMapping;
+        this.personAttributeDefaults = personAttributeDefaults;
+    }
+
+    /**
+     * Sets the person attribute mapping.
+     * 
+     * @param personAttributeMapping
+     *            the person attribute mapping
+     */
+    public void setPersonAttributeMapping(Map<String, String> personAttributeMapping)
+    {
+        this.personAttributeMapping = personAttributeMapping;
+    }
+
+    /**
+     * Sets the group attribute defaults.
+     * 
+     * @param groupAttributeDefaults
+     *            the group attribute defaults
+     */
+    public void setGroupAttributeDefaults(Map<String, String> groupAttributeDefaults)
+    {
+        this.groupAttributeDefaults = groupAttributeDefaults;
+    }
+
+    /**
+     * Sets the group attribute mapping.
+     * 
+     * @param groupAttributeMapping
+     *            the group attribute mapping
+     */
+    public void setGroupAttributeMapping(Map<String, String> groupAttributeMapping)
+    {
+        this.groupAttributeMapping = groupAttributeMapping;
     }
 
     /**
@@ -450,7 +481,7 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
     {
         this.queryBatchSize = queryBatchSize;
     }
-    
+
     /**
      * Sets the attribute batch size.
      * 
@@ -478,27 +509,24 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
      */
     public void afterPropertiesSet() throws Exception
     {
-        Set<String> userAttributeSet = new TreeSet<String>();
+        if (this.personAttributeMapping == null)
-        userAttributeSet.add(this.userIdAttributeName);
-        userAttributeSet.add(this.modifyTimestampAttributeName);
-        for (String attribute : this.attributeMapping.values())
         {
-            if (attribute != null)
+            this.personAttributeMapping = new HashMap<String, String>(5);
-            {
-                userAttributeSet.add(attribute);
-            }
         }
-        this.userAttributeNames = new String[userAttributeSet.size()];
+        this.personAttributeMapping.put(ContentModel.PROP_USERNAME.toPrefixString(this.namespaceService),
-        userAttributeSet.toArray(this.userAttributeNames);
+                this.userIdAttributeName);
+        this.userAttributeNames = getAttributeNames(this.personAttributeMapping);
 
         // Include a range restriction for the multi-valued member attribute if this is enabled
-        this.groupAttributeNames = new String[]
+        if (this.groupAttributeMapping == null)
         {
-            this.groupIdAttributeName,
+            this.groupAttributeMapping = new HashMap<String, String>(5);
-            this.modifyTimestampAttributeName,
+        }
-            this.attributeBatchSize > 0 ? this.memberAttributeName + ";range=0-" + (this.attributeBatchSize - 1)
+        this.groupAttributeMapping.put(ContentModel.PROP_AUTHORITY_NAME.toPrefixString(this.namespaceService),
-                    : this.memberAttributeName
+                this.groupIdAttributeName);
-        };
+        this.groupAttributeNames = getAttributeNames(this.groupAttributeMapping,
+                this.attributeBatchSize > 0 ? this.memberAttributeName + ";range=0-" + (this.attributeBatchSize - 1)
+                        : this.memberAttributeName);
     }
 
     /*
@@ -648,7 +676,11 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
                 NodeDescription group = lookup.get(gid);
                 if (group == null)
                 {
-                    group = new NodeDescription(result.getNameInNamespace());
+                    // Apply the mapped properties to the node description
+                    group = mapToNode(LDAPUserRegistry.this.groupAttributeMapping,
+                            LDAPUserRegistry.this.groupAttributeDefaults, result);
+
+                    // Make sure the "GROUP_" prefix is applied
                     group.getProperties().put(ContentModel.PROP_AUTHORITY_NAME, gid);
                     lookup.put(gid, group);
                 }
@@ -661,17 +693,11 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
                     LDAPUserRegistry.logger.warn("Duplicate gid found for " + gid + " -> merging definitions");
                 }
 
-                Attribute modifyTimestamp = attributes.get(LDAPUserRegistry.this.modifyTimestampAttributeName);
-                if (modifyTimestamp != null)
-                {
-                    group
-                            .setLastModified(LDAPUserRegistry.this.timestampFormat.parse(modifyTimestamp.get()
-                                    .toString()));
-                }
                 Set<String> childAssocs = group.getChildAssociations();
 
                 // Get the repeating (and possibly range restricted) member attribute
-                Attribute memAttribute = getRangeRestrictedAttribute(attributes, LDAPUserRegistry.this.memberAttributeName);
+                Attribute memAttribute = getRangeRestrictedAttribute(attributes,
+                        LDAPUserRegistry.this.memberAttributeName);
                 int nextStart = LDAPUserRegistry.this.attributeBatchSize;
 
                 // Loop until we get to the end of the range
@@ -722,11 +748,12 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
                                 {
                                     try
                                     {
-                                        Attributes childAttributes = this.ctx.getAttributes(jndiName(attribute), new String[]
+                                        Attributes childAttributes = this.ctx.getAttributes(jndiName(attribute),
-                                        {
+                                                new String[]
-                                            "objectclass", LDAPUserRegistry.this.groupIdAttributeName,
+                                                {
-                                            LDAPUserRegistry.this.userIdAttributeName
+                                                    "objectclass", LDAPUserRegistry.this.groupIdAttributeName,
-                                        });
+                                                    LDAPUserRegistry.this.userIdAttributeName
+                                                });
                                         Attribute objectClass = childAttributes.get("objectclass");
                                         if (hasAttributeValue(objectClass, LDAPUserRegistry.this.personType))
                                         {
@@ -803,16 +830,18 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
                             }
                         }
                     }
-                    
+
                     // If we are using attribute matching and we haven't got to the end (indicated by an asterisk),
                     // fetch the next batch
-                    if (nextStart > 0 && !PATTERN_RANGE_END.matcher(memAttribute.getID().toLowerCase()).find())
+                    if (nextStart > 0
+                            && !LDAPUserRegistry.PATTERN_RANGE_END.matcher(memAttribute.getID().toLowerCase()).find())
                     {
-                        Attributes childAttributes = this.ctx.getAttributes(jndiName(result.getNameInNamespace()), new String[]
+                        Attributes childAttributes = this.ctx.getAttributes(jndiName(result.getNameInNamespace()),
-                        {
+                                new String[]
-                            LDAPUserRegistry.this.memberAttributeName + ";range=" + nextStart + '-'
+                                {
-                                    + (nextStart + LDAPUserRegistry.this.attributeBatchSize - 1)
+                                    LDAPUserRegistry.this.memberAttributeName + ";range=" + nextStart + '-'
-                        });
+                                            + (nextStart + LDAPUserRegistry.this.attributeBatchSize - 1)
+                                });
                         memAttribute = getRangeRestrictedAttribute(childAttributes,
                                 LDAPUserRegistry.this.memberAttributeName);
                         nextStart += LDAPUserRegistry.this.attributeBatchSize;
@@ -838,6 +867,127 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
         return lookup.values();
     }
 
+    /*
+     * (non-Javadoc)
+     * @see org.alfresco.repo.security.sync.ldap.LDAPNameResolver#resolveDistinguishedName(java.lang.String)
+     */
+    public String resolveDistinguishedName(String userId) throws AuthenticationException
+    {
+        SearchControls userSearchCtls = new SearchControls();
+        userSearchCtls.setSearchScope(SearchControls.SUBTREE_SCOPE);
+        userSearchCtls.setReturningAttributes(new String[] {});
+        InitialDirContext ctx = null;
+        try
+        {
+            ctx = this.ldapInitialContextFactory.getDefaultIntialDirContext();
+
+            // Execute the user query with an additional condition that ensures only the user with the required ID is
+            // returned
+            NamingEnumeration<SearchResult> searchResults = ctx.search(this.userSearchBase, "(&" + this.personQuery
+                    + "(" + this.userIdAttributeName + "=" + userId + "))", userSearchCtls);
+
+            if (searchResults.hasMore())
+            {
+                return searchResults.next().getNameInNamespace();
+            }
+            throw new AuthenticationException("Failed to resolve user: " + userId);
+        }
+        catch (NamingException e)
+        {
+            throw new AlfrescoRuntimeException("Failed to resolve user ID: " + userId, e);
+        }
+        finally
+        {
+            if (ctx != null)
+            {
+                try
+                {
+                    ctx.close();
+                }
+                catch (NamingException e)
+                {
+                }
+            }
+        }
+    }
+
+    private String[] getAttributeNames(Map<String, String> attributeMapping, String... extraAttibutes)
+    {
+        Set<String> attributeSet = new TreeSet<String>();
+        attributeSet.addAll(Arrays.asList(extraAttibutes));
+        attributeSet.add(this.modifyTimestampAttributeName);
+        for (String attribute : attributeMapping.values())
+        {
+            if (attribute != null)
+            {
+                attributeSet.add(attribute);
+            }
+        }
+        String[] attributeNames = new String[attributeSet.size()];
+        attributeSet.toArray(attributeNames);
+        return attributeNames;
+    }
+
+    private NodeDescription mapToNode(Map<String, String> attributeMapping, Map<String, String> attributeDefaults,
+            SearchResult result) throws NamingException
+    {
+        NodeDescription nodeDescription = new NodeDescription(result.getNameInNamespace());
+        Attributes ldapAttributes = result.getAttributes();
+
+        // Parse the timestamp
+        Attribute modifyTimestamp = ldapAttributes.get(this.modifyTimestampAttributeName);
+        if (modifyTimestamp != null)
+        {
+            try
+            {
+                nodeDescription.setLastModified(this.timestampFormat.parse(modifyTimestamp.get().toString()));
+            }
+            catch (ParseException e)
+            {
+                throw new AlfrescoRuntimeException("Failed to parse timestamp.", e);
+            }
+        }
+
+        // Apply the mapped attributes
+        PropertyMap properties = nodeDescription.getProperties();
+        for (String key : attributeMapping.keySet())
+        {
+            QName keyQName = QName.createQName(key, this.namespaceService);
+
+            // cater for null
+            String attributeName = attributeMapping.get(key);
+            if (attributeName != null)
+            {
+                Attribute attribute = ldapAttributes.get(attributeName);
+                if (attribute != null)
+                {
+                    String value = (String) attribute.get(0);
+                    if (value != null)
+                    {
+                        properties.put(keyQName, value);
+                    }
+                }
+                else
+                {
+                    String defaultValue = attributeDefaults.get(key);
+                    if (defaultValue != null)
+                    {
+                        properties.put(keyQName, defaultValue);
+                    }
+                }
+            }
+            else
+            {
+                String defaultValue = attributeDefaults.get(key);
+                if (defaultValue != null)
+                {
+                    properties.put(keyQName, defaultValue);
+                }
+            }
+        }
+        return nodeDescription;
+    }
+
     /**
      * Converts a given DN into one suitable for use through JNDI. In particular, escapes special characters such as '/'
      * which have special meaning to JNDI.
@@ -920,50 +1070,6 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
         }
     }
 
-    /*
-     * (non-Javadoc)
-     * @see org.alfresco.repo.security.sync.ldap.LDAPNameResolver#resolveDistinguishedName(java.lang.String)
-     */
-    public String resolveDistinguishedName(String userId) throws AuthenticationException
-    {
-        SearchControls userSearchCtls = new SearchControls();
-        userSearchCtls.setSearchScope(SearchControls.SUBTREE_SCOPE);
-        userSearchCtls.setReturningAttributes(new String[] {});
-        InitialDirContext ctx = null;
-        try
-        {
-            ctx = this.ldapInitialContextFactory.getDefaultIntialDirContext();
-
-            // Execute the user query with an additional condition that ensures only the user with the required ID is
-            // returned
-            NamingEnumeration<SearchResult> searchResults = ctx.search(this.userSearchBase, "(&" + this.personQuery
-                    + "(" + this.userIdAttributeName + "=" + userId + "))", userSearchCtls);
-
-            if (searchResults.hasMore())
-            {
-                return jndiName(searchResults.next().getNameInNamespace()).toString();
-            }
-            throw new AuthenticationException("Failed to resolve user: " + userId);
-        }
-        catch (NamingException e)
-        {
-            throw new AlfrescoRuntimeException("Failed to resolve user ID: " + userId, e);
-        }
-        finally
-        {
-            if (ctx != null)
-            {
-                try
-                {
-                    ctx.close();
-                }
-                catch (NamingException e)
-                {
-                }
-            }
-        }
-    }
-
     /**
      * Does a case-insensitive search for the given value in an attribute.
      * 
@@ -994,7 +1100,7 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
         }
         return false;
     }
-    
+
     /**
      * Gets the values of a repeating attribute that may have range restriction options. If an attribute is range
      * restricted, it will appear in the attribute set with a ";range=i-j" option, where i and j indicate the start and
@@ -1016,7 +1122,7 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
             return unrestricted;
         }
         NamingEnumeration<? extends Attribute> i = attributes.getAll();
-        String searchString = attributeName.toLowerCase() + ';'; 
+        String searchString = attributeName.toLowerCase() + ';';
         while (i.hasMore())
         {
             Attribute attribute = i.next();
@@ -1257,59 +1363,9 @@ public class LDAPUserRegistry implements UserRegistry, LDAPNameResolver, Initial
                             LDAPUserRegistry.logger.debug("Adding user for " + uid);
                         }
 
-                        NodeDescription person = new NodeDescription(result.getNameInNamespace());
+                        // Apply the mapped properties to the node description
-
+                        return mapToNode(LDAPUserRegistry.this.personAttributeMapping,
-                        Attribute modifyTimestamp = attributes.get(LDAPUserRegistry.this.modifyTimestampAttributeName);
+                                LDAPUserRegistry.this.personAttributeDefaults, result);
-                        if (modifyTimestamp != null)
-                        {
-                            try
-                            {
-                                person.setLastModified(LDAPUserRegistry.this.timestampFormat.parse(modifyTimestamp
-                                        .get().toString()));
-                            }
-                            catch (ParseException e)
-                            {
-                                throw new AlfrescoRuntimeException("Failed to import people.", e);
-                            }
-                        }
-
-                        PropertyMap properties = person.getProperties();
-                        for (String key : LDAPUserRegistry.this.attributeMapping.keySet())
-                        {
-                            QName keyQName = QName.createQName(key, LDAPUserRegistry.this.namespaceService);
-
-                            // cater for null
-                            String attributeName = LDAPUserRegistry.this.attributeMapping.get(key);
-                            if (attributeName != null)
-                            {
-                                Attribute attribute = attributes.get(attributeName);
-                                if (attribute != null)
-                                {
-                                    String value = (String) attribute.get(0);
-                                    if (value != null)
-                                    {
-                                        properties.put(keyQName, value);
-                                    }
-                                }
-                                else
-                                {
-                                    String defaultValue = LDAPUserRegistry.this.attributeDefaults.get(key);
-                                    if (defaultValue != null)
-                                    {
-                                        properties.put(keyQName, defaultValue);
-                                    }
-                                }
-                            }
-                            else
-                            {
-                                String defaultValue = LDAPUserRegistry.this.attributeDefaults.get(key);
-                                if (defaultValue != null)
-                                {
-                                    properties.put(keyQName, defaultValue);
-                                }
-                            }
-                        }
-                        return person;
                     }
 
                     // Examine the paged results control response for an indication that another page is available