fix propfind listing for files (cs3org#2506)

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

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
-}

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)
+	}
+}

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])
+	}
+}

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

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 {

internal/http/services/owncloud/ocdav/trashbin.go

@@ -170,27 +170,24 @@ 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")
 		w.WriteHeader(http.StatusInternalServerError)
 		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