allow to refuse an upload from a sync upload hook

Fixes #880

Signed-off-by: Nicola Murino <nicola.murino@gmail.com>

common/actions.go

@@ -27,7 +27,7 @@ import (
 var (
 	errUnconfiguredAction    = errors.New("no hook is configured for this action")
 	errNoHook                = errors.New("unable to execute action, no hook defined")
-	errUnexpectedHTTResponse = errors.New("unexpected HTTP response code")
+	errUnexpectedHTTResponse = errors.New("unexpected HTTP hook response code")
 )
 
 // ProtocolActions defines the action to execute on file operations and SSH commands
@@ -84,11 +84,11 @@ func ExecutePreAction(conn *BaseConnection, operation, filePath, virtualPath str
 // ExecuteActionNotification executes the defined hook, if any, for the specified action
 func ExecuteActionNotification(conn *BaseConnection, operation, filePath, virtualPath, target, virtualTarget, sshCmd string,
 	fileSize int64, err error,
-) {
+) error {
 	hasNotifiersPlugin := plugin.Handler.HasNotifiers()
 	hasHook := util.Contains(Config.Actions.ExecuteOn, operation)
 	if !hasHook && !hasNotifiersPlugin {
-		return
+		return nil
 	}
 	notification := newActionNotification(&conn.User, operation, filePath, virtualPath, target, virtualTarget, sshCmd,
 		conn.protocol, conn.GetRemoteIP(), conn.ID, fileSize, 0, err)
@@ -98,12 +98,12 @@ func ExecuteActionNotification(conn *BaseConnection, operation, filePath, virtua
 
 	if hasHook {
 		if util.Contains(Config.Actions.ExecuteSync, operation) {
-			actionHandler.Handle(notification) //nolint:errcheck
-			return
+			return actionHandler.Handle(notification)
 		}
 
 		go actionHandler.Handle(notification) //nolint:errcheck
 	}
+	return nil
 }
 
 // ActionHandler handles a notification for a Protocol Action.

common/actions_test.go

@@ -157,9 +157,11 @@ func TestActionCMD(t *testing.T) {
 	assert.NoError(t, err)
 
 	c := NewBaseConnection("id", ProtocolSFTP, "", "", *user)
-	ExecuteActionNotification(c, OperationSSHCmd, "path", "vpath", "target", "vtarget", "sha1sum", 0, nil)
+	err = ExecuteActionNotification(c, OperationSSHCmd, "path", "vpath", "target", "vtarget", "sha1sum", 0, nil)
+	assert.NoError(t, err)
 
-	ExecuteActionNotification(c, operationDownload, "path", "vpath", "", "", "", 0, nil)
+	err = ExecuteActionNotification(c, operationDownload, "path", "vpath", "", "", "", 0, nil)
+	assert.NoError(t, err)
 
 	Config.Actions = actionsCopy
 }
@@ -272,7 +274,8 @@ func TestUnconfiguredHook(t *testing.T) {
 	err = ExecutePreAction(c, operationPreDelete, "", "", 0, 0)
 	assert.ErrorIs(t, err, errUnconfiguredAction)
 
-	ExecuteActionNotification(c, operationDownload, "", "", "", "", "", 0, nil)
+	err = ExecuteActionNotification(c, operationDownload, "", "", "", "", "", 0, nil)
+	assert.NoError(t, err)
 
 	err = plugin.Initialize(nil, true)
 	assert.NoError(t, err)

common/connection.go

@@ -344,7 +344,7 @@ func (c *BaseConnection) CreateDir(virtualPath string, checkFilePatterns bool) e
 
 	logger.CommandLog(mkdirLogSender, fsPath, "", c.User.Username, "", c.ID, c.protocol, -1, -1, "", "", "", -1,
 		c.localAddr, c.remoteAddr)
-	ExecuteActionNotification(c, operationMkdir, fsPath, virtualPath, "", "", "", 0, nil)
+	ExecuteActionNotification(c, operationMkdir, fsPath, virtualPath, "", "", "", 0, nil) //nolint:errcheck
 	return nil
 }
 
@@ -391,7 +391,7 @@ func (c *BaseConnection) RemoveFile(fs vfs.Fs, fsPath, virtualPath string, info
 		}
 	}
 	if actionErr != nil {
-		ExecuteActionNotification(c, operationDelete, fsPath, virtualPath, "", "", "", size, nil)
+		ExecuteActionNotification(c, operationDelete, fsPath, virtualPath, "", "", "", size, nil) //nolint:errcheck
 	}
 	return nil
 }
@@ -455,7 +455,7 @@ func (c *BaseConnection) RemoveDir(virtualPath string) error {
 
 	logger.CommandLog(rmdirLogSender, fsPath, "", c.User.Username, "", c.ID, c.protocol, -1, -1, "", "", "", -1,
 		c.localAddr, c.remoteAddr)
-	ExecuteActionNotification(c, operationRmdir, fsPath, virtualPath, "", "", "", 0, nil)
+	ExecuteActionNotification(c, operationRmdir, fsPath, virtualPath, "", "", "", 0, nil) //nolint:errcheck
 	return nil
 }
 
@@ -520,8 +520,8 @@ func (c *BaseConnection) Rename(virtualSourcePath, virtualTargetPath string) err
 	c.updateQuotaAfterRename(fsDst, virtualSourcePath, virtualTargetPath, fsTargetPath, initialSize) //nolint:errcheck
 	logger.CommandLog(renameLogSender, fsSourcePath, fsTargetPath, c.User.Username, "", c.ID, c.protocol, -1, -1,
 		"", "", "", -1, c.localAddr, c.remoteAddr)
-	ExecuteActionNotification(c, operationRename, fsSourcePath, virtualSourcePath, fsTargetPath, virtualTargetPath,
-		"", 0, nil)
+	ExecuteActionNotification(c, operationRename, fsSourcePath, virtualSourcePath, fsTargetPath, //nolint:errcheck
+		virtualTargetPath, "", 0, nil)
 
 	return nil
 }

common/protocol_test.go

@@ -2808,10 +2808,13 @@ func TestSyncUploadAction(t *testing.T) {
 	common.Config.Actions.ExecuteSync = []string{"upload"}
 	common.Config.Actions.Hook = uploadScriptPath
 
-	user, _, err := httpdtest.AddUser(getTestUser(), http.StatusCreated)
+	u := getTestUser()
+	u.QuotaFiles = 1000
+	user, _, err := httpdtest.AddUser(u, http.StatusCreated)
 	assert.NoError(t, err)
-	movedPath := filepath.Join(user.HomeDir, "moved.dat")
-	err = os.WriteFile(uploadScriptPath, getUploadScriptContent(movedPath), 0755)
+	movedFileName := "moved.dat"
+	movedPath := filepath.Join(user.HomeDir, movedFileName)
+	err = os.WriteFile(uploadScriptPath, getUploadScriptContent(movedPath, 0), 0755)
 	assert.NoError(t, err)
 	conn, client, err := getSftpClient(user)
 	if assert.NoError(t, err) {
@@ -2823,10 +2826,47 @@ func TestSyncUploadAction(t *testing.T) {
 		assert.NoError(t, err)
 		_, err = client.Stat(testFileName)
 		assert.Error(t, err)
-		info, err := client.Stat(filepath.Base(movedPath))
+		info, err := client.Stat(movedFileName)
 		if assert.NoError(t, err) {
 			assert.Equal(t, size, info.Size())
 		}
+		user, _, err = httpdtest.GetUserByUsername(user.Username, http.StatusOK)
+		assert.NoError(t, err)
+		assert.Equal(t, 1, user.UsedQuotaFiles)
+		assert.Equal(t, size, user.UsedQuotaSize)
+		// test some hook failure
+		// the uploaded file is moved and the hook fails, it will be not removed from the quota
+		err = os.WriteFile(uploadScriptPath, getUploadScriptContent(movedPath, 1), 0755)
+		assert.NoError(t, err)
+		err = writeSFTPFileNoCheck(testFileName+"_1", size, client)
+		assert.Error(t, err)
+		user, _, err = httpdtest.GetUserByUsername(user.Username, http.StatusOK)
+		assert.NoError(t, err)
+		assert.Equal(t, 2, user.UsedQuotaFiles)
+		assert.Equal(t, size*2, user.UsedQuotaSize)
+
+		// the uploaded file is not moved and the hook fails, the uploaded file will be deleted
+		// and removed from the quota
+		movedPath = filepath.Join(user.HomeDir, "missing dir", movedFileName)
+		err = os.WriteFile(uploadScriptPath, getUploadScriptContent(movedPath, 1), 0755)
+		assert.NoError(t, err)
+		err = writeSFTPFileNoCheck(testFileName+"_2", size, client)
+		assert.Error(t, err)
+		user, _, err = httpdtest.GetUserByUsername(user.Username, http.StatusOK)
+		assert.NoError(t, err)
+		assert.Equal(t, 2, user.UsedQuotaFiles)
+		assert.Equal(t, size*2, user.UsedQuotaSize)
+		// overwrite an existing file
+		_, err = client.Stat(movedFileName)
+		assert.NoError(t, err)
+		err = writeSFTPFileNoCheck(movedFileName, size, client)
+		assert.Error(t, err)
+		_, err = client.Stat(movedFileName)
+		assert.Error(t, err)
+		user, _, err = httpdtest.GetUserByUsername(user.Username, http.StatusOK)
+		assert.NoError(t, err)
+		assert.Equal(t, 1, user.UsedQuotaFiles)
+		assert.Equal(t, size, user.UsedQuotaSize)
 	}
 
 	err = os.Remove(uploadScriptPath)
@@ -3774,10 +3814,11 @@ func writeSFTPFileNoCheck(name string, size int64, client *sftp.Client) error {
 	return f.Close()
 }
 
-func getUploadScriptContent(movedPath string) []byte {
+func getUploadScriptContent(movedPath string, exitStatus int) []byte {
 	content := []byte("#!/bin/sh\n\n")
 	content = append(content, []byte("sleep 1\n")...)
 	content = append(content, []byte(fmt.Sprintf("mv ${SFTPGO_ACTION_PATH} %v\n", movedPath))...)
+	content = append(content, []byte(fmt.Sprintf("exit %d", exitStatus))...)
 	return content
 }
 

common/transfer.go

@@ -375,7 +375,7 @@ func (t *BaseTransfer) Close() error {
 	if t.transferType == TransferDownload {
 		logger.TransferLog(downloadLogSender, t.fsPath, elapsed, atomic.LoadInt64(&t.BytesSent), t.Connection.User.Username,
 			t.Connection.ID, t.Connection.protocol, t.Connection.localAddr, t.Connection.remoteAddr, t.ftpMode)
-		ExecuteActionNotification(t.Connection, operationDownload, t.fsPath, t.requestPath, "", "", "",
+		ExecuteActionNotification(t.Connection, operationDownload, t.fsPath, t.requestPath, "", "", "", //nolint:errcheck
 			atomic.LoadInt64(&t.BytesSent), t.ErrTransfer)
 	} else {
 		fileSize := atomic.LoadInt64(&t.BytesReceived) + t.MinWriteOffset
@@ -383,11 +383,11 @@ func (t *BaseTransfer) Close() error {
 			fileSize = statSize
 		}
 		t.Connection.Log(logger.LevelDebug, "uploaded file size %v", fileSize)
+		numFiles, fileSize = t.executeUploadHook(numFiles, fileSize)
 		t.updateQuota(numFiles, fileSize)
 		t.updateTimes()
 		logger.TransferLog(uploadLogSender, t.fsPath, elapsed, atomic.LoadInt64(&t.BytesReceived), t.Connection.User.Username,
 			t.Connection.ID, t.Connection.protocol, t.Connection.localAddr, t.Connection.remoteAddr, t.ftpMode)
-		ExecuteActionNotification(t.Connection, operationUpload, t.fsPath, t.requestPath, "", "", "", fileSize, t.ErrTransfer)
 	}
 	if t.ErrTransfer != nil {
 		t.Connection.Log(logger.LevelError, "transfer error: %v, path: %#v", t.ErrTransfer, t.fsPath)
@@ -398,6 +398,27 @@ func (t *BaseTransfer) Close() error {
 	return err
 }
 
+func (t *BaseTransfer) executeUploadHook(numFiles int, fileSize int64) (int, int64) {
+	err := ExecuteActionNotification(t.Connection, operationUpload, t.fsPath, t.requestPath, "", "", "",
+		fileSize, t.ErrTransfer)
+	if err != nil {
+		if t.ErrTransfer == nil {
+			t.ErrTransfer = err
+		}
+		// try to remove the uploaded file
+		err = t.Fs.Remove(t.fsPath, false)
+		if err == nil {
+			numFiles--
+			fileSize = 0
+			atomic.StoreInt64(&t.BytesReceived, 0)
+			t.MinWriteOffset = 0
+		} else {
+			t.Connection.Log(logger.LevelWarn, "unable to remove path %q after upload hook failure: %v", t.fsPath, err)
+		}
+	}
+	return numFiles, fileSize
+}
+
 func (t *BaseTransfer) updateTimes() {
 	if !t.aTime.IsZero() && !t.mTime.IsZero() {
 		err := t.Fs.Chtimes(t.fsPath, t.aTime, t.mTime, true)

docs/custom-actions.md

@@ -75,6 +75,8 @@ If the `hook` defines an HTTP URL then this URL will be invoked as HTTP POST. Th
 The HTTP hook will use the global configuration for HTTP clients and will respect the retry configurations.
 
 The `pre-*` actions are always executed synchronously while the other ones are asynchronous. You can specify the actions to run synchronously via the `execute_sync` configuration key. Executing an action synchronously means that SFTPGo will not return a result code to the client (which is waiting for it) until your hook have completed its execution. If your hook takes a long time to complete this could cause a timeout on the client side, which wouldn't receive the server response in a timely manner and eventually drop the connection.
+If you add the `upload` action to the `execute_sync` configuration key, SFTPGo will try to delete the uploaded file and return an error to the client if the hook fails. A hook is considered failed if the external command completes with a non-zero exit status or the HTTP notification response code is other than `200` (or the HTTP endpoint cannot be reached or times out).
+After a hook failure, the uploaded size is removed from the quota if SFTPGo is able to remove the file.
 
 ## Provider events
 

go.mod

@@ -68,7 +68,7 @@ require (
 	golang.org/x/crypto v0.0.0-20220525230936-793ad666bf5e
 	golang.org/x/net v0.0.0-20220607020251-c690dde0001d
 	golang.org/x/oauth2 v0.0.0-20220608161450-d0670ef3b1eb
-	golang.org/x/sys v0.0.0-20220614162138-6c1b26c55098
+	golang.org/x/sys v0.0.0-20220615213510-4f61da869c0c
 	golang.org/x/time v0.0.0-20220609170525-579cf78fd858
 	google.golang.org/api v0.84.0
 	gopkg.in/natefinch/lumberjack.v2 v2.0.0
@@ -155,7 +155,7 @@ require (
 	golang.org/x/tools v0.1.11 // indirect
 	golang.org/x/xerrors v0.0.0-20220609144429-65e65417b02f // indirect
 	google.golang.org/appengine v1.6.7 // indirect
-	google.golang.org/genproto v0.0.0-20220615141314-f1464d18c36b // indirect
+	google.golang.org/genproto v0.0.0-20220616135557-88e70c0c3a90 // indirect
 	google.golang.org/grpc v1.47.0 // indirect
 	google.golang.org/protobuf v1.28.0 // indirect
 	gopkg.in/ini.v1 v1.66.6 // indirect

go.sum

@@ -960,8 +960,8 @@ golang.org/x/sys v0.0.0-20220503163025-988cb79eb6c6/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220513210249-45d2b4557a2a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220610221304-9f5ed59c137d/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20220614162138-6c1b26c55098 h1:PgOr27OhUx2IRqGJ2RxAWI4dJQ7bi9cSrB82uzFzfUA=
-golang.org/x/sys v0.0.0-20220614162138-6c1b26c55098/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.0.0-20220615213510-4f61da869c0c h1:aFV+BgZ4svzjfabn8ERpuB4JI4N6/rdy1iusx77G3oU=
+golang.org/x/sys v0.0.0-20220615213510-4f61da869c0c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 h1:JGgROgKl9N8DuW20oFS5gxc+lE67/N3FcwmBPMe7ArY=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
@@ -1205,8 +1205,8 @@ google.golang.org/genproto v0.0.0-20220505152158-f39f71e6c8f3/go.mod h1:RAyBrSAP
 google.golang.org/genproto v0.0.0-20220518221133-4f43b3371335/go.mod h1:RAyBrSAP7Fh3Nc84ghnVLDPuV51xc9agzmm4Ph6i0Q4=
 google.golang.org/genproto v0.0.0-20220523171625-347a074981d8/go.mod h1:RAyBrSAP7Fh3Nc84ghnVLDPuV51xc9agzmm4Ph6i0Q4=
 google.golang.org/genproto v0.0.0-20220608133413-ed9918b62aac/go.mod h1:KEWEmljWE5zPzLBa/oHl6DaEt9LmfH6WtH1OHIvleBA=
-google.golang.org/genproto v0.0.0-20220615141314-f1464d18c36b h1:2LXbOcxY7BehyA9yu5hxYzaY67bLaJQhBX9O1zxxVis=
-google.golang.org/genproto v0.0.0-20220615141314-f1464d18c36b/go.mod h1:KEWEmljWE5zPzLBa/oHl6DaEt9LmfH6WtH1OHIvleBA=
+google.golang.org/genproto v0.0.0-20220616135557-88e70c0c3a90 h1:4SPz2GL2CXJt28MTF8V6Ap/9ZiVbQlJeGSd9qtA7DLs=
+google.golang.org/genproto v0.0.0-20220616135557-88e70c0c3a90/go.mod h1:KEWEmljWE5zPzLBa/oHl6DaEt9LmfH6WtH1OHIvleBA=
 google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c=
 google.golang.org/grpc v1.20.1/go.mod h1:10oTOabMzJvdu6/UiuZezV6QK5dSlG84ov/aaiqXj38=
 google.golang.org/grpc v1.21.1/go.mod h1:oYelfM1adQP15Ek0mdvEgi9Df8B9CZIaU1084ijfRaM=

sftpd/ssh_cmd.go

@@ -767,8 +767,8 @@ func (c *sshCommand) sendExitStatus(err error) {
 				targetPath = p
 			}
 		}
-		common.ExecuteActionNotification(c.connection.BaseConnection, common.OperationSSHCmd, cmdPath, vCmdPath, targetPath,
-			vTargetPath, c.command, 0, err)
+		common.ExecuteActionNotification(c.connection.BaseConnection, common.OperationSSHCmd, cmdPath, vCmdPath, //nolint:errcheck
+			targetPath, vTargetPath, c.command, 0, err)
 		if err == nil {
 			logger.CommandLog(sshCommandLogSender, cmdPath, targetPath, c.connection.User.Username, "", c.connection.ID,
 				common.ProtocolSSH, -1, -1, "", "", c.connection.command, -1, c.connection.GetLocalAddress(),