File transfer fixes and improvements (#202)

* Added a test around changes brought by #161 (fixes #162) * Updated the overall file transfer logic * Ensure to close the file in every case * Add test case for failed seek * Ignore close error for reads * Add tests for partial RETR/STOR and transfer open error Co-authored-by: Nicola Murino <[email protected]>
fclairamb · Dec 17, 2020 · fdb9611 · fdb9611
1 parent c84759b
commit fdb9611
Show file tree

Hide file tree

Showing 9 changed files with 272 additions and 130 deletions.
diff --git a/client_handler.go b/client_handler.go
@@ -3,6 +3,7 @@ package ftpserver
 
 import (
 	"bufio"
+	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -24,6 +25,11 @@ const (
 	HASHAlgoSHA512
 )
 
+var (
+	errNoTrasferConnection = errors.New("unable to open transfer: no transfer connection")
+	errTLSRequired         = errors.New("unable to open transfer: TLS is required")
+)
+
 func getHashMapping() map[string]HASHAlgo {
 	mapping := make(map[string]HASHAlgo)
 	mapping["CRC32"] = HASHAlgoCRC32
@@ -35,14 +41,6 @@ func getHashMapping() map[string]HASHAlgo {
 	return mapping
 }
 
-type openTransferError struct {
-	err string
-}
-
-func (e *openTransferError) Error() string {
-	return fmt.Sprintf("Unable to open transfer: %s", e.err)
-}
-
 // nolint: maligned
 type clientHandler struct {
 	id               uint32          // ID of the client
@@ -341,22 +339,17 @@ func (c *clientHandler) writeMessage(code int, message string) {
 	}
 }
 
-// ErrNoPassiveConnectionDeclared is defined when a transfer is openeed without any passive connection declared
-// var ErrNoPassiveConnectionDeclared = errors.New("no passive connection declared")
-
 func (c *clientHandler) TransferOpen() (net.Conn, error) {
 	if c.transfer == nil {
-		err := &openTransferError{err: "No passive connection declared"}
-		c.writeMessage(StatusActionNotTaken, err.Error())
+		c.writeMessage(StatusActionNotTaken, errNoTrasferConnection.Error())
 
-		return nil, err
+		return nil, errNoTrasferConnection
 	}
 
 	if c.server.settings.TLSRequired == MandatoryEncryption && !c.transferTLS {
-		err := &openTransferError{err: "TLS is required"}
-		c.writeMessage(StatusServiceNotAvailable, err.Error())
+		c.writeMessage(StatusServiceNotAvailable, errTLSRequired.Error())
 
-		return nil, err
+		return nil, errTLSRequired
 	}
 
 	conn, err := c.transfer.Open()
@@ -365,10 +358,9 @@ func (c *clientHandler) TransferOpen() (net.Conn, error) {
 			"Unable to open transfer",
 			"error", err)
 
-		err = &openTransferError{err: err.Error()}
 		c.writeMessage(StatusCannotOpenDataConnection, err.Error())
 
-		return conn, err
+		return nil, err
 	}
 
 	c.writeMessage(StatusFileStatusOK, "Using transfer connection")
@@ -384,13 +376,10 @@ func (c *clientHandler) TransferOpen() (net.Conn, error) {
 }
 
 func (c *clientHandler) TransferClose(err error) {
-	if c.transfer != nil {
-		if err == nil {
-			// only send the OK status if there is no error
-			c.writeMessage(StatusClosingDataConn, "Closing transfer connection")
-		}
+	var errClose error
 
-		if err := c.transfer.Close(); err != nil {
+	if c.transfer != nil {
+		if errClose = c.transfer.Close(); errClose != nil {
 			c.logger.Warn(
 				"Problem closing transfer connection",
 				"err", err,
@@ -403,6 +392,15 @@ func (c *clientHandler) TransferClose(err error) {
 			c.logger.Debug("Transfer connection closed")
 		}
 	}
+
+	switch {
+	case err == nil && errClose == nil:
+		c.writeMessage(StatusClosingDataConn, "Closing transfer connection")
+	case errClose != nil:
+		c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Issue during transfer close: %v", errClose))
+	case err != nil:
+		c.writeMessage(StatusActionNotTaken, fmt.Sprintf("Issue during transfer: %v", err))
+	}
 }
 
 func parseLine(line string) (string, string) {

diff --git a/client_handler_test.go b/client_handler_test.go
@@ -50,6 +50,28 @@ func TestLastCommand(t *testing.T) {
 	assert.Empty(t, cc.GetLastCommand())
 }
 
+func TestTransferOpenError(t *testing.T) {
+	s := NewTestServer(t, true)
+	conf := goftp.Config{
+		User:     authUser,
+		Password: authPass,
+	}
+
+	c, err := goftp.DialConfig(conf, s.Addr())
+	require.NoError(t, err, "Couldn't connect")
+
+	defer func() { panicOnError(c.Close()) }()
+
+	raw, err := c.OpenRawConn()
+	require.NoError(t, err, "Couldn't open raw connection")
+
+	// we send STOR without opening a transfer connection
+	rc, response, err := raw.SendCommand("STOR file")
+	require.NoError(t, err)
+	require.Equal(t, StatusActionNotTaken, rc)
+	require.Equal(t, "unable to open transfer: no transfer connection", response)
+}
+
 func TestTLSMethods(t *testing.T) {
 	t.Run("without-tls", func(t *testing.T) {
 		cc := clientHandler{

diff --git a/driver_test.go b/driver_test.go
@@ -6,6 +6,7 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
+	"strings"
 	"testing"
 
 	gklog "github.com/go-kit/kit/log"
@@ -78,17 +79,49 @@ type TestServerDriver struct {
 	Debug bool // To display connection logs information
 	TLS   bool
 
-	Settings     *Settings // Settings
-	FileOverride afero.File
-	fs           afero.Fs
+	Settings *Settings // Settings
+	fs       afero.Fs
 }
 
 // TestClientDriver defines a minimal serverftp client driver
 type TestClientDriver struct {
-	FileOverride afero.File
 	afero.Fs
 }
 
+type testFile struct {
+	afero.File
+}
+
+var errFailClose = errors.New("couldn't close")
+
+var errFailWrite = errors.New("couldn't write")
+
+var errFailSeek = errors.New("couldn't seek")
+
+func (f *testFile) Write(b []byte) (int, error) {
+	if strings.Contains(f.File.Name(), "fail-to-write") {
+		return 0, errFailWrite
+	}
+
+	return f.File.Write(b)
+}
+
+func (f *testFile) Close() error {
+	if strings.Contains(f.File.Name(), "fail-to-close") {
+		return errFailClose
+	}
+
+	return f.File.Close()
+}
+
+func (f *testFile) Seek(offset int64, whence int) (int64, error) {
+	if strings.Contains(f.File.Name(), "fail-to-seek") {
+		return 0, errFailSeek
+	}
+
+	return f.File.Seek(offset, whence)
+}
+
 // NewTestClientDriver creates a client driver
 func NewTestClientDriver(server *TestServerDriver) *TestClientDriver {
 	return &TestClientDriver{
@@ -117,10 +150,6 @@ func (driver *TestServerDriver) AuthUser(_ ClientContext, user, pass string) (Cl
 	if user == authUser && pass == authPass {
 		clientdriver := NewTestClientDriver(driver)
 
-		if driver.FileOverride != nil {
-			clientdriver.FileOverride = driver.FileOverride
-		}
-
 		return clientdriver, nil
 	}
 
@@ -156,11 +185,13 @@ func (driver *TestServerDriver) GetTLSConfig() (*tls.Config, error) {
 
 // OpenFile opens a file in 3 possible modes: read, write, appending write (use appropriate flags)
 func (driver *TestClientDriver) OpenFile(path string, flag int, perm os.FileMode) (afero.File, error) {
-	if driver.FileOverride != nil {
-		return driver.FileOverride, nil
+	file, err := driver.Fs.OpenFile(path, flag, perm)
+
+	if err == nil {
+		file = &testFile{File: file}
 	}
 
-	return driver.Fs.OpenFile(path, flag, perm)
+	return file, err
 }
 
 var errTooMuchSpaceRequested = errors.New("you're requesting too much space")

diff --git a/go.mod b/go.mod
@@ -10,4 +10,4 @@ require (
 	golang.org/x/text v0.3.4 // indirect
 )
 
-replace github.com/secsy/goftp => github.com/drakkan/goftp v0.0.0-20200916091733-843d4cca4bb2
+replace github.com/secsy/goftp => github.com/drakkan/goftp v0.0.0-20201217084041-93793d4ac54d
diff --git a/go.sum b/go.sum
@@ -40,8 +40,8 @@ github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSs
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ=
-github.com/drakkan/goftp v0.0.0-20200916091733-843d4cca4bb2 h1:aQCoXDfkaeU2/KqIvTZAcVnVmaiGdfrGF9PUnjNVjac=
-github.com/drakkan/goftp v0.0.0-20200916091733-843d4cca4bb2/go.mod h1:K3yqfa64LwJzUpdUWC6b524HO7U7DmBnrJuBjxKSZOQ=
+github.com/drakkan/goftp v0.0.0-20201217084041-93793d4ac54d h1:hIhohhJRpUzEuZj2qExBNzEUAxWk8+ahitt7BEFP1KU=
+github.com/drakkan/goftp v0.0.0-20201217084041-93793d4ac54d/go.mod h1:K3yqfa64LwJzUpdUWC6b524HO7U7DmBnrJuBjxKSZOQ=
 github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
 github.com/eapache/go-resiliency v1.1.0/go.mod h1:kFI+JgMyC7bLPUVY133qvEBtVayf5mFgVsvEsIPBvNs=
 github.com/eapache/go-xerial-snappy v0.0.0-20180814174437-776d5712da21/go.mod h1:+020luEh2TKB4/GOp8oxxtq0Daoen/Cii55CzbTV6DU=

diff --git a/handle_dirs_test.go b/handle_dirs_test.go
@@ -235,7 +235,7 @@ func TestTLSTransfer(t *testing.T) {
 	rc, response, err = raw.SendCommand("MLSD /")
 	require.NoError(t, err)
 	require.Equal(t, StatusServiceNotAvailable, rc)
-	require.Equal(t, "Unable to open transfer: TLS is required", response)
+	require.Equal(t, "unable to open transfer: TLS is required", response)
 }
 
 func TestDirListingBeforeLogin(t *testing.T) {