diff --git a/changelog/unreleased/fix-user-filter.md b/changelog/unreleased/fix-user-filter.md new file mode 100644 index 0000000000..7e427c306d --- /dev/null +++ b/changelog/unreleased/fix-user-filter.md @@ -0,0 +1,5 @@ +Bugfix: Fix user filter + +We fixed the user filter to display the users drives properly and allow admins to list other users drives. + +https://github.com/cs3org/reva/pull/3096 diff --git a/pkg/storage/utils/decomposedfs/spaces.go b/pkg/storage/utils/decomposedfs/spaces.go index 7e93404b90..9ffedc99f8 100644 --- a/pkg/storage/utils/decomposedfs/spaces.go +++ b/pkg/storage/utils/decomposedfs/spaces.go @@ -55,6 +55,7 @@ const ( spaceTypeShare = "share" spaceTypeAny = "*" spaceIDAny = "*" + userIDAny = "*" ) // CreateStorageSpace creates a storage space @@ -244,9 +245,9 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide // we would not need /nodes/root if access always happened via spaceid+relative path var ( - spaceID = spaceIDAny - nodeID = spaceIDAny - userID = spaceIDAny + spaceID = spaceIDAny + nodeID = spaceIDAny + requestedUserID = userIDAny ) spaceTypes := map[string]struct{}{} @@ -269,20 +270,20 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide } case provider.ListStorageSpacesRequest_Filter_TYPE_USER: // TODO: refactor this to GetUserId() in cs3 - userID = filter[i].GetUser().GetOpaqueId() + requestedUserID = filter[i].GetUser().GetOpaqueId() } } if len(spaceTypes) == 0 { spaceTypes[spaceTypeAny] = struct{}{} } - canListAllSpaces := fs.canListAllSpaces(ctx) + authenticatedUserID := ctxpkg.ContextMustGetUser(ctx).GetId().GetOpaqueId() - if userID != spaceTypeAny && !canListAllSpaces { - return nil, errtypes.PermissionDenied(fmt.Sprintf("user %s is not allowed to list spaces of other users", ctxpkg.ContextMustGetUser(ctx).GetId().GetOpaqueId())) + if !fs.CanListSpacesOfRequestedUser(ctx, requestedUserID) { + return nil, errtypes.PermissionDenied(fmt.Sprintf("user %s is not allowed to list spaces of other users", authenticatedUserID)) } - checkNodePermissions := !canListAllSpaces || !unrestricted + checkNodePermissions := fs.MustCheckNodePermissions(ctx, requestedUserID, unrestricted) spaces := []*provider.StorageSpace{} // build the glob path, eg. @@ -317,8 +318,8 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide matches := map[string]struct{}{} - if userID != spaceTypeAny { - path := filepath.Join(fs.o.Root, "indexes", "by-user-id", userID, nodeID) + if requestedUserID != userIDAny { + path := filepath.Join(fs.o.Root, "indexes", "by-user-id", requestedUserID, nodeID) m, err := filepath.Glob(path) if err != nil { return nil, err @@ -332,7 +333,7 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide } } - if userID == spaceTypeAny { + if requestedUserID == userIDAny { for spaceType := range spaceTypes { path := filepath.Join(fs.o.Root, "indexes", "by-type", spaceType, nodeID) m, err := filepath.Glob(path) @@ -427,6 +428,34 @@ func (fs *Decomposedfs) ListStorageSpaces(ctx context.Context, filter []*provide } +// MustCheckNodePermissions checks if permission checks are needed to be performed when user requests spaces +func (fs *Decomposedfs) MustCheckNodePermissions(ctx context.Context, requestedUserID string, unrestricted bool) bool { + canListAllSpaces := fs.canListAllSpaces(ctx) + switch { + case (canListAllSpaces && requestedUserID == userIDAny): + // admins should not see any other spaces when they request their own, without settings filters + return true + case canListAllSpaces, unrestricted: + return false + } + return true +} + +// CanListSpacesOfRequestedUser checks if user is allowed to list spaces of another user +func (fs *Decomposedfs) CanListSpacesOfRequestedUser(ctx context.Context, requestedUserID string) bool { + authenticatedUserID := ctxpkg.ContextMustGetUser(ctx).GetId().GetOpaqueId() + switch { + case requestedUserID == userIDAny: + // there is no filter + return true + case requestedUserID == authenticatedUserID: + return true + case fs.canListAllSpaces(ctx): + return true + } + return false +} + // UpdateStorageSpace updates a storage space func (fs *Decomposedfs) UpdateStorageSpace(ctx context.Context, req *provider.UpdateStorageSpaceRequest) (*provider.UpdateStorageSpaceResponse, error) { var restore bool diff --git a/pkg/storage/utils/decomposedfs/spaces_test.go b/pkg/storage/utils/decomposedfs/spaces_test.go index 9841eeb9f1..53835897fc 100644 --- a/pkg/storage/utils/decomposedfs/spaces_test.go +++ b/pkg/storage/utils/decomposedfs/spaces_test.go @@ -19,16 +19,21 @@ package decomposedfs_test import ( + "context" "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/storage/utils/decomposedfs" helpers "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/testhelpers" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/stretchr/testify/mock" + "google.golang.org/grpc" ) var _ = Describe("Spaces", func() { @@ -41,7 +46,17 @@ var _ = Describe("Spaces", func() { var err error env, err = helpers.NewTestEnv(nil) 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( + 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}} + } + // id of generic user + return &permissionsv1beta1.CheckPermissionResponse{Status: &rpcv1beta1.Status{Code: rpcv1beta1.Code_CODE_PERMISSION_DENIED}} + }, + nil) + }) AfterEach(func() { @@ -71,6 +86,44 @@ var _ = Describe("Spaces", func() { Expect(resp.StorageSpace.SpaceType).To(Equal("project")) }) }) + + Context("needs to check node permissions", func() { + It("returns true on requesting for other user as non-admin", func() { + ctx := ruser.ContextSetUser(context.Background(), env.Users[0]) + resp := env.Fs.MustCheckNodePermissions(ctx, helpers.User1ID, false) + Expect(resp).To(Equal(true)) + }) + It("returns false on requesting for other user as admin", func() { + resp := env.Fs.MustCheckNodePermissions(env.Ctx, helpers.User0ID, false) + Expect(resp).To(Equal(false)) + }) + It("returns true on requesting for own spaces", func() { + ctx := ruser.ContextSetUser(context.Background(), env.Users[0]) + resp := env.Fs.MustCheckNodePermissions(ctx, helpers.User0ID, false) + Expect(resp).To(Equal(true)) + }) + It("returns false on unrestricted", func() { + resp := env.Fs.MustCheckNodePermissions(env.Ctx, "some-uuid-that-does-not-make-sense", true) + Expect(resp).To(Equal(false)) + }) + }) + + 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]) + res := env.Fs.CanListSpacesOfRequestedUser(ctx, helpers.User1ID) + Expect(res).To(Equal(false)) + }) + It("returns true on requesting for other user as admin", func() { + res := env.Fs.CanListSpacesOfRequestedUser(env.Ctx, helpers.User0ID) + Expect(res).To(Equal(true)) + }) + It("returns true on requesting for own spaces", func() { + res := env.Fs.CanListSpacesOfRequestedUser(env.Ctx, helpers.OwnerID) + Expect(res).To(Equal(true)) + }) + }) + Describe("Create Spaces with custom alias template", func() { var ( env *helpers.TestEnv diff --git a/pkg/storage/utils/decomposedfs/testhelpers/helpers.go b/pkg/storage/utils/decomposedfs/testhelpers/helpers.go index ee4f9baeba..750f443c5e 100644 --- a/pkg/storage/utils/decomposedfs/testhelpers/helpers.go +++ b/pkg/storage/utils/decomposedfs/testhelpers/helpers.go @@ -35,7 +35,6 @@ import ( v1beta11 "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" providerv1beta1 "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" ruser "github.com/cs3org/reva/v2/pkg/ctx" - "github.com/cs3org/reva/v2/pkg/storage" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/mocks" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node" @@ -48,17 +47,25 @@ import ( // TestEnv represents a test environment for unit tests type TestEnv struct { Root string - Fs storage.FS + Fs *decomposedfs.Decomposedfs Tree *tree.Tree Permissions *mocks.PermissionsChecker Blobstore *treemocks.Blobstore Owner *userpb.User + Users []*userpb.User Lookup *lookup.Lookup Ctx context.Context SpaceRootRes *providerv1beta1.ResourceId PermissionsClient *mocks.CS3PermissionsClient } +// Constant UUIDs for the space users +const ( + OwnerID = "25b69780-5f39-43be-a7ac-a9b9e9fe4230" + User0ID = "824385ae-8fc6-4896-8eb2-d1d171290bd0" + User1ID = "693b0d96-80a2-4016-b53d-425ce4f66114" +) + // NewTestEnv prepares a test environment on disk // The storage contains some directories and a file: // @@ -93,11 +100,27 @@ func NewTestEnv(config map[string]interface{}) (*TestEnv, error) { owner := &userpb.User{ Id: &userpb.UserId{ Idp: "idp", - OpaqueId: "25b69780-5f39-43be-a7ac-a9b9e9fe4230", + OpaqueId: OwnerID, Type: userpb.UserType_USER_TYPE_PRIMARY, }, Username: "username", } + users := []*userpb.User{ + { + Id: &userpb.UserId{ + Idp: "idp", + OpaqueId: User0ID, + Type: userpb.UserType_USER_TYPE_PRIMARY, + }, + }, + { + Id: &userpb.UserId{ + Idp: "idp", + OpaqueId: User1ID, + Type: userpb.UserType_USER_TYPE_PRIMARY, + }, + }, + } lookup := &lookup.Lookup{Options: o} permissions := &mocks.PermissionsChecker{} cs3permissionsclient := &mocks.CS3PermissionsClient{} @@ -109,14 +132,17 @@ func NewTestEnv(config map[string]interface{}) (*TestEnv, error) { } ctx := ruser.ContextSetUser(context.Background(), owner) + tmpFs, _ := fs.(*decomposedfs.Decomposedfs) + env := &TestEnv{ Root: tmpRoot, - Fs: fs, + Fs: tmpFs, Tree: tree, Lookup: lookup, Permissions: permissions, Blobstore: bs, Owner: owner, + Users: users, Ctx: ctx, PermissionsClient: cs3permissionsclient, }