From 8b1effcadaccc6f13e2e30109d256a17c84c9972 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Thu, 9 Mar 2023 15:03:08 +0100 Subject: [PATCH] properly enforce the password on writeable public links --- changelog/unreleased/public-link-password.md | 1 + .../publicshareprovider.go | 19 ++++---- pkg/publicshare/manager/json/json.go | 46 ++++++++++++------- pkg/publicshare/manager/json/json_test.go | 4 +- pkg/publicshare/publicshare.go | 14 ++++-- 5 files changed, 51 insertions(+), 33 deletions(-) diff --git a/changelog/unreleased/public-link-password.md b/changelog/unreleased/public-link-password.md index e9e50d883d..d4f899fa74 100644 --- a/changelog/unreleased/public-link-password.md +++ b/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 diff --git a/internal/grpc/services/publicshareprovider/publicshareprovider.go b/internal/grpc/services/publicshareprovider/publicshareprovider.go index b52e1d6939..7f7e301662 100644 --- a/internal/grpc/services/publicshareprovider/publicshareprovider.go +++ b/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,14 +253,6 @@ 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") @@ -268,7 +260,14 @@ func (s *service) UpdatePublicShare(ctx context.Context, req *link.UpdatePublicS 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{ diff --git a/pkg/publicshare/manager/json/json.go b/pkg/publicshare/manager/json/json.go index ddeda09cab..31eb31eeb2 100644 --- a/pkg/publicshare/manager/json/json.go +++ b/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,18 +115,19 @@ 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() @@ -134,10 +135,11 @@ func New(gwAddr string, pwHashCost, janitorRunInterval int, enableCleanup bool, } 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 { diff --git a/pkg/publicshare/manager/json/json_test.go b/pkg/publicshare/manager/json/json_test.go index f9385896ae..d392257336 100644 --- a/pkg/publicshare/manager/json/json_test.go +++ b/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) diff --git a/pkg/publicshare/publicshare.go b/pkg/publicshare/publicshare.go index 2f3ba444e4..187c3165c5 100644 --- a/pkg/publicshare/publicshare.go +++ b/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) }