From 2987785b0291bbc803fd7c47b4dd4b66ff20dc34 Mon Sep 17 00:00:00 2001 From: Florian Schade Date: Fri, 14 Jun 2024 16:02:38 +0200 Subject: [PATCH] fix: ocs handles the mount_point on its own --- .../enhance-unique-share-mountpoint-name.md | 1 + .../usershareprovider/usershareprovider.go | 136 +++++++++--------- .../usershareprovider_test.go | 16 +-- .../apps/sharing/shares/export_test.go | 3 - .../handlers/apps/sharing/shares/pending.go | 40 +++--- .../apps/sharing/shares/pending_test.go | 27 +--- 6 files changed, 100 insertions(+), 123 deletions(-) delete mode 100644 internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/export_test.go diff --git a/changelog/unreleased/enhance-unique-share-mountpoint-name.md b/changelog/unreleased/enhance-unique-share-mountpoint-name.md index e8ffaf43d2..b2f6509ee6 100644 --- a/changelog/unreleased/enhance-unique-share-mountpoint-name.md +++ b/changelog/unreleased/enhance-unique-share-mountpoint-name.md @@ -2,6 +2,7 @@ Enhancement: Unique share mountpoint name Accepting a received share with a mountpoint name that already exists will now append a unique suffix to the mountpoint name. +https://github.com/cs3org/reva/pull/4725 https://github.com/cs3org/reva/pull/4723 https://github.com/cs3org/reva/pull/4714 https://github.com/owncloud/ocis/issues/8961 diff --git a/internal/grpc/services/usershareprovider/usershareprovider.go b/internal/grpc/services/usershareprovider/usershareprovider.go index 178c6f78d3..4bec8dd75d 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider.go +++ b/internal/grpc/services/usershareprovider/usershareprovider.go @@ -547,8 +547,66 @@ func (s *service) UpdateReceivedShare(ctx context.Context, req *collaboration.Up }, nil } -// GetAvailableMountpoint returns a new or existing mountpoint -func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, id *provider.ResourceId, name string, userId *userpb.UserId) (string, error) { +func setReceivedShareMountPoint(ctx context.Context, gwc gateway.GatewayAPIClient, req *collaboration.UpdateReceivedShareRequest) (*rpc.Status, error) { + receivedShare, err := gwc.GetReceivedShare(ctx, &collaboration.GetReceivedShareRequest{ + Ref: &collaboration.ShareReference{ + Spec: &collaboration.ShareReference_Id{ + Id: req.GetShare().GetShare().GetId(), + }, + }, + }) + switch { + case err != nil: + fallthrough + case receivedShare.GetStatus().GetCode() != rpc.Code_CODE_OK: + return receivedShare.GetStatus(), err + } + + if receivedShare.GetShare().GetMountPoint().GetPath() != "" { + return status.NewOK(ctx), nil + } + + resourceStat, err := gwc.Stat(ctx, &provider.StatRequest{ + Ref: &provider.Reference{ + ResourceId: receivedShare.GetShare().GetShare().GetResourceId(), + }, + }) + switch { + case err != nil: + fallthrough + case resourceStat.GetStatus().GetCode() != rpc.Code_CODE_OK: + return resourceStat.GetStatus(), err + } + + // handle mount point related updates + { + var userID *userpb.UserId + _ = utils.ReadJSONFromOpaque(req.Opaque, "userid", &userID) + + // check if the requested mount point is available and if not, find a suitable one + availableMountpoint, _, err := GetMountpointAndUnmountedShares(ctx, gwc, + resourceStat.GetInfo().GetId(), + resourceStat.GetInfo().GetName(), + userID, + ) + if err != nil { + return status.NewInternal(ctx, err.Error()), nil + } + + if !slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) { + req.GetUpdateMask().Paths = append(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) + } + + req.GetShare().MountPoint = &provider.Reference{ + Path: availableMountpoint, + } + } + + return status.NewOK(ctx), nil +} + +// GetMountpointAndUnmountedShares returns a new or existing mountpoint for the given info and produces a list of unmounted received shares for the same resource +func GetMountpointAndUnmountedShares(ctx context.Context, gwc gateway.GatewayAPIClient, id *provider.ResourceId, name string, userId *userpb.UserId) (string, []*collaboration.ReceivedShare, error) { listReceivedSharesReq := &collaboration.ListReceivedSharesRequest{} if userId != nil { listReceivedSharesReq.Opaque = utils.AppendJSONToOpaque(nil, "userid", userId) @@ -556,13 +614,14 @@ func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, i listReceivedSharesRes, err := gwc.ListReceivedShares(ctx, listReceivedSharesReq) if err != nil { - return "", errtypes.InternalError("grpc list received shares request failed") + return "", nil, errtypes.InternalError("grpc list received shares request failed") } if err := errtypes.NewErrtypeFromStatus(listReceivedSharesRes.GetStatus()); err != nil { - return "", err + return "", nil, err } + unmountedShares := []*collaboration.ReceivedShare{} base := filepath.Clean(name) mount := base existingMountpoint := "" @@ -580,6 +639,11 @@ func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, i } } + if resourceIDEqual && s.State != collaboration.ShareState_SHARE_STATE_ACCEPTED { + // a share to the resource already exists but is not mounted, collect the unmounted share + unmountedShares = append(unmountedShares, s) + } + if s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED { // collect all accepted mount points mountedShares = append(mountedShares, s.GetMountPoint().GetPath()) @@ -596,7 +660,7 @@ func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, i if existingMountpoint != "" { // we want to reuse the same mountpoint for all unmounted shares to the same resource - return existingMountpoint, nil + return existingMountpoint, unmountedShares, nil } // If the mount point really already exists, we need to insert a number into the filename @@ -608,68 +672,10 @@ func GetAvailableMountpoint(ctx context.Context, gwc gateway.GatewayAPIClient, i mount = name + " (" + strconv.Itoa(i) + ")" + ext if !slices.Contains(mountedShares, mount) { - return mount, nil + return mount, unmountedShares, nil } } } - return mount, nil -} - -func setReceivedShareMountPoint(ctx context.Context, gwc gateway.GatewayAPIClient, req *collaboration.UpdateReceivedShareRequest) (*rpc.Status, error) { - receivedShare, err := gwc.GetReceivedShare(ctx, &collaboration.GetReceivedShareRequest{ - Ref: &collaboration.ShareReference{ - Spec: &collaboration.ShareReference_Id{ - Id: req.GetShare().GetShare().GetId(), - }, - }, - }) - switch { - case err != nil: - fallthrough - case receivedShare.GetStatus().GetCode() != rpc.Code_CODE_OK: - return receivedShare.GetStatus(), err - } - - if receivedShare.GetShare().GetMountPoint().GetPath() != "" { - return status.NewOK(ctx), nil - } - - resourceStat, err := gwc.Stat(ctx, &provider.StatRequest{ - Ref: &provider.Reference{ - ResourceId: receivedShare.GetShare().GetShare().GetResourceId(), - }, - }) - switch { - case err != nil: - fallthrough - case resourceStat.GetStatus().GetCode() != rpc.Code_CODE_OK: - return resourceStat.GetStatus(), err - } - - // handle mount point related updates - { - var userID *userpb.UserId - _ = utils.ReadJSONFromOpaque(req.Opaque, "userid", &userID) - - // check if the requested mount point is available and if not, find a suitable one - availableMountpoint, err := GetAvailableMountpoint(ctx, gwc, - resourceStat.GetInfo().GetId(), - resourceStat.GetInfo().GetName(), - userID, - ) - if err != nil { - return status.NewInternal(ctx, err.Error()), nil - } - - if !slices.Contains(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) { - req.GetUpdateMask().Paths = append(req.GetUpdateMask().GetPaths(), _fieldMaskPathMountPoint) - } - - req.GetShare().MountPoint = &provider.Reference{ - Path: availableMountpoint, - } - } - - return status.NewOK(ctx), nil + return mount, unmountedShares, nil } diff --git a/internal/grpc/services/usershareprovider/usershareprovider_test.go b/internal/grpc/services/usershareprovider/usershareprovider_test.go index ebf9cea594..afaf689559 100644 --- a/internal/grpc/services/usershareprovider/usershareprovider_test.go +++ b/internal/grpc/services/usershareprovider/usershareprovider_test.go @@ -582,15 +582,15 @@ var _ = Describe("user share provider service", func() { }) var _ = Describe("helpers", func() { - type GetAvailableMountpointArgs struct { + type GetMountpointAndUnmountedSharesArgs struct { withName string withResourceId *providerpb.ResourceId listReceivedSharesResponse *collaborationpb.ListReceivedSharesResponse listReceivedSharesError error expectedName string } - DescribeTable("GetAvailableMountpoint", - func(args GetAvailableMountpointArgs) { + DescribeTable("GetMountpointAndUnmountedShares", + func(args GetMountpointAndUnmountedSharesArgs) { gatewayClient := cs3mocks.NewGatewayAPIClient(GinkgoT()) gatewayClient.EXPECT(). @@ -627,7 +627,7 @@ var _ = Describe("helpers", func() { }).Times(statCallCount) } - availableMountpoint, err := usershareprovider.GetAvailableMountpoint(context.Background(), gatewayClient, args.withResourceId, args.withName, nil) + availableMountpoint, _, err := usershareprovider.GetMountpointAndUnmountedShares(context.Background(), gatewayClient, args.withResourceId, args.withName, nil) if args.listReceivedSharesError != nil { Expect(err).To(HaveOccurred(), "expected error, got none") @@ -640,13 +640,13 @@ var _ = Describe("helpers", func() { }, Entry( "listing received shares errors", - GetAvailableMountpointArgs{ + GetMountpointAndUnmountedSharesArgs{ listReceivedSharesError: errors.New("some error"), }, ), Entry( "returns the given name if no shares are found", - GetAvailableMountpointArgs{ + GetMountpointAndUnmountedSharesArgs{ withName: "name1", listReceivedSharesResponse: &collaborationpb.ListReceivedSharesResponse{ Status: status.NewOK(context.Background()), @@ -656,7 +656,7 @@ var _ = Describe("helpers", func() { ), Entry( "returns the path as name if a share with the same resourceId is found", - GetAvailableMountpointArgs{ + GetMountpointAndUnmountedSharesArgs{ withName: "name", withResourceId: &providerpb.ResourceId{ StorageId: "1", @@ -686,7 +686,7 @@ var _ = Describe("helpers", func() { ), Entry( "enumerates the name if a share with the same path already exists", - GetAvailableMountpointArgs{ + GetMountpointAndUnmountedSharesArgs{ withName: "some name", listReceivedSharesResponse: &collaborationpb.ListReceivedSharesResponse{ Status: status.NewOK(context.Background()), diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/export_test.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/export_test.go deleted file mode 100644 index 8098851e26..0000000000 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/export_test.go +++ /dev/null @@ -1,3 +0,0 @@ -package shares - -var GetUnmountedShares = getUnmountedShares diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go index 5bfd78cd29..9502862446 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go @@ -34,11 +34,11 @@ import ( "github.com/pkg/errors" "google.golang.org/protobuf/types/known/fieldmaskpb" + "github.com/cs3org/reva/v2/internal/grpc/services/usershareprovider" "github.com/cs3org/reva/v2/internal/http/services/owncloud/ocs/response" "github.com/cs3org/reva/v2/pkg/appctx" "github.com/cs3org/reva/v2/pkg/conversions" "github.com/cs3org/reva/v2/pkg/errtypes" - "github.com/cs3org/reva/v2/pkg/utils" ) const ( @@ -73,8 +73,13 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) { response.WriteOCSResponse(w, r, *ocsResponse, nil) return } - - unmountedShares, err := getUnmountedShares(ctx, client, sharedResource.GetInfo().GetId()) + mount, unmountedShares, err := usershareprovider.GetMountpointAndUnmountedShares( + ctx, + client, + sharedResource.GetInfo().GetId(), + sharedResource.GetInfo().GetName(), + nil, + ) if err != nil { response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "could not determine mountpoint", err) return @@ -82,8 +87,12 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) { // first update the requested share receivedShare.State = collaboration.ShareState_SHARE_STATE_ACCEPTED + // we need to add a path to the share + receivedShare.MountPoint = &provider.Reference{ + Path: mount, + } - updateMask := &fieldmaskpb.FieldMask{Paths: []string{"state"}} + updateMask := &fieldmaskpb.FieldMask{Paths: []string{"state", "mount_point"}} data, meta, err := h.updateReceivedShare(r.Context(), receivedShare, updateMask) if err != nil { // we log an error for affected shares, for the actual share we return an error @@ -100,6 +109,10 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) { } rs.State = collaboration.ShareState_SHARE_STATE_ACCEPTED + // set the same mountpoint as for the requested received share + rs.MountPoint = &provider.Reference{ + Path: mount, + } _, _, err := h.updateReceivedShare(r.Context(), rs, updateMask) if err != nil { @@ -314,25 +327,6 @@ func getReceivedShareFromID(ctx context.Context, client gateway.GatewayAPIClient return s.Share, nil } -func getUnmountedShares(ctx context.Context, gwc gateway.GatewayAPIClient, id *provider.ResourceId) ([]*collaboration.ReceivedShare, error) { - var unmountedShares []*collaboration.ReceivedShare - receivedShares, err := listReceivedShares(ctx, gwc) - if err != nil { - return unmountedShares, err - } - - for _, s := range receivedShares { - resourceIDEqual := utils.ResourceIDEqual(s.GetShare().GetResourceId(), id) - - if resourceIDEqual && s.State != collaboration.ShareState_SHARE_STATE_ACCEPTED { - // a share to the resource already exists but is not mounted, collect the unmounted share - unmountedShares = append(unmountedShares, s) - } - } - - return unmountedShares, err -} - // getSharedResource attempts to get a shared resource from the storage from the resource reference. func getSharedResource(ctx context.Context, client gateway.GatewayAPIClient, resID *provider.ResourceId) (*provider.StatResponse, *response.Response) { res, err := client.Stat(ctx, &provider.StatRequest{ diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go index 84fc9ce559..a2043c8489 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go +++ b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending_test.go @@ -24,7 +24,6 @@ import ( 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" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/go-chi/chi/v5" @@ -259,7 +258,7 @@ var _ = Describe("The ocs API", func() { It("accepts both pending shares", func() { client.On("UpdateReceivedShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateReceivedShareRequest) bool { - return req.Share.Share.Id.OpaqueId == "1" + return req.Share.Share.Id.OpaqueId == "1" && req.Share.MountPoint.Path == "share1" })).Return(&collaboration.UpdateReceivedShareResponse{ Status: status.NewOK(context.Background()), Share: &collaboration.ReceivedShare{ @@ -270,7 +269,7 @@ var _ = Describe("The ocs API", func() { }, nil) client.On("UpdateReceivedShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateReceivedShareRequest) bool { - return req.Share.Share.Id.OpaqueId == "2" + return req.Share.Share.Id.OpaqueId == "2" && req.Share.MountPoint.Path == "share1" })).Return(&collaboration.UpdateReceivedShareResponse{ Status: status.NewOK(context.Background()), Share: &collaboration.ReceivedShare{ @@ -320,7 +319,7 @@ var _ = Describe("The ocs API", func() { It("accepts the remaining pending share", func() { client.On("UpdateReceivedShare", mock.Anything, mock.MatchedBy(func(req *collaboration.UpdateReceivedShareRequest) bool { - return req.Share.Share.Id.OpaqueId == "2" + return req.Share.Share.Id.OpaqueId == "2" && req.Share.MountPoint.Path == "existing/mountpoint" })).Return(&collaboration.UpdateReceivedShareResponse{ Status: status.NewOK(context.Background()), Share: &collaboration.ReceivedShare{ @@ -417,26 +416,6 @@ var _ = Describe("The ocs API", func() { }, }, nil) }) - - DescribeTable("Resolve unmounted shares", - func(info *provider.ResourceInfo, unmountedLen int) { - // GetMountpointAndUnmountedShares check the error Stat response only - client.On("Stat", mock.Anything, mock.Anything). - Return(&provider.StatResponse{Status: &rpc.Status{Code: rpc.Code_CODE_OK}, - Info: &provider.ResourceInfo{}}, nil) - unmounted, err := shares.GetUnmountedShares(ctx, client, info.GetId()) - Expect(len(unmounted)).To(Equal(unmountedLen)) - Expect(err).To(BeNil()) - }, - Entry("new mountpoint, name changed", &provider.ResourceInfo{ - Name: "b.txt", - Id: &provider.ResourceId{StorageId: storage, OpaqueId: "not-exist", SpaceId: userOneSpaceId}, - }, 0), - Entry("unmountpoint", &provider.ResourceInfo{ - Name: "b.txt", - Id: &provider.ResourceId{StorageId: storage, OpaqueId: "be098d47-4518-4a96-92e3-52e904b3958d", SpaceId: userOneSpaceId}, - }, 1), - ) }) }) })