Fix ALF-4118 - PersonServiceImpl - prevent creation of duplicate people

- add validation to prevent direct creation / deletion (if using people container)
- should fix other possible inconsistencies with personCache
- add unit tests
- minor cleanup

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

source/java/org/alfresco/repo/security/person/PersonServiceImpl.java

@@ -35,6 +35,7 @@ import org.alfresco.model.ContentModel;
 import org.alfresco.repo.cache.SimpleCache;
 import org.alfresco.repo.domain.permissions.AclDAO;
 import org.alfresco.repo.node.NodeServicePolicies;
+import org.alfresco.repo.node.NodeServicePolicies.BeforeCreateNodePolicy;
 import org.alfresco.repo.node.NodeServicePolicies.BeforeDeleteNodePolicy;
 import org.alfresco.repo.node.NodeServicePolicies.OnCreateNodePolicy;
 import org.alfresco.repo.node.NodeServicePolicies.OnUpdatePropertiesPolicy;
@@ -75,10 +76,13 @@ import org.alfresco.util.PropertyCheck;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-public class PersonServiceImpl extends TransactionListenerAdapter implements PersonService, NodeServicePolicies.OnCreateNodePolicy, NodeServicePolicies.BeforeDeleteNodePolicy,
+public class PersonServiceImpl extends TransactionListenerAdapter implements PersonService,
-        NodeServicePolicies.OnUpdatePropertiesPolicy
+                                                                             NodeServicePolicies.BeforeCreateNodePolicy,
+                                                                             NodeServicePolicies.OnCreateNodePolicy,
+                                                                             NodeServicePolicies.BeforeDeleteNodePolicy,
+                                                                             NodeServicePolicies.OnUpdatePropertiesPolicy
 {
-    private static Log s_logger = LogFactory.getLog(PersonServiceImpl.class);
+    private static Log logger = LogFactory.getLog(PersonServiceImpl.class);
 
     private static final String DELETE = "DELETE";
 
@@ -144,6 +148,9 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
     
     private UserNameMatcher userNameMatcher;
     
+    private JavaBehaviour beforeCreateNodeValidationBehaviour;
+    private JavaBehaviour beforeDeleteNodeValidationBehaviour;
+    
     static
     {
         Set<QName> props = new HashSet<QName>();
@@ -186,6 +193,18 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         PropertyCheck.mandatory(this, "aclDao", aclDao);
         PropertyCheck.mandatory(this, "homeFolderManager", homeFolderManager);
         
+        beforeCreateNodeValidationBehaviour = new JavaBehaviour(this, "beforeCreateNodeValidation");
+        this.policyComponent.bindClassBehaviour(
+                BeforeCreateNodePolicy.QNAME,
+                ContentModel.TYPE_PERSON,
+                beforeCreateNodeValidationBehaviour);
+        
+        beforeDeleteNodeValidationBehaviour = new JavaBehaviour(this, "beforeDeleteNodeValidation");
+        this.policyComponent.bindClassBehaviour(
+                BeforeDeleteNodePolicy.QNAME,
+                ContentModel.TYPE_PERSON,
+                beforeDeleteNodeValidationBehaviour);
+        
         this.policyComponent.bindClassBehaviour(
                 OnCreateNodePolicy.QNAME,
                 ContentModel.TYPE_PERSON,
@@ -264,13 +283,7 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
     }
     
     /**
-     * Retrieve the person NodeRef for a username key. Depending on configuration missing people will be created if not
+     * {@inheritDoc}
-     * found, else a NoSuchPersonException exception will be thrown.
-     * 
-     * @param userName
-     *            of the person NodeRef to retrieve
-     * @return NodeRef of the person as specified by the username
-     * @throws NoSuchPersonException
      */
     public NodeRef getPerson(String userName)
     {
@@ -278,17 +291,7 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
     }
     
     /**
-     * Retrieve the person NodeRef for a username key. Depending on the <code>autoCreate</code> parameter and
+     * {@inheritDoc}
-     * configuration missing people will be created if not found, else a NoSuchPersonException exception will be thrown.
-     * 
-     * @param userName
-     *            of the person NodeRef to retrieve
-     * @param autoCreate
-     *            should we auto-create the person node and home folder if they don't exist? (and configuration allows
-     *            us to)
-     * @return NodeRef of the person as specified by the username
-     * @throws NoSuchPersonException
-     *             if the person doesn't exist and can't be created
      */
     public NodeRef getPerson(final String userName, final boolean autoCreate)
     {
@@ -342,6 +345,9 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         return personNode;
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public boolean personExists(String caseSensitiveUserName)
     {
         return getPersonOrNull(caseSensitiveUserName) != null;
@@ -349,14 +355,13 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
     
     private NodeRef getPersonOrNull(String searchUserName)
     {
-        String cacheKey = searchUserName.toLowerCase();
+        Set<NodeRef> allRefs = getFromCache(searchUserName);
-        Set<NodeRef> allRefs = this.personCache.get(cacheKey);
         if (allRefs == null)
         {
             List<ChildAssociationRef> childRefs = nodeService.getChildAssocs(
                     getPeopleContainer(),
                     ContentModel.ASSOC_CHILDREN,
-                    QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, searchUserName.toLowerCase()),
+                    getChildNameLower(searchUserName),
                     false);
             allRefs = new LinkedHashSet<NodeRef>(childRefs.size() * 2);
             
@@ -366,6 +371,7 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
                 allRefs.add(nodeRef);
             }
         }
+        
         List<NodeRef> refs = new ArrayList<NodeRef>(allRefs.size());
         for (NodeRef nodeRef : allRefs)
         {
@@ -376,6 +382,7 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
                 refs.add(nodeRef);
             }
         }
+        
         NodeRef returnRef = null;
         if (refs.size() > 1)
         {
@@ -386,7 +393,7 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
             returnRef = refs.get(0);
             
             // Don't bother caching unless we get a result that doesn't need duplicate processing
-            personCache.put(cacheKey, allRefs);            
+            putToCache(searchUserName, allRefs);
         }
         return returnRef;
     }
@@ -467,18 +474,18 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
                         // Allow UIDs to be updated in this transaction
                         AlfrescoTransactionSupport.bindResource(KEY_ALLOW_UID_UPDATE, Boolean.TRUE);
                         split(postTxnDuplicates);
-                        s_logger.info("Split duplicate person objects");
+                        logger.info("Split duplicate person objects");
                     }
                     else if (duplicateMode.equalsIgnoreCase(DELETE))
                     {
                         delete(postTxnDuplicates);
-                        s_logger.info("Deleted duplicate person objects");
+                        logger.info("Deleted duplicate person objects");
                     }
                     else
                     {
-                        if (s_logger.isDebugEnabled())
+                        if (logger.isDebugEnabled())
                         {
-                            s_logger.debug("Duplicate person objects exist");
+                            logger.debug("Duplicate person objects exist");
                         }
                     }
                 }
@@ -579,21 +586,33 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         return true;
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public boolean createMissingPeople()
     {
         return createMissingPeople;
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public Set<QName> getMutableProperties()
     {
         return mutableProperties;
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public void setPersonProperties(String userName, Map<QName, Serializable> properties)
     {
         setPersonProperties(userName, properties, true);
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public void setPersonProperties(String userName, Map<QName, Serializable> properties, boolean autoCreate)
     {
         NodeRef personNode = getPersonOrNull(userName);
@@ -631,6 +650,9 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         nodeService.setProperties(personNode, update);
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public boolean isMutable()
     {
         return true;
@@ -681,11 +703,17 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         return properties;
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public NodeRef createPerson(Map<QName, Serializable> properties)
     {
         return createPerson(properties, authorityService.getDefaultZones());
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public NodeRef createPerson(Map<QName, Serializable> properties, Set<String> zones)
     {
         String userName = DefaultTypeConverter.INSTANCE.convert(String.class, properties.get(ContentModel.PROP_USERNAME));
@@ -705,11 +733,21 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         properties.put(ContentModel.PROP_USERNAME, userName);
         properties.put(ContentModel.PROP_SIZE_CURRENT, 0L);
         
-        NodeRef personRef = nodeService.createNode(
+        NodeRef personRef = null;
-                getPeopleContainer(),
+        try
-                ContentModel.ASSOC_CHILDREN,
+        {
-                QName.createQName("cm", userName.toLowerCase(), namespacePrefixResolver), // Lowercase:
+            beforeCreateNodeValidationBehaviour.disable();
-                ContentModel.TYPE_PERSON, properties).getChildRef();
+            
+            personRef = nodeService.createNode(
+                    getPeopleContainer(),
+                    ContentModel.ASSOC_CHILDREN,
+                    getChildNameLower(userName), // Lowercase:
+                    ContentModel.TYPE_PERSON, properties).getChildRef();
+        }
+        finally
+        {
+            beforeCreateNodeValidationBehaviour.enable();
+        }
         
         if (zones != null)
         {
@@ -717,14 +755,18 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
             {
                 // Add the person to an authentication zone (corresponding to an external user registry)
                 // Let's preserve case on this child association
-                nodeService.addChild(authorityService.getOrCreateZone(zone), personRef, ContentModel.ASSOC_IN_ZONE, QName.createQName("cm", userName, namespacePrefixResolver));
+                nodeService.addChild(authorityService.getOrCreateZone(zone), personRef, ContentModel.ASSOC_IN_ZONE, QName.createQName(NamespaceService.CONTENT_MODEL_PREFIX, userName, namespacePrefixResolver));
             }
         }
         
-        personCache.remove(userName.toLowerCase());
+        removeFromCache(userName);
+        
         return personRef;
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public NodeRef getPeopleContainer()
     {
         String cacheKey = tenantService.getCurrentUserDomain();
@@ -758,6 +800,9 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         return peopleNodeRef;
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public void deletePerson(String userName)
     {
         // Normalize the username to avoid case sensitivity issues
@@ -767,38 +812,75 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
             return;
         }
         
-        // Remove internally-stored password information, if any
+        NodeRef personRef = getPersonOrNull(userName);
-        try
-        {
-            authenticationService.deleteAuthentication(userName);
-        }
-        catch (AuthenticationException e)
-        {
-            // Ignore this - externally authenticated user
-        }
         
-        // Invalidate all that user's tickets
+        deletePersonImpl(userName, personRef);
-        try
-        {
-            authenticationService.invalidateUserSession(userName);
-        }
-        catch (AuthenticationException e)
-        {
-            // Ignore this
-        }
-
-        // remove any user permissions
-        permissionServiceSPI.deletePermissions(userName);
-
-        // delete the person
-        NodeRef personNodeRef = getPersonOrNull(userName);
-        if (personNodeRef != null)
-        {
-            nodeService.deleteNode(personNodeRef);
-        }
-        personCache.remove(userName.toLowerCase());
     }
     
+    /**
+     * {@inheritDoc}
+     */
+    public void deletePerson(NodeRef personRef)
+    {
+        QName typeQName = nodeService.getType(personRef);
+        if (typeQName.equals(ContentModel.TYPE_PERSON))
+        {
+            String userName = (String) this.nodeService.getProperty(personRef, ContentModel.PROP_USERNAME);
+            deletePersonImpl(userName, personRef);
+        }
+        else
+        {
+            throw new AlfrescoRuntimeException("deletePerson: invalid type of node "+personRef+" (actual="+typeQName+", expected="+ContentModel.TYPE_PERSON+")");
+        }
+    }
+    
+    private void deletePersonImpl(String userName, NodeRef personRef)
+    {
+        if (userName != null)
+        {
+            // Remove internally-stored password information, if any
+            try
+            {
+                authenticationService.deleteAuthentication(userName);
+            }
+            catch (AuthenticationException e)
+            {
+                // Ignore this - externally authenticated user
+            }
+            
+            // Invalidate all that user's tickets
+            try
+            {
+                authenticationService.invalidateUserSession(userName);
+            }
+            catch (AuthenticationException e)
+            {
+                // Ignore this
+            }
+            
+            // remove any user permissions
+            permissionServiceSPI.deletePermissions(userName);
+        }
+        
+        // delete the person
+        if (personRef != null)
+        {
+            try
+            {
+                beforeDeleteNodeValidationBehaviour.disable();
+                
+                nodeService.deleteNode(personRef);
+            }
+            finally
+            {
+                beforeDeleteNodeValidationBehaviour.enable();
+            }
+        }
+    }
+    
+    /**
+     * {@inheritDoc}
+     */
     public Set<NodeRef> getAllPeople()
     {
         List<ChildAssociationRef> childRefs = nodeService.getChildAssocs(getPeopleContainer(),
@@ -811,6 +893,9 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         return refs;
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public Set<NodeRef> getPeopleFilteredByProperty(QName propertyKey, Serializable propertyValue)
     {
         // check that given property key is defined for content model type 'cm:person'
@@ -866,9 +951,15 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
     public void onCreateNode(ChildAssociationRef childAssocRef)
     {
         NodeRef personRef = childAssocRef.getChildRef();
-        String username = (String) this.nodeService.getProperty(personRef, ContentModel.PROP_USERNAME);
+        
-        personCache.remove(username.toLowerCase());
+        String userName = (String) this.nodeService.getProperty(personRef, ContentModel.PROP_USERNAME);
-        permissionsManager.setPermissions(personRef, username, username);
+        
+        if (getPeopleContainer().equals(childAssocRef.getParentRef()))
+        {
+            removeFromCache(userName);
+        }
+        
+        permissionsManager.setPermissions(personRef, userName, userName);
         
         // Make sure there is an authority entry - with a DB constraint for uniqueness
         // aclDao.createAuthority(username);
@@ -877,21 +968,95 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         homeFolderManager.onCreateNode(childAssocRef);
     }
     
-    /**
+    private QName getChildNameLower(String userName)
-     * {@inheritDoc}
+    {
-     */
+        return QName.createQName(NamespaceService.CONTENT_MODEL_PREFIX, userName.toLowerCase(), namespacePrefixResolver);
+    }
+    
+    public void beforeCreateNode(
+            NodeRef parentRef,
+            QName assocTypeQName,
+            QName assocQName,
+            QName nodeTypeQName)
+    {
+        // NOOP
+    }
+    
+    public void beforeCreateNodeValidation(
+            NodeRef parentRef,
+            QName assocTypeQName,
+            QName assocQName,
+            QName nodeTypeQName)
+    {
+        if (getPeopleContainer().equals(parentRef))
+        {
+            throw new AlfrescoRuntimeException("beforeCreateNode: use PersonService to create person");
+        }
+        else
+        {
+            logger.info("Person node is not being created under the people container (actual="+parentRef+", expected="+getPeopleContainer()+")");
+        }
+    }
+    
     public void beforeDeleteNode(NodeRef nodeRef)
     {
-        String username = (String) this.nodeService.getProperty(nodeRef, ContentModel.PROP_USERNAME);
+        String userName = (String) this.nodeService.getProperty(nodeRef, ContentModel.PROP_USERNAME);
-        if (this.authorityService.isGuestAuthority(username))
+        if (this.authorityService.isGuestAuthority(userName))
         {
-            throw new AlfrescoRuntimeException("The " + username + " user cannot be deleted.");
+            throw new AlfrescoRuntimeException("The " + userName + " user cannot be deleted.");
         }
-        this.personCache.remove(username.toLowerCase());
+        
+        NodeRef parentRef = null;
+        ChildAssociationRef parentAssocRef = nodeService.getPrimaryParent(nodeRef);
+        if (parentAssocRef != null)
+        {
+            parentRef = parentAssocRef.getParentRef();
+            if (getPeopleContainer().equals(parentRef))
+            {
+                removeFromCache(userName);
+            }
+        }
+    }
+    
+    public void beforeDeleteNodeValidation(NodeRef nodeRef)
+    {
+        NodeRef parentRef = null;
+        ChildAssociationRef parentAssocRef = nodeService.getPrimaryParent(nodeRef);
+        if (parentAssocRef != null)
+        {
+            parentRef = parentAssocRef.getParentRef();
+        }
+        
+        if (getPeopleContainer().equals(parentRef))
+        {
+            throw new AlfrescoRuntimeException("beforeDeleteNode: use PersonService to delete person");
+        }
+        else
+        {
+            logger.info("Person node that is being deleted is not under the parent people container (actual="+parentRef+", expected="+getPeopleContainer()+")");
+        }
+    }
+    
+    private Set<NodeRef> getFromCache(String userName)
+    {
+        return this.personCache.get(userName.toLowerCase());
+    }
+    
+    private void putToCache(String userName, Set<NodeRef> refs)
+    {
+        this.personCache.put(userName.toLowerCase(), refs);
+    }
+    
+    private void removeFromCache(String userName)
+    {
+        this.personCache.remove(userName.toLowerCase());
     }
 
     // IOC Setters
     
+    /**
+     * {@inheritDoc}
+     */
     public void setCreateMissingPeople(boolean createMissingPeople)
     {
         this.createMissingPeople = createMissingPeople;
@@ -957,6 +1122,9 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         this.storeRef = new StoreRef(storeUrl);
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public String getUserIdentifier(String caseSensitiveUserName)
     {
         NodeRef nodeRef = getPersonOrNull(caseSensitiveUserName);
@@ -1011,6 +1179,9 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
         }
     }
     
+    /**
+     * {@inheritDoc}
+     */
     public boolean getUserNamesAreCaseSensitive()
     {
         return userNameMatcher.getUserNamesAreCaseSensitive();
@@ -1045,15 +1216,14 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
                     aclDao.renameAuthority(uidBefore, uidAfter);
                 }
                 
-
                 // Fix primary association local name
-                QName newAssocQName = QName.createQName("cm", uidAfter.toLowerCase(), namespacePrefixResolver);
+                QName newAssocQName = getChildNameLower(uidAfter);
                 ChildAssociationRef assoc = nodeService.getPrimaryParent(nodeRef);
                 nodeService.moveNode(nodeRef, assoc.getParentRef(), assoc.getTypeQName(), newAssocQName);
                 
                 // Fix other non-case sensitive parent associations
-                QName oldAssocQName = QName.createQName("cm", uidBefore, namespacePrefixResolver);
+                QName oldAssocQName = QName.createQName(NamespaceService.CONTENT_MODEL_PREFIX, uidBefore, namespacePrefixResolver);
-                newAssocQName = QName.createQName("cm", uidAfter, namespacePrefixResolver);
+                newAssocQName = QName.createQName(NamespaceService.CONTENT_MODEL_PREFIX, uidAfter, namespacePrefixResolver);
                 for (ChildAssociationRef parent : nodeService.getParentAssocs(nodeRef))
                 {
                     if (!parent.isPrimary() && parent.getQName().equals(oldAssocQName))
@@ -1064,7 +1234,7 @@ public class PersonServiceImpl extends TransactionListenerAdapter implements Per
                 }
                 
                 // Fix cache
-                personCache.remove(uidBefore.toLowerCase());
+                removeFromCache(uidBefore);
             }
             else
             {

source/java/org/alfresco/repo/security/person/PersonTest.java

@@ -34,7 +34,9 @@ import javax.transaction.UserTransaction;
 import junit.framework.Assert;
 import junit.framework.TestCase;
 
+import org.alfresco.error.AlfrescoRuntimeException;
 import org.alfresco.model.ContentModel;
+import org.alfresco.repo.policy.BehaviourFilter;
 import org.alfresco.repo.security.authentication.AuthenticationUtil;
 import org.alfresco.repo.transaction.RetryingTransactionHelper;
 import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback;
@@ -57,12 +59,14 @@ import org.springframework.context.ApplicationContext;
 
 public class PersonTest extends TestCase
 {
-    private static ApplicationContext applicationContext = ApplicationContextHelper.getApplicationContext();
+    private static ApplicationContext ctx = ApplicationContextHelper.getApplicationContext();
     
     private TransactionService transactionService;
 
     private PersonService personService;
     
+    private BehaviourFilter policyBehaviourFilter;
+
     private NodeService nodeService;
 
     private NodeRef rootNodeRef;
@@ -81,12 +85,12 @@ public class PersonTest extends TestCase
 
     public void setUp() throws Exception
     {
-        transactionService = (TransactionService) applicationContext.getBean("transactionService");
+        transactionService = (TransactionService) ctx.getBean("transactionService");
-        personService = (PersonService) applicationContext.getBean("personService");
+        personService = (PersonService) ctx.getBean("personService");
-        nodeService = (NodeService) applicationContext.getBean("nodeService");
+        nodeService = (NodeService) ctx.getBean("nodeService");
-        permissionService = (PermissionService) applicationContext.getBean("permissionService");
+        permissionService = (PermissionService) ctx.getBean("permissionService");
-        authorityService = (AuthorityService) applicationContext.getBean("authorityService");
+        authorityService = (AuthorityService) ctx.getBean("authorityService");
-
+        policyBehaviourFilter = (BehaviourFilter) ctx.getBean("policyBehaviourFilter");
         
         testTX = transactionService.getUserTransaction();
         testTX.begin();
@@ -99,7 +103,7 @@ public class PersonTest extends TestCase
             String uid = DefaultTypeConverter.INSTANCE.convert(String.class, nodeService.getProperty(nodeRef, ContentModel.PROP_USERNAME));
             if (!uid.equals("admin") && !uid.equals("guest") )
             {
-                nodeService.deleteNode(nodeRef);
+                personService.deletePerson(nodeRef);
             }
         }
         
@@ -784,7 +788,6 @@ public class PersonTest extends TestCase
         final NodeRef[] duplicates = transactionService.getRetryingTransactionHelper().doInTransaction(
                 new RetryingTransactionCallback<NodeRef[]>()
                 {
-
                     public NodeRef[] execute() throws Throwable
                     {
                         NodeRef[] duplicates = new NodeRef[10];
@@ -796,20 +799,30 @@ public class PersonTest extends TestCase
                         List<ChildAssociationRef> parents = nodeService.getParentAssocs(duplicates[0]);
                         
                         // Generate some duplicates
-                        for (int i = 1; i < duplicates.length; i++)
+                        try
                         {
-                            // Create the node with the same parent assocs
+                            policyBehaviourFilter.disableBehaviour(ContentModel.TYPE_PERSON);
-                            duplicates[i] = nodeService.createNode(container.getParentRef(), container.getTypeQName(),
+                            
-                                    container.getQName(), ContentModel.TYPE_PERSON, properties).getChildRef();
+                            for (int i = 1; i < duplicates.length; i++)
-                            for (ChildAssociationRef parent : parents)
                             {
-                                if (!parent.isPrimary())
+                                // Create the node with the same parent assocs
+                                duplicates[i] = nodeService.createNode(container.getParentRef(), container.getTypeQName(),
+                                        container.getQName(), ContentModel.TYPE_PERSON, properties).getChildRef();
+                                for (ChildAssociationRef parent : parents)
                                 {
-                                    nodeService.addChild(parent.getParentRef(), duplicates[i], parent.getTypeQName(),
+                                    if (!parent.isPrimary())
-                                            parent.getQName());
+                                    {
+                                        nodeService.addChild(parent.getParentRef(), duplicates[i], parent.getTypeQName(),
+                                                parent.getQName());
+                                    }
                                 }
                             }
                         }
+                        finally
+                        {
+                            policyBehaviourFilter.enableBehaviour(ContentModel.TYPE_PERSON);
+                        }
+                        
                         // With the default settings, the last created node should be the one that wins
                         assertEquals(duplicates[duplicates.length - 1], personService.getPerson(duplicateUserName));
                         return duplicates;
@@ -842,4 +855,301 @@ public class PersonTest extends TestCase
             }
         }, false, true);
     }
+    
+    public void testCheckForDuplicateCaseInsensitive()
+    {
+        final String TEST_PERSON_MIXED = "Test_Person_One";
+        final String TEST_PERSON_UPPER = TEST_PERSON_MIXED.toUpperCase();
+        final String TEST_PERSON_LOWER = TEST_PERSON_MIXED.toLowerCase();
+        
+        AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName());
+        
+        final NodeRef peopleContainer = personService.getPeopleContainer();
+        
+        final Map<QName, Serializable> personProps = new HashMap<QName, Serializable>();
+        
+        personProps.put(ContentModel.PROP_HOMEFOLDER, peopleContainer);
+        personProps.put(ContentModel.PROP_FIRSTNAME, "test first name");
+        personProps.put(ContentModel.PROP_LASTNAME, "test last name");
+        personProps.put(ContentModel.PROP_SIZE_CURRENT, 0);
+        
+        RetryingTransactionHelper txnHelper = transactionService.getRetryingTransactionHelper();
+        
+        RetryingTransactionCallback<Void> callback = new RetryingTransactionCallback<Void>()
+        {
+            public Void execute() throws Throwable
+            {
+                if (! personService.personExists(TEST_PERSON_UPPER))
+                {
+                    personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_MIXED);
+                    personService.createPerson(personProps);
+                }
+                
+                return null;
+            }
+        };
+        
+        txnHelper.doInTransaction(callback);
+        
+        @SuppressWarnings("unused")
+        NodeRef personRef = null;
+        
+        // -ve test
+        try
+        {
+            @SuppressWarnings("unused")
+            ChildAssociationRef childAssocRef = nodeService.createNode(
+                        peopleContainer,
+                        ContentModel.ASSOC_CHILDREN,
+                        QName.createQName("{test}testperson"),
+                        ContentModel.TYPE_PERSON,
+                        personProps);
+            
+            fail("Shouldn't be able to create person node directly (within people container) - use createPerson instead");
+        }
+        catch (AlfrescoRuntimeException are)
+        {
+            if (! are.getMessage().contains("use PersonService"))
+            {
+                throw are;
+            }
+            // ignore - expected
+        }
+        
+        // -ve test
+        try
+        {
+            personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_LOWER);
+            personRef = personService.createPerson(personProps);
+            
+            fail("Shouldn't be able to create duplicate person");
+        }
+        catch (AlfrescoRuntimeException are)
+        {
+            if (! are.getMessage().contains("already exists"))
+            {
+                throw are;
+            }
+            // ignore - expected
+        }
+        
+        // -ve test
+        try
+        {
+            personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_UPPER);
+            personRef = personService.createPerson(personProps);
+            
+            fail("Shouldn't be able to create duplicate person");
+        }
+        catch (AlfrescoRuntimeException are)
+        {
+            if (! are.getMessage().contains("already exists"))
+            {
+                throw are;
+            }
+            // ignore - expected
+        }
+    }
+    
+    public void testCheckForDuplicateCaseSensitive()
+    {
+        final String TEST_PERSON_MIXED = "Test_Person_Two";
+        final String TEST_PERSON_UPPER = TEST_PERSON_MIXED.toUpperCase();
+        final String TEST_PERSON_LOWER = TEST_PERSON_MIXED.toLowerCase();
+        
+        UserNameMatcherImpl usernameMatcher = new UserNameMatcherImpl();
+        usernameMatcher.setUserNamesAreCaseSensitive(true);
+        ((PersonServiceImpl)personService).setUserNameMatcher(usernameMatcher); // case-sensitive
+        
+        AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName());
+        
+        final NodeRef peopleContainer = personService.getPeopleContainer();
+        
+        final Map<QName, Serializable> personProps = new HashMap<QName, Serializable>();
+        
+        personProps.put(ContentModel.PROP_HOMEFOLDER, peopleContainer);
+        personProps.put(ContentModel.PROP_FIRSTNAME, "test first name");
+        personProps.put(ContentModel.PROP_LASTNAME, "test last name");
+        personProps.put(ContentModel.PROP_SIZE_CURRENT, 0);
+        
+        RetryingTransactionHelper txnHelper = transactionService.getRetryingTransactionHelper();
+        
+        RetryingTransactionCallback<Void> callback = new RetryingTransactionCallback<Void>()
+        {
+            public Void execute() throws Throwable
+            {
+                if (! personService.personExists(TEST_PERSON_MIXED))
+                {
+                    personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_MIXED);
+                    personService.createPerson(personProps);
+                }
+                
+                return null;
+            }
+        };
+        
+        txnHelper.doInTransaction(callback);
+        
+        @SuppressWarnings("unused")
+        NodeRef personRef = null;
+        
+        personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_LOWER);
+        personRef = personService.createPerson(personProps);
+        
+        personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_UPPER);
+        personRef = personService.createPerson(personProps);
+        
+        // -ve test
+        try
+        {
+            personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_MIXED);
+            personRef = personService.createPerson(personProps);
+            
+            fail("Shouldn't be able to create duplicate person");
+        }
+        catch (AlfrescoRuntimeException are)
+        {
+            if (! are.getMessage().contains("already exists"))
+            {
+                throw are;
+            }
+            // ignore - expected
+        }
+        
+        // -ve test
+        try
+        {
+            personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_LOWER);
+            personRef = personService.createPerson(personProps);
+            
+            fail("Shouldn't be able to create duplicate person");
+        }
+        catch (AlfrescoRuntimeException are)
+        {
+            if (! are.getMessage().contains("already exists"))
+            {
+                throw are;
+            }
+            // ignore - expected
+        }
+        
+        // -ve test
+        try
+        {
+            personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_MIXED);
+            personRef = personService.createPerson(personProps);
+            
+            fail("Shouldn't be able to create duplicate person");
+        }
+        catch (AlfrescoRuntimeException are)
+        {
+            if (! are.getMessage().contains("already exists"))
+            {
+                throw are;
+            }
+            // ignore - expected
+        }
+        
+        usernameMatcher.setUserNamesAreCaseSensitive(false);
+        ((PersonServiceImpl)personService).setUserNameMatcher(usernameMatcher); // case-insensitive
+    }
+    
+    public void testUpdateUserNameCase()
+    {
+        final String TEST_PERSON_UPPER = "TEST_PERSON_THREE";
+        final String TEST_PERSON_LOWER = TEST_PERSON_UPPER.toLowerCase();
+        
+        AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName());
+        
+        RetryingTransactionHelper txnHelper = transactionService.getRetryingTransactionHelper();
+        
+        final Map<QName, Serializable> personProps = new HashMap<QName, Serializable>();
+        
+        personProps.put(ContentModel.PROP_HOMEFOLDER, rootNodeRef);
+        personProps.put(ContentModel.PROP_FIRSTNAME, "test first name ");
+        personProps.put(ContentModel.PROP_LASTNAME, "test last name");
+        personProps.put(ContentModel.PROP_SIZE_CURRENT, 0);
+        
+        RetryingTransactionCallback<NodeRef> callback = new RetryingTransactionCallback<NodeRef>()
+        {
+            public NodeRef execute() throws Throwable
+            {
+                personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON_LOWER);
+                return personService.createPerson(personProps);
+            }
+        };
+        
+        final NodeRef personRef = txnHelper.doInTransaction(callback);
+        
+        RetryingTransactionCallback<Void> callback2 = new RetryingTransactionCallback<Void>()
+        {
+            public Void execute() throws Throwable
+            {
+                nodeService.setProperty(personRef, ContentModel.PROP_USERNAME, TEST_PERSON_UPPER);
+                
+                return null;
+            }
+        };
+        
+        txnHelper.doInTransaction(callback2);
+    }
+    
+    public void testCheckForIndirectUsage()
+    {
+        final String TEST_PERSON = "Test_Person_Four";
+        
+        AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getAdminUserName());
+        
+        final NodeRef peopleContainer = personService.getPeopleContainer();
+        
+        final Map<QName, Serializable> personProps = new HashMap<QName, Serializable>();
+        
+        personProps.put(ContentModel.PROP_USERNAME, TEST_PERSON);
+        personProps.put(ContentModel.PROP_HOMEFOLDER, peopleContainer);
+        personProps.put(ContentModel.PROP_FIRSTNAME, "test first name");
+        personProps.put(ContentModel.PROP_LASTNAME, "test last name");
+        personProps.put(ContentModel.PROP_SIZE_CURRENT, 0);
+        
+        // -ve test
+        try
+        {
+            @SuppressWarnings("unused")
+            ChildAssociationRef childAssocRef = nodeService.createNode(
+                        peopleContainer,
+                        ContentModel.ASSOC_CHILDREN,
+                        QName.createQName("{test}testperson"),
+                        ContentModel.TYPE_PERSON,
+                        personProps);
+            
+            fail("Shouldn't be able to create person node directly (within people container) - use createPerson instead");
+        }
+        catch (AlfrescoRuntimeException are)
+        {
+            if (! are.getMessage().contains("use PersonService"))
+            {
+                throw are;
+            }
+            // ignore - expected
+        }
+        
+        NodeRef personRef = personService.createPerson(personProps);
+        
+        // -ve test
+        try
+        {
+            nodeService.deleteNode(personRef);
+            
+            fail("Shouldn't be able to delete person node directly (within people container) - use deletePerson instead");
+        }
+        catch (AlfrescoRuntimeException are)
+        {
+            if (! are.getMessage().contains("use PersonService"))
+            {
+                throw are;
+            }
+            // ignore - expected
+        }
+        
+        personService.deletePerson(TEST_PERSON);
+    }
 }

source/java/org/alfresco/service/cmr/security/PersonService.java

@@ -185,6 +185,14 @@ public interface PersonService
     @Auditable(parameters = {"userName"})
     public void deletePerson(String userName);
     
+    /**
+     * Delete the person identified by the given ref.
+     * 
+     * @param personRef
+     */
+    @Auditable(parameters = {"personRef"})
+    public void deletePerson(NodeRef personRef);
+
     /**
      * Get all the people we know about.
      *