REST API/Web admin: add a parameter to disconnect a user after an update

This way you can force the user to login again and so to use the updated configuration. A deleted user will be automatically disconnected. Fixes drakkan#163 Improved some docs too.
devopsmakers · Sep 1, 2020 · 3925c7f · 3925c7f
1 parent dbed110
commit 3925c7f
Show file tree

Hide file tree

Showing 20 changed files with 270 additions and 110 deletions.
diff --git a/dataprovider/dataprovider.go b/dataprovider/dataprovider.go
@@ -579,7 +579,7 @@ func UpdateUser(user User) error {
 	return err
 }
 
-// DeleteUser deletes an existing SFTP user.
+// DeleteUser deletes an existing SFTPGo user.
 // ManageUsers configuration must be set to 1 to enable this method
 func DeleteUser(user User) error {
 	if config.ManageUsers == 0 {

diff --git a/docs/dynamic-user-mod.md b/docs/dynamic-user-mod.md
@@ -18,7 +18,7 @@ The program must write, on its the standard output:
 If the hook is an HTTP URL then it will be invoked as HTTP POST. The login method, the used protocol and the ip address of the user trying to login are added to the query string, for example `<http_url>?login_method=password&ip=1.2.3.4&protocol=SSH`.
 The request body will contain the user trying to login serialized as JSON. If no modification is needed the HTTP response code must be 204, otherwise the response code must be 200 and the response body a valid SFTPGo user serialized as JSON.
 
-Actions defined for user's updates will not be executed in this case.
+Actions defined for user's updates will not be executed in this case and an already logged in user with the same username will not be disconnected, you have to handle these things yourself.
 
 The JSON response can include only the fields to update instead of the full user. For example, if you want to disable the user, you can return a response like this:
 

diff --git a/docs/external-auth.md b/docs/external-auth.md
@@ -25,7 +25,7 @@ If the hook is an HTTP URL then it will be invoked as HTTP POST. The request bod
 
 If authentication succeed the HTTP response code must be 200 and the response body a valid SFTPGo user serialized as JSON. If the authentication fails the HTTP response code must be != 200 or the response body must be empty.
 
-If the authentication succeeds, the user will be automatically added/updated inside the defined data provider. Actions defined for users added/updated will not be executed in this case.
+If the authentication succeeds, the user will be automatically added/updated inside the defined data provider. Actions defined for users added/updated will not be executed in this case and an already logged in user with the same username will not be disconnected, you have to handle these things yourself.
 
 The program hook must finish within 30 seconds, the HTTP hook timeout will use the global configuration for HTTP clients.
 

diff --git a/docs/full-configuration.md b/docs/full-configuration.md
@@ -103,7 +103,7 @@ The configuration file contains the following sections:
     - `exposed_headers`, list of strings.
     - `allow_credentials` boolean.
     - `max_age`, integer.
-  - `cache` struct containing cache configuration.
+  - `cache` struct containing cache configuration for the authenticated users.
     - `enabled`, boolean, set to true to enable user caching. Default: true.
     - `expiration_time`, integer. Expiration time, in minutes, for the cached users. 0 means unlimited. Default: 0.
     - `max_size`, integer. Maximum number of users to cache. 0 means unlimited. Default: 50.

diff --git a/docs/ssh-commands.md b/docs/ssh-commands.md
@@ -15,7 +15,19 @@ For system commands we have no direct control on file creation/deletion and so t
  For these reasons we should limit system commands usage as much as possible, we currently support the following system commands:
 
 - `git-receive-pack`, `git-upload-pack`, `git-upload-archive`. These commands enable support for Git repositories over SSH. They need to be installed and in your system's `PATH`.
-- `rsync`. The `rsync` command needs to be installed and in your system's `PATH`. We cannot avoid that rsync creates symlinks, so if the user has the permission to create symlinks, we add the option `--safe-links` to the received rsync command if it is not already set. This should prevent creating symlinks that point outside the home dir. If the user cannot create symlinks, we add the option `--munge-links` if it is not already set. This should make symlinks unusable (but manually recoverable).
+- `rsync`. The `rsync` command needs to be installed and in your system's `PATH`.
+
+At least the following permissions are required to be able to run system commands:
+
+- `list`
+- `download`
+- `upload`
+- `create_dirs`
+- `overwrite`
+- `delete`
+
+For `rsync`  we cannot avoid that it creates symlinks so if the `create_symlinks` permission is granted we add the option `--safe-links`, if it is not already set, to the received `rsync` command. This should prevent to create symlinks that point outside the home directory.
+If the user cannot create symlinks we add the option `--munge-links`, if it is not already set, to the received `rsync` command. This should make symlinks unusable (but manually recoverable).
 
 SFTPGo support the following built-in SSH commands:
 

diff --git a/docs/webdav.md b/docs/webdav.md
@@ -9,8 +9,8 @@ If you enable quota support a dataprovider query is required, to update the user
 
 The caching configuration allows to set:
 
-- `expiration_time` in minutes. If a user is cached for more than the specificied minutes it will be removed from the cache and a new dataprovider query will be performed. Please note that the `last_login` field will not be updated and `external_auth_hook`, `pre_login_hook` and `check_password_hook` will not be executed is the user is obtained from the cache.
-- `max_size`. Maximum number of users to cache. When this limit is reached the user with the oldest `expiration_time` is removed from the cache. 0 means no limit however the cache size cannot exceed the number of users so if you have a small number of users you can leave this setting to 0.
+- `expiration_time` in minutes. If a user is cached for more than the specificied minutes it will be removed from the cache and a new dataprovider query will be performed. Please note that the `last_login` field will not be updated and `external_auth_hook`, `pre_login_hook` and `check_password_hook` will not be executed if the user is obtained from the cache.
+- `max_size`. Maximum number of users to cache. When this limit is reached the user with the oldest expiration date will be removed from the cache. 0 means no limit however the cache size cannot exceed the number of users so if you have a small number of users you can leave this setting to 0.
 
 Users are automatically removed from the cache after an update/delete.
 

diff --git a/examples/rest-api-cli/README.md b/examples/rest-api-cli/README.md
@@ -157,6 +157,8 @@ Output:
 }
 ```
 
+You can set the argument `--disconnect` to `1` to disconnect the user, if connected, after a successful update and so force it to login again and to use the new configuration. If this parameter is not specified the user will continue to use the old configuration as long as he is logged in.
+
 ## Get user by id
 
 Command:

diff --git a/examples/rest-api-cli/sftpgo_api_cli.py b/examples/rest-api-cli/sftpgo_api_cli.py
@@ -280,14 +280,15 @@ def updateUser(self, user_id, username='', password='', public_keys='', home_dir
 				s3_key_prefix='', gcs_bucket='', gcs_key_prefix='', gcs_storage_class='', gcs_credentials_file='',
 				gcs_automatic_credentials='automatic', denied_login_methods=[], virtual_folders=[], denied_extensions=[],
 				allowed_extensions=[], s3_upload_part_size=0, s3_upload_concurrency=0, max_upload_file_size=0,
-				denied_protocols=[]):
+				denied_protocols=[], disconnect=0):
 		u = self.buildUserObject(user_id, username, password, public_keys, home_dir, uid, gid, max_sessions,
 			quota_size, quota_files, self.buildPermissions(perms, subdirs_permissions), upload_bandwidth, download_bandwidth,
 			status, expiration_date, allowed_ip, denied_ip, fs_provider, s3_bucket, s3_region, s3_access_key,
 			s3_access_secret, s3_endpoint, s3_storage_class, s3_key_prefix, gcs_bucket, gcs_key_prefix, gcs_storage_class,
 			gcs_credentials_file, gcs_automatic_credentials, denied_login_methods, virtual_folders, denied_extensions,
 			allowed_extensions, s3_upload_part_size, s3_upload_concurrency, max_upload_file_size, denied_protocols)
-		r = requests.put(urlparse.urljoin(self.userPath, 'user/' + str(user_id)), json=u, auth=self.auth, verify=self.verify)
+		r = requests.put(urlparse.urljoin(self.userPath, 'user/' + str(user_id)), params={'disconnect':disconnect},
+						json=u, auth=self.auth, verify=self.verify)
 		self.printResponse(r)
 
 	def deleteUser(self, user_id):
@@ -648,6 +649,11 @@ def addCommonUserArguments(parser):
 
 	parserUpdateUser = subparsers.add_parser('update-user', help='Update an existing user')
 	parserUpdateUser.add_argument('id', type=int, help='User\'s ID to update')
+	parserUpdateUser.add_argument('--disconnect', type=int, choices=[0, 1], default=0,
+							help='0 means the user will not be disconnected and it will continue to use the old ' +
+							'configuration until connected. 1 means the user will be disconnected after a successful ' +
+							'update. It must login again and so it will be forced to use the new configuration. ' +
+							'Default: %(default)s')
 	addCommonUserArguments(parserUpdateUser)
 
 	parserDeleteUser = subparsers.add_parser('delete-user', help='Delete an existing user')
@@ -708,9 +714,10 @@ def addCommonUserArguments(parser):
 	parserLoadData.add_argument('-Q', '--scan-quota', type=int, choices=[0, 1, 2], default=0,
 							help='0 means no quota scan after a user is added/updated. 1 means always scan quota. 2 ' +
 							'means scan quota if the user has quota restrictions. Default: %(default)s')
-	parserLoadData.add_argument('-M', '--mode', type=int, choices=[0, 1], default=0,
+	parserLoadData.add_argument('-M', '--mode', type=int, choices=[0, 1, 2], default=0,
 							help='0 means new users are added, existing users are updated. 1 means new users are added,' +
-							' existing users are not modified. Default: %(default)s')
+							' existing users are not modified. 2 is the same as 0 but if an updated user is connected ' +
+							'it will be disconnected and so forced to use the new configuration Default: %(default)s')
 
 	parserUpdateQuotaUsage = subparsers.add_parser('update-quota-usage', help='Update the user used quota limits')
 	parserUpdateQuotaUsage.add_argument('username', type=str)
@@ -772,7 +779,7 @@ def addCommonUserArguments(parser):
 					args.s3_key_prefix, args.gcs_bucket, args.gcs_key_prefix, args.gcs_storage_class,
 					args.gcs_credentials_file, args.gcs_automatic_credentials, args.denied_login_methods,
 					args.virtual_folders, args.denied_extensions, args.allowed_extensions, args.s3_upload_part_size,
-					args.s3_upload_concurrency, args.max_upload_file_size, args.denied_protocols)
+					args.s3_upload_concurrency, args.max_upload_file_size, args.denied_protocols, args.disconnect)
 	elif args.command == 'delete-user':
 		api.deleteUser(args.id)
 	elif args.command == 'get-users':

diff --git a/ftpd/ftpd_test.go b/ftpd/ftpd_test.go
@@ -456,14 +456,12 @@ func TestMaxSessions(t *testing.T) {
 	assert.NoError(t, err)
 	client, err := getFTPClient(user, true)
 	if assert.NoError(t, err) {
-		defer func() {
-			err := client.Quit()
-			assert.NoError(t, err)
-		}()
 		err = checkBasicFTP(client)
 		assert.NoError(t, err)
 		_, err = getFTPClient(user, false)
 		assert.Error(t, err)
+		err = client.Quit()
+		assert.NoError(t, err)
 	}
 	_, err = httpd.RemoveUser(user, http.StatusOK)
 	assert.NoError(t, err)
@@ -685,7 +683,7 @@ func TestDeniedLoginMethod(t *testing.T) {
 	_, err = getFTPClient(user, false)
 	assert.Error(t, err)
 	user.Filters.DeniedLoginMethods = []string{dataprovider.SSHLoginMethodPublicKey, dataprovider.SSHLoginMethodKeyAndPassword}
-	user, _, err = httpd.UpdateUser(user, http.StatusOK)
+	user, _, err = httpd.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err)
 	client, err := getFTPClient(user, true)
 	if assert.NoError(t, err) {
@@ -708,7 +706,7 @@ func TestDeniedProtocols(t *testing.T) {
 	_, err = getFTPClient(user, false)
 	assert.Error(t, err)
 	user.Filters.DeniedProtocols = []string{common.ProtocolSSH, common.ProtocolWebDAV}
-	user, _, err = httpd.UpdateUser(user, http.StatusOK)
+	user, _, err = httpd.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err)
 	client, err := getFTPClient(user, true)
 	if assert.NoError(t, err) {
@@ -756,7 +754,7 @@ func TestQuotaLimits(t *testing.T) {
 	// test quota size
 	user.QuotaSize = testFileSize - 1
 	user.QuotaFiles = 0
-	user, _, err = httpd.UpdateUser(user, http.StatusOK)
+	user, _, err = httpd.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err)
 	client, err = getFTPClient(user, true)
 	if assert.NoError(t, err) {
@@ -770,7 +768,7 @@ func TestQuotaLimits(t *testing.T) {
 	// now test quota limits while uploading the current file, we have 1 bytes remaining
 	user.QuotaSize = testFileSize + 1
 	user.QuotaFiles = 0
-	user, _, err = httpd.UpdateUser(user, http.StatusOK)
+	user, _, err = httpd.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err)
 	client, err = getFTPClient(user, false)
 	if assert.NoError(t, err) {
@@ -951,17 +949,14 @@ func TestRename(t *testing.T) {
 		assert.NoError(t, err)
 	}
 	user.Permissions[path.Join("/", testDir)] = []string{dataprovider.PermListItems}
-	user, _, err = httpd.UpdateUser(user, http.StatusOK)
+	user, _, err = httpd.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err)
 	client, err = getFTPClient(user, false)
 	if assert.NoError(t, err) {
-		defer func() {
-			err := client.Quit()
-			assert.NoError(t, err)
-		}()
-
 		err = client.Rename(path.Join(testDir, testFileName), testFileName)
 		assert.Error(t, err)
+		err := client.Quit()
+		assert.NoError(t, err)
 	}
 
 	err = os.Remove(testFilePath)
@@ -1028,11 +1023,6 @@ func TestStat(t *testing.T) {
 	assert.NoError(t, err)
 	client, err := getFTPClient(user, false)
 	if assert.NoError(t, err) {
-		defer func() {
-			err := client.Quit()
-			assert.NoError(t, err)
-		}()
-
 		subDir := "subdir"
 		testFilePath := filepath.Join(homeBasePath, testFileName)
 		testFileSize := int64(65535)
@@ -1051,6 +1041,8 @@ func TestStat(t *testing.T) {
 		assert.Error(t, err)
 		_, err = client.FileSize("missing file")
 		assert.Error(t, err)
+		err = client.Quit()
+		assert.NoError(t, err)
 
 		err = os.Remove(testFilePath)
 		assert.NoError(t, err)
@@ -1141,7 +1133,7 @@ func TestAllocate(t *testing.T) {
 		assert.NoError(t, err)
 	}
 	user.QuotaSize = 100
-	user, _, err = httpd.UpdateUser(user, http.StatusOK)
+	user, _, err = httpd.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err)
 	client, err = getFTPClient(user, false)
 	if assert.NoError(t, err) {
@@ -1184,7 +1176,7 @@ func TestAllocate(t *testing.T) {
 
 	user.Filters.MaxUploadFileSize = 100
 	user.QuotaSize = 0
-	user, _, err = httpd.UpdateUser(user, http.StatusOK)
+	user, _, err = httpd.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err)
 	client, err = getFTPClient(user, false)
 	if assert.NoError(t, err) {
@@ -1217,11 +1209,6 @@ func TestChtimes(t *testing.T) {
 	assert.NoError(t, err)
 	client, err := getFTPClient(user, false)
 	if assert.NoError(t, err) {
-		defer func() {
-			err := client.Quit()
-			assert.NoError(t, err)
-		}()
-
 		testFilePath := filepath.Join(homeBasePath, testFileName)
 		testFileSize := int64(65535)
 		err = createTestFile(testFilePath, testFileSize)
@@ -1236,6 +1223,8 @@ func TestChtimes(t *testing.T) {
 		assert.NoError(t, err)
 		assert.Equal(t, ftp.StatusFile, code)
 		assert.Equal(t, fmt.Sprintf("Modify=%v; %v", mtime, testFileName), response)
+		err = client.Quit()
+		assert.NoError(t, err)
 
 		err = os.Remove(testFilePath)
 		assert.NoError(t, err)
@@ -1255,11 +1244,6 @@ func TestChmod(t *testing.T) {
 	assert.NoError(t, err)
 	client, err := getFTPClient(user, true)
 	if assert.NoError(t, err) {
-		defer func() {
-			err := client.Quit()
-			assert.NoError(t, err)
-		}()
-
 		testFilePath := filepath.Join(homeBasePath, testFileName)
 		testFileSize := int64(131072)
 		err = createTestFile(testFilePath, testFileSize)
@@ -1278,6 +1262,8 @@ func TestChmod(t *testing.T) {
 		if assert.NoError(t, err) {
 			assert.Equal(t, os.FileMode(0600), fi.Mode().Perm())
 		}
+		err = client.Quit()
+		assert.NoError(t, err)
 
 		err = os.Remove(testFilePath)
 		assert.NoError(t, err)

diff --git a/httpd/api_maintenance.go b/httpd/api_maintenance.go
@@ -175,6 +175,9 @@ func restoreUsers(users []dataprovider.User, inputFile string, mode, scanQuota i
 			err = dataprovider.UpdateUser(user)
 			user.Password = "[redacted]"
 			logger.Debug(logSender, "", "restoring existing user: %+v, dump file: %#v, error: %v", user, inputFile, err)
+			if mode == 2 && err == nil {
+				disconnectUser(user.Username)
+			}
 		} else {
 			err = dataprovider.AddUser(user)
 			user.Password = "[redacted]"

diff --git a/httpd/api_user.go b/httpd/api_user.go
@@ -2,12 +2,14 @@ package httpd
 
 import (
 	"errors"
+	"fmt"
 	"net/http"
 	"strconv"
 
 	"github.com/go-chi/chi"
 	"github.com/go-chi/render"
 
+	"github.com/drakkan/sftpgo/common"
 	"github.com/drakkan/sftpgo/dataprovider"
 	"github.com/drakkan/sftpgo/utils"
 )
@@ -100,6 +102,15 @@ func updateUser(w http.ResponseWriter, r *http.Request) {
 		sendAPIResponse(w, r, err, "", http.StatusBadRequest)
 		return
 	}
+	disconnect := 0
+	if _, ok := r.URL.Query()["disconnect"]; ok {
+		disconnect, err = strconv.Atoi(r.URL.Query().Get("disconnect"))
+		if err != nil {
+			err = fmt.Errorf("invalid disconnect parameter: %v", err)
+			sendAPIResponse(w, r, err, "", http.StatusBadRequest)
+			return
+		}
+	}
 	user, err := dataprovider.GetUserByID(userID)
 	if err != nil {
 		sendAPIResponse(w, r, err, "", getRespStatus(err))
@@ -136,6 +147,9 @@ func updateUser(w http.ResponseWriter, r *http.Request) {
 		sendAPIResponse(w, r, err, "", getRespStatus(err))
 	} else {
 		sendAPIResponse(w, r, err, "User updated", http.StatusOK)
+		if disconnect == 1 {
+			disconnectUser(user.Username)
+		}
 	}
 }
 
@@ -156,5 +170,14 @@ func deleteUser(w http.ResponseWriter, r *http.Request) {
 		sendAPIResponse(w, r, err, "", http.StatusInternalServerError)
 	} else {
 		sendAPIResponse(w, r, err, "User deleted", http.StatusOK)
+		disconnectUser(user.Username)
+	}
+}
+
+func disconnectUser(username string) {
+	for _, stat := range common.Connections.GetStats() {
+		if stat.Username == username {
+			common.Connections.Close(stat.ConnectionID)
+		}
 	}
 }
diff --git a/httpd/api_utils.go b/httpd/api_utils.go
@@ -119,12 +119,15 @@ func AddUser(user dataprovider.User, expectedStatusCode int) (dataprovider.User,
 }
 
 // UpdateUser updates an existing user and checks the received HTTP Status code against expectedStatusCode.
-func UpdateUser(user dataprovider.User, expectedStatusCode int) (dataprovider.User, []byte, error) {
+func UpdateUser(user dataprovider.User, expectedStatusCode int, disconnect string) (dataprovider.User, []byte, error) {
 	var newUser dataprovider.User
 	var body []byte
+	url, err := addDisconnectQueryParam(buildURLRelativeToBase(userPath, strconv.FormatInt(user.ID, 10)), disconnect)
+	if err != nil {
+		return user, body, err
+	}
 	userAsJSON, _ := json.Marshal(user)
-	resp, err := sendHTTPRequest(http.MethodPut, buildURLRelativeToBase(userPath, strconv.FormatInt(user.ID, 10)),
-		bytes.NewBuffer(userAsJSON), "application/json")
+	resp, err := sendHTTPRequest(http.MethodPut, url.String(), bytes.NewBuffer(userAsJSON), "application/json")
 	if err != nil {
 		return user, body, err
 	}
@@ -839,3 +842,16 @@ func addModeQueryParam(rawurl, mode string) (*url.URL, error) {
 	url.RawQuery = q.Encode()
 	return url, err
 }
+
+func addDisconnectQueryParam(rawurl, disconnect string) (*url.URL, error) {
+	url, err := url.Parse(rawurl)
+	if err != nil {
+		return nil, err
+	}
+	q := url.Query()
+	if len(disconnect) > 0 {
+		q.Add("disconnect", disconnect)
+	}
+	url.RawQuery = q.Encode()
+	return url, err
+}
-Original file line number
+Diff line change
@@ Expand Up / @@ -157,6 +157,8 @@ Output: @@
     }
     ```
+    You can set the argument `--disconnect` to `1` to disconnect the user, if connected, after a successful update and so force it to login again and to use the new configuration. If this parameter is not specified the user will continue to use the old configuration as long as he is logged in.
     ## Get user by id
     Command:
@@ Expand Down @@