From 5a7fb97e07a7c1d1952ac18511c8c2e42d05b5d0 Mon Sep 17 00:00:00 2001 From: SimFG Date: Thu, 14 Dec 2023 16:20:43 +0800 Subject: [PATCH] fix: missing the grant info when using the SelectGrant api with the default db (#29173) issue: #29172 Signed-off-by: SimFG --- internal/metastore/kv/rootcoord/kv_catalog.go | 10 ++++- .../metastore/kv/rootcoord/kv_catalog_test.go | 45 ++++++++++++++----- internal/rootcoord/root_coord.go | 3 +- internal/rootcoord/root_coord_test.go | 33 ++++++++++++++ 4 files changed, 78 insertions(+), 13 deletions(-) diff --git a/internal/metastore/kv/rootcoord/kv_catalog.go b/internal/metastore/kv/rootcoord/kv_catalog.go index 0d20a4bc85..5490d9f361 100644 --- a/internal/metastore/kv/rootcoord/kv_catalog.go +++ b/internal/metastore/kv/rootcoord/kv_catalog.go @@ -1024,7 +1024,7 @@ func (kc *Catalog) ListGrant(ctx context.Context, tenant string, entity *milvusp appendGrantEntity := func(v string, object string, objectName string) error { dbName := "" dbName, objectName = funcutil.SplitObjectName(objectName) - if dbName != entity.DbName { + if dbName != entity.DbName && dbName != util.AnyWord { return nil } granteeIDKey := funcutil.HandleTenantForEtcdKey(GranteeIDPrefix, tenant, v) @@ -1069,6 +1069,14 @@ func (kc *Catalog) ListGrant(ctx context.Context, tenant string, entity *milvusp } } + if entity.DbName != util.AnyWord { + granteeKey = funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, fmt.Sprintf("%s/%s/%s", entity.Role.Name, entity.Object.Name, funcutil.CombineObjectName(util.AnyWord, entity.ObjectName))) + v, err := kc.Txn.Load(granteeKey) + if err == nil { + _ = appendGrantEntity(v, entity.Object.Name, funcutil.CombineObjectName(util.AnyWord, entity.ObjectName)) + } + } + granteeKey = funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, fmt.Sprintf("%s/%s/%s", entity.Role.Name, entity.Object.Name, funcutil.CombineObjectName(entity.DbName, entity.ObjectName))) v, err := kc.Txn.Load(granteeKey) if err != nil { diff --git a/internal/metastore/kv/rootcoord/kv_catalog_test.go b/internal/metastore/kv/rootcoord/kv_catalog_test.go index c6e4da3b84..72d8cee203 100644 --- a/internal/metastore/kv/rootcoord/kv_catalog_test.go +++ b/internal/metastore/kv/rootcoord/kv_catalog_test.go @@ -2317,9 +2317,14 @@ func TestRBAC_Grant(t *testing.T) { kvmock.EXPECT().Load(validGranteeKey).Call. Return(func(key string) string { return crypto.MD5(key) }, nil) validGranteeKey2 := funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, - fmt.Sprintf("%s/%s/%s", "role1", "obj1", "foo.obj_name2")) + fmt.Sprintf("%s/%s/%s", "role1", "obj2", "foo.obj_name2")) kvmock.EXPECT().Load(validGranteeKey2).Call. Return(func(key string) string { return crypto.MD5(key) }, nil) + validGranteeKey3 := funcutil.HandleTenantForEtcdKey(GranteePrefix, tenant, + fmt.Sprintf("%s/%s/%s", "role1", "obj3", "*.obj_name3")) + kvmock.EXPECT().Load(validGranteeKey3).Call. + Return(func(key string) string { return crypto.MD5(key) }, nil) + kvmock.EXPECT().Load(mock.Anything).Call. Return("", errors.New("mock Load error")) @@ -2338,7 +2343,8 @@ func TestRBAC_Grant(t *testing.T) { // Mock kv_catalog.go:ListGrant:L912 return []string{ fmt.Sprintf("%s/%s", key, "obj1/obj_name1"), - fmt.Sprintf("%s/%s", key, "obj2/obj_name2"), + fmt.Sprintf("%s/%s", key, "obj2/foo.obj_name2"), + fmt.Sprintf("%s/%s", key, "obj3/*.obj_name3"), } }, func(key string) []string { @@ -2347,7 +2353,8 @@ func TestRBAC_Grant(t *testing.T) { } return []string{ crypto.MD5(fmt.Sprintf("%s/%s", key, "obj1/obj_name1")), - crypto.MD5(fmt.Sprintf("%s/%s", key, "obj2/obj_name2")), + crypto.MD5(fmt.Sprintf("%s/%s", key, "obj2/foo.obj_name2")), + crypto.MD5(fmt.Sprintf("%s/%s", key, "obj3/*.obj_name3")), } }, nil, @@ -2358,31 +2365,46 @@ func TestRBAC_Grant(t *testing.T) { entity *milvuspb.GrantEntity description string + count int }{ - {true, &milvuspb.GrantEntity{Role: &milvuspb.RoleEntity{Name: "role1"}}, "valid role role1 with empty entity"}, - {false, &milvuspb.GrantEntity{Role: &milvuspb.RoleEntity{Name: invalidRole}}, "invalid role with empty entity"}, + {true, &milvuspb.GrantEntity{Role: &milvuspb.RoleEntity{Name: "role1"}}, "valid role role1 with empty entity", 4}, + {false, &milvuspb.GrantEntity{Role: &milvuspb.RoleEntity{Name: invalidRole}}, "invalid role with empty entity", 0}, {false, &milvuspb.GrantEntity{ Object: &milvuspb.ObjectEntity{Name: "random"}, ObjectName: "random2", Role: &milvuspb.RoleEntity{Name: "role1"}, - }, "valid role with not exist entity"}, + }, "valid role with not exist entity", 0}, {true, &milvuspb.GrantEntity{ Object: &milvuspb.ObjectEntity{Name: "obj1"}, ObjectName: "obj_name1", Role: &milvuspb.RoleEntity{Name: "role1"}, - }, "valid role with valid entity"}, + }, "valid role with valid entity", 2}, {true, &milvuspb.GrantEntity{ - Object: &milvuspb.ObjectEntity{Name: "obj1"}, + Object: &milvuspb.ObjectEntity{Name: "obj2"}, ObjectName: "obj_name2", DbName: "foo", Role: &milvuspb.RoleEntity{Name: "role1"}, - }, "valid role and dbName with valid entity"}, + }, "valid role and dbName with valid entity", 2}, {false, &milvuspb.GrantEntity{ - Object: &milvuspb.ObjectEntity{Name: "obj1"}, + Object: &milvuspb.ObjectEntity{Name: "obj2"}, ObjectName: "obj_name2", DbName: "foo2", Role: &milvuspb.RoleEntity{Name: "role1"}, - }, "valid role and invalid dbName with valid entity"}, + }, "valid role and invalid dbName with valid entity", 0}, + {false, &milvuspb.GrantEntity{ + Object: &milvuspb.ObjectEntity{Name: "obj3"}, + ObjectName: "obj_name3", + DbName: "default", + Role: &milvuspb.RoleEntity{Name: "role1"}, + }, "valid role and dbName with default db", 2}, + {true, &milvuspb.GrantEntity{ + DbName: "default", + Role: &milvuspb.RoleEntity{Name: "role1"}, + }, "valid role and default dbName without object", 4}, + {true, &milvuspb.GrantEntity{ + DbName: "*", + Role: &milvuspb.RoleEntity{Name: "role1"}, + }, "valid role and any dbName without object", 2}, } for _, test := range tests { @@ -2399,6 +2421,7 @@ func TestRBAC_Grant(t *testing.T) { } else { assert.Error(t, err) } + assert.Equal(t, test.count, len(grants)) }) } }) diff --git a/internal/rootcoord/root_coord.go b/internal/rootcoord/root_coord.go index 29da7f4efd..f093d0f1f9 100644 --- a/internal/rootcoord/root_coord.go +++ b/internal/rootcoord/root_coord.go @@ -2674,7 +2674,8 @@ func (c *Core) SelectGrant(ctx context.Context, in *milvuspb.SelectGrantRequest) grantEntities, err := c.meta.SelectGrant(util.DefaultTenant, in.Entity) if errors.Is(err, merr.ErrIoKeyNotFound) { return &milvuspb.SelectGrantResponse{ - Status: merr.Success(), + Status: merr.Success(), + Entities: grantEntities, }, nil } if err != nil { diff --git a/internal/rootcoord/root_coord_test.go b/internal/rootcoord/root_coord_test.go index 1086879cf5..29cd645318 100644 --- a/internal/rootcoord/root_coord_test.go +++ b/internal/rootcoord/root_coord_test.go @@ -2036,6 +2036,39 @@ func TestRootCoord_RBACError(t *testing.T) { } }) + t.Run("select grant success", func(t *testing.T) { + mockMeta := c.meta.(*mockMetaTable) + mockMeta.SelectRoleFunc = func(tenant string, entity *milvuspb.RoleEntity, includeUserInfo bool) ([]*milvuspb.RoleResult, error) { + return []*milvuspb.RoleResult{ + { + Role: &milvuspb.RoleEntity{Name: "foo"}, + }, + }, nil + } + mockMeta.SelectGrantFunc = func(tenant string, entity *milvuspb.GrantEntity) ([]*milvuspb.GrantEntity, error) { + return []*milvuspb.GrantEntity{ + { + Role: &milvuspb.RoleEntity{Name: "foo"}, + }, + }, merr.ErrIoKeyNotFound + } + + { + resp, err := c.SelectGrant(ctx, &milvuspb.SelectGrantRequest{Entity: &milvuspb.GrantEntity{Role: &milvuspb.RoleEntity{Name: "foo"}, Object: &milvuspb.ObjectEntity{Name: "Collection"}, ObjectName: "fir"}}) + assert.NoError(t, err) + assert.Equal(t, 1, len(resp.GetEntities())) + assert.Equal(t, commonpb.ErrorCode_Success, resp.GetStatus().GetErrorCode()) + } + + mockMeta.SelectRoleFunc = func(tenant string, entity *milvuspb.RoleEntity, includeUserInfo bool) ([]*milvuspb.RoleResult, error) { + return nil, errors.New("mock error") + } + + mockMeta.SelectGrantFunc = func(tenant string, entity *milvuspb.GrantEntity) ([]*milvuspb.GrantEntity, error) { + return nil, errors.New("mock error") + } + }) + t.Run("list policy failed", func(t *testing.T) { resp, err := c.ListPolicy(ctx, &internalpb.ListPolicyRequest{}) assert.NoError(t, err)