From df1264deff5889c8f83f09b5d5e8bff17d8312b9 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Tue, 15 Feb 2022 14:08:02 +0100 Subject: [PATCH] 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 | 8 ++++ .../services/owncloud/ocs/conversions/role.go | 38 ++-------------- .../handlers/apps/sharing/shares/shares.go | 4 +- .../handlers/apps/sharing/shares/spaces.go | 43 +++++++++++++++++++ pkg/storage/registry/spaces/spaces.go | 3 +- .../utils/decomposedfs/decomposedfs.go | 2 +- pkg/storage/utils/decomposedfs/grants.go | 36 +++++++++++----- pkg/storage/utils/decomposedfs/lookup.go | 2 +- pkg/storage/utils/decomposedfs/node/node.go | 15 ++++--- .../utils/decomposedfs/node/permissions.go | 19 +++++--- pkg/storage/utils/decomposedfs/spaces.go | 26 +++++++---- pkg/storage/utils/decomposedfs/tree/tree.go | 4 +- tests/integration/grpc/fixtures/gateway.toml | 2 +- .../grpc/gateway_storageprovider_test.go | 1 - 14 files changed, 129 insertions(+), 74 deletions(-) create mode 100644 changelog/unreleased/remove-space-members.md diff --git a/changelog/unreleased/remove-space-members.md b/changelog/unreleased/remove-space-members.md new file mode 100644 index 0000000000..b294f1fdf2 --- /dev/null +++ b/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 diff --git a/internal/http/services/owncloud/ocs/conversions/role.go b/internal/http/services/owncloud/ocs/conversions/role.go index 9440756040..6d0e786b29 100644 --- a/internal/http/services/owncloud/ocs/conversions/role.go +++ b/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 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 ab98033537..f5a7166310 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 @@ -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 diff --git a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/spaces.go b/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/spaces.go index 0d7945dbc1..5f3a5b5871 100644 --- a/internal/http/services/owncloud/ocs/handlers/apps/sharing/shares/spaces.go +++ b/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 +} diff --git a/pkg/storage/registry/spaces/spaces.go b/pkg/storage/registry/spaces/spaces.go index a1fa633597..9b5eca2945 100644 --- a/pkg/storage/registry/spaces/spaces.go +++ b/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 } diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index 4dd5b313f7..7c7c0dce3a 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/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 } diff --git a/pkg/storage/utils/decomposedfs/grants.go b/pkg/storage/utils/decomposedfs/grants.go index 71a11aef1c..923a4478a1 100644 --- a/pkg/storage/utils/decomposedfs/grants.go +++ b/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 } diff --git a/pkg/storage/utils/decomposedfs/lookup.go b/pkg/storage/utils/decomposedfs/lookup.go index 70a8daa596..06017e96a2 100644 --- a/pkg/storage/utils/decomposedfs/lookup.go +++ b/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 } diff --git a/pkg/storage/utils/decomposedfs/node/node.go b/pkg/storage/utils/decomposedfs/node/node.go index dfa428c46e..cc4e5bcad9 100644 --- a/pkg/storage/utils/decomposedfs/node/node.go +++ b/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 } diff --git a/pkg/storage/utils/decomposedfs/node/permissions.go b/pkg/storage/utils/decomposedfs/node/permissions.go index 24d06ef976..ec0f8c8789 100644 --- a/pkg/storage/utils/decomposedfs/node/permissions.go +++ b/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 } diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go index 4d5d642bb0..a96a9a57be 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/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 diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index ef94fe30c3..3a65789614 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/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 { diff --git a/tests/integration/grpc/fixtures/gateway.toml b/tests/integration/grpc/fixtures/gateway.toml index 1aba40164f..f7512a0ee6 100644 --- a/tests/integration/grpc/fixtures/gateway.toml +++ b/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}}" diff --git a/tests/integration/grpc/gateway_storageprovider_test.go b/tests/integration/grpc/gateway_storageprovider_test.go index 8265e1c1e8..07bd12ecc3 100644 --- a/tests/integration/grpc/gateway_storageprovider_test.go +++ b/tests/integration/grpc/gateway_storageprovider_test.go @@ -495,7 +495,6 @@ var _ = Describe("gateway", func() { info := statRes.Info Expect(info.Type).To(Equal(storagep.ResourceType_RESOURCE_TYPE_CONTAINER)) Expect(info.Path).To(Equal(embeddedSpaceID)) - Expect(info.Owner.OpaqueId).To(Equal(user.Id.OpaqueId)) Expect(info.Size).To(Equal(uint64(2))) })