diff --git a/changelog/unreleased/bump-reva.md b/changelog/unreleased/bump-reva.md index e8fd1390bb..72915a731f 100644 --- a/changelog/unreleased/bump-reva.md +++ b/changelog/unreleased/bump-reva.md @@ -5,3 +5,4 @@ Bumps reva version https://github.com/owncloud/ocis/pull/7793 https://github.com/owncloud/ocis/pull/7978 https://github.com/owncloud/ocis/pull/7979 +https://github.com/owncloud/ocis/pull/7963 diff --git a/changelog/unreleased/fix-update-share-ng.md b/changelog/unreleased/fix-update-share-ng.md new file mode 100644 index 0000000000..031a3e3320 --- /dev/null +++ b/changelog/unreleased/fix-update-share-ng.md @@ -0,0 +1,6 @@ +Bugfix: Update permission validation + +We fixed a bug where the permission validation was not working correctly. + +https://github.com/owncloud/ocis/pull/7963 +https://github.com/cs3org/reva/pull/4405 diff --git a/go.mod b/go.mod index 11459fd33a..72b8c2abd6 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/coreos/go-oidc v2.2.1+incompatible github.com/coreos/go-oidc/v3 v3.9.0 github.com/cs3org/go-cs3apis v0.0.0-20231023073225-7748710e0781 - github.com/cs3org/reva/v2 v2.17.1-0.20231215113433-48c0ea55bf47 + github.com/cs3org/reva/v2 v2.17.1-0.20231215134723-5142bf31838d github.com/dhowden/tag v0.0.0-20230630033851-978a0926ee25 github.com/disintegration/imaging v1.6.2 github.com/dutchcoders/go-clamd v0.0.0-20170520113014-b970184f4d9e diff --git a/go.sum b/go.sum index ad2f34a708..b2dba7dca6 100644 --- a/go.sum +++ b/go.sum @@ -1023,6 +1023,8 @@ github.com/cs3org/go-cs3apis v0.0.0-20231023073225-7748710e0781 h1:BUdwkIlf8IS2F github.com/cs3org/go-cs3apis v0.0.0-20231023073225-7748710e0781/go.mod h1:UXha4TguuB52H14EMoSsCqDj7k8a/t7g4gVP+bgY5LY= github.com/cs3org/reva/v2 v2.17.1-0.20231215113433-48c0ea55bf47 h1:6DfMeFpCXoqlfm/+FJ/mFs8Ul5WCZNlorsbDM9Z/ATE= github.com/cs3org/reva/v2 v2.17.1-0.20231215113433-48c0ea55bf47/go.mod h1:oX1YtLKGr7jatGk0CpPM4GKbSEIdHhmsQuSAYElnN1U= +github.com/cs3org/reva/v2 v2.17.1-0.20231215134723-5142bf31838d h1:OYkjbcOAntD5JBMAuyj+bR1bg5jM+BjRvFBiTlmnxWQ= +github.com/cs3org/reva/v2 v2.17.1-0.20231215134723-5142bf31838d/go.mod h1:JyvlRw2v8BTH5t+ISj1Yc+EMDBcyf8LMB5o98HufWis= github.com/cyberdelia/templates v0.0.0-20141128023046-ca7fffd4298c/go.mod h1:GyV+0YP4qX0UQ7r2MoYZ+AvYDp12OF5yg4q8rGnyNh4= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= diff --git a/services/graph/pkg/service/v0/driveitems.go b/services/graph/pkg/service/v0/driveitems.go index 4f9432c558..ec623507aa 100644 --- a/services/graph/pkg/service/v0/driveitems.go +++ b/services/graph/pkg/service/v0/driveitems.go @@ -670,6 +670,7 @@ func (g Graph) UpdatePermission(w http.ResponseWriter, r *http.Request) { updatedPermission, err := g.updateUserShare(ctx, permissionID, oldPermission, permission) if err != nil { errorcode.RenderError(w, r, err) + return } render.Status(r, http.StatusOK) render.JSON(w, r, &updatedPermission) @@ -800,14 +801,11 @@ func (g Graph) updateUserShare(ctx context.Context, permissionID string, oldPerm } cs3UpdateShareReq := &collaboration.UpdateShareRequest{ - Ref: &collaboration.ShareReference{ - Spec: &collaboration.ShareReference_Id{ - Id: &collaboration.ShareId{ - OpaqueId: permissionID, - }, + Share: &collaboration.Share{ + Id: &collaboration.ShareId{ + OpaqueId: permissionID, }, }, - Share: &collaboration.Share{}, } fieldmask := []string{} if expiration, ok := newPermission.GetExpirationDateTimeOk(); ok { diff --git a/services/graph/pkg/service/v0/driveitems_test.go b/services/graph/pkg/service/v0/driveitems_test.go index 0fbca67f9d..fc10596fe2 100644 --- a/services/graph/pkg/service/v0/driveitems_test.go +++ b/services/graph/pkg/service/v0/driveitems_test.go @@ -597,7 +597,7 @@ var _ = Describe("Driveitems", func() { updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { - if req.GetRef().GetId().GetOpaqueId() == "permissionid" { + if req.GetShare().GetId().GetOpaqueId() == "permissionid" { return expiration.Equal(utils.TSToTime(req.GetShare().GetExpiration())) } return false @@ -628,7 +628,7 @@ var _ = Describe("Driveitems", func() { updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { - if req.GetRef().GetId().GetOpaqueId() == "permissionid" { + if req.GetShare().GetId().GetOpaqueId() == "permissionid" { return true } return false @@ -659,7 +659,7 @@ var _ = Describe("Driveitems", func() { updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { - return req.GetRef().GetId().GetOpaqueId() == "permissionid" + return req.GetShare().GetId().GetOpaqueId() == "permissionid" }), ) updateShareMock.Return(updateShareMockResponse, nil) @@ -687,7 +687,7 @@ var _ = Describe("Driveitems", func() { updateShareMock := gatewayClient.On("UpdateShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool { - return req.GetRef().GetId().GetOpaqueId() == "permissionid" + return req.GetShare().GetId().GetOpaqueId() == "permissionid" }), ) updateShareMockResponse.Share.Permissions = &collaboration.SharePermissions{ diff --git a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/gateway/usershareprovider.go b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/gateway/usershareprovider.go index fd5a57f9e9..d9677866f4 100644 --- a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/gateway/usershareprovider.go +++ b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/gateway/usershareprovider.go @@ -119,6 +119,9 @@ func (s *svc) updateShare(ctx context.Context, req *collaboration.UpdateShareReq if err != nil { return nil, errors.Wrap(err, "gateway: error calling UpdateShare") } + if res.GetStatus().GetCode() != rpc.Code_CODE_OK { + return res, nil + } if s.c.CommitShareToStorageGrant { creator := ctxpkg.ContextMustGetUser(ctx) diff --git a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider/usershareprovider.go b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider/usershareprovider.go index bc4ccd46d0..5e09a084e1 100644 --- a/vendor/github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/vendor/github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider/usershareprovider.go @@ -21,7 +21,9 @@ package usershareprovider import ( "context" "regexp" + "slices" + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" collaboration "github.com/cs3org/go-cs3apis/cs3/sharing/collaboration/v1beta1" @@ -34,20 +36,24 @@ import ( "github.com/cs3org/reva/v2/pkg/conversions" ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" "github.com/cs3org/reva/v2/pkg/errtypes" + "github.com/cs3org/reva/v2/pkg/permission" "github.com/cs3org/reva/v2/pkg/rgrpc" "github.com/cs3org/reva/v2/pkg/rgrpc/status" + "github.com/cs3org/reva/v2/pkg/rgrpc/todo/pool" "github.com/cs3org/reva/v2/pkg/share" "github.com/cs3org/reva/v2/pkg/share/manager/registry" + "github.com/cs3org/reva/v2/pkg/sharedconf" "github.com/cs3org/reva/v2/pkg/utils" ) func init() { - rgrpc.Register("usershareprovider", New) + rgrpc.Register("usershareprovider", NewDefault) } 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"` } @@ -58,8 +64,8 @@ func (c *config) init() { } type service struct { - conf *config sm share.Manager + gatewaySelector pool.Selectable[gateway.GatewayAPIClient] allowedPathsForShares []*regexp.Regexp } @@ -92,8 +98,8 @@ func parseConfig(m map[string]interface{}) (*config, error) { return c, nil } -// New creates a new user share provider svc -func New(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { +// New creates a new user share provider svc initialized from defaults +func NewDefault(m map[string]interface{}, ss *grpc.Server) (rgrpc.Service, error) { c, err := parseConfig(m) if err != nil { @@ -116,13 +122,23 @@ 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 + } + + return New(gatewaySelector, sm, allowedPathsForShares), nil +} + +// New creates a new user share provider svc +func New(gatewaySelector pool.Selectable[gateway.GatewayAPIClient], sm share.Manager, allowedPathsForShares []*regexp.Regexp) rgrpc.Service { service := &service{ - conf: c, sm: sm, + gatewaySelector: gatewaySelector, allowedPathsForShares: allowedPathsForShares, } - return service, nil + return service } func (s *service) isPathAllowed(path string) bool { @@ -140,6 +156,24 @@ func (s *service) isPathAllowed(path string) bool { func (s *service) CreateShare(ctx context.Context, req *collaboration.CreateShareRequest) (*collaboration.CreateShareResponse, error) { user := ctxpkg.ContextMustGetUser(ctx) + gatewayClient, err := s.gatewaySelector.Next() + if err != nil { + return nil, err + } + + // check if the user has the permission to create shares at all + ok, err := utils.CheckPermission(ctx, permission.WriteShare, gatewayClient) + if err != nil { + return &collaboration.CreateShareResponse{ + Status: status.NewInternal(ctx, "failed check user permission to write public link"), + }, err + } + if !ok { + return &collaboration.CreateShareResponse{ + Status: status.NewPermissionDenied(ctx, nil, "no permission to create public links"), + }, nil + } + if req.GetGrant().GetGrantee().GetType() == provider.GranteeType_GRANTEE_TYPE_USER && req.GetGrant().GetGrantee().GetUserId().GetIdp() == "" { // use logged in user Idp as default. req.GetGrant().GetGrantee().Id = &provider.Grantee_UserId{ @@ -244,6 +278,78 @@ func (s *service) ListShares(ctx context.Context, req *collaboration.ListSharesR } func (s *service) UpdateShare(ctx context.Context, req *collaboration.UpdateShareRequest) (*collaboration.UpdateShareResponse, error) { + log := appctx.GetLogger(ctx) + gatewayClient, err := s.gatewaySelector.Next() + if err != nil { + return nil, err + } + + // check if the user has the permission to create shares at all + ok, err := utils.CheckPermission(ctx, permission.WriteShare, gatewayClient) + if err != nil { + return &collaboration.UpdateShareResponse{ + Status: status.NewInternal(ctx, "failed check user permission to write share"), + }, err + } + if !ok { + return &collaboration.UpdateShareResponse{ + Status: status.NewPermissionDenied(ctx, nil, "no permission to create user share"), + }, nil + } + + // Read share from backend. We need the shared resource's id for STATing it, it might not be in + // the incoming request + currentShare, err := s.sm.GetShare(ctx, + &collaboration.ShareReference{ + Spec: &collaboration.ShareReference_Id{ + Id: req.GetShare().GetId(), + }, + }, + ) + if err != nil { + var st *rpc.Status + switch err.(type) { + case errtypes.IsNotFound: + st = status.NewNotFound(ctx, err.Error()) + default: + st = status.NewInternal(ctx, err.Error()) + } + return &collaboration.UpdateShareResponse{ + Status: st, + }, nil + } + + sRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{Ref: &provider.Reference{ResourceId: currentShare.GetResourceId()}}) + if err != nil { + log.Err(err).Interface("resource_id", req.GetShare().GetResourceId()).Msg("failed to stat resource to share") + return &collaboration.UpdateShareResponse{ + Status: status.NewInternal(ctx, "failed to stat shared resource"), + }, err + } + + // If this is a permissions update, check if user's permissions on the resource are sufficient to set the desired permissions + var newPermissions *provider.ResourcePermissions + if slices.Contains(req.GetUpdateMask().GetPaths(), "permissions") { + newPermissions = req.GetShare().GetPermissions().GetPermissions() + } else { + newPermissions = req.GetField().GetPermissions().GetPermissions() + } + if newPermissions != nil && !conversions.SufficientCS3Permissions(sRes.GetInfo().GetPermissionSet(), newPermissions) { + return &collaboration.UpdateShareResponse{ + Status: status.NewPermissionDenied(ctx, nil, "insufficient permissions to create that kind of share"), + }, nil + } + + // check if the requested permission are plausible for the Resource + // do we need more here? + if sRes.GetInfo().GetType() == provider.ResourceType_RESOURCE_TYPE_FILE { + if newPermissions.GetCreateContainer() || newPermissions.GetMove() || newPermissions.GetDelete() { + return &collaboration.UpdateShareResponse{ + Status: status.NewInvalid(ctx, "cannot set the requested permissions on that type of resource"), + }, nil + } + } + share, err := s.sm.UpdateShare(ctx, req.Ref, req.Field.GetPermissions(), req.Share, req.UpdateMask) // TODO(labkode): check what to update if err != nil { return &collaboration.UpdateShareResponse{ diff --git a/vendor/modules.txt b/vendor/modules.txt index e43d9ef670..e4089ff682 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -362,7 +362,7 @@ github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1 github.com/cs3org/go-cs3apis/cs3/storage/registry/v1beta1 github.com/cs3org/go-cs3apis/cs3/tx/v1beta1 github.com/cs3org/go-cs3apis/cs3/types/v1beta1 -# github.com/cs3org/reva/v2 v2.17.1-0.20231215113433-48c0ea55bf47 +# github.com/cs3org/reva/v2 v2.17.1-0.20231215134723-5142bf31838d ## explicit; go 1.21 github.com/cs3org/reva/v2/cmd/revad/internal/grace github.com/cs3org/reva/v2/cmd/revad/runtime