Remove space members (#2524)

* remove owner from project spaces

* block removal of last space manager

The last manager of a space can not be removed.
If the user should be removed then a new manager need to be set.
After that the user can be removed.

* check node properties instead of create-space permission

* update integration tests

The integration tests needed to be updated since I removed the owner from project spaces.
Now the tests don't depend on an owner being set.

* add changelog

* add constants for space types and root node id

changelog/unreleased/remove-space-members.md

@@ -0,0 +1,8 @@
+Enhancement: add checks when removing space members
+
+- Removed owners from project spaces
+- Prevent deletion of last space manager
+- Viewers and editors can always be deleted
+- Managers can only be deleted when there will be at least one remaining
+
+https://github.com/cs3org/reva/pull/2524

internal/http/services/owncloud/ocs/conversions/role.go

@@ -40,8 +40,6 @@ const (
 	RoleEditor = "editor"
 	// RoleFileEditor grants editor permission on a single file.
 	RoleFileEditor = "file-editor"
-	// RoleCoowner grants co-owner permissions on a resource.
-	RoleCoowner = "coowner"
 	// RoleUploader grants uploader permission to upload onto a resource.
 	RoleUploader = "uploader"
 	// RoleManager grants manager permissions on a resource. Semantically equivalent to co-owner.
@@ -127,8 +125,6 @@ func RoleFromName(name string) *Role {
 		return NewEditorRole()
 	case RoleFileEditor:
 		return NewFileEditorRole()
-	case RoleCoowner:
-		return NewCoownerRole()
 	case RoleUploader:
 		return NewUploaderRole()
 	case RoleManager:
@@ -211,34 +207,6 @@ func NewFileEditorRole() *Role {
 	}
 }
 
-// NewCoownerRole creates a coowner role
-func NewCoownerRole() *Role {
-	return &Role{
-		Name: RoleCoowner,
-		cS3ResourcePermissions: &provider.ResourcePermissions{
-			GetPath:              true,
-			GetQuota:             true,
-			InitiateFileDownload: true,
-			ListGrants:           true,
-			ListContainer:        true,
-			ListFileVersions:     true,
-			ListRecycle:          true,
-			Stat:                 true,
-			InitiateFileUpload:   true,
-			RestoreFileVersion:   true,
-			RestoreRecycleItem:   true,
-			CreateContainer:      true,
-			Delete:               true,
-			Move:                 true,
-			PurgeRecycle:         true,
-			AddGrant:             true,
-			UpdateGrant:          true,
-			RemoveGrant:          true,
-		},
-		ocsPermissions: PermissionAll,
-	}
-}
-
 // NewUploaderRole creates an uploader role
 func NewUploaderRole() *Role {
 	return &Role{
@@ -254,7 +222,7 @@ func NewUploaderRole() *Role {
 	}
 }
 
-// NewManagerRole creates an editor role
+// NewManagerRole creates an manager role
 func NewManagerRole() *Role {
 	return &Role{
 		Name: RoleManager,
@@ -394,9 +362,9 @@ func RoleFromResourcePermissions(rp *provider.ResourcePermissions) *Role {
 		if r.ocsPermissions.Contain(PermissionWrite) && r.ocsPermissions.Contain(PermissionCreate) && r.ocsPermissions.Contain(PermissionDelete) {
 			r.Name = RoleEditor
 			if r.ocsPermissions.Contain(PermissionShare) {
-				r.Name = RoleCoowner
+				r.Name = RoleManager
 			}
-			return r // editor or coowner
+			return r // editor or manager
 		}
 		if r.ocsPermissions == PermissionRead {
 			r.Name = RoleViewer

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/shares.go

@@ -247,8 +247,8 @@ func (h *Handler) CreateShare(w http.ResponseWriter, r *http.Request) {
 
 	switch shareType {
 	case int(conversions.ShareTypeUser), int(conversions.ShareTypeGroup):
-		// user collaborations default to coowner
-		role, val, ocsErr := h.extractPermissions(w, r, statRes.Info, conversions.NewCoownerRole())
+		// user collaborations default to Manager (=all permissions)
+		role, val, ocsErr := h.extractPermissions(w, r, statRes.Info, conversions.NewManagerRole())
 		if ocsErr != nil {
 			response.WriteOCSError(w, r, ocsErr.Code, ocsErr.Message, ocsErr.Error)
 			return

internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/spaces.go

@@ -158,6 +158,17 @@ func (h *Handler) removeSpaceMember(w http.ResponseWriter, r *http.Request, spac
 		return
 	}
 
+	lgRes, err := providerClient.ListGrants(ctx, &provider.ListGrantsRequest{Ref: &ref})
+	if err != nil || lgRes.Status.Code != rpc.Code_CODE_OK {
+		response.WriteOCSError(w, r, response.MetaServerError.StatusCode, "error listing space grants", err)
+		return
+	}
+
+	if len(lgRes.Grants) == 1 || !isSpaceManagerRemaining(lgRes.Grants, grantee) {
+		response.WriteOCSError(w, r, http.StatusForbidden, "can't remove the last manager", nil)
+		return
+	}
+
 	removeGrantRes, err := providerClient.RemoveGrant(ctx, &provider.RemoveGrantRequest{
 		Ref: &ref,
 		Grant: &provider.Grant{
@@ -233,3 +244,35 @@ func (h *Handler) findProvider(ctx context.Context, ref *provider.Reference) (*r
 
 	return res.Providers[0], nil
 }
+
+func isSpaceManagerRemaining(grants []*provider.Grant, grantee provider.Grantee) bool {
+	for _, g := range grants {
+		// AddGrant is currently the way to check for the manager role
+		// If it is not set than the current grant is not for a manager and
+		// we can just continue with the next one.
+		if g.Permissions.AddGrant && !isEqualGrantee(*g.Grantee, grantee) {
+			return true
+		}
+	}
+	return false
+}
+
+func isEqualGrantee(a, b provider.Grantee) bool {
+	// Ideally we would want to use utils.GranteeEqual()
+	// but the grants stored in the decomposedfs aren't complete (missing usertype and idp)
+	// because of that the check would fail so we can only check the ... for now.
+	if a.Type != b.Type {
+		return false
+	}
+
+	var aID, bID string
+	switch a.Type {
+	case provider.GranteeType_GRANTEE_TYPE_GROUP:
+		aID = a.GetGroupId().OpaqueId
+		bID = b.GetGroupId().OpaqueId
+	case provider.GranteeType_GRANTEE_TYPE_USER:
+		aID = a.GetUserId().OpaqueId
+		bID = b.GetUserId().OpaqueId
+	}
+	return aID == bID
+}

pkg/storage/registry/spaces/spaces.go

@@ -194,7 +194,8 @@ func (r *registry) GetProvider(ctx context.Context, space *providerpb.StorageSpa
 				continue
 			}
 			if space.Owner != nil {
-				spacePath, err = sc.SpacePath(nil, space)
+				user := ctxpkg.ContextMustGetUser(ctx)
+				spacePath, err = sc.SpacePath(user, space)
 				if err != nil {
 					continue
 				}

pkg/storage/utils/decomposedfs/decomposedfs.go

@@ -250,7 +250,7 @@ func (fs *Decomposedfs) CreateHome(ctx context.Context) (err error) {
 	}
 
 	// add storage space
-	if err := fs.createStorageSpace(ctx, "personal", h.ID); err != nil {
+	if err := fs.createStorageSpace(ctx, spaceTypePersonal, h.ID); err != nil {
 		return err
 	}
 

pkg/storage/utils/decomposedfs/grants.go

@@ -51,15 +51,31 @@ func (fs *Decomposedfs) AddGrant(ctx context.Context, ref *provider.Reference, g
 		return
 	}
 
-	ok, err := fs.p.HasPermission(ctx, node, func(rp *provider.ResourcePermissions) bool {
-		// TODO remove AddGrant or UpdateGrant grant from CS3 api, redundant? tracked in https://github.com/cs3org/cs3apis/issues/92
-		return rp.AddGrant || rp.UpdateGrant
-	})
-	switch {
-	case err != nil:
-		return errtypes.InternalError(err.Error())
-	case !ok:
-		return errtypes.PermissionDenied(filepath.Join(node.ParentID, node.Name))
+	grantees, err := node.ListGrantees(ctx)
+	if err != nil {
+		return err
+	}
+
+	owner, err := node.Owner()
+	if err != nil {
+		return err
+	}
+
+	// If the owner is empty and there are no grantees then we are dealing with a just created project space.
+	// In this case we don't need to check for permissions and just add the grant since this will be the project
+	// manager.
+	// When the owner is empty but grants are set then we do want to check the grants.
+	if !(len(grantees) == 0 && owner.OpaqueId == "") {
+		ok, err := fs.p.HasPermission(ctx, node, func(rp *provider.ResourcePermissions) bool {
+			// TODO remove AddGrant or UpdateGrant grant from CS3 api, redundant? tracked in https://github.com/cs3org/cs3apis/issues/92
+			return rp.AddGrant || rp.UpdateGrant
+		})
+		switch {
+		case err != nil:
+			return errtypes.InternalError(err.Error())
+		case !ok:
+			return errtypes.PermissionDenied(filepath.Join(node.ParentID, node.Name))
+		}
 	}
 
 	// check lock
@@ -75,7 +91,7 @@ func (fs *Decomposedfs) AddGrant(ctx context.Context, ref *provider.Reference, g
 
 	// when a grant is added to a space, do not add a new space under "shares"
 	if spaceGrant := ctx.Value(utils.SpaceGrant); spaceGrant == nil {
-		err := fs.createStorageSpace(ctx, "share", node.ID)
+		err := fs.createStorageSpace(ctx, spaceTypeShare, node.ID)
 		if err != nil {
 			return err
 		}

pkg/storage/utils/decomposedfs/lookup.go

@@ -130,7 +130,7 @@ func (lu *Lookup) Path(ctx context.Context, n *node.Node) (p string, err error)
 
 // RootNode returns the root node of the storage
 func (lu *Lookup) RootNode(ctx context.Context) (*node.Node, error) {
-	n := node.New("root", "", "", 0, "", nil, lu)
+	n := node.New(node.RootID, "", "", 0, "", nil, lu)
 	n.Exists = true
 	return n, nil
 }

pkg/storage/utils/decomposedfs/node/node.go

@@ -64,6 +64,9 @@ const (
 
 	// TrashIDDelimiter represents the characters used to separate the nodeid and the deletion time.
 	TrashIDDelimiter = ".T."
+
+	// RootID defines the root node's ID
+	RootID = "root"
 )
 
 // Node represents a node in the tree and provides methods to get a Parent or Child instance
@@ -803,16 +806,18 @@ func (n *Node) ReadUserPermissions(ctx context.Context, u *userpb.User) (ap prov
 	o, err := n.Owner()
 	if err != nil {
 		// TODO check if a parent folder has the owner set?
-		appctx.GetLogger(ctx).Error().Err(err).Interface("node", n).Msg("could not determine owner, returning default permissions")
+		appctx.GetLogger(ctx).Error().Err(err).Str("node", n.ID).Msg("could not determine owner, returning default permissions")
 		return NoPermissions(), err
 	}
 	if o.OpaqueId == "" {
-		// this happens for root nodes in the storage. the extended attributes are set to emptystring to indicate: no owner
-		// TODO what if no owner is set but grants are present?
-		return NoOwnerPermissions(), nil
+		// this happens for root nodes and project spaces in the storage. the extended attributes are set to emptystring to indicate: no owner
+		// for project spaces we need to go over the grants and check the granted permissions
+		if n.ID == RootID {
+			return NoOwnerPermissions(), nil
+		}
 	}
 	if utils.UserEqual(u.Id, o) {
-		appctx.GetLogger(ctx).Debug().Interface("node", n).Msg("user is owner, returning owner permissions")
+		appctx.GetLogger(ctx).Debug().Str("node", n.ID).Msg("user is owner, returning owner permissions")
 		return OwnerPermissions(), nil
 	}
 

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

@@ -108,16 +108,18 @@ func (p *Permissions) AssemblePermissions(ctx context.Context, n *Node) (ap prov
 		return NoPermissions(), err
 	}
 	if o.OpaqueId == "" {
-		// this happens for root nodes in the storage. the extended attributes are set to emptystring to indicate: no owner
-		// TODO what if no owner is set but grants are present?
-		return NoOwnerPermissions(), nil
+		// this happens for root nodes and project spaces in the storage. the extended attributes are set to emptystring to indicate: no owner
+		// for project spaces we need to go over the grants and check the granted permissions
+		if n.ID == RootID {
+			return NoOwnerPermissions(), nil
+		}
 	}
 	if utils.UserEqual(u.Id, o) {
 		lp, err := n.lu.Path(ctx, n)
 		if err == nil && lp == n.lu.ShareFolder() {
 			return ShareFolderPermissions(), nil
 		}
-		appctx.GetLogger(ctx).Debug().Interface("node", n.ID).Msg("user is owner, returning owner permissions")
+		appctx.GetLogger(ctx).Debug().Str("node", n.ID).Msg("user is owner, returning owner permissions")
 		return OwnerPermissions(), nil
 	}
 	// determine root
@@ -276,13 +278,16 @@ func (p *Permissions) getUserAndPermissions(ctx context.Context, n *Node) (*user
 		return nil, &perms
 	}
 	if o.OpaqueId == "" {
-		// this happens for root nodes in the storage. the extended attributes are set to emptystring to indicate: no owner
-		// TODO what if no owner is set but grants are present?
+		// this happens for root nodes and project spaces in the storage. the extended attributes are set to emptystring to indicate: no owner
+		// for project spaces we need to go over the grants and check the granted permissions
+		if n.ID != RootID {
+			return u, nil
+		}
 		perms := NoOwnerPermissions()
 		return nil, &perms
 	}
 	if utils.UserEqual(u.Id, o) {
-		appctx.GetLogger(ctx).Debug().Interface("node", n.ID).Msg("user is owner, returning owner permissions")
+		appctx.GetLogger(ctx).Debug().Str("node", n.ID).Msg("user is owner, returning owner permissions")
 		perms := OwnerPermissions()
 		return u, &perms
 	}

pkg/storage/utils/decomposedfs/spaces.go

@@ -47,8 +47,11 @@ import (
 )
 
 const (
-	spaceTypeAny = "*"
-	spaceIDAny   = "*"
+	spaceTypePersonal = "personal"
+	spaceTypeProject  = "project"
+	spaceTypeShare    = "share"
+	spaceTypeAny      = "*"
+	spaceIDAny        = "*"
 )
 
 // CreateStorageSpace creates a storage space
@@ -76,7 +79,7 @@ func (fs *Decomposedfs) CreateStorageSpace(ctx context.Context, req *provider.Cr
 	}
 	// TODO enforce a uuid?
 	// TODO clarify if we want to enforce a single personal storage space or if we want to allow sending the spaceid
-	if req.Type == "personal" {
+	if req.Type == spaceTypePersonal {
 		spaceID = req.Owner.Id.OpaqueId
 	}
 
@@ -109,7 +112,12 @@ func (fs *Decomposedfs) CreateStorageSpace(ctx context.Context, req *provider.Cr
 		return nil, fmt.Errorf("decomposedfs: spaces: contextual user not found")
 	}
 
-	if err := n.ChangeOwner(u.Id); err != nil {
+	ownerID := u.Id
+	if req.Type == spaceTypeProject {
+		ownerID = &userv1beta1.UserId{}
+	}
+
+	if err := n.ChangeOwner(ownerID); err != nil {
 		return nil, err
 	}
 
@@ -278,7 +286,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide
 		spaceType := filepath.Base(filepath.Dir(matches[i]))
 
 		// FIXME type share evolved to grant on the edge branch ... make it configurable if the driver should support them or not for now ... ignore type share
-		if spaceType == "share" {
+		if spaceType == spaceTypeShare {
 			numShares++
 			// do not list shares as spaces for the owner
 			continue
@@ -507,7 +515,7 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De
 			return err
 		}
 
-		matches, err = filepath.Glob(filepath.Join(fs.o.Root, "nodes", "root", req.Id.OpaqueId+node.TrashIDDelimiter+"*"))
+		matches, err = filepath.Glob(filepath.Join(fs.o.Root, "nodes", node.RootID, req.Id.OpaqueId+node.TrashIDDelimiter+"*"))
 		if err != nil {
 			return err
 		}
@@ -659,8 +667,10 @@ func (fs *Decomposedfs) storageSpaceFromNode(ctx context.Context, n *node.Node,
 		}
 	}
 
-	space.Owner = &userv1beta1.User{ // FIXME only return a UserID, not a full blown user object
-		Id: owner,
+	if spaceType != spaceTypeProject && owner.OpaqueId != "" {
+		space.Owner = &userv1beta1.User{ // FIXME only return a UserID, not a full blown user object
+			Id: owner,
+		}
 	}
 
 	// we set the space mtime to the root item mtime

pkg/storage/utils/decomposedfs/tree/tree.go

@@ -111,7 +111,7 @@ func (t *Tree) Setup(owner *userpb.UserId, propagateToRoot bool) error {
 
 	// the root node has an empty name
 	// the root node has no parent
-	n := node.New("root", "", "", 0, "", nil, t.lookup)
+	n := node.New(node.RootID, "", "", 0, "", nil, t.lookup)
 	err := t.createNode(n, owner)
 	if err != nil {
 		return err
@@ -207,7 +207,7 @@ func (t *Tree) linkSpace(spaceType, spaceID, nodeID string) {
 
 func isRootNode(nodePath string) bool {
 	attr, err := xattrs.Get(nodePath, xattrs.ParentidAttr)
-	return err == nil && attr == "root"
+	return err == nil && attr == node.RootID
 }
 func isSharedNode(nodePath string) bool {
 	if attrs, err := xattr.List(nodePath); err == nil {

tests/integration/grpc/fixtures/gateway.toml

@@ -27,7 +27,7 @@ home_template = "/users/{{.Id.OpaqueId}}"
 "personal" = { "mount_point" = "/users", "path_template" = "/users/{{.Space.Owner.Id.OpaqueId}}" }
 
 [grpc.services.storageregistry.drivers.spaces.providers."{{storage2_address}}".spaces]
-"project" = { "mount_point" = "/users/[^/]+/Projects", "path_template" = "/users/{{.Space.Owner.Id.OpaqueId}}/Projects/{{.Space.Name}}" }
+"project" = { "mount_point" = "/users/[^/]+/Projects", "path_template" = "/users/{{.CurrentUser.Id.OpaqueId}}/Projects/{{.Space.Name}}" }
 
 [http]
 address = "{{grpc_address+1}}"