[SSHD-893] Fix SCP download with pattern issue in rooted filesystem
authorThe-Yoda <yoda.thi.master@gmail.com>
Mon, 11 Feb 2019 13:11:20 +0000 (15:11 +0200)
committerLyor Goldstein <lgoldstein@apache.org>
Tue, 12 Feb 2019 10:02:19 +0000 (12:02 +0200)
sshd-common/src/main/java/org/apache/sshd/common/util/io/DirectoryScanner.java
sshd-common/src/test/java/org/apache/sshd/common/util/io/DirectoryScannerTest.java [new file with mode: 0644]
sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpFileOpener.java
sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpHelper.java
sshd-scp/src/test/java/org/apache/sshd/client/scp/ScpTest.java

index 5ba7f75..c75c889 100644 (file)
 package org.apache.sshd.common.util.io;
 
 import java.io.File;
+import java.io.IOException;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
 import org.apache.sshd.common.util.GenericUtils;
+import org.apache.sshd.common.util.OsUtils;
 import org.apache.sshd.common.util.SelectorUtils;
 
 /**
@@ -115,49 +126,37 @@ import org.apache.sshd.common.util.SelectorUtils;
  * @author <a href="mailto:levylambert@tiscali-dsl.de">Antoine Levy-Lambert</a>
  */
 public class DirectoryScanner {
-
     /**
      * The base directory to be scanned.
      */
-    protected File basedir;
+    private Path basedir;
 
     /**
      * The patterns for the files to be included.
      */
-    protected String[] includes;
-
-    /**
-     * The files which matched at least one include and no excludes
-     * and were selected.
-     */
-    protected List<String> filesIncluded;
+    private List<String> includePatterns;
 
     /**
-     * Whether or not the file system should be treated as a case sensitive
-     * one.
+     * Whether or not the file system should be treated as
+     * a case sensitive one.
      */
-    protected boolean isCaseSensitive = true;
+    private boolean caseSensitive = OsUtils.isUNIX();
 
     public DirectoryScanner() {
         super();
     }
 
-    public DirectoryScanner(String basedir, String... includes) {
-        setBasedir(basedir);
-        setIncludes(includes);
+    public DirectoryScanner(Path dir) {
+        this(dir, Collections.emptyList());
     }
 
-    /**
-     * Sets the base directory to be scanned. This is the directory which is
-     * scanned recursively. All '/' and '\' characters are replaced by
-     * <code>File.separatorChar</code>, so the separator used need not match
-     * <code>File.separatorChar</code>.
-     *
-     * @param basedir The base directory to scan.
-     *                Must not be {@code null}.
-     */
-    public void setBasedir(String basedir) {
-        setBasedir(new File(basedir.replace('/', File.separatorChar).replace('\\', File.separatorChar)));
+    public DirectoryScanner(Path dir, String... includes) {
+        this(dir, GenericUtils.isEmpty(includes) ? Collections.emptyList() : Arrays.asList(includes));
+    }
+
+    public DirectoryScanner(Path dir, Collection<String> includes) {
+        setBasedir(dir);
+        setIncludes(includes);
     }
 
     /**
@@ -167,7 +166,7 @@ public class DirectoryScanner {
      * @param basedir The base directory for scanning.
      *                Should not be {@code null}.
      */
-    public void setBasedir(File basedir) {
+    public void setBasedir(Path basedir) {
         this.basedir = basedir;
     }
 
@@ -177,7 +176,7 @@ public class DirectoryScanner {
      *
      * @return the base directory to be scanned
      */
-    public File getBasedir() {
+    public Path getBasedir() {
         return basedir;
     }
 
@@ -188,21 +187,36 @@ public class DirectoryScanner {
      *
      * <p>When a pattern ends with a '/' or '\', "**" is appended.</p>
      *
-     * @param includes A list of include patterns.
-     *                 May be {@code null}, indicating that all files
-     *                 should be included. If a non-{@code null}
-     *                 list is given, all elements must be
-     *                 non-{@code null}.
+     * @param includes A list of include patterns. May be {@code null}, indicating
+     * that all files should be included. If a non-{@code null} list is given, all
+     * elements must be non-{@code null}.
      */
-    public void setIncludes(String[] includes) {
-        if (includes == null) {
-            this.includes = null;
-        } else {
-            this.includes = new String[includes.length];
-            for (int i = 0; i < includes.length; i++) {
-                this.includes[i] = normalizePattern(includes[i]);
-            }
-        }
+    public void setIncludes(String... includes) {
+        setIncludes(GenericUtils.isEmpty(includes) ? Collections.emptyList() : Arrays.asList(includes));
+    }
+
+    /**
+     * @return Un-modifiable list of the inclusion patterns
+     */
+    public List<String> getIncludes() {
+        return includePatterns;
+    }
+
+    public void setIncludes(Collection<String> includes) {
+        this.includePatterns = GenericUtils.isEmpty(includes)
+            ? Collections.emptyList()
+            : Collections.unmodifiableList(
+                    includes.stream()
+                        .map(v -> normalizePattern(v))
+                        .collect(Collectors.toCollection(() -> new ArrayList<>(includes.size()))));
+    }
+
+    public boolean isCaseSensitive() {
+        return caseSensitive;
+    }
+
+    public void setCaseSensitive(boolean caseSensitive) {
+        this.caseSensitive = caseSensitive;
     }
 
     /**
@@ -211,31 +225,30 @@ public class DirectoryScanner {
      * then the files must pass muster there, as well.
      *
      * @return the matching files
-     * @throws IllegalStateException if the base directory was set
-     *                               incorrectly (i.e. if it is {@code null}, doesn't exist,
-     *                               or isn't a directory).
+     * @throws IllegalStateException if the base directory was set incorrectly
+     * (i.e. if it is {@code null}, doesn't exist, or isn't a directory).
+     * @throws IOExcepion if failed to scan the directory (e.g., access denied)
      */
-    public String[] scan() throws IllegalStateException {
-        if (basedir == null) {
+    public Collection<String> scan() throws IOException, IllegalStateException {
+        return scan(LinkedList::new);
+    }
+
+    public <C extends Collection<String>> C scan(Supplier<? extends C> factory) throws IOException, IllegalStateException {
+        Path dir = getBasedir();
+        if (dir == null) {
             throw new IllegalStateException("No basedir set");
         }
-        if (!basedir.exists()) {
-            throw new IllegalStateException("basedir " + basedir
-                    + " does not exist");
+        if (!Files.exists(dir)) {
+            throw new IllegalStateException("basedir " + dir + " does not exist");
         }
-        if (!basedir.isDirectory()) {
-            throw new IllegalStateException("basedir " + basedir
-                    + " is not a directory");
+        if (!Files.isDirectory(dir)) {
+            throw new IllegalStateException("basedir " + dir + " is not a directory");
         }
-        if (includes == null || includes.length == 0) {
-            throw new IllegalStateException("No includes set ");
+        if (GenericUtils.isEmpty(getIncludes())) {
+            throw new IllegalStateException("No includes set for " + dir);
         }
 
-        filesIncluded = new ArrayList<>();
-
-        scandir(basedir, "");
-
-        return getIncludedFiles();
+        return scandir(dir, "", factory.get());
     }
 
     /**
@@ -244,46 +257,35 @@ public class DirectoryScanner {
      * matching of includes, excludes, and the selectors. When a directory
      * is found, it is scanned recursively.
      *
-     * @param dir   The directory to scan. Must not be {@code null}.
-     * @param vpath The path relative to the base directory (needed to
-     *              prevent problems with an absolute path when using
-     *              dir). Must not be {@code null}.
+     * @param <C> Target matches collection type
+     * @param dir The directory to scan. Must not be {@code null}.
+     * @param vpath The path relative to the base directory (needed to prevent
+     * problems with an absolute path when using <tt>dir</tt>). Must not be {@code null}.
+     * @param filesList Target {@link Collection} to accumulate the relative
+     * path matches
+     * @throws IOException if failed to scan the directory
      */
-    protected void scandir(File dir, String vpath) {
-        String[] newfiles = dir.list();
-        if (GenericUtils.isEmpty(newfiles)) {
-            newfiles = GenericUtils.EMPTY_STRING_ARRAY;
-        }
-
-        for (String newfile : newfiles) {
-            String name = vpath + newfile;
-            File file = new File(dir, newfile);
-            if (file.isDirectory()) {
-                if (isIncluded(name)) {
-                    filesIncluded.add(name);
-                    scandir(file, name + File.separator);
-                } else if (couldHoldIncluded(name)) {
-                    scandir(file, name + File.separator);
-                }
-            } else if (file.isFile()) {
-                if (isIncluded(name)) {
-                    filesIncluded.add(name);
+    protected <C extends Collection<String>> C scandir(Path dir, String vpath, C filesList) throws IOException {
+        try (DirectoryStream<Path> ds = Files.newDirectoryStream(dir)) {
+            for (Path p : ds) {
+                Path n = p.getFileName();
+                String name = vpath + n;
+                if (Files.isDirectory(p)) {
+                    if (isIncluded(name)) {
+                        filesList.add(name);
+                        scandir(p, name + File.separator, filesList);
+                    } else if (couldHoldIncluded(name)) {
+                        scandir(p, name + File.separator, filesList);
+                    }
+                } else if (Files.isRegularFile(p)) {
+                    if (isIncluded(name)) {
+                        filesList.add(name);
+                    }
                 }
             }
         }
-    }
 
-    /**
-     * Returns the names of the files which matched at least one of the
-     * include patterns and none of the exclude patterns.
-     * The names are relative to the base directory.
-     *
-     * @return the names of the files which matched at least one of the
-     * include patterns and none of the exclude patterns.
-     */
-    public String[] getIncludedFiles() {
-        String[] files = new String[filesIncluded.size()];
-        return filesIncluded.toArray(files);
+        return filesList;
     }
 
     /**
@@ -295,11 +297,18 @@ public class DirectoryScanner {
      * include pattern, or <code>false</code> otherwise.
      */
     protected boolean isIncluded(String name) {
+        Collection<String> includes = getIncludes();
+        if (GenericUtils.isEmpty(includes)) {
+            return false;
+        }
+
+        boolean cs = isCaseSensitive();
         for (String include : includes) {
-            if (SelectorUtils.matchPath(include, name, isCaseSensitive)) {
+            if (SelectorUtils.matchPath(include, name, cs)) {
                 return true;
             }
         }
+
         return false;
     }
 
@@ -312,11 +321,18 @@ public class DirectoryScanner {
      * least one include pattern, or <code>false</code> otherwise.
      */
     protected boolean couldHoldIncluded(String name) {
+        Collection<String> includes = getIncludes();
+        if (GenericUtils.isEmpty(includes)) {
+            return false;
+        }
+
+        boolean cs = isCaseSensitive();
         for (String include : includes) {
-            if (SelectorUtils.matchPatternStart(include, name, isCaseSensitive)) {
+            if (SelectorUtils.matchPatternStart(include, name, cs)) {
                 return true;
             }
         }
+
         return false;
     }
 
@@ -326,7 +342,7 @@ public class DirectoryScanner {
      * @param pattern The pattern to normalize, must not be {@code null}.
      * @return The normalized pattern, never {@code null}.
      */
-    private String normalizePattern(String pattern) {
+    public static String normalizePattern(String pattern) {
         pattern = pattern.trim();
 
         if (pattern.startsWith(SelectorUtils.REGEX_HANDLER_PREFIX)) {
@@ -364,8 +380,8 @@ public class DirectoryScanner {
             return text;
         }
 
-        StringBuilder buf = new StringBuilder(text.length());
         int start = 0;
+        StringBuilder buf = new StringBuilder(text.length());
         for (int end = text.indexOf(repl, start); end != -1; end = text.indexOf(repl, start)) {
             buf.append(text.substring(start, end)).append(with);
             start = end + repl.length();
diff --git a/sshd-common/src/test/java/org/apache/sshd/common/util/io/DirectoryScannerTest.java b/sshd-common/src/test/java/org/apache/sshd/common/util/io/DirectoryScannerTest.java
new file mode 100644 (file)
index 0000000..86f488b
--- /dev/null
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.util.io;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
+
+import org.apache.sshd.util.test.CommonTestSupportUtils;
+import org.apache.sshd.util.test.JUnitTestSupport;
+import org.apache.sshd.util.test.NoIoTestCase;
+import org.junit.FixMethodOrder;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runners.MethodSorters;
+
+/**
+ * TODO Add javadoc
+ *
+ * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
+ */
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
+@Category({ NoIoTestCase.class })
+public class DirectoryScannerTest extends JUnitTestSupport {
+    public DirectoryScannerTest() {
+        super();
+    }
+
+    @Test
+    public void testDeepScanning() throws IOException {
+        Path rootDir = getTempTargetRelativeFile(getClass().getSimpleName(), getCurrentTestName());
+        CommonTestSupportUtils.deleteRecursive(rootDir);    // start fresh
+
+        List<String> expected = new ArrayList<>();
+        Path curLevel = rootDir;
+        for (int level = 1; level <= 3; level++) {
+            Path dir = Files.createDirectories(curLevel.resolve(Integer.toString(level)));
+            Path name = rootDir.relativize(dir);
+            expected.add(name.toString());
+            Path file = dir.resolve(Integer.toString(level) + ".txt");
+            Files.write(file, Collections.singletonList(file.toString()), StandardCharsets.UTF_8);
+
+            name = rootDir.relativize(file);
+            expected.add(name.toString());
+            curLevel = dir;
+        }
+        Collections.sort(expected);
+
+        DirectoryScanner ds = new DirectoryScanner(rootDir, "**/*");
+        List<String> actual = ds.scan(ArrayList::new);
+        Collections.sort(actual);
+        assertListEquals(getCurrentTestName(), expected, actual);
+    }
+
+    @Test
+    public void testFileSuffixMatching() throws IOException {
+        Path rootDir = getTempTargetRelativeFile(getClass().getSimpleName(), getCurrentTestName());
+        CommonTestSupportUtils.deleteRecursive(rootDir);    // start fresh
+        Files.createDirectories(rootDir);
+
+        List<String> expected = new ArrayList<>();
+        for (int level = 1; level <= Byte.SIZE; level++) {
+            Path file = rootDir.resolve(Integer.toString(level) + (((level & 0x03) == 0) ? ".csv" : ".txt"));
+            Files.write(file, Collections.singletonList(file.toString()), StandardCharsets.UTF_8);
+            String name = Objects.toString(file.getFileName());
+            if (name.endsWith(".txt")) {
+                expected.add(name);
+            }
+        }
+        Collections.sort(expected);
+
+        DirectoryScanner ds = new DirectoryScanner(rootDir, "*.txt");
+        List<String> actual = ds.scan(ArrayList::new);
+        Collections.sort(actual);
+        assertListEquals(getCurrentTestName(), expected, actual);
+    }
+}
index eab3e11..1d8f5cd 100644 (file)
@@ -35,14 +35,11 @@ import java.nio.file.attribute.BasicFileAttributeView;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.attribute.FileTime;
 import java.nio.file.attribute.PosixFilePermission;
-import java.util.Arrays;
-import java.util.Collections;
 import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import org.apache.sshd.common.SshException;
 import org.apache.sshd.common.session.Session;
-import org.apache.sshd.common.util.GenericUtils;
 import org.apache.sshd.common.util.SelectorUtils;
 import org.apache.sshd.common.util.io.DirectoryScanner;
 import org.apache.sshd.common.util.io.IoUtils;
@@ -121,14 +118,11 @@ public interface ScpFileOpener {
      * @param basedir The base directory - may be {@code null}/empty to indicate CWD
      * @param pattern The required pattern
      * @return The matching <U>relative paths</U> of the children to send
+     * @throws IOException If failed to scan the directory
      */
-    default Iterable<String> getMatchingFilesToSend(Session session, String basedir, String pattern) {
-        String[] matches = new DirectoryScanner(basedir, pattern).scan();
-        if (GenericUtils.isEmpty(matches)) {
-            return Collections.emptyList();
-        }
-
-        return Arrays.asList(matches);
+    default Iterable<String> getMatchingFilesToSend(Session session, Path basedir, String pattern) throws IOException {
+        DirectoryScanner ds = new DirectoryScanner(basedir, pattern);
+        return ds.scan();
     }
 
     /**
index e51d2f4..fed18d5 100644 (file)
@@ -401,9 +401,11 @@ public class ScpHelper extends AbstractLoggingBean implements SessionHolder<Sess
                 }
 
                 Session session = getSession();
-                Iterable<String> included = opener.getMatchingFilesToSend(session, basedir, pattern);
+                Path basePath = resolveLocalPath(basedir);
+                Iterable<String> included =
+                    opener.getMatchingFilesToSend(session, basePath, pattern);
                 for (String path : included) {
-                    Path file = resolveLocalPath(basedir, path);
+                    Path file = basePath.resolve(path);
                     if (opener.sendAsRegularFile(session, file, options)) {
                         sendFile(file, preserve, bufferSize);
                     } else if (opener.sendAsDirectory(session, file, options)) {
index 2611fff..7f5955d 100644 (file)
@@ -124,11 +124,11 @@ public class ScpTest extends BaseTestSupport {
             }
             StringBuilder sb = new StringBuilder(Byte.MAX_VALUE);
             sb.append("    ").append(type)
-                    .append('[').append(s).append(']')
-                    .append('[').append(op).append(']')
-                    .append(' ').append(isFile ? "File" : "Directory").append('=').append(path)
-                    .append(' ').append("length=").append(length)
-                    .append(' ').append("perms=").append(perms);
+                .append('[').append(s).append(']')
+                .append('[').append(op).append(']')
+                .append(' ').append(isFile ? "File" : "Directory").append('=').append(path)
+                .append(' ').append("length=").append(length)
+                .append(' ').append("perms=").append(perms);
             if (t != null) {
                 sb.append(' ').append("ERROR=").append(t.getClass().getSimpleName()).append(": ").append(t.getMessage());
             }
@@ -554,7 +554,7 @@ public class ScpTest extends BaseTestSupport {
         }
     }
 
-    @Test
+    @Test   // see SSHD-893
     public void testScpNativeOnDirWithPattern() throws Exception {
         try (ClientSession session = client.connect(getCurrentTestName(), TEST_LOCALHOST, port)
                     .verify(CONNECT_TIMEOUT, TimeUnit.SECONDS)
@@ -590,6 +590,56 @@ public class ScpTest extends BaseTestSupport {
     }
 
     @Test
+    public void testScpVirtualOnDirWithPattern() throws Exception {
+        Path remoteDir = getTempTargetRelativeFile(getClass().getSimpleName(), getCurrentTestName(), ScpHelper.SCP_COMMAND_PREFIX, "virtual");
+        CommonTestSupportUtils.deleteRecursive(remoteDir);  // start fresh
+        Files.createDirectories(remoteDir);
+
+        try (ClientSession session = client.connect(getCurrentTestName(), TEST_LOCALHOST, port)
+                .verify(CONNECT_TIMEOUT, TimeUnit.SECONDS)
+                .getSession()) {
+            session.addPasswordIdentity(getCurrentTestName());
+            session.auth().verify(AUTH_TIMEOUT, TimeUnit.SECONDS);
+
+            sshd.setFileSystemFactory(new VirtualFileSystemFactory(remoteDir));
+
+            ScpClient scp = createScpClient(session);
+            Path targetPath = detectTargetFolder();
+            Path scpRoot = CommonTestSupportUtils.resolve(targetPath,
+                ScpHelper.SCP_COMMAND_PREFIX, getClass().getSimpleName(), getCurrentTestName());
+            CommonTestSupportUtils.deleteRecursive(scpRoot);
+
+            Path localDir = assertHierarchyTargetFolderExists(scpRoot.resolve("local"));
+            Path local1 = localDir.resolve("file-1.txt");
+            byte[] data = CommonTestSupportUtils.writeFile(local1, getClass().getName() + "#" + getCurrentTestName() + IoUtils.EOL);
+            Path local2 = localDir.resolve("file-2.txt");
+            Files.write(local2, data);
+
+            scp.upload(localDir.toString() + File.separator + "*", "/");
+
+            Path remote1 = remoteDir.resolve(local1.getFileName());
+            Path remote2 = remoteDir.resolve(local2.getFileName());
+
+            assertFileLength(remote1, data.length, TimeUnit.SECONDS.toMillis(11L));
+            assertFileLength(remote2, data.length, TimeUnit.SECONDS.toMillis(11L));
+
+            Files.delete(local1);
+            Files.delete(local2);
+
+            scp.download("/*", localDir);
+            assertFileLength(local1, data.length, TimeUnit.SECONDS.toMillis(11L));
+            assertFileLength(local2, data.length, TimeUnit.SECONDS.toMillis(11L));
+
+            Files.delete(remote1);
+            Files.delete(remote2);
+
+            CommonTestSupportUtils.deleteRecursive(remoteDir);
+        } finally {
+            sshd.setFileSystemFactory(fileSystemFactory);   // restore original
+        }
+    }
+
+    @Test
     public void testScpNativeOnMixedDirAndFiles() throws Exception {
         try (ClientSession session = client.connect(getCurrentTestName(), TEST_LOCALHOST, port)
                     .verify(CONNECT_TIMEOUT, TimeUnit.SECONDS)