shares: add permission to deny sharing without password

Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
2022-02-19 13:31:58 +01:00 · 2022-02-19 13:31:58 +01:00 · c19b03a3f7
commit c19b03a3f7
parent c6b8644828
8 changed files with 177 additions and 18 deletions
--- a/dataprovider/share.go
+++ b/dataprovider/share.go
@ -255,12 +255,15 @@ func (s *Share) validate() error {
 	return nil
 }

-// CheckPassword verifies the share password if set
-func (s *Share) CheckPassword(password string) (bool, error) {
+// CheckCredentials verifies the share credentials if a password if set
+func (s *Share) CheckCredentials(username, password string) (bool, error) {
 	if s.Password == "" {
 		return true, nil
 	}
-	if password == "" {
+	if username == "" || password == "" {
+		return false, ErrInvalidCredentials
+	}
+	if username != s.Username {
 		return false, ErrInvalidCredentials
 	}
 	if strings.HasPrefix(s.Password, bcryptPwdPrefix) {
--- a/docs/custom-actions.md
+++ b/docs/custom-actions.md
@ -43,7 +43,7 @@ If the `hook` defines a path to an external program, then this program can read
 - `SFTPGO_ACTION_BUCKET`, non-empty for S3, GCS and Azure backends
 - `SFTPGO_ACTION_ENDPOINT`, non-empty for S3, SFTP and Azure backend if configured
 - `SFTPGO_ACTION_STATUS`, integer. Status for `upload`, `download` and `ssh_cmd` actions. 1 means no error, 2 means a generic error occurred, 3 means quota exceeded error
- `SFTPGO_ACTION_PROTOCOL`, string. Possible values are `SSH`, `SFTP`, `SCP`, `FTP`, `DAV`, `HTTP`, `HTTPShare`, `DataRetention`
+- `SFTPGO_ACTION_PROTOCOL`, string. Possible values are `SSH`, `SFTP`, `SCP`, `FTP`, `DAV`, `HTTP`, `HTTPShare`, `OIDC`, `DataRetention`
 - `SFTPGO_ACTION_IP`, the action was executed from this IP address
 - `SFTPGO_ACTION_SESSION_ID`, string. Unique protocol session identifier. For stateless protocols such as HTTP the session id will change for each request
 - `SFTPGO_ACTION_OPEN_FLAGS`, integer. File open flags, can be non-zero for `pre-upload` action. If `SFTPGO_ACTION_FILE_SIZE` is greater than zero and `SFTPGO_ACTION_OPEN_FLAGS&512 == 0` the target file will not be truncated
@ -66,7 +66,7 @@ If the `hook` defines an HTTP URL then this URL will be invoked as HTTP POST. Th
 - `bucket`, string, inlcuded for S3, GCS and Azure backends
 - `endpoint`, string, included for S3, SFTP and Azure backend if configured
 - `status`, integer. Status for `upload`, `download` and `ssh_cmd` actions. 1 means no error, 2 means a generic error occurred, 3 means quota exceeded error
- `protocol`, string. Possible values are `SSH`, `SFTP`, `SCP`, `FTP`, `DAV`, `HTTP`, `HTTPShare`, `DataRetention`
+- `protocol`, string. Possible values are `SSH`, `SFTP`, `SCP`, `FTP`, `DAV`, `HTTP`, `HTTPShare`, `OIDC`, `DataRetention`
 - `ip`, string. The action was executed from this IP address
 - `session_id`, string. Unique protocol session identifier. For stateless protocols such as HTTP the session id will change for each request
 - `open_flags`, integer. File open flags, can be non-zero for `pre-upload` action. If `file_size` is greater than zero and `file_size&512 == 0` the target file will not be truncated
--- a/go.mod
+++ b/go.mod
@ -40,7 +40,7 @@ require (
 	github.com/rs/cors v1.8.2
 	github.com/rs/xid v1.3.0
 	github.com/rs/zerolog v1.26.2-0.20220203140311-fc26014bd4e1
-	github.com/sftpgo/sdk v0.0.0-20220201111021-563c373f8012
+	github.com/sftpgo/sdk v0.1.1-0.20220219112139-4616f3c10321
 	github.com/shirou/gopsutil/v3 v3.22.1
 	github.com/spf13/afero v1.8.1
 	github.com/spf13/cobra v1.3.0
--- a/go.sum
+++ b/go.sum
@ -695,8 +695,8 @@ github.com/satori/go.uuid v1.2.0/go.mod h1:dA0hQrYB0VpLJoorglMZABFdXlWrHn1NEOzdh
 github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc=
 github.com/secsy/goftp v0.0.0-20200609142545-aa2de14babf4 h1:PT+ElG/UUFMfqy5HrxJxNzj3QBOf7dZwupeVC+mG1Lo=
 github.com/secsy/goftp v0.0.0-20200609142545-aa2de14babf4/go.mod h1:MnkX001NG75g3p8bhFycnyIjeQoOjGL6CEIsdE/nKSY=
-github.com/sftpgo/sdk v0.0.0-20220201111021-563c373f8012 h1:tkzS0kxhatqIVrWZePzsFlp1xQgR9q6Wt0UYKsBiCUU=
-github.com/sftpgo/sdk v0.0.0-20220201111021-563c373f8012/go.mod h1:gcYbk4z578GfwbC9kJOz2rltYoPYUIcGZgV13r74MJw=
+github.com/sftpgo/sdk v0.1.1-0.20220219112139-4616f3c10321 h1:woOiGu0/qrh2nzCQLlX7k3VK2s+kB+wIGaS/jh40/9o=
+github.com/sftpgo/sdk v0.1.1-0.20220219112139-4616f3c10321/go.mod h1:zqCRMcwS28IViwekJHNkFu4GqSfyVmOQTlh8h3icAXE=
 github.com/shirou/gopsutil/v3 v3.22.1 h1:33y31Q8J32+KstqPfscvFwBlNJ6xLaBy4xqBXzlYV5w=
 github.com/shirou/gopsutil/v3 v3.22.1/go.mod h1:WapW1AOOPlHyXr+yOyw3uYx36enocrtSoSBy0L5vUHY=
 github.com/shopspring/decimal v0.0.0-20180709203117-cd690d0c9e24/go.mod h1:M+9NzErvs504Cn4c5DxATwIqPbtswREoFCre64PpcG4=
--- a/httpd/api_shares.go
+++ b/httpd/api_shares.go
@ -11,6 +11,7 @@ import (

 	"github.com/go-chi/render"
 	"github.com/rs/xid"
+	"github.com/sftpgo/sdk"

 	"github.com/drakkan/sftpgo/v2/common"
 	"github.com/drakkan/sftpgo/v2/dataprovider"
@ -76,6 +77,13 @@ func addShare(w http.ResponseWriter, r *http.Request) {
 	if share.Name == "" {
 		share.Name = share.ShareID
 	}
+	if share.Password == "" {
+		if util.IsStringInSlice(sdk.WebClientShareNoPasswordDisabled, claims.Permissions) {
+			sendAPIResponse(w, r, nil, "You are not authorized to share files/folders without a password",
+				http.StatusForbidden)
+			return
+		}
+	}
 	err = dataprovider.AddShare(&share, claims.Username, util.GetIPFromRemoteAddress(r.RemoteAddr))
 	if err != nil {
 		sendAPIResponse(w, r, err, "", getRespStatus(err))
@ -112,6 +120,13 @@ func updateShare(w http.ResponseWriter, r *http.Request) {
 	if share.Password == redactedSecret {
 		share.Password = oldPassword
 	}
+	if share.Password == "" {
+		if util.IsStringInSlice(sdk.WebClientShareNoPasswordDisabled, claims.Permissions) {
+			sendAPIResponse(w, r, nil, "You are not authorized to share files/folders without a password",
+				http.StatusForbidden)
+			return
+		}
+	}
 	if err := dataprovider.UpdateShare(&share, claims.Username, util.GetIPFromRemoteAddress(r.RemoteAddr)); err != nil {
 		sendAPIResponse(w, r, err, "", getRespStatus(err))
 		return
@ -363,13 +378,13 @@ func checkPublicShare(w http.ResponseWriter, r *http.Request, shareShope datapro
 		return share, nil, err
 	}
 	if share.Password != "" {
-		_, password, ok := r.BasicAuth()
+		username, password, ok := r.BasicAuth()
 		if !ok {
 			w.Header().Set(common.HTTPAuthenticationHeader, basicRealm)
 			renderError(dataprovider.ErrInvalidCredentials, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
 			return share, nil, dataprovider.ErrInvalidCredentials
 		}
-		match, err := share.CheckPassword(password)
+		match, err := share.CheckCredentials(username, password)
 		if !match || err != nil {
 			w.Header().Set(common.HTTPAuthenticationHeader, basicRealm)
 			renderError(dataprovider.ErrInvalidCredentials, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
--- a/httpd/httpd_test.go
+++ b/httpd/httpd_test.go
@ -10086,7 +10086,7 @@ func TestUserAPIShares(t *testing.T) {
 	token1, err := getJWTAPIUserTokenFromTestServer(user1.Username, defaultPassword)
 	assert.NoError(t, err)

-	// the share username will be set from
+	// the share username will be set from the claims
 	share := dataprovider.Share{
 		Name:        "share1",
 		Description: "description1",
@ -10145,10 +10145,13 @@ func TestUserAPIShares(t *testing.T) {

 	s, err := dataprovider.ShareExists(objectID, defaultUsername)
 	assert.NoError(t, err)
-	match, err := s.CheckPassword(defaultPassword)
+	match, err := s.CheckCredentials(defaultUsername, defaultPassword)
 	assert.True(t, match)
 	assert.NoError(t, err)
-	match, err = s.CheckPassword(defaultPassword + "mod")
+	match, err = s.CheckCredentials(defaultUsername, defaultPassword+"mod")
+	assert.False(t, match)
+	assert.Error(t, err)
+	match, err = s.CheckCredentials(altAdminUsername, defaultPassword)
 	assert.False(t, match)
 	assert.Error(t, err)

@ -10163,10 +10166,10 @@ func TestUserAPIShares(t *testing.T) {

 	s, err = dataprovider.ShareExists(objectID, defaultUsername)
 	assert.NoError(t, err)
-	match, err = s.CheckPassword(defaultPassword)
+	match, err = s.CheckCredentials(defaultUsername, defaultPassword)
 	assert.True(t, match)
 	assert.NoError(t, err)
-	match, err = s.CheckPassword(defaultPassword + "mod")
+	match, err = s.CheckCredentials(defaultUsername, defaultPassword+"mod")
 	assert.False(t, match)
 	assert.Error(t, err)

@ -10283,6 +10286,58 @@ func TestUserAPIShares(t *testing.T) {
 	assert.NoError(t, err)
 }

+func TestUsersAPISharesNoPasswordDisabled(t *testing.T) {
+	u := getTestUser()
+	u.Filters.WebClient = []string{sdk.WebClientShareNoPasswordDisabled}
+	user, _, err := httpdtest.AddUser(u, http.StatusCreated)
+	assert.NoError(t, err)
+	token, err := getJWTAPIUserTokenFromTestServer(defaultUsername, defaultPassword)
+	assert.NoError(t, err)
+
+	share := dataprovider.Share{
+		Name:  "s",
+		Scope: dataprovider.ShareScopeRead,
+		Paths: []string{"/"},
+	}
+	asJSON, err := json.Marshal(share)
+	assert.NoError(t, err)
+	req, err := http.NewRequest(http.MethodPost, userSharesPath, bytes.NewBuffer(asJSON))
+	assert.NoError(t, err)
+	setBearerForReq(req, token)
+	rr := executeRequest(req)
+	checkResponseCode(t, http.StatusForbidden, rr)
+	assert.Contains(t, rr.Body.String(), "You are not authorized to share files/folders without a password")
+
+	share.Password = defaultPassword
+	asJSON, err = json.Marshal(share)
+	assert.NoError(t, err)
+	req, err = http.NewRequest(http.MethodPost, userSharesPath, bytes.NewBuffer(asJSON))
+	assert.NoError(t, err)
+	setBearerForReq(req, token)
+	rr = executeRequest(req)
+	checkResponseCode(t, http.StatusCreated, rr)
+	location := rr.Header().Get("Location")
+	assert.NotEmpty(t, location)
+	objectID := rr.Header().Get("X-Object-ID")
+	assert.NotEmpty(t, objectID)
+	assert.Equal(t, fmt.Sprintf("%v/%v", userSharesPath, objectID), location)
+
+	share.Password = ""
+	asJSON, err = json.Marshal(share)
+	assert.NoError(t, err)
+	req, err = http.NewRequest(http.MethodPut, location, bytes.NewBuffer(asJSON))
+	assert.NoError(t, err)
+	setBearerForReq(req, token)
+	rr = executeRequest(req)
+	checkResponseCode(t, http.StatusForbidden, rr)
+	assert.Contains(t, rr.Body.String(), "You are not authorized to share files/folders without a password")
+
+	_, err = httpdtest.RemoveUser(user, http.StatusOK)
+	assert.NoError(t, err)
+	err = os.RemoveAll(user.GetHomeDir())
+	assert.NoError(t, err)
+}
+
 func TestUserAPIKey(t *testing.T) {
 	u := getTestUser()
 	u.Filters.AllowAPIKeyAuth = true
@ -12519,7 +12574,7 @@ func TestWebUserShare(t *testing.T) {
 	// check the password
 	s, err := dataprovider.ShareExists(share.ShareID, user.Username)
 	assert.NoError(t, err)
-	match, err := s.CheckPassword(defaultPassword)
+	match, err := s.CheckCredentials(user.Username, defaultPassword)
 	assert.NoError(t, err)
 	assert.True(t, match)

@ -12566,6 +12621,76 @@ func TestWebUserShare(t *testing.T) {
 	assert.NoError(t, err)
 }

+func TestWebUserShareNoPasswordDisabled(t *testing.T) {
+	u := getTestUser()
+	u.Filters.WebClient = []string{sdk.WebClientShareNoPasswordDisabled}
+	user, _, err := httpdtest.AddUser(u, http.StatusCreated)
+	assert.NoError(t, err)
+	csrfToken, err := getCSRFToken(httpBaseURL + webClientLoginPath)
+	assert.NoError(t, err)
+	token, err := getJWTWebClientTokenFromTestServer(defaultUsername, defaultPassword)
+	assert.NoError(t, err)
+	userAPItoken, err := getJWTAPIUserTokenFromTestServer(defaultUsername, defaultPassword)
+	assert.NoError(t, err)
+
+	share := dataprovider.Share{
+		Name:  "s",
+		Scope: dataprovider.ShareScopeRead,
+		Paths: []string{"/"},
+	}
+	form := make(url.Values)
+	form.Set("name", share.Name)
+	form.Set("scope", strconv.Itoa(int(share.Scope)))
+	form.Set("paths", "/")
+	form.Set("max_tokens", "0")
+	form.Set(csrfFormToken, csrfToken)
+	req, err := http.NewRequest(http.MethodPost, webClientSharePath, bytes.NewBuffer([]byte(form.Encode())))
+	assert.NoError(t, err)
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	setJWTCookieForReq(req, token)
+	rr := executeRequest(req)
+	checkResponseCode(t, http.StatusForbidden, rr)
+	assert.Contains(t, rr.Body.String(), "You are not authorized to share files/folders without a password")
+
+	form.Set("password", defaultPassword)
+	req, err = http.NewRequest(http.MethodPost, webClientSharePath, bytes.NewBuffer([]byte(form.Encode())))
+	assert.NoError(t, err)
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	setJWTCookieForReq(req, token)
+	rr = executeRequest(req)
+	checkResponseCode(t, http.StatusSeeOther, rr)
+
+	req, err = http.NewRequest(http.MethodGet, userSharesPath, nil)
+	assert.NoError(t, err)
+	setBearerForReq(req, userAPItoken)
+	rr = executeRequest(req)
+	checkResponseCode(t, http.StatusOK, rr)
+	var shares []dataprovider.Share
+	err = json.Unmarshal(rr.Body.Bytes(), &shares)
+	assert.NoError(t, err)
+	if assert.Len(t, shares, 1) {
+		s := shares[0]
+		assert.Equal(t, share.Name, s.Name)
+		assert.Equal(t, share.Scope, s.Scope)
+		assert.Equal(t, share.Paths, s.Paths)
+		share.ShareID = s.ShareID
+	}
+	assert.NotEmpty(t, share.ShareID)
+	form.Set("password", "")
+	req, err = http.NewRequest(http.MethodPost, webClientSharePath+"/"+share.ShareID, bytes.NewBuffer([]byte(form.Encode())))
+	assert.NoError(t, err)
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	setJWTCookieForReq(req, token)
+	rr = executeRequest(req)
+	checkResponseCode(t, http.StatusForbidden, rr)
+	assert.Contains(t, rr.Body.String(), "You are not authorized to share files/folders without a password")
+
+	err = os.RemoveAll(user.GetHomeDir())
+	assert.NoError(t, err)
+	_, err = httpdtest.RemoveUser(user, http.StatusOK)
+	assert.NoError(t, err)
+}
+
 func TestWebUserProfile(t *testing.T) {
 	user, _, err := httpdtest.AddUser(getTestUser(), http.StatusCreated)
 	assert.NoError(t, err)
--- a/httpd/webclient.go
+++ b/httpd/webclient.go
@ -982,6 +982,12 @@ func handleClientAddSharePost(w http.ResponseWriter, r *http.Request) {
 	share.ShareID = util.GenerateUniqueID()
 	share.LastUseAt = 0
 	share.Username = claims.Username
+	if share.Password == "" {
+		if util.IsStringInSlice(sdk.WebClientShareNoPasswordDisabled, claims.Permissions) {
+			renderClientForbiddenPage(w, r, "You are not authorized to share files/folders without a password")
+			return
+		}
+	}
 	err = dataprovider.AddShare(share, claims.Username, util.GetIPFromRemoteAddress(r.RemoteAddr))
 	if err == nil {
 		http.Redirect(w, r, webClientSharesPath, http.StatusSeeOther)
@ -1020,6 +1026,12 @@ func handleClientUpdateSharePost(w http.ResponseWriter, r *http.Request) {
 	if updatedShare.Password == redactedSecret {
 		updatedShare.Password = share.Password
 	}
+	if updatedShare.Password == "" {
+		if util.IsStringInSlice(sdk.WebClientShareNoPasswordDisabled, claims.Permissions) {
+			renderClientForbiddenPage(w, r, "You are not authorized to share files/folders without a password")
+			return
+		}
+	}
 	err = dataprovider.UpdateShare(updatedShare, claims.Username, util.GetIPFromRemoteAddress(r.RemoteAddr))
 	if err == nil {
 		http.Redirect(w, r, webClientSharesPath, http.StatusSeeOther)
--- a/openapi/openapi.yaml
+++ b/openapi/openapi.yaml
@ -4355,6 +4355,7 @@ components:
        - FTP
        - DAV
        - HTTP
+        - HTTPShare
        - DataRetention
        - OIDC
      description: |
@ -4364,6 +4365,7 @@ components:
          * `FTP` - plain FTP and FTPES/FTPS
          * `DAV` - WebDAV
          * `HTTP` - WebClient/REST API
+          * `HTTPShare` - the event is generated in a public share
          * `DataRetention` - the event is generated by a data retention check
          * `OIDC` - OpenID Connect
    WebClientOptions:
@ -4377,6 +4379,7 @@ components:
        - info-change-disabled
        - shares-disabled
        - password-reset-disabled
+        - shares-without-password-disabled
      description: |
        Options:
          * `publickey-change-disabled` - changing SSH public keys is not allowed
@ -4385,8 +4388,9 @@ components:
          * `password-change-disabled` - changing password is not allowed
          * `api-key-auth-change-disabled` - enabling/disabling API key authentication is not allowed
          * `info-change-disabled` - changing info such as email and description is not allowed
-          * `shares-disabled` - sharing files and directories with external users is disabled
-          * `password-reset-disabled` - resetting the password is disabled
+          * `shares-disabled` - sharing files and directories with external users is not allowed
+          * `password-reset-disabled` - resetting the password is not allowed
+          * `shares-without-password-disabled` - creating shares without password protection is not allowed
    RetentionCheckNotification:
      type: string
      enum: