Merged V3.2 to HEAD

15636: ETHREEOH-2626: LDAP sync will no longer delete and recreate colliding users and groups in zones that aren't even in the authentication chain.
      - Instead such users and groups will be 're-zoned' to the first zone where they were found
      - Avoids losing site memberships, etc. on upgrade or change of authentication chain
      - Will continue to recreate users and groups from lower priority zones in the authentication chain
      - Updated unit tests appropriately


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

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

@@ -25,6 +25,7 @@
 package org.alfresco.repo.security.sync;
 
 import java.text.DateFormat;
+import java.util.Collection;
 import java.util.Date;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -231,7 +232,16 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
     public void synchronize(boolean force, boolean splitTxns)
     {
         Set<String> visitedZoneIds = new TreeSet<String>();
-        for (String id : this.applicationContextManager.getInstanceIds())
+        Collection<String> instanceIds = this.applicationContextManager.getInstanceIds();
+
+        // Work out the set of all zone IDs in the authentication chain so that we can decide which users / groups need
+        // 're-zoning'
+        Set<String> allZoneIds = new TreeSet<String>();
+        for (String id : instanceIds)
+        {
+            allZoneIds.add(AuthorityService.ZONE_AUTH_EXT_PREFIX + id);
+        }
+        for (String id : instanceIds)
         {
             ApplicationContext context = this.applicationContextManager.getApplicationContext(id);
             try
@@ -257,8 +267,10 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                     boolean requiresNew = splitTxns
                             || AlfrescoTransactionSupport.getTransactionReadState() == TxnReadState.TXN_READ_ONLY;
 
-                    int personsProcessed = syncPersonsWithPlugin(id, plugin, force, requiresNew, visitedZoneIds);
+                    int personsProcessed = syncPersonsWithPlugin(id, plugin, force, requiresNew, visitedZoneIds,
-                    int groupsProcessed = syncGroupsWithPlugin(id, plugin, force, requiresNew, visitedZoneIds);
+                            allZoneIds);
+                    int groupsProcessed = syncGroupsWithPlugin(id, plugin, force, requiresNew, visitedZoneIds,
+                            allZoneIds);
                     if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled())
                     {
                         ChainingUserRegistrySynchronizer.logger
@@ -334,10 +346,14 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
      *            the set of zone ids already processed. These zones have precedence over the current zone when it comes
      *            to user name 'collisions'. If a user is queried that already exists locally but is tagged with one of
      *            the zones in this set, then it will be ignored as this zone has lower priority.
+     * @param allZoneIds
+     *            the set of all zone ids in the authentication chain. Helps us work out whether the zone information
+     *            recorded against a user is invalid for the current authentication chain and whether the user needs to
+     *            be 're-zoned'.
      * @return the number of users processed
      */
-    private int syncPersonsWithPlugin(String zone, UserRegistry userRegistry, boolean force, boolean splitTxns,
+    private int syncPersonsWithPlugin(final String zone, UserRegistry userRegistry, boolean force, boolean splitTxns,
-            final Set<String> visitedZoneIds)
+            final Set<String> visitedZoneIds, final Set<String> allZoneIds)
     {
         // Create a prefixed zone ID for use with the authority service
         final String zoneId = AuthorityService.ZONE_AUTH_EXT_PREFIX + zone;
@@ -408,23 +424,47 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                     }
                     else
                     {
-                        // The person does not exist in this zone, but may exist in another zone
+                        // Check whether the user is in any of the authentication chain zones
-                        zones.retainAll(visitedZoneIds);
+                        Set<String> intersection = new TreeSet<String>(zones);
-                        if (zones.size() > 0)
+                        intersection.retainAll(allZoneIds);
+                        if (intersection.size() == 0)
                         {
-                            // A person that exists in a different zone with higher precedence
+                            // The person exists, but not in a zone that's in the authentication chain. May be due to
-                            continue;
+                            // upgrade or zone changes. Let's re-zone them
+                            if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled())
+                            {
+                                ChainingUserRegistrySynchronizer.logger.warn("Updating user '" + personName
+                                        + "'. This user will in future be assumed to originate from user registry '"
+                                        + zone + "'.");
+                            }
+                            ChainingUserRegistrySynchronizer.this.authorityService.removeAuthorityFromZones(personName,
+                                    zones);
+                            ChainingUserRegistrySynchronizer.this.authorityService.addAuthorityToZones(personName,
+                                    zoneSet);
+                            ChainingUserRegistrySynchronizer.this.personService.setPersonProperties(personName,
+                                    personProperties);
                         }
-                        // The person existed, but in a zone with lower precedence
+                        else
-                        if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled())
                         {
-                            ChainingUserRegistrySynchronizer.logger
+                            // Check whether the user is in any of the higher priority authentication chain zones
-                                    .warn("Recreating occluded user '"
+                            intersection.retainAll(visitedZoneIds);
-                                            + personName
+                            if (intersection.size() > 0)
-                                            + "'. This user was previously created manually or through synchronization with a lower priority user registry.");
+                            {
+                                // A person that exists in a different zone with higher precedence - ignore
+                                continue;
+                            }
+
+                            // The person existed, but in a zone with lower precedence
+                            if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled())
+                            {
+                                ChainingUserRegistrySynchronizer.logger
+                                        .warn("Recreating occluded user '"
+                                                + personName
+                                                + "'. This user was previously created through synchronization with a lower priority user registry.");
+                            }
+                            ChainingUserRegistrySynchronizer.this.personService.deletePerson(personName);
+                            ChainingUserRegistrySynchronizer.this.personService.createPerson(personProperties, zoneSet);
                         }
-                        ChainingUserRegistrySynchronizer.this.personService.deletePerson(personName);
-                        ChainingUserRegistrySynchronizer.this.personService.createPerson(personProperties, zoneSet);
                     }
                     // Increment the count of processed people
                     personsCreated.add(personName);
@@ -513,10 +553,14 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
      *            the set of zone ids already processed. These zones have precedence over the current zone when it comes
      *            to group name 'collisions'. If a group is queried that already exists locally but is tagged with one
      *            of the zones in this set, then it will be ignored as this zone has lower priority.
+     * @param allZoneIds
+     *            the set of all zone ids in the authentication chain. Helps us work out whether the zone information
+     *            recorded against a group is invalid for the current authentication chain and whether the group needs
+     *            to be 're-zoned'.
      * @return the number of groups processed
      */
-    private int syncGroupsWithPlugin(String zone, UserRegistry userRegistry, boolean force, boolean splitTxns,
+    private int syncGroupsWithPlugin(final String zone, UserRegistry userRegistry, boolean force, boolean splitTxns,
-            final Set<String> visitedZoneIds)
+            final Set<String> visitedZoneIds, final Set<String> allZoneIds)
     {
         // Create a prefixed zone ID for use with the authority service
         final String zoneId = AuthorityService.ZONE_AUTH_EXT_PREFIX + zone;
@@ -566,14 +610,14 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                     NodeDescription group = groups.next();
                     PropertyMap groupProperties = group.getProperties();
                     String groupName = (String) groupProperties.get(ContentModel.PROP_AUTHORITY_NAME);
+                    String groupShortName = ChainingUserRegistrySynchronizer.this.authorityService
+                            .getShortName(groupName);
                     Set<String> groupZones = ChainingUserRegistrySynchronizer.this.authorityService
                             .getAuthorityZones(groupName);
 
                     if (groupZones == null)
                     {
                         // The group did not exist at all
-                        String groupShortName = ChainingUserRegistrySynchronizer.this.authorityService
-                                .getShortName(groupName);
                         if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled())
                         {
                             ChainingUserRegistrySynchronizer.logger.info("Creating group '" + groupShortName + "'");
@@ -588,64 +632,81 @@ public class ChainingUserRegistrySynchronizer extends AbstractLifecycleBean impl
                             groupAssocsToCreate.put(groupName, children);
                         }
                     }
-                    else if (groupZones.contains(zoneId))
-                    {
-                        // The group already existed in this zone: update the group
-                        Set<String> oldChildren = ChainingUserRegistrySynchronizer.this.authorityService
-                                .getContainedAuthorities(null, groupName, true);
-                        Set<String> newChildren = group.getChildAssociations();
-                        Set<String> toDelete = new TreeSet<String>(oldChildren);
-                        Set<String> toAdd = new TreeSet<String>(newChildren);
-                        toDelete.removeAll(newChildren);
-                        toAdd.removeAll(oldChildren);
-                        if (!toAdd.isEmpty())
-                        {
-                            groupAssocsToCreate.put(groupName, toAdd);
-                        }
-                        for (String child : toDelete)
-                        {
-                            if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled())
-                            {
-                                ChainingUserRegistrySynchronizer.logger.info("Removing '"
-                                        + ChainingUserRegistrySynchronizer.this.authorityService.getShortName(child)
-                                        + "' from group '"
-                                        + ChainingUserRegistrySynchronizer.this.authorityService
-                                                .getShortName(groupName) + "'");
-                            }
-                            ChainingUserRegistrySynchronizer.this.authorityService.removeAuthority(groupName, child);
-                        }
-                    }
                     else
                     {
-                        // The group does not exist in this zone, but may exist in another zone
+                        // Check whether the group is in any of the authentication chain zones
-                        groupZones.retainAll(visitedZoneIds);
+                        Set<String> intersection = new TreeSet<String>(groupZones);
-                        if (groupZones.size() > 0)
+                        intersection.retainAll(allZoneIds);
+                        if (intersection.isEmpty())
                         {
-                            // A group that exists in a different zone with higher precedence
+                            // The group exists, but not in a zone that's in the authentication chain. May be due to
-                            continue;
+                            // upgrade or zone changes. Let's re-zone them
+                            if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled())
+                            {
+                                ChainingUserRegistrySynchronizer.logger.warn("Updating group '" + groupShortName
+                                        + "'. This group will in future be assumed to originate from user registry '"
+                                        + zone + "'.");
+                            }
+                            ChainingUserRegistrySynchronizer.this.authorityService.removeAuthorityFromZones(groupName,
+                                    groupZones);
+                            ChainingUserRegistrySynchronizer.this.authorityService.addAuthorityToZones(groupName,
+                                    zoneSet);
                         }
-                        String groupShortName = ChainingUserRegistrySynchronizer.this.authorityService
+                        if (groupZones.contains(zoneId) || intersection.isEmpty())
-                                .getShortName(groupName);
-                        // The group existed, but in a zone with lower precedence
-                        if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled())
                         {
-                            ChainingUserRegistrySynchronizer.logger
+                            // The group already existed in this zone or no valid zone: update the group
-                                    .warn("Recreating occluded group '"
+                            Set<String> oldChildren = ChainingUserRegistrySynchronizer.this.authorityService
-                                            + groupShortName
+                                    .getContainedAuthorities(null, groupName, true);
-                                            + "'. This group was previously created manually or through synchronization with a lower priority user registry.");
+                            Set<String> newChildren = group.getChildAssociations();
+                            Set<String> toDelete = new TreeSet<String>(oldChildren);
+                            Set<String> toAdd = new TreeSet<String>(newChildren);
+                            toDelete.removeAll(newChildren);
+                            toAdd.removeAll(oldChildren);
+                            if (!toAdd.isEmpty())
+                            {
+                                groupAssocsToCreate.put(groupName, toAdd);
+                            }
+                            for (String child : toDelete)
+                            {
+                                if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled())
+                                {
+                                    ChainingUserRegistrySynchronizer.logger.info("Removing '"
+                                            + ChainingUserRegistrySynchronizer.this.authorityService
+                                                    .getShortName(child) + "' from group '" + groupShortName + "'");
+                                }
+                                ChainingUserRegistrySynchronizer.this.authorityService
+                                        .removeAuthority(groupName, child);
+                            }
                         }
-                        ChainingUserRegistrySynchronizer.this.authorityService.deleteAuthority(groupName);
+                        else
-                        // create the group
-                        ChainingUserRegistrySynchronizer.this.authorityService.createAuthority(AuthorityType
-                                .getAuthorityType(groupName), groupShortName, (String) groupProperties
-                                .get(ContentModel.PROP_AUTHORITY_DISPLAY_NAME), zoneSet);
-                        Set<String> children = group.getChildAssociations();
-                        if (!children.isEmpty())
                         {
-                            groupAssocsToCreate.put(groupName, children);
+                            // Check whether the group is in any of the higher priority authentication chain zones
+                            intersection.retainAll(visitedZoneIds);
+                            if (!intersection.isEmpty())
+                            {
+                                // A group that exists in a different zone with higher precedence
+                                continue;
+                            }
+                            // The group existed, but in a zone with lower precedence
+                            if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled())
+                            {
+                                ChainingUserRegistrySynchronizer.logger
+                                        .warn("Recreating occluded group '"
+                                                + groupShortName
+                                                + "'. This group was previously created through synchronization with a lower priority user registry.");
+                            }
+                            ChainingUserRegistrySynchronizer.this.authorityService.deleteAuthority(groupName);
+                            // create the group
+                            ChainingUserRegistrySynchronizer.this.authorityService.createAuthority(AuthorityType
+                                    .getAuthorityType(groupName), groupShortName, (String) groupProperties
+                                    .get(ContentModel.PROP_AUTHORITY_DISPLAY_NAME), zoneSet);
+                            Set<String> children = group.getChildAssociations();
+                            if (!children.isEmpty())
+                            {
+                                groupAssocsToCreate.put(groupName, children);
+                            }
                         }
                     }
-
                     // Increment the count of processed groups
                     processedCount++;
                     groupsCreated.add(groupName);

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

@@ -117,12 +117,17 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
     }
 
     /**
-     * Sets up the test users and groups in two zones, "Z1" and "Z2", by doing a forced synchronize with a Mock user
+     * Sets up the test users and groups in three zones, "Z0", "Z1" and "Z2", by doing a forced synchronize with a Mock
-     * registry. Note that the zones have some overlapping entries. The layout is as follows
+     * user registry. Note that the zones have some overlapping entries. "Z0" is not used in subsequent synchronizations
+     * and is used to test that users and groups in zones that aren't in the authentication chain get 're-zoned'
+     * appropriately. The layout is as follows
      * 
      * <pre>
-     * Z1
+     * Z0
      * G1
+     * U6
+     * 
+     * Z1
      * G2 - U1, G3 - U2, G4, G5
      * 
      * Z2
@@ -135,13 +140,18 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
      */
     private void setUpTestUsersAndGroups() throws Exception
     {
-        this.applicationContextManager.setUserRegistries(new MockUserRegistry("Z1", new NodeDescription[]
+        this.applicationContextManager.setUserRegistries(new MockUserRegistry("Z0", new NodeDescription[]
+        {
+            newPerson("U6")
+        }, new NodeDescription[]
+        {
+            newGroup("G1")
+        }), new MockUserRegistry("Z1", new NodeDescription[]
         {
             newPerson("U1"), newPerson("U2")
         }, new NodeDescription[]
         {
-            newGroup("G1"), newGroup("G2", "U1", "G3"), newGroup("G3", "U2", "G4", "G5"), newGroup("G4"),
+            newGroup("G2", "U1", "G3"), newGroup("G3", "U2", "G4", "G5"), newGroup("G4"), newGroup("G5")
-            newGroup("G5")
         }), new MockUserRegistry("Z2", new NodeDescription[]
         {
             newPerson("U1"), newPerson("U3"), newPerson("U4"), newPerson("U5")
@@ -163,9 +173,10 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
 
             public Object execute() throws Throwable
             {
+                assertExists("Z0", "U6");
+                assertExists("Z0", "G1");
                 assertExists("Z1", "U1");
                 assertExists("Z1", "U2");
-                assertExists("Z1", "G1");
                 assertExists("Z1", "G2", "U1", "G3");
                 assertExists("Z1", "G3", "U2", "G4", "G5");
                 assertExists("Z1", "G4");
@@ -183,7 +194,8 @@ public class ChainingUserRegistrySynchronizerTest extends TestCase
     private void tearDownTestUsersAndGroups() throws Exception
     {
         // Wipe out everything that was in Z1 and Z2
-        this.applicationContextManager.setUserRegistries(new MockUserRegistry("Z1", new NodeDescription[] {},
+        this.applicationContextManager.setUserRegistries(new MockUserRegistry("Z0", new NodeDescription[] {},
+                new NodeDescription[] {}), new MockUserRegistry("Z1", new NodeDescription[] {},
                 new NodeDescription[] {}), new MockUserRegistry("Z2", new NodeDescription[] {},
                 new NodeDescription[] {}));
         this.retryingTransactionHelper.doInTransaction(new RetryingTransactionCallback<Object>()