From edebe0b263b3203bd7b4d61f341cd0a972054dc2 Mon Sep 17 00:00:00 2001 From: Christian Richter Date: Tue, 30 Aug 2022 11:23:32 +0200 Subject: [PATCH 1/4] Add permission checks on homespace delete Co-authored-by: Ralf Haferkamp --- pkg/storage/utils/decomposedfs/spaces.go | 26 ++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go index cb2040ceb4..c42a1cee35 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/pkg/storage/utils/decomposedfs/spaces.go @@ -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) @@ -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)) From af313764996fcb3c0b9f19a6d7332d8f130a2092 Mon Sep 17 00:00:00 2001 From: Christian Richter Date: Tue, 30 Aug 2022 11:28:19 +0200 Subject: [PATCH 2/4] Add changelog Co-authored-by: Ralf Haferkamp --- changelog/unreleased/check-home-space-delte-permission.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/unreleased/check-home-space-delte-permission.md diff --git a/changelog/unreleased/check-home-space-delte-permission.md b/changelog/unreleased/check-home-space-delte-permission.md new file mode 100644 index 0000000000..4674385fe8 --- /dev/null +++ b/changelog/unreleased/check-home-space-delte-permission.md @@ -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 \ No newline at end of file From 6a33832678a7dd0de6278c581307c5a9347e762f Mon Sep 17 00:00:00 2001 From: Christian Richter Date: Tue, 30 Aug 2022 16:22:30 +0200 Subject: [PATCH 3/4] add delete home space test & refactor duplicate imports Signed-off-by: Christian Richter --- pkg/storage/utils/decomposedfs/spaces_test.go | 69 ++++++++++++++++--- 1 file changed, 58 insertions(+), 11 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/spaces_test.go b/pkg/storage/utils/decomposedfs/spaces_test.go index 7720adb7aa..1f040d4ed9 100644 --- a/pkg/storage/utils/decomposedfs/spaces_test.go +++ b/pkg/storage/utils/decomposedfs/spaces_test.go @@ -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" @@ -51,13 +52,30 @@ 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 { + if ctxpkg.ContextMustGetUser(ctx).Id.GetOpaqueId() == "25b69780-5f39-43be-a7ac-a9b9e9fe4230" { + // id of owner/admin + return true + } + // id of generic user + return false + }, + 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() { @@ -95,12 +113,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)) }) @@ -112,7 +130,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)) }) @@ -126,6 +144,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 @@ -138,7 +185,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() { @@ -220,10 +267,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) @@ -252,7 +299,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{ From 42de0e7201102a1bc952442cc1db430cd405d60c Mon Sep 17 00:00:00 2001 From: David Christofas Date: Fri, 2 Sep 2022 11:26:48 +0200 Subject: [PATCH 4/4] fix linter issue --- pkg/storage/utils/decomposedfs/spaces_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/storage/utils/decomposedfs/spaces_test.go b/pkg/storage/utils/decomposedfs/spaces_test.go index 1f040d4ed9..61968a0f21 100644 --- a/pkg/storage/utils/decomposedfs/spaces_test.go +++ b/pkg/storage/utils/decomposedfs/spaces_test.go @@ -60,12 +60,7 @@ var _ = Describe("Spaces", func() { 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 { - if ctxpkg.ContextMustGetUser(ctx).Id.GetOpaqueId() == "25b69780-5f39-43be-a7ac-a9b9e9fe4230" { - // id of owner/admin - return true - } - // id of generic user - return false + 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" {