From 3d1c50d288e6bcf3b949552e213b18cf801ed166 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 08:55:29 +0100 Subject: [PATCH 1/5] docs: add changelog --- .../unreleased/add-validation-to-public-share-provider.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/add-validation-to-public-share-provider.md diff --git a/changelog/unreleased/add-validation-to-public-share-provider.md b/changelog/unreleased/add-validation-to-public-share-provider.md new file mode 100644 index 0000000000..6ddfd4121a --- /dev/null +++ b/changelog/unreleased/add-validation-to-public-share-provider.md @@ -0,0 +1,6 @@ +Enhancement: Add validation to the public share provider + +We added validation to the public share provider. The idea behind it is that the cs3 clients will become much simpler. The provider can do the validation and return different status codes. The API clients then just need to convert CS3 status codes to http status codes. + +https://github.com/cs3org/reva/pull/4372/ +https://github.com/owncloud/ocis/issues/6993 From f73ddd10c8efe3279d8ae10030959f57150dbb85 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 08:56:37 +0100 Subject: [PATCH 2/5] fix: return better status code --- internal/grpc/services/usershareprovider/usershareprovider.go | 4 ++-- .../grpc/services/usershareprovider/usershareprovider_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index 313928c39e..bc4ccd46d0 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -156,13 +156,13 @@ func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShar req.GetGrant().GetPermissions().GetPermissions(), ); !shareCreationAllowed { return &collaboration.CreateShareResponse{ - Status: status.NewInvalid(ctx, "insufficient permissions to create that kind of share"), + Status: status.NewPermissionDenied(ctx, nil, "insufficient permissions to create that kind of share"), }, nil } if !s.isPathAllowed(req.GetResourceInfo().GetPath()) { return &collaboration.CreateShareResponse{ - Status: status.NewInvalid(ctx, "share creation is not allowed for the specified path"), + Status: status.NewFailedPrecondition(ctx, nil, "share creation is not allowed for the specified path"), }, nil } diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index 3aa58e1925..93168092bf 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -115,7 +115,7 @@ var _ = Describe("user share provider service", func() { "insufficient permissions", conversions.RoleFromName("spaceeditor", true).CS3ResourcePermissions(), conversions.RoleFromName("manager", true).CS3ResourcePermissions(), - rpcpb.Code_CODE_INVALID_ARGUMENT, + rpcpb.Code_CODE_PERMISSION_DENIED, 0, ), Entry( From 3dca7585ece95ea985622541843a80b585ea5a9e Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 08:58:14 +0100 Subject: [PATCH 3/5] refactor: remove password validation from json share manager --- pkg/publicshare/manager/json/json.go | 47 ++++++++--------------- pkg/publicshare/manager/json/json_test.go | 4 +- pkg/publicshare/publicshare.go | 6 --- 3 files changed, 19 insertions(+), 38 deletions(-) diff --git a/pkg/publicshare/manager/json/json.go b/pkg/publicshare/manager/json/json.go index 5d7673b824..960a2eea6e 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, conf.WriteableShareMustHavePassword) + return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p) } // 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, conf.WriteableShareMustHavePassword) + return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p) } // NewCS3 returns a new cs3 public shares manager. @@ -115,19 +115,18 @@ func NewCS3(c map[string]interface{}) (publicshare.Manager, error) { return nil, err } - return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p, conf.WriteableShareMustHavePassword) + return New(conf.GatewayAddr, conf.SharePasswordHashCost, conf.JanitorRunInterval, conf.EnableExpiredSharesCleanup, p) } // New returns a new public share manager instance -func New(gwAddr string, pwHashCost, janitorRunInterval int, enableCleanup bool, p persistence.Persistence, writeableShareMustHavePassword bool) (publicshare.Manager, error) { +func New(gwAddr string, pwHashCost, janitorRunInterval int, enableCleanup bool, p persistence.Persistence) (publicshare.Manager, error) { m := &manager{ - gatewayAddr: gwAddr, - mutex: &sync.Mutex{}, - passwordHashCost: pwHashCost, - janitorRunInterval: janitorRunInterval, - enableExpiredSharesCleanup: enableCleanup, - persistence: p, - writeableShareMustHavePassword: writeableShareMustHavePassword, + gatewayAddr: gwAddr, + mutex: &sync.Mutex{}, + passwordHashCost: pwHashCost, + janitorRunInterval: janitorRunInterval, + enableExpiredSharesCleanup: enableCleanup, + persistence: p, } go m.startJanitorRun() @@ -135,11 +134,10 @@ 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"` - WriteableShareMustHavePassword bool `mapstructure:"writeable_share_must_have_password"` + GatewayAddr string `mapstructure:"gateway_addr"` + SharePasswordHashCost int `mapstructure:"password_hash_cost"` + JanitorRunInterval int `mapstructure:"janitor_run_interval"` + EnableExpiredSharesCleanup bool `mapstructure:"enable_expired_shares_cleanup"` } type fileConfig struct { @@ -171,10 +169,9 @@ type manager struct { mutex *sync.Mutex persistence persistence.Persistence - passwordHashCost int - janitorRunInterval int - enableExpiredSharesCleanup bool - writeableShareMustHavePassword bool + passwordHashCost int + janitorRunInterval int + enableExpiredSharesCleanup bool } func (m *manager) startJanitorRun() { @@ -343,12 +340,6 @@ func (m *manager) UpdatePublicShare(ctx context.Context, u *user.User, req *link old, _ := json.Marshal(share.Permissions) new, _ := json.Marshal(req.Update.GetGrant().Permissions) - if m.writeableShareMustHavePassword && - publicshare.IsWriteable(req.GetUpdate().GetGrant().GetPermissions()) && - (!share.PasswordProtected && req.GetUpdate().GetGrant().GetPassword() == "") { - return nil, publicshare.ErrShareNeedsPassword - } - if req.GetUpdate().GetGrant().GetPassword() != "" { passwordChanged = true h, err := bcrypt.GenerateFromPassword([]byte(req.Update.GetGrant().Password), m.passwordHashCost) @@ -369,10 +360,6 @@ 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 d392257336..f9385896ae 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, false) + m, err = json.New("https://localhost:9200", 11, 60, false, persistence) 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, false) + m, err = json.New("https://localhost:9200", 11, 60, false, p) 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 187c3165c5..4eca5cdccb 100644 --- a/pkg/publicshare/publicshare.go +++ b/pkg/publicshare/publicshare.go @@ -24,7 +24,6 @@ import ( "crypto/sha256" "crypto/sha512" "encoding/hex" - "errors" "time" user "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -41,11 +40,6 @@ 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) From b15eeed274de1c3a4950f341317770ab74d27b61 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 08:59:53 +0100 Subject: [PATCH 4/5] feat: add validation to public share provider --- .../publicshareprovider.go | 227 ++++++++++++++++-- 1 file changed, 209 insertions(+), 18 deletions(-) diff --git a/internal/grpc/services/publicshareprovider/publicshareprovider.go b/internal/grpc/services/publicshareprovider/publicshareprovider.go index 33b7bcd3dc..7abac602a3 100644 --- a/internal/grpc/services/publicshareprovider/publicshareprovider.go +++ b/internal/grpc/services/publicshareprovider/publicshareprovider.go @@ -20,10 +20,19 @@ package publicshareprovider import ( "context" + "fmt" "regexp" + "strconv" + "time" + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" + provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" + typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" + "github.com/cs3org/reva/v2/pkg/password" + "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" + "github.com/cs3org/reva/v2/pkg/sharedconf" "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "google.golang.org/grpc" @@ -38,6 +47,8 @@ import ( "github.com/cs3org/reva/v2/pkg/rgrpc/status" ) +const getUserCtxErrMsg = "error getting user from context" + func init() { rgrpc.Register("publicshareprovider", New) } @@ -45,9 +56,21 @@ func init() { type config struct { Driver string `mapstructure:"driver"` Drivers map[string]map[string]interface{} `mapstructure:"drivers"` + GatewayAddr string `mapstructure:"gateway_addr"` AllowedPathsForShares []string `mapstructure:"allowed_paths_for_shares"` EnableExpiredSharesCleanup bool `mapstructure:"enable_expired_shares_cleanup"` WriteableShareMustHavePassword bool `mapstructure:"writeable_share_must_have_password"` + PublicShareMustHavePassword bool `mapstructure:"public_share_must_have_password"` + PasswordPolicy map[string]interface{} `mapstructure:"password_policy"` +} + +type passwordPolicy struct { + MinCharacters int `mapstructure:"min_characters"` + MinLowerCaseCharacters int `mapstructure:"min_lowercase_characters"` + MinUpperCaseCharacters int `mapstructure:"min_uppercase_characters"` + MinDigits int `mapstructure:"min_digits"` + MinSpecialCharacters int `mapstructure:"min_special_characters"` + BannedPasswordsList map[string]struct{} `mapstructure:"banned_passwords_list"` } func (c *config) init() { @@ -59,7 +82,9 @@ func (c *config) init() { type service struct { conf *config sm publicshare.Manager + gatewaySelector pool.Selectable[gateway.GatewayAPIClient] allowedPathsForShares []*regexp.Regexp + passwordValidator password.Validator } func getShareManager(c *config) (publicshare.Manager, error) { @@ -84,12 +109,21 @@ func (s *service) Register(ss *grpc.Server) { func parseConfig(m map[string]interface{}) (*config, error) { c := &config{} if err := mapstructure.Decode(m, c); err != nil { - err = errors.Wrap(err, "error decoding conf") + err = errors.Wrap(err, "error decoding config") return nil, err } return c, nil } +func parsePasswordPolicy(m map[string]interface{}) (*passwordPolicy, error) { + p := &passwordPolicy{} + if err := mapstructure.Decode(m, p); err != nil { + err = errors.Wrap(err, "error decoding password policy config") + return nil, err + } + return p, nil +} + // New creates a new user share provider svc func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { @@ -97,6 +131,10 @@ func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { if err != nil { return nil, err } + p, err := parsePasswordPolicy(c.PasswordPolicy) + if err != nil { + return nil, err + } c.init() @@ -114,15 +152,36 @@ func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { allowedPathsForShares = append(allowedPathsForShares, regex) } + gatewaySelector, err := pool.GatewaySelector(sharedconf.GetGatewaySVC(c.GatewayAddr)) + if err != nil { + return nil, err + } + service := &service{ conf: c, sm: sm, + gatewaySelector: gatewaySelector, allowedPathsForShares: allowedPathsForShares, + passwordValidator: newPasswordPolicy(p), } return service, nil } +func newPasswordPolicy(c *passwordPolicy) password.Validator { + if c == nil { + return password.NewPasswordPolicy(0, 0, 0, 0, 0, nil) + } + return password.NewPasswordPolicy( + c.MinCharacters, + c.MinLowerCaseCharacters, + c.MinUpperCaseCharacters, + c.MinDigits, + c.MinSpecialCharacters, + c.BannedPasswordsList, + ) +} + func (s *service) isPathAllowed(path string) bool { if len(s.allowedPathsForShares) == 0 { return true @@ -139,33 +198,110 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS log := appctx.GetLogger(ctx) log.Info().Str("publicshareprovider", "create").Msg("create public share") - if !conversions.SufficientCS3Permissions(req.GetResourceInfo().GetPermissionSet(), req.GetGrant().GetPermissions().GetPermissions()) { + gatewayClient, err := s.gatewaySelector.Next() + if err != nil { + return nil, err + } + + sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: req.GetResourceInfo().GetId()}}) + if err != nil { + log.Err(err).Interface("resource_id", req.GetResourceInfo().GetId()).Msg("failed to stat resource to share") + return &link.CreatePublicShareResponse{ + Status: status.NewInternal(ctx, "failed to stat resource to share"), + }, err + } + // check that user has share permissions + if !sRes.GetInfo().GetPermissionSet().AddGrant { + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, "no share permission"), + }, nil + } + + // check if the user can share with the desired permissions + if !conversions.SufficientCS3Permissions(sRes.GetInfo().GetPermissionSet(), req.GetGrant().GetPermissions().GetPermissions()) { + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, "insufficient permissions to create that kind of share"), + }, nil + } + + // validate path + if !s.isPathAllowed(req.GetResourceInfo().GetPath()) { + return &link.CreatePublicShareResponse{ + Status: status.NewFailedPrecondition(ctx, nil, "share creation is not allowed for the specified path"), + }, nil + } + + // check that this is a not a personal space root + if req.GetResourceInfo().GetId().GetOpaqueId() == req.GetResourceInfo().GetId().GetSpaceId() && + req.GetResourceInfo().GetSpace().GetSpaceType() == "personal" { return &link.CreatePublicShareResponse{ - Status: status.NewInvalid(ctx, "insufficient permissions to create that kind of share"), + Status: status.NewInvalidArg(ctx, "cannot create link on personal space root"), }, nil } - if !s.isPathAllowed(req.ResourceInfo.Path) { + // quick link returns the existing one if already present + quickLink, err := checkQuicklink(req.GetResourceInfo()) + if err != nil { return &link.CreatePublicShareResponse{ - Status: status.NewInvalid(ctx, "share creation is not allowed for the specified path"), + Status: status.NewInvalidArg(ctx, "invalid quicklink value"), }, nil } + if quickLink { + f := []*link.ListPublicSharesRequest_Filter{publicshare.ResourceIDFilter(req.GetResourceInfo().GetId())} + req := link.ListPublicSharesRequest{Filters: f} + res, err := s.ListPublicShares(ctx, &req) + if err != nil || res.GetStatus().GetCode() != rpc.Code_CODE_OK { + return &link.CreatePublicShareResponse{ + Status: status.NewInternal(ctx, "could not list public links"), + }, nil + } + for _, l := range res.GetShare() { + if l.Quicklink { + return &link.CreatePublicShareResponse{ + Status: status.NewOK(ctx), + Share: l, + }, nil + } + } + } grant := req.GetGrant() - if grant != nil && s.conf.WriteableShareMustHavePassword && - publicshare.IsWriteable(grant.GetPermissions()) && grant.Password == "" { + + // validate expiration date + if grant.GetExpiration() != nil { + expirationDateTime := cs3TimestampToTime(grant.GetExpiration()).UTC() + if expirationDateTime.Before(time.Now().UTC()) { + msg := fmt.Sprintf("expiration date is in the past: %s", expirationDateTime.Format(time.RFC3339)) + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, msg), + }, nil + } + } + + // enforce password if needed + setPassword := grant.GetPassword() + if enforcePassword(grant, s.conf) && len(setPassword) == 0 { return &link.CreatePublicShareResponse{ - Status: status.NewInvalid(ctx, "writeable shares must have a password protection"), + Status: status.NewInvalidArg(ctx, "password protection is enforced"), }, nil } + // validate password policy + if len(setPassword) > 0 { + if err := s.passwordValidator.Validate(setPassword); err != nil { + return &link.CreatePublicShareResponse{ + Status: status.NewInvalidArg(ctx, err.Error()), + }, nil + } + } + u, ok := ctxpkg.ContextGetUser(ctx) if !ok { - log.Error().Msg("error getting user from context") + log.Error().Msg(getUserCtxErrMsg) } res := &link.CreatePublicShareResponse{} - share, err := s.sm.CreatePublicShare(ctx, u, req.ResourceInfo, req.Grant) + share, err := s.sm.CreatePublicShare(ctx, u, req.GetResourceInfo(), req.GetGrant()) switch { case err != nil: log.Error().Err(err).Interface("request", req).Msg("could not write public share") @@ -179,11 +315,37 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS } func (s *service) RemovePublicShare(ctx context.Context, req *link.RemovePublicShareRequest) (*link.RemovePublicShareResponse, error) { + gatewayClient, err := s.gatewaySelector.Next() + if err != nil { + return nil, err + } + log := appctx.GetLogger(ctx) log.Info().Str("publicshareprovider", "remove").Msg("remove public share") user := ctxpkg.ContextMustGetUser(ctx) - err := s.sm.RevokePublicShare(ctx, user, req.Ref) + ps, err := s.sm.GetPublicShare(ctx, user, req.GetRef(), false) + if err != nil { + return &link.RemovePublicShareResponse{ + Status: status.NewInternal(ctx, "error loading public share"), + }, err + } + if !publicshare.IsCreatedByUser(*ps, user) { + sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: ps.ResourceId}}) + if err != nil { + log.Err(err).Interface("resource_id", ps.ResourceId).Msg("failed to stat shared resource") + return &link.RemovePublicShareResponse{ + Status: status.NewInternal(ctx, "failed to stat shared resource"), + }, err + } + + if !sRes.GetInfo().GetPermissionSet().RemoveGrant { + return &link.RemovePublicShareResponse{ + Status: status.NewPermissionDenied(ctx, nil, "no permission to delete public share"), + }, err + } + } + err = s.sm.RevokePublicShare(ctx, user, req.Ref) if err != nil { return &link.RemovePublicShareResponse{ Status: status.NewInternal(ctx, "error deleting public share"), @@ -227,7 +389,7 @@ func (s *service) GetPublicShare(ctx context.Context, req *link.GetPublicShareRe u, ok := ctxpkg.ContextGetUser(ctx) if !ok { - log.Error().Msg("error getting user from context") + log.Error().Msg(getUserCtxErrMsg) } ps, err := s.sm.GetPublicShare(ctx, u, req.Ref, req.GetSign()) @@ -281,16 +443,11 @@ func (s *service) UpdatePublicShare(ctx context.Context, req *link.UpdatePublicS u, ok := ctxpkg.ContextGetUser(ctx) if !ok { - log.Error().Msg("error getting user from context") + log.Error().Msg(getUserCtxErrMsg) } updateR, err := s.sm.UpdatePublicShare(ctx, u, req) if err != nil { - 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 @@ -302,3 +459,37 @@ func (s *service) UpdatePublicShare(ctx context.Context, req *link.UpdatePublicS } return res, nil } + +func enforcePassword(grant *link.Grant, conf *config) bool { + if conf.PublicShareMustHavePassword { + return true + } + isReadOnly := conversions.SufficientCS3Permissions(conversions.NewViewerRole(true).CS3ResourcePermissions(), grant.GetPermissions().GetPermissions()) + if !isReadOnly && conf.WriteableShareMustHavePassword { + return true + } + return false +} + +func checkQuicklink(info *provider.ResourceInfo) (bool, error) { + if info == nil { + return false, nil + } + if m := info.GetArbitraryMetadata().GetMetadata(); m != nil { + q, ok := m["quicklink"] + // empty string would trigger an error in ParseBool() + if !ok || q == "" { + return false, nil + } + quickLink, err := strconv.ParseBool(q) + if err != nil { + return false, err + } + return quickLink, nil + } + return false, nil +} + +func cs3TimestampToTime(t *typesv1beta1.Timestamp) time.Time { + return time.Unix(int64(t.Seconds), int64(t.Nanos)) +} From 0fd5eba44b02a4a736c7a1a4b19963e684e5c6d3 Mon Sep 17 00:00:00 2001 From: Michael Barz Date: Wed, 6 Dec 2023 11:21:58 +0100 Subject: [PATCH 5/5] style: address feedback from code review --- .../publicshareprovider.go | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/internal/grpc/services/publicshareprovider/publicshareprovider.go b/internal/grpc/services/publicshareprovider/publicshareprovider.go index 7abac602a3..8687a4da45 100644 --- a/internal/grpc/services/publicshareprovider/publicshareprovider.go +++ b/internal/grpc/services/publicshareprovider/publicshareprovider.go @@ -29,10 +29,12 @@ import ( rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" link "github.com/cs3org/go-cs3apis/cs3/sharing/link/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" - typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1" "github.com/cs3org/reva/v2/pkg/password" + "github.com/cs3org/reva/v2/pkg/permission" "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" "github.com/cs3org/reva/v2/pkg/sharedconf" + "github.com/cs3org/reva/v2/pkg/storage/utils/grants" + "github.com/cs3org/reva/v2/pkg/utils" "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "google.golang.org/grpc" @@ -203,6 +205,8 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS return nil, err } + isInternalLink := grants.PermissionsEqual(req.GetGrant().GetPermissions().GetPermissions(), &provider.ResourcePermissions{}) + sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: req.GetResourceInfo().GetId()}}) if err != nil { log.Err(err).Interface("resource_id", req.GetResourceInfo().GetId()).Msg("failed to stat resource to share") @@ -210,6 +214,23 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS Status: status.NewInternal(ctx, "failed to stat resource to share"), }, err } + + // all users can create internal links + if !isInternalLink { + // check if the user has the permission in the user role + ok, err := utils.CheckPermission(ctx, permission.WritePublicLink, gatewayClient) + if err != nil { + return &link.CreatePublicShareResponse{ + Status: status.NewInternal(ctx, "failed check user permission to write public link"), + }, err + } + if !ok { + return &link.CreatePublicShareResponse{ + Status: status.NewPermissionDenied(ctx, nil, "no permission to create public links"), + }, nil + } + } + // check that user has share permissions if !sRes.GetInfo().GetPermissionSet().AddGrant { return &link.CreatePublicShareResponse{ @@ -269,7 +290,7 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS // validate expiration date if grant.GetExpiration() != nil { - expirationDateTime := cs3TimestampToTime(grant.GetExpiration()).UTC() + expirationDateTime := utils.TSToTime(grant.GetExpiration()).UTC() if expirationDateTime.Before(time.Now().UTC()) { msg := fmt.Sprintf("expiration date is in the past: %s", expirationDateTime.Format(time.RFC3339)) return &link.CreatePublicShareResponse{ @@ -280,7 +301,7 @@ func (s *service) CreatePublicShare(ctx context.Context, req *link.CreatePublicS // enforce password if needed setPassword := grant.GetPassword() - if enforcePassword(grant, s.conf) && len(setPassword) == 0 { + if !isInternalLink && enforcePassword(grant, s.conf) && len(setPassword) == 0 { return &link.CreatePublicShareResponse{ Status: status.NewInvalidArg(ctx, "password protection is enforced"), }, nil @@ -465,10 +486,7 @@ func enforcePassword(grant *link.Grant, conf *config) bool { return true } isReadOnly := conversions.SufficientCS3Permissions(conversions.NewViewerRole(true).CS3ResourcePermissions(), grant.GetPermissions().GetPermissions()) - if !isReadOnly && conf.WriteableShareMustHavePassword { - return true - } - return false + return !isReadOnly && conf.WriteableShareMustHavePassword } func checkQuicklink(info *provider.ResourceInfo) (bool, error) { @@ -489,7 +507,3 @@ func checkQuicklink(info *provider.ResourceInfo) (bool, error) { } return false, nil } - -func cs3TimestampToTime(t *typesv1beta1.Timestamp) time.Time { - return time.Unix(int64(t.Seconds), int64(t.Nanos)) -}