Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add permission checks on homespace delete #3180

Merged
merged 4 commits into from
Sep 2, 2022
Merged

Add permission checks on homespace delete #3180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edebe0b
Add permission checks on homespace delete
dragonchaser Aug 30, 2022
af31376
Add changelog
dragonchaser Aug 30, 2022
6a33832
add delete home space test & refactor duplicate imports
dragonchaser Aug 30, 2022
42de0e7
fix linter issue
Sep 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/unreleased/check-home-space-delte-permission.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Enhancement: Add canDeleteAllHomeSpaces permission

We added a permission to the admin role in ocis that allows deleting homespaces on user delete.

https://github.com/cs3org/reva/pull/3180
https://github.com/owncloud/ocis/pull/4447/files
26 changes: 26 additions & 0 deletions pkg/storage/utils/decomposedfs/spaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,23 @@ func (fs *Decomposedfs) canListAllSpaces(ctx context.Context) bool {
return checkRes.Status.Code == v1beta11.Code_CODE_OK
}

func (fs *Decomposedfs) canDeleteAllHomeSpaces(ctx context.Context) bool {
user := ctxpkg.ContextMustGetUser(ctx)
checkRes, err := fs.permissionsClient.CheckPermission(ctx, &cs3permissions.CheckPermissionRequest{
Permission: "can-delete-all-home-spaces",
SubjectRef: &cs3permissions.SubjectReference{
Spec: &cs3permissions.SubjectReference_UserId{
UserId: user.Id,
},
},
})
if err != nil {
return false
}

return checkRes.Status.Code == v1beta11.Code_CODE_OK
}

// returns true when the user in the context can create a space / resource with storageID and nodeID set to his user opaqueID
func (fs *Decomposedfs) canCreateSpace(ctx context.Context, spaceID string) bool {
user := ctxpkg.ContextMustGetUser(ctx)
Expand Down Expand Up @@ -613,6 +630,15 @@ func (fs *Decomposedfs) DeleteStorageSpace(ctx context.Context, req *provider.De
return err
}

st, err := n.SpaceRoot.GetMetadata(xattrs.SpaceTypeAttr)
if err != nil {
return errtypes.InternalError(fmt.Sprintf("space %s does not have a spacetype, possible corrupt decompsedfs", n.ID))
}

if st == "personal" && !fs.canDeleteAllHomeSpaces(ctx) {
return errtypes.PermissionDenied(fmt.Sprintf("user is not allowed to delete home space %s", n.ID))
}

// only managers are allowed to disable or purge a drive
if err := fs.checkManagerPermission(ctx, n); err != nil {
return errtypes.PermissionDenied(fmt.Sprintf("user is not allowed to delete spaces %s", n.ID))
Expand Down
64 changes: 53 additions & 11 deletions pkg/storage/utils/decomposedfs/spaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,16 @@ package decomposedfs_test

import (
"context"
"fmt"
"os"

cs3permissions "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1"
permissionsv1beta1 "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1"
rpcv1beta1 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
ctxpkg "github.com/cs3org/reva/v2/pkg/ctx"
ruser "github.com/cs3org/reva/v2/pkg/ctx"
"github.com/cs3org/reva/v2/pkg/errtypes"
"github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs"
"github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node"
helpers "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/testhelpers"
"github.com/cs3org/reva/v2/pkg/storagespace"
. "github.com/onsi/ginkgo/v2"
Expand All @@ -51,13 +52,25 @@ var _ = Describe("Spaces", func() {
func(ctx context.Context, in *cs3permissions.CheckPermissionRequest, opts ...grpc.CallOption) *cs3permissions.CheckPermissionResponse {
if ctxpkg.ContextMustGetUser(ctx).Id.GetOpaqueId() == "25b69780-5f39-43be-a7ac-a9b9e9fe4230" {
// id of owner/admin
return &permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}
return &cs3permissions.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}
}
// id of generic user
return &permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_PERMISSION_DENIED}}
return &cs3permissions.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_PERMISSION_DENIED}}
},
nil)
env.Permissions.On("HasPermission", mock.Anything, mock.Anything, mock.Anything).Return(
func(ctx context.Context, n *node.Node, check func(*provider.ResourcePermissions) bool) bool {
return ctxpkg.ContextMustGetUser(ctx).Id.GetOpaqueId() == "25b69780-5f39-43be-a7ac-a9b9e9fe4230" // id of owner/admin
},
func(ctx context.Context, n *node.Node, check func(*provider.ResourcePermissions) bool) error {
if ctxpkg.ContextMustGetUser(ctx).Id.GetOpaqueId() == "25b69780-5f39-43be-a7ac-a9b9e9fe4230" {
// id of owner/admin
return nil
}
// id of generic user
return errtypes.PermissionDenied(fmt.Sprintf("user is not allowed to delete home space %s", n.ID))

})
})

AfterEach(func() {
Expand Down Expand Up @@ -95,12 +108,12 @@ var _ = Describe("Spaces", func() {
Expect(resp).To(Equal(true))
})
It("returns true on requesting unrestricted as non-admin", func() {
ctx := ruser.ContextSetUser(context.Background(), env.Users[0])
ctx := ctxpkg.ContextSetUser(context.Background(), env.Users[0])
resp := env.Fs.MustCheckNodePermissions(ctx, true)
Expect(resp).To(Equal(true))
})
It("returns true on requesting for own spaces", func() {
ctx := ruser.ContextSetUser(context.Background(), env.Users[0])
ctx := ctxpkg.ContextSetUser(context.Background(), env.Users[0])
resp := env.Fs.MustCheckNodePermissions(ctx, false)
Expect(resp).To(Equal(true))
})
Expand All @@ -112,7 +125,7 @@ var _ = Describe("Spaces", func() {

Context("can list spaces of requested user", func() {
It("returns false on requesting for other user as non-admin", func() {
ctx := ruser.ContextSetUser(context.Background(), env.Users[0])
ctx := ctxpkg.ContextSetUser(context.Background(), env.Users[0])
res := env.Fs.CanListSpacesOfRequestedUser(ctx, helpers.User1ID)
Expect(res).To(Equal(false))
})
Expand All @@ -126,6 +139,35 @@ var _ = Describe("Spaces", func() {
})
})

Context("can delete homespace", func() {
It("fails on trying to delete a homespace as non-admin", func() {
ctx := ctxpkg.ContextSetUser(context.Background(), env.Users[1])
resp, err := env.Fs.ListStorageSpaces(env.Ctx, nil, false)
Expect(err).ToNot(HaveOccurred())
Expect(len(resp)).To(Equal(1))
Expect(string(resp[0].Opaque.GetMap()["spaceAlias"].Value)).To(Equal("personal/username"))
Expect(resp[0].Name).To(Equal("username"))
Expect(resp[0].SpaceType).To(Equal("personal"))
err = env.Fs.DeleteStorageSpace(ctx, &provider.DeleteStorageSpaceRequest{
Id: resp[0].GetId(),
})
Expect(err).To(HaveOccurred())
})
It("succeeds on trying to delete homespace as admin", func() {
ctx := ctxpkg.ContextSetUser(context.Background(), env.Owner)
resp, err := env.Fs.ListStorageSpaces(env.Ctx, nil, false)
Expect(err).ToNot(HaveOccurred())
Expect(len(resp)).To(Equal(1))
Expect(string(resp[0].Opaque.GetMap()["spaceAlias"].Value)).To(Equal("personal/username"))
Expect(resp[0].Name).To(Equal("username"))
Expect(resp[0].SpaceType).To(Equal("personal"))
err = env.Fs.DeleteStorageSpace(ctx, &provider.DeleteStorageSpaceRequest{
Id: resp[0].GetId(),
})
Expect(err).To(Not(HaveOccurred()))
})
})

Describe("Create Spaces with custom alias template", func() {
var (
env *helpers.TestEnv
Expand All @@ -138,7 +180,7 @@ var _ = Describe("Spaces", func() {
"generalspacealias_template": "{{.SpaceType}}:{{.SpaceName | replace \" \" \"-\" | upper}}",
})
Expect(err).ToNot(HaveOccurred())
env.PermissionsClient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return(&permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}, nil)
env.PermissionsClient.On("CheckPermission", mock.Anything, mock.Anything, mock.Anything).Return(&cs3permissions.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}, nil)
})

AfterEach(func() {
Expand Down Expand Up @@ -220,10 +262,10 @@ var _ = Describe("Spaces", func() {
func(ctx context.Context, in *cs3permissions.CheckPermissionRequest, opts ...grpc.CallOption) *cs3permissions.CheckPermissionResponse {
if ctxpkg.ContextMustGetUser(ctx).Id.GetOpaqueId() == "25b69780-5f39-43be-a7ac-a9b9e9fe4230" {
// id of owner/admin
return &permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}
return &cs3permissions.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_OK}}
}
// id of generic user
return &permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_PERMISSION_DENIED}}
return &cs3permissions.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_PERMISSION_DENIED}}
},
nil)

Expand Down Expand Up @@ -252,7 +294,7 @@ var _ = Describe("Spaces", func() {
Expect(updateResp.StorageSpace.Quota.QuotaMaxBytes, uint64(1000))
})
It("try to change quota as a non admin user", func() {
ctx := ruser.ContextSetUser(context.Background(), env.Users[0])
ctx := ctxpkg.ContextSetUser(context.Background(), env.Users[0])
updateResp, err := env.Fs.UpdateStorageSpace(
ctx,
&provider.UpdateStorageSpaceRequest{
Expand Down