Fix conversion of custom ocs permissions to roles

When creating shares with just `view` permission we wrongly converted that into the `SpacerViewer` sharing role. The correct role for that would be `legacy`. Fixes: https://github.com/owncloud/enterprise/issues/6209
cs3org · Nov 15, 2023 · 2b540fc · 2b540fc
1 parent 558f619
commit 2b540fc
Show file tree

Hide file tree

Showing 9 changed files with 114 additions and 22 deletions.
diff --git a/changelog/unreleased/fix-roleconversion-custompermissions.md b/changelog/unreleased/fix-roleconversion-custompermissions.md
@@ -0,0 +1,7 @@
+Bugfix: Fix conversion of custom ocs permissions to roles
+
+When creating shares with just `view` permission we wrongly converted that
+into the `SpacerViewer` sharing role. The correct role for that would be `legacy`.
+
+https://github.com/cs3org/reva/pull/4342
+https://github.com/owncloud/enterprise/issues/6209
diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/public.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/public.go
@@ -160,7 +160,7 @@ func (h *Handler) createPublicLinkShare(w http.ResponseWriter, r *http.Request,
 		p := role.OCSPermissions()
 		p &^= conversions.PermissionCreate
 		p &^= conversions.PermissionDelete
-		permissions = conversions.RoleFromOCSPermissions(p).CS3ResourcePermissions()
+		permissions = conversions.RoleFromOCSPermissions(p, statInfo).CS3ResourcePermissions()
 	}
 
 	if !sufficientPermissions(statInfo.PermissionSet, permissions, true) {
@@ -636,7 +636,7 @@ func ocPublicPermToCs3(pk *int) (*provider.ResourcePermissions, error) {
 		return nil, err
 	}
 
-	return conversions.RoleFromOCSPermissions(perm).CS3ResourcePermissions(), nil
+	return conversions.RoleFromOCSPermissions(perm, nil).CS3ResourcePermissions(), nil
 }
 
 // pointer will be nil if no permission is set

diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go
@@ -465,7 +465,7 @@ func (h *Handler) extractPermissions(reqRole string, reqPermissions string, ri *
 					Error:   err,
 				}
 			}
-			role = conversions.RoleFromOCSPermissions(perm)
+			role = conversions.RoleFromOCSPermissions(perm, ri)
 		}
 	}
 
@@ -481,7 +481,7 @@ func (h *Handler) extractPermissions(reqRole string, reqPermissions string, ri *
 				Error:   errors.New("cannot set the requested share permissions"),
 			}
 		}
-		role = conversions.RoleFromOCSPermissions(permissions)
+		role = conversions.RoleFromOCSPermissions(permissions, ri)
 	}
 
 	if !sufficientPermissions(ri.PermissionSet, role.CS3ResourcePermissions(), false) && role.Name != conversions.RoleDenied {

diff --git a/pkg/conversions/permissions_test.go b/pkg/conversions/permissions_test.go
@@ -20,6 +20,8 @@ package conversions
 
 import (
 	"testing"
+
+	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 )
 
 func TestNewPermissions(t *testing.T) {
@@ -141,16 +143,81 @@ func TestPermissions2Role(t *testing.T) {
 		}
 	}
 
-	table := map[Permissions]string{
-		PermissionRead | PermissionShare: RoleViewer,
-		PermissionRead | PermissionWrite | PermissionCreate | PermissionDelete | PermissionShare: RoleEditor,
-		PermissionWrite:                   RoleLegacy,
-		PermissionShare:                   RoleLegacy,
-		PermissionWrite | PermissionShare: RoleLegacy,
+	resourceInfoSpaceRoot := &provider.ResourceInfo{
+		Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER,
+		Id: &provider.ResourceId{
+			StorageId: "storageid",
+			SpaceId:   "spaceid",
+			OpaqueId:  "spaceid",
+		},
+		Space: &provider.StorageSpace{
+			Root: &provider.ResourceId{
+				StorageId: "storageid",
+				SpaceId:   "spaceid",
+				OpaqueId:  "spaceid",
+			},
+		},
+	}
+	resourceInfoDir := &provider.ResourceInfo{
+		Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER,
+		Id: &provider.ResourceId{
+			StorageId: "storageid",
+			SpaceId:   "spaceid",
+			OpaqueId:  "fileid",
+		},
+		Space: &provider.StorageSpace{
+			Root: &provider.ResourceId{
+				StorageId: "storageid",
+				SpaceId:   "spaceid",
+				OpaqueId:  "spaceid",
+			},
+		},
+	}
+
+	type permissionOnResourceInfo2Role struct {
+		permissions  Permissions
+		resourceInfo *provider.ResourceInfo
+		role         string
+	}
+
+	table := []permissionOnResourceInfo2Role{
+		{
+			permissions:  PermissionRead | PermissionShare,
+			role:         RoleViewer,
+			resourceInfo: resourceInfoDir,
+		}, {
+			permissions:  PermissionRead | PermissionShare,
+			role:         RoleLegacy,
+			resourceInfo: resourceInfoSpaceRoot,
+		}, {
+			permissions:  PermissionRead,
+			role:         RoleLegacy,
+			resourceInfo: resourceInfoDir,
+		}, {
+			permissions:  PermissionRead,
+			role:         RoleSpaceViewer,
+			resourceInfo: resourceInfoSpaceRoot,
+		}, {
+			permissions:  PermissionRead | PermissionWrite | PermissionCreate | PermissionDelete | PermissionShare,
+			role:         RoleEditor,
+			resourceInfo: nil,
+		}, {
+			permissions:  PermissionWrite,
+			role:         RoleLegacy,
+			resourceInfo: nil,
+		}, {
+			permissions:  PermissionShare,
+			role:         RoleLegacy,
+			resourceInfo: nil,
+		}, {
+			permissions:  PermissionWrite | PermissionShare,
+			role:         RoleLegacy,
+			resourceInfo: nil,
+		},
 	}
 
-	for permissions, role := range table {
-		actual := RoleFromOCSPermissions(permissions).Name
-		checkRole(role, actual)
+	for _, t := range table {
+		actual := RoleFromOCSPermissions(t.permissions, t.resourceInfo).Name
+		checkRole(t.role, actual)
 	}
 }
diff --git a/pkg/conversions/role.go b/pkg/conversions/role.go
@@ -379,7 +379,7 @@ func NewManagerRole() *Role {
 // RoleFromOCSPermissions tries to map ocs permissions to a role
 // TODO: rethink using this. ocs permissions cannot be assigned 1:1 to roles
 // NOTE: If resharing=false in the system this function will return SpaceViewerRole instead ViewerRole
-func RoleFromOCSPermissions(p Permissions) *Role {
+func RoleFromOCSPermissions(p Permissions, ri *provider.ResourceInfo) *Role {
 	if p == PermissionInvalid {
 		return NewNoneRole()
 	}
@@ -392,12 +392,14 @@ func RoleFromOCSPermissions(p Permissions) *Role {
 
 			return NewSpaceEditorRole()
 		}
-		if p == PermissionRead|PermissionShare {
-			return NewViewerRole(true)
-		}
-		if p == PermissionRead {
+
+		if p == PermissionRead && isSpaceRoot(ri) {
 			return NewSpaceViewerRole()
 		}
+
+		if p == PermissionRead|PermissionShare && !isSpaceRoot(ri) {
+			return NewViewerRole(true)
+		}
 	}
 	if p == PermissionCreate {
 		return NewUploaderRole()
@@ -406,6 +408,22 @@ func RoleFromOCSPermissions(p Permissions) *Role {
 	return NewLegacyRoleFromOCSPermissions(p)
 }
 
+func isSpaceRoot(ri *provider.ResourceInfo) bool {
+	if ri == nil {
+		return false
+	}
+	if ri.Type != provider.ResourceType_RESOURCE_TYPE_CONTAINER {
+		return false
+	}
+
+	if ri.GetId().GetOpaqueId() != ri.GetSpace().GetRoot().GetOpaqueId() ||
+		ri.GetId().GetSpaceId() != ri.GetSpace().GetRoot().GetSpaceId() ||
+		ri.GetId().GetStorageId() != ri.GetSpace().GetRoot().GetStorageId() {
+		return false
+	}
+	return true
+}
+
 // NewLegacyRoleFromOCSPermissions tries to map a legacy combination of ocs permissions to cs3 resource permissions as a legacy role
 func NewLegacyRoleFromOCSPermissions(p Permissions) *Role {
 	r := &Role{

diff --git a/pkg/publicshare/manager/owncloudsql/conversions.go b/pkg/publicshare/manager/owncloudsql/conversions.go
@@ -162,7 +162,7 @@ func intTosharePerm(p int) (*provider.ResourcePermissions, error) {
 		return nil, err
 	}
 
-	return conversions.RoleFromOCSPermissions(perms).CS3ResourcePermissions(), nil
+	return conversions.RoleFromOCSPermissions(perms, nil).CS3ResourcePermissions(), nil
 }
 
 func formatUserID(u *userpb.UserId) string {

diff --git a/pkg/share/manager/owncloudsql/conversions.go b/pkg/share/manager/owncloudsql/conversions.go
@@ -214,7 +214,7 @@ func intTosharePerm(p int) (*provider.ResourcePermissions, error) {
 		return nil, err
 	}
 
-	return conversions.RoleFromOCSPermissions(perms).CS3ResourcePermissions(), nil
+	return conversions.RoleFromOCSPermissions(perms, nil).CS3ResourcePermissions(), nil
 }
 
 func intToShareState(g int) collaboration.ShareState {

diff --git a/pkg/storage/fs/owncloudsql/filecache/filecache.go b/pkg/storage/fs/owncloudsql/filecache/filecache.go
@@ -362,7 +362,7 @@ func (c *Cache) Permissions(ctx context.Context, storage interface{}, p string)
 		return nil, err
 	}
 
-	return conversions.RoleFromOCSPermissions(perms).CS3ResourcePermissions(), nil
+	return conversions.RoleFromOCSPermissions(perms, nil).CS3ResourcePermissions(), nil
 }
 
 // InsertOrUpdate creates or updates a cache entry

diff --git a/pkg/storage/fs/owncloudsql/owncloudsql.go b/pkg/storage/fs/owncloudsql/owncloudsql.go
@@ -650,7 +650,7 @@ func (fs *owncloudsqlfs) readPermissions(ctx context.Context, ip string) (p *pro
 	if err != nil {
 		return nil, err
 	}
-	return conversions.RoleFromOCSPermissions(perms).CS3ResourcePermissions(), nil
+	return conversions.RoleFromOCSPermissions(perms, nil).CS3ResourcePermissions(), nil
 }
 
 // The os not exists error is buried inside the xattr error,