From 064ec8279012878cce958a225ee98a7d853a23d4 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Fri, 4 Feb 2022 18:29:01 +0100 Subject: [PATCH] fix propfind listing for files (#2506) --- changelog/unreleased/propfind-listing.md | 6 ++ internal/http/services/owncloud/ocdav/copy.go | 34 +++++------- .../http/services/owncloud/ocdav/net/net.go | 35 ++++++++++++ .../services/owncloud/ocdav/net/net_test.go | 55 +++++++++++++++++++ .../owncloud/ocdav/propfind/propfind.go | 27 +++++---- .../services/owncloud/ocdav/publicfile.go | 14 ++--- .../http/services/owncloud/ocdav/trashbin.go | 23 ++++---- 7 files changed, 139 insertions(+), 55 deletions(-) create mode 100644 changelog/unreleased/propfind-listing.md create mode 100644 internal/http/services/owncloud/ocdav/net/net_test.go diff --git a/changelog/unreleased/propfind-listing.md b/changelog/unreleased/propfind-listing.md new file mode 100644 index 00000000000..168c17773db --- /dev/null +++ b/changelog/unreleased/propfind-listing.md @@ -0,0 +1,6 @@ +Bugfix: Fix propfind listing for files + +When doing a propfind for a file the result contained the files twice. + +https://github.com/owncloud/ocis/issues/3066 +https://github.com/cs3org/reva/pull/2506 diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go index f3c94eb355a..843a467f84f 100644 --- a/internal/http/services/owncloud/ocdav/copy.go +++ b/internal/http/services/owncloud/ocdav/copy.go @@ -47,7 +47,7 @@ type copy struct { source *provider.Reference sourceInfo *provider.ResourceInfo destination *provider.Reference - depth string + depth net.Depth successCode int } @@ -105,7 +105,7 @@ func (s *svc) handlePathCopy(w http.ResponseWriter, r *http.Request, ns string) } if err := s.executePathCopy(ctx, client, w, r, cp); err != nil { - sublog.Error().Err(err).Str("depth", cp.depth).Msg("error executing path copy") + sublog.Error().Err(err).Str("depth", cp.depth.String()).Msg("error executing path copy") w.WriteHeader(http.StatusInternalServerError) } w.WriteHeader(cp.successCode) @@ -136,7 +136,7 @@ func (s *svc) executePathCopy(ctx context.Context, client gateway.GatewayAPIClie // TODO: also copy properties: https://tools.ietf.org/html/rfc4918#section-9.8.2 - if cp.depth != "infinity" { + if cp.depth != net.DepthInfinity { return nil } @@ -325,7 +325,7 @@ func (s *svc) handleSpacesCopy(w http.ResponseWriter, r *http.Request, spaceID s err = s.executeSpacesCopy(ctx, w, client, cp) if err != nil { - sublog.Error().Err(err).Str("depth", cp.depth).Msg("error descending directory") + sublog.Error().Err(err).Str("depth", cp.depth.String()).Msg("error descending directory") w.WriteHeader(http.StatusInternalServerError) } w.WriteHeader(cp.successCode) @@ -358,7 +358,7 @@ func (s *svc) executeSpacesCopy(ctx context.Context, w http.ResponseWriter, clie // TODO: also copy properties: https://tools.ietf.org/html/rfc4918#section-9.8.2 - if cp.depth != "infinity" { + if cp.depth != net.DepthInfinity { return nil } @@ -488,16 +488,23 @@ func (s *svc) prepareCopy(ctx context.Context, w http.ResponseWriter, r *http.Re errors.HandleWebdavError(log, w, b, err) return nil } - depth, err := extractDepth(w, r) + dh := r.Header.Get(net.HeaderDepth) + depth, err := net.ParseDepth(dh) + if err != nil { w.WriteHeader(http.StatusBadRequest) - m := fmt.Sprintf("Depth header is set to incorrect value %v", depth) + m := fmt.Sprintf("Depth header is set to incorrect value %v", dh) b, err := errors.Marshal(errors.SabredavBadRequest, m, "") errors.HandleWebdavError(log, w, b, err) return nil } + if dh == "" { + // net.ParseDepth returns "1" for an empty value but copy expects "infinity" + // so we overwrite it here + depth = net.DepthInfinity + } - log.Debug().Str("overwrite", overwrite).Str("depth", depth).Msg("copy") + log.Debug().Str("overwrite", overwrite).Str("depth", depth.String()).Msg("copy") client, err := s.getClient() if err != nil { @@ -605,14 +612,3 @@ func extractOverwrite(w http.ResponseWriter, r *http.Request) (string, error) { return overwrite, nil } - -func extractDepth(w http.ResponseWriter, r *http.Request) (string, error) { - depth := r.Header.Get(net.HeaderDepth) - if depth == "" { - depth = "infinity" - } - if depth != "infinity" && depth != "0" { - return "", errInvalidValue - } - return depth, nil -} diff --git a/internal/http/services/owncloud/ocdav/net/net.go b/internal/http/services/owncloud/ocdav/net/net.go index e5f3ded8dc5..06472d607da 100644 --- a/internal/http/services/owncloud/ocdav/net/net.go +++ b/internal/http/services/owncloud/ocdav/net/net.go @@ -44,8 +44,23 @@ const ( PropQuotaUnknown = "-2" // PropOcFavorite is the favorite ns property PropOcFavorite = "http://owncloud.org/ns/favorite" + + // DepthZero represents the webdav zero depth value + DepthZero Depth = "0" + // DepthOne represents the webdav one depth value + DepthOne Depth = "1" + // DepthInfinity represents the webdav infinity depth value + DepthInfinity Depth = "infinity" ) +// Depth is a type representing the webdav depth header value +type Depth string + +// String returns the string representation of the webdav depth value +func (d Depth) String() string { + return string(d) +} + // replaceAllStringSubmatchFunc is taken from 'Go: Replace String with Regular Expression Callback' // see: https://elliotchance.medium.com/go-replace-string-with-regular-expression-callback-f89948bad0bb func replaceAllStringSubmatchFunc(re *regexp.Regexp, str string, repl func([]string) string) string { @@ -78,3 +93,23 @@ func EncodePath(path string) string { return sb.String() }) } + +// ParseDepth parses the depth header value defined in https://tools.ietf.org/html/rfc4918#section-9.1 +// Valid values are "0", "1" and "infinity". An empty string will be parsed to "1". +// For all other values this method returns an error. +func ParseDepth(s string) (Depth, error) { + if s == "" { + return DepthOne, nil + } + + switch strings.ToLower(s) { + case DepthZero.String(): + return DepthZero, nil + case DepthOne.String(): + return DepthOne, nil + case DepthInfinity.String(): + return DepthInfinity, nil + default: + return "", fmt.Errorf("invalid depth: %s", s) + } +} diff --git a/internal/http/services/owncloud/ocdav/net/net_test.go b/internal/http/services/owncloud/ocdav/net/net_test.go new file mode 100644 index 00000000000..2b0e2c32aaa --- /dev/null +++ b/internal/http/services/owncloud/ocdav/net/net_test.go @@ -0,0 +1,55 @@ +// Copyright 2018-2022 CERN +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// In applying this license, CERN does not waive the privileges and immunities +// granted to it by virtue of its status as an Intergovernmental Organization +// or submit itself to any jurisdiction. + +package net + +import "testing" + +func TestParseDepth(t *testing.T) { + tests := map[string]Depth{ + "": DepthOne, + "0": DepthZero, + "1": DepthOne, + "infinity": DepthInfinity, + } + + for input, expected := range tests { + parsed, err := ParseDepth(input) + if err != nil { + t.Errorf("failed to parse depth %s", input) + } + if parsed != expected { + t.Errorf("parseDepth returned %s expected %s", parsed.String(), expected.String()) + } + } + + _, err := ParseDepth("invalid") + if err == nil { + t.Error("parse depth didn't return an error for invalid depth: invalid") + } +} + +var result Depth + +func BenchmarkParseDepth(b *testing.B) { + inputs := []string{"", "0", "1", "infinity", "INFINITY"} + size := len(inputs) + for i := 0; i < b.N; i++ { + result, _ = ParseDepth(inputs[i%size]) + } +} diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go index 96f6aab6bf3..2af6777e9f7 100644 --- a/internal/http/services/owncloud/ocdav/propfind/propfind.go +++ b/internal/http/services/owncloud/ocdav/propfind/propfind.go @@ -237,15 +237,12 @@ func (p *Handler) statSpace(ctx context.Context, client gateway.GatewayAPIClient } func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r *http.Request, pf XML, spaces []*provider.StorageSpace, requestPath string, spacesPropfind bool, log zerolog.Logger) ([]*provider.ResourceInfo, bool, bool) { - depth := r.Header.Get(net.HeaderDepth) - if depth == "" { - depth = "1" - } - // see https://tools.ietf.org/html/rfc4918#section-9.1 - if depth != "0" && depth != "1" && depth != "infinity" { - log.Debug().Str("depth", depth).Msg("invalid Depth header value") + dh := r.Header.Get(net.HeaderDepth) + depth, err := net.ParseDepth(dh) + if err != nil { + log.Debug().Str("depth", dh).Msg(err.Error()) w.WriteHeader(http.StatusBadRequest) - m := fmt.Sprintf("Invalid Depth header value: %v", depth) + m := fmt.Sprintf("Invalid Depth header value: %v", dh) b, err := errors.Marshal(errors.SabredavBadRequest, m, "") errors.HandleWebdavError(&log, w, b, err) return nil, false, false @@ -355,7 +352,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r // then add children for _, spaceInfo := range spaceInfos { switch { - case !spacesPropfind && spaceInfo.Type != provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth != "infinity": + case !spacesPropfind && spaceInfo.Type != provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth != net.DepthInfinity: // The propfind is requested for a file that exists childPath := strings.TrimPrefix(spaceInfo.Path, requestPath) @@ -375,7 +372,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r childInfos[childName] = spaceInfo } - case spaceInfo.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth == "1": + case spaceInfo.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth == net.DepthOne: switch { case strings.HasPrefix(requestPath, spaceInfo.Path): req := &provider.ListContainerRequest{ @@ -423,7 +420,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r log.Debug().Msg("unhandled") } - case depth == "infinity": + case depth == net.DepthInfinity: // use a stack to explore sub-containers breadth-first if spaceInfo != rootInfo { resourceInfos = append(resourceInfos, spaceInfo) @@ -472,9 +469,11 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r } } - // now add all aggregated child infos - for _, childInfo := range childInfos { - resourceInfos = append(resourceInfos, childInfo) + if rootInfo.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER { + // now add all aggregated child infos + for _, childInfo := range childInfos { + resourceInfos = append(resourceInfos, childInfo) + } } sendTusHeaders := true diff --git a/internal/http/services/owncloud/ocdav/publicfile.go b/internal/http/services/owncloud/ocdav/publicfile.go index e64ceb92995..512a9e39ba8 100644 --- a/internal/http/services/owncloud/ocdav/publicfile.go +++ b/internal/http/services/owncloud/ocdav/publicfile.go @@ -92,14 +92,10 @@ func (s *svc) handlePropfindOnToken(w http.ResponseWriter, r *http.Request, ns s sublog := appctx.GetLogger(ctx).With().Interface("tokenStatInfo", tokenStatInfo).Logger() sublog.Debug().Msg("handlePropfindOnToken") - depth := r.Header.Get(net.HeaderDepth) - if depth == "" { - depth = "1" - } - - // see https://tools.ietf.org/html/rfc4918#section-10.2 - if depth != "0" && depth != "1" && depth != "infinity" { - sublog.Debug().Msgf("invalid Depth header value %s", depth) + dh := r.Header.Get(net.HeaderDepth) + depth, err := net.ParseDepth(dh) + if err != nil { + sublog.Debug().Msg(err.Error()) w.WriteHeader(http.StatusBadRequest) return } @@ -114,7 +110,7 @@ func (s *svc) handlePropfindOnToken(w http.ResponseWriter, r *http.Request, ns s // prefix tokenStatInfo.Path with token tokenStatInfo.Path = filepath.Join(r.URL.Path, tokenStatInfo.Path) - infos := s.getPublicFileInfos(onContainer, depth == "0", tokenStatInfo) + infos := s.getPublicFileInfos(onContainer, depth == net.DepthZero, tokenStatInfo) propRes, err := propfind.MultistatusResponse(ctx, &pf, infos, s.c.PublicURL, ns, nil) if err != nil { diff --git a/internal/http/services/owncloud/ocdav/trashbin.go b/internal/http/services/owncloud/ocdav/trashbin.go index dd58d7b22a3..49470f73811 100644 --- a/internal/http/services/owncloud/ocdav/trashbin.go +++ b/internal/http/services/owncloud/ocdav/trashbin.go @@ -170,12 +170,16 @@ func (h *TrashbinHandler) listTrashbin(w http.ResponseWriter, r *http.Request, s ctx, span := rtrace.Provider.Tracer("trash-bin").Start(r.Context(), "list_trashbin") defer span.End() - depth := r.Header.Get(net.HeaderDepth) - if depth == "" { - depth = "1" + sublog := appctx.GetLogger(ctx).With().Logger() + + dh := r.Header.Get(net.HeaderDepth) + depth, err := net.ParseDepth(dh) + if err != nil { + sublog.Debug().Str("depth", dh).Msg(err.Error()) + w.WriteHeader(http.StatusBadRequest) + return } - sublog := appctx.GetLogger(ctx).With().Logger() client, err := s.getClient() if err != nil { sublog.Error().Err(err).Msg("error getting grpc client") @@ -183,14 +187,7 @@ func (h *TrashbinHandler) listTrashbin(w http.ResponseWriter, r *http.Request, s return } - // see https://tools.ietf.org/html/rfc4918#section-9.1 - if depth != "0" && depth != "1" && depth != "infinity" { - sublog.Debug().Str("depth", depth).Msgf("invalid Depth header value") - w.WriteHeader(http.StatusBadRequest) - return - } - - if depth == "0" { + if depth == net.DepthZero { propRes, err := h.formatTrashPropfind(ctx, s, u, nil, nil) if err != nil { sublog.Error().Err(err).Msg("error formatting propfind") @@ -253,7 +250,7 @@ func (h *TrashbinHandler) listTrashbin(w http.ResponseWriter, r *http.Request, s items := getRecycleRes.RecycleItems - if depth == "infinity" { + if depth == net.DepthInfinity { var stack []string // check sub-containers in reverse order and add them to the stack // the reversed order here will produce a more logical sorting of results