Merge branch 'edge' into nodes-per-space

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

@@ -137,7 +137,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(node.NoSpaceID, "root", "", "", 0, "", nil, lu)
+ n := node.New(node.NoSpaceID, node.RootID, "", "", 0, "", nil, lu)
  n.Exists = true
  return n, nil
 }

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

@@ -65,6 +65,9 @@ const (
  // TrashIDDelimiter represents the characters used to separate the nodeid and the deletion time.
  TrashIDDelimiter = ".T."
  NoSpaceID = ""
+
+ // 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
@@ -833,16 +836,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

@@ -112,7 +112,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(node.NoSpaceID, "root", "", "", 0, "", nil, t.lookup)
+ n := node.New(node.NoSpaceID, node.RootID, "", "", 0, "", nil, t.lookup)
  err := t.createNode(n, owner)
  if err != nil {
  return err
@@ -208,7 +208,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}}"