Merge pull request #7963 from rhafer/update-share-permission-check

[full-ci] graph sharing: Fixes for UpdateShare

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

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

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

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=

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 {

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{

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)

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{

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