allow to execute actions for file operations and SSH commands synchronously

The actions to run synchronously can be configured via the `execute_sync`
configuration key.

Executing an action synchronously means that SFTPGo will not return a result
code to the client until your hook have completed its execution.

Fixes #409

common/actions.go

@@ -31,6 +31,11 @@ var (
 type ProtocolActions struct {
 	// Valid values are download, upload, pre-delete, delete, rename, ssh_cmd. Empty slice to disable
 	ExecuteOn []string `json:"execute_on" mapstructure:"execute_on"`
+	// Actions to be performed synchronously.
+	// The pre-delete action is always executed synchronously while the other ones are asynchronous.
+	// 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.
+	ExecuteSync []string `json:"execute_sync" mapstructure:"execute_sync"`
 	// Absolute path to an external program or an HTTP URL
 	Hook string `json:"hook" mapstructure:"hook"`
 }
@@ -44,11 +49,16 @@ func InitializeActionHandler(handler ActionHandler) {
 	actionHandler = handler
 }
 
-// SSHCommandActionNotification executes the defined action for the specified SSH command.
-func SSHCommandActionNotification(user *dataprovider.User, filePath, target, sshCmd string, err error) {
-	notification := newActionNotification(user, operationSSHCmd, filePath, target, sshCmd, ProtocolSSH, 0, err)
+// ExecuteActionNotification executes the defined hook, if any, for the specified action
+func ExecuteActionNotification(user *dataprovider.User, operation, filePath, target, sshCmd, protocol string, fileSize int64, err error) {
+	notification := newActionNotification(user, operation, filePath, target, sshCmd, protocol, fileSize, err)
 
-	go actionHandler.Handle(notification) // nolint:errcheck
+	if utils.IsStringInSlice(operation, Config.Actions.ExecuteSync) {
+		actionHandler.Handle(notification) //nolint:errcheck
+		return
+	}
+
+	go actionHandler.Handle(notification) //nolint:errcheck
 }
 
 // ActionHandler handles a notification for a Protocol Action.

common/actions_test.go

@@ -110,7 +110,7 @@ func TestActionCMD(t *testing.T) {
 	err = actionHandler.Handle(a)
 	assert.NoError(t, err)
 
-	SSHCommandActionNotification(user, "path", "target", "sha1sum", nil)
+	ExecuteActionNotification(user, operationSSHCmd, "path", "target", "sha1sum", ProtocolSSH, 0, nil)
 
 	Config.Actions = actionsCopy
 }

common/connection.go

@@ -285,8 +285,7 @@ func (c *BaseConnection) RemoveFile(fs vfs.Fs, fsPath, virtualPath string, info
 		}
 	}
 	if actionErr != nil {
-		action := newActionNotification(&c.User, operationDelete, fsPath, "", "", c.protocol, size, nil)
-		go actionHandler.Handle(action) // nolint:errcheck
+		ExecuteActionNotification(&c.User, operationDelete, fsPath, "", "", c.protocol, size, nil)
 	}
 	return nil
 }
@@ -405,9 +404,7 @@ 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)
-	action := newActionNotification(&c.User, operationRename, fsSourcePath, fsTargetPath, "", c.protocol, 0, nil)
-	// the returned error is used in test cases only, we already log the error inside action.execute
-	go actionHandler.Handle(action) // nolint:errcheck
+	ExecuteActionNotification(&c.User, operationRename, fsSourcePath, fsTargetPath, "", c.protocol, 0, nil)
 
 	return nil
 }

common/protocol_test.go

@@ -2186,6 +2186,48 @@ func TestPasswordCaching(t *testing.T) {
 	assert.False(t, match)
 }
 
+func TestSyncUploadAction(t *testing.T) {
+	if runtime.GOOS == osWindows {
+		t.Skip("this test is not available on Windows")
+	}
+	uploadScriptPath := filepath.Join(os.TempDir(), "upload.sh")
+	common.Config.Actions.ExecuteOn = []string{"upload"}
+	common.Config.Actions.ExecuteSync = []string{"upload"}
+	common.Config.Actions.Hook = uploadScriptPath
+
+	user, _, err := httpdtest.AddUser(getTestUser(), http.StatusCreated)
+	assert.NoError(t, err)
+	movedPath := filepath.Join(user.HomeDir, "moved.dat")
+	err = os.WriteFile(uploadScriptPath, getUploadScriptContent(movedPath), 0755)
+	assert.NoError(t, err)
+	conn, client, err := getSftpClient(user)
+	if assert.NoError(t, err) {
+		defer conn.Close()
+		defer client.Close()
+
+		size := int64(32768)
+		err = writeSFTPFileNoCheck(testFileName, size, client)
+		assert.NoError(t, err)
+		_, err = client.Stat(testFileName)
+		assert.Error(t, err)
+		info, err := client.Stat(filepath.Base(movedPath))
+		if assert.NoError(t, err) {
+			assert.Equal(t, size, info.Size())
+		}
+	}
+
+	err = os.Remove(uploadScriptPath)
+	assert.NoError(t, err)
+	_, err = httpdtest.RemoveUser(user, http.StatusOK)
+	assert.NoError(t, err)
+	err = os.RemoveAll(user.GetHomeDir())
+	assert.NoError(t, err)
+
+	common.Config.Actions.ExecuteOn = nil
+	common.Config.Actions.ExecuteSync = nil
+	common.Config.Actions.Hook = uploadScriptPath
+}
+
 func TestQuotaTrackDisabled(t *testing.T) {
 	err := dataprovider.Close()
 	assert.NoError(t, err)
@@ -2691,6 +2733,21 @@ func getCryptFsUser() dataprovider.User {
 }
 
 func writeSFTPFile(name string, size int64, client *sftp.Client) error {
+	err := writeSFTPFileNoCheck(name, size, client)
+	if err != nil {
+		return err
+	}
+	info, err := client.Stat(name)
+	if err != nil {
+		return err
+	}
+	if info.Size() != size {
+		return fmt.Errorf("file size mismatch, wanted %v, actual %v", size, info.Size())
+	}
+	return nil
+}
+
+func writeSFTPFileNoCheck(name string, size int64, client *sftp.Client) error {
 	content := make([]byte, size)
 	_, err := rand.Read(content)
 	if err != nil {
@@ -2705,16 +2762,12 @@ func writeSFTPFile(name string, size int64, client *sftp.Client) error {
 		f.Close()
 		return err
 	}
-	err = f.Close()
-	if err != nil {
-		return err
-	}
-	info, err := client.Stat(name)
-	if err != nil {
-		return err
-	}
-	if info.Size() != size {
-		return fmt.Errorf("file size mismatch, wanted %v, actual %v", size, info.Size())
-	}
-	return nil
+	return f.Close()
+}
+
+func getUploadScriptContent(movedPath string) []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))...)
+	return content
 }

common/transfer.go

@@ -235,9 +235,8 @@ 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)
-		action := newActionNotification(&t.Connection.User, operationDownload, t.fsPath, "", "", t.Connection.protocol,
+		ExecuteActionNotification(&t.Connection.User, operationDownload, t.fsPath, "", "", t.Connection.protocol,
 			atomic.LoadInt64(&t.BytesSent), t.ErrTransfer)
-		go actionHandler.Handle(action) //nolint:errcheck
 	} else {
 		fileSize := atomic.LoadInt64(&t.BytesReceived) + t.MinWriteOffset
 		if statSize, err := t.getUploadFileSize(); err == nil {
@@ -247,9 +246,8 @@ func (t *BaseTransfer) Close() error {
 		t.updateQuota(numFiles, fileSize)
 		logger.TransferLog(uploadLogSender, t.fsPath, elapsed, atomic.LoadInt64(&t.BytesReceived), t.Connection.User.Username,
 			t.Connection.ID, t.Connection.protocol)
-		action := newActionNotification(&t.Connection.User, operationUpload, t.fsPath, "", "", t.Connection.protocol,
-			fileSize, t.ErrTransfer)
-		go actionHandler.Handle(action) //nolint:errcheck
+		ExecuteActionNotification(&t.Connection.User, operationUpload, t.fsPath, "", "", t.Connection.protocol, fileSize,
+			t.ErrTransfer)
 	}
 	if t.ErrTransfer != nil {
 		t.Connection.Log(logger.LevelWarn, "transfer error: %v, path: %#v", t.ErrTransfer, t.fsPath)

config/config.go

@@ -110,8 +110,9 @@ func Init() {
 			IdleTimeout: 15,
 			UploadMode:  0,
 			Actions: common.ProtocolActions{
-				ExecuteOn: []string{},
-				Hook:      "",
+				ExecuteOn:   []string{},
+				ExecuteSync: []string{},
+				Hook:        "",
 			},
 			SetstatMode:           0,
 			ProxyProtocol:         0,
@@ -882,6 +883,7 @@ func setViperDefaults() {
 	viper.SetDefault("common.idle_timeout", globalConf.Common.IdleTimeout)
 	viper.SetDefault("common.upload_mode", globalConf.Common.UploadMode)
 	viper.SetDefault("common.actions.execute_on", globalConf.Common.Actions.ExecuteOn)
+	viper.SetDefault("common.actions.execute_sync", globalConf.Common.Actions.ExecuteSync)
 	viper.SetDefault("common.actions.hook", globalConf.Common.Actions.Hook)
 	viper.SetDefault("common.setstat_mode", globalConf.Common.SetstatMode)
 	viper.SetDefault("common.proxy_protocol", globalConf.Common.ProxyProtocol)

docs/custom-actions.md

@@ -48,7 +48,9 @@ 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 `actions` struct inside the "data_provider" configuration section allows you to configure actions on user add, update, delete.
+The `pre-delete` action is 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.
+
+The `actions` struct inside the `data_provider` configuration section allows you to configure actions on user add, update, delete.
 
 Actions will not be fired for internal updates, such as the last login or the user quota fields, or after external authentication.
 

docs/full-configuration.md

@@ -53,6 +53,7 @@ The configuration file contains the following sections:
   - `upload_mode` integer. 0 means standard: the files are uploaded directly to the requested path. 1 means atomic: files are uploaded to a temporary path and renamed to the requested path when the client ends the upload. Atomic mode avoids problems such as a web server that serves partial files when the files are being uploaded. In atomic mode, if there is an upload error, the temporary file is deleted and so the requested upload path will not contain a partial file. 2 means atomic with resume support: same as atomic but if there is an upload error, the temporary file is renamed to the requested path and not deleted. This way, a client can reconnect and resume the upload.
   - `actions`, struct. It contains the command to execute and/or the HTTP URL to notify and the trigger conditions. See [Custom Actions](./custom-actions.md) for more details
     - `execute_on`, list of strings. Valid values are `download`, `upload`, `pre-delete`, `delete`, `rename`, `ssh_cmd`. Leave empty to disable actions.
+    - `execute_sync`, list of strings. Actions to be performed synchronously. The `pre-delete` action is always executed synchronously while the other ones are asynchronous. 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. Leave empty to execute only the `pre-delete` hook synchronously
     - `hook`, string. Absolute path to the command to execute or HTTP URL to notify.
   - `setstat_mode`, integer. 0 means "normal mode": requests for changing permissions, owner/group and access/modification times are executed. 1 means "ignore mode": requests for changing permissions, owner/group and access/modification times are silently ignored. 2 means "ignore mode for cloud based filesystems": requests for changing permissions, owner/group and access/modification times are silently ignored for cloud filesystems and executed for local filesystem.
   - `proxy_protocol`, integer. Support for [HAProxy PROXY protocol](https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt). If you are running SFTPGo behind a proxy server such as HAProxy, AWS ELB or NGNIX, you can enable the proxy protocol. It provides a convenient way to safely transport connection information such as a client's address across multiple layers of NAT or TCP proxies to get the real client IP address instead of the proxy IP. Both protocol versions 1 and 2 are supported. If the proxy protocol is enabled in SFTPGo then you have to enable the protocol in your proxy configuration too. For example, for HAProxy, add `send-proxy` or `send-proxy-v2` to each server configuration line. The following modes are supported:

sftpd/ssh_cmd.go

@@ -747,7 +747,7 @@ func (c *sshCommand) sendExitStatus(err error) {
 				targetPath = p
 			}
 		}
-		common.SSHCommandActionNotification(&c.connection.User, cmdPath, targetPath, c.command, err)
+		common.ExecuteActionNotification(&c.connection.User, "ssh_cmd", cmdPath, targetPath, c.command, common.ProtocolSSH, 0, err)
 	}
 }
 

sftpgo.json

@@ -4,6 +4,7 @@
     "upload_mode": 0,
     "actions": {
       "execute_on": [],
+      "execute_sync": [],
       "hook": ""
     },
     "setstat_mode": 0,

webdavd/webdavd_test.go

@@ -1168,11 +1168,11 @@ func TestDeniedProtocols(t *testing.T) {
 
 func TestQuotaLimits(t *testing.T) {
 	u := getTestUser()
-	u.QuotaFiles = 1
+	u.QuotaFiles = 100
 	localUser, _, err := httpdtest.AddUser(u, http.StatusCreated)
 	assert.NoError(t, err)
 	u = getTestSFTPUser()
-	u.QuotaFiles = 1
+	u.QuotaFiles = 100
 	sftpUser, _, err := httpdtest.AddUser(u, http.StatusCreated)
 	assert.NoError(t, err)
 	for _, user := range []dataprovider.User{localUser, sftpUser} {
@@ -1190,7 +1190,7 @@ func TestQuotaLimits(t *testing.T) {
 		testFilePath2 := filepath.Join(homeBasePath, testFileName2)
 		err = createTestFile(testFilePath2, testFileSize2)
 		assert.NoError(t, err)
-		client := getWebDavClient(user, true, nil)
+		client := getWebDavClient(user, false, nil)
 		// test quota files
 		err = uploadFile(testFilePath, testFileName+".quota", testFileSize, client)
 		if !assert.NoError(t, err, "username: %v", user.Username) {
@@ -1200,6 +1200,9 @@ func TestQuotaLimits(t *testing.T) {
 			}
 			printLatestLogs(20)
 		}
+		user.QuotaFiles = 1
+		user, _, err = httpdtest.UpdateUser(user, http.StatusOK, "")
+		assert.NoError(t, err)
 		err = uploadFile(testFilePath, testFileName+".quota1", testFileSize, client)
 		assert.Error(t, err, "username: %v", user.Username)
 		err = client.Rename(testFileName+".quota", testFileName, false)
@@ -2476,7 +2479,7 @@ func getWebDavClient(user dataprovider.User, useTLS bool, tlsConfig *tls.Config)
 		pwd = user.Password
 	}
 	client := gowebdav.NewClient(rootPath, user.Username, pwd)
-	client.SetTimeout(5 * time.Second)
+	client.SetTimeout(10 * time.Second)
 	if tlsConfig != nil {
 		customTransport := http.DefaultTransport.(*http.Transport).Clone()
 		customTransport.TLSClientConfig = tlsConfig
@@ -2589,18 +2592,7 @@ func createTestFile(path string, size int64) error {
 		return err
 	}
 
-	f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.ModePerm)
-	if err != nil {
-		return err
-	}
-	_, err = f.Write(content)
-	if err == nil {
-		err = f.Sync()
-	}
-	if err1 := f.Close(); err1 != nil && err == nil {
-		err = err1
-	}
-	return err
+	return os.WriteFile(path, content, os.ModePerm)
 }
 
 func printLatestLogs(maxNumberOfLines int) {