Merge pull request #4517 from rhafer/ocisissue/8080

fix(sharesstorageprovider): Merge shares of the same resourceid

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

internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go

@@ -810,53 +810,41 @@ 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 {
+		// 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 {
-				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
 			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

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,15 +254,33 @@ var _ = Describe("Sharesstorageprovider", func() {
 							Id: &sprovider.ResourceId{
 								StorageId: "share1-storageid",
 								SpaceId:   "share1-storageid",
-								OpaqueId:  "shareddir",
+								OpaqueId:  req.Ref.ResourceId.OpaqueId,
 							},
-							PermissionSet: &sprovider.ResourcePermissions{
-								Stat: true,
-							},
-							Size: 100,
+							PermissionSet: permissionSet,
+							Size:          100,
 						},
 					}
 				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"),
 					}
@@ -468,81 +492,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 +949,84 @@ 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.Share.Ctime = utils.TSNow()
+			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
+			BaseShareTwo.Share.Ctime = utils.TSNow()
+
+			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())
+			})
+		})
+		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"))
+			})
+		})
+
+	})
 })