Prevent editing disabled spaces (#2856)

* disable editing of disabled spaces Signed-off-by: jkoberg <jkoberg@owncloud.com> * changelog Signed-off-by: jkoberg <jkoberg@owncloud.com> * fix unit tests Signed-off-by: jkoberg <jkoberg@owncloud.com>
cs3org · May 13, 2022 · 5b97700 · 5b97700
1 parent 6a0a516
commit 5b97700
Show file tree

Hide file tree

Showing 8 changed files with 26 additions and 17 deletions.
diff --git a/changelog/unreleased/prevent-editing-disabled-spaces.md b/changelog/unreleased/prevent-editing-disabled-spaces.md
@@ -0,0 +1,5 @@
+Change: Do not allow to edit disabled spaces
+
+Previously managers could still upload to disabled spaces. This is now forbidden
+
+https://github.com/cs3org/reva/pull/2856
diff --git a/pkg/storage/utils/decomposedfs/lookup/lookup.go b/pkg/storage/utils/decomposedfs/lookup/lookup.go
@@ -74,7 +74,7 @@ func (lu *Lookup) NodeFromID(ctx context.Context, id *provider.ResourceId) (n *n
 		// The Resource references the root of a space
 		return lu.NodeFromSpaceID(ctx, id)
 	}
-	return node.ReadNode(ctx, lu, id.StorageId, id.OpaqueId)
+	return node.ReadNode(ctx, lu, id.StorageId, id.OpaqueId, false)
 }
 
 // Pathify segments the beginning of a string into depth segments of width length
@@ -95,7 +95,7 @@ func Pathify(id string, depth, width int) string {
 
 // NodeFromSpaceID converts a resource id without an opaque id into a Node
 func (lu *Lookup) NodeFromSpaceID(ctx context.Context, id *provider.ResourceId) (n *node.Node, err error) {
-	node, err := node.ReadNode(ctx, lu, id.StorageId, id.StorageId)
+	node, err := node.ReadNode(ctx, lu, id.StorageId, id.StorageId, false)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go
@@ -183,8 +183,7 @@ func (n *Node) WriteOwner(owner *userpb.UserId) error {
 }
 
 // ReadNode creates a new instance from an id and checks if it exists
-// FIXME check if user is allowed to access disabled spaces
-func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string) (n *Node, err error) {
+func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string, canListDisabledSpace bool) (n *Node, err error) {
 
 	// read space root
 	r := &Node{
@@ -202,6 +201,11 @@ func ReadNode(ctx context.Context, lu PathLookup, spaceID, nodeID string) (n *No
 	}
 	r.Exists = true
 
+	if !canListDisabledSpace && r.IsDisabled() {
+		// no permission = not found
+		return nil, errtypes.NotFound(spaceID)
+	}
+
 	// check if this is a space root
 	if spaceID == nodeID {
 		return r, nil
@@ -336,7 +340,7 @@ func (n *Node) Child(ctx context.Context, name string) (*Node, error) {
 	}
 
 	var c *Node
-	c, err = ReadNode(ctx, n.lu, spaceID, nodeID)
+	c, err = ReadNode(ctx, n.lu, spaceID, nodeID, false)
 	if err != nil {
 		return nil, errors.Wrap(err, "could not read child node")
 	}

diff --git a/pkg/storage/utils/decomposedfs/node/node_test.go b/pkg/storage/utils/decomposedfs/node/node_test.go
@@ -71,7 +71,7 @@ var _ = Describe("Node", func() {
 			})
 			Expect(err).ToNot(HaveOccurred())
 
-			n, err := node.ReadNode(env.Ctx, env.Lookup, lookupNode.SpaceID, lookupNode.ID)
+			n, err := node.ReadNode(env.Ctx, env.Lookup, lookupNode.SpaceID, lookupNode.ID, false)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(n.BlobID).To(Equal("file1-blobid"))
 		})

diff --git a/pkg/storage/utils/decomposedfs/revisions.go b/pkg/storage/utils/decomposedfs/revisions.go
@@ -109,7 +109,7 @@ func (fs *Decomposedfs) DownloadRevision(ctx context.Context, ref *provider.Refe
 
 	spaceID := ref.ResourceId.OpaqueId
 	// check if the node is available and has not been deleted
-	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0])
+	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0], false)
 	if err != nil {
 		return nil, err
 	}
@@ -154,7 +154,7 @@ func (fs *Decomposedfs) RestoreRevision(ctx context.Context, ref *provider.Refer
 
 	spaceID := ref.ResourceId.StorageId
 	// check if the node is available and has not been deleted
-	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0])
+	n, err := node.ReadNode(ctx, fs.lu, spaceID, kp[0], false)
 	if err != nil {
 		return err
 	}

diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go
@@ -81,7 +81,7 @@ func (fs *Decomposedfs) CreateStorageSpace(ctx context.Context, req *provider.Cr
 		alias = templates.WithSpacePropertiesAndUser(u, req.Type, req.Name, fs.o.PersonalSpaceAliasTemplate)
 	}
 
-	root, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID)
+	root, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true) // will fall into `Exists` case below
 	if err == nil && root.Exists {
 		return nil, errtypes.AlreadyExists("decomposedfs: spaces: space already exists")
 	}
@@ -286,7 +286,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 
 	if spaceID != spaceIDAny && nodeID != spaceIDAny {
 		// try directly reading the node
-		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID)
+		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true) // permission to read disabled space is checked later
 		if err != nil {
 			appctx.GetLogger(ctx).Error().Err(err).Str("id", nodeID).Msg("could not read node")
 			return nil, err
@@ -345,7 +345,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 			continue
 		}
 
-		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID)
+		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true)
 		if err != nil {
 			appctx.GetLogger(ctx).Error().Err(err).Str("id", nodeID).Msg("could not read node, skipping")
 			continue
@@ -378,7 +378,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 	// 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 {
 		// try node id
-		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID)
+		n, err := node.ReadNode(ctx, fs.lu, spaceID, nodeID, true) // permission to read disabled space is checked in storageSpaceFromNode
 		if err != nil {
 			return nil, err
 		}
@@ -405,7 +405,7 @@ func (fs *Decomposedfs) UpdateStorageSpace(ctx context.Context, req *provider.Up
 	space := req.StorageSpace
 	spaceID, _, _ := storagespace.SplitID(space.Id.OpaqueId)
 
-	node, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID)
+	node, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true) // permission to read disabled space will be checked later
 	if err != nil {
 		return nil, err
 	}
@@ -506,7 +506,7 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De
 
 	spaceID := req.Id.OpaqueId
 
-	n, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID)
+	n, err := node.ReadNode(ctx, fs.lu, spaceID, spaceID, true) // permission to read disabled space is checked later
 	if err != nil {
 		return err
 	}

diff --git a/pkg/storage/utils/decomposedfs/testhelpers/helpers.go b/pkg/storage/utils/decomposedfs/testhelpers/helpers.go
@@ -205,7 +205,7 @@ func (t *TestEnv) CreateTestStorageSpace(typ string, quota *providerv1beta1.Quot
 	ref := buildRef(space.StorageSpace.Id.OpaqueId, "")
 
 	// the space name attribute is the stop condition in the lookup
-	h, err := node.ReadNode(t.Ctx, t.Lookup, space.StorageSpace.Id.OpaqueId, space.StorageSpace.Id.OpaqueId)
+	h, err := node.ReadNode(t.Ctx, t.Lookup, space.StorageSpace.Id.OpaqueId, space.StorageSpace.Id.OpaqueId, false)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go
@@ -371,7 +371,7 @@ func (t *Tree) ListFolder(ctx context.Context, n *node.Node) ([]*node.Node, erro
 			continue
 		}
 
-		child, err := node.ReadNode(ctx, t.lookup, n.SpaceID, nodeID)
+		child, err := node.ReadNode(ctx, t.lookup, n.SpaceID, nodeID, false)
 		if err != nil {
 			// TODO log
 			continue
@@ -826,7 +826,7 @@ func (t *Tree) readRecycleItem(ctx context.Context, spaceID, key, path string) (
 	}
 
 	recycleNode = node.New(spaceID, nodeID, "", "", 0, "", nil, t.lookup)
-	recycleNode.SpaceRoot, err = node.ReadNode(ctx, t.lookup, spaceID, spaceID)
+	recycleNode.SpaceRoot, err = node.ReadNode(ctx, t.lookup, spaceID, spaceID, false)
 	if err != nil {
 		return
 	}