Skip to content

Commit

Permalink
Start splitting up ocdav (#2434)
Browse files Browse the repository at this point in the history 
* Start splitting up ocdav into smaller chunks

That increases clarity and allows for making things testable.

* Add a basic propfind unit test

* Fix linter and hound issues

* Add changelog
  • Loading branch information
@aduffeck
aduffeck authored Jan 12, 2022
1 parent f839002 commit 07451f6
Show file tree
Hide file tree
Showing 35 changed files with 4,102 additions and 1,019 deletions.
5 changes: 5 additions & 0 deletions changelog/unreleased/refactor-ocdav.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Enhancement: Refactor ocdav into smaller chunks

That increases code clarity and enables testing.

https://github.com/cs3org/reva/pull/2434
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ require (
github.com/sethvargo/go-password v0.2.0
github.com/stretchr/objx v0.3.0 // indirect
github.com/stretchr/testify v1.7.0
github.com/studio-b12/gowebdav v0.0.0-20210917133250-a3a86976a1df
github.com/studio-b12/gowebdav v0.0.0-20211109083228-3f8721cd4b6f
github.com/thanhpk/randstr v1.0.4
github.com/tidwall/pretty v1.2.0 // indirect
github.com/tus/tusd v1.8.0
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,8 @@ github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5Cc
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/studio-b12/gowebdav v0.0.0-20210917133250-a3a86976a1df h1:C+J/LwTqP8gRPt1MdSzBNZP0OYuDm5wsmDKgwpLjYzo=
github.com/studio-b12/gowebdav v0.0.0-20210917133250-a3a86976a1df/go.mod h1:gCcfDlA1Y7GqOaeEKw5l9dOGx1VLdc/HuQSlQAaZ30s=
github.com/studio-b12/gowebdav v0.0.0-20211109083228-3f8721cd4b6f h1:L2NE7BXnSlSLoNYZ0lCwZDjdnYjCNYC71k9ClZUTFTs=
github.com/studio-b12/gowebdav v0.0.0-20211109083228-3f8721cd4b6f/go.mod h1:bHA7t77X/QFExdeAnDzK6vKM34kEZAcE1OX4MfiwjkE=
github.com/syndtr/goleveldb v1.0.0/go.mod h1:ZVVdQEZoIme9iO1Ch2Jdy24qqXrMMOU6lpPAyBWyWuQ=
github.com/thanhpk/randstr v1.0.4 h1:IN78qu/bR+My+gHCvMEXhR/i5oriVHcTB/BJJIRTsNo=
github.com/thanhpk/randstr v1.0.4/go.mod h1:M/H2P1eNLZzlDwAzpkkkUvoyNNMbzRGhESZuEQk3r0U=
Expand Down
3 changes: 2 additions & 1 deletion internal/http/services/owncloud/ocdav/avatars.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"encoding/hex"
"net/http"

"github.com/cs3org/reva/internal/http/services/owncloud/ocdav/net"
"github.com/cs3org/reva/pkg/appctx"
"github.com/cs3org/reva/pkg/rhttp/router"
)
Expand Down Expand Up @@ -57,7 +58,7 @@ func (h *AvatarsHandler) Handler(s *svc) http.Handler {
log.Error().Err(err).Msg("error decoding string")
w.WriteHeader(http.StatusInternalServerError)
}
w.Header().Set(HeaderContentType, "image/png")
w.Header().Set(net.HeaderContentType, "image/png")
if _, err := w.Write(decoded); err != nil {
log.Error().Err(err).Msg("error writing data response")
}
Expand Down
122 changes: 51 additions & 71 deletions internal/http/services/owncloud/ocdav/copy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ import (
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
typespb "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
"github.com/cs3org/reva/internal/http/services/datagateway"
"github.com/cs3org/reva/internal/http/services/owncloud/ocdav/errors"
"github.com/cs3org/reva/internal/http/services/owncloud/ocdav/net"
"github.com/cs3org/reva/internal/http/services/owncloud/ocdav/spacelookup"
"github.com/cs3org/reva/pkg/appctx"
"github.com/cs3org/reva/pkg/rhttp"
"github.com/cs3org/reva/pkg/rhttp/router"
Expand Down Expand Up @@ -68,39 +71,39 @@ func (s *svc) handlePathCopy(w http.ResponseWriter, r *http.Request, ns string)

sublog := appctx.GetLogger(ctx).With().Str("src", src).Str("dst", dst).Logger()

srcSpace, status, err := s.lookUpStorageSpaceForPath(ctx, src)
client, err := s.getClient()
if err != nil {
sublog.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}

srcSpace, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, client, src)
if err != nil {
sublog.Error().Err(err).Str("path", src).Msg("failed to look up storage space")
w.WriteHeader(http.StatusInternalServerError)
return
}
if status.Code != rpc.Code_CODE_OK {
HandleErrorStatus(&sublog, w, status)
errors.HandleErrorStatus(&sublog, w, status)
return
}
dstSpace, status, err := s.lookUpStorageSpaceForPath(ctx, dst)
dstSpace, status, err := spacelookup.LookUpStorageSpaceForPath(ctx, client, dst)
if err != nil {
sublog.Error().Err(err).Str("path", dst).Msg("failed to look up storage space")
w.WriteHeader(http.StatusInternalServerError)
return
}
if status.Code != rpc.Code_CODE_OK {
HandleErrorStatus(&sublog, w, status)
errors.HandleErrorStatus(&sublog, w, status)
return
}

cp := s.prepareCopy(ctx, w, r, makeRelativeReference(srcSpace, src, false), makeRelativeReference(dstSpace, dst, false), &sublog)
cp := s.prepareCopy(ctx, w, r, spacelookup.MakeRelativeReference(srcSpace, src, false), spacelookup.MakeRelativeReference(dstSpace, dst, false), &sublog)
if cp == nil {
return
}

client, err := s.getClient()
if err != nil {
sublog.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}

if err := s.executePathCopy(ctx, client, w, r, cp); err != nil {
sublog.Error().Err(err).Str("depth", cp.depth).Msg("error executing path copy")
w.WriteHeader(http.StatusInternalServerError)
Expand All @@ -125,11 +128,8 @@ func (s *svc) executePathCopy(ctx context.Context, client gateway.GatewayAPIClie
if createRes.Status.Code == rpc.Code_CODE_PERMISSION_DENIED {
w.WriteHeader(http.StatusForbidden)
m := fmt.Sprintf("Permission denied to create %v", createReq.Ref.Path)
b, err := Marshal(exception{
code: SabredavPermissionDenied,
message: m,
})
HandleWebdavError(log, w, b, err)
b, err := errors.Marshal(errors.SabredavPermissionDenied, m, "")
errors.HandleWebdavError(log, w, b, err)
}
return nil
}
Expand Down Expand Up @@ -218,14 +218,11 @@ func (s *svc) executePathCopy(ctx context.Context, client gateway.GatewayAPIClie
if uRes.Status.Code == rpc.Code_CODE_PERMISSION_DENIED {
w.WriteHeader(http.StatusForbidden)
m := fmt.Sprintf("Permissions denied to create %v", uReq.Ref.Path)
b, err := Marshal(exception{
code: SabredavPermissionDenied,
message: m,
})
HandleWebdavError(log, w, b, err)
b, err := errors.Marshal(errors.SabredavPermissionDenied, m, "")
errors.HandleWebdavError(log, w, b, err)
return nil
}
HandleErrorStatus(log, w, uRes.Status)
errors.HandleErrorStatus(log, w, uRes.Status)
return nil
}

Expand Down Expand Up @@ -286,44 +283,45 @@ func (s *svc) handleSpacesCopy(w http.ResponseWriter, r *http.Request, spaceID s

sublog := appctx.GetLogger(ctx).With().Str("spaceid", spaceID).Str("path", r.URL.Path).Str("destination", dst).Logger()

client, err := s.getClient()
if err != nil {
sublog.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}

// retrieve a specific storage space
srcRef, status, err := s.lookUpStorageSpaceReference(ctx, spaceID, r.URL.Path, true)
srcRef, status, err := spacelookup.LookUpStorageSpaceReference(ctx, client, spaceID, r.URL.Path, true)
if err != nil {
sublog.Error().Err(err).Msg("error sending a grpc request")
w.WriteHeader(http.StatusInternalServerError)
return
}

if status.Code != rpc.Code_CODE_OK {
HandleErrorStatus(&sublog, w, status)
errors.HandleErrorStatus(&sublog, w, status)
return
}

dstSpaceID, dstRelPath := router.ShiftPath(dst)

// retrieve a specific storage space
dstRef, status, err := s.lookUpStorageSpaceReference(ctx, dstSpaceID, dstRelPath, true)
dstRef, status, err := spacelookup.LookUpStorageSpaceReference(ctx, client, dstSpaceID, dstRelPath, true)
if err != nil {
sublog.Error().Err(err).Msg("error sending a grpc request")
w.WriteHeader(http.StatusInternalServerError)
return
}

if status.Code != rpc.Code_CODE_OK {
HandleErrorStatus(&sublog, w, status)
errors.HandleErrorStatus(&sublog, w, status)
return
}

cp := s.prepareCopy(ctx, w, r, srcRef, dstRef, &sublog)
if cp == nil {
return
}
client, err := s.getClient()
if err != nil {
sublog.Error().Err(err).Msg("error getting grpc client")
w.WriteHeader(http.StatusInternalServerError)
return
}

err = s.executeSpacesCopy(ctx, w, client, cp)
if err != nil {
Expand Down Expand Up @@ -352,11 +350,8 @@ func (s *svc) executeSpacesCopy(ctx context.Context, w http.ResponseWriter, clie
w.WriteHeader(http.StatusForbidden)
// TODO path could be empty or relative...
m := fmt.Sprintf("Permission denied to create %v", createReq.Ref.Path)
b, err := Marshal(exception{
code: SabredavPermissionDenied,
message: m,
})
HandleWebdavError(log, w, b, err)
b, err := errors.Marshal(errors.SabredavPermissionDenied, m, "")
errors.HandleWebdavError(log, w, b, err)
}
return nil
}
Expand Down Expand Up @@ -412,7 +407,7 @@ func (s *svc) executeSpacesCopy(ctx context.Context, w http.ResponseWriter, clie
Ref: cp.destination,
Opaque: &typespb.Opaque{
Map: map[string]*typespb.OpaqueEntry{
HeaderUploadLength: {
net.HeaderUploadLength: {
Decoder: "plain",
// TODO: handle case where size is not known in advance
Value: []byte(strconv.FormatUint(cp.sourceInfo.GetSize(), 10)),
Expand All @@ -431,14 +426,11 @@ func (s *svc) executeSpacesCopy(ctx context.Context, w http.ResponseWriter, clie
w.WriteHeader(http.StatusForbidden)
// TODO path can be empty or relative
m := fmt.Sprintf("Permissions denied to create %v", uReq.Ref.Path)
b, err := Marshal(exception{
code: SabredavPermissionDenied,
message: m,
})
HandleWebdavError(log, w, b, err)
b, err := errors.Marshal(errors.SabredavPermissionDenied, m, "")
errors.HandleWebdavError(log, w, b, err)
return nil
}
HandleErrorStatus(log, w, uRes.Status)
errors.HandleErrorStatus(log, w, uRes.Status)
return nil
}

Expand Down Expand Up @@ -492,22 +484,16 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
if err != nil {
w.WriteHeader(http.StatusBadRequest)
m := fmt.Sprintf("Overwrite header is set to incorrect value %v", overwrite)
b, err := Marshal(exception{
code: SabredavBadRequest,
message: m,
})
HandleWebdavError(log, w, b, err)
b, err := errors.Marshal(errors.SabredavBadRequest, m, "")
errors.HandleWebdavError(log, w, b, err)
return nil
}
depth, err := extractDepth(w, r)
if err != nil {
w.WriteHeader(http.StatusBadRequest)
m := fmt.Sprintf("Depth header is set to incorrect value %v", depth)
b, err := Marshal(exception{
code: SabredavBadRequest,
message: m,
})
HandleWebdavError(log, w, b, err)
b, err := errors.Marshal(errors.SabredavBadRequest, m, "")
errors.HandleWebdavError(log, w, b, err)
return nil
}

Expand All @@ -532,13 +518,10 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
if srcStatRes.Status.Code == rpc.Code_CODE_NOT_FOUND {
w.WriteHeader(http.StatusNotFound)
m := fmt.Sprintf("Resource %v not found", srcStatReq.Ref.Path)
b, err := Marshal(exception{
code: SabredavNotFound,
message: m,
})
HandleWebdavError(log, w, b, err)
b, err := errors.Marshal(errors.SabredavNotFound, m, "")
errors.HandleWebdavError(log, w, b, err)
}
HandleErrorStatus(log, w, srcStatRes.Status)
errors.HandleErrorStatus(log, w, srcStatRes.Status)
return nil
}

Expand All @@ -550,7 +533,7 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
return nil
}
if dstStatRes.Status.Code != rpc.Code_CODE_OK && dstStatRes.Status.Code != rpc.Code_CODE_NOT_FOUND {
HandleErrorStatus(log, w, srcStatRes.Status)
errors.HandleErrorStatus(log, w, srcStatRes.Status)
return nil
}

Expand All @@ -562,11 +545,8 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
log.Warn().Str("overwrite", overwrite).Msg("dst already exists")
w.WriteHeader(http.StatusPreconditionFailed)
m := fmt.Sprintf("Could not overwrite Resource %v", dstRef.Path)
b, err := Marshal(exception{
code: SabredavPreconditionFailed,
message: m,
})
HandleWebdavError(log, w, b, err) // 412, see https://tools.ietf.org/html/rfc4918#section-9.8.5
b, err := errors.Marshal(errors.SabredavPreconditionFailed, m, "")
errors.HandleWebdavError(log, w, b, err) // 412, see https://tools.ietf.org/html/rfc4918#section-9.8.5
return nil
}

Expand All @@ -580,7 +560,7 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
}

if delRes.Status.Code != rpc.Code_CODE_OK && delRes.Status.Code != rpc.Code_CODE_NOT_FOUND {
HandleErrorStatus(log, w, delRes.Status)
errors.HandleErrorStatus(log, w, delRes.Status)
return nil
}
} else if p := path.Dir(dstRef.Path); p != "" {
Expand All @@ -602,7 +582,7 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
log.Debug().Interface("parent", pRef).Interface("status", intStatRes.Status).Msg("conflict")
w.WriteHeader(http.StatusConflict)
} else {
HandleErrorStatus(log, w, intStatRes.Status)
errors.HandleErrorStatus(log, w, intStatRes.Status)
}
return nil
}
Expand All @@ -613,7 +593,7 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re
}

func extractOverwrite(w http.ResponseWriter, r *http.Request) (string, error) {
overwrite := r.Header.Get(HeaderOverwrite)
overwrite := r.Header.Get(net.HeaderOverwrite)
overwrite = strings.ToUpper(overwrite)
if overwrite == "" {
overwrite = "T"
Expand All @@ -627,7 +607,7 @@ func extractOverwrite(w http.ResponseWriter, r *http.Request) (string, error) {
}

func extractDepth(w http.ResponseWriter, r *http.Request) (string, error) {
depth := r.Header.Get(HeaderDepth)
depth := r.Header.Get(net.HeaderDepth)
if depth == "" {
depth = "infinity"
}
Expand Down
Loading

0 comments on commit 07451f6

Please sign in to comment.