fix: [OCISDEV-810] fix the space role and custom role checks

changelog/unreleased/bugfix-space-role-bitmap-check.md

@@ -0,0 +1,9 @@
+Bugfix: Harden space role classification and grant checks
+
+The `IsManager` space predicate treated any single grant-management bit as a full
+manager, allowing a partial grant to act as a stealth manager. It now requires the
+full add/update/remove grant triad. `IsEditor`/`IsViewer` keep classifying by their
+defining capability bit so space-role variants still work. Partial grant-management
+grants are rejected on space roots on both the create and update paths.
+
+https://github.com/owncloud/reva/pull/645

internal/grpc/services/gateway/usershareprovider.go

@@ -28,6 +28,7 @@ import (
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
 	"github.com/owncloud/reva/v2/pkg/appctx"
+	"github.com/owncloud/reva/v2/pkg/conversions"
 	ctxpkg "github.com/owncloud/reva/v2/pkg/ctx"
 	"github.com/owncloud/reva/v2/pkg/errtypes"
 	"github.com/owncloud/reva/v2/pkg/rgrpc/status"
@@ -37,6 +38,14 @@ import (
 	"github.com/pkg/errors"
 )
 
+// managerPerms is the canonical Manager bitmap used to reject partial grant-management grants on space roots
+var managerPerms = conversions.NewManagerRole().CS3ResourcePermissions()
+
+// hasGrantManagementBits reports whether p carries any grant-management permission.
+func hasGrantManagementBits(p *provider.ResourcePermissions) bool {
+	return p.GetAddGrant() || p.GetUpdateGrant() || p.GetRemoveGrant() || p.GetDenyGrant()
+}
+
 // TODO(labkode): add multi-phase commit logic when commit share or commit ref is enabled.
 func (s *svc) CreateShare(ctx context.Context, req *collaboration.CreateShareRequest) (*collaboration.CreateShareResponse, error) {
 	// Don't use the share manager when sharing a space root
@@ -171,6 +180,10 @@ func (s *svc) updateSpaceShare(ctx context.Context, req *collaboration.UpdateSha
 	if req.GetShare().GetGrantee() == nil {
 		return &collaboration.UpdateShareResponse{Status: status.NewInvalid(ctx, "updating requires a received grantee object")}, nil
 	}
+	// reject partial grant-management grants that would mint a stealth manager
+	if perms := req.GetShare().GetPermissions().GetPermissions(); hasGrantManagementBits(perms) && !conversions.SufficientCS3Permissions(perms, managerPerms) {
+		return &collaboration.UpdateShareResponse{Status: status.NewInvalid(ctx, "partial grant-management permissions are not allowed on space roots")}, nil
+	}
 	// If the share is a denial we call  denyGrant instead.
 	var st *rpc.Status
 	var err error
@@ -649,6 +662,10 @@ func (s *svc) addSpaceShare(ctx context.Context, req *collaboration.CreateShareR
 		(req.GetResourceInfo().GetSpace().GetSpaceType() == _spaceTypePersonal || req.GetResourceInfo().GetSpace().GetSpaceType() == _spaceTypeVirtual) {
 		return &collaboration.CreateShareResponse{Status: status.NewInvalid(ctx, "space type is not eligible for sharing")}, nil
 	}
+	// reject partial grant-management grants that would mint a stealth manager
+	if perms := req.GetGrant().GetPermissions().GetPermissions(); hasGrantManagementBits(perms) && !conversions.SufficientCS3Permissions(perms, managerPerms) {
+		return &collaboration.CreateShareResponse{Status: status.NewInvalid(ctx, "partial grant-management permissions are not allowed on space roots")}, nil
+	}
 	// If the share is a denial we call  denyGrant instead.
 	var st *rpc.Status
 	var err error

internal/grpc/services/gateway/usershareprovider_test.go

@@ -24,6 +24,7 @@ import (
 	userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
+	"github.com/owncloud/reva/v2/pkg/conversions"
 )
 
 func userGrantee(id string) *provider.Grantee {
@@ -104,3 +105,30 @@ func TestIsSpaceManagerRemaining(t *testing.T) {
 		})
 	}
 }
+
+// spaceRootGrantRejected mirrors the guard used in both addSpaceShare and updateSpaceShare.
+func spaceRootGrantRejected(perms *provider.ResourcePermissions) bool {
+	return hasGrantManagementBits(perms) && !conversions.SufficientCS3Permissions(perms, managerPerms)
+}
+
+func TestSpaceShareRejectsPartialGrantManagement(t *testing.T) {
+	// a single grant-management bit must be rejected
+	if !spaceRootGrantRejected(&provider.ResourcePermissions{RemoveGrant: true}) {
+		t.Error("single-bit RemoveGrant grant should be rejected on a space root")
+	}
+	// read bits plus a grant-management bit (the update-path stealth-manager payload) must be rejected
+	if !spaceRootGrantRejected(&provider.ResourcePermissions{Stat: true, GetPath: true, ListGrants: true, RemoveGrant: true}) {
+		t.Error("read+RemoveGrant grant should be rejected on a space root")
+	}
+	// a full manager grant must be accepted
+	if spaceRootGrantRejected(conversions.NewManagerRole().CS3ResourcePermissions()) {
+		t.Error("full manager grant should be accepted on a space root")
+	}
+	// viewer/editor grants carry no grant-management bits and must be accepted
+	if spaceRootGrantRejected(conversions.NewSpaceViewerRole().CS3ResourcePermissions()) {
+		t.Error("space viewer grant should be accepted on a space root")
+	}
+	if spaceRootGrantRejected(conversions.NewSpaceEditorRole().CS3ResourcePermissions()) {
+		t.Error("space editor grant should be accepted on a space root")
+	}
+}

pkg/storage/utils/decomposedfs/permissions/spacepermissions.go

@@ -7,6 +7,7 @@ import (
 	cs3permissions "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1"
 	v1beta11 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
+	"github.com/owncloud/reva/v2/pkg/conversions"
 	ctxpkg "github.com/owncloud/reva/v2/pkg/ctx"
 	"github.com/owncloud/reva/v2/pkg/rgrpc/todo/pool"
 	"github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node"
@@ -144,19 +145,26 @@ func (p Permissions) checkPermission(ctx context.Context, perm string, ref *prov
 	return checkRes.Status.Code == v1beta11.Code_CODE_OK
 }
 
-// IsManager returns true if the given resource permissions evaluate the user as "manager"
-func IsManager(rp *provider.ResourcePermissions) bool {
-	return rp.RemoveGrant
+// editorPerms is the smallest space-editor bitmap; every richer space-editor variant and
+// the manager role are supersets of it, while viewers are not.
+var editorPerms = conversions.NewSpaceEditorWithoutVersionsWithoutTrashbinRole().CS3ResourcePermissions()
+
+// IsSpaceManager returns true if the given resource permissions evaluate the user as "Space manager".
+// A manager holds the full grant-management triad; a single grant-management bit is not enough.
+// DenyGrant is not required, so legitimate manager/coowner grants that predate it still classify correctly.
+func IsSpaceManager(rp *provider.ResourcePermissions) bool {
+	return rp.GetAddGrant() && rp.GetUpdateGrant() && rp.GetRemoveGrant()
 }
 
-// IsEditor returns true if the given resource permissions evaluate the user as "editor"
-func IsEditor(rp *provider.ResourcePermissions) bool {
-	return rp.InitiateFileUpload
+// IsSpaceEditor returns true if the given resource permissions evaluate the user as "Space editor"
+func IsSpaceEditor(rp *provider.ResourcePermissions) bool {
+	return conversions.SufficientCS3Permissions(rp, editorPerms)
 }
 
-// IsViewer returns true if the given resource permissions evaluate the user as "viewer"
-func IsViewer(rp *provider.ResourcePermissions) bool {
-	return rp.Stat
+// IsSpaceViewer returns true if the given resource permissions evaluate the user as "Space viewer"
+// We couldn't use NewSpaceViewerRole because it carries ListRecycle, which the WithoutTrashbin space editors lack.
+func IsSpaceViewer(rp *provider.ResourcePermissions) bool {
+	return rp.GetStat() && rp.GetGetPath() && rp.GetListGrants()
 }
 
 func spaceRef(spaceid string) *provider.Reference {

pkg/storage/utils/decomposedfs/permissions/spacepermissions_test.go

@@ -0,0 +1,106 @@
+// Copyright 2018-2021 CERN
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+// In applying this license, CERN does not waive the privileges and immunities
+// granted to it by virtue of its status as an Intergovernmental Organization
+// or submit itself to any jurisdiction.
+
+package permissions
+
+import (
+	"testing"
+
+	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
+	"github.com/owncloud/reva/v2/pkg/conversions"
+)
+
+// TestIsManagerRejectsPartialGrantManagement ensures a single grant-management bit
+// must not classify as manager, but the full triad must, without requiring DenyGrant
+// (so coowner/legacy manager grants still pass).
+func TestIsManagerRejectsPartialGrantManagement(t *testing.T) {
+	cases := []struct {
+		name string
+		rp   *provider.ResourcePermissions
+		want bool
+	}{
+		{"RemoveGrant only", &provider.ResourcePermissions{RemoveGrant: true}, false},
+		{"AddGrant only", &provider.ResourcePermissions{AddGrant: true}, false},
+		{"UpdateGrant only", &provider.ResourcePermissions{UpdateGrant: true}, false},
+		{"Add+Remove without Update", &provider.ResourcePermissions{AddGrant: true, RemoveGrant: true}, false},
+		{"full triad", &provider.ResourcePermissions{AddGrant: true, UpdateGrant: true, RemoveGrant: true}, true},
+		{"manager role", conversions.NewManagerRole().CS3ResourcePermissions(), true},
+		{"coowner role (no DenyGrant)", conversions.NewCoownerRole().CS3ResourcePermissions(), true},
+	}
+	for _, tc := range cases {
+		if got := IsSpaceManager(tc.rp); got != tc.want {
+			t.Errorf("IsSpaceManager(%s) = %v, want %v", tc.name, got, tc.want)
+		}
+	}
+}
+
+// TestIsEditorAcceptsSpaceEditorVariants ensures every space-editor variant (and manager,
+// a superset) classifies as editor, while viewers do not.
+func TestIsEditorAcceptsSpaceEditorVariants(t *testing.T) {
+	editors := map[string]*provider.ResourcePermissions{
+		"space editor":                        conversions.NewSpaceEditorRole().CS3ResourcePermissions(),
+		"space editor without versions":       conversions.NewSpaceEditorWithoutVersionsRole().CS3ResourcePermissions(),
+		"space editor without trashbin":       conversions.NewSpaceEditorWithoutTrashbinRole().CS3ResourcePermissions(),
+		"space editor without versions/trash": conversions.NewSpaceEditorWithoutVersionsWithoutTrashbinRole().CS3ResourcePermissions(),
+		"manager":                             conversions.NewManagerRole().CS3ResourcePermissions(),
+	}
+	for name, rp := range editors {
+		if !IsSpaceEditor(rp) {
+			t.Errorf("IsSpaceEditor(%s) = false, want true", name)
+		}
+	}
+
+	notEditors := map[string]*provider.ResourcePermissions{
+		"space viewer":  conversions.NewSpaceViewerRole().CS3ResourcePermissions(),
+		"secure viewer": conversions.NewSecureViewerRole().CS3ResourcePermissions(),
+	}
+	for name, rp := range notEditors {
+		if IsSpaceEditor(rp) {
+			t.Errorf("IsSpaceEditor(%s) = true, want false", name)
+		}
+	}
+}
+
+// TestIsSpaceViewerAcceptsSpaceRoleVariants ensures the space-viewer predicate accepts every
+// space role that can read (all space roles carry ListGrants), including subset editors and
+// the manager, while non-space and grant-only grants do not.
+func TestIsSpaceViewerAcceptsSpaceRoleVariants(t *testing.T) {
+	viewers := map[string]*provider.ResourcePermissions{
+		"space viewer":                        conversions.NewSpaceViewerRole().CS3ResourcePermissions(),
+		"space editor without versions/trash": conversions.NewSpaceEditorWithoutVersionsWithoutTrashbinRole().CS3ResourcePermissions(),
+		"space editor":                        conversions.NewSpaceEditorRole().CS3ResourcePermissions(),
+		"manager":                             conversions.NewManagerRole().CS3ResourcePermissions(),
+	}
+	for name, rp := range viewers {
+		if !IsSpaceViewer(rp) {
+			t.Errorf("IsSpaceViewer(%s) = false, want true", name)
+		}
+	}
+
+	// secure viewer is a file/folder role (no ListGrants) and is not a space role
+	notViewers := map[string]*provider.ResourcePermissions{
+		"secure viewer":     conversions.NewSecureViewerRole().CS3ResourcePermissions(),
+		"empty/denied":      {},
+		"remove grant only": {RemoveGrant: true},
+	}
+	for name, rp := range notViewers {
+		if IsSpaceViewer(rp) {
+			t.Errorf("IsSpaceViewer(%s) = true, want false", name)
+		}
+	}
+}

pkg/storage/utils/decomposedfs/spaces.go

@@ -632,18 +632,18 @@ func (fs *Decomposedfs) UpdateStorageSpace(ctx context.Context, req *provider.Up
 
 	}
 
-	if !restore && len(metadata) == 0 && !permissions.IsViewer(sp) {
+	if !restore && len(metadata) == 0 && !permissions.IsSpaceViewer(sp) {
 		// you may land here when making an update request without changes
 		// check if user has access to the drive before continuing
 		return &provider.UpdateStorageSpaceResponse{
 			Status: &v1beta11.Status{Code: v1beta11.Code_CODE_NOT_FOUND},
 		}, nil
 	}
 
-	if !permissions.IsManager(sp) {
+	if !permissions.IsSpaceManager(sp) {
 		// We are not a space manager. We need to check for additional permissions.
 		k := []string{prefixes.NameAttr, prefixes.SpaceDescriptionAttr}
-		if !permissions.IsEditor(sp) {
+		if !permissions.IsSpaceEditor(sp) {
 			k = append(k, prefixes.SpaceReadmeAttr, prefixes.SpaceAliasAttr, prefixes.SpaceImageAttr)
 		}
 
@@ -968,7 +968,7 @@ func (fs *Decomposedfs) StorageSpaceFromNode(ctx context.Context, n *node.Node,
 	user := ctxpkg.ContextMustGetUser(ctx)
 	if checkPermissions && n.SpaceRoot.IsDisabled(ctx) {
 		rp, err := fs.p.AssemblePermissions(ctx, n)
-		if err != nil || !permissions.IsManager(rp) {
+		if err != nil || !permissions.IsSpaceManager(rp) {
 			return nil, errtypes.PermissionDenied(fmt.Sprintf("user %s is not allowed to list deleted spaces %s", user.Username, n.ID))
 		}
 	}
@@ -1270,7 +1270,7 @@ func canDeleteSpace(ctx context.Context, spaceID string, typ string, purge bool,
 
 	// space managers are allowed to disable and delete their project spaces
 	rp, err := p.AssemblePermissions(ctx, n)
-	if err == nil && permissions.IsManager(rp) {
+	if err == nil && permissions.IsSpaceManager(rp) {
 		return nil
 	}
 

pkg/storage/utils/decomposedfs/spaces_test.go

@@ -28,6 +28,7 @@ import (
 	typesv1beta1 "github.com/cs3org/go-cs3apis/cs3/types/v1beta1"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/owncloud/reva/v2/pkg/conversions"
 	ctxpkg "github.com/owncloud/reva/v2/pkg/ctx"
 	"github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node"
 	helpers "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/testhelpers"
@@ -292,9 +293,9 @@ var _ = Describe("Spaces", func() {
 					case manager.GetId().GetOpaqueId():
 						return node.OwnerPermissions() // id of owner/admin
 					case editor.GetId().GetOpaqueId():
-						return &provider.ResourcePermissions{InitiateFileUpload: true} // mock editor
+						return conversions.NewSpaceEditorRole().CS3ResourcePermissions() // mock editor
 					case viewer.GetId().GetOpaqueId():
-						return &provider.ResourcePermissions{Stat: true} // mock viewer
+						return conversions.NewSpaceViewerRole().CS3ResourcePermissions() // mock viewer
 					default:
 						return node.NoPermissions()
 					}