Merge pull request #3096 from dragonchaser/fix-user-filter

add userid matching

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

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

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

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,
 	}