Skip to content

Commit

Permalink
apacheGH-383: SFTP: tighten modes for file channels and streams
Browse files Browse the repository at this point in the history 
As pointed out in apache#283, FileSystemProvider.newFileChannel(...) uses
READ as default not only when no options are given, but always when
neither WRITE nor APPEND are present.

Also tighten some other rules, and fail if write modes are specified
in newInputStream(...).

Bug: apache#383
  • Loading branch information
@tomaswolf
tomaswolf committed Jun 3, 2023
1 parent 99a7923 commit b1d5cfe
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 10 deletions.
2 changes: 1 addition & 1 deletion CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

* [GH-370](https://github.com/apache/mina-sshd/issues/370) Also compare file keys in `ModifiableFileWatcher`.
* [GH-371](https://github.com/apache/mina-sshd/issues/371) Fix channel pool in `SftpFileSystem`.

* [GH-383](https://github.com/apache/mina-sshd/issues/383) Use correct default `OpenOption`s in `SftpFileSystemProvider.newFileChannel()`.

* [SSHD-1259](https://issues.apache.org/jira/browse/SSHD-1259) Consider all applicable host keys from the known_hosts files.
* [SSHD-1310](https://issues.apache.org/jira/browse/SSHD-1310) `SftpFileSystem`: do not close user session.
Expand Down
4 changes: 2 additions & 2 deletions sshd-sftp/src/main/java/org/apache/sshd/sftp/client/SftpClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,13 @@ enum OpenMode {
* Converts {@link StandardOpenOption}-s into {@link OpenMode}-s
*
* @param options The original options - ignored if {@code null}/empty
* @return A {@link Set} of the equivalent modes
* @return A modifiable {@link Set} of the equivalent modes
* @throws IllegalArgumentException If an unsupported option is requested
* @see #SUPPORTED_OPTIONS
*/
public static Set<OpenMode> fromOpenOptions(Collection<? extends OpenOption> options) {
if (GenericUtils.isEmpty(options)) {
return Collections.emptySet();
return EnumSet.noneOf(OpenMode.class);
}

Set<OpenMode> modes = EnumSet.noneOf(OpenMode.class);
Expand Down
43 changes: 36 additions & 7 deletions sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpFileSystemProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -577,9 +577,28 @@ public FileChannel newByteChannel(Path path, Set<? extends OpenOption> options,
@Override
public FileChannel newFileChannel(Path path, Set<? extends OpenOption> options, FileAttribute<?>... attrs)
throws IOException {
Collection<OpenMode> modes = OpenMode.fromOpenOptions(options);
if (modes.isEmpty()) {
modes = EnumSet.of(OpenMode.Read);
Set<OpenMode> modes = OpenMode.fromOpenOptions(options);
boolean readable = modes.contains(OpenMode.Read);
boolean writable = modes.contains(OpenMode.Write) || modes.contains(OpenMode.Append);
if (!readable && !writable) {
// As per {@link FileChannel#open(Path,Set,FileAttribute[])}: Read is default unless Write or Append are
// given. Other flags do imply write access, but Java does not supply write access by default for them.
modes.add(OpenMode.Read);
} else if (modes.contains(OpenMode.Append)) {
// As per {@link FileChannel#open(Path,Set,FileAttribute[])}: Append + Truncate or Append + Read are not
// allowed.
if (modes.contains(OpenMode.Truncate)) {
throw new IllegalArgumentException("APPEND + TRUNCATE_EXISTING not allowed");
} else if (modes.contains(OpenMode.Read)) {
throw new IllegalArgumentException("APPEND + READ not allowed");
}
}
if (!writable) {
// As per {@link FileChannel#open(Path,Set,FileAttribute[])}: Truncate, Create, and Create_New are ignored
// if opening read-only.
modes.remove(OpenMode.Truncate);
modes.remove(OpenMode.Create);
modes.remove(OpenMode.Exclusive);
}
// TODO: process file attributes
SftpPath p = toSftpPath(path);
Expand All @@ -588,10 +607,13 @@ public FileChannel newFileChannel(Path path, Set<? extends OpenOption> options,

@Override
public InputStream newInputStream(Path path, OpenOption... options) throws IOException {
Collection<OpenMode> modes = OpenMode.fromOpenOptions(Arrays.asList(options));
if (modes.isEmpty()) {
modes = EnumSet.of(OpenMode.Read);
Set<OpenMode> modes = OpenMode.fromOpenOptions(Arrays.asList(options));
if (modes.contains(OpenMode.Write) || modes.contains(OpenMode.Append)) {
throw new IllegalArgumentException("WRITE or APPEND not allowed");
}
// As per {@link FileChannel#open(Path,Set,FileAttribute[])}: Truncate, Create, and Create_New are ignored
// if opening read-only. Which leaves only Read.
modes = EnumSet.of(OpenMode.Read);
SftpPath p = toSftpPath(path);
SftpClient client = p.getFileSystem().getClient();
return new FilterInputStream(client.read(p.toString(), modes)) {
Expand All @@ -615,7 +637,14 @@ public OutputStream newOutputStream(Path path, OpenOption... options) throws IOE
if (modes.isEmpty()) {
modes = EnumSet.of(OpenMode.Create, OpenMode.Truncate, OpenMode.Write);
} else {
modes.add(OpenMode.Write);
// As per {@link FileChannel#open(Path,Set,FileAttribute[])}: Append + Truncate is not allowed.
if (modes.contains(OpenMode.Append)) {
if (modes.contains(OpenMode.Truncate)) {
throw new IllegalArgumentException("APPEND + TRUNCATE_EXISTING not allowed");
}
} else {
modes.add(OpenMode.Write);
}
}
SftpPath p = toSftpPath(path);
SftpClient client = p.getFileSystem().getClient();
Expand Down
50 changes: 50 additions & 0 deletions sshd-sftp/src/test/java/org/apache/sshd/sftp/client/fs/AbstractSftpFilesSystemSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
package org.apache.sshd.sftp.client.fs;

import java.io.IOException;
import java.io.OutputStream;
import java.net.URI;
import java.nio.ByteBuffer;
import java.nio.channels.FileChannel;
import java.nio.channels.FileLock;
import java.nio.channels.OverlappingFileLockException;
Expand All @@ -36,7 +38,9 @@
import java.nio.file.StandardOpenOption;
import java.nio.file.attribute.AclEntry;
import java.nio.file.attribute.AclFileAttributeView;
import java.util.Arrays;
import java.util.Collections;
import java.util.EnumSet;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -80,10 +84,40 @@ protected void testFileSystem(FileSystem fs, int version) throws Exception {
String buf = new String(Files.readAllBytes(file1), StandardCharsets.UTF_8);
assertEquals("Mismatched read test data", expected, buf);

try (OutputStream out = file1.getFileSystem().provider().newOutputStream(file1, StandardOpenOption.APPEND)) {
out.write("xyz".getBytes(StandardCharsets.US_ASCII));
}
expected += "xyz";
buf = new String(Files.readAllBytes(file1), StandardCharsets.UTF_8);
assertEquals("Mismatched read test data", expected, buf);

// Neither WRITE nor APPEND given: READ is default, and CREATE_NEW or TRUNCATE_EXISTING should be ignored.
assertEquals("Mismatched read test data", expected,
readFromChannel(file1, StandardOpenOption.CREATE_NEW, StandardOpenOption.TRUNCATE_EXISTING));

IllegalArgumentException ex = assertThrows(IllegalArgumentException.class,
() -> readFromChannel(file1, StandardOpenOption.APPEND, StandardOpenOption.TRUNCATE_EXISTING));
assertTrue("unexpected exception message " + ex.getMessage(), ex.getMessage().contains("APPEND"));
assertTrue("unexpected exception message " + ex.getMessage(), ex.getMessage().contains("TRUNCATE_EXISTING"));

ex = assertThrows(IllegalArgumentException.class,
() -> readFromChannel(file1, StandardOpenOption.APPEND, StandardOpenOption.READ));
assertTrue("unexpected exception message " + ex.getMessage(), ex.getMessage().contains("APPEND"));
assertTrue("unexpected exception message " + ex.getMessage(), ex.getMessage().contains("READ"));

assertEquals("Mismatched read test data", expected,
readFromChannel(file1, StandardOpenOption.READ, StandardOpenOption.WRITE));

if (version >= SftpConstants.SFTP_V4) {
testAclFileAttributeView(file1);
}

assertEquals("Mismatched read test data", "", readFromChannel(file1, StandardOpenOption.READ, StandardOpenOption.WRITE,
StandardOpenOption.TRUNCATE_EXISTING));

// Restore file contents, other tests need it.
Files.write(file1, expected.getBytes(StandardCharsets.UTF_8));

String remFile2Path = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, clientFolder.resolve("file-2.txt"));
Path file2 = fs.getPath(remFile2Path);
String remFile3Path = CommonTestSupportUtils.resolveRelativeRemotePath(parentPath, clientFolder.resolve("file-3.txt"));
Expand Down Expand Up @@ -131,6 +165,22 @@ protected void testFileSystem(FileSystem fs, int version) throws Exception {
Files.delete(file1);
}

protected static String readFromChannel(Path path, StandardOpenOption... options) throws IOException {
try (FileChannel ch = path.getFileSystem().provider().newFileChannel(path, EnumSet.copyOf(Arrays.asList(options)))) {
byte[] data = new byte[500];
ByteBuffer buffer = ByteBuffer.wrap(data);
int length = 0;
for (;;) {
int n = ch.read(buffer);
if (n < 0) {
break;
}
length += n;
}
return new String(data, 0, length, StandardCharsets.UTF_8);
}
}

protected static Iterable<Path> testRootDirs(FileSystem fs) throws IOException {
Iterable<Path> rootDirs = fs.getRootDirectories();
for (Path root : rootDirs) {
Expand Down

0 comments on commit b1d5cfe

Please sign in to comment.