bring back manager delete and fix tests

Signed-off-by: jkoberg <jkoberg@owncloud.com>
cs3org · May 16, 2023 · 0a46537 · 0a46537
1 parent ec41b0b
commit 0a46537
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 50 deletions.
diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go
@@ -652,29 +652,9 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De
 		return errtypes.InternalError(fmt.Sprintf("space %s does not have a spacetype, possible corrupt decompsedfs", n.ID))
 	}
 
-	// - spaces of type personal can be deleted by users with the "delete-all-home-spaces" permission
-	// - a User with the "delete-all-spaces" permission can delete any project space
-	// - otherwise a space can be disabled by its manager (i.e. users have the "remove" grant) or a user with "Drive.ReadWriteEnabled"
-	switch {
-	case st == "personal":
-		if !fs.p.DeleteAllHomeSpaces(ctx) {
-			return errtypes.PermissionDenied(fmt.Sprintf("user is not allowed to delete home space %s", n.ID))
-		}
-	case purge:
-		// We are trying to purge a space - delete permission is needed for that
-		if !fs.p.DeleteAllSpaces(ctx) {
-			return errtypes.PermissionDenied(fmt.Sprintf("user is not allowed to purge space %s", n.ID))
-		}
-	default: // aka "disable a non-personal space"
-		// managers and users with 'space ability' permission are allowed to disable a drive
-		if !fs.p.SpaceAbility(ctx, spaceID) {
-			rp, err := fs.p.AssemblePermissions(ctx, n)
-			if err != nil || !IsManager(rp) {
-				return errtypes.PermissionDenied(fmt.Sprintf("user is not allowed to delete spaces %s", n.ID))
-			}
-		}
+	if err := canDeleteSpace(ctx, spaceID, st, purge, n, fs.p); err != nil {
+		return err
 	}
-
 	if purge {
 		if !n.IsDisabled() {
 			return errtypes.NewErrtypeFromStatus(status.NewInvalid(ctx, "can't purge enabled space"))
@@ -1059,3 +1039,35 @@ func isGrantExpired(g *provider.Grant) bool {
 func (fs *Decomposedfs) getSpaceRoot(spaceID string) string {
 	return filepath.Join(fs.o.Root, "spaces", lookup.Pathify(spaceID, 1, 2))
 }
+
+// Space deletion can be tricky as there are lots of different cases:
+// - spaces of type personal can only be disabled and deleted by users with the "delete-all-home-spaces" permission
+// - a user with the "delete-all-spaces" permission may delete but not enable/disable any project space
+// - a user with the "Drive.ReadWriteEnabled" permission may enable/disable but not delete any project space
+// - a project space can always be enabled/disabled/deleted by its manager (i.e. users have the "remove" grant)
+func canDeleteSpace(ctx context.Context, spaceID string, typ string, purge bool, n *node.Node, p Permissions) error {
+	// delete-all-home spaces allows to disable and delete a personal space
+	if typ == "personal" {
+		if p.DeleteAllHomeSpaces(ctx) {
+			return nil
+		}
+		return errtypes.PermissionDenied("user is not allowed to delete a personal space")
+	}
+
+	// space managers are allowed to disable and delete their project spaces
+	if rp, err := p.AssemblePermissions(ctx, n); err == nil && IsManager(rp) {
+		return nil
+	}
+
+	// delete-all-spaces permissions allows to delete (purge, NOT disable) project spaces
+	if purge && p.DeleteAllSpaces(ctx) {
+		return nil
+	}
+
+	// Drive.ReadWriteEnabled allows to disable a space
+	if !purge && p.SpaceAbility(ctx, spaceID) {
+		return nil
+	}
+
+	return errtypes.PermissionDenied(fmt.Sprintf("user is not allowed to delete space %s", n.ID))
+}
diff --git a/pkg/storage/utils/decomposedfs/spaces_test.go b/pkg/storage/utils/decomposedfs/spaces_test.go
@@ -147,7 +147,7 @@ var _ = Describe("Spaces", func() {
 				})
 				Expect(err).To(Not(HaveOccurred()))
 			})
-			It("succeeds on trying to delete homespace as user with 'delete-all-spaces' permission", func() {
+			It("fails on trying to delete homespace as user with 'delete-all-spaces' permission", func() {
 				ctx := ctxpkg.ContextSetUser(context.Background(), env.DeleteAllSpacesUser)
 				resp, err := env.Fs.ListStorageSpaces(env.Ctx, nil, false)
 				Expect(err).ToNot(HaveOccurred())
@@ -158,52 +158,51 @@ var _ = Describe("Spaces", func() {
 				err = env.Fs.DeleteStorageSpace(ctx, &provider.DeleteStorageSpaceRequest{
 					Id: resp[0].GetId(),
 				})
-				Expect(err).To(Not(HaveOccurred()))
+				Expect(err).To(HaveOccurred())
 			})
 		})
 
-		Context("can delete project spaces", func() {
-			It("fails as a unprivileged user", func() {
+		Context("can delete (purge) project spaces", func() {
+			var delReq *provider.DeleteStorageSpaceRequest
+			BeforeEach(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))
-				ctx := ctxpkg.ContextSetUser(context.Background(), env.Users[1])
-				err = env.Fs.DeleteStorageSpace(ctx, &provider.DeleteStorageSpaceRequest{
-					Id: resp.StorageSpace.GetId(),
+				spaceID := resp.StorageSpace.GetId()
+				err = env.Fs.DeleteStorageSpace(env.Ctx, &provider.DeleteStorageSpaceRequest{
+					Id: spaceID,
 				})
+				Expect(err).To(Not(HaveOccurred()))
+				delReq = &provider.DeleteStorageSpaceRequest{
+					Opaque: &typesv1beta1.Opaque{
+						Map: map[string]*typesv1beta1.OpaqueEntry{
+							"purge": {
+								Decoder: "plain",
+								Value:   []byte("true"),
+							},
+						},
+					},
+					Id: spaceID,
+				}
+			})
+			It("fails as a unprivileged user", func() {
+				ctx := ctxpkg.ContextSetUser(context.Background(), env.Users[1])
+				err := env.Fs.DeleteStorageSpace(ctx, delReq)
 				Expect(err).To(HaveOccurred())
 			})
 			It("fails as a user with 'delete-all-home-spaces privilege", 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))
 				ctx := ctxpkg.ContextSetUser(context.Background(), env.DeleteHomeSpacesUser)
-				err = env.Fs.DeleteStorageSpace(ctx, &provider.DeleteStorageSpaceRequest{
-					Id: resp.StorageSpace.GetId(),
-				})
+				err := env.Fs.DeleteStorageSpace(ctx, delReq)
 				Expect(err).To(HaveOccurred())
 			})
 			It("succeeds as a user with 'delete-all-spaces privilege", 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))
 				ctx := ctxpkg.ContextSetUser(context.Background(), env.DeleteAllSpacesUser)
-				err = env.Fs.DeleteStorageSpace(ctx, &provider.DeleteStorageSpaceRequest{
-					Id: resp.StorageSpace.GetId(),
-				})
+				err := env.Fs.DeleteStorageSpace(ctx, delReq)
 				Expect(err).To(Not(HaveOccurred()))
 			})
 			It("succeeds as the space owner", 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))
-				err = env.Fs.DeleteStorageSpace(env.Ctx, &provider.DeleteStorageSpaceRequest{
-					Id: resp.StorageSpace.GetId(),
-				})
+				err := env.Fs.DeleteStorageSpace(env.Ctx, delReq)
 				Expect(err).To(Not(HaveOccurred()))
 			})
 		})