From 2b540fc0353555f7aaa819f7cfe767614094c2db Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 15 Nov 2023 15:36:41 +0100 Subject: [PATCH] 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 --- .../fix-roleconversion-custompermissions.md | 7 ++ .../handlers/apps/sharing/shares/public.go | 4 +- .../handlers/apps/sharing/shares/shares.go | 4 +- pkg/conversions/permissions_test.go | 85 +++++++++++++++++-- pkg/conversions/role.go | 28 ++++-- .../manager/owncloudsql/conversions.go | 2 +- pkg/share/manager/owncloudsql/conversions.go | 2 +- .../fs/owncloudsql/filecache/filecache.go | 2 +- pkg/storage/fs/owncloudsql/owncloudsql.go | 2 +- 9 files changed, 114 insertions(+), 22 deletions(-) create mode 100644 changelog/unreleased/fix-roleconversion-custompermissions.md diff --git a/changelog/unreleased/fix-roleconversion-custompermissions.md b/changelog/unreleased/fix-roleconversion-custompermissions.md new file mode 100644 index 00000000000..9f7b8de9875 --- /dev/null +++ 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 index 051783b5414..092d6c3b4a0 100644 --- 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 index 1ec428fce02..2621a331326 100644 --- 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 index de41ce47703..39087ee87d5 100644 --- 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 index b15e7fd9643..ab713c25ee3 100644 --- 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 index 4bbf8c8e749..edca2554e56 100644 --- 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 index 8801cb40621..f3aa081d768 100644 --- 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 index a9fac0630d3..82497c932d0 100644 --- 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 index 2e09d1e480f..67ba8123938 100644 --- 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,