Spaces fixes

changelog/unreleased/spaces-fixes.md

@@ -0,0 +1,5 @@
+Bugfix: list project spaces for share recipients
+
+The sharing handler now uses the ListProvider call on the registry when sharing by reference. Furthermore, the decomposedfs now checks permissions on the root of a space so that a space is listed for users that have access to a space.
+
+https://github.com/cs3org/reva/pull/2419

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

@@ -28,12 +28,14 @@ import (
 	rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	registry "github.com/cs3org/go-cs3apis/cs3/storage/registry/v1beta1"
+	types "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
 	"github.com/cs3org/reva/internal/http/services/owncloud/ocs/conversions"
 	"github.com/cs3org/reva/internal/http/services/owncloud/ocs/response"
 	"github.com/cs3org/reva/pkg/appctx"
 	"github.com/cs3org/reva/pkg/errtypes"
 	"github.com/cs3org/reva/pkg/rgrpc/status"
 	"github.com/cs3org/reva/pkg/rgrpc/todo/pool"
+	sdk "github.com/cs3org/reva/pkg/sdk/common"
 	"github.com/cs3org/reva/pkg/utils"
 	"github.com/pkg/errors"
 )
@@ -88,15 +90,15 @@ func (h *Handler) addSpaceMember(w http.ResponseWriter, r *http.Request, info *p
 
 	ref := &provider.Reference{ResourceId: info.Id}
 
-	providers, err := h.findProviders(ctx, ref)
+	p, err := h.findProvider(ctx, ref)
 	if err != nil {
 		response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider", err)
 		return
 	}
 
-	providerClient, err := h.getStorageProviderClient(providers[0])
+	providerClient, err := h.getStorageProviderClient(p)
 	if err != nil {
-		response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider", err)
+		response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider client", err)
 		return
 	}
 
@@ -136,15 +138,15 @@ func (h *Handler) removeSpaceMember(w http.ResponseWriter, r *http.Request, spac
 		return
 	}
 
-	providers, err := h.findProviders(ctx, &ref)
+	p, err := h.findProvider(ctx, &ref)
 	if err != nil {
 		response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider", err)
 		return
 	}
 
-	providerClient, err := h.getStorageProviderClient(providers[0])
+	providerClient, err := h.getStorageProviderClient(p)
 	if err != nil {
-		response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider", err)
+		response.WriteOCSError(w, r, response.MetaNotFound.StatusCode, "error getting storage provider client", err)
 		return
 	}
 
@@ -169,45 +171,57 @@ func (h *Handler) removeSpaceMember(w http.ResponseWriter, r *http.Request, spac
 func (h *Handler) getStorageProviderClient(p *registry.ProviderInfo) (provider.ProviderAPIClient, error) {
 	c, err := pool.GetStorageProviderServiceClient(p.Address)
 	if err != nil {
-		err = errors.Wrap(err, "gateway: error getting a storage provider client")
+		err = errors.Wrap(err, "shares spaces: error getting a storage provider client")
 		return nil, err
 	}
 
 	return c, nil
 }
 
-func (h *Handler) findProviders(ctx context.Context, ref *provider.Reference) ([]*registry.ProviderInfo, error) {
+func (h *Handler) findProvider(ctx context.Context, ref *provider.Reference) (*registry.ProviderInfo, error) {
 	c, err := pool.GetStorageRegistryClient(h.storageRegistryAddr)
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error getting storage registry client")
+		return nil, errors.Wrap(err, "shares spaces: error getting storage registry client")
 	}
 
-	res, err := c.GetStorageProviders(ctx, &registry.GetStorageProvidersRequest{
-		Ref: ref,
-	})
+	filters := map[string]string{}
+	if ref.Path != "" {
+		filters["path"] = ref.Path
+	}
+	if ref.ResourceId != nil {
+		filters["storage_id"] = ref.ResourceId.StorageId
+		filters["opaque_id"] = ref.ResourceId.OpaqueId
+	}
+
+	listReq := &registry.ListStorageProvidersRequest{
+		Opaque: &types.Opaque{},
+	}
+	sdk.EncodeOpaqueMap(listReq.Opaque, filters)
+
+	res, err := c.ListStorageProviders(ctx, listReq)
 
 	if err != nil {
-		return nil, errors.Wrap(err, "gateway: error calling GetStorageProvider")
+		return nil, errors.Wrap(err, "shares spaces: error calling ListStorageProviders")
 	}
 
 	if res.Status.Code != rpc.Code_CODE_OK {
 		switch res.Status.Code {
 		case rpc.Code_CODE_NOT_FOUND:
-			return nil, errtypes.NotFound("gateway: storage provider not found for reference:" + ref.String())
+			return nil, errtypes.NotFound("shares spaces: storage provider not found for reference:" + ref.String())
 		case rpc.Code_CODE_PERMISSION_DENIED:
-			return nil, errtypes.PermissionDenied("gateway: " + res.Status.Message + " for " + ref.String() + " with code " + res.Status.Code.String())
+			return nil, errtypes.PermissionDenied("shares spaces: " + res.Status.Message + " for " + ref.String() + " with code " + res.Status.Code.String())
 		case rpc.Code_CODE_INVALID_ARGUMENT, rpc.Code_CODE_FAILED_PRECONDITION, rpc.Code_CODE_OUT_OF_RANGE:
-			return nil, errtypes.BadRequest("gateway: " + res.Status.Message + " for " + ref.String() + " with code " + res.Status.Code.String())
+			return nil, errtypes.BadRequest("shares spaces: " + res.Status.Message + " for " + ref.String() + " with code " + res.Status.Code.String())
 		case rpc.Code_CODE_UNIMPLEMENTED:
-			return nil, errtypes.NotSupported("gateway: " + res.Status.Message + " for " + ref.String() + " with code " + res.Status.Code.String())
+			return nil, errtypes.NotSupported("shares spaces: " + res.Status.Message + " for " + ref.String() + " with code " + res.Status.Code.String())
 		default:
-			return nil, status.NewErrorFromCode(res.Status.Code, "gateway")
+			return nil, status.NewErrorFromCode(res.Status.Code, "shares spaces")
 		}
 	}
 
-	if res.Providers == nil {
-		return nil, errtypes.NotFound("gateway: provider is nil")
+	if len(res.Providers) < 1 {
+		return nil, errtypes.NotFound("shares spaces: no provider found")
 	}
 
-	return res.Providers, nil
+	return res.Providers[0], nil
 }

pkg/storage/utils/decomposedfs/node/permissions.go

@@ -200,60 +200,71 @@ func (p *Permissions) HasPermission(ctx context.Context, n *Node, check func(*pr
 		groupsMap[u.Groups[i]] = true
 	}
 
-	var g *provider.Grant
 	// for all segments, starting at the leaf
 	cn := n
 	for cn.ID != n.SpaceRoot.ID {
+		if ok := nodeHasPermission(ctx, cn, groupsMap, u.Id.OpaqueId, check); ok {
+			return true, nil
+		}
 
-		var grantees []string
-		if grantees, err = cn.ListGrantees(ctx); err != nil {
-			appctx.GetLogger(ctx).Error().Err(err).Interface("node", cn).Msg("error listing grantees")
-			return false, err
+		if cn, err = cn.Parent(); err != nil {
+			return false, errors.Wrap(err, "Decomposedfs: error getting parent "+cn.ParentID)
 		}
+	}
+
+	// also check permissions on root, eg. for for project spaces
+	if ok := nodeHasPermission(ctx, cn, groupsMap, u.Id.OpaqueId, check); ok {
+		return true, nil
+	}
+
+	return false, nil
+}
+
+func nodeHasPermission(ctx context.Context, cn *Node, groupsMap map[string]bool, userid string, check func(*provider.ResourcePermissions) bool) (ok bool) {
 
-		userace := xattrs.GrantPrefix + xattrs.UserAcePrefix + u.Id.OpaqueId
-		userFound := false
-		for i := range grantees {
-			// we only need the find the user once per node
-			switch {
-			case !userFound && grantees[i] == userace:
+	var grantees []string
+	var err error
+	if grantees, err = cn.ListGrantees(ctx); err != nil {
+		appctx.GetLogger(ctx).Error().Err(err).Interface("node", cn).Msg("error listing grantees")
+		return false
+	}
+
+	userace := xattrs.GrantPrefix + xattrs.UserAcePrefix + userid
+	userFound := false
+	for i := range grantees {
+		// we only need the find the user once per node
+		var g *provider.Grant
+		switch {
+		case !userFound && grantees[i] == userace:
+			g, err = cn.ReadGrant(ctx, grantees[i])
+		case strings.HasPrefix(grantees[i], xattrs.GrantPrefix+xattrs.GroupAcePrefix):
+			gr := strings.TrimPrefix(grantees[i], xattrs.GrantPrefix+xattrs.GroupAcePrefix)
+			if groupsMap[gr] {
 				g, err = cn.ReadGrant(ctx, grantees[i])
-			case strings.HasPrefix(grantees[i], xattrs.GrantPrefix+xattrs.GroupAcePrefix):
-				gr := strings.TrimPrefix(grantees[i], xattrs.GrantPrefix+xattrs.GroupAcePrefix)
-				if groupsMap[gr] {
-					g, err = cn.ReadGrant(ctx, grantees[i])
-				} else {
-					// no need to check attribute
-					continue
-				}
-			default:
+			} else {
 				// no need to check attribute
 				continue
 			}
-
-			switch {
-			case err == nil:
-				appctx.GetLogger(ctx).Debug().Interface("node", cn).Str("grant", grantees[i]).Interface("permissions", g.GetPermissions()).Msg("checking permissions")
-				if check(g.GetPermissions()) {
-					return true, nil
-				}
-			case isAttrUnset(err):
-				err = nil
-				appctx.GetLogger(ctx).Error().Interface("node", cn).Str("grant", grantees[i]).Interface("grantees", grantees).Msg("grant vanished from node after listing")
-			default:
-				appctx.GetLogger(ctx).Error().Err(err).Interface("node", cn).Str("grant", grantees[i]).Msg("error reading permissions")
-				return false, err
-			}
+		default:
+			// no need to check attribute
+			continue
 		}
 
-		if cn, err = cn.Parent(); err != nil {
-			return false, errors.Wrap(err, "Decomposedfs: error getting parent "+cn.ParentID)
+		switch {
+		case err == nil:
+			appctx.GetLogger(ctx).Debug().Interface("node", cn).Str("grant", grantees[i]).Interface("permissions", g.GetPermissions()).Msg("checking permissions")
+			if check(g.GetPermissions()) {
+				return true
+			}
+		case isAttrUnset(err):
+			appctx.GetLogger(ctx).Error().Interface("node", cn).Str("grant", grantees[i]).Interface("grantees", grantees).Msg("grant vanished from node after listing")
+		default:
+			appctx.GetLogger(ctx).Error().Err(err).Interface("node", cn).Str("grant", grantees[i]).Msg("error reading permissions")
+			return false
 		}
 	}
 
-	// NOTE: this log is being printed 1 million times on a simple test. TODO: Check if this is expected
-	// appctx.GetLogger(ctx).Debug().Interface("permissions", NoPermissions()).Interface("node", n).Interface("user", u).Msg("no grant found, returning default permissions")
-	return false, nil
+	return false
 }
 
 func (p *Permissions) getUserAndPermissions(ctx context.Context, n *Node) (*userv1beta1.User, *provider.ResourcePermissions) {

pkg/storage/utils/decomposedfs/spaces.go

@@ -216,12 +216,6 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 		matches = append(matches, m...)
 	}
 
-	u, ok := ctxpkg.ContextGetUser(ctx)
-	if !ok {
-		appctx.GetLogger(ctx).Debug().Msg("expected user in context")
-		return spaces, nil
-	}
-
 	// FIXME if the space does not exist try a node as the space root.
 
 	// But then the whole /spaces/{spaceType}/{spaceid} becomes obsolete
@@ -235,41 +229,39 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 
 	numShares := 0
 	for i := range matches {
+		var target string
+		var err error
 		// always read link in case storage space id != node id
-		if target, err := os.Readlink(matches[i]); err != nil {
+		if target, err = os.Readlink(matches[i]); err != nil {
 			appctx.GetLogger(ctx).Error().Err(err).Str("match", matches[i]).Msg("could not read link, skipping")
 			continue
-		} else {
-			n, err := node.ReadNode(ctx, fs.lu, filepath.Base(target))
-			if err != nil {
-				appctx.GetLogger(ctx).Error().Err(err).Str("id", filepath.Base(target)).Msg("could not read node, skipping")
-				continue
-			}
+		}
 
-			spaceType := filepath.Base(filepath.Dir(matches[i]))
+		n, err := node.ReadNode(ctx, fs.lu, filepath.Base(target))
+		if err != nil {
+			appctx.GetLogger(ctx).Error().Err(err).Str("id", filepath.Base(target)).Msg("could not read node, skipping")
+			continue
+		}
 
-			owner, err := n.Owner()
-			if err != nil {
-				appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not read owner, skipping")
-				continue
-			}
+		spaceType := filepath.Base(filepath.Dir(matches[i]))
 
-			if spaceType == "share" && utils.UserEqual(u.Id, owner) {
-				numShares++
-				// do not list shares as spaces for the owner
-				continue
-			}
+		// FIXME type share evolved to grant on the edge branch ... make it configurable if the driver should support them or not for now ... ignore type share
+		if spaceType == "share" {
+			numShares++
+			// do not list shares as spaces for the owner
+			continue
+		}
 
-			// TODO apply more filters
-			space, err := fs.storageSpaceFromNode(ctx, n, matches[i], permissions)
-			if err != nil {
-				if _, ok := err.(errtypes.IsPermissionDenied); !ok {
-					appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not convert to storage space")
-				}
-				continue
+		// TODO apply more filters
+		space, err := fs.storageSpaceFromNode(ctx, n, spaceType, matches[i], permissions)
+		if err != nil {
+			if _, ok := err.(errtypes.IsPermissionDenied); !ok {
+				appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not convert to storage space")
 			}
-			spaces = append(spaces, space)
+			continue
 		}
+		spaces = append(spaces, space)
+
 	}
 	// if there are no matches (or they happened to be spaces for the owner) and the node is a child return a space
 	if len(matches) <= numShares && nodeID != spaceID {
@@ -280,7 +272,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 			return nil, err
 		}
 		if n.Exists {
-			space, err := fs.storageSpaceFromNode(ctx, n, n.InternalPath(), permissions)
+			space, err := fs.storageSpaceFromNode(ctx, n, "*", n.InternalPath(), permissions)
 			if err != nil {
 				return nil, err
 			}
@@ -414,7 +406,7 @@ func (fs *Decomposedfs) createStorageSpace(ctx context.Context, spaceType, space
 	return nil
 }
 
-func (fs *Decomposedfs) storageSpaceFromNode(ctx context.Context, n *node.Node, nodePath string, permissions map[string]struct{}) (*provider.StorageSpace, error) {
+func (fs *Decomposedfs) storageSpaceFromNode(ctx context.Context, n *node.Node, spaceType, nodePath string, permissions map[string]struct{}) (*provider.StorageSpace, error) {
 	owner, err := n.Owner()
 	if err != nil {
 		return nil, err
@@ -431,7 +423,7 @@ func (fs *Decomposedfs) storageSpaceFromNode(ctx context.Context, n *node.Node,
 		return nil, err
 	}
 
-	glob := filepath.Join(fs.o.Root, "spaces", "*", n.SpaceRoot.ID)
+	glob := filepath.Join(fs.o.Root, "spaces", spaceType, n.SpaceRoot.ID)
 	matches, err := filepath.Glob(glob)
 	if err != nil {
 		return nil, err
@@ -441,7 +433,7 @@ func (fs *Decomposedfs) storageSpaceFromNode(ctx context.Context, n *node.Node,
 		return nil, errtypes.InternalError("expected only one match for " + glob)
 	}
 
-	spaceType := filepath.Base(filepath.Dir(matches[0]))
+	spaceType = filepath.Base(filepath.Dir(matches[0]))
 
 	space := &provider.StorageSpace{
 		Id: &provider.StorageSpaceId{OpaqueId: n.SpaceRoot.ID},