Merge pull request cs3org#3716 from C0rby/writeable-share-password

properly enforce the password on writeable public links

changelog/unreleased/public-link-password.md

@@ -2,4 +2,5 @@ Enhancement: Add config option to enforce passwords on public links
 
 Added a new config option to enforce passwords on public links with "Uploader, Editor, Contributor" roles.
 
+https://github.com/cs3org/reva/pull/3716
 https://github.com/cs3org/reva/pull/3698

internal/grpc/services/publicshareprovider/publicshareprovider.go

@@ -143,7 +143,7 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS
 
 	grant := req.GetGrant()
 	if grant != nil && s.conf.WriteableShareMustHavePassword &&
-		publicshare.IsWriteable(grant) && grant.Password == "" {
+		publicshare.IsWriteable(grant.GetPermissions()) && grant.Password == "" {
 		return &link.CreatePublicShareResponse{
 			Status: status.NewInvalid(ctx, "writeable shares must have a password protection"),
 		}, nil
@@ -253,22 +253,21 @@ func (s *service) UpdatePublicShare(ctx context.Context, req *link.UpdatePublicS
 	log := appctx.GetLogger(ctx)
 	log.Info().Str("publicshareprovider", "update").Msg("update public share")
 
-	grant := req.GetUpdate().GetGrant()
-	if grant != nil && s.conf.WriteableShareMustHavePassword &&
-		publicshare.IsWriteable(grant) && grant.Password == "" {
-		return &link.UpdatePublicShareResponse{
-			Status: status.NewInvalid(ctx, "writeable shares must have a password protection"),
-		}, nil
-	}
-
 	u, ok := ctxpkg.ContextGetUser(ctx)
 	if !ok {
 		log.Error().Msg("error getting user from context")
 	}
 
 	updateR, err := s.sm.UpdatePublicShare(ctx, u, req)
 	if err != nil {
-		log.Err(err).Msgf("error updating public shares: %v", err)
+		if errors.Is(err, publicshare.ErrShareNeedsPassword) {
+			return &link.UpdatePublicShareResponse{
+				Status: status.NewInvalid(ctx, err.Error()),
+			}, nil
+		}
+		return &link.UpdatePublicShareResponse{
+			Status: status.NewInternal(ctx, err.Error()),
+		}, nil
 	}
 
 	res := &link.UpdatePublicShareResponse{

pkg/publicshare/manager/json/json.go

@@ -76,7 +76,7 @@ func NewFile(c map[string]interface{}) (publicshare.Manager, error) {
 		return nil, err
 	}
 
-	return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p)
+	return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p, conf.WriteableShareMustHavePassword)
 }
 
 // NewMemory returns a new in-memory public shares manager.
@@ -93,7 +93,7 @@ func NewMemory(c map[string]interface{}) (publicshare.Manager, error) {
 		return nil, err
 	}
 
-	return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p)
+	return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p, conf.WriteableShareMustHavePassword)
 }
 
 // NewCS3 returns a new cs3 public shares manager.
@@ -115,29 +115,31 @@ func NewCS3(c map[string]interface{}) (publicshare.Manager, error) {
 		return nil, err
 	}
 
-	return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p)
+	return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p, conf.WriteableShareMustHavePassword)
 }
 
 // New returns a new public share manager instance
-func New(gwAddr string, pwHashCost, janitorRunInterval int, enableCleanup bool, p persistence.Persistence) (publicshare.Manager, error) {
+func New(gwAddr string, pwHashCost, janitorRunInterval int, enableCleanup bool, p persistence.Persistence, writeableShareMustHavePassword bool) (publicshare.Manager, error) {
 	m := &manager{
-		gatewayAddr:                gwAddr,
-		mutex:                      &sync.Mutex{},
-		passwordHashCost:           pwHashCost,
-		janitorRunInterval:         janitorRunInterval,
-		enableExpiredSharesCleanup: enableCleanup,
-		persistence:                p,
+		gatewayAddr:                    gwAddr,
+		mutex:                          &sync.Mutex{},
+		passwordHashCost:               pwHashCost,
+		janitorRunInterval:             janitorRunInterval,
+		enableExpiredSharesCleanup:     enableCleanup,
+		persistence:                    p,
+		writeableShareMustHavePassword: writeableShareMustHavePassword,
 	}
 
 	go m.startJanitorRun()
 	return m, nil
 }
 
 type commonConfig struct {
-	GatewayAddr                string `mapstructure:"gateway_addr"`
-	SharePasswordHashCost      int    `mapstructure:"password_hash_cost"`
-	JanitorRunInterval         int    `mapstructure:"janitor_run_interval"`
-	EnableExpiredSharesCleanup bool   `mapstructure:"enable_expired_shares_cleanup"`
+	GatewayAddr                    string `mapstructure:"gateway_addr"`
+	SharePasswordHashCost          int    `mapstructure:"password_hash_cost"`
+	JanitorRunInterval             int    `mapstructure:"janitor_run_interval"`
+	EnableExpiredSharesCleanup     bool   `mapstructure:"enable_expired_shares_cleanup"`
+	WriteableShareMustHavePassword bool   `mapstructure:"writeable_share_must_have_password"`
 }
 
 type fileConfig struct {
@@ -169,9 +171,10 @@ type manager struct {
 	mutex       *sync.Mutex
 	persistence persistence.Persistence
 
-	passwordHashCost           int
-	janitorRunInterval         int
-	enableExpiredSharesCleanup bool
+	passwordHashCost               int
+	janitorRunInterval             int
+	enableExpiredSharesCleanup     bool
+	writeableShareMustHavePassword bool
 }
 
 func (m *manager) startJanitorRun() {
@@ -339,6 +342,11 @@ func (m *manager) UpdatePublicShare(ctx context.Context, u *user.User, req *link
 	case link.UpdatePublicShareRequest_Update_TYPE_PERMISSIONS:
 		old, _ := json.Marshal(share.Permissions)
 		new, _ := json.Marshal(req.Update.GetGrant().Permissions)
+
+		if m.writeableShareMustHavePassword && publicshare.IsWriteable(req.GetUpdate().GetGrant().GetPermissions()) && !share.PasswordProtected {
+			return nil, publicshare.ErrShareNeedsPassword
+		}
+
 		log.Debug().Str("json", "update grants").Msgf("from: `%v`\nto\n`%v`", old, new)
 		share.Permissions = req.Update.GetGrant().GetPermissions()
 	case link.UpdatePublicShareRequest_Update_TYPE_EXPIRATION:
@@ -349,6 +357,10 @@ func (m *manager) UpdatePublicShare(ctx context.Context, u *user.User, req *link
 	case link.UpdatePublicShareRequest_Update_TYPE_PASSWORD:
 		passwordChanged = true
 		if req.Update.GetGrant().Password == "" {
+			if m.writeableShareMustHavePassword && publicshare.IsWriteable(share.Permissions) {
+				return nil, publicshare.ErrShareNeedsPassword
+			}
+
 			share.PasswordProtected = false
 			newPasswordEncoded = ""
 		} else {

pkg/publicshare/manager/json/json_test.go

@@ -190,7 +190,7 @@ var _ = Describe("Json", func() {
 			persistence := cs3.New(storage)
 			Expect(persistence.Init(context.Background())).To(Succeed())
 
-			m, err = json.New("https://localhost:9200", 11, 60, false, persistence)
+			m, err = json.New("https://localhost:9200", 11, 60, false, persistence, false)
 			Expect(err).ToNot(HaveOccurred())
 
 			ctx = ctxpkg.ContextSetUser(context.Background(), user1)
@@ -228,7 +228,7 @@ var _ = Describe("Json", func() {
 				p := cs3.New(storage)
 				Expect(p.Init(context.Background())).To(Succeed())
 
-				m, err = json.New("https://localhost:9200", 11, 60, false, p)
+				m, err = json.New("https://localhost:9200", 11, 60, false, p, false)
 				Expect(err).ToNot(HaveOccurred())
 
 				ps, err := m.ListPublicShares(ctx, user1, []*link.ListPublicSharesRequest_Filter{}, false)

pkg/publicshare/publicshare.go

@@ -24,6 +24,7 @@ import (
 	"crypto/sha256"
 	"crypto/sha512"
 	"encoding/hex"
+	"errors"
 	"time"
 
 	user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
@@ -40,6 +41,11 @@ const (
 	StorageIDFilterType link.ListPublicSharesRequest_Filter_Type = 4
 )
 
+var (
+	// ErrShareNeedsPassword is an error which is returned when a public share must have a password.
+	ErrShareNeedsPassword = errors.New("the public share needs to have a password")
+)
+
 // Manager manipulates public shares.
 type Manager interface {
 	CreatePublicShare(ctx context.Context, u *user.User, md *provider.ResourceInfo, g *link.Grant) (*link.PublicShare, error)
@@ -213,8 +219,8 @@ func IsCreatedByUser(share link.PublicShare, user *user.User) bool {
 }
 
 // IsWriteable checks if the grant for a publicshare allows writes or uploads.
-func IsWriteable(g *link.Grant) bool {
-	p := g.Permissions.Permissions
-	return p.CreateContainer || p.Delete || p.InitiateFileUpload ||
-		p.Move || p.AddGrant || p.PurgeRecycle || p.RestoreFileVersion || p.RestoreRecycleItem
+func IsWriteable(perm *link.PublicSharePermissions) bool {
+	p := perm.GetPermissions()
+	return p != nil && (p.CreateContainer || p.Delete || p.InitiateFileUpload ||
+		p.Move || p.AddGrant || p.PurgeRecycle || p.RestoreFileVersion || p.RestoreRecycleItem)
 }