Merge pull request #4318 from aduffeck/disallow-moves

Disallow moves
cs3org · Nov 7, 2023 · 4b08a8b · 4b08a8b
2 parents d3831fd + f3863bb
commit 4b08a8b
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 15 deletions.
diff --git a/changelog/unreleased/disallow-moves-between-shares.md b/changelog/unreleased/disallow-moves-between-shares.md
@@ -0,0 +1,5 @@
+Bugfix: Do not allow moves between shares
+
+We no longer allow moves between shares, even if they resolve to the same space.
+
+https://github.com/cs3org/reva/pull/4318
diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go
@@ -643,9 +643,10 @@ func (s *service) Move(ctx context.Context, req *provider.MoveRequest) (*provide
 			Status: rpcStatus,
 		}, nil
 	}
-	if srcReceivedShare.Share.ResourceId.SpaceId != dstReceivedShare.Share.ResourceId.SpaceId {
+
+	if dstReceivedShare.Share.Id.OpaqueId != srcReceivedShare.Share.Id.OpaqueId {
 		return &provider.MoveResponse{
-			Status: status.NewInvalid(ctx, "sharesstorageprovider: can not move between shares on different storages"),
+			Status: status.NewUnimplemented(ctx, nil, "sharesstorageprovider: can not move between shares"),
 		}, nil
 	}
 

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() {