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

This reverts commit 2a6ff11.

internal/grpc/services/gateway/storageprovider.go

@@ -837,12 +837,7 @@ 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,
-		FieldMask:             req.FieldMask,
-	})
+	return c.Stat(ctx, &provider.StatRequest{Opaque: req.Opaque, Ref: ref, ArbitraryMetadataKeys: req.ArbitraryMetadataKeys})
 }
 
 func (s *svc) ListContainerStream(_ *provider.ListContainerStreamRequest, _ gateway.GatewayAPI_ListContainerStreamServer) error {
@@ -863,7 +858,6 @@ func (s *svc) ListContainer(ctx context.Context, req *provider.ListContainerRequ
 		Opaque:                req.Opaque,
 		Ref:                   ref,
 		ArbitraryMetadataKeys: req.ArbitraryMetadataKeys,
-		FieldMask:             req.FieldMask,
 	})
 }
 

internal/grpc/services/gateway/storageprovidercache.go

@@ -229,37 +229,24 @@ 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, fieldMaskPaths []string) string {
+func statKey(user *userpb.User, ref *provider.Reference, metaDataKeys []string) string {
 	if ref == nil || ref.ResourceId == nil || ref.ResourceId.StorageId == "" {
 		return ""
 	}
 
-	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)
+	key := "uid:" + user.Id.OpaqueId + "!sid:" + ref.ResourceId.StorageId + "!oid:" + ref.ResourceId.OpaqueId + "!path:" + ref.Path
 	for _, k := range metaDataKeys {
-		key.WriteString("!mdk:")
-		key.WriteString(k)
-	}
-	for _, p := range fieldMaskPaths {
-		key.WriteString("!fmp:")
-		key.WriteString(p)
+		key += "!mdk:" + k
 	}
 
-	return key.String()
+	return key
 }
 
 // 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.GetRef(), in.GetArbitraryMetadataKeys(), in.GetFieldMask().GetPaths())
+	key := statKey(ctxpkg.ContextMustGetUser(ctx), in.Ref, in.ArbitraryMetadataKeys)
 	if key != "" {
 		s := &provider.StatResponse{}
 		if err := pullFromCache(cache, key, s); err == nil {

internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go

@@ -24,6 +24,7 @@ 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"
@@ -368,10 +369,10 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 	}
 
 	var receivedShares []*collaboration.ReceivedShare
-	var shareInfo map[string]*provider.ResourceInfo
+	var shareMd map[string]share.Metadata
 	var err error
 	if fetchShares {
-		receivedShares, shareInfo, err = s.fetchShares(ctx)
+		receivedShares, shareMd, err = s.fetchShares(ctx)
 		if err != nil {
 			return nil, errors.Wrap(err, "sharesstorageprovider: error calling ListReceivedSharesRequest")
 		}
@@ -388,13 +389,13 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 				OpaqueId:  utils.ShareStorageProviderID,
 			}
 			if spaceID == nil || isShareJailRoot(spaceID) {
-				earliestShare, atLeastOneAccepted := findEarliestShare(receivedShares, shareInfo)
+				earliestShare, atLeastOneAccepted := findEarliestShare(receivedShares, shareMd)
 				var opaque *typesv1beta1.Opaque
 				var mtime *typesv1beta1.Timestamp
 				if earliestShare != nil {
-					if info, ok := shareInfo[earliestShare.Id.OpaqueId]; ok {
-						mtime = info.Mtime
-						opaque = utils.AppendPlainToOpaque(opaque, "etag", info.Etag)
+					if md, ok := shareMd[earliestShare.Id.OpaqueId]; ok {
+						mtime = md.Mtime
+						opaque = utils.AppendPlainToOpaque(opaque, "etag", md.ETag)
 					}
 				}
 				// only display the shares jail if we have accepted shares
@@ -423,16 +424,20 @@ 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,
-					RootInfo: shareInfo[receivedShare.Share.Id.OpaqueId],
+					Root: root,
 				}
 
 				res.StorageSpaces = append(res.StorageSpaces, space)
@@ -460,7 +465,9 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 					}
 				}
 				var opaque *typesv1beta1.Opaque
-				if _, ok := shareInfo[receivedShare.Share.Id.OpaqueId]; !ok {
+				if md, ok := shareMd[receivedShare.Share.Id.OpaqueId]; ok {
+					opaque = utils.AppendPlainToOpaque(opaque, "etag", md.ETag)
+				} else {
 					// we could not stat the share, skip it
 					continue
 				}
@@ -483,8 +490,7 @@ 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,
-					RootInfo: shareInfo[receivedShare.Share.Id.OpaqueId],
+					Root: root,
 				}
 
 				// TODO in the future the spaces registry will handle the alias for share spaces.
@@ -705,7 +711,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
@@ -1025,7 +1031,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]*provider.ResourceInfo, error) {
+func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedShare, map[string]share.Metadata, error) {
 	lsRes, err := s.sharesProviderClient.ListReceivedShares(ctx, &collaboration.ListReceivedSharesRequest{
 		// FIXME filter by received shares for resource id - listing all shares is tooo expensive!
 	})
@@ -1036,7 +1042,7 @@ func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedSha
 		return nil, nil, fmt.Errorf("sharesstorageprovider: error calling ListReceivedSharesRequest")
 	}
 
-	shareMetaData := make(map[string]*provider.ResourceInfo, len(lsRes.Shares))
+	shareMetaData := make(map[string]share.Metadata, len(lsRes.Shares))
 	for _, rs := range lsRes.Shares {
 		// only stat accepted shares
 		if rs.State != collaboration.ShareState_SHARE_STATE_ACCEPTED {
@@ -1057,13 +1063,13 @@ func (s *service) fetchShares(ctx context.Context) ([]*collaboration.ReceivedSha
 				Msg("ListRecievedShares: failed to stat the resource")
 			continue
 		}
-		shareMetaData[rs.Share.Id.OpaqueId] = sRes.Info
+		shareMetaData[rs.Share.Id.OpaqueId] = share.Metadata{ETag: sRes.Info.Etag, Mtime: sRes.Info.Mtime}
 	}
 
 	return lsRes.Shares, shareMetaData, nil
 }
 
-func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareInfo map[string]*provider.ResourceInfo) (earliestShare *collaboration.Share, atLeastOneAccepted bool) {
+func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareMd map[string]share.Metadata) (earliestShare *collaboration.Share, atLeastOneAccepted bool) {
 	for _, rs := range receivedShares {
 		var hasCurrentMd bool
 		var hasEarliestMd bool
@@ -1075,10 +1081,10 @@ func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareInfo
 
 		// We cannot assume that every share has metadata
 		if current.Id != nil {
-			_, hasCurrentMd = shareInfo[current.Id.OpaqueId]
+			_, hasCurrentMd = shareMd[current.Id.OpaqueId]
 		}
 		if earliestShare != nil && earliestShare.Id != nil {
-			_, hasEarliestMd = shareInfo[earliestShare.Id.OpaqueId]
+			_, hasEarliestMd = shareMd[earliestShare.Id.OpaqueId]
 		}
 
 		switch {
@@ -1087,10 +1093,10 @@ func findEarliestShare(receivedShares []*collaboration.ReceivedShare, shareInfo
 		// ignore if one of the shares has no metadata
 		case !hasEarliestMd || !hasCurrentMd:
 			continue
-		case shareInfo[current.Id.OpaqueId].Mtime.Seconds > shareInfo[earliestShare.Id.OpaqueId].Mtime.Seconds:
+		case shareMd[current.Id.OpaqueId].Mtime.Seconds > shareMd[earliestShare.Id.OpaqueId].Mtime.Seconds:
 			earliestShare = current
-		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:
+		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:
 			earliestShare = current
 		}
 	}

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{}, []string{"id"})
+	md, err := s.storage.GetMD(ctx, req.Ref, []string{})
 	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.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths())
+	md, err := s.storage.GetMD(ctx, req.Ref, req.ArbitraryMetadataKeys)
 	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.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths())
+	mds, err := s.storage.ListFolder(ctx, req.Ref, req.ArbitraryMetadataKeys)
 	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.GetRef(), req.GetArbitraryMetadataKeys(), req.GetFieldMask().GetPaths())
+	mds, err := s.storage.ListFolder(ctx, req.Ref, req.ArbitraryMetadataKeys)
 	res := &provider.ListContainerResponse{
 		Status: status.NewStatusFromErrType(ctx, "list container", err),
 		Infos:  mds,

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

@@ -96,7 +96,8 @@ 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)
-		m := "Resource not found" // mimic the oc10 error message
+		// TODO path might be empty or relative...
+		m := fmt.Sprintf("Resource %v not found", ref.Path)
 		b, err := errors.Marshal(http.StatusNotFound, m, "")
 		errors.HandleWebdavError(&log, w, b, err)
 	case rpc.Code_CODE_PERMISSION_DENIED:
@@ -118,11 +119,12 @@ 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 do not leak existence of a file
+			// return not found error so we dont 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
-			m = "Resource not found" // mimic the oc10 error message
+			// TODO path might be empty or relative...
+			m = fmt.Sprintf("%s not fount", ref.Path)
 		}
 		w.WriteHeader(status)
 		b, err := errors.Marshal(status, m, "")

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

@@ -400,12 +400,23 @@ func (s *svc) handleSpacesLock(w http.ResponseWriter, r *http.Request, spaceID s
 
 	span.SetAttributes(attribute.String("component", "ocdav"))
 
-	ref, err := spacelookup.MakeStorageSpaceReference(spaceID, r.URL.Path)
+	client, err := s.getClient()
 	if err != nil {
-		return http.StatusBadRequest, fmt.Errorf("invalid space id")
+		return http.StatusInternalServerError, err
 	}
 
-	return s.lockReference(ctx, w, r, &ref)
+	// 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)
 }
 
 func (s *svc) lockReference(ctx context.Context, w http.ResponseWriter, r *http.Request, ref *provider.Reference) (retStatus int, retErr error) {

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 do not leak existence of a file
+			// return not found error so we dont 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("Resource not found")
+			return http.StatusNotFound, fmt.Errorf(sRes.Status.Message)
 		}
 		return http.StatusForbidden, fmt.Errorf(sRes.Status.Message)
 	case res.Status.Code == rpc.Code_CODE_ABORTED: