Cleanup Space Delete Permissions

changelog/unreleased/cleanup-space-delete-permissions.md

@@ -0,0 +1,10 @@
+Enhancement: Cleanup Space Delete permissions
+
+Space Delete and Disable permissions ("Drive.ReadWriteEnabled", "delete-all-spaces", "delete-all-home-spaces") were overlapping and not clear differentiatable.
+The new logic is as follows:
+  -  "Drive.ReadWriteEnabled" allows enabling or disabling a project space
+  -  "delete-all-home-spaces" allows deleting personal spaces of users
+  -  "delete-all-spaces" allows deleting a project space
+  -  Space Mangers can still disable/enable a drive
+
+https://github.com/cs3org/reva/pull/3893

pkg/storage/utils/decomposedfs/spaces.go

@@ -652,26 +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))
 	}
 
-	// - a User with the "delete-all-spaces" permission can delete any space
-	// - spaces of type personal can also be deleted by users with the "delete-all-home-spaces" permission
-	// - otherwise a space can be deleted by its manager (i.e. users have the "remove" grant)
-	switch {
-	case fs.p.DeleteAllSpaces(ctx):
-		// We are allowed to delete any space, no further permission checks needed
-	case st == "personal":
-		if !fs.p.DeleteAllHomeSpaces(ctx) {
-			return errtypes.PermissionDenied(fmt.Sprintf("user is not allowed to delete home space %s", n.ID))
-		}
-	default:
-		// managers and users with 'space ability' permission are allowed to disable or purge 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"))
@@ -1056,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))
+}

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()))
 			})
 		})