From 0d3debce8238ebcf1f76245a0ba5d50fbd67368c Mon Sep 17 00:00:00 2001 From: Roman Perekhod Date: Mon, 2 Oct 2023 08:07:54 +0200 Subject: [PATCH] [full-ci] fix issue-6739 --- changelog/unreleased/fix-move-copy.md | 7 ++ internal/http/services/owncloud/ocdav/copy.go | 11 ++- internal/http/services/owncloud/ocdav/move.go | 25 +++--- .../http/services/owncloud/ocdav/ocdav.go | 6 ++ .../owncloud/ocdav/ocdav_blackbox_test.go | 84 ++++++++++--------- 5 files changed, 78 insertions(+), 55 deletions(-) create mode 100644 changelog/unreleased/fix-move-copy.md diff --git a/changelog/unreleased/fix-move-copy.md b/changelog/unreleased/fix-move-copy.md new file mode 100644 index 0000000000..b5ec1fc395 --- /dev/null +++ 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 index 7b4bce24ee..7415b9cd0f 100644 --- 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 index 65d298c244..d0550ae624 100644 --- 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 index a96d11ebf4..840f6fe3e3 100644 --- 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 index 783083dfca..d46e06267e 100644 --- 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)) }) }) - }) })