FTP: try to return more specific error codes/messages for some errors

We now return 552 code for quota exceeded errors and 553 in the following
cases:

- filename denied by a filter
- no upload permission
- no overwrite permission
- pre upload hook error

Fixes #442
This commit is contained in:
Nicola Murino 2021-06-28 19:40:04 +02:00
parent 076b2f0ee0
commit 04001f7ad3
No known key found for this signature in database
GPG key ID: 2F1FB59433D5A8CB
8 changed files with 41 additions and 19 deletions

View file

@ -10,6 +10,7 @@ import (
"sync/atomic" "sync/atomic"
"time" "time"
ftpserver "github.com/fclairamb/ftpserverlib"
"github.com/pkg/sftp" "github.com/pkg/sftp"
"github.com/drakkan/sftpgo/v2/dataprovider" "github.com/drakkan/sftpgo/v2/dataprovider"
@ -796,7 +797,7 @@ func (c *BaseConnection) GetMaxWriteSize(quotaResult vfs.QuotaCheckResult, isRes
return 0, c.GetOpUnsupportedError() return 0, c.GetOpUnsupportedError()
} }
if c.User.Filters.MaxUploadFileSize > 0 && c.User.Filters.MaxUploadFileSize <= fileSize { if c.User.Filters.MaxUploadFileSize > 0 && c.User.Filters.MaxUploadFileSize <= fileSize {
return 0, ErrQuotaExceeded return 0, c.GetQuotaExceededError()
} }
if c.User.Filters.MaxUploadFileSize > 0 { if c.User.Filters.MaxUploadFileSize > 0 {
maxUploadSize := c.User.Filters.MaxUploadFileSize - fileSize maxUploadSize := c.User.Filters.MaxUploadFileSize - fileSize
@ -1058,6 +1059,26 @@ func (c *BaseConnection) GetOpUnsupportedError() error {
} }
} }
// GetQuotaExceededError returns an appropriate storage limit exceeded error for the connection protocol
func (c *BaseConnection) GetQuotaExceededError() error {
switch c.protocol {
case ProtocolFTP:
return ftpserver.ErrStorageExceeded
default:
return ErrQuotaExceeded
}
}
// IsQuotaExceededError returns true if the given error is a quota exceeded error
func (c *BaseConnection) IsQuotaExceededError(err error) bool {
switch c.protocol {
case ProtocolFTP:
return errors.Is(err, ftpserver.ErrStorageExceeded)
default:
return errors.Is(err, ErrQuotaExceeded)
}
}
// GetGenericError returns an appropriate generic error for the connection protocol // GetGenericError returns an appropriate generic error for the connection protocol
func (c *BaseConnection) GetGenericError(err error) error { func (c *BaseConnection) GetGenericError(err error) error {
switch c.protocol { switch c.protocol {

View file

@ -259,6 +259,8 @@ func TestErrorsMapping(t *testing.T) {
} else { } else {
assert.EqualError(t, err, vfs.ErrStorageSizeUnavailable.Error()) assert.EqualError(t, err, vfs.ErrStorageSizeUnavailable.Error())
} }
err = conn.GetQuotaExceededError()
assert.True(t, conn.IsQuotaExceededError(err))
err = conn.GetFsError(fs, nil) err = conn.GetFsError(fs, nil)
assert.NoError(t, err) assert.NoError(t, err)
err = conn.GetOpUnsupportedError() err = conn.GetOpUnsupportedError()
@ -307,7 +309,7 @@ func TestMaxWriteSize(t *testing.T) {
quotaResult.QuotaSize = 0 quotaResult.QuotaSize = 0
quotaResult.UsedSize = 0 quotaResult.UsedSize = 0
size, err = conn.GetMaxWriteSize(quotaResult, true, 100, fs.IsUploadResumeSupported()) size, err = conn.GetMaxWriteSize(quotaResult, true, 100, fs.IsUploadResumeSupported())
assert.EqualError(t, err, ErrQuotaExceeded.Error()) assert.True(t, conn.IsQuotaExceededError(err))
assert.Equal(t, int64(0), size) assert.Equal(t, int64(0), size)
size, err = conn.GetMaxWriteSize(quotaResult, true, 10, fs.IsUploadResumeSupported()) size, err = conn.GetMaxWriteSize(quotaResult, true, 10, fs.IsUploadResumeSupported())

View file

@ -207,7 +207,7 @@ func (t *BaseTransfer) Close() error {
numFiles = 1 numFiles = 1
} }
metrics.TransferCompleted(atomic.LoadInt64(&t.BytesSent), atomic.LoadInt64(&t.BytesReceived), t.transferType, t.ErrTransfer) metrics.TransferCompleted(atomic.LoadInt64(&t.BytesSent), atomic.LoadInt64(&t.BytesReceived), t.transferType, t.ErrTransfer)
if t.ErrTransfer == ErrQuotaExceeded && t.File != nil { if t.File != nil && t.Connection.IsQuotaExceededError(t.ErrTransfer) {
// if quota is exceeded we try to remove the partial file for uploads to local filesystem // if quota is exceeded we try to remove the partial file for uploads to local filesystem
err = t.Fs.Remove(t.File.Name(), false) err = t.Fs.Remove(t.File.Name(), false)
if err == nil { if err == nil {

View file

@ -19,6 +19,7 @@ import (
"testing" "testing"
"time" "time"
ftpserver "github.com/fclairamb/ftpserverlib"
"github.com/jlaffaye/ftp" "github.com/jlaffaye/ftp"
"github.com/rs/zerolog" "github.com/rs/zerolog"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
@ -819,11 +820,11 @@ func TestPreUploadHook(t *testing.T) {
assert.NoError(t, err) assert.NoError(t, err)
err = ftpUploadFile(testFilePath, testFileName, testFileSize, client, 0) err = ftpUploadFile(testFilePath, testFileName, testFileSize, client, 0)
if assert.Error(t, err) { if assert.Error(t, err) {
assert.Contains(t, err.Error(), "permission denied") assert.Contains(t, err.Error(), ftpserver.ErrFileNameNotAllowed.Error())
} }
err = ftpUploadFile(testFilePath, testFileName+"1", testFileSize, client, 0) err = ftpUploadFile(testFilePath, testFileName+"1", testFileSize, client, 0)
if assert.Error(t, err) { if assert.Error(t, err) {
assert.Contains(t, err.Error(), "permission denied") assert.Contains(t, err.Error(), ftpserver.ErrFileNameNotAllowed.Error())
} }
err := client.Quit() err := client.Quit()
assert.NoError(t, err) assert.NoError(t, err)

View file

@ -2,6 +2,7 @@ package ftpd
import ( import (
"errors" "errors"
"fmt"
"io" "io"
"os" "os"
"path" "path"
@ -318,7 +319,7 @@ func (c *Connection) downloadFile(fs vfs.Fs, fsPath, ftpPath string, offset int6
func (c *Connection) uploadFile(fs vfs.Fs, fsPath, ftpPath string, flags int) (ftpserver.FileTransfer, error) { func (c *Connection) uploadFile(fs vfs.Fs, fsPath, ftpPath string, flags int) (ftpserver.FileTransfer, error) {
if !c.User.IsFileAllowed(ftpPath) { if !c.User.IsFileAllowed(ftpPath) {
c.Log(logger.LevelWarn, "writing file %#v is not allowed", ftpPath) c.Log(logger.LevelWarn, "writing file %#v is not allowed", ftpPath)
return nil, c.GetPermissionDeniedError() return nil, ftpserver.ErrFileNameNotAllowed
} }
filePath := fsPath filePath := fsPath
@ -329,7 +330,7 @@ func (c *Connection) uploadFile(fs vfs.Fs, fsPath, ftpPath string, flags int) (f
stat, statErr := fs.Lstat(fsPath) stat, statErr := fs.Lstat(fsPath)
if (statErr == nil && stat.Mode()&os.ModeSymlink != 0) || fs.IsNotExist(statErr) { if (statErr == nil && stat.Mode()&os.ModeSymlink != 0) || fs.IsNotExist(statErr) {
if !c.User.HasPerm(dataprovider.PermUpload, path.Dir(ftpPath)) { if !c.User.HasPerm(dataprovider.PermUpload, path.Dir(ftpPath)) {
return nil, c.GetPermissionDeniedError() return nil, fmt.Errorf("%w, no upload permission", ftpserver.ErrFileNameNotAllowed)
} }
return c.handleFTPUploadToNewFile(fs, fsPath, filePath, ftpPath) return c.handleFTPUploadToNewFile(fs, fsPath, filePath, ftpPath)
} }
@ -346,7 +347,7 @@ func (c *Connection) uploadFile(fs vfs.Fs, fsPath, ftpPath string, flags int) (f
} }
if !c.User.HasPerm(dataprovider.PermOverwrite, path.Dir(ftpPath)) { if !c.User.HasPerm(dataprovider.PermOverwrite, path.Dir(ftpPath)) {
return nil, c.GetPermissionDeniedError() return nil, fmt.Errorf("%w, no overwrite permission", ftpserver.ErrFileNameNotAllowed)
} }
return c.handleFTPUploadToExistingFile(fs, flags, fsPath, filePath, stat.Size(), ftpPath) return c.handleFTPUploadToExistingFile(fs, flags, fsPath, filePath, stat.Size(), ftpPath)
@ -356,11 +357,11 @@ func (c *Connection) handleFTPUploadToNewFile(fs vfs.Fs, resolvedPath, filePath,
quotaResult := c.HasSpace(true, false, requestPath) quotaResult := c.HasSpace(true, false, requestPath)
if !quotaResult.HasSpace { if !quotaResult.HasSpace {
c.Log(logger.LevelInfo, "denying file write due to quota limits") c.Log(logger.LevelInfo, "denying file write due to quota limits")
return nil, common.ErrQuotaExceeded return nil, ftpserver.ErrStorageExceeded
} }
if err := common.ExecutePreAction(&c.User, common.OperationPreUpload, resolvedPath, requestPath, c.GetProtocol(), 0, 0); err != nil { if err := common.ExecutePreAction(&c.User, common.OperationPreUpload, resolvedPath, requestPath, c.GetProtocol(), 0, 0); err != nil {
c.Log(logger.LevelDebug, "upload for file %#v denied by pre action: %v", requestPath, err) c.Log(logger.LevelDebug, "upload for file %#v denied by pre action: %v", requestPath, err)
return nil, c.GetPermissionDeniedError() return nil, fmt.Errorf("%w, denied by pre-upload action", ftpserver.ErrFileNameNotAllowed)
} }
file, w, cancelFn, err := fs.Create(filePath, 0) file, w, cancelFn, err := fs.Create(filePath, 0)
if err != nil { if err != nil {
@ -386,7 +387,7 @@ func (c *Connection) handleFTPUploadToExistingFile(fs vfs.Fs, flags int, resolve
quotaResult := c.HasSpace(false, false, requestPath) quotaResult := c.HasSpace(false, false, requestPath)
if !quotaResult.HasSpace { if !quotaResult.HasSpace {
c.Log(logger.LevelInfo, "denying file write due to quota limits") c.Log(logger.LevelInfo, "denying file write due to quota limits")
return nil, common.ErrQuotaExceeded return nil, ftpserver.ErrStorageExceeded
} }
minWriteOffset := int64(0) minWriteOffset := int64(0)
// ftpserverlib sets: // ftpserverlib sets:
@ -404,7 +405,7 @@ func (c *Connection) handleFTPUploadToExistingFile(fs vfs.Fs, flags int, resolve
} }
if err := common.ExecutePreAction(&c.User, common.OperationPreUpload, resolvedPath, requestPath, c.GetProtocol(), fileSize, flags); err != nil { if err := common.ExecutePreAction(&c.User, common.OperationPreUpload, resolvedPath, requestPath, c.GetProtocol(), fileSize, flags); err != nil {
c.Log(logger.LevelDebug, "upload for file %#v denied by pre action: %v", requestPath, err) c.Log(logger.LevelDebug, "upload for file %#v denied by pre action: %v", requestPath, err)
return nil, c.GetPermissionDeniedError() return nil, fmt.Errorf("%w, denied by pre-upload action", ftpserver.ErrFileNameNotAllowed)
} }
if common.Config.IsAtomicUploadEnabled() && fs.IsAtomicUploadSupported() { if common.Config.IsAtomicUploadEnabled() && fs.IsAtomicUploadSupported() {

View file

@ -65,7 +65,7 @@ func (t *transfer) Write(p []byte) (n int, err error) {
atomic.AddInt64(&t.BytesReceived, int64(n)) atomic.AddInt64(&t.BytesReceived, int64(n))
if t.MaxWriteSize > 0 && err == nil && atomic.LoadInt64(&t.BytesReceived) > t.MaxWriteSize { if t.MaxWriteSize > 0 && err == nil && atomic.LoadInt64(&t.BytesReceived) > t.MaxWriteSize {
err = common.ErrQuotaExceeded err = t.Connection.GetQuotaExceededError()
} }
if err != nil { if err != nil {
t.TransferError(err) t.TransferError(err)

2
go.mod
View file

@ -11,7 +11,7 @@ require (
github.com/aws/aws-sdk-go v1.38.68 github.com/aws/aws-sdk-go v1.38.68
github.com/cockroachdb/cockroach-go/v2 v2.1.1 github.com/cockroachdb/cockroach-go/v2 v2.1.1
github.com/eikenb/pipeat v0.0.0-20210603033007-44fc3ffce52b github.com/eikenb/pipeat v0.0.0-20210603033007-44fc3ffce52b
github.com/fclairamb/ftpserverlib v0.13.3-0.20210614220040-27dccea41813 github.com/fclairamb/ftpserverlib v0.13.3-0.20210627202858-3bf551007432
github.com/frankban/quicktest v1.13.0 // indirect github.com/frankban/quicktest v1.13.0 // indirect
github.com/go-chi/chi/v5 v5.0.3 github.com/go-chi/chi/v5 v5.0.3
github.com/go-chi/jwtauth/v5 v5.0.1 github.com/go-chi/jwtauth/v5 v5.0.1

7
go.sum
View file

@ -233,8 +233,8 @@ github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7
github.com/fatih/color v1.7.0 h1:DkWD4oS2D8LGGgTQ6IvwJJXSL5Vp2ffcQg58nFV38Ys= github.com/fatih/color v1.7.0 h1:DkWD4oS2D8LGGgTQ6IvwJJXSL5Vp2ffcQg58nFV38Ys=
github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4= github.com/fatih/color v1.7.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4=
github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M= github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga6PJ7M=
github.com/fclairamb/ftpserverlib v0.13.3-0.20210614220040-27dccea41813 h1:wXDgbyHQT62eyVff37ocEXSp0N5RiTLdoOmugP/tRRU= github.com/fclairamb/ftpserverlib v0.13.3-0.20210627202858-3bf551007432 h1:UtF6df4idbDpc/x9lMyJT/fLmu6YLRZG6nFyz7qi81Y=
github.com/fclairamb/ftpserverlib v0.13.3-0.20210614220040-27dccea41813/go.mod h1:T7GFWYWSREftxINf3ielPlaQ+aiE04/elEH0zLwkxBA= github.com/fclairamb/ftpserverlib v0.13.3-0.20210627202858-3bf551007432/go.mod h1:ATLgn4bHgiM9+vfZbK+rMu/dqgkxO5nk94x/9f8ffDI=
github.com/form3tech-oss/jwt-go v3.2.2+incompatible h1:TcekIExNqud5crz4xD2pavyTgWiPvpYe4Xau31I0PRk= github.com/form3tech-oss/jwt-go v3.2.2+incompatible h1:TcekIExNqud5crz4xD2pavyTgWiPvpYe4Xau31I0PRk=
github.com/form3tech-oss/jwt-go v3.2.2+incompatible/go.mod h1:pbq4aXjuKjdthFRnoDwaVPLA+WlJuPGy+QneDUgJi2k= github.com/form3tech-oss/jwt-go v3.2.2+incompatible/go.mod h1:pbq4aXjuKjdthFRnoDwaVPLA+WlJuPGy+QneDUgJi2k=
github.com/fortytw2/leaktest v1.3.0/go.mod h1:jDsjWgpAGjm2CA7WthBh/CdZYEPF31XHquHwclZch5g= github.com/fortytw2/leaktest v1.3.0/go.mod h1:jDsjWgpAGjm2CA7WthBh/CdZYEPF31XHquHwclZch5g=
@ -294,8 +294,6 @@ github.com/gobwas/httphead v0.0.0-20180130184737-2c6c146eadee/go.mod h1:L0fX3K22
github.com/gobwas/pool v0.2.0/go.mod h1:q8bcK0KcYlCgd9e7WYLm9LpyS+YeLd8JVDW6WezmKEw= github.com/gobwas/pool v0.2.0/go.mod h1:q8bcK0KcYlCgd9e7WYLm9LpyS+YeLd8JVDW6WezmKEw=
github.com/gobwas/ws v1.0.2/go.mod h1:szmBTxLgaFppYjEmNtny/v3w89xOydFnnZMcgRRu/EM= github.com/gobwas/ws v1.0.2/go.mod h1:szmBTxLgaFppYjEmNtny/v3w89xOydFnnZMcgRRu/EM=
github.com/goccy/go-json v0.4.8/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I= github.com/goccy/go-json v0.4.8/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I=
github.com/goccy/go-json v0.7.1 h1:VMhnh5gcc8De8f6m2DLvSqY1x8Jwl3btet+EqMP0QNs=
github.com/goccy/go-json v0.7.1/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I=
github.com/goccy/go-json v0.7.2 h1:MY1gMmtCxRpaI8YGpeHCvXUb+FVIo09pnjqF9Rhh274= github.com/goccy/go-json v0.7.2 h1:MY1gMmtCxRpaI8YGpeHCvXUb+FVIo09pnjqF9Rhh274=
github.com/goccy/go-json v0.7.2/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I= github.com/goccy/go-json v0.7.2/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I=
github.com/godbus/dbus v0.0.0-20190422162347-ade71ed3457e/go.mod h1:bBOAhwG1umN6/6ZUMtDFBMQR8jRg9O75tm9K00oMsK4= github.com/godbus/dbus v0.0.0-20190422162347-ade71ed3457e/go.mod h1:bBOAhwG1umN6/6ZUMtDFBMQR8jRg9O75tm9K00oMsK4=
@ -843,7 +841,6 @@ github.com/streadway/amqp v0.0.0-20190827072141-edfb9018d271/go.mod h1:AZpEONHx3
github.com/streadway/handy v0.0.0-20190108123426-d5acb3125c2a/go.mod h1:qNTQ5P5JnDBl6z3cMAg/SywNDC5ABu5ApDIw6lUbRmI= github.com/streadway/handy v0.0.0-20190108123426-d5acb3125c2a/go.mod h1:qNTQ5P5JnDBl6z3cMAg/SywNDC5ABu5ApDIw6lUbRmI=
github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48=
github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=