From 74ce1c722feaf80ff8a9e313e2640baa888c4bf7 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 15 Feb 2024 18:07:55 +0100 Subject: [PATCH] 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 00000000000..7f20e5e1fd2 --- /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 49ffb87df33..8d5316c6d93 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 21508b41b38..9ff1f8592a4 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")) + }) + }) + }) })