SEC-204: Improve startup time detection of errors by FilterInvocationDefinitionSourceEditor.

core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionMap.java

@@ -1,4 +1,4 @@
-/* Copyright 2004 Acegi Technology Pty Limited
+/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,8 +28,10 @@ import org.acegisecurity.ConfigAttributeDefinition;
 public interface FilterInvocationDefinitionMap {
     //~ Methods ================================================================
 
+    public void addSecureUrl(String expression, ConfigAttributeDefinition attr);
+
+    public boolean isConvertUrlToLowercaseBeforeComparison();
+
     public void setConvertUrlToLowercaseBeforeComparison(
         boolean convertUrlToLowercaseBeforeComparison);
-
-    public void addSecureUrl(String expression, ConfigAttributeDefinition attr);
 }

core/src/main/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditor.java

@@ -55,6 +55,9 @@ public class FilterInvocationDefinitionSourceEditor
     //~ Static fields/initializers =============================================
 
     private static final Log logger = LogFactory.getLog(FilterInvocationDefinitionSourceEditor.class);
+    public static final String DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON =
+        "CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON";
+    public static final String DIRECTIVE_PATTERN_TYPE_APACHE_ANT = "PATTERN_TYPE_APACHE_ANT";
 
     //~ Methods ================================================================
 
@@ -65,14 +68,27 @@ public class FilterInvocationDefinitionSourceEditor
             // Leave target object empty
         } else {
             // Check if we need to override the default definition map
-            if (s.lastIndexOf("PATTERN_TYPE_APACHE_ANT") != -1) {
+            if (s.lastIndexOf(DIRECTIVE_PATTERN_TYPE_APACHE_ANT) != -1) {
                 source = new PathBasedFilterInvocationDefinitionMap();
 
                 if (logger.isDebugEnabled()) {
-                    logger.debug(("Detected PATTERN_TYPE_APACHE_ANT directive; using Apache Ant style path expressions"));
+                    logger.debug(("Detected "
+                        + DIRECTIVE_PATTERN_TYPE_APACHE_ANT
+                        + " directive; using Apache Ant style path expressions"));
                 }
             }
 
+            if (s.lastIndexOf(
+                    DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON) != -1) {
+                if (logger.isDebugEnabled()) {
+                    logger.debug("Detected "
+                        + DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON
+                        + " directive; Instructing mapper to convert URLs to lowercase before comparison");
+                }
+
+                source.setConvertUrlToLowercaseBeforeComparison(true);
+            }
+
             BufferedReader br = new BufferedReader(new StringReader(s));
             int counter = 0;
             String line;
@@ -100,24 +116,36 @@ public class FilterInvocationDefinitionSourceEditor
                     continue;
                 }
 
-                if (line.equals("CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON")) {
+                // Attempt to detect malformed lines (as per SEC-204)
-                    if (logger.isDebugEnabled()) {
+                if (line.lastIndexOf(
-                        logger.debug("Line " + counter
+                        DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON) != -1) {
-                            + ": Instructing mapper to convert URLs to lowercase before comparison");
+                    // Directive found; check for second directive or name=value
+                    if ((line.lastIndexOf(DIRECTIVE_PATTERN_TYPE_APACHE_ANT) != -1)
+                        || (line.lastIndexOf("=") != -1)) {
+                        throw new IllegalArgumentException(
+                            "Line appears to be malformed: " + line);
                     }
-
-                    source.setConvertUrlToLowercaseBeforeComparison(true);
-
-                    continue;
                 }
 
+                // Attempt to detect malformed lines (as per SEC-204)
+                if (line.lastIndexOf(DIRECTIVE_PATTERN_TYPE_APACHE_ANT) != -1) {
+                    // Directive found; check for second directive or name=value
+                    if ((line.lastIndexOf(
+                            DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON) != -1)
+                        || (line.lastIndexOf("=") != -1)) {
+                        throw new IllegalArgumentException(
+                            "Line appears to be malformed: " + line);
+                    }
+                }
+
+                // Skip lines that are not directives
                 if (line.lastIndexOf('=') == -1) {
                     continue;
                 }
 
                 if (line.lastIndexOf("==") != -1) {
                     throw new IllegalArgumentException(
-                            "Only single equals should be used in line " + line);
+                        "Only single equals should be used in line " + line);
                 }
 
                 // Tokenize the line into its name/value tokens
@@ -129,7 +157,25 @@ public class FilterInvocationDefinitionSourceEditor
                     throw new IllegalArgumentException(
                         "Failed to parse a valid name/value pair from " + line);
                 }
-                
+
+                // Attempt to detect malformed lines (as per SEC-204)
+                if (source.isConvertUrlToLowercaseBeforeComparison()
+                    && source instanceof PathBasedFilterInvocationDefinitionMap) {
+                    // Should all be lowercase; check each character
+                    // We only do this for Ant (regexp have control chars)
+                    for (int i = 0; i < name.length(); i++) {
+                        String character = name.substring(i, i + 1);
+
+                        if (!character.toLowerCase().equals(character)) {
+                            throw new IllegalArgumentException(
+                                "You are using the "
+                                + DIRECTIVE_CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON
+                                + " with Ant Paths, yet you have specified an uppercase character in line: "
+                                + line + " (character '" + character + "')");
+                        }
+                    }
+                }
+
                 // Convert value to series of security configuration attributes
                 ConfigAttributeEditor configAttribEd = new ConfigAttributeEditor();
                 configAttribEd.setAsText(value);

core/src/test/java/org/acegisecurity/intercept/web/FilterInvocationDefinitionSourceEditorTests.java

@@ -1,4 +1,4 @@
-/* Copyright 2004 Acegi Technology Pty Limited
+/* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -19,15 +19,13 @@ import junit.framework.TestCase;
 
 import org.acegisecurity.ConfigAttributeDefinition;
 import org.acegisecurity.MockFilterChain;
-
-
 import org.acegisecurity.SecurityConfig;
 
-import java.util.Iterator;
-
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 
+import java.util.Iterator;
+
 
 /**
  * Tests {@link FilterInvocationDefinitionSourceEditor} and its associated
@@ -49,14 +47,14 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase {
 
     //~ Methods ================================================================
 
-    public final void setUp() throws Exception {
-        super.setUp();
-    }
-
     public static void main(String[] args) {
         junit.textui.TestRunner.run(FilterInvocationDefinitionSourceEditorTests.class);
     }
 
+    public final void setUp() throws Exception {
+        super.setUp();
+    }
+
     public void testConvertUrlToLowercaseDefaultSettingUnchangedByEditor() {
         FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor();
         editor.setAsText(
@@ -67,6 +65,19 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase {
         assertFalse(map.isConvertUrlToLowercaseBeforeComparison());
     }
 
+    public void testConvertUrlToLowercaseDetectsUppercaseEntries() {
+        FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor();
+
+        try {
+            editor.setAsText(
+                "CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON\r\nPATTERN_TYPE_APACHE_ANT\r\n\\/secUre/super/**=ROLE_WE_DONT_HAVE");
+            fail("Should have thrown IllegalArgumentException");
+        } catch (IllegalArgumentException expected) {
+            assertTrue(expected.getMessage()
+                               .lastIndexOf("you have specified an uppercase character in line") != -1);
+        }
+    }
+
     public void testConvertUrlToLowercaseSettingApplied() {
         FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor();
         editor.setAsText(
@@ -87,6 +98,45 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase {
         assertTrue(map instanceof RegExpBasedFilterInvocationDefinitionMap);
     }
 
+    public void testDetectsDuplicateDirectivesOnSameLineSituation1() {
+        FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor();
+
+        try {
+            editor.setAsText(
+                "CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON PATTERN_TYPE_APACHE_ANT\r\n\\/secure/super/**=ROLE_WE_DONT_HAVE");
+            fail("Should have thrown IllegalArgumentException");
+        } catch (IllegalArgumentException expected) {
+            assertTrue(expected.getMessage()
+                               .lastIndexOf("Line appears to be malformed") != -1);
+        }
+    }
+
+    public void testDetectsDuplicateDirectivesOnSameLineSituation2() {
+        FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor();
+
+        try {
+            editor.setAsText(
+                "CONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON\r\nPATTERN_TYPE_APACHE_ANT /secure/super/**=ROLE_WE_DONT_HAVE");
+            fail("Should have thrown IllegalArgumentException");
+        } catch (IllegalArgumentException expected) {
+            assertTrue(expected.getMessage()
+                               .lastIndexOf("Line appears to be malformed") != -1);
+        }
+    }
+
+    public void testDetectsDuplicateDirectivesOnSameLineSituation3() {
+        FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor();
+
+        try {
+            editor.setAsText(
+                "PATTERN_TYPE_APACHE_ANT\r\nCONVERT_URL_TO_LOWERCASE_BEFORE_COMPARISON /secure/super/**=ROLE_WE_DONT_HAVE");
+            fail("Should have thrown IllegalArgumentException");
+        } catch (IllegalArgumentException expected) {
+            assertTrue(expected.getMessage()
+                               .lastIndexOf("Line appears to be malformed") != -1);
+        }
+    }
+
     public void testEmptyStringReturnsEmptyMap() {
         FilterInvocationDefinitionSourceEditor editor = new FilterInvocationDefinitionSourceEditor();
         editor.setAsText("");
@@ -158,7 +208,7 @@ public class FilterInvocationDefinitionSourceEditorTests extends TestCase {
         Class clazz = RegExpBasedFilterInvocationDefinitionMap.EntryHolder.class;
 
         try {
-            clazz.getDeclaredConstructor((Class[])null);
+            clazz.getDeclaredConstructor((Class[]) null);
             fail("Should have thrown NoSuchMethodException");
         } catch (NoSuchMethodException expected) {
             assertTrue(true);