From 729f30aebf65c0d860083333f12e7825e369531e Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Thu, 16 Jun 2022 18:42:17 +0200 Subject: [PATCH] allow to refuse an upload from a sync upload hook Fixes #880 Signed-off-by: Nicola Murino --- common/actions.go | 10 ++++---- common/actions_test.go | 9 +++++--- common/connection.go | 10 ++++---- common/protocol_test.go | 51 +++++++++++++++++++++++++++++++++++++---- common/transfer.go | 25 ++++++++++++++++++-- docs/custom-actions.md | 2 ++ go.mod | 4 ++-- go.sum | 8 +++---- sftpd/ssh_cmd.go | 4 ++-- 9 files changed, 95 insertions(+), 28 deletions(-) diff --git a/common/actions.go b/common/actions.go index 6f0fcaaf..ed624341 100644 --- a/common/actions.go +++ b/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. diff --git a/common/actions_test.go b/common/actions_test.go index a81c19c9..3a4569f7 100644 --- a/common/actions_test.go +++ b/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) diff --git a/common/connection.go b/common/connection.go index 567e3f87..29658c3a 100644 --- a/common/connection.go +++ b/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 } diff --git a/common/protocol_test.go b/common/protocol_test.go index 4873f7ea..8d2a5a39 100644 --- a/common/protocol_test.go +++ b/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 } diff --git a/common/transfer.go b/common/transfer.go index c84c5edc..d7b46755 100644 --- a/common/transfer.go +++ b/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) diff --git a/docs/custom-actions.md b/docs/custom-actions.md index 36fc0167..56251f80 100644 --- a/docs/custom-actions.md +++ b/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 diff --git a/go.mod b/go.mod index a6c71111..71630b23 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index edbf6db6..b29afa8c 100644 --- a/go.sum +++ b/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= diff --git a/sftpd/ssh_cmd.go b/sftpd/ssh_cmd.go index 5d2caf62..467587f9 100644 --- a/sftpd/ssh_cmd.go +++ b/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(),