From 6c1a7449fef8eebb87c58d92956004774bb998b2 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sat, 19 Sep 2020 10:14:30 +0200 Subject: [PATCH] ssh commands: return better error messages This improve the fix for #171 and return better error message for SSH commands other than SCP too --- common/connection.go | 13 ++++++++----- common/connection_test.go | 12 +++++++++--- ftpd/handler.go | 2 +- sftpd/scp.go | 35 ++++++++++++++--------------------- vfs/gcsfs.go | 10 ---------- vfs/osfs.go | 10 ---------- vfs/s3fs.go | 10 ---------- vfs/vfs.go | 2 -- 8 files changed, 32 insertions(+), 62 deletions(-) diff --git a/common/connection.go b/common/connection.go index fab5959e..e0d50b5f 100644 --- a/common/connection.go +++ b/common/connection.go @@ -320,7 +320,7 @@ func (c *BaseConnection) RemoveDir(fsPath, virtualPath string) error { } if !fi.IsDir() || fi.Mode()&os.ModeSymlink == os.ModeSymlink { c.Log(logger.LevelDebug, "cannot remove %#v is not a directory", fsPath) - return c.GetGenericError() + return c.GetGenericError(nil) } if err := c.Fs.Remove(fsPath, true); err != nil { @@ -379,7 +379,7 @@ func (c *BaseConnection) Rename(fsSourcePath, fsTargetPath, virtualSourcePath, v } if !c.hasSpaceForRename(virtualSourcePath, virtualTargetPath, initialSize, fsSourcePath) { c.Log(logger.LevelInfo, "denying cross rename due to space limit") - return c.GetGenericError() + return c.GetGenericError(ErrQuotaExceeded) } if err := c.Fs.Rename(fsSourcePath, fsTargetPath); err != nil { c.Log(logger.LevelWarn, "failed to rename %#v -> %#v: %+v", fsSourcePath, fsTargetPath, err) @@ -412,7 +412,7 @@ func (c *BaseConnection) CreateSymlink(fsSourcePath, fsTargetPath, virtualSource } if c.isCrossFoldersRequest(virtualSourcePath, virtualTargetPath) { c.Log(logger.LevelWarn, "cross folder symlink is not supported, src: %v dst: %v", virtualSourcePath, virtualTargetPath) - return c.GetGenericError() + return c.GetOpUnsupportedError() } if c.User.IsMappedPath(fsSourcePath) { c.Log(logger.LevelWarn, "symlinking a directory mapped as virtual folder is not allowed: %#v", fsSourcePath) @@ -932,11 +932,14 @@ func (c *BaseConnection) GetOpUnsupportedError() error { } // GetGenericError returns an appropriate generic error for the connection protocol -func (c *BaseConnection) GetGenericError() error { +func (c *BaseConnection) GetGenericError(err error) error { switch c.protocol { case ProtocolSFTP: return sftp.ErrSSHFxFailure default: + if err == ErrPermissionDenied || err == ErrNotExist || err == ErrOpUnsupported || err == ErrQuotaExceeded { + return err + } return ErrGenericFailure } } @@ -948,7 +951,7 @@ func (c *BaseConnection) GetFsError(err error) error { } else if c.Fs.IsPermission(err) { return c.GetPermissionDeniedError() } else if err != nil { - return c.GetGenericError() + return c.GetGenericError(err) } return nil } diff --git a/common/connection_test.go b/common/connection_test.go index b3c24554..3a9712ca 100644 --- a/common/connection_test.go +++ b/common/connection_test.go @@ -227,7 +227,7 @@ func TestRemoveDir(t *testing.T) { assert.NoError(t, err) err = c.RemoveDir(testDir, "testDir") if assert.Error(t, err) { - assert.EqualError(t, err, c.GetGenericError().Error()) + assert.EqualError(t, err, c.GetGenericError(err).Error()) } err = os.Remove(testDir) assert.NoError(t, err) @@ -349,7 +349,7 @@ func TestRename(t *testing.T) { // and so the remaining space cannot be computed err = c.Rename(filepath.Join(user.GetHomeDir(), "adir"), filepath.Join(user.GetHomeDir(), "another"), "/vdir1/sub/a", "/vdir2/b") if assert.Error(t, err) { - assert.EqualError(t, err, c.GetGenericError().Error()) + assert.EqualError(t, err, c.GetGenericError(err).Error()) } err = os.RemoveAll(mappedPath1) @@ -409,7 +409,7 @@ func TestCreateSymlink(t *testing.T) { } err = c.CreateSymlink(filepath.Join(user.GetHomeDir(), "b"), mappedPath1, "/vdir1/b", "/vdir2/b") if assert.Error(t, err) { - assert.EqualError(t, err, c.GetGenericError().Error()) + assert.EqualError(t, err, c.GetOpUnsupportedError().Error()) } err = c.CreateSymlink(mappedPath1, filepath.Join(mappedPath1, "b"), "/vdir1/a", "/vdir1/b") if assert.Error(t, err) { @@ -1140,6 +1140,12 @@ func TestErrorsMapping(t *testing.T) { } else { assert.EqualError(t, err, ErrGenericFailure.Error()) } + err = conn.GetFsError(ErrPermissionDenied) + if protocol == ProtocolSFTP { + assert.EqualError(t, err, sftp.ErrSSHFxFailure.Error()) + } else { + assert.EqualError(t, err, ErrPermissionDenied.Error()) + } err = conn.GetFsError(nil) assert.NoError(t, err) err = conn.GetOpUnsupportedError() diff --git a/ftpd/handler.go b/ftpd/handler.go index f530be81..e51fbbd5 100644 --- a/ftpd/handler.go +++ b/ftpd/handler.go @@ -101,7 +101,7 @@ func (c *Connection) Remove(name string) error { if fi.IsDir() && fi.Mode()&os.ModeSymlink != os.ModeSymlink { c.Log(logger.LevelDebug, "cannot remove %#v is not a file/symlink", p) - return c.GetGenericError() + return c.GetGenericError(nil) } return c.RemoveFile(p, name, fi) } diff --git a/sftpd/scp.go b/sftpd/scp.go index fdda0658..08a12c4e 100644 --- a/sftpd/scp.go +++ b/sftpd/scp.go @@ -124,9 +124,8 @@ func (c *scpCommand) handleCreateDir(dirPath string) error { } if !c.connection.User.HasPerm(dataprovider.PermCreateDirs, path.Dir(dirPath)) { c.connection.Log(logger.LevelWarn, "error creating dir: %#v, permission denied", dirPath) - err := c.connection.Fs.GetPermissionError() - c.sendErrorMessage(err) - return err + c.sendErrorMessage(common.ErrPermissionDenied) + return common.ErrPermissionDenied } err = c.createDir(p) @@ -240,9 +239,8 @@ func (c *scpCommand) handleUpload(uploadFilePath string, sizeToRead int64) error if !c.connection.User.IsFileAllowed(uploadFilePath) { c.connection.Log(logger.LevelWarn, "writing file %#v is not allowed", uploadFilePath) - err := c.connection.Fs.GetPermissionError() - c.sendErrorMessage(err) - return err + c.sendErrorMessage(common.ErrPermissionDenied) + return common.ErrPermissionDenied } p, err := c.connection.Fs.ResolvePath(uploadFilePath) @@ -259,9 +257,8 @@ func (c *scpCommand) handleUpload(uploadFilePath string, sizeToRead int64) error if (statErr == nil && stat.Mode()&os.ModeSymlink == os.ModeSymlink) || c.connection.Fs.IsNotExist(statErr) { if !c.connection.User.HasPerm(dataprovider.PermUpload, path.Dir(uploadFilePath)) { c.connection.Log(logger.LevelWarn, "cannot upload file: %#v, permission denied", uploadFilePath) - err := c.connection.Fs.GetPermissionError() - c.sendErrorMessage(err) - return err + c.sendErrorMessage(common.ErrPermissionDenied) + return common.ErrPermissionDenied } return c.handleUploadFile(p, filePath, sizeToRead, true, 0, uploadFilePath) } @@ -281,9 +278,8 @@ func (c *scpCommand) handleUpload(uploadFilePath string, sizeToRead int64) error if !c.connection.User.HasPerm(dataprovider.PermOverwrite, uploadFilePath) { c.connection.Log(logger.LevelWarn, "cannot overwrite file: %#v, permission denied", uploadFilePath) - err := c.connection.Fs.GetPermissionError() - c.sendErrorMessage(err) - return err + c.sendErrorMessage(common.ErrPermissionDenied) + return common.ErrPermissionDenied } if common.Config.IsAtomicUploadEnabled() && c.connection.Fs.IsAtomicUploadSupported() { @@ -463,9 +459,8 @@ func (c *scpCommand) handleDownload(filePath string) error { if stat.IsDir() { if !c.connection.User.HasPerm(dataprovider.PermDownload, filePath) { c.connection.Log(logger.LevelWarn, "error downloading dir: %#v, permission denied", filePath) - err := c.connection.Fs.GetPermissionError() - c.sendErrorMessage(err) - return err + c.sendErrorMessage(common.ErrPermissionDenied) + return common.ErrPermissionDenied } err = c.handleRecursiveDownload(p, stat) return err @@ -473,16 +468,14 @@ func (c *scpCommand) handleDownload(filePath string) error { if !c.connection.User.HasPerm(dataprovider.PermDownload, path.Dir(filePath)) { c.connection.Log(logger.LevelWarn, "error downloading dir: %#v, permission denied", filePath) - err := c.connection.Fs.GetPermissionError() - c.sendErrorMessage(err) - return err + c.sendErrorMessage(common.ErrPermissionDenied) + return common.ErrPermissionDenied } if !c.connection.User.IsFileAllowed(filePath) { c.connection.Log(logger.LevelWarn, "reading file %#v is not allowed", filePath) - err := c.connection.Fs.GetPermissionError() - c.sendErrorMessage(err) - return err + c.sendErrorMessage(common.ErrPermissionDenied) + return common.ErrPermissionDenied } file, r, cancelFn, err := c.connection.Fs.Open(p, 0) diff --git a/vfs/gcsfs.go b/vfs/gcsfs.go index ac0a605c..59103cb6 100644 --- a/vfs/gcsfs.go +++ b/vfs/gcsfs.go @@ -402,16 +402,6 @@ func (GCSFs) IsPermission(err error) bool { return strings.Contains(err.Error(), "403") } -// GetPermissionError returns a permission error for this FS -func (GCSFs) GetPermissionError() error { - return errors.New("403 permission denied") -} - -// GetNotExistError returns a not exist error for this FS -func (GCSFs) GetNotExistError() error { - return errors.New("404 no such file or directory") -} - // CheckRootPath creates the specified local root directory if it does not exists func (fs GCSFs) CheckRootPath(username string, uid int, gid int) bool { // we need a local directory for temporary files diff --git a/vfs/osfs.go b/vfs/osfs.go index b60b5719..d3558561 100644 --- a/vfs/osfs.go +++ b/vfs/osfs.go @@ -183,16 +183,6 @@ func (OsFs) IsPermission(err error) bool { return os.IsPermission(err) } -// GetPermissionError returns a permission error for this FS -func (OsFs) GetPermissionError() error { - return os.ErrPermission -} - -// GetNotExistError returns a not exist error for this FS -func (OsFs) GetNotExistError() error { - return os.ErrNotExist -} - // CheckRootPath creates the root directory if it does not exists func (fs OsFs) CheckRootPath(username string, uid int, gid int) bool { var err error diff --git a/vfs/s3fs.go b/vfs/s3fs.go index 837fbd1d..95bbba43 100644 --- a/vfs/s3fs.go +++ b/vfs/s3fs.go @@ -427,16 +427,6 @@ func (S3Fs) IsPermission(err error) bool { return strings.Contains(err.Error(), "403") } -// GetPermissionError returns a permission error for this FS -func (S3Fs) GetPermissionError() error { - return errors.New("403 permission denied") -} - -// GetNotExistError returns a not exist error for this FS -func (S3Fs) GetNotExistError() error { - return errors.New("404 no such file or directory") -} - // CheckRootPath creates the specified local root directory if it does not exists func (fs S3Fs) CheckRootPath(username string, uid int, gid int) bool { // we need a local directory for temporary files diff --git a/vfs/vfs.go b/vfs/vfs.go index 1015515c..fbcf7108 100644 --- a/vfs/vfs.go +++ b/vfs/vfs.go @@ -40,8 +40,6 @@ type Fs interface { ResolvePath(sftpPath string) (string, error) IsNotExist(err error) bool IsPermission(err error) bool - GetPermissionError() error - GetNotExistError() error ScanRootDirContents() (int, int64, error) GetDirSize(dirname string) (int, int64, error) GetAtomicUploadPath(name string) string