MNT-20764: Searches fail for users who are members of groups where the authorityName contains double quotes (#239)

* Guard QName against invalid characters

Co-authored-by: Alex Mukha <alex.mukha@alfresco.com>

src/main/java/org/alfresco/service/namespace/QName.java

@@ -46,6 +46,7 @@ public final class QName implements QNamePattern, Serializable, Cloneable, Compa
     public static final String EMPTY_URI_SUBSTITUTE = ".empty";
     
     private static final long serialVersionUID = 3977016258204348976L;
+    private static final char[] illegalCharacters = {'/', '\\', '\r', '\n', '"'};
 
     private final String namespaceURI;                // never null
     private final String localName;                   // never null
@@ -69,10 +70,7 @@ public final class QName implements QNamePattern, Serializable, Cloneable, Compa
     public static QName createQName(String namespaceURI, String localName)
         throws InvalidQNameException
     {
-        if (localName == null || localName.length() == 0)
+        checkLocalName(localName);
-        {
-            throw new InvalidQNameException("A QName must consist of a local name");
-        }
         return new QName(namespaceURI, localName, null);
     }
 
@@ -89,10 +87,7 @@ public final class QName implements QNamePattern, Serializable, Cloneable, Compa
         throws InvalidQNameException, NamespaceException
     {
         // Validate Arguments
-        if (localName == null || localName.length() == 0)
+        checkLocalName(localName);
-        {
-            throw new InvalidQNameException("A QName must consist of a local name");
-        }
         if (prefixResolver == null)
         {
             throw new IllegalArgumentException("A Prefix Resolver must be specified"); 
@@ -174,10 +169,7 @@ public final class QName implements QNamePattern, Serializable, Cloneable, Compa
 
         // Parse name
         localName = qname.substring(namespaceEnd + 1);
-        if (localName == null || localName.length() == 0)
+        checkLocalName(localName);
-        {
-            throw new InvalidQNameException("QName '" + qname + "' must consist of a local name");
-        }
 
         // Construct QName
         return new QName(namespaceURI, localName, null);
@@ -203,11 +195,7 @@ public final class QName implements QNamePattern, Serializable, Cloneable, Compa
      */
     public static String createValidLocalName(String name)
     {
-        // Validate length
+        checkLocalName(name);
-        if (name == null || name.length() == 0)
-        {
-            throw new IllegalArgumentException("Local name cannot be null or empty.");
-        }
         if (name.length() > MAX_LENGTH)
         {
             name = name.substring(0, MAX_LENGTH);
@@ -216,6 +204,33 @@ public final class QName implements QNamePattern, Serializable, Cloneable, Compa
         return name;
     }
 
+    /**
+     * Condition to check string name provided for QName
+     *
+     * @param name  name to create valid local name from
+     * @throws InvalidQNameException
+     */
+    private static void checkLocalName(String name)
+        throws InvalidQNameException
+    {
+        if (name == null || name.length() == 0)
+        {
+            throw new InvalidQNameException("A QName must consist of a local name and cannot be null or empty");
+        }
+
+        if (name != null)
+        {
+            for (char illegalCharacter : illegalCharacters)
+            {
+                if (name.indexOf(illegalCharacter) != -1)
+                {
+                    throw new InvalidQNameException("Local name contains characters that are not permitted: "+name.charAt(name.indexOf(illegalCharacter)));
+                }
+            }
+        }
+
+    }
+
     /**
      * Create a QName
      * 
@@ -445,11 +460,7 @@ public final class QName implements QNamePattern, Serializable, Cloneable, Compa
     public static QName resolveToQName(NamespacePrefixResolver prefixResolver, String str)
     {
         QName qname = null;
-
+        checkLocalName(str);
-        if (str == null || str.length() == 0)
-        {
-            throw new IllegalArgumentException("str parameter is mandatory");
-        }
 
         if (str.charAt(0) == (NAMESPACE_BEGIN))
         {

src/test/java/org/alfresco/service/namespace/QNameTest.java

@@ -28,65 +28,50 @@ package org.alfresco.service.namespace;
 import java.util.Collection;
 import java.util.HashSet;
 
-import junit.framework.TestCase;
+import org.junit.Test;
-
+import static org.junit.Assert.assertEquals;
-
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.fail;
 
 /**
  * @see org.alfresco.service.namespace.QName
  * 
  * @author David Caruana
  */
-public class QNameTest extends TestCase
+public class QNameTest
 {
+    private final NamespacePrefixResolver mockResolver = new MockNamespacePrefixResolver();
 
-    public QNameTest(String name)
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameFromInternalStringRepresentationWithEmptyString()
     {
-        super(name);
+        QName.createQName("");
     }
 
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameFromInternalStringRepresentationWithInvalidNameCase1()
+    {
+        QName.createQName("invalid{}name");
+    }
 
-    public void testInvalidQName() throws Exception
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameFromInternalStringRepresentationWithInvalidNameCase2()
+    {
+        QName.createQName("{name");
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameFromInternalStringRepresentationWithInvalidNameCase3()
+    {
+        QName.createQName("{}");
+    }
+
+    @Test
+    public void testCreateQNameFromInternalStringRepresentationWithInvalidNameCase4()
     {
         try
         {
-            QName qname = QName.createQName("");
+            QName.createQName("{}name");
-            fail("Missing local name was not caught");
-        }
-        catch (InvalidQNameException e)
-        {
-        }
-
-        try
-        {
-            QName qname = QName.createQName("invalid{}name");
-            fail("Namespace not at start was not caught");
-        }
-        catch (InvalidQNameException e)
-        {
-        }
-
-        try
-        {
-            QName qname = QName.createQName("{name");
-            fail("Missing closing namespace token was not caught");
-        }
-        catch (InvalidQNameException e)
-        {
-        }
-
-        try
-        {
-            QName qname = QName.createQName("{}");
-            fail("Missing local name after namespace was not caught");
-        }
-        catch (InvalidQNameException e)
-        {
-        }
-
-        try
-        {
-            QName qname = QName.createQName("{}name");
         }
         catch (InvalidQNameException e)
         {
@@ -103,28 +88,152 @@ public class QNameTest extends TestCase
         {
             fail("Valid namespace has been thrown out");
         }
+    }
 
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameWithNoPrefixWithNullString()
+    {
+        QName.createQName(NamespaceService.ALFRESCO_PREFIX, (String) null);
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameWithNoPrefixWithEmptyString()
+    {
+        QName.createQName(NamespaceService.ALFRESCO_PREFIX, "");
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameWithPrefixWithNullString()
+    {
+        QName.createQName(NamespaceService.ALFRESCO_PREFIX, null, mockResolver);
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameWithPrefixWithEmptyString()
+    {
+        QName.createQName(NamespaceService.ALFRESCO_PREFIX, "", mockResolver);
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameWithPrefixFormatWithEmptyString()
+    {
+        QName.createQName("", mockResolver);
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateValidLocalNameWithNullString()
+    {
+        QName.createValidLocalName(null);
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateValidLocalNameWithEmptyString()
+    {
+        QName.createValidLocalName("");
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameResolveToNameWithNullString()
+    {
+        QName.resolveToQName(mockResolver, null);
+    }
+
+    @Test(expected = InvalidQNameException.class)
+    public void testCreateQNameResolveToNameWithEmptyString()
+    {
+        QName.resolveToQName(mockResolver, "");
+    }
+
+    @Test
+    public void testCreateQnameWithNoPrefixWithIllegalCharactersThrowsInvalidQNameException()
+    {
+        char[] illegalCharacters = {'/', '\\', '\n', '\r', '"'};
+        for (char illegalCharacter : illegalCharacters)
+        {
             try
             {
-            QName qname = QName.createQName((String) null, (String) null);
+                String localName = "testLocalNameWith" + illegalCharacter;
-            fail("Null name was not caught");
+                QName.createQName(NamespaceService.ALFRESCO_PREFIX, localName);
+                fail("InvalidQNameException not caught for illegalCharacter: " +localName.charAt(localName.indexOf(illegalCharacter)));
             }
-        catch (InvalidQNameException e)
+            catch (InvalidQNameException ignored)
             {
             }
+        }
+    }
 
+    @Test
+    public void testCreateQnameWithPrefixWithIllegalCharactersThrowsInvalidQNameException()
+    {
+        char[] illegalCharacters = {'/', '\\', '\n', '\r', '"'};
+        for (char illegalCharacter : illegalCharacters)
+        {
             try
             {
-            QName qname = QName.createQName((String) null, "");
+                String localName = "testLocalNameWith" + illegalCharacter;
-            fail("Empty name was not caught");
+                QName.createQName(NamespaceService.ALFRESCO_PREFIX, localName, mockResolver);
+                fail("InvalidQNameException not caught for illegalCharacter: " +localName.charAt(localName.indexOf(illegalCharacter)));
             }
-        catch (InvalidQNameException e)
+            catch (InvalidQNameException ignored)
             {
             }
-        
+        }
     }
 
+    @Test
+    public void testCreateQnameWithPrefixFormatWithIllegalCharactersThrowsInvalidQNameException()
+    {
+        char[] illegalCharacters = {'/', '\\', '\n', '\r', '"'};
+        for (char illegalCharacter : illegalCharacters)
+        {
+            try
+            {
+                String localName = "testPrefix:testLocalNameWith" + illegalCharacter;
+                QName.createQName(localName, mockResolver);
+                fail("InvalidQNameException not caught for illegalCharacter: " +localName.charAt(localName.indexOf(illegalCharacter)));
+            }
+            catch (InvalidQNameException ignored)
+            {
+            }
+        }
+    }
 
+    @Test
+    public void testCreateValidLocalNameWithIllegalCharactersThrowsInvalidQNameException()
+    {
+        char[] illegalCharacters = {'/', '\\', '\n', '\r', '"'};
+        for (char illegalCharacter : illegalCharacters)
+        {
+            try
+            {
+                String localName = "testNameWith" + illegalCharacter;
+                QName.createValidLocalName(localName);
+                fail("InvalidQNameException not caught for illegalCharacter: " +localName.charAt(localName.indexOf(illegalCharacter)));
+            }
+            catch (InvalidQNameException ignored)
+            {
+            }
+        }
+    }
+
+    @Test
+    public void testResolveToQNameWithIllegalCharactersThrowsInvalidQNameException() {
+        char[] illegalCharacters = {'/', '\\', '\n', '\r', '"'};
+        for (char illegalCharacter : illegalCharacters)
+        {
+            try
+            {
+                String localName = "testNameWith" + illegalCharacter;
+                QName.resolveToQName(mockResolver, localName);
+                fail("InvalidQNameException not caught for illegalCharacter: " +localName.charAt(localName.indexOf(illegalCharacter)));
+            }
+            catch (InvalidQNameException ignored)
+            {
+            }
+        }
+    }
+
+    @Test
     public void testConstruction()
     {
         QName qname1 = QName.createQName("namespace1", "name1");
@@ -148,7 +257,7 @@ public class QNameTest extends TestCase
         assertEquals("", qname6.getNamespaceURI());
     }
 
-
+    @Test
     public void testStringRepresentation()
     {
         QName qname1 = QName.createQName("namespace", "name1");
@@ -167,6 +276,7 @@ public class QNameTest extends TestCase
         assertEquals("{}name5", qname5.toString());
     }
 
+    @Test
     public void testCommonTypes()
     {
         QName qname3 = QName.createQName("{http://www.alfresco.org/model/content/1.0}created");
@@ -179,6 +289,7 @@ public class QNameTest extends TestCase
         assertEquals("{http://www.alfresco.org/model/content/1.0}content.mimetype", qname5.toString());
     }
 
+    @Test
     public void testEquality()
     {
         QName qname1 = QName.createQName("namespace", "name");
@@ -208,7 +319,7 @@ public class QNameTest extends TestCase
         assertFalse(qname9.hashCode() == qname10.hashCode());
     }
 
-
+    @Test
     public void testPrefix()
     {
         try
@@ -238,7 +349,6 @@ public class QNameTest extends TestCase
         }
     }
 
-    
     public static class MockNamespacePrefixResolver
         implements NamespacePrefixResolver
     {