ocdav: skip space lookup on spaces propfind (cs3org#2977)

* ocdav: skip space lookup on spaces propfind Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * add changelog Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * update unit test Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * lint Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * reduce stat requests Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * update propfind unit tests Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fix moq propfind comparison in propfind tests Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * lint Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * error handling and response codes Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * remove commented code Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * typos, mimic oc10 not found error message Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * add fieldmask to ListContainer and Stat Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * mimic oc10 not found error message Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * mimic oc10 not found error message in more places Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fix oc:permissions Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fix test compile Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fix nc tests Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * lint Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * small cleanup Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * decomposedfs: add space if fieldmask is empty, * or has 'space' Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fix propfind root resource path Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * update tests & changelog Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * storageprovidercache: use stringbuffer to build statKey Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * comment cleanup Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
kobergj · Jul 7, 2022 · 2a6ff11 · 2a6ff11
1 parent b060ddc
commit 2a6ff11
Show file tree

Hide file tree

Showing 36 changed files with 723 additions and 395 deletions.
diff --git a/changelog/unreleased/skip-space-lookup-on-space-propfind.md b/changelog/unreleased/skip-space-lookup-on-space-propfind.md
@@ -0,0 +1,8 @@
+Enhancement: skip space lookup on space propfind
+
+We now construct the space id from the /dav/spaces URL intead of making a request to the registry.
+
+https://github.com/cs3org/reva/pull/2977
+https://github.com/owncloud/ocis/issues/1277
+https://github.com/owncloud/ocis/issues/2144
+https://github.com/owncloud/ocis/issues/3073
diff --git a/internal/grpc/services/gateway/storageprovider.go b/internal/grpc/services/gateway/storageprovider.go
@@ -837,7 +837,12 @@ func (s *svc) Stat(ctx context.Context, req *provider.StatRequest) (*provider.St
 		}, nil
 	}
 
-	return c.Stat(ctx, &provider.StatRequest{Opaque: req.Opaque, Ref: ref, ArbitraryMetadataKeys: req.ArbitraryMetadataKeys})
+	return c.Stat(ctx, &provider.StatRequest{
+		Opaque:                req.Opaque,
+		Ref:                   ref,
+		ArbitraryMetadataKeys: req.ArbitraryMetadataKeys,
+		FieldMask:             req.FieldMask,
+	})
 }
 
 func (s *svc) ListContainerStream(_ *provider.ListContainerStreamRequest, _ gateway.GatewayAPI_ListContainerStreamServer) error {
@@ -858,6 +863,7 @@ func (s *svc) ListContainer(ctx context.Context, req *provider.ListContainerRequ
 		Opaque:                req.Opaque,
 		Ref:                   ref,
 		ArbitraryMetadataKeys: req.ArbitraryMetadataKeys,
+		FieldMask:             req.FieldMask,
 	})
 }
 

diff --git a/internal/grpc/services/gateway/storageprovidercache.go b/internal/grpc/services/gateway/storageprovidercache.go
@@ -229,24 +229,37 @@ type cachedAPIClient struct {
 // generates a user specific key pointing to ref - used for statcache
 // a key looks like: uid:1234-1233!sid:5678-5677!oid:9923-9934!path:/path/to/source
 // as you see it adds "uid:"/"sid:"/"oid:" prefixes to the uuids so they can be differentiated
-func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys []string) string {
+func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys, fieldMaskPaths []string) string {
 	if ref == nil || ref.ResourceId == nil || ref.ResourceId.StorageId == "" {
 		return ""
 	}
 
-	key := "uid:" + user.Id.OpaqueId + "!sid:" + ref.ResourceId.StorageId + "!oid:" + ref.ResourceId.OpaqueId + "!path:" + ref.Path
+	key := strings.Builder{}
+	key.WriteString("uid:")
+	key.WriteString(user.Id.OpaqueId)
+	key.WriteString("!sid:")
+	key.WriteString(ref.ResourceId.StorageId)
+	key.WriteString("!oid:")
+	key.WriteString(ref.ResourceId.OpaqueId)
+	key.WriteString("!path:")
+	key.WriteString(ref.Path)
 	for _, k := range metaDataKeys {
-		key += "!mdk:" + k
+		key.WriteString("!mdk:")
+		key.WriteString(k)
+	}
+	for _, p := range fieldMaskPaths {
+		key.WriteString("!fmp:")
+		key.WriteString(p)
 	}
 
-	return key
+	return key.String()
 }
 
 // Stat looks in cache first before forwarding to storage provider
 func (c *cachedAPIClient) Stat(ctx context.Context, in *provider.StatRequest, opts ...grpc.CallOption) (*provider.StatResponse, error) {
 	cache := c.caches[stat]
 
-	key := statKey(ctxpkg.ContextMustGetUser(ctx), in.Ref, in.ArbitraryMetadataKeys)
+	key := statKey(ctxpkg.ContextMustGetUser(ctx), in.GetRef(), in.GetArbitraryMetadataKeys(), in.GetFieldMask().GetPaths())
 	if key != "" {
 		s := &provider.StatResponse{}
 		if err := pullFromCache(cache, key, s); err == nil {

diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go
@@ -24,7 +24,6 @@ import (
 	"path/filepath"
 	"strings"
 
-	"github.com/cs3org/reva/v2/pkg/share"
 	"github.com/cs3org/reva/v2/pkg/storagespace"
 	"google.golang.org/grpc"
 	codes "google.golang.org/grpc/codes"
@@ -369,10 +368,10 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 	}
 
 	var receivedShares []*collaboration.ReceivedShare
-	var shareMd map[string]share.Metadata
+	var shareInfo map[string]*provider.ResourceInfo
 	var err error
 	if fetchShares {
-		receivedShares, shareMd, err = s.fetchShares(ctx)
+		receivedShares, shareInfo, err = s.fetchShares(ctx)
 		if err != nil {
 			return nil, errors.Wrap(err, "sharesstorageprovider: error calling ListReceivedSharesRequest")
 		}
@@ -389,13 +388,13 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 				OpaqueId:  utils.ShareStorageProviderID,
 			}
 			if spaceID == nil || isShareJailRoot(spaceID) {
-				earliestShare, atLeastOneAccepted := findEarliestShare(receivedShares, shareMd)
+				earliestShare, atLeastOneAccepted := findEarliestShare(receivedShares, shareInfo)
 				var opaque *typesv1beta1.Opaque
 				var mtime *typesv1beta1.Timestamp
 				if earliestShare != nil {
-					if md, ok := shareMd[earliestShare.Id.OpaqueId]; ok {
-						mtime = md.Mtime
-						opaque = utils.AppendPlainToOpaque(opaque, "etag", md.ETag)
+					if info, ok := shareInfo[earliestShare.Id.OpaqueId]; ok {
+						mtime = info.Mtime
+						opaque = utils.AppendPlainToOpaque(opaque, "etag", info.Etag)
 					}
 				}
 				// only display the shares jail if we have accepted shares
@@ -424,20 +423,16 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 					// none of our business
 					continue
 				}
-				var opaque *typesv1beta1.Opaque
-				if md, ok := shareMd[receivedShare.Share.Id.OpaqueId]; ok {
-					opaque = utils.AppendPlainToOpaque(opaque, "etag", md.ETag)
-				}
 				// we know a grant for this resource
 				space := &provider.StorageSpace{
-					Opaque: opaque,
 					Id: &provider.StorageSpaceId{
 						OpaqueId: storagespace.FormatResourceID(*root),
 					},
 					SpaceType: "grant",
 					Owner:     &userv1beta1.User{Id: receivedShare.Share.Owner},
 					// the sharesstorageprovider keeps track of mount points
-					Root: root,
+					Root:     root,
+					RootInfo: shareInfo[receivedShare.Share.Id.OpaqueId],
 				}
 
 				res.StorageSpaces = append(res.StorageSpaces, space)
@@ -465,9 +460,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 					}
 				}
 				var opaque *typesv1beta1.Opaque
-				if md, ok := shareMd[receivedShare.Share.Id.OpaqueId]; ok {
-					opaque = utils.AppendPlainToOpaque(opaque, "etag", md.ETag)
-				} else {
+				if _, ok := shareInfo[receivedShare.Share.Id.OpaqueId]; !ok {
 					// we could not stat the share, skip it
 					continue
 				}
@@ -490,7 +483,8 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 					SpaceType: "mountpoint",
 					Owner:     &userv1beta1.User{Id: receivedShare.Share.Owner}, // FIXME actually, the mount point belongs to the recipient
 					// the sharesstorageprovider keeps track of mount points
-					Root: root,
+					Root:     root,
+					RootInfo: shareInfo[receivedShare.Share.Id.OpaqueId],
 				}
 
 				// TODO in the future the spaces registry will handle the alias for share spaces.
@@ -711,7 +705,7 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide
 				PermissionSet: &provider.ResourcePermissions{
 					// TODO
 				},
-				Etag:  shareMd[earliestShare.Id.OpaqueId].ETag,
+				Etag:  shareMd[earliestShare.Id.OpaqueId].Etag,
 				Owner: owner.Id,
 			},
 		}, nil
@@ -1031,7 +1025,7 @@ func (s *service) rejectReceivedShare(ctx context.Context, receivedShare *collab
 	return errtypes.NewErrtypeFromStatus(res.Status)
 }
 
-func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedShare, map[string]share.Metadata, error) {
+func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedShare, map[string]*provider.ResourceInfo, error) {
 	lsRes, err := s.sharesProviderClient.ListReceivedShares(ctx, &collaboration.ListReceivedSharesRequest{
 		// FIXME filter by received shares for resource id - listing all shares is tooo expensive!
 	})
@@ -1042,7 +1036,7 @@ func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedSha
 		return nil, nil, fmt.Errorf("sharesstorageprovider: error calling ListReceivedSharesRequest")
 	}
 
-	shareMetaData := make(map[string]share.Metadata, len(lsRes.Shares))
+	shareMetaData := make(map[string]*provider.ResourceInfo, len(lsRes.Shares))
 	for _, rs := range lsRes.Shares {
 		// only stat accepted shares
 		if rs.State != collaboration.ShareState_SHARE_STATE_ACCEPTED {
@@ -1063,13 +1057,13 @@ func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedSha
 				Msg("ListRecievedShares: failed to stat the resource")
 			continue
 		}
-		shareMetaData[rs.Share.Id.OpaqueId] = share.Metadata{ETag: sRes.Info.Etag, Mtime: sRes.Info.Mtime}
+		shareMetaData[rs.Share.Id.OpaqueId] = sRes.Info
 	}
 
 	return lsRes.Shares, shareMetaData, nil
 }
 
-func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareMd map[string]share.Metadata) (earliestShare *collaboration.Share, atLeastOneAccepted bool) {
+func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareInfo map[string]*provider.ResourceInfo) (earliestShare *collaboration.Share, atLeastOneAccepted bool) {
 	for _, rs := range receivedShares {
 		var hasCurrentMd bool
 		var hasEarliestMd bool
@@ -1081,10 +1075,10 @@ func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareMd ma
 
 		// We cannot assume that every share has metadata
 		if current.Id != nil {
-			_, hasCurrentMd = shareMd[current.Id.OpaqueId]
+			_, hasCurrentMd = shareInfo[current.Id.OpaqueId]
 		}
 		if earliestShare != nil && earliestShare.Id != nil {
-			_, hasEarliestMd = shareMd[earliestShare.Id.OpaqueId]
+			_, hasEarliestMd = shareInfo[earliestShare.Id.OpaqueId]
 		}
 
 		switch {
@@ -1093,10 +1087,10 @@ func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareMd ma
 		// ignore if one of the shares has no metadata
 		case !hasEarliestMd || !hasCurrentMd:
 			continue
-		case shareMd[current.Id.OpaqueId].Mtime.Seconds > shareMd[earliestShare.Id.OpaqueId].Mtime.Seconds:
+		case shareInfo[current.Id.OpaqueId].Mtime.Seconds > shareInfo[earliestShare.Id.OpaqueId].Mtime.Seconds:
 			earliestShare = current
-		case shareMd[current.Id.OpaqueId].Mtime.Seconds == shareMd[earliestShare.Id.OpaqueId].Mtime.Seconds &&
-			shareMd[current.Id.OpaqueId].Mtime.Nanos > shareMd[earliestShare.Id.OpaqueId].Mtime.Nanos:
+		case shareInfo[current.Id.OpaqueId].Mtime.Seconds == shareInfo[earliestShare.Id.OpaqueId].Mtime.Seconds &&
+			shareInfo[current.Id.OpaqueId].Mtime.Nanos > shareInfo[earliestShare.Id.OpaqueId].Mtime.Nanos:
 			earliestShare = current
 		}
 	}

diff --git a/internal/grpc/services/storageprovider/storageprovider.go b/internal/grpc/services/storageprovider/storageprovider.go
@@ -672,7 +672,7 @@ func (s *service) Delete(ctx context.Context, req *provider.DeleteRequest) (*pro
 		}
 	}
 
-	md, err := s.storage.GetMD(ctx, req.Ref, []string{})
+	md, err := s.storage.GetMD(ctx, req.Ref, []string{}, []string{"id"})
 	if err != nil {
 		return &provider.DeleteResponse{
 			Status: status.NewStatusFromErrType(ctx, "can't stat resource to delete", err),
@@ -718,7 +718,7 @@ func (s *service) Stat(ctx context.Context, req *provider.StatRequest) (*provide
 		Value: attribute.StringValue(req.Ref.String()),
 	})
 
-	md, err := s.storage.GetMD(ctx, req.Ref, req.ArbitraryMetadataKeys)
+	md, err := s.storage.GetMD(ctx, req.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths())
 	if err != nil {
 		return &provider.StatResponse{
 			Status: status.NewStatusFromErrType(ctx, "stat", err),
@@ -747,7 +747,7 @@ func (s *service) ListContainerStream(req *provider.ListContainerStreamRequest,
 	ctx := ss.Context()
 	log := appctx.GetLogger(ctx)
 
-	mds, err := s.storage.ListFolder(ctx, req.Ref, req.ArbitraryMetadataKeys)
+	mds, err := s.storage.ListFolder(ctx, req.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths())
 	if err != nil {
 		var st *rpc.Status
 		switch err.(type) {
@@ -792,7 +792,7 @@ func (s *service) ListContainer(ctx context.Context, req *provider.ListContainer
 	providerID := unwrapProviderID(req.Ref.GetResourceId())
 	defer rewrapProviderID(req.Ref.GetResourceId(), providerID)
 
-	mds, err := s.storage.ListFolder(ctx, req.Ref, req.ArbitraryMetadataKeys)
+	mds, err := s.storage.ListFolder(ctx, req.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths())
 	res := &provider.ListContainerResponse{
 		Status: status.NewStatusFromErrType(ctx, "list container", err),
 		Infos:  mds,

diff --git a/internal/http/services/owncloud/ocdav/delete.go b/internal/http/services/owncloud/ocdav/delete.go
@@ -96,8 +96,7 @@ func (s *svc) handleDelete(ctx context.Context, w http.ResponseWriter, r *http.R
 		w.WriteHeader(http.StatusNoContent)
 	case rpc.Code_CODE_NOT_FOUND:
 		w.WriteHeader(http.StatusNotFound)
-		// TODO path might be empty or relative...
-		m := fmt.Sprintf("Resource %v not found", ref.Path)
+		m := "Resource not found" // mimic the oc10 error message
 		b, err := errors.Marshal(http.StatusNotFound, m, "")
 		errors.HandleWebdavError(&log, w, b, err)
 	case rpc.Code_CODE_PERMISSION_DENIED:
@@ -119,12 +118,11 @@ func (s *svc) handleDelete(ctx context.Context, w http.ResponseWriter, r *http.R
 			return
 		}
 		if sRes.Status.Code != rpc.Code_CODE_OK {
-			// return not found error so we dont leak existence of a file
+			// return not found error so we do not leak existence of a file
 			// TODO hide permission failed for users without access in every kind of request
 			// TODO should this be done in the driver?
 			status = http.StatusNotFound
-			// TODO path might be empty or relative...
-			m = fmt.Sprintf("%s not fount", ref.Path)
+			m = "Resource not found" // mimic the oc10 error message
 		}
 		w.WriteHeader(status)
 		b, err := errors.Marshal(status, m, "")

diff --git a/internal/http/services/owncloud/ocdav/locks.go b/internal/http/services/owncloud/ocdav/locks.go
@@ -400,23 +400,12 @@ func (s *svc) handleSpacesLock(w http.ResponseWriter, r *http.Request, spaceID s
 
 	span.SetAttributes(attribute.String("component", "ocdav"))
 
-	client, err := s.getClient()
+	ref, err := spacelookup.MakeStorageSpaceReference(spaceID, r.URL.Path)
 	if err != nil {
-		return http.StatusInternalServerError, err
+		return http.StatusBadRequest, fmt.Errorf("invalid space id")
 	}
 
-	// retrieve a specific storage space
-	space, cs3Status, err := spacelookup.LookUpStorageSpaceByID(ctx, client, spaceID)
-	if err != nil {
-		return http.StatusInternalServerError, err
-	}
-	if cs3Status.Code != rpc.Code_CODE_OK {
-		return http.StatusInternalServerError, errtypes.NewErrtypeFromStatus(cs3Status)
-	}
-
-	ref := spacelookup.MakeRelativeReference(space, r.URL.Path, true)
-
-	return s.lockReference(ctx, w, r, ref)
+	return s.lockReference(ctx, w, r, &ref)
 }
 
 func (s *svc) lockReference(ctx context.Context, w http.ResponseWriter, r *http.Request, ref *provider.Reference) (retStatus int, retErr error) {

diff --git a/internal/http/services/owncloud/ocdav/mkcol.go b/internal/http/services/owncloud/ocdav/mkcol.go
@@ -136,10 +136,10 @@ func (s *svc) handleMkcol(ctx context.Context, w http.ResponseWriter, r *http.Re
 			return http.StatusInternalServerError, err
 		}
 		if sRes.Status.Code != rpc.Code_CODE_OK {
-			// return not found error so we dont leak existence of a file
+			// return not found error so we do not leak existence of a file
 			// TODO hide permission failed for users without access in every kind of request
 			// TODO should this be done in the driver?
-			return http.StatusNotFound, fmt.Errorf(sRes.Status.Message)
+			return http.StatusNotFound, fmt.Errorf("Resource not found")
 		}
 		return http.StatusForbidden, fmt.Errorf(sRes.Status.Message)
 	case res.Status.Code == rpc.Code_CODE_ABORTED: