Leverage shares space storageid and type when listing shares (#2975)

* use shares providerid to ease routing Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * introduce ShareStorageSpaceID Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * make sharesstorageprovider return storageid Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * allow empty providerid when checking share jail root Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * add todo for cache Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * skip storageproviders that are not configured for the requested space type Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * add changelog Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * use ShareStorageSpaceID Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * make codacy happy Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
cs3org · Jun 16, 2022 · f7b1b5f · f7b1b5f
1 parent c22571c
commit f7b1b5f
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 12 deletions.
diff --git a/changelog/unreleased/leverage-sharesstorage-storageid.md b/changelog/unreleased/leverage-sharesstorage-storageid.md
@@ -0,0 +1,5 @@
+Enhancement: Leverage shares space storageid and type when listing shares
+
+The list shares call now also fills the storageid to allow the space registry to directly route requests to the correct storageprovider. The spaces registry will now also skip storageproviders that are not configured for a requested type, causing type 'personal' requests to skip the sharestorageprovider.
+
+https://github.com/cs3org/reva/pull/2975
diff --git a/internal/grpc/services/gateway/storageprovidercache.go b/internal/grpc/services/gateway/storageprovidercache.go
@@ -200,7 +200,7 @@ func (c *cachedRegistryClient) ListStorageProviders(ctx context.Context, in *reg
 		return resp, nil
 	case storageID == "":
 		return resp, nil
-	case storageID == utils.ShareStorageProviderID:
+	case storageID == utils.ShareStorageProviderID: // TODO do we need to compare providerid and spaceid separately?
 		return resp, nil
 	default:
 		return resp, pushToCache(cache, key, resp)

diff --git a/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go b/internal/grpc/services/sharesstorageprovider/sharesstorageprovider.go
@@ -368,7 +368,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 		switch k {
 		case "virtual":
 			virtualRootID := &provider.ResourceId{
-				StorageId: utils.ShareStorageProviderID,
+				StorageId: storagespace.FormatStorageID(utils.ShareStorageProviderID, utils.ShareStorageSpaceID),
 				OpaqueId:  utils.ShareStorageProviderID,
 			}
 			if spaceID == nil || isShareJailRoot(spaceID) {
@@ -387,7 +387,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 					space := &provider.StorageSpace{
 						Opaque: opaque,
 						Id: &provider.StorageSpaceId{
-							OpaqueId: virtualRootID.StorageId + "!" + virtualRootID.OpaqueId,
+							OpaqueId: storagespace.FormatResourceID(*virtualRootID),
 						},
 						SpaceType: "virtual",
 						//Owner:     &userv1beta1.User{Id: receivedShare.Share.Owner}, // FIXME actually, the mount point belongs to the recipient
@@ -415,7 +415,7 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 				space := &provider.StorageSpace{
 					Opaque: opaque,
 					Id: &provider.StorageSpaceId{
-						OpaqueId: root.StorageId + "!" + root.OpaqueId,
+						OpaqueId: storagespace.FormatResourceID(*root),
 					},
 					SpaceType: "grant",
 					Owner:     &userv1beta1.User{Id: receivedShare.Share.Owner},
@@ -433,7 +433,6 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 				root := &provider.ResourceId{
 					StorageId: utils.ShareStorageProviderID,
 					OpaqueId:  receivedShare.Share.Id.OpaqueId,
-					//OpaqueId: utils.ShareStorageProviderID,
 				}
 				// do we filter by id
 				if spaceID != nil {
@@ -461,10 +460,15 @@ func (s *service) ListStorageSpaces(ctx context.Context, req *provider.ListStora
 					opaque = utils.AppendPlainToOpaque(opaque, "grantOpaqueID", receivedShare.Share.ResourceId.OpaqueId)
 				}
 
+				// prefix storageid if we are responsible
+				if root.StorageId == utils.ShareStorageSpaceID {
+					root.StorageId = storagespace.FormatStorageID(utils.ShareStorageProviderID, root.StorageId)
+				}
+
 				space := &provider.StorageSpace{
 					Opaque: opaque,
 					Id: &provider.StorageSpaceId{
-						OpaqueId: root.StorageId + "!" + root.OpaqueId,
+						OpaqueId: storagespace.FormatResourceID(*root),
 					},
 					SpaceType: "mountpoint",
 					Owner:     &userv1beta1.User{Id: receivedShare.Share.Owner}, // FIXME actually, the mount point belongs to the recipient

diff --git a/internal/http/services/owncloud/ocdav/propfind/propfind.go b/internal/http/services/owncloud/ocdav/propfind/propfind.go
@@ -1354,9 +1354,14 @@ func (pn *Props) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
 	}
 }
 
+// isVirtualRootResourceID returns true if the id points to the share jail root. The providerid is optional for legacy ids
 func isVirtualRootResourceID(id *provider.ResourceId) bool {
-	return utils.ResourceIDEqual(id, &provider.ResourceId{
-		StorageId: utils.ShareStorageProviderID,
-		OpaqueId:  utils.ShareStorageProviderID,
-	})
+	switch {
+	case id == nil:
+		return false
+	case id.OpaqueId != utils.ShareStorageProviderID:
+		return false
+	}
+	providerID, spaceID := storagespace.SplitStorageID(id.StorageId)
+	return spaceID == utils.ShareStorageProviderID && (providerID == "" || providerID == utils.ShareStorageProviderID)
 }
diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
@@ -864,7 +864,7 @@ func (h *Handler) listSharesWithMe(w http.ResponseWriter, r *http.Request) {
 			// first stat mount point, but the shares storage provider only handles accepted shares so we send the try to make the requests for only those
 			if rs.State == collaboration.ShareState_SHARE_STATE_ACCEPTED {
 				mountID := &provider.ResourceId{
-					StorageId: utils.ShareStorageProviderID,
+					StorageId: storagespace.FormatStorageID(utils.ShareStorageProviderID, utils.ShareStorageSpaceID),
 					OpaqueId:  rs.Share.Id.OpaqueId,
 				}
 				info, status, err = h.getResourceInfoByID(ctx, client, mountID)

diff --git a/pkg/storage/registry/spaces/spaces.go b/pkg/storage/registry/spaces/spaces.go
@@ -321,9 +321,28 @@ func (r *registry) buildFilters(filterMap map[string]string) []*providerpb.ListS
 
 func (r *registry) findProvidersForFilter(ctx context.Context, filters []*providerpb.ListStorageSpacesRequest_Filter, unrestricted bool, _ string) []*registrypb.ProviderInfo {
 
+	var requestedSpaceType string
+	for _, f := range filters {
+		if f.Type == providerpb.ListStorageSpacesRequest_Filter_TYPE_SPACE_TYPE {
+			requestedSpaceType = f.GetSpaceType()
+		}
+	}
+
 	currentUser := ctxpkg.ContextMustGetUser(ctx)
 	providerInfos := []*registrypb.ProviderInfo{}
 	for address, provider := range r.c.Providers {
+		if requestedSpaceType != "" {
+			// check if we can skip this provider altogether
+			found := false
+			for spaceType := range provider.Spaces {
+				if spaceType == requestedSpaceType {
+					found = true
+				}
+			}
+			if !found {
+				continue
+			}
+		}
 		p := &registrypb.ProviderInfo{
 			Address: address,
 		}

diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go
@@ -50,8 +50,10 @@ var (
 	// globals is not encouraged, and this is a workaround until the PR is out of a draft state.
 	GlobalRegistry registry.Registry = memory.New(map[string]interface{}{})
 
-	// ShareStorageProviderID is the id used by the sharestorageprovider
+	// ShareStorageProviderID is the provider id used by the sharestorageprovider
 	ShareStorageProviderID = "a0ca6a90-a365-4782-871e-d44447bbc668"
+	// ShareStorageSpaceID is the space id used by the sharestorageprovider share jail space
+	ShareStorageSpaceID = "a0ca6a90-a365-4782-871e-d44447bbc668"
 
 	// PublicStorageProviderID is the id used by the sharestorageprovider
 	PublicStorageProviderID = "7993447f-687f-490d-875c-ac95e89a62a4"