From a929a7fd747ea7eb85981d297af1d888db1f2416 Mon Sep 17 00:00:00 2001 From: Dave Ward Date: Wed, 8 Jul 2009 09:59:43 +0000 Subject: [PATCH] Merged V3.2 to HEAD 15099: Do not allow referential integrity problems in user registry data (dangling references to users in groups) fail the whole sync operation. Just warn and continue. git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@15100 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- .../ChainingUserRegistrySynchronizer.java | 164 ++++++++++++------ .../ChainingUserRegistrySynchronizerTest.java | 5 +- 2 files changed, 114 insertions(+), 55 deletions(-) diff --git a/source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizer.java b/source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizer.java index f91e6ab78c..2ab205eecb 100644 --- a/source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizer.java +++ b/source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizer.java @@ -196,38 +196,39 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize Set visitedZoneIds = new TreeSet(); for (String id : this.applicationContextManager.getInstanceIds()) { - StringBuilder builder = new StringBuilder(32); - builder.append(AuthorityService.ZONE_AUTH_EXT_PREFIX); - builder.append(id); - String zoneId = builder.toString(); ApplicationContext context = this.applicationContextManager.getApplicationContext(id); try { UserRegistry plugin = (UserRegistry) context.getBean(this.sourceBeanName); if (!(plugin instanceof ActivateableBean) || ((ActivateableBean) plugin).isActive()) { - ChainingUserRegistrySynchronizer.logger.info("Synchronizing users and groups with user registry '" - + id + "'"); - if (force) + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) + { + ChainingUserRegistrySynchronizer.logger + .info("Synchronizing users and groups with user registry '" + id + "'"); + } + if (force && ChainingUserRegistrySynchronizer.logger.isWarnEnabled()) { ChainingUserRegistrySynchronizer.logger .warn("Forced synchronization with user registry '" + id + "'; some users and groups previously created by synchronization with this user registry may be removed."); } - int personsProcessed = syncPersonsWithPlugin(zoneId, plugin, force, visitedZoneIds); - int groupsProcessed = syncGroupsWithPlugin(zoneId, plugin, force, visitedZoneIds); - ChainingUserRegistrySynchronizer.logger - .info("Finished synchronizing users and groups with user registry '" + zoneId + "'"); - ChainingUserRegistrySynchronizer.logger.info(personsProcessed + " user(s) and " + groupsProcessed - + " group(s) processed"); + int personsProcessed = syncPersonsWithPlugin(id, plugin, force, visitedZoneIds); + int groupsProcessed = syncGroupsWithPlugin(id, plugin, force, visitedZoneIds); + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) + { + ChainingUserRegistrySynchronizer.logger + .info("Finished synchronizing users and groups with user registry '" + id + "'"); + ChainingUserRegistrySynchronizer.logger.info(personsProcessed + " user(s) and " + + groupsProcessed + " group(s) processed"); + } } } catch (NoSuchBeanDefinitionException e) { // Ignore and continue } - visitedZoneIds.add(zoneId); } } @@ -264,7 +265,7 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize /** * Synchronizes local users (persons) with a {@link UserRegistry} for a particular zone. * - * @param zoneId + * @param zone * the zone id. This identifier is used to tag all created users, so that in the future we can tell those * that have been deleted from the registry. * @param userRegistry @@ -278,21 +279,26 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize * the zones in this set, then it will be ignored as this zone has lower priority. * @return the number of users processed */ - private int syncPersonsWithPlugin(String zoneId, UserRegistry userRegistry, boolean force, - Set visitedZoneIds) + private int syncPersonsWithPlugin(String zone, UserRegistry userRegistry, boolean force, Set visitedZoneIds) { + // Create a prefixed zone ID for use with the authority service + String zoneId = AuthorityService.ZONE_AUTH_EXT_PREFIX + zone; + int processedCount = 0; long lastModifiedMillis = force ? -1L : getMostRecentUpdateTime( ChainingUserRegistrySynchronizer.PERSON_LAST_MODIFIED_ATTRIBUTE, zoneId); Date lastModified = lastModifiedMillis == -1 ? null : new Date(lastModifiedMillis); - if (lastModified == null) + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) { - ChainingUserRegistrySynchronizer.logger.info("Retrieving all users from user registry '" + zoneId + "'"); - } - else - { - ChainingUserRegistrySynchronizer.logger.info("Retrieving users changed since " - + DateFormat.getDateTimeInstance().format(lastModified) + " from user registry '" + zoneId + "'"); + if (lastModified == null) + { + ChainingUserRegistrySynchronizer.logger.info("Retrieving all users from user registry '" + zone + "'"); + } + else + { + ChainingUserRegistrySynchronizer.logger.info("Retrieving users changed since " + + DateFormat.getDateTimeInstance().format(lastModified) + " from user registry '" + zone + "'"); + } } Iterator persons = userRegistry.getPersons(lastModified); Set personsToDelete = this.authorityService.getAllAuthoritiesInZone(zoneId, AuthorityType.USER); @@ -304,7 +310,10 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize if (personsToDelete.remove(personName)) { // The person already existed in this zone: update the person - ChainingUserRegistrySynchronizer.logger.info("Updating user '" + personName + "'"); + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) + { + ChainingUserRegistrySynchronizer.logger.info("Updating user '" + personName + "'"); + } this.personService.setPersonProperties(personName, personProperties); } else @@ -320,16 +329,22 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize continue; } // The person existed, but in a zone with lower precedence - ChainingUserRegistrySynchronizer.logger - .warn("Recreating occluded user '" - + personName - + "'. This user was previously created manually or through synchronization with a lower priority user registry."); + if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled()) + { + ChainingUserRegistrySynchronizer.logger + .warn("Recreating occluded user '" + + personName + + "'. This user was previously created manually or through synchronization with a lower priority user registry."); + } this.personService.deletePerson(personName); } else { // The person did not exist at all - ChainingUserRegistrySynchronizer.logger.info("Creating user '" + personName + "'"); + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) + { + ChainingUserRegistrySynchronizer.logger.info("Creating user '" + personName + "'"); + } } this.personService.createPerson(personProperties, getZones(zoneId)); } @@ -348,7 +363,10 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize { for (String personName : personsToDelete) { - ChainingUserRegistrySynchronizer.logger.warn("Deleting user '" + personName + "'"); + if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled()) + { + ChainingUserRegistrySynchronizer.logger.warn("Deleting user '" + personName + "'"); + } this.personService.deletePerson(personName); processedCount++; } @@ -360,13 +378,16 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize lastModifiedMillis); } + // Remember we have visited this zone + visitedZoneIds.add(zoneId); + return processedCount; } /** * Synchronizes local groups (authorities) with a {@link UserRegistry} for a particular zone. * - * @param zoneId + * @param zone * the zone id. This identifier is used to tag all created groups, so that in the future we can tell * those that have been deleted from the registry. * @param userRegistry @@ -380,20 +401,26 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize * of the zones in this set, then it will be ignored as this zone has lower priority. * @return the number of groups processed */ - private int syncGroupsWithPlugin(String zoneId, UserRegistry userRegistry, boolean force, Set visitedZoneIds) + private int syncGroupsWithPlugin(String zone, UserRegistry userRegistry, boolean force, Set visitedZoneIds) { + // Create a prefixed zone ID for use with the authority service + String zoneId = AuthorityService.ZONE_AUTH_EXT_PREFIX + zone; + int processedCount = 0; long lastModifiedMillis = force ? -1L : getMostRecentUpdateTime( ChainingUserRegistrySynchronizer.GROUP_LAST_MODIFIED_ATTRIBUTE, zoneId); Date lastModified = lastModifiedMillis == -1 ? null : new Date(lastModifiedMillis); - if (lastModified == null) + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) { - ChainingUserRegistrySynchronizer.logger.info("Retrieving all groups from user registry '" + zoneId + "'"); - } - else - { - ChainingUserRegistrySynchronizer.logger.info("Retrieving groups changed since " - + DateFormat.getDateTimeInstance().format(lastModified) + " from user registry '" + zoneId + "'"); + if (lastModified == null) + { + ChainingUserRegistrySynchronizer.logger.info("Retrieving all groups from user registry '" + zone + "'"); + } + else + { + ChainingUserRegistrySynchronizer.logger.info("Retrieving groups changed since " + + DateFormat.getDateTimeInstance().format(lastModified) + " from user registry '" + zone + "'"); + } } Iterator groups = userRegistry.getGroups(lastModified); @@ -419,9 +446,12 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize } for (String child : toDelete) { - ChainingUserRegistrySynchronizer.logger.info("Removing '" - + this.authorityService.getShortName(child) + "' from group '" - + this.authorityService.getShortName(groupName) + "'"); + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) + { + ChainingUserRegistrySynchronizer.logger.info("Removing '" + + this.authorityService.getShortName(child) + "' from group '" + + this.authorityService.getShortName(groupName) + "'"); + } this.authorityService.removeAuthority(groupName, child); } } @@ -438,15 +468,21 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize continue; } // The group existed, but in a zone with lower precedence - ChainingUserRegistrySynchronizer.logger - .warn("Recreating occluded group '" - + groupShortName - + "'. This group was previously created manually or through synchronization with a lower priority user registry."); + if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled()) + { + ChainingUserRegistrySynchronizer.logger + .warn("Recreating occluded group '" + + groupShortName + + "'. This group was previously created manually or through synchronization with a lower priority user registry."); + } this.authorityService.deleteAuthority(groupName); } else { - ChainingUserRegistrySynchronizer.logger.info("Creating group '" + groupShortName + "'"); + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) + { + ChainingUserRegistrySynchronizer.logger.info("Creating group '" + groupShortName + "'"); + } } // create the group @@ -476,9 +512,25 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize for (String child : entry.getValue()) { String groupName = entry.getKey(); - ChainingUserRegistrySynchronizer.logger.info("Adding '" + this.authorityService.getShortName(child) - + "' to group '" + this.authorityService.getShortName(groupName) + "'"); - this.authorityService.addAuthority(groupName, child); + if (ChainingUserRegistrySynchronizer.logger.isInfoEnabled()) + { + ChainingUserRegistrySynchronizer.logger.info("Adding '" + this.authorityService.getShortName(child) + + "' to group '" + this.authorityService.getShortName(groupName) + "'"); + } + try + { + this.authorityService.addAuthority(groupName, child); + } + catch (Exception e) + { + // Let's not allow referential integrity problems (dangling references) kill the whole process + if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled()) + { + ChainingUserRegistrySynchronizer.logger.warn("Failed to add '" + + this.authorityService.getShortName(child) + "' to group '" + + this.authorityService.getShortName(groupName) + "'", e); + } + } } } @@ -488,8 +540,11 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize { for (String group : groupsToDelete) { - ChainingUserRegistrySynchronizer.logger.warn("Deleting group '" - + this.authorityService.getShortName(group) + "'"); + if (ChainingUserRegistrySynchronizer.logger.isWarnEnabled()) + { + ChainingUserRegistrySynchronizer.logger.warn("Deleting group '" + + this.authorityService.getShortName(group) + "'"); + } this.authorityService.deleteAuthority(group); processedCount++; } @@ -501,6 +556,9 @@ public class ChainingUserRegistrySynchronizer implements UserRegistrySynchronize lastModifiedMillis); } + // Remember we have visited this zone + visitedZoneIds.add(zoneId); + return processedCount; } diff --git a/source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizerTest.java b/source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizerTest.java index 074f823d3c..0c543b53ec 100644 --- a/source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizerTest.java +++ b/source/java/org/alfresco/repo/security/sync/ChainingUserRegistrySynchronizerTest.java @@ -220,7 +220,8 @@ public class ChainingUserRegistrySynchronizerTest extends BaseSpringTest /** * Tests a forced update of the test users and groups. Also tests that groups and users that previously existed in - * Z2 get moved when they appear in Z1. The layout is as follows + * Z2 get moved when they appear in Z1. Also tests that 'dangling references' to removed users (U4, U5) do not cause + * any problems. The layout is as follows * * 
      * Z1
@@ -252,7 +253,7 @@ public class ChainingUserRegistrySynchronizerTest extends BaseSpringTest
             newPerson("U1", "somenewemail@alfresco.com"), newPerson("U3"), newPerson("U6")
         }, new NodeDescription[]
         {
-            newGroup("G2", "U1", "U3", "U6"), newGroup("G6", "U3", "G7"), newGroup("G7")
+            newGroup("G2", "U1", "U3", "U4", "U6"), newGroup("G6", "U3", "U4", "G7"), newGroup("G7", "U4", "U5")
         }));
         this.synchronizer.synchronize(true);
         assertExists("Z1", "U2");