Merge pull request #32 from fclairamb/error-on-file-access

Checking if we can write the file *before* opening the transfer connection

server/client_handler.go

@@ -172,7 +172,7 @@ func (c *clientHandler) writeMessage(code int, message string) {
 func (c *clientHandler) TransferOpen() (net.Conn, error) {
 	if c.transfer == nil {
 		c.writeMessage(550, "No passive connection declared")
-		return nil, errors.New("No passive connection declared")
+		return nil, errors.New("no passive connection declared")
 	}
 	c.writeMessage(150, "Using transfer connection")
 	conn, err := c.transfer.Open()

server/handle_files.go

@@ -19,17 +19,30 @@ func (c *clientHandler) handleAPPE() {
 
 // Handles both the "STOR" and "APPE" commands
 func (c *clientHandler) handleStoreAndAppend(append bool) {
+	file, err := c.openFile(c.absPath(c.param), append)
 
-	path := c.absPath(c.param)
+	if err != nil {
+		c.writeMessage(550, "Could not open file: "+err.Error())
+		return
+	}
 
 	if tr, err := c.TransferOpen(); err == nil {
 		defer c.TransferClose()
-		if _, err := c.storeOrAppend(tr, path, append); err != nil && err != io.EOF {
+		if _, err := c.storeOrAppend(tr, file); err != nil && err != io.EOF {
 			c.writeMessage(550, err.Error())
 		}
 	} else {
-		c.writeMessage(550, err.Error())
+		c.writeMessage(550, "Could not open transfer: "+err.Error())
+	}
+}
+
+func (c *clientHandler) openFile(path string, append bool) (FileStream, error) {
+	flag := os.O_WRONLY
+	if append {
+		flag |= os.O_APPEND
 	}
+
+	return c.driver.OpenFile(c, path, flag)
 }
 
 func (c *clientHandler) handleRETR() {
@@ -81,18 +94,7 @@ func (c *clientHandler) handleCHMOD(params string) {
 	c.writeMessage(200, "SITE CHMOD command successful")
 }
 
-func (c *clientHandler) storeOrAppend(conn net.Conn, name string, append bool) (int64, error) {
-	flag := os.O_WRONLY
-	if append {
-		flag |= os.O_APPEND
-	}
-
-	file, err := c.driver.OpenFile(c, name, flag)
-
-	if err != nil {
-		return 0, err
-	}
-
+func (c *clientHandler) storeOrAppend(conn net.Conn, file FileStream) (int64, error) {
 	if c.ctxRest != 0 {
 		file.Seek(c.ctxRest, 0)
 		c.ctxRest = 0

tests/misc_commands_test.go

@@ -1,8 +1,9 @@
 package tests
 
 import (
-	"github.com/secsy/goftp"
 	"testing"
+
+	"github.com/secsy/goftp"
 )
 
 func TestSiteCommand(t *testing.T) {

tests/transfer_test.go

@@ -96,6 +96,7 @@ func ftpDelete(t *testing.T, ftp *goftp.Client, filename string) {
 	}
 }
 
+// TestTransfer validates the upload of file in both active and passive mode
 func TestTransfer(t *testing.T) {
 	s := NewTestServer(true)
 	s.Settings.NonStandardActiveDataPort = true
@@ -137,3 +138,32 @@ func testTransferOnConnection(t *testing.T, server *server.FtpServer, active boo
 		t.Fatal("The two files don't have the same hash:", hashUpload, "!=", hashDownload)
 	}
 }
+
+// TestFailedTransfer validates the handling of failed transfer caused by file access issues
+func TestFailedTransfer(t *testing.T) {
+	s := NewTestServer(true)
+	defer s.Stop()
+
+	conf := goftp.Config{
+		User:     "test",
+		Password: "test",
+	}
+
+	var err error
+	var c *goftp.Client
+
+	if c, err = goftp.DialConfig(conf, s.Listener.Addr().String()); err != nil {
+		t.Fatal("Couldn't connect", err)
+	}
+	defer c.Close()
+
+	// We create a 1KB file and upload it
+	file := createTemporaryFile(t, 1*1024)
+	if err = c.Store("/non/existing/path/file.bin", file); err == nil {
+		t.Fatal("This upload should have failed")
+	}
+
+	if err = c.Store("file.bin", file); err != nil {
+		t.Fatal("This upload should have succeeded", err)
+	}
+}