fix moving. do not allow overwriting spaces

cs3org · Jul 14, 2023 · fd54c11 · fd54c11
1 parent 7ee83f2
commit fd54c11
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 9 deletions.
diff --git a/changelog/unreleased/fix-moving.md b/changelog/unreleased/fix-moving.md
@@ -0,0 +1,6 @@
+Bugfix: fix destroys data destination when moving issue
+
+Fix moving a file and providing the fileID of the destination destroys data
+
+https://github.com/cs3org/reva/pull/4056
+https://github.com/owncloud/ocis/issues/6739
diff --git a/internal/http/services/owncloud/ocdav/move.go b/internal/http/services/owncloud/ocdav/move.go
@@ -218,6 +218,13 @@ func (s *svc) handleMove(ctx context.Context, w http.ResponseWriter, r *http.Req
 			return
 		}
 
+		// do not allow overwriting spaces
+		if err := s.validateDestination(dstStatRes); err != nil {
+			log.Warn().Bool("overwrite", overwrite).Msg(err.Error())
+			w.WriteHeader(http.StatusPreconditionFailed) // 412, see https://tools.ietf.org/html/rfc4918#section-9.9.4
+			return
+		}
+
 		// delete existing tree
 		delReq := &provider.DeleteRequest{Ref: dst}
 		delRes, err := client.Delete(ctx, delReq)
@@ -305,3 +312,14 @@ func (s *svc) handleMove(ctx context.Context, w http.ResponseWriter, r *http.Req
 	w.Header().Set(net.HeaderOCETag, info.Etag)
 	w.WriteHeader(successCode)
 }
+
+func (s *svc) validateDestination(dstStatRes *provider.StatResponse) error {
+	// do not allow overwriting spaces
+	if dstStatRes.GetInfo().GetPath() == "." {
+		return fmt.Errorf("overwriting spaces via dav is not allowed")
+	}
+	if dstStatRes.GetInfo().GetId().GetOpaqueId() != "" && dstStatRes.GetInfo().GetId().GetOpaqueId() == dstStatRes.GetInfo().GetId().GetSpaceId() {
+		return fmt.Errorf("overwriting is not allowed")
+	}
+	return nil
+}
diff --git a/internal/http/services/owncloud/ocdav/ocdav_blackbox_test.go b/internal/http/services/owncloud/ocdav/ocdav_blackbox_test.go
@@ -80,7 +80,7 @@ var _ = Describe("ocdav", func() {
 			})).Return(&cs3storageprovider.StatResponse{
 				Status: s,
 				Info:   info,
-			}, nil).Once()
+			}, nil)
 		}
 		mockStat = func(ref *cs3storageprovider.Reference, s *cs3rpc.Status, info *cs3storageprovider.ResourceInfo) {
 			client.On("Stat", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.StatRequest) bool {
@@ -620,7 +620,10 @@ var _ = Describe("ocdav", func() {
 				It("the source exists, the destination doesn't exists", func() {
 
 					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
-					mockPathStat(mReq.Destination.Path, status.NewNotFound(ctx, ""), nil)
+					client.On("Stat", mock.Anything, mock.Anything).Return(&cs3storageprovider.StatResponse{
+						Status: status.NewNotFound(ctx, ""),
+						Info:   &cs3storageprovider.ResourceInfo{},
+					}, nil).Once()
 					mockPathStat(".", status.NewOK(ctx), nil)
 
 					client.On("Move", mock.Anything, mReq).Return(&cs3storageprovider.MoveResponse{
@@ -635,8 +638,8 @@ var _ = Describe("ocdav", func() {
 
 				It("the source and the destination exist", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
-					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
+					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Destination.ResourceId})
 
 					client.On("Delete", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.DeleteRequest) bool {
 						return utils.ResourceEqual(req.Ref, mReq.Destination)
@@ -699,8 +702,8 @@ var _ = Describe("ocdav", func() {
 
 				It("error when deleting an existing tree", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
-					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Path: "./file"})
+					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Path: "./newfile"})
 
 					client.On("Delete", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.DeleteRequest) bool {
 						return utils.ResourceEqual(req.Ref, mReq.Destination)
@@ -721,8 +724,8 @@ var _ = Describe("ocdav", func() {
 
 				It("error when Delete returns unexpected code", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
-					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Path: "./file"})
+					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Path: "./newfile"})
 
 					client.On("Delete", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.DeleteRequest) bool {
 						return utils.ResourceEqual(req.Ref, mReq.Destination)
@@ -810,7 +813,10 @@ var _ = Describe("ocdav", func() {
 				It("the destination Stat error after moving", func() {
 
 					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
-					mockPathStat(mReq.Destination.Path, status.NewNotFound(ctx, ""), nil)
+					client.On("Stat", mock.Anything, mock.Anything).Return(&cs3storageprovider.StatResponse{
+						Status: status.NewNotFound(ctx, ""),
+						Info:   &cs3storageprovider.ResourceInfo{},
+					}, nil).Once()
 					mockPathStat(".", status.NewOK(ctx), nil)
 
 					client.On("Move", mock.Anything, mReq).Return(&cs3storageprovider.MoveResponse{
@@ -845,6 +851,50 @@ var _ = Describe("ocdav", func() {
 				})
 			})
 		})
+
+		Describe("MOVE /dav/spaces failed", func() {
+
+			BeforeEach(func() {
+				// setup the request
+				basePath = "/dav/spaces"
+				rr = httptest.NewRecorder()
+				req, err = http.NewRequest("MOVE", basePath+"/file", strings.NewReader(""))
+				Expect(err).ToNot(HaveOccurred())
+				req = req.WithContext(ctx)
+				req.Header.Set("Destination", basePath+"/provider-1$userspace")
+				req.Header.Set("Overwrite", "T")
+
+				mReq = &cs3storageprovider.MoveRequest{
+					Source: &cs3storageprovider.Reference{
+						ResourceId: &cs3storageprovider.ResourceId{
+							SpaceId: "userspace",
+						},
+						Path: "./file",
+					},
+					Destination: &cs3storageprovider.Reference{
+						ResourceId: &cs3storageprovider.ResourceId{
+							StorageId: "provider-1",
+							OpaqueId:  "userspace",
+							SpaceId:   "userspace",
+						},
+						Path: ".",
+					},
+				}
+			})
+
+			When("the gateway returns OK when moving file", func() {
+
+				It("the source and the destination exist", func() {
+
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
+					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Destination.ResourceId, Path: "."})
+
+					handler.Handler().ServeHTTP(rr, req)
+					Expect(rr).To(HaveHTTPStatus(http.StatusPreconditionFailed))
+				})
+			})
+
+		})
 	})
 
 	Context("at the /dav/avatars endpoint", func() {