From d3d4b1b2019151f9a10c58066365e0d3cace3fc8 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 14 Feb 2024 17:00:15 +0100 Subject: [PATCH 1/3] fix(sharesstorageprovider): Remove superfluous Stat call The fetchShares call is already stat'ing the resource a few lines above. If that didn't return and info, why should it do here? --- .../sharesstorageprovider.go | 36 +++---------------- 1 file changed, 4 insertions(+), 32 deletions(-) diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go index da3624fd7a..49ffb87df3 100644 --- a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go +++ b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go @@ -810,11 +810,6 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer return nil, errors.Wrap(err, "sharesstorageprovider: error calling ListReceivedSharesRequest") } - gatewayClient, err := s.gatewaySelector.Next() - if err != nil { - return nil, err - } - infos := []*provider.ResourceInfo{} for _, share := range receivedShares { if share.GetState() != collaboration.ShareState_SHARE_STATE_ACCEPTED { @@ -823,33 +818,10 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer info := shareMd[share.GetShare().GetId().GetOpaqueId()] if info == nil { - if share.GetShare().GetResourceId().GetSpaceId() == "" { - // convert backwards compatible share id - share.Share.ResourceId.StorageId, share.Share.ResourceId.SpaceId = storagespace.SplitStorageID(share.GetShare().GetResourceId().GetSpaceId()) - } - statRes, err := gatewayClient.Stat(ctx, &provider.StatRequest{ - Opaque: req.Opaque, - Ref: &provider.Reference{ - ResourceId: share.Share.ResourceId, - Path: ".", - }, - ArbitraryMetadataKeys: req.ArbitraryMetadataKeys, - }) - switch { - case err != nil: - appctx.GetLogger(ctx).Error(). - Err(err). - Interface("share", share). - Msg("sharesstorageprovider: could not make stat request when listing virtual root, skipping") - continue - case statRes.Status.Code != rpc.Code_CODE_OK: - appctx.GetLogger(ctx).Debug(). - Interface("share", share). - Interface("status", statRes.Status). - Msg("sharesstorageprovider: could not stat share when listing virtual root, skipping") - continue - } - info = statRes.Info + appctx.GetLogger(ctx).Debug(). + Interface("share", share). + Msg("sharesstorageprovider: no resource info for share") + continue } // override resource id info From 43e5fad1a84a64034f975200a6e3bdf1561d772f Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 15 Feb 2024 17:53:23 +0100 Subject: [PATCH 2/3] fix(sharesstorageprovider): Testcase for merged share permission --- .../sharesstorageprovider_test.go | 151 ++++++++---------- 1 file changed, 71 insertions(+), 80 deletions(-) diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go index eac7ac8035..21508b41b3 100644 --- a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go +++ b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go @@ -240,6 +240,12 @@ var _ = Describe("Sharesstorageprovider", func() { }, } case ".": + permissionSet := &sprovider.ResourcePermissions{ + Stat: true, + } + if req.Ref.ResourceId.OpaqueId == "shareddir-merged" { + permissionSet.ListContainer = true + } return &sprovider.StatResponse{ Status: status.NewOK(context.Background()), Info: &sprovider.ResourceInfo{ @@ -248,12 +254,10 @@ var _ = Describe("Sharesstorageprovider", func() { Id: &sprovider.ResourceId{ StorageId: "share1-storageid", SpaceId: "share1-storageid", - OpaqueId: "shareddir", - }, - PermissionSet: &sprovider.ResourcePermissions{ - Stat: true, + OpaqueId: req.Ref.ResourceId.OpaqueId, }, - Size: 100, + PermissionSet: permissionSet, + Size: 100, }, } default: @@ -468,81 +472,6 @@ var _ = Describe("Sharesstorageprovider", func() { Expect(res.Info.Size).To(Equal(uint64(100))) }) - It("merges permissions from multiple shares", func() { - s1 := &collaboration.ReceivedShare{ - State: collaboration.ShareState_SHARE_STATE_ACCEPTED, - Share: &collaboration.Share{ - Id: &collaboration.ShareId{ - OpaqueId: "multishare1", - }, - ResourceId: &sprovider.ResourceId{ - StorageId: "share1-storageid", - SpaceId: "share1-storageid", - OpaqueId: "shareddir", - }, - Permissions: &collaboration.SharePermissions{ - Permissions: &sprovider.ResourcePermissions{ - Stat: true, - }, - }, - }, - MountPoint: &sprovider.Reference{Path: "share1-shareddir"}, - } - s2 := &collaboration.ReceivedShare{ - State: collaboration.ShareState_SHARE_STATE_ACCEPTED, - Share: &collaboration.Share{ - Id: &collaboration.ShareId{ - OpaqueId: "multishare2", - }, - ResourceId: &sprovider.ResourceId{ - StorageId: "share1-storageid", - SpaceId: "share1-storageid", - OpaqueId: "shareddir", - }, - Permissions: &collaboration.SharePermissions{ - Permissions: &sprovider.ResourcePermissions{ - ListContainer: true, - }, - }, - }, - MountPoint: &sprovider.Reference{Path: "share2-shareddir"}, - } - - sharingCollaborationClient.On("ListReceivedShares", mock.Anything, mock.Anything).Return(&collaboration.ListReceivedSharesResponse{ - Status: status.NewOK(context.Background()), - Shares: []*collaboration.ReceivedShare{s1, s2}, - }, nil) - sharingCollaborationClient.On("GetReceivedShare", mock.Anything, mock.Anything).Return( - func(_ context.Context, req *collaboration.GetReceivedShareRequest, _ ...grpc.CallOption) *collaboration.GetReceivedShareResponse { - switch req.Ref.GetId().GetOpaqueId() { - case BaseShare.Share.Id.OpaqueId: - return &collaboration.GetReceivedShareResponse{ - Status: status.NewOK(context.Background()), - Share: s1, - } - case BaseShareTwo.Share.Id.OpaqueId: - return &collaboration.GetReceivedShareResponse{ - Status: status.NewOK(context.Background()), - Share: s2, - } - default: - return &collaboration.GetReceivedShareResponse{ - Status: status.NewNotFound(context.Background(), "not found"), - } - } - }, nil) - - res, err := s.Stat(ctx, BaseStatRequest) - Expect(err).ToNot(HaveOccurred()) - Expect(res).ToNot(BeNil()) - Expect(res.Info).ToNot(BeNil()) - Expect(res.Status.Code).To(Equal(rpc.Code_CODE_OK)) - Expect(res.Info.Type).To(Equal(sprovider.ResourceType_RESOURCE_TYPE_CONTAINER)) - Expect(res.Info.Path).To(Equal("share1-shareddir")) - Expect(res.Info.PermissionSet.Stat).To(BeTrue()) - // Expect(res.Info.PermissionSet.ListContainer).To(BeTrue()) // TODO reenable - }) - It("stats a subfolder in a share", func() { statReq := BaseStatRequest statReq.Ref.Path = "./share1-subdir" @@ -1000,4 +929,66 @@ var _ = Describe("Sharesstorageprovider", func() { }) }) }) + Context("with two accepted shares for the same resource", func() { + BeforeEach(func() { + BaseShare.Share.Id.OpaqueId = "multishare1" + BaseShare.Share.ResourceId = &sprovider.ResourceId{ + StorageId: "share1-storageid", + SpaceId: "share1-storageid", + OpaqueId: "shareddir-merged", + } + BaseShare.Share.Permissions = &collaboration.SharePermissions{ + Permissions: &sprovider.ResourcePermissions{ + Stat: true, + }, + } + BaseShare.MountPoint = &sprovider.Reference{Path: "share1-shareddir"} + BaseShareTwo.Share.Id.OpaqueId = "multishare2" + BaseShareTwo.Share.ResourceId = BaseShare.Share.ResourceId + BaseShareTwo.Share.Permissions = &collaboration.SharePermissions{ + Permissions: &sprovider.ResourcePermissions{ + ListContainer: true, + }, + } + BaseShareTwo.MountPoint = BaseShare.MountPoint + + sharingCollaborationClient.On("ListReceivedShares", mock.Anything, mock.Anything).Return(&collaboration.ListReceivedSharesResponse{ + Status: status.NewOK(context.Background()), + Shares: []*collaboration.ReceivedShare{BaseShare, BaseShareTwo}, + }, nil) + sharingCollaborationClient.On("GetReceivedShare", mock.Anything, mock.Anything).Return( + func(_ context.Context, req *collaboration.GetReceivedShareRequest, _ ...grpc.CallOption) *collaboration.GetReceivedShareResponse { + switch req.Ref.GetId().GetOpaqueId() { + case BaseShare.Share.Id.OpaqueId: + return &collaboration.GetReceivedShareResponse{ + Status: status.NewOK(context.Background()), + Share: BaseShare, + } + case BaseShareTwo.Share.Id.OpaqueId: + return &collaboration.GetReceivedShareResponse{ + Status: status.NewOK(context.Background()), + Share: BaseShareTwo, + } + default: + return &collaboration.GetReceivedShareResponse{ + Status: status.NewNotFound(context.Background(), "not found"), + } + } + }, nil) + }) + Describe("Stat", func() { + It("merges permissions from multiple shares", func() { + BaseStatRequest.Ref.ResourceId.OpaqueId = "multishare2" + res, err := s.Stat(ctx, BaseStatRequest) + Expect(err).ToNot(HaveOccurred()) + Expect(res).ToNot(BeNil()) + Expect(res.Info).ToNot(BeNil()) + Expect(res.Status.Code).To(Equal(rpc.Code_CODE_OK)) + Expect(res.Info.Type).To(Equal(sprovider.ResourceType_RESOURCE_TYPE_CONTAINER)) + Expect(res.Info.Path).To(Equal("share1-shareddir")) + Expect(res.Info.PermissionSet.Stat).To(BeTrue()) + Expect(res.Info.PermissionSet.ListContainer).To(BeTrue()) + }) + }) + }) }) From 62cb6cd94e83364ec2d1e4e52e743e8616ff4791 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 15 Feb 2024 18:07:55 +0100 Subject: [PATCH 3/3] fix(sharesstorageprovider): Merge shares of the same resourceid When listing the sharejail root, merge shares that target the same resource into a single resource. In order to avoid the resourceId changing randomly the id will be composed from the oldest accepted share that exist for the resource. Ideally we'd compose the resourceId based on the id of the shared resource, but that is currently not possible in a backwards compatible way. Some clients seem to rely on the fact that the resource ids in the sharejail contain valid shareids. Fixes: https://github.com/owncloud/ocis/issues/8080 --- .../fix-duplicate-sharejail-items.md | 7 ++++ .../sharesstorageprovider.go | 24 ++++++++++-- .../sharesstorageprovider_test.go | 38 +++++++++++++++++++ 3 files changed, 65 insertions(+), 4 deletions(-) create mode 100644 changelog/unreleased/fix-duplicate-sharejail-items.md diff --git a/changelog/unreleased/fix-duplicate-sharejail-items.md b/changelog/unreleased/fix-duplicate-sharejail-items.md new file mode 100644 index 0000000000..7f20e5e1fd --- /dev/null +++ b/changelog/unreleased/fix-duplicate-sharejail-items.md @@ -0,0 +1,7 @@ +Bugfix: Fix duplicated items in the sharejail root + +We fixed a bug, that caused duplicate items to listed in the sharejail, when +a user received multiple shares for the same resource. + +https://github.com/cs3org/reva/pull/4517 +https://github.com/owncloud/ocis/issues/8080 diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go index 49ffb87df3..8d5316c6d9 100644 --- a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go +++ b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go @@ -810,12 +810,28 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer return nil, errors.Wrap(err, "sharesstorageprovider: error calling ListReceivedSharesRequest") } - infos := []*provider.ResourceInfo{} - for _, share := range receivedShares { - if share.GetState() != collaboration.ShareState_SHARE_STATE_ACCEPTED { + // Create map of shares that contains only the oldest share per shared resource. This is to avoid + // returning multiple resourceInfos for the same resource. But still be able to maintain a + // "somewhat" stable resourceID + oldestReceivedSharesByResourceID := make(map[string]*collaboration.ReceivedShare, len(receivedShares)) + for _, receivedShare := range receivedShares { + if receivedShare.GetState() != collaboration.ShareState_SHARE_STATE_ACCEPTED { continue } + rIDStr := storagespace.FormatResourceID(*receivedShare.GetShare().GetResourceId()) + if oldest, ok := oldestReceivedSharesByResourceID[rIDStr]; ok { + // replace if older than current oldest + if utils.TSToTime(receivedShare.GetShare().GetCtime()).Before(utils.TSToTime(oldest.GetShare().GetCtime())) { + oldestReceivedSharesByResourceID[rIDStr] = receivedShare + } + } else { + oldestReceivedSharesByResourceID[rIDStr] = receivedShare + } + } + // now compose the resourceInfos for the unified list of shares + infos := []*provider.ResourceInfo{} + for _, share := range oldestReceivedSharesByResourceID { info := shareMd[share.GetShare().GetId().GetOpaqueId()] if info == nil { appctx.GetLogger(ctx).Debug(). @@ -828,7 +844,7 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer info.Id = &provider.ResourceId{ StorageId: utils.ShareStorageProviderID, SpaceId: utils.ShareStorageSpaceID, - OpaqueId: share.Share.Id.OpaqueId, + OpaqueId: share.GetShare().GetId().GetOpaqueId(), } info.Path = filepath.Base(share.MountPoint.Path) info.Name = info.Path diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go index 21508b41b3..9ff1f8592a 100644 --- a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go +++ b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go @@ -261,6 +261,26 @@ var _ = Describe("Sharesstorageprovider", func() { }, } default: + if req.Ref.ResourceId.OpaqueId == "shareddir-merged" { + permissionSet := &sprovider.ResourcePermissions{ + Stat: true, + ListContainer: true, + } + return &sprovider.StatResponse{ + Status: status.NewOK(context.Background()), + Info: &sprovider.ResourceInfo{ + Type: sprovider.ResourceType_RESOURCE_TYPE_CONTAINER, + Path: "share1-shareddir", + Id: &sprovider.ResourceId{ + StorageId: "share1-storageid", + SpaceId: "share1-storageid", + OpaqueId: req.Ref.ResourceId.OpaqueId, + }, + PermissionSet: permissionSet, + Size: 100, + }, + } + } return &sprovider.StatResponse{ Status: status.NewNotFound(context.Background(), "not found"), } @@ -942,6 +962,7 @@ var _ = Describe("Sharesstorageprovider", func() { Stat: true, }, } + BaseShare.Share.Ctime = utils.TSNow() BaseShare.MountPoint = &sprovider.Reference{Path: "share1-shareddir"} BaseShareTwo.Share.Id.OpaqueId = "multishare2" BaseShareTwo.Share.ResourceId = BaseShare.Share.ResourceId @@ -951,6 +972,7 @@ var _ = Describe("Sharesstorageprovider", func() { }, } BaseShareTwo.MountPoint = BaseShare.MountPoint + BaseShareTwo.Share.Ctime = utils.TSNow() sharingCollaborationClient.On("ListReceivedShares", mock.Anything, mock.Anything).Return(&collaboration.ListReceivedSharesResponse{ Status: status.NewOK(context.Background()), @@ -990,5 +1012,21 @@ var _ = Describe("Sharesstorageprovider", func() { Expect(res.Info.PermissionSet.ListContainer).To(BeTrue()) }) }) + Describe("ListContainer", func() { + It("Returns just one item", func() { + req := BaseListContainerRequest + req.Ref.ResourceId.OpaqueId = utils.ShareStorageSpaceID + req.Ref.Path = "" + res, err := s.ListContainer(ctx, req) + Expect(err).ToNot(HaveOccurred()) + Expect(res).ToNot(BeNil()) + Expect(res.Status.Code).To(Equal(rpc.Code_CODE_OK)) + Expect(len(res.Infos)).To(Equal(1)) + + entry := res.Infos[0] + Expect(entry.Path).To(Equal("share1-shareddir")) + }) + }) + }) })