This commit contains a fix for QPID-2600

Added a test case to check if all allowed chars are accepted and the rest is rejected.
Added a check for empty continuation lines.
Improved error reporting by adding a line number.
Removed test_allowed_chars_for_username method from acl.py as the check for group name will flag the "@" char, making this test case redundent.


git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk/qpid@943351 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 72 insertions, 61 deletions

diff --git a/cpp/src/qpid/acl/AclReader.cpp b/cpp/src/qpid/acl/AclReader.cpp
index 2f59f4453f..9a39347399 100644
--- a/cpp/src/qpid/acl/AclReader.cpp
+++ b/cpp/src/qpid/acl/AclReader.cpp
@@ -297,13 +297,19 @@ int AclReader::read(const std::string& fn, boost::shared_ptr<AclData> d) {
 bool AclReader::processLine(char* line) {
     bool ret = false;
     std::vector<std::string> toks;
-
+  
     // Check for continuation
     char* contCharPtr = std::strrchr(line, '\\');
     bool cont = contCharPtr != 0;
     if (cont) *contCharPtr = 0;
 
     int numToks = tokenize(line, toks);
+    
+    if (cont && numToks == 0){
+      errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line \"" << lineNumber << "\" contains an illegal extension.";
+      return false;
+    }
+
     if (numToks && (toks[0].compare("group") == 0 || contFlag)) {
         ret = processGroupLine(toks, cont);
     } else if (numToks && toks[0].compare("acl") == 0) {
@@ -317,7 +323,8 @@ bool AclReader::processLine(char* line) {
         if (ws) {
             ret = true;
         } else {
-            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Non-continuation line must start with \"group\" or \"acl\".";
+            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber 
+                   << ", Non-continuation line must start with \"group\" or \"acl\".";
             ret = false;
         }
     }
@@ -341,32 +348,27 @@ int  AclReader::tokenize(char* line, std::vector<std::string>& toks) {
 // If cont is true, then groupName must be set to the continuation group name
 bool AclReader::processGroupLine(tokList& toks, const bool cont) {
     const unsigned toksSize = toks.size();
+    
     if (contFlag) {
         gmCitr citr = groups.find(groupName);
         for (unsigned i = 0; i < toksSize; i++) {
-            if (!checkName(toks[i])) {
-                errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Name \"" << toks[i] << "\" contains illegal characters.";
-                return false;
-            }
             if (!isValidUserName(toks[i])) return false;
             addName(toks[i], citr->second);
         }
     } else {
         if (toksSize < (cont ? 2 : 3)) {
-            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Insufficient tokens for group definition.";
+            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber 
+                        << ", Insufficient tokens for group definition.";
             return false;
         }
-        if (!checkName(toks[1])) {
-            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Group name \"" << toks[1] << "\" contains illegal characters.";
+        if (!isValidGroupName(toks[1])) {
+            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                        << ", Group name \"" << toks[1] << "\" contains illegal characters.";
             return false;
         }
         gmCitr citr = addGroup(toks[1]);
         if (citr == groups.end()) return false;
         for (unsigned i = 2; i < toksSize; i++) {
-            if (!checkName(toks[i])) {
-                errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Name \"" << toks[i] << "\" contains illegal characters.";
-                return false;
-            }
             if (!isValidUserName(toks[i])) return false;
             addName(toks[i], citr->second);
         }
@@ -378,7 +380,8 @@ bool AclReader::processGroupLine(tokList& toks, const bool cont) {
 AclReader::gmCitr AclReader::addGroup(const std::string& newGroupName) {
     gmCitr citr = groups.find(newGroupName);
     if (citr != groups.end()) {
-        errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Duplicate group name \"" << newGroupName << "\".";
+        errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                    << ", Duplicate group name \"" << newGroupName << "\".";
         return groups.end();
     }
     groupPair p(newGroupName, nameSetPtr(new nameSet));
@@ -431,7 +434,8 @@ void AclReader::printNames() const {
 bool AclReader::processAclLine(tokList& toks) {
     const unsigned toksSize = toks.size();
     if (toksSize < 4) {
-        errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Insufficient tokens for acl definition.";
+        errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                    << ", Insufficient tokens for acl definition.";
         return false;
     }
 
@@ -439,7 +443,8 @@ bool AclReader::processAclLine(tokList& toks) {
     try {
         res = AclHelper::getAclResult(toks[1]);
     } catch (...) {
-        errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Unknown ACL permission \"" << toks[1] << "\".";
+        errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                    << ", Unknown ACL permission \"" << toks[1] << "\".";
         return false;
     }
 
@@ -449,7 +454,8 @@ bool AclReader::processAclLine(tokList& toks) {
     if (actionAllFlag) {
 
         if (userAllFlag && toksSize > 4) {
-            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Tokens found after action \"all\".";
+            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                        << ", Tokens found after action \"all\".";
             return false;
         }
         action = ACT_CONSUME; // dummy; compiler must initialize action for this code path
@@ -457,7 +463,8 @@ bool AclReader::processAclLine(tokList& toks) {
         try {
             action = AclHelper::getAction(toks[3]);
         } catch (...) {
-            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Unknown action \"" << toks[3] << "\".";
+            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                        << ", Unknown action \"" << toks[3] << "\".";
             return false;
         }
     }
@@ -477,7 +484,8 @@ bool AclReader::processAclLine(tokList& toks) {
             try {
                 rule->setObjectType(AclHelper::getObjectType(toks[4]));
             } catch (...) {
-                errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Unknown object \"" << toks[4] << "\".";
+                errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                            << ", Unknown object \"" << toks[4] << "\".";
                 return false;
             }
         }
@@ -487,14 +495,17 @@ bool AclReader::processAclLine(tokList& toks) {
         for (unsigned i=5; i<toksSize; i++) {
             nvPair propNvp = splitNameValuePair(toks[i]);
             if (propNvp.second.size() == 0) {
-                errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Badly formed property name-value pair \"" << propNvp.first << "\". (Must be name=value)";
+                errorStream << ACL_FORMAT_ERR_LOG_PREFIX <<  "Line : " << lineNumber
+                            <<", Badly formed property name-value pair \"" 
+                            << propNvp.first << "\". (Must be name=value)";
                 return false;
             }
             Property prop;
             try {
                 prop = AclHelper::getProperty(propNvp.first);
             } catch (...) {
-                errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Unknown property \"" << propNvp.first << "\".";
+                errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                            << ", Unknown property \"" << propNvp.first << "\".";
                 return false;
             }
             rule->addProperty(prop, propNvp.second);
@@ -509,7 +520,8 @@ bool AclReader::processAclLine(tokList& toks) {
 
     // If rule validates, add to rule list
     if (!rule->validate(validationMap)) {
-        errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Invalid object/action/property combination.";
+        errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                    << ", Invalid object/action/property combination.";
         return false;
     }
     rules.push_back(rule);
@@ -528,10 +540,10 @@ void AclReader::printRules() const {
 
 // Static function
 // Return true if the name is well-formed (ie contains legal characters)
-bool AclReader::checkName(const std::string& name) {
+bool AclReader::isValidGroupName(const std::string& name) {
     for (unsigned i=0; i<name.size(); i++) {
         const char ch = name.at(i);
-        if (!std::isalnum(ch) && ch != '-' && ch != '_' && ch != '@' && ch != '.') return false;
+        if (!std::isalnum(ch) && ch != '-' && ch != '_') return false;
     }
     return true;
 }
@@ -548,11 +560,20 @@ AclReader::nvPair AclReader::splitNameValuePair(const std::string& nvpString) {
 
 // Returns true if a username has the name@realm format
 bool AclReader::isValidUserName(const std::string& name){
-	size_t pos = name.find('@');
+    size_t pos = name.find('@');
 	if ( pos == std::string::npos || pos == name.length() -1){
-		errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Username '" << name << "' must contain a realm";
+		errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                    << ", Username '" << name << "' must contain a realm";
 		return false;
 	}
+    for (unsigned i=0; i<name.size(); i++) {
+        const char ch = name.at(i);
+        if (!std::isalnum(ch) && ch != '-' && ch != '_' && ch != '@' && ch != '.' && ch != '/'){
+            errorStream << ACL_FORMAT_ERR_LOG_PREFIX << "Line : " << lineNumber
+                        << ", Username \"" << name << "\" contains illegal characters.";
+            return false;
+        }
+    }
 	return true;
 }
 
diff --git a/cpp/src/qpid/acl/AclReader.h b/cpp/src/qpid/acl/AclReader.h
index dccb450192..62c6f38f37 100644
--- a/cpp/src/qpid/acl/AclReader.h
+++ b/cpp/src/qpid/acl/AclReader.h
@@ -109,7 +109,7 @@ class AclReader {
     void printRules() const; // debug aid
     bool isValidUserName(const std::string& name);
 
-    static bool checkName(const std::string& name);
+    static bool isValidGroupName(const std::string& name);
     static nvPair splitNameValuePair(const std::string& nvpString);
 };
 
diff --git a/cpp/src/tests/acl.py b/cpp/src/tests/acl.py
index 96dd7934c7..a7725b36a6 100755
--- a/cpp/src/tests/acl.py
+++ b/cpp/src/tests/acl.py
@@ -117,36 +117,6 @@ class ACLTests(TestBase010):
         except qpid.session.SessionException, e:
             self.assertEqual(403,e.args[0].error_code)                
         
-
-    def test_group_and_user_with_same_name(self):
-        """
-        Test a group and user with same name
-        Ex. group admin admin 
-        """
-        aclf = ACLFile()
-        aclf.write('group bob@QPID bob@QPID\n')
-        aclf.write('acl deny bob@QPID bind exchange\n')
-        aclf.write('acl allow all all')
-        aclf.close()
-
-        result = self.reload_acl()
-        if (result.text.find("format error",0,len(result.text)) != -1):
-            self.fail(result) 
-
-        session = self.get_session('bob','bob')
-        try:
-            session.queue_declare(queue="allow_queue")
-        except qpid.session.SessionException, e:
-            if (403 == e.args[0].error_code):
-                self.fail("ACL should allow queue create request");
-            self.fail("Error during queue create request");
-
-        try:
-            session.exchange_bind(exchange="amq.direct", queue="allow_queue", binding_key="routing_key")
-            self.fail("ACL should deny queue bind request");
-        except qpid.session.SessionException, e:
-            self.assertEqual(403,e.args[0].error_code)
-       
  
    #=====================================
    # ACL file format tests
@@ -185,23 +155,26 @@ class ACLTests(TestBase010):
         """
          
         aclf = ACLFile()
-        aclf.write('group admins bob@QPID \ ')
+        aclf.write('group admins bob@QPID \n')
         aclf.write('          \ \n')
         aclf.write('joe@QPID \n')
         aclf.write('acl allow all all')
         aclf.close()        
         
         result = self.reload_acl()       
-        if (result.text.find("contains illegal characters",0,len(result.text)) == -1):
+        if (result.text.find("contains an illegal extension",0,len(result.text)) == -1):
+            self.fail(result)
+
+        if (result.text.find("Non-continuation line must start with \"group\" or \"acl\"",0,len(result.text)) == -1):
             self.fail(result)
 
+
     def test_user_domain(self):
         """
         Test a user defined without a realm
         Ex. group admin rajith
         """
-        aclf = ACLFile()
-        aclf.write('group test joe@EXAMPLE.com\n') # should be allowed
+        aclf = ACLFile()        
         aclf.write('group admin bob\n') # shouldn't be allowed
         aclf.write('acl deny admin bind exchange\n')
         aclf.write('acl allow all all')
@@ -211,6 +184,23 @@ class ACLTests(TestBase010):
         if (result.text.find("Username 'bob' must contain a realm",0,len(result.text)) == -1):
             self.fail(result)
 
+    def test_allowed_chars_for_username(self):
+        """
+        Test a user defined without a realm
+        Ex. group admin rajith
+        """
+        aclf = ACLFile()        
+        aclf.write('group test1 joe@EXAMPLE.com\n') # should be allowed
+        aclf.write('group test2 jack-jill@EXAMPLE.com\n') # should be allowed
+        aclf.write('group test3 jack_jill@EXAMPLE.com\n') # should be allowed
+        aclf.write('group test4 host/somemachine.example.com@EXAMPLE.COM\n') # should be allowed
+        aclf.write('acl allow all all')
+        aclf.close()
+         
+        result = self.reload_acl()
+        if (result.text.find("ACL format error",0,len(result.text)) != -1):
+            self.fail(result)
+
    #=====================================
    # ACL validation tests
    #=====================================