[full-ci] fix issue-6739

cs3org · Oct 4, 2023 · 0d3debc · 0d3debc
1 parent 0411450
commit 0d3debc
Show file tree

Hide file tree

Showing 5 changed files with 78 additions and 55 deletions.
diff --git a/changelog/unreleased/fix-move-copy.md b/changelog/unreleased/fix-move-copy.md
@@ -0,0 +1,7 @@
+Bugfix: Fix destroying the Personal and Project spaces data
+
+We fixed a bug that caused destroying the Personal and Project spaces data when providing as a destination while move/copy file.
+Disallow use the Personal and Project spaces root as a source while move/copy file.
+
+https://github.com/cs3org/reva/pull/4229
+https://github.com/owncloud/ocis/issues/6739
diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go
@@ -608,6 +608,11 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
 		errors.HandleErrorStatus(log, w, srcStatRes.Status)
 		return nil
 	}
+	if isSpaceRoot(srcStatRes.GetInfo()) {
+		log.Error().Msg("the source is disallowed")
+		w.WriteHeader(http.StatusBadRequest)
+		return nil
+	}
 
 	dstStatReq := &provider.StatRequest{Ref: dstRef}
 	dstStatRes, err := client.Stat(ctx, dstStatReq)
@@ -625,6 +630,11 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
 	if dstStatRes.Status.Code == rpc.Code_CODE_OK {
 		successCode = http.StatusNoContent // 204 if target already existed, see https://tools.ietf.org/html/rfc4918#section-9.8.5
 
+		if isSpaceRoot(dstStatRes.GetInfo()) {
+			log.Error().Msg("overwriting is not allowed")
+			w.WriteHeader(http.StatusBadRequest)
+			return nil
+		}
 		if !overwrite {
 			log.Warn().Bool("overwrite", overwrite).Msg("dst already exists")
 			w.WriteHeader(http.StatusPreconditionFailed)
@@ -633,7 +643,6 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
 			errors.HandleWebdavError(log, w, b, err) // 412, see https://tools.ietf.org/html/rfc4918#section-9.8.5
 			return nil
 		}
-
 		// delete existing tree when overwriting a directory or replacing a file with a directory
 		if dstStatRes.Info.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER ||
 			(dstStatRes.Info.Type == provider.ResourceType_RESOURCE_TYPE_FILE &&

diff --git a/internal/http/services/owncloud/ocdav/move.go b/internal/http/services/owncloud/ocdav/move.go
@@ -141,12 +141,6 @@ func (s *svc) handleSpacesMove(w http.ResponseWriter, r *http.Request, srcSpaceI
 }
 
 func (s *svc) handleMove(ctx context.Context, w http.ResponseWriter, r *http.Request, src, dst *provider.Reference, log zerolog.Logger) {
-	// do not allow overwriting spaces
-	if err := s.validateDestination(dst); err != nil {
-		log.Error().Err(err)
-		w.WriteHeader(http.StatusPreconditionFailed) // 412, see https://tools.ietf.org/html/rfc4918#section-9.9.4
-		return
-	}
 	isChild, err := s.referenceIsChildOf(ctx, s.gatewaySelector, dst, src)
 	if err != nil {
 		switch err.(type) {
@@ -200,6 +194,11 @@ func (s *svc) handleMove(ctx context.Context, w http.ResponseWriter, r *http.Req
 		errors.HandleErrorStatus(&log, w, srcStatRes.Status)
 		return
 	}
+	if isSpaceRoot(srcStatRes.GetInfo()) {
+		log.Error().Msg("the source is disallowed")
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
 
 	// check dst exists
 	dstStatReq := &provider.StatRequest{Ref: dst}
@@ -218,12 +217,16 @@ func (s *svc) handleMove(ctx context.Context, w http.ResponseWriter, r *http.Req
 	if dstStatRes.Status.Code == rpc.Code_CODE_OK {
 		successCode = http.StatusNoContent // 204 if target already existed, see https://tools.ietf.org/html/rfc4918#section-9.9.4
 
+		if isSpaceRoot(dstStatRes.GetInfo()) {
+			log.Error().Msg("overwriting is not allowed")
+			w.WriteHeader(http.StatusBadRequest)
+			return
+		}
 		if !overwrite {
 			log.Warn().Bool("overwrite", overwrite).Msg("dst already exists")
 			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)
@@ -311,11 +314,3 @@ 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.Reference) error {
-	// do not allow overwriting spaces
-	if dstStatRes.GetPath() == "." && dstStatRes.GetResourceId().GetOpaqueId() == dstStatRes.GetResourceId().GetSpaceId() {
-		return fmt.Errorf("overwriting is not allowed")
-	}
-	return nil
-}
diff --git a/internal/http/services/owncloud/ocdav/ocdav.go b/internal/http/services/owncloud/ocdav/ocdav.go
@@ -413,3 +413,9 @@ func (s *svc) referenceIsChildOf(ctx context.Context, selector pool.Selectable[g
 	pp := path.Join(parentPathRes.Path, parent.Path) + "/"
 	return strings.HasPrefix(cp, pp), nil
 }
+
+func isSpaceRoot(info *provider.ResourceInfo) bool {
+	f := info.GetId()
+	s := info.GetSpace().GetRoot()
+	return f.GetOpaqueId() == s.GetOpaqueId() && f.GetSpaceId() == s.GetSpaceId()
+}
diff --git a/internal/http/services/owncloud/ocdav/ocdav_blackbox_test.go b/internal/http/services/owncloud/ocdav/ocdav_blackbox_test.go
@@ -602,7 +602,7 @@ var _ = Describe("ocdav", func() {
 				req, err = http.NewRequest("MOVE", basePath+"/file", strings.NewReader(""))
 				Expect(err).ToNot(HaveOccurred())
 				req = req.WithContext(ctx)
-				req.Header.Set("Destination", basePath+"/newfile")
+				req.Header.Set(net.HeaderDestination, basePath+"/newfile")
 				req.Header.Set("Overwrite", "T")
 
 				mReq = &cs3storageprovider.MoveRequest{
@@ -620,7 +620,7 @@ var _ = Describe("ocdav", func() {
 			When("the gateway returns OK when moving file", func() {
 				It("the source exists, the destination doesn't exists", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					client.On("Stat", mock.Anything, mock.Anything).Return(&cs3storageprovider.StatResponse{
 						Status: status.NewNotFound(ctx, ""),
 						Info:   &cs3storageprovider.ResourceInfo{},
@@ -680,7 +680,7 @@ var _ = Describe("ocdav", func() {
 
 				It("the destination Stat error", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 
 					client.On("Stat", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.StatRequest) bool {
 						return utils.ResourceEqual(req.Ref, mReq.Destination)
@@ -698,13 +698,13 @@ var _ = Describe("ocdav", func() {
 					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), nil)
 
 					handler.Handler().ServeHTTP(rr, req)
-					Expect(rr).To(HaveHTTPStatus(http.StatusPreconditionFailed))
+					Expect(rr).To(HaveHTTPStatus(http.StatusBadRequest))
 				})
 
 				It("error when deleting an existing tree", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Path: "./file"})
-					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Path: "./newfile"})
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId, Path: "./file"})
+					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Destination.ResourceId, Path: "./newfile"})
 
 					client.On("Delete", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.DeleteRequest) bool {
 						return utils.ResourceEqual(req.Ref, mReq.Destination)
@@ -716,7 +716,7 @@ var _ = Describe("ocdav", func() {
 
 				It("error when destination Stat returns unexpected code", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					mockPathStat(mReq.Destination.Path, status.NewInternal(ctx, ""), nil)
 
 					handler.Handler().ServeHTTP(rr, req)
@@ -725,8 +725,8 @@ var _ = Describe("ocdav", func() {
 
 				It("error when Delete returns unexpected code", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Path: "./file"})
-					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Path: "./newfile"})
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId, Path: "./file"})
+					mockPathStat(mReq.Destination.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Destination.ResourceId, Path: "./newfile"})
 
 					client.On("Delete", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.DeleteRequest) bool {
 						return utils.ResourceEqual(req.Ref, mReq.Destination)
@@ -739,7 +739,7 @@ var _ = Describe("ocdav", func() {
 
 				It("the destination Stat error", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					mockPathStat(mReq.Destination.Path, status.NewNotFound(ctx, ""), nil)
 					client.On("Stat", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.StatRequest) bool {
 						return utils.ResourceEqual(req.Ref, &cs3storageprovider.Reference{
@@ -754,7 +754,7 @@ var _ = Describe("ocdav", func() {
 
 				It("error when destination Stat is not found", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					mockPathStat(mReq.Destination.Path, status.NewNotFound(ctx, ""), nil)
 					mockPathStat(".", status.NewNotFound(ctx, ""), nil)
 
@@ -764,7 +764,7 @@ var _ = Describe("ocdav", func() {
 
 				It("an unexpected error when destination Stat", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					mockPathStat(mReq.Destination.Path, status.NewNotFound(ctx, ""), nil)
 					mockPathStat(".", status.NewInvalid(ctx, ""), nil)
 
@@ -774,7 +774,7 @@ var _ = Describe("ocdav", func() {
 
 				It("error when removing", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					mockPathStat(mReq.Destination.Path, status.NewNotFound(ctx, ""), nil)
 					mockPathStat(".", status.NewOK(ctx), nil)
 					client.On("Move", mock.Anything, mReq).Return(nil, fmt.Errorf("unexpected io error"))
@@ -794,12 +794,12 @@ var _ = Describe("ocdav", func() {
 					}, nil)
 
 					handler.Handler().ServeHTTP(rr, req)
-					Expect(rr).To(HaveHTTPStatus(http.StatusPreconditionFailed))
+					Expect(rr).To(HaveHTTPStatus(http.StatusBadRequest))
 				})
 
 				It("status 'Unimplemented' when removing", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					mockPathStat(mReq.Destination.Path, status.NewNotFound(ctx, ""), nil)
 					mockPathStat(".", status.NewOK(ctx), nil)
 
@@ -813,7 +813,7 @@ var _ = Describe("ocdav", func() {
 
 				It("the destination Stat error after moving", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					client.On("Stat", mock.Anything, mock.Anything).Return(&cs3storageprovider.StatResponse{
 						Status: status.NewNotFound(ctx, ""),
 						Info:   &cs3storageprovider.ResourceInfo{},
@@ -837,7 +837,7 @@ var _ = Describe("ocdav", func() {
 
 				It("the destination Stat returned not OK status after moving", func() {
 
-					mockPathStat(mReq.Source.Path, status.NewOK(ctx), nil)
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
 					mockPathStat(mReq.Destination.Path, status.NewNotFound(ctx, ""), nil)
 					mockPathStat(".", status.NewOK(ctx), nil)
 
@@ -853,44 +853,50 @@ var _ = Describe("ocdav", func() {
 			})
 		})
 
-		Describe("MOVE /dav/spaces failed", func() {
+		Describe("MOVE validation failed", func() {
 
 			BeforeEach(func() {
 				// setup the request
-				basePath = "/dav/spaces"
+				// set the webdav endpoint to test
+				basePath = "/webdav"
+				userspace.Id = &cs3storageprovider.StorageSpaceId{OpaqueId: storagespace.FormatResourceID(cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "userspace"})}
+				userspace.Root = &cs3storageprovider.ResourceId{StorageId: "provider-1", SpaceId: "userspace", OpaqueId: "userspace"}
+
+				// path based requests at the /webdav endpoint first look up the storage space
+				client.On("ListStorageSpaces", mock.Anything, mock.MatchedBy(func(req *cs3storageprovider.ListStorageSpacesRequest) bool {
+					p := string(req.Opaque.Map["path"].Value)
+					return p == "/" || strings.HasPrefix(p, "/users")
+				})).Return(&cs3storageprovider.ListStorageSpacesResponse{
+					Status:        status.NewOK(ctx),
+					StorageSpaces: []*cs3storageprovider.StorageSpace{userspace},
+				}, nil)
+
 				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!userspace")
+				req.Header.Set(net.HeaderDestination, basePath+"/provider-1$userspace!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: ".",
-					},
+					Source:      mockReference("userspace", "./file"),
+					Destination: mockReference("userspace", ""),
 				}
 			})
 
-			When("the gateway returns OK when moving file", func() {
+			When("the gateway returns error when moving file", func() {
+				It("error when the source is a file and the destination is a folder", func() {
+					mockPathStat(mReq.Source.Path, status.NewOK(ctx), &cs3storageprovider.ResourceInfo{Id: mReq.Source.ResourceId})
+
+					mockStat(mockReference("userspace", ""), status.NewOK(ctx), &cs3storageprovider.ResourceInfo{
+						Id: mReq.Destination.ResourceId, Path: mReq.Destination.Path,
+						Type:  cs3storageprovider.ResourceType_RESOURCE_TYPE_CONTAINER,
+						Space: userspace,
+					})
 
-				It("the source and the destination exist", func() {
 					handler.Handler().ServeHTTP(rr, req)
-					Expect(rr).To(HaveHTTPStatus(http.StatusPreconditionFailed))
+					Expect(rr).To(HaveHTTPStatus(http.StatusBadRequest))
 				})
 			})
-
 		})
 	})