This is an automated email from the ASF dual-hosted git repository. lgoldstein pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 31442c62f90bb63c075d4279997c45aab21f1fa2 Author: The-Yoda <[email protected]> AuthorDate: Mon Feb 11 15:11:20 2019 +0200 [SSHD-893] Fix SCP download with pattern issue in rooted filesystem --- .../sshd/common/util/io/DirectoryScanner.java | 214 +++++++++++---------- .../sshd/common/util/io/DirectoryScannerTest.java | 99 ++++++++++ .../org/apache/sshd/common/scp/ScpFileOpener.java | 14 +- .../java/org/apache/sshd/common/scp/ScpHelper.java | 6 +- .../java/org/apache/sshd/client/scp/ScpTest.java | 62 +++++- 5 files changed, 278 insertions(+), 117 deletions(-) diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/io/DirectoryScanner.java b/sshd-common/src/main/java/org/apache/sshd/common/util/io/DirectoryScanner.java index 5ba7f75..c75c889 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/util/io/DirectoryScanner.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/util/io/DirectoryScanner.java @@ -19,10 +19,21 @@ 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:[email protected]";>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 index 0000000..86f488b --- /dev/null +++ b/sshd-common/src/test/java/org/apache/sshd/common/util/io/DirectoryScannerTest.java @@ -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:[email protected]";>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); + } +} diff --git a/sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpFileOpener.java b/sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpFileOpener.java index eab3e11..1d8f5cd 100644 --- a/sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpFileOpener.java +++ b/sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpFileOpener.java @@ -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(); } /** diff --git a/sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpHelper.java b/sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpHelper.java index e51d2f4..fed18d5 100644 --- a/sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpHelper.java +++ b/sshd-scp/src/main/java/org/apache/sshd/common/scp/ScpHelper.java @@ -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)) { diff --git a/sshd-scp/src/test/java/org/apache/sshd/client/scp/ScpTest.java b/sshd-scp/src/test/java/org/apache/sshd/client/scp/ScpTest.java index 2611fff..7f5955d 100644 --- a/sshd-scp/src/test/java/org/apache/sshd/client/scp/ScpTest.java +++ b/sshd-scp/src/test/java/org/apache/sshd/client/scp/ScpTest.java @@ -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)
