From a1f848098a0db3133cb80e00a69129eca9daeb6b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= <jfd@butonic.de>
Date: Fri, 11 Nov 2022 13:34:02 +0000
Subject: [PATCH] fix space update permissions check
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
---
 changelog/unreleased/dont-leak-spaces.md |  3 +-
 pkg/storage/utils/decomposedfs/spaces.go | 57 ++++++++++++------------
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/changelog/unreleased/dont-leak-spaces.md b/changelog/unreleased/dont-leak-spaces.md
index 84428aca5e..779053eeb4 100644
--- a/changelog/unreleased/dont-leak-spaces.md
+++ b/changelog/unreleased/dont-leak-spaces.md
@@ -4,4 +4,5 @@ There were some problems with the `UpdateDrive` func in decomposedfs when it is
 -   When calling with empty request it would leak the complete drive info
 -   When calling with non-empty request it would leak the drive name
 
-https://github.com/cs3org/reva/pull/3447
+https://github.com/cs3org/reva/pull/3449
+https://github.com/cs3org/reva/pull/3453
diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go
index fa2a7e7dd2..f8cd0d4254 100644
--- a/pkg/storage/utils/decomposedfs/spaces.go
+++ b/pkg/storage/utils/decomposedfs/spaces.go
@@ -527,13 +527,6 @@ func (fs *Decomposedfs) UpdateStorageSpace(ctx context.Context, req *provider.Up
 		return nil, err
 	}
 
-	// check if user has access to the drive before continuing
-	if err := fs.checkViewerPermission(ctx, node); err != nil {
-		return &provider.UpdateStorageSpaceResponse{
-			Status: &v1beta11.Status{Code: v1beta11.Code_CODE_NOT_FOUND, Message: err.Error()},
-		}, nil
-	}
-
 	metadata := make(map[string]string, 5)
 	if space.Name != "" {
 		metadata[xattrs.NameAttr] = space.Name
@@ -573,38 +566,44 @@ func (fs *Decomposedfs) UpdateStorageSpace(ctx context.Context, req *provider.Up
 	}
 
 	// TODO change the permission handling
-	// these three attributes need manager permissions
-	if space.Name != "" || mapHasKey(metadata, xattrs.SpaceDescriptionAttr) || restore {
+	switch {
+	case space.Name != "", mapHasKey(metadata, xattrs.SpaceDescriptionAttr), restore:
+		// these three attributes need manager permissions
 		err = fs.checkManagerPermission(ctx, node)
-	}
-	if err != nil {
-		if restore {
-			// a disabled space is invisible to non admins
+		if err != nil {
+			if restore {
+				// a disabled space is invisible to non admins
+				return &provider.UpdateStorageSpaceResponse{
+					Status: &v1beta11.Status{Code: v1beta11.Code_CODE_NOT_FOUND, Message: err.Error()},
+				}, nil
+			}
 			return &provider.UpdateStorageSpaceResponse{
-				Status: &v1beta11.Status{Code: v1beta11.Code_CODE_NOT_FOUND, Message: err.Error()},
+				Status: &v1beta11.Status{Code: v1beta11.Code_CODE_PERMISSION_DENIED, Message: err.Error()},
 			}, nil
 		}
-		return &provider.UpdateStorageSpaceResponse{
-			Status: &v1beta11.Status{Code: v1beta11.Code_CODE_PERMISSION_DENIED, Message: err.Error()},
-		}, nil
-	}
-	// these three attributes need editor permissions
-	if mapHasKey(metadata, xattrs.SpaceReadmeAttr) || mapHasKey(metadata, xattrs.SpaceAliasAttr) || mapHasKey(metadata, xattrs.SpaceImageAttr) {
+	case mapHasKey(metadata, xattrs.SpaceReadmeAttr), mapHasKey(metadata, xattrs.SpaceAliasAttr), mapHasKey(metadata, xattrs.SpaceImageAttr):
+		// these three attributes need editor permissions
 		err = fs.checkEditorPermission(ctx, node)
-	}
-	if err != nil {
-		return &provider.UpdateStorageSpaceResponse{
-			Status: &v1beta11.Status{Code: v1beta11.Code_CODE_PERMISSION_DENIED, Message: err.Error()},
-		}, nil
-	}
-
-	// this attribute needs a permission on the user role
-	if mapHasKey(metadata, xattrs.QuotaAttr) {
+		if err != nil {
+			return &provider.UpdateStorageSpaceResponse{
+				Status: &v1beta11.Status{Code: v1beta11.Code_CODE_PERMISSION_DENIED, Message: err.Error()},
+			}, nil
+		}
+	case mapHasKey(metadata, xattrs.QuotaAttr):
+		// this attribute needs a permission on the user role
 		if !fs.canSetSpaceQuota(ctx, spaceID) {
 			return &provider.UpdateStorageSpaceResponse{
 				Status: &v1beta11.Status{Code: v1beta11.Code_CODE_PERMISSION_DENIED, Message: errors.New("No permission to set space quota").Error()},
 			}, nil
 		}
+	default:
+		// you may land here when making an update request without changes
+		// check if user has access to the drive before continuing
+		if err := fs.checkViewerPermission(ctx, node); err != nil {
+			return &provider.UpdateStorageSpaceResponse{
+				Status: &v1beta11.Status{Code: v1beta11.Code_CODE_NOT_FOUND},
+			}, nil
+		}
 	}
 
 	err = node.SetXattrs(metadata)