Skip to content

Prevent space managers from purging spaces #672

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

Open
aduffeck wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Prevent space managers from purging spaces #672

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fd20845
Prevent space managers from purging spaces
aduffeck Jun 11, 2026
0f88a64
Fix linter issues
aduffeck Jun 11, 2026
143d815
Fix unit tests
aduffeck Jun 11, 2026
8932540
Fix integration test flakiness
aduffeck Jun 12, 2026
b57d336
Always set the space id attribute so the node can be identified
aduffeck Jun 12, 2026
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
4 changes: 2 additions & 2 deletions pkg/storage/pkg/decomposedfs/spaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1147,8 +1147,8 @@ func canDeleteSpace(ctx context.Context, spaceID string, typ string, purge bool,
return errtypes.PermissionDenied("user is not allowed to delete a personal space")
}

// space managers are allowed to disable and delete their project spaces
if rp, err := p.AssemblePermissions(ctx, n); err == nil && permissions.IsManager(rp) {
// space managers are allowed to disable their project spaces
if rp, err := p.AssemblePermissions(ctx, n); err == nil && !purge && permissions.IsManager(rp) {
return nil
}

Expand Down
16 changes: 5 additions & 11 deletions pkg/storage/pkg/decomposedfs/spaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,6 @@ var _ = Describe("Spaces", func() {
return &cs3permissions.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_PERMISSION_DENIED}}
},
nil)
env.Permissions.On("AssemblePermissions", mock.Anything, mock.Anything, mock.Anything).Return(func(ctx context.Context, n *node.Node) *provider.ResourcePermissions {
if ctxpkg.ContextMustGetUser(ctx).Id.GetOpaqueId() == "25b69780-5f39-43be-a7ac-a9b9e9fe4230" {
return node.OwnerPermissions() // id of owner/admin
}
return node.NoPermissions()
}, nil)
})

AfterEach(func() {
Expand All @@ -85,7 +79,6 @@ var _ = Describe("Spaces", func() {
})
Context("when creating a space", func() {
It("project space is created", func() {
env.Owner = nil
resp, err := env.Fs.CreateStorageSpace(env.Ctx, &provider.CreateStorageSpaceRequest{Name: "Mission to Mars", Type: "project"})
Expect(err).ToNot(HaveOccurred())
Expect(resp.Status.Code).To(Equal(rpcv1beta1.Code_CODE_OK))
Expand Down Expand Up @@ -198,9 +191,10 @@ var _ = Describe("Spaces", func() {
err := env.Fs.DeleteStorageSpace(ctx, delReq)
Expect(err).To(Not(HaveOccurred()))
})
It("succeeds as the space owner", func() {
err := env.Fs.DeleteStorageSpace(env.Ctx, delReq)
Expect(err).To(Not(HaveOccurred()))
It("fails as a space manager", func() {
ctx := ctxpkg.ContextSetUser(context.Background(), env.SpaceManager)
err := env.Fs.DeleteStorageSpace(ctx, delReq)
Expect(err).To(HaveOccurred())
})
})
})
Expand Down Expand Up @@ -241,7 +235,6 @@ var _ = Describe("Spaces", func() {
})
Context("creating a space", func() {
It("project space is created with custom alias", func() {
env.Owner = nil
resp, err := env.Fs.CreateStorageSpace(env.Ctx, &provider.CreateStorageSpaceRequest{Name: "Mission to Venus", Type: "project"})
Expect(err).ToNot(HaveOccurred())
Expect(resp.Status.Code).To(Equal(rpcv1beta1.Code_CODE_OK))
Expand Down Expand Up @@ -287,6 +280,7 @@ var _ = Describe("Spaces", func() {
}
}, nil)

env.Permissions.On("AssemblePermissions", mock.Anything, mock.Anything, mock.Anything).Unset()
env.Permissions.On("AssemblePermissions", mock.Anything, mock.Anything, mock.Anything).Return(
func(ctx context.Context, n *node.Node) *provider.ResourcePermissions {
switch ctxpkg.ContextMustGetUser(ctx).GetId().GetOpaqueId() {
Expand Down
46 changes: 40 additions & 6 deletions pkg/storage/pkg/decomposedfs/testhelpers/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"time"

"github.com/google/uuid"
conv "github.com/opencloud-eu/reva/v2/pkg/conversions"
ctxpkg "github.com/opencloud-eu/reva/v2/pkg/ctx"
"github.com/opencloud-eu/reva/v2/pkg/rgrpc/todo/pool"
"github.com/opencloud-eu/reva/v2/pkg/storage"
"github.com/opencloud-eu/reva/v2/pkg/storage/fs/posix/timemanager"
Expand All @@ -44,7 +46,6 @@ import (
cs3permissions "github.com/cs3org/go-cs3apis/cs3/permissions/v1beta1"
v1beta11 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1"
providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
ruser "github.com/opencloud-eu/reva/v2/pkg/ctx"
"github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs"
"github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/node"
nodemocks "github.com/opencloud-eu/reva/v2/pkg/storage/pkg/decomposedfs/node/mocks"
Expand Down Expand Up @@ -73,6 +74,7 @@ type DecomposedTestEnv struct {
Permissions *mocks.PermissionsChecker
Blobstore *nodemocks.Blobstore
Owner *userpb.User
SpaceManager *userpb.User
DeleteAllSpacesUser *userpb.User
DeleteHomeSpacesUser *userpb.User
Users []*userpb.User
Expand Down Expand Up @@ -108,6 +110,7 @@ const (
OwnerID = "25b69780-5f39-43be-a7ac-a9b9e9fe4230"
DeleteAllSpacesUserID = "39885dbc-68c0-47c0-a873-9d5e5646dceb"
DeleteHomeSpacesUserID = "ca8c6bf1-36a7-4d10-87a5-a2806566f983"
ManagerID = "c02cb356-9df3-429d-9e05-b74c14117a78"
User0ID = "824385ae-8fc6-4896-8eb2-d1d171290bd0"
User1ID = "693b0d96-80a2-4016-b53d-425ce4f66114"
)
Expand Down Expand Up @@ -169,6 +172,14 @@ func NewTestEnv(config map[string]interface{}) (*DecomposedTestEnv, error) {
},
Username: "username",
}
spaceManager := &userpb.User{
Id: &userpb.UserId{
Idp: "idp",
OpaqueId: ManagerID,
Type: userpb.UserType_USER_TYPE_PRIMARY,
},
Username: "manager",
}
users := []*userpb.User{
{
Id: &userpb.UserId{
Expand Down Expand Up @@ -220,7 +231,7 @@ func NewTestEnv(config map[string]interface{}) (*DecomposedTestEnv, error) {
if err != nil {
return nil, err
}
ctx := ruser.ContextSetUser(context.Background(), owner)
ctx := ctxpkg.ContextSetUser(context.Background(), owner)

tmpFs, _ := fs.(*decomposedfs.Decomposedfs)

Expand All @@ -232,6 +243,7 @@ func NewTestEnv(config map[string]interface{}) (*DecomposedTestEnv, error) {
Permissions: pmock,
Blobstore: bs,
Owner: owner,
SpaceManager: spaceManager,
DeleteAllSpacesUser: deleteAllSpacesUser,
DeleteHomeSpacesUser: deleteHomeSpacesUser,
Users: users,
Expand Down Expand Up @@ -325,10 +337,17 @@ func (t *DecomposedTestEnv) CreateTestStorageSpace(typ string, quota *providerv1
Status: &v1beta11.Status{Code: v1beta11.Code_CODE_OK},
}, nil)
// Permissions required for setup below
t.Permissions.On("AssemblePermissions", mock.Anything, mock.Anything, mock.Anything).Return(&providerv1beta1.ResourcePermissions{
Stat: true,
AddGrant: true,
}, nil).Times(1) //
t.Permissions.On("AssemblePermissions", mock.Anything, mock.Anything, mock.Anything).Return(
func(ctx context.Context, n *node.Node) (*providerv1beta1.ResourcePermissions, error) {
id := ctxpkg.ContextMustGetUser(ctx).Id.GetOpaqueId()
switch id {
case t.Owner.GetId().GetOpaqueId(): // owner/admin
return node.OwnerPermissions(), nil
case t.SpaceManager.GetId().GetOpaqueId(): // space manager
return conv.NewManagerRole().CS3ResourcePermissions(), nil
}
return node.NoPermissions(), nil
}, nil)

var owner *userpb.User
if typ == "personal" {
Expand All @@ -343,6 +362,21 @@ func (t *DecomposedTestEnv) CreateTestStorageSpace(typ string, quota *providerv1
return nil, err
}

err = t.Fs.AddGrant(t.Ctx, &providerv1beta1.Reference{
ResourceId: space.StorageSpace.Root,
}, &providerv1beta1.Grant{
Grantee: &providerv1beta1.Grantee{
Type: providerv1beta1.GranteeType_GRANTEE_TYPE_USER,
Id: &providerv1beta1.Grantee_UserId{
UserId: t.SpaceManager.Id,
},
},
Permissions: conv.NewManagerRole().CS3ResourcePermissions(),
})
if err != nil {
return nil, err
}

ref := buildRef(space.StorageSpace.Id.OpaqueId, "")

// the space name attribute is the stop condition in the lookup
Expand Down