Skip to content

Commit

Permalink
[full-ci] fix issue-6739
Browse files Browse the repository at this point in the history
  • Loading branch information
@2403905
2403905 committed Oct 2, 2023
1 parent 0411450 commit e20de4e
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 55 deletions.
7 changes: 7 additions & 0 deletions changelog/unreleased/fix-move-copy.md
View file
Original file line number Diff line number Diff line change
@@ -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
11 changes: 10 additions & 1 deletion internal/http/services/owncloud/ocdav/copy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 err := validateSrc(srcStatRes.GetInfo()); err != nil {
log.Err(err).Send()
w.WriteHeader(http.StatusBadRequest)
return nil
}

dstStatReq := &provider.StatRequest{Ref: dstRef}
dstStatRes, err := client.Stat(ctx, dstStatReq)
Expand All @@ -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 err := validateDst(dstStatRes.GetInfo()); err != nil {
log.Err(err).Send()
w.WriteHeader(http.StatusBadRequest)
return nil
}
if !overwrite {
log.Warn().Bool("overwrite", overwrite).Msg("dst already exists")
w.WriteHeader(http.StatusPreconditionFailed)
Expand All @@ -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 &&
Expand Down
25 changes: 10 additions & 15 deletions internal/http/services/owncloud/ocdav/move.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 err := validateSrc(srcStatRes.GetInfo()); err != nil {
log.Err(err).Send()
w.WriteHeader(http.StatusBadRequest)
return
}

// check dst exists
dstStatReq := &provider.StatRequest{Ref: dst}
Expand All @@ -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 err := validateDst(dstStatRes.GetInfo()); err != nil {
log.Err(err).Send()
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)
Expand Down Expand Up @@ -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
}
21 changes: 21 additions & 0 deletions internal/http/services/owncloud/ocdav/ocdav.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package ocdav

import (
"context"
"fmt"
"net/http"
"path"
"strings"
Expand Down Expand Up @@ -413,3 +414,23 @@ 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 validateSrc(srcInf *provider.ResourceInfo) error {
if isSpaceRoot(srcInf) {
return fmt.Errorf("the source is disallowed")
}
return nil
}

func validateDst(dstInf *provider.ResourceInfo) error {
if isSpaceRoot(dstInf) {
return fmt.Errorf("overwriting is not allowed")
}
return nil
}

func isSpaceRoot(info *provider.ResourceInfo) bool {
f := info.GetId()
s := info.GetSpace().GetRoot()
return f.GetOpaqueId() == s.GetOpaqueId() && f.GetSpaceId() == s.GetSpaceId()
}
84 changes: 45 additions & 39 deletions internal/http/services/owncloud/ocdav/ocdav_blackbox_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{},
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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{
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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"))
Expand All @@ -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)

Expand All @@ -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{},
Expand All @@ -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)

Expand All @@ -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))
})
})

})
})

Expand Down

0 comments on commit e20de4e

Please sign in to comment.