Change invalid move detection to cover corner cases

cs3org · Nov 6, 2023 · f3863bb · f3863bb
1 parent a81237c
commit f3863bb
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 19 deletions.
diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go
@@ -591,12 +591,6 @@ func (s *service) Move(ctx context.Context, req *provider.MoveRequest) (*provide
 		Interface("destination", req.Destination).
 		Msg("sharesstorageprovider: Got Move request")
 
-	if !utils.ResourceIDEqual(req.Source.ResourceId, req.Destination.ResourceId) {
-		return &provider.MoveResponse{
-			Status: status.NewUnimplemented(ctx, nil, "sharesstorageprovider: can not move between shares"),
-		}, nil
-	}
-
 	// TODO moving inside a shared tree should just be a forward of the move
 	//      but when do we rename a mounted share? Does that request even hit us?
 	//      - the registry needs to invalidate the alias
@@ -650,6 +644,12 @@ func (s *service) Move(ctx context.Context, req *provider.MoveRequest) (*provide
 		}, nil
 	}
 
+	if dstReceivedShare.Share.Id.OpaqueId != srcReceivedShare.Share.Id.OpaqueId {
+		return &provider.MoveResponse{
+			Status: status.NewUnimplemented(ctx, nil, "sharesstorageprovider: can not move between shares"),
+		}, nil
+	}
+
 	gatewayClient, err := s.gatewaySelector.Next()
 	if err != nil {
 		return nil, err

diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider_test.go
@@ -89,8 +89,8 @@ var _ = Describe("Sharesstorageprovider", func() {
 					OpaqueId: "shareid",
 				},
 				ResourceId: &sprovider.ResourceId{
-					StorageId: "share1-storageid",
-					SpaceId:   "share1-storageid",
+					StorageId: "storageid",
+					SpaceId:   "storageid",
 					OpaqueId:  "shareddir",
 				},
 				Permissions: &collaboration.SharePermissions{
@@ -102,20 +102,20 @@ var _ = Describe("Sharesstorageprovider", func() {
 				},
 			},
 			MountPoint: &sprovider.Reference{
-				Path: "oldname",
+				Path: "share1-shareddir",
 			},
 		}
 
 		BaseShareTwo = &collaboration.ReceivedShare{
 			State: collaboration.ShareState_SHARE_STATE_ACCEPTED,
 			Share: &collaboration.Share{
 				Id: &collaboration.ShareId{
-					OpaqueId: "shareidtwo",
+					OpaqueId: "shareid2",
 				},
 				ResourceId: &sprovider.ResourceId{
-					StorageId: "share1-storageid",
-					SpaceId:   "share1-storageid",
-					OpaqueId:  "shareddir",
+					StorageId: "storageid",
+					SpaceId:   "storageid",
+					OpaqueId:  "shareddir2",
 				},
 				Permissions: &collaboration.SharePermissions{
 					Permissions: &sprovider.ResourcePermissions{
@@ -126,7 +126,7 @@ var _ = Describe("Sharesstorageprovider", func() {
 				},
 			},
 			MountPoint: &sprovider.Reference{
-				Path: "share1-shareddir",
+				Path: "share2-shareddir2",
 			},
 		}
 
@@ -298,6 +298,12 @@ var _ = Describe("Sharesstorageprovider", func() {
 						})
 					}
 					return resp
+				case utils.ResourceIDEqual(req.Ref.ResourceId, BaseShareTwo.Share.ResourceId):
+					resp := &sprovider.ListContainerResponse{
+						Status: status.NewOK(context.Background()),
+						Infos:  []*sprovider.ResourceInfo{},
+					}
+					return resp
 				case utils.ResourceIDEqual(req.Ref.ResourceId, BaseShareTwo.Share.ResourceId):
 					return &sprovider.ListContainerResponse{
 						Status: status.NewOK(context.Background()),
@@ -772,11 +778,11 @@ var _ = Describe("Sharesstorageprovider", func() {
 				req := &sprovider.MoveRequest{
 					Source: &sprovider.Reference{
 						ResourceId: ShareJail,
-						Path:       "./oldname",
+						Path:       "./share1-shareddir",
 					},
 					Destination: &sprovider.Reference{
 						ResourceId: ShareJail,
-						Path:       "./newname",
+						Path:       "./share1-shareddir-renamed",
 					},
 				}
 				res, err := s.Move(ctx, req)
@@ -790,17 +796,37 @@ var _ = Describe("Sharesstorageprovider", func() {
 			It("refuses to move a file between shares", func() {
 				req := &sprovider.MoveRequest{
 					Source: &sprovider.Reference{
-						Path: "/shares/share1-shareddir/share1-shareddir-file",
+						ResourceId: ShareJail,
+						Path:       "./share1-shareddir/share1-shareddir-file",
 					},
 					Destination: &sprovider.Reference{
-						Path: "/shares/share2-shareddir/share2-shareddir-file",
+						ResourceId: ShareJail,
+						Path:       "./share2-shareddir2/share1-shareddir-file",
 					},
 				}
 				res, err := s.Move(ctx, req)
 				gatewayClient.AssertNotCalled(GinkgoT(), "Move", mock.Anything, mock.Anything)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(res).ToNot(BeNil())
-				Expect(res.Status.Code).To(Equal(rpc.Code_CODE_INVALID_ARGUMENT))
+				Expect(res.Status.Code).To(Equal(rpc.Code_CODE_UNIMPLEMENTED))
+			})
+
+			It("refuses to move a file between shares resolving to the same space", func() {
+				req := &sprovider.MoveRequest{
+					Source: &sprovider.Reference{
+						ResourceId: ShareJail,
+						Path:       "./share1-shareddir/share1-shareddir-file",
+					},
+					Destination: &sprovider.Reference{
+						ResourceId: ShareJail,
+						Path:       "./share2-shareddir2/share1-shareddir-file",
+					},
+				}
+				res, err := s.Move(ctx, req)
+				gatewayClient.AssertNotCalled(GinkgoT(), "Move", mock.Anything, mock.Anything)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(res).ToNot(BeNil())
+				Expect(res.Status.Code).To(Equal(rpc.Code_CODE_UNIMPLEMENTED))
 			})
 
 			It("moves a file", func() {