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 diff --git a/internal/grpc/services/publicshareprovider/publicshareprovider.go b/internal/grpc/services/publicshareprovider/publicshareprovider.go index 33b7bcd3dc..8687a4da45 100644 --- a/internal/grpc/services/publicshareprovider/publicshareprovider.go +++ b/internal/grpc/services/publicshareprovider/publicshareprovider.go @@ -20,10 +20,21 @@ 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" + "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" @@ -38,6 +49,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 +58,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 +84,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 +111,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 +133,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 +154,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 +200,129 @@ 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 + } + + 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") + return &link.CreatePublicShareResponse{ + 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{ - Status: status.NewInvalid(ctx, "insufficient permissions to create that kind of share"), + Status: status.NewInvalidArg(ctx, "no share permission"), }, nil } - if !s.isPathAllowed(req.ResourceInfo.Path) { + // 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.NewInvalid(ctx, "share creation is not allowed for the specified path"), + 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.NewInvalidArg(ctx, "cannot create link on personal space root"), + }, nil + } + + // quick link returns the existing one if already present + quickLink, err := checkQuicklink(req.GetResourceInfo()) + if err != nil { + return &link.CreatePublicShareResponse{ + 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 := 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{ + Status: status.NewInvalidArg(ctx, msg), + }, nil + } + } + + // enforce password if needed + setPassword := grant.GetPassword() + if !isInternalLink && 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 +336,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 +410,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 +464,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 +480,30 @@ 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()) + return !isReadOnly && conf.WriteableShareMustHavePassword +} + +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 +} 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( 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)