From 3d30413841ed3f002b93a28df86829e1c63a231c Mon Sep 17 00:00:00 2001 From: Sri Sankaran Date: Tue, 6 May 2014 16:34:44 -0400 Subject: [PATCH] Fixed edge case bug with handling similar content paths one with variables and another without. Upgraded to 0.0.3 --- pom.xml | 7 +- src/main/java/com/redhat/trie/NodePair.java | 8 ++ src/main/java/com/redhat/trie/PathNode.java | 2 +- src/main/java/com/redhat/trie/PathTree.java | 80 +++++++------ .../java/com/redhat/trie/TestHelpers.java | 17 ++- .../java/com/redhat/trie/TestValidater.java | 106 ++++++++++++++++++ src/test/resources/another-contents.list | 1 + src/test/resources/emtpy.contents.list | 0 src/test/resources/log4j.properties | 4 + .../resources/variables-first-contents.list | 16 +++ .../resources/variables-last-contents.list | 16 +++ .../resources/variables-mixed-contents.list | 16 +++ 12 files changed, 235 insertions(+), 38 deletions(-) create mode 100644 src/test/java/com/redhat/trie/TestValidater.java create mode 100644 src/test/resources/another-contents.list create mode 100644 src/test/resources/emtpy.contents.list create mode 100644 src/test/resources/log4j.properties create mode 100644 src/test/resources/variables-first-contents.list create mode 100644 src/test/resources/variables-last-contents.list create mode 100644 src/test/resources/variables-mixed-contents.list diff --git a/pom.xml b/pom.xml index b03138b..b5e95b9 100644 --- a/pom.xml +++ b/pom.xml @@ -3,7 +3,7 @@ 4.0.0 com.redhat.trie PathPacker - 0.0.2 + 0.0.3 bundle @@ -22,6 +22,11 @@ bcprov-jdk16 1.46 + + org.apache.maven.surefire + surefire-booter + 2.17 + log4j diff --git a/src/main/java/com/redhat/trie/NodePair.java b/src/main/java/com/redhat/trie/NodePair.java index 1178434..93308d4 100644 --- a/src/main/java/com/redhat/trie/NodePair.java +++ b/src/main/java/com/redhat/trie/NodePair.java @@ -50,5 +50,13 @@ public class NodePair implements Comparable { public int compareTo(Object other) { return this.name.compareTo(((NodePair) other).name); } + + /** + * Does this NodePair have any children? + * @return true if it has at least one connection and false otherwise. + */ + public boolean hasNoChildren() { + return getConnection().getChildren().size() == 0; + } } diff --git a/src/main/java/com/redhat/trie/PathNode.java b/src/main/java/com/redhat/trie/PathNode.java index 245465f..fbddf56 100644 --- a/src/main/java/com/redhat/trie/PathNode.java +++ b/src/main/java/com/redhat/trie/PathNode.java @@ -28,7 +28,7 @@ import org.apache.log4j.Logger; * * It holds the relationships to children, as well as all parents that regard it as a child. * - * The Name of a given PathNode, is inferred by the NodePair that regards this PathNode as it's "connection" + * The Name of a given PathNode, is inferred by the NodePair that regards this PathNode as its "connection" */ public class PathNode { private static org.apache.log4j.Logger log = Logger.getLogger(PathTree.class); diff --git a/src/main/java/com/redhat/trie/PathTree.java b/src/main/java/com/redhat/trie/PathTree.java index fcb43ea..77ce089 100644 --- a/src/main/java/com/redhat/trie/PathTree.java +++ b/src/main/java/com/redhat/trie/PathTree.java @@ -308,40 +308,56 @@ public class PathTree { /** * Validate whether contentPath is included in this tree. * - * @param contentPath A String, like "/foo/bar/baz" - * @return true or false + * @param contentPath A String, like "/foo/bar/baz" + * @return true or false */ - public boolean validate(String contentPath) { - StringTokenizer st = new StringTokenizer(contentPath, "/"); - PathNode root; - PathNode pn; - String curTok; - - try { - root = this.getRootPathNode(); - } catch (PayloadException ex) { - log.error(ex); - return false; - } - - pn = root; - while (st.hasMoreTokens()) { - curTok = st.nextToken(); - - for (NodePair np : pn.getChildren()) { - if (curTok.equals(np.getName()) || np.getName().startsWith("$")) { - //System.out.println("[" + curTok + "] == [" + np.getName() + "]"); - if (np.getConnection().getChildren().size() == 0) { - return true; - } - - pn = np.getConnection(); - break; - } - } - } - + public boolean validate(final String contentPath) { + PathNode rootPathNode = null; + try { + rootPathNode = getRootPathNode(); + } catch(PayloadException pe) { + log.error(pe); return false; + } + return test(contentPath, rootPathNode); + } + + /** Character used to delimit client path request elements */ + private static String PATH_DELIMITER = "/"; + /** Character used as a variable name prefix in content path definitions */ + private static String CONTENT_PATH_VARIABLE_PREFIX = "$"; + /** + * Tests if the given path request is reachable via the current tree.. + * @param request The request to test. + * @param tree The content path tree. + * @return true if the path is reachable and false otherwise. + */ + private boolean test(final String request, final PathNode tree) { + /* Request is of the form "/content/rc/rhel/7/..." + * Grab the next element. + */ + log.debug("test(" + request + ")"); + StringTokenizer tokenizer = new StringTokenizer(request, PATH_DELIMITER); + if(tokenizer.countTokens() == 0) { + return false; + } + String currentToken = tokenizer.nextToken(); + for(NodePair nodePair: tree.getChildren()) { + String nodePairName = nodePair.getName(); + log.debug("Current token: [" + currentToken + "] =??= NodePair name: [" + nodePairName + "]"); + if(currentToken.equals(nodePairName) || nodePairName.startsWith(CONTENT_PATH_VARIABLE_PREFIX)) { + if(nodePair.hasNoChildren()) { + return true; + } else { + String s = PATH_DELIMITER + currentToken; + boolean retval = test(request.substring(currentToken.length()+1), nodePair.getConnection()); + if(retval) { + return true; + } + } + } + } + return false; } /** diff --git a/src/test/java/com/redhat/trie/TestHelpers.java b/src/test/java/com/redhat/trie/TestHelpers.java index 6f5fe93..9b8d051 100644 --- a/src/test/java/com/redhat/trie/TestHelpers.java +++ b/src/test/java/com/redhat/trie/TestHelpers.java @@ -16,11 +16,13 @@ import static org.junit.Assert.fail; import static org.junit.Assert.assertNotNull; import org.junit.Test; +import org.apache.log4j.Logger; /** * This class is just to provide helpers for the other tests */ public class TestHelpers { + private static org.apache.log4j.Logger log = Logger.getLogger(TestHelpers.class); /** * junit requires at least one runnable test @@ -33,8 +35,8 @@ public class TestHelpers { // Helpers // public static boolean cmpStrings(List thisList, List thatList) { - Collection thisColl = new ArrayList(thisList); - Collection thatColl = new ArrayList(thatList); + Collection thisColl = new ArrayList(thisList); + Collection thatColl = new ArrayList(thatList); Collection similar = new HashSet( thisColl ); Collection different = new HashSet(); @@ -43,7 +45,7 @@ public class TestHelpers { similar.retainAll( thatColl ); different.removeAll( similar ); - + if (different.size() > 0) { System.out.printf("Different:%s%n", different); } @@ -91,7 +93,14 @@ public class TestHelpers { String content; List contentList = new ArrayList(); InputStream in = resStream(klass, filename); - BufferedReader br = new BufferedReader(new InputStreamReader(in)); + BufferedReader br = null; + try { + br = new BufferedReader(new InputStreamReader(in)); + } catch(NullPointerException npe) { + // can happen in the case of an empty content set list + log.warn(">>>>>>> Empty content set <<<<<"); + return contentList; + } try { while ((content = br.readLine()) != null) { diff --git a/src/test/java/com/redhat/trie/TestValidater.java b/src/test/java/com/redhat/trie/TestValidater.java new file mode 100644 index 0000000..0fecf76 --- /dev/null +++ b/src/test/java/com/redhat/trie/TestValidater.java @@ -0,0 +1,106 @@ +package com.redhat.trie; + +import static org.junit.Assert.*; + +import java.util.Arrays; +import java.util.Collection; +import java.util.List; + +import org.apache.log4j.Logger; +import org.apache.log4j.Level; + +import org.junit.Test; +import org.junit.Before; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +/** + * Tests of whether a URL should be granted access based on a given content set + * (as would be specified in the requester's entitlement certificate). + * + */ +@RunWith(value = Parameterized.class) +public class TestValidater { + private String contentSetFileName; + private String requestedPath; + private boolean expectedResult; + private PathTree pathTree; + + @Parameters + public static Collection data() { + /* + * Data for each run of the test. + * [0] = The name of the file containing content set paths + * [1] = The requested resource + * [3] = The expected result. + */ + Object[][] data = new Object[][] { + /* + * WARNING -- + * Do not uncomment empty.contents.list cases unless + * PathTree has been updated to handle that case. + * + { "empty.contents.list", "/content/rc/rhel/7/vt", false}, + { "empty.contents.list", "/content/rc/rhel/7", false}, + */ + { "contents.list", "/content/beta/rhel/server/5/5server/x86_64/sap/os/repomd.xml", true}, + { "contents.list", "/fart/face/mcjones", false}, + { "contents.list", "/content/dist/rhel/server/6/$releasever/$basearch/vt/os", false}, + + { "variables-last-contents.list", "/content/rc/rhel/server/7/x86_64/debug", true}, + { "variables-last-contents.list", "/content/rc/rhel/server/7/x86_64/debug/tools/firefox", true}, + { "variables-last-contents.list", "/content/rc/rhel/server/7/x86_64/Debug", false}, + { "variables-last-contents.list", "/content/rc/rhel/server/7/Debug", false}, + { "variables-last-contents.list", "/content/rc/rhel/server/7/x86_64/os", true}, + + { "variables-first-contents.list", "/content/rc/rhel/server/7/x86_64/debug", true}, + { "variables-first-contents.list", "/content/rc/rhel/server/7/x86_64/debug/tools/firefox", true}, + { "variables-first-contents.list", "/content/rc/rhel/server/7/x86_64/Debug", false}, + { "variables-first-contents.list", "/content/rc/rhel/server/7/Debug", false}, + { "variables-first-contents.list", "/content/rc/rhel/server/7/x86_64/os", true}, + + { "variables-mixed-contents.list", "/content/rc/rhel/server/7/x86_64/debug", true}, + { "variables-mixed-contents.list", "/content/rc/rhel/server/7/x86_64/debug/tools/firefox", true}, + { "variables-mixed-contents.list", "/content/rc/rhel/server/7/x86_64/Debug", false}, + { "variables-mixed-contents.list", "/content/rc/rhel/server/7/Debug", false}, + { "variables-mixed-contents.list", "/content/rc/rhel/server/7/x86_64/os", true}, + + { "another-contents.list", "/content/rc/rhel/server/7/x86_64/os", true}, + { "another-contents.list", "/content/rc/rhel/server/7/x86_64/debug/os", false} + }; + return Arrays.asList(data); + } + + /** + * Instatiate a test class with the given parameters. + * @param contentSetFileName The name of the file containing the allowed content sets. + * @param requestedPath The resource being requested + * @param expectedResult The correct response. + */ + public TestValidater(final String contentSetFileName, final String requestedPath, final boolean expectedResult) { + this.contentSetFileName = contentSetFileName; + this.requestedPath = requestedPath; + this.expectedResult = expectedResult; + } + + /** + * Prepare for a test run. + */ + @Before + public void setup() throws Exception { + List validContentPaths = TestHelpers.loadContents(this, contentSetFileName); + pathTree = new PathTree(); + pathTree.setContentSets(validContentPaths); + assertTrue(TestHelpers.cmpStrings(validContentPaths, pathTree.toList())); + } + + /** + * Run the test. + */ + @Test + public void justDoIt() throws Exception { + assertEquals("[" + requestedPath + "] " + (expectedResult?"allowed ":"not allowed ") + "using " + contentSetFileName, + expectedResult, pathTree.validate(requestedPath)); + } +} diff --git a/src/test/resources/another-contents.list b/src/test/resources/another-contents.list new file mode 100644 index 0000000..0d8e946 --- /dev/null +++ b/src/test/resources/another-contents.list @@ -0,0 +1 @@ +/content/rc/rhel/server/$releasever/$basearch/os diff --git a/src/test/resources/emtpy.contents.list b/src/test/resources/emtpy.contents.list new file mode 100644 index 0000000..e69de29 diff --git a/src/test/resources/log4j.properties b/src/test/resources/log4j.properties new file mode 100644 index 0000000..373f7a1 --- /dev/null +++ b/src/test/resources/log4j.properties @@ -0,0 +1,4 @@ +log4j.rootLogger=DEBUG, R +log4j.appender.R=org.apache.log4j.ConsoleAppender +log4j.appender.R.layout=org.apache.log4j.PatternLayout +log4j.appender.R.layout.ConversionPattern=[%-5p] %c{1} - %m%n diff --git a/src/test/resources/variables-first-contents.list b/src/test/resources/variables-first-contents.list new file mode 100644 index 0000000..e159489 --- /dev/null +++ b/src/test/resources/variables-first-contents.list @@ -0,0 +1,16 @@ +/content/rc/rhel/server/7/$basearch/highavailability/debug +/content/rc/rhel/server/7/$basearch/highavailability/os +/content/rc/rhel/server/7/$basearch/highavailability/source/SRPMS +/content/rc/rhel/server/7/$basearch/resilientstorage/debug +/content/rc/rhel/server/7/$basearch/resilientstorage/os +/content/rc/rhel/server/7/$basearch/resilientstorage/source/SRPMS +/content/rc/rhel/server/7/x86_64/debug +/content/rc/rhel/server/7/x86_64/iso +/content/rc/rhel/server/7/x86_64/kickstart +/content/rc/rhel/server/7/x86_64/os +/content/rc/rhel/server/7/x86_64/source/iso +/content/rc/rhel/server/7/x86_64/source/SRPMS +/content/rc/rhel/server/7/x86_64/supplementary/debug +/content/rc/rhel/server/7/x86_64/supplementary/iso +/content/rc/rhel/server/7/x86_64/supplementary/os +/content/rc/rhel/server/7/x86_64/supplementary/source/SRPMS diff --git a/src/test/resources/variables-last-contents.list b/src/test/resources/variables-last-contents.list new file mode 100644 index 0000000..8a22e8e --- /dev/null +++ b/src/test/resources/variables-last-contents.list @@ -0,0 +1,16 @@ +/content/rc/rhel/server/7/x86_64/debug +/content/rc/rhel/server/7/x86_64/iso +/content/rc/rhel/server/7/x86_64/kickstart +/content/rc/rhel/server/7/x86_64/os +/content/rc/rhel/server/7/x86_64/source/iso +/content/rc/rhel/server/7/x86_64/source/SRPMS +/content/rc/rhel/server/7/x86_64/supplementary/debug +/content/rc/rhel/server/7/x86_64/supplementary/iso +/content/rc/rhel/server/7/x86_64/supplementary/os +/content/rc/rhel/server/7/x86_64/supplementary/source/SRPMS +/content/rc/rhel/server/7/$basearch/highavailability/debug +/content/rc/rhel/server/7/$basearch/highavailability/os +/content/rc/rhel/server/7/$basearch/highavailability/source/SRPMS +/content/rc/rhel/server/7/$basearch/resilientstorage/debug +/content/rc/rhel/server/7/$basearch/resilientstorage/os +/content/rc/rhel/server/7/$basearch/resilientstorage/source/SRPMS diff --git a/src/test/resources/variables-mixed-contents.list b/src/test/resources/variables-mixed-contents.list new file mode 100644 index 0000000..f23e154 --- /dev/null +++ b/src/test/resources/variables-mixed-contents.list @@ -0,0 +1,16 @@ +/content/rc/rhel/server/7/$basearch/highavailability/debug +/content/rc/rhel/server/7/$basearch/resilientstorage/debug +/content/rc/rhel/server/7/$basearch/resilientstorage/source/SRPMS +/content/rc/rhel/server/7/x86_64/debug +/content/rc/rhel/server/7/$basearch/highavailability/os +/content/rc/rhel/server/7/$basearch/highavailability/source/SRPMS +/content/rc/rhel/server/7/x86_64/iso +/content/rc/rhel/server/7/x86_64/kickstart +/content/rc/rhel/server/7/x86_64/os +/content/rc/rhel/server/7/x86_64/source/iso +/content/rc/rhel/server/7/x86_64/source/SRPMS +/content/rc/rhel/server/7/$basearch/resilientstorage/os +/content/rc/rhel/server/7/x86_64/supplementary/debug +/content/rc/rhel/server/7/x86_64/supplementary/iso +/content/rc/rhel/server/7/x86_64/supplementary/os +/content/rc/rhel/server/7/x86_64/supplementary/source/SRPMS