simplify some code

api/api_utils.go

@@ -197,6 +197,15 @@ func checkUser(expected dataprovider.User, actual dataprovider.User) error {
 			return errors.New("user ID mismatch")
 		}
 	}
+	for _, v := range expected.Permissions {
+		if !utils.IsStringInSlice(v, actual.Permissions) {
+			return errors.New("Permissions contents mismatch")
+		}
+	}
+	return compareEqualsUserFields(expected, actual)
+}
+
+func compareEqualsUserFields(expected dataprovider.User, actual dataprovider.User) error {
 	if expected.Username != actual.Username {
 		return errors.New("Username mismatch")
 	}
@@ -221,11 +230,6 @@ func checkUser(expected dataprovider.User, actual dataprovider.User) error {
 	if len(expected.Permissions) != len(actual.Permissions) {
 		return errors.New("Permissions mismatch")
 	}
-	for _, v := range expected.Permissions {
-		if !utils.IsStringInSlice(v, actual.Permissions) {
-			return errors.New("Permissions contents mismatch")
-		}
-	}
 	if expected.UploadBandwidth != actual.UploadBandwidth {
 		return errors.New("UploadBandwidth mismatch")
 	}

go.mod

@@ -4,6 +4,7 @@ go 1.12
 
 require (
 	github.com/alexedwards/argon2id v0.0.0-20190612080829-01a59b2b8802
+	github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835 // indirect
 	github.com/go-chi/chi v4.0.2+incompatible
 	github.com/go-chi/render v1.0.1
 	github.com/go-sql-driver/mysql v1.4.1

go.sum

@@ -3,6 +3,8 @@ github.com/alexedwards/argon2id v0.0.0-20190612080829-01a59b2b8802/go.mod h1:4ds
 github.com/coreos/go-systemd v0.0.0-20190321100706-95778dfbb74e/go.mod h1:F5haX7vjVVG0kc13fIWeqUViNPyEJxv/OmvnBo0Yme4=
 github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835 h1:roDmqJ4Qes7hrDOsWsMCce0vQHz3xiMPjJ9m4c2eeNs=
+github.com/fzipp/gocyclo v0.0.0-20150627053110-6acd4345c835/go.mod h1:BjL/N0+C+j9uNX+1xcNuM9vdSIcXCZrQZUYbXOFbgN8=
 github.com/go-chi/chi v4.0.2+incompatible h1:maB6vn6FqCxrpz4FqWdh4+lwpyZIQS7YEAUcHlgXVRs=
 github.com/go-chi/chi v4.0.2+incompatible/go.mod h1:eB3wogJHnLi3x/kFX2A+IbTBlXxmMeXJVKy9tTv1XzQ=
 github.com/go-chi/render v1.0.1 h1:4/5tis2cKaNdnv9zFLfXzcquC9HbeZgCnxGnKrltBS8=

sftpd/handler.go

@@ -106,21 +106,12 @@ func (c Connection) Filewrite(request *sftp.Request) (io.WriterAt, error) {
 			}
 		}
 
-		dirsToCreate, err := c.findNonexistentDirs(p)
+		err = c.createMissingDirs(p)
 		if err != nil {
+			logger.Error(logSender, "error making missing dir for path %v: %v", p, err)
 			return nil, sftp.ErrSshFxFailure
 		}
 
-		last := len(dirsToCreate) - 1
-		for i := range dirsToCreate {
-			d := dirsToCreate[last-i]
-			if err := os.Mkdir(d, 0777); err != nil {
-				logger.Error(logSender, "error making path for file, dir: %v, path: %v", d, p)
-				return nil, sftp.ErrSshFxFailure
-			}
-			utils.SetPathPermissions(d, c.User.GetUID(), c.User.GetGID())
-		}
-
 		file, err := os.Create(p)
 		if err != nil {
 			logger.Error(logSender, "error creating file %v: %v", p, err)
@@ -162,24 +153,7 @@ func (c Connection) Filewrite(request *sftp.Request) (io.WriterAt, error) {
 		return nil, sftp.ErrSshFxOpUnsupported
 	}
 
-	var osFlags int
-	trunc := false
-	sftpFileOpenFlags := request.Pflags()
-	if sftpFileOpenFlags.Read && sftpFileOpenFlags.Write {
-		osFlags |= os.O_RDWR
-	} else if sftpFileOpenFlags.Write {
-		osFlags |= os.O_WRONLY
-	}
-	if sftpFileOpenFlags.Append {
-		osFlags |= os.O_APPEND
-	}
-	if sftpFileOpenFlags.Creat {
-		osFlags |= os.O_CREATE
-	}
-	if sftpFileOpenFlags.Trunc {
-		osFlags |= os.O_TRUNC
-		trunc = true
-	}
+	osFlags, trunc := getOSOpenFlags(request.Pflags())
 
 	if !trunc {
 		// see https://github.com/pkg/sftp/issues/295
@@ -228,109 +202,44 @@ func (c Connection) Filecmd(request *sftp.Request) error {
 		return sftp.ErrSshFxNoSuchFile
 	}
 
-	var target string
-	// If a target is provided in this request validate that it is going to the correct
-	// location for the server. If it is not, return an operation unsupported error. This
-	// is maybe not the best error response, but its not wrong either.
-	if request.Target != "" {
-		target, err = c.buildPath(request.Target)
-		if err != nil {
-			return sftp.ErrSshFxOpUnsupported
-		}
+	target, err := c.getSFTPCmdTargetPath(request.Target)
+	if err != nil {
+		return sftp.ErrSshFxOpUnsupported
 	}
 
-	logger.Debug(logSender, "new cmd, method: %v user: %v", request.Method, c.User.Username)
+	logger.Debug(logSender, "new cmd, method: %v user: %v sourcePath: %v, targetPath: %v", request.Method, c.User.Username,
+		p, target)
 
 	switch request.Method {
 	case "Setstat":
 		return nil
 	case "Rename":
-		if !c.User.HasPerm(dataprovider.PermRename) {
-			return sftp.ErrSshFxPermissionDenied
-		}
-
-		logger.CommandLog(sftpdRenameLogSender, p, target, c.User.Username, c.ID)
-		if err := os.Rename(p, target); err != nil {
-			logger.Error("failed to rename file, source: %v target: %v: %v", p, target, err)
-			return sftp.ErrSshFxFailure
+		err = c.handleSFTPRename(p, target)
+		if err != nil {
+			return err
 		}
 
 		break
 	case "Rmdir":
-		if !c.User.HasPerm(dataprovider.PermDelete) {
-			return sftp.ErrSshFxPermissionDenied
-		}
+		return c.handleSFTPRmdir(p)
 
-		logger.CommandLog(sftpdRmdirLogSender, p, target, c.User.Username, c.ID)
-		numFiles, size, err := utils.ScanDirContents(p)
-		if err != nil {
-			logger.Error("failed to remove directory %v, scanning error: %v", p, err)
-			return sftp.ErrSshFxFailure
-		}
-		if err := os.RemoveAll(p); err != nil {
-			logger.Error("failed to remove directory %v: %v", p, err)
-			return sftp.ErrSshFxFailure
-		}
-
-		dataprovider.UpdateUserQuota(dataProvider, c.User.Username, -numFiles, -size, false)
-
-		return sftp.ErrSshFxOk
 	case "Mkdir":
-		if !c.User.HasPerm(dataprovider.PermCreateDirs) {
-			return sftp.ErrSshFxPermissionDenied
-		}
-
-		logger.CommandLog(sftpdMkdirLogSender, p, target, c.User.Username, c.ID)
-		dirsToCreate, err := c.findNonexistentDirs(filepath.Join(p, "testfile"))
+		err = c.handleSFTPMkdir(p)
 		if err != nil {
-			return sftp.ErrSshFxFailure
+			return err
 		}
 
-		last := len(dirsToCreate) - 1
-		for i := range dirsToCreate {
-			d := dirsToCreate[last-i]
-			if err := os.Mkdir(d, 0777); err != nil {
-				logger.Error(logSender, "error making path dir: %v, full path: %v", d, p)
-				return sftp.ErrSshFxFailure
-			}
-			utils.SetPathPermissions(d, c.User.GetUID(), c.User.GetGID())
-		}
 		break
 	case "Symlink":
-		if !c.User.HasPerm(dataprovider.PermCreateSymlinks) {
-			return sftp.ErrSshFxPermissionDenied
-		}
-
-		logger.CommandLog(sftpdSymlinkLogSender, p, target, c.User.Username, c.ID)
-		if err := os.Symlink(p, target); err != nil {
-			logger.Warn("failed to create symlink %v->%v: %v", p, target, err)
-			return sftp.ErrSshFxFailure
+		err = c.handleSFTPSymlink(p, target)
+		if err != nil {
+			return err
 		}
 
 		break
 	case "Remove":
-		if !c.User.HasPerm(dataprovider.PermDelete) {
-			return sftp.ErrSshFxPermissionDenied
-		}
+		return c.handleSFTPRemove(p)
 
-		logger.CommandLog(sftpdRemoveLogSender, p, target, c.User.Username, c.ID)
-		var size int64
-		var fi os.FileInfo
-		if fi, err = os.Lstat(p); err != nil {
-			logger.Error(logSender, "failed to remove a file %v: stat error: %v", p, err)
-			return sftp.ErrSshFxFailure
-		}
-		size = fi.Size()
-		if err := os.Remove(p); err != nil {
-			logger.Error(logSender, "failed to remove a file %v: %v", p, err)
-			return sftp.ErrSshFxFailure
-		}
-
-		if fi.Mode()&os.ModeSymlink != os.ModeSymlink {
-			dataprovider.UpdateUserQuota(dataProvider, c.User.Username, -1, -size, false)
-		}
-
-		return sftp.ErrSshFxOk
 	default:
 		return sftp.ErrSshFxOpUnsupported
 	}
@@ -340,6 +249,7 @@ func (c Connection) Filecmd(request *sftp.Request) error {
 		fileLocation = target
 	}
 
+	// we return if we remove a file or a dir so source path or target path always exists here
 	utils.SetPathPermissions(fileLocation, c.User.GetUID(), c.User.GetGID())
 
 	return sftp.ErrSshFxOk
@@ -389,6 +299,106 @@ func (c Connection) Filelist(request *sftp.Request) (sftp.ListerAt, error) {
 	}
 }
 
+func (c Connection) getSFTPCmdTargetPath(requestTarget string) (string, error) {
+	var target string
+	// If a target is provided in this request validate that it is going to the correct
+	// location for the server. If it is not, return an operation unsupported error. This
+	// is maybe not the best error response, but its not wrong either.
+	if requestTarget != "" {
+		var err error
+		target, err = c.buildPath(requestTarget)
+		if err != nil {
+			return target, sftp.ErrSshFxOpUnsupported
+		}
+	}
+	return target, nil
+}
+
+func (c Connection) handleSFTPRename(sourcePath string, targetPath string) error {
+	if !c.User.HasPerm(dataprovider.PermRename) {
+		return sftp.ErrSshFxPermissionDenied
+	}
+	if err := os.Rename(sourcePath, targetPath); err != nil {
+		logger.Error(logSender, "failed to rename file, source: %v target: %v: %v", sourcePath, targetPath, err)
+		return sftp.ErrSshFxFailure
+	}
+	logger.CommandLog(sftpdRenameLogSender, sourcePath, targetPath, c.User.Username, c.ID)
+	return nil
+}
+
+func (c Connection) handleSFTPRmdir(path string) error {
+	if !c.User.HasPerm(dataprovider.PermDelete) {
+		return sftp.ErrSshFxPermissionDenied
+	}
+
+	numFiles, size, err := utils.ScanDirContents(path)
+	if err != nil {
+		logger.Error(logSender, "failed to remove directory %v, scanning error: %v", path, err)
+		return sftp.ErrSshFxFailure
+	}
+	if err := os.RemoveAll(path); err != nil {
+		logger.Error(logSender, "failed to remove directory %v: %v", path, err)
+		return sftp.ErrSshFxFailure
+	}
+
+	logger.CommandLog(sftpdRmdirLogSender, path, "", c.User.Username, c.ID)
+	dataprovider.UpdateUserQuota(dataProvider, c.User.Username, -numFiles, -size, false)
+
+	return sftp.ErrSshFxOk
+}
+
+func (c Connection) handleSFTPSymlink(sourcePath string, targetPath string) error {
+	if !c.User.HasPerm(dataprovider.PermCreateSymlinks) {
+		return sftp.ErrSshFxPermissionDenied
+	}
+	if err := os.Symlink(sourcePath, targetPath); err != nil {
+		logger.Warn(logSender, "failed to create symlink %v -> %v: %v", sourcePath, targetPath, err)
+		return sftp.ErrSshFxFailure
+	}
+
+	logger.CommandLog(sftpdSymlinkLogSender, sourcePath, targetPath, c.User.Username, c.ID)
+	return nil
+}
+
+func (c Connection) handleSFTPMkdir(path string) error {
+	if !c.User.HasPerm(dataprovider.PermCreateDirs) {
+		return sftp.ErrSshFxPermissionDenied
+	}
+
+	if err := c.createMissingDirs(filepath.Join(path, "testfile")); err != nil {
+		logger.Error(logSender, "error making missing dir for path %v: %v", path, err)
+		return sftp.ErrSshFxFailure
+	}
+	logger.CommandLog(sftpdMkdirLogSender, path, "", c.User.Username, c.ID)
+	return nil
+}
+
+func (c Connection) handleSFTPRemove(path string) error {
+	if !c.User.HasPerm(dataprovider.PermDelete) {
+		return sftp.ErrSshFxPermissionDenied
+	}
+
+	var size int64
+	var fi os.FileInfo
+	var err error
+	if fi, err = os.Lstat(path); err != nil {
+		logger.Error(logSender, "failed to remove a file %v: stat error: %v", path, err)
+		return sftp.ErrSshFxFailure
+	}
+	size = fi.Size()
+	if err := os.Remove(path); err != nil {
+		logger.Error(logSender, "failed to remove a file/symlink %v: %v", path, err)
+		return sftp.ErrSshFxFailure
+	}
+
+	logger.CommandLog(sftpdRemoveLogSender, path, "", c.User.Username, c.ID)
+	if fi.Mode()&os.ModeSymlink != os.ModeSymlink {
+		dataprovider.UpdateUserQuota(dataProvider, c.User.Username, -1, -size, false)
+	}
+
+	return sftp.ErrSshFxOk
+}
+
 func (c Connection) hasSpace(checkFiles bool) bool {
 	if (checkFiles && c.User.QuotaFiles > 0) || c.User.QuotaSize > 0 {
 		numFile, size, err := dataprovider.GetUsedQuota(c.dataProvider, c.User.Username)
@@ -505,3 +515,44 @@ func (c Connection) isSubDir(sub string) error {
 	}
 	return nil
 }
+
+func (c Connection) createMissingDirs(filePath string) error {
+	dirsToCreate, err := c.findNonexistentDirs(filePath)
+	if err != nil {
+		return err
+	}
+	last := len(dirsToCreate) - 1
+	for i := range dirsToCreate {
+		d := dirsToCreate[last-i]
+		if err := os.Mkdir(d, 0777); err != nil {
+			logger.Error(logSender, "error creating missing dir: %v", d)
+			return err
+		}
+		utils.SetPathPermissions(d, c.User.GetUID(), c.User.GetGID())
+	}
+	return nil
+}
+
+func getOSOpenFlags(requestFlags sftp.FileOpenFlags) (flags int, trunc bool) {
+	var osFlags int
+	truncateFile := false
+	if requestFlags.Read && requestFlags.Write {
+		osFlags |= os.O_RDWR
+	} else if requestFlags.Write {
+		osFlags |= os.O_WRONLY
+	}
+	if requestFlags.Append {
+		osFlags |= os.O_APPEND
+	}
+	if requestFlags.Creat {
+		osFlags |= os.O_CREATE
+	}
+	if requestFlags.Trunc {
+		osFlags |= os.O_TRUNC
+		truncateFile = true
+	}
+	if requestFlags.Excl {
+		osFlags |= os.O_EXCL
+	}
+	return osFlags, truncateFile
+}

sftpd/sftpd_test.go

@@ -23,7 +23,7 @@ import (
 
 const (
 	sftpServerAddr  = "127.0.0.1:2022"
-	defaultUsername = "test_user"
+	defaultUsername = "test_user_sftp"
 	defaultPassword = "test_password"
 	testPubKey      = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC03jj0D+djk7pxIf/0OhrxrchJTRZklofJ1NoIu4752Sq02mdXmarMVsqJ1cAjV5LBVy3D1F5U6XW4rppkXeVtd04Pxb09ehtH0pRRPaoHHlALiJt8CoMpbKYMA8b3KXPPriGxgGomvtU2T2RMURSwOZbMtpsugfjYSWenyYX+VORYhylWnSXL961LTyC21ehd6d6QnW9G7E5hYMITMY9TuQZz3bROYzXiTsgN0+g6Hn7exFQp50p45StUMfV/SftCMdCxlxuyGny2CrN/vfjO7xxOo2uv7q1qm10Q46KPWJQv+pgZ/OfL+EDjy07n5QVSKHlbx+2nT4Q0EgOSQaCTYwn3YjtABfIxWwgAFdyj6YlPulCL22qU4MYhDcA6PSBwDdf8hvxBfvsiHdM+JcSHvv8/VeJhk6CmnZxGY0fxBupov27z3yEO8nAg8k+6PaUiW1MSUfuGMF/ktB8LOstXsEPXSszuyXiOv4DaryOXUiSn7bmRqKcEFlJusO6aZP0= nicola@p1"
 	testPrivateKey  = `-----BEGIN OPENSSH PRIVATE KEY-----
@@ -193,14 +193,6 @@ func TestBasicSFTPHandling(t *testing.T) {
 		t.Errorf("unable to create sftp client: %v", err)
 	} else {
 		defer client.Close()
-		_, err := client.Getwd()
-		if err != nil {
-			t.Errorf("unable to get working dir: %v", err)
-		}
-		_, err = client.ReadDir(".")
-		if err != nil {
-			t.Errorf("unable to read remote dir: %v", err)
-		}
 		testFileName := "test_file.dat"
 		testFilePath := filepath.Join(homeBasePath, testFileName)
 		testFileSize := int64(65535)
@@ -214,14 +206,6 @@ func TestBasicSFTPHandling(t *testing.T) {
 		if err != nil {
 			t.Errorf("file upload error: %v", err)
 		}
-		err = client.Symlink(testFileName, testFileName+".link")
-		if err != nil {
-			t.Errorf("error creating symlink: %v", err)
-		}
-		err = client.Remove(testFileName + ".link")
-		if err != nil {
-			t.Errorf("error removing symlink: %v", err)
-		}
 		localDownloadPath := filepath.Join(homeBasePath, "test_download.dat")
 		err = sftpDownloadFile(testFileName, localDownloadPath, testFileSize, client)
 		if err != nil {
@@ -255,6 +239,32 @@ func TestBasicSFTPHandling(t *testing.T) {
 		if (expectedQuotaSize - testFileSize) != user.UsedQuotaSize {
 			t.Errorf("quota size does not match, expected: %v, actual: %v", expectedQuotaSize-testFileSize, user.UsedQuotaSize)
 		}
+	}
+	err = api.RemoveUser(user, http.StatusOK)
+	if err != nil {
+		t.Errorf("unable to remove user: %v", err)
+	}
+}
+
+func TestDirCommands(t *testing.T) {
+	usePubKey := false
+	user, err := api.AddUser(getTestUser(usePubKey), http.StatusOK)
+	if err != nil {
+		t.Errorf("unable to add user: %v", err)
+	}
+	client, err := getSftpClient(user, usePubKey)
+	if err != nil {
+		t.Errorf("unable to create sftp client: %v", err)
+	} else {
+		defer client.Close()
+		_, err := client.Getwd()
+		if err != nil {
+			t.Errorf("unable to get working dir: %v", err)
+		}
+		_, err = client.ReadDir(".")
+		if err != nil {
+			t.Errorf("unable to read remote dir: %v", err)
+		}
 		err = client.Mkdir("test")
 		if err != nil {
 			t.Errorf("error mkdir: %v", err)
@@ -286,6 +296,47 @@ func TestBasicSFTPHandling(t *testing.T) {
 	}
 }
 
+func TestSymlink(t *testing.T) {
+	usePubKey := false
+	user, err := api.AddUser(getTestUser(usePubKey), http.StatusOK)
+	if err != nil {
+		t.Errorf("unable to add user: %v", err)
+	}
+	client, err := getSftpClient(user, usePubKey)
+	if err != nil {
+		t.Errorf("unable to create sftp client: %v", err)
+	} else {
+		defer client.Close()
+		testFileName := "test_file.dat"
+		testFilePath := filepath.Join(homeBasePath, testFileName)
+		testFileSize := int64(65535)
+		err = createTestFile(testFilePath, testFileSize)
+		if err != nil {
+			t.Errorf("unable to create test file: %v", err)
+		}
+		err = sftpUploadFile(testFilePath, testFileName, testFileSize, client)
+		if err != nil {
+			t.Errorf("file upload error: %v", err)
+		}
+		err = client.Symlink(testFileName, testFileName+".link")
+		if err != nil {
+			t.Errorf("error creating symlink: %v", err)
+		}
+		err = client.Remove(testFileName + ".link")
+		if err != nil {
+			t.Errorf("error removing symlink: %v", err)
+		}
+		err = client.Remove(testFileName)
+		if err != nil {
+			t.Errorf("error removing uploaded file: %v", err)
+		}
+	}
+	err = api.RemoveUser(user, http.StatusOK)
+	if err != nil {
+		t.Errorf("unable to remove user: %v", err)
+	}
+}
+
 // basic tests to verify virtual chroot, should be improved to cover more cases ...
 func TestEscapeHomeDir(t *testing.T) {
 	usePubKey := true