correct share jail child aggregation

changelog/unreleased/fix-shares-jail-child-size.md

@@ -0,0 +1,5 @@
+Bugfix: correct share jail child aggregation
+
+We now add up the size of all mount points when aggregating the size for a child with the same name. Furthermore, the listing should no longer contain duplicate entries.
+
+https://github.com/cs3org/reva/pull/2907

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

@@ -144,6 +144,12 @@ type PropstatUnmarshalXML struct {
 	ResponseDescription string              `xml:"responsedescription,omitempty"`
 }
 
+// spaceData is used to remember the space for a resource info
+type spaceData struct {
+	Ref   *provider.Reference
+	Space *provider.StorageSpace
+}
+
 // NewMultiStatusResponseXML returns a preconfigured instance of MultiStatusResponseXML
 func NewMultiStatusResponseXML() *MultiStatusResponseXML {
 	return &MultiStatusResponseXML{
@@ -209,12 +215,28 @@ func (p *Handler) HandlePathPropfind(w http.ResponseWriter, r *http.Request, ns
 		return
 	}
 
+	var root *provider.StorageSpace
+
+	switch {
+	case len(spaces) == 1:
+		root = spaces[0]
+	case len(spaces) > 1:
+		for _, space := range spaces {
+			if isVirtualRootResourceID(space.Root) {
+				root = space
+			}
+		}
+		if root == nil {
+			root = spaces[0]
+		}
+	}
+
 	resourceInfos, sendTusHeaders, ok := p.getResourceInfos(ctx, w, r, pf, spaces, fn, false, sublog)
 	if !ok {
 		// getResourceInfos handles responses in case of an error so we can just return here.
 		return
 	}
-	p.propfindResponse(ctx, w, r, ns, spaces[0].SpaceType, pf, sendTusHeaders, resourceInfos, sublog)
+	p.propfindResponse(ctx, w, r, ns, root.SpaceType, pf, sendTusHeaders, resourceInfos, sublog)
 }
 
 // HandleSpacesPropfind handles a spaces based propfind request
@@ -305,7 +327,6 @@ func (p *Handler) propfindResponse(ctx context.Context, w http.ResponseWriter, r
 	}
 	w.Header().Set(net.HeaderDav, "1, 3, extended-mkcol")
 	w.Header().Set(net.HeaderContentType, "application/xml; charset=utf-8")
-
 	if sendTusHeaders {
 		w.Header().Add(net.HeaderAccessControlExposeHeaders, strings.Join([]string{net.HeaderTusResumable, net.HeaderTusVersion, net.HeaderTusExtension}, ", "))
 		w.Header().Set(net.HeaderTusResumable, "1.0.0")
@@ -374,7 +395,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r
 	var mostRecentChildInfo *provider.ResourceInfo
 	var aggregatedChildSize uint64
 	spaceInfos := make([]*provider.ResourceInfo, 0, len(spaces))
-	spaceMap := map[*provider.ResourceInfo]*provider.Reference{}
+	spaceMap := map[*provider.ResourceInfo]spaceData{}
 	for _, space := range spaces {
 		if space.Opaque == nil || space.Opaque.Map == nil || space.Opaque.Map["path"] == nil || space.Opaque.Map["path"].Decoder != "plain" {
 			continue // not mounted
@@ -398,8 +419,9 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r
 			info.Path = filepath.Join(spacePath, spaceRef.Path)
 		}
 
-		spaceMap[info] = spaceRef
+		spaceMap[info] = spaceData{Ref: spaceRef, Space: space}
 		spaceInfos = append(spaceInfos, info)
+
 		if rootInfo == nil && (requestPath == info.Path || (spacesPropfind && requestPath == path.Join("/", info.Path))) {
 			rootInfo = info
 		} else if requestPath != spacePath && strings.HasPrefix(spacePath, requestPath) { // Check if the space is a child of the requested path
@@ -461,11 +483,12 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r
 		}
 		spaceInfo.Path = path.Join(requestPath, childName)
 		if existingChild, ok := childInfos[childName]; ok {
+			// aggregate size
+			childInfos[childName].Size += spaceInfo.Size
 			// use most recent child
 			if existingChild.Mtime == nil || (spaceInfo.Mtime != nil && utils.TSToUnixNano(spaceInfo.Mtime) > utils.TSToUnixNano(existingChild.Mtime)) {
 				childInfos[childName].Mtime = spaceInfo.Mtime
 				childInfos[childName].Etag = spaceInfo.Etag
-				childInfos[childName].Size += spaceInfo.Size
 			}
 			// only update fileid if the resource is a direct child
 			if tail == "/" {
@@ -483,9 +506,9 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r
 
 		case spaceInfo.Type == provider.ResourceType_RESOURCE_TYPE_CONTAINER && depth == net.DepthOne:
 			switch {
-			case strings.HasPrefix(requestPath, spaceInfo.Path):
+			case strings.HasPrefix(requestPath, spaceInfo.Path) && spaceMap[spaceInfo].Space.SpaceType != "virtual":
 				req := &provider.ListContainerRequest{
-					Ref:                   spaceMap[spaceInfo],
+					Ref:                   spaceMap[spaceInfo].Ref,
 					ArbitraryMetadataKeys: metadataKeys,
 				}
 				res, err := client.ListContainer(ctx, req)
@@ -519,7 +542,7 @@ func (p *Handler) getResourceInfos(ctx context.Context, w http.ResponseWriter, r
 				info := stack[0]
 				stack = stack[1:]
 
-				if info.Type != provider.ResourceType_RESOURCE_TYPE_CONTAINER {
+				if info.Type != provider.ResourceType_RESOURCE_TYPE_CONTAINER || spaceMap[spaceInfo].Space.SpaceType == "virtual" {
 					continue
 				}
 				req := &provider.ListContainerRequest{
@@ -1320,3 +1343,10 @@ func (pn *Props) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
 		}
 	}
 }
+
+func isVirtualRootResourceID(id *provider.ResourceId) bool {
+	return utils.ResourceIDEqual(id, &provider.ResourceId{
+		StorageId: utils.ShareStorageProviderID,
+		OpaqueId:  utils.ShareStorageProviderID,
+	})
+}

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/pending.go

@@ -23,7 +23,10 @@ import (
 	"fmt"
 	"net/http"
 	"path"
+	"path/filepath"
 	"sort"
+	"strconv"
+	"strings"
 
 	gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1"
 	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
@@ -74,7 +77,7 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) {
 	// we need to sort the received shares by mount point in order to make things easier to evaluate.
 	base := path.Base(sharedResource.GetInfo().GetPath())
 	mount := base
-	var mountPoints []string
+	var mountedShares []*collaboration.ReceivedShare
 	sharesToAccept := map[string]bool{shareID: true}
 	for _, s := range lrs.Shares {
 		if utils.ResourceIDEqual(s.Share.ResourceId, rs.Share.Share.GetResourceId()) {
@@ -85,22 +88,40 @@ func (h *Handler) AcceptReceivedShare(w http.ResponseWriter, r *http.Request) {
 			}
 		} else {
 			if s.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
-				mountPoints = append(mountPoints, s.MountPoint.Path)
+				mountedShares = append(mountedShares, s)
 			}
 		}
 	}
 
-	sort.Strings(mountPoints)
+	compareMountPoint := func(i, j int) bool {
+		return mountedShares[i].MountPoint.Path > mountedShares[j].MountPoint.Path
+	}
+	sort.Slice(mountedShares, compareMountPoint)
 
 	// now we have a list of shares, we want to iterate over all of them and check for name collisions
-	// FIXME: adjust logic
-	/*
-		for i, mp := range mountPoints {
-			if mp == mount {
-				mount = fmt.Sprintf("%s (%s)", base, strconv.Itoa(i+1))
+	for i, ms := range mountedShares {
+		if ms.MountPoint.Path == mount {
+			// does the shared resource still exist?
+			res, err := client.Stat(ctx, &provider.StatRequest{
+				Ref: &provider.Reference{
+					ResourceId: ms.Share.ResourceId,
+				},
+			})
+			if err == nil && res.Status.Code == rpc.Code_CODE_OK {
+				// The mount point really already exists, we need to insert a number into the filename
+				ext := filepath.Ext(base)
+				name := strings.TrimSuffix(base, ext)
+				// be smart about .tar.(gz|bz) files
+				if strings.HasSuffix(name, ".tar") {
+					name = strings.TrimSuffix(name, ".tar")
+					ext = ".tar" + ext
+				}
+
+				mount = fmt.Sprintf("%s (%s)%s", name, strconv.Itoa(i+1), ext)
 			}
+			// TODO we could delete shares here if the stat returns code NOT FOUND ... but listening for file deletes would be better
 		}
-	*/
+	}
 
 	for id := range sharesToAccept {
 		data := h.updateReceivedShare(w, r, id, false, mount)