Address reviews. Fix reversed parameters. Compare path elements, not whole paths. #3110

.gitignore

@@ -19,7 +19,7 @@ tags
 overlays/
 
 ## Ignore project files created by NetBeans
-nbproject/private/
+nbproject/
 build/
 nbbuild/
 dist/
@@ -41,4 +41,4 @@ nb-configuration.xml
 .DS_Store
 
 ##Ignore JRebel project configuration
-rebel.xml
+rebel.xml

dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/URLUtils.java

@@ -54,35 +54,55 @@ public class URLUtils {
 
     /**
      * Is one URL a prefix of another?  Ignores credentials, fragments and queries.
-     * @param candidate
-     * @param pattern
+     * @param pattern the potential prefix.
+     * @param candidate does this URL match the pattern?
      * @return {@code true} if the URLs have equal protocol, host and port,
-     *         and {@code pattern}'s path {@code String.startsWith} {@code candidate}'s path.
+     *         and {@code candidate}'s path {@code String.startsWith} {@code pattern}'s path.
      * @throws IllegalArgumentException if either URL is malformed.
      */
-    public static boolean urlIsPrefixOf(String candidate, String pattern)
+    public static boolean urlIsPrefixOf(String pattern, String candidate)
             throws IllegalArgumentException {
-        URL candidateURL;
         URL patternURL;
+        URL candidateURL;
+
         try {
-            candidateURL = new URL(candidate);
             patternURL = new URL(pattern);
         } catch (MalformedURLException e) {
-            throw new IllegalArgumentException("the supplied uri is not valid: " + pattern);
+            throw new IllegalArgumentException("The pattern URL is not valid:  " + pattern);
+        }
+
+        try {
+            candidateURL = new URL(candidate);
+        } catch (MalformedURLException e) {
+            throw new IllegalArgumentException("The candidate URL is not valid:  " + candidate);
+        }
+
+        // Deal with port defaults.
+        int patternPort = patternURL.getPort();
+        if (patternPort < 0) {
+            patternPort = patternURL.getDefaultPort();
         }
         int candidatePort = candidateURL.getPort();
         if (candidatePort < 0) {
             candidatePort = candidateURL.getDefaultPort();
         }
-        int patternPort = patternURL.getPort();
-        if (patternPort < 0) {
-            patternPort = patternURL.getDefaultPort();
+
+        boolean isPrefix;
+        isPrefix = StringUtils.equals(candidateURL.getProtocol(), patternURL.getProtocol());
+        isPrefix &= StringUtils.equals(candidateURL.getHost(), patternURL.getHost());
+        isPrefix &= candidatePort == patternPort;
+
+        String[] candidateElements = StringUtils.split(candidateURL.getPath(), '/');
+        String[] patternElements = StringUtils.split(patternURL.getPath(), '/');
+
+        // Candidate path cannot be shorter than pattern path.
+        if (patternElements.length > candidateElements.length) {
+            return false;
+        }
+        for (int elementN = 0; elementN < patternElements.length; elementN++) {
+            isPrefix &= candidateElements[elementN].equals(patternElements[elementN]);
         }
 
-        // FIXME paths should be compared element-by-element, not string-wise.
-        return StringUtils.equals(patternURL.getProtocol(), candidateURL.getProtocol())
-                && StringUtils.equals(patternURL.getHost(), candidateURL.getHost())
-                && patternPort == candidatePort
-                && StringUtils.startsWith(patternURL.getPath(), candidateURL.getPath());
+        return isPrefix;
     }
 }

dspace-server-webapp/src/main/java/org/dspace/app/rest/utils/Utils.java

@@ -930,13 +930,9 @@ public class Utils {
     */
     public BaseObjectRest getBaseObjectRestFromUri(Context context, String uri) throws SQLException {
         String dspaceUrl = configurationService.getProperty("dspace.server.url");
-        // first check if the uri could be valid
-        if (!urlIsPrefixOf(dspaceUrl, uri)) {
-            throw new IllegalArgumentException("the supplied uri is not ours: " + uri);
-        }
 
-        // Extract from the URI the category, model and id components.
-        // They start after the dspaceUrl/api/{apiCategory}/{apiModel}/{id}
+        // Convert strings to URL objects.
+        // Do this early to check that inputs are well-formed.
         URL dspaceUrlObject;
         URL requestUrlObject;
         try {
@@ -946,6 +942,14 @@ public class Utils {
             throw new IllegalArgumentException(
                     String.format("Configuration '%s' or request '%s' is malformed", dspaceUrl, uri));
         }
+
+        // Check whether the URI could be valid.
+        if (!urlIsPrefixOf(dspaceUrl, uri)) {
+            throw new IllegalArgumentException("the supplied uri is not ours: " + uri);
+        }
+
+        // Extract from the URI the category, model and id components.
+        // They start after the dspaceUrl/api/{apiCategory}/{apiModel}/{id}
         int dspacePathLength = StringUtils.split(dspaceUrlObject.getPath(), '/').length;
         String[] requestPath = StringUtils.split(requestUrlObject.getPath(), '/');
         String[] uriParts = Arrays.copyOfRange(requestPath, dspacePathLength,

dspace-server-webapp/src/test/java/org/dspace/app/rest/utils/URLUtilsTest.java

@@ -61,6 +61,13 @@ public class URLUtilsTest {
         isPrefix = URLUtils.urlIsPrefixOf("http://example.com/path1/a", "http://example.com/path2/a");
         assertFalse("Should not match if paths don't match", isPrefix);
 
+        isPrefix = URLUtils.urlIsPrefixOf("http://example.com/path", "http://example.com/path/");
+        assertTrue("Should match with, without trailing slash", isPrefix);
+        isPrefix = URLUtils.urlIsPrefixOf("http://example.com/path1", "http://example.com/path2");
+        assertFalse("Should not match if paths don't match", isPrefix);
+        isPrefix = URLUtils.urlIsPrefixOf("http://example.com/path", "http://example.com/path2/sub");
+        assertFalse("Should not match if interior path elements don't match", isPrefix);
+
         // Check if a malformed URL raises an exception
         isPrefix = URLUtils.urlIsPrefixOf(null, "http://example.com/");
     }