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 11, 2022 · 77817d2 · 77817d2
1 parent c2cc9b5
commit 77817d2
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: