Decomposedfs allow arbitrary length spaceid

changelog/unreleased/decomposedfs-allow-arbitrary-length-spaceid.md

@@ -0,0 +1,5 @@
+Bugfix: Handle non uuid space and nodeid in decomposedfs
+
+The decomposedfs no longer panics when trying to look up spaces with a non uuid length id.
+
+https://github.com/cs3org/reva/pull/2854

pkg/storage/utils/decomposedfs/spaces.go

@@ -215,20 +215,21 @@ func (fs *Decomposedfs) canCreateSpace(ctx context.Context, spaceID string) bool
 	return checkRes.Status.Code == v1beta11.Code_CODE_OK
 }
 
-func readSpaceAndNodeFromSpaceTypeLink(path string) (string, string, error) {
+// ReadSpaceAndNodeFromSpaceTypeLink reads a symlink and parses space and node id if the link has the correct format, eg:
+// ../../spaces/4c/510ada-c86b-4815-8820-42cdf82c3d51/nodes/4c/51/0a/da/-c86b-4815-8820-42cdf82c3d51
+// ../../spaces/4c/510ada-c86b-4815-8820-42cdf82c3d51/nodes/4c/51/0a/da/-c86b-4815-8820-42cdf82c3d51.T.2022-02-24T12:35:18.196484592Z
+func ReadSpaceAndNodeFromSpaceTypeLink(path string) (string, string, error) {
 	link, err := os.Readlink(path)
 	if err != nil {
 		return "", "", err
 	}
-	// ../../spaces/4c/510ada-c86b-4815-8820-42cdf82c3d51/nodes/4c/51/0a/da/-c86b-4815-8820-42cdf82c3d51
-	// ../../spaces/4c/510ada-c86b-4815-8820-42cdf82c3d51/nodes/4c/51/0a/da/-c86b-4815-8820-42cdf82c3d51.T.2022-02-24T12:35:18.196484592Z
-	// TODO use filepath.Separator to support windows
-	link = strings.ReplaceAll(link, "/", "")
-	// ....spaces4c510ada-c86b-4815-8820-42cdf82c3d51nodes4c510ada-c86b-4815-8820-42cdf82c3d51
-	if link[0:10] != "....spaces" || link[46:51] != "nodes" {
+	// ../../spaces/sp/ace-id/nodes/sh/or/tn/od/eid
+	// 0  1  2      3  4      5     6  7  8  9  10
+	parts := strings.Split(link, string(filepath.Separator))
+	if len(parts) != 11 || parts[0] != ".." || parts[1] != ".." || parts[2] != "spaces" || parts[5] != "nodes" {
 		return "", "", errtypes.InternalError("malformed link")
 	}
-	return link[10:46], link[51:], nil
+	return strings.Join(parts[3:5], ""), strings.Join(parts[6:11], ""), nil
 }
 
 // ListStorageSpaces returns a list of StorageSpaces.
@@ -338,7 +339,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 			continue
 		}
 		// always read link in case storage space id != node id
-		spaceID, nodeID, err = readSpaceAndNodeFromSpaceTypeLink(matches[i])
+		spaceID, nodeID, err = ReadSpaceAndNodeFromSpaceTypeLink(matches[i])
 		if err != nil {
 			appctx.GetLogger(ctx).Error().Err(err).Str("match", matches[i]).Msg("could not read link, skipping")
 			continue

pkg/storage/utils/decomposedfs/spaces_test.go

@@ -19,65 +19,28 @@
 package decomposedfs_test
 
 import (
+	"os"
+	"path/filepath"
+
 	permissionsv1beta1 "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1"
 	rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
+	"github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs"
 	helpers "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/testhelpers"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"github.com/stretchr/testify/mock"
 )
 
-var _ = Describe("Create Spaces", func() {
-	var (
-		env *helpers.TestEnv
-	)
-
-	BeforeEach(func() {
-		var err error
-		env, err = helpers.NewTestEnv(nil)
-		Expect(err).ToNot(HaveOccurred())
-		env.PermissionsClient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return(&permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}, nil)
-	})
-
-	AfterEach(func() {
-		if env != nil {
-			env.Cleanup()
-		}
-	})
+var _ = Describe("Spaces", func() {
 
-	Context("during login", func() {
-		It("space is created", func() {
-			resp, err := env.Fs.ListStorageSpaces(env.Ctx, nil, false)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(len(resp)).To(Equal(1))
-			Expect(string(resp[0].Opaque.GetMap()["spaceAlias"].Value)).To(Equal("personal/username"))
-			Expect(resp[0].Name).To(Equal("username"))
-			Expect(resp[0].SpaceType).To(Equal("personal"))
-		})
-	})
-	Context("when creating a space", func() {
-		It("project space is created", func() {
-			resp, err := env.Fs.CreateStorageSpace(env.Ctx, &provider.CreateStorageSpaceRequest{Name: "Mission to Mars", Type: "project"})
-			Expect(err).ToNot(HaveOccurred())
-			Expect(resp.Status.Code).To(Equal(rpcv1beta1.Code_CODE_OK))
-			Expect(resp.StorageSpace).ToNot(Equal(nil))
-			Expect(string(resp.StorageSpace.Opaque.Map["spaceAlias"].Value)).To(Equal("project/mission-to-mars"))
-			Expect(resp.StorageSpace.Name).To(Equal("Mission to Mars"))
-			Expect(resp.StorageSpace.SpaceType).To(Equal("project"))
-		})
-	})
-	Describe("Create Spaces with custom alias template", func() {
+	Describe("Create Space", func() {
 		var (
 			env *helpers.TestEnv
 		)
-
 		BeforeEach(func() {
 			var err error
-			env, err = helpers.NewTestEnv(map[string]interface{}{
-				"personalspacealias_template": "{{.SpaceType}}/{{.Email.Local}}@{{.Email.Domain}}",
-				"generalspacealias_template":  "{{.SpaceType}}:{{.SpaceName | replace \" \" \"-\" | upper}}",
-			})
+			env, err = helpers.NewTestEnv(nil)
 			Expect(err).ToNot(HaveOccurred())
 			env.PermissionsClient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return(&permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}, nil)
 		})
@@ -87,23 +50,108 @@ var _ = Describe("Create Spaces", func() {
 				env.Cleanup()
 			}
 		})
+
 		Context("during login", func() {
-			It("personal space is created with custom alias", func() {
+			It("space is created", func() {
 				resp, err := env.Fs.ListStorageSpaces(env.Ctx, nil, false)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(len(resp)).To(Equal(1))
-				Expect(string(resp[0].Opaque.GetMap()["spaceAlias"].Value)).To(Equal("personal/username@_unknown"))
+				Expect(string(resp[0].Opaque.GetMap()["spaceAlias"].Value)).To(Equal("personal/username"))
+				Expect(resp[0].Name).To(Equal("username"))
+				Expect(resp[0].SpaceType).To(Equal("personal"))
 			})
 		})
-		Context("creating a space", func() {
-			It("project space is created with custom alias", func() {
-				resp, err := env.Fs.CreateStorageSpace(env.Ctx, &provider.CreateStorageSpaceRequest{Name: "Mission to Venus", Type: "project"})
+		Context("when creating a space", func() {
+			It("project space is created", func() {
+				resp, err := env.Fs.CreateStorageSpace(env.Ctx, &provider.CreateStorageSpaceRequest{Name: "Mission to Mars", Type: "project"})
 				Expect(err).ToNot(HaveOccurred())
 				Expect(resp.Status.Code).To(Equal(rpcv1beta1.Code_CODE_OK))
 				Expect(resp.StorageSpace).ToNot(Equal(nil))
-				Expect(string(resp.StorageSpace.Opaque.Map["spaceAlias"].Value)).To(Equal("project:MISSION-TO-VENUS"))
+				Expect(string(resp.StorageSpace.Opaque.Map["spaceAlias"].Value)).To(Equal("project/mission-to-mars"))
+				Expect(resp.StorageSpace.Name).To(Equal("Mission to Mars"))
+				Expect(resp.StorageSpace.SpaceType).To(Equal("project"))
+			})
+		})
+		Describe("Create Spaces with custom alias template", func() {
+			var (
+				env *helpers.TestEnv
+			)
+
+			BeforeEach(func() {
+				var err error
+				env, err = helpers.NewTestEnv(map[string]interface{}{
+					"personalspacealias_template": "{{.SpaceType}}/{{.Email.Local}}@{{.Email.Domain}}",
+					"generalspacealias_template":  "{{.SpaceType}}:{{.SpaceName | replace \" \" \"-\" | upper}}",
+				})
+				Expect(err).ToNot(HaveOccurred())
+				env.PermissionsClient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return(&permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}, nil)
+			})
 
+			AfterEach(func() {
+				if env != nil {
+					env.Cleanup()
+				}
 			})
+			Context("during login", func() {
+				It("personal space is created with custom alias", func() {
+					resp, err := env.Fs.ListStorageSpaces(env.Ctx, nil, false)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(len(resp)).To(Equal(1))
+					Expect(string(resp[0].Opaque.GetMap()["spaceAlias"].Value)).To(Equal("personal/username@_unknown"))
+				})
+			})
+			Context("creating a space", func() {
+				It("project space is created with custom alias", func() {
+					resp, err := env.Fs.CreateStorageSpace(env.Ctx, &provider.CreateStorageSpaceRequest{Name: "Mission to Venus", Type: "project"})
+					Expect(err).ToNot(HaveOccurred())
+					Expect(resp.Status.Code).To(Equal(rpcv1beta1.Code_CODE_OK))
+					Expect(resp.StorageSpace).ToNot(Equal(nil))
+					Expect(string(resp.StorageSpace.Opaque.Map["spaceAlias"].Value)).To(Equal("project:MISSION-TO-VENUS"))
+
+				})
+			})
+		})
+	})
+
+	Describe("ReadSpaceAndNodeFromSpaceTypeLink", func() {
+		var (
+			tmpdir string
+		)
+
+		BeforeEach(func() {
+			tmpdir, _ = os.MkdirTemp(os.TempDir(), "ReadSpaceAndNodeFromSpaceTypeLink-")
 		})
+
+		AfterEach(func() {
+			if tmpdir != "" {
+				os.RemoveAll(tmpdir)
+			}
+		})
+
+		DescribeTable("ReadSpaceAndNodeFromSpaceTypeLink",
+			func(link string, expectSpace string, expectedNode string, shouldErr bool) {
+				err := os.Symlink(link, filepath.Join(tmpdir, "link"))
+				Expect(err).ToNot(HaveOccurred())
+
+				space, node, err := decomposedfs.ReadSpaceAndNodeFromSpaceTypeLink(filepath.Join(tmpdir, "link"))
+				if shouldErr {
+					Expect(err).To(HaveOccurred())
+				} else {
+					Expect(err).ToNot(HaveOccurred())
+				}
+				Expect(space).To(Equal(expectSpace))
+				Expect(node).To(Equal(expectedNode))
+			},
+
+			Entry("invalid number of slashes", "../../spaces/sp_ace-id/nodes/sh/or/tn/od/eid", "", "", true),
+			Entry("does not contain spaces", "../../spac_s/sp/ace-id/nodes/sh/or/tn/od/eid", "", "", true),
+			Entry("does not contain nodes", "../../spaces/sp/ace-id/nod_s/sh/or/tn/od/eid", "", "", true),
+			Entry("does not start with ..", "_./../spaces/sp/ace-id/nodes/sh/or/tn/od/eid", "", "", true),
+			Entry("does not start with ../..", "../_./spaces/sp/ace-id/nodes/sh/or/tn/od/eid", "", "", true),
+			Entry("invalid", "../../spaces/space-id/nodes/sh/or/tn/od/eid", "", "", true),
+			Entry("uuid", "../../spaces/4c/510ada-c86b-4815-8820-42cdf82c3d51/nodes/4c/51/0a/da/-c86b-4815-8820-42cdf82c3d51", "4c510ada-c86b-4815-8820-42cdf82c3d51", "4c510ada-c86b-4815-8820-42cdf82c3d51", false),
+			Entry("uuid", "../../spaces/4c/510ada-c86b-4815-8820-42cdf82c3d51/nodes/4c/51/0a/da/-c86b-4815-8820-42cdf82c3d51.T.2022-02-24T12:35:18.196484592Z", "4c510ada-c86b-4815-8820-42cdf82c3d51", "4c510ada-c86b-4815-8820-42cdf82c3d51.T.2022-02-24T12:35:18.196484592Z", false),
+			Entry("short", "../../spaces/sp/ace-id/nodes/sh/or/tn/od/eid", "space-id", "shortnodeid", false),
+		)
 	})
 })